1059573 - Add support to using DSP-based echo canceller and noise suppressor

Status: RESOLVED FIXED

(Core :: WebRTC: Audio/Video, defect)

Description

[Jay]

[Blocking Requested - why for this release]:
With DSP-based EC and NS, we can improve the webRTC CPU utilization around 3%. 8x10 devices support DSP-based EC, NS. To enable it, gecko webRTC stack needs to construct AudioEffect and enable the echo canceller through AudioEffect interface.

Here is information about the AudiEffect class.
http://developer.android.com/reference/android/media/audiofx/AudioEffect.html#EFFECT_TYPE_AEC
http://developer.android.com/reference/android/media/audiofx/AudioEffect.html#EFFECT_TYPE_NS 

You can refert to AudioGroup::DeviceThread::threadLoop() (line 809-858) in 
https://android.googlesource.com/platform/frameworks/opt/net/voip/+/master/src/jni/rtp/AudioGroup.cpp about how to setup the path. As shown in the above exmaple, the source of recording path also needs to set to AUDIO_SOURCE_VOICE_COMMUNICATION instead of AUDIO_SOURCE_MIC.

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

The working group has considered using platform-based AEC since the start, and we've also considered it for overall quality improvement (closer to the hardware the better for AEC).

However, the restrictions associated with platform AEC/etc can be problematic.  Some are limited to 8KHz sampling rate.  Others have limited AEC tail lengths, or poor implementations, or are software implementations that aren't as good at the WebRTC.org implementation.

The generic API given has no knobs for aggressiveness of AEC, and also from the code linked to in AudioGroup, no way to tell if the AEC provided is a DSP/HW AEC, or a software AEC (which we may not want to use in preference to the webrtc.org one).  Some applications may not want AEC/NS.

All that said: it may be a reasonable option for specific hardware/firmware, controlled by pref/etc.  THe hardest part will be getting the right audio channels to be used to comply, I suspect, and that may be considerable work.

Comment 2

[bhavana bajaj [:bajaj]]

This seems like a considerable amount of work at this point in the cycle, may be something we can target for 2.1 as an improvement here ?

Comment 3

[Jay]

Considering high CPU utilization with Loop mobile app, we should consider all the options to reduce the overall CPU usage. However, I am not the best person to comment on the risk and complexity to support this as I am not familiar with gecko/webrtc audio stack. 
I like Randell's suggestion for using pref to enable DSP-basd EC/NS. Can we get some more analysis on scope of work and the risk?

Comment 4

[Paul Adenot (:padenot)]

What would be needed, here:
(1) Have a way to query the platform for capabilities, or hardcode things for some devices (this is a OpenSLEL API)
(2) Have a way to get the audio session id from OpenSLES (the audio input we use). This is not currently possible in the NDK. We need a way to get this audio session id.
(3) Have a way to retrieve the latency from the audio output stream. We have that already, but we need to make sure the number is accurate
(4) Insert the Android's echo canceler effect using the recorder audio session id, and the latency figures

(1), (3), (4) are easy.

(2) is hard, and would require AOSP changes.

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

Given this, barring a surprise answer from Jay/etc, this should not block 2.0, and may be a stretch for 2.1 (given #2).

Comment 6

[Michael Vines [:m1] [:evilmachines]]

Let's consider the short-term tactical angle for v2.0 here please, where we know that we have CPU load issues and this helps reduce it.  This bug isn't  about fixing this in a generic way for the universe (that's a follow-up bug), it's about what can be done in v2.0 for the well-known hardware.

Comment 7

[Randell Jesup [:jesup] (needinfo me)]

Without access to the relevant APIs (especially #2), it would be hard to even attempt this or prototype it to see if it works in practice for us.  I know google doesn't use the built-in AEC functionality for webrtc in Android Chrome, though perhaps it's on their "someday" list.

Comment 8

[Michael Vines [:m1] [:evilmachines]]

Jay has prototyped this over here already, might be good to sync up on a call with him.

Comment 9

[Randell Jesup [:jesup] (needinfo me)]

Sure - if this can be done without intrusive or problematic mods, that's great! email, IRC, call - whatever makes sense.

Are there any specs available on the AEC and NS DSP implementations?  Have these been used in real applications at 16k input, 48K or 44.1K output?  (I think the "natural" rate on Frame is 44.1, but it's been a while since I checked.)

Comment 10

[Jay]

I don't think specs is available to the public. The DSP-based EC solution has been used for other Android-based VoIP application. It supports 16K input and 48K/44.1K output.
As we discussed over IRC, I will email the instruction to enable the DSP based-EC/NS and let you know if there is better way to resolve #2.

Comment 11

[Jay]

Attachment: 0001-Use-android-configuration-interface-to-retrive-the-s.patch

Randell,

Did you have chance look at the proposed AOSP change to retrieve the session Id from Opensl interface? I am attaching the patch here, again. Can we proceed with this?

Comment 12

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Jay from comment #11)
> Created attachment 8482956 [details] [diff] [review]
> 0001-Use-android-configuration-interface-to-retrive-the-s.patch
> 
> Randell,
> 
> Did you have chance look at the proposed AOSP change to retrieve the session
> Id from Opensl interface? I am attaching the patch here, again. Can we
> proceed with this?

Thanks!

We're looking at the code mods now to use it.

Our side seems pretty simple, especially if we make this a vendor-pref-on item.
(Hardest part might be plumbing the pref down the the code that turns it on - we'll start with it hard-coded on)

Comment 13

[bhavana bajaj [:bajaj]]

Spoke offline with maire and given :jesup's feedback above and comment #6, blocking 2.0 on this. If the risk ends up being too hig for the gain we may achieve based on the final patch lets discuss this at the time of uplift once again

Comment 14

[Randell Jesup [:jesup] (needinfo me)]

Attachment: WIP ugly patch for HW AEC

Note: compiles bug does not link due to issues finding aec_->initCheck(). Several ugly hacks taken to get it to build that can be fixed.

Comment 15

[Jay]

Comment on attachment 8483792 [details] [diff] [review]
WIP ugly patch for HW AEC

Review of attachment 8483792 [details] [diff] [review]:
-----------------------------------------------------------------

One thing also needs to be added is that the audio source must be set to AUDIO_SOURCE_VOICE_COMMUNICATION. Here is some information about different input sources: http://developer.android.com/reference/android/media/MediaRecorder.AudioSource.html#VOICE_COMMUNICATION.

This can be set through opensl android configuration set interface.

Here is the key pair and header location.
#define SL_ANDROID_KEY_RECORDING_PRESET ((const SLchar*) "androidRecordingPreset")
#define SL_ANDROID_RECORDING_PRESET_VOICE_COMMUNICATION ((SLuint32) 0x00000004)

root/frameworks/wilhelm/include/SLES/OpenSLES_AndroidConfiguration.h

There is gecko version of it, but it doesn't have the value 
root/gecko/media/libcubeb/src/android/sles_definitions.h

::: media/webrtc/trunk/webrtc/modules/audio_device/android/opensles_input.cc
@@ +27,5 @@
> +// HACKS
> +//#include <audio_effects/effect_aec.h>
> +static const effect_uuid_t FX_IID_AEC_ =
> +    { 0x7b491460, 0x8d4d, 0x11e0, 0xbd61, { 0x00, 0x02, 0xa5, 0xd5, 0xc5, 0x1b } };
> +const effect_uuid_t * const FX_IID_AEC = &FX_IID_AEC_;

Can we add support for NS too?
http://developer.android.com/reference/android/media/audiofx/AudioEffect.html#EFFECT_TYPE_NS

@@ +465,5 @@
> +                                                                 AUDIO_CHANNEL_IN_ALL /*mChannelMask see [1] */,
> +                                                                 session_id_);
> +        if (input) {
> +          WEBRTC_TRACE(kTraceError, kTraceAudioDevice, id_, "OpenSL input: %p", input);
> +          aec_ = new android::AudioEffect(FX_IID_AEC, NULL, 0, 0, 0, session_id_, input);

There is a logic in AudioFlinger::createEffect() to search the matching io handle based on session ID when io handle is 0. You can try this out. Then, you don't need android::AudioSystem::getInput(), etc.

Comment 16

[Randell Jesup [:jesup] (needinfo me)]

Gian-Carlo - can you take a shot at cleaning this up this morning?  It's a 2.0 blocker.  

Once we prove it's working we'll decide how to enable/disable this for the QC builds; the "right" way would be a pref value in the media.getusermedia.aec var that means "use platform AEC", but plumbing that down to the opensl code would be a pain (not impossible though, and the aec pref is already handed to (different) webrtc.org code).  Given the short timeframe and need to avoid risk I'm open to other solutions.

Thanks

Comment 17

[Randell Jesup [:jesup] (needinfo me)]

-> gcp (IRC)

Comment 18

[Gian-Carlo Pascutto [:gcp]]

Attachment: WIP ugly patch for HW AEC

Updated WIP with cleanups. It now links. I didn't incorporate
all the feedback from jay yet.

Comment 19

[Gian-Carlo Pascutto [:gcp]]

I did a whole bunch of fixes, but the code is crashing in the OpenSL part that was patched:

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 17280.17487]
android::AudioRecord::getSessionId (this=0x0) at frameworks/av/media/libmedia/AudioRecord.cpp:706
706     }
(gdb) bt
#0  android::AudioRecord::getSessionId (this=0x0) at frameworks/av/media/libmedia/AudioRecord.cpp:706
#1  0xb34a085c in android_audioRecorder_getConfig (ar=0xb2ec4400, configKey=0xb625b260 "androidRecordingSessionId", 
    pValueSize=0xb25ffca4, pConfigValue=0xb25ffca0) at frameworks/wilhelm/src/android/AudioRecorder_to_android.cpp:366
#2  0xb34ab0ca in IAndroidConfiguration_GetConfiguration (self=0xb2ec46cc, 
    configKey=0xb625b260 "androidRecordingSessionId", pValueSize=0xb25ffca4, pConfigValue=0xb25ffca0)
    at frameworks/wilhelm/src/itf/IAndroidConfiguration.c:82
#3  0xb597f260 in webrtc::OpenSlesInput::SetupAECAndNS (this=0xb36aaafc)
    at ../../../../../../../media/webrtc/trunk/webrtc/modules/audio_device/opensl/../android/opensles_input.cc:416
#4  0xb597f430 in webrtc::OpenSlesInput::CreateAudioRecorder (this=0xb36aaafc)
    at ../../../../../../../media/webrtc/trunk/webrtc/modules/audio_device/opensl/../android/opensles_input.cc:492
#5  0xb597f692 in webrtc::OpenSlesInput::StartRecording (this=0xb36aaafc)
    at ../../../../../../../media/webrtc/trunk/webrtc/modules/audio_device/opensl/../android/opensles_input.cc:202
#6  0xb597fadc in webrtc::AudioDeviceModuleImpl::StartRecording (this=<optimized out>)
    at /home/morbo/hg/mozilla-central/media/webrtc/trunk/webrtc/modules/audio_device/audio_device_impl.cc:1679
#7  0xb598c2ec in webrtc::VoEBaseImpl::StartSend (this=0xb36170cc)
    at /home/morbo/hg/mozilla-central/media/webrtc/trunk/webrtc/voice_engine/voe_base_impl.cc:1050
#8  0xb598c388 in webrtc::VoEBaseImpl::StartSend (this=0xb36170cc, channel=<optimized out>)
    at /home/morbo/hg/mozilla-central/media/webrtc/trunk/webrtc/voice_engine/voe_base_impl.cc:751
#9  0xb578576e in mozilla::MediaEngineWebRTCAudioSource::Start (this=0xb2ea6080, aStream=0x0, aID=2)
    at ../../../../content/media/webrtc/MediaEngineWebRTCAudio.cpp:348
#10 0xb5562eb0 in mozilla::MediaOperationRunnable::Run (this=0xb36e9540) at ../../../dom/media/MediaManager.h:405
#11 0xb4e77dca in ProcessNextEvent (aResult=0xb25ffe57, aMayWait=<optimized out>, this=0xb33eea90)
    at ../../../xpcom/threads/nsThread.cpp:823
#12 nsThread::ProcessNextEvent (this=0xb33eea90, aMayWait=<optimized out>, aResult=0xb25ffe57)
    at ../../../xpcom/threads/nsThread.cpp:717
#13 0xb4e86096 in NS_ProcessNextEvent (aThread=<optimized out>, aMayWait=<optimized out>)
    at /home/morbo/hg/mozilla-central/xpcom/glue/nsThreadUtils.cpp:265
#14 0xb4fc1ae8 in mozilla::ipc::MessagePumpForNonMainThreads::Run (this=0xb2e73a90, aDelegate=0xb33d2de0)
    at ../../../ipc/glue/MessagePump.cpp:326
#15 0xb4fb67de in MessageLoop::RunInternal (this=<optimized out>) at ../../../ipc/chromium/src/base/message_loop.cc:229
#16 0xb4fb6890 in RunHandler (this=0xb33d2de0) at ../../../ipc/chromium/src/base/message_loop.cc:222
#17 MessageLoop::Run (this=0xb33d2de0) at ../../../ipc/chromium/src/base/message_loop.cc:196
#18 0xb4e7ae28 in nsThread::ThreadFunc (aArg=0xb33eea90) at ../../../xpcom/threads/nsThread.cpp:350
#19 0xb69a589a in _pt_root (arg=0xb2ea7680) at ../../../../../nsprpub/pr/src/pthreads/ptthread.c:212
#20 0xb6e90ba4 in __thread_entry (func=0xb69a5801 <_pt_root>, arg=0xb2ea7680, tls=0xb25fff00)
    at bionic/libc/bionic/pthread_create.cpp:92
#21 0xb6e90d20 in pthread_create (thread_out=0xbecdd2c4, attr=<optimized out>, start_routine=0x78, arg=0xb2ea7680)
    at bionic/libc/bionic/pthread_create.cpp:201
#22 0x00000000 in ?? ()
(gdb) up
#1  0xb34a085c in android_audioRecorder_getConfig (ar=0xb2ec4400, configKey=0xb625b260 "androidRecordingSessionId", 
    pValueSize=0xb25ffca4, pConfigValue=0xb25ffca0) at frameworks/wilhelm/src/android/AudioRecorder_to_android.cpp:366
366                 *(SLuint32*)pConfigValue = ar->mAudioRecord->getSessionId();
(gdb) print ar
$1 = (CAudioRecorder *) 0xb2ec4400
(gdb) print ar->mAudioRecord
$2 = (android::AudioRecord *) 0x0
(gdb)

Comment 20

[Gian-Carlo Pascutto [:gcp]]

Attachment: WIP ugly patch for HW AEC

Current WIP

Comment 21

[Gian-Carlo Pascutto [:gcp]]

Okay, easily fixed by moving the Setup after the Realize call. Now:

ERROR     ; (19:41:32:877 |    4) AUDIO DEVICE:    1    99;     18414; OpenSL GetInterface: 0
ERROR     ; (19:41:32:877 |    0) AUDIO DEVICE:    1    99;     18414; OpenSL Get sessionId res: 0
ERROR     ; (19:41:32:877 |    0) AUDIO DEVICE:    1    99;     18414; OpenSL sessionId: 63
ERROR     ; (19:41:32:882 |    5) AUDIO DEVICE:    1    99;     18414; OpenSL aec: 0xb6a91790
ERROR     ; (19:41:32:882 |    0) AUDIO DEVICE:    1    99;     18414; OpenSL aec disabled
ERROR     ; (19:41:32:883 |    1) AUDIO DEVICE:    1    99;     18414; OpenSL ns: 0xb6a91880
ERROR     ; (19:41:32:883 |    0) AUDIO DEVICE:    1    99;     18414; OpenSL ns disabled

I guess setting AUDIO_SOURCE_VOICE_COMMUNICATION is next.

Comment 22

[Gian-Carlo Pascutto [:gcp]]

Attachment: WIP ugly patch for HW AEC

Updated WIP. Fails with:

ERROR     ; (20: 5: 6:587 |    0) AUDIO DEVICE:    1    99;     21286; OpenSL GetInterface: 0
ERROR     ; (20: 5: 6:587 |    0) AUDIO DEVICE:    1    99;     21286; OpenSL Set Voice mode res: 0
ERROR     ; (20: 5: 6:597 |   10) AUDIO DEVICE:    1    99;     21286; OpenSL GetInterface: 0
ERROR     ; (20: 5: 6:597 |    0) AUDIO DEVICE:    1    99;     21286; OpenSL Get sessionId res: 0
ERROR     ; (20: 5: 6:597 |    0) AUDIO DEVICE:    1    99;     21286; OpenSL sessionId: 84
ERROR     ; (20: 5: 6:597 |    0) AUDIO DEVICE:    1    99;     21286; OpenSL aec: 0xb17386c0
ERROR     ; (20: 5: 6:598 |    1) AUDIO DEVICE:    1    99;     21286; OpenSL aec disabled
ERROR     ; (20: 5: 6:598 |    0) AUDIO DEVICE:    1    99;     21286; OpenSL ns: 0xb17386c0
ERROR     ; (20: 5: 6:598 |    0) AUDIO DEVICE:    1    99;     21286; OpenSL ns disabled

Comment 23

[Gian-Carlo Pascutto [:gcp]]

I implemented this: https://android.googlesource.com/platform/frameworks/opt/net/voip/+/master/src/jni/rtp/AudioGroup.cpp#761

It reports that the *platform has no AEC*.

The error message from the AEC and NS initCheck() is -22, which doesn't seem to be documented. Please advice how to proceed. I will upload the WIP code.

ERROR     ; ( 9:34:36:998 |    0) AUDIO DEVICE:    1    99;     23922; OpenSL GetInterface: 0
ERROR     ; ( 9:34:36:998 |    0) AUDIO DEVICE:    1    99;     23922; OpenSL Set Voice mode res: 0
ERROR     ; ( 9:34:37:  3 |    0) AUDIO DEVICE:    1    99;     23922; Platform has AEC: 0
ERROR     ; ( 9:34:37:  3 |    0) AUDIO DEVICE:    1    99;     23922; OpenSL GetInterface: 0
ERROR     ; ( 9:34:37:  3 |    0) AUDIO DEVICE:    1    99;     23922; OpenSL Get sessionId res: 0
ERROR     ; ( 9:34:37:  3 |    0) AUDIO DEVICE:    1    99;     23922; OpenSL sessionId: 94
ERROR     ; ( 9:34:37:  4 |    1) AUDIO DEVICE:    1    99;     23922; OpenSL aec: 0xb6a911f0
ERROR     ; ( 9:34:37:  4 |    0) AUDIO DEVICE:    1    99;     23922; OpenSL aec disabled: -22
ERROR     ; ( 9:34:37:  4 |    0) AUDIO DEVICE:    1    99;     23922; OpenSL ns: 0xb6a911f0
ERROR     ; ( 9:34:37:  4 |    0) AUDIO DEVICE:    1    99;     23922; OpenSL ns disabled: -22

Comment 24

[Gian-Carlo Pascutto [:gcp]]

Attachment: WIP ugly patch for HW AEC

Updated WIP

Comment 25

[Gian-Carlo Pascutto [:gcp]]

This is what the error -22 means, confirms the AEC & NS are not found. I've tried this on both KK and JB base builds. There are 10 effects reported by the AndroidEffects code, but AEC and NS are not among them.

W/AudioFlinger(  201): createEffect() effect not found
E/AudioEffect( 1451): set(): AudioFlinger could not create effect, status: -22
W/AudioFlinger(  201): createEffect() effect not found
E/AudioEffect( 1451): set(): AudioFlinger could not create effect, status: -22

Comment 26

[Jay]

(In reply to Gian-Carlo Pascutto [:gcp] from comment #25)
> This is what the error -22 means, confirms the AEC & NS are not found. I've
> tried this on both KK and JB base builds. There are 10 effects reported by
> the AndroidEffects code, but AEC and NS are not among them.
> 
> W/AudioFlinger(  201): createEffect() effect not found
> E/AudioEffect( 1451): set(): AudioFlinger could not create effect, status:
> -22
> W/AudioFlinger(  201): createEffect() effect not found
> E/AudioEffect( 1451): set(): AudioFlinger could not create effect, status:
> -22
Let me check and I will get back to you on this.

Comment 27

[Jay]

I don't see the "effect not found error" here.

Please, check the following audio effect configuration files on the target. One of them should define the aec and ns library.

/system/etc/audio_effects.conf
/system/vendor/etc/audio_effects.conf

If it's not there. Please, add them in the conf file. You can refer to 
https://www.codeaurora.org/cgit/quic/la/platform/vendor/qcom/msm8610/tree/audio_effects.conf?h=b2g_kk_3.5#n209

Comment 28

[Randell Jesup [:jesup] (needinfo me)]

Commented out in audio_effects.conf:
#  aec {
#    library pre_processing
#    uuid bb392ec0-8d4d-11e0-a896-0002a5d5c51b
#  }
#  ns {
#    library pre_processing
#    uuid c06c8400-8e06-11e0-9cb6-0002a5d5c51b
#  }

Not commented out in the vendor version: (I also note the UUIDs are different)

  aec {
    library audio_pre_processing
    uuid 0f8d0d2a-59e5-45fe-b6e4-248c8a799109
  }
  ns {
    library audio_pre_processing
    uuid 1d97bb0b-9e2f-4403-9ae3-58c2554306f8
  }

Comment 29

[Randell Jesup [:jesup] (needinfo me)]

Attachment: audio_effects.conf

Comment 30

[Randell Jesup [:jesup] (needinfo me)]

Attachment: vendor.conf

These are from the v180 drop

Comment 31

[Jay]

(In reply to Randell Jesup [:jesup] from comment #30)
> Created attachment 8486750 [details]
> vendor.conf
> 
> These are from the v180 drop
The aec/ns uuid looks correct.
There are two configuration. One is generic configuration and one is vendor specific one. The vendor specific one should be stored under /system/vendor/etc/audio_effects.conf on target device. can you confirm this?

Comment 32

[Randell Jesup [:jesup] (needinfo me)]

vendor.conf is /system/vendor/etc/audio_effects.conf

Comment 33

[Jay]

(In reply to Randell Jesup [:jesup] from comment #32)
> vendor.conf is /system/vendor/etc/audio_effects.conf

Sorry, actually, it should be under /vendor/etc/audio_effects.conf instead of /system/vendor/etc/audio_effects.conf.
The path is defined in system/media/audio_effects/include/audio_effects/audio_effects_conf.h
#define AUDIO_EFFECT_VENDOR_CONFIG_FILE "/vendor/etc/audio_effects.conf"

If the configuration file is in the correct directory than I don't see any reason why it can't find the effect.
In fact, I tried the attachment 8486257 [details] [diff] [review] on my setup and I saw the effect is triggered to audio HAL properly. Can you collect the log from media/libeffects/factory/EffectsFactory.c and services/audioflinger/AudioFlinger.cpp by adding "#define LOG_NDEBUG 0"?

Comment 34

[Randell Jesup [:jesup] (needinfo me)]

/vendor/etc/audio_effects.conf and /system/vendor/etc/audio_effects.conf are identical
(this is with KK-based build v180)

Comment 35

[Gian-Carlo Pascutto [:gcp]]

My device (v166 KK flash, I don't see a v180 anywhere) doesn't have a /vendor/etc nor does it have /system/vendor/etc. I'm handing this bug off since the problem isn't in Gecko but in the system/vendor images.

Comment 36

[Gian-Carlo Pascutto [:gcp]]

(In reply to Randell Jesup [:jesup] from comment #34)
> /vendor/etc/audio_effects.conf and /system/vendor/etc/audio_effects.conf are
> identical
> (this is with KK-based build v180)

Are you sure? With v180, in /system/etc/audio_effects.conf I see only the commented out ones. In /vendor/etc/audio_effects.conf I see both the commented out ones, but also non-commented out ones, right above the commented-out ones.

Comment 37

[Gian-Carlo Pascutto [:gcp]]

This is the result with a current v180 image:

ERROR     ; (19:43:40:187 |    0) AUDIO DEVICE:    1    99;      2307; OpenSL GetInterface: 0
ERROR     ; (19:43:40:187 |    0) AUDIO DEVICE:    1    99;      2307; OpenSL Set Voice mode res: 0
ERROR     ; (19:43:40:191 |    4) AUDIO DEVICE:    1    99;      2307; Platform has 11 effects
ERROR     ; (19:43:40:193 |    2) AUDIO DEVICE:    1    99;      2307; Platform has AEC: 0
ERROR     ; (19:43:40:193 |    0) AUDIO DEVICE:    1    99;      2307; OpenSL GetInterface: 0
ERROR     ; (19:43:40:193 |    0) AUDIO DEVICE:    1    99;      2307; OpenSL Get sessionId res: 0
ERROR     ; (19:43:40:193 |    0) AUDIO DEVICE:    1    99;      2307; OpenSL sessionId: 11
ERROR     ; (19:43:40:193 |    0) AUDIO DEVICE:    1    99;      2307; OpenSL aec: 0xb2fd86c0
ERROR     ; (19:43:40:194 |    1) AUDIO DEVICE:    1    99;      2307; OpenSL aec disabled: -22
ERROR     ; (19:43:40:195 |    1) AUDIO DEVICE:    1    99;      2307; OpenSL ns: 0xb2fd86c0
ERROR     ; (19:43:40:195 |    0) AUDIO DEVICE:    1    99;      2307; OpenSL ns disabled: -22

Comment 38

[Jay]

(In reply to Gian-Carlo Pascutto [:gcp] from comment #37)
> This is the result with a current v180 image:
> 
> ERROR     ; (19:43:40:187 |    0) AUDIO DEVICE:    1    99;      2307;
> OpenSL GetInterface: 0
> ERROR     ; (19:43:40:187 |    0) AUDIO DEVICE:    1    99;      2307;
> OpenSL Set Voice mode res: 0
> ERROR     ; (19:43:40:191 |    4) AUDIO DEVICE:    1    99;      2307;
> Platform has 11 effects
> ERROR     ; (19:43:40:193 |    2) AUDIO DEVICE:    1    99;      2307;
> Platform has AEC: 0
> ERROR     ; (19:43:40:193 |    0) AUDIO DEVICE:    1    99;      2307;
> OpenSL GetInterface: 0
> ERROR     ; (19:43:40:193 |    0) AUDIO DEVICE:    1    99;      2307;
> OpenSL Get sessionId res: 0
> ERROR     ; (19:43:40:193 |    0) AUDIO DEVICE:    1    99;      2307;
> OpenSL sessionId: 11
> ERROR     ; (19:43:40:193 |    0) AUDIO DEVICE:    1    99;      2307;
> OpenSL aec: 0xb2fd86c0
> ERROR     ; (19:43:40:194 |    1) AUDIO DEVICE:    1    99;      2307;
> OpenSL aec disabled: -22
> ERROR     ; (19:43:40:195 |    1) AUDIO DEVICE:    1    99;      2307;
> OpenSL ns: 0xb2fd86c0
> ERROR     ; (19:43:40:195 |    0) AUDIO DEVICE:    1    99;      2307;
> OpenSL ns disabled: -22
Can you enable the AudioFlinger and EffectFactory logs as I mentioned in comment #33?

Comment 39

[Gian-Carlo Pascutto [:gcp]]

Attachment: audio logs.txt

Comment 40

[Gian-Carlo Pascutto [:gcp]]

I think I understand the problem here:

Flash v180 image: /vendor/etc/audio_effects.conf is present
Flash only gecko on v180: phone is unusable, file is present
Flash only gaia on v180, phone refuses to boot, file is present
Flash full build on v180: file is gone

I'll try with readding the file manually, but this looks like an issue with the B2G build that must be fixed.

Comment 41

[Gian-Carlo Pascutto [:gcp]]

I manually added the file and rebooted:

ERROR     ; (10: 3:45:630 |    0) AUDIO DEVICE:    1    99;      1433; OpenSL GetInterface: 0
ERROR     ; (10: 3:45:630 |    0) AUDIO DEVICE:    1    99;      1433; OpenSL Set Voice mode res: 0
ERROR     ; (10: 3:45:636 |    6) AUDIO DEVICE:    1    99;      1433; Platform has 12 effects
ERROR     ; (10: 3:45:636 |    0) AUDIO DEVICE:    1    99;      1433; Platform has AEC: 1
ERROR     ; (10: 3:45:637 |    1) AUDIO DEVICE:    1    99;      1433; OpenSL GetInterface: 0
ERROR     ; (10: 3:45:637 |    0) AUDIO DEVICE:    1    99;      1433; OpenSL Get sessionId res: 0
ERROR     ; (10: 3:45:637 |    0) AUDIO DEVICE:    1    99;      1433; OpenSL sessionId: 12
ERROR     ; (10: 3:45:639 |    2) AUDIO DEVICE:    1    99;      1433; OpenSL aec: 0xb6b8c110
ERROR     ; (10: 3:45:639 |    0) AUDIO DEVICE:    1    99;      1433; OpenSL aec enabled
ERROR     ; (10: 3:45:641 |    2) AUDIO DEVICE:    1    99;      1433; OpenSL ns: 0xb6b8d1f0
ERROR     ; (10: 3:45:642 |    1) AUDIO DEVICE:    1    99;      1433; OpenSL ns enabled


W/AudioRecord( 1371): AUDIO_INPUT_FLAG_FAST denied by client
V/AudioFlinger(  199): openInput() openInputStream returned input 0xb80acea8, SamplingRate 44100, Format 1, Channels 10, status 0
V/AudioFlinger(  199): openInput() created record thread: ID 13 thread 0xb80b2498
W/AudioFlinger(  199): Thread AudioIn_D cannot connect to the power manager service
V/AudioFlinger(  199): acquiring 12 from 1371
V/AudioFlinger(  199):  added new entry for 12
V/EffectsFactory(  199): EffectQueryNumberEffects(): 12
V/EffectsFactory(  199): EffectQueryEffect() desc:- TYPE: 58b4b260-8e06-11e0-aa8e-0002a5d5c51b
V/EffectsFactory(  199): - apiVersion: 00020000
V/EffectsFactory(  199): - flags: 00000203
V/EffectsFactory(  199): - name: Noise Suppression
V/EffectsFactory(  199): - implementor: Qualcomm Fluence
V/EffectsFactory(  199): EffectQueryEffect() desc:- TYPE: 7b491460-8d4d-11e0-bd61-0002a5d5c51b
V/EffectsFactory(  199): - apiVersion: 00020000
V/EffectsFactory(  199): - flags: 00000203
V/EffectsFactory(  199): - name: Acoustic Echo Canceler
V/EffectsFactory(  199): - implementor: Qualcomm Fluence
V/AudioFlinger(  199): createEffect pid 1371, effectClient 0xb80b6118, priority 0, sessionId 12, io 0
V/EffectsFactory(  199): EffectQueryNumberEffects(): 12
V/EffectsFactory(  199): EffectQueryEffect() desc:- TYPE: 58b4b260-8e06-11e0-aa8e-0002a5d5c51b
V/EffectsFactory(  199): - apiVersion: 00020000
V/EffectsFactory(  199): - flags: 00000203
V/EffectsFactory(  199): - name: Noise Suppression
V/EffectsFactory(  199): - implementor: Qualcomm Fluence
V/EffectsFactory(  199): EffectQueryEffect() desc:- TYPE: 7b491460-8d4d-11e0-bd61-0002a5d5c51b
V/EffectsFactory(  199): - apiVersion: 00020000
V/EffectsFactory(  199): - flags: 00000203
V/EffectsFactory(  199): - name: Acoustic Echo Canceler
V/EffectsFactory(  199): - implementor: Qualcomm Fluence
V/AudioFlinger(  199): createEffect() got io 13 for effect Acoustic Echo Canceler
V/EffectsFactory(  199): EffectCreate() UUID: 0F8D0D2A-59E5-45FE-B6E4-248C8A799109
V/EffectsFactory(  199): findEffect() found effect: Acoustic Echo Canceler in lib audio_pre_processing
V/EffectsFactory(  199): EffectCreate() gInterface
V/EffectsFactory(  199): EffectCreate() created entry 0xb80b6078 with sub itfe 0xb80b4998 in library audio_pre_processing
V/AudioFlinger(  199): createEffect pid 1371, effectClient 0xb80b4ad0, priority 0, sessionId 12, io 0
V/EffectsFactory(  199): EffectQueryNumberEffects(): 12
V/EffectsFactory(  199): EffectQueryEffect() desc:- TYPE: 58b4b260-8e06-11e0-aa8e-0002a5d5c51b
V/EffectsFactory(  199): - apiVersion: 00020000
V/EffectsFactory(  199): - flags: 00000203
V/EffectsFactory(  199): - name: Noise Suppression
V/EffectsFactory(  199): - implementor: Qualcomm Fluence
V/AudioFlinger(  199): createEffect() got io 13 for effect Noise Suppression
V/EffectsFactory(  199): EffectCreate() UUID: 1D97BB0B-9E2F-4403-9AE3-58C2554306F8
V/EffectsFactory(  199): findEffect() found effect: Noise Suppression in lib audio_pre_processing
V/EffectsFactory(  199): EffectCreate() gInterface
V/EffectsFactory(  199): EffectCreate() created entry 0xb80b34b0 with sub itfe 0xb80b49a8 in library audio_pre_processing
V/AudioFlinger(  199): setParameters(): io 13, keyvalue input_source=7;routing=-2147483644, calling pid 199
W/AudioFlinger(  199): Thread AudioIn_D cannot connect to the power manager service
W/AudioFlinger(  199): Thread AudioIn_D cannot connect to the power manager service
E/AudioFlinger(  199): no wake lock to update!
E/msm8974_platform(  199): set_echo_reference: Could not get ctl for mixer cmd - EC_REF_RX
D/audio_hw_primary(  199): select_devices: out_snd_device(0: ) in_snd_device(38: speaker-mic)
D/hardware_info(  199): hw_info_append_hw_type : device_name = speaker-mic
E/ACDB-LOADER(  199): ACDB -> Not correctly initialized!
D/hardware_info(  199): hw_info_append_hw_type : device_name = speaker-mic
V/AudioFlinger(  199): setParameters(): io 13, keyvalue routing=0, calling pid 199
W/AudioFlinger(  199): Thread AudioIn_D cannot connect to the power manager service
V/AudioFlinger(  199): releasing 12 from 1371
V/AudioFlinger(  199):  decremented refcount to 0
V/AudioFlinger(  199): purging stale effects
V/AudioFlinger(  199): closeInput() 13
W/AudioFlinger(  199): Thread AudioIn_D cannot connect to the power manager service

I will do some testing to see how well it works.

Comment 42

[Gian-Carlo Pascutto [:gcp]]

I did a test with the above setup (file added back) and the posted WIP (which seems to find and insert the AEC/NS, at least, but no idea if they actually work).

Audio delay on the call was pretty big, like >1s. I used Loop/Hello from the Flame to a laptop with Nightly. There was a very strong echo.

I retried this with the software AEC enabled again, and the HW AEC disabled, in the same conditions. There was barely none or no echo.

I conclude the hardware AEC is not working well, and until this is fixed we should keep software on.

I noticed "V/AudioFlinger(  199): openInput() openInputStream returned input 0xb80acea8, SamplingRate 44100, Format 1, Channels 10, status 0" versus Jay's statement "The DSP-based EC solution has been used for other Android-based VoIP application. It supports 16K input and 48K/44.1K output."

In any case, further testing should be possible with the WIP in thus bug, and the opensl patch. The WIP is reasonably clean and could be reviewed/landed quickly if needed now.

Comment 43

[Gian-Carlo Pascutto [:gcp]]

I tested with 16kHz as OpenSL input rate:
V/AudioFlinger(  199): openInput() openInputStream returned input 0xb80cb390, SamplingRate 16000, Format 1, Channels 10, status 0

Result is the same, hardware AEC is ineffective.

I noticed this:
E/msm8974_platform(  199): set_echo_reference: Could not get ctl for mixer cmd - EC_REF_RX

That error looks related to AEC.

Comment 44

[Gian-Carlo Pascutto [:gcp]]

If we look in the Android source, we can see that this function is indeed setting stuff up for the AEC:
https://android.googlesource.com/platform/hardware/qcom/audio/+/master/hal/msm8974/platform.c#691

Comment 45

[Jay]

(In reply to Gian-Carlo Pascutto [:gcp] from comment #42)
> I did a test with the above setup (file added back) and the posted WIP
> (which seems to find and insert the AEC/NS, at least, but no idea if they
> actually work).
> 
> Audio delay on the call was pretty big, like >1s. I used Loop/Hello from the
> Flame to a laptop with Nightly. There was a very strong echo.
> 
> I retried this with the software AEC enabled again, and the HW AEC disabled,
> in the same conditions. There was barely none or no echo.
> 
> I conclude the hardware AEC is not working well, and until this is fixed we
> should keep software on.
> 
> I noticed "V/AudioFlinger(  199): openInput() openInputStream returned input
> 0xb80acea8, SamplingRate 44100, Format 1, Channels 10, status 0" versus
> Jay's statement "The DSP-based EC solution has been used for other
> Android-based VoIP application. It supports 16K input and 48K/44.1K output."
> 
> In any case, further testing should be possible with the WIP in thus bug,
> and the opensl patch. The WIP is reasonably clean and could be
> reviewed/landed quickly if needed now.
I had a couple test patches that was shared through the email with Randell. Did you include those patch too?

Comment 46

[Gian-Carlo Pascutto [:gcp]]

No, I used what is published in this bug. Please put any required patches here.

Comment 47

[Jay]

Attachment: PATCH_70592_Test-patch-for-enabling-DSP-based-ECNS-on-Firefox-OS_kernel_20140814.tar.gz

Attached file has 3 kernel patches that are required to enable DSP based AEC.

Comment 48

[Jay]

Any update?

Comment 49

[Gian-Carlo Pascutto [:gcp]]

I tested with these kernel patches and the errors in adb logcat appear to have gone, but hardware echo cancellation is either still completely ineffective or not working. In the same call setup software AEC works fine.

I tested with 16kHz input sampling rate, same result.

Comment 50

[Gian-Carlo Pascutto [:gcp]]

Attachment: logaec.txt

Comment 51

[Jay]

Comment on attachment 8490086 [details]
logaec.txt

It looks to me this is the problem
"E/ACDB-LOADER(  202): ACDB -> Not correctly initialized!".

Do you see any error from "ACDB-LOADER" during system boot up?

Comment 52

[Gian-Carlo Pascutto [:gcp]]

E/ACDB-LOADER(  207): ACDB -> Error: directory open failed for /etc/acdbdata/MTP
E/ACDB-LOADER(  207): ACDB -> Could not load .acdb files!

Comment 53

[Jay]

It seems like either .acdb files are not there or in the different directory. Please, check if there is any .acdb file under /etc. If so, you can just copy them to /etc/acdbdata/MTP through the following adb command.

adb root
adb remount
adb shell mkdir /etc/acdbdata
adb shell mkdir /etc/acdbdata/MTP
adb shell cp /etc/*.acdb /etc/acdbdata/MTP
adb shell sync
adb shell ls -l /etc/acdbdata/MTP

If there is no .acdb file at all, then please, try to load the unmodified image from OEM.

Comment 54

[Randell Jesup [:jesup] (needinfo me)]

There are acdb files in the original v180 build (in /src/acdbdata/MTP and QRD)
Likely they're not copied and retained when a full build/flash is done.

Comment 55

[Gian-Carlo Pascutto [:gcp]]

>try to load the unmodified image from OEM.

This seems pointless as we already determined that both kernel patches and patches to system libraries (wilhelm/OpenSL) are needed.

Comment 56

[Randell Jesup [:jesup] (needinfo me)]

it may have been a broken build or out-of-date backup-flame; a new config.sh flame-kk and build pulled the acdbdata directory.  I haven't rebuilt the full set yet with this to see if they survive flash.sh or not

Comment 57

[Gian-Carlo Pascutto [:gcp]]

They won't. I was using a freshly flashed v180 with an up-to-date ./config flame-kk tree.

Comment 58

[Jay]

Gian-Carlo,

I tested "WIP ugly patch for HW AEC" patch locally here and I see the HW AEC is enabled properly. Can we get this mainlined?

Comment 59

[Gian-Carlo Pascutto [:gcp]]

We'll need to extend the patch here to only enable the HW AEC (and disable the webrtc.org one...) if a pref is set. A Gecko build flag is actually easiest, so I'll make a MOZ_WEBRTC_HW_AEC for it, and then the B2G build will need to set that flag *and be fixed to include the missing files*. If the latter isn't done this will fail spectacularly.

Comment 60

[Gian-Carlo Pascutto [:gcp]]

Alternate approach: add a pref in prefs.js for enabling HW AEC, reading that into a ::Config field, implement VoEBase::audio_device_module() (shoving things in shared_data if needed) and then calling EnableBuiltinAEC on that (which will set a flag that will cause the AEC to be enabled on Init() time).

That's the "proper" fix but way more involved.

Comment 61

[Gian-Carlo Pascutto [:gcp]]

Attachment: Add an option to use hardware AEC for WebRTC

This is the slightly hackish but easier to uplift solution using
a build flag rather than adding a real pref and reengineering the
webrtc code to get it down to the low level audio stack.

B2G devices with hardware AEC then just need to set the right
Gecko flag.

Comment 62

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8491401 [details] [diff] [review]
Add an option to use hardware AEC for WebRTC

Review of attachment 8491401 [details] [diff] [review]:
-----------------------------------------------------------------

::: build/gyp.mozbuild
@@ +49,5 @@
>      # Creates AEC internal sample dump files in current directory
>      'aec_debug_dump': 1,
>  
> +    # Enable and force use of hardware AEC
> +    'hardware_aec': 1 if CONFIG['MOZ_WEBRTC_HARDWARE_AEC'] else 0,

'hardware_aec_ns' ?

::: configure.in
@@ +5010,5 @@
> +dnl ========================================================
> +dnl = Force hardware AEC, disable webrtc.org AEC
> +dnl ========================================================
> +MOZ_ARG_ENABLE_BOOL(hardware-aec,
> +[  --enable-hardware-aec   Enable support for hardware AEC],

"Force use of hardware AEC and noise suppression"
HARDWARE_AEC_NS perhaps?
Or two prefs, though at this point I don't see them as separate.

::: media/webrtc/trunk/webrtc/modules/audio_device/android/opensles_input.cc
@@ +399,5 @@
>    return true;
>  }
>  
> +#if defined(WEBRTC_GONK) && defined(WEBRTC_HARDWARE_AEC)
> +bool OpenSlesInput::CheckPlatformAEC() {

Do we need to check for NS as well?  Or if it fails to open, we just ignore that?  (Note that we'll likely have disabled NS in prefs, though on a platform with AEC and no NS I suppose we could leave SW NS enabled)

@@ +413,5 @@
> +  for (uint32_t i = 0; i < numFx; i++) {
> +    if (android::AudioEffect::queryEffect(i, &fxDesc) != android::NO_ERROR) {
> +      continue;
> +    }
> +    if (memcmp(&fxDesc.type, FX_IID_AEC, sizeof(effect_uuid_t)) == 0) {

my normal preference is for sizeof(field_I'm_comparing) instead of sizeof(type) - that way if I ever change the type I'm covered (and I can't typo the type or misread/misremember the source).   Just my preference and thought I'd mention it, I don't insist on this in reviews.

Comment 63

[(not currently active) Ted Mielczarek]

Comment on attachment 8491401 [details] [diff] [review]
Add an option to use hardware AEC for WebRTC

Review of attachment 8491401 [details] [diff] [review]:
-----------------------------------------------------------------

::: configure.in
@@ +5010,5 @@
> +dnl ========================================================
> +dnl = Force hardware AEC, disable webrtc.org AEC
> +dnl ========================================================
> +MOZ_ARG_ENABLE_BOOL(hardware-aec,
> +[  --enable-hardware-aec   Enable support for hardware AEC],

Do we really need a configure argument for this?

Comment 64

[Randell Jesup [:jesup] (needinfo me)]

> > +[  --enable-hardware-aec   Enable support for hardware AEC],
> 
> Do we really need a configure argument for this?

Yes; 'default' builds for b2g won't use it; only ones where the vendor supports and has tested HW AEC (like 8x10).

Comment 65

[Nils Ohlmeier [:drno]]

Would it be possible to have options likes (and H264) as options in the developer menu?
That would allow for easier testing on the Flame device. And make integration testing for partners easier as well. Users who would try to turn it on on devices which don't support H264 or AEC would have to live with the consequences.

Comment 66

[Gian-Carlo Pascutto [:gcp]]

>Do we really need a configure argument for this?

I tried doing it via an "export MOZ_WEBRTC_HARDWARE_AEC" but that didn't seem to mix well with the B2G build.

As explained in the comments before, we'd like a pref but it involves quite a bit more WebRTC engineering than is upliftable. (This also answers Nils' question for the AEC side, I have no idea about H264)

Comment 67

[Gian-Carlo Pascutto [:gcp]]

https://tbpl.mozilla.org/?tree=Try&rev=57335f1fc511

Comment 68

[Gian-Carlo Pascutto [:gcp]]

https://tbpl.mozilla.org/?tree=Try&rev=d5eb41b867ba

Comment 69

[Gian-Carlo Pascutto [:gcp]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fa58a19147aa

Comment 70

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/fa58a19147aa

Comment 71

[Ryan VanderMeulen [:RyanVM]]

Please request b2g32 and aurora approval on this patch when you get a chance.

Comment 72

[Jay]

Just had a discussion on how to have the device/vendor specific configure.in file with Randell and mwu but according to mwu, configure.in should only include the per base-version changes that apply to all the devices. For device-specific changes, it needs to be done run-time.

However, the challenge is how to know the AudioEffect has HW AEC/NS support during run-time. I don't see a good way to indicate that but from effect descriptor, we can check the implementor field[1] to know this is Qualcomm effect and it can be implied that HW AEC/NS is used.

Can we add the implementor check in OpenSlesInput::CheckPlatformAEC()[2] and decide if HW or SW AEC should be used?


[1]https://source.android.com/devices/reference/structeffect__descriptor__s.html
[2]https://hg.mozilla.org/integration/mozilla-inbound/rev/fa58a19147aa#l5.72

Comment 73

[Jay]

Gian-Carlo,
Can you also comment on comment#72?

Comment 74

[Gian-Carlo Pascutto [:gcp]]

No, we cannot do this for a number of reasons:

1) If we see in this initialization code that we can not use HW AEC, we are very, very far away from the place we need to be to re-enable the software AEC (and I've made the code hard assert in this situation for this very reason). I discussed the complexity of switching this in comment 60, but what you want is even harder because we need to percolate the failure back instead of just respecting a pref. Even if we find the time to implement this, the resulting patch would be harder to uplift.

2) Trying to switch the effect on based on the chipset vendor, or the result of CheckPlatformAEC seems like a bad idea to me. You will notice that I have reported that this is currently reporting success *yet I have never seen the hardware AEC working myself*. There is absolutely no guarantee whatsoever that just because a device has HW AEC that it is also working correctly or with a sufficient quality. I think the only time we should enable hardware AEC should be if we have a hard confirmation from QA testing that it provides comparable quality to software AEC, and that should very much be a per-device decision, and not something that we try to guess at runtime.

Lastly, I notice that you talk about "specific configure.in" file. There is no need for that? You do not need to modify the Gecko build at all, you just need to specify that flag when Gecko's configure is called from the B2G build, for example via gonk-misc/default-gecko-config. Is it really a problem for B2G to have device-specific Gecko flags?

If the patch here indeed doesn't work, it needs to be backed out, but I have no alternate solution that is feasible in the short term.

Comment 75

[Jay]

(In reply to Gian-Carlo Pascutto [:gcp] from comment #74)
> No, we cannot do this for a number of reasons:
> 
> 1) If we see in this initialization code that we can not use HW AEC, we are
> very, very far away from the place we need to be to re-enable the software
> AEC (and I've made the code hard assert in this situation for this very
> reason). I discussed the complexity of switching this in comment 60, but
> what you want is even harder because we need to percolate the failure back
> instead of just respecting a pref. Even if we find the time to implement
> this, the resulting patch would be harder to uplift.
> 
> 2) Trying to switch the effect on based on the chipset vendor, or the result
> of CheckPlatformAEC seems like a bad idea to me. You will notice that I have
> reported that this is currently reporting success *yet I have never seen the
> hardware AEC working myself*. There is absolutely no guarantee whatsoever
> that just because a device has HW AEC that it is also working correctly or
> with a sufficient quality. I think the only time we should enable hardware
> AEC should be if we have a hard confirmation from QA testing that it
> provides comparable quality to software AEC, and that should very much be a
> per-device decision, and not something that we try to guess at runtime.
> 
> Lastly, I notice that you talk about "specific configure.in" file. There is
> no need for that? You do not need to modify the Gecko build at all, you just
> need to specify that flag when Gecko's configure is called from the B2G
> build, for example via gonk-misc/default-gecko-config. Is it really a
> problem for B2G to have device-specific Gecko flags?
> 
> If the patch here indeed doesn't work, it needs to be backed out, but I have
> no alternate solution that is feasible in the short term.

Ok. I guess we just need to go with this approach. Please, have this patch merged in v2.0 branch too.

Comment 76

[Gian-Carlo Pascutto [:gcp]]

I had some discussion with jesup yesterday and one thing I think we might be able to do reasonably easily is to trigger the switch off of the presence or non-presence of a file on the device. (Jesup was thinking about a global variable because he believed sandboxing would prevent the use of a file - my understanding of e10s in B2G is that it's the opposite, i.e. a file should work as long as we don't have a true sandbox and a global might not be accessible from the content process)

This would allow the switch to happen without needing a different(ly compiled) Gecko.

It sounds ugly but on the other hand it works better, and if that file is one which identifies the device as hardware we have tested HW AEC to work on, I think it's actually quite acceptable. Thoughts?

Comment 77

[Michael Vines [:m1] [:evilmachines]]

We are able to pass the additional Gecko configure flag in our tree.  Since time is running out on v2.0 and v2.1, let's just do that for now and we can spawn a new bug to perhaps do something nicer for master for the next release.  That is, the code currently in master can be uplifted as is to v2.0

Comment 78

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8491401 [details] [diff] [review]
Add an option to use hardware AEC for WebRTC

[Approval Request Comment]
Bug caused by (feature/regressing bug #): General WebRTC capability
User impact if declined: Increased CPU usage during WebRTC calls
Testing completed: Landed on m-c a few days ago.
Risk to taking this patch (and alternatives if risky): At worst we will end up with no AEC - unlikely given that the flag is supposed to be only enabled after verifying that's not the case.
String or UUID changes made by this patch: N/A

Comment 79

[Gian-Carlo Pascutto [:gcp]]

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/586f8560f032

Comment 80

[Gian-Carlo Pascutto [:gcp]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5d0ec7211d63

Comment 81

[Kai-Zhen Li [:seinlin][:kli]]

http://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/586f8560f032

Comment 82

[Maria Angeles Oteo (:oteo)]

Jay, Gian Carlo, we have applied the Linux patches and used the latest binaries (base build 184) and it seems the audio quality is better. Is there any way to check in the logcat if we are really using the HW Echo and Noise Cancelation?

Comment 83

[Gian-Carlo Pascutto [:gcp]]

If you enable webrtc logging (remount system partition rw, change b2g.sh, add export NSPR_LOG_MODULES=webrtc_trace:65535 and export NSPR_LOG_FILE=/some/writeable/dir/log.txt, then restart b2g) then you should see log output in that file as in the first part of comment 41, i.e.:

ERROR     ; (10: 3:45:636 |    0) AUDIO DEVICE:    1    99;      1433; Platform has AEC: 1

Note that the log is recycled (circular) so you have to do this rather quickly after the audio starts.

You can then check if the AEC actually works. For example by putting the volume to the maximum, setting up a call, putting the phone in an isolated room and speaking loudly on the other end of the call. You should not hear anything come back.

Note that enabling the HW AEC requires passing the build flag to Gecko and making sure the B2G build preserves the needed files in the filesystem. I have no idea if the work needed for that was done.

Comment 84

[Jay]

(In reply to Maria Angeles Oteo (:oteo) from comment #82)
> Jay, Gian Carlo, we have applied the Linux patches and used the latest
> binaries (base build 184) and it seems the audio quality is better. Is there
> any way to check in the logcat if we are really using the HW Echo and Noise
> Cancelation?

You can also check from Audio HAL but you need to re-build the code.
Just un-comment the following line [1] and rebuild the image. It will print "Setting EC Reference: 1" when HW EC is enabled

[1] https://www.codeaurora.org/cgit/quic/la/platform/hardware/qcom/audio/tree/hal/msm8974/platform.c?h=LNX.LF.3.5.1_RB1.2#n21