Patch Detail
Show a patch.
GET /api/1.1/patches/20096/?format=api
{ "id": 20096, "url": "https://patchwork.libcamera.org/api/1.1/patches/20096/?format=api", "web_url": "https://patchwork.libcamera.org/patch/20096/", "project": { "id": 1, "url": "https://patchwork.libcamera.org/api/1.1/projects/1/?format=api", "name": "libcamera", "link_name": "libcamera", "list_id": "libcamera_core", "list_email": "libcamera-devel@lists.libcamera.org", "web_url": "", "scm_url": "", "webscm_url": "" }, "msgid": "<20240524065419.94005-1-umang.jain@ideasonboard.com>", "date": "2024-05-24T06:54:19", "name": "[RFC] libcamera: converter: Replace usage of stream index by Stream pointer", "commit_ref": null, "pull_url": null, "state": "superseded", "archived": false, "hash": "cac0316c8481c6fbb2b8a44b662b5e75e0d1f457", "submitter": { "id": 86, "url": "https://patchwork.libcamera.org/api/1.1/people/86/?format=api", "name": "Umang Jain", "email": "umang.jain@ideasonboard.com" }, "delegate": null, "mbox": "https://patchwork.libcamera.org/patch/20096/mbox/", "series": [ { "id": 4324, "url": "https://patchwork.libcamera.org/api/1.1/series/4324/?format=api", "web_url": "https://patchwork.libcamera.org/project/libcamera/list/?series=4324", "date": "2024-05-24T06:54:19", "name": "[RFC] libcamera: converter: Replace usage of stream index by Stream pointer", "version": 1, "mbox": "https://patchwork.libcamera.org/series/4324/mbox/" } ], "comments": "https://patchwork.libcamera.org/api/patches/20096/comments/", "check": "pending", "checks": "https://patchwork.libcamera.org/api/patches/20096/checks/", "tags": {}, "headers": { "Return-Path": "<libcamera-devel-bounces@lists.libcamera.org>", "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 8B73ABD87C\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri, 24 May 2024 06:54:29 +0000 (UTC)", "from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 64AAC634A0;\n\tFri, 24 May 2024 08:54:28 +0200 (CEST)", "from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DF30861A47\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 24 May 2024 08:54:25 +0200 (CEST)", "from fedora.local (unknown\n\t[IPv6:2405:201:2015:f873:c173:4b:4a04:3a21])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 279C8475;\n\tFri, 24 May 2024 08:54:10 +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=\"REz/zDyX\"; dkim-atps=neutral", "DKIM-Signature": "v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1716533651;\n\tbh=Goo4ZmpOfPTOJJpFOeCMWNWYc6eGStZ6Pl87lBdFILY=;\n\th=From:To:Cc:Subject:Date:From;\n\tb=REz/zDyXPzJJ+YMQ0r9a2wjstdlCCgldkDCL6aWe5cQuGh0v2uBg2LiO9ShJ4UJZt\n\t4KSlekJq5uhg6n/hEUxzCNNeW72CVhXUltGldG+aFCpuoUKawzvtRY6i05L8R7R3f+\n\tGyoxMQ9c1fwerwiWOiYqn1Z8B7RgwzfrwH4lHq5o=", "From": "Umang Jain <umang.jain@ideasonboard.com>", "To": "libcamera-devel@lists.libcamera.org", "Cc": "Umang Jain <umang.jain@ideasonboard.com>", "Subject": "[RFC PATCH] libcamera: converter: Replace usage of stream index by\n\tStream pointer", "Date": "Fri, 24 May 2024 12:24:19 +0530", "Message-ID": "<20240524065419.94005-1-umang.jain@ideasonboard.com>", "X-Mailer": "git-send-email 2.44.0", "MIME-Version": "1.0", "Content-Transfer-Encoding": "8bit", "X-BeenThere": "libcamera-devel@lists.libcamera.org", "X-Mailman-Version": "2.1.29", "Precedence": "list", "List-Id": "<libcamera-devel.lists.libcamera.org>", "List-Unsubscribe": "<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>", "List-Archive": "<https://lists.libcamera.org/pipermail/libcamera-devel/>", "List-Post": "<mailto:libcamera-devel@lists.libcamera.org>", "List-Help": "<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>", "List-Subscribe": "<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>", "Errors-To": "libcamera-devel-bounces@lists.libcamera.org", "Sender": "\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>" }, "content": "The converter interface uses the unsigned int output stream index to map\nto the output frame buffers. This is cumbersome to implement new\nconverters because one has to keep around additional book keeping\nto track the streams with their correct indexes.\n\nThis patch (still an RFC) intends to drop stream index usage.\nThe index is replaced by Stream pointer. This is convenient since\nStream pointers are easily accessible at configuration time (from\nStreamConfiguration) and from Request objects.\n\nThe v4l2_converter_m2m and simple pipeline handler are adapt to\nuse the new interface. This work roped in software ISP as well,\nwhich also seems to use indexes (although it doesn't implement converter\ninterface) because of a common conversionQueue_ queue used for\nconverter_ and swIsp_.\n\nSigned-off-by: Umang Jain <umang.jain@ideasonboard.com>\n---\n\nThe patch is an RFC and needs more work to be finalised. I want to check\nif this direction is decent enough to take since this is going to block\nmy DW100 converter rework.\n\n- [PATCH v2 0/4] libcamera: rkisp1: Plumb i.MX8MP DW100 dewarper\n\n- Some \\todos still around validation of outputs framebuffers.\n- compile tested with -Dpipelines=rkisp1,simple \n\n---\n include/libcamera/internal/converter.h | 5 +-\n .../internal/converter/converter_v4l2_m2m.h | 11 +++--\n .../internal/software_isp/software_isp.h | 4 +-\n .../converter/converter_v4l2_m2m.cpp | 47 +++++++++----------\n src/libcamera/pipeline/simple/simple.cpp | 12 ++---\n src/libcamera/software_isp/software_isp.cpp | 22 ++++-----\n 6 files changed, 46 insertions(+), 55 deletions(-)", "diff": "diff --git a/include/libcamera/internal/converter.h b/include/libcamera/internal/converter.h\nindex 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 \tvirtual int configure(const StreamConfiguration &inputCfg,\n \t\t\t const std::vector<std::reference_wrapper<StreamConfiguration>> &outputCfgs) = 0;\n-\tvirtual int exportBuffers(unsigned int output, unsigned int count,\n+\tvirtual int exportBuffers(const Stream *stream, unsigned int count,\n \t\t\t\t std::vector<std::unique_ptr<FrameBuffer>> *buffers) = 0;\n \n \tvirtual int start() = 0;\n \tvirtual void stop() = 0;\n \n \tvirtual int queueBuffers(FrameBuffer *input,\n-\t\t\t\t const std::map<unsigned int, FrameBuffer *> &outputs) = 0;\n+\t\t\t\t const std::map<const Stream *, FrameBuffer *> &outputs) = 0;\n \n \tSignal<FrameBuffer *> inputBufferReady;\n \tSignal<FrameBuffer *> outputBufferReady;\ndiff --git a/include/libcamera/internal/converter/converter_v4l2_m2m.h b/include/libcamera/internal/converter/converter_v4l2_m2m.h\nindex 1126050c..711a1a5f 100644\n--- a/include/libcamera/internal/converter/converter_v4l2_m2m.h\n+++ b/include/libcamera/internal/converter/converter_v4l2_m2m.h\n@@ -28,6 +28,7 @@ class FrameBuffer;\n class MediaDevice;\n class Size;\n class SizeRange;\n+class Stream;\n struct StreamConfiguration;\n class V4L2M2MDevice;\n \n@@ -47,20 +48,20 @@ public:\n \n \tint configure(const StreamConfiguration &inputCfg,\n \t\t const std::vector<std::reference_wrapper<StreamConfiguration>> &outputCfg);\n-\tint exportBuffers(unsigned int output, unsigned int count,\n+\tint exportBuffers(const Stream *stream, unsigned int count,\n \t\t\t std::vector<std::unique_ptr<FrameBuffer>> *buffers);\n \n \tint start();\n \tvoid stop();\n \n \tint queueBuffers(FrameBuffer *input,\n-\t\t\t const std::map<unsigned int, FrameBuffer *> &outputs);\n+\t\t\t const std::map<const Stream *, FrameBuffer *> &outputs);\n \n private:\n \tclass Stream : protected Loggable\n \t{\n \tpublic:\n-\t\tStream(V4L2M2MConverter *converter, unsigned int index);\n+\t\tStream(V4L2M2MConverter *converter, const libcamera::Stream *stream);\n \n \t\tbool isValid() const { return m2m_ != nullptr; }\n \n@@ -82,7 +83,7 @@ private:\n \t\tvoid outputBufferReady(FrameBuffer *buffer);\n \n \t\tV4L2M2MConverter *converter_;\n-\t\tunsigned int index_;\n+\t\tconst libcamera::Stream *stream_;\n \t\tstd::unique_ptr<V4L2M2MDevice> m2m_;\n \n \t\tunsigned int inputBufferCount_;\n@@ -91,7 +92,7 @@ private:\n \n \tstd::unique_ptr<V4L2M2MDevice> m2m_;\n \n-\tstd::vector<Stream> streams_;\n+\tstd::map<const libcamera::Stream *, Stream> streams_;\n \tstd::map<FrameBuffer *, unsigned int> queue_;\n };\n \ndiff --git a/include/libcamera/internal/software_isp/software_isp.h b/include/libcamera/internal/software_isp/software_isp.h\nindex 7e9fae6a..be3d1b2f 100644\n--- a/include/libcamera/internal/software_isp/software_isp.h\n+++ b/include/libcamera/internal/software_isp/software_isp.h\n@@ -62,7 +62,7 @@ public:\n \t\t const std::vector<std::reference_wrapper<StreamConfiguration>> &outputCfgs,\n \t\t const ControlInfoMap &sensorControls);\n \n-\tint exportBuffers(unsigned int output, unsigned int count,\n+\tint exportBuffers(const Stream *stream, unsigned int count,\n \t\t\t std::vector<std::unique_ptr<FrameBuffer>> *buffers);\n \n \tvoid processStats(const ControlList &sensorControls);\n@@ -71,7 +71,7 @@ public:\n \tvoid stop();\n \n \tint queueBuffers(FrameBuffer *input,\n-\t\t\t const std::map<unsigned int, FrameBuffer *> &outputs);\n+\t\t\t const std::map<const Stream *, FrameBuffer *> &outputs);\n \n \tvoid process(FrameBuffer *input, FrameBuffer *output);\n \ndiff --git a/src/libcamera/converter/converter_v4l2_m2m.cpp b/src/libcamera/converter/converter_v4l2_m2m.cpp\nindex d8929fc5..e0619689 100644\n--- a/src/libcamera/converter/converter_v4l2_m2m.cpp\n+++ b/src/libcamera/converter/converter_v4l2_m2m.cpp\n@@ -35,8 +35,8 @@ LOG_DECLARE_CATEGORY(Converter)\n * V4L2M2MConverter::Stream\n */\n \n-V4L2M2MConverter::Stream::Stream(V4L2M2MConverter *converter, unsigned int index)\n-\t: converter_(converter), index_(index)\n+V4L2M2MConverter::Stream::Stream(V4L2M2MConverter *converter, const libcamera::Stream *stream)\n+\t: converter_(converter), stream_(stream)\n {\n \tm2m_ = std::make_unique<V4L2M2MDevice>(converter->deviceNode());\n \n@@ -157,7 +157,7 @@ int V4L2M2MConverter::Stream::queueBuffers(FrameBuffer *input, FrameBuffer *outp\n \n std::string V4L2M2MConverter::Stream::logPrefix() const\n {\n-\treturn \"stream\" + std::to_string(index_);\n+\treturn stream_->configuration().toString();\n }\n \n void V4L2M2MConverter::Stream::outputBufferReady(FrameBuffer *buffer)\n@@ -333,10 +333,13 @@ int V4L2M2MConverter::configure(const StreamConfiguration &inputCfg,\n \tint ret = 0;\n \n \tstreams_.clear();\n-\tstreams_.reserve(outputCfgs.size());\n \n \tfor (unsigned int i = 0; i < outputCfgs.size(); ++i) {\n-\t\tStream &stream = streams_.emplace_back(this, i);\n+\t\tconst StreamConfiguration &cfg = outputCfgs[i];\n+\t\tstreams_.emplace(cfg.stream(),\n+\t\t\t\t Stream(this, cfg.stream()));\n+\n+\t\tStream &stream = streams_.at(cfg.stream());\n \n \t\tif (!stream.isValid()) {\n \t\t\tLOG(Converter, Error)\n@@ -345,7 +348,7 @@ int V4L2M2MConverter::configure(const StreamConfiguration &inputCfg,\n \t\t\tbreak;\n \t\t}\n \n-\t\tret = stream.configure(inputCfg, outputCfgs[i]);\n+\t\tret = stream.configure(inputCfg, cfg);\n \t\tif (ret < 0)\n \t\t\tbreak;\n \t}\n@@ -361,13 +364,14 @@ int V4L2M2MConverter::configure(const StreamConfiguration &inputCfg,\n /**\n * \\copydoc libcamera::Converter::exportBuffers\n */\n-int V4L2M2MConverter::exportBuffers(unsigned int output, unsigned int count,\n+int V4L2M2MConverter::exportBuffers(const libcamera::Stream *stream, unsigned int count,\n \t\t\t\t std::vector<std::unique_ptr<FrameBuffer>> *buffers)\n {\n-\tif (output >= streams_.size())\n+\tauto iter = streams_.find(stream);\n+\tif (iter == streams_.end())\n \t\treturn -EINVAL;\n \n-\treturn streams_[output].exportBuffers(count, buffers);\n+\treturn iter->second.exportBuffers(count, buffers);\n }\n \n /**\n@@ -377,8 +381,8 @@ int V4L2M2MConverter::start()\n {\n \tint ret;\n \n-\tfor (Stream &stream : streams_) {\n-\t\tret = stream.start();\n+\tfor (auto &iter : streams_) {\n+\t\tret = iter.second.start();\n \t\tif (ret < 0) {\n \t\t\tstop();\n \t\t\treturn ret;\n@@ -393,17 +397,16 @@ int V4L2M2MConverter::start()\n */\n void V4L2M2MConverter::stop()\n {\n-\tfor (Stream &stream : utils::reverse(streams_))\n-\t\tstream.stop();\n+\tfor (auto &iter : streams_)\n+\t\titer.second.stop();\n }\n \n /**\n * \\copydoc libcamera::Converter::queueBuffers\n */\n int V4L2M2MConverter::queueBuffers(FrameBuffer *input,\n-\t\t\t\t const std::map<unsigned int, FrameBuffer *> &outputs)\n+\t\t\t\t const std::map<const libcamera::Stream *, FrameBuffer *> &outputs)\n {\n-\tunsigned int mask = 0;\n \tint ret;\n \n \t/*\n@@ -414,20 +417,12 @@ int V4L2M2MConverter::queueBuffers(FrameBuffer *input,\n \tif (outputs.empty())\n \t\treturn -EINVAL;\n \n-\tfor (auto [index, buffer] : outputs) {\n-\t\tif (!buffer)\n-\t\t\treturn -EINVAL;\n-\t\tif (index >= streams_.size())\n-\t\t\treturn -EINVAL;\n-\t\tif (mask & (1 << index))\n-\t\t\treturn -EINVAL;\n \n-\t\tmask |= 1 << index;\n-\t}\n+\t/* \\TODO: validate outputs against streams */\n \n \t/* Queue the input and output buffers to all the streams. */\n-\tfor (auto [index, buffer] : outputs) {\n-\t\tret = streams_[index].queueBuffers(input, buffer);\n+\tfor (auto [stream, buffer] : outputs) {\n+\t\tret = streams_.at(stream).queueBuffers(input, buffer);\n \t\tif (ret < 0)\n \t\t\treturn ret;\n \t}\ndiff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\nindex db3575c3..c22b2f89 100644\n--- a/src/libcamera/pipeline/simple/simple.cpp\n+++ b/src/libcamera/pipeline/simple/simple.cpp\n@@ -278,7 +278,7 @@ public:\n \tstd::map<PixelFormat, std::vector<const Configuration *>> formats_;\n \n \tstd::vector<std::unique_ptr<FrameBuffer>> conversionBuffers_;\n-\tstd::queue<std::map<unsigned int, FrameBuffer *>> conversionQueue_;\n+\tstd::queue<std::map<const Stream *, FrameBuffer *>> conversionQueue_;\n \tbool useConversion_;\n \n \tstd::unique_ptr<Converter> converter_;\n@@ -837,7 +837,7 @@ void SimpleCameraData::bufferReady(FrameBuffer *buffer)\n \tRequest *request = buffer->request();\n \n \tif (useConversion_ && !conversionQueue_.empty()) {\n-\t\tconst std::map<unsigned int, FrameBuffer *> &outputs =\n+\t\tconst std::map<const Stream *, FrameBuffer *> &outputs =\n \t\t\tconversionQueue_.front();\n \t\tif (!outputs.empty()) {\n \t\t\tFrameBuffer *outputBuffer = outputs.begin()->second;\n@@ -1304,9 +1304,9 @@ int SimplePipelineHandler::exportFrameBuffers(Camera *camera, Stream *stream,\n \t */\n \tif (data->useConversion_)\n \t\treturn data->converter_\n-\t\t\t ? data->converter_->exportBuffers(data->streamIndex(stream),\n+\t\t\t ? data->converter_->exportBuffers(stream,\n \t\t\t\t\t\t\t\t count, buffers)\n-\t\t\t : data->swIsp_->exportBuffers(data->streamIndex(stream),\n+\t\t\t : data->swIsp_->exportBuffers(stream,\n \t\t\t\t\t\t\t count, buffers);\n \telse\n \t\treturn data->video_->exportBuffers(count, buffers);\n@@ -1399,7 +1399,7 @@ int SimplePipelineHandler::queueRequestDevice(Camera *camera, Request *request)\n \tSimpleCameraData *data = cameraData(camera);\n \tint ret;\n \n-\tstd::map<unsigned int, FrameBuffer *> buffers;\n+\tstd::map<const Stream *, FrameBuffer *> buffers;\n \n \tfor (auto &[stream, buffer] : request->buffers()) {\n \t\t/*\n@@ -1408,7 +1408,7 @@ int SimplePipelineHandler::queueRequestDevice(Camera *camera, Request *request)\n \t\t * completion handler.\n \t\t */\n \t\tif (data->useConversion_) {\n-\t\t\tbuffers.emplace(data->streamIndex(stream), buffer);\n+\t\t\tbuffers.emplace(stream, buffer);\n \t\t} else {\n \t\t\tret = data->video_->queueBuffer(buffer);\n \t\t\tif (ret < 0)\ndiff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp\nindex c9b6be56..b81b90bd 100644\n--- a/src/libcamera/software_isp/software_isp.cpp\n+++ b/src/libcamera/software_isp/software_isp.cpp\n@@ -227,13 +227,13 @@ int SoftwareIsp::configure(const StreamConfiguration &inputCfg,\n * \\return The number of allocated buffers on success or a negative error code\n * otherwise\n */\n-int SoftwareIsp::exportBuffers(unsigned int output, unsigned int count,\n+int SoftwareIsp::exportBuffers(const Stream *stream, unsigned int count,\n \t\t\t std::vector<std::unique_ptr<FrameBuffer>> *buffers)\n {\n \tASSERT(debayer_ != nullptr);\n \n \t/* single output for now */\n-\tif (output >= 1)\n+\tif (stream != nullptr)\n \t\treturn -EINVAL;\n \n \tfor (unsigned int i = 0; i < count; i++) {\n@@ -265,9 +265,8 @@ int SoftwareIsp::exportBuffers(unsigned int output, unsigned int count,\n * \\return 0 on success, a negative errno on failure\n */\n int SoftwareIsp::queueBuffers(FrameBuffer *input,\n-\t\t\t const std::map<unsigned int, FrameBuffer *> &outputs)\n+\t\t\t const std::map<const Stream *, FrameBuffer *> &outputs)\n {\n-\tunsigned int mask = 0;\n \n \t/*\n \t * Validate the outputs as a sanity check: at least one output is\n@@ -277,18 +276,13 @@ int SoftwareIsp::queueBuffers(FrameBuffer *input,\n \tif (outputs.empty())\n \t\treturn -EINVAL;\n \n-\tfor (auto [index, buffer] : outputs) {\n-\t\tif (!buffer)\n-\t\t\treturn -EINVAL;\n-\t\tif (index >= 1) /* only single stream atm */\n-\t\t\treturn -EINVAL;\n-\t\tif (mask & (1 << index))\n-\t\t\treturn -EINVAL;\n+\t/* \\todo validate outputs against streams*/\n \n-\t\tmask |= 1 << index;\n-\t}\n+\tif (outputs.size() != 1) /* only single stream atm */\n+\t\treturn -EINVAL;\n \n-\tprocess(input, outputs.at(0));\n+\tfor (auto iter = outputs.begin(); iter != outputs.end(); iter++)\n+\t\tprocess(input, iter->second);\n \n \treturn 0;\n }\n", "prefixes": [ "RFC" ] }