Issue 1414743004: Implement AudioSendStream::GetStats().

Issue 1414743004: Implement AudioSendStream::GetStats(). (Closed)

Created:
5 years, 2 months ago by the sun

Modified:
5 years, 1 month ago

Reviewers:
tommi, kwiberg-webrtc

CC:
webrtc-reviews_webrtc.org, interface-changes_webrtc.org, rwolff_gocast.it, yujie_mao (webrtc), Andrew MacDonald, tterriberry_mozilla.com, qiang.lu, niklas.enbom, peah-webrtc, kwiberg-webrtc, tlegrand-webrtc

Base URL:
https://chromium.googlesource.com/external/webrtc.git@master

Target Ref:
refs/pending/heads/master

Project:
webrtc

Visibility:
Public.

More Reviews

Description

Implement AudioSendStream::GetStats(). BUG=webrtc:4690 Committed: https://crrev.com/85a0496b8c4ac01da7c716ea7950093659864c8e Cr-Commit-Position: refs/heads/master@{#10424}

Patch Set 1 #

Patch Set 2 : TODOs #

Total comments: 52

Patch Set 3 : comments #

Total comments: 2

Patch Set 4 : win compile error+comments #

Patch Set 5 : const fail #

Patch Set 6 : comments #

Total comments: 18

Patch Set 7 : android compile error #

Patch Set 8 : desperate attempt #

Patch Set 9 : comments #

Patch Set 10 : workaround for android build issue #

Created: 5 years, 1 month ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+713 lines, -411 lines)			Patch
M	talk/media/webrtc/fakewebrtccall.h	View		3 chunks	+6 lines, -6 lines	0 comments	Download
M	talk/media/webrtc/fakewebrtccall.cc	View		3 chunks	+11 lines, -2 lines	0 comments	Download
M	talk/media/webrtc/fakewebrtcvoiceengine.h	View	1 2 4	6 chunks	+10 lines, -44 lines	0 comments	Download
M	talk/media/webrtc/webrtcvoiceengine.cc	View	1 2 3 4 5 6 7 8	9 chunks	+44 lines, -105 lines	0 comments	Download
M	talk/media/webrtc/webrtcvoiceengine_unittest.cc	View	1 2 3 4 5 6 7 8 9	22 chunks	+176 lines, -156 lines	0 comments	Download
M	webrtc/audio/audio_receive_stream.h	View		2 chunks	+3 lines, -1 line	0 comments	Download
M	webrtc/audio/audio_receive_stream.cc	View	1 2	5 chunks	+21 lines, -16 lines	0 comments	Download
M	webrtc/audio/audio_receive_stream_unittest.cc	View	1 2 3 4 5 6 7 8	2 chunks	+36 lines, -15 lines	0 comments	Download
M	webrtc/audio/audio_send_stream.h	View		2 chunks	+16 lines, -5 lines	0 comments	Download
M	webrtc/audio/audio_send_stream.cc	View	1 2 3 4 5 6 7 8	3 chunks	+115 lines, -5 lines	0 comments	Download
M	webrtc/audio/audio_send_stream_unittest.cc	View	1 2 3 4 5 6 7 8 9	2 chunks	+44 lines, -2 lines	0 comments	Download
M	webrtc/audio/conversion.h	View		1 chunk	+6 lines, -1 line	0 comments	Download
M	webrtc/audio_send_stream.h	View		1 chunk	+19 lines, -1 line	0 comments	Download
M	webrtc/call/call.cc	View	1 2 3 4 5 6 7 8 9	2 chunks	+2 lines, -2 lines	0 comments	Download
M	webrtc/test/fake_voice_engine.h	View	1 2 3	7 chunks	+133 lines, -50 lines	0 comments	Download
A	webrtc/test/fake_voice_engine.cc	View	1 2	1 chunk	+70 lines, -0 lines	0 comments	Download
M	webrtc/test/webrtc_test_common.gyp	View	1 2	1 chunk	+1 line, -0 lines	0 comments	Download

Messages

Total messages: 18 (5 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

tommi

https://codereview.webrtc.org/1414743004/diff/20001/talk/media/webrtc/fakewebrtccall.cc File talk/media/webrtc/fakewebrtccall.cc (right): https://codereview.webrtc.org/1414743004/diff/20001/talk/media/webrtc/fakewebrtccall.cc#newcode44 talk/media/webrtc/fakewebrtccall.cc:44: stats_ = stats; if you can have a thread ...

5 years, 2 months ago (2015-10-23 12:50:37 UTC) #4

the sun

5 years, 2 months ago (2015-10-23 15:14:17 UTC) #5

https://codereview.webrtc.org/1414743004/diff/20001/talk/media/webrtc/fakeweb...
File talk/media/webrtc/fakewebrtccall.cc (right):

https://codereview.webrtc.org/1414743004/diff/20001/talk/media/webrtc/fakeweb...
talk/media/webrtc/fakewebrtccall.cc:44: stats_ = stats;
On 2015/10/23 12:50:37, tommi (webrtc) wrote:
> if you can have a thread checker here I think that would be a good thing even
> though this is just a test class.

Hmm. Hmm. Hmm. No.

For that to have any good effects the threading assumptions must be kept in
tight sync with the real AudioSendStream. Such implicit assumptions are likely
to fall short.

This class is intended for injection in unit tests for
WebRtcVoiceEngine/WebRtcVoiceMediaChannel and should be as simple as ever
possible, else Gödel comes chasing us. Threading assumptions of the webrtc::Call
classes should be tested separately, and integration tests should verify that
the classes make the same assumptions.

https://codereview.webrtc.org/1414743004/diff/20001/talk/media/webrtc/fakeweb...
talk/media/webrtc/fakewebrtccall.cc:53: return stats_;
On 2015/10/23 12:50:37, tommi (webrtc) wrote:
> i.e. same thread as this runs on and any other call that touches stats_

Know what you're saying, but see above comment.

https://codereview.webrtc.org/1414743004/diff/20001/talk/media/webrtc/fakeweb...
File talk/media/webrtc/fakewebrtcvoiceengine.h (right):

https://codereview.webrtc.org/1414743004/diff/20001/talk/media/webrtc/fakeweb...
talk/media/webrtc/fakewebrtcvoiceengine.h:266: bool ec_metrics_enabled() {
return ec_metrics_enabled_; }
On 2015/10/23 12:50:37, tommi (webrtc) wrote:
> nit: const?

Yeah!

https://codereview.webrtc.org/1414743004/diff/20001/talk/media/webrtc/webrtcv...
File talk/media/webrtc/webrtcvoiceengine.cc (right):

https://codereview.webrtc.org/1414743004/diff/20001/talk/media/webrtc/webrtcv...
talk/media/webrtc/webrtcvoiceengine.cc:1324: RTC_DCHECK(call);
On 2015/10/23 12:50:37, tommi (webrtc) wrote:
> nit: Since call is already used, this check isn't necessary

call/call_ aren't dereferenced yet, in the context of this class. I'm keeping
the assert. Fail early. Enforce invariant.

https://codereview.webrtc.org/1414743004/diff/20001/talk/media/webrtc/webrtcv...
talk/media/webrtc/webrtcvoiceengine.cc:1328: stream_ =
call_->CreateAudioSendStream(config);
On 2015/10/23 12:50:37, tommi (webrtc) wrote:
> can we make stream_ const?  I.e.
> webrtc::AudioSendStream* const stream_;
> 
> This would mean the initialization of config has to be factored out to a
> separate function or a new ctor added, but I think it's worth it to make it
> clear that using |stream_| without a thread check, is safe.
> In the initializer list this could then look like this (+wrapping):
> 
> stream_(call->CreateAudioSendStream(webrtc::AudioSendStream::Config(nullptr,
ch,
> ssrc)),

I can foresee the need subsequently to call non-const methods on *stream_: i.e.
Start(), Stop(), DeliverRtcp(). So, not making it const.

https://codereview.webrtc.org/1414743004/diff/20001/talk/media/webrtc/webrtcv...
talk/media/webrtc/webrtcvoiceengine.cc:1338: // This method is called on the
libjingle worker thread.
On 2015/10/23 12:50:37, tommi (webrtc) wrote:
> would be great to have a thread check for this :D

You do fancy those thread checkers! :)

https://codereview.webrtc.org/1414743004/diff/20001/talk/media/webrtc/webrtcv...
talk/media/webrtc/webrtcvoiceengine.cc:1373: RTC_DCHECK(voe_audio_transport_);
On 2015/10/23 12:50:37, tommi (webrtc) wrote:
> nit: this dcheck isn't necessary either so you could remove it if you want

You're right in principle. The check should go in the c-tor so that we enforce
the invariant of the class at time of creation.

However, right now FakeWebRtcVoiceEngine (as opposed to FakeVoiceEngine, to be
clear), will return a NULL audio transport. I'm adding a TODO to move the assert
once we no longer need that class for testing.

https://codereview.webrtc.org/1414743004/diff/20001/talk/media/webrtc/webrtcv...
talk/media/webrtc/webrtcvoiceengine.cc:1384: void OnClose() override {
On 2015/10/23 12:50:37, tommi (webrtc) wrote:
> Do you know on what thread this method runs?  I'm wondering since both of the
> other methods that touch renderer_ are documented to run on the worker thread
> and if this one does too, then the lock isn't necessary (right?)

Indeed. The lock appears not useful.

So. Much. To. Clean.

https://codereview.webrtc.org/1414743004/diff/20001/talk/media/webrtc/webrtcv...
talk/media/webrtc/webrtcvoiceengine.cc:2627: info->senders.clear();
On 2015/10/23 12:50:37, tommi (webrtc) wrote:
> is this necessary in practice or will senders always be empty?  If it is an
> assumption of GetStats that this array always be empty, then we could just
have
> a DCHECK() here that makes sure it's always empty.  I'd prefer that actually
> since clearing the array here, could mask a bug somewhere else.

I absolute agree. Thanks for pointing that out; prefer offensive programming to
defensive programming. ;)

https://codereview.webrtc.org/1414743004/diff/20001/talk/media/webrtc/webrtcv...
talk/media/webrtc/webrtcvoiceengine.cc:2630: VoiceSenderInfo sinfo;
On 2015/10/23 12:50:37, tommi (webrtc) wrote:
> Will VoiceSenderInfo eventually go away or possibly have the below members
> replaced by a AudioSendStream::Stats variable?

Yes, possibly.

https://codereview.webrtc.org/1414743004/diff/20001/talk/media/webrtc/webrtcv...
File talk/media/webrtc/webrtcvoiceengine_unittest.cc (right):

https://codereview.webrtc.org/1414743004/diff/20001/talk/media/webrtc/webrtcv...
talk/media/webrtc/webrtcvoiceengine_unittest.cc:292: static
webrtc::AudioSendStream::Stats stats;
On 2015/10/23 12:50:37, tommi (webrtc) wrote:
> can you add a comment why this is static and not e.g. a member? (is it an
> optimization?) 

Nah, if that's required I rather return by value.

https://codereview.webrtc.org/1414743004/diff/20001/talk/media/webrtc/webrtcv...
talk/media/webrtc/webrtcvoiceengine_unittest.cc:319: const auto& kStats =
GetAudioSendStreamStats();
On 2015/10/23 12:50:37, tommi (webrtc) wrote:
> kStats -> stats since this is not a compile time constant

Done.

https://codereview.webrtc.org/1414743004/diff/20001/webrtc/audio/audio_receiv...
File webrtc/audio/audio_receive_stream.cc (right):

https://codereview.webrtc.org/1414743004/diff/20001/webrtc/audio/audio_receiv...
webrtc/audio/audio_receive_stream.cc:111: webrtc::CallStatistics call_stats =
{0};
On 2015/10/23 12:50:37, tommi (webrtc) wrote:
> nice :)

Acknowledged.

https://codereview.webrtc.org/1414743004/diff/20001/webrtc/audio/audio_receiv...
File webrtc/audio/audio_receive_stream_unittest.cc (right):

https://codereview.webrtc.org/1414743004/diff/20001/webrtc/audio/audio_receiv...
webrtc/audio/audio_receive_stream_unittest.cc:90: config.voe_channel_id =
voice_engine.kRecvChannelId;
On 2015/10/23 12:50:37, tommi (webrtc) wrote:
> it looks a bit strange to me to have a constant referred to like this.  If
this
> is a class level constant, I'd prefer to have it referred to by the class name
> and not an instance of the class.  E.g. Foo::kBar.  If it's actually a const
> member who's value varies from one instance to another, then it should be
named
> recv_channel_id_.

Done.

https://codereview.webrtc.org/1414743004/diff/20001/webrtc/audio/audio_receiv...
webrtc/audio/audio_receive_stream_unittest.cc:113: config.rtp.remote_ssrc =
voice_engine.kRecvSsrc;
On 2015/10/23 12:50:37, tommi (webrtc) wrote:
> same here

Done.

https://codereview.webrtc.org/1414743004/diff/20001/webrtc/audio/audio_send_s...
File webrtc/audio/audio_send_stream.cc (right):

https://codereview.webrtc.org/1414743004/diff/20001/webrtc/audio/audio_send_s...
webrtc/audio/audio_send_stream.cc:56:
RTC_DCHECK(thread_checker_.CalledOnValidThread());
On 2015/10/23 12:50:37, tommi (webrtc) wrote:
> nit: since we're in the ctor, this will always succeed

Ah, right, I thought TC wasn't initialized until the first
CalledOnValidThread(). Memory was wrong. Fixed here, as well as in Call,
AudioReceiveStream and WVoMC.

https://codereview.webrtc.org/1414743004/diff/20001/webrtc/audio/audio_send_s...
webrtc/audio/audio_send_stream.cc:68:
RTC_DCHECK(thread_checker_.CalledOnValidThread());
On 2015/10/23 12:50:37, tommi (webrtc) wrote:
> I'm digging the thread checks :)

Acknowledged.

https://codereview.webrtc.org/1414743004/diff/20001/webrtc/test/fake_voice_en...
File webrtc/test/fake_voice_engine.h (right):

https://codereview.webrtc.org/1414743004/diff/20001/webrtc/test/fake_voice_en...
webrtc/test/fake_voice_engine.h:28: const int kRecvChannelId = 2;
On 2015/10/23 12:50:37, tommi (webrtc) wrote:
> I think these should all be static and be referenced via the class name (not
an
> instance).

Ok, you get the .cc :)

https://codereview.webrtc.org/1414743004/diff/20001/webrtc/test/fake_voice_en...
webrtc/test/fake_voice_engine.h:52: static const CallStatistics kStats = {
On 2015/10/23 12:50:37, tommi (webrtc) wrote:
> I'm a bit on the fence whether using statics like this is a good idea.  I
don't
> think this really needs to be optimized, so can we have a const stats_ member
> variable instead?  The thing I'm worried about is not so much exactly this
> particular use of the pattern but the pattern itself.
> 
> It has the potential to cause unwanted side effects if more than one instance
of
> the class attempts to initialize the static on different threads.  We could
> either have a race if static initialization is not thread safe (current
default
> build config) or contention.
> If we use this pattern then with a non-const static variable, one test
touching
> the static variable could affect the outcome of another test (i.e. singleton
> anti pattern).
> 
> Actually, why do these methods need to return a reference?  (they're not
> overrides, right?)

I've made this static const structs and moved them to the .cc.

kwiberg-webrtc

https://codereview.webrtc.org/1414743004/diff/20001/webrtc/test/fake_voice_engine.h File webrtc/test/fake_voice_engine.h (right): https://codereview.webrtc.org/1414743004/diff/20001/webrtc/test/fake_voice_engine.h#newcode52 webrtc/test/fake_voice_engine.h:52: static const CallStatistics kStats = { On 2015/10/23 12:50:37, ...

5 years, 2 months ago (2015-10-25 02:07:43 UTC) #7

the sun

5 years, 1 month ago (2015-10-26 09:11:02 UTC) #8

tommi

Thanks for the updates. To be clear, my comment was more about the singleton pattern ...

5 years, 1 month ago (2015-10-26 09:54:34 UTC) #9

Thanks for the updates.  To be clear, my comment was more about the singleton
pattern than specifically the usage of it in the Fake class.
Singletons are bad in production code (actually test code too) and we should
avoid them even after we get thread safe static initialization.

lgtm.  I'll leave it up to you how you want to address the below.

https://codereview.webrtc.org/1414743004/diff/20001/talk/media/webrtc/webrtcv...
File talk/media/webrtc/webrtcvoiceengine.cc (right):

https://codereview.webrtc.org/1414743004/diff/20001/talk/media/webrtc/webrtcv...
talk/media/webrtc/webrtcvoiceengine.cc:1324: RTC_DCHECK(call);
On 2015/10/23 15:14:17, the sun wrote:
> On 2015/10/23 12:50:37, tommi (webrtc) wrote:
> > nit: Since call is already used, this check isn't necessary
> 
> call/call_ aren't dereferenced yet, in the context of this class. I'm keeping
> the assert. Fail early. Enforce invariant.

Alright with me if you feel strongly about keeping it.

The convention is though to not DCHECK pointers that will be dereferenced anyway
(ask any senior Chromium committer).  If there was an early return before
dereferencing though, it would be another matter.

https://codereview.webrtc.org/1414743004/diff/20001/talk/media/webrtc/webrtcv...
talk/media/webrtc/webrtcvoiceengine.cc:1328: stream_ =
call_->CreateAudioSendStream(config);
On 2015/10/23 15:14:17, the sun wrote:
> On 2015/10/23 12:50:37, tommi (webrtc) wrote:
> > can we make stream_ const?  I.e.
> > webrtc::AudioSendStream* const stream_;
> > 
> > This would mean the initialization of config has to be factored out to a
> > separate function or a new ctor added, but I think it's worth it to make it
> > clear that using |stream_| without a thread check, is safe.
> > In the initializer list this could then look like this (+wrapping):
> > 
> > stream_(call->CreateAudioSendStream(webrtc::AudioSendStream::Config(nullptr,
> ch,
> > ssrc)),
> 
> I can foresee the need subsequently to call non-const methods on *stream_:
i.e.
> Start(), Stop(), DeliverRtcp(). So, not making it const.

OK, sgtm. thanks for adding the thread checks.

https://codereview.webrtc.org/1414743004/diff/20001/talk/media/webrtc/webrtcv...
talk/media/webrtc/webrtcvoiceengine.cc:1338: // This method is called on the
libjingle worker thread.
On 2015/10/23 15:14:17, the sun wrote:
> On 2015/10/23 12:50:37, tommi (webrtc) wrote:
> > would be great to have a thread check for this :D
> 
> You do fancy those thread checkers! :)

Yes! I consider them at least as valuable as unit tests, they are so much
quicker to write and they are self-enforcing documentation.  I have been burned
many times :)

https://codereview.webrtc.org/1414743004/diff/20001/talk/media/webrtc/webrtcv...
talk/media/webrtc/webrtcvoiceengine.cc:1373: RTC_DCHECK(voe_audio_transport_);
On 2015/10/23 15:14:17, the sun wrote:
> On 2015/10/23 12:50:37, tommi (webrtc) wrote:
> > nit: this dcheck isn't necessary either so you could remove it if you want
> 
> You're right in principle. The check should go in the c-tor so that we enforce
> the invariant of the class at time of creation.
> 
> However, right now FakeWebRtcVoiceEngine (as opposed to FakeVoiceEngine, to be
> clear), will return a NULL audio transport. I'm adding a TODO to move the
assert
> once we no longer need that class for testing.

See above for DCHECKs.  You could move the TODO to the ctor.

https://codereview.webrtc.org/1414743004/diff/20001/talk/media/webrtc/webrtcv...
talk/media/webrtc/webrtcvoiceengine.cc:1384: void OnClose() override {
On 2015/10/23 15:14:16, the sun wrote:
> On 2015/10/23 12:50:37, tommi (webrtc) wrote:
> > Do you know on what thread this method runs?  I'm wondering since both of
the
> > other methods that touch renderer_ are documented to run on the worker
thread
> > and if this one does too, then the lock isn't necessary (right?)
> 
> Indeed. The lock appears not useful.

W00t!

> So. Much. To. Clean.

Yes and it's already looking so much better :)

https://codereview.webrtc.org/1414743004/diff/20001/talk/media/webrtc/webrtcv...
talk/media/webrtc/webrtcvoiceengine.cc:1406: rtc::CriticalSection lock_;
Nice!

https://codereview.webrtc.org/1414743004/diff/40001/webrtc/audio/audio_receiv...
File webrtc/audio/audio_receive_stream_unittest.cc (right):

https://codereview.webrtc.org/1414743004/diff/40001/webrtc/audio/audio_receiv...
webrtc/audio/audio_receive_stream_unittest.cc:90: config.voe_channel_id =
voice_engine.kRecvChannelId;
nit: FakeVoiceEngine::

the sun

https://codereview.webrtc.org/1414743004/diff/20001/talk/media/webrtc/webrtcvoiceengine.cc File talk/media/webrtc/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/1414743004/diff/20001/talk/media/webrtc/webrtcvoiceengine.cc#newcode1324 talk/media/webrtc/webrtcvoiceengine.cc:1324: RTC_DCHECK(call); On 2015/10/26 09:54:34, tommi (webrtc) wrote: > On ...

5 years, 1 month ago (2015-10-26 10:54:35 UTC) #10

https://codereview.webrtc.org/1414743004/diff/20001/talk/media/webrtc/webrtcv...
File talk/media/webrtc/webrtcvoiceengine.cc (right):

https://codereview.webrtc.org/1414743004/diff/20001/talk/media/webrtc/webrtcv...
talk/media/webrtc/webrtcvoiceengine.cc:1324: RTC_DCHECK(call);
On 2015/10/26 09:54:34, tommi (webrtc) wrote:
> On 2015/10/23 15:14:17, the sun wrote:
> > On 2015/10/23 12:50:37, tommi (webrtc) wrote:
> > > nit: Since call is already used, this check isn't necessary
> > 
> > call/call_ aren't dereferenced yet, in the context of this class. I'm
keeping
> > the assert. Fail early. Enforce invariant.
> 
> Alright with me if you feel strongly about keeping it.
> 
> The convention is though to not DCHECK pointers that will be dereferenced
anyway
> (ask any senior Chromium committer).  If there was an early return before
> dereferencing though, it would be another matter.

Acknowledged.

https://codereview.webrtc.org/1414743004/diff/20001/talk/media/webrtc/webrtcv...
talk/media/webrtc/webrtcvoiceengine.cc:1328: stream_ =
call_->CreateAudioSendStream(config);
On 2015/10/26 09:54:34, tommi (webrtc) wrote:
> On 2015/10/23 15:14:17, the sun wrote:
> > On 2015/10/23 12:50:37, tommi (webrtc) wrote:
> > > can we make stream_ const?  I.e.
> > > webrtc::AudioSendStream* const stream_;
> > > 
> > > This would mean the initialization of config has to be factored out to a
> > > separate function or a new ctor added, but I think it's worth it to make
it
> > > clear that using |stream_| without a thread check, is safe.
> > > In the initializer list this could then look like this (+wrapping):
> > > 
> > >
stream_(call->CreateAudioSendStream(webrtc::AudioSendStream::Config(nullptr,
> > ch,
> > > ssrc)),
> > 
> > I can foresee the need subsequently to call non-const methods on *stream_:
> i.e.
> > Start(), Stop(), DeliverRtcp(). So, not making it const.
> 
> OK, sgtm. thanks for adding the thread checks.

Acknowledged.

https://codereview.webrtc.org/1414743004/diff/20001/talk/media/webrtc/webrtcv...
talk/media/webrtc/webrtcvoiceengine.cc:1338: // This method is called on the
libjingle worker thread.
On 2015/10/26 09:54:34, tommi (webrtc) wrote:
> On 2015/10/23 15:14:17, the sun wrote:
> > On 2015/10/23 12:50:37, tommi (webrtc) wrote:
> > > would be great to have a thread check for this :D
> > 
> > You do fancy those thread checkers! :)
> 
> Yes! I consider them at least as valuable as unit tests, they are so much
> quicker to write and they are self-enforcing documentation.  I have been
burned
> many times :)

Acknowledged.

https://codereview.webrtc.org/1414743004/diff/20001/talk/media/webrtc/webrtcv...
talk/media/webrtc/webrtcvoiceengine.cc:1373: RTC_DCHECK(voe_audio_transport_);
On 2015/10/26 09:54:34, tommi (webrtc) wrote:
> On 2015/10/23 15:14:17, the sun wrote:
> > On 2015/10/23 12:50:37, tommi (webrtc) wrote:
> > > nit: this dcheck isn't necessary either so you could remove it if you want
> > 
> > You're right in principle. The check should go in the c-tor so that we
enforce
> > the invariant of the class at time of creation.
> > 
> > However, right now FakeWebRtcVoiceEngine (as opposed to FakeVoiceEngine, to
be
> > clear), will return a NULL audio transport. I'm adding a TODO to move the
> assert
> > once we no longer need that class for testing.
> 
> See above for DCHECKs.  You could move the TODO to the ctor.

Done.

https://codereview.webrtc.org/1414743004/diff/20001/talk/media/webrtc/webrtcv...
talk/media/webrtc/webrtcvoiceengine.cc:1384: void OnClose() override {
On 2015/10/26 09:54:34, tommi (webrtc) wrote:
> On 2015/10/23 15:14:16, the sun wrote:
> > On 2015/10/23 12:50:37, tommi (webrtc) wrote:
> > > Do you know on what thread this method runs?  I'm wondering since both of
> the
> > > other methods that touch renderer_ are documented to run on the worker
> thread
> > > and if this one does too, then the lock isn't necessary (right?)
> > 
> > Indeed. The lock appears not useful.
> 
> W00t!
>  
> > So. Much. To. Clean.
> 
> Yes and it's already looking so much better :)

Acknowledged.

https://codereview.webrtc.org/1414743004/diff/20001/talk/media/webrtc/webrtcv...
talk/media/webrtc/webrtcvoiceengine.cc:1406: rtc::CriticalSection lock_;
On 2015/10/26 09:54:34, tommi (webrtc) wrote:
> Nice!

Acknowledged.

https://codereview.webrtc.org/1414743004/diff/40001/webrtc/audio/audio_receiv...
File webrtc/audio/audio_receive_stream_unittest.cc (right):

https://codereview.webrtc.org/1414743004/diff/40001/webrtc/audio/audio_receiv...
webrtc/audio/audio_receive_stream_unittest.cc:90: config.voe_channel_id =
voice_engine.kRecvChannelId;
On 2015/10/26 09:54:34, tommi (webrtc) wrote:
> nit: FakeVoiceEngine::

Done.

the sun

On 2015/10/26 10:54:35, the sun wrote: > https://codereview.webrtc.org/1414743004/diff/20001/talk/media/webrtc/webrtcvoiceengine.cc > File talk/media/webrtc/webrtcvoiceengine.cc (right): > > https://codereview.webrtc.org/1414743004/diff/20001/talk/media/webrtc/webrtcvoiceengine.cc#newcode1324 ...

5 years, 1 month ago (2015-10-26 10:56:02 UTC) #11

On 2015/10/26 10:54:35, the sun wrote:
>
https://codereview.webrtc.org/1414743004/diff/20001/talk/media/webrtc/webrtcv...
> File talk/media/webrtc/webrtcvoiceengine.cc (right):
> 
>
https://codereview.webrtc.org/1414743004/diff/20001/talk/media/webrtc/webrtcv...
> talk/media/webrtc/webrtcvoiceengine.cc:1324: RTC_DCHECK(call);
> On 2015/10/26 09:54:34, tommi (webrtc) wrote:
> > On 2015/10/23 15:14:17, the sun wrote:
> > > On 2015/10/23 12:50:37, tommi (webrtc) wrote:
> > > > nit: Since call is already used, this check isn't necessary
> > > 
> > > call/call_ aren't dereferenced yet, in the context of this class. I'm
> keeping
> > > the assert. Fail early. Enforce invariant.
> > 
> > Alright with me if you feel strongly about keeping it.
> > 
> > The convention is though to not DCHECK pointers that will be dereferenced
> anyway
> > (ask any senior Chromium committer).  If there was an early return before
> > dereferencing though, it would be another matter.
> 
> Acknowledged.
> 
>
https://codereview.webrtc.org/1414743004/diff/20001/talk/media/webrtc/webrtcv...
> talk/media/webrtc/webrtcvoiceengine.cc:1328: stream_ =
> call_->CreateAudioSendStream(config);
> On 2015/10/26 09:54:34, tommi (webrtc) wrote:
> > On 2015/10/23 15:14:17, the sun wrote:
> > > On 2015/10/23 12:50:37, tommi (webrtc) wrote:
> > > > can we make stream_ const?  I.e.
> > > > webrtc::AudioSendStream* const stream_;
> > > > 
> > > > This would mean the initialization of config has to be factored out to a
> > > > separate function or a new ctor added, but I think it's worth it to make
> it
> > > > clear that using |stream_| without a thread check, is safe.
> > > > In the initializer list this could then look like this (+wrapping):
> > > > 
> > > >
> stream_(call->CreateAudioSendStream(webrtc::AudioSendStream::Config(nullptr,
> > > ch,
> > > > ssrc)),
> > > 
> > > I can foresee the need subsequently to call non-const methods on *stream_:
> > i.e.
> > > Start(), Stop(), DeliverRtcp(). So, not making it const.
> > 
> > OK, sgtm. thanks for adding the thread checks.
> 
> Acknowledged.
> 
>
https://codereview.webrtc.org/1414743004/diff/20001/talk/media/webrtc/webrtcv...
> talk/media/webrtc/webrtcvoiceengine.cc:1338: // This method is called on the
> libjingle worker thread.
> On 2015/10/26 09:54:34, tommi (webrtc) wrote:
> > On 2015/10/23 15:14:17, the sun wrote:
> > > On 2015/10/23 12:50:37, tommi (webrtc) wrote:
> > > > would be great to have a thread check for this :D
> > > 
> > > You do fancy those thread checkers! :)
> > 
> > Yes! I consider them at least as valuable as unit tests, they are so much
> > quicker to write and they are self-enforcing documentation.  I have been
> burned
> > many times :)
> 
> Acknowledged.
> 
>
https://codereview.webrtc.org/1414743004/diff/20001/talk/media/webrtc/webrtcv...
> talk/media/webrtc/webrtcvoiceengine.cc:1373: RTC_DCHECK(voe_audio_transport_);
> On 2015/10/26 09:54:34, tommi (webrtc) wrote:
> > On 2015/10/23 15:14:17, the sun wrote:
> > > On 2015/10/23 12:50:37, tommi (webrtc) wrote:
> > > > nit: this dcheck isn't necessary either so you could remove it if you
want
> > > 
> > > You're right in principle. The check should go in the c-tor so that we
> enforce
> > > the invariant of the class at time of creation.
> > > 
> > > However, right now FakeWebRtcVoiceEngine (as opposed to FakeVoiceEngine,
to
> be
> > > clear), will return a NULL audio transport. I'm adding a TODO to move the
> > assert
> > > once we no longer need that class for testing.
> > 
> > See above for DCHECKs.  You could move the TODO to the ctor.
> 
> Done.
> 
>
https://codereview.webrtc.org/1414743004/diff/20001/talk/media/webrtc/webrtcv...
> talk/media/webrtc/webrtcvoiceengine.cc:1384: void OnClose() override {
> On 2015/10/26 09:54:34, tommi (webrtc) wrote:
> > On 2015/10/23 15:14:16, the sun wrote:
> > > On 2015/10/23 12:50:37, tommi (webrtc) wrote:
> > > > Do you know on what thread this method runs?  I'm wondering since both
of
> > the
> > > > other methods that touch renderer_ are documented to run on the worker
> > thread
> > > > and if this one does too, then the lock isn't necessary (right?)
> > > 
> > > Indeed. The lock appears not useful.
> > 
> > W00t!
> >  
> > > So. Much. To. Clean.
> > 
> > Yes and it's already looking so much better :)
> 
> Acknowledged.
> 
>
https://codereview.webrtc.org/1414743004/diff/20001/talk/media/webrtc/webrtcv...
> talk/media/webrtc/webrtcvoiceengine.cc:1406: rtc::CriticalSection lock_;
> On 2015/10/26 09:54:34, tommi (webrtc) wrote:
> > Nice!
> 
> Acknowledged.
> 
>
https://codereview.webrtc.org/1414743004/diff/40001/webrtc/audio/audio_receiv...
> File webrtc/audio/audio_receive_stream_unittest.cc (right):
> 
>
https://codereview.webrtc.org/1414743004/diff/40001/webrtc/audio/audio_receiv...
> webrtc/audio/audio_receive_stream_unittest.cc:90: config.voe_channel_id =
> voice_engine.kRecvChannelId;
> On 2015/10/26 09:54:34, tommi (webrtc) wrote:
> > nit: FakeVoiceEngine::
> 
> Done.

Agree about singletons btw. Thanks for pointing that out.

kwiberg-webrtc

lgtm But consider having one thread checker per thread, because it won't work quite right ...

5 years, 1 month ago (2015-10-26 13:19:53 UTC) #12

the sun

https://codereview.webrtc.org/1414743004/diff/100001/talk/media/webrtc/webrtcvoiceengine.cc File talk/media/webrtc/webrtcvoiceengine.cc (right): https://codereview.webrtc.org/1414743004/diff/100001/talk/media/webrtc/webrtcvoiceengine.cc#newcode1324 talk/media/webrtc/webrtcvoiceengine.cc:1324: RTC_DCHECK(ch >= 0); On 2015/10/26 13:19:53, kwiberg-webrtc wrote: > ...

5 years, 1 month ago (2015-10-26 14:44:02 UTC) #13

commit-bot: I haz the power

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

5 years, 1 month ago (2015-10-27 09:48:45 UTC) #16

commit-bot: I haz the power

5 years, 1 month ago (2015-10-27 10:35:39 UTC) #18

Message was sent while issue was closed.

Patchset 10 (id:??) landed as
https://crrev.com/85a0496b8c4ac01da7c716ea7950093659864c8e
Cr-Commit-Position: refs/heads/master@{#10424}

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages