Closed Bug 1264195 Opened 8 years ago Closed 8 years ago

SetMicrophoneActive() call was lost in GraphDriver landing


(Core :: Audio/Video: MediaStreamGraph, defect, P1)




Tracking Status
firefox46 --- wontfix
firefox47 --- affected
firefox48 --- fixed


(Reporter: jesup, Assigned: jesup)




(3 files)

In Bug 848954 - Part 10 (which actually inserted the GraphDriver into use - initial landing of the GraphDriver), it removed the SetMicrophoneActive() calls from UpdateStreamOrder().  The patch 22 of that bug, it added SetMicrophoneActive() to the Graphdriver code -- but it didn't add the call to it back into MSG.

Thus we've lost the "PanOutput" functionality we added for MacbookPro's to avoid audio coming out the speaker immediately underneath the microphone. In a AEC trace in speakerphone mode, I saw an echo of -5 to -10 db with maybe 50-70% volume - and an output signal of -5 db.  When I talked, the mic saw it as about -15-20 db, so the echo was at least 8-10x stronger than my speaking - and volume was well below max.  When I manually panned to the right speaker, the echo went to -25-35db, or a good 20-25db lower raw echo, or perhaps 100-150x lower volume.
Assignee: nobody → rjesup
Rank: 10
This patch only works in full-duplex mode; it may need to be tied to the mixer callbacks as it was before GraphDriver landed, at least if full_duplex is off
untested patch to maintain mMicrophoneActive across multiple drivers; should work for non-full-duplex unless there's a problem calling PanOutput before we Start() the driver (there may be - if so, we can defer until after Start)
Attachment #8740801 - Flags: feedback?(padenot)
Attachment #8740808 - Flags: feedback?(padenot)
Comment on attachment 8740801 [details] [diff] [review]
WIP patch to add SetMicrophoneActive

Review of attachment 8740801 [details] [diff] [review]:

::: dom/media/GraphDriver.cpp
@@ +659,5 @@
>        NextDriver()->Start();
>        return;
>      }
>    }
> +  SetMicrophoneActive(mGraphImpl->mInputWanted);

Hrm why are a bunch of attributes of the MSGImpl public? In any case, this should be a method that asserts that you're on the right thread.
Attachment #8740801 - Flags: feedback?(padenot)
Comment on attachment 8740808 [details] [diff] [review]
WIP patch to add SetMicrophoneActive for non-full_duplex

Review of attachment 8740808 [details] [diff] [review]:

::: dom/media/GraphDriver.cpp
@@ +59,5 @@
>      mWaitState(WAITSTATE_RUNNING),
>      mCurrentTimeStamp(TimeStamp::Now()),
>      mPreviousDriver(nullptr),
> +    mNextDriver(nullptr),
> +    mMicrophoneActive(false)

I'd prefer querying the graph state each time, duplicating state leads to errors.

@@ +1104,5 @@
> +GraphDriver::SetMicrophoneActive(bool aActive) {
> +  MonitorAutoLock mon(mGraphImpl->GetMonitor());
> +
> +  mMicrophoneActive = aActive;
> +}

Can we just get the state from the graph to when we're ::Init-ing an AudioCallbackDriver ? I think that would be less code and would work.
Attachment #8740808 - Flags: feedback?(padenot)
Updated (simpler) patch to use the far-end-observer state to know if we should pan or not.  Added a graph-thread assertion as well.  Also made the MicrophoneActive code mac-only since nothing else (currently) cares.  We may be able to avoid the device-changed stuff if full-duplex is on, but that's a separate issue
Attachment #8740979 - Flags: review?(padenot)
Attachment #8740979 - Flags: review?(padenot) → review+
Blocks: 848954
OS: Unspecified → Mac OS X
Backed out for "/home/worker/workspace/build/src/dom/media/GraphDriver.h:536:8: error: private field 'mMicrophoneActive' is not used [-Werror,-Wunused-private-field]"


Push with failure:
Failure log:
Flags: needinfo?(rjesup)
Flags: needinfo?(rjesup)
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.