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

media/formats/mp4/aac_unittest.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include <string>
 
 #include "media/base/mock_media_log.h"
 #include "media/formats/mp4/aac.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 using ::testing::InSequence;
 using ::testing::StrictMock;
+using ::testing::AllOf;
 
 namespace media {
 
 namespace mp4 {
 
 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>()) {}
 
   bool Parse(const std::vector<uint8>& data) {
     return aac_.Parse(data, media_log_);
   }
 
   scoped_refptr<StrictMock<MockMediaLog>> media_log_;
   AAC aac_;
 };
 
 TEST_F(AACTest, BasicProfileTest) {
   uint8 buffer[] = {0x12, 0x10};
   std::vector<uint8> data;
 
   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);
 }
 
 TEST_F(AACTest, ExtensionTest) {
   uint8 buffer[] = {0x13, 0x08, 0x56, 0xe5, 0x9d, 0x48, 0x80};
   std::vector<uint8> data;
 
   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);
   EXPECT_EQ(aac_.GetChannelLayout(false), CHANNEL_LAYOUT_STEREO);
 }
 
 // Test implicit SBR with mono channel config.
 // Mono channel layout should only be reported if SBR is not
 // specified. Otherwise stereo should be reported.
 // See ISO-14496-3 Section 1.6.6.1.2 for details about this special casing.
 TEST_F(AACTest, ImplicitSBR_ChannelConfig0) {
   uint8 buffer[] = {0x13, 0x08};
   std::vector<uint8> data;
 
   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.
   EXPECT_EQ(aac_.GetOutputSamplesPerSecond(false), 24000);
   EXPECT_EQ(aac_.GetChannelLayout(false), CHANNEL_LAYOUT_MONO);
 
   // Test implicit SBR.
   EXPECT_EQ(aac_.GetOutputSamplesPerSecond(true), 48000);
   EXPECT_EQ(aac_.GetChannelLayout(true), CHANNEL_LAYOUT_STEREO);
 }
 
 // Tests implicit SBR with a stereo channel config.
 TEST_F(AACTest, ImplicitSBR_ChannelConfig1) {
   uint8 buffer[] = {0x13, 0x10};
   std::vector<uint8> data;
 
   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.
   EXPECT_EQ(aac_.GetOutputSamplesPerSecond(false), 24000);
   EXPECT_EQ(aac_.GetChannelLayout(false), CHANNEL_LAYOUT_STEREO);
 
   // Test implicit SBR.
   EXPECT_EQ(aac_.GetOutputSamplesPerSecond(true), 48000);
   EXPECT_EQ(aac_.GetChannelLayout(true), CHANNEL_LAYOUT_STEREO);
 }
 
 TEST_F(AACTest, SixChannelTest) {
   uint8 buffer[] = {0x11, 0xb0};
   std::vector<uint8> data;
 
   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);
 }
 
 TEST_F(AACTest, DataTooShortTest) {
   std::vector<uint8> data;
 
   EXPECT_FALSE(Parse(data));
 
   data.push_back(0x12);
   EXPECT_FALSE(Parse(data));
 }
 
 TEST_F(AACTest, IncorrectProfileTest) {
   InSequence s;
   uint8 buffer[] = {0x0, 0x08};
   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;
   // No media log for this profile 5, since not enough bits are in |data| to
   // first parse profile 5's extension frequency index.
   EXPECT_FALSE(Parse(data));
 }
 
 TEST_F(AACTest, IncorrectFrequencyTest) {
   uint8 buffer[] = {0x0f, 0x88};
   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));
 }
 
 TEST_F(AACTest, IncorrectChannelTest) {
   uint8 buffer[] = {0x0e, 0x00};
   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));
 }
 
 }  // namespace mp4
 
 }  // namespace media