[prev in list] [next in list] [prev in thread] [next in thread] 

List:       wine-devel
Subject:    Re: [PATCH v7 3/3] shell32: Partially implement IShellItemImageFactory (icon only, no thumbnail).
From:       Jinoh Kang <jinoh.kang.kr () gmail ! com>
Date:       2022-04-29 17:38:55
Message-ID: ea040d2c-7821-17e5-1d68-9ddaf5023530 () gmail ! com
[Download RAW message or body]

On 4/29/22 23:44, Nikolay Sivov wrote:
> I think this needs a lot of cleanup.
> 
> On 4/26/22 21:17, Jinoh Kang wrote:
> > 
> > 
> > +
> > typedef struct _ShellItem {
> > IShellItem2             IShellItem2_iface;
> > LONG                    ref;
> > @@ -565,17 +568,205 @@ static ULONG WINAPI \
> > ShellItem_IShellItemImageFactory_Release(IShellItemImageFact return \
> > IShellItem2_Release(&This->IShellItem2_iface); }
> > +static HRESULT ShellItem_get_icons(ShellItem *This, SIZE size, HICON *big_icon, \
> > HICON *small_icon)
> 
> You don't really need two icons, for SIIGBF_BIGGERSIZEOK.

SIIGBF_BIGGERSIZEOK does not appear to signify that the size parameter can be \
entirely disregarded.  Windows still takes the size argument as a hint. Maybe it \
needs more testing, but the impression I got is that it would choose the image that \
will experience the *least* distortion should it be eventually resized to the given \
size. It would then actually perform the resize if SIIGBF_RESIZETOFIT is specified; \
otherwise, the application is supposed to do the resize as needed. Although returning \
an image of arbitrary size isn't *contractually wrong* per se, it may choose a worse \
candidate for shrinking: for example, icons tend to have the same text size in both \
versions; thus, resizing a bigger icon would render the text unreadable. Also, it \
doesn't match Windows behavior anyway.

> > +{
> > +    HRESULT hr;
> > +    IBindCtx *pbc;
> > +    IExtractIconW *ei;
> > +    WCHAR icon_file[MAX_PATH];
> > +    INT source_index;
> > +    UINT gil_in_flags = 0, gil_out_flags;
> > +    INT iconsize;
> > +
> > +    iconsize = min(size.cx, size.cy);
> > +    if (iconsize <= 0 || iconsize > 0x7fff)
> > +        iconsize = 0x7fff;
> > +
> > +    hr = CreateBindCtx(0, &pbc);
> > +    if (FAILED(hr)) goto done;
> > +
> > +    hr = IShellItem2_BindToHandler(&This->IShellItem2_iface, pbc, \
> > &BHID_SFUIObject, +                                   &IID_IExtractIconW, (void \
> > **)&ei); +    IBindCtx_Release(pbc);
> > +    if (FAILED(hr)) goto done;
> > +
> > +    hr = IExtractIconW_GetIconLocation(ei, gil_in_flags, icon_file, MAX_PATH, \
> > &source_index, &gil_out_flags); +    if (FAILED(hr)) goto free_ei;
> 
> You probably can get rid of gil_in_flags and pbc.

For gil_in_flags: ACK. I just wanted to clarify the purpose of the parameter; maybe \
it wasn't a good idea to unnecessarily make a constant-as-of-now a variable after \
all.

For pbc: While the IBindCtx is currently ignored, wouldn't it make sense to pass \
something valid as required by the BindToHandler's interface definition anyway?

> 
> > +
> > +    if (!(gil_out_flags & GIL_NOTFILENAME))
> > +    {
> > +        UINT ei_res;
> > +
> > +        if (source_index == -1)
> > +            source_index = 0;  /* special case for some control panel \
> > applications */ +
> > +        FIXME("%s %d\n", debugstr_w(icon_file), source_index);
> > +        ei_res = ExtractIconExW(icon_file, source_index, big_icon, small_icon, \
> > 1); +        if (!ei_res || ei_res == (UINT)-1)
> > +        {
> > +            WARN("Failed to extract icon.\n");
> > +            hr = E_FAIL;
> > +        }
> 
> Instead of this you can probably do SHGetFileInfo(SHGFI_SYSICONINDEX), and then \
> pick appropriate imagelist.

That would preclude GIL_NOTFILENAME in Wine's current implementation; maybe it \
doesn't matter that much anyway...

> > +    }
> > +    else
> > +    {
> > +        hr = IExtractIconW_Extract(ei, icon_file, source_index, big_icon, \
> > small_icon, MAKELONG(iconsize, iconsize)); +    }
> > +
> > +free_ei:
> > +    IExtractIconW_Release(ei);
> > +done:
> > +    return hr;
> > +}
> > +
> > +static HICON choose_best_icon(HICON *icons, UINT count, SIZE size_limit, SIZE \
> > *out_size) +{
> > +    HICON best_icon = NULL;
> > +    SIZE best_size = {0, 0};
> > +    UINT i;
> > +
> > +    for (i = 0; i < count; i++)
> > +    {
> > +        ICONINFO iinfo;
> > +        BITMAP bm;
> > +        SIZE size;
> > +        BOOL is_color, ret;
> > +
> > +        if (!icons[i] || !GetIconInfo(icons[i], &iinfo)) continue;
> > +
> > +        is_color = iinfo.hbmColor != NULL;
> > +        ret = GetObjectW(is_color ? iinfo.hbmColor : iinfo.hbmMask, sizeof(bm), \
> > &bm); +        DeleteObject(iinfo.hbmColor);
> > +        DeleteObject(iinfo.hbmMask);
> > +        if (!ret) continue;
> > +
> > +        size.cx = bm.bmWidth;
> > +        size.cy = is_color ? abs(bm.bmHeight) : abs(bm.bmHeight) / 2;
> > +
> > +        if (!best_icon || (best_size.cx < size.cx && size.cx <= size_limit.cx &&
> > +                           best_size.cy < size.cy && size.cy <= size_limit.cy))
> > +        {
> > +            best_icon = icons[i];
> > +            best_size = size;
> > +        }
> > +    }
> > +
> > +    *out_size = best_size;
> > +    return best_icon;
> > +}
> 
> This looks too complicated. System imaglists come in few sizes.

Not all icons belong to the system imagelist, are they?

> Desired size is know on GetImage(), I think it make sense to look for exact match \
> first, and load that.

I don't see how that would simplify the algorithm. An exact match may not always \
exist, but only subpar candidates.

> For non-file based case Extract() has size argument on its own, should that do \
> something to pick "the best" size?

My intention was not to rely on Extract() to actually do what we want, even as we \
provide the hint.

> 
> > +
> > +static HRESULT ShellItem_get_icon_bitmap(ShellItem *This, IWICImagingFactory \
> > *imgfactory, +                                         SIZE size, SIIGBF flags, \
> > IWICBitmap **bitmap) +{
> > +    HRESULT hr;
> > +    HICON icons[2] = { NULL, NULL }, best_icon;
> > +    SIZE best_icon_size;
> > +    UINT i;
> > +
> > +    *bitmap = NULL;
> > +
> > +    hr = ShellItem_get_icons(This, size, &icons[0], &icons[1]);
> > +    if (FAILED(hr)) return hr;
> > +
> > +    best_icon = choose_best_icon(icons, ARRAY_SIZE(icons), size, \
> > &best_icon_size); +    for (i = 0; i < ARRAY_SIZE(icons); i++)
> > +        if (icons[i] && icons[i] != best_icon) DeleteObject(icons[i]);
> > +
> > +    if (!best_icon) return E_FAIL;
> > +
> > +    hr = IWICImagingFactory_CreateBitmapFromHICON(imgfactory, best_icon, \
> > bitmap); +    DeleteObject(best_icon);
> > +    return hr;
> > +}
> > +
> > +static HRESULT convert_wicbitmapsource_to_gdi(IWICImagingFactory *imgfactory,
> > +                                              IWICBitmapSource *bitmapsource, \
> > HBITMAP *gdibitmap) +{
> > +    BITMAPINFOHEADER bmi;
> > +    HRESULT hr;
> > +    UINT width, height;
> > +    IWICBitmapSource *newsrc;
> > +    HDC dc;
> > +    HBITMAP bm;
> > +    void *bits;
> > +
> > +    *gdibitmap = NULL;
> > +
> > +    hr = WICConvertBitmapSource(&GUID_WICPixelFormat32bppBGRA, bitmapsource, \
> > &newsrc); +    if (FAILED(hr)) goto done;
> > +
> > +    hr = IWICBitmapSource_GetSize(newsrc, &width, &height);
> > +    if (FAILED(hr)) goto free_newsrc;
> > +
> > +    dc = CreateCompatibleDC(NULL);
> > +    if (!dc)
> > +    {
> > +        hr = E_FAIL;
> > +        goto free_newsrc;
> > +    }
> > +
> > +    memset(&bmi, 0, sizeof(bmi));
> > +    bmi.biSize = sizeof(bmi);
> > +    bmi.biWidth = width;
> > +    bmi.biHeight = -height;
> > +    bmi.biPlanes = 1;
> > +    bmi.biBitCount = 32;
> > +    bmi.biCompression = BI_RGB;
> > +
> > +    bm = CreateDIBSection(dc, (const BITMAPINFO *)&bmi, DIB_RGB_COLORS, &bits, \
> > NULL, 0); +    DeleteDC(dc);
> I don't think you need a device context for this.

ACK.  TIL that CreateDIBSection(NULL, ..., DIB_RGB_COLORS, ...) is legal.

> 
> > +    if (!bm)
> > +    {
> > +        WARN("Cannot create bitmap.\n");
> > +        hr = E_FAIL;
> > +        goto free_newsrc;
> > +    }
> > +
> > +    hr = IWICBitmapSource_CopyPixels(newsrc, NULL, width * 4, width * height * \
> > 4, bits); +    if (FAILED(hr))
> > +    {
> > +        DeleteObject(bm);
> > +        goto free_newsrc;
> > +    }
> > +
> > +    hr = S_OK;
> > +    *gdibitmap = bm;
> > +
> > +free_newsrc:
> > +    IWICBitmapSource_Release(newsrc);
> > +done:
> > +    return hr;
> > +}
> > +
> > static HRESULT WINAPI \
> > ShellItem_IShellItemImageFactory_GetImage(IShellItemImageFactory *iface, SIZE \
> > size, SIIGBF flags, HBITMAP *phbm) {
> > ShellItem *This = impl_from_IShellItemImageFactory(iface);
> > +    HRESULT hr;
> > +    IWICImagingFactory *imgfactory;
> > +    IWICBitmap *bitmap = NULL;
> > static int once;
> > if (!once++)
> > -        FIXME("%p ({%lu, %lu} %d %p): stub\n", This, size.cx, size.cy, flags, \
> > phbm); +        FIXME("%p ({%lu, %lu} %d %p): partial stub\n", This, size.cx, \
> >                 size.cy, flags, phbm);
> > *phbm = NULL;
> > -    return E_NOTIMPL;
> > +
> > +    if (flags != SIIGBF_BIGGERSIZEOK) return E_NOTIMPL;
> > +
> > +    hr = WICCreateImagingFactory_Proxy(WINCODEC_SDK_VERSION, &imgfactory);
> > +    if (SUCCEEDED(hr))
> > +    {
> > +        hr = ShellItem_get_icon_bitmap(This, imgfactory, size, flags, &bitmap);
> > +        if (SUCCEEDED(hr))
> > +        {
> > +            hr = convert_wicbitmapsource_to_gdi(imgfactory, (IWICBitmapSource \
> > *)bitmap, phbm); +            IWICBitmap_Release(bitmap);
> > +        }
> > +        IWICImagingFactory_Release(imgfactory);
> > +    }
> > +
> > +    return hr;
> > }
> 


-- 
Sincerely,
Jinoh Kang


[prev in list] [next in list] [prev in thread] [next in thread]