calibrate: refuse a fit that is not to a chart, and a chart off the frame - #43
Conversation
…rame Calibrate solved and offered "Apply to the camera" for any 24 samples, chart or not. On a camera's plain red wall with no chart in view, the detector reported 19 cells with two corners outside the 1920x1080 frame (at 2170,1986 and 634,1095 of a lattice that starts at 946,230). Every quad measured on that wall -- the detected one, the default middle third, a dragged one -- solved at 19 to 26 mean dE2000, with coefficients up to +-20 and rows summing to exactly 1, so the camera accepted it too. Applied to a gk7205v200, it turned the live picture into amplified noise; a user reported the same as "mosaic black and white tiled image" (OpenIPC/firmware#2234). - detectChart() returns null when any corner lies outside the frame. The cells past the edge have nothing under them to measure, and it is what the detector made of that wall. - solveFromPatches() refuses a mean dE2000 over 12. The real chart in tests/chart-on-wood.dng fits at 4.77 and a make-chart.mjs chart at 7.47 (drawn from 8-bit sRGB, scored against Lab); the wall and that frame's own wood fit at 19 to 26. The UI checks measured fixture.dng's middle third, which has no chart in it, and relied on getting a matrix back. They now measure a chart drawn under the default corners, carrying fixture.dng's ColorMatrix1 so the light is still named; and a new check measures fixture.dng and expects the refusal on the panel with nothing to apply. make-chart.mjs and the check server pass camera matrices through for that. Every new check was seen failing with the fix reverted. Refs #42
PR Summary by QodoRefuse off-frame charts and poor calibration fits
AI Description
Diagram
High-Level Assessment
Files changed (8)
|
Code Review by Qodo
1. Incomplete charts are still reported
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
| // solved from it wrecked the camera's picture (OpenIPC/raw-editor#42). | ||
| if (corners.some(([cx, cy]) => !(cx >= 0 && cy >= 0 && cx <= i.width && cy <= i.height))) | ||
| return null; | ||
| return { corners, cells: found }; |
There was a problem hiding this comment.
1. Incomplete charts are still reported 📎 Requirement gap ≡ Correctness
detectChart() returns { corners, cells: found } after checking corner bounds, without requiring
found to equal 24. The underlying detector accepts as few as 18 distinct cells, so an incomplete
detection with in-frame corners reaches the editor as a found chart, where it is offered for
measurement with only a warning.
Agent Prompt
## Issue description
The new bounds check still reports detections containing fewer than 24 distinct cells as charts.
## Fix Focus Areas
- src/engine.js[269-281]
- dist/engine.js[269-281]
## Recommended Fix
Return null unless the detector reports all 24 cells, in both the source and built copy. Add a test for an in-frame detection with fewer than 24 cells.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Re the review's "incomplete charts are still reported" point: I'm not taking this one. Requiring all 24 cells would reject the tree's own real-chart fixture. What keeps a false detection from reaching a camera is the fit gate added here. The patches measured under it have to fit the chart (mean ΔE2000 ≤ 12), and the chartless wall fitted at 21–25 wherever the corners landed, including the quad the detector reported. Corners off the frame are refused because those cells can't be measured at all, which a partial but in-frame chart can. |
1 participant
Refs #42. This fixes the two defects there. The third point, that the 30 s hold asks the user to confirm a picture they cannot see because the panel shows the RAW frame and not the live stream, is left open on the issue.
What was wrong
Calibrate solved and offered Apply to the camera for any 24 samples, chart or not. On a gk7205v200's plain, red-lit wall with no chart in view:
detectChart()reported 19 cells, with two corners outside the 1920×1080 frame.POST /api/v1/config, the matrix turned the live picture into amplified noise. That matches the field report in Gk7201v200 support firmware#2234: "mosaic black and white tiled image".The change
detectChart()returnsnullwhen any corner lies outside the frame.solveFromPatches()throws when the mean ΔE2000 is over 12. The editor already shows solver errors on the panel, so the user sees the refusal and gets no Apply button.tests/chart-on-wood.dng, the real chartmake-chart.mjschart at the default cornerschart-on-wood.dng's wood, three quadsA coefficient-size bound was tried as well and dropped. The ΔE limit caught every case on its own, so there was nothing to justify the second number. The camera-side bound belongs in majestic.
Tests
smoke.mjs: a chart hanging off the bottom edge (18 cells, corners extrapolated to y=521 on a 480 frame) is not reported. The wood beside the real chart is refused, and the real chart still calibrates.ui-check: the calibrate checks used to measurefixture.dng's middle third, which has no chart, and depended on getting a matrix back. They now measure a chart drawn under the default corners, carryingfixture.dng's ColorMatrix1 so the light is still named.make-chart.mjsand the check server pass the matrices through for that. A new check measuresfixture.dngand expects the refusal on the panel, with no Apply button.Every new check was seen failing with the fix reverted: 2 in smoke and 2 in ui-check. The full loop passes:
node --check,build.sh, thedist/diff,smoke.mjsandui-check.mjs.dist/engine.wasmis unchanged, sinceengine.cis not touched.