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)
Tracking
()
VERIFIED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox59 | --- | unaffected |
firefox60 | + | verified |
firefox61 | + | verified |
People
(Reporter: pehrsons, Assigned: pehrsons)
References
Details
(Keywords: regression)
Attachments
(12 files)
59 bytes,
text/x-review-board-request
|
padenot
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
padenot
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
padenot
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
padenot
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
padenot
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
padenot
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
padenot
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
padenot
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
padenot
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
padenot
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
jib
:
review+
|
Details |
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().
Assignee | ||
Updated•3 years ago
|
Rank: 11
Priority: -- → P2
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 12•3 years ago
|
||
mozreview-review |
Comment on attachment 8962311 [details] Bug 1447982 - Remove unnecessary conditional. https://reviewboard.mozilla.org/r/231150/#review237270
Attachment #8962311 -
Flags: review?(padenot) → review+
Comment 13•3 years ago
|
||
mozreview-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 14•3 years ago
|
||
mozreview-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 15•3 years ago
|
||
mozreview-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 16•3 years ago
|
||
mozreview-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 17•3 years ago
|
||
mozreview-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 18•3 years ago
|
||
mozreview-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 19•3 years ago
|
||
mozreview-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 20•3 years ago
|
||
mozreview-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 21•3 years ago
|
||
mozreview-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+
Comment 22•3 years ago
|
||
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!
![]() |
||
Updated•3 years ago
|
Flags: needinfo?(apehrson)
![]() |
||
Updated•3 years ago
|
Priority: P2 → P1
Assignee | ||
Comment 23•3 years ago
|
||
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 35•3 years ago
|
||
mozreview-review |
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+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 47•3 years ago
|
||
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
Comment 48•3 years ago
|
||
bugherder |
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
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Assignee | ||
Comment 49•3 years ago
|
||
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 50•3 years ago
|
||
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+
Comment 51•3 years ago
|
||
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
Flags: needinfo?(apehrson)
Assignee | ||
Comment 52•3 years ago
|
||
(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)
Updated•3 years ago
|
Flags: needinfo?(btara)
Comment 53•3 years ago
|
||
bugherderuplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/9e8eacca6e47 https://hg.mozilla.org/releases/mozilla-beta/rev/15079c3a4d0f
Flags: in-testsuite+
Updated•3 years ago
|
Flags: qe-verify+
Comment 54•3 years ago
|
||
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
Flags: qe-verify+
You need to log in
before you can comment on or make changes to this bug.
Description
•