Closed Bug 1590579 Opened 1 year ago Closed 1 year ago

Audio competing fails on certain situation

Categories

(Core :: Audio/Video: Playback, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla72
Tracking Status
firefox-esr68 --- unaffected
firefox67 --- unaffected
firefox68 --- unaffected
firefox69 --- unaffected
firefox70 --- unaffected
firefox71 --- fixed
firefox72 --- fixed

People

(Reporter: alwu, Assigned: alwu)

References

(Blocks 1 open bug, Regression)

Details

(Keywords: regression)

Attachments

(2 files)

Bug 1578615 introduced a regression, which causes audio competing failed on centain situation.


STR.

  1. turn the pref media.audioFocus.management on
  2. play any music from tab1
  3. play any other music from tab2
  4. tab1 should be stopped when started tab2
  5. start music from tab1 again

Expected.
6. tab2 should be stopped

Actual.
6. tab2 is still playing, two tab are playing together

--

In bug 1578615, we have separated delaying media playback logic from the normal audio channel usage in HTMLMediaElement, so in theory other audio channel agent owners won't need to handle with such a complex detail hiding in the delay autoplay. However, AudibleState is something we created for delaying autoplay, because we introduced a new value eMaybeAudible, which would only be used in delaying autoplay.

In this context, AudibleState is also something we should eliminate from other usage. Therefore, we should use boolean to replace it and then create a new method to handle delaying autoplay sepecific logic.

If we do so, when other audio channel agent owners are calling methods such as notifyStartedPlaying() and notifyStartedAudible(), they can simply sent a boolean, and don't need to figure out what AudibleState is.

Regressed by: 1578615
No longer regressed by: 1580659

(In reply to Alastor Wu [:alwu] from comment #0)

In bug 1578615, we have separated delaying media playback logic from the normal audio channel usage in HTMLMediaElement, so in theory other audio channel agent owners won't need to handle with such a complex detail hiding in the delay autoplay. However, AudibleState is something we created for delaying autoplay, because we introduced a new value eMaybeAudible, which would only be used in delaying autoplay.

In this context, AudibleState is also something we should eliminate from other usage. Therefore, we should use boolean to replace it and then create a new method to handle delaying autoplay sepecific logic.

If we do so, when other audio channel agent owners are calling methods such as notifyStartedPlaying() and notifyStartedAudible(), they can simply sent a boolean, and don't need to figure out what AudibleState is.

Ah, I forgot that we have already had bug1583978 to remove eMaybeAudible. So I would only solve the audio competing issue in this bug and leave remaining work in bug1583978.

Add a test case to ensure that audio competiting could work multiple times when we play media again and again from different tabs.

Everytime when we start a controller, we should also update its audible state correctly, in order to request audio focus correctly.

Pushed by alwu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e8517c1bc0d7
part1 : add test. r=chunmin
https://hg.mozilla.org/integration/autoland/rev/e41130821190
part2 : update controller's audible state when it starts. r=chunmin
Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla72

Is this something we should consider uplifting to Beta or can this ride Fx72 to release?

Flags: needinfo?(alwu)
Flags: in-testsuite+

Comment on attachment 9103422 [details]
Bug 1590579 - part2 : update controller's audible state when it starts.

Beta/Release Uplift Approval Request

  • User impact if declined: The audio competing would fail on certain situation, then user would hear multiple sound playing and mixing together, it's kind of a mess. Although the audio focus management is of by default on desktop, but it's on by default on mobile.
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: No
  • Needs manual test from QE?: No
  • If yes, steps to reproduce: Follow the instruction in comment0.
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): This feature is default off on desktop and we have also added an automation test for it.
  • String changes made/needed: no
Flags: needinfo?(alwu)
Attachment #9103422 - Flags: approval-mozilla-beta?
Attachment #9103421 - Flags: approval-mozilla-beta?

Comment on attachment 9103422 [details]
Bug 1590579 - part2 : update controller's audible state when it starts.

Fix for a 71 audio regression, low risk patch with tests, does not affect desktop but likely to impact our mobile apps, uplift approved for 71 beta 6, thanks.

Attachment #9103422 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9103421 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.