Closed Bug 815569 Opened 8 years ago Closed 7 years ago

[Audio] Audio Competing Policy Should Pause Low Priority Streams not Mute

Categories

(Firefox OS Graveyard :: General, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-kilimanjaro:+, blocking-basecamp:+, firefox19 fixed, firefox20 fixed, b2g18 fixed)

RESOLVED FIXED
blocking-kilimanjaro +
blocking-basecamp +
Tracking Status
firefox19 --- fixed
firefox20 --- fixed
b2g18 --- fixed

People

(Reporter: mchen, Assigned: kinetik)

References

Details

Attachments

(1 file, 2 obsolete files)

Following the bug 805333 which version is implemented as "mute" first. But according the meeting including Andreas, Jonas and Robert, the conclusion is to "pause".

The main reason for pausing not muting is the concern on power consumption.
Marco: Do you think we need to block on this? It seems like we might get away without blocking for this, but battery consumption will be higher, though only for short periods of time.
blocking-basecamp: --- → ?
If we ignore the power consumption first then the only concern is on bug 796333.

Ex: 1. A game is on playing
    2. Another App is launched and game falls to background.
    3. The audio stream from game is muted now.

If user presses the power key for suspend, then screen will be off and no sound appeared too.
But actually the device doesn't go into suspend but screen_off only because the wakelock from audio driver.

So maybe the patch on 796333 can be evaluated to commit for pausing the normal channel in background. Then this bug can be non blocking-basecamp from my point of view.
Would that let us still play audio from the "content" channel when the user presses the power key?
1. The default channel on media element is normal not content and content needs permission.
   So the existing game or web content will be on normal channel.
2. And we expect that sound from game and a web content from browser tab should not be hear 
   while they are in background.
This seems like it would be a lot of work.  We discussed it during triage and decided we can't block on it.  Please re-nom if you disagree.
blocking-basecamp: ? → -
blocking-kilimanjaro: --- → +
We talked through it some more and decided that we need to either fix this bug or bug 796333. We should fix whichever one is easier. So if we decide that fixing this one is easier just renominate and then start hacking, no need to wait for a plus.
I think we should fix this. I don't think it should be a lot of work.
Taking this.  The fix for this will address bug 796333 too once bug 819836 is fixed.
Assignee: nobody → kinetik
Status: NEW → ASSIGNED
Attached patch patch v0 (obsolete) — Splinter Review
This needs more testing (I've only managed to test it on desktop with the MOZ_B2G ifdefs removed), so only requesting feedback to ensure the approach makes sense.  Similar to my simple patch from bug 796333, but this uses the audio channel service for determining the suspend state (rather than testing the channel type directly), and addresses roc's comment in bug 796333 comment 56.
Attachment #690652 - Flags: feedback?(roc)
Comment on attachment 690652 [details] [diff] [review]
patch v0

Review of attachment 690652 [details] [diff] [review]:
-----------------------------------------------------------------

Can you change the name of mPausedForInactiveDocument to cover the audiochannel case? I can't think of a good name though :-(

mPausedForInactiveDocumentOrChannel?
Attachment #690652 - Flags: feedback?(roc) → review+
Blocks: 819873
Re-nom because this blocks a blocking bug which cannot be fixed in Gaia.
Blocks: 819852
blocking-basecamp: - → ?
Triage: Blocks a blocker.
blocking-basecamp: ? → +
I'm not convinced bug 819873 is a blocker. And so there's no blockers depending on this right now.
blocking-basecamp: + → ?
Due to the power consumption concern and the fact that bug 796333 will likely fix this (and has an r+'d patch), we decided during triage to block on this.
Blocks: 796333
blocking-basecamp: ? → +
Attached patch patch v1 (obsolete) — Splinter Review
Address review comments.  Passes local desktop tests, try push: https://tbpl.mozilla.org/?tree=Try&rev=8ba3d5ce6706
Attachment #690652 - Attachment is obsolete: true
One test failure so far:

https://tbpl.mozilla.org/php/getParsedLog.php?id=17841023&tree=Try#error0

###!!! ASSERTION: Should not have pending events: 'mPendingEvents.IsEmpty()', file e:/builds/moz2_slave/try-w32-dbg/build/content/html/content/src/nsHTMLMediaElement.cpp, line 2308
REFTEST TEST-UNEXPECTED-FAIL | file:///C:/talos-slave/test/build/reftest/tests/content/media/test/crashtests/459439-1.html | assertion count 1 is more than expected 0 assertions

I'll remove the assertion and clear mPendingEvents at the same location, that seems to be the correct solution.
Attachment #691067 - Attachment is obsolete: true
Attached patch patch v2Splinter Review
Remove the mPendingEvents assertion and clear mPendingEvents when resetting mSuspendEvents.  Also move the UpdateAudioChannelPlayingState call in ChangeReadyState to avoid calling it when the ready state has not changed.

Try push: https://tbpl.mozilla.org/?tree=Try&rev=53f5f6dbf45e
Attachment #691210 - Flags: review?(roc)
(In reply to Jonas Sicking (:sicking) from comment #13)
> I'm not convinced bug 819873 is a blocker. And so there's no blockers
> depending on this right now.

Jonas, please see bug 819873 comment 5.
https://hg.mozilla.org/mozilla-central/rev/a00257dcffab
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
I missed the news about beta and b2g18.  Backed out on beta: https://hg.mozilla.org/releases/mozilla-beta/rev/bacea371020a

I'll land it on b2g18 once my clone finishes.
And landed on b2g18: https://hg.mozilla.org/releases/mozilla-b2g18/rev/07f691e14c24
Whiteboard: [status-b2g18:fixed]
Whiteboard: [status-b2g18:fixed]
FYI, I wouldn't lose too much sleep over landing it on beta in the future. The plan is to be merging beta to b2g18 regularly anyway.
Depends on: 822647
You need to log in before you can comment on or make changes to this bug.