Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make sure physical units, not logical units, are even #56

Merged
merged 1 commit into from May 25, 2020

Conversation

mortie
Copy link
Contributor

@mortie mortie commented Oct 17, 2019

The current wf-recorder just makes sure that the logical geometry provided by the user are even, but that doesn't guarantee that the dimensions of the buffer provided by zwlr_screencopy are even. With this patch, wf-recorder instead makes sure that the width and height of the physical buffer received from zwlr_screencopy is even.

This fixes #55.

There are a couple of behavior changes:

  • The old code would potentially increase the width/height by one logical pixel. The new code potentially decreases the width/height by one physical pixel instead.
  • The new code doesn't print Adjusted geometry: %d,%d %dx%d. If it's required, I could add it back, but I haven't because that would require adding global state to avoid printing the message for every new frame.

Copy link
Owner

@ammen99 ammen99 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like this approach. The screencopy protocol says: "The client should then create a buffer with the provided attributes".

Instead, we should just save output scale and change make_even to take it into account.

Copy link
Owner

@ammen99 ammen99 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, I reviewed this a little bit more carefully and I can see that you are right, this could work. My only question is, why is stride modified? Shouldn't it stay the same?

@mortie
Copy link
Contributor Author

mortie commented May 24, 2020

Yeah, you're right. That doesn't make sense.

I also found out that my patches didn't work, which is strange because I could've sworn I tested it; maybe I just checked if recording videos worked or not without checking the video after to see if it was correct. It turns out that the FrameWriter just assumes that stride = width * 4, which obviously isn't correct when changing the width without modifying the buffer.

I've now removed the stride modification, and adjusted FrameWriter to take a stride parameter in addition to width/height.

Copy link
Owner

@ammen99 ammen99 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also tested this new version and it seems to work. Thanks and sorry for the delay!

I have a last request, can you rebase your commits? I'd prefer to not have merge commits in the PR.

@mortie
Copy link
Contributor Author

mortie commented May 24, 2020

There, rebased :)

@ammen99 ammen99 merged commit bda77be into ammen99:master May 25, 2020
@WhyNotHugo
Copy link
Contributor

Thanks!

@ojab ojab mentioned this pull request Aug 23, 2021
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.

wf-recorder might end up trying to encode odd widths/heights if display scaling isn't an integer
3 participants