[GLIB] MediaSession is not enabled

• Component: Media
• Status: RESOLVED FIXED

[Diego Pino]

The following tests are failing:
  imported/w3c/web-platform-tests/mediasession/idlharness.window.html [ Failure ]
  imported/w3c/web-platform-tests/mediasession/mediametadata.html [ Failure ]
  imported/w3c/web-platform-tests/mediasession/playbackstate.html [ Failure ]
  imported/w3c/web-platform-tests/mediasession/positionstate.html [ Failure ]
  imported/w3c/web-platform-tests/mediasession/setactionhandler.html [ Failure ]

[Diego Pino]

Created attachment 411921 [details]
Diff for each reported test failing

[Diego Pino]

Created attachment 411922 [details]
Diff for each reported test failing

[Diego Pino]

Two more MediaSession tests are failing:

  media/media-session/mock-currentPosition.html [ Failure ]
  media/media-session/mock-actionHandlers.html [ Timeout ]

At this moment MediaSession is an experimental feature not enabled for GTK or WPE, so I will skip MediaSession tests for those platforms.

[Radar WebKit Bug Importer]

<rdar://problem/70740119>

[Philippe Normand]

Should this be implemented as a MPRIS2 dbus object? Seems like this is what Chromium on Linux does.

Spec: https://specifications.freedesktop.org/mpris-spec/latest/

[Philippe Normand]

WIP branch: https://github.com/philn/webkit/commits/media-session

TODO: actual backend...

[Philippe Normand]

Created attachment 434760 [details]
Patch

[EWS Watchlist]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See https://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Philippe Normand]

Created attachment 434761 [details]
Patch

[Patrick Griffis]

Comment on attachment 434761 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=434761&action=review

> Source/WebCore/platform/audio/glib/MediaSessionManagerGLib.cpp:283
> +    auto instanceId = makeString("org.mpris.MediaPlayer2.webkit.instance", getpid());

If this is changed to `org.mpris.MediaPlayer2.$APP_ID.webkit.instance` (where APP_ID is g_application_get_application_id()). Then this will work in flatpak without granting any extra permissions.

[Patrick Griffis]

View in context: https://bugs.webkit.org/attachment.cgi?id=434761&action=review

> Source/WebCore/platform/audio/glib/MediaSessionManagerGLib.cpp:283
> +    auto instanceId = makeString("org.mpris.MediaPlayer2.webkit.instance", getpid());

If this is changed to `org.mpris.MediaPlayer2.$APP_ID.webkit.instance` (where APP_ID is g_application_get_application_id()). Then this will work in flatpak without granting any extra permissions.

[Philippe Normand]

Comment on attachment 434761 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=434761&action=review

>> Source/WebCore/platform/audio/glib/MediaSessionManagerGLib.cpp:283
>> +    auto instanceId = makeString("org.mpris.MediaPlayer2.webkit.instance", getpid());
> 
> If this is changed to `org.mpris.MediaPlayer2.$APP_ID.webkit.instance` (where APP_ID is g_application_get_application_id()). Then this will work in flatpak without granting any extra permissions.

This runs in the WebProcess currently, so I'm not sure g_application_get_application_id() would return anything useful? I can test anyway, thanks for the tip :)

[Michael Catanzaro]

(In reply to Philippe Normand from comment #12)
> This runs in the WebProcess currently, so I'm not sure
> g_application_get_application_id() would return anything useful? I can test
> anyway, thanks for the tip :)

It won't, because the web process does not run a GApplication instance. We'll need to pass the app ID from the UI process to the web process.

I asked Patrick to look because I saw --own-name. Extra permissions like --talk-name or --own-name will sooner or later cause applications to be displayed as "unsafe," and most applications won't use them anyway. It would be better to get this right from the beginning and figure out how to do it without requiring extra permissions in the flatpak manifest. In this case, it seems like passing the app ID to the web process should suffice for flatpak.

The next question is: does the web process have permission to own that name when running under the bubblewrap sandbox? Probably it requires changes in BubblewrapLauncher.cpp?

[Patrick Griffis]

(In reply to Michael Catanzaro from comment #13)
 
> The next question is: does the web process have permission to own that name
> when running under the bubblewrap sandbox? Probably it requires changes in
> BubblewrapLauncher.cpp?

It should not have permission and would require changes.

Ideally just following the same as flatpak where it allows app-id matched ownership.

[Michael Catanzaro]

Comment on attachment 434761 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=434761&action=review

> Source/WebCore/platform/audio/glib/MediaSessionManagerGLib.cpp:109
> +#define DBUS_MPRIS_SERVICE_NAME "org.mpris.MediaPlayer2.webkit"

This one is unused.

> Source/WebCore/platform/audio/glib/MediaSessionManagerGLib.cpp:111
> +#define DBUS_MPRIS_INTERFACE "org.mpris.MediaPlayer2"

This one too.

> Source/WebCore/platform/audio/glib/MediaSessionManagerGLib.cpp:248
> +        // FIXME: Would be nice to get UIProcess name, somehow.
> +        return g_variant_new_string("WebKit");

What we want here is g_get_application_name(), but this has the same problem as the app ID. I doubt this is the first time we've wanted this stuff in the web process. I bet we could send both in the WebProcessCreationParameters.

> Source/WebCore/platform/audio/glib/MediaSessionManagerGLib.cpp:301
> +        WTFLogAlways("Failed at parsing XML Interface definition: %s", error ? error->message : "Unknown Error");

Nit: I prefer to use g_warning() for unexpected errors in platform-specific code like this. A nice benefit is that in the unlikely event that it somehow breaks, it will fail API tests.

Bonus: can get rid of the ternary here, because you don't actually need to check if error is set. The API guarantees that error will be set if m_mprisInterface is NULL. "Error must be set if function fails" is convention for all GLib-style APIs, unless otherwise stated. Occasionally this will lead to crashes when the error is missing, but if so that would be a bug in the function you're calling.

In this case, there's actually an explicit guarantee in the docs for g_dbus_node_info_new_for_xml() -- it returns "[a] GDBusNodeInfo structure or NULL if error is set" -- but it's still guaranteed even if it's not written out.

> Source/WebKit/UIProcess/API/glib/WebKitSettings.cpp:1559
> +     * Enable or disable support for media session on pages. This feature allows web-pages to

web pages

> Source/WebKit/UIProcess/API/gtk/WebKitSettings.h:521
> +WEBKIT_API gboolean
> +webkit_settings_get_enable_media_session                       (WebKitSettings *settings);
> +
> +WEBKIT_API void
> +webkit_settings_set_enable_media_session                       (WebKitSettings *settings,
> +                                                                gboolean        enabled);

My concern here is that these APIs will be public, but they won't do anything in releases until it no longer depends on ${ENABLE_EXPERIMENTAL_FEATURES}. Reminds me of the broken webkit_settings_[g,s]et_media_stream() API, which has been exposed since WebKitGTK 2.4, but has never worked in releases because media stream is still disabled at build time. What I would do instead is enable the setting at runtime so that the public API isn't required for testing it. Then in the future when we're finally ready to enable media session in releases, that's when we would want to add these APIs.

[Philippe Normand]

Created attachment 435570 [details]
Patch

[Philippe Normand]

Ping?

[Adrian Perez]

Comment on attachment 435570 [details]
Patch

Patch LGTM with a couple of comments below, and also it's probably
a good idea that somebody who knows their way better around D-Bus
takes a look as well.


View in context: https://bugs.webkit.org/attachment.cgi?id=435570&action=review

> Source/WTF/wtf/glib/GRefPtr.h:28
> +#include <gio/gio.h>

Maybe we could typedef the GDBusNodeInfo type to avoid including all
of <gio/gio.h>, which will increase build times:

  extern "C" {
  typedef struct _GDBusNodeInfo GDBusNodeInfo;
  };

WDYT?

(Technically the same could be done for all the rest of types pulled
from <glib/glib.h>, but that would be a side quest for another day :)

> Source/WebCore/platform/audio/glib/MediaSessionManagerGLib.cpp:33
> +static const char s_mprisInterface[] =

I would add a comment next before this with a link to the spec

  https://specifications.freedesktop.org/mpris-spec/latest/

:)

> Source/WebCore/platform/audio/glib/MediaSessionManagerGLib.cpp:143
> +    }());

It seems a bit of overkill to create an std::unordered_map for only seven elements,
it's a waste of memory. This asks for WTF::SortedArrayMap, which uses linear search
below a certain size threshold, or binary search otherwise.

(And yes, we should probably use WTF::SortedArrayMap in more places to save some
memory!)

[Philippe Normand]

Created attachment 436268 [details]
Patch

[Adrian Perez]

Comment on attachment 436268 [details]
Patch

Patch LGTM, it would be nice if someone else could rubber-stamp as well :)

[Philippe Normand]

Ping reviewers

[Michael Catanzaro]

Comment on attachment 436268 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=436268&action=review

Perhaps overly cynical, but I like to assume that any code that involves D-Bus is plagued with problems that we won't notice until users complain about crashes. But I didn't spot any myself, and there's only one way to find out. ;)

> Source/WebCore/ChangeLog:9
> +
> +

Extra blank line here

> Source/WTF/wtf/glib/GRefPtr.h:34
> +    GDBusNodeInfo* g_dbus_node_info_ref(GDBusNodeInfo*);
> +    void g_dbus_node_info_unref(GDBusNodeInfo*);

These two should be declared in the .cpp file, since that's where they're used.

> Source/WebCore/platform/audio/glib/MediaSessionManagerGLib.cpp:135
> +static inline std::optional<PlatformMediaSession::RemoteControlCommandType> getCommand(const char* name)

Why inline?

> Source/WebCore/platform/audio/glib/MediaSessionManagerGLib.cpp:173
> +enum class Property : uint8_t {

This is such a generic name, I would be worried about potential ODR violations if any other file has anything named "Property," especially now that distros are building with LTO enabled. Simplest solution is to name it something else, or you could enclose it in an anonymous namespace to restrict it to file scope:

namespace {

enum class Property : uint8_t {
    // ...
};

} // anonymous namespace

> Source/WebCore/platform/audio/glib/MediaSessionManagerGLib.cpp:234
> +    auto& manager = *reinterpret_cast<MediaSessionManagerGLib*>(userData);
> +    switch (property.value()) {
> +    case Property::NoProperty:
> +        ASSERT_NOT_REACHED();

I don't think this ASSERT is correct because it shouldn't be possible to trigger an assert by passing invalid input over D-Bus. ASSERT should mean programmer error in WebKit itself.

> Source/WebCore/platform/audio/glib/MediaSessionManagerGLib.cpp:319
> +        g_warning("Failed at root registration: %s", error->message);

Maybe "Failed to register MPRIS D-Bus object: %s" or something like that. "root registration" sounds meaningless to me.

> Source/WebCore/platform/audio/glib/MediaSessionManagerGLib.cpp:327
> +        g_warning("Failed at object registration: %s", error->message);

Could mention MPRIS here too? Same for the two warnings in MediaSessionManagerGLib::nameLost.

> Source/WebCore/platform/audio/glib/MediaSessionManagerGLib.cpp:484
> +    // FIXME: Fix this layering violation.
> +    if (auto element = HTMLMediaElement::bestMediaElementForRemoteControls(MediaElementSession::PlaybackControlsPurpose::NowPlaying))
> +        return &element->mediaSession();

The right way to do this without making Carlos Garcia sad is to add a "client" (delegate) interface outside WebCore/platform and then implement it here. It's a bit annoying to do, though. I guess you know all that already. I haven't seen any serious action on platform layering violations in a long time, so you probably won't be tarred and feathered if you leave it like this. I'd never have noticed had you not added the FIXME comment.

> Source/WebCore/platform/audio/glib/MediaSessionManagerGLib.cpp:537
> +    g_variant_builder_init(&builder, G_VARIANT_TYPE("a{sv}"));

Important: all of your calls to g_variant_builder_init() need to be paired with a g_variant_builder_clear(), otherwise they will leak. Same in MediaSessionManagerGLib::sessionStateChanged.

> Source/WebCore/platform/glib/RemoteCommandListenerGLib.h:30
> +    RemoteCommandListenerGLib(RemoteCommandListenerClient&);

explicit

> Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp:227
> +            g_error("Unable to own DBus MPRIS name in the WebKit sandbox: %s", error->message);

Don't you think crashing is pretty harsh? The code up above is fatal because there are going to be a *lot* of problems (e.g. broken fonts) if xdg-desktop-portal isn't working at all. But MPRIS? I think we can survive without MPRIS. So I would print a warning and move on if this isn't working.

Also: DBus -> D-Bus

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #22)
> > Source/WTF/wtf/glib/GRefPtr.h:34
> > +    GDBusNodeInfo* g_dbus_node_info_ref(GDBusNodeInfo*);
> > +    void g_dbus_node_info_unref(GDBusNodeInfo*);
> 
> These two should be declared in the .cpp file, since that's where they're
> used.

Actually, in the .cpp file you would have #included the gio header already, so you won't need the forward declarations after all, and you can drop the style check exception.

[Philippe Normand]

(In reply to Michael Catanzaro from comment #23)
> (In reply to Michael Catanzaro from comment #22)
> > > Source/WTF/wtf/glib/GRefPtr.h:34
> > > +    GDBusNodeInfo* g_dbus_node_info_ref(GDBusNodeInfo*);
> > > +    void g_dbus_node_info_unref(GDBusNodeInfo*);
> > 
> > These two should be declared in the .cpp file, since that's where they're
> > used.
> 
> Actually, in the .cpp file you would have #included the gio header already,
> so you won't need the forward declarations after all, and you can drop the
> style check exception.

Are you sure about that? What about the many others cpp units where we include GRefPtr.h? I'd rather keep the forward declarations. It's simpler.

[Philippe Normand]

Comment on attachment 436268 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=436268&action=review

>> Source/WebCore/platform/audio/glib/MediaSessionManagerGLib.cpp:537
>> +    g_variant_builder_init(&builder, G_VARIANT_TYPE("a{sv}"));
> 
> Important: all of your calls to g_variant_builder_init() need to be paired with a g_variant_builder_clear(), otherwise they will leak. Same in MediaSessionManagerGLib::sessionStateChanged.

Only in the cases where g_variant_builder_end() is not called IIUC? See https://docs.gtk.org/glib/method.VariantBuilder.end.html

[Michael Catanzaro]

(In reply to Philippe Normand from comment #24)
> Are you sure about that? What about the many others cpp units where we
> include GRefPtr.h? I'd rather keep the forward declarations. It's simpler.

I think I was very confused yesterday. Of course you are right. I was thinking the declarations were not actually used in the header, but they are.

(In reply to Philippe Normand from comment #25)
> Only in the cases where g_variant_builder_end() is not called IIUC? See
> https://docs.gtk.org/glib/method.VariantBuilder.end.html

You're right about this too!

[Philippe Normand]

Committed r282307 (241579@main): <https://commits.webkit.org/241579@main>

[Philippe Normand]

*** Bug 219341 has been marked as a duplicate of this bug. ***

[Philippe Normand]

(In reply to Patrick Griffis from comment #11)
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=434761&action=review
> 
> > Source/WebCore/platform/audio/glib/MediaSessionManagerGLib.cpp:283
> > +    auto instanceId = makeString("org.mpris.MediaPlayer2.webkit.instance", getpid());
> 
> If this is changed to `org.mpris.MediaPlayer2.$APP_ID.webkit.instance`
> (where APP_ID is g_application_get_application_id()). Then this will work in
> flatpak without granting any extra permissions.

Trying in Ephy Canary:

flatpak run --user org.gnome.Epiphany.Canary -p https://googlechrome.github.io/samples/media-session/video.html

** (WebKitWebProcess:2): WARNING **: 11:37:52.791: Unable to acquire MPRIS D-Bus session ownership for name org.mpris.MediaPlayer2.org.gnome.Epiphany.Canary

[Philippe Normand]

Probably this is because our SDK doesn't provide bwrap, so the bwrap launcher is disabled. I'll try to fix that...

[Michael Catanzaro]

The bubblewrap launcher is disabled under flatpak regardless. You get the flatpak-spawn subsandbox instead. Sounds like that D-Bus name might not be automatically exposed after all?

[Patrick Griffis]

(In reply to Michael Catanzaro from comment #31)
> The bubblewrap launcher is disabled under flatpak regardless. You get the
> flatpak-spawn subsandbox instead. Sounds like that D-Bus name might not be
> automatically exposed after all?

To subsandboxes, probably not. But the main sandbox has the permission which means we can extend `flatpak-spawn` to expose it in them.