bug fix: avoid OpenCV display calls on multiple threads, merge debug video windows

[phlash]

These changes ensure OpenCV debug display calls (cv::imshow()) are on the main thread only, in an attempt to fix crashes experienced by others when using animated backgrounds. We also merge the separate background debug window into the main debug window (as picture-in-picture) and add keyboard controls to turn this on/off, along with FPS display, so users can grab the debug window as a clean image if required.

[BenBE]

Optional: Can we add similar pic-in-pic for the segmentation mask too?

[phlash]

Optional: Can we add similar pic-in-pic for the segmentation mask too?

I'm sure we could - top right?

[BenBE]

Please have a look to have consistent space after comma for arguments and similar style issues.
Functionality itself looks mostly good already, except for some performance/locking issues that keep the lock for too long and thus may degrade performance.

[phlash]

Please have a look to have consistent space after comma for arguments and similar style issues.

If you don't mind, I'll do that in a separate PR, which covers the wider codebase?

[BenBE]

Please have a look to have consistent space after comma for arguments and similar style issues.

If you don't mind, I'll do that in a separate PR, which covers the wider codebase?

Sure, not complains with splitting this into a separate PR.

[phlash]

Over in #124 (comment), referring to

backscrub/app/background.cc

Line 28 in 44f94f3

static void read_thread(background_t *pbkd) {

This somewhat counter-acts the reference counting done by the std::shared_ptr, thus extra care should be taken, that there is always one live reference of the original std::shared_ptr while any code of the read_thread function executes; otherwise risking UB. If you really have to pass a plain pointer here, make that point to the std::shared_ptr and copy the actual std::shared_ptr inside the routine to ensure this required local copy to ensure correct object lifetimes.

The intended behaviour was to terminate the read_thread and clean up memory when the last reference to the background object is dropped (presently at the end on main). To this end, the object is created with a deleter function that performs this task, but it wasn't being called, resulting in random crashes on exit, as the thread itself held an additional reference. This change was to prevent this erroneous behaviour. There is no risk of UB as the deleter function will always be called before the object is freed.

[BenBE]

If that's the case, we're probably better of (even if this takes some extra checks) to use a std::weak_ptr inside the thread, that needs to be grabbed explicitly before use. That way no owning reference is retained inside the thread (unless needed) and the main thread can properly delete the object upon termination.

Cf. https://en.cppreference.com/w/cpp/memory/weak_ptr/lock for an usage example.

Additionally all threads should be joined. In combination with releasing the central std::shared_ptr this might signal another condition for thread termination.

[phlash]

OK, this is done - minor change so checked in without a PR (also I forgot which repo was upstream of my local work area!)