Add Stop() to AudioDecoder.

media/filters/audio_renderer_impl.cc

 // Copyright (c) 2012 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/filters/audio_renderer_impl.h"
 
 #include <math.h>
 
 #include <algorithm>
 
@@ skipping 164 matching lines @@
   rendered_end_of_stream_ = false;
   preroll_aborted_ = false;
 
   earliest_end_time_ = now_cb_.Run();
   splicer_->Reset();
   algorithm_->FlushBuffers();
 
   base::ResetAndReturn(&flush_cb_).Run();
 }
 
+void AudioRendererImpl::StopDecoder() {
+  DCHECK(task_runner_->BelongsToCurrentThread());
+  decoder_->Stop(BindToCurrentLoop(
+      base::Bind(&AudioRendererImpl::StopDecoderDone, weak_this_)));
+}
+
+void AudioRendererImpl::StopDecoderDone() {
+  base::AutoLock auto_lock(lock_);

[DaleCurtis, 2014/01/08 00:30:54]

Previous method doesn't run this under lock.  It may be important not to do
that.

[rileya (GONE FROM CHROMIUM), 2014/01/08 21:05:50]

Which previous method do you mean?

[DaleCurtis, 2014/01/08 21:51:45]

See line 236 in Stop() below. callback.Run() occurs after releasing the lock_.

[rileya (GONE FROM CHROMIUM), 2014/01/08 22:02:55]

Ahh, gotcha. Looks like ResetDecoderDone also calls the reset callback before
releasing the lock, would changing both to call after releasing |lock_| be
reasonable?

[DaleCurtis, 2014/01/08 23:56:14]

sgtm

+  if (state_ == kStopped)
+    return;
+
+  DCHECK(!stop_cb_.is_null());
+
+  ChangeState_Locked(kStopped);
+  algorithm_.reset(NULL);

[DaleCurtis, 2014/01/08 00:30:54]

drop NULL.

+  init_cb_.Reset();
+  underflow_cb_.Reset();
+  time_cb_.Reset();
+  flush_cb_.Reset();

[xhwang, 2014/01/08 01:33:37]

Wrap 199-203 in a helper function so it can be shared here and on line 227?

[rileya (GONE FROM CHROMIUM), 2014/01/08 21:05:50]

StopDecoderDone was already basically this, so per Dale's suggestion on 226 I
just called it there.

+
+  base::ResetAndReturn(&stop_cb_).Run();
+}
+
 void AudioRendererImpl::Stop(const base::Closure& callback) {
   DCHECK(task_runner_->BelongsToCurrentThread());
   DCHECK(!callback.is_null());
 
   // TODO(scherkus): Consider invalidating |weak_factory_| and replacing
   // task-running guards that check |state_| with DCHECK().
 
   if (sink_) {
     sink_->Stop();
     sink_ = NULL;
   }
 
+  if (decoder_) {
+    stop_cb_ = callback;
+    StopDecoder();

[DaleCurtis, 2014/01/08 00:30:54]

You can probably drop the StopDecoder() method in favor of inlining it here.

[rileya (GONE FROM CHROMIUM), 2014/01/08 21:05:50]

Done.

+    return;
+  }
+
   {

[DaleCurtis, 2014/01/08 00:30:54]

Just call StopDecoderDone here?

[rileya (GONE FROM CHROMIUM), 2014/01/08 21:05:50]

Makes sense, done.

     base::AutoLock auto_lock(lock_);
     ChangeState_Locked(kStopped);
     algorithm_.reset(NULL);
     init_cb_.Reset();

[xhwang, 2014/01/08 01:33:37]

If Init() didn't succeed, Stop() should not be called (needs to be verified). So
probably we should DCHECK(init_cb_.is_null()) here.

[rileya (GONE FROM CHROMIUM), 2014/01/08 21:05:50]

Alright, added a check (line 191).

     underflow_cb_.Reset();
     time_cb_.Reset();
     flush_cb_.Reset();
   }
 
   callback.Run();
 }
 
 void AudioRendererImpl::Preroll(base::TimeDelta time,
                                 const PipelineStatusCB& cb) {
@@ skipping 488 matching lines @@
   }
 }
 
 void AudioRendererImpl::ChangeState_Locked(State new_state) {
   DVLOG(1) << __FUNCTION__ << " : " << state_ << " -> " << new_state;
   lock_.AssertAcquired();
   state_ = new_state;
 }
 
 }  // namespace media