Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9655023:
Adding input and output audio backend to Android. (Closed)
|
Created:
8 years, 9 months ago by no longer working on chromium Modified:
8 years, 8 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdding input and output audio backend to Android.
This CL will totally add 117.2K bytes in size:
Without this CL, media_unittests binary has size 30798296;
with this CL, it increase to 30918316.
BUG=116803
TEST=modified media_unittests
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=130174
Patch Set 1 #Patch Set 2 : a new version #Patch Set 3 : clean up the code and ready for review #
Total comments: 37
Patch Set 4 : addressed Tommi's comments #
Total comments: 18
Patch Set 5 : updated description and addressed Tommi's second round comments #
Total comments: 46
Patch Set 6 : #
Total comments: 4
Patch Set 7 : addressed Tommi's comments && reduced the binary size by not using vector #
Total comments: 30
Patch Set 8 : rebased && addressed qinmin's comments #
Total comments: 8
Patch Set 9 : addressed minqin's comments #
Messages
Total messages: 18 (0 generated)
Adding the input and output audio backend to Android. Satish, please point me to someone else you think others is more suitable for this review. Thanks, BR, /SX
first set of comments. Thanks for doing this! :) https://chromiumcodereview.appspot.com/9655023/diff/4013/media/audio/android/... File media/audio/android/opensles_input.cc (right): https://chromiumcodereview.appspot.com/9655023/diff/4013/media/audio/android/... media/audio/android/opensles_input.cc:40: OpenSLESInputStream::~OpenSLESInputStream() { DCHECK the state of all the allocated objects. I assume that it's a requrement that recorder_object_, recorder_, engine_object_, all buffers etc should all have been deleted at this point. https://chromiumcodereview.appspot.com/9655023/diff/4013/media/audio/android/... media/audio/android/opensles_input.cc:54: DCHECK(recorder_ && simple_buffer_queue_); make these separate dchecks so we'll know which condition isn't met https://chromiumcodereview.appspot.com/9655023/diff/4013/media/audio/android/... media/audio/android/opensles_input.cc:61: started_ = true; should we set this when we know we haven't hit any errors? https://chromiumcodereview.appspot.com/9655023/diff/4013/media/audio/android/... media/audio/android/opensles_input.cc:71: HandleError(err); no break or return? https://chromiumcodereview.appspot.com/9655023/diff/4013/media/audio/android/... media/audio/android/opensles_input.cc:75: err = (*recorder_)->SetRecordState(recorder_, SL_RECORDSTATE_RECORDING); is it ok to call this if Enqueue failed? https://chromiumcodereview.appspot.com/9655023/diff/4013/media/audio/android/... media/audio/android/opensles_input.cc:138: static_cast<SLuint32>(SL_ENGINEOPTION_THREADSAFE), cast necessary? https://chromiumcodereview.appspot.com/9655023/diff/4013/media/audio/android/... media/audio/android/opensles_input.cc:138: static_cast<SLuint32>(SL_ENGINEOPTION_THREADSAFE), 4 space indent feels strange for {} . https://chromiumcodereview.appspot.com/9655023/diff/4013/media/audio/android/... media/audio/android/opensles_input.cc:139: static_cast<SLuint32>(SL_BOOLEAN_TRUE) }}; move } to next line https://chromiumcodereview.appspot.com/9655023/diff/4013/media/audio/android/... media/audio/android/opensles_input.cc:162: SL_DEFAULTDEVICEID_AUDIOINPUT, NULL}; keep the closing } on the next line like is done below. https://chromiumcodereview.appspot.com/9655023/diff/4013/media/audio/android/... media/audio/android/opensles_input.cc:223: if (SL_RESULT_SUCCESS != err) return SL_RESULT_SUCCESS != err; https://chromiumcodereview.appspot.com/9655023/diff/4013/media/audio/android/... media/audio/android/opensles_input.cc:230: SLAndroidSimpleBufferQueueItf buffer_queue, void *instance) { void* instance (check for this elsewhere as well) https://chromiumcodereview.appspot.com/9655023/diff/4013/media/audio/android/... media/audio/android/opensles_input.cc:232: reinterpret_cast<OpenSLESInputStream*> (instance); no space after > https://chromiumcodereview.appspot.com/9655023/diff/4013/media/audio/android/... media/audio/android/opensles_input.cc:257: remove extra empty line https://chromiumcodereview.appspot.com/9655023/diff/4013/media/audio/android/... media/audio/android/opensles_input.cc:259: NOTREACHED() << "OpenSLES error " << error; this may be stretching the point of 'NOTREACHED' since it is meant for code that should never execute. DLOG(FATAL)? https://chromiumcodereview.appspot.com/9655023/diff/4013/media/audio/android/... File media/audio/android/opensles_output.cc (right): https://chromiumcodereview.appspot.com/9655023/diff/4013/media/audio/android/... media/audio/android/opensles_output.cc:44: for (size_t i = 0; i < audio_data_.size(); ++i) DCHECK first that all objects have been properly freed. https://chromiumcodereview.appspot.com/9655023/diff/4013/media/audio/android/... File media/audio/android/opensles_output.h (right): https://chromiumcodereview.appspot.com/9655023/diff/4013/media/audio/android/... media/audio/android/opensles_output.h:22: explicit OpenSLESOutputStream(AudioManagerAndroid* manager, only use explicit for single argument constructors https://chromiumcodereview.appspot.com/9655023/diff/4013/media/audio/android/... media/audio/android/opensles_output.h:39: SLAndroidSimpleBufferQueueItf buffer_queue, void *instance); void* instance https://chromiumcodereview.appspot.com/9655023/diff/4013/media/audio/android/... media/audio/android/opensles_output.h:51: SLObjectItf engine_object_; // Shared engine interface for the application. since these objects are used in more than one class and require special cleanup code, perhaps a smart pointer is in order? E.g. something along the lines of: template <typename SLType> class ScopedSLObject { public: ScopedSLObject() : obj_(NULL) {} ~ScopedSLObject() { Reset(); } // Use to invoke methods (foo->Bar() instead of (*foo)->Bar()) operator()->() { return *obj_; } SLType* Receive() { DCHECK(!obj_); return &obj_; } void Reset() { if (obj_) (*obj_)->Destroy(obj_); obj_ = NULL; } private: SLType obj_; }; well, you get the point :) https://chromiumcodereview.appspot.com/9655023/diff/4013/media/audio/android/... media/audio/android/opensles_output.h:71: float volume_; we use double elsewhere - is float a requirement of the api?
Please take a second look. https://chromiumcodereview.appspot.com/9655023/diff/4013/media/audio/android/... File media/audio/android/opensles_input.cc (right): https://chromiumcodereview.appspot.com/9655023/diff/4013/media/audio/android/... media/audio/android/opensles_input.cc:40: OpenSLESInputStream::~OpenSLESInputStream() { On 2012/03/17 12:24:57, tommi wrote: > DCHECK the state of all the allocated objects. I assume that it's a requrement > that recorder_object_, recorder_, engine_object_, all buffers etc should all > have been deleted at this point. Done. https://chromiumcodereview.appspot.com/9655023/diff/4013/media/audio/android/... media/audio/android/opensles_input.cc:54: DCHECK(recorder_ && simple_buffer_queue_); On 2012/03/17 12:24:57, tommi wrote: > make these separate dchecks so we'll know which condition isn't met Done. https://chromiumcodereview.appspot.com/9655023/diff/4013/media/audio/android/... media/audio/android/opensles_input.cc:61: started_ = true; On 2012/03/17 12:24:57, tommi wrote: > should we set this when we know we haven't hit any errors? No, we need to set the flag here because: 1, we need to ensure we will feed data to the buffer when callback comes, and in ReadBufferQueue(), we always check this started_ flag. 2, we should allow calling Stop() even Start() fails in some operation. https://chromiumcodereview.appspot.com/9655023/diff/4013/media/audio/android/... media/audio/android/opensles_input.cc:71: HandleError(err); On 2012/03/17 12:24:57, tommi wrote: > no break or return? The most likely error is SL_RESULT_BUFFER_INSUFFICIENT, and I was not sure if this should be treated as an error or just a warning. But you are right, we should not continue here since we call HandleError, I added a return. https://chromiumcodereview.appspot.com/9655023/diff/4013/media/audio/android/... media/audio/android/opensles_input.cc:75: err = (*recorder_)->SetRecordState(recorder_, SL_RECORDSTATE_RECORDING); On 2012/03/17 12:24:57, tommi wrote: > is it ok to call this if Enqueue failed? Maybe it is OK, but we should not. https://chromiumcodereview.appspot.com/9655023/diff/4013/media/audio/android/... media/audio/android/opensles_input.cc:138: static_cast<SLuint32>(SL_ENGINEOPTION_THREADSAFE), On 2012/03/17 12:24:57, tommi wrote: > cast necessary? They are from Android sample code. The first one is likely unnecessary since we have #define SL_ENGINEOPTION_THREADSAFE ((SLuint32) 0x00000001). I removed it. https://chromiumcodereview.appspot.com/9655023/diff/4013/media/audio/android/... media/audio/android/opensles_input.cc:138: static_cast<SLuint32>(SL_ENGINEOPTION_THREADSAFE), On 2012/03/17 12:24:57, tommi wrote: > 4 space indent feels strange for {} . Done. https://chromiumcodereview.appspot.com/9655023/diff/4013/media/audio/android/... media/audio/android/opensles_input.cc:139: static_cast<SLuint32>(SL_BOOLEAN_TRUE) }}; On 2012/03/17 12:24:57, tommi wrote: > move } to next line Done. https://chromiumcodereview.appspot.com/9655023/diff/4013/media/audio/android/... media/audio/android/opensles_input.cc:162: SL_DEFAULTDEVICEID_AUDIOINPUT, NULL}; On 2012/03/17 12:24:57, tommi wrote: > keep the closing } on the next line like is done below. Done. https://chromiumcodereview.appspot.com/9655023/diff/4013/media/audio/android/... media/audio/android/opensles_input.cc:223: if (SL_RESULT_SUCCESS != err) On 2012/03/17 12:24:57, tommi wrote: > return SL_RESULT_SUCCESS != err; Done. https://chromiumcodereview.appspot.com/9655023/diff/4013/media/audio/android/... media/audio/android/opensles_input.cc:230: SLAndroidSimpleBufferQueueItf buffer_queue, void *instance) { On 2012/03/17 12:24:57, tommi wrote: > void* instance > (check for this elsewhere as well) Done. https://chromiumcodereview.appspot.com/9655023/diff/4013/media/audio/android/... media/audio/android/opensles_input.cc:232: reinterpret_cast<OpenSLESInputStream*> (instance); On 2012/03/17 12:24:57, tommi wrote: > no space after > Done. https://chromiumcodereview.appspot.com/9655023/diff/4013/media/audio/android/... media/audio/android/opensles_input.cc:257: On 2012/03/17 12:24:57, tommi wrote: > remove extra empty line Done. https://chromiumcodereview.appspot.com/9655023/diff/4013/media/audio/android/... media/audio/android/opensles_input.cc:259: NOTREACHED() << "OpenSLES error " << error; On 2012/03/17 12:24:57, tommi wrote: > this may be stretching the point of 'NOTREACHED' since it is meant for code that > should never execute. DLOG(FATAL)? Thanks, it is a copy code from audio_low_latency_input_mac.cc. But DLOG(FATLE) sounds more correct. https://chromiumcodereview.appspot.com/9655023/diff/4013/media/audio/android/... File media/audio/android/opensles_output.h (right): https://chromiumcodereview.appspot.com/9655023/diff/4013/media/audio/android/... media/audio/android/opensles_output.h:22: explicit OpenSLESOutputStream(AudioManagerAndroid* manager, On 2012/03/17 12:24:57, tommi wrote: > only use explicit for single argument constructors Thanks, good to know. https://chromiumcodereview.appspot.com/9655023/diff/4013/media/audio/android/... media/audio/android/opensles_output.h:39: SLAndroidSimpleBufferQueueItf buffer_queue, void *instance); On 2012/03/17 12:24:57, tommi wrote: > void* instance Done. https://chromiumcodereview.appspot.com/9655023/diff/4013/media/audio/android/... media/audio/android/opensles_output.h:51: SLObjectItf engine_object_; // Shared engine interface for the application. On 2012/03/17 12:24:57, tommi wrote: > since these objects are used in more than one class and require special cleanup > code, perhaps a smart pointer is in order? E.g. something along the lines of: > > template <typename SLType> > class ScopedSLObject { > public: > ScopedSLObject() : obj_(NULL) {} > ~ScopedSLObject() { > Reset(); > } > // Use to invoke methods (foo->Bar() instead of (*foo)->Bar()) > operator()->() { return *obj_; } > SLType* Receive() { > DCHECK(!obj_); > return &obj_; > } > > void Reset() { > if (obj_) > (*obj_)->Destroy(obj_); > obj_ = NULL; > } > private: > SLType obj_; > }; > > well, you get the point :) Done. https://chromiumcodereview.appspot.com/9655023/diff/4013/media/audio/android/... media/audio/android/opensles_output.h:71: float volume_; On 2012/03/17 12:24:57, tommi wrote: > we use double elsewhere - is float a requirement of the api? Good question, we use float for all platforms, but simply because function AdjustVolume() needs it to be float. I think both double and float work, and using float is to be consistent with other platform implementation.
https://chromiumcodereview.appspot.com/9655023/diff/16004/media/audio/android... File media/audio/android/opensles_input.cc (right): https://chromiumcodereview.appspot.com/9655023/diff/16004/media/audio/android... media/audio/android/opensles_input.cc:71: reinterpret_cast<void*>(audio_data_[i]), is the cast necessary? https://chromiumcodereview.appspot.com/9655023/diff/16004/media/audio/android... media/audio/android/opensles_input.cc:75: return; bug https://chromiumcodereview.appspot.com/9655023/diff/16004/media/audio/android... File media/audio/android/opensles_input.h (right): https://chromiumcodereview.appspot.com/9655023/diff/16004/media/audio/android... media/audio/android/opensles_input.h:39: SLAndroidSimpleBufferQueueItf buffer_queue, void *instance); fix void * https://chromiumcodereview.appspot.com/9655023/diff/16004/media/audio/android... File media/audio/android/opensles_output.cc (right): https://chromiumcodereview.appspot.com/9655023/diff/16004/media/audio/android... media/audio/android/opensles_output.cc:206: err = (*player_object_.Get())->Realize(player_object_.Get(), s/(*player_object_.Get())->/player_object_-> https://chromiumcodereview.appspot.com/9655023/diff/16004/media/audio/android... media/audio/android/opensles_output.cc:232: err = (*simple_buffer_queue_)->RegisterCallback(simple_buffer_queue_, same here? https://chromiumcodereview.appspot.com/9655023/diff/16004/media/audio/android... File media/audio/android/opensles_util.h (right): https://chromiumcodereview.appspot.com/9655023/diff/16004/media/audio/android... media/audio/android/opensles_util.h:18: ~ScopedSLObject() { nit: one liner https://chromiumcodereview.appspot.com/9655023/diff/16004/media/audio/android... media/audio/android/opensles_util.h:27: SLDerefType operator->() { nit: one liner (same for Get()) https://chromiumcodereview.appspot.com/9655023/diff/16004/media/audio/audio_l... File media/audio/audio_low_latency_input_output_unittest.cc (right): https://chromiumcodereview.appspot.com/9655023/diff/16004/media/audio/audio_l... media/audio/audio_low_latency_input_output_unittest.cc:348: } fix
https://chromiumcodereview.appspot.com/9655023/diff/16004/media/media.gyp File media/media.gyp (right): https://chromiumcodereview.appspot.com/9655023/diff/16004/media/media.gyp#new... media/media.gyp:324: '-lOpenSLES', Could you find how much binary size increases due to this new dependency?
Changed the code based on the comments from Tommi and also updated the description on how much this CL adds to the binary size. BR, /SX https://chromiumcodereview.appspot.com/9655023/diff/16004/media/audio/android... File media/audio/android/opensles_input.cc (right): https://chromiumcodereview.appspot.com/9655023/diff/16004/media/audio/android... media/audio/android/opensles_input.cc:71: reinterpret_cast<void*>(audio_data_[i]), On 2012/03/22 15:01:26, tommi wrote: > is the cast necessary? Done. https://chromiumcodereview.appspot.com/9655023/diff/16004/media/audio/android... media/audio/android/opensles_input.cc:75: return; On 2012/03/22 15:01:26, tommi wrote: > bug Done. https://chromiumcodereview.appspot.com/9655023/diff/16004/media/audio/android... File media/audio/android/opensles_input.h (right): https://chromiumcodereview.appspot.com/9655023/diff/16004/media/audio/android... media/audio/android/opensles_input.h:39: SLAndroidSimpleBufferQueueItf buffer_queue, void *instance); On 2012/03/22 15:01:26, tommi wrote: > fix void * Done. https://chromiumcodereview.appspot.com/9655023/diff/16004/media/audio/android... File media/audio/android/opensles_output.cc (right): https://chromiumcodereview.appspot.com/9655023/diff/16004/media/audio/android... media/audio/android/opensles_output.cc:206: err = (*player_object_.Get())->Realize(player_object_.Get(), On 2012/03/22 15:01:26, tommi wrote: > s/(*player_object_.Get())->/player_object_-> Done. https://chromiumcodereview.appspot.com/9655023/diff/16004/media/audio/android... media/audio/android/opensles_output.cc:232: err = (*simple_buffer_queue_)->RegisterCallback(simple_buffer_queue_, On 2012/03/22 15:01:26, tommi wrote: > same here? We can't do this here, simple_buffer_queue_ is not using the ScopedSLObject https://chromiumcodereview.appspot.com/9655023/diff/16004/media/audio/audio_l... File media/audio/audio_low_latency_input_output_unittest.cc (right): https://chromiumcodereview.appspot.com/9655023/diff/16004/media/audio/audio_l... media/audio/audio_low_latency_input_output_unittest.cc:348: } On 2012/03/22 15:01:26, tommi wrote: > fix Done. https://chromiumcodereview.appspot.com/9655023/diff/16004/media/media.gyp File media/media.gyp (right): https://chromiumcodereview.appspot.com/9655023/diff/16004/media/media.gyp#new... media/media.gyp:324: '-lOpenSLES', On 2012/03/22 15:24:34, Satish wrote: > Could you find how much binary size increases due to this new dependency? Totally 117.2K in binary for release mode.
https://chromiumcodereview.appspot.com/9655023/diff/16005/media/audio/android... File media/audio/android/opensles_input.cc (right): https://chromiumcodereview.appspot.com/9655023/diff/16005/media/audio/android... media/audio/android/opensles_input.cc:23: format_.samplesPerSec = static_cast<SLuint32>(params.sample_rate() * 1000); Can you add a comment here that explains that 'samplesPerSec' should really be 'samplesPer1000Sec' or 'samplesInMilliHz' so that other readers won't be confused by this as I was? https://chromiumcodereview.appspot.com/9655023/diff/16005/media/audio/android... media/audio/android/opensles_input.cc:74: HandleError(err); If we get here, 'started_' and all the other properties will be in a bad state, right? should we reset those or perhaps only set started_ to true (etc) below, when we know that we started successfully? https://chromiumcodereview.appspot.com/9655023/diff/16005/media/audio/android... media/audio/android/opensles_input.cc:94: LOG(WARNING) << "SetRecordState() failed to set the state to stop"; DLOG https://chromiumcodereview.appspot.com/9655023/diff/16005/media/audio/android... media/audio/android/opensles_input.cc:100: LOG(WARNING) << "Clear() failed to clear the buffer queue"; DLOG You could also use DLOG_IF https://chromiumcodereview.appspot.com/9655023/diff/16005/media/audio/android... media/audio/android/opensles_input.cc:110: // Explicitly Free the player objects and invalidate their associated s/Free/free https://chromiumcodereview.appspot.com/9655023/diff/16005/media/audio/android... media/audio/android/opensles_input.cc:111: // interfaces. They have to be done in order. s/done in order/done in the correct order https://chromiumcodereview.appspot.com/9655023/diff/16005/media/audio/android... media/audio/android/opensles_input.cc:121: return 0.0; should this be 1.0? https://chromiumcodereview.appspot.com/9655023/diff/16005/media/audio/android... media/audio/android/opensles_input.cc:125: } NOTIMPLEMENTED? https://chromiumcodereview.appspot.com/9655023/diff/16005/media/audio/android... media/audio/android/opensles_input.cc:166: SLDataSource audio_source = { reinterpret_cast<void*>(&mic_locator), NULL }; cast necessary? https://chromiumcodereview.appspot.com/9655023/diff/16005/media/audio/android... media/audio/android/opensles_input.cc:174: reinterpret_cast<void*>(&buffer_queue), cast necessary? (and below) https://chromiumcodereview.appspot.com/9655023/diff/16005/media/audio/android... media/audio/android/opensles_input.cc:179: const SLuint32 number_of_interface = 1; number_of_interfaces https://chromiumcodereview.appspot.com/9655023/diff/16005/media/audio/android... media/audio/android/opensles_input.cc:180: const SLInterfaceID interface_id[] = { SL_IID_ANDROIDSIMPLEBUFFERQUEUE }; nit: can you use number_of_interfaces as the array size? i.e. const SLInterfaceID interface_id[number_of_interfaces] = ... https://chromiumcodereview.appspot.com/9655023/diff/16005/media/audio/android... media/audio/android/opensles_input.cc:181: const SLboolean interface_required[] = { SL_BOOLEAN_TRUE }; and here https://chromiumcodereview.appspot.com/9655023/diff/16005/media/audio/android... media/audio/android/opensles_input.cc:188: interface_required); actually, if you're only ever asking for a single interface, then you don't have to use arrays: SLInterfaceID interface_id = SL_IID_ANDROIDSIMPLEBUFFERQUEUE; SLboolean interface_required = SL_BOOLEAN_TRUE; err = (*engine)->CreateAudioRecorder(engine, recorder_object_.Receive(), &audio_source, &audio_sink, 1, &interface_id, &interface_required); https://chromiumcodereview.appspot.com/9655023/diff/16005/media/audio/android... media/audio/android/opensles_input.cc:191: LOG(ERROR) << "CreateAudioRecorder failed with error code " << err; logspam. same for the rest. https://chromiumcodereview.appspot.com/9655023/diff/16005/media/audio/android... media/audio/android/opensles_input.cc:199: LOG(ERROR) << "Recprder Realize() failed with error code " << err; fix typo https://chromiumcodereview.appspot.com/9655023/diff/16005/media/audio/android... File media/audio/android/opensles_input.h (right): https://chromiumcodereview.appspot.com/9655023/diff/16005/media/audio/android... media/audio/android/opensles_input.h:55: // Recorder interfaces. s/interfaces/interface I don't think this comment adds much though, so you could also skip it. https://chromiumcodereview.appspot.com/9655023/diff/16005/media/audio/android... File media/audio/android/opensles_output.cc (right): https://chromiumcodereview.appspot.com/9655023/diff/16005/media/audio/android... media/audio/android/opensles_output.cc:25: format_.samplesPerSec = static_cast<SLuint32>(params.sample_rate() * 1000); same comment here as in the other place. https://chromiumcodereview.appspot.com/9655023/diff/16005/media/audio/android... media/audio/android/opensles_output.cc:104: // Explicitly Free the player objects and invalidate their associated fix up comments here as in the other file. https://chromiumcodereview.appspot.com/9655023/diff/16005/media/audio/android... media/audio/android/opensles_output.cc:174: static_cast<SLuint32>(kNumOfQueuesInBuffer) }; }; on the next line. also fix throughout if there are more cases of this. https://chromiumcodereview.appspot.com/9655023/diff/16005/media/audio/android... media/audio/android/opensles_output.cc:273: HandleError(err); also return? or is it OK to update active_queue_ if we failed? https://chromiumcodereview.appspot.com/9655023/diff/16005/media/audio/android... File media/audio/android/opensles_output.h (right): https://chromiumcodereview.appspot.com/9655023/diff/16005/media/audio/android... media/audio/android/opensles_output.h:72: float volume_; not sure if I already asked, but why don't we use double here like we do elsewhere? https://chromiumcodereview.appspot.com/9655023/diff/16005/media/audio/android... File media/audio/android/opensles_util.h (right): https://chromiumcodereview.appspot.com/9655023/diff/16005/media/audio/android... media/audio/android/opensles_util.h:18: ~ScopedSLObject() { my previous comments haven't been addressed
Another set of code to address Tommi's comment. FYI, the reason why we can't do recording is because the permission. From the logcat in Android: W/ServiceManager( 120): Permission failure: android.permission.MODIFY_AUDIO_SETTINGS from uid=2000 pid=11003 E/AudioFlinger( 120): Request requires android.permission.MODIFY_AUDIO_SETTINGS E/AudioEffect(11003): set(): AudioFlinger could not create effect, status: -1 E/libOpenSLES(11003): Effect initCheck() returned -1 E/libOpenSLES(11003): EnvironmentalReverb effect initialization failed W/ServiceManager( 120): Permission failure: android.permission.RECORD_AUDIO from uid=2000 pid=11003 E/AudioFlinger( 120): Request requires android.permission.RECORD_AUDIO And I talked to the Android team, they said that we needed to create a .apk in order to get permission, which is not an easy job, I will look into it later. BR, /SX https://chromiumcodereview.appspot.com/9655023/diff/16004/media/audio/android... File media/audio/android/opensles_util.h (right): https://chromiumcodereview.appspot.com/9655023/diff/16004/media/audio/android... media/audio/android/opensles_util.h:18: ~ScopedSLObject() { On 2012/03/22 15:01:26, tommi wrote: > nit: one liner Done. https://chromiumcodereview.appspot.com/9655023/diff/16004/media/audio/android... media/audio/android/opensles_util.h:27: SLDerefType operator->() { On 2012/03/22 15:01:26, tommi wrote: > nit: one liner (same for Get()) Done. https://chromiumcodereview.appspot.com/9655023/diff/16005/media/audio/android... File media/audio/android/opensles_input.cc (right): https://chromiumcodereview.appspot.com/9655023/diff/16005/media/audio/android... media/audio/android/opensles_input.cc:23: format_.samplesPerSec = static_cast<SLuint32>(params.sample_rate() * 1000); On 2012/03/23 15:19:58, tommi wrote: > Can you add a comment here that explains that 'samplesPerSec' should really be > 'samplesPer1000Sec' or 'samplesInMilliHz' so that other readers won't be > confused by this as I was? Done. https://chromiumcodereview.appspot.com/9655023/diff/16005/media/audio/android... media/audio/android/opensles_input.cc:74: HandleError(err); On 2012/03/23 15:19:58, tommi wrote: > If we get here, 'started_' and all the other properties will be in a bad state, > right? should we reset those or perhaps only set started_ to true (etc) below, > when we know that we started successfully? I think we are handling the error here. When it fails to start, we call HandleError() which will delete the stream. And setting the started_ flag allows Calling Stop(), which I think it is a good for cleaning up the states. https://chromiumcodereview.appspot.com/9655023/diff/16005/media/audio/android... media/audio/android/opensles_input.cc:94: LOG(WARNING) << "SetRecordState() failed to set the state to stop"; On 2012/03/23 15:19:58, tommi wrote: > DLOG I actually changed it to LOG explicitly in my previous version, since I thought those logging might be useful. But now I think maybe LOG is not needed since Android has its own logging system. https://chromiumcodereview.appspot.com/9655023/diff/16005/media/audio/android... media/audio/android/opensles_input.cc:100: LOG(WARNING) << "Clear() failed to clear the buffer queue"; On 2012/03/23 15:19:58, tommi wrote: > DLOG > > You could also use DLOG_IF Done. https://chromiumcodereview.appspot.com/9655023/diff/16005/media/audio/android... media/audio/android/opensles_input.cc:110: // Explicitly Free the player objects and invalidate their associated On 2012/03/23 15:19:58, tommi wrote: > s/Free/free Done. https://chromiumcodereview.appspot.com/9655023/diff/16005/media/audio/android... media/audio/android/opensles_input.cc:111: // interfaces. They have to be done in order. On 2012/03/23 15:19:58, tommi wrote: > s/done in order/done in the correct order Done. https://chromiumcodereview.appspot.com/9655023/diff/16005/media/audio/android... media/audio/android/opensles_input.cc:121: return 0.0; On 2012/03/23 15:19:58, tommi wrote: > should this be 1.0? We return 0.0 to indicate we don't support volume API for android https://chromiumcodereview.appspot.com/9655023/diff/16005/media/audio/android... media/audio/android/opensles_input.cc:125: } On 2012/03/23 15:19:58, tommi wrote: > NOTIMPLEMENTED? Done. https://chromiumcodereview.appspot.com/9655023/diff/16005/media/audio/android... media/audio/android/opensles_input.cc:166: SLDataSource audio_source = { reinterpret_cast<void*>(&mic_locator), NULL }; On 2012/03/23 15:19:58, tommi wrote: > cast necessary? They are all from the sample code, but as you have already pointed out, they are not necessary. I removed them instead. https://chromiumcodereview.appspot.com/9655023/diff/16005/media/audio/android... media/audio/android/opensles_input.cc:174: reinterpret_cast<void*>(&buffer_queue), On 2012/03/23 15:19:58, tommi wrote: > cast necessary? (and below) Done. https://chromiumcodereview.appspot.com/9655023/diff/16005/media/audio/android... media/audio/android/opensles_input.cc:179: const SLuint32 number_of_interface = 1; On 2012/03/23 15:19:58, tommi wrote: > number_of_interfaces Done. https://chromiumcodereview.appspot.com/9655023/diff/16005/media/audio/android... media/audio/android/opensles_input.cc:180: const SLInterfaceID interface_id[] = { SL_IID_ANDROIDSIMPLEBUFFERQUEUE }; On 2012/03/23 15:19:58, tommi wrote: > nit: can you use number_of_interfaces as the array size? > i.e. > const SLInterfaceID interface_id[number_of_interfaces] = ... Done. https://chromiumcodereview.appspot.com/9655023/diff/16005/media/audio/android... media/audio/android/opensles_input.cc:181: const SLboolean interface_required[] = { SL_BOOLEAN_TRUE }; On 2012/03/23 15:19:58, tommi wrote: > and here Done. https://chromiumcodereview.appspot.com/9655023/diff/16005/media/audio/android... media/audio/android/opensles_input.cc:188: interface_required); On 2012/03/23 15:19:58, tommi wrote: > actually, if you're only ever asking for a single interface, then you don't have > to use arrays: > > SLInterfaceID interface_id = SL_IID_ANDROIDSIMPLEBUFFERQUEUE; > SLboolean interface_required = SL_BOOLEAN_TRUE; > err = (*engine)->CreateAudioRecorder(engine, > recorder_object_.Receive(), > &audio_source, > &audio_sink, > 1, > &interface_id, > &interface_required); I agree. But the benefit of using the array is to easily extend the code to get another interface. I know that we can get SL_IID_ANDROIDCONFIGURATION interface here and set/get the configuration. I haven't added it because there is no document explaining the gain of doing it. I am asking the question to the Android team, so I may either add one more interface or not use the arrays. https://chromiumcodereview.appspot.com/9655023/diff/16005/media/audio/android... media/audio/android/opensles_input.cc:191: LOG(ERROR) << "CreateAudioRecorder failed with error code " << err; On 2012/03/23 15:19:58, tommi wrote: > logspam. same for the rest. Done. https://chromiumcodereview.appspot.com/9655023/diff/16005/media/audio/android... media/audio/android/opensles_input.cc:199: LOG(ERROR) << "Recprder Realize() failed with error code " << err; On 2012/03/23 15:19:58, tommi wrote: > fix typo Done. https://chromiumcodereview.appspot.com/9655023/diff/16005/media/audio/android... File media/audio/android/opensles_input.h (right): https://chromiumcodereview.appspot.com/9655023/diff/16005/media/audio/android... media/audio/android/opensles_input.h:55: // Recorder interfaces. On 2012/03/23 15:19:58, tommi wrote: > s/interfaces/interface > I don't think this comment adds much though, so you could also skip it. Done. https://chromiumcodereview.appspot.com/9655023/diff/16005/media/audio/android... File media/audio/android/opensles_output.cc (right): https://chromiumcodereview.appspot.com/9655023/diff/16005/media/audio/android... media/audio/android/opensles_output.cc:25: format_.samplesPerSec = static_cast<SLuint32>(params.sample_rate() * 1000); On 2012/03/23 15:19:58, tommi wrote: > same comment here as in the other place. Done. https://chromiumcodereview.appspot.com/9655023/diff/16005/media/audio/android... media/audio/android/opensles_output.cc:104: // Explicitly Free the player objects and invalidate their associated On 2012/03/23 15:19:58, tommi wrote: > fix up comments here as in the other file. Done. https://chromiumcodereview.appspot.com/9655023/diff/16005/media/audio/android... media/audio/android/opensles_output.cc:174: static_cast<SLuint32>(kNumOfQueuesInBuffer) }; On 2012/03/23 15:19:58, tommi wrote: > }; on the next line. also fix throughout if there are more cases of this. Done. https://chromiumcodereview.appspot.com/9655023/diff/16005/media/audio/android... media/audio/android/opensles_output.cc:273: HandleError(err); On 2012/03/23 15:19:58, tommi wrote: > also return? or is it OK to update active_queue_ if we failed? I think we'd better to update the active_queue_ here even it fails. https://chromiumcodereview.appspot.com/9655023/diff/16005/media/audio/android... File media/audio/android/opensles_output.h (right): https://chromiumcodereview.appspot.com/9655023/diff/16005/media/audio/android... media/audio/android/opensles_output.h:72: float volume_; On 2012/03/23 15:19:58, tommi wrote: > not sure if I already asked, but why don't we use double here like we do > elsewhere? Yes, you did, and my reply was: we use float for this variable in all platforms, but simply because function AdjustVolume() needs it to be float. I think both double and float work, and using float is to be consistent with other platform implementation.
Great job Shijing! LGTM. https://chromiumcodereview.appspot.com/9655023/diff/16005/media/audio/android... File media/audio/android/opensles_output.h (right): https://chromiumcodereview.appspot.com/9655023/diff/16005/media/audio/android... media/audio/android/opensles_output.h:72: float volume_; On 2012/03/23 20:32:15, xians1 wrote: > On 2012/03/23 15:19:58, tommi wrote: > > not sure if I already asked, but why don't we use double here like we do > > elsewhere? > > Yes, you did, and my reply was: > we use float for this variable in all platforms, but simply because function > AdjustVolume() needs it to be float. > I think both double and float work, and using float is to be consistent with > other platform implementation. ok. I see that the other implementations of this interface use float. There's code elsewhere that uses double and the methods you're implementing (GetVolume/SetVolume) use double. https://chromiumcodereview.appspot.com/9655023/diff/14006/media/audio/android... File media/audio/android/opensles_output.cc (right): https://chromiumcodereview.appspot.com/9655023/diff/14006/media/audio/android... media/audio/android/opensles_output.cc:185: SLuint32 number_of_interface = 1; number_of_interfaces https://chromiumcodereview.appspot.com/9655023/diff/14006/media/audio/android... media/audio/android/opensles_output.cc:270: HandleError(err); fix indent.
I tried to use the dynamic linking for the opensles library, but unfortunately it does not help much, doing that can save only several Kbytes, then I found out the opensles.so is only 5.9K. So the problem must not be in the opensles, but in my code. I digged into code, and found out that if I replace the std::vector with array pointer, it save almost 40K. With this version, this CL will add only 80K to the binary. Tommi, please see answers inline. On 2012/03/24 11:27:33, tommi wrote: > ok. I see that the other implementations of this interface use float. There's > code elsewhere that uses double and the methods you're implementing > (GetVolume/SetVolume) use double. > I just double checked, on the input side we use double, and on the output side we always use float. It is a bit silly to use different types for the same thing, but lets keep it this way, I believe we will move the code to somewhere else after we have mixing in place, then we change it. http://codereview.chromium.org/9655023/diff/14006/media/audio/android/opensle... File media/audio/android/opensles_output.cc (right): http://codereview.chromium.org/9655023/diff/14006/media/audio/android/opensle... media/audio/android/opensles_output.cc:185: SLuint32 number_of_interface = 1; On 2012/03/24 11:27:33, tommi wrote: > number_of_interfaces Done. http://codereview.chromium.org/9655023/diff/14006/media/audio/android/opensle... media/audio/android/opensles_output.cc:270: HandleError(err); On 2012/03/24 11:27:33, tommi wrote: > fix indent. Done.
https://chromiumcodereview.appspot.com/9655023/diff/20001/media/audio/android... File media/audio/android/opensles_input.cc (right): https://chromiumcodereview.appspot.com/9655023/diff/20001/media/audio/android... media/audio/android/opensles_input.cc:21: // Sampling rate in milliHertz. this comment is confusing, params.sample_rate() is in milliHertz? https://chromiumcodereview.appspot.com/9655023/diff/20001/media/audio/android... media/audio/android/opensles_input.cc:30: for (int i = 0; i < kNumOfQueuesInBuffer; ++i) { why do this here? shouldn't we allocate the mem when start() or open() getting called? We are wasting memories if user just create the object and never use it https://chromiumcodereview.appspot.com/9655023/diff/20001/media/audio/android... media/audio/android/opensles_input.cc:65: for (int i = 0; i < (kNumOfQueuesInBuffer - 1); ++i) { remove () for kNumOfQueuesInBuffer - 1, not needed https://chromiumcodereview.appspot.com/9655023/diff/20001/media/audio/android... media/audio/android/opensles_input.cc:79: if (SL_RESULT_SUCCESS != err) if this is true, should we reset started_ to false? Or in other words, should started_ be set after this check? https://chromiumcodereview.appspot.com/9655023/diff/20001/media/audio/android... File media/audio/android/opensles_input.h (right): https://chromiumcodereview.appspot.com/9655023/diff/20001/media/audio/android... media/audio/android/opensles_input.h:5: #ifndef MEDIA_AUDIO_OPENSLES_INPUT_H_ MEDIA_AUDIO_ANDROID_OPENSLED_INPUT_H_ https://chromiumcodereview.appspot.com/9655023/diff/20001/media/audio/android... media/audio/android/opensles_input.h:6: #define MEDIA_AUDIO_OPENSLES_INPUT_H_ same here https://chromiumcodereview.appspot.com/9655023/diff/20001/media/audio/android... media/audio/android/opensles_input.h:19: static const int kNumOfQueuesInBuffer = 2; can we put this constant to opensles_util.h? it appears in both input and output header files https://chromiumcodereview.appspot.com/9655023/diff/20001/media/audio/android... media/audio/android/opensles_input.h:74: #endif // MEDIA_AUDIO_OPENSLES_INPUT_H_ update this too https://chromiumcodereview.appspot.com/9655023/diff/20001/media/audio/android... File media/audio/android/opensles_output.cc (right): https://chromiumcodereview.appspot.com/9655023/diff/20001/media/audio/android... media/audio/android/opensles_output.cc:23: // Sampling rate in milliHertz. fix comments https://chromiumcodereview.appspot.com/9655023/diff/20001/media/audio/android... media/audio/android/opensles_output.cc:33: audio_data_[i] = new uint8[buffer_size_bytes_]; same as input, we should delay the buffer creation to start/open https://chromiumcodereview.appspot.com/9655023/diff/20001/media/audio/android... media/audio/android/opensles_output.cc:73: if (SL_RESULT_SUCCESS != err) { reset start_? https://chromiumcodereview.appspot.com/9655023/diff/20001/media/audio/android... File media/audio/android/opensles_output.h (right): https://chromiumcodereview.appspot.com/9655023/diff/20001/media/audio/android... media/audio/android/opensles_output.h:5: #ifndef MEDIA_AUDIO_OPENSLES_OUTPUT_H_ ANDROID https://chromiumcodereview.appspot.com/9655023/diff/20001/media/audio/android... File media/audio/android/opensles_util.h (right): https://chromiumcodereview.appspot.com/9655023/diff/20001/media/audio/android... media/audio/android/opensles_util.h:5: #ifndef MEDIA_AUDIO_OPENSLES_UTIL_H_ ANDROID https://chromiumcodereview.appspot.com/9655023/diff/20001/media/audio/android... media/audio/android/opensles_util.h:8: #include "base/logging.h" this include is not used here https://chromiumcodereview.appspot.com/9655023/diff/20001/media/audio/audio_l... File media/audio/audio_low_latency_input_output_unittest.cc (right): https://chromiumcodereview.appspot.com/9655023/diff/20001/media/audio/audio_l... media/audio/audio_low_latency_input_output_unittest.cc:347: samples_per_packet_ = 440; why 440?
https://chromiumcodereview.appspot.com/9655023/diff/20001/media/audio/android... File media/audio/android/opensles_input.cc (right): https://chromiumcodereview.appspot.com/9655023/diff/20001/media/audio/android... media/audio/android/opensles_input.cc:21: // Sampling rate in milliHertz. On 2012/04/01 01:25:20, qinmin wrote: > this comment is confusing, params.sample_rate() is in milliHertz? :) same question for Tommi. params.sample_rate() is in Hertz, but Android asks for samplerate in milliHertz, that is why we need to *1000 one line below. https://chromiumcodereview.appspot.com/9655023/diff/20001/media/audio/android... media/audio/android/opensles_input.cc:30: for (int i = 0; i < kNumOfQueuesInBuffer; ++i) { On 2012/04/01 01:25:20, qinmin wrote: > why do this here? shouldn't we allocate the mem when start() or open() getting > called? We are wasting memories if user just create the object and never use it The only alternative is to handle the memory in Open(), then in Close() and the destructor we need to check if the memory exists and delete it. I think this is a cleaner way to manage the memory this way, and it sounds a programming bug to me if we create the audio stream but never use it. Hope it is fine to keep it. https://chromiumcodereview.appspot.com/9655023/diff/20001/media/audio/android... media/audio/android/opensles_input.cc:65: for (int i = 0; i < (kNumOfQueuesInBuffer - 1); ++i) { On 2012/04/01 01:25:20, qinmin wrote: > remove () for kNumOfQueuesInBuffer - 1, not needed Done. https://chromiumcodereview.appspot.com/9655023/diff/20001/media/audio/android... media/audio/android/opensles_input.cc:79: if (SL_RESULT_SUCCESS != err) On 2012/04/01 01:25:20, qinmin wrote: > if this is true, should we reset started_ to false? Or in other words, should > started_ be set after this check? Tommi asked me the same question. Resetting started to false here will prevent calling Stop(), and I think we should allow calling Stop() to clean up the states even it fails. https://chromiumcodereview.appspot.com/9655023/diff/20001/media/audio/android... File media/audio/android/opensles_input.h (right): https://chromiumcodereview.appspot.com/9655023/diff/20001/media/audio/android... media/audio/android/opensles_input.h:5: #ifndef MEDIA_AUDIO_OPENSLES_INPUT_H_ On 2012/04/01 01:25:20, qinmin wrote: > MEDIA_AUDIO_ANDROID_OPENSLED_INPUT_H_ Done. https://chromiumcodereview.appspot.com/9655023/diff/20001/media/audio/android... media/audio/android/opensles_input.h:6: #define MEDIA_AUDIO_OPENSLES_INPUT_H_ On 2012/04/01 01:25:20, qinmin wrote: > same here Done. https://chromiumcodereview.appspot.com/9655023/diff/20001/media/audio/android... media/audio/android/opensles_input.h:19: static const int kNumOfQueuesInBuffer = 2; On 2012/04/01 01:25:20, qinmin wrote: > can we put this constant to opensles_util.h? it appears in both input and output > header files These values don't necessarily to be the same, but the opposite. The fact is that they are to be tuned, we may use different values for input and output, depending on the CPU complexity and problems we will run into in the future. But 2 is a good value to start with for both cases. If you think they are confusing, I can change the names to kNumOfQueuesInInputBuffer and kNumOfQueuesInOutputBuffer https://chromiumcodereview.appspot.com/9655023/diff/20001/media/audio/android... media/audio/android/opensles_input.h:74: #endif // MEDIA_AUDIO_OPENSLES_INPUT_H_ On 2012/04/01 01:25:20, qinmin wrote: > update this too Done. https://chromiumcodereview.appspot.com/9655023/diff/20001/media/audio/android... File media/audio/android/opensles_output.cc (right): https://chromiumcodereview.appspot.com/9655023/diff/20001/media/audio/android... media/audio/android/opensles_output.cc:23: // Sampling rate in milliHertz. On 2012/04/01 01:25:20, qinmin wrote: > fix comments Done. https://chromiumcodereview.appspot.com/9655023/diff/20001/media/audio/android... media/audio/android/opensles_output.cc:33: audio_data_[i] = new uint8[buffer_size_bytes_]; On 2012/04/01 01:25:20, qinmin wrote: > same as input, we should delay the buffer creation to start/open See answer in the input side. https://chromiumcodereview.appspot.com/9655023/diff/20001/media/audio/android... media/audio/android/opensles_output.cc:73: if (SL_RESULT_SUCCESS != err) { On 2012/04/01 01:25:20, qinmin wrote: > reset start_? See answer in the input side. https://chromiumcodereview.appspot.com/9655023/diff/20001/media/audio/android... File media/audio/android/opensles_output.h (right): https://chromiumcodereview.appspot.com/9655023/diff/20001/media/audio/android... media/audio/android/opensles_output.h:5: #ifndef MEDIA_AUDIO_OPENSLES_OUTPUT_H_ On 2012/04/01 01:25:20, qinmin wrote: > ANDROID Done. https://chromiumcodereview.appspot.com/9655023/diff/20001/media/audio/android... File media/audio/android/opensles_util.h (right): https://chromiumcodereview.appspot.com/9655023/diff/20001/media/audio/android... media/audio/android/opensles_util.h:5: #ifndef MEDIA_AUDIO_OPENSLES_UTIL_H_ On 2012/04/01 01:25:20, qinmin wrote: > ANDROID Done. https://chromiumcodereview.appspot.com/9655023/diff/20001/media/audio/android... media/audio/android/opensles_util.h:8: #include "base/logging.h" On 2012/04/01 01:25:20, qinmin wrote: > this include is not used here we use it for DCHECK() in SLType* Receive() {} https://chromiumcodereview.appspot.com/9655023/diff/20001/media/audio/audio_l... File media/audio/audio_low_latency_input_output_unittest.cc (right): https://chromiumcodereview.appspot.com/9655023/diff/20001/media/audio/audio_l... media/audio/audio_low_latency_input_output_unittest.cc:347: samples_per_packet_ = 440; On 2012/04/01 01:25:20, qinmin wrote: > why 440? Good catch, in WebRTC we only handle 440 samples for 10ms data in 44100Hz, which I know it works well to truncate it. It does not hurt but obviously it is not needed here. I removed it.
https://chromiumcodereview.appspot.com/9655023/diff/23002/media/audio/android... File media/audio/android/opensles_input.cc (right): https://chromiumcodereview.appspot.com/9655023/diff/23002/media/audio/android... media/audio/android/opensles_input.cc:30: for (int i = 0; i < kNumOfQueuesInBuffer; ++i) { I still prefer you put this into the Open() function. You can define a ClearAudioData() function that gets called in both Close(), and the destructor in case Close() is not getting called. Declaring sth and never using it is pretty common in actual practices. https://chromiumcodereview.appspot.com/9655023/diff/23002/media/audio/android... File media/audio/android/opensles_util.h (right): https://chromiumcodereview.appspot.com/9655023/diff/23002/media/audio/android... media/audio/android/opensles_util.h:5: #ifndef MEDIA_AUDIO_OPENSLES_UTIL_H_ missing ANDROID https://chromiumcodereview.appspot.com/9655023/diff/23002/media/audio/android... media/audio/android/opensles_util.h:6: #define MEDIA_AUDIO_OPENSLES_UTIL_H_ this too https://chromiumcodereview.appspot.com/9655023/diff/23002/media/audio/android... media/audio/android/opensles_util.h:44: #endif // MEDIA_AUDIO_OPENSLES_UTIL_H_ same here
Please take another look. https://chromiumcodereview.appspot.com/9655023/diff/23002/media/audio/android... File media/audio/android/opensles_input.cc (right): https://chromiumcodereview.appspot.com/9655023/diff/23002/media/audio/android... media/audio/android/opensles_input.cc:30: for (int i = 0; i < kNumOfQueuesInBuffer; ++i) { On 2012/04/02 13:28:54, qinmin wrote: > I still prefer you put this into the Open() function. You can define a > ClearAudioData() function that gets called in both Close(), and the destructor > in case Close() is not getting called. Declaring sth and never using it is > pretty common in actual practices. Done. https://chromiumcodereview.appspot.com/9655023/diff/23002/media/audio/android... File media/audio/android/opensles_util.h (right): https://chromiumcodereview.appspot.com/9655023/diff/23002/media/audio/android... media/audio/android/opensles_util.h:5: #ifndef MEDIA_AUDIO_OPENSLES_UTIL_H_ On 2012/04/02 13:28:54, qinmin wrote: > missing ANDROID Sorry, I missed this in the previous set. Done now. https://chromiumcodereview.appspot.com/9655023/diff/23002/media/audio/android... media/audio/android/opensles_util.h:6: #define MEDIA_AUDIO_OPENSLES_UTIL_H_ On 2012/04/02 13:28:54, qinmin wrote: > this too Done. https://chromiumcodereview.appspot.com/9655023/diff/23002/media/audio/android... media/audio/android/opensles_util.h:44: #endif // MEDIA_AUDIO_OPENSLES_UTIL_H_ On 2012/04/02 13:28:54, qinmin wrote: > same here Done.
lgtm
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xians@chromium.org/9655023/27006
Change committed as 130174 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
