Skip to content

Make QuadSurface::gen() safe for concurrent rendering#1075

Open
robert-kofler42 wants to merge 1 commit into
ScrollPrize:mainfrom
robert-kofler42:fix/quadsurface-gen-thread-safety
Open

Make QuadSurface::gen() safe for concurrent rendering#1075
robert-kofler42 wants to merge 1 commit into
ScrollPrize:mainfrom
robert-kofler42:fix/quadsurface-gen-thread-safety

Conversation

@robert-kofler42

@robert-kofler42 robert-kofler42 commented Jun 29, 2026

Copy link
Copy Markdown

Problem

The renderer (Render.cpp, vc_render_tifxyz) calls QuadSurface::gen() from OpenMP tile workers on a single surface instance. gen() wrote into shared mutable scratch members (_genCoordsScratch/_genNormalsScratch/_genValidScratch) and lazily built shared caches (_normalCache, _validMaskCache) with no synchronization. Concurrently this corrupts output and can crash — one thread's cv::Mat::create() reallocates a buffer another thread is mid-read. This is the vc-render-tifxyz SIGSEGV in #1046, and the thread-safety gap tracked in #1054.

Closes #1054. Closes #1046 (same root cause — backtrace points at the _normalCache warp in gen(); see analysis on that issue).

Fix

  • Scratch buffers → thread_local. Each worker reuses its own coords/normals/validity buffer across frames, keeping the per-frame allocation-reuse optimization without cross-thread sharing.
  • Lazy caches serialized with a new _cacheMutex. validMask() takes the lock for the whole accessor and returns a header copy made under it. _normalCache is built and snapshotted under the lock; the expensive warp then runs lock-free on the snapshot.
    • The snapshot is the crucial part: plain double-checked locking is not enough, because cv::Mat::create() marks the Mat non-empty before it is filled — so a second thread on the cache-hit path would warp a half-built cache (silent wrong normals, no crash).
    • Every cache-clear site (unloadPoints, unloadCaches, invalidateCache, invalidateMask) takes the same lock. Lock ordering is always _loadMutex -> _cacheMutex.

No numeric change: outputs are bit-identical to the serial path.

Test

Adds test_quadsurface_gen_concurrency: runs many gen() calls concurrently on one surface and checks each result matches a serial reference bit-for-bit.

  • Unfixed: fails 20/20 (corrupted output).
  • Fixed: passes 40/40 stress runs, and clean under ThreadSanitizer.

ThreadSanitizer note: with -DVC_ENABLE_TSAN=ON the only remaining warnings are inside validMask()'s internal #pragma omp parallel for reduction — a known GCC libgomp false positive (libgomp barriers aren't modeled by TSAN without the archer runtime). Suppress with race:libgomp.so / called_from_lib:libgomp.so.

cmake -B build -DCMAKE_BUILD_TYPE=RelWithDebInfo -DVC_TESTING=ON
cmake --build build --target test_quadsurface_gen_concurrency -j
ctest --test-dir build -R test_quadsurface_gen_concurrency --output-on-failure

The renderer (Render.cpp, vc_render_tifxyz) calls QuadSurface::gen() from
OpenMP tile workers on a single surface instance. gen() wrote into shared
mutable scratch members and lazily built shared caches (_normalCache,
_validMaskCache) without synchronization. Concurrently this corrupted output
and could crash: one thread's cv::Mat::create() reallocated a buffer another
thread was mid-read (the vc-render-tifxyz SIGSEGV, ScrollPrize#1046/ScrollPrize#1054).

Fixes:
- Move gen()'s coords/normals/validity scratch buffers from shared mutable
  members to thread_local locals. Preserves the per-frame allocation-reuse
  optimization (each thread reuses its own buffer) without cross-thread
  sharing.
- Serialize the lazy caches with a new _cacheMutex. validMask() now takes the
  lock for the whole accessor and returns a header copy made under it. The
  _normalCache is built *and snapshotted* under the lock, and the expensive
  warp then runs lock-free on the snapshot. This is the crucial part: plain
  double-checked locking is not enough, because create() marks the Mat
  non-empty before it is filled, so a second thread taking the cache-hit path
  would warp a half-built cache. Every cache-clear site (unloadPoints,
  unloadCaches, invalidateCache, invalidateMask) takes the same lock. Lock
  ordering is always _loadMutex -> _cacheMutex.

No numeric change: outputs are bit-identical to the serial path.

Adds test_quadsurface_gen_concurrency: runs many gen() calls concurrently on
one surface and checks the results match a serial reference bit-for-bit.
Unfixed it fails 20/20 (corrupted output); fixed it passes (40/40 stress
runs, and clean under ThreadSanitizer modulo libgomp's barrier
false-positives).
@vercel

vercel Bot commented Jun 29, 2026

Copy link
Copy Markdown

Someone is attempting to deploy a commit to the scroll Team on Vercel.

A member of the Team first needs to authorize it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

vc-render-tifxyz fails with sigsegv

1 participant