Bug 139496 – [GStreamer] Fix deadlock when shutting down AudioDestination

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Sebastian Dröge (slomo)]

Patch coming soon

[Sebastian Dröge (slomo)]

Created attachment 243048 [details]
Patch

[Sebastian Dröge (slomo)]

The GStreamer bug will be fixed in 1.4.5:
https://bugzilla.gnome.org/show_bug.cgi?id=740001

[Philippe Normand]

Comment on attachment 243048 [details]
Patch

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

Looks good, thanks! Just a few nits to brag about :)

> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:343
> +            // FLUSHING and EOS are no errors

are not errors.

> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:378
> +                // FLUSHING and EOS are no errors

Ditto :)

> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:385
> +        } else {
> +            gst_buffer_unref(channelBuffer);
>          }

for one-line blocks we don't use {} usually.

[Sebastian Dröge (slomo)]

(In reply to comment #3)

> > Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:385
> > +        } else {
> > +            gst_buffer_unref(channelBuffer);
> >          }
> 
> for one-line blocks we don't use {} usually.

Even for cases like

if (foo) {
    multiple
    lines
} else
  one_line

? I'll change that later when I'm at my laptop again. Thanks for the fast review :)

[Philippe Normand]

Yep but you can double-check with the check-webkit-style script.

[Sebastian Dröge (slomo)]

Created attachment 243206 [details]
Patch

[Philippe Normand]

Comment on attachment 243206 [details]
Patch

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

> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:429
> +        gst_buffer_pool_set_flushing(src->priv->pool, TRUE);

Seems like an #ifdef is needed here to make EFL EWS happy... Unless they bump their jhbuild gstreamer version to 1.4.x but that's not up to me to decide :)

[Sebastian Dröge (slomo)]

(In reply to comment #7)
> Comment on attachment 243206 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=243206&action=review
> 
> > Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:429
> > +        gst_buffer_pool_set_flushing(src->priv->pool, TRUE);
> 
> Seems like an #ifdef is needed here to make EFL EWS happy... Unless they
> bump their jhbuild gstreamer version to 1.4.x but that's not up to me to
> decide :)

An #ifdef should be fine. That's really just an optimization in the end :) If we're shutting down, gst_buffer_pool_acquire() would return FLUSHING afterwards and we would go out of the srcpad loop earlier... instead of going out of it when trying to push buffers to the appsrcs.

[Sebastian Dröge (slomo)]

Created attachment 243300 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 243300 [details]
Patch

Clearing flags on attachment: 243300

Committed r177341: <http://trac.webkit.org/changeset/177341>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.