Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2495193004:
Refactor VP9 codec string parsing (Closed)
|
Created:
4 years, 1 month ago by kqyang Modified:
4 years ago CC:
chromium-reviews, creis+watch_chromium.org, posciak+watch_chromium.org, nasko+codewatch_chromium.org, jam, feature-media-reviews_chromium.org, darin-cc_chromium.org, mcasas+watch+vc_chromium.org, tinskip1, fgalligan1 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRefactor VP9 codec string parsing
- Also fixed source_buffer_state failed to find codec with vp9 in mp4 contents when VP9 in MP4 is enabled.
- And added a few playback/demuxer tests.
BUG=580623
Committed: https://crrev.com/f93bb7be32da55f1ea60694b2ec803f3ac0cdf9f
Cr-Commit-Position: refs/heads/master@{#436144}
Patch Set 1 #Patch Set 2 : Fix build break due to removal of include header #Patch Set 3 : Fix break in source_buffer_state failed to find codec" #Patch Set 4 : Add additional tests #Patch Set 5 : Keep vp9 in mp4 disabled #Patch Set 6 : Fix test #
Total comments: 24
Patch Set 7 : Address comments #Patch Set 8 : fix CodecSupportTest_mp4 test #
Total comments: 21
Patch Set 9 : Address comments again #
Total comments: 6
Patch Set 10 : Added a new test in chunk_demuxer_unittest #
Total comments: 14
Patch Set 11 : Address comments #
Total comments: 2
Patch Set 12 : Add a comment to check |level_idc|. #
Messages
Total messages: 77 (54 generated)
Description was changed from ========== Remove --enable-vp9-in-mp4 and enable it by default BUG=580623 ========== to ========== Remove --enable-vp9-in-mp4 and enable it by default BUG=580623 ==========
kqyang@chromium.org changed reviewers: + ddorwin@chromium.org, wolenetz@chromium.org
The CQ bit was checked by kqyang@chromium.org to run a CQ dry run
Please review. Thanks.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by kqyang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by kqyang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by kqyang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by kqyang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Remove --enable-vp9-in-mp4 and enable it by default BUG=580623 ========== to ========== Also added a few playback tests. Note that --enable-vp9-in-mp4 is still disabled by default. BUG=580623 ==========
I updated this cl to fix code break instead. I have also added a few tests with vp9 in mp4 enabled in the tests to make sure the code is working. Please review.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Please be more specific about the break. You can probably leave out the switch if you need more characters. Also, the subject in the code review tool does not get included in the commit message so you need to include it in the description. (This is different than, for example, GitHub.) https://codereview.chromium.org/2495193004/diff/100001/chrome/browser/media/e... File chrome/browser/media/encrypted_media_browsertest.cc (right): https://codereview.chromium.org/2495193004/diff/100001/chrome/browser/media/e... chrome/browser/media/encrypted_media_browsertest.cc:556: IN_PROC_BROWSER_TEST_P(EncryptedMediaTest, Playback_Vp9_VideoOnly_MP4) { Probably add _Vp9 to the end instead. https://codereview.chromium.org/2495193004/diff/100001/content/browser/media/... File content/browser/media/media_browsertest.cc (right): https://codereview.chromium.org/2495193004/diff/100001/content/browser/media/... content/browser/media/media_browsertest.cc:155: IN_PROC_BROWSER_TEST_P(MediaTest, VideoBearVp9Mp4) { Maybe ...Mp4Vp9 or even ...Mp4_Vp9. https://codereview.chromium.org/2495193004/diff/100001/media/base/mime_util_i... File media/base/mime_util_internal.cc (right): https://codereview.chromium.org/2495193004/diff/100001/media/base/mime_util_i... media/base/mime_util_internal.cc:624: if (mime_type_lower_case == "video/webm") { Explain what this is doing in a comment. Probably inside the block (i.e. on the next line). https://codereview.chromium.org/2495193004/diff/100001/media/base/mime_util_i... media/base/mime_util_internal.cc:625: if (codec_id != "vp9" && codec_id != "vp9.0") { This logic is duplicated in ParseVp9CodecID(), and it seems strange to even call ParseVp9CodecID() for the old style. For now, perhaps we should only call ParseVp9CodecID() for "video/mp4". We might rename it "NewStyle" or something like that. If we do later support the new style for WebM, we can refactor to check the old style if ParseVp9CodecID() fails and "video/webm". https://codereview.chromium.org/2495193004/diff/100001/media/base/mime_util_i... media/base/mime_util_internal.cc:629: } else if (codec_id == "vp9" || codec_id == "vp9.0") { The above suggestion would eliminate this third instance of these strings too I think. https://codereview.chromium.org/2495193004/diff/100001/media/base/video_codec... File media/base/video_codecs.cc (right): https://codereview.chromium.org/2495193004/diff/100001/media/base/video_codec... media/base/video_codecs.cc:180: bool ParseVp9CodecID(const std::string& codec_id, VideoCodecProfile* profile) { As mentioned earlier, we should probably rename this and remove the first block below. https://codereview.chromium.org/2495193004/diff/100001/media/base/video_codec... media/base/video_codecs.cc:188: if (!base::CommandLine::ForCurrentProcess()->HasSwitch( We should also move this to mime_util_internal.cc once we've made the change above. Codec parsing doesn't need to depend on switches. https://codereview.chromium.org/2495193004/diff/100001/media/base/video_codec... media/base/video_codecs.cc:201: if (fields.size() > 8) File a bug to finalize how we handle the fields and check whether they are supported then reference it in a TODO here. https://codereview.chromium.org/2495193004/diff/100001/media/base/video_codec... media/base/video_codecs.cc:218: // We do not allow missing fields. This is important to now by line 206. https://codereview.chromium.org/2495193004/diff/100001/media/base/video_codec... media/base/video_codecs.cc:219: if (values.size() < 7) We have three checks for the size, but it all boils down to == 8. Having a single check at 195 would allow us to move the comment above up and be a general comment. https://codereview.chromium.org/2495193004/diff/100001/media/base/video_codec... media/base/video_codecs.cc:384: if (ParseVp9CodecID(codec_id, &profile)) Hmm, I guess this is a problem for my proposal above. Perhaps we need a Legacy and New function and to OR them here. https://codereview.chromium.org/2495193004/diff/100001/media/base/video_codecs.h File media/base/video_codecs.h (right): https://codereview.chromium.org/2495193004/diff/100001/media/base/video_codec... media/base/video_codecs.h:86: // Handle parsing of vp9 codec IDs. It seems better to keep AVC and HEVC together. Maybe move this first. Ditto in the .cc file.
I'm deferring CR to others on the team (ddorwin@). If I'm really needed on this review, add me back to R=.
wolenetz@chromium.org changed reviewers: - wolenetz@chromium.org
Description was changed from ========== Also added a few playback tests. Note that --enable-vp9-in-mp4 is still disabled by default. BUG=580623 ========== to ========== Fix source_buffer_state failed to find codec with vp9 in mp4 contents when --enable-vp9-in-mp4 is enabled. Also added a few playback tests and cleanup the codes. Note that --enable-vp9-in-mp4 is still disabled by default. BUG=580623 ==========
The CQ bit was checked by kqyang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by kqyang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL https://codereview.chromium.org/2495193004/diff/100001/chrome/browser/media/e... File chrome/browser/media/encrypted_media_browsertest.cc (right): https://codereview.chromium.org/2495193004/diff/100001/chrome/browser/media/e... chrome/browser/media/encrypted_media_browsertest.cc:556: IN_PROC_BROWSER_TEST_P(EncryptedMediaTest, Playback_Vp9_VideoOnly_MP4) { On 2016/11/22 01:34:42, ddorwin wrote: > Probably add _Vp9 to the end instead. Done. https://codereview.chromium.org/2495193004/diff/100001/content/browser/media/... File content/browser/media/media_browsertest.cc (right): https://codereview.chromium.org/2495193004/diff/100001/content/browser/media/... content/browser/media/media_browsertest.cc:155: IN_PROC_BROWSER_TEST_P(MediaTest, VideoBearVp9Mp4) { On 2016/11/22 01:34:42, ddorwin wrote: > Maybe ...Mp4Vp9 or even ...Mp4_Vp9. Done. https://codereview.chromium.org/2495193004/diff/100001/media/base/mime_util_i... File media/base/mime_util_internal.cc (right): https://codereview.chromium.org/2495193004/diff/100001/media/base/mime_util_i... media/base/mime_util_internal.cc:624: if (mime_type_lower_case == "video/webm") { On 2016/11/22 01:34:42, ddorwin wrote: > Explain what this is doing in a comment. Probably inside the block (i.e. on the > next line). no longer applicable https://codereview.chromium.org/2495193004/diff/100001/media/base/mime_util_i... media/base/mime_util_internal.cc:625: if (codec_id != "vp9" && codec_id != "vp9.0") { On 2016/11/22 01:34:42, ddorwin wrote: > This logic is duplicated in ParseVp9CodecID(), and it seems strange to even call > ParseVp9CodecID() for the old style. > > For now, perhaps we should only call ParseVp9CodecID() for "video/mp4". We might > rename it "NewStyle" or something like that. > > If we do later support the new style for WebM, we can refactor to check the old > style if ParseVp9CodecID() fails and "video/webm". Done. https://codereview.chromium.org/2495193004/diff/100001/media/base/mime_util_i... media/base/mime_util_internal.cc:629: } else if (codec_id == "vp9" || codec_id == "vp9.0") { On 2016/11/22 01:34:42, ddorwin wrote: > The above suggestion would eliminate this third instance of these strings too I > think. Done. https://codereview.chromium.org/2495193004/diff/100001/media/base/video_codec... File media/base/video_codecs.cc (right): https://codereview.chromium.org/2495193004/diff/100001/media/base/video_codec... media/base/video_codecs.cc:180: bool ParseVp9CodecID(const std::string& codec_id, VideoCodecProfile* profile) { On 2016/11/22 01:34:42, ddorwin wrote: > As mentioned earlier, we should probably rename this and remove the first block > below. Done. https://codereview.chromium.org/2495193004/diff/100001/media/base/video_codec... media/base/video_codecs.cc:188: if (!base::CommandLine::ForCurrentProcess()->HasSwitch( On 2016/11/22 01:34:42, ddorwin wrote: > We should also move this to mime_util_internal.cc once we've made the change > above. Codec parsing doesn't need to depend on switches. Done. https://codereview.chromium.org/2495193004/diff/100001/media/base/video_codec... media/base/video_codecs.cc:201: if (fields.size() > 8) On 2016/11/22 01:34:42, ddorwin wrote: > File a bug to finalize how we handle the fields and check whether they are > supported then reference it in a TODO here. Done. https://codereview.chromium.org/2495193004/diff/100001/media/base/video_codec... media/base/video_codecs.cc:218: // We do not allow missing fields. On 2016/11/22 01:34:42, ddorwin wrote: > This is important to now by line 206. Acknowledged. https://codereview.chromium.org/2495193004/diff/100001/media/base/video_codec... media/base/video_codecs.cc:219: if (values.size() < 7) On 2016/11/22 01:34:42, ddorwin wrote: > We have three checks for the size, but it all boils down to == 8. Having a > single check at 195 would allow us to move the comment above up and be a general > comment. The code was initially created to allow optional fields. That is why there are separate checks in different places... Done. https://codereview.chromium.org/2495193004/diff/100001/media/base/video_codec... media/base/video_codecs.cc:384: if (ParseVp9CodecID(codec_id, &profile)) On 2016/11/22 01:34:42, ddorwin wrote: > Hmm, I guess this is a problem for my proposal above. Perhaps we need a Legacy > and New function and to OR them here. Done. https://codereview.chromium.org/2495193004/diff/100001/media/base/video_codecs.h File media/base/video_codecs.h (right): https://codereview.chromium.org/2495193004/diff/100001/media/base/video_codec... media/base/video_codecs.h:86: // Handle parsing of vp9 codec IDs. On 2016/11/22 01:34:42, ddorwin wrote: > It seems better to keep AVC and HEVC together. Maybe move this first. Ditto in > the .cc file. Done.
https://codereview.chromium.org/2495193004/diff/140001/media/base/mime_util_i... File media/base/mime_util_internal.cc (right): https://codereview.chromium.org/2495193004/diff/140001/media/base/mime_util_i... media/base/mime_util_internal.cc:39: // ParseVp9CodecID(). Update https://codereview.chromium.org/2495193004/diff/140001/media/base/mime_util_i... media/base/mime_util_internal.cc:596: // Parse new style vp9 codec string in MP4. nit: VP9 https://codereview.chromium.org/2495193004/diff/140001/media/base/mime_util_i... media/base/mime_util_internal.cc:617: if (ParseLegacyVp9CodecID(codec_id, out_profile, out_level)) { If legacy supported profiles other than 0, we'd need to also call the code above. However, we don't support other levels and may not with this format, so we could DCHECK. Better yet, we could remove the two parameters and reject any other strings inside the function. The other option is to move this entire if-else block into a local ParseVp9CodecId() that moves the switch block to the end. That might be better anyway since we can refactor all this without messing with this higher level function. https://codereview.chromium.org/2495193004/diff/140001/media/base/mime_util_i... media/base/mime_util_internal.cc:625: // either H.264 or HEVC/H.265 codec ID because currently those are the only I think this comment applies to VP9 too and thus should be updated and moved above. https://codereview.chromium.org/2495193004/diff/140001/media/base/video_codec... File media/base/video_codecs.cc (right): https://codereview.chromium.org/2495193004/diff/140001/media/base/video_codec... media/base/video_codecs.cc:164: *level_idc = 1; Is level 1 the base? Or is there a level 0? https://codereview.chromium.org/2495193004/diff/140001/media/base/video_codec... media/base/video_codecs.cc:371: VideoCodec StringToVideoCodec(const std::string& codec_id) { This is called by SourceBufferState::Init(), which doesn't have container context. I'm not sure what it is used for, but I'm concerned that: a) There is no way to restrict codec string formats by container type. b) This does not block the new format based on the flag. This is a regression and something we should fix. +wolenetz@ for comments on whether this is a concern or somehow blocked by mime_util_internal.cc code. https://codereview.chromium.org/2495193004/diff/140001/media/base/video_codec... media/base/video_codecs.cc:380: if (ParseNewStyleVp9CodecID(codec_id, &profile, &level) || Ah, this is why we need the extra params. But maybe not since we don't use them. Thus, leaving them as the defaults is fine. This is moot if we choose the second option in _internal.cc.
The CQ bit was checked by kqyang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL https://codereview.chromium.org/2495193004/diff/140001/media/base/mime_util_i... File media/base/mime_util_internal.cc (right): https://codereview.chromium.org/2495193004/diff/140001/media/base/mime_util_i... media/base/mime_util_internal.cc:39: // ParseVp9CodecID(). On 2016/11/23 00:22:17, ddorwin wrote: > Update No longer applicable https://codereview.chromium.org/2495193004/diff/140001/media/base/mime_util_i... media/base/mime_util_internal.cc:596: // Parse new style vp9 codec string in MP4. On 2016/11/23 00:22:17, ddorwin wrote: > nit: VP9 Comment removed (not needed) https://codereview.chromium.org/2495193004/diff/140001/media/base/mime_util_i... media/base/mime_util_internal.cc:617: if (ParseLegacyVp9CodecID(codec_id, out_profile, out_level)) { On 2016/11/23 00:22:17, ddorwin wrote: > If legacy supported profiles other than 0, we'd need to also call the code > above. However, we don't support other levels and may not with this format, so > we could DCHECK. Better yet, we could remove the two parameters and reject any > other strings inside the function. > > The other option is to move this entire if-else block into a local > ParseVp9CodecId() that moves the switch block to the end. That might be better > anyway since we can refactor all this without messing with this higher level > function. Good idea. Done. https://codereview.chromium.org/2495193004/diff/140001/media/base/mime_util_i... media/base/mime_util_internal.cc:625: // either H.264 or HEVC/H.265 codec ID because currently those are the only On 2016/11/23 00:22:17, ddorwin wrote: > I think this comment applies to VP9 too and thus should be updated and moved > above. Good catch. https://codereview.chromium.org/2495193004/diff/140001/media/base/video_codec... File media/base/video_codecs.cc (right): https://codereview.chromium.org/2495193004/diff/140001/media/base/video_codec... media/base/video_codecs.cc:164: *level_idc = 1; On 2016/11/23 00:22:17, ddorwin wrote: > Is level 1 the base? Or is there a level 0? I think it is codec dependent. I guess 0 indicates unknown. I copied it (=1) from the original code. It does not seem level is currently used anywhere though. https://codereview.chromium.org/2495193004/diff/140001/media/base/video_codec... media/base/video_codecs.cc:380: if (ParseNewStyleVp9CodecID(codec_id, &profile, &level) || On 2016/11/23 00:22:17, ddorwin wrote: > Ah, this is why we need the extra params. But maybe not since we don't use them. > Thus, leaving them as the defaults is fine. This is moot if we choose the second > option in _internal.cc. Not sure what you meant here. I think it is good to keep the function signature consistent for old and new style functions.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
wolenetz@chromium.org changed reviewers: + wolenetz@chromium.org
Not a full review, just responding to the comment mentioning me: https://codereview.chromium.org/2495193004/diff/140001/media/base/video_codec... File media/base/video_codecs.cc (right): https://codereview.chromium.org/2495193004/diff/140001/media/base/video_codec... media/base/video_codecs.cc:371: VideoCodec StringToVideoCodec(const std::string& codec_id) { On 2016/11/23 00:22:17, ddorwin wrote: > This is called by SourceBufferState::Init(), which doesn't have container > context. I'm not sure what it is used for, but I'm concerned that: > a) There is no way to restrict codec string formats by container type. > b) This does not block the new format based on the flag. This is a regression > and something we should fix. > > +wolenetz@ for comments on whether this is a concern or somehow blocked by > mime_util_internal.cc code. The only call-chain (outside of tests) of this method is ChunkDemuxer::AddId -> SourceBufferState::Init -> here AddId first calls media::ParseCodecString (https://cs.chromium.org/chromium/src/media/filters/chunk_demuxer.cc?rcl=14799...) (similarly with no container context) And then attempts construction of an appropriate StreamParser (https://cs.chromium.org/chromium/src/media/filters/chunk_demuxer.cc?rcl=14799...) (this *does* have container context) If StreamParserFactory's kVideoMP4Codecs is unconditionally including VP9, that might be a problem. If either of those fail, then no SourceBufferState::Init is attempted. Also, ChunkDemuxer::AddId is called only after MediaSource::addSourceBuffer has first confirmed that MediaSource::isTypeSupported() has succeeded (https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/medias...) and MediaSource::isTypeSupported() will fail if HTMLMediaElement::supportsType() fails (IOW, if canPlayType() returns IsNotSupported). That path involves MimeUtil::IsSupportedMediaFormat(), which *does* have container context. Yes, a bit convoluted. If StreamParserFactory doesn't condition kVideoMP4Codecs on the flag, then that should be fixed as part of this change to at least make the expectation around the MSE side clearer. chunk_demuxer_unittests should be included to condition their expectation around AddId ( mp4 container with vp9 codec ) upon the flag. Ideally, the flag would be configurable on bots and layout tests could be added with virtual suites that vary that flag, but that might be overkill :)
On 2016/11/23 22:54:18, wolenetz wrote: > If StreamParserFactory doesn't condition kVideoMP4Codecs on the flag, then that > should be fixed as part of this change to at least make the expectation around > the MSE side clearer. chunk_demuxer_unittests should be included to condition > their expectation around AddId ( mp4 container with vp9 codec ) upon the flag. > Ideally, the flag would be configurable on bots and layout tests could be added > with virtual suites that vary that flag, but that might be overkill :) StreamParserFactory does condition kVideoMP4Codecs on --enable-vp9-in-mp4. See: https://cs.chromium.org/chromium/src/media/filters/stream_parser_factory.cc?r.... I can add a test in chunk_demuxer_unittests when I am back to office next week if it is necessary.
Two new comments in PS8. I would mention that this refactors the VP9 codec string parsing code in the description. That is a majority of the work. Other description nits: * ... when VP9 in MP4 is enabled. * Remove " Note that --enable-vp9-in-mp4 is still disabled by default." https://codereview.chromium.org/2495193004/diff/140001/media/base/video_codec... File media/base/video_codecs.cc (right): https://codereview.chromium.org/2495193004/diff/140001/media/base/video_codec... media/base/video_codecs.cc:164: *level_idc = 1; On 2016/11/23 01:08:20, kqyang wrote: > On 2016/11/23 00:22:17, ddorwin wrote: > > Is level 1 the base? Or is there a level 0? > > I think it is codec dependent. I guess 0 indicates unknown. I copied it (=1) > from the original code. > > It does not seem level is currently used anywhere though. https://www.webmproject.org/vp9/levels/ shows levels start at 1. However, https://storage.googleapis.com/downloads.webmproject.org/docs/vp9/vp-codec-is... says "The value is 0 if a codec level is not specified." That is for the new format, but maybe we should use 0 here too. OTOH, this is just supposed to work, so maybe we should stick with 1 and add a comment to assume the lowest. https://codereview.chromium.org/2495193004/diff/140001/media/base/video_codec... media/base/video_codecs.cc:371: VideoCodec StringToVideoCodec(const std::string& codec_id) { On 2016/11/23 22:54:18, wolenetz_ooo_nov28 wrote: > On 2016/11/23 00:22:17, ddorwin wrote: > > This is called by SourceBufferState::Init(), which doesn't have container > > context. I'm not sure what it is used for, but I'm concerned that: > > a) There is no way to restrict codec string formats by container type. > > b) This does not block the new format based on the flag. This is a regression > > and something we should fix. > > > > +wolenetz@ for comments on whether this is a concern or somehow blocked by > > mime_util_internal.cc code. > > The only call-chain (outside of tests) of this method is ChunkDemuxer::AddId -> > SourceBufferState::Init -> here > > AddId first calls media::ParseCodecString > (https://cs.chromium.org/chromium/src/media/filters/chunk_demuxer.cc?rcl=14799...) > (similarly with no container context) > And then attempts construction of an appropriate StreamParser > (https://cs.chromium.org/chromium/src/media/filters/chunk_demuxer.cc?rcl=14799...) > (this *does* have container context) If StreamParserFactory's kVideoMP4Codecs is > unconditionally including VP9, that might be a problem. > > If either of those fail, then no SourceBufferState::Init is attempted. > > Also, ChunkDemuxer::AddId is called only after MediaSource::addSourceBuffer has > first confirmed that MediaSource::isTypeSupported() has succeeded > (https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/medias...) > > and MediaSource::isTypeSupported() will fail if HTMLMediaElement::supportsType() > fails (IOW, if canPlayType() returns IsNotSupported). That path involves > MimeUtil::IsSupportedMediaFormat(), which *does* have container context. > > Yes, a bit convoluted. > > If StreamParserFactory doesn't condition kVideoMP4Codecs on the flag, then that > should be fixed as part of this change to at least make the expectation around > the MSE side clearer. chunk_demuxer_unittests should be included to condition > their expectation around AddId ( mp4 container with vp9 codec ) upon the flag. > Ideally, the flag would be configurable on bots and layout tests could be added > with virtual suites that vary that flag, but that might be overkill :) > kqyang, please consider the above, especially the conditional inclusion of VP9 with kVideoMP4Codecs. https://codereview.chromium.org/2495193004/diff/160001/media/base/video_codec... File media/base/video_codecs.cc (right): https://codereview.chromium.org/2495193004/diff/160001/media/base/video_codec... media/base/video_codecs.cc:132: *level_idc = values[1]; According to https://www.webmproject.org/vp9/levels/, levels are decimals (e.g., 1.1). It's not clear how that would be represented and https://storage.googleapis.com/downloads.webmproject.org/docs/vp9/vp-codec-is... does not appear to allow for this. https://codereview.chromium.org/2495193004/diff/160001/media/base/video_codec... media/base/video_codecs.cc:380: if (ParseNewStyleVp9CodecID(codec_id, &profile, &level) || Is this line what the current commit message is referring to? https://codereview.chromium.org/2495193004/diff/160001/media/base/video_codecs.h File media/base/video_codecs.h (right): https://codereview.chromium.org/2495193004/diff/160001/media/base/video_codec... media/base/video_codecs.h:81: // Handle parsing of vp9 codec IDs. We should describe the reason for each of these, but maybe that should wait for a final conclusion and definition. For now, maybe a TODO to provide context.
https://codereview.chromium.org/2495193004/diff/140001/media/base/video_codec... File media/base/video_codecs.cc (right): https://codereview.chromium.org/2495193004/diff/140001/media/base/video_codec... media/base/video_codecs.cc:371: VideoCodec StringToVideoCodec(const std::string& codec_id) { On 2016/11/28 22:08:51, ddorwin wrote: > On 2016/11/23 22:54:18, wolenetz_ooo_nov28 wrote: > > On 2016/11/23 00:22:17, ddorwin wrote: > > > This is called by SourceBufferState::Init(), which doesn't have container > > > context. I'm not sure what it is used for, but I'm concerned that: > > > a) There is no way to restrict codec string formats by container type. > > > b) This does not block the new format based on the flag. This is a > regression > > > and something we should fix. > > > > > > +wolenetz@ for comments on whether this is a concern or somehow blocked by > > > mime_util_internal.cc code. > > > > The only call-chain (outside of tests) of this method is ChunkDemuxer::AddId > -> > > SourceBufferState::Init -> here > > > > AddId first calls media::ParseCodecString > > > (https://cs.chromium.org/chromium/src/media/filters/chunk_demuxer.cc?rcl=14799...) > > (similarly with no container context) > > And then attempts construction of an appropriate StreamParser > > > (https://cs.chromium.org/chromium/src/media/filters/chunk_demuxer.cc?rcl=14799...) > > (this *does* have container context) If StreamParserFactory's kVideoMP4Codecs > is > > unconditionally including VP9, that might be a problem. > > > > If either of those fail, then no SourceBufferState::Init is attempted. > > > > Also, ChunkDemuxer::AddId is called only after MediaSource::addSourceBuffer > has > > first confirmed that MediaSource::isTypeSupported() has succeeded > > > (https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/medias...) > > > > and MediaSource::isTypeSupported() will fail if > HTMLMediaElement::supportsType() > > fails (IOW, if canPlayType() returns IsNotSupported). That path involves > > MimeUtil::IsSupportedMediaFormat(), which *does* have container context. > > > > Yes, a bit convoluted. > > > > If StreamParserFactory doesn't condition kVideoMP4Codecs on the flag, then > that > > should be fixed as part of this change to at least make the expectation around > > the MSE side clearer. chunk_demuxer_unittests should be included to condition > > their expectation around AddId ( mp4 container with vp9 codec ) upon the flag. > > Ideally, the flag would be configurable on bots and layout tests could be > added > > with virtual suites that vary that flag, but that might be overkill :) > > > > kqyang, please consider the above, especially the conditional inclusion of VP9 > with kVideoMP4Codecs. I missed this response that was a general reply and not included in the thread: >StreamParserFactory does condition kVideoMP4Codecs on --enable-vp9-in-mp4. See: >https://cs.chromium.org/chromium/src/media/filters/stream_parser_factory.cc?rcl=1479917866&l=113. >I can add a test in chunk_demuxer_unittests when I am back to office next week >if it is necessary. wolenetz@, please make a call on what else needs to be done. It would be nice if this was not so complex and could be verified by the code. I'm not sure we want to introduce container types to that code path, though.
The CQ bit was checked by kqyang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Fix source_buffer_state failed to find codec with vp9 in mp4 contents when --enable-vp9-in-mp4 is enabled. Also added a few playback tests and cleanup the codes. Note that --enable-vp9-in-mp4 is still disabled by default. BUG=580623 ========== to ========== Refactor VP9 codec string parsing - Also fixed source_buffer_state failed to find codec with vp9 in mp4 contents when VP9 in MP4 is enabled. - And added a few playback/demuxer tests. BUG=580623 ==========
PTAL https://codereview.chromium.org/2495193004/diff/140001/media/base/video_codec... File media/base/video_codecs.cc (right): https://codereview.chromium.org/2495193004/diff/140001/media/base/video_codec... media/base/video_codecs.cc:164: *level_idc = 1; On 2016/11/28 22:08:51, ddorwin wrote: > On 2016/11/23 01:08:20, kqyang wrote: > > On 2016/11/23 00:22:17, ddorwin wrote: > > > Is level 1 the base? Or is there a level 0? > > > > I think it is codec dependent. I guess 0 indicates unknown. I copied it (=1) > > from the original code. > > > > It does not seem level is currently used anywhere though. > > https://www.webmproject.org/vp9/levels/ shows levels start at 1. However, > https://storage.googleapis.com/downloads.webmproject.org/docs/vp9/vp-codec-is... > says "The value is 0 if a codec level is not specified." That is for the new > format, but maybe we should use 0 here too. > > OTOH, this is just supposed to work, so maybe we should stick with 1 and add a > comment to assume the lowest. I'll change it to 0 for now, which seems to be more correct. Level starts with 1, which is represented as "10" actually: https://docs.google.com/document/d/1Nn22IpMEOxBqRSVFfSvzmq1ZRL0s3d_1OTvWZx43G... @fgalligan, what do you think? https://codereview.chromium.org/2495193004/diff/140001/media/base/video_codec... media/base/video_codecs.cc:371: VideoCodec StringToVideoCodec(const std::string& codec_id) { On 2016/11/28 22:24:10, ddorwin wrote: > On 2016/11/28 22:08:51, ddorwin wrote: > > On 2016/11/23 22:54:18, wolenetz_ooo_nov28 wrote: > > > On 2016/11/23 00:22:17, ddorwin wrote: > > > > This is called by SourceBufferState::Init(), which doesn't have container > > > > context. I'm not sure what it is used for, but I'm concerned that: > > > > a) There is no way to restrict codec string formats by container type. > > > > b) This does not block the new format based on the flag. This is a > > regression > > > > and something we should fix. > > > > > > > > +wolenetz@ for comments on whether this is a concern or somehow blocked by > > > > mime_util_internal.cc code. > > > > > > The only call-chain (outside of tests) of this method is ChunkDemuxer::AddId > > -> > > > SourceBufferState::Init -> here > > > > > > AddId first calls media::ParseCodecString > > > > > > (https://cs.chromium.org/chromium/src/media/filters/chunk_demuxer.cc?rcl=14799...) > > > (similarly with no container context) > > > And then attempts construction of an appropriate StreamParser > > > > > > (https://cs.chromium.org/chromium/src/media/filters/chunk_demuxer.cc?rcl=14799...) > > > (this *does* have container context) If StreamParserFactory's > kVideoMP4Codecs > > is > > > unconditionally including VP9, that might be a problem. > > > > > > If either of those fail, then no SourceBufferState::Init is attempted. > > > > > > Also, ChunkDemuxer::AddId is called only after MediaSource::addSourceBuffer > > has > > > first confirmed that MediaSource::isTypeSupported() has succeeded > > > > > > (https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/medias...) > > > > > > and MediaSource::isTypeSupported() will fail if > > HTMLMediaElement::supportsType() > > > fails (IOW, if canPlayType() returns IsNotSupported). That path involves > > > MimeUtil::IsSupportedMediaFormat(), which *does* have container context. > > > > > > Yes, a bit convoluted. > > > > > > If StreamParserFactory doesn't condition kVideoMP4Codecs on the flag, then > > that > > > should be fixed as part of this change to at least make the expectation > around > > > the MSE side clearer. chunk_demuxer_unittests should be included to > condition > > > their expectation around AddId ( mp4 container with vp9 codec ) upon the > flag. > > > Ideally, the flag would be configurable on bots and layout tests could be > > added > > > with virtual suites that vary that flag, but that might be overkill :) > > > > > > > kqyang, please consider the above, especially the conditional inclusion of VP9 > > with kVideoMP4Codecs. > > I missed this response that was a general reply and not included in the thread: > > >StreamParserFactory does condition kVideoMP4Codecs on --enable-vp9-in-mp4. See: > >https://cs.chromium.org/chromium/src/media/filters/stream_parser_factory.cc?rcl=1479917866&l=113. > >I can add a test in chunk_demuxer_unittests when I am back to office next week > >if it is necessary. > > wolenetz@, please make a call on what else needs to be done. It would be nice if > this was not so complex and could be verified by the code. I'm not sure we want > to introduce container types to that code path, though. Added a test in chunk_demuxer_unittests. https://codereview.chromium.org/2495193004/diff/160001/media/base/video_codec... File media/base/video_codecs.cc (right): https://codereview.chromium.org/2495193004/diff/160001/media/base/video_codec... media/base/video_codecs.cc:132: *level_idc = values[1]; On 2016/11/28 22:08:51, ddorwin wrote: > According to https://www.webmproject.org/vp9/levels/, levels are decimals (e.g., > 1.1). It's not clear how that would be represented and > https://storage.googleapis.com/downloads.webmproject.org/docs/vp9/vp-codec-is... > does not appear to allow for this. See https://docs.google.com/document/d/1Nn22IpMEOxBqRSVFfSvzmq1ZRL0s3d_1OTvWZx43G..., the value is multiplied by 10 when represented in uint8_t. https://codereview.chromium.org/2495193004/diff/160001/media/base/video_codec... media/base/video_codecs.cc:380: if (ParseNewStyleVp9CodecID(codec_id, &profile, &level) || On 2016/11/28 22:08:51, ddorwin wrote: > Is this line what the current commit message is referring to? Yes https://codereview.chromium.org/2495193004/diff/160001/media/base/video_codecs.h File media/base/video_codecs.h (right): https://codereview.chromium.org/2495193004/diff/160001/media/base/video_codec... media/base/video_codecs.h:81: // Handle parsing of vp9 codec IDs. On 2016/11/28 22:08:52, ddorwin wrote: > We should describe the reason for each of these, but maybe that should wait for > a final conclusion and definition. For now, maybe a TODO to provide context. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
LGTM with comments. Thanks. https://codereview.chromium.org/2495193004/diff/180001/media/base/video_codec... File media/base/video_codecs.cc (right): https://codereview.chromium.org/2495193004/diff/180001/media/base/video_codec... media/base/video_codecs.cc:131: Do we have a separate bug to actually check the values? We should and included it in a TODO here. https://codereview.chromium.org/2495193004/diff/180001/media/base/video_codec... media/base/video_codecs.cc:132: *level_idc = values[1]; An easy check is that this can't be less than 10, right? Probably with a comment about the multiplication. https://codereview.chromium.org/2495193004/diff/180001/media/base/video_codecs.h File media/base/video_codecs.h (right): https://codereview.chromium.org/2495193004/diff/180001/media/base/video_codec... media/base/video_codecs.h:82: // Codec ISO Media File Format Binding specification: maybe add "proposed". https://codereview.chromium.org/2495193004/diff/180001/media/base/video_codec... media/base/video_codecs.h:86: // TODO(kqyang): Consolidate the two functions once we addressed nit: address https://codereview.chromium.org/2495193004/diff/180001/media/filters/chunk_de... File media/filters/chunk_demuxer_unittest.cc (right): https://codereview.chromium.org/2495193004/diff/180001/media/filters/chunk_de... media/filters/chunk_demuxer_unittest.cc:4780: demuxer_->AddId("source_id", "video/mp4", "vp09.00.01.08.02.01.01.00"), This does not have a valid level. I imagine the other test cases do not either.
PTAL https://codereview.chromium.org/2495193004/diff/180001/media/base/video_codec... File media/base/video_codecs.cc (right): https://codereview.chromium.org/2495193004/diff/180001/media/base/video_codec... media/base/video_codecs.cc:131: On 2016/11/29 23:48:51, ddorwin wrote: > Do we have a separate bug to actually check the values? We should and included > it in a TODO here. No. We haven't decided whether we want to validate every fields. How about settle crbug.com/667834 first and if needed we can open additional bugs later? https://codereview.chromium.org/2495193004/diff/180001/media/base/video_codec... media/base/video_codecs.cc:132: *level_idc = values[1]; On 2016/11/29 23:48:51, ddorwin wrote: > An easy check is that this can't be less than 10, right? Probably with a comment > about the multiplication. This mapping (actual level = level / 10) does not seem to be available publicly anywhere right now. I think we should include the mapping in VP9 in ISO-BMFF spec. We can then do additional validations here after finalizing the spec. https://codereview.chromium.org/2495193004/diff/180001/media/base/video_codecs.h File media/base/video_codecs.h (right): https://codereview.chromium.org/2495193004/diff/180001/media/base/video_codec... media/base/video_codecs.h:82: // Codec ISO Media File Format Binding specification: On 2016/11/29 23:48:51, ddorwin wrote: > maybe add "proposed". Done. https://codereview.chromium.org/2495193004/diff/180001/media/base/video_codec... media/base/video_codecs.h:86: // TODO(kqyang): Consolidate the two functions once we addressed On 2016/11/29 23:48:51, ddorwin wrote: > nit: address Done. https://codereview.chromium.org/2495193004/diff/180001/media/filters/chunk_de... File media/filters/chunk_demuxer_unittest.cc (right): https://codereview.chromium.org/2495193004/diff/180001/media/filters/chunk_de... media/filters/chunk_demuxer_unittest.cc:4780: demuxer_->AddId("source_id", "video/mp4", "vp09.00.01.08.02.01.01.00"), On 2016/11/29 23:48:51, ddorwin wrote: > This does not have a valid level. I imagine the other test cases do not either. I'd prefer we update it after we include the "level" mapping or possible values of level in vp9 in iso-bmff spec.
The CQ bit was checked by kqyang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: blimp_linux_dbg on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
On 2016/11/28 22:24:10, ddorwin wrote: > https://codereview.chromium.org/2495193004/diff/140001/media/base/video_codec... > File media/base/video_codecs.cc (right): > > https://codereview.chromium.org/2495193004/diff/140001/media/base/video_codec... > media/base/video_codecs.cc:371: VideoCodec StringToVideoCodec(const std::string& > codec_id) { > On 2016/11/28 22:08:51, ddorwin wrote: > > On 2016/11/23 22:54:18, wolenetz_ooo_nov28 wrote: > > > On 2016/11/23 00:22:17, ddorwin wrote: > > > > This is called by SourceBufferState::Init(), which doesn't have container > > > > context. I'm not sure what it is used for, but I'm concerned that: > > > > a) There is no way to restrict codec string formats by container type. > > > > b) This does not block the new format based on the flag. This is a > > regression > > > > and something we should fix. > > > > > > > > +wolenetz@ for comments on whether this is a concern or somehow blocked by > > > > mime_util_internal.cc code. > > > > > > The only call-chain (outside of tests) of this method is ChunkDemuxer::AddId > > -> > > > SourceBufferState::Init -> here > > > > > > AddId first calls media::ParseCodecString > > > > > > (https://cs.chromium.org/chromium/src/media/filters/chunk_demuxer.cc?rcl=14799...) > > > (similarly with no container context) > > > And then attempts construction of an appropriate StreamParser > > > > > > (https://cs.chromium.org/chromium/src/media/filters/chunk_demuxer.cc?rcl=14799...) > > > (this *does* have container context) If StreamParserFactory's > kVideoMP4Codecs > > is > > > unconditionally including VP9, that might be a problem. > > > > > > If either of those fail, then no SourceBufferState::Init is attempted. > > > > > > Also, ChunkDemuxer::AddId is called only after MediaSource::addSourceBuffer > > has > > > first confirmed that MediaSource::isTypeSupported() has succeeded > > > > > > (https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/medias...) > > > > > > and MediaSource::isTypeSupported() will fail if > > HTMLMediaElement::supportsType() > > > fails (IOW, if canPlayType() returns IsNotSupported). That path involves > > > MimeUtil::IsSupportedMediaFormat(), which *does* have container context. > > > > > > Yes, a bit convoluted. > > > > > > If StreamParserFactory doesn't condition kVideoMP4Codecs on the flag, then > > that > > > should be fixed as part of this change to at least make the expectation > around > > > the MSE side clearer. chunk_demuxer_unittests should be included to > condition > > > their expectation around AddId ( mp4 container with vp9 codec ) upon the > flag. > > > Ideally, the flag would be configurable on bots and layout tests could be > > added > > > with virtual suites that vary that flag, but that might be overkill :) > > > > > > > kqyang, please consider the above, especially the conditional inclusion of VP9 > > with kVideoMP4Codecs. > > I missed this response that was a general reply and not included in the thread: > > >StreamParserFactory does condition kVideoMP4Codecs on --enable-vp9-in-mp4. See: > >https://cs.chromium.org/chromium/src/media/filters/stream_parser_factory.cc?rcl=1479917866&l=113. > >I can add a test in chunk_demuxer_unittests when I am back to office next week > >if it is necessary. > > wolenetz@, please make a call on what else needs to be done. It would be nice if > this was not so complex and could be verified by the code. I'm not sure we want > to introduce container types to that code path, though. kqyang@: kVideoMP4Codecs conditioning looks good. Unifying MSE and src= mime+codec verification methods that service canPlayType (src=, also consulted by MSE), isTypeSupported (MSE), addSourceBuffer(MSE) might reduce complexity, but I'm unclear at the moment the right way forward there, and probably needs to happen in its own CL. Container type verification is already done in both paths, so I'm unclear what your comment's final sentence was meaning (unless maybe overriding build-flags to let test condition them at runtime? I suspect we don't want to do that since the build would thus contain ability for runtime selection...)
On 2016/12/01 03:07:25, wolenetz_slow_nov29 wrote: > On 2016/11/28 22:24:10, ddorwin wrote: > > > https://codereview.chromium.org/2495193004/diff/140001/media/base/video_codec... > > File media/base/video_codecs.cc (right): > > > > > https://codereview.chromium.org/2495193004/diff/140001/media/base/video_codec... > > media/base/video_codecs.cc:371: VideoCodec StringToVideoCodec(const > std::string& > > codec_id) { > > On 2016/11/28 22:08:51, ddorwin wrote: > > > On 2016/11/23 22:54:18, wolenetz_ooo_nov28 wrote: > > > > On 2016/11/23 00:22:17, ddorwin wrote: > > > > > This is called by SourceBufferState::Init(), which doesn't have > container > > > > > context. I'm not sure what it is used for, but I'm concerned that: > > > > > a) There is no way to restrict codec string formats by container type. > > > > > b) This does not block the new format based on the flag. This is a > > > regression > > > > > and something we should fix. > > > > > > > > > > +wolenetz@ for comments on whether this is a concern or somehow blocked > by > > > > > mime_util_internal.cc code. > > > > > > > > The only call-chain (outside of tests) of this method is > ChunkDemuxer::AddId > > > -> > > > > SourceBufferState::Init -> here > > > > > > > > AddId first calls media::ParseCodecString > > > > > > > > > > (https://cs.chromium.org/chromium/src/media/filters/chunk_demuxer.cc?rcl=14799...) > > > > (similarly with no container context) > > > > And then attempts construction of an appropriate StreamParser > > > > > > > > > > (https://cs.chromium.org/chromium/src/media/filters/chunk_demuxer.cc?rcl=14799...) > > > > (this *does* have container context) If StreamParserFactory's > > kVideoMP4Codecs > > > is > > > > unconditionally including VP9, that might be a problem. > > > > > > > > If either of those fail, then no SourceBufferState::Init is attempted. > > > > > > > > Also, ChunkDemuxer::AddId is called only after > MediaSource::addSourceBuffer > > > has > > > > first confirmed that MediaSource::isTypeSupported() has succeeded > > > > > > > > > > (https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/medias...) > > > > > > > > and MediaSource::isTypeSupported() will fail if > > > HTMLMediaElement::supportsType() > > > > fails (IOW, if canPlayType() returns IsNotSupported). That path involves > > > > MimeUtil::IsSupportedMediaFormat(), which *does* have container context. > > > > > > > > Yes, a bit convoluted. > > > > > > > > If StreamParserFactory doesn't condition kVideoMP4Codecs on the flag, then > > > that > > > > should be fixed as part of this change to at least make the expectation > > around > > > > the MSE side clearer. chunk_demuxer_unittests should be included to > > condition > > > > their expectation around AddId ( mp4 container with vp9 codec ) upon the > > flag. > > > > Ideally, the flag would be configurable on bots and layout tests could be > > > added > > > > with virtual suites that vary that flag, but that might be overkill :) > > > > > > > > > > kqyang, please consider the above, especially the conditional inclusion of > VP9 > > > with kVideoMP4Codecs. > > > > I missed this response that was a general reply and not included in the > thread: > > > > >StreamParserFactory does condition kVideoMP4Codecs on --enable-vp9-in-mp4. > See: > > > >https://cs.chromium.org/chromium/src/media/filters/stream_parser_factory.cc?rcl=1479917866&l=113. > > >I can add a test in chunk_demuxer_unittests when I am back to office next > week > > >if it is necessary. > > > > wolenetz@, please make a call on what else needs to be done. It would be nice > if > > this was not so complex and could be verified by the code. I'm not sure we > want > > to introduce container types to that code path, though. > > kqyang@: kVideoMP4Codecs conditioning looks good. > Unifying MSE and src= mime+codec verification methods that service canPlayType > (src=, also consulted by MSE), isTypeSupported (MSE), addSourceBuffer(MSE) might > reduce complexity, but I'm unclear at the moment the right way forward there, > and probably needs to happen in its own CL. Container type verification is > already done in both paths, so I'm unclear what your comment's final sentence > was meaning (unless maybe overriding build-flags to let test condition them at > runtime? I suspect we don't want to do that since the build would thus contain > ability for runtime selection...) (my "Unifying..." reply portion was meant for ddorwin@)
wolenetz@chromium.org changed reviewers: - wolenetz@chromium.org
fgalligan@chromium.org changed reviewers: + fgalligan@chromium.org
lgtm https://codereview.chromium.org/2495193004/diff/200001/media/base/mime_util_i... File media/base/mime_util_internal.cc (right): https://codereview.chromium.org/2495193004/diff/200001/media/base/mime_util_i... media/base/mime_util_internal.cc:154: // TODO(kqyang): Should we support new codec string in WebM? I think we should switch and deprecate the legacy way. https://codereview.chromium.org/2495193004/diff/200001/media/base/video_codec... File media/base/video_codecs.cc (right): https://codereview.chromium.org/2495193004/diff/200001/media/base/video_codec... media/base/video_codecs.cc:165: *level_idc = 0; From the discussion earlier. I guess 0 is fine. I seem to remember having a discussion with Android that they return the lowest level if unknown. But I can't find it now. I also see that they define OMX_VIDEO_VP9LevelUnknown in the OMX.
LGTM. Two comments in PS10 and a reply to wolenetz@ in PS11. https://codereview.chromium.org/2495193004/diff/140001/media/base/video_codec... File media/base/video_codecs.cc (right): https://codereview.chromium.org/2495193004/diff/140001/media/base/video_codec... media/base/video_codecs.cc:371: VideoCodec StringToVideoCodec(const std::string& codec_id) { On 2016/11/28 22:24:10, ddorwin wrote: > On 2016/11/28 22:08:51, ddorwin wrote: > > On 2016/11/23 22:54:18, wolenetz_ooo_nov28 wrote: > > > On 2016/11/23 00:22:17, ddorwin wrote: > > > > This is called by SourceBufferState::Init(), which doesn't have container > > > > context. I'm not sure what it is used for, but I'm concerned that: > > > > a) There is no way to restrict codec string formats by container type. > > > > b) This does not block the new format based on the flag. This is a > > regression > > > > and something we should fix. > > > > > > > > +wolenetz@ for comments on whether this is a concern or somehow blocked by > > > > mime_util_internal.cc code. > > > > > > The only call-chain (outside of tests) of this method is ChunkDemuxer::AddId > > -> > > > SourceBufferState::Init -> here > > > > > > AddId first calls media::ParseCodecString > > > > > > (https://cs.chromium.org/chromium/src/media/filters/chunk_demuxer.cc?rcl=14799...) > > > (similarly with no container context) > > > And then attempts construction of an appropriate StreamParser > > > > > > (https://cs.chromium.org/chromium/src/media/filters/chunk_demuxer.cc?rcl=14799...) > > > (this *does* have container context) If StreamParserFactory's > kVideoMP4Codecs > > is > > > unconditionally including VP9, that might be a problem. > > > > > > If either of those fail, then no SourceBufferState::Init is attempted. > > > > > > Also, ChunkDemuxer::AddId is called only after MediaSource::addSourceBuffer > > has > > > first confirmed that MediaSource::isTypeSupported() has succeeded > > > > > > (https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/medias...) > > > > > > and MediaSource::isTypeSupported() will fail if > > HTMLMediaElement::supportsType() > > > fails (IOW, if canPlayType() returns IsNotSupported). That path involves > > > MimeUtil::IsSupportedMediaFormat(), which *does* have container context. > > > > > > Yes, a bit convoluted. > > > > > > If StreamParserFactory doesn't condition kVideoMP4Codecs on the flag, then > > that > > > should be fixed as part of this change to at least make the expectation > around > > > the MSE side clearer. chunk_demuxer_unittests should be included to > condition > > > their expectation around AddId ( mp4 container with vp9 codec ) upon the > flag. > > > Ideally, the flag would be configurable on bots and layout tests could be > > added > > > with virtual suites that vary that flag, but that might be overkill :) > > > > > > > kqyang, please consider the above, especially the conditional inclusion of VP9 > > with kVideoMP4Codecs. > > I missed this response that was a general reply and not included in the thread: > > >StreamParserFactory does condition kVideoMP4Codecs on --enable-vp9-in-mp4. See: > >https://cs.chromium.org/chromium/src/media/filters/stream_parser_factory.cc?rcl=1479917866&l=113. > >I can add a test in chunk_demuxer_unittests when I am back to office next week > >if it is necessary. > > wolenetz@, please make a call on what else needs to be done. It would be nice if > this was not so complex and could be verified by the code. I'm not sure we want > to introduce container types to that code path, though. On 2016/12/01 03:07:25, wolenetz wrote: > kqyang@: kVideoMP4Codecs conditioning looks good. > Unifying MSE and src= mime+codec verification methods that service canPlayType > (src=, also consulted by MSE), isTypeSupported (MSE), addSourceBuffer(MSE) might > reduce complexity, but I'm unclear at the moment the right way forward there, > and probably needs to happen in its own CL. Container type verification is > already done in both paths, so I'm unclear what your comment's final sentence > was meaning (unless maybe overriding build-flags to let test condition them at > runtime? I suspect we don't want to do that since the build would thus contain > ability for runtime selection...) I meant that even though we can inspect the path and see that the container type is checked, it would be nice if we could be sure of this by looking at this code rather than having to inspect the path. If we do already have the container, maybe we _should_ pass it here. This can be addressed in a separate CL. https://codereview.chromium.org/2495193004/diff/180001/media/base/video_codec... File media/base/video_codecs.cc (right): https://codereview.chromium.org/2495193004/diff/180001/media/base/video_codec... media/base/video_codecs.cc:131: On 2016/11/30 02:31:17, kqyang wrote: > On 2016/11/29 23:48:51, ddorwin wrote: > > Do we have a separate bug to actually check the values? We should and included > > it in a TODO here. > > No. We haven't decided whether we want to validate every fields. How about > settle crbug.com/667834 first and if needed we can open additional bugs later? We can avoid commenting here, but there should be a bug tracking that decision and any subsequent work. That bug should block the launch bug. https://codereview.chromium.org/2495193004/diff/180001/media/base/video_codec... media/base/video_codecs.cc:132: *level_idc = values[1]; On 2016/11/30 02:31:17, kqyang wrote: > On 2016/11/29 23:48:51, ddorwin wrote: > > An easy check is that this can't be less than 10, right? Probably with a > comment > > about the multiplication. > > This mapping (actual level = level / 10) does not seem to be available publicly > anywhere right now. > > I think we should include the mapping in VP9 in ISO-BMFF spec. We can then do > additional validations here after finalizing the spec. > > This appears to be the one value where we don't check the value at all. Regardless of whether we probe the codec implementation, we'd check the validity of the string. Maybe we should have a TODO to add checks for valid values after crbug.com/667834 is resolved.
wolenetz@chromium.org changed reviewers: + wolenetz@chromium.org
https://codereview.chromium.org/2495193004/diff/140001/media/base/video_codec... File media/base/video_codecs.cc (right): https://codereview.chromium.org/2495193004/diff/140001/media/base/video_codec... media/base/video_codecs.cc:371: VideoCodec StringToVideoCodec(const std::string& codec_id) { On 2016/12/02 18:55:54, ddorwin wrote: > On 2016/11/28 22:24:10, ddorwin wrote: > > On 2016/11/28 22:08:51, ddorwin wrote: > > > On 2016/11/23 22:54:18, wolenetz_ooo_nov28 wrote: > > > > On 2016/11/23 00:22:17, ddorwin wrote: > > > > > This is called by SourceBufferState::Init(), which doesn't have > container > > > > > context. I'm not sure what it is used for, but I'm concerned that: > > > > > a) There is no way to restrict codec string formats by container type. > > > > > b) This does not block the new format based on the flag. This is a > > > regression > > > > > and something we should fix. > > > > > > > > > > +wolenetz@ for comments on whether this is a concern or somehow blocked > by > > > > > mime_util_internal.cc code. > > > > > > > > The only call-chain (outside of tests) of this method is > ChunkDemuxer::AddId > > > -> > > > > SourceBufferState::Init -> here > > > > > > > > AddId first calls media::ParseCodecString > > > > > > > > > > (https://cs.chromium.org/chromium/src/media/filters/chunk_demuxer.cc?rcl=14799...) > > > > (similarly with no container context) > > > > And then attempts construction of an appropriate StreamParser > > > > > > > > > > (https://cs.chromium.org/chromium/src/media/filters/chunk_demuxer.cc?rcl=14799...) > > > > (this *does* have container context) If StreamParserFactory's > > kVideoMP4Codecs > > > is > > > > unconditionally including VP9, that might be a problem. > > > > > > > > If either of those fail, then no SourceBufferState::Init is attempted. > > > > > > > > Also, ChunkDemuxer::AddId is called only after > MediaSource::addSourceBuffer > > > has > > > > first confirmed that MediaSource::isTypeSupported() has succeeded > > > > > > > > > > (https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/medias...) > > > > > > > > and MediaSource::isTypeSupported() will fail if > > > HTMLMediaElement::supportsType() > > > > fails (IOW, if canPlayType() returns IsNotSupported). That path involves > > > > MimeUtil::IsSupportedMediaFormat(), which *does* have container context. > > > > > > > > Yes, a bit convoluted. > > > > > > > > If StreamParserFactory doesn't condition kVideoMP4Codecs on the flag, then > > > that > > > > should be fixed as part of this change to at least make the expectation > > around > > > > the MSE side clearer. chunk_demuxer_unittests should be included to > > condition > > > > their expectation around AddId ( mp4 container with vp9 codec ) upon the > > flag. > > > > Ideally, the flag would be configurable on bots and layout tests could be > > > added > > > > with virtual suites that vary that flag, but that might be overkill :) > > > > > > > > > > kqyang, please consider the above, especially the conditional inclusion of > VP9 > > > with kVideoMP4Codecs. > > > > I missed this response that was a general reply and not included in the > thread: > > > > >StreamParserFactory does condition kVideoMP4Codecs on --enable-vp9-in-mp4. > See: > > > >https://cs.chromium.org/chromium/src/media/filters/stream_parser_factory.cc?rcl=1479917866&l=113. > > >I can add a test in chunk_demuxer_unittests when I am back to office next > week > > >if it is necessary. > > > > wolenetz@, please make a call on what else needs to be done. It would be nice > if > > this was not so complex and could be verified by the code. I'm not sure we > want > > to introduce container types to that code path, though. > > On 2016/12/01 03:07:25, wolenetz wrote: > > kqyang@: kVideoMP4Codecs conditioning looks good. > > Unifying MSE and src= mime+codec verification methods that service canPlayType > > (src=, also consulted by MSE), isTypeSupported (MSE), addSourceBuffer(MSE) > might > > reduce complexity, but I'm unclear at the moment the right way forward there, > > and probably needs to happen in its own CL. Container type verification is > > already done in both paths, so I'm unclear what your comment's final sentence > > was meaning (unless maybe overriding build-flags to let test condition them at > > runtime? I suspect we don't want to do that since the build would thus contain > > ability for runtime selection...) > > I meant that even though we can inspect the path and see that the container type > is checked, it would be nice if we could be sure of this by looking at this code > rather than having to inspect the path. If we do already have the container, > maybe we _should_ pass it here. This can be addressed in a separate CL. Got it. Thanks for clarifying, ddorwin@. I've filed https://crbug.com/670810.
wolenetz@chromium.org changed reviewers: - wolenetz@chromium.org
https://codereview.chromium.org/2495193004/diff/180001/media/base/video_codec... File media/base/video_codecs.cc (right): https://codereview.chromium.org/2495193004/diff/180001/media/base/video_codec... media/base/video_codecs.cc:131: On 2016/12/02 18:55:54, ddorwin wrote: > On 2016/11/30 02:31:17, kqyang wrote: > > On 2016/11/29 23:48:51, ddorwin wrote: > > > Do we have a separate bug to actually check the values? We should and > included > > > it in a TODO here. > > > > No. We haven't decided whether we want to validate every fields. How about > > settle crbug.com/667834 first and if needed we can open additional bugs later? > > We can avoid commenting here, but there should be a bug tracking that decision > and any subsequent work. That bug should block the launch bug. We can use crbug.com/667834 to track the decision and subsequent work. https://codereview.chromium.org/2495193004/diff/180001/media/base/video_codec... media/base/video_codecs.cc:132: *level_idc = values[1]; On 2016/12/02 18:55:54, ddorwin wrote: > On 2016/11/30 02:31:17, kqyang wrote: > > On 2016/11/29 23:48:51, ddorwin wrote: > > > An easy check is that this can't be less than 10, right? Probably with a > > comment > > > about the multiplication. > > > > This mapping (actual level = level / 10) does not seem to be available > publicly > > anywhere right now. > > > > I think we should include the mapping in VP9 in ISO-BMFF spec. We can then do > > additional validations here after finalizing the spec. > > > > > > This appears to be the one value where we don't check the value at all. > Regardless of whether we probe the codec implementation, we'd check the validity > of the string. > Maybe we should have a TODO to add checks for valid values after > crbug.com/667834 is resolved. Done.
The CQ bit was checked by kqyang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from fgalligan@chromium.org, ddorwin@chromium.org Link to the patchset: https://codereview.chromium.org/2495193004/#ps220001 (title: "Add a comment to check |level_idc|.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 220001, "attempt_start_ts": 1480713325683040,
"parent_rev": "4701a75201e9e67344ef1a1f3d6d9dd0394eafe3", "commit_rev":
"46964378506ac2684a29d37c2fd5e948e092bcff"}
Message was sent while issue was closed.
Description was changed from ========== Refactor VP9 codec string parsing - Also fixed source_buffer_state failed to find codec with vp9 in mp4 contents when VP9 in MP4 is enabled. - And added a few playback/demuxer tests. BUG=580623 ========== to ========== Refactor VP9 codec string parsing - Also fixed source_buffer_state failed to find codec with vp9 in mp4 contents when VP9 in MP4 is enabled. - And added a few playback/demuxer tests. BUG=580623 ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== Refactor VP9 codec string parsing - Also fixed source_buffer_state failed to find codec with vp9 in mp4 contents when VP9 in MP4 is enabled. - And added a few playback/demuxer tests. BUG=580623 ========== to ========== Refactor VP9 codec string parsing - Also fixed source_buffer_state failed to find codec with vp9 in mp4 contents when VP9 in MP4 is enabled. - And added a few playback/demuxer tests. BUG=580623 Committed: https://crrev.com/f93bb7be32da55f1ea60694b2ec803f3ac0cdf9f Cr-Commit-Position: refs/heads/master@{#436144} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/f93bb7be32da55f1ea60694b2ec803f3ac0cdf9f Cr-Commit-Position: refs/heads/master@{#436144} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
