Encrypted Media: Support Audio Decrypt-Only.

media/filters/audio_renderer_impl.cc

Index: media/filters/audio_renderer_impl.cc
diff --git a/media/filters/audio_renderer_impl.cc b/media/filters/audio_renderer_impl.cc
index c07048161d9967fde1beb70e79059d208d7e2719..00f26b27f19a0aa649f0392c091082d0314bdf77 100644
--- a/media/filters/audio_renderer_impl.cc
+++ b/media/filters/audio_renderer_impl.cc
@@ -19,6 +19,8 @@
 #include "media/base/bind_to_loop.h"
 #include "media/base/demuxer_stream.h"
 #include "media/base/media_switches.h"
+#include "media/filters/audio_decoder_factory.h"
+#include "media/filters/decrypting_demuxer_stream.h"
 
 namespace media {
 
@@ -92,6 +94,14 @@ void AudioRendererImpl::DoPause() {
 
 void AudioRendererImpl::Flush(const base::Closure& callback) {
   DCHECK(pipeline_thread_checker_.CalledOnValidThread());
+
+  // XXX(xhwang): We we want to wait for decrypting_demuxer_stream_->Reset() to
+  // fire the callback? It seems it's not strictly necessary. If that's true,
+  // shall we remove the completion callback in DecyrptingDemuxerStream::Reset()
+  // all together?
+  if (decrypting_demuxer_stream_)
+    decrypting_demuxer_stream_->Reset(base::Bind(&base::DoNothing));

[xhwang, 2012/12/11 02:27:40]

Similar to what we are doing in media pipeline, the decoder->Reset() doesn't
need to wait for demuxer_stream->Reset(). If we stick to this pattern, we could
probably remove the callback parameter in DecyrptingDemuxerStream::Reset().
Otherwise, I can add another callback to make sure the two Reset() are called in
sequence.

[ddorwin, 2012/12/11 05:13:34]

I defer to scherkus.

[scherkus (not reviewing), 2012/12/11 20:52:15]

Why do we not need to wait? Is it because it's synchronous or something else?

What are you referring to when you say "similar to what we are doing in media
pipeline"?

[xhwang, 2012/12/12 23:43:28]

Added another callback so we actually wait.

+
   decoder_->Reset(callback);
 }
 
@@ -149,15 +159,17 @@ void AudioRendererImpl::Preroll(base::TimeDelta time,
   sink_->Pause(true);
 }
 
-void AudioRendererImpl::Initialize(const scoped_refptr<DemuxerStream>& stream,
-                                   const AudioDecoderList& decoders,
-                                   const PipelineStatusCB& init_cb,
-                                   const StatisticsCB& statistics_cb,
-                                   const base::Closure& underflow_cb,
-                                   const TimeCB& time_cb,
-                                   const base::Closure& ended_cb,
-                                   const base::Closure& disabled_cb,
-                                   const PipelineStatusCB& error_cb) {
+void AudioRendererImpl::Initialize(
+    const scoped_refptr<DemuxerStream>& stream,
+    const AudioDecoderList& decoders,
+    const RequestDecryptorNotificationCB& request_decryptor_notification_cb,
+    const PipelineStatusCB& init_cb,
+    const StatisticsCB& statistics_cb,
+    const base::Closure& underflow_cb,
+    const TimeCB& time_cb,
+    const base::Closure& ended_cb,
+    const base::Closure& disabled_cb,
+    const PipelineStatusCB& error_cb) {
   DCHECK(pipeline_thread_checker_.CalledOnValidThread());
   DCHECK(stream);
   DCHECK(!decoders.empty());
@@ -180,46 +192,31 @@ void AudioRendererImpl::Initialize(const scoped_refptr<DemuxerStream>& stream,
   disabled_cb_ = disabled_cb;
   error_cb_ = error_cb;
 
-  scoped_ptr<AudioDecoderList> decoder_list(new AudioDecoderList(decoders));
-  InitializeNextDecoder(stream, decoder_list.Pass());
-}
-
-void AudioRendererImpl::InitializeNextDecoder(
-    const scoped_refptr<DemuxerStream>& demuxer_stream,
-    scoped_ptr<AudioDecoderList> decoders) {
-  DCHECK(pipeline_thread_checker_.CalledOnValidThread());
-  DCHECK(!decoders->empty());
-
-  scoped_refptr<AudioDecoder> decoder = decoders->front();
-  decoders->pop_front();
-
-  DCHECK(decoder);
-  decoder_ = decoder;
-  decoder->Initialize(
-      demuxer_stream, BindToLoop(base::MessageLoopProxy::current(), base::Bind(
-          &AudioRendererImpl::OnDecoderInitDone, this, demuxer_stream,
-          base::Passed(&decoders))),
-      statistics_cb_);
+  factory_.reset(new AudioDecoderFactory(base::MessageLoopProxy::current(),
+                                         decoders,
+                                         request_decryptor_notification_cb));
+  factory_->InitAudioDecoder(
+      stream,
+      statistics_cb,
+      base::Bind(&AudioRendererImpl::OnDecoderInitDone, this));
 }
 
 void AudioRendererImpl::OnDecoderInitDone(
-    const scoped_refptr<DemuxerStream>& demuxer_stream,
-    scoped_ptr<AudioDecoderList> decoders,
-    PipelineStatus status) {
+    const scoped_refptr<AudioDecoder>& audio_decoder,

[ddorwin, 2012/12/11 05:13:34]

Would selected_decoder be a more helpful name?

[xhwang, 2012/12/12 23:43:28]

Done.

+    const scoped_refptr<DecryptingDemuxerStream>& decrypting_demuxer_stream) {
   DCHECK(pipeline_thread_checker_.CalledOnValidThread());
 
+  decoder_ = audio_decoder;
+  decrypting_demuxer_stream_ = decrypting_demuxer_stream;
+  factory_.reset();

[ddorwin, 2012/12/11 05:13:34]

Should we have the factory manage its own lifetime? See comment in ADF.h.

[xhwang, 2012/12/11 19:43:04]

After the audio decoder (and decrypting_demuxer_stream) is/are populated, the
factory is not needed anymore. That's why it's destroyed here. I am not sure how
it could manage it's own lifetime...

[ddorwin, 2012/12/12 00:30:25]

Init() AddRef()'s then errors or success DeRef() after calling the callback.

That's a bit odd too the way the factory has a constructor. Really we don't need
a factory object - we just want this class to do something and notify us when
it's done. This happens to need state due to all the callbacks. Maybe we can
hide or avoid this. Possibilities, respectively:
 * Factory::Get() creates a self-managing pimpl.
 * Pass a state object to each function in the flow. Maybe as a scoped_ptr or
something so it is always deleted.

WDYT?

[xhwang, 2012/12/12 23:43:28]

Pass the scoped_ptr in the callback to keep it alive.

+
   if (state_ == kStopped) {
     DCHECK(!sink_);

[ddorwin, 2012/12/11 05:13:34]

Should we have stored decoder_ at 209 in this case?
I'm thinking we should have 211 then 213-216 then 218-221 then 209-210. WDYT?

[xhwang, 2012/12/11 19:43:04]

It's a little tricky. We are passing const-ref of the scoped_refptr, so we are
not adding ref counts to the audio_decoder and decrypting_demuxer_stream. If we
destroy the factory early, we could end up destroying the audio_decoder and/or
decrypting_demuxer_stream as well.

Other than that I agree the order can be improved. Will work on that.

     return;
   }
 
-  if (!decoders->empty() && status == DECODER_ERROR_NOT_SUPPORTED) {
-    InitializeNextDecoder(demuxer_stream, decoders.Pass());
-    return;
-  }
-
-  if (status != PIPELINE_OK) {
-    base::ResetAndReturn(&init_cb_).Run(status);
+  if (!decoder_) {
+    base::ResetAndReturn(&init_cb_).Run(DECODER_ERROR_NOT_SUPPORTED);
     return;
   }