Normalize and cleanup encoders#2269
Conversation
JimBobSquarePants
requested review from
antonfirsov,
brianpopow,
dlemstra and
tocsoft
gfoidl
left a comment
gfoidl
left a comment
There was a problem hiding this comment.
Just eye-rolled product code changes.
src/ImageSharp/Processing/Processors/Quantization/DefaultPixelSamplingStrategy.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Günther Foidl <gue@korporal.at>
…SamplingStrategy.cs Co-authored-by: Günther Foidl <gue@korporal.at>
|
Will take a look at this within a day or two! |
antonfirsov
left a comment
antonfirsov
left a comment
There was a problem hiding this comment.
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.
| 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; } | ||
| } |
There was a problem hiding this comment.
I can't find any code utilizing these abstractions in a meaningful way. Wouldn't it be better to drop them?
There was a problem hiding this comment.
Yeah, happy to use the base classes instead.
| /// <summary> | ||
| /// The base class for all image encoders that allow color palette generation via quantization. | ||
| /// </summary> | ||
| public abstract class QuantizingImageEncoder : ImageEncoder, IQuantizingEncoderOptions |
There was a problem hiding this comment.
[Note on asymmetry, not to be addressed.] Also weird that we have subclassing while with decoders the structure is flat.
There was a problem hiding this comment.
I actually want to subclass in the decoders. Just trying to figure out a nice way.
| public TPixel this[int x, int y] | ||
| { | ||
| [MethodImpl(InliningOptions.ShortMethod)] | ||
| [MethodImpl(MethodImplOptions.AggressiveInlining)] |
There was a problem hiding this comment.
Any reason for the InliningOptions -> MethodImplOptions changes?
There was a problem hiding this comment.
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 ;))
There was a problem hiding this comment.
Makes sense for these 2 methods actually.
| public override void Encode<TPixel>(Image<TPixel> image, Stream stream) | ||
| { | ||
| var encoder = new BmpEncoderCore(this, image.GetMemoryAllocator()); | ||
| BmpEncoderCore encoder = new(this, image.GetMemoryAllocator()); |
There was a problem hiding this comment.
It's easy to overlook important semantic changes if there are so many style refactorings in the same PR.
There was a problem hiding this comment.
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
Outdated
Show resolved
Hide resolved
| frameQuantizer.BuildPalette(this.pixelSamplingStrategy, image.Frames.RootFrame); | ||
| quantized = frameQuantizer.QuantizeFrame(image.Frames.RootFrame, image.Bounds()); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
| } | ||
|
|
||
| /// <inheritdoc /> | ||
| public IEnumerable<Buffer2DRegion<TPixel>> EnumeratePixelRegions<TPixel>(ImageFrame<TPixel> frame) |
There was a problem hiding this comment.
The multi-frame overload has some unit tests, might be worth to test this one too.
4 participants
Prerequisites
Description
There's no point trying to copy the options pattern that we introduced in #2180 as the requirements are different.
Instead, I've refactored the encoders to do the following
ImageEncoderandQuantizingImageEncoderwith shared, preconfigured options properties. Two new shared options propertiesSkipMetadataandPixelSamplingStrategyhave been introduced into the respective types.QuantizingImageEncoderinstance to use the sampling strategy from the options. This applies to local and global palette generation.