[{"id":14506,"web_url":"https://patchwork.libcamera.org/comment/14506/","msgid":"<X/uRB78GsYh2kXu0@pendragon.ideasonboard.com>","date":"2021-01-10T23:43:03","subject":"Re: [libcamera-devel] [PATCH 07/12] libcamera: ipu3: Register\n\tScalerCrop control","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the patch.\n\nOn Tue, Jan 05, 2021 at 08:05:17PM +0100, Jacopo Mondi wrote:\n> Register the ScalerCrop control in the ImgU pipeline handler computed\n> by using the Viewfinder [1280x720] pipeline output configuration and\n> the sensor resolution as parameters.\n> \n> The ScalerCrop control limits should be updated everytime a new\n> configuration is applied to the sensor.\n> \n> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>\n> ---\n>  src/libcamera/pipeline/ipu3/ipu3.cpp | 53 ++++++++++++++++++++++++++--\n>  1 file changed, 51 insertions(+), 2 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 418301b33a5e..f1329ffb0463 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -41,6 +41,7 @@ static constexpr unsigned int IMGU_OUTPUT_WIDTH_ALIGN = 64;\n>  static constexpr unsigned int IMGU_OUTPUT_HEIGHT_ALIGN = 4;\n>  static constexpr unsigned int IMGU_OUTPUT_WIDTH_MARGIN = 64;\n>  static constexpr unsigned int IMGU_OUTPUT_HEIGHT_MARGIN = 32;\n> +static constexpr Size IPU3ViewfinderSize(1280, 720);\n>  \n>  static const ControlInfoMap::Map IPU3Controls = {\n>  \t{ &controls::draft::PipelineDepth, ControlInfo(2, 3) },\n> @@ -378,7 +379,7 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,\n>  \t\t\t * capped to the maximum sensor resolution and aligned\n>  \t\t\t * to the ImgU output constraints.\n>  \t\t\t */\n> -\t\t\tsize = sensorResolution.boundedTo({ 1280, 720 })\n> +\t\t\tsize = sensorResolution.boundedTo(IPU3ViewfinderSize)\n\nAn unrelated change, but a good one. Could you briefly mention it in the\ncommit message ?\n\n>  \t\t\t\t\t       .alignedDownTo(IMGU_OUTPUT_WIDTH_ALIGN,\n>  \t\t\t\t\t\t\t      IMGU_OUTPUT_HEIGHT_ALIGN);\n>  \t\t\tpixelFormat = formats::NV12;\n> @@ -785,7 +786,7 @@ int PipelineHandlerIPU3::initProperties(IPU3CameraData *data)\n>   */\n>  int PipelineHandlerIPU3::initControls(IPU3CameraData *data)\n>  {\n> -\tconst CameraSensor *sensor = data->cio2_.sensor();\n> +\tCameraSensor *sensor = data->cio2_.sensor();\n>  \tCameraSensorInfo sensorInfo{};\n>  \n>  \tint ret = sensor->sensorInfo(&sensorInfo);\n> @@ -822,6 +823,54 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data)\n>  \tcontrols[&controls::ExposureTime] = ControlInfo(minExposure, maxExposure,\n>  \t\t\t\t\t\t\tdefExposure);\n>  \n> +\t/*\n> +\t * Compute the scaler crop limits.\n> +\t *\n> +\t * \\todo The scaler crop limits depend on the sensor configuration.  It\n> +\t * should be updated when a new configuration is applied.  To initialize\n> +\t * the control use the 'Viewfinder' configuration (1280x720) as the\n> +\t * pipeline output resolution and the full sensor size as input frame\n> +\t * (see the todo note in the validation function).\n\ns/the validation function/IPU3CameraConfiguration::validate()/\n\nI'm not sure what todo note you're referring too though.\n\n> +\t */\n> +\n> +\t/*\n> +\t * The maximum scaler crop rectangle is the analogue crop used to\n> +\t * produce the maximum frame size.\n> +\t */\n> +\tV4L2SubdeviceFormat sensorFormat;\n> +\tsensorFormat.size = sensor->resolution();\n> +\tret = sensor->setFormat(&sensorFormat);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\t/* Re-fetch the sensor info updated to use the largest resolution. */\n> +\tret = sensor->sensorInfo(&sensorInfo);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\tconst Rectangle &analogueCrop = sensorInfo.analogCrop;\n> +\tRectangle maxCrop = analogueCrop;\n> +\n> +\t/*\n> +\t * The minimum crop rectangle is the default viewfinder configuration\n> +\t * (or the sensor resolution, if smaller) re-scaled in the sensor's pixel\n\nJust rescaled, or does the ImgU apply cropping along the pipeline ? If\nit does (or just if you're not sure), a \\todo comment would be good.\n\n> +\t * array coordinates. As the ImgU cannot up-scale, the minimum selection\n> +\t * rectangle has to be as large as the desired pipeline output size.\n> +\t *\n> +\t * The top-left corner position is not relevant as the minimum crop\n> +\t * rectangle can be placed anywhere inside the analogue crop region.\n\nI would have sworn we had documented it as being (0,0) as a convention,\nbut I don't see that in the control documentation. As it's not relevant\nwe can set it to anything, but I believe we should standardize the\nbehaviour. What do you think would be best, (0,0), the same (x,y)\nposition as the maximium value, or centering the minimum rectangle in\nthe maximum rectangle ?\n\nThe code otherwise looks fine.\n\n> +\t */\n> +\tconst Size &sensorOutput = sensorInfo.outputSize;\n> +\tSize minCropSize = sensorOutput.boundedTo(IPU3ViewfinderSize)\n> +\t\t\t\t       .alignedDownTo(IMGU_OUTPUT_WIDTH_ALIGN,\n> +\t\t\t\t\t\t      IMGU_OUTPUT_HEIGHT_ALIGN);\n> +\tRectangle minCrop(minCropSize);\n> +\tminCrop.scaledBy(analogueCrop.size(), sensorOutput);\n> +\tminCrop.x = analogueCrop.x;\n> +\tminCrop.y = analogueCrop.y;\n> +\n> +\tcontrols[&controls::ScalerCrop] = ControlInfo(minCrop, maxCrop, maxCrop);\n> +\n>  \tdata->controlInfo_ = std::move(controls);\n>  \n>  \treturn 0;","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 A7BE7BD80C\n\tfor <parsemail@patchwork.libcamera.org>;\n\tSun, 10 Jan 2021 23:43:19 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3151D68086;\n\tMon, 11 Jan 2021 00:43:19 +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 7572760317\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 11 Jan 2021 00:43:17 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E906AEC;\n\tMon, 11 Jan 2021 00:43:16 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"pjHgrHXP\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1610322197;\n\tbh=+jqP0KVC1JoqneASGcZHnXvrhxuz7vGmoUzJFyVGTYw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=pjHgrHXPnXMiCqdJ2LWdV354x3IkFNp9u3PyrcwRJjVbTatOIVYxsKovy8o+KhDOg\n\t1FiSEzCsamY1vLYJPppfcJj332vtSDQ0fKggJN5UfYvkr8lnzBrIAAsL5QsYC6w7kZ\n\tN0ixqdgaQ8CvCyudBhzsvLWY1LxxQ8XSbdTznDT0=","Date":"Mon, 11 Jan 2021 01:43:03 +0200","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Jacopo Mondi <jacopo@jmondi.org>","Message-ID":"<X/uRB78GsYh2kXu0@pendragon.ideasonboard.com>","References":"<20210105190522.682324-1-jacopo@jmondi.org>\n\t<20210105190522.682324-8-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210105190522.682324-8-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH 07/12] libcamera: ipu3: Register\n\tScalerCrop control","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>","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":14575,"web_url":"https://patchwork.libcamera.org/comment/14575/","msgid":"<20210118104638.i7p2fob22z6lvhtq@uno.localdomain>","date":"2021-01-18T10:46:38","subject":"Re: [libcamera-devel] [PATCH 07/12] libcamera: ipu3: Register\n\tScalerCrop control","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Mon, Jan 11, 2021 at 01:43:03AM +0200, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Tue, Jan 05, 2021 at 08:05:17PM +0100, Jacopo Mondi wrote:\n> > Register the ScalerCrop control in the ImgU pipeline handler computed\n> > by using the Viewfinder [1280x720] pipeline output configuration and\n> > the sensor resolution as parameters.\n> >\n> > The ScalerCrop control limits should be updated everytime a new\n> > configuration is applied to the sensor.\n> >\n> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>\n> > ---\n> >  src/libcamera/pipeline/ipu3/ipu3.cpp | 53 ++++++++++++++++++++++++++--\n> >  1 file changed, 51 insertions(+), 2 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > index 418301b33a5e..f1329ffb0463 100644\n> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > @@ -41,6 +41,7 @@ static constexpr unsigned int IMGU_OUTPUT_WIDTH_ALIGN = 64;\n> >  static constexpr unsigned int IMGU_OUTPUT_HEIGHT_ALIGN = 4;\n> >  static constexpr unsigned int IMGU_OUTPUT_WIDTH_MARGIN = 64;\n> >  static constexpr unsigned int IMGU_OUTPUT_HEIGHT_MARGIN = 32;\n> > +static constexpr Size IPU3ViewfinderSize(1280, 720);\n> >\n> >  static const ControlInfoMap::Map IPU3Controls = {\n> >  \t{ &controls::draft::PipelineDepth, ControlInfo(2, 3) },\n> > @@ -378,7 +379,7 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,\n> >  \t\t\t * capped to the maximum sensor resolution and aligned\n> >  \t\t\t * to the ImgU output constraints.\n> >  \t\t\t */\n> > -\t\t\tsize = sensorResolution.boundedTo({ 1280, 720 })\n> > +\t\t\tsize = sensorResolution.boundedTo(IPU3ViewfinderSize)\n>\n> An unrelated change, but a good one. Could you briefly mention it in the\n> commit message ?\n>\n\nWhy do you think it's unrelated ? As I need the Viewfinder resolution\nto be used in multiple places, I just have defined it and used it here\nas well.\n\n> >  \t\t\t\t\t       .alignedDownTo(IMGU_OUTPUT_WIDTH_ALIGN,\n> >  \t\t\t\t\t\t\t      IMGU_OUTPUT_HEIGHT_ALIGN);\n> >  \t\t\tpixelFormat = formats::NV12;\n> > @@ -785,7 +786,7 @@ int PipelineHandlerIPU3::initProperties(IPU3CameraData *data)\n> >   */\n> >  int PipelineHandlerIPU3::initControls(IPU3CameraData *data)\n> >  {\n> > -\tconst CameraSensor *sensor = data->cio2_.sensor();\n> > +\tCameraSensor *sensor = data->cio2_.sensor();\n> >  \tCameraSensorInfo sensorInfo{};\n> >\n> >  \tint ret = sensor->sensorInfo(&sensorInfo);\n> > @@ -822,6 +823,54 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data)\n> >  \tcontrols[&controls::ExposureTime] = ControlInfo(minExposure, maxExposure,\n> >  \t\t\t\t\t\t\tdefExposure);\n> >\n> > +\t/*\n> > +\t * Compute the scaler crop limits.\n> > +\t *\n> > +\t * \\todo The scaler crop limits depend on the sensor configuration.  It\n> > +\t * should be updated when a new configuration is applied.  To initialize\n> > +\t * the control use the 'Viewfinder' configuration (1280x720) as the\n> > +\t * pipeline output resolution and the full sensor size as input frame\n> > +\t * (see the todo note in the validation function).\n>\n> s/the validation function/IPU3CameraConfiguration::validate()/\n>\n> I'm not sure what todo note you're referring too though.\n\nThe one that says we currently run with the full frame size as ImgU\ninput. Otherwise, if a smaller frame is used, the limits should be\nupdated accordingly.\n\n        validate() :\n\n\t * \\todo The image sensor frame size should be selected to optimize\n\t * operations based on the sizes of the requested streams. However such\n\t * a selection makes the pipeline configuration procedure fail for small\n\t * resolutions (for example: 640x480 with OV5670) and causes the capture\n\t * operations to stall for some stream size combinations (see the\n\t * commit message of the patch that introduced this comment for more\n\t * failure examples).\n\t *\n\n>\n> > +\t */\n> > +\n> > +\t/*\n> > +\t * The maximum scaler crop rectangle is the analogue crop used to\n> > +\t * produce the maximum frame size.\n> > +\t */\n> > +\tV4L2SubdeviceFormat sensorFormat;\n> > +\tsensorFormat.size = sensor->resolution();\n> > +\tret = sensor->setFormat(&sensorFormat);\n> > +\tif (ret)\n> > +\t\treturn ret;\n> > +\n> > +\t/* Re-fetch the sensor info updated to use the largest resolution. */\n> > +\tret = sensor->sensorInfo(&sensorInfo);\n> > +\tif (ret)\n> > +\t\treturn ret;\n> > +\n> > +\tconst Rectangle &analogueCrop = sensorInfo.analogCrop;\n> > +\tRectangle maxCrop = analogueCrop;\n> > +\n> > +\t/*\n> > +\t * The minimum crop rectangle is the default viewfinder configuration\n> > +\t * (or the sensor resolution, if smaller) re-scaled in the sensor's pixel\n>\n> Just rescaled, or does the ImgU apply cropping along the pipeline ? If\n> it does (or just if you're not sure), a \\todo comment would be good.\n>\n\nI read this question as: \"do we need to take into account any\nadditional pixels row/columns required by the ImgU for its processing\" ?\nAm I right ?\n\nIn this case I think it's plausible, as in validate() we apply a margin of\n64 columns and 32 rows to the CIO2 output frame size to calculate the stream\nsize limits.\n\n\t\t\tunsigned int limit;\n\t\t\tlimit = utils::alignDown(cio2Configuration_.size.width - 1,\n\t\t\t\t\t\t IMGU_OUTPUT_WIDTH_MARGIN);\n\t\t\tcfg->size.width = std::clamp(cfg->size.width,\n\t\t\t\t\t\t     IMGU_OUTPUT_MIN_SIZE.width,\n\t\t\t\t\t\t     limit);\n\n\t\t\tlimit = utils::alignDown(cio2Configuration_.size.height - 1,\n\t\t\t\t\t\t IMGU_OUTPUT_HEIGHT_MARGIN);\n\t\t\tcfg->size.height = std::clamp(cfg->size.height,\n\t\t\t\t\t\t      IMGU_OUTPUT_MIN_SIZE.height,\n\t\t\t\t\t\t      limit);\n\n\t\t\tcfg->size.alignDownTo(IMGU_OUTPUT_WIDTH_ALIGN,\n\t\t\t\t\t      IMGU_OUTPUT_HEIGHT_ALIGN);\n\nIt might make sense to apply the same margins here by adding the same\nmargins to the viewfinder sizes\n\n> > +\t * array coordinates. As the ImgU cannot up-scale, the minimum selection\n> > +\t * rectangle has to be as large as the desired pipeline output size.\n> > +\t *\n> > +\t * The top-left corner position is not relevant as the minimum crop\n> > +\t * rectangle can be placed anywhere inside the analogue crop region.\n>\n> I would have sworn we had documented it as being (0,0) as a convention,\n> but I don't see that in the control documentation. As it's not relevant\n> we can set it to anything, but I believe we should standardize the\n> behaviour. What do you think would be best, (0,0), the same (x,y)\n> position as the maximium value, or centering the minimum rectangle in\n> the maximum rectangle ?\n\nI guess it also depends on the platform capabilities. Some platforms\nmight only be capable of center-zoom (I suspect so as Android has a\nproperty to report the zooming capabilities).\n\nCentering it would be safer option to me. does it make sense ?\n\n>\n> The code otherwise looks fine.\n>\n> > +\t */\n> > +\tconst Size &sensorOutput = sensorInfo.outputSize;\n> > +\tSize minCropSize = sensorOutput.boundedTo(IPU3ViewfinderSize)\n> > +\t\t\t\t       .alignedDownTo(IMGU_OUTPUT_WIDTH_ALIGN,\n> > +\t\t\t\t\t\t      IMGU_OUTPUT_HEIGHT_ALIGN);\n\nAlso, aligning the ViewfinderSize downTo() might result in a size\nsmaller than the input size. It does not happen as the viewfinder\nsizes are aligned already\n\n> > +\tRectangle minCrop(minCropSize);\n> > +\tminCrop.scaledBy(analogueCrop.size(), sensorOutput);\n> > +\tminCrop.x = analogueCrop.x;\n> > +\tminCrop.y = analogueCrop.y;\n> > +\n> > +\tcontrols[&controls::ScalerCrop] = ControlInfo(minCrop, maxCrop, maxCrop);\n> > +\n> >  \tdata->controlInfo_ = std::move(controls);\n> >\n> >  \treturn 0;\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","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 94180BD808\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 18 Jan 2021 10:46:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 21CF76811D;\n\tMon, 18 Jan 2021 11:46:20 +0100 (CET)","from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net\n\t[217.70.183.194])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6A1A160314\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 18 Jan 2021 11:46:19 +0100 (CET)","from uno.localdomain (93-34-118-233.ip49.fastwebnet.it\n\t[93.34.118.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay2-d.mail.gandi.net (Postfix) with ESMTPSA id E91F24000D;\n\tMon, 18 Jan 2021 10:46:18 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Mon, 18 Jan 2021 11:46:38 +0100","From":"Jacopo Mondi <jacopo@jmondi.org>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Message-ID":"<20210118104638.i7p2fob22z6lvhtq@uno.localdomain>","References":"<20210105190522.682324-1-jacopo@jmondi.org>\n\t<20210105190522.682324-8-jacopo@jmondi.org>\n\t<X/uRB78GsYh2kXu0@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<X/uRB78GsYh2kXu0@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 07/12] libcamera: ipu3: Register\n\tScalerCrop control","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>","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":14592,"web_url":"https://patchwork.libcamera.org/comment/14592/","msgid":"<YAaHdpOtezQDHcxx@pendragon.ideasonboard.com>","date":"2021-01-19T07:17:10","subject":"Re: [libcamera-devel] [PATCH 07/12] libcamera: ipu3: Register\n\tScalerCrop control","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Mon, Jan 18, 2021 at 11:46:38AM +0100, Jacopo Mondi wrote:\n> On Mon, Jan 11, 2021 at 01:43:03AM +0200, Laurent Pinchart wrote:\n> > On Tue, Jan 05, 2021 at 08:05:17PM +0100, Jacopo Mondi wrote:\n> > > Register the ScalerCrop control in the ImgU pipeline handler computed\n> > > by using the Viewfinder [1280x720] pipeline output configuration and\n> > > the sensor resolution as parameters.\n> > >\n> > > The ScalerCrop control limits should be updated everytime a new\n> > > configuration is applied to the sensor.\n> > >\n> > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>\n> > > ---\n> > >  src/libcamera/pipeline/ipu3/ipu3.cpp | 53 ++++++++++++++++++++++++++--\n> > >  1 file changed, 51 insertions(+), 2 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > index 418301b33a5e..f1329ffb0463 100644\n> > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > @@ -41,6 +41,7 @@ static constexpr unsigned int IMGU_OUTPUT_WIDTH_ALIGN = 64;\n> > >  static constexpr unsigned int IMGU_OUTPUT_HEIGHT_ALIGN = 4;\n> > >  static constexpr unsigned int IMGU_OUTPUT_WIDTH_MARGIN = 64;\n> > >  static constexpr unsigned int IMGU_OUTPUT_HEIGHT_MARGIN = 32;\n> > > +static constexpr Size IPU3ViewfinderSize(1280, 720);\n> > >\n> > >  static const ControlInfoMap::Map IPU3Controls = {\n> > >  \t{ &controls::draft::PipelineDepth, ControlInfo(2, 3) },\n> > > @@ -378,7 +379,7 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,\n> > >  \t\t\t * capped to the maximum sensor resolution and aligned\n> > >  \t\t\t * to the ImgU output constraints.\n> > >  \t\t\t */\n> > > -\t\t\tsize = sensorResolution.boundedTo({ 1280, 720 })\n> > > +\t\t\tsize = sensorResolution.boundedTo(IPU3ViewfinderSize)\n> >\n> > An unrelated change, but a good one. Could you briefly mention it in the\n> > commit message ?\n> \n> Why do you think it's unrelated ? As I need the Viewfinder resolution\n> to be used in multiple places, I just have defined it and used it here\n> as well.\n\nNot completely unrelated indeed.\n\n> > >  \t\t\t\t\t       .alignedDownTo(IMGU_OUTPUT_WIDTH_ALIGN,\n> > >  \t\t\t\t\t\t\t      IMGU_OUTPUT_HEIGHT_ALIGN);\n> > >  \t\t\tpixelFormat = formats::NV12;\n> > > @@ -785,7 +786,7 @@ int PipelineHandlerIPU3::initProperties(IPU3CameraData *data)\n> > >   */\n> > >  int PipelineHandlerIPU3::initControls(IPU3CameraData *data)\n> > >  {\n> > > -\tconst CameraSensor *sensor = data->cio2_.sensor();\n> > > +\tCameraSensor *sensor = data->cio2_.sensor();\n> > >  \tCameraSensorInfo sensorInfo{};\n> > >\n> > >  \tint ret = sensor->sensorInfo(&sensorInfo);\n> > > @@ -822,6 +823,54 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data)\n> > >  \tcontrols[&controls::ExposureTime] = ControlInfo(minExposure, maxExposure,\n> > >  \t\t\t\t\t\t\tdefExposure);\n> > >\n> > > +\t/*\n> > > +\t * Compute the scaler crop limits.\n> > > +\t *\n> > > +\t * \\todo The scaler crop limits depend on the sensor configuration.  It\n> > > +\t * should be updated when a new configuration is applied.  To initialize\n> > > +\t * the control use the 'Viewfinder' configuration (1280x720) as the\n> > > +\t * pipeline output resolution and the full sensor size as input frame\n> > > +\t * (see the todo note in the validation function).\n> >\n> > s/the validation function/IPU3CameraConfiguration::validate()/\n> >\n> > I'm not sure what todo note you're referring too though.\n> \n> The one that says we currently run with the full frame size as ImgU\n> input. Otherwise, if a smaller frame is used, the limits should be\n> updated accordingly.\n> \n>         validate() :\n> \n> \t * \\todo The image sensor frame size should be selected to optimize\n> \t * operations based on the sizes of the requested streams. However such\n> \t * a selection makes the pipeline configuration procedure fail for small\n> \t * resolutions (for example: 640x480 with OV5670) and causes the capture\n> \t * operations to stall for some stream size combinations (see the\n> \t * commit message of the patch that introduced this comment for more\n> \t * failure examples).\n> \t *\n\nThanks.\n\n> > > +\t */\n> > > +\n> > > +\t/*\n> > > +\t * The maximum scaler crop rectangle is the analogue crop used to\n> > > +\t * produce the maximum frame size.\n> > > +\t */\n> > > +\tV4L2SubdeviceFormat sensorFormat;\n> > > +\tsensorFormat.size = sensor->resolution();\n> > > +\tret = sensor->setFormat(&sensorFormat);\n> > > +\tif (ret)\n> > > +\t\treturn ret;\n> > > +\n> > > +\t/* Re-fetch the sensor info updated to use the largest resolution. */\n> > > +\tret = sensor->sensorInfo(&sensorInfo);\n> > > +\tif (ret)\n> > > +\t\treturn ret;\n> > > +\n> > > +\tconst Rectangle &analogueCrop = sensorInfo.analogCrop;\n> > > +\tRectangle maxCrop = analogueCrop;\n> > > +\n> > > +\t/*\n> > > +\t * The minimum crop rectangle is the default viewfinder configuration\n> > > +\t * (or the sensor resolution, if smaller) re-scaled in the sensor's pixel\n> >\n> > Just rescaled, or does the ImgU apply cropping along the pipeline ? If\n> > it does (or just if you're not sure), a \\todo comment would be good.\n> \n> I read this question as: \"do we need to take into account any\n> additional pixels row/columns required by the ImgU for its processing\" ?\n> Am I right ?\n\nYes that's right.\n\n> In this case I think it's plausible, as in validate() we apply a margin of\n> 64 columns and 32 rows to the CIO2 output frame size to calculate the stream\n> size limits.\n> \n> \t\t\tunsigned int limit;\n> \t\t\tlimit = utils::alignDown(cio2Configuration_.size.width - 1,\n> \t\t\t\t\t\t IMGU_OUTPUT_WIDTH_MARGIN);\n> \t\t\tcfg->size.width = std::clamp(cfg->size.width,\n> \t\t\t\t\t\t     IMGU_OUTPUT_MIN_SIZE.width,\n> \t\t\t\t\t\t     limit);\n> \n> \t\t\tlimit = utils::alignDown(cio2Configuration_.size.height - 1,\n> \t\t\t\t\t\t IMGU_OUTPUT_HEIGHT_MARGIN);\n> \t\t\tcfg->size.height = std::clamp(cfg->size.height,\n> \t\t\t\t\t\t      IMGU_OUTPUT_MIN_SIZE.height,\n> \t\t\t\t\t\t      limit);\n> \n> \t\t\tcfg->size.alignDownTo(IMGU_OUTPUT_WIDTH_ALIGN,\n> \t\t\t\t\t      IMGU_OUTPUT_HEIGHT_ALIGN);\n> \n> It might make sense to apply the same margins here by adding the same\n> margins to the viewfinder sizes\n\nSomething needs to be done indeed. As we're not entirely sure what the\nhardware constraints are it's difficult to know for sure. We can start\nwith a first implementation and a \\todo comment to mention it has to be\ndouble-checked.\n\n> > > +\t * array coordinates. As the ImgU cannot up-scale, the minimum selection\n> > > +\t * rectangle has to be as large as the desired pipeline output size.\n> > > +\t *\n> > > +\t * The top-left corner position is not relevant as the minimum crop\n> > > +\t * rectangle can be placed anywhere inside the analogue crop region.\n> >\n> > I would have sworn we had documented it as being (0,0) as a convention,\n> > but I don't see that in the control documentation. As it's not relevant\n> > we can set it to anything, but I believe we should standardize the\n> > behaviour. What do you think would be best, (0,0), the same (x,y)\n> > position as the maximium value, or centering the minimum rectangle in\n> > the maximum rectangle ?\n> \n> I guess it also depends on the platform capabilities. Some platforms\n> might only be capable of center-zoom (I suspect so as Android has a\n> property to report the zooming capabilities).\n> \n> Centering it would be safer option to me. does it make sense ?\n\nApplications should really ignore the (x,y) offset so it shouldn't\nmatter too much. Centering it seems a bit more logical, at the expense\nof a bit more complexity on the pipeline handler side. I don't mind\nmuch.\n\nGiving it a second though, and given your comment about centre-zoom,\nmaybe the (x,y) offset could be used to indicate whether panning is\npossible. We could require (x,y) to be set to the smallest valid value,\nand when only centre-zoom is possible, the rectangle would then be\ncentered. That way applications would know what type of zoom is\npossible.\n\n> > The code otherwise looks fine.\n> >\n> > > +\t */\n> > > +\tconst Size &sensorOutput = sensorInfo.outputSize;\n> > > +\tSize minCropSize = sensorOutput.boundedTo(IPU3ViewfinderSize)\n> > > +\t\t\t\t       .alignedDownTo(IMGU_OUTPUT_WIDTH_ALIGN,\n> > > +\t\t\t\t\t\t      IMGU_OUTPUT_HEIGHT_ALIGN);\n> \n> Also, aligning the ViewfinderSize downTo() might result in a size\n> smaller than the input size. It does not happen as the viewfinder\n> sizes are aligned already\n> \n> > > +\tRectangle minCrop(minCropSize);\n> > > +\tminCrop.scaledBy(analogueCrop.size(), sensorOutput);\n> > > +\tminCrop.x = analogueCrop.x;\n> > > +\tminCrop.y = analogueCrop.y;\n> > > +\n> > > +\tcontrols[&controls::ScalerCrop] = ControlInfo(minCrop, maxCrop, maxCrop);\n> > > +\n> > >  \tdata->controlInfo_ = std::move(controls);\n> > >\n> > >  \treturn 0;","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 0A365BD808\n\tfor <parsemail@patchwork.libcamera.org>;\n\tTue, 19 Jan 2021 07:17:30 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8661468137;\n\tTue, 19 Jan 2021 08:17:29 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EE6CB60310\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 19 Jan 2021 08:17:27 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 731624FB;\n\tTue, 19 Jan 2021 08:17:27 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"ftxkNYl4\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1611040647;\n\tbh=wVr7oGIqESPGOkr675m/BCwYiwdxgWNDaz65ei8lr1U=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ftxkNYl45ScEmfo4DF7xapUv+ERwTVwiQ3lUeDqX7t8iDzPQW8mz5JpHmu8YmxUzn\n\tmYJmRehAQDLINhZBfZeJW6+Gft/OTQRZzMD4K/pdjzm6khn3FzT3Xw8h7pKGzVjqQk\n\tt6K8u++RmbXI/rPe8X05uEbAkx/Dj44M0n9nls5c=","Date":"Tue, 19 Jan 2021 09:17:10 +0200","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Jacopo Mondi <jacopo@jmondi.org>","Message-ID":"<YAaHdpOtezQDHcxx@pendragon.ideasonboard.com>","References":"<20210105190522.682324-1-jacopo@jmondi.org>\n\t<20210105190522.682324-8-jacopo@jmondi.org>\n\t<X/uRB78GsYh2kXu0@pendragon.ideasonboard.com>\n\t<20210118104638.i7p2fob22z6lvhtq@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210118104638.i7p2fob22z6lvhtq@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH 07/12] libcamera: ipu3: Register\n\tScalerCrop control","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>","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":14597,"web_url":"https://patchwork.libcamera.org/comment/14597/","msgid":"<20210119120954.xtlgxuz4bqmzkg7h@uno.localdomain>","date":"2021-01-19T12:09:54","subject":"Re: [libcamera-devel] [PATCH 07/12] libcamera: ipu3: Register\n\tScalerCrop control","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Tue, Jan 19, 2021 at 09:17:10AM +0200, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> On Mon, Jan 18, 2021 at 11:46:38AM +0100, Jacopo Mondi wrote:\n> > On Mon, Jan 11, 2021 at 01:43:03AM +0200, Laurent Pinchart wrote:\n> > > On Tue, Jan 05, 2021 at 08:05:17PM +0100, Jacopo Mondi wrote:\n> > > > Register the ScalerCrop control in the ImgU pipeline handler computed\n> > > > by using the Viewfinder [1280x720] pipeline output configuration and\n> > > > the sensor resolution as parameters.\n> > > >\n> > > > The ScalerCrop control limits should be updated everytime a new\n> > > > configuration is applied to the sensor.\n> > > >\n> > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>\n> > > > ---\n> > > >  src/libcamera/pipeline/ipu3/ipu3.cpp | 53 ++++++++++++++++++++++++++--\n> > > >  1 file changed, 51 insertions(+), 2 deletions(-)\n> > > >\n> > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > index 418301b33a5e..f1329ffb0463 100644\n> > > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > @@ -41,6 +41,7 @@ static constexpr unsigned int IMGU_OUTPUT_WIDTH_ALIGN = 64;\n> > > >  static constexpr unsigned int IMGU_OUTPUT_HEIGHT_ALIGN = 4;\n> > > >  static constexpr unsigned int IMGU_OUTPUT_WIDTH_MARGIN = 64;\n> > > >  static constexpr unsigned int IMGU_OUTPUT_HEIGHT_MARGIN = 32;\n> > > > +static constexpr Size IPU3ViewfinderSize(1280, 720);\n> > > >\n> > > >  static const ControlInfoMap::Map IPU3Controls = {\n> > > >  \t{ &controls::draft::PipelineDepth, ControlInfo(2, 3) },\n> > > > @@ -378,7 +379,7 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,\n> > > >  \t\t\t * capped to the maximum sensor resolution and aligned\n> > > >  \t\t\t * to the ImgU output constraints.\n> > > >  \t\t\t */\n> > > > -\t\t\tsize = sensorResolution.boundedTo({ 1280, 720 })\n> > > > +\t\t\tsize = sensorResolution.boundedTo(IPU3ViewfinderSize)\n> > >\n> > > An unrelated change, but a good one. Could you briefly mention it in the\n> > > commit message ?\n> >\n> > Why do you think it's unrelated ? As I need the Viewfinder resolution\n> > to be used in multiple places, I just have defined it and used it here\n> > as well.\n>\n> Not completely unrelated indeed.\n>\n> > > >  \t\t\t\t\t       .alignedDownTo(IMGU_OUTPUT_WIDTH_ALIGN,\n> > > >  \t\t\t\t\t\t\t      IMGU_OUTPUT_HEIGHT_ALIGN);\n> > > >  \t\t\tpixelFormat = formats::NV12;\n> > > > @@ -785,7 +786,7 @@ int PipelineHandlerIPU3::initProperties(IPU3CameraData *data)\n> > > >   */\n> > > >  int PipelineHandlerIPU3::initControls(IPU3CameraData *data)\n> > > >  {\n> > > > -\tconst CameraSensor *sensor = data->cio2_.sensor();\n> > > > +\tCameraSensor *sensor = data->cio2_.sensor();\n> > > >  \tCameraSensorInfo sensorInfo{};\n> > > >\n> > > >  \tint ret = sensor->sensorInfo(&sensorInfo);\n> > > > @@ -822,6 +823,54 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data)\n> > > >  \tcontrols[&controls::ExposureTime] = ControlInfo(minExposure, maxExposure,\n> > > >  \t\t\t\t\t\t\tdefExposure);\n> > > >\n> > > > +\t/*\n> > > > +\t * Compute the scaler crop limits.\n> > > > +\t *\n> > > > +\t * \\todo The scaler crop limits depend on the sensor configuration.  It\n> > > > +\t * should be updated when a new configuration is applied.  To initialize\n> > > > +\t * the control use the 'Viewfinder' configuration (1280x720) as the\n> > > > +\t * pipeline output resolution and the full sensor size as input frame\n> > > > +\t * (see the todo note in the validation function).\n> > >\n> > > s/the validation function/IPU3CameraConfiguration::validate()/\n> > >\n> > > I'm not sure what todo note you're referring too though.\n> >\n> > The one that says we currently run with the full frame size as ImgU\n> > input. Otherwise, if a smaller frame is used, the limits should be\n> > updated accordingly.\n> >\n> >         validate() :\n> >\n> > \t * \\todo The image sensor frame size should be selected to optimize\n> > \t * operations based on the sizes of the requested streams. However such\n> > \t * a selection makes the pipeline configuration procedure fail for small\n> > \t * resolutions (for example: 640x480 with OV5670) and causes the capture\n> > \t * operations to stall for some stream size combinations (see the\n> > \t * commit message of the patch that introduced this comment for more\n> > \t * failure examples).\n> > \t *\n>\n> Thanks.\n>\n> > > > +\t */\n> > > > +\n> > > > +\t/*\n> > > > +\t * The maximum scaler crop rectangle is the analogue crop used to\n> > > > +\t * produce the maximum frame size.\n> > > > +\t */\n> > > > +\tV4L2SubdeviceFormat sensorFormat;\n> > > > +\tsensorFormat.size = sensor->resolution();\n> > > > +\tret = sensor->setFormat(&sensorFormat);\n> > > > +\tif (ret)\n> > > > +\t\treturn ret;\n> > > > +\n> > > > +\t/* Re-fetch the sensor info updated to use the largest resolution. */\n> > > > +\tret = sensor->sensorInfo(&sensorInfo);\n> > > > +\tif (ret)\n> > > > +\t\treturn ret;\n> > > > +\n> > > > +\tconst Rectangle &analogueCrop = sensorInfo.analogCrop;\n> > > > +\tRectangle maxCrop = analogueCrop;\n> > > > +\n> > > > +\t/*\n> > > > +\t * The minimum crop rectangle is the default viewfinder configuration\n> > > > +\t * (or the sensor resolution, if smaller) re-scaled in the sensor's pixel\n> > >\n> > > Just rescaled, or does the ImgU apply cropping along the pipeline ? If\n> > > it does (or just if you're not sure), a \\todo comment would be good.\n> >\n> > I read this question as: \"do we need to take into account any\n> > additional pixels row/columns required by the ImgU for its processing\" ?\n> > Am I right ?\n>\n> Yes that's right.\n>\n> > In this case I think it's plausible, as in validate() we apply a margin of\n> > 64 columns and 32 rows to the CIO2 output frame size to calculate the stream\n> > size limits.\n> >\n> > \t\t\tunsigned int limit;\n> > \t\t\tlimit = utils::alignDown(cio2Configuration_.size.width - 1,\n> > \t\t\t\t\t\t IMGU_OUTPUT_WIDTH_MARGIN);\n> > \t\t\tcfg->size.width = std::clamp(cfg->size.width,\n> > \t\t\t\t\t\t     IMGU_OUTPUT_MIN_SIZE.width,\n> > \t\t\t\t\t\t     limit);\n> >\n> > \t\t\tlimit = utils::alignDown(cio2Configuration_.size.height - 1,\n> > \t\t\t\t\t\t IMGU_OUTPUT_HEIGHT_MARGIN);\n> > \t\t\tcfg->size.height = std::clamp(cfg->size.height,\n> > \t\t\t\t\t\t      IMGU_OUTPUT_MIN_SIZE.height,\n> > \t\t\t\t\t\t      limit);\n> >\n> > \t\t\tcfg->size.alignDownTo(IMGU_OUTPUT_WIDTH_ALIGN,\n> > \t\t\t\t\t      IMGU_OUTPUT_HEIGHT_ALIGN);\n> >\n> > It might make sense to apply the same margins here by adding the same\n> > margins to the viewfinder sizes\n>\n> Something needs to be done indeed. As we're not entirely sure what the\n> hardware constraints are it's difficult to know for sure. We can start\n> with a first implementation and a \\todo comment to mention it has to be\n> double-checked.\n>\n\nI reworked the implementation and made sure the calculated minimum\nrectangle gives me a size that is suitable for the pipeline\nconfiguration tool, so I hope I'm on the right track.\n\n> > > > +\t * array coordinates. As the ImgU cannot up-scale, the minimum selection\n> > > > +\t * rectangle has to be as large as the desired pipeline output size.\n> > > > +\t *\n> > > > +\t * The top-left corner position is not relevant as the minimum crop\n> > > > +\t * rectangle can be placed anywhere inside the analogue crop region.\n> > >\n> > > I would have sworn we had documented it as being (0,0) as a convention,\n> > > but I don't see that in the control documentation. As it's not relevant\n> > > we can set it to anything, but I believe we should standardize the\n> > > behaviour. What do you think would be best, (0,0), the same (x,y)\n> > > position as the maximium value, or centering the minimum rectangle in\n> > > the maximum rectangle ?\n> >\n> > I guess it also depends on the platform capabilities. Some platforms\n> > might only be capable of center-zoom (I suspect so as Android has a\n> > property to report the zooming capabilities).\n> >\n> > Centering it would be safer option to me. does it make sense ?\n>\n> Applications should really ignore the (x,y) offset so it shouldn't\n> matter too much. Centering it seems a bit more logical, at the expense\n> of a bit more complexity on the pipeline handler side. I don't mind\n> much.\n>\n> Giving it a second though, and given your comment about centre-zoom,\n> maybe the (x,y) offset could be used to indicate whether panning is\n> possible. We could require (x,y) to be set to the smallest valid value,\n> and when only centre-zoom is possible, the rectangle would then be\n> centered. That way applications would know what type of zoom is\n> possible.\n>\n\nI agree that (0,0) might be used to indicate it is possible to freely pan in\nthe largest crop rectangle.\n\nWe're free to decide what to do, as currently the ScalerCrop control\nlimits are not used by regular applications but just by the HAL to\ncalculate the digital zoom limits. Otherwise the ScalerCropMaximum property is\nused.\n\nOnce we move to updating the ControlInfo limits per each configuration\nand drop ScalerCropMaximum we'll have to describe the policy we have\nchosen in the control documentation.\n\n> > > The code otherwise looks fine.\n> > >\n> > > > +\t */\n> > > > +\tconst Size &sensorOutput = sensorInfo.outputSize;\n> > > > +\tSize minCropSize = sensorOutput.boundedTo(IPU3ViewfinderSize)\n> > > > +\t\t\t\t       .alignedDownTo(IMGU_OUTPUT_WIDTH_ALIGN,\n> > > > +\t\t\t\t\t\t      IMGU_OUTPUT_HEIGHT_ALIGN);\n> >\n> > Also, aligning the ViewfinderSize downTo() might result in a size\n> > smaller than the input size. It does not happen as the viewfinder\n> > sizes are aligned already\n> >\n> > > > +\tRectangle minCrop(minCropSize);\n> > > > +\tminCrop.scaledBy(analogueCrop.size(), sensorOutput);\n> > > > +\tminCrop.x = analogueCrop.x;\n> > > > +\tminCrop.y = analogueCrop.y;\n> > > > +\n> > > > +\tcontrols[&controls::ScalerCrop] = ControlInfo(minCrop, maxCrop, maxCrop);\n> > > > +\n> > > >  \tdata->controlInfo_ = std::move(controls);\n> > > >\n> > > >  \treturn 0;\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","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 6644EBD808\n\tfor <parsemail@patchwork.libcamera.org>;\n\tTue, 19 Jan 2021 12:09:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 09C4668140;\n\tTue, 19 Jan 2021 13:09:39 +0100 (CET)","from relay3-d.mail.gandi.net (relay3-d.mail.gandi.net\n\t[217.70.183.195])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2206A60314\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 19 Jan 2021 13:09:37 +0100 (CET)","from uno.localdomain (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay3-d.mail.gandi.net (Postfix) with ESMTPSA id 9BE4460008;\n\tTue, 19 Jan 2021 12:09:36 +0000 (UTC)"],"X-Originating-IP":"93.61.96.190","Date":"Tue, 19 Jan 2021 13:09:54 +0100","From":"Jacopo Mondi <jacopo@jmondi.org>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Message-ID":"<20210119120954.xtlgxuz4bqmzkg7h@uno.localdomain>","References":"<20210105190522.682324-1-jacopo@jmondi.org>\n\t<20210105190522.682324-8-jacopo@jmondi.org>\n\t<X/uRB78GsYh2kXu0@pendragon.ideasonboard.com>\n\t<20210118104638.i7p2fob22z6lvhtq@uno.localdomain>\n\t<YAaHdpOtezQDHcxx@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<YAaHdpOtezQDHcxx@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 07/12] libcamera: ipu3: Register\n\tScalerCrop control","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>","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]