Normalize and cleanup encoders #2269

JimBobSquarePants · 2022-10-16T11:56:36Z

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

There's no point trying to copy the options pattern that we introduced in #2180 as the requirements are different.

We DON'T want to allow passing specific DECODERS to the General API.
We DO want to allow passing specific ENCODERS to the General API.

Instead, I've refactored the encoders to do the following

Introduced 2 new abstract types ImageEncoder and QuantizingImageEncoder with shared, preconfigured options properties. Two new shared options properties SkipMetadata and PixelSamplingStrategy have been introduced into the respective types.
Inlined the individual encoder options interfaces into the concrete implementations and removed any duplication (no more separate mutable options types)
Made the individual encoder options immutable.
Refactored each encoder to allow skipping metadata on encode - This allows us to encode images without metadata while preserving the metadata in the decoded image.
Refactored each QuantizingImageEncoder instance to use the sampling strategy from the options. This applies to local and global palette generation.
General cleanup in files I was altering.

br3aker

Don't have time for a proper look but it definitely looks interesting.

src/ImageSharp/Formats/Jpeg/JpegEncoder.cs

gfoidl

Just eye-rolled product code changes.

src/ImageSharp/Formats/Png/PngEncoderCore.cs

src/ImageSharp/Processing/Processors/Quantization/DefaultPixelSamplingStrategy.cs

Co-authored-by: Günther Foidl <gue@korporal.at>

…SamplingStrategy.cs Co-authored-by: Günther Foidl <gue@korporal.at>

antonfirsov · 2022-10-24T12:25:15Z

Will take a look at this within a day or two!

antonfirsov

My review assumes that we are OK with the general design direction, pointing out a few minor issues, while the PR looks mostly good.

However:
While I do understand the differences in the requirements, I'm still not happy with the decoder-encoder API asymmetry. It feels weird, and I still think we can do better. We should re-evaluate the tradeoffs of different decoder/encoder design strategies.

That should be another iteration/discussion though, since this PR contains way too many changes better to be merged sooner than later.

antonfirsov · 2022-10-26T19:23:18Z

src/ImageSharp/Formats/IEncoderOptions.cs

+public interface IEncoderOptions
+{
+    /// <summary>
+    /// Gets a value indicating whether to ignore decoded metadata when encoding.
+    /// </summary>
+    bool SkipMetadata { get; init; }
+}
+
+/// <summary>
+/// Defines the contract for encoder options that allow color palette generation via quantization.
+/// </summary>
+public interface IQuantizingEncoderOptions : IEncoderOptions
+{
+    /// <summary>
+    /// Gets the quantizer used to generate the color palette.
+    /// </summary>
+    IQuantizer Quantizer { get; init; }
+
+    /// <summary>
+    /// Gets the <see cref="IPixelSamplingStrategy"/> used for quantization when building color palettes.
+    /// </summary>
+    IPixelSamplingStrategy PixelSamplingStrategy { get; init; }
+}


I can't find any code utilizing these abstractions in a meaningful way. Wouldn't it be better to drop them?

Yeah, happy to use the base classes instead.

antonfirsov · 2022-10-26T19:26:46Z

src/ImageSharp/Formats/ImageEncoder.cs

+/// <summary>
+/// The base class for all image encoders that allow color palette generation via quantization.
+/// </summary>
+public abstract class QuantizingImageEncoder : ImageEncoder, IQuantizingEncoderOptions


[Note on asymmetry, not to be addressed.] Also weird that we have subclassing while with decoders the structure is flat.

I actually want to subclass in the decoders. Just trying to figure out a nice way.

antonfirsov · 2022-10-26T19:32:52Z

src/ImageSharp/Image{TPixel}.cs

@@ -181,7 +173,7 @@ public new ImageFrameCollection<TPixel> Frames
    /// <exception cref="ArgumentOutOfRangeException">Thrown when the provided (x,y) coordinates are outside the image boundary.</exception>
    public TPixel this[int x, int y]
    {
-        [MethodImpl(InliningOptions.ShortMethod)]
+        [MethodImpl(MethodImplOptions.AggressiveInlining)]


Any reason for the InliningOptions -> MethodImplOptions changes?

I think we should be using the custom methods more sparingly on areas we're actively profiling. (I recall you saying similar a few years back now ;))

Makes sense for these 2 methods actually.

antonfirsov · 2022-10-26T19:39:30Z

src/ImageSharp/Formats/Bmp/BmpEncoder.cs

    {
-        var encoder = new BmpEncoderCore(this, image.GetMemoryAllocator());
+        BmpEncoderCore encoder = new(this, image.GetMemoryAllocator());


It's easy to overlook important semantic changes if there are so many style refactorings in the same PR.

Yeah, but every time I open a file just now there's a heap of warnings from the new rules. I'm adopting the boy scout rule.

src/ImageSharp/Processing/Processors/Quantization/DefaultPixelSamplingStrategy.cs

antonfirsov · 2022-10-26T20:04:28Z

src/ImageSharp/Formats/Gif/GifEncoderCore.cs

+                frameQuantizer.BuildPalette(this.pixelSamplingStrategy, image.Frames.RootFrame);
+                quantized = frameQuantizer.QuantizeFrame(image.Frames.RootFrame, image.Bounds());


Do I understand it correctly that this introduces an optimization for single-frame gifs? Can't remember why didn't I add it originally single-frame, but would be nice to see a performance comparison. I assume there is no impact on the output quality?

Yeah. It is but it only affects 4K or greater images by default. I didn't introduce a change to the default dimension limit as I didn't want to affect quality on smaller images.

antonfirsov · 2022-10-26T20:07:34Z

src/ImageSharp/Processing/Processors/Quantization/DefaultPixelSamplingStrategy.cs

@@ -101,4 +103,55 @@ Buffer2DRegion<TPixel> GetRow(int pos)
            }
        }
    }
+
+    /// <inheritdoc />
+    public IEnumerable<Buffer2DRegion<TPixel>> EnumeratePixelRegions<TPixel>(ImageFrame<TPixel> frame)


The multi-frame overload has some unit tests, might be worth to test this one too.

JimBobSquarePants added 15 commits October 13, 2022 13:03

Define base classes and options

e238cc6

Migrate Gif encoder

52bac6f

Migrate Bmp encoder

a824703

Merge branch 'main' into js/encoder-normalization

e58f292

BmpEncoder uses Octree as default

ca51d41

Fix frame enumeration and clean up

073cefd

Use sampling strategy in BmpEncoder

715b0ba

Convert jpeg encoder.

6485dde

Convert PbmEncoder. Make options immutable

8c90f78

Migrate PngEncoder and fix tests

4499df7

Migrate TgaEncoder

b88b181

TgaEncoder options should be immutable.

f0b935e

Convert TiffEncoder, Use sampling strategy for local palette building

1ac9de5

Convert WebpEncoder

2fc9719

Allow skipping Tiff metadata

37d4ed2

JimBobSquarePants added area:formats API labels Oct 16, 2022

JimBobSquarePants added this to the 3.0.0 milestone Oct 16, 2022

JimBobSquarePants requested review from brianpopow, antonfirsov, tocsoft and dlemstra October 16, 2022 11:56

br3aker reviewed Oct 16, 2022

View reviewed changes

src/ImageSharp/Formats/Jpeg/JpegEncoder.cs Outdated Show resolved Hide resolved

gfoidl reviewed Oct 17, 2022

View reviewed changes

src/ImageSharp/Formats/Png/PngEncoderCore.cs Outdated Show resolved Hide resolved

src/ImageSharp/Processing/Processors/Quantization/DefaultPixelSamplingStrategy.cs Outdated Show resolved Hide resolved

JimBobSquarePants mentioned this pull request Oct 20, 2022

Replace ImageExtensions.Save.tt with Source Generator #2237

Open

4 tasks

JimBobSquarePants and others added 4 commits October 24, 2022 21:01

Merge branch 'main' into js/encoder-normalization

ebdf5c8

Update src/ImageSharp/Formats/Png/PngEncoderCore.cs

6745cae

Co-authored-by: Günther Foidl <gue@korporal.at>

Update src/ImageSharp/Processing/Processors/Quantization/DefaultPixel…

45a7b8b

…SamplingStrategy.cs Co-authored-by: Günther Foidl <gue@korporal.at>

Build fixes and feedback

30a245a

antonfirsov reviewed Oct 26, 2022

View reviewed changes

JimBobSquarePants and others added 6 commits October 27, 2022 19:48

Remove IEncoderOptions

48b3abe

Use DebugGuard

307a3ba

Update PixelSamplingStrategyTests.cs

af7eadd

Fix sampling tests

e5df749

Use DebugGuard

830d3e2

Move check outside of the loop

864735f

antonfirsov approved these changes Oct 28, 2022

View reviewed changes

JimBobSquarePants merged commit b404826 into main Oct 28, 2022

JimBobSquarePants deleted the js/encoder-normalization branch October 28, 2022 11:04

KLuuKer mentioned this pull request Jul 28, 2023

Weird PNG resaving issue, broken image? #2498

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Normalize and cleanup encoders #2269

Normalize and cleanup encoders #2269

JimBobSquarePants commented Oct 16, 2022 •

edited

Loading

br3aker left a comment •

edited

Loading

gfoidl left a comment

antonfirsov commented Oct 24, 2022

antonfirsov left a comment

antonfirsov Oct 26, 2022 •

edited

Loading

JimBobSquarePants Oct 26, 2022

JimBobSquarePants Oct 27, 2022

antonfirsov Oct 26, 2022 •

edited

Loading

JimBobSquarePants Oct 26, 2022

antonfirsov Oct 26, 2022

JimBobSquarePants Oct 26, 2022

antonfirsov Oct 27, 2022

antonfirsov Oct 26, 2022

JimBobSquarePants Oct 26, 2022

antonfirsov Oct 26, 2022

JimBobSquarePants Oct 26, 2022

antonfirsov Oct 26, 2022

JimBobSquarePants Oct 26, 2022

JimBobSquarePants Oct 27, 2022

		frameQuantizer.BuildPalette(this.pixelSamplingStrategy, image.Frames.RootFrame);
		quantized = frameQuantizer.QuantizeFrame(image.Frames.RootFrame, image.Bounds());

Normalize and cleanup encoders #2269

Normalize and cleanup encoders #2269

Conversation

JimBobSquarePants commented Oct 16, 2022 • edited Loading

Prerequisites

Description

br3aker left a comment • edited Loading

Choose a reason for hiding this comment

gfoidl left a comment

Choose a reason for hiding this comment

antonfirsov commented Oct 24, 2022

antonfirsov left a comment

Choose a reason for hiding this comment

antonfirsov Oct 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antonfirsov Oct 26, 2022 • edited Loading

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

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 Oct 16, 2022 •

edited

Loading

br3aker left a comment •

edited

Loading

antonfirsov Oct 26, 2022 •

edited

Loading

antonfirsov Oct 26, 2022 •

edited

Loading