Bug 96912 – [GStreamer] initial volume events on media elements could happen earlier

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Jussi Kukkonen (jku)]

playbin2 (when used on a Pulseaudio-running platform) will initialize the stream volume and mute state from pulseaudio at some point in the streams lifetime. this is a useful feature but currently it's slightly annoying as the HTMLAudioElement gets updated with that initial value only after playback starts. 

This leads to a bad user experience (click play and suddenly the volume changes) and we should be able to fix that. We already set the stream to GST_STATE_PAUSED asap after the url has been set: volume and mute state should be available at that point.

I'll cook up a patch.

[Jussi Kukkonen (jku)]

Hmm, I'm seeing a strange playbin issue: updated "volume" property is available on GST_STATE_PAUSED as I mentioned, but "mute" is not. That only happens some milliseconds later (maybe this is related to why bug 36299 includes the timeout hack, but that one is not long enough in this case).

So I was wrong, I don't have a good solution for this. Not sure whether I had misunderstood in the first place or because there's a bug in gstreamer

[Philippe Normand]

Can you upload your WIP patch?
There can totally be a bug in GStreamer, wouldn't be the first time we find an issue when dealing with a WebKit patch ;)

[Jussi Kukkonen (jku)]

(In reply to comment #2)
> Can you upload your WIP patch?
> There can totally be a bug in GStreamer, wouldn't be the first time we find an issue when dealing with a WebKit patch ;)

I actually got it to work by accident: it seems that getting the "mute" property first will not work, but doing anything else first makes it work (as  an example it starts to work even if I just do the same g_object_get() call twice). This does not sound like a very probable bug, but it is totally reproducable.

So the working patch that I'm uploading in a minute gets volume first and mute second. That works fine for me but doing it the other way round will fail: mute will have the uninitialized value.

There's a small issue still: there are two javascript "volumechange" events. This isn't entirely wrong (if both mute and volume changed) but best would be a single event I think.

[Jussi Kukkonen (jku)]

Created attachment 164525 [details]
Patch

[Jussi Kukkonen (jku)]

Created attachment 164527 [details]
simple test page

[Philippe Normand]

Comment on attachment 164525 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1161
> +                notifyPlayerOfVolumeChange();
> +                notifyPlayerOfMute();

These 2 are meant to be called from idle callbacks. Have you tried calling muteChanged() and volumeChanged() instead?
Also this can be done from updateAudioSink() I think.

[Jussi Kukkonen (jku)]

(In reply to comment #6)
> These 2 are meant to be called from idle callbacks. Have you tried calling muteChanged() and volumeChanged() instead?

I noticed this pattern. I didn't understand the reasoning but I did try it after finding bug 36299: the results are the same, things only work if volumeChanged() is called before muteChanged(). 

> Also this can be done from updateAudioSink() I think.

Yes, that looks like it makes sense.

[Jussi Kukkonen (jku)]

(In reply to comment #6)
> These 2 are meant to be called from idle callbacks. Have you tried calling muteChanged() and volumeChanged() instead?

Btw, I think not using idle timeouts is fine in this case as the they are called from GstBus "message" signal which is emitted in the main thread.

> Also this can be done from updateAudioSink() I think.

...and this isn't actually a good idea as we'd want the initialization to happen if the url is changed as well, right?

[Philippe Normand]

(In reply to comment #8)
> (In reply to comment #6)
> > These 2 are meant to be called from idle callbacks. Have you tried calling muteChanged() and volumeChanged() instead?
> 
> Btw, I think not using idle timeouts is fine in this case as the they are called from GstBus "message" signal which is emitted in the main thread.
> 

Alright.

> > Also this can be done from updateAudioSink() I think.
> 
> ...and this isn't actually a good idea as we'd want the initialization to happen if the url is changed as well, right?

Oh you reset that new member variable in ::load(). Right.

[Philippe Normand]

Jussi would you like to add a ChangeLog and check the media tests with your patch? It'd be nice, I think, to fix this bug.

[Jussi Kukkonen (jku)]

Created attachment 176937 [details]
Patch

[Jussi Kukkonen (jku)]

Comment on attachment 176937 [details]
Patch

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

> Source/WebCore/ChangeLog:15
> +        No new tests: media/event-attributes.html already tests media events.
> +        Unfortunately this test is flaky on EFL and linux (more specifically
> +        it is flaky on pulseaudio-using platforms: http://webkit.org/b/83704).

Additional comment on this: event-attributes.html result seems to have an addititional durationchange event nowadays (after playing event) but that is unrelated and also happens without this patch.

I've run the media tests on EFL and just as before, if the system volume is 100% and mute is off, I get the expected results for all.

[Jussi Kukkonen (jku)]

argh, pressed commit too soon...

So, the reason this languished so long was that I was hoping we could get the flakiness of the testing improved, but that turned out to be more difficult to solve. Once pulseaudio module-match actually works on common linux OSes it might be easier but before that it might not be worth it.

[Jussi Kukkonen (jku)]

Created attachment 176943 [details]
Improve Changelog

[WebKit Review Bot]

Comment on attachment 176943 [details]
Improve Changelog

Clearing flags on attachment: 176943

Committed r136382: <http://trac.webkit.org/changeset/136382>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Jussi Kukkonen (jku)]

There may have been a regression on media/video-volume.html, I'll check that locally and ask for rollback if needed.

[Philippe Normand]

(In reply to comment #17)
> There may have been a regression on media/video-volume.html, I'll check that locally and ask for rollback if needed.

Ok thanks for keeping an eye out!

[Jussi Kukkonen (jku)]

For reference, I filed bug 96912 for this issue: it seems like the bug/functionality has been there all the time, this patch just made the test work better so the problem is now visible.

[Jussi Kukkonen (jku)]

(In reply to comment #19)
> For reference, I filed bug 96912 for this issue: it seems like the bug/functionality has been there all the time, this patch just made the test work better so the problem is now visible.

bug 103893 actually.