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.

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

feat: bitdepth option for avif/heif #4036

Merged
merged 1 commit into from
Mar 21, 2024
Merged

feat: bitdepth option for avif/heif #4036

merged 1 commit into from
Mar 21, 2024

Conversation

mertalev
Copy link
Contributor

@mertalev mertalev commented Mar 20, 2024 •
edited
Loading

Following #4031, this PR exposes a bitdepth setting to allow outputting 10-bit or 12-bit AVIF/HEIF files. The default remains 8, so this doesn't change the current behavior.

Fixes #4031

@lovell
Copy link
Owner

lovell commented Mar 20, 2024

Thank you for the PR. The prebuilt binaries only include support for a bitdepth of 8, so you won't be able to add a unit test for this path, but you can check valid values are accepted.

It looks like lots of the tests are failing as the new heifBitdepth property needs adding somewhere around here in constructor.js.

@mertalev
Copy link
Contributor Author

Oh, right! I forgot the default build only supports 8-bit output.

@mertalev
added bitdepth setting
26afdb2 
add validation unit tests

add ushort unit test

add test for valid bitdepth, make test formatting consistent

consistent test wording

add setting to constructor

remove ushort test

fix unit tests

bitdepth is not a boolean
@mertalev
Copy link
Contributor Author

I got the tests working locally after adding --build-from-source and fixing a few bugs. Bit depth as a boolean didn't work too well haha

lovell
lovell approved these changes Mar 21, 2024
View reviewed changes
Copy link
Owner

@lovell lovell left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the updates, all the tests are passing now, this is good to merge.

(I'll also add something to the docs to highlight that the prebuilt binaries only support a bitdepth of 8.)

@lovell lovell added this to the v0.33.3 milestone Mar 21, 2024
@lovell lovell merged commit 3c26080 into lovell:main Mar 21, 2024
15 checks passed
lovell added a commit that referenced this pull request Mar 21, 2024
@lovell
Docs: changelog and credit for #4036
eaf31a5
@lovell
Copy link
Owner

lovell commented Mar 21, 2024

I added a guard against this property when using the prebuilt binaries via commit aa1bbcb as the failure condition could otherwise lead to a segfault.

@mertalev
Copy link
Contributor Author

Yeah, that makes sense.

@adityapatadia
Copy link

Just a question, is there a harm if we pass bitdepth: 10 for all images? Will it end up causing bigger output size?

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

@lovell lovell lovell approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v0.33.3
Development

Successfully merging this pull request may close these issues.

Support outputting higher bit-depth AVIF
3 participants
@mertalev @lovell @adityapatadia