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

Status: RESOLVED FIXED

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

Description

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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.

Comment 1

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

[Tracking Requested - why for this release]:

Comment 2

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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

Comment 3

[Gerry Chang [:gchang]]

Hi :jesup,
Can you help take a look at this one?

Comment 4

[Alex Chronopoulos [:achronop]]

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.

Comment 5

[Alex Chronopoulos [:achronop]]

I cannot repro using Logitech C930. Do you think it's a model issue? Can you repro with other equipment like Plantronics headset?

Comment 6

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Sorry don't have one of those.

Comment 7

[Randell Jesup [:jesup] (needinfo me)]

Confirmed on a c920.  Note that c920 is odd and ends up using 16KHz input

Comment 8

[Alex Chronopoulos [:achronop]]

I got a c920 and I am able to repro. No guess right now what is the root cause. I will investigate further.

Comment 9

[Maire Reavy [:mreavy]]

We've disabled (pref'd off) full duplex in Fx 50; so this bug no longer exists in Fx 50.

Comment 10

[Gerry Chang [:gchang]]

Track 51+ as regression.

Hi :mreavy,
Per comment #9, this will be enabled in 51, can you help find an owner for this?

Comment 11

[Alex Chronopoulos [: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

Comment 12

[Alex Chronopoulos [:achronop]]

Landed as part of Bug 1315928

Comment 13

[Marcia Knous [:marcia]]

Tracking 52- since this resolved fixed.

Comment 14

[Alex Chronopoulos [:achronop]]

Attachment: bug-1311911-delay

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

Comment 15

[Randell Jesup [:jesup] (needinfo me)]

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.

Comment 16

[Alex Chronopoulos [:achronop]]

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.

Comment 17

[Matthew Gregan [:kinetik]]

Please remember to add a .patch and modify update.sh before checking this in.

Comment 18

[Matthew Gregan [:kinetik]]

Comment on attachment 8809840 [details] [diff] [review]
bug-1311911-delay

Clearing approval until the patch is updated, otherwise the wrong patch may land automatically.

Comment 19

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Calculate silence frames needed in the input

Comment 20

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8809955 [details] [diff] [review]
Calculate silence frames needed in the input

Added patch to update.sh
Carrying forward r+

Comment 21

[Alex Chronopoulos [:achronop]]

Thanks for taking care, I will remember next time.

Comment 22

[Gerry Chang [:gchang]]

Comment on attachment 8809955 [details] [diff] [review]
Calculate silence frames needed in the input

Fix a regression. Aurora51+.

Comment 23

[Carsten Book [:Tomcat]]

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

Comment 24

[Alex Chronopoulos [:achronop]]

I do not know how but patch is already landed:

changeset:   341014:950df3cfb779
user:        Alex Chronopoulos <[e-mail]>
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.

Comment 25

[Carsten Book [:Tomcat]]

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 :)

Comment 26

[Randell Jesup [:jesup] (needinfo me)]

Missed this one, sorry. (was landing a lot of Aurora uplifts over the weekend)