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

media/formats/mp4/aac.cc

Index: media/formats/mp4/aac.cc
diff --git a/media/formats/mp4/aac.cc b/media/formats/mp4/aac.cc
index 9aa749ca310cffe9eb0520c8c2c6395da439c4f0..4613adebbe3d092b2e3c2de4cd9b716bba5b252f 100644
--- a/media/formats/mp4/aac.cc
+++ b/media/formats/mp4/aac.cc
@@ -58,9 +58,6 @@ bool AAC::Parse(const std::vector<uint8>& data,
     RCHECK(reader.ReadBits(5, &profile_));
   }
 
-  MEDIA_LOG(INFO, media_log) << "Audio codec: mp4a.40." << std::hex
-                             << static_cast<int>(profile_);
-
   RCHECK(SkipDecoderGASpecificConfig(&reader));
   RCHECK(SkipErrorSpecificConfig());
 
@@ -96,12 +93,26 @@ bool AAC::Parse(const std::vector<uint8>& data,
   }
 
   if (frequency_ == 0) {
-    RCHECK(frequency_index_ < kADTSFrequencyTableSize);
+    if (frequency_index_ >= kADTSFrequencyTableSize) {
+      MEDIA_LOG(ERROR, media_log)
+          << "Unsupported Sampling Frequency Index with value 0x"

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

please add a unit test that exposes this log and verifies it occurs.

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

I understood your intention about this comments, but I'm not sure these are
inevitable. I tried to add some MATCHERS to verify the error cases, but it's
hard to find the cases because if the incoming data is invalid, Parse() already
handles it as error before it comes to this line. 
I think the current unittest already well figures out the success cases, and
also errors are well emitted when it happens.
So I would leave the unittest as it is. Or do you have any specific data to
check the error handling on unittest?

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

Is it true that Parse() would error first always? If so, then this is dead code.
I suspect it isn't dead code, and the test data could be constructed to exercise
the error log. Same for those ditto'ed below.

[msu.koo, 2015/09/15 11:10:19]

Done. and ditto'ed below.

+          << std::hex << static_cast<int>(frequency_index_)
+          << ". Please see ISO 14496 Part 3 Table 1.16 "
+          << "for supported Sampling Frequencies.";
+      return false;
+    }
     frequency_ = kADTSFrequencyTable[frequency_index_];
   }
 
   if (extension_frequency_ == 0 && extension_frequency_index != 0xff) {
-    RCHECK(extension_frequency_index < kADTSFrequencyTableSize);
+    if (extension_frequency_index >= kADTSFrequencyTableSize) {
+      MEDIA_LOG(ERROR, media_log)
+          << "Unsupported Extension Sampling Frequency Index with value 0x"

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

ditto

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

ditto

+          << std::hex << static_cast<int>(frequency_index_)
+          << ". Please see ISO 14496 Part 3 Table 1.16 "
+          << "for supported Sampling Frequencies.";
+      return false;
+    }
     extension_frequency_ = kADTSFrequencyTable[extension_frequency_index];
   }
 
@@ -109,12 +120,38 @@ bool AAC::Parse(const std::vector<uint8>& data,
   if (ps_present && channel_config_ == 1) {
     channel_layout_ = CHANNEL_LAYOUT_STEREO;
   } else {
-    RCHECK(channel_config_ < kADTSChannelLayoutTableSize);
+    if (channel_config_ >= kADTSChannelLayoutTableSize) {
+      MEDIA_LOG(ERROR, media_log)
+          << "Unsupported Channel Configuration with value "

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

ditto

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

ditto

+          << static_cast<int>(channel_config_) << ". "
+          << "Please see ISO 14496 Part 3 Table 1.17 "
+          << "for supported Channel Configurations.";
+      return false;
+    }
     channel_layout_ = kADTSChannelLayoutTable[channel_config_];
   }
 
-  return frequency_ != 0 && channel_layout_ != CHANNEL_LAYOUT_NONE &&
-         profile_ >= 1 && profile_ <= 4;
+  RCHECK_MEDIA_LOGGED(frequency_ != 0, media_log,
+                      "Audio frequency is invalid with value 0.");

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

ditto

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

This line is dead. removed.

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

Acknowledged.

+  RCHECK_MEDIA_LOGGED(channel_layout_ != CHANNEL_LAYOUT_NONE, media_log,
+                      "Undefined Audio Channel layout with value "
+                      "CHANNEL_LAYOUT_NONE.");
+  if (profile_ < 1 || profile_ > 4) {
+    MEDIA_LOG(ERROR, media_log)
+        << "Audio codec: mp4a.40." << static_cast<int>(profile_)

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

ditto

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

ditto

+        << " is not supported. "
+        << "Please see ISO 14496 Part 3 Table 1.3 "
+        << "for Audio Profile Definitions.";
+    return false;
+  }
+
+  MEDIA_LOG(INFO, media_log)
+      << "Audio codec: mp4a.40." << static_cast<int>(profile_)
+      << ", Sampling frequency: " << frequency_ << "Hz"
+      << ", Sampling frequency(Extension): " << extension_frequency_ << "Hz"
+      << ", Channel layout: " << channel_layout_;
+
+  return true;
 }
 
 int AAC::GetOutputSamplesPerSecond(bool sbr_in_mimetype) const {