Last Comment Bug 811261 - Fullscreen video should disable screensaver during playback on Linux
: Fullscreen video should disable screensaver during playback on Linux
Status: RESOLVED FIXED
[Advo]
:
Product: Toolkit
Classification: Components
Component: Video/Audio Controls (show other bugs)
: Trunk
: All Linux
: -- enhancement with 3 votes (vote)
: mozilla33
Assigned To: Edwin Flores [:eflores] [:edwin]
: Catalin Varga [QA][:VarCat]
Mentors:
Depends on: 697132 739542 805017 963366 1028913
Blocks: cuts-control 453063 486276 517870
  Show dependency treegraph
 
Reported: 2012-11-13 03:34 PST by Martin Stránský
Modified: 2015-05-25 06:00 PDT (History)
70 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP (6.60 KB, patch)
2012-11-14 04:43 PST, Martin Stránský
no flags Details | Diff | Review
v1 (7.16 KB, patch)
2012-11-14 07:00 PST, Martin Stránský
karlt: feedback+
Details | Diff | Review
WIP (10.28 KB, patch)
2012-12-13 07:26 PST, Martin Stránský
no flags Details | Diff | Review
v2 (10.98 KB, patch)
2012-12-18 04:40 PST, Martin Stránský
karlt: review-
karlt: feedback+
justin.lebar+bug: feedback-
Details | Diff | Review
Implement WakeLockListener on Linux to disable screensaver while video is playing (12.19 KB, patch)
2014-04-03 21:38 PDT, Edwin Flores [:eflores] [:edwin]
karlt: review-
cpearce: feedback+
Details | Diff | Review
Implement WakeLockListener on Linux to disable screensaver while video is playing (11.70 KB, patch)
2014-04-09 14:13 PDT, Edwin Flores [:eflores] [:edwin]
no flags Details | Diff | Review
Implement WakeLockListener on Linux to disable screensaver while video is playing (11.70 KB, patch)
2014-04-09 14:14 PDT, Edwin Flores [:eflores] [:edwin]
karlt: review-
Details | Diff | Review
Implement WakeLockListener on Linux to disable screensaver while video is playing (14.01 KB, patch)
2014-04-30 16:53 PDT, Edwin Flores [:eflores] [:edwin]
karlt: review-
Details | Diff | Review
Implement WakeLockListener on Linux to disable screensaver while video is playing (12.98 KB, patch)
2014-05-08 21:35 PDT, Edwin Flores [:eflores] [:edwin]
karlt: review-
Details | Diff | Review
Implement WakeLockListener on Linux to disable screensaver while video is playing (14.70 KB, patch)
2014-05-18 16:51 PDT, Edwin Flores [:eflores] [:edwin]
karlt: review-
Details | Diff | Review
Implement WakeLockListener on Linux to disable screensaver while video is playing (14.68 KB, patch)
2014-05-19 11:05 PDT, Edwin Flores [:eflores] [:edwin]
karlt: review+
Details | Diff | Review
patch v.4 (14.70 KB, patch)
2014-06-17 06:46 PDT, Martin Stránský
karlt: review-
karlt: feedback+
Details | Diff | Review
patch v.5 (14.67 KB, patch)
2014-06-18 06:11 PDT, Martin Stránský
karlt: review+
cbook: checkin+
Details | Diff | Review

Description Martin Stránský 2012-11-13 03:34:43 PST
+++ This bug was initially created as a clone of Bug #517870 +++

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.11; .NET CLR 2.0.50727; ffco7) Gecko/2009060215 Firefox/3.0.11
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.11; .NET CLR 2.0.50727; ffco7) Gecko/2009060215 Firefox/3.0.11

During playback, fullscreen video should disable the screensaver to allow watching arbitrarily long videos uninterrupted.  This is a feature that is common in native player and would be a leg up on flash-based players.

Reproducible: Always




There is a registry key that could be used for this purpose on Windows, but I'm not sure I like it for the same reasons given here: http://mailman.videolan.org/pipermail/vlc-devel/2008-December/054231.html but the workaround they came up with, while perhaps a bit hacky, would work fine here too.
Comment 1 Martin Stránský 2012-11-14 04:43:26 PST
Created attachment 681449 [details] [diff] [review]
WIP

WIP. Unfortunately the D-Bus interface seems to be changed so the Inhibit method does not work.
Comment 2 Martin Stránský 2012-11-14 07:00:09 PST
Created attachment 681480 [details] [diff] [review]
v1

A working one, via. Session Manager & D-Bus.
Comment 3 Karl Tomlinson (ni?:karlt) 2012-11-20 20:45:48 PST
Comment on attachment 681480 [details] [diff] [review]
v1

IIUC this will inhibit the screensaver, even if there is an advert or audio
playing out of view on any foreground tab of an unminimized window.

I'm not sure it is reasonable to require users to minimize their windows to
allow the screensaver to activate.  Perhaps there is some other mechanism to
prevent media playing until the user interacts.  Better get feedback from the
media team on this.  Does Gnome Shell let you minimize windows?  Does it unmap
windows on background virtual desktops?

  Don't know whether only considering the active window would be better.  On
  Android, I assume it doesn't make much difference because only one window
  will be visible at a time.  At least considering only active windows would
  filter out windows on other virtual desktops.  But the same question still
  remains about whether leaving an active window visible should allow the site
  to disable the screensaver.

  Seems we should only be disabling the screensaver on some user action,
  such as asking for (or allowing fullscreen).

Does the screensaver that you work with support org.freedesktop.ScreenSaver?
That sounds like the preferred API to use, but I haven't found official
documentation.  It seems very similar to org.gnome.ScreenSaver.
http://lists.freedesktop.org/archives/xdg/2007-March/009187.html

Using a GNOME interface is OK if the freedesktop interface is not supported
and using the GNOME interface doesn't cause any new processes to start (in
sessions with different managers).  What is the reason to prefer
gnome.SessionManager over gnome.ScreenSaver?

reason = "Fullscreen mode" isn't matching up with the conditions of calling
Inhibit.  I agree fullscreen mode seems the more appropriate condition under
which to disable the screensaver.

We don't want to wait for the DBus server when starting to play, so the calls
should be asynchronous.  Blocking on the inhibit reply before uninhibit should
be OK.

  When not using dbus_connection_send_with_reply_and_block, something will be
  necessary to actually flush the dbus send queue.  dbus_connection_flush()
  doesn't seem quite ideal because it "blocks until it can write out the
  entire outgoing queue."  I'm guessing dbus-glib's
  dbus_connection_setup_with_g_main will be the easiest way to look after
  that.  We perhaps could get lucky that dbus_connection_setup_with_g_main has
  already been called elsewhere, but better to ensure it is called by adding
  another call here.  Feel free to use other dbus-glib API if that makes
  things simpler.  I don't really mind whether libdbus or dbus-glib functions
  are used because both are superseded or deprecated by GIO's GDBus, and GDBus
  is too new to use.

The concept of activating a WakeLock on notification that the foreground has
already been locked seems quite strange to me, but it does follow the Android
implementation.  Best to get feedback from the author or reviewer of those
changes because there are some things that I don't understand:

  There doesn't seem to be any documentation on what a Wakelock is for, what
  the topic is used for, or the difference between hidden and active locks.

  The android-specific listener provides for concurrent locks of different
  topic/tag.

  Why is ModifyWakeLock implemented in the hardware abstraction layer when it
  doesn't interact with the hardware?
Comment 4 Martin Stránský 2012-11-27 07:16:16 PST
(In reply to Karl Tomlinson (:karlt) from comment #3)
> IIUC this will inhibit the screensaver, even if there is an advert or audio
> playing out of view on any foreground tab of an unminimized window.

I tested it and it inhibits the screensaver in fullscreen mode only? Why do you think it affects non-fullscreen playback too? 

> Does the screensaver that you work with support org.freedesktop.ScreenSaver?
> That sounds like the preferred API to use, but I haven't found official
> documentation.  It seems very similar to org.gnome.ScreenSaver.
> http://lists.freedesktop.org/archives/xdg/2007-March/009187.html

org.freedesktop.ScreenSaver is outdated and does not work. The SessionManager is used by gnome itself (by http://git.gnome.org/browse/gtk+/tree/gtk/gtkapplication.c#n1412).
Comment 5 Martin Stránský 2012-11-27 07:18:12 PST
Sorry, I mean it inhibits the screen saver only in fullscreen, not when the video is embeded.
Comment 6 Karl Tomlinson (ni?:karlt) 2012-11-27 12:17:52 PST
(In reply to Martin Stránský from comment #4)
> (In reply to Karl Tomlinson (:karlt) from comment #3)
> > IIUC this will inhibit the screensaver, even if there is an advert or audio
> > playing out of view on any foreground tab of an unminimized window.
> 
> I tested it and it inhibits the screensaver in fullscreen mode only? Why do
> you think it affects non-fullscreen playback too?

The code added in bug 739542 looks like it adds a WakeLock with a "Playing_media" tag whenever an nsHTMLMediaElement is not paused.

I'm assuming nsHTMLMediaElement is used for non-fullscreen media.

> > Does the screensaver that you work with support org.freedesktop.ScreenSaver?
> > That sounds like the preferred API to use, but I haven't found official
> > documentation.  It seems very similar to org.gnome.ScreenSaver.
> > http://lists.freedesktop.org/archives/xdg/2007-March/009187.html
> 
> org.freedesktop.ScreenSaver is outdated ...

KDE people may contest that.  It would seem strange to switch from a freedesktop interface to a gnome interface.

> ... and does not work.

But if org.freedesktop.ScreenSaver doesn't work we do need something else.

> The SessionManager is used by gnome itself (by
> http://git.gnome.org/browse/gtk+/tree/gtk/gtkapplication.c#n1412).

It is a shame that GTK3 now depends on GNOME.
But this does look like evidence that org.gnome.SessionManager is preferred over org.gnome.ScreenSaver, thanks.
Comment 7 Martin Stránský 2012-11-28 02:39:46 PST
> But if org.freedesktop.ScreenSaver doesn't work we do need something else.

Ahh, sorry. org.gnome.ScreenSaver is outdated and does not work. org.freedesktop.ScreenSaver seems to run but on KDE only, it's not exposed by GTK/Gnome.

So I'm not sure how to implement it, because KDE uses org.freedesktop.ScreenSaver and org.gnome.SessionManager. It's shame that org.freedesktop.ScreenSaver is not implemented by Gnome.
Comment 8 Christian Fredrik Kalager Schaller 2012-11-28 05:03:07 PST
I checked around a bit. So there is no 'freedesktop' interface. The so called freedesktop interface was a proposal from some KDE developers that never got adopted as a freedesktop standard, but KDE kept 'freedesktop' in their DBus API name which one could argue they shouldn't have done.
Comment 9 Bastien Nocera 2012-11-28 10:13:44 PST
I've implemented the "freedesktop"[1] interface in GNOME, it will be available for GNOME 3.6 (the current version) and later[2].

Please use this (the inhibit/uninhibit calls) to implement the screensaver inhibition in Firefox.

[1]: https://projects.kde.org/projects/kde/kde-workspace/repository/revisions/master/entry/ksmserver/screenlocker/dbus/org.freedesktop.ScreenSaver.xml
[2]: https://bugzilla.gnome.org/show_bug.cgi?id=689225
Comment 10 Martin Stránský 2012-12-13 07:26:31 PST
Created attachment 691815 [details] [diff] [review]
WIP

Adds FreeDesktop interface with SessionManager fallback.
Comment 11 Martin Stránský 2012-12-18 04:40:11 PST
Created attachment 693331 [details] [diff] [review]
v2

Katl, what do you think about this one? The Inhibit() call is async. I use FreeDesktop interface with SessionManager fallback. Only the Fullscreen topic is handled.

I'm not sure about the questions of ModifyWakeLock/Hal...shall we ask someone who works on that?
Comment 12 Karl Tomlinson (ni?:karlt) 2013-01-07 20:29:07 PST
Comment on attachment 693331 [details] [diff] [review]
v2

How about disabling the screensaver only when "DOM_Fullscreen" and
"Playing_media" are both in "locked-foreground" state?
That would still lock the screen while a fullscreen window is playing audio,
but at least that reduces the number of situations considerably.

I don't want any DBus calls blocking on reply from the service, especially
during nsAppShell::Init(), which would be called during start-up.

I think it would be simplest, instead of testing different methods during
initialization, to wait until after calling "Inhibit" on the freedesktop
interface first.  If an error is received from that call, then set some state to indicate that the freedesktop interface is not available and try again.  When the freedesktop interface is not available, the gnome interface is tried.  If that also fails, the state could even be updated to so that no interfaces are tried next time.

These situations need to be handled:  After a screenSaverInhibit() call,
WakeLockListener::Callback() is called with a "locked-foreground" or other
state before WakeLockListenerStatusNotify() receives the cookie for
mInhibitRequest.

>+LOCAL_INCLUDES  += $(MOZ_DBUS_CFLAGS)
>+CFLAGS          += $(MOZ_DBUS_GLIB_CFLAGS)
>+CXXFLAGS        += $(MOZ_DBUS_GLIB_CFLAGS) -DHAVE_PTHREADS

Do you know why -DHAVE_PTHREADS is here?  I see one other instance of it
beside MOZ_DBUS_GLIB_CFLAGS in Gecko, but other places don't have it.
Please remove if it is not required.

I expect MOZ_DBUS_CFLAGS is unnecessary with MOZ_DBUS_GLIB_CFLAGS.

>+#include <dbus/dbus-glib.h>

Looks like this may be unnecessary.

>+        if(!topic.Equals(NS_LITERAL_STRING("DOM_Fullscreen")))

EqualsLiteral

Remember Gecko style is "if (".

>+nsCOMPtr<nsIPowerManagerService> sPowerManagerService = nullptr;
>+nsCOMPtr<nsIDOMMozWakeLockListener> sWakeLockListener = nullptr;

nsCOMPtr should not be used for global variables.  (It runs constructors
at startup.)  I don't know whether or not StaticRefPtr would work here.
Comment 13 Karl Tomlinson (ni?:karlt) 2013-01-07 20:39:50 PST
Comment on attachment 693331 [details] [diff] [review]
v2

Justin, can you check that the use of nsIDOMMozWakeLockListener is as intended, and comment on whether the suggestion in comment 12 of listening for two topics is workable, please?  I had trouble working out how the WakeLock code was meant to be used - see end of comment 3.
Comment 14 Justin Lebar (not reading bugmail) 2013-01-08 02:22:54 PST
Comment on attachment 693331 [details] [diff] [review]
v2

You got the wake lock API totally right except for one key point: Wake locks
are not trusted.  Any content can ask for a wake lock on anything.

As currently designed, the fact that the "DOM_Fullscreen" wake lock is held
tells you absolutely nothing; any webpage could hold that wake lock by doing
navigator.requestWakeLock("DOM_Fullscreen").

I see that this mistake is all over the place.  :(  I'll try to get a handle on
the bustage and file bugs.

>+NS_IMPL_ISUPPORTS1(WakeLockListener, nsIDOMMozWakeLockListener)
>+nsCOMPtr<nsIPowerManagerService> sPowerManagerService = nullptr;
>+nsCOMPtr<nsIDOMMozWakeLockListener> sWakeLockListener = nullptr;

A much more minor point: static nsCOMPtr/nsRefPtr/nsAutoPtr is an anti-pattern.
It creates a static constructor, which slows startup.

You can use |static StaticRefPtr|, but it's probably better to use members on nsAppShell.

But more specifically, I don't think you need either of these refs.  The power
manager service will keep a strong ref to the wake lock listener, so you can
just register the listener and be done with it.
Comment 15 Justin Lebar (not reading bugmail) 2013-01-08 03:12:12 PST
I filed bug 827756 for the wake lock issues.
Comment 16 Martin Stránský 2013-01-11 04:28:09 PST
(In reply to Justin Lebar [:jlebar] from comment #14)
> Comment on attachment 693331 [details] [diff] [review]
> v2
> 
> You got the wake lock API totally right except for one key point: Wake locks
> are not trusted.  Any content can ask for a wake lock on anything.

Justin, thanks for the clarification. So how we can get notification about the "screen" wake lock you're talking about in Bug 827756? I guess it should be driven by GeckoApp.java, right?
Comment 17 Justin Lebar (not reading bugmail) 2013-01-13 23:28:12 PST
> So how we can get notification about the "screen" wake lock you're talking about in Bug 
> 827756?

I'm not sure the screen wake lock is what you want here, though, since in its current form, any page can acquire the "screen" wake lock and keep the screensaver from appearing.

I think you want a /trusted/ wake lock coming from Gecko, which is not a concept which currently exists in the API.  I think I'd support adding something like that, if someone wanted to implement it.

Anywho, to answer your question, if you just replace

> +        if(!topic.Equals(NS_LITERAL_STRING("DOM_Fullscreen")))

with "screen", you'll listen for the "screen" wake lock.  At least, if I follow the code correctly (it's a bit tricky for me to follow since I'm not familiar with the dbus stuff).
Comment 18 Martin Stránský 2013-01-14 06:14:59 PST
(In reply to Justin Lebar [:jlebar] from comment #17)
> I think you want a /trusted/ wake lock coming from Gecko, which is not a
> concept which currently exists in the API.  I think I'd support adding
> something like that, if someone wanted to implement it.

Yes, it looks like something we need. I volunteer to implement it if you give me some hints how to design it. The untrusted locks from web pages is not a good idea.
Comment 19 Justin Lebar (not reading bugmail) 2013-01-14 07:37:04 PST
> I volunteer to implement it if you give me some hints how to design it.

Thanks!

If you file a new bug I'll be happy to help out.
Comment 20 Martin Stránský 2013-01-15 01:28:10 PST
Filed as Bug 830660.
Comment 21 Chris Pearce (:cpearce) 2014-02-06 16:40:56 PST
(In reply to Justin Lebar (not reading bugmail) from comment #14)
> Comment on attachment 693331 [details] [diff] [review]
> v2
> 
> You got the wake lock API totally right except for one key point: Wake locks
> are not trusted.  Any content can ask for a wake lock on anything.

Is this still the case? According to the documentation:
https://developer.mozilla.org/en-US/docs/WebAPI/Power_Management
Wake locks are only available to privileged apps. Indeed on desktop navigator.mozPower isn't visible, so I don't see why we should be worrying about trusted screen locks for desktop builds here.

mounir: It was suggested you understand wake locks, do we still need the trusted wake lock service Martin implemented in bug 830660? If not I'd like to see the patch here landed ASAP.
Comment 22 Chris Pearce (:cpearce) 2014-02-06 16:54:01 PST
mounir no longer works for us, so may not be checking bugmail.

kanru: you wrote the WakeLock, can you answer my question to mounir in comment 21 please?

I'm trying to figure out if there's any reason why we can use "screen" wake lock listeners on desktop to disable the screen saver while video is playing. We're currently doing this in B2G, so I don't see why we shouldn't for Linux (and other desktop platforms) too.

Justin Lebar previously said we needed a "trusted" wake lock to be implemented, but I don't understand why that should be necessary now, since mozPower is only accessible to content in privileged apps.
Comment 23 Chris Pearce (:cpearce) 2014-02-06 16:57:52 PST
Comment on attachment 693331 [details] [diff] [review]
v2

Review of attachment 693331 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/gtk2/nsAppShell.cpp
@@ +167,5 @@
> +    {        
> +        if(!mConnection)
> +            return NS_ERROR_FAILURE;
> +    
> +        if(!topic.Equals(NS_LITERAL_STRING("DOM_Fullscreen")))

I don't think we should only disable the screen saver when we're playing fullscreen video. I think we should do it whenever video is playing, provided playback was initiated in a user generated event handler.

I'll write a patch to ensure that we only send the notification if we're in a user generated event handler. In the meantime, we can make the topic here "screen".
Comment 24 Chris Pearce (:cpearce) 2014-02-06 17:25:21 PST
Actually, I don't think that only disabling the screen saver in a user generated event handler would be a good experience. Some videos we'd want to disable the screen saver on start automatically. Like YouTube videos, they start playing automatically.
Comment 25 antistress 2014-02-07 19:53:39 PST
Maybe disabling the screen could depend wether sounds are played or not ?
Comment 26 Kan-Ru Chen [:kanru] (UTC+8) 2014-02-08 18:51:31 PST
(In reply to Chris Pearce (:cpearce) from comment #22)
> mounir no longer works for us, so may not be checking bugmail.
> 
> kanru: you wrote the WakeLock, can you answer my question to mounir in
> comment 21 please?
> 
> I'm trying to figure out if there's any reason why we can use "screen" wake
> lock listeners on desktop to disable the screen saver while video is
> playing. We're currently doing this in B2G, so I don't see why we shouldn't
> for Linux (and other desktop platforms) too.
> 
> Justin Lebar previously said we needed a "trusted" wake lock to be
> implemented, but I don't understand why that should be necessary now, since
> mozPower is only accessible to content in privileged apps.

It's not about mozPower but navigator.requestWakeLock. Currently any web page could request a wake lock and keep screensaver from appearing. Note I'm going to land a patch for bug 963366 that hide wake locks from the web so using it in gecko is safe but this might change in the future.
Comment 27 Chris Pearce (:cpearce) 2014-02-09 13:39:55 PST
I think we should go ahead and try to get the wake lock listener here cleaned up and landed.
Comment 28 Edwin Flores [:eflores] [:edwin] 2014-04-03 21:38:22 PDT
Created attachment 8401684 [details] [diff] [review]
Implement WakeLockListener on Linux to disable screensaver while video is playing
Comment 29 Chris Pearce (:cpearce) 2014-04-06 15:38:39 PDT
Comment on attachment 8401684 [details] [diff] [review]
Implement WakeLockListener on Linux to disable screensaver while video is playing

Review of attachment 8401684 [details] [diff] [review]:
-----------------------------------------------------------------

This is the behaviour we want (locking video irrespective of whether we're fullscreen or not), but someone who has a clue about Linux should also review this. Karl?

::: widget/gtk/nsAppShell.cpp
@@ +93,5 @@
> +                                           FREEDESKTOP_SCREENSAVER_OBJECT,
> +                                           FREEDESKTOP_SCREENSAVER_INTERFACE,
> +                                           p_method);
> +        }
> +        else {

} else {

@@ +120,5 @@
> +                                     DBUS_TYPE_STRING, &application,
> +                                     DBUS_TYPE_STRING, &reason,
> +                                     DBUS_TYPE_INVALID);
> +        }
> +        else {

} else {

@@ +391,1 @@
>      return nsBaseAppShell::Init();

You should probably remove the listener during shutdown too.
Comment 30 Karl Tomlinson (ni?:karlt) 2014-04-06 20:20:47 PDT
Comment on attachment 8401684 [details] [diff] [review]
Implement WakeLockListener on Linux to disable screensaver while video is playing

># User Edwin Flores <eflores@mozilla.com>

I assume you built on Martin's patch here.  Please include him as an author.

>+LOCAL_INCLUDES  += $(MOZ_DBUS_CFLAGS)
>+CFLAGS          += $(MOZ_DBUS_GLIB_CFLAGS)
>+CXXFLAGS        += $(MOZ_DBUS_GLIB_CFLAGS) -DHAVE_PTHREADS

Which parts of previous reviews have not been addressed and why not?
Comment 31 Edwin Flores [:eflores] [:edwin] 2014-04-09 14:12:01 PDT
(In reply to Chris Pearce (:cpearce) from comment #29)
> @@ +391,1 @@
> >      return nsBaseAppShell::Init();
> 
> You should probably remove the listener during shutdown too.

Shouldn't be neccessary. The WakeLockListener doesn't depend on anything else being alive, and as jlebar noted, it will be destroyed with the power manager service anyway.
Comment 32 Edwin Flores [:eflores] [:edwin] 2014-04-09 14:13:14 PDT
(In reply to Karl Tomlinson (:karlt) from comment #30)
> >+LOCAL_INCLUDES  += $(MOZ_DBUS_CFLAGS)
> >+CFLAGS          += $(MOZ_DBUS_GLIB_CFLAGS)
> >+CXXFLAGS        += $(MOZ_DBUS_GLIB_CFLAGS) -DHAVE_PTHREADS
> 
> Which parts of previous reviews have not been addressed and why not?

Oops, totally missed most of your review comments. New patch incoming.
Comment 33 Edwin Flores [:eflores] [:edwin] 2014-04-09 14:13:28 PDT
Created attachment 8404228 [details] [diff] [review]
Implement WakeLockListener on Linux to disable screensaver while video is playing
Comment 34 Edwin Flores [:eflores] [:edwin] 2014-04-09 14:14:53 PDT
Created attachment 8404230 [details] [diff] [review]
Implement WakeLockListener on Linux to disable screensaver while video is playing
Comment 35 Karl Tomlinson (ni?:karlt) 2014-04-22 15:54:21 PDT
Comment on attachment 8404230 [details] [diff] [review]
Implement WakeLockListener on Linux to disable screensaver while video is playing

The main thing missing here is appropriate handling of this situation: 
After a InhibitScreenSaver() call, WakeLockListener::Callback() is called with
a non-"locked-foreground" state before WakeLockListenerStatusNotify() receives
the cookie for mInhibitRequest.  The Uninhibit message can't be sent until the
cookie is received in the reply from Inhibit.

The other things below should be easier to resolve.

>+CFLAGS          += $(MOZ_DBUS_GLIB_CFLAGS)

This line can be dropped because only cpp files are involved here.

>+using namespace mozilla;
> 
> using mozilla::unused;

I assume the first line here makes the other unnecessary.

>+    char const *const sReason = "Media Playback";

This is generic code inhibiting the screen saver on any "topic" in
locked-foreground state, so mention of "Media Playback" seems strange.  It
seems that either Callback() should listen only for the "Playing_media" topic
or should pass on each topic to the screen saver control (the cookie means
that multiple inhibit/unhibit pairs will be handled appropriately).

>+            mDesktopEnvironment++;

Re a postfix ++ expression, N3242 says "The type of the operand shall be an
arithmetic type or a pointer to a complete object type."

gcc 4.7.3 says:

> /home/karl/moz/dev/widget/gtk/nsAppShell.cpp: In member function 'void WakeLockListener::InhibitFailed()':
> /home/karl/moz/dev/widget/gtk/nsAppShell.cpp:156:32: error: no 'operator++(int)' declared for postfix '++' [-fpermissive]

>+    void Init(void)
>+    {
>+        if (mConnection) {
>+            dbus_connection_setup_with_g_main(mConnection, nullptr);
>+        }
>+    }

The dbus_connection_set_exit_on_disconnect() call seems to have been lost.
If there is no lazy initialization, then there is no need to have a separate
Init() function and moving the code into the constructor would make it clearer
that this can only happen once.

>+      GNOME,
>+      FreeDesktop,
>+      Unsupported,

This is now a freedesktop spec and implemented on GNOME (comment 9) so try the
FreeDesktop interface first and fall back to GNOME if not available.
I assume the GNOME interface will be deprecated at some point.
http://specs.freedesktop.org/idle-inhibit-spec/latest/re01.html

>+    DBusConnection*         mConnection;

This needs a dbus_connection_unref(mConnection).

>+    // Keep track of all the topics that have requested a wake lock. When the
>+    // number of topics in the hashtable reaches zero, we can uninhibit the
>+    // screensaver again.
>+    nsTHashtable<nsStringHashKey> mLockedTopics;

If only one topic is handled, then this hash table should no longer be
necessary.
If multiple topics are handled then this could hold the cookies.

>+    nsCOMPtr<nsIPowerManagerService> powerManagerService;
>+
>     // make the pipe nonblocking

>+    powerManagerService = do_GetService(POWERMANAGERSERVICE_CONTRACTID);

Please declare at the initialization.
Comment 36 Edwin Flores [:eflores] [:edwin] 2014-04-30 16:53:15 PDT
Created attachment 8415668 [details] [diff] [review]
Implement WakeLockListener on Linux to disable screensaver while video is playing
Comment 37 Edwin Flores [:eflores] [:edwin] 2014-05-07 15:59:31 PDT
Comment on attachment 8415668 [details] [diff] [review]
Implement WakeLockListener on Linux to disable screensaver while video is playing

Review of attachment 8415668 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/gtk/WakeLockListener.cpp
@@ +154,5 @@
> +    }
> +  case Unsupported:
> +    return false;
> +  }
> +}

Oops, need a |return false| here to keep GCC happy.
Comment 38 Karl Tomlinson (ni?:karlt) 2014-05-07 19:05:40 PDT
Comment on attachment 8415668 [details] [diff] [review]
Implement WakeLockListener on Linux to disable screensaver while video is playing

All the DBus callbacks should be happening on the main thread unless someone
calls dbus_connection_setup_with_g_main() with an off-main-thread
context, which would be scary.  This means that the ReentrantMonitor can be removed, and there are also some other simplifications possible due to things that won't happen.

There is still a problem if shouldLock changes to true, then false, then true,
before any replies are received.  Two inhibit requests would end up in flight
and then only one of the reply cookies would be handled.  I think this
requires state to be tracked a little differently.  Perhaps use one state
variable to record what state has been requested of the WakeLockListener and
another for what state has been requested through DBus interfaces.

>+#include "WakeLockListener.h"
>+
>+#ifdef MOZ_ENABLE_DBUS

WakeLockListener.h includes dbus headers unconditionally, so something needs
change to avoid that when MOZ_ENABLE_DBUS is not set.

Providing a forward declaration for class DBusConnection I think will mean that WakeLockListener.h no longer needs to include any dbus headers.

WakeLockTopic and DesktopEnvironment are used only by the WakeLockListener
implementation and so their declarations can move to the .cpp file to make
this clearer.

>+static char const* const sApplication = "Mozilla Firefox";

g_get_prgname() is a simple way to get something reasonable here.
I'd probably use that unless there is a good reason why
chrome://branding/locale/brand.properties is necessary.
The "reverse domain" suggestion seems to indicate this doesn't need to be human
readable, just an identifier.  I wouldn't bother constructing any reverse
domain hierarchy - we can have a tld.

>+bool
>+WakeLockTopic::SendInhibit()
>+{
>+  NS_ASSERTION(!mInhibitRequest, "Screensaver has already been inhibited");
>+
>+  switch (mDesktopEnvironment)
>+  {
>+  case Unknown:
>+    mDesktopEnvironment = FreeDesktop;
>+  case FreeDesktop:
>+    if (SendFreeDesktopInhibitMessage()) {
>+      return true;
>+    } else {
>+      mDesktopEnvironment = GNOME;
>+    }
>+  case GNOME:
>+    if (SendGNOMEInhibitMessage()) {
>+      return true;
>+    } else {
>+      mDesktopEnvironment = Unsupported;
>+    }
>+  case Unsupported:
>+    return false;
>+  }
>+}

dbus_connection_send_with_reply() may set the pending_return parameter to NULL
on OOM or if the connection is disconnected, but there is no check to see
whether the interface is available.  Therefore Send*() method failure is not
an indication to try a fallback interface.  The FreeDesktop and GNOME cases
can just return the result of the Send*() methods.

>+  NS_ASSERTION(mInhibitRequest || mDesktopEnvironment == Unsupported,
>+               "Screensaver wasn't inhibited");

I liked having these kind of assertions due to the assumptions that
WakeLockListener::Callback() would only be called when the state changed.
However, the documentation for the interfaces doesn't specify any particular
values for the cookie, so a valid cookie could be zero.  A rethink of the
state variables, as discussed at the top of this comment, may make this
assertion easier to make or unnecessary.

>+WakeLockTopic::InhibitFailed()

>+  if (!mWaitingForInhibit) {
>+    // We were interrupted by UninhibitScreensaver() before we could find the
>+    // correct desktop environment.
>+    return;
>+  }

Even if the WakeLock state has changed, this failure indicates that the
interface is not supported and so this test can be moved to after
mDesktopEnvironment is updated.

>+  if (mDesktopEnvironment < Unsupported) {
>+    switch (mDesktopEnvironment) {
>+    case Unknown:
>+      mDesktopEnvironment = FreeDesktop;
>+      break;
>+    case FreeDesktop:
>+      mDesktopEnvironment = GNOME;
>+      break;
>+    case GNOME:
>+      mDesktopEnvironment = Unsupported;
>+      return;
>+    default:
>+      NS_WARNING("Unknown desktop environment");
>+      return;
>+    }

The warning can be an assertion because mDesktopEnvironment is controlled by
this code and so the default case should not happen.

The Unknown case should also not happen because a messages has been sent to
some interface.  The Unknown enumerator is not really used, so I think it
would be simplest to just remove it and initialize mDesktopEnvironment to
FreeDesktop.

If SendInhibit() no longer modifies mDesktopEnvironment, and we have only one method call in flight at a time, then mDesktopEnvironment will never be Unsupported here.  I would just have a single case (perhaps FreeDesktop) and
default, with an assert in the default case that the mDesktopEnvironment is
the expected value (GNOME).

>+WakeLockTopic::InhibitSucceeded(uint32_t aInhibitRequest)
>+{
>+  ReentrantMonitorAutoEnter mon(mMonitor);
>+
>+  NS_ASSERTION(!mInhibitRequest || mDesktopEnvironment == Unsupported,
>+               "Inhibit request handle is already set.");

I expect there will also be no way to get here when mDesktopEnvironment == Unsupported.

>+//#include "nsTHashtable.h"

Can remove this line now.

>+class WakeLockListener : public nsIDOMMozWakeLockListener
>+{
>+public:
>+  NS_DECL_ISUPPORTS;
>+
>+  WakeLockListener();
>+  virtual ~WakeLockListener();

This is a reference counted class, so please make the destructor private.
The (virtual) Release() method is implemented on this class and there are no
derived classes, so the destructor need not be virtual.  Adding MOZ_FINAL
would clarify this. 

>-nsAppShell::EventProcessorCallback(GIOChannel *source, 
>+nsAppShell::EventProcessorCallback(GIOChannel *source,

No unrelated whitespace changes please, even if it is removing unnecessary
whitespace.
Comment 39 Edwin Flores [:eflores] [:edwin] 2014-05-08 21:35:29 PDT
Created attachment 8419908 [details] [diff] [review]
Implement WakeLockListener on Linux to disable screensaver while video is playing
Comment 40 Edwin Flores [:eflores] [:edwin] 2014-05-08 21:35:53 PDT
Comment on attachment 8419908 [details] [diff] [review]
Implement WakeLockListener on Linux to disable screensaver while video is playing

Addressed review comments.
Comment 41 Karl Tomlinson (ni?:karlt) 2014-05-11 23:19:10 PDT
Comment on attachment 8419908 [details] [diff] [review]
Implement WakeLockListener on Linux to disable screensaver while video is playing

>+NS_IMPL_ISUPPORTS1(WakeLockListener, nsIDOMMozWakeLockListener)

Will need to remove the '1' here now.

>+  dbus_pending_call_set_notify(reply, &ReceiveInhibitReply, this, NULL);

|this| needs to stay alive until the reply is received, and I assume that may
be after the power manager service is deleted.

There are probably several ways to handle this: perhaps add a reference to the
WakeLockListener, and remove in free_user_data; or make the WakeLockListener a
singleton and check that it still exists in ReceiveInhibitReply().

>+    DBusError err;
>+    dbus_error_init(&err);

This should be freed if set, but easier is to just pass nullptr.

>+WakeLockTopic::InhibitScreensaver()
>+{
>+  NS_ASSERTION(!mShouldInhibit, "Screensaver is already inhibited");

>+WakeLockTopic::UninhibitScreensaver()
>+{
>+  if (!mShouldInhibit) {
>+    // Screensaver isn't inhibited. Nothing to do here.
>+    return NS_OK;

Could this instead assert that this path is not reached, as in
InhibitScreensaver()?

>+  nsClassHashtable<nsStringHashKey, WakeLockTopic> mTopics;

Topics are never removed from this hashtable (until shutdown).  Is there a
small finite sized set of topics?  Please add a comment stating this
assumption.

>+  virtual ~WakeLockListener() MOZ_FINAL;

By moving the MOZ_FINAL to the class definition, this destructor need not be
virtual.
Comment 42 Edwin Flores [:eflores] [:edwin] 2014-05-13 21:59:06 PDT
(In reply to Karl Tomlinson (needinfo?:karlt) from comment #41)
> >+WakeLockTopic::UninhibitScreensaver()
> >+{
> >+  if (!mShouldInhibit) {
> >+    // Screensaver isn't inhibited. Nothing to do here.
> >+    return NS_OK;
> 
> Could this instead assert that this path is not reached, as in
> InhibitScreensaver()?

No. There are two states "unlocked" and "locked-background" that we map to Uninhibit, that can both be reached directly from "locked-foreground".

Addressing other comments, new patch coming.
Comment 43 Edwin Flores [:eflores] [:edwin] 2014-05-18 16:51:12 PDT
Created attachment 8424521 [details] [diff] [review]
Implement WakeLockListener on Linux to disable screensaver while video is playing
Comment 44 Karl Tomlinson (ni?:karlt) 2014-05-18 19:19:32 PDT
Comment on attachment 8424521 [details] [diff] [review]
Implement WakeLockListener on Linux to disable screensaver while video is playing

>+nsCOMPtr<nsIDOMMozWakeLockListener> WakeLockListener::sSingleton = nullptr;

AFAIK compilers are not clever enough to avoid running a constructor for this
at startup, and so this should be avoided (comment 12 and comment 14).  It
will be used soon after startup anyway, but I don't know the details on how
the different reads benefit from read-ahead.  There are people who have worked
hard to remove similar variables, so let's not add one here.  I guess
either StaticRefPtr or manual ref counting can be used.
Comment 45 Edwin Flores [:eflores] [:edwin] 2014-05-19 11:05:42 PDT
Created attachment 8424975 [details] [diff] [review]
Implement WakeLockListener on Linux to disable screensaver while video is playing
Comment 46 Edwin Flores [:eflores] [:edwin] 2014-05-19 11:09:22 PDT
Comment on attachment 8424975 [details] [diff] [review]
Implement WakeLockListener on Linux to disable screensaver while video is playing

Addressed One. Last. Review. Comment.
Comment 47 Edwin Flores [:eflores] [:edwin] 2014-05-19 17:51:08 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/a5885f4bebdb
Comment 48 Phil Ringnalda (:philor) 2014-05-19 20:23:37 PDT
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/e2d6d80b8644 four four unexpected assertion failures in dom/browser-element/mochitest/priority/test_HighPriority.html, e.g. https://tbpl.mozilla.org/php/getParsedLog.php?id=39985786&full=1&branch=mozilla-inbound#error0
Comment 49 Karl Tomlinson (ni?:karlt) 2014-05-27 16:18:15 PDT
The assertions are due to changes from bug 870480.
I don't understand why they should have been necessary, but similarly with most of the WakeLock behavior, so I guess widget code can deal with duplicate notifications.
Comment 50 Karl Tomlinson (ni?:karlt) 2014-05-27 16:20:13 PDT
Are they cpu and high-priority wake locks inhibiting the screensaver?
That doesn't sound right.
Comment 51 Martin Stránský 2014-06-05 07:30:12 PDT
IMHO we should only listen the "DOM_Fullscreen" wake lock topic. See 
http://mxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#6024

Why has it been removed from the original patch?
Comment 52 Chris Pearce (:cpearce) 2014-06-08 13:19:10 PDT
Because opinions differ. ;)

We also want non-fullscreen video and webrtc chats to disable the screensaver. I, for one, find it very annoying when I'm watching a non-fullscreen video and the screensaver kicks in.

FYI, there's code in HTMLMediaElement to ensure the screensaver is only disabled when the video is actually in a visible region of the webpage, in a foreground tab.
Comment 53 Martin Stránský 2014-06-17 06:37:50 PDT
The screensaver should not be inhibited by "cpu" and "high-priority" wake locks. It looks like those are used to inhibit GC or disable power save (see Bug 872430).

I've found two screen related wake locks - "DOM_Fullscreen" (Bug 805017) and "screen" (Bug 868325). I think any element which wants to inhibit screensaver should create the "screen" or "DOM_Fullscreen" wake locks - like the VideoElement does.
Comment 54 Martin Stránský 2014-06-17 06:46:03 PDT
Created attachment 8441363 [details] [diff] [review]
patch v.4

I watches "DOM_Fullscreen" and "screen" wakelocks only. 

Passes the broken test:
./mach mochitest-plain dom/browser-element/mochitest/priority
Comment 55 Martin Stránský 2014-06-17 06:48:06 PDT
Comment on attachment 8441363 [details] [diff] [review]
patch v.4

Try build: https://tbpl.mozilla.org/?tree=Try&rev=e2e2ba6d85b8
Comment 56 Karl Tomlinson (ni?:karlt) 2014-06-17 15:50:46 PDT
Comment on attachment 8441363 [details] [diff] [review]
patch v.4

The "screen" wake lock sounds good.
I suspect we shouldn't need DOM_Fullscreen, but I'm happy to let that by because other platforms treat that similarly.

However, we now know that the "Screensaver is already inhibited" assertion can fail, so that needs to be changed to an early return.
Comment 57 Edwin Flores [:eflores] [:edwin] 2014-06-17 15:52:24 PDT
(In reply to Martin Stránský from comment #53)
> The screensaver should not be inhibited by "cpu" and "high-priority" wake
> locks. It looks like those are used to inhibit GC or disable power save (see
> Bug 872430).

This isn't an issue with wake lock code, it's an issue with wake locks being used incorrectly on desktop. The better solution here would be to make sure that "cpu" and "high-priority" are never used in the first place, on desktop.

(In reply to Martin Stránský from comment #54)
> Passes the broken test:
> ./mach mochitest-plain dom/browser-element/mochitest/priority

...by ignoring those topic strings outright. It doesn't fix the actual problem.
Comment 58 Chris Pearce (:cpearce) 2014-06-17 16:23:00 PDT
We want to inhibit the screensaver when video is playing, not when *fullscreen* video is playing. It looks like the patch above is trying to only inhibit the screensaver for fullscreen video.
Comment 59 Karl Tomlinson (ni?:karlt) 2014-06-17 22:00:27 PDT
(In reply to Chris Pearce (:cpearce) from comment #58)
> We want to inhibit the screensaver when video is playing,

I assume the "screen" wake lock is meant to take care of that.
http://hg.mozilla.org/mozilla-central/annotate/37f08ddaea48/content/html/content/src/HTMLVideoElement.cpp#l312
Comment 60 Martin Stránský 2014-06-18 01:10:20 PDT
(In reply to Edwin Flores [:eflores] [:edwin] from comment #57)
> (In reply to Martin Stránský from comment #53)
> > The screensaver should not be inhibited by "cpu" and "high-priority" wake
> > locks. It looks like those are used to inhibit GC or disable power save (see
> > Bug 872430).
> 
> This isn't an issue with wake lock code, it's an issue with wake locks being
> used incorrectly on desktop. The better solution here would be to make sure
> that "cpu" and "high-priority" are never used in the first place, on desktop.

That's possible but we just can't disable screensaver for *any* lock, right? We want to disable it for the screen-related events only, not for audio events or so.

I see the "DOM_Fullscreen" should not be used here - It seems to be set for any fullscreen elements, like full-screen webgl canvas, full-screen browsing and so.
Comment 61 Chris Pearce (:cpearce) 2014-06-18 01:52:56 PDT
I think you want the "screen" topic, and the "locked-foreground" state to denote when to disable the screen saver.

HTMLMediaElement should already manage only disabling the screen saver when media is playing and in a foreground tab, and bug 1022669 is being worked on right now to ensure that we don't take a screen wake lock for audio-only media.

Edwin's point is that we shouldn't be taking non-screen wakelocks on desktop, they're meant for battery constrained mobile devices; for example the "cpu" wakelock code in HTMLMediaElement.cpp should really be behind b2g and android include guards.
Comment 62 Karl Tomlinson (ni?:karlt) 2014-06-18 02:27:37 PDT
This can be simplified considerably given we need only listen for the "screen" lock.
The hash table is not necessary, and classes can probably be merged.
Comment 63 Martin Stránský 2014-06-18 06:11:32 PDT
Created attachment 8442071 [details] [diff] [review]
patch v.5

Something like that? It listens to "screen" locks only and returns when screensaver is already inhibited.
Comment 64 Karl Tomlinson (ni?:karlt) 2014-06-18 11:51:46 PDT
Comment on attachment 8442071 [details] [diff] [review]
patch v.5

This could be much simpler, but let's not hold off this feature any longer.
Tidying up can be a followup.
Comment 65 Martin Stránský 2014-06-19 01:04:42 PDT
Comment on attachment 8442071 [details] [diff] [review]
patch v.5

Try: https://tbpl.mozilla.org/?tree=Try&rev=ae11997b18f8
Comment 66 Ryan VanderMeulen [:RyanVM] 2014-06-23 07:33:16 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/38476bd45285
Comment 67 Wes Kocher (:KWierso) 2014-06-23 17:38:48 PDT
https://hg.mozilla.org/mozilla-central/rev/38476bd45285
Comment 68 Milan Bouchet-Valat 2014-06-26 02:49:16 PDT
Great!
Comment 69 Catalin Varga [QA][:VarCat] 2014-09-05 00:08:53 PDT
I've tested this using Ubuntu 14.04 x64 and FF 33d.0b1(BUild Id: 20140903133924).

The turn screen off enters on the set time(Ubuntu 14.04 doesn’t ship with any screen savers, just a black screen that appears when your system is idle) when playing a youtube video on fullscreen. The same is happening when playing a video without going to fullscreen. I even installed XScreenSaver to see if the issue is reproducible using it instead of gnome screensaver and it is reproducible with XScreenSaver.

Is the fix not suppose to work for the turning off screen feature from Ubuntu? or is this an issue with the fix?
Comment 70 Florin Mezei, QA (:FlorinMezei) 2014-09-26 01:31:35 PDT
So this does not appear to work on Ubuntu and Fedora. Can anyone shed some light on the test results from comment 69 and what is actually expected here? Otherwise we'll just have to reopen this as it does not seem to do anything.
Comment 71 Constantine 2014-09-26 08:47:09 PDT
It works for me with gnome-screensaver 2.30, firefox nightly 2014-08-30, both with and without fullscreen. Sometimes screensaver stiil appears after ~ 25 minutes of video (but it's configured to get activated after 2 minutes), so it's another problem.
Comment 72 Martin Stránský 2014-09-26 09:00:18 PDT
The patch utilizes the org.freedesktop.ScreenSaver or org.gnome.SessionManager D-BUS interfaces so it depends if you have such services enabled or not and if your screen saver listen here.
Comment 73 Martin Stránský 2014-09-26 09:03:28 PDT
Check d-feet (#yum install d-feet.noarch in Fedora) to inspect your active dbus interfaces.
Comment 74 Martin Stránský 2014-09-26 09:05:42 PDT
For instance I see only "org.mate.ScreenSaver" interface on Fedora20/MATE desktop. So the problem may be here.
Comment 75 Catalin Varga [QA][:VarCat] 2014-09-30 02:57:00 PDT
On Ubuntu 14.04 I have org.gnome.SessionManager but the screensaver still enters. 
Based on previous comments it appears that the fix is working for gnome-screensaver 2.30 but that version of gnome-screensaver is pretty old( from September 2010). It appears that the fix is not working for Gnome 3. I found a post that discusses the changes suffered by Gnome lock-screen with the 3.0 release, not sure how relevant is to this bug but I'm going to post it nevertheless. http://www.lucidelectricdreams.com/2011/06/disabling-screensaverlock-screen-on.html

Our testing machines are running only newer versions of Ubuntu and no other Linux based operating systems so our verification of this bug options are limited. We can only confirm that the fix is not working on the newer Ubuntu releases.
Comment 76 Florin Mezei, QA (:FlorinMezei) 2014-10-03 01:00:09 PDT
Removing "qe-verify+" since it seems we cannot verify this.
Comment 77 Sergey Kondakov 2014-10-19 01:38:28 PDT
Doesn't seem to be fixed in Firefox 33 on OpenSuse 13.1 with KDE4 & XScreenSaver-5.29 (not KDE's one): xscreensaver still sprungs up while playing fullscreen video on Youtube. May be a result of using "Download Manager (S3)" addon which draws a bar at the bottom that disappear slightly after video goes fullscreen. One more reason why it should work on non-fullscreen videos also, since the definition of "fullscreen" seems ambiguous.

Also, how about inhibiting screensaver on flash plugin spawn and uninhibiting on its termination ? Adobe will never add this functionality, but we are stuck with it. Doing so in Firefox as extension of this patch looks like the most straightforward solution. Especially in comparison to something like https://github.com/iye/lightsOn
Comment 78 Milan Bouchet-Valat 2014-10-19 02:02:31 PDT
Sergey, could you check whether with the S3 add-on disabled the screensaver is still not inhibited?
Comment 79 Sergey Kondakov 2014-10-19 13:06:35 PDT
(In reply to Milan Bouchet-Valat from comment #78)
> Sergey, could you check whether with the S3 add-on disabled the screensaver
> is still not inhibited?

It's that way even with almost empty profile and "safe mode". I also noticed that VLC-2.1.5 doesn't inhibit it also even thought it supposedly got dbus xscreensaver interface support (https://trac.videolan.org/vlc/ticket/4739). Upgrading to xscreensaver-5.30 didn't help either. Not sure what's going on.
Comment 80 Bastien Nocera 2014-10-19 13:50:57 PDT
(In reply to Sergey Kondakov from comment #79)
> (In reply to Milan Bouchet-Valat from comment #78)
> > Sergey, could you check whether with the S3 add-on disabled the screensaver
> > is still not inhibited?
> 
> It's that way even with almost empty profile and "safe mode". I also noticed
> that VLC-2.1.5 doesn't inhibit it also even thought it supposedly got dbus
> xscreensaver interface support (https://trac.videolan.org/vlc/ticket/4739).

It's not "dbus xscreensaver interface support". It has nothing to do with XScreensaver.

> Upgrading to xscreensaver-5.30 didn't help either. Not sure what's going on.

XScreensaver implements no D-Bus interface. The D-Bus interface should work in GNOME and KDE though.
Comment 81 Sergey Kondakov 2014-10-19 21:06:18 PDT
(In reply to Bastien Nocera from comment #80)
> It's not "dbus xscreensaver interface support". It has nothing to do with
> XScreensaver.

Ah, just noticed ancient https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=377056

> XScreensaver implements no D-Bus interface. The D-Bus interface should work
> in GNOME and KDE though.

For some reason i thought that XSS not only had it, but also was the first one to implement it. But it seems that its developer isn't too keen on this new "kit & bus" fashion.
So now one has to use those abominations just to have a screensaver ? Then `xdg-screensaver` abstraction should have, probably, been used instead of direct dbus calls. It supports both dbus and console commands.

Otherwise https://launchpad.net/caffeine shim is the only cross-DE option, it seems.
Comment 82 Anthony Jones (:kentuckyfriedtakahe, :k17e) 2014-10-20 10:16:30 PDT
(In reply to Sergey Kondakov from comment #81)
> So now one has to use those abominations just to have a screensaver ? Then
> `xdg-screensaver` abstraction should have, probably, been used instead of
> direct dbus calls. It supports both dbus and console commands.
> 
> Otherwise https://launchpad.net/caffeine shim is the only cross-DE option,
> it seems.

I doubt that anyone is going to have time to work on it. You've already looked up the details so the next step for you is creating a bug for the new change and providing a patch. :-)
Comment 83 Bastien Nocera 2014-10-20 10:28:46 PDT
I'll add that xdg-screensaver is an unmitigated piece of ****. It reimplements the bugs that using D-Bus for inhibition is supposed to fix (namely that if the app disappears, the screensaver shouldn't be left inhibited).

You could add D-Bus support to xscreensaver without linking to a D-Bus implementation. Look at the SDL2 code, it implements the same inhibition mechanism that Firefox and VLC implement through a dlopen()'ed libdbus.so.

However, I would recommend that you use a modern screensaver if you expect modern features.
Comment 84 Milan Bouchet-Valat 2014-10-20 12:11:00 PDT
Is that supposed to work when watching a fullscreen Flash video, like Youtube? It doesn't here on Fedora 20 with Firefox 33 (and the D-Bus interface is present).
Comment 85 Anthony Jones (:kentuckyfriedtakahe, :k17e) 2014-10-20 18:47:17 PDT
(In reply to Milan Bouchet-Valat from comment #84)
> Is that supposed to work when watching a fullscreen Flash video, like
> Youtube? It doesn't here on Fedora 20 with Firefox 33 (and the D-Bus
> interface is present).

No.
Comment 86 Sergey Kondakov 2014-10-21 17:51:15 PDT
(In reply to Bastien Nocera from comment #83)
> I'll add that xdg-screensaver is an unmitigated piece of ****. It
> reimplements the bugs that using D-Bus for inhibition is supposed to fix
> (namely that if the app disappears, the screensaver shouldn't be left
> inhibited).

Mh, yeah, it's odd that it still doesn't do something like periodically checking that calling app is still alive.

> You could add D-Bus support to xscreensaver without linking to a D-Bus
> implementation. Look at the SDL2 code, it implements the same inhibition
> mechanism that Firefox and VLC implement through a dlopen()'ed libdbus.so.

If so, it probably should be at least communicated to the XSS author. But he doesn't seem to give much **** about Linux & X these days, ironically (http://www.jwz.org/xscreensaver/faq.html).

> However, I would recommend that you use a modern screensaver if you expect
> modern features.

It would be nice, except:
1) KDE screensaver doesn't seem to be even coming as separate binary, let alone DE-independent one. At one point KDE devs even tried to completely remove it and forcibly replace with their NIH plasma locking (http://ostatic.com/blog/kde-to-say-buh-bye-to-screensavers). The thing still draws plasma lock screen (which supposed to be disabled) and only few seconds after the screensaver above it. And they don't give a damn.
2) GNOME screensaver requires the core GNOME framework to go with it, not just GTK. Kinda overkill for a screensaver.

And i don't exactly expect "modern features". I expect _Firefox to inhibit/suspend a screensaver when it draws a video. Any videos and any screensavers_.

(In reply to Anthony Jones (:kentuckyfriedtakahe, :k17e) from comment #85)
> (In reply to Milan Bouchet-Valat from comment #84)
> > Is that supposed to work when watching a fullscreen Flash video, like
> > Youtube? It doesn't here on Fedora 20 with Firefox 33 (and the D-Bus
> > interface is present).
> 
> No.

Which makes whole thing pretty much useless. It should:
1) Work with any unpaused (like mplayer does) HTML5 video, fullscreen or not.
Time to put that famous "platform-independent" video abstraction (from gstreamer, proprietary renderers and etc.) code, that makes video scaling, pausing and any interaction to be laggy as hell (even laggier than flash) on any machine, to good use of detecting paused state.
2) Work on flash spawn.
3) Work with any screensaver.
I hate to say it, but maybe such disparity warrants a dirty "Windows-way" of simulating user activity periodically while video is playing. Examples of this probably can be found in KDE/GNOME code that creates dbus "SimulateUserActivity" "method" or in Firefox's code itself.

Moreover, aforementioned "Caffeine" Ubuntu hack is pretty crude (it supposed to somehow force screensavers off when detecting a fullscreen window). It didn't seem to work for me with HTML5 video in Firefox. And going for fullscreen windows is a questionable approach anyway.
Comment 87 Sergey Kondakov 2014-11-11 13:31:42 PST
(In reply to Sergey Kondakov from comment #86)
> 2) GNOME screensaver requires the core GNOME framework to go with it, not
> just GTK. Kinda overkill for a screensaver.
Scratch that ! I just read up on:
http://www.jwz.org/blog/2011/10/has-gnome-3-decided-that-people-shouldnt-want-screen-savers/ (from XSS author)
https://blueprints.launchpad.net/ubuntu/+spec/desktop-o-screensaver (Ubuntu fix plan that gone nowhere)
https://bugs.launchpad.net/ubuntu/+source/gnome-screensaver/+bug/994612 (report about complete uselessness of GNOME SS)

Long story short:
Gnome gone Reverse-Xzibit and removed screensaver out of... their screensaver. Which locks the screen... but HAS NOT SCREENSAVERS.

> Which makes whole thing pretty much useless.
Even more than i imagined and complained previously.
> Moreover, aforementioned "Caffeine" Ubuntu hack is pretty crude...
Looks like in non-obsolete versions they removed XSS support... if it even worked. Which makes it also not an option.
> And i don't exactly expect "modern features". I expect _Firefox to
> inhibit/suspend a screensaver when it draws a video. Any videos and any
> screensavers_.
All of that above makes XSS The Only Option of having actual screensavers under X (except using KDE SS BUT ONLY under KDE session, because it's not even a binary).
And for that to work either:
1) FF devs must play nice with XSS
2) XSS dev should be persuaded to provide dbus interface as dbus today is the cross-DE daemon interface
3) XSS could be forked by a hacker who's not afraid of dbus

In any case this bug cannot be closed as "FIXED" because actual screesaver is not getting disabled under Linux, only lockers are.
Comment 88 electrovalent 2015-02-13 11:50:11 PST
Hi, i can see this is marked as fixed but i can confirm that on latest xubuntu and latest debian testing, firefox 35.01 is not able to disable screensaver or screen power management.
Comment 89 js54434 2015-02-23 19:29:32 PST
(In reply to electrovalent from comment #88)
> Hi, i can see this is marked as fixed but i can confirm that on latest
> xubuntu and latest debian testing, firefox 35.01 is not able to disable
> screensaver or screen power management.

Ditto. I just reinstalled Ubuntu Studio 14.04 and installed Unity, and in Firefox 35.0.1 it's not disabling the screensaver during playback.
Comment 90 Wes Kocher (:KWierso) 2015-05-02 18:40:36 PDT
ni? for the assignee as it appears this didn't fix it for everyone. I'm not sure if we should reopen this bug or file a new one for any further fixes required.
Comment 91 Anthony Jones (:kentuckyfriedtakahe, :k17e) 2015-05-06 17:01:03 PDT
(In reply to Wes Kocher (:KWierso) from comment #90)
> ni? for the assignee as it appears this didn't fix it for everyone. I'm not
> sure if we should reopen this bug or file a new one for any further fixes
> required.

It would make sense to open a new bug for Gnome 3 screensaver rather than re-opening this bug.
Comment 92 electrovalent 2015-05-07 00:34:40 PDT
All versions through 37.01 are still not able to disable screensaver OR power management during playback on Linux. Tested with several power managers and several screensavers. To avoid dealing with screen savers, can firefox handle Xorg and xdg-utils which is the stantard in a linux system?
Comment 93 Anthony Jones (:kentuckyfriedtakahe, :k17e) 2015-05-07 22:01:07 PDT
(In reply to electrovalent from comment #92)
> All versions through 37.01 are still not able to disable screensaver OR
> power management during playback on Linux. Tested with several power
> managers and several screensavers. To avoid dealing with screen savers, can
> firefox handle Xorg and xdg-utils which is the stantard in a linux system?

I suggest filing a bug. This one is resolved.
Comment 94 electrovalent 2015-05-20 05:22:11 PDT
(In reply to Anthony Jones (:kentuckyfriedtakahe, :k17e) from comment #93)
 
> I suggest filing a bug. This one is resolved.

This bug WAS resolved. Now it is not resolved. Filing a bug report for every new version doesn't change anything. The bug still exist on 38.01.
Comment 95 Martin Stránský 2015-05-25 06:00:22 PDT
(In reply to electrovalent from comment #94)
> (In reply to Anthony Jones (:kentuckyfriedtakahe, :k17e) from comment #93)
>  
> > I suggest filing a bug. This one is resolved.
> 
> This bug WAS resolved. Now it is not resolved. Filing a bug report for every
> new version doesn't change anything. The bug still exist on 38.01.

Let's solve that in Bug 1168090.

Note You need to log in before you can comment on or make changes to this bug.