Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
228823003:
Adding check for MIME types that do not take codecs parameter (Closed)
|
Created:
6 years, 8 months ago by amogh.bihani Modified:
6 years, 8 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org, cbentzel+watch_chromium.org, jam, darin-cc_chromium.org, ddorwin Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAdding check for MIME types that do not take codecs parameter
audio/mpeg, audio/mp3, audio/x-mp3 do not take any 'codecs' parameter and
canPlayType query must return "probably" of them. Browser in general returns
"maybe" when no 'codecs' parameter is present.
We need to check whether the mime type specifically wants 'codecs' parameter
to be absent.
BUG=53193
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=265577
Patch Set 1 #
Total comments: 14
Patch Set 2 : Changing comments #Patch Set 3 : #Patch Set 4 : removing unnecessary method #
Total comments: 4
Patch Set 5 : addressing comments #
Total comments: 2
Patch Set 6 : changing AreSupportedCodecs #
Messages
Total messages: 18 (0 generated)
PTAL. I have made changes in some of the mime types, as discussed in https://codereview.chromium.org/216893002/
https://codereview.chromium.org/228823003/diff/1/content/browser/media/media_... File content/browser/media/media_canplaytype_browsertest.cc (right): https://codereview.chromium.org/228823003/diff/1/content/browser/media/media_... content/browser/media/media_canplaytype_browsertest.cc:359: // audio/mpeg do not allow any codecs parameter nit: "do not" -> "does not" also add period to end of comments https://codereview.chromium.org/228823003/diff/1/content/browser/media/media_... content/browser/media/media_canplaytype_browsertest.cc:407: // audio/mp3 do not allow any codecs parameter ditto https://codereview.chromium.org/228823003/diff/1/content/browser/media/media_... content/browser/media/media_canplaytype_browsertest.cc:455: // audio/x-mp3 do not allow any codecs parameter ditto https://codereview.chromium.org/228823003/diff/1/net/base/mime_util_unittest.cc File net/base/mime_util_unittest.cc (right): https://codereview.chromium.org/228823003/diff/1/net/base/mime_util_unittest.... net/base/mime_util_unittest.cc:113: // be fixed with bug 53193 (http://crbug.com/53193)--------------------------- nit: you can just say "This will be fixed in http://crbug.com/53193" -- no need to duplicate yourself :)
https://codereview.chromium.org/228823003/diff/1/net/base/mime_util.h File net/base/mime_util.h (right): https://codereview.chromium.org/228823003/diff/1/net/base/mime_util.h#newcode89 net/base/mime_util.h:89: // not to be present with it. The comment here needs to be improved, but unfortunately, I'm not intimately familiar enough to know how to suggest improving it. My naive response was first just fixing the grammar, to something like // Check to see if a particular MIME type specifically requires the 'codecs' parameter be absent. But that doesn't describe what happens for mime types where no codecs are supported (say, application/x-microsoft-word or the like) Alternatively, it might be posed as "SupportedStrictMediaMimeTypeSupportsCodecs", which indicates if the given |mime_type| (which is a strict media mime type, presumably?) supports the "codecs" parameter. If it doesn't, returns false. However, when thinking about that, I'm not sure why you can't use IsSupportedStrictMediaMimeType(), which an empty "codecs" used to indicate no codecs parameter is supported. Is there a case where we would support a strict media mime type, but "codecs" would be empty AND "codecs" be a valid parameter? It doesn't seem like it. https://codereview.chromium.org/228823003/diff/1/net/base/mime_util_unittest.cc File net/base/mime_util_unittest.cc (right): https://codereview.chromium.org/228823003/diff/1/net/base/mime_util_unittest.... net/base/mime_util_unittest.cc:117: EXPECT_FALSE(IsStrictMediaMimeType("audio/x-m4a")); Seems like an unrelated change?
Thanks for the review :) Comments in patch set 1. https://codereview.chromium.org/228823003/diff/1/content/browser/media/media_... File content/browser/media/media_canplaytype_browsertest.cc (right): https://codereview.chromium.org/228823003/diff/1/content/browser/media/media_... content/browser/media/media_canplaytype_browsertest.cc:359: // audio/mpeg do not allow any codecs parameter On 2014/04/10 18:30:11, scherkus wrote: > nit: "do not" -> "does not" > > also add period to end of comments Done. https://codereview.chromium.org/228823003/diff/1/content/browser/media/media_... content/browser/media/media_canplaytype_browsertest.cc:407: // audio/mp3 do not allow any codecs parameter On 2014/04/10 18:30:11, scherkus wrote: > ditto Done. https://codereview.chromium.org/228823003/diff/1/content/browser/media/media_... content/browser/media/media_canplaytype_browsertest.cc:455: // audio/x-mp3 do not allow any codecs parameter On 2014/04/10 18:30:11, scherkus wrote: > ditto Done. https://codereview.chromium.org/228823003/diff/1/net/base/mime_util.h File net/base/mime_util.h (right): https://codereview.chromium.org/228823003/diff/1/net/base/mime_util.h#newcode89 net/base/mime_util.h:89: // not to be present with it. > However, when thinking about that, I'm not sure why you can't use > IsSupportedStrictMediaMimeType(), which an empty "codecs" used to indicate no > codecs parameter is supported. Is there a case where we would support a strict > media mime type, but "codecs" would be empty AND "codecs" be a valid parameter? > It doesn't seem like it. There are two things: [a] Browser will never reach the IsSupportedStrictMediaMimeType() if the 'codecs' is null[1]. It will return a "maybe" as soon as it sees a null 'codecs' parameter. [b] Strict mime type can have empty 'codecs' parameter, in that case we return a "maybe"[2]. That's why we need the check. ------- [1] https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r... [2] http://www.whatwg.org/specs/web-apps/current-work/multipage/the-video-element... https://codereview.chromium.org/228823003/diff/1/net/base/mime_util_unittest.cc File net/base/mime_util_unittest.cc (right): https://codereview.chromium.org/228823003/diff/1/net/base/mime_util_unittest.... net/base/mime_util_unittest.cc:113: // be fixed with bug 53193 (http://crbug.com/53193)--------------------------- On 2014/04/10 18:30:11, scherkus wrote: > nit: you can just say "This will be fixed in http://crbug.com/53193%22 -- no need > to duplicate yourself :) Done. https://codereview.chromium.org/228823003/diff/1/net/base/mime_util_unittest.... net/base/mime_util_unittest.cc:117: EXPECT_FALSE(IsStrictMediaMimeType("audio/x-m4a")); On 2014/04/10 19:04:16, Ryan Sleevi wrote: > Seems like an unrelated change? Yes it is an unrelated change. This test was added in my last CL[1] for fixing */ogg and audio/x-wav false positive results. I made a typo there. Since the expectation is false, the test passed and I did not notice it. I thought I should correct it here itself. Or should I create another CL for this? ------- [1] https://codereview.chromium.org/218303002/
https://codereview.chromium.org/228823003/diff/1/net/base/mime_util.h File net/base/mime_util.h (right): https://codereview.chromium.org/228823003/diff/1/net/base/mime_util.h#newcode89 net/base/mime_util.h:89: // not to be present with it. On 2014/04/11 05:24:07, amogh.bihani wrote: > > However, when thinking about that, I'm not sure why you can't use > > IsSupportedStrictMediaMimeType(), which an empty "codecs" used to indicate no > > codecs parameter is supported. Is there a case where we would support a strict > > media mime type, but "codecs" would be empty AND "codecs" be a valid > parameter? > > It doesn't seem like it. > > There are two things: > [a] Browser will never reach the IsSupportedStrictMediaMimeType() if the > 'codecs' is null[1]. It will return a "maybe" as soon as it sees a null 'codecs' > parameter. > > [b] Strict mime type can have empty 'codecs' parameter, in that case we return a > "maybe"[2]. > > That's why we need the check. > ------- > [1] > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r... > > [2] > http://www.whatwg.org/specs/web-apps/current-work/multipage/the-video-element... The naming here still bothers me, in that it doesn't seem the substance of the concern was addressed. Please understand, I'm not familiar with the mime-type rules - I'm deferring to scherkus and such here, but the fact that this code continues to "surprise" me (at least as far as function name/comment/implementation) is part of the concern. Again, using the examples above, if you pass some random mime type, like "application/x-microsoft-word", then I cannot reasonably guess what this function will return without _also_ looking into the implementation. That is, this function 'implies' it's authoritative for all mime types, but it's not - it only applies to the supported strict mime types. Minimally, something like DoesStrictMediaMimeTypeAllowEmptyCodecs() or something similar, so that someone reading the net/ code can understand. 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; }
Thanks for the review. :) I have made the changes. https://codereview.chromium.org/228823003/diff/1/net/base/mime_util.h File net/base/mime_util.h (right): https://codereview.chromium.org/228823003/diff/1/net/base/mime_util.h#newcode89 net/base/mime_util.h:89: // not to be present with it. On 2014/04/14 23:52:25, Ryan Sleevi wrote: > On 2014/04/11 05:24:07, amogh.bihani wrote: > > > However, when thinking about that, I'm not sure why you can't use > > > IsSupportedStrictMediaMimeType(), which an empty "codecs" used to indicate > no > > > codecs parameter is supported. Is there a case where we would support a > strict > > > media mime type, but "codecs" would be empty AND "codecs" be a valid > > parameter? > > > It doesn't seem like it. > > > > There are two things: > > [a] Browser will never reach the IsSupportedStrictMediaMimeType() if the > > 'codecs' is null[1]. It will return a "maybe" as soon as it sees a null > 'codecs' > > parameter. > > > > [b] Strict mime type can have empty 'codecs' parameter, in that case we return > a > > "maybe"[2]. > > > > That's why we need the check. > > ------- > > [1] > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r... > > > > [2] > > > http://www.whatwg.org/specs/web-apps/current-work/multipage/the-video-element... > > The naming here still bothers me, in that it doesn't seem the substance of the > concern was addressed. > > Please understand, I'm not familiar with the mime-type rules - I'm deferring to > scherkus and such here, but the fact that this code continues to "surprise" me > (at least as far as function name/comment/implementation) is part of the > concern. > > Again, using the examples above, if you pass some random mime type, like > "application/x-microsoft-word", then I cannot reasonably guess what this > function will return without _also_ looking into the implementation. > > That is, this function 'implies' it's authoritative for all mime types, but it's > not - it only applies to the supported strict mime types. > > Minimally, something like > > DoesStrictMediaMimeTypeAllowEmptyCodecs() or something similar, so that someone > reading the net/ code can understand. > > 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; > } Done. :)
few nits I'm OK w/ the current structure https://codereview.chromium.org/228823003/diff/60001/content/renderer/rendere... File content/renderer/renderer_webkitplatformsupport_impl.cc (right): https://codereview.chromium.org/228823003/diff/60001/content/renderer/rendere... content/renderer/renderer_webkitplatformsupport_impl.cc:445: // Not Supported this comment isn't very helpful as the code on the next line captures what's going on pretty well :) https://codereview.chromium.org/228823003/diff/60001/net/base/mime_util_unitt... File net/base/mime_util_unittest.cc (right): https://codereview.chromium.org/228823003/diff/60001/net/base/mime_util_unitt... net/base/mime_util_unittest.cc:112: // TODO(amogh.bihani):These will be fixed http://crbug.com/53193-------------- nit: space after : also just remove the extra trailing ----'s
Thanks for the review :) https://codereview.chromium.org/228823003/diff/60001/content/renderer/rendere... File content/renderer/renderer_webkitplatformsupport_impl.cc (right): https://codereview.chromium.org/228823003/diff/60001/content/renderer/rendere... content/renderer/renderer_webkitplatformsupport_impl.cc:445: // Not Supported On 2014/04/15 21:05:52, scherkus wrote: > this comment isn't very helpful as the code on the next line captures what's > going on pretty well :) Done. https://codereview.chromium.org/228823003/diff/60001/net/base/mime_util_unitt... File net/base/mime_util_unittest.cc (right): https://codereview.chromium.org/228823003/diff/60001/net/base/mime_util_unitt... net/base/mime_util_unittest.cc:112: // TODO(amogh.bihani):These will be fixed http://crbug.com/53193-------------- On 2014/04/15 21:05:52, scherkus wrote: > nit: space after : > > also just remove the extra trailing ----'s Done.
lgtm
https://codereview.chromium.org/228823003/diff/80001/content/renderer/rendere... File content/renderer/renderer_webkitplatformsupport_impl.cc (right): https://codereview.chromium.org/228823003/diff/80001/content/renderer/rendere... content/renderer/renderer_webkitplatformsupport_impl.cc:438: if (net::IsSupportedStrictMediaMimeType(mime_type_ascii, strict_codecs)) How does this work without you also requiring to change AreSupportedCodecs? ParseCodecString results in an empty strict_codecs net::IsSupportedStrictMediaMimeType finds "audio/mp3" within strict_format_map_, so it calls AreSupportedCodecs(), with the first parameter as an empty vector (as no supported codecs) and the second parameter as an empty vector (as it's the above strict_codecs parameter) AreSupportedCodecs() iterates through codecs (empty) to see if any of them are within supported_codecs - but this is a no-op. As a result, it hits the final condition - "return !codecs.empty()". However, codecs.empty() == true, !codecs.empty() == false, therefore AreSupportedCodecs() should return false, so shouldn't we be hidding line 441-443? Have you tested this code? Am I missing something? Is it creating a codecs_out with an empty string instead of an empty vector?
Thanks for the review. :) I have made the corresponding changes. https://codereview.chromium.org/228823003/diff/80001/content/renderer/rendere... File content/renderer/renderer_webkitplatformsupport_impl.cc (right): https://codereview.chromium.org/228823003/diff/80001/content/renderer/rendere... content/renderer/renderer_webkitplatformsupport_impl.cc:438: if (net::IsSupportedStrictMediaMimeType(mime_type_ascii, strict_codecs)) On 2014/04/18 01:57:41, Ryan Sleevi wrote: > How does this work without you also requiring to change AreSupportedCodecs? > > ParseCodecString results in an empty strict_codecs > > net::IsSupportedStrictMediaMimeType finds "audio/mp3" within strict_format_map_, > so it calls AreSupportedCodecs(), with the first parameter as an empty vector > (as no supported codecs) and the second parameter as an empty vector (as it's > the above strict_codecs parameter) > > AreSupportedCodecs() iterates through codecs (empty) to see if any of them are > within supported_codecs - but this is a no-op. As a result, it hits the final > condition - "return !codecs.empty()". However, codecs.empty() == true, > !codecs.empty() == false, therefore AreSupportedCodecs() should return false, so > shouldn't we be hidding line 441-443? > > Have you tested this code? Am I missing something? Is it creating a codecs_out > with an empty string instead of an empty vector? I'm sorry I missed this. I did test this code. The testcases were passing. But they were passing because USE_PROPRIETARY_CODECS was not enabled in my system. We cannot remove lines 441 to 443 as "maybe" is a valid response and we need to give this when no codecs are present for the containers which expect 'codecs' parameter.
lgtm
Avi@ PTAL.
lgtm Cool.
Thanks for the reviews :)
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/228823003/100001
Message was sent while issue was closed.
Change committed as 265577 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
