Creates a protocol based on Pipewire that allows us to request feeds to
be set up by KWin rather than having to do it from a separate process.
This will offer tighter control to KWin to do serve not only outputs but
also windows as well.
Details
Details
- Reviewers
jgrulich davidedmundson zzag - Group Reviewers
KWin
Treat this as a proof of concept to discuss if it's the protocol
we need right now.
Diff Detail
Diff Detail
- Repository
- R127 KWayland
- Branch
- master
- Lint
Lint OK - Unit
No Unit Test Coverage - Build Status
Buildable 25525 Build 25543: arc lint + arc unit
| src/client/protocols/screencast.xml | ||
|---|---|---|
| 14 | Can we switch the values to follow xdg-desktop-portal specification? Even not following the specs, output will be the most used one. | |
Comment Actions
Doesn't seem to build here:
In file included from /home/jgrulich/development/projects/kde/kwayland/src/server/screencasting_interface.cpp:7:
/home/jgrulich/development/projects/kde/kwayland/src/server/screencasting_interface.h:32:121: error: ‘std::function’ has not been declared
32 | ScreencastingSource(const QString &description, const QString &iconName, bool isOutput, const QRect &geometry, std::function<void(const ScreencastingSource &, wl_resource *)>);
| ^~~~~~~~
In file included from /home/jgrulich/development/projects/kde/kwayland/src/server/screencasting_interface.cpp:7:
/home/jgrulich/development/projects/kde/kwayland/src/server/screencasting_interface.h:32:129: error: expected ‘,’ or ‘...’ before ‘<’ token
32 | ScreencastingSource(const QString &description, const QString &iconName, bool isOutput, const QRect &geometry, std::function<void(const ScreencastingSource &, wl_resource *)>);
| ^
/home/jgrulich/development/projects/kde/kwayland/src/server/screencasting_interface.cpp:41:1: error: no declaration matches ‘KWayland::Server::ScreencastingSource::ScreencastingSource(const QString&, const QString&, bool, const QRect&, std::function<void(const KWayland::Server::ScreencastingSource&, wl_resource*)>)’
41 | ScreencastingSource::ScreencastingSource(const QString &description, const QString &iconName, bool isOutput, const QRect &geometry, std::function<void(const ScreencastingSource &,wl_resource *r)> call)
| ^~~~~~~~~~~~~~~~~~~Comment Actions
Also propagate the buffer size, it's important for the client to know what buffer size it will have.
Comment Actions
I have tested this and it now works as before, tested with Chromium, while checking all the values we pass to the portal and PipeWire which seem to be correct.
Comment Actions
In future, it might be faster to put up just the interface xml for review first.
In terms of wayland protocols this is non-standard.
All clients get a list of available sources. On bind they get all all available sources via addSource. Additional sources are new events.
That part's fine.
The part with nodeID is unusual. It's convention (though not technically the law) that globals broadcast the same thing to all bound resources.
ClientA requests create.
All clients get a created event
There's no way to tie a created or failed to the original request
Client A and B can both connect to the same source, yet either can call close.
If we're doing a waland protocol I would have expected:
ScreenCastInterface - global event: source_added event: done request: get_stream (returns a ScreenStream object) ScreenStream - resource event: created event: failed event: closed (closed by external sources, clients should now release the stream resource) request: release (closes the stream if applicable, is also of type destructor)
| src/client/protocols/screencast.xml | ||
|---|---|---|
| 17 ↗ | (On Diff #80658) | Typically events are like signals, so would be called "source_added" Convention is also lower_snake_case |
| 32 ↗ | (On Diff #80658) | this is racey with create. |
| src/client/protocols/screencast.xml | ||
|---|---|---|
| 32 ↗ | (On Diff #80658) | Any suggestion on how to do it better? |
| src/client/protocols/screencast.xml | ||
|---|---|---|
| 47 ↗ | (On Diff #80658) | add type = "destructor" so that clients calling this release the resource |
Comment Actions
In future, it might be faster to put up just the interface xml for review first.
++
What about using existing wl_output objects? The add_source event can be simplified quite a lot (even maybe dropped). For windows, we could use string handles.
<request name="record_monitor"> <description/> <arg name="id" type="new_id" interface="zkde_screencast_stream_v1"/> <arg name="output" type="object" interface="wl_output"/> </request> <request name="record_window"> <description/> <arg name="id" type="new_id" interface="zkde_screencast_stream_v1"/> <arg name="handle" type="string"/> </request>
Please notice that the manager object also needs to advertise supported cursor modes
<enum name="cursor_mode"> <entry name="hidden" value="0" summary="the cursor is not part of the screen cast stream"/> <entry name="metadata" value="1" summary="the cursor is not part of the screen cast stream, but sent as PipeWire metadata"/> <entry name="embedded" value="2" summary="the cursor is embedded as part of the stream buffers"/> </enum> <event name="cursor_mode"> <description summary="blah-blah"></description> <arg name="mode" type="uint" enum="cursor_mode" summary="supported cursor mode"/> </event>
| src/client/protocols/screencast.xml | ||
|---|---|---|
| 9 ↗ | (On Diff #80658) | Interfaces usually come without unstable in the name. s/zkde_screencast_unstable_v1/zkde_screencast_v1/ |
| 53–55 ↗ | (On Diff #80658) | zkde_screencast_stream_unstable_v1 must have a destructor request. |
Comment Actions
In future, it might be faster to put up just the interface xml for review first.
@davidedmundson @zzag I don't really see how it would have made a difference, you only decided to review it 7 to 10 days after I first submitted it. 🤷
What about using existing wl_output objects? The add_source event can be simplified quite a lot (even maybe dropped). For windows, we could use string handles.
So your proposal would be to pass a random resource id or string and see what the compositor spits back?
My intention was to keep the protocol auto-contained to some extent so every client doesn't need to track windows and screens to be able to request a stream.
Comment Actions
Well, it's not that different from passsing uints like this patch proposes. In either case we have to have some protection against garbage input values and possibly inert resources. wl_outputs are not some random objects and practically every GUI application keeps track of them.
My intention was to keep the protocol auto-contained to some extent so every client doesn't need to track windows and screens to be able to request a stream.