[{"id":29100,"web_url":"https://patchwork.libcamera.org/comment/29100/","msgid":"<20240327172707.GH4721@pendragon.ideasonboard.com>","date":"2024-03-27T17:27:07","subject":"Re: [PATCH v6 10/18] libcamera: introduce SoftwareIsp","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Milan and Andrey,\n\nThank you for the patch.\n\nOn Tue, Mar 19, 2024 at 01:35:57PM +0100, Milan Zamazal wrote:\n> From: Andrey Konovalov <andrey.konovalov@linaro.org>\n> \n> Doxygen documentation by Dennis Bonke.\n> \n> Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> # sc8280xp Lenovo x13s\n> Tested-by: Pavel Machek <pavel@ucw.cz>\n> Reviewed-by: Pavel Machek <pavel@ucw.cz>\n> Co-developed-by: Dennis Bonke <admin@dennisbonke.com>\n> Signed-off-by: Dennis Bonke <admin@dennisbonke.com>\n> Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>\n> Signed-off-by: Hans de Goede <hdegoede@redhat.com>\n> ---\n>  .../internal/software_isp/meson.build         |   1 +\n>  .../internal/software_isp/software_isp.h      |  98 +++++\n>  src/libcamera/software_isp/meson.build        |   1 +\n>  src/libcamera/software_isp/software_isp.cpp   | 349 ++++++++++++++++++\n>  4 files changed, 449 insertions(+)\n>  create mode 100644 include/libcamera/internal/software_isp/software_isp.h\n>  create mode 100644 src/libcamera/software_isp/software_isp.cpp\n> \n> diff --git a/include/libcamera/internal/software_isp/meson.build b/include/libcamera/internal/software_isp/meson.build\n> index a620e16d..508ddddc 100644\n> --- a/include/libcamera/internal/software_isp/meson.build\n> +++ b/include/libcamera/internal/software_isp/meson.build\n> @@ -2,5 +2,6 @@\n>  \n>  libcamera_internal_headers += files([\n>      'debayer_params.h',\n> +    'software_isp.h',\n>      'swisp_stats.h',\n>  ])\n> diff --git a/include/libcamera/internal/software_isp/software_isp.h b/include/libcamera/internal/software_isp/software_isp.h\n> new file mode 100644\n> index 00000000..8d25e979\n> --- /dev/null\n> +++ b/include/libcamera/internal/software_isp/software_isp.h\n> @@ -0,0 +1,98 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2023, Linaro Ltd\n> + *\n> + * software_isp.h - Simple software ISP implementation\n> + */\n> +\n> +#pragma once\n> +\n> +#include <functional>\n> +#include <initializer_list>\n> +#include <map>\n> +#include <memory>\n> +#include <string>\n> +#include <tuple>\n> +#include <vector>\n> +\n> +#include <libcamera/base/class.h>\n> +#include <libcamera/base/log.h>\n> +#include <libcamera/base/signal.h>\n> +#include <libcamera/base/thread.h>\n> +\n> +#include <libcamera/geometry.h>\n> +#include <libcamera/pixel_format.h>\n> +\n> +#include <libcamera/ipa/soft_ipa_interface.h>\n> +#include <libcamera/ipa/soft_ipa_proxy.h>\n> +\n> +#include \"libcamera/internal/dma_heaps.h\"\n> +#include \"libcamera/internal/pipeline_handler.h\"\n> +#include \"libcamera/internal/shared_mem_object.h\"\n> +#include \"libcamera/internal/software_isp/debayer_params.h\"\n> +\n> +namespace libcamera {\n> +\n> +class DebayerCpu;\n> +class FrameBuffer;\n> +class PixelFormat;\n> +struct StreamConfiguration;\n> +\n> +LOG_DECLARE_CATEGORY(SoftwareIsp)\n> +\n> +class SoftwareIsp\n> +{\n> +public:\n> +\tSoftwareIsp(PipelineHandler *pipe, const ControlInfoMap &sensorControls);\n> +\t~SoftwareIsp();\n> +\n> +\tint loadConfiguration([[maybe_unused]] const std::string &filename) { return 0; }\n> +\n> +\tbool isValid() const;\n> +\n> +\tstd::vector<PixelFormat> formats(PixelFormat input);\n> +\n> +\tSizeRange sizes(PixelFormat inputFormat, const Size &inputSize);\n> +\n> +\tstd::tuple<unsigned int, unsigned int>\n> +\tstrideAndFrameSize(const PixelFormat &outputFormat, const Size &size);\n> +\n> +\tint configure(const StreamConfiguration &inputCfg,\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> +\t\t\t  std::vector<std::unique_ptr<FrameBuffer>> *buffers);\n> +\n> +\tvoid processStats(const ControlList &sensorControls);\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> +\n> +\tvoid process(FrameBuffer *input, FrameBuffer *output);\n> +\n> +\tSignal<FrameBuffer *> inputBufferReady;\n> +\tSignal<FrameBuffer *> outputBufferReady;\n> +\tSignal<int> ispStatsReady;\n> +\tSignal<const ControlList &> setSensorControls;\n> +\n> +private:\n> +\tvoid saveIspParams(int dummy);\n> +\tvoid setSensorCtrls(const ControlList &sensorControls);\n> +\tvoid statsReady(int dummy);\n> +\tvoid inputReady(FrameBuffer *input);\n> +\tvoid outputReady(FrameBuffer *output);\n> +\n> +\tstd::unique_ptr<DebayerCpu> debayer_;\n> +\tThread ispWorkerThread_;\n> +\tSharedMemObject<DebayerParams> sharedParams_;\n> +\tDebayerParams debayerParams_;\n> +\tDmaHeap dmaHeap_;\n> +\n> +\tstd::unique_ptr<ipa::soft::IPAProxySoft> ipa_;\n> +};\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/software_isp/meson.build b/src/libcamera/software_isp/meson.build\n> index 71b46539..e9266e54 100644\n> --- a/src/libcamera/software_isp/meson.build\n> +++ b/src/libcamera/software_isp/meson.build\n> @@ -10,5 +10,6 @@ endif\n>  libcamera_sources += files([\n>      'debayer.cpp',\n>      'debayer_cpu.cpp',\n> +    'software_isp.cpp',\n>      'swstats_cpu.cpp',\n>  ])\n> diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp\n> new file mode 100644\n> index 00000000..388b4496\n> --- /dev/null\n> +++ b/src/libcamera/software_isp/software_isp.cpp\n> @@ -0,0 +1,349 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2023, Linaro Ltd\n> + *\n> + * software_isp.cpp - Simple software ISP implementation\n> + */\n> +\n> +#include \"libcamera/internal/software_isp/software_isp.h\"\n> +\n> +#include <sys/mman.h>\n> +#include <sys/types.h>\n> +#include <unistd.h>\n> +\n> +#include <libcamera/formats.h>\n> +#include <libcamera/stream.h>\n> +\n> +#include \"libcamera/internal/bayer_format.h\"\n> +#include \"libcamera/internal/framebuffer.h\"\n> +#include \"libcamera/internal/ipa_manager.h\"\n> +#include \"libcamera/internal/mapped_framebuffer.h\"\n> +\n> +#include \"debayer_cpu.h\"\n> +\n> +/**\n> + * \\file software_isp.cpp\n> + * \\brief Simple software ISP implementation\n> + */\n> +\n> +namespace libcamera {\n> +\n> +LOG_DEFINE_CATEGORY(SoftwareIsp)\n> +\n> +/**\n> + * \\class SoftwareIsp\n> + * \\brief Class for the Software ISP\n> + */\n> +\n> +/**\n> + * \\var SoftwareIsp::inputBufferReady\n> + * \\brief A signal emitted when the input frame buffer completes\n> + */\n> +\n> +/**\n> + * \\var SoftwareIsp::outputBufferReady\n> + * \\brief A signal emitted when the output frame buffer completes\n> + */\n> +\n> +/**\n> + * \\var SoftwareIsp::ispStatsReady\n> + * \\brief A signal emitted when the statistics for IPA are ready\n> + *\n> + * The int parameter isn't actually used.\n\nDrop it :-)\n\n> + */\n> +\n> +/**\n> + * \\var SoftwareIsp::setSensorControls\n\nTo make components reusable, signals should be named based on the event\nthey report, not based on what the connected slot is expected to do.\n\n> + * \\brief A signal emitted when the values to write to the sensor controls are ready\n> + */\n> +\n> +/**\n> + * \\brief Constructs SoftwareIsp object\n> + * \\param[in] pipe The pipeline handler in use\n> + * \\param[in] sensorControls ControlInfoMap describing the controls supported by the sensor\n> + */\n> +SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const ControlInfoMap &sensorControls)\n> +\t: debayer_(nullptr),\n\nNot needed, the default unique_ptr<> constructor will handle that.\n\n> +\t  debayerParams_{ DebayerParams::kGain10, DebayerParams::kGain10, DebayerParams::kGain10, 0.5f },\n> +\t  dmaHeap_(DmaHeap::DmaHeapFlag::Cma | DmaHeap::DmaHeapFlag::System)\n> +{\n> +\tif (!dmaHeap_.isValid()) {\n> +\t\tLOG(SoftwareIsp, Error) << \"Failed to create DmaHeap object\";\n> +\t\treturn;\n> +\t}\n> +\n> +\tsharedParams_ = SharedMemObject<DebayerParams>(\"softIsp_params\");\n> +\tif (!sharedParams_) {\n> +\t\tLOG(SoftwareIsp, Error) << \"Failed to create shared memory for parameters\";\n> +\t\treturn;\n> +\t}\n> +\n> +\tauto stats = std::make_unique<SwStatsCpu>();\n> +\tif (!stats->isValid()) {\n> +\t\tLOG(SoftwareIsp, Error) << \"Failed to create SwStatsCpu object\";\n> +\t\treturn;\n> +\t}\n> +\tstats->statsReady.connect(this, &SoftwareIsp::statsReady);\n> +\n> +\tdebayer_ = std::make_unique<DebayerCpu>(std::move(stats));\n> +\tdebayer_->inputBufferReady.connect(this, &SoftwareIsp::inputReady);\n> +\tdebayer_->outputBufferReady.connect(this, &SoftwareIsp::outputReady);\n> +\n> +\tipa_ = IPAManager::createIPA<ipa::soft::IPAProxySoft>(pipe, 0, 0);\n> +\tif (!ipa_) {\n> +\t\tLOG(SoftwareIsp, Error)\n> +\t\t\t<< \"Creating IPA for software ISP failed\";\n> +\t\tdebayer_.reset();\n\nIs this needed, or can we let the destructor handle it ?\n\n> +\t\treturn;\n> +\t}\n> +\n> +\tint ret = ipa_->init(IPASettings{ \"No cfg file\", \"No sensor model\" },\n\nThe change from \"No sensor model\" to using the CameraSensor class, as\ndone in patch 18/18, should be squashed in this patch.\n\n> +\t\t\t     debayer_->getStatsFD(),\n> +\t\t\t     sharedParams_.fd(),\n> +\t\t\t     sensorControls);\n> +\tif (ret) {\n> +\t\tLOG(SoftwareIsp, Error) << \"IPA init failed\";\n> +\t\tdebayer_.reset();\n> +\t\treturn;\n> +\t}\n> +\n> +\tipa_->setIspParams.connect(this, &SoftwareIsp::saveIspParams);\n> +\tipa_->setSensorControls.connect(this, &SoftwareIsp::setSensorCtrls);\n> +\n> +\tdebayer_->moveToThread(&ispWorkerThread_);\n> +}\n> +\n> +SoftwareIsp::~SoftwareIsp()\n> +{\n> +\t/* make sure to destroy the DebayerCpu before the ispWorkerThread_ is gone */\n> +\tdebayer_.reset();\n> +}\n> +\n> +/**\n> + * \\fn int SoftwareIsp::loadConfiguration([[maybe_unused]] const std::string &filename)\n> + * \\brief Load a configuration from a file\n> + * \\param[in] filename The file to load the configuration data from\n> + *\n> + * Currently is a stub doing nothing and always returning \"success\".\n> + *\n> + * \\return 0 on success\n> + */\n> +\n> +/**\n> + * \\brief Process the statistics gathered\n> + * \\param[in] sensorControls The sensor controls\n> + *\n> + * Requests the IPA to calculate new parameters for ISP and new control\n> + * values for the sensor.\n> + */\n> +void SoftwareIsp::processStats(const ControlList &sensorControls)\n> +{\n> +\tASSERT(ipa_);\n> +\tipa_->processStats(sensorControls);\n> +}\n> +\n> +/**\n> + * \\brief Check the validity of Software Isp object\n> + * \\return True if Software Isp is valid, false otherwise\n> + */\n> +bool SoftwareIsp::isValid() const\n> +{\n> +\treturn !!debayer_;\n> +}\n> +\n> +/**\n> +  * \\brief Get the output formats supported for the given input format\n> +  * \\param[in] inputFormat The input format\n> +  * \\return All the supported output formats or an empty vector if there are none\n> +  */\n> +std::vector<PixelFormat> SoftwareIsp::formats(PixelFormat inputFormat)\n> +{\n> +\tASSERT(debayer_ != nullptr);\n\nYou can write\n\n\tASSERT(debayer_);\n\nSame below.\n\n> +\n> +\treturn debayer_->formats(inputFormat);\n> +}\n> +\n> +/**\n> + * \\brief Get the supported output sizes for the given input format and size\n> + * \\param[in] inputFormat The input format\n> + * \\param[in] inputSize The input frame size\n> + * \\return The valid size range or an empty range if there are none\n> + */\n> +SizeRange SoftwareIsp::sizes(PixelFormat inputFormat, const Size &inputSize)\n> +{\n> +\tASSERT(debayer_ != nullptr);\n> +\n> +\treturn debayer_->sizes(inputFormat, inputSize);\n> +}\n> +\n> +/**\n> + * Get the output stride and the frame size in bytes for the given output format and size\n> + * \\param[in] outputFormat The output format\n> + * \\param[in] size The output size (width and height in pixels)\n> + * \\return A tuple of the stride and the frame size in bytes, or a tuple of 0,0\n> + * if there is no valid output config\n> + */\n> +std::tuple<unsigned int, unsigned int>\n> +SoftwareIsp::strideAndFrameSize(const PixelFormat &outputFormat, const Size &size)\n> +{\n> +\tASSERT(debayer_ != nullptr);\n> +\n> +\treturn debayer_->strideAndFrameSize(outputFormat, size);\n> +}\n> +\n> +/**\n> + * \\brief Configure the SoftwareIsp object according to the passed in parameters\n> + * \\param[in] inputCfg The input configuration\n> + * \\param[in] outputCfgs The output configurations\n> + * \\param[in] sensorControls ControlInfoMap of the controls supported by the sensor\n> + * \\return 0 on success, a negative errno on failure\n> + */\n> +int SoftwareIsp::configure(const StreamConfiguration &inputCfg,\n> +\t\t\t   const std::vector<std::reference_wrapper<StreamConfiguration>> &outputCfgs,\n> +\t\t\t   const ControlInfoMap &sensorControls)\n> +{\n> +\tASSERT(ipa_ != nullptr && debayer_ != nullptr);\n\nIs there a specific reason to check ipa_ here ?\n\n> +\n> +\tint ret = ipa_->configure(sensorControls);\n> +\tif (ret < 0)\n> +\t\treturn ret;\n> +\n> +\treturn debayer_->configure(inputCfg, outputCfgs);\n> +}\n> +\n> +/**\n> + * \\brief Export the buffers from the Software ISP\n> + * \\param[in] output Output stream index 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> +\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> +\t\treturn -EINVAL;\n> +\n> +\tfor (unsigned int i = 0; i < count; i++) {\n> +\t\tconst std::string name = \"frame-\" + std::to_string(i);\n> +\t\tconst size_t frameSize = debayer_->frameSize();\n> +\n> +\t\tFrameBuffer::Plane outPlane;\n> +\t\toutPlane.fd = SharedFD(dmaHeap_.alloc(name.c_str(), frameSize));\n> +\t\tif (!outPlane.fd.isValid()) {\n> +\t\t\tLOG(SoftwareIsp, Error)\n> +\t\t\t\t<< \"failed to allocate a dma_buf\";\n> +\t\t\treturn -ENOMEM;\n> +\t\t}\n> +\t\toutPlane.offset = 0;\n> +\t\toutPlane.length = frameSize;\n> +\n> +\t\tstd::vector<FrameBuffer::Plane> planes{ outPlane };\n> +\t\tbuffers->emplace_back(std::make_unique<FrameBuffer>(std::move(planes)));\n> +\t}\n> +\n> +\treturn count;\n> +}\n> +\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> + * their respective frame buffer outputs\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> +{\n> +\tunsigned int mask = 0;\n> +\n> +\t/*\n> +\t * Validate the outputs as a sanity check: at least one output is\n> +\t * required, all outputs must reference a valid stream and no two\n> +\t * outputs can reference the same stream.\n> +\t */\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> +\n> +\t\tmask |= 1 << index;\n> +\t}\n> +\n> +\tprocess(input, outputs.at(0));\n> +\n> +\treturn 0;\n> +}\n> +\n> +/**\n> + * \\brief Starts the Software ISP streaming operation\n> + * \\return 0 on success, any other value indicates an error\n> + */\n> +int SoftwareIsp::start()\n> +{\n> +\tint ret = ipa_->start();\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\tispWorkerThread_.start();\n> +\treturn 0;\n> +}\n> +\n> +/**\n> + * \\brief Stops the Software ISP streaming operation\n> + */\n> +void SoftwareIsp::stop()\n> +{\n> +\tispWorkerThread_.exit();\n> +\tispWorkerThread_.wait();\n> +\n> +\tipa_->stop();\n> +}\n> +\n> +/**\n> + * \\brief Passes the input framebuffer to the ISP worker to process\n> + * \\param[in] input The input framebuffer\n> + * \\param[out] output The framebuffer to write the processed frame to\n> + */\n> +void SoftwareIsp::process(FrameBuffer *input, FrameBuffer *output)\n> +{\n> +\tdebayer_->invokeMethod(&DebayerCpu::process,\n> +\t\t\t       ConnectionTypeQueued, input, output, debayerParams_);\n> +}\n> +\n> +void SoftwareIsp::saveIspParams([[maybe_unused]] int dummy)\n> +{\n> +\tdebayerParams_ = *sharedParams_;\n> +}\n> +\n> +void SoftwareIsp::setSensorCtrls(const ControlList &sensorControls)\n> +{\n> +\tsetSensorControls.emit(sensorControls);\n> +}\n> +\n> +void SoftwareIsp::statsReady(int dummy)\n> +{\n> +\tispStatsReady.emit(dummy);\n> +}\n> +\n> +void SoftwareIsp::inputReady(FrameBuffer *input)\n> +{\n> +\tinputBufferReady.emit(input);\n> +}\n> +\n> +void SoftwareIsp::outputReady(FrameBuffer *output)\n> +{\n> +\toutputBufferReady.emit(output);\n> +}\n> +\n> +} /* namespace libcamera */","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 CFD37BD160\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed, 27 Mar 2024 17:27:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BCC8663331;\n\tWed, 27 Mar 2024 18:27:17 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2496261C35\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 27 Mar 2024 18:27:16 +0100 (CET)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 257431571;\n\tWed, 27 Mar 2024 18:26:43 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"S2gHun1k\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1711560403;\n\tbh=db5Ltmq9FQqlHv41v+uoI/+Q4HtiLb+tJEdAN0h/Zr4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=S2gHun1kGzj8vS36aGMePJZw7lM1TtZsdY8Phz3+MUgQozBC+SiRYzAlI/Lr5T4nv\n\tC6lfDhd841GGzRgQIjJJevU2qLeTZQZKcdBFhDskNscAMJGpIuRbXgcXv6twTS1f/Y\n\tYNf1lZVygCxvE+LauD0M3GCbk3n+m4a1psEmLZbE=","Date":"Wed, 27 Mar 2024 19:27:07 +0200","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Milan Zamazal <mzamazal@redhat.com>","Cc":"libcamera-devel@lists.libcamera.org,\n\tAndrey Konovalov <andrey.konovalov.ynk@gmail.com>,\n\tBryan O'Donoghue <bryan.odonoghue@linaro.org>,\n\tMaxime Ripard <mripard@redhat.com>, Pavel Machek <pavel@ucw.cz>,\n\tHans de Goede <hdegoede@redhat.com>,\n\tKieran Bingham <kieran.bingham@ideasonboard.com>,\n\tDennis Bonke <admin@dennisbonke.com>","Subject":"Re: [PATCH v6 10/18] libcamera: introduce SoftwareIsp","Message-ID":"<20240327172707.GH4721@pendragon.ideasonboard.com>","References":"<20240319123622.675599-1-mzamazal@redhat.com>\n\t<20240319123622.675599-11-mzamazal@redhat.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240319123622.675599-11-mzamazal@redhat.com>","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>"}},{"id":29135,"web_url":"https://patchwork.libcamera.org/comment/29135/","msgid":"<87v84zh7yx.fsf@redhat.com>","date":"2024-04-02T19:37:10","subject":"Re: [PATCH v6 10/18] libcamera: introduce SoftwareIsp","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes:\n\n> On Tue, Mar 19, 2024 at 01:35:57PM +0100, Milan Zamazal wrote:\n>> From: Andrey Konovalov <andrey.konovalov@linaro.org>\n\n[...]\n\n>> + * \\var SoftwareIsp::setSensorControls\n>\n> To make components reusable, signals should be named based on the event\n> they report, not based on what the connected slot is expected to do.\n\nThe current naming is consistent with similar libcamera code in other places.\nIf we want to change it, it should be done at once everywhere.\n\n[...]\n\n>> + debayerParams_{ DebayerParams::kGain10, DebayerParams::kGain10, DebayerParams::kGain10, 0.5f },\n>> +\t  dmaHeap_(DmaHeap::DmaHeapFlag::Cma | DmaHeap::DmaHeapFlag::System)\n>> +{\n>> +\tif (!dmaHeap_.isValid()) {\n>> +\t\tLOG(SoftwareIsp, Error) << \"Failed to create DmaHeap object\";\n>> +\t\treturn;\n>> +\t}\n>> +\n>> +\tsharedParams_ = SharedMemObject<DebayerParams>(\"softIsp_params\");\n>> +\tif (!sharedParams_) {\n>> +\t\tLOG(SoftwareIsp, Error) << \"Failed to create shared memory for parameters\";\n>> +\t\treturn;\n>> +\t}\n>> +\n>> +\tauto stats = std::make_unique<SwStatsCpu>();\n>> +\tif (!stats->isValid()) {\n>> +\t\tLOG(SoftwareIsp, Error) << \"Failed to create SwStatsCpu object\";\n>> +\t\treturn;\n>> +\t}\n>> +\tstats->statsReady.connect(this, &SoftwareIsp::statsReady);\n>> +\n>> +\tdebayer_ = std::make_unique<DebayerCpu>(std::move(stats));\n>> +\tdebayer_->inputBufferReady.connect(this, &SoftwareIsp::inputReady);\n>> +\tdebayer_->outputBufferReady.connect(this, &SoftwareIsp::outputReady);\n>> +\n>> +\tipa_ = IPAManager::createIPA<ipa::soft::IPAProxySoft>(pipe, 0, 0);\n>> +\tif (!ipa_) {\n>> +\t\tLOG(SoftwareIsp, Error)\n>> +\t\t\t<< \"Creating IPA for software ISP failed\";\n>> +\t\tdebayer_.reset();\n>\n> Is this needed, or can we let the destructor handle it ?\n\nI think it is needed, debayer_ should be set to a consistent, i.e. default,\nstate before leaving the constructor.  It doesn't know when the destructor will\nbe called.\n\n[...]\n\n>> +/**\n>> + * \\brief Configure the SoftwareIsp object according to the passed in parameters\n>> + * \\param[in] inputCfg The input configuration\n>> + * \\param[in] outputCfgs The output configurations\n>> + * \\param[in] sensorControls ControlInfoMap of the controls supported by the sensor\n>> + * \\return 0 on success, a negative errno on failure\n>> + */\n>> +int SoftwareIsp::configure(const StreamConfiguration &inputCfg,\n>> +\t\t\t   const std::vector<std::reference_wrapper<StreamConfiguration>> &outputCfgs,\n>> +\t\t\t   const ControlInfoMap &sensorControls)\n>> +{\n>> +\tASSERT(ipa_ != nullptr && debayer_ != nullptr);\n>\n> Is there a specific reason to check ipa_ here ?\n\nI don't know the answer but I'd say it doesn't harm and better to catch this\nhere than letting it simply crash:\n\n>> +\n>> +\tint ret = ipa_->configure(sensorControls);","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 22F29BD16B\n\tfor <parsemail@patchwork.libcamera.org>;\n\tTue,  2 Apr 2024 19:37:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3D35163363;\n\tTue,  2 Apr 2024 21:37:22 +0200 (CEST)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.129.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 57B5F63334\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue,  2 Apr 2024 21:37:20 +0200 (CEST)","from mail-ed1-f69.google.com (mail-ed1-f69.google.com\n\t[209.85.208.69]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-111-DjgACHyUN_an8n696MBhvg-1; Tue, 02 Apr 2024 15:37:12 -0400","by mail-ed1-f69.google.com with SMTP id\n\t4fb4d7f45d1cf-5681b29771fso4907923a12.1\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 02 Apr 2024 12:37:12 -0700 (PDT)","from nuthatch (ip-77-48-47-2.net.vodafone.cz. [77.48.47.2])\n\tby smtp.gmail.com with ESMTPSA id\n\tc15-20020a1709060fcf00b00a4e4c944e77sm4460714ejk.40.2024.04.02.12.37.10\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tTue, 02 Apr 2024 12:37:10 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"Lhtenmep\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1712086639;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=ODzFVVWVvvr2J1jFrOooo3uQmxS54CMrzasvxQj/jws=;\n\tb=LhtenmepNMUDtrhoKmUftR+FrFdOSwp3y1w6p00iuj+vj3gquceKj0K+vq9K0K3pLyUQWB\n\tzkjQBfwMoA4usbo1m620MlP/NCR9bQ1KhpTTYQITA1QOiilB5aa3r6Xb3E5hkW7b3TqiPy\n\tfLQlpx68lGHPyAO/uMOAQ+mBeVC9hIU=","X-MC-Unique":"DjgACHyUN_an8n696MBhvg-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1712086632; x=1712691432;\n\th=mime-version:user-agent:message-id:date:references:in-reply-to\n\t:subject:cc:to:from:x-gm-message-state:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=ODzFVVWVvvr2J1jFrOooo3uQmxS54CMrzasvxQj/jws=;\n\tb=EUMd9DrKWUL+C6n63PiEZm9zEnfAIRcl9/pynfaO0ZSK9Et2gdjkK9BAAWNNWwV7Os\n\tqawXAKnqPKU9VvEHk0sj7LGCSyT59gx7kRkOXnYb9TU5zGs0fvNwyA238kITwo8CEZm0\n\tz3iDNr3mieJx8fJI1UJB6iJSD8oh/UphIaf/ozke3CJ07QIUULnFfPj0MR3hNOUFbU1J\n\t1yKXjylnSitM3c70AEg/KNZl180eUYoQQgUUfBoUamyPyzG3yvOR82HDgIjAoyfdtWwZ\n\tkxXG6Zh6ake3NrAIzgRf/dStQ4JKMXtGbBV98R3TJQPB76XY5ukDZJm0hDp+2qH+kGCn\n\t48VA==","X-Gm-Message-State":"AOJu0YyGCOCfuQB/TrF0+12WhjFZHKFOZ6oG7FjITpXj3SjvCwMPIKqx\n\t5E3L1vyy87k1WAECafP7tYvlfWM2gRA2uywODZ2m9z+gc4grv5opeSupTy0xCUVPDcaHgqhu/wO\n\tghXnbIwttvyyqSJyAb8e9vE9xFBoBKElgd27qsNWEEQuEnM6vYN9J1AzxaFlm9mBG0lNeK8c=","X-Received":["by 2002:a17:906:5794:b0:a4e:7b8e:35ae with SMTP id\n\tk20-20020a170906579400b00a4e7b8e35aemr3720111ejq.38.1712086631904; \n\tTue, 02 Apr 2024 12:37:11 -0700 (PDT)","by 2002:a17:906:5794:b0:a4e:7b8e:35ae with SMTP id\n\tk20-20020a170906579400b00a4e7b8e35aemr3720097ejq.38.1712086631504; \n\tTue, 02 Apr 2024 12:37:11 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IHtWluve7jmYVEiicKgBj5h+I7ox8xYEvxiSHDwsBwkbtWS9DR5UKqyYvbQs0d07GL9twO9dg==","From":"Milan Zamazal <mzamazal@redhat.com>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org,  Andrey Konovalov\n\t<andrey.konovalov.ynk@gmail.com>,  Bryan O'Donoghue\n\t<bryan.odonoghue@linaro.org>, Maxime Ripard <mripard@redhat.com>, Pavel\n\tMachek <pavel@ucw.cz>, Hans de Goede <hdegoede@redhat.com>, Kieran\n\tBingham <kieran.bingham@ideasonboard.com>,  Dennis Bonke\n\t<admin@dennisbonke.com>","Subject":"Re: [PATCH v6 10/18] libcamera: introduce SoftwareIsp","In-Reply-To":"<20240327172707.GH4721@pendragon.ideasonboard.com> (Laurent\n\tPinchart's message of \"Wed, 27 Mar 2024 19:27:07 +0200\")","References":"<20240319123622.675599-1-mzamazal@redhat.com>\n\t<20240319123622.675599-11-mzamazal@redhat.com>\n\t<20240327172707.GH4721@pendragon.ideasonboard.com>","Date":"Tue, 02 Apr 2024 21:37:10 +0200","Message-ID":"<87v84zh7yx.fsf@redhat.com>","User-Agent":"Gnus/5.13 (Gnus v5.13)","MIME-Version":"1.0","X-Mimecast-Spam-Score":"0","X-Mimecast-Originator":"redhat.com","Content-Type":"text/plain","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>"}},{"id":29142,"web_url":"https://patchwork.libcamera.org/comment/29142/","msgid":"<20240402210916.GD20073@pendragon.ideasonboard.com>","date":"2024-04-02T21:09:16","subject":"Re: [PATCH v6 10/18] libcamera: introduce SoftwareIsp","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Tue, Apr 02, 2024 at 09:37:10PM +0200, Milan Zamazal wrote:\n> Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes:\n> \n> > On Tue, Mar 19, 2024 at 01:35:57PM +0100, Milan Zamazal wrote:\n> >> From: Andrey Konovalov <andrey.konovalov@linaro.org>\n> \n> [...]\n> \n> >> + * \\var SoftwareIsp::setSensorControls\n> >\n> > To make components reusable, signals should be named based on the event\n> > they report, not based on what the connected slot is expected to do.\n> \n> The current naming is consistent with similar libcamera code in other places.\n> If we want to change it, it should be done at once everywhere.\n\nIndeed, I thought we had fixed that already.\n\n> [...]\n> \n> >> + debayerParams_{ DebayerParams::kGain10, DebayerParams::kGain10, DebayerParams::kGain10, 0.5f },\n> >> +\t  dmaHeap_(DmaHeap::DmaHeapFlag::Cma | DmaHeap::DmaHeapFlag::System)\n> >> +{\n> >> +\tif (!dmaHeap_.isValid()) {\n> >> +\t\tLOG(SoftwareIsp, Error) << \"Failed to create DmaHeap object\";\n> >> +\t\treturn;\n> >> +\t}\n> >> +\n> >> +\tsharedParams_ = SharedMemObject<DebayerParams>(\"softIsp_params\");\n> >> +\tif (!sharedParams_) {\n> >> +\t\tLOG(SoftwareIsp, Error) << \"Failed to create shared memory for parameters\";\n> >> +\t\treturn;\n> >> +\t}\n> >> +\n> >> +\tauto stats = std::make_unique<SwStatsCpu>();\n> >> +\tif (!stats->isValid()) {\n> >> +\t\tLOG(SoftwareIsp, Error) << \"Failed to create SwStatsCpu object\";\n> >> +\t\treturn;\n> >> +\t}\n> >> +\tstats->statsReady.connect(this, &SoftwareIsp::statsReady);\n> >> +\n> >> +\tdebayer_ = std::make_unique<DebayerCpu>(std::move(stats));\n> >> +\tdebayer_->inputBufferReady.connect(this, &SoftwareIsp::inputReady);\n> >> +\tdebayer_->outputBufferReady.connect(this, &SoftwareIsp::outputReady);\n> >> +\n> >> +\tipa_ = IPAManager::createIPA<ipa::soft::IPAProxySoft>(pipe, 0, 0);\n> >> +\tif (!ipa_) {\n> >> +\t\tLOG(SoftwareIsp, Error)\n> >> +\t\t\t<< \"Creating IPA for software ISP failed\";\n> >> +\t\tdebayer_.reset();\n> >\n> > Is this needed, or can we let the destructor handle it ?\n> \n> I think it is needed, debayer_ should be set to a consistent, i.e. default,\n> state before leaving the constructor.  It doesn't know when the destructor will\n> be called.\n\nI don't mind keeping it.\n\n> [...]\n> \n> >> +/**\n> >> + * \\brief Configure the SoftwareIsp object according to the passed in parameters\n> >> + * \\param[in] inputCfg The input configuration\n> >> + * \\param[in] outputCfgs The output configurations\n> >> + * \\param[in] sensorControls ControlInfoMap of the controls supported by the sensor\n> >> + * \\return 0 on success, a negative errno on failure\n> >> + */\n> >> +int SoftwareIsp::configure(const StreamConfiguration &inputCfg,\n> >> +\t\t\t   const std::vector<std::reference_wrapper<StreamConfiguration>> &outputCfgs,\n> >> +\t\t\t   const ControlInfoMap &sensorControls)\n> >> +{\n> >> +\tASSERT(ipa_ != nullptr && debayer_ != nullptr);\n> >\n> > Is there a specific reason to check ipa_ here ?\n> \n> I don't know the answer but I'd say it doesn't harm and better to catch this\n> here than letting it simply crash:\n\nIf debayer_ is not null, I don't see how ipa_ could be null, so I think\nwe can drop that part of the check.\n\n> >> +\n> >> +\tint ret = ipa_->configure(sensorControls);","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 2B08EBD16B\n\tfor <parsemail@patchwork.libcamera.org>;\n\tTue,  2 Apr 2024 21:09:29 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5816E63334;\n\tTue,  2 Apr 2024 23:09: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 A88BB6308D\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue,  2 Apr 2024 23:09:27 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5400C564;\n\tTue,  2 Apr 2024 23:08: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=\"Zzt/grqf\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1712092130;\n\tbh=KpFojbfZQ7hOb09YWC9Csfy8g2YnqopSjIgTlQLNoJ4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Zzt/grqfVcN2Tuxz8dnQP3Uj7GxxO+TpmhzflMhc27I2yXpd48wZJFdd48MxHBrrB\n\tBtN70DOL3WxDUDX8m1D2+Rs/886SjTRxZQKd7hytGBypANh9s2pzqgdfS+i7LMNcG+\n\tb7P+iUoNG2JgHjPqxYn6Q0h11MsGMUTSbfzu1A9Y=","Date":"Wed, 3 Apr 2024 00:09:16 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Milan Zamazal <mzamazal@redhat.com>","Cc":"libcamera-devel@lists.libcamera.org,\n\tAndrey Konovalov <andrey.konovalov.ynk@gmail.com>,\n\tBryan O'Donoghue <bryan.odonoghue@linaro.org>,\n\tMaxime Ripard <mripard@redhat.com>, Pavel Machek <pavel@ucw.cz>,\n\tHans de Goede <hdegoede@redhat.com>,\n\tKieran Bingham <kieran.bingham@ideasonboard.com>,\n\tDennis Bonke <admin@dennisbonke.com>","Subject":"Re: [PATCH v6 10/18] libcamera: introduce SoftwareIsp","Message-ID":"<20240402210916.GD20073@pendragon.ideasonboard.com>","References":"<20240319123622.675599-1-mzamazal@redhat.com>\n\t<20240319123622.675599-11-mzamazal@redhat.com>\n\t<20240327172707.GH4721@pendragon.ideasonboard.com>\n\t<87v84zh7yx.fsf@redhat.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<87v84zh7yx.fsf@redhat.com>","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>"}},{"id":29146,"web_url":"https://patchwork.libcamera.org/comment/29146/","msgid":"<87ttkieqab.fsf@redhat.com>","date":"2024-04-03T09:42:04","subject":"Re: [PATCH v6 10/18] libcamera: introduce SoftwareIsp","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes:\n\n> On Tue, Apr 02, 2024 at 09:37:10PM +0200, Milan Zamazal wrote:\n>> Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes:\n>> \n>\n>> > On Tue, Mar 19, 2024 at 01:35:57PM +0100, Milan Zamazal wrote:\n>> >> From: Andrey Konovalov <andrey.konovalov@linaro.org>\n\n[...]\n\n>> >> +\tipa_ = IPAManager::createIPA<ipa::soft::IPAProxySoft>(pipe, 0, 0);\n>> >> +\tif (!ipa_) {\n>> >> +\t\tLOG(SoftwareIsp, Error)\n>> >> +\t\t\t<< \"Creating IPA for software ISP failed\";\n>> >> +\t\tdebayer_.reset();\n>> >\n>> > Is this needed, or can we let the destructor handle it ?\n>> \n>> I think it is needed, debayer_ should be set to a consistent, i.e. default,\n>> state before leaving the constructor.  It doesn't know when the destructor will\n>> be called.\n>\n> I don't mind keeping it.\n>\n>> [...]\n>> \n>> >> +/**\n>> >> + * \\brief Configure the SoftwareIsp object according to the passed in parameters\n>> >> + * \\param[in] inputCfg The input configuration\n>> >> + * \\param[in] outputCfgs The output configurations\n>> >> + * \\param[in] sensorControls ControlInfoMap of the controls supported by the sensor\n>> >> + * \\return 0 on success, a negative errno on failure\n>> >> + */\n>> >> +int SoftwareIsp::configure(const StreamConfiguration &inputCfg,\n>> >> +\t\t\t   const std::vector<std::reference_wrapper<StreamConfiguration>> &outputCfgs,\n>> >> +\t\t\t   const ControlInfoMap &sensorControls)\n>> >> +{\n>> >> +\tASSERT(ipa_ != nullptr && debayer_ != nullptr);\n>> >\n>> > Is there a specific reason to check ipa_ here ?\n>> \n>> I don't know the answer but I'd say it doesn't harm and better to catch this\n>> here than letting it simply crash:\n>\n> If debayer_ is not null, I don't see how ipa_ could be null, \n\nIn the current code yes.  But I cannot see this documented anywhere and it's not\nsomething that SoftwareIsp::configure can derive itself, so it's only an\nimplicit assumption based on current implementation.  Such assumptions are\nnotoriously dangerous and if the implementation gets changed e.g. by dropping\ndebayer_.reset() calls or other early quit in the constructor then debayer_ can\nbe non-null and ipa_ null.  And it's easy to forget about updating the assertion\nin configure().\n\n> so I think we can drop that part of the check.\n\nSo I think we should keep it. :-)\n\n>> >> +\n>> >> +\tint ret = ipa_->configure(sensorControls);","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 7386DBD16B\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed,  3 Apr 2024 09:42:13 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A239A6336D;\n\tWed,  3 Apr 2024 11:42:12 +0200 (CEST)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.129.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 93ABB6332E\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed,  3 Apr 2024 11:42:10 +0200 (CEST)","from mail-ej1-f70.google.com (mail-ej1-f70.google.com\n\t[209.85.218.70]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-185-LxI3Uh35NtKfYyda0fCQJA-1; Wed, 03 Apr 2024 05:42:07 -0400","by mail-ej1-f70.google.com with SMTP id\n\ta640c23a62f3a-a466c7b0587so418749466b.0\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 03 Apr 2024 02:42:07 -0700 (PDT)","from nuthatch (ip-77-48-47-2.net.vodafone.cz. [77.48.47.2])\n\tby smtp.gmail.com with ESMTPSA id\n\tbk24-20020a170906b0d800b00a46a71c5324sm7571433ejb.34.2024.04.03.02.42.05\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tWed, 03 Apr 2024 02:42:05 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"E1bfwWPT\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1712137329;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=Uw1U5OiJJz87/tRWlqfdLpkHpfKONXvjLMpKIJtc49E=;\n\tb=E1bfwWPTIA6dBDoWejEq5wopS9jKRMlj80+5wdrrrcCkknf0H74ymFVnzXETgsxxv9PAhJ\n\tR+hmByYebhASVsgpEPgZVxTEtq8s/MOAYG+7fThFrrqaidIfWfc3S3uz9llWtqCEXZgV7v\n\trVVnCoL67uIZW/Kv0XD2xXBdvKf8TNo=","X-MC-Unique":"LxI3Uh35NtKfYyda0fCQJA-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1712137326; x=1712742126;\n\th=mime-version:user-agent:message-id:date:references:in-reply-to\n\t:subject:cc:to:from:x-gm-message-state:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=Uw1U5OiJJz87/tRWlqfdLpkHpfKONXvjLMpKIJtc49E=;\n\tb=tnL49NwcEyUS9+ayHI3zihVY2mx4EjeArlAxW1bCTLluuvcclQYNzqiWTCsLQCkQJs\n\tBgJvJrNbxIv+peM5TtwCUx8cP5RqZ7ipQQxZ8m7YZDqU5TbD2NgIF0RZ9hp24MmY6w3U\n\t4soTZ6jB/O7V4F5xSkiLzyJDJ9cDlwhjsL/Qtct+GYMW5sdEIDxvUf1dLQLsHMGS3DCl\n\trZ29FVLZ2htnF/aYxd/PMGsZkA5+M1yXYuByTcr7ZEiwu0RZYM3XLB5g1+UPJQGQqxvO\n\tGIK2oJqumi+ZyhAO7VfivtuUvNSQvV22eNCYquwg35mzs0nc1c+lXcZcnnZej6F+765v\n\tNMpg==","X-Gm-Message-State":"AOJu0YwlX9p7Oy1KkET1Uxqs4mQPrYvBj9VxPuleYLgqZiY6iXjgOqya\n\tdErRZsfVUCUTlTVi0f2wH3FqIu9b06m2KefeJFe0UPSwundl2Sfrpu+bZ/x03bGv2Vg5U+cxx8o\n\taAZ45AqRz/Y4jVfh1haOP+YtCLLmIaIRXFMr+z987h/vdTv3wq0JSVTdkomxHTB05/QDUQD8=","X-Received":["by 2002:a17:906:1354:b0:a4e:38c0:8730 with SMTP id\n\tx20-20020a170906135400b00a4e38c08730mr1271205ejb.77.1712137326579; \n\tWed, 03 Apr 2024 02:42:06 -0700 (PDT)","by 2002:a17:906:1354:b0:a4e:38c0:8730 with SMTP id\n\tx20-20020a170906135400b00a4e38c08730mr1271196ejb.77.1712137326206; \n\tWed, 03 Apr 2024 02:42:06 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IG7i8hl+1GNo4OUiGI8133zl8bSlpy+PBdA214dus3AkoTL/W+3LL91qN3aaYgHLbCC6+CqeA==","From":"Milan Zamazal <mzamazal@redhat.com>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org,  Andrey Konovalov\n\t<andrey.konovalov.ynk@gmail.com>,  Bryan O'Donoghue\n\t<bryan.odonoghue@linaro.org>, Maxime Ripard <mripard@redhat.com>, Pavel\n\tMachek <pavel@ucw.cz>, Hans de Goede <hdegoede@redhat.com>, Kieran\n\tBingham <kieran.bingham@ideasonboard.com>,  Dennis Bonke\n\t<admin@dennisbonke.com>","Subject":"Re: [PATCH v6 10/18] libcamera: introduce SoftwareIsp","In-Reply-To":"<20240402210916.GD20073@pendragon.ideasonboard.com> (Laurent\n\tPinchart's message of \"Wed, 3 Apr 2024 00:09:16 +0300\")","References":"<20240319123622.675599-1-mzamazal@redhat.com>\n\t<20240319123622.675599-11-mzamazal@redhat.com>\n\t<20240327172707.GH4721@pendragon.ideasonboard.com>\n\t<87v84zh7yx.fsf@redhat.com>\n\t<20240402210916.GD20073@pendragon.ideasonboard.com>","Date":"Wed, 03 Apr 2024 11:42:04 +0200","Message-ID":"<87ttkieqab.fsf@redhat.com>","User-Agent":"Gnus/5.13 (Gnus v5.13)","MIME-Version":"1.0","X-Mimecast-Spam-Score":"0","X-Mimecast-Originator":"redhat.com","Content-Type":"text/plain","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>"}}]