772347 - Add screen WakeLockListener on MacOSX to disable screensaver

Status: VERIFIED FIXED

(Core :: Audio/Video, defect)

Description

[(no longer active)]

I noticed this on Mac, not sure if it happens on other platforms.

Comment 1

[cajbir (:cajbir)]

Dupe of bug 517870? Or is the password screen on the mac different?

Comment 2

[(no longer active)]

I don't really know.  Calling this a dupe and CCing smichaud to yell at me if the password screen on Mac is activated using another mechanism.

Comment 3

[Steven Michaud [:smichaud] (Retired)]

Sounds reasonable to me.

Only you, Ehsan, can tell us whether or not what you saw was the screensaver asking you for a password :-)

If you did, then you're right.

Comment 4

[(no longer active)]

I have screen saver disabled, so my mac goes directly to the password screen after 30 minutes.

Comment 5

[Steven Michaud [:smichaud] (Retired)]

> I have screen saver disabled, so my mac goes directly to the
> password screen after 30 minutes.

So you have your computer set to log you out after 30 minutes of
inactivity?

(This can be done in System Preferences : Security, and is off by
default.)

If so then this bug isn't a dup of bug 517870.  (Though it's
conceivable you could "fix" both bugs by faking "activity".)

And by the way, what you saw wasn't the "password screen", it was the
"login screen".

Comment 6

[(no longer active)]

(In reply to comment #5)
> > I have screen saver disabled, so my mac goes directly to the
> > password screen after 30 minutes.
> 
> So you have your computer set to log you out after 30 minutes of
> inactivity?
> 
> (This can be done in System Preferences : Security, and is off by
> default.)
> 
> If so then this bug isn't a dup of bug 517870.  (Though it's
> conceivable you could "fix" both bugs by faking "activity".)
> 
> And by the way, what you saw wasn't the "password screen", it was the
> "login screen".

Hmm, OK seems like I've got everything wrong!  Here's the settings I have:

In Security, I have "Require password for sleep and screen saver" checked.  In "Desktop and Screen Saver", I do have a screen saver set up (sorry for misdirecting you on that).  In Energy Saver, I have set up "Display sleep" to a value which I think might be lower than the screen saver timeout, so I don't see the screen saver  before my display goes to sleep.

Comment 7

[Steven Michaud [:smichaud] (Retired)]

So, when you're in fullscreen mode your display goes to sleep, and then when you move the mouse you see the "password for sleep and screensaver" screen?

Comment 8

[(no longer active)]

(In reply to comment #7)
> So, when you're in fullscreen mode your display goes to sleep, and then when
> you move the mouse you see the "password for sleep and screensaver" screen?

Correct.

Comment 9

[Steven Michaud [:smichaud] (Retired)]

I think the bug here is that your display shouldn't go to sleep when you're in "fullscreen mode".  Which makes this bug different from bug 517870 (though still related to it).

By the way, I assume the "fullscreen mode" you're talking about is a plugin's fullscreen mode, and not the browser's fullscreen mode.

Comment 10

[(no longer active)]

(In reply to comment #9)
> I think the bug here is that your display shouldn't go to sleep when you're in
> "fullscreen mode".  Which makes this bug different from bug 517870 (though
> still related to it).

So should we reopen this?

> By the way, I assume the "fullscreen mode" you're talking about is a plugin's
> fullscreen mode, and not the browser's fullscreen mode.

It's the new fancy 10.7 full-screen thing, whatever it's called.  This is the video in question, fwiw: <http://assange.rt.com/ibrahim-episode-eleven/>

Comment 11

[Steven Michaud [:smichaud] (Retired)]

> So should we reopen this?

Yes, let's do that.

I need to dig into this further to figure out its full implications.

Comment 12

[Axel Hecht]

Not sure if this is different code wise, but we should keep the display from dimming, too. With default settings, that happens before the actual sleep and screen savers kick in .

Comment 13

[Chris Peterson [:cpeterson]]

Firefox on Android has the same problem (bug 786893). It would be nice for the core video code notify the platform widget code when to disallow/allow screen savers.

Comment 14

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

To disable the screensaver during <video> playback and WebRTC calls all we need to do is implement a WakeLockListener for each platform, and the existing code with cause the screensaver disable as appropriate. The WakeLockListener is already implemented for Windows, B2G, and Android. We need them implemented on other platforms.

I'll contort this bug to be implementing the MacOSX specific WakeLockListener. 

See bug 968603 for an example of how to do this (on Windows).

Comment 15

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

Chris, as you identified, this bug has already been fixed on Windows, B2G, and Android. Can we prioritize this for MacOS and Linux (bug 811261) for platform consistency?

Comment 16

[Ralph Giles (:rillian)]

I'm happy to look at this, but I've been asked to finish bug 941296 first.

Comment 17

[Jeff Muizelaar [:jrmuizel]]

https://developer.apple.com/library/mac/qa/qa1340/_index.html explains how to do this.

See also:

https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/power_save_blocker_mac.cc

Comment 18

[Jeff Muizelaar [:jrmuizel]]

And here's VLC's code. It seems like the api for doing this might be a bit of disaster:
http://git.videolan.org/?p=vlc.git;a=blob;f=modules/gui/macosx/intf.m;h=005390a01bca62ccad094c5d88c9ac79cba73bd3;hb=HEAD#l1442

Comment 20

[Lucas GOLVEN]

Do you steel work on this bug ? Do you want us to continue what you have done ?

Comment 21

[Ralph Giles (:rillian)]

I won't get to it for a few weeks yet, so if you want to take it, please go ahead.

Comment 22

[Lucas GOLVEN]

Attachment: bug-772347-fix.patch

Is it possible to be assigned ? Can you review the patch ?

Comment 23

[Paul Adenot (:padenot)]

Comment on attachment 8436316 [details] [diff] [review]
bug-772347-fix.patch

Steven, would you mind having a look at this?

Comment 24

[Steven Michaud [:smichaud] (Retired)]

I should be able to get to this in a few days.  Ping me if I forget :-)

Comment 25

[Lucas GOLVEN]

Attachment: bug-772347.patch

I have made some improvements compared to the first patch. If you haven't already look at the first patch you should ignore it, otherwise I can tell you what differs.

Comment 26

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8437506 [details] [diff] [review]
bug-772347.patch

This patch has one fairly big problem:

+        NS_ConvertUTF16toUTF8 s(aTopic);
+        CFStringRef cf_topic = (CFStringRef) &s;

This *shouldn't* work, even if it actually does.

Instead you should do something like this:

CFStringRef cf_topic =
  ::CFStringCreateWithCharacters(kCFAllocatorDefault,
                                 reinterpret_cast<const UniChar*>
                                   (aTopic.get()),
                                 aTopic.length());

Then you'll need to call CFRelease(cf_topic) after the call to IOPMAssertionCreateWithName().

And there's a style nit I'd like you to fix:

+  AddScreenWakeLockListener();
   NS_OBJC_TRY_ABORT([NSApp run]);
-
+  RemoveScreenWakeLockListener();
   return NS_OK;

Please leave an empty line above and below the calls to AddScreenWakeLockListener() and RemoveScreenWakeLockListener().

Otherwise this patch looks fine to me.  Please make these changes and ask me again to review them.

By the way, I notice that you've mostly just copied the Windows implementation.  This is exactly the right way to go about it!  And it's what I would have done in your place.  The less you write from scratch, the fewer mistakes you'll make :-)

Comment 27

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #14)

Chris:  I'd like to test Lucas's patch, but I don't know where to find examples to test with.  Could you list a few?

Comment 28

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8437506 [details] [diff] [review]
bug-772347.patch

Another style nit I forgot to mention:

Please ensure that there's a "::" before every call to an external, OS-provided API.  For example, IOPMAssertionCreateWithName(), IOPMAssertionRelease(), CFStringCreateWithCharacters() and CFRelease().

Comment 29

[Ralph Giles (:rillian)]

(In reply to Steven Michaud from comment #27)

> Chris:  I'd like to test Lucas's patch, but I don't know where to find
> examples to test with.  Could you list a few?

The issue here is just that the screen goes to sleep (or activates the screensaver) while a video is playing. I run into it all the time with the youtube html player.

Set your screensaver to activate in 1 minute, or a similar short time, and then leave an html5 video playing for longer than that without touching the keyboard or mouse. If the screensaver doesn't come on, the patch is working. Here's any example video you can use: https://people.xiph.org/~giles/2013/02-Digital_Show_and_Tell.html

Then make sure it goes to sleep again if you stop the video. There may be more subtle things things, like whether autoplay for script invocations trigger the wakelock. I haven't tested those.

Comment 30

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

What Ralph said; just play an HTML video, and wait for the screensaver to kick in. If the patch works, the screen saver should not kick in.

Comment 31

[Lucas GOLVEN]

Attachment: bug-772347.patch

Thanks for your review Steven.
I tried to change the patch as you suggested. I hope it's ok now.

I just want to add that I don't want to take all the credit for this patch. We are a french student team composed of Corentin Cos, Benjamin Michel, Nicolas Vignes, myself and last but not least Othman Darraz ;)

Comment 32

[Steven Michaud [:smichaud] (Retired)]

I should be able to get to this tomorrow, Lucas.  Sorry for the delay.

Comment 33

[Lucas GOLVEN]

No problem, I understand ;)

Comment 34

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8438392 [details] [diff] [review]
bug-772347.patch

This looks fine to me (save for one more style nit).  And my manual tests worked perfectly.

I separately set the "Start screen saver" and "Display sleep" to their lowest possible values, then played the HTML5 video from comment #29 (in a separate tab).  The screen saver and display sleep didn't kick in when they normally would have.  But when I closed the tab (and therefore stopped the video), they did kick in normally.

Here's the style nit:

+
+        CFStringRef cf_topic =
+          ::CFStringCreateWithCharacters(kCFAllocatorDefault,
+                                         reinterpret_cast<const UniChar*>
+                                           (aTopic.Data()),
+                                         aTopic.Length());
+

Please remove the blank lines above and below this function call, and attach a new patch (so that someone else can land it).

Thanks for your work on this!

Comment 35

[Lucas GOLVEN]

Attachment: bug-772347.patch

I've done the small change you wanted me to do.
Thanks you very much Steven for the reviews. 
We are proud to help Mozilla, as small as our contribution is ! :)

Comment 36

[Ralph Giles (:rillian)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d437a1b97e7c

Further nits:

* Your patch had no commit message. I wrote a minimal one, but please do something similar for your future patches.

* Your patch added trailing whitespace. I removed that for you, but please check for this as well before submitting. 'git' and 'hg export' will highlight whitespace issues in red. 'git' will also warn you on commit.

Comment 37

[Ralph Giles (:rillian)]

Attachment: Patch with fixed commit message.

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/064a0120db2d

This version doesn't compile:

/builds/slave/m-in-osx64_g-00000000000000000/build/widget/cocoa/nsAppShell.mm:209:1: error: no template named 'StaticRefPtr'; did you mean 'mozilla::StaticRefPtr'?

I've attached the version I committed with the fixed commit message so you can continue from there.

Comment 38

[Steven Michaud [:smichaud] (Retired)]

> This version doesn't compile:

Odd.  It compiled fine for me, earlier today.

Comment 39

[Steven Michaud [:smichaud] (Retired)]

> Odd.  It compiled fine for me, earlier today.

But I landed the patch against m-c code current as of yesterday.

Maybe the patch has bitrotted since then.

Comment 40

[Lucas GOLVEN]

Attachment: bug-772347.patch

This one should work.
Adding "--disable-unified-compilation" in the mozconfig worked to reproduce the compilation error.
There wasn't any error expect the namespace for |StaticRefPtr|.

Comment 41

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8440108 [details] [diff] [review]
bug-772347.patch

This looks good to me.

I made sure that it applies to current trunk code and builds successfully with or without --disable-unified-compilation.

I even tested it again -- it works fine.

Comment 42

[Ralph Giles (:rillian)]

Pushing to try for good measure: https://tbpl.mozilla.org/?tree=Try&rev=ac8029e08a62

Comment 43

[Lucas GOLVEN]

I'm not really familiar with Trys... Everything is green, sounds good ?

Comment 44

[Chris Peterson [:cpeterson]]

Lucas: green is good! :)

Because you added the checkin-needed keyword, someone will land your patch on the mozilla-inbound branch within a day or so.

Comment 45

[Lucas GOLVEN]

Cool, Thanks Chris :)

Comment 46

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dde8abeff07a

Comment 47

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/dde8abeff07a

Comment 48

[Florian Bender]

Is this relnote-worthy?

Comment 49

[Ralph Giles (:rillian)]

We didn't note it when the feature landed for Windows in ff 30. But does seem a reasonable, if minor, thing to mention.

Comment 50

[Catalin Varga [QA][:VarCat]]

I've verified this bug using:

FF 33 Aurora 
Build Id: 20140723004001
OS: Mac Os X 10.8.5

And I've encountered the following issue:
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.

Comment 51

[Ralph Giles (:rillian)]

Thanks for verifying. I'm not sure if that's a feature or a bug, but I've also noticed occasional log messages about not being able to release the wakelock. I filed bug 1043489 for the new issue.

Comment 52

[Catalin Varga [QA][:VarCat]]

The bug was verified using:

FF 32.09 Beta
Build Id: 20141002185629
OS: Mac Os X 10.9.5

Comment 53

[Ralph Giles (:rillian)]

Attachment: feature backout

Back out the feature for Firefox 33 due to bug 104389.

Approval Request Comment
[Feature/regressing bug #]: feature 772347, problem bug 104389
[User impact if declined]: Machine will not sleep after video playback finishes in fullscreen mode, consuming unnecessary power.
[Describe test coverage new/current, TBPL]: Manual test.
[Risks and why]: Risk is moderate. Backout of the feature is straightforward, but there's no time for testing.
[String/UUID change made/needed]: None

Already landed to make rc deadline:
https://hg.mozilla.org/releases/mozilla-release/rev/df37248fafcb

Comment 54

[Ryan VanderMeulen [:RyanVM]]

Target milestone tracks trunk landing. Status flags track doing things on release branches :)

Comment 55

[Ralph Giles (:rillian)]

Thanks for the correction. :)

Comment 56

[Catalin Varga [QA][:VarCat]]

I've checked the FF 33 RC and the back-out was successfully done.

Comment 57

[Liz Henry (:lizzard) (relman/hg->git project)]

Release Note Request (optional, but appreciated)
[Why is this notable]: 
[Suggested wording]: Fullscreen video on Mac disables display sleep, and dimming, during playback.
[Links (documentation, blog post, etc)]:

I don't have any particular documentation for this. I thought it was annoying enough that users might like to hear that it's been fixed.

Comment 58

[Catalin Varga [QA][:VarCat]]

Verified as fixed using the following environment:

FF 34.0b8
Build Id:20141110195804
OS: Mac Os X 10.9.5