Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
805193006:
Fix VideoDecoderShim to return correct decode_id for all codecs. (Closed)
|
Created:
5 years, 11 months ago by Sergey Ulanov Modified:
5 years, 11 months ago CC:
chromium-reviews, mkwst+moarreviews-renderer_chromium.org, darin-cc_chromium.org, feature-media-reviews_chromium.org, jam, mlamouri+watch-content_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix VideoDecoderShim to return correct decode_id for all codecs.
Previously VideoDecoderShim was assuming that the decoder calls output
callback only after decode is finished. That was a correct assumption
for VpxVideoDecoder, but not for FFmpegVideoDecoder. As result
PPB_VideoDecoder wasn't returning correct decode_id for decoded frames.
Now software decoders (i.e. VpxVideoDecoder and FFmpegVideoDecoder)
guarantee that they output all decoded frame before Decode() call
completes, which allows to return correct decode_id from
VideoDecoderShim.
BUG=448983
R=bbudge@chromium.org, xhwang@chromium.org
Committed: https://chromium.googlesource.com/chromium/src/+/be7ac82117cb884b2905a4565e49130d11274d80
Patch Set 1 : #
Total comments: 11
Patch Set 2 : #
Total comments: 2
Patch Set 3 : #
Total comments: 9
Patch Set 4 : #Patch Set 5 : #
Total comments: 2
Patch Set 6 : #
Messages
Total messages: 30 (5 generated)
Patchset #1 (id:1) has been deleted
sergeyu@chromium.org changed reviewers: + bbudge@chromium.org, xhwang@chromium.org
bbudge@ - please review video_decoder_shim.cc xhwang@ - please review media/*
video_decoder_shim.cc LGTM w/nits https://codereview.chromium.org/805193006/diff/20001/content/renderer/pepper/... File content/renderer/pepper/video_decoder_shim.cc (right): https://codereview.chromium.org/805193006/diff/20001/content/renderer/pepper/... content/renderer/pepper/video_decoder_shim.cc:110: bool waiting_decoder_; nit: s/waiting/awaiting https://codereview.chromium.org/805193006/diff/20001/content/renderer/pepper/... content/renderer/pepper/video_decoder_shim.cc:112: // that generated it, but software decoder always generate corresponding nit: s/decoder/decoders https://codereview.chromium.org/805193006/diff/20001/content/renderer/pepper/... content/renderer/pepper/video_decoder_shim.cc:121: waiting_decoder_(0), s/0/false https://codereview.chromium.org/805193006/diff/20001/content/renderer/pepper/... content/renderer/pepper/video_decoder_shim.cc:264: // Software decoders are expected to generated frames only when Decode() call s/generated/generate s/Decode()/a Decode()
I have a general question... https://codereview.chromium.org/805193006/diff/20001/media/base/video_decoder.h File media/base/video_decoder.h (right): https://codereview.chromium.org/805193006/diff/20001/media/base/video_decoder... media/base/video_decoder.h:83: // |output_cb| before calling |decode_cb|. Hmm, is there any reason why VideoDecoderShim can't work with the current VideoDecoder interface and has to have multiple exceptions?
On 2015/01/15 21:21:00, bbudge wrote: > video_decoder_shim.cc LGTM w/nits > > https://codereview.chromium.org/805193006/diff/20001/content/renderer/pepper/... > File content/renderer/pepper/video_decoder_shim.cc (right): > > https://codereview.chromium.org/805193006/diff/20001/content/renderer/pepper/... > content/renderer/pepper/video_decoder_shim.cc:110: bool waiting_decoder_; > nit: s/waiting/awaiting > > https://codereview.chromium.org/805193006/diff/20001/content/renderer/pepper/... > content/renderer/pepper/video_decoder_shim.cc:112: // that generated it, but > software decoder always generate corresponding > nit: s/decoder/decoders > > https://codereview.chromium.org/805193006/diff/20001/content/renderer/pepper/... > content/renderer/pepper/video_decoder_shim.cc:121: waiting_decoder_(0), > s/0/false > > https://codereview.chromium.org/805193006/diff/20001/content/renderer/pepper/... > content/renderer/pepper/video_decoder_shim.cc:264: // Software decoders are > expected to generated frames only when Decode() call > s/generated/generate > s/Decode()/a Decode() I forgot to say thanks for fixing this!
https://codereview.chromium.org/805193006/diff/20001/media/base/video_decoder.h File media/base/video_decoder.h (right): https://codereview.chromium.org/805193006/diff/20001/media/base/video_decoder... media/base/video_decoder.h:83: // |output_cb| before calling |decode_cb|. On 2015/01/15 21:35:10, xhwang wrote: > Hmm, is there any reason why VideoDecoderShim can't work with the current > VideoDecoder interface and has to have multiple exceptions? PPB_VideoDecoder API needs to tell the plugin which frame is generated by which input buffer. VideoDecoder interface currently doesn't support that. VideoDecoder just calls OutputCB, but it's not possible to tell which buffer the frame comes from. Another possible solution is to add decode_id parameter in Decode() and OutputCB, but that would also require updating AudioDecoder (because DecoderStream is shared), and would touch a lot of code, while this feature is needed only in VideoDecoderShim
https://codereview.chromium.org/805193006/diff/20001/media/base/video_decoder.h File media/base/video_decoder.h (right): https://codereview.chromium.org/805193006/diff/20001/media/base/video_decoder... media/base/video_decoder.h:83: // |output_cb| before calling |decode_cb|. On 2015/01/15 22:59:20, Sergey Ulanov wrote: > On 2015/01/15 21:35:10, xhwang wrote: > > Hmm, is there any reason why VideoDecoderShim can't work with the current > > VideoDecoder interface and has to have multiple exceptions? > > PPB_VideoDecoder API needs to tell the plugin which frame is generated by which > input buffer. VideoDecoder interface currently doesn't support that. > VideoDecoder just calls OutputCB, but it's not possible to tell which buffer the > frame comes from. Another possible solution is to add decode_id parameter in > Decode() and OutputCB, but that would also require updating AudioDecoder > (because DecoderStream is shared), and would touch a lot of code, while this > feature is needed only in VideoDecoderShim Thanks for the info! But what exactly do you mean here by "software decoders must always call |output_cb| before calling |decode_cb|"? What if the software has decoding delay? e.g. input 1, no output, decode_cb called input 2, no output, decode_cb called input 3, output 1, decode_cb called In the last line, |output_cb| is called before |decode_cb|, is it okay? But the |decode_cb| is also called before the future |output_cb| for input 3. So this is kinda ambiguous...
On 2015/01/15 23:41:47, xhwang wrote: > https://codereview.chromium.org/805193006/diff/20001/media/base/video_decoder.h > File media/base/video_decoder.h (right): > > https://codereview.chromium.org/805193006/diff/20001/media/base/video_decoder... > media/base/video_decoder.h:83: // |output_cb| before calling |decode_cb|. > On 2015/01/15 22:59:20, Sergey Ulanov wrote: > > On 2015/01/15 21:35:10, xhwang wrote: > > > Hmm, is there any reason why VideoDecoderShim can't work with the current > > > VideoDecoder interface and has to have multiple exceptions? > > > > PPB_VideoDecoder API needs to tell the plugin which frame is generated by > which > > input buffer. VideoDecoder interface currently doesn't support that. > > VideoDecoder just calls OutputCB, but it's not possible to tell which buffer > the > > frame comes from. Another possible solution is to add decode_id parameter in > > Decode() and OutputCB, but that would also require updating AudioDecoder > > (because DecoderStream is shared), and would touch a lot of code, while this > > feature is needed only in VideoDecoderShim > > Thanks for the info! But what exactly do you mean here by "software decoders > must always call |output_cb| before calling |decode_cb|"? What if the software > has decoding delay? e.g. > > input 1, no output, decode_cb called > input 2, no output, decode_cb called > input 3, output 1, decode_cb called > > In the last line, |output_cb| is called before |decode_cb|, is it okay? But the > |decode_cb| is also called before the future |output_cb| for input 3. So this is > kinda ambiguous... Good point. Updated this comment to apply only to low-delay mode. VideoDecoderShim uses low-delay mode.
https://codereview.chromium.org/805193006/diff/20001/content/renderer/pepper/... File content/renderer/pepper/video_decoder_shim.cc (right): https://codereview.chromium.org/805193006/diff/20001/content/renderer/pepper/... content/renderer/pepper/video_decoder_shim.cc:110: bool waiting_decoder_; On 2015/01/15 21:21:00, bbudge wrote: > nit: s/waiting/awaiting Done. https://codereview.chromium.org/805193006/diff/20001/content/renderer/pepper/... content/renderer/pepper/video_decoder_shim.cc:112: // that generated it, but software decoder always generate corresponding On 2015/01/15 21:21:00, bbudge wrote: > nit: s/decoder/decoders Done. https://codereview.chromium.org/805193006/diff/20001/content/renderer/pepper/... content/renderer/pepper/video_decoder_shim.cc:121: waiting_decoder_(0), On 2015/01/15 21:21:00, bbudge wrote: > s/0/false Done. https://codereview.chromium.org/805193006/diff/20001/content/renderer/pepper/... content/renderer/pepper/video_decoder_shim.cc:264: // Software decoders are expected to generated frames only when Decode() call On 2015/01/15 21:21:00, bbudge wrote: > s/generated/generate > s/Decode()/a Decode() Done.
On 2015/01/16 00:46:02, Sergey Ulanov wrote: > On 2015/01/15 23:41:47, xhwang wrote: > > > https://codereview.chromium.org/805193006/diff/20001/media/base/video_decoder.h > > File media/base/video_decoder.h (right): > > > > > https://codereview.chromium.org/805193006/diff/20001/media/base/video_decoder... > > media/base/video_decoder.h:83: // |output_cb| before calling |decode_cb|. > > On 2015/01/15 22:59:20, Sergey Ulanov wrote: > > > On 2015/01/15 21:35:10, xhwang wrote: > > > > Hmm, is there any reason why VideoDecoderShim can't work with the current > > > > VideoDecoder interface and has to have multiple exceptions? > > > > > > PPB_VideoDecoder API needs to tell the plugin which frame is generated by > > which > > > input buffer. VideoDecoder interface currently doesn't support that. > > > VideoDecoder just calls OutputCB, but it's not possible to tell which buffer > > the > > > frame comes from. Another possible solution is to add decode_id parameter in > > > Decode() and OutputCB, but that would also require updating AudioDecoder > > > (because DecoderStream is shared), and would touch a lot of code, while this > > > feature is needed only in VideoDecoderShim > > > > Thanks for the info! But what exactly do you mean here by "software decoders > > must always call |output_cb| before calling |decode_cb|"? What if the software > > has decoding delay? e.g. > > > > input 1, no output, decode_cb called > > input 2, no output, decode_cb called > > input 3, output 1, decode_cb called > > > > In the last line, |output_cb| is called before |decode_cb|, is it okay? But > the > > |decode_cb| is also called before the future |output_cb| for input 3. So this > is > > kinda ambiguous... > > Good point. Updated this comment to apply only to low-delay mode. > VideoDecoderShim uses low-delay mode. Hmm, but is |low-delay| equivalent to zero-delay? I doubt a decoder can guarantee this (even when low-delay is set) if the video stream is coded with inter-frame dependencies...
https://codereview.chromium.org/805193006/diff/40001/media/base/video_decoder.h File media/base/video_decoder.h (right): https://codereview.chromium.org/805193006/diff/40001/media/base/video_decoder... media/base/video_decoder.h:64: bool low_delay, We need a documentation of |low_delay| here...
Patchset #3 (id:60001) has been deleted
On 2015/01/16 01:24:26, xhwang wrote: > Hmm, but is |low-delay| equivalent to zero-delay? I doubt a decoder can > guarantee this (even when low-delay is set) if the video stream is coded with > inter-frame dependencies... Added a comment there. Note that low_delay only matters for FFmpegDecoder where this flag is just a way to workaround the way multi-threading is implemented in ffmpeg. I think all other decoders returns all frames as soon as they are decoded and this flag doesn't really matter for them (mentioned this in the comment).
https://codereview.chromium.org/805193006/diff/40001/media/base/video_decoder.h File media/base/video_decoder.h (right): https://codereview.chromium.org/805193006/diff/40001/media/base/video_decoder... media/base/video_decoder.h:64: bool low_delay, On 2015/01/16 01:24:33, xhwang wrote: > We need a documentation of |low_delay| here... Done.
Sorry for the delay. More comments for discussion. https://codereview.chromium.org/805193006/diff/80001/content/renderer/pepper/... File content/renderer/pepper/video_decoder_shim.cc (right): https://codereview.chromium.org/805193006/diff/80001/content/renderer/pepper/... content/renderer/pepper/video_decoder_shim.cc:113: // frames before decode is finished. See my comment below. It's hard to maintain this assumption for the future. Maybe say something like the software decoders we currently use are fine; if we ever want to use a different decoder, need to make sure there's no decoder delay etc. Also add comments in VpxVideoDecoder and FFmpegVideoDecoder that VideoDecoderShim relies on this feature. https://codereview.chromium.org/805193006/diff/80001/media/base/video_decoder.h File media/base/video_decoder.h (right): https://codereview.chromium.org/805193006/diff/80001/media/base/video_decoder... media/base/video_decoder.h:61: // do not need to delay frames. 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. https://codereview.chromium.org/805193006/diff/80001/media/base/video_decoder... media/base/video_decoder.h:88: // call |output_cb| for all frames in |buffer| before calling |decode_cb|. 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. https://codereview.chromium.org/805193006/diff/80001/media/filters/vpx_video_... File media/filters/vpx_video_decoder.cc (right): https://codereview.chromium.org/805193006/diff/80001/media/filters/vpx_video_... media/filters/vpx_video_decoder.cc:360: base::ResetAndReturn(&decode_cb_).Run(kOk); See my comment above. What I suggested should also work for this change.
https://codereview.chromium.org/805193006/diff/80001/content/renderer/pepper/... File content/renderer/pepper/video_decoder_shim.cc (right): https://codereview.chromium.org/805193006/diff/80001/content/renderer/pepper/... content/renderer/pepper/video_decoder_shim.cc:113: // frames before decode is finished. On 2015/01/16 23:50:09, xhwang wrote: > See my comment below. It's hard to maintain this assumption for the future. > Maybe say something like the software decoders we currently use are fine; if we > ever want to use a different decoder, need to make sure there's no decoder delay > etc. Updated the comment > > Also add comments in VpxVideoDecoder and FFmpegVideoDecoder that > VideoDecoderShim relies on this feature. Done. https://codereview.chromium.org/805193006/diff/80001/media/base/video_decoder.h File media/base/video_decoder.h (right): https://codereview.chromium.org/805193006/diff/80001/media/base/video_decoder... media/base/video_decoder.h:61: // do not need to delay frames. On 2015/01/16 23:50:09, xhwang wrote: > 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. 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). > > 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. Given that ffmpeg is the only reason we need this flag (see my comment above) I think it's appropriate to mention it here. https://codereview.chromium.org/805193006/diff/80001/media/base/video_decoder... media/base/video_decoder.h:88: // call |output_cb| for all frames in |buffer| before calling |decode_cb|. On 2015/01/16 23:50:09, xhwang wrote: > 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. Reworded this comment. https://codereview.chromium.org/805193006/diff/80001/media/filters/vpx_video_... File media/filters/vpx_video_decoder.cc (right): https://codereview.chromium.org/805193006/diff/80001/media/filters/vpx_video_... media/filters/vpx_video_decoder.cc:360: base::ResetAndReturn(&decode_cb_).Run(kOk); On 2015/01/16 23:50:09, xhwang wrote: > See my comment above. What I suggested should also work for this change. Done.
https://codereview.chromium.org/805193006/diff/80001/media/base/video_decoder.h File media/base/video_decoder.h (right): https://codereview.chromium.org/805193006/diff/80001/media/base/video_decoder... media/base/video_decoder.h:61: // do not need to delay frames. On 2015/01/20 23:52:26, Sergey Ulanov wrote: > On 2015/01/16 23:50:09, xhwang wrote: > > 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. > > 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). 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...). > > > > 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. > > Given that ffmpeg is the only reason we need this flag (see my comment above) I > think it's appropriate to mention it here. 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...
Given that I removed the mentioning of low-delay mode from the comments for Decode(), I think the low_delay comment is not required for the bug I'm fixing here. I've uploaded a separate CL for that here: https://codereview.chromium.org/868443004/ and removed the comment for low_delay in video_decoder.h in this CL. PTAL. On 2015/01/21 23:05:04, xhwang wrote: > https://codereview.chromium.org/805193006/diff/80001/media/base/video_decoder.h > File media/base/video_decoder.h (right): > > https://codereview.chromium.org/805193006/diff/80001/media/base/video_decoder... > media/base/video_decoder.h:61: // do not need to delay frames. > On 2015/01/20 23:52:26, Sergey Ulanov wrote: > > On 2015/01/16 23:50:09, xhwang wrote: > > > 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. > > > > 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). > > 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...). > > > > > > > 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. > > > > Given that ffmpeg is the only reason we need this flag (see my comment above) > I > > think it's appropriate to mention it here. > > 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...
Given that I removed the mentioning of low-delay mode from the comments for Decode(), I think the low_delay comment is not required for the bug I'm fixing here. I've uploaded a separate CL for that here: https://codereview.chromium.org/868443004/ and removed the comment for low_delay in video_decoder.h in this CL. PTAL. On 2015/01/21 23:05:04, xhwang wrote: > https://codereview.chromium.org/805193006/diff/80001/media/base/video_decoder.h > File media/base/video_decoder.h (right): > > https://codereview.chromium.org/805193006/diff/80001/media/base/video_decoder... > media/base/video_decoder.h:61: // do not need to delay frames. > On 2015/01/20 23:52:26, Sergey Ulanov wrote: > > On 2015/01/16 23:50:09, xhwang wrote: > > > 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. > > > > 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). > > 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...). > > > > > > > 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. > > > > Given that ffmpeg is the only reason we need this flag (see my comment above) > I > > think it's appropriate to mention it here. > > 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...
xhwang: ping
lgtm % one suggestion Thank you for the patience! https://codereview.chromium.org/805193006/diff/120001/media/base/video_decoder.h File media/base/video_decoder.h (right): https://codereview.chromium.org/805193006/diff/120001/media/base/video_decode... media/base/video_decoder.h:82: // before or after |decode_cb|, but software decoders must call |output_cb| I get what you mean, but this can be complicated. For example, the hardware decoder (GpuVideoDecoder) on Mac can fallback to software mode internally. I suggest you do the same as |low_delay| where VideoDecoderShim assumes FFVD and VPxVD always do this and relax this requirement to a recommendation. But it's up to you :)
https://codereview.chromium.org/805193006/diff/120001/media/base/video_decoder.h File media/base/video_decoder.h (right): https://codereview.chromium.org/805193006/diff/120001/media/base/video_decode... media/base/video_decoder.h:82: // before or after |decode_cb|, but software decoders must call |output_cb| On 2015/01/22 23:52:43, xhwang wrote: > I get what you mean, but this can be complicated. For example, the hardware > decoder (GpuVideoDecoder) on Mac can fallback to software mode internally. > > I suggest you do the same as |low_delay| where VideoDecoderShim assumes FFVD and > VPxVD always do this and relax this requirement to a recommendation. But it's up > to you :) Done.
The CQ bit was checked by sergeyu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/805193006/140001
The CQ bit was unchecked by commit-bot@chromium.org
Exceeded time limit waiting for builds to trigger.
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/be7ac82117cb884b2905a4565e49130d11274d80 Cr-Commit-Position: refs/heads/master@{#312874}
Message was sent while issue was closed.
Committed patchset #6 (id:140001) manually as be7ac82117cb884b2905a4565e49130d11274d80 (presubmit successful). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
