fix(export): stop counting letterbox rows as an upscale - #229
Conversation
📝 WalkthroughWalkthroughThe export flow now uses contain-fit output dimensions to detect upscaling. The dialog removes fixed target-size metadata and applies the shared check to all quality tiers. Tests cover letterboxed captures and genuine source upscaling. ChangesUpscale detection
Estimated code review effort: 2 (Simple) | ~15 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/components/ai-edition/ExportDialog.tsx (1)
409-417: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a same-package test for the ExportDialog upscale badge path.
src/components/ai-edition/ExportDialog.test.tsdoes not coverwillUpscale(dims, smallestSource)orqualityUpscaleWarning, while the exported badge useswouldUpscaleon the dialog’s tier output dimensions. Add integration coverage for the letterboxed source case and a genuine upscale case across the relevant quality options.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/ai-edition/ExportDialog.tsx` around lines 409 - 417, Add same-package integration tests in ExportDialog.test.ts covering the exported upscale badge path that calls wouldUpscale(dims, smallestSource), including a letterboxed source case that remains unflagged and a genuine upscale case that is flagged. Exercise the relevant quality options and verify qualityUpscaleWarning behavior using the dialog’s tier output dimensions.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/components/ai-edition/ExportDialog.tsx`:
- Around line 409-417: Add same-package integration tests in
ExportDialog.test.ts covering the exported upscale badge path that calls
wouldUpscale(dims, smallestSource), including a letterboxed source case that
remains unflagged and a genuine upscale case that is flagged. Exercise the
relevant quality options and verify qualityUpscaleWarning behavior using the
dialog’s tier output dimensions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e04c0496-2993-482a-9a92-ac78c92ab642
📒 Files selected for processing (3)
src/components/ai-edition/ExportDialog.tsxsrc/lib/exporter/mp4ExportSettings.test.tssrc/lib/exporter/mp4ExportSettings.ts
A 1920x1032 window capture in a 16:9 project exports into a 1920x1080 frame: the 48 extra rows are wallpaper, and the clip itself still renders at 1:1. The export dialog decided whether a tier upscaled by comparing short sides, so it read those rows as stretched pixels and flagged "1080p - Upscale" on the exact frame the "Source" tier produced unflagged, since both tiers resolve to the same 1920x1080 there. The two tiles sat side by side with identical dimensions and different warnings. wouldUpscale() now asks the contain-fit question the compositor actually answers: min(out.w/src.w, out.h/src.h) > 1. Nothing changes for a source whose shape matches the project ratio, which is why a full-screen capture never showed this and a window capture did. That also makes the "Source" exemption provable instead of asserted - its frame is the source's long side at the project ratio, so its contain scale is never above 1 - and the general test now covers it. The exemption goes, and with it the MEDIUM_SHORT_SIDE/HIGH_SHORT_SIDE constants duplicated from mp4ExportSettings and the write-only targetShortSide field they fed.
ee28154 to
85b0ab7
Compare
Reported from the export dialog: with a 1920x1032 window capture on the timeline, the Source tile reads
1920 × 1080.That number is right — it is the output frame, and the smallest 16:9 frame holding a 1920-wide source without shrinking it is 1920x1080. The source's real 1032 was never lost either: it is what makes the 720p tile read as a downscale. So the size SSOT is fine.
What was wrong is next to it: 1080p and Source resolve to the exact same 1920x1080 frame, and only one of them carries the "Upscale" warning.
1280 × 7201280 × 7201920 × 1080 · Upscale1920 × 10801920 × 10801920 × 1080The badge compared short sides (
1080 > 1032), which counts the letterbox rows a non-16:9 source gets in a 16:9 project as if they were stretched pixels. Those 48 rows are wallpaper; the clip is contain-fitted and still renders at 1:1.wouldUpscale()now asks the contain-fit question the compositor actually answers:A source whose shape matches the project ratio is unaffected — which is why a full-screen capture never showed this and a window capture did. A tier that genuinely stretches the source (1280x720 → 1080p, scale 1.5) is still flagged.
The
q.value !== "source"exemption is gone: Source's frame is the source's long side at the project ratio, so its contain scale is never above 1 and the general test covers it. TheMEDIUM_SHORT_SIDE/HIGH_SHORT_SIDEconstants (duplicated frommp4ExportSettings.ts) and the write-onlytargetShortSidefield they fed go with it.Verification
src/lib/exporter/mp4ExportSettings.test.ts— 8/8, two new cases: the reported 1920x1032 @ 16:9 (all three tiers unflagged) and a real stretch that must stay flagged.src/components/ai-edition/ExportDialog.test.ts— 8/8.biome checkclean on the three files (also via the pre-commit hook).tsc --noEmitreports one pre-existing unrelated error,src/i18n/loader.ts: Cannot find module 'i18next'— the localnode_modulespredates that dependency and needs annpm install.Summary by CodeRabbit