[{"id":30212,"web_url":"https://patchwork.libcamera.org/comment/30212/","msgid":"","date":"2024-07-02T12:36:18","subject":"Re: [PATCH v4 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, Jun 27, 2024 at 07:16:53PM +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> For implementing the getCropBounds(), V4L2VideoDevice needs\n> to be extended to support VIDIOC_G_SELECTION via\n> V4L2VideoDevice::getSelection().\n> \n> Signed-off-by: Umang Jain \n> ---\n> include/libcamera/internal/converter.h | 5 ++\n> .../internal/converter/converter_v4l2_m2m.h | 7 ++\n> include/libcamera/internal/v4l2_videodevice.h | 1 +\n> src/libcamera/converter.cpp | 35 ++++++++\n> .../converter/converter_v4l2_m2m.cpp | 85 +++++++++++++++++++\n> src/libcamera/v4l2_videodevice.cpp | 32 +++++++\n> 6 files changed, 165 insertions(+)\n> \n> diff --git a/include/libcamera/internal/converter.h b/include/libcamera/internal/converter.h\n> index 15bc7dca..deba75e3 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 = (1 << 0),\n> +\t\tCrop = (1 << 1),\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..e97fbdd2 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,9 @@ private:\n> \n> \t\tint queueBuffers(FrameBuffer *input, FrameBuffer *output);\n> \n> +\t\tint getSelection(unsigned int target, Rectangle *rect);\n> +\t\tint setSelection(unsigned int target, Rectangle *rect);\n> +\n> \tprotected:\n> \t\tstd::string logPrefix() const override;\n> \n> diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h\n> index 9057be08..506d6a65 100644\n> --- a/include/libcamera/internal/v4l2_videodevice.h\n> +++ b/include/libcamera/internal/v4l2_videodevice.h\n> @@ -209,6 +209,7 @@ public:\n> \tFormats formats(uint32_t code = 0);\n> \n> \tint setSelection(unsigned int target, Rectangle *rect);\n> +\tint getSelection(unsigned int target, Rectangle *rect);\n> \n> \tint allocateBuffers(unsigned int count,\n> \t\t\t std::vector> *buffers);\n> diff --git a/src/libcamera/converter.cpp b/src/libcamera/converter.cpp\n> index 2dc7d984..19634e1c 100644\n> --- a/src/libcamera/converter.cpp\n> +++ b/src/libcamera/converter.cpp\n> @@ -39,6 +39,9 @@ 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> /**\n> @@ -161,6 +164,38 @@ 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> +\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> +\tRectangle rect;\n> +\n> +\treturn { rect, rect };\n> +}\n\nThese look strange to me... they seem to not to expected to ever be\ncalled... is there no way to keep them virtual and require them to be\noverridden by inheritors? Otherwise the implementations clearly imply\n\"crop is not supported\", and any application that does actually call\nthese would end up with unexpected nops.\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..4bb625e9 100644\n> --- a/src/libcamera/converter/converter_v4l2_m2m.cpp\n> +++ b/src/libcamera/converter/converter_v4l2_m2m.cpp\n> @@ -155,6 +155,24 @@ 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> +int V4L2M2MConverter::V4L2M2MStream::getSelection(unsigned int target, Rectangle *rect)\n> +{\n> +\tint ret = m2m_->output()->getSelection(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 +393,69 @@ 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> +\tauto iter = streams_.find(stream);\n> +\tif (iter == streams_.end())\n> +\t\treturn {};\n> +\n> +\tif (!(getFeatures() & Feature::Crop)) {\n> +\t\tLOG(Converter, Error) << \"Crop functionality is not supported\";\n> +\t\treturn {};\n> +\t}\n> +\n> +\t/* Get the maximum crop bound */\n> +\tret = iter->second->getSelection(V4L2_SEL_TGT_CROP_BOUNDS, &maxCrop);\n\nDo you not need to set an astronomically large crop bound like the\nminimum one? Isn't it possible to simply use the size of the configured\nformat?\n\n\nPaul\n\n> +\tif (ret) {\n> +\t\tLOG(Converter, Error) << \"Failed to query maximum crop bound\";\n> +\t\treturn {};\n> +\t}\n> +\n> +\t/*\n> +\t * Get the minimum crop bound by setting 1x1 crop rectangle. The returned\n> +\t * rectangle should be them minimum crop supported by the driver.\n> +\t */\n> +\tminCrop.width = 1;\n> +\tminCrop.height = 1;\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> +\t/* Reset the crop rectangle to maximum */\n> +\tret = setCrop(stream, &maxCrop);\n> +\tif (ret) {\n> +\t\tLOG(Converter, Error) << \"Failed to reset the crop to \" << maxCrop.toString();\n> +\t\treturn {};\n> +\t}\n> +\n> +\treturn { minCrop, maxCrop };\n> +}\n> +\n> /**\n> * \\copydoc libcamera::Converter::start\n> */\n> @@ -448,6 +529,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> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> index 4947aa3d..dfbfae2c 100644\n> --- a/src/libcamera/v4l2_videodevice.cpp\n> +++ b/src/libcamera/v4l2_videodevice.cpp\n> @@ -1226,6 +1226,38 @@ int V4L2VideoDevice::setSelection(unsigned int target, Rectangle *rect)\n> \treturn 0;\n> }\n> \n> +/**\n> + * \\brief Get the selection rectangle \\a rect for \\a target\n> + * \\param[in] target The selection target defined by the V4L2_SEL_TGT_* flags\n> + * \\param[inout] rect The selection rectangle that has been queried\n> + *\n> + * \\todo Define a V4L2SelectionTarget enum for the selection target\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n> + */\n> +int V4L2VideoDevice::getSelection(unsigned int target, Rectangle *rect)\n> +{\n> +\tstruct v4l2_selection sel = {};\n> +\n> +\tsel.type = bufferType_;\n> +\tsel.target = target;\n> +\tsel.flags = 0;\n> +\n> +\tint ret = ioctl(VIDIOC_G_SELECTION, &sel);\n> +\tif (ret < 0) {\n> +\t\tLOG(V4L2, Error) << \"Unable to get rectangle \" << target\n> +\t\t\t\t << \": \" << strerror(-ret);\n> +\t\treturn ret;\n> +\t}\n> +\n> +\trect->x = sel.r.left;\n> +\trect->y = sel.r.top;\n> +\trect->width = sel.r.width;\n> +\trect->height = sel.r.height;\n> +\n> +\treturn 0;\n> +}\n> +\n> int V4L2VideoDevice::requestBuffers(unsigned int count,\n> \t\t\t\t enum v4l2_memory memoryType)\n> {\n> -- \n> 2.44.0\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 0ABB6BEFBE\n\tfor ;\n\tTue, 2 Jul 2024 12:36:27 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 335E462C96;\n\tTue, 2 Jul 2024 14:36:26 +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 C19AE619CC\n\tfor ;\n\tTue, 2 Jul 2024 14:36:24 +0200 (CEST)","from pyrite.rasen.tech (h175-177-049-156.catv02.itscom.jp\n\t[175.177.49.156])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 63BED5A4;\n\tTue, 2 Jul 2024 14:35:56 +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=\"TR41XTHb\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1719923757;\n\tbh=lLBc83rmPIYDt+SJ2FlCAs7AtuqmjPGI+AHLqvysDwA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=TR41XTHb6vHYBi5Qy+suoLYMk11mJ5Md1q28WKebnGKLYo7/FIpIRUoMtKc533TCL\n\tqtB825ZNb6dvhqTA1lzMlhW6HieQ3Pe5+3mvVHCtvVCvpeE87Pz66ngVRkIwUG601Z\n\tqGlvFr4kw2u4awYfMaeRcszsLwmIAroT0tido1qs=","Date":"Tue, 2 Jul 2024 21:36:18 +0900","From":"Paul Elder ","To":"Umang Jain ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v4 2/5] libcamera: converter: Add interface to support\n\tcropping capability","Message-ID":"","References":"<20240627134656.582462-1-umang.jain@ideasonboard.com>\n\t<20240627134656.582462-3-umang.jain@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20240627134656.582462-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":30231,"web_url":"https://patchwork.libcamera.org/comment/30231/","msgid":"","date":"2024-07-03T03:28:53","subject":"Re: [PATCH v4 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 Paul\n\nOn 02/07/24 6:06 pm, Paul Elder wrote:\n> On Thu, Jun 27, 2024 at 07:16:53PM +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>> For implementing the getCropBounds(), V4L2VideoDevice needs\n>> to be extended to support VIDIOC_G_SELECTION via\n>> V4L2VideoDevice::getSelection().\n>>\n>> Signed-off-by: Umang Jain \n>> ---\n>> include/libcamera/internal/converter.h | 5 ++\n>> .../internal/converter/converter_v4l2_m2m.h | 7 ++\n>> include/libcamera/internal/v4l2_videodevice.h | 1 +\n>> src/libcamera/converter.cpp | 35 ++++++++\n>> .../converter/converter_v4l2_m2m.cpp | 85 +++++++++++++++++++\n>> src/libcamera/v4l2_videodevice.cpp | 32 +++++++\n>> 6 files changed, 165 insertions(+)\n>>\n>> diff --git a/include/libcamera/internal/converter.h b/include/libcamera/internal/converter.h\n>> index 15bc7dca..deba75e3 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 = (1 << 0),\n>> +\t\tCrop = (1 << 1),\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..e97fbdd2 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,9 @@ private:\n>> \n>> \t\tint queueBuffers(FrameBuffer *input, FrameBuffer *output);\n>> \n>> +\t\tint getSelection(unsigned int target, Rectangle *rect);\n>> +\t\tint setSelection(unsigned int target, Rectangle *rect);\n>> +\n>> \tprotected:\n>> \t\tstd::string logPrefix() const override;\n>> \n>> diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h\n>> index 9057be08..506d6a65 100644\n>> --- a/include/libcamera/internal/v4l2_videodevice.h\n>> +++ b/include/libcamera/internal/v4l2_videodevice.h\n>> @@ -209,6 +209,7 @@ public:\n>> \tFormats formats(uint32_t code = 0);\n>> \n>> \tint setSelection(unsigned int target, Rectangle *rect);\n>> +\tint getSelection(unsigned int target, Rectangle *rect);\n>> \n>> \tint allocateBuffers(unsigned int count,\n>> \t\t\t std::vector> *buffers);\n>> diff --git a/src/libcamera/converter.cpp b/src/libcamera/converter.cpp\n>> index 2dc7d984..19634e1c 100644\n>> --- a/src/libcamera/converter.cpp\n>> +++ b/src/libcamera/converter.cpp\n>> @@ -39,6 +39,9 @@ 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>> /**\n>> @@ -161,6 +164,38 @@ 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>> +\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>> +\tRectangle rect;\n>> +\n>> +\treturn { rect, rect };\n>> +}\n> These look strange to me... they seem to not to expected to ever be\n> called... is there no way to keep them virtual and require them to be\n> overridden by inheritors? Otherwise the implementations clearly imply\n\nDo you mean I should make the getCropBounds() and setCrop() 'pure' \nvirtual functions (like the others in the converter interface) ? In that \ncase, all inherited class will need to provide a crop implementation \nregardless they support the capability or not.\n\nCurrently, they are just virtual functions, and inherited ones are not \nmandated to provide implementation. But the converter interface need to \ngive a its implementation in this case.\n\n> \"crop is not supported\", and any application that does actually call\n> these would end up with unexpected nops.\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..4bb625e9 100644\n>> --- a/src/libcamera/converter/converter_v4l2_m2m.cpp\n>> +++ b/src/libcamera/converter/converter_v4l2_m2m.cpp\n>> @@ -155,6 +155,24 @@ 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>> +int V4L2M2MConverter::V4L2M2MStream::getSelection(unsigned int target, Rectangle *rect)\n>> +{\n>> +\tint ret = m2m_->output()->getSelection(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 +393,69 @@ 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>> +\tauto iter = streams_.find(stream);\n>> +\tif (iter == streams_.end())\n>> +\t\treturn {};\n>> +\n>> +\tif (!(getFeatures() & Feature::Crop)) {\n>> +\t\tLOG(Converter, Error) << \"Crop functionality is not supported\";\n>> +\t\treturn {};\n>> +\t}\n>> +\n>> +\t/* Get the maximum crop bound */\n>> +\tret = iter->second->getSelection(V4L2_SEL_TGT_CROP_BOUNDS, &maxCrop);\n> Do you not need to set an astronomically large crop bound like the\n> minimum one? Isn't it possible to simply use the size of the configured\n> format?\n\nPossibly. That would reduce a lot of code here as well (esp. the \nintroduction of G_SELECTION)\n\n>\n> Paul\n>\n>> +\tif (ret) {\n>> +\t\tLOG(Converter, Error) << \"Failed to query maximum crop bound\";\n>> +\t\treturn {};\n>> +\t}\n>> +\n>> +\t/*\n>> +\t * Get the minimum crop bound by setting 1x1 crop rectangle. The returned\n>> +\t * rectangle should be them minimum crop supported by the driver.\n>> +\t */\n>> +\tminCrop.width = 1;\n>> +\tminCrop.height = 1;\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>> +\t/* Reset the crop rectangle to maximum */\n>> +\tret = setCrop(stream, &maxCrop);\n>> +\tif (ret) {\n>> +\t\tLOG(Converter, Error) << \"Failed to reset the crop to \" << maxCrop.toString();\n>> +\t\treturn {};\n>> +\t}\n>> +\n>> +\treturn { minCrop, maxCrop };\n>> +}\n>> +\n>> /**\n>> * \\copydoc libcamera::Converter::start\n>> */\n>> @@ -448,6 +529,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>> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n>> index 4947aa3d..dfbfae2c 100644\n>> --- a/src/libcamera/v4l2_videodevice.cpp\n>> +++ b/src/libcamera/v4l2_videodevice.cpp\n>> @@ -1226,6 +1226,38 @@ int V4L2VideoDevice::setSelection(unsigned int target, Rectangle *rect)\n>> \treturn 0;\n>> }\n>> \n>> +/**\n>> + * \\brief Get the selection rectangle \\a rect for \\a target\n>> + * \\param[in] target The selection target defined by the V4L2_SEL_TGT_* flags\n>> + * \\param[inout] rect The selection rectangle that has been queried\n>> + *\n>> + * \\todo Define a V4L2SelectionTarget enum for the selection target\n>> + *\n>> + * \\return 0 on success or a negative error code otherwise\n>> + */\n>> +int V4L2VideoDevice::getSelection(unsigned int target, Rectangle *rect)\n>> +{\n>> +\tstruct v4l2_selection sel = {};\n>> +\n>> +\tsel.type = bufferType_;\n>> +\tsel.target = target;\n>> +\tsel.flags = 0;\n>> +\n>> +\tint ret = ioctl(VIDIOC_G_SELECTION, &sel);\n>> +\tif (ret < 0) {\n>> +\t\tLOG(V4L2, Error) << \"Unable to get rectangle \" << target\n>> +\t\t\t\t << \": \" << strerror(-ret);\n>> +\t\treturn ret;\n>> +\t}\n>> +\n>> +\trect->x = sel.r.left;\n>> +\trect->y = sel.r.top;\n>> +\trect->width = sel.r.width;\n>> +\trect->height = sel.r.height;\n>> +\n>> +\treturn 0;\n>> +}\n>> +\n>> int V4L2VideoDevice::requestBuffers(unsigned int count,\n>> \t\t\t\t enum v4l2_memory memoryType)\n>> {\n>> -- \n>> 2.44.0\n>>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 7B508BEFBE\n\tfor ;\n\tWed, 3 Jul 2024 03:29:02 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4465462E22;\n\tWed, 3 Jul 2024 05:29:01 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 92EE3619C0\n\tfor ;\n\tWed, 3 Jul 2024 05:28:58 +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 0D05E5A4;\n\tWed, 3 Jul 2024 05:28:29 +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=\"Q8t5DqCj\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1719977310;\n\tbh=Cw0kazDBvZtHA7p0RbHwKfmd2V/7j2kVAZVfNsKERpI=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=Q8t5DqCj1yhOCTbfmydMwUvlKr4nt85JQzhkIVpn034zE4ud44cq0Lkdv+PZfIr1u\n\tSc5TRcUkkLnDLX4B4WmC2R7AEalaV4DjVeBRUqiJbVQ9/VfcM3G6a99mYnbMpJllTy\n\tItyxFAT4sP6DL3nTLmYEWhiPk8rZyj7jfAd+QwB8=","Message-ID":"","Date":"Wed, 3 Jul 2024 08:58:53 +0530","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v4 2/5] libcamera: converter: Add interface to support\n\tcropping capability","To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org","References":"<20240627134656.582462-1-umang.jain@ideasonboard.com>\n\t<20240627134656.582462-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":30236,"web_url":"https://patchwork.libcamera.org/comment/30236/","msgid":"","date":"2024-07-03T07:32:41","subject":"Re: [PATCH v4 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 03, 2024 at 08:58:53AM +0530, Umang Jain wrote:\n> Hi Paul\n> \n> On 02/07/24 6:06 pm, Paul Elder wrote:\n> > On Thu, Jun 27, 2024 at 07:16:53PM +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> > > For implementing the getCropBounds(), V4L2VideoDevice needs\n> > > to be extended to support VIDIOC_G_SELECTION via\n> > > V4L2VideoDevice::getSelection().\n> > > \n> > > Signed-off-by: Umang Jain \n> > > ---\n> > > include/libcamera/internal/converter.h | 5 ++\n> > > .../internal/converter/converter_v4l2_m2m.h | 7 ++\n> > > include/libcamera/internal/v4l2_videodevice.h | 1 +\n> > > src/libcamera/converter.cpp | 35 ++++++++\n> > > .../converter/converter_v4l2_m2m.cpp | 85 +++++++++++++++++++\n> > > src/libcamera/v4l2_videodevice.cpp | 32 +++++++\n> > > 6 files changed, 165 insertions(+)\n> > > \n> > > diff --git a/include/libcamera/internal/converter.h b/include/libcamera/internal/converter.h\n> > > index 15bc7dca..deba75e3 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> > > #include \n> > > @@ -35,6 +36,7 @@ class Converter\n> > > public:\n> > > \tenum class Feature {\n> > > \t\tNone = (1 << 0),\n> > > +\t\tCrop = (1 << 1),\n> > > \t};\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> > > +\tvirtual int setCrop(const Stream *stream, Rectangle *rect);\n> > > +\tvirtual std::pair getCropBounds(const Stream *stream);\n> > > +\n> > > \tSignal inputBufferReady;\n> > > \tSignal outputBufferReady;\n> > > diff --git a/include/libcamera/internal/converter/converter_v4l2_m2m.h b/include/libcamera/internal/converter/converter_v4l2_m2m.h\n> > > index 91701dbe..e97fbdd2 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> > > class V4L2M2MConverter : public Converter\n> > > @@ -57,6 +58,9 @@ public:\n> > > \tint queueBuffers(FrameBuffer *input,\n> > > \t\t\t const std::map &outputs);\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,9 @@ private:\n> > > \t\tint queueBuffers(FrameBuffer *input, FrameBuffer *output);\n> > > +\t\tint getSelection(unsigned int target, Rectangle *rect);\n> > > +\t\tint setSelection(unsigned int target, Rectangle *rect);\n> > > +\n> > > \tprotected:\n> > > \t\tstd::string logPrefix() const override;\n> > > diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h\n> > > index 9057be08..506d6a65 100644\n> > > --- a/include/libcamera/internal/v4l2_videodevice.h\n> > > +++ b/include/libcamera/internal/v4l2_videodevice.h\n> > > @@ -209,6 +209,7 @@ public:\n> > > \tFormats formats(uint32_t code = 0);\n> > > \tint setSelection(unsigned int target, Rectangle *rect);\n> > > +\tint getSelection(unsigned int target, Rectangle *rect);\n> > > \tint allocateBuffers(unsigned int count,\n> > > \t\t\t std::vector> *buffers);\n> > > diff --git a/src/libcamera/converter.cpp b/src/libcamera/converter.cpp\n> > > index 2dc7d984..19634e1c 100644\n> > > --- a/src/libcamera/converter.cpp\n> > > +++ b/src/libcamera/converter.cpp\n> > > @@ -39,6 +39,9 @@ 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 +164,38 @@ Converter::~Converter()\n> > > * \\return 0 on success or a negative error code otherwise\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> > > +\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> > > +\tRectangle rect;\n> > > +\n> > > +\treturn { rect, rect };\n> > > +}\n> > These look strange to me... they seem to not to expected to ever be\n> > called... is there no way to keep them virtual and require them to be\n> > overridden by inheritors? Otherwise the implementations clearly imply\n> \n> Do you mean I should make the getCropBounds() and setCrop() 'pure' virtual\n> functions (like the others in the converter interface) ? In that case, all\n> inherited class will need to provide a crop implementation regardless they\n> support the capability or not.\n\nOh okay I see what you mean. So it's not expected that these will be\ncalled when the capability is unavailable, and this class implements\nnops so that specializations don't have to, as they must override all\nvirtual functions.\n\n> Currently, they are just virtual functions, and inherited ones are not\n> mandated to provide implementation. But the converter interface need to give\n> a its implementation in this case.\n> \n> > \"crop is not supported\", and any application that does actually call\n> > these would end up with unexpected nops.\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..4bb625e9 100644\n> > > --- a/src/libcamera/converter/converter_v4l2_m2m.cpp\n> > > +++ b/src/libcamera/converter/converter_v4l2_m2m.cpp\n> > > @@ -155,6 +155,24 @@ int V4L2M2MConverter::V4L2M2MStream::queueBuffers(FrameBuffer *input, FrameBuffe\n> > > \treturn 0;\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> > > +int V4L2M2MConverter::V4L2M2MStream::getSelection(unsigned int target, Rectangle *rect)\n> > > +{\n> > > +\tint ret = m2m_->output()->getSelection(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 +393,69 @@ int V4L2M2MConverter::exportBuffers(const Stream *stream, unsigned int count,\n> > > \treturn iter->second->exportBuffers(count, buffers);\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> > > +\tauto iter = streams_.find(stream);\n> > > +\tif (iter == streams_.end())\n> > > +\t\treturn {};\n> > > +\n> > > +\tif (!(getFeatures() & Feature::Crop)) {\n> > > +\t\tLOG(Converter, Error) << \"Crop functionality is not supported\";\n> > > +\t\treturn {};\n> > > +\t}\n> > > +\n> > > +\t/* Get the maximum crop bound */\n> > > +\tret = iter->second->getSelection(V4L2_SEL_TGT_CROP_BOUNDS, &maxCrop);\n> > Do you not need to set an astronomically large crop bound like the\n> > minimum one? Isn't it possible to simply use the size of the configured\n> > format?\n> \n> Possibly. That would reduce a lot of code here as well (esp. the\n> introduction of G_SELECTION)\n> \n\nI keep thinking it would be nice to have G_SELECTION but if there aren't\nany users I suppose it doesn't matter.\n\n\nPaul\n\n> > \n> > > +\tif (ret) {\n> > > +\t\tLOG(Converter, Error) << \"Failed to query maximum crop bound\";\n> > > +\t\treturn {};\n> > > +\t}\n> > > +\n> > > +\t/*\n> > > +\t * Get the minimum crop bound by setting 1x1 crop rectangle. The returned\n> > > +\t * rectangle should be them minimum crop supported by the driver.\n> > > +\t */\n> > > +\tminCrop.width = 1;\n> > > +\tminCrop.height = 1;\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> > > +\t/* Reset the crop rectangle to maximum */\n> > > +\tret = setCrop(stream, &maxCrop);\n> > > +\tif (ret) {\n> > > +\t\tLOG(Converter, Error) << \"Failed to reset the crop to \" << maxCrop.toString();\n> > > +\t\treturn {};\n> > > +\t}\n> > > +\n> > > +\treturn { minCrop, maxCrop };\n> > > +}\n> > > +\n> > > /**\n> > > * \\copydoc libcamera::Converter::start\n> > > */\n> > > @@ -448,6 +529,10 @@ int V4L2M2MConverter::queueBuffers(FrameBuffer *input,\n> > > \treturn 0;\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> > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> > > index 4947aa3d..dfbfae2c 100644\n> > > --- a/src/libcamera/v4l2_videodevice.cpp\n> > > +++ b/src/libcamera/v4l2_videodevice.cpp\n> > > @@ -1226,6 +1226,38 @@ int V4L2VideoDevice::setSelection(unsigned int target, Rectangle *rect)\n> > > \treturn 0;\n> > > }\n> > > +/**\n> > > + * \\brief Get the selection rectangle \\a rect for \\a target\n> > > + * \\param[in] target The selection target defined by the V4L2_SEL_TGT_* flags\n> > > + * \\param[inout] rect The selection rectangle that has been queried\n> > > + *\n> > > + * \\todo Define a V4L2SelectionTarget enum for the selection target\n> > > + *\n> > > + * \\return 0 on success or a negative error code otherwise\n> > > + */\n> > > +int V4L2VideoDevice::getSelection(unsigned int target, Rectangle *rect)\n> > > +{\n> > > +\tstruct v4l2_selection sel = {};\n> > > +\n> > > +\tsel.type = bufferType_;\n> > > +\tsel.target = target;\n> > > +\tsel.flags = 0;\n> > > +\n> > > +\tint ret = ioctl(VIDIOC_G_SELECTION, &sel);\n> > > +\tif (ret < 0) {\n> > > +\t\tLOG(V4L2, Error) << \"Unable to get rectangle \" << target\n> > > +\t\t\t\t << \": \" << strerror(-ret);\n> > > +\t\treturn ret;\n> > > +\t}\n> > > +\n> > > +\trect->x = sel.r.left;\n> > > +\trect->y = sel.r.top;\n> > > +\trect->width = sel.r.width;\n> > > +\trect->height = sel.r.height;\n> > > +\n> > > +\treturn 0;\n> > > +}\n> > > +\n> > > int V4L2VideoDevice::requestBuffers(unsigned int count,\n> > > \t\t\t\t enum v4l2_memory memoryType)\n> > > {\n> > > -- \n> > > 2.44.0\n> > > \n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id C6A5EBD87C\n\tfor ;\n\tWed, 3 Jul 2024 07:32:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CDE2862E24;\n\tWed, 3 Jul 2024 09:32:49 +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 A5AB3619C7\n\tfor ;\n\tWed, 3 Jul 2024 09:32:48 +0200 (CEST)","from pyrite.rasen.tech (h175-177-049-156.catv02.itscom.jp\n\t[175.177.49.156])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id BBFBA5A4;\n\tWed, 3 Jul 2024 09:32:19 +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=\"I+XXWR+Y\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1719991940;\n\tbh=UySNpgInACETat4UMHsShZ1OO68AQRLxWG2JXYVLNiY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=I+XXWR+YsLLb5cJ9hFlv/MCHvz98CWCy10WGHx09d3gREpusm9M8hsm11H5tfwwm0\n\tr3cComl9QYsNtljGF3DSY5BQxy6ya2TxEcczbYt2tl32C8yz/vcwvKaiP9oMoJQSdu\n\tkkQ21sQTANTzp/38LSWP4Px0WwOMSZGZD7W8vO0Y=","Date":"Wed, 3 Jul 2024 16:32:41 +0900","From":"Paul Elder ","To":"Umang Jain ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v4 2/5] libcamera: converter: Add interface to support\n\tcropping capability","Message-ID":"","References":"<20240627134656.582462-1-umang.jain@ideasonboard.com>\n\t<20240627134656.582462-3-umang.jain@ideasonboard.com>\n\t\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":30238,"web_url":"https://patchwork.libcamera.org/comment/30238/","msgid":"","date":"2024-07-03T07:39:50","subject":"Re: [PATCH v4 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 03, 2024 at 08:58:53AM +0530, Umang Jain wrote:\n> Hi Paul\n> \n> On 02/07/24 6:06 pm, Paul Elder wrote:\n> > On Thu, Jun 27, 2024 at 07:16:53PM +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> > > For implementing the getCropBounds(), V4L2VideoDevice needs\n> > > to be extended to support VIDIOC_G_SELECTION via\n> > > V4L2VideoDevice::getSelection().\n> > > \n> > > Signed-off-by: Umang Jain \n> > > ---\n> > > include/libcamera/internal/converter.h | 5 ++\n> > > .../internal/converter/converter_v4l2_m2m.h | 7 ++\n> > > include/libcamera/internal/v4l2_videodevice.h | 1 +\n> > > src/libcamera/converter.cpp | 35 ++++++++\n> > > .../converter/converter_v4l2_m2m.cpp | 85 +++++++++++++++++++\n> > > src/libcamera/v4l2_videodevice.cpp | 32 +++++++\n> > > 6 files changed, 165 insertions(+)\n> > > \n> > > diff --git a/include/libcamera/internal/converter.h b/include/libcamera/internal/converter.h\n> > > index 15bc7dca..deba75e3 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> > > #include \n> > > @@ -35,6 +36,7 @@ class Converter\n> > > public:\n> > > \tenum class Feature {\n> > > \t\tNone = (1 << 0),\n> > > +\t\tCrop = (1 << 1),\n> > > \t};\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> > > +\tvirtual int setCrop(const Stream *stream, Rectangle *rect);\n> > > +\tvirtual std::pair getCropBounds(const Stream *stream);\n> > > +\n> > > \tSignal inputBufferReady;\n> > > \tSignal outputBufferReady;\n> > > diff --git a/include/libcamera/internal/converter/converter_v4l2_m2m.h b/include/libcamera/internal/converter/converter_v4l2_m2m.h\n> > > index 91701dbe..e97fbdd2 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> > > class V4L2M2MConverter : public Converter\n> > > @@ -57,6 +58,9 @@ public:\n> > > \tint queueBuffers(FrameBuffer *input,\n> > > \t\t\t const std::map &outputs);\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,9 @@ private:\n> > > \t\tint queueBuffers(FrameBuffer *input, FrameBuffer *output);\n> > > +\t\tint getSelection(unsigned int target, Rectangle *rect);\n> > > +\t\tint setSelection(unsigned int target, Rectangle *rect);\n> > > +\n> > > \tprotected:\n> > > \t\tstd::string logPrefix() const override;\n> > > diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h\n> > > index 9057be08..506d6a65 100644\n> > > --- a/include/libcamera/internal/v4l2_videodevice.h\n> > > +++ b/include/libcamera/internal/v4l2_videodevice.h\n> > > @@ -209,6 +209,7 @@ public:\n> > > \tFormats formats(uint32_t code = 0);\n> > > \tint setSelection(unsigned int target, Rectangle *rect);\n> > > +\tint getSelection(unsigned int target, Rectangle *rect);\n> > > \tint allocateBuffers(unsigned int count,\n> > > \t\t\t std::vector> *buffers);\n> > > diff --git a/src/libcamera/converter.cpp b/src/libcamera/converter.cpp\n> > > index 2dc7d984..19634e1c 100644\n> > > --- a/src/libcamera/converter.cpp\n> > > +++ b/src/libcamera/converter.cpp\n> > > @@ -39,6 +39,9 @@ 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 +164,38 @@ Converter::~Converter()\n> > > * \\return 0 on success or a negative error code otherwise\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> > > +\treturn 0;\n> > > +}\n\nActually, it just occurred to me that this should probably return\n-ENOTSUP?\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> > > +\tRectangle rect;\n> > > +\n> > > +\treturn { rect, rect };\n> > > +}\n\nAnd this should return the size of the configured format?\n\nThose sound more correct to me imo.\n\nPaul\n\n> > These look strange to me... they seem to not to expected to ever be\n> > called... is there no way to keep them virtual and require them to be\n> > overridden by inheritors? Otherwise the implementations clearly imply\n> \n> Do you mean I should make the getCropBounds() and setCrop() 'pure' virtual\n> functions (like the others in the converter interface) ? In that case, all\n> inherited class will need to provide a crop implementation regardless they\n> support the capability or not.\n> \n> Currently, they are just virtual functions, and inherited ones are not\n> mandated to provide implementation. But the converter interface need to give\n> a its implementation in this case.\n> \n> > \"crop is not supported\", and any application that does actually call\n> > these would end up with unexpected nops.\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..4bb625e9 100644\n> > > --- a/src/libcamera/converter/converter_v4l2_m2m.cpp\n> > > +++ b/src/libcamera/converter/converter_v4l2_m2m.cpp\n> > > @@ -155,6 +155,24 @@ int V4L2M2MConverter::V4L2M2MStream::queueBuffers(FrameBuffer *input, FrameBuffe\n> > > \treturn 0;\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> > > +int V4L2M2MConverter::V4L2M2MStream::getSelection(unsigned int target, Rectangle *rect)\n> > > +{\n> > > +\tint ret = m2m_->output()->getSelection(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 +393,69 @@ int V4L2M2MConverter::exportBuffers(const Stream *stream, unsigned int count,\n> > > \treturn iter->second->exportBuffers(count, buffers);\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> > > +\tauto iter = streams_.find(stream);\n> > > +\tif (iter == streams_.end())\n> > > +\t\treturn {};\n> > > +\n> > > +\tif (!(getFeatures() & Feature::Crop)) {\n> > > +\t\tLOG(Converter, Error) << \"Crop functionality is not supported\";\n> > > +\t\treturn {};\n> > > +\t}\n> > > +\n> > > +\t/* Get the maximum crop bound */\n> > > +\tret = iter->second->getSelection(V4L2_SEL_TGT_CROP_BOUNDS, &maxCrop);\n> > Do you not need to set an astronomically large crop bound like the\n> > minimum one? Isn't it possible to simply use the size of the configured\n> > format?\n> \n> Possibly. That would reduce a lot of code here as well (esp. the\n> introduction of G_SELECTION)\n> \n> > \n> > Paul\n> > \n> > > +\tif (ret) {\n> > > +\t\tLOG(Converter, Error) << \"Failed to query maximum crop bound\";\n> > > +\t\treturn {};\n> > > +\t}\n> > > +\n> > > +\t/*\n> > > +\t * Get the minimum crop bound by setting 1x1 crop rectangle. The returned\n> > > +\t * rectangle should be them minimum crop supported by the driver.\n> > > +\t */\n> > > +\tminCrop.width = 1;\n> > > +\tminCrop.height = 1;\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> > > +\t/* Reset the crop rectangle to maximum */\n> > > +\tret = setCrop(stream, &maxCrop);\n> > > +\tif (ret) {\n> > > +\t\tLOG(Converter, Error) << \"Failed to reset the crop to \" << maxCrop.toString();\n> > > +\t\treturn {};\n> > > +\t}\n> > > +\n> > > +\treturn { minCrop, maxCrop };\n> > > +}\n> > > +\n> > > /**\n> > > * \\copydoc libcamera::Converter::start\n> > > */\n> > > @@ -448,6 +529,10 @@ int V4L2M2MConverter::queueBuffers(FrameBuffer *input,\n> > > \treturn 0;\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> > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> > > index 4947aa3d..dfbfae2c 100644\n> > > --- a/src/libcamera/v4l2_videodevice.cpp\n> > > +++ b/src/libcamera/v4l2_videodevice.cpp\n> > > @@ -1226,6 +1226,38 @@ int V4L2VideoDevice::setSelection(unsigned int target, Rectangle *rect)\n> > > \treturn 0;\n> > > }\n> > > +/**\n> > > + * \\brief Get the selection rectangle \\a rect for \\a target\n> > > + * \\param[in] target The selection target defined by the V4L2_SEL_TGT_* flags\n> > > + * \\param[inout] rect The selection rectangle that has been queried\n> > > + *\n> > > + * \\todo Define a V4L2SelectionTarget enum for the selection target\n> > > + *\n> > > + * \\return 0 on success or a negative error code otherwise\n> > > + */\n> > > +int V4L2VideoDevice::getSelection(unsigned int target, Rectangle *rect)\n> > > +{\n> > > +\tstruct v4l2_selection sel = {};\n> > > +\n> > > +\tsel.type = bufferType_;\n> > > +\tsel.target = target;\n> > > +\tsel.flags = 0;\n> > > +\n> > > +\tint ret = ioctl(VIDIOC_G_SELECTION, &sel);\n> > > +\tif (ret < 0) {\n> > > +\t\tLOG(V4L2, Error) << \"Unable to get rectangle \" << target\n> > > +\t\t\t\t << \": \" << strerror(-ret);\n> > > +\t\treturn ret;\n> > > +\t}\n> > > +\n> > > +\trect->x = sel.r.left;\n> > > +\trect->y = sel.r.top;\n> > > +\trect->width = sel.r.width;\n> > > +\trect->height = sel.r.height;\n> > > +\n> > > +\treturn 0;\n> > > +}\n> > > +\n> > > int V4L2VideoDevice::requestBuffers(unsigned int count,\n> > > \t\t\t\t enum v4l2_memory memoryType)\n> > > {\n> > > -- \n> > > 2.44.0\n> > > \n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 2267EBEFBE\n\tfor ;\n\tWed, 3 Jul 2024 07:39:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C1B0962E23;\n\tWed, 3 Jul 2024 09:39:57 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 576F8619C7\n\tfor ;\n\tWed, 3 Jul 2024 09:39:56 +0200 (CEST)","from pyrite.rasen.tech (h175-177-049-156.catv02.itscom.jp\n\t[175.177.49.156])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 868B55A4;\n\tWed, 3 Jul 2024 09:39:27 +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=\"AgclykMD\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1719992368;\n\tbh=n/OOS/mU7jhh9Fny46tuT8VZemWlc/zADYBM7h5sWdc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=AgclykMDxrvQiBKEX/LzTHmpbcvF5N1jbDWCnEdTOC0FdO/EH40OcTerovmXejSer\n\tgsCGGxdYimwUC6EjDspEOEzpIBvbv6Dn1rTzOQopW4xyjnxdNOYWLMtTLWUfAsTZuU\n\tImKAXrAElBEN06LKjJm9RwA8VnSBtzzGKao+9Y+c=","Date":"Wed, 3 Jul 2024 16:39:50 +0900","From":"Paul Elder ","To":"Umang Jain ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v4 2/5] libcamera: converter: Add interface to support\n\tcropping capability","Message-ID":"","References":"<20240627134656.582462-1-umang.jain@ideasonboard.com>\n\t<20240627134656.582462-3-umang.jain@ideasonboard.com>\n\t\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\" "}}]