Find the bad pixels, and rings that say where they really are - #33
Merged
Merged
Conversation
The defect scan reports a Clark-Evans index beside the count, and the panel turns it into a sentence: below 0.8 it reads "clustered -- these are following the picture", which is the scan telling an operator that what it found is the furniture rather than the sensor. On any frame that overflowed the store that sentence was wrong, and wrong in the direction that costs most. R is a mean nearest-neighbour distance over what a Poisson process of the same density would give, so the area underneath has to be the area that was observed. Two things make that smaller than the frame. The walk needs two pixels of margin to have four same-colour neighbours, so nothing is ever found outside [2, w-3] x [2, h-3]. And when the store fills it fills in raster order, so what is kept is every defect down to the row it stopped on and none below -- a complete census of the top of the frame rather than a sample of all of it. Dividing that by the whole frame understates the density, which inflates the expected distance and deflates R by roughly sqrt(h / y_reached). Measured on a synthetic 2592x1944 frame carrying 5115 uniformly scattered defects, of which 4096 were stored: R read 0.91 -- "following the picture", about a set that is uniform by construction. Over the window it reached, the same run reads 1.015. Sampling the stored set instead of taking a prefix was tried first and is wrong for a reason that is not obvious from here: the compare panel tallies sites across captures by exact coordinate, so the stored list has to be stable between scans. Any rank-based sample shifts phase the moment one extra defect turns up early, and two scans of one sensor would come back nearly disjoint. Raising the cap is no better -- clark_evans is O(n^2) and already runs 16.8 million distance evaluations at 4096 points. The prefix was never the problem. The area was. Reported now as well: the median of each plane, which is what says whether the lens was covered. Nothing else in the reading does -- the black floor is the 0.1st percentile and sits near black in any frame with a shadow in it, and the clipped fraction is about the other end. It comes off the value histogram the scan already builds, so it is one more walk of 16384 bins rather than of the frame: 86.1 ms without it against 85.3 ms with, both engines in one process reading the same 2592x1944 capture twelve times each, which is to say the difference is under the noise. Timed across two containers instead it appeared to cost 4 ms, and that was the containers. What it reads, measured on the lab gk7205v300 + imx335: a capture inside a black box sits 0.000 to 0.001 of the way from black to saturation across exposures from 0.5 s to 7 s, and this repository's own lit fixture sits at 0.105. Watched failing with the change reverted, which also needed the last_y assignment taken out -- left in, it is set and never read, and -Werror stops the build before any test can run. With the area back to w*h the capped reading goes to R = 0.420 against the same frame's 1.016 uncapped, and two of the three new checks go red.
A camera frame carrying 4054 defects had every one of its rings inside the top sixth of the picture, in a band with a ragged edge along the bottom of it, and it was read -- reasonably -- as the sensor's bad pixels being clustered at the top. They are not. The same scan's own spread index, a couple of inches to the right of that picture, said 1.01: scattered, as sensor defects are. The two halves of one panel said opposite things, and the picture is the half a person believes. The overlay caps what it draws at 600, which is worth doing -- a sensor with ten thousand bad pixels would otherwise spend a second building circles nobody can tell apart. But it took the first 600 of a list the engine fills in raster order, so the cap and that order between them made a spatial claim that nothing had measured. 600 of 4054 is 14.8% of the rows; measured off the picture, the rings stopped at 15.0% of the frame height. Every k-th now, k = ceil(n / 600). The same cost, deterministic so it survives the redraws that a resize or a tab switch bring, and it samples the whole frame. On the reproduction -- 2592x1944, 5115 scattered defects -- the drawn set goes from 600/0/0/0/0/0/0/0 across eight bands to 88/98/90/95/85/94/36/0, which tracks the full set's own 612/684/630/666/594/660/250/0. On a live capture from the lab gk7205v300 + imx335: 571 defects, R = 1.02, rings 68/71/61/72/68/65/70/96. The panel was quoting the wrong number too, and only sometimes. "of which the first N are marked" named the stored list, up to 4096, while the overlay was drawing 600 -- and it appeared only when the store had overflowed, so the common case, a few thousand defects ringed six hundred at a time, said nothing at all. The ring count now comes from the same helper the overlay draws with, so the two cannot drift, and a scan that filled its store says so and says what that costs the arrangement reported under it. The first version of the guard was worthless and passed with the bug put back. It took the midpoint of the drawn marks and counted how many fell below it -- but a prefix is evenly spread inside its own band, so about half of them always do. It is measured against the picture now: with 1342 defects drawn 600 at a time, a prefix reaches 44% of the way down and stops, and a sample reaches the bottom. Watched going red with the stride reverted.
Everything needed to tell a sensor's own bad pixels from detail in a picture was already in this tab -- the brightness gate, the tally across captures, the arrangement index -- and every bit of it was behind knowing to press Scan, knowing to choose Darkest, knowing to press Keep five times, and knowing what "4 of 5" buys. This is a way in. It drives those and owns no judgement of its own: five captures, each scanned and kept, then the rule the compare panel would have suggested, applied through the same path the Mark button uses. The first capture also decides what kind of run this is, and it decides it from the frame rather than from what it was told. A covered lens and an operator who believes the lens is covered are different things, and only one of them shows up in the numbers. Measured on the lab gk7205v300 + imx335, a capture inside a black box sits 0.000 to 0.001 of the way from black to saturation across exposures from 0.5 s to 7 s, while this repository's lit fixture sits at 0.105; the cut is at 0.01, an order of magnitude clear of both, so nothing turns on where exactly it goes. A dark frame is then left alone -- the camera should not be moved, because there is nothing for it to move relative to. A frame with a picture in it switches the gate to Darkest and asks for the camera to be moved between captures, which is what stops scene detail agreeing with itself. Telling those two apart turned up a second thing worth having. The gate is for separating a sensor from a scene, and on a frame that is dark all over it does the opposite. Its cut is a percentile of the local background, so on such a frame it lands just under the level nearly every pixel sits at -- and a hot pixel lifts its own 7x7 background a couple of counts above that, which is enough to be gated out. It therefore discards the defects in preference to everything else, on exactly the frames where the defects are all there is. On this camera's 7 s dark frame, 3921 sites arranged at R = 1.00 become 104 at R = 0.68, which the panel reports as "these are following the picture" about a frame with no picture in it; on a live capture, 571 become 6. A run never narrows a dark frame, and the gate now says so when the frame on screen is one. The camera's own burst is deliberately not used. /image.dng?frames=N averages consecutive frames in the sensor, and an average is the one thing that cannot answer this question: a pixel that was wrong in every frame and a pixel that was wrong in one of sixteen come out of it looking alike. The same reasoning is written out where the Plates tab chooses between its two paths. Without a capture provider the run still works and asks for each frame to be dropped in, the way Plates and Calibrate already degrade. It refuses to read the same frame twice inside one run: a frame read twice agrees with itself perfectly, which is the one answer the tally must never be allowed to give. And it ends in something that leaves the page -- the confirmed sites as plain text, one x,y to a line, under a header naming the sensor, the exposure, the gain and the rule that produced them. Plain text because nothing on the camera consumes a defect map yet, and a list of coordinates is the shape every other tool can read. The header is not decoration: dark current builds up with time, so a map taken at seven seconds describes a sensor that is not the one running at a thirtieth. On this camera, in a black box, the same sensor reported 882 sites at 0.5 s, 1329 at 1 s, 1782 at 2 s and 3921 at 7 s. The fixtures cannot exercise the gate honestly, and the check says so rather than pretending. They are a flat field with spikes on it, so on the lit arm the gate has no texture to discriminate against and takes the planted sites out along with everything else -- 52 per capture become 0. That is the gate doing what it does on a frame no camera produces, so what is pinned is that the run still ends somewhere rather than offering an empty file.
PR Summary by QodoCorrect defect mapping and add guided bad-pixel discovery
AI Description
Diagram
High-Level Assessment
Files changed (10)
|
Code Review by Qodo
1.
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Comment on lines
+960
to
+962
| const step = markStep(diag.defects.length); | ||
| for (let i = 0; i < diag.defects.length; i += step) { | ||
| const d = diag.defects[i]; |
There was a problem hiding this comment.
6. Large scans still draw rings at the top 🐞 Bug ≡ Correctness
drawMarks() strides through diag.defects, but a truncated diagnosis exposes only the capped raster-order prefix ending near the top of the frame. When the defect count exceeds the storage cap, the rings remain spatially biased even though the new caption says they are spread through the frame, and the loop cannot reach rows represented only by the separate total count.
Agent Prompt
## Issue description
The stride samples only the stored defect array. On truncated scans, every available coordinate belongs to the first raster-order prefix near the top of the frame, so the resulting rings are not a spatially representative frame-wide sample.
## Fix Focus Areas
- src/editor.js[938-963]
- src/editor.js[1851-1870]
- src/engine.c[1815-1824]
## Recommended Fix
Keep the stable raster-order defect list used for cross-capture comparison, but do not present its truncated prefix as a frame-wide sample. Either produce a separate bounded, spatially distributed overlay sample during diagnosis and have `drawMarks()` use it when `diag.truncated` is true, or suppress defect rings for truncated diagnoses and replace the frame-wide caption with an explicit message that no representative overlay is available because only the scan's top prefix was retained. Continue using the stride overlay for complete, untruncated lists.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
…m whole Review of the three changes below this one found two claims the code could not support, two more it could not quite support, and three numbers that should not have been written down. A kept scan was identified by the frame's display name. Nothing in the mount contract requires a host to vary that -- the camera's own WebUI happens to timestamp each capture, but a host handing back one filename would have had five captures collapse into a single kept scan, and a guided run that ended having confirmed nothing at all. Identity is the frame that was opened now. The case the old key existed for, reading one frame twice, is itself an open, so the open is the thing to count. Deduplicating on the READING instead was tried first, on the grounds that two captures of a real sensor never give byte-identical defect lists. They do not. But two other things do, and the suite caught both: a fixture built to repeat one view exactly, which is how the "these are following the picture" verdict is tested, and any pair of captures that find nothing at all, whose empty lists are equal. Each collapsed to a single capture and took the tally with it, and three checks went red -- one of them a check that predates this work. A run whose captures overflowed the engine's store was presenting a partial answer as a complete one. A capture that fills the store has looked at the top of its frame and nothing below, so candidates under the row it stopped on were never candidates for the tally either, and the list is short by an unknown number in a known place. finishHunt cleared the truncated flag while doing it. The flag is carried through the kept scans now, and the run says what it could not see, in the panel and in the exported file. It still finishes rather than refusing: the sites it confirmed are confirmed, and a list with a caveat is worth more than no list. 1024x1024 with 20000 asked for yields about 4800 found against 4096 stored, which is what the new check runs on; 512x512 cannot be made to overflow at all, because the planted spikes start landing on each other and the count saturates at 3538 however many are asked for. The caption made the same overstatement in miniature. The rings sample the stored list evenly, but that list ends at the row the scan filled up on, so on a truncated frame the bottom of the picture carries none of them and "spread through the frame" was the wrong phrase. It says which part of the frame it reached instead. And the area handed to the spatial index counted the last stored row whole, although the store fills partway along a row and the rest of it was never examined. Counting area nobody looked at reads as a set more clustered than it is -- the same mistake the area fix below was written to undo. The column is recorded alongside the row and the area is now exact: whole rows above, plus the part of the last one actually walked. clark_evans takes that area rather than two sides, because it was never really about a rectangle. The effect is small, as expected: the capped fixture reads 1.087 either way. The three numbers: - The comment introducing the run said an operator had to know what "3 of 5" buys. Both implementations compute four of five. - The overlay's cap was justified by ten thousand rings costing a second to build. Measured in headless Chromium on this desktop x86, median of seven: 89.9 ms for ten thousand, 47.0 ms for 4096, 6.2 ms for 600. So the cap is right and the reason was not -- past a few hundred the rings merge into a wash that hides the picture, which is what it says now. - The dark-frame advice told operators that dark current roughly doubles every 6-8 degrees. Nothing here measured that. The advice keeps the part that is supportable -- take the map where the camera lives -- and drops the number. Both new guards were watched failing with their fix reverted.
"Reading a plate here" asks three questions about one licence plate: how many pixels across it is, and how much blur it survives before it stops reading. Those are questions about the picture. Diagnose is about the sensor's own health -- hot pixels, the black level the frame implies, what has clipped -- and the card has sat there since it was written, telling the reader to go to a different tab and pick something before it could say anything at all. The guided defect run went in above it and made the misfiling plain: the card landed between the steps of a sequence somebody is meant to follow and the result that sequence produces. Asked what it had to do with defect pixels, the honest answer was nothing. It moves to Plates, directly under the list the plate is chosen from, where "pick one of the plates above" is an instruction the reader can act on without leaving the panel. Choosing a different candidate now refreshes it in place -- it is refilled rather than rebuilt, so selecting a plate does not stack a second copy of the card underneath the first -- and clearing the selection to search again puts it back to its empty state. Nothing about what it measures changed. The checks that placed it were pointed at the tab it is on now, and one was added for the tab it is not on, because "the card exists somewhere in the inspector" would have passed either way.
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.
A camera frame carrying 4054 defects drew every one of its rings inside the top
sixth of the picture, and was read — reasonably — as the sensor's bad pixels
being clustered at the top. They are not. The same scan's own spread index, a
couple of inches to the right of that picture, said 1.01: scattered, as sensor
defects are. Two halves of one panel saying opposite things, and the picture is
the half a person believes.
Two independent prefix bugs, and behind them a feature with no way in.
The rings were a prefix, not a sample
drawMarkscaps what it draws at 600, which is worth doing. But it took thefirst 600 of a list the engine fills in raster order, so the cap and that
order between them made a spatial claim nothing had measured. 600 of 4054 is
14.8% of the rows; measured off the picture, the rings stopped at 15.0% of the
frame height. Every k-th now.
This also explains the rest of the report — "Darker half" and "Darkest" looked
random while "Everywhere" did not. Those gate candidates by local background,
so fewer than 600 survive, all of them are drawn, and nothing is truncated.
Clark-Evans divided by an area it had not observed
When the store overflows it keeps a census of the top of the frame, but the
index normalised by the whole frame — so R came out low by about
sqrt(h / y_reached). On a synthetic 2592×1944 frame carrying 5115 uniformlyscattered defects it read 0.91, which the panel renders as "clustered —
these are following the picture", about a set uniform by construction. Over
the window actually reached it reads 1.015.
Sampling the stored set was tried first and is wrong: the compare panel tallies
sites across captures by exact coordinate, so the stored list has to be stable
between scans. Raising the cap is no better —
clark_evansis O(n²) andalready runs 16.8M distance evaluations at 4096 points.
Find the bad pixels
Everything needed to tell a sensor from a scene was already in this tab, behind
knowing to press Scan, to choose Darkest, to press Keep five times, and what
"4 of 5" buys. This drives those and owns no judgement of its own: five
captures, each scanned and kept, then the rule the compare panel would have
suggested.
It grades the first capture from the frame rather than from what it was told —
a covered lens and an operator who believes the lens is covered are different
things. Measured on a lab gk7205v300 + imx335, a capture inside a black box
sits 0.000–0.001 of the way from black to saturation across 0.5 s to 7 s, and
this repository's lit fixture sits at 0.105. A dark frame is then left alone; a
frame with a picture in it gets the gate and is told to move the camera.
Which turned up a second thing. The brightness gate ruins a dark frame. Its
cut is a percentile of the local background, so on a frame dark all over it
lands just under where nearly every pixel sits — and a hot pixel lifts its own
7×7 background above it and is gated out. It removes the defects first, on
exactly the frames where the defects are all there is. On that camera's 7 s
dark frame, 3921 sites at R = 1.00 become 104 at R = 0.68; on a live
capture, 571 become 6. The run never narrows a dark frame, and the gate says
why.
It ends in a list that leaves the page: the confirmed sites as plain text under
a header naming the sensor, exposure, gain and rule. That header is not
decoration — dark current builds with time, so a map taken at 7 s is not a map
of a camera running at 1/30. On this camera, in a black box: 882 sites at
0.5 s, 1329 at 1 s, 1782 at 2 s, 3921 at 7 s.
Verified
tools/build.sh, thedist/diff,smoke.mjsandui-check.mjs, all green,plus a live capture from the lab camera: 571 defects, R = 1.02, rings spread
68/71/61/72/68/65/70/96 across eight bands where they had been 600/0/0/0/0/0/0/0.
Both guards were watched failing with their change reverted. The engine one
goes to R = 0.420 against 1.016. The first version of the ring guard was
worthless and passed with the bug put back — it measured the marks against
their own midpoint, and a prefix is evenly spread inside its own band. Measured
against the picture instead, it now reports the lowest ring at 44% and fails.