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

LibGfx: Use libwebp #636

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

doctortheemh
Copy link
Contributor

@doctortheemh doctortheemh commented Jul 15, 2024

Use libwebp to do decoding of webp images, dropping the existing code. A number of tests are altered to match new conversion, this is largely expected since the YUV->RGB conversion isn't bit-exact, and the results were compared visually. Resolves #593.

This matches libwebp (see ZeroFillCanvas() call in
libwebp/src/demux/anim_decode.c:355 and ZeroFillFrameRect() call
in line 435, but in WebPAnimDecoderGetNext()) and makes files
written e.g. by asesprite look correct -- even though the old
behavior is also spec-compliant and arguably makes more sense.
Now nothing looks at the background color stored in the file.

See PR for an example image where it makes a visible difference.
Copy link
Member

@ADKaster ADKaster left a comment

Choose a reason for hiding this comment

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

Looks reasonable. could you update the commit message for nico's test update to document which serenity/master commit it was cherry-picked from?

Comment on lines +137 to +138
uint8_t* frame_data;
int timestamp;
Copy link
Member

Choose a reason for hiding this comment

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

please always initialize basic types, even if they're expected to be overridden by a function call.

State state { State::NotDecoded };
ReadonlyBytes data;

ReadonlyBytes chunks_cursor;

// Image properties
Optional<IntSize> size;
Copy link
Contributor

Choose a reason for hiding this comment

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

I know that you didn't write that, but this shouldn't be an Optional

Comment on lines +111 to +112
context.icc_data.resize(icc_profile.size);
memcpy(context.icc_data.data(), icc_profile.bytes, icc_profile.size);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that it can be replaced by ByteBuffer::copy

WebPAnimDecoderOptions anim_decoder_options {};
WebPAnimDecoderOptionsInit(&anim_decoder_options);
anim_decoder_options.color_mode = MODE_BGRA;
anim_decoder_options.use_threads = 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we set that rather than letting the default value?

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.

Use libwebp for WebPLoader
4 participants