perf(screenshot): read the crop region instead of decoding the whole capture - #2504
perf(screenshot): read the crop region instead of decoding the whole capture#2504thymikee wants to merge 1 commit into
Conversation
|
Size Report
Startup median (7 runs, lower is better):
|
|
Two PNG validation regressions remain at 3ed12d5:
Coverage fails five eager-import budget checks because The 1,437-line diff also needs a split or an explicit exception to the 1,000-line budget. For the smaller-design review, the current reader still allocates the full inflated scanline buffer; it only stops reconstruction at the crop boundary. Clarify that scope and justify the added reader/writer surface against a smaller change. |
3ed12d5 to
d3a0d39
Compare
|
The earlier checksum, critical-chunk, full-image and eager-import findings are fixed at d3a0d39. One malformed-input case remains: The diff is now 1,457 gross lines, so the split or explicit budget exception is still needed. Update the PR's validation section for this head; it currently names the previous stack head. Current-head CI is still running. |
e6749fc to
4d849a3
Compare
|
Fixed at
Scope wording: the reader's module doc says plainly that a deflate stream cannot be cut short — both paths inflate the whole compressed image, and what stops at the box is reconstruction and allocation. Diff size: 1,466 gross lines is 570 production across seven modules (largest 250), 895 mirrored tests and fixtures, one docs line. I would like an explicit budget exception rather than a split, for the reason in the body: every split point leaves a layer that is dead code on One slip I found while recounting the diff: a squash dropped the |
|
Commit identity, since it is out of the description now: this layer is On the first-row filter finding, I pushed on it and I don't think the restriction is in the format, so I have not added the guard:
On the constants: If you have a source that restricts the first scanline, point me at it and I'll re-check, because then this is a real divergence and I want it fixed here rather than in the next PR. |
…capture `screenshot --crop-on` paid for a full PNG decode and an RGBA re-encode of the capture before keeping a frame. One worker job now turns the captured bytes into the cropped bytes: a region reader that reconstructs pixels only down to the box's last row and allocates only the box's pixels, and a truecolor writer that drops the alpha channel when the cropped pixels carry none. The reader claims the 8-bit non-interlaced truecolor layout that iOS simulator and Android emulator captures arrive in, and only for a file it can vouch for: the IHDR and every chunk checksum are verified, an unrecognised critical chunk name is a decline, and every row's filter byte is read whether or not the box reaches that row. Everything else — palette, grayscale, interlaced, 16-bit, a checksum that does not match — falls through to the general PNG reader, which keeps owning the canonical decode error and the previous RGBA output. A box covering the whole image reads through that general reader too, so an unchanged answer is only reported for a file that reader accepts. Cropped bytes verify pixel-for-pixel against ImageMagick's own crop across RGB, RGBA, grayscale, palette, 16-bit, interlaced, and translucent sources, on both iOS simulator and Android emulator captures.
4d849a3 to
ad5be9e
Compare
|
Follow-up on the remaining asks from this thread. Heads: this layer is Scope, stated where the code is. The reader's module doc and the benchmark README now say the allocation explicitly rather than gesturing at it: a deflate stream cannot be cut short, so the new path inflates the whole compressed capture into a buffer sized for every filtered row, and it is the pixel work that stops at the box. My earlier wording ("only the region's pixels are allocated") under-described the inflate buffer. Justifying the reader/writer surface against a smaller change. I measured the halves instead of arguing about them: same bytes, one 20%-height crop box, main thread, median of 7, swapping one half of the change at a time.
Each single-sided change buys one axis and gives the other back. The writer alone gets the artifact (93 kB to 30 kB) and is slower than today on the noisy capture (67.2ms to 85.5ms). The reader alone gets the time (55.2ms to 28.1ms) and leaves a 60 kB RGBA artifact where the writer's answer is 30 kB. Together: 4.1x faster and 3.1x smaller on the flat capture where That is the same argument for keeping them in one layer, which is also the budget question: 1,467 gross lines is 570 production across seven modules (largest 250), 895 mirrored tests and fixtures, one docs line. I am asking for an explicit exception rather than a split — every split point here puts a writer with no reader, or a reader with no writer, on Correction on my comment above: I posted that about a first-scanline filter finding that is not in this thread. The first-scanline question is worth its own answer on its own merits (Average and Paeth are legal on row 0; the format treats the prior scanline as zeroes there and both decoders read such files), but you did not raise it and I should not have answered a comment that does not exist. |
|
The below-crop filter fix is correct at ad5be9e. One parser mismatch remains in png-format.ts:100: readPngChunks returns at IEND without checking that it ends the buffer. A partial crop therefore accepts a PNG with trailing bytes that the previous decoder rejects. Decline that input and add a partial-crop regression to preserve the canonical decode error. Current-head checks pass and there are no conflicts. The reader/writer measurements address the smaller-design question; the over-budget diff still needs an explicit exception or split, and live Android crop validation remains unreported. |
1 participant
Summary
screenshot --crop-on <selector>decoded the whole capture to RGBA, copied the frame's rows, and re-encoded it. One PNG worker job now turns captured bytes into cropped bytes:The reader claims the 8-bit non-interlaced truecolor layout iOS simulator and Android emulator captures arrive in. Palette, grayscale, interlaced, 16-bit and structurally untrusted files fall through to the general PNG reader, which keeps owning the canonical decode error and the previous RGBA output.
agent-device screenshot card.png --crop-on 'label="Apple Account"'What the region reader does not claim. A deflate stream cannot be cut short, so the new path inflates the whole compressed capture into a buffer sized for every filtered row, exactly as the old path did; what stops at the box is the pixel work, not the read. And the reader only acts on a file it can vouch for: the IHDR and every chunk checksum are verified, an unrecognised critical chunk name is a decline, and every row's filter byte is read whether or not the crop reaches that row. A box covering the whole image is reported as "your file is already the answer" only after the general reader has decoded the file cleanly. A truncated capture, a bad checksum, an unknown critical chunk or an unreadable filter below the box therefore fails the same way it did before, on both paths.
Why the reader and the writer together, and not one of them. Measured on the same bytes with only one half of the change in place (main thread, median of 7, one 20%-height crop box; full table in a comment below):
Each half alone buys one thing. The writer alone buys the artifact (93 kB to 30 kB) and costs time on noisy captures; the reader alone buys the time (55.2ms to 28.1ms) and leaves an RGBA artifact bigger than the one the writer produces. Only together do they get 4.1x faster and 3.1x smaller on the flat capture where
--crop-onis actually used. The reader cannot avoid the writer, because an RGBA-only answer gives back part of what the region read won; the writer cannot avoid the reader, because re-encoding from a full RGBA bitmap is the cost the reader removes.18 files, 1,467 gross lines: 570 production across seven modules (largest 250 lines), 895 mirrored tests and fixtures, one docs line. Asking for an explicit exception to the 1,000-line budget here rather than a split: the 570 production lines are one reader, the writer it feeds, and the format module both of them read, and the 895 remaining lines are their mirrored tests, which by repo convention land with the source. A split would ship a PNG writer with no reader feeding it, or a reader with no writer, and every intermediate layer would be dead code on
main.Validation
Validated at this layer's head
ad5be9e8aa, and at the stack head3b121f7dd5whose only additions over it arescripts/png-crop-benchmarkand its gate registration.pnpm check:affected --run: 368 test files / 2,350 tests pass, including the eager-closure budget suite (590 checks), plus format, oxlint, typecheck, layering, di-seams,check:fallow --base origin/mainand command-doc coverage. Two gates fail and both fail identically atorigin/mainin this worktree:mutation-model, andproduction-exports(63 findings, 63 at the merge base, none inpng-*). Oneprovider-integrationscenario timed out under full-suite load and passes in isolation (2.4s of a 5s budget).pnpm build:android), so that path rests on the comparison above.screencapit is a wash on time and the crop can come out ~13% larger, because inflating and reconstructing that much entropy dominates and theNone-filter writer cannot beat the general writer's filter search there. The benchmark layer documents how to reproduce that.