[{"id":31343,"web_url":"https://patchwork.libcamera.org/comment/31343/","msgid":"","date":"2024-09-24T19:50:31","subject":"Re: [PATCH v7 2/4] libcamera: converter: Add interface to support\n\tcropping capability","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Umang,\n\nThank you for the patch. \n\nOn Fri, Sep 06, 2024 at 12:04:42PM +0530, Umang Jain wrote:\n> If the converter has cropping capability on its input, the interface\n> should support it by providing appropriate virtual functions. Provide\n> Feature::InputCrop in Feature enumeration for the same.\n> \n> Provide virtual setInputCrop() and inputCropBounds() 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 ability\n> on its input, hence it needs to discovered during construction time.\n> If the capability to crop to identified successfully, the cropping\n> bounds are determined during configure() time.\n> \n> Signed-off-by: Umang Jain \n> Reviewed-by: Paul Elder \n> ---\n> include/libcamera/internal/converter.h | 5 +\n> .../internal/converter/converter_v4l2_m2m.h | 13 ++-\n> src/libcamera/converter.cpp | 38 +++++++\n> .../converter/converter_v4l2_m2m.cpp | 106 +++++++++++++++++-\n> 4 files changed, 158 insertions(+), 4 deletions(-)\n> \n> diff --git a/include/libcamera/internal/converter.h b/include/libcamera/internal/converter.h\n> index 6623de4d..ffbb6f34 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\tInputCrop = (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 setInputCrop(const Stream *stream, Rectangle *rect) = 0;\n> +\tvirtual std::pair inputCropBounds(const Stream *stream) = 0;\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 b9e59899..7d321c85 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,11 +58,15 @@ public:\n> \tint queueBuffers(FrameBuffer *input,\n> \t\t\t const std::map &outputs);\n> \n> +\tint setInputCrop(const Stream *stream, Rectangle *rect);\n> +\tstd::pair inputCropBounds(const Stream *stream);\n> +\n> private:\n> \tclass V4L2M2MStream : protected Loggable\n> \t{\n> \tpublic:\n> -\t\tV4L2M2MStream(V4L2M2MConverter *converter, const Stream *stream);\n> +\t\tV4L2M2MStream(V4L2M2MConverter *converter, const Stream *stream,\n> +\t\t\t Features features);\n> \n> \t\tbool isValid() const { return m2m_ != nullptr; }\n> \n> @@ -75,6 +80,9 @@ private:\n> \n> \t\tint queueBuffers(FrameBuffer *input, FrameBuffer *output);\n> \n> +\t\tint setInputSelection(unsigned int target, Rectangle *rect);\n> +\t\tstd::pair inputCropBounds();\n> +\n> \tprotected:\n> \t\tstd::string logPrefix() const override;\n> \n> @@ -88,6 +96,9 @@ private:\n> \n> \t\tunsigned int inputBufferCount_;\n> \t\tunsigned int outputBufferCount_;\n> +\n> +\t\tstd::pair inputCropBounds_;\n> +\t\tFeatures features_;\n> \t};\n> \n> \tstd::unique_ptr m2m_;\n> diff --git a/src/libcamera/converter.cpp b/src/libcamera/converter.cpp\n> index d7bb7273..7cb6532a 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::InputCrop\n> + * \\brief Cropping capability at input is supported by the converter\n> */\n> \n> /**\n> @@ -161,6 +165,40 @@ Converter::~Converter()\n> * \\return 0 on success or a negative error code otherwise\n> */\n> \n> +/**\n> + * \\fn Converter::setInputCrop()\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 apply and return the rectangle\n> + * that is actually applied\n> + *\n> + * Set the crop rectangle \\a rect for \\a stream provided the converter supports\n> + * cropping. The converter has the Feature::InputCrop flag in this case.\n> + *\n> + * The underlying hardware can adjust the rectangle supplied by the user\n> + * due to hardware constraints. Caller can inspect \\a rect to determine the\n> + * actual rectangle that has been applied by the converter, after this function\n> + * returns.\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n> + */\n> +\n> +/**\n> + * \\fn Converter::inputCropBounds()\n> + * \\brief Retrieve the crop bounds for \\a stream\n> + * \\param[in] stream The output stream\n> + *\n> + * Retrieve the minimum and maximum crop bounds for \\a stream. The converter\n> + * should support cropping (Feature::InputCrop).\n> + *\n> + * The crop bounds depend on the configuration of the output stream hence, this\n> + * function should be called after the \\a stream has been configured using\n> + * configure().\n> + *\n> + * \\return A std::pair containing minimum and maximum\n> + * crop bound respectively.\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 4f3e8ce4..1bf47ff9 100644\n> --- a/src/libcamera/converter/converter_v4l2_m2m.cpp\n> +++ b/src/libcamera/converter/converter_v4l2_m2m.cpp\n> @@ -34,8 +34,9 @@ LOG_DECLARE_CATEGORY(Converter)\n> * V4L2M2MConverter::V4L2M2MStream\n> */\n> \n> -V4L2M2MConverter::V4L2M2MStream::V4L2M2MStream(V4L2M2MConverter *converter, const Stream *stream)\n> -\t: converter_(converter), stream_(stream)\n> +V4L2M2MConverter::V4L2M2MStream::V4L2M2MStream(V4L2M2MConverter *converter, const Stream *stream,\n> +\t\t\t\t\t Features features)\n> +\t: converter_(converter), stream_(stream), features_(features)\n> {\n> \tm2m_ = std::make_unique(converter->deviceNode());\n> \n> @@ -97,6 +98,33 @@ int V4L2M2MConverter::V4L2M2MStream::configure(const StreamConfiguration &inputC\n> \tinputBufferCount_ = inputCfg.bufferCount;\n> \toutputBufferCount_ = outputCfg.bufferCount;\n> \n> +\tif (features_ & Feature::InputCrop) {\n\nI don't fully understand why the stream has it's own features_ member.\nWouldn't it be sufficient to do\n\nif(converter_->features() & Feature::InputCrop) {\n\nor did I miss something?\n\n> +\t\tRectangle minCrop;\n> +\t\tRectangle maxCrop;\n> +\n> +\t\t/* Find crop bounds */\n> +\t\tminCrop.width = 1;\n> +\t\tminCrop.height = 1;\n> +\t\tmaxCrop.width = UINT_MAX;\n> +\t\tmaxCrop.height = UINT_MAX;\n> +\n> +\t\tret = setInputSelection(V4L2_SEL_TGT_CROP, &minCrop);\n> +\t\tif (ret) {\n> +\t\t\tLOG(Converter, Error) << \"Could not query minimum selection crop\"\n> +\t\t\t\t\t << strerror(-ret);\n> +\t\t\treturn ret;\n> +\t\t}\n> +\n> +\t\tret = setInputSelection(V4L2_SEL_TGT_CROP, &maxCrop);\n> +\t\tif (ret) {\n> +\t\t\tLOG(Converter, Error) << \"Could not query maximum selection crop\"\n> +\t\t\t\t\t << strerror(-ret);\n> +\t\t\treturn ret;\n> +\t\t}\n> +\n> +\t\tinputCropBounds_ = { minCrop, maxCrop };\n> +\t}\n> +\n> \treturn 0;\n> }\n> \n> @@ -154,6 +182,20 @@ int V4L2M2MConverter::V4L2M2MStream::queueBuffers(FrameBuffer *input, FrameBuffe\n> \treturn 0;\n> }\n> \n> +int V4L2M2MConverter::V4L2M2MStream::setInputSelection(unsigned int target, Rectangle *rect)\n> +{\n> +\tint ret = m2m_->output()->setSelection(target, rect);\n> +\tif (ret < 0)\n\nWhy the explicit check for < 0 and not if(ret)... that is used elsewhere?\n\n\n\n> +\t\treturn ret;\n> +\n> +\treturn 0;\n> +}\n> +\n> +std::pair V4L2M2MConverter::V4L2M2MStream::inputCropBounds()\n> +{\n> +\treturn inputCropBounds_;\n> +}\n> +\n> std::string V4L2M2MConverter::V4L2M2MStream::logPrefix() const\n> {\n> \treturn stream_->configuration().toString();\n> @@ -204,6 +246,34 @@ V4L2M2MConverter::V4L2M2MConverter(MediaDevice *media)\n> \t\tm2m_.reset();\n> \t\treturn;\n> \t}\n> +\n> +\t/* Discover Feature::InputCrop */\n> +\tRectangle maxCrop;\n> +\tmaxCrop.width = UINT_MAX;\n> +\tmaxCrop.height = UINT_MAX;\n> +\n> +\tret = m2m_->output()->setSelection(V4L2_SEL_TGT_CROP, &maxCrop);\n> +\tif (!ret) {\n\nYou could early out and save an indentation with \n\nif (ret)\n\treturn;\n\nWith or without the changes:\nReviewed-by: Stefan Klug \n\nCheers,\nStefan\n\n> +\t\t/*\n> +\t\t * Rectangles for cropping targets are defined even if the device\n> +\t\t * does not support cropping. Their sizes and positions will be\n> +\t\t * fixed in such cases.\n> +\t\t *\n> +\t\t * Set and inspect a crop equivalent to half of the maximum crop\n> +\t\t * returned earlier. Use this to determine whether the crop on\n> +\t\t * input is really supported.\n> +\t\t */\n> +\t\tRectangle halfCrop(maxCrop.size() / 2);\n> +\n> +\t\tret = m2m_->output()->setSelection(V4L2_SEL_TGT_CROP, &halfCrop);\n> +\t\tif (!ret && halfCrop.width != maxCrop.width &&\n> +\t\t halfCrop.height != maxCrop.height) {\n> +\t\t\tfeatures_ |= Feature::InputCrop;\n> +\n> +\t\t\tLOG(Converter, Info)\n> +\t\t\t\t<< \"Converter supports cropping on its input\";\n> +\t\t}\n> +\t}\n> }\n> \n> /**\n> @@ -336,7 +406,7 @@ int V4L2M2MConverter::configure(const StreamConfiguration &inputCfg,\n> \tfor (unsigned int i = 0; i < outputCfgs.size(); ++i) {\n> \t\tconst StreamConfiguration &cfg = outputCfgs[i];\n> \t\tstd::unique_ptr stream =\n> -\t\t\tstd::make_unique(this, cfg.stream());\n> +\t\t\tstd::make_unique(this, cfg.stream(), features_);\n> \n> \t\tif (!stream->isValid()) {\n> \t\t\tLOG(Converter, Error)\n> @@ -373,6 +443,36 @@ int V4L2M2MConverter::exportBuffers(const Stream *stream, unsigned int count,\n> \treturn iter->second->exportBuffers(count, buffers);\n> }\n> \n> +/**\n> + * \\copydoc libcamera::Converter::setInputCrop\n> + */\n> +int V4L2M2MConverter::setInputCrop(const Stream *stream, Rectangle *rect)\n> +{\n> +\tif (!(features_ & Feature::InputCrop))\n> +\t\treturn -ENOTSUP;\n> +\n> +\tauto iter = streams_.find(stream);\n> +\tif (iter == streams_.end()) {\n> +\t\tLOG(Converter, Error) << \"Invalid output stream\";\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\treturn iter->second->setInputSelection(V4L2_SEL_TGT_CROP, rect);\n> +}\n> +\n> +/**\n> + * \\copydoc libcamera::Converter::inputCropBounds\n> + */\n> +std::pair\n> +V4L2M2MConverter::inputCropBounds(const Stream *stream)\n> +{\n> +\tauto iter = streams_.find(stream);\n> +\tif (iter == streams_.end())\n> +\t\treturn {};\n> +\n> +\treturn iter->second->inputCropBounds();\n> +}\n> +\n> /**\n> * \\copydoc libcamera::Converter::start\n> */\n> -- \n> 2.45.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 0C88FC3257\n\tfor ;\n\tTue, 24 Sep 2024 19:50:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id F208B6350F;\n\tTue, 24 Sep 2024 21:50:35 +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 8057D63500\n\tfor ;\n\tTue, 24 Sep 2024 21:50:34 +0200 (CEST)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:f788:1cdc:64c2:c2c9])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 79168827;\n\tTue, 24 Sep 2024 21:49:07 +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=\"GyqG37Gx\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1727207347;\n\tbh=slM1x2IZ0zSb61DZ3dwGNWwuLe6loKgje20HJNH9z9o=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=GyqG37Gx+Ewr8zZXmwmd7QkBFWN1vXtTgiRWqLSIgu0f8KVtj4Lcv6lHChTiqQMPi\n\tiPEaC9wtQMesxPbMFWUTSDJrk8njEhm0Ab1bcdRmgQYu5LeApFxlssdonoDjdJSstQ\n\tSGrNemEcYMQXVM/bDXqsDlkgTVDDDLvQj/67nZyg=","Date":"Tue, 24 Sep 2024 21:50:31 +0200","From":"Stefan Klug ","To":"Umang Jain ","Cc":"libcamera-devel@lists.libcamera.org, \n\tPaul Elder ","Subject":"Re: [PATCH v7 2/4] libcamera: converter: Add interface to support\n\tcropping capability","Message-ID":"","References":"<20240906063444.32772-1-umang.jain@ideasonboard.com>\n\t<20240906063444.32772-3-umang.jain@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240906063444.32772-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":31358,"web_url":"https://patchwork.libcamera.org/comment/31358/","msgid":"<20240925181900.GE27666@pendragon.ideasonboard.com>","date":"2024-09-25T18:19:00","subject":"Re: [PATCH v7 2/4] libcamera: converter: Add interface to support\n\tcropping capability","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Tue, Sep 24, 2024 at 09:50:31PM +0200, Stefan Klug wrote:\n> Hi Umang,\n> \n> Thank you for the patch. \n> \n> On Fri, Sep 06, 2024 at 12:04:42PM +0530, Umang Jain wrote:\n> > If the converter has cropping capability on its input, the interface\n> > should support it by providing appropriate virtual functions. Provide\n> > Feature::InputCrop in Feature enumeration for the same.\n> > \n> > Provide virtual setInputCrop() and inputCropBounds() interfaces so that\n> > the converter can implement their own cropping functionality.\n\ns/their/its/\n\n> > \n> > The V4L2M2MConverter implements these interfaces of the Converter\n> > interface. Not all V4L2M2M converters will have cropping ability\n> > on its input, hence it needs to discovered during construction time.\n\ns/to discovered during/to be discovered at/\n\n> > If the capability to crop to identified successfully, the cropping\n\ns/to identified/is identified/\n\n> > bounds are determined during configure() time.\n> > \n> > Signed-off-by: Umang Jain \n> > Reviewed-by: Paul Elder \n> > ---\n> > include/libcamera/internal/converter.h | 5 +\n> > .../internal/converter/converter_v4l2_m2m.h | 13 ++-\n> > src/libcamera/converter.cpp | 38 +++++++\n> > .../converter/converter_v4l2_m2m.cpp | 106 +++++++++++++++++-\n> > 4 files changed, 158 insertions(+), 4 deletions(-)\n> > \n> > diff --git a/include/libcamera/internal/converter.h b/include/libcamera/internal/converter.h\n> > index 6623de4d..ffbb6f34 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\tInputCrop = (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 setInputCrop(const Stream *stream, Rectangle *rect) = 0;\n> > +\tvirtual std::pair inputCropBounds(const Stream *stream) = 0;\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 b9e59899..7d321c85 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,11 +58,15 @@ public:\n> > \tint queueBuffers(FrameBuffer *input,\n> > \t\t\t const std::map &outputs);\n> > \n> > +\tint setInputCrop(const Stream *stream, Rectangle *rect);\n> > +\tstd::pair inputCropBounds(const Stream *stream);\n> > +\n> > private:\n> > \tclass V4L2M2MStream : protected Loggable\n> > \t{\n> > \tpublic:\n> > -\t\tV4L2M2MStream(V4L2M2MConverter *converter, const Stream *stream);\n> > +\t\tV4L2M2MStream(V4L2M2MConverter *converter, const Stream *stream,\n> > +\t\t\t Features features);\n> > \n> > \t\tbool isValid() const { return m2m_ != nullptr; }\n> > \n> > @@ -75,6 +80,9 @@ private:\n> > \n> > \t\tint queueBuffers(FrameBuffer *input, FrameBuffer *output);\n> > \n> > +\t\tint setInputSelection(unsigned int target, Rectangle *rect);\n> > +\t\tstd::pair inputCropBounds();\n> > +\n> > \tprotected:\n> > \t\tstd::string logPrefix() const override;\n> > \n> > @@ -88,6 +96,9 @@ private:\n> > \n> > \t\tunsigned int inputBufferCount_;\n> > \t\tunsigned int outputBufferCount_;\n> > +\n> > +\t\tstd::pair inputCropBounds_;\n> > +\t\tFeatures features_;\n> > \t};\n> > \n> > \tstd::unique_ptr m2m_;\n> > diff --git a/src/libcamera/converter.cpp b/src/libcamera/converter.cpp\n> > index d7bb7273..7cb6532a 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::InputCrop\n> > + * \\brief Cropping capability at input is supported by the converter\n> > */\n> > \n> > /**\n> > @@ -161,6 +165,40 @@ Converter::~Converter()\n> > * \\return 0 on success or a negative error code otherwise\n> > */\n> > \n> > +/**\n> > + * \\fn Converter::setInputCrop()\n> > + * \\brief Set the crop rectangle \\a rect for \\a stream\n> > + * \\param[in] stream Pointer to output stream\n\ns/Pointer to/The/\n\n(consistent with the documentation below)\n\n> > + * \\param[inout] rect The crop rectangle to apply and return the rectangle\n> > + * that is actually applied\n> > + *\n> > + * Set the crop rectangle \\a rect for \\a stream provided the converter supports\n> > + * cropping. The converter has the Feature::InputCrop flag in this case.\n> > + *\n> > + * The underlying hardware can adjust the rectangle supplied by the user\n> > + * due to hardware constraints. Caller can inspect \\a rect to determine the\n\ns/Caller/The caller/\n\n> > + * actual rectangle that has been applied by the converter, after this function\n> > + * returns.\n> > + *\n> > + * \\return 0 on success or a negative error code otherwise\n> > + */\n> > +\n> > +/**\n> > + * \\fn Converter::inputCropBounds()\n> > + * \\brief Retrieve the crop bounds for \\a stream\n> > + * \\param[in] stream The output stream\n> > + *\n> > + * Retrieve the minimum and maximum crop bounds for \\a stream. The converter\n> > + * should support cropping (Feature::InputCrop).\n> > + *\n> > + * The crop bounds depend on the configuration of the output stream hence, this\n\ns/hence, this/and hence this/\n\n> > + * function should be called after the \\a stream has been configured using\n> > + * configure().\n> > + *\n> > + * \\return A std::pair containing minimum and maximum\n\ns/std::pair/pair/ (the type is already indicated\nby the function prototype).\n\ns/containing/containing the/\n\n> > + * crop bound respectively.\n\ns/respectively/in that order/\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 4f3e8ce4..1bf47ff9 100644\n> > --- a/src/libcamera/converter/converter_v4l2_m2m.cpp\n> > +++ b/src/libcamera/converter/converter_v4l2_m2m.cpp\n> > @@ -34,8 +34,9 @@ LOG_DECLARE_CATEGORY(Converter)\n> > * V4L2M2MConverter::V4L2M2MStream\n> > */\n> > \n> > -V4L2M2MConverter::V4L2M2MStream::V4L2M2MStream(V4L2M2MConverter *converter, const Stream *stream)\n> > -\t: converter_(converter), stream_(stream)\n> > +V4L2M2MConverter::V4L2M2MStream::V4L2M2MStream(V4L2M2MConverter *converter, const Stream *stream,\n> > +\t\t\t\t\t Features features)\n> > +\t: converter_(converter), stream_(stream), features_(features)\n> > {\n> > \tm2m_ = std::make_unique(converter->deviceNode());\n> > \n> > @@ -97,6 +98,33 @@ int V4L2M2MConverter::V4L2M2MStream::configure(const StreamConfiguration &inputC\n> > \tinputBufferCount_ = inputCfg.bufferCount;\n> > \toutputBufferCount_ = outputCfg.bufferCount;\n> > \n> > +\tif (features_ & Feature::InputCrop) {\n> \n> I don't fully understand why the stream has it's own features_ member.\n> Wouldn't it be sufficient to do\n> \n> if(converter_->features() & Feature::InputCrop) {\n> \n> or did I miss something?\n\nI would also drop the member variable.\n\n> > +\t\tRectangle minCrop;\n> > +\t\tRectangle maxCrop;\n> > +\n> > +\t\t/* Find crop bounds */\n> > +\t\tminCrop.width = 1;\n> > +\t\tminCrop.height = 1;\n> > +\t\tmaxCrop.width = UINT_MAX;\n> > +\t\tmaxCrop.height = UINT_MAX;\n> > +\n> > +\t\tret = setInputSelection(V4L2_SEL_TGT_CROP, &minCrop);\n> > +\t\tif (ret) {\n> > +\t\t\tLOG(Converter, Error) << \"Could not query minimum selection crop\"\n\ns/crop\"/crop: \"/\n\nBut it should be formatted as\n\n\t\t\tLOG(Converter, Error)\n\t\t\t\t<< \"Could not query minimum selection crop: \"\n\t\t\t\t<< strerror(-ret);\n\n> > +\t\t\t\t\t << strerror(-ret);\n> > +\t\t\treturn ret;\n> > +\t\t}\n> > +\n> > +\t\tret = setInputSelection(V4L2_SEL_TGT_CROP, &maxCrop);\n\nShouldn't this be instead implemented by getting\nV4L2_SEL_TGT_CROP_BOUNDS ? I've sent a patch that adds support for\nV4L2VideoDevice::getSelection() (\"[PATCH] libcamera: v4l2_videodevice:\nAdd getSelection() function\").\n\n> > +\t\tif (ret) {\n> > +\t\t\tLOG(Converter, Error) << \"Could not query maximum selection crop\"\n\nSame here.\n\n> > +\t\t\t\t\t << strerror(-ret);\n> > +\t\t\treturn ret;\n> > +\t\t}\n> > +\n> > +\t\tinputCropBounds_ = { minCrop, maxCrop };\n> > +\t}\n> > +\n> > \treturn 0;\n> > }\n> > \n> > @@ -154,6 +182,20 @@ int V4L2M2MConverter::V4L2M2MStream::queueBuffers(FrameBuffer *input, FrameBuffe\n> > \treturn 0;\n> > }\n> > \n> > +int V4L2M2MConverter::V4L2M2MStream::setInputSelection(unsigned int target, Rectangle *rect)\n> > +{\n> > +\tint ret = m2m_->output()->setSelection(target, rect);\n> > +\tif (ret < 0)\n> \n> Why the explicit check for < 0 and not if(ret)... that is used elsewhere?\n\nYou could simply\n\n\treturn m2m_->output()->setSelection(target, rect);\n\n> > +\t\treturn ret;\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +std::pair V4L2M2MConverter::V4L2M2MStream::inputCropBounds()\n> > +{\n> > +\treturn inputCropBounds_;\n> > +}\n> > +\n> > std::string V4L2M2MConverter::V4L2M2MStream::logPrefix() const\n> > {\n> > \treturn stream_->configuration().toString();\n> > @@ -204,6 +246,34 @@ V4L2M2MConverter::V4L2M2MConverter(MediaDevice *media)\n> > \t\tm2m_.reset();\n> > \t\treturn;\n> > \t}\n> > +\n> > +\t/* Discover Feature::InputCrop */\n> > +\tRectangle maxCrop;\n> > +\tmaxCrop.width = UINT_MAX;\n> > +\tmaxCrop.height = UINT_MAX;\n> > +\n> > +\tret = m2m_->output()->setSelection(V4L2_SEL_TGT_CROP, &maxCrop);\n> > +\tif (!ret) {\n> \n> You could early out and save an indentation with \n> \n> if (ret)\n> \treturn;\n> \n> With or without the changes:\n> Reviewed-by: Stefan Klug \n> \n> Cheers,\n> Stefan\n> \n> > +\t\t/*\n> > +\t\t * Rectangles for cropping targets are defined even if the device\n> > +\t\t * does not support cropping. Their sizes and positions will be\n> > +\t\t * fixed in such cases.\n> > +\t\t *\n> > +\t\t * Set and inspect a crop equivalent to half of the maximum crop\n> > +\t\t * returned earlier. Use this to determine whether the crop on\n> > +\t\t * input is really supported.\n> > +\t\t */\n> > +\t\tRectangle halfCrop(maxCrop.size() / 2);\n> > +\n> > +\t\tret = m2m_->output()->setSelection(V4L2_SEL_TGT_CROP, &halfCrop);\n> > +\t\tif (!ret && halfCrop.width != maxCrop.width &&\n> > +\t\t halfCrop.height != maxCrop.height) {\n\nHow about\n\n\t\tif (!ret && halfCrop != maxCrop) {\n\n> > +\t\t\tfeatures_ |= Feature::InputCrop;\n> > +\n> > +\t\t\tLOG(Converter, Info)\n> > +\t\t\t\t<< \"Converter supports cropping on its input\";\n> > +\t\t}\n> > +\t}\n> > }\n> > \n> > /**\n> > @@ -336,7 +406,7 @@ int V4L2M2MConverter::configure(const StreamConfiguration &inputCfg,\n> > \tfor (unsigned int i = 0; i < outputCfgs.size(); ++i) {\n> > \t\tconst StreamConfiguration &cfg = outputCfgs[i];\n> > \t\tstd::unique_ptr stream =\n> > -\t\t\tstd::make_unique(this, cfg.stream());\n> > +\t\t\tstd::make_unique(this, cfg.stream(), features_);\n> > \n> > \t\tif (!stream->isValid()) {\n> > \t\t\tLOG(Converter, Error)\n> > @@ -373,6 +443,36 @@ int V4L2M2MConverter::exportBuffers(const Stream *stream, unsigned int count,\n> > \treturn iter->second->exportBuffers(count, buffers);\n> > }\n> > \n> > +/**\n> > + * \\copydoc libcamera::Converter::setInputCrop\n> > + */\n> > +int V4L2M2MConverter::setInputCrop(const Stream *stream, Rectangle *rect)\n> > +{\n> > +\tif (!(features_ & Feature::InputCrop))\n> > +\t\treturn -ENOTSUP;\n> > +\n> > +\tauto iter = streams_.find(stream);\n> > +\tif (iter == streams_.end()) {\n> > +\t\tLOG(Converter, Error) << \"Invalid output stream\";\n> > +\t\treturn -EINVAL;\n> > +\t}\n> > +\n> > +\treturn iter->second->setInputSelection(V4L2_SEL_TGT_CROP, rect);\n> > +}\n> > +\n> > +/**\n> > + * \\copydoc libcamera::Converter::inputCropBounds\n> > + */\n> > +std::pair\n> > +V4L2M2MConverter::inputCropBounds(const Stream *stream)\n> > +{\n> > +\tauto iter = streams_.find(stream);\n> > +\tif (iter == streams_.end())\n> > +\t\treturn {};\n> > +\n> > +\treturn iter->second->inputCropBounds();\n> > +}\n> > +\n> > /**\n> > * \\copydoc libcamera::Converter::start\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 18822C0F1B\n\tfor ;\n\tWed, 25 Sep 2024 18:19:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 504906350E;\n\tWed, 25 Sep 2024 20:19:04 +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 CBC56634F4\n\tfor ;\n\tWed, 25 Sep 2024 20:19:02 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id BA9AE5B2;\n\tWed, 25 Sep 2024 20:17:34 +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=\"DLyiOFea\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1727288255;\n\tbh=XxyUNnuu8MghMgYXMTAyABMBk/6JaG3zUWMfUPyij3o=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=DLyiOFea/XNXssp6rg0I4iXl8nZGrJhNCd1Uvg+P2Tc/Qrs2hTs/YmnuZXQO1ySnm\n\tMuKnv3oX1ycY9vzAsHV6AAZjdutdcEfauwuPDWqXjUI4df8sfCB8mnVF0r3xSEO9Fo\n\t5HQ2puTnBeVmUZ6TpaMiVptbJY54bUOdXO7OP1KY=","Date":"Wed, 25 Sep 2024 21:19:00 +0300","From":"Laurent Pinchart ","To":"Stefan Klug ","Cc":"Umang Jain ,\n\tlibcamera-devel@lists.libcamera.org,\n\tPaul Elder ","Subject":"Re: [PATCH v7 2/4] libcamera: converter: Add interface to support\n\tcropping capability","Message-ID":"<20240925181900.GE27666@pendragon.ideasonboard.com>","References":"<20240906063444.32772-1-umang.jain@ideasonboard.com>\n\t<20240906063444.32772-3-umang.jain@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","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\" "}}]