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) 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))