1447982 - getSettings for microphone broken after applyConstraints makes changes

Status: VERIFIED FIXED

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

Description

[Andreas Pehrson [:pehrsons]]

STR
1 Go to https://jsfiddle.net/jib1/n7bmkjnf/
2 Accept the permission prompt
3 Uncheck the "echoCancellation" checkbox

Expected:
Checkbox remains unchecked

Actual:
Checkbox immediately re-checked


Looks like a regression from bug 1440040 when I replaced SetLastPrefs() with ApplySettings().

Comment 1

[Julien Cristau [:jcristau]]

tracking as new regression

Comment 2

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1447982 - Remove unnecessary conditional.

Review commit: https://reviewboard.mozilla.org/r/231150/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/231150/

Comment 3

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1447982 - Guard against destroyed stream.

Review commit: https://reviewboard.mozilla.org/r/231152/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/231152/

Comment 4

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1447982 - Cover channel count changes also when mic source is stopped (muted).

Review commit: https://reviewboard.mozilla.org/r/231154/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/231154/

Comment 5

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1447982 - Move mic source allocation logging so it actually covers allocations.

Review commit: https://reviewboard.mozilla.org/r/231156/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/231156/

Comment 6

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1447982 - Mic source's ApplySettings shouldn't have weird side effects.

Review commit: https://reviewboard.mozilla.org/r/231158/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/231158/

Comment 7

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1447982 - Apply and update mic source settings on Reconfigure.

Review commit: https://reviewboard.mozilla.org/r/231160/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/231160/

Comment 8

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1447982 - Remove unused MediaEnginePrefs from AllocationHandle.

Review commit: https://reviewboard.mozilla.org/r/231162/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/231162/

Comment 9

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1447982 - Pass GraphImpl into ApplySettings.

Review commit: https://reviewboard.mozilla.org/r/231164/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/231164/

Comment 10

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1447982 - Remove early exit when no change on Reconfigure.

We don't want to skip all remaining steps. For now it just affects some logging,
but there may be new ones added in the future.

Review commit: https://reviewboard.mozilla.org/r/231166/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/231166/

Comment 11

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1447982 - Rename mLastPrefs to mNetPrefs and update comment for clarity.

Review commit: https://reviewboard.mozilla.org/r/231168/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/231168/

Comment 12

[Paul Adenot (:padenot)]

Comment on attachment 8962311 [details]
Bug 1447982 - Remove unnecessary conditional.

https://reviewboard.mozilla.org/r/231150/#review237270

Comment 13

[Paul Adenot (:padenot)]

Comment on attachment 8962312 [details]
Bug 1447982 - Guard against destroyed stream.

https://reviewboard.mozilla.org/r/231152/#review237272

Comment 14

[Paul Adenot (:padenot)]

Comment on attachment 8962313 [details]
Bug 1447982 - Cover channel count changes also when mic source is stopped (muted).

https://reviewboard.mozilla.org/r/231154/#review237276

Comment 15

[Paul Adenot (:padenot)]

Comment on attachment 8962314 [details]
Bug 1447982 - Move mic source allocation logging so it actually covers allocations.

https://reviewboard.mozilla.org/r/231156/#review237278

Comment 16

[Paul Adenot (:padenot)]

Comment on attachment 8962315 [details]
Bug 1447982 - Mic source's ApplySettings shouldn't have weird side effects.

https://reviewboard.mozilla.org/r/231158/#review237280

Comment 17

[Paul Adenot (:padenot)]

Comment on attachment 8962316 [details]
Bug 1447982 - Apply and update mic source settings on Reconfigure.

https://reviewboard.mozilla.org/r/231160/#review237284

Comment 18

[Paul Adenot (:padenot)]

Comment on attachment 8962317 [details]
Bug 1447982 - Remove unused MediaEnginePrefs from AllocationHandle.

https://reviewboard.mozilla.org/r/231162/#review237286

Comment 19

[Paul Adenot (:padenot)]

Comment on attachment 8962318 [details]
Bug 1447982 - Pass GraphImpl into ApplySettings.

https://reviewboard.mozilla.org/r/231164/#review237290

Comment 20

[Paul Adenot (:padenot)]

Comment on attachment 8962319 [details]
Bug 1447982 - Remove early exit when no change on Reconfigure.

https://reviewboard.mozilla.org/r/231166/#review237292

Comment 21

[Paul Adenot (:padenot)]

Comment on attachment 8962320 [details]
Bug 1447982 - Rename mLastPrefs to mNetPrefs and update comment for clarity.

https://reviewboard.mozilla.org/r/231168/#review237294

Comment 22

[Paul Adenot (:padenot)]

Can you write the test that would have prevented regressing this? It should be rather straightforward (a pair of `applyConstraint`/`getConstraints`, using constraints that cannot fail (AEC, AGC, NS...).

Thanks!

Comment 23

[Andreas Pehrson [:pehrsons]]

Yes, the plan here is to write the test prior to landing so we're sure it gets done. It should be sufficient to uplift two of these patches so I plan to squash them into one beta/uplift patch as soon as this lands.

For now I have more pressing issues but this is high on the list.

Comment 24

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1447982 - Add mochitest for audio constraints.

Review commit: https://reviewboard.mozilla.org/r/235668/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/235668/

Comment 25

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8962311 [details]
Bug 1447982 - Remove unnecessary conditional.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/231150/diff/1-2/

Comment 26

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8962312 [details]
Bug 1447982 - Guard against destroyed stream.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/231152/diff/1-2/

Comment 27

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8962313 [details]
Bug 1447982 - Cover channel count changes also when mic source is stopped (muted).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/231154/diff/1-2/

Comment 28

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8962314 [details]
Bug 1447982 - Move mic source allocation logging so it actually covers allocations.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/231156/diff/1-2/

Comment 29

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8962315 [details]
Bug 1447982 - Mic source's ApplySettings shouldn't have weird side effects.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/231158/diff/1-2/

Comment 30

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8962316 [details]
Bug 1447982 - Apply and update mic source settings on Reconfigure.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/231160/diff/1-2/

Comment 31

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8962317 [details]
Bug 1447982 - Remove unused MediaEnginePrefs from AllocationHandle.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/231162/diff/1-2/

Comment 32

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8962318 [details]
Bug 1447982 - Pass GraphImpl into ApplySettings.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/231164/diff/1-2/

Comment 33

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8962319 [details]
Bug 1447982 - Remove early exit when no change on Reconfigure.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/231166/diff/1-2/

Comment 34

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8962320 [details]
Bug 1447982 - Rename mLastPrefs to mNetPrefs and update comment for clarity.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/231168/diff/1-2/

Comment 35

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

Comment on attachment 8966995 [details]
Bug 1447982 - Add mochitest for audio constraints.

https://reviewboard.mozilla.org/r/235668/#review241584

::: dom/media/tests/mochitest/test_getUserMedia_audioConstraints.html:11
(Diff revision 1)
> +  createHTML({
> +    title: "Test that microphone getSettings report correct settings after applyConstraints",
> +    bug: "1447982",
> +  });
> +
> +  function testTrackAgainstAudioConstraints(track, audioConstraints) {

Nit: Should we be indenting top-level functions?

::: dom/media/tests/mochitest/test_getUserMedia_audioConstraints.html:63
(Diff revision 1)
> +    let stream = await getUserMedia({
> +      audio: { autoGainControl: true
> +             , echoCancellation: true
> +             , noiseSuppression: true
> +             },
> +    });

Mozilla code style is {
  autoGainControl: false,
  echoCancellation: true,
  noiseSuppression: true
}

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#JavaScript_objects

::: dom/media/tests/mochitest/test_getUserMedia_audioConstraints.html:70
(Diff revision 1)
> +    await testAudioConstraints(track, { autoGainControl: false
> +                                      , echoCancellation: true
> +                                      , noiseSuppression: true
> +                                      });

I'm supportive of repetition in tests, but here I worry about maintenance. I had to strain to see if all combinations were covered, and the risk of silent typos in dictionaries is high.

Instead of relying on unusual indent for readability, why not put the data into an array?

I wrote a similar test long ago that never landed FWIW. Feel free to crib (you might have hit return again in the url bar for Firefox to jump to the correct anchor here. sigh Firefox):

https://reviewboard.mozilla.org/r/65378/diff/15/#3

Comment 36

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8966995 [details]
Bug 1447982 - Add mochitest for audio constraints.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/235668/diff/1-2/

Comment 37

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8962311 [details]
Bug 1447982 - Remove unnecessary conditional.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/231150/diff/2-3/

Comment 38

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8962312 [details]
Bug 1447982 - Guard against destroyed stream.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/231152/diff/2-3/

Comment 39

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8962313 [details]
Bug 1447982 - Cover channel count changes also when mic source is stopped (muted).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/231154/diff/2-3/

Comment 40

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8962314 [details]
Bug 1447982 - Move mic source allocation logging so it actually covers allocations.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/231156/diff/2-3/

Comment 41

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8962315 [details]
Bug 1447982 - Mic source's ApplySettings shouldn't have weird side effects.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/231158/diff/2-3/

Comment 42

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8962316 [details]
Bug 1447982 - Apply and update mic source settings on Reconfigure.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/231160/diff/2-3/

Comment 43

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8962317 [details]
Bug 1447982 - Remove unused MediaEnginePrefs from AllocationHandle.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/231162/diff/2-3/

Comment 44

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8962318 [details]
Bug 1447982 - Pass GraphImpl into ApplySettings.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/231164/diff/2-3/

Comment 45

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8962319 [details]
Bug 1447982 - Remove early exit when no change on Reconfigure.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/231166/diff/2-3/

Comment 46

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8962320 [details]
Bug 1447982 - Rename mLastPrefs to mNetPrefs and update comment for clarity.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/231168/diff/2-3/

Comment 47

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7d36baa26b68
Add mochitest for audio constraints. r=jib
https://hg.mozilla.org/integration/autoland/rev/b9533a2015d5
Remove unnecessary conditional. r=padenot
https://hg.mozilla.org/integration/autoland/rev/509e08753980
Guard against destroyed stream. r=padenot
https://hg.mozilla.org/integration/autoland/rev/5ce4dd0d0ca5
Cover channel count changes also when mic source is stopped (muted). r=padenot
https://hg.mozilla.org/integration/autoland/rev/1e59416d1a77
Move mic source allocation logging so it actually covers allocations. r=padenot
https://hg.mozilla.org/integration/autoland/rev/5180d718e5a5
Mic source's ApplySettings shouldn't have weird side effects. r=padenot
https://hg.mozilla.org/integration/autoland/rev/e87afd1fda35
Apply and update mic source settings on Reconfigure. r=padenot
https://hg.mozilla.org/integration/autoland/rev/01a47840071b
Remove unused MediaEnginePrefs from AllocationHandle. r=padenot
https://hg.mozilla.org/integration/autoland/rev/19e183125269
Pass GraphImpl into ApplySettings. r=padenot
https://hg.mozilla.org/integration/autoland/rev/3f68ebab5039
Remove early exit when no change on Reconfigure. r=padenot
https://hg.mozilla.org/integration/autoland/rev/a67e5015e456
Rename mLastPrefs to mNetPrefs and update comment for clarity. r=padenot

Comment 48

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/7d36baa26b68
https://hg.mozilla.org/mozilla-central/rev/b9533a2015d5
https://hg.mozilla.org/mozilla-central/rev/509e08753980
https://hg.mozilla.org/mozilla-central/rev/5ce4dd0d0ca5
https://hg.mozilla.org/mozilla-central/rev/1e59416d1a77
https://hg.mozilla.org/mozilla-central/rev/5180d718e5a5
https://hg.mozilla.org/mozilla-central/rev/e87afd1fda35
https://hg.mozilla.org/mozilla-central/rev/01a47840071b
https://hg.mozilla.org/mozilla-central/rev/19e183125269
https://hg.mozilla.org/mozilla-central/rev/3f68ebab5039
https://hg.mozilla.org/mozilla-central/rev/a67e5015e456

Comment 49

[Andreas Pehrson [:pehrsons]]

Attachment: [beta 60] Squashed fixes

Approval Request Comment
[Feature/Bug causing the regression]: bug 1440040
[User impact if declined]: Websites trying to use microphone constraints may fail.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: No
[Needs manual test from QE? If yes, steps to reproduce]: Yes, see comment 0
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: It essentially just adds a call to update settings on main thread where appropriate. There are some other changes too but they're trivial and doesn't touch any logic.
[String changes made/needed]: None

Remember to include the test in the uplift too, attachment 8966995 [details].

Comment 50

[Julien Cristau [:jcristau]]

Comment on attachment 8967721 [details] [diff] [review]
[beta 60] Squashed fixes

fix webrtc regression in fx60, approved for 60.0b13

Comment 51

[Bogdan Tara[:bogdan_tara | bogdant]]

Can't uplift. Conflict with https://bugzilla.mozilla.org/show_bug.cgi?id=1438134 .
Conflicting changeset: https://hg.mozilla.org/integration/autoland/rev/e87afd1fda35
https://hg.mozilla.org/mozilla-central/rev/44e300270943#l3.30

Comment 52

[Andreas Pehrson [:pehrsons]]

(In reply to Bogdan Tara[:bogdan_tara] from comment #51)
> Can't uplift. Conflict with
> https://bugzilla.mozilla.org/show_bug.cgi?id=1438134 .
> Conflicting changeset:
> https://hg.mozilla.org/integration/autoland/rev/e87afd1fda35
> https://hg.mozilla.org/mozilla-central/rev/44e300270943#l3.30

Mine went clean on freshly pulled beta just now.

Can you confirm whether you tried to uplift the test (attachment 8966995 [details]) and the squashed beta patch (attachment 8967721 [details] [diff] [review]) only, or perhaps some of the original m-c patches too?

Are you uplifting some other bugs at the same time that end up conflicting (that I don't have)?

Comment 53

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/9e8eacca6e47
https://hg.mozilla.org/releases/mozilla-beta/rev/15079c3a4d0f

Comment 54

[Anca Soncutean, Desktop QA]

I reproduced this issue on Firefox 61.0a1 (20180322220118) under Windows 10 x64.
The issue is fixed on Firefox 61.0a1 (20180417225505) and Firefox 60.0b13 (20180416175245) under Windows 10 x64, Ubuntu 16.04 x32 and macOS 10.13.