Bug 223195 - [GStreamer] Videos start playing muted in epiphany with no unmute icon visible in tab, webkit_web_view_get_is_muted() returns incorrect results
Summary: [GStreamer] Videos start playing muted in epiphany with no unmute icon visibl...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: WebKit Nightly Build
Hardware: PC Linux
: P2 Normal
Assignee: Philippe Normand
URL:
Keywords: InRadar
: 223433 (view as bug list)
Depends on:
Blocks:
 
Reported: 2021-03-15 10:56 PDT by hoboprimate
Modified: 2021-04-07 02:59 PDT (History)
19 users (show)

See Also:


Attachments
Patch (5.33 KB, patch)
2021-03-28 03:52 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (5.69 KB, patch)
2021-03-28 05:32 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (9.46 KB, patch)
2021-04-02 02:26 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (9.55 KB, patch)
2021-04-07 01:54 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (9.55 KB, patch)
2021-04-07 01:59 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description hoboprimate 2021-03-15 10:56:31 PDT
Fedora 34 (alpha)
Epiphany 40.beta
gstreamer1-1.18.2-2
pipewire-0.3.23-2

Videos played on epiphany start with sound muted and no icon indication in the tab. One has to go to settings > sound and unmute the specific WebKitWebProcess . Can be seen on youtube.com and in other places.

Also, a related issue seems to be that (sometimes) the unmute sound icon is not shown in the tab. As in, when playing a video with sound and after clicking the sound icon on the tab to mute, no unmute icon replaces it.
Comment 1 Philippe Normand 2021-03-17 04:46:42 PDT
Works as expected in Ephy TP 40.rc-8-g7ff912064+
Comment 2 hoboprimate 2021-03-18 03:22:35 PDT
Can't reproduce it anymore since updating to epiphany 40.rc .

Should I close this?
Comment 3 Michael Catanzaro 2021-03-18 07:39:37 PDT
Yeah if you can't reproduce, let's close it. That said, there's little chance that an Epiphany change is related....
Comment 4 Michael Catanzaro 2021-03-19 07:32:03 PDT
Reopening because I hit this today myself on youtube.com. Either the WebKitWebProcess set its own volume to zero, or pipewire decided to for some reason.

Sadly I'm not sure how to reproduce.
Comment 5 Michael Catanzaro 2021-03-19 07:42:42 PDT
Ah sorry, it only happens when I mute a YouTube video and then quit Epiphany when the video is playing, such that the YouTube tab is focused when I close the window. If any other tab is focused when closing the browser, then it is not muted when I reopen the browser.

That's weird, but the bigger problem is there's no way to unmute. That's because webkit_web_view_is_playing_audio() is broken, which we just discovered recently. I was planning to report a separate bug for that, but maybe I'll repurpose this one, since I think that's what we're really dealing with here....
Comment 6 Michael Catanzaro 2021-03-19 07:50:12 PDT
(In reply to hoboprimate from comment #0)
> Fedora 34 (alpha)

It could be related to pulseaudio -> pipewire? Alex cannot reproduce on F33, but Jan-Michael and I can both reproduce on F34. The bug doesn't seem to be due to any change in WebKit.
Comment 7 Michael Catanzaro 2021-03-19 08:38:55 PDT
Problem is that after muting the web view, webkit_web_view_is_playing_audio() returns FALSE. That seems kinda reasonable, but it's not what Epiphany and MiniBrowser expect. Both expect use webkit_web_view_is_playing_audio() to decide whether to show the audio indicator, so it needs to return TRUE if audio is playing but muted.

Specifically, MiniBrowser does this:

g_object_bind_property(tab->webView, "is-playing-audio", tab->titleAudioButton, "visible", G_BINDING_DEFAULT | G_BINDING_SYNC_CREATE);

so once the video is muted, the indicator disappears and there's no way to unmute.
Comment 8 Michael Catanzaro 2021-03-19 09:27:14 PDT
I found further confirmation in https://bugs.webkit.org/show_bug.cgi?id=176119#c46, where Carlos says setting WebKitWebView:is-muted does not change the state of WebKitWebView:is-playing-audio. So that does appear to be the intended and past behavior.

Also, our API docs for WebKitWebView:is-muted say:

"Whether the WebKitWebView audio is muted. When TRUE, audio is silenced. It may still be playing, i.e. “is-playing-audio” may be TRUE."

So webkit_web_view_get_is_muted() and WebKitWebView:is-muted are returning incorrect results.
Comment 9 Michael Catanzaro 2021-03-19 13:18:20 PDT
(In reply to Michael Catanzaro from comment #8)
> So webkit_web_view_get_is_muted() and WebKitWebView:is-muted are returning
> incorrect results.

Sorry, Jan-Michael noticed this is a typo. There's nothing wrong with is-muted. The problem is WebKitWebView:is-playing-audio and webkit_web_view_is_playing_audio() return FALSE when the video is muted, but it needs to return TRUE.
Comment 10 Philippe Normand 2021-03-20 03:00:42 PDT
*** Bug 223433 has been marked as a duplicate of this bug. ***
Comment 11 Philippe Normand 2021-03-20 04:26:07 PDT
(In reply to Michael Catanzaro from comment #6)
> (In reply to hoboprimate from comment #0)
> > Fedora 34 (alpha)
> 
> It could be related to pulseaudio -> pipewire? Alex cannot reproduce on F33,
> but Jan-Michael and I can both reproduce on F34. The bug doesn't seem to be
> due to any change in WebKit.

Can't reproduce on F33 either. I think this could be related with pipewire indded. Please provide logs, as usual.
Comment 12 Philippe Normand 2021-03-20 05:40:14 PDT
OK I've rebased to F34. Can reproduce the issue now:

0:02:25.077821336    69       0x652550 INFO       webkitmediaplayer MediaPlayerPrivateGStreamer.cpp:1235:isMuted:<media-player-1> Player is muted: false
0:02:25.077855310    69       0x652550 INFO       webkitmediaplayer MediaPlayerPrivateGStreamer.cpp:1706:setMuted:<media-player-1> Setting muted state to true
0:02:25.078523841    69       0x652550 DEBUG      webkitmediaplayer MediaPlayerPrivateGStreamer.cpp:1717:notifyPlayerOfMute:<media-player-1> Notifying player of new mute value: false
0:02:25.078564079    69       0x652550 DEBUG      webkitmediaplayer MediaPlayerPrivateGStreamer.cpp:1653:setVolume:<media-player-1> Setting volume: 0.530206
0:02:25.079093901    69       0x652550 DEBUG      webkitmediaplayer MediaPlayerPrivateGStreamer.cpp:1684:volumeChangedCallback:<media-player-1> Volume changed to: 0.530206
0:02:25.082536880    69       0x67c2a0 DEBUG      webkitmediaplayer MediaPlayerPrivateGStreamer.cpp:1684:volumeChangedCallback:<media-player-1> Volume changed to: 0.530206
0:02:25.083244316    69       0x652550 DEBUG      webkitmediaplayer MediaPlayerPrivateGStreamer.cpp:1717:notifyPlayerOfMute:<media-player-1> Notifying player of new mute value: true


So we call setMuted(true) and get a GObject notification from playbin but the value is false instead of true, and a while later another notification which is correct this time...
Comment 13 Philippe Normand 2021-03-20 06:38:06 PDT
(In reply to Michael Catanzaro from comment #9)
> (In reply to Michael Catanzaro from comment #8)
> > So webkit_web_view_get_is_muted() and WebKitWebView:is-muted are returning
> > incorrect results.
> 
> Sorry, Jan-Michael noticed this is a typo. There's nothing wrong with
> is-muted. The problem is WebKitWebView:is-playing-audio and
> webkit_web_view_is_playing_audio() return FALSE when the video is muted, but
> it needs to return TRUE.

MediaProducer::MediaStateFlags HTMLMediaElement::mediaState() const
{
...
    if (hasAudio && !muted() && volume())
        state |= IsPlayingAudio;
}

So that's why webkit_web_view_is_playing_audio() returns FALSE when the video is muted.
Comment 14 Michael Catanzaro 2021-03-20 07:53:51 PDT
(In reply to Philippe Normand from comment #13)
> MediaProducer::MediaStateFlags HTMLMediaElement::mediaState() const
> {
> ...
>     if (hasAudio && !muted() && volume())
>         state |= IsPlayingAudio;
> }
> 
> So that's why webkit_web_view_is_playing_audio() returns FALSE when the
> video is muted.

I found that yesterday and thought it looked concerning, given the API requires different behavior, but it clearly didn't work before now.

Any ideas what the path forward is? Do we dare try changing that...?
Comment 15 Philippe Normand 2021-03-20 08:17:14 PDT
(In reply to Michael Catanzaro from comment #14)
> (In reply to Philippe Normand from comment #13)
> > MediaProducer::MediaStateFlags HTMLMediaElement::mediaState() const
> > {
> > ...
> >     if (hasAudio && !muted() && volume())
> >         state |= IsPlayingAudio;
> > }
> > 
> > So that's why webkit_web_view_is_playing_audio() returns FALSE when the
> > video is muted.
> 
> I found that yesterday and thought it looked concerning, given the API
> requires different behavior, but it clearly didn't work before now.
> 
> Any ideas what the path forward is? Do we dare try changing that...?

I'm not sure what to do yet. I think there are 3 issues:

- muting the WebProcess from the GNOME settings makes the speaker icon disappear from the MB tab that is playing media
- there's a racy behavior where muting audio through the MB speaker icon sometimes makes it disappear
- if a WebProcess was muted from GNOME settings and the browser is closed, reopening it and playing media leads to muted audio and invisible speaker icon in the tab
Comment 16 Philippe Normand 2021-03-23 10:33:30 PDT
I've updated to Pipewire 0.3.24 and am now unable to reproduce this issue here. Can you check as well please?
Comment 17 Philippe Normand 2021-03-23 10:34:33 PDT
Dang it, scratch this, it's a racy speaker icon disappearance.
Comment 18 Philippe Normand 2021-03-28 03:52:54 PDT
Created attachment 424491 [details]
Patch
Comment 19 Philippe Normand 2021-03-28 05:32:48 PDT
Created attachment 424494 [details]
Patch
Comment 20 Eric Carlson 2021-03-28 13:04:17 PDT
Comment on attachment 424494 [details]
Patch

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

> Source/WebCore/html/HTMLMediaElement.cpp:7690
> +    bool isPlayingAudio = hasAudio && volume();

Shouldn't this be `bool isPlayingAudio = hasAudio && !muted() && volume()` to avoid changing behavior on non-GStreamer ports?

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1678
> +    auto volume = this->volume();
> +    auto oldVolume = volume;
> +
>      // get_volume() can return values superior to 1.0 if the user
>      // applies software user gain via third party application (GNOME
>      // volume control for instance).
>      volume = CLAMP(volume, 0.0, 1.0);

Nit: this would be sightly more efficient as:

auto oldVolume = this->volume();
auto volume = CLAMP(oldVolume, 0.0, 1.0);
Comment 21 Michael Catanzaro 2021-03-28 15:15:02 PDT
Comment on attachment 424494 [details]
Patch

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

>> Source/WebCore/html/HTMLMediaElement.cpp:7690
>> +    bool isPlayingAudio = hasAudio && volume();
> 
> Shouldn't this be `bool isPlayingAudio = hasAudio && !muted() && volume()` to avoid changing behavior on non-GStreamer ports?

On non-GStreamer ports, && !muted is tested on the next line. It's an #if !USE(GSTREAMER) condition.
Comment 22 Carlos Garcia Campos 2021-03-28 23:22:45 PDT
Comment on attachment 424494 [details]
Patch

Would it be possible to add a unit test to check the fixed behavior?
Comment 23 Philippe Normand 2021-03-30 06:25:36 PDT
(In reply to Carlos Garcia Campos from comment #22)
> Comment on attachment 424494 [details]
> Patch
> 
> Would it be possible to add a unit test to check the fixed behavior?

Yes I think so, at least update existing API tests. The current ones don't test mixed usage of the is_playing_audio and is_muted APIs, which is not great because in practice we do use both in interesting ways.
Comment 24 Philippe Normand 2021-04-02 02:26:14 PDT
Created attachment 424998 [details]
Patch
Comment 25 Michael Catanzaro 2021-04-02 07:20:30 PDT
Comment on attachment 424998 [details]
Patch

New API test is r=me, though 50ms * 20 = 1 second, which is a little long for a test. (I would be more concerned if this were a layout test. We don't have so many API tests that one extra second would be a big deal.)

(You already have Eric's r+ for the media changes.)
Comment 26 Philippe Normand 2021-04-07 01:54:21 PDT
Created attachment 425368 [details]
Patch
Comment 27 Philippe Normand 2021-04-07 01:54:55 PDT
(In reply to Michael Catanzaro from comment #25)
> Comment on attachment 424998 [details]
> Patch
> 
> New API test is r=me, though 50ms * 20 = 1 second, which is a little long
> for a test. (I would be more concerned if this were a layout test. We don't
> have so many API tests that one extra second would be a big deal.)
> 
> (You already have Eric's r+ for the media changes.)

I´ve changed it to 500ms.
Comment 28 Philippe Normand 2021-04-07 01:59:01 PDT
Created attachment 425369 [details]
Patch
Comment 29 EWS 2021-04-07 02:58:58 PDT
Committed r275600: <https://commits.webkit.org/r275600>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 425369 [details].
Comment 30 Radar WebKit Bug Importer 2021-04-07 02:59:25 PDT
<rdar://problem/76330028>