wxAutoBufferedPaintDC can cause very slow rendering if drawing bitmaps with alpha on MSW (Issue #23841)
VZ
wxAutoBufferedPaintDC can cause very slow rendering if drawing bitmaps with alpha on MSW.
The reason is that the backing store bitmap used by wxAutoBufferedPaintDC is a DDB and not a DIB. If you draw an alpha bitmap onto the DC then AlphaBlt will use wxAlphaPixelData which when not using a DIB will make a copy of the whole backing store. It does this for each bitmap draw (can be pretty bad on wxAuiToolBar).
I've worked around it by making my own double buffering solution that uses this:
// Convert to DIB. This way, if transparent images are drawn then // you do not get a DDB=>DIB=>DDB conversion via temporary // expensive resource allocations! store.ConvertToDIB();
Thats meant I have to derive wxAuiToolBar and re-implement OnPaint. Would be nice to be able to avoid doing that.
Originally posted by @petebannister in #23585 (comment)
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
For the record, this is probably the same problem as alluded to here:
Except that using depth of 24 doesn't work when alpha is effectively used, of course. OTOH fixing this would allow to remove the workaround in the code above.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
dsa-t
KiCad also spends 90% of time drawing bitmaps while resizing
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
dsa-t
After updating from 3.2.4 to 3.2.5, aDc.GetAsBitmap().ConvertToDIB(); causes a (recursive) assert
"deleting bitmap still selected into wxMemoryDC"
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
It's normal that destroying a bitmap selected into a memory DC results in an assert, this really shouldn't be done.
The question is whether we should be always creating the backing store bitmap as a DIB or not and I am still not sure about the answer here. I think that in general using DIB should be faster because it is a better target for drawing into, which is done many times while composing it, and then it's only blitted to the screen once, but it's not at all impossible that there might be some situations in which drawing to it is fast anyhow but blitting it to screen is faster with DDB. I don't have any benchmarks to check it, however, and even if we had/I wrote one, there are so many different combinations of Windows version, display depth, video driver etc that it seems impossible to test all of them.
I think we could experiment with always using a DIB here in master, but I'm really not sure about doing this in 3.2.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
BTW, I forgot to mention, but the change needed to always create DIPs is trivial:
diff --git a/src/common/dcbufcmn.cpp b/src/common/dcbufcmn.cpp index ba31002c1d..74710fe561 100644 --- a/src/common/dcbufcmn.cpp +++ b/src/common/dcbufcmn.cpp @@ -84,7 +84,7 @@ private: // we must always return a valid bitmap but creating a bitmap of // size 0 would fail, so create a 1*1 bitmap in this case - buffer->CreateWithLogicalSize(wxMax(w, 1), wxMax(h, 1), scale); + buffer->CreateWithLogicalSize(wxMax(w, 1), wxMax(h, 1), scale, 32); return buffer; }
so it's not at all a problem to do it, the problem is to determine whether it's really a good idea.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Mark Roszko
Hmm Is that a valid fix? I've had some user complaints in kicad because at high DPIs, when you mouse hover over a wxAuiToolbar, you end up seeing painting lag. I suspect it's related to this.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@marekr I think this should help, but I don't have any simple way to benchmark this. If you could try rebuilding KiCad with this change and see if it helps, it would be great!
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
petebannister
I overrode wxAuiToolBar and duplicated OnPaint
//-----------------------------------------------------------------------------
void AuiToolBar::OnPaint(wxPaintEvent& evt)
{
// Modified from wxAuiToolBar::OnPaint() to use gwx::BufferedPaintDC
// instead which performs far better when drawing transparent images.
//
// Only other changes are to use accessor functions on items instead of
// accessing member variables directly since our derived class is not a
// friend of wxAuiToolBarItem.
static wxBitmap buffer;
gwx::BufferedPaintDC dc(buffer, this);
wxRect cli_rect(wxPoint(0, 0), GetClientSize());
...
And implementation of BufferedPaintDC:
class BufferedPaintDC : public wxMemoryDC
{
public:
using PixelDataType = wxAlphaPixelData;
static wxBitmap& initStore(wxBitmap& store, wxWindow* win) {
auto buf_size = store.GetSize();
auto this_size = win->GetClientSize();
auto new_buf_size = buf_size;
new_buf_size.IncTo(this_size);
new_buf_size.IncTo(wxSize(16,16)); // avoid trying to create invalid sized buffers.
auto scale = win->GetDPIScaleFactor();
if (!store.IsOk() || (buf_size != new_buf_size) || (store.GetScaleFactor() != scale)) {
if (win) {
store.CreateWithDIPSize(new_buf_size.x, new_buf_size.y, win->GetDPIScaleFactor(), 32);
}
else {
store.Create(new_buf_size.x, new_buf_size.y, 32);
}
// Convert to DIB. This way, if transparent images are drawn then
// you do not get a DDB=>DIB=>DDB conversion via temporary
// expensive resource allocations!
store.ConvertToDIB();
}
return store;
}
static wxBitmap& sharedStore() {
static wxBitmap store;
return store;
}
BufferedPaintDC(wxWindow* win)
: wxMemoryDC(initStore(sharedStore(), win))
, paint_dc_(win)
{
}
BufferedPaintDC(wxBitmap& store, wxWindow* win)
: wxMemoryDC(initStore(store, win))
, paint_dc_(win)
{
}
~BufferedPaintDC() {
blitToPaintDC();
}
void blitToPaintDC()
{
paint_dc_.Blit(wxPoint(0, 0), paint_dc_.GetSize(), this, wxPoint(0, 0));
}
private:
wxPaintDC paint_dc_;
};
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#23841)Well, thanks but, again, the question is what happens if you just make the text proposed above instead? AFAICS it should have exactly the same effect. In particular, the call to ConvertToDIB() seems completely unnecessary because specifying the depth of 32 should already result in a DIB being created.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#23841)@marekr Do you have a known way to reproduce the problem? I don't see anything wrong with the toolbars of the auidemo sample in the dark mode, but they use relatively small icons, do you use (much) bigger ones?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Mark Roszko
marekr left a comment (wxWidgets/wxWidgets#23841)@marekr Do you have a known way to reproduce the problem? I don't see anything wrong with the toolbars of the auidemo sample in the dark mode, but they use relatively small icons, do you use (much) bigger ones?
I've seen it demonstrated to me at a kicad conference. And I then in reproduced it on a high dpi tv as I lack high dpi monitors
I'll need to dig up a repro again. Our icons for hidpi are I believe 64x64
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#23841)FWIW I don't see anything obviously wrong with this diff:
diff --git a/samples/aui/auidemo.cpp b/samples/aui/auidemo.cpp index e0065863e50..e84d4cd7365 100644 --- a/samples/aui/auidemo.cpp +++ b/samples/aui/auidemo.cpp @@ -890,7 +890,7 @@ MyFrame::MyFrame(wxWindow* parent, wxAUI_TB_DEFAULT_STYLE | wxAUI_TB_OVERFLOW | wxAUI_TB_HORIZONTAL); tb2->SetExtraStyle(tb2->GetExtraStyle() | wxWS_EX_PROCESS_UI_UPDATES); - wxBitmapBundle tb2_bmp1 = wxArtProvider::GetBitmapBundle(wxART_QUESTION, wxART_OTHER, wxSize(16,16)); + wxBitmapBundle tb2_bmp1 = wxArtProvider::GetBitmapBundle(wxART_QUESTION, wxART_OTHER, wxSize(128,128)); tb2->AddTool(ID_SampleItem+6, "Disabled", tb2_bmp1); tb2->AddTool(ID_SampleItem+7, "Test", tb2_bmp1); tb2->AddTool(ID_SampleItem+8, "Test", tb2_bmp1);
but I'm not 100% sure if this bitmap actually uses alpha, I'll try to find one which definitely does to be sure.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#23841)There must be something else involved here because I still don't see the problem even with this diff
diff --git a/samples/aui/auidemo.cpp b/samples/aui/auidemo.cpp index e0065863e50..226da5f261e 100644 --- a/samples/aui/auidemo.cpp +++ b/samples/aui/auidemo.cpp @@ -934,7 +934,9 @@ MyFrame::MyFrame(wxWindow* parent, wxAUI_TB_TEXT | wxAUI_TB_HORZ_TEXT); tb4->SetExtraStyle(tb4->GetExtraStyle() | wxWS_EX_PROCESS_UI_UPDATES); - wxBitmapBundle tb4_bmp1 = wxArtProvider::GetBitmapBundle(wxART_NORMAL_FILE, wxART_OTHER, wxSize(16,16)); + wxInitAllImageHandlers(); + wxBitmapBundle tb4_bmp1 = wxBitmapBundle::FromFiles("../image/toucan.png"); + tb4->SetToolBitmapSize(wxSize(64,64)); tb4->AddTool(ID_DropDownToolbarItem, "Item 1", tb4_bmp1); tb4->AddTool(ID_SampleItem+23, "Item 2", tb4_bmp1); tb4->SetToolSticky(ID_SampleItem+23, true);
and this image definitely has alpha.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#23841)If anybody can provide a reproducer for this problem, I'd really like to fix it because it seems like it should be simple to do — but I don't want to do it blindly, without being able to check that the fix really works.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Squall Leonhart
Squall-Leonhart left a comment (wxWidgets/wxWidgets#23841)If anybody can provide a reproducer for this problem, I'd really like to fix it because it seems like it should be simple to do — but I don't want to do it blindly, without being able to check that the fix really works.
it looks like Cemu might be affected at high dpi, but you'll need an affected build from the point 3.3.0 was started being used and a set of games to generate icons from in the game list.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Mark Roszko
marekr left a comment (wxWidgets/wxWidgets#23841)@marekr Do you have a known way to reproduce the problem? I don't see anything wrong with the toolbars of the auidemo sample in the dark mode, but they use relatively small icons, do you use (much) bigger ones?
So far my only reproducer is two laptops, when on battery (CPU throttling?). They are fine on AC power lol.
The laptops aren't old, one of them is a bleeding edge snapdragon x elite processor.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#23841)So it's the same problem as was reported ~12 years ago... As discussed there, this can be worked around by using the following patch:
diff --git a/src/aui/auibook.cpp b/src/aui/auibook.cpp index c7283d65f39..716de48f126 100644 --- a/src/aui/auibook.cpp +++ b/src/aui/auibook.cpp @@ -923,7 +923,7 @@ void wxAuiTabContainer::Render(wxDC* raw_dc, wxWindow* wnd) wxBitmap bmp; // create off-screen bitmap - bmp.Create(m_rect.GetWidth(), m_rect.GetHeight(),*raw_dc); + bmp.CreateWithLogicalSize(m_rect.GetSize(), raw_dc->GetContentScaleFactor(), 24); dc.SelectObject(bmp); if (!dc.IsOk()) diff --git a/src/common/dcbufcmn.cpp b/src/common/dcbufcmn.cpp index ba31002c1d8..f60d6a2d5e7 100644
--- a/src/common/dcbufcmn.cpp +++ b/src/common/dcbufcmn.cpp @@ -84,7 +84,7 @@ private: // we must always return a valid bitmap but creating a bitmap of // size 0 would fail, so create a 1*1 bitmap in this case - buffer->CreateWithLogicalSize(wxMax(w, 1), wxMax(h, 1), scale);
+ buffer->CreateWithLogicalSize(wxMax(w, 1), wxMax(h, 1), scale, 24);
return buffer;
}which avoids having to tweak alpha manually, and I'm relatively confident that it should fix your problem (but please test if you can), so it should probably be committed, but it certainly would be great if we could avoid this kind of hacks... Unfortunately I still don't see how to do it, disabling the code adjusting alpha makes the bug originally reported in #14403 reappear and this is not good either.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#23841)I've created a PR with this change, please test it.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Mark Roszko
marekr left a comment (wxWidgets/wxWidgets#23841)I've created a PR with this change, please test it.
I already applied your previous posted diff to builds yesterday. I am awaiting the user where the issue is very blatant to test it.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
vadz left a comment (wxWidgets/wxWidgets#23841)OK, thanks, the effect should be the same.
FWIW I've checked that the code manually adjusting alpha in AlphaBlt() is never executed with this patch, so if there is still some problem it would be great to have the new stack trace/flamegraph as it would then be coming from some other place.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.