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

media/formats/mp4/aac.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 "media/formats/mp4/aac.h"
 
 #include <algorithm>
 
 #include "base/logging.h"
 #include "media/base/bit_reader.h"
@@ skipping 40 matching lines @@
   // Read extension configuration.
   if (profile_ == 5 || profile_ == 29) {
     ps_present = (profile_ == 29);
     extension_type = 5;
     RCHECK(reader.ReadBits(4, &extension_frequency_index));
     if (extension_frequency_index == 0xf)
       RCHECK(reader.ReadBits(24, &extension_frequency_));
     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());
 
   // Read extension configuration again
   // Note: The check for 16 available bits comes from the AAC spec.
   if (extension_type != 5 && reader.bits_available() >= 16) {
     uint16 sync_extension_type;
     uint8 sbr_present_flag;
     uint8 ps_present_flag;
 
@@ skipping 15 matching lines @@
               RCHECK(reader.ReadBits(1, &ps_present_flag));
               ps_present = ps_present_flag != 0;
             }
           }
         }
       }
     }
   }
 
   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];
   }
 
   // When Parametric Stereo is on, mono will be played as stereo.
   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 {
   if (extension_frequency_ > 0)
     return extension_frequency_;
 
   if (!sbr_in_mimetype)
     return frequency_;
 
   // The following code is written according to ISO 14496 Part 3 Table 1.11 and
@@ skipping 113 matching lines @@
 
     RCHECK(bit_reader->ReadBits(1, &dummy));  // extensionFlag3
   }
 
   return true;
 }
 
 }  // namespace mp4
 
 }  // namespace media