[PATCH v7 0/1] MR3865: gdiplus: implement PixelOffsetMode Half and HighQuality for GdipDrawImagePointsRect
-- v7: gdiplus: implement PixelOffsetMode Half and HighQuality for GdipDrawImagePointsRect https://gitlab.winehq.org/wine/wine/-/merge_requests/3865
From: Bartosz Kosiorek <gang65(a)poczta.onet.pl> Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=53947 --- dlls/gdiplus/graphics.c | 62 +++++++++++++++++++++++++++----------- dlls/gdiplus/tests/image.c | 38 +++++++++++++---------- 2 files changed, 67 insertions(+), 33 deletions(-) diff --git a/dlls/gdiplus/graphics.c b/dlls/gdiplus/graphics.c index 65b33cdc960..980726bbb9c 100644 --- a/dlls/gdiplus/graphics.c +++ b/dlls/gdiplus/graphics.c @@ -1072,7 +1072,7 @@ static ARGB resample_bitmap_pixel(GDIPCONST GpRect *src_rect, LPBYTE bits, UINT break; } return sample_bitmap_pixel(src_rect, bits, width, height, - floorf(point->X + pixel_offset), floorf(point->Y + pixel_offset), attributes); + (INT)(point->X + pixel_offset), (INT)(point->Y + pixel_offset), attributes); } } @@ -3094,6 +3094,7 @@ 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); @@ -3131,6 +3132,7 @@ GpStatus WINGDIPAPI GdipDrawImagePointsRect(GpGraphics *graphics, GpImage *image if (use_software) { RECT dst_area; + REAL dst_pixel_offset, 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 +3145,50 @@ 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; + 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 > 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 (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 = 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); + 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; + + // Based on value of dst_area, the pixel number to draw is calculated. + if ((offset_mode == PixelOffsetModeHalf) || + (offset_mode == PixelOffsetModeHighQuality)) + { + // Based on experiments dst_area with value 0.5 should be rounded to 0.0 + // (rounding half down), that't why we are adding 0.4999 instead of 0.5 + dst_area.left = (INT)(dst_area_left + 0.4999); + dst_area.top = (INT)(dst_area_top + 0.4999); + dst_area.right = (INT)(dst_area_right + 0.4999); + dst_area.bottom = (INT)(dst_area_bottom + 0.4999); + + // With PixelOffsetMode Half we are checking color in the middle of destination pixel, + // that's why we are adding 0.5 to destination. + dst_pixel_offset = 0.5; + } + else + { + // PixelOffsetMode None and HighSpeed is not drawing outside dst_area, + // that's why we are using ceilf and floorf. + dst_area.left = ceilf(dst_area_left); + dst_area.top = ceilf(dst_area_top); + dst_area.right = floorf(dst_area_right); + dst_area.bottom = floorf(dst_area_bottom); + + dst_pixel_offset = 0.0; + } TRACE("dst_area: %s\n", wine_dbgstr_rect(&dst_area)); @@ -3250,9 +3279,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 + dst_pixel_offset) * x_dx + (dst_area.top + dst_pixel_offset) * y_dx; src_pointf_row.Y = dst_to_src.matrix[5] + - dst_area.left * x_dy + dst_area.top * y_dy; + (dst_area.left + dst_pixel_offset) * x_dy + (dst_area.top + dst_pixel_offset) * y_dy; for (y = dst_area.top; y < dst_area.bottom; y++, src_pointf_row.X += y_dx, src_pointf_row.Y += y_dy) @@ -3260,10 +3289,9 @@ GpStatus WINGDIPAPI GdipDrawImagePointsRect(GpGraphics *graphics, GpImage *image for (x = dst_area.left, src_pointf = src_pointf_row; x < dst_area.right; x++, src_pointf.X += x_dx, src_pointf.Y += x_dy) { - if (src_pointf.X >= srcx && src_pointf.X < srcx + srcwidth && - src_pointf.Y >= srcy && src_pointf.Y < srcy + srcheight) - *dst_color = resample_bitmap_pixel(&src_area, src_data, bitmap->width, bitmap->height, &src_pointf, - imageAttributes, interpolation, offset_mode); + + *dst_color = resample_bitmap_pixel(&src_area, src_data, bitmap->width, bitmap->height, &src_pointf, + imageAttributes, interpolation, offset_mode); dst_color++; } } diff --git a/dlls/gdiplus/tests/image.c b/dlls/gdiplus/tests/image.c index 18fe8fcba37..84beee49cd2 100644 --- a/dlls/gdiplus/tests/image.c +++ b/dlls/gdiplus/tests/image.c @@ -4618,8 +4618,10 @@ static void test_DrawImage(void) match = memcmp(white_2x2, black_2x2, sizeof(black_2x2)) == 0; ok(match, "data should match\n"); if (!match) - trace("%s\n", dbgstr_hexdata(white_2x2, sizeof(white_2x2))); - + { + trace("Expected: %s\n", dbgstr_hexdata(black_2x2, sizeof(black_2x2))); + trace("Got: %s\n", dbgstr_hexdata(white_2x2, sizeof(white_2x2))); + } status = GdipDeleteGraphics(graphics); expect(Ok, status); status = GdipDisposeImage(u1.image); @@ -4708,7 +4710,10 @@ static void test_GdipDrawImagePointRect(void) match = memcmp(white_2x2, black_2x2, sizeof(black_2x2)) == 0; ok(match, "data should match\n"); if (!match) - trace("%s\n", dbgstr_hexdata(white_2x2, sizeof(white_2x2))); + { + trace("Expected: %s\n", dbgstr_hexdata(black_2x2, sizeof(black_2x2))); + trace("Got: %s\n", dbgstr_hexdata(white_2x2, sizeof(white_2x2))); + } status = GdipDeleteGraphics(graphics); expect(Ok, status); @@ -4825,12 +4830,13 @@ static void test_DrawImage_scale(void) 0xcc,0xcc,0xcc, 0xcc,0xcc,0xcc, 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, 0xcc,0xcc,0xcc, 0xcc,0xcc,0xcc, 0x40,0x40,0x40 }; + static const BYTE image_251_half[24] = { 0x40,0x40,0x40, 0x40,0x40,0x40, 0x40,0x40,0x40, 0x80,0x80,0x80, + 0x80,0x80,0x80, 0xcc,0xcc,0xcc, 0xcc,0xcc,0xcc, 0xcc,0xcc,0xcc }; static const struct test_data { REAL scale_x; PixelOffsetMode pixel_offset_mode; const BYTE *image; - BOOL todo; } td[] = { { 0.8, PixelOffsetModeNone, image_080 }, /* 0 */ @@ -4851,19 +4857,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.8, PixelOffsetModeHalf, image_180_half, TRUE }, - { 2.0, PixelOffsetModeHalf, image_200_half, TRUE }, - { 2.5, PixelOffsetModeHalf, image_250_half, TRUE }, + { 1.2, PixelOffsetModeHalf, image_120_half }, + { 1.5, PixelOffsetModeHalf, image_150_half }, + { 1.8, PixelOffsetModeHalf, image_180_half }, + { 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.8, PixelOffsetModeHighQuality, image_180_half, TRUE }, - { 2.0, PixelOffsetModeHighQuality, image_200_half, TRUE }, - { 2.5, PixelOffsetModeHighQuality, image_250_half, TRUE }, + { 1.2, PixelOffsetModeHighQuality, image_120_half }, + { 1.5, PixelOffsetModeHighQuality, image_150_half }, + { 1.8, PixelOffsetModeHighQuality, image_180_half }, + { 2.0, PixelOffsetModeHighQuality, image_200_half }, + { 2.5, PixelOffsetModeHighQuality, image_250_half }, + { 2.51, PixelOffsetModeHighQuality, image_251_half }, }; BYTE src_2x1[6] = { 0x80,0x80,0x80, 0xcc,0xcc,0xcc }; BYTE dst_8x1[24]; @@ -4907,8 +4914,7 @@ static void test_DrawImage_scale(void) 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); + ok(match, "%d: data should match\n", i); if (!match) { trace("Expected: %s\n", dbgstr_hexdata(td[i].image, sizeof(dst_8x1))); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3865
On Sat Sep 16 17:36:13 2023 +0000, Jeffrey Smith wrote:
"Banker's rounding" is the term I usually hear for this: round to nearest, but for ###.5 it rounds down if ### is even and up if ### is odd. It is considered more numerically stable when summing large sets of values, so it's very common in financial and statistics contexts. As such, it shows up lots of places in computing, and it's the default for .NET's Math.Round(). https://en.wikipedia.org/wiki/Rounding#Rounding_half_to_even Is there a reason it's 3 9's, specifically?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3865#note_45653
Esme Povirk (@madewokherd) commented about dlls/gdiplus/graphics.c:
imageAttributes = &defaultImageAttributes;
- dst_area.left = dst_area.right = pti[0].x; - dst_area.top = dst_area.bottom = pti[0].y; + 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 > 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 (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;
I don't understand why these were changed to floats. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3865#note_45654
Esme Povirk (@madewokherd) commented about dlls/gdiplus/graphics.c:
+ if (graphics_bounds.Y + graphics_bounds.Height < dst_area_bottom) dst_area_bottom = graphics_bounds.Y + graphics_bounds.Height; + + // Based on value of dst_area, the pixel number to draw is calculated. + if ((offset_mode == PixelOffsetModeHalf) || + (offset_mode == PixelOffsetModeHighQuality)) + { + // Based on experiments dst_area with value 0.5 should be rounded to 0.0 + // (rounding half down), that't why we are adding 0.4999 instead of 0.5 + dst_area.left = (INT)(dst_area_left + 0.4999); + dst_area.top = (INT)(dst_area_top + 0.4999); + dst_area.right = (INT)(dst_area_right + 0.4999); + dst_area.bottom = (INT)(dst_area_bottom + 0.4999); + + // With PixelOffsetMode Half we are checking color in the middle of destination pixel, + // that's why we are adding 0.5 to destination. + dst_pixel_offset = 0.5; This seems complicated for what I'd expect is equivalent to a different set of destination points in device space.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3865#note_45655
Esme Povirk (@madewokherd) commented about dlls/gdiplus/graphics.c:
for (x = dst_area.left, src_pointf = src_pointf_row; x < dst_area.right; x++, src_pointf.X += x_dx, src_pointf.Y += x_dy) { - if (src_pointf.X >= srcx && src_pointf.X < srcx + srcwidth && - src_pointf.Y >= srcy && src_pointf.Y < srcy + srcheight) +
We still need this check for non-aligned destination rectangles. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3865#note_45656
I really think you should try doing the thing MSDN says PixelOffsetMode does, and treat it as an adjustment of the coordinate space. Changing dst_area doesn't make sense because it's only a bounding box that includes the output area, not the full output area (which isn't always rectangular and might be smaller even when it is). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3865#note_45657
Specifically, I think `get_graphics_transform` should apply the pixel offset when transforming between `WineCoordinateSpaceGdiDevice` and other coordinate spaces. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3865#note_45658
On Sat Sep 16 20:44:43 2023 +0000, Esme Povirk wrote:
I don't understand why these were changed to floats. To be able to convert it to integer, according to pixel offset mode.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3865#note_45668
Thanks. I will create new PR then. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3865#note_45669
This merge request was closed by Bartosz Kosiorek. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3865
participants (3)
-
Bartosz Kosiorek -
Bartosz Kosiorek (＠gang65)
-
Esme Povirk (＠madewokherd)