Changeset 249325 in webkit

Timestamp:

Aug 30, 2019 8:14:54 AM (5 years ago)

Author:

aboya@igalia.com

Message:

[MSE][GStreamer] Gracefully fail on invalid non-first initialization segment
​https://bugs.webkit.org/show_bug.cgi?id=201322

Reviewed by Xabier Rodriguez-Calvar.

Source/WebCore:

In normal operation of AppendPipeline, except during tear down,
qtdemux never removes a pad. Even if a new initialization segment is
appended, the pad is reused.

There is an exception though: when the new initialization segment has
an incompatible set of tracks. This is invalid under the MSE spec and
should produce an error, but in this case this was making an assertion
fail -- in particular by sending an EOS to the to-be-removed pad, which
AppendPipeline doesn't expect.

This patch changes the assertion with graceful error handling for that
error.

Fixes media/media-source/media-source-seek-detach-crash.html

• platform/graphics/gstreamer/mse/AppendPipeline.cpp:

(WebCore::AppendPipeline::AppendPipeline):
(WebCore::AppendPipeline::handleErrorConditionFromStreamingThread):
(WebCore::AppendPipeline::handleErrorSyncMessage):

• platform/graphics/gstreamer/mse/AppendPipeline.h:

LayoutTests:

• platform/gtk/TestExpectations:
• platform/wpe/TestExpectations:

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/platform/gtk/TestExpectations (modified) (1 diff)
• LayoutTests/platform/wpe/TestExpectations (modified) (1 diff)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp (modified) (3 diffs)
• Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.h (modified) (1 diff)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2019-08-30  Alicia Boya García  <aboya@igalia.com>
+
+        [MSE][GStreamer] Gracefully fail on invalid non-first initialization segment
+        https://bugs.webkit.org/show_bug.cgi?id=201322
+
+        Reviewed by Xabier Rodriguez-Calvar.
+
+        * platform/gtk/TestExpectations:
+        * platform/wpe/TestExpectations:
+
 2019-08-30  Joonghun Park  <jh718.park@samsung.com>
 

trunk/LayoutTests/platform/gtk/TestExpectations

@@ -242 +242 @@
 webkit.org/b/167108 imported/w3c/web-platform-tests/media-source/mediasource-trackdefault.html [ Failure ]
 webkit.org/b/167108 imported/w3c/web-platform-tests/media-source/mediasource-trackdefaultlist.html [ Failure ]
-
-# We don't support multiple streams per sourcebuffer nor dynamic type changes (audio/video/text)
-webkit.org/b/165394 media/media-source/media-source-seek-detach-crash.html [ Skip ]
 
 # There is an oggdemux bug that deadlocks WebKit: https://gitlab.freedesktop.org/gstreamer/gst-plugins-base/issues/639

trunk/LayoutTests/platform/wpe/TestExpectations

@@ -1312 +1312 @@
 webkit.org/b/171726 media/media-source/media-source-init-segment-duration.html [ Failure ]
 webkit.org/b/168373 media/media-source/media-source-resize.html [ Failure ]
-webkit.org/b/165394 media/media-source/media-source-seek-detach-crash.html [ Timeout ]
 webkit.org/b/168373 media/media-source/only-bcp47-language-tags-accepted-as-valid.html [ Timeout ]
 

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2019-08-30  Alicia Boya García  <aboya@igalia.com>
+
+        [MSE][GStreamer] Gracefully fail on invalid non-first initialization segment
+        https://bugs.webkit.org/show_bug.cgi?id=201322
+
+        Reviewed by Xabier Rodriguez-Calvar.
+
+        In normal operation of AppendPipeline, except during tear down,
+        qtdemux never removes a pad. Even if a new initialization segment is
+        appended, the pad is reused.
+
+        There is an exception though: when the new initialization segment has
+        an incompatible set of tracks. This is invalid under the MSE spec and
+        should produce an error, but in this case this was making an assertion
+        fail -- in particular by sending an EOS to the to-be-removed pad, which
+        AppendPipeline doesn't expect.
+
+        This patch changes the assertion with graceful error handling for that
+        error.
+
+        Fixes media/media-source/media-source-seek-detach-crash.html
+
+        * platform/graphics/gstreamer/mse/AppendPipeline.cpp:
+        (WebCore::AppendPipeline::AppendPipeline):
+        (WebCore::AppendPipeline::handleErrorConditionFromStreamingThread):
+        (WebCore::AppendPipeline::handleErrorSyncMessage):
+        * platform/graphics/gstreamer/mse/AppendPipeline.h:
+
 2019-08-30  Charlie Turner  <cturner@igalia.com>
 

trunk/Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp

@@ -217 +217 @@
     }), this);
     g_signal_connect(m_appsink.get(), "eos", G_CALLBACK(+[](GstElement*, AppendPipeline* appendPipeline) {
-        // basesrc will emit an EOS after it has received a GST_FLOW_ERROR. That's the only case we are expecting.
-        if (!appendPipeline->m_errorReceived) {
-            GST_ERROR("Unexpected appsink EOS in AppendPipeline");
-            ASSERT_NOT_REACHED();
-        }
+        if (appendPipeline->m_errorReceived)
+            return;
+
+        GST_ERROR("AppendPipeline's appsink received EOS. This is usually caused by an invalid initialization segment.");
+        appendPipeline->handleErrorConditionFromStreamingThread();
     }), this);
 
@@ -280 +280 @@
 }
 
-void AppendPipeline::handleErrorSyncMessage(GstMessage* message)
+void AppendPipeline::handleErrorConditionFromStreamingThread()
 {
     ASSERT(!isMainThread());
-    GST_WARNING_OBJECT(m_pipeline.get(), "Demuxing error: %" GST_PTR_FORMAT, message);
     // Notify the main thread that the append has a decode error.
     auto response = m_taskQueue.enqueueTaskAndWait<AbortableTaskQueue::Void>([this]() {
@@ -293 +292 @@
     // The streaming thread has now been unblocked because we are aborting in the main thread.
     ASSERT(!response);
+}
+
+void AppendPipeline::handleErrorSyncMessage(GstMessage* message)
+{
+    ASSERT(!isMainThread());
+    GST_WARNING_OBJECT(m_pipeline.get(), "Demuxing error: %" GST_PTR_FORMAT, message);
+    handleErrorConditionFromStreamingThread();
 }
 

trunk/Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.h

@@ -68 +68 @@
 
     void handleAppsinkNewSampleFromStreamingThread(GstElement*);
+    void handleErrorConditionFromStreamingThread();
 
     // Takes ownership of caps.