Skip to content

Introduce Shared General Decoder Options plus Specialization #2180

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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 41 commits into from
Aug 29, 2022
Merged

Introduce Shared General Decoder Options plus Specialization #2180

Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
41 commits
Select commit Hold shift + click to select a range
4099205
Stub out options types.
JimBobSquarePants Jun 29, 2022
dfe04e3
Refactor Bmp and Gif
JimBobSquarePants Jun 29, 2022
d453922
Port Pbm decoder
JimBobSquarePants Jun 29, 2022
c9beb02
Port Tga and Png
JimBobSquarePants Jun 29, 2022
3917b12
Port Tiff, fix constructors
JimBobSquarePants Jun 29, 2022
48a8106
Port WebP and cleanup options
JimBobSquarePants Jun 29, 2022
22421db
Limit to collection max
JimBobSquarePants Jun 29, 2022
653351c
Merge branch 'main' into js/decoder-options
JimBobSquarePants Jul 17, 2022
d53ad0e
Refactor jpeg decoder and general load
JimBobSquarePants Jul 17, 2022
1c2d1f4
Update tests and fix issues
JimBobSquarePants Jul 17, 2022
8d50996
Fix cache key resolution
JimBobSquarePants Jul 17, 2022
6dbc5ec
Fix param caching test
JimBobSquarePants Jul 17, 2022
730b215
Merge branch 'main' into js/decoder-options
JimBobSquarePants Jul 18, 2022
ae2fe1a
Merge branch 'main' into js/decoder-options
JimBobSquarePants Jul 18, 2022
688b9ef
Merge branch 'main' into js/decoder-options
JimBobSquarePants Jul 18, 2022
a42101e
Migrate new Tiff WebPDecoder
JimBobSquarePants Jul 18, 2022
204fddd
Pass cancellation token to decompressors.
JimBobSquarePants Jul 18, 2022
c823323
Merge branch 'main' into js/decoder-options
JimBobSquarePants Jul 18, 2022
1d9b209
Update TestImageExtensions.cs
JimBobSquarePants Jul 18, 2022
8826e87
Update src/ImageSharp/Formats/DecoderOptions.cs
JimBobSquarePants Jul 18, 2022
2355de6
Update src/ImageSharp/Image.FromBytes.cs
JimBobSquarePants Jul 18, 2022
01e497f
Update src/ImageSharp/Image.FromBytes.cs
JimBobSquarePants Jul 18, 2022
91a6413
Update src/ImageSharp/Image.FromBytes.cs
JimBobSquarePants Jul 18, 2022
37d020c
Update src/ImageSharp/Image.FromBytes.cs
JimBobSquarePants Jul 18, 2022
085ff1a
Normalize not loaded exception
JimBobSquarePants Jul 18, 2022
0472ffd
Make default DecoderOptions internal
JimBobSquarePants Jul 30, 2022
d29cf8a
Implement specialized options for limited types, add extensions.
JimBobSquarePants Aug 7, 2022
07e8578
Merge branch 'main' into js/decoder-options
JimBobSquarePants Aug 7, 2022
ac4fb62
Only expose sanitized decoder methods.
JimBobSquarePants Aug 8, 2022
0e13e6c
Add sampler option, add comment.
JimBobSquarePants Aug 8, 2022
f78ec51
Add jpeg tests
JimBobSquarePants Aug 17, 2022
75b0e07
Merge branch 'main' into js/decoder-options
JimBobSquarePants Aug 17, 2022
c5c1575
Fix warnings
JimBobSquarePants Aug 17, 2022
a4c14e7
Add additional tests
JimBobSquarePants Aug 20, 2022
acbc7bc
Update PngDecoderTests.cs
JimBobSquarePants Aug 20, 2022
6242723
Update PngDecoderTests.cs
JimBobSquarePants Aug 20, 2022
3b6b30a
Change percentages for mac
JimBobSquarePants Aug 20, 2022
efb2511
Fix whitespace
JimBobSquarePants Aug 20, 2022
751bf48
Tweak WebP test percentage for MacOS
JimBobSquarePants Aug 20, 2022
52a26d1
Merge branch 'main' into js/decoder-options
JimBobSquarePants Aug 28, 2022
92ac2d7
Merge branch 'main' into js/decoder-options
JimBobSquarePants Aug 29, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 11 additions & 13 deletions src/ImageSharp/Advanced/AotCompilerTools.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,7 @@ private static unsafe void AotCompileImage<TPixel>()
img.CloneAs<Short2>(default);
img.CloneAs<Short4>(default);

ImageFrame.LoadPixelData<TPixel>(default, default(ReadOnlySpan<TPixel>), default, default);
ImageFrame.LoadPixelData(default, default(ReadOnlySpan<TPixel>), default, default);
ImageFrame.LoadPixelData<TPixel>(default, default(ReadOnlySpan<byte>), default, default);
}

Expand Down Expand Up @@ -210,21 +210,21 @@ private static void AotCompileImageEncoderInternals<TPixel>()
}

/// <summary>
/// This method pre-seeds the all <see cref="IImageDecoderInternals"/> in the AoT compiler.
/// This method pre-seeds the all <see cref="IImageDecoderInternals{T}"/> in the AoT compiler.
/// </summary>
/// <typeparam name="TPixel">The pixel format.</typeparam>
[Preserve]
private static void AotCompileImageDecoderInternals<TPixel>()
where TPixel : unmanaged, IPixel<TPixel>
{
default(WebpDecoderCore).Decode<TPixel>(default, default, default);
default(BmpDecoderCore).Decode<TPixel>(default, default, default);
default(GifDecoderCore).Decode<TPixel>(default, default, default);
default(JpegDecoderCore).Decode<TPixel>(default, default, default);
default(PbmDecoderCore).Decode<TPixel>(default, default, default);
default(PngDecoderCore).Decode<TPixel>(default, default, default);
default(TgaDecoderCore).Decode<TPixel>(default, default, default);
default(TiffDecoderCore).Decode<TPixel>(default, default, default);
default(WebpDecoderCore).Decode<TPixel>(default, default);
default(BmpDecoderCore).Decode<TPixel>(default, default);
default(GifDecoderCore).Decode<TPixel>(default, default);
default(JpegDecoderCore).Decode<TPixel>(default, default);
default(PbmDecoderCore).Decode<TPixel>(default, default);
default(PngDecoderCore).Decode<TPixel>(default, default);
default(TgaDecoderCore).Decode<TPixel>(default, default);
default(TiffDecoderCore).Decode<TPixel>(default, default);
}

/// <summary>
Expand Down Expand Up @@ -286,9 +286,7 @@ private static void AotCompileImageEncoder<TPixel, TEncoder>()
private static void AotCompileImageDecoder<TPixel, TDecoder>()
where TPixel : unmanaged, IPixel<TPixel>
where TDecoder : class, IImageDecoder
{
default(TDecoder).Decode<TPixel>(default, default, default);
}
=> default(TDecoder).Decode<TPixel>(default, default, default);

/// <summary>
/// This method pre-seeds the all <see cref="IImageProcessor" /> in the AoT compiler.
Expand Down
38 changes: 13 additions & 25 deletions src/ImageSharp/Formats/Bmp/BmpDecoder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,43 +10,31 @@ namespace SixLabors.ImageSharp.Formats.Bmp
/// <summary>
/// Image decoder for generating an image out of a Windows bitmap stream.
/// </summary>
/// <remarks>
/// Does not support the following formats at the moment:
/// <list type="bullet">
/// <item>JPG</item>
/// <item>PNG</item>
/// <item>Some OS/2 specific subtypes like: Bitmap Array, Color Icon, Color Pointer, Icon, Pointer.</item>
/// </list>
/// Formats will be supported in a later releases. We advise always
/// to use only 24 Bit Windows bitmaps.
/// </remarks>
public sealed class BmpDecoder : IImageDecoder, IBmpDecoderOptions, IImageInfoDetector
public class BmpDecoder : ImageDecoder<BmpDecoderOptions>
{
/// <summary>
/// Gets or sets a value indicating how to deal with skipped pixels, which can occur during decoding run length encoded bitmaps.
/// </summary>
public RleSkippedPixelHandling RleSkippedPixelHandling { get; set; } = RleSkippedPixelHandling.Black;

/// <inheritdoc/>
public Image<TPixel> Decode<TPixel>(Configuration configuration, Stream stream, CancellationToken cancellationToken)
where TPixel : unmanaged, IPixel<TPixel>
public override Image<TPixel> DecodeSpecialized<TPixel>(BmpDecoderOptions options, Stream stream, CancellationToken cancellationToken)
{
Guard.NotNull(stream, nameof(stream));

var decoder = new BmpDecoderCore(configuration, this);
return decoder.Decode<TPixel>(configuration, stream, cancellationToken);
BmpDecoderCore decoder = new(options);
Image<TPixel> image = decoder.Decode<BmpDecoderOptions, TPixel>(options.GeneralOptions.Configuration, stream, cancellationToken);

Resize(options.GeneralOptions, image);

return image;
}

/// <inheritdoc />
public Image Decode(Configuration configuration, Stream stream, CancellationToken cancellationToken)
=> this.Decode<Rgba32>(configuration, stream, cancellationToken);
/// <inheritdoc/>
public override Image DecodeSpecialized(BmpDecoderOptions options, Stream stream, CancellationToken cancellationToken)
=> this.DecodeSpecialized<Rgba32>(options, stream, cancellationToken);

/// <inheritdoc/>
public IImageInfo Identify(Configuration configuration, Stream stream, CancellationToken cancellationToken)
public override IImageInfo IdentifySpecialized(BmpDecoderOptions options, Stream stream, CancellationToken cancellationToken)
{
Guard.NotNull(stream, nameof(stream));

return new BmpDecoderCore(configuration, this).Identify(configuration, stream, cancellationToken);
return new BmpDecoderCore(options).Identify(options.GeneralOptions.Configuration, stream, cancellationToken);
}
}
}
41 changes: 19 additions & 22 deletions src/ImageSharp/Formats/Bmp/BmpDecoderCore.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ namespace SixLabors.ImageSharp.Formats.Bmp
/// <remarks>
/// A useful decoding source example can be found at <see href="https://dxr.mozilla.org/mozilla-central/source/image/decoders/nsBMPDecoder.cpp"/>
/// </remarks>
internal sealed class BmpDecoderCore : IImageDecoderInternals
internal sealed class BmpDecoderCore : IImageDecoderInternals<BmpDecoderOptions>
{
/// <summary>
/// The default mask for the red part of the color for 16 bit rgb bitmaps.
Expand Down Expand Up @@ -90,33 +90,30 @@ internal sealed class BmpDecoderCore : IImageDecoderInternals
private BmpInfoHeader infoHeader;

/// <summary>
/// Used for allocating memory during processing operations.
/// The global configuration.
/// </summary>
private readonly MemoryAllocator memoryAllocator;
private readonly Configuration configuration;

/// <summary>
/// The bitmap decoder options.
/// Used for allocating memory during processing operations.
/// </summary>
private readonly IBmpDecoderOptions options;
private readonly MemoryAllocator memoryAllocator;

/// <summary>
/// Initializes a new instance of the <see cref="BmpDecoderCore"/> class.
/// </summary>
/// <param name="configuration">The configuration.</param>
/// <param name="options">The options.</param>
public BmpDecoderCore(Configuration configuration, IBmpDecoderOptions options)
public BmpDecoderCore(BmpDecoderOptions options)
{
this.Configuration = configuration;
this.memoryAllocator = configuration.MemoryAllocator;
this.options = options;
this.Options = options;
this.configuration = options.GeneralOptions.Configuration;
this.memoryAllocator = this.configuration.MemoryAllocator;
}

/// <inheritdoc />
public Configuration Configuration { get; }
public BmpDecoderOptions Options { get; }

/// <summary>
/// Gets the dimensions of the image.
/// </summary>
/// <inheritdoc />
public Size Dimensions => new(this.infoHeader.Width, this.infoHeader.Height);

/// <inheritdoc />
Expand All @@ -128,7 +125,7 @@ public Image<TPixel> Decode<TPixel>(BufferedReadStream stream, CancellationToken
{
int bytesPerColorMapEntry = this.ReadImageHeaders(stream, out bool inverted, out byte[] palette);

image = new Image<TPixel>(this.Configuration, this.infoHeader.Width, this.infoHeader.Height, this.metadata);
image = new Image<TPixel>(this.configuration, this.infoHeader.Width, this.infoHeader.Height, this.metadata);

Buffer2D<TPixel> pixels = image.GetRootFramePixelBuffer();

Expand Down Expand Up @@ -325,7 +322,7 @@ private void ReadRle<TPixel>(BmpCompression compression, Buffer2D<TPixel> pixels
byte colorIdx = bufferRow[x];
if (undefinedPixelsSpan[rowStartIdx + x])
{
switch (this.options.RleSkippedPixelHandling)
switch (this.Options.RleSkippedPixelHandling)
{
case RleSkippedPixelHandling.FirstColorOfPalette:
color.FromBgr24(Unsafe.As<byte, Bgr24>(ref colors[colorIdx * 4]));
Expand Down Expand Up @@ -397,7 +394,7 @@ private void ReadRle24<TPixel>(Buffer2D<TPixel> pixels, int width, int height, b
int idx = rowStartIdx + (x * 3);
if (undefinedPixelsSpan[yMulWidth + x])
{
switch (this.options.RleSkippedPixelHandling)
switch (this.Options.RleSkippedPixelHandling)
{
case RleSkippedPixelHandling.FirstColorOfPalette:
color.FromBgr24(Unsafe.As<byte, Bgr24>(ref bufferSpan[idx]));
Expand Down Expand Up @@ -943,7 +940,7 @@ private void ReadRgb24<TPixel>(Buffer2D<TPixel> pixels, int width, int height, b
int newY = Invert(y, height, inverted);
Span<TPixel> pixelSpan = pixels.DangerousGetRowSpan(newY);
PixelOperations<TPixel>.Instance.FromBgr24Bytes(
this.Configuration,
this.configuration,
rowSpan,
pixelSpan,
width);
Expand Down Expand Up @@ -971,7 +968,7 @@ private void ReadRgb32Fast<TPixel>(Buffer2D<TPixel> pixels, int width, int heigh
int newY = Invert(y, height, inverted);
Span<TPixel> pixelSpan = pixels.DangerousGetRowSpan(newY);
PixelOperations<TPixel>.Instance.FromBgra32Bytes(
this.Configuration,
this.configuration,
rowSpan,
pixelSpan,
width);
Expand Down Expand Up @@ -1006,7 +1003,7 @@ private void ReadRgb32Slow<TPixel>(Buffer2D<TPixel> pixels, int width, int heigh
this.stream.Read(rowSpan);

PixelOperations<Bgra32>.Instance.FromBgra32Bytes(
this.Configuration,
this.configuration,
rowSpan,
bgraRowSpan,
width);
Expand Down Expand Up @@ -1042,7 +1039,7 @@ private void ReadRgb32Slow<TPixel>(Buffer2D<TPixel> pixels, int width, int heigh
Span<TPixel> pixelSpan = pixels.DangerousGetRowSpan(newY);

PixelOperations<TPixel>.Instance.FromBgra32Bytes(
this.Configuration,
this.configuration,
rowSpan,
pixelSpan,
width);
Expand All @@ -1056,7 +1053,7 @@ private void ReadRgb32Slow<TPixel>(Buffer2D<TPixel> pixels, int width, int heigh
{
this.stream.Read(rowSpan);
PixelOperations<Bgra32>.Instance.FromBgra32Bytes(
this.Configuration,
this.configuration,
rowSpan,
bgraRowSpan,
width);
Expand Down
20 changes: 20 additions & 0 deletions src/ImageSharp/Formats/Bmp/BmpDecoderOptions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
// Copyright (c) Six Labors.
// Licensed under the Six Labors Split License.

namespace SixLabors.ImageSharp.Formats.Bmp
{
/// <summary>
/// Configuration options for decoding Windows Bitmap images.
/// </summary>
public sealed class BmpDecoderOptions : ISpecializedDecoderOptions
{
/// <inheritdoc/>
public DecoderOptions GeneralOptions { get; set; } = new();
Copy link
Member

@antonfirsov antonfirsov Jul 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This is the only specialized decoder option class that actually contains a feature. All the others are dummies, their only purpose is to satisfy the pattern introduced. I believe this is alarming.

Copy link
Member Author

@JimBobSquarePants JimBobSquarePants Jul 30, 2022

Choose a reason for hiding this comment

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

I wanted something that was guaranteed to handle some of the more exotic codes that we'll end up implementing so that we don't have to further breaks. I can imagine both AVIF and Jpeg-XL requiring customization.

I'm actually very surprised we don't already have something for Tiff and I believe that is most likely an oversight thus far.

Copy link
Member

@antonfirsov antonfirsov Aug 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Even with Bmp + Tiff + AVIF we would have too much of boilerplate of generics and unnecessary empty classes IMO. And Tiff + AVIF are purely hypothetical at the moment, we don't have a proof this feature really needed & will be in fact implemented in the near future. I would prefer a design, where we could YAGNI this, and postpone creating specialized types until the moment they are proven to be actually needed for a format.

On top of that, I realized now, that we are creating an API disparity between decoders and encoders:
Decoders are becoming stateless objects with this PR with separate types for decoding options, while encoder objects carry the encoding options.

I know that I was an advocate of stateless decoders/encoders, but now I disagree with my past self and it feels like the simplest thing would be to move the format specific options back to the decoder objects, so users could do specialized decoding the following way:

var decoder = new BmpDecoder { RleSkippedPixelHandling = RleSkippedPixelHandling.Transparent };
using Image img = decoder.Decode(...);

Alternatively, we can also consider turning the current abstaction into a pattern, but this would not solve the decoder-encoder API disparity:

public class ImageDecoder : IImageDecoder {
    public Image<TPixel> Decode<TPixel>(DecoderOptions options, Stream stream, CancellationToken cancellationToken);
}

public class BmpDecoderOptions {
    public DecoderOptions GeneralOptions { get; set; }
    public RleSkippedPixelHandling RleSkippedPixelHandling { get; set; }
}

public BmpDecoder : ImageDecoder {
    public Image<TPixel> DecodeSpecialized(BmpDecoderOptions options, Stream stream, CancellationToken cancellationToken);
}

Copy link
Member Author

@JimBobSquarePants JimBobSquarePants Aug 2, 2022

Choose a reason for hiding this comment

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

On top of that, I realized now, that we are creating an API disparity between decoders and encoders:
Decoders are becoming stateless objects with this PR with separate types for decoding options, while encoder objects carry the encoding options.

I think we'll end up doing something similar to encoders anyway.

How would you enforce the options pattern for specialized decoders? I really want to ensure that for specialization we are forced into following something.

The problem I see with the pattern above is that now you actually have more boilerplate for non-specialized decoding since you now have to implement the resize functionality for each concrete Decode implementation.

Copy link
Member Author

@JimBobSquarePants JimBobSquarePants Aug 2, 2022

Choose a reason for hiding this comment

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

Maybe we can revert the specialized options and make the decoder stateful again?

DecodeSpecialized is simply the concrete implementation and each decoder then becomes responsible for passing addition options to the core.

Copy link
Member

@antonfirsov antonfirsov Aug 2, 2022

Choose a reason for hiding this comment

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

Actually, it can be useful as a potential solution to the problem described in #2180 (comment).

Copy link
Member Author

@JimBobSquarePants JimBobSquarePants Aug 2, 2022

Choose a reason for hiding this comment

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

I don't see why is the ImageDecoderSpecialized base type needed. Am I missing something?

Yeah, I guess that could be simply an interface unless we implement overloads. The ImageDecoder actually handles the meat now.

I don't feel the need for the verbose name.

The reason it's verbose is so I can do this with the JpegDecoder. We need a method that allows IDCT resize only and creating a JpegDecoderOptions type as you've identified is not great.

/// <summary>
/// Decoder for generating an image out of a jpeg encoded stream.
/// </summary>
public sealed class JpegDecoder : ImageDecoderSpecialized<DecoderOptions>
{
    /// <inheritdoc/>
    public override IImageInfo Identify(DecoderOptions options, Stream stream, CancellationToken cancellationToken)
    {
        Guard.NotNull(stream, nameof(stream));

        using JpegDecoderCore decoder = new(options);
        return decoder.Identify(options.Configuration, stream, cancellationToken);
    }

    /// <inheritdoc/>
    public override Image<TPixel> Decode<TPixel>(DecoderOptions options, Stream stream, CancellationToken cancellationToken)
    {
        Image<TPixel> image = this.DecodeSpecialized<TPixel>(options, stream, cancellationToken);

        Resize(options, image);

        return image;
    }

    /// <inheritdoc/>
    public override Image Decode(DecoderOptions options, Stream stream, CancellationToken cancellationToken)
    {
        Image image = this.DecodeSpecialized(options, stream, cancellationToken);

        Resize(options, image);

        return image;
    }

    /// <inheritdoc/>
    /// <remarks>
    /// Unlike <see cref="IImageDecoder.Decode{TPixel}(DecoderOptions, Stream, CancellationToken)"/>, when
    /// <see cref="DecoderOptions.TargetSize"/> is passed, the codec may not be able to scale efficiently to
    /// the exact scale factor requested, so returns a size that approximates that scale.
    /// Upscaling is not supported, so the original size will be returned.
    /// </remarks>
    public override Image<TPixel> DecodeSpecialized<TPixel>(DecoderOptions options, Stream stream, CancellationToken cancellationToken)
    {
        using JpegDecoderCore decoder = new(options);
        return decoder.Decode<TPixel>(options.Configuration, stream, cancellationToken);
    }

    /// <inheritdoc/>
    /// <remarks>
    /// Unlike <see cref="IImageDecoder.Decode(DecoderOptions, Stream, CancellationToken)"/>, when
    /// <see cref="DecoderOptions.TargetSize"/> is passed, the codec may not be able to scale efficiently to
    /// the exact scale factor requested, so returns a size that approximates that scale.
    /// Upscaling is not supported, so the original size will be returned.
    /// </remarks>
    public override Image DecodeSpecialized(DecoderOptions options, Stream stream, CancellationToken cancellationToken)
        => this.DecodeSpecialized<Rgb24>(options, stream, cancellationToken);
}

Copy link
Member

@antonfirsov antonfirsov Aug 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

We need a method that allows IDCT resize only

JpegDecoder : ImageDecoderSpecialized<DecoderOptions>

To me it looks very unfortunate to use the general DecoderOptions as specialized options just to tell that a user wants an IDCT resize only. If I would look for such API the last thing I would expect that I just simply need to call a method called DecodeSpcialized with the same args.

I would rather introduce JpegDecoderOptions with a bool IdctResize property, or create another, jpeg-spcific DecodeIdctResize method, or simply keep the "IDCT resize only" feature out of the public API. It's a niche use-case users would only use if they are unhappy with our default TargetSize decoding and need better control on the Resize phase. (Which IMHO would mean that we are doing something wrong and maybe we should reconsider exposing ResizeOptions.)

Copy link
Member

@antonfirsov antonfirsov Aug 3, 2022

Choose a reason for hiding this comment

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

If we want maximum flexibility, I would consider something like this for Jpeg:

public enum JpegResizeMode
{
    Combined, // This is what we do today
    IdctOnly, // IDCT-only to nearest block scale
    ScaleOnly // Opt-out the IDCT part and only Resize. Can be useful in case of quality concerns.
}

public class JpegDecoderOptions
{
    public DecoderOptions GeneralOptions { get; set; }
    public JpegResizeMode ResizeMode { get; set; } = JpegResizeMode.Combined;
}

Copy link
Member Author

@JimBobSquarePants JimBobSquarePants Aug 4, 2022

Choose a reason for hiding this comment

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

OK. I love this!


/// <summary>
/// Gets or sets the value indicating how to deal with skipped pixels,
/// which can occur during decoding run length encoded bitmaps.
/// </summary>
public RleSkippedPixelHandling RleSkippedPixelHandling { get; set; }
}
}
16 changes: 0 additions & 16 deletions src/ImageSharp/Formats/Bmp/IBmpDecoderOptions.cs
View file
Open in desktop

This file was deleted.

42 changes: 42 additions & 0 deletions src/ImageSharp/Formats/DecoderOptions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
// Copyright (c) Six Labors.
// Licensed under the Six Labors Split License.

using System;

namespace SixLabors.ImageSharp.Formats
{
/// <summary>
/// Provides general configuration options for decoding image formats.
/// </summary>
public sealed class DecoderOptions
{
private static readonly Lazy<DecoderOptions> LazyOptions = new(() => new());

private uint maxFrames = int.MaxValue;

/// <summary>
/// Gets the shared default general decoder options instance.
/// </summary>
public static DecoderOptions Default { get; } = LazyOptions.Value;
Copy link
Member

@antonfirsov antonfirsov Jul 29, 2022

Choose a reason for hiding this comment

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

This is a mutable global state. Was it meant to be? It introduces a second global configuration point to the library in parallel to Configuration.Default, which I think is unfortunate.

Personally, I don't see a good use-case for changing these settings globally, so I would just make it internal.

Copy link
Member Author

@JimBobSquarePants JimBobSquarePants Jul 30, 2022

Choose a reason for hiding this comment

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

Yep agreed.


/// <summary>
/// Gets or sets a custom Configuration instance to be used by the image processing pipeline.
/// </summary>
public Configuration Configuration { get; set; } = Configuration.Default;

/// <summary>
/// Gets or sets the target size to decode the image into.
/// </summary>
public Size? TargetSize { get; set; } = null;
Copy link
Member

@antonfirsov antonfirsov Jul 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

It looks like we are missing tests validating various usages of TargetSize (decode to specific size & keep aspect ratio). These tests can be also used as code samples to ask some community members to do a final sanity-check on the target size decoding API.

Copy link
Member Author

@JimBobSquarePants JimBobSquarePants Jul 30, 2022

Choose a reason for hiding this comment

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

You mean validating the result of given dimensions? I would have thought that would already be covered by ResizeHelperTests? We simply pass the size to that and JpegDecoderCore already has tests for scaled decoding.

Copy link
Member

@antonfirsov antonfirsov Aug 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I mean end-to-end testing resize-decode the way users will work with the feature, including aspect-ratio preservation. The only setter call I see for DecoderOptions.TargetSize is in LoadResizeSaveStressRunner.

Even though currently it's implemented with a simple Resize, it's a separate feature that needs testing. Later it could be optimized by utilizing resizer internals (feeding the image to ResizeWorker row-by-row).

and JpegDecoderCore already has tests for scaled decoding

I only see unit tests for SpectralConverter.CalculateResultingImageSize. Or am I missing something?

Copy link
Contributor

@br3aker br3aker Aug 1, 2022

Choose a reason for hiding this comment

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

I only see unit tests for SpectralConverter.CalculateResultingImageSize. Or am I missing something?

Yep, I've left actual image tests to the moment when API is ready as jpeg resize-decode entry point was somewhat of a stub. As that was internal we didn't risk exposing invalid code to users but this PR is a game changer. I know this solution isn't 'synchronized' but I'll write proper test suite for jpeg after API is set in stone.

Copy link
Member

@antonfirsov antonfirsov Aug 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I'm fine delaying it for jpeg, the implementation is somewhat special anyways, but we could add test for another decoder (eg. PNG) in this PR to validate the feature. Then later we can extend as we optimize decoders.


/// <summary>
/// Gets or sets a value indicating whether to ignore encoded metadata when decoding.
/// </summary>
public bool SkipMetadata { get; set; } = false;

/// <summary>
/// Gets or sets the maximum number of image frames to decode, inclusive.
/// </summary>
public uint MaxFrames { get => this.maxFrames; set => this.maxFrames = Math.Min(Math.Max(value, 1), int.MaxValue); }
}
}
35 changes: 13 additions & 22 deletions src/ImageSharp/Formats/Gif/GifDecoder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,45 +3,36 @@

using System.IO;
using System.Threading;
using SixLabors.ImageSharp.Metadata;
using SixLabors.ImageSharp.PixelFormats;

namespace SixLabors.ImageSharp.Formats.Gif
{
/// <summary>
/// Decoder for generating an image out of a gif encoded stream.
/// </summary>
public sealed class GifDecoder : IImageDecoder, IGifDecoderOptions, IImageInfoDetector
public sealed class GifDecoder : ImageDecoder<GifDecoderOptions>
{
/// <summary>
/// Gets or sets a value indicating whether the metadata should be ignored when the image is being decoded.
/// </summary>
public bool IgnoreMetadata { get; set; } = false;

/// <summary>
/// Gets or sets the decoding mode for multi-frame images
/// </summary>
public FrameDecodingMode DecodingMode { get; set; } = FrameDecodingMode.All;

/// <inheritdoc/>
public Image<TPixel> Decode<TPixel>(Configuration configuration, Stream stream, CancellationToken cancellationToken)
where TPixel : unmanaged, IPixel<TPixel>
public override Image<TPixel> DecodeSpecialized<TPixel>(GifDecoderOptions options, Stream stream, CancellationToken cancellationToken)
{
var decoder = new GifDecoderCore(configuration, this);
return decoder.Decode<TPixel>(configuration, stream, cancellationToken);
GifDecoderCore decoder = new(options);
Image<TPixel> image = decoder.Decode<GifDecoderOptions, TPixel>(options.GeneralOptions.Configuration, stream, cancellationToken);

Resize(options.GeneralOptions, image);

return image;
}

/// <inheritdoc />
public Image Decode(Configuration configuration, Stream stream, CancellationToken cancellationToken)
=> this.Decode<Rgba32>(configuration, stream, cancellationToken);
/// <inheritdoc/>
public override Image DecodeSpecialized(GifDecoderOptions options, Stream stream, CancellationToken cancellationToken)
=> this.DecodeSpecialized<Rgba32>(options, stream, cancellationToken);

/// <inheritdoc/>
public IImageInfo Identify(Configuration configuration, Stream stream, CancellationToken cancellationToken)
public override IImageInfo IdentifySpecialized(GifDecoderOptions options, Stream stream, CancellationToken cancellationToken)
{
Guard.NotNull(stream, nameof(stream));

var decoder = new GifDecoderCore(configuration, this);
return decoder.Identify(configuration, stream, cancellationToken);
return new GifDecoderCore(options).Identify(options.GeneralOptions.Configuration, stream, cancellationToken);
}
}
}
Loading