Bug 83607

Summary: Update Media Source implementation to reflect changes in 0.5 spec.
Product: WebKit Reporter: Aaron Colwell <acolwell>
Component: MediaAssignee: Aaron Colwell <acolwell>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, arun.patole, dglazkov, eric.carlson, feature-media-reviews, fishd, jamesr, ojan, tkent+wkapi, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 83616, 83788, 84996    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ec2-cr-linux-03
none
Updated ChangeLog & moved codecs parsing into ContentType
none
Patch
none
Fix extra line nit none

Description Aaron Colwell 2012-04-10 12:38:49 PDT
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.
Comment 1 Aaron Colwell 2012-04-16 17:22:05 PDT
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
Comment 2 Aaron Colwell 2012-05-09 13:31:59 PDT
Created attachment 141006 [details]
Patch
Comment 3 WebKit Review Bot 2012-05-09 13:35:44 PDT
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 4 Eric Carlson 2012-05-09 13:43:33 PDT
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?
Comment 5 Eric Carlson 2012-05-09 13:45:25 PDT
(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?
Comment 6 WebKit Review Bot 2012-05-09 15:20:31 PDT
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
Comment 7 WebKit Review Bot 2012-05-09 15:20:37 PDT
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
Comment 8 Aaron Colwell 2012-05-09 16:18:18 PDT
(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.
Comment 9 Aaron Colwell 2012-05-09 16:22:16 PDT
(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.
Comment 10 Eric Carlson 2012-05-10 10:30:03 PDT
(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"?
Comment 11 Aaron Colwell 2012-05-14 10:26:14 PDT
Created attachment 141749 [details]
Updated ChangeLog & moved codecs parsing into ContentType
Comment 12 Aaron Colwell 2012-05-16 09:12:42 PDT
Ping. eric.carlson & fishd can I please get a review.
Comment 13 Eric Carlson 2012-05-16 14:00:51 PDT
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?
Comment 14 James Robinson 2012-05-16 14:47:00 PDT
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.
Comment 15 Aaron Colwell 2012-05-16 16:26:13 PDT
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. ;)
Comment 16 Aaron Colwell 2012-05-16 16:26:37 PDT
Created attachment 142365 [details]
Patch
Comment 17 Aaron Colwell 2012-05-16 16:33:57 PDT
(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)
Comment 18 Darin Fisher (:fishd, Google) 2012-05-18 15:34:17 PDT
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
Comment 19 Aaron Colwell 2012-05-18 15:53:22 PDT
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
Comment 20 Aaron Colwell 2012-05-18 15:54:53 PDT
Created attachment 142807 [details]
Fix extra line nit
Comment 21 WebKit Review Bot 2012-05-18 18:08:04 PDT
Comment on attachment 142807 [details]
Fix extra line nit

Clearing flags on attachment: 142807

Committed r117662: <http://trac.webkit.org/changeset/117662>
Comment 22 WebKit Review Bot 2012-05-18 18:08:12 PDT
All reviewed patches have been landed.  Closing bug.