From a05f1233d40f8de00d89da1d655a2026452b5077 Mon Sep 17 00:00:00 2001 From: akallabeth Date: Tue, 18 Nov 2025 10:14:06 +0100 Subject: [PATCH 1/3] [gdi,test] simplify TestGdiClip --- libfreerdp/gdi/test/TestGdiClip.c | 261 ++++++++++++++++-------------- 1 file changed, 140 insertions(+), 121 deletions(-) diff --git a/libfreerdp/gdi/test/TestGdiClip.c b/libfreerdp/gdi/test/TestGdiClip.c index 41ba07a03..b2a6a1ac9 100644 --- a/libfreerdp/gdi/test/TestGdiClip.c +++ b/libfreerdp/gdi/test/TestGdiClip.c @@ -12,13 +12,96 @@ #include "brush.h" #include "clipping.h" +static bool test_gdi_coords(size_t testNr, HGDI_DC hdc, const GDI_RGN* clip, const GDI_RGN* input, + const GDI_RGN* expect) +{ + bool rc = false; + + WINPR_ASSERT(hdc); + WINPR_ASSERT(input); + + HGDI_RGN rgn1 = gdi_CreateRectRgn(0, 0, 0, 0); + HGDI_RGN rgn2 = gdi_CreateRectRgn(0, 0, 0, 0); + if (!rgn1 || !rgn2) + { + (void)fprintf(stderr, "[%s:%" PRIuz "] gdi_CreateRectRgn failed: rgn1=%p, rgn2=%p\n", + __func__, testNr, rgn1, rgn2); + goto fail; + } + + if (!clip) + { + if (!gdi_SetNullClipRgn(hdc)) + { + (void)fprintf(stderr, "[%s:%" PRIuz "] gdi_SetNullClipRgn failed\n", __func__, testNr); + goto fail; + } + } + else if (!gdi_SetClipRgn(hdc, clip->x, clip->y, clip->w, clip->h)) + { + (void)fprintf(stderr, "[%s:%" PRIuz "] gdi_SetClipRgn failed\n", __func__, testNr); + goto fail; + } + + if (!gdi_SetRgn(rgn1, input->x, input->y, input->w, input->h)) + { + (void)fprintf(stderr, "[%s:%" PRIuz "] gdi_SetRgn (rgn1) failed\n", __func__, testNr); + goto fail; + } + + if (expect) + { + if (!gdi_SetRgn(rgn2, expect->x, expect->y, expect->w, expect->h)) + { + (void)fprintf(stderr, "[%s:%" PRIuz "] gdi_SetRgn (rgn2) failed\n", __func__, testNr); + goto fail; + } + } + + const BOOL draw = + gdi_ClipCoords(hdc, &(rgn1->x), &(rgn1->y), &(rgn1->w), &(rgn1->h), NULL, NULL); + if (expect) + { + if (!draw) + { + (void)fprintf(stderr, + "[%s:%" PRIuz "] gdi_ClipCoords failed: expected TRUE, got FALSE\n", + __func__, testNr); + goto fail; + } + + if (!gdi_EqualRgn(rgn1, rgn2)) + { + char buffer1[64] = { 0 }; + char buffer2[64] = { 0 }; + (void)fprintf(stderr, "[%s:%" PRIuz "] gdi_EqualRgn failed: expected %s, got %s\n", + __func__, testNr, buffer1, buffer2); + goto fail; + } + } + else + { + if (draw) + { + (void)fprintf(stderr, + "[%s:%" PRIuz "] gdi_ClipCoords failed: expected FALSE, got TRUE\n", + __func__, testNr); + goto fail; + } + } + + rc = true; + +fail: + gdi_DeleteObject((HGDIOBJECT)rgn1); + gdi_DeleteObject((HGDIOBJECT)rgn2); + return rc; +} + static int test_gdi_ClipCoords(void) { int rc = -1; - BOOL draw = 0; HGDI_DC hdc = NULL; - HGDI_RGN rgn1 = NULL; - HGDI_RGN rgn2 = NULL; HGDI_BITMAP bmp = NULL; const UINT32 format = PIXEL_FORMAT_ARGB32; @@ -32,131 +115,67 @@ static int test_gdi_ClipCoords(void) bmp = gdi_CreateBitmapEx(1024, 768, PIXEL_FORMAT_XRGB32, 0, NULL, NULL); gdi_SelectObject(hdc, (HGDIOBJECT)bmp); gdi_SetNullClipRgn(hdc); - rgn1 = gdi_CreateRectRgn(0, 0, 0, 0); - rgn2 = gdi_CreateRectRgn(0, 0, 0, 0); - rgn1->null = TRUE; - rgn2->null = TRUE; - /* null clipping region */ - gdi_SetNullClipRgn(hdc); - gdi_SetRgn(rgn1, 20, 20, 100, 100); - gdi_SetRgn(rgn2, 20, 20, 100, 100); - gdi_ClipCoords(hdc, &(rgn1->x), &(rgn1->y), &(rgn1->w), &(rgn1->h), NULL, NULL); - if (!gdi_EqualRgn(rgn1, rgn2)) - goto fail; + struct testcase_t + { + const GDI_RGN* clip; + const GDI_RGN* in; + const GDI_RGN* expect; + }; - /* region all inside clipping region */ - gdi_SetClipRgn(hdc, 0, 0, 1024, 768); - gdi_SetRgn(rgn1, 20, 20, 100, 100); - gdi_SetRgn(rgn2, 20, 20, 100, 100); - gdi_ClipCoords(hdc, &(rgn1->x), &(rgn1->y), &(rgn1->w), &(rgn1->h), NULL, NULL); + const GDI_RGN rgn0x0_1024x768 = { 0, 0, 0, 1024, 768, FALSE }; + const GDI_RGN rgn20x20_100x100 = { 0, 20, 20, 100, 100, FALSE }; + const GDI_RGN rgn100x300_250x100 = { 0, 100, 300, 250, 100, FALSE }; + const GDI_RGN rgn100x300_300x100 = { 0, 100, 300, 300, 100, FALSE }; + const GDI_RGN rgn300x20_100x100 = { 0, 300, 20, 100, 100, FALSE }; + const GDI_RGN rgn300x100_300x300 = { 0, 300, 100, 300, 300, FALSE }; + const GDI_RGN rgn300x300_50x100 = { 0, 300, 300, 50, 100, FALSE }; + const GDI_RGN rgn300x300_100x100 = { 0, 300, 300, 100, 100, FALSE }; + const GDI_RGN rgn300x300_300x100 = { 0, 300, 300, 300, 100, FALSE }; + const GDI_RGN rgn300x300_300x200 = { 0, 300, 300, 100, 200, FALSE }; + const GDI_RGN rgn300x420_100x100 = { 0, 300, 420, 100, 100, FALSE }; + const GDI_RGN rgn350x300_50x100 = { 0, 350, 300, 50, 100, FALSE }; + const GDI_RGN rgn350x300_200x100 = { 0, 350, 300, 200, 100, FALSE }; + const GDI_RGN rgn420x420_100x100 = { 0, 420, 420, 100, 100, FALSE }; - if (!gdi_EqualRgn(rgn1, rgn2)) - goto fail; + const struct testcase_t testcases[] = { + /* null clipping region */ + { NULL, &rgn20x20_100x100, &rgn20x20_100x100 }, + /* region all inside clipping region */ + { &rgn0x0_1024x768, &rgn20x20_100x100, &rgn20x20_100x100 }, + /* region all outside clipping region, on the left */ + { &rgn300x300_100x100, &rgn20x20_100x100, NULL }, + /* region all outside clipping region, on the right */ + { &rgn300x300_100x100, &rgn420x420_100x100, NULL }, + /* region all outside clipping region, on top */ + { &rgn300x300_100x100, &rgn300x20_100x100, NULL }, + /* region all outside clipping region, at the bottom */ + { &rgn300x300_100x100, &rgn300x420_100x100, NULL }, + /* left outside, right = clip, top = clip, bottom = clip */ + { &rgn300x300_100x100, &rgn100x300_300x100, &rgn300x300_100x100 }, + /* left outside, right inside, top = clip, bottom = clip */ + { &rgn300x300_100x100, &rgn100x300_250x100, &rgn300x300_50x100 }, + /* left = clip, right outside, top = clip, bottom = clip */ + { &rgn300x300_100x100, &rgn300x300_300x100, &rgn300x300_100x100 }, + /* left inside, right outside, top = clip, bottom = clip */ + { &rgn300x300_100x100, &rgn350x300_200x100, &rgn350x300_50x100 }, + /* top outside, bottom = clip, left = clip, right = clip */ + { &rgn300x300_100x100, &rgn300x100_300x300, &rgn300x300_100x100 }, + /* top = clip, bottom outside, left = clip, right = clip */ + { &rgn300x300_100x100, &rgn300x300_300x200, &rgn300x300_100x100 }, + /* top = clip, bottom = clip, top = clip, bottom = clip */ + { &rgn300x300_100x100, &rgn300x300_100x100, &rgn300x300_100x100 } + }; - /* region all outside clipping region, on the left */ - gdi_SetClipRgn(hdc, 300, 300, 100, 100); - gdi_SetRgn(rgn1, 20, 20, 100, 100); - gdi_SetRgn(rgn2, 0, 0, 0, 0); - draw = gdi_ClipCoords(hdc, &(rgn1->x), &(rgn1->y), &(rgn1->w), &(rgn1->h), NULL, NULL); - - if (draw) - goto fail; - - /* region all outside clipping region, on the right */ - gdi_SetClipRgn(hdc, 300, 300, 100, 100); - gdi_SetRgn(rgn1, 420, 420, 100, 100); - gdi_SetRgn(rgn2, 0, 0, 0, 0); - draw = gdi_ClipCoords(hdc, &(rgn1->x), &(rgn1->y), &(rgn1->w), &(rgn1->h), NULL, NULL); - - if (draw) - goto fail; - - /* region all outside clipping region, on top */ - gdi_SetClipRgn(hdc, 300, 300, 100, 100); - gdi_SetRgn(rgn1, 300, 20, 100, 100); - gdi_SetRgn(rgn2, 0, 0, 0, 0); - draw = gdi_ClipCoords(hdc, &(rgn1->x), &(rgn1->y), &(rgn1->w), &(rgn1->h), NULL, NULL); - - if (draw) - goto fail; - - /* region all outside clipping region, at the bottom */ - gdi_SetClipRgn(hdc, 300, 300, 100, 100); - gdi_SetRgn(rgn1, 300, 420, 100, 100); - gdi_SetRgn(rgn2, 0, 0, 0, 0); - draw = gdi_ClipCoords(hdc, &(rgn1->x), &(rgn1->y), &(rgn1->w), &(rgn1->h), NULL, NULL); - - if (draw) - goto fail; - - /* left outside, right = clip, top = clip, bottom = clip */ - gdi_SetClipRgn(hdc, 300, 300, 100, 100); - gdi_SetRgn(rgn1, 100, 300, 300, 100); - gdi_SetRgn(rgn2, 300, 300, 100, 100); - gdi_ClipCoords(hdc, &(rgn1->x), &(rgn1->y), &(rgn1->w), &(rgn1->h), NULL, NULL); - - if (!gdi_EqualRgn(rgn1, rgn2)) - goto fail; - - /* left outside, right inside, top = clip, bottom = clip */ - gdi_SetClipRgn(hdc, 300, 300, 100, 100); - gdi_SetRgn(rgn1, 100, 300, 250, 100); - gdi_SetRgn(rgn2, 300, 300, 50, 100); - gdi_ClipCoords(hdc, &(rgn1->x), &(rgn1->y), &(rgn1->w), &(rgn1->h), NULL, NULL); - - if (!gdi_EqualRgn(rgn1, rgn2)) - goto fail; - - /* left = clip, right outside, top = clip, bottom = clip */ - gdi_SetClipRgn(hdc, 300, 300, 100, 100); - gdi_SetRgn(rgn1, 300, 300, 300, 100); - gdi_SetRgn(rgn2, 300, 300, 100, 100); - gdi_ClipCoords(hdc, &(rgn1->x), &(rgn1->y), &(rgn1->w), &(rgn1->h), NULL, NULL); - - if (!gdi_EqualRgn(rgn1, rgn2)) - goto fail; - - /* left inside, right outside, top = clip, bottom = clip */ - gdi_SetClipRgn(hdc, 300, 300, 100, 100); - gdi_SetRgn(rgn1, 350, 300, 200, 100); - gdi_SetRgn(rgn2, 350, 300, 50, 100); - gdi_ClipCoords(hdc, &(rgn1->x), &(rgn1->y), &(rgn1->w), &(rgn1->h), NULL, NULL); - - if (!gdi_EqualRgn(rgn1, rgn2)) - goto fail; - - /* top outside, bottom = clip, left = clip, right = clip */ - gdi_SetClipRgn(hdc, 300, 300, 100, 100); - gdi_SetRgn(rgn1, 300, 100, 300, 300); - gdi_SetRgn(rgn2, 300, 300, 100, 100); - gdi_ClipCoords(hdc, &(rgn1->x), &(rgn1->y), &(rgn1->w), &(rgn1->h), NULL, NULL); - - if (!gdi_EqualRgn(rgn1, rgn2)) - goto fail; - - /* top = clip, bottom outside, left = clip, right = clip */ - gdi_SetClipRgn(hdc, 300, 300, 100, 100); - gdi_SetRgn(rgn1, 300, 300, 100, 200); - gdi_SetRgn(rgn2, 300, 300, 100, 100); - gdi_ClipCoords(hdc, &(rgn1->x), &(rgn1->y), &(rgn1->w), &(rgn1->h), NULL, NULL); - - if (!gdi_EqualRgn(rgn1, rgn2)) - goto fail; - - /* top = clip, bottom = clip, top = clip, bottom = clip */ - gdi_SetClipRgn(hdc, 300, 300, 100, 100); - gdi_SetRgn(rgn1, 300, 300, 100, 100); - gdi_SetRgn(rgn2, 300, 300, 100, 100); - gdi_ClipCoords(hdc, &(rgn1->x), &(rgn1->y), &(rgn1->w), &(rgn1->h), NULL, NULL); - - if (!gdi_EqualRgn(rgn1, rgn2)) - goto fail; + for (size_t x = 0; x < ARRAYSIZE(testcases); x++) + { + const struct testcase_t* cur = &testcases[x]; + if (!test_gdi_coords(x, hdc, cur->clip, cur->in, cur->expect)) + goto fail; + } rc = 0; fail: - gdi_DeleteObject((HGDIOBJECT)rgn1); - gdi_DeleteObject((HGDIOBJECT)rgn2); gdi_DeleteObject((HGDIOBJECT)bmp); gdi_DeleteDC(hdc); return rc; From 3607c0ac61fae0ccdbf924ef7cbf9f3d9b68738a Mon Sep 17 00:00:00 2001 From: akallabeth Date: Tue, 18 Nov 2025 11:03:16 +0100 Subject: [PATCH 2/3] [gdi,gdi] remove unnecessary gdi_ClipCoords --- libfreerdp/gdi/gdi.c | 47 ++++++++++++++++++++++++++------------------ 1 file changed, 28 insertions(+), 19 deletions(-) diff --git a/libfreerdp/gdi/gdi.c b/libfreerdp/gdi/gdi.c index f9aa8e6ce..143871559 100644 --- a/libfreerdp/gdi/gdi.c +++ b/libfreerdp/gdi/gdi.c @@ -717,7 +717,6 @@ static BOOL gdi_opaque_rect(rdpContext* context, const OPAQUE_RECT_ORDER* opaque INT32 y = opaque_rect->nTopRect; INT32 w = opaque_rect->nWidth; INT32 h = opaque_rect->nHeight; - gdi_ClipCoords(gdi->drawing->hdc, &x, &y, &w, &h, NULL, NULL); gdi_CRgnToRect(x, y, w, h, &rect); if (!gdi_decode_color(gdi, opaque_rect->color, &brush_color, NULL)) @@ -751,11 +750,11 @@ static BOOL gdi_multi_opaque_rect(rdpContext* context, for (UINT32 i = 0; i < multi_opaque_rect->numRectangles; i++) { const DELTA_RECT* rectangle = &multi_opaque_rect->rectangles[i]; - INT32 x = rectangle->left; - INT32 y = rectangle->top; - INT32 w = rectangle->width; - INT32 h = rectangle->height; - gdi_ClipCoords(gdi->drawing->hdc, &x, &y, &w, &h, NULL, NULL); + const INT32 x = rectangle->left; + const INT32 y = rectangle->top; + const INT32 w = rectangle->width; + const INT32 h = rectangle->height; + gdi_CRgnToRect(x, y, w, h, &rect); ret = gdi_FillRect(gdi->drawing->hdc, &rect, hBrush); @@ -770,16 +769,22 @@ static BOOL gdi_multi_opaque_rect(rdpContext* context, static BOOL gdi_line_to(rdpContext* context, const LINE_TO_ORDER* lineTo) { UINT32 color = 0; - HGDI_PEN hPen = NULL; + WINPR_ASSERT(context); + WINPR_ASSERT(lineTo); + rdpGdi* gdi = context->gdi; + WINPR_ASSERT(gdi); if (!gdi_decode_color(gdi, lineTo->penColor, &color, NULL)) return FALSE; - if (!(hPen = gdi_CreatePen(lineTo->penStyle, lineTo->penWidth, color, gdi->drawing->hdc->format, - &gdi->palette))) + HGDI_PEN hPen = gdi_CreatePen(lineTo->penStyle, lineTo->penWidth, color, + gdi->drawing->hdc->format, &gdi->palette); + if (!hPen) return FALSE; + WINPR_ASSERT(gdi->drawing); + gdi_SelectObject(gdi->drawing->hdc, (HGDIOBJECT)hPen); gdi_SetROP2(gdi->drawing->hdc, WINPR_ASSERTING_INT_CAST(int32_t, lineTo->bRop2)); gdi_MoveToEx(gdi->drawing->hdc, lineTo->nXStart, lineTo->nYStart, NULL); @@ -790,25 +795,29 @@ static BOOL gdi_line_to(rdpContext* context, const LINE_TO_ORDER* lineTo) static BOOL gdi_polyline(rdpContext* context, const POLYLINE_ORDER* polyline) { - INT32 x = 0; - INT32 y = 0; - UINT32 color = 0; - HGDI_PEN hPen = NULL; - DELTA_POINT* points = NULL; - rdpGdi* gdi = context->gdi; + WINPR_ASSERT(context); + WINPR_ASSERT(polyline); + rdpGdi* gdi = context->gdi; + WINPR_ASSERT(gdi); + + UINT32 color = 0; if (!gdi_decode_color(gdi, polyline->penColor, &color, NULL)) return FALSE; - if (!(hPen = gdi_CreatePen(GDI_PS_SOLID, 1, color, gdi->drawing->hdc->format, &gdi->palette))) + WINPR_ASSERT(gdi->drawing); + WINPR_ASSERT(gdi->drawing->hdc); + + HGDI_PEN hPen = gdi_CreatePen(GDI_PS_SOLID, 1, color, gdi->drawing->hdc->format, &gdi->palette); + if (!hPen) return FALSE; gdi_SelectObject(gdi->drawing->hdc, (HGDIOBJECT)hPen); gdi_SetROP2(gdi->drawing->hdc, WINPR_ASSERTING_INT_CAST(int32_t, polyline->bRop2)); - x = polyline->xStart; - y = polyline->yStart; + INT32 x = polyline->xStart; + INT32 y = polyline->yStart; gdi_MoveToEx(gdi->drawing->hdc, x, y, NULL); - points = polyline->points; + DELTA_POINT* points = polyline->points; for (UINT32 i = 0; i < polyline->numDeltaEntries; i++) { From 5ba99a585b53d53f54a04183a543001a88e7f52d Mon Sep 17 00:00:00 2001 From: akallabeth Date: Tue, 18 Nov 2025 11:03:48 +0100 Subject: [PATCH 3/3] [gdi,region] assert parameters --- libfreerdp/gdi/region.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/libfreerdp/gdi/region.c b/libfreerdp/gdi/region.c index 6a973454f..7a3f6ab74 100644 --- a/libfreerdp/gdi/region.c +++ b/libfreerdp/gdi/region.c @@ -143,11 +143,12 @@ GDI_RECT* gdi_CreateRect(INT32 xLeft, INT32 yTop, INT32 xRight, INT32 yBottom) BOOL gdi_RectToRgn(const GDI_RECT* rect, GDI_RGN* rgn) { + WINPR_ASSERT(rect); + WINPR_ASSERT(rgn); + BOOL rc = TRUE; - INT64 w = 0; - INT64 h = 0; - w = rect->right - rect->left + 1ll; - h = rect->bottom - rect->top + 1ll; + INT64 w = rect->right - rect->left + 1ll; + INT64 h = rect->bottom - rect->top + 1ll; if ((w < 0) || (h < 0) || (w > INT32_MAX) || (h > INT32_MAX)) {