[MSE][GStreamer] Remove the AppendPipeline state machine
authoraboya@igalia.com <aboya@igalia.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 5 Dec 2018 14:56:33 +0000 (14:56 +0000)
committeraboya@igalia.com <aboya@igalia.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 5 Dec 2018 14:56:33 +0000 (14:56 +0000)
https://bugs.webkit.org/show_bug.cgi?id=192204

Reviewed by Xabier Rodriguez-Calvar.

LayoutTests/imported/w3c:

Added a test checking that initialization segments with invalid codec
identifiers are flagged as errors.

* web-platform-tests/media-source/mediasource-invalid-codec-expected.txt: Added.
* web-platform-tests/media-source/mediasource-invalid-codec.html: Added.
* web-platform-tests/media-source/mp4/invalid-codec.mp4: Added.
* web-platform-tests/media-source/webm/invalid-codec.webm: Added.

Source/WebCore:

This patch tries to reduce the complexity of the AppendPipeline by
removing the appendState state machine and cleaning all the
conditional code around it that is not necessary anymore.

For the most part the behavior is the same, but some edge cases have
been improved in the process:

Demuxing errors now result in the append being flagged as
ParsingFailed and the error being propagated to the application. This
fixes media/media-source/media-source-error-crash.html (or at least
gets it up to date with cross platform expectations).

AbortableTaskQueue now allows the task handler to perform an abort
safely. This is used in the GstBus error message sync handler, since
it needs to ask the MainThread to raise a parse error, which will in
turn abort. An API test has been added for this new functionality.
Also, code has been added to the API tests to ensure the correct
destruction of the response object, especially in this case.

The code handling invalid track codecs has been made clearer by also
explicitly raising a parse error, but it should not expose behavior
differences for the application. A test has been added for this
behavior: web-platform-tests/media-source/mediasource-invalid-codec.html

The reporting of EOS events have been made more rigorous. EOS is only
expected after a demuxing error, otherwise it's a g_critical.

AppendPipeline::abort() has been renamed to
AppendPipeline::resetParserState() to honor the fact that it's not
only called when the user calls abort() and match better the names
used in the spec.

Test: imported/w3c/web-platform-tests/media-source/mediasource-invalid-codec.html

* platform/AbortableTaskQueue.h:
* platform/graphics/gstreamer/mse/AppendPipeline.cpp:
(WebCore::assertedElementSetState):
(WebCore::AppendPipeline::AppendPipeline):
(WebCore::AppendPipeline::~AppendPipeline):
(WebCore::AppendPipeline::handleErrorSyncMessage):
(WebCore::AppendPipeline::appsrcEndOfAppendCheckerProbe):
(WebCore::AppendPipeline::handleNeedContextSyncMessage):
(WebCore::AppendPipeline::appsinkCapsChanged):
(WebCore::AppendPipeline::handleEndOfAppend):
(WebCore::AppendPipeline::appsinkNewSample):
(WebCore::AppendPipeline::didReceiveInitializationSegment):
(WebCore::AppendPipeline::resetParserState):
(WebCore::AppendPipeline::pushNewBuffer):
(WebCore::AppendPipeline::handleAppsinkNewSampleFromStreamingThread):
(WebCore::AppendPipeline::connectDemuxerSrcPadToAppsinkFromStreamingThread):
(WebCore::AppendPipeline::connectDemuxerSrcPadToAppsink):
(WebCore::AppendPipeline::disconnectDemuxerSrcPadFromAppsinkFromAnyThread):
(WebCore::AppendPipeline::dumpAppendState): Deleted.
(WebCore::AppendPipeline::demuxerNoMorePads): Deleted.
(WebCore::AppendPipeline::setAppendState): Deleted.
(WebCore::AppendPipeline::appsinkEOS): Deleted.
(WebCore::AppendPipeline::resetPipeline): Deleted.
(WebCore::AppendPipeline::abort): Deleted.
* platform/graphics/gstreamer/mse/AppendPipeline.h:
(WebCore::AppendPipeline::appendState): Deleted.
* platform/graphics/gstreamer/mse/MediaSourceClientGStreamerMSE.cpp:
(WebCore::MediaSourceClientGStreamerMSE::abort):
(WebCore::MediaSourceClientGStreamerMSE::resetParserState):
* platform/graphics/gstreamer/mse/SourceBufferPrivateGStreamer.cpp:
(WebCore::SourceBufferPrivateGStreamer::appendParsingFailed):
* platform/graphics/gstreamer/mse/SourceBufferPrivateGStreamer.h:

Tools:

Updated AbortableTaskQueue tests:

Added test: AbortedBySyncTaskHandler.

Renamed test: AbortDuringSyncTask -> AbortBeforeSyncTaskRun (in
order to avoid confusion with the new test).

Added checks for the correct destruction of response objects.

* TestWebKitAPI/Tests/WebCore/AbortableTaskQueue.cpp:
(TestWebKitAPI::FancyResponse::FancyResponse):
(TestWebKitAPI::FancyResponse::~FancyResponse):
(TestWebKitAPI::TEST):

LayoutTests:

Removed timeout expectations for
media/media-source/media-source-error-crash.html

Added expectations for mediasource-invalid-codec.html for Mac, where
WebM is not supported.

* platform/gtk/TestExpectations:
* platform/wpe/TestExpectations:
* platform/mac/imported/w3c/web-platform-tests/media-source/mediasource-invalid-codec-expected.txt: Added.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@238892 268f45cc-cd09-0410-ab3c-d52691b4dbfc

18 files changed:
LayoutTests/ChangeLog
LayoutTests/imported/w3c/ChangeLog
LayoutTests/imported/w3c/web-platform-tests/media-source/mediasource-invalid-codec-expected.txt [new file with mode: 0644]
LayoutTests/imported/w3c/web-platform-tests/media-source/mediasource-invalid-codec.html [new file with mode: 0644]
LayoutTests/imported/w3c/web-platform-tests/media-source/mp4/invalid-codec.mp4 [new file with mode: 0644]
LayoutTests/imported/w3c/web-platform-tests/media-source/webm/invalid-codec.webm [new file with mode: 0644]
LayoutTests/platform/gtk/TestExpectations
LayoutTests/platform/mac/imported/w3c/web-platform-tests/media-source/mediasource-invalid-codec-expected.txt [new file with mode: 0644]
LayoutTests/platform/wpe/TestExpectations
Source/WebCore/ChangeLog
Source/WebCore/platform/AbortableTaskQueue.h
Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.cpp
Source/WebCore/platform/graphics/gstreamer/mse/AppendPipeline.h
Source/WebCore/platform/graphics/gstreamer/mse/MediaSourceClientGStreamerMSE.cpp
Source/WebCore/platform/graphics/gstreamer/mse/SourceBufferPrivateGStreamer.cpp
Source/WebCore/platform/graphics/gstreamer/mse/SourceBufferPrivateGStreamer.h
Tools/ChangeLog
Tools/TestWebKitAPI/Tests/WebCore/AbortableTaskQueue.cpp

index 2b6eb8c..c2c64fe 100644 (file)
@@ -1,3 +1,20 @@
+2018-12-05  Alicia Boya García  <aboya@igalia.com>
+
+        [MSE][GStreamer] Remove the AppendPipeline state machine
+        https://bugs.webkit.org/show_bug.cgi?id=192204
+
+        Reviewed by Xabier Rodriguez-Calvar.
+
+        Removed timeout expectations for
+        media/media-source/media-source-error-crash.html
+
+        Added expectations for mediasource-invalid-codec.html for Mac, where
+        WebM is not supported.
+
+        * platform/gtk/TestExpectations:
+        * platform/wpe/TestExpectations:
+        * platform/mac/imported/w3c/web-platform-tests/media-source/mediasource-invalid-codec-expected.txt: Added.
+
 2018-12-05  Rob Buis  <rbuis@igalia.com>
 
         [Mac] HEAD requests changed to GET after 301, 302, and 303 redirections (http/tests/xmlhttprequest/head-redirection.html)
index 4a214f2..1f4eb24 100644 (file)
@@ -1,3 +1,18 @@
+2018-12-05  Alicia Boya García  <aboya@igalia.com>
+
+        [MSE][GStreamer] Remove the AppendPipeline state machine
+        https://bugs.webkit.org/show_bug.cgi?id=192204
+
+        Reviewed by Xabier Rodriguez-Calvar.
+
+        Added a test checking that initialization segments with invalid codec
+        identifiers are flagged as errors.
+
+        * web-platform-tests/media-source/mediasource-invalid-codec-expected.txt: Added.
+        * web-platform-tests/media-source/mediasource-invalid-codec.html: Added.
+        * web-platform-tests/media-source/mp4/invalid-codec.mp4: Added.
+        * web-platform-tests/media-source/webm/invalid-codec.webm: Added.
+
 2018-12-05  Rob Buis  <rbuis@igalia.com>
 
         [Mac] HEAD requests changed to GET after 301, 302, and 303 redirections (http/tests/xmlhttprequest/head-redirection.html)
diff --git a/LayoutTests/imported/w3c/web-platform-tests/media-source/mediasource-invalid-codec-expected.txt b/LayoutTests/imported/w3c/web-platform-tests/media-source/mediasource-invalid-codec-expected.txt
new file mode 100644 (file)
index 0000000..6d90236
--- /dev/null
@@ -0,0 +1,4 @@
+
+PASS Test an MP4 with an invalid codec results in an error. 
+PASS Test a WebM with an invalid codec results in an error. 
+
diff --git a/LayoutTests/imported/w3c/web-platform-tests/media-source/mediasource-invalid-codec.html b/LayoutTests/imported/w3c/web-platform-tests/media-source/mediasource-invalid-codec.html
new file mode 100644 (file)
index 0000000..19aa00c
--- /dev/null
@@ -0,0 +1,45 @@
+<!DOCTYPE html>
+<html>
+<head>
+    <meta charset="utf-8">
+    <title>SourceBuffer handling of invalid codecs in the initialization segment</title>
+    <link rel="author" title="Alicia Boya García" href="mailto:aboya@igalia.com">
+    <script src="/resources/testharness.js"></script>
+    <script src="/resources/testharnessreport.js"></script>
+    <script src="mediasource-util.js"></script>
+</head>
+<body>
+<div id="log"></div>
+<script>
+    function testInvalidCodec(test, mediaElement, mediaSource, mediaType, url) {
+        assert_true(MediaSource.isTypeSupported(mediaType), `Media type not supported in this browser: isTypeSupported('${mediaType}')`);
+
+        MediaSourceUtil.loadBinaryData(test, url, (mediaData) => {
+            _testInvalidCodecWithData(test, mediaElement, mediaSource, mediaType, mediaData);
+        });
+    }
+
+    function _testInvalidCodecWithData(test, mediaElement, mediaSource, mediaType, mediaData) {
+        const sourceBuffer = mediaSource.addSourceBuffer(mediaType);
+        sourceBuffer.appendBuffer(mediaData);
+        test.expectEvent(sourceBuffer, 'error', 'Append ended with error');
+        test.waitForExpectedEvents(() => {
+            test.done();
+        })
+    }
+
+    // These test cases provide a typical media MIME type, but the actual files have been mangled to declare a different,
+    // unsupported, fictitious codec (MP4 fourcc: 'zzzz', WebM codec id 'V_ZZZ'). The browser should report a parsing
+    // error.
+
+    mediasource_test((test, mediaElement, mediaSource) => {
+        testInvalidCodec(test, mediaElement, mediaSource, 'video/mp4;codecs="avc1.4D4001"', 'mp4/invalid-codec.mp4');
+    }, 'Test an MP4 with an invalid codec results in an error.');
+
+    mediasource_test((test, mediaElement, mediaSource) => {
+        testInvalidCodec(test, mediaElement, mediaSource, 'video/webm; codecs="vp8"', 'webm/invalid-codec.webm');
+    }, 'Test a WebM with an invalid codec results in an error.');
+
+</script>
+</body>
+</html>
diff --git a/LayoutTests/imported/w3c/web-platform-tests/media-source/mp4/invalid-codec.mp4 b/LayoutTests/imported/w3c/web-platform-tests/media-source/mp4/invalid-codec.mp4
new file mode 100644 (file)
index 0000000..6fcc7c2
Binary files /dev/null and b/LayoutTests/imported/w3c/web-platform-tests/media-source/mp4/invalid-codec.mp4 differ
diff --git a/LayoutTests/imported/w3c/web-platform-tests/media-source/webm/invalid-codec.webm b/LayoutTests/imported/w3c/web-platform-tests/media-source/webm/invalid-codec.webm
new file mode 100644 (file)
index 0000000..f1c8bdd
Binary files /dev/null and b/LayoutTests/imported/w3c/web-platform-tests/media-source/webm/invalid-codec.webm differ
index c81e7a3..3688e3c 100644 (file)
@@ -2360,7 +2360,6 @@ webkit.org/b/163780 media/media-controls-drag-timeline-set-controls-property.htm
 
 webkit.org/b/168373 http/tests/media/track-in-band-hls-metadata-crash.html [ Timeout ]
 webkit.org/b/168373 media/media-fullscreen-loop-inline.html [ Timeout ]
-webkit.org/b/168373 media/media-source/media-source-error-crash.html [ Timeout ]
 webkit.org/b/168373 media/media-source/only-bcp47-language-tags-accepted-as-valid.html [ Timeout ]
 
 webkit.org/b/169211 fast/parser/adoption-agency-unload-iframe-4.html [ Timeout ]
diff --git a/LayoutTests/platform/mac/imported/w3c/web-platform-tests/media-source/mediasource-invalid-codec-expected.txt b/LayoutTests/platform/mac/imported/w3c/web-platform-tests/media-source/mediasource-invalid-codec-expected.txt
new file mode 100644 (file)
index 0000000..0a3fdfe
--- /dev/null
@@ -0,0 +1,4 @@
+
+PASS Test an MP4 with an invalid codec results in an error. 
+FAIL Test a WebM with an invalid codec results in an error. assert_true: Media type not supported in this browser: isTypeSupported('video/webm; codecs="vp8"') expected true got false
+
index 12a6a56..5c70a82 100644 (file)
@@ -1213,7 +1213,6 @@ legacy-animation-engine/transitions/svg-text-shadow-transition.html [ Pass ]
 
 # ENABLE_MEDIA_SOURCE
 media/media-source/ [ Pass ]
-webkit.org/b/168373 media/media-source/media-source-error-crash.html [ Timeout ]
 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 ]
index 4b5e21b..55139e2 100644 (file)
@@ -1,3 +1,77 @@
+2018-12-05  Alicia Boya García  <aboya@igalia.com>
+
+        [MSE][GStreamer] Remove the AppendPipeline state machine
+        https://bugs.webkit.org/show_bug.cgi?id=192204
+
+        Reviewed by Xabier Rodriguez-Calvar.
+
+        This patch tries to reduce the complexity of the AppendPipeline by
+        removing the appendState state machine and cleaning all the
+        conditional code around it that is not necessary anymore.
+
+        For the most part the behavior is the same, but some edge cases have
+        been improved in the process:
+
+        Demuxing errors now result in the append being flagged as
+        ParsingFailed and the error being propagated to the application. This
+        fixes media/media-source/media-source-error-crash.html (or at least
+        gets it up to date with cross platform expectations).
+
+        AbortableTaskQueue now allows the task handler to perform an abort
+        safely. This is used in the GstBus error message sync handler, since
+        it needs to ask the MainThread to raise a parse error, which will in
+        turn abort. An API test has been added for this new functionality.
+        Also, code has been added to the API tests to ensure the correct
+        destruction of the response object, especially in this case.
+
+        The code handling invalid track codecs has been made clearer by also
+        explicitly raising a parse error, but it should not expose behavior
+        differences for the application. A test has been added for this
+        behavior: web-platform-tests/media-source/mediasource-invalid-codec.html
+
+        The reporting of EOS events have been made more rigorous. EOS is only
+        expected after a demuxing error, otherwise it's a g_critical.
+
+        AppendPipeline::abort() has been renamed to
+        AppendPipeline::resetParserState() to honor the fact that it's not
+        only called when the user calls abort() and match better the names
+        used in the spec.
+
+        Test: imported/w3c/web-platform-tests/media-source/mediasource-invalid-codec.html
+
+        * platform/AbortableTaskQueue.h:
+        * platform/graphics/gstreamer/mse/AppendPipeline.cpp:
+        (WebCore::assertedElementSetState):
+        (WebCore::AppendPipeline::AppendPipeline):
+        (WebCore::AppendPipeline::~AppendPipeline):
+        (WebCore::AppendPipeline::handleErrorSyncMessage):
+        (WebCore::AppendPipeline::appsrcEndOfAppendCheckerProbe):
+        (WebCore::AppendPipeline::handleNeedContextSyncMessage):
+        (WebCore::AppendPipeline::appsinkCapsChanged):
+        (WebCore::AppendPipeline::handleEndOfAppend):
+        (WebCore::AppendPipeline::appsinkNewSample):
+        (WebCore::AppendPipeline::didReceiveInitializationSegment):
+        (WebCore::AppendPipeline::resetParserState):
+        (WebCore::AppendPipeline::pushNewBuffer):
+        (WebCore::AppendPipeline::handleAppsinkNewSampleFromStreamingThread):
+        (WebCore::AppendPipeline::connectDemuxerSrcPadToAppsinkFromStreamingThread):
+        (WebCore::AppendPipeline::connectDemuxerSrcPadToAppsink):
+        (WebCore::AppendPipeline::disconnectDemuxerSrcPadFromAppsinkFromAnyThread):
+        (WebCore::AppendPipeline::dumpAppendState): Deleted.
+        (WebCore::AppendPipeline::demuxerNoMorePads): Deleted.
+        (WebCore::AppendPipeline::setAppendState): Deleted.
+        (WebCore::AppendPipeline::appsinkEOS): Deleted.
+        (WebCore::AppendPipeline::resetPipeline): Deleted.
+        (WebCore::AppendPipeline::abort): Deleted.
+        * platform/graphics/gstreamer/mse/AppendPipeline.h:
+        (WebCore::AppendPipeline::appendState): Deleted.
+        * platform/graphics/gstreamer/mse/MediaSourceClientGStreamerMSE.cpp:
+        (WebCore::MediaSourceClientGStreamerMSE::abort):
+        (WebCore::MediaSourceClientGStreamerMSE::resetParserState):
+        * platform/graphics/gstreamer/mse/SourceBufferPrivateGStreamer.cpp:
+        (WebCore::SourceBufferPrivateGStreamer::appendParsingFailed):
+        * platform/graphics/gstreamer/mse/SourceBufferPrivateGStreamer.h:
+
 2018-12-05  Rob Buis  <rbuis@igalia.com>
 
         [Mac] HEAD requests changed to GET after 301, 302, and 303 redirections (http/tests/xmlhttprequest/head-redirection.html)
index 78a37f8..a669d94 100644 (file)
@@ -134,6 +134,9 @@ public:
     // forwarded to the background thread, wrapped in an optional.
     //
     // If we are aborting, the call finishes immediately, returning an empty optional.
+    //
+    // It is allowed for the main thread task handler to abort the AbortableTaskQueue. In that case, the return
+    // value is discarded and the caller receives an empty optional.
     template<typename R>
     std::optional<R> enqueueTaskAndWait(WTF::Function<R()>&& mainThreadTaskHandler)
     {
@@ -148,7 +151,8 @@ public:
         postTask([this, &response, &mainThreadTaskHandler]() {
             R responseValue = mainThreadTaskHandler();
             LockHolder lockHolder(m_mutex);
-            response = WTFMove(responseValue);
+            if (!m_aborting)
+                response = WTFMove(responseValue);
             m_abortedOrResponseSet.notifyAll();
         });
         m_abortedOrResponseSet.wait(m_mutex, [this, &response]() {
index b1fbac6..026a205 100644 (file)
@@ -67,28 +67,6 @@ void AppendPipeline::staticInitialization()
     s_webKitEndOfAppendMetaInfo = gst_meta_register(s_endOfAppendMetaType, "WebKitEndOfAppendMeta", sizeof(EndOfAppendMeta), EndOfAppendMeta::init, EndOfAppendMeta::free, EndOfAppendMeta::transform);
 }
 
-const char* AppendPipeline::dumpAppendState(AppendPipeline::AppendState appendState)
-{
-    switch (appendState) {
-    case AppendPipeline::AppendState::Invalid:
-        return "Invalid";
-    case AppendPipeline::AppendState::NotStarted:
-        return "NotStarted";
-    case AppendPipeline::AppendState::Ongoing:
-        return "Ongoing";
-    case AppendPipeline::AppendState::DataStarve:
-        return "DataStarve";
-    case AppendPipeline::AppendState::Sampling:
-        return "Sampling";
-    case AppendPipeline::AppendState::LastSample:
-        return "LastSample";
-    case AppendPipeline::AppendState::Aborting:
-        return "Aborting";
-    default:
-        return "(unknown)";
-    }
-}
-
 #if !LOG_DISABLED
 static GstPadProbeReturn appendPipelinePadProbeDebugInformation(GstPad*, GstPadProbeInfo*, struct PadProbeInformation*);
 #endif
@@ -101,14 +79,30 @@ static GstPadProbeReturn appendPipelineDemuxerBlackHolePadProbe(GstPad*, GstPadP
 
 static GstPadProbeReturn matroskademuxForceSegmentStartToEqualZero(GstPad*, GstPadProbeInfo*, void*);
 
+// Wrapper for gst_element_set_state() that emits a critical if the state change fails or is not synchronous.
+static void assertedElementSetState(GstElement* element, GstState desiredState)
+{
+    GstState oldState;
+    gst_element_get_state(element, &oldState, nullptr, 0);
+
+    GstStateChangeReturn result = gst_element_set_state(element, desiredState);
+
+    GstState newState;
+    gst_element_get_state(element, &newState, nullptr, 0);
+
+    if (desiredState != newState || result != GST_STATE_CHANGE_SUCCESS) {
+        GST_ERROR("AppendPipeline state change failed (returned %d): %" GST_PTR_FORMAT " %d -> %d (expected %d)",
+            static_cast<int>(result), element, static_cast<int>(oldState), static_cast<int>(newState), static_cast<int>(desiredState));
+        ASSERT_NOT_REACHED();
+    }
+}
+
 AppendPipeline::AppendPipeline(Ref<MediaSourceClientGStreamerMSE> mediaSourceClient, Ref<SourceBufferPrivateGStreamer> sourceBufferPrivate, MediaPlayerPrivateGStreamerMSE& playerPrivate)
     : m_mediaSourceClient(mediaSourceClient.get())
     , m_sourceBufferPrivate(sourceBufferPrivate.get())
     , m_playerPrivate(&playerPrivate)
     , m_id(0)
     , m_wasBusAlreadyNotifiedOfAvailableSamples(false)
-    , m_appendState(AppendState::NotStarted)
-    , m_abortPending(false)
     , m_streamType(Unknown)
 {
     ASSERT(isMainThread());
@@ -127,6 +121,9 @@ AppendPipeline::AppendPipeline(Ref<MediaSourceClientGStreamerMSE> mediaSourceCli
     gst_bus_add_signal_watch_full(m_bus.get(), RunLoopSourcePriority::RunLoopDispatcher);
     gst_bus_enable_sync_message_emission(m_bus.get());
 
+    g_signal_connect(m_bus.get(), "sync-message::error", G_CALLBACK(+[](GstBus*, GstMessage* message, AppendPipeline* appendPipeline) {
+        appendPipeline->handleErrorSyncMessage(message);
+    }), this);
     g_signal_connect(m_bus.get(), "sync-message::need-context", G_CALLBACK(+[](GstBus*, GstMessage* message, AppendPipeline* appendPipeline) {
         appendPipeline->handleNeedContextSyncMessage(message);
     }), this);
@@ -155,6 +152,8 @@ AppendPipeline::AppendPipeline(Ref<MediaSourceClientGStreamerMSE> mediaSourceCli
 
     gst_app_sink_set_emit_signals(GST_APP_SINK(m_appsink.get()), TRUE);
     gst_base_sink_set_sync(GST_BASE_SINK(m_appsink.get()), FALSE);
+    gst_base_sink_set_async_enabled(GST_BASE_SINK(m_appsink.get()), FALSE); // No prerolls, no async state changes.
+    gst_base_sink_set_drop_out_of_segment(GST_BASE_SINK(m_appsink.get()), FALSE);
     gst_base_sink_set_last_sample_enabled(GST_BASE_SINK(m_appsink.get()), FALSE);
 
     GRefPtr<GstPad> appsinkPad = adoptGRef(gst_element_get_static_pad(m_appsink.get(), "sink"));
@@ -202,33 +201,32 @@ AppendPipeline::AppendPipeline(Ref<MediaSourceClientGStreamerMSE> mediaSourceCli
         ASSERT(!isMainThread());
         GST_DEBUG("Posting no-more-pads task to main thread");
         appendPipeline->m_taskQueue.enqueueTask([appendPipeline]() {
-            appendPipeline->demuxerNoMorePads();
+            appendPipeline->didReceiveInitializationSegment();
         });
     }), this);
     g_signal_connect(m_appsink.get(), "new-sample", G_CALLBACK(+[](GstElement* appsink, AppendPipeline* appendPipeline) {
         appendPipeline->handleAppsinkNewSampleFromStreamingThread(appsink);
     }), this);
     g_signal_connect(m_appsink.get(), "eos", G_CALLBACK(+[](GstElement*, AppendPipeline* appendPipeline) {
-        ASSERT(!isMainThread());
-        GST_DEBUG("Posting appsink-eos task to main thread");
-        appendPipeline->m_taskQueue.enqueueTask([appendPipeline]() {
-            appendPipeline->appsinkEOS();
-        });
+        // 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();
+        }
     }), this);
 
     // Add_many will take ownership of a reference. That's why we used an assignment before.
     gst_bin_add_many(GST_BIN(m_pipeline.get()), m_appsrc.get(), m_demux.get(), nullptr);
     gst_element_link(m_appsrc.get(), m_demux.get());
 
-    gst_element_set_state(m_pipeline.get(), GST_STATE_READY);
-};
+    assertedElementSetState(m_pipeline.get(), GST_STATE_PLAYING);
+}
 
 AppendPipeline::~AppendPipeline()
 {
-    GST_TRACE("Destructing AppendPipeline (%p)", this);
+    GST_DEBUG_OBJECT(m_pipeline.get(), "Destructing AppendPipeline (%p)", this);
     ASSERT(isMainThread());
 
-    setAppendState(AppendState::Invalid);
     // Forget all pending tasks and unblock the streaming thread if it was blocked.
     m_taskQueue.startAborting();
 
@@ -273,6 +271,21 @@ AppendPipeline::~AppendPipeline()
         gst_element_set_state(m_pipeline.get(), GST_STATE_NULL);
 }
 
+void AppendPipeline::handleErrorSyncMessage(GstMessage* message)
+{
+    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]() {
+        m_errorReceived = true;
+        // appendParsingFailed() will cause resetParserState() to be called.
+        m_sourceBufferPrivate->appendParsingFailed();
+        return AbortableTaskQueue::Void();
+    });
+    // The streaming thread has now been unblocked because we are aborting in the main thread.
+    ASSERT(!response);
+}
+
 GstPadProbeReturn AppendPipeline::appsrcEndOfAppendCheckerProbe(GstPadProbeInfo* padProbeInfo)
 {
     ASSERT(!isMainThread());
@@ -281,6 +294,8 @@ GstPadProbeReturn AppendPipeline::appsrcEndOfAppendCheckerProbe(GstPadProbeInfo*
     GstBuffer* buffer = GST_BUFFER(padProbeInfo->data);
     ASSERT(GST_IS_BUFFER(buffer));
 
+    GST_TRACE_OBJECT(m_pipeline.get(), "Buffer entered appsrcEndOfAppendCheckerProbe: %" GST_PTR_FORMAT, buffer);
+
     EndOfAppendMeta* endOfAppendMeta = reinterpret_cast<EndOfAppendMeta*>(gst_buffer_get_meta(buffer, s_endOfAppendMetaType));
     if (!endOfAppendMeta) {
         // Normal buffer, nothing to do.
@@ -301,16 +316,7 @@ void AppendPipeline::handleNeedContextSyncMessage(GstMessage* message)
     GST_TRACE("context type: %s", contextType);
 
     // MediaPlayerPrivateGStreamerBase will take care of setting up encryption.
-    if (m_playerPrivate)
-        m_playerPrivate->handleSyncMessage(message);
-}
-
-void AppendPipeline::demuxerNoMorePads()
-{
-    GST_TRACE("calling didReceiveInitializationSegment");
-    didReceiveInitializationSegment();
-    GST_TRACE("set pipeline to playing");
-    gst_element_set_state(m_pipeline.get(), GST_STATE_PLAYING);
+    m_playerPrivate->handleSyncMessage(message);
 }
 
 void AppendPipeline::handleStateChangeMessage(GstMessage* message)
@@ -364,146 +370,6 @@ gint AppendPipeline::id()
     return m_id;
 }
 
-void AppendPipeline::setAppendState(AppendState newAppendState)
-{
-    ASSERT(isMainThread());
-    // Valid transitions:
-    // NotStarted-->Ongoing-->DataStarve-->NotStarted
-    //           |         |            `->Aborting-->NotStarted
-    //           |         `->Sampling-···->Sampling-->LastSample-->NotStarted
-    //           |                                               `->Aborting-->NotStarted
-    //           `->Aborting-->NotStarted
-    AppendState oldAppendState = m_appendState;
-    AppendState nextAppendState = AppendState::Invalid;
-
-    if (oldAppendState != newAppendState)
-        GST_TRACE("%s --> %s", dumpAppendState(oldAppendState), dumpAppendState(newAppendState));
-
-    bool ok = false;
-
-    switch (oldAppendState) {
-    case AppendState::NotStarted:
-        switch (newAppendState) {
-        case AppendState::Ongoing:
-            ok = true;
-            gst_element_set_state(m_pipeline.get(), GST_STATE_PLAYING);
-            break;
-        case AppendState::NotStarted:
-            ok = true;
-            if (m_pendingBuffer) {
-                GST_TRACE("pushing pending buffer %" GST_PTR_FORMAT, m_pendingBuffer.get());
-                gst_app_src_push_buffer(GST_APP_SRC(appsrc()), m_pendingBuffer.leakRef());
-                nextAppendState = AppendState::Ongoing;
-            }
-            break;
-        case AppendState::Aborting:
-            ok = true;
-            nextAppendState = AppendState::NotStarted;
-            break;
-        case AppendState::Invalid:
-            ok = true;
-            break;
-        default:
-            break;
-        }
-        break;
-    case AppendState::Ongoing:
-        switch (newAppendState) {
-        case AppendState::Sampling:
-        case AppendState::Invalid:
-            ok = true;
-            break;
-        case AppendState::DataStarve:
-            ok = true;
-            GST_DEBUG("received all pending samples");
-            m_sourceBufferPrivate->didReceiveAllPendingSamples();
-            if (m_abortPending)
-                nextAppendState = AppendState::Aborting;
-            else
-                nextAppendState = AppendState::NotStarted;
-            break;
-        default:
-            break;
-        }
-        break;
-    case AppendState::DataStarve:
-        switch (newAppendState) {
-        case AppendState::NotStarted:
-        case AppendState::Invalid:
-            ok = true;
-            break;
-        case AppendState::Aborting:
-            ok = true;
-            nextAppendState = AppendState::NotStarted;
-            break;
-        default:
-            break;
-        }
-        break;
-    case AppendState::Sampling:
-        switch (newAppendState) {
-        case AppendState::Sampling:
-        case AppendState::Invalid:
-            ok = true;
-            break;
-        case AppendState::LastSample:
-            ok = true;
-            GST_DEBUG("received all pending samples");
-            m_sourceBufferPrivate->didReceiveAllPendingSamples();
-            if (m_abortPending)
-                nextAppendState = AppendState::Aborting;
-            else
-                nextAppendState = AppendState::NotStarted;
-            break;
-        default:
-            break;
-        }
-        break;
-    case AppendState::LastSample:
-        switch (newAppendState) {
-        case AppendState::NotStarted:
-        case AppendState::Invalid:
-            ok = true;
-            break;
-        case AppendState::Aborting:
-            ok = true;
-            nextAppendState = AppendState::NotStarted;
-            break;
-        default:
-            break;
-        }
-        break;
-    case AppendState::Aborting:
-        switch (newAppendState) {
-        case AppendState::NotStarted:
-            ok = true;
-            resetPipeline();
-            m_abortPending = false;
-            nextAppendState = AppendState::NotStarted;
-            break;
-        case AppendState::Invalid:
-            ok = true;
-            break;
-        default:
-            break;
-        }
-        break;
-    case AppendState::Invalid:
-        ok = true;
-        break;
-    }
-
-    if (ok)
-        m_appendState = newAppendState;
-    else
-        GST_ERROR("Invalid append state transition %s --> %s", dumpAppendState(oldAppendState), dumpAppendState(newAppendState));
-
-    ASSERT(ok);
-
-    if (nextAppendState != AppendState::Invalid)
-        setAppendState(nextAppendState);
-}
-
 void AppendPipeline::parseDemuxerSrcPadCaps(GstCaps* demuxerSrcPadCaps)
 {
     ASSERT(isMainThread());
@@ -536,9 +402,6 @@ void AppendPipeline::appsinkCapsChanged()
 {
     ASSERT(isMainThread());
 
-    if (!m_appsink)
-        return;
-
     GRefPtr<GstPad> pad = adoptGRef(gst_element_get_static_pad(m_appsink.get(), "sink"));
     GRefPtr<GstCaps> caps = adoptGRef(gst_pad_get_current_caps(pad.get()));
 
@@ -550,31 +413,16 @@ void AppendPipeline::appsinkCapsChanged()
 
     if (m_appsinkCaps != caps) {
         m_appsinkCaps = WTFMove(caps);
-        if (m_playerPrivate)
-            m_playerPrivate->trackDetected(this, m_track, previousCapsWereNull);
-        gst_element_set_state(m_pipeline.get(), GST_STATE_PLAYING);
+        m_playerPrivate->trackDetected(this, m_track, previousCapsWereNull);
     }
 }
 
 void AppendPipeline::handleEndOfAppend()
 {
     ASSERT(isMainThread());
-    GST_TRACE_OBJECT(m_pipeline.get(), "received end-of-append");
-
-    // Regardless of the state transition, the result is the same: didReceiveAllPendingSamples() is called.
-    switch (m_appendState) {
-    case AppendState::Ongoing:
-        GST_TRACE("DataStarve");
-        setAppendState(AppendState::DataStarve);
-        break;
-    case AppendState::Sampling:
-        GST_TRACE("LastSample");
-        setAppendState(AppendState::LastSample);
-        break;
-    default:
-        ASSERT_NOT_REACHED();
-        break;
-    }
+    consumeAppsinkAvailableSamples();
+    GST_TRACE_OBJECT(m_pipeline.get(), "Notifying SourceBufferPrivate the append is complete");
+    sourceBufferPrivate()->didReceiveAllPendingSamples();
 }
 
 void AppendPipeline::appsinkNewSample(GRefPtr<GstSample>&& sample)
@@ -595,11 +443,10 @@ void AppendPipeline::appsinkNewSample(GRefPtr<GstSample>&& sample)
         mediaSample->duration().toString().utf8().data(),
         mediaSample->presentationSize().width(), mediaSample->presentationSize().height());
 
-    // If we're beyond the duration, ignore this sample and the remaining ones.
+    // If we're beyond the duration, ignore this sample.
     MediaTime duration = m_mediaSourceClient->duration();
     if (duration.isValid() && !duration.indefiniteTime() && mediaSample->presentationTime() > duration) {
-        GST_DEBUG("Detected sample (%f) beyond the duration (%f), declaring LastSample", mediaSample->presentationTime().toFloat(), duration.toFloat());
-        setAppendState(AppendState::LastSample);
+        GST_DEBUG_OBJECT(m_pipeline.get(), "Detected sample (%s) beyond the duration (%s), discarding", mediaSample->presentationTime().toString().utf8().data(), duration.toString().utf8().data());
         return;
     }
 
@@ -610,28 +457,6 @@ void AppendPipeline::appsinkNewSample(GRefPtr<GstSample>&& sample)
     }
 
     m_sourceBufferPrivate->didReceiveSample(*mediaSample);
-    setAppendState(AppendState::Sampling);
-}
-
-void AppendPipeline::appsinkEOS()
-{
-    ASSERT(isMainThread());
-
-    switch (m_appendState) {
-    case AppendState::Aborting:
-        // Ignored. Operation completion will be managed by the Aborting->NotStarted transition.
-        return;
-    case AppendState::Ongoing:
-        // Finish Ongoing and Sampling states.
-        setAppendState(AppendState::DataStarve);
-        break;
-    case AppendState::Sampling:
-        setAppendState(AppendState::LastSample);
-        break;
-    default:
-        GST_DEBUG("Unexpected EOS");
-        break;
-    }
 }
 
 void AppendPipeline::didReceiveInitializationSegment()
@@ -690,51 +515,45 @@ void AppendPipeline::consumeAppsinkAvailableSamples()
     GST_TRACE_OBJECT(m_pipeline.get(), "batchedSampleCount = %d", batchedSampleCount);
 }
 
-void AppendPipeline::resetPipeline()
+void AppendPipeline::resetParserState()
 {
     ASSERT(isMainThread());
-    GST_DEBUG("resetting pipeline");
+    GST_DEBUG_OBJECT(m_pipeline.get(), "Handling resetParserState() in AppendPipeline by resetting the pipeline");
+
+    // FIXME: Implement a flush event-based resetParserState() implementation would allow the initialization segment to
+    // survive, in accordance with the spec.
+
+    // This function restores the GStreamer pipeline to the same state it was when the AppendPipeline constructor
+    // finished. All previously enqueued data is lost and the demuxer is reset, losing all pads and track data.
+
+    // Unlock the streaming thread.
+    m_taskQueue.startAborting();
+
+    // Reset the state of all elements in the pipeline.
+    assertedElementSetState(m_pipeline.get(), GST_STATE_READY);
+
+    // The parser is tear down automatically when the demuxer is reset (see disconnectDemuxerSrcPadFromAppsinkFromAnyThread()).
+    ASSERT(!m_parser);
+
+    // Set the pipeline to PLAYING so that it can be used again.
+    assertedElementSetState(m_pipeline.get(), GST_STATE_PLAYING);
 
-    gst_element_set_state(m_pipeline.get(), GST_STATE_READY);
-    gst_element_get_state(m_pipeline.get(), nullptr, nullptr, 0);
+    // All processing related to the previous append has been aborted and the pipeline is idle.
+    // We can listen again to new requests coming from the streaming thread.
+    m_taskQueue.finishAborting();
 
 #if (!(LOG_DISABLED || defined(GST_DISABLE_GST_DEBUG)))
     {
         static unsigned i = 0;
         // This is here for debugging purposes. It does not make sense to have it as class member.
-        WTF::String  dotFileName = String::format("reset-pipeline-%d", ++i);
+        WTF::String dotFileName = String::format("reset-pipeline-%d", ++i);
         gst_debug_bin_to_dot_file(GST_BIN(m_pipeline.get()), GST_DEBUG_GRAPH_SHOW_ALL, dotFileName.utf8().data());
     }
 #endif
-
-}
-
-void AppendPipeline::abort()
-{
-    ASSERT(isMainThread());
-    GST_DEBUG("aborting");
-
-    m_pendingBuffer = nullptr;
-
-    // Abort already ongoing.
-    if (m_abortPending)
-        return;
-
-    m_abortPending = true;
-    if (m_appendState == AppendState::NotStarted)
-        setAppendState(AppendState::Aborting);
-    // Else, the automatic state transitions will take care when the ongoing append finishes.
 }
 
 GstFlowReturn AppendPipeline::pushNewBuffer(GstBuffer* buffer)
 {
-    if (m_abortPending) {
-        m_pendingBuffer = adoptGRef(buffer);
-        return GST_FLOW_OK;
-    }
-
-    setAppendState(AppendPipeline::AppendState::Ongoing);
-
     GST_TRACE_OBJECT(m_pipeline.get(), "pushing data buffer %" GST_PTR_FORMAT, buffer);
     GstFlowReturn pushDataBufferRet = gst_app_src_push_buffer(GST_APP_SRC(m_appsrc.get()), buffer);
     // Pushing buffers to appsrc can only fail if the appsrc is flushing, in EOS or stopped. Neither of these should
@@ -770,15 +589,10 @@ GstFlowReturn AppendPipeline::handleAppsinkNewSampleFromStreamingThread(GstEleme
         // removes the appsrcEndOfAppendCheckerProbe(). Either way, the end-of-append detection would be broken.
         // AppendPipeline should have only one streaming thread. Otherwise we can't detect reliably when an appends has
         // been demuxed completely.;
-        g_critical("Appsink received a sample in a different thread than appsrcEndOfAppendCheckerProbe run.");
+        GST_ERROR_OBJECT(m_pipeline.get(), "Appsink received a sample in a different thread than appsrcEndOfAppendCheckerProbe run.");
         ASSERT_NOT_REACHED();
     }
 
-    if (!m_playerPrivate || m_appendState == AppendState::Invalid) {
-        GST_WARNING("AppendPipeline has been disabled, ignoring this sample");
-        return GST_FLOW_ERROR;
-    }
-
     if (!m_wasBusAlreadyNotifiedOfAvailableSamples.test_and_set()) {
         GST_TRACE("Posting appsink-new-sample task to the main thread");
         m_taskQueue.enqueueTask([this]() {
@@ -825,8 +639,6 @@ createOptionalParserForFormat(GstPad* demuxerSrcPad)
 void AppendPipeline::connectDemuxerSrcPadToAppsinkFromStreamingThread(GstPad* demuxerSrcPad)
 {
     ASSERT(!isMainThread());
-    if (!m_appsink)
-        return;
 
     GST_DEBUG("connecting to appsink");
 
@@ -865,7 +677,6 @@ void AppendPipeline::connectDemuxerSrcPadToAppsinkFromStreamingThread(GstPad* de
         || (m_streamType == WebCore::MediaSourceStreamTypeGStreamer::Text);
 
     if (isData) {
-        // FIXME: Only add appsink one time. This method can be called several times.
         GRefPtr<GstObject> parent = adoptGRef(gst_element_get_parent(m_appsink.get()));
         if (!parent)
             gst_bin_add(GST_BIN(m_pipeline.get()), m_appsink.get());
@@ -892,9 +703,6 @@ void AppendPipeline::connectDemuxerSrcPadToAppsinkFromStreamingThread(GstPad* de
 
         gst_element_sync_state_with_parent(m_appsink.get());
 
-        gst_element_set_state(m_pipeline.get(), GST_STATE_PAUSED);
-        gst_element_sync_state_with_parent(m_appsink.get());
-
         GST_DEBUG_BIN_TO_DOT_FILE_WITH_TS(GST_BIN(m_pipeline.get()), GST_DEBUG_GRAPH_SHOW_ALL, "webkit-after-link");
     }
 }
@@ -915,10 +723,6 @@ void AppendPipeline::connectDemuxerSrcPadToAppsink(GstPad* demuxerSrcPad)
 
     GRefPtr<GstCaps> caps = adoptGRef(gst_pad_get_current_caps(GST_PAD(demuxerSrcPad)));
 
-    if (!caps || m_appendState == AppendState::Invalid || !m_playerPrivate) {
-        return;
-    }
-
 #ifndef GST_DISABLE_GST_DEBUG
     {
         GUniquePtr<gchar> strcaps(gst_caps_to_string(caps.get()));
@@ -933,34 +737,31 @@ void AppendPipeline::connectDemuxerSrcPadToAppsink(GstPad* demuxerSrcPad)
 
     switch (m_streamType) {
     case WebCore::MediaSourceStreamTypeGStreamer::Audio:
-        if (m_playerPrivate)
-            m_track = WebCore::AudioTrackPrivateGStreamer::create(makeWeakPtr(*m_playerPrivate), id(), sinkSinkPad.get());
+        m_track = WebCore::AudioTrackPrivateGStreamer::create(makeWeakPtr(*m_playerPrivate), id(), sinkSinkPad.get());
         break;
     case WebCore::MediaSourceStreamTypeGStreamer::Video:
-        if (m_playerPrivate)
-            m_track = WebCore::VideoTrackPrivateGStreamer::create(makeWeakPtr(*m_playerPrivate), id(), sinkSinkPad.get());
+        m_track = WebCore::VideoTrackPrivateGStreamer::create(makeWeakPtr(*m_playerPrivate), id(), sinkSinkPad.get());
         break;
     case WebCore::MediaSourceStreamTypeGStreamer::Text:
         m_track = WebCore::InbandTextTrackPrivateGStreamer::create(id(), sinkSinkPad.get());
         break;
     case WebCore::MediaSourceStreamTypeGStreamer::Invalid:
-        {
-            GUniquePtr<gchar> strcaps(gst_caps_to_string(caps.get()));
-            GST_DEBUG("Unsupported track codec: %s", strcaps.get());
-        }
-        // This is going to cause an error which will detach the SourceBuffer and tear down this
-        // AppendPipeline, so we need the padAddRemove lock released before continuing.
-        m_track = nullptr;
-        didReceiveInitializationSegment();
+        GST_WARNING_OBJECT(m_pipeline.get(), "Unsupported track codec: %" GST_PTR_FORMAT, caps.get());
+        // 3.5.7 Initialization Segment Received
+        // 5.1. If the initialization segment contains tracks with codecs the user agent does not support, then run the
+        // append error algorithm and abort these steps.
+
+        // appendParsingFailed() will immediately cause a resetParserState() which will stop demuxing, then the
+        // AppendPipeline will be destroyed.
+        m_sourceBufferPrivate->appendParsingFailed();
         return;
     default:
-        // No useful data.
+        GST_WARNING_OBJECT(m_pipeline.get(), "Pad has unknown track type, ignoring: %" GST_PTR_FORMAT, caps.get());
         break;
     }
 
     m_appsinkCaps = WTFMove(caps);
-    if (m_playerPrivate)
-        m_playerPrivate->trackDetected(this, m_track, true);
+    m_playerPrivate->trackDetected(this, m_track, true);
 }
 
 void AppendPipeline::disconnectDemuxerSrcPadFromAppsinkFromAnyThread(GstPad*)
@@ -973,7 +774,7 @@ void AppendPipeline::disconnectDemuxerSrcPadFromAppsinkFromAnyThread(GstPad*)
     GST_DEBUG("Disconnecting appsink");
 
     if (m_parser) {
-        gst_element_set_state(m_parser.get(), GST_STATE_NULL);
+        assertedElementSetState(m_parser.get(), GST_STATE_NULL);
         gst_bin_remove(GST_BIN(m_pipeline.get()), m_parser.get());
         m_parser = nullptr;
     }
index 871b436..d3a520d 100644 (file)
@@ -50,21 +50,20 @@ public:
     virtual ~AppendPipeline();
 
     GstFlowReturn pushNewBuffer(GstBuffer*);
-    void abort();
+    void resetParserState();
     Ref<SourceBufferPrivateGStreamer> sourceBufferPrivate() { return m_sourceBufferPrivate.get(); }
     GstCaps* appsinkCaps() { return m_appsinkCaps.get(); }
     RefPtr<WebCore::TrackPrivateBase> track() { return m_track; }
     MediaPlayerPrivateGStreamerMSE* playerPrivate() { return m_playerPrivate; }
 
 private:
-    enum class AppendState { Invalid, NotStarted, Ongoing, DataStarve, Sampling, LastSample, Aborting };
 
+    void handleErrorSyncMessage(GstMessage*);
     void handleNeedContextSyncMessage(GstMessage*);
+    // For debug purposes only:
     void handleStateChangeMessage(GstMessage*);
 
     gint id();
-    AppendState appendState() { return m_appendState; }
-    void setAppendState(AppendState);
 
     GstFlowReturn handleAppsinkNewSampleFromStreamingThread(GstElement*);
 
@@ -72,7 +71,6 @@ private:
     void parseDemuxerSrcPadCaps(GstCaps*);
     void appsinkCapsChanged();
     void appsinkNewSample(GRefPtr<GstSample>&&);
-    void appsinkEOS();
     void handleEndOfAppend();
     void didReceiveInitializationSegment();
     AtomicString trackId();
@@ -89,15 +87,12 @@ private:
     void connectDemuxerSrcPadToAppsink(GstPad*);
 
     void resetPipeline();
-    void checkEndOfAppend();
-    void demuxerNoMorePads();
 
     void consumeAppsinkAvailableSamples();
 
     GstPadProbeReturn appsrcEndOfAppendCheckerProbe(GstPadProbeInfo*);
 
     static void staticInitialization();
-    static const char* dumpAppendState(AppendPipeline::AppendState);
 
     static std::once_flag s_staticInitializationFlag;
     static GType s_endOfAppendMetaType;
@@ -107,6 +102,9 @@ private:
     // Only the pointers are compared.
     WTF::Thread* m_streamingThread;
 
+    // Used only for asserting EOS events are only caused by demuxing errors.
+    bool m_errorReceived { false };
+
     Ref<MediaSourceClientGStreamerMSE> m_mediaSourceClient;
     Ref<SourceBufferPrivateGStreamer> m_sourceBufferPrivate;
     MediaPlayerPrivateGStreamerMSE* m_playerPrivate;
@@ -143,14 +141,6 @@ private:
 #if ENABLE(ENCRYPTED_MEDIA)
     struct PadProbeInformation m_appsinkPadEventProbeInformation;
 #endif
-    // Keeps track of the states of append processing, to avoid performing actions inappropriate for the current state
-    // (eg: processing more samples when the last one has been detected, etc.). See setAppendState() for valid
-    // transitions.
-    AppendState m_appendState;
-
-    // Aborts can only be completed when the normal sample detection has finished. Meanwhile, the willing to abort is
-    // expressed in this field.
-    bool m_abortPending;
 
     WebCore::MediaSourceStreamTypeGStreamer m_streamType;
     RefPtr<WebCore::TrackPrivateBase> m_track;
index 38bdd6c..c84fc36 100644 (file)
@@ -106,7 +106,7 @@ void MediaSourceClientGStreamerMSE::abort(RefPtr<SourceBufferPrivateGStreamer> s
 
     ASSERT(appendPipeline);
 
-    appendPipeline->abort();
+    appendPipeline->resetParserState();
 }
 
 void MediaSourceClientGStreamerMSE::resetParserState(RefPtr<SourceBufferPrivateGStreamer> sourceBufferPrivate)
@@ -122,7 +122,7 @@ void MediaSourceClientGStreamerMSE::resetParserState(RefPtr<SourceBufferPrivateG
 
     ASSERT(appendPipeline);
 
-    appendPipeline->abort();
+    appendPipeline->resetParserState();
 }
 
 bool MediaSourceClientGStreamerMSE::append(RefPtr<SourceBufferPrivateGStreamer> sourceBufferPrivate, Vector<unsigned char>&& data)
index 0f1c4e6..fa4f561 100644 (file)
@@ -178,5 +178,11 @@ void SourceBufferPrivateGStreamer::didReceiveAllPendingSamples()
         m_sourceBufferPrivateClient->sourceBufferPrivateAppendComplete(SourceBufferPrivateClient::AppendSucceeded);
 }
 
+void SourceBufferPrivateGStreamer::appendParsingFailed()
+{
+    if (m_sourceBufferPrivateClient)
+        m_sourceBufferPrivateClient->sourceBufferPrivateAppendComplete(SourceBufferPrivateClient::ParsingFailed);
+}
+
 }
 #endif
index b07c2b0..18f1abd 100644 (file)
@@ -76,6 +76,7 @@ public:
     void didReceiveInitializationSegment(const SourceBufferPrivateClient::InitializationSegment&);
     void didReceiveSample(MediaSample&);
     void didReceiveAllPendingSamples();
+    void appendParsingFailed();
 
     ContentType type() const { return m_type; }
 
index c745a0a..8df4323 100644 (file)
@@ -1,3 +1,24 @@
+2018-12-05  Alicia Boya García  <aboya@igalia.com>
+
+        [MSE][GStreamer] Remove the AppendPipeline state machine
+        https://bugs.webkit.org/show_bug.cgi?id=192204
+
+        Reviewed by Xabier Rodriguez-Calvar.
+
+        Updated AbortableTaskQueue tests:
+
+        Added test: AbortedBySyncTaskHandler.
+
+        Renamed test: AbortDuringSyncTask -> AbortBeforeSyncTaskRun (in
+        order to avoid confusion with the new test).
+
+        Added checks for the correct destruction of response objects.
+
+        * TestWebKitAPI/Tests/WebCore/AbortableTaskQueue.cpp:
+        (TestWebKitAPI::FancyResponse::FancyResponse):
+        (TestWebKitAPI::FancyResponse::~FancyResponse):
+        (TestWebKitAPI::TEST):
+
 2018-12-04  Chris Dumez  <cdumez@apple.com>
 
         ProcessSwap.UseSessionCookiesAfterProcessSwapInPrivateBrowsing API test is failing
index 5df8f5a..5fee1f7 100644 (file)
@@ -65,13 +65,25 @@ TEST(AbortableTaskQueue, AsyncTasks)
 struct FancyResponse {
     WTF_MAKE_NONCOPYABLE(FancyResponse);
 public:
-    FancyResponse(int fancyInt)
+    FancyResponse(int fancyInt, bool* destructedFlagPointer = nullptr)
         : fancyInt(fancyInt)
+        , destructedFlagPointer(destructedFlagPointer)
     { }
 
-    FancyResponse(FancyResponse&& a)
-        : fancyInt(a.fancyInt)
-    { }
+    FancyResponse(FancyResponse&& original)
+        : fancyInt(original.fancyInt)
+        , destructedFlagPointer(original.destructedFlagPointer)
+    {
+        original.destructedFlagPointer = nullptr;
+    }
+
+    ~FancyResponse()
+    {
+        if (destructedFlagPointer) {
+            RELEASE_ASSERT(!*destructedFlagPointer);
+            *destructedFlagPointer = true;
+        }
+    }
 
     FancyResponse& operator=(FancyResponse&& a)
     {
@@ -80,12 +92,14 @@ public:
     }
 
     int fancyInt;
+    bool* destructedFlagPointer;
 };
 
 TEST(AbortableTaskQueue, SyncTasks)
 {
     AbortableTaskQueue taskQueue;
     bool testFinished { false };
+    bool destructedResponseFlag { false };
     int currentStep { 0 };
     RunLoop::initializeMainRunLoop();
 
@@ -95,13 +109,16 @@ TEST(AbortableTaskQueue, SyncTasks)
             EXPECT_TRUE(isMainThread());
             currentStep++;
             EXPECT_EQ(1, currentStep);
-            FancyResponse returnValue(100);
+            FancyResponse returnValue(100, &destructedResponseFlag);
             return returnValue;
         });
         currentStep++;
         EXPECT_EQ(2, currentStep);
         EXPECT_TRUE(response);
+        EXPECT_FALSE(destructedResponseFlag);
         EXPECT_EQ(100, response->fancyInt);
+        response = std::nullopt;
+        EXPECT_TRUE(destructedResponseFlag);
         RunLoop::main().dispatch([&]() {
             testFinished = true;
         });
@@ -219,7 +236,7 @@ TEST(AbortableTaskQueue, Abort)
     Util::run(&testFinished);
 }
 
-TEST(AbortableTaskQueue, AbortDuringSyncTask)
+TEST(AbortableTaskQueue, AbortBeforeSyncTaskRun)
 {
     AbortableTaskQueue taskQueue;
     bool testFinished { false };
@@ -254,4 +271,46 @@ TEST(AbortableTaskQueue, AbortDuringSyncTask)
     Util::run(&testFinished);
 }
 
+TEST(AbortableTaskQueue, AbortedBySyncTaskHandler)
+{
+    AbortableTaskQueue taskQueue;
+    bool testFinished { false };
+    int currentStep { 0 };
+    bool destructedResponseFlag { false };
+    RunLoop::initializeMainRunLoop();
+
+    auto backgroundThreadFunction = [&]() {
+        EXPECT_FALSE(isMainThread());
+        currentStep++;
+        EXPECT_EQ(1, currentStep);
+
+        std::optional<FancyResponse> response = taskQueue.enqueueTaskAndWait<FancyResponse>([&]() -> FancyResponse {
+            currentStep++;
+            EXPECT_EQ(2, currentStep);
+            taskQueue.startAborting();
+            // This task should not have been able to run under the scheduling of this test.
+            return FancyResponse(100, &destructedResponseFlag);
+        });
+
+        currentStep++;
+        EXPECT_EQ(3, currentStep);
+
+        // Main thread has called startAborting().
+        EXPECT_FALSE(response);
+
+        // The response object has not been leaked.
+        EXPECT_TRUE(destructedResponseFlag);
+
+        RunLoop::main().dispatch([&]() {
+            testFinished = true;
+        });
+    };
+    RunLoop::current().dispatch([&, backgroundThreadFunction = WTFMove(backgroundThreadFunction)]() mutable {
+        EXPECT_TRUE(isMainThread());
+        WTF::Thread::create("atq-background", WTFMove(backgroundThreadFunction))->detach();
+    });
+
+    Util::run(&testFinished);
+}
+
 } // namespace TestWebKitAPI