From fb2c843fa965353c4e07a66e033116d19d3279e3 Mon Sep 17 00:00:00 2001
From: npeter83 <npeter83+claude@gmail.com>
Date: Sat, 29 Aug 2026 05:43:41 +0200
Subject: [PATCH] fix(downloads): anchor crop overlay to the letterboxed
 picture (E7 S2)

The video preview is object-fit: contain (the browser default for <video>), so a
non-16:9 source is letter/pillarboxed inside the element box. The crop marquee was
drawn in fractions of the whole box (bars included) and those fractions were then
multiplied by the source resolution, so the emitted ffmpeg crop diverged from what
the selector showed. Anchor the overlay to the computed content rect (and set
object-contain explicitly) so a marquee fraction is a fraction of the real picture;
extract the display-px<->source-px math into cropGeometry.ts with unit tests, and
clamp the even-rounded window in-bounds so a full-frame crop can't overflow.
---
 frontend/src/components/VideoEditor.tsx | 55 ++++++++++++----
 frontend/src/lib/cropGeometry.test.ts   | 88 +++++++++++++++++++++++++
 frontend/src/lib/cropGeometry.ts        | 48 ++++++++++++++
 3 files changed, 177 insertions(+), 14 deletions(-)
 create mode 100644 frontend/src/lib/cropGeometry.test.ts
 create mode 100644 frontend/src/lib/cropGeometry.ts

diff --git a/frontend/src/components/VideoEditor.tsx b/frontend/src/components/VideoEditor.tsx
index bab983f6..042956c9 100644
--- a/frontend/src/components/VideoEditor.tsx
+++ b/frontend/src/components/VideoEditor.tsx
@@ -7,6 +7,7 @@ import clsx from "clsx";
 import Modal from "./Modal";
 import { notify } from "../lib/notifications";
 import { api, type DownloadJob, type EditSpec } from "../lib/api";
+import { contentRect, cropToSourcePx, type Frac } from "../lib/cropGeometry";
 import { inputCls, btnCls } from "./ui/form";
 import { Button } from "./ui/Button";
 
@@ -31,7 +32,6 @@ function parseTc(v: string): number | null {
 }
 
 type Seg = { id: number; start: number; end: number; keep: boolean };
-type Frac = { x: number; y: number; w: number; h: number };
 type Drag = { type: "seek" } | { type: "boundary"; bi: number };
 
 const numCls =
@@ -51,6 +51,11 @@ export default function VideoEditor({ job, onClose }: { job: DownloadJob; onClos
   const [current, setCurrent] = useState(0);
   const [playing, setPlaying] = useState(false);
   const [trackW, setTrackW] = useState(0);
+  // Intrinsic (source) video dims and the video element's rendered box, both needed to map the
+  // crop marquee onto the letterboxed picture. `vdim` starts from asset metadata so the overlay
+  // can size before the media decodes; `box` is measured off the element (max-h can clamp it).
+  const [vdim, setVdim] = useState({ w: job.asset?.width ?? 0, h: job.asset?.height ?? 0 });
+  const [box, setBox] = useState({ w: 0, h: 0 });
 
   // The cut-list: contiguous segments covering [0, duration]. Starts as the whole video, kept.
   const [segments, setSegments] = useState<Seg[]>([{ id: 0, start: 0, end: srcDur, keep: true }]);
@@ -83,11 +88,28 @@ export default function VideoEditor({ job, onClose }: { job: DownloadJob; onClos
     return () => ro.disconnect();
   }, []);
 
+  // Measure the video element's rendered box (max-h can shrink it below its intrinsic size, which
+  // then pillarboxes the picture). The crop overlay is anchored to the picture, not this box.
+  useEffect(() => {
+    const v = videoRef.current;
+    if (!v || typeof ResizeObserver === "undefined") return;
+    const ro = new ResizeObserver(() => setBox({ w: v.clientWidth, h: v.clientHeight }));
+    ro.observe(v);
+    setBox({ w: v.clientWidth, h: v.clientHeight });
+    return () => ro.disconnect();
+  }, []);
+
+  // The picture's rendered rectangle inside the (possibly letterboxed) video box — the overlay's
+  // frame, so a crop fraction is a fraction of the real picture, not of the black bars.
+  const va = vdim.w && vdim.h ? vdim.w / vdim.h : aspect;
+  const picRect = contentRect(box.w, box.h, va);
+
   const onLoaded = () => {
     const v = videoRef.current;
     if (!v) return;
     const d = v.duration && isFinite(v.duration) ? v.duration : srcDur;
     setDuration(d);
+    if (v.videoWidth && v.videoHeight) setVdim({ w: v.videoWidth, h: v.videoHeight });
     // Reconcile the still-pristine full-length segment to the REAL decoded duration. The initial
     // segment ends at the (integer, often-rounded) metadata `srcDur`; once the media loads we know
     // the true length (e.g. 213.44 vs 213). Without this the untouched last segment stays short, so
@@ -258,16 +280,12 @@ export default function VideoEditor({ job, onClose }: { job: DownloadJob; onClos
 
   const cropPx = (): { x: number; y: number; w: number; h: number } | undefined => {
     const v = videoRef.current;
-    const vw = v?.videoWidth || job.asset?.width || 0;
-    const vh = v?.videoHeight || job.asset?.height || 0;
-    if (!cropOn || !vw || !vh) return undefined;
-    const even = (n: number) => Math.max(2, Math.round(n / 2) * 2);
-    return {
-      x: even(crop.x * vw),
-      y: even(crop.y * vh),
-      w: even(crop.w * vw),
-      h: even(crop.h * vh),
-    };
+    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 create = useMutation({
@@ -322,11 +340,20 @@ export default function VideoEditor({ job, onClose }: { job: DownloadJob; onClos
             onTimeUpdate={onTimeUpdate}
             onPlay={() => setPlaying(true)}
             onPause={() => setPlaying(false)}
-            className="w-full max-h-[42vh] mx-auto block"
+            className="w-full max-h-[42vh] mx-auto block object-contain"
             playsInline
           />
-          {cropOn && (
-            <div ref={overlayRef} className="absolute inset-0">
+          {cropOn && picRect && (
+            <div
+              ref={overlayRef}
+              className="absolute"
+              style={{
+                left: picRect.left,
+                top: picRect.top,
+                width: picRect.width,
+                height: picRect.height,
+              }}
+            >
               <div
                 className="absolute border-2 border-accent cursor-move"
                 style={{
diff --git a/frontend/src/lib/cropGeometry.test.ts b/frontend/src/lib/cropGeometry.test.ts
new file mode 100644
index 00000000..85b47c56
--- /dev/null
+++ b/frontend/src/lib/cropGeometry.test.ts
@@ -0,0 +1,88 @@
+import { describe, expect, it } from "vitest";
+import { contentRect, cropToSourcePx } from "./cropGeometry";
+
+describe("contentRect (display-px letterbox mapping)", () => {
+  it("returns the full box when box aspect matches the video aspect", () => {
+    // 16:9 picture in a 16:9 box → no bars, content == box
+    expect(contentRect(1600, 900, 16 / 9)).toEqual({
+      left: 0,
+      top: 0,
+      width: 1600,
+      height: 900,
+    });
+  });
+
+  it("pillarboxes (bars left/right) when the box is wider than the picture", () => {
+    // 16:9 picture in a 2:1 box → full height, narrower centered width
+    const r = contentRect(2000, 1000, 16 / 9)!;
+    expect(r.height).toBe(1000);
+    expect(r.width).toBeCloseTo(1000 * (16 / 9), 3);
+    expect(r.top).toBe(0);
+    expect(r.left).toBeCloseTo((2000 - 1000 * (16 / 9)) / 2, 3);
+  });
+
+  it("letterboxes (bars top/bottom) when the box is taller than the picture", () => {
+    // 16:9 picture in a 1:1 box → full width, shorter centered height
+    const r = contentRect(1000, 1000, 16 / 9)!;
+    expect(r.width).toBe(1000);
+    expect(r.height).toBeCloseTo(1000 / (16 / 9), 3);
+    expect(r.left).toBe(0);
+    expect(r.top).toBeCloseTo((1000 - 1000 / (16 / 9)) / 2, 3);
+  });
+
+  it("returns null for unmeasurable inputs", () => {
+    expect(contentRect(0, 900, 16 / 9)).toBeNull();
+    expect(contentRect(1600, 0, 16 / 9)).toBeNull();
+    expect(contentRect(1600, 900, 0)).toBeNull();
+    expect(contentRect(1600, 900, NaN)).toBeNull();
+  });
+});
+
+describe("cropToSourcePx (fraction-of-content → even source px)", () => {
+  it("scales content fractions straight to source pixels", () => {
+    // A marquee anchored to the content rect: a fraction is a fraction of the real picture.
+    expect(cropToSourcePx({ x: 0.25, y: 0.25, w: 0.5, h: 0.5 }, 1920, 1080)).toEqual({
+      x: 480,
+      y: 270,
+      w: 960,
+      h: 540,
+    });
+  });
+
+  it("rounds to even dimensions", () => {
+    // 0.1 * 1920 = 192 (even); 0.333 * 1080 = 359.64 → 360
+    const c = cropToSourcePx({ x: 0.1, y: 0.333, w: 0.2, h: 0.111 }, 1920, 1080)!;
+    expect(c.w % 2).toBe(0);
+    expect(c.h % 2).toBe(0);
+    expect(c.x % 2).toBe(0);
+    expect(c.y % 2).toBe(0);
+  });
+
+  it("allows a top-left origin (x/y may be 0)", () => {
+    const c = cropToSourcePx({ x: 0, y: 0, w: 0.5, h: 0.5 }, 1920, 1080)!;
+    expect(c.x).toBe(0);
+    expect(c.y).toBe(0);
+  });
+
+  it("clamps the window inside the frame so even-rounding can't overflow", () => {
+    // A full-frame crop must stay exactly at the source size, not round past it.
+    const c = cropToSourcePx({ x: 0, y: 0, w: 1, h: 1 }, 1921, 1081)!;
+    expect(c.w).toBeLessThanOrEqual(1921);
+    expect(c.h).toBeLessThanOrEqual(1081);
+    expect(c.x + c.w).toBeLessThanOrEqual(1921);
+    expect(c.y + c.h).toBeLessThanOrEqual(1081);
+  });
+
+  it("keeps x+w within the frame for an off-square region near the right edge", () => {
+    const srcW = 1280;
+    const srcH = 720;
+    const c = cropToSourcePx({ x: 0.7, y: 0.1, w: 0.3, h: 0.8 }, srcW, srcH)!;
+    expect(c.x + c.w).toBeLessThanOrEqual(srcW);
+    expect(c.y + c.h).toBeLessThanOrEqual(srcH);
+  });
+
+  it("returns null for an unusable source size", () => {
+    expect(cropToSourcePx({ x: 0, y: 0, w: 1, h: 1 }, 0, 1080)).toBeNull();
+    expect(cropToSourcePx({ x: 0, y: 0, w: 1, h: 1 }, 1920, 0)).toBeNull();
+  });
+});
diff --git a/frontend/src/lib/cropGeometry.ts b/frontend/src/lib/cropGeometry.ts
new file mode 100644
index 00000000..eebd268f
--- /dev/null
+++ b/frontend/src/lib/cropGeometry.ts
@@ -0,0 +1,48 @@
+// Crop geometry for the video editor — the display-px ↔ source-px mapping, isolated as pure
+// functions so the math is unit-testable without a DOM.
+//
+// The bug this fixes (E7 S1→S2): the crop marquee is drawn in fractions of the *video element box*,
+// but a letterboxed video (object-fit: contain) does NOT fill that box — pillar/letterbox bars sit
+// between the box edge and the actual picture. Multiplying box-fractions by the source resolution
+// therefore emitted an ffmpeg crop shifted/scaled off from what the selector showed. The fix is to
+// anchor the overlay to the *content* rect (below), so a fraction of the overlay is a fraction of
+// the real picture, and cropToSourcePx() is then a straight fraction→pixel scale.
+
+export type Frac = { x: number; y: number; w: number; h: number };
+type Rect = { left: number; top: number; width: number; height: number };
+type CropPx = { x: number; y: number; w: number; h: number };
+
+/**
+ * The rendered video-content rectangle inside an element box of `boxW × boxH`, given the video's
+ * intrinsic aspect ratio, when the video is laid out `object-fit: contain` (letterboxed). Returns
+ * null while any input is unmeasurable (0/NaN), so callers can skip drawing until the video sizes.
+ */
+export function contentRect(boxW: number, boxH: number, videoAspect: number): Rect | null {
+  if (!(boxW > 0) || !(boxH > 0) || !(videoAspect > 0)) return null;
+  const boxAspect = boxW / boxH;
+  if (boxAspect > videoAspect) {
+    // box wider than the picture → pillarbox (bars left/right), full height
+    const width = boxH * videoAspect;
+    return { left: (boxW - width) / 2, top: 0, width, height: boxH };
+  }
+  // box taller than the picture → letterbox (bars top/bottom), full width
+  const height = boxW / videoAspect;
+  return { left: 0, top: (boxH - height) / 2, width: boxW, height };
+}
+
+/**
+ * A crop expressed as fractions of the video *content* → integer, even source-pixel crop args
+ * (ffmpeg `crop=w:h:x:y`). Even dimensions keep chroma-subsampled encoders happy; the window is
+ * clamped inside the frame so even-rounding can never push `x+w`/`y+h` past the source resolution
+ * (which ffmpeg rejects as "crop area out of bounds"). Returns null for an unusable source size.
+ */
+export function cropToSourcePx(crop: Frac, srcW: number, srcH: number): CropPx | null {
+  if (!(srcW > 0) || !(srcH > 0)) return null;
+  const evenDim = (n: number) => Math.max(2, Math.round(n / 2) * 2); // w/h ≥ 2
+  const evenOff = (n: number) => Math.max(0, Math.round(n / 2) * 2); // x/y ≥ 0 (top-left is valid)
+  const w = Math.min(evenDim(crop.w * srcW), srcW);
+  const h = Math.min(evenDim(crop.h * srcH), srcH);
+  const x = Math.min(evenOff(crop.x * srcW), srcW - w);
+  const y = Math.min(evenOff(crop.y * srcH), srcH - h);
+  return { x, y, w, h };
+}