Add callback in VideoDecoder and AudioDecoder to return decoded frames.

media/base/audio_decoder.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_BASE_AUDIO_DECODER_H_
 #define MEDIA_BASE_AUDIO_DECODER_H_
 
 #include "base/callback.h"
 #include "base/memory/ref_counted.h"
 #include "media/base/audio_decoder_config.h"
 #include "media/base/channel_layout.h"
 #include "media/base/decoder_buffer.h"
 #include "media/base/media_export.h"
 #include "media/base/pipeline_status.h"
 
 namespace media {
 
 class AudioBuffer;
 class DemuxerStream;
 
 class MEDIA_EXPORT AudioDecoder {
  public:
   // Status codes for decode operations.
   // TODO(rileya): Now that both AudioDecoder and VideoDecoder Status enums
   // match, break them into a decoder_status.h.
   enum Status {
     kOk,  // We're all good.
     kAborted,  // We aborted as a result of Stop() or Reset().
-    kNotEnoughData,  // Not enough data to produce a video frame.
     kDecodeError,  // A decoding error occurred.
     kDecryptError  // Decrypting error happened.
   };
 
+  // Callback to return decoded buffers.
+  typedef base::Callback<void(const scoped_refptr<AudioBuffer>&)> OutputCB;
+
+  // Callback for Decode();
+  typedef base::Callback<void(Status)> DecodeCB;

[xhwang, 2014/05/29 22:15:14]

What does this exactly mean? We need a comment here to make the API contract
clear. For example (if I understand it correctly): Called when the input has
been processed and the VideoDecoder can accept more inputs for decode.

[Sergey Ulanov, 2014/06/03 00:08:11]

Done.

+
   AudioDecoder();
   virtual ~AudioDecoder();
 
   // Initializes an AudioDecoder with the given DemuxerStream, executing the
   // callback upon completion.
   // statistics_cb is used to update global pipeline statistics.

[xhwang, 2014/05/29 22:15:14]

Add a comment on |output_cb|.

[Sergey Ulanov, 2014/06/03 00:08:11]

Done.

   virtual void Initialize(const AudioDecoderConfig& config,
-                          const PipelineStatusCB& status_cb) = 0;
+                          const PipelineStatusCB& status_cb,
+                          const OutputCB& output_cb) = 0;
 
   // Requests samples to be decoded and returned via the provided callback.

[xhwang, 2014/05/29 22:15:14]

This needs to be updated.

[Sergey Ulanov, 2014/06/03 00:08:11]

Done.

   // Only one decode may be in flight at any given time.
   //
   // Implementations guarantee that the callback will not be called from within
   // this method.
   //
   // Non-NULL sample buffer pointers will contain decoded audio data or may
   // indicate the end of the stream. A NULL buffer pointer indicates an aborted
   // Decode().
-  typedef base::Callback<void(Status, const scoped_refptr<AudioBuffer>&)>
-      DecodeCB;
   virtual void Decode(const scoped_refptr<DecoderBuffer>& buffer,
                       const DecodeCB& decode_cb) = 0;
 
-  // Some AudioDecoders will queue up multiple AudioBuffers from a single
-  // DecoderBuffer, if we have any such queued buffers this will return the next
-  // one. Otherwise we return a NULL AudioBuffer.
-  virtual scoped_refptr<AudioBuffer> GetDecodeOutput();
-
   // Resets decoder state, dropping any queued encoded data.
   virtual void Reset(const base::Closure& closure) = 0;

[xhwang, 2014/05/29 22:15:14]

In this case, shall all decoded frames be simply dropped, or fired via OutputCB?

[Sergey Ulanov, 2014/06/03 00:08:11]

I think the comment is not entirely true. The buffers may not be actually
dropped. Audio decoders post a task for all callbacks, and these tasks are not
canceled by Reset(). Updated it to reflect this issue.

[xhwang, 2014/06/05 21:53:48]

To clarify, do you mean when Reset() is called, there may already be DecodeCBs
posted to the message loop but not executed yet? In that case, the AudioDecoders
can always post the reset callback. Callbacks should be executed in the order
they are posted so my previous comment will be valid. Am I missing anything?

[Sergey Ulanov, 2014/06/06 22:49:39]

Yes, that's right.
My point was that pending Decode requests may still finish with kOk, so from
callers perspective it doesn't look like all data is dropped as the comment was
saying. Reworded the comment as follows:
  // Resets decoder state. All pending Decode() requests will be finished or
  // aborted before |closure| is called.

 
   // Stops decoder, fires any pending callbacks and sets the decoder to an
   // uninitialized state. An AudioDecoder cannot be re-initialized after it has
   // been stopped.
   // Note that if Initialize() is pending or has finished successfully, Stop()
   // must be called before destructing the decoder.
   virtual void Stop() = 0;

[xhwang, 2014/05/29 22:15:14]

ditto about "shall all decoded frames be simply dropped, or fired via OutputCB"?

[Sergey Ulanov, 2014/06/03 00:08:11]

Done.

BTW, Do we really need this method? Given that the object is not usable after
Stop() it looks like decoders can just call Stop() from destructor. What am I
missing?

[xhwang, 2014/06/05 21:53:48]

You are absolutely right! We are planning to drop Stop() call entirely. See
crbug/349211

 
  private:
   DISALLOW_COPY_AND_ASSIGN(AudioDecoder);
 };
 
 }  // namespace media
 
 #endif  // MEDIA_BASE_AUDIO_DECODER_H_