Bug 64731 – Add MediaSource API to HTMLMediaElement

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Aaron Colwell]

Adding an experimental API for allowing JavaScript to stream media data to an HTMLMediaElement. This functionality will be hidden behind the ENABLE_MEDIA_SOURCE feature define.

This change adds a 2 methods, 2 attributes, and 1 event to the HTMLMediaElement.

webkitMediaSourceUrl - This attribute is passed to the src attribute when JavaScript wants to use the MediaSource API. An HTMLMediaElement will only accept a media source URL from itself. It will not accept a URL from a different element.

webkitSourceAppend() - This method allows one to stream media data to the HTMLMediaElement. This data is expected to be in a suitable streaming container like WebM or Ogg.

webkitSourceEndOfStream(status) - This method signals that the end of stream has been reached. The status parameter indicates what caused the end of stream.
  EOS_NO_ERROR - The stream ended normally without any error.
  EOS_NETWORK_ERROR - The stream ended because a network error was encountered.
  EOS_DECODE_ERROR - The stream ended because a decoding error was encountered while processing the stream.

webkitSourceState - This attribute is similar to the networkState & readyState attributes. It simply reflects the state of the media source. The source can be closed, open, or ended. 
  SOURCE_CLOSED - Indicates that the media source is closed and media data cannot be sent to it.
  SOURCE_OPEN - Indicates that the media source is open and ready to accept data via webkitSourceAppend().
  SOURCE_ENDED - Indicates that the media source is open, but the end of the stream has been reached. Transition to this state can be triggered by the browser or a call to webkitSourceEndOfStream().

open - This event is fired when the media source is ready to receive data. It is triggered when the browser transitions webkitSourceState from SOURCE_CLOSED to SOURCE_OPEN.

JavaScript handles seeks by attaching to the seeking event and appending data for the new seek point after that event fires. The media source implementation is expected to have flushed all pre-seek media data before the seeking event fires. When seeking fires the currentTime attribute contains the desired seek point so JavaScript can use that to figure out what data to fetch. The media source implementation will use the timestamps on the first data passed in after the seeking event fires to determine the actual point seeked to.

[Aaron Colwell]

Here are links to some discussions I've been having about this on the WHATWG list.

http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2011-June/032277.html
http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2011-July/032283.html

http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2011-July/032384.html

The implementation proposed in this bug is a simplified version of what was proposed on the list. What I have here constitutes the minimal changes needed to get something working.

[Aaron Colwell]

Created attachment 101173 [details]
Patch

[Early Warning System Bot]

Comment on attachment 101173 [details]
Patch

Attachment 101173 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9120051

[WebKit Review Bot]

Comment on attachment 101173 [details]
Patch

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

[Gustavo Noronha (kov)]

Comment on attachment 101173 [details]
Patch

Attachment 101173 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9116354

[Aaron Colwell]

Created attachment 101220 [details]
Patch

[WebKit Review Bot]

Comment on attachment 101220 [details]
Patch

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

[Antonio Gomes]

Comment on attachment 101220 [details]
Patch

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

Quick look, and one quick suggestion: I would change references to url's and Url's in the patch to URL.

Similarly to DOM and not Dom, and HTML and not Html throughout webkit source.

> Source/WebCore/html/HTMLMediaElement.cpp:1631
> +    }
> +
> +
> +    MediaPlayer::EndOfStreamStatus eosStatus = MediaPlayer::EosNoError;

Drop an empty line.

Eos = End of Stream?

[Aaron Colwell]

Created attachment 101348 [details]
Patch

[Aaron Colwell]

Created attachment 101364 [details]
Patch

[Aaron Colwell]

Created attachment 101371 [details]
Patch

[Eric Carlson]

Comment on attachment 101371 [details]
Patch

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

> Source/WebCore/html/HTMLMediaElement.cpp:198
> +    m_mediaSourceURL.setProtocol(mediaSourceURLProtocol);
> +    m_mediaSourceURL.setPath(createCanonicalUUIDString());

Why isn't *all* of the code for this experiment wrapped with #if ENABLE(MEDIA_SOURCE)?


> Source/WebCore/html/HTMLMediaElement.cpp:1675
> +    if (m_sourceState == SOURCE_CLOSED) {
> +        scheduleEvent(eventNames().closeEvent);
> +        return;
> +    }
> +
> +    if (oldState == SOURCE_CLOSED && m_sourceState == SOURCE_OPEN) {
> +        scheduleEvent(eventNames().openEvent);
> +        return;

Is it safe to use un-prefixed event names for an experimental feature?

[Aaron Colwell]

(In reply to comment #12)
> (From update of attachment 101371 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=101371&action=review
> 
> > Source/WebCore/html/HTMLMediaElement.cpp:198
> > +    m_mediaSourceURL.setProtocol(mediaSourceURLProtocol);
> > +    m_mediaSourceURL.setPath(createCanonicalUUIDString());
> 
> Why isn't *all* of the code for this experiment wrapped with #if ENABLE(MEDIA_SOURCE)?

I'll fix this. I didn't know how much #ifdefing would be tolerated. I just #ifdefed out the stuff that was exposed in the DOM. I'm assuming that you mean I should put #ifdefs every single source file I've modified.

> 
> 
> > Source/WebCore/html/HTMLMediaElement.cpp:1675
> > +    if (m_sourceState == SOURCE_CLOSED) {
> > +        scheduleEvent(eventNames().closeEvent);
> > +        return;
> > +    }
> > +
> > +    if (oldState == SOURCE_CLOSED && m_sourceState == SOURCE_OPEN) {
> > +        scheduleEvent(eventNames().openEvent);
> > +        return;
> 
> Is it safe to use un-prefixed event names for an experimental feature?
Probably not. Should I change this to webkitSourceOpen or something like that.

I'm open to suggestions. This is my first time trying to add an experimental feature so I'd appreciate any tips you may have.

[Aaron Colwell]

Created attachment 101481 [details]
Patch

[Aaron Colwell]

New patch is available for review. 

- I've surrounded all the new code with #if ENABLE(MEDIA_SOURCE) .
- I renamed the new event to webkitsourceopen.

[Eric Carlson]

Comment on attachment 101481 [details]
Patch

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

Two meta-issues:

1) I think this is complex enough that is needs to be completely described in a spec before any changes are checked in.

2) Even if this feature will only be enabled in Chromium, the code is in the main WebKit repository so we need tests in the same repository.


> Source/WebCore/html/HTMLMediaElement.cpp:1682
> +    if (m_sourceState == SOURCE_CLOSED) {
> +        scheduleEvent(eventNames().closeEvent);
> +        return;
> +    }

Shouldn't this be "webkitsourcecloseEvent" for symmetry?

> Source/WebCore/platform/graphics/MediaPlayer.cpp:144
> +    virtual bool sourceAppend(const unsigned char* data, unsigned length) { return false; }

These parameter names aren't needed.

> Source/WebCore/platform/graphics/MediaPlayerPrivate.h:151
> +    virtual bool sourceAppend(const unsigned char* data, unsigned length) { return false; }

These parameter names aren't needed.

[Aaron Colwell]

Created attachment 101674 [details]
Patch

[Aaron Colwell]

(In reply to comment #16)
> (From update of attachment 101481 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=101481&action=review
> 
> Two meta-issues:
> 
> 1) I think this is complex enough that is needs to be completely described in a spec before any changes are checked in.
> 

Ok. Here is my attempt at a W3C style spec.
http://html5-mediasource-api.googlecode.com/svn/tags/0.2/draft-spec/mediasource-draft-spec.html

Hopefully this is what you were looking for.

> 2) Even if this feature will only be enabled in Chromium, the code is in the main WebKit repository so we need tests in the same repository.
> 

Ok. I'll start working on this.

> 
> > Source/WebCore/html/HTMLMediaElement.cpp:1682
> > +    if (m_sourceState == SOURCE_CLOSED) {
> > +        scheduleEvent(eventNames().closeEvent);
> > +        return;
> > +    }
> 
> Shouldn't this be "webkitsourcecloseEvent" for symmetry?

Yes. Done.

> 
> > Source/WebCore/platform/graphics/MediaPlayer.cpp:144
> > +    virtual bool sourceAppend(const unsigned char* data, unsigned length) { return false; }
> 
> These parameter names aren't needed.

Done.

> 
> > Source/WebCore/platform/graphics/MediaPlayerPrivate.h:151
> > +    virtual bool sourceAppend(const unsigned char* data, unsigned length) { return false; }
> 
> These parameter names aren't needed.

Done.

[Aaron Colwell]

Created attachment 102526 [details]
Patch

[Aaron Colwell]

Spec & LayoutTests created. Reviewers please take another look. Thanks.

[WebKit Review Bot]

Comment on attachment 102526 [details]
Patch

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

New failing tests:
http/tests/media/video-media-source-errors.html
http/tests/media/video-media-source-state-changes.html
http/tests/media/video-media-source-play.html
http/tests/media/video-media-source-seek.html

[Aaron Colwell]

Created attachment 102684 [details]
Patch

[Aaron Colwell]

Created attachment 102925 [details]
Patch

[Aaron Colwell]

Ping. Can I get a review please.

[Eric Carlson]

Comment on attachment 102925 [details]
Patch

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

All of these tests are extremely WebM specific, whereas all of the existing media tests are format agnostic. This seems reasonable for now because I assume the feature only works with WebM files at the moment, but the test file names should reflect this. Or it may make more sense to make a "mediasource" directory in LayoutTests/media, and to have a folder in that for each format. This test structure will also make it easier to skip tests on ports where they aren't supported because you can just include "media/mediasource/" in the skipped file to skip all tests.

> LayoutTests/http/tests/media/media-source.js:23
> +  var r = new XMLHttpRequest();
> +  r.open("GET", url, false);
> +  r.responseType = 'arraybuffer';
> +  r.send();

Nit: We try to use descriptive names for variables in WebKit.

> LayoutTests/http/tests/media/media-source.js:25
> +  if (r.status != 200) {

Nit: Although there isn't a formal standard for tab width in JS files, I believe that all of the media tests use 4-tab spaces.

> LayoutTests/http/tests/media/media-source.js:39
> +function getNumClusters() {
> +  return cluster_info.length;
> +}

Nit: We try to use full names instead of abbreviations, eg. getClusterCount, getNumberOfClusters, etc.

> LayoutTests/http/tests/media/media-source.js:41
> +function getCluster(i) {

Nit: descriptive variable names are preferred. Here and throughout the rest of the file.

> LayoutTests/http/tests/media/media-source.js:119
> +    case 0:
> +      str = "SOURCE_CLOSED";
> +      break;
> +    case 1:
> +      str = "SOURCE_OPEN";
> +      break;
> +    case 2:

Is there any reason to not use the named constants, eg. "case HTMLMediaElement.SOURCE_CLOSED:", etc?

> LayoutTests/http/tests/media/video-media-source-errors.html:1
> +<html>

Nit: Is there any reason to not include an html5 DOCTYPE? Here and elsewhere.

> LayoutTests/http/tests/media/video-media-source-errors.html:35
> +        appendCluster(v, next_cluster_index);
> +        window.setTimeout(f, 0, next_cluster_index + 1);

Is there any way to do this with an event listener instead of setTimeout (here and elsewhere)? Timeouts have been the source of lots of flakiness in media tests.

> LayoutTests/http/tests/media/video-media-source-seek.html:40
> +       if (Math.abs(currentTime - seekTime) > 0.001) {

Why do you need the magic number? At the very least you should add a comment about why this is needed and what this tolerance is OK. How much variation do you actually see?

> LayoutTests/http/tests/media/video-media-source-state-changes.html:17
> +      window.setTimeout(appendUntilEnd, 0, v, i + 1);

More setTimeout.

> LayoutTests/http/tests/media/video-media-source-state-changes.html:23
> +        return

Missing a semi-colon here.

> LayoutTests/http/tests/media/video-media-source-state-changes.html:32
> +      window.setTimeout(appendClustersUntilTrue, 0, v, i + 1, conditionStr, done_cb);

More setTimeout.

> LayoutTests/http/tests/media/video-media-source-state-changes.html:62
> +      window.setTimeout(appendUntilEnd, 0, v, 0);

More setTimeout.

> LayoutTests/http/tests/media/video-media-source-state-changes.html:88
> +      window.setTimeout(appendClustersUntilTrue, 0,
> +                        v, startIndex, "firstSeekComplete",
> +                        function () { firstSeekClusterAppendComplete(v);} );

More setTimeout.

> LayoutTests/http/tests/media/video-media-source-state-changes.html:120
> +      window.setTimeout(appendClustersUntilTrue, 0,
> +                        v, startIndex, "secondSeekComplete",
> +                        function () { secondSeekClusterAppendComplete(v);} );
> +    }

More setTimeout.

> LayoutTests/http/tests/media/video-media-source-state-changes.html:159
> +      window.setTimeout(appendUntilEnd, 0, v, 0);

More setTimeout.

> Source/WebCore/html/HTMLMediaElement.cpp:571
>      if (m_networkState != NETWORK_EMPTY) {
> +#if ENABLE(MEDIA_SOURCE)
> +        setSourceState(SOURCE_CLOSED);
> +#endif
>          m_networkState = NETWORK_EMPTY;
>          m_readyState = HAVE_NOTHING;

Why do you set the state to SOURCE_CLOSED here...

> Source/WebCore/html/HTMLMediaElement.cpp:599
>      m_networkState = NETWORK_NO_SOURCE;
>  
> +#if ENABLE(MEDIA_SOURCE)
> +    setSourceState(SOURCE_CLOSED);
> +#endif
> +
>      // 2 - Asynchronously await a stable state.

and here?

> Source/WebCore/html/HTMLMediaElement.idl:104
> +    // media source state

Comment should be a complete sentence.

> Source/WebKit/chromium/ChangeLog:19
> [phone]  Aaron Colwell  <[e-mail]>
> +
> +        Add MediaSource API to HTMLMediaElement
> +        https://bugs.webkit.org/show_bug.cgi?id=64731
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        * features.gypi:
> +        * public/WebMediaPlayer.h:
> +        (WebKit::WebMediaPlayer::sourceAppend):
> +        (WebKit::WebMediaPlayer::sourceEndOfStream):
> +        * public/WebMediaPlayerClient.h:
> +        * src/WebMediaPlayerClientImpl.cpp:
> +        (WebKit::WebMediaPlayerClientImpl::sourceOpened):
> +        (WebKit::WebMediaPlayerClientImpl::sourceURL):
> +        (WebKit::WebMediaPlayerClientImpl::sourceAppend):
> +        (WebKit::WebMediaPlayerClientImpl::sourceEndOfStream):
> +        * src/WebMediaPlayerClientImpl.h:
> +

I am going to assume that you have had someone familiar with Chromium's embedder API review these changes, because I am not qualified.

[Aaron Colwell]

Created attachment 103964 [details]
Patch

[WebKit Review Bot]

Attachment 103964 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'LayoutTests/ChangeLog', u'La..." exit_code: 1

ERROR: FAILURES FOR <lucid, x86_64, release, cpu>
ERROR: Line:264 Path does not exist. platform/mac-snowleopard/fast/text/international/Geeza-Pro-vertical-metrics-adjustment.html
LayoutTests/platform/chromium/test_expectations.txt:264:  Path does not exist. platform/mac-snowleopard/fast/text/international/Geeza-Pro-vertical-metrics-adjustment.html  [test/expectations] [2]
Total errors found: 1 in 45 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Aaron Colwell]

Comment on attachment 102925 [details]
Patch

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

>> LayoutTests/http/tests/media/media-source.js:23
>> +  r.send();
> 
> Nit: We try to use descriptive names for variables in WebKit.

Done

>> LayoutTests/http/tests/media/media-source.js:25
>> +  if (r.status != 200) {
> 
> Nit: Although there isn't a formal standard for tab width in JS files, I believe that all of the media tests use 4-tab spaces.

Done

>> LayoutTests/http/tests/media/media-source.js:39
>> +}
> 
> Nit: We try to use full names instead of abbreviations, eg. getClusterCount, getNumberOfClusters, etc.

Done.

>> LayoutTests/http/tests/media/media-source.js:41
>> +function getCluster(i) {
> 
> Nit: descriptive variable names are preferred. Here and throughout the rest of the file.

Done.

>> LayoutTests/http/tests/media/media-source.js:119
>> +    case 2:
> 
> Is there any reason to not use the named constants, eg. "case HTMLMediaElement.SOURCE_CLOSED:", etc?

Nope. Fixed.

>> LayoutTests/http/tests/media/video-media-source-errors.html:1
>> +<html>
> 
> Nit: Is there any reason to not include an html5 DOCTYPE? Here and elsewhere.

Nope. Done.

>> LayoutTests/http/tests/media/video-media-source-errors.html:35
>> +        window.setTimeout(f, 0, next_cluster_index + 1);
> 
> Is there any way to do this with an event listener instead of setTimeout (here and elsewhere)? Timeouts have been the source of lots of flakiness in media tests.

Yes. Removed.

>> LayoutTests/http/tests/media/video-media-source-seek.html:40
>> +       if (Math.abs(currentTime - seekTime) > 0.001) {
> 
> Why do you need the magic number? At the very least you should add a comment about why this is needed and what this tolerance is OK. How much variation do you actually see?

This was just to compensate for the fact that I only went to 3 decimal places on the timestamps in the cluster_info table. I've replaced the values in the table with full precision values and removed this magic number code.

>> LayoutTests/http/tests/media/video-media-source-state-changes.html:17
>> +      window.setTimeout(appendUntilEnd, 0, v, i + 1);
> 
> More setTimeout.

Removed.

>> LayoutTests/http/tests/media/video-media-source-state-changes.html:23
>> +        return
> 
> Missing a semi-colon here.

Done.

>> LayoutTests/http/tests/media/video-media-source-state-changes.html:62
>> +      window.setTimeout(appendUntilEnd, 0, v, 0);
> 
> More setTimeout.

Removed.

>> LayoutTests/http/tests/media/video-media-source-state-changes.html:88
>> +                        function () { firstSeekClusterAppendComplete(v);} );
> 
> More setTimeout.

Removed.

>> LayoutTests/http/tests/media/video-media-source-state-changes.html:120
>> +    }
> 
> More setTimeout.

Removed.

>> LayoutTests/http/tests/media/video-media-source-state-changes.html:159
>> +      window.setTimeout(appendUntilEnd, 0, v, 0);
> 
> More setTimeout.

Removed.

> Source/WebCore/html/HTMLMediaElement.cpp:571
>          m_readyState = HAVE_NOTHING;

Oops. Good catch. I only need one and I've moved it slightly earlier in the method. I've also moved the location of a similar call in userCancelledLoad() so that the 2 methods fire abortEvent, webkitsourceclose, and emptied in the same order.

>> Source/WebCore/html/HTMLMediaElement.idl:104
>> +    // media source state
> 
> Comment should be a complete sentence.

Done. I was just copying the comment style from the networkState & readyState variables above.

>> Source/WebKit/chromium/ChangeLog:19
>> +
> 
> I am going to assume that you have had someone familiar with Chromium's embedder API review these changes, because I am not qualified.

Ok. I'll have some Chromium WebKit reviewers take a look.

[Aaron Colwell]

Created attachment 103985 [details]
Patch

[Eric Carlson]

Comment on attachment 103985 [details]
Patch

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

Marking r+ although I would prefer to have the stylistic nits addressed before it is landed. 

I think that it is important to run test for basic functionality like those in LayoutTests/media/ when data is loaded with this API. It would be great if there was a way to reuse the existing tests so we are sure there aren't any subtle errors caused by the use of this API, but I can't immediately think of a way to make that work.


> LayoutTests/http/tests/media/media-source/webm/video-media-source-errors.html:8
> + <head>
> +  <script src="../../../media-resources/video-test.js"></script>
> +  <script src="webm-media-source.js"></script>
> +  <script>
> +    function onError(event) {
> +        var video_tag = event.target;

Indentation should be consistent, not a mixture of 2 and 4 tab spaces.

> LayoutTests/http/tests/media/media-source/webm/video-media-source-errors.html:11
> +        var error_string = "UNKNOWN";
> +        switch(video_tag.error.code) {

Nit: WebKit tests almost always use camelCase for JavaScript variables. I was going to say that *all* WebKit tests do this but then I found a few WebGL tests... None-the-less, I would prefer to see these tests follow the convention used in all other media tests.


> LayoutTests/http/tests/media/media-source/webm/video-media-source-errors.html:29
> +    function appendClustersUntilMetadataLoaded(video_tag, done_callback) {

Very minor nit: this function name is confusing, I assumed that it would append clusters until the event, not just append two and wait.


> LayoutTests/http/tests/media/media-source/webm/video-media-source-errors.html:45
> +    function didNotSendHeadersFirst(event) {
> +        appendCluster(event.target, 0);
> +    }

WebKit convention is to put a function's first brace on its own line:

function didNotSendHeadersFirst(event)
{
    appendCluster(event.target, 0);
}


> LayoutTests/http/tests/media/media-source/webm/video-media-source-errors.html:54
> +        appendClustersUntilMetadataLoaded(video_tag, function(next_cluster_id) {
> +            appendCluster(video_tag, next_cluster_id - 1);
> +        });

Nit: declaring next_cluster_id as a global would have made this and the other places it is used slightly easier to understand.


> LayoutTests/http/tests/media/media-source/webm/video-media-source-errors.html:130
> +        var event_handler_function = function (event) {
> +             consoleWrite("running " + function_name);
> +             event.target.removeEventListener('webkitsourceopen', event_handler_function);
> +             on_open_function(event);
> +         };

Minor Nit: Having a blank line between tests results would make them slightly easier to read.


> LayoutTests/http/tests/media/media-source/webm/video-media-source-seek.html:28
> +        if (video_tag.currentTime > 2 && do_seek) {
> +            consoleWrite("EVENT(timeupdate) : seeking to " + seek_time);
> +            video_tag.pause();
> +            video_tag.currentTime = seek_time;
> +            do_seek = false;

Nit: logging a float with full precision caused platform specific failures in other media tests, so tests were changed to log and compare "value.toFixed(2)".


> LayoutTests/http/tests/media/media-source/webm/video-media-source-seek.html:45
> +        if (current_time != seek_time ) {
> +            failTest("Seeked to " + current_time + " instead of " + seek_time);
> +            return;
> +        }

Ditto.


> LayoutTests/http/tests/media/media-source/webm/webm-media-source.js:80
> +function appendCluster(video_tag, clusterIndex) {

Nit: Please use consistent naming style: camelCase or underscores.


> LayoutTests/http/tests/media/media-source/webm/webm-media-source.js:94
> +function appendUntilEndOfStream(video_tag, startIndex) {

Ditto.


> Source/WebKit/chromium/features.gypi:64
>        'ENABLE_METER_TAG=1',
> +      'ENABLE_MEDIA_SOURCE=1',
>        'ENABLE_MEDIA_STATISTICS=1',

Do you mean to turn this feature on for all Chromium builds even though it isn't complete?

[Aaron Colwell]

Comment on attachment 103985 [details]
Patch

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

>> LayoutTests/http/tests/media/media-source/webm/video-media-source-errors.html:8
>> +        var video_tag = event.target;
> 
> Indentation should be consistent, not a mixture of 2 and 4 tab spaces.

Done.

>> LayoutTests/http/tests/media/media-source/webm/video-media-source-errors.html:11
>> +        switch(video_tag.error.code) {
> 
> Nit: WebKit tests almost always use camelCase for JavaScript variables. I was going to say that *all* WebKit tests do this but then I found a few WebGL tests... None-the-less, I would prefer to see these tests follow the convention used in all other media tests.

Done.

>> LayoutTests/http/tests/media/media-source/webm/video-media-source-errors.html:29
>> +    function appendClustersUntilMetadataLoaded(video_tag, done_callback) {
> 
> Very minor nit: this function name is confusing, I assumed that it would append clusters until the event, not just append two and wait.

Done.

>> LayoutTests/http/tests/media/media-source/webm/video-media-source-errors.html:45
>> +    }
> 
> WebKit convention is to put a function's first brace on its own line:
> 
> function didNotSendHeadersFirst(event)
> {
>     appendCluster(event.target, 0);
> }

Done.

>> LayoutTests/http/tests/media/media-source/webm/video-media-source-errors.html:54
>> +        });
> 
> Nit: declaring next_cluster_id as a global would have made this and the other places it is used slightly easier to understand.

Done.

>> LayoutTests/http/tests/media/media-source/webm/video-media-source-errors.html:130
>> +         };
> 
> Minor Nit: Having a blank line between tests results would make them slightly easier to read.

Done

>> LayoutTests/http/tests/media/media-source/webm/video-media-source-seek.html:28
>> +            do_seek = false;
> 
> Nit: logging a float with full precision caused platform specific failures in other media tests, so tests were changed to log and compare "value.toFixed(2)".

Done.

>> LayoutTests/http/tests/media/media-source/webm/video-media-source-seek.html:45
>> +        }
> 
> Ditto.

Done.

>> LayoutTests/http/tests/media/media-source/webm/webm-media-source.js:80
>> +function appendCluster(video_tag, clusterIndex) {
> 
> Nit: Please use consistent naming style: camelCase or underscores.

Done.

>> LayoutTests/http/tests/media/media-source/webm/webm-media-source.js:94
>> +function appendUntilEndOfStream(video_tag, startIndex) {
> 
> Ditto.

Done.

>> Source/WebKit/chromium/features.gypi:64
>>        'ENABLE_MEDIA_STATISTICS=1',
> 
> Do you mean to turn this feature on for all Chromium builds even though it isn't complete?

No. I'm going to create a follow-on patch that will make the media source methods enabled at runtime so that Chromium can control this w/ a command-line flag. I'll add this line back in that patch.

[Aaron Colwell]

Created attachment 105535 [details]
Patch

[Eric Carlson]

Comment on attachment 105535 [details]
Patch

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

Marking r+ although, once again, I would prefer to have the stylistic nits addressed before it is landed.


> LayoutTests/http/tests/media/media-source/webm/video-media-source-errors.html:11
> +<html>
> + <head>
> +  <script src="../../../media-resources/video-test.js"></script>
> +  <script src="webm-media-source.js"></script>
> +  <script>
> +function onError(event)
> +{
> +    var videoTag = event.target;
> +
> +    var errorString = "UNKNOWN";

Now your indentation is using a mixture of *one* and four space tabs:

<html>
+<head>
++<script src="webm-media-source.js"></script>
++<script>
function onError(event)
{
++++var videoTag = event.target;

Please be consistent - use the same size tabs throughout for JS and markup. Indenting the markup and leaving the JS flush left is unusual.

Please pick a tab size and stick with it. As I noted before, the rest of the media tests use four space tabs so I would prefer that:

<html>
++++<head>
++++++++<script src="webm-media-source.js"></script>
++++++++<script>
++++++++++++function onError(event)
++++++++++++{
++++++++++++++++var videoTag = event.target;

In any case, be consistent. This may seem a niggling complaint, but indentation is meant to make code easier to read and comprehend and to make structure obvious. This is especially important in a big project like WebKit, and I don't think your current style succeeds on either account.

> LayoutTests/http/tests/media/media-source/webm/video-media-source-errors.html:60
> +    appendEnoughClustersToTriggerMetadataLoaded(videoTag, function() {
> +            appendCluster(videoTag, nextClusterIndex - 1);
> +        });

The closing brace should line up with the beginning of the line with the opening brace, as it does in the "eventHandler" function in appendEnoughClustersToTriggerMetadataLoaded above.

> LayoutTests/http/tests/media/media-source/webm/video-media-source-errors.html:75
> +    appendEnoughClustersToTriggerMetadataLoaded(videoTag, function() {
> +            // Append 1 past the next cluster.
> +            appendCluster(videoTag, nextClusterIndex + 1);
> +
> +            // Append the next cluster.
> +            appendCluster(videoTag, nextClusterIndex);
> +        });

The closing brace should line up with the beginning of the line with the opening brace

> LayoutTests/http/tests/media/media-source/webm/video-media-source-errors.html:101
> +    appendEnoughClustersToTriggerMetadataLoaded(videoTag, function() {
> +            videoTag.webkitSourceEndOfStream(HTMLMediaElement.EOS_DECODE_ERR);
> +        });

Ditto.

> LayoutTests/http/tests/media/media-source/webm/video-media-source-errors.html:123
> +    "networkErrorAfterHaveMetadata"
> +                 ];

Ditto, here and throughout the rest of the patch.

> Source/WebCore/ChangeLog:20
> +        (WebCore::HTMLMediaElement::HTMLMediaElement):
> +        (WebCore::HTMLMediaElement::prepareForLoad):
> +        (WebCore::HTMLMediaElement::loadResource):

It is helpful to have function by function explanations of what changed in your changelog.

> Source/WebCore/html/HTMLMediaElement.cpp:1297
> +    bool noSeekRequired = (!seekableRanges->length() || (time == now && displayMode() != Poster));

Is this change necessary?

> Source/WebCore/html/HTMLMediaElement.h:149
> +    enum EndOfStreamStatus { EOS_NO_ERROR, EOS_NETWORK_ERR, EOS_DECODE_ERR };
> +    void webkitSourceEndOfStream(unsigned short status, ExceptionCode&);

Is there any reason to not have "status" be an EndOfStreamStatus instead of an unsigned short?

> Source/WebCore/html/HTMLMediaElement.idl:110
> +#if defined(ENABLE_MEDIA_SOURCE) && ENABLE_MEDIA_SOURCE
> +    // URL passed to src attribute to enable the media source logic.
> +    readonly attribute [URL] DOMString webkitMediaSourceURL;
> +
> +    // Appends media to to the source.
> +    void webkitSourceAppend(in Uint8Array data) raises (DOMException);
> +
> +    // Signals the end of stream.
> +    const unsigned short EOS_NO_ERROR = 0; // End of stream reached w/o error.
> +    const unsigned short EOS_NETWORK_ERR = 1; // A network error triggered end of stream.
> +    const unsigned short EOS_DECODE_ERR = 2; // A decode error triggered end of stream.
> +    void webkitSourceEndOfStream(in unsigned short status) raises (DOMException);
> +
> +    // Indicates the current state of the media source.
> +    const unsigned short SOURCE_CLOSED = 0;
> +    const unsigned short SOURCE_OPEN = 1;
> +    const unsigned short SOURCE_ENDED = 2;
> +    readonly attribute unsigned short webkitSourceState;
> +#endif

Can these use "[Conditional= ENABLE_MEDIA_SOURCE]" instead of "#if definedENABLE_MEDIA_SOURCE"? See https://bugs.webkit.org/show_bug.cgi?id=64231 and https://bugs.webkit.org/show_bug.cgi?id=64961.

[Aaron Colwell]

Comment on attachment 105535 [details]
Patch

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

>> LayoutTests/http/tests/media/media-source/webm/video-media-source-errors.html:11
>> +    var errorString = "UNKNOWN";
> 
> Now your indentation is using a mixture of *one* and four space tabs:
> 
> <html>
> +<head>
> ++<script src="webm-media-source.js"></script>
> ++<script>
> function onError(event)
> {
> ++++var videoTag = event.target;
> 
> Please be consistent - use the same size tabs throughout for JS and markup. Indenting the markup and leaving the JS flush left is unusual.
> 
> Please pick a tab size and stick with it. As I noted before, the rest of the media tests use four space tabs so I would prefer that:
> 
> <html>
> ++++<head>
> ++++++++<script src="webm-media-source.js"></script>
> ++++++++<script>
> ++++++++++++function onError(event)
> ++++++++++++{
> ++++++++++++++++var videoTag = event.target;
> 
> In any case, be consistent. This may seem a niggling complaint, but indentation is meant to make code easier to read and comprehend and to make structure obvious. This is especially important in a big project like WebKit, and I don't think your current style succeeds on either account.

Done. Sorry about this. I was only focusing on the JS last time and my editor wasn't helping me with the HTML/JS mix. :(

>> LayoutTests/http/tests/media/media-source/webm/video-media-source-errors.html:60
>> +        });
> 
> The closing brace should line up with the beginning of the line with the opening brace, as it does in the "eventHandler" function in appendEnoughClustersToTriggerMetadataLoaded above.

Done.

>> LayoutTests/http/tests/media/media-source/webm/video-media-source-errors.html:75
>> +        });
> 
> The closing brace should line up with the beginning of the line with the opening brace

Done.

>> LayoutTests/http/tests/media/media-source/webm/video-media-source-errors.html:101
>> +        });
> 
> Ditto.

Done.

>> LayoutTests/http/tests/media/media-source/webm/video-media-source-errors.html:123
>> +                 ];
> 
> Ditto, here and throughout the rest of the patch.

Done.

>> Source/WebCore/ChangeLog:20
>> +        (WebCore::HTMLMediaElement::loadResource):
> 
> It is helpful to have function by function explanations of what changed in your changelog.

Ok. Will when I upload the new patch.

>> Source/WebCore/html/HTMLMediaElement.cpp:1297
>> +    bool noSeekRequired = (!seekableRanges->length() || (time == now && displayMode() != Poster));
> 
> Is this change necessary?

Nope. Removed.

>> Source/WebCore/html/HTMLMediaElement.h:149
>> +    void webkitSourceEndOfStream(unsigned short status, ExceptionCode&);
> 
> Is there any reason to not have "status" be an EndOfStreamStatus instead of an unsigned short?

It doesn't compile if I use EndOfStreamStatus. Clang complains w/ this error.

In file included from gen/webkit/bindings/V8DerivedSources06.cpp:58:
gen/webcore/bindings/V8HTMLMediaElement.cpp:449:34: error: cannot initialize a parameter of type 'WebCore::HTMLMediaElement::EndOfStreamStatus' with an lvalue of type 'int'
    imp->webkitSourceEndOfStream(status, ec);

I think this has to do with how I have the method declared in the IDL. I was modelling my code after the readyState & networkState attributes. I haven't seen a example of an enum in IDL that doesn't use unsigned short. If you can point me to an example, I'd be happy to change this code.

>> Source/WebCore/html/HTMLMediaElement.idl:110
>> +#endif
> 
> Can these use "[Conditional= ENABLE_MEDIA_SOURCE]" instead of "#if definedENABLE_MEDIA_SOURCE"? See https://bugs.webkit.org/show_bug.cgi?id=64231 and https://bugs.webkit.org/show_bug.cgi?id=64961.

So it appears that the short answer to this is no. It looks like the V8 code generator doesn't fully support Conditional on constants and functions. I'm getting all sorts of compile errors in the generated code that indicate some of the code isn't aware that the method/constant should be disabled. 

I can track down what is going on with the code generator, but for now I'd just like to leave this as is. I'll circle back and update this once the code generator is fixed.

[Aaron Colwell]

Created attachment 105683 [details]
Patch

[Eric Carlson]

(In reply to comment #34)
> > Can these use "[Conditional= ENABLE_MEDIA_SOURCE]" instead of "#if definedENABLE_MEDIA_SOURCE"? See https://bugs.webkit.org/show_bug.cgi?id=64231 and https://bugs.webkit.org/show_bug.cgi?id=64961.
> 
> So it appears that the short answer to this is no. It looks like the V8 code generator doesn't fully support Conditional on constants and functions. I'm getting all sorts of compile errors in the generated code that indicate some of the code isn't aware that the method/constant should be disabled. 
> 
> I can track down what is going on with the code generator, but for now I'd just like to leave this as is. I'll circle back and update this once the code generator is fixed.

Sounds like this might be worth a bug report.

[WebKit Review Bot]

Comment on attachment 105683 [details]
Patch

Clearing flags on attachment: 105683

Committed r94121: <http://trac.webkit.org/changeset/94121>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Aaron Colwell]

Comment on attachment 105535 [details]
Patch

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

>>> Source/WebCore/html/HTMLMediaElement.idl:110
>>> +#endif
>> 
>> Can these use "[Conditional= ENABLE_MEDIA_SOURCE]" instead of "#if definedENABLE_MEDIA_SOURCE"? See https://bugs.webkit.org/show_bug.cgi?id=64231 and https://bugs.webkit.org/show_bug.cgi?id=64961.
> 
> So it appears that the short answer to this is no. It looks like the V8 code generator doesn't fully support Conditional on constants and functions. I'm getting all sorts of compile errors in the generated code that indicate some of the code isn't aware that the method/constant should be disabled. 
> 
> I can track down what is going on with the code generator, but for now I'd just like to leave this as is. I'll circle back and update this once the code generator is fixed.

Created https://bugs.webkit.org/show_bug.cgi?id=67666 to track this issue.