[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 #20 from Philippe Normand <pnormand at igalia.com>  2012-08-23 01:12:49 PST ---
(From update of attachment 159648)
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 :)

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