diff --git a/backend/app/downloads/edit.py b/backend/app/downloads/edit.py index 2ce485b1..c0e51e90 100644 --- a/backend/app/downloads/edit.py +++ b/backend/app/downloads/edit.py @@ -60,6 +60,31 @@ class EditAborted(Exception): _MAX_SEGMENTS = 100 +def _crop_frame(crop: dict) -> dict | None: + """The picture size a crop was measured on, or None when it has none it could be scaled by (a + spec from before it was sent, which `crop_filter` cuts in the file's own pixels).""" + try: + w, h = int(crop["frame_w"]), int(crop["frame_h"]) + # silent-ok: no frame is a crop's older form, and it is still the crop that was asked for. + except (KeyError, TypeError, ValueError): + return None + return {"frame_w": w, "frame_h": h} if w > 0 and h > 0 else None + + +def crop_filter(crop: dict) -> str: + """The ffmpeg `crop` of an edit. Its numbers count the picture as the browser reports it, which + is its SHOWN size: a video with wide pixels (320 stored, 640 shown) would be cut off to one side + in ffmpeg's stored ones, so they are scaled to `iw` / `ih` by the size they were measured on. A + rotated file needs nothing more: both the browser and ffmpeg's filters see it upright.""" + frame_w, frame_h = crop.get("frame_w"), crop.get("frame_h") + if not frame_w or not frame_h: + return f"crop={crop['w']}:{crop['h']}:{crop['x']}:{crop['y']}" + return ( + f"crop=iw*{crop['w']}/{frame_w}:ih*{crop['h']}/{frame_h}" + f":iw*{crop['x']}/{frame_w}:ih*{crop['y']}/{frame_h}" + ) + + def normalize_edit_spec(spec: dict | None) -> dict: """Canonicalize a raw edit spec: clamp/round trim, coerce crop to ints, drop empties. @@ -81,7 +106,8 @@ def normalize_edit_spec(spec: dict | None) -> dict: ) c = None if c and c["w"] > 0 and c["h"] > 0 and c["x"] >= 0 and c["y"] >= 0: - out["crop"] = c + frame = _crop_frame(crop) + out["crop"] = {**c, **frame} if frame else c # Multi-segment JOIN (cut-list → one concatenated file). Takes precedence over `trim`. segs = spec.get("segments") @@ -268,7 +294,7 @@ def build_edit_cmd(src: Path, dest: Path, spec: dict, out_ext: str) -> list[str] if reencode: # Input -ss + re-encode is frame-accurate (decodes from the prior keyframe, discards up to # the exact cut). A crop adds the filter; an accurate trim only evens the frame. - cropf = f"crop={crop['w']}:{crop['h']}:{crop['x']}:{crop['y']}," if crop else "" + cropf = f"{crop_filter(crop)}," if crop else "" cmd += ["-vf", cropf + EVEN_FRAME, *X264_ARGS, *AAC_ARGS] else: cmd += ["-c", "copy", "-avoid_negative_ts", "make_zero"] @@ -313,7 +339,7 @@ def build_concat_plan(src: Path, dest: Path, spec: dict, out_ext: str, staging: ] if needs_reencode(spec): - cropf = f",crop={crop['w']}:{crop['h']}:{crop['x']}:{crop['y']}" if crop else "" + cropf = f",{crop_filter(crop)}" if crop else "" parts, labels = [], [] for i, s in enumerate(segs): st, en = s["start_s"], s["end_s"] diff --git a/backend/app/downloads/quota.py b/backend/app/downloads/quota.py index 179b3b92..3843613d 100644 --- a/backend/app/downloads/quota.py +++ b/backend/app/downloads/quota.py @@ -16,7 +16,8 @@ That is a per-job ceiling, not an aggregate ledger: concurrent jobs of one user remainder (in-flight bytes are not `ready`, so `footprint` cannot see them), so a wave of max_concurrent downloads can still land over the cap — the deliberate trade, since a download's size is unknown until it finishes. CLIP jobs consult it before their fetch or encode and again with -their output's real size before it is placed. Edit jobs do not consult it at all (tracked as SEC-19). +their output's real size before it is placed. Edit jobs consult it with their output's real size +before it is placed (`test_edit_cap.py`). """ from dataclasses import dataclass diff --git a/backend/tests/test_edit_crop_frame.py b/backend/tests/test_edit_crop_frame.py new file mode 100644 index 00000000..98b151b4 --- /dev/null +++ b/backend/tests/test_edit_crop_frame.py @@ -0,0 +1,112 @@ +"""The editor's crop lands on the part of the picture the frame was drawn on, also when a video's +pixels are not square. The browser reports such a video by its shown size (320 stored, 640 shown), +and ffmpeg's `crop` counts stored pixels: measured on a real clip, a frame over the middle half kept +half the frame's red surroundings. So a crop carries the size it was measured on. + +Real ffmpeg: the pixels of the output are the oracle. +""" + +import shutil +import subprocess +from pathlib import Path + +import pytest + +from app.config import settings +from app.downloads import edit, pipeline, service, storage +from app.downloads import upload as uploadmod +from app.downloads.models import DownloadJob, MediaAsset + +pytestmark = pytest.mark.skipif(shutil.which("ffmpeg") is None, reason="ffmpeg not available") + + +def _source(path: Path, size: str, box: str, sar: int) -> None: + """Red, with the picture's middle half green: a crop of that middle holds no red.""" + subprocess.run( + [ + "ffmpeg", "-y", "-loglevel", "error", + "-f", "lavfi", "-i", f"color=c=red:s={size}:d=2:r=10", + "-f", "lavfi", "-i", "sine=duration=2", + "-vf", f"drawbox={box}:c=lime:t=fill,setsar={sar}", + "-c:v", "libx264", "-pix_fmt", "yuv420p", "-c:a", "aac", str(path), + ], + check=True, + ) # fmt: skip + + +def _red_and_green(path: Path) -> tuple[int, int]: + raw = subprocess.run( + ["ffmpeg", "-loglevel", "error", "-i", str(path), "-frames:v", "1", + "-f", "rawvideo", "-pix_fmt", "rgb24", "-"], + check=True, + capture_output=True, + ).stdout # fmt: skip + red = green = 0 + for i in range(0, len(raw) - 2, 3): + if raw[i] > 180 and raw[i + 1] < 80: + red += 1 + elif raw[i + 1] > 180 and raw[i] < 80: + green += 1 + return red, green + + +def _cut(db, make_user, tmp_path, src: Path, crop: dict) -> Path: + user = make_user() + source = uploadmod.ingest_upload( + db, user.id, filename="clip.mp4", src_path=src, size=src.stat().st_size + ) + assert source.status == "done" + job = service.enqueue_edit(db, user.id, source, {"crop": crop}) + asset = db.get(MediaAsset, job.asset_id) + job.status, asset.status = "running", "downloading" # the worker's claim + db.commit() + + pipeline._process_edit(job.id, asset.id) + + db.expire_all() + job, asset = db.get(DownloadJob, job.id), db.get(MediaAsset, job.asset_id) + assert (job.status, job.error, asset.status) == ("done", None, "ready") + return storage.abs_path(str(tmp_path), asset.rel_path) + + +def test_a_crop_of_a_video_with_wide_pixels_keeps_what_the_frame_showed( + db, make_user, tmp_path, monkeypatch +): + monkeypatch.setattr(settings, "files_root", str(tmp_path)) + src = tmp_path / "in.mp4" + _source(src, "320x240", "x=80:y=60:w=160:h=120", sar=2) + + # What the editor sends for the middle half of this video: the browser shows it at 640 x 240. + final = _cut( + db, make_user, tmp_path, src, + {"x": 160, "y": 60, "w": 320, "h": 120, "frame_w": 640, "frame_h": 240}, + ) # fmt: skip + + red, green = _red_and_green(final) + assert (red, green) == (0, 160 * 120) + + +def test_a_crop_of_square_pixels_is_cut_where_it_was_drawn(db, make_user, tmp_path, monkeypatch): + monkeypatch.setattr(settings, "files_root", str(tmp_path)) + src = tmp_path / "in.mp4" + _source(src, "480x640", "x=120:y=160:w=240:h=320", sar=1) + + final = _cut( + db, make_user, tmp_path, src, + {"x": 120, "y": 160, "w": 240, "h": 320, "frame_w": 480, "frame_h": 640}, + ) # fmt: skip + + assert _red_and_green(final) == (0, 240 * 320) + + +def test_a_crop_stored_before_it_carried_its_frame_is_cut_as_it_always_was(): + spec = edit.normalize_edit_spec({"crop": {"x": 4, "y": 6, "w": 100, "h": 50}}) + assert spec["crop"] == {"x": 4, "y": 6, "w": 100, "h": 50} + cmd = edit.build_edit_cmd(Path("in.mp4"), Path("out.mp4"), spec, "mp4") + assert cmd[cmd.index("-vf") + 1].startswith("crop=100:50:4:6,") + + +@pytest.mark.parametrize("frame", [{"frame_w": 0, "frame_h": 240}, {"frame_w": "x", "frame_h": 2}]) +def test_a_frame_that_cannot_be_read_is_not_divided_by(frame): + spec = edit.normalize_edit_spec({"crop": {"x": 4, "y": 6, "w": 100, "h": 50, **frame}}) + assert spec["crop"] == {"x": 4, "y": 6, "w": 100, "h": 50} diff --git a/frontend/src/lib/api/downloads.ts b/frontend/src/lib/api/downloads.ts index c959c63d..acc06f26 100644 --- a/frontend/src/lib/api/downloads.ts +++ b/frontend/src/lib/api/downloads.ts @@ -70,7 +70,9 @@ export interface ClipRequest { export interface EditSpec { trim?: { start_s: number; end_s?: number }; segments?: { start_s: number; end_s: number }[]; - crop?: { x: number; y: number; w: number; h: number }; + // In the picture's size as the browser reports it, which `frame_w` x `frame_h` says: the server + // scales them to the file's own pixels (a video's stored and shown sizes can differ). + crop?: { x: number; y: number; w: number; h: number; frame_w?: number; frame_h?: number }; accurate?: boolean; } diff --git a/frontend/src/modules/files/VideoEditor.crop.dom.test.tsx b/frontend/src/modules/files/VideoEditor.crop.dom.test.tsx new file mode 100644 index 00000000..5c8d86cf --- /dev/null +++ b/frontend/src/modules/files/VideoEditor.crop.dom.test.tsx @@ -0,0 +1,56 @@ +// @vitest-environment jsdom +// +// A crop's numbers count the picture as the browser reports it, its shown size, which differs from the +// file's own on a video with wide pixels. The size goes along, or the server cuts the wrong part. +import { describe, it, expect, vi, beforeAll, afterEach } from "vitest"; +import { render, screen, fireEvent, waitFor, cleanup } from "@testing-library/react"; +import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; + +vi.mock("react-i18next", async (importOriginal) => ({ + ...(await importOriginal()), + useTranslation: () => ({ t: (key: string) => key, i18n: { language: "en" } }), +})); + +import VideoEditor from "./VideoEditor"; +import { api, type DownloadJob } from "../../lib/api"; + +beforeAll(() => { + class Stub { + observe() {} + unobserve() {} + disconnect() {} + takeRecords() { + return []; + } + } + globalThis.ResizeObserver ??= Stub as unknown as typeof ResizeObserver; + globalThis.IntersectionObserver ??= Stub as unknown as typeof IntersectionObserver; +}); +afterEach(() => { + cleanup(); + vi.restoreAllMocks(); +}); + +const job = { + id: 7, + display_name: "Talk", + asset: { duration_s: 100, width: 1920, height: 1080, title: "Talk" }, +} as unknown as DownloadJob; + +describe("VideoEditor crop", () => { + it("sends the size the crop was measured on with it", async () => { + const enqueue = vi + .spyOn(api, "enqueueEdit") + .mockResolvedValue({ id: 8 } as unknown as DownloadJob); + render( + + {}} /> + , + ); + fireEvent.click(screen.getByRole("button", { name: "editor.cropOff" })); + fireEvent.click(screen.getByRole("button", { name: "editor.create" })); + await waitFor(() => expect(enqueue).toHaveBeenCalledTimes(1)); + const crop = enqueue.mock.calls[0]?.[0].edit_spec.crop; + expect(crop).toMatchObject({ frame_w: 1920, frame_h: 1080, w: 1536, h: 864 }); + }); +}); diff --git a/frontend/src/modules/files/VideoEditor.tsx b/frontend/src/modules/files/VideoEditor.tsx index 0063e9ca..f2a2e016 100644 --- a/frontend/src/modules/files/VideoEditor.tsx +++ b/frontend/src/modules/files/VideoEditor.tsx @@ -226,14 +226,16 @@ export default function VideoEditor({ job, onClose }: { job: DownloadJob; onClos const willJoin = output === "join" && kept.length > 1; const reencode = cropOn || accurate; - const cropPx = (): { x: number; y: number; w: number; h: number } | undefined => { + const cropPx = (): EditSpec["crop"] => { const v = videoRef.current; const vw = v?.videoWidth || vdim.w; const vh = v?.videoHeight || vdim.h; if (!cropOn) return undefined; // crop fractions are relative to the picture (the overlay is anchored to it), so this is a // straight fraction→source-px scale; see cropGeometry for the even-rounding + in-bounds clamp. - return cropToSourcePx(crop, vw, vh) ?? undefined; + const px = cropToSourcePx(crop, vw, vh); + // The size they count in goes along: it is the shown one, and the file's own can differ. + return px ? { ...px, frame_w: vw, frame_h: vh } : undefined; }; const create = useMutation({