fix a problem that surface can be deleted before calling VideoCodecBridge::Start()

If a surface is released, MediaCodec will throw an exception when calling configure. This can crash chrome.
Supress the exception and return an error instead.

BUG=248726

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

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@206580 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..713a574
--- /dev/null
+++ b/media/base/android/media_source_player_unittest.cc
@@ -0,0 +1,224 @@
+// Copyright 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 22a150f..3c01b19 100644
--- a/media/media.gyp
+++ b/media/media.gyp
@@ -890,6 +890,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',
@@ -1492,6 +1493,8 @@
           ],
           'dependencies': [
             '../base/base.gyp:base',
+            '../ui/gl/gl.gyp:gl',
+            '../url/url.gyp:url_lib',
             'media_android_jni_headers',
           ],
           'include_dirs': [