Skip to content

Implemented adaptive frame selection mechanism - #1014

Open
gabrieltmonkai wants to merge 5 commits into
mainfrom
feat/MN-876/adaptive-frame-rate
Open

gabrieltmonkai wants to merge 5 commits into
mainfrom
feat/MN-876/adaptive-frame-rate

Conversation

@gabrieltmonkai

@gabrieltmonkai gabrieltmonkai commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Overview

Jira Ticket Reference : MN-876

Implemented adaptive frame selection mechanism when moving around the car.

Checklist before requesting a review

  • I have updated the unit tests based on the changes I made
  • I have updated the docs (TSDoc / README / global doc) to reflect my changes
  • I have updated the local app configs if needed
  • I have performed self-QA of my feature by testing the apps and packages and made sure that :
    • No regression or new bug has occurred
    • The acceptance criteria listed in the ticket are met
    • Self-QA was made on both desktop and mobile

@gabrieltmonkai
gabrieltmonkai requested a review from dlymonkai July 28, 2026 09:45
}

const SCREENSHOT_INTERVAL_MS = 200;
const FRAME_SELECTION_INTERVAL_MS = 1000;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I remember correctly, we talked about adding a new prop for targetPictureCount, so frameSelectionInterval can adapt to it.

"recording": {
"discardDialog": {
"message": "Do you want to discard the video? You haven' t gone all the way around the vehicle.",
"messageMissingFrames": "Do you want to discard the video? Not all of the required photos have been captured yet.",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you swap "photos" for "footage" here (and other languages also) — I think we should use another word than "photos" since the user doesn't know we're not actually recording a video (they'd expect "video" and "photos" not to be mixed in the same sentence):

@gabrieltmonkai
gabrieltmonkai force-pushed the feat/MN-876/adaptive-frame-rate branch from 51302a6 to 5ce23af Compare September 3, 2026 09:07
dlymonkai

This comment was marked as spam.

dlymonkai

This comment was marked as spam.

dlymonkai

This comment was marked as spam.

dlymonkai

This comment was marked as spam.

@dlymonkai dlymonkai left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated code review — 3 confirmed correctness bugs and 3 lower-severity findings.

if (flushTrigger === undefined) {
return;
}
flushBestFrame();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Bug — CONFIRMED] Stale frame from discarded recording uploaded to the next one (adaptive mode only)

In adaptive mode useInterval is disabled (null delay), so bestFrame.current is never drained after a discard. When the user starts a new recording, startSegmentTracking() immediately increments flushTrigger, which triggers this flushBestFrame() call — uploading whatever frame was accumulated in the previous (discarded) session into the new recording.

Reproduction: record partway → click stop → click "Discard Video" → start a new recording → the first uploaded frame belongs to the old session.

Fix: reset bestFrame.current = null and bestScore.current = null inside onDiscardDialogDiscardVideo.

const currentBucket = Math.floor(walkaroundPosition / bucketSizeDegrees) % totalBuckets;
if (!capturedBuckets.has(currentBucket)) {
setCapturedBuckets((prev) => new Set(prev).add(currentBucket));
setFlushTrigger((value) => value + 1);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Bug — CONFIRMED] Last bucket's best frame is always silently dropped

Flushes fire on entering a bucket and upload the prior bucket's best frame. When capturedFramesCount reaches targetFramesCount, the END tooltip appears and the user clicks stop — but no subsequent flushTrigger increment ever occurs, so the best frame captured in the final bucket sits in bestFrame.current and is discarded when the component unmounts.

Result: the API consistently receives targetFramesCount − 1 real frames (e.g. 39 for a default config of 40). Fixed-rate mode is unaffected because its useInterval keeps flushing after setIsRecording(false).

Fix: trigger one final flush when isRecording transitions to false (e.g. watch isRecording going false in useFrameSelection, or expose a dedicated end-of-recording signal from this hook).


const startSegmentTracking = useCallback(() => {
setCapturedBuckets(new Set([0]));
setFlushTrigger((value) => value + 1);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Bug — CONFIRMED] Initial startSegmentTracking flush is always a no-op → every walkaround delivers targetFramesCount − 1 real uploads

startSegmentTracking increments flushTrigger at the moment recording begins, before the screenshot interval has fired even once. The resulting flushBestFrame() fires immediately, finds bestFrame.current === null, and exits without uploading anything — while bucket 0 is already marked as captured.

Combined with the missing final-bucket flush above, a targetFramesCount=40 config produces 39 real uploads, not the "+/−1" stated in the hook's docstring — it is consistently −1.

Fix: remove setFlushTrigger(v => v + 1) from startSegmentTracking. Bucket 0 will be captured naturally once the first screenshot is scored and the user moves to bucket 1.

@@ -80,8 +89,7 @@ export interface VideoCaptureHUDProps
showCloseVideoButton?: boolean;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Efficiency — PLAUSIBLE] Screenshot interval halved to 100 ms doubles synchronous Laplace load

SCREENSHOT_INTERVAL_MS was changed 200 ms → 100 ms, so onCaptureVideoFrame fires 10×/sec instead of 5×/sec. Each call copies raw ImageData and enqueues an O(width × height) Laplace convolution on the JS main thread.

At 1080p this is ~20 M pixel ops/sec competing with gyroscope events, React renders, and the upload queue. On mid-range Android this risks saturating the JS thread, causing camera-preview jank and degrading the sharpness of the very frames being scored — the opposite of the intended effect. Consider whether a lower rate (or a device-adaptive rate) is sufficient for the adaptive strategy.

const [flushTrigger, setFlushTrigger] = useState(0);
const [capturedBuckets, setCapturedBuckets] = useState<Set<number>>(new Set());

useEffect(() => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Simplification — PLAUSIBLE] Bucket-tracking logic duplicated from useVehicleWalkaround

The Math.floor(walkaroundPosition / bucketSizeDegrees) % totalBuckets + Set<number> pattern is identical to the one in useVehicleWalkaround. The two have already diverged in guard conditions (useVehicleWalkaround additionally guards on startingAlpha !== null), meaning a future edge-case fix must be applied independently in both.

Consider extracting a shared useBucketTracker(position, totalBuckets, isActive) primitive that both hooks consume.


[Efficiency — PLAUSIBLE] capturedBuckets in deps double-fires per bucket boundary

Including capturedBuckets in this effect's dependency array causes the body to run twice per new-bucket crossing: once when walkaroundPosition changes (guard passes → setCapturedBuckets + setFlushTrigger), and again after the state update commits (capturedBuckets.has() is now true → early exit). At 60 Hz gyroscope updates this doubles closure invocations at every angular boundary and adds an extra React reconciliation per capture. Consider using a useRef for the set to avoid the second run.

@gabrieltmonkai
gabrieltmonkai force-pushed the feat/MN-876/adaptive-frame-rate branch from 7074b02 to 17c3e56 Compare September 10, 2026 16:28
@gabrieltmonkai
gabrieltmonkai force-pushed the feat/MN-876/adaptive-frame-rate branch from 17c3e56 to 2459367 Compare September 16, 2026 15:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants