This is a WIP B2G WebRTC audio_device implementation. The patch only contain the platform module codes, and it will need some change in common module code to let it work. I will add those change later. It is porting from Android Jni version, since we meet some trouble to link OpenSLES library. This is only for anyone interest in B2G WebRTC feature.

What problems are you having linking OpenSLES? On the Android side we've temporarily made it work by requiring Android 2.3. Our slightly longer-term plan is to make the code dynamically load OpenSLES rather than link against it, because that would cause it fail gracefully on Android 2.2, rather than having all of Fennec either crash or fail to start. Does that sound like what you ran into, or something else?

[WIP] V1 part#2 This patch change some common audio_module code. WARNING: This patch will cause build break now. We need a complete B2G WebRTC build related change to build it correctly. (See Bug#818843)

(In reply to Dan Mosedale (:dmose) from comment #2) > What problems are you having linking OpenSLES? > > On the Android side we've temporarily made it work by requiring Android 2.3. > Our slightly longer-term plan is to make the code dynamically load OpenSLES > rather than link against it, because that would cause it fail gracefully on > Android 2.2, rather than having all of Fennec either crash or fail to start. > > Does that sound like what you ran into, or something else? When we tried to enable the makefile for it last time, the b2g process can not start-up normally with a segmentation fault in dlfree(). StevenLee found it before, and I am not sure whether dynamic load helps.

It'd be interesting to see whole stack-trace from the crash. What version of Android is B2G based on? 2.2? This provoked a short discussion in IRC about the JNI vs. OpenSLES approaches: [10:20am] dmose: i seem to recall reading comments somewhere in the tree suggesting that the JNI stuff was older [10:20am] dmose: and OpenSLES was the way of the future [10:20am] gcp: it would be good to find out what was wrong with JNI that they switched to OpenSLES [10:20am] dmose: but who knows [10:21am] dmose: derf? [10:22am] derf: dmose: Mostly the Java API has astoundingly large latency. [10:23am] dmose: so large that it would be perceptible? [10:23am] derf: So large that it is larger than the rest of the stack, including the network, combined. [10:23am] gcp: lol [10:23am] derf: So yeah.. it'll be perceptible. [10:24am] jesup: Last I remember the JNI stuff had latencies in the hundreds of ms. Perhaps even 1/2 second if it's the problem I remember [10:30am] derf: jesup: IIRC, 200-300 ms was "best case". [10:30am] jesup: derf: yeah, that's about right. [10:30am] dmose: yikes [10:31am] dmose: so, like, extremely unpleasant user-experience for voice conversation

Hi dmose, I would like to invite you to review this patch. This patch is basically porting from Android counterpart. And if you had read my WIP patches, here is a changelog: - Clean out masked code - Remove unwanted logs - Fix Makefile

Sorry for the delayed response. I'm working on getting video up for an upcoming Android demo, so I don't expect to have time to review something like this for a number of weeks. The Android audio story has gotten a lot more complicated in the last week or so. The first part of the story is in bug 832551. Since filing that bug, jesup and others have met with the webrtc.org team, who seem to have encountered other bad problems with OpenSL that we haven't hit, so much so that they intend to focus on the JNI code for the next while. I don't think we really have a sense yet of what we want to do about this. The new webrtc.org code is landing on mozilla-central next week, which will force this issue once the Android WebRTC hackers come back to the trunk (we're mostly living on Alder at the moment). One thing that it could be productive to work on in the mean time is getting the webrtc C++ tests and mochitests running (and passing?) on B2G, as that will make the reviewing easier.

(In reply to Dan Mosedale (:dmose) from comment #7) > Sorry for the delayed response. I'm working on getting video up for an > upcoming Android demo, so I don't expect to have time to review something > like this for a number of weeks. It is OK for me to delay review as I got other task to go at the same time. However, I think we may got some schedule problem as this feature is requested from our partner. I think the deadline should be end of March...I will find a PM to confirm that. > > The Android audio story has gotten a lot more complicated in the last week > or so. The first part of the story is in bug 832551. Since filing that bug, > jesup and others have met with the webrtc.org team, who seem to have > encountered other bad problems with OpenSL that we haven't hit, so much so > that they intend to focus on the JNI code for the next while. I don't think > we really have a sense yet of what we want to do about this. We do not use OpenSL now since we got some linking problem. And after reading some OpenSLES document/discussion, I think it is not good solution now. Since Android do not implement any specific profile in OpenSLES 1.1 and add many Android extensions without clear document (or I did not find them :S). I am afraid that we got some platform work but not others if they implement their drivers with slight different behavior. Besides, I don't think we can complete OpenSL B2G porting before the deadline... > > The new webrtc.org code is landing on mozilla-central next week, which will > force this issue once the Android WebRTC hackers come back to the trunk > (we're mostly living on Alder at the moment). Then I must review some code and porting them into my patch. However, I will join some WebGL related topic this week and start to implement other multimedia framework (the encoder part for MediaRecord API) from next week, I am not sure when I can get it done :S > > One thing that it could be productive to work on in the mean time is getting > the webrtc C++ tests and mochitests running (and passing?) on B2G, as that > will make the reviewing easier. I heard that mochitests is for desktop, and B2G have a test framework called Marionette. I will discuss this part with StevenLee first.

(In reply to Chiajung Hung [:chiajung] from comment #8) > > The Android audio story has gotten a lot more complicated in the last week > > or so. The first part of the story is in bug 832551. Since filing that bug, > > jesup and others have met with the webrtc.org team, who seem to have > > encountered other bad problems with OpenSL that we haven't hit, so much so > > that they intend to focus on the JNI code for the next while. I don't think > > we really have a sense yet of what we want to do about this. > > We do not use OpenSL now since we got some linking problem. And after > reading some OpenSLES document/discussion, I think it is not good solution > now. Since Android do not implement any specific profile in OpenSLES 1.1 and > add many Android extensions without clear document (or I did not find them > :S). I am afraid that we got some platform work but not others if they > implement their drivers with slight different behavior. > > Besides, I don't think we can complete OpenSL B2G porting before the > deadline... Given the deadlines you've mentioned, I totally agree that if you've got something that works well enough on the hardware you care about, I definitely agree that now is not the time to switch out stacks. For what it's worth, $ANDROID_NDK_ROOT/docs/opensles has some documentation about exactly what parts of OpenSL ES Android does and doesn't implement. The master copy is at <https://android.googlesource.com/platform/ndk.git/+/master/docs/opensles/index.html>. > > One thing that it could be productive to work on in the mean time is getting > > the webrtc C++ tests and mochitests running (and passing?) on B2G, as that > > will make the reviewing easier. > > I heard that mochitests is for desktop, and B2G have a test framework called > Marionette. I will discuss this part with StevenLee first. There are a bunch of already-written WebRTC mochitests that work on both desktop and Android in the tree today. Looking at Tbpl, it looks to me as thought B2G can run mochitests too. The idea here is not to write new mochitests, but rather to make the existing ones pass on B2G. There are also a bunch of already-written WebRTC C++ unit tests that help ensure that the low-level code is working. As part of the Android bringup, we got help from ted and gbrown so that it's now possible to run those tests by hand (but not yet in the automation) on Android. I'm not sure what the status of both running and automating C++ unit tests on B2G is. As far as Marionette, that might well make sense for writing some set of new tests going forward, but w.r.t. to this patch, I was really just suggesting getting existing tests passing.

First off, my apologies for not getting back to this bug sooner. Before getting into the review bits specifically, I have some higher order concerns: * It's not totally clear to me whether the intent here is to land this code with WebRTC actually turned on by default. I assume, however, that intent is to turn it on at the time of landing. Is that correct? * Assuming that is, one of my concerns is that the core WebRTC team is already badly overstretched, and I'm concerned about the addition platform burden in terms of review, architecture adjustments, and bug-fixing support. * I'm also concerned that code with as much complexity as the WebRTC stack is likely to exact high costs in terms of shipped code quality, vulnerability to regressions, and impact on future feature work, particularly as exposed by the variety of hardware platforms B2G wants to ship on. Those concerns would be significantly mitigated by (at the very least) ensuring that all of the existing WebRTC automated tests (both mochitest and C++ unit tests) were both made to pass for B2G and turned on in the automation. So the first question I have is, what is the state of the WebRTC automated tests in each of those suites on B2G? Separately from this, I was expecting that I could both have the bandwidth and be a good reviewer for these changes. However, I'm both busier than expected, and upon deeper inspection, I think these changes really want review from 2 or 3 people, and I'm probably not the best person to fill any of these roles: at least: * somebody who has a deep understanding of the getUserMedia stuff (jesup?) as well as one or both of: * somebody who has more current understanding of the state of the Android audio world (gcp? snorp?) * somebody who has a deep understanding of b2g audio (not sure who fits this best here)

OK, I will rebase and change something then request review to jesup. Thanks for the information!!

Rebase the patch to new architecture.

Fix an occassionally deadlock when using Audio/Video at same time.

Comment on attachment 732200 [details] [diff] [review] Patch V2 Review of attachment 732200 [details] [diff] [review]: ----------------------------------------------------------------- ::: configure.in @@ +9135,4 @@ > AC_MSG_RESULT("generating WebRTC Makefiles...") > > if test "${MOZ_WIDGET_TOOLKIT}" = "gonk"; then > + EXTRA_GYP_DEFINES="${EXTRA_GYP_DEFINES} -D build_with_gonk=1" remove trailing space ::: media/webrtc/trunk/webrtc/modules/audio_device/gonk/audio_device_gonk.cc @@ +25,5 @@ > +// Android logging, uncomment to print trace to logcat instead of > +// trace file/callback > +//#include <android/log.h> > +//#define WEBRTC_TRACE(a,b,c,...) __android_log_print(ANDROID_LOG_DEBUG, \ > +// "WebRTC AD jni", __VA_ARGS__) Please make this a #ifdef instead of uncomment to enable @@ +303,5 @@ > + _playThreadIsInitialized = false; > + } > + _speakerIsInitialized = false; > + _playoutDeviceIsSpecified = false; > + _initialized = false; Since we're putting _initialized back to false, should we reset the other state vars like _shutdownPlayThread? @@ +314,5 @@ > +// ---------------------------------------------------------------------------- > + > +bool AudioDeviceGonk::Initialized() const > +{ > + return _initialized; If this is not guaranteed to be accessed from the same thread that handles Init() and Terminate(), this needs a critical section lock to avoid a data race. I note that the other platforms don't lock here, so they may be on the same thread (or conversely they may all have a bug here). @@ +405,5 @@ > +// ---------------------------------------------------------------------------- > + > +bool AudioDeviceGonk::SpeakerIsInitialized() const > +{ > + return _speakerIsInitialized; If this is not guaranteed to be accessed from the same thread that handles InitSpeaker(), this needs a critical section lock to avoid a data race. I note that the other platforms don't lock here, so they may be on the same thread (or conversely they may all have a bug here). @@ +414,5 @@ > +// ---------------------------------------------------------------------------- > + > +bool AudioDeviceGonk::MicrophoneIsInitialized() const > +{ > + return _micIsInitialized; If this is not guaranteed to be accessed from the same thread that handles InitMicrophone(), this needs a critical section lock to avoid a data race. I note that the other platforms don't lock here, so they may be on the same thread (or conversely they may all have a bug here). @@ +586,5 @@ > +// ---------------------------------------------------------------------------- > + > +WebRtc_Word32 AudioDeviceGonk::MicrophoneMuteIsAvailable(bool& available) > +{ > + available = false; // Mic mute not supported on Android There are a bunch of "not supported on Android" that should be changed to Gonk or wahtever @@ +630,5 @@ > +// ---------------------------------------------------------------------------- > + > +WebRtc_Word32 AudioDeviceGonk::SetMicrophoneBoost(bool enable) > +{ > + if (!_micIsInitialized) If this is not guaranteed to be accessed from the same thread that handles InitPicrophone(), this needs a critical section lock to avoid a data race. I note that the other platforms don't lock here, so they may be on the same thread (or conversely they may all have a bug here). Assume this comment is repeated for other state vars set under critical sections. @@ +964,5 @@ > + > + // Recording device index will be used for specifying recording > + // audio source, allow any value > + _recAudioSource = index; > + _recordingDeviceIsSpecified = true; Does this need to be locked? I note that the mac doesn't. How many recording devices can we have? @@ +1033,5 @@ > +#define ERROR_BAD_VALUE -2 > + > +int GetMinTrackBufferSize(int sampleRateInHz, int channelCount, int audioFormat) { > + int frameCount = AudioTrack::getMinFrameCount(&frameCount, AUDIO_STREAM_DEFAULT, sampleRateInHz); > + int size = frameCount * channelCount * (audioFormat == AUDIO_FORMAT_PCM_16_BIT ? 2 : 1);//native_get_min_buff_size(sampleRateInHz, channelCount, audioFormat); sizeof(int16_t or whichever type is being used for audio samples) instead of '2'. Ditto for '1'. Which definition of AUDIO_FORMAT_PCM_16_BIT is being used? And what value gets put in audioFormat other than it? media/libcubeb/src/audiotrack_definitions.h: typedef enum { AUDIO_FORMAT_PCM = 0x00000000, AUDIO_FORMAT_PCM_SUB_16_BIT = 0x1, AUDIO_FORMAT_PCM_16_BIT = (AUDIO_FORMAT_PCM | AUDIO_FORMAT_PCM_SUB_16_BIT), } AudioTrack_SampleType; or dom/system/gonk/android_audio/AudioSystem.h: AUDIO_FORMAT_PCM_SUB_16_BIT = 0x1, /* DO NOT CHANGE - PCM signed 16 bits */ AUDIO_FORMAT_PCM_SUB_8_BIT = 0x2, /* DO NOT CHANGE - PCM unsigned 8 bits */ AUDIO_FORMAT_PCM = 0x00000000UL, /* DO NOT CHANGE */ /* Aliases */ AUDIO_FORMAT_PCM_16_BIT = (AUDIO_FORMAT_PCM | AUDIO_FORMAT_PCM_SUB_16_BIT), @@ +1039,5 @@ > +} > + > +int AudioDeviceGonk::InitPlayback(int sampleRate) > +{ > + // get the minimum buffer size that can be used I see this is called from within InitPlayout(), which grabs a CriticalSection, so we're locked here (good) @@ +1055,5 @@ > + delete _audioTrack; > + _audioTrack = NULL; > + } > + > + int frameCount; tabs/indent @@ +1161,5 @@ > + } > + if (result != NO_ERROR) { > + return -1; > + } > + return frameCount * nbChannels * (audioFormat == AUDIO_FORMAT_PCM_16_BIT ? 2 : 1); sizeof(int16_t) please, not 2. Ditto for 1; see above. @@ +1307,5 @@ > + // Signal to recording thread that we want to start > + _startRec = true; > + _timeEventRec.Set(); // Release thread from waiting state > + // Wait for thread to init > + if (kEventSignaled != _recStartStopEvent.Wait(5000)) Do we need to release the criticalSection here, and re-acquire it? @@ +1404,5 @@ > + // Signal to playout thread that we want to start > + _startPlay = true; > + _timeEventPlay.Set(); // Release thread from waiting state > + // Wait for thread to init > + if (kEventSignaled != _playStartStopEvent.Wait(5000)) Do we need to release the criticalSection here, and re-acquire it? @@ +1447,5 @@ > +// ---------------------------------------------------------------------------- > + > +WebRtc_Word32 AudioDeviceGonk::PlayoutDelay(WebRtc_UWord16& delayMS) const > +{ > + delayMS = _delayPlayout; Like a lot of others here, do we need to lock? @@ +1610,5 @@ > + _samplingFreqIn = samplesPerSec / 1000; > + } > + > + // Update the AudioDeviceBuffer > + _ptrAudioBuffer->SetRecordingSampleRate(samplesPerSec); is locking required around changing _samplingFreqIn and calling SetRecordingSampleRate? @@ +1640,5 @@ > + _samplingFreqOut = samplesPerSec / 1000; > + } > + > + // Update the AudioDeviceBuffer > + _ptrAudioBuffer->SetPlayoutSampleRate(samplesPerSec); ditto @@ +1676,5 @@ > + > +// ---------------------------------------------------------------------------- > +// InitSampleRate > +// > +// checks supported sample rates for playback trailing space @@ +1681,5 @@ > +// and recording and initializes the rates to be used > +// Also stores the max playout volume returned from InitPlayout > +// ---------------------------------------------------------------------------- > + > +WebRtc_Word32 AudioDeviceGonk::InitSampleRate() I assume this is called with the critical section locked @@ +1710,5 @@ > + case 44100: > + samplingFreq = 16000; > + break; > + case 16000: > + samplingFreq = 8000; This seems... very odd @@ +1776,5 @@ > + case 44100: > + samplingFreq = 16000; > + break; > + case 16000: > + samplingFreq = 8000; ditto @@ +1889,5 @@ > + } > + > + if (_playing) > + { > + WebRtc_Word8 playBuffer[2 * 480]; // Max 10 ms @ 48 kHz / 16 bit Please hoist this constant/calculation out to a .h file; it's embedded in a number of places @@ +1921,5 @@ > + uint32_t pos; > + _audioTrack->getPosition(&pos); > + _bufferedPlaySamples -= (pos - _playPosition); > + _playPosition = pos; > + a few instances of trailing whitespace ::: media/webrtc/trunk/webrtc/modules/audio_device/gonk/audio_device_gonk.h @@ +4,5 @@ > + * Use of this source code is governed by a BSD-style license > + * that can be found in the LICENSE file in the root of the source > + * tree. An additional intellectual property rights grant can be found > + * in the file PATENTS. All contributing project authors may > + * be found in the AUTHORS file in the root of the source tree. Ask Gerv if you need to modify the AUTHORS file as well. @@ +160,5 @@ > +private: > + int InitPlayback(int sampleRate); > + int InitRecorder(int audioSource, int sampleRate); > + WebRtc_Word32 InitSampleRate(); > + indentation (perhaps tabs instead of spaces were used). Also, remove trailing spaces or tabs. @@ +238,5 @@ > + WebRtc_UWord32 _maxSpeakerVolume; // The maximum speaker volume value > + bool _loudSpeakerOn; > + // Stores the desired audio source to use, set in SetRecordingDevice > + int _recAudioSource; > + trailing spaces @@ +245,5 @@ > + int _bufferedRecSamples; > + int _playPosition; > + android::AudioTrack* _audioTrack; > + android::AudioRecord* _audioRecord; > + trailing spaces

Comment on attachment 732200 [details] [diff] [review] Patch V2 Review of attachment 732200 [details] [diff] [review]: ----------------------------------------------------------------- There are a bunch of things that need fixing here, though mostly minor. ::: media/webrtc/trunk/webrtc/modules/audio_device/audio_device_impl.h @@ +32,5 @@ > kPlatformLinux = 3, > kPlatformMac = 4, > kPlatformAndroid = 5, > + kPlatformIOS = 6, > + kPlatformGonk = 7 So, is the plan to merge this code upstream? If not, I think we should implement it as an "external AudioDeviceModule". This is, for example, the route Chrome takes to abstract away the real audio device in its renderer processes. This would involve extending webrtc::AudioDeviceModule instead of AudioDeviceGeneric, and then passing it to VoEBase::Init() each time a VoiceEngine is created. ::: media/webrtc/trunk/webrtc/modules/audio_device/gonk/audio_device_gonk.cc @@ +1,2 @@ > +/* > + * Copyright (c) 2012 The WebRTC project authors. All Rights Reserved. It's 2013. @@ +56,5 @@ > + _recThreadIsInitialized(false), > + _shutdownPlayThread(false), > + _shutdownRecThread(false), > + _recordingDeviceIsSpecified(false), > + _playoutDeviceIsSpecified(false), _initialized(false), I know you copied this from audio_device_jni_android.cc, but I think you should keep each initializer on a separate line here. @@ +65,5 @@ > + _playError(0), _recWarning(0), _recError(0), _delayPlayout(0), > + _delayRecording(0), > + _AGC(false), > + _samplingFreqIn(0), > + _samplingFreqOut(0), Any particular reason these are initialized to 0 instead of N_REC_SAMPLES_PER_SEC/1000 and N_PLAY_SAMPLES_PER_SEC/1000 like in audio_device_jni_android.cc? (I'm not actually sure what difference this makes) @@ +243,5 @@ > + _critSect.Enter(); > + > + // Close down rec thread > + ThreadWrapper* tmpThread = _ptrThreadRec; > + _ptrThreadRec = NULL; Don't you need to leave the critical section again here? @@ +255,5 @@ > + else > + { > + WEBRTC_TRACE(kTraceWarning, kTraceAudioDevice, _id, > + " failed to close down the rec audio thread"); > + } And re-enter it here? @@ +286,5 @@ > + _critSect.Enter(); > + > + // Close down play thread > + ThreadWrapper* tmpThread = _ptrThreadPlay; > + _ptrThreadPlay = NULL; Don't you need to leave the critical section again here? @@ +297,5 @@ > + else > + { > + WEBRTC_TRACE(kTraceWarning, kTraceAudioDevice, _id, > + " failed to close down the play audio thread"); > + } And re-enter it here? @@ +441,5 @@ > + " Speaker not initialized"); > + return -1; > + } > + > + _audioFlinger->setStreamVolume(AUDIO_STREAM_VOICE_CALL, static_cast<float>(volume), 0); _speakerIsInitialized can be set even if Init() was never called. I think you need to test to make sure _audioFlinger is non-NULL here. Are we sure this is the stream we want to use? libcubeb (which normally does all audio output for Gecko) uses AUDIO_STREAM_TYPE_MUSIC... though it's not clear anything should be using this at all, since we don't actually use the GIPS stack for audio output. However, the same question applies to all other uses of AUDIO_STREAM_VOICE_CALL in this patch. @@ +459,5 @@ > + " Speaker not initialized"); > + return -1; > + } > + > + volume = _audioFlinger->streamVolume(AUDIO_STREAM_VOICE_CALL, 0); _speakerIsInitialized can be set even if Init() was never called. I think you need to test to make sure _audioFlinger is non-NULL here. @@ +1032,5 @@ > +#define ERROR -1 > +#define ERROR_BAD_VALUE -2 > + > +int GetMinTrackBufferSize(int sampleRateInHz, int channelCount, int audioFormat) { > + int frameCount = AudioTrack::getMinFrameCount(&frameCount, AUDIO_STREAM_DEFAULT, sampleRateInHz); Why is this audio stream different than all of the other ones? @@ +1033,5 @@ > +#define ERROR_BAD_VALUE -2 > + > +int GetMinTrackBufferSize(int sampleRateInHz, int channelCount, int audioFormat) { > + int frameCount = AudioTrack::getMinFrameCount(&frameCount, AUDIO_STREAM_DEFAULT, sampleRateInHz); > + int size = frameCount * channelCount * (audioFormat == AUDIO_FORMAT_PCM_16_BIT ? 2 : 1);//native_get_min_buff_size(sampleRateInHz, channelCount, audioFormat); A) Split very long lines (Google style splits at 80 characters). B) Please don't leave commented out code in here without (at the minimum) some explanation of what it is. @@ +1045,5 @@ > + > + int playBufSize = minPlayBufSize; > + if (playBufSize < 6000) { > + playBufSize *= 2; > + } What is this code trying to do? It doesn't seem to guarantee that playBufSize is at least 6000, and there's no explanation of why that even matters. Please at least add some comments. @@ +1129,5 @@ > + } > + > + int retVal = -1; > + > + // Call java sc object method I don't think this comment is accurate. @@ +1147,5 @@ > + > + return retVal; > +} > + > +int GetMinRecordBufferSize(int sampleRateInHertz, int nbChannels, int audioFormat) It would be nice to keep sampleRateInHertz consistent with other variable names in this file that have the same purpose (especially the one passed to this function). @@ +1174,5 @@ > + int minRecBufSize = GetMinRecordBufferSize(sampleRate, > + 1, // Single channel > + AUDIO_FORMAT_PCM_16_BIT); > + > + // double size to be more safe Safe from what? Also "safer" not "more safe". @@ +1254,5 @@ > + } > + > + int retVal = -1; > + > + // call java sc object method I don't think this comment is accurate. @@ +1267,5 @@ > + // Set the audio device buffer sampling rate > + _ptrAudioBuffer->SetRecordingSampleRate(_samplingFreqIn * 1000); > + > + // the init rec function returns a fixed delay > + _delayRecording = res / _samplingFreqIn; I think padenot has patches for libcubeb to let us get at the actual delay values. You should probably coordinate with him to do something smarter here. @@ +1305,5 @@ > + _recError = 0; > + > + // Signal to recording thread that we want to start > + _startRec = true; > + _timeEventRec.Set(); // Release thread from waiting state Don't you need to leave the critical section here? @@ +1404,5 @@ > + // Signal to playout thread that we want to start > + _startPlay = true; > + _timeEventPlay.Set(); // Release thread from waiting state > + // Wait for thread to init > + if (kEventSignaled != _playStartStopEvent.Wait(5000)) I think we do (though we may be able to skip re-acquisition... the event interface is thread-safe, and we don't do anything after that). @@ +1606,5 @@ > + _samplingFreqIn = 44; > + } > + else > + { > + _samplingFreqIn = samplesPerSec / 1000; I know this is copied from audio_device_jni_android.cc, but this else clause has the same effect as the if, so I don't think we need the if. @@ +1636,5 @@ > + _samplingFreqOut = 44; > + } > + else > + { > + _samplingFreqOut = samplesPerSec / 1000; Same as above. @@ +1651,5 @@ > +// ---------------------------------------------------------------------------- > + > +WebRtc_Word32 AudioDeviceGonk::SetLoudspeakerStatus(bool enable) > +{ > + AudioSystem::setForceUse((audio_policy_force_use_t)0, (audio_policy_forced_cfg_t)enable); Shouldn't you check for an error return here and not update _loudSpeakerOn if this fails? @@ +1681,5 @@ > +// and recording and initializes the rates to be used > +// Also stores the max playout volume returned from InitPlayout > +// ---------------------------------------------------------------------------- > + > +WebRtc_Word32 AudioDeviceGonk::InitSampleRate() It is (way back up in Init()). @@ +1683,5 @@ > +// ---------------------------------------------------------------------------- > + > +WebRtc_Word32 AudioDeviceGonk::InitSampleRate() > +{ > + int samplingFreq = 44100; I think we should default to 48000 here if possible. It's at least a multiple of 1000, and will interact much better with the rates used by the rest of the WebRTC stack (most of which evenly divide 48000). @@ +1688,5 @@ > + int res = 0; > + > + if (_samplingFreqIn > 0) > + { > + // read the configured sampling rate This needs adjusting if you change the 44100 above. @@ +1706,5 @@ > + if (res < 0) > + { > + switch (samplingFreq) > + { > + case 44100: If you use 48000 above, you'll need a case for that... I recommend skipping straight to 16000 if 48000 isn't supported. @@ +1710,5 @@ > + case 44100: > + samplingFreq = 16000; > + break; > + case 16000: > + samplingFreq = 8000; It's copied from audio_device_jni_android.cc. I believe it is trying to step down the sampling rate until it gets one that succeeds. @@ +1733,5 @@ > + _samplingFreqIn = 44; > + } > + else > + { > + _samplingFreqIn = samplingFreq / 1000; See above about the if being useless. @@ +1807,5 @@ > + _samplingFreqOut = 44; > + } > + else > + { > + _samplingFreqOut = samplingFreq / 1000; Again, see above. Though after seeing this pattern 4 times, I begin to wonder if it (and the conversion in the other direction) shouldn't be factored out into their own functions instead of copy-pasted all over the place. @@ +1894,5 @@ > + WebRtc_UWord32 samplesToPlay = _samplingFreqOut * 10; > + > + // ask for new PCM data to be played out using the AudioDeviceBuffer > + // ensure that this callback is executed without taking the > + // audio-thread lock Shouldn't you UnLock() here? The comment even says so. @@ +1896,5 @@ > + // ask for new PCM data to be played out using the AudioDeviceBuffer > + // ensure that this callback is executed without taking the > + // audio-thread lock > + WebRtc_UWord32 nSamples = > + _ptrAudioBuffer->RequestPlayoutData(samplesToPlay); And Lock() again here? @@ +1913,5 @@ > + " invalid number of output samples(%d)", nSamples); > + _playWarning = 1; > + } > + > + // Copy data to our direct buffer (held by java sc object) This comment is no longer accurate. @@ +1914,5 @@ > + _playWarning = 1; > + } > + > + // Copy data to our direct buffer (held by java sc object) > + int written = _audioTrack->write(playBuffer, nSamples * 2); Can this function block? If so, I think you need to release the lock beforehand and reacquire it afterwards. @@ +1915,5 @@ > + } > + > + // Copy data to our direct buffer (held by java sc object) > + int written = _audioTrack->write(playBuffer, nSamples * 2); > + _bufferedPlaySamples += (written >> 1); We carefully adjust the buffer size based on the sample format elsewhere in this file, but here you appear to be hard-coding the fact that it is two bytes per sample. Please pick one strategy. @@ +1918,5 @@ > + int written = _audioTrack->write(playBuffer, nSamples * 2); > + _bufferedPlaySamples += (written >> 1); > + > + uint32_t pos; > + _audioTrack->getPosition(&pos); Can this function fail? Should we at least assert that it doesn't advance the position farther than the number of samples we just submitted? @@ +1992,5 @@ > + { > + WebRtc_UWord32 samplesToRec = _samplingFreqIn * 10; > + > + int playDelayInSamples = _bufferedPlaySamples; > + if (playDelayInSamples < 0) Is this something that can actually happen? @@ +2007,5 @@ > + // Check again since recording may have stopped during Java call > + if (_recording) > + { > + // Copy data to our direct buffer (held by java sc object) > + int sizeInBytes = 2 * samplesToRec; Again, you're hard-coding the size here where it was determined by the sample format elsewhere. Please stick with one strategy. @@ +2008,5 @@ > + if (_recording) > + { > + // Copy data to our direct buffer (held by java sc object) > + int sizeInBytes = 2 * samplesToRec; > + int recorderBuffSize = _audioRecord->frameCount() * _audioRecord->frameSize(); Can these functions fail? @@ +2009,5 @@ > + { > + // Copy data to our direct buffer (held by java sc object) > + int sizeInBytes = 2 * samplesToRec; > + int recorderBuffSize = _audioRecord->frameCount() * _audioRecord->frameSize(); > + _audioRecord->read(_recBuffer, sizeInBytes > recorderBuffSize ? recorderBuffSize : sizeInBytes); Can these functions block? If so, I think you need to UnLock() beforehand and Lock() again afterwards. @@ +2019,5 @@ > + // store vqe delay values > + _ptrAudioBuffer->SetVQEData(_delayPlayout, _delayRecording, 0); > + > + // deliver recorded samples at specified sample rate, mic level > + // etc. to the observer using callback Don't you need to UnLock() here? @@ +2020,5 @@ > + _ptrAudioBuffer->SetVQEData(_delayPlayout, _delayRecording, 0); > + > + // deliver recorded samples at specified sample rate, mic level > + // etc. to the observer using callback > + _ptrAudioBuffer->DeliverRecordedData(); And Lock() again here? ::: media/webrtc/trunk/webrtc/modules/audio_device/gonk/audio_device_gonk.h @@ +4,5 @@ > + * Use of this source code is governed by a BSD-style license > + * that can be found in the LICENSE file in the root of the source > + * tree. An additional intellectual property rights grant can be found > + * in the file PATENTS. All contributing project authors may > + * be found in the AUTHORS file in the root of the source tree. You certainly need to update the copyright year. ::: media/webrtc/trunk/webrtc/modules/audio_device/gonk/audio_device_utility_gonk.cc @@ +1,2 @@ > +/* > + * Copyright (c) 2012 The WebRTC project authors. All Rights Reserved. It's 2013. ::: media/webrtc/trunk/webrtc/modules/audio_device/gonk/audio_device_utility_gonk.h @@ +1,2 @@ > +/* > + * Copyright (c) 2011 The WebRTC project authors. All Rights Reserved. It's 2013.

(In reply to Timothy B. Terriberry (:derf) from comment #15) > So, is the plan to merge this code upstream? If not, I think we should > implement it as an "external AudioDeviceModule". This is, for example, the > route Chrome takes to abstract away the real audio device in its renderer > processes. To be clear, I'm okay with either approach, but I don't think we should carry large patches against the upstream code for the long term.

Changelog: - Fix various threading problem: - Holding Mutex while access member variable - Leave critical section when process large data or going to wait event - Rename some utility function which should be called with Mutex locked - Fix various style problem - Clear some redundant code - Fix the samplingRate query logics. Since Gonk device basically use Android native Record device now, no need to query any samplingRate under 44100 when using default value. - Remove some hard coded value

(In reply to Randell Jesup [:jesup] from comment #14) > Comment on attachment 732200 [details] [diff] [review] > Patch V2 > > @@ +303,5 @@ > > + _playThreadIsInitialized = false; > > + } > > + _speakerIsInitialized = false; > > + _playoutDeviceIsSpecified = false; > > + _initialized = false; > > Since we're putting _initialized back to false, should we reset the other > state vars like _shutdownPlayThread? Other values resetted before here. > > @@ +1033,5 @@ > > +#define ERROR_BAD_VALUE -2 > > + > > +int GetMinTrackBufferSize(int sampleRateInHz, int channelCount, int audioFormat) { > > + int frameCount = AudioTrack::getMinFrameCount(&frameCount, AUDIO_STREAM_DEFAULT, sampleRateInHz); > > + int size = frameCount * channelCount * (audioFormat == AUDIO_FORMAT_PCM_16_BIT ? 2 : 1);//native_get_min_buff_size(sampleRateInHz, channelCount, audioFormat); > > sizeof(int16_t or whichever type is being used for audio samples) instead of > '2'. Ditto for '1'. > > Which definition of AUDIO_FORMAT_PCM_16_BIT is being used? And what value > gets put in audioFormat other than it? > > media/libcubeb/src/audiotrack_definitions.h: > typedef enum { > AUDIO_FORMAT_PCM = 0x00000000, > AUDIO_FORMAT_PCM_SUB_16_BIT = 0x1, > AUDIO_FORMAT_PCM_16_BIT = (AUDIO_FORMAT_PCM | AUDIO_FORMAT_PCM_SUB_16_BIT), > } AudioTrack_SampleType; > > or dom/system/gonk/android_audio/AudioSystem.h: > AUDIO_FORMAT_PCM_SUB_16_BIT = 0x1, /* DO NOT CHANGE - PCM > signed 16 bits */ > AUDIO_FORMAT_PCM_SUB_8_BIT = 0x2, /* DO NOT CHANGE - PCM > unsigned 8 bits */ > > AUDIO_FORMAT_PCM = 0x00000000UL, /* DO NOT CHANGE */ > > /* Aliases */ > AUDIO_FORMAT_PCM_16_BIT = (AUDIO_FORMAT_PCM | > AUDIO_FORMAT_PCM_SUB_16_BIT), Basically, I use dom/system/gonk/android_audio/AudioSystem.h, and I checked that they are the same value (0x03) > > @@ +1039,5 @@ > > +} > > + > > +int AudioDeviceGonk::InitPlayback(int sampleRate) > > +{ > > + // get the minimum buffer size that can be used > > I see this is called from within InitPlayout(), which grabs a > CriticalSection, so we're locked here (good) I changed the function name to **Locked. Those function works like utility, and must be called with Mutex hold. > @@ +1710,5 @@ > > + case 44100: > > + samplingFreq = 16000; > > + break; > > + case 16000: > > + samplingFreq = 8000; > > This seems... very odd Since Android HAL do not provide any reliable mechanism to query the samplingRate compatibility, we have to try it until success.

(In reply to Timothy B. Terriberry (:derf) from comment #16) > (In reply to Timothy B. Terriberry (:derf) from comment #15) > > So, is the plan to merge this code upstream? If not, I think we should > > implement it as an "external AudioDeviceModule". This is, for example, the > > route Chrome takes to abstract away the real audio device in its renderer > > processes. > > To be clear, I'm okay with either approach, but I don't think we should > carry large patches against the upstream code for the long term. I had tried to implement this way, but there are some problems: 1. If I implement AudioDeviceModule directly, I would have to duplicate much code from AudioDeviceModuleImpl, since logic is the same. 2. If I inherit from AudioDeviceModuleImpl, I would have to change private members to protected members, which is strange for me. 3. Either case 1 and 2, I will have to remove LINUX/ANDOIRD defines on B2G, but that would be a big change. Since B2G use some code from those platform, I can not remove it directly without change related part... If you think it is better, I think we should file a follow bug for that.

(In reply to Chiajung Hung [:chiajung] from comment #18) > (In reply to Randell Jesup [:jesup] from comment #14) > > Which definition of AUDIO_FORMAT_PCM_16_BIT is being used? And what value > > gets put in audioFormat other than it? > > > > media/libcubeb/src/audiotrack_definitions.h: > > or dom/system/gonk/android_audio/AudioSystem.h: > Basically, I use dom/system/gonk/android_audio/AudioSystem.h, and I checked > that they are the same value (0x03) Part of my concern (which was implied) is that we have two definitions of the same enum which are very different, even if in this case they yield the same value. That can lead to confusion or possibly bugs. If the two could be merged somehow, or if B2G needs an extension to the types in cubeb, perhaps it could be a superset of cubeb instead of a totally different way to build the values. At minimum, there should be comments in the B2G section describing what's going on here and why it's ok. kinetik: can you comment? > > @@ +1710,5 @@ > > > + case 44100: > > > + samplingFreq = 16000; > > > + break; > > > + case 16000: > > > + samplingFreq = 8000; > > > > This seems... very odd > Since Android HAL do not provide any reliable mechanism to query the > samplingRate compatibility, we have to try it until success. Please comment on why this is being done then. Thanks.

(In reply to Chiajung Hung [:chiajung] from comment #19) > (In reply to Timothy B. Terriberry (:derf) from comment #16) > > (In reply to Timothy B. Terriberry (:derf) from comment #15) > > > So, is the plan to merge this code upstream? If not, I think we should > > > implement it as an "external AudioDeviceModule". This is, for example, the > > > route Chrome takes to abstract away the real audio device in its renderer > > > processes. > > > > To be clear, I'm okay with either approach, but I don't think we should > > carry large patches against the upstream code for the long term. > > I had tried to implement this way, but there are some problems: > > 1. If I implement AudioDeviceModule directly, I would have to duplicate much > code from AudioDeviceModuleImpl, since logic is the same. > 2. If I inherit from AudioDeviceModuleImpl, I would have to change private > members to protected members, which is strange for me. > 3. Either case 1 and 2, I will have to remove LINUX/ANDOIRD defines on B2G, > but that would be a big change. Since B2G use some code from those platform, > I can not remove it directly without change related part... > > If you think it is better, I think we should file a follow bug for that. I suspect #1 makes the most sense, though you'll need to monitor AudioDeviceModuleImpl for any changes you want to uplift, or you could refactor AudioDeviceModuleImpl into a class holding the "generic" stuff you'd like to inherit, and then a wrapper class around that with anything you don't think is general enough. I'm not sure it's worth the work or that such a change would be easily accepted. Instead of removing #ifdef ANDROID/etc's, you might consider making a new define which is set for ANDROID or B2G. Tim, what do you think?

(In reply to Randell Jesup [:jesup] from comment #20) > (In reply to Chiajung Hung [:chiajung] from comment #18) > > (In reply to Randell Jesup [:jesup] from comment #14) > > > > > Which definition of AUDIO_FORMAT_PCM_16_BIT is being used? And what value > > > gets put in audioFormat other than it? > > > > > > media/libcubeb/src/audiotrack_definitions.h: > > > or dom/system/gonk/android_audio/AudioSystem.h: > > > Basically, I use dom/system/gonk/android_audio/AudioSystem.h, and I checked > > that they are the same value (0x03) > > Part of my concern (which was implied) is that we have two definitions of > the same enum which are very different, even if in this case they yield the > same value. That can lead to confusion or possibly bugs. If the two could > be merged somehow, or if B2G needs an extension to the types in cubeb, > perhaps it could be a superset of cubeb instead of a totally different way > to build the values. > > At minimum, there should be comments in the B2G section describing what's > going on here and why it's ok. kinetik: can you comment? I think libcubeb in fact duplicate those values in AudioSystem.h to avoid direct inclusion just like it dlopen libmedia.so to avoid direct link to Android libraries. (Since these headers are not exposed to Android NDK) That means libcubeb *must* sync all these values if AOSP change them. And since B2G can include those headers directly, I do not have to maintain the static value and avoid most problem. By the way, it seems to me that libcubeb is built on top of android::AudioTrack which I use in this module. That means this module would placed at the same layer as libcubeb in the architechture. And I can not use libcubeb to implement this module since I can not find AudioRecord capability in libcubeb.

(In reply to Randell Jesup [:jesup] from comment #21) > (In reply to Chiajung Hung [:chiajung] from comment #19) > > (In reply to Timothy B. Terriberry (:derf) from comment #16) > > > (In reply to Timothy B. Terriberry (:derf) from comment #15) > > > > So, is the plan to merge this code upstream? If not, I think we should > > > > implement it as an "external AudioDeviceModule". This is, for example, the > > > > route Chrome takes to abstract away the real audio device in its renderer > > > > processes. > > > > > > To be clear, I'm okay with either approach, but I don't think we should > > > carry large patches against the upstream code for the long term. > > > > I had tried to implement this way, but there are some problems: > > > > 1. If I implement AudioDeviceModule directly, I would have to duplicate much > > code from AudioDeviceModuleImpl, since logic is the same. > > 2. If I inherit from AudioDeviceModuleImpl, I would have to change private > > members to protected members, which is strange for me. > > 3. Either case 1 and 2, I will have to remove LINUX/ANDOIRD defines on B2G, > > but that would be a big change. Since B2G use some code from those platform, > > I can not remove it directly without change related part... > > > > If you think it is better, I think we should file a follow bug for that. > > I suspect #1 makes the most sense, though you'll need to monitor > AudioDeviceModuleImpl for any changes you want to uplift, or you could > refactor AudioDeviceModuleImpl into a class holding the "generic" stuff > you'd like to inherit, and then a wrapper class around that with anything > you don't think is general enough. I'm not sure it's worth the work or that > such a change would be easily accepted. > > Instead of removing #ifdef ANDROID/etc's, you might consider making a new > define which is set for ANDROID or B2G. > > Tim, what do you think? In fact, those defines are reside in configure file/gyp file and affect *all* WebRTC modules. The most simple (but dirty) way to avoid these defines in audio_module only I can think is #ifdef WEBRTC_GONK #undef WEBRTC_ANDROID #undef WEBRTC_LINUX in the audio_device_impl.cc...which is not that clear for me.

(In reply to Chiajung Hung [:chiajung] from comment #22) > I think libcubeb in fact duplicate those values in AudioSystem.h to avoid > direct inclusion just like it dlopen libmedia.so to avoid direct link to > Android libraries. (Since these headers are not exposed to Android NDK) That's right. > That means libcubeb *must* sync all these values if AOSP change them. And > since B2G can include those headers directly, I do not have to maintain the > static value and avoid most problem. The copies in libcubeb should be exact copies of the AOSP versions, with the exception of a C/C++ compile fix that will break in a fairly obvious way if it's reverted. The versions in dom/system/gonk/android_audio were originally at media/libsydneyaudio/src/gonk and also included from dom/system/gonk. I moved them to dom/system/gonk when sydneyaudio was removed, but we should remove that copy and have everything in-tree use the versions in libcubeb if possible.

(In reply to Randell Jesup [:jesup] from comment #21) > (In reply to Chiajung Hung [:chiajung] from comment #19) > > I had tried to implement this way, but there are some problems: > > > > 1. If I implement AudioDeviceModule directly, I would have to duplicate much > > code from AudioDeviceModuleImpl, since logic is the same. > > 2. If I inherit from AudioDeviceModuleImpl, I would have to change private > > members to protected members, which is strange for me. > > 3. Either case 1 and 2, I will have to remove LINUX/ANDOIRD defines on B2G, > > but that would be a big change. Since B2G use some code from those platform, > > I can not remove it directly without change related part... > > > > If you think it is better, I think we should file a follow bug for that. > > I suspect #1 makes the most sense, though you'll need to monitor > AudioDeviceModuleImpl for any changes you want to uplift, or you could > refactor AudioDeviceModuleImpl into a class holding the "generic" stuff > you'd like to inherit, and then a wrapper class around that with anything > you don't think is general enough. I'm not sure it's worth the work or that > such a change would be easily accepted. > > Instead of removing #ifdef ANDROID/etc's, you might consider making a new > define which is set for ANDROID or B2G. > > Tim, what do you think? Adding new define needs to modify the source code and we need to implement "another AudioDeviceModuleImpl" that is very similar to the current one. I think the current implementation fits the "creation style" of AudioDeviceModuleImpl. For me, it's easier to read. I know it increases some effort when we need to merge from upstream. Could we use this version first? If it really causes problems, we can try other methods. How do you think?

(In reply to Matthew Gregan [:kinetik] from comment #24) > (In reply to Chiajung Hung [:chiajung] from comment #22) > > I think libcubeb in fact duplicate those values in AudioSystem.h to avoid > > direct inclusion just like it dlopen libmedia.so to avoid direct link to > > Android libraries. (Since these headers are not exposed to Android NDK) > > That's right. > > > That means libcubeb *must* sync all these values if AOSP change them. And > > since B2G can include those headers directly, I do not have to maintain the > > static value and avoid most problem. > > The copies in libcubeb should be exact copies of the AOSP versions, with the > exception of a C/C++ compile fix that will break in a fairly obvious way if > it's reverted. > > The versions in dom/system/gonk/android_audio were originally at > media/libsydneyaudio/src/gonk and also included from dom/system/gonk. I > moved them to dom/system/gonk when sydneyaudio was removed, but we should > remove that copy and have everything in-tree use the versions in libcubeb if > possible. Sorry, I found a mistake here... In fact, I include the Android's header directly, which is under B2G/system/core/include/system/audio.h And I think it is the origin copy or these defines.

Add a comment about samplingRate configuration.

Comment on attachment 735053 [details] [diff] [review] Patch V3 Review of attachment 735053 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Chiajung Hung [:chiajung] from comment #19) > I had tried to implement this way, but there are some problems: > ... > If you think it is better, I think we should file a follow bug for that. Okay, let's try to get this upstream then. There's detailed instructions for submitting patches here: <http://www.webrtc.org/reference/contributing>. If they refuse to take it, then we can look more closely at the external AudioDeviceModule approach. But first, this patch still gets an r-, since I think you may have added almost as many threading bugs as you fixed. ::: media/webrtc/trunk/webrtc/modules/audio_device/gonk/audio_device_gonk.cc @@ +164,5 @@ > + > + // Check the sample rate to be used for playback and recording > + // and the max playout volume > + UnLock(); > + if (InitSampleRate() != 0) This is private, only called from here, and immediately re-acquires the lock. Can we just avoid this extra locking/unlocking? @@ +227,5 @@ > +// ---------------------------------------------------------------------------- > + > +WebRtc_Word32 AudioDeviceGonk::Terminate() > +{ > + Lock(); This used to be a CriticalSectionScoped lock. You've now made it possible to acquire the lock and not release it before returning. @@ +231,5 @@ > + Lock(); > + > + if (!_initialized) > + { > + return 0; E.g., right here. @@ +235,5 @@ > + return 0; > + } > + > + // RECORDING > + UnLock(); CriticalSectionWrapper allows recursive locking. No need to unlock before calling another function which locks (unless that function needs to actually release the lock). @@ +242,5 @@ > + _shutdownRecThread = true; > + _timeEventRec.Set(); // Release rec thread from waiting state > + if (_ptrThreadRec) > + { > + UnLock(); Tab. @@ +252,5 @@ > + "%s: Recording thread shutdown timed out, cannot " > + "terminate thread", > + __FUNCTION__); > + // If we close thread anyway, the app will crash > + return -1; I think this is a bug in the actual Android code you copied from. If you return here, you won't own the lock, but if you use a CriticalSectionScoped lock, it will try to Leave() it anyway. What you really want is an analog to our own ReentrantMonitorAutoExit that works with WrappedCriticalSection. Please file that bug upstream. @@ +262,5 @@ > + ThreadWrapper* tmpThread = _ptrThreadRec; > + _ptrThreadRec = NULL; > + UnLock(); > + tmpThread->SetNotAlive(); > + Lock(); I think you need to move this down below the if/else. You want the other thread to be able to make progress in order for Stop() to succeed. @@ +281,5 @@ > + _micIsInitialized = false; > + _recordingDeviceIsSpecified = false; > + > + // PLAYOUT > + UnLock(); Unnecessary. See above. @@ +298,5 @@ > + "%s: Playout thread shutdown timed out, cannot " > + "terminate thread", > + __FUNCTION__); > + // If we close thread anyway, the app will crash > + return -1; Same upstream bug as above. @@ +308,5 @@ > + ThreadWrapper* tmpThread = _ptrThreadPlay; > + _ptrThreadPlay = NULL; > + UnLock(); > + tmpThread->SetNotAlive(); > + Lock(); Move this below if the if/else, for the same reason as above. @@ +336,5 @@ > +// ---------------------------------------------------------------------------- > + > +bool AudioDeviceGonk::Initialized() const > +{ > + CriticalSectionScoped lock(&_critSect); If this variable really is used from multiple threads, then I don't think it's possible to implement this API correctly. The value you return can instantly be out-of-date. Please investigate (and document!) whether this is actually needed instead of adding it blindly "just in case". Some of the locks you added are clearly unnecessary. Please investigate all of them, and file upstream bugs on any of the ones that _are_ necessary, but not locked in the Android implementation. @@ +346,5 @@ > +// ---------------------------------------------------------------------------- > + > +WebRtc_Word32 AudioDeviceGonk::SpeakerIsAvailable(bool& available) > +{ > + CriticalSectionScoped lock(&_critSect); You're not accessing any non-local variables here. Please don't lock. @@ +388,5 @@ > +// ---------------------------------------------------------------------------- > + > +WebRtc_Word32 AudioDeviceGonk::MicrophoneIsAvailable(bool& available) > +{ > + CriticalSectionScoped lock(&_critSect); You're not accessing any non-local variables here. Please don't lock. @@ +450,5 @@ > +// ---------------------------------------------------------------------------- > + > +WebRtc_Word32 AudioDeviceGonk::SpeakerVolumeIsAvailable(bool& available) > +{ > + CriticalSectionScoped lock(&_critSect); You're not accessing any non-local variables here. Please don't lock. @@ +477,5 @@ > + " Speaker not initialized"); > + return -1; > + } > + > + _audioFlinger->setStreamVolume(AUDIO_STREAM_MUSIC, static_cast<float>(volume), 0); Wrap at 80 characters. @@ +1100,5 @@ > + _audioTrack = new AudioTrack(); > + _audioTrack->set(AUDIO_STREAM_MUSIC, // streamType > + sampleRate, // sampleRate > + N_AUDIO_FORMAT, // format > + N_PLAY_CHANNELS == 1 ? AUDIO_CHANNEL_OUT_MONO : AUDIO_CHANNEL_OUT_STEREO, // channelMask Wrap at 80 characters (store this in a temporary variable instead of computing it inline in the middle of the function call if you need to). @@ +1117,5 @@ > + } > + > + // XXX: Since there is no mechnism to determine MAX_VOLUME on Gonk now, > + // default to (MAX_STREAM_VOLUME[STREAM_VOICE_CALL] + 5) / 10, where > + // MAX_STREAM_VOLUME[STREAM_VOICE_CALL] = 5, from AudioService.java STREAM_MUSIC? @@ +1123,5 @@ > +} > + > +WebRtc_Word32 AudioDeviceGonk::InitPlayout() > +{ > + Lock(); This used to be a CriticalSectionScoped lock. Now you continue to hold the lock after every single error return in this function, making them all into bugs. @@ +1129,5 @@ > + if (!_initialized) > + { > + WEBRTC_TRACE(kTraceError, kTraceAudioDevice, _id, > + " Not initialized"); > + return -1; E.g., here. @@ +1154,5 @@ > + return 0; > + } > + > + // Initialize the speaker > + UnLock(); Unnecessary. See above. @@ +1188,5 @@ > + > + return retVal; > +} > + > +int GetPCMSampleSize(int audioFormat) This should be static. @@ +1228,5 @@ > +int AudioDeviceGonk::InitRecorderLocked(int audioSource, int sampleRate) > +{ > + // get the minimum buffer size that can be used > + int minRecBufSize = GetMinRecordBufferSize(sampleRate, > + N_REC_CHANNELS, // Single channel Wrap at 80 characters (or kill this comment completely, since it's not accurate if someone changes the constant). @@ +1231,5 @@ > + int minRecBufSize = GetMinRecordBufferSize(sampleRate, > + N_REC_CHANNELS, // Single channel > + N_AUDIO_FORMAT); > + > + // double size to be more safer Safe from what? Also "safer" not "more safe". @@ +1242,5 @@ > + delete _audioRecord; > + _audioRecord = NULL; > + } > + > + unsigned int frameCount = recBufSize / 2; This is only correct for 16-bit mono or 8-bit stereo, right? @@ +1248,5 @@ > + _audioRecord = new AudioRecord(); > + _audioRecord->set(audioSource, // inputSource > + sampleRate, // sampleRate > + N_AUDIO_FORMAT, // format > + N_REC_CHANNELS == 1 ? AUDIO_CHANNEL_IN_MONO : AUDIO_CHANNEL_IN_STEREO, // channelMask Wrap at 80 characters. @@ +1266,5 @@ > +} > + > +WebRtc_Word32 AudioDeviceGonk::InitRecording() > +{ > + Lock(); Used to be a CriticalSectionScoped lock. Same bugs as above. @@ +1297,5 @@ > + return 0; > + } > + > + // Initialize the microphone > + UnLock(); Unnecessary. See above. @@ +1340,5 @@ > +// ---------------------------------------------------------------------------- > + > +WebRtc_Word32 AudioDeviceGonk::StartRecording() > +{ > + Lock(); Used to be a CriticalSectionScoped lock. Same bugs as above. @@ +1441,5 @@ > +// ---------------------------------------------------------------------------- > + > +WebRtc_Word32 AudioDeviceGonk::StartPlayout() > +{ > + Lock(); Used to be a CriticalSectionScoped lock. Same bugs as above. @@ +1714,5 @@ > +// ---------------------------------------------------------------------------- > + > +WebRtc_Word32 AudioDeviceGonk::SetLoudspeakerStatus(bool enable) > +{ > + CriticalSectionScoped lock(&_critSect); Tab. @@ +1715,5 @@ > + > +WebRtc_Word32 AudioDeviceGonk::SetLoudspeakerStatus(bool enable) > +{ > + CriticalSectionScoped lock(&_critSect); > + status_t err = AudioSystem::setForceUse((audio_policy_force_use_t)0, (audio_policy_forced_cfg_t)enable); Wrap at 80 characters. @@ +1730,5 @@ > +// ---------------------------------------------------------------------------- > + > +WebRtc_Word32 AudioDeviceGonk::GetLoudspeakerStatus(bool& enabled) const > +{ > + CriticalSectionScoped lock(&_critSect); Tab. @@ +1750,5 @@ > +// ---------------------------------------------------------------------------- > + > +WebRtc_Word32 AudioDeviceGonk::InitSampleRate() > +{ > + CriticalSectionScoped lock(&_critSect); Tab. @@ +1768,5 @@ > + samplingFreq); > + } > + > + res = InitRecorderLocked(_recAudioSource, samplingFreq); > + // According to Android SDK, 44100 is guaranteed to work on all devices, when error occurs here, default to 44100 Wrap at 80 characters. @@ +1773,5 @@ > + if (res < 0 && samplingFreq != 48000) > + { > + WEBRTC_TRACE(kTraceError, > + kTraceAudioDevice, _id, > + "%s: InitRecording failed (%d)", __FUNCTION__, This function's name is InitSampleRate. @@ +1777,5 @@ > + "%s: InitRecording failed (%d)", __FUNCTION__, > + res); > + return -1; > + } > + InitRecorderLocked(_recAudioSource, 44100); Lots of tabs. If possible, please configure your editor to never insert tabs, ever. Also, the second call to InitRecorderLock() is outside the if, so you always execute it. You also need to check the error return. Even if the device supports 44100, the call could still fail for other reasons. You also don't update samplingFreq, so all the calculations involving it are wrong in the case you need the 44100 fallback. @@ +1811,5 @@ > + } > + // else use same as recording > + } > + > + // Since Android HAL does not provide any reliable way to query its samplingRate capacity samplingFreq, for consistency. You also probably mean "capability" instead of "capacity", but "which samplingFreqs it supports" would be better still. Also, wrap at 80 characters. @@ +1812,5 @@ > + // else use same as recording > + } > + > + // Since Android HAL does not provide any reliable way to query its samplingRate capacity > + // We have to try it until success "we have to try them until one succeeds." @@ +1822,5 @@ > + { > + switch (samplingFreq) > + { > + case 48000: > + samplingFreq = 48000; So after 48000 fails you try... 48000 again? This will never succeed. @@ +1833,5 @@ > + break; > + default: // error > + WEBRTC_TRACE(kTraceError, > + kTraceAudioDevice, _id, > + "InitPlaybackLocked failed (%d)", res); This function's name is InitSampleRate. @@ +1891,5 @@ > +// ---------------------------------------------------------------------------- > + > +bool AudioDeviceGonk::PlayThreadProcess() > +{ > + Lock(); Tab. @@ +1900,5 @@ > + } > + > + if (!_playing) > + { > + UnLock(); Tab. @@ +1935,5 @@ > + } > + > + if (_playing) > + { > + WebRtc_Word8 playBuffer[REC_BUF_SIZE_IN_SAMPLES * GetPCMSampleSize(AUDIO_FORMAT_PCM_16_BIT)]; // Max 10 ms @ 48 kHz / 16 bit MSVC does not support variable-length arrays, and I doubt the compiler is smart enough to realize this is a constant. I'm not sure what motivated this change, but you didn't actually hoist the calculation out somewhere else like jesup suggested, and in fact have made it misleading since you are using "REC_BUF_SIZE..." for the playback buffer size (which will confuse someone who tries to change the constant). @@ +1966,5 @@ > + > + // Copy data to our direct buffer > + UnLock(); > + const int sampleSize = GetPCMSampleSize(N_AUDIO_FORMAT); > + int written = _audioTrack->write(playBuffer, nSamples * N_PLAY_CHANNELS * sampleSize); Since you didn't answer my question, I looked it up myself. According to <http://developer.android.com/reference/android/media/AudioTrack.html#write%28byte[],%20int,%20int%29> this function can, in fact fail, which means you need to handle that failure. Also, wrap at 80 characters. @@ +1969,5 @@ > + const int sampleSize = GetPCMSampleSize(N_AUDIO_FORMAT); > + int written = _audioTrack->write(playBuffer, nSamples * N_PLAY_CHANNELS * sampleSize); > + Lock(); > + _bufferedPlaySamples += (written / (sampleSize * N_PLAY_CHANNELS)); > + WS. @@ +1971,5 @@ > + Lock(); > + _bufferedPlaySamples += (written / (sampleSize * N_PLAY_CHANNELS)); > + > + uint32_t pos; > + _audioTrack->getPosition(&pos); If we're relying on it, I still think we should assert that pos >= _playPosition. @@ +2009,5 @@ > + > + // just sleep if rec has not started > + if (!_recording) > + { > + UnLock(); Tab. @@ +2045,5 @@ > + { > + WebRtc_UWord32 samplesToRec = _samplingFreqIn * 10; > + > + int playDelayInSamples = _bufferedPlaySamples; > + if (playDelayInSamples < 0) When this was calling a Java method which would fail on short reads, I think it made sense to check for failure. I'm not sure it makes sense here. If we think the number of samples that are buffered for playback is negative, something has gone badly wrong, and it's a bug in our code, not a failure in the system. I would just assert. @@ +2060,5 @@ > + // Copy data to our direct buffer > + // NOTE: Do not unlock here! since the read function manipulate _recBuffer directly > + int sizeInBytes = GetPCMSampleSize(N_AUDIO_FORMAT) * samplesToRec; > + int recorderBuffSize = _audioRecord->frameCount() * _audioRecord->frameSize(); > + _audioRecord->read(_recBuffer, sizeInBytes > recorderBuffSize ? recorderBuffSize : sizeInBytes); I'm still not sure about frameCount() and frameSize(), because it's hard to tell what the corresponding Android functions really are, but <http://developer.android.com/reference/android/media/AudioRecord.html#read%28byte[],%20int,%20int%29> certainly claims read() can fail, which means you have to handle that failure. That documentation doesn't say if read() blocks, but since you (and the Android backend) seem to always expect it to return 10 ms of data, and nothing else delays you between calls to read(), it must. That means you need to release the lock. The only other thread that touches _recBuffer is the one that calls Init(), but this happens before the recording thread is created, which gives those writes a strict happens-before relationship with anything that happens on this thread. Therefore I don't think you need to hold a lock to write into _recBuffer. Also, all of the above needs to wrap at 80 characters. ::: media/webrtc/trunk/webrtc/modules/audio_device/gonk/audio_device_gonk.h @@ +25,5 @@ > +namespace webrtc > +{ > +class EventWrapper; > + > +const WebRtc_UWord32 N_REC_SAMPLES_PER_SEC = 44000; // Default is 44.1 kHz Upstream is likely to complain about the WebRTC_* integer types (they have stopped using them and are migrating away from them). You should use the stdint types wherever you can.

(In reply to Timothy B. Terriberry (:derf) from comment #28) > Comment on attachment 735053 [details] [diff] [review] > Patch V3 > > Review of attachment 735053 [details] [diff] [review]: > ----------------------------------------------------------------- > > (In reply to Chiajung Hung [:chiajung] from comment #19) > > I had tried to implement this way, but there are some problems: > > ... > > If you think it is better, I think we should file a follow bug for that. > > Okay, let's try to get this upstream then. There's detailed instructions for > submitting patches here: <http://www.webrtc.org/reference/contributing>. If > they refuse to take it, then we can look more closely at the external > AudioDeviceModule approach. > > But first, this patch still gets an r-, since I think you may have added > almost as many threading bugs as you fixed. OK, let me pass your review first, and then figure out how to commit it to upstream. > @@ +336,5 @@ > > +// ---------------------------------------------------------------------------- > > + > > +bool AudioDeviceGonk::Initialized() const > > +{ > > + CriticalSectionScoped lock(&_critSect); > > If this variable really is used from multiple threads, then I don't think > it's possible to implement this API correctly. The value you return can > instantly be out-of-date. Please investigate (and document!) whether this is > actually needed instead of adding it blindly "just in case". Some of the > locks you added are clearly unnecessary. Please investigate all of them, and > file upstream bugs on any of the ones that _are_ necessary, but not locked > in the Android implementation. I will double-check this part, and compare with Android implementation. > @@ +1231,5 @@ > > + int minRecBufSize = GetMinRecordBufferSize(sampleRate, > > + N_REC_CHANNELS, // Single channel > > + N_AUDIO_FORMAT); > > + > > + // double size to be more safer > > Safe from what? Also "safer" not "more safe". This is copied from Android version, too. Basically, we have to pass a buffer size bigger than minimum to make AudioRecord happy, and Android team decided to double this value. And I think if we poll data from AudioRecord too slow and the buffer size is small, we may lost some sample or cause some problem in AudioRecord. I will try to check AudioRecord source code to see what happens if buffer size is small. > > @@ +1935,5 @@ > > + } > > + > > + if (_playing) > > + { > > + WebRtc_Word8 playBuffer[REC_BUF_SIZE_IN_SAMPLES * GetPCMSampleSize(AUDIO_FORMAT_PCM_16_BIT)]; // Max 10 ms @ 48 kHz / 16 bit > > MSVC does not support variable-length arrays, and I doubt the compiler is > smart enough to realize this is a constant. > I'm not sure what motivated this change, but you didn't actually hoist the > calculation out somewhere else like jesup suggested, and in fact have made > it misleading since you are using "REC_BUF_SIZE..." for the playback buffer > size (which will confuse someone who tries to change the constant). I am not sure I understood you about "hoist this to somewhere else". Here we just need a big enough buffer to save 10ms samples using max configuration possible. That's why it say the buffer size is 10ms@48kHz/16bit. > > @@ +1966,5 @@ > > + > > + // Copy data to our direct buffer > > + UnLock(); > > + const int sampleSize = GetPCMSampleSize(N_AUDIO_FORMAT); > > + int written = _audioTrack->write(playBuffer, nSamples * N_PLAY_CHANNELS * sampleSize); > > Since you didn't answer my question, I looked it up myself. According to > <http://developer.android.com/reference/android/media/AudioTrack. > html#write%28byte[],%20int,%20int%29> this function can, in fact fail, which > means you need to handle that failure. > > Also, wrap at 80 characters. Sorry for miss this part in last review. Yes, you are right, I checked the native counterpart for this function and it can fail with negative return value. > > @@ +2060,5 @@ > > + // Copy data to our direct buffer > > + // NOTE: Do not unlock here! since the read function manipulate _recBuffer directly > > + int sizeInBytes = GetPCMSampleSize(N_AUDIO_FORMAT) * samplesToRec; > > + int recorderBuffSize = _audioRecord->frameCount() * _audioRecord->frameSize(); > > + _audioRecord->read(_recBuffer, sizeInBytes > recorderBuffSize ? recorderBuffSize : sizeInBytes); > > I'm still not sure about frameCount() and frameSize(), because it's hard to > tell what the corresponding Android functions really are, but > <http://developer.android.com/reference/android/media/AudioRecord. > html#read%28byte[],%20int,%20int%29> certainly claims read() can fail, which > means you have to handle that failure. Sorry for miss this part in last review. This function is defined in AudioRecord.h frameCount() just return a member value of AudioRecord object which assigned as soon as AudioRecord::set called and succeeded. frameSize() does some calculation based on the parameter send to AudioRecord::set function. They can not fail if AudioRecord initialized without problem.

I had reviewed all mutex usage and compared with Android then create this patch. Here is a simple list about member data and thread in audio_device: ProcessThread: PlayoutWarning PlayoutError RecordingWarning RecordingError MediaThread: (GetUserMediaRunnable, MediaOperationRunnable) Other operation on VoE RecThread: _recThreadIsInitialized _recording _id _startRec _recording _recWarning _recError _samplingFreqIn _bufferedPlaySamples _delayPlayout _samplingFreqOut _audioRecord _recBuffer _ptrAudioBuffer _delayRecording _ptrAudioBuffer _shutdownRecThread _recStartStopEvent PlayThread: _playThreadIsInitialized _playing _id _startPlay _playWarning _playError _samplingFreqOut _audioTrack _ptrAudioBuffer _bufferedPlaySamples _playPosition _delayPlayout _shutdownPlayThread And after check against this list, I think some mutex handling in Android maybe questionable: AudioDeviceAndroidJni::Recording() : _recording AudioDeviceAndroidJni::PlayoutDelay() : _delayPlayout AudioDeviceAndroidJni::RecordingDelay : _delayRecording AudioDeviceAndroidJni::Playing() : _playing AudioDeviceAndroidJni::PlayoutBuffer() : _delayPlayout AudioDeviceAndroidJni::PlayoutWarning() : _playWarning AudioDeviceAndroidJni::PlayoutError() : _playError AudioDeviceAndroidJni::RecordingWarning() : _recWarning AudioDeviceAndroidJni::RecordingError() : _recError AudioDeviceAndroidJni::ClearPlayoutWarning() : _playWarning AudioDeviceAndroidJni::ClearPlayoutError() : _playError AudioDeviceAndroidJni::ClearRecordingWarning() : _recWarning AudioDeviceAndroidJni::ClearRecordingError() : _recError AudioDeviceAndroidJni::InitSampleRate() : _samplingFreqIn, _samplingFreqOut AudioDeviceAndroidJni::PlayThreadProcess() : _playing, _playWarning AudioDeviceAndroidJni::RecThreadProcess() : _recording, _recWarning for example, RecThreadProcess runs on recording thread, and it may change value or _recWarning which can be read from ProcessThread via RecordingWarning call.

I can easily believe that the existing android implementation is missing locks around accesses to certain variables (and indicated that in my earlier review I believe).

Yes, that's why I list them here. I just not sure whether they access those variables in a strict order that no mutex locking needed. But I think error handling related variables and device state variables access are really bad in original Android version.

Well, if they're accessed from two threads without locking, expect in special cases like "set on thread A before thread B starts" or "set on thread A, then message sent (with locking) to thread B which reads it (and thread A doesn't set it again)", there's a problem. It's not just an ordering thing; data races can cause random variable corruption (even variables totally unrelated to the variable with the data race). Thread a: x = 1; Thread b: y = x; can corrupt variable z (bizarre, but true). http://software.intel.com/en-us/blogs/2013/01/06/benign-data-races-what-could-possibly-go-wrong http://corensic.wordpress.com/2011/06/07/benign-data-races-considered-harmful/ http://www.reddit.com/r/programming/comments/19rtbn/benign_data_races_what_could_possibly_go_wrong/ http://corensic.wordpress.com/page/2/

Thanks for the information. If we have to only take care about those 2 conditions, than most Android questionable in my list should become Android problematic then :p

Yes, likely most of these are dangerous data races in Android.

This patch a a plain copy of upstream audio_device_impl

This patch contains the diff to make duplicated Audio device impl gonk only.

I uploaded a new patch that is just the duplicated code so I can look at it in isolation rather than as a diff.

Duplicate audio_device to another place for implement the audio device as external device.

Move AudioDeviceGonk to another place, and make it indepenedent from GIPS. Since we do not use playback capability of this device, stub out all playback related functions.

This is ekr's patch from http://firefox-codereview.appspot.com/19001 cleaned and fixed up. Updates: - Fixed crash on my unagi (mInitialized wasn't initialized) - Fixed no-op Terminate() - Doubled recording buffer (was at minimum). - Fixed TODOs: - Replaced PR_Now() with xpcom/ds/TimeStamp which can't go backwards. - Uses PR_LOG() throughout over WEBRTC_TRACE Tested with gUM and call, and seems to work.

Jan-Ivar, You should take a look at the issues I raised in my email when I handed off this patch: 1. The synchronous calls to the worker thread block on the worker thread's read off of mAudioRec. I don't know what the longest block is here, but that could be an issue. The fix is to have these things be asynchronous and report back errors in some other way. Note: this is a preexisting issue. 2. If you look at the Google ADM, you can see that their stubs all just return 0. We should consider that: https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/media/webrtc_audio_device_not_impl.cc&sq=package:chromium 3. I'm concerned about taking the result of PR_Now() /1000 and assigning it to a 32-bit. This is in milliseconds, right? We probably want to use PR_Interval... 4. We should consider taking all the WEBRTC_TRACE calls and making them use PR_log() 5. We still need an audit of all the members used on two different threads, as well as of all the locks. 6. We should also consider doing WebRTC_Word32 -> uint32_t, etc.

Regarding #6, the 3.30 release I'm about to make (now delayed until 25 opens to avoid destabilizing Android) will swap most or all of webrtc.org code from WebRTC_Word* to int*_t, so might as well make new code int*_t as well. 32bit unsigned ms is 49 days. And PR_Now likely starts at boot time. If the code handles or doesn't care about wraparound, this isn't a problem; if it does other things with the result, then 64 bits will avoid it for a Long Time.

Comment on attachment 762587 [details] [diff] [review] Gonk audio recording device. cleanup + works (3) Review of attachment 762587 [details] [diff] [review]: ----------------------------------------------------------------- Clearing review until the issues in comment 43 are addressed.

> 32bit unsigned ms is 49 days. And PR_Now likely starts at boot time. The latest patch doesn't use PR_Now anymore since it can go backwards. Instead it uses a 64-bit internal ticks, with a TimeDuration.ToMilliseconds() function that returns the delta as a double, which is then stuffs into a 32 bit integer just fine. (not sure what that does to performance, but it should be stable and unlimited).

Another question. The current patch is all green on b2g: https://tbpl.mozilla.org/?tree=Try&rev=2df30fcc5155 But fails to build on everything else: https://tbpl.mozilla.org/?tree=Try&rev=cccb56f92902 media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp: In member function 'void mozilla::MediaPipelineReceiveVideo::PipelineListener::RenderVideoFrame (const unsigned char*, unsigned int, uint32_t, int64_t)': media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:1112:66: error: 'format' was not declared in this scope media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp: At global scope: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:787:13: warning: 'void mozilla::FillBlackYCbCr420PixelData(uint8_t*, const gfxIntSize&)' defined but not used [-Wunused-function] make[7]: Leaving directory `/builds/slave/try-lx-00000000000000000000000/build/obj-firefox/media/webrtc/signaling/signaling_ecc' make[7]: *** [src/mediapipeline/MediaPipeline.o] Error 1 make[6]: *** [signaling_ecc_libs] Error 2 make[6]: *** Waiting for unfinished jobs.... I pushed the try from my B2G copy of mozilla-central from a couple of days ago.

media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp, line 1112: // Create a video frame and append it to the track. #ifdef MOZ_WIDGET_GONK // ImageFormat format = GRALLOC_PLANAR_YCBCR; //#else ImageFormat format = PLANAR_YCBCR; #endif

Comment on attachment 762587 [details] [diff] [review] Gonk audio recording device. cleanup + works (3) Clearing review flags until comments addressed as well.

> //#else Found it. It was in patch 0010-Fix-display-problem.patch

Is there some reason not just use PR_IntervalNow and friends (i.e. the standard way to do this in Gecko code)?

I was going to and someone mentioned the limitations with wraparound and 32-bit epoch range, and then someone mentioned TimeStamp. Looking again I see the "epoch" language is what threw me, and that I should be able to use PR_IntervalNow by effectively updating the epoch each call and convert deltas to milliseconds asap.

Updated to use PR_IntervalNow(), as discussed.

This is the refactor to rely on AudioRecord's callback api. - Many of the critical sections no longer seem needed, but when I started removing them, I hit no wall that told me where to stop (some of the virtual methods gave me pause), so I left them all. Input welcome. - AudioRecord's minFrameCount is 1024, and I'm now concerned if using callbacks adds latency over the old 10 ms read-approach, not sure. If so, maybe EVENT_MARKER can be used over EVENT_MORE_DATA to improve granularity? Has anyone used those before?

Comment on attachment 768378 [details] [diff] [review] Gonk audio recording device. cleanup + works (4) Review of attachment 768378 [details] [diff] [review]: ----------------------------------------------------------------- The 44100/44000 stuff can be adjusted in the "remove 44000" patch from bug 886886, though please link this to that or make comments there when this lands (or if that lands first, fix this). The TSAN issues need to be resolved License issues need to be resolved r-, but these should all be resolvable. You can run this by Ted for a trivial build-peer review (or ask if it even needs review by a peer) ::: content/media/webrtc/AudioDeviceGonk.cpp @@ +57,5 @@ > + int frameCount = 0; > + status_t result = AudioRecord::getMinFrameCount(&frameCount, > + sampleRate, > + audioFormat, > + nbChannels); This seems to come from somewhere in the hardware audio drivers via get_input_buffer_size() @@ +307,5 @@ > + int samplingFreq = 44100; > + if (mSamplingFreqIn != 44) > + { > + samplingFreq = mSamplingFreqIn * 1000; > + } We're going to want to use 44100 directly once bug 886886 lands. Until then, a kludge like this is needed, but will cause 0.23% audio drift (delay). So all these *1000's are going to need to go away, and we'll need to store real frequencies. @@ +323,5 @@ > + } > + else > + { > + // Set the audio device buffer sampling rate > + mAudioDeviceBuffer.SetRecordingSampleRate(mSamplingFreqIn * 1000); ditto on *1000 @@ +326,5 @@ > + // Set the audio device buffer sampling rate > + mAudioDeviceBuffer.SetRecordingSampleRate(mSamplingFreqIn * 1000); > + > + // the init rec function returns a fixed delay > + mDelayRecording = res / mSamplingFreqIn; This is a reasonable lower-bound on delay (1 capture buffersize). Actual delay will be higher. You can ask AudioRecord::latency() for the latency value, but per padenot, the AudioRecord code does the same thing effectively (and 4.2 is the same). http://androidxref.com/4.0.4/xref/frameworks/base/media/libmedia/AudioRecord.cpp#228 https://android.googlesource.com/platform/frameworks/av/+/android-4.2.2_r1/media/libmedia/AudioRecord.cpp @@ +479,5 @@ > + ASSERT_ON_THREAD(mThreadRec); > + > + mAudioRecord->stop(); > + > + mRecording = false; Thread safety of mRecording (and mRecError) is suspect - in other places it locks around access, here it doesn't (mRecError is modified in RecordingWorker_w() without lock). This is likely a latent TSAN bug (data race). With only a few exceptions (like where you know all other threads won't touch it, *and* there was a lock barrier since the last touch), a var touched by multiple threads needs to be locked around access. @@ +492,5 @@ > + // store PCM data, to avoid data loss if polling thread runs to slow > + int recBufSize = GetMinRecordBufferSize(sampleRate, > + N_REC_CHANNELS, > + N_AUDIO_FORMAT) * 2; > + mBufferedRecSamples = (5 * sampleRate) / 200; comment how many ms of buffer this is (2.5% * samplerate == 25ms) @@ +537,5 @@ > + CSFLogInfo(logTag, "%s %d Recording already off", __FUNCTION__, mId); > + return; > + } > + > + WebRtc_UWord32 samplesToRec = mSamplingFreqIn * 10; This needs to change to support 44100 - either to be an actual rate (not khz rate), or to be a samples-per-10ms value ::: content/media/webrtc/AudioDeviceGonk.h @@ +247,5 @@ > + // Delay > + WebRtc_UWord16 mDelayRecording; > + > + // Stored device properties > + WebRtc_UWord16 mSamplingFreqIn; // Sampling frequency for Mic Right now this is not the frequency; it's the frequency in kHz. Likely this should become either the actual frequency (UWord32 please, in case 96000 is someday supported), or samples-per-10ms (i.e. frequency/100). That won't properly cover some odd rates like 22050 and 11025, though I don't think we'll generally support them anyways for mic/speaker. ::: content/media/webrtc/AudioDeviceGonkStubs.cpp @@ +4,5 @@ > + * Use of this source code is governed by a BSD-style license > + * that can be found in the LICENSE file in the root of the source > + * tree. An additional intellectual property rights grant can be found > + * in the file PATENTS. All contributing project authors may > + * be found in the AUTHORS file in the root of the source tree. Check with gerv as to the correct license to use. This is almost certainly NOT the correct license, as it's in content, not media/webrtc/trunk. If this is a modification of their source, at least the pointers to other files need to be adjusted. But I think this should just be MPL2 Repeat for other new files. Some of those may be partial copies of webrtc.org code, though. ::: content/media/webrtc/MediaEngineWebRTCAudio.cpp @@ +249,2 @@ > mVoEBase->Init(); > +#endif Use #ifndef like in MediaEngineWebRTC.cpp - or swap that, but make them consistent. ::: content/media/webrtc/moz.build @@ +23,5 @@ > + CPP_SOURCES += [ > + 'AudioDeviceGonk.cpp', > + 'AudioDeviceGonkStubs.cpp', > + ] > + In theory this may need build-peer review (trivial though).

Comment on attachment 768381 [details] [diff] [review] Gonk audio recording reworked to use callback api Review of attachment 768381 [details] [diff] [review]: ----------------------------------------------------------------- r- fix the latency value and the partial-frame calculation per comment and r+ (assuming the tsan/license/etc issues from the other patch are dealt with) If used with 44100, this may have issues until bug 886886 lands (as the current solution does) ::: content/media/webrtc/AudioDeviceGonk.cpp @@ +286,5 @@ > + // Set the audio device buffer sampling rate > + mAudioDeviceBuffer.SetRecordingSampleRate(mSamplingFreq); > + > + // TODO(jib@mozilla.com): Find optimal delay for AEC (may be hardware specific) > + mDelayRecording = 0; Per other patch - either use the buffersize-in-samples/frequency, or use AudioRecord::latency() Note that we don't do anything useful with this currently @@ +336,5 @@ > } > > mRecIsInitialized = false; > + mAudioRecord->stop(); > + mRecording = false; tsan... @@ +459,5 @@ > + // expects 10ms chunks (e.g. 160), so break things up and handle partials. > + > + unsigned int i=0; > + > + if (mNumPartialFrames) { Add a comment that numPartialFrames <= mNumFrames10ms (or an assert) @@ +475,5 @@ > + mAudioDeviceBuffer.SetVQEData(0, mDelayRecording, 0); > + mAudioDeviceBuffer.DeliverRecordedData(); > + } > + if (i < b->frameCount) { > + mNumPartialFrames = b->frameCount - i; This should be outside the if(); if i == b->frameCount we don't set nNumPartialFrames. Luckily at the moment it always sets it in the first if() above, but if we ever capture an exact multiple of 10ms this will be wrong. Then it can become "if (mNumPartialFrames != 0)" ::: media/webrtc/trunk/webrtc/modules/audio_device/audio_device_buffer.cc @@ +404,5 @@ > + return AudioDeviceBuffer::SetRecordedBufferCombine(audioBuffer, nSamples, > + NULL, 0); > +} > + > +WebRtc_Word32 AudioDeviceBuffer::SetRecordedBufferCombine(const void* audioBuffer1, Add a comment about what this is for @@ +461,5 @@ > + { > + ptr16In++; > + } > + > + // exctract left or right channel from input buffer to the local buffer don't copy spelling error (and correct the other one above)

Comment on attachment 768378 [details] [diff] [review] Gonk audio recording device. cleanup + works (4) Review of attachment 768378 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/webrtc/AudioDeviceGonk.cpp @@ +303,5 @@ > + { > + CSFLogWarn(logTag, "%s %d InitMicrophone() failed", __FUNCTION__, mId); > + } > + > + int samplingFreq = 44100; Note: this overrides the constructor's value of 16000!!!!

> Note: this overrides the constructor's value of 16000!!!! The second patch changes this. I uploaded this as two patches in case we wanted to go back to not using callbacks. I realize now this was a mistake as it makes it hard to review and causes confusion. My appologies. I'll upload a merged patch instead.

Merged patch.

Updated with feedback. > Check with gerv as to the correct license to use. This is almost certainly > NOT the correct license, as it's in content, not media/webrtc/trunk. If this > is a modification of their source, at least the pointers to other files need > to be adjusted. But I think this should just be MPL2 > > Repeat for other new files. Some of those may be partial copies of webrtc.org > code, though. OK. I've put the MPL2 in for now until I hear from gerv (CCed) > You can ask AudioRecord::latency() for the latency value Done. > This should be outside the if(); Good catch, thanks! > Note: this overrides the constructor's value of 16000!!!! It actually doesn't. Look at the subsequent lines, it's just an odd way of picking 441000 *only if* the sample rate is 44000. Thankfully this code is gone in the latest patch. I've fixed up the other comments that weren't already covered by the refactor.

Try - https://tbpl.mozilla.org/?tree=Try&rev=776c89ac6019

Hmm, after I pulled I now get "No permission to access camera". Apparently Bug 776934 landed. I have to find out what I need to do.

Sorry, I linked to the wrong bug. That one is old. Maybe something changed to enable this on b2g.

Comment on attachment 768744 [details] [diff] [review] Gonk audio recording device - cleanup + refactored (2) Got some feedback on email I hope to incorporate today before I turn on r? again.

I can't help here without more context. Some rules of thumb which might be helpful: * New additions to an existing non-MPL codebase/library should be under the licence of that codebase * New additions to an MPLed codebase/library should be under the MPL * If you are copying code between two files which have different licences, then I need exact details so I can tell you how to handle that case. Gerv

This is code which will be in mozilla-central in a new file, not an existing code base. However, it is *derived* from code in that code base, which is not under the MPL.

If the file in question is solely code from a WebRTC file plus new code, then put it under the WebRTC licence, even if it's going into the Mozilla part of the code. Gerv

(In reply to Gervase Markham [:gerv] from comment #67) > If the file in question is solely code from a WebRTC file plus new code, > then put it under the WebRTC licence, even if it's going into the Mozilla > part of the code. Gerv - this is the "WebRTC license" currently at the start of these copied & modified files. The issue is the pointers in the text. I'm guessing we should adjust the textual pointers to point to media/webrtc/trunk/foo. Correct? ::: content/media/webrtc/AudioDeviceGonkStubs.cpp @@ +4,5 @@ > + * Use of this source code is governed by a BSD-style license > + * that can be found in the LICENSE file in the root of the source > + * tree. An additional intellectual property rights grant can be found > + * in the file PATENTS. All contributing project authors may > + * be found in the AUTHORS file in the root of the source tree. i.e. replace "root of the source tree" with "media/webrtc/trunk"

Gerv suggested I just paste in the license for these three source files, which I have.

Updated with feedback. - Correct licenses. Thanks gerv! - Added mCritSecStop for race between RecorderCbf() and Stop - Fixed a bug in mNumPartialFrames logic (now never === 10ms) - Cleaned up AudioDeviceGonkStubs to assert + return 0 on unnecessary methods - CSFLog -> PR_Log - WebRtc_Word32 -> int32_t etc. - Documented SetRecordedBufferCombine() - Consistent #ifdef - Comment typos

Try - https://tbpl.mozilla.org/?tree=Try&rev=1b6ee2dab401

Comment on attachment 769440 [details] [diff] [review] Gonk audio recording device - cleanup + refactored (3) Review of attachment 769440 [details] [diff] [review]: ----------------------------------------------------------------- I didn't bother to mark all instances of certain patterns in the Stubs; please take the corrections and then apply them to all similar cases there. ::: content/media/webrtc/AudioDeviceGonk.cpp @@ +278,5 @@ > + N_AUDIO_FORMAT, > + N_REC_CHANNELS) != NO_ERROR) { > + return -1; > + } > + Log the min frame count @@ +282,5 @@ > + > + // release the object > + if (mAudioRecord) { > + mAudioRecord->stop(); > + mAudioRecord = nullptr; This means stop() had better ensure RecordingCbf() isn't in the middle of a call, and won't be called again after it returns. Check. @@ +313,5 @@ > + > + // Set the audio device buffer sampling rate > + mAudioDeviceBuffer.SetRecordingSampleRate(mSamplingFreq); > + > + //SetSpeakerVolume(0); Remove @@ +376,5 @@ > + } > + { > + ReentrantMonitorAutoEnter lock(mCritSectStop); > + mAudioRecord->stop(); > + } I don't see how we've avoided a race against RecordingCbf() - if we call Stop(), what happens if we're in RecordingCbf()? This may be ok, so long as we ensure we're not in RecordingCbf() before shutting down/deleting this object or anything touched by RecordingCbf(), or if mAudioRecord->stop() implements the locking we need (which it may). Please check the docs for that. @@ +505,5 @@ > + MOZ_ASSERT(mNumPartialFrames < mNumFrames10ms); > + i = mNumFrames10ms - mNumPartialFrames; > + mAudioDeviceBuffer.SetRecordedBufferCombine(mPartialBuffer, > + mNumPartialFrames, > + b->i16, i); For now, do a memcpy create a single buffer in mPartialBuffer of mNumFrames10ms, then call SetRecordedBuffer(). It doesn't need 100% efficiency if it adds maintenance hassle by modifying core GIPS code and carrying the mod. File a followup enhancement bug to reduce CPU use by modifying the core GIPS code to support AppendRecordedData(more_data, num_samples) (instead of the fixed-use SetRecordedBufferCombine()) which would let us get rid of the copy. File an issue at webrtc.org for this enhancement (with a patch if possible for code review). @@ +515,5 @@ > + { > + mAudioDeviceBuffer.SetRecordedBuffer(&b->i16[i], mNumFrames10ms); > + mAudioDeviceBuffer.SetVQEData(0, mDelayRecording, 0); > + mAudioDeviceBuffer.DeliverRecordedData(); > + } Arguable style point: it can be done this way, or like while (mNumPartialFrames + frames_left >= mNumFrames10ms) { if (mNumPartialFrames) { memcpy(...) SetRecordedBuffer() mPartialFrames = 0; frames_left -= ... } else { SetRecordedBuffer() frames_left -= mNumFrames10ms } SetVQEData() DeliverRecordedData() } if (frames_left != 0) { ... } The only reason for doing it that way would be to avoid a small amount of simple duplicated code (SetVQEData and DeliverRecordedData calls). I don't see this as compelling, and it's perhaps slightly lower performance. So I say leave it the way it is. ::: content/media/webrtc/AudioDeviceGonkStubs.cpp @@ +54,5 @@ > +#define WARN(msg) > +#endif > + > +#define MOZ_ADM_UNIMPLEMENTED_RESULT(r) \ > + MOZ_ASSERT(false, "Function not implemented"); \ Nit, and may not matter much in practice due to stack backtraces, but I'd be tempted to have that be MOZ_ASSERT(false, __FUNCTION__ " not implemented") @@ +57,5 @@ > +#define MOZ_ADM_UNIMPLEMENTED_RESULT(r) \ > + MOZ_ASSERT(false, "Function not implemented"); \ > + return (r); > + > +#define MOZ_ADM_UNIMPLEMENTED() MOZ_ADM_UNIMPLEMENTED_RESULT(0) Get rid of the two-stage return and just have functions do "MOZ_ADM_UNIMPLEMENTED(0);" so when reading the code you can see the result. Or, perhaps even better yet, don't work to save 6 characters by hiding the returns: MOZ_ADM_UNIMPLEMENTED(); return 0; or MOZ_ADM_WARN(); return -1; etc. @@ +99,5 @@ > + > +int32_t AudioDeviceGonk::SpeakerIsAvailable(bool* available) > +{ > + *available = false; > + MOZ_ADM_SATISFY_GIPS(-1); We implement this function (and return it's not available). I assume Speaker isn't available so it won't try to call us for output @@ +118,5 @@ > + > +int32_t AudioDeviceGonk::MicrophoneIsAvailable(bool* available) > +{ > + // We always assume it's available > + *available = true; Perhaps we should file a followup bug to hook into the audiorouting code to actually find this out, and comment with the bug number here. We could warn that we don't know the actual mic availability. @@ +140,5 @@ > +// ---------------------------------------------------------------------------- > + > +int32_t AudioDeviceGonk::StereoRecordingIsAvailable(bool* available) const > +{ > + // Stereo recording not supported on Gonk Are sure that will always be true? What about bluetooth/etc? Tablets with USB headsets? However, this is likely true enough for any foreseeable near future. @@ +152,5 @@ > + > +int32_t AudioDeviceGonk::SpeakerVolumeIsAvailable(bool* available) > +{ > + *available = false; > + MOZ_ADM_UNIMPLEMENTED(); We implement this, and say we don't implement SpeakerVolume control. Remove Unimplemented. @@ +200,5 @@ > +// ---------------------------------------------------------------------------- > + > +bool AudioDeviceGonk::SpeakerIsInitialized() const > +{ > + MOZ_ADM_UNIMPLEMENTED_RESULT(false); Per above, if we say the speaker is available, we should say it's initialized and not mark as unimplemented. @@ +240,5 @@ > + > +int32_t AudioDeviceGonk::SpeakerMuteIsAvailable(bool* available) > +{ > + *available = false; > + MOZ_ADM_UNIMPLEMENTED(); We implement this, and say we don't implement SpeakerMute. Remove Unimplemented. @@ +316,5 @@ > +// ---------------------------------------------------------------------------- > + > +int32_t AudioDeviceGonk::MicrophoneBoost(bool* enabled) const > +{ > + MOZ_ADM_UNIMPLEMENTED(); With the current macro, this returns success, so you better set *enabled to something. @@ +335,5 @@ > +// ---------------------------------------------------------------------------- > + > +int32_t AudioDeviceGonk::MicrophoneVolume(uint32_t* volume) const > +{ > + MOZ_ADM_UNIMPLEMENTED(); With the current macro, this returns success, so you better set *volume to something. @@ +353,5 @@ > +// ---------------------------------------------------------------------------- > + > +int32_t AudioDeviceGonk::StereoRecording(bool* enabled) const > +{ > + MOZ_ADM_SATISFY_GIPS(-1); Probably safer to set *enabled = false, even though we're returning failure. @@ +371,5 @@ > +// ---------------------------------------------------------------------------- > + > +int32_t AudioDeviceGonk::RecordingChannel(ChannelType* channel) const > +{ > + MOZ_ADM_SATISFY_GIPS(-1); Probably safer to set *channel = nullptr, even though we're returning failure. @@ +381,5 @@ > + > +int32_t AudioDeviceGonk::StereoPlayoutIsAvailable(bool* available) const > +{ > + *available = false; > + MOZ_ADM_SATISFY_GIPS(-1); We implement this availability call. Remove Unimplemented. @@ +399,5 @@ > +// ---------------------------------------------------------------------------- > + > +int32_t AudioDeviceGonk::StereoPlayout(bool* enabled) const > +{ > + MOZ_ADM_UNIMPLEMENTED(); With the current macro, this returns success, so you better set *enabled to something. @@ +427,5 @@ > + > +int32_t AudioDeviceGonk::PlayoutIsAvailable(bool* available) > +{ > + *available = false; > + MOZ_ADM_UNIMPLEMENTED(); We implement this. Remove Unimplemented. @@ +446,5 @@ > +// ---------------------------------------------------------------------------- > + > +int32_t AudioDeviceGonk::MaxMicrophoneVolume(uint32_t* maxVolume) const > +{ > + MOZ_ADM_UNIMPLEMENTED(); With the current macro, this returns success, so you better set *maxVolume to something. @@ +455,5 @@ > +// ---------------------------------------------------------------------------- > + > +int32_t AudioDeviceGonk::MinMicrophoneVolume(uint32_t* minVolume) const > +{ > + MOZ_ADM_UNIMPLEMENTED(); Ditto @@ +464,5 @@ > +// ---------------------------------------------------------------------------- > + > +int32_t AudioDeviceGonk::MicrophoneVolumeStepSize(uint16_t* stepSize) const > +{ > + MOZ_ADM_UNIMPLEMENTED(); Ditto @@ +484,5 @@ > + > +int16_t AudioDeviceGonk::PlayoutDevices() > +{ > + // There is one device only > + return 1; Do we have a playout device? I'd think 0 here, unless that's invalid or causes callers to blow up @@ +496,5 @@ > + uint16_t index, > + char name[kAdmMaxDeviceNameSize], > + char guid[kAdmMaxGuidSize]) > +{ > + MOZ_ADM_SATISFY_GIPS(0); I'd really prefer to return names here, even though we've kludged around null names. And right now this returns success without setting the names, which might be safe if the callers are paranoid (and some are), but better to not assume it. I'd like to return something semi-reasonable, like "Default Microphone" (assuming system audio routing is deciding which one we get). @@ +542,5 @@ > +// ---------------------------------------------------------------------------- > + > +int32_t AudioDeviceGonk::SetRecordingDevice(uint16_t index) > +{ > + MOZ_ADM_SATISFY_GIPS(0); This should just return 0 for index 0, and -1 for all other indexes I believe @@ +551,5 @@ > +// ---------------------------------------------------------------------------- > + > +int32_t AudioDeviceGonk::SetRecordingDevice(WindowsDeviceType device) > +{ > + MOZ_ADM_SATISFY_GIPS(0); likely this should just return 0. @@ +605,5 @@ > +// ---------------------------------------------------------------------------- > + > +bool AudioDeviceGonk::Playing() const > +{ > + MOZ_ADM_UNIMPLEMENTED(); This returns bool, not int, but if my suggestions are implemented this won't matter anymore @@ +644,5 @@ > +// ---------------------------------------------------------------------------- > + > +int32_t AudioDeviceGonk::CPULoad(uint16_t* load) const > +{ > + MOZ_ADM_UNIMPLEMENTED(); Set *load and/or return failure. @@ +753,5 @@ > +} > + > +int32_t AudioDeviceGonk::EnableBuiltInAEC(bool enable) > +{ > + MOZ_ADM_UNIMPLEMENTED(); Return failure here ::: content/media/webrtc/MediaEngineWebRTCAudio.cpp @@ +249,2 @@ > mVoEBase->Init(); > +#endif please make this and MediaEngineWebRTC.cpp use the same pattern... They're close. ::: media/webrtc/trunk/webrtc/modules/audio_device/audio_device_buffer.cc @@ +402,5 @@ > WebRtc_UWord32 nSamples) > { > + return AudioDeviceBuffer::SetRecordedBufferCombine(audioBuffer, nSamples, > + NULL, 0); > +} See comments to AudioDeviceGonk.cpp

Comment on attachment 769440 [details] [diff] [review] Gonk audio recording device - cleanup + refactored (3) Review of attachment 769440 [details] [diff] [review]: ----------------------------------------------------------------- I reviewed this on Rietveld. http://firefox-codereview.appspot.com/19001/#msg2 I see a number of issues as noted below. It seems the stubs still need more review. Please go through and examine them for: - Side effects on the arguments when they are unimplemented. (Why?) - Differences from the Chromium code. Each such instance needs an explanation and probably a comment. The threading model seems confusing and I believe contains errors. In addition to addressing the threading issues noted below, please. 1. Add a block comment documenting the model. 2. Document each member with the threads it is used in. http://firefox-codereview.appspot.com/19001/diff/13001/content/media/webrtc/A... File content/media/webrtc/AudioDeviceGonk.cpp (right): http://firefox-codereview.appspot.com/19001/diff/13001/content/media/webrtc/A... content/media/webrtc/AudioDeviceGonk.cpp:48: PRLogModuleInfo* GetAudioLog() Please do not name extern functions this generically. Why isn't this a class static? http://firefox-codereview.appspot.com/19001/diff/13001/content/media/webrtc/A... content/media/webrtc/AudioDeviceGonk.cpp:50: static PRLogModuleInfo *sLog = PR_NewLogModule("audio"); If this is a static, why are you getting it repeatedly? Doesn't this need a thread lock? http://firefox-codereview.appspot.com/19001/diff/13001/content/media/webrtc/A... content/media/webrtc/AudioDeviceGonk.cpp:53: #define LOG(msg) PR_LOG(GetAudioLog(), PR_LOG_DEBUG, msg) PR_LOG already has this wrapper. That's why it's been done as a macro. No need for an additional conditional compilation stage. http://firefox-codereview.appspot.com/19001/diff/13001/content/media/webrtc/A... content/media/webrtc/AudioDeviceGonk.cpp:68: AudioDeviceModule* AudioDeviceGonk::Create(const int32_t id, Please fix indents, here and below http://firefox-codereview.appspot.com/19001/diff/13001/content/media/webrtc/A... content/media/webrtc/AudioDeviceGonk.cpp:317: //SetSpeakerVolume(0); If you're not going to use this, remotve it, not comment out. http://firefox-codereview.appspot.com/19001/diff/13001/content/media/webrtc/A... content/media/webrtc/AudioDeviceGonk.cpp:379: mAudioRecord->stop(); What are the API guarantees here? Does mAudioRecord promise synchronous termination? Note that if (for instance) it was implemented with a message queue, you might not get the expected behavior. http://firefox-codereview.appspot.com/19001/diff/13001/content/media/webrtc/A... content/media/webrtc/AudioDeviceGonk.cpp:488: struct AudioRecord::Buffer *b = static_cast<AudioRecord::Buffer *>(info); If you are going to have a CritSect here you need to actually check for recording being terminated. Otherwise you can have: T1: T2: Acquire lock RecordingCbf() Try to acquire (wait) Stop recording Release Lock Lock acquired try to process frames Note that you are also trying to modify mNumPartialFrames concurrently in two threads, since StopRecording does it outside of the stop critsect. http://firefox-codereview.appspot.com/19001/diff/13001/content/media/webrtc/A... content/media/webrtc/AudioDeviceGonk.cpp:507: mAudioDeviceBuffer.SetRecordedBufferCombine(mPartialBuffer, Please rewrite this to buffer internally rather than modifying GIPS. http://firefox-codereview.appspot.com/19001/diff/13001/content/media/webrtc/A... File content/media/webrtc/AudioDeviceGonk.h (right): http://firefox-codereview.appspot.com/19001/diff/13001/content/media/webrtc/A... content/media/webrtc/AudioDeviceGonk.h:229: mutable mozilla::ReentrantMonitor mCritSect; Please annotate each of these variables with which threads they are used in. http://firefox-codereview.appspot.com/19001/diff/13001/content/media/webrtc/A... File content/media/webrtc/AudioDeviceGonkStubs.cpp (right): http://firefox-codereview.appspot.com/19001/diff/13001/content/media/webrtc/A... content/media/webrtc/AudioDeviceGonkStubs.cpp:54: #define WARN(msg) Again, no need for this. http://firefox-codereview.appspot.com/19001/diff/13001/content/media/webrtc/A... content/media/webrtc/AudioDeviceGonkStubs.cpp:103: MOZ_ADM_SATISFY_GIPS(-1); This returns 0 in Chromium. Why not here. http://firefox-codereview.appspot.com/19001/diff/13001/content/media/webrtc/A... content/media/webrtc/AudioDeviceGonkStubs.cpp:112: MOZ_ADM_SATISFY_GIPS(-1); Returns 0 in chromiumm http://firefox-codereview.appspot.com/19001/diff/13001/content/media/webrtc/A... content/media/webrtc/AudioDeviceGonkStubs.cpp:122: *available = true; Returns 0 in chromium. http://firefox-codereview.appspot.com/19001/diff/13001/content/media/webrtc/A... content/media/webrtc/AudioDeviceGonkStubs.cpp:174: *volume = 0; Why are you setting values when these are supposed to be asserted? Reply

Thanks for thorough reviews! - Some quick notes on select items only: > Log the min frame count It is logged 10 lines down: LOG(("%s %d mSamplingFreq=%d minFrameCount=%d", __FUNCTION__, mId, mSamplingFreq, minFrameCount)); > @@ +282,5 @@ > > + > > + // release the object > > + if (mAudioRecord) { > > + mAudioRecord->stop(); > > + mAudioRecord = nullptr; > > This means stop() had better ensure RecordingCbf() isn't in the middle of a > call, and won't be called again after it returns. Check. On further inspection, The "mAudioRecord->stop();" line is redundant and can be removed, because mRecIsInitialized ensures we can't get here without stop() having already been called. > ::: content/media/webrtc/AudioDeviceGonkStubs.cpp > @@ +54,5 @@ > > +#define WARN(msg) > > +#endif > > + > > +#define MOZ_ADM_UNIMPLEMENTED_RESULT(r) \ > > + MOZ_ASSERT(false, "Function not implemented"); \ > > Nit, and may not matter much in practice due to stack backtraces, but I'd be > tempted to have that be MOZ_ASSERT(false, __FUNCTION__ " not implemented") Wont compile, except on Windows where __FUNCTION__ is a literal string. http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2004/n1642.html > @@ +57,5 @@ > > +#define MOZ_ADM_UNIMPLEMENTED_RESULT(r) \ > > + MOZ_ASSERT(false, "Function not implemented"); \ > > + return (r); > > + > > +#define MOZ_ADM_UNIMPLEMENTED() MOZ_ADM_UNIMPLEMENTED_RESULT(0) > > Get rid of the two-stage return and just have functions do > "MOZ_ADM_UNIMPLEMENTED(0);" so when reading the code you can see the result. > Or, perhaps even better yet, don't work to save 6 characters by hiding the > returns: > > MOZ_ADM_UNIMPLEMENTED(); > return 0; Having the macro take the return value was ekr's idea. I believe he sought some consistency controls here (or why use macros)? - I went further by having the macro control the policy (MOZ_CRASH/MOZ_ASSERT and return value) for consistency and quick change. Reviewers please discuss. > @@ +99,5 @@ > > + > > +int32_t AudioDeviceGonk::SpeakerIsAvailable(bool* available) > > +{ > > + *available = false; > > + MOZ_ADM_SATISFY_GIPS(-1); > > We implement this function (and return it's not available). > > I assume Speaker isn't available so it won't try to call us for output Because of code like this ... http://mxr.mozilla.org/mozilla-central/source/media/webrtc/trunk/webrtc/voice_engine/voe_base_impl.cc#423 > @@ +152,5 @@ > > + > > +int32_t AudioDeviceGonk::SpeakerVolumeIsAvailable(bool* available) > > +{ > > + *available = false; > > + MOZ_ADM_UNIMPLEMENTED(); > > We implement this, and say we don't implement SpeakerVolume control. Remove > Unimplemented. OK. > @@ +200,5 @@ > > +// ---------------------------------------------------------------------------- > > + > > +bool AudioDeviceGonk::SpeakerIsInitialized() const > > +{ > > + MOZ_ADM_UNIMPLEMENTED_RESULT(false); > > Per above, if we say the speaker is available, we should say it's initialized > and not mark as unimplemented. Above we say the speaker is *never* available. > @@ +335,5 @@ > > +// ---------------------------------------------------------------------------- > > + > > +int32_t AudioDeviceGonk::MicrophoneVolume(uint32_t* volume) const > > +{ > > + MOZ_ADM_UNIMPLEMENTED(); > > With the current macro, this returns success, so you better set *volume to > something. Ekr asked me to mirror Chrome: https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/media/webrtc_audio_device_not_impl.cc&sq=package:chromium&rcl=1372250331&l=153 Reviewers please discuss. > @@ +353,5 @@ > > +// ---------------------------------------------------------------------------- > > + > > +int32_t AudioDeviceGonk::StereoRecording(bool* enabled) const > > +{ > > + MOZ_ADM_SATISFY_GIPS(-1); > > Probably safer to set *enabled = false, even though we're returning failure. That's not a code-pattern I've observed us using elsewhere. Any reason we should here? A caller shouldn't rely on results from a failing function. > @@ +371,5 @@ > > +// ---------------------------------------------------------------------------- > > + > > +int32_t AudioDeviceGonk::RecordingChannel(ChannelType* channel) const > > +{ > > + MOZ_ADM_SATISFY_GIPS(-1); > > Probably safer to set *channel = nullptr, even though we're returning failure. Or 0xDEADBEEF ? > @@ +551,5 @@ > > +// ---------------------------------------------------------------------------- > > + > > +int32_t AudioDeviceGonk::SetRecordingDevice(WindowsDeviceType device) > > +{ > > + MOZ_ADM_SATISFY_GIPS(0); > > likely this should just return 0. It does. You mean remove warning? > @@ +644,5 @@ > > +// ---------------------------------------------------------------------------- > > + > > +int32_t AudioDeviceGonk::CPULoad(uint16_t* load) const > > +{ > > + MOZ_ADM_UNIMPLEMENTED(); > > Set *load and/or return failure. https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/media/webrtc_audio_device_not_impl.cc&sq=package:chromium&l=233 Reviewers please discuss.

Thanks! More quick notes on select items only: > It seems the stubs still need more review. Please go through and > examine them for: > > - Side effects on the arguments when they are unimplemented. (Why?) > - Differences from the Chromium code. Chromium ignores arguments. How do I reconcile this? > http://firefox-codereview.appspot.com/19001/diff/13001/content/media/webrtc/A... > content/media/webrtc/AudioDeviceGonk.cpp:50: static PRLogModuleInfo *sLog = > PR_NewLogModule("audio"); > If this is a static, why are you getting it repeatedly? > > Doesn't this need a thread lock? http://en.wikipedia.org/wiki/Static_variable#Example > http://firefox-codereview.appspot.com/19001/diff/13001/content/media/webrtc/A... > content/media/webrtc/AudioDeviceGonk.cpp:53: #define LOG(msg) > PR_LOG(GetAudioLog(), PR_LOG_DEBUG, msg) > PR_LOG already has this wrapper. That's why it's been done as a macro. No need > for an additional conditional compilation stage. OK. FYI I copied this from http://mxr.mozilla.org/mozilla-central/source/dom/media/MediaManager.cpp#44 > http://firefox-codereview.appspot.com/19001/diff/13001/content/media/webrtc/A... > content/media/webrtc/AudioDeviceGonkStubs.cpp:103: MOZ_ADM_SATISFY_GIPS(-1); > This returns 0 in Chromium. Why not here. > > http://firefox-codereview.appspot.com/19001/diff/13001/content/media/webrtc/A... > content/media/webrtc/AudioDeviceGonkStubs.cpp:112: MOZ_ADM_SATISFY_GIPS(-1); > Returns 0 in chromiumm > > http://firefox-codereview.appspot.com/19001/diff/13001/content/media/webrtc/A... > content/media/webrtc/AudioDeviceGonkStubs.cpp:122: *available = true; > Returns 0 in chromium. > > http://firefox-codereview.appspot.com/19001/diff/13001/content/media/webrtc/A... > content/media/webrtc/AudioDeviceGonkStubs.cpp:174: *volume = 0; > Why are you setting values when these are supposed to be asserted? Reviewers please discuss.

I agree on all the other comments. Thanks again.

Wrong __FUNCTION__ link. I meant http://software.intel.com/en-us/articles/treating-func-and-function-as-string-literals

(In reply to Eric Rescorla (:ekr) from comment #73) > http://firefox-codereview.appspot.com/19001/diff/13001/content/media/webrtc/ > A... > content/media/webrtc/AudioDeviceGonk.cpp:48: PRLogModuleInfo* GetAudioLog() > Please do not name extern functions this generically. Why isn't this a class > static? So you can use the log from more than one class - this is the normal pattern. Especially with logs with non-class-specific names. > > http://firefox-codereview.appspot.com/19001/diff/13001/content/media/webrtc/ > A... > content/media/webrtc/AudioDeviceGonk.cpp:50: static PRLogModuleInfo *sLog = > PR_NewLogModule("audio"); > If this is a static, why are you getting it repeatedly? Follow the pattern used elsewhere: static PRLogModuleInfo *sLog; if (!sLog) sLog = PR_NewLogModule(); return sLog; > Doesn't this need a thread lock? In light of modern data-race stuff, you would think so. However a) I've never seen it flagged by tsan (which looks at of the race actually occurs, as opposed to seems like it could occur in theory), and b) after looking at the other PR_NewLogModule uses, the only one I saw that uses a lock is also initing a list. There are some run from module::Init() functions, but they're the minority. So if it's an issue, it's an issue for the entire tree, and in practice we have evidence the races around this static don't happen. (In classic non-tsan view of things, the worst a race would do is leak a LogModule structure.) I wouldn't go trying to fix this one instance. We should consider opening a bug against the whole tree with bsmedberg and other compiler/xpcom/threading people to discuss this. > > http://firefox-codereview.appspot.com/19001/diff/13001/content/media/webrtc/ > A... > content/media/webrtc/AudioDeviceGonk.cpp:53: #define LOG(msg) > PR_LOG(GetAudioLog(), PR_LOG_DEBUG, msg) > PR_LOG already has this wrapper. That's why it's been done as a macro. No > need > for an additional conditional compilation stage. No, you need this for a log-specific LOG() macro (and in this case it puts them all at the LOG_DEBUG level, as a lot of the module LOG macros do, though some leave level exposed like MSG). This is in keeping with the typical pattern though. > http://firefox-codereview.appspot.com/19001/diff/13001/content/media/webrtc/ > A... > content/media/webrtc/AudioDeviceGonk.cpp:379: mAudioRecord->stop(); > What are the API guarantees here? Does mAudioRecord promise synchronous > termination? Note that if (for instance) it was implemented with a message > queue, you might not get the expected behavior. > Right; this was one of my biggest questions - check how stop() works. > http://firefox-codereview.appspot.com/19001/diff/13001/content/media/webrtc/ > A... > content/media/webrtc/AudioDeviceGonk.cpp:507: > mAudioDeviceBuffer.SetRecordedBufferCombine(mPartialBuffer, > Please rewrite this to buffer internally rather than modifying GIPS. My other big flag here - see my review for how we should do this. > http://firefox-codereview.appspot.com/19001/diff/13001/content/media/webrtc/ > A... > content/media/webrtc/AudioDeviceGonkStubs.cpp:174: *volume = 0; > Why are you setting values when these are supposed to be asserted? Because we may not catch/fix anything that would fire an assert in testing, and returning a safe value in opt builds makes sense. If we believe it's unsafe to call this even in opt builds with a safe return, we should either fix the calling code, or use MOZ_CRASH. I didn't see any that I thought required MOZ_CRASH

(In reply to Jan-Ivar Bruaroey [:jib] from comment #74) > Thanks for thorough reviews! - Some quick notes on select items only: > > > Log the min frame count > > It is logged 10 lines down: > > LOG(("%s %d mSamplingFreq=%d minFrameCount=%d", > __FUNCTION__, mId, mSamplingFreq, minFrameCount)); > > > @@ +282,5 @@ > > > + > > > + // release the object > > > + if (mAudioRecord) { > > > + mAudioRecord->stop(); > > > + mAudioRecord = nullptr; > > > > This means stop() had better ensure RecordingCbf() isn't in the middle of a > > call, and won't be called again after it returns. Check. > > On further inspection, The "mAudioRecord->stop();" line is redundant and can > be removed, because mRecIsInitialized ensures we can't get here without > stop() having already been called. > > > ::: content/media/webrtc/AudioDeviceGonkStubs.cpp > > @@ +54,5 @@ > > > +#define WARN(msg) > > > +#endif > > > + > > > +#define MOZ_ADM_UNIMPLEMENTED_RESULT(r) \ > > > + MOZ_ASSERT(false, "Function not implemented"); \ > > > > Nit, and may not matter much in practice due to stack backtraces, but I'd be > > tempted to have that be MOZ_ASSERT(false, __FUNCTION__ " not implemented") > > Wont compile, except on Windows where __FUNCTION__ is a literal string. > http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2004/n1642.html > > > @@ +57,5 @@ > > > +#define MOZ_ADM_UNIMPLEMENTED_RESULT(r) \ > > > + MOZ_ASSERT(false, "Function not implemented"); \ > > > + return (r); > > > + > > > +#define MOZ_ADM_UNIMPLEMENTED() MOZ_ADM_UNIMPLEMENTED_RESULT(0) > > > > Get rid of the two-stage return and just have functions do > > "MOZ_ADM_UNIMPLEMENTED(0);" so when reading the code you can see the result. > > Or, perhaps even better yet, don't work to save 6 characters by hiding the > > returns: > > > > MOZ_ADM_UNIMPLEMENTED(); > > return 0; > > Having the macro take the return value was ekr's idea. I believe he sought > some consistency controls here (or why use macros)? - I went further by > having the macro control the policy (MOZ_CRASH/MOZ_ASSERT and return value) > for consistency and quick change. > > Reviewers please discuss. It wasn't my idea to have a special empty-argument macro that defaulted to 0. > Above we say the speaker is *never* available. > > > @@ +335,5 @@ > > > +// ---------------------------------------------------------------------------- > > > + > > > +int32_t AudioDeviceGonk::MicrophoneVolume(uint32_t* volume) const > > > +{ > > > + MOZ_ADM_UNIMPLEMENTED(); > > > > With the current macro, this returns success, so you better set *volume to > > something. > > Ekr asked me to mirror Chrome: > > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/ > media/webrtc_audio_device_not_impl.cc&sq=package: > chromium&rcl=1372250331&l=153 > > Reviewers please discuss. What I asked you to do was to review each usage and determine what was right.

(In reply to Jan-Ivar Bruaroey [:jib] from comment #74) > Thanks for thorough reviews! - Some quick notes on select items only: > > > Log the min frame count > > It is logged 10 lines down: > > LOG(("%s %d mSamplingFreq=%d minFrameCount=%d", > __FUNCTION__, mId, mSamplingFreq, minFrameCount)); > > > @@ +282,5 @@ > > > + > > > + // release the object > > > + if (mAudioRecord) { > > > + mAudioRecord->stop(); > > > + mAudioRecord = nullptr; > > > > This means stop() had better ensure RecordingCbf() isn't in the middle of a > > call, and won't be called again after it returns. Check. > > On further inspection, The "mAudioRecord->stop();" line is redundant and can > be removed, because mRecIsInitialized ensures we can't get here without > stop() having already been called. > > > ::: content/media/webrtc/AudioDeviceGonkStubs.cpp > > @@ +54,5 @@ > > > +#define WARN(msg) > > > +#endif > > > + > > > +#define MOZ_ADM_UNIMPLEMENTED_RESULT(r) \ > > > + MOZ_ASSERT(false, "Function not implemented"); \ > > > > Nit, and may not matter much in practice due to stack backtraces, but I'd be > > tempted to have that be MOZ_ASSERT(false, __FUNCTION__ " not implemented") > > Wont compile, except on Windows where __FUNCTION__ is a literal string. > http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2004/n1642.html > > > @@ +57,5 @@ > > > +#define MOZ_ADM_UNIMPLEMENTED_RESULT(r) \ > > > + MOZ_ASSERT(false, "Function not implemented"); \ > > > + return (r); > > > + > > > +#define MOZ_ADM_UNIMPLEMENTED() MOZ_ADM_UNIMPLEMENTED_RESULT(0) > > > > Get rid of the two-stage return and just have functions do > > "MOZ_ADM_UNIMPLEMENTED(0);" so when reading the code you can see the result. > > Or, perhaps even better yet, don't work to save 6 characters by hiding the > > returns: > > > > MOZ_ADM_UNIMPLEMENTED(); > > return 0; > > Having the macro take the return value was ekr's idea. I believe he sought > some consistency controls here (or why use macros)? - I went further by > having the macro control the policy (MOZ_CRASH/MOZ_ASSERT and return value) > for consistency and quick change. > > Reviewers please discuss. It wasn't my idea to have a special empty-argument macro that defaulted to 0. > Above we say the speaker is *never* available. > > > @@ +335,5 @@ > > > +// ---------------------------------------------------------------------------- > > > + > > > +int32_t AudioDeviceGonk::MicrophoneVolume(uint32_t* volume) const > > > +{ > > > + MOZ_ADM_UNIMPLEMENTED(); > > > > With the current macro, this returns success, so you better set *volume to > > something. > > Ekr asked me to mirror Chrome: > > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/ > media/webrtc_audio_device_not_impl.cc&sq=package: > chromium&rcl=1372250331&l=153 > > Reviewers please discuss. What I asked you to do was to review each usage and determine what was right.

(In reply to Jan-Ivar Bruaroey [:jib] from comment #75) > Thanks! More quick notes on select items only: > > > It seems the stubs still need more review. Please go through and > > examine them for: > > > > - Side effects on the arguments when they are unimplemented. (Why?) > > - Differences from the Chromium code. > > Chromium ignores arguments. How do I reconcile this? Yes, I think you should probably ignore the arguments, as Chrome does. Where you differ from Chrome, please explain why. > > http://firefox-codereview.appspot.com/19001/diff/13001/content/media/webrtc/A... > > content/media/webrtc/AudioDeviceGonk.cpp:50: static PRLogModuleInfo *sLog = > > PR_NewLogModule("audio"); > > If this is a static, why are you getting it repeatedly? > > > > Doesn't this need a thread lock? > > http://en.wikipedia.org/wiki/Static_variable#Example > That's not an answer. You're repeatedly assigning a static value of a function. That's simply not correct. > > http://firefox-codereview.appspot.com/19001/diff/13001/content/media/webrtc/A... > > content/media/webrtc/AudioDeviceGonk.cpp:53: #define LOG(msg) > > PR_LOG(GetAudioLog(), PR_LOG_DEBUG, msg) > > PR_LOG already has this wrapper. That's why it's been done as a macro. No need > > for an additional conditional compilation stage. > > OK. FYI I copied this from > http://mxr.mozilla.org/mozilla-central/source/dom/media/MediaManager.cpp#44 OK, but see the code at: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/npapi/npspy/extern/nspr/prlog.h&q=PR_LOG&sq=package:chromium&type=cs&l=216 What is the extra wrapping buying you?

(In reply to Jan-Ivar Bruaroey [:jib] from comment #75) > Thanks! More quick notes on select items only: > > > It seems the stubs still need more review. Please go through and > > examine them for: > > > > - Side effects on the arguments when they are unimplemented. (Why?) > > - Differences from the Chromium code. > > Chromium ignores arguments. How do I reconcile this? Yes, I think you should probably ignore the arguments, as Chrome does. Where you differ from Chrome, please explain why. > > http://firefox-codereview.appspot.com/19001/diff/13001/content/media/webrtc/A... > > content/media/webrtc/AudioDeviceGonk.cpp:50: static PRLogModuleInfo *sLog = > > PR_NewLogModule("audio"); > > If this is a static, why are you getting it repeatedly? > > > > Doesn't this need a thread lock? > > http://en.wikipedia.org/wiki/Static_variable#Example > That's not an answer. You're repeatedly assigning a static value of a function. That's simply not correct. > > http://firefox-codereview.appspot.com/19001/diff/13001/content/media/webrtc/A... > > content/media/webrtc/AudioDeviceGonk.cpp:53: #define LOG(msg) > > PR_LOG(GetAudioLog(), PR_LOG_DEBUG, msg) > > PR_LOG already has this wrapper. That's why it's been done as a macro. No need > > for an additional conditional compilation stage. > > OK. FYI I copied this from > http://mxr.mozilla.org/mozilla-central/source/dom/media/MediaManager.cpp#44 OK, but see the code at: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/npapi/npspy/extern/nspr/prlog.h&q=PR_LOG&sq=package:chromium&type=cs&l=216 What is the extra wrapping buying you?

(In reply to Jan-Ivar Bruaroey [:jib] from comment #75) > Thanks! More quick notes on select items only: > > > It seems the stubs still need more review. Please go through and > > examine them for: > > > > - Side effects on the arguments when they are unimplemented. (Why?) > > - Differences from the Chromium code. > > Chromium ignores arguments. How do I reconcile this? Yes, I think you should probably ignore the arguments, as Chrome does. Where you differ from Chrome, please explain why. > > http://firefox-codereview.appspot.com/19001/diff/13001/content/media/webrtc/A... > > content/media/webrtc/AudioDeviceGonk.cpp:50: static PRLogModuleInfo *sLog = > > PR_NewLogModule("audio"); > > If this is a static, why are you getting it repeatedly? > > > > Doesn't this need a thread lock? > > http://en.wikipedia.org/wiki/Static_variable#Example > That's not an answer. You're repeatedly assigning a static value of a function. That's simply not correct. > > http://firefox-codereview.appspot.com/19001/diff/13001/content/media/webrtc/A... > > content/media/webrtc/AudioDeviceGonk.cpp:53: #define LOG(msg) > > PR_LOG(GetAudioLog(), PR_LOG_DEBUG, msg) > > PR_LOG already has this wrapper. That's why it's been done as a macro. No need > > for an additional conditional compilation stage. > > OK. FYI I copied this from > http://mxr.mozilla.org/mozilla-central/source/dom/media/MediaManager.cpp#44 OK, but see the code at: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/npapi/npspy/extern/nspr/prlog.h&q=PR_LOG&sq=package:chromium&type=cs&l=216 What is the extra wrapping buying you?

(In reply to Randell Jesup [:jesup] from comment #78) > (In reply to Eric Rescorla (:ekr) from comment #73) > > > http://firefox-codereview.appspot.com/19001/diff/13001/content/media/webrtc/ > > A... > > content/media/webrtc/AudioDeviceGonk.cpp:48: PRLogModuleInfo* GetAudioLog() > > Please do not name extern functions this generically. Why isn't this a class > > static? > > So you can use the log from more than one class - this is the normal > pattern. Especially with logs with non-class-specific names. I don't think that's a good argument in this case: 1. This actually shouldn't be "audio". That's going to make it super-hard to actually debug anything. 2. I strongly suspect that everyone else who wants to use "audio" just calls PR_NewLogModule() directly. 3. This isn't even declared in a header file, so it's of scant use to others. 4. Having a function like this exported to multiple threads w/o a thread lock is a terrible idea, because it's totally impossible to guarantee the API contract. This function is only used by this class. It should be either a member fxn or a class static. > > http://firefox-codereview.appspot.com/19001/diff/13001/content/media/webrtc/ > > A... > > content/media/webrtc/AudioDeviceGonk.cpp:53: #define LOG(msg) > > PR_LOG(GetAudioLog(), PR_LOG_DEBUG, msg) > > PR_LOG already has this wrapper. That's why it's been done as a macro. No > > need > > for an additional conditional compilation stage. > > No, you need this for a log-specific LOG() macro (and in this case it puts > them all at the LOG_DEBUG level, as a lot of the module LOG macros do, > though some leave level exposed like MSG). This is in keeping with the > typical pattern though. I'm objecting to the conditional compilation of LOG(). Why not simply define LOG() as PR_LOG(....). If PR_LOGGING is not set, that will just turn into a no-op. Though now that you mention it, avoiding the log level parameter seems prett silly. > > http://firefox-codereview.appspot.com/19001/diff/13001/content/media/webrtc/ > > A... > > content/media/webrtc/AudioDeviceGonkStubs.cpp:174: *volume = 0; > > Why are you setting values when these are supposed to be asserted? > > Because we may not catch/fix anything that would fire an assert in testing, > and returning a safe value in opt builds makes sense. If we believe it's > unsafe to call this even in opt builds with a safe return, we should either > fix the calling code, or use MOZ_CRASH. I didn't see any that I thought > required MOZ_CRASH I understand what the semantics of assert are here, but as noted above, the Chromium code ignores the arguments and then returns success. The reason for the assert is to detect if GIPS has changed out from under us and now depends on some other behavior.

(In reply to Eric Rescorla (:ekr) from comment #83) > (In reply to Jan-Ivar Bruaroey [:jib] from comment #75) > > Thanks! More quick notes on select items only: > > > > > It seems the stubs still need more review. Please go through and > > > examine them for: > > > > > > - Side effects on the arguments when they are unimplemented. (Why?) > > > - Differences from the Chromium code. > > > > Chromium ignores arguments. How do I reconcile this? > > Yes, I think you should probably ignore the arguments, as Chrome does. > Where you differ from Chrome, please explain why. > > > > http://firefox-codereview.appspot.com/19001/diff/13001/content/media/webrtc/A... > > > content/media/webrtc/AudioDeviceGonk.cpp:50: static PRLogModuleInfo *sLog = > > > PR_NewLogModule("audio"); > > > If this is a static, why are you getting it repeatedly? > > > > > > Doesn't this need a thread lock? > > > > http://en.wikipedia.org/wiki/Static_variable#Example > > > > That's not an answer. You're repeatedly assigning a static > value of a function. That's simply not correct. Sorry, I slightly misread this and didn't notice that it was a *function* static. In a non-threaded environment, this will be fine. In a threaded environment, you have a race condition which depends for correctness on API guarantees which are neither enforced nor documented by this interface.

(In reply to Eric Rescorla (:ekr) from comment #84) > (In reply to Randell Jesup [:jesup] from comment #78) > > (In reply to Eric Rescorla (:ekr) from comment #73) > > > > > http://firefox-codereview.appspot.com/19001/diff/13001/content/media/webrtc/ > > > A... > > > content/media/webrtc/AudioDeviceGonk.cpp:48: PRLogModuleInfo* GetAudioLog() > > > Please do not name extern functions this generically. Why isn't this a class > > > static? > > > > So you can use the log from more than one class - this is the normal > > pattern. Especially with logs with non-class-specific names. > > I don't think that's a good argument in this case: > > 1. This actually shouldn't be "audio". That's going to make it super-hard > to actually debug anything. "webrtc_adm"? Or "webrtc_audio"? (Or "gonk_audio") Or reuse "getUserMedia" which already is used.... > 2. I strongly suspect that everyone else who wants to use "audio" > just calls PR_NewLogModule() directly. Sure. > 3. This isn't even declared in a header file, so it's of scant use to > others. Excellent point; it should be in a .h file. > 4. Having a function like this exported to multiple threads w/o a > thread lock is a terrible idea, because it's totally impossible to > guarantee the API contract. That's the larger meta-issue. To that point, either add a grab of one of the crit sections around the LOG() in the constructor, or document why it can only be called from one thread. The crit sect is easier and guaranteed safe, and we already have them here. (Also, you need to test the static before doing PR_NewLogModule) > This function is only used by this class. It should be either a member > fxn or a class static. Or just call GetUserMediaLog() :-) Seriously, that's defined, and having these debugs on getusermedia:5 probably makes some sense - but I'd be equally ok with a new (reasonably specific) logging name. > > > http://firefox-codereview.appspot.com/19001/diff/13001/content/media/webrtc/ > > > A... > > > content/media/webrtc/AudioDeviceGonk.cpp:53: #define LOG(msg) > > > PR_LOG(GetAudioLog(), PR_LOG_DEBUG, msg) > > > PR_LOG already has this wrapper. That's why it's been done as a macro. No > > > need > > > for an additional conditional compilation stage. > > > > No, you need this for a log-specific LOG() macro (and in this case it puts > > them all at the LOG_DEBUG level, as a lot of the module LOG macros do, > > though some leave level exposed like MSG). This is in keeping with the > > typical pattern though. > > I'm objecting to the conditional compilation of LOG(). Why not simply > define LOG() as PR_LOG(....). If PR_LOGGING is not set, that will just > turn into a no-op. This is also the standard paradigm for LOG() in the tree - a macro around PR_LOG() Try cfind "#define LOG(" > Though now that you mention it, avoiding the log level parameter seems prett > silly. Done lots of places, though, where there's little or no need for multiple levels. If levels are needed in the future in a file/module, it's not hard to revise the callers to add them. Both ways are fine, and both are in extensive use in the tree. > > > http://firefox-codereview.appspot.com/19001/diff/13001/content/media/webrtc/ > > > A... > > > content/media/webrtc/AudioDeviceGonkStubs.cpp:174: *volume = 0; > > > Why are you setting values when these are supposed to be asserted? > > > > Because we may not catch/fix anything that would fire an assert in testing, > > and returning a safe value in opt builds makes sense. If we believe it's > > unsafe to call this even in opt builds with a safe return, we should either > > fix the calling code, or use MOZ_CRASH. I didn't see any that I thought > > required MOZ_CRASH > > I understand what the semantics of assert are here, but as noted above, > the Chromium code ignores the arguments and then returns success. The > reason for the assert is to detect if GIPS has changed out from under us > and now depends on some other behavior. If the API usage changes, the asserts we're adding would help us catch that before we get near release. But all of this is a minor and forward-hypothetical issue - we can do lots of things here, and honestly I'm open most ways of doing this if someone feels strongly about it. I just dislike having code that is on the face of it buggy (as is appears the Chrome code is, if it returns success without setting the out param).

(In reply to Randell Jesup [:jesup] from comment #86) > (In reply to Eric Rescorla (:ekr) from comment #84) > > (In reply to Randell Jesup [:jesup] from comment #78) > > > (In reply to Eric Rescorla (:ekr) from comment #73) > > > > > > > http://firefox-codereview.appspot.com/19001/diff/13001/content/media/webrtc/ > > > > A... > > > > content/media/webrtc/AudioDeviceGonk.cpp:48: PRLogModuleInfo* GetAudioLog() > > > > Please do not name extern functions this generically. Why isn't this a class > > > > static? > > > > > > So you can use the log from more than one class - this is the normal > > > pattern. Especially with logs with non-class-specific names. > > > > I don't think that's a good argument in this case: > > > > 1. This actually shouldn't be "audio". That's going to make it super-hard > > to actually debug anything. > > "webrtc_adm"? Or "webrtc_audio"? (Or "gonk_audio") Or reuse > "getUserMedia" which already is used.... > > > 2. I strongly suspect that everyone else who wants to use "audio" > > just calls PR_NewLogModule() directly. > > Sure. > > > 3. This isn't even declared in a header file, so it's of scant use to > > others. > > Excellent point; it should be in a .h file. > > > 4. Having a function like this exported to multiple threads w/o a > > thread lock is a terrible idea, because it's totally impossible to > > guarantee the API contract. > > That's the larger meta-issue. To that point, either add a grab of one of > the crit sections around the LOG() in the constructor, or document why it > can only be called from one thread. The crit sect is easier and guaranteed > safe, and we already have them here. (Also, you need to test the static > before doing PR_NewLogModule) Actually, I believe JIB is right that you don't, because function-level statics are only initialized once. Though I too, found this confusing. > > This function is only used by this class. It should be either a member > > fxn or a class static. > > Or just call GetUserMediaLog() :-) Seriously, that's defined, and having > these debugs on getusermedia:5 probably makes some sense - but I'd be > equally ok with a new (reasonably specific) logging name. That's totally fine with me. > > I'm objecting to the conditional compilation of LOG(). Why not simply > > define LOG() as PR_LOG(....). If PR_LOGGING is not set, that will just > > turn into a no-op. > > This is also the standard paradigm for LOG() in the tree - a macro around > PR_LOG() > Try cfind "#define LOG(" Sure, but why does it need to be conditional? I appreciate it's common, but does it do anything? > > > > http://firefox-codereview.appspot.com/19001/diff/13001/content/media/webrtc/ > > > > A... > > > > content/media/webrtc/AudioDeviceGonkStubs.cpp:174: *volume = 0; > > > > Why are you setting values when these are supposed to be asserted? > > > > > > Because we may not catch/fix anything that would fire an assert in testing, > > > and returning a safe value in opt builds makes sense. If we believe it's > > > unsafe to call this even in opt builds with a safe return, we should either > > > fix the calling code, or use MOZ_CRASH. I didn't see any that I thought > > > required MOZ_CRASH > > > > I understand what the semantics of assert are here, but as noted above, > > the Chromium code ignores the arguments and then returns success. The > > reason for the assert is to detect if GIPS has changed out from under us > > and now depends on some other behavior. > > If the API usage changes, the asserts we're adding would help us catch that > before we get near release. But all of this is a minor and > forward-hypothetical issue - we can do lots of things here, and honestly I'm > open most ways of doing this if someone feels strongly about it. I just > dislike having code that is on the face of it buggy (as is appears the > Chrome code is, if it returns success without setting the out param). I think I would deal with this with a MOZ_CRASH() But I can go either way here if you want to set the side effects.

>Right; this was one of my biggest questions - check how stop() works. Jesup asked me this; i.e. what guarantees that stop() has. AudioRecord::stop() *in C++* isn't a supported API at all on Android, so there really are no guarantees at all, let alone documentation. The only thing we can do is reverse-engineer the source. Some digging finds this: http://grepcode.com/file/repository.grepcode.com/java/ext/com.google.android/android/4.0.1_r1/android/media/AudioRecord.java#764 The callback contains a WeakReference to the object that was relevant when invoked. The Android code checks if the actual object the WeakReference points to is still alive, and if it isn't, it turns itself into a no-op. This strongly suggests you can get callbacks even after you've already killed the object. Some more digging seems to indicate the callbacks will only stop after .release() or finalize() has been called. In other words, it looks to me like this code: void AudioDeviceGonk::RecordingCbf(int event, void *user, void *info) { static_cast<AudioDeviceGonk *>(user)->RecordingCbf (event, info); } Can be called when user is pointing to an object that is already freed.

Comment on attachment 769440 [details] [diff] [review] Gonk audio recording device - cleanup + refactored (3) r? to padenot specifically for the threading issues around stop()

Thanks for looking at this, though I drew a different conclusion. FWIW the code I looked at is here (from padenot): http://androidxref.com/4.0.4/xref/frameworks/base/media/libmedia/AudioRecord.cpp#104 Here, the lifespan of mClientRecordThread (the API's callback thread which we're using) is within the lifespan of AudioRecord: AudioRecord::~AudioRecord() 105{ 106 if (mStatus == NO_ERROR) { 107 // Make sure that callback function exits in the case where 108 // it is looping on buffer empty condition in obtainBuffer(). 109 // Otherwise the callback thread will never exit. 110 stop(); 111 if (mClientRecordThread != 0) { 112 mClientRecordThread->requestExitAndWait(); 113 mClientRecordThread.clear(); 114 } 115 mAudioRecord.clear(); Since AudioDeviceGonk (the callback's user arg), owns AudioRecord, it is guaranteed to exist for the life of the callback, so I conclude that there's no ASAN risk. But for TSAN, I see that stop() itself only calls: http://androidxref.com/4.0.4/xref/frameworks/base/media/libmedia/AudioRecord.cpp#345 364 t->requestExit(); From the lack of "AndWait" in the latter function-name, I agree the callback *thread* can still be alive after stop(), though that's not the same as conceding that the callback can be called after stop(), in fact, stop() seems to contain attempts to guarantee that it is in fact not called, barring a bug... I think there's a bug here: http://androidxref.com/4.0.4/xref/frameworks/base/media/libmedia/AudioRecord.cpp#677 I think after acquiring mLock, it needs to test mActive, like this: 677 mLock.lock(); 678 if (mActive) { // proceed If it had done this, then this would guarantee that the callback was never called after stop() - Seems desirable enough to me to call it a bug. The workaround for us, I think, is to test on mActive in the callback. Thanks everyone for suspecting this area.

> 677 mLock.lock(); Actually, I see the lock is only held briefly, only enough to get a strong pointer (to prevent ASAN), not long enough to protect the callback (TSAN). My workaround should be the same though.

Yeah so I read the android code slightly wrong, so please disregard my claim of a bug. It seems mLock was never intended to protect the callback. Here's the fix I'm planning in AudioDeviceGonk.cpp (in upcoming patch): case AudioRecord::EVENT_MORE_DATA: { ReentrantMonitorAutoEnter lock(mCritSectStop); > if (!mRecording) { > return; > } this lock *is* held for the duration of this callback.

Ok, what if you're stopped right before the lock(), and stop() is called from another thrad, and then the object goes away, and then your code continues here in the callback to try to grab the lock? FYI, the gerv needinfo is resolved

> Ok, what if you're stopped right before the lock(), and stop() is called from > another thrad, and then the object goes away, and then your code continues here > in the callback to try to grab the lock? (Assuming lock() = mCritSectStop, and object going away = AudioDeviceGonk) Object is guaranteed not to go away during the callback. See Comment 90.

(In reply to Jan-Ivar Bruaroey [:jib] from comment #94) > > Ok, what if you're stopped right before the lock(), and stop() is called from > > another thrad, and then the object goes away, and then your code continues here > > in the callback to try to grab the lock? > > (Assuming lock() = mCritSectStop, and object going away = AudioDeviceGonk) > > Object is guaranteed not to go away during the callback. See Comment 90. Thanks - lots of messageshere. I see it stops the thread and waits for it to exit before destroying the object.

In order to speed things up, I'm putting this patch up for review while I'm researching all the calls to the stubs as ekr requested. Update: - Documented threading model + Fixed race on stop() - Refactored members into two classes to make locking/threading model obvious - PR_LOG now uses existing GetUserMediaLog() + removed redundant #ifdefs - Half a memcpy slower to avoid modding GIPS (scrapped SetRecordedBufferCombine) - Unimplemented stubs (which return success) set falsy results + chromium parity - Stub return values no longer hidden in macros - removed unused AttachAudioBuffer construct inherited from android

Try - https://tbpl.mozilla.org/?tree=Try&rev=7886be299355

Comment on attachment 770564 [details] [diff] [review] Gonk audio recording device - cleanup + refactored (4) I forgot to switch MOZ_ASSERT to MOZ_CRASH, so please imagine this is done when you review. Thanks.

IF you do that, I am fine with you not double-checking any of the stubs agains tthe GIPS code. We'll find out soon enought.

OK great.

Comment on attachment 770564 [details] [diff] [review] Gonk audio recording device - cleanup + refactored (4) Review of attachment 770564 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/webrtc/AudioDeviceGonk.cpp @@ +80,5 @@ > + , mInitialized(false) > + , mRecIsInitialized(false) > + , mRecAudioSource(1) // 1 is AudioSource.MIC which is our default > +{} > + trailing space - several instances; please fix each @@ +258,5 @@ > + __FUNCTION__, mId, mAudioRecord->initCheck())); > + return -1; > + } > + > + // TODO(jib@mozilla.com): Find optimal delay for AEC (may be hardware specific) Remove email address @@ +451,5 @@ > + // etc. to the observer using callback > + > + // Downstream system can't handle AudioRecord's 1024 minFrameCount, and > + // expects 10ms chunks (e.g. 160), so break things up and handle partials. > + File a followup bug for adding AppendToBuffer()/whatever, put the bug # here, and open an issue at webrtc.org and flag it in that bug. You don't need to provide a patch (on webrtc.org's code tracker), though if you can do so quickly that's fine. (It should be pretty trivial). @@ +452,5 @@ > + > + // Downstream system can't handle AudioRecord's 1024 minFrameCount, and > + // expects 10ms chunks (e.g. 160), so break things up and handle partials. > + > + unsigned int i = 0; uint32_t please @@ +471,5 @@ > + SetVQEData(0, mLatency, 0); > + DeliverRecordedData(); > + } > + mNumPartialFrames = b->frameCount - i; > + if (mNumPartialFrames) { For integer types, I'd prefer that to be > 0 (or != 0 for unsigned ints). Same for an earlier test. Note mNumPartialFrames is an int

> File a followup bug for adding AppendToBuffer()/whatever, put the bug # here Filed as Bug 889933.

I reviewed this on rietveld. I believe you have at least one bug in the cbf. https://firefox-codereview.appspot.com/19001/ There are a bunch of nits here, but I think you have some actual bugs in the cbf. https://firefox-codereview.appspot.com/19001/diff/18001/content/media/webrtc/... File content/media/webrtc/AudioDeviceGonk.cpp (right): https://firefox-codereview.appspot.com/19001/diff/18001/content/media/webrtc/... content/media/webrtc/AudioDeviceGonk.cpp:50: #define WARN(msg) PR_LOG(GetUserMediaLog(), PR_LOG_WARNING, msg) Do you need #undef WARN https://firefox-codereview.appspot.com/19001/diff/18001/content/media/webrtc/... content/media/webrtc/AudioDeviceGonk.cpp:236: // members are guaranteed to exist for the life of callback. No ASAN risk. I'm not sure you want to use ASAN/TSAN here. Those are just tools. https://firefox-codereview.appspot.com/19001/diff/18001/content/media/webrtc/... content/media/webrtc/AudioDeviceGonk.cpp:253: false, // threadCanCallJava Are these tabs? https://firefox-codereview.appspot.com/19001/diff/18001/content/media/webrtc/... content/media/webrtc/AudioDeviceGonk.cpp:316: // (calls may not stop immediately, but access is neutered). can you rewrite this comment. I'm not sure what access is neutered means. https://firefox-codereview.appspot.com/19001/diff/18001/content/media/webrtc/... File content/media/webrtc/AudioDeviceGonk.h (right): https://firefox-codereview.appspot.com/19001/diff/18001/content/media/webrtc/... content/media/webrtc/AudioDeviceGonk.h:233: void SetLatency(uint16_t latency); Whitespace. https://firefox-codereview.appspot.com/19001/diff/18001/content/media/webrtc/... content/media/webrtc/AudioDeviceGonk.h:233: void SetLatency(uint16_t latency); Maybe rename latency to latencyMS or something? https://firefox-codereview.appspot.com/19001/diff/18001/content/media/webrtc/... content/media/webrtc/AudioDeviceGonk.h:235: // All member access is locked s/locked/protected by mLock/ https://firefox-codereview.appspot.com/19001/diff/18001/content/media/webrtc/... content/media/webrtc/AudioDeviceGonk.h:246: mutable mozilla::ReentrantMonitor mLock; What is mutable doing here? https://firefox-codereview.appspot.com/19001/diff/24001/content/media/webrtc/... File content/media/webrtc/AudioDeviceGonk.cpp (right): https://firefox-codereview.appspot.com/19001/diff/24001/content/media/webrtc/... content/media/webrtc/AudioDeviceGonk.cpp:50: #define WARN(msg) PR_LOG(GetUserMediaLog(), PR_LOG_WARNING, msg) #undef WARN? https://firefox-codereview.appspot.com/19001/diff/24001/content/media/webrtc/... content/media/webrtc/AudioDeviceGonk.cpp:238: // TSAN: All members accessed by RecordingBuffer::Cbf are inside that subclass I wouldn't use ASAN and TSAN as terms this way. They're tools for detecting specific kinds of errors. You should refer to the actual errors. https://firefox-codereview.appspot.com/19001/diff/24001/content/media/webrtc/... content/media/webrtc/AudioDeviceGonk.cpp:241: // around stop as well, and test on RecordingBuffer::mRecording in callback Wouldn't it be a lot simpler to just instantiate a new mAudioRecord with each call to StartRecording(). Then you wouldn't need any recording interlock here. https://firefox-codereview.appspot.com/19001/diff/24001/content/media/webrtc/... content/media/webrtc/AudioDeviceGonk.cpp:262: // TODO(jib@mozilla.com): Find optimal delay for AEC (may be hardware specific) Is this TODO still active? Where would you get it other than from here? https://firefox-codereview.appspot.com/19001/diff/24001/content/media/webrtc/... content/media/webrtc/AudioDeviceGonk.cpp:316: // (calls may not stop immediately, but access is neutered). This comment is pretty unclear. What does "access is neutered" mean? https://firefox-codereview.appspot.com/19001/diff/24001/content/media/webrtc/... content/media/webrtc/AudioDeviceGonk.cpp:458: if (mNumPartialFrames) { What happens if Android removes the 1024 restriction. As a thought experiment, say b->frameCount is always 1. This is not going to be good. https://firefox-codereview.appspot.com/19001/diff/24001/content/media/webrtc/... content/media/webrtc/AudioDeviceGonk.cpp:471: SetVQEData(0, mLatency, 0); Don't you need to recompute latency here, since you are not delivering packets all at once. https://firefox-codereview.appspot.com/19001/diff/24001/content/media/webrtc/... File content/media/webrtc/AudioDeviceGonk.h (right): https://firefox-codereview.appspot.com/19001/diff/24001/content/media/webrtc/... content/media/webrtc/AudioDeviceGonk.h:58: const uint32_t N_REC_SAMPLES_PER_SEC = 16000; // Same as for android I thought Android didn't guarantee that rates != 44.1 kHz would work. https://firefox-codereview.appspot.com/19001/diff/24001/content/media/webrtc/... content/media/webrtc/AudioDeviceGonk.h:224: // by the AudioRecord API's mClientRecordingThread, which we're using. I don't understand this reference to mClientRecordThread. That's an internal detail. The question is what the API guarantees are. https://firefox-codereview.appspot.com/19001/diff/24001/content/media/webrtc/... content/media/webrtc/AudioDeviceGonk.h:236: mutable mozilla::ReentrantMonitor mLock; Why is this "mutable". Are there const methods that require the lock? https://firefox-codereview.appspot.com/19001/diff/24001/content/media/webrtc/... content/media/webrtc/AudioDeviceGonk.h:246: mutable mozilla::ReentrantMonitor mLock; Same question as above.

(In reply to Eric Rescorla (:ekr) from comment #103) > I reviewed this on rietveld. I believe you have at least one bug in the cbf. > > https://firefox-codereview.appspot.com/19001/ > > There are a bunch of nits here, but I think you have some actual bugs in the > cbf. FYI, ekr, derf and I discussed this in #media > https://firefox-codereview.appspot.com/19001/diff/24001/content/media/webrtc/ > ... > content/media/webrtc/AudioDeviceGonk.cpp:262: // TODO(jib@mozilla.com): Find > optimal delay for AEC (may be hardware specific) > Is this TODO still active? Where would you get it other than from here? It turns out that the latency() value from the Android driver is a kludge/lower-bound (just the inherent buffering due to 1024-sample chunks). It's unclear if we can do better (add other fixed delays), but we may be able to. For example, we could add hardware/driver-specific known delays for specific hardware, to save more AEC "tail" for real cancelling. > https://firefox-codereview.appspot.com/19001/diff/24001/content/media/webrtc/ > ... > content/media/webrtc/AudioDeviceGonk.cpp:458: if (mNumPartialFrames) { > What happens if Android removes the 1024 restriction. As a thought > experiment, > say b->frameCount is always 1. This is not going to be good. Delivering audio 1 sample at a time will never be good (though it should work). However, what ekr was implying here was that frameCount is an unsigned type (I checked, it's "size_t"). Right now, frameCount will always be larger than mNumFrames10ms, but if it isn't in the future somehow (like it stops returning 1024 always) the math will blow up due to the unsignedness. "i <= b->frameCount - mNumFrames10ms". Note that simple switching to 44100 won't do it, we'd need to switch to 102500 Hz or higher to cause this problem with the current 1024 sample buffers (or so, not sure what they do with rate%100 != 0 sample rates). Also, if less than 10ms is delivered, the "finish filling partial" section may fail. Instead, handle too little input to deliver a 10ms chunk up front: if (mNumPartialFrames + b->frameCount < mNumFrames10ms) { memcpy(&mPartialBuffer[mNumPartialFrames], &b->i16[0], b->frameCount * sizeof(int16_t)); mNumPartialFrames += b->frameCount; } else { // We know we have at least 1 10ms chunk to deliver >if (mNumPartialFrames) { > MOZ_ASSERT(mNumPartialFrames < mNumFrames10ms); > i = mNumFrames10ms - mNumPartialFrames; ... > for(; i <= b->frameCount - mNumFrames10ms; i += mNumFrames10ms) Since b->frameCount may be less than nNumFrames10ms (if we delivered audio data right above combined with a partial), we need to account for thisby rearranging the calculation: for(; mNumFrames10ms <= b->frameCount - i; i += mNumFrames10ms) 'i' starts at 0 (or what was eaten by the partial), and can never get above b->frameCount. > https://firefox-codereview.appspot.com/19001/diff/24001/content/media/webrtc/ > ... > content/media/webrtc/AudioDeviceGonk.cpp:471: SetVQEData(0, mLatency, 0); > Don't you need to recompute latency here, since you are not delivering > packets > all at once. mLatency is the hardware driver's lower bound on latency (though the actual lower bound may be higher per above). You need to tell the AEC what the lower-bound is. Also, I'm not sure if it's safe to change it on a function-call-to-call basis, and if it does, it's likely the AEC will need to reset unless it's smart about handling that - and I doubt they saw a need to. > https://firefox-codereview.appspot.com/19001/diff/24001/content/media/webrtc/ > ... > File content/media/webrtc/AudioDeviceGonk.h (right): > > https://firefox-codereview.appspot.com/19001/diff/24001/content/media/webrtc/ > ... > content/media/webrtc/AudioDeviceGonk.h:58: const uint32_t > N_REC_SAMPLES_PER_SEC > = 16000; // Same as for android > I thought Android didn't guarantee that rates != 44.1 kHz would work. Android specs (checked on a bunch of revs) say 8000, 16000 and 44100 must work. However, AudioRecord's API docs say that only 44100 works reliably across all devices (but you can try 11025/16000/22050), but the AudioRecord.h file has 8000 as the default frequency. And http://stackoverflow.com/questions/5300501/audiorecord-latency-android-gt-i5700?rq=1 (answer 3 in particular) shows that even 44100 may not work - and there's no way to reliably probe. What a mess... a) file a bug as we talked about for switching to 44100 after bug 886886 lands (and make it depend on 886886). We can't switch this patch to 44100 until that lands. We believe most of not all B2G devices in use support 16000 (unagi and peak do I believe). b) Note this fact and bug # in the code c) if devices are used with B2G that don't support 44100, we'll need to special-case them explicitly like several instances with video.

Thanks, will fix with upcoming patch today. Comments on open issues: > #define WARN(msg) > PR_LOG(GetUserMediaLog(), PR_LOG_WARNING, msg) > Do you need #undef WARN I didn't (see try in comment 97), but can't hurt I suppose. > https://firefox-codereview.appspot.com/19001/diff/18001/content/media/webrtc/... > content/media/webrtc/AudioDeviceGonk.cpp:253: false, // > threadCanCallJava > Are these tabs? No, they are spaces. > content/media/webrtc/AudioDeviceGonk.h:233: void SetLatency(uint16_t latency); > Whitespace. I don't see any white-space here. > content/media/webrtc/AudioDeviceGonk.h:246: mutable mozilla::ReentrantMonitor > mLock; > What is mutable doing here? Needed to compile. AudioDeviceGonk::Initialized() is a const method and needs to access mLock. > content/media/webrtc/AudioDeviceGonk.cpp:241: // around stop as well, and test > on RecordingBuffer::mRecording in callback > Wouldn't it be a lot simpler to just instantiate a new mAudioRecord with each > call to StartRecording(). Then you wouldn't need any recording interlock here. Sorry, I'll update that comment, as there isn't a separate lock around stop() anymore. Instead I call mAudioDeviceBuffer.SetRecording(false) first which safely sets mRecording to false, rendering subsequent (or waiting) callbacks harmless. > https://firefox-codereview.appspot.com/19001/diff/24001/content/media/webrtc/... > content/media/webrtc/AudioDeviceGonk.cpp:471: SetVQEData(0, mLatency, 0); > Don't you need to recompute latency here, since you are not delivering packets > all at once. I think you guys arrived at that I don't, is that correct? If not, please let me know. > https://firefox-codereview.appspot.com/19001/diff/24001/content/media/webrtc/... > content/media/webrtc/AudioDeviceGonk.h:236: mutable mozilla::ReentrantMonitor > mLock; > Why is this "mutable". Are there const methods that require the lock? Yes, AudioDeviceGonk::RecordingDelay(uint16_t* delayMS) const

Update. Carrying forward r+ from rjesup. - Fixed nits - MOZ_CRASH - Pasted in "frameCount<10ms"-fix from Randell, thanks!

Comment on attachment 772277 [details] [diff] [review] Gonk audio recording device - cleanup + refactored (5) r=rjesup Review of attachment 772277 [details] [diff] [review]: ----------------------------------------------------------------- I have looked only at the android API bits, I can't comment on GIPS interface/MediaEngines stuff. ::: content/media/webrtc/AudioDeviceGonk.cpp @@ +242,5 @@ > + // after we set RecordingBuffer::SetRecording(false) are harmless. > + > + mAudioRecord = new AudioRecord(); > + mAudioRecord->set(mRecAudioSource, // inputSource > + N_REC_SAMPLES_PER_SEC, // sampleRate If you pass 0 here, it will be set to the device's default, which could be something else than 16k (we don't have OEM modifications source code on androidxref.com). From what I read, there is no way to get native input samplerate (there are TODOs in the code about that, though). Letting the device choose it for us (by passing 0) and querying it afterwards would be more robust, considering there is no way we can test on all the devices that exist.

Looking at http://androidxref.com/4.0.4/xref/frameworks/base/include/media/AudioRecord.h#43 might that give us 8Khz on some devices that could do more? Is that desirable? I guess I'm asking how do we know that OEMs modify this default to a desirable value rather than leaving it at 8Khz?

This additional patch modifies the first one to rely on the device's default sample-rate. - Note that this uses 8kHz on the unagi. :-/ I made it a separate patch so we can decide whether we want to use it.

Comment on attachment 773730 [details] [diff] [review] Add'l mod to use device's default sample-rate (1) Actually, I have to look at more code to check if we can get the value from the HAL.

Comment on attachment 773730 [details] [diff] [review] Add'l mod to use device's default sample-rate (1) Review of attachment 773730 [details] [diff] [review]: ----------------------------------------------------------------- r- solely because as I suspected, some (Unagi for one) devices return the default-in-the-.h-file value of 8000 if you pass in 0Hz. I doubt the Unagi hardware is actually 8000Hz maximum native. I'd love to have a way of *safely* using the actual hardware rate/fast-path. But this appears not to be it.

> I'd love to have a way of *safely* using the actual hardware rate/fast-path. > But this appears not to be it. I vote we open a separate issue for this then, to not block the rest of the feature.

Comment on attachment 772277 [details] [diff] [review] Gonk audio recording device - cleanup + refactored (5) r=rjesup Review of attachment 772277 [details] [diff] [review]: ----------------------------------------------------------------- This needs at least one more round of cleanup. The copyrights on all the files we borrowed from WebRTC need updating to have Mozilla. Please adopt a more consistent logging policy. I'm fine with a big pile of logging, but I would like you to log all over the place then, not just occasionally. Alternately, consistently log less. The latency question seems to me to still be outstanding. Can you point to either the Google discussion where you resolved it or the bug where it is open. ::: content/media/webrtc/AudioDeviceGonk.cpp @@ +36,5 @@ > +#include "common_audio/signal_processing/include/signal_processing_library.h" > +#include "system_wrappers/interface/ref_count.h" > +#include "system_wrappers/interface/event_wrapper.h" > + > +#include <assert.h> You don't need assert.h b/c you are calling moz_assert() @@ +37,5 @@ > +#include "system_wrappers/interface/ref_count.h" > +#include "system_wrappers/interface/event_wrapper.h" > + > +#include <assert.h> > +#include <string.h> Prefer system headers ahead of other headers. Also, why do you need string.h? Even if you do, generally style or C++ is to ask for <cstring> @@ +71,5 @@ > +// ---------------------------------------------------------------------------- > +// AudioDeviceGonk - ctor > +// ---------------------------------------------------------------------------- > + > +AudioDeviceGonk::AudioDeviceGonk(const int32_t id) Having this be const in the signature doesn't seem to add a huge amount of value. @@ +79,5 @@ > + , mId(id) > + , mLastProcessTime(PR_IntervalNow()) > + , mInitialized(false) > + , mRecIsInitialized(false) > + , mRecAudioSource(1) // 1 is AudioSource.MIC which is our default Why are you not using AudioSource.MIC then? @@ +111,5 @@ > + > +// ---------------------------------------------------------------------------- > +// Module::TimeUntilNextProcess > +// > +// Returns the number of milliseconds until the module want a worker thread s/want/wants/ @@ +117,5 @@ > +// ---------------------------------------------------------------------------- > + > +int32_t AudioDeviceGonk::TimeUntilNextProcess() > +{ > + PRUint32 passed = PR_IntervalToMilliseconds (PR_IntervalNow() - mLastProcessTime); Why aren't you locking here? You're using mLastProcesstime @@ +131,5 @@ > + > +int32_t AudioDeviceGonk::Process() > +{ > + ReentrantMonitorAutoEnter lock(mLock); > + mLastProcessTime = PR_IntervalNow(); Why do you have vertical whitespace hre? @@ +176,5 @@ > +bool AudioDeviceGonk::Initialized() const > +{ > + ReentrantMonitorAutoEnter lock(mLock); > + > + LOG(("%s %d output: %d", __FUNCTION__, mId, mInitialized)); Why are you logging here? @@ +187,5 @@ > +// ---------------------------------------------------------------------------- > +int32_t AudioDeviceGonk::InitRecording() > +{ > + ReentrantMonitorAutoEnter lock(mLock); > + mAudioDeviceBuffer.InitRecording(); Please be more consistent about spaces. Does the lock have a whitespace after it or not? @@ +204,5 @@ > + > + // Initialize the microphone > + if (InitMicrophone() == -1) > + { > + WARN(("%s %d InitMicrophone() failed", __FUNCTION__, mId)); Why isn't this causing a failure? Minimally it needs a comment. I note that you are calling a stub here. @@ +210,5 @@ > + > + int minFrameCount; > + if (AudioRecord::getMinFrameCount(&minFrameCount, N_REC_SAMPLES_PER_SEC, > + N_AUDIO_FORMAT, > + N_REC_CHANNELS) != NO_ERROR) { Log here. @@ +217,5 @@ > + > + // release the object > + if (mAudioRecord) { > + mAudioRecord = nullptr; > + } Why does this need to be done explicitly? You are going to free it when you assign later. @@ +219,5 @@ > + if (mAudioRecord) { > + mAudioRecord = nullptr; > + } > + > + int channelMask = N_REC_CHANNELS == 1 ? Please don't use N_REC_CHANNELS here as if it might change. Either assume it's 1 or assign it to a member in the ctor and then compare (preffered). @@ +230,5 @@ > + // learned through code-inspection: http://androidxref.com/4.0.4 > + // > + // We use the AudioRecord's callback feature. This means that our function > + // RecordingBuffer::Cbf() is called on the API's dedicated thread, which is > + // fully managed for us. This is the only other thread. You don't know that. There could be 100 other threads spun up and you wouldn't care. The guarantee here you want is that you (1) don't spin up other threads (2) rely on there being only one Cbf thread (do you?) @@ +236,5 @@ > + // ~AudioRecord() stops mClientRecordThread and waits for it to exit, hence > + // members are guaranteed to exist for the life of callback. > + // > + // All members accessed by RecordingBuffer::Cbf are inside that subclass > + // and protected with that class' mLock. Even though AudioRecord.stop() does class's @@ +238,5 @@ > + // > + // All members accessed by RecordingBuffer::Cbf are inside that subclass > + // and protected with that class' mLock. Even though AudioRecord.stop() does > + // not guarantee synchronous termination of the dedicated thread, extra calls > + // after we set RecordingBuffer::SetRecording(false) are harmless. s/harmless/ignored/ @@ +259,5 @@ > + __FUNCTION__, mId, mAudioRecord->initCheck())); > + return -1; > + } > + > + // TODO(jib): Find optimal delay for AEC (may be hardware specific) File a bug and put the bug number here. @@ +285,5 @@ > + ReentrantMonitorAutoEnter lock(mLock); > + > + if (!mRecIsInitialized) > + { > + LOG(("ERROR: %s %d Recording not initialized", __FUNCTION__, mId)); Why is this not WARN? @@ +290,5 @@ > + return -1; > + } > + if (mAudioDeviceBuffer.IsRecording()) > + { > + LOG(("%s %d Recording already started", __FUNCTION__, mId)); And this? @@ +313,5 @@ > + WARN(("%s %d Recording not initialized", __FUNCTION__, mId)); > + return 0; > + } > + // Setting recording to false safely turns off the callback's member access > + // (calls may not stop immediately, but will do nothing). s/will do nothing/will be ignored/ @@ +339,5 @@ > +// ---------------------------------------------------------------------------- > + > +int32_t AudioDeviceGonk::RegisterEventObserver(AudioDeviceObserver* eventCallback) > +{ > + return 0; Looks like a stub to me. Move to stubs. @@ +361,5 @@ > +{ > + ReentrantMonitorAutoEnter lock(mLock); > + > + *delayMS = mAudioRecord->latency(); > + LOG(("%s %d output: delayMS=%u", __FUNCTION__, mId, *delayMS)); Why are you logging this? @@ +380,5 @@ > +AudioDeviceGonk::RecordingBuffer::RecordingBuffer(const int32_t id) > + : mLock("RecordingLock") > + , mId(id) > + , mRecording(false) > + , mNumFrames10ms(N_REC_SAMPLES_PER_SEC / 100) This seems like it would be better passed as an argument rather than depending on the global const. @@ +389,5 @@ > + SetId(mId); > + > + // default settings for this implementation > + SetRecordingSampleRate(N_REC_SAMPLES_PER_SEC); > + SetRecordingChannels(N_REC_CHANNELS); These values too. Also, it looks like these functions can fail? If so, please check for error. @@ +447,5 @@ > + > + // Downstream system can't handle AudioRecord's 1024 minFrameCount, and > + // expects 10ms chunks (e.g. 160), so break things up and handle partials. > + > + uint32_t i = 0; i only seems to be used in the else clause. please move closer to use site. Also add a comment that i is denominated in frames. ::: content/media/webrtc/AudioDeviceGonk.h @@ +35,5 @@ > + > +#include "audio_device.h" > +#include "audio_device_buffer.h" > +#include "ReentrantMonitor.h" > +#include "nsAutoPtr.h" Is this used? I don't see it in this file and if it's used in the .cpp, include it there not here. @@ +40,5 @@ > +#include "mozilla/Attributes.h" > +#include "mozilla/Scoped.h" > + > +#include "utils/RefBase.h" > +#include "media/AudioRecord.h" Please audit all these include files for whether they are relevant now. @@ +47,5 @@ > +class EventWrapper; > +class AudioDeviceGeneric; > +class AudioDeviceUtility; > +class CriticalSectionWrapper; > +}; None of these seem to be used. Please remove. @@ +55,5 @@ > +class ReentrantMonitor; > +class TimeStamp; > + > +const uint32_t N_REC_SAMPLES_PER_SEC = 16000; // Same as for android > +const uint32_t N_REC_CHANNELS = 1; // default is mono recording Well, since this is a const, it's not just default, right? @@ +57,5 @@ > + > +const uint32_t N_REC_SAMPLES_PER_SEC = 16000; // Same as for android > +const uint32_t N_REC_CHANNELS = 1; // default is mono recording > +const uint32_t N_AUDIO_FORMAT = AUDIO_FORMAT_PCM_16_BIT; > +const uint32_t REC_BUF_SIZE_IN_SAMPLES = 480; // Handle max 10 ms @ 48 kHz Why 48 kHz, since you just said above you are capturing at 16 kHz. Also, shouldn't this be size_t? @@ +70,5 @@ > + virtual int32_t ChangeUniqueId(const int32_t id); > + virtual int32_t TimeUntilNextProcess(); > + virtual int32_t Process(); > + > +public: Why the repeated publics? @@ +71,5 @@ > + virtual int32_t TimeUntilNextProcess(); > + virtual int32_t Process(); > + > +public: > + // Factory methods (resource allocation/deallocation) Please put this next to the ctor/dtor. @@ +77,5 @@ > + const int32_t id, > + const AudioLayer audioLayer = kPlatformDefaultAudio); > + > + // Retrieve the currently utilized audio layer > + virtual int32_t ActiveAudioLayer(AudioLayer* audioLayer) const; Add a comment that these are implementations of the ADM interface. @@ +218,5 @@ > + virtual int32_t EnableBuiltInAEC(bool enable); > + virtual bool BuiltInAECIsEnabled() const; > + > +private: > + No blank line after "private" @@ +226,5 @@ > + class RecordingBuffer : public webrtc::AudioDeviceBuffer { > + public: > + RecordingBuffer(const int32_t id); > + static void Cbf(int event, void *user, void *info); > + void Cbf(int event, void *info); I would rename this to minimize confusion if, for instance, I want to take a pointer to this fxn. @@ +228,5 @@ > + RecordingBuffer(const int32_t id); > + static void Cbf(int event, void *user, void *info); > + void Cbf(int event, void *info); > + bool IsRecording() const; > + void SetRecording(bool on); Add whitespace here. ::: content/media/webrtc/AudioDeviceGonkStubs.cpp @@ +38,5 @@ > +#include "system_wrappers/interface/event_wrapper.h" > +#include "mtransport/runnable_utils.h" > + > +#include <assert.h> > +#include <string.h> Which of these headers are really needed? @@ +55,5 @@ > +// Policy A: MOZ_CRASH > +// > +// Policy B: MOZ_ASSERT + return 0 (success) in opt-builds for parity w/chromium: > +// https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/media > +// except we defensively initialize result-arguments to falsy values on success, I would say "sensible" rather than falsy. @@ +63,5 @@ > +// instead which does not assert. These return values appropriate to function. > +// > +// Query-only methods like *IsAvailable() answer negatively and do not assert > + > +#define MOZ_ADM_UNIMPLEMENTED() MOZ_CRASH("Function not implemented"); Can you use __FUNCTION__ so we know which function failed? @@ +64,5 @@ > +// > +// Query-only methods like *IsAvailable() answer negatively and do not assert > + > +#define MOZ_ADM_UNIMPLEMENTED() MOZ_CRASH("Function not implemented"); > +#define MOZ_ADM_SATISFY_GIPS() WARN(("%s %d not implemented", __FUNCTION__, mId)); Perhaps functions which don't use either MOZ_ADM_UNIMPLEMENTED or MOZ_ADM_SATISFY_GIPS should be in the main file? @@ +68,5 @@ > +#define MOZ_ADM_SATISFY_GIPS() WARN(("%s %d not implemented", __FUNCTION__, mId)); > + > +static char *safe_strncpy (char *dst, const char *src, size_t n) > +{ > + MOZ_ASSERT(n > sizeof(char*)); Presumably you mean sizeof(char)? What do you think this assert is doing, btw? @@ +550,5 @@ > +{ > + safe_strncpy(name, index? "" : "Default Microphone", kAdmMaxDeviceNameSize); > + if (guid) > + { > + safe_strncpy(guid, "", kAdmMaxGuidSize); Why not just do *guid == '\0' @@ +612,5 @@ > +// ---------------------------------------------------------------------------- > + > +int32_t AudioDeviceGonk::SetRecordingDevice(WindowsDeviceType device) > +{ > + return 0; Why doesn't this complain? ::: content/media/webrtc/MediaEngineWebRTC.cpp @@ +33,5 @@ > +#ifdef PR_LOGGING > +PRLogModuleInfo* > +GetUserMediaLog() > +{ > + static PRLogModuleInfo *sLog = PR_NewLogModule("GetUserMedia"); // init'd once There is still a race condition here. Please at least comment it.

(In reply to Eric Rescorla (:ekr) from comment #113) > Comment on attachment 772277 [details] [diff] [review] > Gonk audio recording device - cleanup + refactored (5) r=rjesup > > Review of attachment 772277 [details] [diff] [review]: > ----------------------------------------------------------------- > > This needs at least one more round of cleanup. > > The copyrights on all the files we borrowed from WebRTC need updating to > have Mozilla. Comment 67 from Gerv: "If the file in question is solely code from a WebRTC file plus new code, then put it under the WebRTC licence, even if it's going into the Mozilla part of the code." I can re-verify with him, but that seems clear. > The latency question seems to me to still be outstanding. Can you point to > either > the Google discussion where you resolved it or the bug where it is open. http://logbot.glob.com.au/?c=mozilla%23media&s=7+Jul+2013&e=8+Jul+2013 Especially the discussion around/before 20:39 a) Their existing Android code also uses fixed latency values based on the buffer size used (which is what ->latency() currently returns). b) I believe the code for Pulse and Windows that offsets by 10ms for each returned block is simply wrong, and is counteracted (luckily) by the filter on delay values in the AEC (and limitations on how often they'll change it). I have an open question to Jan Skoglund and Andrew MacDonald at google to verify this (derf and jmspeex are cc'd). Also, while aecm (mobile fixed-point AEC) has the a version of the same filter, it's unclear if this has been fully tested with variable-latency inputs given the Android input code uses fixed values. c) This will only become relevant when the AEC is moved into gUM. Given a) and b), and since latency() is a reasonable lower-bound to latency, we should just return that for now. I'll file a followup to re-evaluate this for gonk when the AEC moves. Until then (especially given c) let's cut further discussion unless there's some game-changing info not previously unearthed. > ::: content/media/webrtc/AudioDeviceGonk.cpp > @@ +36,5 @@ > > +#include "common_audio/signal_processing/include/signal_processing_library.h" > > +#include "system_wrappers/interface/ref_count.h" > > +#include "system_wrappers/interface/event_wrapper.h" > > + > > +#include <assert.h> > > You don't need assert.h b/c you are calling moz_assert() Right > @@ +37,5 @@ > > +#include "system_wrappers/interface/ref_count.h" > > +#include "system_wrappers/interface/event_wrapper.h" > > + > > +#include <assert.h> > > +#include <string.h> > > Prefer system headers ahead of other headers. > > Also, why do you need string.h? Even if you do, generally style or C++ is to > ask for <cstring> I'll try removing it. > @@ +79,5 @@ > > + , mId(id) > > + , mLastProcessTime(PR_IntervalNow()) > > + , mInitialized(false) > > + , mRecIsInitialized(false) > > + , mRecAudioSource(1) // 1 is AudioSource.MIC which is our default > > Why are you not using AudioSource.MIC then? It should. (Note: This was inherited from the GIPS android driver directly) > @@ +117,5 @@ > > +// ---------------------------------------------------------------------------- > > + > > +int32_t AudioDeviceGonk::TimeUntilNextProcess() > > +{ > > + PRUint32 passed = PR_IntervalToMilliseconds (PR_IntervalNow() - mLastProcessTime); > > Why aren't you locking here? You're using mLastProcesstime TimeUntilNextProcess() and Process() are called on the same thread, and both are called with _critSectModules locked (see ProcessThreadImpl::Process()). I'll add a comment. > @@ +217,5 @@ > > + > > + // release the object > > + if (mAudioRecord) { > > + mAudioRecord = nullptr; > > + } > > Why does this need to be done explicitly? You are going to free it when you > assign later. It doesn't need to be done anymore. This is now a ScopedDeletePtr; it used to be a raw pointer and there was also a delete in that if {} block. > @@ +219,5 @@ > > + if (mAudioRecord) { > > + mAudioRecord = nullptr; > > + } > > + > > + int channelMask = N_REC_CHANNELS == 1 ? > > Please don't use N_REC_CHANNELS here as if it might change. Either > assume it's 1 or assign it to a member in the ctor and then compare > (preffered). It's hard-coded in 3 places, and it's not an input - we fix it as a local const in the .h file. If we're changing that, we should be parameterizing all the values there, but I don't see that as gaining us anything at this point for gonk, as we're not getting any inputs telling us how to open the hardware. > @@ +230,5 @@ > > + // learned through code-inspection: http://androidxref.com/4.0.4 > > + // > > + // We use the AudioRecord's callback feature. This means that our function > > + // RecordingBuffer::Cbf() is called on the API's dedicated thread, which is > > + // fully managed for us. This is the only other thread. > > You don't know that. There could be 100 other threads spun up and you > wouldn't care. The guarantee here you want is that you > > (1) don't spin up other threads > (2) rely on there being only one Cbf thread (do you?) I think that's what jib was trying to say (or rather, our callback will be called from only one Cbf thread). I'll reword the comment. > @@ +259,5 @@ > > + __FUNCTION__, mId, mAudioRecord->initCheck())); > > + return -1; > > + } > > + > > + // TODO(jib): Find optimal delay for AEC (may be hardware specific) > > File a bug and put the bug number here. Right > > @@ +380,5 @@ > > +AudioDeviceGonk::RecordingBuffer::RecordingBuffer(const int32_t id) > > + : mLock("RecordingLock") > > + , mId(id) > > + , mRecording(false) > > + , mNumFrames10ms(N_REC_SAMPLES_PER_SEC / 100) > > This seems like it would be better passed as an argument rather than > depending on the global const. We can, but I don't think it buys us anything currently. I'll note that the Android code hard-codes all of this, which is where this is copied from I believe. > > @@ +389,5 @@ > > + SetId(mId); > > + > > + // default settings for this implementation > > + SetRecordingSampleRate(N_REC_SAMPLES_PER_SEC); > > + SetRecordingChannels(N_REC_CHANNELS); > > These values too. Again, this is all parity with the existing android impl. > Also, it looks like these functions can fail? If so, please check for error. They're all defined as having error returns; none of the AudioDeviceBuffer functions actually can fail. Only subclasses of that seem to, and none of the subclasses (except one, OpenSLES) check the result (pulse, windows, alsa, jni). That said, it is a defined return, so we could check it. > @@ +57,5 @@ > > + > > +const uint32_t N_REC_SAMPLES_PER_SEC = 16000; // Same as for android > > +const uint32_t N_REC_CHANNELS = 1; // default is mono recording > > +const uint32_t N_AUDIO_FORMAT = AUDIO_FORMAT_PCM_16_BIT; > > +const uint32_t REC_BUF_SIZE_IN_SAMPLES = 480; // Handle max 10 ms @ 48 kHz > > Why 48 kHz, since you just said above you are capturing at 16 kHz. Inherited from GIPS android code (though the desktop drivers use SAMPLES_PER_SEC/100). I'll change it. > Also, shouldn't this be size_t? All the existing drivers use some form of uint32, but it's not really passed anywhere, so size_t is probably more correct. > @@ +68,5 @@ > > +#define MOZ_ADM_SATISFY_GIPS() WARN(("%s %d not implemented", __FUNCTION__, mId)); > > + > > +static char *safe_strncpy (char *dst, const char *src, size_t n) > > +{ > > + MOZ_ASSERT(n > sizeof(char*)); > > Presumably you mean sizeof(char)? > > What do you think this assert is doing, btw? I'm not sure what was intended, but I think it's to make sure we don't get called with n==0 which would cause a memory error. (and if so, it should be MOZ_ASSERT(n>0)) > ::: content/media/webrtc/MediaEngineWebRTC.cpp > @@ +33,5 @@ > > +#ifdef PR_LOGGING > > +PRLogModuleInfo* > > +GetUserMediaLog() > > +{ > > + static PRLogModuleInfo *sLog = PR_NewLogModule("GetUserMedia"); // init'd once > > There is still a race condition here. Please at least comment it. ok

(In reply to Randell Jesup [:jesup] from comment #114) > (In reply to Eric Rescorla (:ekr) from comment #113) > > Comment on attachment 772277 [details] [diff] [review] > > Gonk audio recording device - cleanup + refactored (5) r=rjesup > > > > Review of attachment 772277 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > This needs at least one more round of cleanup. > > > > The copyrights on all the files we borrowed from WebRTC need updating to > > have Mozilla. > > Comment 67 from Gerv: "If the file in question is solely code from a WebRTC > file plus new code, then put it under the WebRTC licence, even if it's going > into the Mozilla part of the code." > > I can re-verify with him, but that seems clear. > > > The latency question seems to me to still be outstanding. Can you point to > > either > > the Google discussion where you resolved it or the bug where it is open. > > http://logbot.glob.com.au/?c=mozilla%23media&s=7+Jul+2013&e=8+Jul+2013 > > Especially the discussion around/before 20:39 > > a) Their existing Android code also uses fixed latency values based on the > buffer size used (which is what ->latency() currently returns). > b) I believe the code for Pulse and Windows that offsets by 10ms for each > returned block is simply wrong, and is counteracted (luckily) by the filter > on delay values in the AEC (and limitations on how often they'll change it). > I have an open question to Jan Skoglund and Andrew MacDonald at google to > verify this (derf and jmspeex are cc'd). Also, while aecm (mobile > fixed-point AEC) has the a version of the same filter, it's unclear if this > has been fully tested with variable-latency inputs given the Android input > code uses fixed values. > c) This will only become relevant when the AEC is moved into gUM. > > Given a) and b), and since latency() is a reasonable lower-bound to latency, > we should just return that for now. This needs to be documented somewhere permanent. A link to this discussion in the code should be sufficient. > > Prefer system headers ahead of other headers. > > > > Also, why do you need string.h? Even if you do, generally style or C++ is to > > ask for <cstring> > > I'll try removing it. Generally, this code appears to need some include scrubbing. > > @@ +117,5 @@ > > > +// ---------------------------------------------------------------------------- > > > + > > > +int32_t AudioDeviceGonk::TimeUntilNextProcess() > > > +{ > > > + PRUint32 passed = PR_IntervalToMilliseconds (PR_IntervalNow() - mLastProcessTime); > > > > Why aren't you locking here? You're using mLastProcesstime > > TimeUntilNextProcess() and Process() are called on the same thread, and both > are called with _critSectModules locked (see ProcessThreadImpl::Process()). > > I'll add a comment. > > > > @@ +217,5 @@ > > > + > > > + // release the object > > > + if (mAudioRecord) { > > > + mAudioRecord = nullptr; > > > + } > > > > Why does this need to be done explicitly? You are going to free it when you > > assign later. > > It doesn't need to be done anymore. This is now a ScopedDeletePtr; it used > to be a raw pointer and there was also a delete in that if {} block. That's what I thought. > > @@ +219,5 @@ > > > + if (mAudioRecord) { > > > + mAudioRecord = nullptr; > > > + } > > > + > > > + int channelMask = N_REC_CHANNELS == 1 ? > > > > Please don't use N_REC_CHANNELS here as if it might change. Either > > assume it's 1 or assign it to a member in the ctor and then compare > > (preffered). > > It's hard-coded in 3 places, and it's not an input - we fix it as a local > const in the .h file. If we're changing that, we should be parameterizing > all the values there, but I don't see that as gaining us anything at this > point for gonk, as we're not getting any inputs telling us how to open the > hardware. > Right. So my complaint is that this is a conditional but on a hard-coded value. So, either have the hardcoded value in one place and pass around a member that you actually treat as a conditional or don't use this as if it might change here. > > @@ +380,5 @@ > > > +AudioDeviceGonk::RecordingBuffer::RecordingBuffer(const int32_t id) > > > + : mLock("RecordingLock") > > > + , mId(id) > > > + , mRecording(false) > > > + , mNumFrames10ms(N_REC_SAMPLES_PER_SEC / 100) > > > > This seems like it would be better passed as an argument rather than > > depending on the global const. > > We can, but I don't think it buys us anything currently. The usual cleaner factoring reasons. > I'll note that the > Android code hard-codes all of this, which is where this is copied from I > believe. I'm not sure how that matters. > > > > @@ +389,5 @@ > > > + SetId(mId); > > > + > > > + // default settings for this implementation > > > + SetRecordingSampleRate(N_REC_SAMPLES_PER_SEC); > > > + SetRecordingChannels(N_REC_CHANNELS); > > > > These values too. > > Again, this is all parity with the existing android impl. I don't see how that matters. We have no stake in being really close to their code. > > Also, shouldn't this be size_t? > > All the existing drivers use some form of uint32, but it's not really passed > anywhere, so size_t is probably more correct. > > > @@ +68,5 @@ > > > +#define MOZ_ADM_SATISFY_GIPS() WARN(("%s %d not implemented", __FUNCTION__, mId)); > > > + > > > +static char *safe_strncpy (char *dst, const char *src, size_t n) > > > +{ > > > + MOZ_ASSERT(n > sizeof(char*)); > > > > Presumably you mean sizeof(char)? > > > > What do you think this assert is doing, btw? > > I'm not sure what was intended, but I think it's to make sure we don't get > called with n==0 which would cause a memory error. (and if so, it should be > MOZ_ASSERT(n>0)) That would be fine.

Tested and works on Peak

This patch is big because I had to work around the build system. I copied audio_device_opensles_android.(cc|h) to audio_device_opensles_linux.(cc|h) to convince gyp to pick up the new source file. Also, the include for jni.h in audio_device_opensles_linux.h was removed. This can also be removed in the original header - opensles doesn't require any use of jni.h. This has been tested on my Nexus 4.

If we ignore the copy of audio_device_opensles_linux, the patch is about 3.5k. configure.in | 2 - media/webrtc/trunk/webrtc/modules/audio_device/audio_device.gypi | 12 ++++++++++ media/webrtc/trunk/webrtc/modules/audio_device/audio_device_impl.cc | 10 +++++++- 3 files changed, 22 insertions(+), 2 deletions(-)

Michael, Can this be phrased as an Audio Device Module, so we don't need to upstream.

(In reply to Eric Rescorla (:ekr) from comment #120) > Can this be phrased as an Audio Device Module, so we don't need to upstream. To summarize discussion from IRC: Now that our Gonk linking issues with SLES have been resolved, we might be able to use this SLES backend. But we have two patches here that have gone on wildly divergent development paths (message-passing ADM vs. locking, native GIPS code that needs to be upstreamed), and we need to decide 1) Do we need/want both backends? 2) If so, which development path do we follow? (we really should use the same strategy for both) 3) If not, which one do we keep? I have some concerns, but if they can be resolved satisfactorily I'm inclined to go with an upstreamed SLES backend and ditch the AudioRecord ADM. In favor of keeping SLES and ditching AudioRecord: - AudioRecord is not a stable API, while SLES is. - We might be able to share the SLES backend code with the existing upstream Linux/Android code, meaning less maintenance burden for everyone. Possible concerns: - We ditched the locking for both performance and correctness concerns. I don't think there's anything we can do about the performance with the non-ADM SLES approach, but this has not been a measurable problem. We _do_ need to audit the code for similar threading issues (see comment 28 for examples of the defects we found in the Android AudioRecord backend). - SLES is just a wrapper around AudioRecord. Since AudioRecord latency is already pretty bad, we want to make sure that SLES doesn't make that substantially worse. My understanding was that SLES is what is recommended for _low_ latency, but if it's wrapping AudioRecord, it can't possibly do better. So clearly there's something we don't understand here. Instead of spending months more going down the wrong path, some investigation is warranted. I will investigate JB's SLES implementation and see if I can resolve the confusion around the latter point. We can address the former point during review of this patch. Non-concerns: - Backporting to older B2G versions. If we were to do this, mwu tells me we'd update all of Gecko, so we'd always be using a version with the SLES linking issues fixed.

(In reply to Timothy B. Terriberry (:derf) from comment #121) > (In reply to Eric Rescorla (:ekr) from comment #120) > > Can this be phrased as an Audio Device Module, so we don't need to upstream. > > To summarize discussion from IRC: > > Now that our Gonk linking issues with SLES have been resolved, we might be > able to use this SLES backend. But we have two patches here that have gone > on wildly divergent development paths (message-passing ADM vs. locking, > native GIPS code that needs to be upstreamed), and we need to decide > 1) Do we need/want both backends? > 2) If so, which development path do we follow? (we really should use the > same strategy for both) > 3) If not, which one do we keep? > > I have some concerns, but if they can be resolved satisfactorily I'm > inclined to go with an upstreamed SLES backend and ditch the AudioRecord ADM. I concur with this provided we can actually upstream.... > In favor of keeping SLES and ditching AudioRecord: > - AudioRecord is not a stable API, while SLES is. > - We might be able to share the SLES backend code with the existing upstream > Linux/Android code, meaning less maintenance burden for everyone. > > Possible concerns: > - We ditched the locking for both performance and correctness concerns. I > don't think there's anything we can do about the performance with the > non-ADM SLES approach, but this has not been a measurable problem. We _do_ > need to audit the code for similar threading issues (see comment 28 for > examples of the defects we found in the Android AudioRecord backend). > > - SLES is just a wrapper around AudioRecord. Since AudioRecord latency is > already pretty bad, we want to make sure that SLES doesn't make that > substantially worse. My understanding was that SLES is what is recommended > for _low_ latency, but if it's wrapping AudioRecord, it can't possibly do > better. So clearly there's something we don't understand here. Instead of > spending months more going down the wrong path, some investigation is > warranted. > > I will investigate JB's SLES implementation and see if I can resolve the > confusion around the latter point. We can address the former point during > review of this patch. > > Non-concerns: > - Backporting to older B2G versions. If we were to do this, mwu tells me > we'd update all of Gecko, so we'd always be using a version with the SLES > linking issues fixed.

mwu's patch modified to avoid copying files by avoiding gyp's default rules about directories named for an OS. (We rename the files and move them up to the audio_device directory, as they're now shared.) Builds and tests ok on peak. Note: still has an issue with the 'wilhelm' directory

Comment on attachment 777563 [details] [diff] [review] Enable opensles webrtc backend on gonk Review of attachment 777563 [details] [diff] [review]: ----------------------------------------------------------------- r+, but I'd prefer to factor away the dependency on 'wilhelm'. Perhaps webrtc_config.gypi. r? to ted for the build stuff ::: media/webrtc/trunk/webrtc/modules/audio_device/audio_device.gypi @@ +73,5 @@ > }], # OS==android > + ['moz_widget_toolkit_gonk==1', { > + 'include_dirs': [ > + '$(ANDROID_SOURCE)/frameworks/wilhelm/include', > + '$(ANDROID_SOURCE)/system/media/wilhelm/include', let's use some path that won't change and/or is parametrized to the build

Comment on attachment 777563 [details] [diff] [review] Enable opensles webrtc backend on gonk Review of attachment 777563 [details] [diff] [review]: ----------------------------------------------------------------- A lot cleaner for sure. I removed the jni.h include from audio_device_opensles.h though - I didn't see that in this patch. Did you have to remove it? ::: content/media/webrtc/Makefile.in @@ +22,5 @@ > include $(topsrcdir)/ipc/chromium/chromium-config.mk > > ifdef MOZ_WEBRTC > LOCAL_INCLUDES += \ > + -I$(topsrcdir)/media/webrtc/trunk \ What's this for? ::: media/webrtc/trunk/webrtc/modules/audio_device/linux/latebindingsymboltable_linux.cc @@ +37,5 @@ > namespace webrtc_adm_linux { > > inline static const char *GetDllError() { > #if defined(WEBRTC_LINUX) || defined(WEBRTC_BSD) > + const char *err = dlerror(); Is this to suppress a warning? or error?

(In reply to Michael Wu [:mwu] from comment #125) > Comment on attachment 777563 [details] [diff] [review] > Enable opensles webrtc backend on gonk > > Review of attachment 777563 [details] [diff] [review]: > ----------------------------------------------------------------- > > A lot cleaner for sure. I removed the jni.h include from > audio_device_opensles.h though - I didn't see that in this patch. Did you > have to remove it? > > ::: content/media/webrtc/Makefile.in > @@ +22,5 @@ > > include $(topsrcdir)/ipc/chromium/chromium-config.mk > > > > ifdef MOZ_WEBRTC > > LOCAL_INCLUDES += \ > > + -I$(topsrcdir)/media/webrtc/trunk \ > > What's this for? One of the indirect includes that happens now (I forget the chain that led there) wants to include "webrtc/common_types.h", and that's needed to make it work. > ::: > media/webrtc/trunk/webrtc/modules/audio_device/linux/ > latebindingsymboltable_linux.cc > @@ +37,5 @@ > > namespace webrtc_adm_linux { > > > > inline static const char *GetDllError() { > > #if defined(WEBRTC_LINUX) || defined(WEBRTC_BSD) > > + const char *err = dlerror(); > > Is this to suppress a warning? or error? My compiler gives an error on that without const. platforms/android-14/arch-arm/usr/include/dlfcn.h:extern const char* dlerror(void); (on my fedora, dlerror is char *, but not in the android ndk)

(In reply to Michael Wu [:mwu] from comment #125) > A lot cleaner for sure. I removed the jni.h include from > audio_device_opensles.h though - I didn't see that in this patch. Did you > have to remove it? I didn't need to remove it, so I didn't.

(In reply to Randell Jesup [:jesup] from comment #127) > (In reply to Michael Wu [:mwu] from comment #125) > > > A lot cleaner for sure. I removed the jni.h include from > > audio_device_opensles.h though - I didn't see that in this patch. Did you > > have to remove it? > > I didn't need to remove it, so I didn't. Ah, I figured it out. It's a difference between gonk-jb and gonk-ics. On ICS, our removal of dalvik is more complete, so we no longer have access to jni.h. ICS uses https://github.com/mozilla-b2g/fake-dalvik/ whereas JB uses https://github.com/mozilla-b2g/fake-libdvm .

Comment on attachment 777563 [details] [diff] [review] Enable opensles webrtc backend on gonk Review of attachment 777563 [details] [diff] [review]: ----------------------------------------------------------------- I don't have anything to add beyond what's already been commented on.

Comment on attachment 777563 [details] [diff] [review] Enable opensles webrtc backend on gonk Review of attachment 777563 [details] [diff] [review]: ----------------------------------------------------------------- r=me fwiw. Just remove jni.h so we don't break the gonk-jb port.

> > > +static char *safe_strncpy (char *dst, const char *src, size_t n) > > > +{ > > > + MOZ_ASSERT(n > sizeof(char*)); > > > > Presumably you mean sizeof(char)? > > > > What do you think this assert is doing, btw? > > I'm not sure what was intended, but I think it's to make sure we don't get > called with n==0 which would cause a memory error. (and if so, it should be > MOZ_ASSERT(n>0)) The assert is there to catch the common pilot error case where the source is in the form of a string pointer (where the size is unknown at compile time), rather than a string array (where the size is known at compile time). sizeof(source_string) would produce the correct size only in the latter case, and the size of a pointer in the former, very likely producing a buffer overrun. Not a fan of string-math, I prefer to take precautions.

1. You mean the destination, right? That's the apparent semantics of n in this API call. 2. Doesn't this create false positives if dst is a reasonable size < strlen(src)? Consider the following code: char foo[3]; char *bar = "a"; safe_strncpy(foo, bar, sizeof(foo)); This is reasonable code, but if sizeof(char *) == 8 (as it is on 64-bit processors) the assert will fire, no?

1. Yes, sorry. Thanks. 2. Yes, you get a runtime assert on destination-buffers less than 5 to 9 bytes. A reasonable trade-off for a static helper-function function, in my mind, since a buffer-overrun is the deadlier code-maintenance risk here.

This is all kind(In reply to Jan-Ivar Bruaroey [:jib] from comment #133) > 1. Yes, sorry. Thanks. > > 2. Yes, you get a runtime assert on destination-buffers less than 5 to 9 > bytes. A reasonable trade-off for a static helper-function function, in my > mind, since a buffer-overrun is the deadlier code-maintenance risk here. This is all kind of irrelevant since we're not landing this patch, but I don't agree with this assessment. Creating APIs which obviously intend to duplicate APIs but with idiosyncratic semantics is bad You say that this is just a helper function, but this kind of function tends to get pulled out into externals when the next person needs it. Given that stdlib is full of code that has this type of interface, I don't see the point of adding a defense here.

Comment on attachment 777490 [details] [diff] [review] Add opensles backend support for gonk Review of attachment 777490 [details] [diff] [review]: ----------------------------------------------------------------- Reviewing here just as a convenient place. There are many issues. I'm largely of the opinion we should just throw all this code away and start over. ::: media/webrtc/trunk/webrtc/modules/audio_device/linux/audio_device_opensles_linux.cc @@ +868,5 @@ > + record_pcm_.numChannels = N_REC_CHANNELS; > + if (speaker_sampling_rate_ == 44000) { > + record_pcm_.samplesPerSec = 44100 * 1000; > + } else { > + record_pcm_.samplesPerSec = speaker_sampling_rate_ * 1000; This should be mic_sampling_rate_. Note that we've patched this code in bug 886886 to remove the 44100 kludges, but we'll still need to fix this in the new code. @@ +1180,5 @@ > + " Playout is not initialized"); > + return 0; > + } > + > + if (!sles_player_itf_ && !sles_output_mixer_ && !sles_player_) { This is just on crack. If all these pointers are NULL, then we immediately dereference the first one. Needless to say this will either leak all over the place or crash, depending on when this function is called. @@ +1208,5 @@ > + sles_output_mixer_ = NULL; > + } > + } > + > + CriticalSectionScoped lock(&crit_sect_); Locking after we just released the lock seems pretty obtuse. @@ +1309,5 @@ > +} > + > +void AudioDeviceAndroidOpenSLES::PlayerSimpleBufferQueueCallbackHandler( > + SLAndroidSimpleBufferQueueItf queue_itf) { > + if (is_playing_) { Race. @@ +1319,5 @@ > + > + audio = play_queue_.front(); > + play_queue_.pop(); > + > + int num_out = voe_audio_buffer_->RequestPlayoutData(num_samples); Maybe a race, depending on when AttachAudioBuffer can be called, and whether or not access needs to be serialized with recording. @@ +1324,5 @@ > + num_out = voe_audio_buffer_->GetPlayoutData(buf); > + if (num_samples != static_cast<unsigned int>(num_out)) { > + WEBRTC_OPENSL_TRACE(kTraceWarning, kTraceAudioDevice, id_, > + "num (%u) != num_out (%d)", num_samples, num_out); > + play_warning_ = 1; Race. @@ +1335,5 @@ > + num_bytes); > + if (res != SL_RESULT_SUCCESS) { > + WEBRTC_OPENSL_TRACE(kTraceWarning, kTraceAudioDevice, id_, > + " player callback Enqueue failed, %d", res); > + play_warning_ = 1; Race. @@ +1356,5 @@ > + audio_device->RecorderSimpleBufferQueueCallbackHandler(queue_itf); > +} > + > +bool AudioDeviceAndroidOpenSLES::RecThreadFuncImpl() { > + if (is_recording_) { Race. @@ +1368,5 @@ > + > + { > + CriticalSectionScoped lock(&crit_sect_); > + if (rec_voe_audio_queue_.size() <= 0) { > + rec_timer_.Wait(1); You have got to be kidding me. While holding crit_sect_, no less (which is shared with playout). @@ +1380,5 @@ > + rec_voe_ready_queue_.push(audio); > + } > + > + UpdateRecordingDelay(); > + voe_audio_buffer_->SetRecordedBuffer(buf, num_samples); Maybe a race, depending on when AttachAudioBuffer can be called, and whether or not access needs to be serialized with recording. @@ +1382,5 @@ > + > + UpdateRecordingDelay(); > + voe_audio_buffer_->SetRecordedBuffer(buf, num_samples); > + voe_audio_buffer_->SetVQEData(playout_delay_, recording_delay_, 0); > + voe_audio_buffer_->DeliverRecordedData(); Maybe I don't understand the API, but AFAICT this whole thread, and all the audio queues and buffer handling are completely unnecessary. The Android OpenSL ES implementation will stuff data into our buffer until it fills up exactly and then invoke our callback. It does so on a thread that does nothing else, so we're not blocking anything important. So all we're doing is adding more scheduling, synchronization, and allocation overhead, and increasing the chance of buffer xruns. Not to mention race conditions. Instead we can give it a single buffer, which it will hand us in RecorderSimpleBufferQueueCallbackHandler, where we can pass it to GIPS directly, and then hand it back. We only need the one. @@ +1390,5 @@ > +} > + > +void AudioDeviceAndroidOpenSLES::RecorderSimpleBufferQueueCallbackHandler( > + SLAndroidSimpleBufferQueueItf queue_itf) { > + if (is_recording_) { Race. @@ +1420,5 @@ > + total_bytes); > + if (res != SL_RESULT_SUCCESS) { > + WEBRTC_OPENSL_TRACE(kTraceWarning, kTraceAudioDevice, id_, > + " recorder callback Enqueue failed, %d", res); > + rec_warning_ = 1; Race. @@ +1442,5 @@ > + > +void AudioDeviceAndroidOpenSLES::UpdatePlayoutDelay( > + uint32_t nSamplePlayed) { > + // TODO(leozwang): Add accurate delay estimat. > + playout_delay_ = (N_PLAY_QUEUE_BUFFERS - 0.5) * 10 + Race. @@ +1443,5 @@ > +void AudioDeviceAndroidOpenSLES::UpdatePlayoutDelay( > + uint32_t nSamplePlayed) { > + // TODO(leozwang): Add accurate delay estimat. > + playout_delay_ = (N_PLAY_QUEUE_BUFFERS - 0.5) * 10 + > + N_PLAY_QUEUE_BUFFERS * nSamplePlayed / (speaker_sampling_rate_ / 1000); I also don't think this is any more accurate than the recording delay. @@ +1448,5 @@ > +} > + > +void AudioDeviceAndroidOpenSLES::UpdateRecordingDelay() { > + // TODO(leozwang): Add accurate delay estimat. > + recording_delay_ = 10; Race. @@ +1451,5 @@ > + // TODO(leozwang): Add accurate delay estimat. > + recording_delay_ = 10; > + const uint32_t noSamp10ms = mic_sampling_rate_ / 100; > + recording_delay_ += (N_REC_QUEUE_BUFFERS * noSamp10ms) / > + (mic_sampling_rate_ / 1000); This is completely wrong. The actual delay is based on the size of the internal AudioRecord buffer (which we do not know, but which is typically on the order of 1024 samples, i.e., huge), and _one_ queue buffer (i.e., 10 ms). The multiplication and division by mic_sampling_rate_ also seems extraneous. @@ +1455,5 @@ > + (mic_sampling_rate_ / 1000); > +} > + > +int32_t AudioDeviceAndroidOpenSLES::InitSampleRate() { > + if (sles_engine_ == NULL) { What is this being tested for? It isn't used. We _should_ be trying to verify that these rates are actually supported, but I don't think OpenSL ES gives us a way to do that (without actually trying to play back/record audio). It has the same rate restrictions as the underlying AudioRecord API, however. ::: media/webrtc/trunk/webrtc/modules/audio_device/linux/audio_device_opensles_linux.h @@ +200,5 @@ > + > + // Number of the buffers in playout queue > + static const uint16_t N_PLAY_QUEUE_BUFFERS = 8; > + // Number of buffers in recording queue > + // TODO(xian): Reduce the numbers of buffers to improve the latency. The number of buffers here should have nothing to do with latency. @@ +202,5 @@ > + static const uint16_t N_PLAY_QUEUE_BUFFERS = 8; > + // Number of buffers in recording queue > + // TODO(xian): Reduce the numbers of buffers to improve the latency. > + static const uint16_t N_REC_QUEUE_BUFFERS = 8; > + // Some values returned from getMinBufferSize N.B., we lose the ability to query getMinBufferSize() when using the OpenSL ES API, so we actually have no idea what the latency is. @@ +215,5 @@ > + crit_sect_.Enter(); > + }; > + void UnLock() { > + crit_sect_.Leave(); > + }; Dead code.

W.R.T. my other concern (the suitability of OpenSL ES given that it's just a wrapper around AudioRecord), the main issue is that we lose the ability to query the delay and the supported sampling rates. The API also imposes an additional delay while it completely fills whatever buffer we give it, but we can minimize that by using smaller buffers (or using a buffer that's exactly the size we want anyway). I don't think that's a serious issue. I did not see anything in the implementation that could improve on the delay provided by our usage of the native AudioRecord API, however.

Comment on attachment 777490 [details] [diff] [review] Add opensles backend support for gonk Review of attachment 777490 [details] [diff] [review]: ----------------------------------------------------------------- Also, with regards to using AudioRecord (which the earlier patch was doing), this comment is relevant (with normal caveats about stackoverflow): http://stackoverflow.com/a/3299048/105687 "The C++ libmedia library is not part of the public API. Some people use it, but this is highly discouraged because it might break on some devices and/or in a future Android release. I developed an audio recording app, and trust me, audio support is very inconsistent across devices, it's very tricky, so IMO using libmedia directly is a bad idea." ::: media/webrtc/trunk/webrtc/modules/audio_device/linux/audio_device_opensles_linux.cc @@ +1180,5 @@ > + " Playout is not initialized"); > + return 0; > + } > + > + if (!sles_player_itf_ && !sles_output_mixer_ && !sles_player_) { > if (!sles_player_itf_ && !sles_output_mixer_ && !sles_player_) { > This is just on crack. This was fixed post-3.30. We could pull those changes in. However, I should note we're not using OpenSLES for output. @@ +1208,5 @@ > + sles_output_mixer_ = NULL; > + } > + } > + > + CriticalSectionScoped lock(&crit_sect_); > Locking after we just released the lock This was fixed too. @@ +1335,5 @@ > + num_bytes); > + if (res != SL_RESULT_SUCCESS) { > + WEBRTC_OPENSL_TRACE(kTraceWarning, kTraceAudioDevice, id_, > + " player callback Enqueue failed, %d", res); > + play_warning_ = 1; These 4 races are all on the playout side @@ +1356,5 @@ > + audio_device->RecorderSimpleBufferQueueCallbackHandler(queue_itf); > +} > + > +bool AudioDeviceAndroidOpenSLES::RecThreadFuncImpl() { > + if (is_recording_) { This is only set and cleared under the crit_sect lock. It is also only set before this runs, and only cleared after Stop() is called, but if Stop() isn't synchronous, that wouldn't protect us here. Better to simply move it inside the lock already grabbed here and not worry about Stop(). (Note that ThreadPosix::Stop() appears to be virtually synchronous (waits up to 10 sec for the thread to exit), but not 100% guaranteed. @@ +1368,5 @@ > + > + { > + CriticalSectionScoped lock(&crit_sect_); > + if (rec_voe_audio_queue_.size() <= 0) { > + rec_timer_.Wait(1); Yeah... though since we don't use playout, it's merely silly/inefficient. We do need to return from this function frequently for ::Stop() to work (or have some other way to signal we want to stop - another event perhaps - but then we'd have to wait on two events here). Better is to wait a "reasonable" time for the queue to be non-empty, and call rec_time_.Set() when putting a buffer in the queue to wake this code up. @@ +1382,5 @@ > + > + UpdateRecordingDelay(); > + voe_audio_buffer_->SetRecordedBuffer(buf, num_samples); > + voe_audio_buffer_->SetVQEData(playout_delay_, recording_delay_, 0); > + voe_audio_buffer_->DeliverRecordedData(); From android opensles docs: "Application callback handlers generally run in a restricted context, and should be written to perform their work quickly and then return as soon as possible. Do not do complex operations within a callback handler. For example, within a buffer queue completion callback, you can enqueue another buffer, but do not create an audio player. " So that might explain why it uses a separate thread. (Though I suspect tim is right and the calling thread is the dedicated AudioRecord thread, but that's not guaranteed, in opensles or AudioRecord) i.e. this would suggest not encoding/echo-cancelling/etc on the callback thread (which is all done in DeliverRecordedData(), right now called off the dedicated thread in the webrtc opensles code) Note that for playback at low latency, it says this: "To avoid audible glitches, the buffer queue callback handler must execute within a small and predictable time window. This typically implies no blocking on mutexes, conditions, or I/O operations. Instead consider "try locks" and non-blocking algorithms. Avoid algorithms that execute in a non-deterministic amount of time, or are "bursty" in their computations." Existing example code for OpenSLES enqueues incoming data into circular queues and the like to process on another thread. @@ +1390,5 @@ > +} > + > +void AudioDeviceAndroidOpenSLES::RecorderSimpleBufferQueueCallbackHandler( > + SLAndroidSimpleBufferQueueItf queue_itf) { > + if (is_recording_) { Ditto. May not be a race depending on Stop(), but safer to just move down. @@ +1401,5 @@ > + { > + CriticalSectionScoped lock(&crit_sect_); > + audio = rec_queue_.front(); > + rec_queue_.pop(); > + rec_voe_audio_queue_.push(audio); here is where we should call rec_timer_.Set() @@ +1442,5 @@ > + > +void AudioDeviceAndroidOpenSLES::UpdatePlayoutDelay( > + uint32_t nSamplePlayed) { > + // TODO(leozwang): Add accurate delay estimat. > + playout_delay_ = (N_PLAY_QUEUE_BUFFERS - 0.5) * 10 + We don't use playout @@ +1451,5 @@ > + // TODO(leozwang): Add accurate delay estimat. > + recording_delay_ = 10; > + const uint32_t noSamp10ms = mic_sampling_rate_ / 100; > + recording_delay_ += (N_REC_QUEUE_BUFFERS * noSamp10ms) / > + (mic_sampling_rate_ / 1000); All this recording delay is a constant based on the buffer size. No need to update it on each pull anyways.

FYI, I filed bug 899159 (for input side generally) and bug 899204 (for the latency value)

Comment on attachment 777431 [details] [diff] [review] Gonk audio recording device (cleanup + refactored) Review of attachment 777431 [details] [diff] [review]: ----------------------------------------------------------------- overtaken by events.

(In reply to Michael Wu [:mwu] from comment #130) > Comment on attachment 777563 [details] [diff] [review] > Enable opensles webrtc backend on gonk > > Review of attachment 777563 [details] [diff] [review]: > ----------------------------------------------------------------- > > r=me fwiw. Just remove jni.h so we don't break the gonk-jb port. So this did land with a jni.h dep and broke the gonk-jb port :(

Remove jni.h per the review. This may need to be uplifted to Aurora https://hg.mozilla.org/integration/mozilla-inbound/rev/9ffbe336a432

These changes might be a cause of a build failed on b2g after merging to m-c, see bug 902443

dougc, The patch in Comment 144 should fix bug 902443

[Approval Request Comment] Bug caused by (feature/regressing bug #): 825112 User impact if declined: Breaks Nexus 4 Gonk builds (and all other JB gonk builds) Testing completed (on m-c, etc.): On M-C. Not sure how much it has been tested; :diego may be able to tell you Risk to taking this patch (and alternatives if risky): Nil. Compile issue. String or IDL/UUID changes made by this patch: none

jesup, Has the jni.h change landed on m-c? I don't see it. https://mxr.mozilla.org/mozilla-central/source/media/webrtc/trunk/webrtc/modules/audio_device/audio_device_opensles.h#14

you're right, it landed on inbound over a day ago, but hasn't uplifted to m-c yet

(In reply to Randell Jesup [:jesup] from comment #149) > Created attachment 786919 [details] [diff] [review] > Patch to fix gonk-JB > > [Approval Request Comment] > Bug caused by (feature/regressing bug #): 825112 > > User impact if declined: Breaks Nexus 4 Gonk builds (and all other JB gonk > builds) > > Testing completed (on m-c, etc.): On M-C. Not sure how much it has been > tested; :diego may be able to tell you > > Risk to taking this patch (and alternatives if risky): Nil. Compile issue. > > String or IDL/UUID changes made by this patch: none We're not shipping a Gecko 25 release (1.2=Gecko 26). Any other reason to take this?