Build-time test failure in 0.8.0: SelectSources called without persist_mode, restore_token

[smcv]

While trying to update Debian's libportal to 0.8.0 I saw a build-time test failure.

In test_create_session it seems that SelectSources was unexpectedly called without specifying persist_mode:

>       assert list(options.keys()) == [
            "handle_token",
            "types",
            "multiple",
            "cursor_mode",
            "persist_mode",
            "restore_token",
        ]
E       AssertionError: assert [dbus.String('handle_token'), dbus.String('types'), dbus.String('multiple'), dbus.String('cursor_mode')] == ['handle_token', 'types', 'multiple', 'cursor_mode', 'persist_mode', 'restore_token']
E         
E         Right contains 2 more items, first extra item: 'persist_mode'
E         
E         Full diff:
E           [
E         +     dbus.String('handle_token'),
E         +     dbus.String('types'),
E         +     dbus.String('multiple'),
E         +     dbus.String('cursor_mode'),
E         -     'handle_token',
E         -     'types',
E         -     'multiple',
E         -     'cursor_mode',
E         -     'persist_mode',
E         -     'restore_token',
E           ]

and similarly in test_create_session_restore, it seems that SelectSources was called without persist_mode or restore_token:

>       assert list(options.keys()) == [
            "handle_token",
            "types",
            "multiple",
            "cursor_mode",
            "persist_mode",
            "restore_token",
        ]
E       AssertionError: assert [dbus.String('handle_token'), dbus.String('types'), dbus.String('multiple'), dbus.String('cursor_mode')] == ['handle_token', 'types', 'multiple', 'cursor_mode', 'persist_mode', 'restore_token']
E         
E         Right contains 2 more items, first extra item: 'persist_mode'
E         
E         Full diff:
E           [
E         +     dbus.String('handle_token'),
E         +     dbus.String('types'),
E         +     dbus.String('multiple'),
E         +     dbus.String('cursor_mode'),
E         -     'handle_token',
E         -     'types',
E         -     'multiple',
E         -     'cursor_mode',
E         -     'persist_mode',
E         -     'restore_token',
E           ]

(This is with #157 to get the full diff between lists of items, but I was seeing the same failure with plain 0.8.0.)

[smcv]

With some debug hacked in:

diff --git a/libportal/remote.c b/libportal/remote.c
index f631f07..7840895 100644
--- a/libportal/remote.c
+++ b/libportal/remote.c
@@ -151,6 +151,10 @@ select_sources (CreateCall *call)
       if (call->restore_token)
         g_variant_builder_add (&options, "{sv}", "restore_token", g_variant_new_string (call->restore_token));
     }
+  else
+    {
+      g_debug ("Screencast interface is old: %u", call->portal->screencast_interface_version);
+    }
   g_dbus_connection_call (call->portal->bus,
                           PORTAL_BUS_NAME,
                           PORTAL_OBJECT_PATH,
@@ -243,6 +247,10 @@ select_devices (CreateCall *call)
       if (call->restore_token)
         g_variant_builder_add (&options, "{sv}", "restore_token", g_variant_new_string (call->restore_token));
     }
+  else
+    {
+      g_debug ("RemoteDesktop interface is old: %u", call->portal->remote_desktop_interface_version);
+    }
   g_dbus_connection_call (call->portal->bus,
                           PORTAL_BUS_NAME,
                           PORTAL_OBJECT_PATH,
@@ -382,6 +390,7 @@ get_remote_desktop_interface_version_returned (GObject *object,

   g_variant_get_child (ret, 0, "v", &version_variant);
   call->portal->remote_desktop_interface_version = g_variant_get_uint32 (version_variant);
+  g_debug ("RemoteDesktop interface version is %u", call->portal->remote_desktop_interface_version);

   create_session (call);
 }
@@ -423,6 +432,7 @@ get_screencast_interface_version_returned (GObject *object,

   g_variant_get_child (ret, 0, "v", &version_variant);
   call->portal->screencast_interface_version = g_variant_get_uint32 (version_variant);
+  g_debug ("Screencast interface version is %u", call->portal->screencast_interface_version);

   create_session (call);
 }

it seems that the problem here is that the screencast interface version is never retrieved in this test:

(process:656394): libportal-DEBUG: 16:43:20.199: RemoteDesktop interface version is 2
...
(process:656394): libportal-DEBUG: 16:43:21.207: Screencast interface is old: 0

probably a regression in a1530a98 "remote: fix screencasting on wlroots-based compositors" (#150).

In that change, @MStarvik wrote:

xdp_portal_create_screencast_session fails if the RemoteDesktop interface is unavailable, which is currently the case on all wlroots-based compositors. xdp_portal_create_screencast_session queries the version of the RemoteDesktop interface despite this interface not beings used in the screencast session.

I think this is probably a true statement? It is possible to implement screencasting without also implementing remote-desktop.

But @MStarvik also wrote:

Likewise, xdp_portal_create_remote_desktop_session queries the version of the ScreenCast interface despite this interface not being used in the remote desktop session

and I don't think that's true. The documentation for the remote desktop interface specifically says that you may call certain screencast methods on its sessions:

A remote desktop session may only be started and stopped with this interface, but you can use the Session object created with this method together with certain methods on the ScreenCast and Clipboard interfaces. Specifically, you can call org.freedesktop.portal.ScreenCast.SelectSources to also get screen content, and org.freedesktop.portal.ScreenCast.OpenPipeWireRemote to acquire a file descriptor for a PipeWire remote. See ScreenCast for more information on how to use those methods. To capture clipboard content, you can call org.freedesktop.portal.Clipboard.RequestClipboard. See Clipboard for more information on the clipboard integration. — https://flatpak.github.io/xdg-desktop-portal/docs/doc-org.freedesktop.portal.RemoteDesktop.html

cc @MStarvik @whot @GeorgesStavracas

[smcv]

If I'm reading correctly, my conclusion is that this is asymmetric: creating a screencasting session should not query the remote-desktop interface version (to avoid breaking screencasting from x-d-p-wlroots), but creating a remote desktop session should query the screencasting interface version.

[eli-schwartz]

probably a regression in a1530a9 "remote: fix screencasting on wlroots-based compositors" (#150).

As I analyzed in the previous report, it is definitely a regression there as I git bisected it...

[MStarvik]

The way i interpret org.freedesktop.portal.RemoteDesktop.html is that the RemoteDesktop interface may be used in conjunction with the ScreenCast interface to provide remote desktop functionality, but that the two interfaces are independent.

I guess it's up to the owner whether this library should impose an interdependence between the two interfaces that is not described in the standard, but i don't see why it's necessary; if you need both the ScreenCast interface and the RemoteDesktop interface, the solution is to create both a ScreenCast session and a RemoteDesktop session.

The test creates a RemoteDesktop session and calls SelectSources on it. Since SelectSources is not part of the RemoteDesktop interface i think the expected behavior should be that the call fails.

[GeorgesStavracas]

The two interfaces are independent, in the sense that you can create a remote desktop session without ever interacting with the screencasting portal. But the screencasting portal can be used with remote desktop sessions as well.

[GeorgesStavracas]

Can you folks confirm that https://github.com/flatpak/libportal/pull/159 fixes the test failure? It certainly does for me.

I'll work on making CI run tests after we figure this one out.

[smcv]

Can you folks confirm that #159 fixes the test failure? It certainly does for me.

Yes it does

[MStarvik]

But the screencasting portal can be used with remote desktop sessions as well.

I agree, but i don't think the ScreenCast interface should be required in order to use the RemoteDesktop interface, considering this is not required by the XDG Desktop Portal standard. This is what i changed in #150, and is reverted by #159.

The RemoteDesktop interface is strictly for injecting user input, and i don't think it makes sense to expose screencasting to a sandboxed application that only needs to inject user input.

[smcv]

The test creates a RemoteDesktop session and calls SelectSources on it. Since SelectSources is not part of the RemoteDesktop interface i think the expected behavior should be that the call fails.

No, the documentation of the interface specifically says that you can call SelectSources (and a couple of other methods) on a remote desktop session.

The RemoteDesktop interface is strictly for injecting user input, and i don't think it makes sense to expose screencasting to a sandboxed application that only needs to inject user input.

If that was its intended purpose then it would be called InputInjection or something. If you think an input-injection-but-not-screen-capture interface is something that should exist, then https://github.com/flatpak/xdg-desktop-portal would be the place to ask for that, but libportal is not the place to ask for changing the design of the portal services, and there is currently no input-injection-but-not-screen-capture interface.

(I'm not sure what would use such an interface. Possibly Steam Input or other user-space, application-level input drivers? But Steam also has remote-desktop-like functionality, "Remote Play", so it wants RemoteDesktop at least some of the time anyway...)

[MStarvik]

The name of the interface is indeed misleading, and the documentation could be worded better, but what methods are or are not part of the RemoteDesktop interface not a matter of interpretation. The interface is clearly defined in the standard, and does not include the SelectSources method. This is easily confirmed by introspecting it's dbus object using, for example, busctl --user introspect org.freedesktop.portal.Desktop /org/freedesktop/portal/desktop.

Whether this library - which is an abstraction of the underlying dbus interface - should include the functionality exposed by the ScreenCast interface in it's abstraction of the RemoteDesktop interface is matter of taste, and our tastes apparently differ.

Personally, i think this library should include an almost 1-to-1 interface to the underlying dbus object. Higher-level abstractions, such as a remote desktop session that includes the functionality exposed by both ScreenCast and RemoteDesktop, should be built on top of the low-level interface.

I would be happy to continue this discussion elsewhere if this is not the right place.

[whot]

No, the documentation of the interface specifically says that you can call SelectSources (and a couple of other methods) on a remote desktop session.

afaict the libportal API doesn't directly allow this right now so there may be a workaround. The entry points are only the create_session calls but there's no xdp_session_select_sources direct API. So I think a simple fix (on top of 58d45670b3ba8ada69c7d13b320044893024fc46) might be

diff --git ./libportal/remote.c ../libportal/remote.c
index 4992513c56fa..eb8d353aec27 100644
--- ./libportal/remote.c
+++ ../libportal/remote.c
@@ -424,7 +424,8 @@ get_remote_desktop_interface_version_returned (GObject *object,
   g_variant_get_child (ret, 0, "v", &version_variant);
   call->portal->remote_desktop_interface_version = g_variant_get_uint32 (version_variant);

-  if (call->portal->screencast_interface_version == 0)
+  if ((call->type != XDP_SESSION_REMOTE_DESKTOP || call->outputs != XDP_OUTPUT_NONE) &&
+     call->portal->screencast_interface_version == 0)
     get_screencast_interface_version (call);
   else
     create_session (call);

IOW only query the version if we may otherwise run select_sources() on it. havent' tested that one though.