Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Empty HandlerBox name instead of "libavif" #2378

Merged
merged 6 commits into from
Aug 22, 2024
Merged

Conversation

y-guyon
Copy link
Collaborator

@y-guyon y-guyon commented Aug 9, 2024

Empty HandlerBox name by default.

@@ -216,6 +216,8 @@ typedef enum avifHeaderFormat
{
// AVIF file with an "avif" brand, a MetaBox and all its required boxes for maximum compatibility.
AVIF_HEADER_FULL,
// Same as AVIF_HEADER_FULL but the name field of the HandlerBox is empty instead of "libavif".
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may be better to always write an empty handler name so that we can avoid adding a new encoder option. (This new enum variant is equivalent to a new boolean option.) Most of our clients use our defaults, so they will experience the change of behavior.

One way to justify this is that "libavif" is not really a track type.

I checked libheif. It does not seem to write the handler name. The call in libheif/libheif/context.cc is commented out:

  // TODO: the hdlr box is not the right place for comments
  // m_heif_file->set_hdlr_library_info(encoder->plugin->get_plugin_name());

It would be good to inspect HEIC photos taken by an iPhone and see what their handler name field is.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@leo-barnes Leo: This is the libavif HandlerBox name issue I mentioned at the end of the AOMedia Image Container subgroup meeting today.

Yannis: I suggest we make this change without adding a new AVIF header format option.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can confirm that Apple platforms create HEIF files with empty handler name.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yannis: I suggest we make this change without adding a new AVIF header format option.

Done. PTAL.

Copy link
Collaborator

@wantehchang wantehchang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks. I suggest some changes.

src/write.c Outdated Show resolved Hide resolved
src/write.c Outdated Show resolved Hide resolved
tests/gtest/avifheadertest.cc Outdated Show resolved Hide resolved
tests/gtest/avifheadertest.cc Outdated Show resolved Hide resolved
@y-guyon y-guyon changed the title Add AVIF_HEADER_DEFAULT enum variant Empty HandlerBox name instead of "libavif" Aug 22, 2024
@y-guyon y-guyon merged commit 8dc2a62 into AOMediaCodec:main Aug 22, 2024
32 checks passed
@y-guyon y-guyon deleted the header branch August 22, 2024 11:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants