Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
3007983002:
Implement ANA statistics. (Closed)
DescriptionImplement ANA statistics.
This CL also makes it possible to enable/prevent ANA controllers from making adaptations using field trials.
BUG=webrtc:8127
Review-Url: https://codereview.webrtc.org/3007983002
Cr-Commit-Position: refs/heads/master@{#19761}
Committed: https://chromium.googlesource.com/external/webrtc/+/17289097f08280b377c4ca550e8d75e46ffcede3
Patch Set 1 #
Total comments: 8
Patch Set 2 : Review comments. #Patch Set 3 : Changed upstream dependency. #Patch Set 4 : Implemented additional stats. #
Total comments: 7
Patch Set 5 : Review comments. #Patch Set 6 : Fix for failing unittests. #
Depends on Patchset: Messages
Total messages: 25 (11 generated)
Description was changed from ========== Implement ANA statistics. This CL also makes it possible to enable/prevent ANA controllers from making adaptations using field trials. BUG= ========== to ========== Implement ANA statistics. This CL also makes it possible to enable/prevent ANA controllers from making adaptations using field trials. BUG=webrtc:8127 ==========
ivoc@webrtc.org changed reviewers: + alexnarest@webrtc.org, ossu@webrtc.org
Hi guys, PTAL at this CL to implement ANA statistics and add support for field trial flags to enable/disable adaptation for sub-controllers.
minyue@webrtc.org changed reviewers: + minyue@webrtc.org
https://codereview.webrtc.org/3007983002/diff/1/webrtc/modules/audio_coding/a... File webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.h (right): https://codereview.webrtc.org/3007983002/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.h:82: bool enable_bitrate_adaptation_ = true; My comment: add const, then you need to 1. no assignment here (natural follow-up of 1) 2. put these in initializer list
https://codereview.webrtc.org/3007983002/diff/1/webrtc/modules/audio_coding/a... File webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.cc (right): https://codereview.webrtc.org/3007983002/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.cc:147: if (config.enable_fec != prev_config_->enable_fec) { In addition I think we want to record the fact FEC controller smoothed the PL or in case of RPLR-FEC sent RPL instead of RTCP PL in the control group. I suggest we increment here on FEC enable or PL is greater than 1%. https://codereview.webrtc.org/3007983002/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.cc:166: if (!enable_fec_adaptation_ && config.enable_fec) { In addition to enable_fec we want to prevent PL smoothing and RPL reporting in case of RPLR-FEC
Thanks for the comments. https://codereview.webrtc.org/3007983002/diff/1/webrtc/modules/audio_coding/a... File webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.cc (right): https://codereview.webrtc.org/3007983002/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.cc:147: if (config.enable_fec != prev_config_->enable_fec) { On 2017/09/04 11:24:02, alexnarest wrote: > In addition I think we want to record the fact FEC controller smoothed the PL or > in case of RPLR-FEC sent RPL instead of RTCP PL in the control group. > I suggest we increment here on FEC enable or PL is greater than 1%. Another option is to add another stat for uplink packet loss fraction, what do you think? It could also be implemented as a cumulative stat since the start of the call, which would make it possible to calculate the average value between the GetStats() calls. Would that be useful to have? https://codereview.webrtc.org/3007983002/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.cc:166: if (!enable_fec_adaptation_ && config.enable_fec) { On 2017/09/04 11:24:01, alexnarest wrote: > In addition to enable_fec we want to prevent PL smoothing and RPL reporting in > case of RPLR-FEC Good point, done. https://codereview.webrtc.org/3007983002/diff/1/webrtc/modules/audio_coding/a... File webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.h (right): https://codereview.webrtc.org/3007983002/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.h:82: bool enable_bitrate_adaptation_ = true; On 2017/09/01 14:52:13, minyue-webrtc(BackIn2018March) wrote: > My comment: > > add const, > > then you need to > 1. no assignment here (natural follow-up of 1) > 2. put these in initializer list Done
https://codereview.webrtc.org/3007983002/diff/1/webrtc/modules/audio_coding/a... File webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.cc (right): https://codereview.webrtc.org/3007983002/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.cc:147: if (config.enable_fec != prev_config_->enable_fec) { On 2017/09/05 09:01:40, ivoc wrote: > On 2017/09/04 11:24:02, alexnarest wrote: > > In addition I think we want to record the fact FEC controller smoothed the PL > or > > in case of RPLR-FEC sent RPL instead of RTCP PL in the control group. > > I suggest we increment here on FEC enable or PL is greater than 1%. > > Another option is to add another stat for uplink packet loss fraction, what do > you think? It could also be implemented as a cumulative stat since the start of > the call, which would make it possible to calculate the average value between > the GetStats() calls. Would that be useful to have? Yes, I think it is a good idea https://codereview.webrtc.org/3007983002/diff/1/webrtc/modules/audio_coding/a... webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.cc:150: if (config.frame_length_ms != prev_config_->frame_length_ms) { It would be helpful to report inc/dec counters rather than the general change counter.
I just added the implementation of the new stats that are introduced in https://codereview.webrtc.org/3007243002/ to this CL. PTAL (preferably before monday!)
lgtm
A few comments https://codereview.webrtc.org/3007983002/diff/60001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.cc (right): https://codereview.webrtc.org/3007983002/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.cc:137: a = rtc::Optional<uint32_t>(1); I think you could just do a = a.value_or(0) + 1 as the implementation of increment_opt. https://codereview.webrtc.org/3007983002/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.cc:161: static_cast<double>(*config.uplink_packet_loss_fraction)); Why does this need a static_cast? Why isn't uplink_packet_loss_fraction the same type in both? https://codereview.webrtc.org/3007983002/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.cc:168: config.bitrate_bps = rtc::Optional<int>(); You can use .reset() to empty Optionals and save on typing. (Especially having to fully qualify the template type).
Thanks! https://codereview.webrtc.org/3007983002/diff/60001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.cc (right): https://codereview.webrtc.org/3007983002/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.cc:137: a = rtc::Optional<uint32_t>(1); On 2017/09/08 10:56:01, ossu wrote: > I think you could just do a = a.value_or(0) + 1 as the implementation of > increment_opt. Nice! Done, although I needed to wrap the result in an optional to make it work. https://codereview.webrtc.org/3007983002/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.cc:161: static_cast<double>(*config.uplink_packet_loss_fraction)); On 2017/09/08 10:56:01, ossu wrote: > Why does this need a static_cast? Why isn't uplink_packet_loss_fraction the same > type in both? That's a good point, I changed the stat into a float so this is no longer needed. https://codereview.webrtc.org/3007983002/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.cc:168: config.bitrate_bps = rtc::Optional<int>(); On 2017/09/08 10:56:01, ossu wrote: > You can use .reset() to empty Optionals and save on typing. (Especially having > to fully qualify the template type). Done!
LGTM! https://codereview.webrtc.org/3007983002/diff/60001/webrtc/modules/audio_codi... File webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.cc (right): https://codereview.webrtc.org/3007983002/diff/60001/webrtc/modules/audio_codi... webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.cc:137: a = rtc::Optional<uint32_t>(1); On 2017/09/08 11:43:47, ivoc wrote: > On 2017/09/08 10:56:01, ossu wrote: > > I think you could just do a = a.value_or(0) + 1 as the implementation of > > increment_opt. > > Nice! Done, although I needed to wrap the result in an optional to make it work. Ah, that's right. I'll try to look at getting a newer version of Optional from Chromium, so that we can avoid having to explicitly construct Optionals like this in future.
Patchset #6 (id:100001) has been deleted
The CQ bit was checked by ivoc@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from alexnarest@webrtc.org Link to the patchset: https://codereview.webrtc.org/3007983002/#ps80001 (title: "Review comments.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_ubsan_vptr on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan_vptr/builds...)
The CQ bit was checked by ivoc@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from ossu@webrtc.org, alexnarest@webrtc.org Link to the patchset: https://codereview.webrtc.org/3007983002/#ps120001 (title: "Fix for failing unittests.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1504970178400600,
"parent_rev": "a37de39216cccecb1448725b7dbd00bb69c52360", "commit_rev":
"17289097f08280b377c4ca550e8d75e46ffcede3"}
Message was sent while issue was closed.
Description was changed from ========== Implement ANA statistics. This CL also makes it possible to enable/prevent ANA controllers from making adaptations using field trials. BUG=webrtc:8127 ========== to ========== Implement ANA statistics. This CL also makes it possible to enable/prevent ANA controllers from making adaptations using field trials. BUG=webrtc:8127 Review-Url: https://codereview.webrtc.org/3007983002 Cr-Commit-Position: refs/heads/master@{#19761} Committed: https://chromium.googlesource.com/external/webrtc/+/17289097f08280b377c4ca550... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:120001) as https://chromium.googlesource.com/external/webrtc/+/17289097f08280b377c4ca550... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
