[{"id":29618,"web_url":"https://patchwork.libcamera.org/comment/29618/","msgid":"<171680554860.2626842.2879341746430648392@ping.linuxembedded.co.uk>","date":"2024-05-27T10:25:48","subject":"Re: [RFC PATCH] libcamera: converter: Replace usage of stream index\n\tby Stream pointer","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Umang Jain (2024-05-24 07:54:19)\n> The converter interface uses the unsigned int output stream index to map\n> to the output frame buffers. This is cumbersome to implement new\n> converters because one has to keep around additional book keeping\n> to track the streams with their correct indexes.\n> \n> This patch (still an RFC) intends to drop stream index usage.\n> The index is replaced by Stream pointer. This is convenient since\n> Stream pointers are easily accessible at configuration time (from\n> StreamConfiguration) and from Request objects.\n\nIt's a little awkward that the term Stream is used in multiple places\n(not your patch, the existing situation!)\n\nBut I think a convertor Stream does / should map to a libcamera::Stream.\n\n> The v4l2_converter_m2m and simple pipeline handler are adapt to\n> use the new interface. This work roped in software ISP as well,\n> which also seems to use indexes (although it doesn't implement converter\n> interface) because of a common conversionQueue_ queue used for\n> converter_ and swIsp_.\n> \n> Signed-off-by: Umang Jain \n> ---\n> \n> The patch is an RFC and needs more work to be finalised. I want to check\n> if this direction is decent enough to take since this is going to block\n> my DW100 converter rework.\n\nI think it more or less still makes sense! Could do with a check from\nthe SoftISP team to consider it from that side too.\n\n\n> \n> - [PATCH v2 0/4] libcamera: rkisp1: Plumb i.MX8MP DW100 dewarper\n> \n> - Some \\todos still around validation of outputs framebuffers.\n> - compile tested with -Dpipelines=rkisp1,simple \n> \n> ---\n> include/libcamera/internal/converter.h | 5 +-\n> .../internal/converter/converter_v4l2_m2m.h | 11 +++--\n> .../internal/software_isp/software_isp.h | 4 +-\n> .../converter/converter_v4l2_m2m.cpp | 47 +++++++++----------\n> src/libcamera/pipeline/simple/simple.cpp | 12 ++---\n> src/libcamera/software_isp/software_isp.cpp | 22 ++++-----\n> 6 files changed, 46 insertions(+), 55 deletions(-)\n> \n> diff --git a/include/libcamera/internal/converter.h b/include/libcamera/internal/converter.h\n> index 5d74db6b..b51563d7 100644\n> --- a/include/libcamera/internal/converter.h\n> +++ b/include/libcamera/internal/converter.h\n> @@ -26,6 +26,7 @@ namespace libcamera {\n> class FrameBuffer;\n> class MediaDevice;\n> class PixelFormat;\n> +class Stream;\n> struct StreamConfiguration;\n> \n> class Converter\n> @@ -46,14 +47,14 @@ public:\n> \n> virtual int configure(const StreamConfiguration &inputCfg,\n> const std::vector> &outputCfgs) = 0;\n> - virtual int exportBuffers(unsigned int output, unsigned int count,\n> + virtual int exportBuffers(const Stream *stream, unsigned int count,\n> std::vector> *buffers) = 0;\n> \n> virtual int start() = 0;\n> virtual void stop() = 0;\n> \n> virtual int queueBuffers(FrameBuffer *input,\n> - const std::map &outputs) = 0;\n> + const std::map &outputs) = 0;\n> \n> Signal inputBufferReady;\n> Signal outputBufferReady;\n> diff --git a/include/libcamera/internal/converter/converter_v4l2_m2m.h b/include/libcamera/internal/converter/converter_v4l2_m2m.h\n> index 1126050c..711a1a5f 100644\n> --- a/include/libcamera/internal/converter/converter_v4l2_m2m.h\n> +++ b/include/libcamera/internal/converter/converter_v4l2_m2m.h\n> @@ -28,6 +28,7 @@ class FrameBuffer;\n> class MediaDevice;\n> class Size;\n> class SizeRange;\n> +class Stream;\n> struct StreamConfiguration;\n> class V4L2M2MDevice;\n> \n> @@ -47,20 +48,20 @@ public:\n> \n> int configure(const StreamConfiguration &inputCfg,\n> const std::vector> &outputCfg);\n> - int exportBuffers(unsigned int output, unsigned int count,\n> + int exportBuffers(const Stream *stream, unsigned int count,\n> std::vector> *buffers);\n> \n> int start();\n> void stop();\n> \n> int queueBuffers(FrameBuffer *input,\n> - const std::map &outputs);\n> + const std::map &outputs);\n> \n> private:\n> class Stream : protected Loggable\n> {\n> public:\n> - Stream(V4L2M2MConverter *converter, unsigned int index);\n> + Stream(V4L2M2MConverter *converter, const libcamera::Stream *stream);\n> \n> bool isValid() const { return m2m_ != nullptr; }\n> \n> @@ -82,7 +83,7 @@ private:\n> void outputBufferReady(FrameBuffer *buffer);\n> \n> V4L2M2MConverter *converter_;\n> - unsigned int index_;\n> + const libcamera::Stream *stream_;\n> std::unique_ptr m2m_;\n> \n> unsigned int inputBufferCount_;\n> @@ -91,7 +92,7 @@ private:\n> \n> std::unique_ptr m2m_;\n> \n> - std::vector streams_;\n> + std::map streams_;\n\nThis is where it gets 'funny'. We have to map a stream to a stream ;+)\n\nI guess this is only used for indexing though.\n\n> std::map queue_;\n> };\n> \n> diff --git a/include/libcamera/internal/software_isp/software_isp.h b/include/libcamera/internal/software_isp/software_isp.h\n> index 7e9fae6a..be3d1b2f 100644\n> --- a/include/libcamera/internal/software_isp/software_isp.h\n> +++ b/include/libcamera/internal/software_isp/software_isp.h\n> @@ -62,7 +62,7 @@ public:\n> const std::vector> &outputCfgs,\n> const ControlInfoMap &sensorControls);\n> \n> - int exportBuffers(unsigned int output, unsigned int count,\n> + int exportBuffers(const Stream *stream, unsigned int count,\n> std::vector> *buffers);\n> \n> void processStats(const ControlList &sensorControls);\n> @@ -71,7 +71,7 @@ public:\n> void stop();\n> \n> int queueBuffers(FrameBuffer *input,\n> - const std::map &outputs);\n> + const std::map &outputs);\n> \n> void process(FrameBuffer *input, FrameBuffer *output);\n> \n> diff --git a/src/libcamera/converter/converter_v4l2_m2m.cpp b/src/libcamera/converter/converter_v4l2_m2m.cpp\n> index d8929fc5..e0619689 100644\n> --- a/src/libcamera/converter/converter_v4l2_m2m.cpp\n> +++ b/src/libcamera/converter/converter_v4l2_m2m.cpp\n> @@ -35,8 +35,8 @@ LOG_DECLARE_CATEGORY(Converter)\n> * V4L2M2MConverter::Stream\n> */\n> \n> -V4L2M2MConverter::Stream::Stream(V4L2M2MConverter *converter, unsigned int index)\n> - : converter_(converter), index_(index)\n> +V4L2M2MConverter::Stream::Stream(V4L2M2MConverter *converter, const libcamera::Stream *stream)\n> + : converter_(converter), stream_(stream)\n> {\n> m2m_ = std::make_unique(converter->deviceNode());\n> \n> @@ -157,7 +157,7 @@ int V4L2M2MConverter::Stream::queueBuffers(FrameBuffer *input, FrameBuffer *outp\n> \n> std::string V4L2M2MConverter::Stream::logPrefix() const\n> {\n> - return \"stream\" + std::to_string(index_);\n> + return stream_->configuration().toString();\n\nWhat does this produce? I'm not sure if this will be correct. The\nV4L2M2MConvertor might simply be only one 'component' of the\nlibcamera::Stream ?\n\n\n> }\n> \n> void V4L2M2MConverter::Stream::outputBufferReady(FrameBuffer *buffer)\n> @@ -333,10 +333,13 @@ int V4L2M2MConverter::configure(const StreamConfiguration &inputCfg,\n> int ret = 0;\n> \n> streams_.clear();\n> - streams_.reserve(outputCfgs.size());\n> \n> for (unsigned int i = 0; i < outputCfgs.size(); ++i) {\n> - Stream &stream = streams_.emplace_back(this, i);\n> + const StreamConfiguration &cfg = outputCfgs[i];\n> + streams_.emplace(cfg.stream(),\n> + Stream(this, cfg.stream()));\n> +\n> + Stream &stream = streams_.at(cfg.stream());\n> \n\nAha, but this clears one bit up for me. The convertor does deal with\nmultiple libcamera::Streams and they do map to the Stream instance\nabove.\n\n> if (!stream.isValid()) {\n> LOG(Converter, Error)\n> @@ -345,7 +348,7 @@ int V4L2M2MConverter::configure(const StreamConfiguration &inputCfg,\n> break;\n> }\n> \n> - ret = stream.configure(inputCfg, outputCfgs[i]);\n> + ret = stream.configure(inputCfg, cfg);\n> if (ret < 0)\n> break;\n> }\n> @@ -361,13 +364,14 @@ int V4L2M2MConverter::configure(const StreamConfiguration &inputCfg,\n> /**\n> * \\copydoc libcamera::Converter::exportBuffers\n> */\n> -int V4L2M2MConverter::exportBuffers(unsigned int output, unsigned int count,\n> +int V4L2M2MConverter::exportBuffers(const libcamera::Stream *stream, unsigned int count,\n> std::vector> *buffers)\n> {\n> - if (output >= streams_.size())\n> + auto iter = streams_.find(stream);\n> + if (iter == streams_.end())\n> return -EINVAL;\n> \n> - return streams_[output].exportBuffers(count, buffers);\n> + return iter->second.exportBuffers(count, buffers);\n> }\n> \n> /**\n> @@ -377,8 +381,8 @@ int V4L2M2MConverter::start()\n> {\n> int ret;\n> \n> - for (Stream &stream : streams_) {\n> - ret = stream.start();\n> + for (auto &iter : streams_) {\n> + ret = iter.second.start();\n> if (ret < 0) {\n> stop();\n> return ret;\n> @@ -393,17 +397,16 @@ int V4L2M2MConverter::start()\n> */\n> void V4L2M2MConverter::stop()\n> {\n> - for (Stream &stream : utils::reverse(streams_))\n> - stream.stop();\n> + for (auto &iter : streams_)\n> + iter.second.stop();\n> }\n> \n> /**\n> * \\copydoc libcamera::Converter::queueBuffers\n> */\n> int V4L2M2MConverter::queueBuffers(FrameBuffer *input,\n> - const std::map &outputs)\n> + const std::map &outputs)\n> {\n> - unsigned int mask = 0;\n> int ret;\n> \n> /*\n> @@ -414,20 +417,12 @@ int V4L2M2MConverter::queueBuffers(FrameBuffer *input,\n> if (outputs.empty())\n> return -EINVAL;\n> \n> - for (auto [index, buffer] : outputs) {\n> - if (!buffer)\n> - return -EINVAL;\n> - if (index >= streams_.size())\n> - return -EINVAL;\n> - if (mask & (1 << index))\n> - return -EINVAL;\n> \n> - mask |= 1 << index;\n> - }\n> + /* \\TODO: validate outputs against streams */\n> \n> /* Queue the input and output buffers to all the streams. */\n> - for (auto [index, buffer] : outputs) {\n> - ret = streams_[index].queueBuffers(input, buffer);\n> + for (auto [stream, buffer] : outputs) {\n> + ret = streams_.at(stream).queueBuffers(input, buffer);\n> if (ret < 0)\n> return ret;\n> }\n> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> index db3575c3..c22b2f89 100644\n> --- a/src/libcamera/pipeline/simple/simple.cpp\n> +++ b/src/libcamera/pipeline/simple/simple.cpp\n> @@ -278,7 +278,7 @@ public:\n> std::map> formats_;\n> \n> std::vector> conversionBuffers_;\n> - std::queue> conversionQueue_;\n> + std::queue> conversionQueue_;\n> bool useConversion_;\n> \n> std::unique_ptr converter_;\n> @@ -837,7 +837,7 @@ void SimpleCameraData::bufferReady(FrameBuffer *buffer)\n> Request *request = buffer->request();\n> \n> if (useConversion_ && !conversionQueue_.empty()) {\n> - const std::map &outputs =\n> + const std::map &outputs =\n> conversionQueue_.front();\n> if (!outputs.empty()) {\n> FrameBuffer *outputBuffer = outputs.begin()->second;\n> @@ -1304,9 +1304,9 @@ int SimplePipelineHandler::exportFrameBuffers(Camera *camera, Stream *stream,\n> */\n> if (data->useConversion_)\n> return data->converter_\n> - ? data->converter_->exportBuffers(data->streamIndex(stream),\n> + ? data->converter_->exportBuffers(stream,\n> count, buffers)\n> - : data->swIsp_->exportBuffers(data->streamIndex(stream),\n> + : data->swIsp_->exportBuffers(stream,\n> count, buffers);\n> else\n> return data->video_->exportBuffers(count, buffers);\n> @@ -1399,7 +1399,7 @@ int SimplePipelineHandler::queueRequestDevice(Camera *camera, Request *request)\n> SimpleCameraData *data = cameraData(camera);\n> int ret;\n> \n> - std::map buffers;\n> + std::map buffers;\n> \n> for (auto &[stream, buffer] : request->buffers()) {\n> /*\n> @@ -1408,7 +1408,7 @@ int SimplePipelineHandler::queueRequestDevice(Camera *camera, Request *request)\n> * completion handler.\n> */\n> if (data->useConversion_) {\n> - buffers.emplace(data->streamIndex(stream), buffer);\n> + buffers.emplace(stream, buffer);\n> } else {\n> ret = data->video_->queueBuffer(buffer);\n> if (ret < 0)\n> diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp\n> index c9b6be56..b81b90bd 100644\n> --- a/src/libcamera/software_isp/software_isp.cpp\n> +++ b/src/libcamera/software_isp/software_isp.cpp\n> @@ -227,13 +227,13 @@ int SoftwareIsp::configure(const StreamConfiguration &inputCfg,\n> * \\return The number of allocated buffers on success or a negative error code\n> * otherwise\n> */\n> -int SoftwareIsp::exportBuffers(unsigned int output, unsigned int count,\n> +int SoftwareIsp::exportBuffers(const Stream *stream, unsigned int count,\n> std::vector> *buffers)\n> {\n> ASSERT(debayer_ != nullptr);\n> \n> /* single output for now */\n> - if (output >= 1)\n> + if (stream != nullptr)\n> return -EINVAL;\n\nSurely this should be\n\tif (stream == nullptr)\n\nBut can that ever even happen? We can't get here with a null stream can\nwe ? (We shouldn't!)\n\nPerhaps there needs to be a check/mapping for each stream expected to be\nsupported - or maybe that will already be limited in\nconfigure()/validate().\n\n> for (unsigned int i = 0; i < count; i++) {\n> @@ -265,9 +265,8 @@ int SoftwareIsp::exportBuffers(unsigned int output, unsigned int count,\n> * \\return 0 on success, a negative errno on failure\n> */\n> int SoftwareIsp::queueBuffers(FrameBuffer *input,\n> - const std::map &outputs)\n> + const std::map &outputs)\n> {\n> - unsigned int mask = 0;\n> \n> /*\n> * Validate the outputs as a sanity check: at least one output is\n> @@ -277,18 +276,13 @@ int SoftwareIsp::queueBuffers(FrameBuffer *input,\n> if (outputs.empty())\n> return -EINVAL;\n> \n> - for (auto [index, buffer] : outputs) {\n> - if (!buffer)\n> - return -EINVAL;\n> - if (index >= 1) /* only single stream atm */\n> - return -EINVAL;\n> - if (mask & (1 << index))\n> - return -EINVAL;\n> + /* \\todo validate outputs against streams*/\n> \n> - mask |= 1 << index;\n> - }\n> + if (outputs.size() != 1) /* only single stream atm */\n> + return -EINVAL;\n> \n> - process(input, outputs.at(0));\n> + for (auto iter = outputs.begin(); iter != outputs.end(); iter++)\n> + process(input, iter->second);\n> \n> return 0;\n> }\n> -- \n> 2.44.0\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 016FDBD87C\n\tfor ;\n\tMon, 27 May 2024 10:26:02 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B45C5634AF;\n\tMon, 27 May 2024 12:26:01 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 631C1634AD\n\tfor ;\n\tMon, 27 May 2024 12:25:52 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 0E939471;\n\tMon, 27 May 2024 12:25:50 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"UGsx5EiA\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1716805550;\n\tbh=RmLhmHCx9L2/C9a+nmc6Eec3MUpC9VzlOHEDb2WdAgY=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=UGsx5EiAQd2K61Q47ILcxoXav7dvEr+zQBgHOzGn4Z/qyj1NQwwUJ8xZ4ea/jGV0U\n\tm23/S85AuUCJyH5EASbZ+pbrsjIRBJi5YCR4R5SrFnUUEjjZ8JqwaRmtqR8njHAq8W\n\tTTI3iLAyqi0F0YrtQbwLggdiCP3ch5pzHEcW0M/g=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20240524065419.94005-1-umang.jain@ideasonboard.com>","References":"<20240524065419.94005-1-umang.jain@ideasonboard.com>","Subject":"Re: [RFC PATCH] libcamera: converter: Replace usage of stream index\n\tby Stream pointer","From":"Kieran Bingham ","Cc":"Umang Jain ","To":"Umang Jain ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Mon, 27 May 2024 11:25:48 +0100","Message-ID":"<171680554860.2626842.2879341746430648392@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":29621,"web_url":"https://patchwork.libcamera.org/comment/29621/","msgid":"","date":"2024-05-27T12:05:01","subject":"Re: [RFC PATCH] libcamera: converter: Replace usage of stream index\n\tby Stream pointer","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Kieran,\n\nOn 27/05/24 3:55 pm, Kieran Bingham wrote:\n> Quoting Umang Jain (2024-05-24 07:54:19)\n>> The converter interface uses the unsigned int output stream index to map\n>> to the output frame buffers. This is cumbersome to implement new\n>> converters because one has to keep around additional book keeping\n>> to track the streams with their correct indexes.\n>>\n>> This patch (still an RFC) intends to drop stream index usage.\n>> The index is replaced by Stream pointer. This is convenient since\n>> Stream pointers are easily accessible at configuration time (from\n>> StreamConfiguration) and from Request objects.\n> It's a little awkward that the term Stream is used in multiple places\n> (not your patch, the existing situation!)\n>\n> But I think a convertor Stream does / should map to a libcamera::Stream.\n>\n>> The v4l2_converter_m2m and simple pipeline handler are adapt to\n>> use the new interface. This work roped in software ISP as well,\n>> which also seems to use indexes (although it doesn't implement converter\n>> interface) because of a common conversionQueue_ queue used for\n>> converter_ and swIsp_.\n>>\n>> Signed-off-by: Umang Jain \n>> ---\n>>\n>> The patch is an RFC and needs more work to be finalised. I want to check\n>> if this direction is decent enough to take since this is going to block\n>> my DW100 converter rework.\n> I think it more or less still makes sense! Could do with a check from\n> the SoftISP team to consider it from that side too.\n>\n>\n>> - [PATCH v2 0/4] libcamera: rkisp1: Plumb i.MX8MP DW100 dewarper\n>>\n>> - Some \\todos still around validation of outputs framebuffers.\n>> - compile tested with -Dpipelines=rkisp1,simple\n>>\n>> ---\n>> include/libcamera/internal/converter.h | 5 +-\n>> .../internal/converter/converter_v4l2_m2m.h | 11 +++--\n>> .../internal/software_isp/software_isp.h | 4 +-\n>> .../converter/converter_v4l2_m2m.cpp | 47 +++++++++----------\n>> src/libcamera/pipeline/simple/simple.cpp | 12 ++---\n>> src/libcamera/software_isp/software_isp.cpp | 22 ++++-----\n>> 6 files changed, 46 insertions(+), 55 deletions(-)\n>>\n>> diff --git a/include/libcamera/internal/converter.h b/include/libcamera/internal/converter.h\n>> index 5d74db6b..b51563d7 100644\n>> --- a/include/libcamera/internal/converter.h\n>> +++ b/include/libcamera/internal/converter.h\n>> @@ -26,6 +26,7 @@ namespace libcamera {\n>> class FrameBuffer;\n>> class MediaDevice;\n>> class PixelFormat;\n>> +class Stream;\n>> struct StreamConfiguration;\n>> \n>> class Converter\n>> @@ -46,14 +47,14 @@ public:\n>> \n>> virtual int configure(const StreamConfiguration &inputCfg,\n>> const std::vector> &outputCfgs) = 0;\n>> - virtual int exportBuffers(unsigned int output, unsigned int count,\n>> + virtual int exportBuffers(const Stream *stream, unsigned int count,\n>> std::vector> *buffers) = 0;\n>> \n>> virtual int start() = 0;\n>> virtual void stop() = 0;\n>> \n>> virtual int queueBuffers(FrameBuffer *input,\n>> - const std::map &outputs) = 0;\n>> + const std::map &outputs) = 0;\n>> \n>> Signal inputBufferReady;\n>> Signal outputBufferReady;\n>> diff --git a/include/libcamera/internal/converter/converter_v4l2_m2m.h b/include/libcamera/internal/converter/converter_v4l2_m2m.h\n>> index 1126050c..711a1a5f 100644\n>> --- a/include/libcamera/internal/converter/converter_v4l2_m2m.h\n>> +++ b/include/libcamera/internal/converter/converter_v4l2_m2m.h\n>> @@ -28,6 +28,7 @@ class FrameBuffer;\n>> class MediaDevice;\n>> class Size;\n>> class SizeRange;\n>> +class Stream;\n>> struct StreamConfiguration;\n>> class V4L2M2MDevice;\n>> \n>> @@ -47,20 +48,20 @@ public:\n>> \n>> int configure(const StreamConfiguration &inputCfg,\n>> const std::vector> &outputCfg);\n>> - int exportBuffers(unsigned int output, unsigned int count,\n>> + int exportBuffers(const Stream *stream, unsigned int count,\n>> std::vector> *buffers);\n>> \n>> int start();\n>> void stop();\n>> \n>> int queueBuffers(FrameBuffer *input,\n>> - const std::map &outputs);\n>> + const std::map &outputs);\n>> \n>> private:\n>> class Stream : protected Loggable\n>> {\n>> public:\n>> - Stream(V4L2M2MConverter *converter, unsigned int index);\n>> + Stream(V4L2M2MConverter *converter, const libcamera::Stream *stream);\n>> \n>> bool isValid() const { return m2m_ != nullptr; }\n>> \n>> @@ -82,7 +83,7 @@ private:\n>> void outputBufferReady(FrameBuffer *buffer);\n>> \n>> V4L2M2MConverter *converter_;\n>> - unsigned int index_;\n>> + const libcamera::Stream *stream_;\n>> std::unique_ptr m2m_;\n>> \n>> unsigned int inputBufferCount_;\n>> @@ -91,7 +92,7 @@ private:\n>> \n>> std::unique_ptr m2m_;\n>> \n>> - std::vector streams_;\n>> + std::map streams_;\n> This is where it gets 'funny'. We have to map a stream to a stream ;+)\n\nThis is mapping a libcamera::Stream to a V4L2M2MConverter::Stream. The \nlack of namespace is confusing things, I am tempted to change it.\n>\n> I guess this is only used for indexing though.\n>\n>> std::map queue_;\n>> };\n>> \n>> diff --git a/include/libcamera/internal/software_isp/software_isp.h b/include/libcamera/internal/software_isp/software_isp.h\n>> index 7e9fae6a..be3d1b2f 100644\n>> --- a/include/libcamera/internal/software_isp/software_isp.h\n>> +++ b/include/libcamera/internal/software_isp/software_isp.h\n>> @@ -62,7 +62,7 @@ public:\n>> const std::vector> &outputCfgs,\n>> const ControlInfoMap &sensorControls);\n>> \n>> - int exportBuffers(unsigned int output, unsigned int count,\n>> + int exportBuffers(const Stream *stream, unsigned int count,\n>> std::vector> *buffers);\n>> \n>> void processStats(const ControlList &sensorControls);\n>> @@ -71,7 +71,7 @@ public:\n>> void stop();\n>> \n>> int queueBuffers(FrameBuffer *input,\n>> - const std::map &outputs);\n>> + const std::map &outputs);\n>> \n>> void process(FrameBuffer *input, FrameBuffer *output);\n>> \n>> diff --git a/src/libcamera/converter/converter_v4l2_m2m.cpp b/src/libcamera/converter/converter_v4l2_m2m.cpp\n>> index d8929fc5..e0619689 100644\n>> --- a/src/libcamera/converter/converter_v4l2_m2m.cpp\n>> +++ b/src/libcamera/converter/converter_v4l2_m2m.cpp\n>> @@ -35,8 +35,8 @@ LOG_DECLARE_CATEGORY(Converter)\n>> * V4L2M2MConverter::Stream\n>> */\n>> \n>> -V4L2M2MConverter::Stream::Stream(V4L2M2MConverter *converter, unsigned int index)\n>> - : converter_(converter), index_(index)\n>> +V4L2M2MConverter::Stream::Stream(V4L2M2MConverter *converter, const libcamera::Stream *stream)\n>> + : converter_(converter), stream_(stream)\n>> {\n>> m2m_ = std::make_unique(converter->deviceNode());\n>> \n>> @@ -157,7 +157,7 @@ int V4L2M2MConverter::Stream::queueBuffers(FrameBuffer *input, FrameBuffer *outp\n>> \n>> std::string V4L2M2MConverter::Stream::logPrefix() const\n>> {\n>> - return \"stream\" + std::to_string(index_);\n>> + return stream_->configuration().toString();\n> What does this produce? I'm not sure if this will be correct. The\n> V4L2M2MConvertor might simply be only one 'component' of the\n> libcamera::Stream ?\n\nI don't understand here.\n>\n>\n>> }\n>> \n>> void V4L2M2MConverter::Stream::outputBufferReady(FrameBuffer *buffer)\n>> @@ -333,10 +333,13 @@ int V4L2M2MConverter::configure(const StreamConfiguration &inputCfg,\n>> int ret = 0;\n>> \n>> streams_.clear();\n>> - streams_.reserve(outputCfgs.size());\n>> \n>> for (unsigned int i = 0; i < outputCfgs.size(); ++i) {\n>> - Stream &stream = streams_.emplace_back(this, i);\n>> + const StreamConfiguration &cfg = outputCfgs[i];\n>> + streams_.emplace(cfg.stream(),\n>> + Stream(this, cfg.stream()));\n>> +\n>> + Stream &stream = streams_.at(cfg.stream());\n>> \n> Aha, but this clears one bit up for me. The convertor does deal with\n> multiple libcamera::Streams and they do map to the Stream instance\n> above.\n>\n>> if (!stream.isValid()) {\n>> LOG(Converter, Error)\n>> @@ -345,7 +348,7 @@ int V4L2M2MConverter::configure(const StreamConfiguration &inputCfg,\n>> break;\n>> }\n>> \n>> - ret = stream.configure(inputCfg, outputCfgs[i]);\n>> + ret = stream.configure(inputCfg, cfg);\n>> if (ret < 0)\n>> break;\n>> }\n>> @@ -361,13 +364,14 @@ int V4L2M2MConverter::configure(const StreamConfiguration &inputCfg,\n>> /**\n>> * \\copydoc libcamera::Converter::exportBuffers\n>> */\n>> -int V4L2M2MConverter::exportBuffers(unsigned int output, unsigned int count,\n>> +int V4L2M2MConverter::exportBuffers(const libcamera::Stream *stream, unsigned int count,\n>> std::vector> *buffers)\n>> {\n>> - if (output >= streams_.size())\n>> + auto iter = streams_.find(stream);\n>> + if (iter == streams_.end())\n>> return -EINVAL;\n>> \n>> - return streams_[output].exportBuffers(count, buffers);\n>> + return iter->second.exportBuffers(count, buffers);\n>> }\n>> \n>> /**\n>> @@ -377,8 +381,8 @@ int V4L2M2MConverter::start()\n>> {\n>> int ret;\n>> \n>> - for (Stream &stream : streams_) {\n>> - ret = stream.start();\n>> + for (auto &iter : streams_) {\n>> + ret = iter.second.start();\n>> if (ret < 0) {\n>> stop();\n>> return ret;\n>> @@ -393,17 +397,16 @@ int V4L2M2MConverter::start()\n>> */\n>> void V4L2M2MConverter::stop()\n>> {\n>> - for (Stream &stream : utils::reverse(streams_))\n>> - stream.stop();\n>> + for (auto &iter : streams_)\n>> + iter.second.stop();\n>> }\n>> \n>> /**\n>> * \\copydoc libcamera::Converter::queueBuffers\n>> */\n>> int V4L2M2MConverter::queueBuffers(FrameBuffer *input,\n>> - const std::map &outputs)\n>> + const std::map &outputs)\n>> {\n>> - unsigned int mask = 0;\n>> int ret;\n>> \n>> /*\n>> @@ -414,20 +417,12 @@ int V4L2M2MConverter::queueBuffers(FrameBuffer *input,\n>> if (outputs.empty())\n>> return -EINVAL;\n>> \n>> - for (auto [index, buffer] : outputs) {\n>> - if (!buffer)\n>> - return -EINVAL;\n>> - if (index >= streams_.size())\n>> - return -EINVAL;\n>> - if (mask & (1 << index))\n>> - return -EINVAL;\n>> \n>> - mask |= 1 << index;\n>> - }\n>> + /* \\TODO: validate outputs against streams */\n>> \n>> /* Queue the input and output buffers to all the streams. */\n>> - for (auto [index, buffer] : outputs) {\n>> - ret = streams_[index].queueBuffers(input, buffer);\n>> + for (auto [stream, buffer] : outputs) {\n>> + ret = streams_.at(stream).queueBuffers(input, buffer);\n>> if (ret < 0)\n>> return ret;\n>> }\n>> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n>> index db3575c3..c22b2f89 100644\n>> --- a/src/libcamera/pipeline/simple/simple.cpp\n>> +++ b/src/libcamera/pipeline/simple/simple.cpp\n>> @@ -278,7 +278,7 @@ public:\n>> std::map> formats_;\n>> \n>> std::vector> conversionBuffers_;\n>> - std::queue> conversionQueue_;\n>> + std::queue> conversionQueue_;\n>> bool useConversion_;\n>> \n>> std::unique_ptr converter_;\n>> @@ -837,7 +837,7 @@ void SimpleCameraData::bufferReady(FrameBuffer *buffer)\n>> Request *request = buffer->request();\n>> \n>> if (useConversion_ && !conversionQueue_.empty()) {\n>> - const std::map &outputs =\n>> + const std::map &outputs =\n>> conversionQueue_.front();\n>> if (!outputs.empty()) {\n>> FrameBuffer *outputBuffer = outputs.begin()->second;\n>> @@ -1304,9 +1304,9 @@ int SimplePipelineHandler::exportFrameBuffers(Camera *camera, Stream *stream,\n>> */\n>> if (data->useConversion_)\n>> return data->converter_\n>> - ? data->converter_->exportBuffers(data->streamIndex(stream),\n>> + ? data->converter_->exportBuffers(stream,\n>> count, buffers)\n>> - : data->swIsp_->exportBuffers(data->streamIndex(stream),\n>> + : data->swIsp_->exportBuffers(stream,\n>> count, buffers);\n>> else\n>> return data->video_->exportBuffers(count, buffers);\n>> @@ -1399,7 +1399,7 @@ int SimplePipelineHandler::queueRequestDevice(Camera *camera, Request *request)\n>> SimpleCameraData *data = cameraData(camera);\n>> int ret;\n>> \n>> - std::map buffers;\n>> + std::map buffers;\n>> \n>> for (auto &[stream, buffer] : request->buffers()) {\n>> /*\n>> @@ -1408,7 +1408,7 @@ int SimplePipelineHandler::queueRequestDevice(Camera *camera, Request *request)\n>> * completion handler.\n>> */\n>> if (data->useConversion_) {\n>> - buffers.emplace(data->streamIndex(stream), buffer);\n>> + buffers.emplace(stream, buffer);\n>> } else {\n>> ret = data->video_->queueBuffer(buffer);\n>> if (ret < 0)\n>> diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp\n>> index c9b6be56..b81b90bd 100644\n>> --- a/src/libcamera/software_isp/software_isp.cpp\n>> +++ b/src/libcamera/software_isp/software_isp.cpp\n>> @@ -227,13 +227,13 @@ int SoftwareIsp::configure(const StreamConfiguration &inputCfg,\n>> * \\return The number of allocated buffers on success or a negative error code\n>> * otherwise\n>> */\n>> -int SoftwareIsp::exportBuffers(unsigned int output, unsigned int count,\n>> +int SoftwareIsp::exportBuffers(const Stream *stream, unsigned int count,\n>> std::vector> *buffers)\n>> {\n>> ASSERT(debayer_ != nullptr);\n>> \n>> /* single output for now */\n>> - if (output >= 1)\n>> + if (stream != nullptr)\n>> return -EINVAL;\n> Surely this should be\n> \tif (stream == nullptr)\n\nah yes.\n> But can that ever even happen? We can't get here with a null stream can\n> we ? (We shouldn't!)\n\nI am not sure, since the softISP is not using the converter interface, \nso things are left to will.\n>\n> Perhaps there needs to be a check/mapping for each stream expected to be\n> supported - or maybe that will already be limited in\n> configure()/validate().\n>\n>> for (unsigned int i = 0; i < count; i++) {\n>> @@ -265,9 +265,8 @@ int SoftwareIsp::exportBuffers(unsigned int output, unsigned int count,\n>> * \\return 0 on success, a negative errno on failure\n>> */\n>> int SoftwareIsp::queueBuffers(FrameBuffer *input,\n>> - const std::map &outputs)\n>> + const std::map &outputs)\n>> {\n>> - unsigned int mask = 0;\n>> \n>> /*\n>> * Validate the outputs as a sanity check: at least one output is\n>> @@ -277,18 +276,13 @@ int SoftwareIsp::queueBuffers(FrameBuffer *input,\n>> if (outputs.empty())\n>> return -EINVAL;\n>> \n>> - for (auto [index, buffer] : outputs) {\n>> - if (!buffer)\n>> - return -EINVAL;\n>> - if (index >= 1) /* only single stream atm */\n>> - return -EINVAL;\n>> - if (mask & (1 << index))\n>> - return -EINVAL;\n>> + /* \\todo validate outputs against streams*/\n>> \n>> - mask |= 1 << index;\n>> - }\n>> + if (outputs.size() != 1) /* only single stream atm */\n>> + return -EINVAL;\n>> \n>> - process(input, outputs.at(0));\n>> + for (auto iter = outputs.begin(); iter != outputs.end(); iter++)\n>> + process(input, iter->second);\n>> \n>> return 0;\n>> }\n>> -- \n>> 2.44.0\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 04383BDE6B\n\tfor ;\n\tMon, 27 May 2024 12:05:11 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6983C634B1;\n\tMon, 27 May 2024 14:05:10 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 06658634AD\n\tfor ;\n\tMon, 27 May 2024 14:05:08 +0200 (CEST)","from [IPV6:2409:4055:4e99:4251:f523:b915:4362:f847] (unknown\n\t[IPv6:2409:4055:4e99:4251:f523:b915:4362:f847])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 9B6F6710;\n\tMon, 27 May 2024 14:05:05 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"vYPlJwJf\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1716811506;\n\tbh=PCl0OU+qq3GzHdevF+qvl8nz3i9yFlY9Yned6gsJSyc=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=vYPlJwJf1DAc6/0XcPsz8MREfxieO9XHEDFmInYPLeqsyKVUiE4m5Y2LxjKdqFKq/\n\tSUp/4hJTH5TGhUk4lA4AXECC7HEDVkKuqtYU4KTrpbq7GXGmLFH0jObHLYnwYDVf8s\n\th+wck6FzgNxQdeEPXUiEFXibr39443Rsbna++gCQ=","Message-ID":"","Date":"Mon, 27 May 2024 17:35:01 +0530","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [RFC PATCH] libcamera: converter: Replace usage of stream index\n\tby Stream pointer","Content-Language":"en-US","To":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20240524065419.94005-1-umang.jain@ideasonboard.com>\n\t<171680554860.2626842.2879341746430648392@ping.linuxembedded.co.uk>","From":"Umang Jain ","In-Reply-To":"<171680554860.2626842.2879341746430648392@ping.linuxembedded.co.uk>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]