Expose ImageProcessor internals to unblock externalizing ImageSharp.Drawing

@Sergio0694

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

Expose Buffer2D<T> with a minimal subset of public methods. Keep MemorySource<T> hidden. Consider reintroducing IBuffer2D<T>.
Expose ImageProcessor<T> but consider refactoring the "processor application" logic. See follow-up explanation.
Expose PixelBlender<T> and PixelOperations<T>.GetPixelBlender(...)
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

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

UPDATE 3:

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.

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.

@tocsoft

@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.

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

@tocsoft

@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.

@tocsoft

@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?

Added an update to the issue description.

@antonfirsov

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

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.

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

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

Expose pixelblenders. Touch #967

Merge pull request #1028 from SixLabors/js/expose-pixel-blenders

Fix color and pixel operations. Touches #967 #1002

Fix color and pixel operations. Touches SixLabors#967 SixLabors#1002

Fix color and pixel operations. Touches #967 #1002

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.

ImageSharp/src/ImageSharp/Advanced/ParallelUtils/ParallelHelper.cs

Line 85 in 06bf7ac

internal static void IterateRowsWithTempBuffer<T>(

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

Navigation Menu

Repository navigation

Expose ImageProcessor internals to unblock externalizing ImageSharp.Drawing #967

Plans to externalize SixLabors.ImageSharp.Drawing

Blockers

Suggested solution

Proposal to refactor current ImageProcessor logic

Further, "nice to have" points

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Expose ImageProcessor internals to unblock externalizing ImageSharp.Drawing #967

Description

Plans to externalize SixLabors.ImageSharp.Drawing

Blockers

Suggested solution

Proposal to refactor current ImageProcessor logic

Further, "nice to have" points

Activity

tocsoft commented on Aug 19, 2019

JimBobSquarePants commented on Aug 20, 2019

JimBobSquarePants commented on Aug 20, 2019

antonfirsov commented on Aug 20, 2019

antonfirsov commented on Aug 24, 2019

antonfirsov commented on Aug 24, 2019

JimBobSquarePants commented on Aug 25, 2019

JimBobSquarePants commented on Sep 17, 2019

JimBobSquarePants commented on Oct 14, 2019

antonfirsov commented on Oct 14, 2019

JimBobSquarePants commented on Jan 9, 2020

JimBobSquarePants commented on Jan 23, 2020

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions