Re: gdiplus:GdipDrawImagePointsRect has a nosie vertical line output after resampled

[Sebastian Lackner]

Hi,

while looking quickly over your patch I noticed a couple of mistakes / things you should probably change:

On 21.11.2014 15:32, 刘昌辉 wrote:
> - int src_img_height = src_rect.Height;
> - int dest_img_width = dest_rect.X + dest_rect.Width;
> - int dest_img_height = dest_rect.Height;
> - BYTE* src_img_data = malloc (src_img_width*src_img_height*4);
> - BYTE* dest_img_data = malloc (dest_img_width*dest_img_height*4);

You shouldn't use malloc(...) and free(...) in wine source, use HeapAlloc(...) and HeapFree(...) instead.
> -
> - /*first line*/
> - value = *( (DWORD*)dest_img_data + dest_rect.X) ;
> - /*printf ("pixel(%d,%d) = %08x\n", dest_rect.X, 0, value );*/

I assume the printf was for debugging, remove it, or if it is really very important then use trace(...) instead.
Other tests use GdipBitmapGetPixel(...) for reading the color of specific pixels. If its possible to use that here too, it would look a bit better than directly accessing the memory.
> - ok ( value > 0,
> -
>        "dest image color should same as src image color\n");

You are switching between different styles in your patch several times. Try to make the style match a bit more the sourrounding code, especially whitespace before and after functions. In this case a linebreak is also not really necessary for example, other lines are much longer.

Regards,
Sebastian

gdiplus:GdipDrawImagePointsRect has a nosie vertical line outputafter resampled

[刘昌辉]

Dear All, 
I'm sorry for my mistakes. I modified my patch, please review it again if you have time.
Thanks a lot!

------------------ Original ------------------
From:  "Sebastian Lackner";
Date:  Sat, Nov 22, 2014 00:19 AM
To:  "wine-devel"; "liuchanghui";

Subject:  Re: gdiplus:GdipDrawImagePointsRect has a nosie vertical line outputafter resampled

Hi,

while looking quickly over your patch I noticed a couple of mistakes / things you should probably change:

On 21.11.2014 15:32, 刘昌辉 wrote:
> - int src_img_height = src_rect.Height;
> - int dest_img_width = dest_rect.X + dest_rect.Width;
> - int dest_img_height = dest_rect.Height;
> - BYTE* src_img_data = malloc (src_img_width*src_img_height*4);
> - BYTE* dest_img_data = malloc (dest_img_width*dest_img_height*4);

You shouldn't use malloc(...) and free(...) in wine source, use HeapAlloc(...) and HeapFree(...) instead.
> -
> - /*first line*/
> - value = *( (DWORD*)dest_img_data + dest_rect.X) ;
> - /*printf ("pixel(%d,%d) = %08x\n", dest_rect.X, 0, value );*/

I assume the printf was for debugging, remove it, or if it is really very important then use trace(...) instead.
Other tests use GdipBitmapGetPixel(...) for reading the color of specific pixels. If its possible to use that here too, it would look a bit better than directly accessing the memory.
> - ok ( value > 0,
> -
>        "dest image color should same as src image color\n");

You are switching between different styles in your patch several times. Try to make the style match a bit more the sourrounding code, especially whitespace before and after functions. In this case a linebreak is also not really necessary for example, other lines are much longer.

Regards,
Sebastian

gdiplus:GdipDrawImagePointsRect has a nosie vertical line outputafter resampled

[Vincent Povirk]

This will cause us to draw outside of the destination when the image
is both enlarged and rotated.

That if statement is there to make sure we only draw to the area
inside the shape defined by the points array. src_pointf should be
within the range defined by srcx/srcy/srcwidth/srcheight if and only
if (x,y) is within that shape. If that's not the case, it probably
means we're calculating src_pointf incorrectly for the whole image,
not just the edges.

I'm not sure Sebastian realized this, but in this case
GdipBitmapGetPixel is different from reading the image directly.
Because PixelFormat32bppRGB does not store alpha data in the bitmap,
GdipBitmapGetPixel will always return an alpha value of 255, and you
will get a non-zero value even if the pixel is black. I suggest
testing for the exact value you expect. That should also remove the
need for the trace() calls.

gdiplus:GdipDrawImagePointsRect has a nosie vertical lineoutputafter resampled

[Liu Changhui]

Dear Vincent: 
The PixelFormat32bppRGB shuld be PixelFormat32bppARGB in the 
test_GdipDrawImagePointsRectNeedResample function of my patch.

In test_GdipDrawImagePointsRectNeedResample,  the first src_pointf valuse is :
X=5.992676, src_pointf.Y=0.000000 when the destination pixel is(x=699,y=0),
and srcx=6.000000,srcy=0.000000,srcwidth=48.000000,srcheight=26.000000,

so the follow if statement will test false :
if (src_pointf.X >= srcx && src_pointf.X < srcx + srcwidth && src_pointf.Y >= srcy && src_pointf.Y < srcy+srcheight)

then the destination pixel (x=699,y=0) will be filled with zero value,
but we expect that pixel shoule be filled a color same as the source image.

I think make these variables:
dst_to_src_points, src_pointf, x_dx, x_dy, y_dx, y_dy all use 64bit double float type maybe can fixed this situation. 
But GpointF and GpMatrix has used 32bit float type, and many gdiplus functions depended on them.

Can you give me some advice ? Thank you!

------------------ Original ------------------
From:  "Vincent Povirk";
Date:  Sun, Nov 23, 2014 12:57 PM
To:  "刘昌辉"; 
Cc:  "Sebastian Lackner"; "wine-devel"; 
Subject:  Re: gdiplus:GdipDrawImagePointsRect has a nosie vertical lineoutputafter resampled

This will cause us to draw outside of the destination when the image
is both enlarged and rotated.

That if statement is there to make sure we only draw to the area
inside the shape defined by the points array. src_pointf should be
within the range defined by srcx/srcy/srcwidth/srcheight if and only
if (x,y) is within that shape. If that's not the case, it probably
means we're calculating src_pointf incorrectly for the whole image,
not just the edges.

I'm not sure Sebastian realized this, but in this case
GdipBitmapGetPixel is different from reading the image directly.
Because PixelFormat32bppRGB does not store alpha data in the bitmap,
GdipBitmapGetPixel will always return an alpha value of 255, and you
will get a non-zero value even if the pixel is black. I suggest
testing for the exact value you expect. That should also remove the
need for the trace() calls.

gdiplus:GdipDrawImagePointsRect has a nosie vertical lineoutputafter resampled

[Vincent Povirk]

I don't think switching to doubles will help. The extra precision
should get our result closer to 6.0, but 5.999999 will still fail.

We should look for a more direct way to calculate src_pointf, to avoid
introducing rounding errors. Maybe if we start by subtracting ptf[0]
from dst_point, we can more directly transform that to coordinates
relative to the source rectangle, then add srcx,srcy. This needs more
thought than I can put in right now, but I suspect the current
solution is more complicated than it needs to be.

On Sun, Nov 23, 2014 at 9:23 PM, Liu Changhui
 wrote:
> Dear Vincent:
> The PixelFormat32bppRGB shuld be PixelFormat32bppARGB in the
> test_GdipDrawImagePointsRectNeedResample function of my patch.
>
> In test_GdipDrawImagePointsRectNeedResample,  the first src_pointf valuse is
> :
> X=5.992676, src_pointf.Y=0.000000 when the destination pixel is(x=699,y=0),
> and srcx=6.000000,srcy=0.000000,srcwidth=48.000000,srcheight=26.000000,
>
> so the follow if statement will test false :
> if (src_pointf.X >= srcx && src_pointf.X < srcx + srcwidth && src_pointf.Y
> > = srcy && src_pointf.Y < srcy+srcheight)
>
> then the destination pixel (x=699,y=0) will be filled with zero value,
> but we expect that pixel shoule be filled a color same as the source image.
>
> I think make these variables:
> dst_to_src_points, src_pointf, x_dx, x_dy, y_dx, y_dy all use 64bit double
> float type maybe can fixed this situation.
> But GpointF and GpMatrix has used 32bit float type, and many gdiplus
> functions depended on them.
>
> Can you give me some advice ? Thank you!
>
> ------------------ Original ------------------
> From:  "Vincent Povirk";
> Date:  Sun, Nov 23, 2014 12:57 PM
> To:  "刘昌辉";
> Cc:  "Sebastian Lackner";
> "wine-devel";
> Subject:  Re: gdiplus:GdipDrawImagePointsRect has a nosie vertical
> lineoutputafter resampled
>
> This will cause us to draw outside of the destination when the image
> is both enlarged and rotated.
>
> That if statement is there to make sure we only draw to the area
> inside the shape defined by the points array. src_pointf should be
> within the range defined by srcx/srcy/srcwidth/srcheight if and only
> if (x,y) is within that shape. If that's not the case, it probably
> means we're calculating src_pointf incorrectly for the whole image,
> not just the edges.
>
> I'm not sure Sebastian realized this, but in this case
> GdipBitmapGetPixel is different from reading the image directly.
> Because PixelFormat32bppRGB does not store alpha data in the bitmap,
> GdipBitmapGetPixel will always return an alpha value of 255, and you
> will get a non-zero value even if the pixel is black. I suggest
> testing for the exact value you expect. That should also remove the
> need for the trace() calls.