Closed Bug 1043489 Opened 6 years ago Closed 5 years ago
Fullscreen video doesn't release wakelock when playback stops
In verifying bug 772347, :VarCat reported > I've played https://people.xiph.org/~giles/2013/02-Digital_Show_and_Tell.html > in fullscreen and waited the video to finish. After the video finished the > screensaver will not start after the set time (in my case 2 min). This issue > is not reproducible when the video is played in window mode. Which sounds like a bug to me.
I've also seen occasional log messages about not being able to release the wakelock, e.g. at shutdown. May be unrelated.
I can no longer reproduce with today's Nightly 34.0a1 (2014-08-05).
I tested this issue on: FF 34 Nightly Build Id:20140808030201 OS: Mac Os X 10.7.5 and is still reproducible using https://people.xiph.org/~giles/2013/02-Digital_Show_and_Tell.html
[Tracking Requested - why for this release]: backout per email: 'We should probably back out this feature from Beta and fix this on Aurora. Chris P."
Chris, do you know which patch we would have to backout? 33 RC goes to build in a few hours.
I'm marking tracking across the board. Sylvestre will be looking to determine what to do in 33.
The backout would be the patch from bug 772347. I expect that would be relatively clean, but I'm nervous about a backout so close to RC. There's no time to test. https://hg.mozilla.org/mozilla-central/rev/dde8abeff07a She issue didn't seem too critical to me, so I've had it too low in my priority list for it to get work time. Sorry, I should have said.
Exiting fullscreen is sufficient to release the wakelock, so this bug only keeps the machine from sleeping while a video is in fullscreen mode. The wakelock doesn't hang around indefinitely.
(In reply to Sylvestre Ledru [:sylvestre] from comment #5) > Chris, do you know which patch we would have to backout? > 33 RC goes to build in a few hours. Sorry, I don't know with patch to back out. I didn't write or review any of the patches involved with this bug or bug 772347. The reference to "Chris P." in comment 4 may have been Chris Pearce, who had commented on bug 772347.
(In reply to Chris Peterson (:cpeterson) from comment #9) > The reference to "Chris > P." in comment 4 may have been Chris Pearce, who had commented on bug 772347. Yep, that was me.
We had a similar issue on Windows; I fixed that by only toggling the screen saver on the "screen" topic, not other topics (like "DOMFullscreen" or whatever it is). That was in bug 1063995. We probably just need a similar patch for MacOSX like what I did on Windows.
Thanks for the tip, Chris. Porting your changes to the cocoa code resolves the issue.
Attachment #8500762 - Flags: review?(smichaud)
Comment on attachment 8500762 [details] [diff] [review] Only watch for the 'screen' topic when taking wakelock Looks reasonable to me. However I didn't have time to test it. One small nit: + // "locked-foreground" notifications when multipe wake locks are held. "multiple" is misspelled here.
Attachment #8500762 - Flags: review?(smichaud) → review+
Thanks. Addressing comment nit and carrying forward r=smichaud.
Comment on attachment 8501139 [details] [diff] [review] Only watch for the 'screen' topic when taking wakelock Approval Request Comment [Feature/regressing bug #]: feature bug 772347 [User impact if declined]: Firefox will prevent the screen from sleeping while video is fullscreen, causing unnecessary power drain after playback is ended. [Describe test coverage new/current, TBPL]: Landed on m-c, manual testing. [Risks and why]: New code is simpler, and matches what we do on Windows, so I think the risk is low. I would like to land on Aurora for additional user testing in case there are other issues of this nature, and so we can release this feature on 34. [String/UUID change made/needed]: None
Attachment #8501139 - Flags: approval-mozilla-aurora?
If QA has time to verify this bug, please pay special attention to interactions with multiple videos playing simultaneously. We've changed the way we account for that, and I've only tested with a single video. See description for STR.
Comment on attachment 8501139 [details] [diff] [review] Only watch for the 'screen' topic when taking wakelock Aurora+ I see this has been marked as qe-verify+. I'm unclear whether that means that QE has verified or will be verifying. I would like to see us test some edge cases (like multiple videos) to ensure that this is functioning as expected with the change.
Attachment #8501139 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Verified as fixed using the following environment: FF 34 Build Id: 20141013004002 FF 35 Build Id: 20141013030202 OS: Mac Os X 10.9.5
You need to log in before you can comment on or make changes to this bug.