[{"id":29695,"web_url":"https://patchwork.libcamera.org/comment/29695/","msgid":"<171715521072.1000377.5290665602081381455@ping.linuxembedded.co.uk>","date":"2024-05-31T11:33:30","subject":"Re: [PATCH v3 4/4] libcamera: converter: Replace usage of stream\n\tindex by 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-31 06:25:05)\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> The v4l2_converter_m2m and simple pipeline handler are adapted 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> include/libcamera/internal/converter.h | 5 ++-\n> .../internal/converter/converter_v4l2_m2m.h | 11 ++---\n> .../internal/software_isp/software_isp.h | 5 ++-\n> src/libcamera/converter.cpp | 6 +--\n> .../converter/converter_v4l2_m2m.cpp | 40 ++++++++++---------\n> src/libcamera/pipeline/simple/simple.cpp | 14 +++----\n> src/libcamera/software_isp/software_isp.cpp | 17 ++++----\n> 7 files changed, 51 insertions(+), 47 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 0da62290..58fd19db 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 V4L2M2MStream : protected Loggable\n> {\n> public:\n> - V4L2M2MStream(V4L2M2MConverter *converter, unsigned int index);\n> + V4L2M2MStream(V4L2M2MConverter *converter, const 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 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> 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..09cc0f6f 100644\n> --- a/include/libcamera/internal/software_isp/software_isp.h\n> +++ b/include/libcamera/internal/software_isp/software_isp.h\n> @@ -37,6 +37,7 @@ namespace libcamera {\n> class DebayerCpu;\n> class FrameBuffer;\n> class PixelFormat;\n> +class Stream;\n> struct StreamConfiguration;\n> \n> LOG_DECLARE_CATEGORY(SoftwareIsp)\n> @@ -62,7 +63,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 +72,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.cpp b/src/libcamera/converter.cpp\n> index d3d38c1b..c5e38937 100644\n> --- a/src/libcamera/converter.cpp\n> +++ b/src/libcamera/converter.cpp\n> @@ -111,12 +111,12 @@ Converter::~Converter()\n> /**\n> * \\fn Converter::exportBuffers()\n> * \\brief Export buffers from the converter device\n> - * \\param[in] output Output stream index exporting the buffers\n> + * \\param[in] stream Output stream pointer exporting the buffers\n> * \\param[in] count Number of buffers to allocate\n> * \\param[out] buffers Vector to store allocated buffers\n> *\n> * This function operates similarly to V4L2VideoDevice::exportBuffers() on the\n> - * output stream indicated by the \\a output index.\n> + * output stream indicated by the \\a output.\n> *\n> * \\return The number of allocated buffers on success or a negative error code\n> * otherwise\n> @@ -137,7 +137,7 @@ Converter::~Converter()\n> * \\fn Converter::queueBuffers()\n> * \\brief Queue buffers to converter device\n> * \\param[in] input The frame buffer to apply the conversion\n> - * \\param[out] outputs The container holding the output stream indexes and\n> + * \\param[out] outputs The container holding the output stream pointer and\n> * their respective frame buffer outputs.\n> *\n> * This function queues the \\a input frame buffer on the output streams of the\n> diff --git a/src/libcamera/converter/converter_v4l2_m2m.cpp b/src/libcamera/converter/converter_v4l2_m2m.cpp\n> index 6309a0c0..a48b2a87 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::V4L2M2MStream\n> */\n> \n> -V4L2M2MConverter::V4L2M2MStream::V4L2M2MStream(V4L2M2MConverter *converter, unsigned int index)\n> - : converter_(converter), index_(index)\n> +V4L2M2MConverter::V4L2M2MStream::V4L2M2MStream(V4L2M2MConverter *converter, const Stream *stream)\n> + : converter_(converter), stream_(stream)\n> {\n> m2m_ = std::make_unique(converter->deviceNode());\n> \n> @@ -157,7 +157,7 @@ int V4L2M2MConverter::V4L2M2MStream::queueBuffers(FrameBuffer *input, FrameBuffe\n> \n> std::string V4L2M2MConverter::V4L2M2MStream::logPrefix() const\n> {\n> - return \"stream\" + std::to_string(index_);\n> + return stream_->configuration().toString();\n\nI'm still weary about this. What does it print ?\n\nThough the stream config might be useful to convey which convertor is\nbeing used if there are multiple streams - but I think it should really\nbe more tied to what the /convertor/ is rather than the stream. Maybe we\nshould drop the logPrefix or maybe do something better here.\n\nBut I don't mind if this is done on top or later though.\n\n\n> }\n> \n> void V4L2M2MConverter::V4L2M2MStream::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\nWhy is this line removed?\n\n\n> for (unsigned int i = 0; i < outputCfgs.size(); ++i) {\n> - V4L2M2MStream &stream = streams_.emplace_back(this, i);\n> + const StreamConfiguration &cfg = outputCfgs[i];\n> + streams_.emplace(cfg.stream(),\n> + V4L2M2MStream(this, cfg.stream()));\n> +\n> + V4L2M2MStream &stream = streams_.at(cfg.stream());\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 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 (V4L2M2MStream &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,15 +397,15 @@ int V4L2M2MConverter::start()\n> */\n> void V4L2M2MConverter::stop()\n> {\n> - for (V4L2M2MStream &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> std::set outputBufs;\n> int ret;\n> @@ -414,11 +418,9 @@ int V4L2M2MConverter::queueBuffers(FrameBuffer *input,\n> if (outputs.empty())\n> return -EINVAL;\n> \n> - for (auto [index, buffer] : outputs) {\n> + for (auto [stream, buffer] : outputs) {\n> if (!buffer)\n> return -EINVAL;\n> - if (index >= streams_.size())\n> - return -EINVAL;\n> \n> outputBufs.insert(buffer);\n> }\n> @@ -427,8 +429,8 @@ int V4L2M2MConverter::queueBuffers(FrameBuffer *input,\n> return -EINVAL;\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..01ad91a7 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,10 +1304,8 @@ int SimplePipelineHandler::exportFrameBuffers(Camera *camera, Stream *stream,\n> */\n> if (data->useConversion_)\n> return data->converter_\n> - ? data->converter_->exportBuffers(data->streamIndex(stream),\n> - count, buffers)\n> - : data->swIsp_->exportBuffers(data->streamIndex(stream),\n> - count, buffers);\n> + ? data->converter_->exportBuffers(stream, count, buffers)\n> + : data->swIsp_->exportBuffers(stream, count, buffers);\n> else\n> return data->video_->exportBuffers(count, buffers);\n> }\n> @@ -1399,7 +1397,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 +1406,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 ac10d82d..fe0946d7 100644\n> --- a/src/libcamera/software_isp/software_isp.cpp\n> +++ b/src/libcamera/software_isp/software_isp.cpp\n> @@ -221,19 +221,19 @@ int SoftwareIsp::configure(const StreamConfiguration &inputCfg,\n> \n> /**\n> * \\brief Export the buffers from the Software ISP\n> - * \\param[in] output Output stream index exporting the buffers\n> + * \\param[in] stream Output stream exporting the buffers\n> * \\param[in] count Number of buffers to allocate\n> * \\param[out] buffers Vector to store the allocated buffers\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> \n> for (unsigned int i = 0; i < count; i++) {\n> @@ -260,12 +260,12 @@ int SoftwareIsp::exportBuffers(unsigned int output, unsigned int count,\n> /**\n> * \\brief Queue buffers to Software ISP\n> * \\param[in] input The input framebuffer\n> - * \\param[in] outputs The container holding the output stream indexes and\n> + * \\param[in] outputs The container holding the output stream pointer and\n> * their respective frame buffer outputs\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> /*\n> * Validate the outputs as a sanity check: at least one output is\n> @@ -274,14 +274,15 @@ int SoftwareIsp::queueBuffers(FrameBuffer *input,\n> if (outputs.empty())\n> return -EINVAL;\n> \n> - for (auto [index, buffer] : outputs) {\n> + for (auto [stream, buffer] : outputs) {\n> if (!buffer)\n> return -EINVAL;\n> - if (index >= 1) /* only single stream atm */\n> + if (outputs.size() != 1) /* only single stream atm */\n\nI still think this \"can't\" happen, or should have been prevented during\nconfigure()/validate() but it's existing code so it's fine to just\nupdate - but I think this error check could be dropped sometime.\n\n\nI think the comments above are relatively minor (the dropped reserve\nseems like something that could be kept? But it's not so harmful if\nthere's a legitimate reason to drop it...)\n\nSo\n\nReviewed-by: Kieran Bingham \n\nand lets see if anyone else has anything more to add.\n\n--\nKieran\n\n> return -EINVAL;\n> }\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 7F8ABBD87C\n\tfor ;\n\tFri, 31 May 2024 11:33:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 48134634B8;\n\tFri, 31 May 2024 13:33:35 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8FE10634AF\n\tfor ;\n\tFri, 31 May 2024 13:33:33 +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 6E22CE22;\n\tFri, 31 May 2024 13:33:28 +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=\"Lz3k5qto\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1717155208;\n\tbh=5DGgJU0tHgYPzHfPcRlw/W2/h3owLQtUVvM3Zfj7vdg=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=Lz3k5qtoc71IFUkdfVLTa2mntzetGbJ8DHu7GT0p2XUE73MqptUiTZ2RpxRt2Krd2\n\tDmYydKhkwAqXZ+XRjsJx/a1KttSR+Y9NmtLUiE4h2/LK/3ow65y0p+LYUuZa87KmYH\n\te3+yU8OmzRBCLdjMdNlYIjYBpFz+W8JRsJXSc4DA=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20240531052505.150921-5-umang.jain@ideasonboard.com>","References":"<20240531052505.150921-1-umang.jain@ideasonboard.com>\n\t<20240531052505.150921-5-umang.jain@ideasonboard.com>","Subject":"Re: [PATCH v3 4/4] libcamera: converter: Replace usage of stream\n\tindex by Stream pointer","From":"Kieran Bingham ","Cc":"Milan Zamazal ,\n\tAndrey Konovalov ,\n\tUmang Jain ","To":"Umang Jain ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Fri, 31 May 2024 12:33:30 +0100","Message-ID":"<171715521072.1000377.5290665602081381455@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":29856,"web_url":"https://patchwork.libcamera.org/comment/29856/","msgid":"<0706ba0d-d568-4619-9cea-5bff23fa5b7a@ideasonboard.com>","date":"2024-06-12T05:34:56","subject":"Re: [PATCH v3 4/4] libcamera: converter: Replace usage of stream\n\tindex by 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 31/05/24 5:03 pm, Kieran Bingham wrote:\n> Quoting Umang Jain (2024-05-31 06:25:05)\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>> The v4l2_converter_m2m and simple pipeline handler are adapted 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>> include/libcamera/internal/converter.h | 5 ++-\n>> .../internal/converter/converter_v4l2_m2m.h | 11 ++---\n>> .../internal/software_isp/software_isp.h | 5 ++-\n>> src/libcamera/converter.cpp | 6 +--\n>> .../converter/converter_v4l2_m2m.cpp | 40 ++++++++++---------\n>> src/libcamera/pipeline/simple/simple.cpp | 14 +++----\n>> src/libcamera/software_isp/software_isp.cpp | 17 ++++----\n>> 7 files changed, 51 insertions(+), 47 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 0da62290..58fd19db 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 V4L2M2MStream : protected Loggable\n>> {\n>> public:\n>> - V4L2M2MStream(V4L2M2MConverter *converter, unsigned int index);\n>> + V4L2M2MStream(V4L2M2MConverter *converter, const 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 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>> 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..09cc0f6f 100644\n>> --- a/include/libcamera/internal/software_isp/software_isp.h\n>> +++ b/include/libcamera/internal/software_isp/software_isp.h\n>> @@ -37,6 +37,7 @@ namespace libcamera {\n>> class DebayerCpu;\n>> class FrameBuffer;\n>> class PixelFormat;\n>> +class Stream;\n>> struct StreamConfiguration;\n>> \n>> LOG_DECLARE_CATEGORY(SoftwareIsp)\n>> @@ -62,7 +63,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 +72,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.cpp b/src/libcamera/converter.cpp\n>> index d3d38c1b..c5e38937 100644\n>> --- a/src/libcamera/converter.cpp\n>> +++ b/src/libcamera/converter.cpp\n>> @@ -111,12 +111,12 @@ Converter::~Converter()\n>> /**\n>> * \\fn Converter::exportBuffers()\n>> * \\brief Export buffers from the converter device\n>> - * \\param[in] output Output stream index exporting the buffers\n>> + * \\param[in] stream Output stream pointer exporting the buffers\n>> * \\param[in] count Number of buffers to allocate\n>> * \\param[out] buffers Vector to store allocated buffers\n>> *\n>> * This function operates similarly to V4L2VideoDevice::exportBuffers() on the\n>> - * output stream indicated by the \\a output index.\n>> + * output stream indicated by the \\a output.\n>> *\n>> * \\return The number of allocated buffers on success or a negative error code\n>> * otherwise\n>> @@ -137,7 +137,7 @@ Converter::~Converter()\n>> * \\fn Converter::queueBuffers()\n>> * \\brief Queue buffers to converter device\n>> * \\param[in] input The frame buffer to apply the conversion\n>> - * \\param[out] outputs The container holding the output stream indexes and\n>> + * \\param[out] outputs The container holding the output stream pointer and\n>> * their respective frame buffer outputs.\n>> *\n>> * This function queues the \\a input frame buffer on the output streams of the\n>> diff --git a/src/libcamera/converter/converter_v4l2_m2m.cpp b/src/libcamera/converter/converter_v4l2_m2m.cpp\n>> index 6309a0c0..a48b2a87 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::V4L2M2MStream\n>> */\n>> \n>> -V4L2M2MConverter::V4L2M2MStream::V4L2M2MStream(V4L2M2MConverter *converter, unsigned int index)\n>> - : converter_(converter), index_(index)\n>> +V4L2M2MConverter::V4L2M2MStream::V4L2M2MStream(V4L2M2MConverter *converter, const Stream *stream)\n>> + : converter_(converter), stream_(stream)\n>> {\n>> m2m_ = std::make_unique(converter->deviceNode());\n>> \n>> @@ -157,7 +157,7 @@ int V4L2M2MConverter::V4L2M2MStream::queueBuffers(FrameBuffer *input, FrameBuffe\n>> \n>> std::string V4L2M2MConverter::V4L2M2MStream::logPrefix() const\n>> {\n>> - return \"stream\" + std::to_string(index_);\n>> + return stream_->configuration().toString();\n> I'm still weary about this. What does it print ?\n>\n> Though the stream config might be useful to convey which convertor is\n> being used if there are multiple streams - but I think it should really\n> be more tied to what the /convertor/ is rather than the stream. Maybe we\n> should drop the logPrefix or maybe do something better here.\n\nThis should print the entire stream configuration of the Converter logs, \nno ?  Earlier it would print \"stream0\" or \"stream1\" which is less \nexpressive IMO\n\nI don't have a opinion on this right now, but there might be a better \noption.\n>\n> But I don't mind if this is done on top or later though.\n\nSeconded. If all agrees...\n>\n>\n>> }\n>> \n>> void V4L2M2MConverter::V4L2M2MStream::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> Why is this line removed?\n\nThis is removed because streams_ is not a std::vector anymore (see the \nchange above in the patch). streams_ is now a std::map, for which \nreserve() is non-existent (rather not makes sense).\n>\n>\n>> for (unsigned int i = 0; i < outputCfgs.size(); ++i) {\n>> - V4L2M2MStream &stream = streams_.emplace_back(this, i);\n>> + const StreamConfiguration &cfg = outputCfgs[i];\n>> + streams_.emplace(cfg.stream(),\n>> + V4L2M2MStream(this, cfg.stream()));\n>> +\n>> + V4L2M2MStream &stream = streams_.at(cfg.stream());\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 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 (V4L2M2MStream &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,15 +397,15 @@ int V4L2M2MConverter::start()\n>> */\n>> void V4L2M2MConverter::stop()\n>> {\n>> - for (V4L2M2MStream &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>> std::set outputBufs;\n>> int ret;\n>> @@ -414,11 +418,9 @@ int V4L2M2MConverter::queueBuffers(FrameBuffer *input,\n>> if (outputs.empty())\n>> return -EINVAL;\n>> \n>> - for (auto [index, buffer] : outputs) {\n>> + for (auto [stream, buffer] : outputs) {\n>> if (!buffer)\n>> return -EINVAL;\n>> - if (index >= streams_.size())\n>> - return -EINVAL;\n>> \n>> outputBufs.insert(buffer);\n>> }\n>> @@ -427,8 +429,8 @@ int V4L2M2MConverter::queueBuffers(FrameBuffer *input,\n>> return -EINVAL;\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..01ad91a7 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,10 +1304,8 @@ int SimplePipelineHandler::exportFrameBuffers(Camera *camera, Stream *stream,\n>> */\n>> if (data->useConversion_)\n>> return data->converter_\n>> - ? data->converter_->exportBuffers(data->streamIndex(stream),\n>> - count, buffers)\n>> - : data->swIsp_->exportBuffers(data->streamIndex(stream),\n>> - count, buffers);\n>> + ? data->converter_->exportBuffers(stream, count, buffers)\n>> + : data->swIsp_->exportBuffers(stream, count, buffers);\n>> else\n>> return data->video_->exportBuffers(count, buffers);\n>> }\n>> @@ -1399,7 +1397,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 +1406,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 ac10d82d..fe0946d7 100644\n>> --- a/src/libcamera/software_isp/software_isp.cpp\n>> +++ b/src/libcamera/software_isp/software_isp.cpp\n>> @@ -221,19 +221,19 @@ int SoftwareIsp::configure(const StreamConfiguration &inputCfg,\n>> \n>> /**\n>> * \\brief Export the buffers from the Software ISP\n>> - * \\param[in] output Output stream index exporting the buffers\n>> + * \\param[in] stream Output stream exporting the buffers\n>> * \\param[in] count Number of buffers to allocate\n>> * \\param[out] buffers Vector to store the allocated buffers\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>> \n>> for (unsigned int i = 0; i < count; i++) {\n>> @@ -260,12 +260,12 @@ int SoftwareIsp::exportBuffers(unsigned int output, unsigned int count,\n>> /**\n>> * \\brief Queue buffers to Software ISP\n>> * \\param[in] input The input framebuffer\n>> - * \\param[in] outputs The container holding the output stream indexes and\n>> + * \\param[in] outputs The container holding the output stream pointer and\n>> * their respective frame buffer outputs\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>> /*\n>> * Validate the outputs as a sanity check: at least one output is\n>> @@ -274,14 +274,15 @@ int SoftwareIsp::queueBuffers(FrameBuffer *input,\n>> if (outputs.empty())\n>> return -EINVAL;\n>> \n>> - for (auto [index, buffer] : outputs) {\n>> + for (auto [stream, buffer] : outputs) {\n>> if (!buffer)\n>> return -EINVAL;\n>> - if (index >= 1) /* only single stream atm */\n>> + if (outputs.size() != 1) /* only single stream atm */\n> I still think this \"can't\" happen, or should have been prevented during\n> configure()/validate() but it's existing code so it's fine to just\n> update - but I think this error check could be dropped sometime.\n\nThis pre-existing code in soft ISP was copy/pasted and adjusted \naccordingly to fit the needs. Proper validation should be done as you \nsuggested (probably orthogonal to this series)\n>\n>\n> I think the comments above are relatively minor (the dropped reserve\n> seems like something that could be kept? But it's not so harmful if\n> there's a legitimate reason to drop it...)\n\ncommented above.\n>\n> So\n>\n> Reviewed-by: Kieran Bingham \n>\n> and lets see if anyone else has anything more to add.\n\nI'll wait for more feedback and reviews, as currently I don't see a need \nfor v4 ?\n\n>\n> --\n> Kieran\n>\n>> return -EINVAL;\n>> }\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 2BF46BD87C\n\tfor ;\n\tWed, 12 Jun 2024 05:35:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DC8666545A;\n\tWed, 12 Jun 2024 07:35:07 +0200 (CEST)","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 DED4F61A1F\n\tfor ;\n\tWed, 12 Jun 2024 07:35:05 +0200 (CEST)","from [IPV6:2409:4056:183:2b0:212:ef68:4946:57cf] (unknown\n\t[IPv6:2409:4056:183:2b0:212:ef68:4946:57cf])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 74F0329A;\n\tWed, 12 Jun 2024 07:34: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=\"vXD8mY4Q\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1718170492;\n\tbh=+HYPIX4JRk6kHpac58yyhAJGDOG6fpZE2rzBw43LpWg=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=vXD8mY4Qt25F6UXGnHU/9OeENuzgXU2UKEVzgLVDvGqnNlLFrYGWAdKmC+D3IkuMb\n\tYtQuvtemGotP0D5INurvsbYu8H9LZnf7A+rxC/uRf+r6rT0nA+gNf9q9VOeo89dMdS\n\tGPnZVeSFJKP1IykW291q1o5oggZRBSLIl6/aUAno=","Message-ID":"<0706ba0d-d568-4619-9cea-5bff23fa5b7a@ideasonboard.com>","Date":"Wed, 12 Jun 2024 11:04:56 +0530","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v3 4/4] libcamera: converter: Replace usage of stream\n\tindex by Stream pointer","To":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org","Cc":"Milan Zamazal ,\n\tAndrey Konovalov ","References":"<20240531052505.150921-1-umang.jain@ideasonboard.com>\n\t<20240531052505.150921-5-umang.jain@ideasonboard.com>\n\t<171715521072.1000377.5290665602081381455@ping.linuxembedded.co.uk>","Content-Language":"en-US","From":"Umang Jain ","In-Reply-To":"<171715521072.1000377.5290665602081381455@ping.linuxembedded.co.uk>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","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":29875,"web_url":"https://patchwork.libcamera.org/comment/29875/","msgid":"<171819603389.2248009.16813117071106851040@ping.linuxembedded.co.uk>","date":"2024-06-12T12:40:33","subject":"Re: [PATCH v3 4/4] libcamera: converter: Replace usage of stream\n\tindex by 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-06-12 06:34:56)\n> Hi Kieran\n> \n> On 31/05/24 5:03 pm, Kieran Bingham wrote:\n> > Quoting Umang Jain (2024-05-31 06:25:05)\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> >> The v4l2_converter_m2m and simple pipeline handler are adapted 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> >> include/libcamera/internal/converter.h | 5 ++-\n> >> .../internal/converter/converter_v4l2_m2m.h | 11 ++---\n> >> .../internal/software_isp/software_isp.h | 5 ++-\n> >> src/libcamera/converter.cpp | 6 +--\n> >> .../converter/converter_v4l2_m2m.cpp | 40 ++++++++++---------\n> >> src/libcamera/pipeline/simple/simple.cpp | 14 +++----\n> >> src/libcamera/software_isp/software_isp.cpp | 17 ++++----\n> >> 7 files changed, 51 insertions(+), 47 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 0da62290..58fd19db 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 V4L2M2MStream : protected Loggable\n> >> {\n> >> public:\n> >> - V4L2M2MStream(V4L2M2MConverter *converter, unsigned int index);\n> >> + V4L2M2MStream(V4L2M2MConverter *converter, const 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 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> >> 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..09cc0f6f 100644\n> >> --- a/include/libcamera/internal/software_isp/software_isp.h\n> >> +++ b/include/libcamera/internal/software_isp/software_isp.h\n> >> @@ -37,6 +37,7 @@ namespace libcamera {\n> >> class DebayerCpu;\n> >> class FrameBuffer;\n> >> class PixelFormat;\n> >> +class Stream;\n> >> struct StreamConfiguration;\n> >> \n> >> LOG_DECLARE_CATEGORY(SoftwareIsp)\n> >> @@ -62,7 +63,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 +72,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.cpp b/src/libcamera/converter.cpp\n> >> index d3d38c1b..c5e38937 100644\n> >> --- a/src/libcamera/converter.cpp\n> >> +++ b/src/libcamera/converter.cpp\n> >> @@ -111,12 +111,12 @@ Converter::~Converter()\n> >> /**\n> >> * \\fn Converter::exportBuffers()\n> >> * \\brief Export buffers from the converter device\n> >> - * \\param[in] output Output stream index exporting the buffers\n> >> + * \\param[in] stream Output stream pointer exporting the buffers\n> >> * \\param[in] count Number of buffers to allocate\n> >> * \\param[out] buffers Vector to store allocated buffers\n> >> *\n> >> * This function operates similarly to V4L2VideoDevice::exportBuffers() on the\n> >> - * output stream indicated by the \\a output index.\n> >> + * output stream indicated by the \\a output.\n> >> *\n> >> * \\return The number of allocated buffers on success or a negative error code\n> >> * otherwise\n> >> @@ -137,7 +137,7 @@ Converter::~Converter()\n> >> * \\fn Converter::queueBuffers()\n> >> * \\brief Queue buffers to converter device\n> >> * \\param[in] input The frame buffer to apply the conversion\n> >> - * \\param[out] outputs The container holding the output stream indexes and\n> >> + * \\param[out] outputs The container holding the output stream pointer and\n> >> * their respective frame buffer outputs.\n> >> *\n> >> * This function queues the \\a input frame buffer on the output streams of the\n> >> diff --git a/src/libcamera/converter/converter_v4l2_m2m.cpp b/src/libcamera/converter/converter_v4l2_m2m.cpp\n> >> index 6309a0c0..a48b2a87 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::V4L2M2MStream\n> >> */\n> >> \n> >> -V4L2M2MConverter::V4L2M2MStream::V4L2M2MStream(V4L2M2MConverter *converter, unsigned int index)\n> >> - : converter_(converter), index_(index)\n> >> +V4L2M2MConverter::V4L2M2MStream::V4L2M2MStream(V4L2M2MConverter *converter, const Stream *stream)\n> >> + : converter_(converter), stream_(stream)\n> >> {\n> >> m2m_ = std::make_unique(converter->deviceNode());\n> >> \n> >> @@ -157,7 +157,7 @@ int V4L2M2MConverter::V4L2M2MStream::queueBuffers(FrameBuffer *input, FrameBuffe\n> >> \n> >> std::string V4L2M2MConverter::V4L2M2MStream::logPrefix() const\n> >> {\n> >> - return \"stream\" + std::to_string(index_);\n> >> + return stream_->configuration().toString();\n> > I'm still weary about this. What does it print ?\n> >\n> > Though the stream config might be useful to convey which convertor is\n> > being used if there are multiple streams - but I think it should really\n> > be more tied to what the /convertor/ is rather than the stream. Maybe we\n> > should drop the logPrefix or maybe do something better here.\n> \n> This should print the entire stream configuration of the Converter logs, \n> no ?  Earlier it would print \"stream0\" or \"stream1\" which is less \n> expressive IMO\n\nThat's actually what I'm worried about - the entire stream configuration\nsounds like too much, but without a demonstration/printout - I don't know.\n\nThe log-prefix is the part of a log message that indicates what the log\nmessage is from. It's supposed to be 'short'.\n\nBut if 'it's just \"1920x1080-MJPEG\" or such - then maybe it's fine.\nThat's why I asked \"What does it print?\"\n\nCould you capture a before and after of the logs here and compare them\nplease? (Ideally pasting a print of one line before and after here in\nthis thread). Highlighting what the change does to the logprefix would\nbe something I'd include in the commit message.\n\n\"\"\"\nThe logPrefix is no longer able to generate an index from a stream, and\nis updated to be more expressive by reporting the stream configuration\ninstead, reporting \"1920x1080-MJPEG\" in place of \"stream0\".\n\"\"\"\n\n(Of course I made up the example, so please ... update with what it\nactually produces!)\n\n\n\n\n> \n> I don't have a opinion on this right now, but there might be a better \n> option.\n> >\n> > But I don't mind if this is done on top or later though.\n> \n> Seconded. If all agrees...\n> >\n> >\n> >> }\n> >> \n> >> void V4L2M2MConverter::V4L2M2MStream::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> > Why is this line removed?\n> \n> This is removed because streams_ is not a std::vector anymore (see the \n> change above in the patch). streams_ is now a std::map, for which \n> reserve() is non-existent (rather not makes sense).\n\nAha - ok that's fine then.\n\n> >\n> >\n> >> for (unsigned int i = 0; i < outputCfgs.size(); ++i) {\n> >> - V4L2M2MStream &stream = streams_.emplace_back(this, i);\n> >> + const StreamConfiguration &cfg = outputCfgs[i];\n> >> + streams_.emplace(cfg.stream(),\n> >> + V4L2M2MStream(this, cfg.stream()));\n> >> +\n> >> + V4L2M2MStream &stream = streams_.at(cfg.stream());\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 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 (V4L2M2MStream &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,15 +397,15 @@ int V4L2M2MConverter::start()\n> >> */\n> >> void V4L2M2MConverter::stop()\n> >> {\n> >> - for (V4L2M2MStream &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> >> std::set outputBufs;\n> >> int ret;\n> >> @@ -414,11 +418,9 @@ int V4L2M2MConverter::queueBuffers(FrameBuffer *input,\n> >> if (outputs.empty())\n> >> return -EINVAL;\n> >> \n> >> - for (auto [index, buffer] : outputs) {\n> >> + for (auto [stream, buffer] : outputs) {\n> >> if (!buffer)\n> >> return -EINVAL;\n> >> - if (index >= streams_.size())\n> >> - return -EINVAL;\n> >> \n> >> outputBufs.insert(buffer);\n> >> }\n> >> @@ -427,8 +429,8 @@ int V4L2M2MConverter::queueBuffers(FrameBuffer *input,\n> >> return -EINVAL;\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..01ad91a7 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,10 +1304,8 @@ int SimplePipelineHandler::exportFrameBuffers(Camera *camera, Stream *stream,\n> >> */\n> >> if (data->useConversion_)\n> >> return data->converter_\n> >> - ? data->converter_->exportBuffers(data->streamIndex(stream),\n> >> - count, buffers)\n> >> - : data->swIsp_->exportBuffers(data->streamIndex(stream),\n> >> - count, buffers);\n> >> + ? data->converter_->exportBuffers(stream, count, buffers)\n> >> + : data->swIsp_->exportBuffers(stream, count, buffers);\n> >> else\n> >> return data->video_->exportBuffers(count, buffers);\n> >> }\n> >> @@ -1399,7 +1397,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 +1406,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 ac10d82d..fe0946d7 100644\n> >> --- a/src/libcamera/software_isp/software_isp.cpp\n> >> +++ b/src/libcamera/software_isp/software_isp.cpp\n> >> @@ -221,19 +221,19 @@ int SoftwareIsp::configure(const StreamConfiguration &inputCfg,\n> >> \n> >> /**\n> >> * \\brief Export the buffers from the Software ISP\n> >> - * \\param[in] output Output stream index exporting the buffers\n> >> + * \\param[in] stream Output stream exporting the buffers\n> >> * \\param[in] count Number of buffers to allocate\n> >> * \\param[out] buffers Vector to store the allocated buffers\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> >> \n> >> for (unsigned int i = 0; i < count; i++) {\n> >> @@ -260,12 +260,12 @@ int SoftwareIsp::exportBuffers(unsigned int output, unsigned int count,\n> >> /**\n> >> * \\brief Queue buffers to Software ISP\n> >> * \\param[in] input The input framebuffer\n> >> - * \\param[in] outputs The container holding the output stream indexes and\n> >> + * \\param[in] outputs The container holding the output stream pointer and\n> >> * their respective frame buffer outputs\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> >> /*\n> >> * Validate the outputs as a sanity check: at least one output is\n> >> @@ -274,14 +274,15 @@ int SoftwareIsp::queueBuffers(FrameBuffer *input,\n> >> if (outputs.empty())\n> >> return -EINVAL;\n> >> \n> >> - for (auto [index, buffer] : outputs) {\n> >> + for (auto [stream, buffer] : outputs) {\n> >> if (!buffer)\n> >> return -EINVAL;\n> >> - if (index >= 1) /* only single stream atm */\n> >> + if (outputs.size() != 1) /* only single stream atm */\n> > I still think this \"can't\" happen, or should have been prevented during\n> > configure()/validate() but it's existing code so it's fine to just\n> > update - but I think this error check could be dropped sometime.\n> \n> This pre-existing code in soft ISP was copy/pasted and adjusted \n> accordingly to fit the needs. Proper validation should be done as you \n> suggested (probably orthogonal to this series)\n> >\n> >\n> > I think the comments above are relatively minor (the dropped reserve\n> > seems like something that could be kept? But it's not so harmful if\n> > there's a legitimate reason to drop it...)\n> \n> commented above.\n> >\n> > So\n> >\n> > Reviewed-by: Kieran Bingham \n> >\n> > and lets see if anyone else has anything more to add.\n> \n> I'll wait for more feedback and reviews, as currently I don't see a need \n> for v4 ?\n> \n\nYup - this is otherwise fine with me - worst case would be updating the\ncommit message while applying perhaps.\n\n--\nKieran\n\n\n> >\n> > --\n> > Kieran\n> >\n> >> return -EINVAL;\n> >> }\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> >>\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 2C2B7C3237\n\tfor ;\n\tWed, 12 Jun 2024 12:40:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 612BD6545D;\n\tWed, 12 Jun 2024 14:40:38 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2228B6545A\n\tfor ;\n\tWed, 12 Jun 2024 14:40:37 +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 863B14D0;\n\tWed, 12 Jun 2024 14:40:23 +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=\"F2DzRLwC\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1718196023;\n\tbh=hvzrUTsazvpKKxb2jdk4IabYZBsN+n8ZJMpPCuUh8KA=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=F2DzRLwCCixbE6LbrtyMXR+GeW4pcWY32oGnbyph4Eaxo21Jtw8AuwpVNZi+ZeoLN\n\td32l5VoVBWZQjnxP9lfcYDSS4VPDJLJWwUel/XaktZQhMQTNiZf9zR1sjrBixvUURp\n\tn1AMO0i95Il9FeNIES0VUFKBhUriRMOEB7Q3+Gc4=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<0706ba0d-d568-4619-9cea-5bff23fa5b7a@ideasonboard.com>","References":"<20240531052505.150921-1-umang.jain@ideasonboard.com>\n\t<20240531052505.150921-5-umang.jain@ideasonboard.com>\n\t<171715521072.1000377.5290665602081381455@ping.linuxembedded.co.uk>\n\t<0706ba0d-d568-4619-9cea-5bff23fa5b7a@ideasonboard.com>","Subject":"Re: [PATCH v3 4/4] libcamera: converter: Replace usage of stream\n\tindex by Stream pointer","From":"Kieran Bingham ","Cc":"Milan Zamazal ,\n\tAndrey Konovalov ","To":"Umang Jain ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Wed, 12 Jun 2024 13:40:33 +0100","Message-ID":"<171819603389.2248009.16813117071106851040@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":30043,"web_url":"https://patchwork.libcamera.org/comment/30043/","msgid":"","date":"2024-06-24T07:13:06","subject":"Re: [PATCH v3 4/4] libcamera: converter: Replace usage of stream\n\tindex by 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 12/06/24 6:10 pm, Kieran Bingham wrote:\n> Quoting Umang Jain (2024-06-12 06:34:56)\n>> Hi Kieran\n>>\n>> On 31/05/24 5:03 pm, Kieran Bingham wrote:\n>>> Quoting Umang Jain (2024-05-31 06:25:05)\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>>>> The v4l2_converter_m2m and simple pipeline handler are adapted 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>>>> include/libcamera/internal/converter.h | 5 ++-\n>>>> .../internal/converter/converter_v4l2_m2m.h | 11 ++---\n>>>> .../internal/software_isp/software_isp.h | 5 ++-\n>>>> src/libcamera/converter.cpp | 6 +--\n>>>> .../converter/converter_v4l2_m2m.cpp | 40 ++++++++++---------\n>>>> src/libcamera/pipeline/simple/simple.cpp | 14 +++----\n>>>> src/libcamera/software_isp/software_isp.cpp | 17 ++++----\n>>>> 7 files changed, 51 insertions(+), 47 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 0da62290..58fd19db 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 V4L2M2MStream : protected Loggable\n>>>> {\n>>>> public:\n>>>> - V4L2M2MStream(V4L2M2MConverter *converter, unsigned int index);\n>>>> + V4L2M2MStream(V4L2M2MConverter *converter, const 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 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>>>> 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..09cc0f6f 100644\n>>>> --- a/include/libcamera/internal/software_isp/software_isp.h\n>>>> +++ b/include/libcamera/internal/software_isp/software_isp.h\n>>>> @@ -37,6 +37,7 @@ namespace libcamera {\n>>>> class DebayerCpu;\n>>>> class FrameBuffer;\n>>>> class PixelFormat;\n>>>> +class Stream;\n>>>> struct StreamConfiguration;\n>>>> \n>>>> LOG_DECLARE_CATEGORY(SoftwareIsp)\n>>>> @@ -62,7 +63,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 +72,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.cpp b/src/libcamera/converter.cpp\n>>>> index d3d38c1b..c5e38937 100644\n>>>> --- a/src/libcamera/converter.cpp\n>>>> +++ b/src/libcamera/converter.cpp\n>>>> @@ -111,12 +111,12 @@ Converter::~Converter()\n>>>> /**\n>>>> * \\fn Converter::exportBuffers()\n>>>> * \\brief Export buffers from the converter device\n>>>> - * \\param[in] output Output stream index exporting the buffers\n>>>> + * \\param[in] stream Output stream pointer exporting the buffers\n>>>> * \\param[in] count Number of buffers to allocate\n>>>> * \\param[out] buffers Vector to store allocated buffers\n>>>> *\n>>>> * This function operates similarly to V4L2VideoDevice::exportBuffers() on the\n>>>> - * output stream indicated by the \\a output index.\n>>>> + * output stream indicated by the \\a output.\n>>>> *\n>>>> * \\return The number of allocated buffers on success or a negative error code\n>>>> * otherwise\n>>>> @@ -137,7 +137,7 @@ Converter::~Converter()\n>>>> * \\fn Converter::queueBuffers()\n>>>> * \\brief Queue buffers to converter device\n>>>> * \\param[in] input The frame buffer to apply the conversion\n>>>> - * \\param[out] outputs The container holding the output stream indexes and\n>>>> + * \\param[out] outputs The container holding the output stream pointer and\n>>>> * their respective frame buffer outputs.\n>>>> *\n>>>> * This function queues the \\a input frame buffer on the output streams of the\n>>>> diff --git a/src/libcamera/converter/converter_v4l2_m2m.cpp b/src/libcamera/converter/converter_v4l2_m2m.cpp\n>>>> index 6309a0c0..a48b2a87 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::V4L2M2MStream\n>>>> */\n>>>> \n>>>> -V4L2M2MConverter::V4L2M2MStream::V4L2M2MStream(V4L2M2MConverter *converter, unsigned int index)\n>>>> - : converter_(converter), index_(index)\n>>>> +V4L2M2MConverter::V4L2M2MStream::V4L2M2MStream(V4L2M2MConverter *converter, const Stream *stream)\n>>>> + : converter_(converter), stream_(stream)\n>>>> {\n>>>> m2m_ = std::make_unique(converter->deviceNode());\n>>>> \n>>>> @@ -157,7 +157,7 @@ int V4L2M2MConverter::V4L2M2MStream::queueBuffers(FrameBuffer *input, FrameBuffe\n>>>> \n>>>> std::string V4L2M2MConverter::V4L2M2MStream::logPrefix() const\n>>>> {\n>>>> - return \"stream\" + std::to_string(index_);\n>>>> + return stream_->configuration().toString();\n>>> I'm still weary about this. What does it print ?\n>>>\n>>> Though the stream config might be useful to convey which convertor is\n>>> being used if there are multiple streams - but I think it should really\n>>> be more tied to what the /convertor/ is rather than the stream. Maybe we\n>>> should drop the logPrefix or maybe do something better here.\n>> This should print the entire stream configuration of the Converter logs,\n>> no ?  Earlier it would print \"stream0\" or \"stream1\" which is less\n>> expressive IMO\n> That's actually what I'm worried about - the entire stream configuration\n> sounds like too much, but without a demonstration/printout - I don't know.\n>\n> The log-prefix is the part of a log message that indicates what the log\n> message is from. It's supposed to be 'short'.\n>\n> But if 'it's just \"1920x1080-MJPEG\" or such - then maybe it's fine.\n> That's why I asked \"What does it print?\"\n\nThis is what it exactly prints:\n\n[20:34:50.175903750] [1267]  INFO Converter converter_v4l2_m2m.cpp:155 \n2736x1824-NV12: output()->queueBuffer 0xffff84023c50\n\nIs one of the example. This looks fine to me.\n\nHowever, the validility of the  call to stream_->configuration() is \nactually depending on the configure() phase. It is documentation as :\n\n/**\n  * \\var Stream::configuration_\n  * \\brief The stream configuration\n  *\n  * The configuration for the stream is set by any successful call to\n  * Camera::configure() that includes the stream, and remains valid \nuntil the\n  * next call to Camera::configure() regardless of if it includes the \nstream.\n  */\n\nSo if you try to put a LOG(Converter,...) before the configure(), the \nprint log prefix comes out to be: \"0x0-\"\n\nFor example:\n[20:25:02.595393125] [1251] ERROR Converter converter_v4l2_m2m.cpp:49 \n0x0-:  m2m open return : 0\n\nThis is not ideal. Maybe we need to\n\n\n  std::string V4L2M2MConverter::V4L2M2MStream::logPrefix() const\n  {\n-       return stream_->configuration().toString();\n+       if (stream_->configuration().size <= Size(0, 0))\n+               return \"\";\n+       else\n+               return stream_->configuration().toString();\n  }\n\n\nWhat are your thoughts ?\n\n>\n> Could you capture a before and after of the logs here and compare them\n> please? (Ideally pasting a print of one line before and after here in\n> this thread). Highlighting what the change does to the logprefix would\n> be something I'd include in the commit message.\n>\n> \"\"\"\n> The logPrefix is no longer able to generate an index from a stream, and\n> is updated to be more expressive by reporting the stream configuration\n> instead, reporting \"1920x1080-MJPEG\" in place of \"stream0\".\n> \"\"\"\n>\n> (Of course I made up the example, so please ... update with what it\n> actually produces!)\n\nYes, this seems correct.\n>\n>\n>\n>\n>> I don't have a opinion on this right now, but there might be a better\n>> option.\n>>> But I don't mind if this is done on top or later though.\n>> Seconded. If all agrees...\n>>>\n>>>> }\n>>>> \n>>>> void V4L2M2MConverter::V4L2M2MStream::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>>> Why is this line removed?\n>> This is removed because streams_ is not a std::vector anymore (see the\n>> change above in the patch). streams_ is now a std::map, for which\n>> reserve() is non-existent (rather not makes sense).\n> Aha - ok that's fine then.\n>\n>>>\n>>>> for (unsigned int i = 0; i < outputCfgs.size(); ++i) {\n>>>> - V4L2M2MStream &stream = streams_.emplace_back(this, i);\n>>>> + const StreamConfiguration &cfg = outputCfgs[i];\n>>>> + streams_.emplace(cfg.stream(),\n>>>> + V4L2M2MStream(this, cfg.stream()));\n>>>> +\n>>>> + V4L2M2MStream &stream = streams_.at(cfg.stream());\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 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 (V4L2M2MStream &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,15 +397,15 @@ int V4L2M2MConverter::start()\n>>>> */\n>>>> void V4L2M2MConverter::stop()\n>>>> {\n>>>> - for (V4L2M2MStream &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>>>> std::set outputBufs;\n>>>> int ret;\n>>>> @@ -414,11 +418,9 @@ int V4L2M2MConverter::queueBuffers(FrameBuffer *input,\n>>>> if (outputs.empty())\n>>>> return -EINVAL;\n>>>> \n>>>> - for (auto [index, buffer] : outputs) {\n>>>> + for (auto [stream, buffer] : outputs) {\n>>>> if (!buffer)\n>>>> return -EINVAL;\n>>>> - if (index >= streams_.size())\n>>>> - return -EINVAL;\n>>>> \n>>>> outputBufs.insert(buffer);\n>>>> }\n>>>> @@ -427,8 +429,8 @@ int V4L2M2MConverter::queueBuffers(FrameBuffer *input,\n>>>> return -EINVAL;\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..01ad91a7 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,10 +1304,8 @@ int SimplePipelineHandler::exportFrameBuffers(Camera *camera, Stream *stream,\n>>>> */\n>>>> if (data->useConversion_)\n>>>> return data->converter_\n>>>> - ? data->converter_->exportBuffers(data->streamIndex(stream),\n>>>> - count, buffers)\n>>>> - : data->swIsp_->exportBuffers(data->streamIndex(stream),\n>>>> - count, buffers);\n>>>> + ? data->converter_->exportBuffers(stream, count, buffers)\n>>>> + : data->swIsp_->exportBuffers(stream, count, buffers);\n>>>> else\n>>>> return data->video_->exportBuffers(count, buffers);\n>>>> }\n>>>> @@ -1399,7 +1397,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 +1406,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 ac10d82d..fe0946d7 100644\n>>>> --- a/src/libcamera/software_isp/software_isp.cpp\n>>>> +++ b/src/libcamera/software_isp/software_isp.cpp\n>>>> @@ -221,19 +221,19 @@ int SoftwareIsp::configure(const StreamConfiguration &inputCfg,\n>>>> \n>>>> /**\n>>>> * \\brief Export the buffers from the Software ISP\n>>>> - * \\param[in] output Output stream index exporting the buffers\n>>>> + * \\param[in] stream Output stream exporting the buffers\n>>>> * \\param[in] count Number of buffers to allocate\n>>>> * \\param[out] buffers Vector to store the allocated buffers\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>>>> \n>>>> for (unsigned int i = 0; i < count; i++) {\n>>>> @@ -260,12 +260,12 @@ int SoftwareIsp::exportBuffers(unsigned int output, unsigned int count,\n>>>> /**\n>>>> * \\brief Queue buffers to Software ISP\n>>>> * \\param[in] input The input framebuffer\n>>>> - * \\param[in] outputs The container holding the output stream indexes and\n>>>> + * \\param[in] outputs The container holding the output stream pointer and\n>>>> * their respective frame buffer outputs\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>>>> /*\n>>>> * Validate the outputs as a sanity check: at least one output is\n>>>> @@ -274,14 +274,15 @@ int SoftwareIsp::queueBuffers(FrameBuffer *input,\n>>>> if (outputs.empty())\n>>>> return -EINVAL;\n>>>> \n>>>> - for (auto [index, buffer] : outputs) {\n>>>> + for (auto [stream, buffer] : outputs) {\n>>>> if (!buffer)\n>>>> return -EINVAL;\n>>>> - if (index >= 1) /* only single stream atm */\n>>>> + if (outputs.size() != 1) /* only single stream atm */\n>>> I still think this \"can't\" happen, or should have been prevented during\n>>> configure()/validate() but it's existing code so it's fine to just\n>>> update - but I think this error check could be dropped sometime.\n>> This pre-existing code in soft ISP was copy/pasted and adjusted\n>> accordingly to fit the needs. Proper validation should be done as you\n>> suggested (probably orthogonal to this series)\n>>>\n>>> I think the comments above are relatively minor (the dropped reserve\n>>> seems like something that could be kept? But it's not so harmful if\n>>> there's a legitimate reason to drop it...)\n>> commented above.\n>>> So\n>>>\n>>> Reviewed-by: Kieran Bingham \n>>>\n>>> and lets see if anyone else has anything more to add.\n>> I'll wait for more feedback and reviews, as currently I don't see a need\n>> for v4 ?\n>>\n> Yup - this is otherwise fine with me - worst case would be updating the\n> commit message while applying perhaps.\n>\n> --\n> Kieran\n>\n>\n>>> --\n>>> Kieran\n>>>\n>>>> return -EINVAL;\n>>>> }\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 D83C7BD87C\n\tfor ;\n\tMon, 24 Jun 2024 07:13:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A2E8C654A9;\n\tMon, 24 Jun 2024 09:13:14 +0200 (CEST)","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 7B3E5619EA\n\tfor ;\n\tMon, 24 Jun 2024 09:13:12 +0200 (CEST)","from [IPV6:2405:201:2015:f873:9278:2c85:fd02:c5f5] (unknown\n\t[IPv6:2405:201:2015:f873:9278:2c85:fd02:c5f5])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 41FD6674;\n\tMon, 24 Jun 2024 09:12:48 +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=\"iGUb7NIC\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1719213170;\n\tbh=sXBEuVwXcp/jsIiR82Phz36cpHVNj9Ybr7J1RTf5ep8=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=iGUb7NICUeBhNiLZClwXv5MOBGcctH4aDkrqp9JXmU7S43HN7Z4J2SIagVWmgaDmh\n\tJUvLx8kQ+iSt8MnDpR4XY+MfBsS9d1xRp7+pBIiPMgH4ORsq3Coj/3fGwewdtH6lJH\n\tC4P/UL9TlqVqisx8KSGY4eoYNsIfpkKUdVH87RQo=","Message-ID":"","Date":"Mon, 24 Jun 2024 12:43:06 +0530","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v3 4/4] libcamera: converter: Replace usage of stream\n\tindex by Stream pointer","To":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org","Cc":"Milan Zamazal ,\n\tAndrey Konovalov ","References":"<20240531052505.150921-1-umang.jain@ideasonboard.com>\n\t<20240531052505.150921-5-umang.jain@ideasonboard.com>\n\t<171715521072.1000377.5290665602081381455@ping.linuxembedded.co.uk>\n\t<0706ba0d-d568-4619-9cea-5bff23fa5b7a@ideasonboard.com>\n\t<171819603389.2248009.16813117071106851040@ping.linuxembedded.co.uk>","Content-Language":"en-US","From":"Umang Jain ","In-Reply-To":"<171819603389.2248009.16813117071106851040@ping.linuxembedded.co.uk>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","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":30047,"web_url":"https://patchwork.libcamera.org/comment/30047/","msgid":"<171922800735.341316.15004106603366992057@ping.linuxembedded.co.uk>","date":"2024-06-24T11:20:07","subject":"Re: [PATCH v3 4/4] libcamera: converter: Replace usage of stream\n\tindex by 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-06-24 08:13:06)\n> Hi Kieran\n> \n> On 12/06/24 6:10 pm, Kieran Bingham wrote:\n> > Quoting Umang Jain (2024-06-12 06:34:56)\n> >> Hi Kieran\n> >>\n> >> On 31/05/24 5:03 pm, Kieran Bingham wrote:\n> >>> Quoting Umang Jain (2024-05-31 06:25:05)\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> >>>> The v4l2_converter_m2m and simple pipeline handler are adapted 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> >>>> include/libcamera/internal/converter.h | 5 ++-\n> >>>> .../internal/converter/converter_v4l2_m2m.h | 11 ++---\n> >>>> .../internal/software_isp/software_isp.h | 5 ++-\n> >>>> src/libcamera/converter.cpp | 6 +--\n> >>>> .../converter/converter_v4l2_m2m.cpp | 40 ++++++++++---------\n> >>>> src/libcamera/pipeline/simple/simple.cpp | 14 +++----\n> >>>> src/libcamera/software_isp/software_isp.cpp | 17 ++++----\n> >>>> 7 files changed, 51 insertions(+), 47 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 0da62290..58fd19db 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 V4L2M2MStream : protected Loggable\n> >>>> {\n> >>>> public:\n> >>>> - V4L2M2MStream(V4L2M2MConverter *converter, unsigned int index);\n> >>>> + V4L2M2MStream(V4L2M2MConverter *converter, const 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 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> >>>> 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..09cc0f6f 100644\n> >>>> --- a/include/libcamera/internal/software_isp/software_isp.h\n> >>>> +++ b/include/libcamera/internal/software_isp/software_isp.h\n> >>>> @@ -37,6 +37,7 @@ namespace libcamera {\n> >>>> class DebayerCpu;\n> >>>> class FrameBuffer;\n> >>>> class PixelFormat;\n> >>>> +class Stream;\n> >>>> struct StreamConfiguration;\n> >>>> \n> >>>> LOG_DECLARE_CATEGORY(SoftwareIsp)\n> >>>> @@ -62,7 +63,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 +72,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.cpp b/src/libcamera/converter.cpp\n> >>>> index d3d38c1b..c5e38937 100644\n> >>>> --- a/src/libcamera/converter.cpp\n> >>>> +++ b/src/libcamera/converter.cpp\n> >>>> @@ -111,12 +111,12 @@ Converter::~Converter()\n> >>>> /**\n> >>>> * \\fn Converter::exportBuffers()\n> >>>> * \\brief Export buffers from the converter device\n> >>>> - * \\param[in] output Output stream index exporting the buffers\n> >>>> + * \\param[in] stream Output stream pointer exporting the buffers\n> >>>> * \\param[in] count Number of buffers to allocate\n> >>>> * \\param[out] buffers Vector to store allocated buffers\n> >>>> *\n> >>>> * This function operates similarly to V4L2VideoDevice::exportBuffers() on the\n> >>>> - * output stream indicated by the \\a output index.\n> >>>> + * output stream indicated by the \\a output.\n> >>>> *\n> >>>> * \\return The number of allocated buffers on success or a negative error code\n> >>>> * otherwise\n> >>>> @@ -137,7 +137,7 @@ Converter::~Converter()\n> >>>> * \\fn Converter::queueBuffers()\n> >>>> * \\brief Queue buffers to converter device\n> >>>> * \\param[in] input The frame buffer to apply the conversion\n> >>>> - * \\param[out] outputs The container holding the output stream indexes and\n> >>>> + * \\param[out] outputs The container holding the output stream pointer and\n> >>>> * their respective frame buffer outputs.\n> >>>> *\n> >>>> * This function queues the \\a input frame buffer on the output streams of the\n> >>>> diff --git a/src/libcamera/converter/converter_v4l2_m2m.cpp b/src/libcamera/converter/converter_v4l2_m2m.cpp\n> >>>> index 6309a0c0..a48b2a87 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::V4L2M2MStream\n> >>>> */\n> >>>> \n> >>>> -V4L2M2MConverter::V4L2M2MStream::V4L2M2MStream(V4L2M2MConverter *converter, unsigned int index)\n> >>>> - : converter_(converter), index_(index)\n> >>>> +V4L2M2MConverter::V4L2M2MStream::V4L2M2MStream(V4L2M2MConverter *converter, const Stream *stream)\n> >>>> + : converter_(converter), stream_(stream)\n> >>>> {\n> >>>> m2m_ = std::make_unique(converter->deviceNode());\n> >>>> \n> >>>> @@ -157,7 +157,7 @@ int V4L2M2MConverter::V4L2M2MStream::queueBuffers(FrameBuffer *input, FrameBuffe\n> >>>> \n> >>>> std::string V4L2M2MConverter::V4L2M2MStream::logPrefix() const\n> >>>> {\n> >>>> - return \"stream\" + std::to_string(index_);\n> >>>> + return stream_->configuration().toString();\n> >>> I'm still weary about this. What does it print ?\n> >>>\n> >>> Though the stream config might be useful to convey which convertor is\n> >>> being used if there are multiple streams - but I think it should really\n> >>> be more tied to what the /convertor/ is rather than the stream. Maybe we\n> >>> should drop the logPrefix or maybe do something better here.\n> >> This should print the entire stream configuration of the Converter logs,\n> >> no ?  Earlier it would print \"stream0\" or \"stream1\" which is less\n> >> expressive IMO\n> > That's actually what I'm worried about - the entire stream configuration\n> > sounds like too much, but without a demonstration/printout - I don't know.\n> >\n> > The log-prefix is the part of a log message that indicates what the log\n> > message is from. It's supposed to be 'short'.\n> >\n> > But if 'it's just \"1920x1080-MJPEG\" or such - then maybe it's fine.\n> > That's why I asked \"What does it print?\"\n> \n> This is what it exactly prints:\n> \n> [20:34:50.175903750] [1267]  INFO Converter converter_v4l2_m2m.cpp:155 \n> 2736x1824-NV12: output()->queueBuffer 0xffff84023c50\n> \n> Is one of the example. This looks fine to me.\n> \n> However, the validility of the  call to stream_->configuration() is \n> actually depending on the configure() phase. It is documentation as :\n> \n> /**\n>  * \\var Stream::configuration_\n>  * \\brief The stream configuration\n>  *\n>  * The configuration for the stream is set by any successful call to\n>  * Camera::configure() that includes the stream, and remains valid \n> until the\n>  * next call to Camera::configure() regardless of if it includes the \n> stream.\n>  */\n> \n> So if you try to put a LOG(Converter,...) before the configure(), the \n> print log prefix comes out to be: \"0x0-\"\n> \n> For example:\n> [20:25:02.595393125] [1251] ERROR Converter converter_v4l2_m2m.cpp:49 \n> 0x0-:  m2m open return : 0\n> \n> This is not ideal. Maybe we need to\n> \n> \n>  std::string V4L2M2MConverter::V4L2M2MStream::logPrefix() const\n>  {\n> -       return stream_->configuration().toString();\n> +       if (stream_->configuration().size <= Size(0, 0))\n> +               return \"\";\n\nThat's possible indeed... Or just keep it as above.\n\nThat's sort of where my concern was about using the stream configuration\nas a way to convey which stream it is. But I don't see a better solution\nright now so I'm fine with this. It's a corner case that we'll get\nerrors here.\n\n\n> +       else\n> +               return stream_->configuration().toString();\n>  }\n> \n> \n> What are your thoughts ?\n> \n> >\n> > Could you capture a before and after of the logs here and compare them\n> > please? (Ideally pasting a print of one line before and after here in\n> > this thread). Highlighting what the change does to the logprefix would\n> > be something I'd include in the commit message.\n> >\n> > \"\"\"\n> > The logPrefix is no longer able to generate an index from a stream, and\n> > is updated to be more expressive by reporting the stream configuration\n> > instead, reporting \"1920x1080-MJPEG\" in place of \"stream0\".\n> > \"\"\"\n> >\n> > (Of course I made up the example, so please ... update with what it\n> > actually produces!)\n> \n> Yes, this seems correct.\n\nOk - well I'll leave it to you if you want to post a v4 - it's only the\ncommit message to update I think so may not be worth it.\n\n--\nKieran\n\n\n> >\n> >\n> >\n> >\n> >> I don't have a opinion on this right now, but there might be a better\n> >> option.\n> >>> But I don't mind if this is done on top or later though.\n> >> Seconded. If all agrees...\n> >>>\n> >>>> }\n> >>>> \n> >>>> void V4L2M2MConverter::V4L2M2MStream::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> >>> Why is this line removed?\n> >> This is removed because streams_ is not a std::vector anymore (see the\n> >> change above in the patch). streams_ is now a std::map, for which\n> >> reserve() is non-existent (rather not makes sense).\n> > Aha - ok that's fine then.\n> >\n> >>>\n> >>>> for (unsigned int i = 0; i < outputCfgs.size(); ++i) {\n> >>>> - V4L2M2MStream &stream = streams_.emplace_back(this, i);\n> >>>> + const StreamConfiguration &cfg = outputCfgs[i];\n> >>>> + streams_.emplace(cfg.stream(),\n> >>>> + V4L2M2MStream(this, cfg.stream()));\n> >>>> +\n> >>>> + V4L2M2MStream &stream = streams_.at(cfg.stream());\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 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 (V4L2M2MStream &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,15 +397,15 @@ int V4L2M2MConverter::start()\n> >>>> */\n> >>>> void V4L2M2MConverter::stop()\n> >>>> {\n> >>>> - for (V4L2M2MStream &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> >>>> std::set outputBufs;\n> >>>> int ret;\n> >>>> @@ -414,11 +418,9 @@ int V4L2M2MConverter::queueBuffers(FrameBuffer *input,\n> >>>> if (outputs.empty())\n> >>>> return -EINVAL;\n> >>>> \n> >>>> - for (auto [index, buffer] : outputs) {\n> >>>> + for (auto [stream, buffer] : outputs) {\n> >>>> if (!buffer)\n> >>>> return -EINVAL;\n> >>>> - if (index >= streams_.size())\n> >>>> - return -EINVAL;\n> >>>> \n> >>>> outputBufs.insert(buffer);\n> >>>> }\n> >>>> @@ -427,8 +429,8 @@ int V4L2M2MConverter::queueBuffers(FrameBuffer *input,\n> >>>> return -EINVAL;\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..01ad91a7 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,10 +1304,8 @@ int SimplePipelineHandler::exportFrameBuffers(Camera *camera, Stream *stream,\n> >>>> */\n> >>>> if (data->useConversion_)\n> >>>> return data->converter_\n> >>>> - ? data->converter_->exportBuffers(data->streamIndex(stream),\n> >>>> - count, buffers)\n> >>>> - : data->swIsp_->exportBuffers(data->streamIndex(stream),\n> >>>> - count, buffers);\n> >>>> + ? data->converter_->exportBuffers(stream, count, buffers)\n> >>>> + : data->swIsp_->exportBuffers(stream, count, buffers);\n> >>>> else\n> >>>> return data->video_->exportBuffers(count, buffers);\n> >>>> }\n> >>>> @@ -1399,7 +1397,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 +1406,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 ac10d82d..fe0946d7 100644\n> >>>> --- a/src/libcamera/software_isp/software_isp.cpp\n> >>>> +++ b/src/libcamera/software_isp/software_isp.cpp\n> >>>> @@ -221,19 +221,19 @@ int SoftwareIsp::configure(const StreamConfiguration &inputCfg,\n> >>>> \n> >>>> /**\n> >>>> * \\brief Export the buffers from the Software ISP\n> >>>> - * \\param[in] output Output stream index exporting the buffers\n> >>>> + * \\param[in] stream Output stream exporting the buffers\n> >>>> * \\param[in] count Number of buffers to allocate\n> >>>> * \\param[out] buffers Vector to store the allocated buffers\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> >>>> \n> >>>> for (unsigned int i = 0; i < count; i++) {\n> >>>> @@ -260,12 +260,12 @@ int SoftwareIsp::exportBuffers(unsigned int output, unsigned int count,\n> >>>> /**\n> >>>> * \\brief Queue buffers to Software ISP\n> >>>> * \\param[in] input The input framebuffer\n> >>>> - * \\param[in] outputs The container holding the output stream indexes and\n> >>>> + * \\param[in] outputs The container holding the output stream pointer and\n> >>>> * their respective frame buffer outputs\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> >>>> /*\n> >>>> * Validate the outputs as a sanity check: at least one output is\n> >>>> @@ -274,14 +274,15 @@ int SoftwareIsp::queueBuffers(FrameBuffer *input,\n> >>>> if (outputs.empty())\n> >>>> return -EINVAL;\n> >>>> \n> >>>> - for (auto [index, buffer] : outputs) {\n> >>>> + for (auto [stream, buffer] : outputs) {\n> >>>> if (!buffer)\n> >>>> return -EINVAL;\n> >>>> - if (index >= 1) /* only single stream atm */\n> >>>> + if (outputs.size() != 1) /* only single stream atm */\n> >>> I still think this \"can't\" happen, or should have been prevented during\n> >>> configure()/validate() but it's existing code so it's fine to just\n> >>> update - but I think this error check could be dropped sometime.\n> >> This pre-existing code in soft ISP was copy/pasted and adjusted\n> >> accordingly to fit the needs. Proper validation should be done as you\n> >> suggested (probably orthogonal to this series)\n> >>>\n> >>> I think the comments above are relatively minor (the dropped reserve\n> >>> seems like something that could be kept? But it's not so harmful if\n> >>> there's a legitimate reason to drop it...)\n> >> commented above.\n> >>> So\n> >>>\n> >>> Reviewed-by: Kieran Bingham \n> >>>\n> >>> and lets see if anyone else has anything more to add.\n> >> I'll wait for more feedback and reviews, as currently I don't see a need\n> >> for v4 ?\n> >>\n> > Yup - this is otherwise fine with me - worst case would be updating the\n> > commit message while applying perhaps.\n> >\n> > --\n> > Kieran\n> >\n> >\n> >>> --\n> >>> Kieran\n> >>>\n> >>>> return -EINVAL;\n> >>>> }\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> >>>>\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 AF298BD87C\n\tfor ;\n\tMon, 24 Jun 2024 11:20:12 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EE4A3654A9;\n\tMon, 24 Jun 2024 13:20:11 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4DBAE654A1\n\tfor ;\n\tMon, 24 Jun 2024 13:20:10 +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 23CEE7E0;\n\tMon, 24 Jun 2024 13:19:48 +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=\"YTdlB5Nh\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1719227988;\n\tbh=5nZD9QvV3f3b32KPwMszlepQYt8qBZWgluu6b5+u+xg=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=YTdlB5NhM9ZDFWCvuOEzokIWEJlQip8nfZbmet7+E+uHDYsqvsSD9aob/t3Q0rZLo\n\tn2Hh9aPUfnRWXwPBMmtxoTo78y1a6S9hfCrn5g51VJs9fXQiacWgW3CLi6TafUKKoW\n\tQwZbcIq9sqJKAWs5sPkreXdnKrBAmYYqNaFPI3zA=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20240531052505.150921-1-umang.jain@ideasonboard.com>\n\t<20240531052505.150921-5-umang.jain@ideasonboard.com>\n\t<171715521072.1000377.5290665602081381455@ping.linuxembedded.co.uk>\n\t<0706ba0d-d568-4619-9cea-5bff23fa5b7a@ideasonboard.com>\n\t<171819603389.2248009.16813117071106851040@ping.linuxembedded.co.uk>\n\t","Subject":"Re: [PATCH v3 4/4] libcamera: converter: Replace usage of stream\n\tindex by Stream pointer","From":"Kieran Bingham ","Cc":"Milan Zamazal ,\n\tAndrey Konovalov ","To":"Umang Jain ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Mon, 24 Jun 2024 12:20:07 +0100","Message-ID":"<171922800735.341316.15004106603366992057@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":30051,"web_url":"https://patchwork.libcamera.org/comment/30051/","msgid":"<97eb7fa9-bcf0-4057-b328-997f01092cf8@ideasonboard.com>","date":"2024-06-24T12:44:59","subject":"Re: [PATCH v3 4/4] libcamera: converter: Replace usage of stream\n\tindex by 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 24/06/24 4:50 pm, Kieran Bingham wrote:\n> Quoting Umang Jain (2024-06-24 08:13:06)\n>> Hi Kieran\n>>\n>> On 12/06/24 6:10 pm, Kieran Bingham wrote:\n>>> Quoting Umang Jain (2024-06-12 06:34:56)\n>>>> Hi Kieran\n>>>>\n>>>> On 31/05/24 5:03 pm, Kieran Bingham wrote:\n>>>>> Quoting Umang Jain (2024-05-31 06:25:05)\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>>>>>> The v4l2_converter_m2m and simple pipeline handler are adapted 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>>>>>> include/libcamera/internal/converter.h | 5 ++-\n>>>>>> .../internal/converter/converter_v4l2_m2m.h | 11 ++---\n>>>>>> .../internal/software_isp/software_isp.h | 5 ++-\n>>>>>> src/libcamera/converter.cpp | 6 +--\n>>>>>> .../converter/converter_v4l2_m2m.cpp | 40 ++++++++++---------\n>>>>>> src/libcamera/pipeline/simple/simple.cpp | 14 +++----\n>>>>>> src/libcamera/software_isp/software_isp.cpp | 17 ++++----\n>>>>>> 7 files changed, 51 insertions(+), 47 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 0da62290..58fd19db 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 V4L2M2MStream : protected Loggable\n>>>>>> {\n>>>>>> public:\n>>>>>> - V4L2M2MStream(V4L2M2MConverter *converter, unsigned int index);\n>>>>>> + V4L2M2MStream(V4L2M2MConverter *converter, const 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 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>>>>>> 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..09cc0f6f 100644\n>>>>>> --- a/include/libcamera/internal/software_isp/software_isp.h\n>>>>>> +++ b/include/libcamera/internal/software_isp/software_isp.h\n>>>>>> @@ -37,6 +37,7 @@ namespace libcamera {\n>>>>>> class DebayerCpu;\n>>>>>> class FrameBuffer;\n>>>>>> class PixelFormat;\n>>>>>> +class Stream;\n>>>>>> struct StreamConfiguration;\n>>>>>> \n>>>>>> LOG_DECLARE_CATEGORY(SoftwareIsp)\n>>>>>> @@ -62,7 +63,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 +72,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.cpp b/src/libcamera/converter.cpp\n>>>>>> index d3d38c1b..c5e38937 100644\n>>>>>> --- a/src/libcamera/converter.cpp\n>>>>>> +++ b/src/libcamera/converter.cpp\n>>>>>> @@ -111,12 +111,12 @@ Converter::~Converter()\n>>>>>> /**\n>>>>>> * \\fn Converter::exportBuffers()\n>>>>>> * \\brief Export buffers from the converter device\n>>>>>> - * \\param[in] output Output stream index exporting the buffers\n>>>>>> + * \\param[in] stream Output stream pointer exporting the buffers\n>>>>>> * \\param[in] count Number of buffers to allocate\n>>>>>> * \\param[out] buffers Vector to store allocated buffers\n>>>>>> *\n>>>>>> * This function operates similarly to V4L2VideoDevice::exportBuffers() on the\n>>>>>> - * output stream indicated by the \\a output index.\n>>>>>> + * output stream indicated by the \\a output.\n>>>>>> *\n>>>>>> * \\return The number of allocated buffers on success or a negative error code\n>>>>>> * otherwise\n>>>>>> @@ -137,7 +137,7 @@ Converter::~Converter()\n>>>>>> * \\fn Converter::queueBuffers()\n>>>>>> * \\brief Queue buffers to converter device\n>>>>>> * \\param[in] input The frame buffer to apply the conversion\n>>>>>> - * \\param[out] outputs The container holding the output stream indexes and\n>>>>>> + * \\param[out] outputs The container holding the output stream pointer and\n>>>>>> * their respective frame buffer outputs.\n>>>>>> *\n>>>>>> * This function queues the \\a input frame buffer on the output streams of the\n>>>>>> diff --git a/src/libcamera/converter/converter_v4l2_m2m.cpp b/src/libcamera/converter/converter_v4l2_m2m.cpp\n>>>>>> index 6309a0c0..a48b2a87 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::V4L2M2MStream\n>>>>>> */\n>>>>>> \n>>>>>> -V4L2M2MConverter::V4L2M2MStream::V4L2M2MStream(V4L2M2MConverter *converter, unsigned int index)\n>>>>>> - : converter_(converter), index_(index)\n>>>>>> +V4L2M2MConverter::V4L2M2MStream::V4L2M2MStream(V4L2M2MConverter *converter, const Stream *stream)\n>>>>>> + : converter_(converter), stream_(stream)\n>>>>>> {\n>>>>>> m2m_ = std::make_unique(converter->deviceNode());\n>>>>>> \n>>>>>> @@ -157,7 +157,7 @@ int V4L2M2MConverter::V4L2M2MStream::queueBuffers(FrameBuffer *input, FrameBuffe\n>>>>>> \n>>>>>> std::string V4L2M2MConverter::V4L2M2MStream::logPrefix() const\n>>>>>> {\n>>>>>> - return \"stream\" + std::to_string(index_);\n>>>>>> + return stream_->configuration().toString();\n>>>>> I'm still weary about this. What does it print ?\n>>>>>\n>>>>> Though the stream config might be useful to convey which convertor is\n>>>>> being used if there are multiple streams - but I think it should really\n>>>>> be more tied to what the /convertor/ is rather than the stream. Maybe we\n>>>>> should drop the logPrefix or maybe do something better here.\n>>>> This should print the entire stream configuration of the Converter logs,\n>>>> no ?  Earlier it would print \"stream0\" or \"stream1\" which is less\n>>>> expressive IMO\n>>> That's actually what I'm worried about - the entire stream configuration\n>>> sounds like too much, but without a demonstration/printout - I don't know.\n>>>\n>>> The log-prefix is the part of a log message that indicates what the log\n>>> message is from. It's supposed to be 'short'.\n>>>\n>>> But if 'it's just \"1920x1080-MJPEG\" or such - then maybe it's fine.\n>>> That's why I asked \"What does it print?\"\n>> This is what it exactly prints:\n>>\n>> [20:34:50.175903750] [1267]  INFO Converter converter_v4l2_m2m.cpp:155\n>> 2736x1824-NV12: output()->queueBuffer 0xffff84023c50\n>>\n>> Is one of the example. This looks fine to me.\n>>\n>> However, the validility of the  call to stream_->configuration() is\n>> actually depending on the configure() phase. It is documentation as :\n>>\n>> /**\n>>  * \\var Stream::configuration_\n>>  * \\brief The stream configuration\n>>  *\n>>  * The configuration for the stream is set by any successful call to\n>>  * Camera::configure() that includes the stream, and remains valid\n>> until the\n>>  * next call to Camera::configure() regardless of if it includes the\n>> stream.\n>>  */\n>>\n>> So if you try to put a LOG(Converter,...) before the configure(), the\n>> print log prefix comes out to be: \"0x0-\"\n>>\n>> For example:\n>> [20:25:02.595393125] [1251] ERROR Converter converter_v4l2_m2m.cpp:49\n>> 0x0-:  m2m open return : 0\n>>\n>> This is not ideal. Maybe we need to\n>>\n>>\n>>  std::string V4L2M2MConverter::V4L2M2MStream::logPrefix() const\n>>  {\n>> -       return stream_->configuration().toString();\n>> +       if (stream_->configuration().size <= Size(0, 0))\n>> +               return \"\";\n> That's possible indeed... Or just keep it as above.\n>\n> That's sort of where my concern was about using the stream configuration\n> as a way to convey which stream it is. But I don't see a better solution\n> right now so I'm fine with this. It's a corner case that we'll get\n> errors here.\n>\n>\n>> +       else\n>> +               return stream_->configuration().toString();\n>>  }\n>>\n>>\n>> What are your thoughts ?\n>>\n>>> Could you capture a before and after of the logs here and compare them\n>>> please? (Ideally pasting a print of one line before and after here in\n>>> this thread). Highlighting what the change does to the logprefix would\n>>> be something I'd include in the commit message.\n>>>\n>>> \"\"\"\n>>> The logPrefix is no longer able to generate an index from a stream, and\n>>> is updated to be more expressive by reporting the stream configuration\n>>> instead, reporting \"1920x1080-MJPEG\" in place of \"stream0\".\n>>> \"\"\"\n>>>\n>>> (Of course I made up the example, so please ... update with what it\n>>> actually produces!)\n>> Yes, this seems correct.\n> Ok - well I'll leave it to you if you want to post a v4 - it's only the\n> commit message to update I think so may not be worth it.\n\nI'll post a v4 with the updated committed message. I have a couple of \ncalls shuffled in the V4L2M2MConverter::configure() function (trivial \nchange).\n\nI'll keep this version of logPrefix()\n\nstd::string V4L2M2MConverter::V4L2M2MStream::logPrefix() const\n{\n     return stream_->configuration().toString();\n}\n\n>\n> --\n> Kieran\n>\n>\n>>>\n>>>\n>>>\n>>>> I don't have a opinion on this right now, but there might be a better\n>>>> option.\n>>>>> But I don't mind if this is done on top or later though.\n>>>> Seconded. If all agrees...\n>>>>>> }\n>>>>>> \n>>>>>> void V4L2M2MConverter::V4L2M2MStream::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>>>>> Why is this line removed?\n>>>> This is removed because streams_ is not a std::vector anymore (see the\n>>>> change above in the patch). streams_ is now a std::map, for which\n>>>> reserve() is non-existent (rather not makes sense).\n>>> Aha - ok that's fine then.\n>>>\n>>>>>> for (unsigned int i = 0; i < outputCfgs.size(); ++i) {\n>>>>>> - V4L2M2MStream &stream = streams_.emplace_back(this, i);\n>>>>>> + const StreamConfiguration &cfg = outputCfgs[i];\n>>>>>> + streams_.emplace(cfg.stream(),\n>>>>>> + V4L2M2MStream(this, cfg.stream()));\n>>>>>> +\n>>>>>> + V4L2M2MStream &stream = streams_.at(cfg.stream());\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 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 (V4L2M2MStream &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,15 +397,15 @@ int V4L2M2MConverter::start()\n>>>>>> */\n>>>>>> void V4L2M2MConverter::stop()\n>>>>>> {\n>>>>>> - for (V4L2M2MStream &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>>>>>> std::set outputBufs;\n>>>>>> int ret;\n>>>>>> @@ -414,11 +418,9 @@ int V4L2M2MConverter::queueBuffers(FrameBuffer *input,\n>>>>>> if (outputs.empty())\n>>>>>> return -EINVAL;\n>>>>>> \n>>>>>> - for (auto [index, buffer] : outputs) {\n>>>>>> + for (auto [stream, buffer] : outputs) {\n>>>>>> if (!buffer)\n>>>>>> return -EINVAL;\n>>>>>> - if (index >= streams_.size())\n>>>>>> - return -EINVAL;\n>>>>>> \n>>>>>> outputBufs.insert(buffer);\n>>>>>> }\n>>>>>> @@ -427,8 +429,8 @@ int V4L2M2MConverter::queueBuffers(FrameBuffer *input,\n>>>>>> return -EINVAL;\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..01ad91a7 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,10 +1304,8 @@ int SimplePipelineHandler::exportFrameBuffers(Camera *camera, Stream *stream,\n>>>>>> */\n>>>>>> if (data->useConversion_)\n>>>>>> return data->converter_\n>>>>>> - ? data->converter_->exportBuffers(data->streamIndex(stream),\n>>>>>> - count, buffers)\n>>>>>> - : data->swIsp_->exportBuffers(data->streamIndex(stream),\n>>>>>> - count, buffers);\n>>>>>> + ? data->converter_->exportBuffers(stream, count, buffers)\n>>>>>> + : data->swIsp_->exportBuffers(stream, count, buffers);\n>>>>>> else\n>>>>>> return data->video_->exportBuffers(count, buffers);\n>>>>>> }\n>>>>>> @@ -1399,7 +1397,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 +1406,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 ac10d82d..fe0946d7 100644\n>>>>>> --- a/src/libcamera/software_isp/software_isp.cpp\n>>>>>> +++ b/src/libcamera/software_isp/software_isp.cpp\n>>>>>> @@ -221,19 +221,19 @@ int SoftwareIsp::configure(const StreamConfiguration &inputCfg,\n>>>>>> \n>>>>>> /**\n>>>>>> * \\brief Export the buffers from the Software ISP\n>>>>>> - * \\param[in] output Output stream index exporting the buffers\n>>>>>> + * \\param[in] stream Output stream exporting the buffers\n>>>>>> * \\param[in] count Number of buffers to allocate\n>>>>>> * \\param[out] buffers Vector to store the allocated buffers\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>>>>>> \n>>>>>> for (unsigned int i = 0; i < count; i++) {\n>>>>>> @@ -260,12 +260,12 @@ int SoftwareIsp::exportBuffers(unsigned int output, unsigned int count,\n>>>>>> /**\n>>>>>> * \\brief Queue buffers to Software ISP\n>>>>>> * \\param[in] input The input framebuffer\n>>>>>> - * \\param[in] outputs The container holding the output stream indexes and\n>>>>>> + * \\param[in] outputs The container holding the output stream pointer and\n>>>>>> * their respective frame buffer outputs\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>>>>>> /*\n>>>>>> * Validate the outputs as a sanity check: at least one output is\n>>>>>> @@ -274,14 +274,15 @@ int SoftwareIsp::queueBuffers(FrameBuffer *input,\n>>>>>> if (outputs.empty())\n>>>>>> return -EINVAL;\n>>>>>> \n>>>>>> - for (auto [index, buffer] : outputs) {\n>>>>>> + for (auto [stream, buffer] : outputs) {\n>>>>>> if (!buffer)\n>>>>>> return -EINVAL;\n>>>>>> - if (index >= 1) /* only single stream atm */\n>>>>>> + if (outputs.size() != 1) /* only single stream atm */\n>>>>> I still think this \"can't\" happen, or should have been prevented during\n>>>>> configure()/validate() but it's existing code so it's fine to just\n>>>>> update - but I think this error check could be dropped sometime.\n>>>> This pre-existing code in soft ISP was copy/pasted and adjusted\n>>>> accordingly to fit the needs. Proper validation should be done as you\n>>>> suggested (probably orthogonal to this series)\n>>>>> I think the comments above are relatively minor (the dropped reserve\n>>>>> seems like something that could be kept? But it's not so harmful if\n>>>>> there's a legitimate reason to drop it...)\n>>>> commented above.\n>>>>> So\n>>>>>\n>>>>> Reviewed-by: Kieran Bingham \n>>>>>\n>>>>> and lets see if anyone else has anything more to add.\n>>>> I'll wait for more feedback and reviews, as currently I don't see a need\n>>>> for v4 ?\n>>>>\n>>> Yup - this is otherwise fine with me - worst case would be updating the\n>>> commit message while applying perhaps.\n>>>\n>>> --\n>>> Kieran\n>>>\n>>>\n>>>>> --\n>>>>> Kieran\n>>>>>\n>>>>>> return -EINVAL;\n>>>>>> }\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 185C7BDB1D\n\tfor ;\n\tMon, 24 Jun 2024 12:45:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 099E7654A9;\n\tMon, 24 Jun 2024 14:45:07 +0200 (CEST)","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 BF989654A1\n\tfor ;\n\tMon, 24 Jun 2024 14:45:04 +0200 (CEST)","from [IPV6:2405:201:2015:f873:55d7:c02e:b2eb:ee3f] (unknown\n\t[IPv6:2405:201:2015:f873:55d7:c02e:b2eb:ee3f])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 8DF73674;\n\tMon, 24 Jun 2024 14:44:41 +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=\"HgtpX/XR\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1719233082;\n\tbh=vxDk8v7QfNTRJ949UMa7IC4q8BTeFqzvUIQ2XKPJ3os=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=HgtpX/XRmzUxGKoSjK4Gs8QaW/2G1Yk6vPH1uuDKbEiZpPzxr6RLXmJLlyWOTH3CQ\n\tZtYxMbpJVQ980IL6v90o/ISRTAVnQVp7mal4Yt/pFlp/td33oHxykUdqrWTEr216E0\n\tcd1zsGztm1u3h+gmPh/qKejdMQNIvYCIkqQynwus=","Message-ID":"<97eb7fa9-bcf0-4057-b328-997f01092cf8@ideasonboard.com>","Date":"Mon, 24 Jun 2024 18:14:59 +0530","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v3 4/4] libcamera: converter: Replace usage of stream\n\tindex by Stream pointer","To":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org","Cc":"Milan Zamazal ,\n\tAndrey Konovalov ","References":"<20240531052505.150921-1-umang.jain@ideasonboard.com>\n\t<20240531052505.150921-5-umang.jain@ideasonboard.com>\n\t<171715521072.1000377.5290665602081381455@ping.linuxembedded.co.uk>\n\t<0706ba0d-d568-4619-9cea-5bff23fa5b7a@ideasonboard.com>\n\t<171819603389.2248009.16813117071106851040@ping.linuxembedded.co.uk>\n\t\n\t<171922800735.341316.15004106603366992057@ping.linuxembedded.co.uk>","Content-Language":"en-US","From":"Umang Jain ","In-Reply-To":"<171922800735.341316.15004106603366992057@ping.linuxembedded.co.uk>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","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\" "}}]