Attachment 346153 Details for Bug 188193 – Patch

[patch] Patch

bug-188193-20180731131802.patch (text/plain), 4.90 KB, created by Charlie Turner on 2018-07-31 05:18:03 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Charlie Turner

Created: 2018-07-31 05:18:03 PDT

Size: 4.90 KB

patch

obsolete

>Subversion Revision: 234192
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index c3399d860bef2498cc113209d99a225f81a8fbce..9c52e6d3df4d92aea443e8b99d9b9dd55efdbe81 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,31 @@
>+2018-07-31  Charlie Turner  <cturner@igalia.com>
>+
>+        [GStreamer] Stop pushing buffers when seeking status changes
>+        https://bugs.webkit.org/show_bug.cgi?id=188193
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        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-07-24  Chris Dumez  <cdumez@apple.com>
> 
>         REGRESSION (r219757): Accessing response getter of XHR instance from IFRAME sets constructor to Object from the IFRAME
>diff --git a/Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp b/Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp
>index a393f14196b9dc2d16fe9c3decf1e7025cdfe653..17e04fbe5492dbe9fb8aa929b825272bf72cefe6 100644
>--- a/Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp
>+++ b/Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp
>@@ -897,32 +897,42 @@ 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));
>+        // 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)) {
>             GST_ELEMENT_ERROR(src, CORE, FAILED, (nullptr), (nullptr));
>             break;
>         }
>+
>+        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;
>+        }
>     }
> 
>     priv->buffer.clear();

Actions: View | Formatted Diff | Diff

Attachments on bug 188193: 346153 | 346256 | 346272 | 346366