fix(downloads): GIF review round 1 — shape, HDR, caps, audio, copy

- A GIF squares its pixels before the size, so an anamorphic source keeps
  its shown shape (a 720x480 DVD at 16:9 was 480x320).
- An editor GIF of an HDR upload is tonemapped, as a clip GIF is.
- The length caps are judged in whole milliseconds: a range exactly at a
  cap is no longer refused for a float hair over it.
- A GIF is made of a video only: an audio file is refused with its reason
  and the editor offers no GIF for it.
- A GIF clip keeps no audio track: the track neither splits its cache key
  nor fails it.
- The editor's GIF notes keep their live region mounted; one size reads
  as one ("about 1 MB"), none is shown with nothing kept; the size choices
  show their unit; a shared picture that fails to load says so.
- Copy: the two-sentence GIF notes keep their last stop (a copy-rule test
  now holds it); the English warns of "only a still picture".

backend/app/downloads/clip.py

@@ -110,7 +110,10 @@ def normalize(
     if not (math.isfinite(start_s) and math.isfinite(end_s)) or start_s < 0:
         raise ClipError("bad_range")
     start, end = round(start_s, 3), round(end_s, 3)
-    if end - start < MIN_CLIP_S:
+    # Judged in whole milliseconds, as the ends are kept: a float difference lands a hair off
+    # (60.529 - 30.529 = 30.000000000000004), which would refuse a range exactly at a cap.
+    span_ms = round((end - start) * 1000)
+    if span_ms < MIN_CLIP_S * 1000:
         raise ClipError("too_short")
     side = None
     if gif_side is not None:
@@ -118,15 +121,16 @@ def normalize(
             side = gifmod.normalize_side(gif_side)
         except ValueError as e:
             raise ClipError("bad_gif") from e
-    if side is not None and end - start > gifmod.MAX_S:
+    if side is not None and gifmod.too_long(start, end):
         # Its own reason: a GIF's cap is said in seconds, a clip's in minutes.
         raise ClipError("gif_too_long", limit_s=gifmod.MAX_S)
-    if end - start > MAX_CLIP_S:
+    if span_ms > MAX_CLIP_S * 1000:
         raise ClipError("too_long", limit_s=MAX_CLIP_S)
     spec: dict = {"source": source, "ref": ref, "start_s": start, "end_s": end}
     if side is not None:
         spec["gif"] = {"side": side}
-    if source == "plex" and audio_track is not None:
+    # A GIF has no sound: the track the player was on is not part of what it is (its cache key).
+    if source == "plex" and audio_track is not None and side is None:
         if not 0 <= audio_track <= _MAX_AUDIO_TRACK:
             raise ClipError("bad_audio")
         spec["audio_track"] = audio_track

backend/app/downloads/gif.py

@@ -39,6 +39,19 @@ def normalize_side(side: object) -> int:
     return side
 
 
+def too_long(start_s: float, end_s: float) -> bool:
+    """Over `MAX_S`, counted in whole milliseconds: the ends are kept to the millisecond, and their
+    float difference may land a hair over (60.529 - 30.529 = 30.000000000000004), which would refuse
+    the very 30 s the panels allow."""
+    return round((end_s - start_s) * 1000) > MAX_S * 1000
+
+
+# A GIF has no pixel shape a viewer honours (browsers ignore its aspect byte), so a source stored
+# with wide or narrow pixels (an anamorphic DVD: 720x480 shown at 16:9) is first resampled to square
+# ones, or it shows squeezed. `sar` is 1 for a source that names none; even, as a 4:2:0 frame wants.
+_SQUARE_PIXELS = "scale=w='trunc(iw*sar/2)*2':h=ih,setsar=1"
+
+
 def _scale(side: int) -> str:
     """Fit the long side into `side`, never upscaling a smaller picture; the short side keeps the
     aspect (-1: a GIF has no chroma grid that wants it even)."""
@@ -50,8 +63,10 @@ def _scale(side: int) -> str:
 
 def filter_graph(pre: str, side: int) -> str:
     """The GIF's filters: `pre` (a crop, an HDR tonemap) on the source's own frame first, then the
-    frame rate and the size, then a palette made from these frames and applied to them."""
-    head = ",".join([*([pre] if pre else []), f"fps={FPS}", _scale(side)])
+    frame rate, square pixels and the size, then a palette made from these frames and applied to them.
+    A crop counts the stored frame (both callers' crops are fractions of `iw`/`ih`), so it comes
+    before the pixels are squared."""
+    head = ",".join([*([pre] if pre else []), f"fps={FPS}", _SQUARE_PIXELS, _scale(side)])
     return (
         f"[0:v:0]{head},split[a][b];"
         "[a]palettegen=stats_mode=diff[p];"

backend/app/downloads/pipeline.py

@@ -888,15 +888,21 @@ def _process_edit(job_id: int, asset_id: int) -> None:
 
         _set_job(job_id, phase="editing", progress=0, speed_bps=None, eta_s=None)
         if gif:
-            # One range with an end (the service's check), cropped first if a frame was drawn.
+            # One range with an end (the service's check), cropped first if a frame was drawn, then
+            # tonemapped as a clip GIF is: an HDR upload keeps its HLG/PQ tags, and a GIF has none,
+            # so a palette made from those frames would be washed out.
             trim = edit_spec["trim"]
             start = trim.get("start_s") or 0.0
+            data = probe.probe_json(src_path)
             cmd = gifmod.build_cmd(
                 src_path,
                 dest_staging,
                 start,
                 trim["end_s"] - start,
-                pre=editmod.crop_filter(edit_spec["crop"]) if crop else "",
+                pre=clipmod.gif_pre(
+                    editmod.crop_filter(edit_spec["crop"]) if crop else "",
+                    bool(data) and clipmod.is_hdr(data),
+                ),
                 side=gif["side"],
             )
         elif edit_spec.get("segments"):
@@ -1115,14 +1121,17 @@ def _process_clip(job_id: int, asset_id: int) -> None:
             raise StatedFailure("The source file could not be read.")
         audio_track = spec.get("audio_track")
         tracks = clipmod.audio_track_count(data)
-        if tracks == 0:
+        gif = spec.get("gif")
+        if gif:
+            # A GIF has no sound: no track is chosen, so none can be missing.
+            audio_track = None
+        elif tracks == 0:
             # A silent source: the player still names its track 0, and the clip is silent too.
             audio_track = None
         elif audio_track is not None and audio_track >= tracks:
             raise StatedFailure("The chosen audio track is not in the file.")
         crop = _clip_crop(spec, data)
         total_s = clipmod.duration(spec)
-        gif = spec.get("gif")
         hdr = clipmod.is_hdr(data)
         out = staging / ("clip.gif" if gif else "clip.mp4")
         _set_job(job_id, phase="editing", progress=0, speed_bps=None, eta_s=None)

backend/app/downloads/service.py

@@ -44,7 +44,13 @@ from app.userscope import owned_by, owned_row
 # Every reason an edit is refused; the UI names each one (`downloads.errors.edit.<reason>`, held by
 # the frontend's backendIds contract, which reads this Literal).
 EditReason = Literal[
-    "source_not_ready", "not_media", "empty_edit", "bad_gif", "gif_range", "gif_too_long"
+    "source_not_ready",
+    "not_media",
+    "empty_edit",
+    "bad_gif",
+    "gif_range",
+    "gif_too_long",
+    "gif_not_video",
 ]
 
 
@@ -268,7 +274,7 @@ def _check_gif_edit(spec: dict) -> None:
     trim = spec.get("trim") or {}
     if spec.get("segments") or trim.get("end_s") is None:
         raise EditError("gif_range")
-    if trim["end_s"] - trim.get("start_s", 0.0) > gifmod.MAX_S:
+    if gifmod.too_long(trim.get("start_s", 0.0), trim["end_s"]):
         raise EditError("gif_too_long", limit_s=gifmod.MAX_S)
 
 
@@ -298,6 +304,9 @@ def enqueue_edit(
     if not spec:
         raise EditError("empty_edit")
     if "gif" in (edit_spec or {}):
+        # A GIF is made of the first video stream: audio has none, or only its cover picture.
+        if src_asset.media_kind != mediakind.VIDEO:
+            raise EditError("gif_not_video")
         _check_gif_edit(spec)
 
     quota.check_enqueue(db, user_id)

backend/tests/test_clip.py

@@ -740,6 +740,30 @@ def test_the_thirty_second_gif_cap_itself_is_allowed():
     assert clipmod.duration(clipmod.normalize("plex", "42", 0, 30, gif_side=480)) == 30
 
 
+@pytest.mark.parametrize(
+    ("start", "end", "kw"),
+    [
+        # Each a range exactly at a cap whose float difference lands a hair past it.
+        (30.529, 60.529, {"gif_side": 480}),
+        (424.005, 1024.005, {}),
+        (0.001, 1.001, {}),
+    ],
+)
+def test_a_range_exactly_at_a_cap_is_judged_in_milliseconds(start, end, kw):
+    assert end - start not in (1, 30, 600)  # the float difference is off the cap itself
+    assert round((end - start) * 1000) in (1000, 30000, 600000)
+    clipmod.normalize("youtube", "abcdefghijk", start, end, **kw)
+
+
+def test_a_gif_clip_keeps_no_audio_track_so_the_track_does_not_make_another_gif():
+    first = clipmod.normalize("plex", "42", 10, 20, audio_track=0, gif_side=480)
+    other = clipmod.normalize("plex", "42", 10, 20, audio_track=1, gif_side=480)
+    assert "audio_track" not in first
+    assert clipmod.sig(first) == clipmod.sig(other)
+    # A video clip keeps the track it was heard on.
+    assert clipmod.normalize("plex", "42", 10, 20, audio_track=1)["audio_track"] == 1
+
+
 def test_a_gif_clip_is_queued_as_a_file_and_too_long_says_its_own_cap(
     db, make_user, as_user, yt_video
 ):
@@ -810,6 +834,19 @@ def test_a_gif_clip_is_cut_through_the_gif_command_and_stored_as_an_image_file(
     assert cmd[cmd.index("-t") + 1] == "15.000"
 
 
+def test_a_gif_clip_is_never_refused_for_an_audio_track(
+    db, make_user, as_user, plex_episode, gif_worker
+):
+    # A GIF queued with a track (as one made before GIFs dropped it was): the file has one track,
+    # the spec names a fourth, and a GIF, which has no sound, is made all the same.
+    job = _clip(as_user(make_user()), source="plex", ref="777", gif_side=480, end_s=25).json()
+    row = db.get(DownloadJob, job["id"])
+    row.edit_spec = {**row.edit_spec, "audio_track": 3}
+    db.commit()
+    job, asset = gif_worker["run"](job)
+    assert (job.status, asset.status) == ("done", "ready")
+
+
 def test_a_gif_over_its_size_cap_is_not_kept_and_says_why(
     db, make_user, as_user, plex_episode, gif_worker, monkeypatch
 ):

backend/tests/test_edit_gif.py

@@ -3,12 +3,15 @@ to the GIF's limits at enqueue, and made by the worker with real ffmpeg."""
 
 import shutil
 import subprocess
+from pathlib import Path
 
 import pytest
 
 from app.config import settings
+from app.downloads import clip as clipmod
 from app.downloads import edit as editmod
 from app.downloads import pipeline, service
+from app.downloads import probe as probemod
 from app.downloads import gif as gifmod
 from app.downloads import upload as uploadmod
 from app.downloads.models import DownloadJob, MediaAsset
@@ -63,6 +66,84 @@ def test_a_gif_edit_it_cannot_make_is_refused_with_its_reason(
     assert e.value.reason == reason
 
 
+@needs_ffmpeg
+def test_a_gif_edit_of_exactly_thirty_seconds_is_allowed(db, make_user, tmp_path, monkeypatch):
+    monkeypatch.setattr(settings, "files_root", str(tmp_path / "root"))
+    u = make_user()
+    source = _source(db, u, tmp_path)
+    # Ends kept to the millisecond whose float difference lands a hair over 30 s.
+    assert 32.02 - 2.02 > gifmod.MAX_S
+    job = service.enqueue_edit(
+        db, u.id, source, {"trim": {"start_s": 2.02, "end_s": 32.02}, "gif": {"side": 480}}
+    )
+    assert job.edit_spec["gif"] == {"side": 480}
+
+
+@needs_ffmpeg
+def test_a_gif_is_made_of_a_video_never_of_audio(db, make_user, tmp_path, monkeypatch):
+    monkeypatch.setattr(settings, "files_root", str(tmp_path / "root"))
+    u = make_user()
+    src = tmp_path / "in.m4a"
+    subprocess.run(
+        [
+            "ffmpeg", "-y", "-loglevel", "error",
+            "-f", "lavfi", "-i", "sine=frequency=440:duration=10", "-c:a", "aac", str(src),
+        ],
+        check=True,
+    )  # fmt: skip
+    source = uploadmod.ingest_upload(
+        db, u.id, filename="in.m4a", src_path=src, size=src.stat().st_size
+    )
+    assert db.get(MediaAsset, source.asset_id).media_kind == "audio"
+    spec = {"trim": {"start_s": 1, "end_s": 5}}
+    with pytest.raises(service.EditError) as e:
+        service.enqueue_edit(db, u.id, source, {**spec, "gif": {"side": 480}})
+    assert e.value.reason == "gif_not_video"
+    # The same range as audio is an edit as it always was.
+    assert service.enqueue_edit(db, u.id, source, spec).edit_spec["trim"]["end_s"] == 5
+
+
+@needs_ffmpeg
+def test_an_hdr_source_is_tonemapped_before_its_gif_palette(db, make_user, tmp_path, monkeypatch):
+    monkeypatch.setattr(settings, "files_root", str(tmp_path / "root"))
+    u = make_user()
+    src = tmp_path / "hlg.mp4"
+    subprocess.run(
+        [
+            "ffmpeg", "-y", "-loglevel", "error",
+            "-f", "lavfi", "-i", "testsrc2=size=640x360:rate=10:duration=4",
+            "-vf", "setparams=color_trc=arib-std-b67:color_primaries=bt2020:colorspace=bt2020nc",
+            "-c:v", "libx264", "-pix_fmt", "yuv420p", str(src),
+        ],
+        check=True,
+    )  # fmt: skip
+    source = uploadmod.ingest_upload(
+        db, u.id, filename="hlg.mp4", src_path=src, size=src.stat().st_size
+    )
+    # The upload keeps its HLG tag, as an HDR phone video's does.
+    placed = Path(settings.files_root) / db.get(MediaAsset, source.asset_id).rel_path
+    assert clipmod.is_hdr(probemod.probe_json(placed) or {})
+    job = service.enqueue_edit(
+        db, u.id, source, {"trim": {"start_s": 1, "end_s": 3}, "gif": {"side": 320}}
+    )
+    asset = db.get(MediaAsset, job.asset_id)
+    job.status, asset.status = "running", "downloading"
+    db.commit()
+    seen: dict = {}
+    build = gifmod.build_cmd
+
+    def spy(*a, **k):
+        seen["pre"] = k["pre"]
+        return build(*a, **k)
+
+    monkeypatch.setattr(pipeline.gifmod, "build_cmd", spy)
+    pipeline._process_edit(job.id, asset.id)
+
+    assert "tonemap" in seen["pre"]
+    db.expire_all()
+    assert db.get(DownloadJob, job.id).status == "done"
+
+
 @needs_ffmpeg
 def test_a_gif_edit_is_made_by_the_worker_and_kept_as_an_image_file(
     db, make_user, tmp_path, monkeypatch

backend/tests/test_gif.py

@@ -66,11 +66,25 @@ def test_a_crop_or_a_tonemap_comes_before_the_scale_and_the_palette():
     assert graph.index("crop=100:100:0:0") < graph.index("scale=") < graph.index("palettegen")
 
 
-def _video(path: Path, size: str, seconds: int = 2) -> Path:
+def test_the_length_cap_counts_whole_milliseconds():
+    # Two ends kept to the millisecond whose float difference lands a hair over 30 s.
+    assert 60.529 - 30.529 > gifmod.MAX_S
+    assert not gifmod.too_long(30.529, 60.529)
+    assert not gifmod.too_long(0, 30)
+    assert gifmod.too_long(0, 30.001)
+
+
+def test_the_pixels_are_squared_after_the_crop_and_before_the_size():
+    graph = gifmod.filter_graph("crop=100:100:0:0", 480)
+    assert graph.index("crop=") < graph.index("setsar=1") < graph.index("flags=lanczos")
+
+
+def _video(path: Path, size: str, seconds: int = 2, sar: str = "1") -> Path:
     subprocess.run(
         [
             "ffmpeg", "-y", "-loglevel", "error",
             "-f", "lavfi", "-i", f"testsrc2=size={size}:rate=25:duration={seconds}",
+            "-vf", f"setsar={sar}",
             "-c:v", "libx264", "-pix_fmt", "yuv420p", str(path),
         ],
         check=True,
@@ -78,6 +92,24 @@ def _video(path: Path, size: str, seconds: int = 2) -> Path:
     return path
 
 
+@needs_ffmpeg
+@pytest.mark.parametrize(
+    ("size", "sar", "expected"),
+    [
+        # A DVD stored 720x480 and shown at 16:9: a GIF shows its stored pixels as they are, so
+        # they are squared first, or the picture comes out 480x320 (3:2), squeezed.
+        ("720x480", "32/27", (480, 270)),
+        # HDV: 1440x1080 shown at 16:9.
+        ("1440x1080", "4/3", (480, 270)),
+    ],
+)
+def test_a_source_with_wide_pixels_keeps_its_shown_shape(tmp_path, size, sar, expected):
+    src = _video(tmp_path / "in.mp4", size, sar=sar)
+    out = tmp_path / "out.gif"
+    subprocess.run(gifmod.build_cmd(src, out, 0.0, 1.0, pre="", side=480), check=True)
+    assert gifmod.size(out) == expected
+
+
 @needs_ffmpeg
 @pytest.mark.parametrize(
     ("size", "side", "expected"),