Opened 2 years ago

Last modified 5 months ago

#14403 portneeded defect (port to stable)

wxMemoryDC::DrawBitmap() with a bitmap with alpha loses the existing memory DC contents

Reported by: GP89 Owned by: vadz
Priority: normal Milestone:
Component: wxMSW Version: stable-latest
Keywords: DrawBitmap alpha Cc:
Blocked By: #15876, #15876, #15876, #15876, #15876, #15876, #15876, #15876, #15876, #15876, #15876, #15876 Blocking:
Patch: yes

Description

Link to question on wxpython user list: [https://groups.google.com/forum/?fromgroups#!topic/wxpython-users/Iqtjj37sxXo
]

I'm experiencing a weird problem on windows where I create a bitmap in a memory dc, but if I try and draw a bitmap onto it with DrawBitmap I lose everything else in the image.

As Robin pointed out in my post the issue can be fixed by setting the banner_bmp image to use a bit depth of 24

I've attached a sample showing the issue and the bitmap in question (although other pngs I've tried all have the same effect)

windows 7 64bit, python 2.7.3 32bit, wx 2.8.12.1 (msw-unicode)

Attachments (11)

windowsbitmap.py download (1.8 KB) - added by GP89 2 years ago.
sample showing the problem
day_on.png download (283 bytes) - added by GP89 2 years ago.
simpler_bitmap_demo.py download (841 bytes) - added by robind 2 years ago.
Snap001.png download (13.0 KB) - added by robind 2 years ago.
test_alpha_ok.png download (584 bytes) - added by awi 8 months ago.
_test.bmp download (146.5 KB) - added by ericj 8 months ago.
_test.png download (30.0 KB) - added by ericj 8 months ago.
alpha-fix.patch download (4.4 KB) - added by awi 8 months ago.
Fixed AlphaBlend results for non-alpha bitmaps with 32-bit DIBs
reset-alpha-after-blit.diff download (6.4 KB) - added by vadz 8 months ago.
Fix avoiding modifying a bitmap selected into a DC
alpha-fix-2.patch download (2.5 KB) - added by awi 7 months ago.
Temporary hack fo reset alpha channel.
alpha-fix-4.patch download (1.5 KB) - added by awi 7 months ago.
Patch to extended wxBitmap::UseAlpha

Download all attachments as: .zip

Change History (51)

Changed 2 years ago by GP89

sample showing the problem

Changed 2 years ago by GP89

comment:1 Changed 2 years ago by vadz

  • Keywords needs-debug added

Does this still happen with 2.9.3?

I'd need to debug this to see what's going on as I have no idea from just looking at the code neither. But for this the code would need to be translated to C++ first...

comment:2 Changed 2 years ago by robind

  • Status changed from new to confirmed

It also happens with current trunk.

I'll attach a new, simpler version of the example that should be easy to translate to C++. The only thing that is Python specific in it is the wx.EmptyBitmap call, which is just another name for wxBitmap(width, height). The key par tof the code is the draw_me_a_bitmap function it makes a new bitmap either with or without drawing another bitmap on the memory DC. Two of these bitmaps are created and put on a panel with wxStaticBitmap widgets. I'll also attach a screen-shot of the result. Notice that the blue background is missing from the one which had the other bitmap drawn on it.

Changed 2 years ago by robind

Changed 2 years ago by robind

comment:3 Changed 2 years ago by vadz

  • Keywords alpha added; needs-debug removed
  • Summary changed from unable to DrawBitmap in a dc without losing the background of the image (win32 only) to wxMemoryDC::DrawBitmap() with a bitmap with alpha loses the existing memory DC contents
  • Version changed from 2.8.12 to 2.9-svn

I can reproduce the bug but I don't know what's going on here :-( My guess is that some kind of conversion happens internally but I have no idea what could it be and why does it lose the existing bitmap contents. Worse, I can't even work around this. I tried:

  1. Setting clipping region: it's ignored by all Blit() functions apparently.
  2. Using Blit() instead of DrawBitmap(): unsurprisingly, doesn't change anything.
  3. Drawing to an intermediate DC and blitting from it: still no change.
  4. Disabling the use of AlphaBlt(): still no. This one is really surprising as the bug doesn't happen if the bitmap has no alpha.

So I'm really stumped.

FWIW here is the patch reproducing the problem:

  • samples/minimal/minimal.cpp

    diff --git a/samples/minimal/minimal.cpp b/samples/minimal/minimal.cpp
    index a78e462..4870e47 100644
    a b MyFrame::MyFrame(const wxString& title) 
    167167    SetMenuBar(menuBar); 
    168168#endif // wxUSE_MENUS 
    169169 
     170    wxBitmap bmp(250, 50); 
     171    { 
     172        wxMemoryDC dc(bmp); 
     173        dc.SetBackground(wxColour(181, 211, 234)); 
     174        dc.Clear(); 
     175    } 
     176    new wxStaticBitmap(this, -1, bmp, wxPoint(5, 5)); 
     177 
     178    { 
     179        wxMemoryDC dc(bmp); 
     180 
     181        wxImage::AddHandler( new wxPNGHandler ); 
     182        wxBitmap bmpDay("day_on.png", wxBITMAP_TYPE_PNG); 
     183 
     184        dc.DrawBitmap(bmpDay, 18, 9); 
     185    } 
     186    new wxStaticBitmap(this, -1, bmp, wxPoint(5, 65)); 
     187 
    170188#if wxUSE_STATUSBAR 
    171189    // create a status bar just for fun (by default with 1 pane only) 
    172190    CreateStatusBar(2); 

Changed 8 months ago by awi

comment:4 Changed 8 months ago by awi

It's interesting that if DC object and wxStaticBitmap constructor are in the same scope, everything works fine even for images with alpha channel (see attachment):

    wxBitmap bmp(250, 50);
    {
        wxMemoryDC dc(bmp);
        dc.SetBackground(wxColour(181, 211, 234));
        dc.Clear();
    }
    new wxStaticBitmap(this, -1, bmp, wxPoint(5, 5));

    wxMemoryDC dc(bmp);
    {
        wxImage::AddHandler( new wxPNGHandler );
        wxBitmap bmpDay("day_on.png", wxBITMAP_TYPE_PNG);

        dc.DrawBitmap(bmpDay, 18, 9);
    }
    new wxStaticBitmap(this, -1, bmp, wxPoint(5, 65));

This could suggest that maybe the issue is related to the lifetime of memory DC.
But on the other hand, for 24-bit images the scope doesn't matter.
This is strange.

comment:5 Changed 8 months ago by ericj

I didn't find a solution, but some information. If - at the last step - you save the generated wxBitmap as BMP (which doesn't support alpha), you'll see that the "missing" pixels are actually all there. But when you save it as PNG, they are transparent.

Somehow the MSW AlphaBlend function changes the structure of the destination bitmap to have an alpha channel. I confirmed that by disabling it in dc.cpp, so that the wxAlphaBlend fallback is used. Then, everything looks ok.

comment:6 follow-ups: Changed 8 months ago by vadz

I must be blind but I don't see any difference between the code in the comment:4 and the code I tested with in comment:3?

As for comment:5, AlphaBlend() modifying the bitmap would actually make sense (in some twisted meaning of sense) but I had apparently tested this and didn't see the same thing... Eric, did you look at the alpha values of the PNG you created by chance? Are they all fully transparent?

comment:7 in reply to: ↑ 6 Changed 8 months ago by ericj

Replying to vadz:

As for comment:5, AlphaBlend() modifying the bitmap would actually make sense (in some twisted meaning of sense) but I had apparently tested this and didn't see the same thing... Eric, did you look at the alpha values of the PNG you created by chance? Are they all fully transparent?

I'm not quite sure what you mean by that. In a way it looks like the two bitmaps have been ANDed together. The destination bitmap only has non-(totally)-transparent pixels where both input bitmaps have.

I'm adding both files as attachment. I used a different background color and bitmap than the original code to make the effect better visible.

Changed 8 months ago by ericj

Changed 8 months ago by ericj

comment:8 Changed 8 months ago by ericj

I just noticed that in Chrome the _test.bmp looks exactly like _test.png. Everywhere else i tested (PhotoShop, Gimp, etc) the background is not transparent.

comment:9 in reply to: ↑ 6 Changed 8 months ago by awi

Replying to vadz:

In your sample wxMemoryDC object is inside the block:

{ 
    wxMemoryDC dc(bmp); 
    wxImage::AddHandler( new wxPNGHandler );
    // ...
}
new wxStaticBitmap(this, -1, bmp, wxPoint(5, 65));

In this case wrong bitmap is displayed (wxMemoryDC and wxStaticBitmap are not in the same scope)

Im my sample wxMemoryDC object is outside the block:

wxMemoryDC dc(bmp);
{
    wxImage::AddHandler( new wxPNGHandler );
// ...
}
new wxStaticBitmap(this, -1, bmp, wxPoint(5, 65));

In this case correct bitmap is displayed (wxMemoryDC and wxStaticBitmap are in the same scope).

Changed 8 months ago by awi

Fixed AlphaBlend results for non-alpha bitmaps with 32-bit DIBs

comment:10 Changed 8 months ago by awi

I found that the problem can occur when wxBitmap has no alpha channel but its DIB has 32-bit depth.
This can happen, eg. in our sample code there is created wxBitmap with no alpha channel but it has 32-bit DIB:

    wxBitmap bmp(250, 50);
 	{
 	    wxMemoryDC dc(bmp);
 	    dc.SetBackground(wxColour(181, 211, 234));
            dc.Clear();
    }
    bool hasAlpha = bmp.HasAlpha(); // false
    BITMAP bm;
    GetObject(bmp.GetHBITMAP(), sizeof(BITMAP), &bm); // bm.bmBitsPixel = 32
    // ...

If on top of such a RGB bitmap with 32-bit DIB (i.e. with fake alpha channel) there is drawn another bitmap with real alpha channel (using AlphaBlend function) then AlphaBlend interprets destination and source bitmaps as both having alpha channels and modifies also alpha channel bits for destination bitmap:
"If the destination bitmap has an alpha channel, then the blend is as follows.
Dest.alpha = Src.Alpha + (1 - SrcAlpha) * Dst.Alpha"
http://msdn.microsoft.com/en-us/library/windows/desktop/dd183393%28v=vs.85%29.aspx

This way our RGB-only bitmap receives RGBA DIB and then it can be displayed in an unexpected way.

The solution could be to set (override) alpha channel bytes to opaque value for RGB (non-alpha) bitmaps with 32-bit DIBs when they are the subject of AlphaBlend function (just after AlphaBlend call).

Patch file attached.

comment:11 follow-up: Changed 8 months ago by vadz

Thanks for your investigations, I had totally failed to realize that AlphaBlend() affects the alpha too, but retrospectively this looks fairly obvious and explains that the previously 0 alpha values in the destination bitmap now become transparent (as long as all of them are 0, they're considered to be opaque but as soon as this is no longer the case, alpha is interpreted normally and 0 means fully transparent).

The question is whether we should do what your patch does and make the destination bitmap fully opaque or, rather, prefill the alpha channel with 0xff (a.k.a. wxALPHA_OPAQUE) before calling AlphaBlt(). I think the latter would make more sense but at least wxGTK doesn't seem to do it and the bitmap we draw on never gets any alpha channel at all. So should we follow wxGTK here and discard the alpha information in the produced bitmap or keep it?

comment:12 in reply to: ↑ 11 Changed 8 months ago by ericj

Replying to vadz:

So should we follow wxGTK here and discard the alpha information in the produced bitmap or keep it?

If i have a 24bit bitmap and draw another bitmap onto it, i would no expect it to have 32bit with alpha afterwards. I think this should not happen.

But i'm a little concerned about the performance penalty this will cause and especially that it will be totally hidden from the user. (Of course it's better than a buggy result, but still).

comment:13 Changed 8 months ago by vadz

Another thing: the documentation of SetDIBits() explicitly says that the HBITMAP it works on must not be selected into another device context, but this is always the case here (it is selected into wxMemoryDC we call DrawBitmap() on). So I don't think it's a good idea to call it here. OTOH recreating the bitmap as the current code in wxBitmap::UngetRawData() does (it has changed since the version you used for your patch and the "TODO" there has been fixed) doesn't work neither as we can't be deleting a bitmap while it's selected neither.

So we need to add a way to temporarily unselect a bitmap selected into memory DC, modify it and select it back...

Changed 8 months ago by vadz

Fix avoiding modifying a bitmap selected into a DC

comment:14 follow-up: Changed 8 months ago by vadz

  • Patch set

Here is my version of the original patch avoiding modifying the bitmap while it's selected and also doing it directly in AlphaBlt() so that it should also fix the same bug in StretchBlit().

Any comments/testing results?

comment:15 in reply to: ↑ 14 Changed 8 months ago by awi

Replying to vadz:

Here is my version of the original patch avoiding modifying the bitmap while it's selected and also doing it directly in AlphaBlt() so that it should also fix the same bug in StretchBlit().

Any comments/testing results?

For me it works like a charm.
And your fix solves also StretchBlit() case, what I missed.
Definitely, the fix looks better now.
Please take special care of wxBitmap::UngetRawData() which is important for all this manual manipulations of bitmaps.

comment:16 Changed 7 months ago by VZ

  • Resolution set to fixed
  • Status changed from confirmed to closed

(In [75566]) Fix alpha after drawing a bitmap with alpha on a purely RGB bitmap in wxMSW.

Previously, the area of the bitmap outside of the rectangle covered by the
bitmap being drawn became completely transparent, "losing" the original bitmap
contents. This was due to some pixels of the bitmap having non-0 alpha value
after AlphaBlend() call, as it updates both the colours and alpha of the
destination. As there doesn't seem to be any way to prevent it from doing
this, just reset alpha back to 0 after calling it to avoid the problem.

Closes #14403.

comment:17 follow-up: Changed 7 months ago by vadz

  • Resolution changed from fixed to port to stable
  • Status changed from closed to portneeded

This should be backported to 3.0 if no new problems are found.

comment:18 in reply to: ↑ 17 Changed 7 months ago by johnr

r75566 leaves the wxRibbon with a totally white background without panel or tab borders leaving only some but not all button icons visible. There may be other controls affected.

comment:19 follow-up: Changed 7 months ago by vadz

  • Owner set to vadz
  • Status changed from portneeded to accepted

Argh, I knew it couldn't be that simple :-( Thanks for testing, John.

comment:20 Changed 7 months ago by awi

It seems that to reproduce the issue only below part of the fix 75566 (in dc.cpp) is enough:

    wxBitmap bmpOld = bmpDst;
    dcDst->DoSelect(wxNullBitmap);

    {
         wxAlphaPixelData data(bmpOld);
/*
         // inner loops don't count
         // ...
*/
    }
    dcDst->DoSelect(bmpOld);

In above case there are visible these strange effects in the ribbon sample.

If one more line would be commented then everything looks fine:

    wxBitmap bmpOld = bmpDst;
    dcDst->DoSelect(wxNullBitmap);

    {
/*
         wxAlphaPixelData data(bmpOld);
         // inner loops don't count
         // ...
*/
    }
    dcDst->DoSelect(bmpOld);

It would suggest that there is a problem with the pair: wxBitmap::GetRawData and wxBitmap::UngetRawData.

Changed 7 months ago by awi

Temporary hack fo reset alpha channel.

comment:21 Changed 7 months ago by awi

It seems that using pure GDI functions solves the issue.
This is not the best (fastest) solution but unless wxBitmap::GetRawData and wxBitmap::UngetRawData pair is fixed it could be a temporary hack.

Patch attached.

comment:22 in reply to: ↑ 19 Changed 7 months ago by awi

Replying to vadz:

Argh, I knew it couldn't be that simple :-( Thanks for testing, John.

The problem lies in wxAlphaPixelData destructor where there is a code which marks 32-bit bitmap as a bitmap with active alpha channel. This way every 32-bit bitmap being the subject of wxAlphaPixelData operations is flagged as a full RGBA bitmap even if alpha channel is not in use.
If this flagging would be removed the everything works fine.
Patch below:

--- wxWidgets-trunk\include/wx/rawbmp.h
+++ wxWidgets-14403\include/wx/rawbmp.h
@@ -656,7 +656,7 @@
         {
             if ( m_pixels.IsOk() )
             {
-#if defined(__WXMSW__) || defined(__WXMAC__)
+#if defined(__WXMAC__)
                 // this is a hack to mark wxBitmap as using alpha channel
                 if ( Format::HasAlpha )
                     m_bmp.UseAlpha();

comment:23 Changed 7 months ago by vadz

We need to keep this here, otherwise a bitmap wouldn't be marked as having alpha after manipulating it directly, just look at the raw bitmaps window in the image sample after that change -- it doesn't work correctly any more. However we clearly need to prevent the bitmap from being given alpha in this particular case. I'll do something about it, thanks for finding the reason for the problem.

Changed 7 months ago by awi

Patch to extended wxBitmap::UseAlpha

comment:24 Changed 7 months ago by awi

So maybe it would be OK to manually set m_hasAlpha property in wxBitmap to false after resetting alpha channel in AlphaBlt function?
But as far as I can see there is no public function to do so. One possible solution could be to use existing wxBitmap::UseAlpha function for this purpose but it should get an additional parameter (use flag) with default value (in order to preserve current calls without argument).

Patch attached.

comment:25 Changed 7 months ago by VZ

(In [75588]) wxBitmap with cleared alpha channel doesn't keep its alpha flag any more.

Explicitly reset wxBitmap alpha flag after clearing its alpha channel to
ensure that we don't treat it as having alpha after going to all the trouble
of ensuring that it doesn't/

See #14403.

comment:26 Changed 7 months ago by vadz

  • Status changed from accepted to portneeded

Note: when backporting to 3.0, only ResetAlpha() should be backported (and implemented), we should avoid changing UseAlpha() there in non-ABI-compatible way.

comment:27 Changed 7 months ago by vadz

  • Blocked By 15876 added

comment:28 Changed 7 months ago by awi

  • Blocked By

(In #15876) These strange effects occur if there is a coincidence of three conditions:

  1. DDB bitmap without alpha channel selected into DC.
  2. Another bitmap with alpha channel is drawn using DrawBitmap function.
  3. In the code is used a GDI (physical) handle of the bitmap selected into DC (destination bitmap).

In this case destination bitmap (DDB) is converted to DIB before its blended with source bitmap (in AlphaBlt function, see #14403) and after that it is converted back to DDB. During this conversion GDI handle of the destination bitmap is changed (in fact the bitmap is recreated from temporary DIB).
If calling application has stored original handle of the bitmap for future use then after DrawBitmap call it is stored an invalid value.

E.g. wxToolBar::Realize() function is affected by this issue.

Application cannot assume that state of the destination bitmap (including its physical handle) will be preserved in DrawBitmap call.

Patch to fix wxToolbar attached.
Webview and Html/About samples should now work.

comment:29 Changed 7 months ago by awi

  • Blocked By

(In #15876) And one more note.
It's quite possible that tons of wx code rely on the assumption that bitmap handle is invariant of DrawBitamp calls. Generally, I think this is not the best practice but it can be also useful.
Technically it would be possible to preserve bitmap handle in internal transformations but in order to do so wxBitmap::UngetRawData function should be rewritten.
Now, old DDB is deleted and new one is created (if necessary) from temporary DIB. Instead, the existing DDB would be only updated.

comment:30 Changed 7 months ago by troelsk

  • Blocked By

(In #15876) toolbar-draw.patch works for me

comment:31 Changed 7 months ago by johnr

  • Blocked By

(In #15876) Replying to troelsk:

toolbar-draw.patch works for me

Fixes things for me also for active icons. Toolbar icons don't look right using bmpDisabled=wxNullBitmap generated bitmaps when disabled. Nothing as good as OSX.

comment:32 Changed 7 months ago by awi

  • Blocked By

(In #15876) Replying to johnr:

Replying to troelsk:

toolbar-draw.patch works for me

Fixes things for me also for active icons. Toolbar icons don't look right using bmpDisabled=wxNullBitmap generated bitmaps when disabled. Nothing as good as OSX.

Disabled bitmaps can look better if bitmap with transparency would be enabled in the toolbar.
Patch attached.

comment:33 Changed 7 months ago by johnr

  • Blocked By

(In #15876) Replying to awi:

Disabled bitmaps can look better if bitmap with transparency would be enabled in the toolbar.
Patch attached.

Vast improvement, thanks.

comment:34 Changed 7 months ago by VZ

  • Blocked By

(In #15876) (In [75649]) Fix appearance of tools with alpha bitmaps in wxMSW wxToolBar.

Recent changes broke the handling of tools with alpha bitmaps as drawing them
on the global toolbar bitmap changes its underlying HBITMAP now, but the code
in wxToolBar didn't expect this.

Fix it by updating the HBITMAP used after every DrawBitmap() call, just in
case it changed (it doesn't cost anything to reset it if it did not).

Closes #15876.

comment:35 Changed 7 months ago by VZ

  • Blocked By

(In #15876) (In [75650]) Improve appearance of tools using bitmaps with alpha in wxMSW wxToolBar.

Use alpha in the combined toolbar bitmap if any of its tools has a bitmap
using alpha. This greatly improves the appearance of the automatically
generated disabled images for the tools with bitmaps using alpha.

See #15876.

comment:36 Changed 7 months ago by vadz

  • Blocked By

comment:37 Changed 7 months ago by VZ

  • Blocked By

(In #15876) (In [75769]) Fix appearance of tools with alpha bitmaps in wxMSW wxToolBar.

Recent changes broke the handling of tools with alpha bitmaps as drawing them
on the global toolbar bitmap changes its underlying HBITMAP now, but the code
in wxToolBar didn't expect this.

Fix it by updating the HBITMAP used after every DrawBitmap() call, just in
case it changed (it doesn't cost anything to reset it if it did not).

Closes #15876.

comment:38 Changed 7 months ago by VZ

  • Blocked By

(In #15876) (In [75770]) Improve appearance of tools using bitmaps with alpha in wxMSW wxToolBar.

Use alpha in the combined toolbar bitmap if any of its tools has a bitmap
using alpha. This greatly improves the appearance of the automatically
generated disabled images for the tools with bitmaps using alpha.

See #15876.

comment:39 follow-up: Changed 5 months ago by Aardappel

Would like to note that the hack added to AlphaBlt unexpectedly made my app (treesheets) slow down to a crawl, so I had to comment it out. Not noticing any adverse effects yet.

comment:40 in reply to: ↑ 39 Changed 5 months ago by awi

Replying to Aardappel:

Would like to note that the hack added to AlphaBlt unexpectedly made my app (treesheets) slow down to a crawl, so I had to comment it out. Not noticing any adverse effects yet.

Manual iterating over all pixels of the target bitmap in order to fix it is an expensive operation but there is not so much room for optimization here, in the GDI world.
I did a quick tests of DrawBitmap() performance for drawing 32-bit source bitmap with real transparency over 200x200 target bitmap and the results looks as follows:

Target       Drawing
bitmap       time [ms]   Remarks
24-bit DIB   0.037       Only AlpahBlend() Win API 
32-bit DIB   0.069       Plus iterating over the target bitmap
32-Bit DDB   0.362       Plus DDB -> DIB conversion

It can be seen that what costs most is conversion form DDB to DIB.

So, if you work with source bitmaps with transparency and at the same time transparency of target bitmaps is not important for you then you should avoid using DDB bitmaps in this case by declaring target bitmaps explicitly as 24-bit DIB's:

wxBitmap bmp(w, h, 24);
wxMemoryDC(bmp);
//...
Note: See TracTickets for help on using tickets.