| Message ID | 20210131224702.8838-20-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Superseded |
| Delegated to: | Laurent Pinchart |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, On Mon, Feb 01, 2021 at 12:47:01AM +0200, Laurent Pinchart wrote: > To extend the multi-stream support to runtime operation of the pipeline, > expand the converter queue to store multiple output buffers, and update > the request queuing and buffer completion handlers accordingly. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > src/libcamera/pipeline/simple/simple.cpp | 93 ++++++++++++++---------- > 1 file changed, 54 insertions(+), 39 deletions(-) > > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp > index 58e5f0d23139..55a5528611c8 100644 > --- a/src/libcamera/pipeline/simple/simple.cpp > +++ b/src/libcamera/pipeline/simple/simple.cpp > @@ -173,7 +173,7 @@ public: > > std::vector<std::unique_ptr<FrameBuffer>> converterBuffers_; > bool useConverter_; > - std::queue<FrameBuffer *> converterQueue_; > + std::queue<std::map<unsigned int, FrameBuffer *>> converterQueue_; > }; > > class SimpleCameraConfiguration : public CameraConfiguration > @@ -762,10 +762,12 @@ int SimplePipelineHandler::exportFrameBuffers(Camera *camera, Stream *stream, > * Export buffers on the converter or capture video node, depending on > * whether the converter is used or not. > */ > - if (data->useConverter_) > - return converter_->exportBuffers(0, count, buffers); > - else > + if (data->useConverter_) { > + unsigned int index = stream - &data->streams_.front(); > + return converter_->exportBuffers(index, count, buffers); > + } else { > return data->video_->exportBuffers(count, buffers); > + } > } > > int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] ControlList *controls) > @@ -830,25 +832,30 @@ void SimplePipelineHandler::stop(Camera *camera) > int SimplePipelineHandler::queueRequestDevice(Camera *camera, Request *request) > { > SimpleCameraData *data = cameraData(camera); > - Stream *stream = &data->streams_[0]; > + int ret; > > - FrameBuffer *buffer = request->findBuffer(stream); > - if (!buffer) { > - LOG(SimplePipeline, Error) > - << "Attempt to queue request with invalid stream"; > - return -ENOENT; > - } > + std::map<unsigned int, FrameBuffer *> buffers; > > - /* > - * If conversion is needed, push the buffer to the converter queue, it > - * will be handed to the converter in the capture completion handler. > - */ > - if (data->useConverter_) { > - data->converterQueue_.push(buffer); > - return 0; > + for (auto &[stream, buffer] : request->buffers()) { > + /* > + * If conversion is needed, push the buffer to the converter > + * queue, it will be handed to the converter in the capture > + * completion handler. > + */ > + if (data->useConverter_) { > + unsigned int index = stream - &data->streams_.front(); > + buffers.emplace(index, buffer); > + } else { > + ret = data->video_->queueBuffer(buffer); > + if (ret < 0) > + return ret; > + } > } > > - return data->video_->queueBuffer(buffer); > + if (data->useConverter_) > + data->converterQueue_.push(std::move(buffers)); > + > + return 0; > } > > /* ----------------------------------------------------------------------------- > @@ -1020,24 +1027,34 @@ void SimplePipelineHandler::bufferReady(FrameBuffer *buffer) > * point converting an erroneous buffer. > */ > if (buffer->metadata().status != FrameMetadata::FrameSuccess) { > - if (data->useConverter_) { > - /* Requeue the buffer for capture. */ > - data->video_->queueBuffer(buffer); > + if (!data->useConverter_) { > + /* No conversion, just complete the request. */ > + Request *request = buffer->request(); > + completeBuffer(request, buffer); > + completeRequest(request); > + return; > + } > + > + /* > + * The converter is in use. Requeue the internal buffer for > + * capture, and complete the request with all the user-facing > + * buffers. > + */ > + data->video_->queueBuffer(buffer); > > - /* > - * Get the next user-facing buffer to complete the > - * request. > - */ > - if (data->converterQueue_.empty()) > - return; > + if (data->converterQueue_.empty()) > + return; > > - buffer = data->converterQueue_.front(); > - data->converterQueue_.pop(); > + Request *request = nullptr; > + for (auto &item : data->converterQueue_.front()) { > + FrameBuffer *outputBuffer = item.second; > + request = outputBuffer->request(); > + completeBuffer(request, outputBuffer); > } > + data->converterQueue_.pop(); > > - Request *request = buffer->request(); > - completeBuffer(request, buffer); > - completeRequest(request); > + if (request) This check doesn't seem necessary, as we return early if the converterQueue_ is empty, so the loop will always run once. Is it just to appease coverity? Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > + completeRequest(request); > return; > } > > @@ -1052,10 +1069,8 @@ void SimplePipelineHandler::bufferReady(FrameBuffer *buffer) > return; > } > > - FrameBuffer *output = data->converterQueue_.front(); > + converter_->queueBuffers(buffer, data->converterQueue_.front()); > data->converterQueue_.pop(); > - > - converter_->queueBuffers(buffer, { { 0, output } }); > return; > } > > @@ -1078,10 +1093,10 @@ void SimplePipelineHandler::converterOutputDone(FrameBuffer *buffer) > { > ASSERT(activeCamera_); > > - /* Complete the request. */ > + /* Complete the buffer and the request. */ > Request *request = buffer->request(); > - completeBuffer(request, buffer); > - completeRequest(request); > + if (completeBuffer(request, buffer)) > + completeRequest(request); > } > > REGISTER_PIPELINE_HANDLER(SimplePipelineHandler)
Hi Paul, On Tue, Mar 02, 2021 at 04:09:33PM +0900, paul.elder@ideasonboard.com wrote: > On Mon, Feb 01, 2021 at 12:47:01AM +0200, Laurent Pinchart wrote: > > To extend the multi-stream support to runtime operation of the pipeline, > > expand the converter queue to store multiple output buffers, and update > > the request queuing and buffer completion handlers accordingly. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > src/libcamera/pipeline/simple/simple.cpp | 93 ++++++++++++++---------- > > 1 file changed, 54 insertions(+), 39 deletions(-) > > > > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp > > index 58e5f0d23139..55a5528611c8 100644 > > --- a/src/libcamera/pipeline/simple/simple.cpp > > +++ b/src/libcamera/pipeline/simple/simple.cpp > > @@ -173,7 +173,7 @@ public: > > > > std::vector<std::unique_ptr<FrameBuffer>> converterBuffers_; > > bool useConverter_; > > - std::queue<FrameBuffer *> converterQueue_; > > + std::queue<std::map<unsigned int, FrameBuffer *>> converterQueue_; > > }; > > > > class SimpleCameraConfiguration : public CameraConfiguration > > @@ -762,10 +762,12 @@ int SimplePipelineHandler::exportFrameBuffers(Camera *camera, Stream *stream, > > * Export buffers on the converter or capture video node, depending on > > * whether the converter is used or not. > > */ > > - if (data->useConverter_) > > - return converter_->exportBuffers(0, count, buffers); > > - else > > + if (data->useConverter_) { > > + unsigned int index = stream - &data->streams_.front(); > > + return converter_->exportBuffers(index, count, buffers); > > + } else { > > return data->video_->exportBuffers(count, buffers); > > + } > > } > > > > int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] ControlList *controls) > > @@ -830,25 +832,30 @@ void SimplePipelineHandler::stop(Camera *camera) > > int SimplePipelineHandler::queueRequestDevice(Camera *camera, Request *request) > > { > > SimpleCameraData *data = cameraData(camera); > > - Stream *stream = &data->streams_[0]; > > + int ret; > > > > - FrameBuffer *buffer = request->findBuffer(stream); > > - if (!buffer) { > > - LOG(SimplePipeline, Error) > > - << "Attempt to queue request with invalid stream"; > > - return -ENOENT; > > - } > > + std::map<unsigned int, FrameBuffer *> buffers; > > > > - /* > > - * If conversion is needed, push the buffer to the converter queue, it > > - * will be handed to the converter in the capture completion handler. > > - */ > > - if (data->useConverter_) { > > - data->converterQueue_.push(buffer); > > - return 0; > > + for (auto &[stream, buffer] : request->buffers()) { > > + /* > > + * If conversion is needed, push the buffer to the converter > > + * queue, it will be handed to the converter in the capture > > + * completion handler. > > + */ > > + if (data->useConverter_) { > > + unsigned int index = stream - &data->streams_.front(); > > + buffers.emplace(index, buffer); > > + } else { > > + ret = data->video_->queueBuffer(buffer); > > + if (ret < 0) > > + return ret; > > + } > > } > > > > - return data->video_->queueBuffer(buffer); > > + if (data->useConverter_) > > + data->converterQueue_.push(std::move(buffers)); > > + > > + return 0; > > } > > > > /* ----------------------------------------------------------------------------- > > @@ -1020,24 +1027,34 @@ void SimplePipelineHandler::bufferReady(FrameBuffer *buffer) > > * point converting an erroneous buffer. > > */ > > if (buffer->metadata().status != FrameMetadata::FrameSuccess) { > > - if (data->useConverter_) { > > - /* Requeue the buffer for capture. */ > > - data->video_->queueBuffer(buffer); > > + if (!data->useConverter_) { > > + /* No conversion, just complete the request. */ > > + Request *request = buffer->request(); > > + completeBuffer(request, buffer); > > + completeRequest(request); > > + return; > > + } > > + > > + /* > > + * The converter is in use. Requeue the internal buffer for > > + * capture, and complete the request with all the user-facing > > + * buffers. > > + */ > > + data->video_->queueBuffer(buffer); > > > > - /* > > - * Get the next user-facing buffer to complete the > > - * request. > > - */ > > - if (data->converterQueue_.empty()) > > - return; > > + if (data->converterQueue_.empty()) > > + return; > > > > - buffer = data->converterQueue_.front(); > > - data->converterQueue_.pop(); > > + Request *request = nullptr; > > + for (auto &item : data->converterQueue_.front()) { > > + FrameBuffer *outputBuffer = item.second; > > + request = outputBuffer->request(); > > + completeBuffer(request, outputBuffer); > > } > > + data->converterQueue_.pop(); > > > > - Request *request = buffer->request(); > > - completeBuffer(request, buffer); > > - completeRequest(request); > > + if (request) > > This check doesn't seem necessary, as we return early if the > converterQueue_ is empty, so the loop will always run once. Is it just > to appease coverity? You're right, I'll drop that. > Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > > > + completeRequest(request); > > return; > > } > > > > @@ -1052,10 +1069,8 @@ void SimplePipelineHandler::bufferReady(FrameBuffer *buffer) > > return; > > } > > > > - FrameBuffer *output = data->converterQueue_.front(); > > + converter_->queueBuffers(buffer, data->converterQueue_.front()); > > data->converterQueue_.pop(); > > - > > - converter_->queueBuffers(buffer, { { 0, output } }); > > return; > > } > > > > @@ -1078,10 +1093,10 @@ void SimplePipelineHandler::converterOutputDone(FrameBuffer *buffer) > > { > > ASSERT(activeCamera_); > > > > - /* Complete the request. */ > > + /* Complete the buffer and the request. */ > > Request *request = buffer->request(); > > - completeBuffer(request, buffer); > > - completeRequest(request); > > + if (completeBuffer(request, buffer)) > > + completeRequest(request); > > } > > > > REGISTER_PIPELINE_HANDLER(SimplePipelineHandler)
Hi again, On Tue, Mar 02, 2021 at 12:34:10PM +0200, Laurent Pinchart wrote: > On Tue, Mar 02, 2021 at 04:09:33PM +0900, paul.elder@ideasonboard.com wrote: > > On Mon, Feb 01, 2021 at 12:47:01AM +0200, Laurent Pinchart wrote: > > > To extend the multi-stream support to runtime operation of the pipeline, > > > expand the converter queue to store multiple output buffers, and update > > > the request queuing and buffer completion handlers accordingly. > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > --- > > > src/libcamera/pipeline/simple/simple.cpp | 93 ++++++++++++++---------- > > > 1 file changed, 54 insertions(+), 39 deletions(-) > > > > > > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp > > > index 58e5f0d23139..55a5528611c8 100644 > > > --- a/src/libcamera/pipeline/simple/simple.cpp > > > +++ b/src/libcamera/pipeline/simple/simple.cpp > > > @@ -173,7 +173,7 @@ public: > > > > > > std::vector<std::unique_ptr<FrameBuffer>> converterBuffers_; > > > bool useConverter_; > > > - std::queue<FrameBuffer *> converterQueue_; > > > + std::queue<std::map<unsigned int, FrameBuffer *>> converterQueue_; > > > }; > > > > > > class SimpleCameraConfiguration : public CameraConfiguration > > > @@ -762,10 +762,12 @@ int SimplePipelineHandler::exportFrameBuffers(Camera *camera, Stream *stream, > > > * Export buffers on the converter or capture video node, depending on > > > * whether the converter is used or not. > > > */ > > > - if (data->useConverter_) > > > - return converter_->exportBuffers(0, count, buffers); > > > - else > > > + if (data->useConverter_) { > > > + unsigned int index = stream - &data->streams_.front(); > > > + return converter_->exportBuffers(index, count, buffers); > > > + } else { > > > return data->video_->exportBuffers(count, buffers); > > > + } > > > } > > > > > > int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] ControlList *controls) > > > @@ -830,25 +832,30 @@ void SimplePipelineHandler::stop(Camera *camera) > > > int SimplePipelineHandler::queueRequestDevice(Camera *camera, Request *request) > > > { > > > SimpleCameraData *data = cameraData(camera); > > > - Stream *stream = &data->streams_[0]; > > > + int ret; > > > > > > - FrameBuffer *buffer = request->findBuffer(stream); > > > - if (!buffer) { > > > - LOG(SimplePipeline, Error) > > > - << "Attempt to queue request with invalid stream"; > > > - return -ENOENT; > > > - } > > > + std::map<unsigned int, FrameBuffer *> buffers; > > > > > > - /* > > > - * If conversion is needed, push the buffer to the converter queue, it > > > - * will be handed to the converter in the capture completion handler. > > > - */ > > > - if (data->useConverter_) { > > > - data->converterQueue_.push(buffer); > > > - return 0; > > > + for (auto &[stream, buffer] : request->buffers()) { > > > + /* > > > + * If conversion is needed, push the buffer to the converter > > > + * queue, it will be handed to the converter in the capture > > > + * completion handler. > > > + */ > > > + if (data->useConverter_) { > > > + unsigned int index = stream - &data->streams_.front(); > > > + buffers.emplace(index, buffer); > > > + } else { > > > + ret = data->video_->queueBuffer(buffer); > > > + if (ret < 0) > > > + return ret; > > > + } > > > } > > > > > > - return data->video_->queueBuffer(buffer); > > > + if (data->useConverter_) > > > + data->converterQueue_.push(std::move(buffers)); > > > + > > > + return 0; > > > } > > > > > > /* ----------------------------------------------------------------------------- > > > @@ -1020,24 +1027,34 @@ void SimplePipelineHandler::bufferReady(FrameBuffer *buffer) > > > * point converting an erroneous buffer. > > > */ > > > if (buffer->metadata().status != FrameMetadata::FrameSuccess) { > > > - if (data->useConverter_) { > > > - /* Requeue the buffer for capture. */ > > > - data->video_->queueBuffer(buffer); > > > + if (!data->useConverter_) { > > > + /* No conversion, just complete the request. */ > > > + Request *request = buffer->request(); > > > + completeBuffer(request, buffer); > > > + completeRequest(request); > > > + return; > > > + } > > > + > > > + /* > > > + * The converter is in use. Requeue the internal buffer for > > > + * capture, and complete the request with all the user-facing > > > + * buffers. > > > + */ > > > + data->video_->queueBuffer(buffer); > > > > > > - /* > > > - * Get the next user-facing buffer to complete the > > > - * request. > > > - */ > > > - if (data->converterQueue_.empty()) > > > - return; > > > + if (data->converterQueue_.empty()) > > > + return; > > > > > > - buffer = data->converterQueue_.front(); > > > - data->converterQueue_.pop(); > > > + Request *request = nullptr; > > > + for (auto &item : data->converterQueue_.front()) { > > > + FrameBuffer *outputBuffer = item.second; > > > + request = outputBuffer->request(); > > > + completeBuffer(request, outputBuffer); > > > } > > > + data->converterQueue_.pop(); > > > > > > - Request *request = buffer->request(); > > > - completeBuffer(request, buffer); > > > - completeRequest(request); > > > + if (request) > > > > This check doesn't seem necessary, as we return early if the > > converterQueue_ is empty, so the loop will always run once. Is it just > > to appease coverity? > > You're right, I'll drop that. Actually, we're not looping over data->converterQueue_, but over converterQueue_.front(). While it should never be empty, I think it will be difficult for compilers (and coverity) to know that, and a check here can also act as a bit of defensive programming. I think I'd prefer keeping the check. > > Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > > > > > + completeRequest(request); > > > return; > > > } > > > > > > @@ -1052,10 +1069,8 @@ void SimplePipelineHandler::bufferReady(FrameBuffer *buffer) > > > return; > > > } > > > > > > - FrameBuffer *output = data->converterQueue_.front(); > > > + converter_->queueBuffers(buffer, data->converterQueue_.front()); > > > data->converterQueue_.pop(); > > > - > > > - converter_->queueBuffers(buffer, { { 0, output } }); > > > return; > > > } > > > > > > @@ -1078,10 +1093,10 @@ void SimplePipelineHandler::converterOutputDone(FrameBuffer *buffer) > > > { > > > ASSERT(activeCamera_); > > > > > > - /* Complete the request. */ > > > + /* Complete the buffer and the request. */ > > > Request *request = buffer->request(); > > > - completeBuffer(request, buffer); > > > - completeRequest(request); > > > + if (completeBuffer(request, buffer)) > > > + completeRequest(request); > > > } > > > > > > REGISTER_PIPELINE_HANDLER(SimplePipelineHandler)
On 02/03/2021 10:39, Laurent Pinchart wrote: > Hi again, > > On Tue, Mar 02, 2021 at 12:34:10PM +0200, Laurent Pinchart wrote: >> On Tue, Mar 02, 2021 at 04:09:33PM +0900, paul.elder@ideasonboard.com wrote: >>> On Mon, Feb 01, 2021 at 12:47:01AM +0200, Laurent Pinchart wrote: >>>> To extend the multi-stream support to runtime operation of the pipeline, >>>> expand the converter queue to store multiple output buffers, and update >>>> the request queuing and buffer completion handlers accordingly. >>>> >>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >>>> --- >>>> src/libcamera/pipeline/simple/simple.cpp | 93 ++++++++++++++---------- >>>> 1 file changed, 54 insertions(+), 39 deletions(-) >>>> >>>> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp >>>> index 58e5f0d23139..55a5528611c8 100644 >>>> --- a/src/libcamera/pipeline/simple/simple.cpp >>>> +++ b/src/libcamera/pipeline/simple/simple.cpp >>>> @@ -173,7 +173,7 @@ public: >>>> >>>> std::vector<std::unique_ptr<FrameBuffer>> converterBuffers_; >>>> bool useConverter_; >>>> - std::queue<FrameBuffer *> converterQueue_; >>>> + std::queue<std::map<unsigned int, FrameBuffer *>> converterQueue_; >>>> }; >>>> >>>> class SimpleCameraConfiguration : public CameraConfiguration >>>> @@ -762,10 +762,12 @@ int SimplePipelineHandler::exportFrameBuffers(Camera *camera, Stream *stream, >>>> * Export buffers on the converter or capture video node, depending on >>>> * whether the converter is used or not. >>>> */ >>>> - if (data->useConverter_) >>>> - return converter_->exportBuffers(0, count, buffers); >>>> - else >>>> + if (data->useConverter_) { >>>> + unsigned int index = stream - &data->streams_.front(); >>>> + return converter_->exportBuffers(index, count, buffers); >>>> + } else { >>>> return data->video_->exportBuffers(count, buffers); >>>> + } >>>> } >>>> >>>> int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] ControlList *controls) >>>> @@ -830,25 +832,30 @@ void SimplePipelineHandler::stop(Camera *camera) >>>> int SimplePipelineHandler::queueRequestDevice(Camera *camera, Request *request) >>>> { >>>> SimpleCameraData *data = cameraData(camera); >>>> - Stream *stream = &data->streams_[0]; >>>> + int ret; >>>> >>>> - FrameBuffer *buffer = request->findBuffer(stream); >>>> - if (!buffer) { >>>> - LOG(SimplePipeline, Error) >>>> - << "Attempt to queue request with invalid stream"; >>>> - return -ENOENT; We used to validate the stream here... >>>> - } >>>> + std::map<unsigned int, FrameBuffer *> buffers; >>>> >>>> - /* >>>> - * If conversion is needed, push the buffer to the converter queue, it >>>> - * will be handed to the converter in the capture completion handler. >>>> - */ >>>> - if (data->useConverter_) { >>>> - data->converterQueue_.push(buffer); >>>> - return 0; >>>> + for (auto &[stream, buffer] : request->buffers()) { >>>> + /* >>>> + * If conversion is needed, push the buffer to the converter >>>> + * queue, it will be handed to the converter in the capture >>>> + * completion handler. >>>> + */ >>>> + if (data->useConverter_) { >>>> + unsigned int index = stream - &data->streams_.front(); Should we have a helper to convert from stream to index, which includes a check to validate that it is within streams_.size() ? That would add some safety to here, and the usage above in exportFrameBuffers(). >>>> + buffers.emplace(index, buffer); >>>> + } else { >>>> + ret = data->video_->queueBuffer(buffer); >>>> + if (ret < 0) >>>> + return ret; >>>> + } >>>> } >>>> >>>> - return data->video_->queueBuffer(buffer); >>>> + if (data->useConverter_) >>>> + data->converterQueue_.push(std::move(buffers)); >>>> + >>>> + return 0; >>>> } >>>> >>>> /* ----------------------------------------------------------------------------- >>>> @@ -1020,24 +1027,34 @@ void SimplePipelineHandler::bufferReady(FrameBuffer *buffer) >>>> * point converting an erroneous buffer. >>>> */ >>>> if (buffer->metadata().status != FrameMetadata::FrameSuccess) { >>>> - if (data->useConverter_) { >>>> - /* Requeue the buffer for capture. */ >>>> - data->video_->queueBuffer(buffer); >>>> + if (!data->useConverter_) { >>>> + /* No conversion, just complete the request. */ >>>> + Request *request = buffer->request(); >>>> + completeBuffer(request, buffer); >>>> + completeRequest(request); >>>> + return; >>>> + } >>>> + >>>> + /* >>>> + * The converter is in use. Requeue the internal buffer for >>>> + * capture, and complete the request with all the user-facing >>>> + * buffers. >>>> + */ >>>> + data->video_->queueBuffer(buffer); Does this incorrectly requeue buffers if we're stopping? (I.e. if we were 'FrameCancelled' or such? >>>> >>>> - /* >>>> - * Get the next user-facing buffer to complete the >>>> - * request. >>>> - */ >>>> - if (data->converterQueue_.empty()) >>>> - return; >>>> + if (data->converterQueue_.empty()) >>>> + return; >>>> >>>> - buffer = data->converterQueue_.front(); >>>> - data->converterQueue_.pop(); >>>> + Request *request = nullptr; >>>> + for (auto &item : data->converterQueue_.front()) { >>>> + FrameBuffer *outputBuffer = item.second; >>>> + request = outputBuffer->request(); >>>> + completeBuffer(request, outputBuffer); >>>> } >>>> + data->converterQueue_.pop(); >>>> >>>> - Request *request = buffer->request(); >>>> - completeBuffer(request, buffer); >>>> - completeRequest(request); >>>> + if (request) >>> >>> This check doesn't seem necessary, as we return early if the >>> converterQueue_ is empty, so the loop will always run once. Is it just >>> to appease coverity? >> >> You're right, I'll drop that. > > Actually, we're not looping over data->converterQueue_, but over > converterQueue_.front(). While it should never be empty, I think it will > be difficult for compilers (and coverity) to know that, and a check here > can also act as a bit of defensive programming. I think I'd prefer > keeping the check. I presume at this point no buffer has been passed to any convertor, so there can't be anything happening in parallel at this point. I don't think there can be so: Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >>> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> >>> >>>> + completeRequest(request); >>>> return; >>>> } >>>> >>>> @@ -1052,10 +1069,8 @@ void SimplePipelineHandler::bufferReady(FrameBuffer *buffer) >>>> return; >>>> } >>>> >>>> - FrameBuffer *output = data->converterQueue_.front(); >>>> + converter_->queueBuffers(buffer, data->converterQueue_.front()); >>>> data->converterQueue_.pop(); >>>> - >>>> - converter_->queueBuffers(buffer, { { 0, output } }); >>>> return; >>>> } >>>> >>>> @@ -1078,10 +1093,10 @@ void SimplePipelineHandler::converterOutputDone(FrameBuffer *buffer) >>>> { >>>> ASSERT(activeCamera_); >>>> >>>> - /* Complete the request. */ >>>> + /* Complete the buffer and the request. */ >>>> Request *request = buffer->request(); >>>> - completeBuffer(request, buffer); >>>> - completeRequest(request); >>>> + if (completeBuffer(request, buffer)) >>>> + completeRequest(request);>>>> } >>>> >>>> REGISTER_PIPELINE_HANDLER(SimplePipelineHandler) >
Hi Kieran, On Tue, Mar 02, 2021 at 11:06:13AM +0000, Kieran Bingham wrote: > On 02/03/2021 10:39, Laurent Pinchart wrote: > > On Tue, Mar 02, 2021 at 12:34:10PM +0200, Laurent Pinchart wrote: > >> On Tue, Mar 02, 2021 at 04:09:33PM +0900, paul.elder@ideasonboard.com wrote: > >>> On Mon, Feb 01, 2021 at 12:47:01AM +0200, Laurent Pinchart wrote: > >>>> To extend the multi-stream support to runtime operation of the pipeline, > >>>> expand the converter queue to store multiple output buffers, and update > >>>> the request queuing and buffer completion handlers accordingly. > >>>> > >>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >>>> --- > >>>> src/libcamera/pipeline/simple/simple.cpp | 93 ++++++++++++++---------- > >>>> 1 file changed, 54 insertions(+), 39 deletions(-) > >>>> > >>>> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp > >>>> index 58e5f0d23139..55a5528611c8 100644 > >>>> --- a/src/libcamera/pipeline/simple/simple.cpp > >>>> +++ b/src/libcamera/pipeline/simple/simple.cpp > >>>> @@ -173,7 +173,7 @@ public: > >>>> > >>>> std::vector<std::unique_ptr<FrameBuffer>> converterBuffers_; > >>>> bool useConverter_; > >>>> - std::queue<FrameBuffer *> converterQueue_; > >>>> + std::queue<std::map<unsigned int, FrameBuffer *>> converterQueue_; > >>>> }; > >>>> > >>>> class SimpleCameraConfiguration : public CameraConfiguration > >>>> @@ -762,10 +762,12 @@ int SimplePipelineHandler::exportFrameBuffers(Camera *camera, Stream *stream, > >>>> * Export buffers on the converter or capture video node, depending on > >>>> * whether the converter is used or not. > >>>> */ > >>>> - if (data->useConverter_) > >>>> - return converter_->exportBuffers(0, count, buffers); > >>>> - else > >>>> + if (data->useConverter_) { > >>>> + unsigned int index = stream - &data->streams_.front(); > >>>> + return converter_->exportBuffers(index, count, buffers); > >>>> + } else { > >>>> return data->video_->exportBuffers(count, buffers); > >>>> + } > >>>> } > >>>> > >>>> int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] ControlList *controls) > >>>> @@ -830,25 +832,30 @@ void SimplePipelineHandler::stop(Camera *camera) > >>>> int SimplePipelineHandler::queueRequestDevice(Camera *camera, Request *request) > >>>> { > >>>> SimpleCameraData *data = cameraData(camera); > >>>> - Stream *stream = &data->streams_[0]; > >>>> + int ret; > >>>> > >>>> - FrameBuffer *buffer = request->findBuffer(stream); > >>>> - if (!buffer) { > >>>> - LOG(SimplePipeline, Error) > >>>> - << "Attempt to queue request with invalid stream"; > >>>> - return -ENOENT; > > We used to validate the stream here... > > >>>> - } > >>>> + std::map<unsigned int, FrameBuffer *> buffers; > >>>> > >>>> - /* > >>>> - * If conversion is needed, push the buffer to the converter queue, it > >>>> - * will be handed to the converter in the capture completion handler. > >>>> - */ > >>>> - if (data->useConverter_) { > >>>> - data->converterQueue_.push(buffer); > >>>> - return 0; > >>>> + for (auto &[stream, buffer] : request->buffers()) { > >>>> + /* > >>>> + * If conversion is needed, push the buffer to the converter > >>>> + * queue, it will be handed to the converter in the capture > >>>> + * completion handler. > >>>> + */ > >>>> + if (data->useConverter_) { > >>>> + unsigned int index = stream - &data->streams_.front(); > > Should we have a helper to convert from stream to index, which includes > a check to validate that it is within streams_.size() ? I'll add a streamIndex() helper. I don't think the check is needed though, as the Camera class verifies that the stream belongs to the camera, so it has to be valid. > That would add some safety to here, and the usage above in > exportFrameBuffers(). > > >>>> + buffers.emplace(index, buffer); > >>>> + } else { > >>>> + ret = data->video_->queueBuffer(buffer); > >>>> + if (ret < 0) > >>>> + return ret; > >>>> + } > >>>> } > >>>> > >>>> - return data->video_->queueBuffer(buffer); > >>>> + if (data->useConverter_) > >>>> + data->converterQueue_.push(std::move(buffers)); > >>>> + > >>>> + return 0; > >>>> } > >>>> > >>>> /* ----------------------------------------------------------------------------- > >>>> @@ -1020,24 +1027,34 @@ void SimplePipelineHandler::bufferReady(FrameBuffer *buffer) > >>>> * point converting an erroneous buffer. > >>>> */ > >>>> if (buffer->metadata().status != FrameMetadata::FrameSuccess) { > >>>> - if (data->useConverter_) { > >>>> - /* Requeue the buffer for capture. */ > >>>> - data->video_->queueBuffer(buffer); > >>>> + if (!data->useConverter_) { > >>>> + /* No conversion, just complete the request. */ > >>>> + Request *request = buffer->request(); > >>>> + completeBuffer(request, buffer); > >>>> + completeRequest(request); > >>>> + return; > >>>> + } > >>>> + > >>>> + /* > >>>> + * The converter is in use. Requeue the internal buffer for > >>>> + * capture, and complete the request with all the user-facing > >>>> + * buffers. > >>>> + */ > >>>> + data->video_->queueBuffer(buffer); > > Does this incorrectly requeue buffers if we're stopping? (I.e. if we > were 'FrameCancelled' or such? Indeed, I'll fix that. > >>>> > >>>> - /* > >>>> - * Get the next user-facing buffer to complete the > >>>> - * request. > >>>> - */ > >>>> - if (data->converterQueue_.empty()) > >>>> - return; > >>>> + if (data->converterQueue_.empty()) > >>>> + return; > >>>> > >>>> - buffer = data->converterQueue_.front(); > >>>> - data->converterQueue_.pop(); > >>>> + Request *request = nullptr; > >>>> + for (auto &item : data->converterQueue_.front()) { > >>>> + FrameBuffer *outputBuffer = item.second; > >>>> + request = outputBuffer->request(); > >>>> + completeBuffer(request, outputBuffer); > >>>> } > >>>> + data->converterQueue_.pop(); > >>>> > >>>> - Request *request = buffer->request(); > >>>> - completeBuffer(request, buffer); > >>>> - completeRequest(request); > >>>> + if (request) > >>> > >>> This check doesn't seem necessary, as we return early if the > >>> converterQueue_ is empty, so the loop will always run once. Is it just > >>> to appease coverity? > >> > >> You're right, I'll drop that. > > > > Actually, we're not looping over data->converterQueue_, but over > > converterQueue_.front(). While it should never be empty, I think it will > > be difficult for compilers (and coverity) to know that, and a check here > > can also act as a bit of defensive programming. I think I'd prefer > > keeping the check. > > I presume at this point no buffer has been passed to any convertor, so > there can't be anything happening in parallel at this point. That's correct. > I don't think there can be so: > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > >>> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > >>> > >>>> + completeRequest(request); > >>>> return; > >>>> } > >>>> > >>>> @@ -1052,10 +1069,8 @@ void SimplePipelineHandler::bufferReady(FrameBuffer *buffer) > >>>> return; > >>>> } > >>>> > >>>> - FrameBuffer *output = data->converterQueue_.front(); > >>>> + converter_->queueBuffers(buffer, data->converterQueue_.front()); > >>>> data->converterQueue_.pop(); > >>>> - > >>>> - converter_->queueBuffers(buffer, { { 0, output } }); > >>>> return; > >>>> } > >>>> > >>>> @@ -1078,10 +1093,10 @@ void SimplePipelineHandler::converterOutputDone(FrameBuffer *buffer) > >>>> { > >>>> ASSERT(activeCamera_); > >>>> > >>>> - /* Complete the request. */ > >>>> + /* Complete the buffer and the request. */ > >>>> Request *request = buffer->request(); > >>>> - completeBuffer(request, buffer); > >>>> - completeRequest(request); > >>>> + if (completeBuffer(request, buffer)) > >>>> + completeRequest(request);>>>> } > >>>> > >>>> REGISTER_PIPELINE_HANDLER(SimplePipelineHandler)
diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp index 58e5f0d23139..55a5528611c8 100644 --- a/src/libcamera/pipeline/simple/simple.cpp +++ b/src/libcamera/pipeline/simple/simple.cpp @@ -173,7 +173,7 @@ public: std::vector<std::unique_ptr<FrameBuffer>> converterBuffers_; bool useConverter_; - std::queue<FrameBuffer *> converterQueue_; + std::queue<std::map<unsigned int, FrameBuffer *>> converterQueue_; }; class SimpleCameraConfiguration : public CameraConfiguration @@ -762,10 +762,12 @@ int SimplePipelineHandler::exportFrameBuffers(Camera *camera, Stream *stream, * Export buffers on the converter or capture video node, depending on * whether the converter is used or not. */ - if (data->useConverter_) - return converter_->exportBuffers(0, count, buffers); - else + if (data->useConverter_) { + unsigned int index = stream - &data->streams_.front(); + return converter_->exportBuffers(index, count, buffers); + } else { return data->video_->exportBuffers(count, buffers); + } } int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] ControlList *controls) @@ -830,25 +832,30 @@ void SimplePipelineHandler::stop(Camera *camera) int SimplePipelineHandler::queueRequestDevice(Camera *camera, Request *request) { SimpleCameraData *data = cameraData(camera); - Stream *stream = &data->streams_[0]; + int ret; - FrameBuffer *buffer = request->findBuffer(stream); - if (!buffer) { - LOG(SimplePipeline, Error) - << "Attempt to queue request with invalid stream"; - return -ENOENT; - } + std::map<unsigned int, FrameBuffer *> buffers; - /* - * If conversion is needed, push the buffer to the converter queue, it - * will be handed to the converter in the capture completion handler. - */ - if (data->useConverter_) { - data->converterQueue_.push(buffer); - return 0; + for (auto &[stream, buffer] : request->buffers()) { + /* + * If conversion is needed, push the buffer to the converter + * queue, it will be handed to the converter in the capture + * completion handler. + */ + if (data->useConverter_) { + unsigned int index = stream - &data->streams_.front(); + buffers.emplace(index, buffer); + } else { + ret = data->video_->queueBuffer(buffer); + if (ret < 0) + return ret; + } } - return data->video_->queueBuffer(buffer); + if (data->useConverter_) + data->converterQueue_.push(std::move(buffers)); + + return 0; } /* ----------------------------------------------------------------------------- @@ -1020,24 +1027,34 @@ void SimplePipelineHandler::bufferReady(FrameBuffer *buffer) * point converting an erroneous buffer. */ if (buffer->metadata().status != FrameMetadata::FrameSuccess) { - if (data->useConverter_) { - /* Requeue the buffer for capture. */ - data->video_->queueBuffer(buffer); + if (!data->useConverter_) { + /* No conversion, just complete the request. */ + Request *request = buffer->request(); + completeBuffer(request, buffer); + completeRequest(request); + return; + } + + /* + * The converter is in use. Requeue the internal buffer for + * capture, and complete the request with all the user-facing + * buffers. + */ + data->video_->queueBuffer(buffer); - /* - * Get the next user-facing buffer to complete the - * request. - */ - if (data->converterQueue_.empty()) - return; + if (data->converterQueue_.empty()) + return; - buffer = data->converterQueue_.front(); - data->converterQueue_.pop(); + Request *request = nullptr; + for (auto &item : data->converterQueue_.front()) { + FrameBuffer *outputBuffer = item.second; + request = outputBuffer->request(); + completeBuffer(request, outputBuffer); } + data->converterQueue_.pop(); - Request *request = buffer->request(); - completeBuffer(request, buffer); - completeRequest(request); + if (request) + completeRequest(request); return; } @@ -1052,10 +1069,8 @@ void SimplePipelineHandler::bufferReady(FrameBuffer *buffer) return; } - FrameBuffer *output = data->converterQueue_.front(); + converter_->queueBuffers(buffer, data->converterQueue_.front()); data->converterQueue_.pop(); - - converter_->queueBuffers(buffer, { { 0, output } }); return; } @@ -1078,10 +1093,10 @@ void SimplePipelineHandler::converterOutputDone(FrameBuffer *buffer) { ASSERT(activeCamera_); - /* Complete the request. */ + /* Complete the buffer and the request. */ Request *request = buffer->request(); - completeBuffer(request, buffer); - completeRequest(request); + if (completeBuffer(request, buffer)) + completeRequest(request); } REGISTER_PIPELINE_HANDLER(SimplePipelineHandler)
To extend the multi-stream support to runtime operation of the pipeline, expand the converter queue to store multiple output buffers, and update the request queuing and buffer completion handlers accordingly. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- src/libcamera/pipeline/simple/simple.cpp | 93 ++++++++++++++---------- 1 file changed, 54 insertions(+), 39 deletions(-)