[{"id":32367,"web_url":"https://patchwork.libcamera.org/comment/32367/","msgid":"","date":"2024-11-25T18:36:50","subject":"Re: [PATCH v2 6/8] libcamera: converter_v4l2_m2m: Improve crop\n\tbounds support","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"On Mon, Nov 25, 2024 at 04:14:15PM +0100, Stefan Klug wrote:\n> When a converter (dw100 on imx8mp in this case) is used in a pipeline,\n> the ScalerCrop control get's created at createCamera() time. As this is\n\nKind of unrelated to this patch, right ? :)\n\n> before configure(), no streams were configured on the converter and the\n> stream specific crop bounds are not known. Extend the converter class to\n> report the default crop bounds in case the provided stream is not found.\n>\n> Signed-off-by: Stefan Klug \n> ---\n> Changes in v2:\n> - Added this patch\n> ---\n> .../internal/converter/converter_v4l2_m2m.h | 1 +\n> src/libcamera/converter.cpp | 3 +\n> .../converter/converter_v4l2_m2m.cpp | 113 +++++++++---------\n> 3 files changed, 59 insertions(+), 58 deletions(-)\n>\n> diff --git a/include/libcamera/internal/converter/converter_v4l2_m2m.h b/include/libcamera/internal/converter/converter_v4l2_m2m.h\n> index 0bc0d053e2c4..a5286166f574 100644\n> --- a/include/libcamera/internal/converter/converter_v4l2_m2m.h\n> +++ b/include/libcamera/internal/converter/converter_v4l2_m2m.h\n> @@ -105,6 +105,7 @@ private:\n>\n> \tstd::map> streams_;\n> \tstd::map queue_;\n> +\tstd::pair inputCropBounds_;\n> };\n>\n> } /* namespace libcamera */\n> diff --git a/src/libcamera/converter.cpp b/src/libcamera/converter.cpp\n> index 945f2527b96a..d4b5e5260e02 100644\n> --- a/src/libcamera/converter.cpp\n> +++ b/src/libcamera/converter.cpp\n> @@ -195,6 +195,9 @@ Converter::~Converter()\n> * this function should be called after the \\a stream has been configured using\n> * configure().\n> *\n> + * When called with an invalid \\a stream, the function returns the default crop\n> + * bounds of the converter.\n> + *\n> * \\return A pair containing the minimum and maximum crop bound in that order\n> */\n>\n> diff --git a/src/libcamera/converter/converter_v4l2_m2m.cpp b/src/libcamera/converter/converter_v4l2_m2m.cpp\n> index d63ef2f8028f..bd7e5cce600d 100644\n> --- a/src/libcamera/converter/converter_v4l2_m2m.cpp\n> +++ b/src/libcamera/converter/converter_v4l2_m2m.cpp\n> @@ -30,6 +30,52 @@ namespace libcamera {\n>\n> LOG_DECLARE_CATEGORY(Converter)\n>\n> +namespace {\n> +\n> +int getCropBounds(V4L2VideoDevice *device, Rectangle &minCrop,\n> +\t\t Rectangle &maxCrop)\n> +{\n> +\tRectangle minC;\n> +\tRectangle maxC;\n> +\n> +\t/* Find crop bounds */\n> +\tminC.width = 1;\n> +\tminC.height = 1;\n> +\tmaxC.width = UINT_MAX;\n> +\tmaxC.height = UINT_MAX;\n> +\n> +\tint ret = device->setSelection(V4L2_SEL_TGT_CROP, &minC);\n> +\tif (ret) {\n> +\t\tLOG(Converter, Error)\n> +\t\t\t<< \"Could not query minimum selection crop: \"\n> +\t\t\t<< strerror(-ret);\n> +\t\treturn ret;\n> +\t}\n> +\n> +\tret = device->getSelection(V4L2_SEL_TGT_CROP_BOUNDS, &maxC);\n> +\tif (ret) {\n> +\t\tLOG(Converter, Error)\n> +\t\t\t<< \"Could not query maximum selection crop: \"\n> +\t\t\t<< strerror(-ret);\n> +\t\treturn ret;\n> +\t}\n> +\n> +\t/* Reset the crop to its maximum */\n> +\tret = device->setSelection(V4L2_SEL_TGT_CROP, &maxC);\n> +\tif (ret) {\n> +\t\tLOG(Converter, Error)\n> +\t\t\t<< \"Could not reset selection crop: \"\n> +\t\t\t<< strerror(-ret);\n> +\t\treturn ret;\n> +\t}\n> +\n> +\tminCrop = minC;\n> +\tmaxCrop = maxC;\n> +\treturn 0;\n> +}\n> +\n> +} /* namespace */\n> +\n> /* -----------------------------------------------------------------------------\n> * V4L2M2MConverter::V4L2M2MStream\n> */\n> @@ -98,41 +144,10 @@ int V4L2M2MConverter::V4L2M2MStream::configure(const StreamConfiguration &inputC\n> \toutputBufferCount_ = outputCfg.bufferCount;\n>\n> \tif (converter_->features() & Feature::InputCrop) {\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)\n> -\t\t\t\t<< \"Could not query minimum selection crop: \"\n> -\t\t\t\t<< strerror(-ret);\n> -\t\t\treturn ret;\n> -\t\t}\n> -\n> -\t\tret = getInputSelection(V4L2_SEL_TGT_CROP_BOUNDS, &maxCrop);\n> -\t\tif (ret) {\n> -\t\t\tLOG(Converter, Error)\n> -\t\t\t\t<< \"Could not query maximum selection crop: \"\n> -\t\t\t\t<< strerror(-ret);\n> -\t\t\treturn ret;\n> -\t\t}\n> -\n> -\t\t/* Reset the crop to its maximum */\n> -\t\tret = setInputSelection(V4L2_SEL_TGT_CROP, &maxCrop);\n> -\t\tif (ret) {\n> -\t\t\tLOG(Converter, Error)\n> -\t\t\t\t<< \"Could not reset selection crop: \"\n> -\t\t\t\t<< strerror(-ret);\n\nMaybe I don't understand M2M enough, but is there of Stream-specific\nhere ?\n\nThere's an output and an input queue, right ? Do they have per-stream\ncrop bounds ??\n\n> +\t\tret = getCropBounds(m2m_->output(), inputCropBounds_.first,\n> +\t\t\t\t inputCropBounds_.second);\n> +\t\tif (ret)\n> \t\t\treturn ret;\n> -\t\t}\n> -\n> -\t\tinputCropBounds_ = { minCrop, maxCrop };\n> \t}\n>\n> \treturn 0;\n> @@ -258,27 +273,9 @@ V4L2M2MConverter::V4L2M2MConverter(MediaDevice *media)\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> -\t\treturn;\n> -\n> -\t/*\n> -\t * Rectangles for cropping targets are defined even if the device\n> -\t * does not support cropping. Their sizes and positions will be\n> -\t * fixed in such cases.\n> -\t *\n> -\t * Set and inspect a crop equivalent to half of the maximum crop\n> -\t * returned earlier. Use this to determine whether the crop on\n> -\t * input is really supported.\n> -\t */\n> -\tRectangle halfCrop(maxCrop.size() / 2);\n> -\tret = m2m_->output()->setSelection(V4L2_SEL_TGT_CROP, &halfCrop);\n> -\tif (!ret && halfCrop != maxCrop) {\n> +\tret = getCropBounds(m2m_->output(), inputCropBounds_.first,\n> +\t\t\t inputCropBounds_.second);\n> +\tif (!ret && inputCropBounds_.first != inputCropBounds_.second) {\n> \t\tfeatures_ |= Feature::InputCrop;\n>\n> \t\tLOG(Converter, Info)\n> @@ -477,10 +474,10 @@ std::pair\n> V4L2M2MConverter::inputCropBounds(const Stream *stream)\n> {\n> \tauto iter = streams_.find(stream);\n> -\tif (iter == streams_.end())\n> -\t\treturn {};\n> +\tif (iter != streams_.end())\n> +\t\treturn iter->second->inputCropBounds();\n>\n> -\treturn iter->second->inputCropBounds();\n> +\treturn inputCropBounds_;\n> }\n>\n> /**\n> --\n> 2.43.0\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id AC181C3274\n\tfor ;\n\tMon, 25 Nov 2024 18:36:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 266CD65F81;\n\tMon, 25 Nov 2024 19:36:56 +0100 (CET)","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 EC6A965F66\n\tfor ;\n\tMon, 25 Nov 2024 19:36:54 +0100 (CET)","from ideasonboard.com (mob-5-90-139-188.net.vodafone.it\n\t[5.90.139.188])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id DB07F6B5;\n\tMon, 25 Nov 2024 19:36:32 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"dbZgd6mY\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1732559793;\n\tbh=5qtT5rXgucNlr0EOOlBW4mtaK4gFu8Gw3g88N7x3uHk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=dbZgd6mYNFbcu+pUzzv4l0533WHfZcMnpYzx4CFKjeq0MfgBPshBQvNwTlAotZQga\n\tH5ShetQ1jzbebGiKpqcmVUzc4GDrRLfpHx3r15bAhLIeO73CnX6+8syPffA9AhhZ5G\n\taG6moHoDQ516W+Ei9l0DZ2uqDKZogF/DcdatVfE4=","Date":"Mon, 25 Nov 2024 19:36:50 +0100","From":"Jacopo Mondi ","To":"Stefan Klug ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v2 6/8] libcamera: converter_v4l2_m2m: Improve crop\n\tbounds support","Message-ID":"","References":"<20241125151430.2437285-1-stefan.klug@ideasonboard.com>\n\t<20241125151430.2437285-7-stefan.klug@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20241125151430.2437285-7-stefan.klug@ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":32426,"web_url":"https://patchwork.libcamera.org/comment/32426/","msgid":"","date":"2024-11-28T13:04:12","subject":"Re: [PATCH v2 6/8] libcamera: converter_v4l2_m2m: Improve crop\n\tbounds support","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"On Mon, Nov 25, 2024 at 07:36:50PM +0100, Jacopo Mondi wrote:\n> On Mon, Nov 25, 2024 at 04:14:15PM +0100, Stefan Klug wrote:\n> > When a converter (dw100 on imx8mp in this case) is used in a pipeline,\n> > the ScalerCrop control get's created at createCamera() time. As this is\n> \n> Kind of unrelated to this patch, right ? :)\n\nA bit maybe. Without it, I would ask myself: Why would anyone ask for\ncropBounds before configuring the coverter?\n\nHow would you phrase that?\n\n> \n> > before configure(), no streams were configured on the converter and the\n> > stream specific crop bounds are not known. Extend the converter class to\n> > report the default crop bounds in case the provided stream is not found.\n> >\n> > Signed-off-by: Stefan Klug \n> > ---\n> > Changes in v2:\n> > - Added this patch\n> > ---\n> > .../internal/converter/converter_v4l2_m2m.h | 1 +\n> > src/libcamera/converter.cpp | 3 +\n> > .../converter/converter_v4l2_m2m.cpp | 113 +++++++++---------\n> > 3 files changed, 59 insertions(+), 58 deletions(-)\n> >\n> > diff --git a/include/libcamera/internal/converter/converter_v4l2_m2m.h b/include/libcamera/internal/converter/converter_v4l2_m2m.h\n> > index 0bc0d053e2c4..a5286166f574 100644\n> > --- a/include/libcamera/internal/converter/converter_v4l2_m2m.h\n> > +++ b/include/libcamera/internal/converter/converter_v4l2_m2m.h\n> > @@ -105,6 +105,7 @@ private:\n> >\n> > \tstd::map> streams_;\n> > \tstd::map queue_;\n> > +\tstd::pair inputCropBounds_;\n> > };\n> >\n> > } /* namespace libcamera */\n> > diff --git a/src/libcamera/converter.cpp b/src/libcamera/converter.cpp\n> > index 945f2527b96a..d4b5e5260e02 100644\n> > --- a/src/libcamera/converter.cpp\n> > +++ b/src/libcamera/converter.cpp\n> > @@ -195,6 +195,9 @@ Converter::~Converter()\n> > * this function should be called after the \\a stream has been configured using\n> > * configure().\n> > *\n> > + * When called with an invalid \\a stream, the function returns the default crop\n> > + * bounds of the converter.\n> > + *\n> > * \\return A pair containing the minimum and maximum crop bound in that order\n> > */\n> >\n> > diff --git a/src/libcamera/converter/converter_v4l2_m2m.cpp b/src/libcamera/converter/converter_v4l2_m2m.cpp\n> > index d63ef2f8028f..bd7e5cce600d 100644\n> > --- a/src/libcamera/converter/converter_v4l2_m2m.cpp\n> > +++ b/src/libcamera/converter/converter_v4l2_m2m.cpp\n> > @@ -30,6 +30,52 @@ namespace libcamera {\n> >\n> > LOG_DECLARE_CATEGORY(Converter)\n> >\n> > +namespace {\n> > +\n> > +int getCropBounds(V4L2VideoDevice *device, Rectangle &minCrop,\n> > +\t\t Rectangle &maxCrop)\n> > +{\n> > +\tRectangle minC;\n> > +\tRectangle maxC;\n> > +\n> > +\t/* Find crop bounds */\n> > +\tminC.width = 1;\n> > +\tminC.height = 1;\n> > +\tmaxC.width = UINT_MAX;\n> > +\tmaxC.height = UINT_MAX;\n> > +\n> > +\tint ret = device->setSelection(V4L2_SEL_TGT_CROP, &minC);\n> > +\tif (ret) {\n> > +\t\tLOG(Converter, Error)\n> > +\t\t\t<< \"Could not query minimum selection crop: \"\n> > +\t\t\t<< strerror(-ret);\n> > +\t\treturn ret;\n> > +\t}\n> > +\n> > +\tret = device->getSelection(V4L2_SEL_TGT_CROP_BOUNDS, &maxC);\n> > +\tif (ret) {\n> > +\t\tLOG(Converter, Error)\n> > +\t\t\t<< \"Could not query maximum selection crop: \"\n> > +\t\t\t<< strerror(-ret);\n> > +\t\treturn ret;\n> > +\t}\n> > +\n> > +\t/* Reset the crop to its maximum */\n> > +\tret = device->setSelection(V4L2_SEL_TGT_CROP, &maxC);\n> > +\tif (ret) {\n> > +\t\tLOG(Converter, Error)\n> > +\t\t\t<< \"Could not reset selection crop: \"\n> > +\t\t\t<< strerror(-ret);\n> > +\t\treturn ret;\n> > +\t}\n> > +\n> > +\tminCrop = minC;\n> > +\tmaxCrop = maxC;\n> > +\treturn 0;\n> > +}\n> > +\n> > +} /* namespace */\n> > +\n> > /* -----------------------------------------------------------------------------\n> > * V4L2M2MConverter::V4L2M2MStream\n> > */\n> > @@ -98,41 +144,10 @@ int V4L2M2MConverter::V4L2M2MStream::configure(const StreamConfiguration &inputC\n> > \toutputBufferCount_ = outputCfg.bufferCount;\n> >\n> > \tif (converter_->features() & Feature::InputCrop) {\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)\n> > -\t\t\t\t<< \"Could not query minimum selection crop: \"\n> > -\t\t\t\t<< strerror(-ret);\n> > -\t\t\treturn ret;\n> > -\t\t}\n> > -\n> > -\t\tret = getInputSelection(V4L2_SEL_TGT_CROP_BOUNDS, &maxCrop);\n> > -\t\tif (ret) {\n> > -\t\t\tLOG(Converter, Error)\n> > -\t\t\t\t<< \"Could not query maximum selection crop: \"\n> > -\t\t\t\t<< strerror(-ret);\n> > -\t\t\treturn ret;\n> > -\t\t}\n> > -\n> > -\t\t/* Reset the crop to its maximum */\n> > -\t\tret = setInputSelection(V4L2_SEL_TGT_CROP, &maxCrop);\n> > -\t\tif (ret) {\n> > -\t\t\tLOG(Converter, Error)\n> > -\t\t\t\t<< \"Could not reset selection crop: \"\n> > -\t\t\t\t<< strerror(-ret);\n> \n> Maybe I don't understand M2M enough, but is there of Stream-specific\n> here ?\n> \n> There's an output and an input queue, right ? Do they have per-stream\n> crop bounds ??\n\nI think I don't get the question here. Yes, every context/stream has own\ncrop bounds. What is wrong with that? Or did we settle that in the\ndiscussions?\n\nCheers,\nStefan\n\n> \n> > +\t\tret = getCropBounds(m2m_->output(), inputCropBounds_.first,\n> > +\t\t\t\t inputCropBounds_.second);\n> > +\t\tif (ret)\n> > \t\t\treturn ret;\n> > -\t\t}\n> > -\n> > -\t\tinputCropBounds_ = { minCrop, maxCrop };\n> > \t}\n> >\n> > \treturn 0;\n> > @@ -258,27 +273,9 @@ V4L2M2MConverter::V4L2M2MConverter(MediaDevice *media)\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> > -\t\treturn;\n> > -\n> > -\t/*\n> > -\t * Rectangles for cropping targets are defined even if the device\n> > -\t * does not support cropping. Their sizes and positions will be\n> > -\t * fixed in such cases.\n> > -\t *\n> > -\t * Set and inspect a crop equivalent to half of the maximum crop\n> > -\t * returned earlier. Use this to determine whether the crop on\n> > -\t * input is really supported.\n> > -\t */\n> > -\tRectangle halfCrop(maxCrop.size() / 2);\n> > -\tret = m2m_->output()->setSelection(V4L2_SEL_TGT_CROP, &halfCrop);\n> > -\tif (!ret && halfCrop != maxCrop) {\n> > +\tret = getCropBounds(m2m_->output(), inputCropBounds_.first,\n> > +\t\t\t inputCropBounds_.second);\n> > +\tif (!ret && inputCropBounds_.first != inputCropBounds_.second) {\n> > \t\tfeatures_ |= Feature::InputCrop;\n> >\n> > \t\tLOG(Converter, Info)\n> > @@ -477,10 +474,10 @@ std::pair\n> > V4L2M2MConverter::inputCropBounds(const Stream *stream)\n> > {\n> > \tauto iter = streams_.find(stream);\n> > -\tif (iter == streams_.end())\n> > -\t\treturn {};\n> > +\tif (iter != streams_.end())\n> > +\t\treturn iter->second->inputCropBounds();\n> >\n> > -\treturn iter->second->inputCropBounds();\n> > +\treturn inputCropBounds_;\n> > }\n> >\n> > /**\n> > --\n> > 2.43.0\n> >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 8CD5CBD78E\n\tfor ;\n\tThu, 28 Nov 2024 13:04:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BE69865F67;\n\tThu, 28 Nov 2024 14:04:16 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 54C9565898\n\tfor ;\n\tThu, 28 Nov 2024 14:04:15 +0100 (CET)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:22e0:a94:b035:820])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 69347526;\n\tThu, 28 Nov 2024 14:03:51 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"POtEGWg3\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1732799031;\n\tbh=r5hx6nbZ2MfQ2t9gpJFvMDkbW/GdIlQ1v41DfGKoQA4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=POtEGWg33bUmjrMs+WOzlg16P51qpeuBJ1DH4kAYckkZB04JN94umkkgNX8jqwkSY\n\tQnk02HY2P6wOK4PCGsZVTDZ9LVmZFEd32YQtBE/8xm4t+Itwf7vCWx4ZAz3iAe9OOe\n\ttl03PoTkm95HEJaEZoscFnz+FiKksMUE+TEhOFnk=","Date":"Thu, 28 Nov 2024 14:04:12 +0100","From":"Stefan Klug ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v2 6/8] libcamera: converter_v4l2_m2m: Improve crop\n\tbounds support","Message-ID":"","References":"<20241125151430.2437285-1-stefan.klug@ideasonboard.com>\n\t<20241125151430.2437285-7-stefan.klug@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\" "}},{"id":32432,"web_url":"https://patchwork.libcamera.org/comment/32432/","msgid":"","date":"2024-11-28T13:29:54","subject":"Re: [PATCH v2 6/8] libcamera: converter_v4l2_m2m: Improve crop\n\tbounds support","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Stefan\n\nOn Thu, Nov 28, 2024 at 02:04:12PM +0100, Stefan Klug wrote:\n> On Mon, Nov 25, 2024 at 07:36:50PM +0100, Jacopo Mondi wrote:\n> > On Mon, Nov 25, 2024 at 04:14:15PM +0100, Stefan Klug wrote:\n> > > When a converter (dw100 on imx8mp in this case) is used in a pipeline,\n> > > the ScalerCrop control get's created at createCamera() time. As this is\n> >\n> > Kind of unrelated to this patch, right ? :)\n>\n> A bit maybe. Without it, I would ask myself: Why would anyone ask for\n> cropBounds before configuring the coverter?\n>\n> How would you phrase that?\n>\n\nSomething like:\n\nThe inputCropBounds_ member of the V4L2M2MConverter::Stream class is only\ninitialized after a V4L2M2MConverter::configure() call, when the\nstreams are initialized.\n\nHowever, the converter has crop limits that do not depend on the\nconfigured Streams, and which are currently not accessible from the\nclass interface.\n\nAdd a new inputCropBounds() function to the V4L2M2MConverter class\nthat allows to retrieve the converter crop limits before any stream\nis configured.\n\nThis is particularly useful for pipelines to initialize controls and\nproperties and to implement validation before the Camera gets\nconfigured.\n\n> >\n> > > before configure(), no streams were configured on the converter and the\n> > > stream specific crop bounds are not known. Extend the converter class to\n> > > report the default crop bounds in case the provided stream is not found.\n> > >\n> > > Signed-off-by: Stefan Klug \n> > > ---\n> > > Changes in v2:\n> > > - Added this patch\n> > > ---\n> > > .../internal/converter/converter_v4l2_m2m.h | 1 +\n> > > src/libcamera/converter.cpp | 3 +\n> > > .../converter/converter_v4l2_m2m.cpp | 113 +++++++++---------\n> > > 3 files changed, 59 insertions(+), 58 deletions(-)\n> > >\n> > > diff --git a/include/libcamera/internal/converter/converter_v4l2_m2m.h b/include/libcamera/internal/converter/converter_v4l2_m2m.h\n> > > index 0bc0d053e2c4..a5286166f574 100644\n> > > --- a/include/libcamera/internal/converter/converter_v4l2_m2m.h\n> > > +++ b/include/libcamera/internal/converter/converter_v4l2_m2m.h\n> > > @@ -105,6 +105,7 @@ private:\n> > >\n> > > \tstd::map> streams_;\n> > > \tstd::map queue_;\n> > > +\tstd::pair inputCropBounds_;\n> > > };\n> > >\n> > > } /* namespace libcamera */\n> > > diff --git a/src/libcamera/converter.cpp b/src/libcamera/converter.cpp\n> > > index 945f2527b96a..d4b5e5260e02 100644\n> > > --- a/src/libcamera/converter.cpp\n> > > +++ b/src/libcamera/converter.cpp\n> > > @@ -195,6 +195,9 @@ Converter::~Converter()\n> > > * this function should be called after the \\a stream has been configured using\n> > > * configure().\n> > > *\n> > > + * When called with an invalid \\a stream, the function returns the default crop\n> > > + * bounds of the converter.\n> > > + *\n\nCurrently the pipeline does\n\n dewarper_->inputCropBounds(&data->mainPathStream_);\n\nHowever we know that there's no stream (yet) in the V4L2M2MConverter\nclass that is associated with &data->mainPathStream_.\n\nI wonder if V4L2M2MConverter::inputCropBounds() (without any\nparameter) would be better to express that we want the absolute limits\nfor the device (regardless of the configured streams).\n\nYes, it would require the pipeline to keep track of \"Is the dewarper\nconfigured or not\" which would make things a little more complicated\nas in example updateControls() is called before and after the\nconfiguration.\n\nWe could add a function to the Converter class that give a Stream *\nretrieves if it's configured or not... Too clunky ?\n\n\n> > > * \\return A pair containing the minimum and maximum crop bound in that order\n> > > */\n> > >\n> > > diff --git a/src/libcamera/converter/converter_v4l2_m2m.cpp b/src/libcamera/converter/converter_v4l2_m2m.cpp\n> > > index d63ef2f8028f..bd7e5cce600d 100644\n> > > --- a/src/libcamera/converter/converter_v4l2_m2m.cpp\n> > > +++ b/src/libcamera/converter/converter_v4l2_m2m.cpp\n> > > @@ -30,6 +30,52 @@ namespace libcamera {\n> > >\n> > > LOG_DECLARE_CATEGORY(Converter)\n> > >\n> > > +namespace {\n> > > +\n> > > +int getCropBounds(V4L2VideoDevice *device, Rectangle &minCrop,\n> > > +\t\t Rectangle &maxCrop)\n\nPlease not that all this function needs is a video device *\n\nSo it could be made a private member of the V4L2M2MConverter class and\ncalled from the Stream which has a pointer to converter_.\n\nThis way you could easily implement:\nV4L2M2MConverter::inputCropBounds() and\nV4L2M2MConverter::outputCropBounds().\n\n(Do outputCropBounds() make sense at all ?)\n\n\n> > > +{\n> > > +\tRectangle minC;\n> > > +\tRectangle maxC;\n> > > +\n> > > +\t/* Find crop bounds */\n> > > +\tminC.width = 1;\n> > > +\tminC.height = 1;\n> > > +\tmaxC.width = UINT_MAX;\n> > > +\tmaxC.height = UINT_MAX;\n> > > +\n> > > +\tint ret = device->setSelection(V4L2_SEL_TGT_CROP, &minC);\n> > > +\tif (ret) {\n> > > +\t\tLOG(Converter, Error)\n> > > +\t\t\t<< \"Could not query minimum selection crop: \"\n> > > +\t\t\t<< strerror(-ret);\n> > > +\t\treturn ret;\n> > > +\t}\n> > > +\n> > > +\tret = device->getSelection(V4L2_SEL_TGT_CROP_BOUNDS, &maxC);\n> > > +\tif (ret) {\n> > > +\t\tLOG(Converter, Error)\n> > > +\t\t\t<< \"Could not query maximum selection crop: \"\n> > > +\t\t\t<< strerror(-ret);\n> > > +\t\treturn ret;\n> > > +\t}\n> > > +\n> > > +\t/* Reset the crop to its maximum */\n> > > +\tret = device->setSelection(V4L2_SEL_TGT_CROP, &maxC);\n> > > +\tif (ret) {\n> > > +\t\tLOG(Converter, Error)\n> > > +\t\t\t<< \"Could not reset selection crop: \"\n> > > +\t\t\t<< strerror(-ret);\n> > > +\t\treturn ret;\n> > > +\t}\n> > > +\n> > > +\tminCrop = minC;\n> > > +\tmaxCrop = maxC;\n> > > +\treturn 0;\n> > > +}\n> > > +\n> > > +} /* namespace */\n> > > +\n> > > /* -----------------------------------------------------------------------------\n> > > * V4L2M2MConverter::V4L2M2MStream\n> > > */\n> > > @@ -98,41 +144,10 @@ int V4L2M2MConverter::V4L2M2MStream::configure(const StreamConfiguration &inputC\n> > > \toutputBufferCount_ = outputCfg.bufferCount;\n> > >\n> > > \tif (converter_->features() & Feature::InputCrop) {\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)\n> > > -\t\t\t\t<< \"Could not query minimum selection crop: \"\n> > > -\t\t\t\t<< strerror(-ret);\n> > > -\t\t\treturn ret;\n> > > -\t\t}\n> > > -\n> > > -\t\tret = getInputSelection(V4L2_SEL_TGT_CROP_BOUNDS, &maxCrop);\n> > > -\t\tif (ret) {\n> > > -\t\t\tLOG(Converter, Error)\n> > > -\t\t\t\t<< \"Could not query maximum selection crop: \"\n> > > -\t\t\t\t<< strerror(-ret);\n> > > -\t\t\treturn ret;\n> > > -\t\t}\n> > > -\n> > > -\t\t/* Reset the crop to its maximum */\n> > > -\t\tret = setInputSelection(V4L2_SEL_TGT_CROP, &maxCrop);\n> > > -\t\tif (ret) {\n> > > -\t\t\tLOG(Converter, Error)\n> > > -\t\t\t\t<< \"Could not reset selection crop: \"\n> > > -\t\t\t\t<< strerror(-ret);\n> >\n> > Maybe I don't understand M2M enough, but is there of Stream-specific\n> > here ?\n> >\n> > There's an output and an input queue, right ? Do they have per-stream\n> > crop bounds ??\n>\n> I think I don't get the question here. Yes, every context/stream has own\n> crop bounds. What is wrong with that? Or did we settle that in the\n> discussions?\n\nYeah, thanks, it has been clarified. When there's a Stream with a\nformat applied, the CROP_BOUNDS are relative to that format.\n\nThanks\n j\n\n>\n> Cheers,\n> Stefan\n>\n> >\n> > > +\t\tret = getCropBounds(m2m_->output(), inputCropBounds_.first,\n> > > +\t\t\t\t inputCropBounds_.second);\n> > > +\t\tif (ret)\n> > > \t\t\treturn ret;\n> > > -\t\t}\n> > > -\n> > > -\t\tinputCropBounds_ = { minCrop, maxCrop };\n> > > \t}\n> > >\n> > > \treturn 0;\n> > > @@ -258,27 +273,9 @@ V4L2M2MConverter::V4L2M2MConverter(MediaDevice *media)\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> > > -\t\treturn;\n> > > -\n> > > -\t/*\n> > > -\t * Rectangles for cropping targets are defined even if the device\n> > > -\t * does not support cropping. Their sizes and positions will be\n> > > -\t * fixed in such cases.\n> > > -\t *\n> > > -\t * Set and inspect a crop equivalent to half of the maximum crop\n> > > -\t * returned earlier. Use this to determine whether the crop on\n> > > -\t * input is really supported.\n> > > -\t */\n> > > -\tRectangle halfCrop(maxCrop.size() / 2);\n> > > -\tret = m2m_->output()->setSelection(V4L2_SEL_TGT_CROP, &halfCrop);\n> > > -\tif (!ret && halfCrop != maxCrop) {\n> > > +\tret = getCropBounds(m2m_->output(), inputCropBounds_.first,\n> > > +\t\t\t inputCropBounds_.second);\n> > > +\tif (!ret && inputCropBounds_.first != inputCropBounds_.second) {\n> > > \t\tfeatures_ |= Feature::InputCrop;\n> > >\n> > > \t\tLOG(Converter, Info)\n> > > @@ -477,10 +474,10 @@ std::pair\n> > > V4L2M2MConverter::inputCropBounds(const Stream *stream)\n> > > {\n> > > \tauto iter = streams_.find(stream);\n> > > -\tif (iter == streams_.end())\n> > > -\t\treturn {};\n> > > +\tif (iter != streams_.end())\n> > > +\t\treturn iter->second->inputCropBounds();\n> > >\n> > > -\treturn iter->second->inputCropBounds();\n> > > +\treturn inputCropBounds_;\n> > > }\n> > >\n> > > /**\n> > > --\n> > > 2.43.0\n> > >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 488C4BD78E\n\tfor ;\n\tThu, 28 Nov 2024 13:29:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 397D465FA2;\n\tThu, 28 Nov 2024 14:29:58 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BF37765898\n\tfor ;\n\tThu, 28 Nov 2024 14:29:56 +0100 (CET)","from ideasonboard.com (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C4803526;\n\tThu, 28 Nov 2024 14:29:32 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"nS0zx8HF\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1732800572;\n\tbh=vJE4+oxuVtBGaDYNTWvVcmy1i4QW4vdMA36sXe0K/0Q=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=nS0zx8HFfKZyFi91ba105UGWx5DrGlkR5Gy/kPTj6Mf5fQXAJRbLMGVjbVJgCGdGs\n\tHmeHnV5RaswUJdmTg2m8dHcYY7sJYI0rPMcNf6znQZh2sqBjvdqxlGwNdlKpyCWy/0\n\tYtzeUQbij3OPcSyGckK9rN4d8aD4ksCv4giWEfFU=","Date":"Thu, 28 Nov 2024 14:29:54 +0100","From":"Jacopo Mondi ","To":"Stefan Klug ","Cc":"Jacopo Mondi , \n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v2 6/8] libcamera: converter_v4l2_m2m: Improve crop\n\tbounds support","Message-ID":"","References":"<20241125151430.2437285-1-stefan.klug@ideasonboard.com>\n\t<20241125151430.2437285-7-stefan.klug@ideasonboard.com>\n\t\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\" "}}]