Skip to content

Commit cb05fc7

Browse files
JimBobSquarePantsJimBobSquarePants
committed
Fix #2980 
1 parent d7b88d7 commit cb05fc7

File tree

74 files changed

+246
-194
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

74 files changed

+246
-194
lines changed

‎src/ImageSharp/Formats/Gif/GifDecoderCore.cs‎

Lines changed: 49 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -489,6 +489,12 @@ private bool ReadFrame<TPixel>(
489489
backgroundColor = Color.Transparent;
490490
}
491491

492+
// We zero the alpha only when this frame declares transparency so that
493+
// frames with a transparent index coalesce over a transparent canvas rather than
494+
// baking the LSD background as a matte. When the flag is not set, this frame will
495+
// write an opaque color for every addressed pixel; keeping the LSD background
496+
// opaque here allows ReadFrameColors to show that background in uncovered areas
497+
// for non-transparent GIFs that rely on it. We still do not prefill the canvas here.
492498
if (this.graphicsControlExtension.TransparencyFlag)
493499
{
494500
backgroundColor = backgroundColor.WithAlpha(0);
@@ -498,24 +504,18 @@ private bool ReadFrame<TPixel>(
498504
this.ReadFrameColors(stream, ref image, ref previousFrame, ref previousDisposalMode, colorTable, backgroundColor.ToPixel<TPixel>());
499505

500506
// Update from newly decoded frame.
501-
if (this.graphicsControlExtension.DisposalMethod != FrameDisposalMode.RestoreToPrevious)
507+
FrameDisposalMode disposalMethod = this.graphicsControlExtension.DisposalMethod;
508+
if (disposalMethod != FrameDisposalMode.RestoreToPrevious)
502509
{
503-
if (this.backgroundColorIndex < colorTable.Length)
504-
{
505-
backgroundColor = Color.FromPixel(colorTable[this.backgroundColorIndex]);
506-
}
507-
else
508-
{
509-
backgroundColor = Color.Transparent;
510-
}
511-
512-
// TODO: I don't understand why this is always set to alpha of zero.
513-
// This should be dependent on the transparency flag of the graphics
514-
// control extension. ImageMagick does the same.
515-
// if (this.graphicsControlExtension.TransparencyFlag)
516-
{
517-
backgroundColor = backgroundColor.WithAlpha(0);
518-
}
510+
// Do not key this on the transparency flag. Disposal handling is determined by
511+
// the previous frame's disposal, not by whether the current frame declares a transparent
512+
// index. For editing we carry a transparent background so that RestoreToBackground clears
513+
// remove pixels to transparent rather than painting an opaque matte. The LSD background
514+
// color is display advice and should be used only when explicitly flattening or when
515+
// rendering with an option to honor it.
516+
backgroundColor = (this.backgroundColorIndex < colorTable.Length)
517+
? Color.FromPixel(colorTable[this.backgroundColorIndex]).WithAlpha(0)
518+
: Color.Transparent;
519519
}
520520

521521
// Skip any remaining blocks
@@ -546,30 +546,45 @@ private void ReadFrameColors<TPixel>(
546546
GifImageDescriptor descriptor = this.imageDescriptor;
547547
int imageWidth = this.logicalScreenDescriptor.Width;
548548
int imageHeight = this.logicalScreenDescriptor.Height;
549-
bool transFlag = this.graphicsControlExtension.TransparencyFlag;
549+
bool useTransparency = this.graphicsControlExtension.TransparencyFlag;
550+
bool useBackground;
550551
FrameDisposalMode disposalMethod = this.graphicsControlExtension.DisposalMethod;
551552
ImageFrame<TPixel> currentFrame;
552553
ImageFrame<TPixel>? restoreFrame = null;
553554

554555
if (previousFrame is null && previousDisposalMode is null)
555556
{
556-
image = transFlag
557-
? new Image<TPixel>(this.configuration, imageWidth, imageHeight, this.metadata)
558-
: new Image<TPixel>(this.configuration, imageWidth, imageHeight, backgroundPixel, this.metadata);
557+
// First frame: prefill with LSD background iff a GCT exists (policy: HonorBackgroundColor).
558+
useBackground =
559+
this.logicalScreenDescriptor.GlobalColorTableFlag
560+
&& disposalMethod == FrameDisposalMode.RestoreToBackground;
561+
562+
image = useBackground
563+
? new Image<TPixel>(this.configuration, imageWidth, imageHeight, backgroundPixel, this.metadata)
564+
: new Image<TPixel>(this.configuration, imageWidth, imageHeight, this.metadata);
559565

560566
this.SetFrameMetadata(image.Frames.RootFrame.Metadata);
561567
currentFrame = image.Frames.RootFrame;
562568
}
563569
else
564570
{
571+
// Subsequent frames: use LSD background iff previous disposal was RestoreToBackground and a GCT exists.
572+
useBackground =
573+
this.logicalScreenDescriptor.GlobalColorTableFlag
574+
&& previousDisposalMode == FrameDisposalMode.RestoreToBackground;
575+
565576
if (previousFrame != null)
566577
{
567578
currentFrame = image!.Frames.AddFrame(previousFrame);
568579
}
569-
else
580+
else if (useBackground)
570581
{
571582
currentFrame = image!.Frames.CreateFrame(backgroundPixel);
572583
}
584+
else
585+
{
586+
currentFrame = image!.Frames.CreateFrame();
587+
}
573588

574589
this.SetFrameMetadata(currentFrame.Metadata);
575590

@@ -580,7 +595,7 @@ private void ReadFrameColors<TPixel>(
580595

581596
if (previousDisposalMode == FrameDisposalMode.RestoreToBackground)
582597
{
583-
this.RestoreToBackground(currentFrame, backgroundPixel, transFlag);
598+
this.RestoreToBackground(currentFrame, backgroundPixel, !useBackground);
584599
}
585600
}
586601

@@ -670,22 +685,31 @@ private void ReadFrameColors<TPixel>(
670685
// Take the descriptorLeft..maxX slice of the row, so the loop can be simplified.
671686
row = row[descriptorLeft..maxX];
672687

673-
if (!transFlag)
688+
if (!useTransparency)
674689
{
675690
for (int x = 0; x < row.Length; x++)
676691
{
677692
int index = indicesRow[x];
678-
index = Numerics.Clamp(index, 0, colorTableMaxIdx);
693+
694+
// Treat any out of bounds values as background.
695+
if (index > colorTableMaxIdx)
696+
{
697+
index = Numerics.Clamp(index, 0, colorTableMaxIdx);
698+
}
699+
679700
row[x] = TPixel.FromRgb24(colorTable[index]);
680701
}
681702
}
682703
else
683704
{
705+
TPixel transparentPixel = Color.Transparent.ToPixel<TPixel>();
684706
for (int x = 0; x < row.Length; x++)
685707
{
686708
int index = indicesRow[x];
687709

688710
// Treat any out of bounds values as transparent.
711+
// We explicitly set the pixel to transparent rather than alter the inbound
712+
// color palette.
689713
if (index > colorTableMaxIdx || index == transIndex)
690714
{
691715
continue;

‎src/ImageSharp/Formats/Gif/GifEncoderCore.cs‎

Lines changed: 45 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -120,11 +120,15 @@ public void Encode<TPixel>(Image<TPixel> image, Stream stream, CancellationToken
120120
// Is this a gif with color information. If so use that, otherwise use octree.
121121
if (gifMetadata.ColorTableMode == FrameColorTableMode.Global && gifMetadata.GlobalColorTable?.Length > 0)
122122
{
123-
int transparencyIndex = GetTransparentIndex(quantized, frameMetadata);
124-
if (transparencyIndex >= 0 || gifMetadata.GlobalColorTable.Value.Length < 256)
123+
int ti = GetTransparentIndex(quantized, frameMetadata);
124+
if (ti >= 0 || gifMetadata.GlobalColorTable.Value.Length < 256)
125125
{
126126
// We avoid dithering by default to preserve the original colors.
127-
globalQuantizer = new PaletteQuantizer(gifMetadata.GlobalColorTable.Value, options.DeepClone(o => o.Dither = null));
127+
globalQuantizer = new PaletteQuantizer(
128+
gifMetadata.GlobalColorTable.Value,
129+
options.DeepClone(o => o.Dither = null),
130+
ti,
131+
Color.Transparent);
128132
}
129133
else
130134
{
@@ -173,20 +177,14 @@ public void Encode<TPixel>(Image<TPixel> image, Stream stream, CancellationToken
173177
WriteHeader(stream);
174178

175179
// Write the LSD.
176-
int derivedTransparencyIndex = GetTransparentIndex(quantized, null);
177-
if (derivedTransparencyIndex >= 0)
180+
int transparencyIndex = GetTransparentIndex(quantized, null);
181+
if (transparencyIndex >= 0)
178182
{
179183
frameMetadata.HasTransparency = true;
180-
frameMetadata.TransparencyIndex = ClampIndex(derivedTransparencyIndex);
184+
frameMetadata.TransparencyIndex = ClampIndex(transparencyIndex);
181185
}
182186

183-
// TODO: We should be checking the metadata here also I think?
184-
if (!TryGetBackgroundIndex(quantized, this.backgroundColor, out byte backgroundIndex))
185-
{
186-
backgroundIndex = derivedTransparencyIndex >= 0
187-
? frameMetadata.TransparencyIndex
188-
: gifMetadata.BackgroundColorIndex;
189-
}
187+
byte backgroundIndex = GetBackgroundIndex(quantized, gifMetadata, this.backgroundColor);
190188

191189
// Get the number of bits.
192190
int bitDepth = ColorNumerics.GetBitsNeededForColorDepth(quantized.Palette.Length);
@@ -224,7 +222,7 @@ public void Encode<TPixel>(Image<TPixel> image, Stream stream, CancellationToken
224222
image,
225223
globalQuantizer,
226224
globalFrameQuantizer,
227-
derivedTransparencyIndex,
225+
transparencyIndex,
228226
frameMetadata.DisposalMode,
229227
cancellationToken);
230228
}
@@ -334,15 +332,23 @@ private void EncodeAdditionalFrame<TPixel>(
334332
{
335333
// Capture any explicit transparency index from the metadata.
336334
// We use it to determine the value to use to replace duplicate pixels.
337-
int transparencyIndex = metadata.HasTransparency ? metadata.TransparencyIndex : -1;
335+
bool useTransparency = metadata.HasTransparency;
336+
int transparencyIndex = useTransparency ? metadata.TransparencyIndex : -1;
338337

339338
ImageFrame<TPixel>? previous = previousDisposalMode == FrameDisposalMode.RestoreToBackground
340339
? null :
341340
previousFrame;
342341

343-
Color background = metadata.DisposalMode == FrameDisposalMode.RestoreToBackground
344-
? this.backgroundColor ?? Color.Transparent
345-
: Color.Transparent;
342+
// If the previous frame has a value we need to check the disposal mode of that frame
343+
// to determine if we should use the background color to fill the encoding frame
344+
// when de-duplicating.
345+
FrameDisposalMode disposalMode = previous is null ?
346+
metadata.DisposalMode :
347+
previous.Metadata.GetGifMetadata().DisposalMode;
348+
349+
Color background = !useTransparency && disposalMode == FrameDisposalMode.RestoreToBackground
350+
? this.backgroundColor ?? Color.Transparent
351+
: Color.Transparent;
346352

347353
// Deduplicate and quantize the frame capturing only required parts.
348354
(bool difference, Rectangle bounds) =
@@ -491,6 +497,7 @@ private IndexedImageFrame<TPixel> QuantizeFrameAndUpdateMetadata<TPixel>(
491497
return quantized;
492498
}
493499

500+
[MethodImpl(MethodImplOptions.AggressiveInlining)]
494501
private static byte ClampIndex(int value) => (byte)Numerics.Clamp(value, byte.MinValue, byte.MaxValue);
495502

496503
/// <summary>
@@ -531,41 +538,38 @@ private static int GetTransparentIndex<TPixel>(IndexedImageFrame<TPixel>? quanti
531538
/// Returns the index of the background color in the palette.
532539
/// </summary>
533540
/// <param name="quantized">The current quantized frame.</param>
541+
/// <param name="metadata">The gif metadata</param>
534542
/// <param name="background">The background color to match.</param>
535-
/// <param name="index">The index in the palette of the background color.</param>
536543
/// <typeparam name="TPixel">The pixel format.</typeparam>
537-
/// <returns>The <see cref="bool"/>.</returns>
538-
private static bool TryGetBackgroundIndex<TPixel>(
539-
IndexedImageFrame<TPixel>? quantized,
540-
Color? background,
541-
out byte index)
544+
/// <returns>The <see cref="byte"/> index of the background color.</returns>
545+
private static byte GetBackgroundIndex<TPixel>(IndexedImageFrame<TPixel>? quantized, GifMetadata metadata, Color? background)
542546
where TPixel : unmanaged, IPixel<TPixel>
543547
{
544548
int match = -1;
545-
if (quantized != null && background.HasValue)
549+
if (quantized != null)
546550
{
547-
TPixel backgroundPixel = background.Value.ToPixel<TPixel>();
548-
ReadOnlySpan<TPixel> palette = quantized.Palette.Span;
549-
for (int i = 0; i < palette.Length; i++)
551+
if (background.HasValue)
550552
{
551-
if (!backgroundPixel.Equals(palette[i]))
553+
TPixel backgroundPixel = background.Value.ToPixel<TPixel>();
554+
ReadOnlySpan<TPixel> palette = quantized.Palette.Span;
555+
for (int i = 0; i < palette.Length; i++)
552556
{
553-
continue;
554-
}
557+
if (!backgroundPixel.Equals(palette[i]))
558+
{
559+
continue;
560+
}
555561

556-
match = i;
557-
break;
562+
match = i;
563+
break;
564+
}
565+
}
566+
else if (metadata.BackgroundColorIndex < quantized.Palette.Length)
567+
{
568+
match = metadata.BackgroundColorIndex;
558569
}
559570
}
560571

561-
if (match >= 0)
562-
{
563-
index = (byte)Numerics.Clamp(match, 0, 255);
564-
return true;
565-
}
566-
567-
index = 0;
568-
return false;
572+
return ClampIndex(match);
569573
}
570574

571575
/// <summary>

‎tests/ImageSharp.Tests/Formats/Gif/GifDecoderTests.cs‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -398,4 +398,14 @@ public void Issue2953<TPixel>(TestImageProvider<TPixel> provider)
398398
Assert.Throws<InvalidImageContentException>(() => Image.Identify(options, testFile.FullPath));
399399
Assert.Throws<InvalidImageContentException>(() => Image.Load(options, testFile.FullPath));
400400
}
401+
402+
[Theory]
403+
[WithFile(TestImages.Gif.Issues.Issue2980, PixelTypes.Rgba32)]
404+
public void Issue2980<TPixel>(TestImageProvider<TPixel> provider)
405+
where TPixel : unmanaged, IPixel<TPixel>
406+
{
407+
using Image<TPixel> image = provider.GetImage();
408+
image.DebugSaveMultiFrame(provider);
409+
image.CompareToReferenceOutputMultiFrame(provider, ImageComparer.Exact);
410+
}
401411
}

‎tests/ImageSharp.Tests/Quantization/QuantizedImageTests.cs‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,6 @@ public void OctreeQuantizerYieldsCorrectTransparentPixel<TPixel>(
5353
where TPixel : unmanaged, IPixel<TPixel>
5454
{
5555
using Image<TPixel> image = provider.GetImage();
56-
Assert.True(image[0, 0].Equals(default));
5756

5857
QuantizerOptions options = new();
5958
if (!dither)
@@ -79,7 +78,6 @@ public void WuQuantizerYieldsCorrectTransparentPixel<TPixel>(TestImageProvider<T
7978
where TPixel : unmanaged, IPixel<TPixel>
8079
{
8180
using Image<TPixel> image = provider.GetImage();
82-
Assert.True(image[0, 0].Equals(default));
8381

8482
QuantizerOptions options = new();
8583
if (!dither)

‎tests/ImageSharp.Tests/TestImages.cs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -582,6 +582,7 @@ public static class Issues
582582
public const string Issue2859_A = "Gif/issues/issue_2859_A.gif";
583583
public const string Issue2859_B = "Gif/issues/issue_2859_B.gif";
584584
public const string Issue2953 = "Gif/issues/issue_2953.gif";
585+
public const string Issue2980 = "Gif/issues/issue_2980.gif";
585586
}
586587

587588
public static readonly string[] Animated =

‎tests/Images/External/ReferenceOutput/GifDecoderTests/Decode_Animated_WithTransparency_Rgba32_animated_transparent_firstframerestoreprev_loop.gif/00.png‎

Lines changed: 2 additions & 2 deletions
Loading

‎tests/Images/External/ReferenceOutput/GifDecoderTests/Decode_Animated_WithTransparency_Rgba32_animated_transparent_firstframerestoreprev_loop.gif/01.png‎

Lines changed: 2 additions & 2 deletions
Loading

‎tests/Images/External/ReferenceOutput/GifDecoderTests/Decode_Animated_WithTransparency_Rgba32_animated_transparent_firstframerestoreprev_loop.gif/02.png‎

Lines changed: 2 additions & 2 deletions
Loading

‎tests/Images/External/ReferenceOutput/GifDecoderTests/Decode_Animated_WithTransparency_Rgba32_animated_transparent_firstframerestoreprev_loop.gif/03.png‎

Lines changed: 2 additions & 2 deletions
Loading

‎tests/Images/External/ReferenceOutput/GifDecoderTests/Decode_Animated_WithTransparency_Rgba32_animated_transparent_frame_norestore_loop.gif/00.png‎

Lines changed: 2 additions & 2 deletions
Loading

0 commit comments

Comments
 (0)