[{"id":15394,"web_url":"https://patchwork.libcamera.org/comment/15394/","msgid":"<20210302070933.GE35047@pyrite.rasen.tech>","date":"2021-03-02T07:09:33","subject":"Re: [libcamera-devel] [PATCH 19/20] libcamera: pipeline: simple:\n\tSupport usage of multiple streams","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Laurent,\n\nOn Mon, Feb 01, 2021 at 12:47:01AM +0200, Laurent Pinchart wrote:\n> To extend the multi-stream support to runtime operation of the pipeline,\n> expand the converter queue to store multiple output buffers, and update\n> the request queuing and buffer completion handlers accordingly.\n> \n> Signed-off-by: Laurent Pinchart \n> ---\n> src/libcamera/pipeline/simple/simple.cpp | 93 ++++++++++++++----------\n> 1 file changed, 54 insertions(+), 39 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> index 58e5f0d23139..55a5528611c8 100644\n> --- a/src/libcamera/pipeline/simple/simple.cpp\n> +++ b/src/libcamera/pipeline/simple/simple.cpp\n> @@ -173,7 +173,7 @@ public:\n> \n> \tstd::vector> converterBuffers_;\n> \tbool useConverter_;\n> -\tstd::queue converterQueue_;\n> +\tstd::queue> converterQueue_;\n> };\n> \n> class SimpleCameraConfiguration : public CameraConfiguration\n> @@ -762,10 +762,12 @@ int SimplePipelineHandler::exportFrameBuffers(Camera *camera, Stream *stream,\n> \t * Export buffers on the converter or capture video node, depending on\n> \t * whether the converter is used or not.\n> \t */\n> -\tif (data->useConverter_)\n> -\t\treturn converter_->exportBuffers(0, count, buffers);\n> -\telse\n> +\tif (data->useConverter_) {\n> +\t\tunsigned int index = stream - &data->streams_.front();\n> +\t\treturn converter_->exportBuffers(index, count, buffers);\n> +\t} else {\n> \t\treturn data->video_->exportBuffers(count, buffers);\n> +\t}\n> }\n> \n> int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] ControlList *controls)\n> @@ -830,25 +832,30 @@ void SimplePipelineHandler::stop(Camera *camera)\n> int SimplePipelineHandler::queueRequestDevice(Camera *camera, Request *request)\n> {\n> \tSimpleCameraData *data = cameraData(camera);\n> -\tStream *stream = &data->streams_[0];\n> +\tint ret;\n> \n> -\tFrameBuffer *buffer = request->findBuffer(stream);\n> -\tif (!buffer) {\n> -\t\tLOG(SimplePipeline, Error)\n> -\t\t\t<< \"Attempt to queue request with invalid stream\";\n> -\t\treturn -ENOENT;\n> -\t}\n> +\tstd::map buffers;\n> \n> -\t/*\n> -\t * If conversion is needed, push the buffer to the converter queue, it\n> -\t * will be handed to the converter in the capture completion handler.\n> -\t */\n> -\tif (data->useConverter_) {\n> -\t\tdata->converterQueue_.push(buffer);\n> -\t\treturn 0;\n> +\tfor (auto &[stream, buffer] : request->buffers()) {\n> +\t\t/*\n> +\t\t * If conversion is needed, push the buffer to the converter\n> +\t\t * queue, it will be handed to the converter in the capture\n> +\t\t * completion handler.\n> +\t\t */\n> +\t\tif (data->useConverter_) {\n> +\t\t\tunsigned int index = stream - &data->streams_.front();\n> +\t\t\tbuffers.emplace(index, buffer);\n> +\t\t} else {\n> +\t\t\tret = data->video_->queueBuffer(buffer);\n> +\t\t\tif (ret < 0)\n> +\t\t\t\treturn ret;\n> +\t\t}\n> \t}\n> \n> -\treturn data->video_->queueBuffer(buffer);\n> +\tif (data->useConverter_)\n> +\t\tdata->converterQueue_.push(std::move(buffers));\n> +\n> +\treturn 0;\n> }\n> \n> /* -----------------------------------------------------------------------------\n> @@ -1020,24 +1027,34 @@ void SimplePipelineHandler::bufferReady(FrameBuffer *buffer)\n> \t * point converting an erroneous buffer.\n> \t */\n> \tif (buffer->metadata().status != FrameMetadata::FrameSuccess) {\n> -\t\tif (data->useConverter_) {\n> -\t\t\t/* Requeue the buffer for capture. */\n> -\t\t\tdata->video_->queueBuffer(buffer);\n> +\t\tif (!data->useConverter_) {\n> +\t\t\t/* No conversion, just complete the request. */\n> +\t\t\tRequest *request = buffer->request();\n> +\t\t\tcompleteBuffer(request, buffer);\n> +\t\t\tcompleteRequest(request);\n> +\t\t\treturn;\n> +\t\t}\n> +\n> +\t\t/*\n> +\t\t * The converter is in use. Requeue the internal buffer for\n> +\t\t * capture, and complete the request with all the user-facing\n> +\t\t * buffers.\n> +\t\t */\n> +\t\tdata->video_->queueBuffer(buffer);\n> \n> -\t\t\t/*\n> -\t\t\t * Get the next user-facing buffer to complete the\n> -\t\t\t * request.\n> -\t\t\t */\n> -\t\t\tif (data->converterQueue_.empty())\n> -\t\t\t\treturn;\n> +\t\tif (data->converterQueue_.empty())\n> +\t\t\treturn;\n> \n> -\t\t\tbuffer = data->converterQueue_.front();\n> -\t\t\tdata->converterQueue_.pop();\n> +\t\tRequest *request = nullptr;\n> +\t\tfor (auto &item : data->converterQueue_.front()) {\n> +\t\t\tFrameBuffer *outputBuffer = item.second;\n> +\t\t\trequest = outputBuffer->request();\n> +\t\t\tcompleteBuffer(request, outputBuffer);\n> \t\t}\n> +\t\tdata->converterQueue_.pop();\n> \n> -\t\tRequest *request = buffer->request();\n> -\t\tcompleteBuffer(request, buffer);\n> -\t\tcompleteRequest(request);\n> +\t\tif (request)\n\nThis check doesn't seem necessary, as we return early if the\nconverterQueue_ is empty, so the loop will always run once. Is it just\nto appease coverity?\n\n\nReviewed-by: Paul Elder \n\n> +\t\t\tcompleteRequest(request);\n> \t\treturn;\n> \t}\n> \n> @@ -1052,10 +1069,8 @@ void SimplePipelineHandler::bufferReady(FrameBuffer *buffer)\n> \t\t\treturn;\n> \t\t}\n> \n> -\t\tFrameBuffer *output = data->converterQueue_.front();\n> +\t\tconverter_->queueBuffers(buffer, data->converterQueue_.front());\n> \t\tdata->converterQueue_.pop();\n> -\n> -\t\tconverter_->queueBuffers(buffer, { { 0, output } });\n> \t\treturn;\n> \t}\n> \n> @@ -1078,10 +1093,10 @@ void SimplePipelineHandler::converterOutputDone(FrameBuffer *buffer)\n> {\n> \tASSERT(activeCamera_);\n> \n> -\t/* Complete the request. */\n> +\t/* Complete the buffer and the request. */\n> \tRequest *request = buffer->request();\n> -\tcompleteBuffer(request, buffer);\n> -\tcompleteRequest(request);\n> +\tif (completeBuffer(request, buffer))\n> +\t\tcompleteRequest(request);\n> }\n> \n> REGISTER_PIPELINE_HANDLER(SimplePipelineHandler)","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 8E6B6BD1F1\n\tfor ;\n\tTue, 2 Mar 2021 07:09:44 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0E3A968A8D;\n\tTue, 2 Mar 2021 08:09:44 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E048C60522\n\tfor ;\n\tTue, 2 Mar 2021 08:09:42 +0100 (CET)","from pyrite.rasen.tech (unknown\n\t[IPv6:2400:4051:61:600:2c71:1b79:d06d:5032])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 9C8A545D;\n\tTue, 2 Mar 2021 08:09:40 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"UFXxMF+K\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1614668982;\n\tbh=4tjVlel9uuR2BzJ8vutVsidDrbhO+otLR/ce44HCd6I=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=UFXxMF+K3VYxWK5TAvll2nsmEaDeKn7Lw+TvpUe2e81PfqB7XOf+LNJuHj+mxGbof\n\tLd0tRj3lEPyIH9vBc5RJh8dpIJL6bW3NBvkWGYDgVLIVKCXUu/RkDkzQ8D1ClOB9FX\n\tc9D66ewvNYNSirPkf1bbdQJ5JLg45/Hz8hEMTlrw=","Date":"Tue, 2 Mar 2021 16:09:33 +0900","From":"paul.elder@ideasonboard.com","To":"Laurent Pinchart ","Message-ID":"<20210302070933.GE35047@pyrite.rasen.tech>","References":"<20210131224702.8838-1-laurent.pinchart@ideasonboard.com>\n\t<20210131224702.8838-20-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210131224702.8838-20-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 19/20] libcamera: pipeline: simple:\n\tSupport usage of multiple streams","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"Phi-Bang Nguyen ,\n\tlibcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15406,"web_url":"https://patchwork.libcamera.org/comment/15406/","msgid":"","date":"2021-03-02T10:34:09","subject":"Re: [libcamera-devel] [PATCH 19/20] libcamera: pipeline: simple:\n\tSupport usage of multiple streams","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Paul,\n\nOn Tue, Mar 02, 2021 at 04:09:33PM +0900, paul.elder@ideasonboard.com wrote:\n> On Mon, Feb 01, 2021 at 12:47:01AM +0200, Laurent Pinchart wrote:\n> > To extend the multi-stream support to runtime operation of the pipeline,\n> > expand the converter queue to store multiple output buffers, and update\n> > the request queuing and buffer completion handlers accordingly.\n> > \n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > src/libcamera/pipeline/simple/simple.cpp | 93 ++++++++++++++----------\n> > 1 file changed, 54 insertions(+), 39 deletions(-)\n> > \n> > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> > index 58e5f0d23139..55a5528611c8 100644\n> > --- a/src/libcamera/pipeline/simple/simple.cpp\n> > +++ b/src/libcamera/pipeline/simple/simple.cpp\n> > @@ -173,7 +173,7 @@ public:\n> > \n> > \tstd::vector> converterBuffers_;\n> > \tbool useConverter_;\n> > -\tstd::queue converterQueue_;\n> > +\tstd::queue> converterQueue_;\n> > };\n> > \n> > class SimpleCameraConfiguration : public CameraConfiguration\n> > @@ -762,10 +762,12 @@ int SimplePipelineHandler::exportFrameBuffers(Camera *camera, Stream *stream,\n> > \t * Export buffers on the converter or capture video node, depending on\n> > \t * whether the converter is used or not.\n> > \t */\n> > -\tif (data->useConverter_)\n> > -\t\treturn converter_->exportBuffers(0, count, buffers);\n> > -\telse\n> > +\tif (data->useConverter_) {\n> > +\t\tunsigned int index = stream - &data->streams_.front();\n> > +\t\treturn converter_->exportBuffers(index, count, buffers);\n> > +\t} else {\n> > \t\treturn data->video_->exportBuffers(count, buffers);\n> > +\t}\n> > }\n> > \n> > int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] ControlList *controls)\n> > @@ -830,25 +832,30 @@ void SimplePipelineHandler::stop(Camera *camera)\n> > int SimplePipelineHandler::queueRequestDevice(Camera *camera, Request *request)\n> > {\n> > \tSimpleCameraData *data = cameraData(camera);\n> > -\tStream *stream = &data->streams_[0];\n> > +\tint ret;\n> > \n> > -\tFrameBuffer *buffer = request->findBuffer(stream);\n> > -\tif (!buffer) {\n> > -\t\tLOG(SimplePipeline, Error)\n> > -\t\t\t<< \"Attempt to queue request with invalid stream\";\n> > -\t\treturn -ENOENT;\n> > -\t}\n> > +\tstd::map buffers;\n> > \n> > -\t/*\n> > -\t * If conversion is needed, push the buffer to the converter queue, it\n> > -\t * will be handed to the converter in the capture completion handler.\n> > -\t */\n> > -\tif (data->useConverter_) {\n> > -\t\tdata->converterQueue_.push(buffer);\n> > -\t\treturn 0;\n> > +\tfor (auto &[stream, buffer] : request->buffers()) {\n> > +\t\t/*\n> > +\t\t * If conversion is needed, push the buffer to the converter\n> > +\t\t * queue, it will be handed to the converter in the capture\n> > +\t\t * completion handler.\n> > +\t\t */\n> > +\t\tif (data->useConverter_) {\n> > +\t\t\tunsigned int index = stream - &data->streams_.front();\n> > +\t\t\tbuffers.emplace(index, buffer);\n> > +\t\t} else {\n> > +\t\t\tret = data->video_->queueBuffer(buffer);\n> > +\t\t\tif (ret < 0)\n> > +\t\t\t\treturn ret;\n> > +\t\t}\n> > \t}\n> > \n> > -\treturn data->video_->queueBuffer(buffer);\n> > +\tif (data->useConverter_)\n> > +\t\tdata->converterQueue_.push(std::move(buffers));\n> > +\n> > +\treturn 0;\n> > }\n> > \n> > /* -----------------------------------------------------------------------------\n> > @@ -1020,24 +1027,34 @@ void SimplePipelineHandler::bufferReady(FrameBuffer *buffer)\n> > \t * point converting an erroneous buffer.\n> > \t */\n> > \tif (buffer->metadata().status != FrameMetadata::FrameSuccess) {\n> > -\t\tif (data->useConverter_) {\n> > -\t\t\t/* Requeue the buffer for capture. */\n> > -\t\t\tdata->video_->queueBuffer(buffer);\n> > +\t\tif (!data->useConverter_) {\n> > +\t\t\t/* No conversion, just complete the request. */\n> > +\t\t\tRequest *request = buffer->request();\n> > +\t\t\tcompleteBuffer(request, buffer);\n> > +\t\t\tcompleteRequest(request);\n> > +\t\t\treturn;\n> > +\t\t}\n> > +\n> > +\t\t/*\n> > +\t\t * The converter is in use. Requeue the internal buffer for\n> > +\t\t * capture, and complete the request with all the user-facing\n> > +\t\t * buffers.\n> > +\t\t */\n> > +\t\tdata->video_->queueBuffer(buffer);\n> > \n> > -\t\t\t/*\n> > -\t\t\t * Get the next user-facing buffer to complete the\n> > -\t\t\t * request.\n> > -\t\t\t */\n> > -\t\t\tif (data->converterQueue_.empty())\n> > -\t\t\t\treturn;\n> > +\t\tif (data->converterQueue_.empty())\n> > +\t\t\treturn;\n> > \n> > -\t\t\tbuffer = data->converterQueue_.front();\n> > -\t\t\tdata->converterQueue_.pop();\n> > +\t\tRequest *request = nullptr;\n> > +\t\tfor (auto &item : data->converterQueue_.front()) {\n> > +\t\t\tFrameBuffer *outputBuffer = item.second;\n> > +\t\t\trequest = outputBuffer->request();\n> > +\t\t\tcompleteBuffer(request, outputBuffer);\n> > \t\t}\n> > +\t\tdata->converterQueue_.pop();\n> > \n> > -\t\tRequest *request = buffer->request();\n> > -\t\tcompleteBuffer(request, buffer);\n> > -\t\tcompleteRequest(request);\n> > +\t\tif (request)\n> \n> This check doesn't seem necessary, as we return early if the\n> converterQueue_ is empty, so the loop will always run once. Is it just\n> to appease coverity?\n\nYou're right, I'll drop that.\n\n> Reviewed-by: Paul Elder \n> \n> > +\t\t\tcompleteRequest(request);\n> > \t\treturn;\n> > \t}\n> > \n> > @@ -1052,10 +1069,8 @@ void SimplePipelineHandler::bufferReady(FrameBuffer *buffer)\n> > \t\t\treturn;\n> > \t\t}\n> > \n> > -\t\tFrameBuffer *output = data->converterQueue_.front();\n> > +\t\tconverter_->queueBuffers(buffer, data->converterQueue_.front());\n> > \t\tdata->converterQueue_.pop();\n> > -\n> > -\t\tconverter_->queueBuffers(buffer, { { 0, output } });\n> > \t\treturn;\n> > \t}\n> > \n> > @@ -1078,10 +1093,10 @@ void SimplePipelineHandler::converterOutputDone(FrameBuffer *buffer)\n> > {\n> > \tASSERT(activeCamera_);\n> > \n> > -\t/* Complete the request. */\n> > +\t/* Complete the buffer and the request. */\n> > \tRequest *request = buffer->request();\n> > -\tcompleteBuffer(request, buffer);\n> > -\tcompleteRequest(request);\n> > +\tif (completeBuffer(request, buffer))\n> > +\t\tcompleteRequest(request);\n> > }\n> > \n> > REGISTER_PIPELINE_HANDLER(SimplePipelineHandler)","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 10A17BD808\n\tfor ;\n\tTue, 2 Mar 2021 10:34:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 876DF68A92;\n\tTue, 2 Mar 2021 11:34:39 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3854468A91\n\tfor ;\n\tTue, 2 Mar 2021 11:34:38 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 919DE45D;\n\tTue, 2 Mar 2021 11:34:37 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"ivcSw0Q8\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1614681277;\n\tbh=r+imUw9RAjx1bukbHGsXU845XpplujM/ACZl1xMXv+8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ivcSw0Q8BwNf+XoVndhT8pnuKXm4gTvfB61Z3R3oWmkxawBQ6Gz9tFfsABA0SHBDy\n\tsdOPCePXhQcG5pvJZKFykOXcGhXXlEqpOxkLkvlIFNettRkiC4On4m5u4JGKQstNvr\n\tFYsBeTBx3p4bvg+b+ngYiFvd3wR+2BAVin2MJ+tg=","Date":"Tue, 2 Mar 2021 12:34:09 +0200","From":"Laurent Pinchart ","To":"paul.elder@ideasonboard.com","Message-ID":"","References":"<20210131224702.8838-1-laurent.pinchart@ideasonboard.com>\n\t<20210131224702.8838-20-laurent.pinchart@ideasonboard.com>\n\t<20210302070933.GE35047@pyrite.rasen.tech>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210302070933.GE35047@pyrite.rasen.tech>","Subject":"Re: [libcamera-devel] [PATCH 19/20] libcamera: pipeline: simple:\n\tSupport usage of multiple streams","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"Phi-Bang Nguyen ,\n\tlibcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15407,"web_url":"https://patchwork.libcamera.org/comment/15407/","msgid":"","date":"2021-03-02T10:39:01","subject":"Re: [libcamera-devel] [PATCH 19/20] libcamera: pipeline: simple:\n\tSupport usage of multiple streams","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi again,\n\nOn Tue, Mar 02, 2021 at 12:34:10PM +0200, Laurent Pinchart wrote:\n> On Tue, Mar 02, 2021 at 04:09:33PM +0900, paul.elder@ideasonboard.com wrote:\n> > On Mon, Feb 01, 2021 at 12:47:01AM +0200, Laurent Pinchart wrote:\n> > > To extend the multi-stream support to runtime operation of the pipeline,\n> > > expand the converter queue to store multiple output buffers, and update\n> > > the request queuing and buffer completion handlers accordingly.\n> > > \n> > > Signed-off-by: Laurent Pinchart \n> > > ---\n> > > src/libcamera/pipeline/simple/simple.cpp | 93 ++++++++++++++----------\n> > > 1 file changed, 54 insertions(+), 39 deletions(-)\n> > > \n> > > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> > > index 58e5f0d23139..55a5528611c8 100644\n> > > --- a/src/libcamera/pipeline/simple/simple.cpp\n> > > +++ b/src/libcamera/pipeline/simple/simple.cpp\n> > > @@ -173,7 +173,7 @@ public:\n> > > \n> > > \tstd::vector> converterBuffers_;\n> > > \tbool useConverter_;\n> > > -\tstd::queue converterQueue_;\n> > > +\tstd::queue> converterQueue_;\n> > > };\n> > > \n> > > class SimpleCameraConfiguration : public CameraConfiguration\n> > > @@ -762,10 +762,12 @@ int SimplePipelineHandler::exportFrameBuffers(Camera *camera, Stream *stream,\n> > > \t * Export buffers on the converter or capture video node, depending on\n> > > \t * whether the converter is used or not.\n> > > \t */\n> > > -\tif (data->useConverter_)\n> > > -\t\treturn converter_->exportBuffers(0, count, buffers);\n> > > -\telse\n> > > +\tif (data->useConverter_) {\n> > > +\t\tunsigned int index = stream - &data->streams_.front();\n> > > +\t\treturn converter_->exportBuffers(index, count, buffers);\n> > > +\t} else {\n> > > \t\treturn data->video_->exportBuffers(count, buffers);\n> > > +\t}\n> > > }\n> > > \n> > > int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] ControlList *controls)\n> > > @@ -830,25 +832,30 @@ void SimplePipelineHandler::stop(Camera *camera)\n> > > int SimplePipelineHandler::queueRequestDevice(Camera *camera, Request *request)\n> > > {\n> > > \tSimpleCameraData *data = cameraData(camera);\n> > > -\tStream *stream = &data->streams_[0];\n> > > +\tint ret;\n> > > \n> > > -\tFrameBuffer *buffer = request->findBuffer(stream);\n> > > -\tif (!buffer) {\n> > > -\t\tLOG(SimplePipeline, Error)\n> > > -\t\t\t<< \"Attempt to queue request with invalid stream\";\n> > > -\t\treturn -ENOENT;\n> > > -\t}\n> > > +\tstd::map buffers;\n> > > \n> > > -\t/*\n> > > -\t * If conversion is needed, push the buffer to the converter queue, it\n> > > -\t * will be handed to the converter in the capture completion handler.\n> > > -\t */\n> > > -\tif (data->useConverter_) {\n> > > -\t\tdata->converterQueue_.push(buffer);\n> > > -\t\treturn 0;\n> > > +\tfor (auto &[stream, buffer] : request->buffers()) {\n> > > +\t\t/*\n> > > +\t\t * If conversion is needed, push the buffer to the converter\n> > > +\t\t * queue, it will be handed to the converter in the capture\n> > > +\t\t * completion handler.\n> > > +\t\t */\n> > > +\t\tif (data->useConverter_) {\n> > > +\t\t\tunsigned int index = stream - &data->streams_.front();\n> > > +\t\t\tbuffers.emplace(index, buffer);\n> > > +\t\t} else {\n> > > +\t\t\tret = data->video_->queueBuffer(buffer);\n> > > +\t\t\tif (ret < 0)\n> > > +\t\t\t\treturn ret;\n> > > +\t\t}\n> > > \t}\n> > > \n> > > -\treturn data->video_->queueBuffer(buffer);\n> > > +\tif (data->useConverter_)\n> > > +\t\tdata->converterQueue_.push(std::move(buffers));\n> > > +\n> > > +\treturn 0;\n> > > }\n> > > \n> > > /* -----------------------------------------------------------------------------\n> > > @@ -1020,24 +1027,34 @@ void SimplePipelineHandler::bufferReady(FrameBuffer *buffer)\n> > > \t * point converting an erroneous buffer.\n> > > \t */\n> > > \tif (buffer->metadata().status != FrameMetadata::FrameSuccess) {\n> > > -\t\tif (data->useConverter_) {\n> > > -\t\t\t/* Requeue the buffer for capture. */\n> > > -\t\t\tdata->video_->queueBuffer(buffer);\n> > > +\t\tif (!data->useConverter_) {\n> > > +\t\t\t/* No conversion, just complete the request. */\n> > > +\t\t\tRequest *request = buffer->request();\n> > > +\t\t\tcompleteBuffer(request, buffer);\n> > > +\t\t\tcompleteRequest(request);\n> > > +\t\t\treturn;\n> > > +\t\t}\n> > > +\n> > > +\t\t/*\n> > > +\t\t * The converter is in use. Requeue the internal buffer for\n> > > +\t\t * capture, and complete the request with all the user-facing\n> > > +\t\t * buffers.\n> > > +\t\t */\n> > > +\t\tdata->video_->queueBuffer(buffer);\n> > > \n> > > -\t\t\t/*\n> > > -\t\t\t * Get the next user-facing buffer to complete the\n> > > -\t\t\t * request.\n> > > -\t\t\t */\n> > > -\t\t\tif (data->converterQueue_.empty())\n> > > -\t\t\t\treturn;\n> > > +\t\tif (data->converterQueue_.empty())\n> > > +\t\t\treturn;\n> > > \n> > > -\t\t\tbuffer = data->converterQueue_.front();\n> > > -\t\t\tdata->converterQueue_.pop();\n> > > +\t\tRequest *request = nullptr;\n> > > +\t\tfor (auto &item : data->converterQueue_.front()) {\n> > > +\t\t\tFrameBuffer *outputBuffer = item.second;\n> > > +\t\t\trequest = outputBuffer->request();\n> > > +\t\t\tcompleteBuffer(request, outputBuffer);\n> > > \t\t}\n> > > +\t\tdata->converterQueue_.pop();\n> > > \n> > > -\t\tRequest *request = buffer->request();\n> > > -\t\tcompleteBuffer(request, buffer);\n> > > -\t\tcompleteRequest(request);\n> > > +\t\tif (request)\n> > \n> > This check doesn't seem necessary, as we return early if the\n> > converterQueue_ is empty, so the loop will always run once. Is it just\n> > to appease coverity?\n> \n> You're right, I'll drop that.\n\nActually, we're not looping over data->converterQueue_, but over\nconverterQueue_.front(). While it should never be empty, I think it will\nbe difficult for compilers (and coverity) to know that, and a check here\ncan also act as a bit of defensive programming. I think I'd prefer\nkeeping the check.\n\n> > Reviewed-by: Paul Elder \n> > \n> > > +\t\t\tcompleteRequest(request);\n> > > \t\treturn;\n> > > \t}\n> > > \n> > > @@ -1052,10 +1069,8 @@ void SimplePipelineHandler::bufferReady(FrameBuffer *buffer)\n> > > \t\t\treturn;\n> > > \t\t}\n> > > \n> > > -\t\tFrameBuffer *output = data->converterQueue_.front();\n> > > +\t\tconverter_->queueBuffers(buffer, data->converterQueue_.front());\n> > > \t\tdata->converterQueue_.pop();\n> > > -\n> > > -\t\tconverter_->queueBuffers(buffer, { { 0, output } });\n> > > \t\treturn;\n> > > \t}\n> > > \n> > > @@ -1078,10 +1093,10 @@ void SimplePipelineHandler::converterOutputDone(FrameBuffer *buffer)\n> > > {\n> > > \tASSERT(activeCamera_);\n> > > \n> > > -\t/* Complete the request. */\n> > > +\t/* Complete the buffer and the request. */\n> > > \tRequest *request = buffer->request();\n> > > -\tcompleteBuffer(request, buffer);\n> > > -\tcompleteRequest(request);\n> > > +\tif (completeBuffer(request, buffer))\n> > > +\t\tcompleteRequest(request);\n> > > }\n> > > \n> > > REGISTER_PIPELINE_HANDLER(SimplePipelineHandler)","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id CA1B6BD1F1\n\tfor ;\n\tTue, 2 Mar 2021 10:39:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 487E268A97;\n\tTue, 2 Mar 2021 11:39:31 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A2E9F68A91\n\tfor ;\n\tTue, 2 Mar 2021 11:39:29 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 15E6245D;\n\tTue, 2 Mar 2021 11:39:29 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"gfdXus/h\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1614681569;\n\tbh=Q2754hBWTIYqH0Q1jE5UvzTFR+Z+9y4n/vOs4OfzlZU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=gfdXus/h4O51NaYe6Z4b955gCZRSGUrTyk3DqjmpSezIGSS4yC6s7H1zgrgKeTnHW\n\tTFhJVnPPOZFaw+FEEz8xCc5q9cQCvpQ9mPmi+6GbdfBIQdZK3edhUFNzTo6FoGQPB0\n\taRuL0b3I3WlYruDdxXlv5aTKJt5YVFHugu8EvmtU=","Date":"Tue, 2 Mar 2021 12:39:01 +0200","From":"Laurent Pinchart ","To":"paul.elder@ideasonboard.com","Message-ID":"","References":"<20210131224702.8838-1-laurent.pinchart@ideasonboard.com>\n\t<20210131224702.8838-20-laurent.pinchart@ideasonboard.com>\n\t<20210302070933.GE35047@pyrite.rasen.tech>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 19/20] libcamera: pipeline: simple:\n\tSupport usage of multiple streams","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"Phi-Bang Nguyen ,\n\tlibcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15411,"web_url":"https://patchwork.libcamera.org/comment/15411/","msgid":"","date":"2021-03-02T11:06:13","subject":"Re: [libcamera-devel] [PATCH 19/20] libcamera: pipeline: simple:\n\tSupport usage of multiple streams","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"On 02/03/2021 10:39, Laurent Pinchart wrote:\n> Hi again,\n> \n> On Tue, Mar 02, 2021 at 12:34:10PM +0200, Laurent Pinchart wrote:\n>> On Tue, Mar 02, 2021 at 04:09:33PM +0900, paul.elder@ideasonboard.com wrote:\n>>> On Mon, Feb 01, 2021 at 12:47:01AM +0200, Laurent Pinchart wrote:\n>>>> To extend the multi-stream support to runtime operation of the pipeline,\n>>>> expand the converter queue to store multiple output buffers, and update\n>>>> the request queuing and buffer completion handlers accordingly.\n>>>>\n>>>> Signed-off-by: Laurent Pinchart \n>>>> ---\n>>>> src/libcamera/pipeline/simple/simple.cpp | 93 ++++++++++++++----------\n>>>> 1 file changed, 54 insertions(+), 39 deletions(-)\n>>>>\n>>>> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n>>>> index 58e5f0d23139..55a5528611c8 100644\n>>>> --- a/src/libcamera/pipeline/simple/simple.cpp\n>>>> +++ b/src/libcamera/pipeline/simple/simple.cpp\n>>>> @@ -173,7 +173,7 @@ public:\n>>>> \n>>>> \tstd::vector> converterBuffers_;\n>>>> \tbool useConverter_;\n>>>> -\tstd::queue converterQueue_;\n>>>> +\tstd::queue> converterQueue_;\n>>>> };\n>>>> \n>>>> class SimpleCameraConfiguration : public CameraConfiguration\n>>>> @@ -762,10 +762,12 @@ int SimplePipelineHandler::exportFrameBuffers(Camera *camera, Stream *stream,\n>>>> \t * Export buffers on the converter or capture video node, depending on\n>>>> \t * whether the converter is used or not.\n>>>> \t */\n>>>> -\tif (data->useConverter_)\n>>>> -\t\treturn converter_->exportBuffers(0, count, buffers);\n>>>> -\telse\n>>>> +\tif (data->useConverter_) {\n>>>> +\t\tunsigned int index = stream - &data->streams_.front();\n>>>> +\t\treturn converter_->exportBuffers(index, count, buffers);\n>>>> +\t} else {\n>>>> \t\treturn data->video_->exportBuffers(count, buffers);\n>>>> +\t}\n>>>> }\n>>>> \n>>>> int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] ControlList *controls)\n>>>> @@ -830,25 +832,30 @@ void SimplePipelineHandler::stop(Camera *camera)\n>>>> int SimplePipelineHandler::queueRequestDevice(Camera *camera, Request *request)\n>>>> {\n>>>> \tSimpleCameraData *data = cameraData(camera);\n>>>> -\tStream *stream = &data->streams_[0];\n>>>> +\tint ret;\n>>>> \n>>>> -\tFrameBuffer *buffer = request->findBuffer(stream);\n>>>> -\tif (!buffer) {\n>>>> -\t\tLOG(SimplePipeline, Error)\n>>>> -\t\t\t<< \"Attempt to queue request with invalid stream\";\n>>>> -\t\treturn -ENOENT;\n\nWe used to validate the stream here...\n\n>>>> -\t}\n>>>> +\tstd::map buffers;\n>>>> \n>>>> -\t/*\n>>>> -\t * If conversion is needed, push the buffer to the converter queue, it\n>>>> -\t * will be handed to the converter in the capture completion handler.\n>>>> -\t */\n>>>> -\tif (data->useConverter_) {\n>>>> -\t\tdata->converterQueue_.push(buffer);\n>>>> -\t\treturn 0;\n>>>> +\tfor (auto &[stream, buffer] : request->buffers()) {\n>>>> +\t\t/*\n>>>> +\t\t * If conversion is needed, push the buffer to the converter\n>>>> +\t\t * queue, it will be handed to the converter in the capture\n>>>> +\t\t * completion handler.\n>>>> +\t\t */\n>>>> +\t\tif (data->useConverter_) {\n>>>> +\t\t\tunsigned int index = stream - &data->streams_.front();\n\nShould we have a helper to convert from stream to index, which includes\na check to validate that it is within streams_.size() ?\n\nThat would add some safety to here, and the usage above in\nexportFrameBuffers().\n\n\n>>>> +\t\t\tbuffers.emplace(index, buffer);\n>>>> +\t\t} else {\n>>>> +\t\t\tret = data->video_->queueBuffer(buffer);\n>>>> +\t\t\tif (ret < 0)\n>>>> +\t\t\t\treturn ret;\n>>>> +\t\t}\n>>>> \t}\n>>>> \n>>>> -\treturn data->video_->queueBuffer(buffer);\n>>>> +\tif (data->useConverter_)\n>>>> +\t\tdata->converterQueue_.push(std::move(buffers));\n>>>> +\n>>>> +\treturn 0;\n>>>> }\n>>>> \n>>>> /* -----------------------------------------------------------------------------\n>>>> @@ -1020,24 +1027,34 @@ void SimplePipelineHandler::bufferReady(FrameBuffer *buffer)\n>>>> \t * point converting an erroneous buffer.\n>>>> \t */\n>>>> \tif (buffer->metadata().status != FrameMetadata::FrameSuccess) {\n>>>> -\t\tif (data->useConverter_) {\n>>>> -\t\t\t/* Requeue the buffer for capture. */\n>>>> -\t\t\tdata->video_->queueBuffer(buffer);\n>>>> +\t\tif (!data->useConverter_) {\n>>>> +\t\t\t/* No conversion, just complete the request. */\n>>>> +\t\t\tRequest *request = buffer->request();\n>>>> +\t\t\tcompleteBuffer(request, buffer);\n>>>> +\t\t\tcompleteRequest(request);\n>>>> +\t\t\treturn;\n>>>> +\t\t}\n>>>> +\n>>>> +\t\t/*\n>>>> +\t\t * The converter is in use. Requeue the internal buffer for\n>>>> +\t\t * capture, and complete the request with all the user-facing\n>>>> +\t\t * buffers.\n>>>> +\t\t */\n>>>> +\t\tdata->video_->queueBuffer(buffer);\n\nDoes this incorrectly requeue buffers if we're stopping? (I.e. if we\nwere 'FrameCancelled' or such?\n\n\n>>>> \n>>>> -\t\t\t/*\n>>>> -\t\t\t * Get the next user-facing buffer to complete the\n>>>> -\t\t\t * request.\n>>>> -\t\t\t */\n>>>> -\t\t\tif (data->converterQueue_.empty())\n>>>> -\t\t\t\treturn;\n>>>> +\t\tif (data->converterQueue_.empty())\n>>>> +\t\t\treturn;\n>>>> \n>>>> -\t\t\tbuffer = data->converterQueue_.front();\n>>>> -\t\t\tdata->converterQueue_.pop();\n>>>> +\t\tRequest *request = nullptr;\n>>>> +\t\tfor (auto &item : data->converterQueue_.front()) {\n>>>> +\t\t\tFrameBuffer *outputBuffer = item.second;\n>>>> +\t\t\trequest = outputBuffer->request();\n>>>> +\t\t\tcompleteBuffer(request, outputBuffer);\n>>>> \t\t}\n>>>> +\t\tdata->converterQueue_.pop();\n>>>> \n>>>> -\t\tRequest *request = buffer->request();\n>>>> -\t\tcompleteBuffer(request, buffer);\n>>>> -\t\tcompleteRequest(request);\n>>>> +\t\tif (request)\n>>>\n>>> This check doesn't seem necessary, as we return early if the\n>>> converterQueue_ is empty, so the loop will always run once. Is it just\n>>> to appease coverity?\n>>\n>> You're right, I'll drop that.\n> \n> Actually, we're not looping over data->converterQueue_, but over\n> converterQueue_.front(). While it should never be empty, I think it will\n> be difficult for compilers (and coverity) to know that, and a check here\n> can also act as a bit of defensive programming. I think I'd prefer\n> keeping the check.\n\nI presume at this point no buffer has been passed to any convertor, so\nthere can't be anything happening in parallel at this point.\n\nI don't think there can be so:\n\nReviewed-by: Kieran Bingham \n\n\n\n>>> Reviewed-by: Paul Elder \n>>>\n>>>> +\t\t\tcompleteRequest(request);\n>>>> \t\treturn;\n>>>> \t}\n>>>> \n>>>> @@ -1052,10 +1069,8 @@ void SimplePipelineHandler::bufferReady(FrameBuffer *buffer)\n>>>> \t\t\treturn;\n>>>> \t\t}\n>>>> \n>>>> -\t\tFrameBuffer *output = data->converterQueue_.front();\n>>>> +\t\tconverter_->queueBuffers(buffer, data->converterQueue_.front());\n>>>> \t\tdata->converterQueue_.pop();\n>>>> -\n>>>> -\t\tconverter_->queueBuffers(buffer, { { 0, output } });\n>>>> \t\treturn;\n>>>> \t}\n>>>> \n>>>> @@ -1078,10 +1093,10 @@ void SimplePipelineHandler::converterOutputDone(FrameBuffer *buffer)\n>>>> {\n>>>> \tASSERT(activeCamera_);\n>>>> \n>>>> -\t/* Complete the request. */\n>>>> +\t/* Complete the buffer and the request. */\n>>>> \tRequest *request = buffer->request();\n>>>> -\tcompleteBuffer(request, buffer);\n>>>> -\tcompleteRequest(request);\n>>>> +\tif (completeBuffer(request, buffer))\n>>>> +\t\tcompleteRequest(request);>>>> }\n>>>> \n>>>> REGISTER_PIPELINE_HANDLER(SimplePipelineHandler)\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 8F3CDBD808\n\tfor ;\n\tTue, 2 Mar 2021 11:06:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2102068A7E;\n\tTue, 2 Mar 2021 12:06:18 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7CF6A60521\n\tfor ;\n\tTue, 2 Mar 2021 12:06:16 +0100 (CET)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C647145D;\n\tTue, 2 Mar 2021 12:06:15 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"deCLSjnK\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1614683176;\n\tbh=hRquxWCwqRuOIW3a8SpbLOr3xMKB8A5mpZ0+C3p2sUk=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=deCLSjnKmOTyV0mEz2Ojzixj7NyU1gMEQ3UWmsHFwJBRHDWYRxLKci8MwVh/xQ+My\n\tGTeeDTvHxp9T+J5inVH5+fLN9noxjuvKBzf+Eiq+N2d/bpAg9Z1shl6FvhhKsXdlwN\n\t/5xuSOD63T6lBhzqRi5WWKfxUnFmOzy1gC3bOhrg=","To":"Laurent Pinchart ,\n\tpaul.elder@ideasonboard.com","References":"<20210131224702.8838-1-laurent.pinchart@ideasonboard.com>\n\t<20210131224702.8838-20-laurent.pinchart@ideasonboard.com>\n\t<20210302070933.GE35047@pyrite.rasen.tech>\n\t\n\t","From":"Kieran Bingham ","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"","Date":"Tue, 2 Mar 2021 11:06:13 +0000","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.10.0","MIME-Version":"1.0","In-Reply-To":"","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH 19/20] libcamera: pipeline: simple:\n\tSupport usage of multiple streams","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Reply-To":"kieran.bingham@ideasonboard.com","Cc":"Phi-Bang Nguyen ,\n\tlibcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15414,"web_url":"https://patchwork.libcamera.org/comment/15414/","msgid":"","date":"2021-03-02T11:19:08","subject":"Re: [libcamera-devel] [PATCH 19/20] libcamera: pipeline: simple:\n\tSupport usage of multiple streams","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Tue, Mar 02, 2021 at 11:06:13AM +0000, Kieran Bingham wrote:\n> On 02/03/2021 10:39, Laurent Pinchart wrote:\n> > On Tue, Mar 02, 2021 at 12:34:10PM +0200, Laurent Pinchart wrote:\n> >> On Tue, Mar 02, 2021 at 04:09:33PM +0900, paul.elder@ideasonboard.com wrote:\n> >>> On Mon, Feb 01, 2021 at 12:47:01AM +0200, Laurent Pinchart wrote:\n> >>>> To extend the multi-stream support to runtime operation of the pipeline,\n> >>>> expand the converter queue to store multiple output buffers, and update\n> >>>> the request queuing and buffer completion handlers accordingly.\n> >>>>\n> >>>> Signed-off-by: Laurent Pinchart \n> >>>> ---\n> >>>> src/libcamera/pipeline/simple/simple.cpp | 93 ++++++++++++++----------\n> >>>> 1 file changed, 54 insertions(+), 39 deletions(-)\n> >>>>\n> >>>> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> >>>> index 58e5f0d23139..55a5528611c8 100644\n> >>>> --- a/src/libcamera/pipeline/simple/simple.cpp\n> >>>> +++ b/src/libcamera/pipeline/simple/simple.cpp\n> >>>> @@ -173,7 +173,7 @@ public:\n> >>>> \n> >>>> \tstd::vector> converterBuffers_;\n> >>>> \tbool useConverter_;\n> >>>> -\tstd::queue converterQueue_;\n> >>>> +\tstd::queue> converterQueue_;\n> >>>> };\n> >>>> \n> >>>> class SimpleCameraConfiguration : public CameraConfiguration\n> >>>> @@ -762,10 +762,12 @@ int SimplePipelineHandler::exportFrameBuffers(Camera *camera, Stream *stream,\n> >>>> \t * Export buffers on the converter or capture video node, depending on\n> >>>> \t * whether the converter is used or not.\n> >>>> \t */\n> >>>> -\tif (data->useConverter_)\n> >>>> -\t\treturn converter_->exportBuffers(0, count, buffers);\n> >>>> -\telse\n> >>>> +\tif (data->useConverter_) {\n> >>>> +\t\tunsigned int index = stream - &data->streams_.front();\n> >>>> +\t\treturn converter_->exportBuffers(index, count, buffers);\n> >>>> +\t} else {\n> >>>> \t\treturn data->video_->exportBuffers(count, buffers);\n> >>>> +\t}\n> >>>> }\n> >>>> \n> >>>> int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] ControlList *controls)\n> >>>> @@ -830,25 +832,30 @@ void SimplePipelineHandler::stop(Camera *camera)\n> >>>> int SimplePipelineHandler::queueRequestDevice(Camera *camera, Request *request)\n> >>>> {\n> >>>> \tSimpleCameraData *data = cameraData(camera);\n> >>>> -\tStream *stream = &data->streams_[0];\n> >>>> +\tint ret;\n> >>>> \n> >>>> -\tFrameBuffer *buffer = request->findBuffer(stream);\n> >>>> -\tif (!buffer) {\n> >>>> -\t\tLOG(SimplePipeline, Error)\n> >>>> -\t\t\t<< \"Attempt to queue request with invalid stream\";\n> >>>> -\t\treturn -ENOENT;\n> \n> We used to validate the stream here...\n> \n> >>>> -\t}\n> >>>> +\tstd::map buffers;\n> >>>> \n> >>>> -\t/*\n> >>>> -\t * If conversion is needed, push the buffer to the converter queue, it\n> >>>> -\t * will be handed to the converter in the capture completion handler.\n> >>>> -\t */\n> >>>> -\tif (data->useConverter_) {\n> >>>> -\t\tdata->converterQueue_.push(buffer);\n> >>>> -\t\treturn 0;\n> >>>> +\tfor (auto &[stream, buffer] : request->buffers()) {\n> >>>> +\t\t/*\n> >>>> +\t\t * If conversion is needed, push the buffer to the converter\n> >>>> +\t\t * queue, it will be handed to the converter in the capture\n> >>>> +\t\t * completion handler.\n> >>>> +\t\t */\n> >>>> +\t\tif (data->useConverter_) {\n> >>>> +\t\t\tunsigned int index = stream - &data->streams_.front();\n> \n> Should we have a helper to convert from stream to index, which includes\n> a check to validate that it is within streams_.size() ?\n\nI'll add a streamIndex() helper. I don't think the check is needed\nthough, as the Camera class verifies that the stream belongs to the\ncamera, so it has to be valid.\n\n> That would add some safety to here, and the usage above in\n> exportFrameBuffers().\n> \n> >>>> +\t\t\tbuffers.emplace(index, buffer);\n> >>>> +\t\t} else {\n> >>>> +\t\t\tret = data->video_->queueBuffer(buffer);\n> >>>> +\t\t\tif (ret < 0)\n> >>>> +\t\t\t\treturn ret;\n> >>>> +\t\t}\n> >>>> \t}\n> >>>> \n> >>>> -\treturn data->video_->queueBuffer(buffer);\n> >>>> +\tif (data->useConverter_)\n> >>>> +\t\tdata->converterQueue_.push(std::move(buffers));\n> >>>> +\n> >>>> +\treturn 0;\n> >>>> }\n> >>>> \n> >>>> /* -----------------------------------------------------------------------------\n> >>>> @@ -1020,24 +1027,34 @@ void SimplePipelineHandler::bufferReady(FrameBuffer *buffer)\n> >>>> \t * point converting an erroneous buffer.\n> >>>> \t */\n> >>>> \tif (buffer->metadata().status != FrameMetadata::FrameSuccess) {\n> >>>> -\t\tif (data->useConverter_) {\n> >>>> -\t\t\t/* Requeue the buffer for capture. */\n> >>>> -\t\t\tdata->video_->queueBuffer(buffer);\n> >>>> +\t\tif (!data->useConverter_) {\n> >>>> +\t\t\t/* No conversion, just complete the request. */\n> >>>> +\t\t\tRequest *request = buffer->request();\n> >>>> +\t\t\tcompleteBuffer(request, buffer);\n> >>>> +\t\t\tcompleteRequest(request);\n> >>>> +\t\t\treturn;\n> >>>> +\t\t}\n> >>>> +\n> >>>> +\t\t/*\n> >>>> +\t\t * The converter is in use. Requeue the internal buffer for\n> >>>> +\t\t * capture, and complete the request with all the user-facing\n> >>>> +\t\t * buffers.\n> >>>> +\t\t */\n> >>>> +\t\tdata->video_->queueBuffer(buffer);\n> \n> Does this incorrectly requeue buffers if we're stopping? (I.e. if we\n> were 'FrameCancelled' or such?\n\nIndeed, I'll fix that.\n\n> >>>> \n> >>>> -\t\t\t/*\n> >>>> -\t\t\t * Get the next user-facing buffer to complete the\n> >>>> -\t\t\t * request.\n> >>>> -\t\t\t */\n> >>>> -\t\t\tif (data->converterQueue_.empty())\n> >>>> -\t\t\t\treturn;\n> >>>> +\t\tif (data->converterQueue_.empty())\n> >>>> +\t\t\treturn;\n> >>>> \n> >>>> -\t\t\tbuffer = data->converterQueue_.front();\n> >>>> -\t\t\tdata->converterQueue_.pop();\n> >>>> +\t\tRequest *request = nullptr;\n> >>>> +\t\tfor (auto &item : data->converterQueue_.front()) {\n> >>>> +\t\t\tFrameBuffer *outputBuffer = item.second;\n> >>>> +\t\t\trequest = outputBuffer->request();\n> >>>> +\t\t\tcompleteBuffer(request, outputBuffer);\n> >>>> \t\t}\n> >>>> +\t\tdata->converterQueue_.pop();\n> >>>> \n> >>>> -\t\tRequest *request = buffer->request();\n> >>>> -\t\tcompleteBuffer(request, buffer);\n> >>>> -\t\tcompleteRequest(request);\n> >>>> +\t\tif (request)\n> >>>\n> >>> This check doesn't seem necessary, as we return early if the\n> >>> converterQueue_ is empty, so the loop will always run once. Is it just\n> >>> to appease coverity?\n> >>\n> >> You're right, I'll drop that.\n> > \n> > Actually, we're not looping over data->converterQueue_, but over\n> > converterQueue_.front(). While it should never be empty, I think it will\n> > be difficult for compilers (and coverity) to know that, and a check here\n> > can also act as a bit of defensive programming. I think I'd prefer\n> > keeping the check.\n> \n> I presume at this point no buffer has been passed to any convertor, so\n> there can't be anything happening in parallel at this point.\n\nThat's correct.\n\n> I don't think there can be so:\n> \n> Reviewed-by: Kieran Bingham \n> \n> >>> Reviewed-by: Paul Elder \n> >>>\n> >>>> +\t\t\tcompleteRequest(request);\n> >>>> \t\treturn;\n> >>>> \t}\n> >>>> \n> >>>> @@ -1052,10 +1069,8 @@ void SimplePipelineHandler::bufferReady(FrameBuffer *buffer)\n> >>>> \t\t\treturn;\n> >>>> \t\t}\n> >>>> \n> >>>> -\t\tFrameBuffer *output = data->converterQueue_.front();\n> >>>> +\t\tconverter_->queueBuffers(buffer, data->converterQueue_.front());\n> >>>> \t\tdata->converterQueue_.pop();\n> >>>> -\n> >>>> -\t\tconverter_->queueBuffers(buffer, { { 0, output } });\n> >>>> \t\treturn;\n> >>>> \t}\n> >>>> \n> >>>> @@ -1078,10 +1093,10 @@ void SimplePipelineHandler::converterOutputDone(FrameBuffer *buffer)\n> >>>> {\n> >>>> \tASSERT(activeCamera_);\n> >>>> \n> >>>> -\t/* Complete the request. */\n> >>>> +\t/* Complete the buffer and the request. */\n> >>>> \tRequest *request = buffer->request();\n> >>>> -\tcompleteBuffer(request, buffer);\n> >>>> -\tcompleteRequest(request);\n> >>>> +\tif (completeBuffer(request, buffer))\n> >>>> +\t\tcompleteRequest(request);>>>> }\n> >>>> \n> >>>> REGISTER_PIPELINE_HANDLER(SimplePipelineHandler)","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id BE29DBD808\n\tfor ;\n\tTue, 2 Mar 2021 11:19:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 42E2568AA1;\n\tTue, 2 Mar 2021 12:19:38 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id AC3CC68A93\n\tfor ;\n\tTue, 2 Mar 2021 12:19:36 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 07D9E45D;\n\tTue, 2 Mar 2021 12:19:35 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"TfbmI8kv\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1614683976;\n\tbh=CqThZCMss0nB5o5tRRlh6HfSaHQSaM46NHDIPDQxVHg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=TfbmI8kvRcsnMFOJwSXVfs5vQxO2nKR7GAIDlnC2HgzmSkhM9duChD1R4/Sh6AJk6\n\twz3zDVgtrFbXMutm/g15ZRFj7JVrx/vYwpyBdb1pYZJ5H2yFPEYenC9QO8pAkyUIuv\n\tYumz8BU6W5C0wN0PNG5OUowpucINR9zp5ygHTXMc=","Date":"Tue, 2 Mar 2021 13:19:08 +0200","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"","References":"<20210131224702.8838-1-laurent.pinchart@ideasonboard.com>\n\t<20210131224702.8838-20-laurent.pinchart@ideasonboard.com>\n\t<20210302070933.GE35047@pyrite.rasen.tech>\n\t\n\t\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 19/20] libcamera: pipeline: simple:\n\tSupport usage of multiple streams","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"Phi-Bang Nguyen ,\n\tlibcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]