Initial rate allocation should not use fps = 0

Initial rate allocation should not use fps = 0

A recent cl (https://codereview.webrtc.org/2510583002) introduced an
issue where the initial rate allocation (call to VideoBitrateAllocator
and any associated temporal layers) uses framerate = 0 fps. This may
cause issues, including having the rate control in ScreenshareLayers
ramp up too slowly.

This CL make the initial call use VideoCodec.maxFramerate as framerate.
Also expanded unit tests.

BUG=webrtc:6301
Committed: https://crrev.com/40217c3718b9c6c53f399420d679b3d581886b89
Cr-Commit-Position: refs/heads/master@{#15166}

[sprang_webrtc, 2016-11-21 09:45:46]

The CQ bit was checked by sprang@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-21 09:45:48]

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

[sprang_webrtc, 2016-11-21 10:09:21]

sprang@webrtc.org changed reviewers:
+ danilchap@webrtc.org

[danilchap, 2016-11-21 10:43:25]

https://codereview.webrtc.org/2513383002/diff/20001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/video_coding_impl.h (right):

https://codereview.webrtc.org/2513383002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/video_coding_impl.h:43: static const int64_t
kDefaultProcessIntervalMs = 1000;
Do you plan to use this constant outside SendVideo too?

https://codereview.webrtc.org/2513383002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/video_coding_impl.h:117: bool has_received_frame_;
can it be used from multiple threads? may be protect with a crit_ then.
if not, may be add GUARDED_BY(sequenced_checker_)
(if it is more complicated, but you sure there will be no race, can guard by
race_checker)

[sprang_webrtc, 2016-11-21 11:35:32]

https://codereview.webrtc.org/2513383002/diff/20001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/video_coding_impl.h (right):

https://codereview.webrtc.org/2513383002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/video_coding_impl.h:43: static const int64_t
kDefaultProcessIntervalMs = 1000;
On 2016/11/21 10:43:25, danilchap wrote:
> Do you plan to use this constant outside SendVideo too?

I use it the unit test now.

https://codereview.webrtc.org/2513383002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/video_coding_impl.h:117: bool has_received_frame_;
On 2016/11/21 10:43:25, danilchap wrote:
> can it be used from multiple threads? may be protect with a crit_ then.
> if not, may be add GUARDED_BY(sequenced_checker_)
> (if it is more complicated, but you sure there will be no race, can guard by
> race_checker)

I moved it and protected it by params_crit_

[danilchap, 2016-11-21 12:45:43]

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

https://codereview.webrtc.org/2513383002/diff/60001/webrtc/modules/video_codi...
webrtc/modules/video_coding/video_sender.cc:71: encoder_params_.input_frame_rate
= _mediaOpt.InputFrameRate();
shouldn't you adjust frame_rate here too?
if not, may be avoid setting maxFramerate to encoder_params_.input_frame_rate
below (only pass maxFramerate to bitrate allocator)

https://codereview.webrtc.org/2513383002/diff/60001/webrtc/modules/video_codi...
webrtc/modules/video_coding/video_sender.cc:205: has_received_frame_ = true;
variable name make impression it should be set to true in AddVideoFrame.
May be a different name can explain better why it is set here:
has_calculated_framerate_
had_(nonzero_)framerate_
!use_fake_framerate_

https://codereview.webrtc.org/2513383002/diff/60001/webrtc/modules/video_codi...
webrtc/modules/video_coding/video_sender.cc:206: } else if (input_frame_rate ==
0 && !has_received_frame_) {
This mean input_frame_rate ~= 0 mid call is a valid number?
can it be valid from the start of the call?

https://codereview.webrtc.org/2513383002/diff/60001/webrtc/modules/video_codi...
webrtc/modules/video_coding/video_sender.cc:247: encoder_params =
encoder_params_;
may be other way around make slightly more sense:
first prepare local variable encoder_params,
then assign it to member variable.

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

https://codereview.webrtc.org/2513383002/diff/60001/webrtc/modules/video_codi...
webrtc/modules/video_coding/video_sender_unittest.cc:304: const int
kFrameIntervalMs = 1000 / kActualFrameRate;
may be int64_t for consistency with other ms variables.

https://codereview.webrtc.org/2513383002/diff/60001/webrtc/modules/video_codi...
webrtc/modules/video_coding/video_sender_unittest.cc:321:
(VCMProcessTimer::kDefaultProcessIntervalMs / kActualFrameRate) + 1;
(/kFramesIntervalMs) or (*kActualFrameRate/1000)
instead of /kActualFrameRate

[sprang_webrtc, 2016-11-21 13:01:21]

It turns out fps = 0 is treated as invalid value in other parts of the code, so
I'll simplify this and remove the frame received state.

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

https://codereview.webrtc.org/2513383002/diff/60001/webrtc/modules/video_codi...
webrtc/modules/video_coding/video_sender.cc:71: encoder_params_.input_frame_rate
= _mediaOpt.InputFrameRate();
On 2016/11/21 12:45:42, danilchap wrote:
> shouldn't you adjust frame_rate here too?
> if not, may be avoid setting maxFramerate to encoder_params_.input_frame_rate
> below (only pass maxFramerate to bitrate allocator)

Looking close, SetEncoderParameters() actually treats 0 as invalid and caps to
codec.maxFramerate before calling the encoder, so I'll just simplify this a bit.

https://codereview.webrtc.org/2513383002/diff/60001/webrtc/modules/video_codi...
webrtc/modules/video_coding/video_sender.cc:205: has_received_frame_ = true;
On 2016/11/21 12:45:42, danilchap wrote:
> variable name make impression it should be set to true in AddVideoFrame.
> May be a different name can explain better why it is set here:
> has_calculated_framerate_
> had_(nonzero_)framerate_
> !use_fake_framerate_

Ack, but removed instead.

https://codereview.webrtc.org/2513383002/diff/60001/webrtc/modules/video_codi...
webrtc/modules/video_coding/video_sender.cc:206: } else if (input_frame_rate ==
0 && !has_received_frame_) {
On 2016/11/21 12:45:42, danilchap wrote:
> This mean input_frame_rate ~= 0 mid call is a valid number?
> can it be valid from the start of the call?

My impression was that 0 could be a valid framerate during the call. In a
follow-up CL I would like to clean up media opts and remove that rate tracking,
since it's really poorly implemented, and reuse the RateStatistics class
instead, which returns an Optional with no value if there is no reliable value
available.

https://codereview.webrtc.org/2513383002/diff/60001/webrtc/modules/video_codi...
webrtc/modules/video_coding/video_sender.cc:247: encoder_params =
encoder_params_;
On 2016/11/21 12:45:42, danilchap wrote:
> may be other way around make slightly more sense:
> first prepare local variable encoder_params,
> then assign it to member variable.

Ack, but reverted instead.

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

https://codereview.webrtc.org/2513383002/diff/60001/webrtc/modules/video_codi...
webrtc/modules/video_coding/video_sender_unittest.cc:304: const int
kFrameIntervalMs = 1000 / kActualFrameRate;
On 2016/11/21 12:45:43, danilchap wrote:
> may be int64_t for consistency with other ms variables.

Done.

https://codereview.webrtc.org/2513383002/diff/60001/webrtc/modules/video_codi...
webrtc/modules/video_coding/video_sender_unittest.cc:321:
(VCMProcessTimer::kDefaultProcessIntervalMs / kActualFrameRate) + 1;
On 2016/11/21 12:45:42, danilchap wrote:
> (/kFramesIntervalMs) or (*kActualFrameRate/1000)
> instead of /kActualFrameRate

Oops, kFrameIntervalMs was what I intended to put there.

[danilchap, 2016-11-21 13:14:49]

lgtm

https://codereview.webrtc.org/2513383002/diff/80001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/video_sender_unittest.cc (right):

https://codereview.webrtc.org/2513383002/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/video_sender_unittest.cc:303: const int64_t
kActualFrameRate = settings_.maxFramerate / 2;
since it is not a time variable, may be revert to generic int (some number).

(checked some framerate variables, look like there is no consistency there:
unsigned char, uint32_t, float)

[sprang_webrtc, 2016-11-21 13:21:50]

https://codereview.webrtc.org/2513383002/diff/80001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/video_sender_unittest.cc (right):

https://codereview.webrtc.org/2513383002/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/video_sender_unittest.cc:303: const int64_t
kActualFrameRate = settings_.maxFramerate / 2;
On 2016/11/21 13:14:49, danilchap wrote:
> since it is not a time variable, may be revert to generic int (some number).
> 
> (checked some framerate variables, look like there is no consistency there:
> unsigned char, uint32_t, float)

Sure. And no, little consistency :(
I'd like to clean this up generally. I think uint32_t is the most common.

[sprang_webrtc, 2016-11-21 13:21:53]

The CQ bit was checked by sprang@webrtc.org

[sprang_webrtc, 2016-11-21 13:21:53]

The patchset sent to the CQ was uploaded after l-g-t-m from danilchap@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2513383002/#ps100001 (title:
"nit")

[commit-bot: I haz the power, 2016-11-21 13:21:58]

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-11-21 13:41:53]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1479734513658640,
"parent_rev": ["ec3c84f77b8b3e3654cebfcf97471c0451a3bff4", null], "commit_rev":
["4418ff41d76c14dab267ce480d2189af68ded85f", null]}

[commit-bot: I haz the power, 2016-11-21 13:41:57]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-11-21 13:42:08]

Description was changed from

==========
Initial rate allocation should not use fps = 0

A recent cl (https://codereview.webrtc.org/2510583002) introduced an
issue where the initial rate allocation (call to VideoBitrateAllocator
and any associated temporal layers) uses framerate = 0 fps. This may
cause issues, including having the rate control in ScreenshareLayers
ramp up too slowly.

This CL make the initial call use VideoCodec.maxFramerate as framerate.
Also expanded unit tests.

BUG=webrtc:6301
==========

to

==========
Initial rate allocation should not use fps = 0

A recent cl (https://codereview.webrtc.org/2510583002) introduced an
issue where the initial rate allocation (call to VideoBitrateAllocator
and any associated temporal layers) uses framerate = 0 fps. This may
cause issues, including having the rate control in ScreenshareLayers
ramp up too slowly.

This CL make the initial call use VideoCodec.maxFramerate as framerate.
Also expanded unit tests.

BUG=webrtc:6301

Committed: https://crrev.com/40217c3718b9c6c53f399420d679b3d581886b89
Cr-Commit-Position: refs/heads/master@{#15166}
==========

[commit-bot: I haz the power, 2016-11-21 13:42:08]

Patchset 6 (id:??) landed as
https://crrev.com/40217c3718b9c6c53f399420d679b3d581886b89
Cr-Commit-Position: refs/heads/master@{#15166}