Bug 135086 – [GTK] Layout Test media/video-seek-with-negative-playback.html timeouts on the release bot.

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Carlos Alberto Lopez Perez]

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.

[Enrique Ocaña]

Created attachment 443561 [details]
Patch

[Xabier Rodríguez Calvar]

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.

[Enrique Ocaña]

(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.

[Alicia Boya García]

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.

[Víctor M. Jáquez L.]

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?

[Enrique Ocaña]

(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.

[Enrique Ocaña]

(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.

[Enrique Ocaña]

...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.

[Enrique Ocaña]

Created attachment 443805 [details]
Patch

[EWS]

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].

[Radar WebKit Bug Importer]

<rdar://problem/85258014>

[Yacine Bandou]

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

[Philippe Normand]

(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 :)

[Philippe Normand]

(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

[Philippe Normand]

media/video-seek-with-negative-playback.html is still very much flaky on WPE bots (pass timeout) while in GTK it looks ok...

[Philippe Normand]

Pull request: https://github.com/WebKit/WebKit/pull/14442

[EWS]

Committed 264646@main (435dd73cccb5): <https://commits.webkit.org/264646@main>

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