Bug 188193 – [GStreamer] Stop pushing buffers when seeking status changes

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Charlie Turner]

[GStreamer] Stop pushing buffers when seeking status changes

[Charlie Turner]

Created attachment 346153 [details]
Patch

[EWS Watchlist]

Comment on attachment 346153 [details]
Patch

Attachment 346153 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8718998

New failing tests:
http/tests/security/local-video-source-from-remote.html

[EWS Watchlist]

Created attachment 346256 [details]
Archive of layout-test-results from ews205 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews205  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Xabier Rodríguez Calvar]

Comment on attachment 346153 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=346153&action=review

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:935
> +        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;
> +        }

I am perfectly ok with this here though I wonder if it would or not be better to have this check at the beginning, even maybe as:

// A seek might happen while we run this pushes so we stop pushing if that happens.
for (uint64_t currentOffset = 0; !priv->isSeeking && currentOffset < bufferSize; currentOffset += blockSize) {

[Xabier Rodríguez Calvar]

(In reply to Xabier Rodríguez Calvar from comment #4)
> // A seek might happen while we run this pushes so we stop pushing if that
> happens.
> for (uint64_t currentOffset = 0; !priv->isSeeking && currentOffset <
> bufferSize; currentOffset += blockSize) {

I guess we'd need to find a new home for the logging about the seeking situation.

[Charlie Turner]

(In reply to Build Bot from comment #2)
> Comment on attachment 346153 [details]
> Patch
> 
> Attachment 346153 [details] did not pass win-ews (win):
> Output: https://webkit-queues.webkit.org/results/8718998
> 
> New failing tests:
> http/tests/security/local-video-source-from-remote.html

In the layout-tests archive attachment, the crash log only contains

  No crash log found for DumpRenderTree:9828.

Is there anywhere I can find more information about this crash?

[Charlie Turner]

Created attachment 346272 [details]
Patch

I moved the check to just before the push_buffer so as to minimize a race window. The design of the web source will need some rework to avoid this completely

[Charlie Turner]

Created attachment 346366 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 346366 [details]
Patch for landing

Clearing flags on attachment: 346366

Committed r234497: <https://trac.webkit.org/changeset/234497>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.