844765 - the device should be put to sleep when it stops playing audio.

Status: RESOLVED INVALID

(Firefox OS Graveyard :: General, defect)

Comment 1

[Andrea Marchesini [:baku]]

Attachment: patch

There are 2 issues here:

1. if the process is playing audio, we schedule a timer in order to see when it stops playing. We are tolerant with it, so that if the process was playing but now it's stopped, we just wait another loop of the timer.

2. there was a bug in the AudioChannelServiceChild: the mChannelContents was not properly updated.

Comment 2

[Justin Lebar (not reading bugmail)]

> 2. there was a bug in the AudioChannelServiceChild: the mChannelContents was not properly updated.

Does this fix bug 843417?  (If you're not sure, could you test?)  If so, let's forward-dupe bug 843417 here.

Comment 3

[Justin Lebar (not reading bugmail)]

I'd rather to this a different way in the process priority manager:

Instead of tracking mWasPlaying and adding a new timer, can we simply make transitioning "down" from BG_PERCEIVABLE require a timeout?

That is, right now we transition immediately between any two BACKGROUND* priorities, but we'd change it so that we have a grace period (I'd use the same 1s grace period, unless we know that's not sufficient) if we're going from BG_PERCEIVABLE down to a lower BG*.

I'd change ResetPriority() so that instead of asking "is foreground" and "is background" and then calling SetIsForeground()/SetIsBackground(), we asked "what's the new priority" and then set it either immediately or not, as appropriate.

Does that make sense?

I'll look at the audio manager changes now.

Comment 4

[Justin Lebar (not reading bugmail)]

What does this bug have to do with putting the device "to sleep"?  As far as I can tell we're only changing the priority of processes.

Comment 5

[Justin Lebar (not reading bugmail)]

>diff --git a/dom/audiochannel/AudioChannelService.h b/dom/audiochannel/AudioChannelService.h
>--- a/dom/audiochannel/AudioChannelService.h
>+++ b/dom/audiochannel/AudioChannelService.h
>+void
>+AudioChannelService::UpdateChannelType(AudioChannelType aType,
>+                                       uint64_t aChildID,
>+                                       bool aElementHidden,
>+                                       bool aElementWasHidden)
>+{
>+  // Calculating the new and old type and update the hashtable if needed.

s/Calculating/Calculate
s/type/internal types/

(When you say "X and Y", generally X and Y must be the same sort of thing.
Here X is a gerund ("calculat/ing/"), but Y is a verb.  You could say
"Calculating and updating the hashtable", but then the sentence would have no
verb, so it would be a fragment.  So it's best to make X and Y both be verbs.)

The changes to the AudioChannelService otherwise look correct, but I'm not
entirely comfortable reviewing them; I'd feel better if you got this signed off
by someone else.

But I also think that we should be checking the return values of these
RemoveElement() calls.  If we'd been doing so, we'd have caught this bug
earlier.

Unfortunately nobody runs B2G debug builds, so if we made debug-only
assertions, nobody would see them.  Perhaps we should make these fatal in debug
builds and print something to logcat in release-Android builds.

Comment 6

[Justin Lebar (not reading bugmail)]

Andrea and I talked for a while about this on IRC.

Essentially he'd confused "device going to sleep" with the issue in bug 843417.  The device going to sleep is controlled by the CPU wake lock, which doesn't have to do with the changes in the patches here.

So we're going to close this bug and move the audio manager changes over to bug 843417.  Additionally, I'm going to write a patch to give a grace period for BG_PERCEIVABLE --> BG* priority changes, which is an additional part of Andrea's patch here.  I'll file a bug for that.

Comment 7

[Justin Lebar (not reading bugmail)]

I filed bug 844970 on the process-priority stuff.