Closed Bug 1022669 Opened 10 years ago Closed 10 years ago

Display does not turn off while playing audio

Categories

(Firefox for Android Graveyard :: General, defect)

29 Branch
ARM
Android
defect
Not set
normal

Tracking

(fennec+)

RESOLVED FIXED
Firefox 35
Tracking Status
fennec + ---

People

(Reporter: bugzillamozilla.20.sichersicher, Assigned: esawin)

References

Details

Attachments

(1 file, 2 obsolete files)

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0 (Beta/Release)
Build ID: 20140506152807

Steps to reproduce:

Play an audio file using Firefox' internal audio player. E.g. http://alternativlos.cdn.as250.net/alternativlos-31.mp3


Actual results:

The display did not turn off after a minute to save battery, but instead stayed on the whole time.


Expected results:

The display should have turned off after a minute because it is not needed while listening to audio.
OS: Windows 7 → Android
Hardware: x86_64 → ARM
My system: Sony Xperia Tablet Z, Android 4.3, FF 29.0.1
Likely bug 739542 introduced this. Unsure if we want a different behavior for <video> and <audio>. Ian?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(ibarlow)
(In reply to Kevin Brosnan [:kbrosnan] from comment #2)
> Likely bug 739542 introduced this. Unsure if we want a different behavior
> for <video> and <audio>. Ian?

That seems reasonable to me. I believe we already have different experiences for audio vs video when Firefox is put into the background -- video stops playing whereas audio continues to play.
Flags: needinfo?(ibarlow)
tracking-fennec: --- → ?
tracking-fennec: ? → +
Assignee: nobody → scleymans
Attached patch patch (obsolete) — Splinter Review
Use CPU wake lock instead of screen wake lock when there is no video.
Attachment #8440171 - Flags: review?(wjohnston)
Comment on attachment 8440171 [details] [diff] [review]
patch

Review of attachment 8440171 [details] [diff] [review]:
-----------------------------------------------------------------

The media guys should review the content stuff.

::: mobile/android/base/GeckoApp.java
@@ +2586,5 @@
>          PowerManager.WakeLock wl = mWakeLocks.get(topic);
>          if (state.equals("locked-foreground") && wl == null) {
>              PowerManager pm = (PowerManager) getSystemService(Context.POWER_SERVICE);
> +
> +            if (topic.equals("cpu")) {

reverse this and use a final static constant. i.e.
final static String CPU = "cpu";
if (CPU.equals(topic)) {
}

@@ +2587,5 @@
>          if (state.equals("locked-foreground") && wl == null) {
>              PowerManager pm = (PowerManager) getSystemService(Context.POWER_SERVICE);
> +
> +            if (topic.equals("cpu")) {
> +              wl = pm.newWakeLock(PowerManager.PARTIAL_WAKE_LOCK, topic);

I'd probably just change the second parameter to "Gecko" or something. Its for debugging.
Attachment #8440171 - Flags: review?(cpearce)
Comment on attachment 8440171 [details] [diff] [review]
patch

Review of attachment 8440171 [details] [diff] [review]:
-----------------------------------------------------------------

The media/video element changes look good to me.
Attachment #8440171 - Flags: review?(cpearce) → review+
Attached patch patch (obsolete) — Splinter Review
Carrying forward r+ from :cpearce.
Attachment #8440171 - Attachment is obsolete: true
Attachment #8440171 - Flags: review?(wjohnston)
Attachment #8441754 - Flags: review?(wjohnston)
Attachment #8441754 - Flags: review?(wjohnston) → review+
Can we please run this through Try to ensure no test bustage? Primarily in the media and wakelock tests that we have. Thanks! :)
Keywords: checkin-needed
Anything happening here?
Flags: needinfo?(wjohnston)
Flags: needinfo?(snorp)
pushed to try:

https://tbpl.mozilla.org/?tree=Try&rev=6a2686a30f15

will land if that's fine
Flags: needinfo?(wjohnston)
Flags: needinfo?(snorp)
Red :( Just needs a trivial fix to get video info:
https://tbpl.mozilla.org/?tree=Try&rev=9d8146acb25a
Assignee: scleymans → esawin
Fixing that still revealed a second test failure in:

http://mxr.mozilla.org/mozilla-central/source/content/html/content/test/test_video_wakelock.html

That looks like its a real change from the test, but I haven't had time to figure out whats up. Someone who knows this code better might be able to quickly respond?
Looks like we aren't always setting mHasVideo in time in MetadataLoaded.
With this patch we would update mHasVideo when retrieving the video frame container.

Try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=ac3472bed044
Attachment #8441754 - Attachment is obsolete: true
Comment on attachment 8498542 [details] [diff] [review]
audio-screen.patch

Review of attachment 8498542 [details] [diff] [review]:
-----------------------------------------------------------------

Try results look good and it works on device.
Attachment #8498542 - Flags: review?(wjohnston)
Comment on attachment 8498542 [details] [diff] [review]
audio-screen.patch

I really can't review this content/html stuff. cpearce?
Attachment #8498542 - Flags: review?(wjohnston) → review?(cpearce)
Attachment #8498542 - Flags: review?(cpearce) → review+
Status: NEW → ASSIGNED
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/5da210c60d38
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 35
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.