Bug 28335 – HTML5 media elements do not fire waiting events correctly

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Albert J. Wong]

The HTMLMediaElement implementation of SetReadyState makes it impossible to correctly fire waiting, seeking, and seeked events based purely on the changes in ready state.

There are two separate issues:

  (1) 4.8.10.10 step 8 is not done quite right; waiting is fired after seeking.
  (2) The seekedEvent is not guaranteed to fire for eack seekingEvent.

Fixing #1 should be as simple as moving the code block for waiting up above the seeking event.

Fixing #2 required an extra status variable to track when a seekingEvent has started, and thus a seekedEvent should be fired.

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.

[Albert J. Wong]

> 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.

[Albert J. Wong]

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.

[Albert J. Wong]

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 <[e-mail]> 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(-)

[Albert J. Wong]

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 <[e-mail]> 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(-)

[Eric Carlson]

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);
        }

[Eric Carlson]

> 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.

[Eric Carlson]

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.

[Eric Seidel (no email)]

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.

[Albert J. Wong]

(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?

[Albert J. Wong]

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.

[Albert J. Wong]

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 <[e-mail]> 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 <[e-mail]> 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(-)

[Albert J. Wong]

Created attachment 35077 [details]
waiting/seeking fix...Now with a test\!


---
 5 files changed, 131 insertions(+), 12 deletions(-)

[Albert J. Wong]

Created attachment 35078 [details]
waiting fix, with tests, in *1* patch!

try again...w/o bugzilla-tool.

[Albert J. Wong]

Created attachment 35080 [details]
waiting fix with small style cleanups.

[Eric Carlson]

> +++ 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?

[Albert J. Wong]

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.

[David Levin]

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).

[David Levin]

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