Adding BitrateController to audio network adaptor.

Adding BitrateController to audio network adaptor.

BUG=webrtc:6303
Committed: https://crrev.com/26b039a137be0a8703766f45b546b29323de714f
Cr-Commit-Position: refs/heads/master@{#14293}

[minyue-webrtc, 2016-09-12 06:23:52]

Description was changed from

==========
reformat


adding missing files


Adding BitrateController to audio network adaptor.

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

to

==========
Adding BitrateController to audio network adaptor.

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

[minyue-webrtc, 2016-09-12 06:23:52]

minyue@webrtc.org changed reviewers:
+ michaelt@webrtc.org

[minyue-webrtc, 2016-09-12 06:24:43]

Hi Michael,

This is not complete but I'd like to listen to your idea about the basic concept
first.

[minyue-webrtc, 2016-09-12 08:30:30]

On 2016/09/12 06:24:43, minyue-webrtc wrote:
> Hi Michael,
> 
> This is not complete but I'd like to listen to your idea about the basic
concept
> first.

Hi Michael,

If you have not started reviewing, look at PS2 instead.

[michaelt, 2016-09-12 10:00:32]

https://codereview.webrtc.org/2334613002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/audio_network_adaptor/bitrate_controller.cc
(right):

https://codereview.webrtc.org/2334613002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/audio_network_adaptor/bitrate_controller.cc:34:
overhead_rate_bps_(kPacketOverheadBits * 1000 /
use initial_overhead_rate_bps_

https://codereview.webrtc.org/2334613002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/audio_network_adaptor/bitrate_controller.cc:47: int
overhead_rate =
use actual_overhead_rate_bps

https://codereview.webrtc.org/2334613002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/audio_network_adaptor/bitrate_controller.cc:60: //
TODO(minyue): apply a smoothing on the |overhead_rate_bps_|.
in the actual workaround impl no overhead_rate_bps_ is soothed.
Its the diff between actual and initial overhead which is smoothed.

[minyue-webrtc, 2016-09-12 10:51:52]

Thanks for the comments! see my reply.

https://codereview.webrtc.org/2334613002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/audio_network_adaptor/bitrate_controller.cc
(right):

https://codereview.webrtc.org/2334613002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/audio_network_adaptor/bitrate_controller.cc:34:
overhead_rate_bps_(kPacketOverheadBits * 1000 /
On 2016/09/12 10:00:31, michaelt wrote:
> use initial_overhead_rate_bps_

I am not sure that using initial_overhead_rate_bps_ makes it clearer. Since 
1) overhead_rate is related to particular implementation of
target_audio_bit_rate, and therefore, it would be better to use a implementation
independent term
2) we calculate overhead_rate in MakeDecision by frame length, it would better
keep consistency.

https://codereview.webrtc.org/2334613002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/audio_network_adaptor/bitrate_controller.cc:47: int
overhead_rate =
On 2016/09/12 10:00:31, michaelt wrote:
> use actual_overhead_rate_bps

I think it new_overhead_rate could make better sense, but I think the context is
clear enough to understand overhead_rate. Local variables can be better made
short as long as it is clear.

https://codereview.webrtc.org/2334613002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/audio_network_adaptor/bitrate_controller.cc:60: //
TODO(minyue): apply a smoothing on the |overhead_rate_bps_|.
On 2016/09/12 10:00:31, michaelt wrote:
> in the actual workaround impl no overhead_rate_bps_ is soothed.
> Its the diff between actual and initial overhead which is smoothed.    

right. I chose overhead_rate since I think 
1) it is easier to understand and
2) it is easier to remove when the work on including header in target_bitrate 
(bug 6315)  is fixed.

[minyue-webrtc, 2016-09-13 09:37:16]

Patchset #1 (id:1) has been deleted

[minyue-webrtc, 2016-09-13 09:38:17]

Description was changed from

==========
Adding BitrateController to audio network adaptor.

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

to

==========
Adding BitrateController to audio network adaptor.

BUG=webrtc:6303
==========

[minyue-webrtc, 2016-09-13 09:38:43]

minyue@webrtc.org changed reviewers:
+ henrik.lundin@webrtc.org

[minyue-webrtc, 2016-09-13 09:41:11]

+Henrik

Hi Henrik,

Michael has started reviewing this CL and had no major concerns. I add you now
to borrow another pair of eyes on it.

[minyue-webrtc, 2016-09-13 09:41:31]

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

[commit-bot: I haz the power, 2016-09-13 09:41:33]

Dry run: 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-09-13 11:33:58]

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

[commit-bot: I haz the power, 2016-09-13 11:33:59]

Dry run: This issue passed the CQ dry run.

[hlundin-webrtc, 2016-09-15 11:01:09]

LG. A few questions and nits.

https://codereview.webrtc.org/2334613002/diff/40001/webrtc/modules/BUILD.gn
File webrtc/modules/BUILD.gn (right):

https://codereview.webrtc.org/2334613002/diff/40001/webrtc/modules/BUILD.gn#n...
webrtc/modules/BUILD.gn:239: rtc_source_set("audio_network_adaptor_unittests") {
What is the benefit of breaking this out of the modules_unittests?

https://codereview.webrtc.org/2334613002/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/audio_network_adaptor/bitrate_controller.cc
(right):

https://codereview.webrtc.org/2334613002/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/audio_network_adaptor/bitrate_controller.cc:13:
#include
"webrtc/modules/audio_coding/audio_network_adaptor/bitrate_controller.h"
Goes before any other #includes.

https://codereview.webrtc.org/2334613002/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/audio_network_adaptor/bitrate_controller.cc:20: //
L2(14B) + IPv4(20B) + UDP(8B) + RTP(12B) + SRTP_AUTH(10B) = 64B = 512 bits
This is dependent on transport type. Can it be made settable from higher layers?

https://codereview.webrtc.org/2334613002/diff/40001/webrtc/modules/audio_codi...
File
webrtc/modules/audio_coding/audio_network_adaptor/bitrate_controller_unittest.cc
(right):

https://codereview.webrtc.org/2334613002/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/audio_network_adaptor/bitrate_controller_unittest.cc:28:
EXPECT_EQ(config.bitrate_bps, rtc::Optional<int>(expected_bitrate_bps));
Swap parameters. Expected value should go before the test value.

[minyue-webrtc, 2016-09-15 12:12:15]

Hi Henrik,

Thanks for the comments!

I made some changes and answered your questions.

I expect to made another polishing to the unittests. It will come in PS4. You
may take a look at PS3 first.

https://codereview.webrtc.org/2334613002/diff/40001/webrtc/modules/BUILD.gn
File webrtc/modules/BUILD.gn (right):

https://codereview.webrtc.org/2334613002/diff/40001/webrtc/modules/BUILD.gn#n...
webrtc/modules/BUILD.gn:239: rtc_source_set("audio_network_adaptor_unittests") {
On 2016/09/15 11:01:09, hlundin-webrtc wrote:
> What is the benefit of breaking this out of the modules_unittests?

the title of patch set 2 mentioned this. To be clearer, this is because
bitrate_controller_unittest.cc exists in modules/bitrate_controller but GN does
not allow this type of collision. GN's suggestion is to put it in a  separate
source set, according to the error message:
"
... either rename one of the files or move one of
the sources to a separate source_set to avoid them both being in
the same target.
"
A separate source_set also looks nicer.

https://codereview.webrtc.org/2334613002/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/audio_network_adaptor/bitrate_controller.cc
(right):

https://codereview.webrtc.org/2334613002/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/audio_network_adaptor/bitrate_controller.cc:13:
#include
"webrtc/modules/audio_coding/audio_network_adaptor/bitrate_controller.h"
On 2016/09/15 11:01:09, hlundin-webrtc wrote:
> Goes before any other #includes.

oh, really! I have been putting <xxx> at the top. will pay attention in the
future.

https://codereview.webrtc.org/2334613002/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/audio_network_adaptor/bitrate_controller.cc:20: //
L2(14B) + IPv4(20B) + UDP(8B) + RTP(12B) + SRTP_AUTH(10B) = 64B = 512 bits
On 2016/09/15 11:01:09, hlundin-webrtc wrote:
> This is dependent on transport type. Can it be made settable from higher
layers?

I'd prefer to keep it local since there is an effort in moving it to BWE, then
bitrate controller won't matter, so no API need to add.

https://codereview.webrtc.org/2334613002/diff/40001/webrtc/modules/audio_codi...
File
webrtc/modules/audio_coding/audio_network_adaptor/bitrate_controller_unittest.cc
(right):

https://codereview.webrtc.org/2334613002/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/audio_network_adaptor/bitrate_controller_unittest.cc:28:
EXPECT_EQ(config.bitrate_bps, rtc::Optional<int>(expected_bitrate_bps));
On 2016/09/15 11:01:09, hlundin-webrtc wrote:
> Swap parameters. Expected value should go before the test value.

Ok. There can be a bit more polishing to be made in this test (as the same to
brother CLs), but I will make it in a separate patch set.

[minyue-webrtc, 2016-09-15 14:38:01]

Hi,

Here comes my polishing of the unittests in Patch set 4. Please take a look.
Thanks!

[hlundin-webrtc, 2016-09-19 11:08:57]

One nit in the GN file. Then LGTM.

https://codereview.webrtc.org/2334613002/diff/40001/webrtc/modules/BUILD.gn
File webrtc/modules/BUILD.gn (right):

https://codereview.webrtc.org/2334613002/diff/40001/webrtc/modules/BUILD.gn#n...
webrtc/modules/BUILD.gn:239: rtc_source_set("audio_network_adaptor_unittests") {
On 2016/09/15 12:12:15, minyue-webrtc wrote:
> On 2016/09/15 11:01:09, hlundin-webrtc wrote:
> > What is the benefit of breaking this out of the modules_unittests?
> 
> the title of patch set 2 mentioned this. To be clearer, this is because
> bitrate_controller_unittest.cc exists in modules/bitrate_controller but GN
does
> not allow this type of collision. GN's suggestion is to put it in a  separate
> source set, according to the error message:
> "
> ... either rename one of the files or move one of
> the sources to a separate source_set to avoid them both being in
> the same target.
> "
> A separate source_set also looks nicer.

Oh, I see. Good. Can you just add a comment explaining this in the gn file?

https://codereview.webrtc.org/2334613002/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/audio_network_adaptor/bitrate_controller.cc
(right):

https://codereview.webrtc.org/2334613002/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/audio_network_adaptor/bitrate_controller.cc:13:
#include
"webrtc/modules/audio_coding/audio_network_adaptor/bitrate_controller.h"
On 2016/09/15 12:12:15, minyue-webrtc wrote:
> On 2016/09/15 11:01:09, hlundin-webrtc wrote:
> > Goes before any other #includes.
> 
> oh, really! I have been putting <xxx> at the top. will pay attention in the
> future.

Yes. Include foo.h first of all in foo.cc.
https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes

https://codereview.webrtc.org/2334613002/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/audio_network_adaptor/bitrate_controller.cc:20: //
L2(14B) + IPv4(20B) + UDP(8B) + RTP(12B) + SRTP_AUTH(10B) = 64B = 512 bits
On 2016/09/15 12:12:15, minyue-webrtc wrote:
> On 2016/09/15 11:01:09, hlundin-webrtc wrote:
> > This is dependent on transport type. Can it be made settable from higher
> layers?
> 
> I'd prefer to keep it local since there is an effort in moving it to BWE, then
> bitrate controller won't matter, so no API need to add.

Acknowledged.

[michaelt, 2016-09-19 12:18:46]

lgtm

https://codereview.webrtc.org/2334613002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/audio_network_adaptor/bitrate_controller.cc
(right):

https://codereview.webrtc.org/2334613002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/audio_network_adaptor/bitrate_controller.cc:47: int
overhead_rate =
On 2016/09/12 10:51:52, minyue-webrtc wrote:
> On 2016/09/12 10:00:31, michaelt wrote:
> > use actual_overhead_rate_bps
> 
> I think it new_overhead_rate could make better sense, but I think the context
is
> clear enough to understand overhead_rate. Local variables can be better made
> short as long as it is clear.

Acknowledged.

[minyue-webrtc, 2016-09-19 13:16:30]

https://codereview.webrtc.org/2334613002/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/audio_network_adaptor/bitrate_controller.cc
(right):

https://codereview.webrtc.org/2334613002/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/audio_network_adaptor/bitrate_controller.cc:20: //
L2(14B) + IPv4(20B) + UDP(8B) + RTP(12B) + SRTP_AUTH(10B) = 64B = 512 bits
On 2016/09/19 11:08:57, hlundin-webrtc wrote:
> On 2016/09/15 12:12:15, minyue-webrtc wrote:
> > On 2016/09/15 11:01:09, hlundin-webrtc wrote:
> > > This is dependent on transport type. Can it be made settable from higher
> > layers?
> > 
> > I'd prefer to keep it local since there is an effort in moving it to BWE,
then
> > bitrate controller won't matter, so no API need to add.
> 
> Acknowledged.

I discussed this with Michael again and realized that even with the planned work
in BWE, it would still be good for BitrateController to know and header size,
and the header size is better passed down from a higher level, preferably
through Controller::SetConstraints(). I added it a TODO here in the last patch
set, so as to keep this CL clean.

[minyue-webrtc, 2016-09-19 13:17:25]

The CQ bit was checked by minyue@webrtc.org

[minyue-webrtc, 2016-09-19 13:17:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
henrik.lundin@webrtc.org, michaelt@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2334613002/#ps140001 (title:
"adding a TODO")

[commit-bot: I haz the power, 2016-09-19 13:17:34]

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-09-19 15:18:10]

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

[commit-bot: I haz the power, 2016-09-19 15:18:11]

Try jobs failed on following builders:
  android_dbg on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[minyue-webrtc, 2016-09-19 15:19:33]

The CQ bit was checked by minyue@webrtc.org

[commit-bot: I haz the power, 2016-09-19 15:19:41]

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-09-19 16:56:37]

Description was changed from

==========
Adding BitrateController to audio network adaptor.

BUG=webrtc:6303
==========

to

==========
Adding BitrateController to audio network adaptor.

BUG=webrtc:6303
==========

[commit-bot: I haz the power, 2016-09-19 16:56:38]

Committed patchset #7 (id:140001)

[commit-bot: I haz the power, 2016-09-19 16:56:48]

Description was changed from

==========
Adding BitrateController to audio network adaptor.

BUG=webrtc:6303
==========

to

==========
Adding BitrateController to audio network adaptor.

BUG=webrtc:6303

Committed: https://crrev.com/26b039a137be0a8703766f45b546b29323de714f
Cr-Commit-Position: refs/heads/master@{#14293}
==========

[commit-bot: I haz the power, 2016-09-19 16:56:49]

Patchset 7 (id:??) landed as
https://crrev.com/26b039a137be0a8703766f45b546b29323de714f
Cr-Commit-Position: refs/heads/master@{#14293}

[krasin1, 2016-09-20 21:22:25]

krasin@chromium.org changed reviewers:
+ krasin@chromium.org

[krasin1, 2016-09-20 21:22:26]

https://codereview.webrtc.org/2334613002/diff/140001/webrtc/modules/audio_cod...
File webrtc/modules/audio_coding/audio_network_adaptor/bitrate_controller.h
(right):

https://codereview.webrtc.org/2334613002/diff/140001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/audio_network_adaptor/bitrate_controller.h:19: class
BitrateController final : public Controller {
For the reference, this change has caused an ODR violation, since there's
another webrtc::BitrateController defined in
webrtc/modules/bitrate_controller/include/bitrate_controller.h

https://cs.chromium.org/chromium/src/third_party/webrtc/modules/bitrate_contr...

[minyue-webrtc, 2016-09-20 21:34:53]

A revert of this CL (patchset #7 id:140001) has been created in
https://codereview.webrtc.org/2352223002/ by minyue@webrtc.org.

The reason for reverting is: ODR violation.