Skip to content

heifsave: add option to control chroma subsampling #1961

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jan 13, 2021
Merged

heifsave: add option to control chroma subsampling #1961

merged 1 commit into from
Jan 13, 2021

Conversation

lovell
Copy link
Member

@lovell lovell commented Jan 12, 2021

As discussed in #1788, providing control over the use of chroma subsampling is a genuinely useful feature for still images created via libheif and is consistently supported by all of its encoder plugins.

This PR also adds a test for this now that the CI environment supports the AVIF format.

@jcupitt
Copy link
Member

jcupitt commented Jan 12, 2021 •
edited
Loading

I think the last component of enum values can't be a number or you get problems in bindings, so it would need to be something like _S420. This is why angles like VIPS_ANGLE_D90 have the D there.

Do we need such precise control? How about reusing VipsForeignJpegSubsample (on, off, auto) rather than making a new enum? We could call it VipsForeignSubsample and have a compat typedef for VipsForeignJpegSubsample. In auto mode (the default) the jpg saver turns off chroma subsample for Q>=90.

@lovell
Copy link
Member Author

lovell commented Jan 12, 2021

Yep, makes sense, I've no idea how much if any need there will be for 4:2:2, and can imagine most systems will be optimised for 4:2:0 or 4:4:4.

Do we want the same "quality" > 90 condition to apply for "auto"? In my not-very-scientific testing, HEIC and AVIF generally produce better "quality" than JPEG at a lower "quality" value, if that makes sense (they generally make better decisions about what to throw away and/or better decisions about what can be predicted).

@lovell lovell force-pushed the heif-chroma-subsampling branch 2 times, most recently from cfcea90 to ad37338 Compare January 12, 2021 22:18
lovell
lovell commented Jan 13, 2021
View reviewed changes
Copy link
Member Author

@lovell lovell left a comment

Choose a reason for hiding this comment

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

I've updated this PR to make VipsForeignSubsample more generic and added a few comments inline to help explain the approach.

@lovell
heifsave: add option to control subsample_mode
3ad7363 
Defaults to no subsampling when Q>90 for consistency with jpegsave.

Deprecate VipsForeignJpegSubsample enum, replace with more generic
VipsForeignSubsample.
@lovell lovell force-pushed the heif-chroma-subsampling branch from ad37338 to 3ad7363 Compare January 13, 2021 10:02
lovell
lovell commented Jan 13, 2021
View reviewed changes
Copy link
Member Author

@lovell lovell left a comment

Choose a reason for hiding this comment

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

The VipsForeignJpegSubsample enum remains and is marked as deprecated in favour of the more generic VipsForeignSubsample enum.

@jcupitt jcupitt merged commit a0aa1fb into libvips:master Jan 13, 2021
@jcupitt
Copy link
Member

jcupitt commented Jan 13, 2021

Read through it, looks great!

@lovell lovell deleted the heif-chroma-subsampling branch January 13, 2021 14:02
@lovell lovell mentioned this pull request Feb 7, 2021
Closed
@thomkrupa
Copy link

@jcupitt hey, do you have any ETA for releasing this PR?

I don't want to rush you. I just want to manage the internal work that depends on this PR. We need this to fix: lovell/sharp#2562

Thanks for your work!

@jcupitt
Copy link
Member

jcupitt commented Feb 27, 2021

Hiya, it'll be in 8.11, which (guess!) will be April or May 2021.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jcupitt jcupitt jcupitt left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@lovell @jcupitt @thomkrupa