Added unit test from MediaCodecPlayerAndroid

media/filters/audio_decoder_unittest.cc

Index: media/filters/audio_decoder_unittest.cc
diff --git a/media/filters/audio_decoder_unittest.cc b/media/filters/audio_decoder_unittest.cc
index 5f3e96df8811b6f67fbc57849cd00a5ea48461ae..1b5fcef0ad7f7a70f1496af2a34567cd5d3e848a 100644
--- a/media/filters/audio_decoder_unittest.cc
+++ b/media/filters/audio_decoder_unittest.cc
@@ -5,7 +5,6 @@
 #include <stddef.h>
 #include <stdint.h>
 
-#include <deque>
 #include <vector>
 
 #include "base/bind.h"
@@ -16,6 +15,7 @@
 #include "base/run_loop.h"
 #include "base/strings/stringprintf.h"
 #include "base/sys_byteorder.h"
+#include "base/threading/platform_thread.h"
 #include "build/build_config.h"
 #include "media/base/audio_buffer.h"
 #include "media/base/audio_bus.h"
@@ -32,6 +32,10 @@
 #include "media/filters/opus_audio_decoder.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
+#if defined(OS_ANDROID)
+#include "media/filters/android/media_codec_audio_decoder.h"
+#endif
+
 namespace media {
 
 // The number of packets to read and then decode from each file.
@@ -44,6 +48,9 @@ static const uint8_t kOpusExtraData[] = {
 enum AudioDecoderType {
   FFMPEG,
   OPUS,
+#if defined(OS_ANDROID)
+  MEDIA_CODEC,
+#endif
 };
 
 struct DecodedBufferExpectations {
@@ -113,6 +120,11 @@ class AudioDecoderTest : public testing::TestWithParam<DecoderTestData> {
         decoder_.reset(
             new OpusAudioDecoder(message_loop_.task_runner()));
         break;
+#if defined(OS_ANDROID)
+      case MEDIA_CODEC:
+        decoder_.reset(new MediaCodecAudioDecoder(message_loop_.task_runner()));
+        break;
+#endif
     }
   }
 
@@ -129,10 +141,22 @@ class AudioDecoderTest : public testing::TestWithParam<DecoderTestData> {
     decoder_->Decode(
         buffer,
         base::Bind(&AudioDecoderTest::DecodeFinished, base::Unretained(this)));
-    base::RunLoop().RunUntilIdle();
+    WaitForDecodeResponse();
     ASSERT_FALSE(pending_decode_);
   }
 
+  void WaitForDecodeResponse() {

[liberato (no reviews please), 2016/02/29 17:08:57]

"do work for 100msec" seems flaky.  i see that the async nature of MediaCodec
makes this somewhat difficult to avoid, but 100msec seems way too short.

what would you think about mocking out MediaCodec, maybe with a software
decoder?

or, does this test run on multiple android devices?  is its goal to be more of
an integration test?

[Tima Vaisburd, 2016/02/29 21:01:03]

I think that mocking MediaCodec is a good idea, as it would allow to write the
tests that check input/output timestamps, delays for input and output buffers
etc. For mocking, maybe, we can even have pre-canned output and do not have to
use a software decoder. On the other hand, the async nature of MediaCodec seems
to be the most important thing to simulate, and we would have to wait for
response somehow even for the mocked version of decoder.
Yes, I think this is true. 

[liberato (no reviews please), 2016/03/01 22:20:39]

i was thinking about a very simple MediaCodec mock.  make outputs available
immediately.  that lets us test the correctness of a bunch of MCAD code.

i don't think that what we have here really tests that very reliably.  it's
unpredictable what delays we'll see in practice, and also correspondingly hard
to fix if the test is flaky.  the remainder of these tests do not seem to have
that sort of variation in them (at least, not from my quick reading of them).

if this is the first such test to depend on timing, then maybe the mock is more
appropriate.

if these tests already have weird timing stuff happening for other platforms /
codecs, then i don't see a problem with adding another.

[Tima Vaisburd, 2016/03/01 23:35:33]

Even if the MediaCodec returns the output buffer immediately, MCAD posts it, so
some code would have to be modified anyway. See my reply to your second comment.
Well, in theory the tests should never be flaky. The reality is that
we have to deal with this issue on Android platform.
I can only say that on Android platform media_unittests sometimes show
flakiness, and we fight it.

As for mock, I'm still thinking that the mock is a valuable addition, but should
not be a replacement of the real test on device. I see the value of this test in
that it can run on real device and we can verify that it properly decodes.

Besides, I think that even the simplest mock of the MediaCodec goes beyond the
scope of this CL.

Where do you envision the flakiness? Maybe I can improve the code?

[liberato (no reviews please), 2016/03/02 15:02:01]

flakiness: a delay of 100msec seems like something that could easily be flaky. 
i was going to suggest "wait indefinitely", but it looks like dale's suggestion
does that already.  flakiness avoided.

MCAD posts it: but RunUntilIdle would, i think, always catch that.

mock: without the potential flakiness due to timeout, i agree that it's out of
scope.  as for end-to-end testing, i really don't think that what this test does
is a substitute for actually checking that the output plays, arrives in time,
etc.  however, that's _definitely_ out of scope.  :)

+    const base::TimeDelta timeout = base::TimeDelta::FromMilliseconds(100);
+    const base::TimeDelta sleep_time = base::TimeDelta::FromMilliseconds(10);
+
+    for (int i = 0; i < timeout / sleep_time; ++i) {

[liberato (no reviews please), 2016/02/29 17:08:57]

not sure if ::Sleep guarantees not to wake up early, but "Now() < start+timeout"
makes it not matter.

[Tima Vaisburd, 2016/02/29 21:01:03]

Yes, I was not sure whether to use the Sleep(), or to implement timeout with a
timer, as in
https://code.google.com/p/chromium/codesearch#chromium/src/media/base/android...

There I call RunUnitilIdle() in a loop until a condition happens or a timer
fires. The version with the Sleep seems simpler? What do you think?

[liberato (no reviews please), 2016/03/02 15:02:01]

i think that i like dale's suggestion better than either of ours :)

+      base::RunLoop().RunUntilIdle();
+      if (!pending_decode_)
+        break;
+      base::PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(10));
+    }
+  }
+
   void SendEndOfStream() {
     DecodeBuffer(DecoderBuffer::CreateEOSBuffer());
   }
@@ -331,20 +355,19 @@ TEST_P(AudioDecoderTest, ProduceAudioSamples) {
   // Run the test multiple times with a seek back to the beginning in between.
   std::vector<std::string> decoded_audio_md5_hashes;
   for (int i = 0; i < 2; ++i) {
-    for (size_t j = 0; j < kDecodeRuns; ++j) {
-      do {
-        Decode();
-        ASSERT_EQ(last_decode_status(), AudioDecoder::kOk);
-        // Some codecs have a multiple buffer delay and require an extra
-        // Decode() step to extract the desired number of output buffers.
-      } while (j == 0 && decoded_audio_size() == 0);
-
-      // On the first pass record the exact MD5 hash for each decoded buffer.
-      if (i == 0)
+    // Run decoder until we get at least |kDecodeRuns| output buffers.
+    do {
+      Decode();
+      ASSERT_EQ(last_decode_status(), AudioDecoder::kOk);
+    } while (decoded_audio_size() < kDecodeRuns);

[liberato (no reviews please), 2016/02/29 17:08:57]

why did this change?

does MediaCodec have a variable # buffer delay, or is it some side-effect of the
async issues (WaitForDecodeResponse)?

[Tima Vaisburd, 2016/02/29 21:01:03]

I think first. See below.

+
+    // On the first pass record the exact MD5 hash for each decoded buffer.
+    if (i == 0) {
+      for (size_t j = 0; j < kDecodeRuns; ++j)
         decoded_audio_md5_hashes.push_back(GetDecodedAudioMD5(j));
     }
 
-    ASSERT_EQ(kDecodeRuns, decoded_audio_size());
+    ASSERT_LE(kDecodeRuns, decoded_audio_size());

[liberato (no reviews please), 2016/02/29 17:08:57]

why can't we guarantee EQ here?  is it because one must RunUntilIdle, which
might finish more than one Decode request in one call?

[Tima Vaisburd, 2016/02/29 21:01:03]

As far as I understand, this is the consequence of OpenMax interface which
MediaCodec is based upon. There are no guarantees wrt when output buffers will
be available relative to the moments we enqueue input buffers. Even after you
waited for the first output buffer, there is no guarantee, in principle, that
there will be "one in - one out". I think several output buffers can be
available at once, and our decoder implementation returns them all at once.

The only way I see to have ASSERT_EQ is send the EOS first: decode 3 buffers,
then SendEndOfStream(), then wait for 3 output buffers to come.

I can do this if you think the equality is important.

[liberato (no reviews please), 2016/03/01 22:20:39]

i don't think eq is important, but it's probably worth documenting why it's not
eq.

[Tima Vaisburd, 2016/03/01 23:35:33]

I went a little deeper to see why we cannot ASSERT_EQ, or rather, why do I think
it's not worth it.

The original code asserts on l.344 in GetDecodedAudioMD5(j) because the output
buffer is not available immediately (which in my experiment happens because the
output buffer is posted by MediaCodecAudioDecoder internals).

We could decode |kDecodeRuns| buffers and wait for them on the decoded side, but
we do not know whether |kDecoderRuns| on input would be enough to get the same
number on output.

That's why I decided to go for a simpler algorithm. I tried to explain this in
comments.

 
     // On the first pass verify the basic audio hash and sample info.  On the
     // second, verify the exact MD5 sum for each packet.  It shouldn't change.
@@ -354,7 +377,6 @@ TEST_P(AudioDecoderTest, ProduceAudioSamples) {
     }
 
     SendEndOfStream();
-    ASSERT_EQ(kDecodeRuns, decoded_audio_size());
 
     // Seek back to the beginning.  Calls Reset() on the decoder.
     Seek(start_timestamp());
@@ -455,8 +477,20 @@ INSTANTIATE_TEST_CASE_P(OpusAudioDecoderBehavioralTest,
                         OpusAudioDecoderBehavioralTest,
                         testing::ValuesIn(kOpusBehavioralTest));
 
+#if defined(OS_ANDROID)
+
+const DecoderTestData kMediaCodecTests[] = {
+    {MEDIA_CODEC, kCodecOpus, "bear-opus.ogg", kBearOpusExpectations, 24, 48000,
+     CHANNEL_LAYOUT_STEREO},
+};
+
+INSTANTIATE_TEST_CASE_P(MediaCodecAudioDecoderTest,
+                        AudioDecoderTest,
+                        testing::ValuesIn(kMediaCodecTests));
+
+#else  // !defined(OS_ANDROID)
+
 // Disable all FFmpeg decoder tests on Android. http://crbug.com/570762.
-#if !defined(OS_ANDROID)
 
 #if defined(USE_PROPRIETARY_CODECS)
 const DecodedBufferExpectations kSfxMp3Expectations[] = {