Re: gdiplus:GdipDrawImagePointsRect has a nosie vertical line output after 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
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"<sebastian(a)fds-team.de>; Date: Sat, Nov 22, 2014 00:19 AM To: "wine-devel"<wine-devel(a)winehq.org>; "liuchanghui"<liuchanghui(a)linuxdeepin.com>; 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
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.
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"<madewokherd(a)gmail.com>; Date: Sun, Nov 23, 2014 12:57 PM To: "刘昌辉"<liuchanghui(a)linuxdeepin.com>; Cc: "Sebastian Lackner"<sebastian(a)fds-team.de>; "wine-devel"<wine-devel(a)winehq.org>; 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.
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 <liuchanghui(a)linuxdeepin.com> 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"<madewokherd(a)gmail.com>; Date: Sun, Nov 23, 2014 12:57 PM To: "刘昌辉"<liuchanghui(a)linuxdeepin.com>; Cc: "Sebastian Lackner"<sebastian(a)fds-team.de>; "wine-devel"<wine-devel(a)winehq.org>; 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.
participants (4)
-
Liu Changhui -
Sebastian Lackner -
Vincent Povirk -
刘昌辉