Move VoiceEngineObserver into AudioSendStream so that we detect typing noises and return properly i…

Move VoiceEngineObserver into AudioSendStream so that we detect typing noises and return properly in GetStats().

BUG=webrtc:4690
Committed: https://crrev.com/566ef247b9779f6c9d0e7ec9eea6b037f4682c53
Cr-Commit-Position: refs/heads/master@{#10548}

[the sun, 2015-10-27 15:57:54]

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

[the sun, 2015-10-27 15:57:55]

Before you waste any time on this: how strict are we about static data? 
https://codereview.webrtc.org/1403363003/diff/40001/webrtc/audio/audio_send_s...

We need to keep some per-process state. Longer term goal is to create such state
higher up (PeerConnectionFactory, ChannelManager or MediaEngine, TBD).

[the sun, 2015-10-28 15:45:02]

Here's a new try, without non-pod global data. PTAL, let me know what you think
about the general approach.

Missing:
- Extra test case in AudioSendStream (for the typing detection).
- Unit test for AudioState.

Note:
In the future, it is likely that a per-process object such as
PeerConnectionFactory or ChannelManager will maintain a map of ADM* ->
AudioState objects and push those down for Call:s using the same ADM.

Also Note:
VoiceEngine is going away, which will eliminate it from the AudioState::Config
and make the setting up simpler.

[the sun, 2015-10-28 15:57:51]

Description was changed from

==========
Move VoiceEngineObserver into AudioSendStream so that we detect typing noises
and return properly in GetStats().

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

to

==========
Move VoiceEngineObserver into AudioSendStream so that we detect typing noises
and return properly in GetStats().

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

[tommi, 2015-10-29 16:28:51]

sorry for the delay, thought I had sent this out.

https://codereview.webrtc.org/1403363003/diff/120001/webrtc/audio/audio_state.cc
File webrtc/audio/audio_state.cc (right):

https://codereview.webrtc.org/1403363003/diff/120001/webrtc/audio/audio_state...
webrtc/audio/audio_state.cc:39: return typing_noise_detected_;
I see what you mean.  Basically, the return value of this method is not
guaranteed to be correct and we don't really care, right?  I'm guessing this
might trip off ASAN though, so you might want to consider using an atomic
variable.

https://codereview.webrtc.org/1403363003/diff/120001/webrtc/audio/audio_state...
webrtc/audio/audio_state.cc:58: AudioState* AudioState::Create(const
AudioState::Config& config) {
use scoped_ptr?

https://codereview.webrtc.org/1403363003/diff/120001/webrtc/audio/audio_state.h
File webrtc/audio/audio_state.h (right):

https://codereview.webrtc.org/1403363003/diff/120001/webrtc/audio/audio_state...
webrtc/audio/audio_state.h:44: //                  this case, but reviewer may
think differently...
are we reading/writing from/to this variable from many different threads?  Would
it make sense for it to be atomic?

https://codereview.webrtc.org/1403363003/diff/120001/webrtc/audio_state.h
File webrtc/audio_state.h (right):

https://codereview.webrtc.org/1403363003/diff/120001/webrtc/audio_state.h#new...
webrtc/audio_state.h:23: // VoiceEngine used for audio streams and audio/video
synchronization.
what about ownership/lifetime? (same for voice_engine)

https://codereview.webrtc.org/1403363003/diff/120001/webrtc/call.h
File webrtc/call.h (right):

https://codereview.webrtc.org/1403363003/diff/120001/webrtc/call.h#newcode85
webrtc/call.h:85: // TODO(solenberg): Change this to a shared_ptr once we can
use C++11.
what about using scoped_refptr in the meantime?  As is it feels unsafe.  Another
alternative is linked_ptr.

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

- Implemented remaining tests.
- Using linked_ptr to hold AudioState (note that we can't cast
linked_ptr<AudioState> to linked_ptr<internal::AudioState> so Call passes down
the raw pointer to AudioSendStream/AudioReceiveStream.

https://codereview.webrtc.org/1403363003/diff/120001/webrtc/audio/audio_state.cc
File webrtc/audio/audio_state.cc (right):

https://codereview.webrtc.org/1403363003/diff/120001/webrtc/audio/audio_state...
webrtc/audio/audio_state.cc:39: return typing_noise_detected_;
On 2015/10/29 16:28:50, tommi (webrtc) wrote:
> I see what you mean.  Basically, the return value of this method is not
> guaranteed to be correct and we don't really care, right?  I'm guessing this
> might trip off ASAN though, so you might want to consider using an atomic
> variable.

Yes, we're just returning the latest state that we've seen, and it's a bool, so
we won't fall over even if we've context switched in the middle of writing the
bool. However, ASAN/TSAN is a good point. I'm adding a crit sect just for that
(unless you insist on atomics).

https://codereview.webrtc.org/1403363003/diff/120001/webrtc/audio/audio_state...
webrtc/audio/audio_state.cc:58: AudioState* AudioState::Create(const
AudioState::Config& config) {
On 2015/10/29 16:28:50, tommi (webrtc) wrote:
> use scoped_ptr?

Using linked_ptr.

https://codereview.webrtc.org/1403363003/diff/120001/webrtc/audio/audio_state.h
File webrtc/audio/audio_state.h (right):

https://codereview.webrtc.org/1403363003/diff/120001/webrtc/audio/audio_state...
webrtc/audio/audio_state.h:44: //                  this case, but reviewer may
think differently...
On 2015/10/29 16:28:50, tommi (webrtc) wrote:
> are we reading/writing from/to this variable from many different threads? 
Would
> it make sense for it to be atomic?

Done.

https://codereview.webrtc.org/1403363003/diff/120001/webrtc/audio_state.h
File webrtc/audio_state.h (right):

https://codereview.webrtc.org/1403363003/diff/120001/webrtc/audio_state.h#new...
webrtc/audio_state.h:23: // VoiceEngine used for audio streams and audio/video
synchronization.
On 2015/10/29 16:28:50, tommi (webrtc) wrote:
> what about ownership/lifetime? (same for voice_engine)

Made it clear in the comment, or did you have anything else in mind?

https://codereview.webrtc.org/1403363003/diff/120001/webrtc/call.h
File webrtc/call.h (right):

https://codereview.webrtc.org/1403363003/diff/120001/webrtc/call.h#newcode85
webrtc/call.h:85: // TODO(solenberg): Change this to a shared_ptr once we can
use C++11.
On 2015/10/29 16:28:50, tommi (webrtc) wrote:
> what about using scoped_refptr in the meantime?  As is it feels unsafe. 
Another
> alternative is linked_ptr.

Ah, nice, linked_ptr fits the bill (what I like about std::shared_ptr is that
the ref count is external to the object, with no forced inheriting from a base
class and such).

[the sun, 2015-10-30 13:59:57]

On 2015/10/30 12:19:01, the sun wrote:
> - Implemented remaining tests.
> - Using linked_ptr to hold AudioState (note that we can't cast
> linked_ptr<AudioState> to linked_ptr<internal::AudioState> so Call passes down
> the raw pointer to AudioSendStream/AudioReceiveStream.
> 
>
https://codereview.webrtc.org/1403363003/diff/120001/webrtc/audio/audio_state.cc
> File webrtc/audio/audio_state.cc (right):
> 
>
https://codereview.webrtc.org/1403363003/diff/120001/webrtc/audio/audio_state...
> webrtc/audio/audio_state.cc:39: return typing_noise_detected_;
> On 2015/10/29 16:28:50, tommi (webrtc) wrote:
> > I see what you mean.  Basically, the return value of this method is not
> > guaranteed to be correct and we don't really care, right?  I'm guessing this
> > might trip off ASAN though, so you might want to consider using an atomic
> > variable.
> 
> Yes, we're just returning the latest state that we've seen, and it's a bool,
so
> we won't fall over even if we've context switched in the middle of writing the
> bool. However, ASAN/TSAN is a good point. I'm adding a crit sect just for that
> (unless you insist on atomics).
> 
>
https://codereview.webrtc.org/1403363003/diff/120001/webrtc/audio/audio_state...
> webrtc/audio/audio_state.cc:58: AudioState* AudioState::Create(const
> AudioState::Config& config) {
> On 2015/10/29 16:28:50, tommi (webrtc) wrote:
> > use scoped_ptr?
> 
> Using linked_ptr.
> 
>
https://codereview.webrtc.org/1403363003/diff/120001/webrtc/audio/audio_state.h
> File webrtc/audio/audio_state.h (right):
> 
>
https://codereview.webrtc.org/1403363003/diff/120001/webrtc/audio/audio_state...
> webrtc/audio/audio_state.h:44: //                  this case, but reviewer may
> think differently...
> On 2015/10/29 16:28:50, tommi (webrtc) wrote:
> > are we reading/writing from/to this variable from many different threads? 
> Would
> > it make sense for it to be atomic?
> 
> Done.
> 
> https://codereview.webrtc.org/1403363003/diff/120001/webrtc/audio_state.h
> File webrtc/audio_state.h (right):
> 
>
https://codereview.webrtc.org/1403363003/diff/120001/webrtc/audio_state.h#new...
> webrtc/audio_state.h:23: // VoiceEngine used for audio streams and audio/video
> synchronization.
> On 2015/10/29 16:28:50, tommi (webrtc) wrote:
> > what about ownership/lifetime? (same for voice_engine)
> 
> Made it clear in the comment, or did you have anything else in mind?
> 
> https://codereview.webrtc.org/1403363003/diff/120001/webrtc/call.h
> File webrtc/call.h (right):
> 
> https://codereview.webrtc.org/1403363003/diff/120001/webrtc/call.h#newcode85
> webrtc/call.h:85: // TODO(solenberg): Change this to a shared_ptr once we can
> use C++11.
> On 2015/10/29 16:28:50, tommi (webrtc) wrote:
> > what about using scoped_refptr in the meantime?  As is it feels unsafe. 
> Another
> > alternative is linked_ptr.
> 
> Ah, nice, linked_ptr fits the bill (what I like about std::shared_ptr is that
> the ref count is external to the object, with no forced inheriting from a base
> class and such).

Regarding the issues with linked_ptr. I looked adding ref counting to AudioState
and using scoped_refptr and it turns out RefCountedObject isn't useful for that.
Leaving these options:

1. One-off implementation of thread safe ref counting in AudioState (inherit
from RefCountInterface).
2. Pull in RefCounted and RefCountedBase from Chromium, inherit AudioState from
RefCounted<AudioState>.
3. Use naked pointers.
4. Make linked_ptr thread safe.
5. Create an rtc::shared_ptr so we can start adapting code for C++11 stdlib.
6. Inherit internal::AudioState from RefCountedObject<webrtc::AudioState> and
implement an interim shared_ptr : public scoped_refptr<RefCountedObject<T>>,
although that will turn out pretty close to a real shared_ptr, once all
operators are implemented.

I think probably 3), and leaving TODOs makes the most sense in the short term.

[the sun, 2015-11-02 10:17:35]

On 2015/10/30 13:59:57, the sun wrote:
> On 2015/10/30 12:19:01, the sun wrote:
> > - Implemented remaining tests.
> > - Using linked_ptr to hold AudioState (note that we can't cast
> > linked_ptr<AudioState> to linked_ptr<internal::AudioState> so Call passes
down
> > the raw pointer to AudioSendStream/AudioReceiveStream.
> > 
> >
>
https://codereview.webrtc.org/1403363003/diff/120001/webrtc/audio/audio_state.cc
> > File webrtc/audio/audio_state.cc (right):
> > 
> >
>
https://codereview.webrtc.org/1403363003/diff/120001/webrtc/audio/audio_state...
> > webrtc/audio/audio_state.cc:39: return typing_noise_detected_;
> > On 2015/10/29 16:28:50, tommi (webrtc) wrote:
> > > I see what you mean.  Basically, the return value of this method is not
> > > guaranteed to be correct and we don't really care, right?  I'm guessing
this
> > > might trip off ASAN though, so you might want to consider using an atomic
> > > variable.
> > 
> > Yes, we're just returning the latest state that we've seen, and it's a bool,
> so
> > we won't fall over even if we've context switched in the middle of writing
the
> > bool. However, ASAN/TSAN is a good point. I'm adding a crit sect just for
that
> > (unless you insist on atomics).
> > 
> >
>
https://codereview.webrtc.org/1403363003/diff/120001/webrtc/audio/audio_state...
> > webrtc/audio/audio_state.cc:58: AudioState* AudioState::Create(const
> > AudioState::Config& config) {
> > On 2015/10/29 16:28:50, tommi (webrtc) wrote:
> > > use scoped_ptr?
> > 
> > Using linked_ptr.
> > 
> >
>
https://codereview.webrtc.org/1403363003/diff/120001/webrtc/audio/audio_state.h
> > File webrtc/audio/audio_state.h (right):
> > 
> >
>
https://codereview.webrtc.org/1403363003/diff/120001/webrtc/audio/audio_state...
> > webrtc/audio/audio_state.h:44: //                  this case, but reviewer
may
> > think differently...
> > On 2015/10/29 16:28:50, tommi (webrtc) wrote:
> > > are we reading/writing from/to this variable from many different threads? 
> > Would
> > > it make sense for it to be atomic?
> > 
> > Done.
> > 
> > https://codereview.webrtc.org/1403363003/diff/120001/webrtc/audio_state.h
> > File webrtc/audio_state.h (right):
> > 
> >
>
https://codereview.webrtc.org/1403363003/diff/120001/webrtc/audio_state.h#new...
> > webrtc/audio_state.h:23: // VoiceEngine used for audio streams and
audio/video
> > synchronization.
> > On 2015/10/29 16:28:50, tommi (webrtc) wrote:
> > > what about ownership/lifetime? (same for voice_engine)
> > 
> > Made it clear in the comment, or did you have anything else in mind?
> > 
> > https://codereview.webrtc.org/1403363003/diff/120001/webrtc/call.h
> > File webrtc/call.h (right):
> > 
> > https://codereview.webrtc.org/1403363003/diff/120001/webrtc/call.h#newcode85
> > webrtc/call.h:85: // TODO(solenberg): Change this to a shared_ptr once we
can
> > use C++11.
> > On 2015/10/29 16:28:50, tommi (webrtc) wrote:
> > > what about using scoped_refptr in the meantime?  As is it feels unsafe. 
> > Another
> > > alternative is linked_ptr.
> > 
> > Ah, nice, linked_ptr fits the bill (what I like about std::shared_ptr is
that
> > the ref count is external to the object, with no forced inheriting from a
base
> > class and such).
> 
> Regarding the issues with linked_ptr. I looked adding ref counting to
AudioState
> and using scoped_refptr and it turns out RefCountedObject isn't useful for
that.
> Leaving these options:
> 
> 1. One-off implementation of thread safe ref counting in AudioState (inherit
> from RefCountInterface).
> 2. Pull in RefCounted and RefCountedBase from Chromium, inherit AudioState
from
> RefCounted<AudioState>.
> 3. Use naked pointers.
> 4. Make linked_ptr thread safe.
> 5. Create an rtc::shared_ptr so we can start adapting code for C++11 stdlib.
> 6. Inherit internal::AudioState from RefCountedObject<webrtc::AudioState> and
> implement an interim shared_ptr : public scoped_refptr<RefCountedObject<T>>,
> although that will turn out pretty close to a real shared_ptr, once all
> operators are implemented.
> 
> I think probably 3), and leaving TODOs makes the most sense in the short term.

Ping...

[tommi, 2015-11-02 13:46:53]

https://codereview.webrtc.org/1403363003/diff/120001/webrtc/audio_state.h
File webrtc/audio_state.h (right):

https://codereview.webrtc.org/1403363003/diff/120001/webrtc/audio_state.h#new...
webrtc/audio_state.h:23: // VoiceEngine used for audio streams and audio/video
synchronization.
On 2015/10/30 12:19:01, the sun wrote:
> On 2015/10/29 16:28:50, tommi (webrtc) wrote:
> > what about ownership/lifetime? (same for voice_engine)
> 
> Made it clear in the comment, or did you have anything else in mind?

Thanks, comment is what I meant.

https://codereview.webrtc.org/1403363003/diff/120001/webrtc/call.h
File webrtc/call.h (right):

https://codereview.webrtc.org/1403363003/diff/120001/webrtc/call.h#newcode85
webrtc/call.h:85: // TODO(solenberg): Change this to a shared_ptr once we can
use C++11.
On 2015/10/30 12:19:01, the sun wrote:
> On 2015/10/29 16:28:50, tommi (webrtc) wrote:
> > what about using scoped_refptr in the meantime?  As is it feels unsafe. 
> Another
> > alternative is linked_ptr.
> 
> Ah, nice, linked_ptr fits the bill (what I like about std::shared_ptr is that
> the ref count is external to the object, with no forced inheriting from a base
> class and such).

Yes, I think we'll replace use of linked_ptr with shared_ptr as soon as the
latter becomes available to us.

https://codereview.webrtc.org/1403363003/diff/160001/talk/app/webrtc/mediacon...
File talk/app/webrtc/mediacontroller.cc (right):

https://codereview.webrtc.org/1403363003/diff/160001/talk/app/webrtc/mediacon...
talk/app/webrtc/mediacontroller.cc:50:
channel_manager_->media_engine()->GetAudioState()));
Since we're passing pointers between threads, we can't use linked_ptr
unfortunately :(
Going back to using raw pointers will avoid the race in linked_ptr but it feels
a bit like giving up.  As is, I don't see the lifetime guarantees (how is it
guaranteed that the object will never be deleted when accessed on a different
thread).

https://codereview.webrtc.org/1403363003/diff/160001/talk/media/base/mediaeng...
File talk/media/base/mediaengine.h (right):

https://codereview.webrtc.org/1403363003/diff/160001/talk/media/base/mediaeng...
talk/media/base/mediaengine.h:75: virtual rtc::linked_ptr<webrtc::AudioState>
GetAudioState() const = 0;
are there any threading requirements for being allowed to call this method?
(e.g. must be called on the same thread as calls Init() or something along those
lines).

https://codereview.webrtc.org/1403363003/diff/160001/talk/media/webrtc/webrtc...
File talk/media/webrtc/webrtcvoiceengine.h (right):

https://codereview.webrtc.org/1403363003/diff/160001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.h:69: webrtc::AudioState* audio_state);
if we were using linked_ptr, we'd have to pass that type in here as well.

The more I think about it, the more I'm leaning towards making AudioState an
interface that supports reference counting.  Managing its lifetime is not
simple. It's used by many threads.  There's a more-derived class that this type
is cast to.  Since it's accessed by many threads and shared between many
objects, there is a requirement that it meets some threading demands.  For
testing, having a virtual interface will allow for injecting a test
implementation.

https://codereview.webrtc.org/1403363003/diff/160001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.h:74: rtc::linked_ptr<webrtc::AudioState>
GetAudioState() const {
It would be nice to have a thread check here.
or can audio_state_ never change value throughout the lifetime of the
WebRtcVoiceEngine?

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

https://codereview.webrtc.org/1403363003/diff/160001/webrtc/audio/audio_recei...
webrtc/audio/audio_receive_stream.h:30: AudioState* audio_state);
this would also bypass the linked_ptr... but I think we're backing out of that
now.

[the sun, 2015-11-03 12:41:08]

https://codereview.webrtc.org/1403363003/diff/120001/webrtc/audio_state.h
File webrtc/audio_state.h (right):

https://codereview.webrtc.org/1403363003/diff/120001/webrtc/audio_state.h#new...
webrtc/audio_state.h:23: // VoiceEngine used for audio streams and audio/video
synchronization.
On 2015/11/02 13:46:53, tommi (webrtc) wrote:
> On 2015/10/30 12:19:01, the sun wrote:
> > On 2015/10/29 16:28:50, tommi (webrtc) wrote:
> > > what about ownership/lifetime? (same for voice_engine)
> > 
> > Made it clear in the comment, or did you have anything else in mind?
> 
> Thanks, comment is what I meant.

Acknowledged.

https://codereview.webrtc.org/1403363003/diff/120001/webrtc/call.h
File webrtc/call.h (right):

https://codereview.webrtc.org/1403363003/diff/120001/webrtc/call.h#newcode85
webrtc/call.h:85: // TODO(solenberg): Change this to a shared_ptr once we can
use C++11.
On 2015/11/02 13:46:53, tommi (webrtc) wrote:
> On 2015/10/30 12:19:01, the sun wrote:
> > On 2015/10/29 16:28:50, tommi (webrtc) wrote:
> > > what about using scoped_refptr in the meantime?  As is it feels unsafe. 
> > Another
> > > alternative is linked_ptr.
> > 
> > Ah, nice, linked_ptr fits the bill (what I like about std::shared_ptr is
that
> > the ref count is external to the object, with no forced inheriting from a
base
> > class and such).
> 
> Yes, I think we'll replace use of linked_ptr with shared_ptr as soon as the
> latter becomes available to us.

Acknowledged.

https://codereview.webrtc.org/1403363003/diff/160001/talk/app/webrtc/mediacon...
File talk/app/webrtc/mediacontroller.cc (right):

https://codereview.webrtc.org/1403363003/diff/160001/talk/app/webrtc/mediacon...
talk/app/webrtc/mediacontroller.cc:50:
channel_manager_->media_engine()->GetAudioState()));
On 2015/11/02 13:46:53, tommi (webrtc) wrote:
> Since we're passing pointers between threads, we can't use linked_ptr
> unfortunately :(
> Going back to using raw pointers will avoid the race in linked_ptr but it
feels
> a bit like giving up.  As is, I don't see the lifetime guarantees (how is it
> guaranteed that the object will never be deleted when accessed on a different
> thread).

Well, given how things were being configured, the AudioState and VoiceEngine
were guaranteed to outlive all Call objects (and hence streams too), but I agree
we're better off with some clothes on; using scoped_refptr.

https://codereview.webrtc.org/1403363003/diff/160001/talk/media/base/mediaeng...
File talk/media/base/mediaengine.h (right):

https://codereview.webrtc.org/1403363003/diff/160001/talk/media/base/mediaeng...
talk/media/base/mediaengine.h:75: virtual rtc::linked_ptr<webrtc::AudioState>
GetAudioState() const = 0;
On 2015/11/02 13:46:53, tommi (webrtc) wrote:
> are there any threading requirements for being allowed to call this method?
> (e.g. must be called on the same thread as calls Init() or something along
those
> lines).

No, the returned smart pointer should be thread safe.

https://codereview.webrtc.org/1403363003/diff/160001/talk/media/webrtc/webrtc...
File talk/media/webrtc/webrtcvoiceengine.h (right):

https://codereview.webrtc.org/1403363003/diff/160001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.h:69: webrtc::AudioState* audio_state);
On 2015/11/02 13:46:53, tommi (webrtc) wrote:
> if we were using linked_ptr, we'd have to pass that type in here as well.
> 
> The more I think about it, the more I'm leaning towards making AudioState an
> interface that supports reference counting.  Managing its lifetime is not
> simple. It's used by many threads.  There's a more-derived class that this
type
> is cast to.  Since it's accessed by many threads and shared between many
> objects, there is a requirement that it meets some threading demands.  For
> testing, having a virtual interface will allow for injecting a test
> implementation.

Indeed. Turns out we don't need the AudioState for testing here though.

https://codereview.webrtc.org/1403363003/diff/160001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.h:74: rtc::linked_ptr<webrtc::AudioState>
GetAudioState() const {
On 2015/11/02 13:46:53, tommi (webrtc) wrote:
> It would be nice to have a thread check here.
> or can audio_state_ never change value throughout the lifetime of the
> WebRtcVoiceEngine?

No, it has the same life span as WVoE and VoE. Nevertheless, this class was
missing thread checkers, so now I've added them.

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

https://codereview.webrtc.org/1403363003/diff/160001/webrtc/audio/audio_recei...
webrtc/audio/audio_receive_stream.h:30: AudioState* audio_state);
On 2015/11/02 13:46:53, tommi (webrtc) wrote:
> this would also bypass the linked_ptr... but I think we're backing out of that
> now.

Acknowledged.

[the sun, 2015-11-05 16:11:35]

On 2015/11/03 12:41:08, the sun wrote:
> https://codereview.webrtc.org/1403363003/diff/120001/webrtc/audio_state.h
> File webrtc/audio_state.h (right):
> 
>
https://codereview.webrtc.org/1403363003/diff/120001/webrtc/audio_state.h#new...
> webrtc/audio_state.h:23: // VoiceEngine used for audio streams and audio/video
> synchronization.
> On 2015/11/02 13:46:53, tommi (webrtc) wrote:
> > On 2015/10/30 12:19:01, the sun wrote:
> > > On 2015/10/29 16:28:50, tommi (webrtc) wrote:
> > > > what about ownership/lifetime? (same for voice_engine)
> > > 
> > > Made it clear in the comment, or did you have anything else in mind?
> > 
> > Thanks, comment is what I meant.
> 
> Acknowledged.
> 
> https://codereview.webrtc.org/1403363003/diff/120001/webrtc/call.h
> File webrtc/call.h (right):
> 
> https://codereview.webrtc.org/1403363003/diff/120001/webrtc/call.h#newcode85
> webrtc/call.h:85: // TODO(solenberg): Change this to a shared_ptr once we can
> use C++11.
> On 2015/11/02 13:46:53, tommi (webrtc) wrote:
> > On 2015/10/30 12:19:01, the sun wrote:
> > > On 2015/10/29 16:28:50, tommi (webrtc) wrote:
> > > > what about using scoped_refptr in the meantime?  As is it feels unsafe. 
> > > Another
> > > > alternative is linked_ptr.
> > > 
> > > Ah, nice, linked_ptr fits the bill (what I like about std::shared_ptr is
> that
> > > the ref count is external to the object, with no forced inheriting from a
> base
> > > class and such).
> > 
> > Yes, I think we'll replace use of linked_ptr with shared_ptr as soon as the
> > latter becomes available to us.
> 
> Acknowledged.
> 
>
https://codereview.webrtc.org/1403363003/diff/160001/talk/app/webrtc/mediacon...
> File talk/app/webrtc/mediacontroller.cc (right):
> 
>
https://codereview.webrtc.org/1403363003/diff/160001/talk/app/webrtc/mediacon...
> talk/app/webrtc/mediacontroller.cc:50:
> channel_manager_->media_engine()->GetAudioState()));
> On 2015/11/02 13:46:53, tommi (webrtc) wrote:
> > Since we're passing pointers between threads, we can't use linked_ptr
> > unfortunately :(
> > Going back to using raw pointers will avoid the race in linked_ptr but it
> feels
> > a bit like giving up.  As is, I don't see the lifetime guarantees (how is it
> > guaranteed that the object will never be deleted when accessed on a
different
> > thread).
> 
> Well, given how things were being configured, the AudioState and VoiceEngine
> were guaranteed to outlive all Call objects (and hence streams too), but I
agree
> we're better off with some clothes on; using scoped_refptr.
> 
>
https://codereview.webrtc.org/1403363003/diff/160001/talk/media/base/mediaeng...
> File talk/media/base/mediaengine.h (right):
> 
>
https://codereview.webrtc.org/1403363003/diff/160001/talk/media/base/mediaeng...
> talk/media/base/mediaengine.h:75: virtual rtc::linked_ptr<webrtc::AudioState>
> GetAudioState() const = 0;
> On 2015/11/02 13:46:53, tommi (webrtc) wrote:
> > are there any threading requirements for being allowed to call this method?
> > (e.g. must be called on the same thread as calls Init() or something along
> those
> > lines).
> 
> No, the returned smart pointer should be thread safe.
> 
>
https://codereview.webrtc.org/1403363003/diff/160001/talk/media/webrtc/webrtc...
> File talk/media/webrtc/webrtcvoiceengine.h (right):
> 
>
https://codereview.webrtc.org/1403363003/diff/160001/talk/media/webrtc/webrtc...
> talk/media/webrtc/webrtcvoiceengine.h:69: webrtc::AudioState* audio_state);
> On 2015/11/02 13:46:53, tommi (webrtc) wrote:
> > if we were using linked_ptr, we'd have to pass that type in here as well.
> > 
> > The more I think about it, the more I'm leaning towards making AudioState an
> > interface that supports reference counting.  Managing its lifetime is not
> > simple. It's used by many threads.  There's a more-derived class that this
> type
> > is cast to.  Since it's accessed by many threads and shared between many
> > objects, there is a requirement that it meets some threading demands.  For
> > testing, having a virtual interface will allow for injecting a test
> > implementation.
> 
> Indeed. Turns out we don't need the AudioState for testing here though.
> 
>
https://codereview.webrtc.org/1403363003/diff/160001/talk/media/webrtc/webrtc...
> talk/media/webrtc/webrtcvoiceengine.h:74: rtc::linked_ptr<webrtc::AudioState>
> GetAudioState() const {
> On 2015/11/02 13:46:53, tommi (webrtc) wrote:
> > It would be nice to have a thread check here.
> > or can audio_state_ never change value throughout the lifetime of the
> > WebRtcVoiceEngine?
> 
> No, it has the same life span as WVoE and VoE. Nevertheless, this class was
> missing thread checkers, so now I've added them.
> 
>
https://codereview.webrtc.org/1403363003/diff/160001/webrtc/audio/audio_recei...
> File webrtc/audio/audio_receive_stream.h (right):
> 
>
https://codereview.webrtc.org/1403363003/diff/160001/webrtc/audio/audio_recei...
> webrtc/audio/audio_receive_stream.h:30: AudioState* audio_state);
> On 2015/11/02 13:46:53, tommi (webrtc) wrote:
> > this would also bypass the linked_ptr... but I think we're backing out of
that
> > now.
> 
> Acknowledged.

PTAL.

[tommi, 2015-11-06 23:31:18]

The CQ bit was checked by tommi@webrtc.org

[tommi, 2015-11-06 23:31:19]

lgtm

[commit-bot: I haz the power, 2015-11-06 23:31:36]

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

[commit-bot: I haz the power, 2015-11-06 23:34:53]

Committed patchset #30 (id:580001)

[commit-bot: I haz the power, 2015-11-06 23:35:07]

Patchset 30 (id:??) landed as
https://crrev.com/566ef247b9779f6c9d0e7ec9eea6b037f4682c53
Cr-Commit-Position: refs/heads/master@{#10548}