Fix VideoDecoderShim to return correct decode_id for all codecs.

media/base/video_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_VIDEO_DECODER_H_
 #define MEDIA_BASE_VIDEO_DECODER_H_
 
 #include <string>
 
 #include "base/callback.h"
@@ skipping 37 matching lines @@
   // depends on |this|.
   virtual ~VideoDecoder();
 
   // Returns the name of the decoder for logging purpose.
   virtual std::string GetDisplayName() const = 0;
 
   // Initializes a VideoDecoder with the given |config|, executing the
   // |status_cb| upon completion. |output_cb| is called for each output frame
   // decoded by Decode().
   //
+  // If |low_delay| is true then the decoder is not allowed to queue frames
+  // internally, i.e. every frame must be returned as soon as it is decoded.
+  // Currently this flag is necessary only for FFmpegDecoder. Other decoders
+  // do not need to delay frames.

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

I am still a bit concerned about this change in the API. As far as I know
|low-delay| is really a hint instead a requirement. It's hard to enforce all
VideoDecoder implementations to respect this flag. For example, for
GpuVideoDecoder, it's actually up to the VDAs to decode what the decoding delay
is. If we really want to enforce this, a VideoDecoder should fail to initialize
if |low-delay| is true, and the decoder cannot support this mode. But I don't
think we are enforcing  this right now.

Also, I won't put any implementation detail (e.g. about FFmpegVideoDecoder) in
this file. If VideoDecoderShim is relying on some special property in
FFmpegVideoDecoder and VpxVideoDecoder, maybe we should note in
VideoDecoderShim, e.g. if we use a different VideoDecoder in VideoDecoderShim,
it may not work.

[Sergey Ulanov, 2015/01/20 23:52:26]

There are two separate issues: frame reordering and frame delay. This flag
disallows "frame delay".

For out-of-order streams the decoder will decode frames out of order and may
need to hold some frames before returning them, to make sure they are returned
in correct order. That's fine, no matter what the low_delay is set to. This flag
is here to disable "frame delay", i.e. to tell the decoder that it's not allowed
to hold frames just because it wants to.  I reworded the comment to make it
clear what's expected behavior for out-of-order frames.

Again, this flag is needed only because of ffmpeg. Problem is that ffmpeg's API
is not designed properly for concurrent decoding so it needs to delay frames to
implement multi-threading. This flag is essentially a hack to workaround that
problem with FFmpeg. From what I remember from discussions with Ami when this
flag was added (when I was working on fixing frame-delay issues for MediaSource)
hardware decoders should not have any "decoding delay", meaning that if the next
frame can be decoded from the buffers that have been sent to the decoder it must
be returned by the decoder, even if we don't submit more data to the decoder. If
there are any cases when frames are delayed for any reason other then reordering
then I think we should be fixing them (otherwise the MediaSource and
PPB_VideoDecoder APIs are broken). 
Given that ffmpeg is the only reason we need this flag (see my comment above) I
think it's appropriate to mention it here.

[xhwang, 2015/01/21 23:05:04]

Thanks for the detailed clarification. I think my concern is that what if in the
future, a VideoDecoder implementation cannot support the |low_delay| mode you
described? Should it fail to initialize when |low_delay| is true? Since the
whole media pipeline doesn't really need this restriction, I'd be reluctant to
put it in the API doc. Also, the current comment isn't clear. For example, for
"every frame must be returned as soon as it is decoded", does it exclude the
frames delayed for reordering?

I understand that VideoDecoderShim need this specific property. So I suggest add
comments in VideoDecoderShim, and in FFmpegVideoDecoder/VpxVideoDecoder (e.g. no
delay because VDS depends on it...).
 
This is an interface. I don't really think we should put implementation detail
here unless really necessary. In the future, we may have another VideoDecoder
that supports/needs this flag...

+  //
   // Note:
   // 1) The VideoDecoder will be reinitialized if it was initialized before.
   //    Upon reinitialization, all internal buffered frames will be dropped.
   // 2) This method should not be called during pending decode or reset.
   // 3) No VideoDecoder calls should be made before |status_cb| is executed.
   virtual void Initialize(const VideoDecoderConfig& config,
                           bool low_delay,
                           const PipelineStatusCB& status_cb,
                           const OutputCB& output_cb) = 0;
 
   // Requests a |buffer| to be decoded. The status of the decoder and decoded
   // frame are returned via the provided callback. Some decoders may allow
   // decoding multiple buffers in parallel. Callers should call
   // GetMaxDecodeRequests() to get number of buffers that may be decoded in
   // parallel. Decoder must call |decode_cb| in the same order in which Decode()
   // is called.
   //
   // Implementations guarantee that the callback will not be called from within
   // this method and that |decode_cb| will not be blocked on the following
   // Decode() calls (i.e. |decode_cb| will be called even if Decode() is never
   // called again).
   //
   // After decoding is finished the decoder calls |output_cb| specified in
-  // Initialize() for each decoded frame. |output_cb| may be called before or
-  // after |decode_cb|.
+  // Initialize() for each decoded frame. In general |output_cb| may be called
+  // before or after |decode_cb|, but software decoders in low-delay mode must
+  // call |output_cb| for all frames in |buffer| before calling |decode_cb|.

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

I think it makes sense to require |decode_cb| to be called AFTER |output_cb| for
all frames generated during this decode cycle. But that doesn't necessarily mean
all frames associated with |buffer|, due to decoding delay and/or reordering.
|low_delay| is only a hint and it's hard to make it a requirement.

[Sergey Ulanov, 2015/01/20 23:52:26]

Reworded this comment.

   //
   // If |buffer| is an EOS buffer then the decoder must be flushed, i.e.
   // |output_cb| must be called for each frame pending in the queue and
   // |decode_cb| must be called after that. Callers will not call Decode()
   // again until after the flush completes.
   virtual void Decode(const scoped_refptr<DecoderBuffer>& buffer,
                       const DecodeCB& decode_cb) = 0;
 
   // Resets decoder state. All pending Decode() requests will be finished or
   // aborted before |closure| is called.
@@ skipping 12 matching lines @@
   // Returns maximum number of parallel decode requests.
   virtual int GetMaxDecodeRequests() const;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(VideoDecoder);
 };
 
 }  // namespace media
 
 #endif  // MEDIA_BASE_VIDEO_DECODER_H_