Move logic for calculating needed bitrate overhead used by NACK and FEC to VideoSender.

Move logic for calculating needed bitrate overhead used by NACK and FEC to VideoSender.

This cl split the class MediaOptimization into two parts. One that deals with frame dropping and stats and one new class called ProtectionBitrateCalculator that deals with  calculating the needed FEC parameters and how much of the estimated network bitrate that can be used by an encoder

Note that the logic of how FEC and the needed bitrates is not changed.

BUG=webrtc:5687
R=asapersson@webrtc.org, stefan@webrtc.org

Committed: https://crrev.com/69b332df8386a0cfde9630a9f7fccc4abe452926
Cr-Commit-Position: refs/heads/master@{#13018}

[perkj_webrtc, 2016-05-16 13:18:46]

Renamed + fix Build.gn + git cl format

[perkj_webrtc, 2016-05-17 12:12:16]

Description was changed from

==========
Move logic for calculating needed bitrate overhead used by NACK and FEC to
VideoSender.

BUG=
==========

to

==========
Move logic for calculating needed bitrate overhead used by NACK and FEC to
VideoSender.

This cl split the class MediaOptimization into two parts. One that deals with
frame dropping and stats and one new class called ProtectionBitrateCalculator
that deals with  calculating the needed FEC parameters and how much of the
estimated network bitrate that can be used by an encoder 

Note that the logic of how FEC and the needed bitrates is not changed. 

BUG=webrtc:5687
==========

[perkj_webrtc, 2016-05-17 12:12:16]

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

[perkj_webrtc, 2016-05-17 12:29:00]

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

[perkj_webrtc, 2016-05-17 15:21:24]

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

[perkj_webrtc, 2016-05-17 15:21:25]

Hi
Can you please review?

[perkj_webrtc, 2016-05-19 10:08:18]

please?

[stefan-webrtc, 2016-05-19 12:52:40]

This was fairly hard to review, so I have to do another pass later, but this is
a first set of comments.

https://codereview.webrtc.org/1972083002/diff/200001/webrtc/modules/remote_bi...
File
webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_single_stream.cc
(left):

https://codereview.webrtc.org/1972083002/diff/200001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_single_stream.cc:57:
assert(observer_);
Why?

https://codereview.webrtc.org/1972083002/diff/200001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/protection_bitrate_calculator.cc (right):

https://codereview.webrtc.org/1972083002/diff/200001/webrtc/modules/video_cod...
webrtc/modules/video_coding/protection_bitrate_calculator.cc:50: size_t mtu) {
Call this max_payload_size instead.

https://codereview.webrtc.org/1972083002/diff/200001/webrtc/modules/video_cod...
webrtc/modules/video_coding/protection_bitrate_calculator.cc:63: uint32_t
target_bitrate,
add unit

https://codereview.webrtc.org/1972083002/diff/200001/webrtc/modules/video_cod...
webrtc/modules/video_coding/protection_bitrate_calculator.cc:66:
crit_sect_.Enter();
Change to use a scoped crit

https://codereview.webrtc.org/1972083002/diff/200001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/protection_bitrate_calculator.h (right):

https://codereview.webrtc.org/1972083002/diff/200001/webrtc/modules/video_cod...
webrtc/modules/video_coding/protection_bitrate_calculator.h:25: //
ProtectionBitrateCalculator calculates the calculates how much of the
-"calculates the"

https://codereview.webrtc.org/1972083002/diff/200001/webrtc/modules/video_cod...
webrtc/modules/video_coding/protection_bitrate_calculator.h:27: // is needed for
protection packets such as FEC and NACK. It uses an
Maybe call it redundant packets instead?

https://codereview.webrtc.org/1972083002/diff/200001/webrtc/modules/video_cod...
webrtc/modules/video_coding/protection_bitrate_calculator.h:37: explicit
ProtectionBitrateCalculator(
Remove explicit

https://codereview.webrtc.org/1972083002/diff/200001/webrtc/modules/video_cod...
webrtc/modules/video_coding/protection_bitrate_calculator.h:42: void
SetProtectionMethod(bool enable_fec, bool enable_nack);
Would it make sense to somehow document the thread model now that we're
refactoring this a bit?

https://codereview.webrtc.org/1972083002/diff/200001/webrtc/modules/video_cod...
webrtc/modules/video_coding/protection_bitrate_calculator.h:64: uint32_t
SentBitRate();
It's a bit weird that this class is named something with protection, and then it
also keeps track of framerate and bitrate stats...

https://codereview.webrtc.org/1972083002/diff/200001/webrtc/modules/video_cod...
webrtc/modules/video_coding/protection_bitrate_calculator.h:84: struct
EncodedFrameSample;
Move this to before all members instead to not mix forward declarations with
members.

https://codereview.webrtc.org/1972083002/diff/200001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/protection_bitrate_calculator_unittest.cc
(right):

https://codereview.webrtc.org/1972083002/diff/200001/webrtc/modules/video_cod...
webrtc/modules/video_coding/protection_bitrate_calculator_unittest.cc:32:
*sent_nack_rate_bps = 0;
Should this always be zero? Maybe we should have a test which uses it

https://codereview.webrtc.org/1972083002/diff/200001/webrtc/video/video_send_...
File webrtc/video/video_send_stream.cc (right):

https://codereview.webrtc.org/1972083002/diff/200001/webrtc/video/video_send_...
webrtc/video/video_send_stream.cc:573: encoder_settings->streams.size(),
payload_router_.MaxPayloadLength());
num_layers should be the number of temporal layers. Now I think you are setting
it to number of spatial layers?

[perkj_webrtc, 2016-06-01 20:54:32]

Patchset #13 (id:240001) has been deleted

[perkj_webrtc, 2016-06-01 20:55:24]

PTAL

https://codereview.webrtc.org/1972083002/diff/200001/webrtc/modules/remote_bi...
File
webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_single_stream.cc
(left):

https://codereview.webrtc.org/1972083002/diff/200001/webrtc/modules/remote_bi...
webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_single_stream.cc:57:
assert(observer_);
On 2016/05/19 12:52:39, stefan-webrtc (holmer) wrote:
> Why?

reverted. Needed by new VideoSendStreamTests that I had to revert for now. Will
try to create a cl with that once my other cls has landed.

https://codereview.webrtc.org/1972083002/diff/200001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/protection_bitrate_calculator.cc (right):

https://codereview.webrtc.org/1972083002/diff/200001/webrtc/modules/video_cod...
webrtc/modules/video_coding/protection_bitrate_calculator.cc:50: size_t mtu) {
On 2016/05/19 12:52:39, stefan-webrtc (holmer) wrote:
> Call this max_payload_size instead.

Done.

https://codereview.webrtc.org/1972083002/diff/200001/webrtc/modules/video_cod...
webrtc/modules/video_coding/protection_bitrate_calculator.cc:63: uint32_t
target_bitrate,
On 2016/05/19 12:52:39, stefan-webrtc (holmer) wrote:
> add unit

Done.

https://codereview.webrtc.org/1972083002/diff/200001/webrtc/modules/video_cod...
webrtc/modules/video_coding/protection_bitrate_calculator.cc:66:
crit_sect_.Enter();
On 2016/05/19 12:52:39, stefan-webrtc (holmer) wrote:
> Change to use a scoped crit

ok- but I then have to move a few things around the scope so that I don't hold
the lock while calling protection_callback_->ProtectionRequest.

https://codereview.webrtc.org/1972083002/diff/200001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/protection_bitrate_calculator.h (right):

https://codereview.webrtc.org/1972083002/diff/200001/webrtc/modules/video_cod...
webrtc/modules/video_coding/protection_bitrate_calculator.h:25: //
ProtectionBitrateCalculator calculates the calculates how much of the
On 2016/05/19 12:52:40, stefan-webrtc (holmer) wrote:
> -"calculates the"

Done.

https://codereview.webrtc.org/1972083002/diff/200001/webrtc/modules/video_cod...
webrtc/modules/video_coding/protection_bitrate_calculator.h:27: // is needed for
protection packets such as FEC and NACK. It uses an
On 2016/05/19 12:52:40, stefan-webrtc (holmer) wrote:
> Maybe call it redundant packets instead?

Done.

https://codereview.webrtc.org/1972083002/diff/200001/webrtc/modules/video_cod...
webrtc/modules/video_coding/protection_bitrate_calculator.h:37: explicit
ProtectionBitrateCalculator(
On 2016/05/19 12:52:40, stefan-webrtc (holmer) wrote:
> Remove explicit

Done.

https://codereview.webrtc.org/1972083002/diff/200001/webrtc/modules/video_cod...
webrtc/modules/video_coding/protection_bitrate_calculator.h:42: void
SetProtectionMethod(bool enable_fec, bool enable_nack);
On 2016/05/19 12:52:40, stefan-webrtc (holmer) wrote:
> Would it make sense to somehow document the thread model now that we're
> refactoring this a bit?

Added a short note about thread safety.

https://codereview.webrtc.org/1972083002/diff/200001/webrtc/modules/video_cod...
webrtc/modules/video_coding/protection_bitrate_calculator.h:64: uint32_t
SentBitRate();
On 2016/05/19 12:52:40, stefan-webrtc (holmer) wrote:
> It's a bit weird that this class is named something with protection, and then
it
> also keeps track of framerate and bitrate stats...

Removed. Now relies on fps as an input parameter from the send_stats proxy.

https://codereview.webrtc.org/1972083002/diff/200001/webrtc/modules/video_cod...
webrtc/modules/video_coding/protection_bitrate_calculator.h:84: struct
EncodedFrameSample;
On 2016/05/19 12:52:40, stefan-webrtc (holmer) wrote:
> Move this to before all members instead to not mix forward declarations with
> members.

Done.

https://codereview.webrtc.org/1972083002/diff/200001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/protection_bitrate_calculator_unittest.cc
(right):

https://codereview.webrtc.org/1972083002/diff/200001/webrtc/modules/video_cod...
webrtc/modules/video_coding/protection_bitrate_calculator_unittest.cc:32:
*sent_nack_rate_bps = 0;
On 2016/05/19 12:52:40, stefan-webrtc (holmer) wrote:
> Should this always be zero? Maybe we should have a test which uses it

Done.

https://codereview.webrtc.org/1972083002/diff/200001/webrtc/video/video_send_...
File webrtc/video/video_send_stream.cc (right):

https://codereview.webrtc.org/1972083002/diff/200001/webrtc/video/video_send_...
webrtc/video/video_send_stream.cc:573: encoder_settings->streams.size(),
payload_router_.MaxPayloadLength());
On 2016/05/19 12:52:40, stefan-webrtc (holmer) wrote:
> num_layers should be the number of temporal layers. Now I think you are
setting
> it to number of spatial layers?

Done.

[stefan-webrtc, 2016-06-02 07:42:36]

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

[stefan-webrtc, 2016-06-02 07:42:38]

LG, but adding Åsa to review send_statistics_proxy.*

https://codereview.webrtc.org/1972083002/diff/260001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/protection_bitrate_calculator.cc (right):

https://codereview.webrtc.org/1972083002/diff/260001/webrtc/modules/video_cod...
webrtc/modules/video_coding/protection_bitrate_calculator.cc:122: }  // 
|crit_sect_.Leave()|
We typically don't add comments like this, it's fairly easy to see that it's the
end of the crit sect scope anyway.

https://codereview.webrtc.org/1972083002/diff/260001/webrtc/modules/video_cod...
webrtc/modules/video_coding/protection_bitrate_calculator.cc:176: const size_t&
encoded_length = encoded_image._length;
I don't think there's a point in making this a const reference, the copy
shouldn't be too expensive? :)

https://codereview.webrtc.org/1972083002/diff/260001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/protection_bitrate_calculator_unittest.cc
(right):

https://codereview.webrtc.org/1972083002/diff/260001/webrtc/modules/video_cod...
webrtc/modules/video_coding/protection_bitrate_calculator_unittest.cc:37:
public:
Should this be at the top of the class?

https://codereview.webrtc.org/1972083002/diff/260001/webrtc/video/send_statis...
File webrtc/video/send_statistics_proxy.cc (right):

https://codereview.webrtc.org/1972083002/diff/260001/webrtc/video/send_statis...
webrtc/video/send_statistics_proxy.cc:518: return
uma_container_->sent_frame_rate_tracker_.ComputeRateForInterval(1000);
It seems a bit weird to me that we're getting the frame rate out of the uma
container. Please add asapersson to review this.

https://codereview.webrtc.org/1972083002/diff/260001/webrtc/video/video_send_...
File webrtc/video/video_send_stream.cc (right):

https://codereview.webrtc.org/1972083002/diff/260001/webrtc/video/video_send_...
webrtc/video/video_send_stream.cc:576: 1;
Weird formatting, but if it's how clang format wants it...

[perkj_webrtc, 2016-06-02 10:16:20]

ptal. Åsa, can you also take a look at how I use the frame rate counter and see
what you think?

https://codereview.webrtc.org/1972083002/diff/260001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/protection_bitrate_calculator.cc (right):

https://codereview.webrtc.org/1972083002/diff/260001/webrtc/modules/video_cod...
webrtc/modules/video_coding/protection_bitrate_calculator.cc:122: }  // 
|crit_sect_.Leave()|
On 2016/06/02 07:42:37, stefan-webrtc (holmer) wrote:
> We typically don't add comments like this, it's fairly easy to see that it's
the
> end of the crit sect scope anyway.

Done.

https://codereview.webrtc.org/1972083002/diff/260001/webrtc/modules/video_cod...
webrtc/modules/video_coding/protection_bitrate_calculator.cc:176: const size_t&
encoded_length = encoded_image._length;
On 2016/06/02 07:42:37, stefan-webrtc (holmer) wrote:
> I don't think there's a point in making this a const reference, the copy
> shouldn't be too expensive? :)

Should be the same.

https://codereview.webrtc.org/1972083002/diff/260001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/protection_bitrate_calculator_unittest.cc
(right):

https://codereview.webrtc.org/1972083002/diff/260001/webrtc/modules/video_cod...
webrtc/modules/video_coding/protection_bitrate_calculator_unittest.cc:37:
public:
On 2016/06/02 07:42:37, stefan-webrtc (holmer) wrote:
> Should this be at the top of the class?

Done.

https://codereview.webrtc.org/1972083002/diff/260001/webrtc/video/send_statis...
File webrtc/video/send_statistics_proxy.cc (right):

https://codereview.webrtc.org/1972083002/diff/260001/webrtc/video/send_statis...
webrtc/video/send_statistics_proxy.cc:518: return
uma_container_->sent_frame_rate_tracker_.ComputeRateForInterval(1000);
On 2016/06/02 07:42:37, stefan-webrtc (holmer) wrote:
> It seems a bit weird to me that we're getting the frame rate out of the uma
> container. Please add asapersson to review this.

sure- would it be nice if pc.GetStats, uma and internall usage actually shared
the same data instead of creating separate copies in several places?

https://codereview.webrtc.org/1972083002/diff/260001/webrtc/video/video_send_...
File webrtc/video/video_send_stream.cc (right):

https://codereview.webrtc.org/1972083002/diff/260001/webrtc/video/video_send_...
webrtc/video/video_send_stream.cc:576: 1;
On 2016/06/02 07:42:37, stefan-webrtc (holmer) wrote:
> Weird formatting, but if it's how clang format wants it...

yup

[perkj_webrtc, 2016-06-02 10:17:06]

Patchset #14 (id:280001) has been deleted

[åsapersson, 2016-06-02 11:11:40]

https://codereview.webrtc.org/1972083002/diff/260001/webrtc/video/send_statis...
File webrtc/video/send_statistics_proxy.cc (right):

https://codereview.webrtc.org/1972083002/diff/260001/webrtc/video/send_statis...
webrtc/video/send_statistics_proxy.cc:518: return
uma_container_->sent_frame_rate_tracker_.ComputeRateForInterval(1000);
On 2016/06/02 10:16:20, perkj_webrtc wrote:
> On 2016/06/02 07:42:37, stefan-webrtc (holmer) wrote:
> > It seems a bit weird to me that we're getting the frame rate out of the uma
> > container. Please add asapersson to review this.
> 
> sure- would it be nice if pc.GetStats, uma and internall usage actually shared
> the same data instead of creating separate copies in several places? 

Maybe it is better to return stats_.encode_frame_rate which is used by
pc.GetStats (since there is a todo in function above).

[perkj_webrtc, 2016-06-02 11:33:21]

ptal

https://codereview.webrtc.org/1972083002/diff/260001/webrtc/video/send_statis...
File webrtc/video/send_statistics_proxy.cc (right):

https://codereview.webrtc.org/1972083002/diff/260001/webrtc/video/send_statis...
webrtc/video/send_statistics_proxy.cc:518: return
uma_container_->sent_frame_rate_tracker_.ComputeRateForInterval(1000);
On 2016/06/02 11:11:40, åsapersson wrote:
> On 2016/06/02 10:16:20, perkj_webrtc wrote:
> > On 2016/06/02 07:42:37, stefan-webrtc (holmer) wrote:
> > > It seems a bit weird to me that we're getting the frame rate out of the
uma
> > > container. Please add asapersson to review this.
> > 
> > sure- would it be nice if pc.GetStats, uma and internall usage actually
shared
> > the same data instead of creating separate copies in several places? 
> 
> Maybe it is better to return stats_.encode_frame_rate which is used by
> pc.GetStats (since there is a todo in function above).

Done.

[åsapersson, 2016-06-02 12:21:44]

webrtc/send_statistics_proxy lgtm

https://codereview.webrtc.org/1972083002/diff/360001/webrtc/video/send_statis...
File webrtc/video/send_statistics_proxy.h (right):

https://codereview.webrtc.org/1972083002/diff/360001/webrtc/video/send_statis...
webrtc/video/send_statistics_proxy.h:73: int GetSendFrameRate();
const

[stefan-webrtc, 2016-06-02 12:42:26]

lgtm

[perkj_webrtc, 2016-06-02 13:10:00]

https://codereview.webrtc.org/1972083002/diff/360001/webrtc/video/send_statis...
File webrtc/video/send_statistics_proxy.h (right):

https://codereview.webrtc.org/1972083002/diff/360001/webrtc/video/send_statis...
webrtc/video/send_statistics_proxy.h:73: int GetSendFrameRate();
On 2016/06/02 12:21:44, åsapersson wrote:
> const

Done.

[perkj_webrtc, 2016-06-02 13:10:06]

The CQ bit was checked by perkj@webrtc.org

[perkj_webrtc, 2016-06-02 13:10:07]

The patchset sent to the CQ was uploaded after l-g-t-m from
asapersson@webrtc.org, stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1972083002/#ps380001 (title:
"Addressed Åsas comments.")

[commit-bot: I haz the power, 2016-06-02 13:10:13]

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

[perkj_webrtc, 2016-06-02 13:45:58]

Description was changed from

==========
Move logic for calculating needed bitrate overhead used by NACK and FEC to
VideoSender.

This cl split the class MediaOptimization into two parts. One that deals with
frame dropping and stats and one new class called ProtectionBitrateCalculator
that deals with  calculating the needed FEC parameters and how much of the
estimated network bitrate that can be used by an encoder 

Note that the logic of how FEC and the needed bitrates is not changed. 

BUG=webrtc:5687
==========

to

==========
Move logic for calculating needed bitrate overhead used by NACK and FEC to
VideoSender.

This cl split the class MediaOptimization into two parts. One that deals with
frame dropping and stats and one new class called ProtectionBitrateCalculator
that deals with  calculating the needed FEC parameters and how much of the
estimated network bitrate that can be used by an encoder

Note that the logic of how FEC and the needed bitrates is not changed.

BUG=webrtc:5687
R=asapersson@webrtc.org, stefan@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/69b332df8386a0cfde9630a9f...
==========

[perkj_webrtc, 2016-06-02 13:46:00]

Committed patchset #18 (id:380001) manually as
69b332df8386a0cfde9630a9f7fccc4abe452926 (presubmit successful).

[commit-bot: I haz the power, 2016-06-02 13:46:03]

Description was changed from

==========
Move logic for calculating needed bitrate overhead used by NACK and FEC to
VideoSender.

This cl split the class MediaOptimization into two parts. One that deals with
frame dropping and stats and one new class called ProtectionBitrateCalculator
that deals with  calculating the needed FEC parameters and how much of the
estimated network bitrate that can be used by an encoder

Note that the logic of how FEC and the needed bitrates is not changed.

BUG=webrtc:5687
R=asapersson@webrtc.org, stefan@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/69b332df8386a0cfde9630a9f...
==========

to

==========
Move logic for calculating needed bitrate overhead used by NACK and FEC to
VideoSender.

This cl split the class MediaOptimization into two parts. One that deals with
frame dropping and stats and one new class called ProtectionBitrateCalculator
that deals with  calculating the needed FEC parameters and how much of the
estimated network bitrate that can be used by an encoder

Note that the logic of how FEC and the needed bitrates is not changed.

BUG=webrtc:5687
R=asapersson@webrtc.org, stefan@webrtc.org

Committed: https://crrev.com/69b332df8386a0cfde9630a9f7fccc4abe452926
Cr-Commit-Position: refs/heads/master@{#13018}
==========