Avoid double task trampoline for Pipeline state transitions.

media/filters/renderer_impl.cc

 // Copyright 2014 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/renderer_impl.h"
 
 #include "base/bind.h"
 #include "base/callback.h"
 #include "base/callback_helpers.h"
 #include "base/compiler_specific.h"
 #include "base/location.h"
 #include "base/single_thread_task_runner.h"
 #include "media/base/audio_renderer.h"
+#include "media/base/bind_to_current_loop.h"
 #include "media/base/demuxer_stream_provider.h"
 #include "media/base/time_source.h"
 #include "media/base/video_renderer.h"
 #include "media/base/wall_clock_time_source.h"
 
 namespace media {
 
 RendererImpl::RendererImpl(
     const scoped_refptr<base::SingleThreadTaskRunner>& task_runner,
     scoped_ptr<AudioRenderer> audio_renderer,
@@ skipping 47 matching lines @@
   DCHECK(demuxer_stream_provider->GetStream(DemuxerStream::AUDIO) ||
          demuxer_stream_provider->GetStream(DemuxerStream::VIDEO));
 
   demuxer_stream_provider_ = demuxer_stream_provider;
   statistics_cb_ = statistics_cb;
   buffering_state_cb_ = buffering_state_cb;
   paint_cb_ = paint_cb;
   ended_cb_ = ended_cb;
   error_cb_ = error_cb;
 
-  init_cb_ = init_cb;
+  // Always post the callback because |this| could be destroyed upon failure.
+  init_cb_ = BindToCurrentLoop(init_cb);

[xhwang, 2015/01/16 23:09:07]

This is the tricky part of our rule that callee must "ensure" the callback is
called asynchronously. I think InitializeAudioRenderer() makes sure init_cb_ is
always called asynchronously. We didn't use BindToCurrentLoop here because we
don't want to have too many posts which is unnecessary. But I agree that the
intention is not obvious...

Or are you worrying about calling the callback directly in any public method, to
avoid reentrancy back into the caller?

[DaleCurtis, 2015/01/16 23:15:15]

I realize we post below and init cb still runs asynchronously, but it's not on a
clean stack like it was before. That's likely fine if the caller runs the
init_cb and does nothing after, but may result in some long stacks. I erred on
the side of forcing a clean stack even if it's unnecessary. I'd be okay with not
doing that since the stack still wouldn't be very deep.

[xhwang, 2015/01/16 23:24:04]

I am not too worried about long stack as long as we are not reentering the
caller or the callee in the middle of a previous call. I am trying to think of
some more general rule on this but apparently I need to think more on this :(

[DaleCurtis, 2015/01/16 23:33:49]

I think just adding a note to the demuxer and renderer interfaces saying the
init_cb/status_cb "must be run asynchronously (i.e. not during the context of
the initialize()) call." sounds fine. Is there something else you're thinking
of?

[DaleCurtis, 2015/01/20 21:37:43]

Done.

+
   state_ = STATE_INITIALIZING;
   InitializeAudioRenderer();
 }
 
 void RendererImpl::SetCdm(CdmContext* cdm_context,
                           const CdmAttachedCB& cdm_attached_cb) {
   DVLOG(1) << __FUNCTION__;
   DCHECK(task_runner_->BelongsToCurrentThread());
   DCHECK(cdm_context);
 
@@ skipping 142 matching lines @@
   DVLOG(1) << __FUNCTION__;
   DCHECK(task_runner_->BelongsToCurrentThread());
   DCHECK_EQ(state_, STATE_INITIALIZING);
   DCHECK(!init_cb_.is_null());
 
   PipelineStatusCB done_cb =
       base::Bind(&RendererImpl::OnAudioRendererInitializeDone, weak_this_);
 
   if (!demuxer_stream_provider_->GetStream(DemuxerStream::AUDIO)) {
     audio_renderer_.reset();
     task_runner_->PostTask(FROM_HERE, base::Bind(done_cb, PIPELINE_OK));

[xhwang, 2015/01/16 23:09:07]

See above: We post here; then audio_renderer_->Initialize() *should* also post
the done_cb.

     return;
   }
 
   // Note: After the initialization of a renderer, error events from it may
   // happen at any time and all future calls must guard against STATE_ERROR.
   audio_renderer_->Initialize(
       demuxer_stream_provider_->GetStream(DemuxerStream::AUDIO), done_cb,
       base::Bind(&RendererImpl::SetDecryptorReadyCallback, weak_this_),
       base::Bind(&RendererImpl::OnUpdateStatistics, weak_this_),
       base::Bind(&RendererImpl::OnBufferingStateChanged, weak_this_,
@@ skipping 305 matching lines @@
   DCHECK(task_runner_->BelongsToCurrentThread());
 
   if (!init_cb_.is_null())
     base::ResetAndReturn(&init_cb_).Run();
 
   if (!flush_cb_.is_null())
     base::ResetAndReturn(&flush_cb_).Run();
 }
 
 }  // namespace media