Closed Bug 1447982 Opened 3 years ago Closed 3 years ago

getSettings for microphone broken after applyConstraints makes changes

Categories

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

Product:
Core
Component:
WebRTC: Audio/Video
Version:
60 Branch
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox59 --- unaffected
firefox60 + verified
firefox61 + verified

People

(Reporter: pehrsons, Assigned: pehrsons)

References

Details

(Keywords: regression)

Attachments

(12 files)

Bug 1447982 - Remove unnecessary conditional.
59 bytes, text/x-review-board-request
padenot
: review+
Details
Bug 1447982 - Guard against destroyed stream.
59 bytes, text/x-review-board-request
padenot
: review+
Details
Bug 1447982 - Cover channel count changes also when mic source is stopped (muted).
59 bytes, text/x-review-board-request
padenot
: review+
Details
Bug 1447982 - Move mic source allocation logging so it actually covers allocations.
59 bytes, text/x-review-board-request
padenot
: review+
Details
Bug 1447982 - Mic source's ApplySettings shouldn't have weird side effects.
59 bytes, text/x-review-board-request
padenot
: review+
Details
Bug 1447982 - Apply and update mic source settings on Reconfigure.
59 bytes, text/x-review-board-request
padenot
: review+
Details
Bug 1447982 - Remove unused MediaEnginePrefs from AllocationHandle.
59 bytes, text/x-review-board-request
padenot
: review+
Details
Bug 1447982 - Pass GraphImpl into ApplySettings.
59 bytes, text/x-review-board-request
padenot
: review+
Details
Bug 1447982 - Remove early exit when no change on Reconfigure.
59 bytes, text/x-review-board-request
padenot
: review+
Details
Bug 1447982 - Rename mLastPrefs to mNetPrefs and update comment for clarity.
59 bytes, text/x-review-board-request
padenot
: review+
Details
Bug 1447982 - Add mochitest for audio constraints.
59 bytes, text/x-review-board-request
jib
: review+
Details
[beta 60] Squashed fixes
7.92 KB, patch
jcristau
: approval-mozilla-beta+
Details | Diff | Splinter Review 
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().
Rank: 11
Priority: -- → P2
Comment on attachment 8962311 [details]
Bug 1447982 - Remove unnecessary conditional.

https://reviewboard.mozilla.org/r/231150/#review237270
Attachment #8962311 - Flags: review?(padenot) → review+
Comment on attachment 8962312 [details]
Bug 1447982 - Guard against destroyed stream.

https://reviewboard.mozilla.org/r/231152/#review237272
Attachment #8962312 - Flags: review?(padenot) → review+
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
Attachment #8962313 - Flags: review?(padenot) → review+
Comment on attachment 8962314 [details]
Bug 1447982 - Move mic source allocation logging so it actually covers allocations.

https://reviewboard.mozilla.org/r/231156/#review237278
Attachment #8962314 - Flags: review?(padenot) → review+
Comment on attachment 8962315 [details]
Bug 1447982 - Mic source's ApplySettings shouldn't have weird side effects.

https://reviewboard.mozilla.org/r/231158/#review237280
Attachment #8962315 - Flags: review?(padenot) → review+
Comment on attachment 8962316 [details]
Bug 1447982 - Apply and update mic source settings on Reconfigure.

https://reviewboard.mozilla.org/r/231160/#review237284
Attachment #8962316 - Flags: review?(padenot) → review+
Comment on attachment 8962317 [details]
Bug 1447982 - Remove unused MediaEnginePrefs from AllocationHandle.

https://reviewboard.mozilla.org/r/231162/#review237286
Attachment #8962317 - Flags: review?(padenot) → review+
Comment on attachment 8962318 [details]
Bug 1447982 - Pass GraphImpl into ApplySettings.

https://reviewboard.mozilla.org/r/231164/#review237290
Attachment #8962318 - Flags: review?(padenot) → review+
Comment on attachment 8962319 [details]
Bug 1447982 - Remove early exit when no change on Reconfigure.

https://reviewboard.mozilla.org/r/231166/#review237292
Attachment #8962319 - Flags: review?(padenot) → review+
Comment on attachment 8962320 [details]
Bug 1447982 - Rename mLastPrefs to mNetPrefs and update comment for clarity.

https://reviewboard.mozilla.org/r/231168/#review237294
Attachment #8962320 - Flags: review?(padenot) → review+
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!
Flags: needinfo?(apehrson)
Priority: P2 → P1
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.
Flags: needinfo?(apehrson)
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
Attachment #8966995 - Flags: review?(jib) → review+
Pushed by pehrsons@gmail.com:
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
See Also: → 1453805
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].
Attachment #8967721 - Flags: approval-mozilla-beta?
Comment on attachment 8967721 [details] [diff] [review]
[beta 60] Squashed fixes

fix webrtc regression in fx60, approved for 60.0b13
Attachment #8967721 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(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)?
Flags: needinfo?(apehrson) → needinfo?(btara)
Flags: needinfo?(btara)
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.
Status: RESOLVED → VERIFIED
status-firefox60: fixed → verified
status-firefox61: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.