Re: [5/5] gdiplus: Add some tests for GdipBitmapLockBits/GdipBitmapUnlockBits.

[Vincent Povirk]

Shouldn't your second LockBits in this loop always use
PixelFormat24bppRGB and ImageLockModeRead? As it is, if the
LockBits/UnlockBits is writing but not reading, you won't be able to
tell.

Also, you might want to try flags == 0 in some pixel format other than
24bppRGB. As I understand it, all modes without
ImageLockModeUserInputBuf are treated as read+write regardless of
flags when the bitmap is locked in its native format, as an
optimization. You may simply be running into that optimization.

On Wed, Jul 11, 2012 at 2:49 AM, Dmitry Timoshkov  wrote:
> With additional tests for flags set to ImageLockModeUserInputBuf only.
>
> dlls/gdiplus/tests/image.c | 184 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 184 insertions(+)
>
> diff --git a/dlls/gdiplus/tests/image.c b/dlls/gdiplus/tests/image.c
> index 29182aa..6ce100d 100644
> --- a/dlls/gdiplus/tests/image.c
> +++ b/dlls/gdiplus/tests/image.c
> @@ -3342,6 +3342,189 @@ todo_wine
>      GdipDisposeImage(image);
>  }
>
> +static void test_bitmapbits(void)
> +{
> -
> - static const BYTE pixels_24[24] = {0xff,0xff,0xff, 0,0,0, 0xff,0xff,0xff, 0,0,0,
> -
>                                   0xff,0xff,0xff, 0,0,0, 0xff,0xff,0xff, 0,0,0};
> - static const BYTE pixels_24_00[24] = {0,0,0, 0,0,0, 0,0,0, 0,0,0,
> -
>                                      0,0,0, 0,0,0, 0,0,0, 0,0,0};
> - static const BYTE pixels_24_77[32] = {0xff,0xff,0xff, 0,0,0, 0xff,0xff,0xff, 0,0,0,0x77,0x77,0x77,0x77,
> -
>                                      0xff,0xff,0xff, 0,0,0, 0xff,0xff,0xff, 0,0,0,0x77,0x77,0x77,0x77};
> - static const BYTE pixels_24_88[32] = {0xff,0xff,0xff, 0,0,0, 0xff,0xff,0xff, 0,0,0,0x88,0x88,0x88,0x88,
> -
>                                      0xff,0xff,0xff, 0,0,0, 0xff,0xff,0xff, 0,0,0,0x88,0x88,0x88,0x88};
>
> +#if 0 /* FIXME: these tests crash gdiplus in Wine */
> - static const BYTE pixels_8[8] = {0x01,0,0x01,0,0x01,0,0x01,0};
> - static const BYTE pixels_8_00[8] = {0,0,0,0,0,0,0,0};
> - static const BYTE pixels_8_77[32] = {0x01,0,0x01,0,0x77,0x77,0x77,0x77,
> -
>                                     0x77,0x77,0x77,0x77,0x77,0x77,0x77,0x77,
> -
>                                     0x01,0,0x01,0,0x77,0x77,0x77,0x77,
> -
>                                     0x77,0x77,0x77,0x77,0x77,0x77,0x77,0x77};
> - static const BYTE pixels_8_88[32] = {0x01,0,0x01,0,0x88,0x88,0x88,0x88,
> -
>                                     0x88,0x88,0x88,0x88,0x88,0x88,0x88,0x88,
> -
>                                     0x01,0,0x01,0,0x88,0x88,0x88,0x88,
> -
>                                     0x88,0x88,0x88,0x88,0x88,0x88,0x88,0x88};
> - static const BYTE pixels_a7[32] = {0xa7,0x77,0x77,0x77,0x77,0x77,0x77,0x77,
> -
>                                   0x77,0x77,0x77,0x77,0x77,0x77,0x77,0x77,
> -
>                                   0xa7,0x77,0x77,0x77,0x77,0x77,0x77,0x77,
> -
>                                   0x77,0x77,0x77,0x77,0x77,0x77,0x77,0x77};
> - static const BYTE pixels_1[8] = {0xa0,0,0,0,0xa0,0,0,0};
>
> +#endif
> - static const BYTE pixels_77[32] = {0x77,0x77,0x77,0x77,0x77,0x77,0x77,0x77,
> -
>                                   0x77,0x77,0x77,0x77,0x77,0x77,0x77,0x77,
> -
>                                   0x77,0x77,0x77,0x77,0x77,0x77,0x77,0x77,
> -
>                                   0x77,0x77,0x77,0x77,0x77,0x77,0x77,0x77};
> - static const BYTE pixels_88[32] = {0x88,0x88,0x88,0x88,0x88,0x88,0x88,0x88,
> -
>                                   0x88,0x88,0x88,0x88,0x88,0x88,0x88,0x88,
> -
>                                   0x88,0x88,0x88,0x88,0x88,0x88,0x88,0x88,
> -
>                                   0x88,0x88,0x88,0x88,0x88,0x88,0x88,0x88};
> - static const struct test_data
> - {
> -
>    PixelFormat format;
> -
>    ImageLockMode mode;
> -
>    UINT width, height, stride, size;
> -
>    const BYTE *pixels;
> -
>    const BYTE *pixels_unlocked;
> - } td[] =
> - {
> -
>    { PixelFormat24bppRGB, 0xfff0, 4, 2, 12, 24, pixels_24, pixels_24_00 },
> -
>    { PixelFormat24bppRGB, 0, 4, 2, 12, 24, pixels_24, pixels_24_00 },
> -
> -
>    { PixelFormat24bppRGB, ImageLockModeRead, 4, 2, 12, 24, pixels_24, pixels_24_00 },
> -
>    { PixelFormat24bppRGB, ImageLockModeWrite, 4, 2, 12, 24, pixels_24, pixels_24_00 },
> -
>    { PixelFormat24bppRGB, ImageLockModeRead|ImageLockModeWrite, 4, 2, 12, 24, pixels_24, pixels_24_00 },
> -
>    { PixelFormat24bppRGB, ImageLockModeRead|ImageLockModeUserInputBuf, 4, 2, 16, 32, pixels_24_77, pixels_24_88 },
> -
>    { PixelFormat24bppRGB, ImageLockModeWrite|ImageLockModeUserInputBuf, 4, 2, 16, 32, pixels_77, pixels_88 },
> -
>    { PixelFormat24bppRGB, ImageLockModeUserInputBuf, 4, 2, 16, 32, pixels_77, pixels_88 },
>
> +#if 0 /* FIXME: these tests crash gdiplus in Wine */
> -
>    { PixelFormat8bppIndexed, ImageLockModeRead, 4, 2, 4, 8, pixels_8, pixels_8 },
> -
>    { PixelFormat8bppIndexed, ImageLockModeWrite, 4, 2, 4, 8, pixels_8, pixels_8 },
> -
>    { PixelFormat8bppIndexed, ImageLockModeRead|ImageLockModeWrite, 4, 2, 4, 8, pixels_8, pixels_8_00 },
> -
>    { PixelFormat8bppIndexed, ImageLockModeRead|ImageLockModeUserInputBuf, 4, 2, 16, 32, pixels_8_77, pixels_8_88 },
> -
>    { PixelFormat8bppIndexed, ImageLockModeWrite|ImageLockModeUserInputBuf, 4, 2, 16, 32, pixels_77, pixels_88 },
> -
>    { PixelFormat8bppIndexed, ImageLockModeUserInputBuf, 4, 2, 16, 32, pixels_77, pixels_88 },
> -
> -
>    { PixelFormat1bppIndexed, ImageLockModeRead, 4, 2, 4, 8, pixels_1, pixels_1 },
> -
>    { PixelFormat1bppIndexed, ImageLockModeWrite, 4, 2, 4, 8, pixels_1, pixels_1 },
> -
>    { PixelFormat1bppIndexed, ImageLockModeRead|ImageLockModeWrite, 4, 2, 4, 8, pixels_1, pixels_1 },
> -
>    { PixelFormat1bppIndexed, ImageLockModeRead|ImageLockModeUserInputBuf, 4, 2, 16, 32, pixels_a7, pixels_a7 },
>
> +#endif
> - };
> - BYTE buf[32];
> - GpStatus status;
> - GpBitmap *bitmap;
> - UINT i;
> - BitmapData data;
> - struct
> - {
> -
>    ColorPalette pal;
> -
>    ARGB entries[1];
> - } palette;
> -
> - for (i = 0; i < sizeof(td)/sizeof(td[0]); i++)
> - {
> -
>    BYTE pixels[sizeof(pixels_24)];
> -
>    memcpy(pixels, pixels_24, sizeof(pixels_24));
> -
>    status = GdipCreateBitmapFromScan0(4, 2, 12, PixelFormat24bppRGB, pixels, &bitmap);
> -
>    expect(Ok, status);
> -
> -
>    /* associate known palette with pixel data */
> -
>    palette.pal.Flags = PaletteFlagsGrayScale;
> -
>    palette.pal.Count = 2;
> -
>    palette.pal.Entries[0] = 0;
> -
>    palette.pal.Entries[1] = 0xffffffff;
> -
>    status = GdipSetImagePalette((GpImage *)bitmap, &palette.pal);
> -
>    expect(Ok, status);
> -
> -
>    memset(&data, 0xfe, sizeof(data));
> -
>    if (td[i].mode & ImageLockModeUserInputBuf)
> -
>    {
> -
>        memset(buf, 0x77, sizeof(buf));
> -
>        data.Scan0 = buf;
> -
>        data.Stride = 16;
> -
>    }
> -
>    status = GdipBitmapLockBits(bitmap, NULL, td[i].mode, td[i].format, &data);
> -
>    ok(status == Ok || broken(status == InvalidParameter) /* XP */, "%u: GdipBitmapLockBits error %d\n", i, status);
> -
>    if (status != Ok)
> -
>    {
> -
>        GdipDisposeImage((GpImage *)bitmap);
> -
>        continue;
> -
>    }
> -
>    ok(td[i].width == data.Width, "%u: expected %d, got %d\n", i, td[i].width, data.Width);
> -
>    ok(td[i].height == data.Height, "%u: expected %d, got %d\n", i, td[i].height, data.Height);
> -
>    ok(td[i].stride == data.Stride, "%u: expected %d, got %d\n", i, td[i].stride, data.Stride);
> -
>    ok(td[i].format == data.PixelFormat, "%u: expected %d, got %d\n", i, td[i].format, data.PixelFormat);
> -
>    ok(td[i].size == data.Height * data.Stride, "%u: expected %d, got %d\n", i, td[i].size, data.Height * data.Stride);
> -
>    if (td[i].mode & ImageLockModeUserInputBuf)
> -
>        ok(data.Scan0 == buf, "%u: got wrong buffer\n", i);
> -
>    if (td[i].size == data.Height * data.Stride)
> -
>    {
> -
>        int match = memcmp(data.Scan0, td[i].pixels, td[i].size) == 0;
> -
>        if ((td[i].mode & (ImageLockModeRead|ImageLockModeWrite|ImageLockModeUserInputBuf)) == ImageLockModeWrite && td[i].format != PixelFormat24bppRGB)
> -
>            ok(!match, "%u: data shouldn't match\n", i);
> -
>        else
> -
>        {
> -
>            /* copying to user buffer is very buggy even in win7 */
> -
>            ok(match || broken(!match && (td[i].mode & ImageLockModeUserInputBuf)),
> -
>               "%u: data should match\n", i);
> -
>            if (!match)
> -
>            {
> -
>                UINT j;
> -
>                BYTE *bits = data.Scan0;
> -
>                printf("%u: data mismatch for format %#x:", i, td[i].format);
> -
>                for (j = 0; j < td[i].size; j++)
> -
>                    printf(" %02x", bits[j]);
> -
>                printf("\n");
> -
>            }
> -
>        }
> -
> -
>        memset(data.Scan0, 0, td[i].size);
> -
>    }
> -
> -
>    status = GdipBitmapUnlockBits(bitmap, &data);
> -
>    ok(status == Ok, "%u: GdipBitmapUnlockBits error %d\n", i, status);
> -
> -
>    memset(&data, 0xfe, sizeof(data));
> -
>    if (td[i].mode & ImageLockModeUserInputBuf)
> -
>    {
> -
>        memset(buf, 0x88, sizeof(buf));
> -
>        data.Scan0 = buf;
> -
>        data.Stride = 16;
> -
>    }
> -
>    status = GdipBitmapLockBits(bitmap, NULL, td[i].mode, td[i].format, &data);
> -
>    ok(status == Ok, "%u: GdipBitmapLockBits error %d\n", i, status);
> -
>    ok(td[i].width == data.Width, "%u: expected %d, got %d\n", i, td[i].width, data.Width);
> -
>    ok(td[i].height == data.Height, "%u: expected %d, got %d\n", i, td[i].height, data.Height);
> -
>    ok(td[i].stride == data.Stride, "%u: expected %d, got %d\n", i, td[i].stride, data.Stride);
> -
>    ok(td[i].format == data.PixelFormat, "%u: expected %d, got %d\n", i, td[i].format, data.PixelFormat);
> -
>    ok(td[i].size == data.Height * data.Stride, "%u: expected %d, got %d\n", i, td[i].size, data.Height * data.Stride);
> -
>    if (td[i].mode & ImageLockModeUserInputBuf)
> -
>        ok(data.Scan0 == buf, "%u: got wrong buffer\n", i);
> -
>    /* copying indexed data without associated palette from BitmapData leads
> -
>       to funny effects, that's a design flaw with lock/unlock pixel data */
> -
>    if (td[i].size == data.Height * data.Stride && !(td[i].format & PixelFormatIndexed))
> -
>    {
> -
>        int match = memcmp(data.Scan0, td[i].pixels_unlocked, td[i].size) == 0;
> -
>        ok(match, "%u: data should match\n", i);
> -
>        if (!match)
> -
>        {
> -
>            UINT j;
> -
>            BYTE *bits = data.Scan0;
> -
>            printf("%u: data mismatch for format %#x:", i, td[i].format);
> -
>            for (j = 0; j < td[i].size; j++)
> -
>                printf(" %02x", bits[j]);
> -
>            printf("\n");
> -
>        }
> -
>    }
> -
> -
>    status = GdipBitmapUnlockBits(bitmap, &data);
> -
>    ok(status == Ok, "%u: GdipBitmapUnlockBits error %d\n", i, status);
> -
> -
>    status = GdipDisposeImage((GpImage *)bitmap);
> -
>    expect(Ok, status);
> - }
>
> +}
> -
>
> START_TEST(image)
>  {
>      struct GdiplusStartupInput gdiplusStartupInput;
> @@ -3354,6 +3537,7 @@ START_TEST(image)
>  GdiplusStartup(&gdiplusToken, &gdiplusStartupInput, NULL);
>
> - test_bitmapbits();
> test_tiff_palette();
> test_GdipGetAllPropertyItems();
> test_tiff_properties();
>
> --
> 1.7.11.1

[5/5] gdiplus: Add some tests for GdipBitmapLockBits/GdipBitmapUnlockBits.

[Dmitry Timoshkov]

Vincent Povirk  wrote:
> Shouldn't your second LockBits in this loop always use
> PixelFormat24bppRGB and ImageLockModeRead? As it is, if the
> LockBits/UnlockBits is writing but not reading, you won't be able to
> tell.

Probably that would simplify the test a bit, but I don't see a problem
with testing the same format.
> Also, you might want to try flags == 0 in some pixel format other than
> 24bppRGB. As I understand it, all modes without
> ImageLockModeUserInputBuf are treated as read+write regardless of
> flags when the bitmap is locked in its native format, as an
> optimization. You may simply be running into that optimization.

Probably, but that's a different case IMO.