Re: [PATCH 3/4] winemac.drv: Really fix our handling of DIBs on the clipboard.

[Ken Thomases]

Hi Chip,

Thanks for working on this.

I know you didn't originate this approach, but, as a general concern, is there really no better approach to converting DIBs than what we have with duplicated code for manually laying out the structure and copying data?  I mean, that code has to go someplace, but shouldn't there be proper functions in GDI or something?  Among other things, I worry that there might be subtle details that each duplicate gets slightly wrong or at least different from the others.

For one thing, when converting from a DIBv5 to another format, you copy across the image data, but you drop the color profile.  Shouldn't there be a color conversion to sRGB or whatever the default profile of a v3 DIB is?  I know that most of Wine doesn't handle this stuff properly, anyway, but that's part of my point about doing these conversions in one place.  When such support is added to that one place in the future, we'd want to automatically get the benefit of it here.

Specific points about the code in-line below.  In some cases, I may just be ignorant about the DIB format, so just let me know:

On May 3, 2016, at 2:03 PM, Charles Davis <cdavis5x(a)gmail.com> wrote:
> +static HGLOBAL create_dib_from_dibv5(HGLOBAL src)

…
> +    /* Calculate the size of the new packed DIB */
> +    data_size = bv5->bV5SizeImage;

MSDN says bV5SizeImage can be zero for BI_RGB DIBs.  Is that only for zero-size DIBs or does it mean it's implicit in width, height, and bit count?
> +    packed_size = sizeof(BITMAPINFOHEADER)
> +                  + ((bv5->bV5BitCount <= 8) ? (sizeof(RGBQUAD) * bv5->bV5ClrUsed) : 0)
> +                  + ((bv5->bV5Compression == BI_BITFIELDS) ? (sizeof(DWORD) * 3) : 0)
> +                  + data_size;

MSDN says bV5BitCount can be 0 for BI_JPEG and BI_PNG.  For that case, should a color table be included in the size?

There's an existing function, bitmap_info_size().  Should that be extended for additional header types for its existing uses?  Should it be used for these calculations in your new code?
> +static HGLOBAL create_dibv5_from_dib(HGLOBAL src)
> …
> +        data_size = bmih->biSizeImage;
> +        packed_size = sizeof(BITMAPV5HEADER)
> +                      + ((bmih->biBitCount <= 8) ? (sizeof(RGBQUAD) * bmih->biClrUsed) : 0)
> +                      + data_size;
> +        offset_src = bmih->biSize
> +                     + ((bmih->biBitCount <= 8) ? (sizeof(RGBQUAD) * bmih->biClrUsed) : 0);

Shouldn't offset_src potentially include the color masks for biCompression == BI_BITFIELDS?
> +    if (bv5->bV5Compression == BI_BITFIELDS && bmih->biSize < sizeof(BITMAPV4HEADER))
> +    {
> +        memcpy(&bv5->bV5RedMask, bmih+1, 3*sizeof(DWORD));
> +        bv5->bV5AlphaMask = 0;
> +    }

Is it OK to leave the masks uninitialized for other cases?
> +    hdc = GetDC(0);

You never release this DC.
> +    if (GetLogColorSpaceW(GetColorSpace(hdc), &lcs, sizeof(lcs)))
> +    {
> +        if (bmih->biSize < sizeof(BITMAPV4HEADER))
> +        {
> +            bv5->bV5CSType = lcs.lcsCSType;
> +            bv5->bV5Endpoints = lcs.lcsEndpoints;
> +            bv5->bV5GammaRed = lcs.lcsGammaRed;
> +            bv5->bV5GammaGreen = lcs.lcsGammaGreen;
> +            bv5->bV5GammaBlue = lcs.lcsGammaBlue;
> +        }
> +        bv5->bV5Intent = lcs.lcsIntent;
> +    }
> +    else
> +    {
> +        if (bmih->biSize < sizeof(BITMAPV4HEADER))
> +        {
> +            bv5->bV5CSType = LCS_DEVICE_RGB;

This type isn't listed as a valid value for bV5CSType in MSDN.
> +            memset(&bv5->bV5Endpoints, 0, sizeof(bv5->bV5Endpoints));
> +            bv5->bV5Endpoints.ciexyzRed.ciexyzX = 0xffffffff;
> +            bv5->bV5Endpoints.ciexyzGreen.ciexyzY = 0xffffffff;
> +            bv5->bV5Endpoints.ciexyzBlue.ciexyzZ = 0xffffffff;
> +            bv5->bV5GammaRed = 0;
> +            bv5->bV5GammaGreen = 0;
> +            bv5->bV5GammaBlue = 0;

Are gamma values of 0 right?  I would expect the 16.16-fixed form of 2.2 or maybe 1.0.
> @@ -724,6 +932,7 @@ static HANDLE import_bmp_to_dib(CFDataRef data)
>         }
>
> memcpy(p, bmi, len);
> +

Spurious whitespace change.
> @@ -1254,8 +1552,35 @@ static CFDataRef export_bitmap_to_dib(HANDLE data)
>     dib = create_dib_from_bitmap(data);
>     if (dib)
>     {
> -        ret = export_clipboard_data(dib);
> +        HGLOBAL dib3 = create_dib_from_dibv5(dib);
> +
> +        ret = export_clipboard_data(dib3);
> +        GlobalFree(dib);
> +        if (dib3 != dib) GlobalFree(dib3);
> +    }

Is the above necessary?  create_dib_from_bitmap() will only ever create v3 DIBs, right?  It's specifically allocating and building a BITMAPINFOHEADER.

-Ken