Bug 89122 – [GStreamer] Audio device not closed after playing sound

• Component: Platform
• Status: RESOLVED FIXED

[Daniel Drake]

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.

[Philippe Normand]

What are your versions of gst-plugins-base and -good?

[Daniel Drake]

gst-plugins-base-0.10.36
gst-plugins-good-0.10.31

[Philippe Normand]

Which audio sink is used? alsasink or pulsesink?
Can you attach a debug log with GST_DEBUG=alsa*:5,pulse:5 ?

[Philippe Normand]

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?

[Daniel Drake]

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.

[Philippe Normand]

I started working on a patch.

[Philippe Normand]

Created attachment 153280 [details]
Patch

Daniel can you please test this patch?

[Philippe Normand]

Ping, Daniel?

[Philippe Normand]

Created attachment 155632 [details]
Patch

Rebased against recent ToT

[Martin Robinson]

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.

[Philippe Normand]

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.

[Daniel Drake]

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.

[Philippe Normand]

Comment on attachment 155632 [details]
Patch

Pulling out of review, I started reworking this a bit but still blocked by a regression.

[Philippe Normand]

Created attachment 159390 [details]
Patch

[Daniel Drake]

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.

[Philippe Normand]

Thanks Daniel for the feedback on this. I'll have a look at the issue you found.

[Philippe Normand]

Created attachment 159648 [details]
Patch

Update! Addressing the issue mentioned by Daniel.

[Daniel Drake]

Thanks, tested it and it's looking good, can't see any further issues.

[Martin Robinson]

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?

[Philippe Normand]

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 :)

[Philippe Normand]

Created attachment 160122 [details]
Patch

[Martin Robinson]

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);
}

[Philippe Normand]

(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.

[Philippe Normand]

Created attachment 161170 [details]
Patch

[Philippe Normand]

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.

[Daniel Drake]

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?

[Martin Robinson]

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!

[Philippe Normand]

(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!

[Philippe Normand]

Committed r128298: <http://trac.webkit.org/changeset/128298>

[Daniel Drake]

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.

[Philippe Normand]

Thanks Daniel, this was already cherry-picked for next week's release though.

[Daniel Drake]

Great news - you're one step ahead. Thanks!!