Bug 135086

Summary: [GTK] Layout Test media/video-seek-with-negative-playback.html timeouts on the release bot.
Product: WebKit Reporter: Carlos Alberto Lopez Perez <clopez>
Component: Tools / TestsAssignee: Philippe Normand <philn>
Status: RESOLVED FIXED    
Severity: Normal CC: aboya, bandou.yacine, bugs-noreply, calvaris, cgarcia, eocanha, eric.carlson, ews-watchlist, glenn, gustavo, jer.noble, menard, philipj, philn, pnormand, sergio, vjaquez, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

Description Carlos Alberto Lopez Perez 2014-07-19 07:52:47 PDT
The layout test media/video-seek-with-negative-playback.html timeouts on the GTK Release bot.

I have debugging the issue, but I still have no clear idea of what is happening.

I'm not able to reproduce the timeout on my laptop.

On the bot, the test was passing fine until r171023. I manually started a forced-build on that revision to double-check this: http://build.webkit.org/builders/GTK%20Linux%2064-bit%20Release%20%28Tests%29/builds/1522

After r171023 the test is always timing out on the bot. r171024 touches 2 IPC related functions on Source/WebKit2/Shared/WebProcessCreationParameters.cpp

However I tried to manually build r171023 on the bot (with a shell account) and the test still timeouts, so the issue is hard to reproduce.

I was doing some traces and it seems that it gets stuck for ~20/30 seconds waiting for dbus. So seems something IPC related.

Perhaps the problem is related to some configuration of the release bot, don't know.

I will be marking the test as flaky.
Comment 1 Enrique Ocaña 2021-11-08 09:55:12 PST
Created attachment 443561 [details]
Patch
Comment 2 Xabier Rodríguez Calvar 2021-11-09 02:57:42 PST
Comment on attachment 443561 [details]
Patch

This code is very complex and serves a very concrete purpose. I would add a complete comment just before it.
Comment 3 Enrique Ocaña 2021-11-09 05:54:22 PST
(In reply to Xabier Rodríguez Calvar from comment #2)
> Comment on attachment 443561 [details]
> Patch
> 
> This code is very complex and serves a very concrete purpose. I would add a
> complete comment just before it.

If you talk about the code to get the position, it's the same already used in playbackPosition(). If any, I could factorize it into a common method called from both places.
Comment 4 Alicia Boya García 2021-11-09 06:00:13 PST
Comment on attachment 443561 [details]
Patch

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

Very good catch. It's unfortunate but we have to deal with it.

I have a suggestion. While the code that checks for position and duration seems like a reasonable way to check if indeed we're not EOS anymore by the time we get the message, we might as well check if the sinks have the EOS flag with GST_PAD_IS_EOS().

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1765
> +    case GST_MESSAGE_EOS: {

The ChangeLog includes the reasoning for this code. You should include it here in a comment!
Otherwise, a casual reader would be left wondering why this code is necessary.
Comment 5 Víctor M. Jáquez L. 2021-11-09 07:10:35 PST
I wonder if this gstreamer behavior is common to many elements, or just a couple elements are behaving badly. If it's the later, imo, it would be better to fix those elements. Did you asses something in that direction?
Comment 6 Enrique Ocaña 2021-11-09 08:43:55 PST
(In reply to Víctor M. Jáquez L. from comment #5)
> I wonder if this gstreamer behavior is common to many elements, or just a
> couple elements are behaving badly. If it's the later, imo, it would be
> better to fix those elements. Did you asses something in that direction?

This behaviour has nothing to do with the elements, but with the way GstBus works.

If we were talking about an EOS GstEvent, then I would agree that the flushes caused by the seek should clear the event. However, this is an EOS GstMessage posted to the bus (at its due time). Once it has been posted, it remains there until the bus message handler in the main thread processes it. Whatever happens to the pipeline after the message reaches the bus is irrelevant to the life cycle of the message.
Comment 7 Enrique Ocaña 2021-11-09 11:10:21 PST
(In reply to Alicia Boya García from comment #4)

> I have a suggestion. While the code that checks for position and duration
> seems like a reasonable way to check if indeed we're not EOS anymore by the
> time we get the message, we might as well check if the sinks have the EOS
> flag with GST_PAD_IS_EOS().

I have checked this and it works for regular videos, but not for live videos. I go a regression on http/tests/media/video-no-content-length-stall.html, like I experienced before adding the duration.isFinite() check when checking if the playback position was in the playable ranges. I'll have to keep the duration.isFinite() condition around.
Comment 8 Enrique Ocaña 2021-11-09 14:29:03 PST
...but the GST_PAD_EOS() solution causes 22 new regressions, so it's definitely not a good alternative.

I'll just resubmit the original patch with a refactoring to avoid duplication of the code that gets the playback position from the GStreamer pipeline, plus the extra comment requested in the review.
Comment 9 Enrique Ocaña 2021-11-10 05:42:36 PST
Created attachment 443805 [details]
Patch
Comment 10 EWS 2021-11-10 09:42:44 PST
Committed r285586 (244094@main): <https://commits.webkit.org/244094@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 443805 [details].
Comment 11 Radar WebKit Bug Importer 2021-11-10 09:43:23 PST
<rdar://problem/85258014>
Comment 12 Yacine Bandou 2022-04-13 08:25:30 PDT
This patch causes a regression, now we ignore the EOS message at the end of the playback of some videos like this one:
https://www.tv5mondeplus.com/bumpers/tv5_bumper.mp4
Comment 13 Philippe Normand 2023-05-28 02:39:02 PDT
(In reply to Yacine Bandou from comment #12)
> This patch causes a regression, now we ignore the EOS message at the end of
> the playback of some videos like this one:
> https://www.tv5mondeplus.com/bumpers/tv5_bumper.mp4

This plays and ends just fine here (didEnd() is called). If this regression is still happening please open a new bug :)
Comment 14 Philippe Normand 2023-05-28 03:06:45 PDT
(In reply to Philippe Normand from comment #13)
> (In reply to Yacine Bandou from comment #12)
> > This patch causes a regression, now we ignore the EOS message at the end of
> > the playback of some videos like this one:
> > https://www.tv5mondeplus.com/bumpers/tv5_bumper.mp4
> 
> This plays and ends just fine here (didEnd() is called). If this regression
> is still happening please open a new bug :)

My bad, that was fixed in bug 239387
Comment 15 Philippe Normand 2023-05-28 04:19:16 PDT
media/video-seek-with-negative-playback.html is still very much flaky on WPE bots (pass timeout) while in GTK it looks ok...
Comment 16 Philippe Normand 2023-05-28 08:43:00 PDT
Pull request: https://github.com/WebKit/WebKit/pull/14442
Comment 17 EWS 2023-05-29 00:47:35 PDT
Committed 264646@main (435dd73cccb5): <https://commits.webkit.org/264646@main>

Reviewed commits have been landed. Closing PR #14442 and removing active labels.