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/GIF: Support Colors #24330

Merged
merged 4 commits into from
May 18, 2024
Merged

Conversation

LucasChollet
Copy link
Member

gif

You can clearly see the lack of diversity in the colors due to the size of the table being limited to 256 elements.

@github-actions github-actions bot added the 👀 pr-needs-review PR needs review from a maintainer or community member label May 14, 2024
Copy link
Collaborator

@nico nico left a comment

Choose a reason for hiding this comment

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

So image just TODO()s now if you give it an input with > 256 colors and ask for gif output? That's maybe a bit unfriendly.

Anyways, looks fine (even with that TODO). Some comments below, address the ones that you think are useful. (They're a bit rambly.)

Tests/LibGfx/TestMedianCut.cpp Outdated Show resolved Hide resolved
Tests/LibGfx/TestMedianCut.cpp Outdated Show resolved Hide resolved
Tests/LibGfx/TestMedianCut.cpp Show resolved Hide resolved
Userland/Libraries/LibGfx/MedianCut.cpp Outdated Show resolved Hide resolved
Userland/Libraries/LibGfx/MedianCut.cpp Outdated Show resolved Hide resolved
Userland/Libraries/LibGfx/MedianCut.cpp Outdated Show resolved Hide resolved
Userland/Libraries/LibGfx/MedianCut.cpp Show resolved Hide resolved
Userland/Libraries/LibGfx/MedianCut.cpp Outdated Show resolved Hide resolved
Userland/Libraries/LibGfx/MedianCut.cpp Show resolved Hide resolved
Userland/Libraries/LibGfx/MedianCut.h Show resolved Hide resolved
@LucasChollet
Copy link
Member Author

So image just TODO()s now if you give it an input with > 256 colors and ask for gif output? That's maybe a bit unfriendly.

No it doesn't, as an example the image in the PR description has more than 256 colors. It TODOs if the input color was not present during the creation of the palette. Currently, there is no way to hit that TODO in the code. But we will quickly face it if we try to encode new frames with the same palette.

I started to resolve some comment locally, but I'll do the rest tomorrow.

@LucasChollet LucasChollet force-pushed the gif_colors branch 2 times, most recently from f188bb1 to c1e43d8 Compare May 16, 2024 01:58
@nico nico added ⏳ pr-waiting-for-author PR is blocked by feedback / code changes from the author and removed 👀 pr-needs-review PR needs review from a maintainer or community member labels May 18, 2024
@github-actions github-actions bot added 👀 pr-needs-review PR needs review from a maintainer or community member and removed ⏳ pr-waiting-for-author PR is blocked by feedback / code changes from the author labels May 18, 2024
Copy link
Collaborator

@nico nico left a comment

Choose a reason for hiding this comment

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

Still LG

Tests/LibGfx/TestMedianCut.cpp Show resolved Hide resolved
Userland/Libraries/LibGfx/MedianCut.cpp Outdated Show resolved Hide resolved
@nico nico added ✅ pr-community-approved PR has been approved by a community member and removed 👀 pr-needs-review PR needs review from a maintainer or community member labels May 18, 2024
@nico
Copy link
Collaborator

nico commented May 18, 2024

(maintainers: feel free to hit merge! My 2 comments below are minor nits and optional, and can be in a follow-up of they're addressed at all. I'm excited about having this in tree.)

This is useful to find the best matching color palette from an existing
bitmap. It can be used in PixelPaint but also in encoders of old image
formats that only support indexed colors e.g. GIF.
To determine the palette of colors we use the median cut algorithm.
While being a correct implementation, enhancements are obviously
existing on both the median cut algorithm and the encoding side.
@github-actions github-actions bot added 👀 pr-needs-review PR needs review from a maintainer or community member and removed ✅ pr-community-approved PR has been approved by a community member labels May 18, 2024
@awesomekling awesomekling merged commit a0401b0 into SerenityOS:master May 18, 2024
12 checks passed
@github-actions github-actions bot removed the 👀 pr-needs-review PR needs review from a maintainer or community member label May 18, 2024
@LucasChollet LucasChollet deleted the gif_colors branch May 18, 2024 21:55
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

3 participants