To dropp the decode done callback in RTCVideoDecoderBridgeTv.

To dropp the decode done callback in RTCVideoDecoderBridgeTv.
    
WebRTC measures the decoding time and control the timing of the Decode()
calls. In the current implementation, we call decode done callback when
the browser process gets the frame. In order to get frames constantly
from WebRTC, this fix drops the decode done callback.
    
BUG=264585
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=215797

[wonsik, 2013-07-29 02:33:27]

lgtm % a nit

https://codereview.chromium.org/20646003/diff/1/content/renderer/media/rtc_vi...
File content/renderer/media/rtc_video_decoder_bridge_tv.cc (right):

https://codereview.chromium.org/20646003/diff/1/content/renderer/media/rtc_vi...
content/renderer/media/rtc_video_decoder_bridge_tv.cc:120: void
RTCVideoDecoderBridgeTv::RunDecodeCompleteCallback(int64_t timestamp) {
Since this is used only in RTCVideoDecoderBridgeTv::Decode, you can merge this
method into Decode.

[dwkang1, 2013-07-29 02:55:35]

https://codereview.chromium.org/20646003/diff/1/content/renderer/media/rtc_vi...
File content/renderer/media/rtc_video_decoder_bridge_tv.cc (right):

https://codereview.chromium.org/20646003/diff/1/content/renderer/media/rtc_vi...
content/renderer/media/rtc_video_decoder_bridge_tv.cc:120: void
RTCVideoDecoderBridgeTv::RunDecodeCompleteCallback(int64_t timestamp) {
On 2013/07/29 02:33:28, wonsik wrote:
> Since this is used only in RTCVideoDecoderBridgeTv::Decode, you can merge this
> method into Decode.

Done.

[dwkang1, 2013-07-30 03:24:50]

Found out that Aaron is OOO this week. Ami, could you review this change?
Thanks,

[Ami GONE FROM CHROMIUM, 2013-07-30 03:42:54]

Why is GoogleTV different from other external decoders?
Does your finding in the related bug mean that there's a bug in webrtc's
external decoder support?

(if this CL is correct, then why isn't the correct fix to simply drop the idea
of a decoded callback from webrtc and always assume that Decode() successfully
decoded?)

[dwkang1, 2013-07-30 05:24:17]

On 2013/07/30 03:42:54, Ami Fischman wrote:
> Why is GoogleTV different from other external decoders?

If other external decoder may get frames before its initialization, this will
happen.

> Does your finding in the related bug mean that there's a bug in webrtc's
> external decoder support?

If we use average decode delay instead of max decode delay in
VCMReceiver::FrameForDecoding(),
that can be an alternative, but I don't know about the background of using max
decode delay.
Mikhal, what do you think?

> 
> (if this CL is correct, then why isn't the correct fix to simply drop the idea
> of a decoded callback from webrtc and always assume that Decode() successfully
> decoded?)

As noted in the comment, we calls the callback to make libjingle have statistics
about decoding.

[mikhal, 2013-07-30 20:58:20]

When you call VCMReceiver::FrameForDecoding(), the receiver fetches the next
complete continuous frame and computes the target render time. Eureka should be
using render_timing, and therefore, the receiver will not release frames prior
to their render time. 
The render time is computed based on the frames' timestamp, desired buffer
delay, render delay (const value) and decoding time. The decode time is computed
as a max filter on the decode times, triggered by the DecodeCompleteCallback.
Eureka is using an external HW decoder, and therefore it's not clear to me why
they need to trigger this callback. If this callback is not triggered, the max
MaxDecodeTime() will be 0. What kind of stats is LibJingle tracking? Faking the
timing and then tracking it seems problematic. 

Feel free to ping me if you have any questions,
-Mikhal

On 2013/07/30 05:24:17, dwkang1 wrote:
> On 2013/07/30 03:42:54, Ami Fischman wrote:
> > Why is GoogleTV different from other external decoders?
> 
> If other external decoder may get frames before its initialization, this will
> happen.
> 
> > Does your finding in the related bug mean that there's a bug in webrtc's
> > external decoder support?
> 
> If we use average decode delay instead of max decode delay in
> VCMReceiver::FrameForDecoding(),
> that can be an alternative, but I don't know about the background of using max
> decode delay.
> Mikhal, what do you think?
> 
> > 
> > (if this CL is correct, then why isn't the correct fix to simply drop the
idea
> > of a decoded callback from webrtc and always assume that Decode()
successfully
> > decoded?)
> 
> As noted in the comment, we calls the callback to make libjingle have
statistics
> about decoding.

[Ami GONE FROM CHROMIUM, 2013-07-30 21:18:00]

Dongwon: is the real problem just that the HW initialization time is
getting counted against the decode time of the first few frames, and then
webrtc propagates that high time forever?  If that's the case then maybe
the right fix is to only call Decoded() after HW init is done?

Mikhal: does it make sense to build in a decay of some sort so that a
single long decode interval from the distant pass doesn't affect current
decisions?  (i.e. maybe the max filter should be limited to some recent
interval instead of all time)

[Ami GONE FROM CHROMIUM, 2013-07-30 21:18:23]

Dongwon: is the real problem just that the HW initialization time is
getting counted against the decode time of the first few frames, and then
webrtc propagates that high time forever?  If that's the case then maybe
the right fix is to only call Decoded() after HW init is done?

Mikhal: does it make sense to build in a decay of some sort so that a
single long decode interval from the distant pass doesn't affect current
decisions?  (i.e. maybe the max filter should be limited to some recent
interval instead of all time)

[mikhal, 2013-07-30 21:31:55]

One more point regarding initialization: The decoder should not process
frames if not initialized, see:
https://code.google.com/p/webrtc/source/browse/trunk/webrtc/modules/video_cod...
The DecodeTime is already filtered, so I don't think we should add more
filters there. I'm still not convinced that this should be used at all in
this case.
Another question: Is this problem new? I don't remember this coming up when
we were previously looking at the timing.


On Tue, Jul 30, 2013 at 2:16 PM, Ami Fischman <fischman@chromium.org> wrote:

> Dongwon: is the real problem just that the HW initialization time is
> getting counted against the decode time of the first few frames, and then
> webrtc propagates that high time forever?  If that's the case then maybe
> the right fix is to only call Decoded() after HW init is done?
>
> Mikhal: does it make sense to build in a decay of some sort so that a
> single long decode interval from the distant pass doesn't affect current
> decisions?  (i.e. maybe the max filter should be limited to some recent
> interval instead of all time)
>

[dwkang1, 2013-07-31 11:34:26]

As Mikhal said, webrtc has a window (20sec) for calculating max decode time,
ans that's why we can see the low fps exactly 20 sec later from the initial peak
in the session_detail_base.html in crbug/264585. The low fps happens when the
long decode time
sample is removed from the window.

Regarding stats, it's related to "googFrameRateDecoded".
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjin...
If dropping decode done callbacks makes the max decode time 0, I think it will
resolve this issue also.
Actually, the callback was added because testing team wanted to get decode fps
via webrtc stat log, but I didn't know this have this side effect.
Because we have a tool for measuring real presentation fps now, the stat may not
be needed anymore.
I'll check testing team and update the change. Let me know if you have any
comments on this direction.

(Regarding initialization, the current implementation returns InitDecode() early
and gets
frames before pipeline is ready. And, once the pipeline start, it consumes
frames as fast
as it can to catch up and this lead to the initial peak in terms of fps. Both
dropping callback
and making InitDecode() wait until pipeline is ready are meaningful. I will
prepare a separate patch for this. b/10101341)

Thanks,

On 2013/07/30 21:31:55, mikhal wrote:
> One more point regarding initialization: The decoder should not process
> frames if not initialized, see:
>
https://code.google.com/p/webrtc/source/browse/trunk/webrtc/modules/video_cod...
> The DecodeTime is already filtered, so I don't think we should add more
> filters there. I'm still not convinced that this should be used at all in
> this case.
> Another question: Is this problem new? I don't remember this coming up when
> we were previously looking at the timing.
> 
> 
> On Tue, Jul 30, 2013 at 2:16 PM, Ami Fischman <mailto:fischman@chromium.org>
wrote:
> 
> > Dongwon: is the real problem just that the HW initialization time is
> > getting counted against the decode time of the first few frames, and then
> > webrtc propagates that high time forever?  If that's the case then maybe
> > the right fix is to only call Decoded() after HW init is done?
> >
> > Mikhal: does it make sense to build in a decay of some sort so that a
> > single long decode interval from the distant pass doesn't affect current
> > decisions?  (i.e. maybe the max filter should be limited to some recent
> > interval instead of all time)
> >

[mikhal, 2013-07-31 16:07:32]

I agree with the change of removing the callback.
Not sure I follow on the init issue: The JB will release the frames as soon
as possible, If the decoder falls behind, it will need to speed up in order
to catch up with render times. Note that the decoder should always start
with a key frame.  I don't understand why all the components can't start
simultaneously.


On Wed, Jul 31, 2013 at 4:34 AM, <dwkang@chromium.org> wrote:

> As Mikhal said, webrtc has a window (20sec) for calculating max decode
> time,
> ans that's why we can see the low fps exactly 20 sec later from the
> initial peak
> in the session_detail_base.html in crbug/264585. The low fps happens when
> the
> long decode time
> sample is removed from the window.
>
> Regarding stats, it's related to "googFrameRateDecoded".
> https://code.google.com/p/**chromium/codesearch#chromium/**
>
src/third_party/libjingle/**source/talk/app/webrtc/**statstypes.h&l=130<https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjingle/source/talk/app/webrtc/statstypes.h&l=130>
> If dropping decode done callbacks makes the max decode time 0, I think it
> will
> resolve this issue also.
> Actually, the callback was added because testing team wanted to get decode
> fps
> via webrtc stat log, but I didn't know this have this side effect.
> Because we have a tool for measuring real presentation fps now, the stat
> may not
> be needed anymore.
> I'll check testing team and update the change. Let me know if you have any
> comments on this direction.
>
> (Regarding initialization, the current implementation returns InitDecode()
> early
> and gets
> frames before pipeline is ready. And, once the pipeline start, it consumes
> frames as fast
> as it can to catch up and this lead to the initial peak in terms of fps.
> Both
> dropping callback
> and making InitDecode() wait until pipeline is ready are meaningful. I will
> prepare a separate patch for this. b/10101341)
>
> Thanks,
>
>
> On 2013/07/30 21:31:55, mikhal wrote:
>
>> One more point regarding initialization: The decoder should not process
>> frames if not initialized, see:
>>
>
> https://code.google.com/p/**webrtc/source/browse/trunk/**
>
webrtc/modules/video_coding/**codecs/vp8/vp8_impl.cc#585<https://code.google.com/p/webrtc/source/browse/trunk/webrtc/modules/video_coding/codecs/vp8/vp8_impl.cc#585>
>
>> The DecodeTime is already filtered, so I don't think we should add more
>> filters there. I'm still not convinced that this should be used at all in
>> this case.
>> Another question: Is this problem new? I don't remember this coming up
>> when
>> we were previously looking at the timing.
>>
>
>
>  On Tue, Jul 30, 2013 at 2:16 PM, Ami Fischman <mailto:
>> fischman@chromium.org>
>>
> wrote:
>
>  > Dongwon: is the real problem just that the HW initialization time is
>> > getting counted against the decode time of the first few frames, and
>> then
>> > webrtc propagates that high time forever?  If that's the case then maybe
>> > the right fix is to only call Decoded() after HW init is done?
>> >
>> > Mikhal: does it make sense to build in a decay of some sort so that a
>> > single long decode interval from the distant pass doesn't affect current
>> > decisions?  (i.e. maybe the max filter should be limited to some recent
>> > interval instead of all time)
>> >
>>
>
>
>
https://chromiumcodereview.**appspot.com/20646003/<https://chromiumcodereview...
>

[dwkang1, 2013-08-01 02:51:46]

The decode done callback is dropped in the new patch set. PTAL.

On 2013/07/31 16:07:32, mikhal wrote:
> I agree with the change of removing the callback.
> Not sure I follow on the init issue: The JB will release the frames as soon
> as possible, If the decoder falls behind, it will need to speed up in order
> to catch up with render times. Note that the decoder should always start
> with a key frame.  I don't understand why all the components can't start
> simultaneously.

In the current implementation, WMPAndroid, which includes the decode pipeline,
is created and initialized in a different thread some time after
VideoDecoder::InitDecode() is called.
For the details of GTV's hw decoding implementation, you may want to see
https://docs.google.com/a/google.com/document/d/1ePRSyMmBtgdTttC_F4g_R40RIJkB...
I agree that this doesn't look good and may have potential issues, I am thinking
of making RTCVideoDecoderBridgeTv::InitDecode() return only when WMPAndroid
start requesting frames.
Let me know if you have any concern on this. Thanks,

> 
> 
> On Wed, Jul 31, 2013 at 4:34 AM, <mailto:dwkang@chromium.org> wrote:
> 
> > As Mikhal said, webrtc has a window (20sec) for calculating max decode
> > time,
> > ans that's why we can see the low fps exactly 20 sec later from the
> > initial peak
> > in the session_detail_base.html in crbug/264585. The low fps happens when
> > the
> > long decode time
> > sample is removed from the window.
> >
> > Regarding stats, it's related to "googFrameRateDecoded".
> > https://code.google.com/p/**chromium/codesearch#chromium/**
> >
>
src/third_party/libjingle/**source/talk/app/webrtc/**statstypes.h&l=130<https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjingle/source/talk/app/webrtc/statstypes.h&l=130>
> > If dropping decode done callbacks makes the max decode time 0, I think it
> > will
> > resolve this issue also.
> > Actually, the callback was added because testing team wanted to get decode
> > fps
> > via webrtc stat log, but I didn't know this have this side effect.
> > Because we have a tool for measuring real presentation fps now, the stat
> > may not
> > be needed anymore.
> > I'll check testing team and update the change. Let me know if you have any
> > comments on this direction.
> >
> > (Regarding initialization, the current implementation returns InitDecode()
> > early
> > and gets
> > frames before pipeline is ready. And, once the pipeline start, it consumes
> > frames as fast
> > as it can to catch up and this lead to the initial peak in terms of fps.
> > Both
> > dropping callback
> > and making InitDecode() wait until pipeline is ready are meaningful. I will
> > prepare a separate patch for this. b/10101341)
> >
> > Thanks,
> >
> >
> > On 2013/07/30 21:31:55, mikhal wrote:
> >
> >> One more point regarding initialization: The decoder should not process
> >> frames if not initialized, see:
> >>
> >
> > https://code.google.com/p/**webrtc/source/browse/trunk/**
> >
>
webrtc/modules/video_coding/**codecs/vp8/vp8_impl.cc#585<https://code.google.com/p/webrtc/source/browse/trunk/webrtc/modules/video_coding/codecs/vp8/vp8_impl.cc#585>
> >
> >> The DecodeTime is already filtered, so I don't think we should add more
> >> filters there. I'm still not convinced that this should be used at all in
> >> this case.
> >> Another question: Is this problem new? I don't remember this coming up
> >> when
> >> we were previously looking at the timing.
> >>
> >
> >
> >  On Tue, Jul 30, 2013 at 2:16 PM, Ami Fischman <mailto:
> >> mailto:fischman@chromium.org>
> >>
> > wrote:
> >
> >  > Dongwon: is the real problem just that the HW initialization time is
> >> > getting counted against the decode time of the first few frames, and
> >> then
> >> > webrtc propagates that high time forever?  If that's the case then maybe
> >> > the right fix is to only call Decoded() after HW init is done?
> >> >
> >> > Mikhal: does it make sense to build in a decay of some sort so that a
> >> > single long decode interval from the distant pass doesn't affect current
> >> > decisions?  (i.e. maybe the max filter should be limited to some recent
> >> > interval instead of all time)
> >> >
> >>
> >
> >
> >
>
https://chromiumcodereview.**appspot.com/20646003/%3Chttps://chromiumcoderevi...>
> >

[Ami GONE FROM CHROMIUM, 2013-08-01 19:57:32]

I believe the CL now does what you want it to do, so I'm ok with saying LGTM.
However, I suspect that you don't really want to drop the callback, because the
facility really will be useful, and what you really want is to delay
InitDecoder() returning until the decoder is in fact initialized.

https://chromiumcodereview.appspot.com/20646003/diff/20001/content/renderer/m...
File content/renderer/media/rtc_video_decoder_bridge_tv.h (right):

https://chromiumcodereview.appspot.com/20646003/diff/20001/content/renderer/m...
content/renderer/media/rtc_video_decoder_bridge_tv.h:1: // Copyright 2013 The
Chromium Authors. All rights reserved.
The CL description needs a rewrite.

https://chromiumcodereview.appspot.com/20646003/diff/20001/content/renderer/m...
content/renderer/media/rtc_video_decoder_bridge_tv.h:48:
webrtc::DecodedImageCallback* decode_complete_callback_;
delete

[dwkang1, 2013-08-02 01:55:30]

Thanks for the review all.

I checked CloudView & Dashboard team does not use the stat anymore. So, I am
okay with dropping the callback.

In summary, there are three types of symptoms I am seeing.
(the fps graphs are included in crbug.com/264585)
1) Initial peak in term of fps. This is because the initialization issue
discussed here. b/10101341
2) Sometimes we see some peaks in terms of fps during mirroring. This is because
the frame request from browser process to renderer process takes ~200ms
sometimes. Need more investigation why this happens. b/9763055
3) Once 1) or 2) happens, we can see low fps after 20sec later. This is because
the max decode time discussed here. (This symptom will be removed with this
patch.)

Let me know if you have any concerns. I'll hold committing until your response.
Thanks!

https://chromiumcodereview.appspot.com/20646003/diff/20001/content/renderer/m...
File content/renderer/media/rtc_video_decoder_bridge_tv.h (right):

https://chromiumcodereview.appspot.com/20646003/diff/20001/content/renderer/m...
content/renderer/media/rtc_video_decoder_bridge_tv.h:1: // Copyright 2013 The
Chromium Authors. All rights reserved.
On 2013/08/01 19:57:32, Ami Fischman wrote:
> The CL description needs a rewrite.

Done.

https://chromiumcodereview.appspot.com/20646003/diff/20001/content/renderer/m...
content/renderer/media/rtc_video_decoder_bridge_tv.h:48:
webrtc::DecodedImageCallback* decode_complete_callback_;
On 2013/08/01 19:57:32, Ami Fischman wrote:
> delete

Done.

[Ami GONE FROM CHROMIUM, 2013-08-02 18:04:34]

On Thu, Aug 1, 2013 at 6:55 PM, <dwkang@chromium.org> wrote:

> Let me know if you have any concerns. I'll hold committing until your
> response.
>

AFAICT you have a bug you do not understand (b/9763055) and it has a
symptom and you're proposing to resolve the symptom in a way that will
directly affect your ability to diagnose the bug, so that seems like a
strange way to go, to me.  Your choice, though.

[dwkang1, 2013-08-06 01:09:44]

I discussed with the team, and decided to have this change because
1. this is not harmful and 2. this does not hide the b/9763055.
(IOW, I still see the symptoms even after applying this patch.)
Thanks,

On 2013/08/02 18:04:34, Ami Fischman wrote:
> On Thu, Aug 1, 2013 at 6:55 PM, <mailto:dwkang@chromium.org> wrote:
> 
> > Let me know if you have any concerns. I'll hold committing until your
> > response.
> >
> 
> AFAICT you have a bug you do not understand (b/9763055) and it has a
> symptom and you're proposing to resolve the symptom in a way that will
> directly affect your ability to diagnose the bug, so that seems like a
> strange way to go, to me.  Your choice, though.

[commit-bot: I haz the power, 2013-08-06 01:12:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dwkang@chromium.org/20646003/31001

[commit-bot: I haz the power, 2013-08-06 05:31:37]

Change committed as 215797