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

uncompressed: support version 1 boxes #1198

Closed
wants to merge 2 commits into from

Conversation

bradh
Copy link
Contributor

@bradh bradh commented Jun 26, 2024

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.

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;
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

@farindk
Copy link
Contributor

farindk commented Jun 26, 2024

Thanks for the change.

Ready from your side?
I'll probably remove the prefer_minimised_boxes_everywhere option, because it doesn't work to have this in the options struct as they can contradict each other. When we have more similar options later on, we can add a

heif_encoder_options_set_minimised_boxes(bool);

To switch all those options at the same time.

I'd also prefer to set prefer_uncC_short_form = true;, because that is no breaking change for anyone and currently there are probably very few users of the codec anyway. It's better to have a nice default in the future.

@bradh bradh force-pushed the uncc_ver1_take2 branch from 6b1fd79 to c42b8d5 Compare June 26, 2024 19:20
@bradh
Copy link
Contributor Author

bradh commented Jun 26, 2024

I think I've now fixed the option to match.

Do you want me to redo this against master?

@farindk
Copy link
Contributor

farindk commented Jun 26, 2024

Do you want me to redo this against master?

Not needed, I can merge it into master.

@bradh
Copy link
Contributor Author

bradh commented Jun 26, 2024

Do you want me to redo this against master?

Not needed, I can merge it into master.

Thanks.

@farindk
Copy link
Contributor

farindk commented Jun 26, 2024

Rebased and merged into master.
Thank you.

@farindk farindk closed this Jun 26, 2024
@bradh bradh deleted the uncc_ver1_take2 branch June 26, 2024 19:28
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.

2 participants