Implementation of MediaCodecPlayer stage 1

media/base/android/media_codec_player.cc

 // Copyright 2015 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 "media/base/android/media_codec_player.h"
 
+#include "base/barrier_closure.h"
 #include "base/bind.h"
 #include "base/lazy_instance.h"
 #include "base/logging.h"
+#include "base/threading/thread.h"
+

[wolenetz, 2015/05/19 23:05:23]

nit: drop extra line

[Tima Vaisburd, 2015/05/22 22:48:55]

Done.

+#include "media/base/android/media_codec_audio_decoder.h"
+#include "media/base/android/media_codec_video_decoder.h"
+#include "media/base/android/media_player_manager.h"
+#include "media/base/buffers.h"
 
 #define RUN_ON_MEDIA_THREAD(METHOD, ...)                                      \
   do {                                                                        \
     if (!GetMediaTaskRunner()->BelongsToCurrentThread()) {                    \
       GetMediaTaskRunner()->PostTask(                                         \
           FROM_HERE,                                                          \
           base::Bind(&MediaCodecPlayer:: METHOD, weak_this_, ##__VA_ARGS__)); \
       return;                                                                 \
     }                                                                         \
   } while(0)
@@ skipping 10 matching lines @@
 
 // Create media thread
 base::LazyInstance<MediaThread>::Leaky
     g_media_thread = LAZY_INSTANCE_INITIALIZER;
 
 
 scoped_refptr<base::SingleThreadTaskRunner> GetMediaTaskRunner() {
   return g_media_thread.Pointer()->task_runner();
 }
 
+

[qinmin, 2015/05/19 19:26:25]

nit: remove the extra line.

[Tima Vaisburd, 2015/05/20 05:19:58]

Done.

 // MediaCodecPlayer implementation.
 
+/*

[wolenetz, 2015/05/19 23:05:23]

nit: please move the state machine diagram to the .h
Also, it would be good to identify at least placeholders for seek, error,
completion, destruction, release/request resources, and release/request audio
focus (and any others that have slipped my mind). I suspect doing this sooner
rather than later will help flush out design/implementation issues.

See also media/base/pipeline.h for an example of how much of this is done in the
desktop Chromium video stack.

Also, please add tests. While the number of tests of MediaSourcePlayer is quite
high, I suspect that reviewing them and implementing the necessary coverage in
tests of MediaCodecPlayer will help identify simplifications earlier and prevent
repeating some of the same mistakes :)

+                           State machine diagram.
+
+
+       --------------------------> Paused
+       |  StopDone:
+       |                            ^  |
+       |                            |  |
+       |                    Pause:  |  |  Start: dec.Prefetch
+       |                            |  |
+       |                            |  |
+       |                            |  |
+       |                            |  v          PrefetchDone,
+       |                                           no surface:
+       |   ------------------>   Prefetching   -------------->     Waiting
+       |   |                                                     for surface
+       |   |                          |                             /
+       |   |                          |                            /
+       |   | StopDone w/              |                           /
+       |   | pending start:           | PrefetchDone:            /
+       |   |    dec.Prefetch          |   dec.Start             /
+       |   |                          |                        / SetSurface:
+       |   |                          |                       /     dec.Start
+       |   |                          |                      /
+       |   |                          v                     /
+       |   |                                               /
+       |   |                        Playing    <----------/
+       |   |
+       |   |                          |
+       |   |                          |
+       |   |                          | Pause: dec.RequestToStop
+       |   |                          |
+       |   |                          |
+       |   |                          v
+       |   |
+       --------------------------  Stopping
+
+ */
+
 MediaCodecPlayer::MediaCodecPlayer(
     int player_id,
-    MediaPlayerManager* manager,
+    base::WeakPtr<MediaPlayerManager> manager,
     const RequestMediaResourcesCB& request_media_resources_cb,
     scoped_ptr<DemuxerAndroid> demuxer,
     const GURL& frame_url)
     : MediaPlayerAndroid(player_id,
-                         manager,
+                         manager.get(),
                          request_media_resources_cb,
                          frame_url),
       ui_task_runner_(base::MessageLoopProxy::current()),
       demuxer_(demuxer.Pass()),
+      state_(STATE_PAUSED),
+      interpolator_(&default_tick_clock_),
+      pending_start_(false),
       weak_factory_(this) {
   // UI thread
   DCHECK(ui_task_runner_->BelongsToCurrentThread());
 
   DVLOG(1) << "MediaCodecPlayer::MediaCodecPlayer: player_id:" << player_id;
 
+  request_resources_cb_ = base::Bind(request_media_resources_cb_, player_id);
+
+  metadata_changed_cb_ = base::Bind(
+      &MediaPlayerManager::OnMediaMetadataChanged, manager, player_id);
+  time_update_cb_ = base::Bind(
+      &MediaPlayerManager::OnTimeUpdate, manager, player_id);
+  completion_cb_ = base::Bind(
+      &MediaPlayerManager::OnPlaybackComplete, manager, player_id);
+  attach_listener_cb_ = base::Bind(
+      &MediaPlayerAndroid::AttachListener, WeakPtrForUIThread(), nullptr);
+  detach_listener_cb_ = base::Bind(
+      &MediaPlayerAndroid::DetachListener, WeakPtrForUIThread());
+
   weak_this_ = weak_factory_.GetWeakPtr();
 
   // Finish initializaton on Media thread
   GetMediaTaskRunner()->PostTask(
       FROM_HERE, base::Bind(&MediaCodecPlayer::Initialize, weak_this_));
 }
 
 MediaCodecPlayer::~MediaCodecPlayer()
 {
   // Media thread
   DVLOG(1) << "MediaCodecPlayer::~MediaCodecPlayer";
   DCHECK(GetMediaTaskRunner()->BelongsToCurrentThread());
+
+  // Stop decoder threads before destroying decoders
+  ReleaseDecoderResources();
 }
 
 void MediaCodecPlayer::Initialize() {
   // Media thread
   DVLOG(1) << __FUNCTION__;
   DCHECK(GetMediaTaskRunner()->BelongsToCurrentThread());
 
   demuxer_->Initialize(this);
+
+  interpolator_.SetUpperBound(base::TimeDelta());

[qinmin, 2015/05/19 19:26:25]

Can you call this without calling startInterpolating?
There is a DCHECK(upper_bound != kNoTimestamp()) inside
TimeDeltaInterpolator::SetUpperBound()

+
+  CreateDecoders();
 }
 
-// MediaPlayerAndroid implementation.
+// The implementation of MediaPlayerAndroid interface.
 
 void MediaCodecPlayer::DeleteOnCorrectThread() {
   // UI thread
   DVLOG(1) << __FUNCTION__;
   DCHECK(ui_task_runner_->BelongsToCurrentThread());
 
-  // The listener-related portion of the base class has to be
-  // destroyed on UI thread.
+  DetachListener();
+
+  // The base class part that deals with MediaPlayerListener
+  // has to be destroyed on UI thread.
   DestroyListenerOnUIThread();
 
   // Post deletion onto Media thread
   GetMediaTaskRunner()->DeleteSoon(FROM_HERE, this);
 }
 
 void MediaCodecPlayer::SetVideoSurface(gfx::ScopedJavaSurface surface) {
   RUN_ON_MEDIA_THREAD(SetVideoSurface, base::Passed(&surface));
 
   // Media thread
-  DVLOG(1) << __FUNCTION__;
+  DVLOG(1) << __FUNCTION__ << (surface.IsEmpty() ? " empty" : " non-empty");
 
-  NOTIMPLEMENTED();
+  // I assume that if video decoder already has the surface,
+  // there will be two calls:
+  // (1) SetVideoSurface(0)
+  // (2) SetVideoSurface(new_surface)
+  video_decoder_->SetPendingSurface(surface.Pass());
+
+  if  (video_decoder_->HasPendingSurface() &&

[qinmin, 2015/05/19 19:26:25]

nit: extra space after "if"

[Tima Vaisburd, 2015/05/20 05:19:59]

Done.

+       state_ == STATE_WAITING_FOR_SURFACE) {
+    SetState(STATE_PLAYING);
+    StartPlaybackDecoders();
+  }
 }
 
 void MediaCodecPlayer::Start() {
   RUN_ON_MEDIA_THREAD(Start);
 
   // Media thread
   DVLOG(1) << __FUNCTION__;
 
-  NOTIMPLEMENTED();
+  switch (state_) {
+    case STATE_PAUSED:
+      SetState(STATE_PREFETCHING);
+      StartPrefetchDecoders();
+      break;
+    case STATE_STOPPING:
+      SetPendingStart(true);
+      break;
+    default:
+      // Ignore
+      break;
+  }
 }
 
 void MediaCodecPlayer::Pause(bool is_media_related_action) {
   RUN_ON_MEDIA_THREAD(Pause, is_media_related_action);
 
   // Media thread
   DVLOG(1) << __FUNCTION__;
 
-  NOTIMPLEMENTED();
+  switch (state_) {
+    case STATE_PREFETCHING:
+      SetState(STATE_PAUSED);
+      StopDecoders();
+      break;
+    case STATE_WAITING_FOR_CONFIG:
+      SetState(STATE_PAUSED);
+      break;
+    case STATE_WAITING_FOR_SURFACE:
+      SetState(STATE_PAUSED);
+      StopDecoders();
+      break;
+    case STATE_PLAYING:
+      SetState(STATE_STOPPING);
+      RequestToStopDecoders();
+      break;
+    default:
+      // Ignore
+      break;
+  }
 }
 
 void MediaCodecPlayer::SeekTo(base::TimeDelta timestamp) {
   RUN_ON_MEDIA_THREAD(SeekTo, timestamp);
 
   // Media thread
   DVLOG(1) << __FUNCTION__ << " " << timestamp;
-
   NOTIMPLEMENTED();
 }
 
 void MediaCodecPlayer::Release() {
   RUN_ON_MEDIA_THREAD(Release);
 
   // Media thread
   DVLOG(1) << __FUNCTION__;
 
-  NOTIMPLEMENTED();
+  SetState(STATE_PAUSED);
+  ReleaseDecoderResources();
 }
 
 void MediaCodecPlayer::SetVolume(double volume) {
   RUN_ON_MEDIA_THREAD(SetVolume, volume);
 
   // Media thread
   DVLOG(1) << __FUNCTION__ << " " << volume;
-
-  NOTIMPLEMENTED();
+  audio_decoder_->SetVolume(volume);
 }
 
 int MediaCodecPlayer::GetVideoWidth() {
-  // UI thread
-  DCHECK(ui_task_runner_->BelongsToCurrentThread());
-
-  NOTIMPLEMENTED();
-  return 320;
+  // UI thread, Media thread

[qinmin, 2015/05/19 19:26:25]

It is bad that these calls can span multiple threads. we should cache the values
on UI thread and post tasks from media thread to update the cached values,
instead of using clocks.

OnMediaMetadataChanged() is a good place for the UI thread to cache values.

[Tima Vaisburd, 2015/05/20 05:19:58]

I did the caching but I'm not sure I followed the last sentence. The cache is
inside MediaCodecPlayer. Please take a look.

+  return video_decoder_->GetVideoWidth();
 }
 
 int MediaCodecPlayer::GetVideoHeight() {
-  // UI thread
-  DCHECK(ui_task_runner_->BelongsToCurrentThread());
-
-  NOTIMPLEMENTED();
-  return 240;
+  // UI thread, Media thread

[qinmin, 2015/05/19 19:26:26]

ditto

[Tima Vaisburd, 2015/05/20 05:19:59]

Same as above.

+  return video_decoder_->GetVideoHeight();
 }
 
 base::TimeDelta MediaCodecPlayer::GetCurrentTime() {
   // UI thread, Media thread
-  NOTIMPLEMENTED();
-  return base::TimeDelta();
+  base::TimeDelta interpolator_result;
+  {
+    base::AutoLock lock(interpolator_lock_);
+    interpolator_result = interpolator_.GetInterpolatedTime();
+  }
+  return std::min(interpolator_result, duration_);
 }
 
 base::TimeDelta MediaCodecPlayer::GetDuration() {
   // UI thread
   DCHECK(ui_task_runner_->BelongsToCurrentThread());
-
-  NOTIMPLEMENTED();
-  return base::TimeDelta();
+  return duration_;
 }
 
 bool MediaCodecPlayer::IsPlaying() {
   // UI thread
   DCHECK(ui_task_runner_->BelongsToCurrentThread());
-  NOTIMPLEMENTED();
-  return false;
+  return state_ == STATE_PLAYING;
 }
 
 bool MediaCodecPlayer::CanPause() {
   // UI thread
   DCHECK(ui_task_runner_->BelongsToCurrentThread());
   NOTIMPLEMENTED();
   return false;
 }
 
 bool MediaCodecPlayer::CanSeekForward() {
   // UI thread
   DCHECK(ui_task_runner_->BelongsToCurrentThread());
   NOTIMPLEMENTED();
   return false;
 }
 
 bool MediaCodecPlayer::CanSeekBackward() {
   // UI thread
   DCHECK(ui_task_runner_->BelongsToCurrentThread());
   NOTIMPLEMENTED();
   return false;
 }
 
 bool MediaCodecPlayer::IsPlayerReady() {
   // UI thread
   DCHECK(ui_task_runner_->BelongsToCurrentThread());
-  NOTIMPLEMENTED();
+  // This method is called to check whether it' save to release
+  // this player if the OS needs more resources.
+  // For this player Release() can be called at any time.
   return true;
 }
 
 void MediaCodecPlayer::SetCdm(BrowserCdm* cdm) {
   // UI thread
   DCHECK(ui_task_runner_->BelongsToCurrentThread());
   NOTIMPLEMENTED();
 }
 
 // Callbacks from Demuxer.
 
 void MediaCodecPlayer::OnDemuxerConfigsAvailable(
     const DemuxerConfigs& configs) {
   // Media thread
   DCHECK(GetMediaTaskRunner()->BelongsToCurrentThread());
 
-  NOTIMPLEMENTED();
+  duration_ = configs.duration;
+
+  SetDemuxerConfigs(configs);
+
+  switch (state_) {
+    case STATE_WAITING_FOR_CONFIG:
+      if (HasVideo() && !HasPendingSurface()) {
+        SetState(STATE_WAITING_FOR_SURFACE);
+      } else {
+        SetState(STATE_PLAYING);

[qinmin, 2015/05/19 19:26:25]

No idea why the player has to start playing here. OnDemuxerConfigsAvailable()
can be called when player is paused, no?

[Tima Vaisburd, 2015/05/20 05:19:58]

But below you confirmed that this state is impossible in the first place (no
configuration after prefetching) so I removed it here.
Later, though, I had to recreate it under the same name, but with a completely
different meaning: we need to wait for initial configs if we do not have them as
Start().

[qinmin, 2015/05/20 15:39:25]

Prefetching and OnDemuxerConfigsAvailable are 2 different things.
OnDemuxerConfigsAvailable() is called when DemuxerReady IPC is received, that
happens when chunk demuxer finishes initializing itself. Why the player should
enter a playing state in that case?

[Tima Vaisburd, 2015/05/20 17:34:56]

Please take a look at PatchSet 9 where I changed this. There, we might wait
_for_ OnDemuxerConfigsAvailable to continue, more precisely, to start
prefetching.

+        StartPlaybackDecoders();
+      }
+      break;
+    default:
+      // Do nothing
+      break;
+  }
+
+  // Post on UI thread
+  ui_task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(metadata_changed_cb_, duration_,
+                 GetVideoWidth(), GetVideoHeight(), true));
 }
 
 void MediaCodecPlayer::OnDemuxerDataAvailable(const DemuxerData& data) {
   // Media thread
   DCHECK(GetMediaTaskRunner()->BelongsToCurrentThread());
-  NOTIMPLEMENTED();
+
+  DCHECK_LT(0u, data.access_units.size());
+  CHECK_GE(1u, data.demuxer_configs.size());
+
+  DVLOG(2) << "Player::" << __FUNCTION__;
+
+  if (data.type == DemuxerStream::AUDIO)
+    audio_decoder_->OnDemuxerDataAvailable(data);
+
+  if (data.type == DemuxerStream::VIDEO)
+    video_decoder_->OnDemuxerDataAvailable(data);
 }
 
 void MediaCodecPlayer::OnDemuxerSeekDone(
     base::TimeDelta actual_browser_seek_time) {
   // Media thread
   DCHECK(GetMediaTaskRunner()->BelongsToCurrentThread());
+
+  DVLOG(1) << __FUNCTION__ << " actual_time:" << actual_browser_seek_time;
+
   NOTIMPLEMENTED();
 }
 
 void MediaCodecPlayer::OnDemuxerDurationChanged(
     base::TimeDelta duration) {
   // Media thread
   DCHECK(GetMediaTaskRunner()->BelongsToCurrentThread());
-  NOTIMPLEMENTED();
+  DVLOG(1) << __FUNCTION__ << " duration:" << duration;
+  duration_ = duration;
+}
+

[qinmin, 2015/05/19 19:26:25]

nit: extra line

[Tima Vaisburd, 2015/05/20 05:19:58]

Done.

+
+// Events from Decoders, called on Media thread
+
+void MediaCodecPlayer::RequestDemuxerData(DemuxerStream::Type stream_type) {
+  DVLOG(1) << __FUNCTION__ << " streamType:" << stream_type;
+
+  // Use this method instead of directly binding with
+  // DemuxerAndroid::RequestDemuxerData() to avoid the  race condition

[wolenetz, 2015/05/19 23:05:23]

nit: remove extra space after 'the'

[Tima Vaisburd, 2015/05/22 22:48:55]

Done.

+  // on deletion:
+  // 1. DestroySelf is posted from UI to Media thread.

[wolenetz, 2015/05/19 23:05:23]

What is DestroySelf? It's not in existence in this class. Maybe this is the
cause of my confusion, but I'm also not sure how this comment clarifies what the
bad versus good usage would be. Am I missing somehow implicitly that the bad way
doesn't use a weak_ptr, but the good way does?

[Tima Vaisburd, 2015/05/22 22:48:54]

Yes, exactly. Fixed and reformulated slightly.

+  // 2. RequestDemuxerData callback is posted from Decoder to Media thread.
+  // 3. DestroySelf arrives, we delete the player and detach from
+  //    BrowserDemuxerAndroid.
+  // 4. RequestDemuxerData is sent to the player, but weak_ptr blocks it.
+  //    If we used DemuxerAndroid::RequestDemuxerData() it would arrive,
+  //    but the client (i.e. this player) would not exist.
+  demuxer_->RequestDemuxerData(stream_type);
+}
+
+void MediaCodecPlayer::OnPrefetchDone() {
+  // Media thread

[wolenetz, 2015/05/19 23:05:23]

Here, and elsewhere, a DCHECK ( I am on the right thread ) is sufficient to
fully replace a comment that says "// the right thread".

[Tima Vaisburd, 2015/05/22 22:48:55]

I found it easier to grasp a short comment written in plain English (and
highlighted with a different color) than the long DCHECK statement where the
piece that conveys information ('UI' or 'Media') is buried. I can see that it
might feel redundant, though. Do you feel I'd rather remove the comments?

+  DCHECK(GetMediaTaskRunner()->BelongsToCurrentThread());
+  DVLOG(1) << __FUNCTION__;
+
+  if (state_ != STATE_PREFETCHING)

[wolenetz, 2015/05/19 23:05:23]

When might this happen with this new player/decoders?

[Tima Vaisburd, 2015/05/22 22:48:54]

Release() may be called on Media thread before we are done prefetching, for
instance.

+    return; // Ignore
+
+  if (!HasAudio() && !HasVideo()) {
+    // No configuration at all, let's wait for it.

[qinmin, 2015/05/19 19:26:25]

This shouldn't happen. I would just put NOT_REACHED() here

[Tima Vaisburd, 2015/05/20 05:19:59]

I raised an error

+    // Although it probably should never happen here,
+    // after the data has come from demuxer.
+    SetState(STATE_WAITING_FOR_CONFIG);
+    return;
+  }
+
+  if (HasVideo() && !HasPendingSurface()) {
+    SetState(STATE_WAITING_FOR_SURFACE);

[wolenetz, 2015/05/19 23:05:23]

I'm not sure how this happens given the current code in
OnDemuxerConfigsAvailable. Could the surface be removed after that time? Where
does that happen / get handled in this player version?

[Tima Vaisburd, 2015/05/22 22:48:54]

Yes, the surface can be removed at any time, but changing the surface is not
implemented in this version at all. The intention here is to wait for the very
first video surface. OnDemuxerConfigsAvailable comes from the renderer process,
while the surface comes from Java in browser process. These events are not
synchronized.

+    return;
+  }
+
+  SetState(STATE_PLAYING);
+  StartPlaybackDecoders();
+}
+
+void MediaCodecPlayer::OnStarvation() {
+  // Media thread
+  DCHECK(GetMediaTaskRunner()->BelongsToCurrentThread());
+  DVLOG(1) << __FUNCTION__;
+
+  if (state_ != STATE_PLAYING)
+    return; // Ignore
+
+  SetState(STATE_STOPPING);
+  RequestToStopDecoders();

[wolenetz, 2015/05/19 23:05:23]

10,000' view: why does starvation stop decoders?

[Tima Vaisburd, 2015/05/22 22:48:54]

I think we need to stop the other channel... Upon restart we redo A/V sync, the
same way we do it in the MediaSourcePlayer.

+  SetPendingStart(true);
+}
+
+void MediaCodecPlayer::OnStopDone() {
+  // Media thread
+  DCHECK(GetMediaTaskRunner()->BelongsToCurrentThread());
+  DVLOG(1) << __FUNCTION__;
+
+  if (!(audio_decoder_->IsStopped() && video_decoder_->IsStopped()))
+    return; // Wait until other stream is stopped
+
+  // At this point decoder threads should not be running
+  if (interpolator_.interpolating())
+    interpolator_.StopInterpolating();
+
+  switch (state_) {
+    case STATE_STOPPING:
+      if (HasPendingStart()) {
+        SetState(STATE_PREFETCHING);
+        StartPrefetchDecoders();
+      } else {
+        SetState(STATE_PAUSED);
+      }
+      break;
+    case STATE_PLAYING:
+      // Unexpected stop means completion
+      SetState(STATE_PAUSED);
+      break;
+    default:
+      NOTREACHED();
+      break;
+  }
+
+  // DetachListener to UI thread
+  ui_task_runner_->PostTask(FROM_HERE, detach_listener_cb_);
+
+  if (AudioFinished() && VideoFinished())
+    ui_task_runner_->PostTask(FROM_HERE, completion_cb_);
+}
+
+void MediaCodecPlayer::OnError() {
+  // Media thread
+  DCHECK(GetMediaTaskRunner()->BelongsToCurrentThread());
+  DVLOG(1) << __FUNCTION__;
+
+  // STATE_ERROR blocks all events
+  SetState(STATE_ERROR);
+
+  ReleaseDecoderResources();
+}
+
+// Event from Decoders, called on Decoder thread
+
+void MediaCodecPlayer::OnTimeIntervalUpdate(base::TimeDelta now_playing,
+                                            base::TimeDelta last_buffered) {
+  // Decoder thread
+  base::TimeDelta current_time;
+  {
+    base::AutoLock lock(interpolator_lock_);
+    interpolator_.SetBounds(now_playing, last_buffered);
+
+    current_time = std::min(interpolator_.GetInterpolatedTime(), duration_);
+  }
+
+  // Post on UI thread
+  ui_task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(time_update_cb_, current_time, base::TimeTicks::Now()));
+}
+
+// State machine operations, called on Media thread
+
+void MediaCodecPlayer::SetState(PlayerState new_state) {
+  DCHECK(GetMediaTaskRunner()->BelongsToCurrentThread());
+
+  DVLOG(1) << "SetState:" << AsString(state_) << " -> " << AsString(new_state);
+  state_ = new_state;
+}
+
+#undef ENTRY
+#define ENTRY(x) case x: return #x;
+
+const char* MediaCodecPlayer::AsString(PlayerState state) {
+  switch (state) {
+    ENTRY(STATE_PAUSED);
+    ENTRY(STATE_PREFETCHING);
+    ENTRY(STATE_PLAYING);
+    ENTRY(STATE_STOPPING);
+    ENTRY(STATE_WAITING_FOR_CONFIG);
+    ENTRY(STATE_WAITING_FOR_SURFACE);
+    ENTRY(STATE_WAITING_FOR_SEEK);
+    ENTRY(STATE_ERROR);
+    default: return "Unknown PlayerState";
+  }
+}
+#undef ENTRY
+
+void MediaCodecPlayer::SetPendingSurface(gfx::ScopedJavaSurface surface) {
+  DCHECK(GetMediaTaskRunner()->BelongsToCurrentThread());
+  DVLOG(1) << __FUNCTION__;
+
+  video_decoder_->SetPendingSurface(surface.Pass());
+}
+
+bool MediaCodecPlayer::HasPendingSurface() {
+  return video_decoder_->HasPendingSurface();
+}
+
+void MediaCodecPlayer::SetPendingStart(bool need_to_start) {
+  DCHECK(GetMediaTaskRunner()->BelongsToCurrentThread());
+  DVLOG(1) << __FUNCTION__ << ": " << need_to_start;
+  pending_start_ = need_to_start;
+}
+
+bool MediaCodecPlayer::HasPendingStart() {

[qinmin, 2015/05/19 19:26:25]

The function name just don't match what it does.
How about hasPendingStartAndReset() or split this into 2 functions.

[Tima Vaisburd, 2015/05/20 05:19:58]

Right now called two methods since so far HasPendingStart() is called just once.

+  DCHECK(GetMediaTaskRunner()->BelongsToCurrentThread());
+  // Return and reset.
+  bool ret = pending_start_;
+  pending_start_ = false;
+  return ret;
+}
+
+bool MediaCodecPlayer::HasAudio() {
+  DCHECK(GetMediaTaskRunner()->BelongsToCurrentThread());
+  return audio_decoder_->HasStream();
+}
+
+bool MediaCodecPlayer::HasVideo() {
+  DCHECK(GetMediaTaskRunner()->BelongsToCurrentThread());
+  return video_decoder_->HasStream();
+}
+
+void MediaCodecPlayer::CreateDecoders() {
+  // Media thread
+  DCHECK(GetMediaTaskRunner()->BelongsToCurrentThread());
+  DVLOG(1) << __FUNCTION__;
+
+  error_cb_ =  base::Bind(&MediaCodecPlayer::OnError, weak_this_);
+
+  audio_decoder_.reset(new MediaCodecAudioDecoder(
+      GetMediaTaskRunner(),
+      ui_task_runner_,
+      base::Bind(&MediaCodecPlayer::RequestDemuxerData, weak_this_,
+                 DemuxerStream::AUDIO),
+      base::Bind(&MediaCodecPlayer::OnStarvation, weak_this_),
+      base::Bind(&MediaCodecPlayer::OnStopDone, weak_this_),
+      error_cb_,
+      base::Bind(&MediaCodecPlayer::OnTimeIntervalUpdate,
+                 base::Unretained(this))));
+
+  video_decoder_.reset(new MediaCodecVideoDecoder(
+      GetMediaTaskRunner(),
+      ui_task_runner_,
+      base::Bind(&MediaCodecPlayer::RequestDemuxerData, weak_this_,
+                 DemuxerStream::VIDEO),
+      base::Bind(&MediaCodecPlayer::OnStarvation, weak_this_),
+      base::Bind(&MediaCodecPlayer::OnStopDone, weak_this_),
+      error_cb_,
+      request_resources_cb_));

[qinmin, 2015/05/19 19:26:25]

request_resources_cb_ is binded to the UI thread, and now it is passed to the
video_decoder_, which has no idea about the UI message loop. So it will be
called on the wrong message loop.

[Tima Vaisburd, 2015/05/20 05:19:58]

ui_task_runner_ is accessible from VideoDecoder via protected member and is used
there. I also verified that this callback is called on UI thread by printing
tid.

[qinmin, 2015/05/20 15:48:16]

I don't feel it proper for Decoder to know ui_task_runner. They are created on
media thread, and running on decoder thread, they don't need to know about the
ui thread. I think MediaCodecPlayer should have its own requestVideoResources()
call, and pass that function as a callback to the decoder job. And when that
call runs, mediaCodecPlayer will post a task to UI thread to inform
BrowserMediaPlayerManager. 

[Tima Vaisburd, 2015/05/22 23:37:52]

We have two callbacks now, both from the video decoder: requestVideoResources
and videoResolutionChanged. I would think we want to avoid an extra thread
hopping for efficiency, like we did with demuxer tasks. What do you think about
moving the ui_task_runner from Decoder to the VideoDecoder subclass and post the
task to it directly, like now?

[qinmin, 2015/05/26 00:49:19]

No, i don't that's logically sound. And I don't think a 2 hop CB will have
impact on performances as request_resources_cb is not frequently called.

+}
+
+bool MediaCodecPlayer::AudioFinished() {
+  return audio_decoder_->IsCompleted() || !audio_decoder_->HasStream();
+}
+
+bool MediaCodecPlayer::VideoFinished() {
+  return video_decoder_->IsCompleted() || !video_decoder_->HasStream();
+}
+
+void MediaCodecPlayer::SetDemuxerConfigs(const DemuxerConfigs& configs) {
+  DCHECK(GetMediaTaskRunner()->BelongsToCurrentThread());
+  DVLOG(1) << __FUNCTION__;
+
+  if (configs.audio_codec != kUnknownAudioCodec)
+    audio_decoder_->SetDemuxerConfigs(configs);
+
+  if (configs.video_codec != kUnknownVideoCodec)
+    video_decoder_->SetDemuxerConfigs(configs);
+}
+
+void MediaCodecPlayer::StartPrefetchDecoders() {
+  DCHECK(GetMediaTaskRunner()->BelongsToCurrentThread());
+  DVLOG(1) << __FUNCTION__;
+
+  bool do_audio = false;
+  bool do_video = false;
+  int count = 0;
+  if (!AudioFinished()) {
+    do_audio = true;
+    ++count;
+  }
+  if (!VideoFinished()) {
+    do_video = true;
+    ++count;
+  }
+
+  DCHECK_LT(0, count); // at least one decoder should be active
+
+  base::Closure prefetch_cb = base::BarrierClosure(
+      count, base::Bind(&MediaCodecPlayer::OnPrefetchDone, weak_this_));
+
+  if (do_audio)
+    audio_decoder_->Prefetch(prefetch_cb);
+
+  if (do_video)
+    video_decoder_->Prefetch(prefetch_cb);
+}
+
+void MediaCodecPlayer::StartPlaybackDecoders() {
+  DCHECK(GetMediaTaskRunner()->BelongsToCurrentThread());
+  DVLOG(1) << __FUNCTION__;
+
+  // Configure all streams before the start since
+  // we may discover that browser seek is required.
+
+  bool do_audio = !AudioFinished();
+  bool do_video = !VideoFinished();
+
+  // If there is nothing to play, the state machine should determine
+  // this at the prefetch state and never call this method.
+  DCHECK(do_audio || do_video);
+
+  if (do_audio) {
+    MediaCodecDecoder::ConfigStatus status = audio_decoder_->Configure();
+    if (status != MediaCodecDecoder::CONFIG_OK) {
+      GetMediaTaskRunner()->PostTask(FROM_HERE, error_cb_);
+      return;
+    }
+  }
+
+  if (do_video) {
+    MediaCodecDecoder::ConfigStatus status = video_decoder_->Configure();
+    if (status != MediaCodecDecoder::CONFIG_OK) {
+      GetMediaTaskRunner()->PostTask(FROM_HERE, error_cb_);
+      return;
+    }
+  }
+
+  // At this point decoder threads should not be running
+  if (!interpolator_.interpolating())
+    interpolator_.StartInterpolating();
+
+  if (do_audio) {
+    if (!audio_decoder_->Start(GetCurrentTime())) {
+      GetMediaTaskRunner()->PostTask(FROM_HERE, error_cb_);
+      return;
+    }
+
+    // Attach listener on UI thread
+    ui_task_runner_->PostTask(FROM_HERE, attach_listener_cb_);
+  }
+
+  if (do_video) {
+    if (!video_decoder_->Start(GetCurrentTime())) {
+      GetMediaTaskRunner()->PostTask(FROM_HERE, error_cb_);
+      return;
+    }
+  }
+}
+
+void MediaCodecPlayer::StopDecoders() {
+  DCHECK(GetMediaTaskRunner()->BelongsToCurrentThread());
+  DVLOG(1) << __FUNCTION__;
+
+  audio_decoder_->SyncStop();
+  video_decoder_->SyncStop();
+}
+
+void MediaCodecPlayer::RequestToStopDecoders() {
+  DCHECK(GetMediaTaskRunner()->BelongsToCurrentThread());
+  DVLOG(1) << __FUNCTION__;
+
+  bool do_audio = false;
+  bool do_video = false;
+  int count = 0;
+  if (audio_decoder_->IsPrefetchingOrPlaying()) {
+    do_audio = true;
+    ++count;
+  }
+  if (video_decoder_->IsPrefetchingOrPlaying()) {
+    do_video = true;
+    ++count;
+  }
+
+  if (count == 0) {

[qinmin, 2015/05/19 19:26:26]

No barrier closure is used here, why not simply remove count and use
if(!do_audio && !do_video)

[Tima Vaisburd, 2015/05/20 05:19:59]

Yes, it was a silly copy-pasting.

+    GetMediaTaskRunner()->PostTask(
+        FROM_HERE, base::Bind(&MediaCodecPlayer::OnStopDone, weak_this_));
+    return;
+  }
+
+  if (do_audio)
+    audio_decoder_->RequestToStop();
+
+  if (do_video)
+    video_decoder_->RequestToStop();
+}
+
+void MediaCodecPlayer::ReleaseDecoderResources() {
+  DCHECK(GetMediaTaskRunner()->BelongsToCurrentThread());
+  DVLOG(1) << __FUNCTION__;
+
+  audio_decoder_->ReleaseDecoderResources();
+  video_decoder_->ReleaseDecoderResources();
+
+  // At this point decoder threads should not be running
+  if (interpolator_.interpolating())
+    interpolator_.StopInterpolating();
 }
 
 }  // namespace media