Changeset 228271 in webkit

Timestamp:

Feb 8, 2018 5:10:48 AM (6 years ago)

Author:

Philippe Normand

Message:

[GStreamer] LayoutTest webaudio/silent-audio-interrupted-in-background.html makes its subsequent test flaky crash
​https://bugs.webkit.org/show_bug.cgi?id=173916

Reviewed by Xabier Rodriguez Calvar.

Source/WebCore:

This patch fixes two crashes and a runtime warning:

• The provider client configuration should be done from the main

thread but the no-more-pads signal of deinterleave was fired from
a non-main thread.

• The deinterleave pad-removed signal can be fired for a not fully

configured pipeline if the audio context is interrupted. So the
peer quark of the removed pad needs to be checked, it might be a
null pointer.

• The provider connects to the deinterleave signals only when a

client is provided, so the signal disconnection needs to check
that to avoid runtime warnings.

• platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:

(WebCore::AudioSourceProviderGStreamer::AudioSourceProviderGStreamer):
Create a main thread notifier.
(WebCore::AudioSourceProviderGStreamer::~AudioSourceProviderGStreamer):
Invalidate notifier and check a client was set before
disconnecting from deinterleave signals.
(WebCore::AudioSourceProviderGStreamer::handleRemovedDeinterleavePad):
Check validity of the pad peer.
(WebCore::AudioSourceProviderGStreamer::deinterleavePadsConfigured):
Set client from main thread.

• platform/audio/gstreamer/AudioSourceProviderGStreamer.h:

LayoutTests:

• platform/gtk/TestExpectations: Unskip fixed test.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/platform/gtk/TestExpectations (modified) (1 diff)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp (modified) (4 diffs)
• Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.h (modified) (2 diffs)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2018-02-08  Philippe Normand  <pnormand@igalia.com>
+
+        [GStreamer] LayoutTest webaudio/silent-audio-interrupted-in-background.html makes its subsequent test flaky crash
+        https://bugs.webkit.org/show_bug.cgi?id=173916
+
+        Reviewed by Xabier Rodriguez Calvar.
+
+        * platform/gtk/TestExpectations: Unskip fixed test.
+
 2018-02-06  Yusuke Suzuki  <utatane.tea@gmail.com>
 

trunk/LayoutTests/platform/gtk/TestExpectations

@@ -1294 +1294 @@
 webkit.org/b/172955 media/video-preload.html [ Crash Pass ]
 
-webkit.org/b/173916 webaudio/silent-audio-interrupted-in-background.html [ Skip ]
-
 webkit.org/b/175575 imported/w3c/web-platform-tests/html/semantics/embedded-content/media-elements/ready-states/autoplay-with-slow-text-tracks.html [ Crash Pass ]
 

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2018-02-08  Philippe Normand  <pnormand@igalia.com>
+
+        [GStreamer] LayoutTest webaudio/silent-audio-interrupted-in-background.html makes its subsequent test flaky crash
+        https://bugs.webkit.org/show_bug.cgi?id=173916
+
+        Reviewed by Xabier Rodriguez Calvar.
+
+        This patch fixes two crashes and a runtime warning:
+
+        - The provider client configuration should be done from the main
+        thread but the no-more-pads signal of deinterleave was fired from
+        a non-main thread.
+
+        - The deinterleave pad-removed signal can be fired for a not fully
+        configured pipeline if the audio context is interrupted. So the
+        peer quark of the removed pad needs to be checked, it might be a
+        null pointer.
+
+        - The provider connects to the deinterleave signals only when a
+        client is provided, so the signal disconnection needs to check
+        that to avoid runtime warnings.
+
+        * platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:
+        (WebCore::AudioSourceProviderGStreamer::AudioSourceProviderGStreamer):
+        Create a main thread notifier.
+        (WebCore::AudioSourceProviderGStreamer::~AudioSourceProviderGStreamer):
+        Invalidate notifier and check a client was set before
+        disconnecting from deinterleave signals.
+        (WebCore::AudioSourceProviderGStreamer::handleRemovedDeinterleavePad):
+        Check validity of the pad peer.
+        (WebCore::AudioSourceProviderGStreamer::deinterleavePadsConfigured):
+        Set client from main thread.
+        * platform/audio/gstreamer/AudioSourceProviderGStreamer.h:
+
 2018-02-08  Philippe Normand  <pnormand@igalia.com>
 

trunk/Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp

@@ -84 +84 @@
 
 AudioSourceProviderGStreamer::AudioSourceProviderGStreamer()
-    : m_client(nullptr)
+    : m_notifier(MainThreadNotifier<MainThreadNotification>::create())
+    , m_client(nullptr)
     , m_deinterleaveSourcePads(0)
     , m_deinterleavePadAddedHandlerId(0)
@@ -97 +98 @@
 AudioSourceProviderGStreamer::~AudioSourceProviderGStreamer()
 {
+    m_notifier->invalidate();
+
     GRefPtr<GstElement> deinterleave = adoptGRef(gst_bin_get_by_name(GST_BIN(m_audioSinkBin.get()), "deinterleave"));
-    if (deinterleave) {
+    if (deinterleave && m_client) {
         g_signal_handler_disconnect(deinterleave.get(), m_deinterleavePadAddedHandlerId);
         g_signal_handler_disconnect(deinterleave.get(), m_deinterleaveNoMorePadsHandlerId);
@@ -319 +322 @@
     // Remove the queue ! appsink chain downstream of deinterleave.
     GQuark quark = g_quark_from_static_string("peer");
-    GstPad* sinkPad = reinterpret_cast<GstPad*>(g_object_get_qdata(G_OBJECT(pad), quark));
+    GstPad* sinkPad = GST_PAD_CAST(g_object_get_qdata(G_OBJECT(pad), quark));
+    if (!sinkPad)
+        return;
+
     GRefPtr<GstElement> queue = adoptGRef(gst_pad_get_parent_element(sinkPad));
     GRefPtr<GstPad> queueSrcPad = adoptGRef(gst_element_get_static_pad(queue.get(), "src"));
@@ -332 +338 @@
 void AudioSourceProviderGStreamer::deinterleavePadsConfigured()
 {
-    ASSERT(m_client);
-    ASSERT(m_deinterleaveSourcePads == gNumberOfChannels);
-
-    m_client->setFormat(m_deinterleaveSourcePads, gSampleBitRate);
+    m_notifier->notify(MainThreadNotification::DeinterleavePadsConfigured, [this] {
+        ASSERT(m_client);
+        ASSERT(m_deinterleaveSourcePads == gNumberOfChannels);
+
+        m_client->setFormat(m_deinterleaveSourcePads, gSampleBitRate);
+    });
 }
 

trunk/Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.h

@@ -24 +24 @@
 #include "AudioSourceProvider.h"
 #include "GRefPtrGStreamer.h"
+#include "MainThreadNotifier.h"
 #include <gst/gst.h>
 #include <wtf/Forward.h>
@@ -54 +55 @@
 
 private:
+    enum MainThreadNotification {
+        DeinterleavePadsConfigured = 1 << 0,
+    };
+    Ref<MainThreadNotifier<MainThreadNotification>> m_notifier;
     GRefPtr<GstElement> m_audioSinkBin;
     AudioSourceProviderClient* m_client;