Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
131503006:
Initialization of audio manager for Android is now done on the audio thread (Closed)
|
Created:
6 years, 11 months ago by henrika (OOO until Aug 14) Modified:
6 years, 10 months ago CC:
chromium-reviews, fischman+watch_chromium.org, jam, mcasas+watch_chromium.org, joi+watch-content_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, wjia+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionInitialization of audio manager for Android is now done on the audio thread.
Goal is to move heavy initialization tasks from the IO thread to the audio thread instead.
This CL also ensures that the Android audio unit test calls audio manager methods on the audio thread to ensure the same behavior as in Chrome. In addition, all input/output-streams will be created and used on the audio thread.
BUG=337867
TEST=Manual getSources demos, media_unittests, content_unittests and WebRTC apprtc demo
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=252614
Patch Set 1 #
Total comments: 10
Patch Set 2 : tommi@ #Patch Set 3 : improved version #Patch Set 4 : Moved more calls to the audio thread in C++ #
Total comments: 1
Patch Set 5 : Now passes findbugs #
Total comments: 25
Patch Set 6 : tommi@ #
Total comments: 4
Patch Set 7 : improved unit test pattern #
Total comments: 2
Patch Set 8 : improved structure of all input unittests #
Total comments: 7
Patch Set 9 : Moved ctor+init and dtor of manager to audio thread #
Total comments: 7
Patch Set 10 : Added use of CreateAndInit in more methods #
Total comments: 13
Patch Set 11 : tommi@ #Patch Set 12 : Fixed all unit tests #
Total comments: 12
Patch Set 13 : Render message filter now posts audio messaged on audio thread #Patch Set 14 : No more use of wait in Android audio manager #Patch Set 15 : All methods in unit tests should now be on the audio thread #
Total comments: 24
Patch Set 16 : tommi@ #
Total comments: 4
Patch Set 17 : Removed bad getter in unit test #
Total comments: 4
Patch Set 18 : DEBUG=false #Patch Set 19 : No dangling pointers now #
Total comments: 12
Patch Set 20 : bulach@ #
Total comments: 2
Patch Set 21 : nit #
Total comments: 9
Patch Set 22 : dalecurtis@ #Patch Set 23 : Modified AudioInputTest #
Total comments: 6
Patch Set 24 : non-trivial rebase #Patch Set 25 : nits #Patch Set 26 : AudioInputTest now runs on audio thread #Patch Set 27 : AudioManagerTest now works on Android as well #Patch Set 28 : Removed two DCHECKs to avoid non-trivial fixes in content_unittests #Patch Set 29 : DEBUG=false #Patch Set 30 : Restored DCHECKs and disabled unit test in content_unittessts instead #
Total comments: 6
Patch Set 31 : tommi@ #Patch Set 32 : now compiles #Messages
Total messages: 83 (0 generated)
Tommi, time to take an initial look? I have moved out all complex tasks from the ctor to ensure that we start up Chrome as "thin" as possible. https://codereview.chromium.org/131503006/diff/1/content/browser/renderer_hos... File content/browser/renderer_host/media/device_request_message_filter.cc (right): https://codereview.chromium.org/131503006/diff/1/content/browser/renderer_hos... content/browser/renderer_host/media/device_request_message_filter.cc:96: // if (!resource_context_->AllowMicAccess(request_it->origin)) Test only. Will be removed!
please update the description to explain what functionality we're moving out of the 'init' sequence and defering. It's not exactly clear right now what the cl does. https://codereview.chromium.org/131503006/diff/1/media/base/android/java/src/... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/131503006/diff/1/media/base/android/java/src/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:41: private static final boolean DEBUG = true ; make sure to not check in https://codereview.chromium.org/131503006/diff/1/media/base/android/java/src/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:428: if (DEBUG) logd("initAudioDeviceList"); is there a way to do thread checks in java? https://codereview.chromium.org/131503006/diff/1/media/base/android/java/src/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:433: for (int i = 0; i < DEVICE_COUNT; ++i) { is this loop actually needed? https://codereview.chromium.org/131503006/diff/1/media/base/android/java/src/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:438: if (hasEarpiece()) { nit: could do mAudioDevices[DEVICE_EARPIECE] = hasEarpiece();
Added comments on input from tommi@. https://codereview.chromium.org/131503006/diff/1/media/base/android/java/src/... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/131503006/diff/1/media/base/android/java/src/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:41: private static final boolean DEBUG = true ; As always ;-) https://codereview.chromium.org/131503006/diff/1/media/base/android/java/src/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:428: if (DEBUG) logd("initAudioDeviceList"); Good question. Must to some research and come back in that one. https://codereview.chromium.org/131503006/diff/1/media/base/android/java/src/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:433: for (int i = 0; i < DEVICE_COUNT; ++i) { Added it just in case; most likely asked for in another review. Want me to skip it? https://codereview.chromium.org/131503006/diff/1/media/base/android/java/src/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:438: if (hasEarpiece()) { On 2014/01/27 15:09:44, tommi wrote: > nit: could do > mAudioDevices[DEVICE_EARPIECE] = hasEarpiece(); Done.
https://codereview.chromium.org/131503006/diff/1/media/base/android/java/src/... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/131503006/diff/1/media/base/android/java/src/... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:433: for (int i = 0; i < DEVICE_COUNT; ++i) { On 2014/01/27 15:22:49, henrika wrote: > Added it just in case; most likely asked for in another review. Want me to skip > it? I think that java will initialize all members of this array to false.
Changes based on input from tommi@. Also added initial thread checking. tommy@: PTAL. Adding bulach@ as owner.
tommi@: as discussed offline, more calls are now on the audio thread and is controlled from C++ instead of Java. I have kept some internal logging/comments to make the changes more clear. Will be removed of course. PTAL
https://codereview.chromium.org/131503006/diff/140001/media/base/android/java... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/131503006/diff/140001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:274: logd("@AudioManagerAndroid: " + Thread.currentThread().getName()); All these logs will be removed. Just want to keep it there until we are done for debugging purposes.
https://codereview.chromium.org/131503006/diff/160001/media/audio/android/aud... File media/audio/android/audio_manager_android.cc (right): https://codereview.chromium.org/131503006/diff/160001/media/audio/android/aud... media/audio/android/audio_manager_android.cc:84: GetTaskRunner()->PostTask( nit: You could avoid calling GetTaskRunner more than once. https://codereview.chromium.org/131503006/diff/160001/media/audio/android/aud... media/audio/android/audio_manager_android.cc:376: base::WaitableEvent event(false, false); When is SetAudioDevice called outside of the audio thread? I thought we would basically just add a DCHECK at the top of this function: DCHECK(GetTaskRunner()->BelongsToCurrentThread()); and then do what is now done in SetAudioDeviceOnAudioThread. If that's possible now, I would like to do it now instead of running into a situation like we now have with MakeAudio[Input|Output]Stream. https://codereview.chromium.org/131503006/diff/160001/media/audio/android/aud... File media/audio/android/audio_manager_android.h (right): https://codereview.chromium.org/131503006/diff/160001/media/audio/android/aud... media/audio/android/audio_manager_android.h:63: virtual void WillDestroyCurrentMessageLoop() OVERRIDE; make this private https://codereview.chromium.org/131503006/diff/160001/media/base/android/java... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/131503006/diff/160001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:35: /* I'm assuming you're going to remove all of this :) https://codereview.chromium.org/131503006/diff/160001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:563: if (mIsAudioListIsInitialized) { no {} needed https://codereview.chromium.org/131503006/diff/160001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:566: for (int i = 0; i < DEVICE_COUNT; ++i) { ditto. Also ping on this loop being unnecessary :) https://codereview.chromium.org/131503006/diff/160001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:599: if (!mIsAudioListIsInitialized) { no {} https://codereview.chromium.org/131503006/diff/160001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:606: for (int i = 0; i < DEVICE_COUNT; ++i) { no {} https://codereview.chromium.org/131503006/diff/160001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:684: private boolean hasWiredHeadset() { Does this have to be a method? Why not just call mAudioManager.isWiredHeadsetOn() where you need it? Is it do reduce the scope of @Deprecated? (If so, sgtm). https://codereview.chromium.org/131503006/diff/160001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:757: return; nit: Is the 'return' statement necessary? I think logwtf will basically throw an exception. https://codereview.chromium.org/131503006/diff/160001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:829: return; same here (and if you remove the return, also remove {})
https://codereview.chromium.org/131503006/diff/160001/media/base/android/java... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/131503006/diff/160001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:125: private Long mThreadId = null; nit: use 0 instead of null since this is a Long and not a reference.
Comments only. Will start modifying the unit test next to ensure that those AM calls that are done on the audio thread in Chrome will be called on the same thread in the unit test. https://codereview.chromium.org/131503006/diff/160001/media/audio/android/aud... File media/audio/android/audio_manager_android.cc (right): https://codereview.chromium.org/131503006/diff/160001/media/audio/android/aud... media/audio/android/audio_manager_android.cc:84: GetTaskRunner()->PostTask( On 2014/01/30 12:51:11, tommi wrote: > nit: You could avoid calling GetTaskRunner more than once. Done. https://codereview.chromium.org/131503006/diff/160001/media/audio/android/aud... media/audio/android/audio_manager_android.cc:376: base::WaitableEvent event(false, false); I will have to fix the unit test to do so. Discussed with Joi. Will use same style as he does in his private API test. https://codereview.chromium.org/131503006/diff/160001/media/audio/android/aud... File media/audio/android/audio_manager_android.h (right): https://codereview.chromium.org/131503006/diff/160001/media/audio/android/aud... media/audio/android/audio_manager_android.h:63: virtual void WillDestroyCurrentMessageLoop() OVERRIDE; On 2014/01/30 12:51:11, tommi wrote: > make this private Done. https://codereview.chromium.org/131503006/diff/160001/media/base/android/java... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/131503006/diff/160001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:35: /* On 2014/01/30 12:51:11, tommi wrote: > I'm assuming you're going to remove all of this :) Done. https://codereview.chromium.org/131503006/diff/160001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:125: private Long mThreadId = null; Actually, I need 0L here. https://codereview.chromium.org/131503006/diff/160001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:563: if (mIsAudioListIsInitialized) { On 2014/01/30 12:51:11, tommi wrote: > no {} needed Done. https://codereview.chromium.org/131503006/diff/160001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:566: for (int i = 0; i < DEVICE_COUNT; ++i) { On 2014/01/30 12:51:11, tommi wrote: > ditto. > > Also ping on this loop being unnecessary :) Done. https://codereview.chromium.org/131503006/diff/160001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:599: if (!mIsAudioListIsInitialized) { On 2014/01/30 12:51:11, tommi wrote: > no {} Done. https://codereview.chromium.org/131503006/diff/160001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:606: for (int i = 0; i < DEVICE_COUNT; ++i) { Removed all of it. https://codereview.chromium.org/131503006/diff/160001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:684: private boolean hasWiredHeadset() { Correct, I wanted to make it more clear that it was this API only that was deprecated. Keeping, thanks. https://codereview.chromium.org/131503006/diff/160001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:757: return; I have not been able to verify that actually. Also, think you asked to a return in a previous CL. There are examples of both styles in Chrome today. Most add return. OK with keeping? https://codereview.chromium.org/131503006/diff/160001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:829: return; See comment above.
do you mind uploading the changes that you've marked as "Done" so that reviewing will be easier? https://codereview.chromium.org/131503006/diff/160001/media/base/android/java... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/131503006/diff/160001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:757: return; On 2014/01/30 14:48:24, henrika wrote: > I have not been able to verify that actually. Also, think you asked to a return > in a previous CL. There are examples of both styles in Chrome today. Most add > return. > > OK with keeping? OK.
https://codereview.chromium.org/131503006/diff/180001/media/audio/android/aud... File media/audio/android/audio_android_unittest.cc (right): https://codereview.chromium.org/131503006/diff/180001/media/audio/android/aud... media/audio/android/audio_android_unittest.cc:592: base::WaitableEvent event_; This pattern is racy and could throw TSAN off. CreateAndCloseAISOnAudioThread is now touching this variable on two different threads (at least) and there's no DCHECK inside the |if (!audio_manager()->GetTaskRunner()->BelongsToCurrentThread())| check that we're always on the same thread there, so we're open to even more threads being in the picture and that's a bug. I realize that this is test code with very specific intentions but the bug is not obvious so it's possible that the assumptions that are in here (and not documented) go by unnoticed and someone copies this pattern into production code. https://codereview.chromium.org/131503006/diff/180001/media/audio/android/aud... File media/audio/android/audio_manager_android.cc (right): https://codereview.chromium.org/131503006/diff/180001/media/audio/android/aud... media/audio/android/audio_manager_android.cc:209: // DCHECK(GetTaskRunner()->BelongsToCurrentThread()); will you uncomment this before checkin?
https://codereview.chromium.org/131503006/diff/180001/media/audio/android/aud... File media/audio/android/audio_android_unittest.cc (right): https://codereview.chromium.org/131503006/diff/180001/media/audio/android/aud... media/audio/android/audio_android_unittest.cc:592: base::WaitableEvent event_; Uploaded new and improved version after offline discussion with you. Please check proposal of new pattern in next patch. Thanks. https://codereview.chromium.org/131503006/diff/180001/media/audio/android/aud... File media/audio/android/audio_manager_android.cc (right): https://codereview.chromium.org/131503006/diff/180001/media/audio/android/aud... media/audio/android/audio_manager_android.cc:209: // DCHECK(GetTaskRunner()->BelongsToCurrentThread()); On 2014/01/30 15:26:04, tommi wrote: > will you uncomment this before checkin? Done.
https://codereview.chromium.org/131503006/diff/200001/media/audio/android/aud... File media/audio/android/audio_android_unittest.cc (right): https://codereview.chromium.org/131503006/diff/200001/media/audio/android/aud... media/audio/android/audio_android_unittest.cc:598: EXPECT_TRUE(audio_input_stream_->Open()); I think this line should be in OpenAndClose and not CreateAndClose, right?
https://codereview.chromium.org/131503006/diff/200001/media/audio/android/aud... File media/audio/android/audio_android_unittest.cc (right): https://codereview.chromium.org/131503006/diff/200001/media/audio/android/aud... media/audio/android/audio_android_unittest.cc:598: EXPECT_TRUE(audio_input_stream_->Open()); Ooops ;-) Thanks.
tommi@: I have a final proposal now for all the input unit tests. They work well and now runs on the proper threads. Would be super if you could take a look so I don't inherit any bad style when I fix the output part as well.
https://codereview.chromium.org/131503006/diff/220001/media/audio/android/aud... File media/audio/android/audio_android_unittest.cc (right): https://codereview.chromium.org/131503006/diff/220001/media/audio/android/aud... media/audio/android/audio_android_unittest.cc:429: base::RunLoop().RunUntilIdle(); what exactly is it that happens during this loop? https://codereview.chromium.org/131503006/diff/220001/media/audio/android/aud... media/audio/android/audio_android_unittest.cc:433: base::RunLoop().RunUntilIdle(); add a comment for why we need this? https://codereview.chromium.org/131503006/diff/220001/media/audio/android/aud... media/audio/android/audio_android_unittest.cc:517: AudioInputStream* ais() { return audio_input_stream_; } I would keep audio_input_stream() in the spirit of "Function names, variable names, and filenames should be descriptive; eschew abbreviation."
Only comments about what will be done. Will upload new version on Monday. https://codereview.chromium.org/131503006/diff/220001/media/audio/android/aud... File media/audio/android/audio_android_unittest.cc (right): https://codereview.chromium.org/131503006/diff/220001/media/audio/android/aud... media/audio/android/audio_android_unittest.cc:429: base::RunLoop().RunUntilIdle(); Sorry, my bad. Copied from test that Dale had just modified. Must admit that I don't know the exact benefit. Will remove. https://codereview.chromium.org/131503006/diff/220001/media/audio/android/aud... media/audio/android/audio_android_unittest.cc:433: base::RunLoop().RunUntilIdle(); Will remove. https://codereview.chromium.org/131503006/diff/220001/media/audio/android/aud... media/audio/android/audio_android_unittest.cc:517: AudioInputStream* ais() { return audio_input_stream_; } Will fix.
Follow up question, in the AudioManagerAndroid constructor I see the j_audio_manager_ being attached to the current thread, which is the UI thread. Should that be deferred to the audio thread? Should we even be using the audio thread on Android? https://codereview.chromium.org/131503006/diff/220001/media/audio/android/aud... File media/audio/android/audio_android_unittest.cc (right): https://codereview.chromium.org/131503006/diff/220001/media/audio/android/aud... media/audio/android/audio_android_unittest.cc:429: base::RunLoop().RunUntilIdle(); On 2014/01/31 14:19:05, henrika wrote: > Sorry, my bad. Copied from test that Dale had just modified. Must admit that I > don't know the exact benefit. Will remove. This is only needed on platforms which post initialization to the audio thread.
Some of the operations can block for some time but maybe the audio thread isn't the right thread? Should we use the worker? Also, should we perhaps move all the Java related stuff away from the ui thread? It might simplify our threading story. On Jan 31, 2014 8:18 PM, <dalecurtis@chromium.org> wrote: > Follow up question, in the AudioManagerAndroid constructor I see the > j_audio_manager_ being attached to the current thread, which is the UI > thread. > Should that be deferred to the audio thread? Should we even be using the > audio > thread on Android? > > > https://codereview.chromium.org/131503006/diff/220001/ > media/audio/android/audio_android_unittest.cc > File media/audio/android/audio_android_unittest.cc (right): > > https://codereview.chromium.org/131503006/diff/220001/ > media/audio/android/audio_android_unittest.cc#newcode429 > media/audio/android/audio_android_unittest.cc:429: > base::RunLoop().RunUntilIdle(); > On 2014/01/31 14:19:05, henrika wrote: > >> Sorry, my bad. Copied from test that Dale had just modified. Must >> > admit that I > >> don't know the exact benefit. Will remove. >> > > This is only needed on platforms which post initialization to the audio > thread. > > https://codereview.chromium.org/131503006/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Depends on the operation and how long it's blocking for, but in general the audio thread is probably fine. I'm more concerned with the UI thread usage and races / blocks that may cause.
Henrik, wdyt? Should we move the Java object lifetime and usage completely over to the audio thread? It would simplify initialization/termination and synchronization. Also, there would presumably be a performance increase to Chrome's startup time. On Feb 1, 2014 12:44 AM, <dalecurtis@chromium.org> wrote: > Depends on the operation and how long it's blocking for, but in general the > audio thread is probably fine. I'm more concerned with the UI thread > usage and > races / blocks that may cause. > > https://codereview.chromium.org/131503006/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/02/01 09:31:01, tommi wrote: > Henrik, wdyt? Should we move the Java object lifetime and usage completely > over to the audio thread? > It would simplify initialization/termination and synchronization. > Also, there would presumably be a performance increase to Chrome's startup > time. I did not write the initial version of the AM for Android and can't say for sure why it was designed as it is today. Let me do some more analysis and come back.
..and, many thanks for your valuable feedback Dale :-)
tommi@: uploaded an initial version (not complete) where I do create+init first time needed and close when the message loop dies. Also consolidated init and close into two methods again in Java. Plan was to add CreateAndInit in more methods if you like the style.
https://codereview.chromium.org/131503006/diff/430001/media/audio/android/aud... File media/audio/android/audio_manager_android.cc (right): https://codereview.chromium.org/131503006/diff/430001/media/audio/android/aud... media/audio/android/audio_manager_android.cc:67: void AudioManagerAndroid::GetAudioInputDeviceNames( Is there no way to move this call on to the audio thread? Is this just for unittests? Apologies if I've missed previous discussion on this. https://codereview.chromium.org/131503006/diff/430001/media/audio/android/aud... media/audio/android/audio_manager_android.cc:96: CreateAndInit(); I think you need to always post this task during construction, similar to how we handle on audio thread initialization on OSX/Win. You can then avoid any waitable events for that call.
Feedback on input from Dale. https://codereview.chromium.org/131503006/diff/430001/media/audio/android/aud... File media/audio/android/audio_manager_android.cc (right): https://codereview.chromium.org/131503006/diff/430001/media/audio/android/aud... media/audio/android/audio_manager_android.cc:67: void AudioManagerAndroid::GetAudioInputDeviceNames( It is called on a so called device thread in the audio input device manager and I did not feel confident in changing that since I know that thread was added for a reason. I moved it to the audio thread here to simplify the threading model in Java (to reduce the number of threads calling in). https://codereview.chromium.org/131503006/diff/430001/media/audio/android/aud... media/audio/android/audio_manager_android.cc:96: CreateAndInit(); I recall your recent CL on Windows doing this but off-line discussions with Tommi resulted in this solution instead. I would like to wait for comments from Tommi as well. My main goal here is to make construction of Chrome on Android as light as possible and only touch audio components when needed.
Dale, the last round is not inline with your comments (instead I call CreateAndInit from more methods). Hope it is OK for now while waiting for feedback from Tommi.
https://codereview.chromium.org/131503006/diff/430001/media/audio/android/aud... File media/audio/android/audio_manager_android.cc (right): https://codereview.chromium.org/131503006/diff/430001/media/audio/android/aud... media/audio/android/audio_manager_android.cc:67: void AudioManagerAndroid::GetAudioInputDeviceNames( On 2014/02/03 19:23:42, henrika wrote: > It is called on a so called device thread in the audio input device manager and > I did not feel confident in changing that since I know that thread was added for > a reason. I moved it to the audio thread here to simplify the threading model in > Java (to reduce the number of threads calling in). Actually we can remove that thread now that we have the concept of a worker thread. Here's the CL that reverted the change previously: https://codereview.chromium.org/18325021 It was reverted because the "audio thread" on OSX is the UI thread and enumeration can block for seconds. At the time we didn't have the concept of a worker thread, but we do now. That CL can now be relanded using GetWorkerTaskRunner(). Want to take care of making that change?
https://codereview.chromium.org/131503006/diff/430001/media/audio/android/aud... File media/audio/android/audio_manager_android.cc (right): https://codereview.chromium.org/131503006/diff/430001/media/audio/android/aud... media/audio/android/audio_manager_android.cc:67: void AudioManagerAndroid::GetAudioInputDeviceNames( I would very much appreciate if you could close the loop on that part Dale. You know the whole background and I could fix this unit test once that thread has been removed, but in a separate CL. Took a quick lock at it and can still see some questions regarding UMA and method names that are still indicated that the device thread exists.
https://codereview.chromium.org/131503006/diff/490001/content/browser/rendere... File content/browser/renderer_host/media/device_request_message_filter.cc (right): https://codereview.chromium.org/131503006/diff/490001/content/browser/rendere... content/browser/renderer_host/media/device_request_message_filter.cc:96: // if (!resource_context_->AllowMicAccess(request_it->origin)) fix? https://codereview.chromium.org/131503006/diff/490001/media/audio/android/aud... File media/audio/android/audio_manager_android.cc (left): https://codereview.chromium.org/131503006/diff/490001/media/audio/android/aud... media/audio/android/audio_manager_android.cc:51: j_audio_manager_.Reset( yay! https://codereview.chromium.org/131503006/diff/490001/media/audio/android/aud... File media/audio/android/audio_manager_android.cc (right): https://codereview.chromium.org/131503006/diff/490001/media/audio/android/aud... media/audio/android/audio_manager_android.cc:103: // TODO(henrika): do we nee a requirement to call the rest of this method typo I assume this with regards to Dale's question? https://codereview.chromium.org/131503006/diff/490001/media/audio/android/aud... media/audio/android/audio_manager_android.cc:287: bool AudioManagerAndroid::HadNoAudioStreams() { This should be called HasNoAudioStreams. "Had" is past tense and this suggests that the current state may be different while it is actually the current state that is being returned. I think there must have been some confusion when giving variables names at some call sites: bool had_no_streams = HadNoAudioStreams(); // do some work if (had_no_streams) { // Do more work. } The naming of the above variable is appropriate due to the _second_ place where it is used. The "HadNoAudioStreams" method of course is not involved in that condition, so its name should not be affected by it. So, I guess what I'm saying is please fix :) https://codereview.chromium.org/131503006/diff/490001/media/base/android/java... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/131503006/diff/490001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:41: private static final boolean DEBUG = true; remember to revert. https://codereview.chromium.org/131503006/diff/490001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:253: if (!mNonThreadSafe.calledOnValidThread()) { Could this be consolidated into a method? private void CheckIfCalledOnValidThread() { if (DEBUG && !mNonThreadSafe.calledOnValidThread()) { logwtf("Called on invalid thread!"); } } private void close() { CheckIfCalledOnValidThread(); ... } https://codereview.chromium.org/131503006/diff/490001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:382: if (DEBUG) logd("getAudioInputDeviceNames"); double logging? in general I think we can do with some cleanup of logging code here (or maybe you didn't intend to check any of the new logging code in?)
https://codereview.chromium.org/131503006/diff/430001/media/audio/android/aud... File media/audio/android/audio_manager_android.cc (right): https://codereview.chromium.org/131503006/diff/430001/media/audio/android/aud... media/audio/android/audio_manager_android.cc:67: void AudioManagerAndroid::GetAudioInputDeviceNames( On 2014/02/04 10:09:36, henrika wrote: > I would very much appreciate if you could close the loop on that part Dale. You > know the whole background and I could fix this unit test once that thread has > been removed, but in a separate CL. Took a quick lock at it and can still see > some questions regarding UMA and method names that are still indicated that the > device thread exists. https://codereview.chromium.org/154143002/
Uploaded changes based on input from tommi@. Sorry for all the rounds here but what now remains is: 1) Fix deadlock in unittest for input streaming. 2) Wait one day for Dale's thread-patch to land and do changes based on it. 3) Add similar "thread wrapping" for output as on input. 4 Get OK from owner bulach@ I will work on (1) and (3) next. https://codereview.chromium.org/131503006/diff/490001/content/browser/rendere... File content/browser/renderer_host/media/device_request_message_filter.cc (right): https://codereview.chromium.org/131503006/diff/490001/content/browser/rendere... content/browser/renderer_host/media/device_request_message_filter.cc:96: // if (!resource_context_->AllowMicAccess(request_it->origin)) I am not sure what a proper fix is. I only added the hack we came up with to support non https while testing. OK to use and revert before commit? https://codereview.chromium.org/131503006/diff/490001/media/audio/android/aud... File media/audio/android/audio_manager_android.cc (left): https://codereview.chromium.org/131503006/diff/490001/media/audio/android/aud... media/audio/android/audio_manager_android.cc:51: j_audio_manager_.Reset( thx https://codereview.chromium.org/131503006/diff/490001/media/audio/android/aud... File media/audio/android/audio_manager_android.cc (right): https://codereview.chromium.org/131503006/diff/490001/media/audio/android/aud... media/audio/android/audio_manager_android.cc:103: // TODO(henrika): do we nee a requirement to call the rest of this method On 2014/02/04 14:30:12, tommi wrote: > typo > > I assume this with regards to Dale's question? Done. https://codereview.chromium.org/131503006/diff/490001/media/audio/android/aud... media/audio/android/audio_manager_android.cc:287: bool AudioManagerAndroid::HadNoAudioStreams() { Will fix, nop. Just FYI, it was changed based on comments from other reviewers (bulach@ perhaps). https://codereview.chromium.org/131503006/diff/490001/media/base/android/java... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/131503006/diff/490001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:41: private static final boolean DEBUG = true; I will. Remember ;-) https://codereview.chromium.org/131503006/diff/490001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:253: if (!mNonThreadSafe.calledOnValidThread()) { On 2014/02/04 14:30:12, tommi wrote: > Could this be consolidated into a method? > > private void CheckIfCalledOnValidThread() { > if (DEBUG && !mNonThreadSafe.calledOnValidThread()) { > logwtf("Called on invalid thread!"); > } > } > > > private void close() { > CheckIfCalledOnValidThread(); > ... > } Done.
Updated TODO-list: 1) Fix deadlock in unittest for input streaming. In progress. 2) Wait one day for Dale's thread-patch to land and do changes based on it. 3) Add similar "thread wrapping" for output as on input. DONE 4 Get OK from owner bulach@ bulach@: would be great with comments on the Java side from you as owner. I will remove all logs related to thread names before committing.
https://chromiumcodereview.appspot.com/131503006/diff/640001/media/audio/andr... File media/audio/android/audio_manager_android.cc (right): https://chromiumcodereview.appspot.com/131503006/diff/640001/media/audio/andr... media/audio/android/audio_manager_android.cc:73: // calling thread sees this function call as synchronous. what is the calling thread? we obviously try to avoid blocking threads as much as possible.. is there any chance of changing this API to be fully asynchronous, i.e., rather than taking a AudioDeviceNames* get a callback that would be triggered later on? it's probably more involved than this patch, but may be worth investigating. https://chromiumcodereview.appspot.com/131503006/diff/640001/media/audio/andr... media/audio/android/audio_manager_android.cc:306: base::WaitableEvent event(false, false); yeah, as above, this is a really bad pattern, we should try to avoid it.. https://chromiumcodereview.appspot.com/131503006/diff/640001/media/base/andro... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://chromiumcodereview.appspot.com/131503006/diff/640001/media/base/andro... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:473: return Build.VERSION.SDK_INT >= Build.VERSION_CODES.JELLY_BEAN && I suppose this is a good candidate to make it more robust.. I think since it's a static method, and when loading the class it probably needs the reference to AcousticEchoCanceler, we could try something like: if (Build.VERSION.SDK_INT < Build.VERSION_CODES.JELLY_BEAN) return false; try { } except ... return false;
Thanks Marcus, I understand that you want to break out most parts of this CL into a separate refactor CL and instead focus on a more isolated fix of the exact reported crash issue. I will come up with a new (much smaller) "SDK path" tomorrow but will keep this one as well as a follow-up. https://codereview.chromium.org/131503006/diff/640001/media/audio/android/aud... File media/audio/android/audio_manager_android.cc (right): https://codereview.chromium.org/131503006/diff/640001/media/audio/android/aud... media/audio/android/audio_manager_android.cc:73: // calling thread sees this function call as synchronous. Right now the calling threads are a device manager thread or the IO thread. It will be changed to the audio manager thread or the IO thread. This method is sync for all other platforms and the idea here was to not change that pattern since it causes lots of changes. And as you have noticed, this is a rather large "fix" as is.. https://codereview.chromium.org/131503006/diff/640001/media/audio/android/aud... media/audio/android/audio_manager_android.cc:306: base::WaitableEvent event(false, false); I think it is a topic which we perhaps could discuss some more. But I hear you and will of course take it into consideration. Seems like we will break out a more isolated CL from this one instead of landing as is. So, hope it is OK if we close the topic at a later stage. https://codereview.chromium.org/131503006/diff/640001/media/base/android/java... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/131503006/diff/640001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:473: return Build.VERSION.SDK_INT >= Build.VERSION_CODES.JELLY_BEAN && Thanks for the proposal for a more isolated fix. Will upload a proposal tomorrow.
https://codereview.chromium.org/131503006/diff/640001/media/audio/android/aud... File media/audio/android/audio_manager_android.cc (right): https://codereview.chromium.org/131503006/diff/640001/media/audio/android/aud... media/audio/android/audio_manager_android.cc:73: // calling thread sees this function call as synchronous. On 2014/02/05 21:47:30, henrika wrote: > Right now the calling threads are a device manager thread or the IO thread. It > will be changed to the audio manager thread or the IO thread. This method is > sync for all other platforms and the idea here was to not change that pattern > since it causes lots of changes. And as you have noticed, this is a rather large > "fix" as is.. I suspect you can completely remove the IO thread usage from this entire system with some clever usage of OverrideTaskRunner. See an example here: https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...
https://codereview.chromium.org/131503006/diff/640001/media/audio/android/aud... File media/audio/android/audio_manager_android.cc (right): https://codereview.chromium.org/131503006/diff/640001/media/audio/android/aud... media/audio/android/audio_manager_android.cc:73: // calling thread sees this function call as synchronous. On 2014/02/05 21:50:13, DaleCurtis wrote: > On 2014/02/05 21:47:30, henrika wrote: > > Right now the calling threads are a device manager thread or the IO thread. It > > will be changed to the audio manager thread or the IO thread. This method is > > sync for all other platforms and the idea here was to not change that pattern > > since it causes lots of changes. And as you have noticed, this is a rather > large > > "fix" as is.. > > I suspect you can completely remove the IO thread usage from this entire system > with some clever usage of OverrideTaskRunner. See an example here: > > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re... yeah, let's separate the issues here.. :) 1) I'm hoping the original bug can be fixed (tentatively, at least) with just changes in the java side. that should be far more straightforward to review, land, cherry-pick into release branch and launch... fairly orthogonal, and could be done in multiple smaller steps: 2) whilst I quite like the original idea of this patch, i.e., to make the initialization less heavy, blocking the IO thread[1] is a no-no.. I'd much rather favor a redesign of the API to make it fully asynchronous rather than just a tweak in the implementation (trying to keep a "synchronous" API by blocking it under the cover...).. [1] sorry, this is fairly confusing to me.. is it BrowserThread::IO or the "Audio IO" mentioned in audio_manager.h? I think I'm missing the thread model diagram you mentioned on one of the TODOs.. I'd love to see that so I could get a better understanding! also, it's not quite clear to me why do we need to reduce the number of calling threads to the java layer... that side seems thread safe, no? anyways.... I don't want to be a pain, I just want to help splitting this beast in smaller / more manageable chunks, and solve each problem separately.. how does that sound?
https://codereview.chromium.org/131503006/diff/640001/media/audio/android/aud... File media/audio/android/audio_manager_android.cc (right): https://codereview.chromium.org/131503006/diff/640001/media/audio/android/aud... media/audio/android/audio_manager_android.cc:73: // calling thread sees this function call as synchronous. Calls to GetAudioInputDeviceNames (7 occurrences) - excluding unit tests below => 2 calls: src/chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_api.cc (1 occurrence) 407 AudioManager::Get()->GetAudioInputDeviceNames(&source_devices_); Calls on the audio thread (I don't call this an IO thread). See AudioManager::GetTaskRunner() for details. src/content/browser/renderer_host/media/audio_input_device_manager.cc (1 occurrence) 133 audio_manager_->GetAudioInputDeviceNames(&device_names); Calls on the audio thread. Same as above. Hence, I was wrong. It is only called on the audio thread, not the browser IO thread. Plus unit tests.
https://codereview.chromium.org/131503006/diff/640001/media/audio/android/aud... File media/audio/android/audio_manager_android.cc (right): https://codereview.chromium.org/131503006/diff/640001/media/audio/android/aud... media/audio/android/audio_manager_android.cc:73: // calling thread sees this function call as synchronous. On 2014/02/06 08:58:57, henrika wrote: > Calls to GetAudioInputDeviceNames (7 occurrences) - excluding unit tests below > => 2 calls: > > src/chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_api.cc > (1 occurrence) > 407 AudioManager::Get()->GetAudioInputDeviceNames(&source_devices_); > > Calls on the audio thread (I don't call this an IO thread). See > AudioManager::GetTaskRunner() for details. > > src/content/browser/renderer_host/media/audio_input_device_manager.cc > (1 occurrence) > 133 audio_manager_->GetAudioInputDeviceNames(&device_names); > > Calls on the audio thread. Same as above. > > Hence, I was wrong. It is only called on the audio thread, not the browser IO > thread. Plus unit tests. > > I looked into this and from what I can tell there is only a single call site to GetAudioInputDeviceNames that is not already on the audio thread and that call site is here: https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re... (inside AudioInputDeviceManager::EnumerateOnDeviceThread). This can be very easily fixed by using the audio manager's task runner instead of the device_loop_ here: https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re... (inside AudioInputDeviceManager::EnumerateDevices) It's so simple and it doesn't need any clever tricks. The OverrideTaskRunner option is good to know about though.
I forgot to add that by fixing that call site, we can remove the thread hopping and blocking code and add instead a DCHECK() to make sure that we're on the audio thread there. No interface changes needed.
Agree. Will continue this work once the "more limited" patch has landed.
https://codereview.chromium.org/131503006/diff/640001/media/base/android/java... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/131503006/diff/640001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:473: return Build.VERSION.SDK_INT >= Build.VERSION_CODES.JELLY_BEAN && On 2014/02/05 19:29:39, bulach wrote: > I suppose this is a good candidate to make it more robust.. > I think since it's a static method, and when loading the class it probably needs > the reference to AcousticEchoCanceler, we could try something like: > > if (Build.VERSION.SDK_INT < Build.VERSION_CODES.JELLY_BEAN) > return false; > try { > } except ... > return false; I looked into this quite a bit when I wrote the code. My understanding is that Android will only attempt to load the reference to AcousticEchoCanceler when it is encountered at runtime. I actually tested this with an ICS device at the time, and it behaved correctly (that is, returned false here and didn't crash). I was unable to find an authoritative answer to this question; it would be great to have one.
> I looked into this quite a bit when I wrote the code. My understanding is that > Android will only attempt to load the reference to AcousticEchoCanceler when it > is encountered at runtime. > > I actually tested this with an ICS device at the time, and it behaved correctly > (that is, returned false here and didn't crash). > > I was unable to find an authoritative answer to this question; it would be great > to have one. Thanks Andrew. I did similar tests for the "SDK code" when I added BT support. And it worked for me as well. It seems like most of the crashes are on non-official versions and I assume the can behave differently.
I have restarted the work on this one again. My plan was to not reply on all current comments but do a more profound change taking all your feedback into account instead. Could you please check the C++ parts of the audio manager and see what you guys think. Summary: No more "thread+switch+wait". Instead all calls are on the audio thread. If you like the current version better, I will check the unit tests again based on the new design as well. https://codereview.chromium.org/131503006/diff/640001/media/audio/android/aud... File media/audio/android/audio_manager_android.cc (right): https://codereview.chromium.org/131503006/diff/640001/media/audio/android/aud... media/audio/android/audio_manager_android.cc:73: // calling thread sees this function call as synchronous. Now only called on audio thread. Modified.
Can you ping back when you've made all the changes? Since I have to go through all the files for every review pretty much, I'd like to be efficient with my time. On Thu, Feb 13, 2014 at 4:00 PM, <henrika@chromium.org> wrote: > I have restarted the work on this one again. My plan was to not reply on > all > current comments but do a more profound change taking all your feedback > into > account instead. > > Could you please check the C++ parts of the audio manager and see what you > guys > think. > > Summary: No more "thread+switch+wait". Instead all calls are on the audio > thread. > > If you like the current version better, I will check the unit tests again > based > on the new design as well. > > > > https://codereview.chromium.org/131503006/diff/640001/ > media/audio/android/audio_manager_android.cc > File media/audio/android/audio_manager_android.cc (right): > > https://codereview.chromium.org/131503006/diff/640001/ > media/audio/android/audio_manager_android.cc#newcode73 > media/audio/android/audio_manager_android.cc:73: // calling thread sees > this function call as synchronous. > Now only called on audio thread. Modified. > > https://codereview.chromium.org/131503006/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/02/13 15:06:38, tommi wrote: > Can you ping back when you've made all the changes? Since I have to go > through all the files for every review pretty much, I'd like to be > efficient with my time. Of course! Will come back with a more complete proposal. Sorry for all patches here.
tommi@: hope you fine it OK to review now. Summary: - Only creates and initialized Java part of audio manager when needed. - Create and init takes place on audio thread. - All AM calls are moved to audio thread. - Unit test now calls on audio thread as well.
https://codereview.chromium.org/131503006/diff/970001/media/audio/android/aud... File media/audio/android/audio_android_unittest.cc (right): https://codereview.chromium.org/131503006/diff/970001/media/audio/android/aud... media/audio/android/audio_android_unittest.cc:426: AudioParameters audio_output_parameters() const { nit: return const&? https://codereview.chromium.org/131503006/diff/970001/media/audio/android/aud... media/audio/android/audio_android_unittest.cc:462: void MakeAOSOnAudioThread(const AudioParameters& params) { MakeOutputStreamOnAudioThread https://codereview.chromium.org/131503006/diff/970001/media/audio/android/aud... media/audio/android/audio_android_unittest.cc:469: void OpenAndCloseAOSOnAudioThread() { OpenAndCloseOutputStreamOnAudioThread https://codereview.chromium.org/131503006/diff/970001/media/audio/android/aud... media/audio/android/audio_android_unittest.cc:475: void OpenAndStartAOSOnAudioThread( AOS -> OutputStream same below https://codereview.chromium.org/131503006/diff/970001/media/audio/android/aud... media/audio/android/audio_android_unittest.cc:567: AudioOutputStream* audio_output_stream_; This variable is deleted when Close() is called, right? If so, then there are several places where we have a dangling pointer in these tests. Can you update the code to always NULL this pointer out when we know the object has been deleted? https://codereview.chromium.org/131503006/diff/970001/media/audio/android/aud... media/audio/android/audio_android_unittest.cc:620: void MakeAISOnAudioThread(const AudioParameters& params) { AIS -> InputStream https://codereview.chromium.org/131503006/diff/970001/media/audio/android/aud... media/audio/android/audio_android_unittest.cc:715: audio_input_stream()->Stop(); out of curiosity, is there a reason you call the method instead of using the member variable? https://codereview.chromium.org/131503006/diff/970001/media/audio/android/aud... media/audio/android/audio_android_unittest.cc:720: AudioInputStream* audio_input_stream_; same thing here re dangling pointer https://codereview.chromium.org/131503006/diff/970001/media/audio/android/aud... File media/audio/android/audio_manager_android.cc (right): https://codereview.chromium.org/131503006/diff/970001/media/audio/android/aud... media/audio/android/audio_manager_android.cc:163: void AudioManagerAndroid::ReleaseOutputStream(AudioOutputStream* stream) { If this method is expected to be called on the audio thread, it would be nice to have a dcheck for that. https://codereview.chromium.org/131503006/diff/970001/media/audio/android/aud... media/audio/android/audio_manager_android.cc:247: GetTaskRunner()->PostTask( On which thread is this method called? Shouldn't it be the audio thread now as other methods? https://codereview.chromium.org/131503006/diff/970001/media/audio/android/aud... media/audio/android/audio_manager_android.cc:397: for (OutputStreams::iterator it = streams_.begin(); it is starting to look to me like streams_ is only ever touched on the audio thread. Can you check if that's true? If it is, we can get rid of streams_lock_ and sprinkle DCHECKs for thread correctness wherever we touch this variable. (and yay! if so!) https://codereview.chromium.org/131503006/diff/970001/media/base/android/java... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/131503006/diff/970001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:511: !Build.MODEL.equals("SM-N9005") && // Galaxy Note 3 are you intentionally changing this order or do you just need to rebase?
Thanks Tommi. One more... https://codereview.chromium.org/131503006/diff/970001/media/audio/android/aud... File media/audio/android/audio_android_unittest.cc (right): https://codereview.chromium.org/131503006/diff/970001/media/audio/android/aud... media/audio/android/audio_android_unittest.cc:426: AudioParameters audio_output_parameters() const { On 2014/02/17 13:24:55, tommi wrote: > nit: return const&? Done. https://codereview.chromium.org/131503006/diff/970001/media/audio/android/aud... media/audio/android/audio_android_unittest.cc:462: void MakeAOSOnAudioThread(const AudioParameters& params) { On 2014/02/17 13:24:55, tommi wrote: > MakeOutputStreamOnAudioThread Done. https://codereview.chromium.org/131503006/diff/970001/media/audio/android/aud... media/audio/android/audio_android_unittest.cc:469: void OpenAndCloseAOSOnAudioThread() { On 2014/02/17 13:24:55, tommi wrote: > OpenAndCloseOutputStreamOnAudioThread Done. https://codereview.chromium.org/131503006/diff/970001/media/audio/android/aud... media/audio/android/audio_android_unittest.cc:475: void OpenAndStartAOSOnAudioThread( On 2014/02/17 13:24:55, tommi wrote: > AOS -> OutputStream > same below Done. https://codereview.chromium.org/131503006/diff/970001/media/audio/android/aud... media/audio/android/audio_android_unittest.cc:567: AudioOutputStream* audio_output_stream_; Done. Please check again. Perhaps you meant something more advanced. https://codereview.chromium.org/131503006/diff/970001/media/audio/android/aud... media/audio/android/audio_android_unittest.cc:620: void MakeAISOnAudioThread(const AudioParameters& params) { On 2014/02/17 13:24:55, tommi wrote: > AIS -> InputStream Done. https://codereview.chromium.org/131503006/diff/970001/media/audio/android/aud... media/audio/android/audio_android_unittest.cc:715: audio_input_stream()->Stop(); Actually no good reason at all. Do you want me to remove the getter? https://codereview.chromium.org/131503006/diff/970001/media/audio/android/aud... media/audio/android/audio_android_unittest.cc:720: AudioInputStream* audio_input_stream_; On 2014/02/17 13:24:55, tommi wrote: > same thing here re dangling pointer Done. https://codereview.chromium.org/131503006/diff/970001/media/audio/android/aud... File media/audio/android/audio_manager_android.cc (right): https://codereview.chromium.org/131503006/diff/970001/media/audio/android/aud... media/audio/android/audio_manager_android.cc:163: void AudioManagerAndroid::ReleaseOutputStream(AudioOutputStream* stream) { On 2014/02/17 13:24:55, tommi wrote: > If this method is expected to be called on the audio thread, it would be nice to > have a dcheck for that. Done. https://codereview.chromium.org/131503006/diff/970001/media/audio/android/aud... media/audio/android/audio_manager_android.cc:247: GetTaskRunner()->PostTask( It is a callback from Java and is not on the audio thread. https://codereview.chromium.org/131503006/diff/970001/media/audio/android/aud... media/audio/android/audio_manager_android.cc:397: for (OutputStreams::iterator it = streams_.begin(); It is in fact on only touched on the audio thread now. Removing ;-) https://codereview.chromium.org/131503006/diff/970001/media/base/android/java... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/131503006/diff/970001/media/base/android/java... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:511: !Build.MODEL.equals("SM-N9005") && // Galaxy Note 3 Yes. The idea was to sort them correctly. Sorry, should have mentioned it. Ami and Andrew asked for it earlier.
lgtm % getting rid of the dangling pointers in the test https://codereview.chromium.org/131503006/diff/1040001/media/audio/android/au... File media/audio/android/audio_android_unittest.cc (right): https://codereview.chromium.org/131503006/diff/1040001/media/audio/android/au... media/audio/android/audio_android_unittest.cc:549: audio_output_stream()->Close(); Everywhere where we do this, we have a dangling pointer. If there's no value in the audio_output_stream() method. I'd rather use the member variable directly and remove the methods to reduce the chances of this. https://codereview.chromium.org/131503006/diff/1040001/media/audio/android/au... media/audio/android/audio_android_unittest.cc:705: audio_input_stream()->Close(); same for the input side
Thanks Tommi. bulach@: I would appreciate some comments from you as owner as well. dalecurtis@: any additional feedback? https://codereview.chromium.org/131503006/diff/1040001/media/audio/android/au... File media/audio/android/audio_android_unittest.cc (right): https://codereview.chromium.org/131503006/diff/1040001/media/audio/android/au... media/audio/android/audio_android_unittest.cc:549: audio_output_stream()->Close(); On 2014/02/18 12:03:35, tommi wrote: > Everywhere where we do this, we have a dangling pointer. > > If there's no value in the audio_output_stream() method. I'd rather use the > member variable directly and remove the methods to reduce the chances of this. Done. https://codereview.chromium.org/131503006/diff/1040001/media/audio/android/au... media/audio/android/audio_android_unittest.cc:705: audio_input_stream()->Close(); On 2014/02/18 12:03:35, tommi wrote: > same for the input side Done.
still some dangling pointers... please just search for all places where we call Close() and make sure we null out the pointers after that. https://codereview.chromium.org/131503006/diff/1130001/media/audio/android/au... File media/audio/android/audio_android_unittest.cc (right): https://codereview.chromium.org/131503006/diff/1130001/media/audio/android/au... media/audio/android/audio_android_unittest.cc:546: audio_output_stream_->Close(); dangling pointer. https://codereview.chromium.org/131503006/diff/1130001/media/audio/android/au... media/audio/android/audio_android_unittest.cc:703: audio_input_stream_->Close(); dangling pointer.
Sorry.. now it is perfect. https://codereview.chromium.org/131503006/diff/1130001/media/audio/android/au... File media/audio/android/audio_android_unittest.cc (right): https://codereview.chromium.org/131503006/diff/1130001/media/audio/android/au... media/audio/android/audio_android_unittest.cc:546: audio_output_stream_->Close(); Sorry. Fixed. https://codereview.chromium.org/131503006/diff/1130001/media/audio/android/au... media/audio/android/audio_android_unittest.cc:703: audio_input_stream_->Close(); Sorry. Fixed.
lgtm++
lgtm % tiny nits, thanks a lot! https://codereview.chromium.org/131503006/diff/1330001/media/audio/android/au... File media/audio/android/audio_manager_android.cc (right): https://codereview.chromium.org/131503006/diff/1330001/media/audio/android/au... media/audio/android/audio_manager_android.cc:289: DCHECK(GetTaskRunner()->BelongsToCurrentThread()); nit: unindent https://codereview.chromium.org/131503006/diff/1330001/media/audio/android/au... media/audio/android/audio_manager_android.cc:292: if (!j_audio_manager_.is_null()) { nit: could remove braces (similar to 276 and 318) https://codereview.chromium.org/131503006/diff/1330001/media/audio/android/au... media/audio/android/audio_manager_android.cc:302: Java_AudioManagerAndroid_createAudioManagerAndroid( nit: indent by another 2, and then the following lines as well https://codereview.chromium.org/131503006/diff/1330001/media/base/android/jav... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/131503006/diff/1330001/media/base/android/jav... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:50: private Long mThreadId = 0L; nit: make it final, and assign directly in the constructor, and remove the "ensureTHreadIdAssigned". https://codereview.chromium.org/131503006/diff/1330001/media/base/android/jav... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:220: * init() to the constructor and code in close() to finalize(). is finalize() a new method you'll introduce or a java finalizer? if the java finalizer, we shouldn't rely on that: - there's no guarantees it'll ever be called - even if it is, it's probably called on the GC thread, which is probably different from the thread that it was created. https://codereview.chromium.org/131503006/diff/1330001/media/base/android/jav... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:515: Log.wtf(TAG,"Method is not called on valid thread!"); nit: space after ,
Great comments Marcus! Dale: any final input? https://codereview.chromium.org/131503006/diff/1330001/media/audio/android/au... File media/audio/android/audio_manager_android.cc (right): https://codereview.chromium.org/131503006/diff/1330001/media/audio/android/au... media/audio/android/audio_manager_android.cc:289: DCHECK(GetTaskRunner()->BelongsToCurrentThread()); On 2014/02/18 13:32:07, bulach wrote: > nit: unindent Done. https://codereview.chromium.org/131503006/diff/1330001/media/audio/android/au... media/audio/android/audio_manager_android.cc:292: if (!j_audio_manager_.is_null()) { On 2014/02/18 13:32:07, bulach wrote: > nit: could remove braces (similar to 276 and 318) Done. https://codereview.chromium.org/131503006/diff/1330001/media/audio/android/au... media/audio/android/audio_manager_android.cc:302: Java_AudioManagerAndroid_createAudioManagerAndroid( On 2014/02/18 13:32:07, bulach wrote: > nit: indent by another 2, and then the following lines as well Done. https://codereview.chromium.org/131503006/diff/1330001/media/base/android/jav... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/131503006/diff/1330001/media/base/android/jav... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:50: private Long mThreadId = 0L; Thanks. I had added a detach method first as well (and then the ensure-call was needed). Did as you suggested. https://codereview.chromium.org/131503006/diff/1330001/media/base/android/jav... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:220: * init() to the constructor and code in close() to finalize(). Thanks for pointing that out; I have been warned about using Java finalize. I removed the comment since we are good as is. No idea to make it sound as if we need more improvements at this stage. https://codereview.chromium.org/131503006/diff/1330001/media/base/android/jav... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:515: Log.wtf(TAG,"Method is not called on valid thread!"); impressive ;-)
noticed one tiny little thing when checking the latest patch set. https://codereview.chromium.org/131503006/diff/1200004/media/base/android/jav... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/131503006/diff/1200004/media/base/android/jav... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:54: if (mThreadId == 0L) mThreadId = Thread.currentThread().getId(); nit: just do: mThreadId = Thread.currentThread().getId();
https://codereview.chromium.org/131503006/diff/1200004/media/base/android/jav... File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java (right): https://codereview.chromium.org/131503006/diff/1200004/media/base/android/jav... media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:54: if (mThreadId == 0L) mThreadId = Thread.currentThread().getId(); On 2014/02/18 14:30:31, tommi wrote: > nit: just do: mThreadId = Thread.currentThread().getId(); Done.
lgtm, thanks!!
https://codereview.chromium.org/131503006/diff/1260003/media/audio/android/au... File media/audio/android/audio_manager_android.cc (right): https://codereview.chromium.org/131503006/diff/1260003/media/audio/android/au... media/audio/android/audio_manager_android.cc:256: CreateAndInitOnAudioThread(); You mention in the description that you wanted to make construction less heavy. Can you elaborate on that (ideally in the description of the CL or in a bug)? Specifically I'm wondering if your intent is to minimize the work on the UI thread or to always defer AM construction until necessary (and if so, why?). Now that you're using the audio thread for everything my wonder is that you should just always post the initialization task to the audio thread during construction like we do on other platforms. Doing so simplifies the code here significantly by not having to run this task in every method (as well as not running the risk that future methods forget to do so). https://codereview.chromium.org/131503006/diff/1260003/media/audio/android/au... media/audio/android/audio_manager_android.cc:284: void AudioManagerAndroid::WillDestroyCurrentMessageLoop() { Regardless of the outcome from above, instead of using a message loop observer can you just post CloseOnAudioThread() prior to calling Shutdown() in the destructor? That's how we handle this on other platforms. https://codereview.chromium.org/131503006/diff/1260003/media/audio/android/au... media/audio/android/audio_manager_android.cc:327: void AudioManagerAndroid::Init() { These two methods are only called in one place each. It'd be better just to merge them into the relevant callers.
Comments only. Tommi@: are you OK with the proposal from Dale; that I do init on audio thread in the ctor as on other platforms? And remove the "on-first-use-init" scheme we have so far. https://codereview.chromium.org/131503006/diff/1260003/media/audio/android/au... File media/audio/android/audio_manager_android.cc (right): https://codereview.chromium.org/131503006/diff/1260003/media/audio/android/au... media/audio/android/audio_manager_android.cc:256: CreateAndInitOnAudioThread(); It started out with the crash reported here: https://codereview.chromium.org/152783005 where I initially had most parts of this CL included in my fix. Marcus felt that it was too much and I did a more isolated patch and kept this one as a separate "generic improvement CL". The idea that we had was that we wanted to be able to start Chrome without accessing any audio hardware (check if BT device exists, check if wireless headset exists etc.) and wait with populating the list until it was needed for the first time. On other OS:es, enumeration is done by the OS when we ask for it and I wanted to do the same here. When minimizing the work at startup, we realized that the threading model was complicated and we might as well solve that as well. Goal is to clean things up to avoid synchronization in the Java layer. I could post the init task at construction but then I would always trigger the enumeration (check if BT is enabled etc) and I would like to keep it as simple as possible. But yes, given that we are now on the audio thread, I guess that is possible. Do you feel that it is a requirement from your side? tommi@: what's your opinion? https://codereview.chromium.org/131503006/diff/1260003/media/audio/android/au... media/audio/android/audio_manager_android.cc:284: void AudioManagerAndroid::WillDestroyCurrentMessageLoop() { Will harmonize. Thanks for pointing that out. https://codereview.chromium.org/131503006/diff/1260003/media/audio/android/au... media/audio/android/audio_manager_android.cc:327: void AudioManagerAndroid::Init() { Will fix.
And .... I forgot about all other media unittests which uses the new AM and therefore must call on the audio thread :-(
https://codereview.chromium.org/131503006/diff/1260003/media/audio/android/au... File media/audio/android/audio_manager_android.cc (right): https://codereview.chromium.org/131503006/diff/1260003/media/audio/android/au... media/audio/android/audio_manager_android.cc:256: CreateAndInitOnAudioThread(); On 2014/02/19 10:37:01, henrika wrote: > It started out with the crash reported here: > https://codereview.chromium.org/152783005 where I initially had most parts of > this CL included in my fix. Marcus felt that it was too much and I did a more > isolated patch and kept this one as a separate "generic improvement CL". > > The idea that we had was that we wanted to be able to start Chrome without > accessing any audio hardware (check if BT device exists, check if wireless > headset exists etc.) and wait with populating the list until it was needed for > the first time. On other OS:es, enumeration is done by the OS when we ask for it > and I wanted to do the same here. > > When minimizing the work at startup, we realized that the threading model was > complicated and we might as well solve that as well. Goal is to clean things up > to avoid synchronization in the Java layer. > > I could post the init task at construction but then I would always trigger the > enumeration (check if BT is enabled etc) and I would like to keep it as simple > as possible. > > But yes, given that we are now on the audio thread, I guess that is possible. > > Do you feel that it is a requirement from your side? > > tommi@: what's your opinion? Yes, I think that's (Dale's suggestion) a good idea.
Done changed based on feedback from Dale. PTAL. Will take a look at the other failing unittests next. https://codereview.chromium.org/131503006/diff/1260003/media/audio/android/au... File media/audio/android/audio_manager_android.cc (right): https://codereview.chromium.org/131503006/diff/1260003/media/audio/android/au... media/audio/android/audio_manager_android.cc:256: CreateAndInitOnAudioThread(); On 2014/02/19 10:48:35, tommi wrote: > On 2014/02/19 10:37:01, henrika wrote: > > It started out with the crash reported here: > > https://codereview.chromium.org/152783005 where I initially had most parts of > > this CL included in my fix. Marcus felt that it was too much and I did a more > > isolated patch and kept this one as a separate "generic improvement CL". > > > > The idea that we had was that we wanted to be able to start Chrome without > > accessing any audio hardware (check if BT device exists, check if wireless > > headset exists etc.) and wait with populating the list until it was needed for > > the first time. On other OS:es, enumeration is done by the OS when we ask for > it > > and I wanted to do the same here. > > > > When minimizing the work at startup, we realized that the threading model was > > complicated and we might as well solve that as well. Goal is to clean things > up > > to avoid synchronization in the Java layer. > > > > I could post the init task at construction but then I would always trigger the > > enumeration (check if BT is enabled etc) and I would like to keep it as simple > > as possible. > > > > But yes, given that we are now on the audio thread, I guess that is possible. > > > > Do you feel that it is a requirement from your side? > > > > tommi@: what's your opinion? > > Yes, I think that's (Dale's suggestion) a good idea. Done. https://codereview.chromium.org/131503006/diff/1260003/media/audio/android/au... media/audio/android/audio_manager_android.cc:284: void AudioManagerAndroid::WillDestroyCurrentMessageLoop() { On 2014/02/19 10:37:01, henrika wrote: > Will harmonize. Thanks for pointing that out. Done.
lgtm % q + nits. https://codereview.chromium.org/131503006/diff/1730002/media/audio/android/au... File media/audio/android/audio_manager_android.cc (right): https://codereview.chromium.org/131503006/diff/1730002/media/audio/android/au... media/audio/android/audio_manager_android.cc:66: GetTaskRunner()->PostTask(FROM_HERE, base::Bind( I'm not sure if this is a problem or not, but note that AudioManagerBase::ShutdownOnAudioThread() will run after this, so you need to ensure that method (which just closes dispatchers really), doesn't do anything which might invoke the Java side code after this point. For perspective, you might be interested in this CL https://codereview.chromium.org/148373004/ where I try to early abort methods which execute on the audio thread after this point (which does happen regularly). https://codereview.chromium.org/131503006/diff/1730002/media/audio/android/au... media/audio/android/audio_manager_android.cc:118: JNIEnv* env = AttachCurrentThread(); Check can probably be removed since you're ensuring you're on the audio thread (here and elsewhere). Up to you though. https://codereview.chromium.org/131503006/diff/1730002/media/audio/android/au... File media/audio/android/audio_manager_android.h (right): https://codereview.chromium.org/131503006/diff/1730002/media/audio/android/au... media/audio/android/audio_manager_android.h:21: class MEDIA_EXPORT AudioManagerAndroid One line.
You should also update the CL description now; since this does something different. Thanks for your patience and work!
https://codereview.chromium.org/131503006/diff/1730002/media/audio/android/au... File media/audio/android/audio_manager_android.cc (right): https://codereview.chromium.org/131503006/diff/1730002/media/audio/android/au... media/audio/android/audio_manager_android.cc:66: GetTaskRunner()->PostTask(FROM_HERE, base::Bind( I am unable to see that it should be a problem but thanks for pointing it out. Might have to revisit if we see issues related to closing down on Android. https://codereview.chromium.org/131503006/diff/1730002/media/audio/android/au... media/audio/android/audio_manager_android.cc:118: JNIEnv* env = AttachCurrentThread(); On 2014/02/19 19:10:16, DaleCurtis wrote: > Check can probably be removed since you're ensuring you're on the audio thread > (here and elsewhere). Up to you though. Done. https://codereview.chromium.org/131503006/diff/1730002/media/audio/android/au... File media/audio/android/audio_manager_android.h (right): https://codereview.chromium.org/131503006/diff/1730002/media/audio/android/au... media/audio/android/audio_manager_android.h:21: class MEDIA_EXPORT AudioManagerAndroid On 2014/02/19 19:10:16, DaleCurtis wrote: > One line. Done.
tommi@: one more? Please note that I decided to do https://codereview.chromium.org/131503006/diff2/2230001:2250001/media/audio/a... since my more strict thread checking spread to content_unittests as well. All threading in Chrome is OK, we are only talking about one unit test which does not comply with the special requirements for Android.
sky@: would really appreciate an OK from you as owner in content/browser
All I do is to wrap one message to the audio thread to match underlying thread
restrictions.
** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
content/browser/renderer_host/render_message_filter.cc
lgtm. https://codereview.chromium.org/131503006/diff/2560001/media/audio/android/au... File media/audio/android/audio_manager_android.cc (right): https://codereview.chromium.org/131503006/diff/2560001/media/audio/android/au... media/audio/android/audio_manager_android.cc:361: } else { nit: just remove the else. if (IsAudioLowLatencySupported()) return GetAudioLowLatencyOutputFrameSize(); return std::max(...); https://codereview.chromium.org/131503006/diff/2560001/media/audio/audio_inpu... File media/audio/audio_input_unittest.cc (right): https://codereview.chromium.org/131503006/diff/2560001/media/audio/audio_inpu... media/audio/audio_input_unittest.cc:57: virtual ~AudioInputTest() { nit: {} (since you're already using that style for a getter like audio_manager() that has more lines of code) https://codereview.chromium.org/131503006/diff/2560001/media/audio/audio_inpu... media/audio/audio_input_unittest.cc:66: LOG(WARNING) << "No input devices detected"; nit: use LOG_IF(WARNING, !has_input)
https://codereview.chromium.org/131503006/diff/2560001/media/audio/android/au... File media/audio/android/audio_manager_android.cc (right): https://codereview.chromium.org/131503006/diff/2560001/media/audio/android/au... media/audio/android/audio_manager_android.cc:361: } else { On 2014/02/21 16:10:30, tommi wrote: > nit: just remove the else. > > if (IsAudioLowLatencySupported()) > return GetAudioLowLatencyOutputFrameSize(); > > return std::max(...); Done. https://codereview.chromium.org/131503006/diff/2560001/media/audio/audio_inpu... File media/audio/audio_input_unittest.cc (right): https://codereview.chromium.org/131503006/diff/2560001/media/audio/audio_inpu... media/audio/audio_input_unittest.cc:57: virtual ~AudioInputTest() { On 2014/02/21 16:10:30, tommi wrote: > nit: {} > (since you're already using that style for a getter like audio_manager() that > has more lines of code) Done. https://codereview.chromium.org/131503006/diff/2560001/media/audio/audio_inpu... media/audio/audio_input_unittest.cc:66: LOG(WARNING) << "No input devices detected"; On 2014/02/21 16:10:30, tommi wrote: > nit: use LOG_IF(WARNING, !has_input) Done.
LGTM
The CQ bit was checked by tommi@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/henrika@chromium.org/131503006/2450002
The CQ bit was unchecked by commit-bot@chromium.org
No LGTM from a valid reviewer yet. Only full committers are accepted. Even if an LGTM may have been provided, it was from a non-committer or a lowly provisional committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
The CQ bit was unchecked by commit-bot@chromium.org
No LGTM from a valid reviewer yet. Only full committers are accepted. Even if an LGTM may have been provided, it was from a non-committer or a lowly provisional committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
The CQ bit was checked by sergeyberezin@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/henrika@chromium.org/131503006/2450002
Message was sent while issue was closed.
Change committed as 252614
Message was sent while issue was closed.
Thanks all! I think I had OK from valid reviewers. |
