Make processors public, refactor cloning. #1011

JimBobSquarePants · 2019-09-17T14:46:30Z

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

Touches. #967

Refactors the image processors to make the interfaces and base classes public.
Removes double pixel processor creation.
Simplified cloned target image creation.

I've marked this as draft as I want to do something more extreme and remove ICloningImageProcessor<TPixel> moving the interface method into the base class. This makes additional methods impossible to test against though via mocking.

Changed my mind and figured out a better way to create contracts for cloned/regular processors.

Thoughts please!

codecov · 2019-09-17T17:23:38Z

Codecov Report

Merging #1011 into master will increase coverage by <.01%.
The diff coverage is 91.22%.

@@            Coverage Diff             @@
##           master    #1011      +/-   ##
==========================================
+ Coverage   89.86%   89.86%   +<.01%     
==========================================
  Files        1098     1099       +1     
  Lines       48832    48836       +4     
  Branches     3435     3439       +4     
==========================================
+ Hits        43881    43888       +7     
+ Misses       4249     4246       -3     
  Partials      702      702

Impacted Files	Coverage Δ
.../Processing/Processors/Transforms/SkewProcessor.cs	`84.61% <ø> (ø)`	⬆️
...ng/Processors/Transforms/Resize/ResizeProcessor.cs	`100% <ø> (ø)`	⬆️
...rp/Processing/Processors/ImageProcessor{TPixel}.cs	`100% <ø> (ø)`	⬆️
src/ImageSharp/Advanced/AotCompilerTools.cs	`0% <0%> (ø)`	⬆️
...ing/Processors/Transforms/CropProcessor{TPixel}.cs	`100% <100%> (ø)`	⬆️
...ors/Convolution/EdgeDetector2DProcessor{TPixel}.cs	`100% <100%> (ø)`	⬆️
...arp/Processing/Processors/CloningImageProcessor.cs	`100% <100%> (ø)`
...rocessing/Processors/Transforms/RotateProcessor.cs	`100% <100%> (ø)`	⬆️
...ocessors/Transforms/AutoOrientProcessor{TPixel}.cs	`85.29% <100%> (ø)`	⬆️
.../Processors/Transforms/AffineTransformProcessor.cs	`100% <100%> (ø)`	⬆️
... and 16 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 b004888...464598c. Read the comment docs.

antonfirsov · 2019-09-17T21:45:12Z

I had an idea a few weeks ago for the same point. Actually, I had two options in my mind, and wanted to ask for input to decide which one is better, but I haven't got the the chance to present them so far.

This solution is a third option actually 😄 I will have a deeper look at it now, but I can't promise I can summarize my thoughts properly tonight.

antonfirsov

Pointing out a few not-so-nice points. I remember I had ideas to solve them, when I was thinking about the problem (and faced other non-trivial questions afterwards), I'm just dead tired to recall those ideas now 😄
We'll be back tomorrow!

src/ImageSharp/Processing/DefaultImageProcessorContext{TPixel}.cs

src/ImageSharp/Processing/Processors/CloningImageProcessor.cs

src/ImageSharp/Processing/Processors/CloningImageProcessor{TPixel}.cs

JimBobSquarePants · 2019-09-17T22:59:04Z

This solution is a third option actually 😄 I will have a deeper look at it now, but I can't promise I can summarize my thoughts properly tonight.

Excellent. Looking forward to seeing what you come up with. I'm going to replace the Mocks with dummy pass-through instances so I can demonstrate my intent in the interim and continue to provide coverage.

JimBobSquarePants · 2019-09-19T14:33:45Z

src/ImageSharp/Advanced/AotCompilerTools.cs

        {
-            var test = new OctreeFrameQuantizer<TPixel>(new OctreeQuantizer(false));
-            test.AotGetPalette();
+            using (var test = new OctreeFrameQuantizer<TPixel>(new OctreeQuantizer(false)))


Spotted these when removing the AOT CreateDestination method. No longer required as not virtual.

JimBobSquarePants · 2019-09-19T14:35:08Z

src/ImageSharp/Processing/Processors/CloningImageProcessor{TPixel}.cs


        /// <inheritdoc/>
-        public Image<TPixel> CloneAndApply()
+        Image<TPixel> ICloningImageProcessor<TPixel>.CloneAndExecute()


Hidden so not to clutter child classes.

JimBobSquarePants · 2019-09-19T14:35:44Z

src/ImageSharp/Processing/Processors/CloningImageProcessor{TPixel}.cs

        {
        }
+
+        private Image<TPixel> CreateTarget()


This method was identical in every implementation. The only different was where the target size came from.

JimBobSquarePants · 2019-09-19T14:37:52Z

src/ImageSharp/Processing/Processors/Transforms/AffineTransformProcessor{TPixel}.cs

                ParallelHelper.IterateRows(
                    targetBounds,
-                    this.Configuration,
+                    configuration,


Spotted we were capturing the whole class in the lambda

I don't get the difference. this.Configuration also returns a Configuration instance.

Also: capturing the whole class is not necessarily evil. Or is it? Am I missing something?

Bad practice as I understand that can lead to negative performance and issues since capturing variables affects the lifetime of those captures.

antonfirsov

@JimBobSquarePants the current state might be good enough actually, but if you don't mind, let's keep it open for another few days.
Hope I get the chance to check it once again with a fresh mind. (Sunday probably.)
Maybe we can clean up / simplify even more.

antonfirsov · 2019-09-19T22:14:31Z

src/ImageSharp/Processing/Processors/CloningImageProcessor{TPixel}.cs

-                    throw new ImageProcessingException($"An error occurred when processing the image using {this.GetType().Name}. The processor changed the number of frames.");
-                }
+                Image<TPixel> clone = this.CreateTarget();
+                this.CheckFrameCount(this.Source, clone);


Not sure if the checks are necessary. The centralized cloning logic should guarantee that the frame counts are matching.

I actually thought that but was too lazy to check and just merged the code into a single method

It's an iterative way to delete bloated code 😉

JimBobSquarePants · 2019-09-24T07:00:42Z

Setting a countdown on this one. I want to keep progressing.

antonfirsov

Agree, it's more important now to progress, than to polish everything. I'm struggling to find time this week, improvements could be added later in a separate PR.

Make processors public, refactor cloning.

Make processors public, refactor cloning.

1796862

JimBobSquarePants added the API label Sep 17, 2019

JimBobSquarePants added this to the 1.0.0-rc1 milestone Sep 17, 2019

JimBobSquarePants requested review from antonfirsov, dlemstra and tocsoft September 17, 2019 14:46

antonfirsov mentioned this pull request Sep 17, 2019

Expose visitor through advanced namespace. #1010

Merged

4 tasks

antonfirsov reviewed Sep 17, 2019

View reviewed changes

JimBobSquarePants added 3 commits September 18, 2019 09:34

Merge branch 'master' into feature/processors

75d27b5

Merge branch 'master' into feature/processors

ab856a3

Refactor cloning processors and tests

464598c

JimBobSquarePants marked this pull request as ready for review September 19, 2019 14:32

JimBobSquarePants changed the title ~~WIP: Make processors public, refactor cloning.~~ Make processors public, refactor cloning. Sep 19, 2019

JimBobSquarePants commented Sep 19, 2019

View reviewed changes

antonfirsov reviewed Sep 19, 2019

View reviewed changes

antonfirsov approved these changes Sep 24, 2019

View reviewed changes

JimBobSquarePants merged commit 4a4c8dd into master Sep 25, 2019

JimBobSquarePants deleted the feature/processors branch September 25, 2019 06:08

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

Merge pull request SixLabors#1011 from SixLabors/feature/processors

ca74af7

Make processors public, refactor cloning.

dependabot bot mentioned this pull request Sep 23, 2025

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

Open

dependabot bot mentioned this pull request Oct 19, 2025

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

Open

Uh oh!

Make processors public, refactor cloning. #1011

Make processors public, refactor cloning. #1011

Uh oh!

Conversation

JimBobSquarePants commented Sep 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Prerequisites

Description

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

antonfirsov commented Sep 17, 2019

Uh oh!

antonfirsov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JimBobSquarePants commented Sep 17, 2019

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 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!

JimBobSquarePants commented Sep 24, 2019

Uh oh!

antonfirsov left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

JimBobSquarePants commented Sep 17, 2019 •

edited

Loading

codecov bot commented Sep 17, 2019 •

edited

Loading