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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add smart animated format conversion. #2588

Merged
merged 28 commits into from Nov 29, 2023
Merged

Conversation

JimBobSquarePants
Copy link
Member

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

Note: Most of the file changes here are new references images that were generated to cater for changes in the color distance calculator that improved lookup accuracy.

This PR adds some internal tooling that allows us to convert between animated image formats without the user having to specify the mechanics for each frame to ensure converting between formats returns an equivalent result.

For v4 I plan to update the metadata formats to introduce public bridging types and methods to create smart defaults when switching formats but for now I had to use an internal, non-extensible approach.

For v3.1 we will support bi-directional conversion between gif, png, and webp formats. Full color table mapping is performed between formats when present as well as duration, and disposal, blend modes where applicable.

Here's an example of an automatic conversion.

Gif (45.2KB)

Encode_AnimatedFormatTransform_FromGif_Rgba32_giphy

Png (33.3KB)

Encode_AnimatedFormatTransform_FromGif_Rgba32_giphy

In addition to the conversion functionality all 3 animated formats have been enhanced with shared methodology that allows the optimization of the frames to only include changed data. This prevents the potential explosion in file size (multi MBs) that can be caused when re-encoding the frames that are coalesced during decode.

This example from gifiddle demonstrates the active encoded area of a frame following that optimization.

image

For many gifs this actually leads to an improvement over the incoming file size. For example, the original of the gif above is 52.3KB

@JimBobSquarePants
Copy link
Member Author

JimBobSquarePants commented Nov 25, 2023

@antonfirsov @tocsoft I need your help if you have time?

I'm seeing allocation cleanup failure reports on Linux/Mac and I can recreate them on my old MacBook. Thing is... I've touched nothing related to memory management nor does this test touch code I've changed with my PR.

Is there any way to easily diagnose the cause? I tried using dotmemory Unit in Rider but it won't give the option to run the test in the manner that allows profiling memory.

Running this test in isolation is enough to cause failure.
[xUnit.net 00:00:27.79] TiffDecoder_CanDecode_JpegCompressed(provider: Tiff/twain-rgb-jpeg-with-bogus-ycbcr-subsampling.tiff[Rgba32]) [FAIL]

EDIT.... OK. It was failing locally but now it suddenly isn't. Could it be a memory pressure thing triggered by my additional tests?

EDIT... It was ME!!

@stefannikolei
Copy link
Contributor

stefannikolei commented Nov 25, 2023

Forget this...

I removed my local copy of the repo and did a clean checkout. Now everything works on the main branch.. strange

@antonfirsov @tocsoft I need your help if you have time?

I'm seeing allocation cleanup failure reports on Linux/Mac and I can recreate them on my old MacBook. Thing is... I've touched nothing related to memory management nor does this test touch code I've changed with my PR.

Is there any way to easily diagnose the cause? I tried using dotmemory Unit in Rider but it won't give the option to run the test in the manner that allows profiling memory.

Running this test in isolation is enough to cause failure. [xUnit.net 00:00:27.79] TiffDecoder_CanDecode_JpegCompressed(provider: Tiff/twain-rgb-jpeg-with-bogus-ycbcr-subsampling.tiff[Rgba32]) [FAIL]

EDIT.... OK. It was failing locally but now it suddenly isn't. Could it be a memory pressure thing triggered by my additional tests?

I tried running main first on my Mac. But not even there all tests go green.

     Encode_AnimatedLossy<Rgba32>(provider: Webp/leo_animated_lossy.webp[Rgba32])

SixLabors.ImageSharp.ImageFormatException
Read unexpected webp chunk data
at SixLabors.ImageSharp.Formats.Webp.WebpThrowHelper.ThrowImageFormatException(String errorMessage) in /Users/stefannikolei/projects/github/sixlabors/ImageSharp/src/ImageSharp/Formats/Webp/WebpThrowHelper.cs:line 14

   CanLimitHwIntrinsicFeaturesWithIntrinsicsParam
   CanLimitHwIntrinsicFeaturesWithSerializableAndIntrinsicsParams
   CanLimitHwIntrinsicFeaturesWithSerializableParam

Microsoft.DotNet.RemoteExecutor.RemoteExecutionException
Remote process failed with an unhandled exception.
Child exception:
Xunit.Sdk.FalseException: Assert.False() Failure
Expected: False
Actual: True

@stefannikolei
Copy link
Contributor

stefannikolei commented Nov 25, 2023

following tests are failing on my machine

     GifDecoderTests
     Decode_VerifyRootFrameAndFrameCount<Rgba32>(provider: Gif/cheers.gif[Rgba32], expectedFrameCount: 93)
     Decode_VerifyRootFrameAndFrameCount<Rgba32>(provider: Gif/issues/issue403_baddescriptorwidth.gif[Rgba32], expectedFrameCount: 36)
     Issue1668_InvalidColorIndex<Rgba32>(provider: Gif/issues/issue1668_invalidcolorindex.gif[Rgba32])
   PngEncoderTests
     Encode_APng<Rgba32>(provider: Png/animated/apng.png[Rgba32])     
    TiffDecoderTests
     DecodeMultiframe<Rgba32>(provider: Tiff/multipage_deflate_withPreview.tiff[Rgba32])
     DecodeMultiframe<Rgba32>(provider: Tiff/multipage_lzw.tiff[Rgba32])
     TiffDecoder_CanDecode_JpegCompressed<Rgba32>(provider: Tiff/twain-rgb-jpeg-with-bogus-ycbcr-subsampling.tiff[Rgba32])

I tried debugging it.

In my eyes (looking at the failing png test) The problem starts to manifest here:

image

In all other iterations the width was 32 and resulted in bytesPerFrameScanLine = 129. Here the width is 10

A naive fix (the unit test passes after this:

image

@JimBobSquarePants
Copy link
Member Author

following tests are failing on my machine

     GifDecoderTests
     Decode_VerifyRootFrameAndFrameCount<Rgba32>(provider: Gif/cheers.gif[Rgba32], expectedFrameCount: 93)
     Decode_VerifyRootFrameAndFrameCount<Rgba32>(provider: Gif/issues/issue403_baddescriptorwidth.gif[Rgba32], expectedFrameCount: 36)
     Issue1668_InvalidColorIndex<Rgba32>(provider: Gif/issues/issue1668_invalidcolorindex.gif[Rgba32])
   PngEncoderTests
     Encode_APng<Rgba32>(provider: Png/animated/apng.png[Rgba32])     
    TiffDecoderTests
     DecodeMultiframe<Rgba32>(provider: Tiff/multipage_deflate_withPreview.tiff[Rgba32])
     DecodeMultiframe<Rgba32>(provider: Tiff/multipage_lzw.tiff[Rgba32])
     TiffDecoder_CanDecode_JpegCompressed<Rgba32>(provider: Tiff/twain-rgb-jpeg-with-bogus-ycbcr-subsampling.tiff[Rgba32])

I tried debugging it.

In my eyes (looking at the failing png test) The problem starts to manifest here:

image In all other iterations the width was 32 and resulted in bytesPerFrameScanLine = 129. Here the width is 10

A naive fix (the unit test passes after this:

image

Well spotted! Yes, that's definitely a bug. The interlaced version of the method was doing the correct thing.

Copy link
Contributor

@gfoidl gfoidl left a comment

Choose a reason for hiding this comment

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

Code-wise LGTM.

To the implementation itself I can't say too much as I'm missing knowledge here.

/// </para>
/// </remarks>
private unsafe struct ColorDistanceCache : IDisposable
{
private const int IndexRBits = 5;
private const int IndexGBits = 5;
private const int IndexBBits = 6;
private const int IndexBBits = 5;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a test that unveils that "bug" (was this a bug?)?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was something I just caught doing a visual test which is hard to test for.

The cache was only filled to a maximum value of ~96% which led to matching where it shouldn't. This led to transparent areas in the frame following deduplication. Originally, I was going to simply add more precision for a single component (blue) which fixed the issue, but I discovered that with the new index determinaiton code the cache fills to a maximum of ~98% and the problem simply went away.

@JimBobSquarePants JimBobSquarePants merged commit 60e0298 into main Nov 29, 2023
8 checks passed
@JimBobSquarePants JimBobSquarePants deleted the js/animation-synergy branch November 29, 2023 11:32
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.

None yet

3 participants