Closed Bug 811261 Opened 12 years ago Closed 10 years ago

Fullscreen video should disable screensaver during playback on Linux

Categories

(Toolkit :: Video/Audio Controls, enhancement)

Product:
Toolkit
Component:
Video/Audio Controls
Platform:
All
Linux
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla33

People

(Reporter: stransky, Assigned: eflores)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [Advo])

Attachments

(2 files, 11 obsolete files)

Implement WakeLockListener on Linux to disable screensaver while video is playing
14.68 KB, patch
karlt
: review+
Details | Diff | Splinter Review
patch v.5
14.67 KB, patch
karlt
: review+
cbook
: checkin+
Details | Diff | Splinter Review
+++ 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.
No longer blocks: 797625
Attached patch WIP (obsolete) — — Splinter Review
WIP. Unfortunately the D-Bus interface seems to be changed so the Inhibit method does not work.
Attached patch v1 (obsolete) — — Splinter Review
A working one, via. Session Manager & D-Bus.
Attachment #681480 - Flags: feedback?(karlt)
Attachment #681449 - Attachment is obsolete: true
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?
Attachment #681480 - Flags: feedback?(karlt) → feedback+
(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).
Sorry, I mean it inhibits the screen saver only in fullscreen, not when the video is embeded.
(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.
> 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.
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.
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
Attached patch WIP (obsolete) — — Splinter Review
Adds FreeDesktop interface with SessionManager fallback.
Attached patch v2 (obsolete) — — Splinter Review
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?
Attachment #691815 - Attachment is obsolete: true
Attachment #693331 - Flags: review?(karlt)
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.
Attachment #693331 - Flags: review?(karlt) → review+
Attachment #693331 - Flags: review-
Attachment #693331 - Flags: review+
Attachment #693331 - Flags: feedback+
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.
Attachment #693331 - Flags: feedback?(justin.lebar+bug)
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.
Attachment #693331 - Flags: feedback?(justin.lebar+bug) → feedback-
I filed bug 827756 for the wake lock issues.
(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?
> 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).
(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.
> 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.
Whiteboard: [Advo]
(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.
Flags: needinfo?(mounir)
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.
Flags: needinfo?(mounir) → needinfo?(kchen)
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".
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.
Maybe disabling the screen could depend wether sounds are played or not ?
(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.
Flags: needinfo?(kchen)
Depends on: 963366
I think we should go ahead and try to get the wake lock listener here cleaned up and landed.
Assignee: stransky → edwin
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.
Attachment #8401684 - Flags: review?(karlt)
Attachment #8401684 - Flags: review?(cpearce)
Attachment #8401684 - Flags: feedback+
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?
Attachment #8401684 - Flags: review?(karlt) → review-
(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.
(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.
Attachment #8401684 - Attachment is obsolete: true
Attachment #8404228 - Attachment is obsolete: true
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.
Attachment #8404230 - Flags: review?(karlt) → review-
Attachment #8404230 - Attachment is obsolete: true
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 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.
Attachment #8415668 - Flags: review?(karlt) → review-
Attachment #8415668 - Attachment is obsolete: true
Comment on attachment 8419908 [details] [diff] [review] Implement WakeLockListener on Linux to disable screensaver while video is playing Addressed review comments.
Attachment #8419908 - Flags: review?(karlt)
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.
Attachment #8419908 - Flags: review?(karlt) → review-
(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.
Attachment #8419908 - Attachment is obsolete: true
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.
Attachment #8424521 - Flags: review?(karlt) → review-
Attachment #8424521 - Attachment is obsolete: true
Comment on attachment 8424975 [details] [diff] [review] Implement WakeLockListener on Linux to disable screensaver while video is playing Addressed One. Last. Review. Comment.
Attachment #8424975 - Flags: review?(karlt)
Attachment #8424975 - Flags: review?(karlt) → review+
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
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.
Are they cpu and high-priority wake locks inhibiting the screensaver? That doesn't sound right.
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?
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.
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.
Attached patch patch v.4 (obsolete) — — Splinter Review
I watches "DOM_Fullscreen" and "screen" wakelocks only. Passes the broken test: ./mach mochitest-plain dom/browser-element/mochitest/priority
Attachment #681480 - Attachment is obsolete: true
Attachment #693331 - Attachment is obsolete: true
Attachment #8441363 - Flags: review?(karlt)
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.
Attachment #8441363 - Flags: review?(karlt)
Attachment #8441363 - Flags: review-
Attachment #8441363 - Flags: feedback+
(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.
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.
(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
(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.
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.
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.
Attached patch patch v.5 — — Splinter Review
Something like that? It listens to "screen" locks only and returns when screensaver is already inhibited.
Attachment #8441363 - Attachment is obsolete: true
Attachment #8442071 - Flags: review?(karlt)
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.
Attachment #8442071 - Flags: review?(karlt) → review+
Blocks: 517870
No longer depends on: 517870
Attachment #8442071 - Flags: checkin? → checkin+
Depends on: 1028913
https://hg.mozilla.org/mozilla-central/rev/38476bd45285
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Great!
Flags: qe-verify+
QA Contact: catalin.varga
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?
Flags: needinfo?(edwin)
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.
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.
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.
Check d-feet (#yum install d-feet.noarch in Fedora) to inspect your active dbus interfaces.
For instance I see only "org.mate.ScreenSaver" interface on Fedora20/MATE desktop. So the problem may be here.
Flags: needinfo?(edwin)
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.
Removing "qe-verify+" since it seems we cannot verify this.
Flags: qe-verify+
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
Sergey, could you check whether with the S3 add-on disabled the screensaver is still not inhibited?
(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.
(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.
(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.
(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. :-)
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.
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).
(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.
(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.
(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.
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.
(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.
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.
Flags: needinfo?(edwin)
(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.
Flags: needinfo?(edwin)
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?
(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.
(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.
(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.
Decided to fill a new bug 1305288 about continuous video reproduction like in YouTube playlist or suggested videos with autoplay on, as the common screensaver inhibition functionality works well for me on Ubuntu 16.04.
Blocks: 1305288
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: