Fix T38873: Crashing on undo of ocean modifier.
ClosedPublic
Actions

Authored by Bastien Montagne (mont29) on Feb 28 2014, 10:18 PM.

Details

Reviewers

Brecht Van Lommel (brecht)
Sergey Sharybin (sergey)

Maniphest Tasks

T38873: Crashing on undo of ocean modifier.

Commits

rBSf01d19431d0e: Fix T38873: Crashing on undo of ocean modifier.
rBACf01d19431d0e: Fix T38873: Crashing on undo of ocean modifier.
rBf01d19431d0e: Fix T38873: Crashing on undo of ocean modifier.

Summary

Issue of this bug is that most part of fftw is not thread safe, only compute-intensive fftw_execute & co are threadsafe.

Since I realized smoke was affected by this issue as well, I had to add an helper fftw_threadsafe_private.h/.c file in BKE, hope this is OK.
Audaspace also uses fftw in one of its readers (AUD_BandPassReader.cpp), but this is not an issue currently since this code is disabled in CMake/scons files.

Note I also ran into another threading issue with smoke, and had to copy dm used by emit_from_derivedmesh() func,
not sure this is a nice solution but at least it works and should be reasonably safe!

Diff Detail

Event Timeline

I wonder if it would not be cleaner to have a thin wrapper around fftw, that would make it threadsafe. But since we’d like to use BLI, I’m not sure where we’d place it? /intern?

Anyway, I guess this would be for after 2.70.

If the solution is just this lock, I would instead add a LOCK_FFTW in BLI_threads.h, like there is e.g. LOCK_OPENGL. It's what other code does and requires less code.

Wrapping FFTW would be possible, I don't mind but don't think it's worth it if we can just add a mutex lock.

Much better solution to store our fftw mutex, thanks Brecht for the suggestion!

Looks good, just some suggestions for comments.

source/blender/blenkernel/intern/smoke.c
211	A comment here to indicate that smoke is using FFTW and that it's not thread safe would be good, it's not totally obvious why this is here.
1560	Can you change this comment to something like `/* copy derivedmesh for thread safety because we modify it with e.g. CDDM_calc_normals */` Using `XXX` usually indicates that something is broken and needs to be fixed.