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

RESOLVED FIXED in Firefox 34, Firefox OS v2.0

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: jaywang, Assigned: gcp)

Tracking

unspecified
mozilla35
All
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:2.0+, firefox33 wontfix, firefox34 fixed, firefox35 fixed, b2g-v2.0 fixed, b2g-v2.0M fixed, b2g-v2.1 fixed, b2g-v2.2 fixed)

Details

(Whiteboard: [caf priority: p2][CR 715027])

Attachments

(8 attachments, 4 obsolete attachments)

(Reporter)

Description

4 years ago
[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.
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.
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 ?
(Reporter)

Comment 3

4 years ago
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?
Flags: needinfo?(bbajaj)
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.
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).
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.
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.
Jay has prototyped this over here already, might be good to sync up on a call with him.
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.)
(Reporter)

Comment 10

4 years ago
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.
(Reporter)

Comment 11

4 years ago
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?
Flags: needinfo?(rjesup)
(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)
Flags: needinfo?(rjesup)
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
blocking-b2g: 2.0? → 2.0+
Flags: needinfo?(bbajaj)

Updated

4 years ago
Assignee: nobody → rjesup
Created attachment 8483792 [details] [diff] [review]
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.

Updated

4 years ago
Attachment #8483792 - Flags: feedback?(paul)
Attachment #8483792 - Flags: feedback?(jaywang)
Attachment #8483792 - Flags: feedback?(gpascutto)
(Reporter)

Comment 15

4 years ago
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.
Attachment #8483792 - Flags: feedback?(jaywang) → feedback-
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
Flags: needinfo?(gpascutto)
-> gcp (IRC)
Assignee: rjesup → gpascutto
Flags: needinfo?(gpascutto)
(Assignee)

Comment 18

4 years ago
Created attachment 8484361 [details] [diff] [review]
WIP ugly patch for HW AEC

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

Updated

4 years ago
Attachment #8483792 - Attachment is obsolete: true
Attachment #8483792 - Flags: feedback?(paul)
Attachment #8483792 - Flags: feedback?(gpascutto)
(Assignee)

Comment 19

4 years ago
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)
(Assignee)

Comment 20

4 years ago
Created attachment 8485839 [details] [diff] [review]
WIP ugly patch for HW AEC

Current WIP
(Assignee)

Updated

4 years ago
Attachment #8484361 - Attachment is obsolete: true
(Assignee)

Comment 21

4 years ago
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.
(Assignee)

Comment 22

4 years ago
Created attachment 8485870 [details] [diff] [review]
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
(Assignee)

Updated

4 years ago
Attachment #8485839 - Attachment is obsolete: true
(Assignee)

Comment 23

4 years ago
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
Flags: needinfo?(jaywang)
(Assignee)

Comment 24

4 years ago
Created attachment 8486257 [details] [diff] [review]
WIP ugly patch for HW AEC

Updated WIP
(Assignee)

Updated

4 years ago
Attachment #8485870 - Attachment is obsolete: true
(Assignee)

Comment 25

4 years ago
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
(Reporter)

Comment 26

4 years ago
(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.
(Reporter)

Comment 27

4 years ago
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
Flags: needinfo?(jaywang)
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
  }
Created attachment 8486749 [details]
audio_effects.conf
Created attachment 8486750 [details]
vendor.conf

These are from the v180 drop
(Reporter)

Comment 31

4 years ago
(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?
Flags: needinfo?(rjesup)
vendor.conf is /system/vendor/etc/audio_effects.conf
Flags: needinfo?(rjesup)
(Reporter)

Comment 33

4 years ago
(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"?
Flags: needinfo?(rjesup)
Flags: needinfo?(gpascutto)
/vendor/etc/audio_effects.conf and /system/vendor/etc/audio_effects.conf are identical
(this is with KK-based build v180)
Flags: needinfo?(rjesup)
(Assignee)

Comment 35

4 years ago
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.
Assignee: gpascutto → nobody
Flags: needinfo?(gpascutto)
(Assignee)

Comment 36

4 years ago
(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.
(Assignee)

Comment 37

4 years ago
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
(Reporter)

Comment 38

4 years ago
(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?
Flags: needinfo?(gpascutto)
(Assignee)

Comment 39

4 years ago
Created attachment 8487732 [details]
audio logs.txt
Flags: needinfo?(gpascutto)
(Assignee)

Comment 40

4 years ago
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.
(Assignee)

Comment 41

4 years ago
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.
(Assignee)

Comment 42

4 years ago
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.
(Assignee)

Comment 43

4 years ago
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.
(Assignee)

Comment 44

4 years ago
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
(Reporter)

Comment 45

4 years ago
(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?
(Assignee)

Comment 46

4 years ago
No, I used what is published in this bug. Please put any required patches here.
Blocks: 1060389
No longer blocks: 1060389
Blocks: 1062883
(Reporter)

Comment 47

4 years ago
Created attachment 8488742 [details]
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.

Updated

4 years ago
Whiteboard: [CR 715027]

Updated

4 years ago
Whiteboard: [CR 715027] → [caf priority: p2][CR 715027]
(Reporter)

Comment 48

4 years ago
Any update?
Flags: needinfo?(gpascutto)
(Assignee)

Comment 49

4 years ago
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.
Flags: needinfo?(gpascutto)

Updated

4 years ago
Flags: needinfo?(jaywang)
(Assignee)

Comment 50

4 years ago
Created attachment 8490086 [details]
logaec.txt
(Reporter)

Comment 51

4 years ago
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?
Flags: needinfo?(jaywang)
(Reporter)

Updated

4 years ago
Flags: needinfo?(rjesup)
Flags: needinfo?(gpascutto)
(Assignee)

Comment 52

4 years ago
E/ACDB-LOADER(  207): ACDB -> Error: directory open failed for /etc/acdbdata/MTP
E/ACDB-LOADER(  207): ACDB -> Could not load .acdb files!
Flags: needinfo?(gpascutto)
(Reporter)

Comment 53

4 years ago
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.
Flags: needinfo?(rjesup) → needinfo?(gpascutto)
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.

Updated

4 years ago
Blocks: 1041241
(Assignee)

Comment 55

4 years ago
>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.
Flags: needinfo?(gpascutto)
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
(Assignee)

Comment 57

4 years ago
They won't. I was using a freshly flashed v180 with an up-to-date ./config flame-kk tree.
(Reporter)

Comment 58

4 years ago
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?
Flags: needinfo?(gpascutto)
(Assignee)

Comment 59

4 years ago
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.
Flags: needinfo?(gpascutto)
QA Contact: gpascutto
(Assignee)

Updated

4 years ago
Assignee: nobody → gpascutto
QA Contact: gpascutto
(Assignee)

Comment 60

4 years ago
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.
(Assignee)

Comment 61

4 years ago
Created attachment 8491401 [details] [diff] [review]
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.
Attachment #8491401 - Flags: review?(ted)
Attachment #8491401 - Flags: review?(rjesup)
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.
Attachment #8491401 - Flags: review?(rjesup) → review+
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?
Attachment #8491401 - Flags: review?(ted) → review+
> > +[  --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).
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.
(Assignee)

Comment 66

4 years ago
>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)
https://hg.mozilla.org/mozilla-central/rev/fa58a19147aa
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Please request b2g32 and aurora approval on this patch when you get a chance.
status-b2g-v2.0: --- → affected
status-b2g-v2.1: --- → affected
status-b2g-v2.2: --- → fixed
status-firefox33: --- → wontfix
status-firefox34: --- → affected
status-firefox35: --- → affected
Flags: needinfo?(gpascutto)
(Reporter)

Comment 72

4 years ago
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
Flags: needinfo?(rjesup)
(Reporter)

Comment 73

4 years ago
Gian-Carlo,
Can you also comment on comment#72?
(Assignee)

Comment 74

4 years ago
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.
Flags: needinfo?(jaywang)
(Reporter)

Comment 75

4 years ago
(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.
Flags: needinfo?(jaywang)
(Assignee)

Comment 76

4 years ago
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?
Flags: needinfo?(jaywang)
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
(Assignee)

Comment 78

4 years ago
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
Attachment #8491401 - Flags: approval-mozilla-b2g32?
Attachment #8491401 - Flags: approval-mozilla-aurora?
Flags: needinfo?(rjesup)
Flags: needinfo?(jaywang)
Flags: needinfo?(gpascutto)

Updated

4 years ago
Attachment #8491401 - Flags: approval-mozilla-b2g32?
Attachment #8491401 - Flags: approval-mozilla-b2g32+
Attachment #8491401 - Flags: approval-mozilla-aurora?
Attachment #8491401 - Flags: approval-mozilla-aurora+
(Assignee)

Comment 80

4 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/5d0ec7211d63
status-b2g-v2.0: affected → fixed
status-firefox34: affected → fixed
status-firefox35: affected → fixed
status-b2g-v2.1: affected → fixed
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?
Flags: needinfo?(jaywang)
Flags: needinfo?(gpascutto)
(Assignee)

Comment 83

4 years ago
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.
Flags: needinfo?(gpascutto)
(Reporter)

Comment 84

4 years ago
(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
Flags: needinfo?(jaywang)

Updated

4 years ago
See Also: → bug 1125843
You need to log in before you can comment on or make changes to this bug.