Opened 3 years ago

Closed 6 months ago

#13328 closed defect (fixed)

Converting wxBitmap of depth 24 but with alpha to wxImage doesn't work correctly

Reported by: wojdyr Owned by: vadz
Priority: normal Milestone: 3.0.1
Component: wxMSW Version: stable-latest
Keywords: transparency, wxBitmap, wxImage DIB DDB alpha GDI+ Cc:
Blocked By: Blocking:
Patch: yes

Description

AFAICS wxDIB can be either PixelFormat_PreMultiplied or PixelFormat_NotPreMultiplied.

wxDIB::ConvertToImage() always undoes premultiplication and it sometimes damages image.

Example: http://img143.imageshack.us/img143/9242/mswwxdibproblem.png
In the drawing sample use GraphicContext and then File > Save to file (added in #13327). If I remove this code

                if ( a > 0 )
                {
                    dst[0] = (dst[0] * 255) / a;
                    dst[1] = (dst[1] * 255) / a;
                    dst[2] = (dst[2] * 255) / a;
                }

I get good image in this case.

Attachments (3)

circles_gc.png download (95.2 KB) - added by ericj 3 years ago.
Output from "drawing" sample, using wxGraphicsContext.
reset-alpha-for-DDB.patch download (3.8 KB) - added by awi 7 months ago.
Reset alpha channel in DDB in wxGCDC destructor
convert_DDB_to_DIB_for_GDIplus.patch download (4.5 KB) - added by awi 7 months ago.
Convert DDB to DDI and reset alpha channel in wxGCDC constructor

Download all attachments as: .zip

Change History (27)

comment:1 Changed 3 years ago by vadz

  • Status changed from new to infoneeded_new

Really strange, I don't see the problem in the drawing sample if I save the contents of the "Splines" screen as PNG. Which format do you use for saving? And which OS version do you use?

And the current code looks correct to me, wxBitmap stores pre-multiplied data to the best of my knowledge so we do need to undo pre-multiplication when converting to wxImage.

comment:2 Changed 3 years ago by wojdyr

  • Status changed from infoneeded_new to new

Sorry, I forgot to write what is important here:
it doesn't work when Use GraphicContext is on.

comment:3 Changed 3 years ago by vadz

Ok, I do see the problem now. Trouble is I still don't see the solution: even if I disable the un-pre-multiplication in wxDIB (and I still don't see any reason why should it be disabled, to the best of my knowledge GDI+ uses pre-multiplied alpha too) I still get garbage on output. It looks like the background is somehow never erased properly... and looking at the code I indeed don't see where is this done if the background colour is not explicitly set. But worse, even if I do set the background colour in the sample UI the background still remains black (and I bet alpha still remains garbage) even though Clear() is definitely called then.

I'm clearly missing something here, can anybody see what's going on here?

comment:4 Changed 3 years ago by wojdyr

If I call ClearAlpha() without un-pre-multiplication I get a correct image, although I have no idea why.

comment:5 Changed 3 years ago by vadz

  • Status changed from new to confirmed

This confirms that I thought: alpha channel of wxBitmap is not cleared properly when erasing wxGCDC. I still don't know why does this happen though.

comment:6 Changed 3 years ago by vadz

  • Cc csomor added
  • Milestone set to 3.0
  • Priority changed from normal to high
  • Summary changed from [MSW] wxDIB::ConvertToImage() bug to [MSW] wxGCDC doesn't work with wxMemoryDC

It seems that using wxGCDC with wxMemoryDC simply doesn't work. This simple test:

  • samples/minimal/minimal.cpp

    a b IMPLEMENT_APP(MyApp) 
    116116// the application class 
    117117// ---------------------------------------------------------------------------- 
    118118 
     119#include "wx/rawbmp.h" 
     120#include "wx/dcgraph.h" 
     121 
    119122// 'Main program' equivalent: the program execution "starts" here 
    120123bool MyApp::OnInit() 
    121124{ 
    bool MyApp::OnInit() 
    124127    if ( !wxApp::OnInit() ) 
    125128        return false; 
    126129 
     130    const int WIDTH = 2; 
     131    const int HEIGHT = 2; 
     132    wxBitmap bmp(WIDTH, HEIGHT); 
     133    { 
     134        wxMemoryDC mdc(bmp); 
     135 
     136        wxGCDC gdc(mdc); 
     137        gdc.SetBackground(*wxBLUE); 
     138        gdc.Clear(); 
     139        gdc.SetPen(*wxRED_PEN); 
     140        gdc.DrawLine(0, 0, 1, 1); 
     141    } 
     142 
     143    wxMessageOutputBest out; 
     144 
     145    wxAlphaPixelData data(bmp); 
     146    wxAlphaPixelData::Iterator p(data); 
     147    for ( int y = 0; y < HEIGHT; ++y ) 
     148    { 
     149        wxAlphaPixelData::Iterator rowStart = p; 
     150        for ( int x = 0; x < WIDTH; ++x ) 
     151        { 
     152            out.Printf("(%d, %d): %02x%02x%02x%02x", 
     153                       y, x, p.Red(), p.Green(), p.Blue(), p.Alpha()); 
     154        } 
     155 
     156        p = rowStart; 
     157        p.OffsetY(data, 1); 
     158    } 
     159 
     160    return false; 
     161 
    127162    // create the main application window 
    128163    MyFrame *frame = new MyFrame("Minimal wxWidgets App"); 
    129164 

The output is (if you run it from console to avoid 4 message boxes):

(0, 0): 8f00708f
(0, 1): 8f00708f
(1, 0): 3000cf30
(1, 1): 3000cf30

which makes no sense to me. FWIW drawing directly on wxMemoryDC does work and produces the semi-expected (not sure why does it draw the upper right pixel -- anti-aliasing perhaps?)

(0, 0): ff000000
(0, 1): ff000000
(1, 0): 0000ff00
(1, 1): 0000ff00

My hypothesis is that the bitmap format we use is somehow incompatible with GDI+ but I don't see what else could we do. Stefan, would you have any idea about this by chance?

comment:7 Changed 3 years ago by csomor

I'll try to follow the alpha multiplication modes

comment:8 Changed 3 years ago by ericj

The strange debug output could be explained by not incrementing "p" in the inner loop :)

Anyway, i think there must be another factor involved, because saving the screen from the "drawing" sample creates a correct image for me (attachment follows).

I tested with todays SVN, VS2003.

Changed 3 years ago by ericj

Output from "drawing" sample, using wxGraphicsContext.

comment:9 Changed 3 years ago by vadz

Oops, thanks for noticing the missing increment in the code. However adding it doesn't really help, the new output is:

(0, 0): 8f00708f
(0, 1): 1000ef10
(1, 0): 3000cf30
(1, 1): af0050af

which is still pretty strange...

comment:10 Changed 3 years ago by ericj

If you save the bitmap and load it in a graphics program it looks somewhat ok. But not what it's supposed to look like.

I changed it to create a 16x16 bitmap and the effect becomes a little clearer:

  • if you comment out the gdc.DrawLine() call, the result is correct, a solid blue background
  • with the gdc.DrawLine() call, the result is an anti-aliased red line on a transparent(!) background. How the drawline manages to clear the background is a little of a mystery to me
  • however, if you explicitly create the bitmap with 32bit with wxBitmap bmp(WIDTH, HEIGHT, 32), the resulting bitmap is correct: Blue background with anti-aliased red line on top

comment:11 Changed 20 months ago by vadz

  • Priority changed from high to normal

The problem seems to be with conversion to wxImage rather than wxGCDC itself. If I run this:

  • samples/minimal/minimal.cpp

    diff --git a/samples/minimal/minimal.cpp b/samples/minimal/minimal.cpp
    index a78e462..52dbf51 100644
    a b  
    3030    #include "wx/wx.h" 
    3131#endif 
    3232 
     33#include "wx/dcgraph.h" 
     34 
    3335// ---------------------------------------------------------------------------- 
    3436// resources 
    3537// ---------------------------------------------------------------------------- 
    bool MyApp::OnInit() 
    124126    if ( !wxApp::OnInit() ) 
    125127        return false; 
    126128 
     129    wxBitmap bmp(10, 10); 
     130    { 
     131        wxMemoryDC mdc(bmp); 
     132        wxGCDC gdc(mdc); 
     133        gdc.SetBackground(*wxBLUE); 
     134        gdc.Clear(); 
     135        gdc.SetPen(*wxRED_PEN); 
     136        gdc.DrawLine(0, 0, 10, 10); 
     137    } 
     138    wxInitAllImageHandlers(); 
     139    bmp.SaveFile("testbitmap.bmp", wxBITMAP_TYPE_BMP); 
     140    bmp.ConvertToImage().SaveFile("testimage.bmp", wxBITMAP_TYPE_BMP); 
     141    bmp.ConvertToImage().SaveFile("testimage.png", wxBITMAP_TYPE_PNG); 
     142    return false; 
     143 
    127144    // create the main application window 
    128145    MyFrame *frame = new MyFrame("Minimal wxWidgets App"); 
    129146 

then testbitmap.bmp is correct, testimage.bmp loses alpha entirely (and hence is slightly wrong) and testimage.png loses background (i.e. is very wrong).

Investigating...

comment:12 follow-up: Changed 20 months ago by vadz

  • Cc csomor removed
  • Keywords DIB alpha added
  • Milestone 3.0 deleted
  • Summary changed from [MSW] wxGCDC doesn't work with wxMemoryDC to Converting wxBitmap of depth 24 but with alpha to wxImage doesn't work correctly

OK, I was wrong, it's not about conversion to wxImage but rather about conversion of DDB (that we initially work with) to DIB (which we then use to initialize wxImage). What Windows does here just doesn't make any sense: even though the DDB is clearly correct (as can be seen from the created testbitmap.bmp file), the bits returned for the DIB are wrong: under debugger I see that the upper left pixel is 0x8f8f0070 which is fine (it's in ABGR format and premultiplied, so it's really blue), plenty of subsequent ones have alpha set to 0, i.e. fully transparent, and I just don't see what can we do about it.

If we handle 0 alpha in premultiplied as fully opaque, i.e. apply

  • src/msw/dib.cpp

    diff --git a/src/msw/dib.cpp b/src/msw/dib.cpp
    index b12baf0..2e30178 100644
    a b wxImage wxDIB::ConvertToImage() const 
    797797                // wxImage uses non premultiplied alpha so undo 
    798798                // premultiplication done in Create() above 
    799799                const unsigned char a = *src; 
    800                 *alpha++ = a; 
    801800 
    802801                // Check what kind of alpha do we have. 
    803802                switch ( a ) 
    wxImage wxDIB::ConvertToImage() const 
    817816                        break; 
    818817                } 
    819818 
    820                 if ( a > 0 ) 
     819                if ( a == 0 ) 
    821820                { 
     821                    *alpha++ = wxALPHA_OPAQUE; 
     822                } 
     823                else 
     824                { 
     825                    *alpha++ = a; 
    822826                    dst[0] = (dst[0] * 255) / a; 
    823827                    dst[1] = (dst[1] * 255) / a; 
    824828                    dst[2] = (dst[2] * 255) / a; 

then it's a bit better, we get the following XPM (obtained by converting the PNG from the above code):

/* XPM */
static char *testimage[] = {
/* columns rows colors chars-per-pixel */
"10 10 4 1",
"  c #FF0055",
". c #0000FF",
"X c #FF00C7",
"o c None",
/* pixels */
"Xo........",
"o o.......",
".o o......",
"..o o.....",
"...o o....",
"....o o...",
".....o o..",
"......o o.",
".......o o",
"........o "
};

but it's still different compared with the XPM corresponding to testbitmap.bmp:

/* XPM */
static char *testimage[] = {
/* columns rows colors chars-per-pixel */
"10 10 5 1",
"  c #BF0040",
". c #8F0070",
"X c #3000CF",
"o c #1000EF",
"O c #0000FF",
/* pixels */
".oOOOOOOOO",
"X oOOOOOOO",
"OX oOOOOOO",
"OOX oOOOOO",
"OOOX oOOOO",
"OOOOX oOOO",
"OOOOOX oOO",
"OOOOOOX oO",
"OOOOOOOX o",
"OOOOOOOOX "
};

but this could be due to different handling of semi-transparent pixels in Windows GDI code saving the BMP and ImageMagick code creating XPM from PNG.

Of course, we can't really apply the patch above because it doesn't deal correctly with fully transparent pixels. But it seems that GDI+ represents them as 0x00000000 only. So perhaps we could still do interpret 0 alpha as opaque if the other components are not 0?

And, of course, there is still the question of whether there can be any alpha at all in 24 bit bitmap. Perhaps it should just be discarded completely?

Or maybe we should just say that alpha is only supported with (explicitly) 32bpp bitmaps in wxMSW. At the very least, this is a decent workaround so I'm removing the milestone, this is not 3.0-critical.

comment:13 in reply to: ↑ 12 Changed 16 months ago by wojdyr

I've just read the insightful comments above (Trac notifications don't work for me).
The workaround works fine, thanks!

In my code I also had to add a call to wxGCDC::SetBackground()/Clear(), like in the sample above. I haven't had it before because in other cases (Linux, Mac and saving bitmap on Windows without converting to wxImage) it's enough to clear only wxMemoryDC.

Replying to vadz:

And, of course, there is still the question of whether there can be any alpha at all in 24 bit bitmap. Perhaps it should just be discarded completely?

Or maybe we should just say that alpha is only supported with (explicitly) 32bpp bitmaps in wxMSW.

Both seem reasonable to me.

comment:14 Changed 7 months ago by vadz

I wonder if the fix for #14403 could help with this bug as well?

Changed 7 months ago by awi

Reset alpha channel in DDB in wxGCDC destructor

comment:15 Changed 7 months ago by awi

  • Keywords DDB added
  • Patch set

Indeed, it seems that GDI+ gives unexpected results when its graphics surface is associated with DDB (and DDB is created by default when wxBitamp is constructed with no color depth specified explicitly). In this case it can be observed that alpha channel is messed by any graphics operation. For DIBs everything works as expected.
The solution could be to reset alpha channel for output bitmap in wxGCDC destructor when GDI+ graphics was associated with DDB.

Patch attached.

comment:16 Changed 7 months ago by vadz

We could do this (although I'd prefer to use helper classes from wx/msw/private.h where possible instead of raw GDI calls), but I wonder if it wouldn't be better to switch to DIB representation when selecting a bitmap into wxGraphicsContext instead. Because then we would probably be able to have correct alpha channel in the bitmap instead of none at all, which would seem to be better, wouldn't it?

comment:17 Changed 7 months ago by awi

  • Keywords GDI+ added

You're right, converting DDB to DIB in the background (in wxGCDC constructor) before it is passed to GDI+ is a better option.
New patch file attached.

comment:18 Changed 7 months ago by vadz

  • Milestone set to 3.1.0
  • Owner set to vadz
  • Status changed from confirmed to accepted

Thanks, this looks good to me. I'll test it a bit and apply later.

comment:19 Changed 7 months ago by awi

Updated patch file uploaded.
Assures better interoperability between GDI and GDI+ with regards to bitmaps with fake alpha channel. Such bitmaps have all alpha bytes set to 0 value but GDI+ needs apparently FF value (otherwise strange visual effects appear).
Now its possible to draw on memory DC and GDI+ for the same bitmap, like on the example below:

const int WIDTH = 30;  
const int HEIGHT = 30;
wxBitmap bmp(WIDTH, HEIGHT);  // DDB

{  
        wxMemoryDC mdc(bmp);

        mdc.SetBackground(*wxLIGHT_GREY);
        mdc.Clear();
        mdc.SetPen(*wxBLUE);
        mdc.DrawLine(1, 1, WIDTH-1, HEIGHT-1);

        {
            wxGCDC gdc(mdc);

            gdc.SetPen(*wxRED_PEN);
            gdc.DrawLine(1, HEIGHT-1, WIDTH-1, 1);
        }
}

Changed 7 months ago by awi

Convert DDB to DDI and reset alpha channel in wxGCDC constructor

comment:20 Changed 6 months ago by VZ

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

(In [75648]) Fix alpha channel values when using wxGCDC with wxMemoryDC in wxMSW.

Ensure that 32bpp bitmaps selected in wxMemoryDC use DIB for their internal
representation as GDI+ functions don't seem to work correctly with DDBs with
alpha channel.

Closes #13328.

comment:21 Changed 6 months ago by vadz

  • Milestone changed from 3.1.0 to 3.0.1
  • Resolution changed from fixed to port to stable
  • Status changed from closed to portneeded

We need to backport this if we backport the rest of wxBitmap-related changes.

comment:22 Changed 6 months ago by VZ

(In [75651]) Exclude wxMSW-specific code from compilation under other platforms.

Compilation fix after r75648, see #13328.

comment:23 Changed 6 months ago by VZ

(In [75652]) Use wxUSE_WXDIB in preprocessor tests.

NEVER_USE_DIB is meant to be used only in conjunction with
{SOMETIMES,ALWAYS}_USE_DIB constants, use wxUSE_WXDIB elsewhere.

This is more logical and also fixes inadvertent use of NEVER_USE_DIB before it
is defined added by the changes of r75648.

See #13328.

comment:24 Changed 6 months ago by VZ

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

(In [75765]) Fix alpha channel values when using wxGCDC with wxMemoryDC in wxMSW.

Ensure that 32bpp bitmaps selected in wxMemoryDC use DIB for their internal
representation as GDI+ functions don't seem to work correctly with DDBs with
alpha channel.

Closes #13328.

Note: See TracTickets for help on using tickets.