Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(1664)

Issue 9692038: stopping the audio thread before destroying the AudioManager<Platform> (Closed)

Created:
8 years, 9 months ago by no longer working on chromium
Modified:
8 years, 9 months ago
CC:
chromium-reviews, feature-media-reviews_chromium.org
Visibility:
Public.

Description

Stopping the audio thread before destroying the AudioManager<Platform>. The destruction of the AudioManager family happens in order of: AudioManager<Platform>, AudioManagerBase, AudioManager. So before getting into the destruction of AudioManagerBase, we have make sure the audio thread has been stopped before AudioManager<Platform> is gone, otherwise it will end up into unexpected behavior, for example, crash because of pure virtual function. BUG=117470 TEST=media_unittests, Address Sanitizer Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=126635

Patch Set 1 #

Total comments: 11

Patch Set 2 : used the correct way to delete the fake streams & addressed Tommi's comment #

Total comments: 8

Patch Set 3 : addressed Tommi's comment & added back the static GetCurrentFakeStream function for content unittest #

Patch Set 4 : fix Android compiling problem #

Total comments: 8

Patch Set 5 : #

Patch Set 6 : rebased and fixed the speech recognition unittest #

Total comments: 3
Unified diffs Side-by-side diffs Delta from patch set Stats (+123 lines, -86 lines) Patch
M content/browser/renderer_host/media/audio_renderer_host_unittest.cc View 1 2 1 chunk +2 lines, -2 lines 0 comments Download
M content/browser/speech/speech_recognizer_impl_unittest.cc View 1 2 3 4 5 3 chunks +15 lines, -3 lines 3 comments Download
M media/audio/android/audio_manager_android.cc View 1 2 3 2 chunks +3 lines, -3 lines 0 comments Download
M media/audio/audio_manager_base.h View 1 2 3 chunks +12 lines, -5 lines 0 comments Download
M media/audio/audio_manager_base.cc View 1 5 chunks +36 lines, -9 lines 0 comments Download
M media/audio/fake_audio_input_stream.h View 1 2 3 4 5 2 chunks +8 lines, -7 lines 0 comments Download
M media/audio/fake_audio_input_stream.cc View 1 4 chunks +10 lines, -10 lines 0 comments Download
M media/audio/fake_audio_output_stream.h View 1 2 2 chunks +11 lines, -6 lines 0 comments Download
M media/audio/fake_audio_output_stream.cc View 1 2 3 4 3 chunks +20 lines, -29 lines 0 comments Download
M media/audio/mac/audio_manager_mac.cc View 1 chunk +1 line, -0 lines 0 comments Download
M media/audio/openbsd/audio_manager_openbsd.cc View 1 chunk +1 line, -9 lines 0 comments Download
M media/audio/simple_sources_unittest.cc View 1 2 2 chunks +3 lines, -3 lines 0 comments Download
M media/audio/win/audio_manager_win.cc View 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 20 (0 generated)
no longer working on chromium
Tommi, could you please review. Thanks, BR, /SX
8 years, 9 months ago (2012-03-13 10:20:11 UTC) #1
tommi (sloooow) - chröme
https://chromiumcodereview.appspot.com/9692038/diff/1/media/audio/audio_manager_base.cc File media/audio/audio_manager_base.cc (right): https://chromiumcodereview.appspot.com/9692038/diff/1/media/audio/audio_manager_base.cc#newcode90 media/audio/audio_manager_base.cc:90: ++num_output_streams_; I think I made a comment on this ...
8 years, 9 months ago (2012-03-13 10:50:28 UTC) #2
tommi (sloooow) - chröme
https://chromiumcodereview.appspot.com/9692038/diff/1/media/audio/audio_manager_base.cc File media/audio/audio_manager_base.cc (right): https://chromiumcodereview.appspot.com/9692038/diff/1/media/audio/audio_manager_base.cc#newcode19 media/audio/audio_manager_base.cc:19: static const int kDefaultMaxOutputStreams = 15; 16? https://chromiumcodereview.appspot.com/9692038/diff/1/media/audio/audio_manager_base.cc#newcode23 media/audio/audio_manager_base.cc:23: ...
8 years, 9 months ago (2012-03-13 11:06:48 UTC) #3
no longer working on chromium
Changed the code based on the comments from Tommi. I tried to avoid massive refactoring ...
8 years, 9 months ago (2012-03-13 14:08:59 UTC) #4
tommi (sloooow) - chröme
https://chromiumcodereview.appspot.com/9692038/diff/5002/media/audio/audio_manager_base.h File media/audio/audio_manager_base.h (right): https://chromiumcodereview.appspot.com/9692038/diff/5002/media/audio/audio_manager_base.h#newcode111 media/audio/audio_manager_base.h:111: // Max number of opened output streams, modified by ...
8 years, 9 months ago (2012-03-13 14:16:51 UTC) #5
no longer working on chromium
https://chromiumcodereview.appspot.com/9692038/diff/5002/media/audio/audio_manager_base.h File media/audio/audio_manager_base.h (right): https://chromiumcodereview.appspot.com/9692038/diff/5002/media/audio/audio_manager_base.h#newcode111 media/audio/audio_manager_base.h:111: // Max number of opened output streams, modified by ...
8 years, 9 months ago (2012-03-13 15:03:38 UTC) #6
no longer working on chromium
Wei, Could I have your owner stamp for a tiny change on the audio_renderer_host_unittest? Thanks, ...
8 years, 9 months ago (2012-03-13 15:05:52 UTC) #7
wjia(left Chromium)
lgtm for audio_renderer_host_unittest.cc
8 years, 9 months ago (2012-03-13 15:34:13 UTC) #8
tommi (sloooow) - chröme
https://chromiumcodereview.appspot.com/9692038/diff/35/media/audio/fake_audio_input_stream.h File media/audio/fake_audio_input_stream.h (right): https://chromiumcodereview.appspot.com/9692038/diff/35/media/audio/fake_audio_input_stream.h#newcode26 media/audio/fake_audio_input_stream.h:26: virtual ~FakeAudioInputStream(); hmm... you replied to my comment about ...
8 years, 9 months ago (2012-03-13 16:20:09 UTC) #9
no longer working on chromium
https://chromiumcodereview.appspot.com/9692038/diff/35/media/audio/fake_audio_input_stream.h File media/audio/fake_audio_input_stream.h (right): https://chromiumcodereview.appspot.com/9692038/diff/35/media/audio/fake_audio_input_stream.h#newcode26 media/audio/fake_audio_input_stream.h:26: virtual ~FakeAudioInputStream(); On 2012/03/13 16:20:09, tommi wrote: > hmm... ...
8 years, 9 months ago (2012-03-13 16:38:08 UTC) #10
tommi (sloooow) - chröme
lgtm
8 years, 9 months ago (2012-03-13 17:41:02 UTC) #11
no longer working on chromium
Satish, Can you take a quick look at the speech recognition unittest? We have to ...
8 years, 9 months ago (2012-03-14 10:20:33 UTC) #12
Satish
lgtm for speech_recognizer_impl_unittest.cc +primiano as he is refactoring the speech code now
8 years, 9 months ago (2012-03-14 11:18:59 UTC) #13
Primiano Tucci (use gerrit)
Just a minor request on our unit test. Thanks, Primiano. https://chromiumcodereview.appspot.com/9692038/diff/11001/content/browser/speech/speech_recognizer_impl_unittest.cc File content/browser/speech/speech_recognizer_impl_unittest.cc (right): https://chromiumcodereview.appspot.com/9692038/diff/11001/content/browser/speech/speech_recognizer_impl_unittest.cc#newcode54 ...
8 years, 9 months ago (2012-03-14 11:35:15 UTC) #14
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xians@chromium.org/9692038/11001
8 years, 9 months ago (2012-03-14 11:36:38 UTC) #15
no longer working on chromium
https://chromiumcodereview.appspot.com/9692038/diff/11001/content/browser/speech/speech_recognizer_impl_unittest.cc File content/browser/speech/speech_recognizer_impl_unittest.cc (right): https://chromiumcodereview.appspot.com/9692038/diff/11001/content/browser/speech/speech_recognizer_impl_unittest.cc#newcode54 content/browser/speech/speech_recognizer_impl_unittest.cc:54: const AudioParameters& params) OVERRIDE { return NULL; } On ...
8 years, 9 months ago (2012-03-14 11:40:11 UTC) #16
Primiano Tucci (use gerrit)
Ok, it's fine don't worry. No need to start a new CL. I am refactoring ...
8 years, 9 months ago (2012-03-14 11:46:40 UTC) #17
commit-bot: I haz the power
Change committed as 126635
8 years, 9 months ago (2012-03-14 14:07:41 UTC) #18
scherkus (not reviewing)
https://chromiumcodereview.appspot.com/9692038/diff/11001/content/browser/speech/speech_recognizer_impl_unittest.cc File content/browser/speech/speech_recognizer_impl_unittest.cc (right): https://chromiumcodereview.appspot.com/9692038/diff/11001/content/browser/speech/speech_recognizer_impl_unittest.cc#newcode54 content/browser/speech/speech_recognizer_impl_unittest.cc:54: const AudioParameters& params) OVERRIDE { return NULL; } On ...
8 years, 9 months ago (2012-03-20 13:35:06 UTC) #19
Primiano Tucci (use gerrit)
8 years, 9 months ago (2012-03-20 13:44:17 UTC) #20
It is fine, its purpose is to detect future changes of the audio subsystem
(calling makeLinear etc) by means on errors on the (debug) trybots.
I already added it and committed (should be in the master)

Thanks,
Primiano

On 2012/03/20 13:35:06, scherkus wrote:
>
https://chromiumcodereview.appspot.com/9692038/diff/11001/content/browser/spe...
> File content/browser/speech/speech_recognizer_impl_unittest.cc (right):
> 
>
https://chromiumcodereview.appspot.com/9692038/diff/11001/content/browser/spe...
> content/browser/speech/speech_recognizer_impl_unittest.cc:54: const
> AudioParameters& params) OVERRIDE { return NULL; }
> On 2012/03/14 11:40:11, xians1 wrote:
> > On 2012/03/14 11:35:15, Primiano Tucci wrote:
> > > Can you just add NOTREACHED() (also in the other 3 methods below) before
> > > returning NULL, so we'll be sure not to end in dereferencing a null
pointer?
> > > Thanks.
> > 
> > Hi Primiano, I am sorry I pressed the button too early, please let me make
> > another CL to address your comment. 
> > 
> > Thanks,
> > BR,
> > /SX
> 
> FYI NOTREACHED() only works in debug build as it expands to DCHECK(false)

Powered by Google App Engine
This is Rietveld 408576698