From e26ad723fabe0b7cc0167db1df2b064db5d5c480 Mon Sep 17 00:00:00 2001 From: Your Name Date: Sun, 4 Oct 2026 23:22:50 -0400 Subject: [PATCH 1/2] Measure the frame rate rather than believing the container MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `probe` took `r_frame_rate` whenever it was at or under the cap, on the grounds that it is the rate that keeps every distinct source frame. It is not a claim about frames at all: ordinary iPhone footage declares 120 over a stream whose timestamps are 1/30s apart, and resampling it up turned an 11-second clip into 1293 proxy frames instead of 323 — four times the encode, four times the tracing stills (91MB against 23MB), four times the blobs and the rows, for 970 frames that are copies of their neighbours. So `_measured_rate` reads the timestamps and `_choose_rate` keeps whichever declared rate they bear out. Two details carry it: the times are sorted before differencing, because an HEVC stream arrives in decode order and differencing that measures the reordering delay instead of the rate; and the statistic is the MEDIAN interval, which is what keeps the property the nominal rate was being taken for — a take held on one frame still reports the rate of the parts that move, so no distinct frame is dropped. A genuine 120fps capture still extracts at 120, and there is a test on that specifically. `Source.probe` also stopped being the place a reading goes to be preserved. The facts are a pure function of bytes that are the row's own identity, so a re-upload re-reads them: otherwise every already-uploaded source would have gone on resampling to four times the frames with no way to correct it short of deleting the row. Already-extracted footage is untouched — `extraction_key` still says scheme 3, so those jobs stay done and reachable. Bumping it re-extracts everything at the corrected rate. Co-Authored-By: Claude Opus 5 --- clips/extraction.py | 108 +++++++++++++++++++++++++++++++++++++--- clips/tests/test_api.py | 98 ++++++++++++++++++++++++++++++++++++ clips/views.py | 11 ++++ 3 files changed, 210 insertions(+), 7 deletions(-) diff --git a/clips/extraction.py b/clips/extraction.py index 3f59ac8..4c4c5d0 100644 --- a/clips/extraction.py +++ b/clips/extraction.py @@ -159,6 +159,16 @@ def _extract_stills(job, proxy_path, frames_dir, frames, root): MAX_RATE = 120 # a capture rate; past this the container is describing something else +# How many packet timestamps `_measured_rate` reads, and the fewest intervals it +# will draw a conclusion from. 300 is a flat cost on a long take and still a +# wide enough sample for a median; below 8 intervals there is not enough of a +# stream to outvote one odd timestamp, so the metadata is left to speak. +RATE_SAMPLE = 300 +RATE_MINIMUM = 8 +# How far a declared rate may sit from the measured one and still be taken as +# what the stream is: 2% covers 30 against 30000/1001 and nothing like 120 +# against 30. +RATE_TOLERANCE = 0.02 def probe_image(path): @@ -184,6 +194,75 @@ def probe_audio(path): return duration +def _measured_rate(path): + """The rate the stream's own packet timestamps imply, or None. + + THE CONTAINER'S SUMMARY OF ITSELF IS NOT EVIDENCE, and this is the function + that goes and looks. An iPhone's `r_frame_rate` is 120 on footage whose + timestamps are 1/30s apart, which is the difference between 323 frames and + 1293 — four times the encode, four times the tracing stills, four times the + blobs, for 970 frames that are copies of their neighbours. + + It reads TIMESTAMPS, not frames: `-show_entries packet=pts_time` demuxes + without decoding, so this costs a file read and no pixels. The times are + SORTED before differencing because a stream with B-frames arrives in decode + order — an HEVC clip's first packets come out 0, 0.133, 0.067, 0.033 — and + differencing that order measures the reordering rather than the rate. + + THE MEDIAN INTERVAL, which is what makes this safe on genuinely variable + input. It answers "how far apart are two frames normally", so a take held on + one frame for a second still reports the rate of the parts that move, and + choosing it keeps every distinct frame — the property `probe` used to reach + for by taking the nominal rate. Only the last few intervals of the sample are + unreliable (a frame whose turn comes after the window is missing from it), and + a median does not care. + + Returning None is the honest answer for a clip too short to sample, and this + also swallows a probe that fails outright: the rate the metadata declares is + the documented fallback, so an optimisation must not be able to refuse an + upload that would otherwise have been accepted. + """ + try: + text = _command(["ffprobe", "-v", "error", "-select_streams", "v:0", + "-show_entries", "packet=pts_time", "-of", "json", + "-read_intervals", f"%+#{RATE_SAMPLE}", str(path)]) + packets = json.loads(text).get("packets") or [] + times = sorted(float(packet["pts_time"]) for packet in packets + if (packet.get("pts_time") or "N/A") != "N/A") + except (ValueError, OSError): + return None + intervals = sorted(b - a for a, b in zip(times, times[1:]) if b > a) + if len(intervals) < RATE_MINIMUM: + return None + median = intervals[len(intervals) // 2] + return 1.0 / median if median > 0 else None + + +def _choose_rate(nominal, average, measured): + """The rate to resample onto, as an exact Fraction. + + A DECLARED RATE IS PREFERRED WHEN IT AGREES WITH THE TIMESTAMPS, because it is + the exact rational the stream was authored at — 30000/1001 is not a float, and + `limit_denominator` on a measured 29.97 is a guess at a number the container + already states. So the measured rate is used to CHOOSE between what the + container declares, and only stands in itself when neither declaration + describes the stream. + """ + candidates = [rate for rate in (nominal, average) if 0 < rate <= MAX_RATE] + if measured: + agreeing = [rate for rate in candidates + if abs(float(rate) - measured) <= RATE_TOLERANCE * measured] + if agreeing: + return min(agreeing, key=lambda rate: abs(float(rate) - measured)) + from_timestamps = Fraction(measured).limit_denominator(1001) + if 0 < from_timestamps <= MAX_RATE: + return from_timestamps + # Nothing to go on but the metadata, and nominal first keeps the rate that + # drops no distinct frame. An unusable pair falls through to the refusal + # below, which names the rate the file claimed rather than one of these. + return nominal if 0 < nominal <= MAX_RATE else average + + def probe(path): """What the upload is, as far as choosing a proxy rate goes. @@ -201,12 +280,21 @@ def probe(path): 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. + THE RATE IS MEASURED AND THE DECLARATIONS ARE VOTED ON, which is the same + distrust applied to the one number that still comes from here. This used to + take `r_frame_rate` outright — the rate every timestamp in the stream can be + expressed at, and so the rate that keeps every distinct source frame. The + trouble is that it is not a claim about frames at all: the file above declares + 120 and holds 30, and resampling it up cost four times the encode, four times + the tracing stills and four times the blobs for 970 duplicated frames. So + `_measured_rate` reads the timestamps, `_choose_rate` keeps whichever declared + rate they bear out, and the nominal rate is believed when it is true rather + than because it is nominal. + + Duration is preserved either way — ffmpeg's CFR conversion is driven by + timestamps, so the audio stays in sync at any rate — and the median interval + keeps the no-distinct-frame-dropped property that taking the nominal rate was + reaching for. See `_measured_rate`. """ data = json.loads(_command(["ffprobe", "-v", "error", "-show_streams", "-show_format", "-of", "json", str(path)])) @@ -217,7 +305,8 @@ def probe(path): average = Fraction(video.get("avg_frame_rate") or "0") if nominal <= 0 and average <= 0: raise ValueError("the video's frame rate is unknown") - rate = nominal if 0 < nominal <= MAX_RATE else average + measured = _measured_rate(path) + rate = _choose_rate(nominal, average, measured) 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") @@ -230,6 +319,11 @@ def probe(path): # `-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), + # What the timestamps said, and null when there were too few to ask. + # Recorded because it is the input to a decision this file used not to + # make, and the one number that explains a chosen rate matching + # neither declaration. + "measured_fps": measured, "width": int(video["width"]), "height": int(video["height"]), "duration": duration, # KEPT, AND NO LONGER TRUSTED AS A COUNT. See the docstring: this is diff --git a/clips/tests/test_api.py b/clips/tests/test_api.py index 8e32565..66fc156 100644 --- a/clips/tests/test_api.py +++ b/clips/tests/test_api.py @@ -908,6 +908,37 @@ class UploadTests(TestCase): self.assertEqual("image/jpeg", still["Content-Type"]) self.assertEqual(200, self.client.get(footage["audio"]).status_code) + def test_re_uploading_a_source_re_reads_its_facts(self): + # A SOURCE ROW HOLDS A READING, NOT A DECISION. The facts are a pure + # function of bytes that are themselves this row's identity, so the row + # cannot be the place a reading goes to be preserved: `probe` got better + # at phone footage — it stopped believing a declared 120 over timestamps + # 1/30s apart — and a stored reading that nothing can replace would have + # left every already-uploaded source resampling to four times the frames + # with no way to correct it short of deleting the row. + with tempfile.TemporaryDirectory() as directory: + path = Path(directory) / "four-frames.mp4" + subprocess.run([ + "ffmpeg", "-hide_banner", "-loglevel", "error", "-y", + "-f", "lavfi", "-i", "color=c=red:s=64x48:r=4:d=1", + "-c:v", "mpeg4", str(path), + ], check=True, capture_output=True) + payload = path.read_bytes() + + first = self.client.post("/api/sources", { + "file": SimpleUploadedFile("four-frames.mp4", payload, content_type="video/mp4")}) + self.assertEqual(201, first.status_code, first.content) + self.assertEqual(4.0, first.json()["probe"]["fps"]) + + better = dict(first.json()["probe"], fps=12.0, rate="12/1", measured_fps=12.0) + with patch("clips.extraction.probe", return_value=better): + again = self.client.post("/api/sources", { + "file": SimpleUploadedFile("same.mp4", payload, content_type="video/mp4")}) + self.assertEqual(200, again.status_code, again.content) + self.assertFalse(again.json()["created"], "the same bytes are the same source") + self.assertEqual("12/1", again.json()["probe"]["rate"]) + self.assertEqual(12.0, Source.objects.get(id=first.json()["id"]).probe["fps"]) + def test_the_proxy_is_re_encoded_rather_than_the_upload_re_served(self): # The footage's identity is the proxy's digest, and the proxy is produced # by one ffmpeg invocation whatever the upload was. If the upload were @@ -955,6 +986,73 @@ class UploadTests(TestCase): self.assertTrue(facts["vfr"], "the disagreement is still recorded, just not fatal") self.assertTrue(facts["has_audio"]) + def test_a_declared_rate_the_timestamps_do_not_bear_out_is_not_resampled_to(self): + # THE FOUR-TIMES. An iPhone container declares `r_frame_rate` 120 over a + # stream whose frames are 1/30s apart, and taking the declaration at its + # word turned an 11-second clip into 1293 proxy frames instead of 323: + # four times the encode, four times the tracing stills, four times the + # blobs and the rows, for 970 frames that are copies of their neighbours. + # The timestamps are the evidence and they say 30. + streams = json.dumps({"streams": [ + {"codec_type": "video", "r_frame_rate": "120/1", + "avg_frame_rate": "96900/3233", "nb_frames": "323", + "width": 1920, "height": 1440}, + {"codec_type": "audio"}], + "format": {"duration": "10.775"}}) + # IN DECODE ORDER, which is how an HEVC stream really arrives — the first + # packets of the fixture this was found on come out 0, 0.133, 0.067, + # 0.033. Differencing that order unsorted measures the reordering delay + # and not the rate, so the fixture keeps the hazard in it. + shuffled = [0, 4, 2, 1, 3, 8, 6, 5, 7, 12, 10, 9, 11] + packets = json.dumps({"packets": [{"pts_time": f"{i / 30:.6f}"} for i in shuffled]}) + with patch("clips.extraction._command", side_effect=[streams, packets]): + facts = extraction.probe(Path("phone.mov")) + self.assertEqual("96900/3233", facts["rate"], "resampled to the declared 120") + self.assertAlmostEqual(30.0, facts["measured_fps"], places=2) + + def test_a_genuine_high_rate_capture_is_still_taken_at_its_own_rate(self): + # The other half of the same decision, and the one that would be easy to + # break: a real 120fps capture must not be dragged down to anything. Its + # declaration and its timestamps agree, so the declaration — the exact + # rational the stream was authored at — is what is used. + streams = json.dumps({"streams": [ + {"codec_type": "video", "r_frame_rate": "120/1", "avg_frame_rate": "120/1", + "width": 640, "height": 480}], + "format": {"duration": "2"}}) + packets = json.dumps({"packets": [{"pts_time": f"{i / 120:.6f}"} for i in range(13)]}) + with patch("clips.extraction._command", side_effect=[streams, packets]): + facts = extraction.probe(Path("slowmo.mov")) + self.assertEqual("120/1", facts["rate"]) + + def test_too_few_timestamps_to_measure_leaves_the_declaration_standing(self): + # A clip with nine-ish frames cannot outvote one odd timestamp, so the + # measurement declines to have an opinion and the nominal rate — the one + # that drops no distinct frame — is used exactly as it was before. + streams = json.dumps({"streams": [ + {"codec_type": "video", "r_frame_rate": "30/1", "avg_frame_rate": "24/1", + "width": 640, "height": 480}], + "format": {"duration": "0.1"}}) + packets = json.dumps({"packets": [{"pts_time": f"{i / 30:.6f}"} for i in range(3)]}) + with patch("clips.extraction._command", side_effect=[streams, packets]): + facts = extraction.probe(Path("tiny.mov")) + self.assertEqual("30/1", facts["rate"]) + self.assertIsNone(facts["measured_fps"]) + + def test_a_rate_measurement_that_fails_outright_cannot_refuse_an_upload(self): + # The measurement is an optimisation. If ffprobe cannot read the packets + # of a file whose streams it just read happily, the upload still has to be + # accepted on its metadata — an optimisation that can reject work is worse + # than no optimisation. + streams = json.dumps({"streams": [ + {"codec_type": "video", "r_frame_rate": "25/1", "avg_frame_rate": "25/1", + "width": 640, "height": 480}], + "format": {"duration": "4"}}) + with patch("clips.extraction._command", + side_effect=[streams, ValueError("ffprobe fell over")]): + facts = extraction.probe(Path("awkward.mov")) + self.assertEqual("25/1", facts["rate"]) + self.assertIsNone(facts["measured_fps"]) + 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. diff --git a/clips/views.py b/clips/views.py index 0e431eb..597a286 100644 --- a/clips/views.py +++ b/clips/views.py @@ -218,6 +218,17 @@ def sources(request): "media_type": upload.content_type or "video/mp4"}) row, created = Source.objects.get_or_create( blob=blob, defaults={"filename": Path(upload.name).name[:255], "probe": facts}) + if not created and row.probe != facts: + # THE FACTS ARE RE-READ, NOT REMEMBERED. They are a pure function of + # the bytes, and the bytes are this row's identity — so a + # disagreement means the server reads the file differently now from + # whenever it first saw it, and the fresh reading is the one to keep. + # Storing the first reading forever pins a source to a rate the code + # no longer believes in, and makes it unfixable without deleting the + # row: `extraction.probe` got better at phone footage and every + # already-uploaded source would have gone on being wrong. + row.probe = facts + row.save(update_fields=["probe"]) return JsonResponse({"id": str(row.id), "digest": digest, "filename": row.filename, "probe": row.probe, "created": created}, status=201 if created else 200) From a6b6c116c6124fc6bd0a58d2828dec82ac3ac257 Mon Sep 17 00:00:00 2001 From: Your Name Date: Sun, 4 Oct 2026 23:22:58 -0400 Subject: [PATCH 2/2] Count the upload in the status line MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The transfer is the longest part of an import on anything but a local server, and it was the only part with no number on it: `uploading video…` sat unchanged from the first byte to the last, however many there were, and only the extraction that followed it ever counted. Most of what "slow" means to whoever is waiting was a spinner of unknown length. `fetch` cannot report this. A Request built from a FormData gives no way to observe its own upload — the promise settles when the response arrives — so `POST-form` gained an XMLHttpRequest arity, which is the one thing XHR can still do that fetch cannot. `fail` became `failure`, building the ex-info rather than throwing it, because the two transports raise it differently: fetch throws inside a `.then` and XHR has to reject by hand. `sending` dispatches only when the whole percentage moves, since the browser fires progress as often as it pleases and each dispatch re-renders the pane. Video, sound and image uploads all report. Co-Authored-By: Claude Opus 5 --- frontend/src/arthur/events/footage.cljs | 26 ++++++++- frontend/src/arthur/fx/http.cljs | 77 +++++++++++++++++++++---- 2 files changed, 89 insertions(+), 14 deletions(-) diff --git a/frontend/src/arthur/events/footage.cljs b/frontend/src/arthur/events/footage.cljs index a88f926..7a25a0a 100644 --- a/frontend/src/arthur/events/footage.cljs +++ b/frontend/src/arthur/events/footage.cljs @@ -334,12 +334,32 @@ (.catch (fn [error] (rf/dispatch [::failed (or (ex-message error) (str error))]))))) +(defn- sending + "A progress callback that puts the whole percentage sent in the status line. + + ONLY WHEN THE PERCENTAGE MOVES. The browser fires upload progress as often as + it pleases and a status line has something new to say a hundred times at most, + and each dispatch here re-renders the pane. + + Worth having at all because the transfer is the longest part of an import on + anything but a local server, and it was the part with no number on it: + `uploading video…` sat unchanged from the first byte to the last, however many + there were, and only the extraction that followed it ever counted. See + `arthur.fx.http/POST-form`." + [label] + (let [reported (atom -1)] + (fn [fraction] + (let [percent (js/Math.round (* 100 fraction))] + (when (not= percent @reported) + (reset! reported percent) + (rf/dispatch [::progress (str label " " percent "%")])))))) + (rf/reg-fx ::upload! (fn [file] (let [form (js/FormData.)] (.append form "file" file) - (-> (http/POST-form "/api/sources" form) + (-> (http/POST-form "/api/sources" form (sending "uploading video…")) (.then (fn [^js source] (rf/dispatch [::progress "queued for extraction…"]) (http/POST "/api/extractions" #js {:source (.-id source) @@ -353,7 +373,7 @@ (fn [file] (let [form (js/FormData.)] (.append form "file" file) - (-> (http/POST-form "/api/sounds" form) + (-> (http/POST-form "/api/sounds" form (sending "uploading sound…")) (.then (fn [^js sound] (rf/dispatch [::uploaded (.-id sound) "sound imported"]))) (.catch (fn [error] (rf/dispatch [::failed (or (ex-message error) (str error))]))))))) @@ -363,7 +383,7 @@ (fn [file] (let [form (js/FormData.)] (.append form "file" file) - (-> (http/POST-form "/api/images" form) + (-> (http/POST-form "/api/images" form (sending "uploading image…")) (.then (fn [^js image] (rf/dispatch [::uploaded (.-id image) "image imported"]))) (.catch (fn [error] (rf/dispatch [::failed (or (ex-message error) (str error))]))))))) diff --git a/frontend/src/arthur/fx/http.cljs b/frontend/src/arthur/fx/http.cljs index f4e3650..29d5ec5 100644 --- a/frontend/src/arthur/fx/http.cljs +++ b/frontend/src/arthur/fx/http.cljs @@ -19,17 +19,28 @@ (when (= "csrftoken" (str/trim (or k ""))) v))) (str/split (or (.-cookie js/document) "") #";"))) -(defn- fail +(defn- failure "Turn a non-2xx into an ex-info carrying what the server said. The server's message is the useful one — \"this clip names tier-2 blocks the server does not have\" — and a status code alone would put the interesting half - of it in a console nobody is watching." - [response body] - (throw (ex-info (or (some-> body .-error) - (str "the server answered " (.-status response))) - {:status (.-status response) - :body (when body (js->clj body :keywordize-keys true))}))) + of it in a console nobody is watching. + + Built rather than thrown, because the two transports below raise it in + different ways: `fetch` throws inside a `.then` and XHR has to reject a promise + by hand." + [status body] + (ex-info (or (some-> body .-error) + (str "the server answered " status)) + {:status status + :body (when body (js->clj body :keywordize-keys true))})) + +(defn- parse + "A response body as a parsed JS value, or nil. An empty 204 and a 500 whose + body is an HTML error page both land here, and neither is worth an exception." + [text] + (when (seq text) + (try (js/JSON.parse text) (catch :default _ nil)))) (defn request! ([method url] (request! method url nil)) @@ -47,14 +58,58 @@ (.then (fn [response] (-> (.text response) (.then (fn [text] - (let [parsed (when (seq text) - (try (js/JSON.parse text) (catch :default _ nil)))] + (let [parsed (parse text)] (if (.-ok response) parsed - (fail response parsed)))))))))))) + (throw (failure (.-status response) parsed))))))))))))) + +(defn POST-form + "A multipart POST, optionally reporting how much of the body has gone out. + + XMLHttpRequest RATHER THAN `fetch`, for the one thing fetch cannot do: a + Request built from a FormData gives no way to observe its own upload, so the + promise settles when the response arrives and everything before that is a + single unknown. On the one call that sends a whole video that unknown is + minutes long, and a spinner with no number on it is most of what \"slow\" means + to whoever is waiting — so this is worth the twenty lines XHR costs. + + `on-progress` is called with the fraction sent, 0.0 to 1.0. Only while the + browser can say what the total is: a body whose length it cannot compute + reports nothing rather than a made-up number. + + CONTENT-TYPE IS DELIBERATELY NOT SET. The browser has to write the multipart + boundary into it, and setting it by hand sends a body the server cannot parse." + ([url body] (request! "POST" url body)) + ([url body on-progress] + (js/Promise. + (fn [resolve reject] + (let [xhr (js/XMLHttpRequest.)] + (.open xhr "POST" url) + (.setRequestHeader xhr "Accept" "application/json") + (.setRequestHeader xhr "X-CSRFToken" (or (csrf-token) "")) + (set! (.. xhr -upload -onprogress) + (fn [^js event] + (when (and (.-lengthComputable event) (pos? (.-total event))) + (on-progress (/ (.-loaded event) (.-total event)))))) + (set! (.-onload xhr) + (fn [] + (let [parsed (parse (.-responseText xhr))] + (if (<= 200 (.-status xhr) 299) + (resolve parsed) + (reject (failure (.-status xhr) parsed)))))) + ;; The three ways a send produces no response at all. None of them has a + ;; status, so none of them can go through `failure` — and a dropped + ;; upload has to say so, because it is the one failure here that is + ;; routine rather than a bug. + (set! (.-onerror xhr) + (fn [_] (reject (ex-info "the upload did not reach the server" {})))) + (set! (.-onabort xhr) + (fn [_] (reject (ex-info "the upload was cancelled" {})))) + (set! (.-ontimeout xhr) + (fn [_] (reject (ex-info "the upload timed out" {})))) + (.send xhr body)))))) (defn GET [url] (request! "GET" url)) (defn POST [url body] (request! "POST" url body)) -(defn POST-form [url body] (request! "POST" url body)) (defn PUT [url body] (request! "PUT" url body)) (defn PATCH [url body] (request! "PATCH" url body))