Stop refusing ordinary phone footage as variable-frame-rate

Uploading a clip shot straight from the iPhone camera app failed with
"variable-frame-rate video needs timestamp-aware playback". The file was not
variable: its container reports avg_frame_rate 8670/299 and nb_frames 289 over
a stream whose decoded timestamps are 280 frames exactly 1/30s apart. The
guard compared two pieces of container metadata and rejected CFR video on the
strength of a summary the container had got wrong about its own contents.

The guard was also obsolete. It dates from when the page measured the source's
own frames, where a wandering frame duration really does break
`frame = floor(t * fps)`. Nothing measures the source now — ffmpeg resamples it
onto a constant rate and the proxy is re-probed after it is written — so
variable input is a thing this converts rather than a thing it refuses.

So: probe picks a rate instead of validating one. It takes the nominal rate,
which is the rate every timestamp in the stream can be expressed at and so the
one that keeps every distinct source frame, and carries it as an exact fraction
because 30000/1001 is not a float and a rounded -r is how a long take drifts.
The disagreement is still recorded as `vfr`, just not fatal.

The frame-count cross-check went with it. It compared the proxy against the
source's nb_frames, which is the number this whole bug proves can lie, and a
resample to a constant rate legitimately changes the count. It now checks the
proxy's DURATION against the source's, because what must not drift is how long
the picture lasts against how long the audio lasts.

Verified on the reported file: 280 frames at 30fps, picture 9.3333s against
audio 9.3167s — half a frame — and 280/280 detected in the real app. 41 backend
tests green, including a genuinely variable fixture end to end.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

clips/extraction.py

@@ -101,9 +101,10 @@ def _encode_proxy(job, source_path, proxy_path, facts, root):
     _run_with_progress(
         job,
         ["-i", str(source_path), "-an",
-         # Constant frame rate at the source's own rate. `probe` has already
+         # Constant frame rate at the rate `probe` chose. This RESAMPLES rather
-         # refused VFR, so this asserts that rather than resampling.
+         # than asserts: the upload is allowed to be variable, and this is the
-         "-fps_mode", "cfr", "-r", str(facts["fps"]),
+         # step that makes the thing the page measures not be.
+         "-fps_mode", "cfr", "-r", facts.get("rate") or str(facts["fps"]),
          "-c:v", "libx264", "-preset", "veryfast", "-crf", PROXY_CRF,
          # yuv420p and an even frame size are what makes this playable everywhere
          # rather than only in the browser that happened to be tested.
@@ -123,7 +124,33 @@ def _extract_stills(job, proxy_path, frames_dir, frames, root):
         root, "stills", frames, (55, 85))
 
 
+MAX_RATE = 120  # a capture rate; past this the container is describing something else
+
+
 def probe(path):
+    """What the upload is, as far as choosing a proxy rate goes.
+
+    IT NO LONGER REFUSES VARIABLE-FRAME-RATE INPUT, and the reason is the proxy.
+    That refusal was written when the page measured the source's own frames, where
+    a wandering frame duration really does break `frame = floor(t * fps)`. Nothing
+    measures the source now: ffmpeg resamples it onto a constant rate, and the
+    proxy — constant by construction, and re-probed after it is written — is the
+    only timeline anything downstream sees.
+
+    Keeping the check would have been worse than useless, because the thing it
+    tested is not reliable. Ordinary iPhone footage, shot straight from the camera
+    app, reports `avg_frame_rate` 8670/299 and `nb_frames` 289 on a stream whose
+    decoded timestamps are 280 frames exactly 1/30s apart. The container's summary
+    of itself disagreed with the container's own contents, so the guard rejected
+    CFR video for being variable.
+
+    THE RATE IS THE NOMINAL ONE. `r_frame_rate` is the rate every timestamp in the
+    stream can be expressed at, which is the rate that keeps every distinct source
+    frame; resampling to the average would drop some. Duration is preserved either
+    way — ffmpeg's CFR conversion is driven by timestamps, so the audio stays in
+    sync at any rate — so this trades a possible duplicated frame against a
+    certainly lost one.
+    """
     data = json.loads(_command(["ffprobe", "-v", "error", "-show_streams",
                                 "-show_format", "-of", "json", str(path)]))
     video = next((s for s in data.get("streams", []) if s.get("codec_type") == "video"), None)
@@ -131,22 +158,29 @@ def probe(path):
         raise ValueError("the uploaded file has no video stream")
     nominal = Fraction(video.get("r_frame_rate") or "0")
     average = Fraction(video.get("avg_frame_rate") or "0")
-    if nominal <= 0 or average <= 0:
+    if nominal <= 0 and average <= 0:
         raise ValueError("the video's frame rate is unknown")
-    vfr = abs(float(nominal / average) - 1) > 0.001
+    rate = nominal if 0 < nominal <= MAX_RATE else average
-    if vfr:
+    if not 0 < rate <= MAX_RATE:
-        raise ValueError("variable-frame-rate video needs timestamp-aware playback")
+        raise ValueError(f"the video reports a frame rate of {float(rate):g}, which is "
-    frames = video.get("nb_frames")
+                         "not a rate footage can be measured at")
     duration = float(data.get("format", {}).get("duration") or 0)
-    if ((frames and frames.isdigit() and int(frames) > 900)
+    if duration > 0 and duration * float(rate) > 901:
-            or (duration > 0 and duration * float(average) > 901)):
         raise ValueError("video is longer than the 900-frame footage limit")
-    return {"fps": float(average), "nominal_fps": float(nominal),
+    frames = video.get("nb_frames")
+    return {"fps": float(rate),
+            # The exact rate, for ffmpeg. 30000/1001 is not a float, and handing
+            # `-r` a rounded one is how a long take drifts out of sync.
+            "rate": f"{rate.numerator}/{rate.denominator}",
+            "nominal_fps": float(nominal), "average_fps": float(average),
             "width": int(video["width"]), "height": int(video["height"]),
             "duration": duration,
+            # KEPT, AND NO LONGER TRUSTED AS A COUNT. See the docstring: this is
+            # the container's claim about itself, it is wrong on ordinary phone
+            # footage, and `run` checks the proxy's DURATION instead.
             "reported_frames": int(frames) if frames and frames.isdigit() else None,
             "has_audio": any(s.get("codec_type") == "audio" for s in data.get("streams", [])),
-            "vfr": False}
+            "vfr": nominal != average}
 
 
 def count_frames(path):
@@ -224,11 +258,18 @@ def run(key):
             frames = count_frames(proxy_path)
             if not 1 <= frames <= 900:
                 raise ValueError(f"the proxy holds {frames} frames; the limit is 1–900")
-            expected = facts.get("reported_frames")
+            # CHECKED AS A DURATION, not as a frame count. The page's clock is
-            if expected and frames != expected:
+            # `frame = floor(audio.currentTime * fps)`, so what must not drift is
+            # how long the picture lasts against how long the audio lasts — and
+            # the source's own frame count is a number this has already caught
+            # lying. A resample to a constant rate legitimately changes the count
+            # and must not change the duration.
+            drift = abs(frames / proxy_facts["fps"] - facts["duration"])
+            if facts["duration"] > 0 and drift > 0.5:
                 raise ValueError(
-                    f"the proxy holds {frames} frames and the upload reports {expected}; "
+                    f"the proxy runs {frames / proxy_facts['fps']:.2f}s and the upload "
-                    "refusing footage whose picture and audio would drift")
+                    f"runs {facts['duration']:.2f}s; refusing footage whose picture and "
+                    "audio would drift")
             proxy_facts["frames"] = frames
 
             frames_dir = root / "stills"

clips/tests/test_api.py

@@ -656,6 +656,81 @@ class UploadTests(TestCase):
         self.assertNotEqual(Source.objects.get(id=uploaded.json()["id"]).blob_id,
                             footage.video_id)
 
+    def test_a_container_whose_metadata_disagrees_with_itself_is_not_refused(self):
+        # THE REGRESSION. Ordinary iPhone footage, shot straight from the camera
+        # app, reports avg_frame_rate 8670/299 and nb_frames 289 on a stream whose
+        # decoded timestamps are 280 frames exactly 1/30s apart. Refusing that as
+        # "variable-frame-rate" rejected CFR video on the strength of a summary the
+        # container got wrong about its own contents. Nothing measures the source
+        # any more, so the rate is a choice rather than a fact to be verified.
+        report = json.dumps({"streams": [
+            {"codec_type": "video", "r_frame_rate": "30/1", "avg_frame_rate": "8670/299",
+             "nb_frames": "289", "width": 1920, "height": 1440},
+            {"codec_type": "audio"}],
+            "format": {"duration": "9.316667"}})
+        with patch("clips.extraction._command", return_value=report):
+            facts = extraction.probe(Path("phone.mov"))
+        self.assertEqual(30.0, facts["fps"])
+        self.assertTrue(facts["vfr"], "the disagreement is still recorded, just not fatal")
+        self.assertTrue(facts["has_audio"])
+
+    def test_the_proxy_rate_is_exact_rather_than_a_rounded_float(self):
+        # 30000/1001 is not a float. Handing ffmpeg's -r a rounded one is how a
+        # long take drifts out of sync with its own audio.
+        report = json.dumps({"streams": [
+            {"codec_type": "video", "r_frame_rate": "30000/1001",
+             "avg_frame_rate": "30000/1001", "width": 640, "height": 480}],
+            "format": {"duration": "10"}})
+        with patch("clips.extraction._command", return_value=report):
+            facts = extraction.probe(Path("ntsc.mov"))
+        self.assertEqual("30000/1001", facts["rate"])
+
+    def test_a_rate_no_footage_could_have_been_shot_at_is_refused(self):
+        report = json.dumps({"streams": [
+            {"codec_type": "video", "r_frame_rate": "1000/1", "avg_frame_rate": "900/1",
+             "width": 640, "height": 480}],
+            "format": {"duration": "10"}})
+        with patch("clips.extraction._command", return_value=report):
+            with self.assertRaisesMessage(ValueError, "not a rate footage can be measured at"):
+                extraction.probe(Path("nonsense.mov"))
+
+    def test_variable_frame_rate_video_extracts_to_constant_rate_footage(self):
+        # End to end on a genuinely variable file: irregular timestamps in, one
+        # constant-rate proxy out, and the DURATION preserved — which is the thing
+        # that must not move, because the page's clock is the audio.
+        with tempfile.TemporaryDirectory() as directory:
+            root = Path(directory)
+            held = root / "held.mp4"
+            wobbly = root / "wobbly.mp4"
+            subprocess.run([
+                "ffmpeg", "-hide_banner", "-loglevel", "error", "-y",
+                "-f", "lavfi", "-i", "testsrc=s=64x48:r=5:d=2", "-r", "30",
+                "-c:v", "libx264", "-pix_fmt", "yuv420p", str(held)],
+                check=True, capture_output=True)
+            subprocess.run([
+                "ffmpeg", "-hide_banner", "-loglevel", "error", "-y", "-i", str(held),
+                "-vf", "mpdecimate", "-fps_mode", "vfr",
+                "-c:v", "libx264", "-pix_fmt", "yuv420p", str(wobbly)],
+                check=True, capture_output=True)
+            facts = extraction.probe(wobbly)
+            self.assertTrue(facts["vfr"], "the fixture is not actually variable")
+            payload = wobbly.read_bytes()
+
+        uploaded = self.client.post("/api/sources", {
+            "file": SimpleUploadedFile("wobbly.mp4", payload, content_type="video/mp4")})
+        self.assertEqual(201, uploaded.status_code, uploaded.content)
+        with patch("clips.extraction.enqueue", side_effect=extraction.run):
+            queued = self.client.post("/api/extractions", json.dumps({
+                "source": uploaded.json()["id"], "settings": {},
+            }), content_type="application/json")
+        job = self.client.get(f"/api/extractions/{queued.json()['key']}").json()
+        self.assertEqual("done", job["state"], job)
+
+        footage = Footage.objects.get(id=job["footage"])
+        self.assertEqual(facts["fps"], footage.fps)
+        self.assertAlmostEqual(facts["duration"], footage.frames / footage.fps, delta=0.5)
+        self.assertEqual(footage.frames, footage.frame_set.count())
+
     def test_footage_without_a_proxy_says_so_rather_than_serving_nothing(self):
         # Footage ingested before the proxy existed. The manifest reports a null
         # video so the loader can name the fix; it does not omit the field and let