Image Sequences

[dukesook]

I would like to provide feedback as the sequence branch gets developed.

[dukesook]

The items/properties API is excellent for general purpose reading/writing of that data. Currently, they only work with the file-level meta box.

The introduction of image sequences would allow for multiple meta boxes (possibly one per track). Would it be possible to extend the items/properties API to specify a particular meta box?

[dukesook]

I see, thank you for clarifying.

Instead of heif_image_set_gimi_content_id(), I would prefer to use the name as listed in tables 10 & 11 of the GIMI document: SampleContentID. So something like heif_image_set_sample_content_id(). This also helps to clarify which of the 6 content id types from tables 10 & 11 the function is referring to.

As a side note, we're also making an effort to officially register the 'suid' 4cc aux_info_type through MPEG.

[farindk]

Good idea. I am fine to use the Table 10 names. I would still like to prefix them with gimi because I can imagine that some time in the future someone else comes up with his own "content ID" with a different urn and then we would have a confusion.
That would then be heif_image_set_gimi_sample_content_id().

[dukesook]

I can imagine that some time in the future someone else comes up with his own "content ID" with a different urn

This reminds me, there actually was a recent update to the GIMI document:

Table 9: Syntax Options for Content IDs

Scope of Use	Defining Standard	Form of Content ID
Applications requiring the use of IC_IDs	ODNI XML Data Encoding Specification for Intelligence Community Identifier (IC-ID.XML)	General form of Content ID: 'guide://prefix/' Ex: 'guide://999999/fba91974-fdf8-43af-ae52-e3783f4985e8'
Others	ISO/IEC 9834-8	General form of Content ID: 'urn:uuid:' Ex: 'urn:uuid:47df9863-6731-4115-bec2-2073a8da3420'

In summary, the "guide://" content id is specific to GIMI while the "urn:uuid" is an ISO standardized form. If this doesn't look familiar, then let's make sure you get the latest version of the document.

[farindk]

I see that table in version 1.0 of the document. If I understand the document correctly, each of the content IDs can contain either a UUID ID (with the 'urn:uuid:') prefix, or a IC_ID with the 'guide://' prefix. I.e. this is what is stored in the ID, not used as ID type specifier, right?

Probably this is also the reason why GIMI v0.6 used raw 16 byte UUIDs, while v1.0 switch to strings to hold both variants, right?

[dukesook]

Okay good, it seems like you have the latest version.

Yes, that sounds correct. I also believe with raw 16 byte content ids, there was a concern that the probability of a collision was too high.

In any case, heif_image_set_gimi_sample_content_id() sounds like a good name.

[dukesook]

So far, the heif_sequences.h API is looking very good. I'm surprised you did so much in such a short period of time. This will be a very useful capability.

[dukesook]

Regarding:

struct heif_raw_sequence_sample
{
...
  heif_tai_timestamp_packet* timestamp = nullptr;
  std::string gimi_sample_content_id;
};

I would like a more general capability for adding saiz/saio auxiliary data. I anticipate adding many other types of per sample data, all of which could be handled in the same way.

The user could either pass the aux data as raw bytes, but then it would get complicated at client side since each aux packet would have to be encoded and decoded on client side.

Yes, this would add complexity on the client side, but I don't see that as an issue.

[farindk]

Removing things from previous APIs cannot be done without a major, breaking, release. But we can keep this in heif_track_info as a shortcut to a future, more general API.
I spent some time trying to eliminate this from heif_track_info but then decided that it is not a big deal to keep it. That opinion may change, but for the moment, I think it is the simplest solution and if anyone does not need it, he can just ignore those fields.

[farindk]

I have now removed heif_track_info and replaced it with a heif_track_builder. I think that this is a more flexible interface for future extensions. Some adaptations might be needed in your existing code.

[dukesook]

If the sample_duration isn't set, then I think heif_track_encode_sequence_image() should return heif_error_Usage_error. Or at least have some non-zero default value.

[farindk]

I have renamed it again... (c6b6340) heif_track_builder is now heif_track_options. I think that name fits better.

[dukesook]

I agree, I like the name change.

[dukesook]

Don't forget to assign the image duration to the image with heif_image_set_duration()

I would like the track to be valid even if heif_image_set_duration() doesn't get called. Can we have a non-zero default value for m_sample_duration? Even if the video looks too slow or fast, it should at least play.

If it's truly required for every sample, then perhaps heif_track_encode_sequence_image() should take the duration as a required parameter.

[farindk]

Right. Using sample_duration=1 would be an option, but every application will probably have to set this anyway to get the desired frame-rate in all cases. Thus I tend to return an error if it is not set.

[farindk]

Implemented in 2330269

[dukesook]

I called heif_track_options_set_gimi_track_id() and it looks like the infe item was placed directly inside the meta box instead of the iinf box.

[farindk]

Right, the iinf seems to be missing. Good catch. I will change that.

[farindk]

Changed in 64bef02 and e77df31 for reading

[dukesook]

Out of curiosity, what options do you anticipate putting into heif_sequence_encoding_options?

Between heif_track_options and heif_encoding_options, most configurations seem to be covered. Does the heif_context_add_visual_sequence_track() function need all three structs?

[farindk]

heif_encoding_options will go away as this contains many things only related to images. The required parts will be moved to heif_sequence_encoding_options.

[farindk]

-> commit 45afa76

[dukesook]

Regarding heif_track_options_enable_gimi_content_ids(), if I'm not mistaken, this applies specifically to sample content ids. Perhaps the name should include the word sample? So: heif_track_options_enable_gimi_sample_content_ids()

[farindk]

Fine with me. But then we should also change heif_track_options_enable_tai_timestamps().

[dukesook]

I noticed in track.cc that the gimi sample content id implementation is still under HEIF_ENABLE_EXPERIMENTAL_FEATURES. Was it your intention to remove this? The corresponding API functions are no longer experimental, but still require the experimental flag to work.

[farindk]

86a7d14. I hope I now catched all of them.

[farindk]

If you don't have any issues anymore, I will now release v1.20.0.

[dukesook]

The API looks great. I agree it's likely time to release v1.20.0.