Bug 36165

Summary: [GStreamer] media/video-played-collapse.html
Product: WebKit Reporter: Gustavo Noronha (kov) <gustavo>
Component: MediaAssignee: Philippe Normand <pnormand>
Status: RESOLVED FIXED    
Severity: Normal CC: chinmaya, eric.carlson, mrobinson, pnormand
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Attachments:
Description Flags
Patch
none
test fix proposal
none
test fix proposal
none
Updated patch
none
updated test
none
Patch eric.carlson: review+

Description Gustavo Noronha (kov) 2010-03-16 05:06:20 PDT
Like I said in https://bugs.webkit.org/show_bug.cgi?id=34372, I think fixing the timeout priorities to avoid starvation is exposing a problem in this test. It was failing consistently for me before the patches, but passing on the bots, and now the situation has somewhat reversed, so I think there's some racy behaviour we have to investigate making this test unreliable. I'll skip it.
Comment 1 Maciej Stachowiak 2010-03-17 01:00:24 PDT
Created attachment 50879 [details]
Patch
Comment 2 Maciej Stachowiak 2010-03-17 01:01:07 PDT
Comment on attachment 50879 [details]
Patch

Oops, wrong bug (curse you webkit-patch)
Comment 3 Philippe Normand 2010-03-26 07:03:21 PDT
Created attachment 51737 [details]
test fix proposal
Comment 4 Philippe Normand 2010-03-26 07:15:16 PDT
Comment on attachment 51737 [details]
test fix proposal

Executed alone the test passes fine locally but not if i run the whole media test suite... Will investigate further. Removing review flag for now.
Comment 5 Philippe Normand 2010-03-26 08:31:33 PDT
If media/video-pause-immediately.html is executed before played-collapste.html, the later fails. In the first test a played range is created and the second test fails because there are 3 ranges created instead of 2. If I make the first test not start any playback the video-played-collapse test passes.

So... it looks like the second test reuses the media element of the first one, how is that possible? :( What do you think Gustavo?
Comment 6 Philippe Normand 2010-07-23 07:47:10 PDT
Comment on attachment 51737 [details]
test fix proposal

Marking again for review as I can't reproduce the behavior observed in comment 5 anymore. Can you please test this Gustavo?
Comment 7 Gustavo Noronha (kov) 2010-08-03 11:21:41 PDT
The test fails for me with your patch applied, when running all the media tests =(:

-Test 13 OK
+EXPECTED (video.played.length == '2'), OBSERVED '3' FAIL
 Test 14 OK
-Test 15 OK
-Test 16 OK
-Test 17 OK
+EXPECTED (video.played.end(0).toFixed(2) == '1.30'), OBSERVED '0.28' FAIL
+EXPECTED (video.played.start(1).toFixed(2) == '5.98'), OBSERVED '0.40' FAIL
+EXPECTED (video.played.end(1).toFixed(2) == '6.03'), OBSERVED '1.30' FAIL
Comment 8 Philippe Normand 2010-08-12 05:08:45 PDT
(In reply to comment #7)
> The test fails for me with your patch applied, when running all the media tests =(:

Can you please uncomment the disableFullTestDetailsPrinting() line in the test, run it with DRT and give the results?

Thanks!
Comment 9 Philippe Normand 2010-08-12 07:38:52 PDT
(In reply to comment #7)
> The test fails for me with your patch applied, when running all the media tests =(:
> 
> -Test 13 OK
> +EXPECTED (video.played.length == '2'), OBSERVED '3' FAIL
>  Test 14 OK
> -Test 15 OK
> -Test 16 OK
> -Test 17 OK
> +EXPECTED (video.played.end(0).toFixed(2) == '1.30'), OBSERVED '0.28' FAIL
> +EXPECTED (video.played.start(1).toFixed(2) == '5.98'), OBSERVED '0.40' FAIL
> +EXPECTED (video.played.end(1).toFixed(2) == '6.03'), OBSERVED '1.30' FAIL

Ok I can reproduce that failure on my laptop but not on my other build machine, will have a look ;)
Comment 10 Philippe Normand 2010-08-12 08:48:13 PDT
Created attachment 64228 [details]
test fix proposal

Found out why it failed :) During the last part of the test (looping),
we check that the first 2 timeranges are merged but in some cases the
player had no time to reach past the merge position. Setting the
second range start position earlier in the timeline fixes the issue in
my case. Can you test Gustavo?
Comment 11 Gustavo Noronha (kov) 2010-08-16 06:57:44 PDT
It seems to fail once in a while with this diff (I ran it 8 times and it failed 3 times here):

--- /tmp/layout-test-results/media/video-played-collapse-expected.txt	2010-08-16 10:51:36.000000000 -0300
+++ /tmp/layout-test-results/media/video-played-collapse-actual.txt	2010-08-16 10:51:36.000000000 -0300
@@ -36,10 +36,10 @@
 RUN(video.play())
 RUN(video.pause())
 EVENT(pause)
-Test 13 OK
+EXPECTED (video.played.length == '2'), OBSERVED '3' FAIL
 Test 14 OK
-Test 15 OK
-Test 16 OK
-Test 17 OK
+EXPECTED (video.played.end(0).toFixed(2) == '1.60'), OBSERVED '0.19' FAIL
+EXPECTED (video.played.start(1).toFixed(2) == '5.98'), OBSERVED '0.25' FAIL
+EXPECTED (video.played.end(1).toFixed(2) == '6.03'), OBSERVED '1.60' FAIL
 END OF TEST
 
I also had a GStreamer crash on media/video-layer-crash.html in one of my runs heh. The diff seems to always be mostly the same, though it had "1.56" in the last FAIL the last time I ran it.
Comment 12 Martin Robinson 2010-08-26 07:50:54 PDT
Phillippe is it possible for you to ping Eric Carlson for this bug? He's not the original author, but he'd probably be the best person to review this patch.
Comment 13 Philippe Normand 2010-08-26 23:07:03 PDT
Eric,

Could you test this patch too?
Do you know why the "looping" part of the test was deactivated?
Comment 14 Eric Carlson 2010-09-15 12:46:37 PDT
Created attachment 67701 [details]
Updated patch

(In reply to comment #13)
> 
> Could you test this patch too?
> Do you know why the "looping" part of the test was deactivated?
>
It looks like the problem is that the movie was supposed to play for duration-0.05 seconds, loop, and play at least 0.4 seconds before pausing, but the test was only configured to run for 100ms.

I have changed the test to make the first range start closer to 0 and to calculate the time required to loop and play. Can you if this work for you?
Comment 15 Philippe Normand 2010-11-17 06:47:09 PST
(In reply to comment #14)
> Created an attachment (id=67701) [details]
> Updated patch
> 
> (In reply to comment #13)
> > 
> > Could you test this patch too?
> > Do you know why the "looping" part of the test was deactivated?
> >
> It looks like the problem is that the movie was supposed to play for duration-0.05 seconds, loop, and play at least 0.4 seconds before pausing, but the test was only configured to run for 100ms.
> 
> I have changed the test to make the first range start closer to 0 and to calculate the time required to loop and play. Can you if this work for you?

It fails here:

--- /tmp/layout-test-results/media/video-played-collapse-expected.txt   2010-11-17 15:45:44.000000000 +0100
+++ /tmp/layout-test-results/media/video-played-collapse-actual.txt     2010-11-17 15:45:44.000000000 +0100
@@ -34,12 +34,6 @@
 Test looping
 RUN(video.loop = true)
 RUN(video.play())
-RUN(video.pause())
-EVENT(pause)
-Test 13 OK
-Test 14 OK
-Test 15 OK
-Test 16 OK
-Test 17 OK
+ERROR: test stalled, waited 2.3510000705718994 seconds for movie to play 2.2808964252471924 seconds FAIL
 END OF TEST

Haven't had time yet to debug this.
Comment 16 Philippe Normand 2010-12-10 10:01:29 PST
Created attachment 76214 [details]
updated test

I had some success with this one on GTK. The expected result didn't
change from Eric's version.
Comment 17 Philippe Normand 2012-03-06 01:43:36 PST
Created attachment 130333 [details]
Patch
Comment 18 Philippe Normand 2012-03-06 08:11:26 PST
Committed r109921: <http://trac.webkit.org/changeset/109921>