Closed Bug 772347 Opened 7 years ago Closed 5 years ago

Add screen WakeLockListener on MacOSX to disable screensaver

Categories

(Core :: Audio/Video, defect)

x86
macOS
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla33
Tracking Status
firefox33 --- verified disabled
firefox34 --- verified
relnote-firefox --- 34+

People

(Reporter: ehsan, Assigned: l.golven)

References

Details

(Keywords: feature, relnote, verifyme)

Attachments

(3 files, 4 obsolete files)

I noticed this on Mac, not sure if it happens on other platforms.
Dupe of bug 517870? Or is the password screen on the mac different?
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.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 517870
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.
I have screen saver disabled, so my mac goes directly to the password screen after 30 minutes.
> 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".
(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.
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?
(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.
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.
(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/>
> So should we reopen this?

Yes, let's do that.

I need to dig into this further to figure out its full implications.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Status: REOPENED → NEW
Summary: Playing video in full screen mode should prevent the password screen to show up → Fullscreen video should disable display sleep during playback
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 .
Depends on: 786893
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.
Depends on: 739542
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).
Blocks: 517870
Summary: Fullscreen video should disable display sleep during playback → Add screen WakeLockListener on MacOSX to disable screensaver
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?
Assignee: nobody → giles
I'm happy to look at this, but I've been asked to finish bug 941296 first.
Duplicate of this bug: 1013255
Do you steel work on this bug ? Do you want us to continue what you have done ?
Flags: needinfo?(giles)
I won't get to it for a few weeks yet, so if you want to take it, please go ahead.
Flags: needinfo?(giles)
Attached patch bug-772347-fix.patch (obsolete) — Splinter Review
Is it possible to be assigned ? Can you review the patch ?
Flags: needinfo?(giles)
Assignee: giles → l.golven
Comment on attachment 8436316 [details] [diff] [review]
bug-772347-fix.patch

Steven, would you mind having a look at this?
Attachment #8436316 - Flags: review?(smichaud)
I should be able to get to this in a few days.  Ping me if I forget :-)
Attached patch bug-772347.patch (obsolete) — Splinter Review
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.
Attachment #8436316 - Attachment is obsolete: true
Attachment #8436316 - Flags: review?(smichaud)
Attachment #8437506 - Flags: review?(smichaud)
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 :-)
(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?
Flags: needinfo?(cpearce)
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().
(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.
Flags: needinfo?(giles)
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.
Flags: needinfo?(cpearce)
Attached patch bug-772347.patch (obsolete) — Splinter Review
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 ;)
Attachment #8437506 - Attachment is obsolete: true
Attachment #8437506 - Flags: review?(smichaud)
Attachment #8438392 - Flags: review?(smichaud)
I should be able to get to this tomorrow, Lucas.  Sorry for the delay.
No problem, I understand ;)
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!
Attachment #8438392 - Flags: review?(smichaud) → review+
Attached patch bug-772347.patch (obsolete) — Splinter Review
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 ! :)
Attachment #8438392 - Attachment is obsolete: true
Attachment #8440021 - Flags: review?(smichaud)
Attachment #8440021 - Flags: review?(smichaud) → review+
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.
Keywords: checkin-needed
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.
Attachment #8440021 - Attachment is obsolete: true
> This version doesn't compile:

Odd.  It compiled fine for me, earlier today.
> 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.
Attached patch bug-772347.patchSplinter Review
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|.
Attachment #8440108 - Flags: review?(smichaud)
Attachment #8440108 - Flags: checkin?(giles)
Keywords: checkin-needed
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.
Attachment #8440108 - Flags: review?(smichaud) → review+
I'm not really familiar with Trys... Everything is green, sounds good ?
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.
Cool, Thanks Chris :)
Attachment #8440108 - Flags: checkin?(giles) → checkin+
https://hg.mozilla.org/mozilla-central/rev/dde8abeff07a
Status: NEW → RESOLVED
Closed: 7 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Keywords: verifyme
Is this relnote-worthy?
Keywords: feature, relnote
We didn't note it when the feature landed for Windows in ff 30. But does seem a reasonable, if minor, thing to mention.
Depends on: 1028021
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.
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.
The bug was verified using:

FF 32.09 Beta
Build Id: 20141002185629
OS: Mac Os X 10.9.5
Status: RESOLVED → VERIFIED
Attached patch feature backoutSplinter Review
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
Attachment #8500563 - Flags: approval-mozilla-release?
Target Milestone: mozilla33 → mozilla34
Attachment #8500563 - Flags: approval-mozilla-release? → approval-mozilla-release+
Target milestone tracks trunk landing. Status flags track doing things on release branches :)
Target Milestone: mozilla34 → mozilla33
Thanks for the correction. :)
I've checked the FF 33 RC and the back-out was successfully done.
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.
relnote-firefox: --- → ?
Verified as fixed using the following environment:

FF 34.0b8
Build Id:20141110195804
OS: Mac Os X 10.9.5
You need to log in before you can comment on or make changes to this bug.