[Webkit-unassigned] [Bug 89122] [GStreamer] Audio device not closed after playing sound

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=89122





--- Comment #19 from Martin Robinson <mrobinson at webkit.org>  2012-08-21 11:07:34 PST ---
(From update of attachment 159648)
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?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.