Changeset 234497 in webkit

Timestamp:

Aug 2, 2018 2:23:42 AM (6 years ago)

Author:

cturner@igalia.com

Message:

[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):

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp (modified) (3 diffs)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+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>
 

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

@@ -898 +898 @@
     // 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) {
@@ -906 +906 @@
         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)) {
@@ -913 +912 @@
         }
 
+        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);