Summary: | HTML5 media elements do not fire waiting events correctly | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Albert J. Wong <ajwong> | ||||||||||||||||||
Component: | WebCore Misc. | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||
Severity: | Normal | CC: | eric.carlson, levin | ||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||
Hardware: | PC | ||||||||||||||||||||
OS: | OS X 10.5 | ||||||||||||||||||||
Attachments: |
|
Description
Albert J. Wong
2009-08-14 19:40:04 PDT
> Fixing #1 should be as simple as moving the code block for waiting up above the
> seeking event.
Wrong about this, and possibly about the implementation of #2 as well. Trying to create a patch.
reread the spec some more and looked over the implementation further. The main issue seems to be that HTMLMediaElement doesn't fire waiting before seeking (4.8.10.10 step 8), and doesn't fire seeked if the ready state changes down to HAVE_METADATA during a seek. This patch addresses those issues. I could not setup a test where MediaPlayerQT.mm would lower the readyState, but I did test this change on chromium and it looked right. Patch is attached. Created attachment 34889 [details] waiting/seeked fix the seeked event is missed due when the ready state is changed during a seek. Patch by Albert Wong <ajwong@chromium.org> on 2009-08-15 Reviewed by NOBODY (OOPS!). HTML5 media elements do not fire waiting events correctly https://bugs.webkit.org/show_bug.cgi?id=28335 The video-seeking.html test does not need to be updated because it uses a local file which never fires a waiting/seeking event. To test waiting/seeking, a source that doesn't buffer fully too quickly is required. * html/HTMLMediaElement.cpp: (WebCore::HTMLMediaElement::setReadyState): add support for waiting event when seeking. (WebCore::HTMLMediaElement::finishSeek): send seeked event whenever seeking finishes. (WebCore::HTMLMediaElement::mediaPlayerTimeChanged): dispatch to new function. * html/HTMLMediaElement.h: --- 3 files changed, 50 insertions(+), 12 deletions(-) Created attachment 34890 [details] waiting/seeked fix the seeked event is missed due when the ready state is changed during a seek. Patch by Albert Wong <ajwong@chromium.org> on 2009-08-15 Reviewed by NOBODY (OOPS!). HTML5 media elements do not fire waiting events correctly https://bugs.webkit.org/show_bug.cgi?id=28335 The video-seeking.html test does not need to be updated because it uses a local file which never fires a waiting/seeking event. To test waiting/seeking, a source that doesn't buffer fully too quickly is required. * html/HTMLMediaElement.cpp: (WebCore::HTMLMediaElement::setReadyState): add support for waiting event when seeking. (WebCore::HTMLMediaElement::finishSeek): send seeked event whenever seeking finishes. (WebCore::HTMLMediaElement::mediaPlayerTimeChanged): dispatch to new function. * html/HTMLMediaElement.h: --- 3 files changed, 50 insertions(+), 12 deletions(-) Although it isn't new to your patch, it is possible for a'seeking' event to be fired more than once so: // 4.8.10.10, step 9 if (m_readyState < HAVE_CURRENT_DATA) scheduleEvent(eventNames().seekingEvent); should be something like: // 4.8.10.10, step 9 if (m_readyState < HAVE_CURRENT_DATA) { if (oldState >= HAVE_CURRENT_DATA) scheduleEvent(eventNames().seekingEvent); } > Changes will also need to be made to the the MediaPlayerQT (and likely other
> ports) to support lowering the ready state to HAVE_METADATA on seek.
This should not require any changes as HTMLMediaElement::seek throws an exception unless the seek time is within the seekable() ranges. It is the responsibility of a media engine to report the seekable ranges accurately and to notify HTMLMediaElement of changes to the ready state, so it is an error if a media engine allows seeking to an unbuffered time without also lowering the ready state to HAVE_METADATA. The QuickTime based media engines behave correctly as far as I know.
This change really needs a new seeking test that tests a movie loaded via http. LayoutTests/http/tests/media/ has a cgi used that stalls loads which should be easy to modify to optionally load slowly. Comment on attachment 34890 [details]
waiting/seeked fix
It should be possible to write a layout test for this (or at least a manual test), using a little PHP an the webkit http server. If the test is going to take too long to run, then we should make it a manual test.
(In reply to comment #7) > This change really needs a new seeking test that tests a movie loaded via http. > LayoutTests/http/tests/media/ has a cgi used that stalls loads which should be > easy to modify to optionally load slowly. For the QuickTime based media engine, I'm having a pretty hard time writing up a test. I modified video-load-and-stall.cgi to throttle serving, but the effect that has is that we just end up with a largish unseekable range. What happens is that since the seekable() range is defined maxTimeSeekable(), which, empirically, seems to be equal to maxTimeLoaded(), it does not seem possible to seek to a range where quicktime doesn't have the data cached, and immediately available. Seeking beyond what has been downloaded, like you said earlier, makes the HTMLMediaElement throw a INDEX_SIZE_ERR exception. As far as I can tell, we can't get into a HAVE_METADATA ready state with the QuickTime media player via seeks, thus the seeking/waiting events are never fired. Am I missing some other method to test the behavior of these events? Created attachment 35031 [details]
add Eric's debounce for seeking.
Incorporate Eric's fix to avoid multiple seeking events. Still no unittest though. See previous comment.
Created attachment 35076 [details] waiting/seeking fix...Now with a test\! the seeked event is missed due when the ready state is changed during a seek. Patch by Albert J. Wong <ajwong@chromium.org> on 2009-08-18 Reviewed by NOBODY (OOPS!). HTML5 media elements do not fire waiting events correctly https://bugs.webkit.org/show_bug.cgi?id=28335 Added video-waiting-seeking.html into manual tests because not all platforms allow seeking into non-buffered ranges. * html/HTMLMediaElement.cpp: (WebCore::HTMLMediaElement::setReadyState): add support for waiting event when seeking. (WebCore::HTMLMediaElement::finishSeek): send seeked event whenever seeking finishes. (WebCore::HTMLMediaElement::mediaPlayerTimeChanged): dispatch to new function. * html/HTMLMediaElement.h: * manual-tests/video-waiting-seeking.html: Added. LayoutTests: New manual test to check if waiting/seeking events are fired when seeking into a non-buffered region. Patch by Albert J. Wong <ajwong@chromium.org> on 2009-08-18 Reviewed by NOBODY (OOPS!). HTML5 media elements do not fire waiting events correctly https://bugs.webkit.org/show_bug.cgi?id=28335 * http/tests/media/video-throttled-load.cgi: Added. * media/video-test.js: (isInTimeRanges): --- 2 files changed, 66 insertions(+), 0 deletions(-) Created attachment 35077 [details]
waiting/seeking fix...Now with a test\!
---
5 files changed, 131 insertions(+), 12 deletions(-)
Created attachment 35078 [details]
waiting fix, with tests, in *1* patch!
try again...w/o bugzilla-tool.
Created attachment 35080 [details]
waiting fix with small style cleanups.
> +++ b/LayoutTests/http/tests/media/video-throttled-load.cgi A couple of points about the cgi: 1) it would be nice optionally be able to specify the MIME type so it can be used with other file types, 2) should add a header to prevent the file from being cached in case the same file is used with more than one test, 3) it would also be nice to be able to optionally specify the throttle rate to test different scenarios, 4) the select for the last chunk is likely to pause too long, 5) might be nice to assume that the throttle rate is in KBPS. Something like this maybe (untested): #!/usr/bin/perl -w use CGI; use File::stat; my $query = new CGI; my $name = $query->param('name'); my $filesize = stat($name)->size; # get throttling rate, assume KBPS my $kbPerSec = $query->param('throttle'); if (!$kbPerSec) { $kbPerSec = $filesize / 1024; } $kbPerSec *= 1024; # get MIME type, use video/mp4 if not specified my $type = $query->param('type'); if (!$type) { $type = "video/mp4"; } # print http header print "Content-type: " . $type . "\n"; print "Content-Length: " . $filesize . "\n"; print "Cache-Control: no-cache\n\n"; open FILE, $name or die; binmode FILE; my ($buf, $data, $n); while (($n = read FILE, $data, 1024) != 0) { print $data; if ($kbPerSec > 0) { select(undef, undef, undef, $n / $kbPerSec); } } close(FILE); > +++ b/WebCore/manual-tests/video-waiting-seeking.html I know that this test is in the style of many of the older media test, but I greatly prefer new tests to have a more "trational" style, eg. include <html> & <body> element, put the code in the <head>, trigger the test start on an event (body onload, etc). > + consoleWrite("Running scheduled seek"); > + // Source is assumed to be about 188k with a uniform bitrate, > + // > 4 seconds in length, and served at 25kb/s. This should make > + // seeking one second ahead every 200ms always leave the buffered > + // region. It would be good to have the test assumptions in the markup rather than in a comment so someone looking at the results will see them. > + consoleWrite("Start Load"); > + video.src = "http://127.0.0.1:8000/media/video-throttled-load.cgi?name=../../../media/content/test.mp4&throttle=30&anti_cache=" + (new Date()).getTime(); What is the "anti_cache" param? Created attachment 35156 [details]
Address Eric's comments
I updated the CGI file to add support for mime type, and pass the no-cache header. I also cleaned up the throttling code a bit, but did it slightly different than the suggestion. It seems to work on my box.
The html for the test was cleaned up to be proper html rather than just a fragment.
The comment has been moved into a <p> tag.
The anit-cache param has been removed since the cache-avoidance behavior is handled by the no-cache header.
Comment on attachment 35156 [details]
Address Eric's comments
I had some minor style suggestions for this patch which Albert did and then I r+'ed this patch (though I did a sanity pass, the r+ was in large part due to the fact that Eric Carlson said it looked good).
My previous comment was my attempt to summarize what happened in the past 20 hours but was lost in a bugzilla db corruption. Committed as http://trac.webkit.org/changeset/47619 |