-
-
Notifications
You must be signed in to change notification settings - Fork 308
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
uncompressed: support version 1 boxes #1198
Conversation
libheif/heif.cc
Outdated
@@ -2747,11 +2747,16 @@ static void set_default_options(heif_encoding_options& options) | |||
options.color_conversion_options.preferred_chroma_downsampling_algorithm = heif_chroma_downsampling_average; | |||
options.color_conversion_options.preferred_chroma_upsampling_algorithm = heif_chroma_upsampling_bilinear; | |||
options.color_conversion_options.only_use_preferred_chroma_algorithm = false; | |||
|
|||
options.prefer_minimised = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copy from #1132:
Is there any case where a user would want the longer form of the uncC box, if it is equivalent?
If there is no such case, we could drop the option.
If we decide to keep the option, it's better in my experience to have a very specific option (e.g. "force_uncC_long_form") and then have a function "set_force_long_forms_everywhere" that sets all those specific options. If we have a single catch-all option, it will get very messy if we want to enable it just for some box types, later on.
Moreover, I understand that the default 'false' is the backwards-compatible setting. However, if I see this correctly, nothing will break when we make 'true' the default and now write the short form box instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I meant to default to "make backwards compatible", but to have a switch function as you suggest. Some consumers will be on old versions for a while.
My original thinking was that this flag would be generally applicable. So it could be used to enable minified boxes everywhere, given the concept would be to optimise for size over compatibility. I don't have a problem with multiple option flags that could be set seperately or together as needed.
Will add that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have implemented this. However the interaction of flags might be somewhat tricky if we flip the defaults.
At this point the default is false for both options, and the short form uncC
will be created if either flag is set. If we change the default to be true, both would need to be false in order to write the long form.
Thanks for the change. Ready from your side?
To switch all those options at the same time. I'd also prefer to set |
I think I've now fixed the option to match. Do you want me to redo this against master? |
Not needed, I can merge it into master. |
Thanks. |
Rebased and merged into master. |
This solves the same problem as #1132, but aligned to the "the uncompressed codec looks for both variants and then internally generates the implied boxes. They are never visible outside the uncompressed codec."
It doesn't actually generate the boxes, it just works with and without the
cmpd
box.