Re: gdiplus: fix GdipFillRectangleI no effect on memory DCwhosewindoworigin point has changed

[Changhui Liu]

Hi,
> - /*build a blue solid image to fill rectangle*/
>
> Is this really necessary? Can't we use a solid brush?

Yes, if we use a solid brush, the GdipFillRectangleI will call GDI32_GdipFillPath,
then call brush_fill_path -> gdi32.FillPath , not call SOFTWARE_GdipFillRegion,
so GdipFillRectangleI will take effect.
> - status = GdipFillRectangleI(graphics, (GpBrush*)brush, 0, 0,
>
> IMG_WIDTH*2, IMG_HEIGHT*2);
> - expect(Ok, status);
>
> This makes it unclear where the rectangle is located in the final
> image. We should leave part of the image unaffected by the fill, so we
> can check points outside of it.

My test aim is that expect GdipFillRectangleI filled the whole HDC image.
The HDC image is in a rectangle (4, 4)-(12, 12),
and the filled rectangle by GdipFillRectangleI is (0, 0)-(16, 16),
so the correct fill region should be (4,4)-(12,12), but the fill region
computed by the no patched code is (4,4)-(8,8).
> I still think changing the region in alpha_blend_pixels_hrgn is wrong.
> Do I understand correctly that gdi32 does not apply the world
> transform (including the window origin) to the HREGION when we set it
> as a clipping region? If so, I think that the caller of
> alpha_blend_pixels_hrgn must apply the transform.

Yes, it is like that.
I wanted to modify ExtSelectClipRgn, but after I replace the native gdiplus.dll,
my test passed, so I think the SelectClipRgn is correct.

I wanted to apply the transform in SOFTWARE_GdipFillRegion before call
alpha_blend_pixels_hrgn, but this changes will affect these code in alpha_blend_pixels_hrgn:

if (graphics->image && graphics->image->type == ImageTypeBitmap)
    {
        ...

hrgn = CreateRectRgn(dst_x, dst_y, dst_x + src_width, dst_y + src_height);

...

stat = get_clip_hrgn(graphics, &visible_rgn);

...

if (visible_rgn)
        {
            CombineRgn(hrgn, hrgn, visible_rgn, RGN_AND);
            DeleteObject(visible_rgn);
        }

if (hregion)
            CombineRgn(hrgn, hrgn, hregion, RGN_AND);
        ...
    }

------------------ Original ------------------
From:  "Vincent Povirk";
Date:  Thu, Dec 11, 2014 03:30 AM
To:  "Changhui Liu"; 
Cc:  "[e-mail]"; 
Subject:  Re: gdiplus: fix GdipFillRectangleI no effect on memory DCwhosewindoworigin point has changed

-    if (graphics->hdc &&
-        (GetMapMode(graphics->hdc) != MM_TEXT ||
GetGraphicsMode(graphics->hdc) != GM_COMPATIBLE))
+    if (graphics->hdc)

I think this is a correct change.

+    /*build a blue solid image to fill rectangle*/

Is this really necessary? Can't we use a solid brush?

+    status = GdipFillRectangleI(graphics, (GpBrush*)brush, 0, 0,
IMG_WIDTH*2, IMG_HEIGHT*2);
+    expect(Ok, status);

This makes it unclear where the rectangle is located in the final
image. We should leave part of the image unaffected by the fill, so we
can check points outside of it.

I still think changing the region in alpha_blend_pixels_hrgn is wrong.
Do I understand correctly that gdi32 does not apply the world
transform (including the window origin) to the HREGION when we set it
as a clipping region? If so, I think that the caller of
alpha_blend_pixels_hrgn must apply the transform.

I think that SOFTWARE_GdipFillRegion will have to calculate two
regions: one in gdi32 logical coordinates to find the bounding box and
one in gdi32 device coordinates to pass to alpha_blend_pixels_hrgn.

Before we do this, we should also add a case to test_transformpoints,
in which we change the window origin of an HDC and then use it to
create a graphics object. I believe GdipTransformPoints will be
unaffected by the HDC, but if it is affected then we will need to take
a very different approach to solving this problem.

On Wed, Dec 10, 2014 at 12:33 AM, Changhui Liu
 wrote:
> Hi,
> After debug the get_graphics_bounds, I found that the default mapping mode
> and graphics
> mode of graphics->hdc are:
> MapMode=MM_TEXT, GraphicsMode=GM_COMPATIBLE.
>
> If we don't use GetWindowOrgEx, then change this condition statement:
> if (graphics->hdc &&
>    (GetMapMode(graphics->hdc) != MM_TEXT || GetGraphicsMode(graphics->hdc)
> != GM_COMPATIBLE))
>
> to
>
> if (graphics->hdc )
>
> is OK ?
> > If I fill a
> > rectangle at (0,0) using GDI+, on an HDC with window origin (5,5),
> > what is the origin of that rectangle on the screen?
>
> I think the answer is (5,5).
>
> Last, I modified my patch and test, please help me review it again when you
> are free.
> Thanks you.
>
> ------------------ Original ------------------
> From:  "Vincent Povirk";
> Date:  Wed, Dec 10, 2014 01:51 AM
> To:  "[e-mail]";
> Cc:  "刘昌辉";
> Subject:  Re: gdiplus: fix GdipFillRectangleI no effect on memory DC
> whosewindoworigin point has changed
> -
>    rect->X = 0;
> -
>    rect->Y = 0;
> -
>    /*The user maybe has called SetWindowOrgEx to change origin point*/
> -
>    POINT pt = {0, 0};
> -
>    GetWindowOrgEx(graphics->hdc, &pt);
> -
>    rect->X = pt.x;
> -
>    rect->Y = pt.y;
>
> I don't think we should use GetWindowOrgEx in gdiplus. There are too
> many ways to change the mapping of co-ordinates for an HDC, and it
> doesn't make sense to specifically account for them all.
>
> In get_graphics_bounds, we already have code that accounts for the HDC
> transform using DPtoLP. Maybe we should do that even when GetMapMode
> == MM_TEXT.
>
> Transforming the region in alpha_blend_pixels_hrgn looks wrong to me,
> because we can't do that in the general case. Logical coordinates on
> the HDC may not be pixels.
>
> Your test is very unclear. A good test for this should make it obvious
> how the HDC's transform affects GDI+ world coordinates. If I fill a
> rectangle at (0,0) using GDI+, on an HDC with window origin (5,5),
> what is the origin of that rectangle on the screen?