Fix crash constructing BM3D_FilterData, data race with std::unordered_map #29
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hi, this project is has been very helpful personally and I notice that it has amassed a lot of stars over the years. I wanted to say thank you by submitting fixes for two common issues that I have encountered with more complex pipelines.
Setup
macOS 10.15.7
BM3D - r9
vs.core.threads = 2+
1. Crash when Constructing
BM3D_FilterData
When more than one thread is used and filters are constructed per-frame (like with FrameEvaluator) there is a crash in the constructor for
BM3D_FilterData
.Problem
Execution of plans (via
fftw_execute
) is guaranteed to be thread-safe but all other calls are not. Plan creation involves mutation of the planner's in-memory "wisdom" - see FFTW 3.3.10 - Section 5.4: Thread Safety for more information.Solution
I could not figure out any effective strategy for filter instances to coordinate FFTW resource access within VapourSynth. The best solution I could come up with for now is to use a
static std::mutex
scoped toBM3D_FilterData
. Now the crash is prevented by serializing access to the planner, while plans continue to execute in parallel as before.2. Data Race with
std::unordered_map
This is a fix regarding commit 9b4c106 which introduced per-thread storage. The problem is that
std::unordered_map
is not guaranteed to be thread safe for multiple readers and writers.As a solution I introduced one
std::mutex
to protect access to each map. I would preferstd::shared_mutex
but it is only available in C++17 and I consider upgrading out of scope for this PR.