WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
96912
[GStreamer] initial volume events on media elements could happen earlier
https://bugs.webkit.org/show_bug.cgi?id=96912
Summary
[GStreamer] initial volume events on media elements could happen earlier
Jussi Kukkonen (jku)
Reported
2012-09-17 06:13:45 PDT
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.
Attachments
Patch
(2.03 KB, patch)
2012-09-18 03:27 PDT
,
Jussi Kukkonen (jku)
no flags
Details
Formatted Diff
Diff
simple test page
(665 bytes, text/html)
2012-09-18 03:30 PDT
,
Jussi Kukkonen (jku)
no flags
Details
Patch
(3.38 KB, patch)
2012-11-30 04:18 PST
,
Jussi Kukkonen (jku)
no flags
Details
Formatted Diff
Diff
Improve Changelog
(3.38 KB, patch)
2012-11-30 04:53 PST
,
Jussi Kukkonen (jku)
no flags
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Jussi Kukkonen (jku)
Comment 1
2012-09-18 02:06:26 PDT
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
Comment 2
2012-09-18 02:30:58 PDT
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)
Comment 3
2012-09-18 03:24:52 PDT
(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)
Comment 4
2012-09-18 03:27:57 PDT
Created
attachment 164525
[details]
Patch
Jussi Kukkonen (jku)
Comment 5
2012-09-18 03:30:44 PDT
Created
attachment 164527
[details]
simple test page
Philippe Normand
Comment 6
2012-09-18 03:43:26 PDT
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)
Comment 7
2012-09-18 03:59:17 PDT
(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)
Comment 8
2012-09-18 05:05:11 PDT
(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
Comment 9
2012-09-18 05:27:45 PDT
(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
Comment 10
2012-11-30 02:10:51 PST
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)
Comment 11
2012-11-30 04:18:56 PST
Created
attachment 176937
[details]
Patch
Jussi Kukkonen (jku)
Comment 12
2012-11-30 04:22:34 PST
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)
Comment 13
2012-11-30 04:27:58 PST
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)
Comment 14
2012-11-30 04:53:15 PST
Created
attachment 176943
[details]
Improve Changelog
WebKit Review Bot
Comment 15
2012-12-03 04:38:41 PST
Comment on
attachment 176943
[details]
Improve Changelog Clearing flags on attachment: 176943 Committed
r136382
: <
http://trac.webkit.org/changeset/136382
>
WebKit Review Bot
Comment 16
2012-12-03 04:38:46 PST
All reviewed patches have been landed. Closing bug.
Jussi Kukkonen (jku)
Comment 17
2012-12-03 05:49:31 PST
There may have been a regression on media/video-volume.html, I'll check that locally and ask for rollback if needed.
Philippe Normand
Comment 18
2012-12-03 05:56:10 PST
(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)
Comment 19
2012-12-03 07:55:24 PST
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)
Comment 20
2012-12-03 07:56:01 PST
(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.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug