Updated AAC:Parse() to emit better error logs.

media/formats/mp4/aac_unittest.cc

Index: media/formats/mp4/aac_unittest.cc
diff --git a/media/formats/mp4/aac_unittest.cc b/media/formats/mp4/aac_unittest.cc
index 264727f38c3e7ab8f19ee999cf50d40eb0355568..4fd3883d5f486e1d89f3b64a631b98ab6c59acd9 100644
--- a/media/formats/mp4/aac_unittest.cc
+++ b/media/formats/mp4/aac_unittest.cc
@@ -11,6 +11,7 @@
 
 using ::testing::InSequence;
 using ::testing::StrictMock;
+using ::testing::AllOf;
 
 namespace media {
 
@@ -20,6 +21,20 @@ MATCHER_P(AudioCodecLog, codec_string, "") {
   return CONTAINS_STRING(arg, "Audio codec: " + std::string(codec_string));
 }
 
+MATCHER_P(AudioSamplingFrequencyLog, codec_string, "") {

[wolenetz, 2015/09/01 21:22:37]

nit: s/codec/frequency/

[msu.koo, 2015/09/11 23:57:23]

Done.

+  return CONTAINS_STRING(arg,
+                         "Sampling frequency: " + std::string(codec_string));

[wolenetz, 2015/09/01 21:22:37]

nit ditto

[msu.koo, 2015/09/11 23:57:23]

Done.

+}
+
+MATCHER_P(AudioExtensionSamplingFrequencyLog, codec_string, "") {

[wolenetz, 2015/09/01 21:22:37]

nit: s/codec/extension/

[msu.koo, 2015/09/11 23:57:23]

Done.

+  return CONTAINS_STRING(
+      arg, "Sampling frequency(Extension): " + std::string(codec_string));

[wolenetz, 2015/09/01 21:22:37]

nit ditto

[msu.koo, 2015/09/11 23:57:23]

Done.

+}
+
+MATCHER_P(AudioChannelLayoutLog, codec_string, "") {

[wolenetz, 2015/09/01 21:22:37]

nit: s/codec/layout/

[msu.koo, 2015/09/11 23:57:23]

Done.

+  return CONTAINS_STRING(arg, "Channel layout: " + std::string(codec_string));

[wolenetz, 2015/09/01 21:22:37]

nit ditto

[msu.koo, 2015/09/11 23:57:23]

Done.

+}
+
 class AACTest : public testing::Test {
  public:
   AACTest() : media_log_(new StrictMock<MockMediaLog>()) {}
@@ -38,8 +53,10 @@ TEST_F(AACTest, BasicProfileTest) {
 
   data.assign(buffer, buffer + sizeof(buffer));
 
-  EXPECT_MEDIA_LOG(AudioCodecLog("mp4a.40.2"));
-
+  EXPECT_MEDIA_LOG(AllOf(AudioCodecLog("mp4a.40.2"),

[wolenetz, 2015/09/01 21:22:37]

Neat usage of AllOf. I like it. However, we really just have one "success" log
entry with a static format, so we could just have one MATCHER_P4 for it,
parameterized by each of the codec_string, frequency_string, extension_string,
and layout_string. Then, AllOf wouldn't really be needed. I don't really have a
strong feeling other than changing to my suggestion here would prevent
regression if someone forgot to copy one of the "AllOf" entries in some test. It
also makes the overall success string format more easy to verify.

In fact, if somehow the log string contained a layout_string of "31", a matcher
looking for just "3" would pass and give false success. So please also add an
end-of-string delimiter like "." to the success log, and s/AllOf(multiple
matchers)/one success log matcher/

[msu.koo, 2015/09/11 05:15:37]

Thank you for your review.
I also considered about using MATCHER_P4 before using Allof, because it's easier
to implement :). But regarding the expandability and already-implemented
approach(dedicated MATCHER functions by parameter, ex codec_string), I took the
current way.

It's more concise to use MATCHER_P4, but It'll also cause some duplicated
matcher codes with "AudioSamplingFrequencyLog", and will be too dependent with
current output log.

So I would leave as it is, but will address your "delimiter" comment.
WDYT?

On 2015/09/01 21:22:37, wolenetz wrote:

[wolenetz, 2015/09/14 23:17:57]

Thanks. The new delimiters in the matchers are sufficient to prevent false
matching. Using AllOf instead of MATCHER_P4 is not a big deal to me, so it's ok.

+                         AudioSamplingFrequencyLog("44100"),
+                         AudioExtensionSamplingFrequencyLog("0"),
+                         AudioChannelLayoutLog("3")));
   EXPECT_TRUE(Parse(data));
   EXPECT_EQ(aac_.GetOutputSamplesPerSecond(false), 44100);
   EXPECT_EQ(aac_.GetChannelLayout(false), CHANNEL_LAYOUT_STEREO);
@@ -51,8 +68,10 @@ TEST_F(AACTest, ExtensionTest) {
 
   data.assign(buffer, buffer + sizeof(buffer));
 
-  EXPECT_MEDIA_LOG(AudioCodecLog("mp4a.40.2"));
-
+  EXPECT_MEDIA_LOG(AllOf(AudioCodecLog("mp4a.40.2"),
+                         AudioSamplingFrequencyLog("24000"),
+                         AudioExtensionSamplingFrequencyLog("48000"),
+                         AudioChannelLayoutLog("3")));
   EXPECT_TRUE(Parse(data));
   EXPECT_EQ(aac_.GetOutputSamplesPerSecond(false), 48000);
   EXPECT_EQ(aac_.GetOutputSamplesPerSecond(true), 48000);
@@ -69,8 +88,10 @@ TEST_F(AACTest, ImplicitSBR_ChannelConfig0) {
 
   data.assign(buffer, buffer + sizeof(buffer));
 
-  EXPECT_MEDIA_LOG(AudioCodecLog("mp4a.40.2"));
-
+  EXPECT_MEDIA_LOG(AllOf(AudioCodecLog("mp4a.40.2"),
+                         AudioSamplingFrequencyLog("24000"),
+                         AudioExtensionSamplingFrequencyLog("0"),
+                         AudioChannelLayoutLog("2")));
   EXPECT_TRUE(Parse(data));
 
   // Test w/o implict SBR.
@@ -89,8 +110,10 @@ TEST_F(AACTest, ImplicitSBR_ChannelConfig1) {
 
   data.assign(buffer, buffer + sizeof(buffer));
 
-  EXPECT_MEDIA_LOG(AudioCodecLog("mp4a.40.2"));
-
+  EXPECT_MEDIA_LOG(AllOf(AudioCodecLog("mp4a.40.2"),
+                         AudioSamplingFrequencyLog("24000"),
+                         AudioExtensionSamplingFrequencyLog("0"),
+                         AudioChannelLayoutLog("3")));
   EXPECT_TRUE(Parse(data));
 
   // Test w/o implict SBR.
@@ -108,8 +131,10 @@ TEST_F(AACTest, SixChannelTest) {
 
   data.assign(buffer, buffer + sizeof(buffer));
 
-  EXPECT_MEDIA_LOG(AudioCodecLog("mp4a.40.2"));
-
+  EXPECT_MEDIA_LOG(AllOf(AudioCodecLog("mp4a.40.2"),
+                         AudioSamplingFrequencyLog("48000"),
+                         AudioExtensionSamplingFrequencyLog("0"),
+                         AudioChannelLayoutLog("12")));
   EXPECT_TRUE(Parse(data));
   EXPECT_EQ(aac_.GetOutputSamplesPerSecond(false), 48000);
   EXPECT_EQ(aac_.GetChannelLayout(false), CHANNEL_LAYOUT_5_1_BACK);
@@ -130,12 +155,13 @@ TEST_F(AACTest, IncorrectProfileTest) {
   std::vector<uint8> data;
 
   data.assign(buffer, buffer + sizeof(buffer));
-
-  EXPECT_MEDIA_LOG(AudioCodecLog("mp4a.40.0"));
   EXPECT_FALSE(Parse(data));
 
   data[0] = 0x08;
-  EXPECT_MEDIA_LOG(AudioCodecLog("mp4a.40.1"));
+  EXPECT_MEDIA_LOG(AllOf(AudioCodecLog("mp4a.40.1"),
+                         AudioSamplingFrequencyLog("96000"),
+                         AudioExtensionSamplingFrequencyLog("0"),
+                         AudioChannelLayoutLog("2")));
   EXPECT_TRUE(Parse(data));
 
   data[0] = 0x28;
@@ -149,12 +175,14 @@ TEST_F(AACTest, IncorrectFrequencyTest) {
   std::vector<uint8> data;
 
   data.assign(buffer, buffer + sizeof(buffer));
-
   EXPECT_FALSE(Parse(data));
 
   data[0] = 0x0e;
   data[1] = 0x08;
-  EXPECT_MEDIA_LOG(AudioCodecLog("mp4a.40.1"));
+  EXPECT_MEDIA_LOG(AllOf(AudioCodecLog("mp4a.40.1"),
+                         AudioSamplingFrequencyLog("7350"),
+                         AudioExtensionSamplingFrequencyLog("0"),
+                         AudioChannelLayoutLog("2")));
   EXPECT_TRUE(Parse(data));
 }
 
@@ -163,12 +191,13 @@ TEST_F(AACTest, IncorrectChannelTest) {
   std::vector<uint8> data;
 
   data.assign(buffer, buffer + sizeof(buffer));
-
-  EXPECT_MEDIA_LOG(AudioCodecLog("mp4a.40.1")).Times(2);
-
   EXPECT_FALSE(Parse(data));
 
   data[1] = 0x08;
+  EXPECT_MEDIA_LOG(AllOf(AudioCodecLog("mp4a.40.1"),
+                         AudioSamplingFrequencyLog("7350"),
+                         AudioExtensionSamplingFrequencyLog("0"),
+                         AudioChannelLayoutLog("2")));
   EXPECT_TRUE(Parse(data));
 }