Details

Reviewers

graesslin
Kanedias
romangg
davidedmundson

Maniphest Tasks

T7785: PipeWire support in remote access to KWin

Commits

R838:76811d65478e: Add screen cast portal

Summary

Add support for screen casting, using pipewire for creating streams. Screen data
is supplied by KWin which sends us GBM fd using Remote Access interface from KWayland.

Note that this is meant for sandboxed and also not sandboxed applications as plan for Pipewire is that
if application talks directly to Pipewire, not calling xdg-desktop-portal (the middle man), then Pipewire
will still call backend implementation internally, in our case xdg-desktop-portal-kde to get screen cast stream.

Depends on D1231 and D1230

Diff Detail

Repository

R838 Flatpak Support: KDE Portal for XDG Desktop

Lint

Automatic diff as part of commit; lint not applicable.

Unit

Automatic diff as part of commit; unit tests not applicable.

jgrulich requested review of this revision.Mar 2 2018, 10:42 AM

jgrulich created this revision.

jgrulich added a reviewer: davidedmundson.Mar 2 2018, 10:44 AM

jgrulich added a subscriber: Plasma.

Hi Jan, I'll take a look in the evening once I'm home.
Thanks!

jgrulich edited the summary of this revision. (Show Details)Mar 2 2018, 10:48 AM

jgrulich edited the summary of this revision. (Show Details)Mar 2 2018, 10:51 AM

jgrulich edited the summary of this revision. (Show Details)

jgrulich edited the summary of this revision. (Show Details)Mar 2 2018, 11:06 AM

jgrulich added dependencies: D1231: Add Remote Access interface to KWayland, D1230: GBM remote access support for KWin.

Kanedias added a task: T7785: PipeWire support in remote access to KWin.Mar 3 2018, 5:46 PM

Kanedias mentioned this in T7785: PipeWire support in remote access to KWin.Mar 3 2018, 6:28 PM

Kanedias added inline comments.Mar 4 2018, 10:50 AM

src/screencast.cpp
329	Is this code actually reachable?
409	We can use `QTimer::singleShot` here, no?
443	You can use `QScopedPointer<ScreenChooserDialog, QScopedPointerDeleteLater>` when you define the dialog to avoid duplication of `->deleteLater()` calls
src/screencast.h
97	Some methods have first letter upper-case, some not, why?
135	This is not a list ,this is a map. The variable name is confusing.

I'd wrap some pipewire structures into some kind of PipewirePointer like WaylandPointer in KWayland does. But maybe I'm too picky, overall this stuff looks really good!

jgrulich marked 4 inline comments as done.Mar 5 2018, 7:37 AM

jgrulich added inline comments.

src/screencast.h
97	See [1]. We are supposed to reimplement this interface and these methods are defined by xdg-desktop-portal so we have to follow their naming. [1] - https://github.com/flatpak/xdg-desktop-portal/blob/master/data/org.freedesktop.impl.portal.ScreenCast.xml

Fix issues mentioned in review

Screen chooser dialog will be deleted automatically

Kanedias accepted this revision.Mar 7 2018, 2:23 PM

This revision is now accepted and ready to land.Mar 7 2018, 2:23 PM

I'll push it once your GBM stuff is merged, thank you for your review.

Provide a Findgbm.cmake module file. For example https://cgit.kde.org/kwin.git/tree/cmake/modules/Findgbm.cmake

src/screencaststream.h
35	With self-compiled Pipewire 0.1.9 I needed this to be `<spa/lib/pod.h>`.

Add Findgbm.cmake module and fix build against newer Pipewire

In D10965#230054, @romangg wrote:

Provide a Findgbm.cmake module file. For example https://cgit.kde.org/kwin.git/tree/cmake/modules/Findgbm.cmake

Weird it wasn't pushed, I had that module file locally since the beginning.

Both required reviews for GBM stuff have been already merged, can we proceed with this one? If there are no objections I will push it.

From me there are no objections. I haven't looked in detail through the code.

I could compile, but not test it because according to our last discussion the official Ubuntu xdp PPA isn't compiled with Pipewire support. So if this diff lands it would be good if Pipewire support in the PPA follows swiftly so people on Kubuntu/Neon can test it this way and we don't get needless bug reports.

If there are no objections I will move forward with this review.

Closed by commit R838:76811d65478e: Add screen cast portal (authored by jgrulich). · Explain WhyApr 5 2018, 5:48 AM

This revision was automatically updated to reflect the committed changes.

		Path
M		CMakeLists.txt (26 lines)
A	M	cmake/modules/Findepoxy.cmake (56 lines)
A	M	cmake/modules/Findgbm.cmake (125 lines)
M		data/kde.portal (2 lines)
M		src/CMakeLists.txt (13 lines)
M		src/access.cpp (2 lines)
M		src/accessdialog.cpp (2 lines)
M		src/appchooser.cpp (2 lines)
M		src/appchooserdialog.cpp (2 lines)
P		src/desktopportal.h (2 lines) Copied to src/session.h
P		src/desktopportal.cpp (3 lines) Copied to src/screenchooserdialog.h
M		src/email.cpp (2 lines)
M		src/filechooser.cpp (2 lines)
M		src/inhibit.cpp (2 lines)
M		src/notification.cpp (2 lines)
M		src/print.cpp (2 lines)
M		src/request.cpp (2 lines)
A	M	src/screencast.h (156 lines)
A	M	src/screencast.cpp (573 lines)
A	M	src/screencaststream.h (99 lines)
A	M	src/screencaststream.cpp (419 lines)
P		src/screenchooserdialog.h (42 lines) Copied from src/desktopportal.cpp
A	M	src/screenchooserdialog.cpp (83 lines)
A	M	src/screenchooserdialog.ui (47 lines)
P		src/session.h (43 lines) Copied from src/desktopportal.h
A	M	src/session.cpp (126 lines)
M		src/xdg-desktop-portal-kde.cpp (2 lines)

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	28384	688a88b		Mar 2 2018, 10:42 AM	★	★
Diff 2	28668	688a88b	Fix issues mentioned in review	Mar 5 2018, 7:39 AM	★	★
Diff 3	28670	688a88b	Screen chooser dialog will be deleted automatically	Mar 5 2018, 7:41 AM	★	★
Diff 4	30014	688a88b	Add Findgbm.cmake module and fix build against newer Pipewire	Mar 20 2018, 5:11 PM	★	★
Diff 5	31352	7adf7aa	R838:76811d65478ebd65869f1abad6e158aae14b29b4	Apr 5 2018, 5:48 AM	★	★

Add screen cast portal
ClosedPublic
Actions

Details

Diff Detail

Revision Contents
Changeset List

Diff 31352

CMakeLists.txt

cmake/modules/Findepoxy.cmake

cmake/modules/Findgbm.cmake

data/kde.portal

src/CMakeLists.txt

src/access.cpp

src/accessdialog.cpp

src/appchooser.cpp

src/appchooserdialog.cpp

src/desktopportal.h

src/desktopportal.cpp

src/email.cpp

src/filechooser.cpp

src/inhibit.cpp

src/notification.cpp

src/print.cpp

src/request.cpp

src/screencast.h

src/screencast.cpp

src/screencaststream.h

src/screencaststream.cpp

src/screenchooserdialog.h

src/screenchooserdialog.cpp

src/screenchooserdialog.ui

src/session.h

src/session.cpp

src/xdg-desktop-portal-kde.cpp

Add screen cast portalClosedPublicActions

Details

Diff Detail

Revision ContentsChangeset List

Diff 31352

CMakeLists.txt

cmake/modules/Findepoxy.cmake

cmake/modules/Findgbm.cmake

data/kde.portal

src/CMakeLists.txt

src/access.cpp

src/accessdialog.cpp

src/appchooser.cpp

src/appchooserdialog.cpp

src/desktopportal.h

src/desktopportal.cpp

src/email.cpp

src/filechooser.cpp

src/inhibit.cpp

src/notification.cpp

src/print.cpp

src/request.cpp

src/screencast.h

src/screencast.cpp

src/screencaststream.h

src/screencaststream.cpp

src/screenchooserdialog.h

src/screenchooserdialog.cpp

src/screenchooserdialog.ui

src/session.h

src/session.cpp

src/xdg-desktop-portal-kde.cpp

Add screen cast portal
ClosedPublic
Actions

Revision Contents
Changeset List