Skip to content

Expose ImageProcessor internals to unblock externalizing ImageSharp.Drawing #967

@antonfirsov

Description

@antonfirsov

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:

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.

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    No type

    Projects

    No projects

    Milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions