fix(render): preserve vertical aspect when source is portrait#23
Open
sidorovanthon wants to merge 1 commit intobrowser-use:mainfrom
Open
fix(render): preserve vertical aspect when source is portrait#23sidorovanthon wants to merge 1 commit intobrowser-use:mainfrom
sidorovanthon wants to merge 1 commit intobrowser-use:mainfrom
Conversation
The scale filter in extract_segment hardcoded scale=1920:-2 (or scale=1280:-2 in draft mode), which assumes a landscape source. Given a portrait source like 1080x1920 from a phone, this upscaled width to 1920 and pushed height to 3414, producing an off-spec 1920x3414 output instead of the expected 1080x1920. Probe the source with ffprobe and pick the scale axis from orientation: - portrait (h > w): scale=-2:1920 / scale=-2:1280 - landscape: scale=1920:-2 / scale=1280:-2 This keeps native portrait sources at 1080x1920 in final/preview and 720x1280 in draft, matching the documented vertical social target in SKILL.md.
There was a problem hiding this comment.
1 issue found across 1 file
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="helpers/render.py">
<violation number="1" location="helpers/render.py:157">
P1: Portrait detection ignores rotation metadata, so rotated portrait phone clips can still be treated as landscape and scaled incorrectly.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
Comment on lines
+157
to
+167
| probe = subprocess.run( | ||
| ["ffprobe", "-v", "error", "-select_streams", "v:0", | ||
| "-show_entries", "stream=width,height", | ||
| "-of", "csv=p=0:s=x", str(source)], | ||
| capture_output=True, text=True, | ||
| ) | ||
| try: | ||
| sw, sh = (int(x) for x in probe.stdout.strip().split("x")[:2]) | ||
| is_vertical = sh > sw | ||
| except Exception: | ||
| is_vertical = False |
There was a problem hiding this comment.
P1: Portrait detection ignores rotation metadata, so rotated portrait phone clips can still be treated as landscape and scaled incorrectly.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At helpers/render.py, line 157:
<comment>Portrait detection ignores rotation metadata, so rotated portrait phone clips can still be treated as landscape and scaled incorrectly.</comment>
<file context>
@@ -154,10 +154,21 @@ def extract_segment(
"""
out_path.parent.mkdir(parents=True, exist_ok=True)
+ probe = subprocess.run(
+ ["ffprobe", "-v", "error", "-select_streams", "v:0",
+ "-show_entries", "stream=width,height",
</file context>
Suggested change
| probe = subprocess.run( | |
| ["ffprobe", "-v", "error", "-select_streams", "v:0", | |
| "-show_entries", "stream=width,height", | |
| "-of", "csv=p=0:s=x", str(source)], | |
| capture_output=True, text=True, | |
| ) | |
| try: | |
| sw, sh = (int(x) for x in probe.stdout.strip().split("x")[:2]) | |
| is_vertical = sh > sw | |
| except Exception: | |
| is_vertical = False | |
| try: | |
| probe = subprocess.run( | |
| ["ffprobe", "-v", "error", "-select_streams", "v:0", | |
| "-show_streams", "-of", "json", str(source)], | |
| capture_output=True, text=True, check=True, | |
| ) | |
| stream = json.loads(probe.stdout)["streams"][0] | |
| sw = int(stream["width"]) | |
| sh = int(stream["height"]) | |
| rotation = int(stream.get("tags", {}).get("rotate", 0) or 0) | |
| for sd in stream.get("side_data_list", []): | |
| rotation = int(sd.get("rotation", rotation) or rotation) | |
| is_vertical = (rotation % 180 != 0) or (sh > sw) | |
| except Exception: | |
| print("warning: could not detect source orientation; defaulting to landscape", file=sys.stderr) | |
| is_vertical = False |
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
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.
Problem
extract_segmentinhelpers/render.pyhardcodes the scale filter to width-anchored:This assumes a landscape source. When the source is portrait (e.g. a phone capture at 1080x1920), the filter upscales width to 1920 and
-2derives height to keep the aspect — producing 1920x3414 output instead of the expected 1080x1920.This conflicts with the documented vertical social target in
SKILL.md:Reproduction
python helpers/render.py edl.json -o final.mp4ffprobe final.mp4→width=1920, height=3414Fix
Probe the source's
width,heightonce and pick the scale axis from orientation:scale=-2:1920(or-2:1280for draft)scale=1920:-2/scale=1280:-2)13 lines added in
extract_segment, no behavior change for landscape sources.Verified on a 1080x1920 70s clip: output is now 1080x1920 @ 24fps as expected, matching the documented spec.
Notes
Found while running video-use end-to-end on Windows with a vertical phone capture as part of normal usage — not synthesized for the PR.
Summary by cubic
Fixes scaling for portrait sources so exported video keeps the correct vertical aspect. Portrait clips now render as 1080x1920 in final and 720x1280 in draft, matching the
SKILL.mdvertical social target.ffprobe(readswidth,height).scale=-2:1920(or-2:1280in draft) for portrait; keeps1920:-2/1280:-2for landscape.Written for commit d0b6989. Summary will update on new commits. Review in cubic