Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1624703002:
Implement support for vp9 in ISO-BMFF (Closed)
|
Created:
4 years, 11 months ago by kqyang Modified:
4 years, 8 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org, DaleCurtis, xhwang, tinskip1, fgalligan1 Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement support for vp9 in ISO-BMFF
The feature is implemented under flag ENABLE_MP4_VP9_DEMUXING, off for now.
BUG=580623
Committed: https://crrev.com/f66cf8a7b3e8f169a58c09e6627b0cdc11192d2f
Cr-Commit-Position: refs/heads/master@{#388363}
Patch Set 1 #
Total comments: 8
Patch Set 2 : #Patch Set 3 : #
Total comments: 36
Patch Set 4 : Address review comments #Patch Set 5 : Add a flag ENABLE_MP4_VP8_VP9_DEMUXING for this change. #
Total comments: 17
Patch Set 6 : #
Total comments: 17
Patch Set 7 : #
Total comments: 10
Patch Set 8 : #
Total comments: 11
Patch Set 9 : #
Total comments: 2
Patch Set 10 : #Patch Set 11 : #Patch Set 12 : #
Total comments: 7
Patch Set 13 : #
Total comments: 4
Patch Set 14 : #
Total comments: 12
Patch Set 15 : #Messages
Total messages: 52 (12 generated)
kqyang@chromium.org changed reviewers: + ddorwin@chromium.org, sandersd@chromium.org
Description was changed from ========== Implement support for vpx in ISO-BMFF BUG=580623 ========== to ========== Implement support for vpx in ISO-BMFF BUG=580623 ==========
A few comments based on our earlier discussion. It looks like this CL is moving MP4 out of proprietary_codecs, but it's not clear that that's the right move immediately since it will require reviewing a lot of other code. I assume the main reason to do this is to have better test coverage. Is that correct, and what alternatives do we have? https://codereview.chromium.org/1624703002/diff/1/chrome/renderer/media/chrom... File chrome/renderer/media/chrome_key_systems.cc (right): https://codereview.chromium.org/1624703002/diff/1/chrome/renderer/media/chrom... chrome/renderer/media/chrome_key_systems.cc:187: supported_codecs |= media::EME_CODEC_WEBM_VP8 | media::EME_CODEC_MP4_VP8; If we are going with leaving MP4 as requiring proprietary_codecs, then these should be restricted the same. https://codereview.chromium.org/1624703002/diff/1/media/base/eme_constants.h File media/base/eme_constants.h (right): https://codereview.chromium.org/1624703002/diff/1/media/base/eme_constants.h#... media/base/eme_constants.h:53: EME_CODEC_MP4_VP8 = 1 << 6, Similarly, it doesn't make sense to declare these if !PROPRIETARY_CODECS. https://codereview.chromium.org/1624703002/diff/1/media/base/key_systems.cc File media/base/key_systems.cc (left): https://codereview.chromium.org/1624703002/diff/1/media/base/key_systems.cc#o... media/base/key_systems.cc:46: #if defined(USE_PROPRIETARY_CODECS) Should be left in. https://codereview.chromium.org/1624703002/diff/1/media/base/key_systems.cc File media/base/key_systems.cc (right): https://codereview.chromium.org/1624703002/diff/1/media/base/key_systems.cc#n... media/base/key_systems.cc:56: {"vp08", EME_CODEC_MP4_VP8}, Is this the actual codec string? It's rather strange :-(. Will we need to update all of the codec string parsing in Chrome to distinguish the MP4 and WebM versions?
Thanks for the review. As discussed earlier, let's still keep all codecs in mp4 as proprietary for now. We can update the code in a later cl once we got confirmation on having mp4 in Chromium. PTAL https://codereview.chromium.org/1624703002/diff/1/chrome/renderer/media/chrom... File chrome/renderer/media/chrome_key_systems.cc (right): https://codereview.chromium.org/1624703002/diff/1/chrome/renderer/media/chrom... chrome/renderer/media/chrome_key_systems.cc:187: supported_codecs |= media::EME_CODEC_WEBM_VP8 | media::EME_CODEC_MP4_VP8; On 2016/01/26 21:21:20, sandersd wrote: > If we are going with leaving MP4 as requiring proprietary_codecs, then these > should be restricted the same. Done. https://codereview.chromium.org/1624703002/diff/1/media/base/eme_constants.h File media/base/eme_constants.h (right): https://codereview.chromium.org/1624703002/diff/1/media/base/eme_constants.h#... media/base/eme_constants.h:53: EME_CODEC_MP4_VP8 = 1 << 6, On 2016/01/26 21:21:20, sandersd wrote: > Similarly, it doesn't make sense to declare these if !PROPRIETARY_CODECS. Done. https://codereview.chromium.org/1624703002/diff/1/media/base/key_systems.cc File media/base/key_systems.cc (left): https://codereview.chromium.org/1624703002/diff/1/media/base/key_systems.cc#o... media/base/key_systems.cc:46: #if defined(USE_PROPRIETARY_CODECS) On 2016/01/26 21:21:20, sandersd wrote: > Should be left in. Done. https://codereview.chromium.org/1624703002/diff/1/media/base/key_systems.cc File media/base/key_systems.cc (right): https://codereview.chromium.org/1624703002/diff/1/media/base/key_systems.cc#n... media/base/key_systems.cc:56: {"vp08", EME_CODEC_MP4_VP8}, On 2016/01/26 21:21:20, sandersd wrote: > Is this the actual codec string? It's rather strange :-(. The codec strings are of the form "vp08.xxxxxxx" or "vp09.xxxxxx", see mime_util.cc or draft spec https://github.com/Netflix/vp9-dash/blob/master/Downloads/VPCodecISOMediaFile... for details. It uses 4 characters, to be consistent to other codec strings used in mp4, e.g. avc1.xxxx, mp4a.xxxx. I don't like having different codec strings for webm and mp4 too... We may want to consider supporting the mp4 style codec strings vp08.xxxx in webm too. Will we need to update > all of the codec string parsing in Chrome to distinguish the MP4 and WebM > versions? I think I have updated all relevant places. Let me know if I missed any.
lgtm https://codereview.chromium.org/1624703002/diff/40001/media/base/key_systems.cc File media/base/key_systems.cc (right): https://codereview.chromium.org/1624703002/diff/40001/media/base/key_systems.... media/base/key_systems.cc:58: {"vp08", EME_CODEC_MP4_VP8}, These are only defined when USE_PROPRIETARY_CODECS is set.
ddorwin@chromium.org changed reviewers: + wolenetz@chromium.org, xhwang@chromium.org
not lgtm. I think we need to be more thoughtful about where this is being enabled (especially for EME) and that we have tests for what we're advertising. +wolenetz for comments on MSE histograms. +xhwang for EME platform support issues. https://codereview.chromium.org/1624703002/diff/40001/chrome/renderer/media/c... File chrome/renderer/media/chrome_key_systems.cc (right): https://codereview.chromium.org/1624703002/diff/40001/chrome/renderer/media/c... chrome/renderer/media/chrome_key_systems.cc:191: if (codecs[i] == kCdmSupportedCodecVp8) Missing tests for EME (see chrome/browser/media/encrypted_media_supported_types_browsertest.cc). https://codereview.chromium.org/1624703002/diff/40001/media/base/eme_constants.h File media/base/eme_constants.h (right): https://codereview.chromium.org/1624703002/diff/40001/media/base/eme_constant... media/base/eme_constants.h:47: EME_CODEC_MP4_VP8 = 1 << 6, Is this actually going to be used? If not or we don't know, perhaps we should only support VP9 for now. https://codereview.chromium.org/1624703002/diff/40001/media/base/eme_constant... media/base/eme_constants.h:48: EME_CODEC_MP4_VP9 = 1 << 7, Do we need separate codec enums? I know these are currently tied to containers and we have <container>_ALL values, but is that necessary? I have always been concerned about the _ALL values (see below), and at least one use appears to be obsolete (see next file). https://codereview.chromium.org/1624703002/diff/40001/media/base/eme_constant... media/base/eme_constants.h:49: EME_CODEC_MP4_VIDEO_ALL = Not all platforms that support MP4 necessarily support VPx [in MP4]. If these _ALL values are used incorrectly during registration, support could be incorrectly advertised. (These _ALL values should be eliminated or somehow made internal to media/.) For example, https://code.google.com/p/chromium/codesearch#chromium/src/components/cdm/ren... and https://code.google.com/p/chromium/codesearch#chromium/src/components/cdm/ren.... If we do care about the container a codec supports, we will need specific checks. If not, we can rely on container validity checks (for clear content) along with initDataType checks for whether the CDM can generate license requests. https://codereview.chromium.org/1624703002/diff/40001/media/base/key_systems.cc File media/base/key_systems.cc (right): https://codereview.chromium.org/1624703002/diff/40001/media/base/key_systems.... media/base/key_systems.cc:41: // TODO(sandersd): This definition only makes sense for prefixed EME. Change it Note: I believe this note is about the separation of codecs from containers (in the current EME spec). https://codereview.chromium.org/1624703002/diff/40001/media/base/key_systems.... media/base/key_systems.cc:61: {"vp09", EME_CODEC_MP4_VP9}, Just to confirm, is "vp0x" the value being used in the spec? I guess to be a four-character value. https://codereview.chromium.org/1624703002/diff/40001/media/base/mime_util.cc File media/base/mime_util.cc (right): https://codereview.chromium.org/1624703002/diff/40001/media/base/mime_util.cc... media/base/mime_util.cc:218: // vp08.... - VP8 We should have playback tests (both clear and encrypted) before we go advertising this. Perhaps land the files that add playback support along with tests before changing the type support code. https://codereview.chromium.org/1624703002/diff/40001/media/base/mime_util.cc... media/base/mime_util.cc:241: // This is not a complete list of supported vpx codecs. Does the same comment as for avc1 apply? Perhaps refer to it here (e.g. "Similar to above..."). https://codereview.chromium.org/1624703002/diff/40001/media/base/mime_util.cc... media/base/mime_util.cc:244: #if BUILDFLAG(ENABLE_AC3_EAC3_AUDIO_DEMUXING) It's unfortunate how this was inserted in the mp4a.* list. We should fix that separately. https://codereview.chromium.org/1624703002/diff/40001/media/base/mime_util.cc... media/base/mime_util.cc:681: // It is ambiguous with missing fields. Do we want to support this? Having supported just "avc1" has led to the point where we must support it indefinitely and cannot make any true determination about what is being served. Since this is new and we are likely the first to support it, can we be more strict and require exactly 8 fields? https://codereview.chromium.org/1624703002/diff/40001/media/base/mime_util_un... File media/base/mime_util_unittest.cc (right): https://codereview.chromium.org/1624703002/diff/40001/media/base/mime_util_un... media/base/mime_util_unittest.cc:114: TEST(MimeUtilTest, AreSupportedMediaCodecs) { These things are tested in content/browser/media/media_canplaytype_browsertest.cc. It's weird to have only a subset of types here. Also, the expectation should be based on the preprocessor define, not the running of the tests. See that file for examples and pre-existing constants to use. https://codereview.chromium.org/1624703002/diff/40001/media/base/mime_util_un... media/base/mime_util_unittest.cc:116: EXPECT_TRUE(AreSupportedMediaCodecs({"vp08"})); There are no extended tests for vp8. https://codereview.chromium.org/1624703002/diff/40001/media/filters/stream_pa... File media/filters/stream_parser_factory.cc (right): https://codereview.chromium.org/1624703002/diff/40001/media/filters/stream_pa... media/filters/stream_parser_factory.cc:165: CodecInfo::HISTOGRAM_VP8}; I wonder whether we want to have different histograms for each container. +wolenetz https://codereview.chromium.org/1624703002/diff/40001/media/formats/mp4/fourc... File media/formats/mp4/fourccs.h (right): https://codereview.chromium.org/1624703002/diff/40001/media/formats/mp4/fourc... media/formats/mp4/fourccs.h:101: FOURCC_VPCC = 0x76706343, What is VPCC?
We should probably move some of this discussion (i.e. what to expose when) to the bug for broader input and easier reading in the future. I think we might need to clean up some technical debt first so we can more easily reason about such changes. Please update the description to something like Support VP8 and VP9 in MP4 (ISO BMFF and CENC). https://codereview.chromium.org/1624703002/diff/40001/media/base/eme_constants.h File media/base/eme_constants.h (right): https://codereview.chromium.org/1624703002/diff/40001/media/base/eme_constant... media/base/eme_constants.h:48: EME_CODEC_MP4_VP9 = 1 << 7, On 2016/01/27 01:39:14, ddorwin wrote: > Do we need separate codec enums? I know these are currently tied to containers > and we have <container>_ALL values, but is that necessary? I have always been > concerned about the _ALL values (see below), and at least one use appears to be > obsolete (see next file). Of course, we also rely on this difference here: https://code.google.com/p/chromium/codesearch#chromium/src/components/cdm/ren... However, I'm not sure the Android code will ever set MP4_VP9 as this CL currently stands. There are a lot of corner cases exposed by allowing codecs in different containers, and it appears to be too difficult to reason about them at the moment. Perhaps we should address the technical debt that makes assumptions about codecs, containers, and initialization data types. With the removal of prefixed EME, it might be possible to remove some of these (see the TODO in the next file). https://codereview.chromium.org/1624703002/diff/40001/media/base/mime_util.cc File media/base/mime_util.cc (right): https://codereview.chromium.org/1624703002/diff/40001/media/base/mime_util.cc... media/base/mime_util.cc:218: // vp08.... - VP8 On 2016/01/27 01:39:15, ddorwin wrote: > We should have playback tests (both clear and encrypted) before we go > advertising this. Perhaps land the files that add playback support along with > tests before changing the type support code. Specifically, playback tests for src=, MSE, and MSE/EME. (.src=.mp4 is not supported for EME). https://codereview.chromium.org/1624703002/diff/40001/media/base/mime_util.cc... media/base/mime_util.cc:219: // vp09.... - VP9 Do we want to turn this on by default before the spec is finalized? (It's fine to implement, but should it be behind a flag?) Speaking of which, we may want to do an Intent To... https://codereview.chromium.org/1624703002/diff/40001/media/filters/stream_pa... File media/filters/stream_parser_factory.cc (right): https://codereview.chromium.org/1624703002/diff/40001/media/filters/stream_pa... media/filters/stream_parser_factory.cc:165: CodecInfo::HISTOGRAM_VP8}; On 2016/01/27 01:39:15, ddorwin wrote: > I wonder whether we want to have different histograms for each container. > +wolenetz Are there other playback UMAs that we should update/differentiate as well?
https://codereview.chromium.org/1624703002/diff/40001/media/base/eme_constants.h File media/base/eme_constants.h (right): https://codereview.chromium.org/1624703002/diff/40001/media/base/eme_constant... media/base/eme_constants.h:48: EME_CODEC_MP4_VP9 = 1 << 7, On 2016/01/27 03:17:09, ddorwin wrote: > On 2016/01/27 01:39:14, ddorwin wrote: > > Do we need separate codec enums? I know these are currently tied to containers > > and we have <container>_ALL values, but is that necessary? I have always been > > concerned about the _ALL values (see below), and at least one use appears to > be > > obsolete (see next file). > > Of course, we also rely on this difference here: > https://code.google.com/p/chromium/codesearch#chromium/src/components/cdm/ren... > > However, I'm not sure the Android code will ever set MP4_VP9 as this CL > currently stands. > > There are a lot of corner cases exposed by allowing codecs in different > containers, and it appears to be too difficult to reason about them at the > moment. Perhaps we should address the technical debt that makes assumptions > about codecs, containers, and initialization data types. With the removal of > prefixed EME, it might be possible to remove some of these (see the TODO in the > next file). Specifically, we can address the Android case by adding initDataTypes to SupportedKeySystemResponse. We need to do that anyway if we ever want to support keyids. xhwang: Is there a Mojo interface that will replace this Android-specific query? Alternatively, there is https://crbug.com/457438, which would change all of this.
Thanks for the detail and critic review :) PTAL https://codereview.chromium.org/1624703002/diff/40001/chrome/renderer/media/c... File chrome/renderer/media/chrome_key_systems.cc (right): https://codereview.chromium.org/1624703002/diff/40001/chrome/renderer/media/c... chrome/renderer/media/chrome_key_systems.cc:191: if (codecs[i] == kCdmSupportedCodecVp8) On 2016/01/27 01:39:14, ddorwin wrote: > Missing tests for EME (see > chrome/browser/media/encrypted_media_supported_types_browsertest.cc). Done. https://codereview.chromium.org/1624703002/diff/40001/media/base/eme_constants.h File media/base/eme_constants.h (right): https://codereview.chromium.org/1624703002/diff/40001/media/base/eme_constant... media/base/eme_constants.h:47: EME_CODEC_MP4_VP8 = 1 << 6, On 2016/01/27 01:39:14, ddorwin wrote: > Is this actually going to be used? If not or we don't know, perhaps we should > only support VP9 for now. I don't know, but why not, it does not require a lot of effort to support it. That said, I am fine with removing VP8 support if you have a strong opinion on this. https://codereview.chromium.org/1624703002/diff/40001/media/base/eme_constant... media/base/eme_constants.h:49: EME_CODEC_MP4_VIDEO_ALL = On 2016/01/27 01:39:14, ddorwin wrote: > Not all platforms that support MP4 necessarily support VPx [in MP4]. If these > _ALL values are used incorrectly during registration, support could be > incorrectly advertised. (These _ALL values should be eliminated or somehow made > internal to media/.) Aren't these _ALL values just a list of all possible codecs in EME? Whether a particular codec is supported or not is determined by the platform specific code, and if supported, will be available in KeySystemInfo::supported_codecs. > > For example, > https://code.google.com/p/chromium/codesearch#chromium/src/components/cdm/ren... > and > https://code.google.com/p/chromium/codesearch#chromium/src/components/cdm/ren.... I don't see a problem in the examples above. mp4 uses pssh as init_data. If any of the mp4 codec is supported, it should support pssh init_data. > > If we do care about the container a codec supports, we will need specific > checks. If not, we can rely on container validity checks (for clear content) > along with initDataType checks for whether the CDM can generate license > requests. https://codereview.chromium.org/1624703002/diff/40001/media/base/key_systems.cc File media/base/key_systems.cc (right): https://codereview.chromium.org/1624703002/diff/40001/media/base/key_systems.... media/base/key_systems.cc:58: {"vp08", EME_CODEC_MP4_VP8}, On 2016/01/27 00:47:00, sandersd wrote: > These are only defined when USE_PROPRIETARY_CODECS is set. Good catch. Done. https://codereview.chromium.org/1624703002/diff/40001/media/base/key_systems.... media/base/key_systems.cc:61: {"vp09", EME_CODEC_MP4_VP9}, On 2016/01/27 01:39:15, ddorwin wrote: > Just to confirm, is "vp0x" the value being used in the spec? I guess to be a > four-character value. Correct. https://codereview.chromium.org/1624703002/diff/40001/media/base/mime_util.cc File media/base/mime_util.cc (right): https://codereview.chromium.org/1624703002/diff/40001/media/base/mime_util.cc... media/base/mime_util.cc:218: // vp08.... - VP8 On 2016/01/27 03:17:09, ddorwin wrote: > On 2016/01/27 01:39:15, ddorwin wrote: > > We should have playback tests (both clear and encrypted) before we go > > advertising this. Perhaps land the files that add playback support along with > > tests before changing the type support code. > > Specifically, playback tests for src=, MSE, and MSE/EME. (.src=.mp4 is not > supported for EME). Added tests and test files. Let me know if you'd like them to be in another cl instead. https://codereview.chromium.org/1624703002/diff/40001/media/base/mime_util.cc... media/base/mime_util.cc:219: // vp09.... - VP9 On 2016/01/27 03:17:09, ddorwin wrote: > Do we want to turn this on by default before the spec is finalized? > (It's fine to implement, but should it be behind a flag?) > Speaking of which, we may want to do an Intent To... Sure, added flag ENABLE_MP4_VP8_VP9_DEMUXING, off for now. https://codereview.chromium.org/1624703002/diff/40001/media/base/mime_util.cc... media/base/mime_util.cc:241: // This is not a complete list of supported vpx codecs. On 2016/01/27 01:39:15, ddorwin wrote: > Does the same comment as for avc1 apply? Perhaps refer to it here (e.g. "Similar > to above..."). Done. https://codereview.chromium.org/1624703002/diff/40001/media/base/mime_util.cc... media/base/mime_util.cc:244: #if BUILDFLAG(ENABLE_AC3_EAC3_AUDIO_DEMUXING) On 2016/01/27 01:39:15, ddorwin wrote: > It's unfortunate how this was inserted in the mp4a.* list. We should fix that > separately. Since it is just reordering, I'll fix it in this cl. https://codereview.chromium.org/1624703002/diff/40001/media/base/mime_util.cc... media/base/mime_util.cc:681: // It is ambiguous with missing fields. On 2016/01/27 01:39:15, ddorwin wrote: > Do we want to support this? Having supported just "avc1" has led to the point > where we must support it indefinitely and cannot make any true determination > about what is being served. Since this is new and we are likely the first to > support it, can we be more strict and require exactly 8 fields? I am not sure whether that is a good idea as the spec explicitly mentions that the fields can be omitted and truncated, but sure (we can always relax the code later if needed). Done. https://codereview.chromium.org/1624703002/diff/40001/media/base/mime_util_un... File media/base/mime_util_unittest.cc (right): https://codereview.chromium.org/1624703002/diff/40001/media/base/mime_util_un... media/base/mime_util_unittest.cc:114: TEST(MimeUtilTest, AreSupportedMediaCodecs) { On 2016/01/27 01:39:15, ddorwin wrote: > These things are tested in > content/browser/media/media_canplaytype_browsertest.cc. It's weird to have only > a subset of types here. > > Also, the expectation should be based on the preprocessor define, not the > running of the tests. See that file for examples and pre-existing constants to > use. I think these things should really be tested here instead of in content/browser/media/media_canplaytype_browsertest.cc. It runs much faster and is much easier to debug. Anyway, I will move the tests to media_canplaytype_browsertest.cc to be consistent. https://codereview.chromium.org/1624703002/diff/40001/media/base/mime_util_un... media/base/mime_util_unittest.cc:116: EXPECT_TRUE(AreSupportedMediaCodecs({"vp08"})); On 2016/01/27 01:39:15, ddorwin wrote: > There are no extended tests for vp8. Added. https://codereview.chromium.org/1624703002/diff/40001/media/filters/stream_pa... File media/filters/stream_parser_factory.cc (right): https://codereview.chromium.org/1624703002/diff/40001/media/filters/stream_pa... media/filters/stream_parser_factory.cc:165: CodecInfo::HISTOGRAM_VP8}; On 2016/01/27 01:39:15, ddorwin wrote: > I wonder whether we want to have different histograms for each container. > +wolenetz Sounds like a good idea to me. Done. https://codereview.chromium.org/1624703002/diff/40001/media/formats/mp4/fourc... File media/formats/mp4/fourccs.h (right): https://codereview.chromium.org/1624703002/diff/40001/media/formats/mp4/fourc... media/formats/mp4/fourccs.h:101: FOURCC_VPCC = 0x76706343, On 2016/01/27 01:39:15, ddorwin wrote: > What is VPCC? VP codec configuration box. Removed, since it is not used right now.
https://codereview.chromium.org/1624703002/diff/40001/media/base/eme_constants.h File media/base/eme_constants.h (right): https://codereview.chromium.org/1624703002/diff/40001/media/base/eme_constant... media/base/eme_constants.h:48: EME_CODEC_MP4_VP9 = 1 << 7, On 2016/01/27 06:42:56, ddorwin wrote: > On 2016/01/27 03:17:09, ddorwin wrote: > > On 2016/01/27 01:39:14, ddorwin wrote: > > > Do we need separate codec enums? I know these are currently tied to > containers > > > and we have <container>_ALL values, but is that necessary? I have always > been > > > concerned about the _ALL values (see below), and at least one use appears to > > be > > > obsolete (see next file). > > > > Of course, we also rely on this difference here: > > > https://code.google.com/p/chromium/codesearch#chromium/src/components/cdm/ren... > > > > However, I'm not sure the Android code will ever set MP4_VP9 as this CL > > currently stands. > > > > There are a lot of corner cases exposed by allowing codecs in different > > containers, and it appears to be too difficult to reason about them at the > > moment. Perhaps we should address the technical debt that makes assumptions > > about codecs, containers, and initialization data types. With the removal of > > prefixed EME, it might be possible to remove some of these (see the TODO in > the > > next file). > > Specifically, we can address the Android case by adding initDataTypes to > SupportedKeySystemResponse. We need to do that anyway if we ever want to support > keyids. > > xhwang: Is there a Mojo interface that will replace this Android-specific query? > > Alternatively, there is https://crbug.com/457438, which would change all of > this. Currently the mojo work is focusing mainly on the core EME stack (MediaKeys interface and implementations). We don't have any plan to convert the key system (and codec) support query over render-browser IPC yet.
ddorwin@, can you take another look when you have a chance? Thanks.
Thank you for addressing my comments, including adding tests. Please follow http://www.chromium.org/blink#launch-process. Since this is not implemented in Blink, you may be able to follow the "Web-Facing Change PSA" steps. (However, there was a recent Intent to Implement to add a new codec to WebRTC: https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/rsokc4bi8R4/Hr...) https://codereview.chromium.org/1624703002/diff/40001/media/base/eme_constants.h File media/base/eme_constants.h (right): https://codereview.chromium.org/1624703002/diff/40001/media/base/eme_constant... media/base/eme_constants.h:49: EME_CODEC_MP4_VIDEO_ALL = On 2016/01/29 00:34:16, kqyang wrote: > On 2016/01/27 01:39:14, ddorwin wrote: > > Not all platforms that support MP4 necessarily support VPx [in MP4]. If these > > _ALL values are used incorrectly during registration, support could be > > incorrectly advertised. (These _ALL values should be eliminated or somehow > made > > internal to media/.) > > Aren't these _ALL values just a list of all possible codecs in EME? Whether a > particular codec is supported or not is determined by the platform specific > code, and if supported, will be available in KeySystemInfo::supported_codecs. > > > > > For example, > > > https://code.google.com/p/chromium/codesearch#chromium/src/components/cdm/ren... > > and > > > https://code.google.com/p/chromium/codesearch#chromium/src/components/cdm/ren.... > > > I don't see a problem in the examples above. mp4 uses pssh as init_data. If any > of the mp4 codec is supported, it should support pssh init_data. > > > > > If we do care about the container a codec supports, we will need specific > > checks. If not, we can rely on container validity checks (for clear content) > > along with initDataType checks for whether the CDM can generate license > > requests. Yes, those specific examples are fine, but that code should not have access to them. I filed https://bugs.chromium.org/p/chromium/issues/detail?id=589515. https://codereview.chromium.org/1624703002/diff/80001/content/browser/media/m... File content/browser/media/media_canplaytype_browsertest.cc (right): https://codereview.chromium.org/1624703002/diff/80001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:776: CanPlay("'video/x-m4v; codecs=\"vp08.00.00.08.00.00.00.00\"'")); I wonder if we should *not* allow new codecs with these legacy/compatibility strings. I'll send an email. https://codereview.chromium.org/1624703002/diff/80001/media/base/mime_util.cc File media/base/mime_util.cc (right): https://codereview.chromium.org/1624703002/diff/80001/media/base/mime_util.cc... media/base/mime_util.cc:1: // Copyright 2012 The Chromium Authors. All rights reserved. Please rebase. Most of this code has moved. https://codereview.chromium.org/1624703002/diff/80001/media/base/mime_util.cc... media/base/mime_util.cc:47: WEBM_VP8, We may not need these anymore. We can now (after rebasing) check the MIME type string with the codec for Android. However, we may still need them because the codec strings are different, BUT I do not think that should be exposed in this enum. We may need to pass the MIME type to StringToCodec() to make sure we are checking the appropriate strings. This would also help us avoid adding support for new codecs with legacy container strings. https://codereview.chromium.org/1624703002/diff/80001/media/base/mime_util.cc... media/base/mime_util.cc:49: MPEG4_VP8, As discussed, unless there is a demand for VP8 in MP4, let's not enable it. https://codereview.chromium.org/1624703002/diff/80001/media/base/mime_util.cc... media/base/mime_util.cc:779: case MPEG4_VP8: These codecs aren't actually proprietary. Really, though, I think we should avoid the container in the name.
A few more thoughts. https://codereview.chromium.org/1624703002/diff/80001/content/browser/media/m... File content/browser/media/media_canplaytype_browsertest.cc (right): https://codereview.chromium.org/1624703002/diff/80001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:376: CanPlay("'" + mime + "; codecs=\"vp09.01.01.08.02.01.01.00\"'")); Should we be supporting this or other new strings with WebM? For example, currently there is no way to specify profiles (other than 0). https://codereview.chromium.org/1624703002/diff/80001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:776: CanPlay("'video/x-m4v; codecs=\"vp08.00.00.08.00.00.00.00\"'")); On 2016/02/24 22:44:32, ddorwin wrote: > I wonder if we should *not* allow new codecs with these legacy/compatibility > strings. I'll send an email. I filed https://bugs.chromium.org/p/chromium/issues/detail?id=589675. https://codereview.chromium.org/1624703002/diff/80001/media/BUILD.gn File media/BUILD.gn (right): https://codereview.chromium.org/1624703002/diff/80001/media/BUILD.gn#newcode21 media/BUILD.gn:21: "ENABLE_MP4_VP8_VP9_DEMUXING=0", We won't be able to enable this until we roll in an FFmpeg version that supports demuxing VPx from MP4. https://codereview.chromium.org/1624703002/diff/80001/media/base/mime_util.cc File media/base/mime_util.cc (right): https://codereview.chromium.org/1624703002/diff/80001/media/base/mime_util.cc... media/base/mime_util.cc:192: // TODO(kqyang): Verify if it is supported in Android. Return false for This will work with the unified pipeline (see new code) but not otherwise.
https://codereview.chromium.org/1624703002/diff/100001/media/base/mime_util_i... File media/base/mime_util_internal.cc (right): https://codereview.chromium.org/1624703002/diff/100001/media/base/mime_util_i... media/base/mime_util_internal.cc:137: // ParseVpxCodecID(). s/x/9/ https://codereview.chromium.org/1624703002/diff/100001/media/base/mime_util_i... media/base/mime_util_internal.cc:167: {"vp9", MimeUtil::VP9}, As discussed, you may want to remove these and rely on ParseVp9CodecID in all cases. https://codereview.chromium.org/1624703002/diff/100001/media/base/mime_util_i... media/base/mime_util_internal.cc:316: if (profile > 3) We'll need to actually check whether platform supports the profile, etc. before shipping this. For libvpx, we can know statically. This may share some querying and IPC infrastructure with similar efforts for HEVC. https://codereview.chromium.org/1624703002/diff/100001/media/base/mime_util_i... media/base/mime_util_internal.cc:645: *codec = itr->second.codec; // Exact match. No further parsing necessary. https://codereview.chromium.org/1624703002/diff/100001/media/filters/stream_p... File media/filters/stream_parser_factory.cc (right): https://codereview.chromium.org/1624703002/diff/100001/media/filters/stream_p... media/filters/stream_parser_factory.cc:169: static const CodecInfo kMPEG4VP09CodecInfo = {"vp09.*", CodecInfo::VIDEO, NULL, Note: None of your parsing code gets called for MSE: https://bugs.chromium.org/p/chromium/issues/detail?id=587303
https://codereview.chromium.org/1624703002/diff/100001/media/base/mime_util_i... File media/base/mime_util_internal.cc (right): https://codereview.chromium.org/1624703002/diff/100001/media/base/mime_util_i... media/base/mime_util_internal.cc:282: bool* is_ambiguous) { Since there are no ambiguous VPx strings (ignoring the fact that "vp9" is assumed to be profile 0), I don't think we need this parameter. Just set it at the call site. https://codereview.chromium.org/1624703002/diff/100001/media/base/mime_util_i... media/base/mime_util_internal.cc:390: codec == MimeUtil::VP9) { Check the enum first. As we discussed, should be moved inside StringToCodec() and the logic may change. https://codereview.chromium.org/1624703002/diff/100001/media/base/mime_util_i... media/base/mime_util_internal.cc:639: bool MimeUtil::StringToCodec(const std::string& codec_id, This is no longer a simple conversion. I think we may want to rename it to ParseCodecString(). https://codereview.chromium.org/1624703002/diff/100001/media/base/mime_util_i... media/base/mime_util_internal.cc:640: Codec* codec, When we add platform checks, we'll either need to add them after each of the Parse*() calls before returning true or expose generic values for profile, etc. as output parameters and make a single check, including the Codec enum. In the former case, we'd need to rename this beyond my suggestion above. Having a function whose sole purpose is to parse (the latter case) seems appealing, though, and is easier to test.
Thanks for the review. PTAL https://codereview.chromium.org/1624703002/diff/80001/content/browser/media/m... File content/browser/media/media_canplaytype_browsertest.cc (right): https://codereview.chromium.org/1624703002/diff/80001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:376: CanPlay("'" + mime + "; codecs=\"vp09.01.01.08.02.01.01.00\"'")); On 2016/02/25 00:27:47, ddorwin wrote: > Should we be supporting this or other new strings with WebM? For example, > currently there is no way to specify profiles (other than 0). I like this idea. Keep it as not supported for now until we make a decision. https://codereview.chromium.org/1624703002/diff/80001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:776: CanPlay("'video/x-m4v; codecs=\"vp08.00.00.08.00.00.00.00\"'")); On 2016/02/25 00:27:47, ddorwin wrote: > On 2016/02/24 22:44:32, ddorwin wrote: > > I wonder if we should *not* allow new codecs with these legacy/compatibility > > strings. I'll send an email. > > I filed https://bugs.chromium.org/p/chromium/issues/detail?id=589675. Test removed. As discussed offline, I won't do any special handling of legacy mime types in this cl though. I suppose you are going to do some refactoring in mime_util to separate definitions for legacy mime types and video/mp4. https://codereview.chromium.org/1624703002/diff/80001/media/BUILD.gn File media/BUILD.gn (right): https://codereview.chromium.org/1624703002/diff/80001/media/BUILD.gn#newcode21 media/BUILD.gn:21: "ENABLE_MP4_VP8_VP9_DEMUXING=0", On 2016/02/25 00:27:47, ddorwin wrote: > We won't be able to enable this until we roll in an FFmpeg version that supports > demuxing VPx from MP4. Acknowledged. https://codereview.chromium.org/1624703002/diff/80001/media/base/mime_util.cc File media/base/mime_util.cc (right): https://codereview.chromium.org/1624703002/diff/80001/media/base/mime_util.cc... media/base/mime_util.cc:47: WEBM_VP8, On 2016/02/24 22:44:33, ddorwin wrote: > We may not need these anymore. We can now (after rebasing) check the MIME type > string with the codec for Android. > > However, we may still need them because the codec strings are different, BUT I > do not think that should be exposed in this enum. > > We may need to pass the MIME type to StringToCodec() to make sure we are > checking the appropriate strings. This would also help us avoid adding support > for new codecs with legacy container strings. Removed and pass MIME type to StringToCodec instead as suggested. https://codereview.chromium.org/1624703002/diff/80001/media/base/mime_util.cc... media/base/mime_util.cc:49: MPEG4_VP8, On 2016/02/24 22:44:33, ddorwin wrote: > As discussed, unless there is a demand for VP8 in MP4, let's not enable it. Removed. https://codereview.chromium.org/1624703002/diff/80001/media/base/mime_util.cc... media/base/mime_util.cc:779: case MPEG4_VP8: On 2016/02/24 22:44:33, ddorwin wrote: > These codecs aren't actually proprietary. Really, though, I think we should > avoid the container in the name. Removed. https://codereview.chromium.org/1624703002/diff/100001/media/base/mime_util_i... File media/base/mime_util_internal.cc (right): https://codereview.chromium.org/1624703002/diff/100001/media/base/mime_util_i... media/base/mime_util_internal.cc:137: // ParseVpxCodecID(). On 2016/02/29 23:57:34, ddorwin wrote: > s/x/9/ Done. https://codereview.chromium.org/1624703002/diff/100001/media/base/mime_util_i... media/base/mime_util_internal.cc:167: {"vp9", MimeUtil::VP9}, On 2016/02/29 23:57:34, ddorwin wrote: > As discussed, you may want to remove these and rely on ParseVp9CodecID in all > cases. Done. https://codereview.chromium.org/1624703002/diff/100001/media/base/mime_util_i... media/base/mime_util_internal.cc:282: bool* is_ambiguous) { On 2016/03/01 00:21:40, ddorwin wrote: > Since there are no ambiguous VPx strings (ignoring the fact that "vp9" is > assumed to be profile 0), I don't think we need this parameter. Just set it at > the call site. Done. https://codereview.chromium.org/1624703002/diff/100001/media/base/mime_util_i... media/base/mime_util_internal.cc:390: codec == MimeUtil::VP9) { On 2016/03/01 00:21:40, ddorwin wrote: > Check the enum first. > As we discussed, should be moved inside StringToCodec() and the logic may > change. No longer needed. Removed. https://codereview.chromium.org/1624703002/diff/100001/media/base/mime_util_i... media/base/mime_util_internal.cc:639: bool MimeUtil::StringToCodec(const std::string& codec_id, On 2016/03/01 00:21:40, ddorwin wrote: > This is no longer a simple conversion. I think we may want to rename it to > ParseCodecString(). Unfortunately ParseCodecString is already taken. https://codereview.chromium.org/1624703002/diff/100001/media/base/mime_util_i... media/base/mime_util_internal.cc:640: Codec* codec, On 2016/03/01 00:21:40, ddorwin wrote: > When we add platform checks, we'll either need to add them after each of the > Parse*() calls before returning true or expose generic values for profile, etc. > as output parameters and make a single check, including the Codec enum. In the > former case, we'd need to rename this beyond my suggestion above. Having a > function whose sole purpose is to parse (the latter case) seems appealing, > though, and is easier to test. Another option is to define different Codec enums for different profiles, e.g. VP9_PROFILE_0, VP9_PROFILE_1, HEVC_MAIN etc.
Description was changed from ========== Implement support for vpx in ISO-BMFF BUG=580623 ========== to ========== Implement support for vp9 in ISO-BMFF BUG=580623 ==========
I reviewed the differences from previous patch sets. I'll review against base again after these few comments are addressed. https://codereview.chromium.org/1624703002/diff/80001/content/browser/media/m... File content/browser/media/media_canplaytype_browsertest.cc (right): https://codereview.chromium.org/1624703002/diff/80001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:776: CanPlay("'video/x-m4v; codecs=\"vp08.00.00.08.00.00.00.00\"'")); On 2016/03/01 01:18:51, kqyang wrote: > On 2016/02/25 00:27:47, ddorwin wrote: > > On 2016/02/24 22:44:32, ddorwin wrote: > > > I wonder if we should *not* allow new codecs with these legacy/compatibility > > > strings. I'll send an email. > > > > I filed https://bugs.chromium.org/p/chromium/issues/detail?id=589675. > > Test removed. As discussed offline, I won't do any special handling of legacy > mime types in this cl though. I suppose you are going to do some refactoring in > mime_util to separate definitions for legacy mime types and video/mp4. Keep the test. We should test all of the cases. You can add a comment that this may change as a result of the bug I mentioned above. TODO(ddorwin): This should be kNot. https://crbug.com/589675 https://codereview.chromium.org/1624703002/diff/100001/media/base/mime_util_i... File media/base/mime_util_internal.cc (right): https://codereview.chromium.org/1624703002/diff/100001/media/base/mime_util_i... media/base/mime_util_internal.cc:639: bool MimeUtil::StringToCodec(const std::string& codec_id, On 2016/03/01 01:18:52, kqyang wrote: > On 2016/03/01 00:21:40, ddorwin wrote: > > This is no longer a simple conversion. I think we may want to rename it to > > ParseCodecString(). > > Unfortunately ParseCodecString is already taken. Okay, I'll work on this. https://codereview.chromium.org/1624703002/diff/100001/media/base/mime_util_i... media/base/mime_util_internal.cc:640: Codec* codec, On 2016/03/01 01:18:52, kqyang wrote: > On 2016/03/01 00:21:40, ddorwin wrote: > > When we add platform checks, we'll either need to add them after each of the > > Parse*() calls before returning true or expose generic values for profile, > etc. > > as output parameters and make a single check, including the Codec enum. In the > > former case, we'd need to rename this beyond my suggestion above. Having a > > function whose sole purpose is to parse (the latter case) seems appealing, > > though, and is easier to test. > > Another option is to define different Codec enums for different profiles, e.g. > > VP9_PROFILE_0, VP9_PROFILE_1, > HEVC_MAIN etc. Yes, but I think we want to avoid that in general since it pollutes the switch statements for general codec checks. https://codereview.chromium.org/1624703002/diff/120001/content/browser/media/... File content/browser/media/media_canplaytype_browsertest.cc (right): https://codereview.chromium.org/1624703002/diff/120001/content/browser/media/... content/browser/media/media_canplaytype_browsertest.cc:765: See the comment in PS5. https://codereview.chromium.org/1624703002/diff/120001/content/browser/media/... content/browser/media/media_canplaytype_browsertest.cc:841: ditto https://codereview.chromium.org/1624703002/diff/120001/content/browser/media/... content/browser/media/media_canplaytype_browsertest.cc:1226: IN_PROC_BROWSER_TEST_F(MediaCanPlayTypeTest, CodecSupportTest_HLS) { We should have negative tests for the two HLS containers. https://codereview.chromium.org/1624703002/diff/120001/media/base/mime_util_i... File media/base/mime_util_internal.cc (right): https://codereview.chromium.org/1624703002/diff/120001/media/base/mime_util_i... media/base/mime_util_internal.cc:280: if (mime_type == "video/webm") { I believe we can reach this code with "audio/*". We are special casing webm, but only the video case. Thus, "audio/webm" will fall through to the MP4 code. This is fine because we check container compatibility later, but it is unexpected. Perhaps we should explicitly check for "audio/webm" too. Really, we should replace the |mime_type| string with an enum, but that's another issue. https://codereview.chromium.org/1624703002/diff/120001/media/base/mime_util_i... media/base/mime_util_internal.cc:630: // Otherwise, platform support is required. I think we should check that the container is "video/webm" here since we don't know if the platform (Android) supports MP4. This mostly applies to the MediaPlayer path, which should eventually go away.
PTAL https://codereview.chromium.org/1624703002/diff/80001/content/browser/media/m... File content/browser/media/media_canplaytype_browsertest.cc (right): https://codereview.chromium.org/1624703002/diff/80001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:776: CanPlay("'video/x-m4v; codecs=\"vp08.00.00.08.00.00.00.00\"'")); On 2016/03/02 00:58:21, ddorwin wrote: > On 2016/03/01 01:18:51, kqyang wrote: > > On 2016/02/25 00:27:47, ddorwin wrote: > > > On 2016/02/24 22:44:32, ddorwin wrote: > > > > I wonder if we should *not* allow new codecs with these > legacy/compatibility > > > > strings. I'll send an email. > > > > > > I filed https://bugs.chromium.org/p/chromium/issues/detail?id=589675. > > > > Test removed. As discussed offline, I won't do any special handling of legacy > > mime types in this cl though. I suppose you are going to do some refactoring > in > > mime_util to separate definitions for legacy mime types and video/mp4. > > Keep the test. We should test all of the cases. You can add a comment that this > may change as a result of the bug I mentioned above. TODO(ddorwin): This should > be kNot. https://crbug.com/589675 Done. https://codereview.chromium.org/1624703002/diff/120001/content/browser/media/... File content/browser/media/media_canplaytype_browsertest.cc (right): https://codereview.chromium.org/1624703002/diff/120001/content/browser/media/... content/browser/media/media_canplaytype_browsertest.cc:765: On 2016/03/02 00:58:21, ddorwin wrote: > See the comment in PS5. Done. https://codereview.chromium.org/1624703002/diff/120001/content/browser/media/... content/browser/media/media_canplaytype_browsertest.cc:841: On 2016/03/02 00:58:21, ddorwin wrote: > ditto Done. https://codereview.chromium.org/1624703002/diff/120001/content/browser/media/... content/browser/media/media_canplaytype_browsertest.cc:1226: IN_PROC_BROWSER_TEST_F(MediaCanPlayTypeTest, CodecSupportTest_HLS) { On 2016/03/02 00:58:21, ddorwin wrote: > We should have negative tests for the two HLS containers. Done. https://codereview.chromium.org/1624703002/diff/120001/media/base/mime_util_i... File media/base/mime_util_internal.cc (right): https://codereview.chromium.org/1624703002/diff/120001/media/base/mime_util_i... media/base/mime_util_internal.cc:280: if (mime_type == "video/webm") { On 2016/03/02 00:58:21, ddorwin wrote: > I believe we can reach this code with "audio/*". We are special casing webm, but > only the video case. Thus, "audio/webm" will fall through to the MP4 code. This > is fine because we check container compatibility later, but it is unexpected. > Perhaps we should explicitly check for "audio/webm" too. > > Really, we should replace the |mime_type| string with an enum, but that's > another issue. I am not sure if it is a good idea, but sure. Done. https://codereview.chromium.org/1624703002/diff/120001/media/base/mime_util_i... media/base/mime_util_internal.cc:630: // Otherwise, platform support is required. On 2016/03/02 00:58:21, ddorwin wrote: > I think we should check that the container is "video/webm" here since we don't > know if the platform (Android) supports MP4. This mostly applies to the > MediaPlayer path, which should eventually go away. Done.
Thanks. Minor stuff. https://codereview.chromium.org/1624703002/diff/140001/media/base/mime_util_u... File media/base/mime_util_unittest.cc (right): https://codereview.chromium.org/1624703002/diff/140001/media/base/mime_util_u... media/base/mime_util_unittest.cc:285: // VP9 is only supported with WebM. // *Encrypted* VP9... We should probably have a TODO/bug to figure out whether that is true. If the blocks are exactly the same, then MediaCodec shouldn't care. https://codereview.chromium.org/1624703002/diff/140001/media/base/mime_util_u... media/base/mime_util_unittest.cc:294: RunCodecSupportTest( Empty line. Then explain the purpose of this in a comment. https://codereview.chromium.org/1624703002/diff/140001/media/base/mime_util_u... media/base/mime_util_unittest.cc:348: // VP9 is only supported with WebM. MediaPlayer only supports VP9 in WebM. https://codereview.chromium.org/1624703002/diff/140001/media/base/mime_util_u... media/base/mime_util_unittest.cc:357: RunCodecSupportTest( ditto https://codereview.chromium.org/1624703002/diff/140001/media/filters/stream_p... File media/filters/stream_parser_factory.cc (right): https://codereview.chromium.org/1624703002/diff/140001/media/filters/stream_p... media/filters/stream_parser_factory.cc:197: &kH264AVC1CodecInfo, &kH264AVC3CodecInfo, Why the extra spaces?
https://codereview.chromium.org/1624703002/diff/140001/media/base/mime_util_i... File media/base/mime_util_internal.cc (right): https://codereview.chromium.org/1624703002/diff/140001/media/base/mime_util_i... media/base/mime_util_internal.cc:633: return mime_type_lower_case == "video/webm" && platform_info.supports_vp9; Since the blocks are exactly the same, encrypted content should be supported. Thus, we need to change this to something like: if (!platform_info.supports_vp9) return false; // Encrypted content is demuxed so the container is irrelevant. if (is_encrypted) return true; // MediaPlayer only supports VP9 in WebM. return mime_type_lower_case == "video/webm";
PTAL https://codereview.chromium.org/1624703002/diff/140001/media/base/mime_util_i... File media/base/mime_util_internal.cc (right): https://codereview.chromium.org/1624703002/diff/140001/media/base/mime_util_i... media/base/mime_util_internal.cc:633: return mime_type_lower_case == "video/webm" && platform_info.supports_vp9; On 2016/03/10 18:29:24, ddorwin wrote: > Since the blocks are exactly the same, encrypted content should be supported. > Thus, we need to change this to something like: > > if (!platform_info.supports_vp9) return false; > > // Encrypted content is demuxed so the container is irrelevant. > if (is_encrypted) return true; > > // MediaPlayer only supports VP9 in WebM. > return mime_type_lower_case == "video/webm"; Done. https://codereview.chromium.org/1624703002/diff/140001/media/base/mime_util_u... File media/base/mime_util_unittest.cc (right): https://codereview.chromium.org/1624703002/diff/140001/media/base/mime_util_u... media/base/mime_util_unittest.cc:285: // VP9 is only supported with WebM. On 2016/03/10 18:16:54, ddorwin wrote: > // *Encrypted* VP9... > > We should probably have a TODO/bug to figure out whether that is true. If the > blocks are exactly the same, then MediaCodec shouldn't care. Done. https://codereview.chromium.org/1624703002/diff/140001/media/base/mime_util_u... media/base/mime_util_unittest.cc:294: RunCodecSupportTest( On 2016/03/10 18:16:54, ddorwin wrote: > Empty line. > Then explain the purpose of this in a comment. Removed. No longer needed. https://codereview.chromium.org/1624703002/diff/140001/media/base/mime_util_u... media/base/mime_util_unittest.cc:357: RunCodecSupportTest( On 2016/03/10 18:16:53, ddorwin wrote: > ditto Done. https://codereview.chromium.org/1624703002/diff/140001/media/filters/stream_p... File media/filters/stream_parser_factory.cc (right): https://codereview.chromium.org/1624703002/diff/140001/media/filters/stream_p... media/filters/stream_parser_factory.cc:197: &kH264AVC1CodecInfo, &kH264AVC3CodecInfo, On 2016/03/10 18:16:54, ddorwin wrote: > Why the extra spaces? Generated by "git cl format media". Without it, presubmit check fails.
LG. I reviewed everything other than the following. Dan, do you want to take another look? * media/base/mime_util_internal.cc: ParseVp9CodecID() * media/formats/mp4/* * media/test/pipeline_integration_test.cc
sandersd@, do you have a chance to take another look? Thanks.
On 2016/03/24 19:08:01, kqyang wrote: > sandersd@, do you have a chance to take another look? Thanks. No additional comments, lgtm.
On 2016/03/10 23:28:58, ddorwin wrote: > LG. I reviewed everything other than the following. Dan, do you want to take > another look? > * media/base/mime_util_internal.cc: ParseVp9CodecID() > * media/formats/mp4/* > * media/test/pipeline_integration_test.cc Above LG ==> LGTM
https://codereview.chromium.org/1624703002/diff/160001/media/base/mime_util_i... File media/base/mime_util_internal.cc (right): https://codereview.chromium.org/1624703002/diff/160001/media/base/mime_util_i... media/base/mime_util_internal.cc:71: // This is not a complete list of supported vp9 codecs. Similar to avc1 FYI, this code has changed significantly this week, including addressing this comment. You can just do `mp4_video_codecs.insert(VP9);` now. You might include a short comment that only the extended format is supported if it fits.
Description was changed from ========== Implement support for vp9 in ISO-BMFF BUG=580623 ========== to ========== Implement support for vp9 in ISO-BMFF The feature is implemented under flag ENABLE_MP4_VP9_DEMUXING, off for now. BUG=580623 ==========
PTAL. If you don't mind, I'll submit it after getting your approval. Thanks. https://codereview.chromium.org/1624703002/diff/160001/media/base/mime_util_i... File media/base/mime_util_internal.cc (right): https://codereview.chromium.org/1624703002/diff/160001/media/base/mime_util_i... media/base/mime_util_internal.cc:71: // This is not a complete list of supported vp9 codecs. Similar to avc1 On 2016/04/06 01:43:42, ddorwin wrote: > FYI, this code has changed significantly this week, including addressing this > comment. You can just do `mp4_video_codecs.insert(VP9);` now. > > You might include a short comment that only the extended format is supported if > it fits. Done. Thanks.
LG. Some minor issues. Thanks. https://codereview.chromium.org/1624703002/diff/220001/media/base/mime_util_i... File media/base/mime_util_internal.cc (right): https://codereview.chromium.org/1624703002/diff/220001/media/base/mime_util_i... media/base/mime_util_internal.cc:36: // vp9, vp9.0, vp09.xx.xx.xx.xx.xx.xx.xx may be unambiguous; handled by Is it possible for these to be ambiguous (at least in a way we consider - the first two are assumed to be profile 0)? I think the existing comments may be misleading and related to the fact that "avc1" is ambiguous (line 87). Perhaps we should change these to just say, "....XXXXXX are handled by ..." https://codereview.chromium.org/1624703002/diff/220001/media/base/mime_util_i... media/base/mime_util_internal.cc:709: *is_ambiguous = false; Please see the conversations at https://codereview.chromium.org/1677133003/diff/490001/media/base/mime_util_i... and https://codereview.chromium.org/1677133003/diff/590001/media/base/mime_util_i.... The summary is that even though we know it is a valid string, we don't know whether it is actually supported, and despite the naming of this variable, it is really more about "maybe" vs. "probably" than whether something is "ambiguous." In the future, we need to check whether the implementation supports the profile, etc. That means either exposing the profile, etc. as is done for AVC or exposing |is_certain| or similar. For now, we should probably report "maybe" unless profile==0 and file a bug to fix this. https://codereview.chromium.org/1624703002/diff/220001/media/test/data/README File media/test/data/README (right): https://codereview.chromium.org/1624703002/diff/220001/media/test/data/README... media/test/data/README:60: bear-320x240-v_frag-vp9.mp4 - Bear video with VP9 codec in MP4 container. Please provide instructions for creating these. (The others in this list are not good examples, but we're trying to do better.) See examples below.
PTAL https://codereview.chromium.org/1624703002/diff/220001/media/base/mime_util_i... File media/base/mime_util_internal.cc (right): https://codereview.chromium.org/1624703002/diff/220001/media/base/mime_util_i... media/base/mime_util_internal.cc:36: // vp9, vp9.0, vp09.xx.xx.xx.xx.xx.xx.xx may be unambiguous; handled by On 2016/04/18 22:53:29, ddorwin wrote: > Is it possible for these to be ambiguous (at least in a way we consider - the > first two are assumed to be profile 0)? > > I think the existing comments may be misleading and related to the fact that > "avc1" is ambiguous (line 87). Perhaps we should change these to just say, > "....XXXXXX are handled by ..." Per your suggestion below. Now it might be ambiguous, if profile > 0. https://codereview.chromium.org/1624703002/diff/220001/media/base/mime_util_i... media/base/mime_util_internal.cc:709: *is_ambiguous = false; On 2016/04/18 22:53:29, ddorwin wrote: > Please see the conversations at > https://codereview.chromium.org/1677133003/diff/490001/media/base/mime_util_i... > and > https://codereview.chromium.org/1677133003/diff/590001/media/base/mime_util_i.... > > The summary is that even though we know it is a valid string, we don't know > whether it is actually supported, and despite the naming of this variable, it is > really more about "maybe" vs. "probably" than whether something is "ambiguous." > > In the future, we need to check whether the implementation supports the profile, > etc. That means either exposing the profile, etc. as is done for AVC or exposing > |is_certain| or similar. > > For now, we should probably report "maybe" unless profile==0 and file a bug to > fix this. Done. vp9 and vp9.0 are assumed to be profile 0. https://codereview.chromium.org/1624703002/diff/220001/media/test/data/README File media/test/data/README (right): https://codereview.chromium.org/1624703002/diff/220001/media/test/data/README... media/test/data/README:60: bear-320x240-v_frag-vp9.mp4 - Bear video with VP9 codec in MP4 container. On 2016/04/18 22:53:29, ddorwin wrote: > Please provide instructions for creating these. (The others in this list are not > good examples, but we're trying to do better.) See examples below. Done.
LGTM with comments. Thanks. https://codereview.chromium.org/1624703002/diff/220001/media/base/mime_util_i... File media/base/mime_util_internal.cc (right): https://codereview.chromium.org/1624703002/diff/220001/media/base/mime_util_i... media/base/mime_util_internal.cc:36: // vp9, vp9.0, vp09.xx.xx.xx.xx.xx.xx.xx may be unambiguous; handled by On 2016/04/19 00:28:05, kqyang wrote: > On 2016/04/18 22:53:29, ddorwin wrote: > > Is it possible for these to be ambiguous (at least in a way we consider - the > > first two are assumed to be profile 0)? > > > > I think the existing comments may be misleading and related to the fact that > > "avc1" is ambiguous (line 87). Perhaps we should change these to just say, > > "....XXXXXX are handled by ..." > > Per your suggestion below. Now it might be ambiguous, if profile > 0. ambiguous is not the same as "maybe". I'll make this change in https://codereview.chromium.org/1896983004/. https://codereview.chromium.org/1624703002/diff/240001/content/browser/media/... File content/browser/media/media_canplaytype_browsertest.cc (right): https://codereview.chromium.org/1624703002/diff/240001/content/browser/media/... content/browser/media/media_canplaytype_browsertest.cc:1213: // Codecs strings with missing fields. This deletion of vp08 made me realize we no longer have any negative tests for vp08. I suggest picking one "baseline" string and checking for kNot everywhere we check for vp09, including all the other containers. It might be worth checking for exactly "vp08" as well. https://codereview.chromium.org/1624703002/diff/240001/media/formats/mp4/box_... File media/formats/mp4/box_definitions.cc (right): https://codereview.chromium.org/1624703002/diff/240001/media/formats/mp4/box_... media/formats/mp4/box_definitions.cc:652: // TODO(kqyang): Read VPCodecConfiguration and extract profile. crbug?
Thanks for the review! https://codereview.chromium.org/1624703002/diff/240001/content/browser/media/... File content/browser/media/media_canplaytype_browsertest.cc (right): https://codereview.chromium.org/1624703002/diff/240001/content/browser/media/... content/browser/media/media_canplaytype_browsertest.cc:1213: // Codecs strings with missing fields. On 2016/04/19 17:10:00, ddorwin wrote: > This deletion of vp08 made me realize we no longer have any negative tests for > vp08. > > I suggest picking one "baseline" string and checking for kNot everywhere we > check for vp09, including all the other containers. It might be worth checking > for exactly "vp08" as well. Done. https://codereview.chromium.org/1624703002/diff/240001/media/formats/mp4/box_... File media/formats/mp4/box_definitions.cc (right): https://codereview.chromium.org/1624703002/diff/240001/media/formats/mp4/box_... media/formats/mp4/box_definitions.cc:652: // TODO(kqyang): Read VPCodecConfiguration and extract profile. On 2016/04/19 17:10:00, ddorwin wrote: > crbug? Done.
The CQ bit was checked by kqyang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sandersd@chromium.org, ddorwin@chromium.org Link to the patchset: https://codereview.chromium.org/1624703002/#ps260001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1624703002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1624703002/260001
https://codereview.chromium.org/1624703002/diff/260001/content/browser/media/... File content/browser/media/media_canplaytype_browsertest.cc (right): https://codereview.chromium.org/1624703002/diff/260001/content/browser/media/... content/browser/media/media_canplaytype_browsertest.cc:229: EXPECT_EQ(kNot, CanPlay("'" + mime + "; codecs=\"vp8\"'")); Add vp8.0, vp08, vp9.0, vp09 This can replace most of the new lines in this patch set. https://codereview.chromium.org/1624703002/diff/260001/content/browser/media/... content/browser/media/media_canplaytype_browsertest.cc:236: EXPECT_EQ(kNot, CanPlay("'" + mime + "; codecs=\"vp9, mp4a.40.02\"'")); Also add the most basic (i.e. guaranteed supported profile) "vp08....." https://codereview.chromium.org/1624703002/diff/260001/content/browser/media/... content/browser/media/media_canplaytype_browsertest.cc:349: EXPECT_EQ(kNot, Duplicate the new and suggested lines in the WEBM function below. https://codereview.chromium.org/1624703002/diff/260001/content/browser/media/... content/browser/media/media_canplaytype_browsertest.cc:407: EXPECT_EQ(kNot, CanPlay("'" + mime + "; codecs=\"vp08\"'")); Also, "vp09" and the most basic (i.e. guaranteed supported profile) "vp08....." https://codereview.chromium.org/1624703002/diff/260001/content/browser/media/... content/browser/media/media_canplaytype_browsertest.cc:470: EXPECT_EQ(kNot, Then duplicate here for consistency https://codereview.chromium.org/1624703002/diff/260001/content/browser/media/... content/browser/media/media_canplaytype_browsertest.cc:762: EXPECT_EQ(kNot, CanPlay("'video/mp4; codecs=\"vp8\"'")); See the first comment above - These could have been put in TestMPEGUnacceptableCombinations() since they are never valid. "vp08...." as well.
The CQ bit was unchecked by kqyang@chromium.org
PTAL https://codereview.chromium.org/1624703002/diff/260001/content/browser/media/... File content/browser/media/media_canplaytype_browsertest.cc (right): https://codereview.chromium.org/1624703002/diff/260001/content/browser/media/... content/browser/media/media_canplaytype_browsertest.cc:229: EXPECT_EQ(kNot, CanPlay("'" + mime + "; codecs=\"vp8\"'")); On 2016/04/19 21:11:46, ddorwin wrote: > Add vp8.0, vp08, vp9.0, vp09 > > This can replace most of the new lines in this patch set. Didn't know that there was a common negative test for MPEG. Done. https://codereview.chromium.org/1624703002/diff/260001/content/browser/media/... content/browser/media/media_canplaytype_browsertest.cc:236: EXPECT_EQ(kNot, CanPlay("'" + mime + "; codecs=\"vp9, mp4a.40.02\"'")); On 2016/04/19 21:11:46, ddorwin wrote: > Also add the most basic (i.e. guaranteed supported profile) "vp08....." Done. https://codereview.chromium.org/1624703002/diff/260001/content/browser/media/... content/browser/media/media_canplaytype_browsertest.cc:349: EXPECT_EQ(kNot, On 2016/04/19 21:11:46, ddorwin wrote: > Duplicate the new and suggested lines in the WEBM function below. Done. https://codereview.chromium.org/1624703002/diff/260001/content/browser/media/... content/browser/media/media_canplaytype_browsertest.cc:407: EXPECT_EQ(kNot, CanPlay("'" + mime + "; codecs=\"vp08\"'")); On 2016/04/19 21:11:46, ddorwin wrote: > Also, "vp09" and the most basic (i.e. guaranteed supported profile) "vp08....." Done. https://codereview.chromium.org/1624703002/diff/260001/content/browser/media/... content/browser/media/media_canplaytype_browsertest.cc:470: EXPECT_EQ(kNot, On 2016/04/19 21:11:46, ddorwin wrote: > Then duplicate here for consistency Done. https://codereview.chromium.org/1624703002/diff/260001/content/browser/media/... content/browser/media/media_canplaytype_browsertest.cc:762: EXPECT_EQ(kNot, CanPlay("'video/mp4; codecs=\"vp8\"'")); On 2016/04/19 21:11:46, ddorwin wrote: > See the first comment above - These could have been put in > TestMPEGUnacceptableCombinations() since they are never valid. > "vp08...." as well. Done.
PTAL
lgtm Thanks!
The CQ bit was checked by kqyang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sandersd@chromium.org Link to the patchset: https://codereview.chromium.org/1624703002/#ps280001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1624703002/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1624703002/280001
Message was sent while issue was closed.
Description was changed from ========== Implement support for vp9 in ISO-BMFF The feature is implemented under flag ENABLE_MP4_VP9_DEMUXING, off for now. BUG=580623 ========== to ========== Implement support for vp9 in ISO-BMFF The feature is implemented under flag ENABLE_MP4_VP9_DEMUXING, off for now. BUG=580623 ==========
Message was sent while issue was closed.
Committed patchset #15 (id:280001)
Message was sent while issue was closed.
A revert of this CL (patchset #15 id:280001) has been created in https://codereview.chromium.org/1899423002/ by msramek@chromium.org. The reason for reverting is: Suspect for perf regression: increased the number of static initializers (in stream_parser_factory.cc). https://build.chromium.org/p/chromium/builders/Mac/builds/14593 https://build.chromium.org/p/chromium/builders/Linux%20x64/builds/18412 See crbug.com/605017 for more details..
Message was sent while issue was closed.
On 2016/04/20 07:22:51, msramek wrote: > A revert of this CL (patchset #15 id:280001) has been created in > https://codereview.chromium.org/1899423002/ by mailto:msramek@chromium.org. > > The reason for reverting is: Suspect for perf regression: increased the number > of static initializers (in stream_parser_factory.cc). > > https://build.chromium.org/p/chromium/builders/Mac/builds/14593 > https://build.chromium.org/p/chromium/builders/Linux%20x64/builds/18412 > > See crbug.com/605017 for more details.. This was a wrong call. Sorry for that! Relanding in https://codereview.chromium.org/1897363004/
Message was sent while issue was closed.
Description was changed from ========== Implement support for vp9 in ISO-BMFF The feature is implemented under flag ENABLE_MP4_VP9_DEMUXING, off for now. BUG=580623 ========== to ========== Implement support for vp9 in ISO-BMFF The feature is implemented under flag ENABLE_MP4_VP9_DEMUXING, off for now. BUG=580623 Committed: https://crrev.com/f66cf8a7b3e8f169a58c09e6627b0cdc11192d2f Cr-Commit-Position: refs/heads/master@{#388363} ==========
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/f66cf8a7b3e8f169a58c09e6627b0cdc11192d2f Cr-Commit-Position: refs/heads/master@{#388363} |
