[{"id":30416,"web_url":"https://patchwork.libcamera.org/comment/30416/","msgid":"","date":"2024-07-17T10:02:51","subject":"Re: [PATCH v5 2/5] libcamera: converter: Add interface to support\n\tcropping capability","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Umang\n\nOn Wed, Jul 10, 2024 at 01:51:48AM GMT, Umang Jain wrote:\n> If the converter has cropping capability, the interface should support it\n> by providing appropriate virtual functions. Provide Feature::Crop in\n> Feature enumeration for the same.\n>\n> Provide virtual setCrop() and getCropBounds() interfaces so that\n> the converter can implement their own cropping functionality.\n>\n> The V4L2M2MConverter implements these interfaces of the Converter\n> interface. Not all V4L2M2M converters will have cropping capability,\n> hence Feature::Crop should be set while construction for all those\n> converters.\n>\n> Signed-off-by: Umang Jain \n> ---\n> include/libcamera/internal/converter.h | 5 ++\n> .../internal/converter/converter_v4l2_m2m.h | 6 ++\n> src/libcamera/converter.cpp | 51 +++++++++++\n> .../converter/converter_v4l2_m2m.cpp | 88 +++++++++++++++++++\n> 4 files changed, 150 insertions(+)\n>\n> diff --git a/include/libcamera/internal/converter.h b/include/libcamera/internal/converter.h\n> index 7e478356..9b1223fd 100644\n> --- a/include/libcamera/internal/converter.h\n> +++ b/include/libcamera/internal/converter.h\n> @@ -14,6 +14,7 @@\n> #include \n> #include \n> #include \n> +#include \n> #include \n>\n> #include \n> @@ -35,6 +36,7 @@ class Converter\n> public:\n> \tenum class Feature {\n> \t\tNone = 0,\n> +\t\tCrop = (1 << 0),\n> \t};\n>\n> \tusing Features = Flags;\n> @@ -63,6 +65,9 @@ public:\n> \tvirtual int queueBuffers(FrameBuffer *input,\n> \t\t\t\t const std::map &outputs) = 0;\n>\n> +\tvirtual int setCrop(const Stream *stream, Rectangle *rect);\n\n\"crop\" is loosely defined as a concept.\nA converter has an input and an output and I presume it scales.\n\nWhere is the crop rectangle applied ?\n\nDoes this need to be part of the base class interface ? Or should only\nderived classes that support it implement it ? (it's reasonable for a\nV4L2 M2M device to support this, once better specified the interface\nwhere the crop rectangle has to be applied).\n\n> +\tvirtual std::pair getCropBounds(const Stream *stream);\n\nnot \"get\" in getters\n\n> +\n> \tSignal inputBufferReady;\n> \tSignal outputBufferReady;\n>\n> diff --git a/include/libcamera/internal/converter/converter_v4l2_m2m.h b/include/libcamera/internal/converter/converter_v4l2_m2m.h\n> index 91701dbe..2697eed9 100644\n> --- a/include/libcamera/internal/converter/converter_v4l2_m2m.h\n> +++ b/include/libcamera/internal/converter/converter_v4l2_m2m.h\n> @@ -30,6 +30,7 @@ class Size;\n> class SizeRange;\n> class Stream;\n> struct StreamConfiguration;\n> +class Rectangle;\n> class V4L2M2MDevice;\n>\n> class V4L2M2MConverter : public Converter\n> @@ -57,6 +58,9 @@ public:\n> \tint queueBuffers(FrameBuffer *input,\n> \t\t\t const std::map &outputs);\n>\n> +\tint setCrop(const Stream *stream, Rectangle *rect);\n> +\tstd::pair getCropBounds(const Stream *stream);\n> +\n> private:\n> \tclass V4L2M2MStream : protected Loggable\n> \t{\n> @@ -75,6 +79,8 @@ private:\n>\n> \t\tint queueBuffers(FrameBuffer *input, FrameBuffer *output);\n>\n> +\t\tint setSelection(unsigned int target, Rectangle *rect);\n> +\n> \tprotected:\n> \t\tstd::string logPrefix() const override;\n>\n> diff --git a/src/libcamera/converter.cpp b/src/libcamera/converter.cpp\n> index 2c3da6d4..d51e77a0 100644\n> --- a/src/libcamera/converter.cpp\n> +++ b/src/libcamera/converter.cpp\n> @@ -11,6 +11,8 @@\n>\n> #include \n>\n> +#include \n> +\n> #include \"libcamera/internal/media_device.h\"\n>\n> /**\n> @@ -39,6 +41,8 @@ LOG_DEFINE_CATEGORY(Converter)\n> * \\brief Specify the features supported by the converter\n> * \\var Converter::Feature::None\n> * \\brief No extra features supported by the converter\n> + * \\var Converter::Feature::Crop\n> + * \\brief Cropping capability is supported by the converter\n> */\n>\n> /**\n> @@ -161,6 +165,53 @@ Converter::~Converter()\n> * \\return 0 on success or a negative error code otherwise\n> */\n>\n> +/**\n> + * \\brief Set the crop rectangle \\a rect for \\a stream\n> + * \\param[in] stream Pointer to output stream\n> + * \\param[inout] rect The crop rectangle to be applied\n> + *\n> + * Set the crop rectangle \\a rect for \\a stream provided the converter supports\n> + * cropping. The converter should have the Feature::Crop flag in this case.\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n> + */\n> +int Converter::setCrop([[maybe_unused]] const Stream *stream, [[maybe_unused]] Rectangle *rect)\n> +{\n> +\tif (!(getFeatures() & Feature::Crop)) {\n> +\t\tLOG(Converter, Error) << \"Converter doesn't support cropping capabilities\";\n> +\t\treturn -ENOTSUP;\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> +/**\n> + * \\brief Get the crop bounds \\a stream\n> + * \\param[in] stream Pointer to output stream\n> + *\n> + * Get the minimum and maximum crop bounds for \\a stream. The converter\n> + * should supporting cropping (Feature::Crop).\n> + *\n> + * \\return A std::pair containining minimum and maximum\n> + * crop bound respectively.\n> + */\n> +std::pair Converter::getCropBounds([[maybe_unused]] const Stream *stream)\n> +{\n> +\tconst StreamConfiguration &config = stream->configuration();\n> +\tRectangle rect;\n> +\n> +\tif (!(getFeatures() & Feature::Crop))\n> +\t\tLOG(Converter, Error) << \"Converter doesn't support cropping capabilities\";\n> +\n> +\t/*\n> +\t * This is base implementation for the Converter class, so just return\n> +\t * the stream configured size as minimum and maximum crop bounds.\n> +\t */\n> +\trect.size() = config.size;\n> +\n> +\treturn { rect, rect };\n\nI would rather not make it part of the base class interface\n\n> +}\n> +\n> /**\n> * \\var Converter::inputBufferReady\n> * \\brief A signal emitted when the input frame buffer completes\n> diff --git a/src/libcamera/converter/converter_v4l2_m2m.cpp b/src/libcamera/converter/converter_v4l2_m2m.cpp\n> index 4aeb7dd9..eaae3528 100644\n> --- a/src/libcamera/converter/converter_v4l2_m2m.cpp\n> +++ b/src/libcamera/converter/converter_v4l2_m2m.cpp\n> @@ -155,6 +155,15 @@ int V4L2M2MConverter::V4L2M2MStream::queueBuffers(FrameBuffer *input, FrameBuffe\n> \treturn 0;\n> }\n>\n> +int V4L2M2MConverter::V4L2M2MStream::setSelection(unsigned int target, Rectangle *rect)\n> +{\n> +\tint ret = m2m_->output()->setSelection(target, rect);\n> +\tif (ret < 0)\n> +\t\treturn ret;\n> +\n> +\treturn 0;\n> +}\n> +\n> std::string V4L2M2MConverter::V4L2M2MStream::logPrefix() const\n> {\n> \treturn stream_->configuration().toString();\n> @@ -375,6 +384,81 @@ int V4L2M2MConverter::exportBuffers(const Stream *stream, unsigned int count,\n> \treturn iter->second->exportBuffers(count, buffers);\n> }\n>\n> +/**\n> + * \\copydoc libcamera::Converter::setCrop\n> + */\n> +int V4L2M2MConverter::setCrop(const Stream *stream, Rectangle *rect)\n> +{\n> +\tif (!(getFeatures() & Feature::Crop))\n> +\t\treturn -ENOTSUP;\n> +\n> +\tauto iter = streams_.find(stream);\n> +\tif (iter == streams_.end())\n> +\t\treturn -EINVAL;\n> +\n> +\treturn iter->second->setSelection(V4L2_SEL_TGT_CROP, rect);\n> +}\n> +\n> +/**\n> + * \\copydoc libcamera::Converter::getCropBounds\n> + */\n> +std::pair\n> +V4L2M2MConverter::getCropBounds(const Stream *stream)\n> +{\n> +\tRectangle minCrop;\n> +\tRectangle maxCrop;\n> +\tint ret;\n> +\n> +\tminCrop.width = 1;\n> +\tminCrop.height = 1;\n> +\tmaxCrop.width = UINT_MAX;\n> +\tmaxCrop.height = UINT_MAX;\n\nYou can move all of this after the check for the feature support\n\n> +\n> +\tif (!(getFeatures() & Feature::Crop)) {\n> +\t\tLOG(Converter, Error) << \"Crop functionality is not supported\";\n> +\t\treturn {};\n> +\t}\n> +\n> +\tauto iter = streams_.find(stream);\n> +\tif (iter == streams_.end()) {\n> +\t\t/*\n> +\t\t * No streams configured, return minimum and maximum crop\n> +\t\t * bounds at initialization.\n> +\t\t */\n> +\t\tret = m2m_->output()->setSelection(V4L2_SEL_TGT_CROP, &minCrop);\n> +\t\tif (ret) {\n> +\t\t\tLOG(Converter, Error) << \"Failed to query minimum crop bound\";\n> +\t\t\treturn {};\n> +\t\t}\n> +\n> +\t\tret = m2m_->output()->setSelection(V4L2_SEL_TGT_CROP, &maxCrop);\n> +\t\tif (ret) {\n> +\t\t\tLOG(Converter, Error) << \"Failed to query maximum crop bound\";\n> +\t\t\treturn {};\n> +\t\t}\n> +\n> +\t\treturn { minCrop, maxCrop };\n> +\t}\n> +\n> +\t/*\n> +\t * If the streams are configured, return bounds from according to\n> +\t * stream configuration.\n> +\t */\n> +\tret = setCrop(stream, &minCrop);\n> +\tif (ret) {\n> +\t\tLOG(Converter, Error) << \"Failed to query minimum crop bound\";\n> +\t\treturn {};\n> +\t}\n> +\n> +\tret = setCrop(stream, &maxCrop);\n> +\tif (ret) {\n> +\t\tLOG(Converter, Error) << \"Failed to query maximum crop bound\";\n> +\t\treturn {};\n> +\t}\n> +\n> +\treturn { minCrop, maxCrop };\n> +}\n> +\n> /**\n> * \\copydoc libcamera::Converter::start\n> */\n> @@ -448,6 +532,10 @@ int V4L2M2MConverter::queueBuffers(FrameBuffer *input,\n> \treturn 0;\n> }\n>\n> +/*\n> + * \\todo: This should be extended to include Feature::Flag to denote\n> + * what each converter supports feature-wise.\n> + */\n\nThat's another option.\n\nI'll send to the list the patches I have on top of your series that\nmakes it possible to specify features at registration time for a\ncomparison.\n\n> static std::initializer_list compatibles = {\n> \t\"mtk-mdp\",\n> \t\"pxp\",\n> --\n> 2.45.2\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 B22DFC323E\n\tfor ;\n\tWed, 17 Jul 2024 10:02:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 994EA6336F;\n\tWed, 17 Jul 2024 12:02:55 +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 4D954619AB\n\tfor ;\n\tWed, 17 Jul 2024 12:02:54 +0200 (CEST)","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 5E1DA836;\n\tWed, 17 Jul 2024 12:02:16 +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=\"ErTOPp3V\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1721210536;\n\tbh=96X9o9ejb/EwlyShEHsBfNJhGbWLeG+VZH+BhrzkBNI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ErTOPp3VZi39Alm5UPqS/yNQu57/7uzeQfnMXx1lJ2bM93s4MylMpqrgX90NbcWFn\n\tkr1rudamBpgfS8POrc17Q0J8Iw4cR+/+1NYqeXRW45s5/f1olRtuVnY3xcqCsehr6K\n\tZ+n9Doe+MjAlMygMiTDM+bqsqsElgn/cc5IONlDE=","Date":"Wed, 17 Jul 2024 12:02:51 +0200","From":"Jacopo Mondi ","To":"Umang Jain ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v5 2/5] libcamera: converter: Add interface to support\n\tcropping capability","Message-ID":"","References":"<20240709202151.152289-1-umang.jain@ideasonboard.com>\n\t<20240709202151.152289-3-umang.jain@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240709202151.152289-3-umang.jain@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\" "}},{"id":30419,"web_url":"https://patchwork.libcamera.org/comment/30419/","msgid":"<92cdcc85-6897-40c7-a63c-d4c426e3e18e@ideasonboard.com>","date":"2024-07-17T11:04:34","subject":"Re: [PATCH v5 2/5] libcamera: converter: Add interface to support\n\tcropping capability","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Jacopo\n\nOn 17/07/24 3:32 pm, Jacopo Mondi wrote:\n> Hi Umang\n>\n> On Wed, Jul 10, 2024 at 01:51:48AM GMT, Umang Jain wrote:\n>> If the converter has cropping capability, the interface should support it\n>> by providing appropriate virtual functions. Provide Feature::Crop in\n>> Feature enumeration for the same.\n>>\n>> Provide virtual setCrop() and getCropBounds() interfaces so that\n>> the converter can implement their own cropping functionality.\n>>\n>> The V4L2M2MConverter implements these interfaces of the Converter\n>> interface. Not all V4L2M2M converters will have cropping capability,\n>> hence Feature::Crop should be set while construction for all those\n>> converters.\n>>\n>> Signed-off-by: Umang Jain \n>> ---\n>> include/libcamera/internal/converter.h | 5 ++\n>> .../internal/converter/converter_v4l2_m2m.h | 6 ++\n>> src/libcamera/converter.cpp | 51 +++++++++++\n>> .../converter/converter_v4l2_m2m.cpp | 88 +++++++++++++++++++\n>> 4 files changed, 150 insertions(+)\n>>\n>> diff --git a/include/libcamera/internal/converter.h b/include/libcamera/internal/converter.h\n>> index 7e478356..9b1223fd 100644\n>> --- a/include/libcamera/internal/converter.h\n>> +++ b/include/libcamera/internal/converter.h\n>> @@ -14,6 +14,7 @@\n>> #include \n>> #include \n>> #include \n>> +#include \n>> #include \n>>\n>> #include \n>> @@ -35,6 +36,7 @@ class Converter\n>> public:\n>> \tenum class Feature {\n>> \t\tNone = 0,\n>> +\t\tCrop = (1 << 0),\n>> \t};\n>>\n>> \tusing Features = Flags;\n>> @@ -63,6 +65,9 @@ public:\n>> \tvirtual int queueBuffers(FrameBuffer *input,\n>> \t\t\t\t const std::map &outputs) = 0;\n>>\n>> +\tvirtual int setCrop(const Stream *stream, Rectangle *rect);\n> \"crop\" is loosely defined as a concept.\n> A converter has an input and an output and I presume it scales.\n>\n> Where is the crop rectangle applied ?\n\nA converter is a processing block - outside of ISP. Hence there is a \ninput and output, and the crop is applied at/w.r.t input. The resultant \nshould be obtained/visible on the output.\n\n>\n> Does this need to be part of the base class interface ? Or should only\n> derived classes that support it implement it ? (it's reasonable for a\n> V4L2 M2M device to support this, once better specified the interface\n> where the crop rectangle has to be applied).\n\nI think so. Atleast that's what Laurent initally asked for. Converter \ninterface should dictate the features supported, the implementation left \nto the derived converter classes. No implementation specific details \nshould be handled by the interface itself, only the \"feature\" part\n\n>> +\tvirtual std::pair getCropBounds(const Stream *stream);\n> not \"get\" in getters\n\nah yes\n>\n>> +\n>> \tSignal inputBufferReady;\n>> \tSignal outputBufferReady;\n>>\n>> diff --git a/include/libcamera/internal/converter/converter_v4l2_m2m.h b/include/libcamera/internal/converter/converter_v4l2_m2m.h\n>> index 91701dbe..2697eed9 100644\n>> --- a/include/libcamera/internal/converter/converter_v4l2_m2m.h\n>> +++ b/include/libcamera/internal/converter/converter_v4l2_m2m.h\n>> @@ -30,6 +30,7 @@ class Size;\n>> class SizeRange;\n>> class Stream;\n>> struct StreamConfiguration;\n>> +class Rectangle;\n>> class V4L2M2MDevice;\n>>\n>> class V4L2M2MConverter : public Converter\n>> @@ -57,6 +58,9 @@ public:\n>> \tint queueBuffers(FrameBuffer *input,\n>> \t\t\t const std::map &outputs);\n>>\n>> +\tint setCrop(const Stream *stream, Rectangle *rect);\n>> +\tstd::pair getCropBounds(const Stream *stream);\n>> +\n>> private:\n>> \tclass V4L2M2MStream : protected Loggable\n>> \t{\n>> @@ -75,6 +79,8 @@ private:\n>>\n>> \t\tint queueBuffers(FrameBuffer *input, FrameBuffer *output);\n>>\n>> +\t\tint setSelection(unsigned int target, Rectangle *rect);\n>> +\n>> \tprotected:\n>> \t\tstd::string logPrefix() const override;\n>>\n>> diff --git a/src/libcamera/converter.cpp b/src/libcamera/converter.cpp\n>> index 2c3da6d4..d51e77a0 100644\n>> --- a/src/libcamera/converter.cpp\n>> +++ b/src/libcamera/converter.cpp\n>> @@ -11,6 +11,8 @@\n>>\n>> #include \n>>\n>> +#include \n>> +\n>> #include \"libcamera/internal/media_device.h\"\n>>\n>> /**\n>> @@ -39,6 +41,8 @@ LOG_DEFINE_CATEGORY(Converter)\n>> * \\brief Specify the features supported by the converter\n>> * \\var Converter::Feature::None\n>> * \\brief No extra features supported by the converter\n>> + * \\var Converter::Feature::Crop\n>> + * \\brief Cropping capability is supported by the converter\n>> */\n>>\n>> /**\n>> @@ -161,6 +165,53 @@ Converter::~Converter()\n>> * \\return 0 on success or a negative error code otherwise\n>> */\n>>\n>> +/**\n>> + * \\brief Set the crop rectangle \\a rect for \\a stream\n>> + * \\param[in] stream Pointer to output stream\n>> + * \\param[inout] rect The crop rectangle to be applied\n>> + *\n>> + * Set the crop rectangle \\a rect for \\a stream provided the converter supports\n>> + * cropping. The converter should have the Feature::Crop flag in this case.\n>> + *\n>> + * \\return 0 on success or a negative error code otherwise\n>> + */\n>> +int Converter::setCrop([[maybe_unused]] const Stream *stream, [[maybe_unused]] Rectangle *rect)\n>> +{\n>> +\tif (!(getFeatures() & Feature::Crop)) {\n>> +\t\tLOG(Converter, Error) << \"Converter doesn't support cropping capabilities\";\n>> +\t\treturn -ENOTSUP;\n>> +\t}\n>> +\n>> +\treturn 0;\n>> +}\n>> +\n>> +/**\n>> + * \\brief Get the crop bounds \\a stream\n>> + * \\param[in] stream Pointer to output stream\n>> + *\n>> + * Get the minimum and maximum crop bounds for \\a stream. The converter\n>> + * should supporting cropping (Feature::Crop).\n>> + *\n>> + * \\return A std::pair containining minimum and maximum\n>> + * crop bound respectively.\n>> + */\n>> +std::pair Converter::getCropBounds([[maybe_unused]] const Stream *stream)\n>> +{\n>> +\tconst StreamConfiguration &config = stream->configuration();\n>> +\tRectangle rect;\n>> +\n>> +\tif (!(getFeatures() & Feature::Crop))\n>> +\t\tLOG(Converter, Error) << \"Converter doesn't support cropping capabilities\";\n>> +\n>> +\t/*\n>> +\t * This is base implementation for the Converter class, so just return\n>> +\t * the stream configured size as minimum and maximum crop bounds.\n>> +\t */\n>> +\trect.size() = config.size;\n>> +\n>> +\treturn { rect, rect };\n> I would rather not make it part of the base class interface\n\nno? If the crop is a feature, getting the supported bounds would be \nsomething callers would like to know. Do you think this should be left \nonly to derived converter classes ?\n>> +}\n>> +\n>> /**\n>> * \\var Converter::inputBufferReady\n>> * \\brief A signal emitted when the input frame buffer completes\n>> diff --git a/src/libcamera/converter/converter_v4l2_m2m.cpp b/src/libcamera/converter/converter_v4l2_m2m.cpp\n>> index 4aeb7dd9..eaae3528 100644\n>> --- a/src/libcamera/converter/converter_v4l2_m2m.cpp\n>> +++ b/src/libcamera/converter/converter_v4l2_m2m.cpp\n>> @@ -155,6 +155,15 @@ int V4L2M2MConverter::V4L2M2MStream::queueBuffers(FrameBuffer *input, FrameBuffe\n>> \treturn 0;\n>> }\n>>\n>> +int V4L2M2MConverter::V4L2M2MStream::setSelection(unsigned int target, Rectangle *rect)\n>> +{\n>> +\tint ret = m2m_->output()->setSelection(target, rect);\n>> +\tif (ret < 0)\n>> +\t\treturn ret;\n>> +\n>> +\treturn 0;\n>> +}\n>> +\n>> std::string V4L2M2MConverter::V4L2M2MStream::logPrefix() const\n>> {\n>> \treturn stream_->configuration().toString();\n>> @@ -375,6 +384,81 @@ int V4L2M2MConverter::exportBuffers(const Stream *stream, unsigned int count,\n>> \treturn iter->second->exportBuffers(count, buffers);\n>> }\n>>\n>> +/**\n>> + * \\copydoc libcamera::Converter::setCrop\n>> + */\n>> +int V4L2M2MConverter::setCrop(const Stream *stream, Rectangle *rect)\n>> +{\n>> +\tif (!(getFeatures() & Feature::Crop))\n>> +\t\treturn -ENOTSUP;\n>> +\n>> +\tauto iter = streams_.find(stream);\n>> +\tif (iter == streams_.end())\n>> +\t\treturn -EINVAL;\n>> +\n>> +\treturn iter->second->setSelection(V4L2_SEL_TGT_CROP, rect);\n>> +}\n>> +\n>> +/**\n>> + * \\copydoc libcamera::Converter::getCropBounds\n>> + */\n>> +std::pair\n>> +V4L2M2MConverter::getCropBounds(const Stream *stream)\n>> +{\n>> +\tRectangle minCrop;\n>> +\tRectangle maxCrop;\n>> +\tint ret;\n>> +\n>> +\tminCrop.width = 1;\n>> +\tminCrop.height = 1;\n>> +\tmaxCrop.width = UINT_MAX;\n>> +\tmaxCrop.height = UINT_MAX;\n> You can move all of this after the check for the feature support\n>\n>> +\n>> +\tif (!(getFeatures() & Feature::Crop)) {\n>> +\t\tLOG(Converter, Error) << \"Crop functionality is not supported\";\n>> +\t\treturn {};\n>> +\t}\n>> +\n>> +\tauto iter = streams_.find(stream);\n>> +\tif (iter == streams_.end()) {\n>> +\t\t/*\n>> +\t\t * No streams configured, return minimum and maximum crop\n>> +\t\t * bounds at initialization.\n>> +\t\t */\n>> +\t\tret = m2m_->output()->setSelection(V4L2_SEL_TGT_CROP, &minCrop);\n>> +\t\tif (ret) {\n>> +\t\t\tLOG(Converter, Error) << \"Failed to query minimum crop bound\";\n>> +\t\t\treturn {};\n>> +\t\t}\n>> +\n>> +\t\tret = m2m_->output()->setSelection(V4L2_SEL_TGT_CROP, &maxCrop);\n>> +\t\tif (ret) {\n>> +\t\t\tLOG(Converter, Error) << \"Failed to query maximum crop bound\";\n>> +\t\t\treturn {};\n>> +\t\t}\n>> +\n>> +\t\treturn { minCrop, maxCrop };\n>> +\t}\n>> +\n>> +\t/*\n>> +\t * If the streams are configured, return bounds from according to\n>> +\t * stream configuration.\n>> +\t */\n>> +\tret = setCrop(stream, &minCrop);\n>> +\tif (ret) {\n>> +\t\tLOG(Converter, Error) << \"Failed to query minimum crop bound\";\n>> +\t\treturn {};\n>> +\t}\n>> +\n>> +\tret = setCrop(stream, &maxCrop);\n>> +\tif (ret) {\n>> +\t\tLOG(Converter, Error) << \"Failed to query maximum crop bound\";\n>> +\t\treturn {};\n>> +\t}\n>> +\n>> +\treturn { minCrop, maxCrop };\n>> +}\n>> +\n>> /**\n>> * \\copydoc libcamera::Converter::start\n>> */\n>> @@ -448,6 +532,10 @@ int V4L2M2MConverter::queueBuffers(FrameBuffer *input,\n>> \treturn 0;\n>> }\n>>\n>> +/*\n>> + * \\todo: This should be extended to include Feature::Flag to denote\n>> + * what each converter supports feature-wise.\n>> + */\n> That's another option.\n>\n> I'll send to the list the patches I have on top of your series that\n> makes it possible to specify features at registration time for a\n> comparison.\n\nRight , this bit is something I have to develop once I get a testing setup.\n\nThe compatibles below are currently used for simple pipeline handler only.\n>\n>> static std::initializer_list compatibles = {\n>> \t\"mtk-mdp\",\n>> \t\"pxp\",\n>> --\n>> 2.45.2\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 874B9BDB1C\n\tfor ;\n\tWed, 17 Jul 2024 11:04:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4F1C06336F;\n\tWed, 17 Jul 2024 13:04:41 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C925F619AB\n\tfor ;\n\tWed, 17 Jul 2024 13:04:39 +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 4EB247E2;\n\tWed, 17 Jul 2024 13:04:01 +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=\"sw34oEJo\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1721214241;\n\tbh=v2an7/JEsT3cDo6Qiypa2pqmS/foNcotJ4wIBZ3Ufg8=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=sw34oEJo3rv6zOMKpZoofNfpJTUkqto9YJFqD2H2g4twDD5pytJsKlOo+82rIVjYl\n\tO/KXjLkjMj9krRUJDkgqNTa3GJ3DMBm65O9ac90KOtpWD1DaaGXWE89Y44objAgHpc\n\tcO4gJGAS3CmqDSCzqKnG/jgG90mGjIQGEMP+E7gI=","Message-ID":"<92cdcc85-6897-40c7-a63c-d4c426e3e18e@ideasonboard.com>","Date":"Wed, 17 Jul 2024 16:34:34 +0530","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v5 2/5] libcamera: converter: Add interface to support\n\tcropping capability","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","References":"<20240709202151.152289-1-umang.jain@ideasonboard.com>\n\t<20240709202151.152289-3-umang.jain@ideasonboard.com>\n\t","Content-Language":"en-US","From":"Umang Jain ","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":30426,"web_url":"https://patchwork.libcamera.org/comment/30426/","msgid":"","date":"2024-07-18T10:06:12","subject":"Re: [PATCH v5 2/5] libcamera: converter: Add interface to support\n\tcropping capability","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Umang\n\nOn Wed, Jul 17, 2024 at 04:34:34PM GMT, Umang Jain wrote:\n> Hi Jacopo\n>\n> On 17/07/24 3:32 pm, Jacopo Mondi wrote:\n> > Hi Umang\n> >\n> > On Wed, Jul 10, 2024 at 01:51:48AM GMT, Umang Jain wrote:\n> > > If the converter has cropping capability, the interface should support it\n> > > by providing appropriate virtual functions. Provide Feature::Crop in\n> > > Feature enumeration for the same.\n> > >\n> > > Provide virtual setCrop() and getCropBounds() interfaces so that\n> > > the converter can implement their own cropping functionality.\n> > >\n> > > The V4L2M2MConverter implements these interfaces of the Converter\n> > > interface. Not all V4L2M2M converters will have cropping capability,\n> > > hence Feature::Crop should be set while construction for all those\n> > > converters.\n> > >\n> > > Signed-off-by: Umang Jain \n> > > ---\n> > > include/libcamera/internal/converter.h | 5 ++\n> > > .../internal/converter/converter_v4l2_m2m.h | 6 ++\n> > > src/libcamera/converter.cpp | 51 +++++++++++\n> > > .../converter/converter_v4l2_m2m.cpp | 88 +++++++++++++++++++\n> > > 4 files changed, 150 insertions(+)\n> > >\n> > > diff --git a/include/libcamera/internal/converter.h b/include/libcamera/internal/converter.h\n> > > index 7e478356..9b1223fd 100644\n> > > --- a/include/libcamera/internal/converter.h\n> > > +++ b/include/libcamera/internal/converter.h\n> > > @@ -14,6 +14,7 @@\n> > > #include \n> > > #include \n> > > #include \n> > > +#include \n> > > #include \n> > >\n> > > #include \n> > > @@ -35,6 +36,7 @@ class Converter\n> > > public:\n> > > \tenum class Feature {\n> > > \t\tNone = 0,\n> > > +\t\tCrop = (1 << 0),\n> > > \t};\n> > >\n> > > \tusing Features = Flags;\n> > > @@ -63,6 +65,9 @@ public:\n> > > \tvirtual int queueBuffers(FrameBuffer *input,\n> > > \t\t\t\t const std::map &outputs) = 0;\n> > >\n> > > +\tvirtual int setCrop(const Stream *stream, Rectangle *rect);\n> > \"crop\" is loosely defined as a concept.\n> > A converter has an input and an output and I presume it scales.\n> >\n> > Where is the crop rectangle applied ?\n>\n> A converter is a processing block - outside of ISP. Hence there is a input\n> and output, and the crop is applied at/w.r.t input. The resultant should be\n> obtained/visible on the output.\n>\n\nAnd where is the assumption that crop is applied to the input\ndocumented ? Aren't there converter that allow to crop on the output\nmaybe -after- scaling ?\n\nCropping before or after scaling (or, if you want to put it different,\ncropping on the input or on the output) are both possible options, and\nthis interface doesn't allow you to specify that.\n\nYou have wired the V4L2 converter to apply selection on the TGT_CROP\ntarget on the input queue (aka on the 'output' video device, oh funny\nnames)\n\nint V4L2M2MConverter::setCrop(const Stream *stream, Rectangle *rect)\n{\n\n\treturn iter->second->setSelection(V4L2_SEL_TGT_CROP, rect);\n}\n\n\nint V4L2M2MConverter::V4L2M2MStream::setSelection(unsigned int target, Rectangle *rect)\n{\n\tint ret = m2m_->output()->setSelection(target, rect);\n\tif (ret < 0)\n\t\treturn ret;\n\n\treturn 0;\n}\n\nCan't a converter expose a TGT_CROP target on the capture device to allow\nuserspace to apply a crop on the \"final\" image before returning it to\napplications ?\n\n> >\n> > Does this need to be part of the base class interface ? Or should only\n> > derived classes that support it implement it ? (it's reasonable for a\n> > V4L2 M2M device to support this, once better specified the interface\n> > where the crop rectangle has to be applied).\n>\n> I think so. Atleast that's what Laurent initally asked for. Converter\n> interface should dictate the features supported, the implementation left to\n> the derived converter classes. No implementation specific details should be\n> handled by the interface itself, only the \"feature\" part\n>\n\nYes, but concepts like 'crop' and 'selection' might only apply to\ncertain types of converter, like the v4l2 m2m ones. I wonder if it is\nbetter to provide a stub implementation in the whole hierarcy or only\nexpose such methods in the classes derived from, in example,\nV4L2M2MConverter.\n\n> > > +\tvirtual std::pair getCropBounds(const Stream *stream);\n> > not \"get\" in getters\n>\n> ah yes\n> >\n> > > +\n> > > \tSignal inputBufferReady;\n> > > \tSignal outputBufferReady;\n> > >\n> > > diff --git a/include/libcamera/internal/converter/converter_v4l2_m2m.h b/include/libcamera/internal/converter/converter_v4l2_m2m.h\n> > > index 91701dbe..2697eed9 100644\n> > > --- a/include/libcamera/internal/converter/converter_v4l2_m2m.h\n> > > +++ b/include/libcamera/internal/converter/converter_v4l2_m2m.h\n> > > @@ -30,6 +30,7 @@ class Size;\n> > > class SizeRange;\n> > > class Stream;\n> > > struct StreamConfiguration;\n> > > +class Rectangle;\n> > > class V4L2M2MDevice;\n> > >\n> > > class V4L2M2MConverter : public Converter\n> > > @@ -57,6 +58,9 @@ public:\n> > > \tint queueBuffers(FrameBuffer *input,\n> > > \t\t\t const std::map &outputs);\n> > >\n> > > +\tint setCrop(const Stream *stream, Rectangle *rect);\n> > > +\tstd::pair getCropBounds(const Stream *stream);\n> > > +\n> > > private:\n> > > \tclass V4L2M2MStream : protected Loggable\n> > > \t{\n> > > @@ -75,6 +79,8 @@ private:\n> > >\n> > > \t\tint queueBuffers(FrameBuffer *input, FrameBuffer *output);\n> > >\n> > > +\t\tint setSelection(unsigned int target, Rectangle *rect);\n> > > +\n> > > \tprotected:\n> > > \t\tstd::string logPrefix() const override;\n> > >\n> > > diff --git a/src/libcamera/converter.cpp b/src/libcamera/converter.cpp\n> > > index 2c3da6d4..d51e77a0 100644\n> > > --- a/src/libcamera/converter.cpp\n> > > +++ b/src/libcamera/converter.cpp\n> > > @@ -11,6 +11,8 @@\n> > >\n> > > #include \n> > >\n> > > +#include \n> > > +\n> > > #include \"libcamera/internal/media_device.h\"\n> > >\n> > > /**\n> > > @@ -39,6 +41,8 @@ LOG_DEFINE_CATEGORY(Converter)\n> > > * \\brief Specify the features supported by the converter\n> > > * \\var Converter::Feature::None\n> > > * \\brief No extra features supported by the converter\n> > > + * \\var Converter::Feature::Crop\n> > > + * \\brief Cropping capability is supported by the converter\n> > > */\n> > >\n> > > /**\n> > > @@ -161,6 +165,53 @@ Converter::~Converter()\n> > > * \\return 0 on success or a negative error code otherwise\n> > > */\n> > >\n> > > +/**\n> > > + * \\brief Set the crop rectangle \\a rect for \\a stream\n> > > + * \\param[in] stream Pointer to output stream\n> > > + * \\param[inout] rect The crop rectangle to be applied\n> > > + *\n> > > + * Set the crop rectangle \\a rect for \\a stream provided the converter supports\n> > > + * cropping. The converter should have the Feature::Crop flag in this case.\n> > > + *\n> > > + * \\return 0 on success or a negative error code otherwise\n> > > + */\n> > > +int Converter::setCrop([[maybe_unused]] const Stream *stream, [[maybe_unused]] Rectangle *rect)\n> > > +{\n> > > +\tif (!(getFeatures() & Feature::Crop)) {\n> > > +\t\tLOG(Converter, Error) << \"Converter doesn't support cropping capabilities\";\n> > > +\t\treturn -ENOTSUP;\n> > > +\t}\n> > > +\n> > > +\treturn 0;\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\brief Get the crop bounds \\a stream\n> > > + * \\param[in] stream Pointer to output stream\n> > > + *\n> > > + * Get the minimum and maximum crop bounds for \\a stream. The converter\n> > > + * should supporting cropping (Feature::Crop).\n> > > + *\n> > > + * \\return A std::pair containining minimum and maximum\n> > > + * crop bound respectively.\n> > > + */\n> > > +std::pair Converter::getCropBounds([[maybe_unused]] const Stream *stream)\n> > > +{\n> > > +\tconst StreamConfiguration &config = stream->configuration();\n> > > +\tRectangle rect;\n> > > +\n> > > +\tif (!(getFeatures() & Feature::Crop))\n> > > +\t\tLOG(Converter, Error) << \"Converter doesn't support cropping capabilities\";\n> > > +\n> > > +\t/*\n> > > +\t * This is base implementation for the Converter class, so just return\n> > > +\t * the stream configured size as minimum and maximum crop bounds.\n> > > +\t */\n> > > +\trect.size() = config.size;\n> > > +\n> > > +\treturn { rect, rect };\n> > I would rather not make it part of the base class interface\n>\n> no? If the crop is a feature, getting the supported bounds would be\n> something callers would like to know. Do you think this should be left only\n> to derived converter classes ?\n\nOnly to the hierarchy of derived classes that supports it, like the\nclasses derived from V4L2M2MConverter (and the class itself). For some\nother \"converters\" cropping and scaling might not make sense at all.\n\n> > > +}\n> > > +\n> > > /**\n> > > * \\var Converter::inputBufferReady\n> > > * \\brief A signal emitted when the input frame buffer completes\n> > > diff --git a/src/libcamera/converter/converter_v4l2_m2m.cpp b/src/libcamera/converter/converter_v4l2_m2m.cpp\n> > > index 4aeb7dd9..eaae3528 100644\n> > > --- a/src/libcamera/converter/converter_v4l2_m2m.cpp\n> > > +++ b/src/libcamera/converter/converter_v4l2_m2m.cpp\n> > > @@ -155,6 +155,15 @@ int V4L2M2MConverter::V4L2M2MStream::queueBuffers(FrameBuffer *input, FrameBuffe\n> > > \treturn 0;\n> > > }\n> > >\n> > > +int V4L2M2MConverter::V4L2M2MStream::setSelection(unsigned int target, Rectangle *rect)\n> > > +{\n> > > +\tint ret = m2m_->output()->setSelection(target, rect);\n> > > +\tif (ret < 0)\n> > > +\t\treturn ret;\n> > > +\n> > > +\treturn 0;\n> > > +}\n> > > +\n> > > std::string V4L2M2MConverter::V4L2M2MStream::logPrefix() const\n> > > {\n> > > \treturn stream_->configuration().toString();\n> > > @@ -375,6 +384,81 @@ int V4L2M2MConverter::exportBuffers(const Stream *stream, unsigned int count,\n> > > \treturn iter->second->exportBuffers(count, buffers);\n> > > }\n> > >\n> > > +/**\n> > > + * \\copydoc libcamera::Converter::setCrop\n> > > + */\n> > > +int V4L2M2MConverter::setCrop(const Stream *stream, Rectangle *rect)\n> > > +{\n> > > +\tif (!(getFeatures() & Feature::Crop))\n> > > +\t\treturn -ENOTSUP;\n> > > +\n> > > +\tauto iter = streams_.find(stream);\n> > > +\tif (iter == streams_.end())\n> > > +\t\treturn -EINVAL;\n> > > +\n> > > +\treturn iter->second->setSelection(V4L2_SEL_TGT_CROP, rect);\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\copydoc libcamera::Converter::getCropBounds\n> > > + */\n> > > +std::pair\n> > > +V4L2M2MConverter::getCropBounds(const Stream *stream)\n> > > +{\n> > > +\tRectangle minCrop;\n> > > +\tRectangle maxCrop;\n> > > +\tint ret;\n> > > +\n> > > +\tminCrop.width = 1;\n> > > +\tminCrop.height = 1;\n> > > +\tmaxCrop.width = UINT_MAX;\n> > > +\tmaxCrop.height = UINT_MAX;\n> > You can move all of this after the check for the feature support\n> >\n> > > +\n> > > +\tif (!(getFeatures() & Feature::Crop)) {\n> > > +\t\tLOG(Converter, Error) << \"Crop functionality is not supported\";\n> > > +\t\treturn {};\n> > > +\t}\n> > > +\n> > > +\tauto iter = streams_.find(stream);\n> > > +\tif (iter == streams_.end()) {\n> > > +\t\t/*\n> > > +\t\t * No streams configured, return minimum and maximum crop\n> > > +\t\t * bounds at initialization.\n> > > +\t\t */\n> > > +\t\tret = m2m_->output()->setSelection(V4L2_SEL_TGT_CROP, &minCrop);\n> > > +\t\tif (ret) {\n> > > +\t\t\tLOG(Converter, Error) << \"Failed to query minimum crop bound\";\n> > > +\t\t\treturn {};\n> > > +\t\t}\n> > > +\n> > > +\t\tret = m2m_->output()->setSelection(V4L2_SEL_TGT_CROP, &maxCrop);\n> > > +\t\tif (ret) {\n> > > +\t\t\tLOG(Converter, Error) << \"Failed to query maximum crop bound\";\n> > > +\t\t\treturn {};\n> > > +\t\t}\n> > > +\n> > > +\t\treturn { minCrop, maxCrop };\n> > > +\t}\n> > > +\n> > > +\t/*\n> > > +\t * If the streams are configured, return bounds from according to\n> > > +\t * stream configuration.\n> > > +\t */\n> > > +\tret = setCrop(stream, &minCrop);\n> > > +\tif (ret) {\n> > > +\t\tLOG(Converter, Error) << \"Failed to query minimum crop bound\";\n> > > +\t\treturn {};\n> > > +\t}\n> > > +\n> > > +\tret = setCrop(stream, &maxCrop);\n> > > +\tif (ret) {\n> > > +\t\tLOG(Converter, Error) << \"Failed to query maximum crop bound\";\n> > > +\t\treturn {};\n> > > +\t}\n> > > +\n> > > +\treturn { minCrop, maxCrop };\n> > > +}\n> > > +\n> > > /**\n> > > * \\copydoc libcamera::Converter::start\n> > > */\n> > > @@ -448,6 +532,10 @@ int V4L2M2MConverter::queueBuffers(FrameBuffer *input,\n> > > \treturn 0;\n> > > }\n> > >\n> > > +/*\n> > > + * \\todo: This should be extended to include Feature::Flag to denote\n> > > + * what each converter supports feature-wise.\n> > > + */\n> > That's another option.\n> >\n> > I'll send to the list the patches I have on top of your series that\n> > makes it possible to specify features at registration time for a\n> > comparison.\n>\n> Right , this bit is something I have to develop once I get a testing setup.\n>\n> The compatibles below are currently used for simple pipeline handler only.\n> >\n> > > static std::initializer_list compatibles = {\n> > > \t\"mtk-mdp\",\n> > > \t\"pxp\",\n> > > --\n> > > 2.45.2\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 B2341C323E\n\tfor ;\n\tThu, 18 Jul 2024 10:06:19 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6CB896336F;\n\tThu, 18 Jul 2024 12:06:18 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E14A3619A9\n\tfor ;\n\tThu, 18 Jul 2024 12:06:16 +0200 (CEST)","from ideasonboard.com (unknown [91.80.77.148])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id F1B604E6;\n\tThu, 18 Jul 2024 12:05:37 +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=\"lBh1b2p7\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1721297138;\n\tbh=HU8EWCIuiqXfwc0eLxY3TuQScu5I16JeA74VL4dYA68=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=lBh1b2p7ZfprvNRfY+z5i4uXurRhiXfS9A9K7jICpvIzqZBnhwQ83MABTMvVxuWu/\n\td5wjr6lVZyVMg8XiM0R4+9gjLOO3mD2zDMoMOJNU3cz2w4hgvJrNP8oQvb0YSMZDe4\n\ta77XZZGHatn8O4GH9IC8SGiUaWFF/Q3IpW9RJTPI=","Date":"Thu, 18 Jul 2024 12:06:12 +0200","From":"Jacopo Mondi ","To":"Umang Jain ","Cc":"Jacopo Mondi , \n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v5 2/5] libcamera: converter: Add interface to support\n\tcropping capability","Message-ID":"","References":"<20240709202151.152289-1-umang.jain@ideasonboard.com>\n\t<20240709202151.152289-3-umang.jain@ideasonboard.com>\n\t\n\t<92cdcc85-6897-40c7-a63c-d4c426e3e18e@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<92cdcc85-6897-40c7-a63c-d4c426e3e18e@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\" "}},{"id":30428,"web_url":"https://patchwork.libcamera.org/comment/30428/","msgid":"","date":"2024-07-19T06:52:56","subject":"Re: [PATCH v5 2/5] libcamera: converter: Add interface to support\n\tcropping capability","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"On Wed, Jul 10, 2024 at 01:51:48AM +0530, Umang Jain wrote:\n> If the converter has cropping capability, the interface should support it\n> by providing appropriate virtual functions. Provide Feature::Crop in\n> Feature enumeration for the same.\n> \n> Provide virtual setCrop() and getCropBounds() interfaces so that\n> the converter can implement their own cropping functionality.\n> \n> The V4L2M2MConverter implements these interfaces of the Converter\n> interface. Not all V4L2M2M converters will have cropping capability,\n> hence Feature::Crop should be set while construction for all those\n> converters.\n> \n> Signed-off-by: Umang Jain \n\nMy comments are satisfied... (although probably remove get from getter\nis good)\n\nReviewed-by: Paul Elder \n\n> ---\n> include/libcamera/internal/converter.h | 5 ++\n> .../internal/converter/converter_v4l2_m2m.h | 6 ++\n> src/libcamera/converter.cpp | 51 +++++++++++\n> .../converter/converter_v4l2_m2m.cpp | 88 +++++++++++++++++++\n> 4 files changed, 150 insertions(+)\n> \n> diff --git a/include/libcamera/internal/converter.h b/include/libcamera/internal/converter.h\n> index 7e478356..9b1223fd 100644\n> --- a/include/libcamera/internal/converter.h\n> +++ b/include/libcamera/internal/converter.h\n> @@ -14,6 +14,7 @@\n> #include \n> #include \n> #include \n> +#include \n> #include \n> \n> #include \n> @@ -35,6 +36,7 @@ class Converter\n> public:\n> \tenum class Feature {\n> \t\tNone = 0,\n> +\t\tCrop = (1 << 0),\n> \t};\n> \n> \tusing Features = Flags;\n> @@ -63,6 +65,9 @@ public:\n> \tvirtual int queueBuffers(FrameBuffer *input,\n> \t\t\t\t const std::map &outputs) = 0;\n> \n> +\tvirtual int setCrop(const Stream *stream, Rectangle *rect);\n> +\tvirtual std::pair getCropBounds(const Stream *stream);\n> +\n> \tSignal inputBufferReady;\n> \tSignal outputBufferReady;\n> \n> diff --git a/include/libcamera/internal/converter/converter_v4l2_m2m.h b/include/libcamera/internal/converter/converter_v4l2_m2m.h\n> index 91701dbe..2697eed9 100644\n> --- a/include/libcamera/internal/converter/converter_v4l2_m2m.h\n> +++ b/include/libcamera/internal/converter/converter_v4l2_m2m.h\n> @@ -30,6 +30,7 @@ class Size;\n> class SizeRange;\n> class Stream;\n> struct StreamConfiguration;\n> +class Rectangle;\n> class V4L2M2MDevice;\n> \n> class V4L2M2MConverter : public Converter\n> @@ -57,6 +58,9 @@ public:\n> \tint queueBuffers(FrameBuffer *input,\n> \t\t\t const std::map &outputs);\n> \n> +\tint setCrop(const Stream *stream, Rectangle *rect);\n> +\tstd::pair getCropBounds(const Stream *stream);\n> +\n> private:\n> \tclass V4L2M2MStream : protected Loggable\n> \t{\n> @@ -75,6 +79,8 @@ private:\n> \n> \t\tint queueBuffers(FrameBuffer *input, FrameBuffer *output);\n> \n> +\t\tint setSelection(unsigned int target, Rectangle *rect);\n> +\n> \tprotected:\n> \t\tstd::string logPrefix() const override;\n> \n> diff --git a/src/libcamera/converter.cpp b/src/libcamera/converter.cpp\n> index 2c3da6d4..d51e77a0 100644\n> --- a/src/libcamera/converter.cpp\n> +++ b/src/libcamera/converter.cpp\n> @@ -11,6 +11,8 @@\n> \n> #include \n> \n> +#include \n> +\n> #include \"libcamera/internal/media_device.h\"\n> \n> /**\n> @@ -39,6 +41,8 @@ LOG_DEFINE_CATEGORY(Converter)\n> * \\brief Specify the features supported by the converter\n> * \\var Converter::Feature::None\n> * \\brief No extra features supported by the converter\n> + * \\var Converter::Feature::Crop\n> + * \\brief Cropping capability is supported by the converter\n> */\n> \n> /**\n> @@ -161,6 +165,53 @@ Converter::~Converter()\n> * \\return 0 on success or a negative error code otherwise\n> */\n> \n> +/**\n> + * \\brief Set the crop rectangle \\a rect for \\a stream\n> + * \\param[in] stream Pointer to output stream\n> + * \\param[inout] rect The crop rectangle to be applied\n> + *\n> + * Set the crop rectangle \\a rect for \\a stream provided the converter supports\n> + * cropping. The converter should have the Feature::Crop flag in this case.\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n> + */\n> +int Converter::setCrop([[maybe_unused]] const Stream *stream, [[maybe_unused]] Rectangle *rect)\n> +{\n> +\tif (!(getFeatures() & Feature::Crop)) {\n> +\t\tLOG(Converter, Error) << \"Converter doesn't support cropping capabilities\";\n> +\t\treturn -ENOTSUP;\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> +/**\n> + * \\brief Get the crop bounds \\a stream\n> + * \\param[in] stream Pointer to output stream\n> + *\n> + * Get the minimum and maximum crop bounds for \\a stream. The converter\n> + * should supporting cropping (Feature::Crop).\n> + *\n> + * \\return A std::pair containining minimum and maximum\n> + * crop bound respectively.\n> + */\n> +std::pair Converter::getCropBounds([[maybe_unused]] const Stream *stream)\n> +{\n> +\tconst StreamConfiguration &config = stream->configuration();\n> +\tRectangle rect;\n> +\n> +\tif (!(getFeatures() & Feature::Crop))\n> +\t\tLOG(Converter, Error) << \"Converter doesn't support cropping capabilities\";\n> +\n> +\t/*\n> +\t * This is base implementation for the Converter class, so just return\n> +\t * the stream configured size as minimum and maximum crop bounds.\n> +\t */\n> +\trect.size() = config.size;\n> +\n> +\treturn { rect, rect };\n> +}\n> +\n> /**\n> * \\var Converter::inputBufferReady\n> * \\brief A signal emitted when the input frame buffer completes\n> diff --git a/src/libcamera/converter/converter_v4l2_m2m.cpp b/src/libcamera/converter/converter_v4l2_m2m.cpp\n> index 4aeb7dd9..eaae3528 100644\n> --- a/src/libcamera/converter/converter_v4l2_m2m.cpp\n> +++ b/src/libcamera/converter/converter_v4l2_m2m.cpp\n> @@ -155,6 +155,15 @@ int V4L2M2MConverter::V4L2M2MStream::queueBuffers(FrameBuffer *input, FrameBuffe\n> \treturn 0;\n> }\n> \n> +int V4L2M2MConverter::V4L2M2MStream::setSelection(unsigned int target, Rectangle *rect)\n> +{\n> +\tint ret = m2m_->output()->setSelection(target, rect);\n> +\tif (ret < 0)\n> +\t\treturn ret;\n> +\n> +\treturn 0;\n> +}\n> +\n> std::string V4L2M2MConverter::V4L2M2MStream::logPrefix() const\n> {\n> \treturn stream_->configuration().toString();\n> @@ -375,6 +384,81 @@ int V4L2M2MConverter::exportBuffers(const Stream *stream, unsigned int count,\n> \treturn iter->second->exportBuffers(count, buffers);\n> }\n> \n> +/**\n> + * \\copydoc libcamera::Converter::setCrop\n> + */\n> +int V4L2M2MConverter::setCrop(const Stream *stream, Rectangle *rect)\n> +{\n> +\tif (!(getFeatures() & Feature::Crop))\n> +\t\treturn -ENOTSUP;\n> +\n> +\tauto iter = streams_.find(stream);\n> +\tif (iter == streams_.end())\n> +\t\treturn -EINVAL;\n> +\n> +\treturn iter->second->setSelection(V4L2_SEL_TGT_CROP, rect);\n> +}\n> +\n> +/**\n> + * \\copydoc libcamera::Converter::getCropBounds\n> + */\n> +std::pair\n> +V4L2M2MConverter::getCropBounds(const Stream *stream)\n> +{\n> +\tRectangle minCrop;\n> +\tRectangle maxCrop;\n> +\tint ret;\n> +\n> +\tminCrop.width = 1;\n> +\tminCrop.height = 1;\n> +\tmaxCrop.width = UINT_MAX;\n> +\tmaxCrop.height = UINT_MAX;\n> +\n> +\tif (!(getFeatures() & Feature::Crop)) {\n> +\t\tLOG(Converter, Error) << \"Crop functionality is not supported\";\n> +\t\treturn {};\n> +\t}\n> +\n> +\tauto iter = streams_.find(stream);\n> +\tif (iter == streams_.end()) {\n> +\t\t/*\n> +\t\t * No streams configured, return minimum and maximum crop\n> +\t\t * bounds at initialization.\n> +\t\t */\n> +\t\tret = m2m_->output()->setSelection(V4L2_SEL_TGT_CROP, &minCrop);\n> +\t\tif (ret) {\n> +\t\t\tLOG(Converter, Error) << \"Failed to query minimum crop bound\";\n> +\t\t\treturn {};\n> +\t\t}\n> +\n> +\t\tret = m2m_->output()->setSelection(V4L2_SEL_TGT_CROP, &maxCrop);\n> +\t\tif (ret) {\n> +\t\t\tLOG(Converter, Error) << \"Failed to query maximum crop bound\";\n> +\t\t\treturn {};\n> +\t\t}\n> +\n> +\t\treturn { minCrop, maxCrop };\n> +\t}\n> +\n> +\t/*\n> +\t * If the streams are configured, return bounds from according to\n> +\t * stream configuration.\n> +\t */\n> +\tret = setCrop(stream, &minCrop);\n> +\tif (ret) {\n> +\t\tLOG(Converter, Error) << \"Failed to query minimum crop bound\";\n> +\t\treturn {};\n> +\t}\n> +\n> +\tret = setCrop(stream, &maxCrop);\n> +\tif (ret) {\n> +\t\tLOG(Converter, Error) << \"Failed to query maximum crop bound\";\n> +\t\treturn {};\n> +\t}\n> +\n> +\treturn { minCrop, maxCrop };\n> +}\n> +\n> /**\n> * \\copydoc libcamera::Converter::start\n> */\n> @@ -448,6 +532,10 @@ int V4L2M2MConverter::queueBuffers(FrameBuffer *input,\n> \treturn 0;\n> }\n> \n> +/*\n> + * \\todo: This should be extended to include Feature::Flag to denote\n> + * what each converter supports feature-wise.\n> + */\n> static std::initializer_list compatibles = {\n> \t\"mtk-mdp\",\n> \t\"pxp\",\n> -- \n> 2.45.2\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 44A54C323E\n\tfor ;\n\tFri, 19 Jul 2024 06:53:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 258F56336F;\n\tFri, 19 Jul 2024 08:53:06 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1BAF16199E\n\tfor ;\n\tFri, 19 Jul 2024 08:53:04 +0200 (CEST)","from pyrite.rasen.tech (h175-177-049-024.catv02.itscom.jp\n\t[175.177.49.24])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id DB43E471;\n\tFri, 19 Jul 2024 08:52: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=\"oNhA90IA\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1721371944;\n\tbh=0coti+CWtGmrTi8frxGWfbeLUTgPTdvwIwjgqjV0iRI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=oNhA90IAQGGb1IVIWR0hRY/+O/TS0/HoTMp8K3F9tyZgLUEtdsACa90JhQQqQwOf6\n\t+QTro0VclF9wnJAjqfmaMpr+/jzVgjx66cF2QReI0D19BLqWHHnukAtkHLj7I9NRYU\n\tjMvvi8UYW6Z8xB3xOIqjnq4B0s23amvvYUaNIBAA=","Date":"Fri, 19 Jul 2024 15:52:56 +0900","From":"Paul Elder ","To":"Umang Jain ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v5 2/5] libcamera: converter: Add interface to support\n\tcropping capability","Message-ID":"","References":"<20240709202151.152289-1-umang.jain@ideasonboard.com>\n\t<20240709202151.152289-3-umang.jain@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20240709202151.152289-3-umang.jain@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\" "}},{"id":30429,"web_url":"https://patchwork.libcamera.org/comment/30429/","msgid":"","date":"2024-07-19T07:03:03","subject":"Re: [PATCH v5 2/5] libcamera: converter: Add interface to support\n\tcropping capability","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"On Thu, Jul 18, 2024 at 12:06:12PM +0200, Jacopo Mondi wrote:\n> Hi Umang\n> \n> On Wed, Jul 17, 2024 at 04:34:34PM GMT, Umang Jain wrote:\n> > Hi Jacopo\n> >\n> > On 17/07/24 3:32 pm, Jacopo Mondi wrote:\n> > > Hi Umang\n> > >\n> > > On Wed, Jul 10, 2024 at 01:51:48AM GMT, Umang Jain wrote:\n> > > > If the converter has cropping capability, the interface should support it\n> > > > by providing appropriate virtual functions. Provide Feature::Crop in\n> > > > Feature enumeration for the same.\n> > > >\n> > > > Provide virtual setCrop() and getCropBounds() interfaces so that\n> > > > the converter can implement their own cropping functionality.\n> > > >\n> > > > The V4L2M2MConverter implements these interfaces of the Converter\n> > > > interface. Not all V4L2M2M converters will have cropping capability,\n> > > > hence Feature::Crop should be set while construction for all those\n> > > > converters.\n> > > >\n> > > > Signed-off-by: Umang Jain \n> > > > ---\n> > > > include/libcamera/internal/converter.h | 5 ++\n> > > > .../internal/converter/converter_v4l2_m2m.h | 6 ++\n> > > > src/libcamera/converter.cpp | 51 +++++++++++\n> > > > .../converter/converter_v4l2_m2m.cpp | 88 +++++++++++++++++++\n> > > > 4 files changed, 150 insertions(+)\n> > > >\n> > > > diff --git a/include/libcamera/internal/converter.h b/include/libcamera/internal/converter.h\n> > > > index 7e478356..9b1223fd 100644\n> > > > --- a/include/libcamera/internal/converter.h\n> > > > +++ b/include/libcamera/internal/converter.h\n> > > > @@ -14,6 +14,7 @@\n> > > > #include \n> > > > #include \n> > > > #include \n> > > > +#include \n> > > > #include \n> > > >\n> > > > #include \n> > > > @@ -35,6 +36,7 @@ class Converter\n> > > > public:\n> > > > \tenum class Feature {\n> > > > \t\tNone = 0,\n> > > > +\t\tCrop = (1 << 0),\n> > > > \t};\n> > > >\n> > > > \tusing Features = Flags;\n> > > > @@ -63,6 +65,9 @@ public:\n> > > > \tvirtual int queueBuffers(FrameBuffer *input,\n> > > > \t\t\t\t const std::map &outputs) = 0;\n> > > >\n> > > > +\tvirtual int setCrop(const Stream *stream, Rectangle *rect);\n> > > \"crop\" is loosely defined as a concept.\n> > > A converter has an input and an output and I presume it scales.\n> > >\n> > > Where is the crop rectangle applied ?\n> >\n> > A converter is a processing block - outside of ISP. Hence there is a input\n> > and output, and the crop is applied at/w.r.t input. The resultant should be\n> > obtained/visible on the output.\n> >\n> \n> And where is the assumption that crop is applied to the input\n> documented ? Aren't there converter that allow to crop on the output\n> maybe -after- scaling ?\n\nI suppose this should be added to the documentation of the cropping\nfeature flag.\n\n> \n> Cropping before or after scaling (or, if you want to put it different,\n> cropping on the input or on the output) are both possible options, and\n> this interface doesn't allow you to specify that.\n> \n> You have wired the V4L2 converter to apply selection on the TGT_CROP\n> target on the input queue (aka on the 'output' video device, oh funny\n> names)\n> \n> int V4L2M2MConverter::setCrop(const Stream *stream, Rectangle *rect)\n> {\n> \n> \treturn iter->second->setSelection(V4L2_SEL_TGT_CROP, rect);\n> }\n> \n> \n> int V4L2M2MConverter::V4L2M2MStream::setSelection(unsigned int target, Rectangle *rect)\n> {\n> \tint ret = m2m_->output()->setSelection(target, rect);\n> \tif (ret < 0)\n> \t\treturn ret;\n> \n> \treturn 0;\n> }\n> \n> Can't a converter expose a TGT_CROP target on the capture device to allow\n> userspace to apply a crop on the \"final\" image before returning it to\n> applications ?\n> \n> > >\n> > > Does this need to be part of the base class interface ? Or should only\n> > > derived classes that support it implement it ? (it's reasonable for a\n> > > V4L2 M2M device to support this, once better specified the interface\n> > > where the crop rectangle has to be applied).\n> >\n> > I think so. Atleast that's what Laurent initally asked for. Converter\n> > interface should dictate the features supported, the implementation left to\n> > the derived converter classes. No implementation specific details should be\n> > handled by the interface itself, only the \"feature\" part\n> >\n> \n> Yes, but concepts like 'crop' and 'selection' might only apply to\n> certain types of converter, like the v4l2 m2m ones. I wonder if it is\n> better to provide a stub implementation in the whole hierarcy or only\n> expose such methods in the classes derived from, in example,\n> V4L2M2MConverter.\n\nMaybe we can have another feature flag for output crop when the need\narises?\n\n> \n> > > > +\tvirtual std::pair getCropBounds(const Stream *stream);\n> > > not \"get\" in getters\n> >\n> > ah yes\n> > >\n> > > > +\n> > > > \tSignal inputBufferReady;\n> > > > \tSignal outputBufferReady;\n> > > >\n> > > > diff --git a/include/libcamera/internal/converter/converter_v4l2_m2m.h b/include/libcamera/internal/converter/converter_v4l2_m2m.h\n> > > > index 91701dbe..2697eed9 100644\n> > > > --- a/include/libcamera/internal/converter/converter_v4l2_m2m.h\n> > > > +++ b/include/libcamera/internal/converter/converter_v4l2_m2m.h\n> > > > @@ -30,6 +30,7 @@ class Size;\n> > > > class SizeRange;\n> > > > class Stream;\n> > > > struct StreamConfiguration;\n> > > > +class Rectangle;\n> > > > class V4L2M2MDevice;\n> > > >\n> > > > class V4L2M2MConverter : public Converter\n> > > > @@ -57,6 +58,9 @@ public:\n> > > > \tint queueBuffers(FrameBuffer *input,\n> > > > \t\t\t const std::map &outputs);\n> > > >\n> > > > +\tint setCrop(const Stream *stream, Rectangle *rect);\n> > > > +\tstd::pair getCropBounds(const Stream *stream);\n> > > > +\n> > > > private:\n> > > > \tclass V4L2M2MStream : protected Loggable\n> > > > \t{\n> > > > @@ -75,6 +79,8 @@ private:\n> > > >\n> > > > \t\tint queueBuffers(FrameBuffer *input, FrameBuffer *output);\n> > > >\n> > > > +\t\tint setSelection(unsigned int target, Rectangle *rect);\n> > > > +\n> > > > \tprotected:\n> > > > \t\tstd::string logPrefix() const override;\n> > > >\n> > > > diff --git a/src/libcamera/converter.cpp b/src/libcamera/converter.cpp\n> > > > index 2c3da6d4..d51e77a0 100644\n> > > > --- a/src/libcamera/converter.cpp\n> > > > +++ b/src/libcamera/converter.cpp\n> > > > @@ -11,6 +11,8 @@\n> > > >\n> > > > #include \n> > > >\n> > > > +#include \n> > > > +\n> > > > #include \"libcamera/internal/media_device.h\"\n> > > >\n> > > > /**\n> > > > @@ -39,6 +41,8 @@ LOG_DEFINE_CATEGORY(Converter)\n> > > > * \\brief Specify the features supported by the converter\n> > > > * \\var Converter::Feature::None\n> > > > * \\brief No extra features supported by the converter\n> > > > + * \\var Converter::Feature::Crop\n> > > > + * \\brief Cropping capability is supported by the converter\n> > > > */\n> > > >\n> > > > /**\n> > > > @@ -161,6 +165,53 @@ Converter::~Converter()\n> > > > * \\return 0 on success or a negative error code otherwise\n> > > > */\n> > > >\n> > > > +/**\n> > > > + * \\brief Set the crop rectangle \\a rect for \\a stream\n> > > > + * \\param[in] stream Pointer to output stream\n> > > > + * \\param[inout] rect The crop rectangle to be applied\n> > > > + *\n> > > > + * Set the crop rectangle \\a rect for \\a stream provided the converter supports\n> > > > + * cropping. The converter should have the Feature::Crop flag in this case.\n> > > > + *\n> > > > + * \\return 0 on success or a negative error code otherwise\n> > > > + */\n> > > > +int Converter::setCrop([[maybe_unused]] const Stream *stream, [[maybe_unused]] Rectangle *rect)\n> > > > +{\n> > > > +\tif (!(getFeatures() & Feature::Crop)) {\n> > > > +\t\tLOG(Converter, Error) << \"Converter doesn't support cropping capabilities\";\n> > > > +\t\treturn -ENOTSUP;\n> > > > +\t}\n> > > > +\n> > > > +\treturn 0;\n> > > > +}\n> > > > +\n> > > > +/**\n> > > > + * \\brief Get the crop bounds \\a stream\n> > > > + * \\param[in] stream Pointer to output stream\n> > > > + *\n> > > > + * Get the minimum and maximum crop bounds for \\a stream. The converter\n> > > > + * should supporting cropping (Feature::Crop).\n> > > > + *\n> > > > + * \\return A std::pair containining minimum and maximum\n> > > > + * crop bound respectively.\n> > > > + */\n> > > > +std::pair Converter::getCropBounds([[maybe_unused]] const Stream *stream)\n> > > > +{\n> > > > +\tconst StreamConfiguration &config = stream->configuration();\n> > > > +\tRectangle rect;\n> > > > +\n> > > > +\tif (!(getFeatures() & Feature::Crop))\n> > > > +\t\tLOG(Converter, Error) << \"Converter doesn't support cropping capabilities\";\n> > > > +\n> > > > +\t/*\n> > > > +\t * This is base implementation for the Converter class, so just return\n> > > > +\t * the stream configured size as minimum and maximum crop bounds.\n> > > > +\t */\n> > > > +\trect.size() = config.size;\n> > > > +\n> > > > +\treturn { rect, rect };\n> > > I would rather not make it part of the base class interface\n> >\n> > no? If the crop is a feature, getting the supported bounds would be\n> > something callers would like to know. Do you think this should be left only\n> > to derived converter classes ?\n> \n> Only to the hierarchy of derived classes that supports it, like the\n> classes derived from V4L2M2MConverter (and the class itself). For some\n> other \"converters\" cropping and scaling might not make sense at all.\n> \n\nI thought the features flag tells you which features the converter\nsupports. And then for C++ purposes for converters that don't support\nthe feature, we have default nops for the setter , and a reasonable\ndefault that makes sense for getters.\n\n\nPaul\n\n> > > > +}\n> > > > +\n> > > > /**\n> > > > * \\var Converter::inputBufferReady\n> > > > * \\brief A signal emitted when the input frame buffer completes\n> > > > diff --git a/src/libcamera/converter/converter_v4l2_m2m.cpp b/src/libcamera/converter/converter_v4l2_m2m.cpp\n> > > > index 4aeb7dd9..eaae3528 100644\n> > > > --- a/src/libcamera/converter/converter_v4l2_m2m.cpp\n> > > > +++ b/src/libcamera/converter/converter_v4l2_m2m.cpp\n> > > > @@ -155,6 +155,15 @@ int V4L2M2MConverter::V4L2M2MStream::queueBuffers(FrameBuffer *input, FrameBuffe\n> > > > \treturn 0;\n> > > > }\n> > > >\n> > > > +int V4L2M2MConverter::V4L2M2MStream::setSelection(unsigned int target, Rectangle *rect)\n> > > > +{\n> > > > +\tint ret = m2m_->output()->setSelection(target, rect);\n> > > > +\tif (ret < 0)\n> > > > +\t\treturn ret;\n> > > > +\n> > > > +\treturn 0;\n> > > > +}\n> > > > +\n> > > > std::string V4L2M2MConverter::V4L2M2MStream::logPrefix() const\n> > > > {\n> > > > \treturn stream_->configuration().toString();\n> > > > @@ -375,6 +384,81 @@ int V4L2M2MConverter::exportBuffers(const Stream *stream, unsigned int count,\n> > > > \treturn iter->second->exportBuffers(count, buffers);\n> > > > }\n> > > >\n> > > > +/**\n> > > > + * \\copydoc libcamera::Converter::setCrop\n> > > > + */\n> > > > +int V4L2M2MConverter::setCrop(const Stream *stream, Rectangle *rect)\n> > > > +{\n> > > > +\tif (!(getFeatures() & Feature::Crop))\n> > > > +\t\treturn -ENOTSUP;\n> > > > +\n> > > > +\tauto iter = streams_.find(stream);\n> > > > +\tif (iter == streams_.end())\n> > > > +\t\treturn -EINVAL;\n> > > > +\n> > > > +\treturn iter->second->setSelection(V4L2_SEL_TGT_CROP, rect);\n> > > > +}\n> > > > +\n> > > > +/**\n> > > > + * \\copydoc libcamera::Converter::getCropBounds\n> > > > + */\n> > > > +std::pair\n> > > > +V4L2M2MConverter::getCropBounds(const Stream *stream)\n> > > > +{\n> > > > +\tRectangle minCrop;\n> > > > +\tRectangle maxCrop;\n> > > > +\tint ret;\n> > > > +\n> > > > +\tminCrop.width = 1;\n> > > > +\tminCrop.height = 1;\n> > > > +\tmaxCrop.width = UINT_MAX;\n> > > > +\tmaxCrop.height = UINT_MAX;\n> > > You can move all of this after the check for the feature support\n> > >\n> > > > +\n> > > > +\tif (!(getFeatures() & Feature::Crop)) {\n> > > > +\t\tLOG(Converter, Error) << \"Crop functionality is not supported\";\n> > > > +\t\treturn {};\n> > > > +\t}\n> > > > +\n> > > > +\tauto iter = streams_.find(stream);\n> > > > +\tif (iter == streams_.end()) {\n> > > > +\t\t/*\n> > > > +\t\t * No streams configured, return minimum and maximum crop\n> > > > +\t\t * bounds at initialization.\n> > > > +\t\t */\n> > > > +\t\tret = m2m_->output()->setSelection(V4L2_SEL_TGT_CROP, &minCrop);\n> > > > +\t\tif (ret) {\n> > > > +\t\t\tLOG(Converter, Error) << \"Failed to query minimum crop bound\";\n> > > > +\t\t\treturn {};\n> > > > +\t\t}\n> > > > +\n> > > > +\t\tret = m2m_->output()->setSelection(V4L2_SEL_TGT_CROP, &maxCrop);\n> > > > +\t\tif (ret) {\n> > > > +\t\t\tLOG(Converter, Error) << \"Failed to query maximum crop bound\";\n> > > > +\t\t\treturn {};\n> > > > +\t\t}\n> > > > +\n> > > > +\t\treturn { minCrop, maxCrop };\n> > > > +\t}\n> > > > +\n> > > > +\t/*\n> > > > +\t * If the streams are configured, return bounds from according to\n> > > > +\t * stream configuration.\n> > > > +\t */\n> > > > +\tret = setCrop(stream, &minCrop);\n> > > > +\tif (ret) {\n> > > > +\t\tLOG(Converter, Error) << \"Failed to query minimum crop bound\";\n> > > > +\t\treturn {};\n> > > > +\t}\n> > > > +\n> > > > +\tret = setCrop(stream, &maxCrop);\n> > > > +\tif (ret) {\n> > > > +\t\tLOG(Converter, Error) << \"Failed to query maximum crop bound\";\n> > > > +\t\treturn {};\n> > > > +\t}\n> > > > +\n> > > > +\treturn { minCrop, maxCrop };\n> > > > +}\n> > > > +\n> > > > /**\n> > > > * \\copydoc libcamera::Converter::start\n> > > > */\n> > > > @@ -448,6 +532,10 @@ int V4L2M2MConverter::queueBuffers(FrameBuffer *input,\n> > > > \treturn 0;\n> > > > }\n> > > >\n> > > > +/*\n> > > > + * \\todo: This should be extended to include Feature::Flag to denote\n> > > > + * what each converter supports feature-wise.\n> > > > + */\n> > > That's another option.\n> > >\n> > > I'll send to the list the patches I have on top of your series that\n> > > makes it possible to specify features at registration time for a\n> > > comparison.\n> >\n> > Right , this bit is something I have to develop once I get a testing setup.\n> >\n> > The compatibles below are currently used for simple pipeline handler only.\n> > >\n> > > > static std::initializer_list compatibles = {\n> > > > \t\"mtk-mdp\",\n> > > > \t\"pxp\",\n> > > > --\n> > > > 2.45.2\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 6D65FBDB1C\n\tfor ;\n\tFri, 19 Jul 2024 07:03:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5FD546336F;\n\tFri, 19 Jul 2024 09:03:13 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 00B1F619A3\n\tfor ;\n\tFri, 19 Jul 2024 09:03:11 +0200 (CEST)","from pyrite.rasen.tech (h175-177-049-024.catv02.itscom.jp\n\t[175.177.49.24])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 89DC82B3;\n\tFri, 19 Jul 2024 09:02:31 +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=\"KtVM9IEq\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1721372552;\n\tbh=fxX2+eVTdQzXfKU8G2oop5zr35LdxSwtxywSGm65EdE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=KtVM9IEqlPcBmEb5kA4/wsSaIKJy3qTUomIix1Eo5XnfCfkWP/rBVGp/SE3B3/oyq\n\tHfMjP9cytFp7KenUIl04zok7iuEVwvypdInZlo3d05m5Ub32CemQQYD7rOQ/HvTnVh\n\tf7v1fH71TClnMP0d7JN5hL9I8IJJjsPKKkUee/go=","Date":"Fri, 19 Jul 2024 16:03:03 +0900","From":"Paul Elder ","To":"Jacopo Mondi ","Cc":"Umang Jain ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v5 2/5] libcamera: converter: Add interface to support\n\tcropping capability","Message-ID":"","References":"<20240709202151.152289-1-umang.jain@ideasonboard.com>\n\t<20240709202151.152289-3-umang.jain@ideasonboard.com>\n\t\n\t<92cdcc85-6897-40c7-a63c-d4c426e3e18e@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"","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":30430,"web_url":"https://patchwork.libcamera.org/comment/30430/","msgid":"","date":"2024-07-19T07:05:51","subject":"Re: [PATCH v5 2/5] libcamera: converter: Add interface to support\n\tcropping capability","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"On Wed, Jul 10, 2024 at 01:51:48AM +0530, Umang Jain wrote:\n> If the converter has cropping capability, the interface should support it\n> by providing appropriate virtual functions. Provide Feature::Crop in\n> Feature enumeration for the same.\n> \n> Provide virtual setCrop() and getCropBounds() interfaces so that\n> the converter can implement their own cropping functionality.\n> \n> The V4L2M2MConverter implements these interfaces of the Converter\n> interface. Not all V4L2M2M converters will have cropping capability,\n> hence Feature::Crop should be set while construction for all those\n> converters.\n> \n> Signed-off-by: Umang Jain \n> ---\n> include/libcamera/internal/converter.h | 5 ++\n> .../internal/converter/converter_v4l2_m2m.h | 6 ++\n> src/libcamera/converter.cpp | 51 +++++++++++\n> .../converter/converter_v4l2_m2m.cpp | 88 +++++++++++++++++++\n> 4 files changed, 150 insertions(+)\n> \n> diff --git a/include/libcamera/internal/converter.h b/include/libcamera/internal/converter.h\n> index 7e478356..9b1223fd 100644\n> --- a/include/libcamera/internal/converter.h\n> +++ b/include/libcamera/internal/converter.h\n> @@ -14,6 +14,7 @@\n> #include \n> #include \n> #include \n> +#include \n> #include \n> \n> #include \n> @@ -35,6 +36,7 @@ class Converter\n> public:\n> \tenum class Feature {\n> \t\tNone = 0,\n> +\t\tCrop = (1 << 0),\n> \t};\n> \n> \tusing Features = Flags;\n> @@ -63,6 +65,9 @@ public:\n> \tvirtual int queueBuffers(FrameBuffer *input,\n> \t\t\t\t const std::map &outputs) = 0;\n> \n> +\tvirtual int setCrop(const Stream *stream, Rectangle *rect);\n> +\tvirtual std::pair getCropBounds(const Stream *stream);\n> +\n> \tSignal inputBufferReady;\n> \tSignal outputBufferReady;\n> \n> diff --git a/include/libcamera/internal/converter/converter_v4l2_m2m.h b/include/libcamera/internal/converter/converter_v4l2_m2m.h\n> index 91701dbe..2697eed9 100644\n> --- a/include/libcamera/internal/converter/converter_v4l2_m2m.h\n> +++ b/include/libcamera/internal/converter/converter_v4l2_m2m.h\n> @@ -30,6 +30,7 @@ class Size;\n> class SizeRange;\n> class Stream;\n> struct StreamConfiguration;\n> +class Rectangle;\n> class V4L2M2MDevice;\n> \n> class V4L2M2MConverter : public Converter\n> @@ -57,6 +58,9 @@ public:\n> \tint queueBuffers(FrameBuffer *input,\n> \t\t\t const std::map &outputs);\n> \n> +\tint setCrop(const Stream *stream, Rectangle *rect);\n> +\tstd::pair getCropBounds(const Stream *stream);\n> +\n> private:\n> \tclass V4L2M2MStream : protected Loggable\n> \t{\n> @@ -75,6 +79,8 @@ private:\n> \n> \t\tint queueBuffers(FrameBuffer *input, FrameBuffer *output);\n> \n> +\t\tint setSelection(unsigned int target, Rectangle *rect);\n> +\n> \tprotected:\n> \t\tstd::string logPrefix() const override;\n> \n> diff --git a/src/libcamera/converter.cpp b/src/libcamera/converter.cpp\n> index 2c3da6d4..d51e77a0 100644\n> --- a/src/libcamera/converter.cpp\n> +++ b/src/libcamera/converter.cpp\n> @@ -11,6 +11,8 @@\n> \n> #include \n> \n> +#include \n> +\n> #include \"libcamera/internal/media_device.h\"\n> \n> /**\n> @@ -39,6 +41,8 @@ LOG_DEFINE_CATEGORY(Converter)\n> * \\brief Specify the features supported by the converter\n> * \\var Converter::Feature::None\n> * \\brief No extra features supported by the converter\n> + * \\var Converter::Feature::Crop\n> + * \\brief Cropping capability is supported by the converter\n> */\n> \n> /**\n> @@ -161,6 +165,53 @@ Converter::~Converter()\n> * \\return 0 on success or a negative error code otherwise\n> */\n> \n> +/**\n> + * \\brief Set the crop rectangle \\a rect for \\a stream\n> + * \\param[in] stream Pointer to output stream\n> + * \\param[inout] rect The crop rectangle to be applied\n> + *\n> + * Set the crop rectangle \\a rect for \\a stream provided the converter supports\n> + * cropping. The converter should have the Feature::Crop flag in this case.\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n> + */\n> +int Converter::setCrop([[maybe_unused]] const Stream *stream, [[maybe_unused]] Rectangle *rect)\n> +{\n> +\tif (!(getFeatures() & Feature::Crop)) {\n> +\t\tLOG(Converter, Error) << \"Converter doesn't support cropping capabilities\";\n> +\t\treturn -ENOTSUP;\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> +/**\n> + * \\brief Get the crop bounds \\a stream\n> + * \\param[in] stream Pointer to output stream\n> + *\n> + * Get the minimum and maximum crop bounds for \\a stream. The converter\n> + * should supporting cropping (Feature::Crop).\n> + *\n> + * \\return A std::pair containining minimum and maximum\n> + * crop bound respectively.\n> + */\n> +std::pair Converter::getCropBounds([[maybe_unused]] const Stream *stream)\n> +{\n> +\tconst StreamConfiguration &config = stream->configuration();\n> +\tRectangle rect;\n> +\n> +\tif (!(getFeatures() & Feature::Crop))\n> +\t\tLOG(Converter, Error) << \"Converter doesn't support cropping capabilities\";\n> +\n> +\t/*\n> +\t * This is base implementation for the Converter class, so just return\n> +\t * the stream configured size as minimum and maximum crop bounds.\n> +\t */\n> +\trect.size() = config.size;\n> +\n> +\treturn { rect, rect };\n> +}\n> +\n> /**\n> * \\var Converter::inputBufferReady\n> * \\brief A signal emitted when the input frame buffer completes\n> diff --git a/src/libcamera/converter/converter_v4l2_m2m.cpp b/src/libcamera/converter/converter_v4l2_m2m.cpp\n> index 4aeb7dd9..eaae3528 100644\n> --- a/src/libcamera/converter/converter_v4l2_m2m.cpp\n> +++ b/src/libcamera/converter/converter_v4l2_m2m.cpp\n> @@ -155,6 +155,15 @@ int V4L2M2MConverter::V4L2M2MStream::queueBuffers(FrameBuffer *input, FrameBuffe\n> \treturn 0;\n> }\n> \n> +int V4L2M2MConverter::V4L2M2MStream::setSelection(unsigned int target, Rectangle *rect)\n> +{\n> +\tint ret = m2m_->output()->setSelection(target, rect);\n> +\tif (ret < 0)\n> +\t\treturn ret;\n> +\n> +\treturn 0;\n> +}\n> +\n> std::string V4L2M2MConverter::V4L2M2MStream::logPrefix() const\n> {\n> \treturn stream_->configuration().toString();\n> @@ -375,6 +384,81 @@ int V4L2M2MConverter::exportBuffers(const Stream *stream, unsigned int count,\n> \treturn iter->second->exportBuffers(count, buffers);\n> }\n> \n> +/**\n> + * \\copydoc libcamera::Converter::setCrop\n> + */\n> +int V4L2M2MConverter::setCrop(const Stream *stream, Rectangle *rect)\n> +{\n> +\tif (!(getFeatures() & Feature::Crop))\n> +\t\treturn -ENOTSUP;\n> +\n> +\tauto iter = streams_.find(stream);\n> +\tif (iter == streams_.end())\n> +\t\treturn -EINVAL;\n> +\n> +\treturn iter->second->setSelection(V4L2_SEL_TGT_CROP, rect);\n> +}\n> +\n> +/**\n> + * \\copydoc libcamera::Converter::getCropBounds\n> + */\n> +std::pair\n> +V4L2M2MConverter::getCropBounds(const Stream *stream)\n> +{\n> +\tRectangle minCrop;\n> +\tRectangle maxCrop;\n> +\tint ret;\n> +\n> +\tminCrop.width = 1;\n> +\tminCrop.height = 1;\n> +\tmaxCrop.width = UINT_MAX;\n> +\tmaxCrop.height = UINT_MAX;\n> +\n> +\tif (!(getFeatures() & Feature::Crop)) {\n> +\t\tLOG(Converter, Error) << \"Crop functionality is not supported\";\n> +\t\treturn {};\n\nHold on, shouldn't this return { maxCrop, maxCrop } ? Or at least the\nconfigured output size. That's equivalent to \"crop not supported\".\n\n> +\t}\n> +\n> +\tauto iter = streams_.find(stream);\n> +\tif (iter == streams_.end()) {\n> +\t\t/*\n> +\t\t * No streams configured, return minimum and maximum crop\n> +\t\t * bounds at initialization.\n> +\t\t */\n> +\t\tret = m2m_->output()->setSelection(V4L2_SEL_TGT_CROP, &minCrop);\n> +\t\tif (ret) {\n> +\t\t\tLOG(Converter, Error) << \"Failed to query minimum crop bound\";\n> +\t\t\treturn {};\n\nHere too.\n\n> +\t\t}\n> +\n> +\t\tret = m2m_->output()->setSelection(V4L2_SEL_TGT_CROP, &maxCrop);\n> +\t\tif (ret) {\n> +\t\t\tLOG(Converter, Error) << \"Failed to query maximum crop bound\";\n> +\t\t\treturn {};\n\nAnd here.\n\n\nPaul\n\n> +\t\t}\n> +\n> +\t\treturn { minCrop, maxCrop };\n> +\t}\n> +\n> +\t/*\n> +\t * If the streams are configured, return bounds from according to\n> +\t * stream configuration.\n> +\t */\n> +\tret = setCrop(stream, &minCrop);\n> +\tif (ret) {\n> +\t\tLOG(Converter, Error) << \"Failed to query minimum crop bound\";\n> +\t\treturn {};\n> +\t}\n> +\n> +\tret = setCrop(stream, &maxCrop);\n> +\tif (ret) {\n> +\t\tLOG(Converter, Error) << \"Failed to query maximum crop bound\";\n> +\t\treturn {};\n> +\t}\n> +\n> +\treturn { minCrop, maxCrop };\n> +}\n> +\n> /**\n> * \\copydoc libcamera::Converter::start\n> */\n> @@ -448,6 +532,10 @@ int V4L2M2MConverter::queueBuffers(FrameBuffer *input,\n> \treturn 0;\n> }\n> \n> +/*\n> + * \\todo: This should be extended to include Feature::Flag to denote\n> + * what each converter supports feature-wise.\n> + */\n> static std::initializer_list compatibles = {\n> \t\"mtk-mdp\",\n> \t\"pxp\",\n> -- \n> 2.45.2\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 BB183BDB1C\n\tfor ;\n\tFri, 19 Jul 2024 07:06:00 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 845A363372;\n\tFri, 19 Jul 2024 09:05:59 +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 10E02619A3\n\tfor ;\n\tFri, 19 Jul 2024 09:05:58 +0200 (CEST)","from pyrite.rasen.tech (h175-177-049-024.catv02.itscom.jp\n\t[175.177.49.24])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id DE0D42B3;\n\tFri, 19 Jul 2024 09:05:17 +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=\"NhDhZSIS\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1721372718;\n\tbh=Jz33Qj9pB4ZJ/YGpwTF0lnBXzttfBSvVZutzaL5rmMA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=NhDhZSISSidoc80KyynVFxI/9S3Uf2H81NdcFFdifc4SzYkvtTBZ+MQDpGkyl+21a\n\tD5kysk64cuYvnNUxiTNvHyze8P4uAnbrBdDrKYEMwtjGNGjzMRHujOI9WNeWe3Yceu\n\tre3Kd21aLJuLMpxWE+tiFGfCghy5DzGQBZZKrf90=","Date":"Fri, 19 Jul 2024 16:05:51 +0900","From":"Paul Elder ","To":"Umang Jain ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v5 2/5] libcamera: converter: Add interface to support\n\tcropping capability","Message-ID":"","References":"<20240709202151.152289-1-umang.jain@ideasonboard.com>\n\t<20240709202151.152289-3-umang.jain@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20240709202151.152289-3-umang.jain@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\" "}},{"id":30439,"web_url":"https://patchwork.libcamera.org/comment/30439/","msgid":"","date":"2024-07-19T10:01:48","subject":"Re: [PATCH v5 2/5] libcamera: converter: Add interface to support\n\tcropping capability","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Jacopo\n\nOn 18/07/24 3:36 pm, Jacopo Mondi wrote:\n> Hi Umang\n>\n> On Wed, Jul 17, 2024 at 04:34:34PM GMT, Umang Jain wrote:\n>> Hi Jacopo\n>>\n>> On 17/07/24 3:32 pm, Jacopo Mondi wrote:\n>>> Hi Umang\n>>>\n>>> On Wed, Jul 10, 2024 at 01:51:48AM GMT, Umang Jain wrote:\n>>>> If the converter has cropping capability, the interface should support it\n>>>> by providing appropriate virtual functions. Provide Feature::Crop in\n>>>> Feature enumeration for the same.\n>>>>\n>>>> Provide virtual setCrop() and getCropBounds() interfaces so that\n>>>> the converter can implement their own cropping functionality.\n>>>>\n>>>> The V4L2M2MConverter implements these interfaces of the Converter\n>>>> interface. Not all V4L2M2M converters will have cropping capability,\n>>>> hence Feature::Crop should be set while construction for all those\n>>>> converters.\n>>>>\n>>>> Signed-off-by: Umang Jain \n>>>> ---\n>>>> include/libcamera/internal/converter.h | 5 ++\n>>>> .../internal/converter/converter_v4l2_m2m.h | 6 ++\n>>>> src/libcamera/converter.cpp | 51 +++++++++++\n>>>> .../converter/converter_v4l2_m2m.cpp | 88 +++++++++++++++++++\n>>>> 4 files changed, 150 insertions(+)\n>>>>\n>>>> diff --git a/include/libcamera/internal/converter.h b/include/libcamera/internal/converter.h\n>>>> index 7e478356..9b1223fd 100644\n>>>> --- a/include/libcamera/internal/converter.h\n>>>> +++ b/include/libcamera/internal/converter.h\n>>>> @@ -14,6 +14,7 @@\n>>>> #include \n>>>> #include \n>>>> #include \n>>>> +#include \n>>>> #include \n>>>>\n>>>> #include \n>>>> @@ -35,6 +36,7 @@ class Converter\n>>>> public:\n>>>> \tenum class Feature {\n>>>> \t\tNone = 0,\n>>>> +\t\tCrop = (1 << 0),\n>>>> \t};\n>>>>\n>>>> \tusing Features = Flags;\n>>>> @@ -63,6 +65,9 @@ public:\n>>>> \tvirtual int queueBuffers(FrameBuffer *input,\n>>>> \t\t\t\t const std::map &outputs) = 0;\n>>>>\n>>>> +\tvirtual int setCrop(const Stream *stream, Rectangle *rect);\n>>> \"crop\" is loosely defined as a concept.\n>>> A converter has an input and an output and I presume it scales.\n>>>\n>>> Where is the crop rectangle applied ?\n>> A converter is a processing block - outside of ISP. Hence there is a input\n>> and output, and the crop is applied at/w.r.t input. The resultant should be\n>> obtained/visible on the output.\n>>\n> And where is the assumption that crop is applied to the input\n> documented ? Aren't there converter that allow to crop on the output\n> maybe -after- scaling ?\n>\n> Cropping before or after scaling (or, if you want to put it different,\n> cropping on the input or on the output) are both possible options, and\n> this interface doesn't allow you to specify that.\n\ncropping after scaling means the application would get a different image \noutput (if it tries to set crop on the output) ? Is that possible, a \nruntime crop ? Image output size differs everytime a crop is applied ?\n\nI think what you are referring here is the compose rectangle, rather \nthan the crop rectangle? Is that correct?\n\n>\n> You have wired the V4L2 converter to apply selection on the TGT_CROP\n> target on the input queue (aka on the 'output' video device, oh funny\n> names)\n>\n> int V4L2M2MConverter::setCrop(const Stream *stream, Rectangle *rect)\n> {\n>\n> \treturn iter->second->setSelection(V4L2_SEL_TGT_CROP, rect);\n> }\n>\n>\n> int V4L2M2MConverter::V4L2M2MStream::setSelection(unsigned int target, Rectangle *rect)\n> {\n> \tint ret = m2m_->output()->setSelection(target, rect);\n> \tif (ret < 0)\n> \t\treturn ret;\n>\n> \treturn 0;\n> }\n>\n> Can't a converter expose a TGT_CROP target on the capture device to allow\n> userspace to apply a crop on the \"final\" image before returning it to\n> applications ?\n\nSo having read \nhttps://docs.kernel.org/userspace-api/media/v4l/selection-api-intro.html, \nit seems TGT_CROP can be target for video capture as well. There is no \nlimitation on that front.\n\nHowever, I can't wrap around my head around that kind of crop - applied \nat runtime (like ScalerCrop) ?  For video capture, TGT_COMPOSE makes \nmore sense to me, rather than exposing TGT_CROP.\n\nCertainly the interface is lacking such a distinction in this form - and \nI should clarify that the crop would be set on the input video signal. \nWould that makes things acceptable?\n\nI am also a bit wary about how should we be exposing such a interface \nfor converters, for output and video capture devices, have TGT_CROP and \nTGT_COMPOSE targets ... its seems hairy for now, to me..\n\n>>> Does this need to be part of the base class interface ? Or should only\n>>> derived classes that support it implement it ? (it's reasonable for a\n>>> V4L2 M2M device to support this, once better specified the interface\n>>> where the crop rectangle has to be applied).\n>> I think so. Atleast that's what Laurent initally asked for. Converter\n>> interface should dictate the features supported, the implementation left to\n>> the derived converter classes. No implementation specific details should be\n>> handled by the interface itself, only the \"feature\" part\n>>\n> Yes, but concepts like 'crop' and 'selection' might only apply to\n> certain types of converter, like the v4l2 m2m ones. I wonder if it is\n> better to provide a stub implementation in the whole hierarcy or only\n> expose such methods in the classes derived from, in example,\n> V4L2M2MConverter.\n>\n>>>> +\tvirtual std::pair getCropBounds(const Stream *stream);\n>>> not \"get\" in getters\n>> ah yes\n>>>> +\n>>>> \tSignal inputBufferReady;\n>>>> \tSignal outputBufferReady;\n>>>>\n>>>> diff --git a/include/libcamera/internal/converter/converter_v4l2_m2m.h b/include/libcamera/internal/converter/converter_v4l2_m2m.h\n>>>> index 91701dbe..2697eed9 100644\n>>>> --- a/include/libcamera/internal/converter/converter_v4l2_m2m.h\n>>>> +++ b/include/libcamera/internal/converter/converter_v4l2_m2m.h\n>>>> @@ -30,6 +30,7 @@ class Size;\n>>>> class SizeRange;\n>>>> class Stream;\n>>>> struct StreamConfiguration;\n>>>> +class Rectangle;\n>>>> class V4L2M2MDevice;\n>>>>\n>>>> class V4L2M2MConverter : public Converter\n>>>> @@ -57,6 +58,9 @@ public:\n>>>> \tint queueBuffers(FrameBuffer *input,\n>>>> \t\t\t const std::map &outputs);\n>>>>\n>>>> +\tint setCrop(const Stream *stream, Rectangle *rect);\n>>>> +\tstd::pair getCropBounds(const Stream *stream);\n>>>> +\n>>>> private:\n>>>> \tclass V4L2M2MStream : protected Loggable\n>>>> \t{\n>>>> @@ -75,6 +79,8 @@ private:\n>>>>\n>>>> \t\tint queueBuffers(FrameBuffer *input, FrameBuffer *output);\n>>>>\n>>>> +\t\tint setSelection(unsigned int target, Rectangle *rect);\n>>>> +\n>>>> \tprotected:\n>>>> \t\tstd::string logPrefix() const override;\n>>>>\n>>>> diff --git a/src/libcamera/converter.cpp b/src/libcamera/converter.cpp\n>>>> index 2c3da6d4..d51e77a0 100644\n>>>> --- a/src/libcamera/converter.cpp\n>>>> +++ b/src/libcamera/converter.cpp\n>>>> @@ -11,6 +11,8 @@\n>>>>\n>>>> #include \n>>>>\n>>>> +#include \n>>>> +\n>>>> #include \"libcamera/internal/media_device.h\"\n>>>>\n>>>> /**\n>>>> @@ -39,6 +41,8 @@ LOG_DEFINE_CATEGORY(Converter)\n>>>> * \\brief Specify the features supported by the converter\n>>>> * \\var Converter::Feature::None\n>>>> * \\brief No extra features supported by the converter\n>>>> + * \\var Converter::Feature::Crop\n>>>> + * \\brief Cropping capability is supported by the converter\n>>>> */\n>>>>\n>>>> /**\n>>>> @@ -161,6 +165,53 @@ Converter::~Converter()\n>>>> * \\return 0 on success or a negative error code otherwise\n>>>> */\n>>>>\n>>>> +/**\n>>>> + * \\brief Set the crop rectangle \\a rect for \\a stream\n>>>> + * \\param[in] stream Pointer to output stream\n>>>> + * \\param[inout] rect The crop rectangle to be applied\n>>>> + *\n>>>> + * Set the crop rectangle \\a rect for \\a stream provided the converter supports\n>>>> + * cropping. The converter should have the Feature::Crop flag in this case.\n>>>> + *\n>>>> + * \\return 0 on success or a negative error code otherwise\n>>>> + */\n>>>> +int Converter::setCrop([[maybe_unused]] const Stream *stream, [[maybe_unused]] Rectangle *rect)\n>>>> +{\n>>>> +\tif (!(getFeatures() & Feature::Crop)) {\n>>>> +\t\tLOG(Converter, Error) << \"Converter doesn't support cropping capabilities\";\n>>>> +\t\treturn -ENOTSUP;\n>>>> +\t}\n>>>> +\n>>>> +\treturn 0;\n>>>> +}\n>>>> +\n>>>> +/**\n>>>> + * \\brief Get the crop bounds \\a stream\n>>>> + * \\param[in] stream Pointer to output stream\n>>>> + *\n>>>> + * Get the minimum and maximum crop bounds for \\a stream. The converter\n>>>> + * should supporting cropping (Feature::Crop).\n>>>> + *\n>>>> + * \\return A std::pair containining minimum and maximum\n>>>> + * crop bound respectively.\n>>>> + */\n>>>> +std::pair Converter::getCropBounds([[maybe_unused]] const Stream *stream)\n>>>> +{\n>>>> +\tconst StreamConfiguration &config = stream->configuration();\n>>>> +\tRectangle rect;\n>>>> +\n>>>> +\tif (!(getFeatures() & Feature::Crop))\n>>>> +\t\tLOG(Converter, Error) << \"Converter doesn't support cropping capabilities\";\n>>>> +\n>>>> +\t/*\n>>>> +\t * This is base implementation for the Converter class, so just return\n>>>> +\t * the stream configured size as minimum and maximum crop bounds.\n>>>> +\t */\n>>>> +\trect.size() = config.size;\n>>>> +\n>>>> +\treturn { rect, rect };\n>>> I would rather not make it part of the base class interface\n>> no? If the crop is a feature, getting the supported bounds would be\n>> something callers would like to know. Do you think this should be left only\n>> to derived converter classes ?\n> Only to the hierarchy of derived classes that supports it, like the\n> classes derived from V4L2M2MConverter (and the class itself). For some\n> other \"converters\" cropping and scaling might not make sense at all.\n>\n>>>> +}\n>>>> +\n>>>> /**\n>>>> * \\var Converter::inputBufferReady\n>>>> * \\brief A signal emitted when the input frame buffer completes\n>>>> diff --git a/src/libcamera/converter/converter_v4l2_m2m.cpp b/src/libcamera/converter/converter_v4l2_m2m.cpp\n>>>> index 4aeb7dd9..eaae3528 100644\n>>>> --- a/src/libcamera/converter/converter_v4l2_m2m.cpp\n>>>> +++ b/src/libcamera/converter/converter_v4l2_m2m.cpp\n>>>> @@ -155,6 +155,15 @@ int V4L2M2MConverter::V4L2M2MStream::queueBuffers(FrameBuffer *input, FrameBuffe\n>>>> \treturn 0;\n>>>> }\n>>>>\n>>>> +int V4L2M2MConverter::V4L2M2MStream::setSelection(unsigned int target, Rectangle *rect)\n>>>> +{\n>>>> +\tint ret = m2m_->output()->setSelection(target, rect);\n>>>> +\tif (ret < 0)\n>>>> +\t\treturn ret;\n>>>> +\n>>>> +\treturn 0;\n>>>> +}\n>>>> +\n>>>> std::string V4L2M2MConverter::V4L2M2MStream::logPrefix() const\n>>>> {\n>>>> \treturn stream_->configuration().toString();\n>>>> @@ -375,6 +384,81 @@ int V4L2M2MConverter::exportBuffers(const Stream *stream, unsigned int count,\n>>>> \treturn iter->second->exportBuffers(count, buffers);\n>>>> }\n>>>>\n>>>> +/**\n>>>> + * \\copydoc libcamera::Converter::setCrop\n>>>> + */\n>>>> +int V4L2M2MConverter::setCrop(const Stream *stream, Rectangle *rect)\n>>>> +{\n>>>> +\tif (!(getFeatures() & Feature::Crop))\n>>>> +\t\treturn -ENOTSUP;\n>>>> +\n>>>> +\tauto iter = streams_.find(stream);\n>>>> +\tif (iter == streams_.end())\n>>>> +\t\treturn -EINVAL;\n>>>> +\n>>>> +\treturn iter->second->setSelection(V4L2_SEL_TGT_CROP, rect);\n>>>> +}\n>>>> +\n>>>> +/**\n>>>> + * \\copydoc libcamera::Converter::getCropBounds\n>>>> + */\n>>>> +std::pair\n>>>> +V4L2M2MConverter::getCropBounds(const Stream *stream)\n>>>> +{\n>>>> +\tRectangle minCrop;\n>>>> +\tRectangle maxCrop;\n>>>> +\tint ret;\n>>>> +\n>>>> +\tminCrop.width = 1;\n>>>> +\tminCrop.height = 1;\n>>>> +\tmaxCrop.width = UINT_MAX;\n>>>> +\tmaxCrop.height = UINT_MAX;\n>>> You can move all of this after the check for the feature support\n>>>\n>>>> +\n>>>> +\tif (!(getFeatures() & Feature::Crop)) {\n>>>> +\t\tLOG(Converter, Error) << \"Crop functionality is not supported\";\n>>>> +\t\treturn {};\n>>>> +\t}\n>>>> +\n>>>> +\tauto iter = streams_.find(stream);\n>>>> +\tif (iter == streams_.end()) {\n>>>> +\t\t/*\n>>>> +\t\t * No streams configured, return minimum and maximum crop\n>>>> +\t\t * bounds at initialization.\n>>>> +\t\t */\n>>>> +\t\tret = m2m_->output()->setSelection(V4L2_SEL_TGT_CROP, &minCrop);\n>>>> +\t\tif (ret) {\n>>>> +\t\t\tLOG(Converter, Error) << \"Failed to query minimum crop bound\";\n>>>> +\t\t\treturn {};\n>>>> +\t\t}\n>>>> +\n>>>> +\t\tret = m2m_->output()->setSelection(V4L2_SEL_TGT_CROP, &maxCrop);\n>>>> +\t\tif (ret) {\n>>>> +\t\t\tLOG(Converter, Error) << \"Failed to query maximum crop bound\";\n>>>> +\t\t\treturn {};\n>>>> +\t\t}\n>>>> +\n>>>> +\t\treturn { minCrop, maxCrop };\n>>>> +\t}\n>>>> +\n>>>> +\t/*\n>>>> +\t * If the streams are configured, return bounds from according to\n>>>> +\t * stream configuration.\n>>>> +\t */\n>>>> +\tret = setCrop(stream, &minCrop);\n>>>> +\tif (ret) {\n>>>> +\t\tLOG(Converter, Error) << \"Failed to query minimum crop bound\";\n>>>> +\t\treturn {};\n>>>> +\t}\n>>>> +\n>>>> +\tret = setCrop(stream, &maxCrop);\n>>>> +\tif (ret) {\n>>>> +\t\tLOG(Converter, Error) << \"Failed to query maximum crop bound\";\n>>>> +\t\treturn {};\n>>>> +\t}\n>>>> +\n>>>> +\treturn { minCrop, maxCrop };\n>>>> +}\n>>>> +\n>>>> /**\n>>>> * \\copydoc libcamera::Converter::start\n>>>> */\n>>>> @@ -448,6 +532,10 @@ int V4L2M2MConverter::queueBuffers(FrameBuffer *input,\n>>>> \treturn 0;\n>>>> }\n>>>>\n>>>> +/*\n>>>> + * \\todo: This should be extended to include Feature::Flag to denote\n>>>> + * what each converter supports feature-wise.\n>>>> + */\n>>> That's another option.\n>>>\n>>> I'll send to the list the patches I have on top of your series that\n>>> makes it possible to specify features at registration time for a\n>>> comparison.\n>> Right , this bit is something I have to develop once I get a testing setup.\n>>\n>> The compatibles below are currently used for simple pipeline handler only.\n>>>> static std::initializer_list compatibles = {\n>>>> \t\"mtk-mdp\",\n>>>> \t\"pxp\",\n>>>> --\n>>>> 2.45.2\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 02A50BDB1C\n\tfor ;\n\tFri, 19 Jul 2024 10:01:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1A6F96336F;\n\tFri, 19 Jul 2024 12:01:54 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CDAC4619A7\n\tfor ;\n\tFri, 19 Jul 2024 12:01:52 +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 D779B471;\n\tFri, 19 Jul 2024 12:01:12 +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=\"AiatN6fR\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1721383273;\n\tbh=xg44JV3z1TOTRDU3NWP8CstW2EkBDXrx8Vo3nedujIw=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=AiatN6fRgPDig0AF/uvKAQVe4+lT9tvi4duJQo5Hqqgi2pwUXZoD33aCx+sXcgtCO\n\tp2x9QcahGLtEW4cpNYx4hlGHJZe1lo8q2wDOokstYB16ckCFShmdNS5Did3LFdI+kw\n\t0cVP70wfKuBnPCTcWPc2iOoWkUfi979DV18t+3GY=","Message-ID":"","Date":"Fri, 19 Jul 2024 15:31:48 +0530","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v5 2/5] libcamera: converter: Add interface to support\n\tcropping capability","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","References":"<20240709202151.152289-1-umang.jain@ideasonboard.com>\n\t<20240709202151.152289-3-umang.jain@ideasonboard.com>\n\t\n\t<92cdcc85-6897-40c7-a63c-d4c426e3e18e@ideasonboard.com>\n\t","Content-Language":"en-US","From":"Umang Jain ","In-Reply-To":"","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":30445,"web_url":"https://patchwork.libcamera.org/comment/30445/","msgid":"","date":"2024-07-22T09:32:22","subject":"Re: [PATCH v5 2/5] libcamera: converter: Add interface to support\n\tcropping capability","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Umang\n\nOn Fri, Jul 19, 2024 at 03:31:48PM GMT, Umang Jain wrote:\n> Hi Jacopo\n>\n> On 18/07/24 3:36 pm, Jacopo Mondi wrote:\n> > Hi Umang\n> >\n> > On Wed, Jul 17, 2024 at 04:34:34PM GMT, Umang Jain wrote:\n> > > Hi Jacopo\n> > >\n> > > On 17/07/24 3:32 pm, Jacopo Mondi wrote:\n> > > > Hi Umang\n> > > >\n> > > > On Wed, Jul 10, 2024 at 01:51:48AM GMT, Umang Jain wrote:\n> > > > > If the converter has cropping capability, the interface should support it\n> > > > > by providing appropriate virtual functions. Provide Feature::Crop in\n> > > > > Feature enumeration for the same.\n> > > > >\n> > > > > Provide virtual setCrop() and getCropBounds() interfaces so that\n> > > > > the converter can implement their own cropping functionality.\n> > > > >\n> > > > > The V4L2M2MConverter implements these interfaces of the Converter\n> > > > > interface. Not all V4L2M2M converters will have cropping capability,\n> > > > > hence Feature::Crop should be set while construction for all those\n> > > > > converters.\n> > > > >\n> > > > > Signed-off-by: Umang Jain \n> > > > > ---\n> > > > > include/libcamera/internal/converter.h | 5 ++\n> > > > > .../internal/converter/converter_v4l2_m2m.h | 6 ++\n> > > > > src/libcamera/converter.cpp | 51 +++++++++++\n> > > > > .../converter/converter_v4l2_m2m.cpp | 88 +++++++++++++++++++\n> > > > > 4 files changed, 150 insertions(+)\n> > > > >\n> > > > > diff --git a/include/libcamera/internal/converter.h b/include/libcamera/internal/converter.h\n> > > > > index 7e478356..9b1223fd 100644\n> > > > > --- a/include/libcamera/internal/converter.h\n> > > > > +++ b/include/libcamera/internal/converter.h\n> > > > > @@ -14,6 +14,7 @@\n> > > > > #include \n> > > > > #include \n> > > > > #include \n> > > > > +#include \n> > > > > #include \n> > > > >\n> > > > > #include \n> > > > > @@ -35,6 +36,7 @@ class Converter\n> > > > > public:\n> > > > > \tenum class Feature {\n> > > > > \t\tNone = 0,\n> > > > > +\t\tCrop = (1 << 0),\n> > > > > \t};\n> > > > >\n> > > > > \tusing Features = Flags;\n> > > > > @@ -63,6 +65,9 @@ public:\n> > > > > \tvirtual int queueBuffers(FrameBuffer *input,\n> > > > > \t\t\t\t const std::map &outputs) = 0;\n> > > > >\n> > > > > +\tvirtual int setCrop(const Stream *stream, Rectangle *rect);\n> > > > \"crop\" is loosely defined as a concept.\n> > > > A converter has an input and an output and I presume it scales.\n> > > >\n> > > > Where is the crop rectangle applied ?\n> > > A converter is a processing block - outside of ISP. Hence there is a input\n> > > and output, and the crop is applied at/w.r.t input. The resultant should be\n> > > obtained/visible on the output.\n> > >\n> > And where is the assumption that crop is applied to the input\n> > documented ? Aren't there converter that allow to crop on the output\n> > maybe -after- scaling ?\n> >\n> > Cropping before or after scaling (or, if you want to put it different,\n> > cropping on the input or on the output) are both possible options, and\n> > this interface doesn't allow you to specify that.\n>\n> cropping after scaling means the application would get a different image\n> output (if it tries to set crop on the output) ? Is that possible, a runtime\n> crop ? Image output size differs everytime a crop is applied ?\n\nI don't think runtime crops are possible neither on the 'output video\ndevice' or on the 'capture video device' as it would require changing\nthe buffer sizes at runtime. BUT my experience with m2m devices is\nlimited so..\n\nWhat I meant is slightly different. Given an input buffer size of WxH\nyour converter will output a frame of W'xH'\n\nTo get from W to W' and from H to H' you can combine cropping and\nscaling, theoretically freely. The canonical choice is to crop first\nthen scale, in order to maintain square pixels. In example if you have\nan 1080p input frame (16:9) and you want to get to a 640x480 (4:3)\noutput, if you only scale you will change the image proportions.\nGenerally it is good practice to crop down 1080p to the closest 4:3\nresolution, and then scale it down. So yes, \"setCrop()\" on the input\nis the most reasonable choice. Due to my limited knowledge of\nconverters, I was wondering if other combinations of cropping/scaling\nwould be possible, or if multiple cropping points might be available\nfor a converter and if a generic \"setCrop()\" interface is\nexpressive enough to support more complex devices.\n\nThe setCrop() interface you have designed is \"libcamera::Stream\" oriented,\nand a libcamera::Stream is associated in this class to a V4L2M2MStream\nwhich contains an \"input\" and an \"output\". As the class interface\nalready expresses the \"input\" and \"output\" concepts (in example in\nconfigure()) would it be an overkill to make it part of the setCrop()\ninterface as well ? (either with an INPUT/OUTPUT flag or by making\nthis a setInputCrop() function) ?\n\n>\n> I think what you are referring here is the compose rectangle, rather than\n> the crop rectangle? Is that correct?\n>\n> >\n> > You have wired the V4L2 converter to apply selection on the TGT_CROP\n> > target on the input queue (aka on the 'output' video device, oh funny\n> > names)\n> >\n> > int V4L2M2MConverter::setCrop(const Stream *stream, Rectangle *rect)\n> > {\n> >\n> > \treturn iter->second->setSelection(V4L2_SEL_TGT_CROP, rect);\n> > }\n> >\n> >\n> > int V4L2M2MConverter::V4L2M2MStream::setSelection(unsigned int target, Rectangle *rect)\n> > {\n> > \tint ret = m2m_->output()->setSelection(target, rect);\n> > \tif (ret < 0)\n> > \t\treturn ret;\n> >\n> > \treturn 0;\n> > }\n> >\n> > Can't a converter expose a TGT_CROP target on the capture device to allow\n> > userspace to apply a crop on the \"final\" image before returning it to\n> > applications ?\n>\n> So having read\n> https://docs.kernel.org/userspace-api/media/v4l/selection-api-intro.html, it\n> seems TGT_CROP can be target for video capture as well. There is no\n> limitation on that front.\n>\n> However, I can't wrap around my head around that kind of crop - applied at\n> runtime (like ScalerCrop) ?  For video capture, TGT_COMPOSE makes more sense\n> to me, rather than exposing TGT_CROP.\n\nThese are V4L2 specific concept. How a device expose selection targets\nand how they should be used I presume will be handled by the dedicated\nderived class.\n\nWhat matters for me at the moment is the generic Converter interface\ndesign.\n\n>\n> Certainly the interface is lacking such a distinction in this form - and I\n> should clarify that the crop would be set on the input video signal. Would\n> that makes things acceptable?\n>\n> I am also a bit wary about how should we be exposing such a interface for\n> converters, for output and video capture devices, have TGT_CROP and\n> TGT_COMPOSE targets ... its seems hairy for now, to me..\n>\n> > > > Does this need to be part of the base class interface ? Or should only\n> > > > derived classes that support it implement it ? (it's reasonable for a\n> > > > V4L2 M2M device to support this, once better specified the interface\n> > > > where the crop rectangle has to be applied).\n> > > I think so. Atleast that's what Laurent initally asked for. Converter\n> > > interface should dictate the features supported, the implementation left to\n> > > the derived converter classes. No implementation specific details should be\n> > > handled by the interface itself, only the \"feature\" part\n> > >\n> > Yes, but concepts like 'crop' and 'selection' might only apply to\n> > certain types of converter, like the v4l2 m2m ones. I wonder if it is\n> > better to provide a stub implementation in the whole hierarcy or only\n> > expose such methods in the classes derived from, in example,\n> > V4L2M2MConverter.\n> >\n> > > > > +\tvirtual std::pair getCropBounds(const Stream *stream);\n> > > > not \"get\" in getters\n> > > ah yes\n> > > > > +\n> > > > > \tSignal inputBufferReady;\n> > > > > \tSignal outputBufferReady;\n> > > > >\n> > > > > diff --git a/include/libcamera/internal/converter/converter_v4l2_m2m.h b/include/libcamera/internal/converter/converter_v4l2_m2m.h\n> > > > > index 91701dbe..2697eed9 100644\n> > > > > --- a/include/libcamera/internal/converter/converter_v4l2_m2m.h\n> > > > > +++ b/include/libcamera/internal/converter/converter_v4l2_m2m.h\n> > > > > @@ -30,6 +30,7 @@ class Size;\n> > > > > class SizeRange;\n> > > > > class Stream;\n> > > > > struct StreamConfiguration;\n> > > > > +class Rectangle;\n> > > > > class V4L2M2MDevice;\n> > > > >\n> > > > > class V4L2M2MConverter : public Converter\n> > > > > @@ -57,6 +58,9 @@ public:\n> > > > > \tint queueBuffers(FrameBuffer *input,\n> > > > > \t\t\t const std::map &outputs);\n> > > > >\n> > > > > +\tint setCrop(const Stream *stream, Rectangle *rect);\n> > > > > +\tstd::pair getCropBounds(const Stream *stream);\n> > > > > +\n> > > > > private:\n> > > > > \tclass V4L2M2MStream : protected Loggable\n> > > > > \t{\n> > > > > @@ -75,6 +79,8 @@ private:\n> > > > >\n> > > > > \t\tint queueBuffers(FrameBuffer *input, FrameBuffer *output);\n> > > > >\n> > > > > +\t\tint setSelection(unsigned int target, Rectangle *rect);\n> > > > > +\n> > > > > \tprotected:\n> > > > > \t\tstd::string logPrefix() const override;\n> > > > >\n> > > > > diff --git a/src/libcamera/converter.cpp b/src/libcamera/converter.cpp\n> > > > > index 2c3da6d4..d51e77a0 100644\n> > > > > --- a/src/libcamera/converter.cpp\n> > > > > +++ b/src/libcamera/converter.cpp\n> > > > > @@ -11,6 +11,8 @@\n> > > > >\n> > > > > #include \n> > > > >\n> > > > > +#include \n> > > > > +\n> > > > > #include \"libcamera/internal/media_device.h\"\n> > > > >\n> > > > > /**\n> > > > > @@ -39,6 +41,8 @@ LOG_DEFINE_CATEGORY(Converter)\n> > > > > * \\brief Specify the features supported by the converter\n> > > > > * \\var Converter::Feature::None\n> > > > > * \\brief No extra features supported by the converter\n> > > > > + * \\var Converter::Feature::Crop\n> > > > > + * \\brief Cropping capability is supported by the converter\n> > > > > */\n> > > > >\n> > > > > /**\n> > > > > @@ -161,6 +165,53 @@ Converter::~Converter()\n> > > > > * \\return 0 on success or a negative error code otherwise\n> > > > > */\n> > > > >\n> > > > > +/**\n> > > > > + * \\brief Set the crop rectangle \\a rect for \\a stream\n> > > > > + * \\param[in] stream Pointer to output stream\n> > > > > + * \\param[inout] rect The crop rectangle to be applied\n> > > > > + *\n> > > > > + * Set the crop rectangle \\a rect for \\a stream provided the converter supports\n> > > > > + * cropping. The converter should have the Feature::Crop flag in this case.\n> > > > > + *\n> > > > > + * \\return 0 on success or a negative error code otherwise\n> > > > > + */\n> > > > > +int Converter::setCrop([[maybe_unused]] const Stream *stream, [[maybe_unused]] Rectangle *rect)\n> > > > > +{\n> > > > > +\tif (!(getFeatures() & Feature::Crop)) {\n> > > > > +\t\tLOG(Converter, Error) << \"Converter doesn't support cropping capabilities\";\n> > > > > +\t\treturn -ENOTSUP;\n> > > > > +\t}\n> > > > > +\n> > > > > +\treturn 0;\n> > > > > +}\n> > > > > +\n> > > > > +/**\n> > > > > + * \\brief Get the crop bounds \\a stream\n> > > > > + * \\param[in] stream Pointer to output stream\n> > > > > + *\n> > > > > + * Get the minimum and maximum crop bounds for \\a stream. The converter\n> > > > > + * should supporting cropping (Feature::Crop).\n> > > > > + *\n> > > > > + * \\return A std::pair containining minimum and maximum\n> > > > > + * crop bound respectively.\n> > > > > + */\n> > > > > +std::pair Converter::getCropBounds([[maybe_unused]] const Stream *stream)\n> > > > > +{\n> > > > > +\tconst StreamConfiguration &config = stream->configuration();\n> > > > > +\tRectangle rect;\n> > > > > +\n> > > > > +\tif (!(getFeatures() & Feature::Crop))\n> > > > > +\t\tLOG(Converter, Error) << \"Converter doesn't support cropping capabilities\";\n> > > > > +\n> > > > > +\t/*\n> > > > > +\t * This is base implementation for the Converter class, so just return\n> > > > > +\t * the stream configured size as minimum and maximum crop bounds.\n> > > > > +\t */\n> > > > > +\trect.size() = config.size;\n> > > > > +\n> > > > > +\treturn { rect, rect };\n> > > > I would rather not make it part of the base class interface\n> > > no? If the crop is a feature, getting the supported bounds would be\n> > > something callers would like to know. Do you think this should be left only\n> > > to derived converter classes ?\n> > Only to the hierarchy of derived classes that supports it, like the\n> > classes derived from V4L2M2MConverter (and the class itself). For some\n> > other \"converters\" cropping and scaling might not make sense at all.\n> >\n> > > > > +}\n> > > > > +\n> > > > > /**\n> > > > > * \\var Converter::inputBufferReady\n> > > > > * \\brief A signal emitted when the input frame buffer completes\n> > > > > diff --git a/src/libcamera/converter/converter_v4l2_m2m.cpp b/src/libcamera/converter/converter_v4l2_m2m.cpp\n> > > > > index 4aeb7dd9..eaae3528 100644\n> > > > > --- a/src/libcamera/converter/converter_v4l2_m2m.cpp\n> > > > > +++ b/src/libcamera/converter/converter_v4l2_m2m.cpp\n> > > > > @@ -155,6 +155,15 @@ int V4L2M2MConverter::V4L2M2MStream::queueBuffers(FrameBuffer *input, FrameBuffe\n> > > > > \treturn 0;\n> > > > > }\n> > > > >\n> > > > > +int V4L2M2MConverter::V4L2M2MStream::setSelection(unsigned int target, Rectangle *rect)\n> > > > > +{\n> > > > > +\tint ret = m2m_->output()->setSelection(target, rect);\n> > > > > +\tif (ret < 0)\n> > > > > +\t\treturn ret;\n> > > > > +\n> > > > > +\treturn 0;\n> > > > > +}\n> > > > > +\n> > > > > std::string V4L2M2MConverter::V4L2M2MStream::logPrefix() const\n> > > > > {\n> > > > > \treturn stream_->configuration().toString();\n> > > > > @@ -375,6 +384,81 @@ int V4L2M2MConverter::exportBuffers(const Stream *stream, unsigned int count,\n> > > > > \treturn iter->second->exportBuffers(count, buffers);\n> > > > > }\n> > > > >\n> > > > > +/**\n> > > > > + * \\copydoc libcamera::Converter::setCrop\n> > > > > + */\n> > > > > +int V4L2M2MConverter::setCrop(const Stream *stream, Rectangle *rect)\n> > > > > +{\n> > > > > +\tif (!(getFeatures() & Feature::Crop))\n> > > > > +\t\treturn -ENOTSUP;\n> > > > > +\n> > > > > +\tauto iter = streams_.find(stream);\n> > > > > +\tif (iter == streams_.end())\n> > > > > +\t\treturn -EINVAL;\n> > > > > +\n> > > > > +\treturn iter->second->setSelection(V4L2_SEL_TGT_CROP, rect);\n> > > > > +}\n> > > > > +\n> > > > > +/**\n> > > > > + * \\copydoc libcamera::Converter::getCropBounds\n> > > > > + */\n> > > > > +std::pair\n> > > > > +V4L2M2MConverter::getCropBounds(const Stream *stream)\n> > > > > +{\n> > > > > +\tRectangle minCrop;\n> > > > > +\tRectangle maxCrop;\n> > > > > +\tint ret;\n> > > > > +\n> > > > > +\tminCrop.width = 1;\n> > > > > +\tminCrop.height = 1;\n> > > > > +\tmaxCrop.width = UINT_MAX;\n> > > > > +\tmaxCrop.height = UINT_MAX;\n> > > > You can move all of this after the check for the feature support\n> > > >\n> > > > > +\n> > > > > +\tif (!(getFeatures() & Feature::Crop)) {\n> > > > > +\t\tLOG(Converter, Error) << \"Crop functionality is not supported\";\n> > > > > +\t\treturn {};\n> > > > > +\t}\n> > > > > +\n> > > > > +\tauto iter = streams_.find(stream);\n> > > > > +\tif (iter == streams_.end()) {\n> > > > > +\t\t/*\n> > > > > +\t\t * No streams configured, return minimum and maximum crop\n> > > > > +\t\t * bounds at initialization.\n> > > > > +\t\t */\n> > > > > +\t\tret = m2m_->output()->setSelection(V4L2_SEL_TGT_CROP, &minCrop);\n> > > > > +\t\tif (ret) {\n> > > > > +\t\t\tLOG(Converter, Error) << \"Failed to query minimum crop bound\";\n> > > > > +\t\t\treturn {};\n> > > > > +\t\t}\n> > > > > +\n> > > > > +\t\tret = m2m_->output()->setSelection(V4L2_SEL_TGT_CROP, &maxCrop);\n> > > > > +\t\tif (ret) {\n> > > > > +\t\t\tLOG(Converter, Error) << \"Failed to query maximum crop bound\";\n> > > > > +\t\t\treturn {};\n> > > > > +\t\t}\n> > > > > +\n> > > > > +\t\treturn { minCrop, maxCrop };\n> > > > > +\t}\n> > > > > +\n> > > > > +\t/*\n> > > > > +\t * If the streams are configured, return bounds from according to\n> > > > > +\t * stream configuration.\n> > > > > +\t */\n> > > > > +\tret = setCrop(stream, &minCrop);\n> > > > > +\tif (ret) {\n> > > > > +\t\tLOG(Converter, Error) << \"Failed to query minimum crop bound\";\n> > > > > +\t\treturn {};\n> > > > > +\t}\n> > > > > +\n> > > > > +\tret = setCrop(stream, &maxCrop);\n> > > > > +\tif (ret) {\n> > > > > +\t\tLOG(Converter, Error) << \"Failed to query maximum crop bound\";\n> > > > > +\t\treturn {};\n> > > > > +\t}\n> > > > > +\n> > > > > +\treturn { minCrop, maxCrop };\n> > > > > +}\n> > > > > +\n> > > > > /**\n> > > > > * \\copydoc libcamera::Converter::start\n> > > > > */\n> > > > > @@ -448,6 +532,10 @@ int V4L2M2MConverter::queueBuffers(FrameBuffer *input,\n> > > > > \treturn 0;\n> > > > > }\n> > > > >\n> > > > > +/*\n> > > > > + * \\todo: This should be extended to include Feature::Flag to denote\n> > > > > + * what each converter supports feature-wise.\n> > > > > + */\n> > > > That's another option.\n> > > >\n> > > > I'll send to the list the patches I have on top of your series that\n> > > > makes it possible to specify features at registration time for a\n> > > > comparison.\n> > > Right , this bit is something I have to develop once I get a testing setup.\n> > >\n> > > The compatibles below are currently used for simple pipeline handler only.\n> > > > > static std::initializer_list compatibles = {\n> > > > > \t\"mtk-mdp\",\n> > > > > \t\"pxp\",\n> > > > > --\n> > > > > 2.45.2\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 8543BBDB1C\n\tfor ;\n\tMon, 22 Jul 2024 09:32:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 59B116336F;\n\tMon, 22 Jul 2024 11:32:30 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 02244619A0\n\tfor ;\n\tMon, 22 Jul 2024 11:32:28 +0200 (CEST)","from ideasonboard.com (mob-5-90-43-135.net.vodafone.it\n\t[5.90.43.135])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C2DC945B;\n\tMon, 22 Jul 2024 11:31:45 +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=\"Ksork3Xw\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1721640707;\n\tbh=BKrTzKfA4w60WVbgL71ffZsfGaFlUNRhR1LPgc4JeTo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Ksork3Xwp2EZBnthOdNTIEhOWryTXQue20eTjB9DOmD+Mhc6J2Fc7I4EzsbDzuejM\n\t4GaZilGCSYX+l5Srm0PwThS8L606m7glLh4zn2E+5FPdQqTfTAvOOdsKKvUITNi+pn\n\tv74Z1gUoobqmCfmckGeqy1gaHlppbCD7BZoB27ko=","Date":"Mon, 22 Jul 2024 11:32:22 +0200","From":"Jacopo Mondi ","To":"Umang Jain ","Cc":"Jacopo Mondi , \n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v5 2/5] libcamera: converter: Add interface to support\n\tcropping capability","Message-ID":"","References":"<20240709202151.152289-1-umang.jain@ideasonboard.com>\n\t<20240709202151.152289-3-umang.jain@ideasonboard.com>\n\t\n\t<92cdcc85-6897-40c7-a63c-d4c426e3e18e@ideasonboard.com>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","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":30446,"web_url":"https://patchwork.libcamera.org/comment/30446/","msgid":"","date":"2024-07-22T09:35:43","subject":"Re: [PATCH v5 2/5] libcamera: converter: Add interface to support\n\tcropping capability","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Umang,\n one smaller nit\n\nOn Wed, Jul 10, 2024 at 01:51:48AM GMT, Umang Jain wrote:\n> If the converter has cropping capability, the interface should support it\n> by providing appropriate virtual functions. Provide Feature::Crop in\n> Feature enumeration for the same.\n>\n> Provide virtual setCrop() and getCropBounds() interfaces so that\n> the converter can implement their own cropping functionality.\n>\n> The V4L2M2MConverter implements these interfaces of the Converter\n> interface. Not all V4L2M2M converters will have cropping capability,\n> hence Feature::Crop should be set while construction for all those\n> converters.\n>\n> Signed-off-by: Umang Jain \n> ---\n> include/libcamera/internal/converter.h | 5 ++\n> .../internal/converter/converter_v4l2_m2m.h | 6 ++\n> src/libcamera/converter.cpp | 51 +++++++++++\n> .../converter/converter_v4l2_m2m.cpp | 88 +++++++++++++++++++\n> 4 files changed, 150 insertions(+)\n>\n> diff --git a/include/libcamera/internal/converter.h b/include/libcamera/internal/converter.h\n> index 7e478356..9b1223fd 100644\n> --- a/include/libcamera/internal/converter.h\n> +++ b/include/libcamera/internal/converter.h\n> @@ -14,6 +14,7 @@\n> #include \n> #include \n> #include \n> +#include \n> #include \n>\n> #include \n> @@ -35,6 +36,7 @@ class Converter\n> public:\n> \tenum class Feature {\n> \t\tNone = 0,\n> +\t\tCrop = (1 << 0),\n> \t};\n>\n> \tusing Features = Flags;\n> @@ -63,6 +65,9 @@ public:\n> \tvirtual int queueBuffers(FrameBuffer *input,\n> \t\t\t\t const std::map &outputs) = 0;\n>\n> +\tvirtual int setCrop(const Stream *stream, Rectangle *rect);\n> +\tvirtual std::pair getCropBounds(const Stream *stream);\n> +\n> \tSignal inputBufferReady;\n> \tSignal outputBufferReady;\n>\n> diff --git a/include/libcamera/internal/converter/converter_v4l2_m2m.h b/include/libcamera/internal/converter/converter_v4l2_m2m.h\n> index 91701dbe..2697eed9 100644\n> --- a/include/libcamera/internal/converter/converter_v4l2_m2m.h\n> +++ b/include/libcamera/internal/converter/converter_v4l2_m2m.h\n> @@ -30,6 +30,7 @@ class Size;\n> class SizeRange;\n> class Stream;\n> struct StreamConfiguration;\n> +class Rectangle;\n> class V4L2M2MDevice;\n>\n> class V4L2M2MConverter : public Converter\n> @@ -57,6 +58,9 @@ public:\n> \tint queueBuffers(FrameBuffer *input,\n> \t\t\t const std::map &outputs);\n>\n> +\tint setCrop(const Stream *stream, Rectangle *rect);\n> +\tstd::pair getCropBounds(const Stream *stream);\n> +\n> private:\n> \tclass V4L2M2MStream : protected Loggable\n> \t{\n> @@ -75,6 +79,8 @@ private:\n>\n> \t\tint queueBuffers(FrameBuffer *input, FrameBuffer *output);\n>\n> +\t\tint setSelection(unsigned int target, Rectangle *rect);\n> +\n> \tprotected:\n> \t\tstd::string logPrefix() const override;\n>\n> diff --git a/src/libcamera/converter.cpp b/src/libcamera/converter.cpp\n> index 2c3da6d4..d51e77a0 100644\n> --- a/src/libcamera/converter.cpp\n> +++ b/src/libcamera/converter.cpp\n> @@ -11,6 +11,8 @@\n>\n> #include \n>\n> +#include \n> +\n> #include \"libcamera/internal/media_device.h\"\n>\n> /**\n> @@ -39,6 +41,8 @@ LOG_DEFINE_CATEGORY(Converter)\n> * \\brief Specify the features supported by the converter\n> * \\var Converter::Feature::None\n> * \\brief No extra features supported by the converter\n> + * \\var Converter::Feature::Crop\n> + * \\brief Cropping capability is supported by the converter\n> */\n>\n> /**\n> @@ -161,6 +165,53 @@ Converter::~Converter()\n> * \\return 0 on success or a negative error code otherwise\n> */\n>\n> +/**\n> + * \\brief Set the crop rectangle \\a rect for \\a stream\n> + * \\param[in] stream Pointer to output stream\n> + * \\param[inout] rect The crop rectangle to be applied\n> + *\n> + * Set the crop rectangle \\a rect for \\a stream provided the converter supports\n> + * cropping. The converter should have the Feature::Crop flag in this case.\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n> + */\n> +int Converter::setCrop([[maybe_unused]] const Stream *stream, [[maybe_unused]] Rectangle *rect)\n> +{\n> +\tif (!(getFeatures() & Feature::Crop)) {\n> +\t\tLOG(Converter, Error) << \"Converter doesn't support cropping capabilities\";\n> +\t\treturn -ENOTSUP;\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> +/**\n> + * \\brief Get the crop bounds \\a stream\n> + * \\param[in] stream Pointer to output stream\n> + *\n> + * Get the minimum and maximum crop bounds for \\a stream. The converter\n> + * should supporting cropping (Feature::Crop).\n> + *\n> + * \\return A std::pair containining minimum and maximum\n> + * crop bound respectively.\n> + */\n> +std::pair Converter::getCropBounds([[maybe_unused]] const Stream *stream)\n> +{\n> +\tconst StreamConfiguration &config = stream->configuration();\n> +\tRectangle rect;\n> +\n> +\tif (!(getFeatures() & Feature::Crop))\n> +\t\tLOG(Converter, Error) << \"Converter doesn't support cropping capabilities\";\n> +\n> +\t/*\n> +\t * This is base implementation for the Converter class, so just return\n> +\t * the stream configured size as minimum and maximum crop bounds.\n> +\t */\n> +\trect.size() = config.size;\n> +\n> +\treturn { rect, rect };\n> +}\n> +\n> /**\n> * \\var Converter::inputBufferReady\n> * \\brief A signal emitted when the input frame buffer completes\n> diff --git a/src/libcamera/converter/converter_v4l2_m2m.cpp b/src/libcamera/converter/converter_v4l2_m2m.cpp\n> index 4aeb7dd9..eaae3528 100644\n> --- a/src/libcamera/converter/converter_v4l2_m2m.cpp\n> +++ b/src/libcamera/converter/converter_v4l2_m2m.cpp\n> @@ -155,6 +155,15 @@ int V4L2M2MConverter::V4L2M2MStream::queueBuffers(FrameBuffer *input, FrameBuffe\n> \treturn 0;\n> }\n>\n> +int V4L2M2MConverter::V4L2M2MStream::setSelection(unsigned int target, Rectangle *rect)\n> +{\n> +\tint ret = m2m_->output()->setSelection(target, rect);\n\nYou can just\n return m2m_->output()->setSelection(target, rect);\n\nas\n\n * \\return 0 on success or a negative error code otherwise\n */\nint V4L2VideoDevice::setSelection(unsigned int target, Rectangle *rect)\n\n\n> +\tif (ret < 0)\n> +\t\treturn ret;\n> +\n> +\treturn 0;\n> +}\n> +\n> std::string V4L2M2MConverter::V4L2M2MStream::logPrefix() const\n> {\n> \treturn stream_->configuration().toString();\n> @@ -375,6 +384,81 @@ int V4L2M2MConverter::exportBuffers(const Stream *stream, unsigned int count,\n> \treturn iter->second->exportBuffers(count, buffers);\n> }\n>\n> +/**\n> + * \\copydoc libcamera::Converter::setCrop\n> + */\n> +int V4L2M2MConverter::setCrop(const Stream *stream, Rectangle *rect)\n> +{\n> +\tif (!(getFeatures() & Feature::Crop))\n> +\t\treturn -ENOTSUP;\n> +\n> +\tauto iter = streams_.find(stream);\n> +\tif (iter == streams_.end())\n> +\t\treturn -EINVAL;\n> +\n> +\treturn iter->second->setSelection(V4L2_SEL_TGT_CROP, rect);\n> +}\n> +\n> +/**\n> + * \\copydoc libcamera::Converter::getCropBounds\n> + */\n> +std::pair\n> +V4L2M2MConverter::getCropBounds(const Stream *stream)\n> +{\n> +\tRectangle minCrop;\n> +\tRectangle maxCrop;\n> +\tint ret;\n> +\n> +\tminCrop.width = 1;\n> +\tminCrop.height = 1;\n> +\tmaxCrop.width = UINT_MAX;\n> +\tmaxCrop.height = UINT_MAX;\n> +\n> +\tif (!(getFeatures() & Feature::Crop)) {\n> +\t\tLOG(Converter, Error) << \"Crop functionality is not supported\";\n> +\t\treturn {};\n> +\t}\n> +\n> +\tauto iter = streams_.find(stream);\n> +\tif (iter == streams_.end()) {\n> +\t\t/*\n> +\t\t * No streams configured, return minimum and maximum crop\n> +\t\t * bounds at initialization.\n> +\t\t */\n> +\t\tret = m2m_->output()->setSelection(V4L2_SEL_TGT_CROP, &minCrop);\n> +\t\tif (ret) {\n> +\t\t\tLOG(Converter, Error) << \"Failed to query minimum crop bound\";\n> +\t\t\treturn {};\n> +\t\t}\n> +\n> +\t\tret = m2m_->output()->setSelection(V4L2_SEL_TGT_CROP, &maxCrop);\n> +\t\tif (ret) {\n> +\t\t\tLOG(Converter, Error) << \"Failed to query maximum crop bound\";\n> +\t\t\treturn {};\n> +\t\t}\n> +\n> +\t\treturn { minCrop, maxCrop };\n> +\t}\n> +\n> +\t/*\n> +\t * If the streams are configured, return bounds from according to\n> +\t * stream configuration.\n> +\t */\n> +\tret = setCrop(stream, &minCrop);\n> +\tif (ret) {\n> +\t\tLOG(Converter, Error) << \"Failed to query minimum crop bound\";\n> +\t\treturn {};\n> +\t}\n> +\n> +\tret = setCrop(stream, &maxCrop);\n> +\tif (ret) {\n> +\t\tLOG(Converter, Error) << \"Failed to query maximum crop bound\";\n> +\t\treturn {};\n> +\t}\n> +\n> +\treturn { minCrop, maxCrop };\n> +}\n> +\n> /**\n> * \\copydoc libcamera::Converter::start\n> */\n> @@ -448,6 +532,10 @@ int V4L2M2MConverter::queueBuffers(FrameBuffer *input,\n> \treturn 0;\n> }\n>\n> +/*\n> + * \\todo: This should be extended to include Feature::Flag to denote\n> + * what each converter supports feature-wise.\n> + */\n> static std::initializer_list compatibles = {\n> \t\"mtk-mdp\",\n> \t\"pxp\",\n> --\n> 2.45.2\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 250C2C323E\n\tfor ;\n\tMon, 22 Jul 2024 09:35:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E3DAB6336F;\n\tMon, 22 Jul 2024 11:35:50 +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 B1455619A0\n\tfor ;\n\tMon, 22 Jul 2024 11:35:48 +0200 (CEST)","from ideasonboard.com (mob-5-90-43-135.net.vodafone.it\n\t[5.90.43.135])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5146C45B;\n\tMon, 22 Jul 2024 11:35:06 +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=\"vgNxi0ti\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1721640907;\n\tbh=6pcm7X2lJzdZoCKtVkqSoxeY08Xoxss/zGmvbb9cnYA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=vgNxi0tixLq2zzezOwx/0bo4KORv90TDPs3r1rf055kVD2CVl8z1/ZI98wx1nv489\n\tddwj8UJF7AtZmTg0APLykYNUH9jpXQQh4RS72L2zx/NMovGIOqaK+W1GuxesdsLbSY\n\tjF27Oxo0zfQ3Vo6/Tc4+LGstB5iVDlK5MlhPjNl8=","Date":"Mon, 22 Jul 2024 11:35:43 +0200","From":"Jacopo Mondi ","To":"Umang Jain ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v5 2/5] libcamera: converter: Add interface to support\n\tcropping capability","Message-ID":"","References":"<20240709202151.152289-1-umang.jain@ideasonboard.com>\n\t<20240709202151.152289-3-umang.jain@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240709202151.152289-3-umang.jain@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\" "}},{"id":30450,"web_url":"https://patchwork.libcamera.org/comment/30450/","msgid":"","date":"2024-07-23T06:16:48","subject":"Re: [PATCH v5 2/5] libcamera: converter: Add interface to support\n\tcropping capability","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Jacopo\n\nOn 22/07/24 3:02 pm, Jacopo Mondi wrote:\n> Hi Umang\n>\n> On Fri, Jul 19, 2024 at 03:31:48PM GMT, Umang Jain wrote:\n>> Hi Jacopo\n>>\n>> On 18/07/24 3:36 pm, Jacopo Mondi wrote:\n>>> Hi Umang\n>>>\n>>> On Wed, Jul 17, 2024 at 04:34:34PM GMT, Umang Jain wrote:\n>>>> Hi Jacopo\n>>>>\n>>>> On 17/07/24 3:32 pm, Jacopo Mondi wrote:\n>>>>> Hi Umang\n>>>>>\n>>>>> On Wed, Jul 10, 2024 at 01:51:48AM GMT, Umang Jain wrote:\n>>>>>> If the converter has cropping capability, the interface should support it\n>>>>>> by providing appropriate virtual functions. Provide Feature::Crop in\n>>>>>> Feature enumeration for the same.\n>>>>>>\n>>>>>> Provide virtual setCrop() and getCropBounds() interfaces so that\n>>>>>> the converter can implement their own cropping functionality.\n>>>>>>\n>>>>>> The V4L2M2MConverter implements these interfaces of the Converter\n>>>>>> interface. Not all V4L2M2M converters will have cropping capability,\n>>>>>> hence Feature::Crop should be set while construction for all those\n>>>>>> converters.\n>>>>>>\n>>>>>> Signed-off-by: Umang Jain \n>>>>>> ---\n>>>>>> include/libcamera/internal/converter.h | 5 ++\n>>>>>> .../internal/converter/converter_v4l2_m2m.h | 6 ++\n>>>>>> src/libcamera/converter.cpp | 51 +++++++++++\n>>>>>> .../converter/converter_v4l2_m2m.cpp | 88 +++++++++++++++++++\n>>>>>> 4 files changed, 150 insertions(+)\n>>>>>>\n>>>>>> diff --git a/include/libcamera/internal/converter.h b/include/libcamera/internal/converter.h\n>>>>>> index 7e478356..9b1223fd 100644\n>>>>>> --- a/include/libcamera/internal/converter.h\n>>>>>> +++ b/include/libcamera/internal/converter.h\n>>>>>> @@ -14,6 +14,7 @@\n>>>>>> #include \n>>>>>> #include \n>>>>>> #include \n>>>>>> +#include \n>>>>>> #include \n>>>>>>\n>>>>>> #include \n>>>>>> @@ -35,6 +36,7 @@ class Converter\n>>>>>> public:\n>>>>>> \tenum class Feature {\n>>>>>> \t\tNone = 0,\n>>>>>> +\t\tCrop = (1 << 0),\n>>>>>> \t};\n>>>>>>\n>>>>>> \tusing Features = Flags;\n>>>>>> @@ -63,6 +65,9 @@ public:\n>>>>>> \tvirtual int queueBuffers(FrameBuffer *input,\n>>>>>> \t\t\t\t const std::map &outputs) = 0;\n>>>>>>\n>>>>>> +\tvirtual int setCrop(const Stream *stream, Rectangle *rect);\n>>>>> \"crop\" is loosely defined as a concept.\n>>>>> A converter has an input and an output and I presume it scales.\n>>>>>\n>>>>> Where is the crop rectangle applied ?\n>>>> A converter is a processing block - outside of ISP. Hence there is a input\n>>>> and output, and the crop is applied at/w.r.t input. The resultant should be\n>>>> obtained/visible on the output.\n>>>>\n>>> And where is the assumption that crop is applied to the input\n>>> documented ? Aren't there converter that allow to crop on the output\n>>> maybe -after- scaling ?\n>>>\n>>> Cropping before or after scaling (or, if you want to put it different,\n>>> cropping on the input or on the output) are both possible options, and\n>>> this interface doesn't allow you to specify that.\n>> cropping after scaling means the application would get a different image\n>> output (if it tries to set crop on the output) ? Is that possible, a runtime\n>> crop ? Image output size differs everytime a crop is applied ?\n> I don't think runtime crops are possible neither on the 'output video\n> device' or on the 'capture video device' as it would require changing\n> the buffer sizes at runtime. BUT my experience with m2m devices is\n> limited so..\n>\n> What I meant is slightly different. Given an input buffer size of WxH\n> your converter will output a frame of W'xH'\n>\n> To get from W to W' and from H to H' you can combine cropping and\n> scaling, theoretically freely. The canonical choice is to crop first\n> then scale, in order to maintain square pixels. In example if you have\n> an 1080p input frame (16:9) and you want to get to a 640x480 (4:3)\n> output, if you only scale you will change the image proportions.\n> Generally it is good practice to crop down 1080p to the closest 4:3\n> resolution, and then scale it down. So yes, \"setCrop()\" on the input\n> is the most reasonable choice. Due to my limited knowledge of\n> converters, I was wondering if other combinations of cropping/scaling\n> would be possible, or if multiple cropping points might be available\n> for a converter and if a generic \"setCrop()\" interface is\n> expressive enough to support more complex devices.\n>\n> The setCrop() interface you have designed is \"libcamera::Stream\" oriented,\n> and a libcamera::Stream is associated in this class to a V4L2M2MStream\n> which contains an \"input\" and an \"output\". As the class interface\n> already expresses the \"input\" and \"output\" concepts (in example in\n> configure()) would it be an overkill to make it part of the setCrop()\n> interface as well ? (either with an INPUT/OUTPUT flag or by making\n> this a setInputCrop() function) ?\n>\n>> I think what you are referring here is the compose rectangle, rather than\n>> the crop rectangle? Is that correct?\n>>\n>>> You have wired the V4L2 converter to apply selection on the TGT_CROP\n>>> target on the input queue (aka on the 'output' video device, oh funny\n>>> names)\n>>>\n>>> int V4L2M2MConverter::setCrop(const Stream *stream, Rectangle *rect)\n>>> {\n>>>\n>>> \treturn iter->second->setSelection(V4L2_SEL_TGT_CROP, rect);\n>>> }\n>>>\n>>>\n>>> int V4L2M2MConverter::V4L2M2MStream::setSelection(unsigned int target, Rectangle *rect)\n>>> {\n>>> \tint ret = m2m_->output()->setSelection(target, rect);\n>>> \tif (ret < 0)\n>>> \t\treturn ret;\n>>>\n>>> \treturn 0;\n>>> }\n>>>\n>>> Can't a converter expose a TGT_CROP target on the capture device to allow\n>>> userspace to apply a crop on the \"final\" image before returning it to\n>>> applications ?\n>> So having read\n>> https://docs.kernel.org/userspace-api/media/v4l/selection-api-intro.html, it\n>> seems TGT_CROP can be target for video capture as well. There is no\n>> limitation on that front.\n>>\n>> However, I can't wrap around my head around that kind of crop - applied at\n>> runtime (like ScalerCrop) ?  For video capture, TGT_COMPOSE makes more sense\n>> to me, rather than exposing TGT_CROP.\n> These are V4L2 specific concept. How a device expose selection targets\n> and how they should be used I presume will be handled by the dedicated\n> derived class.\n>\n> What matters for me at the moment is the generic Converter interface\n> design.\n\nMakes sense.\n\nI will make the setCrop() as setInputCrop() in the generic Converter \ninterface.\nAnd will also document as the Crop set on the input stream to the \nconverter. Hopefully, this will help resolve ambiguity.\n\n\nThe configure() has a Input stream and no. of output streams.. So \nsetInputCrop() will corroborate to the input stream, used while \nconfiguring the converter.\n\n>\n>> Certainly the interface is lacking such a distinction in this form - and I\n>> should clarify that the crop would be set on the input video signal. Would\n>> that makes things acceptable?\n>>\n>> I am also a bit wary about how should we be exposing such a interface for\n>> converters, for output and video capture devices, have TGT_CROP and\n>> TGT_COMPOSE targets ... its seems hairy for now, to me..\n>>\n>>>>> Does this need to be part of the base class interface ? Or should only\n>>>>> derived classes that support it implement it ? (it's reasonable for a\n>>>>> V4L2 M2M device to support this, once better specified the interface\n>>>>> where the crop rectangle has to be applied).\n>>>> I think so. Atleast that's what Laurent initally asked for. Converter\n>>>> interface should dictate the features supported, the implementation left to\n>>>> the derived converter classes. No implementation specific details should be\n>>>> handled by the interface itself, only the \"feature\" part\n>>>>\n>>> Yes, but concepts like 'crop' and 'selection' might only apply to\n>>> certain types of converter, like the v4l2 m2m ones. I wonder if it is\n>>> better to provide a stub implementation in the whole hierarcy or only\n>>> expose such methods in the classes derived from, in example,\n>>> V4L2M2MConverter.\n>>>\n>>>>>> +\tvirtual std::pair getCropBounds(const Stream *stream);\n>>>>> not \"get\" in getters\n>>>> ah yes\n>>>>>> +\n>>>>>> \tSignal inputBufferReady;\n>>>>>> \tSignal outputBufferReady;\n>>>>>>\n>>>>>> diff --git a/include/libcamera/internal/converter/converter_v4l2_m2m.h b/include/libcamera/internal/converter/converter_v4l2_m2m.h\n>>>>>> index 91701dbe..2697eed9 100644\n>>>>>> --- a/include/libcamera/internal/converter/converter_v4l2_m2m.h\n>>>>>> +++ b/include/libcamera/internal/converter/converter_v4l2_m2m.h\n>>>>>> @@ -30,6 +30,7 @@ class Size;\n>>>>>> class SizeRange;\n>>>>>> class Stream;\n>>>>>> struct StreamConfiguration;\n>>>>>> +class Rectangle;\n>>>>>> class V4L2M2MDevice;\n>>>>>>\n>>>>>> class V4L2M2MConverter : public Converter\n>>>>>> @@ -57,6 +58,9 @@ public:\n>>>>>> \tint queueBuffers(FrameBuffer *input,\n>>>>>> \t\t\t const std::map &outputs);\n>>>>>>\n>>>>>> +\tint setCrop(const Stream *stream, Rectangle *rect);\n>>>>>> +\tstd::pair getCropBounds(const Stream *stream);\n>>>>>> +\n>>>>>> private:\n>>>>>> \tclass V4L2M2MStream : protected Loggable\n>>>>>> \t{\n>>>>>> @@ -75,6 +79,8 @@ private:\n>>>>>>\n>>>>>> \t\tint queueBuffers(FrameBuffer *input, FrameBuffer *output);\n>>>>>>\n>>>>>> +\t\tint setSelection(unsigned int target, Rectangle *rect);\n>>>>>> +\n>>>>>> \tprotected:\n>>>>>> \t\tstd::string logPrefix() const override;\n>>>>>>\n>>>>>> diff --git a/src/libcamera/converter.cpp b/src/libcamera/converter.cpp\n>>>>>> index 2c3da6d4..d51e77a0 100644\n>>>>>> --- a/src/libcamera/converter.cpp\n>>>>>> +++ b/src/libcamera/converter.cpp\n>>>>>> @@ -11,6 +11,8 @@\n>>>>>>\n>>>>>> #include \n>>>>>>\n>>>>>> +#include \n>>>>>> +\n>>>>>> #include \"libcamera/internal/media_device.h\"\n>>>>>>\n>>>>>> /**\n>>>>>> @@ -39,6 +41,8 @@ LOG_DEFINE_CATEGORY(Converter)\n>>>>>> * \\brief Specify the features supported by the converter\n>>>>>> * \\var Converter::Feature::None\n>>>>>> * \\brief No extra features supported by the converter\n>>>>>> + * \\var Converter::Feature::Crop\n>>>>>> + * \\brief Cropping capability is supported by the converter\n>>>>>> */\n>>>>>>\n>>>>>> /**\n>>>>>> @@ -161,6 +165,53 @@ Converter::~Converter()\n>>>>>> * \\return 0 on success or a negative error code otherwise\n>>>>>> */\n>>>>>>\n>>>>>> +/**\n>>>>>> + * \\brief Set the crop rectangle \\a rect for \\a stream\n>>>>>> + * \\param[in] stream Pointer to output stream\n>>>>>> + * \\param[inout] rect The crop rectangle to be applied\n>>>>>> + *\n>>>>>> + * Set the crop rectangle \\a rect for \\a stream provided the converter supports\n>>>>>> + * cropping. The converter should have the Feature::Crop flag in this case.\n>>>>>> + *\n>>>>>> + * \\return 0 on success or a negative error code otherwise\n>>>>>> + */\n>>>>>> +int Converter::setCrop([[maybe_unused]] const Stream *stream, [[maybe_unused]] Rectangle *rect)\n>>>>>> +{\n>>>>>> +\tif (!(getFeatures() & Feature::Crop)) {\n>>>>>> +\t\tLOG(Converter, Error) << \"Converter doesn't support cropping capabilities\";\n>>>>>> +\t\treturn -ENOTSUP;\n>>>>>> +\t}\n>>>>>> +\n>>>>>> +\treturn 0;\n>>>>>> +}\n>>>>>> +\n>>>>>> +/**\n>>>>>> + * \\brief Get the crop bounds \\a stream\n>>>>>> + * \\param[in] stream Pointer to output stream\n>>>>>> + *\n>>>>>> + * Get the minimum and maximum crop bounds for \\a stream. The converter\n>>>>>> + * should supporting cropping (Feature::Crop).\n>>>>>> + *\n>>>>>> + * \\return A std::pair containining minimum and maximum\n>>>>>> + * crop bound respectively.\n>>>>>> + */\n>>>>>> +std::pair Converter::getCropBounds([[maybe_unused]] const Stream *stream)\n>>>>>> +{\n>>>>>> +\tconst StreamConfiguration &config = stream->configuration();\n>>>>>> +\tRectangle rect;\n>>>>>> +\n>>>>>> +\tif (!(getFeatures() & Feature::Crop))\n>>>>>> +\t\tLOG(Converter, Error) << \"Converter doesn't support cropping capabilities\";\n>>>>>> +\n>>>>>> +\t/*\n>>>>>> +\t * This is base implementation for the Converter class, so just return\n>>>>>> +\t * the stream configured size as minimum and maximum crop bounds.\n>>>>>> +\t */\n>>>>>> +\trect.size() = config.size;\n>>>>>> +\n>>>>>> +\treturn { rect, rect };\n>>>>> I would rather not make it part of the base class interface\n>>>> no? If the crop is a feature, getting the supported bounds would be\n>>>> something callers would like to know. Do you think this should be left only\n>>>> to derived converter classes ?\n>>> Only to the hierarchy of derived classes that supports it, like the\n>>> classes derived from V4L2M2MConverter (and the class itself). For some\n>>> other \"converters\" cropping and scaling might not make sense at all.\n>>>\n>>>>>> +}\n>>>>>> +\n>>>>>> /**\n>>>>>> * \\var Converter::inputBufferReady\n>>>>>> * \\brief A signal emitted when the input frame buffer completes\n>>>>>> diff --git a/src/libcamera/converter/converter_v4l2_m2m.cpp b/src/libcamera/converter/converter_v4l2_m2m.cpp\n>>>>>> index 4aeb7dd9..eaae3528 100644\n>>>>>> --- a/src/libcamera/converter/converter_v4l2_m2m.cpp\n>>>>>> +++ b/src/libcamera/converter/converter_v4l2_m2m.cpp\n>>>>>> @@ -155,6 +155,15 @@ int V4L2M2MConverter::V4L2M2MStream::queueBuffers(FrameBuffer *input, FrameBuffe\n>>>>>> \treturn 0;\n>>>>>> }\n>>>>>>\n>>>>>> +int V4L2M2MConverter::V4L2M2MStream::setSelection(unsigned int target, Rectangle *rect)\n>>>>>> +{\n>>>>>> +\tint ret = m2m_->output()->setSelection(target, rect);\n>>>>>> +\tif (ret < 0)\n>>>>>> +\t\treturn ret;\n>>>>>> +\n>>>>>> +\treturn 0;\n>>>>>> +}\n>>>>>> +\n>>>>>> std::string V4L2M2MConverter::V4L2M2MStream::logPrefix() const\n>>>>>> {\n>>>>>> \treturn stream_->configuration().toString();\n>>>>>> @@ -375,6 +384,81 @@ int V4L2M2MConverter::exportBuffers(const Stream *stream, unsigned int count,\n>>>>>> \treturn iter->second->exportBuffers(count, buffers);\n>>>>>> }\n>>>>>>\n>>>>>> +/**\n>>>>>> + * \\copydoc libcamera::Converter::setCrop\n>>>>>> + */\n>>>>>> +int V4L2M2MConverter::setCrop(const Stream *stream, Rectangle *rect)\n>>>>>> +{\n>>>>>> +\tif (!(getFeatures() & Feature::Crop))\n>>>>>> +\t\treturn -ENOTSUP;\n>>>>>> +\n>>>>>> +\tauto iter = streams_.find(stream);\n>>>>>> +\tif (iter == streams_.end())\n>>>>>> +\t\treturn -EINVAL;\n>>>>>> +\n>>>>>> +\treturn iter->second->setSelection(V4L2_SEL_TGT_CROP, rect);\n>>>>>> +}\n>>>>>> +\n>>>>>> +/**\n>>>>>> + * \\copydoc libcamera::Converter::getCropBounds\n>>>>>> + */\n>>>>>> +std::pair\n>>>>>> +V4L2M2MConverter::getCropBounds(const Stream *stream)\n>>>>>> +{\n>>>>>> +\tRectangle minCrop;\n>>>>>> +\tRectangle maxCrop;\n>>>>>> +\tint ret;\n>>>>>> +\n>>>>>> +\tminCrop.width = 1;\n>>>>>> +\tminCrop.height = 1;\n>>>>>> +\tmaxCrop.width = UINT_MAX;\n>>>>>> +\tmaxCrop.height = UINT_MAX;\n>>>>> You can move all of this after the check for the feature support\n>>>>>\n>>>>>> +\n>>>>>> +\tif (!(getFeatures() & Feature::Crop)) {\n>>>>>> +\t\tLOG(Converter, Error) << \"Crop functionality is not supported\";\n>>>>>> +\t\treturn {};\n>>>>>> +\t}\n>>>>>> +\n>>>>>> +\tauto iter = streams_.find(stream);\n>>>>>> +\tif (iter == streams_.end()) {\n>>>>>> +\t\t/*\n>>>>>> +\t\t * No streams configured, return minimum and maximum crop\n>>>>>> +\t\t * bounds at initialization.\n>>>>>> +\t\t */\n>>>>>> +\t\tret = m2m_->output()->setSelection(V4L2_SEL_TGT_CROP, &minCrop);\n>>>>>> +\t\tif (ret) {\n>>>>>> +\t\t\tLOG(Converter, Error) << \"Failed to query minimum crop bound\";\n>>>>>> +\t\t\treturn {};\n>>>>>> +\t\t}\n>>>>>> +\n>>>>>> +\t\tret = m2m_->output()->setSelection(V4L2_SEL_TGT_CROP, &maxCrop);\n>>>>>> +\t\tif (ret) {\n>>>>>> +\t\t\tLOG(Converter, Error) << \"Failed to query maximum crop bound\";\n>>>>>> +\t\t\treturn {};\n>>>>>> +\t\t}\n>>>>>> +\n>>>>>> +\t\treturn { minCrop, maxCrop };\n>>>>>> +\t}\n>>>>>> +\n>>>>>> +\t/*\n>>>>>> +\t * If the streams are configured, return bounds from according to\n>>>>>> +\t * stream configuration.\n>>>>>> +\t */\n>>>>>> +\tret = setCrop(stream, &minCrop);\n>>>>>> +\tif (ret) {\n>>>>>> +\t\tLOG(Converter, Error) << \"Failed to query minimum crop bound\";\n>>>>>> +\t\treturn {};\n>>>>>> +\t}\n>>>>>> +\n>>>>>> +\tret = setCrop(stream, &maxCrop);\n>>>>>> +\tif (ret) {\n>>>>>> +\t\tLOG(Converter, Error) << \"Failed to query maximum crop bound\";\n>>>>>> +\t\treturn {};\n>>>>>> +\t}\n>>>>>> +\n>>>>>> +\treturn { minCrop, maxCrop };\n>>>>>> +}\n>>>>>> +\n>>>>>> /**\n>>>>>> * \\copydoc libcamera::Converter::start\n>>>>>> */\n>>>>>> @@ -448,6 +532,10 @@ int V4L2M2MConverter::queueBuffers(FrameBuffer *input,\n>>>>>> \treturn 0;\n>>>>>> }\n>>>>>>\n>>>>>> +/*\n>>>>>> + * \\todo: This should be extended to include Feature::Flag to denote\n>>>>>> + * what each converter supports feature-wise.\n>>>>>> + */\n>>>>> That's another option.\n>>>>>\n>>>>> I'll send to the list the patches I have on top of your series that\n>>>>> makes it possible to specify features at registration time for a\n>>>>> comparison.\n>>>> Right , this bit is something I have to develop once I get a testing setup.\n>>>>\n>>>> The compatibles below are currently used for simple pipeline handler only.\n>>>>>> static std::initializer_list compatibles = {\n>>>>>> \t\"mtk-mdp\",\n>>>>>> \t\"pxp\",\n>>>>>> --\n>>>>>> 2.45.2\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 49A24BDB1C\n\tfor ;\n\tTue, 23 Jul 2024 06:16:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1E4A86336F;\n\tTue, 23 Jul 2024 08:16:54 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 67A0E61991\n\tfor ;\n\tTue, 23 Jul 2024 08:16:52 +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 CB43963C;\n\tTue, 23 Jul 2024 08:16:09 +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=\"msflm+7m\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1721715370;\n\tbh=5uePtL3A8LXjhp74t1VbT5D4SqOSFrVHLzfd9rvtMT0=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=msflm+7myuRlx7UWqH9hCEo8UxUthgUSHAe1UsmTeqfUGKvCblId9Osw4UEdQC5vX\n\tgpXbgjz7Mezy/ruomtIxxsdD1hxkVWp+xTeu+Qk1raOVeJ4i21XiLcTz5Oq3QJhUx3\n\tEw52DCEY/WLFkzOJ0TX4vPmQTtosu1sfpv+Ll4rE=","Message-ID":"","Date":"Tue, 23 Jul 2024 11:46:48 +0530","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v5 2/5] libcamera: converter: Add interface to support\n\tcropping capability","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","References":"<20240709202151.152289-1-umang.jain@ideasonboard.com>\n\t<20240709202151.152289-3-umang.jain@ideasonboard.com>\n\t\n\t<92cdcc85-6897-40c7-a63c-d4c426e3e18e@ideasonboard.com>\n\t\n\t\n\t","Content-Language":"en-US","From":"Umang Jain ","In-Reply-To":"","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\" "}}]