[{"id":32720,"web_url":"https://patchwork.libcamera.org/comment/32720/","msgid":"","date":"2024-12-13T11:24:32","subject":"Re: [PATCH v3.1] fixup! libcamera: converter_v4l2_m2m: Improve crop\n\tbounds support","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Stefan\n\nOn Fri, Dec 13, 2024 at 10:26:05AM +0100, Stefan Klug wrote:\n> Changes semantics of inputCropBounds to return null bounds if an\n> unconfigured stream is provided. Return the device crop bounds if a\n> nullptr is provided.\n>\n> Signed-off-by: Stefan Klug \n> ---\n>\n> Hi Jacopo,\n>\n> I forgot to handle your comment in the v3. Sorry for that. This is my\n> proposal. It is somewhere in the middle of your suggestions, but I think\n> it makes the intent clearer.\n\nI like isConfigured() as it allows the pipeline to behave accordingly\nto the converter configuration.\n\nI like less the three-way behaviour of\nConverter::inputCropBounds(Stream *)\n\n1) If called with a configured Stream it returns the input crop bounds\nof that stream\n2) If called with a nullptr it returns the default crop bounds\n3) When called with a Stream it returns an empty Rectangle\n\nTo make it less confusing, instead of\n\n\tvirtual std::pair inputCropBounds(\n\t\t\t\tconst Stream *stream = nullptr) = 0;\n\nI would\n\n\tvirtual std::pair inputCropBounds() = 0;\n\tvirtual std::pair inputCropBounds(const Stream *stream) = 0;\n\nor maybe even just\n\tvirtual std::pair cropBounds() = 0;\n\nSo that pipeline handlers could\n\n\n\t\tstd::pair cropLimits;\n\t\tif (dewarper_->isConfigured(&data->mainPathStream_))\n cropLimits = dewarper_->inputCropBounds(&data->mainPathStream_);\n else\n cropLimits = dewarper_->cropBounds();\n\nWhich I find clearer as an API than\n\n\t\tconst Stream *stream = nullptr;\n\t\tif (dewarper_->isConfigured(&data->mainPathStream_))\n\t\t\tstream = &data->mainPathStream_;\n\n\t\tstd::pair cropLimits =\n\t\t\tdewarper_->inputCropBounds(stream);\n\nUp to you\n\n>\n> I would like to keep the getCropBound() a static function as we\n> discussed before.\n>\n> The commit message will also be updated for v4.\n>\n> Is this change ok for you?\n>\n> Best regards,\n> Stefan\n>\n> include/libcamera/internal/converter.h | 4 +++-\n> .../internal/converter/converter_v4l2_m2m.h | 1 +\n> src/libcamera/converter.cpp | 12 +++++++++++-\n> .../converter/converter_v4l2_m2m.cpp | 19 ++++++++++++++++---\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 6 +++++-\n> 5 files changed, 36 insertions(+), 6 deletions(-)\n>\n> diff --git a/include/libcamera/internal/converter.h b/include/libcamera/internal/converter.h\n> index 9213ae5b9c33..465b0b0596d9 100644\n> --- a/include/libcamera/internal/converter.h\n> +++ b/include/libcamera/internal/converter.h\n> @@ -73,6 +73,7 @@ public:\n>\n> \tvirtual int configure(const StreamConfiguration &inputCfg,\n> \t\t\t const std::vector> &outputCfgs) = 0;\n> +\tvirtual bool isConfigured(const Stream *stream) const = 0;\n> \tvirtual int exportBuffers(const Stream *stream, unsigned int count,\n> \t\t\t\t std::vector> *buffers) = 0;\n>\n> @@ -83,7 +84,8 @@ public:\n> \t\t\t\t const std::map &outputs) = 0;\n>\n> \tvirtual int setInputCrop(const Stream *stream, Rectangle *rect) = 0;\n> -\tvirtual std::pair inputCropBounds(const Stream *stream) = 0;\n> +\tvirtual std::pair inputCropBounds(\n> +\t\t\t\tconst Stream *stream = nullptr) = 0;\n>\n> \tSignal inputBufferReady;\n> \tSignal outputBufferReady;\n> diff --git a/include/libcamera/internal/converter/converter_v4l2_m2m.h b/include/libcamera/internal/converter/converter_v4l2_m2m.h\n> index 89bd2878b190..175f0e1109a6 100644\n> --- a/include/libcamera/internal/converter/converter_v4l2_m2m.h\n> +++ b/include/libcamera/internal/converter/converter_v4l2_m2m.h\n> @@ -54,6 +54,7 @@ public:\n>\n> \tint configure(const StreamConfiguration &inputCfg,\n> \t\t const std::vector> &outputCfg);\n> +\tbool isConfigured(const Stream *stream) const override;\n> \tint exportBuffers(const Stream *stream, unsigned int count,\n> \t\t\t std::vector> *buffers);\n>\n> diff --git a/src/libcamera/converter.cpp b/src/libcamera/converter.cpp\n> index c3da162b7de7..74cea46cc709 100644\n> --- a/src/libcamera/converter.cpp\n> +++ b/src/libcamera/converter.cpp\n> @@ -169,6 +169,13 @@ Converter::~Converter()\n> * \\return 0 on success or a negative error code otherwise\n> */\n>\n> +/**\n> + * \\fn Converter::isConfigured()\n> + * \\brief Check if a given stream is configured\n> + * \\param[in] stream The output stream\n> + * \\return True if the \\a stream is configured or false otherwise\n> + */\n> +\n> /**\n> * \\fn Converter::exportBuffers()\n> * \\brief Export buffers from the converter device\n> @@ -238,9 +245,12 @@ Converter::~Converter()\n> * this function should be called after the \\a stream has been configured using\n> * configure().\n> *\n> - * When called with an invalid \\a stream, the function returns the default crop\n> + * When called with nullptr \\a stream this function returns the default crop\n> * bounds of the converter.\n> *\n> + * When called with an invalid \\a stream, this function returns a pair of null\n> + * rectangles\n> + *\n> * \\return A pair containing the minimum and maximum crop bound in that order\n> */\n>\n> diff --git a/src/libcamera/converter/converter_v4l2_m2m.cpp b/src/libcamera/converter/converter_v4l2_m2m.cpp\n> index 6857472b29f2..4df253749feb 100644\n> --- a/src/libcamera/converter/converter_v4l2_m2m.cpp\n> +++ b/src/libcamera/converter/converter_v4l2_m2m.cpp\n> @@ -559,6 +559,14 @@ int V4L2M2MConverter::configure(const StreamConfiguration &inputCfg,\n> \treturn 0;\n> }\n>\n> +/**\n> + * \\copydoc libcamera::Converter::isConfigured\n> + */\n> +bool V4L2M2MConverter::isConfigured(const Stream *stream) const\n> +{\n> +\treturn streams_.find(stream) != streams_.end();\n> +}\n> +\n> /**\n> * \\copydoc libcamera::Converter::exportBuffers\n> */\n> @@ -595,11 +603,16 @@ int V4L2M2MConverter::setInputCrop(const Stream *stream, Rectangle *rect)\n> std::pair\n> V4L2M2MConverter::inputCropBounds(const Stream *stream)\n> {\n> +\tif (stream == nullptr)\n> +\t\treturn inputCropBounds_;\n> +\n> \tauto iter = streams_.find(stream);\n> -\tif (iter != streams_.end())\n> -\t\treturn iter->second->inputCropBounds();\n> +\tif (iter == streams_.end()) {\n> +\t\tLOG(Converter, Error) << \"Invalid output stream\";\n> +\t\treturn {};\n> +\t}\n>\n> -\treturn inputCropBounds_;\n> +\treturn iter->second->inputCropBounds();\n> }\n>\n> /**\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index 4dcc5a4fa6a1..ad162164df1a 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -1281,8 +1281,12 @@ int PipelineHandlerRkISP1::updateControls(RkISP1CameraData *data)\n> \tControlInfoMap::Map controls;\n>\n> \tif (dewarper_) {\n> +\t\tconst Stream *stream = nullptr;\n> +\t\tif (dewarper_->isConfigured(&data->mainPathStream_))\n> +\t\t\tstream = &data->mainPathStream_;\n> +\n> \t\tstd::pair cropLimits =\n> -\t\t\tdewarper_->inputCropBounds(&data->mainPathStream_);\n> +\t\t\tdewarper_->inputCropBounds(stream);\n>\n> \t\t/*\n> \t\t * ScalerCrop is specified to be in Sensor coordinates.\n> --\n> 2.43.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 15AB5C32F0\n\tfor ;\n\tFri, 13 Dec 2024 11:24:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4633567EEF;\n\tFri, 13 Dec 2024 12:24:37 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C6DC166136\n\tfor ;\n\tFri, 13 Dec 2024 12:24:35 +0100 (CET)","from ideasonboard.com (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 430AF9CE;\n\tFri, 13 Dec 2024 12:24:01 +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=\"oiCNiEyK\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1734089041;\n\tbh=ayxYo9iRVzUYBVwucYVUZbock67BIPU36+iSze1rxFI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=oiCNiEyKRtAaoFIwYN9KKiO5/PW4sf99yLNRh7MW+mFZH5zidkbbBVqhQf9ajmorl\n\trlISDhulRbkmiWuWRf/lwQ7Xj+tUNIKFV5xmRFIhSWJzDuOUFAoDLPGJJOnTdpemTZ\n\tSIc5iwkGMp9Di894OEak3t4kJhniuMu2TlHMoHaA=","Date":"Fri, 13 Dec 2024 12:24:32 +0100","From":"Jacopo Mondi ","To":"Stefan Klug ","Cc":"libcamera-devel@lists.libcamera.org, \n\tJacopo Mondi ","Subject":"Re: [PATCH v3.1] fixup! libcamera: converter_v4l2_m2m: Improve crop\n\tbounds support","Message-ID":"","References":"\n\t<20241213094551.74584-3-stefan.klug@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20241213094551.74584-3-stefan.klug@ideasonboard.com>","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\" "}}]