Fix VideoDecoderShim to return correct decode_id for all codecs.

media/base/video_decoder.h

Index: media/base/video_decoder.h
diff --git a/media/base/video_decoder.h b/media/base/video_decoder.h
index d7e7a791de3d168668476cbdbd73aae22dd90ef4..74adcda3c46571ab5e5a3b09392605ee474bb7da 100644
--- a/media/base/video_decoder.h
+++ b/media/base/video_decoder.h
@@ -55,6 +55,11 @@ class MEDIA_EXPORT VideoDecoder {
   // |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.
@@ -78,8 +83,9 @@ class MEDIA_EXPORT VideoDecoder {
   // 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