From 44f26d6f82085f55f3e4b2cb4f3d50c4dea098de Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Tue, 7 Jan 2014 21:13:20 +0000 Subject: [PATCH] Fix handling of bitmaps with alpha channel in wxMSW wxImageList. Don't use mask and alpha together, this results in visual artefacts and masks are unnecessary with RGBA bitmaps anyhow. The only potentially problematic remaining case is mixing bitmaps with alpha and mask inside the same image list (as we need to indicate whether we use the mask or not when creating it), but this should probably be rare and in the meanwhile we can at least RGBA bitmaps with image lists, which includes doing this implicitly when they are used as button bitmaps. Also refactor wxBitmap code to extract part of CopyFromIconOrCursor() to allow reusing it in the newly added MSWUpdateAlpha(). See #11476. git-svn-id: https://svn.wxwidgets.org/svn/wx/wxWidgets/trunk@75567 c3d73ce0-8a6f-49c7-b76d-6d57e0e08775 --- include/wx/msw/bitmap.h | 4 ++ src/msw/anybutton.cpp | 3 +- src/msw/bitmap.cpp | 106 +++++++++++++++++++++++++--------------- src/msw/imaglist.cpp | 66 +++++++++++++++++-------- 4 files changed, 117 insertions(+), 62 deletions(-) diff --git a/include/wx/msw/bitmap.h b/include/wx/msw/bitmap.h index ef07317b1d..4668eaf93f 100644 --- a/include/wx/msw/bitmap.h +++ b/include/wx/msw/bitmap.h @@ -180,6 +180,10 @@ public: // implementation only from now on // ------------------------------- + // Set alpha flag to true if this is a 32bpp bitmap which has any non-0 + // values in its alpha channel. + void MSWUpdateAlpha(); + public: void SetHBITMAP(WXHBITMAP bmp) { SetHandle((WXHANDLE)bmp); } WXHBITMAP GetHBITMAP() const { return (WXHBITMAP)GetHandle(); } diff --git a/src/msw/anybutton.cpp b/src/msw/anybutton.cpp index b99e2f66e4..a930d3c11a 100644 --- a/src/msw/anybutton.cpp +++ b/src/msw/anybutton.cpp @@ -215,7 +215,8 @@ public: // we must be constructed with the size of our images as we need to create // the image list wxXPButtonImageData(wxAnyButton *btn, const wxBitmap& bitmap) - : m_iml(bitmap.GetWidth(), bitmap.GetHeight(), true /* use mask */, + : m_iml(bitmap.GetWidth(), bitmap.GetHeight(), + !bitmap.HasAlpha() /* use mask only if no alpha */, wxAnyButton::State_Max + 1 /* see "pulse" comment below */), m_hwndBtn(GetHwndOf(btn)) { diff --git a/src/msw/bitmap.cpp b/src/msw/bitmap.cpp index 89ee4104ac..cf30c16840 100644 --- a/src/msw/bitmap.cpp +++ b/src/msw/bitmap.cpp @@ -292,6 +292,60 @@ wxGDIRefData *wxBitmap::CloneGDIRefData(const wxGDIRefData *data) const return new wxBitmapRefData(*static_cast(data)); } +// Premultiply the values of all RGBA pixels in the given range. +static void PremultiplyPixels(unsigned char* begin, unsigned char* end) +{ + for ( unsigned char* pixels = begin; pixels < end; pixels += 4 ) + { + const unsigned char a = pixels[3]; + + pixels[0] = ((pixels[0]*a) + 127)/255; + pixels[1] = ((pixels[1]*a) + 127)/255; + pixels[2] = ((pixels[2]*a) + 127)/255; + } +} + +// Helper which examines the alpha channel for any non-0 values and also +// possibly returns the DIB with premultiplied values if it does have alpha +// (i.e. this DIB is only filled if the function returns true). +// +// The function semantics is complicated but necessary to avoid converting to +// DIB twice, which is expensive for large bitmaps, yet avoid code duplication +// between CopyFromIconOrCursor() and MSWUpdateAlpha(). +static bool CheckAlpha(HBITMAP hbmp, HBITMAP* hdib = NULL) +{ + BITMAP bm; + if ( !::GetObject(hbmp, sizeof(bm), &bm) || (bm.bmBitsPixel != 32) ) + return false; + + wxDIB dib(hbmp); + if ( !dib.IsOk() ) + return false; + + unsigned char* pixels = dib.GetData(); + unsigned char* const end = pixels + 4*dib.GetWidth()*dib.GetHeight(); + for ( ; pixels < end; pixels += 4 ) + { + if ( pixels[3] != 0 ) + { + if ( hdib ) + { + // If we do have alpha, ensure we use premultiplied data for + // our pixels as this is what the bitmaps created in other ways + // do and this is necessary for e.g. AlphaBlend() to work with + // this bitmap. + PremultiplyPixels(dib.GetData(), end); + + *hdib = dib.Detach(); + } + + return true; + } + } + + return false; +} + bool wxBitmap::CopyFromIconOrCursor(const wxGDIImage& icon, wxBitmapTransparency transp) { @@ -332,48 +386,14 @@ bool wxBitmap::CopyFromIconOrCursor(const wxGDIImage& icon, #if wxUSE_WXDIB // If the icon is 32 bits per pixel then it may have alpha channel // data, although there are some icons that are 32 bpp but have no - // alpha... So convert to a DIB and manually check the 4th byte for - // each pixel. + // alpha, so check for this. { - BITMAP bm; - if ( ::GetObject(iconInfo.hbmColor, sizeof(bm), &bm) && - (bm.bmBitsPixel == 32) ) + HBITMAP hdib = 0; + if ( CheckAlpha(iconInfo.hbmColor, &hdib) ) { - wxDIB dib(iconInfo.hbmColor); - if (dib.IsOk()) - { - unsigned char* const pixels = dib.GetData(); - int idx; - for ( idx = 0; idx < w*h*4; idx += 4 ) - { - if (pixels[idx+3] != 0) - { - // If there is an alpha byte that is non-zero - // then set the alpha flag and stop checking - refData->m_hasAlpha = true; - break; - } - } - - if ( refData->m_hasAlpha ) - { - // If we do have alpha, ensure we use premultiplied - // data for our pixels as this is what the bitmaps - // created in other ways do and this is necessary - // for e.g. AlphaBlend() to work with this bitmap. - for ( idx = 0; idx < w*h*4; idx += 4 ) - { - const unsigned char a = pixels[idx+3]; - - pixels[idx] = ((pixels[idx] *a) + 127)/255; - pixels[idx+1] = ((pixels[idx+1]*a) + 127)/255; - pixels[idx+2] = ((pixels[idx+2]*a) + 127)/255; - } - - ::DeleteObject(refData->m_hBitmap); - refData->m_hBitmap = dib.Detach(); - } - } + refData->m_hasAlpha = true; + ::DeleteObject(refData->m_hBitmap); + refData->m_hBitmap = hdib; } } break; @@ -1194,6 +1214,12 @@ bool wxBitmap::HasAlpha() const return GetBitmapData() && GetBitmapData()->m_hasAlpha; } +void wxBitmap::MSWUpdateAlpha() +{ + if ( CheckAlpha(GetHbitmap()) ) + GetBitmapData()->m_hasAlpha = true; +} + // ---------------------------------------------------------------------------- // wxBitmap setters // ---------------------------------------------------------------------------- diff --git a/src/msw/imaglist.cpp b/src/msw/imaglist.cpp index 928a9f6b74..e3a4a29099 100644 --- a/src/msw/imaglist.cpp +++ b/src/msw/imaglist.cpp @@ -158,7 +158,11 @@ int wxImageList::Add(const wxBitmap& bitmap, const wxBitmap& mask) #endif // wxUSE_WXDIB && wxUSE_IMAGE hbmp = GetHbitmapOf(bitmap); - AutoHBITMAP hbmpMask(GetMaskForImage(bitmap, mask)); + // Use mask only if we don't have alpha, the bitmap isn't drawn correctly + // if we use both. + AutoHBITMAP hbmpMask; + if ( !bitmap.HasAlpha() ) + hbmpMask.Init(GetMaskForImage(bitmap, mask)); int index = ImageList_Add(GetHImageList(), hbmp, hbmpMask); if ( index == -1 ) @@ -328,7 +332,6 @@ bool wxImageList::Draw(int index, // Get the bitmap wxBitmap wxImageList::GetBitmap(int index) const { -#if wxUSE_WXDIB && wxUSE_IMAGE int bmp_width = 0, bmp_height = 0; GetSize(index, bmp_width, bmp_height); @@ -336,29 +339,50 @@ wxBitmap wxImageList::GetBitmap(int index) const wxMemoryDC dc; dc.SelectObject(bitmap); - // draw it the first time to find a suitable mask colour - ((wxImageList*)this)->Draw(index, dc, 0, 0, wxIMAGELIST_DRAW_TRANSPARENT); - dc.SelectObject(wxNullBitmap); +#if wxUSE_WXDIB && wxUSE_IMAGE + IMAGEINFO ii; + ImageList_GetImageInfo(GetHImageList(), index, &ii); + if ( ii.hbmMask ) + { + // draw it the first time to find a suitable mask colour + ((wxImageList*)this)->Draw(index, dc, 0, 0, wxIMAGELIST_DRAW_TRANSPARENT); + dc.SelectObject(wxNullBitmap); - // find the suitable mask colour - wxImage image = bitmap.ConvertToImage(); - unsigned char r = 0, g = 0, b = 0; - image.FindFirstUnusedColour(&r, &g, &b); + // find the suitable mask colour + wxImage image = bitmap.ConvertToImage(); + unsigned char r = 0, g = 0, b = 0; + image.FindFirstUnusedColour(&r, &g, &b); - // redraw whole image and bitmap in the mask colour - image.Create(bmp_width, bmp_height); - image.Replace(0, 0, 0, r, g, b); - bitmap = wxBitmap(image); + // redraw whole image and bitmap in the mask colour + image.Create(bmp_width, bmp_height); + image.Replace(0, 0, 0, r, g, b); + bitmap = wxBitmap(image); - // redraw icon over the mask colour to actually draw it - dc.SelectObject(bitmap); - ((wxImageList*)this)->Draw(index, dc, 0, 0, wxIMAGELIST_DRAW_TRANSPARENT); - dc.SelectObject(wxNullBitmap); + // redraw icon over the mask colour to actually draw it + dc.SelectObject(bitmap); + ((wxImageList*)this)->Draw(index, dc, 0, 0, wxIMAGELIST_DRAW_TRANSPARENT); + dc.SelectObject(wxNullBitmap); - // get the image, set the mask colour and convert back to get transparent bitmap - image = bitmap.ConvertToImage(); - image.SetMaskColour(r, g, b); - bitmap = wxBitmap(image); + // get the image, set the mask colour and convert back to get transparent bitmap + image = bitmap.ConvertToImage(); + image.SetMaskColour(r, g, b); + bitmap = wxBitmap(image); + } + else // no mask + { + // Just draw it normally. + ((wxImageList*)this)->Draw(index, dc, 0, 0, wxIMAGELIST_DRAW_NORMAL); + dc.SelectObject(wxNullBitmap); + + // And adjust its alpha flag as the destination bitmap would get it if + // the source one had it. + // + // Note that perhaps we could just call UseAlpha() which would set the + // "has alpha" flag unconditionally as it doesn't seem to do any harm, + // but for now only do it if necessary, just to be on the safe side, + // even if it requires more work (and takes more time). + bitmap.MSWUpdateAlpha(); + } #else wxBitmap bitmap; #endif