This change adds VideoDecoderJob::Create() and AudioDecoderJob::Create() so that the decoder jobs can be NULL if we cannot configure it correctly.

It solves a problem that we may pass an invalid surface to the MediaCodec instance during the shut down phase. In this case, the Configure() will return false and we should not use the codec anymore.

BUG=248726
R=acolwell@chromium.org, dwkang@chromium.org, wolenetz@chromium.org

Review URL: https://codereview.chromium.org/16093045

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

6 files changed, 312 insertions, 42 deletions

diff --git a/media/base/android/java/src/org/chromium/media/MediaCodecBridge.java b/media/base/android/java/src/org/chromium/media/MediaCodecBridge.java
index 0348399..86464b7 100644
--- a/media/base/android/java/src/org/chromium/media/MediaCodecBridge.java
+++ b/media/base/android/java/src/org/chromium/media/MediaCodecBridge.java
@@ -161,7 +161,7 @@ class MediaCodecBridge {
         int index = MEDIA_CODEC_ERROR;
         try {
             index = mMediaCodec.dequeueOutputBuffer(info, timeoutUs);
-        } catch(IllegalStateException e) {
+        } catch (IllegalStateException e) {
             Log.e(TAG, "Cannot dequeue output buffer " + e.toString());
         }
         return new DequeueOutputResult(
@@ -169,9 +169,15 @@ class MediaCodecBridge {
     }
 
     @CalledByNative
-    private void configureVideo(MediaFormat format, Surface surface, MediaCrypto crypto,
+    private boolean configureVideo(MediaFormat format, Surface surface, MediaCrypto crypto,
             int flags) {
-        mMediaCodec.configure(format, surface, crypto, flags);
+        try {
+            mMediaCodec.configure(format, surface, crypto, flags);
+            return true;
+        } catch (IllegalStateException e) {
+          Log.e(TAG, "Cannot configure the video codec " + e.toString());
+        }
+        return false;
     }
 
     @CalledByNative
@@ -203,19 +209,25 @@ class MediaCodecBridge {
     }
 
     @CalledByNative
-    private void configureAudio(MediaFormat format, MediaCrypto crypto, int flags,
+    private boolean configureAudio(MediaFormat format, MediaCrypto crypto, int flags,
             boolean playAudio) {
-        mMediaCodec.configure(format, null, crypto, flags);
-        if (playAudio) {
-            int sampleRate = format.getInteger(MediaFormat.KEY_SAMPLE_RATE);
-            int channelCount = format.getInteger(MediaFormat.KEY_CHANNEL_COUNT);
-            int channelConfig = (channelCount == 1) ? AudioFormat.CHANNEL_OUT_MONO :
-                    AudioFormat.CHANNEL_OUT_STEREO;
-            int minBufferSize = AudioTrack.getMinBufferSize(sampleRate, channelConfig,
-                    AudioFormat.ENCODING_PCM_16BIT);
-            mAudioTrack = new AudioTrack(AudioManager.STREAM_MUSIC, sampleRate, channelConfig,
-                    AudioFormat.ENCODING_PCM_16BIT, minBufferSize, AudioTrack.MODE_STREAM);
+        try {
+            mMediaCodec.configure(format, null, crypto, flags);
+            if (playAudio) {
+                int sampleRate = format.getInteger(MediaFormat.KEY_SAMPLE_RATE);
+                int channelCount = format.getInteger(MediaFormat.KEY_CHANNEL_COUNT);
+                int channelConfig = (channelCount == 1) ? AudioFormat.CHANNEL_OUT_MONO :
+                        AudioFormat.CHANNEL_OUT_STEREO;
+                int minBufferSize = AudioTrack.getMinBufferSize(sampleRate, channelConfig,
+                        AudioFormat.ENCODING_PCM_16BIT);
+                mAudioTrack = new AudioTrack(AudioManager.STREAM_MUSIC, sampleRate, channelConfig,
+                        AudioFormat.ENCODING_PCM_16BIT, minBufferSize, AudioTrack.MODE_STREAM);
+            }
+            return true;
+        } catch (IllegalStateException e) {
+            Log.e(TAG, "Cannot configure the audio codec " + e.toString());
         }
+        return false;
     }
 
     @CalledByNative
diff --git a/media/base/android/media_codec_bridge.cc b/media/base/android/media_codec_bridge.cc
index 12841a6..9dd0cea 100644
--- a/media/base/android/media_codec_bridge.cc
+++ b/media/base/android/media_codec_bridge.cc
@@ -197,8 +197,10 @@ bool AudioCodecBridge::Start(
   if (!ConfigureMediaFormat(j_format.obj(), codec, extra_data, extra_data_size))
     return false;
 
-  Java_MediaCodecBridge_configureAudio(
-      env, media_codec(), j_format.obj(), NULL, 0, play_audio);
+  if (!Java_MediaCodecBridge_configureAudio(
+      env, media_codec(), j_format.obj(), NULL, 0, play_audio)) {
+    return false;
+  }
   StartInternal();
   return true;
 }
@@ -336,8 +338,10 @@ bool VideoCodecBridge::Start(
       Java_MediaCodecBridge_createVideoFormat(
           env, j_mime.obj(), size.width(), size.height()));
   DCHECK(!j_format.is_null());
-  Java_MediaCodecBridge_configureVideo(
-      env, media_codec(), j_format.obj(), surface, NULL, 0);
+  if (!Java_MediaCodecBridge_configureVideo(
+      env, media_codec(), j_format.obj(), surface, NULL, 0)) {
+    return false;
+  }
   StartInternal();
   return true;
 }
diff --git a/media/base/android/media_source_player.cc b/media/base/android/media_source_player.cc
index 446805c..95d0660 100644
--- a/media/base/android/media_source_player.cc
+++ b/media/base/android/media_source_player.cc
@@ -52,8 +52,10 @@ base::LazyInstance<VideoDecoderThread>::Leaky
 
 namespace media {
 
-MediaDecoderJob::MediaDecoderJob(base::Thread* thread, bool is_audio)
-    : message_loop_(base::MessageLoopProxy::current()),
+MediaDecoderJob::MediaDecoderJob(
+    base::Thread* thread, MediaCodecBridge* media_codec_bridge, bool is_audio)
+    : media_codec_bridge_(media_codec_bridge),
+      message_loop_(base::MessageLoopProxy::current()),
       thread_(thread),
       needs_flush_(false),
       is_audio_(is_audio),
@@ -66,21 +68,26 @@ MediaDecoderJob::~MediaDecoderJob() {}
 // Class for managing audio decoding jobs.
 class AudioDecoderJob : public MediaDecoderJob {
  public:
-  AudioDecoderJob(
+  virtual ~AudioDecoderJob() {}
+
+  static AudioDecoderJob* Create(
       const AudioCodec audio_codec, int sample_rate,
       int channel_count, const uint8* extra_data, size_t extra_data_size);
-  virtual ~AudioDecoderJob() {}
+
+ private:
+  AudioDecoderJob(MediaCodecBridge* media_codec_bridge);
 };
 
 // Class for managing video decoding jobs.
 class VideoDecoderJob : public MediaDecoderJob {
  public:
-  VideoDecoderJob(
-      const VideoCodec video_codec, const gfx::Size& size, jobject surface);
   virtual ~VideoDecoderJob() {}
 
-  void Configure(
-      const VideoCodec codec, const gfx::Size& size, jobject surface);
+  static VideoDecoderJob* Create(
+      const VideoCodec video_codec, const gfx::Size& size, jobject surface);
+
+ private:
+  VideoDecoderJob(MediaCodecBridge* media_codec_bridge);
 };
 
 void MediaDecoderJob::Decode(
@@ -220,27 +227,38 @@ void MediaDecoderJob::Release() {
     delete this;
 }
 
-VideoDecoderJob::VideoDecoderJob(
-    const VideoCodec video_codec, const gfx::Size& size, jobject surface)
-    : MediaDecoderJob(g_video_decoder_thread.Pointer(), false) {
+VideoDecoderJob* VideoDecoderJob::Create(
+    const VideoCodec video_codec, const gfx::Size& size, jobject surface) {
   scoped_ptr<VideoCodecBridge> codec(VideoCodecBridge::Create(video_codec));
-  codec->Start(video_codec, size, surface);
-  media_codec_bridge_.reset(codec.release());
+  if (codec->Start(video_codec, size, surface))
+    return new VideoDecoderJob(codec.release());
+  return NULL;
 }
 
-AudioDecoderJob::AudioDecoderJob(
+VideoDecoderJob::VideoDecoderJob(MediaCodecBridge* media_codec_bridge)
+    : MediaDecoderJob(g_video_decoder_thread.Pointer(),
+                      media_codec_bridge,
+                      false) {}
+
+AudioDecoderJob* AudioDecoderJob::Create(
     const AudioCodec audio_codec,
     int sample_rate,
     int channel_count,
     const uint8* extra_data,
-    size_t extra_data_size)
-    : MediaDecoderJob(g_audio_decoder_thread.Pointer(), true) {
+    size_t extra_data_size) {
   scoped_ptr<AudioCodecBridge> codec(AudioCodecBridge::Create(audio_codec));
-  codec->Start(audio_codec, sample_rate, channel_count, extra_data,
-               extra_data_size, true);
-  media_codec_bridge_.reset(codec.release());
+  if (codec->Start(audio_codec, sample_rate, channel_count, extra_data,
+                   extra_data_size, true)) {
+    return new AudioDecoderJob(codec.release());
+  }
+  return NULL;
 }
 
+AudioDecoderJob::AudioDecoderJob(MediaCodecBridge* media_codec_bridge)
+    : MediaDecoderJob(g_audio_decoder_thread.Pointer(),
+                      media_codec_bridge,
+                      true) {}
+
 MediaSourcePlayer::MediaSourcePlayer(
     int player_id,
     MediaPlayerManager* manager)
@@ -616,6 +634,8 @@ void MediaSourcePlayer::ClearDecodingData() {
   received_video_ = MediaPlayerHostMsg_ReadFromDemuxerAck_Params();
   audio_access_unit_index_ = 0;
   video_access_unit_index_ = 0;
+  waiting_for_audio_data_ = false;
+  waiting_for_video_data_ = false;
 }
 
 bool MediaSourcePlayer::HasVideo() {
@@ -631,10 +651,11 @@ void MediaSourcePlayer::CreateAudioDecoderJob() {
 
   // Create audio decoder job only if config changes.
   if (HasAudio() && (reconfig_audio_decoder_ || !audio_decoder_job_)) {
-    audio_decoder_job_.reset(new AudioDecoderJob(
+    audio_decoder_job_.reset(AudioDecoderJob::Create(
         audio_codec_, sampling_rate_, num_channels_,
         &audio_extra_data_[0], audio_extra_data_.size()));
-    reconfig_audio_decoder_ =  false;
+    if (audio_decoder_job_)
+      reconfig_audio_decoder_ =  false;
   }
 }
 
@@ -647,10 +668,13 @@ void MediaSourcePlayer::CreateVideoDecoderJob() {
   }
 
   if (reconfig_video_decoder_ || !video_decoder_job_) {
+    // Release the old VideoDecoderJob first so the surface can get released.
     video_decoder_job_.reset();
-    video_decoder_job_.reset(new VideoDecoderJob(
+    // Create the new VideoDecoderJob.
+    video_decoder_job_.reset(VideoDecoderJob::Create(
         video_codec_, gfx::Size(width_, height_), surface_.j_surface().obj()));
-    reconfig_video_decoder_ = false;
+    if (video_decoder_job_)
+      reconfig_video_decoder_ = false;
   }
 
   // Inform the fullscreen view the player is ready.
diff --git a/media/base/android/media_source_player.h b/media/base/android/media_source_player.h
index 836482c..743a9e9 100644
--- a/media/base/android/media_source_player.h
+++ b/media/base/android/media_source_player.h
@@ -69,7 +69,9 @@ class MediaDecoderJob {
   void OnDecodeCompleted();
 
  protected:
-  MediaDecoderJob(base::Thread* thread, bool is_audio);
+  MediaDecoderJob(base::Thread* thread,
+                  MediaCodecBridge* media_codec_bridge,
+                  bool is_audio);
 
   // Release the output buffer and render it.
   void ReleaseOutputBuffer(
@@ -257,6 +259,7 @@ class MEDIA_EXPORT MediaSourcePlayer : public MediaPlayerAndroid {
   // Weak pointer passed to media decoder jobs for callbacks.
   base::WeakPtrFactory<MediaSourcePlayer> weak_this_;
 
+  friend class MediaSourcePlayerTest;
   DISALLOW_COPY_AND_ASSIGN(MediaSourcePlayer);
 };
 
diff --git a/media/base/android/media_source_player_unittest.cc b/media/base/android/media_source_player_unittest.cc
new file mode 100644
index 0000000..e94e1e9
--- /dev/null
+++ b/media/base/android/media_source_player_unittest.cc
@@ -0,0 +1,224 @@
+// Copyright (c) 2013 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 "base/basictypes.h"
+#include "base/memory/scoped_ptr.h"
+#include "media/base/android/media_codec_bridge.h"
+#include "media/base/android/media_player_manager.h"
+#include "media/base/android/media_source_player.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "ui/gl/android/surface_texture_bridge.h"
+
+namespace media {
+
+// Mock of MediaPlayerManager for testing purpose
+class MockMediaPlayerManager : public MediaPlayerManager {
+ public:
+  MockMediaPlayerManager() : num_requests_(0), last_seek_request_id_(0) {}
+  virtual ~MockMediaPlayerManager() {};
+
+  // MediaPlayerManager implementation.
+  virtual void RequestMediaResources(int player_id) OVERRIDE {}
+  virtual void ReleaseMediaResources(int player_id) OVERRIDE {}
+  virtual MediaResourceGetter* GetMediaResourceGetter() OVERRIDE {
+      return NULL;
+  }
+  virtual void OnTimeUpdate(int player_id,
+                            base::TimeDelta current_time) OVERRIDE {}
+  virtual void OnMediaMetadataChanged(
+      int player_id, base::TimeDelta duration, int width, int height,
+      bool success) OVERRIDE {}
+  virtual void OnPlaybackComplete(int player_id) OVERRIDE {}
+  virtual void OnMediaInterrupted(int player_id) OVERRIDE {}
+  virtual void OnBufferingUpdate(int player_id, int percentage) OVERRIDE {}
+  virtual void OnSeekComplete(int player_id,
+                              base::TimeDelta current_time) OVERRIDE {}
+  virtual void OnError(int player_id, int error) OVERRIDE {}
+  virtual void OnVideoSizeChanged(int player_id, int width,
+                                  int height) OVERRIDE {}
+  virtual MediaPlayerAndroid* GetFullscreenPlayer() OVERRIDE { return NULL; }
+  virtual MediaPlayerAndroid* GetPlayer(int player_id) OVERRIDE { return NULL; }
+  virtual void DestroyAllMediaPlayers() OVERRIDE {}
+  virtual void OnReadFromDemuxer(int player_id, media::DemuxerStream::Type type,
+                                 bool seek_done) OVERRIDE {
+    num_requests_++;
+  }
+  virtual void OnMediaSeekRequest(int player_id, base::TimeDelta time_to_seek,
+                                  unsigned seek_request_id) OVERRIDE {
+    last_seek_request_id_ = seek_request_id;
+  }
+  virtual void OnMediaConfigRequest(int player_id) OVERRIDE {}
+  virtual media::MediaDrmBridge* GetDrmBridge(int media_keys_id) OVERRIDE {
+    return NULL;
+  }
+  virtual void OnKeyAdded(int key_id,
+                          const std::string& key_system,
+                          const std::string& session_id) OVERRIDE {}
+  virtual void OnKeyError(int key_id,
+                          const std::string& key_system,
+                          const std::string& session_id,
+                          media::MediaKeys::KeyError error_code,
+                          int system_code) OVERRIDE {}
+  virtual void OnKeyMessage(int key_id,
+                            const std::string& key_system,
+                            const std::string& session_id,
+                            const std::string& message,
+                            const std::string& destination_url) OVERRIDE {}
+
+  int num_requests() { return num_requests_; }
+  unsigned last_seek_request_id() { return last_seek_request_id_; }
+
+ private:
+  // The number of request this object sents for decoding data.
+  int num_requests_;
+  unsigned last_seek_request_id_;
+};
+
+class MediaSourcePlayerTest : public testing::Test {
+ public:
+  MediaSourcePlayerTest() {
+    manager_.reset(new MockMediaPlayerManager());
+    player_.reset(new MediaSourcePlayer(0, manager_.get()));
+  }
+  virtual ~MediaSourcePlayerTest() {}
+
+ protected:
+  // Get the decoder job from the MediaSourcePlayer.
+  MediaDecoderJob* GetMediaDecoderJob(bool is_audio) {
+    if (is_audio)
+      return player_->audio_decoder_job_.get();
+    return player_->video_decoder_job_.get();
+  }
+
+  // Starts decoding the data.
+  void Start(const MediaPlayerHostMsg_DemuxerReady_Params& params) {
+    player_->DemuxerReady(params);
+    player_->Start();
+  }
+
+  // Set the surface to the player.
+  void SetVideoSurface(gfx::ScopedJavaSurface surface) {
+    unsigned last_seek_request_id = manager_->last_seek_request_id();
+    // Calling SetVideoSurface will trigger a seek.
+    player_->SetVideoSurface(surface.Pass());
+    EXPECT_EQ(last_seek_request_id + 1, manager_->last_seek_request_id());
+    // Sending back the seek ACK.
+    player_->OnSeekRequestAck(manager_->last_seek_request_id());
+  }
+
+ protected:
+  scoped_ptr<MockMediaPlayerManager> manager_;
+  scoped_ptr<MediaSourcePlayer> player_;
+
+  DISALLOW_COPY_AND_ASSIGN(MediaSourcePlayerTest);
+};
+
+
+TEST_F(MediaSourcePlayerTest, StartAudioDecoderWithValidConfig) {
+  if (!MediaCodecBridge::IsAvailable())
+    return;
+
+  // Test audio decoder job will be created when codec is successfully started.
+  MediaPlayerHostMsg_DemuxerReady_Params params;
+  params.audio_codec = kCodecVorbis;
+  params.audio_channels = 2;
+  params.audio_sampling_rate = 44100;
+  params.is_audio_encrypted = false;
+  Start(params);
+  EXPECT_TRUE(NULL != GetMediaDecoderJob(true));
+  EXPECT_EQ(1, manager_->num_requests());
+}
+
+TEST_F(MediaSourcePlayerTest, StartAudioDecoderWithInvalidConfig) {
+  if (!MediaCodecBridge::IsAvailable())
+    return;
+
+  // Test audio decoder job will not be created when failed to start the codec.
+  MediaPlayerHostMsg_DemuxerReady_Params params;
+  params.audio_codec = kCodecVorbis;
+  params.audio_channels = 2;
+  params.audio_sampling_rate = 44100;
+  params.is_audio_encrypted = false;
+  uint8 invalid_codec_data[] = { 0x00, 0xff, 0xff, 0xff, 0xff };
+  params.audio_extra_data.insert(params.audio_extra_data.begin(),
+                                 invalid_codec_data, invalid_codec_data + 4);
+  Start(params);
+  EXPECT_TRUE(NULL == GetMediaDecoderJob(true));
+  EXPECT_EQ(0, manager_->num_requests());
+}
+
+TEST_F(MediaSourcePlayerTest, StartVideoCodecWithValidSurface) {
+  if (!MediaCodecBridge::IsAvailable())
+    return;
+
+  // Test video decoder job will be created when surface is valid.
+  scoped_refptr<gfx::SurfaceTextureBridge> surface_texture(
+      new gfx::SurfaceTextureBridge(0));
+  gfx::ScopedJavaSurface surface(surface_texture.get());
+  MediaPlayerHostMsg_DemuxerReady_Params params;
+  params.video_codec = kCodecVP8;
+  params.video_size = gfx::Size(320, 240);
+  params.is_video_encrypted = false;
+  Start(params);
+  // Video decoder job will not be created until surface is available.
+  EXPECT_TRUE(NULL == GetMediaDecoderJob(false));
+  EXPECT_EQ(0, manager_->num_requests());
+
+  SetVideoSurface(surface.Pass());
+  // The decoder job should be ready now.
+  EXPECT_TRUE(NULL != GetMediaDecoderJob(false));
+  EXPECT_EQ(1, manager_->num_requests());
+}
+
+TEST_F(MediaSourcePlayerTest, StartVideoCodecWithInvalidSurface) {
+  if (!MediaCodecBridge::IsAvailable())
+    return;
+
+  // Test video decoder job will be created when surface is valid.
+  scoped_refptr<gfx::SurfaceTextureBridge> surface_texture(
+      new gfx::SurfaceTextureBridge(0));
+  gfx::ScopedJavaSurface surface(surface_texture.get());
+  MediaPlayerHostMsg_DemuxerReady_Params params;
+  params.video_codec = kCodecVP8;
+  params.video_size = gfx::Size(320, 240);
+  params.is_video_encrypted = false;
+  Start(params);
+  // Video decoder job will not be created until surface is available.
+  EXPECT_TRUE(NULL == GetMediaDecoderJob(false));
+  EXPECT_EQ(0, manager_->num_requests());
+
+  // Release the surface texture.
+  surface_texture = NULL;
+  SetVideoSurface(surface.Pass());
+  EXPECT_TRUE(NULL == GetMediaDecoderJob(false));
+  EXPECT_EQ(0, manager_->num_requests());
+}
+
+TEST_F(MediaSourcePlayerTest, ReadFromDemuxerAfterSeek) {
+  if (!MediaCodecBridge::IsAvailable())
+    return;
+
+  // Test decoder job will resend a ReadFromDemuxer request after seek.
+  MediaPlayerHostMsg_DemuxerReady_Params params;
+  params.audio_codec = kCodecVorbis;
+  params.audio_channels = 2;
+  params.audio_sampling_rate = 44100;
+  params.is_audio_encrypted = false;
+  Start(params);
+  EXPECT_TRUE(NULL != GetMediaDecoderJob(true));
+  EXPECT_EQ(1, manager_->num_requests());
+
+  // Initiate a seek
+  player_->SeekTo(base::TimeDelta());
+  EXPECT_EQ(1u, manager_->last_seek_request_id());
+  // Sending back the seek ACK, this should trigger the player to call
+  // OnReadFromDemuxer() again.
+  player_->OnSeekRequestAck(manager_->last_seek_request_id());
+  EXPECT_EQ(2, manager_->num_requests());
+}
+
+
+}  // namespace media
diff --git a/media/media.gyp b/media/media.gyp
index 7f25f09..031ab57 100644
--- a/media/media.gyp
+++ b/media/media.gyp
@@ -859,6 +859,7 @@
         '../skia/skia.gyp:skia',
         '../testing/gmock.gyp:gmock',
         '../testing/gtest.gyp:gtest',
+        '../ui/gl/gl.gyp:gl',
         '../ui/ui.gyp:ui',
       ],
       'sources': [
@@ -890,6 +891,7 @@
         'audio/win/audio_unified_win_unittest.cc',
         'audio/win/core_audio_util_win_unittest.cc',
         'base/android/media_codec_bridge_unittest.cc',
+        'base/android/media_source_player_unittest.cc',
         'base/audio_bus_unittest.cc',
         'base/audio_converter_unittest.cc',
         'base/audio_fifo_unittest.cc',
@@ -1501,6 +1503,7 @@
           ],
           'dependencies': [
             '../base/base.gyp:base',
+            '../url/url.gyp:url_lib',
             'media_android_jni_headers',
           ],
           'include_dirs': [