Bug 6827

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!