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

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Marco Chen [:mchen]]

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.

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 2

[Marco Chen [:mchen]]

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.

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Would that let us still play audio from the "content" channel when the user presses the power key?

Comment 4

[Marco Chen [:mchen]]

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.

Comment 5

[Andrew Overholt [:overholt]]

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.

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think we should fix this. I don't think it should be a lot of work.

Comment 8

[Matthew Gregan [:kinetik]]

Taking this.  The fix for this will address bug 796333 too once bug 819836 is fixed.

Comment 9

[Matthew Gregan [:kinetik]]

Attachment: patch v0

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.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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?

Comment 11

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Re-nom because this blocks a blocking bug which cannot be fixed in Gaia.

Comment 12

[Ben Francis [:benfrancis]]

Triage: Blocks a blocker.

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I'm not convinced bug 819873 is a blocker. And so there's no blockers depending on this right now.

Comment 14

[Andrew Overholt [:overholt]]

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.

Comment 15

[Matthew Gregan [:kinetik]]

Attachment: patch v1

Address review comments.  Passes local desktop tests, try push: https://tbpl.mozilla.org/?tree=Try&rev=8ba3d5ce6706

Comment 16

[Matthew Gregan [:kinetik]]

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.

Comment 17

[Matthew Gregan [:kinetik]]

Attachment: patch v2

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

Comment 18

[Matthew Gregan [:kinetik]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a00257dcffab

Comment 19

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(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.

Comment 20

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/a00257dcffab

Comment 21

[Matthew Gregan [:kinetik]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/dbf28a8565a5
https://hg.mozilla.org/releases/mozilla-beta/rev/0d1752cf4551

Comment 22

[Matthew Gregan [:kinetik]]

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.

Comment 23

[Matthew Gregan [:kinetik]]

And landed on b2g18: https://hg.mozilla.org/releases/mozilla-b2g18/rev/07f691e14c24

Comment 24

[Ryan VanderMeulen [:RyanVM]]

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.