Added locking when getting echo likelihood stats.

Added locking when getting echo likelihood stats.

Currently no lock is taken when returning echo likelihood stats, which causes a race condition between the thread getting the stats and the thread running the echo detector. This CL resolves the issue by adding locking.

BUG=webrtc:7346
Review-Url: https://codereview.webrtc.org/2749973003
Cr-Commit-Position: refs/heads/master@{#17270}
Committed: https://chromium.googlesource.com/external/webrtc/+/9c192b2b06cb012b3f515923aeb0a3a0f9a09025

[ivoc, 2017-03-15 15:02:27]

Description was changed from

==========
Added locking when getting echo likelihood stats.

Since currently the capture lock is not taken when returning echo likelihood
stats, there is a race condition between the thread getting the stats and the
thread running the echo detector. This CL resolves that issue.

BUG=webrtc:7346
==========

to

==========
Added locking when getting echo likelihood stats.

Currently no lock is taken when returning echo likelihood stats, which causes a
race condition between the thread getting the stats and the thread running the
echo detector. This CL resolves the issue by adding locking.

BUG=webrtc:7346
==========

[ivoc, 2017-03-15 15:02:27]

ivoc@webrtc.org changed reviewers:
+ henrik.lundin@webrtc.org

[ivoc, 2017-03-15 15:02:41]

Hi Henrik, PTAL.

[tommi, 2017-03-15 16:14:45]

drive-by

https://codereview.webrtc.org/2749973003/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/audio_processing_impl.cc (right):

https://codereview.webrtc.org/2749973003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/audio_processing_impl.cc:327: rtc::CritScope
cs_render(&crit_render_);
... any idea why we're grabbing these locks in the constructor?

Maybe the public/private modules could have a constructor that accepts all these
pointers, RAII style?

https://codereview.webrtc.org/2749973003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/audio_processing_impl.cc:1622:
private_submodules_->residual_echo_detector->echo_likelihood();
could we instead make sure that either these stats get updated on the thread
where GetStatistics is called on or that stats from |private_submodules_| are
queried from the thread where those stats are updated on?

As is, this feels like AudioProcessingImpl is applying synchronization outside
of its responsibilities. Knowing about implementation details of several classes
who's pointer is stored in another struct.

[ivoc, 2017-03-15 17:14:15]

https://codereview.webrtc.org/2749973003/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/audio_processing_impl.cc (right):

https://codereview.webrtc.org/2749973003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/audio_processing_impl.cc:327: rtc::CritScope
cs_render(&crit_render_);
On 2017/03/15 16:14:45, tommi (webrtc) wrote:
> ... any idea why we're grabbing these locks in the constructor?
> 
> Maybe the public/private modules could have a constructor that accepts all
these
> pointers, RAII style?

Good question. I didn't write this code, so I'm not completely sure, but it
might be to make sure that no external threads are calling these submodules
while they're being constructed (the public submodules are exposed to other
threads). Although I guess that might still happen before this lock is taken (so
this would be a race).

https://codereview.webrtc.org/2749973003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/audio_processing_impl.cc:1622:
private_submodules_->residual_echo_detector->echo_likelihood();
On 2017/03/15 16:14:45, tommi (webrtc) wrote:
> could we instead make sure that either these stats get updated on the thread
> where GetStatistics is called on or that stats from |private_submodules_| are
> queried from the thread where those stats are updated on?
> 
> As is, this feels like AudioProcessingImpl is applying synchronization outside
> of its responsibilities. Knowing about implementation details of several
classes
> who's pointer is stored in another struct.

This function is called from a Chromium thread (I think the render thread), so I
don't think it's possible to do the audio processing on the same thread as
getting the stats, some sort of synchronization is inevitable.

I agree that the locking in APM and its submodules is more complicated than it
needs to be. One of the main reasons is that the internal submodules are exposed
and used from various different threads (some of which are Chromium threads). I
think it makes sense to refactor APM so that all submodules are private, and
only APM is exposed externally. That way we can solve the locking/threading
issues in this class, and make the submodules lock free.

[tommi, 2017-03-15 17:27:59]

Thanks Ivo - some more info below that I hope is helpful.

https://codereview.webrtc.org/2749973003/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/audio_processing_impl.cc (right):

https://codereview.webrtc.org/2749973003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/audio_processing_impl.cc:327: rtc::CritScope
cs_render(&crit_render_);
On 2017/03/15 17:14:15, ivoc wrote:
> On 2017/03/15 16:14:45, tommi (webrtc) wrote:
> > ... any idea why we're grabbing these locks in the constructor?
> > 
> > Maybe the public/private modules could have a constructor that accepts all
> these
> > pointers, RAII style?
> 
> Good question. I didn't write this code, so I'm not completely sure, but it
> might be to make sure that no external threads are calling these submodules
> while they're being constructed (the public submodules are exposed to other
> threads). Although I guess that might still happen before this lock is taken
(so
> this would be a race).

OK, thanks. I didn't mean to derail the review :)  Constructing an object should
be an operation during which the object should not have to worry about its state
being poked at from other threads.  So if these locks are actually protecting
against something like that, then we might have a bigger problem. 
If you do happen to find out something, then it would be nice to add a comment
here :)

https://codereview.webrtc.org/2749973003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/audio_processing_impl.cc:1622:
private_submodules_->residual_echo_detector->echo_likelihood();
On 2017/03/15 17:14:15, ivoc wrote:
> On 2017/03/15 16:14:45, tommi (webrtc) wrote:
> > could we instead make sure that either these stats get updated on the thread
> > where GetStatistics is called on or that stats from |private_submodules_|
are
> > queried from the thread where those stats are updated on?
> > 
> > As is, this feels like AudioProcessingImpl is applying synchronization
outside
> > of its responsibilities. Knowing about implementation details of several
> classes
> > who's pointer is stored in another struct.
> 
> This function is called from a Chromium thread (I think the render thread), so
I
> don't think it's possible to do the audio processing on the same thread as
> getting the stats, some sort of synchronization is inevitable.
> 
> I agree that the locking in APM and its submodules is more complicated than it
> needs to be. One of the main reasons is that the internal submodules are
exposed
> and used from various different threads (some of which are Chromium threads).
I
> think it makes sense to refactor APM so that all submodules are private, and
> only APM is exposed externally. That way we can solve the locking/threading
> issues in this class, and make the submodules lock free. 

From what I can tell, it's actually called from our signaling thread:

https://cs.chromium.org/chromium/src/third_party/webrtc/pc/statscollector.cc?...

It would be nice if we could start adding some thread checks as we fix thread
issues, that both document the code and help us catching regressions.  As we do
that, the bigger picture becomes a little clearer bit by bit and when we have
those things laid out, it becomes easy to remove locks where we don't need them
and at the same time document the code.

To make what I mean a little clearer, here are a couple of examples. Look for
"TODO(tommi)" in these diffs:
https://codereview.webrtc.org/2746263003/diff/140001/webrtc/modules/video_cod...
https://codereview.webrtc.org/2746263003/diff/140001/webrtc/modules/video_cod...

[ivoc, 2017-03-15 19:30:44]

https://codereview.webrtc.org/2749973003/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/audio_processing_impl.cc (right):

https://codereview.webrtc.org/2749973003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/audio_processing_impl.cc:327: rtc::CritScope
cs_render(&crit_render_);
On 2017/03/15 17:27:59, tommi (webrtc) wrote:
> On 2017/03/15 17:14:15, ivoc wrote:
> > On 2017/03/15 16:14:45, tommi (webrtc) wrote:
> > > ... any idea why we're grabbing these locks in the constructor?
> > > 
> > > Maybe the public/private modules could have a constructor that accepts all
> > these
> > > pointers, RAII style?
> > 
> > Good question. I didn't write this code, so I'm not completely sure, but it
> > might be to make sure that no external threads are calling these submodules
> > while they're being constructed (the public submodules are exposed to other
> > threads). Although I guess that might still happen before this lock is taken
> (so
> > this would be a race).
> 
> OK, thanks. I didn't mean to derail the review :)  Constructing an object
should
> be an operation during which the object should not have to worry about its
state
> being poked at from other threads.  So if these locks are actually protecting
> against something like that, then we might have a bigger problem. 
> If you do happen to find out something, then it would be nice to add a comment
> here :)

I agree that this looks fishy. I'm sure that when we refactor the APM locking
this can be solved.

https://codereview.webrtc.org/2749973003/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/audio_processing_impl.cc:1622:
private_submodules_->residual_echo_detector->echo_likelihood();
On 2017/03/15 17:27:59, tommi (webrtc) wrote:
> On 2017/03/15 17:14:15, ivoc wrote:
> > On 2017/03/15 16:14:45, tommi (webrtc) wrote:
> > > could we instead make sure that either these stats get updated on the
thread
> > > where GetStatistics is called on or that stats from |private_submodules_|
> are
> > > queried from the thread where those stats are updated on?
> > > 
> > > As is, this feels like AudioProcessingImpl is applying synchronization
> outside
> > > of its responsibilities. Knowing about implementation details of several
> > classes
> > > who's pointer is stored in another struct.
> > 
> > This function is called from a Chromium thread (I think the render thread),
so
> I
> > don't think it's possible to do the audio processing on the same thread as
> > getting the stats, some sort of synchronization is inevitable.
> > 
> > I agree that the locking in APM and its submodules is more complicated than
it
> > needs to be. One of the main reasons is that the internal submodules are
> exposed
> > and used from various different threads (some of which are Chromium
threads).
> I
> > think it makes sense to refactor APM so that all submodules are private, and
> > only APM is exposed externally. That way we can solve the locking/threading
> > issues in this class, and make the submodules lock free. 
> 
> From what I can tell, it's actually called from our signaling thread:
> 
>
https://cs.chromium.org/chromium/src/third_party/webrtc/pc/statscollector.cc?...
> 
> It would be nice if we could start adding some thread checks as we fix thread
> issues, that both document the code and help us catching regressions.  As we
do
> that, the bigger picture becomes a little clearer bit by bit and when we have
> those things laid out, it becomes easy to remove locks where we don't need
them
> and at the same time document the code.
> 
> To make what I mean a little clearer, here are a couple of examples. Look for
> "TODO(tommi)" in these diffs:
>
https://codereview.webrtc.org/2746263003/diff/140001/webrtc/modules/video_cod...
>
https://codereview.webrtc.org/2746263003/diff/140001/webrtc/modules/video_cod...

Yes, that's one place this is called from. Another is here:
https://cs.chromium.org/chromium/src/content/renderer/media/media_stream_audi...
Thanks for the examples, those will be useful when we refactor the locking in
APM.

[tommi, 2017-03-15 20:31:00]

On 2017/03/15 19:30:44, ivoc wrote:
>
https://codereview.webrtc.org/2749973003/diff/1/webrtc/modules/audio_processi...
> File webrtc/modules/audio_processing/audio_processing_impl.cc (right):
> 
>
https://codereview.webrtc.org/2749973003/diff/1/webrtc/modules/audio_processi...
> webrtc/modules/audio_processing/audio_processing_impl.cc:327: rtc::CritScope
> cs_render(&crit_render_);
> On 2017/03/15 17:27:59, tommi (webrtc) wrote:
> > On 2017/03/15 17:14:15, ivoc wrote:
> > > On 2017/03/15 16:14:45, tommi (webrtc) wrote:
> > > > ... any idea why we're grabbing these locks in the constructor?
> > > > 
> > > > Maybe the public/private modules could have a constructor that accepts
all
> > > these
> > > > pointers, RAII style?
> > > 
> > > Good question. I didn't write this code, so I'm not completely sure, but
it
> > > might be to make sure that no external threads are calling these
submodules
> > > while they're being constructed (the public submodules are exposed to
other
> > > threads). Although I guess that might still happen before this lock is
taken
> > (so
> > > this would be a race).
> > 
> > OK, thanks. I didn't mean to derail the review :)  Constructing an object
> should
> > be an operation during which the object should not have to worry about its
> state
> > being poked at from other threads.  So if these locks are actually
protecting
> > against something like that, then we might have a bigger problem. 
> > If you do happen to find out something, then it would be nice to add a
comment
> > here :)
> 
> I agree that this looks fishy. I'm sure that when we refactor the APM locking
> this can be solved.
> 
>
https://codereview.webrtc.org/2749973003/diff/1/webrtc/modules/audio_processi...
> webrtc/modules/audio_processing/audio_processing_impl.cc:1622:
> private_submodules_->residual_echo_detector->echo_likelihood();
> On 2017/03/15 17:27:59, tommi (webrtc) wrote:
> > On 2017/03/15 17:14:15, ivoc wrote:
> > > On 2017/03/15 16:14:45, tommi (webrtc) wrote:
> > > > could we instead make sure that either these stats get updated on the
> thread
> > > > where GetStatistics is called on or that stats from
|private_submodules_|
> > are
> > > > queried from the thread where those stats are updated on?
> > > > 
> > > > As is, this feels like AudioProcessingImpl is applying synchronization
> > outside
> > > > of its responsibilities. Knowing about implementation details of several
> > > classes
> > > > who's pointer is stored in another struct.
> > > 
> > > This function is called from a Chromium thread (I think the render
thread),
> so
> > I
> > > don't think it's possible to do the audio processing on the same thread as
> > > getting the stats, some sort of synchronization is inevitable.
> > > 
> > > I agree that the locking in APM and its submodules is more complicated
than
> it
> > > needs to be. One of the main reasons is that the internal submodules are
> > exposed
> > > and used from various different threads (some of which are Chromium
> threads).
> > I
> > > think it makes sense to refactor APM so that all submodules are private,
and
> > > only APM is exposed externally. That way we can solve the
locking/threading
> > > issues in this class, and make the submodules lock free. 
> > 
> > From what I can tell, it's actually called from our signaling thread:
> > 
> >
>
https://cs.chromium.org/chromium/src/third_party/webrtc/pc/statscollector.cc?...
> > 
> > It would be nice if we could start adding some thread checks as we fix
thread
> > issues, that both document the code and help us catching regressions.  As we
> do
> > that, the bigger picture becomes a little clearer bit by bit and when we
have
> > those things laid out, it becomes easy to remove locks where we don't need
> them
> > and at the same time document the code.
> > 
> > To make what I mean a little clearer, here are a couple of examples. Look
for
> > "TODO(tommi)" in these diffs:
> >
>
https://codereview.webrtc.org/2746263003/diff/140001/webrtc/modules/video_cod...
> >
>
https://codereview.webrtc.org/2746263003/diff/140001/webrtc/modules/video_cod...
> 
> Yes, that's one place this is called from. Another is here:
>
https://cs.chromium.org/chromium/src/content/renderer/media/media_stream_audi...
> Thanks for the examples, those will be useful when we refactor the locking in
> APM.

I looked at how we get to that line and it looks like it comes from GetStats(),
which according to the comment is still the libjingle thread.
https://cs.chromium.org/chromium/src/content/renderer/media/media_stream_audi...
However, doesn't matter here, I just thought I'd mention it as it might be good
to know down the line that webrtc is in control of the thread.

[hlundin-webrtc, 2017-03-16 08:32:14]

lgtm

[ivoc, 2017-03-16 10:49:20]

The CQ bit was checked by ivoc@webrtc.org

[commit-bot: I haz the power, 2017-03-16 10:49:24]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-03-16 11:22:15]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1489661360133730, "parent_rev":
"f13b54867f10285262e8a685a6f39c6be8269c03", "commit_rev":
"9c192b2b06cb012b3f515923aeb0a3a0f9a09025"}

[commit-bot: I haz the power, 2017-03-16 11:22:18]

Description was changed from

==========
Added locking when getting echo likelihood stats.

Currently no lock is taken when returning echo likelihood stats, which causes a
race condition between the thread getting the stats and the thread running the
echo detector. This CL resolves the issue by adding locking.

BUG=webrtc:7346
==========

to

==========
Added locking when getting echo likelihood stats.

Currently no lock is taken when returning echo likelihood stats, which causes a
race condition between the thread getting the stats and the thread running the
echo detector. This CL resolves the issue by adding locking.

BUG=webrtc:7346

Review-Url: https://codereview.webrtc.org/2749973003
Cr-Commit-Position: refs/heads/master@{#17270}
Committed:
https://chromium.googlesource.com/external/webrtc/+/9c192b2b06cb012b3f515923a...
==========

[commit-bot: I haz the power, 2017-03-16 11:22:19]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/external/webrtc/+/9c192b2b06cb012b3f515923a...