Hi Eric,
I am looking into a couple of crashes in dlls/x11drv/dib.c caused by a patch from you. The patch (attached) is only 5 years old, I am sure you remember all the details ;)
To be precise, I get X-errors in the calls to XGetSubImage caused by out of range parameters. Fixing this by some parameter checking is not the big problem, but I wonder if it is still necessary to have those calls there.
You call it a "quick hack", would it not be better to remove it now and properly fix any problems that may surface? The patch that I sent in yesterday ( "fix decoding compressed dibs") is an example of such problem..
Rein.
Rein Klazes a écrit :
Hi Eric,
I am looking into a couple of crashes in dlls/x11drv/dib.c caused by a patch from you. The patch (attached) is only 5 years old, I am sure you remember all the details ;)
sure... since RLE encoding can have holes (ie, it can jump a couple of pixels or lines ahead), when you need to render a RLE image, you need to: - paint the pixels from RLE (for the ones that are defined in RLE) - and keep the previous pixels (from the underlying bitmap) for the ones not in RLE The aim of the patch is to implement this by: - first getting a whole copy of the old bitmap - then paint the RLE pixels on it This can be really suboptimal when the RLE contains all the pixels (they get changed twice)
To be precise, I get X-errors in the calls to XGetSubImage caused by out of range parameters. Fixing this by some parameter checking is not the big problem, but I wonder if it is still necessary to have those calls there.
You call it a "quick hack", would it not be better to remove it now and properly fix any problems that may surface? The patch that I sent in yesterday ( "fix decoding compressed dibs") is an example of such problem..
IMO, what you're seeing is that getting the bitmap isn't cropped against all possible dimensions & boundaries. So the fixes are either: - check the boundaries - rewrite the RLE expander to behave properly pixels not defined in RLE
A+
On Sat, 09 Apr 2005 07:15:43 +0200, you wrote:
I am looking into a couple of crashes in dlls/x11drv/dib.c caused by a patch from you. The patch (attached) is only 5 years old, I am sure you remember all the details ;)
sure... since RLE encoding can have holes (ie, it can jump a couple of pixels or lines ahead),
That is what I was wondering about.
when you need to render a RLE image, you need to:
- paint the pixels from RLE (for the ones that are defined in RLE)
- and keep the previous pixels (from the underlying bitmap) for the ones not in RLE
The aim of the patch is to implement this by:
- first getting a whole copy of the old bitmap
- then paint the RLE pixels on it
This can be really suboptimal when the RLE contains all the pixels (they get changed twice)
I see.
To be precise, I get X-errors in the calls to XGetSubImage caused by out of range parameters. Fixing this by some parameter checking is not the big problem, but I wonder if it is still necessary to have those calls there.
You call it a "quick hack", would it not be better to remove it now and properly fix any problems that may surface? The patch that I sent in yesterday ( "fix decoding compressed dibs") is an example of such problem..
IMO, what you're seeing is that getting the bitmap isn't cropped against all possible dimensions & boundaries.
Are you suggesting more checks then that the pixels are within the dimensions of the drawable?
So the fixes are either:
- check the boundaries
- rewrite the RLE expander to behave properly pixels not defined in RLE
Thanks for the clarification.
Rein.
Are you suggesting more checks then that the pixels are within the dimensions of the drawable?
you need at least to use the intersection of the bitmap and the drawable (not sure both start at (0,0) corner though). I was also wondering what would happen when stretching is required, but IIRC this is done way before (normally) A+
On Sat, 09 Apr 2005 10:19:36 +0200, you wrote:
Are you suggesting more checks then that the pixels are within the dimensions of the drawable?
you need at least to use the intersection of the bitmap and the drawable (not sure both start at (0,0) corner though).
I tried to do this, you need to clip to the visible region as well. I could not get it 100% full proof because of races between the clip calculations and the actual positions on the screen.
So I get the pixels now from the root window (clipped to the screen) and everything is rock solid. The code is simpler too.
I was also wondering what would happen when stretching is required, but IIRC this is done way before (normally)
Funny, in my tests StretchDIBits() with such bitmaps leaves black gaps (Wine2K and WinME) as long as you do not do a 1:1 stretch. With 1:1 stretches or using SetDIBitsToDevice I see the behavior as you described.
Changelog: dlls/x11drv : dib.c
In X11DRV_DIB_SetImageBits avoid BadMatch errors when calling XGetSubImage.
Rein.
Rein Klazes wijn@wanadoo.nl writes:
I tried to do this, you need to clip to the visible region as well. I could not get it 100% full proof because of races between the clip calculations and the actual positions on the screen.
So I get the pixels now from the root window (clipped to the screen) and everything is rock solid. The code is simpler too.
That's ugly, and it will fail miserably if the drawable is a bitmap, you really have to use the specified drawable. Look at how the BitBlt code does it to determine the proper limits.
On 11 Apr 2005 17:48:58 +0200, you wrote:
Rein Klazes wijn@wanadoo.nl writes:
I tried to do this, you need to clip to the visible region as well. I could not get it 100% full proof because of races between the clip calculations and the actual positions on the screen.
So I get the pixels now from the root window (clipped to the screen) and everything is rock solid. The code is simpler too.
That's ugly, and it will fail miserably if the drawable is a bitmap, you really have to use the specified drawable. Look at how the BitBlt code does it to determine the proper limits.
Hmm, I was looking at BitBlt for ideas. This was the straight forward implementation but then discarded:
static void X11DRV_DIB_SetImageBits_GetSubImage( const X11DRV_DIB_IMAGEBITS_DESCR *descr, XImage *bmpImage) { /* compressed bitmaps may contain gaps in them. So make a copy * of the existing pixels first */ RECT bmprc = { descr->xDest, descr->yDest, descr->xDest + descr->width , descr->yDest + descr->height}; RECT rc;
GetRgnBox( descr->physDev->region, &rc ); /* convert from dc to drawable origin */ OffsetRect( &rc, descr->physDev->org.x, descr->physDev->org.y);
/* clip visible rect with bitmap */ if( IntersectRect( &rc, &rc, &bmprc)) XGetSubImage( gdi_display, descr->drawable, rc.left, rc.top, rc.right - rc.left, rc.bottom - rc.top, AllPlanes, ZPixmap, bmpImage, descr->xSrc + rc.left - bmprc.left, descr->ySrc + rc.top - bmprc.top); }
As soon as I move the window to which I am calling SetDIBitsToDevice partly outside of the screen, the drawable lies partly outside the screen rectangle and XGetSubImage fails -> crash. Clipping to the screen rectangle helps somewhat for very slow moves, but with faster moves I still get those failures.
What am I doing wrong here?
Rein.
Rein Klazes wijn@wanadoo.nl writes:
As soon as I move the window to which I am calling SetDIBitsToDevice partly outside of the screen, the drawable lies partly outside the screen rectangle and XGetSubImage fails -> crash. Clipping to the screen rectangle helps somewhat for very slow moves, but with faster moves I still get those failures.
What am I doing wrong here?
Nothing, there's no way to avoid the race. You just need to ignore the BadMatch error in that case.
On 13 Apr 2005 11:59:02 +0200, you wrote:
What am I doing wrong here?
Nothing, there's no way to avoid the race. You just need to ignore the BadMatch error in that case.
Right, here is the next try:
Changelog: dlls/x11drv : dib.c
In X11DRV_DIB_SetImageBits avoid BadMatch errors when calling XGetSubImage.
Rein.
Rein Klazes wijn@wanadoo.nl writes:
--- wine/dlls/x11drv/dib.c 2005-04-13 08:55:39.000000000 +0200 +++ mywine/dlls/x11drv/dib.c 2005-04-13 13:24:10.000000000 +0200 @@ -3484,6 +3484,41 @@ static void X11DRV_DIB_GetImageBits_32( } }
+static int XGetSubImageErrorHandler(Display *dpy, XErrorEvent *event, void *arg) +{
- return 1;
+}
You should check that this was really a BadMatch error from XGetSubImage, we don't want to blindly ignore all errors.
On 13 Apr 2005 17:11:57 +0200, you wrote:
Rein Klazes wijn@wanadoo.nl writes:
--- wine/dlls/x11drv/dib.c 2005-04-13 08:55:39.000000000 +0200 +++ mywine/dlls/x11drv/dib.c 2005-04-13 13:24:10.000000000 +0200 @@ -3484,6 +3484,41 @@ static void X11DRV_DIB_GetImageBits_32( } }
+static int XGetSubImageErrorHandler(Display *dpy, XErrorEvent *event, void *arg) +{
- return 1;
+}
You should check that this was really a BadMatch error from XGetSubImage, we don't want to blindly ignore all errors.
I think you want this.
Changelog: dlls/x11drv : dib.c
In X11DRV_DIB_SetImageBits avoid BadMatch errors when calling XGetSubImage.
Rein.
-
Alexandre Julliard -
Eric Pouech -
Rein Klazes