Move AudioRenderer out of Filter heirarchy.

media/base/pipeline.cc

Index: media/base/pipeline.cc
diff --git a/media/base/pipeline.cc b/media/base/pipeline.cc
index fd59a130db6fcf3341a845d4ab96952e7513f5cb..54ea1fa2734e0af1b8696975c0b3ed4014139788 100644
--- a/media/base/pipeline.cc
+++ b/media/base/pipeline.cc
@@ -64,7 +64,6 @@ media::PipelineStatus PipelineStatusNotification::status() {
 struct Pipeline::PipelineInitState {

[scherkus (not reviewing), 2012/07/10 03:58:22]

I think we can remove this entirely very soon

   scoped_refptr<AudioDecoder> audio_decoder;
   scoped_refptr<VideoDecoder> video_decoder;
-  scoped_refptr<AudioRenderer> audio_renderer;
   scoped_refptr<VideoRenderer> video_renderer;
   scoped_refptr<CompositeFilter> composite;

[acolwell GONE FROM CHROMIUM, 2012/07/10 16:34:37]

If this is only holding video_renderer now can we just remove composite?

[scherkus (not reviewing), 2012/07/17 03:43:00]

Not yet because we set pipeline_filter_ to composite after init completes

 };
@@ -454,6 +453,75 @@ TimeDelta Pipeline::TimeForByteOffset_Locked(int64 byte_offset) const {
   return time_offset;
 }
 
+void Pipeline::DoPause(const base::Closure& done_cb) {

[scherkus (not reviewing), 2012/07/10 03:58:22]

these DoXXX() methods essentially replicate CompositeFilter's async serial
execution but via closure chaining.

[acolwell GONE FROM CHROMIUM, 2012/07/10 16:34:37]

Wow. This is not as clean as I had hoped. I think having a ClosureListHelper or
something might make this more straightforward and remove some of the duplicate
code. IWBN if you could make the code look something like this.

ClosureList closures;

if (audio_renderer_) {
   closures.Add(base::Bind(&AudioRenderer::Pause, audio_renderer_));

if (pipeline_filter_)
  closures.Add(base::Bind(&Filter::Pause,pipeline_filter_));

closures.Run(done_cb);

To me this seems more readable & execution order corresponds to reading the code
from top to bottom. The existing code is a little confusing because reading top
to bottom actually shows execution in the reverse order.

[scherkus (not reviewing), 2012/07/17 03:43:00]

I played around with this for an hour or so and it looks like you can't have a
callback-to-a-callback.

Specifically, something like this (which is quite nice!) doesn't work:

typedef base::Callback<void(base::Closure)> ClosureCB;
scoped_ptr<std::queue<ClosureCB> > closures(new std::queue<ClosureCB>());
closures->push(base::Bind(&AudioRenderer::Pause, audio_renderer_));  // ***
Compiler can't resolve templates

// later on...

void RunSerially(scoped_ptr<std::queue<ClosureCB> > closures,
                 const base::Closure& done_cb) {
  ClosureCB cb = closures->front();
  closures->pop();

  if (!closures->empty()) {
    cb.Run(base::Bind(&RunSerially, base::Passed(&closures), done_cb));
    return;
  }

  cb.Run(done_cb);
}



I hacked around a bit with a class that would let you add closures to run but
it's a bit kludgy.

ClosureRunner* crunner = new ClosureRunner();
crunner->Add(base::Bind(&AudioRenderer::Pause, audio_renderer_,
crunner->GetAddCB()));  // Yuck.


I'm not an expert here so I'll chat with ajwong@.

+  base::Closure do_pause = done_cb;
+
+  if (pipeline_filter_) {

[scherkus (not reviewing), 2012/07/10 03:58:22]

note that pipeline_filter_ ~= video_renderer_ (it's the only Filter class left!)

[acolwell GONE FROM CHROMIUM, 2012/07/10 16:34:37]

Any reason we shouldn't rename pipeline_filter_ -> video_renderer_ right now to
reflect this fact?

[scherkus (not reviewing), 2012/07/17 03:43:00]

We have an actual video_renderer_ that we use for different purposes -- I think
I'd like to leave the name unchanged for now but add a TODO comment in the .h

+    do_pause = base::Bind(&Pipeline::RunOnPipelineThread, this, do_pause);
+    do_pause = base::Bind(&Filter::Pause, pipeline_filter_, do_pause);
+  }
+
+  if (audio_renderer_) {
+    do_pause = base::Bind(&Pipeline::RunOnPipelineThread, this, do_pause);
+    do_pause = base::Bind(&AudioRenderer::Pause, audio_renderer_, do_pause);
+  }
+
+  do_pause.Run();
+}
+
+void Pipeline::DoFlush(const base::Closure& done_cb) {

[Ami GONE FROM CHROMIUM, 2012/07/10 17:16:00]

Before this CL CompositeFilter::Flush() was parallel, but this one isn't.

[scherkus (not reviewing), 2012/07/17 03:43:00]

I forget why Flush() was parallel. Possibly as a work around for some race
condition, but I'm not sure anymore.

I need to think a bit more on how I want to write this callback chaining code.

+  base::Closure do_flush = done_cb;
+
+  if (pipeline_filter_) {
+    do_flush = base::Bind(&Pipeline::RunOnPipelineThread, this, do_flush);
+    do_flush = base::Bind(&Filter::Flush, pipeline_filter_, do_flush);
+  }
+
+  if (audio_renderer_) {
+    do_flush = base::Bind(&Pipeline::RunOnPipelineThread, this, do_flush);
+    do_flush = base::Bind(&AudioRenderer::Flush, audio_renderer_, do_flush);
+  }
+
+  do_flush.Run();
+}
+
+void Pipeline::DoPlay(const base::Closure& done_cb) {
+  base::Closure do_play = done_cb;
+
+  if (pipeline_filter_) {
+    do_play = base::Bind(&Pipeline::RunOnPipelineThread, this, do_play);
+    do_play = base::Bind(&Filter::Play, pipeline_filter_, do_play);
+  }
+
+  if (audio_renderer_) {
+    do_play = base::Bind(&Pipeline::RunOnPipelineThread, this, do_play);
+    do_play = base::Bind(&AudioRenderer::Play, audio_renderer_, do_play);
+  }
+
+  do_play.Run();
+}
+
+void Pipeline::DoStop(const base::Closure& done_cb) {
+  base::Closure do_stop = done_cb;
+
+  if (pipeline_filter_) {
+    do_stop = base::Bind(&Pipeline::RunOnPipelineThread, this, do_stop);
+    do_stop = base::Bind(&Filter::Stop, pipeline_filter_, do_stop);
+  }
+
+  if (audio_renderer_) {
+    do_stop = base::Bind(&Pipeline::RunOnPipelineThread, this, do_stop);
+    do_stop = base::Bind(&AudioRenderer::Stop, audio_renderer_, do_stop);
+  }
+
+  if (demuxer_) {
+    do_stop = base::Bind(&Pipeline::RunOnPipelineThread, this, do_stop);
+    do_stop = base::Bind(&Demuxer::Stop, demuxer_, do_stop);
+  }
+
+  do_stop.Run();
+}
+
 void Pipeline::AddBufferedByteRange(int64 start, int64 end) {
   DCHECK(IsRunning());
   base::AutoLock auto_lock(lock_);
@@ -485,12 +553,17 @@ void Pipeline::NotifyEnded() {
   media_log_->AddEvent(media_log_->CreateEvent(MediaLogEvent::ENDED));
 }
 
-void Pipeline::DisableAudioRenderer() {
+void Pipeline::AudioRendererEnded() {
   DCHECK(IsRunning());
+  message_loop_->PostTask(FROM_HERE, base::Bind(
+      &Pipeline::NotifyEndedTask, this));
+  media_log_->AddEvent(media_log_->CreateEvent(MediaLogEvent::ENDED));
+}
 
-  // Disable renderer on the message loop.
+void Pipeline::AudioRendererDisabled() {
+  DCHECK(IsRunning());
   message_loop_->PostTask(FROM_HERE, base::Bind(
-      &Pipeline::DisableAudioRendererTask, this));
+      &Pipeline::AudioRendererDisabledTask, this));
   media_log_->AddEvent(
       media_log_->CreateEvent(MediaLogEvent::AUDIO_RENDERER_DISABLED));
 }
@@ -746,10 +819,12 @@ void Pipeline::PlaybackRateChangedTask(float playback_rate) {
   // hasn't completed yet, the playback rate will be set when initialization
   // completes.
   if (pipeline_filter_) {
-    DCHECK(demuxer_);
     demuxer_->SetPlaybackRate(playback_rate);
     pipeline_filter_->SetPlaybackRate(playback_rate);
   }
+
+  if (audio_renderer_)

[acolwell GONE FROM CHROMIUM, 2012/07/10 16:34:37]

This isn't consistent with the original code. It should be inside the
pipeline_filter_ conditional since that is only set once everything has been
initialized.

[scherkus (not reviewing), 2012/07/17 03:43:00]

Done.

+    audio_renderer_->SetPlaybackRate(playback_rate_);
 }
 
 void Pipeline::VolumeChangedTask(float volume) {
@@ -794,8 +869,7 @@ void Pipeline::SeekTask(TimeDelta time, const PipelineStatusCB& seek_cb) {
     if (clock_->IsPlaying())
       clock_->Pause();
   }
-  pipeline_filter_->Pause(
-      base::Bind(&Pipeline::OnFilterStateTransition, this));
+  DoPause(base::Bind(&Pipeline::OnFilterStateTransition, this));
 }
 
 void Pipeline::NotifyEndedTask() {
@@ -835,7 +909,7 @@ void Pipeline::NotifyEndedTask() {
   ReportStatus(ended_cb_, status_);
 }
 
-void Pipeline::DisableAudioRendererTask() {
+void Pipeline::AudioRendererDisabledTask() {
   DCHECK(message_loop_->BelongsToCurrentThread());
 
   base::AutoLock auto_lock(lock_);
@@ -882,16 +956,13 @@ void Pipeline::FilterStateTransitionTask() {
   // Carry out the action for the current state.
   if (TransientState(state_)) {
     if (state_ == kPausing) {
-      pipeline_filter_->Pause(
-          base::Bind(&Pipeline::OnFilterStateTransition, this));
+      DoPause(base::Bind(&Pipeline::OnFilterStateTransition, this));
     } else if (state_ == kFlushing) {
-      pipeline_filter_->Flush(
-          base::Bind(&Pipeline::OnFilterStateTransition, this));
+      DoFlush(base::Bind(&Pipeline::OnFilterStateTransition, this));
     } else if (state_ == kSeeking) {
       DoSeek(seek_timestamp_);
     } else if (state_ == kStarting) {
-      pipeline_filter_->Play(
-          base::Bind(&Pipeline::OnFilterStateTransition, this));
+      DoPlay(base::Bind(&Pipeline::OnFilterStateTransition, this));
     } else if (state_ == kStopping) {
       DoStop(base::Bind(&Pipeline::OnFilterStateTransition, this));
     } else {
@@ -937,8 +1008,7 @@ void Pipeline::TeardownStateTransitionTask() {
       break;
     case kPausing:
       SetState(kFlushing);
-      pipeline_filter_->Flush(
-          base::Bind(&Pipeline::OnTeardownStateTransition, this));
+      DoFlush(base::Bind(&Pipeline::OnTeardownStateTransition, this));
       break;
     case kFlushing:
       SetState(kStopping);
@@ -1086,23 +1156,18 @@ bool Pipeline::InitializeAudioRenderer(
   if (!decoder)
     return false;
 
-  filter_collection_->SelectAudioRenderer(
-      &pipeline_init_state_->audio_renderer);
-  if (!pipeline_init_state_->audio_renderer) {
+  filter_collection_->SelectAudioRenderer(&audio_renderer_);
+  if (!audio_renderer_) {
     SetError(PIPELINE_ERROR_REQUIRED_FILTER_MISSING);
     return false;
   }
 
-  pipeline_init_state_->composite->AddFilter(
-      pipeline_init_state_->audio_renderer);
-
-  pipeline_init_state_->audio_renderer->Initialize(
+  audio_renderer_->SetHost(this);

[scherkus (not reviewing), 2012/07/10 04:02:52]

I think SetHost() can go away after we replace ARH with individual CBs passed
into Initialize() (see TODO by ARH definition)

[acolwell GONE FROM CHROMIUM, 2012/07/10 16:34:37]

I think we should just add the callbacks now. I can't think of any good reason
to introduce the ARH class as an intermediate step.

[scherkus (not reviewing), 2012/07/17 03:43:00]

Done.

+  audio_renderer_->Initialize(
       decoder,
       base::Bind(&Pipeline::OnFilterInitialize, this),
       base::Bind(&Pipeline::OnAudioUnderflow, this),
       base::Bind(&Pipeline::OnAudioTimeUpdate, this));
-
-  audio_renderer_ = pipeline_init_state_->audio_renderer;
   return true;
 }
 
@@ -1188,8 +1253,7 @@ void Pipeline::TearDownPipeline() {
     case kStarted:
     case kEnded:
       SetState(kPausing);
-      pipeline_filter_->Pause(
-          base::Bind(&Pipeline::OnTeardownStateTransition, this));
+      DoPause(base::Bind(&Pipeline::OnTeardownStateTransition, this));
       break;
 
     case kStopping:
@@ -1201,31 +1265,6 @@ void Pipeline::TearDownPipeline() {
   };
 }
 
-void Pipeline::DoStop(const base::Closure& callback) {
-  if (demuxer_) {
-    demuxer_->Stop(base::Bind(
-        &Pipeline::OnDemuxerStopDone, this, callback));
-    return;
-  }
-
-  OnDemuxerStopDone(callback);
-}
-
-void Pipeline::OnDemuxerStopDone(const base::Closure& callback) {
-  if (!message_loop_->BelongsToCurrentThread()) {
-    message_loop_->PostTask(FROM_HERE, base::Bind(
-        &Pipeline::OnDemuxerStopDone, this, callback));
-    return;
-  }
-
-  if (pipeline_filter_) {
-    pipeline_filter_->Stop(callback);
-    return;
-  }
-
-  callback.Run();
-}
-
 void Pipeline::DoSeek(TimeDelta seek_timestamp) {
   // TODO(acolwell): We might be able to convert this if (demuxer_) into a
   // DCHECK(). Further investigation is needed to make sure this won't introduce
@@ -1247,6 +1286,23 @@ void Pipeline::OnDemuxerSeekDone(TimeDelta seek_timestamp,
     return;
   }
 
+  if (status == PIPELINE_OK && audio_renderer_) {
+    audio_renderer_->Seek(seek_timestamp, base::Bind(
+        &Pipeline::OnAudioRendererSeekDone, this, seek_timestamp));
+    return;
+  }
+
+  OnAudioRendererSeekDone(seek_timestamp, status);
+}
+
+void Pipeline::OnAudioRendererSeekDone(base::TimeDelta seek_timestamp,
+                                       PipelineStatus status) {
+  if (!message_loop_->BelongsToCurrentThread()) {
+    message_loop_->PostTask(FROM_HERE, base::Bind(
+        &Pipeline::OnAudioRendererSeekDone, this, seek_timestamp, status));
+    return;
+  }
+
   PipelineStatusCB done_cb =
       base::Bind(&Pipeline::OnFilterStateTransitionWithStatus, this);
 
@@ -1281,4 +1337,13 @@ void Pipeline::StartClockIfWaitingForTimeUpdate_Locked() {
   clock_->Play();
 }
 
+void Pipeline::RunOnPipelineThread(const base::Closure& closure) {
+  if (!message_loop_->BelongsToCurrentThread()) {
+    message_loop_->PostTask(FROM_HERE, base::Bind(
+        &Pipeline::RunOnPipelineThread, this, closure));
+    return;
+  }
+  closure.Run();
+}
+
 }  // namespace media