Bug 188193 - [GStreamer] Stop pushing buffers when seeking status changes
Summary: [GStreamer] Stop pushing buffers when seeking status changes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Charlie Turner
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-07-31 05:14 PDT by Charlie Turner
Modified: 2018-08-02 02:23 PDT (History)
4 users (show)

See Also:


Attachments
Patch (4.90 KB, patch)
2018-07-31 05:18 PDT, Charlie Turner
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews205 for win-future (13.05 MB, application/zip)
2018-07-31 21:04 PDT, EWS Watchlist
no flags Details
Patch (4.77 KB, patch)
2018-08-01 05:02 PDT, Charlie Turner
no flags Details | Formatted Diff | Diff
Patch for landing (4.78 KB, patch)
2018-08-02 01:45 PDT, Charlie Turner
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Charlie Turner 2018-07-31 05:14:36 PDT
[GStreamer] Stop pushing buffers when seeking status changes
Comment 1 Charlie Turner 2018-07-31 05:18:03 PDT
Created attachment 346153 [details]
Patch
Comment 2 EWS Watchlist 2018-07-31 21:03:54 PDT
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
Comment 3 EWS Watchlist 2018-07-31 21:04:06 PDT
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
Comment 4 Xabier Rodríguez Calvar 2018-07-31 23:37:09 PDT
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) {
Comment 5 Xabier Rodríguez Calvar 2018-07-31 23:38:14 PDT
(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.
Comment 6 Charlie Turner 2018-08-01 02:13:30 PDT
(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?
Comment 7 Charlie Turner 2018-08-01 05:02:04 PDT
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
Comment 8 Charlie Turner 2018-08-02 01:45:39 PDT
Created attachment 346366 [details]
Patch for landing
Comment 9 WebKit Commit Bot 2018-08-02 02:23:49 PDT
Comment on attachment 346366 [details]
Patch for landing

Clearing flags on attachment: 346366

Committed r234497: <https://trac.webkit.org/changeset/234497>
Comment 10 WebKit Commit Bot 2018-08-02 02:23:50 PDT
All reviewed patches have been landed.  Closing bug.