mac_frames::CpuFrames keeps a single CVPixelBufferRef and rewrites it for every decoded frame (ensure_pixel_buffer only reallocates when the dimensions change). The compositor then wraps that buffer as Metal textures and reads it.
This is correct today, but for a reason that is nowhere written down next to it: Compositor::rgb_to_nv12 ends with self.sync(), a full waitUntilCompleted on every frame. By the time the decoder overwrites the buffer for frame N+1, the GPU has finished reading it for frame N.
So the synchronisation that costs 40 % of the export's wall clock (gpu.wait, 7.5 s of an 18.7 s walk — see #583's profile) is also the only thing preventing a data race.
Why this matters now: the obvious next optimisation is to stop waiting per frame and let the GPU run one or more frames behind. Anyone who does that reintroduces exactly the bug 18eb7fdf ("do not reuse memory the encoder is still reading") fixed on Linux — except here it is the decoder's buffer being overwritten under the GPU, and it will show up as intermittent tearing or a frame from the wrong time, not as a crash.
Two things worth doing, independent of whether anyone pipelines:
- Write the invariant down in
CpuFrames, pointing at the sync() that upholds it.
- Give
CpuFrames a ring of pixel buffers deep enough for the frames in flight, so the invariant stops depending on a sync() in a different file.
Note this only affects the software-decode path — which, since #583, is the default for H.264 8-bit exports, so it is now the common path rather than the rare one.
mac_frames::CpuFrameskeeps a singleCVPixelBufferRefand rewrites it for every decoded frame (ensure_pixel_bufferonly reallocates when the dimensions change). The compositor then wraps that buffer as Metal textures and reads it.This is correct today, but for a reason that is nowhere written down next to it:
Compositor::rgb_to_nv12ends withself.sync(), a fullwaitUntilCompletedon every frame. By the time the decoder overwrites the buffer for frame N+1, the GPU has finished reading it for frame N.So the synchronisation that costs 40 % of the export's wall clock (
gpu.wait, 7.5 s of an 18.7 s walk — see #583's profile) is also the only thing preventing a data race.Why this matters now: the obvious next optimisation is to stop waiting per frame and let the GPU run one or more frames behind. Anyone who does that reintroduces exactly the bug
18eb7fdf("do not reuse memory the encoder is still reading") fixed on Linux — except here it is the decoder's buffer being overwritten under the GPU, and it will show up as intermittent tearing or a frame from the wrong time, not as a crash.Two things worth doing, independent of whether anyone pipelines:
CpuFrames, pointing at thesync()that upholds it.CpuFramesa ring of pixel buffers deep enough for the frames in flight, so the invariant stops depending on async()in a different file.Note this only affects the software-decode path — which, since #583, is the default for H.264 8-bit exports, so it is now the common path rather than the rare one.