From c7b14331e188fb1a39adb72ab0c882308555929c Mon Sep 17 00:00:00 2001 From: tannevaled Date: Tue, 11 Aug 2026 11:39:50 +0200 Subject: [PATCH] DrawImage: decide the clip once, not per pixel Moving the toolkit's widgets onto the primitive showed the fast paths running far less often than expected: they required no clip at all, and a clipped blit is what a wallpaper cropped to its bounds, or a page scrolled inside a viewport, actually is. Every one of those fell back to the per-pixel loop. The surface and the clip are both rectangles, and the top of the clip stack is already the intersection of every clip in force, so where a blit may write is ONE rectangle known before the first row. Computing it up front removes the per-pixel bounds and clip tests altogether and lets the row paths run under a clip. scaleRow gained an x0 so a trimmed row still samples the column it would have sampled untrimmed. BenchmarkDrawImageClipped 911278 ns/op BenchmarkPerPixelBlitClipped 2719332 ns/op 3.0x on a 1200x800 image clipped to a 1000x700 window. The equivalence tests changed with it. They used to force the slow path with a covering clip and compare the primitive against itself -- which proves nothing once the fast paths run under a clip. They now compare against `reference`, the nearest-neighbour PutPixel loop DrawImage replaced, over 16 cases: scaled up and down evenly and unevenly, off each edge, clipped in x, in y, in both, clipped away entirely, translated, translated AND clipped, and translucent bands over a non-black ground so a blend that wrongly behaves as a copy cannot hide in zeroes. 100% statement coverage. Co-Authored-By: Claude Opus 4.8 --- image.go | 80 +++++++++++++------------- image_test.go | 152 +++++++++++++++++++++++++++++++++++++------------- 2 files changed, 154 insertions(+), 78 deletions(-) diff --git a/image.go b/image.go index 6c5da37..2af1daa 100644 --- a/image.go +++ b/image.go @@ -44,60 +44,59 @@ func (p *PixelPainter) DrawImage(dst Rect, src []byte, srcW, srcH int) { } dst = shiftRect(p.off, dst) - // Nothing has to be decided per pixel when the row lies entirely on the - // surface, no clip is in force and the source row is fully opaque: what - // lands is the source, not a blend with what was there. Scanning the alphas - // to find that out costs a quarter of a copy and saves the blend on every - // pixel of the row. - plainRows := dst.X >= 0 && dst.X+dst.W <= p.Width && len(p.clip) == 0 + // The surface and the clip are both rectangles, and the top of the clip + // stack is already the intersection of every clip in force, so where the + // blit may write is ONE rectangle known before the first row. Deciding it + // here is what lets the row paths run under a clip: testing per pixel is + // what used to make a clipped blit fall back to the slow loop, and a + // clipped blit is what a wallpaper and a scrolled page actually are. + eff := intersect(dst, Rect{X: 0, Y: 0, W: p.Width, H: p.Height}) + if n := len(p.clip); n > 0 { + eff = intersect(eff, p.clip[n-1]) + } + if eff.W <= 0 || eff.H <= 0 { + return + } // An enlarged image draws several destination rows from ONE source row. // Building that row once and copying it to its repeats turns the cost from // the destination's height into the source's -- which is the whole point of - // enlarging. prevRow is the byte offset of the last row produced, prevSY the + // enlarging. prevRow is the byte offset of the last row written, prevSY the // source row it came from. prevRow, prevSY := -1, -1 - for dy := 0; dy < dst.H; dy++ { - y := dst.Y + dy - if y < 0 || y >= p.Height { - continue - } - sy := dy * srcH / dst.H + for y := eff.Y; y < eff.Y+eff.H; y++ { + sy := (y - dst.Y) * srcH / dst.H srcRow := sy * srcW * 4 dstRow := y * p.Width * 4 - lo, hi := dstRow+dst.X*4, dstRow+(dst.X+dst.W)*4 + lo, hi := dstRow+eff.X*4, dstRow+(eff.X+eff.W)*4 - // The buffer bound is checked here and not with the rest of the - // condition because a caller may hand over a Buf shorter than - // Width*Height*4, exactly as PutPixel tolerates; the fast path must not - // be the one place that panics on it. - if fast := plainRows && hi <= len(p.Buf) && rowOpaque(src[srcRow:srcRow+srcW*4]); fast { + // A caller may hand over a Buf shorter than Width*Height*4, exactly as + // PutPixel tolerates; a row that does not fit is skipped rather than + // fatal. + if hi > len(p.Buf) { + continue + } + + // A fully opaque row replaces what was underneath, so it can be written + // without consulting it. A row with any translucency cannot: the result + // depends on the ground, which differs from row to row. + if rowOpaque(src[srcRow : srcRow+srcW*4]) { switch { case sy == prevSY && prevRow >= 0: - copy(p.Buf[lo:hi], p.Buf[prevRow+dst.X*4:prevRow+(dst.X+dst.W)*4]) - case dst.W == srcW: + copy(p.Buf[lo:hi], p.Buf[prevRow+eff.X*4:prevRow+(eff.X+eff.W)*4]) + case dst.W == srcW && eff.X == dst.X && eff.W == dst.W: copy(p.Buf[lo:hi], src[srcRow:srcRow+srcW*4]) default: - scaleRow(p.Buf[lo:hi], src[srcRow:srcRow+srcW*4], srcW, dst.W) + scaleRow(p.Buf[lo:hi], src[srcRow:srcRow+srcW*4], srcW, dst.W, eff.X-dst.X) } prevRow, prevSY = dstRow, sy continue } - for dx := 0; dx < dst.W; dx++ { - x := dst.X + dx - if x < 0 || x >= p.Width { - continue - } - if !clipAllows(p.clip, x, y) { - continue - } - sOff := srcRow + (dx*srcW/dst.W)*4 + for x := eff.X; x < eff.X+eff.W; x++ { + sOff := srcRow + ((x-dst.X)*srcW/dst.W)*4 dOff := dstRow + x*4 - if dOff < 0 || dOff+3 >= len(p.Buf) { - continue - } if a := src[sOff+3]; a == 0xFF { copy(p.Buf[dOff:dOff+4], src[sOff:sOff+4]) } else if a != 0 { @@ -106,6 +105,7 @@ func (p *PixelPainter) DrawImage(dst Rect, src []byte, srcW, srcH int) { }) } } + prevRow, prevSY = -1, -1 } } @@ -139,11 +139,13 @@ func (p *CellPainter) DrawImage(dst Rect, src []byte, srcW, srcH int) { } // scaleRow writes one destination row by nearest-neighbour sampling of one -// source row. Both are RGBA, dst is dstW pixels wide and src is srcW. -func scaleRow(dst, src []byte, srcW, dstW int) { - for dx := 0; dx < dstW; dx++ { - o := (dx * srcW / dstW) * 4 - copy(dst[dx*4:dx*4+4], src[o:o+4]) +// source row. Both are RGBA. dstW is the width the image was scaled to, which +// may be wider than dst when a clip trimmed it, so x0 says which column of that +// scaled row dst begins at. +func scaleRow(dst, src []byte, srcW, dstW, x0 int) { + for i := 0; i < len(dst)/4; i++ { + o := ((x0 + i) * srcW / dstW) * 4 + copy(dst[i*4:i*4+4], src[o:o+4]) } } diff --git a/image_test.go b/image_test.go index d645f71..898b260 100644 --- a/image_test.go +++ b/image_test.go @@ -135,59 +135,99 @@ func TestCellPainterDrawImage(t *testing.T) { c2.DrawImage(Rect{X: 0, Y: 0, W: 0, H: 1}, srcImage(1, 1), 1, 1) // empty } -// The row-copy fast path: same width as the source, on the surface, unclipped -// and opaque. It must produce exactly what the per-pixel path produces — the -// speed is worthless if the picture differs. -func TestDrawImageRowCopyMatchesPerPixel(t *testing.T) { - src := srcImage(16, 4) - - fast := newPix(16, 4) - fast.DrawImage(Rect{X: 0, Y: 0, W: 16, H: 4}, src, 16, 4) - - // The same blit with a clip covering everything takes the per-pixel path. - slow := newPix(16, 4) - slow.PushClip(Rect{X: 0, Y: 0, W: 16, H: 4}) - slow.DrawImage(Rect{X: 0, Y: 0, W: 16, H: 4}, src, 16, 4) - slow.PopClip() - - for i := range fast.Buf { - if fast.Buf[i] != slow.Buf[i] { - t.Fatalf("byte %d differs: fast %d, per-pixel %d", i, fast.Buf[i], slow.Buf[i]) +// reference is the loop DrawImage replaced, written out. PutPixel applies the +// clip, the translation and the blend, so this honours everything DrawImage +// must honour -- which makes it the yardstick. Comparing the primitive against +// a clip-forced variant of ITSELF would only prove it agrees with itself, and +// stops proving even that once the fast paths learn to run under a clip. +func reference(p *PixelPainter, dst Rect, src []byte, srcW, srcH int) { + for dy := 0; dy < dst.H; dy++ { + sy := dy * srcH / dst.H + for dx := 0; dx < dst.W; dx++ { + o := (sy*srcW + dx*srcW/dst.W) * 4 + p.PutPixel(dst.X+dx, dst.Y+dy, RGBA{ + R: src[o], G: src[o+1], B: src[o+2], A: src[o+3], + }) } } } -// Enlarging draws several destination rows from one source row, and the fast -// path copies the row it already built instead of rebuilding it. Shrinking and -// non-integer ratios take the same route with different arithmetic. All of them -// must still equal what the per-pixel path produces. -func TestDrawImageScaledFastPathMatchesPerPixel(t *testing.T) { +// translucent copies an image and knocks a band of it down to partial alpha, so +// the blend path is exercised on rows that the opaque fast path would take. +func translucent(src []byte, w, h int) []byte { + out := append([]byte(nil), src...) + for y := h / 3; y < 2*h/3; y++ { + for x := 0; x < w; x++ { + out[(y*w+x)*4+3] = 128 + } + } + return out +} + +// Every route through DrawImage -- row copy, row repeat, scaled row, clipped +// span, per-pixel blend -- must land the same pixels as the loop it replaced. +func TestDrawImageMatchesTheLoopItReplaced(t *testing.T) { for _, tc := range []struct { name string srcW, srcH, dstW, dstH int + dstX, dstY int + clip *Rect + tx, ty int + alpha bool }{ - {"enlarged 3x", 5, 4, 15, 12}, - {"enlarged unevenly", 5, 4, 13, 9}, - {"shrunk", 12, 10, 5, 3}, - {"wider, shorter", 4, 9, 17, 3}, + {name: "1:1", srcW: 16, srcH: 6, dstW: 16, dstH: 6}, + {name: "1:1 offset", srcW: 8, srcH: 4, dstW: 8, dstH: 4, dstX: 3, dstY: 2}, + {name: "enlarged 3x", srcW: 5, srcH: 4, dstW: 15, dstH: 12}, + {name: "enlarged unevenly", srcW: 5, srcH: 4, dstW: 13, dstH: 9}, + {name: "shrunk", srcW: 12, srcH: 10, dstW: 5, dstH: 3}, + {name: "wider, shorter", srcW: 4, srcH: 9, dstW: 17, dstH: 3}, + {name: "off the left and top", srcW: 8, srcH: 6, dstW: 8, dstH: 6, dstX: -3, dstY: -2}, + {name: "off the right and bottom", srcW: 8, srcH: 6, dstW: 8, dstH: 6, dstX: 15, dstY: 16}, + {name: "clipped in x only", srcW: 8, srcH: 6, dstW: 16, dstH: 12, clip: &Rect{X: 4, Y: 0, W: 6, H: 20}}, + {name: "clipped in y only", srcW: 8, srcH: 6, dstW: 16, dstH: 12, clip: &Rect{X: 0, Y: 3, W: 20, H: 5}}, + {name: "clipped in both", srcW: 8, srcH: 6, dstW: 16, dstH: 12, clip: &Rect{X: 2, Y: 3, W: 7, H: 5}}, + {name: "clipped away entirely", srcW: 8, srcH: 6, dstW: 8, dstH: 6, clip: &Rect{X: 40, Y: 40, W: 2, H: 2}}, + {name: "translated", srcW: 6, srcH: 5, dstW: 12, dstH: 10, tx: 4, ty: 3}, + {name: "translated and clipped", srcW: 6, srcH: 5, dstW: 12, dstH: 10, tx: 4, ty: 3, clip: &Rect{X: 1, Y: 1, W: 6, H: 6}}, + {name: "translucent band", srcW: 8, srcH: 9, dstW: 8, dstH: 9, alpha: true}, + {name: "translucent band enlarged", srcW: 8, srcH: 9, dstW: 16, dstH: 18, alpha: true}, } { t.Run(tc.name, func(t *testing.T) { src := srcImage(tc.srcW, tc.srcH) - dst := Rect{X: 0, Y: 0, W: tc.dstW, H: tc.dstH} - - fast := newPix(tc.dstW, tc.dstH) - fast.DrawImage(dst, src, tc.srcW, tc.srcH) + if tc.alpha { + src = translucent(src, tc.srcW, tc.srcH) + } + dst := Rect{X: tc.dstX, Y: tc.dstY, W: tc.dstW, H: tc.dstH} + + run := func(f func(p *PixelPainter)) *PixelPainter { + p := newPix(20, 20) + // A non-black ground, so a blend that wrongly behaves as a copy + // shows up instead of hiding in zeroes. + p.FillRect(Rect{X: 0, Y: 0, W: 20, H: 20}, RGBA{R: 40, G: 80, B: 120, A: 255}) + if tc.tx != 0 || tc.ty != 0 { + p.PushTranslate(tc.tx, tc.ty) + } + if tc.clip != nil { + p.PushClip(*tc.clip) + } + f(p) + if tc.clip != nil { + p.PopClip() + } + if tc.tx != 0 || tc.ty != 0 { + p.PopTranslate() + } + return p + } - slow := newPix(tc.dstW, tc.dstH) - slow.PushClip(dst) - slow.DrawImage(dst, src, tc.srcW, tc.srcH) - slow.PopClip() + got := run(func(p *PixelPainter) { p.DrawImage(dst, src, tc.srcW, tc.srcH) }) + want := run(func(p *PixelPainter) { reference(p, dst, src, tc.srcW, tc.srcH) }) - for i := range fast.Buf { - if fast.Buf[i] != slow.Buf[i] { + for i := range got.Buf { + if got.Buf[i] != want.Buf[i] { px := i / 4 - t.Fatalf("pixel %d,%d byte %d differs: fast %d, per-pixel %d", - px%tc.dstW, px/tc.dstW, i%4, fast.Buf[i], slow.Buf[i]) + t.Fatalf("pixel %d,%d byte %d: DrawImage %d, the loop it replaced %d", + px%20, px/20, i%4, got.Buf[i], want.Buf[i]) } } }) @@ -279,6 +319,40 @@ func BenchmarkPerPixelBlitScaled(b *testing.B) { } } +// A clipped blit -- what a wallpaper cropped to its bounds, or a page scrolled +// inside a viewport, actually is. It used to fall through to the per-pixel loop +// because the clip was tested per pixel. +func BenchmarkDrawImageClipped(b *testing.B) { + p := newPix(1000, 700) + src := srcImage(500, 350) + dst := Rect{X: -100, Y: -50, W: 1200, H: 800} + b.ResetTimer() + for i := 0; i < b.N; i++ { + p.PushClip(Rect{X: 0, Y: 0, W: 1000, H: 700}) + p.DrawImage(dst, src, 500, 350) + p.PopClip() + } +} + +func BenchmarkPerPixelBlitClipped(b *testing.B) { + p := newPix(1000, 700) + src := srcImage(500, 350) + var q Painter = p + dst := Rect{X: -100, Y: -50, W: 1200, H: 800} + b.ResetTimer() + for i := 0; i < b.N; i++ { + p.PushClip(Rect{X: 0, Y: 0, W: 1000, H: 700}) + for dy := 0; dy < dst.H; dy++ { + sy := dy * 350 / dst.H + for dx := 0; dx < dst.W; dx++ { + o := (sy*500 + dx*500/dst.W) * 4 + q.PutPixel(dst.X+dx, dst.Y+dy, RGBA{R: src[o], G: src[o+1], B: src[o+2], A: src[o+3]}) + } + } + p.PopClip() + } +} + func BenchmarkPerPixelBlit(b *testing.B) { p := newPix(1000, 700) src := srcImage(1000, 700)