Fix audio hashing. Split hash tests out of normal test.

The previous hashing code didn't work for a few reasons:
- Underflow situations (valgrind, etc) would result in different
  hashes due to silence we output during underflow.
- Hashing was endian specific, so tests would fail on ARM.
- Each FillBufferTask hashed all the audio data for all channels
  each time, which breaks when frames_recieved varies under load.

The above have been fixed:
- Hash tests are split out into new BasicPlaybackHashed test.
- Underflow is now disabled for audio hash testing.
- Hashes are computed in little-endian byte order.
- A separate hash context is maintained for each channel and only
  reduced to a single hash at the end.

BUG=129284
TEST=media_unittests under valgrind/tsan.

Review URL: https://chromiumcodereview.appspot.com/10444120

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@140123 0039d316-1c4b-4281-b951-d872f2087c98

7 files changed, 88 insertions, 22 deletions

diff --git a/media/audio/null_audio_sink.cc b/media/audio/null_audio_sink.cc
index b6a921c..b2674fb 100644
--- a/media/audio/null_audio_sink.cc
+++ b/media/audio/null_audio_sink.cc
@@ -6,6 +6,7 @@
 
 #include "base/bind.h"
 #include "base/stringprintf.h"
+#include "base/sys_byteorder.h"
 #include "base/threading/platform_thread.h"
 
 namespace media {
@@ -30,6 +31,12 @@ void NullAudioSink::Initialize(const AudioParameters& params,
     audio_data_.push_back(channel_data);
   }
 
+  if (hash_audio_for_testing_) {
+    md5_channel_contexts_.reset(new base::MD5Context[params.channels()]);
+    for (int i = 0; i < params.channels(); i++)
+      base::MD5Init(&md5_channel_contexts_[i]);
+  }
+
   callback_ = callback;
   initialized_ = true;
 }
@@ -93,15 +100,18 @@ void NullAudioSink::FillBufferTask() {
     int frames_per_millisecond =
         params_.sample_rate() / base::Time::kMillisecondsPerSecond;
 
-    if (hash_audio_for_testing_) {
+    if (hash_audio_for_testing_ && frames_received > 0) {
+      DCHECK_EQ(sizeof(float), sizeof(uint32));
       int channels = audio_data_.size();
-      // Include hash of channel count in case frames_received == 0.
-      base::MD5Update(&md5_context_, base::StringPiece(
-          base::StringPrintf("%d", channels)));
-      for (int channel_index = 0; channel_index < channels; ++channel_index) {
-        base::MD5Update(&md5_context_, base::StringPiece(
-            reinterpret_cast<char*>(audio_data_[channel_index]),
-            sizeof(float) * frames_received));
+      for (int channel_idx = 0; channel_idx < channels; ++channel_idx) {
+        for (int frame_idx = 0; frame_idx < frames_received; frame_idx++) {
+          // Convert float to uint32 w/o conversion loss.
+          uint32 frame = base::ByteSwapToLE32(*reinterpret_cast<uint32*>(
+              &audio_data_[channel_idx][frame_idx]));
+          base::MD5Update(
+              &md5_channel_contexts_[channel_idx], base::StringPiece(
+                  reinterpret_cast<char*>(&frame), sizeof(frame)));
+        }
       }
     }
 
@@ -121,14 +131,22 @@ void NullAudioSink::FillBufferTask() {
 }
 
 void NullAudioSink::StartAudioHashForTesting() {
+  DCHECK(!initialized_);
   hash_audio_for_testing_ = true;
-  base::MD5Init(&md5_context_);
 }
 
 std::string NullAudioSink::GetAudioHashForTesting() {
   DCHECK(hash_audio_for_testing_);
+
+  // Hash all channels into the first channel.
   base::MD5Digest digest;
-  base::MD5Final(&digest, &md5_context_);
+  for (size_t i = 1; i < audio_data_.size(); i++) {
+    base::MD5Final(&digest, &md5_channel_contexts_[i]);
+    base::MD5Update(&md5_channel_contexts_[0], base::StringPiece(
+        reinterpret_cast<char*>(&digest), sizeof(base::MD5Digest)));
+  }
+
+  base::MD5Final(&digest, &md5_channel_contexts_[0]);
   return base::MD5DigestToBase16(digest);
 }
 
diff --git a/media/audio/null_audio_sink.h b/media/audio/null_audio_sink.h
index 9828010..ed28fff 100644
--- a/media/audio/null_audio_sink.h
+++ b/media/audio/null_audio_sink.h
@@ -37,7 +37,8 @@ class MEDIA_EXPORT NullAudioSink
   virtual bool SetVolume(double volume) OVERRIDE;
   virtual void GetVolume(double* volume) OVERRIDE;
 
-  // Enables audio frame hashing and reinitializes the MD5 context.
+  // Enables audio frame hashing and reinitializes the MD5 context.  Must be
+  // called prior to Initialize().
   void StartAudioHashForTesting();
 
   // Returns the MD5 hash of all audio frames seen since the last reset.
@@ -68,7 +69,7 @@ class MEDIA_EXPORT NullAudioSink
 
   // Controls whether or not a running MD5 hash is computed for audio frames.
   bool hash_audio_for_testing_;
-  base::MD5Context md5_context_;
+  scoped_array<base::MD5Context> md5_channel_contexts_;
 
   DISALLOW_COPY_AND_ASSIGN(NullAudioSink);
 };
diff --git a/media/filters/audio_renderer_impl.cc b/media/filters/audio_renderer_impl.cc
index 4f59593..4af31fc 100644
--- a/media/filters/audio_renderer_impl.cc
+++ b/media/filters/audio_renderer_impl.cc
@@ -27,6 +27,7 @@ AudioRendererImpl::AudioRendererImpl(media::AudioRendererSink* sink)
       stopped_(false),
       sink_(sink),
       is_initialized_(false),
+      underflow_disabled_(false),
       read_cb_(base::Bind(&AudioRendererImpl::DecodedAudioReady,
                           base::Unretained(this))) {
 }
@@ -418,7 +419,7 @@ uint32 AudioRendererImpl::FillBuffer(uint8* dest,
       rendered_end_of_stream_ = true;
       host()->NotifyEnded();
     } else if (!algorithm_->CanFillBuffer() && !received_end_of_stream_ &&
-               state_ == kPlaying) {
+               state_ == kPlaying && !underflow_disabled_) {
       state_ = kUnderflow;
       underflow_cb = underflow_cb_;
     } else if (algorithm_->CanFillBuffer()) {
@@ -502,4 +503,9 @@ void AudioRendererImpl::OnRenderError() {
   host()->DisableAudioRenderer();
 }
 
+void AudioRendererImpl::DisableUnderflowForTesting() {
+  DCHECK(!is_initialized_);
+  underflow_disabled_ = true;
+}
+
 }  // namespace media
diff --git a/media/filters/audio_renderer_impl.h b/media/filters/audio_renderer_impl.h
index 29fc7ce..efa59e7 100644
--- a/media/filters/audio_renderer_impl.h
+++ b/media/filters/audio_renderer_impl.h
@@ -57,6 +57,12 @@ class MEDIA_EXPORT AudioRendererImpl
   virtual void ResumeAfterUnderflow(bool buffer_more_audio) OVERRIDE;
   virtual void SetVolume(float volume) OVERRIDE;
 
+  // Disables underflow support.  When used, |state_| will never transition to
+  // kUnderflow resulting in Render calls that underflow returning 0 frames
+  // instead of some number of silence frames.  Must be called prior to
+  // Initialize().
+  void DisableUnderflowForTesting();
+
  protected:
   virtual ~AudioRendererImpl();
 
@@ -198,6 +204,8 @@ class MEDIA_EXPORT AudioRendererImpl
 
   AudioParameters audio_parameters_;
 
+  bool underflow_disabled_;
+
   AudioDecoder::ReadCB read_cb_;
 
   DISALLOW_COPY_AND_ASSIGN(AudioRendererImpl);
diff --git a/media/filters/pipeline_integration_test.cc b/media/filters/pipeline_integration_test.cc
index c3f81e3..cfb3583 100644
--- a/media/filters/pipeline_integration_test.cc
+++ b/media/filters/pipeline_integration_test.cc
@@ -160,10 +160,17 @@ TEST_F(PipelineIntegrationTest, BasicPlayback) {
   Play();
 
   ASSERT_TRUE(WaitUntilOnEnded());
+}
+
+TEST_F(PipelineIntegrationTest, BasicPlaybackHashed) {
+  ASSERT_TRUE(Start(GetTestDataURL("bear-320x240.webm"), PIPELINE_OK, true));
+
+  Play();
+
+  ASSERT_TRUE(WaitUntilOnEnded());
 
-  ASSERT_EQ(GetVideoHash(), "f0be120a90a811506777c99a2cdf7cc1");
-  // TODO(dalecurtis): Hashing is inconsistent, see http://crbug.com/130371
-  // ASSERT_EQ(GetAudioHash(), "9c5656bacee06f50ca90cc916419f84c");
+  EXPECT_EQ(GetVideoHash(), "f0be120a90a811506777c99a2cdf7cc1");
+  EXPECT_EQ(GetAudioHash(), "6138555be3389e6aba4c8e6f70195d50");
 }
 
 TEST_F(PipelineIntegrationTest, EncryptedPlayback) {
diff --git a/media/filters/pipeline_integration_test_base.cc b/media/filters/pipeline_integration_test_base.cc
index b6d5c37..cdc0a73 100644
--- a/media/filters/pipeline_integration_test_base.cc
+++ b/media/filters/pipeline_integration_test_base.cc
@@ -20,7 +20,8 @@ namespace media {
 const char kNullVideoHash[] = "d41d8cd98f00b204e9800998ecf8427e";
 
 PipelineIntegrationTestBase::PipelineIntegrationTestBase()
-    : message_loop_factory_(new MessageLoopFactory()),
+    : hashing_enabled_(false),
+      message_loop_factory_(new MessageLoopFactory()),
       pipeline_(new Pipeline(&message_loop_, new MediaLog())),
       ended_(false),
       pipeline_status_(PIPELINE_OK) {
@@ -89,6 +90,13 @@ bool PipelineIntegrationTestBase::Start(const std::string& url,
   return (pipeline_status_ == PIPELINE_OK);
 }
 
+bool PipelineIntegrationTestBase::Start(const std::string& url,
+                                        PipelineStatus expected_status,
+                                        bool hashing_enabled) {
+  hashing_enabled_ = hashing_enabled;
+  return Start(url, expected_status);
+}
+
 void PipelineIntegrationTestBase::Play() {
   pipeline_->SetPlaybackRate(1);
 }
@@ -169,20 +177,29 @@ PipelineIntegrationTestBase::CreateFilterCollection(
                  base::Unretained(message_loop_factory_.get()),
                  "VideoDecoderThread"));
   collection->AddVideoDecoder(decoder_);
+  // Disable frame dropping if hashing is enabled.
   renderer_ = new VideoRendererBase(
       base::Bind(&PipelineIntegrationTestBase::OnVideoRendererPaint,
                  base::Unretained(this)),
       base::Bind(&PipelineIntegrationTestBase::OnSetOpaque,
                  base::Unretained(this)),
-      false);
+      !hashing_enabled_);
   collection->AddVideoRenderer(renderer_);
   audio_sink_ = new NullAudioSink();
-  audio_sink_->StartAudioHashForTesting();
-  collection->AddAudioRenderer(new AudioRendererImpl(audio_sink_));
+  if (hashing_enabled_)
+    audio_sink_->StartAudioHashForTesting();
+  scoped_refptr<AudioRendererImpl> audio_renderer(new AudioRendererImpl(
+      audio_sink_));
+  // Disable underflow if hashing is enabled.
+  if (hashing_enabled_)
+    audio_renderer->DisableUnderflowForTesting();
+  collection->AddAudioRenderer(audio_renderer);
   return collection.Pass();
 }
 
 void PipelineIntegrationTestBase::OnVideoRendererPaint() {
+  if (!hashing_enabled_)
+    return;
   scoped_refptr<VideoFrame> frame;
   renderer_->GetCurrentFrame(&frame);
   if (frame)
@@ -191,12 +208,14 @@ void PipelineIntegrationTestBase::OnVideoRendererPaint() {
 }
 
 std::string PipelineIntegrationTestBase::GetVideoHash() {
+  DCHECK(hashing_enabled_);
   base::MD5Digest digest;
   base::MD5Final(&digest, &md5_context_);
   return base::MD5DigestToBase16(digest);
 }
 
 std::string PipelineIntegrationTestBase::GetAudioHash() {
+  DCHECK(hashing_enabled_);
   return audio_sink_->GetAudioHashForTesting();
 }
 
diff --git a/media/filters/pipeline_integration_test_base.h b/media/filters/pipeline_integration_test_base.h
index 7faf959..4a30287 100644
--- a/media/filters/pipeline_integration_test_base.h
+++ b/media/filters/pipeline_integration_test_base.h
@@ -38,6 +38,10 @@ class PipelineIntegrationTestBase {
   bool WaitUntilOnEnded();
   PipelineStatus WaitUntilEndedOrError();
   bool Start(const std::string& url, PipelineStatus expected_status);
+  // Enable playback with audio and video hashing enabled.  Frame dropping and
+  // audio underflow will be disabled to ensure consistent hashes.
+  bool Start(const std::string& url, PipelineStatus expected_status,
+             bool hashing_enabled);
   void Play();
   void Pause();
   bool Seek(base::TimeDelta seek_time);
@@ -49,16 +53,19 @@ class PipelineIntegrationTestBase {
 
   // Returns the MD5 hash of all video frames seen.  Should only be called once
   // after playback completes.  First time hashes should be generated with
-  // --video-threads=1 to ensure correctness.
+  // --video-threads=1 to ensure correctness.  Pipeline must have been started
+  // with hashing enabled.
   std::string GetVideoHash();
 
   // Returns the MD5 hash of all audio frames seen.  Should only be called once
-  // after playback completes.
+  // after playback completes.  Pipeline must have been started with hashing
+  // enabled.
   std::string GetAudioHash();
 
  protected:
   MessageLoop message_loop_;
   base::MD5Context md5_context_;
+  bool hashing_enabled_;
   scoped_ptr<MessageLoopFactory> message_loop_factory_;
   scoped_refptr<Pipeline> pipeline_;
   scoped_refptr<FFmpegVideoDecoder> decoder_;