Ext.DrawImage generalization to support multiple formats. (Experimental) #686

vpenades · 2018-08-27T21:16:58Z

With this pull request I wanted to achieve:

Allow to draw images with a format different than the destination image (API breaking change)
now you can do: image<Rgba>.Mutate(x => x.DrawImage( Image<argb> ) );

About the implementation: Given that image drawing works with rows, and before drawing, both source and destination rows are converted to Vector4, the whole operation is done at no cost. This allows drawing images with different formats, without requiring a conversion.

Faster rendering: for image drawing, the "amount" opacity was expanded to a row, which consumed memory, etc. I've made an alternative path that passes the amount directly (the old one is also there)
Refactored PixelBlender class to move the bulk of the previously generated code to the main class. With this change a lot less code is generated.

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)

…r two loop variations. Generalised DrawingImage methods to support drawing images with a format different than the destination image at no cost.

…l blenders. Pixel blenders are performance critical, so every tiny performance improvement counts.

codecov · 2018-08-29T11:13:09Z

Codecov Report

Merging #686 into master will decrease coverage by 0.79%.
The diff coverage is 94.44%.

@@            Coverage Diff            @@
##           master     #686     +/-   ##
=========================================
- Coverage   89.36%   88.56%   -0.8%     
=========================================
  Files         878      879      +1     
  Lines       39249    38772    -477     
  Branches     2557     2665    +108     
=========================================
- Hits        35074    34340    -734     
- Misses       3485     3742    +257     
  Partials      690      690

Impacted Files	Coverage Δ
.../PixelFormats/PixelBlenders/PorterDuffFunctions.cs	`100% <ø> (+12.5%)`	⬆️
...ts/PixelBlenders/DefaultPixelBlenders.Generated.cs	`77.77% <ø> (-16.5%)`	⬇️
...ats/PixelBlenders/PorterDuffFunctions.Generated.cs	`38.02% <ø> (-14.83%)`	⬇️
...rc/ImageSharp/PixelFormats/PixelBlender{TPixel}.cs	`100% <100%> (ø)`
...Tests/PixelFormats/PixelOperationsTests.Blender.cs	`100% <100%> (ø)`	⬆️
...rocessing/Processors/Drawing/DrawImageProcessor.cs	`100% <100%> (ø)`	⬆️
...ageSharp.Drawing/Processing/DrawImageExtensions.cs	`37.5% <37.5%> (ø)`	⬆️

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 0e6fef0...c596fb1. Read the comment docs.

tocsoft · 2018-08-29T12:42:44Z

src/ImageSharp/PixelFormats/PixelBlenders/DefaultPixelBlenders.Generated.tt

                }

                /// <inheritdoc />
                protected override void BlendFunction(Span<Vector4> destination, ReadOnlySpan<Vector4> background, ReadOnlySpan<Vector4> source, float amount)                
                {
                    for (int i = 0; i < destination.Length; i++)
                    {
-                        destination[i] = PorterDuffFunctions.<#=blender_composer#>(background[i], source[i], amount);
+                        destination[i] = PorterDuffFunctions.<#=blender_composer#>(background[i], source[i], amount.Clamp(0,1));


we should clamp outside the loop

vpenades · 2018-08-29T15:48:01Z

Coverage test fails again? WTF, it was fine yesterday

JimBobSquarePants · 2018-08-29T22:36:32Z

@vpenades Coverage changes aren't failing, there's differences against the reference files.

…ition functions

JimBobSquarePants · 2018-09-06T06:52:54Z

@vpenades Can you have a look at the generated output from these changes compared the master?

We've got differences appearing on NET 4.7.1 and NET 4.6.2 in 64 bit mode.
https://ci.appveyor.com/project/six-labors/imagesharp/build/1.0.0-PullRequest00686001835

It's likely minor but best to cast your eye over to ensure nothing has gone wrong. If you're happy with the output please open a new PR against our reference images repository.

vpenades · 2018-09-06T17:48:16Z

On it...

For some reason, FillRegionProcessor and DrawTextProcessor where overshooting opacity over 1

Added missing AgressiveInlinings.

vpenades · 2018-09-10T09:31:43Z

Finally!

JimBobSquarePants

Those vector conversion changes will need to be updated plus I'd like to remove any per-pixel clamping if possible.

JimBobSquarePants · 2018-09-10T10:25:30Z

src/ImageSharp.Drawing/Processing/Processors/Drawing/FillRegionProcessor.cs

+                                        scanlineDirty = true;
+                                    }
+
+                                    if (scanline[startX] > 1)


What do these new constraining checks do?

When I had the DebugGuard set, FillRegionProcessor was producing some opacities of value 1.02xxx... the algorythms there probably need to be reviewed to ensure that opacity values are always in range. But since I don't fully understand what these functions do, I just added a clamp there.

Now that the clamps are back into the pixels, these checks could be removed...

JimBobSquarePants · 2018-09-10T10:25:51Z

src/ImageSharp.Drawing/Processing/SolidBrush{TPixel}.cs

+#if DEBUG
+                for (int i = 0; i < scanline.Length; i++)
+                {
+                    Guard.MustBeBetweenOrEqualTo(scanline[i], 0, 1, nameof(scanline));


Do we want to throw?

Ah, that one is no longer neccessary if the pixels do the clamp, I'll remove it now

JimBobSquarePants · 2018-09-10T10:29:36Z

src/ImageSharp/PixelFormats/PixelBlenders/PorterDuffFunctions.Generated.tt

@@ -27,44 +27,39 @@ namespace SixLabors.ImageSharp.PixelFormats.PixelBlenders
        [MethodImpl(MethodImplOptions.AggressiveInlining)]
        public static Vector4 <#=blender#>Src(Vector4 backdrop, Vector4 source, float opacity)
        {
-            opacity = opacity.Clamp(0, 1);
-            source.W *= opacity;          
+            source.W *= opacity.Clamp(0,1);


Are these called by per-row operations or are they purely for per-pixel operations?

If they are called from bulk we shouldn't be clamping per pixel.

These are being clamped per pixel. that's why I wanted to remove them and replace them with DebugGuards.

But by using DebugGuards, several tests fail due to out of range values, which prevents the PR to pass the coverage tests, etc.

So, for now, I preffer to go back to Clamp, and solve any potential out of range issues in future PRs

I think it'll be best to fix clamping here while we are working on it.

In the internal method PorterDuffFunctions.NormalSrc(Vector4 backdrop, Vector4 source, float opacity) the opacity is clamped before being used as a multiplier.

As far as I can see that method is only called by.

PorterDuffFunctions.NormalSrc<TPixel>(TPixel backdrop, TPixel source, float opacity)

NormalSrc.Blend(TPixel background, TPixel source, float amount)

NormalSrc.BlendFunction(Span<Vector4> destination, ReadOnlySpan<Vector4> background, ReadOnlySpan<Vector4> source, float amount)

NormalSrc.BlendFunction(Span<Vector4> destination, ReadOnlySpan<Vector4> background, ReadOnlySpan<Vector4> source, ReadOnlySpan<float> amount)

The clamping should take place in the caller methods not the callee, same for the other operations. Only the last method taking the ReadOnlySpan<float> amount requires per-pixel clamping of the values but that seems like something we could do with Vector<float> in the caller in supported architectures.

Thankfully all the changes required can be done in the T4 templates.

I'm happy to make the changes if you like?

Sure, go for it!, it's just lunch time here! 😄

JimBobSquarePants · 2018-09-10T10:31:26Z

src/ImageSharp.Drawing/Processing/Processors/Drawing/DrawImageProcessor.cs

+                    {
+                        Span<TPixelDst> background = source.GetPixelRowSpan(y).Slice(minX, width);
+                        Span<TPixelSrc> foreground = targetImage.GetPixelRowSpan(y - locationY).Slice(targetX, width);
+                        blender.Blend<TPixelSrc>(memoryAllocator, background, background, foreground, this.Opacity);


I couldn't find the callee Blend method in the diff but it's imperative now that if we are mixing pixel formats and conversion to/from Vector4 needs to used the scaled conversion methods.

JimBobSquarePants · 2018-09-10T10:32:53Z

src/ImageSharp/PixelFormats/PixelBlenders/DefaultPixelBlenders.Generated.tt

@@ -78,32 +80,27 @@ namespace SixLabors.ImageSharp.PixelFormats.PixelBlenders
                /// <inheritdoc />
                public override TPixel Blend(TPixel background, TPixel source, float amount)
                {
-                    return PorterDuffFunctions.<#=blender_composer#>(background, source, amount);
+                    TPixel dest = default;
+                    dest.PackFromVector4(PorterDuffFunctions.<#=blender_composer#>(background.ToVector4(),source.ToVector4(),amount.Clamp(0,1)));


I think this is what I was referring to before. Should be ToScaledVector4() with the reverse PackFromScaledVector4() on repacking.

Ahhh, okey, change it now

replaced *Vector with *ScaledVector conversion methods

JimBobSquarePants · 2018-09-10T14:58:25Z

src/ImageSharp/PixelFormats/PixelBlenders/PorterDuffFunctions.Generated.cs

@@ -16,93 +17,93 @@ internal static partial class PorterDuffFunctions



-        [MethodImpl(MethodImplOptions.AggressiveInlining)]
+        [MethodImpl(MethodImplOptions.NoInlining)]


@vpenades @antonfirsov Confirmed that changing this fixes tests in 64bit on Net Framework 4.7.1 and 4.6.2

We'll need to figure out how to create a reduced sample to report.

So that's the thing that's been driving me crazy...

JimBobSquarePants · 2018-09-10T21:58:05Z

I'm happy to merge this now unless anyone objects.

Ext.DrawImage generalization to support multiple formats. (Experimental)

vpenades added 2 commits August 27, 2018 23:05

Refactored PixelBlenders so it generates much less code and allows fo…

112262a

…r two loop variations. Generalised DrawingImage methods to support drawing images with a format different than the destination image at no cost.

removed a redundant opacity.Clamp(0,1) in the innermost level of pixe…

5306eaf

…l blenders. Pixel blenders are performance critical, so every tiny performance improvement counts.

tocsoft reviewed Aug 29, 2018

View reviewed changes

moved Clamp(0,1) one level up

a150e58

JimBobSquarePants added enhancement area:drawing labels Sep 1, 2018

vpenades added 2 commits September 2, 2018 22:31

Merge branch 'master' into master

c14d391

Adding more tests to cover single pixel ColorBlending and AlphaCompos…

2042f1f

…ition functions

vpenades added 6 commits September 6, 2018 22:01

trying to improve Opacity out of range propagation.

029b35c

For some reason, FillRegionProcessor and DrawTextProcessor where overshooting opacity over 1

expanding blocks...

80a27b6

Merge branch 'master' into master

e89c9f3

Merge branch 'master' into master

836ef41

Merge branch 'master' into master

8a4a647

Reverted from DebugGuard back to Clamp.

e59e259

Added missing AgressiveInlinings.

JimBobSquarePants requested changes Sep 10, 2018

View reviewed changes

vpenades and others added 3 commits September 10, 2018 13:05

reverted some changes from using DebugGuard

002fc64

replaced *Vector with *ScaledVector conversion methods

Remove duplicate clamping and normalize vector scaling.

0f4fcb5

Use NoInlining to workaround Vector Jit bug.

c596fb1

JimBobSquarePants reviewed Sep 10, 2018

View reviewed changes

JimBobSquarePants approved these changes Sep 10, 2018

View reviewed changes

JimBobSquarePants merged commit bf1227c into SixLabors:master Sep 11, 2018

antonfirsov pushed a commit to antonfirsov/ImageSharp that referenced this pull request Nov 11, 2019

Merge pull request SixLabors#686 from vpenades/master

c11b44f

Ext.DrawImage generalization to support multiple formats. (Experimental)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ext.DrawImage generalization to support multiple formats. (Experimental) #686

Ext.DrawImage generalization to support multiple formats. (Experimental) #686

vpenades commented Aug 27, 2018 •

edited

Loading

codecov bot commented Aug 29, 2018 •

edited

Loading

tocsoft Aug 29, 2018

vpenades Aug 29, 2018

vpenades commented Aug 29, 2018

JimBobSquarePants commented Aug 29, 2018

JimBobSquarePants commented Sep 6, 2018

vpenades commented Sep 6, 2018

vpenades commented Sep 10, 2018

JimBobSquarePants left a comment

JimBobSquarePants Sep 10, 2018

vpenades Sep 10, 2018

JimBobSquarePants Sep 10, 2018

vpenades Sep 10, 2018

JimBobSquarePants Sep 10, 2018

JimBobSquarePants Sep 10, 2018

vpenades Sep 10, 2018

JimBobSquarePants Sep 10, 2018

JimBobSquarePants Sep 10, 2018

JimBobSquarePants Sep 10, 2018

vpenades Sep 10, 2018

JimBobSquarePants Sep 10, 2018

JimBobSquarePants Sep 10, 2018

vpenades Sep 10, 2018

JimBobSquarePants Sep 10, 2018

vpenades Sep 10, 2018

JimBobSquarePants commented Sep 10, 2018

		@@ -16,93 +17,93 @@ internal static partial class PorterDuffFunctions



		[MethodImpl(MethodImplOptions.AggressiveInlining)]
		[MethodImpl(MethodImplOptions.NoInlining)]

Ext.DrawImage generalization to support multiple formats. (Experimental) #686

Ext.DrawImage generalization to support multiple formats. (Experimental) #686

Conversation

vpenades commented Aug 27, 2018 • edited Loading

Prerequisites

codecov bot commented Aug 29, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vpenades commented Aug 29, 2018

JimBobSquarePants commented Aug 29, 2018

JimBobSquarePants commented Sep 6, 2018

vpenades commented Sep 6, 2018

vpenades commented Sep 10, 2018

JimBobSquarePants left a comment

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

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

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

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

Choose a reason for hiding this comment

JimBobSquarePants commented Sep 10, 2018

vpenades commented Aug 27, 2018 •

edited

Loading

codecov bot commented Aug 29, 2018 •

edited

Loading