DrawingCanvas API: Replace imperative extension methods with stateful canvas-based drawing model#377
DrawingCanvas API: Replace imperative extension methods with stateful canvas-based drawing model#377JimBobSquarePants wants to merge 205 commits intomainfrom
Conversation
OK... I've wired the backend up so it respects I've also removed the multiple parallel steps in the CPU scene builder. There's now only 2 that run always and one optional one when clipping or dashing is required.
All three using the same pattern: int partitionCount = Math.Min(
workItemCount,
requestedParallelism == -1 ? Environment.ProcessorCount : requestedParallelism);
Parallel.For(
0,
count,
new ParallelOptions { MaxDegreeOfParallelism = partitionCount },
...);Given that the parallel approach we take is used for almost all image processing operations I think that if this model caused problems under server load, it would have shown up years ago across the entire library. On top of that I've managed to massively improve performance and reduce the task workload by doing a few things.
I'm getting good competitive numbers across all test scenarios. |
As seen in SixLabors/ImageSharp#3111, most but not all processors react well to parallelization. To be on the safe side, I want to run those benchmarks against the new rasterizer. |
This should scale better than the poor examples given the sheer amount of work that takes place per task, but it would be good to see. |
There was a problem hiding this comment.
Tests:
I'm not sure if all test output looks as expected and I'm somewhat worried if the new test suite gives enough confidence in the implementation, since it seems to be much smaller than the deleted tests.
It shouldn't be hard to migrate the existing tests to the new APIs with LLM-s, or at least a reasonable subset. That coverage is valuable and helps to protect us with changes of this size.
| /// <returns> | ||
| /// A new array containing the elements of both source arrays. | ||
| /// </returns> | ||
| public static T[] Merge<T>(this T[] source1, T[] source2) |
There was a problem hiding this comment.
This should be Concat in .NET terminology. From "merging arrays" I usually associate to this.
| for (int i = 0; i < source1.Length; i++) | ||
| { | ||
| target[i] = source1[i]; | ||
| } | ||
|
|
||
| for (int i = 0; i < source2.Length; i++) | ||
| { | ||
| target[i + source1.Length] = source2[i]; | ||
| } |
There was a problem hiding this comment.
This could be replaced by array or span copy.
| @@ -0,0 +1,120 @@ | |||
| | |||
| Microsoft Visual Studio Solution File, Format Version 12.00 | |||
There was a problem hiding this comment.
I don't see many benefits from this isolation: There aren't that many projects to make IDE-s struggle, and if changing product code working with the other solution breaks samples, they would need to be updated manually.
There was a problem hiding this comment.
This looks odd to me. Are you sure it's the correct output? Have you double and triple checked all output manually?
There was a problem hiding this comment.
Is this supposed to be empty?
| namespace SixLabors.ImageSharp.Drawing.Tests.Processing; | ||
|
|
||
| [GroupOutput("Drawing")] | ||
| public partial class DrawingCanvasTests |
There was a problem hiding this comment.
The amount of pictures in the output is very low compared to what we had previously. I understand these tests try to be more "dense" but there was significant value in the previous per-functionality testing.
I think we are losing a lot of coverage by deleting that test suite entirely.
There was a problem hiding this comment.
A few important remarks on the core library code, but this review focuses mostly on WebGPU utility code.
There is one missing feature: how would one create a NativeSurface/Canvas around an existing WindowHandle? This is important if someone wants to render to an existing area within a Windows application via GPU.
I also noticed many odd behaviors and (code) quality issues done by LLM-s. IMO the amount of how many of those will slip through to a release (and potentially to APIs) is proportional to the speed you want to go with so I wonder how much compromise are you willing to make in order to get things released quickly? I'm worried that given the scale, there might be still many issues in this PR, which will may create significant technical debt if not dealt with now. This is basically the million dollar question of AI assisted coding today.
| /// <param name="workItemCount">The total number of work items available for partitioning.</param> | ||
| /// <returns>The number of partitions to schedule.</returns> | ||
| public static int GetPartitionCount(int maxDegreeOfParallelism, int workItemCount) | ||
| => maxDegreeOfParallelism == -1 ? workItemCount : Math.Min(maxDegreeOfParallelism, workItemCount); |
There was a problem hiding this comment.
Do I understand it right that when workItemCount = scene.RowCount, it means the number of rows to render. I don't see why would one need MaxDegreeOfParallelism to be that high, am I missing something?
See my comment on SixLabors/ImageSharp#3110.
| /// <inheritdoc /> | ||
| protected override void OnFrameApply(ImageFrame<TPixel> source) | ||
| { | ||
| using DrawingCanvas<TPixel> canvas = source.CreateCanvas(this.definition.Options); |
There was a problem hiding this comment.
This does not propagate this.Configuration so anything we pass to Mutate/Clone is gone. Would might make sense to test the propagation too.
| /// reinitialized later by calling <see cref="Acquire"/> again. | ||
| /// </remarks> | ||
| /// <exception cref="InvalidOperationException">Thrown when runtime leases are still active.</exception> | ||
| public static void Shutdown() |
There was a problem hiding this comment.
This is unused, consequentially leaseCount and the whole Lease mechanism is pointless.
| /// Canvas frame adapter that exposes both a CPU region and a native surface. | ||
| /// </summary> | ||
| /// <typeparam name="TPixel">The pixel format.</typeparam> | ||
| public sealed class HybridCanvasFrame<TPixel> : ICanvasFrame<TPixel> |
There was a problem hiding this comment.
I'm failing to find the intended use-case. Is there any test or example code using these?
| public static void Main() | ||
| { | ||
| // FIFO is the safest sample default: it presents in display order with normal v-sync behavior. | ||
| using WebGPUWindow<Bgra32> window = new(new WebGPUWindowOptions |
There was a problem hiding this comment.
Can one make this fullscreen?
| /// <param name="destination">The destination image that receives the readback pixels.</param> | ||
| /// <param name="error">Receives the failure reason when readback cannot complete.</param> | ||
| /// <returns><see langword="true"/> when readback succeeds; otherwise <see langword="false"/>.</returns> | ||
| public bool TryReadbackInto(Image<TPixel> destination, [NotNullWhen(false)] out string? error) |
There was a problem hiding this comment.
It is very unusual to pass around error information like this in .NET. There should be either error codes or exceptions.
| return; | ||
| } | ||
|
|
||
| WebGPUTextureTransfer.Release(this.TextureHandle, this.TextureViewHandle); |
There was a problem hiding this comment.
This dispose is problematic, since it would leak native handles when missed. The old-school practice to deal with is to implement the dispose pattern, however, this is actively discouraged today -- the preferred way is to wrap handles in a SafeHandle since they allow to increase refcounts before P/Invoke, so killing a handle during an outstanding P/Invoke would not lead to a a crash.
The fact that this is likely an LLM-made mistake is very alarming to me. It's not something we would normally miss.
Prerequisites
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
IImageProcessingContextextension methods likeDrawLine(),DrawPolygon(),FillPolygon(),DrawBeziers(),DrawImage(),DrawText(), etc. — has been removed entirely. These methods were individually simple but suffered from several architectural limitations:The new model:
DrawingCanvasAll 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)Standalone usage (without
Image.Mutate)DrawingCanvas<TPixel>can be constructed directly against an image frame:Canvas state management
The canvas supports a save/restore stack (similar to HTML Canvas or SkCanvas):
State includes
DrawingOptions(graphics options, shape options, transform) and clip paths.SaveLayercreates an offscreen layer that composites back onRestore.IDrawingBackend— bring your own rendererThe library's rasterization and composition pipeline is abstracted behind
IDrawingBackend. This interface has the following methods:FlushCompositions<TPixel>TryReadRegion<TPixel>Process()andDrawImage()).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:Migration guide
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))ProcessWithCanvasblock — commands are batched and flushed togetherOther breaking changes in this PR
AntialiasSubpixelDepthremoved — The rasterizer now uses a fixed 256-step (8-bit) subpixel depth. The oldAntialiasSubpixelDepthproperty (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 controlsRasterizationMode(antialiased vs aliased). Whenfalse, coverage is snapped to binary usingAntialiasThreshold.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.
DrawPolygonAll - Renders a 7200x4800px path of the state of Mississippi with a 2px stroke.
FillParis - Renders 1096x1060px scene containing 50K fill paths.