8000 bug fix: avoid OpenCV display calls on multiple threads, merge debug video windows by phlash · Pull Request #122 · floe/backscrub · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 7 commits into from
Nov 6, 2021
Merged

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

merged 7 commits into from
Nov 6, 2021

Conversation

phlash
Copy link
Collaborator
@phlash phlash commented Nov 4, 2021

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
BenBE requested changes Nov 4, 2021
View reviewed changes
Copy link
Collaborator
@BenBE BenBE left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@BenBE BenBE added the bug Something isn't working label Nov 4, 2021
@phlash
Copy link
Collaborator Author
phlash commented Nov 5, 2021

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

I'm sure we could - top right?

BenBE
BenBE requested changes Nov 5, 2021
View reviewed changes
Copy link
Collaborator
@BenBE BenBE left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
review followup: re-ordered internal type for background.cc, reduced …
7aa9410 
…lock period, removed capitalisation in help text
@phlash
Copy link
Collaborator Author
phlash commented Nov 6, 2021

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
Copy link
Collaborator
BenBE commented Nov 6, 2021

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 phlash merged commit 44f94f3 into floe:experimental Nov 6, 2021
@phlash phlash mentioned this pull request Nov 6, 2021
Merged
@phlash
Copy link
Collaborator Author
phlash commented Nov 6, 2021

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
Copy link
Collaborator
BenBE commented Nov 6, 2021

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
Copy link
Collaborator Author
phlash commented Nov 7, 2021

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

@phlash phlash mentioned this pull request Nov 7, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BenBE BenBE BenBE approved these changes

Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@phlash @BenBE
0