[{"id":13981,"web_url":"https://patchwork.libcamera.org/comment/13981/","msgid":"","date":"2020-11-30T14:34:03","subject":"Re: [libcamera-devel] [PATCH v2] libcamera: properties: Re-define\n\tScalerCropMaximum","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Jacopo\n\nThanks for the updated patch!\n\nOn Mon, 30 Nov 2020 at 12:44, Jacopo Mondi wrote:\n>\n> Currently the properties::ScalerCropMaximum property reports the maximum\n> valid Rectangle for the controls::ScalerCrop control, which can be\n> combined with the sensor pixel array size in order to obtain the minimum\n> available zoom factor.\n>\n> It is also useful to be able to calculate the maximum available zoom\n> factor, and in order to do so the minimum valid crop rectangle has to\n> be reported.\n>\n> Transform the ScalerCropMaximum property in ScalerCropLimits, which\n> reports both the maximum and minimum crop limits and port the only user\n> of such a property (Raspberry Pi) to use it.\n>\n> Signed-off-by: Jacopo Mondi \n> ---\n> v1 -> v2:\n> - Re-based on master\n> - Specify that the top-left corner of the minimum rectangle is not relevant\n> - RPi: scale ispMinCropSize_ in the sensor's pixel array matrix coordinates\n> Tested with 640x480 2x2 binned mode -> min = 128x128\n> Tested with 3280x2464 non binned mode -> min = 64x64\n\nPerfect!\n\n>\n> ---\n> src/libcamera/control_ids.yaml | 2 +-\n> .../pipeline/raspberrypi/raspberrypi.cpp | 17 +++++---\n> src/libcamera/property_ids.yaml | 42 ++++++++++++++-----\n> 3 files changed, 45 insertions(+), 16 deletions(-)\n>\n> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml\n> index a883e27e22e9..44c4bb8e86b6 100644\n> --- a/src/libcamera/control_ids.yaml\n> +++ b/src/libcamera/control_ids.yaml\n> @@ -283,7 +283,7 @@ controls:\n> a binning or skipping mode.\n>\n> This control is only present when the pipeline supports scaling. Its\n> - maximum valid value is given by the properties::ScalerCropMaximum\n> + valid value limits are given by the properties::ScalerCropLimits\n> property, and the two can be used to implement digital zoom.\n>\n> # ----------------------------------------------------------------------------\n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index 6fcdf557afcf..22d37e7f1ea1 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -708,13 +708,19 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)\n> LOG(RPI, Error) << \"Failed to configure the IPA: \" << ret;\n>\n> /*\n> - * Update the ScalerCropMaximum to the correct value for this camera mode.\n> - * For us, it's the same as the \"analogue crop\".\n> + * Update the ScalerCropLimits to the correct values for this camera\n> + * mode. For us, they're the same as the \"analogue crop\" and pixel array\n> + * portion used to produce the ispMinCropSize_;\n> *\n> * \\todo Make this property the ScalerCrop maximum value when dynamic\n> * controls are available and set it at validate() time\n> */\n> - data->properties_.set(properties::ScalerCropMaximum, data->sensorInfo_.analogCrop);\n> + Rectangle sensorMinCrop =\n> + Rectangle(data->ispMinCropSize_).scaledBy(\n> + data->sensorInfo_.analogCrop.size(),\n> + data->sensorInfo_.outputSize);\n> + data->properties_.set(properties::ScalerCropLimits,\n> + { data->sensorInfo_.analogCrop, sensorMinCrop });\n>\n\nLooks good to me!\n\n> return ret;\n> }\n> @@ -946,11 +952,12 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)\n> data->properties_ = data->sensor_->properties();\n>\n> /*\n> - * Set a default value for the ScalerCropMaximum property to show\n> + * Set default values for the ScalerCropLimits property to show\n> * that we support its use, however, initialise it to zero because\n> * it's not meaningful until a camera mode has been chosen.\n> */\n> - data->properties_.set(properties::ScalerCropMaximum, Rectangle{});\n> + data->properties_.set(properties::ScalerCropLimits,\n> + { Rectangle{}, Rectangle{} });\n>\n> /*\n> * We cache three things about the sensor in relation to transforms\n> diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml\n> index 64e88f0361d6..97904a666c0d 100644\n> --- a/src/libcamera/property_ids.yaml\n> +++ b/src/libcamera/property_ids.yaml\n> @@ -663,19 +663,41 @@ controls:\n> \\todo Rename this property to ActiveAreas once we will have property\n> categories (i.e. Properties::PixelArray::ActiveAreas)\n>\n> - - ScalerCropMaximum:\n> + - ScalerCropLimits:\n> type: Rectangle\n> + size: [2]\n> description: |\n> - The maximum valid rectangle for the controls::ScalerCrop control. This\n> - reflects the minimum mandatory cropping applied in the camera sensor and\n> - the rest of the pipeline. Just as the ScalerCrop control, it defines a\n> - rectangle taken from the sensor's active pixel array.\n> -\n> - This property is valid only after the camera has been successfully\n> - configured and its value may change whenever a new configuration is\n> + The maximum and minimum valid rectangles for the controls::ScalerCrop\n> + control, specified in this order.\n> +\n> + This two rectangles respectively reflect the minimum and maximum\n> + mandatory cropping applied in the camera sensor and the rest of the\n> + pipeline. Both Rectangles are defined relatively to the sensor's active\n> + pixel array (properties::PixelArrayActiveAreas).\n> +\n> + Implementation details for the maximum valid crop rectangle.\n> + The maximum valid crop rectangle depends on the camera configuration as\n> + pipelines are free to adjust the frame size requested from the sensor\n> + and used to produced the final image streams. The largest possible crop\n\ns/produced/produce/\n\n> + rectangle is then limited by the size of the sensor's active pixel area\n> + portion used to produce the sensor output frame.\n> +\n> + Implementation details for the minimum valid crop rectangle. If a\n> + pipeline cannot up-scale the minimum valid crop rectangle is equal to\n\nStill seem to be missing my comma after up-scale!\n\n> + the size of sensor pixel array portion used to produce the smallest\n> + available stream resolution, accounting any applied pixel sub-sampling\n\ns/accounting/accounting for/\n\n> + technique in use and including any border pixels required by the ISP for\n> + image processing. If a pipeline can up-scale, the minimum valid crop\n> + rectangle is equal to pixel array portion that produces the smallest\n\ns/to/to the/\n\nApart from the little grammar things:\n\nReviewed-by: David Plowman \n\nThanks and best regards\nDavid\n\n> + frame size accepted by the ISP input. The rectangle position, defined\n> + by its top-left corner, is not relevant and should be set to (0, 0) by\n> + pipelines.\n> +\n> + The two rectangles are valid only after the camera has been successfully\n> + configured and their value may change whenever a new configuration is\n> applied.\n>\n> - \\todo Turn this property into a \"maximum control value\" for the\n> - ScalerCrop control once \"dynamic\" controls have been implemented.\n> + \\todo Express this property as the limits for the ScalerCrop control\n> + once \"dynamic\" controls have been implemented.\n>\n> ...\n> --\n> 2.29.1\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 CCBC7BE177\n\tfor ;\n\tMon, 30 Nov 2020 14:34:19 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 63AA66349F;\n\tMon, 30 Nov 2020 15:34:19 +0100 (CET)","from mail-ot1-x32f.google.com (mail-ot1-x32f.google.com\n\t[IPv6:2607:f8b0:4864:20::32f])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BAF8363320\n\tfor ;\n\tMon, 30 Nov 2020 15:34:16 +0100 (CET)","by mail-ot1-x32f.google.com with SMTP id h19so11485836otr.1\n\tfor ;\n\tMon, 30 Nov 2020 06:34:16 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"FCXmOMw5\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=hqZGlgzPo9xW/a6kC30LFcFty9/QTa9b7o6cUweEE64=;\n\tb=FCXmOMw5VHJxNxgZsgxJ2Jacq71pJxkT9WRKbGEFyGwn0ax2/ltRkV0Omnf3RCAakN\n\tW/aaAH+HUp7jdFTH0JY4pyFykKD1uUBgmxyPBt/TEnoCBTmqoNz8cgVTyamo1KPpJ1og\n\tiEnvIIcNNlviyb7rCqZBLjQ7469tVSVd+kcuUAFqA8jBAb6ypvCs+wdYFbK4ITA103En\n\tIzr77l8acv6EP38F8mz0vgGdC0tqqfUkvK8w56gxHEj1DgTG/OwuF55XBqSI9ZPkjJEv\n\tnoqdsUP8cZOOnHhlwLnWZYc0nPizegeY4vLJijMIJ0TpB/q6QMMnKfW7LX6NlMtV/DMA\n\tON7Q==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=hqZGlgzPo9xW/a6kC30LFcFty9/QTa9b7o6cUweEE64=;\n\tb=D7Fjbb5h606HtbI6rU8IoyXlQBpUPaVDHzTR9wpUrOaB0/Zrmr8F//Nl3Tx07vXRH1\n\th2s8lwDBTcUwFHja4rk/FLcUMT73XVISIdEj85zzmyoaF6tylltbaNfGgm81Mb1F2xPy\n\trsxwkgfFWoT4BBq6oAu+0U3f3ODHDeShbIEf6umMXcUXv0dgrnj6zh0ysmkW7W6c10u8\n\tsJNvBKxOyKiBISMsHJwQnK/6L4Da08ZvWDlaE3CjfdupC4d/d9XzbVp8W1XON+VGxfUB\n\t8PosNgZ7hRF2zdiq1oA3W3KvUsBes1PDHpGykk+K5foeJRrsGUL9vzhkMb/2lBDJIYUo\n\tQSbg==","X-Gm-Message-State":"AOAM530MxqjrJ8lbE+o+rq65xPPrtYIs4kmE6vNkwEZMNrdiZBKlVe4Z\n\tyPjAQ4KNu+65lOeEryJTZp00OylGAycxJg+bsLCDiA==","X-Google-Smtp-Source":"ABdhPJyeG1Dyp41XB2irq68BbhS7TGylcR69xiVUIwnHoUrXRIrqFb4c1UXK3D76xz+t424jezqGMDrjYGn8fKJbtW0=","X-Received":"by 2002:a05:6830:18ee:: with SMTP id\n\td14mr16513038otf.317.1606746855132; \n\tMon, 30 Nov 2020 06:34:15 -0800 (PST)","MIME-Version":"1.0","References":"<20201130124432.22039-1-jacopo@jmondi.org>","In-Reply-To":"<20201130124432.22039-1-jacopo@jmondi.org>","From":"David Plowman ","Date":"Mon, 30 Nov 2020 14:34:03 +0000","Message-ID":"","To":"Jacopo Mondi ","Subject":"Re: [libcamera-devel] [PATCH v2] libcamera: properties: Re-define\n\tScalerCropMaximum","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","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":13994,"web_url":"https://patchwork.libcamera.org/comment/13994/","msgid":"<20201201093512.55illrbmqlspeixe@uno.localdomain>","date":"2020-12-01T09:35:12","subject":"Re: [libcamera-devel] [PATCH v2] libcamera: properties: Re-define\n\tScalerCropMaximum","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi David,\n\nOn Mon, Nov 30, 2020 at 02:34:03PM +0000, David Plowman wrote:\n> Hi Jacopo\n>\n> Thanks for the updated patch!\n>\n> On Mon, 30 Nov 2020 at 12:44, Jacopo Mondi wrote:\n> >\n> > Currently the properties::ScalerCropMaximum property reports the maximum\n> > valid Rectangle for the controls::ScalerCrop control, which can be\n> > combined with the sensor pixel array size in order to obtain the minimum\n> > available zoom factor.\n> >\n> > It is also useful to be able to calculate the maximum available zoom\n> > factor, and in order to do so the minimum valid crop rectangle has to\n> > be reported.\n> >\n> > Transform the ScalerCropMaximum property in ScalerCropLimits, which\n> > reports both the maximum and minimum crop limits and port the only user\n> > of such a property (Raspberry Pi) to use it.\n> >\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > v1 -> v2:\n> > - Re-based on master\n> > - Specify that the top-left corner of the minimum rectangle is not relevant\n> > - RPi: scale ispMinCropSize_ in the sensor's pixel array matrix coordinates\n> > Tested with 640x480 2x2 binned mode -> min = 128x128\n> > Tested with 3280x2464 non binned mode -> min = 64x64\n>\n> Perfect!\n>\n> >\n> > ---\n> > src/libcamera/control_ids.yaml | 2 +-\n> > .../pipeline/raspberrypi/raspberrypi.cpp | 17 +++++---\n> > src/libcamera/property_ids.yaml | 42 ++++++++++++++-----\n> > 3 files changed, 45 insertions(+), 16 deletions(-)\n> >\n> > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml\n> > index a883e27e22e9..44c4bb8e86b6 100644\n> > --- a/src/libcamera/control_ids.yaml\n> > +++ b/src/libcamera/control_ids.yaml\n> > @@ -283,7 +283,7 @@ controls:\n> > a binning or skipping mode.\n> >\n> > This control is only present when the pipeline supports scaling. Its\n> > - maximum valid value is given by the properties::ScalerCropMaximum\n> > + valid value limits are given by the properties::ScalerCropLimits\n> > property, and the two can be used to implement digital zoom.\n> >\n> > # ----------------------------------------------------------------------------\n> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > index 6fcdf557afcf..22d37e7f1ea1 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > @@ -708,13 +708,19 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)\n> > LOG(RPI, Error) << \"Failed to configure the IPA: \" << ret;\n> >\n> > /*\n> > - * Update the ScalerCropMaximum to the correct value for this camera mode.\n> > - * For us, it's the same as the \"analogue crop\".\n> > + * Update the ScalerCropLimits to the correct values for this camera\n> > + * mode. For us, they're the same as the \"analogue crop\" and pixel array\n> > + * portion used to produce the ispMinCropSize_;\n> > *\n> > * \\todo Make this property the ScalerCrop maximum value when dynamic\n> > * controls are available and set it at validate() time\n> > */\n> > - data->properties_.set(properties::ScalerCropMaximum, data->sensorInfo_.analogCrop);\n> > + Rectangle sensorMinCrop =\n> > + Rectangle(data->ispMinCropSize_).scaledBy(\n> > + data->sensorInfo_.analogCrop.size(),\n> > + data->sensorInfo_.outputSize);\n> > + data->properties_.set(properties::ScalerCropLimits,\n> > + { data->sensorInfo_.analogCrop, sensorMinCrop });\n> >\n>\n> Looks good to me!\n>\n> > return ret;\n> > }\n> > @@ -946,11 +952,12 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)\n> > data->properties_ = data->sensor_->properties();\n> >\n> > /*\n> > - * Set a default value for the ScalerCropMaximum property to show\n> > + * Set default values for the ScalerCropLimits property to show\n> > * that we support its use, however, initialise it to zero because\n> > * it's not meaningful until a camera mode has been chosen.\n> > */\n> > - data->properties_.set(properties::ScalerCropMaximum, Rectangle{});\n> > + data->properties_.set(properties::ScalerCropLimits,\n> > + { Rectangle{}, Rectangle{} });\n> >\n> > /*\n> > * We cache three things about the sensor in relation to transforms\n> > diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml\n> > index 64e88f0361d6..97904a666c0d 100644\n> > --- a/src/libcamera/property_ids.yaml\n> > +++ b/src/libcamera/property_ids.yaml\n> > @@ -663,19 +663,41 @@ controls:\n> > \\todo Rename this property to ActiveAreas once we will have property\n> > categories (i.e. Properties::PixelArray::ActiveAreas)\n> >\n> > - - ScalerCropMaximum:\n> > + - ScalerCropLimits:\n> > type: Rectangle\n> > + size: [2]\n> > description: |\n> > - The maximum valid rectangle for the controls::ScalerCrop control. This\n> > - reflects the minimum mandatory cropping applied in the camera sensor and\n> > - the rest of the pipeline. Just as the ScalerCrop control, it defines a\n> > - rectangle taken from the sensor's active pixel array.\n> > -\n> > - This property is valid only after the camera has been successfully\n> > - configured and its value may change whenever a new configuration is\n> > + The maximum and minimum valid rectangles for the controls::ScalerCrop\n> > + control, specified in this order.\n> > +\n> > + This two rectangles respectively reflect the minimum and maximum\n> > + mandatory cropping applied in the camera sensor and the rest of the\n> > + pipeline. Both Rectangles are defined relatively to the sensor's active\n> > + pixel array (properties::PixelArrayActiveAreas).\n> > +\n> > + Implementation details for the maximum valid crop rectangle.\n> > + The maximum valid crop rectangle depends on the camera configuration as\n> > + pipelines are free to adjust the frame size requested from the sensor\n> > + and used to produced the final image streams. The largest possible crop\n>\n> s/produced/produce/\n>\n> > + rectangle is then limited by the size of the sensor's active pixel area\n> > + portion used to produce the sensor output frame.\n> > +\n> > + Implementation details for the minimum valid crop rectangle. If a\n> > + pipeline cannot up-scale the minimum valid crop rectangle is equal to\n>\n> Still seem to be missing my comma after up-scale!\n\nOuch!\n\n>\n> > + the size of sensor pixel array portion used to produce the smallest\n> > + available stream resolution, accounting any applied pixel sub-sampling\n>\n> s/accounting/accounting for/\n>\n> > + technique in use and including any border pixels required by the ISP for\n> > + image processing. If a pipeline can up-scale, the minimum valid crop\n> > + rectangle is equal to pixel array portion that produces the smallest\n>\n> s/to/to the/\n>\n> Apart from the little grammar things:\n>\n> Reviewed-by: David Plowman \n\nThanks, will collect your suggestions on v3.\n\nThanks\n j\n\n>\n> Thanks and best regards\n> David\n>\n> > + frame size accepted by the ISP input. The rectangle position, defined\n> > + by its top-left corner, is not relevant and should be set to (0, 0) by\n> > + pipelines.\n> > +\n> > + The two rectangles are valid only after the camera has been successfully\n> > + configured and their value may change whenever a new configuration is\n> > applied.\n> >\n> > - \\todo Turn this property into a \"maximum control value\" for the\n> > - ScalerCrop control once \"dynamic\" controls have been implemented.\n> > + \\todo Express this property as the limits for the ScalerCrop control\n> > + once \"dynamic\" controls have been implemented.\n> >\n> > ...\n> > --\n> > 2.29.1\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 B51B0BE177\n\tfor ;\n\tTue, 1 Dec 2020 09:35:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2DAAB634A3;\n\tTue, 1 Dec 2020 10:35:07 +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 B831C6032B\n\tfor ;\n\tTue, 1 Dec 2020 10:35:05 +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 3360640009;\n\tTue, 1 Dec 2020 09:35:04 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Tue, 1 Dec 2020 10:35:12 +0100","From":"Jacopo Mondi ","To":"David Plowman ","Message-ID":"<20201201093512.55illrbmqlspeixe@uno.localdomain>","References":"<20201130124432.22039-1-jacopo@jmondi.org>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2] libcamera: properties: Re-define\n\tScalerCropMaximum","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","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]