fix(scanner): take the luminance fast path for packed camera frames - #1303
Merged
Conversation
`getLuminancePlaneData()` guarded the Y-plane unpadding with `pixelStride != -1`. A YUV_420_888 Y plane always has a pixel stride of 1 (the format guarantees the Y plane is never interleaved), so that term was vacuously true and the `||` short-circuited the whole condition to always-true. The `return data` fast path was unreachable, and the O(width*height) per-pixel Kotlin copy ran on every analyzed frame even when the buffer was already tightly packed. The referenced upstream (Aegis fb58c877) uses `pixelStride != 1`; this was a `-1` vs `1` typo. Measured on a Pixel 10 emulator, per frame at the analysis resolutions the app requests: 640x480 233us -> 0.05us 1280x720 699us -> 0.05us 1920x1080 1470us -> 0.06us At the 1920x1080 target the analyzer requests, that is ~1.5ms of pure overhead per frame on the analysis thread, which is a plausible contributor to the reports of occasionally-slow scanning. The unpadding logic moves into `unpadLuminancePlane` so it can be covered without an `ImageProxy`; behaviour is otherwise unchanged, and the native scanner only ever reads the first `width * height` bytes (`kikCodeScan` memcpy's exactly that), so handing back the longer backing array on the fast path is safe. Adds a JVM test pinning fast-path/slow-path behaviour and asserting the output stays byte-identical to the pre-fix implementation, plus an instrumented sweep that renders real codes through the production encode -> geometry -> scan pipeline across resolutions, code scales and row strides (18/18 decode; padded and packed planes decode identically).
The Y-plane packing rule now lives in :libs:codes:kikcode commonMain, which SharedCore already exports, so iOS applies the same rule instead of its own. Both platforms had a bug here, in opposite directions. Android guarded the unpadding with `pixelStride != -1` -- vacuously true for a YUV_420_888 Y plane -- and ran the per-pixel copy on every frame. iOS never unpadded at all, and read its stride with the whole-buffer CVPixelBufferGetBytesPerRow rather than the plane-level API; it survives only because the 1080p capture width happens to be 64-aligned. LuminancePlane shares the decision, not the bytes: unpad() stays a JVM-side detail and iOS asks isTightlyPacked() then moves the plane in its own native code. Handing a frame across the Kotlin/Native bridge would convert Data to ByteArray and copy the whole ~2MB plane -- worse than the copy this avoids. The pure-logic tests move to commonTest so they run on iOS targets too (6/6 on both testAndroidHostTest and iosSimulatorArm64Test). Measured on Pixel_10 AVD, packed 1080p frames: the cross-module call costs 4.3ns/frame vs 1.9ns for a module-local copy, against 1565us/frame saved by taking the fast path at all. The instrumented sweep still decodes 18/18 and packed/padded planes decode identically.
github-actions
Bot
added
area: scanner
… a fixed budget The A/B assertion was `shared < local + 5.0`, a threshold picked on an emulator. It encodes the speed of whatever CPU it happens to run on rather than a property of the code, and it duly failed on an S25 Ultra (shared=12.5ns local=5.1ns) while passing on the emulator (4.3 / 1.9). The cross-module hop really does cost ~2.4x a module-local call in a debug build, consistently across both machines. But that is not what the test should guard. A fast path that stops being one moves from nanoseconds to milliseconds -- measured at 1:446,036 on device -- so the gate is now the ratio against the legacy copy timed on the same device, which is meaningful everywhere. The A/B delta is still logged as an observation. Worth recording: R8 closes the gap almost entirely. On a minified release build the same A/B measures 3.77ns shared vs 3.43ns local -- 0.34ns -- since the rule is a two-int comparison and a return. The debug figure is an artifact of the build type, not the cost of sharing.
The wall-clock benchmarks measure a frame in isolation, which says nothing about the shape of the original report — scanning that is occasionally slow rather than uniformly slow. Adds a 300-frame variant comparing the pre-fix path, the current fast path, and a hypothetical reusable frame buffer, so the remaining per-frame allocation can be sized before anyone builds it away.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
getLuminancePlaneData()guarded the unpadding copy with:pixelStridefor the Y plane of aYUV_420_888image is always 1 — the format guarantees it, and CameraX never reports -1 — sopixelStride != -1is vacuously true and the||short-circuits the whole condition to always-true. The result: an O(width × height) nested per-pixel Kotlin copy ran on every analyzed frame, including the common case where the plane is already tightly packed and the buffer could have been passed straight through.The upstream this was adapted from (the Aegis commit linked in the comment above the function) uses
pixelStride != 1. This was a-1vs1typo.Cost
Measured on a Galaxy S25 Ultra, minified release build, packed planes:
At 1080p that is ~16% of a 33 ms frame budget, on a current flagship. A Pixel 10 emulator reports only ~1.4 ms for the same case — it runs on the host CPU with a much larger cache, so it materially understates the bug. Slower phones should see more than the S25 does, not less.
The fast path also drops one ~2 MB
ByteArrayallocation per frame at 1080p, which at 30fps was ~60 MB/s of garbage.Sharing the rule with iOS
The second commit moves the packing rule into
:libs:codes:kikcodeasLuminancePlane, which:kmp:shared-corealready exports, so it reaches iOS throughSharedCoreKit.This matters because iOS had the same class of bug in the other direction:
CodeExtractorreadCVPixelBufferGetBytesPerRow(the buffer-level stride, ~1.5× the luma row on a planar buffer) rather thanCVPixelBufferGetBytesPerRowOfPlane(buffer, 0). Two independent implementations of "is this plane packed?" produced two independent bugs. Companion iOS PR: code-payments/code-ios-app#626.Only the decision is shared, not the bytes —
isTightlyPackedandscannedByteCounttake plain ints. A sharedunpadwould force aData↔ByteArraymarshalling copy of the whole plane on iOS, which is the exact cost the fast path exists to avoid.The cross-module call costs 0.34 ns/frame vs a module-local copy on a minified release build (3.77 ns vs 3.43 ns) — R8 inlines it, since the rule is a two-int comparison and a return. Unoptimized it is a consistent ~2.4× (7.4 ns delta on device, 2.5 ns on the emulator), but users run the optimized build. Every figure here reproduced to six significant figures across repeat runs.
Tests
LuminancePlaneTest(commonTest) — 6 tests running on both JVM and Kotlin/Native, including a byte-identity check against a reproduction of the pre-fix implementation across 6 geometries, so the fix is proven to change only the packed case.KikCodeScanTest(instrumented) — sweeps rendered codes at 3 resolutions × 3 scales × packed/padded: 18/18 decode, packed and padded planes decode identically, plus the benchmarks above. Verified on a Galaxy S25 Ultra in both debug and minified release.The fast-path benchmark asserts a ratio against the legacy copy timed on the same device (measured at 1:446,036) rather than an absolute nanosecond budget. An earlier absolute threshold encoded the speed of whatever CPU it last ran on and flip-flopped between the emulator and a real phone.
Found while investigating reports of occasionally-slow code scanning.