Skip to content

Conversation

@soreau
Copy link
Collaborator

@soreau soreau commented Jul 16, 2025

No description provided.

@dotaxis
Copy link

dotaxis commented Jul 16, 2025

This doesn't fix #173.

The coordinates do not pass the contained_in check and recording fails on rotated monitors.

@soreau
Copy link
Collaborator Author

soreau commented Jul 16, 2025

This doesn't fix #173.

The coordinates do not pass the contained_in check and recording fails on rotated monitors.

I think there are two issues at hand. One is outdated wayfire causing #173. Two is a new feature, that rotates and flips the video where applicable so that the resulting video orientation is always top=up. This PR fixes the latter, so you are probably right that this doesn't fix the original poster's problem in #173.

@soreau
Copy link
Collaborator Author

soreau commented Jul 17, 2025

I have updated the commit message and description so this will not close any issues if merged.

@soreau
Copy link
Collaborator Author

soreau commented Jul 17, 2025

A question I have considered: Should there be a way to disable this functionality so the recording is not automatically transposed? Or is it safe to say that everyone expects the video to always be transposed to the output orientation?

@apiraino
Copy link

My .2 cents as an end-user: I think this is the good default I would expect. If I wanted the other way, I would probably play the video rotated before screen-grabbing it.

@soreau
Copy link
Collaborator Author

soreau commented Jul 17, 2025

My .2 cents as an end-user: I think this is the good default I would expect. If I wanted the other way, I would probably play the video rotated before screen-grabbing it.

True, and if they want it another way, they can reprocess it.

@soreau
Copy link
Collaborator Author

soreau commented Jul 17, 2025

@tkna91 I didn't want to clutter the other thread so I am responding here. I was able to reproduce this with two sway outputs, the 'first'/right-most at 90 and the second/left-most output normal. When I try wf-recorder with slurp on the 90 output, I get the same error as you. I tested and this works with master, so I will look into it and try to find out why this PR breaks it. Thanks for testing!

@soreau
Copy link
Collaborator Author

soreau commented Jul 18, 2025

I found out that the reason is because this PR switched from xdg-output to wl_output for getting the output geometry, but wlroots doesn't properly advertise the x/y coordinates, it just sends 0,0 for all outputs. This means wf-recorder is confused when using multiple outputs because it thinks they are overlapping. The wlroots devs position is that 'clients should not care' about this information and that if you want it, you must use xdg-output. This means binding both interfaces, just to get the transform for the output via wl_output and the rest via xdg-output. I will update this PR to do this when I have the gumption.

@soreau
Copy link
Collaborator Author

soreau commented Jul 18, 2025

@tkna91 It should work now, with 9a0b971.

@tkna91
Copy link

tkna91 commented Jul 18, 2025

@soreau Rotation has normalized. However, the selection appears to be disabled.

20250719-000640.mp4

@soreau
Copy link
Collaborator Author

soreau commented Jul 18, 2025

@soreau Rotation has normalized. However, the selection appears to be disabled.
20250719-000640.mp4

Indeed, I was able to reproduce with your detailed feedback, thanks again for testing. I was under the impression that the width and height needed to be swapped, but apparently this isn't the case. So I omitted that code and it should be working with latest bits.

@tkna91
Copy link

tkna91 commented Jul 18, 2025

@soreau It looks fine to me.

20250719-010512.mp4

@soreau
Copy link
Collaborator Author

soreau commented Jul 18, 2025

@tkna91 That's great, thanks for verifying and for the videos, much appreciated! 👍

@ammen99
Copy link
Owner

ammen99 commented Jul 21, 2025

Nice, thanks!

@ammen99 ammen99 merged commit a6a22c2 into ammen99:master Jul 21, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants