fix(scanner): read the luminance plane at its own row stride - #626
Merged
Conversation
CodeExtractor built the scanner's input from CVPixelBufferGetBytesPerRow, which reports the buffer-level stride, not plane 0's. On a planar 420YpCbCr8 buffer that is roughly 1.5x the luma row -- 2884 for a 1920-wide frame -- so the pointer handed to the scanner did not describe the bytes behind it. The scanner memcpy's height * width from that pointer, so every frame whose plane is padded (CoreVideo aligns plane 0 to 64 bytes: 1440 -> 1472, 1000 -> 1024) fed it skewed rows. Read the plane-level geometry instead (width/height/bytesPerRow of plane 0) and strip row padding when the plane is padded. When rowStride == width the plane is already what the scanner wants, so it is passed through with bytesNoCopy and no per-frame allocation. Two related fixes fall out of the same path: - The zero-copy Data used to escape the CVPixelBufferLockBaseAddress scope; the base address is only guaranteed while the buffer is locked. The scan now runs inside the lock via withLuminanceSample. - KikCodes.scan(data:width:height:) dropped the result of the quality overload it delegates to -- a missing return in Code.mm -- so the three-argument entry point always returned nil. The packing rule matches Android's, which lives in the shared :libs:codes:kikcode module (LuminancePlane). Both platforms feed the same C++ scanner -- the sources under CodeScanner/src are byte-identical to Android's vendor/kik/scanner/src/main/cpp -- so the pixel-buffer glue is the only place the two can disagree. CodeScanSweepTests is the iOS half of the harness that mirrors Android's KikCodeScanTest: it renders real codes, pushes them through real CVPixelBuffers at packed and padded widths, and asserts the packing rule agrees byte-for-byte with the Kotlin one over the same geometries.
…expects The harness drew black marks on white. The detector thresholds for *bright* blobs and fits an ellipse to the centre badge before it ever looks at the ring to decide whether the marks are inverted, so a dark badge is not a candidate at all and nothing was ever detected. Draw light marks and a light badge on a dark field, which is what the bill draws and what Android's harness renders.
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
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.
CodeExtractorbuilt the scanner's input fromCVPixelBufferGetBytesPerRow, which reports the buffer-level stride rather than plane 0's. On a planar420YpCbCr8buffer that is roughly 1.5x the luma row -- 2884 for a 1920-wide frame -- so the pointer handed to the scanner did not describe the bytes behind it.kikCodeScandoesmemcpy(greyscale.data, image, height * width): it reads exactlywidth * heightbytes from the front of whatever it is given and assumes they are a tightly packed greyscale image. CoreVideo aligns plane 0 to 64 bytes, so any width that is not a multiple of 64 arrives padded (1440 -> 1472, 1000 -> 1024), and those frames were being fed skewed rows.This reads the plane-level geometry instead --
CVPixelBufferGetWidthOfPlane/GetHeightOfPlane/GetBytesPerRowOfPlane(buffer, 0)-- and strips row padding when the plane is padded. WhenrowStride == widththe plane is already what the scanner wants, so it passes through withbytesNoCopyand no per-frame allocation.Two related fixes fall out of the same path:
Dataescaped itsCVPixelBufferLockBaseAddressscope. The base address is only guaranteed while the buffer is locked, so this was a latent use-after-unlock. The scan now runs inside the lock viawithLuminanceSample.KikCodes.scan(data:width:height:)dropped the result of the quality overload it delegates to -- a missingreturninCode.mm-- so the three-argument entry point always returned nil.Cross-platform
Android had the same class of bug in the other direction: it guarded its unpadding with
pixelStride != -1, which is vacuously true for aYUV_420_888Y plane, so it ran a per-pixel copy on every frame. Two independent implementations of "is this plane packed?" produced two independent bugs.The rule now lives once, in
:libs:codes:kikcodeasLuminancePlane, whichSharedCoreKitalready exports: code-payments/code-android-app#1303.That version is not published yet, so this PR still carries a Swift-side implementation.
CodeScanSweepTests.packingRuleMatchesSharedRulepins it to the Kotlin one byte-for-byte over the same geometries and the same fill until the pin moves to SharedCore 0.3.2, at which point the Swift copy is deleted and the test becomes a straight regression test.Tests
CodeScanSweepTestsis the iOS half of the harness mirroring Android'sKikCodeScanTest. Both platforms feed the same C++ scanner -- the eight sources underCodeScanner/srcare byte-identical to Android'svendor/kik/scanner/src/main/cpp-- so the pixel-buffer glue is the only place the two can disagree, and that glue is what these cover. It renders real codes, pushes them through realCVPixelBuffers at packed and padded widths, and checks that packed planes are genuinely not copied.