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

V4 : Fix GIF, PNG, and WEBP Edge Case Handling #2894

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

JimBobSquarePants
Copy link
Member

@JimBobSquarePants JimBobSquarePants commented Feb 26, 2025

Prerequisites

  • I have written a descriptive pull-request title
  • I have verified that there are no overlapping pull-requests open
  • I have verified that I am following the existing coding patterns and practice as demonstrated in the repository. These follow strict Stylecop rules 👮.
  • I have provided test coverage for my change (where applicable)

Description

#2882 but for V4.

Fixes #2866
Fixes #2862

Changes shared with V3:

  • Both GIF and WEBP decoders were not handling frame disposal properly.
  • GIF Decoder background color handling was incorrect.
  • GIF Encoder incorrectly used global palette for local root frame.
  • WEBP Decoder did not clear some buffers on load where it should.
  • PNG Encoder palette animations did not work properly
  • Fixed pixel sampling in Wu and Octree quantizers
  • Improved accuracy for matching first 512 colors in EuclidianPixelMap.

V4 Specific changes

  • Rewrite the OctreeQuantizer to support 32bit colors and added memory pooling to massively reduce memory.
  • Added Alpha thresholding to quantizers.
  • Replace Coarse caching with a dedicated type that uses 1/8th of the memory.
  • Removes any decoded color tables should processing occur to allow generation of new tables on encode.

Sorry, something went wrong.

/// </summary>
private sealed class Octree
internal sealed class Octree : IDisposable
Copy link
Member Author

Choose a reason for hiding this comment

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

@rickbrew @saucecontrol This is the new "Octree" I've been chatting to you about.

/// typically very short; in the worst-case, the number of iterations is bounded by 256.
/// This guarantees highly efficient and predictable performance for small, fixed-size color palettes.
/// </remarks>
internal sealed unsafe class ExactCache : IDisposable
Copy link
Member Author

Choose a reason for hiding this comment

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

@rickbrew @saucecontrol Here's the new Exact and Coarse caches I built. They work really well!

Choose a reason for hiding this comment

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

Copilot reviewed 177 out of 177 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (2)

src/ImageSharp/Formats/Png/PngEncoderCore.cs:1482

  • Ensure that 'backgroundColor' is reliably assigned before any use, as the [MemberNotNull] attribute enforces non-nullability and a missing assignment could lead to runtime violations.
[MemberNotNull(nameof(backgroundColor))]

src/ImageSharp/Formats/Png/PngEncoderCore.cs:1557

  • Verify that 'paletteQuantizer' is defined as a nullable value type; if it is or becomes a reference type, using 'HasValue' may introduce nullability issues in future changes.
if (paletteQuantizer.HasValue)
@rickbrew
Copy link

rickbrew commented Mar 2, 2025

As per our discussions on Discord recently, it's probably worth figuring out some additional special treatment for transparent pixels in the Octree Hexadecatree, and in the distance metric. Maybe this would be a follow-up PR at some point. I'm adding this here so there's an easy-ish place to find this info, not because I think this PR needs to incorporate this.

The gist of what I've figured out so far is that when A=0, the distance to any non-A=0 pixel should be the alpha value of that other pixel and should disregard the color channels. So Distance(Rgba(0,0,0,0), Rgba(0,0,0,255)) is 255, but Distance(Rgba(32,64,128,0), Rgba(128,64,32,255)) is also 255. A transparent color is equal to any other transparent color (and this is exactly how it works in premultiplied alpha).

This is important when building the octree so that you don't allocate more than 1 palette slot for fully transparent.

This is also very important during the error diffusion process. If the source color was, say, Rgba(16,16,16,16) but the closest color in the palette was Rgba(0,0,0,0) then the only actual error is that the alpha channel isn't 0. The values of the color channels don't even matter at that point.

We haven't quite figured out the right formula for when working with alpha values other than 0 or 255. My quantization implementation only handles a single A=0 slot in the palette so I haven't had a chance to explore this properly yet. @saucecontrol has a good theory, that the error should be deltaC*(1-abs(deltaA)) but I'm not yet sure it's complete because it doesn't give me good results in some cases (and of course it's possible that the problem is in my code, not necessarily in that formula).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error block in image result after saving after loading some files
2 participants