| Message ID | 20260618123844.656396-14-barnabas.pocze@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Barnabás, Thank you for the patch. On Thu, Jun 18, 2026 at 02:38:30PM +0200, Barnabás Pőcze wrote: > The change that moved frame generation to a separate thread introduced > a race condition. After the last buffer of a request is completed, the > loop in `VirtualCameraData::processRequest()` will reference the buffer > map in the loop condition. This is problematic because concurrently the > request will be completed, then reused or destroyed. This was mostly > hidden by the fact that most application use `ReuseBuffers`, so the > buffer map nodes do not change. > > Fix that by making a copy of the (stream, buffer) pairs locally. > > Fixes: 6c251ae3ef0e ("libcamera: pipeline: virtual: Move image generation to separate thread") > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > --- > src/libcamera/pipeline/virtual/virtual.cpp | 40 +++++++++++++--------- > 1 file changed, 23 insertions(+), 17 deletions(-) > > diff --git a/src/libcamera/pipeline/virtual/virtual.cpp b/src/libcamera/pipeline/virtual/virtual.cpp > index 81d2dddab8..a7cd76e226 100644 > --- a/src/libcamera/pipeline/virtual/virtual.cpp > +++ b/src/libcamera/pipeline/virtual/virtual.cpp > @@ -130,33 +130,39 @@ VirtualCameraData::VirtualCameraData(PipelineHandler *pipe, > > void VirtualCameraData::processRequest(Request *request) > { > + std::array<std::pair<StreamConfig *, FrameBuffer *>, kMaxStream> buffers; std::inplace_vector will be nice when we'll switch to C++26. > + size_t bufferCount = 0; > + > for (const auto &[stream, buffer] : request->buffers()) { > bool found = false; > - /* map buffer and fill test patterns */ > for (auto &streamConfig : streamConfigs_) { > if (stream == &streamConfig.stream) { > - FrameMetadata &fmd = buffer->_d()->metadata(); > - > - fmd.status = FrameMetadata::Status::FrameSuccess; > - fmd.sequence = streamConfig.seq++; > - fmd.timestamp = currentTimestamp(); > - > - Span<const FrameBuffer::Plane> planes = buffer->planes(); > - for (const auto [i, p] : utils::enumerate(planes)) > - fmd.planes()[i].bytesused = p.length; > - > + buffers[bufferCount++] = { &streamConfig, buffer }; > found = true; > - > - if (streamConfig.frameGenerator->generateFrame( > - stream->configuration().size, buffer)) > - fmd.status = FrameMetadata::Status::FrameError; > - > - bufferCompleted.emit(buffer); > break; > } > } > ASSERT(found); > } I feel all this could be simplified by replacing the streamConfigs_ vector with a map. <1h later> 2 files changed, 42 insertions(+), 56 deletions(-) I'll post a patch after testing. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + > + for (size_t i = 0; i < bufferCount; i++) { > + const auto &[stream, buffer] = buffers[i]; > + FrameMetadata &fmd = buffer->_d()->metadata(); > + > + fmd.status = FrameMetadata::Status::FrameSuccess; > + fmd.sequence = stream->seq++; > + fmd.timestamp = currentTimestamp(); > + > + Span<const FrameBuffer::Plane> planes = buffer->planes(); > + for (const auto [j, p] : utils::enumerate(planes)) > + fmd.planes()[j].bytesused = p.length; > + > + if (stream->frameGenerator->generateFrame(stream->stream.configuration().size, > + buffer)) > + fmd.status = FrameMetadata::Status::FrameError; > + > + bufferCompleted.emit(buffer); > + } > } > > VirtualCameraConfiguration::VirtualCameraConfiguration(VirtualCameraData *data)
2026. 06. 23. 23:06 keltezéssel, Laurent Pinchart írta: > Hi Barnabás, > > Thank you for the patch. > > On Thu, Jun 18, 2026 at 02:38:30PM +0200, Barnabás Pőcze wrote: >> The change that moved frame generation to a separate thread introduced >> a race condition. After the last buffer of a request is completed, the >> loop in `VirtualCameraData::processRequest()` will reference the buffer >> map in the loop condition. This is problematic because concurrently the >> request will be completed, then reused or destroyed. This was mostly >> hidden by the fact that most application use `ReuseBuffers`, so the >> buffer map nodes do not change. >> >> Fix that by making a copy of the (stream, buffer) pairs locally. >> >> Fixes: 6c251ae3ef0e ("libcamera: pipeline: virtual: Move image generation to separate thread") >> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> >> --- >> src/libcamera/pipeline/virtual/virtual.cpp | 40 +++++++++++++--------- >> 1 file changed, 23 insertions(+), 17 deletions(-) >> >> diff --git a/src/libcamera/pipeline/virtual/virtual.cpp b/src/libcamera/pipeline/virtual/virtual.cpp >> index 81d2dddab8..a7cd76e226 100644 >> --- a/src/libcamera/pipeline/virtual/virtual.cpp >> +++ b/src/libcamera/pipeline/virtual/virtual.cpp >> @@ -130,33 +130,39 @@ VirtualCameraData::VirtualCameraData(PipelineHandler *pipe, >> >> void VirtualCameraData::processRequest(Request *request) >> { >> + std::array<std::pair<StreamConfig *, FrameBuffer *>, kMaxStream> buffers; > > std::inplace_vector will be nice when we'll switch to C++26. > >> + size_t bufferCount = 0; >> + >> for (const auto &[stream, buffer] : request->buffers()) { >> bool found = false; >> - /* map buffer and fill test patterns */ >> for (auto &streamConfig : streamConfigs_) { >> if (stream == &streamConfig.stream) { >> - FrameMetadata &fmd = buffer->_d()->metadata(); >> - >> - fmd.status = FrameMetadata::Status::FrameSuccess; >> - fmd.sequence = streamConfig.seq++; >> - fmd.timestamp = currentTimestamp(); >> - >> - Span<const FrameBuffer::Plane> planes = buffer->planes(); >> - for (const auto [i, p] : utils::enumerate(planes)) >> - fmd.planes()[i].bytesused = p.length; >> - >> + buffers[bufferCount++] = { &streamConfig, buffer }; >> found = true; >> - >> - if (streamConfig.frameGenerator->generateFrame( >> - stream->configuration().size, buffer)) >> - fmd.status = FrameMetadata::Status::FrameError; >> - >> - bufferCompleted.emit(buffer); >> break; >> } >> } >> ASSERT(found); >> } > > I feel all this could be simplified by replacing the streamConfigs_ > vector with a map. Hmm... yes, I think it's enough to inherit `StreamConfig` from `Stream`, that should work even with the vector, or have you gone in another way? > > <1h later> > > 2 files changed, 42 insertions(+), 56 deletions(-) > > I'll post a patch after testing. > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >> + >> + for (size_t i = 0; i < bufferCount; i++) { >> + const auto &[stream, buffer] = buffers[i]; >> + FrameMetadata &fmd = buffer->_d()->metadata(); >> + >> + fmd.status = FrameMetadata::Status::FrameSuccess; >> + fmd.sequence = stream->seq++; >> + fmd.timestamp = currentTimestamp(); >> + >> + Span<const FrameBuffer::Plane> planes = buffer->planes(); >> + for (const auto [j, p] : utils::enumerate(planes)) >> + fmd.planes()[j].bytesused = p.length; >> + >> + if (stream->frameGenerator->generateFrame(stream->stream.configuration().size, >> + buffer)) >> + fmd.status = FrameMetadata::Status::FrameError; >> + >> + bufferCompleted.emit(buffer); >> + } >> } >> >> VirtualCameraConfiguration::VirtualCameraConfiguration(VirtualCameraData *data) >
diff --git a/src/libcamera/pipeline/virtual/virtual.cpp b/src/libcamera/pipeline/virtual/virtual.cpp index 81d2dddab8..a7cd76e226 100644 --- a/src/libcamera/pipeline/virtual/virtual.cpp +++ b/src/libcamera/pipeline/virtual/virtual.cpp @@ -130,33 +130,39 @@ VirtualCameraData::VirtualCameraData(PipelineHandler *pipe, void VirtualCameraData::processRequest(Request *request) { + std::array<std::pair<StreamConfig *, FrameBuffer *>, kMaxStream> buffers; + size_t bufferCount = 0; + for (const auto &[stream, buffer] : request->buffers()) { bool found = false; - /* map buffer and fill test patterns */ for (auto &streamConfig : streamConfigs_) { if (stream == &streamConfig.stream) { - FrameMetadata &fmd = buffer->_d()->metadata(); - - fmd.status = FrameMetadata::Status::FrameSuccess; - fmd.sequence = streamConfig.seq++; - fmd.timestamp = currentTimestamp(); - - Span<const FrameBuffer::Plane> planes = buffer->planes(); - for (const auto [i, p] : utils::enumerate(planes)) - fmd.planes()[i].bytesused = p.length; - + buffers[bufferCount++] = { &streamConfig, buffer }; found = true; - - if (streamConfig.frameGenerator->generateFrame( - stream->configuration().size, buffer)) - fmd.status = FrameMetadata::Status::FrameError; - - bufferCompleted.emit(buffer); break; } } ASSERT(found); } + + for (size_t i = 0; i < bufferCount; i++) { + const auto &[stream, buffer] = buffers[i]; + FrameMetadata &fmd = buffer->_d()->metadata(); + + fmd.status = FrameMetadata::Status::FrameSuccess; + fmd.sequence = stream->seq++; + fmd.timestamp = currentTimestamp(); + + Span<const FrameBuffer::Plane> planes = buffer->planes(); + for (const auto [j, p] : utils::enumerate(planes)) + fmd.planes()[j].bytesused = p.length; + + if (stream->frameGenerator->generateFrame(stream->stream.configuration().size, + buffer)) + fmd.status = FrameMetadata::Status::FrameError; + + bufferCompleted.emit(buffer); + } } VirtualCameraConfiguration::VirtualCameraConfiguration(VirtualCameraData *data)
The change that moved frame generation to a separate thread introduced a race condition. After the last buffer of a request is completed, the loop in `VirtualCameraData::processRequest()` will reference the buffer map in the loop condition. This is problematic because concurrently the request will be completed, then reused or destroyed. This was mostly hidden by the fact that most application use `ReuseBuffers`, so the buffer map nodes do not change. Fix that by making a copy of the (stream, buffer) pairs locally. Fixes: 6c251ae3ef0e ("libcamera: pipeline: virtual: Move image generation to separate thread") Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> --- src/libcamera/pipeline/virtual/virtual.cpp | 40 +++++++++++++--------- 1 file changed, 23 insertions(+), 17 deletions(-)