There's a problem with fbFetchTransformed function, which can access data outside the pixmap buffer. It frequently leads to Xserver crash. (the offset in device memory is wrong calculated only for offscreen pixmaps) Proposed patch fixes offset calculation. It also introduces 2 new temporary variables dx, dy to avoid frequent double-dereferencing.
Created attachment 5554 [details] [review] proposed patch
think this is the solution to bug #6704, currently testing. radek, do you happen to have a testcase for this? would be a good thing to add to rendercheck if so.
Mozilla 1.9 (trunk builds, what will become Firefox 3.0) is hitting this; see https://bugzilla.mozilla.org/show_bug.cgi?id=334951 .
I don't have a rendercheck test for it. I was using simple impress presentation which exposed that problem - rectangle are with transparency set to 50%. rendercheck test might be probably easy to add, every operation which requires fbFetchTransformed should expose that bug (the picture pixmap must be offscreen though)
The full story to this patch can be viewed in https://bugzilla.novell.com/show_bug.cgi?id=152730 Be aware, this bug is *extremely* long with more than 100 comments.
(In reply to comment #5) > The full story to this patch can be viewed in > https://bugzilla.novell.com/show_bug.cgi?id=152730 > > Be aware, this bug is *extremely* long with more than 100 comments. Wow, nice find. radek++ I passed this patch by keithp and he didn't see anything obviously wrong with it. I'll land this for RC2 if all goes well on #6704.
I'm currently reviewing the patch, and there are a couple of issues I have to think about: - Radek has been using pDrawable->x/y to retransform the coordinates. On the other hand, the offsets created by fbGetDrawable seem to be better suited for this job (pixmap offset with eventually screen offset). - These offsets are first added to x/y, and now pDrawable->x/y is subtracted. This should level out. - AFAICS pict->pComositeClip->extens should *not* be changed. It might have no side effects, but I guess the save way is to not change the values. I'll test this in a second step. - I think Radek missed one box change. I'm currently rewriting and testing this, if it works out to fix the bug as well, I'll post here to you and Keith to comment which one is the right one. Maybe even a mixture :)
(In reply to comment #7) > - I think Radek missed one box change. Update: Right now I believe that several of the box constructs are dead code. My adapted patch (with the idea about xoff/yoff) clearly showed that I didn't understand the offset correctly as it doesn't work as well. I will have to check this further. Please note also, that this fixes the issue for Xorg 7.0, but for current CVS it only improves the situation, but it doesn't completely fix. There are clearly more render bugs triggered in CVS. The background in impress is still black instead of gray, so there must be another bug with SolidColors. I also ask myself whether fbFetchSourcePict is affected. It doesn't do anything with offsets. Ajax, is fbFetchSourcePict only called on non-accelerated or non-offscreen pixmaps?
> Please note also, that this fixes the issue for Xorg 7.0, but for current CVS it > only improves the situation, but it doesn't completely fix. There are clearly > more render bugs triggered in CVS. The background in impress is still black > instead of gray, so there must be another bug with SolidColors. I was preparing the patch with cvs version, week or two ago. What presentation shows the black/gray problem? Are you sure it doesn't work correctly with my patch? (I know how easy it is to break that function when playing with offsets ;-) Note that I was also using your previous patch for pPicture->format, without it I experienced errors in rendercheck.
My previous patch is already in CVS (had been found independently by - err, I don't remember :-] ). Both presentations in the suse bugzilla show the image prefectly with your patch, but the background is just black (not white). On the second slide of the more complex example the background image is not shown at all. This is all only with current CVS. In Xorg 7.0 everything works fine, so presumably another bug shows up that had been obscured before.
> Both presentations in the suse bugzilla show the image prefectly with your > patch, but the background is just black (not white). On the second slide of the > more complex example the background image is not shown at all. Interesting, I am experiencing something similar with Xgl. Might it be that it is a different bug? Does it happen without patch for this bug as well?
Yes, and it appearantly wasn't connected to the additional patch I'm working on. I will update my whole Xorg tree now and try again.
I'm the reporter of both fdo bug #6704 and the mozilla bug mentioned above. The following page still does not display properly which might be related to all of this: http://sinfest.net/d/20060429.html What is interesting about this is that the image gets displayed fine as it loads progressively but as soon as the loading is finished all or a part (from a random row all the way down) of the image turns white. The good news is that Xorg hasn't crashed for me since I installed the RPMs linked to in bug #6704 so this patch definitely improves the situation.
Created attachment 5602 [details] [review] Proposed updated patch This is a trivially updated patch. It nukes some unused boxes and does not change the extens boxes. I'm leaving some common subexpression eliminations to the compiler. Otherwise this patch behave equivalently (verified), on CVS and 7.0. On 7.0 all issues are fixed. On CVS background + background image are broken, but overlay images are ok now.
*** Bug 6704 has been marked as a duplicate of this bug. ***
I've applied this to 1.1 branch and HEAD, as being clearly better than what we had before. Thanks!
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.