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)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla20
Tracking Status
firefox19 --- wontfix
firefox20 --- wontfix
b2g18 --- fixed

People

(Reporter: baku, Assigned: baku)

References

Details

Attachments

(1 file, 3 obsolete files)

      No description provided.
Assignee: nobody → amarchesini
Attached patch patch (obsolete) — Splinter Review
Need a feedback about this algorithm.
Attachment #688986 - Flags: feedback?(jonas)
Blocks: 811224
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?
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 .
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.
Attached patch patch b (obsolete) — Splinter Review
Attachment #688986 - Attachment is obsolete: true
Attachment #688986 - Flags: feedback?(jonas)
Attachment #690445 - Flags: feedback?(jonas)
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.
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 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.
Let's wait until 820704 is landed. This patch must be rebased.
Attachment #690445 - Flags: feedback?(jonas)
Attached patch patch c (obsolete) — Splinter Review
This patch is based on bug 820704
Attachment #690445 - Attachment is obsolete: true
Attachment #693845 - Flags: review?(jonas)
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+
Attached patch patch dSplinter Review
waiting for a try result
Attachment #693845 - Attachment is obsolete: true
Attachment #696243 - Flags: review+
Keywords: checkin-needed
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
it seems green enough to me :)
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
https://hg.mozilla.org/mozilla-central/rev/8620caa05e54
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
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
Comment on attachment 696243 [details] [diff] [review]
patch d

[Triage Comment]
Attachment #696243 - Flags: approval-mozilla-b2g18+
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: