Closed
Bug 818708
Opened 12 years ago
Closed 12 years ago
content audio channels should be muted if in background when another content audio channel is playing
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla20
People
(Reporter: baku, Assigned: baku)
References
Details
Attachments
(1 file, 3 obsolete files)
1.61 KB,
patch
|
baku
:
review+
overholt
:
approval-mozilla-b2g18+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Updated•12 years ago
|
Assignee: nobody → amarchesini
Assignee | ||
Comment 1•12 years ago
|
||
Need a feedback about this algorithm.
Attachment #688986 -
Flags: feedback?(jonas)
Comment on attachment 688986 [details] [diff] [review]
patch
Review of attachment 688986 [details] [diff] [review]:
-----------------------------------------------------------------
It's getting very late here and my brain is a bit too fried to follow the flows here right now. I'll try to grab you tomorrow morning if you're still around. Otherwise I'll give it a second go after some sleep.
::: dom/audiochannel/AudioChannelService.cpp
@@ +138,5 @@
> + // We care about the visibility only for the content audio channel type.
> + if (data.mType == AUDIO_CHANNEL_CONTENT &&
> + data.mElementHidden != aElementHidden) {
> + data.mElementHidden = aElementHidden;
> + mMediaElements.Put(aMediaElement, data);
It seems quite strange to update the hashtable on a "Get" call.
Why isn't the hash table already updated by the time we get here?
Comment 3•12 years ago
|
||
As I knew that the current works on m-c already have this capability.
I tried this case and it muted the background one.
1. Playing a song from music player (content channel).
2. Back to home. (continue to play)
3. Playing a video from video player (content channel).
4. Then music player in background will be muted.
It seems to be done by http://hg.mozilla.org/mozilla-central/file/5f626f86b374/dom/audiochannel/AudioChannelService.cpp#l138 .
Assignee | ||
Comment 4•12 years ago
|
||
Sure, it is. But what about if an app has more than 1 content channel playing?
That is just a quick solution... but we need a proper algorithm.
Assignee | ||
Comment 5•12 years ago
|
||
Attachment #688986 -
Attachment is obsolete: true
Attachment #688986 -
Flags: feedback?(jonas)
Attachment #690445 -
Flags: feedback?(jonas)
Comment 6•12 years ago
|
||
Hi Andrea,
It seems that you will stop all of audio streams on content channel if there are multiple Apps and they are in the background.
I just think that do we need to keep playing on the last one fall to background.
Thanks.
Assignee | ||
Comment 7•12 years ago
|
||
Marco I though about it but our UX doesn't have the concept of 'the last app' fall to background. and there is not an easy way to open the last one app fall to background.
So probably this bug can be fixed in this way (if the patch is good enough) and then implement a better approach later.
I would like to have a feedback from someone working on UX.
Comment 8•12 years ago
|
||
Comment on attachment 690445 [details] [diff] [review]
patch b
Review of attachment 690445 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/audiochannel/AudioChannelService.cpp
@@ +105,5 @@
> + if (!mContentApps.Get(aAppId, &counter)) {
> + counter = 0;
> + }
> + mContentApps.Put(aAppId, counter + 1);
> +
all channel types got mContentApps ++ ?
@@ +197,5 @@
> +
> + // If there are not visible apps but there are more than
> + // one "content app", then we mute all the content channels
> + if (!mVisibleContentChannels && aElementHidden &&
> + mContentApps.Count() > 1)
two or more background apps is muted(paused). That may need UX to decide what behavior they want.
Assignee | ||
Comment 9•12 years ago
|
||
Let's wait until 820704 is landed. This patch must be rebased.
Assignee | ||
Updated•12 years ago
|
Attachment #690445 -
Flags: feedback?(jonas)
Assignee | ||
Comment 10•12 years ago
|
||
This patch is based on bug 820704
Attachment #690445 -
Attachment is obsolete: true
Attachment #693845 -
Flags: review?(jonas)
Assignee | ||
Comment 11•12 years ago
|
||
Jonas, this patch is ready and the bug 820704 is going to land soon. Can you give me a feedback?
Comment on attachment 693845 [details] [diff] [review]
patch c
Review of attachment 693845 [details] [diff] [review]:
-----------------------------------------------------------------
These won't be appid, but rather TabParent-ids, right?
With the name fixed, r=me
However I think that we should do the stack thing. We already will have the case that if you shut down an app which is currently using the "content" channel you can end up hearing audio from another app which isn't the first app in the "card deck". With this patch that can happen if you have two apps currently using the "content" channel and you close one of them.
However I don't think it's important, so I think we should only do it if it's easy. And I'm happy to do it as a followup bug or as a followup patch.
Attachment #693845 -
Flags: review?(jonas) → review+
Assignee | ||
Comment 13•12 years ago
|
||
waiting for a try result
Attachment #693845 -
Attachment is obsolete: true
Attachment #696243 -
Flags: review+
Assignee | ||
Updated•12 years ago
|
Keywords: checkin-needed
Comment 14•12 years ago
|
||
Try run for 2ce640a19d46 is complete.
Detailed breakdown of the results available here:
https://tbpl.mozilla.org/?tree=Try&rev=2ce640a19d46
Results (out of 256 total builds):
exception: 1
success: 238
warnings: 17
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/amarchesini@mozilla.com-2ce640a19d46
Assignee | ||
Comment 15•12 years ago
|
||
it seems green enough to me :)
Comment 16•12 years ago
|
||
Try run for 2ce640a19d46 is complete.
Detailed breakdown of the results available here:
https://tbpl.mozilla.org/?tree=Try&rev=2ce640a19d46
Results (out of 258 total builds):
exception: 1
success: 239
warnings: 18
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/amarchesini@mozilla.com-2ce640a19d46
Comment 17•12 years ago
|
||
Keywords: checkin-needed
Comment 18•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Comment 19•12 years ago
|
||
Try run for 2ce640a19d46 is complete.
Detailed breakdown of the results available here:
https://tbpl.mozilla.org/?tree=Try&rev=2ce640a19d46
Results (out of 280 total builds):
exception: 1
success: 250
warnings: 29
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/amarchesini@mozilla.com-2ce640a19d46
Assignee | ||
Comment 20•12 years ago
|
||
Comment 21•12 years ago
|
||
Comment on attachment 696243 [details] [diff] [review]
patch d
[Triage Comment]
Attachment #696243 -
Flags: approval-mozilla-b2g18+
Assignee | ||
Updated•12 years ago
|
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•