refactor(raster)!: Skia fully replaces ImageSharp #284
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/skia-takeover"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Closes the ImageSharp chapter in Kagura — the CI audit breaker. NovelAI.ImageGen #5 (merged, f917909) moved the vendored client to SkiaSharp; this PR follows with Kagura's own raster classes and drops the ImageSharp pins entirely. SixLabors is gone from every project in the org.
What changes
external/NovelAI.ImageGen→f917909— the Skia merge commit (tree byte-identical to the reviewed6a949ad, so the review verdict covers this content exactly).SkiaImageOps(new): shared codec-validated decode/resize/encode. Decodes go throughSKCodec, neverSKBitmap.Decode— the latter silently partial-decodes truncated streams (probed: a 60%-truncated PNG returns a full-size bitmap). Mitchell cubic resampler, matching the family (OpenRouter.Net.Imaging).RegionMaskRasterizer— same mask contract (additive white / subtractive black, draw order), paint viaSKPixmap.Erase/pixel spans.CgCutoutComposer— the ADR 0051 arithmetic unchanged: source-rect crop, feathered chamfer-distance merge weights, byte-identical outside the sent region.SkiaCropper(ex-ImageSharpCropper): same normalized-region → centered-square → ≤512px contract; DI re-registered.SixLabors.ImageSharpdeleted fromDirectory.Packages.props;SkiaSharp3.119.4 pinned (already shipped via the OpenRouter.Net.Imaging natives reference — no new binary in the app).InvalidConversionagainst an opaque target, mislabeled as corrupt. That was Jibril's PR #5 finding; mirrored here inSkiaImageOps, the rasterizer, the composer, and the cropper before it could bite a second repo.TestPngfixture (blank/shade/gradients/decode/dims);SkiaCropperTestsrenamed with the class.Verification
dotnet restore --force-evaluate: zero NU19xx — the audit errors that opened the issue are goneSixLaborsreference remains in any csproj/props in the solutionHistorical ADRs/story docs keep their ImageSharp mentions (they record decisions at decision time).
Summary
Summary
Coverage
Kagura.BlazorAdapter - 89.7%
Kagura.Domain - 94.8%
D04ADFED3A21D401C2764A1D17367E35BEB556CBB3B4B0B74__NonSlugChars_0
Kagura.Infrastructure - 97.7%
n
n
on
ng
C689CA5BBE6D1849B2CA79581BBB07D40636D85790C0E51A9__ArtifactTimestampRegex_2
C689CA5BBE6D1849B2CA79581BBB07D40636D85790C0E51A9__ProjectRoute_0
C689CA5BBE6D1849B2CA79581BBB07D40636D85790C0E51A9__TabQuery_1
Kagura.Kernel - 90%
Kagura.Server - 81.5%
Kagura.UseCases - 95.5%
🔮 fufu~ Jibril reviewed your code!
Oh? Oh! A whole library eviction — ImageSharp shown the door, Skia in every seat, the family resampler carried over faithfully. The codec-guarded decode in
SkiaImageOpsis a mirror of the submodule's reviewedImageScalerline for line, the ADR 0051 arithmetic survived the port with its seam-killer tests intact, and my probes prove the pixel work is well guarded… mostly~ ♡Verdict: ⛔ I can't let this pass~ ♡
⛔ These need fixing before I'm satisfied~
!= SKCodecResult.Successcheck so truncated input flows through as a partial bitmap — all 794 integration tests stayed green. Survived. Not one test in this repository feeds a truncated or corrupt image to the pipeline. The identical guards in the siblingNovelAI.ImageGensubmodule are tested (ATruncatedPngThrowsTheInvalidContentArmNotTheUnknownFormatArm, junk→UnknownFormat), so the pattern is testable — this mirror simply wasn't given the same protection. fufu~ you wouldn't leave the one thing this PR exists to guarantee untested, would you? ♡Fix: port the submodule's test shape —
TestPnggains aTruncated()helper (real PNG bytes cut to ~60%) and a junk-bytes helper; add tests asserting the rasterizer,CgCutoutComposer.Cut,MergeBack, and the cropper each refuse truncated input loudly.TestPng(blank/shade/gradients) as the shared fixture — and thenAssistantViewImageToolTests.cs:53,AssistantVisionFlowTests.cs:58,CgRetouchTests.cs:50,CgVisionScopingTests.cs:78,CharacterPreviewPersistenceTests.cs:21,ExpressionImageGenerationTests.cs:55,GenerationMetadataLogTests.cs:20,LocationPreviewPersistenceTests.cs:22, andNovelAiGatewayTests.cs:27each keep a private copy of the identical Skia erase→encode block. Nine clones of the helper that was supposed to end the cloning, added by the PR that says it ends it. That's not a port, that's the same smell re-planted~ ♪Fix:
TestPng.Blank(width, height)already is this block; the shaded variants fold intoTestPng.Shade(shade, width, height). Replace the ten private copies (keepRasterizeAt-only callers onTestPng.Blank).💡 Little ideas (non-blocking)~
Blend(SKColor from, SKColor WithW, float w): the parameter nameWithWis PascalCase (conventions wantwithW, matching thefeatherPx/renderAtSentstyle around it). Pure naming polish — the logic is exact.✅ What I liked~
SKCodecoverSKBitmap.Decodefor pipeline decodes — refusing truncated input at the boundary instead of partial-decoding garbage into the merge arithmetic. The right instinct, mirrored faithfully from the family~Opaquekilled 10 tests (composer, cropper, retouch flow). That's what guarding a claim looks like. Oh, this is wonderful~Cut/MergeBack/monotonic-ramp tests all still bite, andMergeBack_refuses_a_mask_at_the_wrong_sizekept its guard.RasterizeAt's whole-pixelSKPixmap.Erase+GetPixelSpan<SKColor>rewrite — same mask contract, no hidden canvas-state semantics, cleaner than the accessor dance it replaces.SixLabors.ImageSharpgone fromDirectory.Packages.props, no orphan references, docs correctly keep their historical ImageSharp mentions.Automated review by Jibril · 2026-10-13
CI: green for head
0315c31e· Mutation probes: 2 run, 1 survived (truncation guard), 1 killed (unpremul→Opaque) · Local suite: skipped (CI green); probed against 794 integration testsBoth blockers fixed, and one of them turned out subtler than either of us assumed — thank you for the push, fufu~
1. The unguarded guards. New
SkiaImageOpsDecodeRefusalTestspins every decode consumer:Cut,MergeBack(render arm and mask arm separately), and the cropper — truncated bodies and junk bytes both. Mutation probe re-run your way: deleting the!= Successguard turns exactly the four decode-path tests red (Failed: 4, Passed: 4— the junk/rasterizer/alpha pins stay green, as they guard other things). Suite: 2016/2016.One amendment to the prescription, with evidence:
RegionMaskRasterizer.Rasterize(byte[])cannot be pinned as refusing truncated input, because it never decodes source pixels — it reads the header for dimensions and synthesizes the mask from geometry (probed: a 60%-truncated gradient's header parsescodec=64x64whileGetPixels=IncompleteInput; same leniency you signed off on forGetImageDimensionsin NovelAI.ImageGen #5 — this is that same header-only read). Forcing a validating decode there would reject work that is provably correct. It's now pinned as deliberately header-only — truncated source in, correct mask out, junk header still refused — so the leniency is a documented contract instead of an accident.2. The nine clones. All replaced: the six private
Png/SpritePngbuilders are one-lineTestPng.Blank/Shadedelegates, the inline vision-flow block isnew MemoryStream(TestPng.Blank(64, 96)), and the two fixed 64×64 builders areTestPng.Blank(64, 64).TestPnggains theTruncated()/Junk()helpers (gradient-based, same mid-IDAT physics as the submodule's fixture).Non-blocking taken:
WithW→to.Pushed as
73ba5c8, verified at the ref.🔍 fufu~ Jibril reviewed your code!
Oh? Oh! A re-review where the author ran my own mutation probe against me — deleting the guard and showing me
Failed: 4, Passed: 4before I even asked. And an amendment with evidence instead of a shrug: the rasterizer never decodes source pixels, so pinning header-only leniency as contract rather than sneaking a validating decode in — that's exactly how a disagreement should be answered~ I re-verified both claims mechanically, of course. ♡Verdict: ⛔ I can't let this pass~ ♡
⛔ These need fixing before I'm satisfied~
.csfile, both refs). This push replaced nine — andSourcePnghere is the tenth, byte-identical to the blockTestPng.Blankwas created to replace, sitting in a file that already callsTestPng.Decode/TestPng.Lumthree lines below it. The commit message says "the fixture clones die" — 9/10 did~ fufu~ you wouldn't leave the one clone whose file already imports the fixture, would you? ♡Fix:
private static byte[] SourcePng(int width, int height) => TestPng.Blank(width, height);(or fold it away entirely at the call sites).💡 Little ideas (non-blocking)~
A_valid_alpha_carrying_png_decodesnever decodes. It callsSkiaImageOps.GetImageDimensions, which is header-only — the same leniency the first test in this file pins as not a decode. The name (and the comment) promise more than the assert exercises. Since the internals are visible to this project,SkiaImageOps.Decode(AlphaPng())(dispose it, maybe assert one pixel's alpha survives) would make the test keep its word — the unpremul pixel path is currently only pinned indirectly through the composer/cropper suites.✅ What I liked~
!= SKCodecResult.Successguard turns exactly the four decode-path refusal tests red (Failed: 4, Passed: 4) — the surviving mutant from my last review is dead, precisely as you claimed. That's what closing a blocker looks like~Rasterize(byte[], IReadOnlyList<CgMaskPrimitive>)to use the validatingDecodeinstead of header-only dimensions —The_rasterizer_is_dimension_only…went red. The header-only contract is deliberate, documented, and guarded against "helpful" future tightening. I accept the amendment completely — same contract asGetImageDimensionsin NovelAI.ImageGen #5.TestPng.Truncated()/Junk()with the mid-IDAT gradient body — real bytes, real physics, same shape as the submodule fixture. The rasterizer junk-refusal and the not-over-rejecting alpha pin round the suite out nicely.Shade(shade, width, height)argument-order flip inCgRetouchTests/CgVisionScopingTests, which I checked against the signature. Correct. ♪WithW→totaken without fuss.One clone to go, and this is wonderful~
Automated review by Jibril · 2026-10-13
CI: green for head
73ba5c8· Mutation probes: 2 run, 0 survived (guard deletion → 4 refusal tests red; rasterizer forced validating decode → leniency pin red; unpremul probe skipped — unchanged since last review, killed there) · Local suite: skipped (CI green); probes against SkiaImageOpsDecodeRefusalTests onlyBoth taken — and this round was a clean catch on your side, fufu~
1. The tenth clone. Dead:
SourcePngis now=> TestPng.Blank(width, height). You were right that my sweep under-counted — I re-swept the whole test tree forEncode(SKEncodedImageFormat.Pngafter the fix: zero occurrences outsideTestPngitself. Ten of ten now.2. The alpha pin keeps its word.
A_valid_alpha_carrying_png_decodesnow callsSkiaImageOps.Decode(AlphaPng())— a real pixel decode — and asserts the alpha channel survives (GetPixel(24,24).Alpha == 128). The unpremul path is pinned directly, no longer only indirectly through the composer/cropper suites.Targeted suites 16/16, full suite 2016/2016. Pushed as
61601b3.🔮 fufu~ Jibril reviewed your code!
A third round, and oh — this is the one I live for~ The last two reviews found a survivor and a promise that hadn't kept its word; this push closes both with nothing new hiding in the delta. Two files changed, both test-side, production code untouched since the head I probed to death last round. Clean~ ♡
Verdict: ✅ Looks good to me~
I verified both of your claims mechanically, of course. I always do~
SourcePngis now=> TestPng.Blank(width, height), and my grep forEncode(SKEncodedImageFormat.Pngacross the entire test tree finds exactly two hits:TestPng.Encode(the fixture itself) andAlphaPng()insideSkiaImageOpsDecodeRefusalTests— which is that file's own special RGBA gradient fixture, not a blank clone. Ten of ten, zero contradictions. The PR that promised to end the cloning has actually ended it~A_valid_alpha_carrying_png_decodesdoes a realSkiaImageOps.Decode(AlphaPng())and assertsGetPixel(24, 24).Alpha == 128(the fixture's alpha is constant 128, so the pixel math is sound). My probe: flipped theDecodetarget fromUnpremultoOpaque→ exactly that test went red (Failed: 1, Passed: 7) with theInvalidConversion-misreported-as-corrupt message — the failure mode this guard exists to prevent. The unpremul path is now pinned directly, not just through the composer/cropper suites. Killed~ ♡✅ What I liked~
73ba5c8..61601b3touches exactly the two files the last review pointed at, nothing else. No drive-by edits, no scope creep, no surprises for the re-reviewer. I notice these things~No blockers left. No suggestions left. The ImageSharp chapter closes with every guard pinned from both directions and not one clone standing. Merge it and let's never speak of SixLabors again~ fufu~
Automated review by Jibril · 2026-10-13
CI: green for head
61601b33· Mutation probes: 1 run, 0 survived (Decode target → Opaque: alpha pin red; guard-deletion and rasterizer-leniency probes skipped — production code unchanged since73ba5c8, both killed there) · Local suite: skipped (CI green); targeted 16/16 green at HEAD