Summary: | Enable MediaSource::isTypeSupported() to handle the upper-cased MIME type & Codec | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Hyunduk Kim <hyunduk.kim> | ||||||||||||||
Component: | Media | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | commit-queue, eric.carlson, jer.noble | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Attachments: |
|
Description
Hyunduk Kim
2015-10-21 23:23:29 PDT
I'm working on the same. I meant not only MIME type but also codec. For example, AUDIO/WEBM;CODECS="vorbis" should work properly. Created attachment 263810 [details]
Patch
Comment on attachment 263810 [details] Patch Attachment 263810 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/320924 New failing tests: http/tests/media/media-source/mediasource-is-type-supported.html Created attachment 263813 [details]
Archive of layout-test-results from ews112 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112 Port: mac-yosemite Platform: Mac OS X 10.10.5
Created attachment 263814 [details]
Patch
Comment on attachment 263814 [details] Patch Attachment 263814 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/321062 New failing tests: http/tests/media/media-source/mediasource-is-type-supported.html Created attachment 263818 [details]
Archive of layout-test-results from ews113 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113 Port: mac-yosemite Platform: Mac OS X 10.10.5
Created attachment 263819 [details]
Patch
Newly added TC is passed in the Chrome Browser ( Version 46.0.2490.71 m ). Comment on attachment 263819 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=263819&action=review > LayoutTests/http/tests/media/media-source/mediasource-is-type-supported-expected.txt:30 > -PASS Test valid WebM type "audio/webm;codecs="vorbis"" > +PASS Test valid WebM type "audio/webm;codecs="vorbis"" > +PASS Test valid WebM type "AUDIO/WEBM;CODECS="vorbis"" > PASS Test valid MP4 type "video/mp4;codecs="avc1.4d001e"" > PASS Test valid MP4 type "video/mp4;codecs="avc1.42001e"" This should be tested on all ports, so you should also have an upper/mixed case type that is supported by ports that don't support webm/vorbis. Created attachment 264027 [details]
Patch
I added upper/mixed case for both webm and mp4. Comment on attachment 264027 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=264027&action=review > Source/WebCore/Modules/mediasource/MediaSource.cpp:732 > if (type.isNull() || type.isEmpty()) Not part of your patch, but this line of code is slightly wasteful. The isNull check isn’t needed because all null strings are empty strings too. > Source/WebCore/Modules/mediasource/MediaSource.cpp:735 > + ContentType contentType(type.lower()); There are two issues here. When we want to fold case for something like this, we want to fold only ASCII case, not full Unicode case. That means we should use functions like convertToASCIILowercase() rather than lower(). I am not sure that this call site is the right level to fold case. We should study the other clients of ContentType and see why they don’t need lowercasing. If they do, then convertToASCIILowercase should be done in the ContentType constructor, not here. There’s even another use of ContentType in the same file that might need the case folding. The problem with that is that changing ContentType will affect a lot more code paths and we’ll have to do more testing, and possibly remove some now-unneeded code that does additional lowercasing in other cases that use ContentType. I am going to say review+ but we should do better on both of those issues if we can. Thank you for your comment, Darin. I'll check what you mentioned, too. Comment on attachment 264027 [details] Patch Clearing flags on attachment: 264027 Committed r191888: <http://trac.webkit.org/changeset/191888> All reviewed patches have been landed. Closing bug. |