[PATCH v2 0/1] MR3568: gdiplus: implement PixelOffsetModeHighQuality for GdipDrawImagePointsRect
After investigation, it looks like the PixelOffsetModeHalf/PixelOffsetModeHighQuality is using floating point numbers, while PixelOffsetModeNone/PixelOffsetModeHighSpeed is using integers to calculate bitmap colours. After using floating numbers, most tests are passing now. -- v2: gdiplus: implement PixelOffsetModeHighQuality for GdipDrawImagePointsRect https://gitlab.winehq.org/wine/wine/-/merge_requests/3568
From: Bartosz Kosiorek <gang65(a)poczta.onet.pl> --- dlls/gdiplus/graphics.c | 88 +++++++++++++++++++++++++++----------- dlls/gdiplus/tests/image.c | 69 ++++++++++++++++-------------- 2 files changed, 99 insertions(+), 58 deletions(-) diff --git a/dlls/gdiplus/graphics.c b/dlls/gdiplus/graphics.c index e3b5661fd67..8ffed2cefbc 100644 --- a/dlls/gdiplus/graphics.c +++ b/dlls/gdiplus/graphics.c @@ -3042,7 +3042,6 @@ GpStatus WINGDIPAPI GdipDrawImagePointsRect(GpGraphics *graphics, GpImage *image DrawImageAbort callback, VOID * callbackData) { GpPointF ptf[4]; - POINT pti[4]; GpStatus stat; TRACE("(%p, %p, %p, %d, %f, %f, %f, %f, %d, %p, %p, %p)\n", graphics, image, points, @@ -3094,11 +3093,6 @@ GpStatus WINGDIPAPI GdipDrawImagePointsRect(GpGraphics *graphics, GpImage *image ptf[3].Y = ptf[2].Y + ptf[1].Y - ptf[0].Y; if (!srcwidth || !srcheight || (ptf[3].X == ptf[0].X && ptf[3].Y == ptf[0].Y)) return Ok; - gdip_transform_points(graphics, WineCoordinateSpaceGdiDevice, CoordinateSpaceWorld, ptf, 4); - round_points(pti, ptf, 4); - - TRACE("%s %s %s %s\n", wine_dbgstr_point(&pti[0]), wine_dbgstr_point(&pti[1]), - wine_dbgstr_point(&pti[2]), wine_dbgstr_point(&pti[3])); srcx = units_to_pixels(srcx, srcUnit, image->xres, graphics->printer_display); srcy = units_to_pixels(srcy, srcUnit, image->yres, graphics->printer_display); @@ -3111,6 +3105,7 @@ GpStatus WINGDIPAPI GdipDrawImagePointsRect(GpGraphics *graphics, GpImage *image GpBitmap* bitmap = (GpBitmap*)image; BOOL do_resampling = FALSE; BOOL use_software = FALSE; + POINT pti[4]; TRACE("graphics: %.2fx%.2f dpi, fmt %#x, scale %f, image: %.2fx%.2f dpi, fmt %#x, color %08lx\n", graphics->xres, graphics->yres, @@ -3118,6 +3113,8 @@ GpStatus WINGDIPAPI GdipDrawImagePointsRect(GpGraphics *graphics, GpImage *image graphics->scale, image->xres, image->yres, bitmap->format, imageAttributes ? imageAttributes->outside_color : 0); + gdip_transform_points(graphics, WineCoordinateSpaceGdiDevice, CoordinateSpaceWorld, ptf, 4); + if (ptf[1].Y != ptf[0].Y || ptf[2].X != ptf[0].X || ptf[1].X - ptf[0].X != srcwidth || ptf[2].Y - ptf[0].Y != srcheight || srcx < 0 || srcy < 0 || @@ -3131,6 +3128,7 @@ GpStatus WINGDIPAPI GdipDrawImagePointsRect(GpGraphics *graphics, GpImage *image if (use_software) { RECT dst_area; + REAL dst_area_left, dst_area_right, dst_area_top, dst_area_bottom; GpRectF graphics_bounds; GpRect src_area; int i, x, y, src_stride, dst_stride; @@ -3143,23 +3141,62 @@ GpStatus WINGDIPAPI GdipDrawImagePointsRect(GpGraphics *graphics, GpImage *image if (!imageAttributes) imageAttributes = &defaultImageAttributes; - dst_area.left = dst_area.right = pti[0].x; - dst_area.top = dst_area.bottom = pti[0].y; - for (i=1; i<4; i++) + stat = get_graphics_device_bounds(graphics, &graphics_bounds); + if (stat != Ok) return stat; + + switch (offset_mode) { - if (dst_area.left > pti[i].x) dst_area.left = pti[i].x; - if (dst_area.right < pti[i].x) dst_area.right = pti[i].x; - if (dst_area.top > pti[i].y) dst_area.top = pti[i].y; - if (dst_area.bottom < pti[i].y) dst_area.bottom = pti[i].y; + default: + case PixelOffsetModeNone: + case PixelOffsetModeHighSpeed: + { + round_points(pti, ptf, 4); + dst_area.left = dst_area.right = pti[0].x; + dst_area.top = dst_area.bottom = pti[0].y; + for (i=1; i<4; i++) + { + if (dst_area.left > pti[i].x) dst_area.left = pti[i].x; + if (dst_area.right < pti[i].x) dst_area.right = pti[i].x; + if (dst_area.top > pti[i].y) dst_area.top = pti[i].y; + if (dst_area.bottom < pti[i].y) dst_area.bottom = pti[i].y; + } + + if (graphics_bounds.X > dst_area.left) dst_area.left = floorf(graphics_bounds.X); + if (graphics_bounds.Y > dst_area.top) dst_area.top = floorf(graphics_bounds.Y); + if (graphics_bounds.X + graphics_bounds.Width < dst_area.right) dst_area.right = ceilf(graphics_bounds.X + graphics_bounds.Width); + if (graphics_bounds.Y + graphics_bounds.Height < dst_area.bottom) dst_area.bottom = ceilf(graphics_bounds.Y + graphics_bounds.Height); + + dst_area_left = dst_area.left; + dst_area_top = dst_area.top; + dst_area_right = dst_area.right; + dst_area_bottom = dst_area.bottom; + break; } + case PixelOffsetModeHalf: + case PixelOffsetModeHighQuality: + { + dst_area_left = dst_area_right = ptf[0].X; + dst_area_top = dst_area_bottom = ptf[0].Y; + for (i=1; i<4; i++) + { + if (dst_area_left > ptf[i].X) dst_area_left = ptf[i].X; + if (dst_area_right < ptf[i].X) dst_area_right = ptf[i].X; + if (dst_area_top > ptf[i].Y) dst_area_top = ptf[i].Y; + if (dst_area_bottom < ptf[i].Y) dst_area_bottom = ptf[i].Y; + } - stat = get_graphics_device_bounds(graphics, &graphics_bounds); - if (stat != Ok) return stat; + if (graphics_bounds.X > dst_area_left) dst_area_left = graphics_bounds.X; + if (graphics_bounds.Y > dst_area_top) dst_area_top = graphics_bounds.Y; + if (graphics_bounds.X + graphics_bounds.Width < dst_area_right) dst_area_right = graphics_bounds.X + graphics_bounds.Width; + if (graphics_bounds.Y + graphics_bounds.Height < dst_area_bottom) dst_area_bottom = graphics_bounds.Y + graphics_bounds.Height; - if (graphics_bounds.X > dst_area.left) dst_area.left = floorf(graphics_bounds.X); - if (graphics_bounds.Y > dst_area.top) dst_area.top = floorf(graphics_bounds.Y); - if (graphics_bounds.X + graphics_bounds.Width < dst_area.right) dst_area.right = ceilf(graphics_bounds.X + graphics_bounds.Width); - if (graphics_bounds.Y + graphics_bounds.Height < dst_area.bottom) dst_area.bottom = ceilf(graphics_bounds.Y + graphics_bounds.Height); + dst_area.left = dst_area_left; + dst_area.top = dst_area_top; + dst_area.right = dst_area_right; + dst_area.bottom = dst_area_bottom; + break; + } + } TRACE("dst_area: %s\n", wine_dbgstr_rect(&dst_area)); @@ -3173,10 +3210,10 @@ GpStatus WINGDIPAPI GdipDrawImagePointsRect(GpGraphics *graphics, GpImage *image else { /* Make sure src_area is equal in size to dst_area. */ - src_area.X = srcx + dst_area.left - pti[0].x; - src_area.Y = srcy + dst_area.top - pti[0].y; - src_area.Width = dst_area.right - dst_area.left; - src_area.Height = dst_area.bottom - dst_area.top; + src_area.X = srcx + dst_area_left - ptf[0].X; + src_area.Y = srcy + dst_area_top - ptf[0].Y; + src_area.Width = dst_area_right - dst_area_left; + src_area.Height = dst_area_bottom - dst_area_top; } TRACE("src_area: %d x %d\n", src_area.Width, src_area.Height); @@ -3250,9 +3287,9 @@ GpStatus WINGDIPAPI GdipDrawImagePointsRect(GpGraphics *graphics, GpImage *image /* Calculate top left point of transformed image. It would be used as reference point for adding */ src_pointf_row.X = dst_to_src.matrix[4] + - dst_area.left * x_dx + dst_area.top * y_dx; + dst_area_left * x_dx + dst_area_top * y_dx; src_pointf_row.Y = dst_to_src.matrix[5] + - dst_area.left * x_dy + dst_area.top * y_dy; + dst_area_left * x_dy + dst_area_top * y_dy; for (y = dst_area.top; y < dst_area.bottom; y++, src_pointf_row.X += y_dx, src_pointf_row.Y += y_dy) @@ -3367,6 +3404,7 @@ GpStatus WINGDIPAPI GdipDrawImagePointsRect(GpGraphics *graphics, GpImage *image gdi_transform_acquire(graphics); + round_points(pti, ptf, 4); if (bitmap->format & (PixelFormatAlpha|PixelFormatPAlpha)) { gdi_alpha_blend(graphics, pti[0].x, pti[0].y, pti[1].x - pti[0].x, pti[2].y - pti[0].y, diff --git a/dlls/gdiplus/tests/image.c b/dlls/gdiplus/tests/image.c index 0baf6eae77b..e0630343be6 100644 --- a/dlls/gdiplus/tests/image.c +++ b/dlls/gdiplus/tests/image.c @@ -4702,30 +4702,30 @@ static void test_image_format(void) static void test_DrawImage_scale(void) { - static const BYTE back_8x1[24] = { 0x40,0x40,0x40,0x40,0x40,0x40,0x40,0x40,0x40,0x40,0x40,0x40, - 0x40,0x40,0x40,0x40,0x40,0x40,0x40,0x40,0x40,0x40,0x40,0x40 }; - static const BYTE image_080[24] = { 0x40,0x40,0x40,0x80,0x80,0x80,0x40,0x40,0x40,0x40,0x40,0x40, - 0x40,0x40,0x40,0x40,0x40,0x40,0x40,0x40,0x40,0x40,0x40,0x40 }; - static const BYTE image_100[24] = { 0x40,0x40,0x40,0x80,0x80,0x80,0x80,0x80,0x80,0x40,0x40,0x40, - 0x40,0x40,0x40,0x40,0x40,0x40,0x40,0x40,0x40,0x40,0x40,0x40 }; - static const BYTE image_120[24] = { 0x40,0x40,0x40,0x40,0x40,0x40,0x80,0x80,0x80,0x40,0x40,0x40, - 0x40,0x40,0x40,0x40,0x40,0x40,0x40,0x40,0x40,0x40,0x40,0x40 }; - static const BYTE image_150[24] = { 0x40,0x40,0x40,0x40,0x40,0x40,0x80,0x80,0x80,0x80,0x80,0x80, - 0x40,0x40,0x40,0x40,0x40,0x40,0x40,0x40,0x40,0x40,0x40,0x40 }; - static const BYTE image_180[24] = { 0x40,0x40,0x40,0x40,0x40,0x40,0x80,0x80,0x80,0x80,0x80,0x80, - 0x80,0x80,0x80,0x40,0x40,0x40,0x40,0x40,0x40,0x40,0x40,0x40 }; - static const BYTE image_200[24] = { 0x40,0x40,0x40,0x40,0x40,0x40,0x80,0x80,0x80,0x80,0x80,0x80, - 0x80,0x80,0x80,0x40,0x40,0x40,0x40,0x40,0x40,0x40,0x40,0x40 }; - static const BYTE image_250[24] = { 0x40,0x40,0x40,0x40,0x40,0x40,0x40,0x40,0x40,0x80,0x80,0x80, - 0x80,0x80,0x80,0x80,0x80,0x80,0x80,0x80,0x80,0x40,0x40,0x40 }; - static const BYTE image_120_half[24] = { 0x40,0x40,0x40,0x80,0x80,0x80,0x80,0x80,0x80,0x80,0x80,0x80, - 0x40,0x40,0x40,0x40,0x40,0x40,0x40,0x40,0x40,0x40,0x40,0x40 }; - static const BYTE image_150_half[24] = { 0x40,0x40,0x40,0x80,0x80,0x80,0x80,0x80,0x80,0x80,0x80,0x80, - 0x40,0x40,0x40,0x40,0x40,0x40,0x40,0x40,0x40,0x40,0x40,0x40 }; - static const BYTE image_200_half[24] = { 0x40,0x40,0x40,0x40,0x40,0x40,0x80,0x80,0x80,0x80,0x80,0x80, - 0x80,0x80,0x80,0x80,0x80,0x80,0x40,0x40,0x40,0x40,0x40,0x40 }; - static const BYTE image_250_half[24] = { 0x40,0x40,0x40,0x40,0x40,0x40,0x80,0x80,0x80,0x80,0x80,0x80, - 0x80,0x80,0x80,0x80,0x80,0x80,0x80,0x80,0x80,0x40,0x40,0x40 }; + static const BYTE back_8x1[24] = { 0x40,0x40,0x40, 0x40,0x40,0x40, 0x40,0x40,0x40, 0x40,0x40,0x40, + 0x40,0x40,0x40, 0x40,0x40,0x40, 0x40,0x40,0x40, 0x40,0x40,0x40 }; + static const BYTE image_080[24] = { 0x40,0x40,0x40, 0x80,0x80,0x80, 0x40,0x40,0x40, 0x40,0x40,0x40, + 0x40,0x40,0x40, 0x40,0x40,0x40, 0x40,0x40,0x40, 0x40,0x40,0x40 }; + static const BYTE image_100[24] = { 0x40,0x40,0x40, 0x80,0x80,0x80, 0x80,0x80,0x80, 0x40,0x40,0x40, + 0x40,0x40,0x40, 0x40,0x40,0x40, 0x40,0x40,0x40, 0x40,0x40,0x40 }; + static const BYTE image_120[24] = { 0x40,0x40,0x40, 0x40,0x40,0x40, 0x80,0x80,0x80, 0x40,0x40,0x40, + 0x40,0x40,0x40, 0x40,0x40,0x40, 0x40,0x40,0x40, 0x40,0x40,0x40 }; + static const BYTE image_150[24] = { 0x40,0x40,0x40, 0x40,0x40,0x40, 0x80,0x80,0x80, 0x80,0x80,0x80, + 0x40,0x40,0x40, 0x40,0x40,0x40, 0x40,0x40,0x40, 0x40,0x40,0x40 }; + static const BYTE image_180[24] = { 0x40,0x40,0x40, 0x40,0x40,0x40, 0x80,0x80,0x80, 0x80,0x80,0x80, + 0x80,0x80,0x80, 0x40,0x40,0x40, 0x40,0x40,0x40, 0x40,0x40,0x40 }; + static const BYTE image_200[24] = { 0x40,0x40,0x40, 0x40,0x40,0x40, 0x80,0x80,0x80, 0x80,0x80,0x80, + 0x80,0x80,0x80, 0x40,0x40,0x40, 0x40,0x40,0x40, 0x40,0x40,0x40 }; + static const BYTE image_250[24] = { 0x40,0x40,0x40, 0x40,0x40,0x40, 0x40,0x40,0x40, 0x80,0x80,0x80, + 0x80,0x80,0x80, 0x80,0x80,0x80, 0x80,0x80,0x80, 0x40,0x40,0x40 }; + static const BYTE image_120_half[24] = { 0x40,0x40,0x40, 0x80,0x80,0x80, 0x80,0x80,0x80, 0x80,0x80,0x80, + 0x40,0x40,0x40, 0x40,0x40,0x40, 0x40,0x40,0x40, 0x40,0x40,0x40 }; + static const BYTE image_150_half[24] = { 0x40,0x40,0x40, 0x80,0x80,0x80, 0x80,0x80,0x80, 0x80,0x80,0x80, + 0x40,0x40,0x40, 0x40,0x40,0x40, 0x40,0x40,0x40, 0x40,0x40,0x40 }; + static const BYTE image_200_half[24] = { 0x40,0x40,0x40, 0x40,0x40,0x40, 0x80,0x80,0x80, 0x80,0x80,0x80, + 0x80,0x80,0x80, 0x80,0x80,0x80, 0x40,0x40,0x40, 0x40,0x40,0x40 }; + static const BYTE image_250_half[24] = { 0x40,0x40,0x40, 0x40,0x40,0x40, 0x80,0x80,0x80, 0x80,0x80,0x80, + 0x80,0x80,0x80, 0x80,0x80,0x80, 0x80,0x80,0x80, 0x40,0x40,0x40 }; static const struct test_data { REAL scale_x; @@ -4753,20 +4753,20 @@ static void test_DrawImage_scale(void) { 0.8, PixelOffsetModeHalf, image_080 }, /* 14 */ { 1.0, PixelOffsetModeHalf, image_100 }, { 1.2, PixelOffsetModeHalf, image_120_half, TRUE }, - { 1.5, PixelOffsetModeHalf, image_150_half, TRUE }, + { 1.5, PixelOffsetModeHalf, image_150_half}, { 1.8, PixelOffsetModeHalf, image_180 }, - { 2.0, PixelOffsetModeHalf, image_200_half, TRUE }, - { 2.5, PixelOffsetModeHalf, image_250_half, TRUE }, + { 2.0, PixelOffsetModeHalf, image_200_half}, + { 2.5, PixelOffsetModeHalf, image_250_half}, { 0.8, PixelOffsetModeHighQuality, image_080 }, /* 21 */ { 1.0, PixelOffsetModeHighQuality, image_100 }, { 1.2, PixelOffsetModeHighQuality, image_120_half, TRUE }, - { 1.5, PixelOffsetModeHighQuality, image_150_half, TRUE }, + { 1.5, PixelOffsetModeHighQuality, image_150_half}, { 1.8, PixelOffsetModeHighQuality, image_180 }, - { 2.0, PixelOffsetModeHighQuality, image_200_half, TRUE }, - { 2.5, PixelOffsetModeHighQuality, image_250_half, TRUE }, + { 2.0, PixelOffsetModeHighQuality, image_200_half}, + { 2.5, PixelOffsetModeHighQuality, image_250_half}, }; - BYTE src_2x1[6] = { 0x80,0x80,0x80,0x80,0x80,0x80 }; + BYTE src_2x1[6] = { 0x80,0x80,0x80, 0x80,0x80,0x80 }; BYTE dst_8x1[24]; GpStatus status; union @@ -4802,16 +4802,19 @@ static void test_DrawImage_scale(void) status = GdipSetWorldTransform(graphics, matrix); expect(Ok, status); GdipDeleteMatrix(matrix); - memcpy(dst_8x1, back_8x1, sizeof(dst_8x1)); status = GdipDrawImageI(graphics, u1.image, 1, 0); expect(Ok, status); + match = memcmp(dst_8x1, td[i].image, sizeof(dst_8x1)) == 0; todo_wine_if (!match && td[i].todo) ok(match, "%d: data should match\n", i); if (!match) - trace("%s\n", dbgstr_hexdata(dst_8x1, sizeof(dst_8x1))); + { + trace("Expected: %s\n", dbgstr_hexdata(td[i].image, sizeof(dst_8x1))); + trace("Got: %s\n", dbgstr_hexdata(dst_8x1, sizeof(dst_8x1))); + } } status = GdipDeleteGraphics(graphics); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3568
Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=136163 Your paranoid android. === debian11b (64 bit WoW report) === gdiplus: image.c:4812: Test failed: 14: data should match image.c:4812: Test failed: 21: data should match
Esme Povirk (@madewokherd) commented about dlls/gdiplus/graphics.c:
if (dst_area.bottom < pti[i].y) dst_area.bottom = pti[i].y; }
- stat = get_graphics_device_bounds(graphics, &graphics_bounds); - if (stat != Ok) return stat; - if (graphics_bounds.X > dst_area.left) dst_area.left = floorf(graphics_bounds.X); if (graphics_bounds.Y > dst_area.top) dst_area.top = floorf(graphics_bounds.Y); if (graphics_bounds.X + graphics_bounds.Width < dst_area.right) dst_area.right = ceilf(graphics_bounds.X + graphics_bounds.Width); if (graphics_bounds.Y + graphics_bounds.Height < dst_area.bottom) dst_area.bottom = ceilf(graphics_bounds.Y + graphics_bounds.Height);
+ dst_area_left = dst_area.left; + dst_area_top = dst_area.top; + dst_area_right = dst_area.right; + dst_area_bottom = dst_area.bottom;
`dst_area` is an optimization to limit the number of pixels rendered. It's not meaningful for this to have a non-integer value. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3568#note_42390
On Tue Aug 15 20:12:05 2023 +0000, Esme Povirk wrote:
`dst_area` is an optimization to limit the number of pixels rendered. It's not meaningful for this to have a non-integer value. I will investigate it further. I think I need improve tests first
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3568#note_42424
This merge request was closed by Bartosz Kosiorek. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3568
On Wed Aug 16 06:55:37 2023 +0000, Bartosz Kosiorek wrote:
I will investigate it further. I think I need improve tests first I assume the documented function is what it actually does, basically translating device coordinates by half a pixel on each axis. That shouldn't require a separate implementation for each drawing function.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3568#note_42500
participants (4)
-
Bartosz Kosiorek -
Bartosz Kosiorek (＠gang65)
-
Esme Povirk (＠madewokherd)
-
Marvin