Expose ImageProcessor internals to unblock externalizing ImageSharp.Drawing

antonfirsov commented 5 years ago

Plans to externalize SixLabors.ImageSharp.Drawing

The main library, SixLabors.ImageSharp almost reached Release Candidate quality, however we realized, that it will take much more time to bring SixLabors.ImageSharp.Drawing and it's base libraries SixLabors.Shapes and SixLabors.Fonts to the same level.

In order to unblock the release of ImageSharp, we decided to externalize SixLabors.ImageSharp.Drawing. It shall be:

Versioned independently
Moved to a separate GitHub repository

We are afraid that Drawing, Shapes, Fonts will reach RC status only after the 1.0 release of the main library.

Blockers

Unfortunately, externalizing ImageSharp.Drawing is not a trivial job, since it depends on certain internals of the main library by using [InternalsVisibleTo(...)], namely:

Buffer2D<T>
ImageProcessor<T>
PixelBlender<T> and PixelOperations<T>.GetPixelBlender(...)
IImageVisitor and Image.AcceptVisitor(...)
ParallelExecutionSettings, configuration.GetParallelSettings() and ParallelHelper

These issues are actually well-known to anyone trying to implement Image Processors, and complaining about classes being internal. (Hi there @Sergio0694 😄!) This turns the issue of ImageSharp.Drawing into an API-cleanup problem.

Suggested solution

[x] Expose Buffer2D<T> with a minimal subset of public methods. Keep MemorySource<T> hidden. Consider reintroducing IBuffer2D<T>.
[x] Expose ImageProcessor<T> but consider refactoring the "processor application" logic. See follow-up explanation.
[x] Expose PixelBlender<T> and PixelOperations<T>.GetPixelBlender(...)
[x] Move IImageVisitor to the Advanced subnamespace, expose IImageVisitor and Image.AcceptVisitor(...)
[ ] Find out what to do with the rest 😄. Personally, I'm not sure if we should encourage parallel pixel processing. It might be better+easier to keep that logic hidden, and replicate it in the Drawing library.

Proposal to refactor current ImageProcessor logic

IQuantizer is a good example for the interaction and lifecycle rules of non-generic (pixel agnostic) objects and their generic (pixel-specific) counterparts:

The pixel-agnostic object is immutable, and reusable.
Creates a one-shot instances of pixel-specific, mutable "applicator" classes.
The applicator object may create temporary resources (memory), therefore it's disposable

I think we should fully adapt this pattern for Image Processors:

~BeforeImageApply should be replaced with~ constructor logic: IImageProcessor.CreatePixelSpecificProcessor() should take Image<T> and a Rectangle which is then passed to ImageProcessor<T> constructor, initialization and resource allocation could be done here.
~AfterImageApply should be replaced with~ Dispose() (to clean up temporary resources)
Image<T>, SourceRectangle and Configuration can be members
IImageProcessor<TPixel>.Apply() should be non-parametric. Consider renaming it to Execute()

UPDATE: I no longer think that constructor/Dispose are replacements for Before/After ImageApply. Those should exist side-by side.

UPDATE 2:

Further, "nice to have" points

[x] Review SourceRectangle (and maybe TargetRectangle) behavior. See: comments
[x] Review IImageProcessor vs ICloningImageProcessor behavior. Expose (I)CloningImageProcessor

UPDATE 3:

[x] Expose PixelOperations<T>.To/FromVector4(...) (nice to have, currently not used by Drawing)

To avoid a longer delay, I'm fine if we can't address them within the current API cleanup process, since these are no trivial topics.

tocsoft commented 5 years ago

I feel we should also take the opportunity to remove the passing of a Rectangle into IImageProcessingContext.ApplyProcessor() & IImageProcessor.CreatePixelSpecificProcessor() the rectangle feels like it should be a concern of individual processors configuration as not all processors honor the rectangle so it becomes a bit of a lie to have it always passed.

JimBobSquarePants commented 5 years ago

@tocsoft Yeah, I had a good dig through the code there and I don't think there's any reason why we cannot do that.

JimBobSquarePants commented 5 years ago

Been looking at the open PR regarding this and we need to fix up the way we use the term Clone and DeepClone in the library. Currently we use both for what is a deep clone. I suggest we always use DeepClone everywhere

antonfirsov commented 5 years ago

@tocsoft good idea. We need to provide specific guidance for #983 to make it happen. I'll also have a dig through the code to see what we can do.

antonfirsov commented 5 years ago

@tocsoft @JimBobSquarePants I had a deeper look at the source, now I'm unsure whether this is the right thing to do.

In my understanding the majority of the processors actually do honor SourceRectangle. Drawing processors are an exception, but it would probably make sense to respect sourceRectangle in drawing. (Eg: user wants to draw to a given subarea of an image masking out the rest, even if some drawing artifacts go outside the bounds.)

I think we could even generalize this logic, and introduce ImageArea and ImageFrameArea<T> types:

class ImageFrameArea<T>
{
    public ImageFrame<T> Frame { get; }
    public Rectangle Rectangle { get; }
    public Span<T> GetPixelRowSpan(int y); // gets the subspan inside the area.
                                           // y = 0 is at row Rectangle.Top
    // etc.
}

Am I missing an important point? Are there other exceptions?

antonfirsov commented 5 years ago

Added an update to the issue description.

JimBobSquarePants commented 5 years ago

@antonfirsov having had a good dig through via refactoring I tend to agree. We can handle that once I finish implementing IDisposable

JimBobSquarePants commented 5 years ago

Just pushed a PR to clean up the way we were implementing IDisposable. I'm gonna have a look at moving the visitor extensions now.

JimBobSquarePants commented 5 years ago

I've started working on exposing the pixel blenders. The lack of method documentation is a chore though!

antonfirsov commented 5 years ago

Great news! I will be available for reviews in the next days.

JimBobSquarePants commented 4 years ago

These APIs are basically the last two methods I think we should expose. They're not currently used in Drawing but I think would be very useful elsewhere.

I'm definitely going to rename anything Helper to Utils for the sake of my sanity anyway.

https://github.com/SixLabors/ImageSharp/blob/06bf7ace63cf5ed4e7576895f27fb66b9c35fe90/src/ImageSharp/Advanced/ParallelUtils/ParallelHelper.cs#L85

JimBobSquarePants commented 4 years ago

I think we can close this since ImageSharp.Drawing is now in an external repository with no internal access.

SixLabors / ImageSharp