Bug 61160 – GTK: media/video-src-invalid-poster.html Failed

• Component: Media
• Status: RESOLVED FIXED

[Jer Noble]

On GTK, the media/video-src-invalid-ponter.html test gives the correct width but the wrong height.

[Jer Noble]

Committed r86911 adding the new test to the GTK skipped list.

[Philippe Normand]

Created attachment 94976 [details]
proposed patch

[Philippe Normand]

(In reply to comment #0)
> On GTK, the media/video-src-invalid-ponter.html test gives the correct width but the wrong height.

Are you sure? What I saw locally is that sometimes the test times out, otherwise it succeeds locally.

Hence the attached patch to avoid the timeout

[Jer Noble]

(In reply to comment #3)
> (In reply to comment #0)
> > On GTK, the media/video-src-invalid-ponter.html test gives the correct width but the wrong height.
> 
> Are you sure? What I saw locally is that sometimes the test times out, otherwise it succeeds locally.

Definitely.  Check the test results from the URL field above.  It shows the video sized to 73 (correct) by 150 (incorrect, but probably the default video element width).

However, if it's not reproducing any longer, then aces. ;)

> Hence the attached patch to avoid the timeout

So the patch seems to significantly change the behavior of the test by calling |video.load()|.  Shouldn't we see the correct behavior without calling load()?  The original test gave the video element a whole second to resize and display the poster.  Is that not enough time?

[Philippe Normand]

(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #0)
> > > On GTK, the media/video-src-invalid-ponter.html test gives the correct width but the wrong height.
> > 
> > Are you sure? What I saw locally is that sometimes the test times out, otherwise it succeeds locally.
> 
> Definitely.  Check the test results from the URL field above.  It shows the video sized to 73 (correct) by 150 (incorrect, but probably the default video element width).
> 

Oh I didn't that link indeed. Yes it's odd. I recall the Release bot might need a GStreamer upgrade to be in par with the Debug bots.
At least locally the test passes but I use a fairly recent GStreamer version.

> However, if it's not reproducing any longer, then aces. ;)
> 
> > Hence the attached patch to avoid the timeout
> 
> So the patch seems to significantly change the behavior of the test by calling |video.load()|.  Shouldn't we see the correct behavior without calling load()?  The original test gave the video element a whole second to resize and display the poster.  Is that not enough time?

On my slow laptop I could see the timeout happen. The GTK bots are know to not be lighning fast either...
In general for media tests (at least) I believe we should react more on events instead of waiting on something to happen before a XXXX ms timeout to fire.

[Jer Noble]

(In reply to comment #5)
> (In reply to comment #4)
> > So the patch seems to significantly change the behavior of the test by calling |video.load()|.  Shouldn't we see the correct behavior without calling load()?  The original test gave the video element a whole second to resize and display the poster.  Is that not enough time?
> 
> On my slow laptop I could see the timeout happen. The GTK bots are know to not be lighning fast either...
> In general for media tests (at least) I believe we should react more on events instead of waiting on something to happen before a XXXX ms timeout to fire.

Okay, I'd be fine with getting rid of the 1s timeout.  What about |video.load()|?  Does the test pass on GTK+ without that, but also without the timeout?

[Philippe Normand]

(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > So the patch seems to significantly change the behavior of the test by calling |video.load()|.  Shouldn't we see the correct behavior without calling load()?  The original test gave the video element a whole second to resize and display the poster.  Is that not enough time?
> > 
> > On my slow laptop I could see the timeout happen. The GTK bots are know to not be lighning fast either...
> > In general for media tests (at least) I believe we should react more on events instead of waiting on something to happen before a XXXX ms timeout to fire.
> 
> Okay, I'd be fine with getting rid of the 1s timeout.  What about |video.load()|?  Does the test pass on GTK+ without that, but also without the timeout?

Doesn't pass because if load() isn't called there's no loadstart event.

[Philippe Normand]

Can this be reviewed please? Eric, would you have some time to look at it?

Thanks!

[Eric Carlson]

Comment on attachment 94976 [details]
proposed patch

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

> LayoutTests/ChangeLog:9
> +        Wait loadstart event to check the poster dimensions and avoid a
> +        potential timeout of the test.

Nit: Wait for loadstart event...

[Philippe Normand]

Committed r90554: <http://trac.webkit.org/changeset/90554>