Count the upload in the status line

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 <noreply@anthropic.com>

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

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.

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)

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

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