Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1677563003:
Implemented parsing for H.264/AVC codec ids (Closed)
DescriptionImplemented parsing for H.264/AVC codec ids
This CL implements parsing of avc1/avc3 codec ids from the mime type
string. Compared to the previous implementation (ParseH264CodecId in
mime_util.cpp) this one handles all H.264/AVC codec profiles defined in
the ISO standard and implements handling of constraint set flags 0
through 2.
BUG=456408
Committed: https://crrev.com/6e41da7fca39986672740e07d37fa6d5ef71b3b4
Cr-Commit-Position: refs/heads/master@{#376078}
Patch Set 1 #Patch Set 2 : nits #Patch Set 3 : ps2 #Patch Set 4 : Refactoring #Patch Set 5 : wip #Patch Set 6 : wip #Patch Set 7 : Clean up level_idc handling #Patch Set 8 : Include stdint.h into video_codecs.h #Patch Set 9 : Fixed unit tests #
Total comments: 23
Patch Set 10 : Fixed ordering of test case + improved test cases for reserved bits #
Total comments: 8
Patch Set 11 : Adjusted unit tests #
Total comments: 9
Patch Set 12 : Rebase + resolve conflicts #
Dependent Patchsets: Messages
Total messages: 27 (11 generated)
Description was changed from ========== Implemented parsing for H.264/AVC codec ids BUG=456408 ========== to ========== Implemented parsing for H.264/AVC codec ids This CL implements parsing of avc1/avc3 codec ids from the mime type string. Compared to the previous implementation (ParseH264CodecId in mime_util.cpp) this one handles all H.264/AVC codec profiles defined in the ISO standard and implements handling of constraint set flags 0 through 2. BUG=456408 ==========
servolk@chromium.org changed reviewers: + ddorwin@chromium.org, sandersd@chromium.org, wolenetz@chromium.org
On 2016/02/08 20:01:13, servolk wrote: ping
lgtm
The CQ bit was checked by servolk@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/1677563003/#ps160001 (title: "Fixed unit tests")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1677563003/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1677563003/160001
The CQ bit was unchecked by ddorwin@chromium.org
Apologies for not getting to this sooner. https://codereview.chromium.org/1677563003/diff/160001/content/browser/media/... File content/browser/media/media_canplaytype_browsertest.cc (right): https://codereview.chromium.org/1677563003/diff/160001/content/browser/media/... content/browser/media/media_canplaytype_browsertest.cc:632: EXPECT_EQ(kPropProbably, CanPlay("'video/mp4; codecs=\"avc1.42E01E\"'")); Why was the codec ID changed to a probably? This was covering the case where we return maybe. It's probably not adding coverage to just have another probably. https://codereview.chromium.org/1677563003/diff/160001/content/browser/media/... content/browser/media/media_canplaytype_browsertest.cc:873: EXPECT_EQ(kPropMaybe, CanPlay("'video/mp4; codecs=\"avc1.42E0FF\"'")); I assume this is an invalid level. If so, it should probably be after 875 to match the order in the comment. https://codereview.chromium.org/1677563003/diff/160001/content/browser/media/... content/browser/media/media_canplaytype_browsertest.cc:888: EXPECT_EQ(kPropMaybe, CanPlay("'video/mp4; codecs=\"avc1.4DE0FF\"'")); ditto https://codereview.chromium.org/1677563003/diff/160001/content/browser/media/... content/browser/media/media_canplaytype_browsertest.cc:903: EXPECT_EQ(kPropMaybe, CanPlay("'video/mp4; codecs=\"avc1.64E0FF\"'")); ditto https://codereview.chromium.org/1677563003/diff/160001/content/browser/media/... content/browser/media/media_canplaytype_browsertest.cc:906: It looks like we lost coverage of this: // The fourth digit must be 0. 975 EXPECT_EQ(kPropMaybe, CanPlay("'video/mp4; codecs=\"avc3.64E11E\"'")); Or is that no longer true and we allow non-zero values. If so, we should have a test for non-zero fourth digit where everything else is valid. https://codereview.chromium.org/1677563003/diff/160001/content/browser/media/... content/browser/media/media_canplaytype_browsertest.cc:915: // When modifying this test, also change CodecSupportTest_Avc1Variants. same as above. https://codereview.chromium.org/1677563003/diff/160001/content/browser/media/... content/browser/media/media_canplaytype_browsertest.cc:1196: CanPlay("'application/x-mpegurl; codecs=\"avc1.42E01E\"'")); ditto here and 1272 https://codereview.chromium.org/1677563003/diff/160001/media/base/mime_util.cc File media/base/mime_util.cc (right): https://codereview.chromium.org/1677563003/diff/160001/media/base/mime_util.c... media/base/mime_util.cc:1: // Copyright 2012 The Chromium Authors. All rights reserved. This is going to conflict with Dale's CL to move most of this code to an _internal.cc file. https://codereview.chromium.org/1677563003/diff/160001/media/base/mime_util.c... media/base/mime_util.cc:45: HEVC_MAIN, Should we remove _MAIN here too?
https://codereview.chromium.org/1677563003/diff/160001/content/browser/media/... File content/browser/media/media_canplaytype_browsertest.cc (right): https://codereview.chromium.org/1677563003/diff/160001/content/browser/media/... content/browser/media/media_canplaytype_browsertest.cc:632: EXPECT_EQ(kPropProbably, CanPlay("'video/mp4; codecs=\"avc1.42E01E\"'")); On 2016/02/17 19:31:20, ddorwin wrote: > Why was the codec ID changed to a probably? This was covering the case where we > return maybe. It's probably not adding coverage to just have another probably. As I've explained in a thread about logging mime types into rappor on videostack-eng, the codec id 'avc1.42E11E' is highly unusual (just 6 google search results, all from chromium) and is not a valid codec id according to the current ISO/IEC standard because it has a reserved bit set (the lower two bits of the fourth digit after the dot are reserved and must be zero). So I think we shouldn't return kMaybe for this, we should return kNot (since this is known to be an invalid codec id). So I've replace the codec id here with a valid codec id and changed the expected outcome to kProbably, and in order to keep test coverage for checking the reserved bits I've added a new test case where the reserved bit is set (but other compat flags are unset) - see line 849 below. https://codereview.chromium.org/1677563003/diff/160001/content/browser/media/... content/browser/media/media_canplaytype_browsertest.cc:873: EXPECT_EQ(kPropMaybe, CanPlay("'video/mp4; codecs=\"avc1.42E0FF\"'")); On 2016/02/17 19:31:19, ddorwin wrote: > I assume this is an invalid level. If so, it should probably be after 875 to > match the order in the comment. Done. https://codereview.chromium.org/1677563003/diff/160001/content/browser/media/... content/browser/media/media_canplaytype_browsertest.cc:888: EXPECT_EQ(kPropMaybe, CanPlay("'video/mp4; codecs=\"avc1.4DE0FF\"'")); On 2016/02/17 19:31:20, ddorwin wrote: > ditto Done. https://codereview.chromium.org/1677563003/diff/160001/content/browser/media/... content/browser/media/media_canplaytype_browsertest.cc:903: EXPECT_EQ(kPropMaybe, CanPlay("'video/mp4; codecs=\"avc1.64E0FF\"'")); On 2016/02/17 19:31:19, ddorwin wrote: > ditto Done. https://codereview.chromium.org/1677563003/diff/160001/content/browser/media/... content/browser/media/media_canplaytype_browsertest.cc:906: On 2016/02/17 19:31:20, ddorwin wrote: > It looks like we lost coverage of this: > // The fourth digit must be 0. > 975 EXPECT_EQ(kPropMaybe, CanPlay("'video/mp4; codecs=\"avc3.64E11E\"'")); > > Or is that no longer true and we allow non-zero values. If so, we should have a > test for non-zero fourth digit where everything else is valid. Well, we didn't lose it completely, it's still covered in line 849 above, I'm just not sure if we really need to have test coverage where a reserved bit is set for each possible profile value. But if you think we need to extend this and test more combinations (various combinations of different profiles and reserved bits) I can add that. https://codereview.chromium.org/1677563003/diff/160001/content/browser/media/... content/browser/media/media_canplaytype_browsertest.cc:912: EXPECT_EQ(kPropMaybe, CanPlay("'video/mp4; codecs=\"avc1.58001E\"'")); Btw, note also that I changed this. Since the third digit being A in the old codec id means that this should be treated as a baseline profile as indicated by the compat flags in the second byte. By changing A to 0 I've reset all the compat flags to 0 to ensure we are actually testing the extended profile (88==0x58) here. https://codereview.chromium.org/1677563003/diff/160001/content/browser/media/... content/browser/media/media_canplaytype_browsertest.cc:915: // When modifying this test, also change CodecSupportTest_Avc1Variants. On 2016/02/17 19:31:20, ddorwin wrote: > same as above. See the explanations above. https://codereview.chromium.org/1677563003/diff/160001/content/browser/media/... content/browser/media/media_canplaytype_browsertest.cc:1196: CanPlay("'application/x-mpegurl; codecs=\"avc1.42E01E\"'")); On 2016/02/17 19:31:20, ddorwin wrote: > ditto here and 1272 See the explanations above. https://codereview.chromium.org/1677563003/diff/160001/media/base/mime_util.cc File media/base/mime_util.cc (right): https://codereview.chromium.org/1677563003/diff/160001/media/base/mime_util.c... media/base/mime_util.cc:45: HEVC_MAIN, On 2016/02/17 19:31:20, ddorwin wrote: > Should we remove _MAIN here too? Yes, I'm planning to do that in a separate CL which will implement parsing of HEVC codec ids (WIP: https://codereview.chromium.org/1677133003/)
Thanks. Changes LG except some duplicate code. See also comments in PS9. https://codereview.chromium.org/1677563003/diff/160001/content/browser/media/... File content/browser/media/media_canplaytype_browsertest.cc (right): https://codereview.chromium.org/1677563003/diff/160001/content/browser/media/... content/browser/media/media_canplaytype_browsertest.cc:632: EXPECT_EQ(kPropProbably, CanPlay("'video/mp4; codecs=\"avc1.42E01E\"'")); On 2016/02/17 19:49:53, servolk wrote: > On 2016/02/17 19:31:20, ddorwin wrote: > > Why was the codec ID changed to a probably? This was covering the case where > we > > return maybe. It's probably not adding coverage to just have another probably. > > As I've explained in a thread about logging mime types into rappor on > videostack-eng, the codec id 'avc1.42E11E' is highly unusual (just 6 google > search results, all from chromium) and is not a valid codec id according to the > current ISO/IEC standard because it has a reserved bit set (the lower two bits > of the fourth digit after the dot are reserved and must be zero). So I think we > shouldn't return kMaybe for this, we should return kNot (since this is known to > be an invalid codec id). So I've replace the codec id here with a valid codec id > and changed the expected outcome to kProbably, and in order to keep test > coverage for checking the reserved bits I've added a new test case where the > reserved bit is set (but other compat flags are unset) - see line 849 below. This was added in https://chromium.googlesource.com/chromium/src/+/0a0cad0dfcd0c8ea77352fb23bd2.... It appears that this was the known valid "42E" with the invalid 1 in the 4th bit. That's probably no longer relevant. I guess the question is why was this changed to a valid value? In fact, this is a duplicate of line 623. https://codereview.chromium.org/1677563003/diff/160001/content/browser/media/... content/browser/media/media_canplaytype_browsertest.cc:906: On 2016/02/17 19:49:54, servolk wrote: > On 2016/02/17 19:31:20, ddorwin wrote: > > It looks like we lost coverage of this: > > // The fourth digit must be 0. > > 975 EXPECT_EQ(kPropMaybe, CanPlay("'video/mp4; codecs=\"avc3.64E11E\"'")); > > > > Or is that no longer true and we allow non-zero values. If so, we should have > a > > test for non-zero fourth digit where everything else is valid. > > Well, we didn't lose it completely, it's still covered in line 849 above, I'm > just not sure if we really need to have test coverage where a reserved bit is > set for each possible profile value. But if you think we need to extend this and > test more combinations (various combinations of different profiles and reserved > bits) I can add that. Okay, thanks for the explanation. Note: The original comment mistakenly copied the old text from the "avc3" version. https://codereview.chromium.org/1677563003/diff/160001/content/browser/media/... content/browser/media/media_canplaytype_browsertest.cc:912: EXPECT_EQ(kPropMaybe, CanPlay("'video/mp4; codecs=\"avc1.58001E\"'")); On 2016/02/17 19:49:53, servolk wrote: > Btw, note also that I changed this. Since the third digit being A in the old > codec id means that this should be treated as a baseline profile as indicated by > the compat flags in the second byte. By changing A to 0 I've reset all the > compat flags to 0 to ensure we are actually testing the extended profile > (88==0x58) here. Acknowledged. If it is valid to "override" the extended profile, should we have tests for that as well? https://codereview.chromium.org/1677563003/diff/180001/content/browser/media/... File content/browser/media/media_canplaytype_browsertest.cc (right): https://codereview.chromium.org/1677563003/diff/180001/content/browser/media/... content/browser/media/media_canplaytype_browsertest.cc:632: EXPECT_EQ(kPropProbably, CanPlay("'video/mp4; codecs=\"avc1.42E01E\"'")); Remove these lines you changed since they are duplicates (line 623 in this case.) See comment in previous PS. https://codereview.chromium.org/1677563003/diff/180001/content/browser/media/... content/browser/media/media_canplaytype_browsertest.cc:713: EXPECT_EQ(kPropProbably, CanPlay("'video/x-m4v; codecs=\"avc1.42E01E\"'")); ditto https://codereview.chromium.org/1677563003/diff/180001/content/browser/media/... content/browser/media/media_canplaytype_browsertest.cc:1198: CanPlay("'application/x-mpegurl; codecs=\"avc1.42E01E\"'")); ditto https://codereview.chromium.org/1677563003/diff/180001/content/browser/media/... content/browser/media/media_canplaytype_browsertest.cc:1275: CanPlay("'application/vnd.apple.mpegurl; codecs=\"avc1.42E01E\"'")); ditto
https://codereview.chromium.org/1677563003/diff/160001/content/browser/media/... File content/browser/media/media_canplaytype_browsertest.cc (right): https://codereview.chromium.org/1677563003/diff/160001/content/browser/media/... content/browser/media/media_canplaytype_browsertest.cc:632: EXPECT_EQ(kPropProbably, CanPlay("'video/mp4; codecs=\"avc1.42E01E\"'")); On 2016/02/17 21:44:21, ddorwin wrote: > On 2016/02/17 19:49:53, servolk wrote: > > On 2016/02/17 19:31:20, ddorwin wrote: > > > Why was the codec ID changed to a probably? This was covering the case where > > we > > > return maybe. It's probably not adding coverage to just have another > probably. > > > > As I've explained in a thread about logging mime types into rappor on > > videostack-eng, the codec id 'avc1.42E11E' is highly unusual (just 6 google > > search results, all from chromium) and is not a valid codec id according to > the > > current ISO/IEC standard because it has a reserved bit set (the lower two bits > > of the fourth digit after the dot are reserved and must be zero). So I think > we > > shouldn't return kMaybe for this, we should return kNot (since this is known > to > > be an invalid codec id). So I've replace the codec id here with a valid codec > id > > and changed the expected outcome to kProbably, and in order to keep test > > coverage for checking the reserved bits I've added a new test case where the > > reserved bit is set (but other compat flags are unset) - see line 849 below. > > This was added in > https://chromium.googlesource.com/chromium/src/+/0a0cad0dfcd0c8ea77352fb23bd2.... > It appears that this was the known valid "42E" with the invalid 1 in the 4th > bit. That's probably no longer relevant. > > I guess the question is why was this changed to a valid value? In fact, this is > a duplicate of line 623. Good point about duplication, I haven't noticed that we already have the same value at line 623, I guess we can remove it here now. I've changed this to a valid value because I thought it was more important to test actual valid values that might be potentially used by some apps, rather than a non-standard compliant codec with the reserved bit set to 1. That's why I've changed this (and other similar combinations for other profile values) to valid codec ids and kept the variation with just one profile and the reserved bits sets in a single place to ensure we still have test coverage for this. https://codereview.chromium.org/1677563003/diff/160001/content/browser/media/... content/browser/media/media_canplaytype_browsertest.cc:912: EXPECT_EQ(kPropMaybe, CanPlay("'video/mp4; codecs=\"avc1.58001E\"'")); On 2016/02/17 21:44:21, ddorwin wrote: > On 2016/02/17 19:49:53, servolk wrote: > > Btw, note also that I changed this. Since the third digit being A in the old > > codec id means that this should be treated as a baseline profile as indicated > by > > the compat flags in the second byte. By changing A to 0 I've reset all the > > compat flags to 0 to ensure we are actually testing the extended profile > > (88==0x58) here. > > Acknowledged. If it is valid to "override" the extended profile, should we have > tests for that as well? Yes, good point. I've added a few test cases for constraint_set0_flag==1, constraint_set1_flag==1 and constraint_set2_flag==1. These are the only flags supported atm. https://codereview.chromium.org/1677563003/diff/180001/content/browser/media/... File content/browser/media/media_canplaytype_browsertest.cc (right): https://codereview.chromium.org/1677563003/diff/180001/content/browser/media/... content/browser/media/media_canplaytype_browsertest.cc:632: EXPECT_EQ(kPropProbably, CanPlay("'video/mp4; codecs=\"avc1.42E01E\"'")); On 2016/02/17 21:44:21, ddorwin wrote: > Remove these lines you changed since they are duplicates (line 623 in this > case.) See comment in previous PS. Done (I've also rearranged the remaining avc1 test cases so that avc1 cases are grouped together and avc3 and old-style avc1 cases are in separate groups). https://codereview.chromium.org/1677563003/diff/180001/content/browser/media/... content/browser/media/media_canplaytype_browsertest.cc:713: EXPECT_EQ(kPropProbably, CanPlay("'video/x-m4v; codecs=\"avc1.42E01E\"'")); On 2016/02/17 21:44:21, ddorwin wrote: > ditto Done. https://codereview.chromium.org/1677563003/diff/180001/content/browser/media/... content/browser/media/media_canplaytype_browsertest.cc:1198: CanPlay("'application/x-mpegurl; codecs=\"avc1.42E01E\"'")); On 2016/02/17 21:44:21, ddorwin wrote: > ditto Done. https://codereview.chromium.org/1677563003/diff/180001/content/browser/media/... content/browser/media/media_canplaytype_browsertest.cc:1275: CanPlay("'application/vnd.apple.mpegurl; codecs=\"avc1.42E01E\"'")); On 2016/02/17 21:44:21, ddorwin wrote: > ditto Done.
Thanks. I noticed some gaps in old-style avc1 testing. Feel free to address those separately. media_canplaytype_browsertest.cc LGTM. https://codereview.chromium.org/1677563003/diff/200001/content/browser/media/... File content/browser/media/media_canplaytype_browsertest.cc (right): https://codereview.chromium.org/1677563003/diff/200001/content/browser/media/... content/browser/media/media_canplaytype_browsertest.cc:635: EXPECT_EQ(kNot, CanPlay("'video/mp4; codecs=\"avc1.100.40\"'")); Should we have an avc3 entry too? Or add it to TestMPEGUnacceptableCombinations as I mention below. https://codereview.chromium.org/1677563003/diff/200001/content/browser/media/... content/browser/media/media_canplaytype_browsertest.cc:1183: IN_PROC_BROWSER_TEST_F(MediaCanPlayTypeTest, CodecSupportTest_HLS) { Both HLS variants are missing a test for old-style avc1. https://codereview.chromium.org/1677563003/diff/200001/content/browser/media/... content/browser/media/media_canplaytype_browsertest.cc:1396: EXPECT_EQ(kMp2tsProbably, CanPlay("'video/mp2t; codecs=\"avc1.66.10\"'")); Is this supported for avc3? If not, let's add at least one negative entry. Perhaps to TestMPEGUnacceptableCombinations.
The CQ bit was checked by servolk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1677563003/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1677563003/220001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
https://codereview.chromium.org/1677563003/diff/200001/content/browser/media/... File content/browser/media/media_canplaytype_browsertest.cc (right): https://codereview.chromium.org/1677563003/diff/200001/content/browser/media/... content/browser/media/media_canplaytype_browsertest.cc:635: EXPECT_EQ(kNot, CanPlay("'video/mp4; codecs=\"avc1.100.40\"'")); On 2016/02/18 00:32:23, ddorwin wrote: > Should we have an avc3 entry too? Or add it to TestMPEGUnacceptableCombinations > as I mention below. We have never seen avc3 being used in old-style codecs so far and the codec id translation handles only avc1 cases. And a few quick searches for exact strings "avc3.100.40", "avc3.77.31" and "avc3.66.13" gave no results. So I think we don't need to support old-style avc3 codec ids. But indeed we could add a couple of old-style avc3 cases to TestMPEGUnacceptableCombinations. https://codereview.chromium.org/1677563003/diff/200001/content/browser/media/... content/browser/media/media_canplaytype_browsertest.cc:1183: IN_PROC_BROWSER_TEST_F(MediaCanPlayTypeTest, CodecSupportTest_HLS) { On 2016/02/18 00:32:23, ddorwin wrote: > Both HLS variants are missing a test for old-style avc1. These are Android tests and Android uses a different HLS path. The old-style translation that I've added is only enabled for video/mp2t mime types and guarded by BUILDFLAG(ENABLE_MSE_MPEG2TS_DEMUX), so it won't work on Android ATM, only on Chromecast. https://codereview.chromium.org/1677563003/diff/200001/content/browser/media/... content/browser/media/media_canplaytype_browsertest.cc:1396: EXPECT_EQ(kMp2tsProbably, CanPlay("'video/mp2t; codecs=\"avc1.66.10\"'")); On 2016/02/18 00:32:23, ddorwin wrote: > Is this supported for avc3? If not, let's add at least one negative entry. > Perhaps to TestMPEGUnacceptableCombinations. https://codereview.chromium.org/1709803003
The CQ bit was checked by servolk@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/1677563003/#ps220001 (title: "Rebase + resolve conflicts")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1677563003/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1677563003/220001
Message was sent while issue was closed.
Description was changed from ========== Implemented parsing for H.264/AVC codec ids This CL implements parsing of avc1/avc3 codec ids from the mime type string. Compared to the previous implementation (ParseH264CodecId in mime_util.cpp) this one handles all H.264/AVC codec profiles defined in the ISO standard and implements handling of constraint set flags 0 through 2. BUG=456408 ========== to ========== Implemented parsing for H.264/AVC codec ids This CL implements parsing of avc1/avc3 codec ids from the mime type string. Compared to the previous implementation (ParseH264CodecId in mime_util.cpp) this one handles all H.264/AVC codec profiles defined in the ISO standard and implements handling of constraint set flags 0 through 2. BUG=456408 ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== Implemented parsing for H.264/AVC codec ids This CL implements parsing of avc1/avc3 codec ids from the mime type string. Compared to the previous implementation (ParseH264CodecId in mime_util.cpp) this one handles all H.264/AVC codec profiles defined in the ISO standard and implements handling of constraint set flags 0 through 2. BUG=456408 ========== to ========== Implemented parsing for H.264/AVC codec ids This CL implements parsing of avc1/avc3 codec ids from the mime type string. Compared to the previous implementation (ParseH264CodecId in mime_util.cpp) this one handles all H.264/AVC codec profiles defined in the ISO standard and implements handling of constraint set flags 0 through 2. BUG=456408 Committed: https://crrev.com/6e41da7fca39986672740e07d37fa6d5ef71b3b4 Cr-Commit-Position: refs/heads/master@{#376078} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/6e41da7fca39986672740e07d37fa6d5ef71b3b4 Cr-Commit-Position: refs/heads/master@{#376078}
Message was sent while issue was closed.
https://codereview.chromium.org/1677563003/diff/200001/content/browser/media/... File content/browser/media/media_canplaytype_browsertest.cc (right): https://codereview.chromium.org/1677563003/diff/200001/content/browser/media/... content/browser/media/media_canplaytype_browsertest.cc:635: EXPECT_EQ(kNot, CanPlay("'video/mp4; codecs=\"avc1.100.40\"'")); On 2016/02/18 01:37:50, servolk wrote: > On 2016/02/18 00:32:23, ddorwin wrote: > > Should we have an avc3 entry too? Or add it to > TestMPEGUnacceptableCombinations > > as I mention below. > > We have never seen avc3 being used in old-style codecs so far and the codec id > translation handles only avc1 cases. And a few quick searches for exact strings > "avc3.100.40", "avc3.77.31" and "avc3.66.13" gave no results. So I think we > don't need to support old-style avc3 codec ids. But indeed we could add a couple > of old-style avc3 cases to TestMPEGUnacceptableCombinations. Acknowledged. https://codereview.chromium.org/1677563003/diff/200001/content/browser/media/... content/browser/media/media_canplaytype_browsertest.cc:1183: IN_PROC_BROWSER_TEST_F(MediaCanPlayTypeTest, CodecSupportTest_HLS) { On 2016/02/18 01:37:50, servolk wrote: > On 2016/02/18 00:32:23, ddorwin wrote: > > Both HLS variants are missing a test for old-style avc1. > > These are Android tests and Android uses a different HLS path. The old-style > translation that I've added is only enabled for video/mp2t mime types and > guarded by BUILDFLAG(ENABLE_MSE_MPEG2TS_DEMUX), so it won't work on Android ATM, > only on Chromecast. Agreed. I meant that we should have a negative test for old-style avc1 like we do for all non-mp2t containers that can contain avc1. I'll add this comment to the new CL you mentioned below. https://codereview.chromium.org/1677563003/diff/200001/content/browser/media/... content/browser/media/media_canplaytype_browsertest.cc:1396: EXPECT_EQ(kMp2tsProbably, CanPlay("'video/mp2t; codecs=\"avc1.66.10\"'")); On 2016/02/18 01:37:50, servolk wrote: > On 2016/02/18 00:32:23, ddorwin wrote: > > Is this supported for avc3? If not, let's add at least one negative entry. > > Perhaps to TestMPEGUnacceptableCombinations. > > https://codereview.chromium.org/1709803003 Acknowledged. Thanks. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
