V4 : Fix GIF, PNG, and WEBP Edge Case Handling #2894
Conversation
| /// </summary> | ||
| private sealed class Octree | ||
| internal sealed class Octree : IDisposable |
There was a problem hiding this comment.
@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 |
There was a problem hiding this comment.
@rickbrew @saucecontrol Here's the new Exact and Coarse caches I built. They work really well!
JimBobSquarePants
requested review from
Copilot,
tocsoft,
antonfirsov,
dlemstra and
brianpopow
There was a problem hiding this comment.
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)
|
As per our discussions on Discord recently, it's probably worth figuring out some additional special treatment for transparent pixels in the The gist of what I've figured out so far is that when 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, 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 |
|
I ran some #2882 vs #2894 benchmarks. This PR regresses SystemEncodeGif_CoarsePaletteEncoderOn #2882 (V3)
On this PR (V4)
EncodeGif_DefaultEncoderOn #2882 (V3)
On this PR (V4)
|
| int bucketIndex = GetBucketIndex(color.R, color.G, color.B); | ||
| byte quantAlpha = QuantizeAlpha(color.A); | ||
| return this.buckets[bucketIndex].TryGetValue(quantAlpha, out paletteIndex); |
There was a problem hiding this comment.
I would expect this code to be significantly slower than the simple indirect lookup in V3. It's a mistery to me why does the regression only show up in 1 of the 4 benchmark cases. Maybe it's not a hot path in the other ones?
There was a problem hiding this comment.
Yeah I’m experimenting with a new coarse cache that uses a couple of hundred KB rather than 4MB.
I’m not sure why there’s such a dramatic speed loss though for that test. Will need to profile.
There was a problem hiding this comment.
I would personally prefer to attempt to optimize for speed here even if it costs more memory. See #2882 (comment).
|
@antonfirsov I've been thinking a lot recently about how we attempt to preserve the original palette when encodings image and I'm not sure whether doing so it worth the complexity in the encoder code. The GIF encoder for example is far, far more complicated than I'd like due to this behavior. I wonder if we ignore the palette from the metadata and keep if for information purpose only. If someone wants to use it, they can pass it to the encoder via the With the new |
Do we have any guess how often do users prefer this over the encoder recreating the palette? Would it be possible to create a low ceremony API for this in our encoder infra? |
|
Maybe a heretic idea, but would it make sense to make this PR an equivalent for the V3 one and do further quantization improvements in a separate one? This would (1) quickly close on the issue of decoder bugfixes (2) simplify reviews and reduce the chance of making mistakes. |
Honestly, I don't think people even notice. I just want to make things easier to maintain while giving them the power to do specific actions.
In that case. I think we can review this now. I have further optimizations planned but they can wait. |
There was a problem hiding this comment.
Pull Request Overview
This pull request addresses a number of edge-case issues related to GIF, PNG, and WEBP encoding/decoding in V4 by updating color table handling, quantization logic and enhancing transparent pixel processing. Key changes include:
- Exposing interface members explicitly via public modifiers in several interfaces.
- Adjustments in GIF metadata and frame handling, including a dedicated fallback quantizer and revised transparent pixel behavior.
- Updates to auxiliary utilities and AOT compiler tools to support the new quantizer and pixel map functionality.
Reviewed Changes
Copilot reviewed 237 out of 239 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/ImageSharp/Formats/IQuantizingImageEncoder.cs | Added explicit public modifiers on interface members. |
| src/ImageSharp/Formats/IFormatMetadata.cs, IFormatFrameMetadata.cs, IAnimatedImageEncoder.cs | Made similar changes to expose interface members explicitly. |
| src/ImageSharp/Formats/Gif/*.cs | Revised GIF metadata and frame processing logic; removed redundant color table copying. |
| src/ImageSharp/Formats/GifEncoderCore.cs | Introduced a fallback quantizer and updated frame quantization options; added a TODO regarding metadata checks. |
| src/ImageSharp/Formats/GifDecoderCore.cs | Updated decoding to better handle background color index and frame disposal behaviors. |
| src/ImageSharp/Formats/* (Cur, Bmp) | Removed the deprecated color table assignments and cleared metadata after image/frame processing. |
| src/ImageSharp/Formats/EncodingUtilities.cs | Renamed and modified functions to replace transparent pixels rather than clearing them. |
| src/ImageSharp/Advanced/AotCompilerTools.cs | Added the AOT pre-seeding for pixel maps. |
| src/ImageSharp/Common/InlineArray.cs | Added new inline fixed sized array types. |
Files not reviewed (2)
- Directory.Build.props: Language not supported
- src/ImageSharp/Common/InlineArray.tt: Language not supported
Comments suppressed due to low confidence (3)
src/ImageSharp/Formats/IQuantizingImageEncoder.cs:16
- [nitpick] Explicitly marking interface members as public is redundant since all interface members are public by default. Consider removing the explicit modifier to keep the code concise and consistent with standard interface definitions.
public IQuantizer? Quantizer { get; }
src/ImageSharp/Formats/Gif/GifEncoderCore.cs:198
- [nitpick] There is a lingering TODO comment regarding checking metadata. It would be helpful to either address this issue or provide a more specific comment about the expected behavior to avoid confusion in production code.
// TODO: We should be checking the metadata here also I think?
src/ImageSharp/Formats/Gif/GifDecoderCore.cs:874
- [nitpick] The background color index is assigned both to a local field and then later set in the GIF metadata. Consider consolidating these assignments into a single source of truth to avoid potential discrepancies between the global field and the metadata.
byte index = this.logicalScreenDescriptor.BackgroundColorIndex;
|
@antonfirsov I added Magick.NET to the GIF Encode benchmarks to guage performance there as SkiaSharp does not support GIF.
It's important to note that neither library performs the same deduplication operations we do so accurate comparison is hard as we cannot isolate quantization. |
What I meant is to draw the line at the exact equivalent of the V3 changes and split out any improvements on top on that to a separate PR. The current state regresses one of the benchmark cases significantly for a reason we don't understand. My primary "review feedback" against this state is that we should go figure out why is there a regression and attempt to reduce it. This may take long, so IMO it would be good to get the codec fixes merged first. Then the next PR would be only scoped on V4 quantization improvements. |
Here's the interesting thing. I'm not seeing the same slowdown for the single test that you saw. I've ran the benchmarks several times now and I'm seeing a consistant slowdown with the new
Switching back to the old cache brings performance back in line.
When comparing using default options we can see that the performance is comparable. Default New Cache
Default Old Cache.
So why such a performance difference for the caches when using the websafe palette? Here's what I think. When the palette is generated from the image, both caches perform similarly because nearly every lookup is a hit. However, with a generic palette like the 141 websafe colors, which doesn’t match the image’s color distribution, more cache misses occur and force a fallback to a full palette scan. In this case, the new CoarseCache's overhead from bucket scanning and branch checks starts to add up. The older 4 MB CoarseCache avoids this by using direct, branch-free indexing, resulting in roughly 3x faster performance despite its larger memory footprint. The websafe palette test was designed to mimic System.Drawing’s output, but since that library doesn’t actually do any quantization work, it’s hard to draw meaningful comparisons. I’m also not sure what the inner workings of the ImageMagick encoder are, but I suspect it isn’t applying dithering either, since the input and output colors are always identical. That limits the usefulness of the test to mostly comparing our own quantizers. What I can say about the benchmark, though, is that it’s testing a worst-case scenario for the new cache. It’s a configuration that would only be used in the real world as a novelty, since the output quality is so poor. The default setup, on the other hand, works really well. |
Prerequisites
Description
#2882 but for V4.
Fixes #2866
Fixes #2862
Changes shared with V3:
V4 Specific changes