Closed Bug 1245216 Opened 8 years ago Closed 8 years ago

full_duplex getUserMedia audio wasn't getting properly plumbed through to MediaStreams

Categories

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

Product:
Core
Component:
WebRTC: Audio/Video
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla47
Tracking Flags:
Tracking Status
firefox47 --- fixed

People

(Reporter: jesup, Assigned: jesup)

Details

Attachments

(4 files)

MozReview Request: Bug 1245216: plumb preferred sample rate from full_duplex cubeb through NotifyInput/Output r?padenot
58 bytes, text/x-review-board-request
padenot
: review+
Details
MozReview Request: Bug 1245216: Fix getUserMedia input in full_duplex mode coming from the wrong place r?padenot
58 bytes, text/x-review-board-request
padenot
: review+
Details
MozReview Request: Bug 1245216: white-list the fake 440Hz audio source used in automation for getUserMedia enumeration r?padenot
58 bytes, text/x-review-board-request
padenot
: review+
Details
MozReview Request: Bug 1245216: Avoid reallocating and leaking AudioPacketizer output buffer r?padenot
58 bytes, text/x-review-board-request
padenot
: review+
Details 
Due to two functions in webrtc.org being executed in the wrong order, the webrtc.org capture code was still getting invoked.

Also, while dealing with this, I found some rough edges and bugs, in particular the handling of the capture rate/channel configuration.  Also there was a leftover overridden implementation of SetInputListener/etc.

With these resolved, it appears to be working correctly when enabled.
Rank: 10
Also cleanup of an leftover overrridden interface, and re-add a line lost in merges

Review commit: https://reviewboard.mozilla.org/r/33279/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/33279/
Attachment #8714957 - Flags: review?(padenot)
https://reviewboard.mozilla.org/r/33279/#review30017

::: dom/media/MediaStreamGraph.cpp:947
(Diff revision 1)
> +    driver->SetInputListener(aListener);

Note: this line appeared to have gotten lost in a merge somewhere.
Comment on attachment 8714956 [details]
MozReview Request: Bug 1245216: plumb preferred sample rate from full_duplex cubeb through NotifyInput/Output r?padenot

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33277/diff/1-2/
Comment on attachment 8714957 [details]
MozReview Request: Bug 1245216: Fix getUserMedia input in full_duplex mode coming from the wrong place r?padenot

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33279/diff/1-2/
Comment on attachment 8714958 [details]
MozReview Request: Bug 1245216: white-list the fake 440Hz audio source used in automation for getUserMedia enumeration r?padenot

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33281/diff/1-2/
https://treeherder.mozilla.org/#/jobs?repo=try&revision=44bdfbb3d7b6
(with the leak fix)  The problems with devtools and UI tests (bc*) from the first pref-on attempt seem fixed
Comment on attachment 8714956 [details]
MozReview Request: Bug 1245216: plumb preferred sample rate from full_duplex cubeb through NotifyInput/Output r?padenot

https://reviewboard.mozilla.org/r/33277/#review30233

::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:462
(Diff revision 2)
>     }

nit: indent weirdness here.
Attachment #8714956 - Flags: review?(padenot) → review+
Attachment #8714957 - Flags: review?(padenot) → review+
Comment on attachment 8714957 [details]
MozReview Request: Bug 1245216: Fix getUserMedia input in full_duplex mode coming from the wrong place r?padenot

https://reviewboard.mozilla.org/r/33279/#review30097

See if you can address the comments here. I'm thinking we should try to gater telemetry for multichannel input and output to see if it makes sense to do something. I know some laptops have stereo mics for some kind of internal noise cancelation, but I don't know if only a mono stream is exposed to the OS.

::: dom/media/GraphDriver.h:414
(Diff revision 2)
> +    mAudioInput = aListener;

Asserts are better than comments :-). I'm a bit paranoid with those issues.

::: dom/media/GraphDriver.cpp:931
(Diff revision 2)
> -                                   mSampleRate, ChannelCount);
> +                                   mSampleRate, 1); // match input.channels above

Can you not just save the value on the instance here instead of hard coding ?
Attachment #8714958 - Flags: review?(padenot) → review+
Comment on attachment 8714958 [details]
MozReview Request: Bug 1245216: white-list the fake 440Hz audio source used in automation for getUserMedia enumeration r?padenot

https://reviewboard.mozilla.org/r/33281/#review30239

::: dom/media/webrtc/MediaEngineWebRTC.h:280
(Diff revision 2)
> +            strcmp(devices->device[i]->friendly_name, "Sine source at 440 Hz") == 0)))

Maybe we should file something upstream?
Comment on attachment 8715103 [details]
MozReview Request: Bug 1245216: Avoid reallocating and leaking AudioPacketizer output buffer r?padenot

https://reviewboard.mozilla.org/r/33339/#review30241

::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:475
(Diff revision 1)
> +    mPacketizer->Output(packet);

32KB per second of call, good catch !
Attachment #8715103 - Flags: review?(padenot) → review+
Comment on attachment 8714956 [details]
MozReview Request: Bug 1245216: plumb preferred sample rate from full_duplex cubeb through NotifyInput/Output r?padenot

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33277/diff/2-3/
Comment on attachment 8714957 [details]
MozReview Request: Bug 1245216: Fix getUserMedia input in full_duplex mode coming from the wrong place r?padenot

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33279/diff/2-3/
Comment on attachment 8714958 [details]
MozReview Request: Bug 1245216: white-list the fake 440Hz audio source used in automation for getUserMedia enumeration r?padenot

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33281/diff/2-3/
Comment on attachment 8715103 [details]
MozReview Request: Bug 1245216: Avoid reallocating and leaking AudioPacketizer output buffer r?padenot

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33339/diff/1-2/
https://reviewboard.mozilla.org/r/33281/#review30239

> Maybe we should file something upstream?

Yes we should
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: