[libcamera-devel] [PATCH 19/20] libcamera: pipeline: simple: Support usage of multiple streams
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Mar 2 12:19:08 CET 2021
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 at 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 at 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 at ideasonboard.com>
>
> >>> Reviewed-by: Paul Elder <paul.elder at 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)
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list