Wire up VCMTiming in FrameBuffer2.

Wire up VCMTiming in FrameBuffer2.

The VCMTiming class is now used to set the render time for frames.

BUG=webrtc:5514
R=stefan@webrtc.org

Committed: https://crrev.com/4f6cd6ac59eab8d0202eea9059371403604e4f6a
Cr-Commit-Position: refs/heads/master@{#13621}

[philipel, 2016-07-12 10:51:38]

philipel@webrtc.org changed reviewers:
+ sprang@webrtc.org, stefan@webrtc.org

[stefan-webrtc, 2016-07-12 16:55:39]

Please add unittests to verify that render time works as it should.

https://codereview.webrtc.org/2138873003/diff/1/webrtc/modules/video_coding/f...
File webrtc/modules/video_coding/frame_buffer2.cc (right):

https://codereview.webrtc.org/2138873003/diff/1/webrtc/modules/video_coding/f...
webrtc/modules/video_coding/frame_buffer2.cc:95: // If the timout occures,
return. Otherwise a new frame has been inserted
If the timeout occurs

https://codereview.webrtc.org/2138873003/diff/1/webrtc/modules/video_coding/f...
webrtc/modules/video_coding/frame_buffer2.cc:116: return frame;
Should we update the render time of this frame before we return it? The render
time could be stale if we have been waiting.

https://codereview.webrtc.org/2138873003/diff/1/webrtc/modules/video_coding/f...
webrtc/modules/video_coding/frame_buffer2.cc:137: // If |newest_picture_id_ is
-1 then this is the first frame we received.
|newest_picture_id_|

[philipel, 2016-07-13 09:21:50]

What kind of unittests are you thinking of? I don't want to write tests for the
VCMTiming class, and there are already tests that test that the FrameBuffer
works as expected given certain render timestamps.

https://codereview.webrtc.org/2138873003/diff/1/webrtc/modules/video_coding/f...
File webrtc/modules/video_coding/frame_buffer2.cc (right):

https://codereview.webrtc.org/2138873003/diff/1/webrtc/modules/video_coding/f...
webrtc/modules/video_coding/frame_buffer2.cc:95: // If the timout occures,
return. Otherwise a new frame has been inserted
On 2016/07/12 16:55:38, stefan-webrtc (holmer) wrote:
> If the timeout occurs

Done.

https://codereview.webrtc.org/2138873003/diff/1/webrtc/modules/video_coding/f...
webrtc/modules/video_coding/frame_buffer2.cc:116: return frame;
On 2016/07/12 16:55:39, stefan-webrtc (holmer) wrote:
> Should we update the render time of this frame before we return it? The render
> time could be stale if we have been waiting.

It could? If that's the case then it's going to be very hard to know for how
long we should wait before returning...

https://codereview.webrtc.org/2138873003/diff/1/webrtc/modules/video_coding/f...
webrtc/modules/video_coding/frame_buffer2.cc:137: // If |newest_picture_id_ is
-1 then this is the first frame we received.
On 2016/07/12 16:55:38, stefan-webrtc (holmer) wrote:
> |newest_picture_id_|

Done.

[stefan-webrtc, 2016-07-14 12:17:30]

https://codereview.webrtc.org/2138873003/diff/1/webrtc/modules/video_coding/f...
File webrtc/modules/video_coding/frame_buffer2.cc (right):

https://codereview.webrtc.org/2138873003/diff/1/webrtc/modules/video_coding/f...
webrtc/modules/video_coding/frame_buffer2.cc:116: return frame;
On 2016/07/13 09:21:49, philipel wrote:
> On 2016/07/12 16:55:39, stefan-webrtc (holmer) wrote:
> > Should we update the render time of this frame before we return it? The
render
> > time could be stale if we have been waiting.
> 
> It could? If that's the case then it's going to be very hard to know for how
> long we should wait before returning...

It can since the inter arrival class might have updated, or the decode time may
have changed.

Check this code for an idea of how it can be done:
https://cs.corp.google.com/piper///depot/google3/third_party/webrtc/files/sta...

I think we're handling it there?

[philipel, 2016-07-14 15:39:39]

https://codereview.webrtc.org/2138873003/diff/1/webrtc/modules/video_coding/f...
File webrtc/modules/video_coding/frame_buffer2.cc (right):

https://codereview.webrtc.org/2138873003/diff/1/webrtc/modules/video_coding/f...
webrtc/modules/video_coding/frame_buffer2.cc:116: return frame;
On 2016/07/14 12:17:30, stefan-webrtc (holmer) wrote:
> On 2016/07/13 09:21:49, philipel wrote:
> > On 2016/07/12 16:55:39, stefan-webrtc (holmer) wrote:
> > > Should we update the render time of this frame before we return it? The
> render
> > > time could be stale if we have been waiting.
> > 
> > It could? If that's the case then it's going to be very hard to know for how
> > long we should wait before returning...
> 
> It can since the inter arrival class might have updated, or the decode time
may
> have changed.
> 
> Check this code for an idea of how it can be done:
>
https://cs.corp.google.com/piper///depot/google3/third_party/webrtc/files/sta...
> 
> I think we're handling it there?

Yes and no. That part of the code only waits for as long as possible before
returning a NULL frame in order to avoid a busy loop.

Since the max wait time is 50 ms (defined here
https://cs.chromium.org/chromium/src/third_party/webrtc/video/video_receive_s...)
it is possible that it returns a NULL frame instead of the next frame and the
next time this function is called the rendertime is updated.

It is also possible that the wait time is shorter than the 50 ms, in which case
it will wait for |wait_time_ms| and then still use the render time that was
calculated before the wait occurred.

So if the wait is longer than 50 ms then the render time will be updated,
otherwise it will not.

I guess this is a coarse way to continuously update the render time, but I
wonder if it is necessary or if we gain anything by doing so.

[philipel, 2016-08-01 10:45:54]

ping

[stefan-webrtc, 2016-08-01 11:04:04]

https://codereview.webrtc.org/2138873003/diff/20001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/frame_buffer2.cc (right):

https://codereview.webrtc.org/2138873003/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/frame_buffer2.cc:109:
timing_->SetJitterDelay(jitter_estimator_->GetJitterEstimate(1.0));
Do we need to add an rtt multiplier here to support the case when we have FEC? I
know it would be temporary and likely removed when we have improved the jitter
estimator, but I think we should keep that functionality until then.

https://codereview.webrtc.org/2138873003/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/frame_buffer2.cc:140:
inter_frame_delay_.Reset(clock_->TimeInMilliseconds());
Shouldn't this already be reset in the beginning when we haven't received any
frames?

[philipel, 2016-08-02 09:25:59]

https://codereview.webrtc.org/2138873003/diff/20001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/frame_buffer2.cc (right):

https://codereview.webrtc.org/2138873003/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/frame_buffer2.cc:109:
timing_->SetJitterDelay(jitter_estimator_->GetJitterEstimate(1.0));
On 2016/08/01 11:04:04, stefan-webrtc (holmer) wrote:
> Do we need to add an rtt multiplier here to support the case when we have FEC?
I
> know it would be temporary and likely removed when we have improved the jitter
> estimator, but I think we should keep that functionality until then.

Added protection mode.

https://codereview.webrtc.org/2138873003/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/frame_buffer2.cc:140:
inter_frame_delay_.Reset(clock_->TimeInMilliseconds());
On 2016/08/01 11:04:04, stefan-webrtc (holmer) wrote:
> Shouldn't this already be reset in the beginning when we haven't received any
> frames?

Yepp, removed.

[stefan-webrtc, 2016-08-02 11:54:40]

lg

https://codereview.webrtc.org/2138873003/diff/40001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/frame_buffer2.cc (right):

https://codereview.webrtc.org/2138873003/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/frame_buffer2.cc:110: float rtt_mult =
protection_mode_ == kProtectionNackFEC ? 0.0 : 1.0;
I think it'd be good to verify this with a unittest too.

[philipel, 2016-08-02 14:05:45]

https://codereview.webrtc.org/2138873003/diff/40001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/frame_buffer2.cc (right):

https://codereview.webrtc.org/2138873003/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/frame_buffer2.cc:110: float rtt_mult =
protection_mode_ == kProtectionNackFEC ? 0.0 : 1.0;
On 2016/08/02 11:54:40, stefan-webrtc (holmer) wrote:
> I think it'd be good to verify this with a unittest too.

Done.

[stefan-webrtc, 2016-08-02 14:19:03]

lgtm

https://codereview.webrtc.org/2138873003/diff/60001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/frame_buffer2_unittest.cc (right):

https://codereview.webrtc.org/2138873003/diff/60001/webrtc/modules/video_codi...
webrtc/modules/video_coding/frame_buffer2_unittest.cc:73:
MOCK_METHOD1(GetJitterEstimate, int(double rttMultiplier));
rtt_multiplier

[philipel, 2016-08-02 14:33:21]

The CQ bit was checked by philipel@webrtc.org

[commit-bot: I haz the power, 2016-08-02 14:33:31]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-08-02 14:44:00]

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

[commit-bot: I haz the power, 2016-08-02 14:44:02]

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

[philipel, 2016-08-03 08:59:51]

Description was changed from

==========
Wire up VCMTiming in FrameBuffer2.

The VCMTiming class is now used to set the render time for frames.

BUG=webrtc:5514
==========

to

==========
Wire up VCMTiming in FrameBuffer2.

The VCMTiming class is now used to set the render time for frames.

BUG=webrtc:5514
R=stefan@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/4f6cd6ac59eab8d0202eea905...
==========

[philipel, 2016-08-03 08:59:54]

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

[commit-bot: I haz the power, 2016-08-03 08:59:56]

Description was changed from

==========
Wire up VCMTiming in FrameBuffer2.

The VCMTiming class is now used to set the render time for frames.

BUG=webrtc:5514
R=stefan@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/4f6cd6ac59eab8d0202eea905...
==========

to

==========
Wire up VCMTiming in FrameBuffer2.

The VCMTiming class is now used to set the render time for frames.

BUG=webrtc:5514
R=stefan@webrtc.org

Committed: https://crrev.com/4f6cd6ac59eab8d0202eea9059371403604e4f6a
Cr-Commit-Position: refs/heads/master@{#13621}
==========