VideoAdapter: Drop frames based on actual fps instead of expected fps

VideoAdapter: Drop frames based on actual fps instead of expected fps

Pass timestamps to VideoAdapter instead of setting expected input frame rate, and use that to calculate when frames should be dropped.

BUG=webrtc:4938
TEST=Enable quality slider and HUD in debug settings. Request low fps with the quality slider and observe dropped frames.

Committed: https://crrev.com/604abe09f1d8bffc9e0d52c09484371f063cc2cd
Cr-Commit-Position: refs/heads/master@{#12811}

[magjed_webrtc, 2016-05-17 13:18:44]

Patchset #1 (id:1) has been deleted

[magjed_webrtc, 2016-05-18 09:03:59]

Description was changed from

==========
VideoAdapter: Drop frames based on actual fps instead of expected fps

Pass timestamps to VideoAdapter instead of setting expected input frame rate.

BUG=webrtc:4938
==========

to

==========
VideoAdapter: Drop frames based on actual fps instead of expected fps

Pass timestamps to VideoAdapter instead of setting expected input frame rate.

BUG=webrtc:4938
==========

[magjed_webrtc, 2016-05-18 09:03:59]

magjed@webrtc.org changed reviewers:
+ nisse@webrtc.org, perkj@webrtc.org

[magjed_webrtc, 2016-05-18 09:04:30]

Description was changed from

==========
VideoAdapter: Drop frames based on actual fps instead of expected fps

Pass timestamps to VideoAdapter instead of setting expected input frame rate.

BUG=webrtc:4938
==========

to

==========
VideoAdapter: Drop frames based on actual fps instead of expected fps

Pass timestamps to VideoAdapter instead of setting expected input frame rate,
and use that to calculate when frames should be dropped.

BUG=webrtc:4938
==========

[magjed_webrtc, 2016-05-18 09:04:50]

Please take a look.

[nisse-webrtc, 2016-05-18 11:44:00]

https://codereview.webrtc.org/1982983003/diff/20001/webrtc/media/base/videoad...
File webrtc/media/base/videoadapter.cc (right):

https://codereview.webrtc.org/1982983003/diff/20001/webrtc/media/base/videoad...
webrtc/media/base/videoadapter.cc:121: previous_timestamp_ns_(0),
Please make it an rtc::Optional, instead of using 0 as a magic value for unset.

https://codereview.webrtc.org/1982983003/diff/20001/webrtc/media/base/videoad...
webrtc/media/base/videoadapter.cc:122: interval_ns_next_frame_(0),
There should be a comment somewhere explaining the meaning of this variable. I
take it is time left until the next frame should be sent?

It might be clearer to replace both previous_timestamp_ns_ and
interval_ns_next_frame_ by a variable holding the absolute time of the next
frame.

nit: I think unit should be moved to the end of the name.

https://codereview.webrtc.org/1982983003/diff/20001/webrtc/media/base/videoad...
webrtc/media/base/videoadapter.cc:128: bool VideoAdapter::ShouldDrop(int64_t
in_timestamp_ns) {
I'd prefer if false meant that the frame should be dropped. Name could be
"KeepFrame".

https://codereview.webrtc.org/1982983003/diff/20001/webrtc/media/base/videoad...
File webrtc/media/base/videoadapter_unittest.cc (right):

https://codereview.webrtc.org/1982983003/diff/20001/webrtc/media/base/videoad...
webrtc/media/base/videoadapter_unittest.cc:31: capture_format_.interval =
VideoFormat::FpsToInterval(30);
Why this change from 50 to 30 fps?

[magjed_webrtc, 2016-05-18 16:35:23]

https://codereview.webrtc.org/1982983003/diff/20001/webrtc/media/base/videoad...
File webrtc/media/base/videoadapter.cc (right):

https://codereview.webrtc.org/1982983003/diff/20001/webrtc/media/base/videoad...
webrtc/media/base/videoadapter.cc:121: previous_timestamp_ns_(0),
On 2016/05/18 11:44:00, nisse-webrtc wrote:
> Please make it an rtc::Optional, instead of using 0 as a magic value for
unset.

Done.

https://codereview.webrtc.org/1982983003/diff/20001/webrtc/media/base/videoad...
webrtc/media/base/videoadapter.cc:122: interval_ns_next_frame_(0),
On 2016/05/18 11:44:00, nisse-webrtc wrote:
> There should be a comment somewhere explaining the meaning of this variable. I
> take it is time left until the next frame should be sent?
> 
> It might be clearer to replace both previous_timestamp_ns_ and
> interval_ns_next_frame_ by a variable holding the absolute time of the next
> frame.
> 
> nit: I think unit should be moved to the end of the name.

Done.

https://codereview.webrtc.org/1982983003/diff/20001/webrtc/media/base/videoad...
webrtc/media/base/videoadapter.cc:128: bool VideoAdapter::ShouldDrop(int64_t
in_timestamp_ns) {
On 2016/05/18 11:44:00, nisse-webrtc wrote:
> I'd prefer if false meant that the frame should be dropped. Name could be
> "KeepFrame".

Done.

https://codereview.webrtc.org/1982983003/diff/20001/webrtc/media/base/videoad...
File webrtc/media/base/videoadapter_unittest.cc (right):

https://codereview.webrtc.org/1982983003/diff/20001/webrtc/media/base/videoad...
webrtc/media/base/videoadapter_unittest.cc:31: capture_format_.interval =
VideoFormat::FpsToInterval(30);
On 2016/05/18 11:44:00, nisse-webrtc wrote:
> Why this change from 50 to 30 fps?

Unrelated, but it makes no sense to request 50 fps because FakeCapturer only
supports 30 fps anyway, and will open in 30 fps regardless.

[nisse-webrtc, 2016-05-19 07:02:36]

lgtm

https://codereview.webrtc.org/1982983003/diff/40001/webrtc/media/base/videoad...
File webrtc/media/base/videoadapter.cc (right):

https://codereview.webrtc.org/1982983003/diff/40001/webrtc/media/base/videoad...
webrtc/media/base/videoadapter.cc:144: time_until_next_frame_ns >=
requested_format_->interval) {
Can you use std::abs?

https://codereview.webrtc.org/1982983003/diff/40001/webrtc/media/base/videoad...
webrtc/media/base/videoadapter.cc:155: *next_frame_timestamp_ns_ +=
requested_format_->interval;
This is the overload T& operator*(), or how does it work?

Some while ago I tried *optional = value, expecting it to do the same as
optional = rtc::Optional<type>(value), but it didn't.

[magjed_webrtc, 2016-05-19 08:52:19]

https://codereview.webrtc.org/1982983003/diff/40001/webrtc/media/base/videoad...
File webrtc/media/base/videoadapter.cc (right):

https://codereview.webrtc.org/1982983003/diff/40001/webrtc/media/base/videoad...
webrtc/media/base/videoadapter.cc:144: time_until_next_frame_ns >=
requested_format_->interval) {
On 2016/05/19 07:02:36, nisse-webrtc wrote:
> Can you use std::abs?

Done.

https://codereview.webrtc.org/1982983003/diff/40001/webrtc/media/base/videoad...
webrtc/media/base/videoadapter.cc:155: *next_frame_timestamp_ns_ +=
requested_format_->interval;
On 2016/05/19 07:02:36, nisse-webrtc wrote:
> This is the overload T& operator*(), or how does it work?
> 
> Some while ago I tried *optional = value, expecting it to do the same as
> optional = rtc::Optional<type>(value), but it didn't.

Yes, it's the overloaded operator T& operator*(). It will work because I know
that I have a value at this point, so I can dereference it.

*optional = value, will not work if !optional, otherwise it's exactly as
optional = rtc::Optional<type>(value).

[nisse-webrtc, 2016-05-19 09:01:58]

lgtm

[perkj_webrtc, 2016-05-19 10:02:41]

lgtm if test below is added and can you please add a TEST= and describe how to
test this using ApprtcDemo with the HUD active and the quality slider to verify.

https://codereview.webrtc.org/1982983003/diff/60001/webrtc/media/base/videoad...
File webrtc/media/base/videoadapter_unittest.cc (right):

https://codereview.webrtc.org/1982983003/diff/60001/webrtc/media/base/videoad...
webrtc/media/base/videoadapter_unittest.cc:200: }
Can you please copy this test and add one that use timestamps with jitter.

[magjed_webrtc, 2016-05-19 11:33:10]

Description was changed from

==========
VideoAdapter: Drop frames based on actual fps instead of expected fps

Pass timestamps to VideoAdapter instead of setting expected input frame rate,
and use that to calculate when frames should be dropped.

BUG=webrtc:4938
==========

to

==========
VideoAdapter: Drop frames based on actual fps instead of expected fps

Pass timestamps to VideoAdapter instead of setting expected input frame rate,
and use that to calculate when frames should be dropped.

BUG=webrtc:4938
TEST=Enable capture quality slider and HUD in debug settings. Change capture fps
lower than input and observe dropped frames.
==========

[magjed_webrtc, 2016-05-19 11:33:58]

Description was changed from

==========
VideoAdapter: Drop frames based on actual fps instead of expected fps

Pass timestamps to VideoAdapter instead of setting expected input frame rate,
and use that to calculate when frames should be dropped.

BUG=webrtc:4938
TEST=Enable capture quality slider and HUD in debug settings. Change capture fps
lower than input and observe dropped frames.
==========

to

==========
VideoAdapter: Drop frames based on actual fps instead of expected fps

Pass timestamps to VideoAdapter instead of setting expected input frame rate,
and use that to calculate when frames should be dropped.

BUG=webrtc:4938
TEST=Enable quality slider and HUD in debug settings. Request low fps with the
quality slider and observe dropped frames.
==========

[magjed_webrtc, 2016-05-19 11:35:32]

https://codereview.webrtc.org/1982983003/diff/60001/webrtc/media/base/videoad...
File webrtc/media/base/videoadapter_unittest.cc (right):

https://codereview.webrtc.org/1982983003/diff/60001/webrtc/media/base/videoad...
webrtc/media/base/videoadapter_unittest.cc:200: }
On 2016/05/19 10:02:41, perkj_webrtc wrote:
> Can you please copy this test and add one that use timestamps with jitter. 

Done.

[magjed_webrtc, 2016-05-19 11:35:36]

The CQ bit was checked by magjed@webrtc.org

[magjed_webrtc, 2016-05-19 11:35:36]

The patchset sent to the CQ was uploaded after l-g-t-m from nisse@webrtc.org,
perkj@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1982983003/#ps80001 (title:
"Add jitter test")

[commit-bot: I haz the power, 2016-05-19 11:35:37]

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

[commit-bot: I haz the power, 2016-05-19 13:05:43]

Description was changed from

==========
VideoAdapter: Drop frames based on actual fps instead of expected fps

Pass timestamps to VideoAdapter instead of setting expected input frame rate,
and use that to calculate when frames should be dropped.

BUG=webrtc:4938
TEST=Enable quality slider and HUD in debug settings. Request low fps with the
quality slider and observe dropped frames.
==========

to

==========
VideoAdapter: Drop frames based on actual fps instead of expected fps

Pass timestamps to VideoAdapter instead of setting expected input frame rate,
and use that to calculate when frames should be dropped.

BUG=webrtc:4938
TEST=Enable quality slider and HUD in debug settings. Request low fps with the
quality slider and observe dropped frames.
==========

[commit-bot: I haz the power, 2016-05-19 13:05:44]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2016-05-19 13:05:52]

Description was changed from

==========
VideoAdapter: Drop frames based on actual fps instead of expected fps

Pass timestamps to VideoAdapter instead of setting expected input frame rate,
and use that to calculate when frames should be dropped.

BUG=webrtc:4938
TEST=Enable quality slider and HUD in debug settings. Request low fps with the
quality slider and observe dropped frames.
==========

to

==========
VideoAdapter: Drop frames based on actual fps instead of expected fps

Pass timestamps to VideoAdapter instead of setting expected input frame rate,
and use that to calculate when frames should be dropped.

BUG=webrtc:4938
TEST=Enable quality slider and HUD in debug settings. Request low fps with the
quality slider and observe dropped frames.

Committed: https://crrev.com/604abe09f1d8bffc9e0d52c09484371f063cc2cd
Cr-Commit-Position: refs/heads/master@{#12811}
==========

[commit-bot: I haz the power, 2016-05-19 13:05:53]

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