Bug 89122 - [GStreamer] Audio device not closed after playing sound
Summary: [GStreamer] Audio device not closed after playing sound
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-06-14 13:31 PDT by Daniel Drake
Modified: 2012-09-13 12:32 PDT (History)
8 users (show)

See Also:


Attachments
Patch (9.68 KB, patch)
2012-07-19 09:49 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (9.72 KB, patch)
2012-07-31 14:08 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (10.54 KB, patch)
2012-08-20 04:12 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (10.37 KB, patch)
2012-08-21 04:04 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (10.76 KB, patch)
2012-08-23 03:03 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (10.39 KB, patch)
2012-08-29 03:21 PDT, Philippe Normand
mrobinson: review+
Details | Formatted Diff | Diff
Backport for 1.9 branch (9.71 KB, patch)
2012-09-13 12:01 PDT, Daniel Drake
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Drake 2012-06-14 13:31:45 PDT
After a sound is played within webkit (loaded e.g. with an <a type="application/log"> hyperlink), the audio device is not closed.

Test case:
1. "cat /proc/asound/card0/pcm*/sub0/status" reads closed
2. Open browser, play ogg vorbis audio
3. "cat /proc/asound/card0/pcm*/sub0/status" reads state: RUNNING
4. state is still RUNNING even minutes after the audio file has stopped playing

Navigating to another page will close the device.

WebKit-1.8.1 reproduced in Epiphany on Fedora 17.
Comment 1 Philippe Normand 2012-06-14 14:02:29 PDT
What are your versions of gst-plugins-base and -good?
Comment 2 Daniel Drake 2012-06-14 14:04:39 PDT
gst-plugins-base-0.10.36
gst-plugins-good-0.10.31
Comment 3 Philippe Normand 2012-06-14 14:25:07 PDT
Which audio sink is used? alsasink or pulsesink?
Can you attach a debug log with GST_DEBUG=alsa*:5,pulse:5 ?
Comment 4 Philippe Normand 2012-06-14 14:30:35 PDT
Actually I think this is because on EOS we pause the gst pipeline. We don't set it to READY or NULL, this is done in the destructor of the player. That's why navigating to another page actually closes the device.

Do you really need the device to be closed on EOS?
Comment 5 Daniel Drake 2012-06-14 14:44:02 PDT
Using alsasink, no pulse on our platform (yet).

The device should be closed on EOS to save power. Not only power to the audio components - our platform (OLPC XO) performs an "idle suspend" on system inactivity (where the CPU gets powered down but the display is left running) saving a lot of power, but having an open audio device inhibits such suspends for obvious reasons.
Comment 6 Philippe Normand 2012-06-17 15:33:57 PDT
I started working on a patch.
Comment 7 Philippe Normand 2012-07-19 09:49:09 PDT
Created attachment 153280 [details]
Patch

Daniel can you please test this patch?
Comment 8 Philippe Normand 2012-07-31 10:22:58 PDT
Ping, Daniel?
Comment 9 Philippe Normand 2012-07-31 14:08:28 PDT
Created attachment 155632 [details]
Patch

Rebased against recent ToT
Comment 10 Martin Robinson 2012-08-01 07:01:49 PDT
Comment on attachment 153280 [details]
Patch

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

I think I'll probably need to talk with you directly about the last change in didEnd, but I have some comments about the rest.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:327
> +    if (m_isEndReached) {
> +        if (m_seeking)
> +            return m_seekTime;
> +        if (m_mediaDuration)
> +            return m_mediaDuration;
> +    }

What happens if m_mediaDuration is 0? What does that mean? Is that an error?

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:363
> +    LOG_VERBOSE(Media, "Current state: %s, pending: %s", gst_element_state_get_name(currentState), gst_element_state_get_name(pending));
> +    if (currentState >= newState || pending >= newState)
> +        return true;

Is it really safe to compare the different GstStates (which, I presume, are enum values) with inequalities?

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:367
> +    GstStateChangeReturn ret = gst_element_set_state(m_playBin, newState);

ret -> setStateResult?

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:-430
> -    if (m_seeking)
> -        return m_seekTime;
> -

Why this change?

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:464
> +    if (m_isEndReached && !time && !mediaElementLooping())
> +        return;
> +

A comment might be useful here explaining why you don't seek in this situation.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:467
> +    if (!m_isEndReached && (time == playbackPosition()))
>          return;

Why the extra check here? Even if the end is reached, why should we allow seeking to a position that is the same as the current playback position?

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1155
> -        if (m_buffering && state != GST_STATE_READY) {
> +        if (m_buffering && state > GST_STATE_READY) {

I'm still a little uneasy about inequality usage here.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1193
> +        } else if (!m_isEndReached)
>              m_paused = true;

It's a bit odd that pause() returns true when m_isReached is true, but m_paused is not true... I'm not sure I understand.
Comment 11 Philippe Normand 2012-08-01 07:40:29 PDT
Comment on attachment 153280 [details]
Patch

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

>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:327
>> +    }
> 
> What happens if m_mediaDuration is 0? What does that mean? Is that an error?

It means there's no duration in TIME available, it's not a case or error though.

>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:363
>> +        return true;
> 
> Is it really safe to compare the different GstStates (which, I presume, are enum values) with inequalities?

Yes I think so. The GstState enum hasn't changed since the early gst 0.10 versions AFAIK. In gst 0.11/1.0 it hasn't changed either. This is one of the basic concepts of the framework.

>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:367
>> +    GstStateChangeReturn ret = gst_element_set_state(m_playBin, newState);
> 
> ret -> setStateResult?

Ok I'll change that!

>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:-430
>> -
> 
> Why this change?

Hum good question, I need to check this one.

>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:467
>>          return;
> 
> Why the extra check here? Even if the end is reached, why should we allow seeking to a position that is the same as the current playback position?

Ermm, good point! I guess this change can be removed. I'll check it again.

>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1193
>>              m_paused = true;
> 
> It's a bit odd that pause() returns true when m_isReached is true, but m_paused is not true... I'm not sure I understand.

Right, maybe we can set m_paused on EOS if the media element is not looping.
Comment 12 Daniel Drake 2012-08-08 13:28:30 PDT
Sorry for not responding earlier. I'm less overloaded now so will be able to test this. But I guess it would be best to wait for a new version addressing the review comments first.
Comment 13 Philippe Normand 2012-08-14 08:55:43 PDT
Comment on attachment 155632 [details]
Patch

Pulling out of review, I started reworking this a bit but still blocked by a regression.
Comment 14 Philippe Normand 2012-08-20 04:12:17 PDT
Created attachment 159390 [details]
Patch
Comment 15 Daniel Drake 2012-08-20 19:02:07 PDT
Thanks, this is looking better - the sound device gets closed at the end of playback. But there is a small issue:

1. Open the browser
2. Select an audio file
3. Let it play to the end - don't touch the playback controls
4. At the end of playback, the playback "progress bar" goes white-ish with the progress blob at the rightmost end, and the play/pause button shows the "Play" icon
5. Click play - the progress blob will jump back to the left side, and the normal progress bar colour will be displayed, but the play/pause button will still be in the "Play" icon state and no sound plays
6. Click play again, now the sound plays.
Comment 16 Philippe Normand 2012-08-20 23:30:39 PDT
Thanks Daniel for the feedback on this. I'll have a look at the issue you found.
Comment 17 Philippe Normand 2012-08-21 04:04:49 PDT
Created attachment 159648 [details]
Patch

Update! Addressing the issue mentioned by Daniel.
Comment 18 Daniel Drake 2012-08-21 10:38:37 PDT
Thanks, tested it and it's looking good, can't see any further issues.
Comment 19 Martin Robinson 2012-08-21 11:07:34 PDT
Comment on attachment 159648 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:337
> +    LOG_MEDIA_MESSAGE("EOS: %d, seeking: %d, seekTime: %f, duration: %f", m_isEndReached, m_seeking, m_seekTime, m_mediaDuration);

This doesn't seem like the sort of message we should keep, since it lacks any sort of informative aspect beyond information dumping. Perhaps remove it to avoid noise in the log?

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:343
> +    if (m_isEndReached) {
> +        if (m_seeking)
> +            return m_seekTime;
> +        if (m_mediaDuration)
> +            return m_mediaDuration;
> +    }

I think this change deserves a comment. Something like, "Position queries don't work on a null pipeline and when we're at the end of the stream the pipipeline is null."

This code will fall back to position queries even if the pipeline is in a null state if !m_seeking and !m_mediaDuration. Is that okay? Perhaps you should just return a "safe" value if position queries aren't allowed on null piplines.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:377
> +    LOG_MEDIA_MESSAGE("Current state: %s, pending: %s", gst_element_state_get_name(currentState), gst_element_state_get_name(pending));

Ditto with this comment.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:489
> +    if (m_isEndReached && !m_player->mediaPlayerClient()->mediaPlayerLoop()) {
> +        LOG_MEDIA_MESSAGE("Seek on EOS, pre-rolling pipeline again.");
> +        gst_element_set_state(m_playBin, GST_STATE_PAUSED);
> +    }
> +
> +    // Avoid useless seeking.
> +    if (time == playbackPosition())
>          return;

If time == playbackPosition() won't this re-open the audio device and then do nothing?

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:522
> +        LOG_MEDIA_MESSAGE("Paused on EOS");

Maybe explain what you're doing with it? For instance: "Ignoring pause at EOS."

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1436
> +    if ((now > 0 && m_playbackRate < 0) || (now <= duration() && m_playbackRate > 0)) {

What does it mean when didEnd happens when (now <= duration() && m_playbackRate > 0)?

Perhaps this code should be self documentating:
bool playingInReverse = now > 0 && m_playbackRate < 0;
bool ??? = now <= duration() && m_playbackRate > 0;
if (playingInReverse || ???) {
    ...
}

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1443
> +        float previousDuration = m_mediaDuration;
> +        if (now != m_mediaDuration) {
> +            m_mediaDurationKnown = true;
> +            m_mediaDuration = now;
> +        }
> +        if (m_mediaDuration != previousDuration)
> +            m_player->durationChanged();

I've tried to run through this several times in my head and each time that I do it seems that both if statements are the same here. Couldn't this just be written as:

if (m_mediaDuration != now) {
    m_mediaDurationKnown = true;
    m_mediaDuration = now;
    m_player->durationChanged();
}

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1451
> +    if (!m_player->mediaPlayerClient()->mediaPlayerLoop())
> +        gst_element_set_state(m_playBin, GST_STATE_NULL);

If we are looping, is it still correct to set m_paused to true?
Comment 20 Philippe Normand 2012-08-23 01:12:49 PDT
Comment on attachment 159648 [details]
Patch

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

Thanks for the detailed review, I'll update the patch.

>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:337
>> +    LOG_MEDIA_MESSAGE("EOS: %d, seeking: %d, seekTime: %f, duration: %f", m_isEndReached, m_seeking, m_seekTime, m_mediaDuration);
> 
> This doesn't seem like the sort of message we should keep, since it lacks any sort of informative aspect beyond information dumping. Perhaps remove it to avoid noise in the log?

Ah yes I forgot to remove it, sorry.

>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:343
>> +    }
> 
> I think this change deserves a comment. Something like, "Position queries don't work on a null pipeline and when we're at the end of the stream the pipipeline is null."
> 
> This code will fall back to position queries even if the pipeline is in a null state if !m_seeking and !m_mediaDuration. Is that okay? Perhaps you should just return a "safe" value if position queries aren't allowed on null piplines.

Ok for the comment.
About the fall back value, supposedly the duration is cached on EOS so it should be returned, I'll double-check this... About position queries on a NULL-state pipeline, the sinks handle this gracefully already (at least the ones based on gstbasesink).

>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:377
>> +    LOG_MEDIA_MESSAGE("Current state: %s, pending: %s", gst_element_state_get_name(currentState), gst_element_state_get_name(pending));
> 
> Ditto with this comment.

I think this one is still useful, actually. It allows to see the pipeline state before doing the actual change.

>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:489
>>          return;
> 
> If time == playbackPosition() won't this re-open the audio device and then do nothing?

Right, I'll update this part of the patch!

>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:522
>> +        LOG_MEDIA_MESSAGE("Paused on EOS");
> 
> Maybe explain what you're doing with it? For instance: "Ignoring pause at EOS."

Yes, good idea!

>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1436
>> +    if ((now > 0 && m_playbackRate < 0) || (now <= duration() && m_playbackRate > 0)) {
> 
> What does it mean when didEnd happens when (now <= duration() && m_playbackRate > 0)?
> 
> Perhaps this code should be self documentating:
> bool playingInReverse = now > 0 && m_playbackRate < 0;
> bool ??? = now <= duration() && m_playbackRate > 0;
> if (playingInReverse || ???) {
>     ...
> }

Actually maybe this test can be simplified to now > 0 && now <= duration(), eg. position is valid.

>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1443
>> +            m_player->durationChanged();
> 
> I've tried to run through this several times in my head and each time that I do it seems that both if statements are the same here. Couldn't this just be written as:
> 
> if (m_mediaDuration != now) {
>     m_mediaDurationKnown = true;
>     m_mediaDuration = now;
>     m_player->durationChanged();
> }

Yes! This simplification is indeed valid! :)

>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1451
>> +        gst_element_set_state(m_playBin, GST_STATE_NULL);
> 
> If we are looping, is it still correct to set m_paused to true?

Hum. Likely not :)
Comment 21 Philippe Normand 2012-08-23 03:03:51 PDT
Created attachment 160122 [details]
Patch
Comment 22 Martin Robinson 2012-08-23 08:35:20 PDT
Comment on attachment 160122 [details]
Patch

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

This patch looks good to me, though I think that someone like Eric should look at the platform-independent changes.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:343
> +        // Position queries don't work on a null pipeline and when we're at the end of the stream the pipeline is null.
> +        if (m_seeking)
> +            return m_seekTime;
> +        if (m_mediaDuration)
> +            return m_mediaDuration;
> +    }

Hrm. I guess if the sync can handle position queries gracefully, then the comment is inaccurate?

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:497
> +    bool prerollRequired = m_isEndReached && !m_player->mediaPlayerClient()->mediaPlayerLoop();
> +    if (prerollRequired) {
> +        LOG_MEDIA_MESSAGE("Seek on EOS, pre-rolling pipeline again.");
> +        gst_element_set_state(m_playBin, GST_STATE_PAUSED);
> +    }
> +
> +    // Avoid useless seeking.
> +    if (time == playbackPosition()) {
> +        if (prerollRequired)
> +            gst_element_set_state(m_playBin, GST_STATE_NULL);
> +        return;
> +    }
> +

Is it important to go from PAUSED back to NULL for no-op seeks here? If not, why can't you just do:

// Avoid useless seeking.
if (time == playbackPosition())
    return;


if (m_isEndReached && !m_player->mediaPlayerClient()->mediaPlayerLoop()) {
    LOG_MEDIA_MESSAGE("Seek on EOS, pre-rolling pipeline again.");
    gst_element_set_state(m_playBin, GST_STATE_PAUSED);
}
Comment 23 Philippe Normand 2012-08-29 02:35:56 PDT
(In reply to comment #22)
> (From update of attachment 160122 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=160122&action=review
> 
> This patch looks good to me, though I think that someone like Eric should look at the platform-independent changes.
> 
> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:343
> > +        // Position queries don't work on a null pipeline and when we're at the end of the stream the pipeline is null.
> > +        if (m_seeking)
> > +            return m_seekTime;
> > +        if (m_mediaDuration)
> > +            return m_mediaDuration;
> > +    }
> 
> Hrm. I guess if the sync can handle position queries gracefully, then the comment is inaccurate?
> 

Yes... I'll elaborate the comment a bit more.

> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:497
> > +    bool prerollRequired = m_isEndReached && !m_player->mediaPlayerClient()->mediaPlayerLoop();
> > +    if (prerollRequired) {
> > +        LOG_MEDIA_MESSAGE("Seek on EOS, pre-rolling pipeline again.");
> > +        gst_element_set_state(m_playBin, GST_STATE_PAUSED);
> > +    }
> > +
> > +    // Avoid useless seeking.
> > +    if (time == playbackPosition()) {
> > +        if (prerollRequired)
> > +            gst_element_set_state(m_playBin, GST_STATE_NULL);
> > +        return;
> > +    }
> > +
> 
> Is it important to go from PAUSED back to NULL for no-op seeks here? If not, why can't you just do:
> 
> // Avoid useless seeking.
> if (time == playbackPosition())
>     return;
> 
> 
> if (m_isEndReached && !m_player->mediaPlayerClient()->mediaPlayerLoop()) {
>     LOG_MEDIA_MESSAGE("Seek on EOS, pre-rolling pipeline again.");
>     gst_element_set_state(m_playBin, GST_STATE_PAUSED);
> }


Well, now that I think about again, if pre-roll is required it means EOS was reached so we should use currentTime() directly instead of playbackPosition(). currentTime() gracefully handles the EOS case.
Comment 24 Philippe Normand 2012-08-29 03:21:57 PDT
Created attachment 161170 [details]
Patch
Comment 25 Philippe Normand 2012-09-04 08:28:49 PDT
Hi Eric,

Can you please have a look at the Media element changes of this patch?

I needed a method in MediaPlayerClient to know if the player is looping over a media.
Comment 26 Daniel Drake 2012-09-11 07:43:51 PDT
Trying to test the latest patch, it doesn't apply to webkitgtk-1.9.91 and the differences don't seem trivial. Which version is the patch based on?
Comment 27 Martin Robinson 2012-09-11 18:21:49 PDT
Comment on attachment 161170 [details]
Patch

No response from Eric, so I'm guessing he has no strong objections. I'll r+ this since it's quite an important fix for our stable branch. Please fix the issues we discussed before landing. Thanks!
Comment 28 Philippe Normand 2012-09-11 23:22:52 PDT
(In reply to comment #26)
> Trying to test the latest patch, it doesn't apply to webkitgtk-1.9.91 and the differences don't seem trivial. Which version is the patch based on?

It's based on ToT (as usual) as of about 2 weeks ago.

Thanks Martin for the r+, I'll land this patch later today!
Comment 29 Philippe Normand 2012-09-12 05:33:31 PDT
Committed r128298: <http://trac.webkit.org/changeset/128298>
Comment 30 Daniel Drake 2012-09-13 12:01:36 PDT
Created attachment 163927 [details]
Backport for 1.9 branch

Thanks so much Philippe - retested the final version, seems to be working well.

Here's a backport for the 1.9 branch which I'll propose for inclusion.
Comment 31 Philippe Normand 2012-09-13 12:22:31 PDT
Thanks Daniel, this was already cherry-picked for next week's release though.
Comment 32 Daniel Drake 2012-09-13 12:32:26 PDT
Great news - you're one step ahead. Thanks!!