Implemented parsing for H.264/AVC codec ids

content/browser/media/media_canplaytype_browsertest.cc

Index: content/browser/media/media_canplaytype_browsertest.cc
diff --git a/content/browser/media/media_canplaytype_browsertest.cc b/content/browser/media/media_canplaytype_browsertest.cc
index 83493ccc317e55bfa32469eddf24d5ab49aa6550..5b714eb52652551dc426bb2dd040bbd6e09e7dd9 100644
--- a/content/browser/media/media_canplaytype_browsertest.cc
+++ b/content/browser/media/media_canplaytype_browsertest.cc
@@ -629,7 +629,7 @@ IN_PROC_BROWSER_TEST_F(MediaCanPlayTypeTest, CodecSupportTest_mp4) {
   EXPECT_EQ(kNot, CanPlay("'video/mp4; codecs=\"avc1.77.30\"'"));
   EXPECT_EQ(kNot, CanPlay("'video/mp4; codecs=\"avc1.100.40\"'"));
 
-  EXPECT_EQ(kPropMaybe, CanPlay("'video/mp4; codecs=\"avc1.42E11E\"'"));
+  EXPECT_EQ(kPropProbably, CanPlay("'video/mp4; codecs=\"avc1.42E01E\"'"));

[ddorwin, 2016/02/17 19:31:20]

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.

[servolk, 2016/02/17 19:49:53]

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.

[ddorwin, 2016/02/17 21:44:21]

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.

[servolk, 2016/02/17 23:57:15]

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.

   EXPECT_EQ(kPropProbably, CanPlay("'video/mp4; codecs=\"avc1.42101E\"'"));
   EXPECT_EQ(kPropProbably, CanPlay("'video/mp4; codecs=\"avc1.42701E\"'"));
   EXPECT_EQ(kPropProbably, CanPlay("'video/mp4; codecs=\"avc1.42F01E\"'"));
@@ -710,7 +710,7 @@ IN_PROC_BROWSER_TEST_F(MediaCanPlayTypeTest, CodecSupportTest_mp4) {
   EXPECT_EQ(kNot, CanPlay("'video/x-m4v; codecs=\"avc1.77.30\"'"));
   EXPECT_EQ(kNot, CanPlay("'video/x-m4v; codecs=\"avc1.100.40\"'"));
 
-  EXPECT_EQ(kPropMaybe, CanPlay("'video/x-m4v; codecs=\"avc1.42E11E\"'"));
+  EXPECT_EQ(kPropProbably, CanPlay("'video/x-m4v; codecs=\"avc1.42E01E\"'"));
   EXPECT_EQ(kPropProbably, CanPlay("'video/x-m4v; codecs=\"avc1.42101E\"'"));
   EXPECT_EQ(kPropProbably, CanPlay("'video/x-m4v; codecs=\"avc1.42701E\"'"));
   EXPECT_EQ(kPropProbably, CanPlay("'video/x-m4v; codecs=\"avc1.42F01E\"'"));
@@ -837,12 +837,17 @@ IN_PROC_BROWSER_TEST_F(MediaCanPlayTypeTest, CodecSupportTest_Avc1Variants) {
   // avc1 without extensions results in "maybe" for compatibility.
   EXPECT_EQ(kPropMaybe, CanPlay("'video/mp4; codecs=\"avc1\"'"));
 
-  // Any 6-digit hexadecimal number will result in at least "maybe".
-  EXPECT_EQ(kPropMaybe, CanPlay("'video/mp4; codecs=\"avc1.123456\"'"));
-  EXPECT_EQ(kPropMaybe, CanPlay("'video/mp4; codecs=\"avc1.ABCDEF\"'"));
-  EXPECT_EQ(kPropMaybe, CanPlay("'video/mp4; codecs=\"avc1.abcdef\"'"));
-  EXPECT_EQ(kNot,       CanPlay("'video/mp4; codecs=\"avc1.12345\"'"));
-  EXPECT_EQ(kNot,       CanPlay("'video/mp4; codecs=\"avc1.1234567\"'"));
+  // A valid-looking 6-digit hexadecimal number will result in at least "maybe".
+  // But the first hex byte after the dot must be a valid profile_idc and the
+  // lower two bits of the second byte/4th digit must be zero.
+  EXPECT_EQ(kPropMaybe, CanPlay("'video/mp4; codecs=\"avc1.42AC23\"'"));
+  EXPECT_EQ(kPropMaybe, CanPlay("'video/mp4; codecs=\"avc1.42ACDF\"'"));
+  EXPECT_EQ(kPropMaybe, CanPlay("'video/mp4; codecs=\"avc1.42acdf\"'"));
+
+  // Invalid profile 0x12.
+  EXPECT_EQ(kNot,       CanPlay("'video/mp4; codecs=\"avc1.123456\"'"));
+  // Valid profile, but the reserved bit is set to 1 (4th digit after dot).
+  EXPECT_EQ(kNot,       CanPlay("'video/mp4; codecs=\"avc1.420100\"'"));
 
   // Both upper and lower case hexadecimal digits are accepted.
   EXPECT_EQ(kPropProbably, CanPlay("'video/mp4; codecs=\"avc1.42E01E\"'"));
@@ -856,55 +861,55 @@ IN_PROC_BROWSER_TEST_F(MediaCanPlayTypeTest, CodecSupportTest_Avc1Variants) {
 
   //
   // Baseline Profile (66 == 0x42).
-  //  The first two digits must be 42. The third digit (constraint_set_flags)
-  //  must be valid hex but it otherwise is ignored. The fourth digit (reserved)
-  //  must be 0. The last two digits must be any valid level.
+  //  The first two digits after the dot must be 42. The third and fourth digits
+  //  contain constraint_set_flags and must be valid hex. The last two digits
+  //  should be any valid H.264 level. If the level value is invalid the result
+  //  will be kMaybe.
   //
   EXPECT_EQ(kPropProbably, CanPlay("'video/mp4; codecs=\"avc1.42001E\"'"));
   EXPECT_EQ(kPropProbably, CanPlay("'video/mp4; codecs=\"avc1.42401E\"'"));
   EXPECT_EQ(kPropProbably, CanPlay("'video/mp4; codecs=\"avc1.42801E\"'"));
   EXPECT_EQ(kPropProbably, CanPlay("'video/mp4; codecs=\"avc1.42E00A\"'"));
+  EXPECT_EQ(kPropMaybe,    CanPlay("'video/mp4; codecs=\"avc1.42E0FF\"'"));

[ddorwin, 2016/02/17 19:31:19]

I assume this is an invalid level. If so, it should probably be after 875 to
match the order in the comment.

[servolk, 2016/02/17 19:49:53]

Done.

   EXPECT_EQ(kNot,          CanPlay("'video/mp4; codecs=\"avc1.42G01E\"'"));
-
-  // The fourth digit must be 0.
-  EXPECT_EQ(kPropMaybe, CanPlay("'video/mp4; codecs=\"avc1.42E11E\"'"));
+  EXPECT_EQ(kNot,          CanPlay("'video/mp4; codecs=\"avc1.42000G\"'"));
 
   //
   // Main Profile (77 == 0x4D).
-  //  The first two digits must be 4D. The third digit (constraint_set_flags)
-  //  must be valid hex but it otherwise is ignored. The fourth digit (reserved)
-  //  must be 0. The last two digits must be any valid level.
+  //  The first two digits after the dot must be 4D. The third and fourth digits
+  //  contain constraint_set_flags and must be valid hex. The last two digits
+  //  should be any valid H.264 level. If the level value is invalid the result
+  //  will be kMaybe.
   //
   EXPECT_EQ(kPropProbably, CanPlay("'video/mp4; codecs=\"avc1.4D001E\"'"));
   EXPECT_EQ(kPropProbably, CanPlay("'video/mp4; codecs=\"avc1.4D400A\"'"));
   EXPECT_EQ(kPropProbably, CanPlay("'video/mp4; codecs=\"avc1.4D800A\"'"));
   EXPECT_EQ(kPropProbably, CanPlay("'video/mp4; codecs=\"avc1.4DE00A\"'"));
+  EXPECT_EQ(kPropMaybe,    CanPlay("'video/mp4; codecs=\"avc1.4DE0FF\"'"));

[ddorwin, 2016/02/17 19:31:20]

ditto

[servolk, 2016/02/17 19:49:54]

Done.

   EXPECT_EQ(kNot,          CanPlay("'video/mp4; codecs=\"avc1.4DG01E\"'"));
-
-  // The fourth digit must be 0.
-  EXPECT_EQ(kPropMaybe, CanPlay("'video/mp4; codecs=\"avc1.4DE11E\"'"));
+  EXPECT_EQ(kNot,          CanPlay("'video/mp4; codecs=\"avc1.4D000G\"'"));
 
   //
   // High Profile (100 == 0x64).
-  //  The first two digits must be 64. The third digit (constraint_set_flags)
-  //  must be valid hex but it otherwise is ignored. The fourth digit (reserved)
-  //  must be 0. The last two digits must be any valid level.
+  //  The first two digits after the dot must be 64. The third and fourth digits
+  //  contain constraint_set_flags and must be valid hex. The last two digits
+  //  should be any valid H.264 level. If the level value is invalid the result
+  //  will be kMaybe.
   //
   EXPECT_EQ(kPropProbably, CanPlay("'video/mp4; codecs=\"avc1.64001E\"'"));
   EXPECT_EQ(kPropProbably, CanPlay("'video/mp4; codecs=\"avc1.64400A\"'"));
   EXPECT_EQ(kPropProbably, CanPlay("'video/mp4; codecs=\"avc1.64800A\"'"));
   EXPECT_EQ(kPropProbably, CanPlay("'video/mp4; codecs=\"avc1.64E00A\"'"));
+  EXPECT_EQ(kPropMaybe,    CanPlay("'video/mp4; codecs=\"avc1.64E0FF\"'"));

[ddorwin, 2016/02/17 19:31:19]

ditto

[servolk, 2016/02/17 19:49:54]

Done.

   EXPECT_EQ(kNot,          CanPlay("'video/mp4; codecs=\"avc1.64G01E\"'"));
-
-  // The fourth digit must be 0.
-  EXPECT_EQ(kPropMaybe, CanPlay("'video/mp4; codecs=\"avc1.64E11E\"'"));
+  EXPECT_EQ(kNot,          CanPlay("'video/mp4; codecs=\"avc1.64000G\"'"));
 

[ddorwin, 2016/02/17 19:31:20]

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.

[servolk, 2016/02/17 19:49:54]

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.

[ddorwin, 2016/02/17 21:44:21]

Okay, thanks for the explanation.

Note: The original comment mistakenly copied the old text from the "avc3"
version.

   //
   //  Other profiles are not known to be supported.
   //
 
   // Extended Profile (88 == 0x58).
-  EXPECT_EQ(kPropMaybe, CanPlay("'video/mp4; codecs=\"avc1.58A01E\"'"));
+  EXPECT_EQ(kPropMaybe, CanPlay("'video/mp4; codecs=\"avc1.58001E\"'"));

[servolk, 2016/02/17 19:49:53]

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.

[ddorwin, 2016/02/17 21:44:21]

Acknowledged. If it is valid to "override" the extended profile, should we have
tests for that as well?

[servolk, 2016/02/17 23:57:15]

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.

 }
 
 // When modifying this test, also change CodecSupportTest_Avc1Variants.

[ddorwin, 2016/02/17 19:31:20]

same as above.

[servolk, 2016/02/17 19:49:53]

See the explanations above.

@@ -912,12 +917,17 @@ IN_PROC_BROWSER_TEST_F(MediaCanPlayTypeTest, CodecSupportTest_Avc3Variants) {
   // avc3 without extensions results in "maybe" for compatibility.
   EXPECT_EQ(kPropMaybe, CanPlay("'video/mp4; codecs=\"avc3\"'"));
 
-  // Any 6-digit hexadecimal number will result in at least "maybe".
-  EXPECT_EQ(kPropMaybe, CanPlay("'video/mp4; codecs=\"avc3.123456\"'"));
-  EXPECT_EQ(kPropMaybe, CanPlay("'video/mp4; codecs=\"avc3.ABCDEF\"'"));
-  EXPECT_EQ(kPropMaybe, CanPlay("'video/mp4; codecs=\"avc3.abcdef\"'"));
-  EXPECT_EQ(kNot,       CanPlay("'video/mp4; codecs=\"avc3.12345\"'"));
-  EXPECT_EQ(kNot,       CanPlay("'video/mp4; codecs=\"avc3.1234567\"'"));
+  // A valid-looking 6-digit hexadecimal number will result in at least "maybe".
+  // But the first hex byte after the dot must be a valid profile_idc and the
+  // lower two bits of the second byte/4th digit must be zero.
+  EXPECT_EQ(kPropMaybe, CanPlay("'video/mp4; codecs=\"avc3.42AC23\"'"));
+  EXPECT_EQ(kPropMaybe, CanPlay("'video/mp4; codecs=\"avc3.42ACDF\"'"));
+  EXPECT_EQ(kPropMaybe, CanPlay("'video/mp4; codecs=\"avc3.42acdf\"'"));
+
+  // Invalid profile 0x12.
+  EXPECT_EQ(kNot,       CanPlay("'video/mp4; codecs=\"avc3.123456\"'"));
+  // Valid profile, but the reserved bit is set to 1 (4th digit after dot).
+  EXPECT_EQ(kNot,       CanPlay("'video/mp4; codecs=\"avc3.420100\"'"));
 
   // Both upper and lower case hexadecimal digits are accepted.
   EXPECT_EQ(kPropProbably, CanPlay("'video/mp4; codecs=\"avc3.42E01E\"'"));
@@ -931,55 +941,55 @@ IN_PROC_BROWSER_TEST_F(MediaCanPlayTypeTest, CodecSupportTest_Avc3Variants) {
 
   //
   // Baseline Profile (66 == 0x42).
-  //  The first two digits must be 42. The third digit (constraint_set_flags)
-  //  must be valid hex but it otherwise is ignored. The fourth digit (reserved)
-  //  must be 0. The last two digits must be any valid level.
+  //  The first two digits after the dot must be 42. The third and fourth digits
+  //  contain constraint_set_flags and must be valid hex. The last two digits
+  //  should be any valid H.264 level. If the level value is invalid the result
+  //  will be kMaybe.
   //
   EXPECT_EQ(kPropProbably, CanPlay("'video/mp4; codecs=\"avc3.42001E\"'"));
   EXPECT_EQ(kPropProbably, CanPlay("'video/mp4; codecs=\"avc3.42400A\"'"));
   EXPECT_EQ(kPropProbably, CanPlay("'video/mp4; codecs=\"avc3.42800A\"'"));
   EXPECT_EQ(kPropProbably, CanPlay("'video/mp4; codecs=\"avc3.42E00A\"'"));
+  EXPECT_EQ(kPropMaybe,    CanPlay("'video/mp4; codecs=\"avc3.42E0FF\"'"));
   EXPECT_EQ(kNot,          CanPlay("'video/mp4; codecs=\"avc3.42G01E\"'"));
-
-  // The fourth digit must be 0.
-  EXPECT_EQ(kPropMaybe, CanPlay("'video/mp4; codecs=\"avc3.42E11E\"'"));
+  EXPECT_EQ(kNot,          CanPlay("'video/mp4; codecs=\"avc3.42000G\"'"));
 
   //
   // Main Profile (77 == 0x4D).
-  //  The first two digits must be 4D. The third digit (constraint_set_flags)
-  //  must be valid hex but it otherwise is ignored. The fourth digit (reserved)
-  //  must be 0. The last two digits must be any valid level.
+  //  The first two digits after the dot must be 4D. The third and fourth digits
+  //  contain constraint_set_flags and must be valid hex. The last two digits
+  //  should be any valid H.264 level. If the level value is invalid the result
+  //  will be kMaybe.
   //
   EXPECT_EQ(kPropProbably, CanPlay("'video/mp4; codecs=\"avc3.4D001E\"'"));
   EXPECT_EQ(kPropProbably, CanPlay("'video/mp4; codecs=\"avc3.4D400A\"'"));
   EXPECT_EQ(kPropProbably, CanPlay("'video/mp4; codecs=\"avc3.4D800A\"'"));
   EXPECT_EQ(kPropProbably, CanPlay("'video/mp4; codecs=\"avc3.4DE00A\"'"));
+  EXPECT_EQ(kPropMaybe,    CanPlay("'video/mp4; codecs=\"avc3.4DE0FF\"'"));
   EXPECT_EQ(kNot,          CanPlay("'video/mp4; codecs=\"avc3.4DG01E\"'"));
-
-  // The fourth digit must be 0.
-  EXPECT_EQ(kPropMaybe, CanPlay("'video/mp4; codecs=\"avc3.4DE11E\"'"));
+  EXPECT_EQ(kNot,          CanPlay("'video/mp4; codecs=\"avc3.4D000G\"'"));
 
   //
   // High Profile (100 == 0x64).
-  //  The first two digits must be 64. The third digit (constraint_set_flags)
-  //  must be valid hex but it otherwise is ignored. The fourth digit (reserved)
-  //  must be 0. The last two digits must be any valid level.
+  //  The first two digits after the dot must be 64. The third and fourth digits
+  //  contain constraint_set_flags and must be valid hex. The last two digits
+  //  should be any valid H.264 level. If the level value is invalid the result
+  //  will be kMaybe.
   //
   EXPECT_EQ(kPropProbably, CanPlay("'video/mp4; codecs=\"avc3.64001E\"'"));
   EXPECT_EQ(kPropProbably, CanPlay("'video/mp4; codecs=\"avc3.64400A\"'"));
   EXPECT_EQ(kPropProbably, CanPlay("'video/mp4; codecs=\"avc3.64800A\"'"));
   EXPECT_EQ(kPropProbably, CanPlay("'video/mp4; codecs=\"avc3.64E00A\"'"));
+  EXPECT_EQ(kPropMaybe,    CanPlay("'video/mp4; codecs=\"avc3.64E0FF\"'"));
   EXPECT_EQ(kNot,          CanPlay("'video/mp4; codecs=\"avc3.64G01E\"'"));
-
-  // The fourth digit must be 0.
-  EXPECT_EQ(kPropMaybe, CanPlay("'video/mp4; codecs=\"avc3.64E11E\"'"));
+  EXPECT_EQ(kNot,          CanPlay("'video/mp4; codecs=\"avc3.64000G\"'"));
 
   //
   //  Other profiles are not known to be supported.
   //
 
   // Extended Profile (88 == 0x58).
-  EXPECT_EQ(kPropMaybe, CanPlay("'video/mp4; codecs=\"avc3.58A01E\"'"));
+  EXPECT_EQ(kPropMaybe, CanPlay("'video/mp4; codecs=\"avc3.58001E\"'"));
 }
 
 // Tests AVC levels using AVC1 Baseline (0x42E0zz).
@@ -1182,8 +1192,8 @@ IN_PROC_BROWSER_TEST_F(MediaCanPlayTypeTest, CodecSupportTest_HLS) {
   EXPECT_EQ(probablyCanPlayHLS,
             CanPlay("'application/x-mpegurl; codecs=\"avc3.42C01E\"'"));
 
-  EXPECT_EQ(maybeCanPlayHLS,
-            CanPlay("'application/x-mpegurl; codecs=\"avc1.42E11E\"'"));
+  EXPECT_EQ(probablyCanPlayHLS,
+            CanPlay("'application/x-mpegurl; codecs=\"avc1.42E01E\"'"));

[ddorwin, 2016/02/17 19:31:20]

ditto here and 1272

[servolk, 2016/02/17 19:49:53]

See the explanations above.

   EXPECT_EQ(probablyCanPlayHLS,
             CanPlay("'application/x-mpegurl; codecs=\"avc1.42101E\"'"));
   EXPECT_EQ(probablyCanPlayHLS,
@@ -1259,8 +1269,8 @@ IN_PROC_BROWSER_TEST_F(MediaCanPlayTypeTest, CodecSupportTest_HLS) {
   EXPECT_EQ(probablyCanPlayHLS,
             CanPlay("'application/vnd.apple.mpegurl; codecs=\"avc3.42C01E\"'"));
 
-  EXPECT_EQ(maybeCanPlayHLS,
-            CanPlay("'application/vnd.apple.mpegurl; codecs=\"avc1.42E11E\"'"));
+  EXPECT_EQ(probablyCanPlayHLS,
+            CanPlay("'application/vnd.apple.mpegurl; codecs=\"avc1.42E01E\"'"));
   EXPECT_EQ(probablyCanPlayHLS,
             CanPlay("'application/vnd.apple.mpegurl; codecs=\"avc1.42101E\"'"));
   EXPECT_EQ(probablyCanPlayHLS,