Cannot use wxMemoryDC in parallel threads

[Frédéric]

Hi,

I am creating a lot of bitmaps that I just want to store on disk; I do
not need to show them in the GUI. For this, I use wxMemoryDC and
wxBitmap. However, to speedup the process, I split the work in
different threads but I get a lot of segmentation faults when
constructing wxMemoryDC or when calling wxMemoryDC::SelectObject():
this apparently calls to SetFont or SetPen which ends in a reference
counting area of GDI objects that causes the segfaults.

So I mutex protected wxMemory creation and all calls to SetFont and
SetForgroundColour... but the interest of multi-threading is not so
great then.

I found those 2 threads from 10 years ago that describe quite well the
problem without giving any solution:
- https://groups.google.com/forum/#!topic/wx-users/ZAnUaEot0Ys
- http://wxwidgets.10942.n7.nabble.com/making-GDI-objects-thread-safe-td22667.html

However, in one of the treads, Vadim said this:
"this should already work if you use wxMemoryDC for wxBitmap creation
and if you create your pens, brushes &c in the thread and only use them
there. If it doesn't, this is a bug we may fix."

It seems to indicate that it is possible to make it work but I just do
not manage to do it. In particular, what does it mean to "create your
pends, brushes... in the thread"?

For example, I do the following in the worker threads:
dc.SetForegroundColour(*wxWHITE)
or
dc.SetFont(wxFont{15, wxFONTFAMILY_DEFAULT, wxFONTSTYLE_NORMAL,
wxFONTWEIGHT_BOLD})

Of course my threads are completely independant from each other, they
do not share anything but clearly wxWHITE is common to all threads but
I do not modify it...

Any idea?

F

[Vadim Zeitlin]

On Wed, 11 Jul 2018 21:13:39 +0200 Frédéric wrote:

F> I am creating a lot of bitmaps that I just want to store on disk; I do
F> not need to show them in the GUI. For this, I use wxMemoryDC and
F> wxBitmap. However, to speedup the process, I split the work in
F> different threads but I get a lot of segmentation faults when
F> constructing wxMemoryDC or when calling wxMemoryDC::SelectObject():
F> this apparently calls to SetFont or SetPen which ends in a reference
F> counting area of GDI objects that causes the segfaults.
F>
F> So I mutex protected wxMemory creation and all calls to SetFont and
F> SetForgroundColour... but the interest of multi-threading is not so
F> great then.

A possible solution is to use wxImage in the threads and then convert it
to wxBitmap. This should be MT-safe, I think.

F> I found those 2 threads from 10 years ago that describe quite well the
F> problem without giving any solution:
F> - https://groups.google.com/forum/#!topic/wx-users/ZAnUaEot0Ys
F> - http://wxwidgets.10942.n7.nabble.com/making-GDI-objects-thread-safe-td22667.html
F>
F> However, in one of the treads, Vadim said this:
F> "this should already work if you use wxMemoryDC for wxBitmap creation
F> and if you create your pens, brushes &c in the thread and only use them
F> there. If it doesn't, this is a bug we may fix."
F>
F> It seems to indicate that it is possible to make it work but I just do
F> not manage to do it. In particular, what does it mean to "create your
F> pends, brushes... in the thread"?

Create objects only accessible from this thread.

F> For example, I do the following in the worker threads:
F> dc.SetForegroundColour(*wxWHITE)
F> or
F> dc.SetFont(wxFont{15, wxFONTFAMILY_DEFAULT, wxFONTSTYLE_NORMAL,
F> wxFONTWEIGHT_BOLD})
F>
F> Of course my threads are completely independant from each other, they
F> do not share anything but clearly wxWHITE is common to all threads but
F> I do not modify it...

wxWHITE and other stock objects are just (fancy) global variables, so you
can't use them. You should be able to use wxColour(0xff, 0xff, 0xff)
however, at least under MSW. I'm not really sure about other systems
though.

Regards,
VZ

--
TT-Solutions: wxWidgets consultancy and technical support
http://www.tt-solutions.com/

[Daniel Kulp]

We ran into these issues as well.   We ended up creating a "pool" of contexts that wrapper the wxBitmap/wxMemoryDC/wxGraphivsContext and related things.   If there isn't a context available in the pool, it does a window->CallAfter(...) kind of thing to create one (and waits) so that they are only created on the main thread.   As part of the creation, it has to go through and "UnShare()"  a bunch of objects to make sure they have a unique copy.   The entire top of:

https://github.com/smeighan/xLights/blob/master/xLights/RenderBuffer.cpp

shows some of what we had to do.  Unfortunately, a lot of it is also platform specific so there a bunch of #ifdefs in there.  :(

Dan

[Frédéric]

> A possible solution is to use wxImage in the threads and then convert it
> to wxBitmap. This should be MT-safe, I think.

But then, I cannot write on the images.

> wxWHITE and other stock objects are just (fancy) global variables, so you
> can't use them. You should be able to use wxColour(0xff, 0xff, 0xff)
> however, at least under MSW. I'm not really sure about other systems
> though.

What about fonts? Can I create a local object?

F

[Frédéric]

> We ran into these issues as well. We ended up creating a "pool" of
> contexts that wrapper the wxBitmap/wxMemoryDC/wxGraphivsContext and related
> things. If there isn't a context available in the pool, it does a
> window->CallAfter(...) kind of thing to create one (and waits) so that they
> are only created on the main thread. As part of the creation, it has to go
> through and "UnShare()" a bunch of objects to make sure they have a unique
> copy.

https://github.com/smeighan/xLights/blob/master/xLights/RenderBuffer.cpp

Thank you for the link, I now see what I have to do!

Kind regards,

F

[Vadim Zeitlin]

On Thu, 12 Jul 2018 06:12:50 +0200 Frédéric wrote:

F> > A possible solution is to use wxImage in the threads and then convert it
F> > to wxBitmap. This should be MT-safe, I think.
F>
F> But then, I cannot write on the images.

You can modify their pixels directly but you indeed can't output text to
them, for example. There is a potentially interesting special case that you
might be interested in: you can output text onto wxImage by using
Cairo-based wxGraphics implementation. This was originally done to allow
creating bitmaps without using X server under Unix (i.e. from console
applications), but it could be useful to you as well.

F> > wxWHITE and other stock objects are just (fancy) global variables, so you
F> > can't use them. You should be able to use wxColour(0xff, 0xff, 0xff)
F> > however, at least under MSW. I'm not really sure about other systems
F> > though.
F>
F> What about fonts? Can I create a local object?

Yes, sure, why not?

[Frédéric]

> You can modify their pixels directly but you indeed can't output text to
> them, for example. There is a potentially interesting special case that you
> might be interested in: you can output text onto wxImage by using
> Cairo-based wxGraphics implementation. This was originally done to allow
> creating bitmaps without using X server under Unix (i.e. from console
> applications), but it could be useful to you as well.

That could be useful because in this application, I run it as a
console application although I have to use a wxApp.

> F> What about fonts? Can I create a local object?
>
> Yes, sure, why not?

I applied the method of Daniel (calling UnShare everywhere...) and it worked.

Thanks,

F

[Vadim Zeitlin]

On Fri, 13 Jul 2018 23:33:45 +0200 Frédéric wrote:

F> > F> What about fonts? Can I create a local object?
F> >
F> > Yes, sure, why not?
F>
F> I applied the method of Daniel (calling UnShare everywhere...) and it worked.

This is very error-prone and fragile. I'm impressed that it works
(although, just to be a spoilsport, have you run it with TSAN?), but I
wouldn't recommend doing it like this.

[Daniel Kulp]

On Friday, July 13, 2018 at 5:42:27 PM UTC-4, Vadim Zeitlin wrote:

On Fri, 13 Jul 2018 23:33:45 +0200 Frédéric wrote:

F> I applied the method of Daniel (calling UnShare everywhere...) and it worked.

 This is very error-prone and fragile. I'm impressed that it works
(although, just to be a spoilsport, have you run it with TSAN?), but I
wouldn't recommend doing it like this.

Several years ago when we were working on getting our rendering to work on the background threads, I strongly considered submitting a patch to change all the reference counting over to use atomic<int> .  I kind of regret not doing that, but at the time, I needed something that would work "soon" and releases of wxWidgets are NEVER "soon".  Thus, we went with the UnShare thing which has worked very well.   If wxWidgets would have a good track record of actually providing releases, maybe things would be different.

[Vadim Zeitlin]

On Fri, 13 Jul 2018 17:18:11 -0700 (PDT) Daniel Kulp wrote:

DK> Several years ago when we were working on getting our rendering to work on
DK> the background threads, I strongly considered submitting a patch to change
DK> all the reference counting over to use atomic<int> . I kind of regret not
DK> doing that, but at the time, I needed something that would work "soon" and
DK> releases of wxWidgets are NEVER "soon". Thus, we went with the UnShare
DK> thing which has worked very well. If wxWidgets would have a good track
DK> record of actually providing releases, maybe things would be different.

Mea culpa and all that -- it's true that we're pretty bad with releases.
This being said, I never understood what prevented people who knew what
they were doing from using more or less any arbitrary Git commit without
waiting for the official release. We do try to keep master always in good
shape and while this does sometimes fail, it gets fixed pretty quickly when
it happens and I think that less than 0.1% of wxWidgets revisions since at
least the last 15 years (things were a bit more wild before then) were
actually broken. Or, to look at it from another direction, most of our
releases don't really differ from any arbitrary snapshot that much (again,
there are exceptions, 3.0.0 was tested much more than any other release
before or after it, but this was exceptional).

So yes, we should release more often, but I still have no idea why should
this be such a huge problem...

[Kenneth Porter]

--On Saturday, July 14, 2018 3:34 AM +0200 Vadim Zeitlin

<va...@wxwidgets.org> wrote:

> I never understood what prevented people who knew what
> they were doing from using more or less any arbitrary Git commit without
> waiting for the official release.

My app links statically to wx so that's what I do. I don't depend on shared
libraries prepared by others, because my app should be the only wx app on
the machine. (It's an embedded industrial project.)

But people in the Linux world depend on a stable API provided by the
distro. So I'd suggest tagging a new "release" (minor version) if setup.h
changes or if there's a significant change to the headers that would break
downstream projects on the big distros. They can then grab whatever tag
they feel comfortable with.

What would the distro people here want in a formal release?

[Frédéric]

> This is very error-prone and fragile. I'm impressed that it works
> (although, just to be a spoilsport, have you run it with TSAN?), but I
> wouldn't recommend doing it like this.

I agree, that it is not really safe as it uses const_cast!

The sanitizer seems fine: just the usual gtk memory leaks.
But in debug mode, I get a lot of assertion failures:

Gtk-Message: GtkDialog mapped without a transient parent. This is discouraged.
include/wx/colour.h(194): assert ""Assert failure"" failed in
CloneGDIRefData(): must be overridden if used
Gtk-Message: GtkDialog mapped without a transient parent. This is discouraged.
src/common/object.cpp(394): assert "m_refData &&
m_refData->GetRefCount() == 1" failed in AllocExclusive():
wxObject::AllocExclusive() failed.

include/wx/colour.h(194): assert ""Assert failure"" failed in
CloneGDIRefData(): must be overridden if used
src/common/object.cpp(394): assert "m_refData &&
m_refData->GetRefCount() == 1" failed in AllocExclusive():
wxObject::AllocExclusive() failed

The thing is that it is just a helper program to build a random
database. Not something supposed to run at a customer! So as soon as
it does the job, I am happy.

F

[Vadim Zeitlin]

On Sat, 14 Jul 2018 20:00:26 +0200 Frédéric wrote:

F> But in debug mode, I get a lot of assertion failures:
F>
F> Gtk-Message: GtkDialog mapped without a transient parent. This is discouraged.
F> include/wx/colour.h(194): assert ""Assert failure"" failed in
F> CloneGDIRefData(): must be overridden if used
F> Gtk-Message: GtkDialog mapped without a transient parent. This is discouraged.
F> src/common/object.cpp(394): assert "m_refData &&
F> m_refData->GetRefCount() == 1" failed in AllocExclusive():
F> wxObject::AllocExclusive() failed.
F>
F> include/wx/colour.h(194): assert ""Assert failure"" failed in
F> CloneGDIRefData(): must be overridden if used
F> src/common/object.cpp(394): assert "m_refData &&
F> m_refData->GetRefCount() == 1" failed in AllocExclusive():
F> wxObject::AllocExclusive() failed

You probably wouldn't get those if you stopped calling UnShare() on
wxColour objects if they're not really shared (and they shouldn't be).
Alternatively, actually implementing {Create,Clone}GDIRefData() for
wxColour in wxGTK shouldn't be that difficult, it just never seemed to be
worth it...

F> The thing is that it is just a helper program to build a random
F> database. Not something supposed to run at a customer! So as soon as
F> it does the job, I am happy.

If you're sure it does it correctly, sure. I'd double check the colours it
uses, just in case.

[Frédéric]

> This being said, I never understood what prevented people who knew what
> they were doing from using more or less any arbitrary Git commit without
> waiting for the official release.

Is there a way to easily get a tarball of the doc from any master commit?

F

[Vadim Zeitlin]

On Sun, 15 Jul 2018 12:53:55 +0200 Frédéric wrote:

F> > This being said, I never understood what prevented people who knew what
F> > they were doing from using more or less any arbitrary Git commit without
F> > waiting for the official release.
F>
F> Is there a way to easily get a tarball of the doc from any master commit?

Not for any master commit, but the latest documentation is always
available online on http://docs.wxwidgets.org/trunk/ and you can easily
build it locally if needed (you basically just need Doxygen and, for the
diagrams, ImageMagick).

[Frédéric]

> Not for any master commit, but the latest documentation is always
> available online on http://docs.wxwidgets.org/trunk/

I like to use a local version because I very much prefer the search
engine than the one on the web site.

> you can easily
> build it locally if needed.

Any readme for that?

Thanks,

F

[Vadim Zeitlin]

On Sun, 15 Jul 2018 15:30:51 +0200 Frédéric wrote:

F> > Not for any master commit, but the latest documentation is always
F> > available online on http://docs.wxwidgets.org/trunk/
F>
F> I like to use a local version because I very much prefer the search
F> engine than the one on the web site.
F>
F> > you can easily build it locally if needed.
F>
F> Any readme for that?

You can check docs/contributing/how-to-add-class-documentation.md for more
details but, basically, you just need to run docs/doxygen/regen.sh script.

[Frédéric]

> You can check docs/contributing/how-to-add-class-documentation.md for more
> details but, basically, you just need to run docs/doxygen/regen.sh script.

Thanks, it worked. I just had to define WX_SKIP_DOXYGEN_VERSION_CHECK:

WX_SKIP_DOXYGEN_VERSION_CHECK="a" ./regen.sh html

[Daniel Kulp]

I just discovered that with the change to wxColour to be a reference counted thing, this "fix" no longer works and I cannot see how to make it work again. There are a few Color objects created in the call to ctx.SelectObject(...) that I cannot "trap" and make sure they are non-defaults.   Thus, various "default" color objects get destroyed and things crash.  :(

The only fix I've been able to find is to properly make the reference counting in wxWidgets thread safe:

diff --git a/include/wx/object.h b/include/wx/object.h
index 1dd57cd654..9eea0aa464 100644
--- a/include/wx/object.h
+++ b/include/wx/object.h
@@ -16,6 +16,7 @@
 // headers
 // ----------------------------------------------------------------------------


+#include <atomic>
 #include "wx/memory.h"


 #define wxDECLARE_CLASS_INFO_ITERATORS()                                     \
@@ -228,7 +229,7 @@ inline T *wxCheckCast(const void *ptr)
 class WXDLLIMPEXP_BASE wxRefCounter
 {
 public:
-    wxRefCounter() { m_count = 1; }
+    wxRefCounter() : m_count(1) { }


     int GetRefCount() const { return m_count; }


@@ -242,7 +243,7 @@ protected:


 private:
     // our refcount:
-    int m_count;
+    std::atomic_int m_count;


     // It doesn't make sense to copy the reference counted objects, a new ref
     // counter should be created for a new object instead and compilation
diff --git a/src/common/object.cpp b/src/common/object.cpp
index 52ff0adb76..aca8caf86d 100644
--- a/src/common/object.cpp
+++ b/src/common/object.cpp
@@ -338,7 +338,7 @@ void wxRefCounter::DecRef()
 {
     wxASSERT_MSG( m_count > 0, "invalid ref data count" );


-    if ( --m_count == 0 )
+    if ( m_count.fetch_sub(1) == 0 )
         delete this;
 }

[Frédéric]

> I just discovered that with the change to wxColour to be a reference counted thing, this "fix" no longer works and I cannot see how to make it work again. There are a few Color objects created in the call to ctx.SelectObject(...) that I cannot "trap" and make sure they are non-defaults. Thus, various "default" color objects get destroyed and things crash. :(

Same problem here! Before, I could make my program succeed 90% of the
time, now it's 0%!

[Daniel Kulp]

If you use my fork/branch:

https://github.com/dkulp/wxWidgets/tree/xlights_fixes

Then it will work again.   I’ve made all the GDI objects PROPERLY reference count using atomics which fixes this and a whole slew of other reference count issues on multiple threads.   Unfortunately, the wxWidgets folks are more interested in hacking around the underlying problem (non-thread safe reference counting) then actually fixing it so no idea if/when any “official” fix will be made.   I’ve just resolved myself to having to maintain a fork of wxWidgets.   

-- 
Daniel Kulp
d...@kulp.com - http://dankulp.com/blog

[Frédéric]

> If you use my fork/branch:
> https://github.com/dkulp/wxWidgets/tree/xlights_fixes
>
> Then it will work again. I’ve made all the GDI objects PROPERLY reference count using atomics which
>fixes this and a whole slew of other reference count issues on multiple threads. Unfortunately, the
>wxWidgets folks are more interested in hacking around the underlying problem (non-thread safe reference
>counting) then actually fixing it so no idea if/when any “official” fix will be made. I’ve just resolved myself to
>having to maintain a fork of wxWidgets.

Thanks. I do not know if I will use your fork as I am already updating
my build regularly for bug fixes and it is already a lot of work.
Currently I have a fallback solution: I can use ImageMagick. But it is
very slow as I cannot really run it in parallel (the overhead is not
worth it). My program is like a script so it is not crucial to what I
do but when the wxWidgets version worked (90% of the time), it was
really quick. When it failed, I just reran it.

It would be better to have your fix in master. However, I guess one
issue is that std::atomic is C++11 and wxWidgets probably still
supports C++03. This makes atomic variable accessible only via a
library like boost.

F

[Daniel Kulp]

On Nov 6, 2018, at 7:57 AM, Frédéric <ufos...@gmail.com> wrote:

It would be better to have your fix in master. However, I guess one
issue is that std::atomic is C++11 and wxWidgets probably still
supports C++03. This makes atomic variable accessible only via a
library like boost.

They do have a wx/atomic.h file, I just haven’t moved to using it since it doesn’t look like they care.  Not worth my time since it will only live in my fork.