emersion / xdg-desktop-portal-wlr

xdg-desktop-portal backend for wlroots
MIT License
579 stars 54 forks source link

Screencast response fixes #292

Closed cptpcrd closed 6 months ago

cptpcrd commented 6 months ago

This PR fixes 2 errors in the response returned by the screencast interface:

emersion commented 6 months ago

The position was hardcoded as (0, 0). If the compositor supports the xdg-output protocol, use that protocol to get the correct position and report it. Otherwise, omit the position entirely.

I'm not sure that's correct. The position is in logical coordinates, while the size is in buffer coordinates.

emersion commented 6 months ago

The first commit LGTM, I've pushed it to master.

cptpcrd commented 6 months ago

I'm not sure that's correct. The position is in logical coordinates, while the size is in buffer coordinates.

Hmm, I hadn't considered that. This is interesting though:

https://flatpak.github.io/xdg-desktop-portal/docs/doc-org.freedesktop.impl.portal.ScreenCast.html#org-freedesktop-impl-portal-screencast-start

  • position ((ii))

    A tuple consisting of the position (x, y) in the compositor coordinate space. Note that the position may not be equivalent to a position in a pixel coordinate space. Only available for monitor streams.

  • size ((ii))

    A tuple consisting of (width, height). The size represents the size of the stream as it is displayed in the compositor coordinate space. Note that this size may not be equivalent to a size in a pixel coordinate space. The size may differ from the size of the stream.

https://gitlab.freedesktop.org/wayland/wayland-protocols/-/blob/main/unstable/xdg-output/xdg-output-unstable-v1.xml

The position event describes the location of the wl_output within the global compositor space.

The logical_position event is sent after creating an xdg_output (see xdg_output_manager.get_xdg_output) and whenever the location of the output changes within the global compositor space.

...

The logical_size event describes the size of the output in the global compositor space.

...

Unless I'm misreading something here (which is possible, I've only done a llittle Wayland programming), it seems that the logical coordinates from xdg-output should really be used for both the position and size?

emersion commented 6 months ago

Yeah, sounds like it.

Note that ideally we shouldn't make xdg-output mandatory (fallback to the current behavior in that case).

cptpcrd commented 6 months ago

Updated to use the logical position and size from xdg-output if available, otherwise report the buffer size and omit the position. (This page seems to indicate the position is optional, and IMO it's better to leave it out than have it be wrong. If you feel differently I can change it.)

emersion commented 6 months ago

Thanks!

columbarius commented 6 months ago

Updated to use the logical position and size from xdg-output if available, otherwise report the buffer size and omit the position. (This page seems to indicate the position is optional, and IMO it's better to leave it out than have it be wrong. If you feel differently I can change it.)

No client should rely on this value for the position of the output or the buffer size. The screencast will continue if the output changes the resolution, the orientation (both changing the buffer size) or position (absolut irrelevant for xdpw) and we will update the buffer size via pipewire accordingly. This was the reason, why I omitted them.

columbarius commented 6 months ago

It would probably be better to attach the output position as a parameter to the PipeWire stream. Would be interesting if xdp can read them.