Decode Jpeg into Rgb24 when pixel type not specified by antonfirsov · Pull Request #1773 · SixLabors/ImageSharp

antonfirsov · 2021-10-03T02:04:58Z

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

Finish the plan defined in #1410:

Remove packing from JpegColorConverters and utilize Implement PackFromRgbPlanes for Rgba32 and Rgb24 #1462 PixelOperations.PackFromRgbPlanes instead
Return Image<Rgb24> from non-generic Decode
ResizeProcessor<T>: omit alpha premultiplication if the pixel type has no alpha channel

Fixes #1410, fixes #1121.

Performance

Did usual run of LoadResizeSaveStressBenchmarks on 10 core desktop. The overall effect on speed is within noise range, but there might be some 1%-ish improvement. I assume the reason is that #1462's packing actually doesn't make big difference to the packing code introduced in #1411.

But memory usage dropped significantly, which is the main goal here.

Before

|        Method | maxDegreeOfParallelism |       Mean |       Error |   StdDev | Ratio | RatioSD |     Gen 0 |     Gen 1 |     Gen 2 |    Allocated |
|-------------- |----------------------- |-----------:|------------:|---------:|------:|--------:|----------:|----------:|----------:|-------------:|
| SystemDrawing |                      1 | 8,711.9 ms |   862.64 ms | 47.28 ms |  1.00 |    0.00 |         - |         - |         - |        25 KB |
|    ImageSharp |                      1 | 5,218.4 ms |   135.62 ms |  7.43 ms |  0.60 |    0.00 | 3000.0000 | 3000.0000 | 3000.0000 | 1,226,257 KB |
|               |                        |            |             |          |       |         |           |           |           |              |
| SystemDrawing |                      5 | 3,128.0 ms | 1,188.61 ms | 65.15 ms |  1.00 |    0.00 |         - |         - |         - |        26 KB |
|    ImageSharp |                      5 | 1,516.9 ms |   782.02 ms | 42.86 ms |  0.49 |    0.02 | 2000.0000 | 2000.0000 | 2000.0000 | 1,226,402 KB |
|               |                        |            |             |          |       |         |           |           |           |              |
| SystemDrawing |                     10 | 2,828.7 ms |   786.61 ms | 43.12 ms |  1.00 |    0.00 |         - |         - |         - |        29 KB |
|    ImageSharp |                     10 |   823.4 ms |    80.49 ms |  4.41 ms |  0.29 |    0.01 | 1000.0000 | 1000.0000 | 1000.0000 | 1,229,766 KB |
|               |                        |            |             |          |       |         |           |           |           |              |
| SystemDrawing |                     20 | 2,793.0 ms |   735.65 ms | 40.32 ms |  1.00 |    0.00 |         - |         - |         - |        30 KB |
|    ImageSharp |                     20 |   686.0 ms |   570.78 ms | 31.29 ms |  0.25 |    0.01 | 1000.0000 | 1000.0000 | 1000.0000 | 1,237,201 KB |

After

|        Method | maxDegreeOfParallelism |       Mean |      Error |   StdDev | Ratio | RatioSD |     Gen 0 |     Gen 1 |     Gen 2 |  Allocated |
|-------------- |----------------------- |-----------:|-----------:|---------:|------:|--------:|----------:|----------:|----------:|-----------:|
| SystemDrawing |                      1 | 8,757.5 ms | 1,109.5 ms | 60.81 ms |  1.00 |    0.00 |         - |         - |         - |      25 KB |
|    ImageSharp |                      1 | 4,868.9 ms |   258.8 ms | 14.18 ms |  0.56 |    0.00 | 2000.0000 | 2000.0000 | 2000.0000 | 776,094 KB |
|               |                        |            |            |          |       |         |           |           |           |            |
| SystemDrawing |                      5 | 3,222.5 ms |   913.8 ms | 50.09 ms |  1.00 |    0.00 |         - |         - |         - |      27 KB |
|    ImageSharp |                      5 | 1,397.8 ms |   624.2 ms | 34.21 ms |  0.43 |    0.02 |         - |         - |         - | 776,093 KB |
|               |                        |            |            |          |       |         |           |           |           |            |
| SystemDrawing |                     10 | 2,849.4 ms | 1,590.1 ms | 87.16 ms |  1.00 |    0.00 |         - |         - |         - |      29 KB |
|    ImageSharp |                     10 |   852.6 ms |   576.0 ms | 31.57 ms |  0.30 |    0.02 | 1000.0000 | 1000.0000 | 1000.0000 | 843,968 KB |
|               |                        |            |            |          |       |         |           |           |           |            |
| SystemDrawing |                     20 | 2,764.5 ms | 1,152.1 ms | 63.15 ms |  1.00 |    0.00 |         - |         - |         - |      30 KB |
|    ImageSharp |                     20 |   622.7 ms |   475.5 ms | 26.07 ms |  0.23 |    0.01 | 1000.0000 | 1000.0000 | 1000.0000 | 949,029 KB |

/cc @br3aker @JimBobSquarePants @saucecontrol

…Planes

antonfirsov · 2021-10-03T02:24:26Z

src/ImageSharp/Formats/Jpeg/Components/Decoder/SpectralConverter{TPixel}.cs

        private bool Converted => this.pixelRowCounter >= this.pixelBuffer.Height;

-        public Buffer2D<TPixel> PixelBuffer
+        public Buffer2D<TPixel> GetPixelBuffer()


There is heavy lazy initialization, with side effects like CancellationToken firing. It's better to expose such stuff from a method than a property.

I didn't like this property approach with a lot of lazy stuff behind from the start but couldn't come up with anything better. There's a lot of confusing bits of code here and there from my pr, I will work on decoder performance/refactoring next as I'm done with the encoder for now.

codecov · 2021-10-03T02:42:56Z

Codecov Report

Merging #1773 (4323c8d) into master (2410a37) will increase coverage by 0.00%.
The diff coverage is 87.87%.

@@           Coverage Diff            @@
##           master    #1773    +/-   ##
========================================
  Coverage   84.49%   84.49%            
========================================
  Files         849      849            
  Lines       37412    37252   -160     
  Branches     4376     4385     +9     
========================================
- Hits        31611    31477   -134     
+ Misses       4947     4916    -31     
- Partials      854      859     +5

Flag	Coverage Δ
unittests	`84.49% <87.87%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...mageSharp/Common/Helpers/SimdUtils.HwIntrinsics.cs	`99.13% <ø> (ø)`
...rConverters/JpegColorConverter.FromYCbCrVector4.cs	`7.31% <0.00%> (+0.17%)`	⬆️
...rmats/Jpeg/Components/Decoder/HuffmanScanBuffer.cs	`94.73% <ø> (ø)`
...ImageSharp/PixelFormats/Utils/Vector4Converters.cs	`100.00% <ø> (ø)`
...ssors/Transforms/Resize/ResizeProcessor{TPixel}.cs	`98.38% <33.33%> (-1.62%)`	⬇️
...mory/DiscontiguousBuffers/MemoryGroupExtensions.cs	`89.81% <70.00%> (+0.70%)`	⬆️
...JpegColorConverter.VectorizedJpegColorConverter.cs	`66.66% <71.42%> (-8.34%)`	⬇️
...ents/Decoder/ColorConverters/JpegColorConverter.cs	`94.02% <71.42%> (+19.20%)`	⬆️
...onverters/JpegColorConverter.FromGrayScaleBasic.cs	`87.50% <84.61%> (-12.50%)`	⬇️
...ColorConverters/JpegColorConverter.FromCmykAvx2.cs	`100.00% <100.00%> (ø)`
... and 20 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2410a37...4323c8d. Read the comment docs.

JimBobSquarePants · 2021-10-03T15:38:53Z

A lot of reading here. Will try to review tomorrow night

gfoidl

Just a quick look over the code, not detailed review.

...arp/Formats/Jpeg/Components/Decoder/ColorConverters/JpegColorConverter.FromGrayScaleBasic.cs

gfoidl · 2021-10-03T19:56:46Z

...ImageSharp/Formats/Jpeg/Components/Decoder/ColorConverters/JpegColorConverter.FromRgbAvx2.cs

-                Vector256<int> vcontrol = Unsafe.As<byte, Vector256<int>>(ref control);
-
-                int n = result.Length / 8;
+                int n = values.Component0.Length / 8;


Suggested change

int n = values.Component0.Length / 8;

int n = (int)((uint)values.Component0.Length / 8);

So the JIT will emit a bit-hack division instead of the (signed) integer division by constant.

...ImageSharp/Formats/Jpeg/Components/Decoder/ColorConverters/JpegColorConverter.FromRgbAvx2.cs

...geSharp/Formats/Jpeg/Components/Decoder/ColorConverters/JpegColorConverter.FromRgbVector8.cs

gfoidl · 2021-10-03T20:00:32Z

...geSharp/Formats/Jpeg/Components/Decoder/ColorConverters/JpegColorConverter.FromRgbVector8.cs


                // Walking 8 elements at one step:
-                int n = result.Length / 8;
+                int n = values.Component0.Length / 8;


Suggested change

int n = values.Component0.Length / 8;

int n = (int)((uint)values.Component0.Length / 8);

I think this trick is only worth it when we are inside a tight loop or, or in a method that is expected to be called from a tight loop. In other cases I prefer readability.

Or is it possible to achieve the same in a simpler way nint? I changed my code everywhere to nint n = values.Component0.Length / 8; because n is only used for the i < n comparison, and I like it better when the types match explicitly.

We should go with readability, as you did now with nint. Codegen isn't ideal, but this pattern {array, span}.Length / ConstantOfPowerOfTwo is so common, that the JIT should be able to optimize this, so we don't need any hacks.
I'll have a look if there's a JIT-issue, otherwise create one.

Edit: filed dotnet/runtime#59922

gfoidl · 2021-10-03T20:02:55Z

...Sharp/Formats/Jpeg/Components/Decoder/ColorConverters/JpegColorConverter.FromYCbCrVector8.cs

-                ref Vector<float> ggRefAsVector = ref Unsafe.As<Vector4Pair, Vector<float>>(ref gg);
-                ref Vector<float> bbRefAsVector = ref Unsafe.As<Vector4Pair, Vector<float>>(ref bb);
-
+                int n = values.Component0.Length / 8;


Suggested change

int n = values.Component0.Length / 8;

int n = (int)((uint)values.Component0.Length / 8);

or maybe create a helper for this? The casts aren't very intuitive, but codegen is quite a difference.

I'm not sure it's worth it, see #1773 (comment)

...Sharp/Formats/Jpeg/Components/Decoder/ColorConverters/JpegColorConverter.FromYCbCrVector8.cs

gfoidl · 2021-10-03T20:03:52Z

...mageSharp/Formats/Jpeg/Components/Decoder/ColorConverters/JpegColorConverter.FromYccKAvx2.cs

+                int n = values.Component0.Length / 8;
                for (int i = 0; i < n; i++)


Suggested change

int n = values.Component0.Length / 8;

for (int i = 0; i < n; i++)

int n = (int)((uint)values.Component0.Length / 8);

for (nint i = 0; i < n; i++)

Now I'll stop commenting on these -- otherwise it's too much noise generated by me 😉

...eSharp/Formats/Jpeg/Components/Decoder/ColorConverters/JpegColorConverter.FromYccKVector8.cs

src/ImageSharp/Memory/DiscontiguousBuffers/MemoryGroupExtensions.cs

…mageSharp into af/jpeg-pack-sandbox

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

…mageSharp into af/jpeg-pack-sandbox

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

antonfirsov added 13 commits October 2, 2021 20:26

Inplace JpegColorConverter basic infra

033ed8e

convert YCbCr inplace

8599caa

Cmyk & Grayscale inplace conversion

ef767cc

inplace ycck conversion

0a6189d

Change SpectralConverter<T> to use inplace converters and PackFromRgb…

5f90e02

…Planes

simplify DecodeJpeg_ImageSpecific benchmark

c9d6556

Decode into Rgb24 by default

d798450

Resize: do not premultiply pixels with no alpha

06566c5

fix whitespace issues

5ba6461

remove old JpegColorConverter methods

adcc6d4

delete more unused methods

179e43a

fix ImageSharp.Benchmarks build

d2989d8

fix ProfilingSandbox

387bdfa

antonfirsov added breaking Signifies a binary breaking change. formats:jpeg labels Oct 3, 2021

antonfirsov commented Oct 3, 2021

View reviewed changes

remove unused test methods

a4cf70f

gfoidl reviewed Oct 3, 2021

View reviewed changes

antonfirsov and others added 10 commits October 3, 2021 22:35

Merge branch 'master' into af/jpeg-pack-sandbox

74a0dea

Delete Vector4Octet

cf4c3fb

use AggressiveInlining in some methods regardless of PROFILING

2537547

LoadResizeSaveStressRunner.ThumbnailSize as a parameter

6e136ec

Merge branch 'af/jpeg-pack-sandbox' of https://github.com/SixLabors/I…

e66f7b2

…mageSharp into af/jpeg-pack-sandbox

!values.IsEmpty

a421d6e

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

use nint with Unsafe.Add

bcfd3f3

Merge branch 'af/jpeg-pack-sandbox' of https://github.com/SixLabors/I…

fb36608

…mageSharp into af/jpeg-pack-sandbox

"less ceremony"

4803b45

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

((uint)bufferIdx >= group.Count) trick

ed57622

This was referenced Sep 23, 2025

Bump the nuget group with 5 updates norschel/enterJSWebSecurity2025-Demo1#8

Open

Bump SixLabors.ImageSharp from 1.0.3 to 2.1.11 sajadbz/Sbz-clean-architecture#10

Open

dependabot bot mentioned this pull request Oct 19, 2025

Bump the nuget group with 1 update ewdlop/Raytracing#1

Open

dependabot bot mentioned this pull request Feb 3, 2026

Bump the nuget group with 5 updates kennethkcox/dep-guard#5

Open

	int n = values.Component0.Length / 8;
	int n = (int)((uint)values.Component0.Length / 8);

		int n = values.Component0.Length / 8;
		for (int i = 0; i < n; i++)

Uh oh!

Conversation

antonfirsov commented Oct 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Prerequisites

Description

Performance

Before

After

Uh oh!

antonfirsov Oct 3, 2021

Choose a reason for hiding this comment

Uh oh!

br3aker Oct 3, 2021

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Oct 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

JimBobSquarePants commented Oct 3, 2021

Uh oh!

gfoidl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gfoidl Oct 3, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

gfoidl Oct 3, 2021

Choose a reason for hiding this comment

Uh oh!

antonfirsov Oct 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

antonfirsov Oct 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gfoidl Oct 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gfoidl Oct 3, 2021

Choose a reason for hiding this comment

Uh oh!

antonfirsov Oct 3, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gfoidl Oct 3, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

antonfirsov commented Oct 3, 2021 •

edited

Loading

codecov bot commented Oct 3, 2021 •

edited

Loading

antonfirsov Oct 3, 2021 •

edited

Loading

antonfirsov Oct 3, 2021 •

edited

Loading

gfoidl Oct 4, 2021 •

edited

Loading