Skip to content

DrawingCanvas API: Replace imperative extension methods with stateful canvas-based drawing model#377

Open
JimBobSquarePants wants to merge 244 commits intomainfrom
js/canvas-api
Open

DrawingCanvas API: Replace imperative extension methods with stateful canvas-based drawing model#377
JimBobSquarePants wants to merge 244 commits intomainfrom
js/canvas-api

Conversation

@JimBobSquarePants
Copy link
Copy Markdown
Member

@JimBobSquarePants JimBobSquarePants commented Mar 1, 2026

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

Breaking Changes: DrawingCanvas API

Fix #106
Fix #244
Fix #344
Fix #367

This is a major breaking change. The library's public API has been completely redesigned around a canvas-based drawing model, replacing the previous collection of imperative extension methods.

What changed

The old API surface — dozens of IImageProcessingContext extension methods like DrawLine(), DrawPolygon(), FillPolygon(), DrawBeziers(), DrawImage(), DrawText(), etc. — has been removed entirely. These methods were individually simple but suffered from several architectural limitations:

  • Each call was an independent image processor that rasterized and composited in isolation, making it impossible to batch or reorder operations.
  • State (blending mode, clip paths, transforms) had to be passed to every single call.
  • There was no way for an alternate rendering backend to intercept or accelerate a sequence of draw calls.

The new model: DrawingCanvas

All drawing now goes through IDrawingCanvas / DrawingCanvas<TPixel>, a stateful canvas that queues draw commands and flushes them as a batch.

Via Image.Mutate() (most common)

using SixLabors.ImageSharp.Drawing;
using SixLabors.ImageSharp.Drawing.Processing;

image.Mutate(ctx => ctx.Paint(canvas =>
{
    // Fill a path
    canvas.Fill(Brushes.Solid(Color.Red), new EllipsePolygon(200, 200, 100));

    // Stroke a path
    canvas.Draw(Pens.Solid(Color.Blue, 3), new RectangularPolygon(50, 50, 200, 100));

    // Draw a polyline
    canvas.DrawLine(Pens.Solid(Color.Green, 2), new PointF(0, 0), new PointF(100, 100));

    // Draw text
    canvas.DrawText(
        new RichTextOptions(font) { Origin = new PointF(10, 10) },
        "Hello, World!",
        brush: Brushes.Solid(Color.Black),
        pen: null);

    // Draw an image
    canvas.DrawImage(sourceImage, sourceRect, destinationRect);

    // Save/Restore state (options, clip paths)
    canvas.Save(new DrawingOptions
    {
        GraphicsOptions = new GraphicsOptions { BlendPercentage = 0.5f }
    });
    canvas.Fill(brush, path);
    canvas.Restore();

    // Apply arbitrary image processing to a path region
    canvas.Apply(path, inner => inner.Brightness(0.5f));

    // Commands are flushed on Dispose (or call canvas.Flush() explicitly)
}));

Standalone usage (without Image.Mutate)

DrawingCanvas<TPixel> can be created directly from an image or frame using the CreateCanvas(...) extensions:

using var canvas = image.CreateCanvas(new DrawingOptions());

canvas.Fill(brush, path);
canvas.Draw(pen, path);
canvas.Flush();

using var canvas = image.CreateCanvas(frameIndex: 0, options: new DrawingOptions());
// ...

using var canvas = frame.CreateCanvas(new DrawingOptions());
// ...

Canvas state management

The canvas supports a save/restore stack (similar to HTML Canvas or SkCanvas):

int saveCount = canvas.Save();             // push current state
canvas.Save(options, clipPath1, clipPath2); // push and replace state

canvas.Restore();              // pop one level
canvas.RestoreTo(saveCount);   // pop to a specific level

State includes DrawingOptions (graphics options, shape options, transform) and clip paths. SaveLayer creates an offscreen layer that composites back on Restore.

IDrawingBackend — bring your own renderer

The library's rasterization and composition pipeline is abstracted behind IDrawingBackend. This interface has the following methods:

Method Purpose
FlushCompositions<TPixel> Flushes queued composition operations for the target.
TryReadRegion<TPixel> Read pixels back from the target (needed for Process() and DrawImage()).

The library ships with DefaultDrawingBackend (CPU, tiled fixed-point rasterizer). An experimental WebGPU compute-shader backend (ImageSharp.Drawing.WebGPU) is also available, demonstrating how alternate backends plug in. Users can provide their own implementations — for example, GPU-accelerated backends, SVG emitters, or recording/replay layers.

Backends are registered on Configuration:

configuration.SetDrawingBackend(myCustomBackend);

Migration guide

Old API New API
ctx.Fill(color, path) ctx.ProcessWithCanvas(c => c.Fill(Brushes.Solid(color), path))
ctx.Fill(brush, path) ctx.ProcessWithCanvas(c => c.Fill(brush, path))
ctx.Draw(pen, path) ctx.ProcessWithCanvas(c => c.Draw(pen, path))
ctx.DrawLine(pen, points) ctx.ProcessWithCanvas(c => c.DrawLine(pen, points))
ctx.DrawPolygon(pen, points) ctx.ProcessWithCanvas(c => c.Draw(pen, new Polygon(new LinearLineSegment(points))))
ctx.FillPolygon(brush, points) ctx.ProcessWithCanvas(c => c.Fill(brush, new Polygon(new LinearLineSegment(points))))
ctx.DrawText(text, font, color, origin) ctx.ProcessWithCanvas(c => c.DrawText(new RichTextOptions(font) { Origin = origin }, text, Brushes.Solid(color), null))
ctx.DrawImage(overlay, opacity) ctx.ProcessWithCanvas(c => c.DrawImage(overlay, sourceRect, destRect))
Multiple independent draw calls Single ProcessWithCanvas block — commands are batched and flushed together

Other breaking changes in this PR

  • AntialiasSubpixelDepth removed — The rasterizer now uses a fixed 256-step (8-bit) subpixel depth. The old AntialiasSubpixelDepth property (default: 16) controlled how many vertical subpixel steps the rasterizer used per pixel row. The new fixed-point scanline rasterizer integrates area/cover analytically per cell rather than sampling at discrete subpixel rows, so the "depth" is a property of the coordinate precision (24.8 fixed-point), not a tunable sample count. 256 steps gives ~0.4% coverage granularity — more than sufficient for all practical use cases. The old default of 16 (~6.25% granularity) could produce visible banding on gentle slopes.
  • GraphicsOptions.Antialias — now controls RasterizationMode (antialiased vs aliased). When false, coverage is snapped to binary using AntialiasThreshold.
  • GraphicsOptions.AntialiasThreshold — new property (0–1, default 0.5) controlling the coverage cutoff in aliased mode. Pixels with coverage at or above this value become fully opaque; pixels below are discarded.

Benchmarks

All benchmarks run under the following environment.

BenchmarkDotNet=v0.13.1, OS=Windows 10.0.26200
Unknown processor
.NET SDK=10.0.103
  [Host] : .NET 8.0.24 (8.0.2426.7010), X64 RyuJIT

Toolchain=InProcessEmitToolchain  InvocationCount=1  IterationCount=40
LaunchCount=3  UnrollFactor=1  WarmupCount=40

DrawPolygonAll - Renders a 7200x4800px path of the state of Mississippi with a 2px stroke.

Method Mean Error StdDev Median Ratio RatioSD
SkiaSharp 42.20 ms 2.197 ms 6.976 ms 38.18 ms 1.00 0.00
SystemDrawing 44.10 ms 0.172 ms 0.538 ms 44.05 ms 1.07 0.16
ImageSharp 12.09 ms 0.083 ms 0.269 ms 12.06 ms 0.29 0.05
ImageSharpWebGPU 12.47 ms 0.291 ms 0.940 ms 12.71 ms 0.30 0.05

FillParis - Renders 1096x1060px scene containing 50K fill paths.

Method Mean Error StdDev Ratio RatioSD
SkiaSharp 104.46 ms 0.356 ms 1.145 ms 1.00 0.00
SystemDrawing 148.53 ms 0.327 ms 1.033 ms 1.42 0.02
ImageSharp 66.32 ms 0.999 ms 3.083 ms 0.64 0.03
ImageSharpWebGPU 41.95 ms 0.457 ms 1.368 ms 0.40 0.01

@JimBobSquarePants
Copy link
Copy Markdown
Member Author

JimBobSquarePants commented Apr 15, 2026

For the small-image case, I also would not expect MP/s to behave like a typical core pixel processor. Tiger rendering has a substantial amount of per-scene work that does not scale with image area in the same way:
[..]

  • preparing paths and strokes

Is there any more information that could be cached when the same path/stroke is being used?

[..]

  • thread coordination

In the core library, threading overhead is being reduced or sometimes eliminated for small images via. MinimumPixelsProcessedPerTask. Is there a way to introduce a similar mechanism?

We memoise the result of IPath.ToLinearGeometry() when the transform is identity but other than that there's nothing else, we can cache.

I just ran some experiments using MinimumPixelsProcessedPerTask to determine partition counts during scene execution which is the only place it made sense. It led to a noticable degrade in performance.

Using

Scenario Size Width Height Concurrent Requests Drawing Parallelism Total Seconds MegaPixelsPerSec
SingleImage Large 2000 2000 1 1 10.03 129.65
SingleImage Large 2000 2000 1 8 10.00 420.67
SingleImage Large 2000 2000 1 16 10.00 625.07
SingleImage Small 200 200 1 1 10.00 15.09
SingleImage Small 200 200 1 8 10.00 36.57
SingleImage Small 200 200 1 16 10.00 42.25
ServiceThroughput Large 2000 2000 8 1 10.03 710.41
ServiceThroughput Large 2000 2000 4 2 10.01 718.99
ServiceThroughput Large 2000 2000 2 4 10.01 536.53
ServiceThroughput Large 2000 2000 1 8 10.01 420.91

Not using

Scenario Size Width Height Concurrent Requests Drawing Parallelism Total Seconds MegaPixelsPerSec
SingleImage Large 2000 2000 1 1 10.01 159.47
SingleImage Large 2000 2000 1 8 10.00 557.43
SingleImage Large 2000 2000 1 16 10.00 803.04
SingleImage Small 200 200 1 1 10.00 18.75
SingleImage Small 200 200 1 8 10.00 65.62
SingleImage Small 200 200 1 16 10.00 68.08
ServiceThroughput Large 2000 2000 8 1 10.03 782.69
ServiceThroughput Large 2000 2000 4 2 10.01 819.33
ServiceThroughput Large 2000 2000 2 4 10.01 662.20
ServiceThroughput Large 2000 2000 1 8 10.00 590.34

@JimBobSquarePants
Copy link
Copy Markdown
Member Author

I renamed the entry point to Paint

Paint sounds awesome!

When I hold -c 1 and vary only -p, internal parallelism is clearly helping rather than hurting [...] So I do not think the previous result supports the conclusion that our internal parallel execution is slower than serial.

Single-user "slowness" wasn't the conclusion I wanted to imply. The data proves that given high concurrency, parallelism hurts throughput, which is something I've seen to happen only with Convolution2DProcessor in the core library. The reason this matters is because AFAIK server-side processing is still the primary use case of the CPU renderer, and server-side processing usually needs to scale well with concurrency. IMO good library design means delivering good defaults for the primary use case, i.e. not asking the user in the documentation to tune MaxDegreeOfParallelism for good throughput. Given you don't want to abandon parallelism, my suggestion is to root cause and fix this before the release.

I will try helping by running some profiling.

I agree that high-concurrency service throughput is worth understanding, but I don't think it should automatically define the default optimization target for ImageSharp.Drawing.

I don't see this library primarily as a web-server drawing component in the same way some ImageSharp image-processing workloads are. The scenarios I have in mind are things like CAD-style rendering, charts and graphs, UI generation, tooling, and other programmatic rendering workloads where single-render performance and overall rendering capability matter more than maximizing throughput under heavily concurrent request load.

That makes the current single-request results directly relevant, and it also means I'm comfortable with MaxDegreeOfParallelism remaining configurable for hosts that do need to tune for saturated server throughput. The data you've gathered is exactly the tuning guidance those hosts need, and I'd rather surface that in the docs than bake in a default that trades away single-render performance for a server shape that isn't the primary use case.

I did explore whether the library could self-throttle under concurrent load without user tuning. The honest answer is that any in-library mechanism needs either a shared scheduler across requests or a runtime pool-pressure signal, neither of which exists in a form we can consume without significant upstream changes. The Parallel.For + MaxDegreeOfParallelism model has the same property everywhere it's used in the ecosystem, and I don't think ImageSharp.Drawing is the right place to invent a workaround for it.

@antonfirsov
Copy link
Copy Markdown
Member

antonfirsov commented Apr 15, 2026

CPU rasterizer parallel performance

The scenarios I have in mind are things like CAD-style rendering, charts and graphs, UI generation, tooling, and other programmatic rendering workloads where single-render performance and overall rendering capability matter more than maximizing throughput under heavily concurrent request load.

Scenarios that involve immediate, on-screen rendering are better addressed by the WebGPU renderer. Whatever is the current primary use-case of ImageSharp.Drawing V2, will be the primary use case of the CPU renderer going forward (which I assume is server, but I may be wrong).

That said, I just ran the same benchmarks with V2 rasterizer, and this PR is significantly improving every aspect perf-wise (🚀), regardless if parallelism is used or not, so I'm no longer pursuing to figure this out in the PR.

I did explore whether the library could self-throttle under concurrent load without user tuning.

I don't know if self-throttling is the only possible answer here, I would recommend to open a tracking issue once this is merged.

I'll move on reviewing other aspects now.

Comment thread samples/DrawShapesWithImageSharp/Program.cs Outdated
Copy link
Copy Markdown
Member

@antonfirsov antonfirsov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Plenty of notes although the review is still incomplete. Most importantly, there seems to be a bug in the GPU renderer, see the comment on the lines demo.

Comment thread samples/DrawingBackendBenchmark/WebGpuBenchmarkBackend.cs
Comment thread src/ImageSharp.Drawing.WebGPU/WebGPUHandle.cs Outdated
Comment thread src/ImageSharp.Drawing.WebGPU/WebGPUFlushContext.cs Outdated
texture = flushContext.Api.DeviceCreateTexture(flushContext.Device, in textureDescriptor);
if (texture is null)
{
error = "Failed to create WebGPU composition texture.";
Copy link
Copy Markdown
Member

@antonfirsov antonfirsov Apr 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: passing around these error strings is odd, it would be cleaner to emit these messages to some sort of IErrorLogger.

toolbar.Controls.Add(this.CreateRunButton("1k", 1_000));
toolbar.Controls.Add(this.CreateRunButton("10k", 10_000));
toolbar.Controls.Add(this.CreateRunButton("100k", 100_000));
toolbar.Controls.Add(this.CreateRunButton("200k", 200_000));
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually wanted to make this 1M, but that makes the implementation hit buffer limits:

WebGPU (Failed: The staged-scene path tiles buffer requires 141001808 bytes, exceeding the current WebGPU binding limit of 134217728 bytes.)

Definitely not a show stopper, but some users may be limited by this, so it's worth noting down.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be possible to work around this. There are hard limits to what you can send though.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Workaround is good, but in long term I would view it as an optimization problem. The buffers are likely heavier than needed.

canvas.Flush();
}

stopwatch.Stop();
Copy link
Copy Markdown
Member

@antonfirsov antonfirsov Apr 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are closing down the perf gap to Skia, but it's still 6-7x on my machine. This is acceptable but worth noting down. According to the profile of rendering 200k lines, it's the buffer mapping that seems to take a lot of time:

Image

Copy link
Copy Markdown
Member Author

@JimBobSquarePants JimBobSquarePants Apr 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, at 200K I'm seeing about 4x. That's annoying. I'll see what I can do.

Comment thread src/ImageSharp.Drawing.WebGPU/WebGPUWindow{TPixel}.cs Outdated
Comment thread src/ImageSharp.Drawing.WebGPU/WebGPURenderTarget{TPixel}.cs Outdated
Comment thread src/ImageSharp.Drawing.WebGPU/WebGPURenderTarget{TPixel}.cs Outdated
Comment thread src/ImageSharp.Drawing.WebGPU/WebGPUWindow{TPixel}.cs Outdated
Copy link
Copy Markdown
Member

@antonfirsov antonfirsov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Mostly) high-level notes on IDrawingCanvas & DrawingCanvas<TPixel>.

Comment thread src/ImageSharp.Drawing/Processing/DrawingCanvas{TPixel}.cs
Comment thread src/ImageSharp.Drawing/Processing/IDrawingCanvas.cs Outdated
Comment thread src/ImageSharp.Drawing/Processing/DrawingCanvas{TPixel}.cs
Comment thread src/ImageSharp.Drawing/Processing/DrawingOptionsDefaultsExtensions.cs Outdated
Comment thread src/ImageSharp.Drawing/Processing/IDrawingCanvas.cs Outdated
Comment thread src/ImageSharp.Drawing/Processing/DrawingCanvas{TPixel}.cs Outdated
Comment thread src/ImageSharp.Drawing/Processing/DrawingCanvas{TPixel}.cs

// Defensive guard: built-in backends should provide either direct readback (CPU/backed surface)
// or shadow fallback. If this fails, it indicates a backend implementation bug or an unsupported custom backend.
if (!this.TryCreateProcessSourceImage(sourceRect, out Image<TPixel>? sourceImage))
Copy link
Copy Markdown
Member

@antonfirsov antonfirsov Apr 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apply methods are very expensive on GPU backends in comparison to GPU accelerated drawing operations. Since the IImageProcessingContext interface doesn't make it obvious there is an Image<T> backed by CPU memory, this can be even surprising. Is there any reasonable scenario where user would be willing to pay the high price? If not, I would say it makes sense to always throw a NotSupportedException on GPU.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The cost is more bounded than it looks. Apply on GPU scissors to the path's AABB, not the full frame:

  1. Flush queued GPU work so the read sees the correct backdrop.
  2. backend.TryReadRegion(...) reads only the path's bounding rectangle.
  3. Image<TPixel>.Mutate(operation) runs the processor on that sub-image.
  4. Result is wrapped as an ImageBrush and composited via the normal GPU fill path.

A 100×100 blur on a 1920×1080 canvas is 10k pixels readback, not 2M. The real cost is the processor itself, which is what the user asked for.

This is the bridge to the entire ImageSharp.Processing surface: blur, convolutions, color correction, quantization, dither, pixelate, resize, tone curves, edge detection, user-authored IImageProcessors. None exist as GPU-native primitives and most never would/could.

If we throw on GPU, the user wanting a blurred backdrop has to read the region back manually (usually a larger rect than the AABB), run the processor, build an ImageBrush, draw it. Same four steps, moved into every caller, with worse scissoring.

Copy link
Copy Markdown
Member

@antonfirsov antonfirsov Apr 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The cost is more bounded than it looks

Have you benchmarked drawing a scene with Apply involved + same without apply?

A 100×100 blur on a 1920×1080 canvas is 10k pixels readback, not 2M.

I expect the readback to imply a significant synchronization cost regardless of the amount of data. Note that it is implemented via sync-over-async to keep the API synchronous:

BufferMapAsyncStatus mapStatus = BufferMapAsyncStatus.Unknown;
using ManualResetEventSlim mapReady = new(false);
void Callback(BufferMapAsyncStatus status, void* userData)
{
_ = userData;
mapStatus = status;
mapReady.Set();
}
using PfnBufferMapCallback callback = PfnBufferMapCallback.From(Callback);
api.BufferMapAsync(readbackBuffer, MapMode.Read, 0, (nuint)readbackByteCount, callback, null);
if (!WaitForMapSignal(wgpuExtension, device, mapReady) || mapStatus != BufferMapAsyncStatus.Success)


the processor itself, which is what the user asked for.

For a newcomer who wants to use the WebGPU pipeline and didn't use ImageSharp before, it might not be obvious from the IImageProcessingContext API shape that processors are running on the CPU.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even as a relative newcomer to GPU programming I would expect the synchronization cost. Something must be copied.

I do not think an "Apply vs no Apply" benchmark would answer this cleanly. That would measure the selected processor, the readback, and the final composition together not the readback cost unless we use an artificial/no-op processor, and then the result is not especially representative of real usage.

The important implementation choice here is that the readback is scoped to the affected bounds rather than the full canvas.

I can make the synchronous readback behavior explicit in the supplementary docs, but I do not think the benchmark suggested would prove much beyond the fact that this operation crosses the GPU/CPU boundary.

Copy link
Copy Markdown
Member

@antonfirsov antonfirsov Apr 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm suggesting the benchmark mostly because you seem to be skeptical that the cost of the readback would be significant. I think it's possible to design it to be informative, e.g. via parametrization.

I'm mostly viewing this API from long-term perspective: even if Apply is relatively less painful in the current pipeline, in a future, highly optimized version of ImageSharp.Drawing.WebGPU the readback+CPU processor cost would be very noticeable. Users who want to render complex scenes with high FPS should avoid Apply.

can make the synchronous readback behavior explicit in the supplementary docs

Yes please! This would address my concern. I would even add a note in the API docs, for example:

<remarks>
On a GPU backend this is implemented using readback and an image processor running on the CPU.
</remarks>

Copy link
Copy Markdown
Member

@antonfirsov antonfirsov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of random observations and follow-ups to previous discussions. The GH review UI is weird and shows comments added during my review to previous discussions two times: in those discussions and here in form of "new" comments.

Will move on to WebGPUHostedWindow<T> tomorrow.

Comment thread src/ImageSharp.Drawing/PolygonGeometry/StrokedShapeGenerator.cs Outdated
Comment thread src/ImageSharp.Drawing.WebGPU/WebGPUTextureTransfer.cs Outdated

// Defensive guard: built-in backends should provide either direct readback (CPU/backed surface)
// or shadow fallback. If this fails, it indicates a backend implementation bug or an unsupported custom backend.
if (!this.TryCreateProcessSourceImage(sourceRect, out Image<TPixel>? sourceImage))
Copy link
Copy Markdown
Member

@antonfirsov antonfirsov Apr 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The cost is more bounded than it looks

Have you benchmarked drawing a scene with Apply involved + same without apply?

A 100×100 blur on a 1920×1080 canvas is 10k pixels readback, not 2M.

I expect the readback to imply a significant synchronization cost regardless of the amount of data. Note that it is implemented via sync-over-async to keep the API synchronous:

BufferMapAsyncStatus mapStatus = BufferMapAsyncStatus.Unknown;
using ManualResetEventSlim mapReady = new(false);
void Callback(BufferMapAsyncStatus status, void* userData)
{
_ = userData;
mapStatus = status;
mapReady.Set();
}
using PfnBufferMapCallback callback = PfnBufferMapCallback.From(Callback);
api.BufferMapAsync(readbackBuffer, MapMode.Read, 0, (nuint)readbackByteCount, callback, null);
if (!WaitForMapSignal(wgpuExtension, device, mapReady) || mapStatus != BufferMapAsyncStatus.Success)


the processor itself, which is what the user asked for.

For a newcomer who wants to use the WebGPU pipeline and didn't use ImageSharp before, it might not be obvious from the IImageProcessingContext API shape that processors are running on the CPU.

Comment thread src/ImageSharp.Drawing/Processing/DrawingCanvas{TPixel}.cs Outdated
Comment thread src/ImageSharp.Drawing/Processing/DrawingCanvasExtensions.cs Outdated
/// A drawing canvas over a frame target.
/// </summary>
/// <typeparam name="TPixel">The pixel format.</typeparam>
public sealed partial class DrawingCanvas<TPixel> : IDrawingCanvas
Copy link
Copy Markdown
Member

@antonfirsov antonfirsov Apr 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a good reason to publicly expose the DrawingCanvas<T> type instead of just having a factory methods returning IDrawingCanvas instances? (Which are already present in DrawingCanvasExtensions)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. DrawingCanvas<TPixel> is the concrete typed canvas, and I want that to remain part of the public surface.

IDrawingCanvas is useful as the common operation contract, but it is not a replacement for the concrete type. The concrete type carries the pixel type, exposes typed construction over Buffer2DRegion<TPixel> / ICanvasFrame<TPixel>, and returns typed child canvases from CreateRegion. That matters for callers building canvases over non-Image<TPixel> targets, including backend integrations.

The factory methods are convenience entry points for Image<TPixel> / ImageFrame<TPixel>. They should return the concrete type for the same reason ImageSharp APIs generally return concrete typed objects when the pixel type is known. Callers who want abstraction can still store it as IDrawingCanvas, but forcing the factory methods to return IDrawingCanvas would throw away useful type information without simplifying the implementation.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked into this even deeper, and now I strongly recommend to give this another thought, because seeing it with fresh eyes, to me it looks like the pixel type can be simplified away from most APIs, leading to smaller API surface end elimination of runtime costs from generic-instantiation. The pixel type information is only useful if there are use-cases that won't work with pixel-agnostic canvas. I'm not convinced this is the case.

They should return the concrete type for the same reason ImageSharp APIs generally return concrete typed objects when the pixel type is known

Image<TPixel> has pixel specific API-s, most importantly ProcessPixelRows where the consumer needs to know the pixel type. There are no such use cases for the canvas where the pixel type is relevant for the consumer.

exposes typed construction over Buffer2DRegion<TPixel> / ICanvasFrame<TPixel> , and returns typed child canvases from CreateRegion

The canvas returned by those methods can be as well pixel-agnostic.

That matters for callers building canvases over non-Image<TPixel> targets, including backend integrations.

From what I see, all dependent types backend types (WebGPUDeviceContext, WebGPURenderTarget, WebGPUWindowFrame, WebGPUHostedWindow ...) can be made pixel-agnostic with pixel-specific factory methods here and there to do the TPixel -> CompositePixelRegistration mapping.

Do you have a specific example that would prove me wrong?

Comment thread src/ImageSharp.Drawing/Processing/DrawingCanvas{TPixel}.cs
/// <summary>
/// Convenience shape helpers that build paths and forward to the core <see cref="IDrawingCanvas"/> fill and draw primitives.
/// </summary>
public static class DrawingCanvasShapeExtensions
Copy link
Copy Markdown
Member

@antonfirsov antonfirsov Apr 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What decides if a method should go here or to IDrawingCanvas? As pointed out in another comment, DrawingCanvas<T> : IDrawingCanvas has a couple of delegating methods that don't use it's internal state directly.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved these to extensions to address the earlier symmetry feedback, and this is the boundary I want here.

IDrawingCanvas contains the core canvas contract: state/layers, region creation, core fill/draw/apply/text/image operations, and flushing. DrawingCanvasShapeExtensions contains shape convenience overloads that only build paths or iterate path collections before forwarding to the core Fill/Draw methods.

These methods do not define new canvas behavior, so they should stay as extensions rather than expanding the interface.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IDrawingCanvas contains the core canvas contract

It also contains many methods that are implemented trivially via delegation: SaveLayer(GraphicsOptions layerOptions), SaveLayer(GraphicsOptions layerOptions, Rectangle bounds), Fill(Brush brush), potentially Clear(Brush brush). Do you consider that core functionality? Woldn't it make sense to move those to extension methods too?

Copy link
Copy Markdown
Member

@antonfirsov antonfirsov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comments on high level API shape & behaviors. Follow-up on the discussion on pixel specific API-s.

Comment on lines +12 to +14
public sealed class NativeSurface
{
private readonly ConcurrentDictionary<Type, object> capabilities = new();
Copy link
Copy Markdown
Member

@antonfirsov antonfirsov Apr 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From what I see, this will only contain a single WebGPUSurfaceCapability in practice. Is it realistic that a backend would need to store multiple "capabilities" given - just like WebGPUSurfaceCapability - it can aggregate all necessary information in a single type?

If not the pattern could be simplified:

public interface INativeSurface { }

class WebGPUSurface : INativeSurface
{
    private readonly WebGPUDeviceHandle deviceHandle;
    private readonly WebGPUQueueHandle queueHandle;
    private readonly WebGPUTextureHandle targetTextureHandle;
    private readonly WebGPUTextureViewHandle targetTextureViewHandle;
}

/// <summary>
/// Gets pixel format information for this destination surface.
/// </summary>
public PixelTypeInfo PixelType { get; }
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is unused. Is it really needed?

/// <remarks>
/// The backing WebGPU device, queue, texture, and texture view must remain valid while canvases target this surface.
/// </remarks>
public sealed class WebGPUSurfaceCapability
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need to be public? Do we expect a user directly interact with this type in practice?

/// A drawing canvas over a frame target.
/// </summary>
/// <typeparam name="TPixel">The pixel format.</typeparam>
public sealed partial class DrawingCanvas<TPixel> : IDrawingCanvas
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked into this even deeper, and now I strongly recommend to give this another thought, because seeing it with fresh eyes, to me it looks like the pixel type can be simplified away from most APIs, leading to smaller API surface end elimination of runtime costs from generic-instantiation. The pixel type information is only useful if there are use-cases that won't work with pixel-agnostic canvas. I'm not convinced this is the case.

They should return the concrete type for the same reason ImageSharp APIs generally return concrete typed objects when the pixel type is known

Image<TPixel> has pixel specific API-s, most importantly ProcessPixelRows where the consumer needs to know the pixel type. There are no such use cases for the canvas where the pixel type is relevant for the consumer.

exposes typed construction over Buffer2DRegion<TPixel> / ICanvasFrame<TPixel> , and returns typed child canvases from CreateRegion

The canvas returned by those methods can be as well pixel-agnostic.

That matters for callers building canvases over non-Image<TPixel> targets, including backend integrations.

From what I see, all dependent types backend types (WebGPUDeviceContext, WebGPURenderTarget, WebGPUWindowFrame, WebGPUHostedWindow ...) can be made pixel-agnostic with pixel-specific factory methods here and there to do the TPixel -> CompositePixelRegistration mapping.

Do you have a specific example that would prove me wrong?

Comment on lines +26 to +33
public bool TryGetCpuRegion(out Buffer2DRegion<TPixel> region);

/// <summary>
/// Attempts to get an opaque native destination surface.
/// </summary>
/// <param name="surface">The native surface when available.</param>
/// <returns><see langword="true"/> when a native surface is available.</returns>
public bool TryGetNativeSurface([NotNullWhen(true)] out NativeSurface? surface);
Copy link
Copy Markdown
Member

@antonfirsov antonfirsov Apr 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is an exclusive or relationship between TryGetCpuRegion and TryGetNativeSurface in implementing types. ICanvasFrame<TPixel> seems to be the only abstraction that makes the presence of TPixel viral through all APIs.

If instead DrawingCanvas<TPixel> would contain either a Buffer2DRegion<TPixel> or a NativeSurface, it seems to me that consumer types could be made pixel-agnostic.

The functionality provided by CanvasRegionFrame<TPixel> can be implemented by other means.

this.DiagnosticLastSceneFailure = exceedsBindingLimit
? error ?? "The staged WebGPU scene exceeded the current binding limits."
: error ?? "Failed to create the staged WebGPU scene.";
this.FlushCompositionsFallback(configuration, target, compositionScene, compositionBounds: null);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fallback mechanism should be optional (IMO rather opt-in than opt-out). For users who want rendering to be fast, the WebGPU failure is an error scenario.

When fallbacks happen, the reason should be observable by the user, which points towards having a public logging mechanism instead of a test-only DiagnosticLastSceneFailure.

Comment on lines +416 to +417
_ = sender;
_ = e;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: if possible, I would rather suppress analyzers reporting errors/warnings here than have these NOP assignments.

/// <summary>
/// Convenience shape helpers that build paths and forward to the core <see cref="IDrawingCanvas"/> fill and draw primitives.
/// </summary>
public static class DrawingCanvasShapeExtensions
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IDrawingCanvas contains the core canvas contract

It also contains many methods that are implemented trivially via delegation: SaveLayer(GraphicsOptions layerOptions), SaveLayer(GraphicsOptions layerOptions, Rectangle bounds), Fill(Brush brush), potentially Clear(Brush brush). Do you consider that core functionality? Woldn't it make sense to move those to extension methods too?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

2 participants