Bug 7555 - Fix for Major problem in Xdmx.
Summary: Fix for Major problem in Xdmx.
Status: RESOLVED FIXED
Alias: None
Product: xorg
Classification: Unclassified
Component: Server/DDX/dmx (show other bugs)
Version: unspecified
Hardware: x86 (IA32) Linux (All)
: high major
Assignee: dmx-bugs
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: xorg-7.2
  Show dependency treegraph
 
Reported: 2006-07-17 16:41 UTC by James Steven Supancic III
Modified: 2006-12-16 09:08 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments
fix bug, add addremovescreen support (9.45 KB, patch)
2006-10-01 23:02 UTC, James Steven Supancic III
no flags Details | Splinter Review

Description James Steven Supancic III 2006-07-17 16:41:26 UTC
Fix for Major problem in Xdmx.

I fixed a major problem in Xdmx. When using the RENDER all programs written with
Qt (that I could find to test) failed to draw text and icons. This made Qt
applications unusable with DMX and RENDER. This problem could be fixed using the
-norender option but this significantly slowed down Xdmx.

This is how I fixed it. In the function dmxChangePictureClip in the file
$xserver/hw/dmx/dmxpict.c There appears:
XRenderSetPictureClipRectangles(dmxScreen->beDisplay,
                      pPictPriv->pict,
                      0, 0, NULL, 0);

This line is run whenever the function int dmxChangePictureClip(PicturePtr
pPicture, int clipType, pointer value, int n) is called with clipType set equal
to CT_NONE (CT_NONE is defined to be 0).

I replaced it with this:
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);

In effect I have inversesed the effect of calling dmxChangePictureClip. Before
it was disabling output on the picture when called with clipType==CT_NONE, now
it enables output on the entire picture.

In doing this, all the problems with Qt apps vanish and no new problems appear
to be introduced.

I am not sure this is the proper solution. From reading the RENDER docs and
source code I am lead to believe that CT_NONE means that output should be
disabled, yet Xdmx only works when input is enabled on CT_NONE...

I am unsure what we should do next. Should this change be merged into the Xorg
repository even if I am unsure of its correctness? I tried to get in contact
with the man responsible for creating DMX's RENDER extension but was unable to
locate him. Perhaps you could forward this report to someone who knows more
about the RENDER extension for review?

Even if it is not correct, being able to use Qt apps (and hence KDE) with RENDER
(and hence acceptable speed) is worth the change, as such I have made it to all
of my Xdmx servers...

Thank you for your time,
James Steven Supancic III
Comment 1 James Steven Supancic III 2006-07-18 09:03:24 UTC
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
Comment 2 Kevin E. Martin 2006-07-18 09:10:26 UTC
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!
Comment 3 James Steven Supancic III 2006-07-18 22:41:48 UTC
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
Comment 4 James Steven Supancic III 2006-07-24 20:44:19 UTC
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
Comment 5 Kevin E. Martin 2006-07-25 07:24:40 UTC
(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.
Comment 6 James Steven Supancic III 2006-07-27 03:20:56 UTC
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

Comment 7 James Steven Supancic III 2006-09-22 15:00:47 UTC
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
Comment 8 James Steven Supancic III 2006-10-01 23:02:24 UTC
Created attachment 7230 [details] [review]
fix bug, add addremovescreen support
Comment 9 Kevin E. Martin 2006-10-02 08:18:51 UTC
Adding to the 7.2 release blocker bug so that it is not forgotten.
Comment 10 Daniel Stone 2006-11-04 09:25:05 UTC
kem: ping.  does this look okay to commit?
Comment 11 Kevin E. Martin 2006-12-16 09:08:37 UTC
Patch cleaned up and checked into master and server-1.2-branch (for 7.2 release).


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.