Instantly pass network changes to controllers in audio network adaptor.

Instantly pass network changes to controllers in audio network adaptor.

This allows controllers to do internal logic upon the network metric changes, e.g., filtering.

BUG=webrtc:6303
Review-Url: https://codereview.webrtc.org/2643133003
Cr-Commit-Position: refs/heads/master@{#16363}
Committed: https://chromium.googlesource.com/external/webrtc/+/a6a6d65278a2194150e0c70547eb649d698b9031

[minyue-webrtc, 2017-01-20 14:17:30]

Description was changed from

==========
Instantly pass network changes to controllers in audio network adaptor.

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

to

==========
Instantly pass network changes to controllers in audio network adaptor.

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

[minyue-webrtc, 2017-01-20 14:17:30]

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

[minyue-webrtc, 2017-01-20 14:23:11]

Hi Michael,

Please take a look at this change.

I wanted to make it smaller, but it is not easy. Given that the change in
controllers are very similar, I think reviewing this won't be difficult.

Note that a follow-up change will be to remove Controller::NetworkMetrics. This
is used in two places: 1) debug dump and 2) controller manager. My idea is to
add APIs to dump writer and controller managers to let them respond to changes
in the respective network dimensions. WDYT?

[minyue-webrtc, 2017-01-20 16:12:16]

On 2017/01/20 14:23:11, minyue-webrtc wrote:
> Hi Michael,
> 
> Please take a look at this change.
> 
> I wanted to make it smaller, but it is not easy. Given that the change in
> controllers are very similar, I think reviewing this won't be difficult.
> 
> Note that a follow-up change will be to remove Controller::NetworkMetrics.
This
> is used in two places: 1) debug dump and 2) controller manager. My idea is to
> add APIs to dump writer and controller managers to let them respond to changes
> in the respective network dimensions. WDYT?

Hi Michael,

Please don't review this now. I think it is better to keep the number of APIs
small by using one API + struct argument. Of course, it would be nice if you
provide your opinion on this.

A new patch set will be uploaded. Then we can discuss which solution to go for.

[minyue-webrtc, 2017-01-20 17:14:45]

Patchset #2 (id:20001) has been deleted

[minyue-webrtc, 2017-01-20 17:23:53]

Michael,

Now finished. You can start review. Take PS2 as an alternative to PS1.

https://codereview.webrtc.org/2643133003/diff/40001/webrtc/modules/audio_codi...
File
webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.h
(right):

https://codereview.webrtc.org/2643133003/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.h:70:
Controller::NetworkMetrics last_metrics_;
I plan to get rid of this also. This means
1) Debug dump will contain one metric at a time
2) ControllerManager will keep its own network states for reordering logic.

[minyue-webrtc, 2017-01-21 19:50:38]

Patchset #2 (id:40001) has been deleted

[michaelt, 2017-01-23 15:15:39]

Should we change the event_log writer according to this change ?

[michaelt, 2017-01-23 15:15:52]

https://codereview.webrtc.org/2643133003/diff/60001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/audio_network_adaptor/fec_controller.cc (left):

https://codereview.webrtc.org/2643133003/diff/60001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/audio_network_adaptor/fec_controller.cc:64:
packet_loss_smoothed_ = std::move(smoothing_filter);
I would prefer to make packet_loss_smoothed_ const

https://codereview.webrtc.org/2643133003/diff/60001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/audio_network_adaptor/fec_controller.h (right):

https://codereview.webrtc.org/2643133003/diff/60001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/audio_network_adaptor/fec_controller.h:97:
std::unique_ptr<SmoothingFilter> packet_loss_smoother_;
i think this can be const

[minyue-webrtc, 2017-01-24 09:44:54]

On 2017/01/23 15:15:39, michaelt wrote:
> Should we change the event_log writer according to this change ?

I don't see what change to make. Does it only record ANA decisions?

[michaelt, 2017-01-24 10:45:08]

Right my fault. :)

[minyue-webrtc, 2017-01-24 14:42:01]

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

[minyue-webrtc, 2017-01-24 14:42:02]

Hi Henrik,

Michael has taken a look. Now it needs review from you.

[hlundin-webrtc, 2017-01-25 08:47:03]

Nice. Just a few minor comments, but LGTM with those answered or fixed.

https://codereview.webrtc.org/2643133003/diff/80001/webrtc/modules/audio_codi...
File
webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl_unittest.cc
(right):

https://codereview.webrtc.org/2643133003/diff/80001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl_unittest.cc:103:
for (auto& mock_controller : states.mock_controllers)
You can break out this for loop into a convenience function (in the unnamed
namespace above) and call it like:
SetMockExpectations(check, &states);

It will save you a few lines of repeated code.

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

https://codereview.webrtc.org/2643133003/diff/80001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/audio_network_adaptor/bitrate_controller_unittest.cc:25:
if (target_audio_bitrate_bps) {
Would it make a difference if you called UpdateNetworkMetrics once with both
metrics (potentially) set? It would save lines in the test, but more
importantly, are both approaches expected to yield the same result?

https://codereview.webrtc.org/2643133003/diff/80001/webrtc/modules/audio_codi...
File
webrtc/modules/audio_coding/audio_network_adaptor/controller_manager_unittest.cc
(right):

https://codereview.webrtc.org/2643133003/diff/80001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/audio_network_adaptor/controller_manager_unittest.cc:307:
// Since controllers are not provide with network metrics, they give the
provide -> provided

https://codereview.webrtc.org/2643133003/diff/80001/webrtc/modules/audio_codi...
File
webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_unittest.cc
(right):

https://codereview.webrtc.org/2643133003/diff/80001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_unittest.cc:79:
if (uplink_bandwidth_bps) {
Same comment here as before: what if both metrics are set in the same call?

[michaelt, 2017-01-25 16:33:27]

lgtm

[minyue-webrtc, 2017-01-26 15:07:58]

https://codereview.webrtc.org/2643133003/diff/80001/webrtc/modules/audio_codi...
File
webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl_unittest.cc
(right):

https://codereview.webrtc.org/2643133003/diff/80001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl_unittest.cc:103:
for (auto& mock_controller : states.mock_controllers)
On 2017/01/25 08:47:03, hlundin-webrtc wrote:
> You can break out this for loop into a convenience function (in the unnamed
> namespace above) and call it like:
> SetMockExpectations(check, &states);
> 
> It will save you a few lines of repeated code.

done, but with a different function name, which I think is more appropriate.

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

https://codereview.webrtc.org/2643133003/diff/80001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/audio_network_adaptor/bitrate_controller_unittest.cc:25:
if (target_audio_bitrate_bps) {
On 2017/01/25 08:47:03, hlundin-webrtc wrote:
> Would it make a difference if you called UpdateNetworkMetrics once with both
> metrics (potentially) set? It would save lines in the test, but more
> importantly, are both approaches expected to yield the same result?

This is a very good question, controller->UpdateNetworkMetrics is suppose to
react on all non-empty fields. Calling it twice with one non-empty field each
should make no difference from calling it ones with the two non-empty fields
combined, for most controller implementations. But it can be potentially
different, e.g., when the controller takes clock into account (for doing
smoothing e.g.)

Currently, the use case is always one non-empty field on each call, since
network change event are independent on the ANA's public interface. To be
consistent with this, we separate the calling to controller.UpdateNetworkMetrics
in most unittests.

I added comments around this. and I added unittests of batched metric update to
relevant controllers, for which, I did some refactoring over the relevant
unittests. PTAL again.

https://codereview.webrtc.org/2643133003/diff/80001/webrtc/modules/audio_codi...
File
webrtc/modules/audio_coding/audio_network_adaptor/controller_manager_unittest.cc
(right):

https://codereview.webrtc.org/2643133003/diff/80001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/audio_network_adaptor/controller_manager_unittest.cc:307:
// Since controllers are not provide with network metrics, they give the
On 2017/01/25 08:47:03, hlundin-webrtc wrote:
> provide -> provided

Done.

https://codereview.webrtc.org/2643133003/diff/80001/webrtc/modules/audio_codi...
File
webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_unittest.cc
(right):

https://codereview.webrtc.org/2643133003/diff/80001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/audio_network_adaptor/fec_controller_unittest.cc:79:
if (uplink_bandwidth_bps) {
On 2017/01/25 08:47:03, hlundin-webrtc wrote:
> Same comment here as before: what if both metrics are set in the same call?

Acknowledged.

[minyue-webrtc, 2017-01-26 15:53:34]

A relatively big update is made here. So may I call for another review? Thanks!

[minyue-webrtc, 2017-01-26 15:54:04]

On 2017/01/26 15:53:34, minyue-webrtc wrote:
> A relatively big update is made here. So may I call for another review?
Thanks!

To add, see Patch set 4.

[hlundin-webrtc, 2017-01-30 08:52:04]

lgtm

[michaelt, 2017-01-30 11:11:25]

lgtm

[minyue-webrtc, 2017-01-30 12:11:27]

The CQ bit was checked by minyue@webrtc.org

[commit-bot: I haz the power, 2017-01-30 12:11:36]

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, 2017-01-30 12:16:46]

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

[commit-bot: I haz the power, 2017-01-30 12:16:47]

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

[minyue-webrtc, 2017-01-30 12:53:30]

Description was changed from

==========
Instantly pass network changes to controllers in audio network adaptor.

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

to

==========
Instantly pass network changes to controllers in audio network adaptor.

This allows controllers to do internal logic upon the network metric changes,
e.g., filtering.

BUG=webrtc;6303
==========

[minyue-webrtc, 2017-01-30 12:53:42]

Description was changed from

==========
Instantly pass network changes to controllers in audio network adaptor.

This allows controllers to do internal logic upon the network metric changes,
e.g., filtering.

BUG=webrtc;6303
==========

to

==========
Instantly pass network changes to controllers in audio network adaptor.

This allows controllers to do internal logic upon the network metric changes,
e.g., filtering.

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

[minyue-webrtc, 2017-01-30 16:25:04]

Sorry, I made some silly mistake. FEC controller unittests have to update again.
This should not affect the validity of the CL.

So I will commit it when bots are green, if you don't bring any concerns.

[minyue-webrtc, 2017-01-30 16:26:42]

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

[commit-bot: I haz the power, 2017-01-30 16:26:51]

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, 2017-01-30 17:04:39]

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

[commit-bot: I haz the power, 2017-01-30 17:04:42]

Dry run: This issue passed the CQ dry run.

[minyue-webrtc, 2017-01-30 18:46:53]

The CQ bit was checked by minyue@webrtc.org

[minyue-webrtc, 2017-01-30 18:46:53]

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/2643133003/#ps140001 (title:
"fixing unittests")

[commit-bot: I haz the power, 2017-01-30 18:47:03]

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, 2017-01-30 18:50:01]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1485802013362550,
"parent_rev": "7b8cddd34cef9c0b2053013cf70a9c37812de17f", "commit_rev":
"a6a6d65278a2194150e0c70547eb649d698b9031"}

[commit-bot: I haz the power, 2017-01-30 18:50:03]

Description was changed from

==========
Instantly pass network changes to controllers in audio network adaptor.

This allows controllers to do internal logic upon the network metric changes,
e.g., filtering.

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

to

==========
Instantly pass network changes to controllers in audio network adaptor.

This allows controllers to do internal logic upon the network metric changes,
e.g., filtering.

BUG=webrtc:6303

Review-Url: https://codereview.webrtc.org/2643133003
Cr-Commit-Position: refs/heads/master@{#16363}
Committed:
https://chromium.googlesource.com/external/webrtc/+/a6a6d65278a2194150e0c7054...
==========

[commit-bot: I haz the power, 2017-01-30 18:50:05]

Committed patchset #6 (id:140001) as
https://chromium.googlesource.com/external/webrtc/+/a6a6d65278a2194150e0c7054...