A couple bugs in the win32 display driver

[nathanwiebe]

The gdisp_lld_blit_area() function is broken in a few ways, both to do with blits whose source X coordinate is non-zero:

1. The following line is intended to compute the source array for the blit operation, but doesn't take into account the source offset.

buffer += g->p.x2*g->p.y1; 

It should be changed to something like:

buffer += g->p.x2 * g->p.y1 + g->p.x1;

2. Near the bottom of the function, the following code is intended to detect if a rotation has been done (thereby changing the buffer variable from the pixmap base buffer to a freshly allocated, rotated buffer:

buffer = g->p.ptr;
buffer += g->p.x2 * g->p.y1 + g->p.x1;

//STUFF

if (orientation needs to change):
	buffer = rotateimg(g, buffer);

//STUFF

if (buffer != (gPixel *)g->p.ptr)
	free(buffer);

The problem is that after setting buffer to the pixmap base array at the top of the function, it is tweaked by horizontal and vertical offsets to a new value representing the top left of the source image, but is still located in the original pixmap buffer.  This means that if your source x or y offsets are non-zero and rotation is not required, it will erroneously decide that is has rotated the image and needs to free the array, causing a crash (free an invalid memory pointer).  It should be fixed to something more like:

buffer = g->p.ptr;
buffer += g->p.x2 * g->p.y1 + g->p.x1;
bufferBase = buffer;

//STUFF

if (orientation needs to change)
	buffer = rotateimg(g, buffer);

//STUFF

if (bufferBase != buffer)
	free(buffer);

 

[Joel Bodenmann]

Thank you for bringing this to our attention!

Can you supply a minimum test-case that allows reproducing the bug(s) and corresponding patch(es)?

[nathanwiebe]

I think for the sake of time I will have to respond in pseudocode:

//create a display and clear it to all white
disp = createDisplay(800, 480)
clearDisplay(disp, WHITE)

//create a pixmap and fill it with an image
pm = createPixmap(800, 600)
img = loadImage("puppy.png")	//an 800x480 (full-screen) image
drawImage(pm, img)

//blit the entire pixmap on the display - works correctly
blit(disp, pm, 0, 0, 800, 480, 0, 0)
//at this point, the display contains the entire image

//blit a partial pixmap on the display with source coord (0,0) - works correctly
blit(disp, pm, 0, 0, 200, 200, 0, 0)
//at this point, the display still contains the entire image, and we re-drew the top left 200x200 pixels

//blit a region from the middle of the pixmap to the display (source coord != (0,0))  - PROBLEM!!
blit(disp, pm, 200, 200, 200, 200, 200, 200)
//If all was working correctly, at this point, the display still would contain the entire image,
//and we would have now also re-drew the middle 200x200 pixels.  But...

//Due to bug #2 above: when the stock code is run with rotation = 0, the line of code labelled
//PROBLEM will cause a cras hbecause the buffer is erroneously freed (see bug #2 description above).

//when the fix for the free bug (#2) is applied, there still is a drawing bug.

//Due to bug #1 above, the code labelled PROBLEM will now draw the wrong region of the image onto the display
//because the source pointer computation does not take into account the supplied source horizontal offset.

**For both cases, the fixes are above.

 

[Joel Bodenmann]

Thanks, we will look into this!

[Joel Bodenmann]

I am working on reproducing this. Unfortunately, I was unsuccessful so far.

Here's my simple test case following your pseudo code:

#include "gfx.h"

int main(int argc, char* argv[])
{
	(void)argc;
	(void)argv;

	gfxInit();

	// Open image
	gImage myImage;
	gdispImageOpenFile(&myImage, "640x480.bmp");
	
	// Create pixmap
	GDisplay* pm = gdispPixmapCreate(640, 480);
	gdispGImageDraw(pm, &myImage, 0, 0, 640, 480, 0, 0);
	
	// #1: blit the entire pixmap on the display - works correctly
	gdispGBlitArea(GDISP, 0, 0, 640, 480, 0, 0, 640, gdispPixmapGetBits(pm));
	
	// #2: Blit a partial pixmap on the display with source coord (0,0) - works correctly
	gdispGBlitArea(GDISP, 0, 0, 200, 200, 0, 0, 640, gdispPixmapGetBits(pm));
	
	// #3: blit a region from the middle of the pixmap to the display (source coord != (0,0))  - PROBLEM!!
	gdispGBlitArea(GDISP, 200, 200, 200, 200, 200, 200, 200, gdispPixmapGetBits(pm));
	
	// Close the image
	gdispImageClose(&myImage);

	// Keep the application alive
	while (gTrue)
		gfxSleepMilliseconds(100);

	return 0;
}

When running this, the window (display) shows the image as expected without any defects.

Am I missing something obvious here?
For reference, here is a ready-to-run Win32 project with the code shown above and the corresponding asset(s): win32_blit_test.zip

[nathanwiebe]

Thanks for taking the time to look into this!  Just to be sure, can you also attach a copy of the gdisp_lld_Win32.c file you are using to test?  I have implemented the fixes in my copy of that file, but I have also implemented some other mods such as scaling/stretching/minimize/maximize on the win32 canvas, as well as window styles), and it would help to be able to do a before and after comparison.

[Joel Bodenmann]

Here's the `gdisp_lld_Win32.c` file I was using for this test (this is the same as in the current `master` branch): gdisp_lld_Win32.c

 

On 12/08/2021 at 02:27, nathanwiebe said:

but I have also implemented some other mods such as scaling/stretching/minimize/maximize on the win32 canvas, as well as window styles)

That sounds like something worth contributing 

[Joel Bodenmann]

@inmarket and I revisited this today. Both problems you reported are now fixed by these commits:

• 1235a9056c8324c5552f739114343b3e91dc15fb
• 7845f44f20034779d7d3c969750279297ff524ce

Thank you for reporting these!
Your contribution were noted in the corresponding commit messages.