[MPlayer-dev-eng] [PATCH] Use XScreenSaverSuspend if supported

[Ismail Donmez]

Hi,

X.org 7.1 will feature a new function called, XScreenSaverSuspend which can
suspend/resume screensavers. This is way more better than the current hacks
we have. The patch is by Fredrik Höglund ( fredrik at kde.org ), I am
proxying for him since he is not subscribed.

Please review/comment or just apply :-)

Regards,
ismail

[Uoti Urpala]

Post by Ismail Donmez
X.org 7.1 will feature a new function called, XScreenSaverSuspend which can
suspend/resume screensavers. This is way more better than the current hacks
we have. The patch is by Fredrik Höglund ( fredrik at kde.org ), I am
proxying for him since he is not subscribed.

This needs a configure check to see whether the extension is available
(<X11/extensions/scrnsaver.h> is not available on my current system for
example). Also should it disable the current hacks if using this new
function succeeds?

[Ismail Donmez]

Post by Uoti Urpala

Post by Ismail Donmez
X.org 7.1 will feature a new function called, XScreenSaverSuspend which
can suspend/resume screensavers. This is way more better than the current
hacks we have. The patch is by Fredrik Höglund ( fredrik at kde.org ), I
am proxying for him since he is not subscribed.

This needs a configure check to see whether the extension is available
(<X11/extensions/scrnsaver.h> is not available on my current system for
example).

Will do that, maybe just checking XScreenSaverSuspend is available is a better
idea.

Post by Uoti Urpala
Also should it disable the current hacks if using this new
function succeeds?

it returns just after XScreenSaverSuspend so it already disables other hacks.

Regards,
ismail

[Ismail Donmez]

Post by Uoti Urpala

Post by Ismail Donmez
X.org 7.1 will feature a new function called, XScreenSaverSuspend which
can suspend/resume screensavers. This is way more better than the current
hacks we have. The patch is by Fredrik Höglund ( fredrik at kde.org ), I
am proxying for him since he is not subscribed.

This needs a configure check to see whether the extension is available
(<X11/extensions/scrnsaver.h> is not available on my current system for
example). Also should it disable the current hacks if using this new
function succeeds?

Here is a better/cleaner patch using a configure check.

Please comment.

Regards,
ismail

[Ismail Donmez]

Post by Ismail Donmez

Post by Uoti Urpala

Post by Ismail Donmez
X.org 7.1 will feature a new function called, XScreenSaverSuspend which
can suspend/resume screensavers. This is way more better than the
current hacks we have. The patch is by Fredrik Höglund ( fredrik at
kde.org ), I am proxying for him since he is not subscribed.

This needs a configure check to see whether the extension is available
(<X11/extensions/scrnsaver.h> is not available on my current system for
example). Also should it disable the current hacks if using this new
function succeeds?

Here is a better/cleaner patch using a configure check.

Simpler patch without getenv stuff.

Regards,
ismail

[Ismail Donmez]

Post by Ismail Donmez

Post by Ismail Donmez

Post by Uoti Urpala

Post by Ismail Donmez
X.org 7.1 will feature a new function called, XScreenSaverSuspend
which can suspend/resume screensavers. This is way more better than
the current hacks we have. The patch is by Fredrik Höglund ( fredrik
at kde.org ), I am proxying for him since he is not subscribed.

This needs a configure check to see whether the extension is available
(<X11/extensions/scrnsaver.h> is not available on my current system for
example). Also should it disable the current hacks if using this new
function succeeds?

Here is a better/cleaner patch using a configure check.

Simpler patch without getenv stuff.

Simpler version without XOpenDisplay , suggested by fredrik.

Please review.

Regards,
ismail

[Reimar Döffinger]

Hi,

void saver_on(Display * mDisplay)
{
+#ifdef HAVE_XSCREENSAVER_SUSPEND
+ XScreenSaverSuspend(mDisplay, False);
+ return;
+#endif
#ifdef HAVE_XDPMS
int nothing;

Unless there are cases where XScreenSaverSuspend might not work and you
check for this, IMHO you should put the rest of the code under #else.
No point in compiling code that can never be executed...

@@ -1684,6 +1692,11 @@
int interval, prefer_blank, allow_exp;
+#ifdef HAVE_XSCREENSAVER_SUSPEND
+ XScreenSaverSuspend(mDisplay, True);
+ return;
+#endif
+
#ifdef HAVE_XDPMS
int nothing;

esp. since in this case it would probably break compilation with gcc
2.95 (declaration "int nothing;" after code).

Greetings,
Reimar Döffinger

[Uoti Urpala]

Post by Ismail Donmez
Simpler version without XOpenDisplay , suggested by fredrik.

This also removes the runtime checks that were in the original. Doesn't
this mean that if compiled with support for XScreenSaverSuspend, MPlayer
would fail to disable screensaver when running under an older X server?

[Ismail Donmez]

Post by Uoti Urpala

Post by Ismail Donmez
Simpler version without XOpenDisplay , suggested by fredrik.

This also removes the runtime checks that were in the original. Doesn't
this mean that if compiled with support for XScreenSaverSuspend, MPlayer
would fail to disable screensaver when running under an older X server?

Yep will fix that and Reimar's suggestion. But need to optimize so that
checking for XScreenSaverSuspend support is only done once. Will post a patch
soon.

Regards,
ismail

[Reimar Döffinger]

Hi,

Post by Uoti Urpala

Post by Ismail Donmez
Simpler version without XOpenDisplay , suggested by fredrik.

This also removes the runtime checks that were in the original. Doesn't
this mean that if compiled with support for XScreenSaverSuspend, MPlayer
would fail to disable screensaver when running under an older X server?

Huh? I'd think this can't be helped since the function (and actually the
whole lib?) will not be available then. Or am I missing something? At
least I am not a fan on dlopen hacks (despite the fact that I used them
for OpenGL as well...)

Greetings,
Reimar Döffinger

[Ismail Donmez]

Post by Reimar Döffinger
Hi,

Post by Uoti Urpala

Post by Ismail Donmez
Simpler version without XOpenDisplay , suggested by fredrik.

This also removes the runtime checks that were in the original. Doesn't
this mean that if compiled with support for XScreenSaverSuspend, MPlayer
would fail to disable screensaver when running under an older X server?

Huh? I'd think this can't be helped since the function (and actually the
whole lib?) will not be available then. Or am I missing something? At
least I am not a fan on dlopen hacks (despite the fact that I used them
for OpenGL as well...)

Running display might not be running this extension. So you have to check if
the current display runs the extension.

Regards,
ismail

[Ismail Donmez]

Post by Uoti Urpala

Post by Ismail Donmez
Simpler version without XOpenDisplay , suggested by fredrik.

This also removes the runtime checks that were in the original. Doesn't
this mean that if compiled with support for XScreenSaverSuspend, MPlayer
would fail to disable screensaver when running under an older X server?

Here is a better(best? ;) patch which checks if the extension is supported.
Check is done in vo_init hence should be better than checking everytime
calling saver_on/off .

Please review/comment.

Regards,
ismail

[Reimar Döffinger]

Hi,

- int interval, prefer_blank, allow_exp;
+ int interval, prefer_blank, allow_exp, nothing;
+
+#ifdef HAVE_XSCREENSAVER_SUSPEND
+ if (have_xscreensaver_suspend)
+ {
+ XScreenSaverSuspend(mDisplay, True);
+ return;
+ }
+#endif
#ifdef HAVE_XDPMS
- int nothing;
if (DPMSQueryExtension(mDisplay, &nothing, &nothing))
{

Since moving "nothing" up may cause a "unused variable" warning, just
creating a new block inside the "HAVE_XDPMS" part may be preferable.
And are you sure that the DPMS stuff will be handled by
XScreenSaverSuspend as well? I couldn't find any documentation for it
:-(
If you're not sure I'd suggest putting the XScreenSaverSuspend stuff
just below the DPMS stuff.

Greetings,
Reimar Döffinger

[Diego Biurrun]

Post by Ismail Donmez
Please review/comment.
--- configure 4 Apr 2006 05:09:12 -0000 1.1159
+++ configure 11 Apr 2006 20:06:09 -0000
@@ -6802,6 +6802,30 @@
+echocheck "XScreenSaverSuspend"
+cat > $TMPC << EOF
+#include <X11/Xlib.h>
+#include <X11/extensions/scrnsaver.h>
+int main(void) {
+ XScreenSaverSuspend(NULL,True);
+ return 0;
+}
+EOF

Put the test under a _xss_suspend=auto check.

Post by Ismail Donmez
+_ld_xss='-lXss'

Skip this variable, you only need the value once.

Post by Ismail Donmez
+if cc_check $_inc_x11 $_ld_x11 $_ld_xss; then
+ _xss_suspend=yes
+fi

simpler:

cc_check $_inc_x11 $_ld_x11 $_ld_xss && _xss_suspend=yes

Diego

[Reimar Döffinger]

Hello,

Post by Ismail Donmez

Post by Uoti Urpala

Post by Ismail Donmez
Simpler version without XOpenDisplay , suggested by fredrik.

This also removes the runtime checks that were in the original. Doesn't
this mean that if compiled with support for XScreenSaverSuspend, MPlayer
would fail to disable screensaver when running under an older X server?

Here is a better(best? ;) patch which checks if the extension is supported.
Check is done in vo_init hence should be better than checking everytime
calling saver_on/off .
Please review/comment.

Here is an updated version (actually it is basically new code). For
simplicity I moved the check so it is done on every saver_on/off, it
should not matter performance-wise.
I also added a QueryExtension because the documentation says without it
the effects of the functions are undefined.
I advise you to not ignore this, because I intend to apply soon and then
the KDE-specific hack _will_ be removed unless you have exceptionally
good reasons not to ("KDE does not support disabling via
XScreenSaverSuspend" does not count).

Greetings,
Reimar Döffinger

[Adam Tlałka]

Post by Reimar Döffinger
I also added a QueryExtension because the documentation says without it
the effects of the functions are undefined.
I advise you to not ignore this, because I intend to apply soon and then
the KDE-specific hack _will_ be removed unless you have exceptionally
good reasons not to ("KDE does not support disabling via
XScreenSaverSuspend" does not count).

A simpler method is usage of periodical call of XResetScreenSaver
function which does not need special X-server extension.
It resets internal X-server counters so if KDE screensaver can see them
it should work out of the box. Also DPMI and blanking will be disabled
in case of not using any screensaver program so there is no need for
special DPMI handling code in mplayer.
I am not using KDE now so I can't tell if it works with its screensaver
but somebody should check this.
DPMS and blanking suspending works OK.
gnome-screensaver program should be patched to monitor internal X-server
counters so this method could be used.

Regards

[Reimar Döffinger]

Hello,

Post by Adam Tlałka

Post by Reimar Döffinger
I also added a QueryExtension because the documentation says without it
the effects of the functions are undefined.
I advise you to not ignore this, because I intend to apply soon and then
the KDE-specific hack _will_ be removed unless you have exceptionally
good reasons not to ("KDE does not support disabling via
XScreenSaverSuspend" does not count).

A simpler method is usage of periodical call of XResetScreenSaver
function which does not need special X-server extension.

Yes, but these periodical calls are a bit ugly. But yes, replacing
xscreensaver_heartbeat for old X versions by this is a possible next step.

Post by Adam Tlałka
It resets internal X-server counters so if KDE screensaver can see them
it should work out of the box. Also DPMI and blanking will be disabled
in case of not using any screensaver program so there is no need for
special DPMI handling code in mplayer.

Should be the same with XScreenSaverSuspend except that it does not need
to be called periodically.

Post by Adam Tlałka
gnome-screensaver program should be patched to monitor internal X-server
counters so this method could be used.

There seems to be some resistance to this from what I heard since
reportedly the X developers themselves are not really "pushing" these
extensions either...

Greetings,
Reimar Döffinger

[Adam Tlałka]

Post by Reimar Döffinger
Hello,

Post by Adam Tlałka

Post by Reimar Döffinger
I also added a QueryExtension because the documentation says without it
the effects of the functions are undefined.
I advise you to not ignore this, because I intend to apply soon and then
the KDE-specific hack _will_ be removed unless you have exceptionally
good reasons not to ("KDE does not support disabling via
XScreenSaverSuspend" does not count).

A simpler method is usage of periodical call of XResetScreenSaver
function which does not need special X-server extension.

Yes, but these periodical calls are a bit ugly. But yes, replacing
xscreensaver_heartbeat for old X versions by this is a possible next step.

Yes, this is what I mean.

Post by Reimar Döffinger

Post by Adam Tlałka
It resets internal X-server counters so if KDE screensaver can see them
it should work out of the box. Also DPMI and blanking will be disabled
in case of not using any screensaver program so there is no need for
special DPMI handling code in mplayer.

Should be the same with XScreenSaverSuspend except that it does not need
to be called periodically.

Only difference is that we do not need use X-server extensions. But to
function properly with different screensavers X-server must support Xss
extension so they could read internal X-server idle counters or register
itself to get message from X-server. So XScreenSaverSuspend only
simplify mplayer code side so it can live without using Xss.

Post by Reimar Döffinger

Post by Adam Tlałka
gnome-screensaver program should be patched to monitor internal X-server
counters so this method could be used.

There seems to be some resistance to this from what I heard since
reportedly the X developers themselves are not really "pushing" these
extensions either...

I don't know what they really want to implement but I can try to make a
patch to gnome-screensaver and send it to gnome people. There is a
patch needed to xscreensaver too so it could be the example of proper
problem solution ;).

Regards

[Adam Tlałka]

Hello,

Post by Adam Tlałka
itself to get message from X-server. So XScreenSaverSuspend only

sorry :( my error: of course this should be ^- XResetScreenSaver

Post by Adam Tlałka
simplify mplayer code side so it can live without using Xss.

Regards

[Adam Tlałka]

Welcome,

more about XScreenSaverSuspend below (from man):

If XScreenSaverSuspend is called multiple times with suspend set to
’True’, it must be called an equal number of times with suspend set to
’False’ in order for the screensaver timer to be restarted. This
request has no affect if a client tries to resume the screensaver
without first having suspended it. XScreenSaverSuspend can thus not
be used by one client to resume the screensaver if it’s been suspended
by another client.

If a client that has suspended the screensaver becomes disconnected
from the X server, the screensaver timer will automatically be
restarted, unless it’s still suspended by another client.

a) Suspending the screensaver timer doesn’t prevent the screensaver from
being forceably activated with the ForceScreenSaver request, or a DPMS
mode from being set with the DPMSForceLevel request.

b) XScreenSaverSuspend also doesn’t deactivate the screensaver or DPMS
if either is active at the time the request to suspend them is received
by the X server. But once they’ve been deactivated, they won’t
automatically be activated again, until the client has canceled the
suspension.

So while using XScreenSaverSuspend according to a) screensaver could be
forcibly activated by other app and as stated in b) XScreenSaverSuspend
call will not deactivate already active screen saving mode - so if you
start mplayer from some script there will be no screen saver
deactivation.

Periodic XResetScreenSaver calling is better IMHO because it always
deactivates screensaver mode. Even one call per second for example
doesn't look as a heavy job and should work in any case ;).
But it should be tested of course (DPMS and blanking deactivation works
for sure but I don't know about KDE screensaver program).

Regards

[Reimar Döffinger]

Hello,
On Fri, Dec 21, 2007 at 08:00:49AM +0100, Adam Tlałka wrote:
[...]

Post by Adam Tlałka
So while using XScreenSaverSuspend according to a) screensaver could be
forcibly activated by other app and as stated in b) XScreenSaverSuspend
call will not deactivate already active screen saving mode - so if you
start mplayer from some script there will be no screen saver
deactivation.

XScreenSaverSuspend actually seems like the much more sensible behaviour
to me, why should we e.g. disallow explicit locking of the screen via the
screensaver application just because MPlayer is running?

Greetings,
Reimar Döffinger

[Adam Tlałka]

Post by Reimar Döffinger
Hello,
[...]

Post by Adam Tlałka
So while using XScreenSaverSuspend according to a) screensaver could be
forcibly activated by other app and as stated in b) XScreenSaverSuspend
call will not deactivate already active screen saving mode - so if you
start mplayer from some script there will be no screen saver
deactivation.

XScreenSaverSuspend actually seems like the much more sensible behaviour
to me, why should we e.g. disallow explicit locking of the screen via the
screensaver application just because MPlayer is running?

Why? Because if mplayer is running it means to me that we want to see
what is played. If we want to turn on the screensaver we can always
pause or stop playing and then switch on the screensaver if we want to
activate it at once or it will be activated automatically after defined
timeout. If using XScreenSaverSuspend call you can start mplayer from
remote script and then you have to generate some mouse or keyboard
events in case of previously activated screen save mode to deactivate
it. So if you want to remotely control mplayer (not from DPY on which
you play) XScreenSaverSuspend will not always be good enough to activate
switching the display to on without user intervention.

Regards

[Adam Tlałka]

Post by Adam Tlałka

Post by Reimar Döffinger
Hello,
[...]

Post by Adam Tlałka
So while using XScreenSaverSuspend according to a) screensaver could be
forcibly activated by other app and as stated in b) XScreenSaverSuspend
call will not deactivate already active screen saving mode - so if you
start mplayer from some script there will be no screen saver
deactivation.

XScreenSaverSuspend actually seems like the much more sensible behaviour
to me, why should we e.g. disallow explicit locking of the screen via the
screensaver application just because MPlayer is running?

Why? Because if mplayer is running it means to me that we want to see
what is played. If we want to turn on the screensaver we can always
pause or stop playing and then switch on the screensaver if we want to
activate it at once or it will be activated automatically after defined
timeout. If using XScreenSaverSuspend call you can start mplayer from
remote script and then you have to generate some mouse or keyboard
events in case of previously activated screen save mode to deactivate
it. So if you want to remotely control mplayer (not from DPY on which
you play) XScreenSaverSuspend will not always be good enough to activate
switching the display to on without user intervention.

Additionally if do not want to use the heartbeat function you can use XScreenSaverSuspend(dpy, True) call
and then XResetScreenSaver(dpy) once to be sure that previously activated mode will be disabled.
If you remotely control the mplayer program you should do these two calls before starting playing
and call XScreenSaverSuspend(dpy, False) in case of pause/stop/end of playing.

Regards

[Rich Felker]

Post by Reimar Döffinger
Hello,
[...]

Post by Adam Tlałka
So while using XScreenSaverSuspend according to a) screensaver could be
forcibly activated by other app and as stated in b) XScreenSaverSuspend
call will not deactivate already active screen saving mode - so if you
start mplayer from some script there will be no screen saver
deactivation.

XScreenSaverSuspend actually seems like the much more sensible behaviour
to me, why should we e.g. disallow explicit locking of the screen via the
screensaver application just because MPlayer is running?

I agree, this is the correct behavior. If someone wants to do odd
scripting things, just run a separate command to unblank the screen
before starting MPlayer. But MPlayer should not go to lots of trouble
to inhibit the correct, sane default behavior.

Rich

[Ivan Kalvachev]

Post by Rich Felker

Post by Reimar Döffinger
Hello,
[...]

Post by Adam Tlałka
So while using XScreenSaverSuspend according to a) screensaver could be
forcibly activated by other app and as stated in b) XScreenSaverSuspend
call will not deactivate already active screen saving mode - so if you
start mplayer from some script there will be no screen saver
deactivation.

XScreenSaverSuspend actually seems like the much more sensible behaviour
to me, why should we e.g. disallow explicit locking of the screen via the
screensaver application just because MPlayer is running?

I agree, this is the correct behavior. If someone wants to do odd
scripting things, just run a separate command to unblank the screen
before starting MPlayer. But MPlayer should not go to lots of trouble
to inhibit the correct, sane default behavior.

Seconded.
BTW, the XReset* would work only on the screensaver, not dpms.

Reimar, I'm against KDE removal and exactly because there are KDE
versions that doesn't honor XSS. However I think that it should only
be triggered by command line (aka
-stop-screensaver=kde/gnome/xscreensaver ). I won't mind if all this
crap is removed if we commit the event generator (mouse movement
etc...).

One more remark, xscreensaver seems to honor XSS extension if
available (it even have workarounds for quite many history bugs in
it)

[Adam Tlałka]

Hello,
<quote who="Ivan Kalvachev">

Post by Ivan Kalvachev
Seconded.
BTW, the XReset* would work only on the screensaver, not dpms.

Hmm, which version of Xorg do you have?
I've tested it and it works with newest Ubuntu Xorg and DPMS.

Post by Ivan Kalvachev
Reimar, I'm against KDE removal and exactly because there are KDE
versions that doesn't honor XSS. However I think that it should only be
triggered by command line (aka -stop-screensaver=kde/gnome/xscreensaver ).

It seems to be some user config option - which command to execute to
disable screen saver instead inner mplayer code. If configured mplayer
will just use it (stop-screensaver-cmd=command_to_execute).

Post by Ivan Kalvachev
I won't mind if all this
crap is removed if we commit the event generator (mouse movement etc...).

Event generator is not that bad solution but you have to use additional
X-server extensions to generate fake key events for example.

Post by Ivan Kalvachev
One more remark, xscreensaver seems to honor XSS extension if
available (it even have workarounds for quite many history bugs in it).

Nice.
Regards

[Reimar Döffinger]

Hello,
On Fri, Dec 21, 2007 at 10:33:33PM +0200, Ivan Kalvachev wrote:
[...]

Post by Ivan Kalvachev
Reimar, I'm against KDE removal and exactly because there are KDE
versions that doesn't honor XSS. However I think that it should only
be triggered by command line (aka
-stop-screensaver=kde/gnome/xscreensaver ). I won't mind if all this
crap is removed if we commit the event generator (mouse movement
etc...).

The KDE thing is exactly one of the completely broken things that
leaves the screensaver disabled forever if e.g. MPlayer crashes.
Even a -screensaveer-ping-cmd option or something like that which is
called via system on each xscreensaver_heartbeat seems better to me.
We just should make it very clear that it's not our problem if they
decide to use this and end up putting something exploitable in (e.g.
something involving "./file") instead of fixing their screensaver.

Greetings,
Reimar Döffinger

[Ivan Kalvachev]

On Dec 22, 2007 1:24 PM, Reimar Döffinger

Post by Reimar Döffinger
Hello,
[...]

Post by Ivan Kalvachev
Reimar, I'm against KDE removal and exactly because there are KDE
versions that doesn't honor XSS. However I think that it should only
be triggered by command line (aka
-stop-screensaver=kde/gnome/xscreensaver ). I won't mind if all this
crap is removed if we commit the event generator (mouse movement
etc...).

The KDE thing is exactly one of the completely broken things that
leaves the screensaver disabled forever if e.g. MPlayer crashes.
Even a -screensaveer-ping-cmd option or something like that which is
called via system on each xscreensaver_heartbeat seems better to me.
We just should make it very clear that it's not our problem if they
decide to use this and end up putting something exploitable in (e.g.
something involving "./file") instead of fixing their screensaver.

That solution is fine with me.
We could probably add 2 sample command lines of how to ping kde or
gnome savers, so user just to uncomment or copy/paste them.

[Jindrich Makovicka]

On Fri, 21 Dec 2007 22:33:33 +0200

Post by Ivan Kalvachev
One more remark, xscreensaver seems to honor XSS extension if
available (it even have workarounds for quite many history bugs in
it).

At least in Debian, XSS support in xscreensaver is turned off by
default, and the man page states that the XSS support will be
eventually removed:

mitSaverExtension (class Boolean)
This resource controls whether the MIT-SCREEN-SAVER
server extension will be used to decide whether the user is idle.
However, the default for this resource is false, because even if this
extension is available, it is flaky (and it also makes the fade
option not work properly.) Use of this extension is strongly
discouraged. Support for it will probably be removed eventually.

Actually, it isn't even built by default - there is neither an
autodetection, nor a configure option; it could be probably enabled by
config.h editing. I didn't check other distros if they bother
with enabling it.

AFAICS with the current state of x11_common.c, screensaver disabling is
defunct in the default install for the majority of users.

The ping command option that would allow one to stay out of this
mess would be very welcome :)

Regards,

[Adam Tlałka]

Post by Jindrich Makovicka
On Fri, 21 Dec 2007 22:33:33 +0200
AFAICS with the current state of x11_common.c, screensaver disabling is
defunct in the default install for the majority of users.
The ping command option that would allow one to stay out of this
mess would be very welcome :)

I agree.

I am using Ubuntu Gutsy Gibbon with Xorg X Window System Version 1.3.0
Release Date: 19 April 2007 and Xss extension is on and working.
So we can see different approach even if the distro is Debian based.

Regards,