Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
254983006:
Fix: Adding list of supported codecs for MP4 containers (Closed)
|
Created:
6 years, 7 months ago by amogh.bihani Modified:
6 years, 6 months ago Reviewers:
Avi (use Gerrit), scherkus (not reviewing), acolwell GONE FROM CHROMIUM, Ryan Sleevi, ddorwin CC:
chromium-reviews, cbentzel+watch_chromium.org, jam, feature-media-reviews_chromium.org, darin-cc_chromium.org, erikwright+watch_chromium.org, jshin+watch_chromium.org, xhwang Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionFix: Adding list of supported codecs for MP4 containers
A table is added for MP4 mime types and their corresponding containers
H.264 codecs vary according to the profile version. These are
represented by hexadecimal numbers (RFC6381). These vary across
platforms, hence a definite "probably" cannot be given for canPlayType
query. The best we can say is a "maybe" if the codec is RFC compliant.
New behaviour of the browser will be
audio/mp4 -> avc1/ avc1./ avc1.4D4001 = ""
audio/mp4 -> mp4a/ mp4a.40/ mp4a.40.2 = "maybe"
audio/mp4 -> mp4a./ mp4a.50/ mp4a.40. = ""
video/mp4 -> avc1/avc1.4D4001/ = "maybe"
video/mp4 -> avc1./ avc1.NonHexDigits = ""
(similar for mp4a and avc3)
BUG=53193
TEST=out/Release/content_browsertests --gtest_filter=MediaCanPlayType*
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=277386
Patch Set 1 #
Total comments: 4
Patch Set 2 : Changing HigherCase to UpperCase #Patch Set 3 : Adding testcases for EME #
Total comments: 13
Patch Set 4 : Removing extended codecs tests #
Total comments: 18
Patch Set 5 : Rebased, comments not addressed yet #Patch Set 6 : Making IsSupportedStrictMediaMimeType return enum #
Total comments: 12
Patch Set 7 : Addressing comments, adding mp4a to list #
Total comments: 14
Patch Set 8 : Added test for wrong extended codecs #
Total comments: 8
Patch Set 9 : Addressing comments #
Total comments: 14
Patch Set 10 : Addressing some comments, changing '.' handling in next PS #Patch Set 11 : Changing AreSupportedCodecsWithProfile() implementation #
Total comments: 9
Patch Set 12 : adding compile_assert, correcting mpeg audio codec values #
Total comments: 4
Patch Set 13 : Addressing comments #
Total comments: 6
Patch Set 14 : Changing mp4a.40 to mp4a.40.2 #
Total comments: 22
Patch Set 15 : adding more comments, adding tests for mp4a #
Total comments: 4
Patch Set 16 : Addressing comments #Patch Set 17 : removing nit #
Total comments: 6
Patch Set 18 : Modifying comments #
Total comments: 1
Patch Set 19 : adding check for std::string::npos #Messages
Total messages: 56 (0 generated)
PTAL.
1) You'll need a base owner for base/ changes 2) I don't think you need base/ changes. 3) Even if you did (which I don't think you do), "IsHigher" -> "IsUpper" https://codereview.chromium.org/254983006/diff/1/net/base/mime_util.cc File net/base/mime_util.cc (right): https://codereview.chromium.org/254983006/diff/1/net/base/mime_util.cc#newcode98 net/base/mime_util.cc:98: StrictExpressionMappings; style: Formatting is wrong here. Use git-cl format https://codereview.chromium.org/254983006/diff/1/net/base/mime_util.cc#newcod... net/base/mime_util.cc:108: const std::vector<std::string>& codecs); Style: Formatting is wrong here https://codereview.chromium.org/254983006/diff/1/net/base/mime_util.cc#newcod... net/base/mime_util.cc:466: "avc1,avc3,avc1.??????,avc3.??????,mp4a,mp4a.40,mp4a.40.?" } Why ? instead of *?
https://codereview.chromium.org/254983006/diff/1/net/base/mime_util.cc File net/base/mime_util.cc (right): https://codereview.chromium.org/254983006/diff/1/net/base/mime_util.cc#newcod... net/base/mime_util.cc:466: "avc1,avc3,avc1.??????,avc3.??????,mp4a,mp4a.40,mp4a.40.?" } On 2014/04/28 22:02:55, Ryan Sleevi wrote: > Why ? instead of *? Because '*' wildcard can take any number of characters[1] and we specifically want 6 digit upper case hexadecimals. If we use something like avc1.*?, then it will return true even for "avc1.123456789" or "avc1.12", which would be wrong. ---------- [1] https://code.google.com/p/chromium/codesearch#chromium/src/base/strings/strin...
ddorwin@ I have added test cases in encrypted_media_istypesupported_browsertest.cc as we had done in https://codereview.chromium.org/150653008/ (I'll be closing that CL when this is done.)
I reviewed encrypted_media_istypesupported_browsertest.cc and a few other things. You'll need acolwell/scherkus to review the main behavior changes. https://codereview.chromium.org/254983006/diff/40001/chrome/browser/media/enc... File chrome/browser/media/encrypted_media_istypesupported_browsertest.cc (right): https://codereview.chromium.org/254983006/diff/40001/chrome/browser/media/enc... chrome/browser/media/encrypted_media_istypesupported_browsertest.cc:511: // Invalid codec format, profile parameter must be present after the period. nit: use a : or - instead of a , https://codereview.chromium.org/254983006/diff/40001/chrome/browser/media/enc... chrome/browser/media/encrypted_media_istypesupported_browsertest.cc:705: // Invalid codec format, profile parameter must be present after the period. ditto https://codereview.chromium.org/254983006/diff/40001/chrome/browser/media/enc... chrome/browser/media/encrypted_media_istypesupported_browsertest.cc:892: "video/mp4", aac_extended_codec(), kWidevineAlpha)); This is the only place where an extended OR audio codec appears in the Valid video types section. Since we have extended checks below, it probably doesn't belong here. It would be nice to check audio-only codecs with video/, but in a different CL. https://codereview.chromium.org/254983006/diff/40001/chrome/browser/media/enc... chrome/browser/media/encrypted_media_istypesupported_browsertest.cc:908: "video/mp4", aac_extended_codec(), kWidevine)); ditto. I see how this makes sense. Maybe we should (in a separate CL) merge extended codecs since they are more relevant now. https://codereview.chromium.org/254983006/diff/40001/chrome/browser/media/enc... chrome/browser/media/encrypted_media_istypesupported_browsertest.cc:920: // Invalid codec format, profile paramter must be present after the period. ditto on comma https://codereview.chromium.org/254983006/diff/40001/content/renderer/rendere... File content/renderer/renderer_webkitplatformsupport_impl.cc (right): https://codereview.chromium.org/254983006/diff/40001/content/renderer/rendere... content/renderer/renderer_webkitplatformsupport_impl.cc:441: // H.264 codecs have profile version in their names. H.264 does not match the function name. https://codereview.chromium.org/254983006/diff/40001/content/renderer/rendere... content/renderer/renderer_webkitplatformsupport_impl.cc:442: if (net::IsSupportedStrictMP4MediaMimeType(mime_type_ascii, strict_codecs)) You might also comment that since we do not check for exact profile versions, we will only return maybe. However, it would really be better to have the function make that determination. (The current implementation would always return maybe, but this code shouldn't need to know that.) Then you wouldn't need the comment. https://codereview.chromium.org/254983006/diff/40001/net/base/mime_util.h File net/base/mime_util.h (right): https://codereview.chromium.org/254983006/diff/40001/net/base/mime_util.h#new... net/base/mime_util.h:89: // supports certain subset of H.264 codecs. Returns true if and only if all the The comment only mentions H.264 (should be "avc1"?) but AAC is also affected. This is also used for HLS. Maybe s/MP4/MPEG/ in the name.
acolwell@, scherkus@ The new behaviour is same as what we had discussed in https://codereview.chromium.org/216893002/ PTAL. https://codereview.chromium.org/254983006/diff/40001/chrome/browser/media/enc... File chrome/browser/media/encrypted_media_istypesupported_browsertest.cc (right): https://codereview.chromium.org/254983006/diff/40001/chrome/browser/media/enc... chrome/browser/media/encrypted_media_istypesupported_browsertest.cc:511: // Invalid codec format, profile parameter must be present after the period. On 2014/04/29 17:12:54, ddorwin wrote: > nit: use a : or - instead of a , Done. https://codereview.chromium.org/254983006/diff/40001/chrome/browser/media/enc... chrome/browser/media/encrypted_media_istypesupported_browsertest.cc:892: "video/mp4", aac_extended_codec(), kWidevineAlpha)); On 2014/04/29 17:12:54, ddorwin wrote: > This is the only place where an extended OR audio codec appears in the Valid > video types section. > > Since we have extended checks below, it probably doesn't belong here. > > It would be nice to check audio-only codecs with video/, but in a different CL. Done. https://codereview.chromium.org/254983006/diff/40001/chrome/browser/media/enc... chrome/browser/media/encrypted_media_istypesupported_browsertest.cc:908: "video/mp4", aac_extended_codec(), kWidevine)); On 2014/04/29 17:12:54, ddorwin wrote: > ditto. I see how this makes sense. Maybe we should (in a separate CL) merge > extended codecs since they are more relevant now. Done. https://codereview.chromium.org/254983006/diff/40001/content/browser/media/me... File content/browser/media/media_canplaytype_browsertest.cc (right): https://codereview.chromium.org/254983006/diff/40001/content/browser/media/me... content/browser/media/media_canplaytype_browsertest.cc:770: IN_PROC_BROWSER_TEST_F(MediaCanPlayTypeTest, CodecSupportTest_HLS) { Please note that changes will be made for these tests as well but, they can be done after https://codereview.chromium.org/230413003/ merges. https://codereview.chromium.org/254983006/diff/40001/net/base/mime_util.h File net/base/mime_util.h (right): https://codereview.chromium.org/254983006/diff/40001/net/base/mime_util.h#new... net/base/mime_util.h:89: // supports certain subset of H.264 codecs. Returns true if and only if all the On 2014/04/29 17:12:54, ddorwin wrote: > The comment only mentions H.264 (should be "avc1"?) but AAC is also affected. > This is also used for HLS. Maybe s/MP4/MPEG/ in the name. Done.
acolwell@ PTAL.
here is an initial round of comments. I agree with rsleevi that you shouldn't need the base changes. https://codereview.chromium.org/254983006/diff/60001/content/browser/media/me... File content/browser/media/media_canplaytype_browsertest.cc (right): https://codereview.chromium.org/254983006/diff/60001/content/browser/media/me... content/browser/media/media_canplaytype_browsertest.cc:505: EXPECT_EQ(kPropMaybe, CanPlay("'video/mp4; codecs=\"mp4a\"'")); "mp4a" is not rfc6381 compliant so this and all other tests that include it should be in the kNot category. https://codereview.chromium.org/254983006/diff/60001/content/browser/media/me... content/browser/media/media_canplaytype_browsertest.cc:525: EXPECT_EQ(kNot, CanPlay("'video/mp4; codecs=\"avc1.4d401e\"'")); While I know the RFC says that these values are case sensitive, I believe you'll likely break sites if you enforce a case sensitive check for the hex values. I think you should still allow lower case hex digits. https://codereview.chromium.org/254983006/diff/60001/content/browser/media/me... content/browser/media/media_canplaytype_browsertest.cc:574: EXPECT_EQ(kPropMaybe, CanPlay("'video/x-m4v; codecs=\"mp4a\"'")); Should be kNot here and other cases below. https://codereview.chromium.org/254983006/diff/60001/content/renderer/rendere... File content/renderer/renderer_webkitplatformsupport_impl.cc (right): https://codereview.chromium.org/254983006/diff/60001/content/renderer/rendere... content/renderer/renderer_webkitplatformsupport_impl.cc:444: if (net::IsSupportedStrictMP4OrHLSMediaMimeType(mime_type_ascii, I don't think MP4OrHLS should be in the method name. It seems to me that IsSupportedStrictMediaMimeType() should be changed to return an enum that includes a MayBe or this method is be something like MayBeSupportedStrictMediaMimeType() since really you are just trying to capture the MayBe situations. My preference is the former solution since I don't really see the point of having 2 methods here. https://codereview.chromium.org/254983006/diff/60001/net/base/mime_util.cc File net/base/mime_util.cc (right): https://codereview.chromium.org/254983006/diff/60001/net/base/mime_util.cc#ne... net/base/mime_util.cc:459: {"audio/mp4", "mp4a,mp4a.40,mp4a.40.?"}, mp4a is not valid. mp4a.67 should be added to this list as well since that is what is used for MPEG2 AAC in an MP4 container. This applies everywhere below as well. https://codereview.chromium.org/254983006/diff/60001/net/base/mime_util.cc#ne... net/base/mime_util.cc:461: {"video/mp4", "avc1,avc3,avc1.??????,avc3.??????,mp4a,mp4a.40,mp4a.40.?"}, nit: perhaps move this into a static constant so it is a little clearer that these are all using the exact same value. https://codereview.chromium.org/254983006/diff/60001/net/base/mime_util.cc#ne... net/base/mime_util.cc:491: if (MatchPattern(static_cast<base::StringPiece>(codecs[i]), nit: reverse condition & continue https://codereview.chromium.org/254983006/diff/60001/net/base/mime_util.cc#ne... net/base/mime_util.cc:497: !IsUpperCaseHexNumber(split_string.back())) { I don't think we should enforce upper case here. You'll likely break existing sites for no good reason. https://codereview.chromium.org/254983006/diff/60001/net/base/mime_util.cc#ne... net/base/mime_util.cc:498: codec_matched = false; nit: just return false here and move codec_matched = true right before the break below since that is when you know you actually have a match.
I too agree that this doesn't have to be in base/.
On 2014/05/14 10:58:14, Nico (traveling this week) wrote: > I too agree that this doesn't have to be in base/. Yes. Now that acolwell has advised not to take care of case sensitivity, I can remove the base changes.
Thanks for the review. :) I have made the changes. https://codereview.chromium.org/254983006/diff/60001/content/browser/media/me... File content/browser/media/media_canplaytype_browsertest.cc (right): https://codereview.chromium.org/254983006/diff/60001/content/browser/media/me... content/browser/media/media_canplaytype_browsertest.cc:505: EXPECT_EQ(kPropMaybe, CanPlay("'video/mp4; codecs=\"mp4a\"'")); On 2014/05/12 14:54:18, acolwell_OOO_5-12_to_5-15 wrote: > "mp4a" is not rfc6381 compliant so this and all other tests that include it > should be in the kNot category. Done. https://codereview.chromium.org/254983006/diff/60001/content/browser/media/me... content/browser/media/media_canplaytype_browsertest.cc:525: EXPECT_EQ(kNot, CanPlay("'video/mp4; codecs=\"avc1.4d401e\"'")); On 2014/05/12 14:54:18, acolwell_OOO_5-12_to_5-15 wrote: > While I know the RFC says that these values are case sensitive, I believe you'll > likely break sites if you enforce a case sensitive check for the hex values. I > think you should still allow lower case hex digits. Done. https://codereview.chromium.org/254983006/diff/60001/content/browser/media/me... content/browser/media/media_canplaytype_browsertest.cc:574: EXPECT_EQ(kPropMaybe, CanPlay("'video/x-m4v; codecs=\"mp4a\"'")); On 2014/05/12 14:54:18, acolwell_OOO_5-12_to_5-15 wrote: > Should be kNot here and other cases below. Done. https://codereview.chromium.org/254983006/diff/60001/content/renderer/rendere... File content/renderer/renderer_webkitplatformsupport_impl.cc (right): https://codereview.chromium.org/254983006/diff/60001/content/renderer/rendere... content/renderer/renderer_webkitplatformsupport_impl.cc:444: if (net::IsSupportedStrictMP4OrHLSMediaMimeType(mime_type_ascii, On 2014/05/12 14:54:18, acolwell_OOO_5-12_to_5-15 wrote: > I don't think MP4OrHLS should be in the method name. It seems to me that > IsSupportedStrictMediaMimeType() should be changed to return an enum that > includes a MayBe or this method is be something like > MayBeSupportedStrictMediaMimeType() since really you are just trying to capture > the MayBe situations. My preference is the former solution since I don't really > see the point of having 2 methods here. Done. https://codereview.chromium.org/254983006/diff/60001/net/base/mime_util.cc File net/base/mime_util.cc (right): https://codereview.chromium.org/254983006/diff/60001/net/base/mime_util.cc#ne... net/base/mime_util.cc:459: {"audio/mp4", "mp4a,mp4a.40,mp4a.40.?"}, On 2014/05/12 14:54:18, acolwell_OOO_5-12_to_5-15 wrote: > mp4a is not valid. mp4a.67 should be added to this list as well since that is > what is used for MPEG2 AAC in an MP4 container. This applies everywhere below as > well. Done. https://codereview.chromium.org/254983006/diff/60001/net/base/mime_util.cc#ne... net/base/mime_util.cc:461: {"video/mp4", "avc1,avc3,avc1.??????,avc3.??????,mp4a,mp4a.40,mp4a.40.?"}, On 2014/05/12 14:54:18, acolwell_OOO_5-12_to_5-15 wrote: > nit: perhaps move this into a static constant so it is a little clearer that > these are all using the exact same value. Done. https://codereview.chromium.org/254983006/diff/60001/net/base/mime_util.cc#ne... net/base/mime_util.cc:491: if (MatchPattern(static_cast<base::StringPiece>(codecs[i]), On 2014/05/12 14:54:18, acolwell_OOO_5-12_to_5-15 wrote: > nit: reverse condition & continue Done. https://codereview.chromium.org/254983006/diff/60001/net/base/mime_util.cc#ne... net/base/mime_util.cc:497: !IsUpperCaseHexNumber(split_string.back())) { On 2014/05/12 14:54:18, acolwell_OOO_5-12_to_5-15 wrote: > I don't think we should enforce upper case here. You'll likely break existing > sites for no good reason. Done. https://codereview.chromium.org/254983006/diff/60001/net/base/mime_util.cc#ne... net/base/mime_util.cc:498: codec_matched = false; On 2014/05/12 14:54:18, acolwell_OOO_5-12_to_5-15 wrote: > nit: just return false here and move codec_matched = true right before the > break below since that is when you know you actually have a match. Done.
https://codereview.chromium.org/254983006/diff/100001/net/base/mime_util.cc File net/base/mime_util.cc (right): https://codereview.chromium.org/254983006/diff/100001/net/base/mime_util.cc#n... net/base/mime_util.cc:516: // We check whether the suffix is hexadecimal or not. Pronouns in comments considered harmful - "We", in this case - https://groups.google.com/a/chromium.org/d/topic/chromium-dev/NH-S6KCkr2M/dis... // Check whether the suffix is hexadecimal https://codereview.chromium.org/254983006/diff/100001/net/base/mime_util.cc#n... net/base/mime_util.cc:519: std::string number = split_string.back(); const std::string& https://codereview.chromium.org/254983006/diff/100001/net/base/mime_util.cc#n... net/base/mime_util.cc:521: // We are neglecting case sensitivity as it might break some websites. // Don't enforce case sensitivity, even though it's called for, as it would break some websites. https://codereview.chromium.org/254983006/diff/100001/net/base/mime_util.cc#n... net/base/mime_util.cc:526: // We have a match! Now no need to compare it with other supported codecs. update https://codereview.chromium.org/254983006/diff/100001/net/base/mime_util.cc#n... net/base/mime_util.cc:530: // Return false if any of the codecs is not matching. This is perhaps too descriptive - the next two lines read the same. https://codereview.chromium.org/254983006/diff/100001/net/base/mime_util.h File net/base/mime_util.h (right): https://codereview.chromium.org/254983006/diff/100001/net/base/mime_util.h#ne... net/base/mime_util.h:85: // supported. This comment block is now out of date and needs to be updated. This "smells" like a layering violation though - letting details about Blink seep into net/.
rsleevi@ comments are in PS#6 ddorwin@ and acolwell@ comments are in PS#7 https://codereview.chromium.org/254983006/diff/100001/net/base/mime_util.cc File net/base/mime_util.cc (right): https://codereview.chromium.org/254983006/diff/100001/net/base/mime_util.cc#n... net/base/mime_util.cc:516: // We check whether the suffix is hexadecimal or not. On 2014/05/15 22:48:35, Ryan Sleevi wrote: > Pronouns in comments considered harmful - "We", in this case - > https://groups.google.com/a/chromium.org/d/topic/chromium-dev/NH-S6KCkr2M/dis... > > // Check whether the suffix is hexadecimal Done. https://codereview.chromium.org/254983006/diff/100001/net/base/mime_util.cc#n... net/base/mime_util.cc:519: std::string number = split_string.back(); On 2014/05/15 22:48:35, Ryan Sleevi wrote: > const std::string& Done. https://codereview.chromium.org/254983006/diff/100001/net/base/mime_util.cc#n... net/base/mime_util.cc:521: // We are neglecting case sensitivity as it might break some websites. On 2014/05/15 22:48:35, Ryan Sleevi wrote: > // Don't enforce case sensitivity, even though it's called for, as it would > break some websites. Done. https://codereview.chromium.org/254983006/diff/100001/net/base/mime_util.cc#n... net/base/mime_util.cc:526: // We have a match! Now no need to compare it with other supported codecs. On 2014/05/15 22:48:35, Ryan Sleevi wrote: > update removed it. The "break" explains this. https://codereview.chromium.org/254983006/diff/100001/net/base/mime_util.cc#n... net/base/mime_util.cc:530: // Return false if any of the codecs is not matching. On 2014/05/15 22:48:35, Ryan Sleevi wrote: > This is perhaps too descriptive - the next two lines read the same. Done. https://codereview.chromium.org/254983006/diff/100001/net/base/mime_util.h File net/base/mime_util.h (right): https://codereview.chromium.org/254983006/diff/100001/net/base/mime_util.h#ne... net/base/mime_util.h:85: // supported. On 2014/05/15 22:48:35, Ryan Sleevi wrote: > This comment block is now out of date and needs to be updated. done. > This "smells" like a layering violation though - letting details about Blink > seep into net/. That's why I had made a new method for checking MP4 and HLS mime types. Acolwell had advised to change this to return enum. This does look cleaner than my previous approach. https://codereview.chromium.org/254983006/diff/110001/content/renderer/render... File content/renderer/renderer_webkitplatformsupport_impl.cc (right): https://codereview.chromium.org/254983006/diff/110001/content/renderer/render... content/renderer/renderer_webkitplatformsupport_impl.cc:422: net::ParseCodecString(ToASCIIOrEmpty(codecs), &strict_codecs, true); ddorwin@ After adding mp4 containers in strict media mime type list, I had to remove this condition as EME does not consider extended codecs[1]. However, I think we do not need to change any thing there. When IsSupportedKeySystemWithMediaMimeType() returns true, the code has to go through line 437. Here the logic is implemented, so from the overall browser's perspective, we might end up executing same commands twice. Is it fine to keep that part as is or do we need to make changes there as well? If yes, can we do that in another CL? I am not much familiar with that part and might need some time. ---------- [1] https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m... https://codereview.chromium.org/254983006/diff/110001/net/base/mime_util.cc File net/base/mime_util.cc (right): https://codereview.chromium.org/254983006/diff/110001/net/base/mime_util.cc#n... net/base/mime_util.cc:472: "mp4a,mp4a.40,mp4a.67,mp4a.40.?,mp4a.67.?"; acolwell@ I think we should keep mp4a in our list, removing it might as well break some websites.
rsleevi@ I think you might need to expand the comment in https://codereview.chromium.org/254983006/diff/100001/net/base/mime_util.h?co.... Otherwise it just shows done.
https://codereview.chromium.org/254983006/diff/110001/content/renderer/render... File content/renderer/renderer_webkitplatformsupport_impl.cc (right): https://codereview.chromium.org/254983006/diff/110001/content/renderer/render... content/renderer/renderer_webkitplatformsupport_impl.cc:422: net::ParseCodecString(ToASCIIOrEmpty(codecs), &strict_codecs, true); On 2014/05/16 11:25:29, amogh.bihani wrote: > ddorwin@ > After adding mp4 containers in strict media mime type list, I had to remove this > condition as EME does not consider extended codecs[1]. > However, I think we do not need to change any thing there. When > IsSupportedKeySystemWithMediaMimeType() returns true, the code has to go through > line 437. Here the logic is implemented, so from the overall browser's > perspective, we might end up executing same commands twice. > > Is it fine to keep that part as is or do we need to make changes there as well? > If yes, can we do that in another CL? I am not much familiar with that part and > might need some time. > ---------- > [1] > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m... This will allow "vp8.1", which it shouldn't. EME should handle extended codecs in the same way as the rest of Chromium. This specific code will eventually be removed, so it's not worth addressing now, but we need to fix the permanent code elsewhere. Please file a bug. Also, please add a test for "vp8.1" that demonstrates this failure and reference the bug. (If we don't have a non-EME test for "vp8.1", we should add that too. https://codereview.chromium.org/254983006/diff/110001/content/renderer/render... content/renderer/renderer_webkitplatformsupport_impl.cc:438: net::IsSupportedStrictMediaMimeType(mime_type_ascii,strict_codecs)); nit: space after ',' https://codereview.chromium.org/254983006/diff/110001/content/renderer/render... content/renderer/renderer_webkitplatformsupport_impl.cc:439: } Where is the "no codecs" case handled now?
https://codereview.chromium.org/254983006/diff/110001/content/renderer/render... File content/renderer/renderer_webkitplatformsupport_impl.cc (right): https://codereview.chromium.org/254983006/diff/110001/content/renderer/render... content/renderer/renderer_webkitplatformsupport_impl.cc:422: net::ParseCodecString(ToASCIIOrEmpty(codecs), &strict_codecs, true); On 2014/05/16 17:26:24, ddorwin wrote: > On 2014/05/16 11:25:29, amogh.bihani wrote: > > ddorwin@ > > After adding mp4 containers in strict media mime type list, I had to remove > this > > condition as EME does not consider extended codecs[1]. > > However, I think we do not need to change any thing there. When > > IsSupportedKeySystemWithMediaMimeType() returns true, the code has to go > through > > line 437. Here the logic is implemented, so from the overall browser's > > perspective, we might end up executing same commands twice. > > > > Is it fine to keep that part as is or do we need to make changes there as > well? > > If yes, can we do that in another CL? I am not much familiar with that part > and > > might need some time. > > ---------- > > [1] > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m... > > This will allow "vp8.1", which it shouldn't. > > EME should handle extended codecs in the same way as the rest of Chromium. This > specific code will eventually be removed, so it's not worth addressing now, but > we need to fix the permanent code elsewhere. Please file a bug. > > Also, please add a test for "vp8.1" that demonstrates this failure and reference > the bug. (If we don't have a non-EME test for "vp8.1", we should add that too. Well the test for vp8.1 will not pass as we next condition, i.e. IsSupportedStrictMediaMimeType will return IsNotSupported. So, IsSupportedKeySystemWithMediaMimeType is temporary or it being handled here is temporary? https://codereview.chromium.org/254983006/diff/110001/content/renderer/render... content/renderer/renderer_webkitplatformsupport_impl.cc:439: } On 2014/05/16 17:26:24, ddorwin wrote: > Where is the "no codecs" case handled now? That has gone in mime_util.cc line 820 of this patch.
https://codereview.chromium.org/254983006/diff/110001/content/renderer/render... File content/renderer/renderer_webkitplatformsupport_impl.cc (right): https://codereview.chromium.org/254983006/diff/110001/content/renderer/render... content/renderer/renderer_webkitplatformsupport_impl.cc:422: net::ParseCodecString(ToASCIIOrEmpty(codecs), &strict_codecs, true); On 2014/05/16 18:13:51, AmoghBihani wrote: > On 2014/05/16 17:26:24, ddorwin wrote: > > On 2014/05/16 11:25:29, amogh.bihani wrote: > > > ddorwin@ > > > After adding mp4 containers in strict media mime type list, I had to remove > > this > > > condition as EME does not consider extended codecs[1]. > > > However, I think we do not need to change any thing there. When > > > IsSupportedKeySystemWithMediaMimeType() returns true, the code has to go > > through > > > line 437. Here the logic is implemented, so from the overall browser's > > > perspective, we might end up executing same commands twice. > > > > > > Is it fine to keep that part as is or do we need to make changes there as > > well? > > > If yes, can we do that in another CL? I am not much familiar with that part > > and > > > might need some time. > > > ---------- > > > [1] > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m... > > > > This will allow "vp8.1", which it shouldn't. > > > > EME should handle extended codecs in the same way as the rest of Chromium. > This > > specific code will eventually be removed, so it's not worth addressing now, > but > > we need to fix the permanent code elsewhere. Please file a bug. > > > > Also, please add a test for "vp8.1" that demonstrates this failure and > reference > > the bug. (If we don't have a non-EME test for "vp8.1", we should add that too. > > Well the test for vp8.1 will not pass as we next condition, i.e. > IsSupportedStrictMediaMimeType will return IsNotSupported. > > So, IsSupportedKeySystemWithMediaMimeType is temporary or it being handled here > is temporary? Okay. It would be good to add tests to make sure we don't regress. :) IsSupportedKeySystemWithMediaMimeType() will live on, but it will be removed from here. The permanent call is in supportsEncryptedMediaMIMEType() at line 485. https://codereview.chromium.org/254983006/diff/110001/net/base/mime_util.cc File net/base/mime_util.cc (right): https://codereview.chromium.org/254983006/diff/110001/net/base/mime_util.cc#n... net/base/mime_util.cc:506: for (size_t i = 0; i < codecs.size(); ++i) { If the above function checks supported_codecs.empty() first, this one should too for consistency. https://codereview.chromium.org/254983006/diff/110001/net/base/mime_util.cc#n... net/base/mime_util.cc:813: StrictExpressionMappings::const_iterator iter = I don't think "it" and "iter" should be used in the same function. https://codereview.chromium.org/254983006/diff/110001/net/base/mime_util.cc#n... net/base/mime_util.cc:820: if (codecs.empty()) Ideally, this should bail early, but we also have to make sure the MIME type is found. It's concerning that we call AreSupported*Codecs without knowing whether it is empty. Specifically, at 810, it's not clear whether we might return the wrong value depending on how AreSupportedCodecs() handles the empty vector.
https://codereview.chromium.org/254983006/diff/110001/net/base/mime_util.cc File net/base/mime_util.cc (right): https://codereview.chromium.org/254983006/diff/110001/net/base/mime_util.cc#n... net/base/mime_util.cc:506: for (size_t i = 0; i < codecs.size(); ++i) { On 2014/05/16 18:49:49, ddorwin wrote: > If the above function checks supported_codecs.empty() first, this one should too > for consistency. This method is called only for MP4 and HLS containers. Supported codecs will never be empty(line 471-484. Adding this check might introduce dead code. The supported_codecs.empty() check was added for mp3 containers which do not take any codec parameter. You can view the change in https://codereview.chromium.org/228823003/diff/100001/net/base/mime_util.cc?c... https://codereview.chromium.org/254983006/diff/110001/net/base/mime_util.cc#n... net/base/mime_util.cc:820: if (codecs.empty()) On 2014/05/16 18:49:49, ddorwin wrote: > Ideally, this should bail early, but we also have to make sure the MIME type is > found. > > It's concerning that we call AreSupported*Codecs without knowing whether it is > empty. Specifically, at 810, it's not clear whether we might return the wrong > value depending on how AreSupportedCodecs() handles the empty vector. This was done specifically because mp3 containers do not expect any codec parameter. If we check this condition first then queries for audio/mp3 will return "maybe" although they must return "probably". You can view the change in https://codereview.chromium.org/228823003/diff/100001/content/renderer/render... AreSupportedCodecs handles this automatically. line 493 checks for the above mentioned scenario. In other cases, loop on line 496 won't be executed and line 500 will take care of the rest.
ddrowin@ I have filed the bug as you had asked. The bug id is 374751.
acolwell@ I have made changes in IsSupportedStrictMediaMimeType to return enum. ddorwin@ I have added test cases for wrong extended codecs like "vp8.1". PTAL.
I'm concerned about the structure of the code and the non-obvious corner cases. (Thankfully, we now have tests!) Is there a better way to express these things? Maybe we should be able to specify whether a MIME type supports codecs and whether it is strict. Then we could process them in a single function. https://codereview.chromium.org/254983006/diff/110001/net/base/mime_util.cc File net/base/mime_util.cc (right): https://codereview.chromium.org/254983006/diff/110001/net/base/mime_util.cc#n... net/base/mime_util.cc:506: for (size_t i = 0; i < codecs.size(); ++i) { On 2014/05/16 19:18:01, AmoghBihani wrote: > On 2014/05/16 18:49:49, ddorwin wrote: > > If the above function checks supported_codecs.empty() first, this one should > too > > for consistency. > > This method is called only for MP4 and HLS containers. Supported codecs will > never be empty(line 471-484. Adding this check might introduce dead code. > > The supported_codecs.empty() check was added for mp3 containers which do not > take any codec parameter. > You can view the change in > https://codereview.chromium.org/228823003/diff/100001/net/base/mime_util.cc?c... Then I recommend DCHECKing this condition. Also, line 534 handles the empty case. https://codereview.chromium.org/254983006/diff/130001/chrome/browser/media/en... File chrome/browser/media/encrypted_media_istypesupported_browsertest.cc (right): https://codereview.chromium.org/254983006/diff/130001/chrome/browser/media/en... chrome/browser/media/encrypted_media_istypesupported_browsertest.cc:140: wrong_extended_codec_.push_back("vp8.1"); Thanks. I believe we are specifically testing checking of VPx or non-MP4 extended codecs. Thus, we should probably name it vp8_invalid_extension_codec_ or something like that. https://codereview.chromium.org/254983006/diff/130001/content/renderer/render... File content/renderer/renderer_webkitplatformsupport_impl.cc (right): https://codereview.chromium.org/254983006/diff/130001/content/renderer/render... content/renderer/renderer_webkitplatformsupport_impl.cc:444: net::IsSupportedStrictMediaMimeType(mime_type_ascii,strict_codecs)); nit: space after ',' https://codereview.chromium.org/254983006/diff/130001/net/base/mime_util.cc File net/base/mime_util.cc (right): https://codereview.chromium.org/254983006/diff/130001/net/base/mime_util.cc#n... net/base/mime_util.cc:506: for (size_t i = 0; i < codecs.size(); ++i) { See comments in previous patchset. https://codereview.chromium.org/254983006/diff/130001/net/base/mime_util.cc#n... net/base/mime_util.cc:813: StrictExpressionMappings::const_iterator iter = I don't think "it" and "iter" should be used in the same function.
"Is there a better way to express these things?
Maybe we should be able to specify whether a MIME type supports codecs and
whether it is strict. Then we could process them in a single function."
I tried to do something of this sort in my previous CL ('bool
MimeTypeDoesNotNeedCodecs'), when Ryan advised me to change the code structure
in renderer_webkitplatformsupport_impl to avoid it.[1] (last part of his second
comment)
"Alternatively, it would seem you could change how webkitplatformsupport is
structured and avoid the issue entirely
if (net::IsStrictMediaMimeType(...)) {
if (net::IsSupportedStrictMediaMimeType(...))
return IsSupported;
if (codecs.isNull())
return MayBeSupported;
return IsNotSupported;
}"
--------
[1]
https://codereview.chromium.org/228823003/diff/1/net/base/mime_util.h#newcode89
https://codereview.chromium.org/254983006/diff/130001/chrome/browser/media/en...
File chrome/browser/media/encrypted_media_istypesupported_browsertest.cc
(right):
https://codereview.chromium.org/254983006/diff/130001/chrome/browser/media/en...
chrome/browser/media/encrypted_media_istypesupported_browsertest.cc:140:
wrong_extended_codec_.push_back("vp8.1");
On 2014/05/20 17:47:27, ddorwin wrote:
> Thanks. I believe we are specifically testing checking of VPx or non-MP4
> extended codecs. Thus, we should probably name it vp8_invalid_extension_codec_
> or something like that.
Done.
https://codereview.chromium.org/254983006/diff/130001/content/renderer/render...
File content/renderer/renderer_webkitplatformsupport_impl.cc (right):
https://codereview.chromium.org/254983006/diff/130001/content/renderer/render...
content/renderer/renderer_webkitplatformsupport_impl.cc:444:
net::IsSupportedStrictMediaMimeType(mime_type_ascii,strict_codecs));
On 2014/05/20 17:47:27, ddorwin wrote:
> nit: space after ','
Done.
https://codereview.chromium.org/254983006/diff/130001/net/base/mime_util.cc
File net/base/mime_util.cc (right):
https://codereview.chromium.org/254983006/diff/130001/net/base/mime_util.cc#n...
net/base/mime_util.cc:506: for (size_t i = 0; i < codecs.size(); ++i) {
On 2014/05/20 17:47:27, ddorwin wrote:
> See comments in previous patchset.
Done.
https://codereview.chromium.org/254983006/diff/130001/net/base/mime_util.cc#n...
net/base/mime_util.cc:813: StrictExpressionMappings::const_iterator iter =
On 2014/05/20 17:47:27, ddorwin wrote:
> I don't think "it" and "iter" should be used in the same function.
Done.
Ping!
On 2014/05/22 13:36:47, amogh.bihani wrote: > Ping! I am waiting to review until you gave addressed the media team concerns
On 2014/05/22 15:52:03, Ryan Sleevi wrote: > On 2014/05/22 13:36:47, amogh.bihani wrote: > > Ping! > > I am waiting to review until you gave addressed the media team concerns Tips: When pinging, please specify who it is directed at. Also, it helps if you add OWNERS for other parts of the codebase (e.g. net/) after getting LGTM from the main reviewer(s). I've reviewed the encrypted test changes and provided my comments on some other things. acolwell/scherkus should weigh in on the general canPlayType() design.
On 2014/05/22 17:06:32, ddorwin wrote: > On 2014/05/22 15:52:03, Ryan Sleevi wrote: > > On 2014/05/22 13:36:47, amogh.bihani wrote: > > > Ping! > > > > I am waiting to review until you gave addressed the media team concerns > > Tips: When pinging, please specify who it is directed at. Also, it helps if you > add OWNERS for other parts of the codebase (e.g. net/) after getting LGTM from > the main reviewer(s). > > I've reviewed the encrypted test changes and provided my comments on some other > things. acolwell/scherkus should weigh in on the general canPlayType() design. Oops, also, don't put those four letters together unless you mean to. For now, not LGTM.
> Tips: When pinging, please specify who it is directed at. Also, it helps if you > add OWNERS for other parts of the codebase (e.g. net/) after getting LGTM from > the main reviewer(s). Oh! Ill take care of it from next time :) acolwell@ please take a look at the new design.
https://codereview.chromium.org/254983006/diff/150001/content/browser/media/m... File content/browser/media/media_canplaytype_browsertest.cc (right): https://codereview.chromium.org/254983006/diff/150001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:498: EXPECT_EQ(kPropMaybe, CanPlay("'video/mp4; codecs=\"mp4a\"'")); Why is this one maybe? mp4a is not RFC compliant. https://codereview.chromium.org/254983006/diff/150001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:567: EXPECT_EQ(kPropMaybe, CanPlay("'video/x-m4v; codecs=\"mp4a\"'")); ditto https://codereview.chromium.org/254983006/diff/150001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:633: EXPECT_EQ(kPropMaybe, CanPlay("'audio/mp4; codecs=\"mp4a\"'")); ditto https://codereview.chromium.org/254983006/diff/150001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:698: EXPECT_EQ(kPropMaybe, CanPlay("'audio/x-m4a; codecs=\"mp4a\"'")); ditto https://codereview.chromium.org/254983006/diff/150001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:765: std::string HLSMaybe = ""; nit: Use kNot here. Also how about s/HLSMaybe/canPlayHLS/ since this variable is only "maybe" for certain Android builds? https://codereview.chromium.org/254983006/diff/150001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:768: HLSMaybe = "maybe"; nit: Use kMaybe here. https://codereview.chromium.org/254983006/diff/150001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:851: CanPlay("'application/vnd.apple.mpegurl; codecs=\"mp4a\"'")); ditto https://codereview.chromium.org/254983006/diff/150001/net/base/mime_util.cc File net/base/mime_util.cc (right): https://codereview.chromium.org/254983006/diff/150001/net/base/mime_util.cc#n... net/base/mime_util.cc:472: "mp4a,mp4a.40,mp4a.67,mp4a.40.?,mp4a.67.?"; mp4a is not RFC compliant. https://codereview.chromium.org/254983006/diff/150001/net/base/mime_util.cc#n... net/base/mime_util.cc:475: "mp4a,mp4a.40,mp4a.67,mp4a.40.?,mp4a.67.?"; mp4a is not RFC compliant. https://codereview.chromium.org/254983006/diff/150001/net/base/mime_util.cc#n... net/base/mime_util.cc:510: if (!MatchPattern(static_cast<base::StringPiece>(codecs[i]), nit: static_cast doesn't seem like the right thing here and below. I think you should probably just use the base::StringPiece constructor. It's even less characters. :) https://codereview.chromium.org/254983006/diff/150001/net/base/mime_util.cc#n... net/base/mime_util.cc:514: if (codecs[i].find('.') != std::string::npos) { You seem to be searching for '.' three different ways here. I believe you can just do the StringSplit here and run the other logic if split_string has a length > 0. '.' at the end case will result in split_string.back() being an empty string. Another alternative would be to just see if the matched pattern has a '?' and just verify that all string offsets in the pattern that contain a '?' are a valid hex digit in the codec string. That is actually what you are trying to verify here anyways and doesn't bake knowledge of the periods into this code. https://codereview.chromium.org/254983006/diff/150001/net/base/mime_util_unit... File net/base/mime_util_unittest.cc (right): https://codereview.chromium.org/254983006/diff/150001/net/base/mime_util_unit... net/base/mime_util_unittest.cc:121: EXPECT_TRUE(IsStrictMediaMimeType("application/x-mpegurl")); I would expect these to be only true for Android. We don't support HLS on desktop Chrome.
Thanks :) I have made the changes. acolwell@ - comments are in patchset 9 https://codereview.chromium.org/254983006/diff/150001/net/base/mime_util.cc File net/base/mime_util.cc (right): https://codereview.chromium.org/254983006/diff/150001/net/base/mime_util.cc#n... net/base/mime_util.cc:514: if (codecs[i].find('.') != std::string::npos) { On 2014/05/22 20:21:42, acolwell wrote: > You seem to be searching for '.' three different ways here. I believe you can > just do the StringSplit here and run the other logic if split_string has a > length > 0. '.' at the end case will result in split_string.back() being an > empty string. > > Another alternative would be to just see if the matched pattern has a '?' and > just verify that all string offsets in the pattern that contain a '?' are a > valid hex digit in the codec string. That is actually what you are trying to > verify here anyways and doesn't bake knowledge of the periods into this code. Wow, the second approach is awesome. :) https://codereview.chromium.org/254983006/diff/150001/net/base/mime_util_unit... File net/base/mime_util_unittest.cc (right): https://codereview.chromium.org/254983006/diff/150001/net/base/mime_util_unit... net/base/mime_util_unittest.cc:121: EXPECT_TRUE(IsStrictMediaMimeType("application/x-mpegurl")); On 2014/05/22 20:21:42, acolwell wrote: > I would expect these to be only true for Android. We don't support HLS on > desktop Chrome. Yes HLS is supported only on Android, but that would be true from the complete browser's perspective. In browser we first check whether the mime_type is supported or not[1] and then move forward. This method comes after the check, hence we do not put #ifdef in its list. Since all the mime types are available in the list, this unittest is supposed to pass on all the platforms. (You may notice that in this test, video/ogg is passing, even for android, because of this reason.) However, the media_canplaytype_browsertest should fail (and it does). In this all the HLS tests return "" for canPlayType() query for non android platforms (line 765-768 in that file of this patchset). ------- [1] https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r...
https://codereview.chromium.org/254983006/diff/190001/chrome/browser/media/en... File chrome/browser/media/encrypted_media_istypesupported_browsertest.cc (right): https://codereview.chromium.org/254983006/diff/190001/chrome/browser/media/en... chrome/browser/media/encrypted_media_istypesupported_browsertest.cc:127: aac_codec_.push_back("mp4a.40"); nit: Why is this called aac_codec_? mp4a.40 does not refer to AAC. You must have mp4a.40.2 or mp4a.40.5 to represent AAC. Please change the string to one of these if you are actually trying to test AAC. Otherwise use a different name. https://codereview.chromium.org/254983006/diff/190001/chrome/browser/media/en... chrome/browser/media/encrypted_media_istypesupported_browsertest.cc:130: avc1_and_aac_codecs_.push_back("mp4a.40"); ditto. https://codereview.chromium.org/254983006/diff/190001/chrome/browser/media/en... chrome/browser/media/encrypted_media_istypesupported_browsertest.cc:133: avc3_and_aac_codecs_.push_back("mp4a.40"); ditto. https://codereview.chromium.org/254983006/diff/190001/content/renderer/render... File content/renderer/renderer_webkitplatformsupport_impl.cc (right): https://codereview.chromium.org/254983006/diff/190001/content/renderer/render... content/renderer/renderer_webkitplatformsupport_impl.cc:443: return static_cast<WebMimeRegistry::SupportsType> ( You should add COMPILE_ASSERTS to content/public/common/assert_matching_enums.cc that verify that the WebMimeRegistry::SupportsType values always match the CanPlayType values. You might also consider renaming the CanPlayType enum since nothing at this level actually refers to canPlayType(). Having the same or a similar name to SupportsType would be better. https://codereview.chromium.org/254983006/diff/190001/net/base/mime_util.cc File net/base/mime_util.cc (right): https://codereview.chromium.org/254983006/diff/190001/net/base/mime_util.cc#n... net/base/mime_util.cc:472: "mp4a.40,mp4a.67,mp4a.40.?,mp4a.67.?"; nit: While mp4a.67.? is allowed by the RFC, it is not actually valid because mp4a.67 does not have sub-types like mp4a.40 does. mp4a.67.? should be removed from this list and the one below.
Thanks :) Comments are in ps#12 https://codereview.chromium.org/254983006/diff/190001/chrome/browser/media/en... File chrome/browser/media/encrypted_media_istypesupported_browsertest.cc (right): https://codereview.chromium.org/254983006/diff/190001/chrome/browser/media/en... chrome/browser/media/encrypted_media_istypesupported_browsertest.cc:127: aac_codec_.push_back("mp4a.40"); On 2014/06/02 21:22:42, acolwell wrote: > nit: Why is this called aac_codec_? mp4a.40 does not refer to AAC. You must have > mp4a.40.2 or mp4a.40.5 to represent AAC. Please change the string to one of > these if you are actually trying to test AAC. Otherwise use a different name. Yes, this is non-compliant, this is just to keep the variable name in sync with the other variable names, like avc1_codec_. In one of the messages in this patch[1], ddorwin@ had told me to add these extended tests in another CL. " I see how this makes sense. Maybe we should (in a separate CL) merge extended codecs since they are more relevant now." Since we have do not have 'mp4a' in kProprietaryAudioCodecsExpression in mime_util, these tests were failing. I added '.40' so that the tests may pass. I shall clean this up in the next CL. :) Or should I name it 'mpeg_audio_codec_' here itself? --------- [1] https://codereview.chromium.org/254983006/#msg5 https://codereview.chromium.org/254983006/diff/190001/content/renderer/render... File content/renderer/renderer_webkitplatformsupport_impl.cc (right): https://codereview.chromium.org/254983006/diff/190001/content/renderer/render... content/renderer/renderer_webkitplatformsupport_impl.cc:443: return static_cast<WebMimeRegistry::SupportsType> ( On 2014/06/02 21:22:42, acolwell wrote: > You should add COMPILE_ASSERTS to content/public/common/assert_matching_enums.cc > that verify that the WebMimeRegistry::SupportsType values always match the > CanPlayType values. You might also consider renaming the CanPlayType enum since > nothing at this level actually refers to canPlayType(). Having the same or a > similar name to SupportsType would be better. Done. https://codereview.chromium.org/254983006/diff/190001/net/base/mime_util.cc File net/base/mime_util.cc (right): https://codereview.chromium.org/254983006/diff/190001/net/base/mime_util.cc#n... net/base/mime_util.cc:472: "mp4a.40,mp4a.67,mp4a.40.?,mp4a.67.?"; On 2014/06/02 21:22:42, acolwell wrote: > nit: While mp4a.67.? is allowed by the RFC, it is not actually valid because > mp4a.67 does not have sub-types like mp4a.40 does. mp4a.67.? should be removed > from this list and the one below. Done. And added commets regarding what are the RFC compliant values. https://codereview.chromium.org/254983006/diff/210001/chrome/browser/media/en... File chrome/browser/media/encrypted_media_istypesupported_browsertest.cc (right): https://codereview.chromium.org/254983006/diff/210001/chrome/browser/media/en... chrome/browser/media/encrypted_media_istypesupported_browsertest.cc:127: aac_codec_.push_back("mp4a.40"); comment in previous patchset
On 2014/06/03 13:26:11, amogh.bihani wrote: > Thanks :) > Comments are in ps#12 Ah! sorry, comments in ps#11
https://codereview.chromium.org/254983006/diff/210001/net/base/mime_util.cc File net/base/mime_util.cc (right): https://codereview.chromium.org/254983006/diff/210001/net/base/mime_util.cc#n... net/base/mime_util.cc:490: **/ Format this comment like the rest of the file - use //, not /* */ https://codereview.chromium.org/254983006/diff/210001/net/base/mime_util.cc#n... net/base/mime_util.cc:491: static const char* kProprietaryAudioCodecsExpression = Use "static const char kProprietary...[]" rather than "static const char*". Ditto line 493 https://codereview.chromium.org/254983006/diff/210001/net/base/mime_util.cc#n... net/base/mime_util.cc:631: } No braces here, like the rest of the file.
https://codereview.chromium.org/254983006/diff/230001/chrome/browser/media/en... File chrome/browser/media/encrypted_media_istypesupported_browsertest.cc (right): https://codereview.chromium.org/254983006/diff/230001/chrome/browser/media/en... chrome/browser/media/encrypted_media_istypesupported_browsertest.cc:127: aac_codec_.push_back("mp4a.40"); acolwell@, reason for still keeping this name in PS#11. https://codereview.chromium.org/254983006/diff/230001/content/public/common/a... File content/public/common/assert_matching_enums.cc (right): https://codereview.chromium.org/254983006/diff/230001/content/public/common/a... content/public/common/assert_matching_enums.cc:37: // SupportsType I feel these should be in a similar file in net/ instead of content/ rsleevi@, acolwell@ ?
acolwell@ Ping!
https://codereview.chromium.org/254983006/diff/230001/content/public/common/a... File content/public/common/assert_matching_enums.cc (right): https://codereview.chromium.org/254983006/diff/230001/content/public/common/a... content/public/common/assert_matching_enums.cc:37: // SupportsType On 2014/06/04 06:51:22, amogh.bihani wrote: > I feel these should be in a similar file in net/ instead of content/ > rsleevi@, acolwell@ ? Ah! Got my mistake. Nevermind.
lgtm % comments https://codereview.chromium.org/254983006/diff/190001/chrome/browser/media/en... File chrome/browser/media/encrypted_media_istypesupported_browsertest.cc (right): https://codereview.chromium.org/254983006/diff/190001/chrome/browser/media/en... chrome/browser/media/encrypted_media_istypesupported_browsertest.cc:127: aac_codec_.push_back("mp4a.40"); On 2014/06/03 13:26:12, amogh.bihani wrote: > On 2014/06/02 21:22:42, acolwell wrote: > > nit: Why is this called aac_codec_? mp4a.40 does not refer to AAC. You must > have > > mp4a.40.2 or mp4a.40.5 to represent AAC. Please change the string to one of > > these if you are actually trying to test AAC. Otherwise use a different name. > > Yes, this is non-compliant, this is just to keep the variable name in sync with > the other variable names, like avc1_codec_. It doesn't make sense to keep the variable names consistant if the names don't actually point to data that makes sense with those names. > > In one of the messages in this patch[1], ddorwin@ had told me to add these > extended tests in another CL. > " I see how this makes sense. Maybe we should (in a separate CL) merge > extended codecs since they are more relevant now." > > Since we have do not have 'mp4a' in kProprietaryAudioCodecsExpression in > mime_util, these tests were failing. I added '.40' so that the tests may pass. I > shall clean this up in the next CL. :) If I understand you correctly, mp4a.40.2 should also pass the tests. I'm suggesting that you use mp4a.40.2 instead because that actually refers to AAC content which would make the variable name and the actual data you are putting in that variable consistent. > > Or should I name it 'mpeg_audio_codec_' here itself? > > --------- > [1] https://codereview.chromium.org/254983006/#msg5 https://codereview.chromium.org/254983006/diff/230001/chrome/browser/media/en... File chrome/browser/media/encrypted_media_istypesupported_browsertest.cc (right): https://codereview.chromium.org/254983006/diff/230001/chrome/browser/media/en... chrome/browser/media/encrypted_media_istypesupported_browsertest.cc:127: aac_codec_.push_back("mp4a.40"); On 2014/06/04 06:51:22, amogh.bihani wrote: > acolwell@, reason for still keeping this name in PS#11. Based on your comment in PS#11 I still think you should use "mp4a.40.2" here so that you are actually putting a codec ID that represent AAC content in the aac_codec_ variable. https://codereview.chromium.org/254983006/diff/230001/chrome/browser/media/en... chrome/browser/media/encrypted_media_istypesupported_browsertest.cc:133: avc3_and_aac_codecs_.push_back("mp4a.40"); ditto
acolwell@ - done the changes. ddorwin@, rsleevi@, avi@ - PTAL. https://codereview.chromium.org/254983006/diff/230001/chrome/browser/media/en... File chrome/browser/media/encrypted_media_istypesupported_browsertest.cc (right): https://codereview.chromium.org/254983006/diff/230001/chrome/browser/media/en... chrome/browser/media/encrypted_media_istypesupported_browsertest.cc:127: aac_codec_.push_back("mp4a.40"); On 2014/06/09 23:50:32, acolwell wrote: > On 2014/06/04 06:51:22, amogh.bihani wrote: > > acolwell@, reason for still keeping this name in PS#11. > Based on your comment in PS#11 I still think you should use "mp4a.40.2" here so > that you are actually putting a codec ID that represent AAC content in the > aac_codec_ variable. Done.
content LGTM
https://codereview.chromium.org/254983006/diff/250001/chrome/browser/media/en... File chrome/browser/media/encrypted_media_istypesupported_browsertest.cc (right): https://codereview.chromium.org/254983006/diff/250001/chrome/browser/media/en... chrome/browser/media/encrypted_media_istypesupported_browsertest.cc:127: aac_codec_.push_back("mp4a.40.2"); aac without an extension is no longer tested. If that's still supported, we should probably revert this line and add aac_extended_codec. https://codereview.chromium.org/254983006/diff/250001/content/browser/media/m... File content/browser/media/media_canplaytype_browsertest.cc (right): https://codereview.chromium.org/254983006/diff/250001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:517: EXPECT_EQ(kNot, CanPlay("'video/mp4; codecs=\"mp4a\"'")); Unextended "mp4a" is no longer supported but "avc1" is? Why the difference? (I haven't been following the discussion.) Is this going to break websites?
https://codereview.chromium.org/254983006/diff/250001/chrome/browser/media/en... File chrome/browser/media/encrypted_media_istypesupported_browsertest.cc (right): https://codereview.chromium.org/254983006/diff/250001/chrome/browser/media/en... chrome/browser/media/encrypted_media_istypesupported_browsertest.cc:127: aac_codec_.push_back("mp4a.40.2"); mp4a is not supported as it is not RFC compliant. This is what Aaron said: "mp4a" is not rfc6381 compliant so this and all other tests that include it should be in the kNot category. https://codereview.chromium.org/254983006/#msg8
line 492 on https://codereview.chromium.org/254983006/diff/250001/net/base/mime_util.cc are allowed now.
https://codereview.chromium.org/254983006/diff/250001/net/base/mime_util.cc File net/base/mime_util.cc (right): https://codereview.chromium.org/254983006/diff/250001/net/base/mime_util.cc#n... net/base/mime_util.cc:108: static bool AreSupportedCodecsWithProfile( Document. https://codereview.chromium.org/254983006/diff/250001/net/base/mime_util.cc#n... net/base/mime_util.cc:127: StrictExpressionMappings strict_mp4_format_map_; Document https://codereview.chromium.org/254983006/diff/250001/net/base/mime_util.cc#n... net/base/mime_util.cc:488: // websites. Document a bit about what non-compliant codecs you're allowing. https://codereview.chromium.org/254983006/diff/250001/net/base/mime_util.cc#n... net/base/mime_util.cc:546: return false; This suggests that only one codec has to match, which runs counter to what I expected (as noted in the previous comment). Good to document :) https://codereview.chromium.org/254983006/diff/250001/net/base/mime_util.h File net/base/mime_util.h (right): https://codereview.chromium.org/254983006/diff/250001/net/base/mime_util.h#ne... net/base/mime_util.h:97: enum SupportsType { IsNotSupported, IsSupported, MayBeSupported }; 1) Have you run git cl format on this? 2) Document That is, as it reads now (and with the documentation below), this has the smell of the infamous tri-state bool ( http://thedailywtf.com/Articles/What_Is_Truth_0x3f_.aspx ) Knowing that I'm not familiar with this exact details of media codecs, I have to defer to the media/ OWNERs to make sure this is correct to them, but I would suspect that: // Indicates whether or not a given MIME type (and possible codec string) are supported by the underlying platform. enum SupportsType { // The underlying platform is known to support this MIME type. IsSupported, // The underlying platform is believed to support this MIME type, // but there may be additional limitations (eg: for some codecs, // only particular codec parameters, such as profiles, may be // supported). MayBeSupported, // The platform is known not to support the requested configuration (e.g. if an unsupported MIME type or codec is supplied) }; https://codereview.chromium.org/254983006/diff/250001/net/base/mime_util.h#ne... net/base/mime_util.h:103: // * Returns MayBeSupported if codecs paramter is empty or has a profile typo: paramter https://codereview.chromium.org/254983006/diff/250001/net/base/mime_util.h#ne... net/base/mime_util.h:105: // * Returns IsNotSupported if none of the aforementioned conditions are met. With the above comment for SupportsType, let's build out this comment // Checks the |mime_type| and |codecs| against the MIME types known // to support only a particular subset of codecs/parameters. // * Returns IsSupported if the |mime_type| is supported and supports // all of the codecs within |codecs| // * Returns MayBeSupported if the |mime_type| is supported and is // known to support only a subset of codecs, but |codecs| was empty. // Also returned if a codec is supported, but additional codec // parameters were supplied (such as a profile) for which support // cannot be decided. // * Returns IsNotSupported in all other cases. That said, as I read both your comment and the attempted reword, what happens if |codecs| contains (X.profile and Y), where Y is not supported, but X.profile is? MayBeSupported or IsNotSupported? My *guess* is that it's IsNotSupported - that is, MayBeSupported is only returned if *all* codecs in |codec| are supported, but at least one codec has a parameter which support cannot be conclusively determined.
https://codereview.chromium.org/254983006/diff/250001/chrome/browser/media/en... File chrome/browser/media/encrypted_media_istypesupported_browsertest.cc (right): https://codereview.chromium.org/254983006/diff/250001/chrome/browser/media/en... chrome/browser/media/encrypted_media_istypesupported_browsertest.cc:115: avc1_codec_.push_back("avc1"); It's odd that the video codecs are not extended unless in the variable name, but the audio codec is. If the avc* codecs _should_ be extended, add a TODO to fix the naming (avc3_extended_codec becomes avc3_codec and the others become something like "no_extension". https://codereview.chromium.org/254983006/diff/250001/chrome/browser/media/en... chrome/browser/media/encrypted_media_istypesupported_browsertest.cc:127: aac_codec_.push_back("mp4a.40.2"); On 2014/06/10 05:55:57, amogh.bihani wrote: > mp4a is not supported as it is not RFC compliant. > This is what Aaron said: > "mp4a" is not rfc6381 compliant so this and all other tests that include it > should be in the kNot category. > > https://codereview.chromium.org/254983006/#msg8 At a minimum, I think we should have a test for "mp4a" - so we confirm its success or failure. You can name it mp4a_invalid_no_extension or something to be clear, and it only needs to be tested a few times. Note that it is more acceptable to add this limitation for EME (assuming it doesn't break the handful of sites using it) than for general canPlayType(). https://codereview.chromium.org/254983006/diff/250001/net/base/mime_util.cc File net/base/mime_util.cc (right): https://codereview.chromium.org/254983006/diff/250001/net/base/mime_util.cc#n... net/base/mime_util.cc:487: // However some non-RFC compliant codecs are allowed so as to not break some Why is mp4a not one of those? What other non-compliant strings have been disallowed? I'm concerned that this CL, while improving compliance, might break websites like a previous one did (https://code.google.com/p/chromium/issues/detail?id=375892#c1). We might want to be conservative in what is disallowed in this CL and collect UMAs to determine whether it's safe to remove support for individual non-compliant values.
ddorwin@ I have addressed the comments regarding why avc1 and not mp4a in PS#14. (I will address the rest of the comments in next PS.) https://codereview.chromium.org/254983006/diff/250001/chrome/browser/media/en... File chrome/browser/media/encrypted_media_istypesupported_browsertest.cc (right): https://codereview.chromium.org/254983006/diff/250001/chrome/browser/media/en... chrome/browser/media/encrypted_media_istypesupported_browsertest.cc:115: avc1_codec_.push_back("avc1"); avc1/avc3 are RFC compliant and can be followed by profile parameter extension but mp4a is not RFC compliant and must be followed by extension. Here is what Aaron had told. https://codereview.chromium.org/216893002/#msg5 https://codereview.chromium.org/254983006/diff/250001/content/browser/media/m... File content/browser/media/media_canplaytype_browsertest.cc (right): https://codereview.chromium.org/254983006/diff/250001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:517: EXPECT_EQ(kNot, CanPlay("'video/mp4; codecs=\"mp4a\"'")); On 2014/06/10 05:41:05, ddorwin wrote: > Unextended "mp4a" is no longer supported but "avc1" is? Why the difference? (I > haven't been following the discussion.) Is this going to break websites? Sorry I missed this comment yesterday. avc1 is RFC compliant but mp4a is not. (https://codereview.chromium.org/216893002/#msg5)
Thanks for the review. I have made the changes. https://codereview.chromium.org/254983006/diff/250001/chrome/browser/media/en... File chrome/browser/media/encrypted_media_istypesupported_browsertest.cc (right): https://codereview.chromium.org/254983006/diff/250001/chrome/browser/media/en... chrome/browser/media/encrypted_media_istypesupported_browsertest.cc:127: aac_codec_.push_back("mp4a.40.2"); On 2014/06/11 00:46:09, ddorwin wrote: > On 2014/06/10 05:55:57, amogh.bihani wrote: > > mp4a is not supported as it is not RFC compliant. > > This is what Aaron said: > > "mp4a" is not rfc6381 compliant so this and all other tests that include it > > should be in the kNot category. > > > > https://codereview.chromium.org/254983006/#msg8 > > At a minimum, I think we should have a test for "mp4a" - so we confirm its > success or failure. You can name it mp4a_invalid_no_extension or something to be > clear, and it only needs to be tested a few times. > > Note that it is more acceptable to add this limitation for EME (assuming it > doesn't break the handful of sites using it) than for general canPlayType(). Done. https://codereview.chromium.org/254983006/diff/250001/net/base/mime_util.cc File net/base/mime_util.cc (right): https://codereview.chromium.org/254983006/diff/250001/net/base/mime_util.cc#n... net/base/mime_util.cc:108: static bool AreSupportedCodecsWithProfile( On 2014/06/10 19:15:43, Ryan Sleevi wrote: > Document. Done. https://codereview.chromium.org/254983006/diff/250001/net/base/mime_util.cc#n... net/base/mime_util.cc:127: StrictExpressionMappings strict_mp4_format_map_; On 2014/06/10 19:15:43, Ryan Sleevi wrote: > Document Done. https://codereview.chromium.org/254983006/diff/250001/net/base/mime_util.cc#n... net/base/mime_util.cc:546: return false; On 2014/06/10 19:15:43, Ryan Sleevi wrote: > This suggests that only one codec has to match, which runs counter to what I > expected (as noted in the previous comment). Good to document :) This returns false as soon as it finds a non-matching codec. Otherwise continues the outer for loop. https://codereview.chromium.org/254983006/diff/250001/net/base/mime_util.h File net/base/mime_util.h (right): https://codereview.chromium.org/254983006/diff/250001/net/base/mime_util.h#ne... net/base/mime_util.h:97: enum SupportsType { IsNotSupported, IsSupported, MayBeSupported }; On 2014/06/10 19:15:43, Ryan Sleevi wrote: > 1) Have you run git cl format on this? > 2) Document > > That is, as it reads now (and with the documentation below), this has the smell > of the infamous tri-state bool ( > http://thedailywtf.com/Articles/What_Is_Truth_0x3f_.aspx ) > > Knowing that I'm not familiar with this exact details of media codecs, I have to > defer to the media/ OWNERs to make sure this is correct to them, but I would > suspect that: > > // Indicates whether or not a given MIME type (and possible codec string) are > supported by the underlying platform. > enum SupportsType { > // The underlying platform is known to support this MIME type. > IsSupported, > // The underlying platform is believed to support this MIME type, > // but there may be additional limitations (eg: for some codecs, > // only particular codec parameters, such as profiles, may be > // supported). > MayBeSupported, > // The platform is known not to support the requested configuration (e.g. if > an unsupported MIME type or codec is supplied) > }; Done. https://codereview.chromium.org/254983006/diff/250001/net/base/mime_util.h#ne... net/base/mime_util.h:103: // * Returns MayBeSupported if codecs paramter is empty or has a profile On 2014/06/10 19:15:43, Ryan Sleevi wrote: > typo: paramter rectified. https://codereview.chromium.org/254983006/diff/250001/net/base/mime_util.h#ne... net/base/mime_util.h:105: // * Returns IsNotSupported if none of the aforementioned conditions are met. On 2014/06/10 19:15:43, Ryan Sleevi wrote: > With the above comment for SupportsType, let's build out this comment > > // Checks the |mime_type| and |codecs| against the MIME types known > // to support only a particular subset of codecs/parameters. > // * Returns IsSupported if the |mime_type| is supported and supports > // all of the codecs within |codecs| > // * Returns MayBeSupported if the |mime_type| is supported and is > // known to support only a subset of codecs, but |codecs| was empty. > // Also returned if a codec is supported, but additional codec > // parameters were supplied (such as a profile) for which support > // cannot be decided. > // * Returns IsNotSupported in all other cases. > > > That said, as I read both your comment and the attempted reword, what happens if > |codecs| contains (X.profile and Y), where Y is not supported, but X.profile is? > MayBeSupported or IsNotSupported? > > My *guess* is that it's IsNotSupported - that is, MayBeSupported is only > returned if *all* codecs in |codec| are supported, but at least one codec has a > parameter which support cannot be conclusively determined. Done.
Thanks for the pointers to the relevant discussion. encrypted_media_istypesupported_browsertest.cc LGTM with comments. https://codereview.chromium.org/254983006/diff/270001/chrome/browser/media/en... File chrome/browser/media/encrypted_media_istypesupported_browsertest.cc (right): https://codereview.chromium.org/254983006/diff/270001/chrome/browser/media/en... chrome/browser/media/encrypted_media_istypesupported_browsertest.cc:546: "video/mp4", mp4a_invalid_no_extension(), kPrefixedClearKey)); nit: I would put this after avc2. Same below x2. https://codereview.chromium.org/254983006/diff/270001/chrome/browser/media/en... chrome/browser/media/encrypted_media_istypesupported_browsertest.cc:567: EXPECT_FALSE(IsSupportedKeySystemWithMediaMimeType( nit: The comment at 564 does not really apply. Same below x2.
ddorwin@ just one question. (Please refer to comment.) https://codereview.chromium.org/254983006/diff/270001/chrome/browser/media/en... File chrome/browser/media/encrypted_media_istypesupported_browsertest.cc (right): https://codereview.chromium.org/254983006/diff/270001/chrome/browser/media/en... chrome/browser/media/encrypted_media_istypesupported_browsertest.cc:567: EXPECT_FALSE(IsSupportedKeySystemWithMediaMimeType( On 2014/06/11 22:33:23, ddorwin wrote: > nit: The comment at 564 does not really apply. > > Same below x2. But that would also apply for line 536. So should we put them under new comment, like // Invalid MP4 codecs or just remove the comment?
https://codereview.chromium.org/254983006/diff/270001/chrome/browser/media/en... File chrome/browser/media/encrypted_media_istypesupported_browsertest.cc (right): https://codereview.chromium.org/254983006/diff/270001/chrome/browser/media/en... chrome/browser/media/encrypted_media_istypesupported_browsertest.cc:567: EXPECT_FALSE(IsSupportedKeySystemWithMediaMimeType( On 2014/06/12 03:38:35, amogh.bihani wrote: > On 2014/06/11 22:33:23, ddorwin wrote: > > nit: The comment at 564 does not really apply. > > > > Same below x2. > > But that would also apply for line 536. > So should we put them under new comment, like > // Invalid MP4 codecs > or just remove the comment? How about this? // Invalid or non-MP4 codecs.
Thanks. I have made the changes. Ryan: Please see if the comments are ok.
https://codereview.chromium.org/254983006/diff/310001/net/base/mime_util.cc File net/base/mime_util.cc (right): https://codereview.chromium.org/254983006/diff/310001/net/base/mime_util.cc#n... net/base/mime_util.cc:481: // Following is the list of RFC compliant codecs: Which RFC(s)? Important to document in case it's updated and additional codecs are now compliant. https://codereview.chromium.org/254983006/diff/310001/net/base/mime_util.cc#n... net/base/mime_util.cc:493: // websites. Eg. mp4a.40 or avc1.123456. This comment - whiel helpful, doesn't help me understand why you have mp4a.6? - why not (6B, 69, 67) in your list? Is 63 a valid mp4a codec? You would seem to be treating it as such, but I have no context to understand whether that's intentional or a non-RFC compliant codec. // Additionally, several non-RFC compliant codecs are allowed, due to // their existing use on the web. // mp4a.40 // avc1 // avc1.xxxxxx // avc3 // avc3.xxxxxx https://codereview.chromium.org/254983006/diff/310001/net/base/mime_util.h File net/base/mime_util.h (right): https://codereview.chromium.org/254983006/diff/310001/net/base/mime_util.h#ne... net/base/mime_util.h:105: // actually trying to play it. Document these inline, as suggested // Comment describing SupportsType enum SupportsType { // Comment describing IsNotSupported IsNotSupported, // Comment describing IsSupported IsSupported, // Comment describing MayBeSupported MayBeSupported }; https://codereview.chromium.org/254983006/diff/310001/net/base/mime_util.h#ne... net/base/mime_util.h:119: // profile) for which the support cannot be decided. Still doesn't document what happens in the case of codecs { A, B.UnsupportedProfile, C } where A is supported, B MAY be supported, and C is not supported. Suggested resolution: "Also returned if all of the codecs listed in |codec| are supported, but additional codec parameters were supplied (such as a profile) for which support cannot be decided" https://codereview.chromium.org/254983006/diff/310001/net/base/mime_util.h#ne... net/base/mime_util.h:121: // |mime_type| is supported but atleast one of the codecs within |codecs| is s/atleast/at least/
rsleevi@ I have made the changes as suggested. PTAL. https://codereview.chromium.org/254983006/diff/310001/net/base/mime_util.cc File net/base/mime_util.cc (right): https://codereview.chromium.org/254983006/diff/310001/net/base/mime_util.cc#n... net/base/mime_util.cc:493: // websites. Eg. mp4a.40 or avc1.123456. we are allowing 6? because some websites might me using something like 6A etc and we do not want to break them. Also avc1 and avc3 are compliant[1], however, they do not tell anything about which profile to use. ---------- [1] https://codereview.chromium.org/216893002/#msg5
LGTM, mod one bug. https://codereview.chromium.org/254983006/diff/330001/net/base/mime_util.cc File net/base/mime_util.cc (right): https://codereview.chromium.org/254983006/diff/330001/net/base/mime_util.cc#n... net/base/mime_util.cc:545: wildcard_pos < supported_codecs[j].length(); BUG: if wildcard_pos is not found, it equals std::string::npos. You shouldn't make relationship assumptions about npos (e.g.: that it will be > .length()) If you're trying to do "not found", do != std::string::npos If you are trying to do "not terminal character", then compare != npos, then compare < .length()
The CQ bit was checked by amogh.bihani@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/amogh.bihani@samsung.com/254983006/350001
Message was sent while issue was closed.
Change committed as 277386 |
