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

RESOLVED FIXED in mozilla34

Status

()

Core
Audio/Video
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jesup, Assigned: padenot)

Tracking

(Blocks: 2 bugs)

Trunk
mozilla34
x86_64
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 9 obsolete attachments)

7.51 KB, patch
kinetik
: review+
Details | Diff | Splinter Review
13.36 KB, patch
kinetik
: review+
Details | Diff | Splinter Review
10.03 KB, patch
Details | Diff | Splinter Review
40.29 KB, patch
kinetik
: review+
Details | Diff | Splinter Review
3.12 KB, patch
jesup
: review+
Details | Diff | Splinter Review
(Reporter)

Description

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

Updated

3 years ago
Summary: When microphone is active, pan all output audio to right speaker on MacBookPro's → When microphone is active, pan all output audio to right speaker on MacBookPros
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
Assignee: nobody → kinetik
Status: NEW → ASSIGNED
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.
Status: ASSIGNED → NEW
Assignee: kinetik → nobody
(Assignee)

Comment 3

3 years ago
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)
Flags: needinfo?(kinetik)
Thanks, Paul!
Assignee: nobody → paul
(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.
Flags: needinfo?(kinetik)
(Reporter)

Comment 6

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

Comment 7

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

Comment 8

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

Comment 9

3 years ago
Created attachment 8454537 [details] [diff] [review]
Add a volume API in cubeb. r=
(Assignee)

Comment 10

3 years ago
Created attachment 8454538 [details] [diff] [review]
Add a cubeb API to query the name of the audio output device in use. r=
(Assignee)

Comment 11

3 years ago
Created attachment 8454539 [details] [diff] [review]
Add a cubeb API to signal that the output device changed. r=
(Assignee)

Comment 12

3 years ago
Created attachment 8454540 [details] [diff] [review]
Pan audio to the right when we are using gUM and outputing sound using MacBookPro speakers. r=
(Assignee)

Comment 13

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

Comment 14

3 years ago
Created attachment 8456230 [details] [diff] [review]
Add a volume API in cubeb. r=
Attachment #8456230 - Flags: review?(kinetik)
(Assignee)

Updated

3 years ago
Attachment #8454537 - Attachment is obsolete: true
(Assignee)

Comment 15

3 years ago
Created attachment 8456232 [details] [diff] [review]
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.
Attachment #8456232 - Flags: review?(kinetik)
(Assignee)

Updated

3 years ago
Attachment #8454538 - Attachment is obsolete: true
(Assignee)

Comment 16

3 years ago
Created attachment 8456233 [details] [diff] [review]
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.
Attachment #8456233 - Flags: review?(kinetik)
(Assignee)

Updated

3 years ago
Attachment #8454539 - Attachment is obsolete: true
(Assignee)

Comment 17

3 years ago
Created attachment 8456234 [details] [diff] [review]
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.
Attachment #8456234 - Flags: review?(rjesup)
(Assignee)

Updated

3 years ago
Attachment #8454540 - Attachment is obsolete: true
(Reporter)

Comment 18

3 years ago
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
Attachment #8456234 - Flags: review?(rjesup) → review+
(Assignee)

Comment 19

3 years ago
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.
Attachment #8456233 - Flags: review?(kinetik)
(Assignee)

Updated

3 years ago
Attachment #8456232 - Flags: review?(kinetik)
(Assignee)

Updated

3 years ago
Attachment #8456230 - Flags: review?(kinetik)
(Assignee)

Comment 20

3 years ago
Created attachment 8458786 [details] [diff] [review]
Add a volume API in cubeb. r=
Attachment #8458786 - Flags: review?(kinetik)
(Assignee)

Updated

3 years ago
Attachment #8456230 - Attachment is obsolete: true
(Assignee)

Comment 21

3 years ago
Created attachment 8458787 [details] [diff] [review]
Add a cubeb API to query the name of the audio output device in use. r=
Attachment #8458787 - Flags: review?(kinetik)
(Assignee)

Updated

3 years ago
Attachment #8456232 - Attachment is obsolete: true
(Assignee)

Comment 22

3 years ago
Created attachment 8458788 [details] [diff] [review]
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.
Attachment #8458788 - Flags: review?(kinetik)
(Assignee)

Updated

3 years ago
Attachment #8456233 - Attachment is obsolete: true
(Assignee)

Comment 23

3 years ago
Created attachment 8458792 [details] [diff] [review]
Pan audio to the right when we are using gUM and outputing sound using MacBookPro speakers. r=

Addressed review comments.
(Assignee)

Updated

3 years ago
Attachment #8456234 - Attachment is obsolete: true
Attachment #8458787 - Flags: review?(kinetik) → review+
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.
Attachment #8458788 - Flags: review?(kinetik) → review+
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?
Attachment #8458786 - Flags: review?(kinetik) → review+
(Assignee)

Comment 26

3 years ago
Created attachment 8459345 [details] [diff] [review]
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).
Attachment #8459345 - Flags: review?(kinetik)
(Assignee)

Updated

3 years ago
Attachment #8458786 - Attachment is obsolete: true
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...
Attachment #8459345 - Flags: review?(kinetik) → review+
(Assignee)

Comment 28

3 years ago
Created attachment 8460181 [details] [diff] [review]
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.
Attachment #8460181 - Flags: review?(rjesup)
(Reporter)

Comment 29

3 years ago
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
Attachment #8460181 - Flags: review?(rjesup) → review+
(Assignee)

Comment 30

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

Updated

3 years ago
Depends on: 1045018
(Assignee)

Comment 32

3 years ago
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)
Flags: needinfo?(paul)
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
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Depends on: 1046231

Comment 34

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

Comment 35

3 years ago
No, I've just landed the fix, I think it'll be in the next nightly.
Depends on: 1054176
Depends on: 1092859
Depends on: 1109802
You need to log in before you can comment on or make changes to this bug.