[GStreamer] Stop pushing buffers when seeking status changes
authorcturner@igalia.com <cturner@igalia.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 2 Aug 2018 09:23:42 +0000 (09:23 +0000)
committercturner@igalia.com <cturner@igalia.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 2 Aug 2018 09:23:42 +0000 (09:23 +0000)
https://bugs.webkit.org/show_bug.cgi?id=188193

Reviewed by Xabier Rodriguez-Calvar.

After switching to splitting buffers into smaller block sizes in

    https://bugs.webkit.org/show_bug.cgi?id=182829

It was found that during the individual buffer pushes, the seeking
status could change behind our backs from another thread. When
this happens, buffers from incorrect offsets would find their way
into appsrc and eventually the demuxer itself, which would start
parsing from a random place and at best give a confusing error
message.

The solution here is break from pushing buffers when the seeking
status has been has changed. Flushes will clear out what we've
already delivered into the appsrc, then we must make sure to not
continue sending buffers in there after the flush.

No new tests since this is a timing bug.

* platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:
(CachedResourceStreamingClient::dataReceived):

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

Source/WebCore/ChangeLog
Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp

index 1b7f4a2..52ea76b 100644 (file)
@@ -1,3 +1,31 @@
+2018-08-02  Charlie Turner  <cturner@igalia.com>
+
+        [GStreamer] Stop pushing buffers when seeking status changes
+        https://bugs.webkit.org/show_bug.cgi?id=188193
+
+        Reviewed by Xabier Rodriguez-Calvar.
+
+        After switching to splitting buffers into smaller block sizes in
+
+            https://bugs.webkit.org/show_bug.cgi?id=182829
+
+        It was found that during the individual buffer pushes, the seeking
+        status could change behind our backs from another thread. When
+        this happens, buffers from incorrect offsets would find their way
+        into appsrc and eventually the demuxer itself, which would start
+        parsing from a random place and at best give a confusing error
+        message.
+
+        The solution here is break from pushing buffers when the seeking
+        status has been has changed. Flushes will clear out what we've
+        already delivered into the appsrc, then we must make sure to not
+        continue sending buffers in there after the flush.
+
+        No new tests since this is a timing bug.
+
+        * platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:
+        (CachedResourceStreamingClient::dataReceived):
+
 2018-08-01  Yusuke Suzuki  <utatane.tea@gmail.com>
 
         Unreviewed, revert TransformationMatrix::operator== change
index a393f14..d20ae55 100644 (file)
@@ -897,26 +897,36 @@ void CachedResourceStreamingClient::dataReceived(PlatformMediaResource&, const c
     // Now split the recv'd buffer into buffers that are of a size basesrc suggests. It is important not
     // to push buffers that are too large, otherwise incorrect buffering messages can be sent from the
     // pipeline.
-    uint64_t bufferSize = gst_buffer_get_size(priv->buffer.get());
-    uint64_t blockSize = static_cast<uint64_t>(GST_BASE_SRC_CAST(priv->appsrc)->blocksize);
-    ASSERT(blockSize);
+    uint64_t bufferSize = static_cast<uint64_t>(length);
+    // FIXME: Implement adaptive block resizing
+    uint64_t blockSize = static_cast<uint64_t>(gst_base_src_get_blocksize(GST_BASE_SRC_CAST(priv->appsrc)));
     GST_LOG_OBJECT(src, "Splitting the received buffer into %" PRIu64 " blocks", bufferSize / blockSize);
     for (uint64_t currentOffset = 0; currentOffset < bufferSize; currentOffset += blockSize) {
         uint64_t subBufferOffset = startingOffset + currentOffset;
         uint64_t currentOffsetSize = std::min(blockSize, bufferSize - currentOffset);
 
-        GST_TRACE_OBJECT(src, "Create sub-buffer from [%" PRIu64 ", %" PRIu64 "]", currentOffset, currentOffset + currentOffsetSize);
         GstBuffer* subBuffer = gst_buffer_copy_region(priv->buffer.get(), GST_BUFFER_COPY_ALL, currentOffset, currentOffsetSize);
         if (UNLIKELY(!subBuffer)) {
             GST_ELEMENT_ERROR(src, CORE, FAILED, ("Failed to allocate sub-buffer"), (nullptr));
             break;
         }
 
+        GST_TRACE_OBJECT(src, "Sub-buffer bounds: %" PRIu64 " -- %" PRIu64, subBufferOffset, subBufferOffset + currentOffsetSize);
         GST_BUFFER_OFFSET(subBuffer) = subBufferOffset;
         GST_BUFFER_OFFSET_END(subBuffer) = subBufferOffset + currentOffsetSize;
-        GST_TRACE_OBJECT(src, "Set sub-buffer offset bounds [%" PRIu64 ", %" PRIu64 "]", GST_BUFFER_OFFSET(subBuffer), GST_BUFFER_OFFSET_END(subBuffer));
 
-        GST_TRACE_OBJECT(src, "Pushing buffer of size %" G_GSIZE_FORMAT " bytes", gst_buffer_get_size(subBuffer));
+        if (priv->isSeeking) {
+            GST_TRACE_OBJECT(src, "Stopping buffer appends due to seek");
+            // A seek has happened in the middle of us breaking the
+            // incoming data up from a previous request. Stop pushing
+            // buffers that are now from the incorrect offset.
+            break;
+        }
+
+        // It may be tempting to use a GstBufferList here, but note
+        // that there is a race condition in GstDownloadBuffer during
+        // seek flushes that can cause decoders to read at incorrect
+        // offsets.
         GstFlowReturn ret = gst_app_src_push_buffer(priv->appsrc, subBuffer);
 
         if (UNLIKELY(ret != GST_FLOW_OK && ret != GST_FLOW_EOS && ret != GST_FLOW_FLUSHING)) {