Closed Bug 1311911 Opened 3 years ago Closed 3 years ago

1.5 second delay on Logitech c920 microphone in OSX (regression)

Categories

(Core :: Audio/Video: cubeb, defect, P1, major)

50 Branch
x86_64
macOS
defect

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox49 --- unaffected
firefox50 --- disabled
firefox51 + fixed
firefox52 - fixed

People

(Reporter: jib, Assigned: achronop)

References

()

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

STR:
 1. In FF50 on OSX, insert a Logitech c920 camera (or possibly others) into USB.
 2. Open URL.
 3. In permission prompt, share "HD PRO Webcam C920"
 4. Tap fingernail on camera.

Expected result:
- Tapping sound is almost instantaneous.

Actual result:
- Tapping sound is delayed by a constant ~1.5 seconds.
[Tracking Requested - why for this release]:
Caused by enabling full duplex:

20:07.53 INFO: Last good revision: 68a682ef9e59eabb5d0f1fda05e84a773ca8340b
20:07.53 INFO: First bad revision: 7f2f44eaea09c02079d8f1a0be777cd39776712e
20:07.53 INFO: Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=68a682ef9e59eabb5d0f1fda05e84a773ca8340b&tochange=7f2f44eaea09c02079d8f1a0be777cd39776712e
Hi :jesup,
Can you help take a look at this one?
Flags: needinfo?(rjesup)
I believe that's mine, the affected versions are versions that full duplex pref is on. I can retest with C930. I will post the results.
Flags: needinfo?(rjesup) → needinfo?(achronop)
I cannot repro using Logitech C930. Do you think it's a model issue? Can you repro with other equipment like Plantronics headset?
Flags: needinfo?(achronop)
Sorry don't have one of those.
Confirmed on a c920.  Note that c920 is odd and ends up using 16KHz input
I got a c920 and I am able to repro. No guess right now what is the root cause. I will investigate further.
We've disabled (pref'd off) full duplex in Fx 50; so this bug no longer exists in Fx 50.
Component: WebRTC: Audio/Video → Audio/Video: cubeb
Track 51+ as regression.

Hi :mreavy,
Per comment #9, this will be enabled in 51, can you help find an owner for this?
Flags: needinfo?(mreavy)
Assignee: nobody → achronop
I am working on this. I have a fix up for review in the following pull request (among other things):

https://github.com/kinetiknz/cubeb/pull/179
Flags: needinfo?(mreavy)
Depends on: 1315928
Landed as part of Bug 1315928
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Tracking 52- since this resolved fixed.
Attached patch bug-1311911-delay (obsolete) — Splinter Review
Approval Request Comment
[Feature/regressing bug #]: Bug 1311911

[User impact if declined]: When Logitech c920 mic is used has ~2sec delay.

[Describe test coverage new/current, TreeHerder]: Manual testing: gUM by sharing Logitech c920 as external mic.

[Risks and why]: Low risk; Small fix that operates already in Nightly with no reported issues.

[String/UUID change made/needed]: none
Attachment #8809840 - Flags: review?(rjesup)
Attachment #8809840 - Flags: approval-mozilla-aurora?
Comment on attachment 8809840 [details] [diff] [review]
bug-1311911-delay

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

::: media/libcubeb/src/cubeb_audiounit.cpp
@@ +375,5 @@
>    if (stm->input_unit != NULL) {
>      /* Output callback came first */
>      if (stm->frames_read == 0) {
> +      uint32_t min_input_frames_required = ceilf(stm->input_hw_rate / stm->output_hw_rate *
> +                                                                      stm->input_buffer_frames);

Nit: parenthesize when using / and * in a sequence, to make it easier to read without having to think about order-of-operations and precedence.
Attachment #8809840 - Flags: review?(rjesup) → review+
That's a valid comment, I will leave it as is in order to be the same with central and cubeb upstream. I will change it there on next cubeb update.
Please remember to add a .patch and modify update.sh before checking this in.
Comment on attachment 8809840 [details] [diff] [review]
bug-1311911-delay

Clearing approval until the patch is updated, otherwise the wrong patch may land automatically.
Attachment #8809840 - Flags: approval-mozilla-aurora?
Flags: needinfo?(achronop)
Assignee: achronop → rjesup
Comment on attachment 8809955 [details] [diff] [review]
Calculate silence frames needed in the input

Added patch to update.sh
Carrying forward r+
Attachment #8809955 - Flags: review+
Attachment #8809955 - Flags: approval-mozilla-aurora?
Attachment #8809840 - Attachment is obsolete: true
Thanks for taking care, I will remember next time.
Flags: needinfo?(achronop)
Comment on attachment 8809955 [details] [diff] [review]
Calculate silence frames needed in the input

Fix a regression. Aurora51+.
Attachment #8809955 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
has conflicts

file media/libcubeb/bug1311911_logitech_delay.patch already exists
1 out of 1 hunks FAILED -- saving rejects to file media/libcubeb/bug1311911_logitech_delay.patch.rej
patching file media/libcubeb/src/cubeb_audiounit.cpp
Hunk #1 FAILED at 159
Hunk #2 FAILED at 367
Hunk #3 FAILED at 1151
Hunk #4 FAILED at 1251
Hunk #5 FAILED at 1273
5 out of 5 hunks FAILED -- saving rejects to file media/libcubeb/src/cubeb_audiounit.cpp.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh bug-1311911-delay.txt
Flags: needinfo?(rjesup)
I do not know how but patch is already landed:

changeset:   341014:950df3cfb779
user:        Alex Chronopoulos <achronop@gmail.com>
date:        Sat Nov 12 23:18:18 2016 -0500
summary:     Bug 1311911 - Calculate silence frames needed in the input. r=jesup a=gchang

Whoever landed it probably forgot to copy back the revision link.
yeah jesup landed this in https://hg.mozilla.org/releases/mozilla-aurora/rev/950df3cfb7798d1c6b1f422457a0e398a2b74587

maybe he was thinking pulsebot does the job here :) but pulsebot only comment on integration +m-c :)
Flags: needinfo?(rjesup)
Missed this one, sorry. (was landing a lot of Aurora uplifts over the weekend)
Assignee: rjesup → achronop
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.