Andrea Marchesini [:baku] Assignee
	Description • 12 years ago

      No description provided.

	Andrea Marchesini [:baku] Assignee
	Comment 1 • 12 years ago

Need a feedback about this algorithm.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 2 • 12 years ago

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?

	Marco Chen [:mchen]
	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 .

	Andrea Marchesini [:baku] 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.

	Marco Chen [:mchen]
	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.

	Andrea Marchesini [:baku] 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.

	Randy Lin [:rlin] - Be Mozillian on 2015/01/26
	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.

	Andrea Marchesini [:baku] Assignee
	Comment 9 • 12 years ago

Let's wait until 820704 is landed. This patch must be rebased.

	Andrea Marchesini [:baku] Assignee
	Comment 10 • 12 years ago

This patch is based on bug 820704

	Andrea Marchesini [:baku] 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?

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 12 • 12 years ago

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.

	Andrea Marchesini [:baku] Assignee
	Comment 13 • 12 years ago

waiting for a try result

	Mozilla RelEng Bot
	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

	Andrea Marchesini [:baku] Assignee
	Comment 15 • 12 years ago

it seems green enough to me :)

	Mozilla RelEng Bot
	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

	Ryan VanderMeulen [:RyanVM]
	Comment 17 • 12 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/8620caa05e54

	Ed Morley [:emorley]
	Comment 18 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/8620caa05e54

	Mozilla RelEng Bot
	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

	Andrea Marchesini [:baku] Assignee
	Comment 20 • 12 years ago

https://hg.mozilla.org/releases/mozilla-b2g18/rev/899e62c52d34

	Andrew Overholt [:overholt]
	Comment 21 • 12 years ago

Comment on attachment 696243 [details] [diff] [review]
patch d

[Triage Comment]