-
-
Notifications
You must be signed in to change notification settings - Fork 888
Description
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>andPixelOperations<T>.GetPixelBlender(...)IImageVisitorandImage.AcceptVisitor(...)ParallelExecutionSettings,configuration.GetParallelSettings()andParallelHelper
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. KeepMemorySource<T>hidden. Consider reintroducingIBuffer2D<T>. - Expose
ImageProcessor<T>but consider refactoring the "processor application" logic. See follow-up explanation. - Expose
PixelBlender<T>andPixelOperations<T>.GetPixelBlender(...) - Move
IImageVisitorto theAdvancedsubnamespace, exposeIImageVisitorandImage.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:
constructor logic:BeforeImageApplyshould be replaced withIImageProcessor.CreatePixelSpecificProcessor()should takeImage<T>and aRectanglewhich is then passed toImageProcessor<T>constructor, initialization and resource allocation could be done here.AfterImageApplyshould be replaced withDispose()(to clean up temporary resources)Image<T>,SourceRectangleandConfigurationcan be membersIImageProcessor<TPixel>.Apply()should be non-parametric. Consider renaming it toExecute()
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 maybeTargetRectangle) behavior. See: comments - Review
IImageProcessorvsICloningImageProcessorbehavior. 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.