Summary: | [GStreamer] Fix deadlock when shutting down AudioDestination | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Sebastian Dröge (slomo) <slomo> | ||||||||
Component: | Media | Assignee: | Sebastian Dröge (slomo) <slomo> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Major | CC: | commit-queue, pnormand | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Sebastian Dröge (slomo)
2014-12-10 10:52:36 PST
Created attachment 243048 [details]
Patch
The GStreamer bug will be fixed in 1.4.5: https://bugzilla.gnome.org/show_bug.cgi?id=740001 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. (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 :) Yep but you can double-check with the check-webkit-style script. Created attachment 243206 [details]
Patch
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 :) (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. Created attachment 243300 [details]
Patch
Comment on attachment 243300 [details] Patch Clearing flags on attachment: 243300 Committed r177341: <http://trac.webkit.org/changeset/177341> All reviewed patches have been landed. Closing bug. |