Opened 7 years ago

Closed 7 years ago

#16512 closed defect (fixed)

wxBitmap reported as invalid if created from monochrome icon/cursor

Reported by: awi Owned by: VZ
Priority: normal Milestone: 3.1.0
Component: wxMSW Version: dev-latest
Keywords: wxBitmap wxIcon wxCursor monochrome conversion Cc:
Blocked By: Blocking:
Patch: no

Description

When wxBitmap is created from monochrome icon or cursor then at first operation the assertion 'bmp.IsOk' fails.
This happens due to the fact that color bitmap handle of the icon/cursor is copied to wxBitmap (and used as its main bitmap handle) but monochrome icon/cursors not necessarily have this color bitmap defined (only bitmmask bitmap). If wxBitmap handle is null that wxBitmap is claimed as invalid (in IsOk() function).
But even if assertion warning is ignored in the application, such a wxBitmap is not drawn.

To fix the issue there is necessary to create in wxBitmap::CopyFromIconOrCursor() private internal bitmap and assign it to wxBitmap if this bitmap is not present in the source icon/cursor.
Patch is attached: attachment:Fix-creating-wxBitmap-from-monochrome-icon-cursor.patch

Patch to minimal sample reproducing the issue (under Win7):

  • samples/minimal/minimal.cpp

    a b public: 
    6767    // event handlers (these functions should _not_ be virtual)
    6868    void OnQuit(wxCommandEvent& event);
    6969    void OnAbout(wxCommandEvent& event);
     70    void OnPaint(wxPaintEvent &event);
    7071
    7172private:
    7273    // any class wishing to process wxWidgets events must use this macro
    enum 
    99100wxBEGIN_EVENT_TABLE(MyFrame, wxFrame)
    100101    EVT_MENU(Minimal_Quit,  MyFrame::OnQuit)
    101102    EVT_MENU(Minimal_About, MyFrame::OnAbout)
     103    EVT_PAINT  (MyFrame::OnPaint)
    102104wxEND_EVENT_TABLE()
    103105
    104106// Create a new application object: this macro will allow wxWidgets to create
    MyFrame::MyFrame(const wxString& title) 
    172174    CreateStatusBar(2);
    173175    SetStatusText("Welcome to wxWidgets!");
    174176#endif // wxUSE_STATUSBAR
     177    wxCursor cursor1(wxCURSOR_MAGNIFIER);
     178    wxBitmap bmp;
     179    bmp = cursor1;
     180    new wxStaticBitmap(this, wxID_ANY, bmp, wxPoint(10, 10));
     181
     182    wxCursor cursor2(wxCURSOR_ARROW);
     183    bmp = cursor2;
     184    new wxStaticBitmap(this, wxID_ANY, bmp, wxPoint(60, 10));
     185
     186    wxCursor cursor3(wxCURSOR_BULLSEYE);
     187    bmp = cursor3;
     188    new wxStaticBitmap(this, wxID_ANY, bmp, wxPoint(110, 10));
    175189}
    176190
    177191
    void MyFrame::OnAbout(wxCommandEvent& WXUNUSED(event)) 
    198212                 wxOK | wxICON_INFORMATION,
    199213                 this);
    200214}
     215
     216void MyFrame::OnPaint(wxPaintEvent &WXUNUSED(event))
     217{
     218//    wxBufferedPaintDC dc(this);
     219    wxPaintDC dc(this);
     220
     221    wxCursor cursor1(wxCURSOR_MAGNIFIER);
     222    wxBitmap bmp;
     223    bmp = cursor1;
     224    dc.DrawBitmap(bmp, wxPoint(10, 100), bmp.GetMask() != 0);
     225
     226    wxCursor cursor2(wxCURSOR_ARROW);
     227    bmp = cursor2;
     228    dc.DrawBitmap(bmp, wxPoint(60, 100), bmp.GetMask() != 0);
     229
     230    wxCursor cursor3(wxCURSOR_BULLSEYE);
     231    bmp = cursor3;
     232    dc.DrawBitmap(bmp, wxPoint(110, 100), bmp.GetMask() != 0);
     233}

Attachments (14)

Fix-creating-wxBitmap-from-monochrome-icon-cursor.patch download (920 bytes) - added by awi 7 years ago.
Fix creating wxBitmap from monochrome icon or cursor.
Modified-minimal-sample-v2.patch download (3.6 KB) - added by awi 7 years ago.
Test case v2.
Extract-XOR-mask-from-monochrome-wxIcon-wxCursor.patch download (2.0 KB) - added by awi 7 years ago.
Extract XOR mask from monochrome wxIcon/wxCursor and use it as main bitmap.
Do-not-black-out-the-transparent-area-when-converting-bitmap-to-icon.patch download (915 bytes) - added by awi 7 years ago.
Don't black out the transparent area when converting wxBitmap to icon.
Fix-DrawBitmap-for-bitmaps-with-masks.patch download (922 bytes) - added by awi 7 years ago.
Mimic system operations in DrawBitmap when drawing bitmaps with masks.
draw-cursor-0.png download (15.5 KB) - added by awi 7 years ago.
Initial results.
draw-cursor-1.png download (16.1 KB) - added by awi 7 years ago.
Result after applying 1-st patch.
draw-cursor-2.png download (16.4 KB) - added by awi 7 years ago.
Results after applying 2-nd patch.
draw-cursor-fixed.png download (16.2 KB) - added by awi 7 years ago.
Final results.
Extract-XOR-mask-from-monochrome-wxIcon-wxCursor_v2.patch download (1.7 KB) - added by awi 7 years ago.
Extract XOR mask from monochrome wxIcon/wxCursor and use it as a main bitmap v2.
Do-not-black-out-the-transparent-area-when-converting_v2.patch download (915 bytes) - added by awi 7 years ago.
Don't black out the transparent area when converting wxBitmap to icon (unchanged).
Fix-DrawBitmap-for-bitmaps-with-masks_v2.patch download (943 bytes) - added by awi 7 years ago.
Mimic system operations in DrawBitmap when drawing bitmaps with masks v2.
draw-monochrome-cursor-basic.png download (16.7 KB) - added by awi 7 years ago.
Simplify drawing of monochrome cursors.
Convert-AND-bitmap-to-wxBitmap-mask.patch download (1.7 KB) - added by awi 7 years ago.
Convert of native monochrome bitmap mask to wxBitmap mask.

Download all attachments as: .zip

Change History (28)

Changed 7 years ago by awi

Fix creating wxBitmap from monochrome icon or cursor.

comment:1 Changed 7 years ago by VZ

  • Owner set to VZ
  • Resolution set to fixed
  • Status changed from new to closed

In 77516:

Fix creation of wxBitmap from monochrome wxIcon or wxCursor in wxMSW.

Don't suppose that we always have hbmColor because this is not true for
monochrome icons/cursors. Create our own bitmap in this case.

Closes #16512.

comment:2 Changed 7 years ago by VZ

In 77517:

Fix creation of wxBitmap from monochrome wxIcon or wxCursor in wxMSW.

Don't suppose that we always have hbmColor because this is not true for
monochrome icons/cursors. Create our own bitmap in this case.

Closes #16512.

comment:3 Changed 7 years ago by awi

  • Resolution fixed deleted
  • Status changed from closed to reopened

Applied patch solves some issues but unfortunately doesn't help in all cases and doesn't give the same results as native drawing operations. You can see these problems on attached screenshot:
Initial results.
Test case: attachment:Modified-minimal-sample-v2.patch

The most problematic monochrome cursors are those for which AND mask is not used to clear destination for bitmap with shape (to eventually overwrite destination by this bitmap). For some monochrome cursors/icons (even standard ones, like wxCURSOR_IBEAM, wxCURSOR_CROSS) AND mask is totally filled with 1 (ones) and bitmap is directly XOR'ed with destination and the final result is inversion of the destination bitmap based on pattern stored in XOR mask.

To fix handling all monochrome icons/cursors it is necessary to fix several sub-problems:

  1. Mask bitmap for monochrome icons/cursors contains both AND and XOR maps. We need to retrieve XOR mask and use it as a main bitmap. Main bitmap cannot be simply a dummy empty bitmap.

attachment:Extract-XOR-mask-from-monochrome-wxIcon-wxCursor.patch
Results after this step look as follows:
Result after applying 1-st patch.

  1. We don't need to manually black out icon's transparent area with AND mask when wxBitmap is converted to wxIcon. Let the system do that.

attachment:Do-not-black-out-the-transparent-area-when-converting-bitmap-to-icon.patch
Results after this step look as follows:
Results after applying 2-nd patch.

  1. We should mimic system behaviour when drawing bitmaps with masks. At first destination should be AND'ed with mask and next it should be XOR'ed with our bitmap.

attachment:Fix-DrawBitmap-for-bitmaps-with-masks.patch
And the final results:
Final results.

Changed 7 years ago by awi

Test case v2.

Changed 7 years ago by awi

Extract XOR mask from monochrome wxIcon/wxCursor and use it as main bitmap.

Changed 7 years ago by awi

Don't black out the transparent area when converting wxBitmap to icon.

Changed 7 years ago by awi

Mimic system operations in DrawBitmap when drawing bitmaps with masks.

Changed 7 years ago by awi

Initial results.

Changed 7 years ago by awi

Result after applying 1-st patch.

Changed 7 years ago by awi

Results after applying 2-nd patch.

Changed 7 years ago by awi

Final results.

comment:4 Changed 7 years ago by vadz

Thanks for the patches, I agree that all this looks like the right thing(s) to do, but I'm a bit worried that in catering to such rare case we risk making the common case twice slower. Do you think it's not much of a concern? Maybe it would be enough to just ensure that the cursors appear correctly on the black (or white?) background?

I also didn't yet manage to convince myself that the new code produces exactly the same results as the old one for "normal" bitmaps with masks. I.e. why exactly is SRCCOPY equivalent to BLACKNESS followed by SRCINVERT? I see that the screenshots seem to show that nothing changed but I don't understand why...

Finally, at the code level, I'd strongly prefer to use MemoryHDC and SelectInHDC helper classes (from source:wxWidgets/trunk/include/wx/msw/private.h#L470) instead of manual calls to make the code automatically safe.

comment:5 Changed 7 years ago by awi

New simplified set patches don't look as bad as previous one. I think issues that are related to visual output should be taken with care and solved if it is only possible. Especially that maybe someone already complained about this issue in ticket:15699#comment:4.
You are right, it's possible to squeeze inside DoDrawBitmap() function two separate operations MaskBlt and BitBlt into single MaskBlt.
New mask operations give the same results for regular bitmaps as "old" call because for these bitmaps bits outside the picture shape are equal to 0 (AND mask is equal to 1 here) and SRCINVERT is equivalent to DSTCOPY.

AND mask orig.:   0            1
AND mask inv.:    1            0
dest. bitmap:     0 XOR src    dst XOR src
                  =src         (src=0 => dst unchanged)

In MSDN is stated that color bitmap is internally applied by using SRCINVERT.

Changed 7 years ago by awi

Extract XOR mask from monochrome wxIcon/wxCursor and use it as a main bitmap v2.

Changed 7 years ago by awi

Don't black out the transparent area when converting wxBitmap to icon (unchanged).

Changed 7 years ago by awi

Mimic system operations in DrawBitmap when drawing bitmaps with masks v2.

comment:6 Changed 7 years ago by vadz

Perfect, thanks for the patches and the explanations! I'm going to apply this soon, thanks again for your great work!

comment:7 Changed 7 years ago by VZ

In 78038:

Fix creating wxBitmap from monochrome icon or cursor in wxMSW.

Handle the "AND" and "XOR" masks inside monochrome icons/cursors correctly
instead of simply copying the monochrome data which didn't work at all.

See #16512.

comment:8 Changed 7 years ago by VZ

In 78039:

Don't replace bitmap transparency with black when converting to icon in wxMSW.

This was a wrong workaround for incorrect drawing of the icons elsewhere and
isn't needed if the destination is first AND-ed with the mask, as it should be.

See #16512.

comment:9 Changed 7 years ago by VZ

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

In 78040:

Use MaskBlt() with the correct ROP when drawing bitmaps with mask.

We need to AND the destination with the mask first and then XOR it with the
bitmap data to achieve the correct results.

Closes #16512.

comment:10 Changed 7 years ago by VZ

In 78054:

Revert "Use MaskBlt() with the correct ROP when drawing bitmaps with mask."

This reverts r78040 (see #16512) as it broke the appearance of the disabled
buttons in MSW toolbars as can be seen in the sample.

The change itself might still be correct and could have just uncovered some
other bug elsewhere, but for now still revert it just to make the toolbars
usable again.

comment:11 Changed 7 years ago by vadz

  • Patch unset
  • Resolution fixed deleted
  • Status changed from closed to reopened

awi, I'll try to look better into what's going on here myself but I wonder if you might already know what the problem is: the changes of r78040 totally broke the drawing of the disabled toolbar buttons as can be seen with "cut" and "copy" buttons in the toolbar sample, they're just blobs of grey now (well, not any more after r78054).

TIA!

comment:12 Changed 7 years ago by awi

Technical reason of this issue with toolbar is that icons has a valid (not-null) pixels also in the area which are covered by the mask and they are XOR'ed with the destination bitmap.
I see now what is a conceptual difference between wxBitmap mask and native Win bitmap mask and I should have taken this into account before.
If wxBitmap has a mask then this mask doesn't have to be used mandatory and one can display wxBitmap using mask as well as without mask. Due to this fact the image itself has to contain valid data also in the areas being covered by the mask and hence DSTCOPY operation works as expected for masked areas and SRCINVERT doesn't.
If native Win icon (especially monochrome one) has a mask bitmap it must be used mandatory when drawing the image in order to get correct output. For such icons image pixels are "nulled" in the areas covered by mask and SRCINVERT operation is equivalent to DSTCOPY. If they are intentionally left as "not-null" then we get the effect of inversion of the destination bitmap what is sometimes used for cursors.

Of course, for backwards compatibility reasons wxBitmap mask has to be optional and we need to find a way to convert native mandatory monochrome bitmap masks into optional wxBitmap masks.
I think a good compromise would be this what you proposed in comment:4. We could resign from displaying this tricky monochrome images with effects of inversion of destination bitmaps and simply display the basic shape from the image.
Result would look as follows:
Simplify drawing of monochrome cursors.

You can find the patch implementing this approach: attachment:Convert-AND-bitmap-to-wxBitmap-mask.patch

With this patch r78039 should be also reverted.

Changed 7 years ago by awi

Simplify drawing of monochrome cursors.

Changed 7 years ago by awi

Convert of native monochrome bitmap mask to wxBitmap mask.

comment:13 Changed 7 years ago by vadz

  • Milestone set to 3.1.0

Thanks for the explanation, much appreciated, as always.

I agree that the latest solution should be ok in practice. I guess we could store some flag in wxBitmap indicating whether it was created from a monochrome bitmap and use the mask differently (i.e. always) if it's set, but I don't believe it's worth the extra complications. The screenshot from the comment:12 is much better than the first screenshot from the comment:3 and IMHO good enough.

I'll test the latest patch a bit more locally and will apply it soon, thanks again!

comment:14 Changed 7 years ago by VZ

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

In 78123:

Improve drawing of monochrome bitmaps with masks in wxMSW.

This is a compromise solution between r78040, which handled monochrome bitmaps
correctly, but broke drawing bitmaps without using their mask, and r78054
which simply reverted it: this version preserves the old behaviour when not
using the mask, but draws at least the shape (if not the colour) correctly
for the monochrome bitmaps.

Notice that this also reverts r78039 which is not needed any more without
r78040.

Closes #16512.

Note: See TracTickets for help on using tickets.