Make the portrait frame set mean something - #376
Merged
Conversation
The "Generate mobile variant" toggle was generated on the path where its output cannot survive, and ignored on the path where it matters. Both halves measured by running the flows in Chrome. Style path, toggle on: two full renders, hasMobileFrames=1 carried into the editor, 120 portrait frames sitting in IndexedDB, and then a 64 KB procedural export with hasMobile=false, mobileCount=0 and no frames-mobile folder. The second render was thrown away every time, having roughly doubled the wait. That discarding is correct: a style background ships as a recipe and is redrawn at whatever size the viewport is, so a phone already gets a portrait background at its own resolution. So the style path now renders once and clears the slot. Video path, toggle on: handleUpload never read generateMobile. No portrait set, no parameter, nothing. Footage is the one case a portrait set cannot be derived from, so that is where the toggle now does its work - a second sampling pass at 720x1280. That pass would have shipped distorted frames. drawImage(video, 0, 0, w, h) scales the whole frame into the box, which stretches a 16:9 clip roughly three times vertically. It now draws from a centred source rectangle when the ratios differ, keeping the crop's aspect ratio within 0.08% of the target across every common source size. Verified from a real 1280x720 clip: 120 landscape frames at 1280x720 and 120 portrait at 720x1280, the portrait one a genuine centre crop. The served page fetches 120 portrait and 0 landscape at 390x844, and the reverse at 1440x900 - one set, not both, so the cellular-data guard still holds. The toggle's label now names the upload it applies to and says why a style background needs nothing. Suite 377 passed; six of the new tests fail on main.
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I had touched this feature twice without ever running it, so I ran both flows in Chrome. The "Generate mobile variant" toggle was generated on the path where its output cannot survive, and ignored on the path where it matters.
Style path: rendered twice, discarded once
hasMobileFrames=1into the editor, 120 portrait frames in IndexedDBhasMobile=false,mobileCount=0, noframes-mobile/The second render roughly doubled the wait and was thrown away every time.
The discarding is correct. A style background ships as a recipe and is redrawn at whatever size the viewport is, so a phone already gets a portrait background at its own resolution — strictly better than fixed portrait JPEGs. So the fix is to stop rendering it: the style path now renders once and clears the slot.
Video path: the toggle was never read
handleUploadnever looked atgenerateMobile. No portrait set, no parameter, nothing. Footage is the one case a portrait set cannot be derived from — nothing can redraw someone's video at a new aspect ratio — so that is where the toggle now does its work, as a second sampling pass at 720×1280.That pass would have shipped distorted frames
ctx.drawImage(video, 0, 0, w, h)scales the whole frame into the box, stretching a 16:9 clip about 3× vertically. I caught this before shipping. It now draws from a centred source rectangle when the ratios differ:A failed second pass cannot lose the upload that already succeeded — the landscape frames are stored first, and the user is told the phone will fall back to them.
Verified end to end
Real 1280×720 clip uploaded through
/createwith the toggle on, exported through the real button:Served and loaded at two widths:
One set, never both — the guard against a phone pulling ~70 MB on cellular still holds, and the canvas paints in both cases.
The toggle's label now names the upload it applies to and says why a style background needs nothing.
Eight new tests in
mobileFrames.test.ts, six failing onmain(two are pure maths that hold either way). Suite 377 passed.