V3 : Fix GIF, PNG, and WEBP Edge Case Handling #2882
Conversation
JimBobSquarePants
requested review from
tocsoft,
antonfirsov,
dlemstra,
brianpopow and
Copilot
There was a problem hiding this comment.
Copilot reviewed 5 out of 16 changed files in this pull request and generated no comments.
Files not reviewed (11)
- tests/Images/External/ReferenceOutput/GifDecoderTests/Issue1962_Rgba32_issue1962_tiniest_gif_1st.png: Language not supported
- tests/Images/External/ReferenceOutput/GifDecoderTests/Issue2012BadMinCode_Rgba32_issue2012_drona1.png: Language not supported
- tests/Images/Input/Gif/issues/issue_2866.gif: Language not supported
- tests/Images/Input/Webp/issues/Issue2866.webp: Language not supported
- src/ImageSharp/Formats/Gif/GifEncoderCore.cs: Evaluated as low risk
- tests/ImageSharp.Tests/TestUtilities/ReferenceCodecs/MagickReferenceDecoder.cs: Evaluated as low risk
- src/ImageSharp/Formats/Webp/Lossy/Vp8Decoder.cs: Evaluated as low risk
- src/ImageSharp/Formats/Webp/BitReader/BitReaderBase.cs: Evaluated as low risk
- tests/ImageSharp.Tests/Formats/Png/PngEncoderTests.cs: Evaluated as low risk
- tests/ImageSharp.Tests/TestImages.cs: Evaluated as low risk
- src/ImageSharp/Formats/Webp/WebpBlendMethod.cs: Evaluated as low risk
Comments suppressed due to low confidence (2)
tests/ImageSharp.Tests/Formats/Gif/GifEncoderTests.cs:389
- [nitpick] The test method name GifEncoder_CanDecode_Issue2866 is misleading since it is testing encoding, not decoding. It should be renamed to GifEncoder_CanEncode_Issue2866.
public void GifEncoder_CanDecode_Issue2866<TPixel>(TestImageProvider<TPixel> provider)
src/ImageSharp/Formats/Webp/WebpAnimationDecoder.cs:89
- The comment should reflect the correct type for backgroundColor, which is TPixel instead of Color.
/// <param name="backgroundColor">The default background color of the canvas in.</param>
|
This is quite a huge change, but I'm planning to take a high-level look in the coming days. |
Thanks! I have a V4 version also that I’m going to push, lots of similarities but also some additional changes. |
There was a problem hiding this comment.
I'm not familiar with the codec logic this is touching, might be good if @brianpopow can also take a look.
Some general concerns:
- Would be nice to have as high functional test coverage as possible for the functionality gap the PR is closing (with validation). The PR description lists both encoder and decoder issues, but only encoder tests are being added. If I get it right there are cases not covered by Error block in image result after saving after loading some files #2866 and
Quality loss when quantizing an image that already has (fewer than) 256 colors #2862, can we get those tested as well? - [Edited] I'm skeptical about the Color Distance Cache changes. I assume the goal is an improvement in quitizer quality, but I think it comes with significant performance cost, so we should do a pro-contra analysis on it. Are there some main vs PR example pictures that demonstrate the improvement? Is it possible to get main vs PR benchmarks to see the performance impact?
|
|
||
| [Theory] | ||
| [WithFile(TestImages.Gif.Issues.Issue2866, PixelTypes.Rgba32)] | ||
| public void GifEncoder_CanDecode_Issue2866<TPixel>(TestImageProvider<TPixel> provider) |
There was a problem hiding this comment.
Isn't this a decoder issue/test actually? Can we do validation here?
There was a problem hiding this comment.
It's actually both. There were issues in the encoder and decoder.
| where TPixel : unmanaged, IPixel<TPixel> | ||
| { | ||
| if (TestEnvironment.RunsOnCI && !TestEnvironment.IsWindows) | ||
| if (TestEnvironment.RunsOnCI) |
There was a problem hiding this comment.
Instead of skipping entirely, can we instead run this with RemoteExecutor and/or make them non-parallel?
There was a problem hiding this comment.
It's a massive file with lots of frames ones decoded. I just wanted to ease the pressure.
There was a problem hiding this comment.
Since this validates a corner-case secanario, IMO it's better to pay the price to have the CI-coverage. We have some tests disabled on 32bit or on Windows when RunsOnCI is true, but AFAIK, we don't have any tests that would be entirely opted out on all CI pipelines; it would set a bad precedent.
| @@ -32,7 +32,7 @@ protected BitReaderBase(Stream inputStream, int imageDataSize, MemoryAllocator m | |||
| /// <param name="memoryAllocator">Used for allocating memory during reading data from the stream.</param> | |||
| protected static IMemoryOwner<byte> ReadImageDataFromStream(Stream input, int bytesToRead, MemoryAllocator memoryAllocator) | |||
| { | |||
| IMemoryOwner<byte> data = memoryAllocator.Allocate<byte>(bytesToRead); | |||
| IMemoryOwner<byte> data = memoryAllocator.Allocate<byte>(bytesToRead, AllocationOptions.Clean); | |||
There was a problem hiding this comment.
Is it necessary to clean the buffer given that we assume we will read bytesToRead bytes anyways? And coming to the assumption: instead of ignoring the input.Read() result, shouldn't we fail the read and abort processing the stream if it's <bytesToRead ?
There was a problem hiding this comment.
I just don't think it's a good idea to do any uncleaned buffer reading in the decoders now we've had CVEs due to it.
There was a problem hiding this comment.
It always depends on the particular buffer usage. This is a a raw chunk buffer that is to be transformed as part of the decoding process, and the raw contents won't show up in image buffers. In any case, ignoring the read result seems to be more worrying and implementing proper handling of the read result would render Clean unnecessary.
There was a problem hiding this comment.
I've rereviewed and in this case there are lots of places expecting the data span to be the requested length. It's much easier for me to clean the array than refactor.
| this.CacheY = memoryAllocator.Allocate<byte>((16 * this.CacheYStride) + extraY, AllocationOptions.Clean); | ||
| int cacheUvSize = (16 * this.CacheUvStride) + extraUv; | ||
| this.CacheU = memoryAllocator.Allocate<byte>(cacheUvSize); | ||
| this.CacheV = memoryAllocator.Allocate<byte>(cacheUvSize); | ||
| this.TmpYBuffer = memoryAllocator.Allocate<byte>((int)width); | ||
| this.TmpUBuffer = memoryAllocator.Allocate<byte>((int)width); | ||
| this.TmpVBuffer = memoryAllocator.Allocate<byte>((int)width); | ||
| this.Pixels = memoryAllocator.Allocate<byte>((int)(width * height * 4)); | ||
| this.CacheU = memoryAllocator.Allocate<byte>(cacheUvSize, AllocationOptions.Clean); | ||
| this.CacheV = memoryAllocator.Allocate<byte>(cacheUvSize, AllocationOptions.Clean); |
There was a problem hiding this comment.
Instead of celaning these, shouldn't we consider removing the #if DEBUG below, and let them being filled with 205?
There was a problem hiding this comment.
I think we can get away without cleaning everything but the Pixels buffer
src/ImageSharp/Processing/Processors/Quantization/EuclideanPixelMap{TPixel}.cs
Outdated
Show resolved
Hide resolved
| /// <param name="key">The key to add.</param> | ||
| /// <param name="value">The value to add.</param> | ||
| /// <returns><see langword="true"/> if the key was added; otherwise, <see langword="false"/>.</returns> | ||
| [MethodImpl(MethodImplOptions.AggressiveInlining)] |
There was a problem hiding this comment.
Does such a complex method have to be aggressively inlined? We can trust the JIT much more these days about inlining decisions.
| } | ||
|
|
||
| [MethodImpl(InliningOptions.ShortMethod)] | ||
| public readonly void Add(Rgba32 rgba, byte index) | ||
| [MethodImpl(MethodImplOptions.AggressiveInlining)] |
There was a problem hiding this comment.
Even though I haven't profiled ImageSharp for a long time, I still think InliningOptions is a helpful tool to support profiling work. If we want to challange that assumption I would prefer to do it in a separate discussion/PR rather than with arbitrary changes.
| [MethodImpl(InliningOptions.ShortMethod)] | ||
| public readonly void Add(Rgba32 rgba, byte index) | ||
| [MethodImpl(MethodImplOptions.AggressiveInlining)] | ||
| public readonly void Add(Rgba32 color, short index) |
There was a problem hiding this comment.
All callsites look like this.cache.Add(rgba, (byte)index). I know the fallback table was a short buffer even before the change, but now I'm confused why is it needed. Naturally I would guess it's for faster memory access, or am I wrong? If it's for perf, did we ever benchmark it?
There was a problem hiding this comment.
I'm not sure why I changed this. Will review.
| /// <param name="value">The value associated with the key, if found.</param> | ||
| /// <returns><see langword="true"/> if the key is found; otherwise, <see langword="false"/>.</returns> | ||
| [MethodImpl(MethodImplOptions.AggressiveInlining)] | ||
| public bool TryGetValue(uint key, out short value) |
There was a problem hiding this comment.
I expect this code to be significantly slower than the GetPalettIeIndex+memory lookup on the fallback path. A main vs PR benchmark would show the performance impact.
There was a problem hiding this comment.
It's not bad actually since we're using the custom buckets (A standard dictionary would be slower). It's a bit slower (will vary image to image depending on color distribution) than the coarse cache on its own but we need that accuracy.
There was a problem hiding this comment.
It's a bit slower
I expect the perf regression to be significant. If I'm understanding the cache logic correctly, the code will be hit by default per pixel.
I still think we should at least be aware of the exact performance impact and make sure we are making a conscious decision. I can implement/run benchmarks later this week if that helps.
we need that accuracy
Are we sure all users prefer accuracy over performance by default? What about users who want to opt into faster perf rather than quality?
There was a problem hiding this comment.
@antonfirsov I haven't pushed the code here yet as I'm still not sure what the default mode should be, and it requires updating reference images but....
I have defined a new enum on QuantizerOptions that allows switching between coarse, hybrid, and exact color distance matching. Here's the benchmarks for the three approaches.
Hybrid isn't that much slower than the coarse cache on its own.
.NET SDK=9.0.200
[Host] : .NET 7.0.20 (7.0.2024.26716), X64 RyuJIT
Job-WTTMLE : .NET 6.0.36 (6.0.3624.51421), X64 RyuJIT
Runtime=.NET 6.0 Arguments=/p:DebugType=portable IterationCount=3
LaunchCount=1 WarmupCount=3
Coarse
| Method | TestImage | Mean | Error | StdDev | Ratio | RatioSD | Gen 0 | Gen 1 | Gen 2 | Allocated |
|---|---|---|---|---|---|---|---|---|---|---|
| 'System.Drawing Gif' | Bmp/Car.bmp | 8.837 ms | 2.073 ms | 0.1136 ms | 1.00 | 0.00 | 15.6250 | 15.6250 | 15.6250 | 198 KB |
| 'ImageSharp Gif' | Bmp/Car.bmp | 10.363 ms | 1.161 ms | 0.0636 ms | 1.17 | 0.01 | 93.7500 | 93.7500 | 93.7500 | 224 KB |
| 'System.Drawing Gif' | Png/rgb-48bpp.png | 35.747 ms | 3.901 ms | 0.2138 ms | 1.00 | 0.00 | 71.4286 | 71.4286 | 71.4286 | 393 KB |
| 'ImageSharp Gif' | Png/rgb-48bpp.png | 43.825 ms | 37.513 ms | 2.0562 ms | 1.23 | 0.05 | 83.3333 | 83.3333 | 83.3333 | 287 KB |
Hybrid
| Method | TestImage | Mean | Error | StdDev | Ratio | RatioSD | Gen 0 | Gen 1 | Gen 2 | Allocated |
|---|---|---|---|---|---|---|---|---|---|---|
| 'System.Drawing Gif' | Bmp/Car.bmp | 8.638 ms | 1.524 ms | 0.0835 ms | 1.00 | 0.00 | 15.6250 | 15.6250 | 15.6250 | 198 KB |
| 'ImageSharp Gif' | Bmp/Car.bmp | 14.259 ms | 5.146 ms | 0.2821 ms | 1.65 | 0.02 | 46.8750 | 46.8750 | 46.8750 | 207 KB |
| 'System.Drawing Gif' | Png/rgb-48bpp.png | 35.540 ms | 4.489 ms | 0.2461 ms | 1.00 | 0.00 | 71.4286 | 71.4286 | 71.4286 | 393 KB |
| 'ImageSharp Gif' | Png/rgb-48bpp.png | 50.919 ms | 7.914 ms | 0.4338 ms | 1.43 | 0.02 | 100.0000 | 100.0000 | 100.0000 | 306 KB |
Exact (No Cache)
| Method | TestImage | Mean | Error | StdDev | Ratio | RatioSD | Gen 0 | Gen 1 | Gen 2 | Allocated |
|---|---|---|---|---|---|---|---|---|---|---|
| 'System.Drawing Gif' | Bmp/Car.bmp | 9.025 ms | 6.737 ms | 0.3693 ms | 1.00 | 0.00 | 15.6250 | 15.6250 | 15.6250 | 198 KB |
| 'ImageSharp Gif' | Bmp/Car.bmp | 132.910 ms | 50.492 ms | 2.7677 ms | 14.74 | 0.47 | 250.0000 | 250.0000 | 250.0000 | 335 KB |
| 'System.Drawing Gif' | Png/rgb-48bpp.png | 35.667 ms | 1.864 ms | 0.1022 ms | 1.00 | 0.00 | 66.6667 | 66.6667 | 66.6667 | 393 KB |
| 'ImageSharp Gif' | Png/rgb-48bpp.png | 360.896 ms | 138.605 ms | 7.5974 ms | 10.12 | 0.19 | - | - | - | 1,258 KB |
There was a problem hiding this comment.
@antonfirsov I've pushed my changes so you can see what I've done. It's still defaulting to hybrid at the moment so the updated tests pass.
Doing it in a way that provided optimial codegen by avoiding branching required using previewd features (static abstract interface) but the usage is tightly constrined internally and is non-viral.
There was a problem hiding this comment.
Sounds promising, I want to take a look in the next couple of days and also benchmark it against some heavier gifs.
…elMap{TPixel}.cs
Co-authored-by: Anton Firszov <antonfir@gmail.com>
|
@JimBobSquarePants I think it would be good to add a few more test case to verify everything works as expected. Here is a github repository with some gif images generated with imagemagick: gif-test-suite, which I think could be useful testcases. I can help with adding tests, I have some time in the next few days. |
|
That would be awesome if you could thanks! |
|
With some of test images I have added with f80aa76, the output of ImageSharp does not match what imagemagick would produce, which may indicate an error. The output from ImageSharp looks like this:
I am not sure yet why. I will try to figure out what's wrong tomorrow. To extract the images with imagemagick, the following command can be used: |
|
@brianpopow That looks like something to do with palette reading though the last frame suggests an additional bug. |
@JimBobSquarePants It seems that the issue is a regression from version 2.1.9 to 3.0.0. It is an issue with handling the disposal method The weird thing is that when I save the frames like this: The images still look black and white. I am not sure what I do wrong here with saving the frames. |
I can fix restore to previous behavior easily enough but I'm seeing some odd behavior in ImageMagick for this image and others. It doesn't seem to follow the gif89a specification at all regarding GCE transparency and is setting entries at a given index with an alpha of zero regardless of whether there is a transparency flag. @dlemstra Any idea why this is? |
|
@brianpopow I figured out why the output is wrong when cloning. The current code attempts to encode the image using the palette data and bit depth extracted from the individual frames however the new frame contains all the data from the combined frame not just te delta. We shouldn't attempt to preserve the palette when converting between formats. I'll push a fix for this today to this branch. |
@JimBobSquarePants: I am glad you found the issue, I had trouble making sense out of this behavior. |
Prerequisites
Description
Fixes #2866
Fixes #2862
There's quite a lot going on here:
I'll have to manually rework this for V4 as the code base has migrated significantly there.