Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
918463002:
Add detailed tests of supported avc1, avc3, and mp4a codec strings. (Closed)
|
Created:
5 years, 10 months ago by ddorwin Modified:
5 years, 10 months ago Reviewers:
wolenetz CC:
chromium-reviews, darin-cc_chromium.org, jam, feature-media-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@avc1 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd detailed tests of supported avc1, avc3, and mp4a codec strings.
Committed: https://crrev.com/dcd5be8ba56591e221ae03e1b3390606744e601b
Cr-Commit-Position: refs/heads/master@{#317717}
Patch Set 1 #Patch Set 2 : Fix Android MPEG2 AAC. #Patch Set 3 : rebase #
Total comments: 42
Patch Set 4 : Addressed feedback except avc1/3 reuse #Patch Set 5 : Added TestAvcVariants() helper. #
Total comments: 2
Patch Set 6 : Revert to separate avc1/avc3 tests and populate avc3 test. #
Total comments: 6
Patch Set 7 : addressed feedback #Messages
Total messages: 17 (4 generated)
ddorwin@chromium.org changed reviewers: + wolenetz@chromium.org
ping
On 2015/02/17 19:42:08, ddorwin wrote: > ping My apologies, David. I'm now caught-up from the long weekend and will review this tonight or first thing in the morning.
Thanks for doing this test cleanup and clarification work. https://codereview.chromium.org/918463002/diff/40001/content/browser/media/me... File content/browser/media/media_canplaytype_browsertest.cc (right): https://codereview.chromium.org/918463002/diff/40001/content/browser/media/me... content/browser/media/media_canplaytype_browsertest.cc:92: // Just "mp4a" is not supported like just "avc1" is supported. nit: rework comment. I kept reading this the wrong way as: "mp4a" is not supported, just like "avc1" is supported. [comma and "just like" word order mine]. I suspect the following is clearer: 'Unlike just "avc1" support, just "mp4a" is not supported.' https://codereview.chromium.org/918463002/diff/40001/content/browser/media/me... content/browser/media/media_canplaytype_browsertest.cc:394: TestOGGUnacceptableCombinations("application/ogg"); aside: please fix this one indent while you're touching this file :) https://codereview.chromium.org/918463002/diff/40001/content/browser/media/me... content/browser/media/media_canplaytype_browsertest.cc:457: EXPECT_EQ(kNot, CanPlay("'video/mp4; codecs=\"mp4a\"'")); nit: this is redundant with the "just like" portion of the TestMPEGUnacceptableCombinations("video/mp4") call, below. https://codereview.chromium.org/918463002/diff/40001/content/browser/media/me... content/browser/media/media_canplaytype_browsertest.cc:501: EXPECT_EQ(kNot, CanPlay("'video/x-m4v; codecs=\"mp4a\"'")); nit: ditto redundant w/"just like" portion of call below. https://codereview.chromium.org/918463002/diff/40001/content/browser/media/me... content/browser/media/media_canplaytype_browsertest.cc:542: EXPECT_EQ(kNot, CanPlay("'audio/mp4; codecs=\"mp4a\"'")); nit ditto https://codereview.chromium.org/918463002/diff/40001/content/browser/media/me... content/browser/media/media_canplaytype_browsertest.cc:561: EXPECT_EQ(kNot, CanPlay("'audio/x-m4a; codecs=\"mp4a\"'")); nit ditto https://codereview.chromium.org/918463002/diff/40001/content/browser/media/me... content/browser/media/media_canplaytype_browsertest.cc:580: // When modifying this test, also change CodecSupportTest_Avc3Variants. nit: combine into one test that loops through all avc# or uses helper that is parameterized by avc#? https://codereview.chromium.org/918463002/diff/40001/content/browser/media/me... content/browser/media/media_canplaytype_browsertest.cc:589: EXPECT_EQ(kNot, CanPlay("'video/mp4; codecs=\"avc1.12345\"'")); nit ditto for these two kNot lines: (redundant with CodecSupportTest_mp4's call to TestMPEGUnacceptableCombinations("video/mp4")) https://codereview.chromium.org/918463002/diff/40001/content/browser/media/me... content/browser/media/media_canplaytype_browsertest.cc:593: EXPECT_EQ(kPropProbably, CanPlay("'video/mp4; codecs=\"avc1.42E01E\"'")); hmm there's more redundancy. I suspect many of these redundancy nits are fine left as-is, to clarify each reason why they are/aren't supported. https://codereview.chromium.org/918463002/diff/40001/content/browser/media/me... content/browser/media/media_canplaytype_browsertest.cc:596: // From a YouTube DASH MSE test manifest. 4d401e is duplicated, below. This seems like a partial enumeration still. Is the intent to explicitly list all kPropProbably (and maybe just a subset of kProbMaybe) avc# CanPlay cases in this CL? If so, enumerating support expectations for "all valid levels" would be good to include (in a helper?). Redundancy elimination could be done by compiling the expectations into a deduplicated set and only evaluating that set. WDYT? https://codereview.chromium.org/918463002/diff/40001/content/browser/media/me... content/browser/media/media_canplaytype_browsertest.cc:611: EXPECT_EQ(kPropMaybe, CanPlay("'video/mp4; codecs=\"avc1.42001E\"'")); aside: thanks for formatting these lines to line up in columns. Great idea for readability here! https://codereview.chromium.org/918463002/diff/40001/content/browser/media/me... content/browser/media/media_canplaytype_browsertest.cc:669: // TODO(ddorwin): Copy the final Avc1Variants contents here and s/avc1/avc3/. nit ditto: parameterize avc# and use a helper for each of #={1,3}? https://codereview.chromium.org/918463002/diff/40001/content/browser/media/me... content/browser/media/media_canplaytype_browsertest.cc:673: // Other supported values for the first for hexadecimal digits should behave nit: s/for hex/four hex/ https://codereview.chromium.org/918463002/diff/40001/content/browser/media/me... content/browser/media/media_canplaytype_browsertest.cc:738: // Note: Level 5.2 (0x34) is not considered valid. The version of ISO14496-10 acolwell@ used may have been too old to include 5.2 (it's not in the March 1, 2006 "corrected version" AFAICT). It looks like ITU-T Rec. H.264 | ISO/IEC 14496-10 version 14 was the first to include level 5.2. That said, we'd need to look into s/w decoder (desktop) support, and android support before adding it as kPropProbably. Please file a bug for this. https://codereview.chromium.org/918463002/diff/40001/content/browser/media/me... content/browser/media/media_canplaytype_browsertest.cc:752: EXPECT_EQ(kNot, CanPlay("'audio/mp4; codecs=\"mp4a\"'")); nit ditto redundancy... https://codereview.chromium.org/918463002/diff/40001/content/browser/media/me... content/browser/media/media_canplaytype_browsertest.cc:755: EXPECT_EQ(kNot, CanPlay("'audio/mp4; codecs=\"mp4a.6\"'")); nit: include comments in this method identifying which ones are expected to be "MP3" or "just a number near the MP3 number", for example. As it is, reader could become confused about mp4a.6A: is it MP3 or not, for example. https://codereview.chromium.org/918463002/diff/40001/content/browser/media/me... content/browser/media/media_canplaytype_browsertest.cc:801: EXPECT_EQ(kPropProbably, CanPlay("'audio/mp4; codecs=\"mp4a.40.29\"'")); nit: try mp4a.40.1D, mp4a.40.1d, mp4a.40.41 should be kNot, too (in case someone wrongly converts 29 to or from hex).
Patchset #4 (id:60001) has been deleted
https://codereview.chromium.org/918463002/diff/40001/content/browser/media/me... File content/browser/media/media_canplaytype_browsertest.cc (right): https://codereview.chromium.org/918463002/diff/40001/content/browser/media/me... content/browser/media/media_canplaytype_browsertest.cc:92: // Just "mp4a" is not supported like just "avc1" is supported. On 2015/02/18 22:44:33, wolenetz_OoO_Feb_19 wrote: > nit: rework comment. I kept reading this the wrong way as: "mp4a" is not > supported, just like "avc1" is supported. [comma and "just like" word order > mine]. I suspect the following is clearer: 'Unlike just "avc1" support, just > "mp4a" is not supported.' Done. https://codereview.chromium.org/918463002/diff/40001/content/browser/media/me... content/browser/media/media_canplaytype_browsertest.cc:394: TestOGGUnacceptableCombinations("application/ogg"); On 2015/02/18 22:44:33, wolenetz_OoO_Feb_19 wrote: > aside: please fix this one indent while you're touching this file :) Done. https://codereview.chromium.org/918463002/diff/40001/content/browser/media/me... content/browser/media/media_canplaytype_browsertest.cc:457: EXPECT_EQ(kNot, CanPlay("'video/mp4; codecs=\"mp4a\"'")); On 2015/02/18 22:44:32, wolenetz_OoO_Feb_19 wrote: > nit: this is redundant with the "just like" portion of the > TestMPEGUnacceptableCombinations("video/mp4") call, below. Done. https://codereview.chromium.org/918463002/diff/40001/content/browser/media/me... content/browser/media/media_canplaytype_browsertest.cc:501: EXPECT_EQ(kNot, CanPlay("'video/x-m4v; codecs=\"mp4a\"'")); On 2015/02/18 22:44:33, wolenetz_OoO_Feb_19 wrote: > nit: ditto redundant w/"just like" portion of call below. Done. https://codereview.chromium.org/918463002/diff/40001/content/browser/media/me... content/browser/media/media_canplaytype_browsertest.cc:542: EXPECT_EQ(kNot, CanPlay("'audio/mp4; codecs=\"mp4a\"'")); On 2015/02/18 22:44:33, wolenetz_OoO_Feb_19 wrote: > nit ditto Done. https://codereview.chromium.org/918463002/diff/40001/content/browser/media/me... content/browser/media/media_canplaytype_browsertest.cc:561: EXPECT_EQ(kNot, CanPlay("'audio/x-m4a; codecs=\"mp4a\"'")); On 2015/02/18 22:44:32, wolenetz_OoO_Feb_19 wrote: > nit ditto Done. https://codereview.chromium.org/918463002/diff/40001/content/browser/media/me... content/browser/media/media_canplaytype_browsertest.cc:580: // When modifying this test, also change CodecSupportTest_Avc3Variants. On 2015/02/18 22:44:33, wolenetz_OoO_Feb_19 wrote: > nit: combine into one test that loops through all avc# or uses helper that is > parameterized by avc#? Done, but I don't like it. I think I will revert. See my comment in the latest patch set. https://codereview.chromium.org/918463002/diff/40001/content/browser/media/me... content/browser/media/media_canplaytype_browsertest.cc:589: EXPECT_EQ(kNot, CanPlay("'video/mp4; codecs=\"avc1.12345\"'")); On 2015/02/18 22:44:33, wolenetz_OoO_Feb_19 wrote: > nit ditto for these two kNot lines: (redundant with CodecSupportTest_mp4's call > to TestMPEGUnacceptableCombinations("video/mp4")) Yeah, these tests are quick, and I think it's nice to see the coverage all together. I did remove the "mp4a" ones because that's clearly checked. https://codereview.chromium.org/918463002/diff/40001/content/browser/media/me... content/browser/media/media_canplaytype_browsertest.cc:593: EXPECT_EQ(kPropProbably, CanPlay("'video/mp4; codecs=\"avc1.42E01E\"'")); On 2015/02/18 22:44:32, wolenetz_OoO_Feb_19 wrote: > hmm there's more redundancy. I suspect many of these redundancy nits are fine > left as-is, to clarify each reason why they are/aren't supported. Acknowledged. https://codereview.chromium.org/918463002/diff/40001/content/browser/media/me... content/browser/media/media_canplaytype_browsertest.cc:596: // From a YouTube DASH MSE test manifest. On 2015/02/18 22:44:33, wolenetz_OoO_Feb_19 wrote: > 4d401e is duplicated, below. Yes. This was an attempt to make sure we were covering some "in the wild" values regardless of all the other rules. > > This seems like a partial enumeration still. Is the intent to explicitly list > all kPropProbably (and maybe just a subset of kProbMaybe) avc# CanPlay cases in > this CL? If so, enumerating support expectations for "all valid levels" would be > good to include (in a helper?). Does "partial enumeration" refer to these four or the entire function? The four are everything from an MPD. The rest of the test is not complete, especially for maybe. It tries to cover edges, etc. > > Redundancy elimination could be done by compiling the expectations into a > deduplicated set and only evaluating that set. WDYT? I think that would add complexity and make it difficult to ensure we are testing what we want. What if there was a bug in the map entry? I'm not really concerned about redundancy as long as they run quickly and the test is reasonably sized. https://codereview.chromium.org/918463002/diff/40001/content/browser/media/me... content/browser/media/media_canplaytype_browsertest.cc:611: EXPECT_EQ(kPropMaybe, CanPlay("'video/mp4; codecs=\"avc1.42001E\"'")); On 2015/02/18 22:44:33, wolenetz_OoO_Feb_19 wrote: > aside: thanks for formatting these lines to line up in columns. Great idea for > readability here! Acknowledged. https://codereview.chromium.org/918463002/diff/40001/content/browser/media/me... content/browser/media/media_canplaytype_browsertest.cc:669: // TODO(ddorwin): Copy the final Avc1Variants contents here and s/avc1/avc3/. On 2015/02/18 22:44:33, wolenetz_OoO_Feb_19 wrote: > nit ditto: parameterize avc# and use a helper for each of #={1,3}? Ditto. https://codereview.chromium.org/918463002/diff/40001/content/browser/media/me... content/browser/media/media_canplaytype_browsertest.cc:673: // Other supported values for the first for hexadecimal digits should behave On 2015/02/18 22:44:33, wolenetz_OoO_Feb_19 wrote: > nit: s/for hex/four hex/ Done. https://codereview.chromium.org/918463002/diff/40001/content/browser/media/me... content/browser/media/media_canplaytype_browsertest.cc:738: // Note: Level 5.2 (0x34) is not considered valid. On 2015/02/18 22:44:33, wolenetz_OoO_Feb_19 wrote: > The version of ISO14496-10 acolwell@ used may have been too old to include 5.2 > (it's not in the March 1, 2006 "corrected version" AFAICT). > It looks like ITU-T Rec. H.264 | ISO/IEC 14496-10 version 14 was the first to > include level 5.2. > > That said, we'd need to look into s/w decoder (desktop) support, and android > support before adding it as kPropProbably. Please file a bug for this. Done. https://codereview.chromium.org/918463002/diff/40001/content/browser/media/me... content/browser/media/media_canplaytype_browsertest.cc:752: EXPECT_EQ(kNot, CanPlay("'audio/mp4; codecs=\"mp4a\"'")); On 2015/02/18 22:44:33, wolenetz_OoO_Feb_19 wrote: > nit ditto redundancy... This is specifically testing mp4a, so I think it's fine. https://codereview.chromium.org/918463002/diff/40001/content/browser/media/me... content/browser/media/media_canplaytype_browsertest.cc:755: EXPECT_EQ(kNot, CanPlay("'audio/mp4; codecs=\"mp4a.6\"'")); On 2015/02/18 22:44:33, wolenetz_OoO_Feb_19 wrote: > nit: include comments in this method identifying which ones are expected to be > "MP3" or "just a number near the MP3 number", for example. As it is, reader > could become confused about mp4a.6A: is it MP3 or not, for example. Hmm. I'm not sure how to do this without adding more clutter that hurts readability. I don't want to document "near/like AAC/66" for each of these. I guess the pattern is that there are comments before a series of supported values and "near" values around them. Let me know if you have suggestions. https://codereview.chromium.org/918463002/diff/40001/content/browser/media/me... content/browser/media/media_canplaytype_browsertest.cc:801: EXPECT_EQ(kPropProbably, CanPlay("'audio/mp4; codecs=\"mp4a.40.29\"'")); On 2015/02/18 22:44:33, wolenetz_OoO_Feb_19 wrote: > nit: try mp4a.40.1D, mp4a.40.1d, mp4a.40.41 should be kNot, too (in case someone > wrongly converts 29 to or from hex). Done. https://codereview.chromium.org/918463002/diff/100001/content/browser/media/m... File content/browser/media/media_canplaytype_browsertest.cc (right): https://codereview.chromium.org/918463002/diff/100001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:285: void TestAvcVariants(const std::string& base) { I don't like this: * It is more complex and potentially buggy to be building strings. * The lines are too long and making them fit 80 characters will be ugly. Wrapping will ruin readability. s/" + base + "/"+n+"/ might make some things fit, but also violates style and is hard to read. * You can't just search this file for "avc1.42401E" to see what we think we're reporting for a given value.
FYI, PS4 contains changes based on the review, and PS5 moves the Avc1Variants to a function (and adds some const refs to existing functions).
PS6 reverts the use of a helper function in PS5. It is mostly the same as PS4 with some minor fixes; plus I implemented the avc3 test. I recommend diffing PS6 against PS4 or earlier and not against PS5.
lgtm % nits: https://codereview.chromium.org/918463002/diff/40001/content/browser/media/me... File content/browser/media/media_canplaytype_browsertest.cc (right): https://codereview.chromium.org/918463002/diff/40001/content/browser/media/me... content/browser/media/media_canplaytype_browsertest.cc:580: // When modifying this test, also change CodecSupportTest_Avc3Variants. On 2015/02/20 05:26:24, ddorwin wrote: > On 2015/02/18 22:44:33, wolenetz_OoO_Feb_19 wrote: > > nit: combine into one test that loops through all avc# or uses helper that is > > parameterized by avc#? > > Done, but I don't like it. I think I will revert. See my comment in the latest > patch set. Yeah, I agree with your reasons for not doing this. Mainly I was hoping to reduce code copy-paste and potential maintenance errors across avc1 vs avc3 versions that otherwise test for exactly the same support for the same sets of mime+codec strings (apart from avc#). A mitigation of the wrapping of lines could be to precompute the "base" from a supplied avc # and the rest of the prefix of the mime string (video/mp4..., etc). I think your code searchability comment has the most strength for why not to use a helper; if that could be mitigated without adding yet more complexity then I would opt for using a helper still. As it stands, no helper is better IMO. https://codereview.chromium.org/918463002/diff/40001/content/browser/media/me... content/browser/media/media_canplaytype_browsertest.cc:589: EXPECT_EQ(kNot, CanPlay("'video/mp4; codecs=\"avc1.12345\"'")); On 2015/02/20 05:26:24, ddorwin wrote: > On 2015/02/18 22:44:33, wolenetz_OoO_Feb_19 wrote: > > nit ditto for these two kNot lines: (redundant with CodecSupportTest_mp4's > call > > to TestMPEGUnacceptableCombinations("video/mp4")) > > Yeah, these tests are quick, and I think it's nice to see the coverage all > together. I did remove the "mp4a" ones because that's clearly checked. Acknowledged. https://codereview.chromium.org/918463002/diff/40001/content/browser/media/me... content/browser/media/media_canplaytype_browsertest.cc:596: // From a YouTube DASH MSE test manifest. On 2015/02/20 05:26:23, ddorwin wrote: > On 2015/02/18 22:44:33, wolenetz_OoO_Feb_19 wrote: > > 4d401e is duplicated, below. > > Yes. This was an attempt to make sure we were covering some "in the wild" values > regardless of all the other rules. Acknowledged. > > > > This seems like a partial enumeration still. Is the intent to explicitly list > > all kPropProbably (and maybe just a subset of kProbMaybe) avc# CanPlay cases > in > > this CL? If so, enumerating support expectations for "all valid levels" would > be > > good to include (in a helper?). > > Does "partial enumeration" refer to these four or the entire function? The four > are everything from an MPD. The rest of the test is not complete, especially for > maybe. It tries to cover edges, etc. Sorry, I meant to refer to the entire function. (IOW, maybe we should test the kProbably support for all valid avc1 variants *including* the idc_level.) However, until and unless we actually differentiate a particular codec string's support by more than just "valid level for any avc", we don't need such exhaustive testing here. IIRC, there might be some other variants of avc codec strings that we might add support in future that do condition on a strict subset of the "valid level for any avc", but we don't have that implemented yet, so no need for such testing. > > > > Redundancy elimination could be done by compiling the expectations into a > > deduplicated set and only evaluating that set. WDYT? > > I think that would add complexity and make it difficult to ensure we are testing > what we want. What if there was a bug in the map entry? I'm not really concerned > about redundancy as long as they run quickly and the test is reasonably sized. Yeah, I was more concerned about execution speed in this comment. If we don't easily know *what* we're testing (or complexity to do this significantly increases risk of errors during maintenance), then execution speed matters less, so let's keep with what you have in PS6 ;) https://codereview.chromium.org/918463002/diff/40001/content/browser/media/me... content/browser/media/media_canplaytype_browsertest.cc:669: // TODO(ddorwin): Copy the final Avc1Variants contents here and s/avc1/avc3/. On 2015/02/20 05:26:23, ddorwin wrote: > On 2015/02/18 22:44:33, wolenetz_OoO_Feb_19 wrote: > > nit ditto: parameterize avc# and use a helper for each of #={1,3}? > > Ditto. Acknowledged. https://codereview.chromium.org/918463002/diff/40001/content/browser/media/me... content/browser/media/media_canplaytype_browsertest.cc:752: EXPECT_EQ(kNot, CanPlay("'audio/mp4; codecs=\"mp4a\"'")); On 2015/02/20 05:26:23, ddorwin wrote: > On 2015/02/18 22:44:33, wolenetz_OoO_Feb_19 wrote: > > nit ditto redundancy... > > This is specifically testing mp4a, so I think it's fine. Acknowledged. https://codereview.chromium.org/918463002/diff/40001/content/browser/media/me... content/browser/media/media_canplaytype_browsertest.cc:755: EXPECT_EQ(kNot, CanPlay("'audio/mp4; codecs=\"mp4a.6\"'")); On 2015/02/20 05:26:23, ddorwin wrote: > On 2015/02/18 22:44:33, wolenetz_OoO_Feb_19 wrote: > > nit: include comments in this method identifying which ones are expected to be > > "MP3" or "just a number near the MP3 number", for example. As it is, reader > > could become confused about mp4a.6A: is it MP3 or not, for example. > > Hmm. I'm not sure how to do this without adding more clutter that hurts > readability. I don't want to document "near/like AAC/66" for each of these. > > I guess the pattern is that there are comments before a series of supported > values and "near" values around them. > > Let me know if you have suggestions. (nit) Perhaps a comment section at the top of this test that describes the grouping/sequencing (similar to the comment at top of *_AvcLevels test, above) would prevent clutter while improving readability here. WDYT? https://codereview.chromium.org/918463002/diff/100001/content/browser/media/m... File content/browser/media/media_canplaytype_browsertest.cc (right): https://codereview.chromium.org/918463002/diff/100001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:285: void TestAvcVariants(const std::string& base) { On 2015/02/20 05:26:24, ddorwin wrote: > I don't like this: > * It is more complex and potentially buggy to be building strings. > * The lines are too long and making them fit 80 characters will be ugly. > Wrapping will ruin readability. s/" + base + "/"+n+"/ might make some things > fit, but also violates style and is hard to read. > * You can't just search this file for "avc1.42401E" to see what we think we're > reporting for a given value. Acknowledged (see my reply to PS3 thread while reviewing PS6). https://codereview.chromium.org/918463002/diff/120001/content/browser/media/m... File content/browser/media/media_canplaytype_browsertest.cc (right): https://codereview.chromium.org/918463002/diff/120001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:45: public: nit: :) you fixed the private: indent. How 'bout this one, too? https://codereview.chromium.org/918463002/diff/120001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:92: // Unlike just "avc1", just "mp4a" is not supported.' nit: s/'// https://codereview.chromium.org/918463002/diff/120001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:661: IN_PROC_BROWSER_TEST_F(MediaCanPlayTypeTest, CodecSupportTest_Avc3Variants) { nit: likewise add a comment about need to maintain avc1 when this avc3 test is changed.
The CQ bit was checked by ddorwin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from wolenetz@chromium.org Link to the patchset: https://codereview.chromium.org/918463002/#ps140001 (title: "addressed feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/918463002/140001
https://codereview.chromium.org/918463002/diff/40001/content/browser/media/me... File content/browser/media/media_canplaytype_browsertest.cc (right): https://codereview.chromium.org/918463002/diff/40001/content/browser/media/me... content/browser/media/media_canplaytype_browsertest.cc:580: // When modifying this test, also change CodecSupportTest_Avc3Variants. On 2015/02/23 19:57:45, wolenetz wrote: > On 2015/02/20 05:26:24, ddorwin wrote: > > On 2015/02/18 22:44:33, wolenetz_OoO_Feb_19 wrote: > > > nit: combine into one test that loops through all avc# or uses helper that > is > > > parameterized by avc#? > > > > Done, but I don't like it. I think I will revert. See my comment in the latest > > patch set. > > Yeah, I agree with your reasons for not doing this. Mainly I was hoping to > reduce code copy-paste and potential maintenance errors across avc1 vs avc3 > versions that otherwise test for exactly the same support for the same sets of > mime+codec strings (apart from avc#). A mitigation of the wrapping of lines > could be to precompute the "base" from a supplied avc # and the rest of the > prefix of the mime string (video/mp4..., etc). I think your code searchability > comment has the most strength for why not to use a helper; if that could be > mitigated without adding yet more complexity then I would opt for using a helper > still. As it stands, no helper is better IMO. Acknowledged. https://codereview.chromium.org/918463002/diff/40001/content/browser/media/me... content/browser/media/media_canplaytype_browsertest.cc:755: EXPECT_EQ(kNot, CanPlay("'audio/mp4; codecs=\"mp4a.6\"'")); On 2015/02/23 19:57:44, wolenetz wrote: > On 2015/02/20 05:26:23, ddorwin wrote: > > On 2015/02/18 22:44:33, wolenetz_OoO_Feb_19 wrote: > > > nit: include comments in this method identifying which ones are expected to > be > > > "MP3" or "just a number near the MP3 number", for example. As it is, reader > > > could become confused about mp4a.6A: is it MP3 or not, for example. > > > > Hmm. I'm not sure how to do this without adding more clutter that hurts > > readability. I don't want to document "near/like AAC/66" for each of these. > > > > I guess the pattern is that there are comments before a series of supported > > values and "near" values around them. > > > > Let me know if you have suggestions. > > (nit) Perhaps a comment section at the top of this test that describes the > grouping/sequencing (similar to the comment at top of *_AvcLevels test, above) > would prevent clutter while improving readability here. WDYT? Done. https://codereview.chromium.org/918463002/diff/120001/content/browser/media/m... File content/browser/media/media_canplaytype_browsertest.cc (right): https://codereview.chromium.org/918463002/diff/120001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:45: public: On 2015/02/23 19:57:45, wolenetz wrote: > nit: :) you fixed the private: indent. How 'bout this one, too? Done. https://codereview.chromium.org/918463002/diff/120001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:92: // Unlike just "avc1", just "mp4a" is not supported.' On 2015/02/23 19:57:45, wolenetz wrote: > nit: s/'// Done. https://codereview.chromium.org/918463002/diff/120001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:661: IN_PROC_BROWSER_TEST_F(MediaCanPlayTypeTest, CodecSupportTest_Avc3Variants) { On 2015/02/23 19:57:45, wolenetz wrote: > nit: likewise add a comment about need to maintain avc1 when this avc3 test is > changed. Done.
On 2015/02/24 00:09:21, ddorwin wrote: > https://codereview.chromium.org/918463002/diff/40001/content/browser/media/me... > File content/browser/media/media_canplaytype_browsertest.cc (right): > > https://codereview.chromium.org/918463002/diff/40001/content/browser/media/me... > content/browser/media/media_canplaytype_browsertest.cc:580: // When modifying > this test, also change CodecSupportTest_Avc3Variants. > On 2015/02/23 19:57:45, wolenetz wrote: > > On 2015/02/20 05:26:24, ddorwin wrote: > > > On 2015/02/18 22:44:33, wolenetz_OoO_Feb_19 wrote: > > > > nit: combine into one test that loops through all avc# or uses helper that > > is > > > > parameterized by avc#? > > > > > > Done, but I don't like it. I think I will revert. See my comment in the > latest > > > patch set. > > > > Yeah, I agree with your reasons for not doing this. Mainly I was hoping to > > reduce code copy-paste and potential maintenance errors across avc1 vs avc3 > > versions that otherwise test for exactly the same support for the same sets of > > mime+codec strings (apart from avc#). A mitigation of the wrapping of lines > > could be to precompute the "base" from a supplied avc # and the rest of the > > prefix of the mime string (video/mp4..., etc). I think your code searchability > > comment has the most strength for why not to use a helper; if that could be > > mitigated without adding yet more complexity then I would opt for using a > helper > > still. As it stands, no helper is better IMO. > > Acknowledged. > > https://codereview.chromium.org/918463002/diff/40001/content/browser/media/me... > content/browser/media/media_canplaytype_browsertest.cc:755: EXPECT_EQ(kNot, > CanPlay("'audio/mp4; codecs=\"mp4a.6\"'")); > On 2015/02/23 19:57:44, wolenetz wrote: > > On 2015/02/20 05:26:23, ddorwin wrote: > > > On 2015/02/18 22:44:33, wolenetz_OoO_Feb_19 wrote: > > > > nit: include comments in this method identifying which ones are expected > to > > be > > > > "MP3" or "just a number near the MP3 number", for example. As it is, > reader > > > > could become confused about mp4a.6A: is it MP3 or not, for example. > > > > > > Hmm. I'm not sure how to do this without adding more clutter that hurts > > > readability. I don't want to document "near/like AAC/66" for each of these. > > > > > > I guess the pattern is that there are comments before a series of supported > > > values and "near" values around them. > > > > > > Let me know if you have suggestions. > > > > (nit) Perhaps a comment section at the top of this test that describes the > > grouping/sequencing (similar to the comment at top of *_AvcLevels test, above) > > would prevent clutter while improving readability here. WDYT? > > Done. > > https://codereview.chromium.org/918463002/diff/120001/content/browser/media/m... > File content/browser/media/media_canplaytype_browsertest.cc (right): > > https://codereview.chromium.org/918463002/diff/120001/content/browser/media/m... > content/browser/media/media_canplaytype_browsertest.cc:45: public: > On 2015/02/23 19:57:45, wolenetz wrote: > > nit: :) you fixed the private: indent. How 'bout this one, too? > > Done. > > https://codereview.chromium.org/918463002/diff/120001/content/browser/media/m... > content/browser/media/media_canplaytype_browsertest.cc:92: // Unlike just > "avc1", just "mp4a" is not supported.' > On 2015/02/23 19:57:45, wolenetz wrote: > > nit: s/'// > > Done. > > https://codereview.chromium.org/918463002/diff/120001/content/browser/media/m... > content/browser/media/media_canplaytype_browsertest.cc:661: > IN_PROC_BROWSER_TEST_F(MediaCanPlayTypeTest, CodecSupportTest_Avc3Variants) { > On 2015/02/23 19:57:45, wolenetz wrote: > > nit: likewise add a comment about need to maintain avc1 when this avc3 test is > > changed. > > Done. Thanks for doing this; my lgtm stands :)
Message was sent while issue was closed.
Committed patchset #7 (id:140001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/dcd5be8ba56591e221ae03e1b3390606744e601b Cr-Commit-Position: refs/heads/master@{#317717} |
