[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 #10 from Martin Robinson <mrobinson at webkit.org>  2012-08-01 07:01:49 PST ---
(From update of attachment 153280)
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.

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