Refactored Mutate/Clone APIs #275

tocsoft · 2017-07-12T06:37:11Z

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 practise as demonstrated in the repository. These follow strict Stylecop rules 👮.
I have provided test coverage for my change (where applicable)

Description

This is a rather significatant change to our public APIs the proposal is thus.

Instead of Image<TPixel> having a range of extension methods on it to do all sorts of mutations that return the save instance I've introduced IImageOperations<TPixel> which is used to queue/indirecty apply IImageProcessors to the source image.

The way you get a handle on an IImageOperations<TPixel> is by using the only 2 remaining extension APIs from Image<TPixel> (should theses be moved directly onto Image<TPixel> instead of being extension methds?).

The driver for this is I wanted the mutateing APIs to be obvious that they are a fluent chaining/mutating set of operations that return the source image again instead of returning a image each time.

Additionally I wanted to enable a version of all the ImageOperations that did return a new instance for those times you wanted to generate multiple outputs from the same input.

So some code examples.

So to perform a simple set of operation you now do this:

image.Mutate(x=>x.Grayscale().Resize(10,10).Pixelatte()); // returns void
image.Save("mutatedImage.png");

this will mutate the pixel data stored in image first making it grayscale, then resizing it, and finaly pixelatting it.

now to do all that again by not alter the source image would be a simple as

using(var new_image = image.Clone(x=>x.Grayscale().Resize(10,10).Pixelatte())) // returns `Image`
{
    // deal with clone in here
    new_image.Save("newImage.png");
}

image.Save("origionalImage.png");

second example has left image in exactly the same unaltered state is was in before starting, and instead has created a new image new_image with all the mutations applied.

Fixes #265

…ants/ImageSharp into tocsoft/mutate-api # Conflicts: # src/ImageSharp/ApplyProcessors.cs

codecov-io · 2017-07-12T06:37:23Z

Codecov Report

Merging #275 into master will increase coverage by 0.21%.
The diff coverage is 96.62%.

@@            Coverage Diff             @@
##           master     #275      +/-   ##
==========================================
+ Coverage   86.71%   86.92%   +0.21%     
==========================================
  Files         671      716      +45     
  Lines       29912    30507     +595     
  Branches     2138     2151      +13     
==========================================
+ Hits        25937    26517     +580     
- Misses       3316     3325       +9     
- Partials      659      665       +6

Impacted Files	Coverage Δ
src/ImageSharp.Drawing/Paths/DrawPath.cs	`66.66% <ø> (ø)`	⬆️
src/ImageSharp.Drawing/Paths/DrawPathCollection.cs	`37.5% <ø> (ø)`	⬆️
...geSharp/PixelFormats/PackedPixelConverterHelper.cs	`0% <ø> (ø)`	⬆️
src/ImageSharp.Drawing/Paths/DrawBeziers.cs	`33.33% <ø> (ø)`	⬆️
src/ImageSharp.Drawing/Paths/FillRectangle.cs	`100% <ø> (ø)`	⬆️
src/ImageSharp.Drawing/Text/TextGraphicsOptions.cs	`81.25% <ø> (-1.11%)`	⬇️
src/ImageSharp.Drawing/Paths/DrawRectangle.cs	`66.66% <ø> (ø)`	⬆️
src/ImageSharp.Drawing/Paths/FillPaths.cs	`100% <ø> (ø)`	⬆️
src/ImageSharp.Drawing/Text/DrawText.Path.cs	`100% <ø> (ø)`	⬆️
src/ImageSharp.Drawing/Text/DrawText.cs	`100% <ø> (ø)`	⬆️
... and 238 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 a710ccc...9b34d09. Read the comment docs.

tocsoft · 2017-07-12T06:41:49Z

src/ImageSharp/ApplyProcessors.cs

+        /// <typeparam name="TPixel">The pixel format.</typeparam>
+        /// <param name="source">The image to rotate, flip, or both.</param>
+        /// <param name="operations">The operations to perform on the source.</param>
+        public static void Mutate<TPixel>(this Image<TPixel> source, Action<IImageOperations<TPixel>> operations)


Question : should theses be extension methods or exposed instance methods of Image<TPixel>?

The more I think about it I feel these should actually be moved onto Image directly thus exposing this APIs without needing to add the using statements.

We should be consistent:

Either by redesigning our whole API to be fully extension method focused, so not only Mutate(), but all the methods/properties producing derived results from a narrow core API, should be extension methods. So even image.Bounds would become an image.Bounds() extension method.

Or just turn both Mutate(operations) and Clone(operations) to instance methods as well

Bounds() should be an extension method I feel, as its only really used as a helper for image operations. (in fact it should probably just be internal)

We should also probably move out all all the overloads of Save() into extension methods too Image would just have Save(Stream, Encoder).

Also things like To<TPixel2>() could also probably be a candidate for changing.

And now the ApplyProcessor() is basically redundant.

antonfirsov

Made a quick review focusing on core stuff.

Haven't checked the concrete processor extension methods. It should be done probably by someone who has better understanding of the processors ;)

antonfirsov · 2017-07-14T00:33:01Z

src/ImageSharp/IImageOperationsProvider.cs

+            where TPixel : struct, IPixel<TPixel>;
+    }
+
+    /// <summary>


The naming seems inconsistent here.

It should be probably IImageOperationsFactory + CreateImageOperations(), or IImageOperationsProvider + GetImageOperations().

antonfirsov · 2017-07-14T00:35:28Z

src/ImageSharp/ImageOperations.cs

+        /// </summary>
+        /// <param name="processors">Processors to apply</param>
+        /// <returns>this </returns>
+        public IImageOperations<TPixel> ApplyProcessors(IEnumerable<IImageProcessor<TPixel>> processors)


This method is unused.

antonfirsov · 2017-07-14T00:56:52Z

src/ImageSharp/ApplyProcessors.cs

+            where TPixel : struct, IPixel<TPixel>
+        {
+            Guard.NotNull(operations, nameof(operations));
+            var generated = new Image<TPixel>(source);


For most processors we (eg. Resize()) we are allocating a new Width*Height image area twice!

First calling this copy constructor

Second time in the processor itself

So the issue Eric Mellino pointed out on gitter is still here, having two allocations is unnecessary. We should find a way to avoid this during this API validation process. I'm not saying that we should refactor everything now, but we need to make sure it is possible with the new API!

The internal parts of the should be refactorable to make it possible to prevent the double allocation.

I'm not sure if it's possible without changing IImageProcessor<TPixel> design.

The processors doing allocation of a new image buffer should be able to take the source and destination image buffers from the outside instead. It would make SwapPixelBuffers() unnecessary:
https://github.com/SixLabors/ImageSharp/blob/master/src/ImageSharp/Processing/Processors/Transforms/ResizeProcessor.cs#L97

@JimBobSquarePants thoughts?

I may play around with this, making a proposal in the weekend.

I was planning to create a new internal interface ICloneableImageProcessor<T> : IImageProcessor<TPixel> (or similare ) then in the DefaultImageOperator check if the passed in IImageProcessor is in fact an ICloneableImageProcessor and if it is then instead of calling IImageProcessor.Apply() then it instead call IImageProcessor.CloneAndApply() which returns the cloned image. All it would require is the DefaultImageOperator cloning the source image lazerly the first time we encounter an IImageProcesssor and its not an ICloneableImageProcessor and if it is on then it will retain the cloned image and just call Apply with it on all remaining IImageProcesssor

feel free to have a go... basically my ideal situation (for v1) is to do it without having to change the public API any, even if that means that only our internal processors have to extra power of generating the clone.

antonfirsov · 2017-07-15T16:35:31Z

@JimBobSquarePants @tocsoft
finding a clean & maintainable solution for the double cloning issue seems really hard, I'm struggling since hours with no success. One thing seems pretty sure: the IImageProcessor<T> API has to be changed to manage the copying family of processors in a clean way.

I don't think any of our users are implementing their own processors though, so probably a minor change on that API somewhere between the beta and the RC should not be an issue. Thoughts? :)

tocsoft · 2017-07-15T21:30:13Z

@antonfirsov c421e6d is basically what I was thinking with regards the fix for double cloning... none of those changes actually alter the public API... so it is possible without having to change IImageProcessor<> true it doesn't expose the ability to third party developers but that can be done an a version or 2 once other devs start wanting to make processors for it.

antonfirsov · 2017-07-16T00:51:41Z

src/ImageSharp/IImageProcessorApplicator{TPixel}.cs

+    /// An interface to queue up image operations.
+    /// </summary>
+    /// <typeparam name="TPixel">The pixel format</typeparam>
+    public interface IImageProcessorApplicator<TPixel>


Shouldn't we find a less verbose name for this? How about IImageProcessingContext<TPixel>? //image.Mutate(ctx => ctx.Resize(..) )

I like IImageProcessingContext

antonfirsov · 2017-07-16T00:52:39Z

src/ImageSharp/Image/ICloningImageProcessor.cs

+    /// Encapsulates methods to alter the pixels of an image.
+    /// </summary>
+    /// <typeparam name="TPixel">The pixel format.</typeparam>
+    public interface ICloningImageProcessor<TPixel> : IImageProcessor<TPixel>


Wasn't your intention to internalize this?

yep i was, must have missed it

antonfirsov · 2017-07-16T01:06:35Z

src/ImageSharp/Processing/Processors/CLoneingImageProcessor.cs

+    /// Allows the application of processors to images.
+    /// </summary>
+    /// <typeparam name="TPixel">The pixel format.</typeparam>
+    internal abstract class CloneingImageProcessor<TPixel> : IImageProcessor<TPixel>, ICloningImageProcessor<TPixel>


Typo: Cloneing...

Can't we avoid code duplication by subclassing ImageProcessor<TPixel>?

there is no duplication really, they both follow the same patterns but the code is different, one takes 2 images the other takes 1. Basically CloneingImageProcessor is for image processors that require a 2nd buffer to apply changes to before overwriting the pixel data in the original. ImageProcessor if for processors that don't require the 2nd buffer and can apply there changes directly to the source.

JimBobSquarePants · 2017-08-14T06:58:00Z

Run makes me think of Task which doesn't feel contextually correct. Apply makes me think of JavaScript's function.Apply which would be contextually correct.

antonfirsov · 2017-08-15T11:44:48Z

@JimBobSquarePants @tocsoft tried to clarify the avatar example by adding some docs, comments + a variant, that does the trick without Apply().

@JimBobSquarePants if you say that users who are not messed with 200 git changes+flu should understand all this, I believe you! 😄
Everything looks good to me now!

tocsoft · 2017-08-15T12:03:50Z

Those updates look good to me 👍

I like that we're showing both methods as its demonstrating that we aren't prescribing doing it one way or the other.

antonfirsov · 2017-08-15T12:16:35Z

Here is the only problem left:

On one hand, we should merge this ASAP to unblock upcoming PR-s.
On the other hand: this is a big change, and we need to explain it to our users first, together with the reasons driving to this change. I'm afraid, we should write a blog post before merging.

tocsoft · 2017-08-15T12:21:57Z

I could use this as the base branch for doing the package renames/namespace changes. That should cause less disruption on people who inadvertently update to latest as they would be having to to pull in a different package anyway.

Also means there is only one large braking change instead of 2.

tocsoft · 2017-08-15T12:22:59Z

obviously would still require a blog post but it would be a single one with API, package name in one.

antonfirsov · 2017-08-15T12:23:29Z

👍 Good thinking!

Update namespaces & package names

tocsoft added 12 commits July 1, 2017 20:09

migrate to a Image.Generate/Image.Mutate context api

f026778

fix demo

456e139

fix apply processors to frames

88e30a0

Rename Generate -> Clone

a61fcc9

Fix stack overflow

001e3ca

merge with master

b4fbf17

initail image operations tests

961e202

move methods

c3f7f83

Merge branch 'tocsoft/mutate-api' of https://github.com/JimBobSquareP…

ccee50b

…ants/ImageSharp into tocsoft/mutate-api # Conflicts: # src/ImageSharp/ApplyProcessors.cs

test extensions methods

f7cbb27

fix warnings

04b6ad8

fix sandbox test

c12f993

tocsoft commented Jul 12, 2017

View reviewed changes

tocsoft requested review from JimBobSquarePants, antonfirsov and dlemstra July 12, 2017 06:42

blank to force rebuild

6270568

antonfirsov requested changes Jul 14, 2017

View reviewed changes

tocsoft added 4 commits July 14, 2017 21:42

Renameing classes + simplifying Image<TPixel>

ab30013

fix some comments

9b86525

Merge remote-tracking branch 'origin/master' into tocsoft/mutate-api

cb165fa

fix broken tests

41cadf9

Cloneing Image Processor

c421e6d

antonfirsov reviewed Jul 16, 2017

View reviewed changes

internalise cloning processor + rename context

7635558

JimBobSquarePants approved these changes Aug 13, 2017

View reviewed changes

JimBobSquarePants added 3 commits August 14, 2017 11:13

Intellisense docs cleanup

9c5798b

Merge branch 'master' into tocsoft/mutate-api

15a7b5a

Update rotate test to use new API

f8a0ed3

added comments, docs + a CloneAndConvertToAvatarWithoutApply() example

de964e5

antonfirsov approved these changes Aug 15, 2017

View reviewed changes

antonfirsov and others added 9 commits August 15, 2017 14:26

fix typo

9245631

inital rename packages and namespaces

94c60cb

fix some tests and updated packages

3653f1f

add missing stylecop.json

fbe9d25

tweek codecov cmd file

8f53b5b

skip tests in build on CI server as they are ran from CodeCoverage.cmd

9be5ba0

publish both packages

9a4d048

ensure local running of build scripts runs tests from correct build

c9db035

fix broken test

4c8ba59

tocsoft mentioned this pull request Aug 17, 2017

Update namespaces & package names #297

Merged

Fix skew

4f38761

antonfirsov mentioned this pull request Aug 19, 2017

[WIP] Building an optimized and accurate Jpeg Decoder [deprecated] #298

Merged

10 tasks

Merge pull request #297 from SixLabors/tocsoft/sixlabors_rename

9b34d09

Update namespaces & package names

JimBobSquarePants mentioned this pull request Aug 19, 2017

Beta 1 #299

Merged

4 tasks

AmadeusW mentioned this pull request Sep 8, 2017

Option to have image operations return new images instead of mutating source image #265

Closed

tocsoft merged commit 9b34d09 into master Sep 14, 2017

tocsoft deleted the tocsoft/mutate-api branch September 14, 2017 18:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactored Mutate/Clone APIs #275

Refactored Mutate/Clone APIs #275

tocsoft commented Jul 12, 2017

codecov-io commented Jul 12, 2017 •

edited by codecov bot

Loading

tocsoft Jul 12, 2017

antonfirsov Jul 14, 2017

tocsoft Jul 14, 2017

antonfirsov left a comment

antonfirsov Jul 14, 2017

antonfirsov Jul 14, 2017

antonfirsov Jul 14, 2017 •

edited

Loading

tocsoft Jul 14, 2017

antonfirsov Jul 14, 2017 •

edited

Loading

antonfirsov Jul 14, 2017

tocsoft Jul 14, 2017

tocsoft Jul 14, 2017 •

edited

Loading

antonfirsov commented Jul 15, 2017

tocsoft commented Jul 15, 2017

antonfirsov Jul 16, 2017

tocsoft Jul 16, 2017

antonfirsov Jul 16, 2017

tocsoft Jul 16, 2017

antonfirsov Jul 16, 2017

tocsoft Jul 16, 2017

JimBobSquarePants commented Aug 14, 2017

antonfirsov commented Aug 15, 2017

tocsoft commented Aug 15, 2017

antonfirsov commented Aug 15, 2017

tocsoft commented Aug 15, 2017

tocsoft commented Aug 15, 2017

antonfirsov commented Aug 15, 2017

Refactored Mutate/Clone APIs #275

Refactored Mutate/Clone APIs #275

Conversation

tocsoft commented Jul 12, 2017

Prerequisites

Description

So some code examples.

codecov-io commented Jul 12, 2017 • edited by codecov bot Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antonfirsov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antonfirsov Jul 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antonfirsov Jul 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tocsoft Jul 14, 2017 • edited Loading

Choose a reason for hiding this comment

antonfirsov commented Jul 15, 2017

tocsoft commented Jul 15, 2017

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 Aug 14, 2017

antonfirsov commented Aug 15, 2017

tocsoft commented Aug 15, 2017

antonfirsov commented Aug 15, 2017

tocsoft commented Aug 15, 2017

tocsoft commented Aug 15, 2017

antonfirsov commented Aug 15, 2017

codecov-io commented Jul 12, 2017 •

edited by codecov bot

Loading

antonfirsov Jul 14, 2017 •

edited

Loading

antonfirsov Jul 14, 2017 •

edited

Loading

tocsoft Jul 14, 2017 •

edited

Loading