Closed
Bug 1022669
Opened 11 years ago
Closed 10 years ago
Display does not turn off while playing audio
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(fennec+)
RESOLVED
FIXED
Firefox 35
Tracking | Status | |
---|---|---|
fennec | + | --- |
People
(Reporter: bugzillamozilla.20.sichersicher, Assigned: esawin)
References
Details
Attachments
(1 file, 2 obsolete files)
5.78 KB,
patch
|
cpearce
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Updated•11 years ago
|
OS: Windows 7 → Android
Hardware: x86_64 → ARM
Reporter | ||
Comment 1•11 years ago
|
||
My system: Sony Xperia Tablet Z, Android 4.3, FF 29.0.1
Comment 2•11 years ago
|
||
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)
Comment 3•11 years ago
|
||
(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)
Updated•11 years ago
|
tracking-fennec: --- → ?
Updated•11 years ago
|
tracking-fennec: ? → +
Updated•11 years ago
|
Assignee: nobody → scleymans
Comment 4•11 years ago
|
||
Use CPU wake lock instead of screen wake lock when there is no video.
Attachment #8440171 -
Flags: review?(wjohnston)
Comment 5•11 years ago
|
||
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 6•11 years ago
|
||
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+
Comment 7•11 years ago
|
||
Carrying forward r+ from :cpearce.
Attachment #8440171 -
Attachment is obsolete: true
Attachment #8440171 -
Flags: review?(wjohnston)
Attachment #8441754 -
Flags: review?(wjohnston)
Updated•11 years ago
|
Attachment #8441754 -
Flags: review?(wjohnston) → review+
Updated•11 years ago
|
Keywords: checkin-needed
Comment 8•11 years ago
|
||
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
Comment 10•10 years ago
|
||
Anything happening here?
Flags: needinfo?(wjohnston)
Flags: needinfo?(snorp)
Comment 11•10 years ago
|
||
pushed to try:
https://tbpl.mozilla.org/?tree=Try&rev=6a2686a30f15
will land if that's fine
Flags: needinfo?(wjohnston)
Flags: needinfo?(snorp)
Comment 12•10 years ago
|
||
Red :( Just needs a trivial fix to get video info:
https://tbpl.mozilla.org/?tree=Try&rev=9d8146acb25a
Updated•10 years ago
|
Assignee: scleymans → esawin
Comment 13•10 years ago
|
||
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?
Assignee | ||
Comment 14•10 years ago
|
||
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
Assignee | ||
Comment 15•10 years ago
|
||
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 16•10 years ago
|
||
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)
Updated•10 years ago
|
Attachment #8498542 -
Flags: review?(cpearce) → review+
Assignee | ||
Updated•10 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
Comment 18•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 35
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•