Bug 7555

hmmm, well it looks like this fix breaks firefox and others. Oh well, back to
the debuger.

thank you for your time,
James Steven Supancic III

I believe you are on the right track, but I think the issue is when and how to
set the picture clip to NULL and when to set it to None.  NULL means that
everything should be clipped, whereas None means to remove all clipping.  I
believe the NULL case is handled, but the None case is not.  I'll give this some
more thought and see if I can come up with any other ideas.

And, Thanks for your work looking into the Render problems with DMX!

After reading the  ChangePicture function in $serverRoot/render/picture.c  I am
fairly certain that CT_NONE is set when nothing should be clipped (ig everything
should be rendered).

When nothing should be rendered the CT_REGION or CT_UNSORTED (For
rectangles)(maybe others) value should be in the picture's clientClipType field
while the pictures clientClip field is set to NULL. (Ig a NULL region or
rectangle list).

I believe the proper way to handle this is to add
    if(clipType==CT_NONE)
      {
    if(pPictPriv->pict)
      {
        //disable clipping, show all
        XRectangle aRect;
        aRect.x = 0; aRect.y = 0;
        aRect.width = pPicture->pDrawable->width;
        aRect.height = pPicture->pDrawable->height;
        XRenderSetPictureClipRectangles(dmxScreen->beDisplay,
                        pPictPriv->pict,0,0,
                        &aRect,1);
    
        DMX_WRAP(ChangePictureClip, dmxChangePictureClip, dmxScreen, ps);
        return Success;
      }
    else
      {
        DMX_WRAP(ChangePictureClip, dmxChangePictureClip, dmxScreen, ps);
        return Success;
      }
}
After  DMX_UNWRAP(ChangePictureClip, dmxScreen, ps); and before #if 1.

This fix appears to have fixed my Qt problems and I believe it is correct.
I am using it right now and everything is working perfectly. gtk apps, qt apps,
firefox, konsole, all with RENDER enabled.

I have noticed one little thing, when running Gtk apps Xdmx appears to spew the
message
(**) dmx: dmxErrorHandler: BadName (named color or font does not exist)
(**) dmx:                  Major opcode: 157 (RENDER)
(**) dmx:                  Minor opcode: 30 (RenderSetPictureFilter)
(**) dmx:                  ResourceID:   0x600ae1
(**) dmx:                  Failed serial number:  49101
(**) dmx:                  Current serial number: 49170
over and over again. This doesn't appear to cause any functional problems. My
guess is that it has more to do with my last fix for DMX's RENDER system
(Refernce bug #7482) than it has to do with this one. Seeing how it doesn't
appear to cause any problems, I think I will let it be for now.

Please review my proposed solution, if it is correct please merge it into the
main Xorg source tree. (As far as I can tell it both works and is correct).

Thank you for your time,
James Steven Supancic III

So what is going on? Does anyone object to my proposed solution? I have been
using it for almost a week without any problems.

Thank you for your time,
James Steven Supancic III

(In reply to comment #4)
> So what is going on? Does anyone object to my proposed solution? I have been
> using it for almost a week without any problems.

I'm out of town for last week and this week, so I won't be able to test it until
I return next Monday.

One issue I see with the patch is that it is specific to the current drawable
size, which could change if the window is resized.  I think it would be better
to call XRenderSetPictureClipRegion(dmxScreen->beDisplay, pPictPriv->pict, None)
instead, which I believe will reset the clip to CT_NONE on the backend servers.
 I'm not 100% sure that this call will work though, you might need to use
XFixesSetPictureClipRegion() instead.

If you get a chance to test this, let me know how it goes; otherwise, I'll give
it a try when I return home.

XFixesSetPictureClipRegion(dmxScreen->beDisplay,pPictPriv->pict, 0, 0, None);
dmxSync(dmxScreen,FALSE); 
appears to be a suitable replacement for 
XRectangle aRect;
aRect.x = 0; aRect.y = 0;
aRect.width = pPicture->pDrawable->width;
aRect.height = pPicture->pDrawable->height;
XRenderSetPictureClipRectangles(dmxScreen->beDisplay,
                pPictPriv->pict,0,0,
                &aRect,1);


XRenderSetPictureClipRegion(dmxScreen->beDisplay, pPictPriv->pict, None);
leads to a Xdmx server crash. 

This looks like it works, but it does add add another backend dependency. I also
had to modify the Makefile.am and Makefile.in files to get my Xdmx server to
build (It needed to be linked against /usr/lib/libXfixes.a).

So, if we decide to use XFixesSetPictureClipRegion we will have to 1) Update the
Makefiles/configure scripts to link against libXfixes and 2) add code to make
sure all backends have XFIXES available. 

Thank you for your time,
James Steven Supancic III

I think this bug has been open long enough, concidering we have a solution. It
would be nice to get the solution into the official source tree so that it will
be included with new releases of Xorg.

Change the dmxChangePictureClip function in $serverRoot/hw/dmx/dmxpict.c to this:
int dmxChangePictureClip(PicturePtr pPicture, int clipType,
			 pointer value, int n)
{
    ScreenPtr         pScreen   = pPicture->pDrawable->pScreen;
    DMXScreenInfo    *dmxScreen = &dmxScreens[pScreen->myNum];
    PictureScreenPtr  ps        = GetPictureScreen(pScreen);
    dmxPictPrivPtr    pPictPriv = DMX_GET_PICT_PRIV(pPicture);

    DMX_UNWRAP(ChangePictureClip, dmxScreen, ps);
#if 1
    if (ps->ChangePictureClip)
	ps->ChangePictureClip(pPicture, clipType, value, n);
#endif

    /* Change picture clip rects on back-end server */
    if (pPictPriv->pict) {
	/* The clip has already been changed into a region by the mi
	 * routine called above.
	 */
      if(clipType==CT_NONE)
	{
	  //disable clipping, show all
	  XFixesSetPictureClipRegion(dmxScreen->beDisplay,pPictPriv->pict, 0, 0, None); 
	}
      else if (pPicture->clientClip) {
	    RegionPtr   pClip = pPicture->clientClip;
	    BoxPtr      pBox  = REGION_RECTS(pClip);
	    int         nBox  = REGION_NUM_RECTS(pClip);
	    XRectangle *pRects;
	    XRectangle *pRect;
	    int         nRects;

	    nRects = nBox;
	    pRects = pRect = xalloc(nRects * sizeof(*pRect));

	    while (nBox--) {
		pRect->x      = pBox->x1;
		pRect->y      = pBox->y1;
		pRect->width  = pBox->x2 - pBox->x1;
		pRect->height = pBox->y2 - pBox->y1;
		pBox++;
		pRect++;
	    }

	    XRenderSetPictureClipRectangles(dmxScreen->beDisplay,
					    pPictPriv->pict,
					    0, 0,
					    pRects,
					    nRects);
	    xfree(pRects);
	} else {
	    XRenderSetPictureClipRectangles(dmxScreen->beDisplay,
					    pPictPriv->pict,
					    0, 0, NULL, 0);
	}
	dmxSync(dmxScreen, FALSE);
    } else {
	/* FIXME: Handle saving clip region when offscreen */
    }

    DMX_WRAP(ChangePictureClip, dmxChangePictureClip, dmxScreen, ps);
    
    return Success;
}

and throw a #include <Xfixes.h> in at the top someware. 

In Makefile.am /usr/lib/libXfixes.a to Xdmx_LDADD.

I have been using this fix for months, it has introduced no new problems and has
ended my problems with KDE on Xdmx. 

Seriously, the hard part has been done (finding the bug, fixing it, and testing
the solution), all that is left is for this to be merged into the source to for
the next release.

As for checking that backend servers have the XFIXES extension, I say we leave
it for now. Even with RENDER enabled the Xdmx server doesn't check if the
backend servers support RENDER, why should it check for XFIXES? If someone tries
to use a backend without XFIXES the Xdmx server should spew error messages that
make it very clear that they need to have XFIXES. Maybe it should be documented
someware in the manpage?

Thank you for your time,
James Steven Supancic III

Created attachment 7230 [details] [review]
fix bug, add addremovescreen support

Adding to the 7.2 release blocker bug so that it is not forgotten.

kem: ping.  does this look okay to commit?

Patch cleaned up and checked into master and server-1.2-branch (for 7.2 release).