diff --git a/clips/extraction.py b/clips/extraction.py index 25ea808..ef012ae 100644 --- a/clips/extraction.py +++ b/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 - # refused VFR, so this asserts that rather than resampling. - "-fps_mode", "cfr", "-r", str(facts["fps"]), + # Constant frame rate at the rate `probe` chose. This RESAMPLES rather + # than asserts: the upload is allowed to be variable, and this is the + # 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 - if vfr: - raise ValueError("variable-frame-rate video needs timestamp-aware playback") - frames = video.get("nb_frames") + rate = nominal if 0 < nominal <= MAX_RATE else average + if not 0 < rate <= MAX_RATE: + raise ValueError(f"the video reports a frame rate of {float(rate):g}, which is " + "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) - or (duration > 0 and duration * float(average) > 901)): + if duration > 0 and duration * float(rate) > 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") - if expected and frames != expected: + # CHECKED AS A DURATION, not as a frame count. The page's clock is + # `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}; " - "refusing footage whose picture and audio would drift") + f"the proxy runs {frames / proxy_facts['fps']:.2f}s and the upload " + f"runs {facts['duration']:.2f}s; refusing footage whose picture and " + "audio would drift") proxy_facts["frames"] = frames frames_dir = root / "stills" diff --git a/clips/tests/test_api.py b/clips/tests/test_api.py index 55a7b6b..411c0d6 100644 --- a/clips/tests/test_api.py +++ b/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