Bug 61160 - GTK: media/video-src-invalid-poster.html Failed
Summary: GTK: media/video-src-invalid-poster.html Failed
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL: http://build.webkit.org/results/GTK%2...
Keywords:
Depends on:
Blocks:
 
Reported: 2011-05-19 17:57 PDT by Jer Noble
Modified: 2011-07-07 04:18 PDT (History)
2 users (show)

See Also:


Attachments
proposed patch (2.76 KB, patch)
2011-05-26 08:18 PDT, Philippe Normand
eric.carlson: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jer Noble 2011-05-19 17:57:03 PDT
On GTK, the media/video-src-invalid-ponter.html test gives the correct width but the wrong height.
Comment 1 Jer Noble 2011-05-19 18:03:43 PDT
Committed r86911 adding the new test to the GTK skipped list.
Comment 2 Philippe Normand 2011-05-26 08:18:53 PDT
Created attachment 94976 [details]
proposed patch
Comment 3 Philippe Normand 2011-05-26 08:20:17 PDT
(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
Comment 4 Jer Noble 2011-05-26 09:39:43 PDT
(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?
Comment 5 Philippe Normand 2011-05-26 09:45:42 PDT
(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.
Comment 6 Jer Noble 2011-05-26 09:56:03 PDT
(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?
Comment 7 Philippe Normand 2011-06-03 00:45:00 PDT
(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.
Comment 8 Philippe Normand 2011-07-06 07:27:50 PDT
Can this be reviewed please? Eric, would you have some time to look at it?

Thanks!
Comment 9 Eric Carlson 2011-07-06 08:17:42 PDT
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...
Comment 10 Philippe Normand 2011-07-07 04:18:55 PDT
Committed r90554: <http://trac.webkit.org/changeset/90554>