Implement AudioReceiveStream::GetStats().

Re-Land: Implement AudioReceiveStream::GetStats().

R=tommi@webrtc.org
BUG=webrtc:4690
Committed: https://chromium.googlesource.com/external/webrtc/+/a457752f4afc496ed7f4d6b584b08d8635f18cc0

Committed: https://crrev.com/4f4ec0a9270a8cefadfa12e9fa3b979b58b15392
Cr-Commit-Position: refs/heads/master@{#10369}

[the sun, 2015-10-19 10:49:57]

solenberg@webrtc.org changed reviewers:
+ hta@webrtc.org, kwiberg@webrtc.org, mflodman@webrtc.org

[tommi, 2015-10-19 12:36:23]

tommi@webrtc.org changed reviewers:
+ tommi@webrtc.org

[tommi, 2015-10-19 12:36:24]

drive by as a getstats enthusiast

https://codereview.webrtc.org/1390753002/diff/240001/talk/media/webrtc/webrtc...
File talk/media/webrtc/webrtcvoiceengine.cc (right):

https://codereview.webrtc.org/1390753002/diff/240001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.cc:2772: rinfo.add_ssrc(stats.remote_ssrc);
What do you think about moving all this boiler plate to a VoiceReceiveInfo ctor?

https://codereview.webrtc.org/1390753002/diff/240001/talk/media/webrtc/webrtc...
File talk/media/webrtc/webrtcvoiceengine_unittest.cc (right):

https://codereview.webrtc.org/1390753002/diff/240001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine_unittest.cc:87: } // namespace
nit: 2 spaces

https://codereview.webrtc.org/1390753002/diff/240001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine_unittest.cc:326: for (auto s :
call_.GetAudioReceiveStreams()) {
nit: no curlies.
also, just to make it clear that this is not by-value (i.e. creating a copy each
time), use auto*

https://codereview.webrtc.org/1390753002/diff/240001/webrtc/audio/audio_recei...
File webrtc/audio/audio_receive_stream.cc (right):

https://codereview.webrtc.org/1390753002/diff/240001/webrtc/audio/audio_recei...
webrtc/audio/audio_receive_stream.cc:27: template<class T> class
scoped_voe_interface {
nit: ScopedVoEInterface
(I know you're following the scoped_foo convention, but there's also another
ScopedFoo convention which is more aligned with the style guide - but I'll leave
it up to you)

https://codereview.webrtc.org/1390753002/diff/240001/webrtc/audio/audio_recei...
webrtc/audio/audio_receive_stream.cc:31: ptr = T::GetInterface(e);
call T::GetInterface(e) in the initializer list

https://codereview.webrtc.org/1390753002/diff/240001/webrtc/audio/audio_recei...
webrtc/audio/audio_receive_stream.cc:34: if (ptr) ptr->Release();
nit: two lines

https://codereview.webrtc.org/1390753002/diff/240001/webrtc/audio/audio_recei...
webrtc/audio/audio_receive_stream.cc:41: T* ptr;
ptr_

https://codereview.webrtc.org/1390753002/diff/240001/webrtc/audio/audio_recei...
webrtc/audio/audio_receive_stream.cc:107: webrtc::AudioReceiveStream::Stats
AudioReceiveStream::GetStats() const {
Thread check?

https://codereview.webrtc.org/1390753002/diff/240001/webrtc/audio/audio_recei...
webrtc/audio/audio_receive_stream.cc:115: unsigned int ssrc = 0;
do we always use unsigned int for ssrc?  do you know why?  (why isn't it just
|int| or a typedef int SSrc?)

https://codereview.webrtc.org/1390753002/diff/240001/webrtc/audio/audio_recei...
webrtc/audio/audio_receive_stream.cc:121:
codec->GetRecCodec(config_.voe_channel_id, ci) != -1) {
nit: could we convert this to an early return instead of a new scope?

https://codereview.webrtc.org/1390753002/diff/240001/webrtc/audio/audio_recei...
webrtc/audio/audio_receive_stream.cc:126: if (ci.pltype != -1) {
no {} (and below for single line if()s)

https://codereview.webrtc.org/1390753002/diff/240001/webrtc/audio/audio_recei...
webrtc/audio/audio_receive_stream.cc:143: unsigned int level = 0;
in some places we use uint32_t for this, but apparently unsigned int in
voe_volume_control.h, maybe it's a legacy thing

https://codereview.webrtc.org/1390753002/diff/240001/webrtc/audio/audio_recei...
webrtc/audio/audio_receive_stream.cc:146: stats.audio_level = level;
I wonder which type audio_level is

https://codereview.webrtc.org/1390753002/diff/240001/webrtc/audio/audio_recei...
webrtc/audio/audio_receive_stream.cc:147: };
no ;

https://codereview.webrtc.org/1390753002/diff/240001/webrtc/audio/audio_recei...
webrtc/audio/audio_receive_stream.cc:156:
static_cast<float>(ns.currentExpandRate) / (1 << 14);
can you add a note why we divide all the rates by 1<<14? (or maybe just point me
to the info in case it's obvious)

https://codereview.webrtc.org/1390753002/diff/240001/webrtc/audio/audio_recei...
File webrtc/audio/audio_receive_stream.h (right):

https://codereview.webrtc.org/1390753002/diff/240001/webrtc/audio/audio_recei...
webrtc/audio/audio_receive_stream.h:48: VoiceEngine* voice_engine_;
document ownership, lifetime?

https://codereview.webrtc.org/1390753002/diff/240001/webrtc/audio/audio_recei...
File webrtc/audio/audio_receive_stream_unittest.cc (right):

https://codereview.webrtc.org/1390753002/diff/240001/webrtc/audio/audio_recei...
webrtc/audio/audio_receive_stream_unittest.cc:59: float Q14ToFloat(uint16_t v) {
ah, is this why?
Can we move this method to a common header and call it in the previous file
instead of having all the /(1<<14)'d inline?

https://codereview.webrtc.org/1390753002/diff/240001/webrtc/audio/audio_recei...
webrtc/audio/audio_receive_stream_unittest.cc:91: const uint32_t kSsrc1 = 667;
if ssrc is uint32_t, it would be good to have that consistently be the case.  I
think that in general the style guide doesn't like 'unsigned' though, so fwiw...

https://codereview.webrtc.org/1390753002/diff/240001/webrtc/audio/audio_recei...
webrtc/audio/audio_receive_stream_unittest.cc:101: const CallStatistics&
kCallStats = fve.GetRecvCallStats();
this should just be call_stats.  Even though it's const, it's not really a
constant since it's a constructed object and each instance of it can be
different from the next when this function is called. Constants are typically
global or class scope and are compile-time const, not runtime.

https://codereview.webrtc.org/1390753002/diff/240001/webrtc/audio/audio_recei...
webrtc/audio/audio_receive_stream_unittest.cc:102: const CodecInst& kCodecInst =
fve.GetRecvRecCodecInst();
same here and throughout.

https://codereview.webrtc.org/1390753002/diff/240001/webrtc/test/fake_voice_e...
File webrtc/test/fake_voice_engine.h (right):

https://codereview.webrtc.org/1390753002/diff/240001/webrtc/test/fake_voice_e...
webrtc/test/fake_voice_engine.h:27: const int kSendChannelId = 1;
static const int (etc)

https://codereview.webrtc.org/1390753002/diff/240001/webrtc/test/fake_voice_e...
webrtc/test/fake_voice_engine.h:45: const CallStatistics& GetRecvCallStats()
const {
will this always be called on the same thread for all instances?
(static construction within a function is intentionally not thread safe with the
current build settings)

https://codereview.webrtc.org/1390753002/diff/240001/webrtc/test/fake_voice_e...
webrtc/test/fake_voice_engine.h:414: int GetOutputVolumePan(int channel, float&
left, float& right) override {
can we move all the implementation into the cc file?

https://codereview.webrtc.org/1390753002/diff/240001/webrtc/voice_engine/voic...
File webrtc/voice_engine/voice_engine_impl.h (right):

https://codereview.webrtc.org/1390753002/diff/240001/webrtc/voice_engine/voic...
webrtc/voice_engine/voice_engine_impl.h:131: protected:
nit: add a comment why protected?

[the sun, 2015-10-19 12:51:30]

solenberg@webrtc.org changed reviewers:
- mflodman@webrtc.org

[the sun, 2015-10-19 14:25:03]

Thanks for the drive-by. It's great with enthusiasts!

https://codereview.webrtc.org/1390753002/diff/240001/talk/media/webrtc/webrtc...
File talk/media/webrtc/webrtcvoiceengine.cc (right):

https://codereview.webrtc.org/1390753002/diff/240001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.cc:2772: rinfo.add_ssrc(stats.remote_ssrc);
On 2015/10/19 12:36:24, tommi (webrtc) wrote:
> What do you think about moving all this boiler plate to a VoiceReceiveInfo
ctor?

You mean forcing VoiceReceiveInfo to know about the AudioReceiveStream::Stats? I
think that is breaking encapsulation. We'll be better off deprecating
VoiceReceiverInfo, going forward.

https://codereview.webrtc.org/1390753002/diff/240001/talk/media/webrtc/webrtc...
File talk/media/webrtc/webrtcvoiceengine_unittest.cc (right):

https://codereview.webrtc.org/1390753002/diff/240001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine_unittest.cc:87: } // namespace
On 2015/10/19 12:36:24, tommi (webrtc) wrote:
> nit: 2 spaces

Done.

https://codereview.webrtc.org/1390753002/diff/240001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine_unittest.cc:326: for (auto s :
call_.GetAudioReceiveStreams()) {
On 2015/10/19 12:36:24, tommi (webrtc) wrote:
> nit: no curlies.
> also, just to make it clear that this is not by-value (i.e. creating a copy
each
> time), use auto*

Yes, curlies.

1. Style guide avoids the issue.
2. Surrounding code (libjingle) generally use curlies for one-liners; style
guide says prefer consistency.
3. Arguments in favor of using curlies (consistency, less risk of copy/paste
bugs, bad formatting doesn't cause reader misinterpretation) trumps arguments
against (an extra line).

:)

auto* is more readable, thanks.

https://codereview.webrtc.org/1390753002/diff/240001/webrtc/audio/audio_recei...
File webrtc/audio/audio_receive_stream.cc (right):

https://codereview.webrtc.org/1390753002/diff/240001/webrtc/audio/audio_recei...
webrtc/audio/audio_receive_stream.cc:27: template<class T> class
scoped_voe_interface {
On 2015/10/19 12:36:24, tommi (webrtc) wrote:
> nit: ScopedVoEInterface
> (I know you're following the scoped_foo convention, but there's also another
> ScopedFoo convention which is more aligned with the style guide - but I'll
leave
> it up to you)

Done.

https://codereview.webrtc.org/1390753002/diff/240001/webrtc/audio/audio_recei...
webrtc/audio/audio_receive_stream.cc:31: ptr = T::GetInterface(e);
On 2015/10/19 12:36:24, tommi (webrtc) wrote:
> call T::GetInterface(e) in the initializer list

Done.

https://codereview.webrtc.org/1390753002/diff/240001/webrtc/audio/audio_recei...
webrtc/audio/audio_receive_stream.cc:34: if (ptr) ptr->Release();
On 2015/10/19 12:36:24, tommi (webrtc) wrote:
> nit: two lines

Done.

https://codereview.webrtc.org/1390753002/diff/240001/webrtc/audio/audio_recei...
webrtc/audio/audio_receive_stream.cc:41: T* ptr;
On 2015/10/19 12:36:24, tommi (webrtc) wrote:
> ptr_

Done.

https://codereview.webrtc.org/1390753002/diff/240001/webrtc/audio/audio_recei...
webrtc/audio/audio_receive_stream.cc:107: webrtc::AudioReceiveStream::Stats
AudioReceiveStream::GetStats() const {
On 2015/10/19 12:36:24, tommi (webrtc) wrote:
> Thread check?

Done.

https://codereview.webrtc.org/1390753002/diff/240001/webrtc/audio/audio_recei...
webrtc/audio/audio_receive_stream.cc:115: unsigned int ssrc = 0;
On 2015/10/19 12:36:24, tommi (webrtc) wrote:
> do we always use unsigned int for ssrc?  do you know why?  (why isn't it just
> |int| or a typedef int SSrc?)

We're pretty consistent with uint32_t for ssrcs. They're defined here:
https://www.ietf.org/rfc/rfc3550 (page 9, search for "Synchronization source
(SSRC)") as a generic 32-bit number, so basically any int >= 32 bits can be
used; but since they're not used for computations, an unsigned is more suitable;
all we want to do is capture the bit pattern.

A type safe SSRC type would be preferable.

Since you were about to ask, unsigned int (and not uint32_t) is used here
because that is what VoERTP_RTCP::GetRemoteSSRC() uses.

https://codereview.webrtc.org/1390753002/diff/240001/webrtc/audio/audio_recei...
webrtc/audio/audio_receive_stream.cc:121:
codec->GetRecCodec(config_.voe_channel_id, ci) != -1) {
On 2015/10/19 12:36:24, tommi (webrtc) wrote:
> nit: could we convert this to an early return instead of a new scope?

Done.

https://codereview.webrtc.org/1390753002/diff/240001/webrtc/audio/audio_recei...
webrtc/audio/audio_receive_stream.cc:126: if (ci.pltype != -1) {
On 2015/10/19 12:36:24, tommi (webrtc) wrote:
> no {} (and below for single line if()s)

please, point me to the style guide if I've missed that part.

https://codereview.webrtc.org/1390753002/diff/240001/webrtc/audio/audio_recei...
webrtc/audio/audio_receive_stream.cc:143: unsigned int level = 0;
On 2015/10/19 12:36:24, tommi (webrtc) wrote:
> in some places we use uint32_t for this, but apparently unsigned int in
> voe_volume_control.h, maybe it's a legacy thing

Yes, I consistently use the types specified in the VoE interfaces when calling
them.

https://codereview.webrtc.org/1390753002/diff/240001/webrtc/audio/audio_recei...
webrtc/audio/audio_receive_stream.cc:146: stats.audio_level = level;
On 2015/10/19 12:36:24, tommi (webrtc) wrote:
> I wonder which type audio_level is

Done.

https://codereview.webrtc.org/1390753002/diff/240001/webrtc/audio/audio_recei...
webrtc/audio/audio_receive_stream.cc:147: };
On 2015/10/19 12:36:24, tommi (webrtc) wrote:
> no ;

thx

https://codereview.webrtc.org/1390753002/diff/240001/webrtc/audio/audio_recei...
webrtc/audio/audio_receive_stream.cc:156:
static_cast<float>(ns.currentExpandRate) / (1 << 14);
On 2015/10/19 12:36:24, tommi (webrtc) wrote:
> can you add a note why we divide all the rates by 1<<14? (or maybe just point
me
> to the info in case it's obvious)

Sure:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc...

https://codereview.webrtc.org/1390753002/diff/240001/webrtc/audio/audio_recei...
File webrtc/audio/audio_receive_stream.h (right):

https://codereview.webrtc.org/1390753002/diff/240001/webrtc/audio/audio_recei...
webrtc/audio/audio_receive_stream.h:48: VoiceEngine* voice_engine_;
On 2015/10/19 12:36:24, tommi (webrtc) wrote:
> document ownership, lifetime?

Done.

https://codereview.webrtc.org/1390753002/diff/240001/webrtc/audio/audio_recei...
File webrtc/audio/audio_receive_stream_unittest.cc (right):

https://codereview.webrtc.org/1390753002/diff/240001/webrtc/audio/audio_recei...
webrtc/audio/audio_receive_stream_unittest.cc:59: float Q14ToFloat(uint16_t v) {
On 2015/10/19 12:36:24, tommi (webrtc) wrote:
> ah, is this why?
> Can we move this method to a common header and call it in the previous file
> instead of having all the /(1<<14)'d inline?

Done.

https://codereview.webrtc.org/1390753002/diff/240001/webrtc/audio/audio_recei...
webrtc/audio/audio_receive_stream_unittest.cc:91: const uint32_t kSsrc1 = 667;
On 2015/10/19 12:36:24, tommi (webrtc) wrote:
> if ssrc is uint32_t, it would be good to have that consistently be the case. 
I
> think that in general the style guide doesn't like 'unsigned' though, so
fwiw...

The best option would be to create a template to represent an ID of type T, then
pass that around. It falls way outside of this CL though and the scope to
implement throughout the code base is huge.

https://codereview.webrtc.org/1390753002/diff/240001/webrtc/audio/audio_recei...
webrtc/audio/audio_receive_stream_unittest.cc:101: const CallStatistics&
kCallStats = fve.GetRecvCallStats();
On 2015/10/19 12:36:24, tommi (webrtc) wrote:
> this should just be call_stats.  Even though it's const, it's not really a
> constant since it's a constructed object and each instance of it can be
> different from the next when this function is called. Constants are typically
> global or class scope and are compile-time const, not runtime.

As long as you don't mind me keeping the const qualifier...

I didn't know the style guide made that distinction; thanks.

https://codereview.webrtc.org/1390753002/diff/240001/webrtc/audio/audio_recei...
webrtc/audio/audio_receive_stream_unittest.cc:102: const CodecInst& kCodecInst =
fve.GetRecvRecCodecInst();
On 2015/10/19 12:36:24, tommi (webrtc) wrote:
> same here and throughout.

Done.

https://codereview.webrtc.org/1390753002/diff/240001/webrtc/test/fake_voice_e...
File webrtc/test/fake_voice_engine.h (right):

https://codereview.webrtc.org/1390753002/diff/240001/webrtc/test/fake_voice_e...
webrtc/test/fake_voice_engine.h:27: const int kSendChannelId = 1;
On 2015/10/19 12:36:24, tommi (webrtc) wrote:
> static const int (etc)

I'm torn. Part of me wants to do that as well, but what good does it do? I can
only see negatives with this particular use case:

1. Need to add a .cc file
2. More verbose in the unit test

https://codereview.webrtc.org/1390753002/diff/240001/webrtc/test/fake_voice_e...
webrtc/test/fake_voice_engine.h:45: const CallStatistics& GetRecvCallStats()
const {
On 2015/10/19 12:36:24, tommi (webrtc) wrote:
> will this always be called on the same thread for all instances?
> (static construction within a function is intentionally not thread safe with
the
> current build settings)

Yes, this class is for unit testing.

https://codereview.webrtc.org/1390753002/diff/240001/webrtc/test/fake_voice_e...
webrtc/test/fake_voice_engine.h:414: int GetOutputVolumePan(int channel, float&
left, float& right) override {
On 2015/10/19 12:36:24, tommi (webrtc) wrote:
> can we move all the implementation into the cc file?

These interfaces are going away. Most of them are implemented with a "return -1"
and will remain so until gone.

Adding the .cc file contributes little except > doubling the code needed. I'd
rather avoid that.

https://codereview.webrtc.org/1390753002/diff/240001/webrtc/voice_engine/voic...
File webrtc/voice_engine/voice_engine_impl.h (right):

https://codereview.webrtc.org/1390753002/diff/240001/webrtc/voice_engine/voic...
webrtc/voice_engine/voice_engine_impl.h:131: protected:
On 2015/10/19 12:36:24, tommi (webrtc) wrote:
> nit: add a comment why protected?

Done.

[tommi, 2015-10-19 14:55:44]

lgtm - thanks for doing all the changes

https://codereview.webrtc.org/1390753002/diff/240001/talk/media/webrtc/webrtc...
File talk/media/webrtc/webrtcvoiceengine.cc (right):

https://codereview.webrtc.org/1390753002/diff/240001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.cc:2772: rinfo.add_ssrc(stats.remote_ssrc);
On 2015/10/19 14:25:01, the sun wrote:
> On 2015/10/19 12:36:24, tommi (webrtc) wrote:
> > What do you think about moving all this boiler plate to a VoiceReceiveInfo
> ctor?
> 
> You mean forcing VoiceReceiveInfo to know about the AudioReceiveStream::Stats?
I
> think that is breaking encapsulation. We'll be better off deprecating
> VoiceReceiverInfo, going forward.

OK, sgtm. There are a lot of vars being set here so my thinking was that if we
need to do this elsewhere, it would be good to have one place where all this is
and possibly have a ctor for it too so that if one forgets one variable, things
don't compile.  If this is a one-off thing though, then fine with me as is.

https://codereview.webrtc.org/1390753002/diff/240001/talk/media/webrtc/webrtc...
File talk/media/webrtc/webrtcvoiceengine_unittest.cc (right):

https://codereview.webrtc.org/1390753002/diff/240001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine_unittest.cc:326: for (auto s :
call_.GetAudioReceiveStreams()) {
On 2015/10/19 14:25:02, the sun wrote:
> On 2015/10/19 12:36:24, tommi (webrtc) wrote:
> > nit: no curlies.
> > also, just to make it clear that this is not by-value (i.e. creating a copy
> each
> > time), use auto*
> 
> Yes, curlies.
> 
> 1. Style guide avoids the issue.
> 2. Surrounding code (libjingle) generally use curlies for one-liners; style
> guide says prefer consistency.
> 3. Arguments in favor of using curlies (consistency, less risk of copy/paste
> bugs, bad formatting doesn't cause reader misinterpretation) trumps arguments
> against (an extra line).
> 
> :)
> 
> auto* is more readable, thanks.

Re 2. I thought the opposite.
Re 3. (I'll need to break that down)
  - consistency - I don't think that's in favor or against {} in general
  - less risk of copy/paste bugs - nah where there's a will, there's a way, see
next line.
  - bad formatting doesn't cause reader misinterpretation - you'd think not :) 
see slide 33 here:
https://docs.google.com/presentation/d/1WAa-k6n9v9XEUPRxYODrCH5Bvng6KckGkr-7Z...
(it's not clear where the author went wrong, but for sure it fooled the
reviewer)

I understand your point though and don't want to keep you occupied with bike
shedding.  In the end, you're right since this file uses curly brackets for
single line statements like this.

https://codereview.webrtc.org/1390753002/diff/240001/webrtc/audio/audio_recei...
File webrtc/audio/audio_receive_stream.cc (right):

https://codereview.webrtc.org/1390753002/diff/240001/webrtc/audio/audio_recei...
webrtc/audio/audio_receive_stream.cc:126: if (ci.pltype != -1) {
On 2015/10/19 14:25:02, the sun wrote:
> On 2015/10/19 12:36:24, tommi (webrtc) wrote:
> > no {} (and below for single line if()s)
> 
> please, point me to the style guide if I've missed that part.

Consistency. All I did was go up a few lines to see what was previously done in
this file. See line 64.
I'm also more used to reviewers feeling strongly about this in Chromium.

https://codereview.webrtc.org/1390753002/diff/240001/webrtc/audio/audio_recei...
File webrtc/audio/audio_receive_stream_unittest.cc (right):

https://codereview.webrtc.org/1390753002/diff/240001/webrtc/audio/audio_recei...
webrtc/audio/audio_receive_stream_unittest.cc:91: const uint32_t kSsrc1 = 667;
On 2015/10/19 14:25:02, the sun wrote:
> On 2015/10/19 12:36:24, tommi (webrtc) wrote:
> > if ssrc is uint32_t, it would be good to have that consistently be the case.

> I
> > think that in general the style guide doesn't like 'unsigned' though, so
> fwiw...
> 
> The best option would be to create a template to represent an ID of type T,
then
> pass that around. It falls way outside of this CL though and the scope to
> implement throughout the code base is huge.

agreed and sgtm

https://codereview.webrtc.org/1390753002/diff/240001/webrtc/test/fake_voice_e...
File webrtc/test/fake_voice_engine.h (right):

https://codereview.webrtc.org/1390753002/diff/240001/webrtc/test/fake_voice_e...
webrtc/test/fake_voice_engine.h:27: const int kSendChannelId = 1;
On 2015/10/19 14:25:02, the sun wrote:
> On 2015/10/19 12:36:24, tommi (webrtc) wrote:
> > static const int (etc)
> 
> I'm torn. Part of me wants to do that as well, but what good does it do? I can
> only see negatives with this particular use case:
> 
> 1. Need to add a .cc file
> 2. More verbose in the unit test

Well, there's also a preference of not writing inline code in header files, so
perhaps that's some motivation.  E.g. in folders such as media/ in Chrome,
owners will actually enforce this and then it's the consistency argument that
applies (non-template code is generally in cc files).

https://codereview.webrtc.org/1390753002/diff/240001/webrtc/test/fake_voice_e...
webrtc/test/fake_voice_engine.h:414: int GetOutputVolumePan(int channel, float&
left, float& right) override {
On 2015/10/19 14:25:02, the sun wrote:
> On 2015/10/19 12:36:24, tommi (webrtc) wrote:
> > can we move all the implementation into the cc file?
> 
> These interfaces are going away. Most of them are implemented with a "return
-1"
> and will remain so until gone.
> 
> Adding the .cc file contributes little except > doubling the code needed. I'd
> rather avoid that.

OK, cool if it's going away I don't care that much and feel free to ignore the
comment above.

https://codereview.webrtc.org/1390753002/diff/280001/webrtc/audio/audio_recei...
File webrtc/audio/audio_receive_stream.h (right):

https://codereview.webrtc.org/1390753002/diff/280001/webrtc/audio/audio_recei...
webrtc/audio/audio_receive_stream.h:54: ScopedVoEInterface<VoEBase> voe_base_;
nice :)

[the sun, 2015-10-20 08:31:29]

solenberg@webrtc.org changed reviewers:
- kwiberg@webrtc.org

[the sun, 2015-10-20 08:31:29]

https://codereview.webrtc.org/1390753002/diff/240001/talk/media/webrtc/webrtc...
File talk/media/webrtc/webrtcvoiceengine.cc (right):

https://codereview.webrtc.org/1390753002/diff/240001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.cc:2772: rinfo.add_ssrc(stats.remote_ssrc);
On 2015/10/19 14:55:44, tommi (webrtc) wrote:
> On 2015/10/19 14:25:01, the sun wrote:
> > On 2015/10/19 12:36:24, tommi (webrtc) wrote:
> > > What do you think about moving all this boiler plate to a VoiceReceiveInfo
> > ctor?
> > 
> > You mean forcing VoiceReceiveInfo to know about the
AudioReceiveStream::Stats?
> I
> > think that is breaking encapsulation. We'll be better off deprecating
> > VoiceReceiverInfo, going forward.
> 
> OK, sgtm. There are a lot of vars being set here so my thinking was that if we
> need to do this elsewhere, it would be good to have one place where all this
is
> and possibly have a ctor for it too so that if one forgets one variable,
things
> don't compile.  If this is a one-off thing though, then fine with me as is.

Acknowledged.

https://codereview.webrtc.org/1390753002/diff/240001/talk/media/webrtc/webrtc...
File talk/media/webrtc/webrtcvoiceengine_unittest.cc (right):

https://codereview.webrtc.org/1390753002/diff/240001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine_unittest.cc:326: for (auto s :
call_.GetAudioReceiveStreams()) {
On 2015/10/19 14:55:44, tommi (webrtc) wrote:
> On 2015/10/19 14:25:02, the sun wrote:
> > On 2015/10/19 12:36:24, tommi (webrtc) wrote:
> > > nit: no curlies.
> > > also, just to make it clear that this is not by-value (i.e. creating a
copy
> > each
> > > time), use auto*
> > 
> > Yes, curlies.
> > 
> > 1. Style guide avoids the issue.
> > 2. Surrounding code (libjingle) generally use curlies for one-liners; style
> > guide says prefer consistency.
> > 3. Arguments in favor of using curlies (consistency, less risk of copy/paste
> > bugs, bad formatting doesn't cause reader misinterpretation) trumps
arguments
> > against (an extra line).
> > 
> > :)
> > 
> > auto* is more readable, thanks.
> 
> Re 2. I thought the opposite.
> Re 3. (I'll need to break that down)
>   - consistency - I don't think that's in favor or against {} in general
>   - less risk of copy/paste bugs - nah where there's a will, there's a way,
see
> next line.
>   - bad formatting doesn't cause reader misinterpretation - you'd think not :)

> see slide 33 here:
>
https://docs.google.com/presentation/d/1WAa-k6n9v9XEUPRxYODrCH5Bvng6KckGkr-7Z...
> (it's not clear where the author went wrong, but for sure it fooled the
> reviewer)
> 
> I understand your point though and don't want to keep you occupied with bike
> shedding.  In the end, you're right since this file uses curly brackets for
> single line statements like this.

Wonder how often these debates pop up? Goes to show that there should just be a
decision in the style guide...

But since we're on the topic, allow me to shoot back:

- Consistency - yes, if consistently using brackets, there is one less
possibility for the reader to take into account when mentally parsing the code.
- Copy/paste - agree the risk isn't big, but how about badly written macros?
Consistent bracketing avoids such issues.
- As I see it, in the example you give, the problem *is* mixed bracketing. It's
more likely the reviewer (and writer) would have spotted a misaligned } even if
the formatting was messed up.

As a friend of mine once said: "Religions? All good. Just don't try to convert
me." :)

https://codereview.webrtc.org/1390753002/diff/240001/webrtc/audio/audio_recei...
File webrtc/audio/audio_receive_stream.cc (right):

https://codereview.webrtc.org/1390753002/diff/240001/webrtc/audio/audio_recei...
webrtc/audio/audio_receive_stream.cc:126: if (ci.pltype != -1) {
On 2015/10/19 14:55:44, tommi (webrtc) wrote:
> On 2015/10/19 14:25:02, the sun wrote:
> > On 2015/10/19 12:36:24, tommi (webrtc) wrote:
> > > no {} (and below for single line if()s)
> > 
> > please, point me to the style guide if I've missed that part.
> 
> Consistency. All I did was go up a few lines to see what was previously done
in
> this file. See line 64.
> I'm also more used to reviewers feeling strongly about this in Chromium.

Oops, touché. Fixed that. Blaming sloppy copy+paste tactics.

https://codereview.webrtc.org/1390753002/diff/240001/webrtc/test/fake_voice_e...
File webrtc/test/fake_voice_engine.h (right):

https://codereview.webrtc.org/1390753002/diff/240001/webrtc/test/fake_voice_e...
webrtc/test/fake_voice_engine.h:27: const int kSendChannelId = 1;
On 2015/10/19 14:55:44, tommi (webrtc) wrote:
> On 2015/10/19 14:25:02, the sun wrote:
> > On 2015/10/19 12:36:24, tommi (webrtc) wrote:
> > > static const int (etc)
> > 
> > I'm torn. Part of me wants to do that as well, but what good does it do? I
can
> > only see negatives with this particular use case:
> > 
> > 1. Need to add a .cc file
> > 2. More verbose in the unit test
> 
> Well, there's also a preference of not writing inline code in header files, so
> perhaps that's some motivation.  E.g. in folders such as media/ in Chrome,
> owners will actually enforce this and then it's the consistency argument that
> applies (non-template code is generally in cc files).

Acknowledged.

https://codereview.webrtc.org/1390753002/diff/240001/webrtc/test/fake_voice_e...
webrtc/test/fake_voice_engine.h:414: int GetOutputVolumePan(int channel, float&
left, float& right) override {
On 2015/10/19 14:55:44, tommi (webrtc) wrote:
> On 2015/10/19 14:25:02, the sun wrote:
> > On 2015/10/19 12:36:24, tommi (webrtc) wrote:
> > > can we move all the implementation into the cc file?
> > 
> > These interfaces are going away. Most of them are implemented with a "return
> -1"
> > and will remain so until gone.
> > 
> > Adding the .cc file contributes little except > doubling the code needed.
I'd
> > rather avoid that.
> 
> OK, cool if it's going away I don't care that much and feel free to ignore the
> comment above.

Acknowledged.

[the sun, 2015-10-20 09:44:55]

Description was changed from

==========
Implement AudioReceiveStream::GetStats().

BUG=webrtc:4690
==========

to

==========
Implement AudioReceiveStream::GetStats().

TBR=hta@webrtc.org
BUG=webrtc:4690
==========

[the sun, 2015-10-20 09:44:55]

solenberg@webrtc.org changed reviewers:
- hta@webrtc.org

[the sun, 2015-10-20 11:18:53]

The CQ bit was checked by solenberg@webrtc.org

[the sun, 2015-10-20 11:18:53]

The patchset sent to the CQ was uploaded after l-g-t-m from tommi@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1390753002/#ps340001 (title:
"removed unorthogonal bit of getstats test cases")

[commit-bot: I haz the power, 2015-10-20 11:18:59]

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

[commit-bot: I haz the power, 2015-10-20 11:28:04]

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

[commit-bot: I haz the power, 2015-10-20 11:28:05]

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

[commit-bot: I haz the power, 2015-10-20 13:02:15]

Patchset 19 (id:??) landed as
https://crrev.com/a457752f4afc496ed7f4d6b584b08d8635f18cc0
Cr-Commit-Position: refs/heads/master@{#10338}

[the sun, 2015-10-20 13:02:29]

Committed patchset #19 (id:360001) manually as
a457752f4afc496ed7f4d6b584b08d8635f18cc0 (presubmit successful).

[the sun, 2015-10-20 13:40:47]

A revert of this CL (patchset #19 id:360001) has been created in
https://codereview.webrtc.org/1411083006/ by solenberg@webrtc.org.

The reason for reverting is: webrtc_perf_tests started failing on Win32 Release,
Mac32 Release and Linux64 Release (all running large tests). These were not
caught by try bots..

[the sun, 2015-10-20 19:04:00]

Description was changed from

==========
Implement AudioReceiveStream::GetStats().

R=tommi@webrtc.org
TBR=hta@webrtc.org
BUG=webrtc:4690

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

to

==========
Re-Land: Implement AudioReceiveStream::GetStats().

R=tommi@webrtc.org
TBR=hta@webrtc.org
BUG=webrtc:4690

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

[hta-webrtc, 2015-10-20 21:28:59]

hta@webrtc.org changed reviewers:
+ hta@webrtc.org

[hta-webrtc, 2015-10-20 21:29:00]

Had a few comments. Main problem in reviewing for me is that the design and the
usage of the new types are not documented in comments; it's not obvious what the
design is.

Since it's been reverted, perhaps you have time to look at these comments?

https://codereview.webrtc.org/1390753002/diff/360001/webrtc/audio/audio_recei...
File webrtc/audio/audio_receive_stream_unittest.cc (right):

https://codereview.webrtc.org/1390753002/diff/360001/webrtc/audio/audio_recei...
webrtc/audio/audio_receive_stream_unittest.cc:66: FakeVoiceEngine fve;
My eyes tend to glaze over for names like "fve".

https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#General_Nami...
says "use meaningful names", more or less. Can we call it voice_engine?

https://codereview.webrtc.org/1390753002/diff/360001/webrtc/audio/audio_recei...
webrtc/audio/audio_receive_stream_unittest.cc:91: FakeVoiceEngine fve;
Here too, for both rbe and fve.

https://codereview.webrtc.org/1390753002/diff/360001/webrtc/audio/conversion.h
File webrtc/audio/conversion.h (right):

https://codereview.webrtc.org/1390753002/diff/360001/webrtc/audio/conversion....
webrtc/audio/conversion.h:16: inline float Q14ToFloat(uint16_t v) {
What is the derviation of Q14? Is it a well known function?

https://codereview.webrtc.org/1390753002/diff/360001/webrtc/audio/scoped_voe_...
File webrtc/audio/scoped_voe_interface.h (right):

https://codereview.webrtc.org/1390753002/diff/360001/webrtc/audio/scoped_voe_...
webrtc/audio/scoped_voe_interface.h:10: 
Is there a good reason to avoid file or class comments - as in "what is this
thing for"?

https://codereview.webrtc.org/1390753002/diff/360001/webrtc/voice_engine/voic...
File webrtc/voice_engine/voice_engine_impl.h (right):

https://codereview.webrtc.org/1390753002/diff/360001/webrtc/voice_engine/voic...
webrtc/voice_engine/voice_engine_impl.h:135: rtc::scoped_ptr<const Config>
own_config_;
but why is own_config_ left out in the protected realm too?

[the sun, 2015-10-20 22:44:16]

https://codereview.webrtc.org/1390753002/diff/360001/webrtc/audio/audio_recei...
File webrtc/audio/audio_receive_stream_unittest.cc (right):

https://codereview.webrtc.org/1390753002/diff/360001/webrtc/audio/audio_recei...
webrtc/audio/audio_receive_stream_unittest.cc:66: FakeVoiceEngine fve;
On 2015/10/20 21:29:00, hta-webrtc wrote:
> My eyes tend to glaze over for names like "fve".
> 
>
https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#General_Nami...
> says "use meaningful names", more or less. Can we call it voice_engine?
> 

Well, can't argue with that. Or, hey, turns out I can. :) I will change it
anyway, but first, here's my point of view:

I find that using these abbreviated nonsense names *locally* actually enhance
readability. Especially with the 80 chars/line limit (you know, there's a
*reason* certain Java style guides allow 100 chars/line).

It's hard to draw a definite line I think. Just glancing at the code below, I
spot "bwe", "voe", "plc", "cng"; members of data structures with much wider
scope than these local variables. Yes, these are more or less well known
acronyms of the field, but they are much harder to figure out the meaning of
(try Google "RED"). The definition of "fve" in this case is 3 lines above its
use.

Anyhow, I'll make it nice and verbose, with extra line breaks...

https://codereview.webrtc.org/1390753002/diff/360001/webrtc/audio/conversion.h
File webrtc/audio/conversion.h (right):

https://codereview.webrtc.org/1390753002/diff/360001/webrtc/audio/conversion....
webrtc/audio/conversion.h:16: inline float Q14ToFloat(uint16_t v) {
On 2015/10/20 21:29:00, hta-webrtc wrote:
> What is the derviation of Q14? Is it a well known function?

https://en.wikipedia.org/wiki/Q_(number_format)

[Andrew MacDonald, 2015-10-20 22:59:56]

andrew@webrtc.org changed reviewers:
+ andrew@webrtc.org

[Andrew MacDonald, 2015-10-20 22:59:57]

Drive-by.

https://codereview.webrtc.org/1390753002/diff/360001/webrtc/audio/conversion.h
File webrtc/audio/conversion.h (right):

https://codereview.webrtc.org/1390753002/diff/360001/webrtc/audio/conversion....
webrtc/audio/conversion.h:16: inline float Q14ToFloat(uint16_t v) {
On 2015/10/20 22:44:16, the sun wrote:
> On 2015/10/20 21:29:00, hta-webrtc wrote:
> > What is the derviation of Q14? Is it a well known function?
> 
> https://en.wikipedia.org/wiki/Q_(number_format)

Haven't looked at what you're using this for, but might make sense to put it
with the various other conversion functions:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc...

[tommi, 2015-10-21 07:35:08]

https://codereview.webrtc.org/1390753002/diff/360001/webrtc/audio/audio_recei...
File webrtc/audio/audio_receive_stream_unittest.cc (right):

https://codereview.webrtc.org/1390753002/diff/360001/webrtc/audio/audio_recei...
webrtc/audio/audio_receive_stream_unittest.cc:66: FakeVoiceEngine fve;
On 2015/10/20 22:44:16, the sun wrote:
> On 2015/10/20 21:29:00, hta-webrtc wrote:
> > My eyes tend to glaze over for names like "fve".
> > 
> >
>
https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#General_Nami...
> > says "use meaningful names", more or less. Can we call it voice_engine?
> > 
> 
> Well, can't argue with that. Or, hey, turns out I can. :) I will change it
> anyway, but first, here's my point of view:
> 
> I find that using these abbreviated nonsense names *locally* actually enhance
> readability. Especially with the 80 chars/line limit (you know, there's a
> *reason* certain Java style guides allow 100 chars/line).
> 
> It's hard to draw a definite line I think. Just glancing at the code below, I
> spot "bwe", "voe", "plc", "cng"; members of data structures with much wider
> scope than these local variables. Yes, these are more or less well known
> acronyms of the field, but they are much harder to figure out the meaning of
> (try Google "RED"). The definition of "fve" in this case is 3 lines above its
> use.
> 
> Anyhow, I'll make it nice and verbose, with extra line breaks...

+1 on more readable code and less debating with the reviewer.

[the sun, 2015-10-21 08:29:13]

https://codereview.webrtc.org/1390753002/diff/360001/webrtc/audio/audio_recei...
File webrtc/audio/audio_receive_stream_unittest.cc (right):

https://codereview.webrtc.org/1390753002/diff/360001/webrtc/audio/audio_recei...
webrtc/audio/audio_receive_stream_unittest.cc:66: FakeVoiceEngine fve;
On 2015/10/20 21:29:00, hta-webrtc wrote:
> My eyes tend to glaze over for names like "fve".
> 
>
https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#General_Nami...
> says "use meaningful names", more or less. Can we call it voice_engine?
> 

Done.

https://codereview.webrtc.org/1390753002/diff/360001/webrtc/audio/audio_recei...
webrtc/audio/audio_receive_stream_unittest.cc:66: FakeVoiceEngine fve;
On 2015/10/21 07:35:08, tommi (webrtc) wrote:
> On 2015/10/20 22:44:16, the sun wrote:
> > On 2015/10/20 21:29:00, hta-webrtc wrote:
> > > My eyes tend to glaze over for names like "fve".
> > > 
> > >
> >
>
https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#General_Nami...
> > > says "use meaningful names", more or less. Can we call it voice_engine?
> > > 
> > 
> > Well, can't argue with that. Or, hey, turns out I can. :) I will change it
> > anyway, but first, here's my point of view:
> > 
> > I find that using these abbreviated nonsense names *locally* actually
enhance
> > readability. Especially with the 80 chars/line limit (you know, there's a
> > *reason* certain Java style guides allow 100 chars/line).
> > 
> > It's hard to draw a definite line I think. Just glancing at the code below,
I
> > spot "bwe", "voe", "plc", "cng"; members of data structures with much wider
> > scope than these local variables. Yes, these are more or less well known
> > acronyms of the field, but they are much harder to figure out the meaning of
> > (try Google "RED"). The definition of "fve" in this case is 3 lines above
its
> > use.
> > 
> > Anyhow, I'll make it nice and verbose, with extra line breaks...
> 
> +1 on more readable code and less debating with the reviewer.

Acknowledged.

https://codereview.webrtc.org/1390753002/diff/360001/webrtc/audio/audio_recei...
webrtc/audio/audio_receive_stream_unittest.cc:91: FakeVoiceEngine fve;
On 2015/10/20 21:29:00, hta-webrtc wrote:
> Here too, for both rbe and fve.

Done.

https://codereview.webrtc.org/1390753002/diff/360001/webrtc/audio/conversion.h
File webrtc/audio/conversion.h (right):

https://codereview.webrtc.org/1390753002/diff/360001/webrtc/audio/conversion....
webrtc/audio/conversion.h:16: inline float Q14ToFloat(uint16_t v) {
On 2015/10/20 22:59:57, Andrew MacDonald wrote:
> On 2015/10/20 22:44:16, the sun wrote:
> > On 2015/10/20 21:29:00, hta-webrtc wrote:
> > > What is the derviation of Q14? Is it a well known function?
> > 
> > https://en.wikipedia.org/wiki/Q_(number_format)
> 
> Haven't looked at what you're using this for, but might make sense to put it
> with the various other conversion functions:
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc...

I looked at that but decided against, since pulling in common_audio here, just
for that one function seemed heavy handed.

Also, putting the function there isn't a 100% fit, since it is used to convert
the Q14 numbers in NetworkStatistics. NetworkStatistics is defined in
common_types.h. I contemplated putting the conversion function right inside the
struct but decided the scope was too wide, hence it ended up in its own file,
locally to the webrtc/audio interface implementation.

Out of the available options, I think the current solution, or putting the
converter inside NetworkStatistics makes the most sense. WDYT?

https://codereview.webrtc.org/1390753002/diff/360001/webrtc/audio/conversion....
webrtc/audio/conversion.h:16: inline float Q14ToFloat(uint16_t v) {
On 2015/10/20 22:44:16, the sun wrote:
> On 2015/10/20 21:29:00, hta-webrtc wrote:
> > What is the derviation of Q14? Is it a well known function?
> 
> https://en.wikipedia.org/wiki/Q_(number_format)

Since you had to ask, there is definitely a need for a comment, so I added one.

https://codereview.webrtc.org/1390753002/diff/360001/webrtc/audio/scoped_voe_...
File webrtc/audio/scoped_voe_interface.h (right):

https://codereview.webrtc.org/1390753002/diff/360001/webrtc/audio/scoped_voe_...
webrtc/audio/scoped_voe_interface.h:10: 
On 2015/10/20 21:29:00, hta-webrtc wrote:
> Is there a good reason to avoid file or class comments - as in "what is this
> thing for"?

Done.

https://codereview.webrtc.org/1390753002/diff/360001/webrtc/voice_engine/voic...
File webrtc/voice_engine/voice_engine_impl.h (right):

https://codereview.webrtc.org/1390753002/diff/360001/webrtc/voice_engine/voic...
webrtc/voice_engine/voice_engine_impl.h:135: rtc::scoped_ptr<const Config>
own_config_;
On 2015/10/20 21:29:00, hta-webrtc wrote:
> but why is own_config_ left out in the protected realm too?

Good catch.

[the sun, 2015-10-21 08:36:31]

Description was changed from

==========
Re-Land: Implement AudioReceiveStream::GetStats().

R=tommi@webrtc.org
TBR=hta@webrtc.org
BUG=webrtc:4690

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

to

==========
Re-Land: Implement AudioReceiveStream::GetStats().

R=tommi@webrtc.org
BUG=webrtc:4690

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

[the sun, 2015-10-21 14:31:19]

https://codereview.webrtc.org/1390753002/diff/420001/talk/media/webrtc/webrtc...
File talk/media/webrtc/webrtcvoiceengine_unittest.cc (right):

https://codereview.webrtc.org/1390753002/diff/420001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine_unittest.cc:2051: EXPECT_EQ(kSsrcs4[i],
info.senders[i].ssrc());
Q: hta@, is there any order requirement on the returned stats?

This test enforces one of two possible orderings: creation order or
increasing-ssrc order. Unclear which one is intended.

[Andrew MacDonald, 2015-10-21 18:15:58]

https://codereview.webrtc.org/1390753002/diff/360001/webrtc/audio/conversion.h
File webrtc/audio/conversion.h (right):

https://codereview.webrtc.org/1390753002/diff/360001/webrtc/audio/conversion....
webrtc/audio/conversion.h:16: inline float Q14ToFloat(uint16_t v) {
On 2015/10/21 08:29:13, the sun wrote:
> On 2015/10/20 22:59:57, Andrew MacDonald wrote:
> > On 2015/10/20 22:44:16, the sun wrote:
> > > On 2015/10/20 21:29:00, hta-webrtc wrote:
> > > > What is the derviation of Q14? Is it a well known function?
> > > 
> > > https://en.wikipedia.org/wiki/Q_(number_format)
> > 
> > Haven't looked at what you're using this for, but might make sense to put it
> > with the various other conversion functions:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc...
> 
> I looked at that but decided against, since pulling in common_audio here, just
> for that one function seemed heavy handed.
> 
> Also, putting the function there isn't a 100% fit, since it is used to convert
> the Q14 numbers in NetworkStatistics. NetworkStatistics is defined in
> common_types.h. I contemplated putting the conversion function right inside
the
> struct but decided the scope was too wide, hence it ended up in its own file,
> locally to the webrtc/audio interface implementation.
> 
> Out of the available options, I think the current solution, or putting the
> converter inside NetworkStatistics makes the most sense. WDYT?

I think what you have is good. Thanks for the explanation.

[the sun, 2015-10-22 08:15:59]

solenberg@webrtc.org changed reviewers:
- andrew@webrtc.org

[the sun, 2015-10-22 08:49:44]

Committed patchset #23 (id:440001) manually as
4f4ec0a9270a8cefadfa12e9fa3b979b58b15392 (presubmit successful).

[commit-bot: I haz the power, 2015-10-22 08:49:49]

Patchset 23 (id:??) landed as
https://crrev.com/4f4ec0a9270a8cefadfa12e9fa3b979b58b15392
Cr-Commit-Position: refs/heads/master@{#10369}

[hta-webrtc, 2015-10-22 10:57:28]

Reply to issue about ordering.

https://codereview.webrtc.org/1390753002/diff/420001/talk/media/webrtc/webrtc...
File talk/media/webrtc/webrtcvoiceengine_unittest.cc (right):

https://codereview.webrtc.org/1390753002/diff/420001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine_unittest.cc:2051: EXPECT_EQ(kSsrcs4[i],
info.senders[i].ssrc());
On 2015/10/21 14:31:19, the sun wrote:
> Q: hta@, is there any order requirement on the returned stats?
> 
> This test enforces one of two possible orderings: creation order or
> increasing-ssrc order. Unclear which one is intended.

The JS API interface was created to be oblivious to order; any order is legal.
So this test, by testing that the SSRCs occur in a specific order, will test
something that statscollector.cc doesn't depend on.

Ideally, one would look up each SSRC by value in info.senders, and verify that
the number of SSRCs was correct - this would be testing to the contract rather
than testing to the implementation.

Your call on whether to do it that way.