Handle config changes for MSE on android

media/base/android/media_source_player.cc

Index: media/base/android/media_source_player.cc
diff --git a/media/base/android/media_source_player.cc b/media/base/android/media_source_player.cc
index a305ca88a06b2fe23501dbfd72b8ce74cfe715fd..dac68f3e5d4e29c77339c8d1d06da4ccd83a81b2 100644
--- a/media/base/android/media_source_player.cc
+++ b/media/base/android/media_source_player.cc
@@ -55,7 +55,8 @@ MediaDecoderJob::MediaDecoderJob(base::Thread* thread, bool is_audio)
       thread_(thread),
       needs_flush_(false),
       is_audio_(is_audio),
-      weak_this_(this) {
+      weak_this_(this),
+      decoding_(false) {
 }
 
 MediaDecoderJob::~MediaDecoderJob() {}
@@ -85,6 +86,7 @@ void MediaDecoderJob::Decode(
     const base::Time& start_wallclock_time,
     const base::TimeDelta& start_presentation_timestamp,
     const MediaDecoderJob::DecoderCallback& callback) {
+  decoding_ = true;

[acolwell GONE FROM CHROMIUM, 2013/06/04 15:19:14]

Should there be a DCHECK(!decoding_) here?

[qinmin, 2013/06/04 19:03:23]

Done.

   thread_->message_loop()->PostTask(FROM_HERE, base::Bind(
       &MediaDecoderJob::DecodeInternal, base::Unretained(this), unit,
       start_wallclock_time, start_presentation_timestamp, needs_flush_,
@@ -103,6 +105,12 @@ void MediaDecoderJob::DecodeInternal(
   base::TimeDelta timeout = base::TimeDelta::FromMicroseconds(
       kMediaCodecTimeoutInMicroseconds);
   int input_buf_index = media_codec_bridge_->DequeueInputBuffer(timeout);
+  if (input_buf_index == MediaCodecBridge::INFO_MEDIA_CODEC_ERROR) {
+    message_loop_->PostTask(FROM_HERE, base::Bind(
+        callback, false, start_presentation_timestamp, start_wallclock_time,
+        false));
+    return;
+  }
   // TODO(qinmin): skip frames if video is falling far behind.
   if (input_buf_index >= 0) {
     if (unit.end_of_stream) {
@@ -179,18 +187,20 @@ void MediaDecoderJob::ReleaseOutputBuffer(
       end_of_stream));
 }
 
+void MediaDecoderJob::OnDecodeCompleted() {
+  decoding_ = false;
+}
+
 void MediaDecoderJob::Flush() {
   // Do nothing, flush when the next Decode() happens.
   needs_flush_ = true;
 }
 
 void MediaDecoderJob::Release() {
-  if (thread_->IsRunning() &&
-      thread_->message_loop() != base::MessageLoop::current()) {
+  if (decoding_ && thread_->message_loop() != base::MessageLoop::current())

[acolwell GONE FROM CHROMIUM, 2013/06/04 15:19:14]

This doesn't seem right. Shouldn't this code look something more like the
following?

if (decoding_ || thread_->message_loop() != base::MessageLoop::current()) {
  thread_->message_loop()->PostTask(FROM_HERE,
base::Bind(&MediaDecoderJob::Release, base::Unretained(this));
  return;
}

delete this;

This makes sure we are always done decoding and are on the proper thread before
deleting.

[qinmin, 2013/06/04 19:03:23]

Backgroud info: there is a problem in android that a java surface cannot be
assigned to 2 MediaCodecs at the same time, or otherwise that will cause an
exception.

If |decoding_| is false, there is nothing running on the decoder thread. So it
is safe to delete the MediaDecoderJob on the main thread. However, if we post a
task to the decoder thread to delete object, then we cannot immediately pass the
surface to a new MediaDecoderJob. So sth like "video_decoder_job_.reset(new
VideoDecoderJob())" will cause the app to crash. Basically we are blocked on the
main thread until the previous decoder got deleted. I can do a
PostTaskAndReply（）， but that will introduce longer delays when config change
happens. And it further complicates the logic in MediaSourcePlayer. Added a TODO
here to mention the bug and see if we can find a better way to handle this.

On 2013/06/04 15:19:14, acolwell wrote:

     thread_->message_loop()->DeleteSoon(FROM_HERE, this);
-  } else {
+  else
     delete this;
-  }
 }
 
 VideoDecoderJob::VideoDecoderJob(
@@ -220,6 +230,7 @@ MediaSourcePlayer::MediaSourcePlayer(
     : MediaPlayerAndroid(player_id, manager),
       pending_event_(NO_EVENT_PENDING),
       active_decoding_tasks_(0),
+      seek_request_id_(0),
       width_(0),
       height_(0),
       audio_codec_(kUnknownAudioCodec),
@@ -234,7 +245,6 @@ MediaSourcePlayer::MediaSourcePlayer(
       video_access_unit_index_(0),
       waiting_for_audio_data_(false),
       waiting_for_video_data_(false),
-      use_empty_surface_(true),
       weak_this_(this) {
 }
 
@@ -243,54 +253,19 @@ MediaSourcePlayer::~MediaSourcePlayer() {
 }
 
 void MediaSourcePlayer::SetVideoSurface(gfx::ScopedJavaSurface surface) {
-  use_empty_surface_ =  surface.IsSurfaceEmpty();
-
-  // If we haven't processed a surface change event, do so now.
-  if (active_decoding_tasks_ > 0) {
-    pending_event_ |= SURFACE_CHANGE_EVENT_PENDING;
-    // Request a seek so that the next decoder will decode an I-frame first.
-    // Or otherwise, MediaCodec might crash. See b/8950387.
-    pending_event_ |= SEEK_EVENT_PENDING;
-    ProcessPendingEvents();
-    return;
-  }
-
-  if (HasVideo()) {
-    video_decoder_job_.reset(new VideoDecoderJob(
-        video_codec_, gfx::Size(width_, height_), surface.j_surface().obj()));
-  }
-
-  // Inform the fullscreen view the player is ready.
-  // TODO(qinmin): refactor MediaPlayerBridge so that we have a better way
-  // to inform ContentVideoView.
-  OnMediaMetadataChanged(duration_, width_, height_, true);
+  surface_ =  surface.Pass();
 
-  if (pending_event_ & SURFACE_CHANGE_EVENT_PENDING) {
-    // We should already requested a seek in this case.
-    pending_event_ &= ~SURFACE_CHANGE_EVENT_PENDING;
-  } else {
-    // Perform a seek so the new decoder can get the I-frame first.
-    pending_event_ |= SEEK_EVENT_PENDING;
-    ProcessPendingEvents();
-    return;
-  }
-
-  if (playing_)
-    StartInternal();
+  pending_event_ |= SURFACE_CHANGE_EVENT_PENDING;
+  // Request a seek so that the next decoder will decode an I-frame first.
+  // Or otherwise, MediaCodec might crash. See b/8950387.

[acolwell GONE FROM CHROMIUM, 2013/06/04 15:19:14]

The seek is needed because you have to create a new decoder for the new surface
right? If so, you should probably updated the comment to indicate something
along these lines because it isn't obvious here that the new surface triggers a
new decoder.

[qinmin, 2013/06/04 19:03:23]

Done.

+  pending_event_ |= SEEK_EVENT_PENDING;
+  ProcessPendingEvents();
 }
 
 void MediaSourcePlayer::Start() {
   playing_ = true;
-  if (HasAudio() && !audio_decoder_job_) {
-    audio_decoder_job_.reset(new AudioDecoderJob(
-        audio_codec_, sampling_rate_, num_channels_,
-        &audio_extra_data_[0], audio_extra_data_.size()));
-  }
 
-  if (HasVideo() && !video_decoder_job_) {
-    // StartInternal() will be delayed until SetVideoSurface() gets called.
-    return;
-  }
+  CreateMediaDecoderJobs(false);
 
   StartInternal();
 }
@@ -333,6 +308,7 @@ void MediaSourcePlayer::Release() {
   active_decoding_tasks_ = 0;
   playing_ = false;
   pending_event_ = NO_EVENT_PENDING;
+  surface_ = gfx::ScopedJavaSurface();
   ReleaseMediaResourcesFromManager();
 }
 
@@ -360,6 +336,12 @@ void MediaSourcePlayer::StartInternal() {
   if (active_decoding_tasks_ > 0 || pending_event_ != NO_EVENT_PENDING)
     return;
 
+  // If one of the decoder job is not ready, do nothing.
+  if ((HasAudio() && !audio_decoder_job_) ||
+      (HasVideo() && !video_decoder_job_)) {
+    return;
+  }
+
   if (HasAudio()) {
     audio_finished_ = false;
     DecodeMoreAudio();
@@ -383,6 +365,15 @@ void MediaSourcePlayer::DemuxerReady(
   video_codec_ = params.video_codec;
   audio_extra_data_ = params.audio_extra_data;
   OnMediaMetadataChanged(duration_, width_, height_, true);
+  if (pending_event_ & CONFIG_CHANGE_EVENT_PENDING) {
+    CreateMediaDecoderJobs(true);
+    pending_event_ &= ~CONFIG_CHANGE_EVENT_PENDING;
+    // If there is a pending surface change, we can merge it with the config
+    // change.
+    pending_event_ &= ~SURFACE_CHANGE_EVENT_PENDING;
+    if (playing_ && pending_event_ == NO_EVENT_PENDING)
+      StartInternal();
+  }
 }
 
 void MediaSourcePlayer::ReadFromDemuxerAck(
@@ -410,11 +401,13 @@ void MediaSourcePlayer::ReadFromDemuxerAck(
   }
 }
 
-void MediaSourcePlayer::OnSeekRequestAck() {
+void MediaSourcePlayer::OnSeekRequestAck(unsigned request_id) {

[acolwell GONE FROM CHROMIUM, 2013/06/04 15:19:14]

nit:s/request/seek_request/

[qinmin, 2013/06/04 19:03:23]

Done.

+  // Do nothing until the most recent seek request is processed.
+  if (seek_request_id_ != request_id)
+    return;
   pending_event_ &= ~SEEK_EVENT_PENDING;
   OnSeekComplete();
-  if (playing_)
-    StartInternal();
+  ProcessPendingEvents();
 }
 
 void MediaSourcePlayer::UpdateTimestamps(
@@ -429,20 +422,30 @@ void MediaSourcePlayer::UpdateTimestamps(
 }
 
 void MediaSourcePlayer::ProcessPendingEvents() {
-  // Wait for all the decoding jobs to finish before sending a seek request.
+  // Wait for all the decoding jobs to finish before processing pending tasks.
   if (active_decoding_tasks_ > 0)
     return;
 
-  DCHECK(pending_event_ != NO_EVENT_PENDING);
-  if (use_empty_surface_ && (pending_event_ & SURFACE_CHANGE_EVENT_PENDING)) {
-    video_decoder_job_.reset();
+  if (pending_event_ & SEEK_EVENT_PENDING) {
+    ClearDecodingData();
+    manager()->OnMediaSeekRequest(
+        player_id(), last_presentation_timestamp_, ++seek_request_id_);
+    return;
+  }
+
+  start_wallclock_time_ = base::Time();
+  if (pending_event_ & CONFIG_CHANGE_EVENT_PENDING) {
+    manager()->OnMediaConfigRequest(player_id());
+    return;
+  }
+
+  if (pending_event_ & SURFACE_CHANGE_EVENT_PENDING) {
+    CreateMediaDecoderJobs(false);
     pending_event_ &= ~SURFACE_CHANGE_EVENT_PENDING;
   }
 
-  ClearDecodingData();
-  manager()->OnMediaSeekRequest(player_id(),
-                                last_presentation_timestamp_,
-                                pending_event_ & SURFACE_CHANGE_EVENT_PENDING);
+  if (playing_ && pending_event_ == NO_EVENT_PENDING)
+    StartInternal();
 }
 
 void MediaSourcePlayer::MediaDecoderCallback(
@@ -452,6 +455,11 @@ void MediaSourcePlayer::MediaDecoderCallback(
   if (active_decoding_tasks_ > 0)
     active_decoding_tasks_--;
 
+  if (is_audio && audio_decoder_job_)
+    audio_decoder_job_->OnDecodeCompleted();
+  if (!is_audio && video_decoder_job_)
+    video_decoder_job_->OnDecodeCompleted();
+
   if (!decode_succeeded) {
     Release();
     OnMediaError(MEDIA_ERROR_DECODE);
@@ -491,6 +499,16 @@ void MediaSourcePlayer::DecodeMoreAudio() {
     return;
   }
 
+  if (DemuxerStream::kConfigChanged ==
+      received_audio_.access_units[audio_access_unit_index_].status) {
+    // Wait for demuxer ready message.
+    pending_event_ |= CONFIG_CHANGE_EVENT_PENDING;
+    received_audio_ = MediaPlayerHostMsg_ReadFromDemuxerAck_Params();
+    audio_access_unit_index_ = 0;
+    ProcessPendingEvents();
+    return;
+  }
+
   audio_decoder_job_->Decode(
       received_audio_.access_units[audio_access_unit_index_],
       start_wallclock_time_, start_presentation_timestamp_,
@@ -511,6 +529,16 @@ void MediaSourcePlayer::DecodeMoreVideo() {
     return;
   }
 
+  if (DemuxerStream::kConfigChanged ==
+      received_video_.access_units[video_access_unit_index_].status) {
+    // Wait for demuxer ready message.
+    pending_event_ |= CONFIG_CHANGE_EVENT_PENDING;
+    received_video_ = MediaPlayerHostMsg_ReadFromDemuxerAck_Params();
+    video_access_unit_index_ = 0;
+    ProcessPendingEvents();
+    return;
+  }
+
   video_decoder_job_->Decode(
       received_video_.access_units[video_access_unit_index_],
       start_wallclock_time_, start_presentation_timestamp_,
@@ -554,4 +582,29 @@ bool MediaSourcePlayer::HasAudio() {
   return kUnknownAudioCodec != audio_codec_;
 }
 
+void MediaSourcePlayer::CreateMediaDecoderJobs(bool config_change) {
+  DCHECK_EQ(0, active_decoding_tasks_);
+
+  // Create audio decoder job only if config changes.
+  if (HasAudio() && (config_change || !audio_decoder_job_)) {

[acolwell GONE FROM CHROMIUM, 2013/06/04 15:19:14]

I don't think this is quite right. Audio & Video config changes are not
necessarily always aligned. They can happen at different times. It looks like
this code resets both decoders here if it sees a config change from either
DemuxerStream. It should only reset the decoder for the stream the signalled the
change.

[qinmin, 2013/06/04 19:03:23]

added 2 new variable, |reconfig_audio_decoder_| and |reconfig_video_decoder_| to
indicate whether we need to recreate the audio/video decoder.

On 2013/06/04 15:19:14, acolwell wrote:

+    audio_decoder_job_.reset(new AudioDecoderJob(
+        audio_codec_, sampling_rate_, num_channels_,
+        &audio_extra_data_[0], audio_extra_data_.size()));
+  }
+
+  video_decoder_job_.reset();
+  if (!HasVideo() || surface_.IsSurfaceEmpty())
+    return;
+
+  // For video, the java surface might have changed even if config does not
+  // change. So always recreate the video decoder job.
+  video_decoder_job_.reset(new VideoDecoderJob(
+      video_codec_, gfx::Size(width_, height_), surface_.j_surface().obj()));
+
+  // Inform the fullscreen view the player is ready.
+  // TODO(qinmin): refactor MediaPlayerBridge so that we have a better way
+  // to inform ContentVideoView.
+  OnMediaMetadataChanged(duration_, width_, height_, true);
+}
+
 }  // namespace media