1043489 - Fullscreen video doesn't release wakelock when playback stops

Status: VERIFIED FIXED

(Core :: Audio/Video, defect)

Description

[Ralph Giles (:rillian)]

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.

Comment 1

[Ralph Giles (:rillian)]

I've also seen occasional log messages about not being able to release the wakelock, e.g. at shutdown. May be unrelated.

Comment 2

[Ralph Giles (:rillian)]

I can no longer reproduce with today's Nightly 34.0a1 (2014-08-05).

Comment 3

[Catalin Varga [QA][:VarCat]]

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

Comment 4

[Tracy Walker [:tracy]]

[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."

Comment 5

[Sylvestre Ledru [:Sylvestre]]

Chris, do you know which patch we would have to backout?
33 RC goes to build in a few hours.

Comment 6

[Lawrence Mandel [:lmandel] (use needinfo)]

I'm marking tracking across the board. Sylvestre will be looking to determine what to do in 33.

Comment 7

[Ralph Giles (:rillian)]

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.

Comment 8

[Ralph Giles (:rillian)]

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.

Comment 9

[Chris Peterson [:cpeterson]]

(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.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Fixed on 33 via backout.

Comment 11

[Chris Pearce [:cpearce (Not reading bugmail)]]

(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.

Comment 12

[Chris Pearce [:cpearce (Not reading bugmail)]]

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.

Comment 13

[Ralph Giles (:rillian)]

Attachment: Only watch for the 'screen' topic when taking wakelock

Thanks for the tip, Chris. Porting your changes to the cocoa code resolves the issue.

Comment 14

[Steven Michaud [:smichaud] (Retired)]

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.

Comment 15

[Ralph Giles (:rillian)]

Attachment: Only watch for the 'screen' topic when taking wakelock

Thanks. Addressing comment nit and carrying forward r=smichaud.

Comment 16

[Ralph Giles (:rillian)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0564e5f5037c

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0564e5f5037c

Comment 18

[Ralph Giles (:rillian)]

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

Comment 19

[Ralph Giles (:rillian)]

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 20

[Lawrence Mandel [:lmandel] (use needinfo)]

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.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9bedbe95e531

Comment 22

[Catalin Varga [QA][:VarCat]]

Verified as fixed using the following environment:

FF 34 Build Id: 20141013004002
FF 35 Build Id: 20141013030202
OS: Mac Os X 10.9.5