Bug 37003 - timeupdate events stop firing after suspend event fires
Summary: timeupdate events stop firing after suspend event fires
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-04-01 19:01 PDT by Andrew Scherkus
Modified: 2010-04-06 20:54 PDT (History)
3 users (show)

See Also:


Attachments
2010-04-01 Andrew Scherkus <scherkus@chromium.org> (6.62 KB, patch)
2010-04-01 19:08 PDT, Andrew Scherkus
no flags Details | Formatted Diff | Diff
2010-04-02 Andrew Scherkus <scherkus@chromium.org> (7.59 KB, patch)
2010-04-02 00:10 PDT, Andrew Scherkus
no flags Details | Formatted Diff | Diff
2010-04-02 Andrew Scherkus <scherkus@chromium.org> (6.08 KB, patch)
2010-04-02 00:16 PDT, Andrew Scherkus
no flags Details | Formatted Diff | Diff
2010-04-02 Andrew Scherkus <scherkus@chromium.org> (6.36 KB, patch)
2010-04-02 11:58 PDT, Andrew Scherkus
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Scherkus 2010-04-01 19:01:17 PDT
When an <audio> or <video> element returns to NETWORK_IDLE from NETWORK_LOADING (i.e., the user agent has intentionally paused downloading the media), we stop the progress event timer (good!) but also stop the playback event timer (bad!).

This has the unfortunate consequence of stopping timeupdate events even the video is still playing and presumably has enough of an internal to keep playing until the user agent resumes downloading and goes back to NETWORK_LOADING.
Comment 1 Andrew Scherkus 2010-04-01 19:08:39 PDT
Created attachment 52367 [details]
2010-04-01  Andrew Scherkus  <scherkus@chromium.org>
Comment 2 Eric Seidel (no email) 2010-04-01 23:47:47 PDT
Looks like you're using good old post-commits. :)  You might want to try out "webkit-patch upload" it will do slightly smarter things with patch names at least.
Comment 3 Eric Seidel (no email) 2010-04-01 23:48:42 PDT
Comment on attachment 52367 [details]
2010-04-01  Andrew Scherkus  <scherkus@chromium.org>

 63 # WebKit doesn't fire suspend events.
Is a confusing comment in all the Skipped tests.  You shoudl link to this bug or mention the platform name, or indicate if hte platofrm plans to fix that. :)
Comment 4 Andrew Scherkus 2010-04-02 00:10:13 PDT
Created attachment 52387 [details]
2010-04-02  Andrew Scherkus  <scherkus@chromium.org>
Comment 5 Andrew Scherkus 2010-04-02 00:12:54 PDT
Does webkit-patch upload work with git?  It seems I need to have the changes uncomitted, which scares me a little :s
Comment 6 Eric Seidel (no email) 2010-04-02 00:12:55 PDT
Comment on attachment 52387 [details]
2010-04-02  Andrew Scherkus  <scherkus@chromium.org>

Skipping in "mac/" will skip on all mac platforms.  no need to skip for Leopard and Tiger individually.
Comment 7 Eric Seidel (no email) 2010-04-02 00:14:19 PDT
ojan is your man.  He's making webkit-patch upload better.  Just wait another day and it will be more git-friendly.
Comment 8 Andrew Scherkus 2010-04-02 00:16:41 PDT
Created attachment 52391 [details]
2010-04-02  Andrew Scherkus  <scherkus@chromium.org>
Comment 9 Andrew Scherkus 2010-04-02 00:18:26 PDT
(In reply to comment #6)
> (From update of attachment 52387 [details])
> Skipping in "mac/" will skip on all mac platforms.  no need to skip for Leopard
> and Tiger individually.

Perfect!  I thought this might be the case, but not having access to a Tiger machine I wasn't really able to test out the theory.  I noticed there were some duplicated Skipped entries between mac platforms, so I assumed that it might be per-platform.

PS ojan if you're reading this... thank you :)
Comment 10 Eric Carlson 2010-04-02 09:26:49 PDT
Comment on attachment 52391 [details]
2010-04-02  Andrew Scherkus  <scherkus@chromium.org>

> +++ b/LayoutTests/http/tests/media/video-play-suspend.html
> @@ -0,0 +1,26 @@
> +<video></video>
> +<p>Test that timeupdate events are sent when media loading suspends itself.</p>
> +<script src=../../../media/media-file.js></script>
> +<script src=../../../media/video-test.js></script>
> +<script>
> +    var suspendCount = 0;
> +
> +    // Make sure we've at least reached NETWORK_LOADING before waiting for suspend.
> +    waitForEvent('loadstart', function() {
> +        // suspend event means the user agent has intentionally paused network usage,
> +        // however we should still receive timeupdate events.
> +        mediaElement.addEventListener('suspend', function () {
> +            // timeupdate events are fired as playback progresses so only verify that at least one
> +            // event is fired
> +            ++suspendCount;
> +            if (suspendCount == 1) {
> +                consoleWrite("EVENT(suspend)");
> +                waitForEventAndEnd('timeupdate');
> +            }
> +        } );
> +    } );
> +
> +    var mediaFile = findMediaFile("video", "resources/test");
> +    video.src = "http://127.0.0.1:8000/media/" + mediaFile;
> +    run("video.play()");
> +</script>

I really prefer to see tests as whole html documents rather than fragments - include <html> and <body>, put the <script> in a function in <head>, trigger the script on 'load', etc. I think it makes the tests *much* easier to read and understand for someone else down the line.

> diff --git a/LayoutTests/platform/gtk/Skipped b/LayoutTests/platform/gtk/Skipped
> index d086452..4c4bd1f 100644
> --- a/LayoutTests/platform/gtk/Skipped
> +++ b/LayoutTests/platform/gtk/Skipped
> @@ -3424,6 +3424,7 @@ http/tests/local/send-sliced-dragged-file.html
>  http/tests/local/send-form-data.html
>  http/tests/media/video-play-stall-seek.html
>  http/tests/media/video-play-stall.html
> +http/tests/media/video-play-suspend.html
>  http/tests/media/video-seekable-stall.html
>  http/tests/mime/standard-mode-loads-stylesheet-with-empty-content-type.html
>  http/tests/mime/standard-mode-loads-stylesheet-with-text-css-and-invalid-type.html

GTK port doesn't get a comment about why the test is skipped?

> diff --git a/LayoutTests/platform/mac/Skipped b/LayoutTests/platform/mac/Skipped
> index 7a6a7f7..1eb9fc1 100644
> --- a/LayoutTests/platform/mac/Skipped
> +++ b/LayoutTests/platform/mac/Skipped
> @@ -60,6 +60,10 @@ svg/hixie/perf/002.xml
>  # <rdar://problem/6710625> Test media/video-error-abort.html doesn't work
>  http/tests/media/video-error-abort.html
>  
> +# Mac uses QuickTime, which currently doesn't fire a suspend event for audio/video elements.
> +# https://bugs.webkit.org/show_bug.cgi?id=37003
> +http/tests/media/video-play-suspend.html
> +
A bugzilla url in a skipped file should refer to the bug that must be fixed in order to un-skip the test. Please write bugs for each of the ports that don't fire 'suspend' and reference them in the skipped files, or omit the url. Are you certain that the state is not set to 'idle' because of QuickTime, or is it a bug in the WebCore code that uses QuickTime? 


> +# Windows uses QuickTime, which currently doesn't fire a suspend event for audio/video elements.
> +# https://bugs.webkit.org/show_bug.cgi?id=37003
> +http/tests/media/video-play-suspend.html
> +
Ditto.

r=me with these minor changes.
Comment 11 Andrew Scherkus 2010-04-02 11:58:37 PDT
Created attachment 52434 [details]
2010-04-02  Andrew Scherkus  <scherkus@chromium.org>
Comment 12 Andrew Scherkus 2010-04-02 11:59:45 PDT
Thanks for the review Eric!

I've created the following bugs tracking the NETWORK_IDLE/suspend work:
https://bugs.webkit.org/show_bug.cgi?id=37034
https://bugs.webkit.org/show_bug.cgi?id=37035
https://bugs.webkit.org/show_bug.cgi?id=37036
Comment 13 Andrew Scherkus 2010-04-02 14:09:26 PDT
Whoops I uploaded and clobbered the review+

Having addressed Eric's concerns I'll go ahead and land the patch.
Comment 14 Andrew Scherkus 2010-04-02 14:22:59 PDT
Committed as http://trac.webkit.org/changeset/57023
Comment 15 Eric Seidel (no email) 2010-04-06 20:54:30 PDT
Comment on attachment 52434 [details]
2010-04-02  Andrew Scherkus  <scherkus@chromium.org>

Clearing r? since this bug is closed.