MRview: ProgressBar construction causes segfault

[Lestropie]

Related to fddc9a1.

Segfault at src/gui/dialog/progressbar.cpp:39:

QSurface* surface = context->surface();

context is 0x0 :-(

P.S. QT version is 5.5.0

[jdtournier]

I'm guessing this happens before the GUI is created...? Otherwise that's pretty odd...

[Lestropie]

Nope; GUI up and running, just trying to open the primary image for the connectome tool (a BufferPreload), whether using the command-line shortcut or doing it manually from the QPushButton.

[jdtournier]

Oh well... I'm not going to worry about this a great deal, we need such a radical rethink for the ProgressBar anyway...

[Lestropie]

Yeah I think we need to dig deeper into this whole QT multi-threaded thing. They say it's all single-threaded unless you explicitly do otherwise, but we've got the weirdness with the ProgressBar, and after dropping back to QT4 I'm now occasionally getting this in the connectome tool:

QPixmap: It is not safe to use pixmaps outside the GUI thread

Running mrview with GDB certainly shows more than one thread starting...

[jdtournier]

Ah yes, the multi-threading thing is one issue. The recursive paint call is another. The first is relatively easy to deal with - Qt has primitives to handle events across threads. The latter issue is more tricky though, since it really implies a redesign of the way the textures are managed (they're the only thing I can think of that currently uses a progressbar). The only way I can think of dealing with this is to somehow cause these operations to instead schedule a callback to do the work, and bail out of the paint call without swapping the buffers (so whatever was displayed stays on the screen, you don't get a partial update, which would otherwise cause nasty flickering when for example running through volumes). That or we simply don't use a progressbar while loading images to texture...

Other thing to look into is whether Qt has ways of preventing widgets from responding to update() events, which would allow us to sidestep the issue quite neatly. Definitely worth investigating...

[jdtournier]

This seems promising...

[Lestropie]

I'm aware of the multi-threading primitives, my point was that I'm getting an error about editing something outside the main thread, even though I haven't explicitly spawned any additional threads. Everything I've read says that shouldn't be happening.

Yes that does look interesting...

[jdtournier]

Well, that depends on exactly what you're doing - even innocuous calls might trigger a multi-threaded image load, for instance. And Qt applications will tend to spawn a few threads by default, but only one of them will handle GUI updates - the rest will handle IO and other potentially locking events, I guess... I certainly also get two threads on my system just running MRView, there's nothing unexpected about that.

[jdtournier]

@Lestropie, I was just thinking, I've never seen the threading error you mentioned. Did you say this was specific to the connectome tool? And yet you don't start any threads at all? No hidden Image:: open() or a threaded_queue...? I can see a potential issue running a ThreadedLoop or a Thread::Queue with the GUI, since the thread responsible for handling the progressbar will not in general be the invoking thread. In fact, for the former all threads can update the progressbar... But it's pretty hard to explain your error message if everything genuinely remains single-threaded...

[Lestropie]

Fairly confident; the pixmap one in particular is constructed single-threaded in the initialise() function at connectome.cpp:2383, and modified throughout the calculate_node_colours() function at connectome.cpp:2895; neither of these could sensibly be called in a multi-threaded fashion.

Was just thinking though, RE the progress bar one: In a couple of places I construct a progress bar in the main thread but then allow worker threads to increment it, that's probably causing a problem. I'll have more of a dig.

[jdtournier]

Darn. The updatesEnabled trick might not be so simple. From the Qt5 docs:

Re-enabling updates implicitly calls update() on the widget.

Means we can't ask to prevent updates while within update() itself, that immediately causes the application to continuously update... Back to the drawing board.

[jdtournier]

please try with the latest commit on master (73b21ad), see if that fixes it...

[jdtournier]

By the way, I haven't had a chance to test this is a multi-threaded context yet. I have confirmed that everything works as expected for the single-threaded case on Qt4.8 & Qt5.5 though...

[jdtournier]

I'll close this off - we can carry on the discussion over on #298.