Re: gdiplus: fix GdipFillRectangleI no effect on memory DC whose windoworigin point has changed

[Vincent Povirk]

-        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?

gdiplus: fix GdipFillRectangleI no effect on memory DC whosewindoworigin point has changed

[Changhui Liu]

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?

gdiplus: fix GdipFillRectangleI no effect on memory DC whosewindoworigin point has changed

[Vincent Povirk]

-    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?

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?

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

[Qian Hong]

Hi  Changhui,

Thanks for your work.

On Thu, Dec 11, 2014 at 3:19 PM, Changhui Liu
 wrote:
> I wanted to modify ExtSelectClipRgn, but after I replace the native
> gdiplus.dll,

I wouldn't rely on native gdiplus.dll on Wine too much, there is no
guarantee that Wine's gdi32 is already compatible enough
 with Windows, a better way to test gdiplus is to submit a patch to
Wine's testbot or using your own Windows.

See http://wiki.winehq.org/WineTestBot and
http://wiki.winehq.org/CompilingDLLsUsingMingw

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

[Vincent Povirk]

> 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.

Ah, you're right, sorry.
> 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 would prefer that the image be made larger and some points outside
the fill region be tested.
> I wanted to modify ExtSelectClipRgn, but after I replace the native
> gdiplus.dll,
> my test passed, so I think the SelectClipRgn is correct.

Native gdiplus probably works differently. As a rule, we do not check
what API calls native makes when implementing our version, to make
sure we ours is an independent implementation and not affected by
Microsoft's copyright.

But Wine's gdi32 is more mature and well-tested than gdiplus, so
ExtSelectClipRgn probably works correctly.
> 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:

I see, this is a larger problem than it first appeared because we must
account for this behavior every time we set a clipping region.

I believe we should modify SOFTWARE_GdipFillRegion,
alpha_blend_pixels_hrgn's call to CreateRectRgn, and get_clip_hrgn
separately.

The other functions using get_clip_hrgn will have a similar problem
(if a clipping region is set on the Graphics object), and changing it
will fix them as well.

For get_clip_hrgn and SOFTWARE_GdipFillRegion, I would create a new
function that takes a GpGraphics* and GpRegion* in gdiplus device
coordinates, and returns an HRGN in gdi32 device coodrinates. For now,
this would just call GdipGetRegionHRgn followed by OffsetRgn where
necessary, but it gives us the ability to do something more
complicated if we need to in the future.

I don't think we should check GetMapMode(). That was my mistake when I
started to account for transforms in get_graphics_bounds.

The necessary changes to get_graphics_bounds, SOFTWARE_GdipFillRegion,
get_clip_hrgn, and alpha_blend_pixels_hrgn should be split into
separate patches, and you can follow them with your test.

gdiplus: fix GdipFillRectangleI no effect on memoryDCwhosewindoworigin point has changed

[Changhui Liu]

Dear all,
I have modified the patch and add more test functions, thank you for your advice.

Because I found that graphics->hdc and graphics->hwnd are both nil when this condition statement: 
    if (graphics->image && graphics->image->type == ImageTypeBitmap)
is true in alpha_blend_pixels_hrgn function,    
So I think there's no chance to change the window origin point for graphics who created from a bitmap image .

So I not modify alpha_blend_pixels_hrgn, I think the changes in get_clip_hrgn and
SOFTWARE_GdipFillRegion will not affect alpha_blend_pixels_hrgn.  
My thought was wrong about alpha_blend_pixels_hrgn in last mail.

Please help me review it again when you are free. 
Thanks a lot.

------------------
Regards.

------------------ Original ------------------
From:  "Vincent Povirk";
Date:  Fri, Dec 12, 2014 01:39 AM
To:  "Changhui Liu"; 
Cc:  "[e-mail]"; 
Subject:  Re: gdiplus: fix GdipFillRectangleI no effect on memoryDCwhosewindoworigin point has changed
> 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.

Ah, you're right, sorry.
> 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 would prefer that the image be made larger and some points outside
the fill region be tested.
> I wanted to modify ExtSelectClipRgn, but after I replace the native
> gdiplus.dll,
> my test passed, so I think the SelectClipRgn is correct.

Native gdiplus probably works differently. As a rule, we do not check
what API calls native makes when implementing our version, to make
sure we ours is an independent implementation and not affected by
Microsoft's copyright.

But Wine's gdi32 is more mature and well-tested than gdiplus, so
ExtSelectClipRgn probably works correctly.
> 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:

I see, this is a larger problem than it first appeared because we must
account for this behavior every time we set a clipping region.

I believe we should modify SOFTWARE_GdipFillRegion,
alpha_blend_pixels_hrgn's call to CreateRectRgn, and get_clip_hrgn
separately.

The other functions using get_clip_hrgn will have a similar problem
(if a clipping region is set on the Graphics object), and changing it
will fix them as well.

For get_clip_hrgn and SOFTWARE_GdipFillRegion, I would create a new
function that takes a GpGraphics* and GpRegion* in gdiplus device
coordinates, and returns an HRGN in gdi32 device coodrinates. For now,
this would just call GdipGetRegionHRgn followed by OffsetRgn where
necessary, but it gives us the ability to do something more
complicated if we need to in the future.

I don't think we should check GetMapMode(). That was my mistake when I
started to account for transforms in get_graphics_bounds.

The necessary changes to get_graphics_bounds, SOFTWARE_GdipFillRegion,
get_clip_hrgn, and alpha_blend_pixels_hrgn should be split into
separate patches, and you can follow them with your test.

gdiplus: fix GdipFillRectangleI no effect on memoryDCwhosewindoworigin point has changed

[Vincent Povirk]

Thank you for continuing to work on this.

It is possible for graphics->hdc to not be NULL when graphics->image
is a Bitmap object. You can see in GdipCreateBitmapFromScan0 that we
create an HBITMAP object for some pixel formats, and
GdipGetImageGraphicsContext will create an HDC if an HBITMAP exists.
This is currently necessary because our non-gdi32 drawing code is
incomplete, but when that is fixed we will no longer create HBITMAP or
HDC objects for Bitmap objects.

We should still leave alpha_blend_pixels_hrgn alone because we control
these HDC objects and will never change their window origin.

Your approach looks good, but I have some issues with the tests.

I would prefer that you use the expect() macro when comparing integers in tests.

If you don't expect a function (in this case GetDIBits, in patch 2) to
fail when running a test, use ok() to check that it succeeded. It's OK
if the test will crash when the ok() check fails.

If GetDIBits really does fail sometimes, we need to understand why
that happens and document it in the test.

When writing comments about this, make sure you specify that you are
talking about gdi32 device coordinates, which are different from
gdiplus device coordinates.

+    status = GdipSetClipRectI(graphics, rect.left+width/2, rect.top+height/2,
+            rect.right, rect.bottom, CombineModeReplace);

This looks wrong to me. GdipSetClipRectI takes a width and height, not
right and bottom.