Expose and conserve the color palette for indexed png images. #2485

JimBobSquarePants · 2023-06-26T05:41:33Z

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

This builds upon #2455 and should be merged after.

Changes the following in PngMetadata:

Exposes a new ReadonlyMemory<Color> ColorTable property.
Merges TransparentRgb24, TransparentRgb48, TransparentL8 and, TransparentL16 into a new nullable TransParentColor Color? property.
Removes HasTransparency since we now know that from the palette or transparent color.

On index Png encode, as with Gif if a palette is found in the metadata the quantizer will use that instead.

JimBobSquarePants · 2023-09-18T06:02:53Z

@SixLabors/core This one next please 🙏

gfoidl · 2023-09-18T07:23:40Z

src/ImageSharp/Formats/Png/PngDecoderCore.cs

+            for (int i = 0; i < alpha.Length; i++)
+            {
+                ref Color color = ref colorTable[i];
+                color = color.WithAlpha(alpha[i] / 255F);


If perf is important here, then

Suggested change

color = color.WithAlpha(alpha[i] / 255F);

color = color.WithAlpha(alpha[i] * (1F / 255F));

will produce a mul instead of div.

Even if perf isn't critical here I think that I'd change the code, as

it's almost as readable

it's not only about the perf of this piece of code, but rather for the whole system, as the cpu-ports can be utilized better when there's no division clogging it

Note: it's Roslyn to blame here for not being able to emit 1 / 255F automatically -- though the JIT could do that optimization too, but it has more stringent timing constraints...

This would not be a valid optimization for either compiler to make, as the results are not the same (due to 1/255f not being precisely representable in float). See https://sharplab.io/#v2:EYLgxg9gTgpgtADwGwBYA0AXEUCuA7AHwAEAmARgFgAoIgBgAIiyA6AGQEs8BHAbmqIDMjEvQDC9AN7V6M4SKlVZS+gDNoMAIZgAFvQAUnDPQBu9TvQCieHAFsYUDcAA2MZgCUNeAOYw9tNPQkAKxBAJShktLK0ewq+qYA9IEhcQCEALwm9ABU+mQJwUEq4VHRZYxkAJx6ACQARBLGAL4gkonJRU30GW05eQUpoU11oXyKZU2l9JNUTUA===

For that reason, unless it's a hot path, this optimization loses precision for no measurable benefit.

as the results are not the same

Yep, but the error is in the region of 1e-8 or less (demo)

In that value range, yes, but the magnitude of the error is relative to the magnitude of the value -- that's how floating point works. Whether the precision loss is acceptable in any given case is a question of the trade-off between performance (~12 cycles for divss vs ~4 for mulss) and precision.

I'm not familiar enough with the code to know whether that trade-off is worthwhile here (though it looks questionable). Mostly, I was commenting on your suggestion that Roslyn and RyuJIT were somehow missing an optimization one of them could have made automatically, which is incorrect.

which is incorrect

You are right (and I have to apologize here by Roslyn).
I had a look what GCC does (before writing the first comment on this) and it emitted the mul -- but I had -Ofast compiler switch set which disregards strict standards compliance (as I re-read just now). With -O{2,3} it's a div.

Thanks for the heads up!

gfoidl · 2023-09-18T07:25:18Z

src/ImageSharp/Formats/Png/PngEncoderCore.cs

@@ -875,7 +875,7 @@ private void WriteGammaChunk(Stream stream)
            // 4-byte unsigned integer of gamma * 100,000.
            uint gammaValue = (uint)(this.gamma * 100_000F);

-            BinaryPrimitives.WriteUInt32BigEndian(this.chunkDataBuffer.Span.Slice(0, 4), gammaValue);
+            BinaryPrimitives.WriteUInt32BigEndian(this.chunkDataBuffer.Span[..4], gammaValue);


FYI: the Slice here was IIRC intentional, as it produces better code than the slice. There are issues in Roslyn and runtime to care about that, but for .NET 8 these aren't resolved (when I'm not mistaken).

We're just taking the hit here. One day Roslyn will catch up, but we need greater consistency to ease the (massive) maintenance burden. I'm hoping come v4/.NET8 with the SIMD migration improvements I have planned we won't notice codegen issues.

gfoidl · 2023-09-18T07:25:44Z

src/ImageSharp/Formats/Png/PngEncoderCore.cs

@@ -1175,7 +1177,7 @@ private void WriteChunk(Stream stream, PngChunkType type, Span<byte> data, int o

        stream.Write(buffer);

-        uint crc = Crc32.Calculate(buffer.Slice(4)); // Write the type buffer
+        uint crc = Crc32.Calculate(buffer[4..]); // Write the type buffer


JimBobSquarePants · 2023-09-23T01:49:44Z

@antonfirsov The changes here are based upon the changes made to the gif format API. If you have a chance, I'd love to get your feedback please.

JimBobSquarePants · 2023-09-25T09:57:32Z

@SixLabors/core Can I get a review here please.

antonfirsov · 2023-09-25T15:07:51Z

@JimBobSquarePants will do it by EOW if that's good enough.

JimBobSquarePants · 2023-09-25T23:45:20Z

@JimBobSquarePants will do it by EOW if that's good enough.

Thanks! Appreciate it.

antonfirsov

A few suggestions, but mostly LGTM.

antonfirsov · 2023-10-02T16:22:24Z

src/ImageSharp/Formats/Png/PngDecoderCore.cs

+        ReadOnlySpan<Rgb24> rgbTable = MemoryMarshal.Cast<byte, Rgb24>(palette);
+        for (int i = 0; i < colorTable.Length; i++)
+        {
+            colorTable[i] = new Color(rgbTable[i]);
+        }


Nit: I see several loops like this one in codecs now. We can maybe define a public static Color.FromPixel<TPixel>(Span<TPixel>, Span<Color>) analogous to the bulk ToPixel<TPixel>(...) method.

Without knowing the component precision of the TPixel we don't know which boxed pixel to assign to. We could do it in v4 though if we added additional information to PixelTypeInfo #2534 (comment)

Ah yeah this indeed goes well beyond this PR, nevermind then.

tests/ImageSharp.Tests/Formats/Png/PngEncoderTests.cs

JimBobSquarePants added 2 commits June 26, 2023 00:42

Allow preservation of png color palette

037d6ea

Fix Color.ToHex()

d7709ee

JimBobSquarePants added enhancement formats:png breaking Signifies a binary breaking change. labels Jun 26, 2023

JimBobSquarePants added this to the v3.1.0 milestone Jun 26, 2023

JimBobSquarePants requested review from tocsoft, antonfirsov, dlemstra and brianpopow June 26, 2023 05:41

JimBobSquarePants added 5 commits July 5, 2023 23:22

Merge branch 'js/gif-fixes' into js/png-pallete

3325380

Merge branch 'js/gif-fixes' into js/png-pallete

fdbaafe

Merge branch 'js/gif-fixes' into js/png-pallete

f38ebfb

Merge branch 'js/gif-fixes' into js/png-pallete

c3ed86d

Use different default pixel sampling strategy for PNG

9787761

JimBobSquarePants mentioned this pull request Aug 14, 2023

Weird PNG resaving issue, broken image? #2498

Closed

4 tasks

JimBobSquarePants added 2 commits August 17, 2023 22:25

Merge branch 'js/gif-fixes' into js/png-pallete

c7da070

Merge branch 'main' into js/png-pallete

78fd1b9

gfoidl reviewed Sep 18, 2023

View reviewed changes

JimBobSquarePants added 2 commits September 22, 2023 17:03

Use safe iteration for reading color palette.

6ad24af

Update WuQuantizer{TPixel}.cs

e995a13

Merge branch 'main' into js/png-pallete

eb5aec5

Add tests for high precision Color cases

fa434dd

antonfirsov reviewed Oct 2, 2023

View reviewed changes

Update PngEncoderTests.cs

31b591a

antonfirsov approved these changes Oct 3, 2023

View reviewed changes

JimBobSquarePants merged commit e905b0a into main Oct 3, 2023
8 checks passed

JimBobSquarePants deleted the js/png-pallete branch October 3, 2023 23:00

JimBobSquarePants mentioned this pull request Oct 3, 2023

APNG support #2511

Merged

4 tasks

antonfirsov mentioned this pull request Oct 4, 2023

Expose non-nullable configuration to remove AOT limiting null check #2514

Merged

4 tasks

antonfirsov mentioned this pull request Dec 11, 2023

Promote PixelTypeInfo to Pixel #2601

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose and conserve the color palette for indexed png images. #2485

Expose and conserve the color palette for indexed png images. #2485

JimBobSquarePants commented Jun 26, 2023

JimBobSquarePants commented Sep 18, 2023

gfoidl Sep 18, 2023

saucecontrol Sep 18, 2023

gfoidl Sep 18, 2023

saucecontrol Sep 18, 2023

gfoidl Sep 18, 2023

gfoidl Sep 18, 2023

JimBobSquarePants Sep 19, 2023

gfoidl Sep 18, 2023

JimBobSquarePants commented Sep 23, 2023

JimBobSquarePants commented Sep 25, 2023

antonfirsov commented Sep 25, 2023

JimBobSquarePants commented Sep 25, 2023

antonfirsov left a comment

antonfirsov Oct 2, 2023

JimBobSquarePants Oct 3, 2023

antonfirsov Oct 3, 2023

	color = color.WithAlpha(alpha[i] / 255F);
	color = color.WithAlpha(alpha[i] * (1F / 255F));

Expose and conserve the color palette for indexed png images. #2485

Expose and conserve the color palette for indexed png images. #2485

Conversation

JimBobSquarePants commented Jun 26, 2023

Prerequisites

Description

JimBobSquarePants commented Sep 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JimBobSquarePants commented Sep 23, 2023

JimBobSquarePants commented Sep 25, 2023

antonfirsov commented Sep 25, 2023

JimBobSquarePants commented Sep 25, 2023

antonfirsov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment