Bug 206234 – [GStreamer] Several buffering fixes

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Thibault Saunier]

[GStreamer] Several buffering fixes

[Thibault Saunier]

Created attachment 387656 [details]
Patch

[Xabier Rodríguez Calvar]

Comment on attachment 387656 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:2308
> +    GST_TRACE_OBJECT(pipeline(), "did download finish %s", boolForPrinting(m_didDownloadFinish));

did download finish?

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:2593
> +                    gst_query_parse_buffering_percent(query.get(), (gboolean*) &m_isBuffering, NULL);

Casting a bool to gboolean is a bad idea, please proxy it thru a gboolean isBuffering.

NULL -> nullptr

[Charlie Turner]

Comment on attachment 387656 [details]
Patch

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

Looking forward to apply this locally and fixing lots of buffering bugs :-)

> Source/WebCore/ChangeLog:25
> +        * platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp: Download as fast as possible when

My review window is not showing me any changes in WKWS, is this up-to-date?

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:2303
> +    auto wasBuffering = m_isBuffering;

auto is not needed here, please use bool

[Thibault Saunier]

Created attachment 387782 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 387782 [details]
Patch for landing

Rejecting attachment 387782 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 387782, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in Source/WebCore/ChangeLog contains OOPS!.

Full output: https://webkit-queues.webkit.org/results/13304751

[Thibault Saunier]

Created attachment 387785 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 387785 [details]
Patch for landing

Clearing flags on attachment: 387785

Committed r254565: <https://trac.webkit.org/changeset/254565>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/58604046>

[Carlos Alberto Lopez Perez]

(In reply to WebKit Commit Bot from comment #7)
> Comment on attachment 387785 [details]
> Patch for landing
> 
> Clearing flags on attachment: 387785
> 
> Committed r254565: <https://trac.webkit.org/changeset/254565>

This has caused lot of timeouts on the GTK port. The bot its aborting early.
I double-checked it locally, reverting this patch makes the timeout go away.
At least 39 new timeouts, and likely more. I didn't ran the whole layout test suite.

Regressions: Unexpected timeouts (39)
  compositing/geometry/clipped-video-controller.html [ Timeout ]
  compositing/geometry/video-fixed-scrolling.html [ Timeout ]
  compositing/geometry/video-opacity-overlay.html [ Timeout ]
  compositing/layers-inside-overflow-scroll.html [ Timeout ]
  compositing/overflow/overflow-compositing-descendant.html [ Timeout ]
  compositing/overflow/scroll-ancestor-update.html [ Timeout ]
  compositing/reflections/load-video-in-reflection.html [ Timeout ]
  compositing/self-painting-layers.html [ Timeout ]
  compositing/shared-backing/clipping-and-shared-backing.html [ Timeout ]
  compositing/video-page-visibility.html [ Timeout ]
  compositing/video/poster.html [ Timeout ]
  compositing/video/video-background-color.html [ Timeout ]
  compositing/video/video-border-radius-clipping.html [ Timeout ]
  compositing/video/video-border-radius.html [ Timeout ]
  compositing/video/video-clip-change-src.html [ Timeout ]
  compositing/video/video-object-fit.html [ Timeout ]
  compositing/video/video-object-position.html [ Timeout ]
  compositing/video/video-poster.html [ Timeout ]
  compositing/video/video-reflection.html [ Timeout ]
  fullscreen/full-screen-iframe-legacy.html [ Timeout ]
  fullscreen/video-controls-timeline.html [ Timeout ]
  legacy-animation-engine/compositing/reflections/load-video-in-reflection.html [ Timeout ]
  media/W3C/audio/events/event_canplaythrough.html [ Timeout ]
  media/W3C/audio/events/event_canplaythrough_manual.html [ Timeout ]
  media/W3C/audio/events/event_order_canplay_canplaythrough.html [ Timeout ]
  media/W3C/audio/paused/paused_false_during_play.html [ Timeout ]
  media/W3C/audio/readyState/readyState_during_canplaythrough.html [ Timeout ]
  media/W3C/audio/readyState/readyState_during_playing.html [ Timeout ]
  media/W3C/video/events/event_canplaythrough.html [ Timeout ]
  media/W3C/video/events/event_order_canplay_canplaythrough.html [ Timeout ]
  media/W3C/video/networkState/networkState_during_progress.html [ Timeout ]
  media/W3C/video/paused/paused_false_during_play.html [ Timeout ]
  media/W3C/video/readyState/readyState_during_canplaythrough.html [ Timeout ]
  media/W3C/video/readyState/readyState_during_playing.html [ Timeout ]
  media/accessiblity-describes-video.html [ Timeout ]
  media/audio-background-playback-playlist.html [ Timeout ]
  media/media-fragments/TC0001.html [ Timeout ]
  media/media-fragments/TC0002.html [ Timeout ]
  media/track/audio/audio-track-mkv-vorbis-addtrack.html [ Timeout ]



See: https://build.webkit.org/builders/GTK%20Linux%2064-bit%20Release%20%28Tests%29/builds/12379


Unless you have a quick fix I think we should revert this patch, because the GTK test bot it is in really bad state.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 206331

[Thibault Saunier]

Created attachment 387912 [details]
Patch

[Thibault Saunier]

(In reply to Carlos Alberto Lopez Perez from comment #10)
> (In reply to WebKit Commit Bot from comment #7)
> > Comment on attachment 387785 [details]
> > Patch for landing
> > 
> > Clearing flags on attachment: 387785
> > 
> > Committed r254565: <https://trac.webkit.org/changeset/254565>
> 
> This has caused lot of timeouts on the GTK port. The bot its aborting early.
> I double-checked it locally, reverting this patch makes the timeout go away.
> At least 39 new timeouts, and likely more. I didn't ran the whole layout
> test suite.
> 
> Regressions: Unexpected timeouts (39)
>   compositing/geometry/clipped-video-controller.html [ Timeout ]
>   compositing/geometry/video-fixed-scrolling.html [ Timeout ]
>   compositing/geometry/video-opacity-overlay.html [ Timeout ]
>   compositing/layers-inside-overflow-scroll.html [ Timeout ]
>   compositing/overflow/overflow-compositing-descendant.html [ Timeout ]
>   compositing/overflow/scroll-ancestor-update.html [ Timeout ]
>   compositing/reflections/load-video-in-reflection.html [ Timeout ]
>   compositing/self-painting-layers.html [ Timeout ]
>   compositing/shared-backing/clipping-and-shared-backing.html [ Timeout ]
>   compositing/video-page-visibility.html [ Timeout ]
>   compositing/video/poster.html [ Timeout ]
>   compositing/video/video-background-color.html [ Timeout ]
>   compositing/video/video-border-radius-clipping.html [ Timeout ]
>   compositing/video/video-border-radius.html [ Timeout ]
>   compositing/video/video-clip-change-src.html [ Timeout ]
>   compositing/video/video-object-fit.html [ Timeout ]
>   compositing/video/video-object-position.html [ Timeout ]
>   compositing/video/video-poster.html [ Timeout ]
>   compositing/video/video-reflection.html [ Timeout ]
>   fullscreen/full-screen-iframe-legacy.html [ Timeout ]
>   fullscreen/video-controls-timeline.html [ Timeout ]
>  
> legacy-animation-engine/compositing/reflections/load-video-in-reflection.
> html [ Timeout ]
>   media/W3C/audio/events/event_canplaythrough.html [ Timeout ]
>   media/W3C/audio/events/event_canplaythrough_manual.html [ Timeout ]
>   media/W3C/audio/events/event_order_canplay_canplaythrough.html [ Timeout ]
>   media/W3C/audio/paused/paused_false_during_play.html [ Timeout ]
>   media/W3C/audio/readyState/readyState_during_canplaythrough.html [ Timeout
> ]
>   media/W3C/audio/readyState/readyState_during_playing.html [ Timeout ]
>   media/W3C/video/events/event_canplaythrough.html [ Timeout ]
>   media/W3C/video/events/event_order_canplay_canplaythrough.html [ Timeout ]
>   media/W3C/video/networkState/networkState_during_progress.html [ Timeout ]
>   media/W3C/video/paused/paused_false_during_play.html [ Timeout ]
>   media/W3C/video/readyState/readyState_during_canplaythrough.html [ Timeout
> ]
>   media/W3C/video/readyState/readyState_during_playing.html [ Timeout ]
>   media/accessiblity-describes-video.html [ Timeout ]
>   media/audio-background-playback-playlist.html [ Timeout ]
>   media/media-fragments/TC0001.html [ Timeout ]
>   media/media-fragments/TC0002.html [ Timeout ]
>   media/track/audio/audio-track-mkv-vorbis-addtrack.html [ Timeout ]
> 
> 
> 
> See:
> https://build.webkit.org/builders/GTK%20Linux%2064-
> bit%20Release%20%28Tests%29/builds/12379
> 
> 
> Unless you have a quick fix I think we should revert this patch, because the
> GTK test bot it is in really bad state.

The problem was stupid simple! I was basically saying that fillTimerFired should not fire an Buffering update if the pipeline didn't answer in DOWNLOAD mode, but this leaded to the pipeline never considering downloadDone for local files. I think further refactoring to avoid that codepath all together could be done, but I added that check mainly because we where querying bufferring on the source instead of the pipelines where we could get result from random elements in the pipeline and where getting results sometimes in DOWNLOAD and sometime in STREAM modes, now that we query the pipeline we can keep updating buffering from the fillTimer callback I think.

No regression timeout with the new version of the patch.

[Diego Pino]

*** Bug 206338 has been marked as a duplicate of this bug. ***

[WebKit Commit Bot]

Comment on attachment 387912 [details]
Patch

Clearing flags on attachment: 387912

Committed r254684: <https://trac.webkit.org/changeset/254684>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.