Bug 66492

Summary: keuphoria occasionally only using quarter of screen
Product: kscreensaver Reporter: Ben Burton <bab>
Component: generalAssignee: kscreensaver bugs tracking <kscreensaver-bugs-null>
Status: RESOLVED FIXED    
Severity: normal CC: 4ernov, alecm, ana, bobby.culture, georg.wittenburg, ivo, kocur666, l.lunak, markrose, missive, nickt, pothibo, ronisbr, sh, simon, thilo, vR, walch.martin
Priority: NOR    
Version: unspecified   
Target Milestone: ---   
Platform: Debian testing   
OS: Linux   
Latest Commit: Version Fixed In:
Sentry Crash Report:
Attachments: Fix
Fix, try 2
Fix, try 3
Fix, try 4
Fix, try 6
Fix, try 7 (hey, whatever happened to 5!?)
Fix, try 8

Description Ben Burton 2003-10-24 07:27:16 UTC
Version:            (using KDE KDE 3.1.4)
Installed from:    Debian testing/unstable Packages

Received through the Debian BTS (#207536).

"Occasionally (1 in 50 times,  ish?) I'm finding that the keuphoria screensaver only uses the top left hand corner of my screen rather than the whole screen.  Its happened a handful of times now."

If you want further details it's probably best to ask the original submitter (David Gilbert, gilbertd@treblig.org) since this one is difficult to reproduce.

Ben. :)
Comment 1 James Guo 2003-11-29 12:39:11 UTC
Have same problem in fedora core 1.
Comment 2 Oswald Buddenhagen 2007-04-12 13:24:46 UTC
anybody can still reproduce this?
Comment 3 Oswald Buddenhagen 2007-05-12 12:29:49 UTC
*** Bug 112872 has been marked as a duplicate of this bug. ***
Comment 4 Oswald Buddenhagen 2007-05-18 20:29:13 UTC
*** Bug 119797 has been marked as a duplicate of this bug. ***
Comment 5 Oswald Buddenhagen 2008-04-25 21:54:48 UTC
is this still reproducible with kde4?
Comment 6 Lubos Lunak 2008-06-05 11:29:04 UTC
Waiting for anybody to confirm the bug still exists.
Comment 7 Oswald Buddenhagen 2008-07-26 12:29:52 UTC
*** Bug 167456 has been marked as a duplicate of this bug. ***
Comment 8 Oswald Buddenhagen 2008-07-27 20:59:48 UTC
*** Bug 163536 has been marked as a duplicate of this bug. ***
Comment 9 Oswald Buddenhagen 2008-07-27 21:00:39 UTC
lubos, i think we have enough confirmations now. :}
Comment 10 Jason 'vanRijn' Kasper 2008-07-28 07:10:15 UTC
I'll just note that I never have and still do not see this in a KDE3 session. But bug #163536 is extremely reproducible (much more than 1/50) in KDE4.
Comment 11 Christian Nitschkowski 2008-08-01 18:24:37 UTC
I just wanted to open a report for this issue.
During the Beta and RC-phase of KDE 4.1 I had the described issue a few times.
Since an RC+-build of SuSE I have this problem all of the time.
I now use KDE 4.1.0 and the problem is still there always.
Using NVidia GeForce 7600 GT with NVidia 173.14.09 drivers
Comment 12 Alexey Chernov 2008-08-09 15:30:56 UTC
I can confirm for KDE 4.1.0 (Qt 4.4.1), NVidia GeForce 8600 GT with 177.13 beta proprietary drivers.
Comment 13 Staffan Hamala 2008-10-05 10:56:59 UTC
The problem still exists in KDE 4.1.2. This happens about 80% of the time. So it is VERY reproducable.
Comment 14 Staffan Hamala 2008-10-05 10:59:27 UTC
(In reply to comment #13)
> The problem still exists in KDE 4.1.2. This happens about 80% of the time. So
> it is VERY reproducable.

I'm also using nVidia. Driver version 169.12. FeForce 7100 GS
Comment 15 Pier-Olivier Thibault 2008-10-05 18:44:00 UTC
I also have this issue running on Nvidia and Dual Screen set up.

Compiled from source on 2008/10/01.

main screen: the screen saver uses 1/4 of the screen (top left hand)
Secondary screen: All blank.
Comment 16 ndarkduck 2008-11-16 20:51:36 UTC
I have exactly the same problem. It wasnt on ubuntu 8.04, but on upgrade to ubuntu 8.10  w/ kde 4 it happens most of the time 9/10.

I'm willing to give any information.
Comment 17 Georg Wittenburg 2009-01-03 00:54:27 UTC
I'm seeing this problem with 4.1.85 (unofficial packages from kde42.debian.net at version 4:4.1.85+svn899741-0r1). It's reproducible about 50% of the time for certain GL screensavers, e.g. Euphoria, Flux, Gravity, KRotation, Particle Fountain, Solar Winds. None of the screensaver packaged in xscreensaver-gl have this issue.

Like some reporters, I'm using a nVidia GPU (GeForce 8600M GS with driver 177.82). Is this reproducible on Intel or ATI GPUs?

Further, I haven't see this happening prior (and including in) 4.1.x. I'm not sure whether the upgrade to 4.2 beta or something else caused it to appear.
Comment 18 Alec Moskvin 2009-01-17 19:13:06 UTC
I'm having the same problem as in Comment #17, except I'd say it's more like 66% of the time.

To reproduce, set screensaver as "Solar Winds (GL)" and click test a few times.
It results in the screensaver starting in the top-left corner with the rest of the screen white.

This happens in KDE 4.1.96 (KDE 4.2 RC1) with the latest beta NVIDIA drivers, version 180.22. My graphics card is GeForce Go7300.
Comment 19 Martin Walch 2009-03-08 00:45:32 UTC
Does not seem to be nvidia related. I get this problem with my Intel GMA X3000, too. It looks like the screensaver is run in demo mode instead of root mode.
Comment 20 Thilo-Alexander Ginkel 2009-03-08 19:53:42 UTC
*** Bug 165552 has been marked as a duplicate of this bug. ***
Comment 21 DarkElven 2009-03-08 23:24:54 UTC
Ok if Bug 165552 is a duplicate, so i confirm that this bug is still not resolved even with kde-4.2.1 and also it happens to me with qt-4.5.0.
Comment 22 Ronan Arraes Jardim Chagas 2009-03-15 04:40:02 UTC
I also confirmed this bug with KDE 4.2.1 and QT 4.5.0, but I noticed something very interesting: this just happen (in my case) with OpenGL screensavers that comes with KDE. I mean, when I use an OpenGL screensaver of xscreensaver, like GLMatrix, I wasn't able to reproduce this bug. I've turned on and off GLMatrix more than 20 times and it always works correctly, with and without the option to show Plasma Widgets with screensaver.

My system:
- Gentoo Linux
- nVidia Drivers: 180.37
- QT: 4.5.0
- KDE: 4.2.1

* KScreenSaver was compiled with xscreensaver support (xscreensaver USE flag in Gentoo)
Comment 23 Ivo Anjo 2009-03-31 19:24:28 UTC
Created attachment 32504 [details]
Fix

Ok so I finally got fed up with this bug this week, and decided to not rest until it's solved :)

My analysis of the problem is: most screensavers just subclass KScreenSaver, and do their stuff on paintEvent(). Opengl screensavers on the other hand, need to work with a QGlWidget, instead of QWidget which is what KScreenSaver is, so they create a QGlWidget, and then use XEmbed to place that widget inside a KScreenSaver. The problem is that by doing this using X11 calls, the QGlWidget isn't alerted of changes on his parent in this case (because it has no parent, unlike normal widgets).

So my solution is for KScreenSaver to keep a pointer to the widget when embed() gets called, and to propagate resizeEvents to this widget when it receives them.

The attached patch implements this.

This patch fixes all the opengl screensavers I tested: kflux, kfountain, ksolarwinds, kwave, keuphoria and kgravity. To test it just start any screensaver from the shell (for example kflux.kss) and then resize the window: you'll notice that without the patch the "opengl" view is never resized.

I haven't commited this yet, but if I can get some confirmations that it's all ok (including trying with --setup and inside systemsettings), I'll commit this to trunk and to branch. Thanks in advance :)
Comment 24 Martin Walch 2009-03-31 20:28:08 UTC
I just tested this and resizing works fine now. But the main problem remains: sometimes the OpenGL screensavers take up only about a quarter of the screen in the upper left. Sorry.
Comment 25 Ivo Anjo 2009-03-31 21:48:59 UTC
Created attachment 32507 [details]
Fix, try 2

Thanks a lot for testing. Yeah you were right, after hammering on it for a while eventually I get the bug again, so it seems that patch reduced the frequency it happened, but didn't fully remove the bug.

Here's a new patch with a fix to set the gl widget size before XEmbed'ing it: I think this was what was missing to get it fully working.

If you could please re-test, thanks :)
Comment 26 Martin Walch 2009-03-31 22:47:13 UTC
(In reply to comment #25)

the good news: the problem seems to be gone

the bad news: WIth intensive testing I get kernel panics (Linux)

the even worse news: I have not figured out, yet, if it is related to the fix.

> Here's a new patch with a fix to set the gl widget size before XEmbed'ing it: I
> think this was what was missing to get it fully working.

I don't want to be mean, but... is this really a fix? Shouldn't the inital size be the right one without setting it explicitely?
Comment 27 Ivo Anjo 2009-03-31 23:03:10 UTC
Nice to see it seems to be fixed for you :)

As for the kernel panics, I tested it pretty hard before posting this second patch, and no problems at all here (pc on since I woke up this morning). I'm running on an nvidia 8600GT with the 180.29 drivers.

Finally,
> I don't want to be mean, but... is this really a fix? Shouldn't the inital size
> be the right one without setting it explicitely?

The problem is that the real screensaver widget is KScreenSaver. The XEmbed is there to suck another top-level window (the one with the opengl animation) inside the screensaver one. I did a small test application and found that if you don't give a size to a top-level QGlWidget, the default will be 640x480 so I'm guessing part of the bug was a race: maybe the default size was set before the embed, maybe after, and that was causing the "works sometimes, other times it doesn't".

In both my small test app and kscreensaver, setting the right size (e.g. telling the gl widget that its size should be that of the real screensaver widget) before XEmbed'ding did the trick.
Comment 28 Martin Walch 2009-03-31 23:18:29 UTC
The crash is indeed related to the fix. The crash is even worse and I can reproduce it. But I agree, that this is probably driver and maybe hardware related (but I am not sure, I am using an Intel G965 with xorg-video-intel-2.5.1).

steps to reproduce (DANGER!):
1. open Konsole
2. run keuphoria.kss
3. close it
4. run keuphoria.kss --root
5. resize that window (this is where the patch comes in!)
6. close the window
7. keuphoria.kss will not stop before killing the process with SIGTERM (ctrl+c)
8. press ctrl+c
9. open systemsettings
10. choose Desktop, then Screen Saver
11. choose Euphoria and click Test

This results in a hard lock. Caps Lock and Scroll Lock are blinking.

(After turning the computer off by pressing the power button for five seconds, and turning it on again, X does not start on first try, because the graphics chip is still in an invalid state.)

Anyway, this probably does not have anything to do with this bug. However, I am not sure, yet, where to open a new bug. :)
Comment 29 Ivo Anjo 2009-03-31 23:34:08 UTC
I can reproduce that crash here -- although I get some X Errors on the shell when I close the screensaver window, but those were already there before the patch.

Let's hope someone with an Intel driver can test this. Unfortunately, I only have access to ati and nvidia hardware.
Comment 30 Ivo Anjo 2009-03-31 23:34:59 UTC
Damn, I meant to say "I can't", not I can. It works fine here.
Comment 31 Oswald Buddenhagen 2009-04-01 00:16:45 UTC
the patch is, ermm, "suboptimal" (it breaks binary compatibility and disregards the coding style), but the analysis of the problem is very helpful. :)

martin: nvidia is very happy about every linux driver bug report they get (of course they are, right? there can never be enough). so make them happy. :D
Comment 32 Ivo Anjo 2009-04-01 00:33:39 UTC
(In reply to comment #31)
> the patch is, ermm, "suboptimal" (it breaks binary compatibility and disregards
> the coding style), but the analysis of the problem is very helpful. :)

I could catch the resize inside the event() call, instead of inside resizeEvent(). As for avoiding the slot for saving the QWidget the only way I can think of is to create a wrapper class that is a QObject, and setting the KScreenSaver as parent, and when we need to access it (not very often), ask for the children of KScreenSaver and search for that object.

As for the coding style, I tried to copy the original. Could you point out where I got it wrong? The only thing I can think of is adding a space after each '(' and before each ')' .

> martin: nvidia is very happy about every linux driver bug report they get (of
> course they are, right? there can never be enough). so make them happy. :D

Martin seems to be having problems with intel, not nvidia hardware.
Comment 33 Ivo Anjo 2009-04-01 10:42:28 UTC
Created attachment 32515 [details]
Fix, try 3

So, third version of the patch. This one should preserve binary compatibility: I catch the resize event inside event() and use the d-pointer to keep the extra member I needed (before I was confused because I thought other subclasses were also using the d-pointer but I understand now).

I also tried to add the extra spaces to preserve the original style.

Is this version of the patch acceptable? Thanks for the feedback so far.
Comment 34 Martin Walch 2009-04-01 11:03:32 UTC
That fix also works fine.
Concerning binary compatibility: also seems to be good. Works for me without relinking.
Comment 35 Oswald Buddenhagen 2009-04-01 11:19:43 UTC
that one is technically correct. there is still room for improvement, though: :)

i don't think you need to include QResizeEvent.
there is a resize() overload which takes a QSize.
trivial constructors of private classes are typically inlined.
the creation of the private object is typically put into the c'tor's variable initializer list (i.e., before the opening brace).
you are leaking the private object.
given that the private object contains exactly one pointer, it might be preferable to replace the unused d pointer with that one, adding a big fat note that if more members are added, they need to be moved to a private object.
make a line break after the if's condition.
Comment 36 Ivo Anjo 2009-04-01 12:03:07 UTC
Created attachment 32516 [details]
Fix, try 4

(In reply to comment #35)

Fixed most of the suggestions, thanks (the patch seems to be getting smaller, a few more iterations and don't I anything will be left).

> given that the private object contains exactly one pointer, it might be
> preferable to replace the unused d pointer with that one, adding a big fat note
> that if more members are added, they need to be moved to a private object.

As for this one, I think that would just complicate things, for a savings of a couple of extra bytes, so I think this way is cleaner. (But if you think that it really should be done that way, I could do that too).

Thanks
Comment 37 Oswald Buddenhagen 2009-04-01 12:48:22 UTC
(In reply to comment #36)
> (the patch seems to be getting smaller, a
> few more iterations and don't I anything will be left).
> 
hehe

> > [...] it might be
> > preferable to replace the unused d pointer with that one, [...]
> 
> As for this one, I think that would just complicate things, for a savings of a
> couple of extra bytes, so I think this way is cleaner.
>
it wouldn't complicate anything, as it would be just the straight-forward, non-d-pointer-ized variant. in fact, it would work towards the goal of making the patch disappear. :D
it is true that it would complicate the next addition, as that one would have to do the d pointer transformation in the first place, but given the frequency at which new members are added, this seems fairly acceptable to me. :)
Comment 38 Ivo Anjo 2009-04-01 13:08:33 UTC
Created attachment 32517 [details]
Fix, try 6

This new version uses the d-pointer to store the widget.

If it's ok with you I'll commit it (always wanted do kill a many-year-old-bug) :)
Comment 39 Lubos Lunak 2009-04-01 13:21:04 UTC
I'd prefer if we didn't ask for ubergeek ugly hacks for reasons that absolutely don't matter in practice. Please use the version with the proper d pointer.
Comment 40 Oswald Buddenhagen 2009-04-01 13:56:30 UTC
huh, i didn't ask you to do typecasts - that's just plain ugly. replace the declaration of d accordingly and leave a note that it needs to be transformed into a d pointer once more members are needed. this "trick" is used fairly often in qt and kde code.

of course, the unnecessary use of a private object doesn't matter at all in this case, but having the minimal implementation just makes the code nicer to read. and, after all, ivo will now have tried about every conceivable approach to the problem - for good practice. try 4 is good enough if the practice is not called for. :D
Comment 41 Ivo Anjo 2009-04-01 19:20:55 UTC
Created attachment 32523 [details]
Fix, try 7 (hey, whatever happened to 5!?)

I misunderstood what Oswald was saying. I think I got it, so here's a version that "replaces" the dpointer with the pointer to the widget.

So, try 4 or try 7, take your pick :)
Comment 42 Oswald Buddenhagen 2009-04-02 00:00:00 UTC
haha, ok, one more try: use the member name from try 1, and move the comment above the commented out d pointer, altering it to something like "for binary compatibility reasons, this class must have exactly one data member which is a pointer. if more members are added, uncomment the private object and move the members into it".

yes, i was also wondering about #5. i assume it was eaten by today's date.
Comment 43 Ivo Anjo 2009-04-02 01:18:52 UTC
Created attachment 32527 [details]
Fix, try 8

Ok, so I've gone back to embeddedWidget, and added the note to the header file.
Comment 44 Oswald Buddenhagen 2009-04-02 11:11:01 UTC
u can't trick me - this is try 7! and that's a fairly good number for a final version. :-D

i assume do do the backport as well?
Comment 45 Ivo Anjo 2009-04-02 12:07:08 UTC
Ok so I'll commit try 8 (or is it 7!?) when I get home, and I'll also backport it (don't know if there's going to be a 4.2.3 though, but I hope so, so I can get this fix via distro packages).
Comment 46 Martin Walch 2009-04-02 12:14:43 UTC
Attachment #32527 [details] also works fine. Resizing works, full screen is used and it
is binary compatible.

(What I did not test was the Kernel panic, which is very likely not gone, but I
do not want to let my system crash again :))
Comment 47 Ivo Anjo 2009-04-02 21:53:45 UTC
SVN commit 948339 by ianjo:

Pass along resize events to the embedded widget, and also set the correct
size for the widget before using XReparentWindow on it.

Thanks for all the feedback and testing of the patch!

BUG: 66492



 M  +5 -1      kscreensaver.cpp  
 M  +5 -1      kscreensaver.h  


WebSVN link: http://websvn.kde.org/?view=rev&revision=948339
Comment 48 Ivo Anjo 2009-04-02 22:15:01 UTC
SVN commit 948349 by ianjo:

Backport r948339.
Pass along resize events to the embedded widget, and also set the
correct size for the widget before using XReparentWindow on it.

Thanks for all the feedback and testing of the patch!

CCBUG: 66492



 M  +5 -1      kscreensaver.cpp  
 M  +5 -1      kscreensaver.h  


WebSVN link: http://websvn.kde.org/?view=rev&revision=948349
Comment 49 Mark Rose 2009-05-08 05:11:42 UTC
Hey, thanks for fixing this!! Now I can use all the beautiful screen savers again! :D
Comment 50 Oswald Buddenhagen 2011-06-02 16:07:24 UTC
*** Bug 165413 has been marked as a duplicate of this bug. ***
Comment 51 Oswald Buddenhagen 2011-06-02 16:08:18 UTC
*** Bug 183819 has been marked as a duplicate of this bug. ***