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

AVIF support #722

Merged
merged 33 commits into from Aug 12, 2020
Merged

AVIF support #722

merged 33 commits into from Aug 12, 2020

Conversation

surma
Copy link
Collaborator

@surma surma commented Feb 6, 2020

This PR lands support for AVIF.

So far, the codec is ported and integrated into Squoosh. Options UI needs polish, which @jakearchibald will do.

codecs/avif_dec/build.sh Outdated Show resolved Hide resolved
codecs/avif_dec/build.sh Outdated Show resolved Hide resolved
avifPixelFormat format;
switch (options.subsample) {
case 0:
format = AVIF_PIXEL_FORMAT_YUV420;
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's expose the actual underlying enum instead.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, I'll pick this up (and as many other options as I can find) when I do the UI

codecs/avif_dec/build.sh Outdated Show resolved Hide resolved
@surma surma closed this Jun 15, 2020
@surma surma reopened this Jun 16, 2020
@surma surma changed the base branch from master to dev June 16, 2020 11:01
@RReverser
Copy link
Contributor

RReverser commented Aug 3, 2020

@surma Just copying some of my unresolved comments from resolved conversation on the build system PR, just so that they're not lost:

I suspect you'll need to free output too, like the official example does, using avifRWDataFree.

You'll probably also want to perform free/destroy above the encodeResult condition, otherwise those structures will get leaked on encoding errors.

I'd probably recommend trying this out in Squoosh with sanitizer enabled once we are done, but this can be left as part of the original PR.

@surma
Copy link
Collaborator Author

surma commented Aug 3, 2020

I (hopefully) fixed the leaky bits now :)

@RReverser
Copy link
Contributor

I (hopefully) fixed the leaky bits now :)

The code looks good to me now, but I'd still double-check with sanitizers enabled and the manual "run leak checks" function call like in the blog post to be sure :)

@surma
Copy link
Collaborator Author

surma commented Aug 3, 2020

Oh yeah, I’m definitely looking forward to you unleashing the sanitizer on the code base :D

src/lib/util.ts Outdated Show resolved Hide resolved
@RReverser
Copy link
Contributor

Oh yeah, I’m definitely looking forward to you unleashing the sanitizer on the code base :D

I already did on the existing codecs, now it needs to be done on any new ones by whoever adds them :P

@surma
Copy link
Collaborator Author

surma commented Aug 3, 2020

Thanks for volunteering.


using namespace emscripten;

class RawImage {
Copy link
Contributor

Choose a reason for hiding this comment

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

@surma Please remove this in favour of ImageData using other codecs as examples.

@RReverser
Copy link
Contributor

Thanks for volunteering.

Nice try.

@surma
Copy link
Collaborator Author

surma commented Aug 11, 2020

@RReverser The current code passed the sanitizer test :)

Screenshot 2020-08-11 at 18 41 16

I made a commit and reverted it to keep the code available. I didn’t want to do all the macro juggling at this point without making a plan with you.

@RReverser
Copy link
Contributor

RReverser commented Aug 11, 2020

Awesome! I see you also temporarily fixed indentation to tabs in the Makefile (my fault 😔), could you please re-introduce those fixes back?

image

@surma
Copy link
Collaborator Author

surma commented Aug 11, 2020

Done

@surma surma changed the base branch from dev to v2-codecs August 12, 2020 10:15
@surma surma merged commit 2c923e5 into v2-codecs Aug 12, 2020
@surma surma deleted the avif branch August 12, 2020 10:15
@nucliweb
Copy link
Contributor

Congrats @surma 🤘🏼😊🤘🏼

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.

None yet

4 participants