1027713 - When microphone is active, pan all output audio to right speaker on MacBookPros

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

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

On a MacBookPro (2011 or later, not sure about earlier), the microphone(s) are located on top of the left speaker(!!!!!!!)  Not sure what the HW designer was smoking when that decision was made - probably the same as when they designed the apple-custom headset jack with massive amounts of crosstalk between mic and output channels. (I imagine there was an edict from someone "here's the casework, no extra holes allowed - we like how it looks.  Use the ones we gave you!)

Needless to say, this sucks from a speakerphone mode on such a device.  That close to the speaker, non-linear echoes overload the microphone and cause the AEC to mis-train.

Webrtc.org code, Skype, and I presume Chrome send all the audio to the right speaker on a MBPro in speakerphone mode.  I've tested this, and it makes a huge difference to audio quality.

Webrtc.org tests like this: (error handling removed) in media/webrtc/trunk/webrtc/modules/audio_device/mac/audio_device_mac.cc

    int intErr = sysctlbyname("hw.model", buf, &length, NULL, 0);
and
        if (strncmp(buf, "MacBookPro", 10) == 0)

And to determine the output type in ::InitPlayout() (which we don't use)
            WEBRTC_CA_LOG_WARN(AudioObjectGetPropertyData(_outputDeviceID,
                    &propertyAddress, 0, NULL, &size, &dataSource));
            if (dataSource == 'ispk')
etc

Note we probably want to dynamically track this (as they do with a listener) and invoke the pan-right whenever we also are told the mic is in use (by WebRTC at least).

So I think we need some tracking and panning code in cubeb, and an api whereby getUserMedia can inform cubeb if it's using the mic.  (If we can determine if it's the internal mic, even better, but I'll note they don't check.)

Comment 1

[Matthew Gregan [:kinetik]]

I'll grab this for now.  As discussed with jesup on IRC, current plan is to do the following:

- Add a volume API to libcubeb to support panning (we want to move volume to libcubeb anyway, so double win)
- Add an output device enumeration API (again, something we want eventually) and implement a stub that returns enough information to detect when we're using internal speakers on a Mac/MBP

Comment 2

[Matthew Gregan [:kinetik]]

I have to drop this for now, got a pile of new MSE bugs that block shipping which I need to address before I can spend time on anything else.

Comment 3

[Paul Adenot (:padenot)]

Matthew, I can probably take this on the side while I wait for trypushes of the MSG refactoring, would something like this work?

Volume API:
/**
 * Set the volume for each channels.
 * @param stm the stream for which to adjust the volume
 * @param volumes an array of floats between 0.0 (muted) and 1.0 (maximum volumes)
 * @param channels the size of the array volumes
 * @return CUBEB_ERROR_INVALID_PARAMETER if volumes contains values outside [0.0; 1.0]
 * @return CUBEB_ERROR_INVALID_PARAMETER if channels is different from the stream's channel count
 * @return CUBEB_ERROR_INVALID_PARAMETER is either stm or volumes are invalid pointers
 * @return CUBEB_OK otherwise
 */
int cubeb_stream_set_volume(cubeb_stream * stm, float * volumes, size_t channels);

Then, we need to get the device currently in use:

struct cubeb_output_device {
  char * name;
  /* we can add stuff here if the name if not enough for now */
}

/**
 * @param stm the stream for which to query the current output device
 * @param device a pointer in which the current output device will be stored.
 * @return CUBEB_OK in case of success
 * @return CUBEB_ERROR_INVALID_PARAMETER if either stm, device or count are invalid pointers
 */
int cubeb_stream_get_current_output_device(cubeb_stream * stm, cubeb_output_device ** device, size_t * count);

/**
 * Destroy a cubeb_output_device structure.
 * @param devices the devices to destroy
 * @return CUBEB_OK in case of success
 * @return CUBEB_ERROR_INVALID_PARAMETER if devices is an invalid pointer
 */
int cubeb_output_devices_destroy(cubeb_output_device * devices);

Then we can think about a proper device enumeration API. We also need a callback that tells us various cases of output device invalidation (plug/unplug headset with a change in sampling rate, for example)

Comment 4

[Maire Reavy [:mreavy]]

Thanks, Paul!

Comment 5

[Matthew Gregan [:kinetik]]

(In reply to Paul Adenot (:padenot) from comment #3)
> Matthew, I can probably take this on the side while I wait for trypushes of
> the MSG refactoring, would something like this work?
> 
> Volume API:
> /**
>  * Set the volume for each channels.
>  * @param stm the stream for which to adjust the volume
>  * @param volumes an array of floats between 0.0 (muted) and 1.0 (maximum
> volumes)
>  * @param channels the size of the array volumes
>  * @return CUBEB_ERROR_INVALID_PARAMETER if volumes contains values outside
> [0.0; 1.0]
>  * @return CUBEB_ERROR_INVALID_PARAMETER if channels is different from the
> stream's channel count
>  * @return CUBEB_ERROR_INVALID_PARAMETER is either stm or volumes are
> invalid pointers
>  * @return CUBEB_OK otherwise
>  */
> int cubeb_stream_set_volume(cubeb_stream * stm, float * volumes, size_t
> channels);

This looks good, however I think we need to *pan* (i.e. mix left channel into right and mute left) rather than just *mute* channels for jesup's use case.

> Then, we need to get the device currently in use:
> 
> struct cubeb_output_device {
>   char * name;
>   /* we can add stuff here if the name if not enough for now */
> }
> 
> /**
>  * @param stm the stream for which to query the current output device
>  * @param device a pointer in which the current output device will be stored.
>  * @return CUBEB_OK in case of success
>  * @return CUBEB_ERROR_INVALID_PARAMETER if either stm, device or count are
> invalid pointers
>  */
> int cubeb_stream_get_current_output_device(cubeb_stream * stm,
> cubeb_output_device ** device, size_t * count);

cubeb_output_device const **

What is count for? Since this returns the current device, I'd only expect a single pointer to be returned.

> /**
>  * Destroy a cubeb_output_device structure.
>  * @param devices the devices to destroy
>  * @return CUBEB_OK in case of success
>  * @return CUBEB_ERROR_INVALID_PARAMETER if devices is an invalid pointer
>  */
> int cubeb_output_devices_destroy(cubeb_output_device * devices);
> 
> Then we can think about a proper device enumeration API. We also need a
> callback that tells us various cases of output device invalidation
> (plug/unplug headset with a change in sampling rate, for example)

Sounds good.

Comment 6

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

(In reply to Matthew Gregan [:kinetik] from comment #5)
> (In reply to Paul Adenot (:padenot) from comment #3)

> > int cubeb_stream_set_volume(cubeb_stream * stm, float * volumes, size_t
> > channels);
> 
> This looks good, however I think we need to *pan* (i.e. mix left channel
> into right and mute left) rather than just *mute* channels for jesup's use
> case.

True - though I'll note that 99.9% of webrtc use for a while will be mono.  But we still should implement it as panning.

Comment 7

[Paul Adenot (:padenot)]

(In reply to Matthew Gregan [:kinetik] from comment #5)
> (In reply to Paul Adenot (:padenot) from comment #3)
> > 
> > /**
> >  * @param stm the stream for which to query the current output device
> >  * @param device a pointer in which the current output device will be stored.
> >  * @return CUBEB_OK in case of success
> >  * @return CUBEB_ERROR_INVALID_PARAMETER if either stm, device or count are
> > invalid pointers
> >  */
> > int cubeb_stream_get_current_output_device(cubeb_stream * stm,
> > cubeb_output_device ** device, size_t * count);
> 
> cubeb_output_device const **
> 
> What is count for? Since this returns the current device, I'd only expect a
> single pointer to be returned.

I think I started writing a device enumeration API and then turned it, apparently without thinking, into a function to get the current device. Sorry about that, `count` of course is not needed.

Comment 8

[Paul Adenot (:padenot)]

(In reply to Matthew Gregan [:kinetik] from comment #5)
> (In reply to Paul Adenot (:padenot) from comment #3)
> > Matthew, I can probably take this on the side while I wait for trypushes of
> > the MSG refactoring, would something like this work?
> > 
> > Volume API:
> > /**
> >  * Set the volume for each channels.
> >  * @param stm the stream for which to adjust the volume
> >  * @param volumes an array of floats between 0.0 (muted) and 1.0 (maximum
> > volumes)
> >  * @param channels the size of the array volumes
> >  * @return CUBEB_ERROR_INVALID_PARAMETER if volumes contains values outside
> > [0.0; 1.0]
> >  * @return CUBEB_ERROR_INVALID_PARAMETER if channels is different from the
> > stream's channel count
> >  * @return CUBEB_ERROR_INVALID_PARAMETER is either stm or volumes are
> > invalid pointers
> >  * @return CUBEB_OK otherwise
> >  */
> > int cubeb_stream_set_volume(cubeb_stream * stm, float * volumes, size_t
> > channels);
> 
> This looks good, however I think we need to *pan* (i.e. mix left channel
> into right and mute left) rather than just *mute* channels for jesup's use
> case.

I'll keep that in mind.

Comment 9

[Paul Adenot (:padenot)]

Attachment: Add a volume API in cubeb. r=

Comment 10

[Paul Adenot (:padenot)]

Attachment: Add a cubeb API to query the name of the audio output device in use. r=

Comment 11

[Paul Adenot (:padenot)]

Attachment: Add a cubeb API to signal that the output device changed. r=

Comment 12

[Paul Adenot (:padenot)]

Attachment: Pan audio to the right when we are using gUM and outputing sound using MacBookPro speakers. r=

Comment 13

[Paul Adenot (:padenot)]

This is almost done, but there is a little deadlock: the listener callback calls back into cubeb to set the panning. This tries to grab the same lock and deadlock.

Also we need stubbing/implementation for the other platforms but OSX is the priority.

Comment 14

[Paul Adenot (:padenot)]

Attachment: Add a volume API in cubeb. r=

Comment 15

[Paul Adenot (:padenot)]

Attachment: Add a cubeb API to query the name of the audio output device in use. r=

This is stubbed in cubeb.c on most platforms but osx, I'll open bugs to
implement the other platforms, those are likely to be good first bugs.

Comment 16

[Paul Adenot (:padenot)]

Attachment: Add a cubeb API to signal that the output device changed. r=

The reentrant mutex is needed so that users can call back into cubeb's API from
the callback.

Comment 17

[Paul Adenot (:padenot)]

Attachment: Pan audio to the right when we are using gUM and outputing sound using MacBookPro speakers. r=

This is _very_ temporary, this will go away when the MSG refactoring lands,
because the MSG won't use AudioStream anymore.

Comment 18

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

Comment on attachment 8456234 [details] [diff] [review]
Pan audio to the right when we are using gUM and outputing sound using MacBookPro speakers. r=

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

::: content/media/AudioStream.cpp
@@ +552,5 @@
> +  char name[128];
> +  size_t length = sizeof(name);
> +
> +  rv = sysctlbyname("hw.model", name, &length, NULL, 0);
> +  if (rv) {

typically people expect rv to be nsresult; how about 'ret' or 'retval' or 'result'?

@@ +562,5 @@
> +      // Check if we are currently outputing sound on external speakers.
> +      if (!strcmp(out->name, "ispk")) {
> +        // Pan everything to the right speaker.
> +        if (aMicrophoneActive) {
> +          printf("panning to the right.\n");

remove printf... or convert to LOG perferably

Comment 19

[Paul Adenot (:padenot)]

Comment on attachment 8456233 [details] [diff] [review]
Add a cubeb API to signal that the output device changed. r=

Those are a bit broken. I have a patch for that, I'll attach tomorrow.

Comment 20

[Paul Adenot (:padenot)]

Attachment: Add a volume API in cubeb. r=

Comment 21

[Paul Adenot (:padenot)]

Attachment: Add a cubeb API to query the name of the audio output device in use. r=

Comment 22

[Paul Adenot (:padenot)]

Attachment: Add a cubeb API to signal that the output device changed. r=

The reentrant mutex is needed so that users can call back into cubeb's API from
the callback.

Comment 23

[Paul Adenot (:padenot)]

Attachment: Pan audio to the right when we are using gUM and outputing sound using MacBookPro speakers. r=

Addressed review comments.

Comment 24

[Matthew Gregan [:kinetik]]

Comment on attachment 8458788 [details] [diff] [review]
Add a cubeb API to signal that the output device changed. r=

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

::: media/libcubeb/include/cubeb.h
@@ +325,5 @@
> + * @return CUBEB_ERROR_INVALID_PARAMETER if either stream or
> + *         device_changed_callback are invalid pointers.
> + * @return CUBEB_OK
> + */
> +int cubeb_stream_register_device_changed_callback(cubeb_stream * stream,

There should be a way to unregister without destroying the stream, maybe calling this with a NULL device_changed_callback?

::: media/libcubeb/src/cubeb_audiounit.c
@@ +38,5 @@
>    AudioUnit unit;
>    cubeb_data_callback data_callback;
>    cubeb_state_callback state_callback;
> +  cubeb_device_changed_callback device_changed_callback;
> +  void * device_changed_user_ptr;

I'd rather keep a single user_ptr for all callbacks unless there's a pressing reason to do otherwise.

Comment 25

[Matthew Gregan [:kinetik]]

Comment on attachment 8458786 [details] [diff] [review]
Add a volume API in cubeb. r=

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

::: content/media/AudioStream.cpp
@@ +738,5 @@
>  {
>    MonitorAutoLock mon(mMonitor);
>    NS_ABORT_IF_FALSE(aVolume >= 0.0 && aVolume <= 1.0, "Invalid volume");
> +
> +  if (cubeb_stream_set_volume(mCubebStream, aVolume) != CUBEB_OK) {

Doesn't this break volume control for backends that haven't implemented stream_set_volume?

Comment 26

[Paul Adenot (:padenot)]

Attachment: Part 1 - Add a volume API in cubeb. r=

Yeah, you're right, I only uploaded the mac patch by mistake. This implements
the function on all backends. I manually tested every platform (apart from
sndio, I don't have a BSD machine, but the function was in fact already written
for some reason, so I merely put it in the vtable).

Comment 27

[Matthew Gregan [:kinetik]]

Comment on attachment 8459345 [details] [diff] [review]
Part 1 - Add a volume API in cubeb. r=

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

::: media/libcubeb/src/cubeb_pulse.c
@@ +253,5 @@
>  {
>    while (WRAP(pa_operation_get_state)(o) == PA_OPERATION_RUNNING) {
>      WRAP(pa_threaded_mainloop_wait)(ctx->mainloop);
> +    if (!PA_CONTEXT_IS_GOOD(WRAP(pa_context_get_state)(ctx->context))) {
> +      printf("%d\n", __LINE__);

Remove the printfs.

::: media/libcubeb/src/cubeb_winmm.c
@@ +640,5 @@
> +  vol = volume * 0xffff;
> +  vol |= vol << 16;
> +
> +  EnterCriticalSection(&stm->lock);
> +  r = waveOutSetVolume(stm->waveout, vol);

I have a bad feeling this controls the volume for the entire process rather than a single stream...

Comment 28

[Paul Adenot (:padenot)]

Attachment: Trigger the panning logic on stream creation. r=

UpdateStreamOrder is not called often because it's expensive (it's called only
when the graph topology changed), and it's earlier in the MSG loop than the
audio stream creation, so we need to tell the newly created AudioStream that a
microphone is active on creation as well.

The panning logic is also now triggered on stream start, because it is async.

Comment 29

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

Comment on attachment 8460181 [details] [diff] [review]
Trigger the panning logic on stream creation. r=

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

::: content/media/MediaStreamGraph.cpp
@@ +959,5 @@
>                                           aStream->mAudioChannelType,
>                                           AudioStream::LowLatency);
>          audioOutputStream->mTrackID = tracks->GetID();
>  
> +        // If there is a mixer, there is a micrphone active.

microphone

Comment 30

[Paul Adenot (:padenot)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/63af4528bd9c
https://hg.mozilla.org/integration/mozilla-inbound/rev/a56aad94c7c9
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad05dc816fa0
https://hg.mozilla.org/integration/mozilla-inbound/rev/03edb1ab3182
https://hg.mozilla.org/integration/mozilla-inbound/rev/e0bba6f4966f

Comment 31

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I backed these out (along with the changes from bug 1023947) in https://hg.mozilla.org/integration/mozilla-inbound/rev/5dc0231d0153 for making cppunit tests frequently fail like this:

https://tbpl.mozilla.org/php/getParsedLog.php?id=44534511&tree=Mozilla-Inbound

Comment 32

[Paul Adenot (:padenot)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/df9e0e991df9
https://hg.mozilla.org/integration/mozilla-inbound/rev/da477d34528d
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d6a16456f11
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e7128b769a7
https://hg.mozilla.org/integration/mozilla-inbound/rev/fd4a70e6d514
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e28cd6f2ab8

(the issue was fixed by 1045018, pushed at the same time)

Comment 33

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/bf4ed2946c45
https://hg.mozilla.org/mozilla-central/rev/bd925dd0edbf
https://hg.mozilla.org/mozilla-central/rev/9c82fdf124df
https://hg.mozilla.org/mozilla-central/rev/df9e0e991df9
https://hg.mozilla.org/mozilla-central/rev/8c91e346fb03
https://hg.mozilla.org/mozilla-central/rev/8d6a16456f11

Comment 34

[Axel Hecht [:Pike]]

This seems to cause a lot of crashes, can we back this out?

https://crash-stats.mozilla.com/report/list?product=Firefox&signature=audiounit_property_listener_callback#tab-table and bug 1046231 for details.

Comment 35

[Paul Adenot (:padenot)]

No, I've just landed the fix, I think it'll be in the next nightly.