[Bug 434476] Re: screensaver starts while playing HTML5 videos
Bug Watch Updater
434476 at bugs.launchpad.net
Fri Mar 28 08:58:53 UTC 2014
Launchpad has imported 28 comments from the remote bug at
https://bugzilla.mozilla.org/show_bug.cgi?id=811261.
If you reply to an imported comment from within Launchpad, your comment
will be sent to the remote bug automatically. Read more about
Launchpad's inter-bugtracker facilities at
https://help.launchpad.net/InterBugTracking.
------------------------------------------------------------------------
On 2012-11-13T11:34:43+00:00 Stransky wrote:
+++ 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.
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox-3.5/+bug/434476/comments/37
------------------------------------------------------------------------
On 2012-11-14T12:43:26+00:00 Stransky wrote:
Created attachment 681449
WIP
WIP. Unfortunately the D-Bus interface seems to be changed so the
Inhibit method does not work.
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox-3.5/+bug/434476/comments/38
------------------------------------------------------------------------
On 2012-11-14T15:00:09+00:00 Stransky wrote:
Created attachment 681480
v1
A working one, via. Session Manager & D-Bus.
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox-3.5/+bug/434476/comments/39
------------------------------------------------------------------------
On 2012-11-21T04:45:48+00:00 Karlt wrote:
Comment on attachment 681480
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?
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox-3.5/+bug/434476/comments/40
------------------------------------------------------------------------
On 2012-11-27T15:16:16+00:00 Stransky wrote:
(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).
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox-3.5/+bug/434476/comments/42
------------------------------------------------------------------------
On 2012-11-27T15:18:12+00:00 Stransky wrote:
Sorry, I mean it inhibits the screen saver only in fullscreen, not when
the video is embeded.
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox-3.5/+bug/434476/comments/43
------------------------------------------------------------------------
On 2012-11-27T20:17:52+00:00 Karlt wrote:
(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.
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox-3.5/+bug/434476/comments/44
------------------------------------------------------------------------
On 2012-11-28T10:39:46+00:00 Stransky wrote:
> 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.
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox-3.5/+bug/434476/comments/45
------------------------------------------------------------------------
On 2012-11-28T13:03:07+00:00 ChristianSchaller wrote:
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.
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox-3.5/+bug/434476/comments/46
------------------------------------------------------------------------
On 2012-11-28T18:13:44+00:00 Bastien Nocera wrote:
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
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox-3.5/+bug/434476/comments/47
------------------------------------------------------------------------
On 2012-12-13T15:26:31+00:00 Stransky wrote:
Created attachment 691815
WIP
Adds FreeDesktop interface with SessionManager fallback.
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox-3.5/+bug/434476/comments/48
------------------------------------------------------------------------
On 2012-12-18T12:40:11+00:00 Stransky wrote:
Created attachment 693331
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?
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox-3.5/+bug/434476/comments/49
------------------------------------------------------------------------
On 2013-01-08T04:29:07+00:00 Karlt wrote:
Comment on attachment 693331
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.
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox-3.5/+bug/434476/comments/50
------------------------------------------------------------------------
On 2013-01-08T04:39:50+00:00 Karlt wrote:
Comment on attachment 693331
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.
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox-3.5/+bug/434476/comments/51
------------------------------------------------------------------------
On 2013-01-08T10:22:54+00:00 Justin-lebar+bug wrote:
Comment on attachment 693331
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.
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox-3.5/+bug/434476/comments/52
------------------------------------------------------------------------
On 2013-01-08T11:12:12+00:00 Justin-lebar+bug wrote:
I filed bug 827756 for the wake lock issues.
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox-3.5/+bug/434476/comments/53
------------------------------------------------------------------------
On 2013-01-11T12:28:09+00:00 Stransky wrote:
(In reply to Justin Lebar [:jlebar] from comment #14)
> Comment on attachment 693331
> 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?
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox-3.5/+bug/434476/comments/54
------------------------------------------------------------------------
On 2013-01-14T07:28:12+00:00 Justin-lebar+bug wrote:
> 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).
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox-3.5/+bug/434476/comments/55
------------------------------------------------------------------------
On 2013-01-14T14:14:59+00:00 Stransky wrote:
(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.
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox-3.5/+bug/434476/comments/56
------------------------------------------------------------------------
On 2013-01-14T15:37:04+00:00 Justin-lebar+bug wrote:
> 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.
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox-3.5/+bug/434476/comments/57
------------------------------------------------------------------------
On 2013-01-15T09:28:10+00:00 Stransky wrote:
Filed as Bug 830660.
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox-3.5/+bug/434476/comments/58
------------------------------------------------------------------------
On 2014-02-07T00:40:56+00:00 Cpearce-t wrote:
(In reply to Justin Lebar (not reading bugmail) from comment #14)
> Comment on attachment 693331
> 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.
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox-3.5/+bug/434476/comments/70
------------------------------------------------------------------------
On 2014-02-07T00:54:01+00:00 Cpearce-t wrote:
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.
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox-3.5/+bug/434476/comments/71
------------------------------------------------------------------------
On 2014-02-07T00:57:52+00:00 Cpearce-t wrote:
Comment on attachment 693331
v2
Review of attachment 693331:
-----------------------------------------------------------------
::: 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".
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox-3.5/+bug/434476/comments/72
------------------------------------------------------------------------
On 2014-02-07T01:25:21+00:00 Cpearce-t wrote:
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.
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox-3.5/+bug/434476/comments/73
------------------------------------------------------------------------
On 2014-02-08T03:53:39+00:00 antistress wrote:
Maybe disabling the screen could depend wether sounds are played or not
?
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox-3.5/+bug/434476/comments/74
------------------------------------------------------------------------
On 2014-02-09T02:51:31+00:00 Kchen-d wrote:
(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.
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox-3.5/+bug/434476/comments/75
------------------------------------------------------------------------
On 2014-02-09T21:39:55+00:00 Cpearce-t wrote:
I think we should go ahead and try to get the wake lock listener here
cleaned up and landed.
Reply at:
https://bugs.launchpad.net/ubuntu/+source/firefox-3.5/+bug/434476/comments/76
** Changed in: firefox
Status: Unknown => Confirmed
** Changed in: firefox
Importance: Unknown => Wishlist
** Bug watch added: GNOME Bug Tracker #689225
https://bugzilla.gnome.org/show_bug.cgi?id=689225
--
You received this bug notification because you are a member of Mozilla
Bugs, which is subscribed to Mozilla Firefox.
https://bugs.launchpad.net/bugs/434476
Title:
screensaver starts while playing HTML5 videos
To manage notifications about this bug go to:
https://bugs.launchpad.net/firefox/+bug/434476/+subscriptions
More information about the Ubuntu-mozillateam-bugs
mailing list