fix(render): detect portrait sources that carry rotation metadata#109
Open
chekazoid wants to merge 1 commit into
Open
fix(render): detect portrait sources that carry rotation metadata#109chekazoid wants to merge 1 commit into
chekazoid wants to merge 1 commit into
Conversation
is_portrait_source() compared the stored stream dimensions, so footage recorded vertically on a phone -- a landscape stream plus a rotation entry in the display matrix -- was classified as landscape. ffmpeg auto-rotates on decode, so the filter graph received a portrait frame and scale=1920:-2 stretched it to 1920x3414. Read the rotation side data (falling back to the pre-ffmpeg-5 rotate tag) and swap the dimensions when rotation is an odd multiple of 90. browser-use#29 covered sources stored portrait; this covers sources rotated by metadata. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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:160">
P2: Non-right-angle rotation metadata is classified as portrait, so slightly rotated landscape sources can take the portrait scaling path. Limit the swap to 90°/270° rotations, which are the only cases that exchange axes.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
| # ffmpeg < 5 exposes the display matrix as a stream tag instead. | ||
| rotation = int(stream.get("tags", {}).get("rotate", 0)) | ||
|
|
||
| if rotation % 180 != 0: |
There was a problem hiding this comment.
P2: Non-right-angle rotation metadata is classified as portrait, so slightly rotated landscape sources can take the portrait scaling path. Limit the swap to 90°/270° rotations, which are the only cases that exchange axes.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At helpers/render.py, line 160:
<comment>Non-right-angle rotation metadata is classified as portrait, so slightly rotated landscape sources can take the portrait scaling path. Limit the swap to 90°/270° rotations, which are the only cases that exchange axes.</comment>
<file context>
@@ -132,15 +132,33 @@ def is_hdr_source(video: Path) -> bool:
+ # ffmpeg < 5 exposes the display matrix as a stream tag instead.
+ rotation = int(stream.get("tags", {}).get("rotate", 0))
+
+ if rotation % 180 != 0:
+ w, h = h, w
return h > w
</file context>
Suggested change
| if rotation % 180 != 0: | |
| if rotation % 180 == 90: |
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
is_portrait_source()compares the dimensions ffprobe reports for the stored stream. Phone cameras record vertical footage as a landscape stream plus a rotation entry in the display matrix. ffmpeg auto-rotates on decode, so the filter graph receives a portrait frame whileis_portrait_source()reports landscape — andscale=1920:-2gets applied to a portrait frame.Real phone clip (Android, 4:55, shot vertically):
extract_segment()on that file, before this change:A 720p vertical clip upscaled to 3414px tall, on the CPU-bound libx264 path.
#29 fixed sources whose stream is stored portrait (
h > w). Sources rotated by metadata — which is how essentially all phone footage is stored — still fall through to the landscape branch. #64 lists portrait orientation as already handled, which is true only for the stored-portrait case.Fix
Probe the rotation side data (falling back to the
rotatestream tag for ffmpeg < 5) and swap width/height when rotation is an odd multiple of 90°. No new dependencies;jsonwas already imported.Verification
is_portrait_source()across every orientation case:FalsefallbackEnd-to-end through
extract_segment()on the real phone clip: 1920x3414 → 720x1280.Rotation-180 sources stay landscape, and the existing
exceptfallback is unchanged.Note
This touches the same function as the open #104 (
--vertical output). Happy to rebase if that lands first.Summary by cubic
Fix portrait detection for videos that store rotation metadata.
is_portrait_source()now reads display rotation so vertical phone clips scale on the correct axis.ffprobeJSON (side_data.rotation, fallback totags.rotate) and swap width/height for odd 90° rotations.Written for commit f38ba27. Summary will update on new commits.