Version compare: single blended fps drives BOTH panes' paused-frame drift tolerance, re-thrashing the decoder on mixed-fps comparisons

[ravirajsinh45]

Component: apps/web/components/review/compare/ (version compare overlay, introduced in #169)

Repro

1. Have two ready versions of a video asset with different native frame rates — e.g. v1 encoded at 24fps, v2 at 60fps.
2. Open Compare (wipe or side-by-side) between the two versions.
3. Pause playback and step/scrub to a frame (or let auto-sync land on a paused frame) on the lower-fps side.
4. Watch the lower-fps <video> element: on some frames, currentTime will be nudged (re-seeked) repeatedly, every rAF tick, instead of settling — the same "decoder thrash" behavior the paused-tolerance mechanism was specifically added to eliminate.

Root cause

CompareOverlay computes a single blended fps for the whole comparison and hands it to the transport hook as one value:

// apps/web/components/review/compare/compare-overlay.tsx
const fps = mediaB.fps ?? mediaA.fps ?? null   // one value, prefers side B

const transport = useSyncedTransport({
  urlA: isVideo ? urlA : null,
  urlB: isVideo ? urlB : null,
  timingA,
  timingB,
  audibleSide: isVideo ? (audioSide === 'none' ? null : audioSide) : null,
  fps,                                          // <- single value passed for both sides
})

useSyncedTransport turns that single fps into one pausedTolRef and applies it to both panes when slaving them to the shared transport clock:

// apps/web/components/review/compare/use-synced-transport.ts
const pausedTolRef = React.useRef(frameStep(fps))
pausedTolRef.current = frameStep(fps)

const slaveBoth = React.useCallback((time: number, playing: boolean) => {
  const a = playerA.videoRef.current
  const b = playerB.videoRef.current
  if (a) applySideState(a, time, timingARef.current, playing, pausedTolRef.current)
  if (b) applySideState(b, time, timingBRef.current, playing, pausedTolRef.current)
}, [playerA.videoRef, playerB.videoRef])

applySideState's own doc comment explains why a paused-frame tolerance exists at all:

A sub-frame threshold (the old 1ms) re-issued the seek on every rAF forever — decoder thrash on the exact paused-frame-inspection workflow compare exists for. One frame is display-accurate...

That tolerance is supposed to be "~one frame" for the side being checked. But because only one blended fps (effectively side B's fps, falling back to A's) is threaded through, whichever side's real frame rate differs from the blended value gets the wrong tolerance. In the 24-vs-60fps example, fps resolves to 60 (~16.7ms/frame) and that ~16.7ms tolerance is applied to the 24fps side too, even though its real frame spacing is ~41.7ms. A paused seek on the 24fps side that snaps to the nearest decodable frame can easily land >16.7ms from expected, re-triggering video.currentTime = expected on every animation frame — the exact thrash loop the mechanism exists to prevent.

Suggested fix

Thread each side's own fps through independently instead of blending them into one value, and keep two tolerance refs (one per side):

interface UseSyncedTransportArgs {
  ...
  fpsA?: number | null
  fpsB?: number | null
}

export function useSyncedTransport({ ..., fpsA = null, fpsB = null }: UseSyncedTransportArgs) {
  ...
  const pausedTolARef = React.useRef(frameStep(fpsA))
  pausedTolARef.current = frameStep(fpsA)
  const pausedTolBRef = React.useRef(frameStep(fpsB))
  pausedTolBRef.current = frameStep(fpsB)

  const slaveBoth = React.useCallback((time: number, playing: boolean) => {
    const a = playerA.videoRef.current
    const b = playerB.videoRef.current
    if (a) applySideState(a, time, timingARef.current, playing, pausedTolARef.current)
    if (b) applySideState(b, time, timingBRef.current, playing, pausedTolBRef.current)
  }, [playerA.videoRef, playerB.videoRef])

and in CompareOverlay, pass each side's own fps:

const transport = useSyncedTransport({
  ...
  fpsA: mediaA.fps ?? null,
  fpsB: mediaB.fps ?? null,
})

(The existing blended fps can stay as-is for anything that legitimately wants a single shared value, e.g. the scrubber's arrow-key frame-step — only the per-side paused-drift tolerance needs to be per-side.)

Test gap

The existing unit test (apps/web/components/review/compare/__tests__/synced-transport.test.ts) only calls applySideState directly with one pausedTol for one side in isolation — it never exercises useSyncedTransport with two sides of differing fps sharing one tolerance, so this case isn't covered.