Create protocol to manage video feeds
AbandonedPublic

Authored by apol on Apr 16 2020, 2:33 PM.

Details

Reviewers
jgrulich
davidedmundson
zzag
Group Reviewers
KWin
Summary

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.

Test Plan

Treat this as a proof of concept to discuss if it's the protocol
we need right now.

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
apol created this revision.Apr 16 2020, 2:33 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptApr 16 2020, 2:33 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
apol requested review of this revision.Apr 16 2020, 2:33 PM
apol updated this revision to Diff 80284.Apr 16 2020, 2:59 PM
apol added a subscriber: zzag.

renamed protocol as per @zzag's suggestion

jgrulich added inline comments.Apr 17 2020, 8:30 AM
src/client/protocols/screencast.xml
14

Can we switch the values to follow xdg-desktop-portal specification?

See https://github.com/flatpak/xdg-desktop-portal/blob/master/data/org.freedesktop.impl.portal.ScreenCast.xml#L168.

Even not following the specs, output will be the most used one.

romangg added inline comments.
src/client/protocols/screencast.xml
9

Interface names should be scoped too. Example.

Don't ask me why exactly why. I think it has something to do with the C structs becoming ambiguous otherwise. Someone else knows?

But in any case I would follow the examples set by upstream.

apol marked 2 inline comments as done.Apr 17 2020, 4:50 PM
apol updated this revision to Diff 80411.Apr 17 2020, 4:52 PM

Include the server side and some renames

apol updated this revision to Diff 80604.Apr 20 2020, 12:07 AM

Include server side, a test (that doesn't pass) and some renaming

apol updated this revision to Diff 80637.Apr 20 2020, 11:28 AM

iterate test

apol updated this revision to Diff 80658.Apr 20 2020, 3:29 PM

Fix test

apol updated this revision to Diff 80662.Apr 20 2020, 4:27 PM

Iterate tests (which work now) and uses

apol updated this revision to Diff 80677.Apr 20 2020, 6:06 PM

Improve how we initialise the sourceId

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)
      | ^~~~~~~~~~~~~~~~~~~

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)
      | ^~~~~~~~~~~~~~~~~~~

Missing #include <functional>

apol updated this revision to Diff 80798.Apr 21 2020, 5:58 PM
  • Test Cleanup
  • When a resource is destroyed, emit to close all its streams
apol updated this revision to Diff 80801.Apr 21 2020, 6:45 PM

Hopefully fix the build for Jan

apol updated this revision to Diff 80984.Apr 23 2020, 11:32 AM

Also propagate the buffer size, it's important for the client to know what buffer size it will have.

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.

davidedmundson requested changes to this revision.Apr 23 2020, 8:50 PM
davidedmundson added a subscriber: davidedmundson.

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.

This revision now requires changes to proceed.Apr 23 2020, 8:50 PM
apol marked an inline comment as done.Apr 24 2020, 4:35 PM
apol added inline comments.
src/client/protocols/screencast.xml
32 ↗(On Diff #80658)

Any suggestion on how to do it better?

apol updated this revision to Diff 81113.Apr 24 2020, 4:56 PM

Refactor the protocol as suggested by David and Vlad

src/client/protocols/screencast.xml
47 ↗(On Diff #80658)

add type = "destructor"

so that clients calling this release the resource

meven added a subscriber: meven.Apr 27 2020, 1:04 PM
meven added inline comments.
src/client/screencasting.cpp
142 ↗(On Diff #80658)

Missing blank line

157 ↗(On Diff #80658)

Two blank lines too mang

apol marked 2 inline comments as done.Apr 27 2020, 1:34 PM
apol updated this revision to Diff 81341.Apr 27 2020, 2:07 PM

Address review comments

zzag requested changes to this revision.Apr 27 2020, 5:34 PM

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.

This revision now requires changes to proceed.Apr 27 2020, 5:34 PM
apol added a comment.Apr 27 2020, 11:26 PM

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.

zzag added a comment.Apr 28 2020, 5:44 AM
In D28882#658780, @apol wrote:

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?

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.

apol abandoned this revision.May 8 2020, 5:34 PM

Moving to kwayland-server