Bug 83607 – Update Media Source implementation to reflect changes in 0.5 spec.

• Component: Media
• Status: RESOLVED FIXED

[Aaron Colwell]

Update the Media Source implementation to reflect the changes made in the 0.4 version of the Media Source Extension spec (http://html5-mediasource-api.googlecode.com/svn/tags/0.4/draft-spec/mediasource-draft-spec.html)

This is an umbrella bug for the overall update. Individual patches will be dependencies.

[Aaron Colwell]

Updating this bug to the spec version proposed to the HTML-WG.

http://dvcs.w3.org/hg/html-media/raw-file/tip/media-source/media-source.html

[Aaron Colwell]

Created attachment 141006 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Eric Carlson]

Comment on attachment 141006 [details]
Patch

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

> Source/WebCore/ChangeLog:21
> +        (WebCore::HTMLMediaElement::parseAttribute):
> +        (WebCore):
> +        (WebCore::isDoubleQuote):
> +        (WebCore::parseCodecs):
> +        (WebCore::HTMLMediaElement::webkitSourceAddId):
> +        (WebCore::HTMLMediaElement::webkitSourceBuffered):
> +        (WebCore::HTMLMediaElement::webkitSourceAppend):
> +        (WebCore::HTMLMediaElement::webkitSourceAbort):

I would *really* prefer to have comments about what changed in each function, especially for a large patch like this.

> Source/WebCore/html/HTMLMediaElement.cpp:2321
> +static bool isDoubleQuote(UChar ch)
> +{
> +    return ch == '"';
> +}

Does this need to be in a separate function instead of just having a parameter in parseCodecs?

> Source/WebCore/html/HTMLMediaElement.cpp:2330
> +static bool parseCodecs(const ContentType& contentType, Vector<String>& codecs)
> +{
> +    String parameter = contentType.parameter("codecs");
> +
> +    if (parameter.isEmpty())
> +        return false;
> +

Why is this a static function in HTMLMediaElement instead of a ContentType method?

[Eric Carlson]

(In reply to comment #1)
> Updating this bug to the spec version proposed to the HTML-WG.
> 
> http://dvcs.w3.org/hg/html-media/raw-file/tip/media-source/media-source.html

These changes were only proposed a few days ago. Is it worth waiting for some feedback before submitting changes?

[WebKit Review Bot]

Comment on attachment 141006 [details]
Patch

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

New failing tests:
http/tests/media/media-source/webm/video-media-source-state-changes.html
http/tests/messaging/cross-domain-message-event-dispatch.html
http/tests/media/media-source/webm/video-media-source-errors.html
http/tests/media/media-source/webm/video-media-source-abort.html
http/tests/media/media-source/webm/video-media-source-play.html
http/tests/media/media-source/webm/video-media-source-add-and-remove-ids.html
http/tests/media/media-source/webm/video-media-source-seek.html

[WebKit Review Bot]

Created attachment 141026 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Aaron Colwell]

(In reply to comment #5)
> (In reply to comment #1)
> > Updating this bug to the spec version proposed to the HTML-WG.
> > 
> > http://dvcs.w3.org/hg/html-media/raw-file/tip/media-source/media-source.html
> 
> These changes were only proposed a few days ago. Is it worth waiting for some feedback before submitting changes?

These are changes that update the implementation to reflect what was proposed to the HTML-WG ~2.5 weeks ago. API-wise they are equivalent to the 0.4 version that was proposed to the Web & TV Interest Group a month before that. During that whole time I have not received any negative feedback about the functionality being added here.

I'd like to get these changes landed so we can start giving people a feel for the proposed API in upcoming Chrome Dev Channel builds.

[Aaron Colwell]

(In reply to comment #4)
> > Source/WebCore/ChangeLog:21
> > +        (WebCore::HTMLMediaElement::parseAttribute):
> > +        (WebCore):
> > +        (WebCore::isDoubleQuote):
> > +        (WebCore::parseCodecs):
> > +        (WebCore::HTMLMediaElement::webkitSourceAddId):
> > +        (WebCore::HTMLMediaElement::webkitSourceBuffered):
> > +        (WebCore::HTMLMediaElement::webkitSourceAppend):
> > +        (WebCore::HTMLMediaElement::webkitSourceAbort):
> 
> I would *really* prefer to have comments about what changed in each function, especially for a large patch like this.

Understood I'll add this information when I upload the next patch.

> 
> > Source/WebCore/html/HTMLMediaElement.cpp:2321
> > +static bool isDoubleQuote(UChar ch)
> > +{
> > +    return ch == '"';
> > +}
> 
> Does this need to be in a separate function instead of just having a parameter in parseCodecs?

This is needed by the removeCharacters() call in parseCodecs(). Is there an alternate way to do this? It seemed strange that I had to declare a function just so I could remove a single character from a string.

> 
> > Source/WebCore/html/HTMLMediaElement.cpp:2330
> > +static bool parseCodecs(const ContentType& contentType, Vector<String>& codecs)
> > +{
> > +    String parameter = contentType.parameter("codecs");
> > +
> > +    if (parameter.isEmpty())
> > +        return false;
> > +
> 
> Why is this a static function in HTMLMediaElement instead of a ContentType method?

I can definitely add this to ContentType. When I do should I wrap it with an #if ENABLE(MEDIA_SOURCE) or would you be ok with this functionality being available when MEDIA_SOURCE is not enabled. It definitely isn't specific to that feature.

[Eric Carlson]

(In reply to comment #9)
> (In reply to comment #4)
> > 
> > > Source/WebCore/html/HTMLMediaElement.cpp:2321
> > > +static bool isDoubleQuote(UChar ch)
> > > +{
> > > +    return ch == '"';
> > > +}
> > 
> > Does this need to be in a separate function instead of just having a parameter in parseCodecs?
> 
> This is needed by the removeCharacters() call in parseCodecs(). Is there an alternate way to do this? It seemed strange that I had to declare a function just so I could remove a single character from a string.
> 
Oops, my mistake. Teach me to do a drive-by review when I was rushing off to a meeting :-(

> > 
> > > Source/WebCore/html/HTMLMediaElement.cpp:2330
> > > +static bool parseCodecs(const ContentType& contentType, Vector<String>& codecs)
> > > +{
> > > +    String parameter = contentType.parameter("codecs");
> > > +
> > > +    if (parameter.isEmpty())
> > > +        return false;
> > > +
> > 
> > Why is this a static function in HTMLMediaElement instead of a ContentType method?
> 
> I can definitely add this to ContentType. When I do should I wrap it with an #if ENABLE(MEDIA_SOURCE) or would you be ok with this functionality being available when MEDIA_SOURCE is not enabled. It definitely isn't specific to that feature.

I don't think it needs to be specific to MEDIA_SOURCE. I think "parseCodecs" is probably not the best name, maybe just "codecs"?

[Aaron Colwell]

Created attachment 141749 [details]
Updated ChangeLog & moved codecs parsing into ContentType

[Aaron Colwell]

Ping. eric.carlson & fishd can I please get a review.

[Eric Carlson]

Comment on attachment 141749 [details]
Updated ChangeLog & moved codecs parsing into ContentType

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

This looks OK to me, but as we discussed on IRC I think it is unfortunate that so many tests of basic functionality are in the "media-source/webm" directory. As we also discussed, I think it is worth thinking about whether there is any way to move some of this API into a separate object that can be used as a source for the media element.

Not marking r+ because it changes public chromium API.

> LayoutTests/http/tests/media/media-source/webm/video-media-source-add-and-remove-ids.html:85
> +                    consoleWrite("Test an unsupported type.");
> +                    expectExceptionOnAddId(vt, "234", "audio/wav", DOMException.NOT_SUPPORTED_ERR);

Is it not possible to support WAV?

[James Robinson]

Comment on attachment 141749 [details]
Updated ChangeLog & moved codecs parsing into ContentType

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

> Source/WebCore/platform/graphics/MediaPlayerPrivate.h:168
> +    virtual MediaPlayer::AddIdStatus sourceAddId(const String&, const String&, const Vector<String>&) { return MediaPlayer::NotSupported; }

I think it'd be worth providing names for these parameters - WebKit style is to omit the name of the parameter if it redundant with the type (so don't call something String string) but to have it if the name isn't clear from the type.  I don't know what any of these parameters mean from the type (I'm guessing one is an id, but I don't know which is the id and I don't know what the other two parameters are).

Same goes for functions below.

[Aaron Colwell]

Comment on attachment 141749 [details]
Updated ChangeLog & moved codecs parsing into ContentType

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

>> Source/WebCore/platform/graphics/MediaPlayerPrivate.h:168
>> +    virtual MediaPlayer::AddIdStatus sourceAddId(const String&, const String&, const Vector<String>&) { return MediaPlayer::NotSupported; }
> 
> I think it'd be worth providing names for these parameters - WebKit style is to omit the name of the parameter if it redundant with the type (so don't call something String string) but to have it if the name isn't clear from the type.  I don't know what any of these parameters mean from the type (I'm guessing one is an id, but I don't know which is the id and I don't know what the other two parameters are).
> 
> Same goes for functions below.

Done

>> LayoutTests/http/tests/media/media-source/webm/video-media-source-add-and-remove-ids.html:85
>> +                    expectExceptionOnAddId(vt, "234", "audio/wav", DOMException.NOT_SUPPORTED_ERR);
> 
> Is it not possible to support WAV?

It is, but it would be pretty limited if someone did that. I've changed this to audio/x-unsupported-format since it is highly unlikely someone would support that. ;)

[Aaron Colwell]

Created attachment 142365 [details]
Patch

[Aaron Colwell]

(In reply to comment #13)
> (From update of attachment 141749 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=141749&action=review
> 
> This looks OK to me, but as we discussed on IRC I think it is unfortunate that so many tests of basic functionality are in the "media-source/webm" directory. As we also discussed, I think it is worth thinking about whether there is any way to move some of this API into a separate object that can be used as a source for the media element.

- Created an HTML-WG bug to discuss moving the Media Source methods and
  attributes out of the HTMLMediaElement.
  https://www.w3.org/Bugs/Public/show_bug.cgi?id=17082

- Filed a bug for moving the format independent tests out of media-source/webm. (https://bugs.webkit.org/show_bug.cgi?id=86688)

[Darin Fisher (:fishd, Google)]

Comment on attachment 142365 [details]
Patch

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

> Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:389
> +

nit: kill extra new line

[Aaron Colwell]

Comment on attachment 142365 [details]
Patch

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

>> Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:389
>> +
> 
> nit: kill extra new line

Done

[Aaron Colwell]

Created attachment 142807 [details]
Fix extra line nit

[WebKit Review Bot]

Comment on attachment 142807 [details]
Fix extra line nit

Clearing flags on attachment: 142807

Committed r117662: <http://trac.webkit.org/changeset/117662>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.