Add new ANA stats to GetStats() to count the number of actions taken by each controller.

Add new ANA stats to the old GetStats() to count the number of actions taken by each controller.

This CL only wires up the new stats but does not set the values yet. This will be added in a follow-up CL.

BUG=webrtc:8127
Review-Url: https://codereview.webrtc.org/3011623002
Cr-Commit-Position: refs/heads/master@{#19751}
Committed: https://chromium.googlesource.com/external/webrtc/+/e1198e068bdcb01dcb8799d6871018092711714e

[ivoc, 2017-08-30 14:53:51]

Description was changed from

==========
Add new ANA stats to GetStats() to count the number of actions taken by each
controller.

BUG=webrtc:8127
==========

to

==========
Add new ANA stats to the old GetStats() to count the number of actions taken by
each controller.

This CL only wires up the new stat but does not set the values yet. This will be
added in a follow-up CL.

BUG=webrtc:8127
==========

[ivoc, 2017-08-30 14:53:51]

ivoc@webrtc.org changed reviewers:
+ hbos@webrtc.org, minyue@webrtc.org, ossu@webrtc.org, solenberg@webrtc.org

[ivoc, 2017-08-30 15:04:15]

Description was changed from

==========
Add new ANA stats to the old GetStats() to count the number of actions taken by
each controller.

This CL only wires up the new stat but does not set the values yet. This will be
added in a follow-up CL.

BUG=webrtc:8127
==========

to

==========
Add new ANA stats to the old GetStats() to count the number of actions taken by
each controller.

This CL only wires up the new stats but does not set the values yet. This will
be added in a follow-up CL.

BUG=webrtc:8127
==========

[ivoc, 2017-08-30 15:05:36]

Hi guys, PTAL at this CL to add some useful new stats related to ANA.

[minyue-webrtc, 2017-08-31 07:24:21]

https://codereview.webrtc.org/3011623002/diff/1/webrtc/modules/audio_coding/i...
File webrtc/modules/audio_coding/include/audio_coding_module.h (right):

https://codereview.webrtc.org/3011623002/diff/1/webrtc/modules/audio_coding/i...
webrtc/modules/audio_coding/include/audio_coding_module.h:827: virtual
AudioEncoder::AudioEncoderStats GetAudioEncoderStatistics() const = 0;
I think it is better to do similar to GetDecodingCallStatistics()

[ossu, 2017-08-31 13:13:10]

Comments ahoy!

https://codereview.webrtc.org/3011623002/diff/1/webrtc/api/audio_codecs/audio...
File webrtc/api/audio_codecs/audio_encoder.h (right):

https://codereview.webrtc.org/3011623002/diff/1/webrtc/api/audio_codecs/audio...
webrtc/api/audio_codecs/audio_encoder.h:88: rtc::Optional<int>
ana_bitrate_action_counter;
Are all these individually optional? 

Since they're counters, it would seem to me they're not optional at all: is a
missing value different from a count of zero? Otherwise they should all just be
ints.

https://codereview.webrtc.org/3011623002/diff/1/webrtc/modules/audio_coding/c...
File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc (right):

https://codereview.webrtc.org/3011623002/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:779: auto stats =
AudioEncoderStats();
No. :)

https://codereview.webrtc.org/3011623002/diff/1/webrtc/modules/audio_coding/i...
File webrtc/modules/audio_coding/include/audio_coding_module.h (right):

https://codereview.webrtc.org/3011623002/diff/1/webrtc/modules/audio_coding/i...
webrtc/modules/audio_coding/include/audio_coding_module.h:827: virtual
AudioEncoder::AudioEncoderStats GetAudioEncoderStatistics() const = 0;
On 2017/08/31 07:24:21, minyue-webrtc wrote:
> I think it is better to do similar to GetDecodingCallStatistics()

Why do you think so? Functions with output parameters are much more unwieldy to
call and only has a significant benefit in a few special cases (i.e. having it
fill in a (large) caller-managed buffer).

For this function, the copy could (and very probably will) be elided thanks to
Named Return Value Optimization.

[minyue-webrtc, 2017-08-31 13:20:17]

https://codereview.webrtc.org/3011623002/diff/1/webrtc/modules/audio_coding/i...
File webrtc/modules/audio_coding/include/audio_coding_module.h (right):

https://codereview.webrtc.org/3011623002/diff/1/webrtc/modules/audio_coding/i...
webrtc/modules/audio_coding/include/audio_coding_module.h:827: virtual
AudioEncoder::AudioEncoderStats GetAudioEncoderStatistics() const = 0;
On 2017/08/31 13:13:10, ossu wrote:
> On 2017/08/31 07:24:21, minyue-webrtc wrote:
> > I think it is better to do similar to GetDecodingCallStatistics()
> 
> Why do you think so? Functions with output parameters are much more unwieldy
to
> call and only has a significant benefit in a few special cases (i.e. having it
> fill in a (large) caller-managed buffer).
> 
> For this function, the copy could (and very probably will) be elided thanks to
> Named Return Value Optimization.

Copying is fine with me, and I understand the merits of not passing pointers.
What I would prefer, though is to let stats defined from top, since stats are
often defined in specs.

[hbos, 2017-08-31 13:29:32]

I'm busy sheriffing and other high priority things, I'll probably be able to
take a look tomorrow.

[ivoc, 2017-08-31 14:57:50]

Thanks for the comments!

https://codereview.webrtc.org/3011623002/diff/1/webrtc/api/audio_codecs/audio...
File webrtc/api/audio_codecs/audio_encoder.h (right):

https://codereview.webrtc.org/3011623002/diff/1/webrtc/api/audio_codecs/audio...
webrtc/api/audio_codecs/audio_encoder.h:88: rtc::Optional<int>
ana_bitrate_action_counter;
On 2017/08/31 13:13:10, ossu wrote:
> Are all these individually optional? 
> 
> Since they're counters, it would seem to me they're not optional at all: is a
> missing value different from a count of zero? Otherwise they should all just
be
> ints.

I think some Optionality is needed here, since ANA can be completely disabled,
and in that case we don't want to report these stats. We want to be able to tell
a difference between calls with 0 actions (this means good enough network
conditions that ANA is not needed), and calls where ANA was completely disabled.
However, I guess it's possible to put all of the int's into a struct, and make
that struct Optional. Do you think that would be better?

https://codereview.webrtc.org/3011623002/diff/1/webrtc/modules/audio_coding/c...
File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc (right):

https://codereview.webrtc.org/3011623002/diff/1/webrtc/modules/audio_coding/c...
webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:779: auto stats =
AudioEncoderStats();
On 2017/08/31 13:13:10, ossu wrote:
> No. :)

Oops. :)

https://codereview.webrtc.org/3011623002/diff/1/webrtc/modules/audio_coding/i...
File webrtc/modules/audio_coding/include/audio_coding_module.h (right):

https://codereview.webrtc.org/3011623002/diff/1/webrtc/modules/audio_coding/i...
webrtc/modules/audio_coding/include/audio_coding_module.h:827: virtual
AudioEncoder::AudioEncoderStats GetAudioEncoderStatistics() const = 0;
On 2017/08/31 13:20:17, minyue-webrtc wrote:
> On 2017/08/31 13:13:10, ossu wrote:
> > On 2017/08/31 07:24:21, minyue-webrtc wrote:
> > > I think it is better to do similar to GetDecodingCallStatistics()
> > 
> > Why do you think so? Functions with output parameters are much more unwieldy
> to
> > call and only has a significant benefit in a few special cases (i.e. having
it
> > fill in a (large) caller-managed buffer).
> > 
> > For this function, the copy could (and very probably will) be elided thanks
to
> > Named Return Value Optimization.
> 
> Copying is fine with me, and I understand the merits of not passing pointers.
> What I would prefer, though is to let stats defined from top, since stats are
> often defined in specs.

I agree with ossu on return values vs passing pointers, but I'm not sure I
understand what minyue is arguing for. Minyue: do you think this function should
pass in the stats struct by pointer or that the stats struct should be defined
in common_types.h like AudioDecodingCallStats, or both?

[minyue-webrtc, 2017-08-31 15:18:08]

On 2017/08/31 14:57:50, ivoc wrote:
> Thanks for the comments!
> 
>
https://codereview.webrtc.org/3011623002/diff/1/webrtc/api/audio_codecs/audio...
> File webrtc/api/audio_codecs/audio_encoder.h (right):
> 
>
https://codereview.webrtc.org/3011623002/diff/1/webrtc/api/audio_codecs/audio...
> webrtc/api/audio_codecs/audio_encoder.h:88: rtc::Optional<int>
> ana_bitrate_action_counter;
> On 2017/08/31 13:13:10, ossu wrote:
> > Are all these individually optional? 
> > 
> > Since they're counters, it would seem to me they're not optional at all: is
a
> > missing value different from a count of zero? Otherwise they should all just
> be
> > ints.
> 
> I think some Optionality is needed here, since ANA can be completely disabled,
> and in that case we don't want to report these stats. We want to be able to
tell
> a difference between calls with 0 actions (this means good enough network
> conditions that ANA is not needed), and calls where ANA was completely
disabled.
> However, I guess it's possible to put all of the int's into a struct, and make
> that struct Optional. Do you think that would be better?
> 
>
https://codereview.webrtc.org/3011623002/diff/1/webrtc/modules/audio_coding/c...
> File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc (right):
> 
>
https://codereview.webrtc.org/3011623002/diff/1/webrtc/modules/audio_coding/c...
> webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:779: auto stats
=
> AudioEncoderStats();
> On 2017/08/31 13:13:10, ossu wrote:
> > No. :)
> 
> Oops. :)
> 
>
https://codereview.webrtc.org/3011623002/diff/1/webrtc/modules/audio_coding/i...
> File webrtc/modules/audio_coding/include/audio_coding_module.h (right):
> 
>
https://codereview.webrtc.org/3011623002/diff/1/webrtc/modules/audio_coding/i...
> webrtc/modules/audio_coding/include/audio_coding_module.h:827: virtual
> AudioEncoder::AudioEncoderStats GetAudioEncoderStatistics() const = 0;
> On 2017/08/31 13:20:17, minyue-webrtc wrote:
> > On 2017/08/31 13:13:10, ossu wrote:
> > > On 2017/08/31 07:24:21, minyue-webrtc wrote:
> > > > I think it is better to do similar to GetDecodingCallStatistics()
> > > 
> > > Why do you think so? Functions with output parameters are much more
unwieldy
> > to
> > > call and only has a significant benefit in a few special cases (i.e.
having
> it
> > > fill in a (large) caller-managed buffer).
> > > 
> > > For this function, the copy could (and very probably will) be elided
thanks
> to
> > > Named Return Value Optimization.
> > 
> > Copying is fine with me, and I understand the merits of not passing
pointers.
> > What I would prefer, though is to let stats defined from top, since stats
are
> > often defined in specs.
> 
> I agree with ossu on return values vs passing pointers, but I'm not sure I
> understand what minyue is arguing for. Minyue: do you think this function
should
> pass in the stats struct by pointer or that the stats struct should be defined
> in common_types.h like AudioDecodingCallStats, or both?

Sorry if I made it unclear. I meant the latter.

[ossu, 2017-08-31 15:40:20]

On 2017/08/31 15:18:08, minyue-webrtc wrote:
> On 2017/08/31 14:57:50, ivoc wrote:
> > Minyue: do you think this function should
> > pass in the stats struct by pointer or that the stats struct should be
defined
> > in common_types.h like AudioDecodingCallStats, or both?
> 
> Sorry if I made it unclear. I meant the latter.

Ah, then I understand. I guess that makes sense.

https://codereview.webrtc.org/3011623002/diff/1/webrtc/api/audio_codecs/audio...
File webrtc/api/audio_codecs/audio_encoder.h (right):

https://codereview.webrtc.org/3011623002/diff/1/webrtc/api/audio_codecs/audio...
webrtc/api/audio_codecs/audio_encoder.h:88: rtc::Optional<int>
ana_bitrate_action_counter;
On 2017/08/31 14:57:50, ivoc wrote:
> On 2017/08/31 13:13:10, ossu wrote:
> > Are all these individually optional? 
> > 
> > Since they're counters, it would seem to me they're not optional at all: is
a
> > missing value different from a count of zero? Otherwise they should all just
> be
> > ints.
> 
> I think some Optionality is needed here, since ANA can be completely disabled,
> and in that case we don't want to report these stats. We want to be able to
tell
> a difference between calls with 0 actions (this means good enough network
> conditions that ANA is not needed), and calls where ANA was completely
disabled.
> However, I guess it's possible to put all of the int's into a struct, and make
> that struct Optional. Do you think that would be better?

1. If ANA being enabled or not is one statistic we want to communicate, then
maybe that's just a separate bool field?
2. If AudioEncoderStats is going to be either "empty" (ANA disabled) or full of
values (ANA enabled), then I think the functions returning AudioEncoderStats
should return rtc::Optional<AudioEncoderStats>.

Both of these depend on the future direction of this statistics struct. Is it
planned to, in future, contain other stats that aren't ANA specific? If not,
perhaps rename it to ANAStats (or something like that). Then having something
return an Optional<ANAStats> makes it quite clear that if there's no ANA
enabled, there'll be no stats for it. If it's conceivable that AudioEncoderStats
will at some point contain other information, but there's nothing planned right
now, then going the Optional<ANAStats> route also makes sense; Once more stats
are needed, the Optional<ANAStats> can be put inside the , then new,
AudioEncoderStats struct.

[ivoc, 2017-09-01 09:34:24]

https://codereview.webrtc.org/3011623002/diff/1/webrtc/api/audio_codecs/audio...
File webrtc/api/audio_codecs/audio_encoder.h (right):

https://codereview.webrtc.org/3011623002/diff/1/webrtc/api/audio_codecs/audio...
webrtc/api/audio_codecs/audio_encoder.h:88: rtc::Optional<int>
ana_bitrate_action_counter;
On 2017/08/31 15:40:20, ossu wrote:
> On 2017/08/31 14:57:50, ivoc wrote:
> > On 2017/08/31 13:13:10, ossu wrote:
> > > Are all these individually optional? 
> > > 
> > > Since they're counters, it would seem to me they're not optional at all:
is
> a
> > > missing value different from a count of zero? Otherwise they should all
just
> > be
> > > ints.
> > 
> > I think some Optionality is needed here, since ANA can be completely
disabled,
> > and in that case we don't want to report these stats. We want to be able to
> tell
> > a difference between calls with 0 actions (this means good enough network
> > conditions that ANA is not needed), and calls where ANA was completely
> disabled.
> > However, I guess it's possible to put all of the int's into a struct, and
make
> > that struct Optional. Do you think that would be better?
> 
> 1. If ANA being enabled or not is one statistic we want to communicate, then
> maybe that's just a separate bool field?
> 2. If AudioEncoderStats is going to be either "empty" (ANA disabled) or full
of
> values (ANA enabled), then I think the functions returning AudioEncoderStats
> should return rtc::Optional<AudioEncoderStats>.
> 
> Both of these depend on the future direction of this statistics struct. Is it
> planned to, in future, contain other stats that aren't ANA specific? If not,
> perhaps rename it to ANAStats (or something like that). Then having something
> return an Optional<ANAStats> makes it quite clear that if there's no ANA
> enabled, there'll be no stats for it. If it's conceivable that
AudioEncoderStats
> will at some point contain other information, but there's nothing planned
right
> now, then going the Optional<ANAStats> route also makes sense; Once more stats
> are needed, the Optional<ANAStats> can be put inside the , then new,
> AudioEncoderStats struct. 

Re 1: I think it would be a bit strange to have a stat in GetStats() (which is
for monitoring fluctuating stats) that is either true or false throughout the
call, just to indicate if a module is active or not. To me it makes more sense
to have the stats in GetStats() when ANA is active, and not set them when ANA is
not used.

About your other proposal, I think you have a good point, indeed my reasoning
was that in the future we may want to add more stats to AudioEncoder. I like
your proposal, but it does mean that if we want to add other stats to
AudioEncoder we will have to change the interface of GetStats() function, which
is a public API (or will be). WDYT?

I'm also not sure how likely it is that we will want to add other non-ANA
related stats from AudioEncoder to GetStats(), Minyue, what do you think?

[minyue-webrtc, 2017-09-01 10:42:27]

On 2017/09/01 09:34:24, ivoc wrote:
>
https://codereview.webrtc.org/3011623002/diff/1/webrtc/api/audio_codecs/audio...
> File webrtc/api/audio_codecs/audio_encoder.h (right):
> 
>
https://codereview.webrtc.org/3011623002/diff/1/webrtc/api/audio_codecs/audio...
> webrtc/api/audio_codecs/audio_encoder.h:88: rtc::Optional<int>
> ana_bitrate_action_counter;
> On 2017/08/31 15:40:20, ossu wrote:
> > On 2017/08/31 14:57:50, ivoc wrote:
> > > On 2017/08/31 13:13:10, ossu wrote:
> > > > Are all these individually optional? 
> > > > 
> > > > Since they're counters, it would seem to me they're not optional at all:
> is
> > a
> > > > missing value different from a count of zero? Otherwise they should all
> just
> > > be
> > > > ints.
> > > 
> > > I think some Optionality is needed here, since ANA can be completely
> disabled,
> > > and in that case we don't want to report these stats. We want to be able
to
> > tell
> > > a difference between calls with 0 actions (this means good enough network
> > > conditions that ANA is not needed), and calls where ANA was completely
> > disabled.
> > > However, I guess it's possible to put all of the int's into a struct, and
> make
> > > that struct Optional. Do you think that would be better?
> > 
> > 1. If ANA being enabled or not is one statistic we want to communicate, then
> > maybe that's just a separate bool field?
> > 2. If AudioEncoderStats is going to be either "empty" (ANA disabled) or full
> of
> > values (ANA enabled), then I think the functions returning AudioEncoderStats
> > should return rtc::Optional<AudioEncoderStats>.
> > 
> > Both of these depend on the future direction of this statistics struct. Is
it
> > planned to, in future, contain other stats that aren't ANA specific? If not,
> > perhaps rename it to ANAStats (or something like that). Then having
something
> > return an Optional<ANAStats> makes it quite clear that if there's no ANA
> > enabled, there'll be no stats for it. If it's conceivable that
> AudioEncoderStats
> > will at some point contain other information, but there's nothing planned
> right
> > now, then going the Optional<ANAStats> route also makes sense; Once more
stats
> > are needed, the Optional<ANAStats> can be put inside the , then new,
> > AudioEncoderStats struct. 
> 
> Re 1: I think it would be a bit strange to have a stat in GetStats() (which is
> for monitoring fluctuating stats) that is either true or false throughout the
> call, just to indicate if a module is active or not. To me it makes more sense
> to have the stats in GetStats() when ANA is active, and not set them when ANA
is
> not used.

> 
> About your other proposal, I think you have a good point, indeed my reasoning
> was that in the future we may want to add more stats to AudioEncoder. I like
> your proposal, but it does mean that if we want to add other stats to
> AudioEncoder we will have to change the interface of GetStats() function,
Oh, do we need to change the signature in the interface, or do we only need to
update the struct?

which
> is a public API (or will be). WDYT?

> 
> I'm also not sure how likely it is that we will want to add other non-ANA
> related stats from AudioEncoder to GetStats(), Minyue, what do you think?
There is a RtcCodecStats defined in the spec
https://w3c.github.io/webrtc-stats/#dom-rtccodecstats
I don't know about our plan in supporting it.

I think defining stats from the top may make the moving from old stats collector
to the new one easier.
Henrik Boström may have clearer idea on this than me.

[hbos, 2017-09-01 13:58:42]

Ah, short on time again, I'll take a look first thing on monday - promise.

[ossu, 2017-09-01 14:28:52]

On 2017/09/01 10:42:27, minyue-webrtc(BackIn2018March) wrote:
> On 2017/09/01 09:34:24, ivoc wrote:
> > 
> > I'm also not sure how likely it is that we will want to add other non-ANA
> > related stats from AudioEncoder to GetStats(), Minyue, what do you think?
> There is a RtcCodecStats defined in the spec
> https://w3c.github.io/webrtc-stats/#dom-rtccodecstats
> I don't know about our plan in supporting it.

It looks like all of those stats (except possibly for the "implementation" one,
if that refers to the actual codec implementation and not just "webrtc.org") is
straightforward to implement outside of the AudioEncoder implementation. They
are all things that you get during negotiation or through already existing
AudioEncoder calls (RtpTimestampRateHz() and NumChannels(), for example).

https://codereview.webrtc.org/3011623002/diff/1/webrtc/api/audio_codecs/audio...
File webrtc/api/audio_codecs/audio_encoder.h (right):

https://codereview.webrtc.org/3011623002/diff/1/webrtc/api/audio_codecs/audio...
webrtc/api/audio_codecs/audio_encoder.h:88: rtc::Optional<int>
ana_bitrate_action_counter;
On 2017/09/01 09:34:24, ivoc wrote:
> On 2017/08/31 15:40:20, ossu wrote:
> > On 2017/08/31 14:57:50, ivoc wrote:
> > > On 2017/08/31 13:13:10, ossu wrote:
> > > > Are all these individually optional? 
> > > > 
> > > > Since they're counters, it would seem to me they're not optional at all:
> is
> > a
> > > > missing value different from a count of zero? Otherwise they should all
> just
> > > be
> > > > ints.
> > > 
> > > I think some Optionality is needed here, since ANA can be completely
> disabled,
> > > and in that case we don't want to report these stats. We want to be able
to
> > tell
> > > a difference between calls with 0 actions (this means good enough network
> > > conditions that ANA is not needed), and calls where ANA was completely
> > disabled.
> > > However, I guess it's possible to put all of the int's into a struct, and
> make
> > > that struct Optional. Do you think that would be better?
> > 
> > 1. If ANA being enabled or not is one statistic we want to communicate, then
> > maybe that's just a separate bool field?
> > 2. If AudioEncoderStats is going to be either "empty" (ANA disabled) or full
> of
> > values (ANA enabled), then I think the functions returning AudioEncoderStats
> > should return rtc::Optional<AudioEncoderStats>.
> > 
> > Both of these depend on the future direction of this statistics struct. Is
it
> > planned to, in future, contain other stats that aren't ANA specific? If not,
> > perhaps rename it to ANAStats (or something like that). Then having
something
> > return an Optional<ANAStats> makes it quite clear that if there's no ANA
> > enabled, there'll be no stats for it. If it's conceivable that
> AudioEncoderStats
> > will at some point contain other information, but there's nothing planned
> right
> > now, then going the Optional<ANAStats> route also makes sense; Once more
stats
> > are needed, the Optional<ANAStats> can be put inside the , then new,
> > AudioEncoderStats struct. 
> 
> Re 1: I think it would be a bit strange to have a stat in GetStats() (which is
> for monitoring fluctuating stats) that is either true or false throughout the
> call, just to indicate if a module is active or not. To me it makes more sense
> to have the stats in GetStats() when ANA is active, and not set them when ANA
is
> not used.
> 
> About your other proposal, I think you have a good point, indeed my reasoning
> was that in the future we may want to add more stats to AudioEncoder. I like
> your proposal, but it does mean that if we want to add other stats to
> AudioEncoder we will have to change the interface of GetStats() function,
which
> is a public API (or will be). WDYT?
> 
> I'm also not sure how likely it is that we will want to add other non-ANA
> related stats from AudioEncoder to GetStats(), Minyue, what do you think?

I guess if we don't really know if other stats will be exposed from the
AudioEncoder object right now, the most descriptive way forward would be:
  rtc::Optional<ANAStats> GetANAStats() const;
or some very similar naming scheme. Then if there's a future with more stats,
that could be implemented as a more general GetStats() that returns something
that could contain an rtc::Optional<ANAStats> as one of its members (and
GetANAStats could become deprecated and eventually removed).

Since any downstream code that wants to use the new stats would need to be
updated anyways, this shouldn't be more breaking than changing an existing
GetStats() signature or adding members to an existing struct.

[ivoc, 2017-09-01 15:27:17]

Thanks for the good discussions, I implemented most of your suggestions.

https://codereview.webrtc.org/3011623002/diff/1/webrtc/api/audio_codecs/audio...
File webrtc/api/audio_codecs/audio_encoder.h (right):

https://codereview.webrtc.org/3011623002/diff/1/webrtc/api/audio_codecs/audio...
webrtc/api/audio_codecs/audio_encoder.h:88: rtc::Optional<int>
ana_bitrate_action_counter;
On 2017/09/01 14:28:52, ossu wrote:
> On 2017/09/01 09:34:24, ivoc wrote:
> > On 2017/08/31 15:40:20, ossu wrote:
> > > On 2017/08/31 14:57:50, ivoc wrote:
> > > > On 2017/08/31 13:13:10, ossu wrote:
> > > > > Are all these individually optional? 
> > > > > 
> > > > > Since they're counters, it would seem to me they're not optional at
all:
> > is
> > > a
> > > > > missing value different from a count of zero? Otherwise they should
all
> > just
> > > > be
> > > > > ints.
> > > > 
> > > > I think some Optionality is needed here, since ANA can be completely
> > disabled,
> > > > and in that case we don't want to report these stats. We want to be able
> to
> > > tell
> > > > a difference between calls with 0 actions (this means good enough
network
> > > > conditions that ANA is not needed), and calls where ANA was completely
> > > disabled.
> > > > However, I guess it's possible to put all of the int's into a struct,
and
> > make
> > > > that struct Optional. Do you think that would be better?
> > > 
> > > 1. If ANA being enabled or not is one statistic we want to communicate,
then
> > > maybe that's just a separate bool field?
> > > 2. If AudioEncoderStats is going to be either "empty" (ANA disabled) or
full
> > of
> > > values (ANA enabled), then I think the functions returning
AudioEncoderStats
> > > should return rtc::Optional<AudioEncoderStats>.
> > > 
> > > Both of these depend on the future direction of this statistics struct. Is
> it
> > > planned to, in future, contain other stats that aren't ANA specific? If
not,
> > > perhaps rename it to ANAStats (or something like that). Then having
> something
> > > return an Optional<ANAStats> makes it quite clear that if there's no ANA
> > > enabled, there'll be no stats for it. If it's conceivable that
> > AudioEncoderStats
> > > will at some point contain other information, but there's nothing planned
> > right
> > > now, then going the Optional<ANAStats> route also makes sense; Once more
> stats
> > > are needed, the Optional<ANAStats> can be put inside the , then new,
> > > AudioEncoderStats struct. 
> > 
> > Re 1: I think it would be a bit strange to have a stat in GetStats() (which
is
> > for monitoring fluctuating stats) that is either true or false throughout
the
> > call, just to indicate if a module is active or not. To me it makes more
sense
> > to have the stats in GetStats() when ANA is active, and not set them when
ANA
> is
> > not used.
> > 
> > About your other proposal, I think you have a good point, indeed my
reasoning
> > was that in the future we may want to add more stats to AudioEncoder. I like
> > your proposal, but it does mean that if we want to add other stats to
> > AudioEncoder we will have to change the interface of GetStats() function,
> which
> > is a public API (or will be). WDYT?
> > 
> > I'm also not sure how likely it is that we will want to add other non-ANA
> > related stats from AudioEncoder to GetStats(), Minyue, what do you think?
> 
> I guess if we don't really know if other stats will be exposed from the
> AudioEncoder object right now, the most descriptive way forward would be:
>   rtc::Optional<ANAStats> GetANAStats() const;
> or some very similar naming scheme. Then if there's a future with more stats,
> that could be implemented as a more general GetStats() that returns something
> that could contain an rtc::Optional<ANAStats> as one of its members (and
> GetANAStats could become deprecated and eventually removed).
> 
> Since any downstream code that wants to use the new stats would need to be
> updated anyways, this shouldn't be more breaking than changing an existing
> GetStats() signature or adding members to an existing struct.

Thanks, that sounds like a good plan. As discussed offline, I kept the
per-controller optionality since it seems that ANA is pretty flexible with
sub-modules that can be individually enabled/disabled.

https://codereview.webrtc.org/3011623002/diff/1/webrtc/modules/audio_coding/i...
File webrtc/modules/audio_coding/include/audio_coding_module.h (right):

https://codereview.webrtc.org/3011623002/diff/1/webrtc/modules/audio_coding/i...
webrtc/modules/audio_coding/include/audio_coding_module.h:827: virtual
AudioEncoder::AudioEncoderStats GetAudioEncoderStatistics() const = 0;
On 2017/08/31 14:57:50, ivoc wrote:
> On 2017/08/31 13:20:17, minyue-webrtc wrote:
> > On 2017/08/31 13:13:10, ossu wrote:
> > > On 2017/08/31 07:24:21, minyue-webrtc wrote:
> > > > I think it is better to do similar to GetDecodingCallStatistics()
> > > 
> > > Why do you think so? Functions with output parameters are much more
unwieldy
> > to
> > > call and only has a significant benefit in a few special cases (i.e.
having
> it
> > > fill in a (large) caller-managed buffer).
> > > 
> > > For this function, the copy could (and very probably will) be elided
thanks
> to
> > > Named Return Value Optimization.
> > 
> > Copying is fine with me, and I understand the merits of not passing
pointers.
> > What I would prefer, though is to let stats defined from top, since stats
are
> > often defined in specs.
> 
> I agree with ossu on return values vs passing pointers, but I'm not sure I
> understand what minyue is arguing for. Minyue: do you think this function
should
> pass in the stats struct by pointer or that the stats struct should be defined
> in common_types.h like AudioDecodingCallStats, or both?

I moved the stats struct to common_types.h.

[hbos, 2017-09-04 08:49:54]

What is the plan for these stats, since they're non-standards? Please document
the meaning of the stats.

Are they specific to our implementation or could they be proposed to be
standardized? Would you want to, why/why not?

Will they be collected for the purpose of an experiment? Should we remove them
in the future? Should we have a flag to enable/disable them?

Do we want to expose these to the web platform (javascript)?

https://codereview.webrtc.org/3011623002/diff/1/webrtc/api/audio_codecs/audio...
File webrtc/api/audio_codecs/audio_encoder.h (right):

https://codereview.webrtc.org/3011623002/diff/1/webrtc/api/audio_codecs/audio...
webrtc/api/audio_codecs/audio_encoder.h:88: rtc::Optional<int>
ana_bitrate_action_counter;
On 2017/09/01 15:27:16, ivoc wrote:
> On 2017/09/01 14:28:52, ossu wrote:
> > On 2017/09/01 09:34:24, ivoc wrote:
> > > On 2017/08/31 15:40:20, ossu wrote:
> > > > On 2017/08/31 14:57:50, ivoc wrote:
> > > > > On 2017/08/31 13:13:10, ossu wrote:
> > > > > > Are all these individually optional? 
> > > > > > 
> > > > > > Since they're counters, it would seem to me they're not optional at
> all:
> > > is
> > > > a
> > > > > > missing value different from a count of zero? Otherwise they should
> all
> > > just
> > > > > be
> > > > > > ints.
> > > > > 
> > > > > I think some Optionality is needed here, since ANA can be completely
> > > disabled,
> > > > > and in that case we don't want to report these stats. We want to be
able
> > to
> > > > tell
> > > > > a difference between calls with 0 actions (this means good enough
> network
> > > > > conditions that ANA is not needed), and calls where ANA was completely
> > > > disabled.
> > > > > However, I guess it's possible to put all of the int's into a struct,
> and
> > > make
> > > > > that struct Optional. Do you think that would be better?
> > > > 
> > > > 1. If ANA being enabled or not is one statistic we want to communicate,
> then
> > > > maybe that's just a separate bool field?
> > > > 2. If AudioEncoderStats is going to be either "empty" (ANA disabled) or
> full
> > > of
> > > > values (ANA enabled), then I think the functions returning
> AudioEncoderStats
> > > > should return rtc::Optional<AudioEncoderStats>.
> > > > 
> > > > Both of these depend on the future direction of this statistics struct.
Is
> > it
> > > > planned to, in future, contain other stats that aren't ANA specific? If
> not,
> > > > perhaps rename it to ANAStats (or something like that). Then having
> > something
> > > > return an Optional<ANAStats> makes it quite clear that if there's no ANA
> > > > enabled, there'll be no stats for it. If it's conceivable that
> > > AudioEncoderStats
> > > > will at some point contain other information, but there's nothing
planned
> > > right
> > > > now, then going the Optional<ANAStats> route also makes sense; Once more
> > stats
> > > > are needed, the Optional<ANAStats> can be put inside the , then new,
> > > > AudioEncoderStats struct. 
> > > 
> > > Re 1: I think it would be a bit strange to have a stat in GetStats()
(which
> is
> > > for monitoring fluctuating stats) that is either true or false throughout
> the
> > > call, just to indicate if a module is active or not. To me it makes more
> sense
> > > to have the stats in GetStats() when ANA is active, and not set them when
> ANA
> > is
> > > not used.
> > > 
> > > About your other proposal, I think you have a good point, indeed my
> reasoning
> > > was that in the future we may want to add more stats to AudioEncoder. I
like
> > > your proposal, but it does mean that if we want to add other stats to
> > > AudioEncoder we will have to change the interface of GetStats() function,
> > which
> > > is a public API (or will be). WDYT?
> > > 
> > > I'm also not sure how likely it is that we will want to add other non-ANA
> > > related stats from AudioEncoder to GetStats(), Minyue, what do you think?
> > 
> > I guess if we don't really know if other stats will be exposed from the
> > AudioEncoder object right now, the most descriptive way forward would be:
> >   rtc::Optional<ANAStats> GetANAStats() const;
> > or some very similar naming scheme. Then if there's a future with more
stats,
> > that could be implemented as a more general GetStats() that returns
something
> > that could contain an rtc::Optional<ANAStats> as one of its members (and
> > GetANAStats could become deprecated and eventually removed).
> > 
> > Since any downstream code that wants to use the new stats would need to be
> > updated anyways, this shouldn't be more breaking than changing an existing
> > GetStats() signature or adding members to an existing struct.
> 
> Thanks, that sounds like a good plan. As discussed offline, I kept the
> per-controller optionality since it seems that ANA is pretty flexible with
> sub-modules that can be individually enabled/disabled.

Using optional is correct. If the value is being counted it should start at 0,
but if it's N/A (such as not enabled for counting) then it shouldn't be present.

Doing Optional<ANAStats> makes sense if its all-or-nothing. Having ANAStats with
Optional fields also makes sense.

https://codereview.webrtc.org/3011623002/diff/40001/webrtc/common_types.h
File webrtc/common_types.h (right):

https://codereview.webrtc.org/3011623002/diff/40001/webrtc/common_types.h#new...
webrtc/common_types.h:440: ~ANAStats();
Is this a style-guide thing or can these be omitted, since they're all =
default?

https://codereview.webrtc.org/3011623002/diff/40001/webrtc/common_types.h#new...
webrtc/common_types.h:445: rtc::Optional<int> frame_length_action_counter;
Please add comments explaining these.

Also, prefer one of the types int32_t, uint32_t, int64_t, uint64_t for integers
when working with stats. int or int32_t shouldn't matter in this case but I
prefer all new stats code to be explicit about the type like that because the
new api and spec is and stats are exported to other formats (javascript, jni,
etc).

https://codereview.webrtc.org/3011623002/diff/40001/webrtc/media/base/mediach...
File webrtc/media/base/mediachannel.h (right):

https://codereview.webrtc.org/3011623002/diff/40001/webrtc/media/base/mediach...
webrtc/media/base/mediachannel.h:648: rtc::Optional<int>
ana_frame_length_action_counter;
Should there be one field that is ANAStats instead of one of each of its fields?

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

https://codereview.webrtc.org/3011623002/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.cc:139:
// TODO(ivoc): Actually implement the stat.
Link to a bug?

https://codereview.webrtc.org/3011623002/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.cc:141:
}
Can you add a audio_network_adapter_impl_unittest.cc that verifies that default
stats are returned so that the test fails if you implement this (forcing you to
fix the test)?

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

https://codereview.webrtc.org/3011623002/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/audio_network_adaptor/include/audio_network_adaptor.h:45:
~AudioNetworkAdaptorStats();
Style-guide thing or can we remove these default con/destructors?

https://codereview.webrtc.org/3011623002/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/audio_network_adaptor/include/audio_network_adaptor.h:50:
rtc::Optional<int> ana_frame_length_action_counter;
Is there a reason for not reusing ANAStats, either completely or as a field of
these Stats?

https://codereview.webrtc.org/3011623002/diff/40001/webrtc/pc/statscollector.cc
File webrtc/pc/statscollector.cc (right):

https://codereview.webrtc.org/3011623002/diff/40001/webrtc/pc/statscollector....
webrtc/pc/statscollector.cc:246: }
Can you add a statscollector_unittest.cc to cover these?

[ossu, 2017-09-04 11:23:02]

https://codereview.webrtc.org/3011623002/diff/40001/webrtc/common_types.h
File webrtc/common_types.h (right):

https://codereview.webrtc.org/3011623002/diff/40001/webrtc/common_types.h#new...
webrtc/common_types.h:440: ~ANAStats();
On 2017/09/04 08:49:54, hbos wrote:
> Is this a style-guide thing or can these be omitted, since they're all =
> default?

Due to the Optional<> members, clang(-tidy) decides that the struct is complex
enough that the constructors / destructors mustn't be inline.

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

https://codereview.webrtc.org/3011623002/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/audio_network_adaptor/include/audio_network_adaptor.h:50:
rtc::Optional<int> ana_frame_length_action_counter;
On 2017/09/04 08:49:54, hbos wrote:
> Is there a reason for not reusing ANAStats, either completely or as a field of
> these Stats?

I agree we should try to reuse the ANAStats struct, wherever it's best defined,
in all of these if at all sensible.

[ivoc, 2017-09-04 15:48:07]

https://codereview.webrtc.org/3011623002/diff/40001/webrtc/common_types.h
File webrtc/common_types.h (right):

https://codereview.webrtc.org/3011623002/diff/40001/webrtc/common_types.h#new...
webrtc/common_types.h:445: rtc::Optional<int> frame_length_action_counter;
On 2017/09/04 08:49:54, hbos wrote:
> Please add comments explaining these.
> 
> Also, prefer one of the types int32_t, uint32_t, int64_t, uint64_t for
integers
> when working with stats. int or int32_t shouldn't matter in this case but I
> prefer all new stats code to be explicit about the type like that because the
> new api and spec is and stats are exported to other formats (javascript, jni,
> etc).

Added comments and changed these to uint32_t.

https://codereview.webrtc.org/3011623002/diff/40001/webrtc/media/base/mediach...
File webrtc/media/base/mediachannel.h (right):

https://codereview.webrtc.org/3011623002/diff/40001/webrtc/media/base/mediach...
webrtc/media/base/mediachannel.h:648: rtc::Optional<int>
ana_frame_length_action_counter;
On 2017/09/04 08:49:54, hbos wrote:
> Should there be one field that is ANAStats instead of one of each of its
fields?

Sounds like a good idea, done.

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

https://codereview.webrtc.org/3011623002/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.cc:139:
// TODO(ivoc): Actually implement the stat.
On 2017/09/04 08:49:54, hbos wrote:
> Link to a bug?

Added.

https://codereview.webrtc.org/3011623002/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.cc:141:
}
On 2017/09/04 08:49:54, hbos wrote:
> Can you add a audio_network_adapter_impl_unittest.cc that verifies that
default
> stats are returned so that the test fails if you implement this (forcing you
to
> fix the test)?

Done.

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

https://codereview.webrtc.org/3011623002/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/audio_network_adaptor/include/audio_network_adaptor.h:45:
~AudioNetworkAdaptorStats();
On 2017/09/04 08:49:54, hbos wrote:
> Style-guide thing or can we remove these default con/destructors?

The optionals made the class "too complex" not to have the explicit constructors
and destructor. This class has now been removed.

https://codereview.webrtc.org/3011623002/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/audio_network_adaptor/include/audio_network_adaptor.h:50:
rtc::Optional<int> ana_frame_length_action_counter;
On 2017/09/04 11:23:02, ossu wrote:
> On 2017/09/04 08:49:54, hbos wrote:
> > Is there a reason for not reusing ANAStats, either completely or as a field
of
> > these Stats?
> 
> I agree we should try to reuse the ANAStats struct, wherever it's best
defined,
> in all of these if at all sensible.

Agreed, removed.

https://codereview.webrtc.org/3011623002/diff/40001/webrtc/pc/statscollector.cc
File webrtc/pc/statscollector.cc (right):

https://codereview.webrtc.org/3011623002/diff/40001/webrtc/pc/statscollector....
webrtc/pc/statscollector.cc:246: }
On 2017/09/04 08:49:54, hbos wrote:
> Can you add a statscollector_unittest.cc to cover these?

Done.

[ossu, 2017-09-05 10:19:02]

lgtm % clarifying comments

https://codereview.webrtc.org/3011623002/diff/60001/webrtc/common_types.h
File webrtc/common_types.h (right):

https://codereview.webrtc.org/3011623002/diff/60001/webrtc/common_types.h#new...
webrtc/common_types.h:441: // Number of actions taken by the ANA bitrate
controller since the start of
Please also include an explanation for what an empty value means in these
comments.

[ivoc, 2017-09-06 11:46:16]

@hbos: I think these new stats could considered for standardization in the
future. I don't think they are super specific to our implementation of ANA.
Also, I think these stats are useful enough to always keep enabled, so I would
prefer not to put them behind a flag (but in the old getStats for now, since we
may want to change/add a few stats in the near future).

https://codereview.webrtc.org/3011623002/diff/60001/webrtc/common_types.h
File webrtc/common_types.h (right):

https://codereview.webrtc.org/3011623002/diff/60001/webrtc/common_types.h#new...
webrtc/common_types.h:441: // Number of actions taken by the ANA bitrate
controller since the start of
On 2017/09/05 10:19:02, ossu wrote:
> Please also include an explanation for what an empty value means in these
> comments.

Done!

[ivoc, 2017-09-06 15:56:40]

By the way, I would like to land this before monday, if possible. Feel free to
only review the parts that you have ownership of if time is an issue.

[hbos, 2017-09-07 15:47:47]

lgtm, I reviewed everything!

Can you file an issue on https://github.com/w3c/webrtc-stats/issues ?

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

https://codereview.webrtc.org/3011623002/diff/80001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.cc:140:
// Tracking bug: https://bugs.chromium.org/p/webrtc/issues/detail?id=8127
nit: https://crbug.com/webrtc/8127 works too

https://codereview.webrtc.org/3011623002/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/3011623002/diff/80001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl_unittest.cc:293:
// implemented yet).
nit: Link to https://crbug.com/webrtc/8127

[ivoc, 2017-09-07 16:14:32]

Thanks! I filed an issue at https://github.com/w3c/webrtc-stats/issues/240

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

https://codereview.webrtc.org/3011623002/diff/80001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl.cc:140:
// Tracking bug: https://bugs.chromium.org/p/webrtc/issues/detail?id=8127
On 2017/09/07 15:47:47, hbos wrote:
> nit: https://crbug.com/webrtc/8127 works too

Done.

https://codereview.webrtc.org/3011623002/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/3011623002/diff/80001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/audio_network_adaptor/audio_network_adaptor_impl_unittest.cc:293:
// implemented yet).
On 2017/09/07 15:47:47, hbos wrote:
> nit: Link to https://crbug.com/webrtc/8127

Done.

[ivoc, 2017-09-08 07:49:09]

ivoc@webrtc.org changed reviewers:
+ kwiberg@webrtc.org

[ivoc, 2017-09-08 07:50:33]

Adding kwiberg to review changes to webrtc/common_types.{cc,h}.

[kwiberg-webrtc, 2017-09-08 08:09:50]

https://codereview.webrtc.org/3011623002/diff/100001/webrtc/common_types.h
File webrtc/common_types.h (right):

https://codereview.webrtc.org/3011623002/diff/100001/webrtc/common_types.h#ne...
webrtc/common_types.h:437: struct ANAStats {
The struct itself looks good, but is there someplace else you could put it? All
the things in this file are going to have to find another home soon-ish, so not
adding more stuff feels like a good idea.

[kwiberg-webrtc, 2017-09-08 08:11:47]

https://codereview.webrtc.org/3011623002/diff/100001/webrtc/common_types.h
File webrtc/common_types.h (right):

https://codereview.webrtc.org/3011623002/diff/100001/webrtc/common_types.h#ne...
webrtc/common_types.h:437: struct ANAStats {
On 2017/09/08 08:09:49, kwiberg-webrtc wrote:
> The struct itself looks good, but is there someplace else you could put it?
All
> the things in this file are going to have to find another home soon-ish, so
not
> adding more stuff feels like a good idea.

Like in audio_encoder.h for example?

[ivoc, 2017-09-08 08:39:08]

https://codereview.webrtc.org/3011623002/diff/100001/webrtc/common_types.h
File webrtc/common_types.h (right):

https://codereview.webrtc.org/3011623002/diff/100001/webrtc/common_types.h#ne...
webrtc/common_types.h:437: struct ANAStats {
On 2017/09/08 08:11:47, kwiberg-webrtc wrote:
> On 2017/09/08 08:09:49, kwiberg-webrtc wrote:
> > The struct itself looks good, but is there someplace else you could put it?
> All
> > the things in this file are going to have to find another home soon-ish, so
> not
> > adding more stuff feels like a good idea.
> 
> Like in audio_encoder.h for example?

That makes sense, I wasn't aware this file is going away. I put the struct in
audio_encoder.h like you suggested.

[ossu, 2017-09-08 09:01:33]

https://codereview.webrtc.org/3011623002/diff/100001/webrtc/common_types.h
File webrtc/common_types.h (right):

https://codereview.webrtc.org/3011623002/diff/100001/webrtc/common_types.h#ne...
webrtc/common_types.h:437: struct ANAStats {
On 2017/09/08 08:39:08, ivoc wrote:
> On 2017/09/08 08:11:47, kwiberg-webrtc wrote:
> > On 2017/09/08 08:09:49, kwiberg-webrtc wrote:
> > > The struct itself looks good, but is there someplace else you could put
it?
> > All
> > > the things in this file are going to have to find another home soon-ish,
so
> > not
> > > adding more stuff feels like a good idea.
> > 
> > Like in audio_encoder.h for example?
> 
> That makes sense, I wasn't aware this file is going away. I put the struct in
> audio_encoder.h like you suggested.

Did this struct just move all the way to common_types.h and back again, changing
its name on the way? :)

(I'm fine with that BTW - just an observation)

[ivoc, 2017-09-08 09:16:38]

https://codereview.webrtc.org/3011623002/diff/100001/webrtc/common_types.h
File webrtc/common_types.h (right):

https://codereview.webrtc.org/3011623002/diff/100001/webrtc/common_types.h#ne...
webrtc/common_types.h:437: struct ANAStats {
On 2017/09/08 09:01:32, ossu wrote:
> On 2017/09/08 08:39:08, ivoc wrote:
> > On 2017/09/08 08:11:47, kwiberg-webrtc wrote:
> > > On 2017/09/08 08:09:49, kwiberg-webrtc wrote:
> > > > The struct itself looks good, but is there someplace else you could put
> it?
> > > All
> > > > the things in this file are going to have to find another home soon-ish,
> so
> > > not
> > > > adding more stuff feels like a good idea.
> > > 
> > > Like in audio_encoder.h for example?
> > 
> > That makes sense, I wasn't aware this file is going away. I put the struct
in
> > audio_encoder.h like you suggested.
> 
> Did this struct just move all the way to common_types.h and back again,
changing
> its name on the way? :)
> 
> (I'm fine with that BTW - just an observation)

Indeed it did, behold the magic of code reviews :) I think it makes sense to
have it here though, and there were many other improvements along the way.

[the sun, 2017-09-08 10:41:37]

On 2017/09/08 09:16:38, ivoc wrote:
> https://codereview.webrtc.org/3011623002/diff/100001/webrtc/common_types.h
> File webrtc/common_types.h (right):
> 
>
https://codereview.webrtc.org/3011623002/diff/100001/webrtc/common_types.h#ne...
> webrtc/common_types.h:437: struct ANAStats {
> On 2017/09/08 09:01:32, ossu wrote:
> > On 2017/09/08 08:39:08, ivoc wrote:
> > > On 2017/09/08 08:11:47, kwiberg-webrtc wrote:
> > > > On 2017/09/08 08:09:49, kwiberg-webrtc wrote:
> > > > > The struct itself looks good, but is there someplace else you could
put
> > it?
> > > > All
> > > > > the things in this file are going to have to find another home
soon-ish,
> > so
> > > > not
> > > > > adding more stuff feels like a good idea.
> > > > 
> > > > Like in audio_encoder.h for example?
> > > 
> > > That makes sense, I wasn't aware this file is going away. I put the struct
> in
> > > audio_encoder.h like you suggested.
> > 
> > Did this struct just move all the way to common_types.h and back again,
> changing
> > its name on the way? :)
> > 
> > (I'm fine with that BTW - just an observation)
> 
> Indeed it did, behold the magic of code reviews :) I think it makes sense to
> have it here though, and there were many other improvements along the way.

Good call! common_types.h should be split up and removed from webrtc/ root.

[the sun, 2017-09-08 10:43:16]

LGTM

Slight concern that we're adding ANA to the public AudioEncoder interface; does
the way it is exposed make sense as a general abstraction?

https://codereview.webrtc.org/3011623002/diff/120001/webrtc/voice_engine/chan...
File webrtc/voice_engine/channel_proxy.cc (right):

https://codereview.webrtc.org/3011623002/diff/120001/webrtc/voice_engine/chan...
webrtc/voice_engine/channel_proxy.cc:147: ANAStats
ChannelProxy::GetANAStatistics() const {
RTC_DCHECK(worker_thread_checker_.CalledOnValidThread());

[ivoc, 2017-09-08 11:25:50]

Thanks!

There was some discussion with ossu about how to put this on the AudioEncoder
interface, but since we don't have plans to add more statistics to it, we
decided that it would be best to expose it directly for now. In the future if we
want to add more stats it may make sense to put the ANAStats inside of a more
general AudioEncoderStats struct, but it doesn't make sense to do that before we
know it will be necessary.

https://codereview.webrtc.org/3011623002/diff/120001/webrtc/voice_engine/chan...
File webrtc/voice_engine/channel_proxy.cc (right):

https://codereview.webrtc.org/3011623002/diff/120001/webrtc/voice_engine/chan...
webrtc/voice_engine/channel_proxy.cc:147: ANAStats
ChannelProxy::GetANAStatistics() const {
On 2017/09/08 10:43:16, the sun wrote:
> RTC_DCHECK(worker_thread_checker_.CalledOnValidThread());

Done.

[ivoc, 2017-09-08 11:28:28]

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

[commit-bot: I haz the power, 2017-09-08 11:28:30]

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-09-08 11:31:32]

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

[commit-bot: I haz the power, 2017-09-08 11:31:33]

Dry run: Try jobs failed on following builders:
  ios64_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios9_dbg/buil...)
  presubmit on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/21362)

[ivoc, 2017-09-08 12:26:03]

Patchset #9 (id:160001) has been deleted

[ivoc, 2017-09-08 12:32:29]

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

[commit-bot: I haz the power, 2017-09-08 12:32: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, 2017-09-08 12:36:50]

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

[commit-bot: I haz the power, 2017-09-08 12:36:51]

Dry run: Try jobs failed on following builders:
  mac_baremetal on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/24812)

[ivoc, 2017-09-08 14:32:20]

The CQ bit was checked by ivoc@webrtc.org

[ivoc, 2017-09-08 14:32:21]

The patchset sent to the CQ was uploaded after l-g-t-m from ossu@webrtc.org,
hbos@webrtc.org, solenberg@webrtc.org
Link to the patchset: https://codereview.webrtc.org/3011623002/#ps220001 (title:
"Fix for failing test.")

[commit-bot: I haz the power, 2017-09-08 14:32:37]

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-09-08 14:38:13]

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

[commit-bot: I haz the power, 2017-09-08 14:38:15]

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

[ivoc, 2017-09-08 14:49:59]

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

[ivoc, 2017-09-08 14:50:21]

ivoc@webrtc.org changed reviewers:
- minyue@webrtc.org

[ivoc, 2017-09-08 14:52:10]

Adding stefan as reviewer for changes in webrtc/pc/statscollector.cc,
webrtc/pc/statscollector_unittest.cc and webrtc/test/mock_voe_channel_proxy.h

[stefan-webrtc, 2017-09-08 15:00:41]

lgtm

[ivoc, 2017-09-08 15:01:16]

The CQ bit was checked by ivoc@webrtc.org

[commit-bot: I haz the power, 2017-09-08 15:01:32]

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-09-08 15:13:20]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1504882876301110,
"parent_rev": "01410dcc99a2dfb9d0d29fad13507cdd08ee71c9", "commit_rev":
"e1198e068bdcb01dcb8799d6871018092711714e"}

[commit-bot: I haz the power, 2017-09-08 15:13:26]

Description was changed from

==========
Add new ANA stats to the old GetStats() to count the number of actions taken by
each controller.

This CL only wires up the new stats but does not set the values yet. This will
be added in a follow-up CL.

BUG=webrtc:8127
==========

to

==========
Add new ANA stats to the old GetStats() to count the number of actions taken by
each controller.

This CL only wires up the new stats but does not set the values yet. This will
be added in a follow-up CL.

BUG=webrtc:8127

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

[commit-bot: I haz the power, 2017-09-08 15:13:28]

Committed patchset #11 (id:220001) as
https://chromium.googlesource.com/external/webrtc/+/e1198e068bdcb01dcb8799d68...