Add DecodedImageCallback::Decoded() function with custom decode time value.

Add DecodedImageCallback::Decoded() function with custom decode time value.
On Android, we would like to use MediaCodec output buffers to hold decoded frames until they can be rendered to a texture. There can only be one texture buffer used at the same time and therefore the calculated decode time in VCMTiming will be wrong since that calculation will also include the time where the decoder waited for the upper layers (that depend on network jitter and actual render time) to release the frame.

This new method will be used in
https://codereview.webrtc.org/1422963003/

BUG=webrtc:4993
R=stefan@webrtc.org
TBR=mflodman@webrtc.org

Committed: https://crrev.com/327d8babc81913410dbb3125a62be3ff0e46e9f4
Cr-Commit-Position: refs/heads/master@{#10576}

[perkj_webrtc, 2015-11-02 13:06:14]

perkj@webrtc.org changed reviewers:
+ stefan@webrtc.org

[perkj_webrtc, 2015-11-02 13:06:15]

Can you please review?

[perkj_webrtc, 2015-11-03 11:36:07]

perkj@webrtc.org changed reviewers:
+ mflodman@webrtc.org

[perkj_webrtc, 2015-11-03 11:36:08]

Adding Magnus since Stefan is ooo.

[stefan-webrtc, 2015-11-03 11:43:29]

stefan@webrtc.org changed reviewers:
- mflodman@webrtc.org

[stefan-webrtc, 2015-11-03 11:43:30]

When magjed and I discussed this a while back, we talked about two options to
this which I think would be nicer:
1. Have the frame that you get from the decoder implement VideoFrame and pass it
out when it finished decoding, and let the renderer be responsible for copying
it onto the texture. If I remember correctly the problem was that you only have
one texture buffer but multiple android specific frame buffers, right? That was
at least how magjed explained it to me.

2. The second option would be to make the decoder a combined decoder and
renderer, and not have it pass out any frames.

Did you consider these options? Before going with this solution?

Btw, I'm at the IETF this week so I may be slow on replying. If you want faster
replies you may want to add mflodman.

https://codereview.webrtc.org/1414693006/diff/1/webrtc/modules/video_coding/m...
File webrtc/modules/video_coding/main/source/generic_decoder.h (right):

https://codereview.webrtc.org/1414693006/diff/1/webrtc/modules/video_coding/m...
webrtc/modules/video_coding/main/source/generic_decoder.h:44: virtual int32_t
Decoded2(VideoFrame& decodedImage, int32_t decode_time_ms);
Decoded2 isn't a very good function name... DecodedWithDecodeTime()? And make
the decode_time_ms int64_t, since webrtc in general uses int64_t to represent
time.

[perkj_webrtc, 2015-11-03 12:07:54]

perkj@webrtc.org changed reviewers:
+ mflodman@webrtc.org

[perkj_webrtc, 2015-11-03 12:17:22]

On 2015/11/03 11:43:30, stefan-webrtc (holmer) wrote:
> When magjed and I discussed this a while back, we talked about two options to
> this which I think would be nicer:
> 1. Have the frame that you get from the decoder implement VideoFrame and pass
it
> out when it finished decoding, and let the renderer be responsible for copying
> it onto the texture. If I remember correctly the problem was that you only
have
> one texture buffer but multiple android specific frame buffers, right? That
was
> at least how magjed explained it to me.
> 

The decoder is the only one that can render the output buffer to a Surface
(texture) but it can only renderer to one texture....
We can do texture copies but that is quite costly.

> 2. The second option would be to make the decoder a combined decoder and
> renderer, and not have it pass out any frames.
> 

That is only possible if there is only one consumer. Ie, we would not be able to
both render and record or forward the frame. 
I don't like that since it would limit how you can use a remote track. 


> Did you consider these options? Before going with this solution?
> 
> Btw, I'm at the IETF this week so I may be slow on replying. If you want
faster
> replies you may want to add mflodman.
> 
>
https://codereview.webrtc.org/1414693006/diff/1/webrtc/modules/video_coding/m...
> File webrtc/modules/video_coding/main/source/generic_decoder.h (right):
> 
>
https://codereview.webrtc.org/1414693006/diff/1/webrtc/modules/video_coding/m...
> webrtc/modules/video_coding/main/source/generic_decoder.h:44: virtual int32_t
> Decoded2(VideoFrame& decodedImage, int32_t decode_time_ms);
> Decoded2 isn't a very good function name... DecodedWithDecodeTime()? And make
> the decode_time_ms int64_t, since webrtc in general uses int64_t to represent
> time.

[stefan-webrtc, 2015-11-03 23:25:10]

On 2015/11/03 12:17:22, perkj1 wrote:
> On 2015/11/03 11:43:30, stefan-webrtc (holmer) wrote:
> > When magjed and I discussed this a while back, we talked about two options
to
> > this which I think would be nicer:
> > 1. Have the frame that you get from the decoder implement VideoFrame and
pass
> it
> > out when it finished decoding, and let the renderer be responsible for
copying
> > it onto the texture. If I remember correctly the problem was that you only
> have
> > one texture buffer but multiple android specific frame buffers, right? That
> was
> > at least how magjed explained it to me.
> > 
> 
> The decoder is the only one that can render the output buffer to a Surface
> (texture) but it can only renderer to one texture....
> We can do texture copies but that is quite costly.

Ok, my understanding was that the decoder had multiple buffers, and if we could
pass them out of the webrtc::VideoDecoder we could wait with copying from the
decoder buffer over to a texture until it's time to render. Is that not
possible? I don't have a lot of details here, so I'm not sure if that solution
would be clean, but I like the fact that it would better match the intended
interface.

> 
> > 2. The second option would be to make the decoder a combined decoder and
> > renderer, and not have it pass out any frames.
> > 
> 
> That is only possible if there is only one consumer. Ie, we would not be able
to
> both render and record or forward the frame. 
> I don't like that since it would limit how you can use a remote track. 

That is a good point.

> 
> 
> > Did you consider these options? Before going with this solution?
> > 
> > Btw, I'm at the IETF this week so I may be slow on replying. If you want
> faster
> > replies you may want to add mflodman.
> > 
> >
>
https://codereview.webrtc.org/1414693006/diff/1/webrtc/modules/video_coding/m...
> > File webrtc/modules/video_coding/main/source/generic_decoder.h (right):
> > 
> >
>
https://codereview.webrtc.org/1414693006/diff/1/webrtc/modules/video_coding/m...
> > webrtc/modules/video_coding/main/source/generic_decoder.h:44: virtual
int32_t
> > Decoded2(VideoFrame& decodedImage, int32_t decode_time_ms);
> > Decoded2 isn't a very good function name... DecodedWithDecodeTime()? And
make
> > the decode_time_ms int64_t, since webrtc in general uses int64_t to
represent
> > time.

[perkj_webrtc, 2015-11-06 15:31:12]

Can you please take another look? Also see email discussion on why.

https://codereview.webrtc.org/1414693006/diff/1/webrtc/modules/video_coding/m...
File webrtc/modules/video_coding/main/source/generic_decoder.h (right):

https://codereview.webrtc.org/1414693006/diff/1/webrtc/modules/video_coding/m...
webrtc/modules/video_coding/main/source/generic_decoder.h:44: virtual int32_t
Decoded2(VideoFrame& decodedImage, int32_t decode_time_ms);
On 2015/11/03 11:43:29, stefan-webrtc (holmer) wrote:
> Decoded2 isn't a very good function name... DecodedWithDecodeTime()? And make
> the decode_time_ms int64_t, since webrtc in general uses int64_t to represent
> time.

Only Decoded(.....) ?

[perkj_webrtc, 2015-11-10 07:17:34]

ping!

[stefan-webrtc, 2015-11-10 10:32:00]

https://codereview.webrtc.org/1414693006/diff/20001/webrtc/video_decoder.h
File webrtc/video_decoder.h (right):

https://codereview.webrtc.org/1414693006/diff/20001/webrtc/video_decoder.h#ne...
webrtc/video_decoder.h:39: }
Are there many places where you have to make this change if you don't provide a
default implementation? I would have preferred that if it's reasonable to avoid.

Maybe we can follow-up with removing this default impl at least? In that case
add a TODO.

[stefan-webrtc, 2015-11-10 11:37:28]

On 2015/11/10 10:32:00, stefan-webrtc (holmer) wrote:
> https://codereview.webrtc.org/1414693006/diff/20001/webrtc/video_decoder.h
> File webrtc/video_decoder.h (right):
> 
>
https://codereview.webrtc.org/1414693006/diff/20001/webrtc/video_decoder.h#ne...
> webrtc/video_decoder.h:39: }
> Are there many places where you have to make this change if you don't provide
a
> default implementation? I would have preferred that if it's reasonable to
avoid.
> 
> Maybe we can follow-up with removing this default impl at least? In that case
> add a TODO.

besides this I'm ok with this after the offline discussions. lgtm

[perkj_webrtc, 2015-11-10 12:05:19]

On 2015/11/10 11:37:28, stefan-webrtc (holmer) wrote:
> On 2015/11/10 10:32:00, stefan-webrtc (holmer) wrote:
> > https://codereview.webrtc.org/1414693006/diff/20001/webrtc/video_decoder.h
> > File webrtc/video_decoder.h (right):
> > 
> >
>
https://codereview.webrtc.org/1414693006/diff/20001/webrtc/video_decoder.h#ne...
> > webrtc/video_decoder.h:39: }
> > Are there many places where you have to make this change if you don't
provide
> a
> > default implementation? I would have preferred that if it's reasonable to
> avoid.
> > 
> > Maybe we can follow-up with removing this default impl at least? In that
case
> > add a TODO.
> 
> besides this I'm ok with this after the offline discussions. lgtm

Can you please ptal?

[stefan-webrtc, 2015-11-10 12:16:32]

A few nits, otherwise lgtm.

https://codereview.webrtc.org/1414693006/diff/40001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/codecs/vp8/simulcast_unittest.h (right):

https://codereview.webrtc.org/1414693006/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/vp8/simulcast_unittest.h:142: return
Decoded(decoded_image);
Just return -1.

https://codereview.webrtc.org/1414693006/diff/40001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/codecs/vp8/test/vp8_impl_unittest.cc (right):

https://codereview.webrtc.org/1414693006/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/vp8/test/vp8_impl_unittest.cc:83: int32_t
Decoded(VideoFrame& frame, int64_t decode_time_ms) override {
Why does one have webrtc::VideoFrame and the other VideoFrame?

https://codereview.webrtc.org/1414693006/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/vp8/test/vp8_impl_unittest.cc:85: return
Decoded(frame);
return -1 here too, and in the other places we don't support this method.

[perkj_webrtc, 2015-11-10 12:50:04]

The CQ bit was checked by perkj@webrtc.org

[perkj_webrtc, 2015-11-10 12:50:04]

The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1414693006/#ps60001 (title:
"addressed comments.")

[perkj_webrtc, 2015-11-10 12:50:16]

https://codereview.webrtc.org/1414693006/diff/40001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/codecs/vp8/test/vp8_impl_unittest.cc (right):

https://codereview.webrtc.org/1414693006/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/vp8/test/vp8_impl_unittest.cc:83: int32_t
Decoded(VideoFrame& frame, int64_t decode_time_ms) override {
On 2015/11/10 12:16:32, stefan-webrtc (holmer) wrote:
> Why does one have webrtc::VideoFrame and the other VideoFrame?

Done.

https://codereview.webrtc.org/1414693006/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/codecs/vp8/test/vp8_impl_unittest.cc:85: return
Decoded(frame);
On 2015/11/10 12:16:32, stefan-webrtc (holmer) wrote:
> return -1 here too, and in the other places we don't support this method.

Done.

[commit-bot: I haz the power, 2015-11-10 12:50:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1414693006/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1414693006/60001

[commit-bot: I haz the power, 2015-11-10 12:52:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-10 12:52:49]

Try jobs failed on following builders:
  presubmit on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/1656)

[perkj_webrtc, 2015-11-10 13:00:14]

Description was changed from

==========
Add DecodedImageCallback::Decoded() function with custom decode time value.
On Android, we would like to use MediaCodec output buffers to hold decoded
frames until they can be rendered to a texture. There can only be one texture
buffer used at the same time and therefore the calculated decode time in
VCMTiming will be wrong since that calculation will also include the time where
the decoder waited for the upper layers (that depend on network jitter and
actual render time) to release the frame.

This new method will be used in
https://codereview.webrtc.org/1422963003/

BUG=webrtc:4993
==========

to

==========
Add DecodedImageCallback::Decoded() function with custom decode time value.
On Android, we would like to use MediaCodec output buffers to hold decoded
frames until they can be rendered to a texture. There can only be one texture
buffer used at the same time and therefore the calculated decode time in
VCMTiming will be wrong since that calculation will also include the time where
the decoder waited for the upper layers (that depend on network jitter and
actual render time) to release the frame.

This new method will be used in
https://codereview.webrtc.org/1422963003/

BUG=webrtc:4993
TBR=mflodman@webrtc.org
==========

[perkj_webrtc, 2015-11-10 13:00:50]

Committed patchset #4 (id:60001) manually as
327d8babc81913410dbb3125a62be3ff0e46e9f4 (presubmit successful).

[commit-bot: I haz the power, 2015-11-10 13:00:53]

Patchset 4 (id:??) landed as
https://crrev.com/327d8babc81913410dbb3125a62be3ff0e46e9f4
Cr-Commit-Position: refs/heads/master@{#10576}