Bug 37003

Summary: timeupdate events stop firing after suspend event fires
Product: WebKit Reporter: Andrew Scherkus <scherkus>
Component: MediaAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: eric.carlson, eric, ojan
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Attachments:
Description Flags
2010-04-01 Andrew Scherkus <scherkus@chromium.org>
none
2010-04-02 Andrew Scherkus <scherkus@chromium.org>
none
2010-04-02 Andrew Scherkus <scherkus@chromium.org>
none
2010-04-02 Andrew Scherkus <scherkus@chromium.org> none

Andrew Scherkus
Reported 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.
Attachments
2010-04-01 Andrew Scherkus <scherkus@chromium.org> (6.62 KB, patch)
2010-04-01 19:08 PDT, Andrew Scherkus 		no flags
DetailsFormatted DiffDiff
2010-04-02 Andrew Scherkus <scherkus@chromium.org> (7.59 KB, patch)
2010-04-02 00:10 PDT, Andrew Scherkus 		no flags
DetailsFormatted DiffDiff
2010-04-02 Andrew Scherkus <scherkus@chromium.org> (6.08 KB, patch)
2010-04-02 00:16 PDT, Andrew Scherkus 		no flags
DetailsFormatted DiffDiff
2010-04-02 Andrew Scherkus <scherkus@chromium.org> (6.36 KB, patch)
2010-04-02 11:58 PDT, Andrew Scherkus 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Andrew Scherkus
Comment 1 2010-04-01 19:08:39 PDT
Created attachment 52367 [details] 2010-04-01 Andrew Scherkus <scherkus@chromium.org>
Eric Seidel (no email)
Comment 2 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.
Eric Seidel (no email)
Comment 3 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. :)
Andrew Scherkus
Comment 4 2010-04-02 00:10:13 PDT
Created attachment 52387 [details] 2010-04-02 Andrew Scherkus <scherkus@chromium.org>
Andrew Scherkus
Comment 5 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
Eric Seidel (no email)
Comment 6 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.
Eric Seidel (no email)
Comment 7 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.
Andrew Scherkus
Comment 8 2010-04-02 00:16:41 PDT
Created attachment 52391 [details] 2010-04-02 Andrew Scherkus <scherkus@chromium.org>
Andrew Scherkus
Comment 9 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 :)
Eric Carlson
Comment 10 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.
Andrew Scherkus
Comment 11 2010-04-02 11:58:37 PDT
Created attachment 52434 [details] 2010-04-02 Andrew Scherkus <scherkus@chromium.org>
Andrew Scherkus
Comment 12 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
Andrew Scherkus
Comment 13 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.
Andrew Scherkus
Comment 14 2010-04-02 14:22:59 PDT
Committed as http://trac.webkit.org/changeset/57023
Eric Seidel (no email)
Comment 15 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.
Note You need to log in before you can comment on or make changes to this bug.