From 8845d1aa491e92bbdfe364a409f272641d480023 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Thu, 12 Nov 2026 23:24:25 +0100 Subject: [PATCH] =?UTF-8?q?fix(downloads):=20GIF=20review=20round=201=20?= =?UTF-8?q?=E2=80=94=20shape,=20HDR,=20caps,=20audio,=20copy?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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 | 12 +- backend/app/downloads/gif.py | 19 +++- backend/app/downloads/pipeline.py | 17 ++- backend/app/downloads/service.py | 13 ++- backend/tests/test_clip.py | 37 +++++++ backend/tests/test_edit_gif.py | 81 ++++++++++++++ backend/tests/test_gif.py | 34 +++++- .../features/player/ClipPanel.dom.test.tsx | 28 ++++- frontend/src/features/player/ClipPanel.tsx | 9 +- frontend/src/i18n/copyRule.test.ts | 7 ++ frontend/src/i18n/locales/en/clip.json | 8 +- frontend/src/i18n/locales/en/downloads.json | 1 + frontend/src/i18n/locales/en/editor.json | 3 +- frontend/src/i18n/locales/hu/clip.json | 8 +- frontend/src/i18n/locales/hu/downloads.json | 1 + frontend/src/i18n/locales/hu/editor.json | 1 + frontend/src/lib/gif.test.ts | 15 +++ frontend/src/lib/gif.ts | 5 +- .../files/VideoEditor.gif.dom.test.tsx | 47 +++++++- frontend/src/modules/files/VideoEditor.tsx | 103 +++++++++++------- frontend/src/watch/WatchPage.dom.test.tsx | 16 +++ frontend/src/watch/WatchPage.tsx | 9 +- frontend/src/watch/watchStrings.ts | 7 +- 23 files changed, 403 insertions(+), 78 deletions(-) diff --git a/backend/app/downloads/clip.py b/backend/app/downloads/clip.py index 3b6ff2f9..99d8385f 100644 --- a/backend/app/downloads/clip.py +++ b/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 diff --git a/backend/app/downloads/gif.py b/backend/app/downloads/gif.py index c95f771e..be0c678f 100644 --- a/backend/app/downloads/gif.py +++ b/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];" diff --git a/backend/app/downloads/pipeline.py b/backend/app/downloads/pipeline.py index 2ed3e42f..d4466c45 100644 --- a/backend/app/downloads/pipeline.py +++ b/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) diff --git a/backend/app/downloads/service.py b/backend/app/downloads/service.py index 8641c6ae..41456cde 100644 --- a/backend/app/downloads/service.py +++ b/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.`, 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) diff --git a/backend/tests/test_clip.py b/backend/tests/test_clip.py index a91a38aa..e89fccb8 100644 --- a/backend/tests/test_clip.py +++ b/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 ): diff --git a/backend/tests/test_edit_gif.py b/backend/tests/test_edit_gif.py index 8c31a078..9ef0193a 100644 --- a/backend/tests/test_edit_gif.py +++ b/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 diff --git a/backend/tests/test_gif.py b/backend/tests/test_gif.py index 3af189b0..2538fb57 100644 --- a/backend/tests/test_gif.py +++ b/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"), diff --git a/frontend/src/features/player/ClipPanel.dom.test.tsx b/frontend/src/features/player/ClipPanel.dom.test.tsx index 67db855c..a1d0bd33 100644 --- a/frontend/src/features/player/ClipPanel.dom.test.tsx +++ b/frontend/src/features/player/ClipPanel.dom.test.tsx @@ -14,7 +14,11 @@ vi.mock("react-i18next", async (importOriginal) => { const actual = await importOriginal(); return { ...actual, - useTranslation: () => ({ t: (k: string) => k, i18n: { language: "en" } }), + // A size option reads as its own words with its unit ("480 px"); every other text is its key. + useTranslation: () => ({ + t: (k: string, o?: { n?: string }) => (k === "clip.gif.side" ? `${o?.n} px` : k), + i18n: { language: "en" }, + }), }; }); @@ -281,7 +285,7 @@ describe("ClipPanel GIF", () => { render(); expect(screen.queryByRole("group", { name: "clip.gif.size" })).toBeNull(); fireEvent.click(screen.getByRole("button", { name: "clip.formatGif" })); - fireEvent.click(screen.getByRole("button", { name: "320" })); + fireEvent.click(screen.getByRole("button", { name: "320 px" })); expect(enqueueClip).not.toHaveBeenCalled(); fireEvent.click(screen.getByRole("button", { name: "clip.gif.make" })); await waitFor(() => expect(onClose).toHaveBeenCalled()); @@ -298,7 +302,9 @@ describe("ClipPanel GIF", () => { it("starts at the default size, and a video again sends no GIF", async () => { render(); fireEvent.click(screen.getByRole("button", { name: "clip.formatGif" })); - expect(screen.getByRole("button", { name: "480" }).getAttribute("aria-pressed")).toBe("true"); + expect(screen.getByRole("button", { name: "480 px" }).getAttribute("aria-pressed")).toBe( + "true", + ); fireEvent.click(screen.getByRole("button", { name: "clip.formatVideo" })); fireEvent.click(screen.getByRole("button", { name: "clip.cut" })); await waitFor(() => expect(enqueueClip).toHaveBeenCalled()); @@ -333,7 +339,21 @@ describe("ClipPanel GIF", () => { expect(range()).toBe("100-120"); expect(note()).toBe("clip.gif.noPreview"); // 20 s at 640: up to about 32 MB, past the server's 30. - fireEvent.click(screen.getByRole("button", { name: "640" })); + fireEvent.click(screen.getByRole("button", { name: "640 px" })); expect(note()).toBe("clip.gif.mayFail"); }); + + it("says one size, not a range of one, when both ends read the same", () => { + // 1 s at 480: 0.4–1.0 MB, both shown as 1. + render(); + fireEvent.click(screen.getByRole("button", { name: "clip.formatGif" })); + expect(screen.getByText("clip.gif.estimateOne")).toBeTruthy(); + expect(screen.queryByText("clip.gif.estimate")).toBeNull(); + // 10 s: a range. + fireEvent.change(screen.getByRole("textbox", { name: "clip.out" }), { + target: { value: "1:50" }, + }); + fireEvent.keyDown(screen.getByRole("textbox", { name: "clip.out" }), { key: "Enter" }); + expect(screen.getByText("clip.gif.estimate")).toBeTruthy(); + }); }); diff --git a/frontend/src/features/player/ClipPanel.tsx b/frontend/src/features/player/ClipPanel.tsx index deb60b6d..6fbe1eaf 100644 --- a/frontend/src/features/player/ClipPanel.tsx +++ b/frontend/src/features/player/ClipPanel.tsx @@ -10,6 +10,7 @@ import { notifyActionError } from "../../lib/actionErrors"; import { showInTray } from "../../lib/downloadQueue"; import { log } from "../../lib/log"; import { formatTimecode } from "../../lib/timecode"; +import { formatNumber } from "../../lib/format"; import { estimateGif, gifEstimateText, @@ -146,6 +147,7 @@ export default function ClipPanel({ const shape = crop ? (crop.rect.w * picture) / crop.rect.h : picture; const estimate = estimateGif(length, side, shape); const verdict = gif ? gifVerdict(length, estimate) : "ok"; + const estimateText = gifEstimateText(estimate, i18n.language); const blocked = verdict === "too_long"; // Opened from a button or a key, the panel takes the focus to its In handle: the button that @@ -421,10 +423,13 @@ export default function ClipPanel({ ariaLabel={t("clip.gif.size")} value={String(side)} onChange={(v) => setSide(Number(v) as GifSide)} - options={GIF_SIDES.map((s) => ({ value: String(s), label: String(s) }))} + options={GIF_SIDES.map((s) => ({ + value: String(s), + label: t("clip.gif.side", { n: formatNumber(s, i18n.language) }), + }))} /> - {t("clip.gif.estimate", gifEstimateText(estimate, i18n.language))} + {t(estimateText.single ? "clip.gif.estimateOne" : "clip.gif.estimate", estimateText)} )} diff --git a/frontend/src/i18n/copyRule.test.ts b/frontend/src/i18n/copyRule.test.ts index 6fc65876..7d6bee90 100644 --- a/frontend/src/i18n/copyRule.test.ts +++ b/frontend/src/i18n/copyRule.test.ts @@ -39,6 +39,13 @@ describe("the copy rule", () => { expect(offenders((v) => v.includes("✓"))).toEqual([]); }); + it("keeps the stops of a message of two sentences, the last one too", () => { + // A sentence break: a stop, a space, then a capital ("kb. {{n}}" is an abbreviation, not one). A + // value ending in a space is the head of a sentence a link or a number completes on screen. + const twoSentences = (v: string) => /[.!?] \p{Lu}/u.test(v) && !v.endsWith(" "); + expect(offenders((v) => twoSentences(v) && !/[.!?…]["”)]?$/u.test(v))).toEqual([]); + }); + it("spells no unit as a bare letter after a number ('90m', '24h', '{{count}}d', '10 s')", () => { expect(offenders((v) => /(\d|\}\})\s?(m|h|d|s)(?![\p{L}\d/])/u.test(v))).toEqual([]); }); diff --git a/frontend/src/i18n/locales/en/clip.json b/frontend/src/i18n/locales/en/clip.json index cc464421..3cb43d38 100644 --- a/frontend/src/i18n/locales/en/clip.json +++ b/frontend/src/i18n/locales/en/clip.json @@ -28,11 +28,13 @@ "gif": { "size": "GIF size, the longer side in pixels", "sizeLabel": "Size", + "side": "{{n}} px", "estimate": "about {{lo}}–{{hi}} MB", + "estimateOne": "about {{lo}} MB", "hint": "No sound, 12 frames a second", - "noPreview": "It may pass 15 MB, and then a Teams link preview shows it still. The downloaded file moves everywhere", - "mayFail": "It may pass 30 MB, and then it isn’t saved. A shorter range or a smaller size helps", - "tooLong": "A GIF is at most {{seconds}} seconds long. Shorten the range", + "noPreview": "It may pass 15 MB, and then a Teams link preview shows only a still picture. The downloaded file plays everywhere.", + "mayFail": "It may pass 30 MB, and then it isn’t saved. A shorter range or a smaller size helps.", + "tooLong": "A GIF is at most {{seconds}} seconds long. Shorten the range.", "make": "Make GIF", "queued": "The GIF is being made — it will be in your downloads", "failed": "Couldn’t make the GIF" diff --git a/frontend/src/i18n/locales/en/downloads.json b/frontend/src/i18n/locales/en/downloads.json index 86de5e6f..f02dd883 100644 --- a/frontend/src/i18n/locales/en/downloads.json +++ b/frontend/src/i18n/locales/en/downloads.json @@ -227,6 +227,7 @@ "bad_gif": "That GIF size isn’t available", "gif_range": "A GIF is made from one range with a start and an end", "gif_too_long": "A GIF is at most {{seconds}} seconds long", + "gif_not_video": "A GIF is made from a video", "generic": "Couldn't create that edit" }, "upload": { diff --git a/frontend/src/i18n/locales/en/editor.json b/frontend/src/i18n/locales/en/editor.json index 34cb6bbd..216108a4 100644 --- a/frontend/src/i18n/locales/en/editor.json +++ b/frontend/src/i18n/locales/en/editor.json @@ -37,10 +37,11 @@ "video": "Video", "gif": "GIF", "estimate": "about {{lo}}–{{hi}} MB in all", + "estimateOne": "about {{lo}} MB in all", "reencode": "A GIF is always re-encoded: no sound, 12 frames a second", "segTooLong": "Segment {{n}} is {{length}} long: a GIF is at most {{seconds}} seconds", "segMayFail": "Segment {{n}} may pass 30 MB as a GIF, and then it isn’t saved", - "segNoPreview": "Segment {{n}} may pass 15 MB as a GIF, and then a Teams link preview shows it still" + "segNoPreview": "Segment {{n}} may pass 15 MB as a GIF, and then a Teams link preview shows only a still picture" }, "createGif_one": "Create {{count}} GIF", "createGif_other": "Create {{count}} GIFs", diff --git a/frontend/src/i18n/locales/hu/clip.json b/frontend/src/i18n/locales/hu/clip.json index 99c6177d..7e78295c 100644 --- a/frontend/src/i18n/locales/hu/clip.json +++ b/frontend/src/i18n/locales/hu/clip.json @@ -28,11 +28,13 @@ "gif": { "size": "A GIF mérete, a hosszabbik oldal pixelben", "sizeLabel": "Méret", + "side": "{{n}} px", "estimate": "kb. {{lo}}–{{hi}} MB", + "estimateOne": "kb. {{lo}} MB", "hint": "Hang nélkül, 12 kép másodpercenként", - "noPreview": "15 MB fölé mehet, és akkor a Teams link-előnézetben állókép lesz. A letöltött fájl mindenhol mozog", - "mayFail": "30 MB fölé mehet, és akkor nem készül el. Rövidebb rész vagy kisebb méret segít", - "tooLong": "A GIF legfeljebb {{seconds}} másodperc lehet. Rövidítsd a kijelölést", + "noPreview": "15 MB fölé mehet, és akkor a Teams link-előnézetben állókép lesz. A letöltött fájl mindenhol mozog.", + "mayFail": "30 MB fölé mehet, és akkor nem készül el. Rövidebb rész vagy kisebb méret segít.", + "tooLong": "A GIF legfeljebb {{seconds}} másodperc lehet. Rövidítsd a kijelölést.", "make": "GIF készítése", "queued": "Készül a GIF — a letöltéseid közé kerül", "failed": "Nem sikerült elkészíteni a GIF-et" diff --git a/frontend/src/i18n/locales/hu/downloads.json b/frontend/src/i18n/locales/hu/downloads.json index 6c93efd8..82250945 100644 --- a/frontend/src/i18n/locales/hu/downloads.json +++ b/frontend/src/i18n/locales/hu/downloads.json @@ -227,6 +227,7 @@ "bad_gif": "Ez a GIF-méret nem választható", "gif_range": "GIF egyetlen szakaszból készül, aminek eleje és vége is van", "gif_too_long": "A GIF legfeljebb {{seconds}} másodperc lehet", + "gif_not_video": "GIF csak videóból készülhet", "generic": "Nem sikerült létrehozni a vágást" }, "upload": { diff --git a/frontend/src/i18n/locales/hu/editor.json b/frontend/src/i18n/locales/hu/editor.json index e5df8783..bf517dd5 100644 --- a/frontend/src/i18n/locales/hu/editor.json +++ b/frontend/src/i18n/locales/hu/editor.json @@ -37,6 +37,7 @@ "video": "Videó", "gif": "GIF", "estimate": "összesen kb. {{lo}}–{{hi}} MB", + "estimateOne": "összesen kb. {{lo}} MB", "reencode": "A GIF mindig újrakódol: hang nélkül, 12 kép másodpercenként", "segTooLong": "{{n}}. szegmens: {{length}} hosszú, de a GIF legfeljebb {{seconds}} másodperc lehet", "segMayFail": "{{n}}. szegmens: GIF-ként 30 MB fölé mehet, és akkor nem készül el", diff --git a/frontend/src/lib/gif.test.ts b/frontend/src/lib/gif.test.ts index 6f65f9f2..35367f18 100644 --- a/frontend/src/lib/gif.test.ts +++ b/frontend/src/lib/gif.test.ts @@ -33,6 +33,21 @@ describe("estimateGif", () => { }); }); +describe("gifEstimateText", () => { + it("says whether both ends read the same, so a panel says one size", () => { + expect(gifEstimateText(estimateGif(1, 480, 16 / 9), "en")).toEqual({ + lo: "1", + hi: "1", + single: true, + }); + expect(gifEstimateText(estimateGif(10, 480, 16 / 9), "en")).toEqual({ + lo: "4", + hi: "10", + single: false, + }); + }); +}); + describe("gifVerdict", () => { it("refuses past the length cap, whatever the size", () => { expect(gifVerdict(30.2, { hi: 1 })).toBe("too_long"); diff --git a/frontend/src/lib/gif.ts b/frontend/src/lib/gif.ts index 2c55bafe..9c4edffa 100644 --- a/frontend/src/lib/gif.ts +++ b/frontend/src/lib/gif.ts @@ -41,11 +41,12 @@ export function estimateGif( } /** An estimate's ends as whole megabytes in the reader's number format (at least 1, as a GIF of a - * second or two still takes one in the reader's eyes). */ + * second or two still takes one in the reader's eyes). `single` when both ends read the same: the + * panels then say one number ("about 1 MB"), not a range of one ("about 1–1 MB"). */ export function gifEstimateText(est: { lo: number; hi: number }, lang: string) { const lo = Math.max(1, Math.round(est.lo / MB)); const hi = Math.max(lo, Math.round(est.hi / MB)); - return { lo: formatNumber(lo, lang), hi: formatNumber(hi, lang) }; + return { lo: formatNumber(lo, lang), hi: formatNumber(hi, lang), single: lo === hi }; } /** What the panels say about a GIF before it is made: too long (refused, it does not start), it may diff --git a/frontend/src/modules/files/VideoEditor.gif.dom.test.tsx b/frontend/src/modules/files/VideoEditor.gif.dom.test.tsx index bd158db5..cd0246cd 100644 --- a/frontend/src/modules/files/VideoEditor.gif.dom.test.tsx +++ b/frontend/src/modules/files/VideoEditor.gif.dom.test.tsx @@ -9,7 +9,11 @@ import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; vi.mock("react-i18next", async (importOriginal) => ({ ...(await importOriginal()), - useTranslation: () => ({ t: (key: string) => key, i18n: { language: "en" } }), + // A size option reads as its own words with its unit ("480 px"); every other text is its key. + useTranslation: () => ({ + t: (key: string, o?: { n?: string }) => (key === "clip.gif.side" ? `${o?.n} px` : key), + i18n: { language: "en" }, + }), })); const { notify } = vi.hoisted(() => ({ notify: vi.fn() })); vi.mock("../../lib/notifications", async (importOriginal) => ({ @@ -39,20 +43,20 @@ afterEach(() => { notify.mockReset(); }); -const jobOf = (duration: number) => +const jobOf = (duration: number, kind = "video") => ({ id: 7, display_name: "Talk", - asset: { duration_s: duration, width: 1920, height: 1080, title: "Talk" }, + asset: { duration_s: duration, width: 1920, height: 1080, title: "Talk", media_kind: kind }, }) as unknown as DownloadJob; -function open(duration: number) { +function open(duration: number, kind = "video") { vi.spyOn(api, "downloadStoryboard").mockResolvedValue({ available: false } as never); const enqueue = vi.spyOn(api, "enqueueEdit").mockResolvedValue({} as never); const onClose = vi.fn(); render( - + , ); const video = document.querySelector("video")!; @@ -67,7 +71,7 @@ describe("VideoEditor — GIF", () => { it("makes a whole short video a GIF at the size picked, with no precise/fast choice", async () => { const { enqueue, onClose } = open(20); fireEvent.click(screen.getByRole("button", { name: "editor.format.gif" })); - fireEvent.click(screen.getByRole("button", { name: "640" })); + fireEvent.click(screen.getByRole("button", { name: "640 px" })); expect(screen.queryByText("editor.accurate.label")).toBeNull(); expect(screen.getByText("editor.format.reencode")).toBeTruthy(); fireEvent.click(screen.getByRole("button", { name: "editor.createGif" })); @@ -116,6 +120,37 @@ describe("VideoEditor — GIF", () => { ).toBe(null); }); + it("offers no GIF for an audio file, which has no picture to make one of", () => { + open(20, "audio"); + expect(screen.queryByRole("button", { name: "editor.format.gif" })).toBeNull(); + expect(screen.getByRole("button", { name: "editor.create" })).toBeTruthy(); + }); + + it("keeps its notes' live region in place, so the first note is read out", () => { + open(20); + fireEvent.click(screen.getByRole("button", { name: "editor.format.gif" })); + // 20 s at 320: about 5–9 MB, nothing to say — but the region is already there. + fireEvent.click(screen.getByRole("button", { name: "320 px" })); + const region = document.querySelector("div[aria-live='polite']")!; + expect(region).toBeTruthy(); + expect(region.textContent).toBe(""); + // 20 s at 640: up to about 32 MB, said inside the same region. + fireEvent.click(screen.getByRole("button", { name: "640 px" })); + expect(document.querySelector("div[aria-live='polite']")).toBe(region); + expect(region.textContent).toBe("editor.format.segMayFail"); + }); + + it("says one size for a short GIF, and none when nothing is kept", () => { + open(2); + fireEvent.click(screen.getByRole("button", { name: "editor.format.gif" })); + fireEvent.click(screen.getByRole("button", { name: "320 px" })); + // 2 s at 320: 0.46–0.94 MB, both shown as 1. + expect(screen.getByText("editor.format.estimateOne")).toBeTruthy(); + fireEvent.click(screen.getByRole("button", { name: "editor.kept" })); + expect(screen.queryByText("editor.format.estimateOne")).toBeNull(); + expect(screen.queryByText("editor.format.estimate")).toBeNull(); + }); + it("Join needs two kept segments, and says so instead of going dead", () => { open(20); const join = screen.getByRole("button", { name: "editor.output.join" }); diff --git a/frontend/src/modules/files/VideoEditor.tsx b/frontend/src/modules/files/VideoEditor.tsx index bc13b438..b03c61c5 100644 --- a/frontend/src/modules/files/VideoEditor.tsx +++ b/frontend/src/modules/files/VideoEditor.tsx @@ -23,6 +23,7 @@ import { SURFACE } from "../../components/ui/surfaces"; import { RangeTrack } from "../../components/ui/RangeTrack"; import { trackLeft } from "../../components/ui/trackPosition"; import { formatTimecode } from "../../lib/timecode"; +import { formatNumber } from "../../lib/format"; import { estimateGif, gifEstimateText, @@ -95,7 +96,10 @@ export default function VideoEditor({ job, onClose }: { job: DownloadJob; onClos const [accurate, setAccurate] = useState(true); const [format, setFormat] = useState<"video" | "gif">("video"); const [side, setSide] = useState(GIF_DEFAULT_SIDE); - const gif = format === "gif"; + // A GIF is made of the picture: an audio file has none (or only its cover), so it is offered for + // a video alone, as the server holds it. + const canGif = job.asset?.media_kind === "video"; + const gif = canGif && format === "gif"; const gifNoteId = useId(); const [name, setName] = useState(""); @@ -268,6 +272,7 @@ export default function VideoEditor({ job, onClose }: { job: DownloadJob; onClos lo: 0, hi: 0, }); + const gifTotalText = gifEstimateText(gifTotal, i18n.language); const gifNotes = GIF_NOTE_ORDER.flatMap((v) => gifSegs.filter((g) => g.verdict === v)); const gifBlocked = gif && gifSegs.some((g) => g.verdict === "too_long"); const joinReason = gif @@ -625,46 +630,62 @@ export default function VideoEditor({ job, onClose }: { job: DownloadJob; onClos {/* format: a video, or each kept segment a GIF */} -
- {t("editor.format.label")}: - - {gif && ( - <> - - setSide(Number(v) as GifSide)} - options={GIF_SIDES.map((s) => ({ value: String(s), label: String(s) }))} - /> - - {t("editor.format.estimate", gifEstimateText(gifTotal, i18n.language))} - - - )} -
- {gif && gifNotes.length > 0 && ( -
    - {gifNotes.map((g) => ( -
  • - {t(GIF_NOTE_KEY[g.verdict as keyof typeof GIF_NOTE_KEY], { - n: g.n, - length: tc(g.length), - seconds: GIF_MAX_S, - })} -
    • - ))} -
+ {canGif && ( +
+ {t("editor.format.label")}: + + {gif && ( + <> + + setSide(Number(v) as GifSide)} + options={GIF_SIDES.map((s) => ({ + value: String(s), + label: t("clip.gif.side", { n: formatNumber(s, i18n.language) }), + }))} + /> + {kept.length > 0 && ( + + {t( + gifTotalText.single ? "editor.format.estimateOne" : "editor.format.estimate", + gifTotalText, + )} + + )} + + )} +
+ )} + {/* Mounted for as long as GIF is chosen: a live region announces what changes inside it, + not what arrives together with it, and the first note is the one that counts. */} + {gif && ( +
+ {gifNotes.length > 0 && ( +
    + {gifNotes.map((g) => ( +
  • + {t(GIF_NOTE_KEY[g.verdict as keyof typeof GIF_NOTE_KEY], { + n: g.n, + length: tc(g.length), + seconds: GIF_MAX_S, + })} +
    • + ))} +
+ )} +
)} {/* crop + precision */} diff --git a/frontend/src/watch/WatchPage.dom.test.tsx b/frontend/src/watch/WatchPage.dom.test.tsx index 847844a9..253280ab 100644 --- a/frontend/src/watch/WatchPage.dom.test.tsx +++ b/frontend/src/watch/WatchPage.dom.test.tsx @@ -184,6 +184,22 @@ describe("WatchPage media element", () => { expect(page.getByText(/drag the downloaded file in/)).toBeTruthy(); }); + it("a picture that does not load says so, rather than looking like a file without one", async () => { + window.history.replaceState({}, "", "/watch/tok-gif-gone"); + mockMeta({ + needs_password: false, + media_kind: "file", + preview: "image", + title: "Gone", + container: "gif", + allow_download: true, + file_url: "/gone.gif", + }); + const page = render(); + fireEvent.error(await page.findByRole("img", { name: "Gone" })); + expect((await page.findByRole("status")).textContent).toBe("Couldn't load the picture."); + }); + it("a view-only picture link shows the picture, without Download or the chat tip", async () => { window.history.replaceState({}, "", "/watch/tok-gif-view"); mockMeta({ diff --git a/frontend/src/watch/WatchPage.tsx b/frontend/src/watch/WatchPage.tsx index 5a5f2f0f..960f3a7e 100644 --- a/frontend/src/watch/WatchPage.tsx +++ b/frontend/src/watch/WatchPage.tsx @@ -488,7 +488,14 @@ function SharedPicture({ meta, nested }: { meta: FileMeta; nested: boolean }) { width={meta.width ?? undefined} height={meta.height ?? undefined} retries={1} - fallback={} + // A picture link always has a picture: a missing one failed to load, and says so — + // the bare file glyph would read as a file with no preview. + fallback={ +
+ +

{T.pictureFailed}

+
+ } className={clsx( "max-w-full h-auto w-auto mx-auto block", nested ? "max-h-[60dvh]" : "max-h-[80dvh]", diff --git a/frontend/src/watch/watchStrings.ts b/frontend/src/watch/watchStrings.ts index b4988aa6..ca3f0e76 100644 --- a/frontend/src/watch/watchStrings.ts +++ b/frontend/src/watch/watchStrings.ts @@ -31,7 +31,8 @@ const STR = { open: "Open", folderEmpty: "This folder is empty.", play: "Play", - gifTip: "To share it in a chat, drag the downloaded file in: it moves everywhere.", + gifTip: "To share it in a chat, drag the downloaded file in: it plays everywhere.", + pictureFailed: "Couldn't load the picture.", files: (n: number) => (n === 1 ? "1 file" : `${n} files`), }, hu: { @@ -63,6 +64,7 @@ const STR = { folderEmpty: "Ez a mappa üres.", play: "Lejátszás", gifTip: "Csetbe a letöltött fájlt húzd be: az mindenhol mozog.", + pictureFailed: "Nem sikerült betölteni a képet.", files: (n: number) => `${n} fájl`, }, de: { @@ -94,7 +96,8 @@ const STR = { folderEmpty: "Dieser Ordner ist leer.", play: "Abspielen", gifTip: - "Zum Teilen in einem Chat die heruntergeladene Datei hineinziehen: Sie bewegt sich überall.", + "Zum Teilen in einem Chat die heruntergeladene Datei hineinziehen: Sie läuft überall als Animation.", + pictureFailed: "Das Bild konnte nicht geladen werden.", files: (n: number) => (n === 1 ? "1 Datei" : `${n} Dateien`), }, } as const;