Summary
MonitorStream runs two threads: the command_processor thread (checkCommandQueue → processCommand) and the main streaming loop in runStream(). processCommand mutates a set of plain (non-atomic, unguarded) member fields that the main loop reads on every iteration, with no mutex or atomics between them. This is a data race (undefined behavior under the C++ memory model), independent of any single symptom.
Affected fields (written by the command thread, read by the stream loop)
pausedreplay_ratescalezoom, x, ytemp_read_index, temp_write_index, temp_image_buffer_count- the seek/
send_frame-related state
processCommand sets these (e.g. paused = true, replay_rate = …, scale = …) on the command thread, while runStream() reads them to decide what/when to send.
Why this matters
In practice it usually "works" on x86 because aligned word reads/writes are atomic-ish and the values are individually small, but it is still UB: there are no ordering or visibility guarantees, and a torn or stale read of the buffer indices can produce wrong playback behavior. It also means tools like TSan will flag it.
Context
This came out of #4936 / #4938. That work fixed the concrete crash:
- a divide-by-zero (SIGFPE) when
temp_image_buffer_count was read as 0 during startup — guarded with a MonitorStreamBufferLevel() helper, and
- the command thread being started before the buffer state was initialised — now deferred until after init.
Those address the startup window. This issue tracks the ongoing concurrent access to the shared playback state, which is broader and riskier to change, so it is intentionally split out rather than folded into the SIGFPE fix.
Suggested direction
- Make the simple scalars (
paused, replay_rate, scale, zoom, x, y) std::atomic, and/or
- Guard the buffer indices/count as a unit under the existing stream mutex, since they must be read consistently together.
- Run the streamer under ThreadSanitizer (
-DTSAN=ON) to enumerate the full set of racing accesses before deciding on the locking strategy.
Summary
MonitorStreamruns two threads: thecommand_processorthread (checkCommandQueue→processCommand) and the main streaming loop inrunStream().processCommandmutates a set of plain (non-atomic, unguarded) member fields that the main loop reads on every iteration, with no mutex or atomics between them. This is a data race (undefined behavior under the C++ memory model), independent of any single symptom.Affected fields (written by the command thread, read by the stream loop)
pausedreplay_ratescalezoom,x,ytemp_read_index,temp_write_index,temp_image_buffer_countsend_frame-related stateprocessCommandsets these (e.g.paused = true,replay_rate = …,scale = …) on the command thread, whilerunStream()reads them to decide what/when to send.Why this matters
In practice it usually "works" on x86 because aligned word reads/writes are atomic-ish and the values are individually small, but it is still UB: there are no ordering or visibility guarantees, and a torn or stale read of the buffer indices can produce wrong playback behavior. It also means tools like TSan will flag it.
Context
This came out of #4936 / #4938. That work fixed the concrete crash:
temp_image_buffer_countwas read as 0 during startup — guarded with aMonitorStreamBufferLevel()helper, andThose address the startup window. This issue tracks the ongoing concurrent access to the shared playback state, which is broader and riskier to change, so it is intentionally split out rather than folded into the SIGFPE fix.
Suggested direction
paused,replay_rate,scale,zoom,x,y)std::atomic, and/or-DTSAN=ON) to enumerate the full set of racing accesses before deciding on the locking strategy.