Closed Bug 1027713 Opened 6 years ago Closed 5 years ago

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

Categories

(Core :: Audio/Video, defect)

x86_64
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: jesup, Assigned: padenot)

References

(Depends on 1 open bug, Blocks 2 open bugs)

Details

Attachments

(5 files, 9 obsolete files)

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
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.)
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
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)
(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.
(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.
(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.
Attached patch Add a volume API in cubeb. r= (obsolete) — Splinter Review
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.
Attached patch Add a volume API in cubeb. r= (obsolete) — Splinter Review
Attachment #8456230 - Flags: review?(kinetik)
Attachment #8454537 - Attachment is obsolete: true
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)
Attachment #8454538 - Attachment is obsolete: true
The reentrant mutex is needed so that users can call back into cubeb's API from
the callback.
Attachment #8456233 - Flags: review?(kinetik)
Attachment #8454539 - Attachment is obsolete: true
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)
Attachment #8454540 - Attachment is obsolete: true
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+
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)
Attachment #8456232 - Flags: review?(kinetik)
Attachment #8456230 - Flags: review?(kinetik)
Attached patch Add a volume API in cubeb. r= (obsolete) — Splinter Review
Attachment #8458786 - Flags: review?(kinetik)
Attachment #8456230 - Attachment is obsolete: true
Attachment #8456232 - Attachment is obsolete: true
The reentrant mutex is needed so that users can call back into cubeb's API from
the callback.
Attachment #8458788 - Flags: review?(kinetik)
Attachment #8456233 - Attachment is obsolete: true
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+
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)
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+
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)
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+
Depends on: 1045018
Depends on: 1046231
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
Depends on: 1426722
You need to log in before you can comment on or make changes to this bug.