[{"id":30548,"web_url":"https://patchwork.libcamera.org/comment/30548/","msgid":"<20240802203320.GA3295@pendragon.ideasonboard.com>","date":"2024-08-02T20:33:20","subject":"Re: [PATCH v6 2/5] 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":"Hi Umang,\n\nThank you for the patch.\n\nOn Fri, Jul 26, 2024 at 05:17:12PM +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 <umang.jain@ideasonboard.com>\n> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>\n> ---\n>  include/libcamera/internal/converter.h        |  5 +\n>  .../internal/converter/converter_v4l2_m2m.h   |  6 ++\n>  src/libcamera/converter.cpp                   | 52 +++++++++++\n>  .../converter/converter_v4l2_m2m.cpp          | 92 +++++++++++++++++++\n>  4 files changed, 155 insertions(+)\n> \n> diff --git a/include/libcamera/internal/converter.h b/include/libcamera/internal/converter.h\n> index 652ff519..853888f4 100644\n> --- a/include/libcamera/internal/converter.h\n> +++ b/include/libcamera/internal/converter.h\n> @@ -14,6 +14,7 @@\n>  #include <memory>\n>  #include <string>\n>  #include <tuple>\n> +#include <utility>\n>  #include <vector>\n>  \n>  #include <libcamera/base/class.h>\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<Feature>;\n> @@ -63,6 +65,9 @@ public:\n>  \tvirtual int queueBuffers(FrameBuffer *input,\n>  \t\t\t\t const std::map<const Stream *, FrameBuffer *> &outputs) = 0;\n>  \n> +\tvirtual int setInputCrop(const Stream *stream, Rectangle *rect);\n> +\tvirtual std::pair<Rectangle, Rectangle> inputCropBounds(const Stream *stream);\n> +\n>  \tSignal<FrameBuffer *> inputBufferReady;\n>  \tSignal<FrameBuffer *> 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..5b69b14e 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<const Stream *, FrameBuffer *> &outputs);\n>  \n> +\tint setInputCrop(const Stream *stream, Rectangle *rect);\n> +\tstd::pair<Rectangle, Rectangle> inputCropBounds(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 e2dbf5e0..7ab56b4c 100644\n> --- a/src/libcamera/converter.cpp\n> +++ b/src/libcamera/converter.cpp\n> @@ -11,6 +11,8 @@\n>  \n>  #include <libcamera/base/log.h>\n>  \n> +#include <libcamera/stream.h>\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,54 @@ 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\nThere's no explanation of the \"out\" side of \"inout\" in the text here.\n\n> + *\n> + * Set the crop rectangle \\a rect for \\a stream provided the converter supports\n> + * cropping. The converter should have the Feature::InputCrop flag in this\n\n\"should\" is not a mandatory requirement.\n\n> + * case.\n> + *\n> + * \\return 0 on success or a negative error code otherwise\n> + */\n> +int Converter::setInputCrop([[maybe_unused]] const Stream *stream, [[maybe_unused]] Rectangle *rect)\n> +{\n> +\tif (!(features() & Feature::InputCrop)) {\n> +\t\tLOG(Converter, Error) << \"Converter doesn't support cropping capabilities\";\n> +\t\treturn -ENOTSUP;\n> +\t}\n> +\n> +\treturn 0;\n\nWhat's the purpose of this default implementation ?\n\n> +}\n> +\n> +/**\n> + * \\brief Get the crop bounds \\a stream\n\ns/Get/Retrieve/\ns/bounds/bounds for/\n\n> + * \\param[in] stream Pointer to output stream\n\ns/Pointer to output stream/The output stream/\n\n> + *\n> + * Get the minimum and maximum crop bounds for \\a stream. The converter\n> + * should supporting cropping (Feature::InputCrop).\n\ns/supporting/support/\n\n> + *\n> + * \\return A std::pair<Rectangle, Rectangle> containining minimum and maximum\n> + * crop bound respectively.\n> + */\n> +std::pair<Rectangle, Rectangle> Converter::inputCropBounds([[maybe_unused]] const Stream *stream)\n> +{\n> +\tconst StreamConfiguration &config = stream->configuration();\n> +\tRectangle rect;\n> +\n> +\tif (!(features() & Feature::InputCrop))\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\nWhy ?\n\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..3295b988 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,85 @@ 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\treturn -EINVAL;\n> +\n> +\treturn iter->second->setSelection(V4L2_SEL_TGT_CROP, rect);\n> +}\n> +\n> +/**\n> + * \\copydoc libcamera::Converter::inputCropBounds\n> + */\n> +std::pair<Rectangle, Rectangle>\n> +V4L2M2MConverter::inputCropBounds(const Stream *stream)\n> +{\n> +\tRectangle minCrop;\n> +\tRectangle maxCrop;\n> +\tint ret;\n> +\n> +\tif (!(features() & Feature::InputCrop)) {\n> +\t\tLOG(Converter, Error) << \"Input Crop functionality is not supported\";\n> +\t\treturn {};\n> +\t}\n> +\n> +\tminCrop.width = 1;\n> +\tminCrop.height = 1;\n> +\tmaxCrop.width = UINT_MAX;\n> +\tmaxCrop.height = UINT_MAX;\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\t\t\t      << strerror(-ret);\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\t\t\t      << strerror(-ret);\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\n\"from according to\" ?\n\n> +\t * stream configuration.\n> +\t */\n\nThe behaviour is ill-defined. The documentation of the inputCropBounds()\nfunction makes no mention of the fact that the crop bounds will vary\ndepending on the stream configuration.\n\n> +\tret = setInputCrop(stream, &minCrop);\n\nModifying the active crop rectangle when querying the bounds isn't a\ngood idea. It's an unexpected side effect when the converter isn't\nstreaming yet, and will mess things up when it is.\n\n> +\tif (ret) {\n> +\t\tLOG(Converter, Error) << \"Failed to query minimum crop bound\"\n> +\t\t\t\t      << strerror(-ret);\n> +\t\treturn {};\n> +\t}\n> +\n> +\tret = setInputCrop(stream, &maxCrop);\n> +\tif (ret) {\n> +\t\tLOG(Converter, Error) << \"Failed to query maximum crop bound\"\n> +\t\t\t\t      << strerror(-ret);\n> +\t\treturn {};\n> +\t}\n> +\n> +\treturn { minCrop, maxCrop };\n> +}\n> +\n>  /**\n>   * \\copydoc libcamera::Converter::start\n>   */\n> @@ -448,6 +536,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\nDoes this belong to patch 1/5 ?\n\n>  static std::initializer_list<std::string> compatibles = {\n>  \t\"mtk-mdp\",\n>  \t\"pxp\",","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 8571EBDC71\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri,  2 Aug 2024 20:33:44 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 89B6463381;\n\tFri,  2 Aug 2024 22:33:43 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 272C76336B\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri,  2 Aug 2024 22:33:42 +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 4CB8C2D5;\n\tFri,  2 Aug 2024 22:32:52 +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=\"e2MCqn4G\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1722630772;\n\tbh=CecDP1l8uX483kRXN4whzs20cBRVobcYPe/8X116OvQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=e2MCqn4GHe39iA+e9SGLdOFdxdIbkwXnHr1xceP134IKIq6u48Irx5M304FqKe2DP\n\tt5GorIFxjbVl8C4jEVv4218EoSbud5ls/ba07F/JBPXFubtMJE2hwTnPK8S3YFem0N\n\tXCuH8FrAsIZJMqoiz51QfDhOyuWk6BdBgZoMtOJ4=","Date":"Fri, 2 Aug 2024 23:33:20 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Umang Jain <umang.jain@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org,\n\tPaul Elder <paul.elder@ideasonboard.com>","Subject":"Re: [PATCH v6 2/5] libcamera: converter: Add interface to support\n\tcropping capability","Message-ID":"<20240802203320.GA3295@pendragon.ideasonboard.com>","References":"<20240726114715.226468-1-umang.jain@ideasonboard.com>\n\t<20240726114715.226468-3-umang.jain@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240726114715.226468-3-umang.jain@ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]