VapourSynth-BM3D icon indicating copy to clipboard operation
VapourSynth-BM3D copied to clipboard

Fix crash constructing BM3D_FilterData, data race with std::unordered_map

Open ceaglest opened this issue 2 years ago • 0 comments

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.

Thread 16 Crashed:
0   libfftw3f.3.dylib             	0x000000010516b109 fftwf_mkapiplan + 338
1   libfftw3f.3.dylib             	0x000000010516da63 fftwf_plan_many_r2r + 225
2   libfftw3f.3.dylib             	0x000000010516db5a fftwf_plan_r2r + 39
3   libfftw3f.3.dylib             	0x000000010516db2e fftwf_plan_r2r_3d + 72
4   libbm3d.dylib                 	0x000000010505f383 BM3D_FilterData::BM3D_FilterData(bool, double, int, int, double) + 643
5   libbm3d.dylib                 	0x0000000105061419 BM3D_Data_Base::init_filter_data() + 601
6   libbm3d.dylib                 	0x0000000105070cf8 BM3D_Basic_Data::arguments_process(VSMap const*, VSMap*) + 120
7   libbm3d.dylib                 	0x000000010508c12a BM3D_Basic_Create(VSMap const*, VSMap*, void*, VSCore*, VSAPI const*) + 138
8   libvapoursynth.dylib          	0x00000001046bf822 VSPluginFunction::invoke(VSMap const&) + 844
...

Thread 17:
0   libfftw3f.3.dylib             	0x00000001050b3260 dotile_buf + 89
...
24  libfftw3f.3.dylib             	0x000000010516b0f9 fftwf_mkapiplan + 322
25  libfftw3f.3.dylib             	0x000000010516da63 fftwf_plan_many_r2r + 225
26  libfftw3f.3.dylib             	0x000000010516db5a fftwf_plan_r2r + 39
27  libfftw3f.3.dylib             	0x000000010516db2e fftwf_plan_r2r_3d + 72
28  libbm3d.dylib                 	0x000000010505f383 BM3D_FilterData::BM3D_FilterData(bool, double, int, int, double) + 643
29  libbm3d.dylib                 	0x0000000105061419 BM3D_Data_Base::init_filter_data() + 601
30  libbm3d.dylib                 	0x0000000105070cf8 BM3D_Basic_Data::arguments_process(VSMap const*, VSMap*) + 120
31  libbm3d.dylib                 	0x000000010508c12a BM3D_Basic_Create(VSMap const*, VSMap*, void*, VSCore*, VSAPI const*) + 138
32  libvapoursynth.dylib          	0x00000001046bf822 VSPluginFunction::invoke(VSMap const&) + 844
...

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 to BM3D_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 https://github.com/HomeOfVapourSynthEvolution/VapourSynth-BM3D/commit/9b4c1065e2a8783e02837f5ed2aa4efc702c7f21 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 prefer std::shared_mutex but it is only available in C++17 and I consider upgrading out of scope for this PR.

ceaglest avatar Jun 07 '22 06:06 ceaglest