Encrypted Media: Support Audio Decrypt-Only.

media/filters/audio_decoder_factory.h

+// 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.
+
+#ifndef MEDIA_FILTERS_AUDIO_DECODER_FACTORY_H_
+#define MEDIA_FILTERS_AUDIO_DECODER_FACTORY_H_
+
+#include <list>
+
+#include "base/callback.h"
+#include "base/memory/ref_counted.h"
+#include "base/memory/weak_ptr.h"
+#include "media/base/audio_decoder.h"
+#include "media/base/decryptor.h"
+#include "media/base/demuxer_stream.h"
+
+namespace base {
+class MessageLoopProxy;
+}
+
+namespace media {
+
+class DecoderBuffer;
+class DecryptingDemuxerStream;
+class Decryptor;
+
+// AudioDecoderFactory (creates if necessary and) initializes the proper
+// AudioDecoder for a given DemuxerStream. If the given DemuxerStream is
+// encrypted, a DecryptingDemuxerStream may also be created.
+class MEDIA_EXPORT AudioDecoderFactory {

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

It's odd for a factory to have state. This factory uses multiple threads I
assume, so we'll need state somewhere. Does someone own the factory instance?
(Thinking out loud, I'll answer this question myself.)

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

It's not factory anymore.

+ public:
+  typedef std::list<scoped_refptr<AudioDecoder> > AudioDecoderList;
+
+  // Indicates completion of AudioDecoder initialization.
+  //
+  // First parameter: The initialized AudioDecoder. If it's set to NULL, then
+  //                  AudioDecoder initialization failed.

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

nit: this style of indentation is a waste of valuable whitespace

my suggestion is to simply add an empty line inbetween first and second
parameter descriptions

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

Done.

+  // Second parameter: The initialized DecryptingDemuxerStream. If it's not
+  //                   NULL, then a DecryptingDemuxerStream is created and
+  //                   initialized to do decryption for the initialized
+  //                   AudioDecoder.
+  typedef base::Callback<
+      void(const scoped_refptr<AudioDecoder>&,
+           const scoped_refptr<DecryptingDemuxerStream>&)> InitDoneCB;

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

Should the callback be DecoderCreatedCB?

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

Changed to DecoderSlectedCB.

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

Done.

+
+  // If |request_decryptor_notification_cb| is null, no decryptor will be
+  // available to perform decryption.

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

I don't understand? The factory will not support decrypted streams? Why would a
callback parameter have control implications?

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

When --enable-encrypted-media is not set, we don't create the ProxyDecryptor at
all
(http://code.google.com/searchframe#OAMlx_jo-ck/src/webkit/media/webmediaplaye...).
That's when this callback could be null. In that case, we want the factory to
just initialize decoders from the precreated list.

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

Why not use an explicit control (Boolean)? It doesn't matter too much since this
condition will go away soonish.

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

Yep, will keep what I have here.

+  AudioDecoderFactory(
+      const scoped_refptr<base::MessageLoopProxy>& message_loop,

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

Is there a method/pattern to who parameters are passed to the constructor or
Init? Is this designed such that the factory could be used to obtain multiple
decoders? In the current use, the class is constructed then Init is called, so
we could even have a simple constructor or eliminate Init. (I'm not saying we
should, just pointing out the usage.)

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

Not sure, keep the code as is unless anybody objects again.

+      const AudioDecoderList& precreated_decoders,

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

"precreated" is an odd term.

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

How about "default", to match "AddDefaultDecodersToCollection" in filter_helper?

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

Sounds better, BUT not all are default. When we do this for video, GPU is not
created by that function. What about standard or normal or clear?

+      const RequestDecryptorNotificationCB& request_decryptor_notification_cb);
+  ~AudioDecoderFactory();
+
+  void InitAudioDecoder(const scoped_refptr<DemuxerStream>& stream,

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

Is this Init or Create?

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

Well, this is tricky. The accurate name should be
"CreateIfNeededAndInitializeAudioDecoder". The complication comes from the fact
that some decoders are pre-created. So I just took the common part "Init" and
skipped the tricky part "Create"...

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

Select? Get? Obtain?

On a related note, Factories create things so maybe this class is misnamed.

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

Renamed to Select. Class name renamed also.

+                        const StatisticsCB& statistics_cb,
+                        const InitDoneCB& init_done_cb);
+
+ private:
+  void DecryptingAudioDecoderInitDone(PipelineStatus status);
+  void DecryptingDemuxerStreamInitDone(PipelineStatus status);
+  void InitializeNextPrecreatedDecoder();
+  void PrecreatedDecoderInitDone(PipelineStatus status);
+
+  scoped_refptr<base::MessageLoopProxy> message_loop_;
+  AudioDecoderList precreated_decoders_;
+  RequestDecryptorNotificationCB request_decryptor_notification_cb_;
+
+  scoped_refptr<DemuxerStream> input_stream_;
+  StatisticsCB statistics_cb_;
+  InitDoneCB init_done_cb_;
+
+  scoped_refptr<AudioDecoder> audio_decoder_;
+  scoped_refptr<DecryptingDemuxerStream> decrypted_stream_;
+
+  base::WeakPtrFactory<AudioDecoderFactory> weak_ptr_factory_;
+  base::WeakPtr<AudioDecoderFactory> weak_this_;
+
+  DISALLOW_COPY_AND_ASSIGN(AudioDecoderFactory);
+};
+
+}  // namespace media
+
+#endif  // MEDIA_FILTERS_AUDIO_DECODER_FACTORY_H_