Target Image<TPixel> loading/decoding APIs #1490

Sergio0694 · 2021-01-03T22:24:54Z

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 matches 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)

Closes #1487

Description

This PR adds new APIs to load/decode images directly into reused Image<TPixel> instances.

Details

I've decided to add the overloads for the Image<TPixel> class to improve interoperability with other APIs.
I didn't feel great about either adding a new, separate IImageMemoryDecoder type, so this should help with consustency 😄

API additions (click to expand):

namespace SixLabors.ImageSharp.Formats
{
    public interface IImageDecoder
    {
        void Decode<TPixel>(Stream stream, Image<TPixel> image)
            where TPixel : unmanaged, IPixel<TPixel>;
        Task DecodeAsync<TPixel>(Stream stream, Image<TPixel> image, CancellationToken cancellationToken)
            where TPixel : unmanaged, IPixel<TPixel>;
    }
}

namespace SixLabors.ImageSharp
{
    public abstract partial class Image
    {
        // byte[]
        public static void Load<TPixel>(byte[] data, Image<TPixel> image, out IImageFormat format)
            where TPixel : unmanaged, IPixel<TPixel>;
        public static void Load<TPixel>(byte[] data, Image<TPixel> image, IImageDecoder decoder)
            where TPixel : unmanaged, IPixel<TPixel>;

        // ReadOnlySpan<byte>
        public static unsafe void Load<TPixel>(
            ReadOnlySpan<byte> data,
            Image<TPixel> image,
            IImageDecoder decoder)
            where TPixel : unmanaged, IPixel<TPixel>;
        public static unsafe void Load<TPixel>(
            ReadOnlySpan<byte> data,
            Image<TPixel> image,
            out IImageFormat format)
            where TPixel : unmanaged, IPixel<TPixel>;

        // Stream
        public static void Load<TPixel>(Stream stream, Image<TPixel> image, IImageDecoder decoder)
            where TPixel : unmanaged, IPixel<TPixel>;
        public static Task LoadAsync<TPixel>(
            Stream stream,
            Image<TPixel> image,
            IImageDecoder decoder,
            CancellationToken cancellationToken = default)
            where TPixel : unmanaged, IPixel<TPixel>;
        public static void Load<TPixel>(Stream stream, Image<TPixel> image, out IImageFormat format)
            where TPixel : unmanaged, IPixel<TPixel>;
        public static async Task<IImageFormat> LoadWithFormatAsync<TPixel>(
            Stream stream,
            Image<TPixel> image,
            CancellationToken cancellationToken = default)
            where TPixel : unmanaged, IPixel<TPixel>;

        // string
        public static Task LoadAsync<TPixel>(
            string path,
            Image<TPixel> image,
            IImageDecoder decoder,
            CancellationToken cancellationToken = default)
            where TPixel : unmanaged, IPixel<TPixel>;
        public static void Load<TPixel>(string path, Image<TPixel> image, out IImageFormat format)
            where TPixel : unmanaged, IPixel<TPixel>;
        public static void Load<TPixel>(string path, Image<TPixel> image, IImageDecoder decoder)
            where TPixel : unmanaged, IPixel<TPixel>;
    }
}

NOTE: opening the PR to have the CI run on it, but I still need to add unit tests.

Use case example

The new APIs allow for eg. GPU processing (assuming DirectX 12 buffers on NUMA architecture) like so (using ComputeSharp):

IImageInfo info = Image.Identify("cat.jpg", out IImageFormat format);

using ReadWriteBuffer<uint> gpuBuffer = Gpu.Default.AllocateReadWriteBuffer<uint>(info.Width * info.Height);

using (var buffer = gpuBuffer.GetBufferForWrite())
fixed (void* pointer = buffer.Span)
using (var image = Image.WrapMemory<Rgb24>(pointer, info.Width, info.Height))
{
    Image.Load("cat.jpg", image, out _);
}

// Do GPU processing...

using (var buffer = gpuBuffer.GetBufferForRead())
fixed (void* pointer = buffer.Span)
using (var image = Image.WrapMemory<Rgb24>(pointer, info.Width, info.Height))
{
    image.SaveAsJpeg("cat_processed.jpg");
}

The big advantage is that we can completely skip one entire copy of the image, as we can use the new APIs to load/decode the image directly into the temporary upload buffer on the GPU. Without these APIs, we'd instead have to load the image into CPU memory, then copy from there to the temporary upload GPU buffer, then copy again from there to the shader visible GPU buffer.

Open questions

❓ Right now I just throw a NotSupportedException for GIF files. Is this ok?
❓ Right now I just throw an ArgumentException if the image has the wrong size. Do we want to try to mutate it instead?

Co-authored-by: Clinton Ingram <[email protected]>

…geSharp into js/Shuffle3Channel

Fix JpegDecoderTests.Decode_IsCancellable

DaZombieKiller · 2021-01-04T04:35:36Z

src/ImageSharp/Image.cs

@@ -67,7 +67,7 @@ protected Image(Configuration configuration, PixelTypeInfo pixelType, ImageMetad
        public int Height => this.size.Height;

        /// <inheritdoc/>
-        public ImageMetadata Metadata { get; }
+        public ImageMetadata Metadata { get; internal set; }


Should there be a public API for setting this somewhere? With the new setter being internal, a third party image decoder can't properly re-use an Image as it can't change the metadata.

You set individual properties on the metadata not the metadata itself.

See ImageMetadata

I see, that makes sense. Seeing different *Metadata classes had me confused for a moment, not noticing that ImageMetadata is sealed.

@Sergio0694 we should rather fill the existing metadata in the decoders. Reasons: (1) We don't want the setter to be public. (2) 3rd party decoders should be able to function the same way our decoders do.

antonfirsov

We should decide on API questions before proceeding.

I like the idea of having a Load(Async) overloads that work on an existing Image<T> instance over the static variants in the original proposal, since they separate the concerns of wrapping the memory and loading the image. This reduces the number of overloads we need to create and maintain to NumOf(WrapVariants) + NumOf(LoadVariants) from NumOf(WrapVariants) * NumOf(LoadVariants). (The numbers will likely keep growing.) On the other hand, the user needs to know the dimensions in advance, I'm not sure if it's always possible. @DaZombieKiller? (Also see my notes in the very end.)

If we stick with the instance API, we need to decide on the location of the new overloads. Here my preference is to have them as extension methods in either AdvancedImageExtensions or a new extension class under the Advanced namespace.

We need to decide if we are ok with the breaking changes of extending IImageDecoder (break a small number of advanced users to deliver a feature for another group of advanced users ... and kinda also break our promise to not break).

@JimBobSquarePants suggestions?

Right now I just throw an ArgumentException if the image has the wrong size. Do we want to try to mutate it instead?

If the stream image size is bigger, we clearly need to throw. For other cases, I see 3 options here:

The user wants to change dimensions (mutate)
The user wants to decode to a sub rectangle of the target image
The user is expecting an exception if sizes mismatch (current behavior)

antonfirsov · 2021-01-04T16:49:30Z

src/ImageSharp/Image.FromStream.cs

+        /// <exception cref="UnknownImageFormatException">Image format not recognised.</exception>
+        /// <exception cref="InvalidImageContentException">Image contains invalid content.</exception>
+        /// <typeparam name="TPixel">The pixel format.</typeparam>
+        public static void Load<TPixel>(Stream stream, Image<TPixel> image, IImageDecoder decoder)


I would either make these instance methods on Image or extension methods in AdvancedImageExtensions.

I'd say AdvancedImageExtensions

antonfirsov · 2021-01-04T16:53:33Z

src/ImageSharp/Image.cs

@@ -67,7 +67,7 @@ protected Image(Configuration configuration, PixelTypeInfo pixelType, ImageMetad
        public int Height => this.size.Height;

        /// <inheritdoc/>
-        public ImageMetadata Metadata { get; }
+        public ImageMetadata Metadata { get; internal set; }


@Sergio0694 we should rather fill the existing metadata in the decoders. Reasons: (1) We don't want the setter to be public. (2) 3rd party decoders should be able to function the same way our decoders do.

antonfirsov · 2021-01-04T17:00:34Z

src/ImageSharp/Formats/IImageDecoder.cs

+        /// <param name="stream">The <see cref="Stream"/> containing image data.</param>
+        /// <param name="image">The target <see cref="Image{TPixel}"/> instance.</param>
+        // TODO: Document ImageFormatExceptions (https://github.com/SixLabors/ImageSharp/issues/1110)
+        void Decode<TPixel>(Stream stream, Image<TPixel> image)


This addition is gonna be a breaking change for users who implemented their own IImageDecoder. The reason @DaZombieKiller proposed IImageMemoryDecoder was to avoid this.

I'm undecided on this, with a slight preference to not break.

Hmmmm.... It's a fairly fundamental break and there are decoder out in the wild. I say new interface.

In that case, now comes the hard part: naming it. IImageMemoryDecoder doesn't seem descriptive enough, especially since it's no longer operating on Memory<T> like in the original proposal. Maybe IImageTargetDecoder or IImageTargetedDecoder, based on the name of this PR?

That also gives rise to another open question: What is expected to happen if you attempt to decode into an Image with a decoder that doesn't support doing that? It could potentially execute a slow path with a copy, but since this is an API intended for high-performance scenarios I think throwing makes more sense.

We'd have to expose the interface and use the type as the method parameter type. Can't call what you don't have then.

What about the overloads that don't take an IImageDecoder currently? Would they just be removed?

When someone calls the new Image.Load without a specified decoder we will loop through our config to find a matching decoder that implements the interface. If there is none it will through.

I generally don't recommend people use specific decoders unless they have to as they skip the format validation checks.

antonfirsov · 2021-01-04T17:13:23Z

src/ImageSharp/Formats/Jpeg/JpegDecoderCore.cs

+            {
+                if (this.ImageWidth != image.Width || this.ImageHeight != image.Height)
+                {
+                    ThrowHelper.ThrowArgumentException("The input image has an invalid size", nameof(image));


Suggested change

ThrowHelper.ThrowArgumentException("The input image has an invalid size", nameof(image));

ThrowHelper.ThrowArgumentException("The input image should match the dimensions of the image in the stream.", nameof(image));

Same for the other decoders. In Jpeg we can throw on this much earlier in the decoding workflow.

DaZombieKiller · 2021-01-04T19:03:35Z

@antonfirsov

On the other hand, the user needs to know the dimensions in advance, I'm not sure if it's always possible. @DaZombieKiller?

In my use case they're always known as I just use Image.Identify to work out the dimensions and format so I can create a texture to fill.

Add PremultiplyAlpha to ResizeOptions

Fix for Issue #1505

…rsion Vectorize Jpeg Encoder Color Conversion

Assembly for loading in the loop went from: ```asm vmovss xmm2, [rax] vbroadcastss xmm2, xmm2 vmovss xmm3, [rax+4] vbroadcastss xmm3, xmm3 vinsertf128 ymm2, ymm2, xmm3, 1 ``` To: ```asm vmovsd xmm3, [rax] vbroadcastsd ymm3, xmm3 vpermps ymm3, ymm1, ymm3 ```

See Vector256.Create issue: dotnet/runtime#47236

Speed improvements to resize kernel (w/ SIMD)

JimBobSquarePants and others added 30 commits October 30, 2020 20:38

Initial 3padshuffle4

49e9364

Add Shuffle4Slice3

1d21dc9

Cleanup

9f38d40

Merge branch 'master' into js/Shuffle3Channel

2421a56

fix spans directly

1b85483

Faster Pad3Shuffle4

21611e1

Faster Shuffle4Slice3

f462bfe

Update benchmark

2d1f2cc

Fast fallbacks

d5b2577

Don't cast full spans

893bfdd

Shuffle3 + Tests

76d5277

Cleanup and fix tests

49062c4

Fix Shuffle4Slice3, wire up shuffles.

8c32469

Add Rgb24 <==> Vector4 benchmarks

1f73b21

Add initial vectorized color converter implementation

210d8f7

Unroll XYZWShuffle4Slice3

a08f906

Fix shuffle +m slice fallback

4416d3d

Refactor JpegColorConverters

f421be2

Inline controls as constants

11cc6af

Refactor and add tests

7cc953e

Add benchmarks

89bb9fe

Drop FromGrayscaleVector8

d76dbaa

Polish benchmarks (fix new warnings)

82a2359

Fix #1414

5748d3d

Merge branch 'master' into af/fix-Decode_IsCancellable

73454f5

Handle Bmp encoder padding.

1ad9fcd

Update src/ImageSharp/Common/Helpers/SimdUtils.HwIntrinsics.cs

e1168ad

Co-authored-by: Clinton Ingram <[email protected]>

Merge branch 'js/Shuffle3Channel' of https://github.com/SixLabors/Ima…

a46fb9b

…geSharp into js/Shuffle3Channel

Use ROS trick all round and optimize Shuffle3

74dd8cd

Merge pull request #1415 from SixLabors/af/fix-Decode_IsCancellable

cf9cc6b

Fix JpegDecoderTests.Decode_IsCancellable

DaZombieKiller reviewed Jan 4, 2021

View reviewed changes

antonfirsov reviewed Jan 4, 2021

View reviewed changes

ptasev and others added 24 commits January 12, 2021 20:15

Add PremultiplyAlpha to ResizeOptions

933dada

Split PixelConversionModifiers into a separate function.

900b73e

Updated referenced image submodule to latest origin master.

770dc06

Use this.maxColors when getting size of the reduced palette, fixes #1505

f85b686

Add test case for #1505

0830a97

Merge pull request #1504 from ptasev/pt/resize-alpha-option

da7a8b7

Add PremultiplyAlpha to ResizeOptions

Merge branch 'master' into bp/Issue1505

b3146a7

Merge pull request #1506 from SixLabors/bp/Issue1505

e2961dc

Fix for Issue #1505

Add initial vectorized implementation with benchmarks

c602dd7

Fix mistakes in final touches

db6f90a

Add unit tests for both converters

51ce97f

Allow epsilon of 1F for existing LUT converter

47b0d9f

Improve algorithm

91b18b1

Merge pull request #1508 from tkp1n/feature/vectorize-rgb2ycbcr-conve…

eab04e4

…rsion Vectorize Jpeg Encoder Color Conversion

Add initial FMA resize kernel convolve implementation

42632c7

Switch from FMA to AVX2 instructions

874e951

Revert to FMA, codegen improvements

941e173

Add unrolled FMA loop

493d04a

Add missing indexing update

407c2d9

Workaround for incorrect codegen on .NET 5

a7ca1b0

See Vector256.Create issue: dotnet/runtime#47236

Update image threshold for resize tests

e2211c3

Merge pull request #1513 from SixLabors/sp/simd-resize-convolve

7eb5cc0

Speed improvements to resize kernel (w/ SIMD)

Merge branch 'master' into sp/target-image-decoding

8f6e3a6

JimBobSquarePants closed this Feb 17, 2021

JimBobSquarePants force-pushed the master branch from db51f69 to 172c48e Compare February 17, 2021 01:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Target Image<TPixel> loading/decoding APIs #1490

Target Image<TPixel> loading/decoding APIs #1490

Sergio0694 commented Jan 3, 2021 •

edited

Loading

DaZombieKiller Jan 4, 2021 •

edited

Loading

JimBobSquarePants Jan 4, 2021

DaZombieKiller Jan 4, 2021

antonfirsov Jan 4, 2021 •

edited

Loading

antonfirsov left a comment •

edited

Loading

antonfirsov Jan 4, 2021

JimBobSquarePants Jan 21, 2021

antonfirsov Jan 4, 2021 •

edited

Loading

antonfirsov Jan 4, 2021 •

edited

Loading

JimBobSquarePants Jan 5, 2021

DaZombieKiller Jan 5, 2021 •

edited

Loading

JimBobSquarePants Jan 5, 2021

DaZombieKiller Jan 5, 2021

JimBobSquarePants Jan 5, 2021

antonfirsov Jan 4, 2021 •

edited

Loading

DaZombieKiller commented Jan 4, 2021

	ThrowHelper.ThrowArgumentException("The input image has an invalid size", nameof(image));
	ThrowHelper.ThrowArgumentException("The input image should match the dimensions of the image in the stream.", nameof(image));

Target Image<TPixel> loading/decoding APIs #1490

Target Image<TPixel> loading/decoding APIs #1490

Conversation

Sergio0694 commented Jan 3, 2021 • edited Loading

Prerequisites

Closes #1487

Description

Details

Use case example

Open questions

DaZombieKiller Jan 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antonfirsov Jan 4, 2021 • edited Loading

Choose a reason for hiding this comment

antonfirsov left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antonfirsov Jan 4, 2021 • edited Loading

Choose a reason for hiding this comment

antonfirsov Jan 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DaZombieKiller Jan 5, 2021 • 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

antonfirsov Jan 4, 2021 • edited Loading

Choose a reason for hiding this comment

DaZombieKiller commented Jan 4, 2021

Sergio0694 commented Jan 3, 2021 •

edited

Loading

DaZombieKiller Jan 4, 2021 •

edited

Loading

antonfirsov Jan 4, 2021 •

edited

Loading

antonfirsov left a comment •

edited

Loading

antonfirsov Jan 4, 2021 •

edited

Loading

antonfirsov Jan 4, 2021 •

edited

Loading

DaZombieKiller Jan 5, 2021 •

edited

Loading

antonfirsov Jan 4, 2021 •

edited

Loading