Summary: | Update Media Source implementation to reflect changes in 0.5 spec. | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Aaron Colwell <acolwell> | ||||||||||||
Component: | Media | Assignee: | 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
Aaron Colwell
2012-04-10 12:38:49 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 Created attachment 141006 [details]
Patch
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 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? (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 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 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
(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. (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. (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"? Created attachment 141749 [details]
Updated ChangeLog & moved codecs parsing into ContentType
Ping. eric.carlson & fishd can I please get a review. 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 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 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. ;) Created attachment 142365 [details]
Patch
(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 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 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 Created attachment 142807 [details]
Fix extra line nit
Comment on attachment 142807 [details] Fix extra line nit Clearing flags on attachment: 142807 Committed r117662: <http://trac.webkit.org/changeset/117662> All reviewed patches have been landed. Closing bug. |