Closed Bug 1043489 Opened 10 years ago Closed 10 years ago

Fullscreen video doesn't release wakelock when playback stops

Categories

(Core :: Audio/Video, defect)

x86
macOS
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla35
Tracking Status
firefox33 + disabled
firefox34 + verified
firefox35 + verified

People

(Reporter: rillian, Assigned: rillian)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Assignee: nobody → giles
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.
Flags: needinfo?(cpeterson)
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.
Depends on: 772347
(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.
Flags: needinfo?(cpeterson)
Fixed on 33 via backout.
(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.
Attachment #8500762 - Attachment is obsolete: true
Attachment #8501139 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0564e5f5037c
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
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.
Keywords: verifyme
Flags: qe-verify+
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
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.