Introduce extended pixel conversion #869

antonfirsov · 2019-04-01T00:26:51Z

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)

Description

This is a replacement PR for #847 introducing extended bulk conversion controlled by flags.

Tests, benchmarks, and SRrgbCompanding and Resize refactors have been reused from Add premultiplied bulk pixel operations #847
Converting from Vector4 buffers is destructive for performance, yet it's clean
Very basic actual optimization have been introduced for Rgb24 and Bgr24.

ResizeBicubic_Rgb24

BEFORE:

                                   Method |  Job | Runtime | SourceSize | DestSize |     Mean |      Error |    StdDev | Ratio | RatioSD | Gen 0/1k Op | Gen 1/1k Op | Gen 2/1k Op | Allocated Memory/Op |
----------------------------------------- |----- |-------- |----------- |--------- |---------:|-----------:|----------:|------:|--------:|------------:|------------:|------------:|--------------------:|
                            SystemDrawing |  Clr |     Clr |       3032 |      400 | 124.9 ms | 101.109 ms | 5.5422 ms |  1.00 |    0.00 |           - |           - |           - |              2048 B |
 'ImageSharp, MaxDegreeOfParallelism = 1' |  Clr |     Clr |       3032 |      400 | 111.9 ms |   2.431 ms | 0.1332 ms |  0.90 |    0.04 |           - |           - |           - |             45875 B |
                                          |      |         |            |          |          |            |           |       |         |             |             |             |                     |
                            SystemDrawing | Core |    Core |       3032 |      400 | 123.1 ms |  41.406 ms | 2.2696 ms |  1.00 |    0.00 |           - |           - |           - |                96 B |
 'ImageSharp, MaxDegreeOfParallelism = 1' | Core |    Core |       3032 |      400 | 101.4 ms |  19.823 ms | 1.0866 ms |  0.82 |    0.02 |           - |           - |           - |             44504 B |

AFTER:

                                   Method |  Job | Runtime | SourceSize | DestSize |      Mean |     Error |    StdDev | Ratio | RatioSD | Gen 0/1k Op | Gen 1/1k Op | Gen 2/1k Op | Allocated Memory/Op |
----------------------------------------- |----- |-------- |----------- |--------- |----------:|----------:|----------:|------:|--------:|------------:|------------:|------------:|--------------------:|
                            SystemDrawing |  Clr |     Clr |       3032 |      400 | 121.37 ms | 48.580 ms | 2.6628 ms |  1.00 |    0.00 |           - |           - |           - |              2048 B |
 'ImageSharp, MaxDegreeOfParallelism = 1' |  Clr |     Clr |       3032 |      400 |  99.36 ms | 11.356 ms | 0.6224 ms |  0.82 |    0.02 |           - |           - |           - |             45056 B |
                                          |      |         |            |          |           |           |           |       |         |             |             |             |                     |
                            SystemDrawing | Core |    Core |       3032 |      400 | 118.06 ms | 15.667 ms | 0.8587 ms |  1.00 |    0.00 |           - |           - |           - |                96 B |
 'ImageSharp, MaxDegreeOfParallelism = 1' | Core |    Core |       3032 |      400 |  92.47 ms |  5.683 ms | 0.3115 ms |  0.78 |    0.01 |           - |           - |           - |             44512 B |

…Vector4)

antonfirsov · 2019-04-01T00:28:16Z

src/ImageSharp/PixelFormats/PixelConversionModifiers.cs

+    /// knowing the pixel type.
+    /// </summary>
+    [Flags]
+    internal enum PixelConversionModifiers


Please review my namings here!

Naming is good! 👍

codecov · 2019-04-01T02:28:25Z

Codecov Report

Merging #869 into master will decrease coverage by 0.01%.
The diff coverage is 99.33%.

@@            Coverage Diff             @@
##           master     #869      +/-   ##
==========================================
- Coverage   88.89%   88.87%   -0.02%     
==========================================
  Files        1014     1017       +3     
  Lines       44295    44438     +143     
  Branches     3209     3228      +19     
==========================================
+ Hits        39376    39495     +119     
+ Misses       4198     4181      -17     
- Partials      721      762      +41

Impacted Files	Coverage Δ
...PixelFormats/PixelConversionModifiersExtensions.cs	`100% <100%> (ø)`
...ing/Processors/Convolution/ConvolutionProcessor.cs	`100% <100%> (ø)`	⬆️
...lFormats/Utils/Vector4Converters.RgbaCompatible.cs	`91.42% <100%> (-1.6%)`	⬇️
...rc/ImageSharp/PixelFormats/PixelBlender{TPixel}.cs	`100% <100%> (ø)`	⬆️
...xelFormats/PixelOperations/PixelOperationsTests.cs	`100% <100%> (ø)`	⬆️
...tions/PixelOperationsTests.Bgr24OperationsTests.cs	`100% <100%> (ø)`	⬆️
...ng/Processors/Transforms/Resize/ResizeProcessor.cs	`96.22% <100%> (-0.23%)`	⬇️
...geSharp.Tests/TestUtilities/TestImageExtensions.cs	`83.26% <100%> (ø)`	⬆️
...tions/Generated/Rgb24.PixelOperations.Generated.cs	`89.28% <100%> (+24.51%)`	⬆️
.../Jpeg/Components/Decoder/JpegImagePostProcessor.cs	`93.18% <100%> (ø)`	⬆️
... and 48 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 45b7490...408e196. Read the comment docs.

JimBobSquarePants

I much, much, much prefer this approach to the tree of methods in my PR. Much easier for us to optimize single methods.

JimBobSquarePants · 2019-04-01T06:22:33Z

src/ImageSharp/Processing/Processors/Transforms/Resize/ResizeProcessor.cs

+            PixelConversionModifiers conversionModifiers = PixelConversionModifiers.Premultiply;
+            if (this.Compand)
+            {
+                conversionModifiers |= PixelConversionModifiers.Scale | PixelConversionModifiers.SRgbCompand;


Maybe we could shortcut this? It's something to remember every time. (Unless that's a good thing for greater understanding?)

I think we'll leave it as-is. It's explicit and should be well understood.

Companding always indicates scaling (otherwise colors are incorrect), right?

Yeah, the algorithm requires scaling.

JimBobSquarePants · 2019-04-01T06:22:56Z

src/ImageSharp/PixelFormats/PixelConversionModifiers.cs

+    /// knowing the pixel type.
+    /// </summary>
+    [Flags]
+    internal enum PixelConversionModifiers


Naming is good! 👍

JimBobSquarePants · 2019-04-01T06:30:20Z

src/ImageSharp/PixelFormats/PixelConversionModifiersExtensions.cs

+    internal static class PixelConversionModifiersExtensions
+    {
+        [MethodImpl(MethodImplOptions.AggressiveInlining)]
+        public static bool IsDefined(this PixelConversionModifiers modifiers, PixelConversionModifiers expected) =>


antonfirsov · 2019-04-01T08:57:30Z

src/ImageSharp/PixelFormats/PixelConversionModifiers.cs

+        /// <summary>
+        /// Select <see cref="IPixel.ToScaledVector4"/> and <see cref="IPixel.FromScaledVector4"/> instead the standard (non scaled) variants.
+        /// </summary>
+        Scale = 1 << 0,


@JimBobSquarePants I keep thinking whether we should replace Scaled / Scale with Normalized / Normalize in our terminology. Seems more precize to me. Or is this wrong thinking?

Or is this wrong thinking?

Actually no. Normalization in our situation is exactly what we are doing and I think I prefer it!

It means breaking IPixel for consistency but you have my blessing to do so. I'd really like to ensure we have our naming correct for RC1.

antonfirsov · 2019-04-01T20:39:37Z

I'm merging this now to unblock stuff like #870. Naming tweaks could be added in a follow-up PR.

antonfirsov added 13 commits March 31, 2019 22:55

make .FromVector4() destructive

e217d13

replace FromScaledVector4() / ToScaledVector4() with flag usages

877f314

copy companding refactor from #847

b0ff7cf

add (failing) tests from #847

d126b85

all pixel types implemented except RgbaVector

1a922f6

fix RgbaVector

d1dd90c

Inlining temporary compatibility methods (FromScaledVector4, ToScaled…

70c4e11

…Vector4)

Optimization: omit premultiplication for Bgr24 and Rgb24

37563cc

extend Resize benchmarks

d2fddce

update ResizeProcessor

acb1dbc

update benchmark results

575a484

rename FromVector4(...) to FromVector4Destructive(...)

4a0b100

pull back the submodule reference with a few revisions

408e196

antonfirsov commented Apr 1, 2019

View reviewed changes

JimBobSquarePants approved these changes Apr 1, 2019

View reviewed changes

JimBobSquarePants added enhancement area:performance API labels Apr 1, 2019

JimBobSquarePants added this to the 1.0.0-rc1 milestone Apr 1, 2019

JimBobSquarePants mentioned this pull request Apr 1, 2019

Add premultiplied bulk pixel operations #847

Closed

4 tasks

antonfirsov commented Apr 1, 2019

View reviewed changes

antonfirsov merged commit c2d2ccb into master Apr 1, 2019

JimBobSquarePants deleted the af/pixel-conversion branch September 3, 2019 11:13

dependabot bot mentioned this pull request Oct 19, 2025

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

Open

Uh oh!

Uh oh!

Introduce extended pixel conversion #869

Introduce extended pixel conversion #869

Uh oh!

Conversation

antonfirsov commented Apr 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Prerequisites

Description

Uh oh!

antonfirsov Apr 1, 2019 • 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!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Apr 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

JimBobSquarePants left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

antonfirsov commented Apr 1, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

antonfirsov commented Apr 1, 2019 •

edited

Loading

antonfirsov Apr 1, 2019 •

edited

Loading

codecov bot commented Apr 1, 2019 •

edited

Loading