[283375]

core-ocr behavior alignments:

• Load T1/T2 ROI windows from JSON layouts bundled under ocr/layouts/device/
  • DeviceRois gains high score, pure shiny, early/late and PFL label windows
• Route device OCR through DeviceOcrScenario, keeping the steps consistent
• Fix CropBlackEdges scans probing out-of-range pixels
  • A fully black image now fails fast, and a valid image keeps at least a two-column crop

Performance optimizations:

• Select hash lookup matches as a stable top-K in a single pass
• Mask all rating class and clear status candidates with a single BGR→HSV conversion per ROI
• Detect gray digits with native channel ops in DeviceRoisMaskerAutoT1

Other optimizations:

• Score lookup labels by the weighted average of their hash-type confidences, improving accuracy
• Empty OCR text no more crashes the parsing
• Fix OpenCV Mat usage violations and proper releases with .use {}

See also: https://github.com/ArcaeaOffline/core-ocr/tree/0.1.0

[283375]

/pi Review this PR

[github-actions]

Overall this is a solid, well-structured refactor: DeviceOcrScenario gives a single ownership story for the cropped Mat + extractor/masker, the layout JSON removes a lot of duplicated arithmetic, hsvVote/stableTopKBy are genuinely good perf wins with tests, and the assert → require change fixes a real correctness hole (asserts are no-ops on Android JVM, so CropBlackEdges.crop could previously produce a non-positive Rect).

I verified the layout mapping numerically against the old formulas and they match on 16:9 (both T1/T2), which is the important compatibility check. A few things need attention before merge, one of which looks like a real regression:

Blocking

1. getCropRect's new "keep at least two columns" guard silently truncates the crop. In the right/bottom loops, if (i <= left + 1) break / if (i <= top + 1) break stop scanning before checking pixels, so the loop can't distinguish "we hit the guard" from "we found a non-black pixel" — right/bottom just stop where the guard fires. Consider an image whose left two columns are the only non-black ones: the crop returns width 2 instead of width 10, i.e. a 5× data loss that then feeds ROI extraction for every T2/T1 field. The old code had no such guard and produced the correct full rect (relying on require to catch the degenerate case). If the intent is only "never return a degenerate rect", the guard should clamp after the scan, not terminate it.
2. DeviceRoisMaskerAutoT1.gray() semantics changed — old pfl() was dilate(2×2) then erode(1×1) (a net dilation), new gray() is erode(2×2) only, and pure/far/lost/maxRecall all route through it. That's a morphological change on the ROI masks that feed countNonZero-based decisions — fine if deliberate (the T2 detector thresholds were also retuned, pflHsvLower/Upper 245→248 and 30→40), but it isn't mentioned in the description and CI can't catch it. Worth a note in the PR body or a comment.
3. T1Detector/T2Detector now probe on the cropped image with layout-derived label rects, whereas the old code probed on the raw imgBgr with roiRect[0] - PFL_LABEL_WIDTH * factor. DeviceRoisAutoSelector.select() is called with imgCropped in DeviceOcrScenario.forImage, but T1Detector.confidence(imgBgr) builds DeviceRoisAutoT1(imgBgr.width(), imgBgr.height()) inside — so the rois are scaled to the cropped dimensions while the label rects in the JSON were authored on the full-frame design canvas. Please double-check the pure_label/far_label/lost_label JSON values against a real screenshot: they're the only fields whose old position was expressed relative to a ROI rect rather than absolutely, so a mismatch here makes T1/T2 detection fail closed (→ UNKNOWN → exception from forImage unless a fallback is passed, which DeviceOcrHelper doesn't do).

Should fix

4. DeviceOcrScenario.forImage defaults fallback to null and DeviceOcrHelper passes nothing, so any screenshot where all detectors score 0 now throws IllegalArgumentException out of OCR. The previous code fell back to T2 unconditionally. This is a UX regression for margin-of-error images (and it's a plain Exception from a helper, so it'll surface as a crash unless the caller catches it). Consider defaulting fallback to T2 in the app call site, or catching at the helper boundary.
5. DeviceOcr.ocrIntOrNull maps empty OCR to 0 for maxRecall, but the field's type is Int? and PLAYER_MAX_RECALL/null is the meaningful "not present" value; 0 will trip the < 0/bound validators differently and round-trip into PlayResult.maxRecall = 0. The doc comment also says "Returns null when the OCR result is empty" — correct — but then the ?: 0 at every call site contradicts the intent. maxRecall should probably stay null on empty.
6. DeviceRoisMasker.ratingClassVotes / clearStatusVotes document index order as "part of the persisted result contract", but the implementation only guarantees that by the ordering of the ratingClassRanges/clearStatusRanges lists in each masker. A reordering of either list silently changes persisted clearStatus values. Since the contract is stated so strongly, an explicit mapping (index constants or a test asserting hsvVote order == ArcaeaRatingClass.fromInt(i)) would be worth it. Also, both lists are byte-for-byte duplicated between the T1 and T2 maskers — hoisting them to the interface file next to hsvVote would remove the drift risk.
7. stableTopKBy uses top.indexOfFirst inside the loop, so it is O(n·k) with k=10 — fine, but for allHashes (potentially hundreds of thousands of rows) the asSequence().filter{} plus a 10-element ArrayList is good; just note that require(limit > 0) is the only guard, and callers passing limit larger than the population are fine.

Nits

8. DeviceLayoutTest.screenRectOnNarrowerScreenCentersVertically asserts 248.0 for y on 1024×768 — 768/720 and 1024/1280 both give factor 0.8, and 1024/768 = 4:3 ≥ 16:9 is false so it centers vertically; the value checks out, but the test name says "narrower" while the comment says "negative design x, so the mapped x clamps to 0" — two behaviors in one assertion. Splitting them would make failures readable.
9. t2.json contains raw -3.3333333333333712 and 646.6666666666666, which are float artifacts of the original pixel constants. Rounding to -10.0/3 or expressing the design canvas in the T2 native units (e.g. design [1920, 1080], where these become -5, 970, 100…) would be much easier to review against the old formulas.
10. CropBlackEdges.kt still has an unused import CropBlackEdges.Companion.cropOrOriginal removed in the diff but the KDoc references still resolve; fine. ScreenshotDetect switched .toInt() → roundToInt() — that changes the probe rect by up to 1px for every call, including the w/h used in the ratio denominator; harmless but it's an unrelated behavior change bundled in.
11. DeviceOcrScenario.close() releases imgCropped but the DeviceRoisExtractor holds submat views into it; any caller that keeps a DeviceOcrResult-adjacent Mat from the extractor will use freed memory. The KDoc says so, but OcrPerformanceBenchmark.extractRois clones correctly while nothing enforces this at the type level — opening the extractor publicly (val extractor) is the risk; consider a helper withRois { } on the scenario instead.

The new StableTopKTest property test against sortedBy().take(10) is exactly the right way to pin that optimization. Let me know if you'd like me to push a patch for the getCropRect guard (item 1) and the maxRecall null handling (item 5).

[github-actions]

I'll start by fetching the PR details and diff.Let me examine the actual files to verify contract consistency and check for issues the diff alone doesn't reveal.The working tree is at master (pre-PR). Let me check the PR branch and remaining referenced sources.Let me check the PR branch source to verify the new code (resources location, tests, masker semantics).Let me check the CI status and test configuration, and verify the test source set actually runs.Let me verify a few behaviors numerically — the crop-edge loop, layout mapping vs. old formulas, and the screenshot-detect rounding change.Let me check the remaining call sites (benchmark's use of CropBlackEdges, and other crop/cropOrOriginal consumers), plus the getMostConfidentItem existing tests.I have enough to review. Let me confirm the one suspicious behavioral detail — T1Detector gray masking change (dilate/erode) and the gray() reuse in the selector.I found a real bug in the crop loop. Let me verify the caller assumption about "detect on cropped image" and double-check forImage detection order (the diff comment says detection is on the cropped image, and the old code detected on the uncropped image — is that intended?).Now I have a complete picture. Writing the review.I reviewed PR #89 and posted a review with 5 inline comments.

Summary

Verdict: strong refactor, but one real regression to fix before merge.

What's good:

• DeviceOcrScenario consolidates the cropped-Mat ownership story (with a correct clone() when cropOrOriginal returns the caller's Mat, and a catch that releases on failure).
• Layout JSON removes a lot of duplicated arithmetic — I verified numerically that both T1 and T2 map to identical pixel rects on 16:9 (e.g. T1 pure → [967.5, 780, 114, 39] on 1920×1080, matching the old formula exactly), so the migration is behavior-preserving on the common case.
• hsvVote (single BGR→HSV for 4–5 ranges) and stableTopKBy (O(n·k) instead of sort-then-take) are genuine wins, and the property test against sortedBy().take(10) is exactly the right way to pin the optimization.
• assert → require in CropBlackEdges fixes a real hole: asserts are no-ops on Android, so crop() could previously return a non-positive Rect.

Blocking issues raised:

1. getCropRect's new "keep at least two columns" guard truncates instead of clamps. The guard runs before the pixel check, so it can't tell "guard fired" from "found a non-black pixel". A 10px-wide image whose only non-black columns are 0–1 now returns width 2 instead of 10 — a 5× data loss feeding every ROI.
2. DeviceRoisMaskerAutoT1.gray() changed morphology (net dilation → erode(2×2)), affecting all PFL masks, plus retuned T2 thresholds — not mentioned in the description and invisible to CI.
3. Detectors now probe the cropped image with layout-derived label rects, while DeviceRoisAutoT1/T2 are constructed with cropped dimensions — the *_label JSON fields are the only ones previously expressed relative to a ROI rect, so they need a real-screenshot check.

Also flagged: UNKNOWN now throws where T2 used to be the fallback (with no fallback passed from DeviceOcrHelper), maxRecall being forced to 0 on empty OCR despite its Int? type, and the vote-index contract only holding implicitly through list ordering.

---

View action run | Model: deepseek/deepseek-flash | Time: 2m 4s | Tokens: 520.8K | Cost: $0.03 | Pi SDK v1.5.2 | Action v2.30.2

[283375]

/pi "DeviceRoisMaskerAutoT1.gray() semantics changed" "T1Detector/T2Detector now probe on the cropped image" are intentional. "DeviceOcr.ocrIntOrNull maps empty OCR to 0 for maxRecall" is now reverted. Update your findings.

[github-actions]

I'll examine the PR and its current state to update the findings.Let me check the current state of the code for the reverted maxRecall change.The working tree is on master. Let me check the PR head itself.Now let me check the updated DeviceOcrScenario (lifecycle fix) and the downstream consumers of maxRecall = null.maxRecall is now nullable end-to-end — that's clean. Let me verify the other two points are genuinely intentional-looking and re-check the previously-flagged issues that may now be stale (the gray() morphology, detector probe rects), plus whether the crop guard and UNKNOWN fallback still stand.Confirmed the current head state. Here's the updated review.

Updated findings for PR #89

Reviewed at head dda13f0 (includes 830a9f5 + its revert dda13f0, and the 44f78e5 lifecycle fix).

Withdrawn — items now resolved or accepted as intentional

1. DeviceOcr.ocrIntOrNull → maxRecall = 0 — withdrawn. dda13f0 reverted the ?: 0, so the head reads:

maxRecall = ocrIntOrNull(extractor.maxRecall),   // nullable, no default
pure = ocrIntOrNull(extractor.pure) ?: 0,        // fields where 0 is sane

I verified the null is now honored end-to-end rather than being absorbed downstream: PlayResult.maxRecall and OcrHistory.maxRecall are both Int?, PlayResultWarning guards with ?: return null / ?: 0, AutoMerger uses mapNotNull, and AutoSelector counts a null as "unknown" (not as a value). So the nullable signal survives into validation, dedup and grouping exactly as intended. This is the right shape and no further change is needed.

2. DeviceRoisMaskerAutoT1.gray() morphology change — withdrawn as a blocker, per your note that it's intentional. Restating it as a note rather than an objection: gray() now pays one erode(2×2) instead of dilate(2×2) + erode(1×1), and it is the implementation behind pure/far/lost/maxRecall in T1 and the grayMasker::gray reference in T1Detector. Since the detector and the masker now share the identical function, the selector's far/lost confidence and the OCR's pixel counts can no longer drift apart — that coupling is a real improvement over the old duplicated maskFarLostLabel path. The T2 threshold bumps (pflHsvLower 245 → 248, spread 30 → 40) ride along with it. My only remaining ask is cosmetic: a one-line comment on gray() saying the erosion is deliberate and shared with the T1 detector, so the next person bisecting a confidence regression doesn't "restore" the dilate. The KDoc currently says the image is eroded 2×2 before the range check but doesn't say why.

3. T1/T2 detectors probing the cropped image — withdrawn as a blocker, intentional. The risk I was pointing at was narrower than I stated: the concern is not that detection runs on the cropped image, but that the *_label fields are the first layout entries expressed in a coordinate space the older code never used for labels. The old code derived each label rect as x - PFL_LABEL_WIDTH * factor from the PFL ROI rect, i.e. anchored to the digits, using PFL_LABEL_WIDTH = 85 (T1) / 180 (T2). The new code reads pure_label/far_label/lost_label straight from the JSON and submats them. Those numbers are authored independently, so they're worth one eyeball against a real screenshot per device type — but that's validation of new data, not a code defect, and it doesn't block the refactor. Note the T2 label widths in the JSON (55/40/54 design px) are nowhere near the old 180, which is consistent with the old value covering the label and part of the digit block, not just the label.

Still open

1. CropBlackEdges.getCropRect truncates instead of clamping — still stands, and is the one I'd fix before merge. The head still carries the if (i <= left + 1) break guard in the right/bottom loops. The guard is evaluated before the pixel is examined, so "guard fired" and "found a non-black column" take the same exit, and right freezes at left + 2. On a 10px-wide image whose only non-black columns are 0–1, the old code returned width 10 and the new code returns 2 — a 5× loss on the image that every ROI is carved from. i <= left + 1 also skips the i == left iteration, so even a fully-black-except-one-column image behaves differently from what the comment promises.

Applying the minimum after the scan keeps both the intent and the old behavior:

val croppedW = max(2, right - left).coerceAtMost(width - left)
return Rect(left, top, croppedW, bottom - top)

(assert → require is a genuine fix, but it's orthogonal — it makes the fully-black case loud instead of silently returning a non-positive Rect under Android's no-op asserts.)

2. UNKNOWN now throws where T2 used to be the fallback — still stands. DeviceOcrScenario.forImage still defaults fallback to null, require(roisType != UNKNOWN) still hard-fails, and neither production caller (DeviceOcrHelper, OcrPerformanceBenchmark.extractRois) passes a fallback. Both are non-nullable in practice since every detector returning 0.0 was previously a "use T2" signal, so a miss on an unusual screenshot is now an exception instead of a T2 attempt. The CreateRoisAndMasker UNKNOWN -> throw branch is unreachable from forImage as written, which is fine, but the default is the thing I'd flip — fallback: DeviceRoisAutoSelectorResult? = DeviceRoisAutoSelectorResult.T2 preserves the old semantics while leaving the throw available to explicit callers.

3. Vote-index contract is only implicit — still stands, low priority. The KDoc on clearStatusVotes now documents the 0–3 ordering against ArcaeaPlayResultClearStatus, which is an improvement, but it's still guaranteed only by the order of the clearStatusRanges/ratingClassRanges list literals. A reorder in either masker silently changes persisted clearStatus values. One test asserting clearStatusVotes(...) order matches the enum order would pin it.

Verdict

Net change from my previous review: two blockers and one behavioral objection drop out — maxRecall is handled correctly now that it's nullable through the whole pipeline, and the masker/detector changes are intentional and, in the gray() case, a genuine deduplication win. The crop guard is the only remaining code defect, and it's a two-line fix. The UNKNOWN-throws default is a smaller correctness call I'd still take.

---

View action run | Model: deepseek/deepseek-flash | Time: 1m 26s | Tokens: 238.3K | Cost: $0.01 | Pi SDK v1.5.2 | Action v2.30.2-develop (develop @ c266065)

[283375]

/pi Re-assess "getCropRect's new 'keep at least two columns' guard silently truncates the crop", and "DeviceOcrScenario.forImage defaults fallback to null" is also intentional

[github-actions]

Re-assessment of the two remaining findings

I re-checked both against the code and against core-ocr 0.1.0 (the reference in the PR description). Both of my earlier findings were wrong, and I'm withdrawing them. The head is in good shape — the only thing I'd still add is one test.

1. CropBlackEdges.getCropRect "keep at least two columns" guard — withdrawn, my analysis was wrong

My claim was: a 10px-wide image whose only non-black columns are 0–1 now returns width == 2 instead of 10. That input is impossible. The left loop advances left while a column is black, so if column 0 is non-black the loop breaks immediately with left == 0 — there is no way to reach the right loop with left >= 2 and columns 0–1 non-black. I built the example backwards from the guard rather than from the loop.

I enumerated the actual behavior instead — every black/white column pattern up to width 14, comparing the new code against the pre-PR master code:

• Patterns where the new width is narrower than the old width: 0.
• Every differing pattern falls into exactly two groups: (old_width ≤ 1 → new_width 2) and (old_width ≤ 0 → new_width 0).

The full set of differing (old_width, new_width) pairs is:

(-13,0) (-12,0) ... (-2,0) (1,2)

So the guard fires only where the old code produced a width of 1 or a negative width — exactly the degenerate cases right > left is there to reject. On any image with a crop of width ≥ 2 it is a no-op, and it can never truncate a usable crop. The require still catches the width-0 outcome, so the "fully black" path stays loud.

It also mirrors upstream exactly. The Python is:

if i <= left + 1 or not cls.is_black_edge(column, black_threshold):
    break

The Kotlin hoists the first disjunct into a separate if i <= left + 1) break — and I verified those two forms are equivalent over all patterns up to width 10 (0 mismatches), because both are pure short-circuit disjunctions. So the i == left iteration I said was "skipped" is skipped in upstream too; the port is faithful, not a drift.

The right/bottom guards are symmetric, so this covers all four loops.

One genuine issue in this area, and it's not in the PR: the master version this replaces loops for (i in 0..width) and calls submat(Rect(i, 0, 1, height)), which reads column width and throws IndexOutOfBounds on every image whose left edge is fully black — i.e. the crop path was largely broken before. The PR's 0 until width / width - 1 downTo 0 form fixes that, and assert → require fixes the silent no-op. Both are strict improvements.

2. DeviceOcrScenario.forImage defaulting fallback to null — withdrawn as intentional

Confirmed this matches upstream, which has the same default and raises on UNKNOWN rather than substituting T2:

def for_image(cls, img_bgr, crnn_provider, image_id_provider, *, fallback=None):
    ...
    if result is DeviceRoisAutoSelectorResult.UNKNOWN:
        raise ValueError("cannot determine device rois type; pass an explicit fallback")

My concern assumed "every detector returned 0.0" previously meant "use T2". It didn't — it meant UNKNOWN, and createRoisAndMasker has no UNKNOWN branch in either implementation, so the old Kotlin path would have thrown there anyway (IllegalArgumentException("No rois/masker pair for UNKNOWN"), the same message the PR keeps). Falling back to T2 on a 0.0-everywhere result would be guessing a layout from zero evidence, which is worse than failing loudly on a screenshot the app doesn't recognise. fallback stays available for callers that genuinely want that guess.

The KDoc is accurate as written: the throw is conditional on "fallback is null", and the require message tells the caller how to opt in. I'd only note that DeviceOcrHelper/OcrPerformanceBenchmark not passing a fallback is the intended posture, not an oversight — worth a one-line comment at the DeviceOcrHelper call site so nobody "fixes" it later by passing T2.

Suggestion: pin the crop loop with a test

Nothing currently covers getCropRect (the PR's only new tests are StableTopKTest and DeviceLayoutTest). The guard is load-bearing for the upstream-equivalence claim above and, as this exchange demonstrates, easy to misread. A table-driven test over a handful of synthetic Mats would lock it in:

• all-black image → throws IllegalArgumentException (via crop), cropOrOriginal returns the original;
• black borders on all four sides → rect equals the known inner box;
• leading black + a single non-black column → width == 2 (the clamp);
• one fully-black row at top and bottom → same for height;
• no black edges at all → full-size rect.

That last one also guards the master off-by-one that 0 until width fixed.

Net verdict

With these two withdrawn, every objection I raised across the three reviews is resolved or accepted as intentional. maxRecall nullability, the gray() deduplication, cropped-image detection, the crop guard, and the null fallback are all correct and all match upstream 0.1.0. No blocking changes from me. The crop test is the one thing I'd add, and the vote-index ordering note from the previous review still stands as a low-priority nicety.

[github-actions]

I'll start by fetching the PR thread and diff to understand the context.I'm on master. Let me check out the PR head and examine the two files in question.Now let me look at the upstream reference, the selector, and the callers to properly reassess both points.Both callers use the default fallback = null. Let me check the upstream reference for both behaviors, and the pre-PR (master) versions to compare against.Important: the master version loops 0..width inclusive and does submat(Rect(i, 0, ...)), which would go out of bounds at i == width. So master is actually buggy and the PR fixed it. Let me check the upstream Python implementation for both behaviors.Now I have the upstream source. The Kotlin code is a faithful port of upstream 0.1.0. Let me verify the DeviceRoisMaskerAuto.kt gray/erode to confirm the port fidelity, and check the callers' semantics on fallback.Now let me check the masker gray port precisely, then verify my earlier claim about the crop loop numerically.Both are faithful ports of upstream 0.1.0. Now let me verify my earlier crop-loop claim numerically rather than by inspection.My earlier numeric example was wrong. Let me characterize the actual divergence between old and new systematically, across all black/white column patterns.The result is decisive. The only patterns that differ are those where the crop would have width 1 or less — exactly the degenerate cases. My original review claim was wrong. Let me confirm against the actual Kotlin semantics and check whether a non-degenerate crop can ever be truncated.Conclusive. Let me verify the same for rows (identical symmetric logic) and confirm the guard mirrors upstream Python exactly.The Kotlin guard is exactly equivalent to the upstream Python condition. Let me check the tests that exist for this, and confirm the fallback semantics once more.Both original findings were wrong. Let me post the corrected review.I re-assessed both findings and withdrew both — my earlier analysis was wrong on the crop guard.

1. getCropRect "keep at least two columns" guard — not a defect

My stated example ("10px image, only columns 0–1 non-black → width 2 instead of 10") is an impossible input. The left loop advances left while a column is black, so if column 0 is non-black it breaks immediately with left == 0; you can never reach the right loop with left >= 2 and columns 0–1 non-black. I constructed the example from the guard instead of from the loop.

I enumerated every black/white column pattern up to width 14, comparing the PR against pre-PR master:

• Patterns where the new width is narrower than the old: 0
• Differing (old_width, new_width) pairs: only (-13,0) … (-2,0) and (1,2)

So the guard fires only where the old code produced width 1 or a negative width — precisely the degenerate cases right > left rejects. It never truncates a usable crop.

It also matches upstream exactly. Upstream Python is if i <= left + 1 or not is_black_edge(col): break; the Kotlin splits that disjunction into two ifs. I verified the two forms are equivalent over all patterns up to width 10 (0 mismatches) — both short-circuit identically, so the i == left iteration is skipped upstream too.

Separately: the master code this replaces loops for (i in 0..width) and subscripts column width, which throws on every image with a fully-black left edge. The PR's 0 until width fixes a real pre-existing bug.

2. forImage defaulting fallback to null — matches upstream, intentional

Upstream has the same fallback=None default and raises on UNKNOWN. My concern assumed "all detectors 0.0" used to mean "use T2"; it meant UNKNOWN, and createRoisAndMasker has no UNKNOWN branch in either implementation — the old Kotlin path would have thrown there anyway. Falling back to T2 on zero evidence would be guessing a layout; failing loudly is better.

One suggestion

getCropRect has no test coverage (the PR's new tests are only StableTopKTest and DeviceLayoutTest). Given how easy the guard is to misread — this exchange being the proof — a table-driven test would lock in the behavior: all-black → throws / cropOrOriginal falls back, black borders on all four sides, single non-black column → width 2, and no-black-edges → full size.

No blocking changes from me. Every objection across the three reviews is now resolved or accepted as intentional.

---

View action run | Model: deepseek/deepseek-flash | Time: 1m 39s | Tokens: 297.9K | Cost: $0.02 | Pi SDK v1.5.2 | Action v2.30.2-develop (develop @ c266065)