Bug 101439 – progress-events-generated-correctly.html expects more progress events than spec

• Component: Tools / Tests
• Status: NEW

[Dominik Röttsches (drott)]

Test introduced in r133660 fails on EFL. See also bug 100984 and bug 100985 for failures on Chromium-linux and mac.

[Jussi Kukkonen (jku)]

I'll take a look

[Jussi Kukkonen (jku)]

MediaPlayerPrivateGstreamer does not update m_mediaDuration as it should (first update happens when playback ends), so progress can't be updated. I'll see what I can do.

[Jussi Kukkonen (jku)]

m_mediaDuration is not updated on GST_STATE_PAUSED but is required in didLoadingProgress(). This would be easy to fix...

Unfortunately that does not make the test pass: the whole file is buffered much earlier than GST_STATE_PAUSED happens (which is the first time we can query duration).

I think I can modify didLoadingProgress() to not depend on media duration though.

[Jussi Kukkonen (jku)]

(In reply to comment #3)
> I think I can modify didLoadingProgress() to not depend on media duration though.

Actually this would not help: We finish loading under the 350ms that is specified as the progress event period. I think a test for this makes sense but currently the test only passes on platforms that load slowly or send more events than they're supposed.

[Jussi Kukkonen (jku)]

For the newly CC'd (people from bug 100378 and philn for gstreamer know-how)

I was trying to find out why media/progress-events-generated-correctly.html fails with gstreamer and came to the conclusion that the test is wrong. 

The test expects more than one progress event. Spec says there should be a progress event every 350 ms. On my laptop using a local web server we finish loading the media file used in the test in ~250ms: emitting only one progress event seems totally valid.

I think testing this makes sense but currently it seems broken. Suggestions on how to fix it?

[John Griggs]

When I wrote this test, I looked for a longer media resource in the existing test cases, but could not find one.  Replacing the media resource with one that takes longer than 350 ms to load on all platforms would fix the test.  I am fairly new to submitting to WebKit and am not sure what the procedure is for adding media resources to the test set - if someone can enlighten me, I will find a better resource, add it and update the test.

[Jussi Kukkonen (jku)]

(In reply to comment #6)
> Replacing the media resource with one that takes longer than 350 ms to load on 
> all platforms would fix the test.  I am fairly new to submitting to WebKit and 
> am not sure what the procedure is for adding media resources to the test set - 
> if someone can enlighten me, I will find a better resource, add it and update 
> the test.

As long as the license is good, I don't think there's anything special to adding media files. However, to really make this resistant to flakiness (remembering that computers get faster all the time and that some build machines might be pretty beefy machines compared to my laptop) we'd possibly have to increase the file size by a lot: a web server on a local machine can probably have a pretty decent throughput.

The test could be made to run in fixed time even if the video is longer but it would definitely increase the repo size, we should avoid that if we can...

[Philippe Normand]

I haven't looked at the test but would it be acceptable to generate a large enough video file by concatenating N times the same small video over and over? That file wouldn't be stored in SVN of course.

[Eric Carlson]

(In reply to comment #8)
> I haven't looked at the test but would it be acceptable to generate a large enough video file by concatenating N times the same small video over and over? That file wouldn't be stored in SVN of course.

We don't need a longer media file, the test can be moved to LayoutTests/http/tests/media/ and changed to use video-throttled-load.cgi to make the load take long enough to generate more than one 'progress' event.

[Jussi Kukkonen (jku)]

(In reply to comment #9)
> (In reply to comment #8)
> > I haven't looked at the test but would it be acceptable to generate a large enough video file by concatenating N times the same small video over and over? That file wouldn't be stored in SVN of course.
> 
> We don't need a longer media file, the test can be moved to LayoutTests/http/tests/media/ and changed to use video-throttled-load.cgi to make the load take long enough to generate more than one 'progress' event.

I was hoping CCing you would results in a comment like this, thanks :)

I'll do this.

[Jussi Kukkonen (jku)]

Created attachment 173623 [details]
Patch

[Jussi Kukkonen (jku)]

(In reply to comment #11)
> Created an attachment (id=173623) [details]
> Patch

Tested on efl. Also removed the failure expectation for mac and chromium on the assumption that it's the same root cause: EWS should confirm. I did not remove the GTK+ exception as it says the test times out (and I didn't have a gtk build and their EWS does not run tests).

[Jussi Kukkonen (jku)]

*** Bug 100984 has been marked as a duplicate of this bug. ***

[Jussi Kukkonen (jku)]

*** Bug 100985 has been marked as a duplicate of this bug. ***

[Eric Carlson]

Comment on attachment 173623 [details]
Patch

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

> LayoutTests/ChangeLog:13
> +        * http/tests/media/progress-events-generated-correctly-expected.txt: Renamed from LayoutTests/media/progress-events-generated-correctly-expected.txt.
> +        * http/tests/media/progress-events-generated-correctly.html: Renamed from LayoutTests/media/progress-events-generated-correctly.html.

Minor nit: Breaking these lines would make it easier to read in editors that aren't configured to wrap.

> LayoutTests/http/tests/media/progress-events-generated-correctly.html:57
> +        // 188 kB at 300kB/s means 470 ms load time: enough for a
> +        // progress event at specified 350ms period.
> +        var rate = 400;
> +        var movie = findMediaFile("video", "resources/test");
> +        var type = mimeTypeForExtension(movie.split('.').pop());
> +        videoElement.src = "http://127.0.0.1:8000/media/video-throttled-load.cgi?name=" + movie + "&throttle=" + rate + "&type=" + type;

470 ms is a long time for a single layout test. Because the purpose of the test is to verify that more than one 'progress' event is fired while loading, why don't you change the test to end once 2 or 3 'progress' events have been fired?

[Jussi Kukkonen (jku)]

(In reply to comment #15)
> 470 ms is a long time for a single layout test. Because the purpose of the test is to verify that more than one 'progress' event is fired while loading, why don't you change the test to end once 2 or 3 'progress' events have been fired?

350 ms is the event sending period by spec so if we want to test that, we always have to wait at least 350 ms. That said I could modify the test to end on first progress event (as long as I made sure the 'extra' progress we send before 'ended' event would not qualify): this would normally save the 120ms that I left there as buffer now.

[Eric Carlson]

(In reply to comment #16)
> (In reply to comment #15)
> > 470 ms is a long time for a single layout test. Because the purpose of the test is to verify that more than one 'progress' event is fired while loading, why don't you change the test to end once 2 or 3 'progress' events have been fired?
> 
> 350 ms is the event sending period by spec so if we want to test that, we always have to wait at least 350 ms. That said I could modify the test to end on first progress event (as long as I made sure the 'extra' progress we send before 'ended' event would not qualify): this would normally save the 120ms that I left there as buffer now.

Actually it will save considerably more than 120 ms, as written the test doesn't end until the ~8 second long file finishes playing.

[WebKit Review Bot]

Comment on attachment 173623 [details]
Patch

Attachment 173623 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14818330

New failing tests:
http/tests/media/progress-events-generated-correctly.html

[Jussi Kukkonen (jku)]

Created attachment 173885 [details]
Patch

[Jussi Kukkonen (jku)]

(In reply to comment #17)
> Actually it will save considerably more than 120 ms, as written the test doesn't end until the ~8 second long file finishes playing.

Oops, you are right. Modifed the test to end on either the first progress event or a 600 ms timeout. Fixed the changelog lines and improved the comment, also fixed gtk TestExpectation to point to the new test location.

[Eric Carlson]

Comment on attachment 173885 [details]
Patch

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

> LayoutTests/http/tests/media/progress-events-generated-correctly.html:22
> +        function progressListener(event)
> +        {
> +            consoleWrite("EVENT(progress)");
> +            endTest();
> +        }

I think it would be better to require two 'progress' events, as the test originally failed because most ports only fire one event when a local file is loaded.

> LayoutTests/http/tests/media/progress-events-generated-correctly.html:37
> +        // slow the download down to make sure we get the progress event at 350ms (±200ms).

Nit: please capitalize, a comment should be a proper sentence.

> LayoutTests/http/tests/media/progress-events-generated-correctly.html:43
> +        // end test with a timeout so A) test fails faster and B) the 'extra' progress

Ditto.

[Jussi Kukkonen (jku)]

(In reply to comment #21)
> > LayoutTests/http/tests/media/progress-events-generated-correctly.html:22
> > +        function progressListener(event)
> > +        {
> > +            consoleWrite("EVENT(progress)");
> > +            endTest();
> > +        }
> I think it would be better to require two 'progress' events, as the test originally failed because most ports only fire one event when a local file is loaded.

Since the download is now throttled I don't see how the loading could possibly finish before the test fails because of the 600 ms timeout.

[Build Bot]

Comment on attachment 173885 [details]
Patch

Attachment 173885 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14822457

New failing tests:
http/tests/media/progress-events-generated-correctly.html

[Jussi Kukkonen (jku)]

(In reply to comment #23)
> Attachment 173885 [details] did not pass mac-ews (mac):
> Output: http://queues.webkit.org/results/14822457
> 
> New failing tests:
> http/tests/media/progress-events-generated-correctly.html

This I really don't understand... I wonder if there are circumstances where it would take more than 600ms for the first event to arrive? Maybe the 250ms buffer is not enough?

For reference, I can't reproduce this problem even after extensive testing. Additional debug prints show the progress event happens reliably at ~360ms from script start.

[Jussi Kukkonen (jku)]

Interesting. on a hunch I started experimenting with lower throttling rates. On my machine I get this:
 * if rate>=70kbps, the first event is always received at ~360ms
 * if rate<70kbs, the first event is always received at ~711ms

It seems if the rate is low enough the first event does not happen, but the second one does. Possibly the mac-ews had other things to do and was slow enough to get this same behaviour.

This looks like a real bug to me.

[Anders Carlsson]

Comment on attachment 173885 [details]
Patch

Clearing review flag on patches from before 2014. If this patch is still relevant, please reset the r? flag.