Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1709803003:
Refactored tests for old-style avc1/avc3 codec strings (Closed)
|
Created:
4 years, 10 months ago by servolk Modified:
4 years, 10 months ago Reviewers:
ddorwin CC:
chromium-reviews, darin-cc_chromium.org, jam, feature-media-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRefactored tests for old-style avc1/avc3 codec strings
Added negative test cases for old-style avc3 codecs, these are not used
by anybody and not supported. Also reorganized the tests for old-style
avc1 codec ids to improve test coverage (add Android HLS test coverage)
while minimizing test code footprint.
Committed: https://crrev.com/3b0df24e46e0d473a2f56843b7e0e855a9bf9468
Cr-Commit-Position: refs/heads/master@{#376303}
Patch Set 1 #
Total comments: 6
Patch Set 2 : CR feedback #
Total comments: 2
Patch Set 3 : Changed avc1/avc3 order #Messages
Total messages: 19 (9 generated)
servolk@chromium.org changed reviewers: + ddorwin@chromium.org
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/1709803003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1709803003/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. Only full committers are accepted. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
Thanks. https://codereview.chromium.org/1709803003/diff/1/content/browser/media/media... File content/browser/media/media_canplaytype_browsertest.cc (right): https://codereview.chromium.org/1709803003/diff/1/content/browser/media/media... content/browser/media/media_canplaytype_browsertest.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. You might provide a little more detail in the CL description, such as your reply at https://codereview.chromium.org/1677563003/diff/200001/content/browser/media/.... https://codereview.chromium.org/1709803003/diff/1/content/browser/media/media... content/browser/media/media_canplaytype_browsertest.cc:141: EXPECT_EQ(kNot, CanPlay("'" + mime + "; codecs=\"avc3.77.31\"'")); 77.31 does not appear in the positive avc1 tests. If we're checking that something is not supported, the rest of it should be. Also, we should use the same values used for the negative tests of avc1 throughout. Thus, this should be .30. https://codereview.chromium.org/1709803003/diff/1/content/browser/media/media... content/browser/media/media_canplaytype_browsertest.cc:1157: IN_PROC_BROWSER_TEST_F(MediaCanPlayTypeTest, CodecSupportTest_HLS) { (Carryover from https://codereview.chromium.org/1677563003/.) Add the negative tests for old-style avc1 from lines 632-634 for each of the two HLS container strings just as we have for other containers that can contain avc1 but do not support the old-style strings. avc3 is covered by TestMPEGUnacceptableCombinations. Alternatively, we could move the avc1 checks to TestMPEGUnacceptableCombinations but skip the test for 'video/mp2t'. That might be cleaner since we could remove all these negative tests caused by a very minor use case.
https://codereview.chromium.org/1709803003/diff/1/content/browser/media/media... File content/browser/media/media_canplaytype_browsertest.cc (right): https://codereview.chromium.org/1709803003/diff/1/content/browser/media/media... content/browser/media/media_canplaytype_browsertest.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2016/02/18 17:50:46, ddorwin wrote: > You might provide a little more detail in the CL description, such as your reply > at > https://codereview.chromium.org/1677563003/diff/200001/content/browser/media/.... Done. https://codereview.chromium.org/1709803003/diff/1/content/browser/media/media... content/browser/media/media_canplaytype_browsertest.cc:141: EXPECT_EQ(kNot, CanPlay("'" + mime + "; codecs=\"avc3.77.31\"'")); On 2016/02/18 17:50:46, ddorwin wrote: > 77.31 does not appear in the positive avc1 tests. If we're checking that > something is not supported, the rest of it should be. Also, we should use the > same values used for the negative tests of avc1 throughout. > > Thus, this should be .30. Done. https://codereview.chromium.org/1709803003/diff/1/content/browser/media/media... content/browser/media/media_canplaytype_browsertest.cc:1157: IN_PROC_BROWSER_TEST_F(MediaCanPlayTypeTest, CodecSupportTest_HLS) { On 2016/02/18 17:50:46, ddorwin wrote: > (Carryover from https://codereview.chromium.org/1677563003/.) > > Add the negative tests for old-style avc1 from lines 632-634 for each of the two > HLS container strings just as we have for other containers that can contain avc1 > but do not support the old-style strings. avc3 is covered by > TestMPEGUnacceptableCombinations. > > Alternatively, we could move the avc1 checks to TestMPEGUnacceptableCombinations > but skip the test for 'video/mp2t'. That might be cleaner since we could remove > all these negative tests caused by a very minor use case. Done (moved the avc1 checks to TestMPEGUnacceptableCombinations per the latter suggestion).
Description was changed from ========== Add negative tests for old-style avc3 codec strings ========== to ========== Add negative tests for old-style avc3 codec strings Added negative test cases for old-style avc3 codecs, these are not used by anybody and not supported. Also reorganized the tests for old-style avc1 codec ids to improve test coverage (add Android HLS test coverage) while minimizing test code footprint. ==========
Description was changed from ========== Add negative tests for old-style avc3 codec strings Added negative test cases for old-style avc3 codecs, these are not used by anybody and not supported. Also reorganized the tests for old-style avc1 codec ids to improve test coverage (add Android HLS test coverage) while minimizing test code footprint. ========== to ========== Refactored tests for old-style avc1/avc3 codec strings Added negative test cases for old-style avc3 codecs, these are not used by anybody and not supported. Also reorganized the tests for old-style avc1 codec ids to improve test coverage (add Android HLS test coverage) while minimizing test code footprint. ==========
lgtm with nit. Thanks. https://codereview.chromium.org/1709803003/diff/20001/content/browser/media/m... File content/browser/media/media_canplaytype_browsertest.cc (right): https://codereview.chromium.org/1709803003/diff/20001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:143: // Old-style avc1 codec ids are supported only for video/mp2t container. nit: avc1 should probalby be before avc3 even though avc1 is more complex.
https://codereview.chromium.org/1709803003/diff/20001/content/browser/media/m... File content/browser/media/media_canplaytype_browsertest.cc (right): https://codereview.chromium.org/1709803003/diff/20001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:143: // Old-style avc1 codec ids are supported only for video/mp2t container. On 2016/02/18 21:44:35, ddorwin wrote: > nit: avc1 should probalby be before avc3 even though avc1 is more complex. Done.
The CQ bit was checked by servolk@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ddorwin@chromium.org Link to the patchset: https://codereview.chromium.org/1709803003/#ps40001 (title: "Changed avc1/avc3 order")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1709803003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1709803003/40001
Message was sent while issue was closed.
Description was changed from ========== Refactored tests for old-style avc1/avc3 codec strings Added negative test cases for old-style avc3 codecs, these are not used by anybody and not supported. Also reorganized the tests for old-style avc1 codec ids to improve test coverage (add Android HLS test coverage) while minimizing test code footprint. ========== to ========== Refactored tests for old-style avc1/avc3 codec strings Added negative test cases for old-style avc3 codecs, these are not used by anybody and not supported. Also reorganized the tests for old-style avc1 codec ids to improve test coverage (add Android HLS test coverage) while minimizing test code footprint. ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Refactored tests for old-style avc1/avc3 codec strings Added negative test cases for old-style avc3 codecs, these are not used by anybody and not supported. Also reorganized the tests for old-style avc1 codec ids to improve test coverage (add Android HLS test coverage) while minimizing test code footprint. ========== to ========== Refactored tests for old-style avc1/avc3 codec strings Added negative test cases for old-style avc3 codecs, these are not used by anybody and not supported. Also reorganized the tests for old-style avc1 codec ids to improve test coverage (add Android HLS test coverage) while minimizing test code footprint. Committed: https://crrev.com/3b0df24e46e0d473a2f56843b7e0e855a9bf9468 Cr-Commit-Position: refs/heads/master@{#376303} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/3b0df24e46e0d473a2f56843b7e0e855a9bf9468 Cr-Commit-Position: refs/heads/master@{#376303} |
