[{"id":13998,"web_url":"https://patchwork.libcamera.org/comment/13998/","msgid":"","date":"2020-12-01T10:12:58","subject":"Re: [libcamera-devel] [PATCH v3] 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 version 3 - just confirming that it all looks good to me now!\n\nBest regards\nDavid\n\nOn Tue, 1 Dec 2020 at 10:06, 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> Reviewed-by: David Plowman \n> Signed-off-by: Jacopo Mondi \n> ---\n> v2 -> v3:\n> - Fix grammar issues pointed out by David\n> - Add David's tag\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> 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, it's 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> 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..76474bfc5c31 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 produce the final image streams. The largest possible crop\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> + the size of sensor pixel array portion used to produce the smallest\n> + available stream resolution, accounting for any applied pixel\n> + sub-sampling technique in use and including any border pixels required\n> + by the ISP for image processing. If a pipeline can up-scale, the minimum\n> + valid crop rectangle is equal to the pixel array portion that produces\n> + the smallest frame size accepted by the ISP input. The rectangle\n> + position, defined by its top-left corner, is not relevant and should be\n> + set to (0, 0) by 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 CE9B2BE177\n\tfor ;\n\tTue, 1 Dec 2020 10:13:12 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5B3EE634A5;\n\tTue, 1 Dec 2020 11:13:12 +0100 (CET)","from mail-ot1-x32c.google.com (mail-ot1-x32c.google.com\n\t[IPv6:2607:f8b0:4864:20::32c])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D99E66032B\n\tfor ;\n\tTue, 1 Dec 2020 11:13:10 +0100 (CET)","by mail-ot1-x32c.google.com with SMTP id f12so1110562oto.10\n\tfor ;\n\tTue, 01 Dec 2020 02:13:10 -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=\"MxBIo/lr\"; 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=DEavcXyonlsGKkCqZPoWUTsKrW9J25uNTOzgjDT7uyo=;\n\tb=MxBIo/lr/BDPyo10sTrYe3ecpzb2Yrdv7YIQddfhVTuSMzrfDo9DJFLSlBozPGiFjm\n\tNAwuXf019wRNLy4GWOv1jdfy0G5Xv5QIZYc/d/3i/x3Y/PPN2S/R4C3heDaG//jQLTEh\n\tZIpGq6JPOtn24Wf9qJBxvUbVpGCT3b/ddPb4Oz5qt6LmJl9QTwfr4pk4yCgIZ+uYpwvT\n\tDRasYEznQWxLUvbRQjkDMr8EagKZsytQRWvog0HeIvNylyhUYw7WDEcG8u3q2SKluhTI\n\tfIPPFVSV8AJtsQ7JAWtzAlENTe4DtE/uMUJRmFcMSUR8HIcPuvmlMG+xQotz91rF/Gab\n\ttKpw==","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=DEavcXyonlsGKkCqZPoWUTsKrW9J25uNTOzgjDT7uyo=;\n\tb=m9JxHdchcQXF81yLHoj6Ox648GiCtdpPX7r9Q2M0MZSIfGJ7WcfMkflkvc3AYaTEHy\n\tfol6ZhYG7pNTxnnvMQQbmS8NTluQNO9XSnRxP2MSNGXK76OhlIdCQdLPTrLPnPGdUBFt\n\tetEu38SwI2xchtEyTwHcW8hvjxhKOHCTOXsICssF0L9JvVjphhTsFQpfxhy1PvRuvn6a\n\tJUg4W68XyZe31YD8lJSK2In8cQ5sXHf2jONdSR6vDKaro8pEpCCt8G/i7nmDsxywncmV\n\t/fx0GzAUPMR9hWQqoSa1D5GouAlQPGEdZarssHeUYF/+frgNInV3CPTU6V52yjc68c9R\n\teGjw==","X-Gm-Message-State":"AOAM531X/PW080hqRmVED49w5wUUgrbHV4/k2ZURaLfHNWMQpj7b72Jd\n\t+gJwjgtxjlAL/sYR/jm344KeA8CZIb8X8p1uoMyDgg==","X-Google-Smtp-Source":"ABdhPJz6tiWI9rw9gBYHqa+E7+tcaFSRkdu0NaqQaOLj80AP1tK7U0d+9v9ixgWIhgDJBvZ02zlEenYcF/hbOBtc2/s=","X-Received":"by 2002:a05:6830:18ee:: with SMTP id\n\td14mr1230987otf.317.1606817589589; \n\tTue, 01 Dec 2020 02:13:09 -0800 (PST)","MIME-Version":"1.0","References":"<20201201100654.33552-1-jacopo@jmondi.org>","In-Reply-To":"<20201201100654.33552-1-jacopo@jmondi.org>","From":"David Plowman ","Date":"Tue, 1 Dec 2020 10:12:58 +0000","Message-ID":"","To":"Jacopo Mondi ","Subject":"Re: [libcamera-devel] [PATCH v3] 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":14000,"web_url":"https://patchwork.libcamera.org/comment/14000/","msgid":"<20201201121009.GB360773@oden.dyn.berto.se>","date":"2020-12-01T12:10:09","subject":"Re: [libcamera-devel] [PATCH v3] libcamera: properties: Re-define\n\tScalerCropMaximum","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Jacopo,\n\nThanks for your patch.\n\nOn 2020-12-01 11:06:54 +0100, Jacopo Mondi wrote:\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> Reviewed-by: David Plowman \n> Signed-off-by: Jacopo Mondi \n\nI think this is a step in the right direction.\n\nReviewed-by: Niklas Söderlund \n\n> ---\n> v2 -> v3:\n> - Fix grammar issues pointed out by David\n> - Add David's tag\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> 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> \t\tLOG(RPI, Error) << \"Failed to configure the IPA: \" << ret;\n> \n> \t/*\n> -\t * Update the ScalerCropMaximum to the correct value for this camera mode.\n> -\t * For us, it's the same as the \"analogue crop\".\n> +\t * Update the ScalerCropLimits to the correct values for this camera\n> +\t * mode. For us, it's the same as the \"analogue crop\" and pixel array\n> +\t * portion used to produce the ispMinCropSize_;\n> \t *\n> \t * \\todo Make this property the ScalerCrop maximum value when dynamic\n> \t * controls are available and set it at validate() time\n> \t */\n> -\tdata->properties_.set(properties::ScalerCropMaximum, data->sensorInfo_.analogCrop);\n> +\tRectangle sensorMinCrop =\n> +\t\tRectangle(data->ispMinCropSize_).scaledBy(\n> +\t\t\t\tdata->sensorInfo_.analogCrop.size(),\n> +\t\t\t\tdata->sensorInfo_.outputSize);\n> +\tdata->properties_.set(properties::ScalerCropLimits,\n> +\t\t\t { data->sensorInfo_.analogCrop, sensorMinCrop });\n> \n> \treturn ret;\n> }\n> @@ -946,11 +952,12 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)\n> \tdata->properties_ = data->sensor_->properties();\n> \n> \t/*\n> -\t * Set a default value for the ScalerCropMaximum property to show\n> +\t * Set default values for the ScalerCropLimits property to show\n> \t * that we support its use, however, initialise it to zero because\n> \t * it's not meaningful until a camera mode has been chosen.\n> \t */\n> -\tdata->properties_.set(properties::ScalerCropMaximum, Rectangle{});\n> +\tdata->properties_.set(properties::ScalerCropLimits,\n> +\t\t\t { Rectangle{}, Rectangle{} });\n> \n> \t/*\n> \t * 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..76474bfc5c31 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 produce the final image streams. The largest possible crop\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> + the size of sensor pixel array portion used to produce the smallest\n> + available stream resolution, accounting for any applied pixel\n> + sub-sampling technique in use and including any border pixels required\n> + by the ISP for image processing. If a pipeline can up-scale, the minimum\n> + valid crop rectangle is equal to the pixel array portion that produces\n> + the smallest frame size accepted by the ISP input. The rectangle\n> + position, defined by its top-left corner, is not relevant and should be\n> + set to (0, 0) by 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> \n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","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 12881BE177\n\tfor ;\n\tTue, 1 Dec 2020 12:10:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 982236032D;\n\tTue, 1 Dec 2020 13:10:13 +0100 (CET)","from mail-lj1-x235.google.com (mail-lj1-x235.google.com\n\t[IPv6:2a00:1450:4864:20::235])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5A4636032D\n\tfor ;\n\tTue, 1 Dec 2020 13:10:11 +0100 (CET)","by mail-lj1-x235.google.com with SMTP id t22so2527021ljk.0\n\tfor ;\n\tTue, 01 Dec 2020 04:10:11 -0800 (PST)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\tw19sm181775lfe.175.2020.12.01.04.10.09\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tTue, 01 Dec 2020 04:10:09 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com\n\theader.b=\"cT6HgGhE\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to;\n\tbh=svNt2Ty9DFJs/t8y85Yb3V9y0Y1jTIO/xRc1wi+DkiE=;\n\tb=cT6HgGhE1vJ9yCaeSvWotrTi1nyAV3tmdvGqfyhgeBZEh3ZPZqOgOAiDyDUtr8M+Jb\n\tH91hnYo+k7Eqbdh5vFEA1Z+5Yz+R6mteLRicVhk0b2LnEdeX58D7rEnVIwDMDuGvJFvp\n\t4s9GCUNkhiFDAUu0NEQt4sg9Pjs7B4f+3ayo3q7XxuTDOJX+zKl2PIzNPg9qiCK7MgIH\n\t68miJDrnIX2c4Zl6Hq3CXL9nFemtwioeq86c48xFy1WLw/5oMJ6X3voObcc0gQ6R6snT\n\tP5N0A8wdXZZFmbiboptZqyfLK1Bgg7VyAiWyT6d+kbg8D828bcnLGRnSEZisHmI6TW6e\n\tIXEA==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to;\n\tbh=svNt2Ty9DFJs/t8y85Yb3V9y0Y1jTIO/xRc1wi+DkiE=;\n\tb=oKfPnwkkVODN0vONtL0pECSSxwgfMF8nKDV4aW2slg8TqsrPTJd2RuNUArOf85Vo/9\n\tWODizEcTh+gzIDjl9ERThNuPcFRPtKrDaza7GmmSQUgVmfsS/ZuhVK4SLX5m7lhrSmx4\n\tjYIdHbZaIbpzNxJ0Lw/doENx8EFtL/jYtNMgHM2DKkNAU18qvXLyPjzEyesITADbF/rW\n\trkTcxeaiz2jU7PmQ+uHclM+CFvczItbUW5Fgy+jlQFMWHzxSi5BNry8+uaJzld7Qo1bo\n\tA34dpoGd5EbtFSnNPJ9Sx4Xj5Tfs7ZnvJyAG2frjkvqcCpLle4FG1sUkh1jVDDjkhMuY\n\tdsqA==","X-Gm-Message-State":"AOAM533U+5RYwybF0aA/yo/M4ZGyALXE6GfdfCygaqzgsp6Ln8rDQS8V\n\t8ZpJ7GUpuEMq5lOxjMy0CIWLLA==","X-Google-Smtp-Source":"ABdhPJzsiBzNb/Z7HYxvt47b3VxwshnmHUbh6wDVzgO0JvKtnRxDM+zJeapfFnAvJEqYuIuaFA+vdQ==","X-Received":"by 2002:a2e:7203:: with SMTP id n3mr1150002ljc.86.1606824610579; \n\tTue, 01 Dec 2020 04:10:10 -0800 (PST)","Date":"Tue, 1 Dec 2020 13:10:09 +0100","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Message-ID":"<20201201121009.GB360773@oden.dyn.berto.se>","References":"<20201201100654.33552-1-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201201100654.33552-1-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH v3] 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@lists.libcamera.org","Content-Type":"text/plain; charset=\"iso-8859-1\"","Content-Transfer-Encoding":"quoted-printable","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14001,"web_url":"https://patchwork.libcamera.org/comment/14001/","msgid":"<20201201135039.GG4569@pendragon.ideasonboard.com>","date":"2020-12-01T13:50:39","subject":"Re: [libcamera-devel] [PATCH v3] libcamera: properties: Re-define\n\tScalerCropMaximum","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, Dec 01, 2020 at 11:06:54AM +0100, Jacopo Mondi wrote:\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> Reviewed-by: David Plowman \n> Signed-off-by: Jacopo Mondi \n> ---\n> v2 -> v3:\n> - Fix grammar issues pointed out by David\n> - Add David's tag\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> 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> \t\tLOG(RPI, Error) << \"Failed to configure the IPA: \" << ret;\n> \n> \t/*\n> -\t * Update the ScalerCropMaximum to the correct value for this camera mode.\n> -\t * For us, it's the same as the \"analogue crop\".\n> +\t * Update the ScalerCropLimits to the correct values for this camera\n> +\t * mode. For us, it's the same as the \"analogue crop\" and pixel array\n> +\t * portion used to produce the ispMinCropSize_;\n> \t *\n> \t * \\todo Make this property the ScalerCrop maximum value when dynamic\n> \t * controls are available and set it at validate() time\n> \t */\n> -\tdata->properties_.set(properties::ScalerCropMaximum, data->sensorInfo_.analogCrop);\n> +\tRectangle sensorMinCrop =\n> +\t\tRectangle(data->ispMinCropSize_).scaledBy(\n> +\t\t\t\tdata->sensorInfo_.analogCrop.size(),\n> +\t\t\t\tdata->sensorInfo_.outputSize);\n> +\tdata->properties_.set(properties::ScalerCropLimits,\n> +\t\t\t { data->sensorInfo_.analogCrop, sensorMinCrop });\n> \n> \treturn ret;\n> }\n> @@ -946,11 +952,12 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)\n> \tdata->properties_ = data->sensor_->properties();\n> \n> \t/*\n> -\t * Set a default value for the ScalerCropMaximum property to show\n> +\t * Set default values for the ScalerCropLimits property to show\n> \t * that we support its use, however, initialise it to zero because\n> \t * it's not meaningful until a camera mode has been chosen.\n> \t */\n> -\tdata->properties_.set(properties::ScalerCropMaximum, Rectangle{});\n> +\tdata->properties_.set(properties::ScalerCropLimits,\n> +\t\t\t { Rectangle{}, Rectangle{} });\n> \n> \t/*\n> \t * 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..76474bfc5c31 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\nIt's quite customary to report limits in the minimum, maximum order. Is\nthere a reason to do it the other way around here ?\n\n> +\n> + This two rectangles respectively reflect the minimum and maximum\n\ns/This/These/\n\n> + mandatory cropping applied in the camera sensor and the rest of the\n\nThat's not quite right I think. It's true for the maximum value\n(reporting the minimum mandatory cropping), but for the minimum value,\nit's more about reporting the scaler limits (maximum scaling factor or\nminimum input size) than the \"maximum mandatory cropping\". How about the\nfollowing ?\n\n The minimum value reflects the upscaling limit, resulting from the\n maximum scaling factor or the minimum scaler input size. The maximum\n value reflects the minimum mandatory cropping applied in the camera\n sensor and the rest of the pipeline. Both rectangles are defined\n relatively to the sensor's active pixel array\n (properties::PixelArrayActiveAreas)/\n\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\nHow about\n\n \\par Implementation details\n\nwith a white line just after to create a header ? Otherwise it looks a\nbit weird to have one single paragraph whose first sentence is\n\"Implementation details for ...\".\n\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 produce the final image streams. The largest possible crop\n\n\"frame size\" makes me think about binning/skipping. Maybe \"... pipelines\nmay select different portions of the sensor's pixel array depending on\nthe requested streams\" ? Up to you.\n\n> + rectangle is then limited by the size of the sensor's active pixel area\n\ns/then/thus/ ?\n\nI would drop \"the size of\", as the position of the maximum rectangle\ndepends on the sensor crop configuration, it's not just the size.\n\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> + the size of sensor pixel array portion used to produce the smallest\n> + available stream resolution, accounting for any applied pixel\n> + sub-sampling technique in use and including any border pixels required\n> + by the ISP for image processing.\n\nI'm having trouble parsing this. By \"smallest available stream\nresolution\", are you talking about the smallest resolution a camera can\nproduce, or the smallest stream in the current configuration ? In the\nlatter case, shouldn't it be the largest stream, as upscaling is not\npossible ?\n\n> If a pipeline can up-scale, the minimum\n> + valid crop rectangle is equal to the pixel array portion that produces\n> + the smallest frame size accepted by the ISP input. The rectangle\n> + position, defined by its top-left corner, is not relevant and should be\n> + set to (0, 0) by pipelines.\n\nIs the last sentence valid for both the non-upscaling and upscaling\ncases ? It sounds like the latter, but I suppose it should be the\nformer. I don't think it's an implementation detail, I'd move it to the\nfirst paragraph that could then be written\n\n The maximum value reflects the minimum mandatory cropping applied in the\n camera sensor and the rest of the pipeline. It specifies the boundary\n rectangle within which the ScalerCrop rectangle can be positioned. The\n value is defined relatively to the sensor's active pixel array\n (properties::PixelArrayActiveAreas)/\n\n The minimum value reflects the upscaling limit. It results from the\n maximum scaling factor or the minimum scaler input size, and specifies\n the minimum size for the ScalerCrop rectangle. The position of the\n minmum rectangle isn't relevant and shall be set to (0, 0).\n\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> ...","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 28787BE176\n\tfor ;\n\tTue, 1 Dec 2020 13:50:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B3664634A3;\n\tTue, 1 Dec 2020 14:50:50 +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 7510E6032D\n\tfor ;\n\tTue, 1 Dec 2020 14:50:48 +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 DC66C538;\n\tTue, 1 Dec 2020 14:50:47 +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=\"X+FMV+pP\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1606830648;\n\tbh=Ajogbnyy89+qF6uUpqCq1P/Di0NbQXbDfNL9m+JW2A8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=X+FMV+pPHd5/ijocj3KsCeVDc/7z89z56Gs+af99UT3e4huNOGbdhc5CkU1JKNjoU\n\t5uAGqVMJaLa655vS7i4lMNRptArbbcnVXv0AaCN0uOARnynvKQmAmyfmdoTcCtFU0r\n\txlZQDKNAeNBR+ey575N8cx/IkDwt3+NgW2i492Rg=","Date":"Tue, 1 Dec 2020 15:50:39 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"<20201201135039.GG4569@pendragon.ideasonboard.com>","References":"<20201201100654.33552-1-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201201100654.33552-1-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH v3] 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@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\" "}},{"id":14013,"web_url":"https://patchwork.libcamera.org/comment/14013/","msgid":"<20201201170157.agpfi5j5heu4a3sy@uno.localdomain>","date":"2020-12-01T17:01:57","subject":"Re: [libcamera-devel] [PATCH v3] 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 Laurent,\n\nOn Tue, Dec 01, 2020 at 03:50:39PM +0200, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Tue, Dec 01, 2020 at 11:06:54AM +0100, Jacopo Mondi wrote:\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> > Reviewed-by: David Plowman \n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > v2 -> v3:\n> > - Fix grammar issues pointed out by David\n> > - Add David's tag\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> > 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> > \t\tLOG(RPI, Error) << \"Failed to configure the IPA: \" << ret;\n> >\n> > \t/*\n> > -\t * Update the ScalerCropMaximum to the correct value for this camera mode.\n> > -\t * For us, it's the same as the \"analogue crop\".\n> > +\t * Update the ScalerCropLimits to the correct values for this camera\n> > +\t * mode. For us, it's the same as the \"analogue crop\" and pixel array\n> > +\t * portion used to produce the ispMinCropSize_;\n> > \t *\n> > \t * \\todo Make this property the ScalerCrop maximum value when dynamic\n> > \t * controls are available and set it at validate() time\n> > \t */\n> > -\tdata->properties_.set(properties::ScalerCropMaximum, data->sensorInfo_.analogCrop);\n> > +\tRectangle sensorMinCrop =\n> > +\t\tRectangle(data->ispMinCropSize_).scaledBy(\n> > +\t\t\t\tdata->sensorInfo_.analogCrop.size(),\n> > +\t\t\t\tdata->sensorInfo_.outputSize);\n> > +\tdata->properties_.set(properties::ScalerCropLimits,\n> > +\t\t\t { data->sensorInfo_.analogCrop, sensorMinCrop });\n> >\n> > \treturn ret;\n> > }\n> > @@ -946,11 +952,12 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)\n> > \tdata->properties_ = data->sensor_->properties();\n> >\n> > \t/*\n> > -\t * Set a default value for the ScalerCropMaximum property to show\n> > +\t * Set default values for the ScalerCropLimits property to show\n> > \t * that we support its use, however, initialise it to zero because\n> > \t * it's not meaningful until a camera mode has been chosen.\n> > \t */\n> > -\tdata->properties_.set(properties::ScalerCropMaximum, Rectangle{});\n> > +\tdata->properties_.set(properties::ScalerCropLimits,\n> > +\t\t\t { Rectangle{}, Rectangle{} });\n> >\n> > \t/*\n> > \t * 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..76474bfc5c31 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> It's quite customary to report limits in the minimum, maximum order. Is\n> there a reason to do it the other way around here ?\n>\n\nNo specific reasons, I can swap them\n\n> > +\n> > + This two rectangles respectively reflect the minimum and maximum\n>\n> s/This/These/\n>\n> > + mandatory cropping applied in the camera sensor and the rest of the\n>\n> That's not quite right I think. It's true for the maximum value\n> (reporting the minimum mandatory cropping), but for the minimum value,\n> it's more about reporting the scaler limits (maximum scaling factor or\n> minimum input size) than the \"maximum mandatory cropping\". How about the\n> following ?\n\nYes, \"max mandatory cropping doesn't sound right\".\n\n>\n> The minimum value reflects the upscaling limit, resulting from the\n> maximum scaling factor or the minimum scaler input size. The maximum\n> value reflects the minimum mandatory cropping applied in the camera\n> sensor and the rest of the pipeline. Both rectangles are defined\n> relatively to the sensor's active pixel array\n> (properties::PixelArrayActiveAreas)/\n>\n\nAck\n\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>\n> How about\n>\n> \\par Implementation details\n>\n> with a white line just after to create a header ? Otherwise it looks a\n> bit weird to have one single paragraph whose first sentence is\n> \"Implementation details for ...\".\n>\n\nAck\n\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 produce the final image streams. The largest possible crop\n>\n> \"frame size\" makes me think about binning/skipping. Maybe \"... pipelines\n> may select different portions of the sensor's pixel array depending on\n> the requested streams\" ? Up to you.\n>\n\nAck\n\n> > + rectangle is then limited by the size of the sensor's active pixel area\n>\n> s/then/thus/ ?\n>\n> I would drop \"the size of\", as the position of the maximum rectangle\n> depends on the sensor crop configuration, it's not just the size.\n>\n\nAck\n\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> > + the size of sensor pixel array portion used to produce the smallest\n> > + available stream resolution, accounting for any applied pixel\n> > + sub-sampling technique in use and including any border pixels required\n> > + by the ISP for image processing.\n>\n> I'm having trouble parsing this. By \"smallest available stream\n> resolution\", are you talking about the smallest resolution a camera can\n> produce, or the smallest stream in the current configuration ? In the\n> latter case, shouldn't it be the largest stream, as upscaling is not\n> possible ?\n\nI meant the \"smallest resolution in the current configuration\"\nand I think \"the smallest\" is right, but I get what you mean.\n\nIt's not easy to express this with words but let me try.\n\n What I meant is that the min crop rectangle should be equal to the\n analog crop rectangle of the smaller sensor mode the pipeline can\n select, when considered in isolation.\n\n I thought at this a static property mostly, that will give you the\n possibility to calculate the largest possible zoom factor, to be\n populated at camera initialization time (to make it possible to\n report ANDROID_SCALER_MAX_DIGITAL_ZOOM). When a mode is applied, the\n pipeline will of course select a sensor mode large enough to\n accommodate the largest stream configuration (assuming no\n up-scaling) and update this. To me it was implied, but I agree it's\n not clear.\n\nSo, I guess the thing boils down to the fact I want to use this also\nto report the max digital zoom factor to android, and this is an\nabsolute value, so I envisioned pipelines to initialize this at the\nsmallest selectable sensor mode.\n\nHow should we reword this to make it fit for both usages ?\n\n>\n> > If a pipeline can up-scale, the minimum\n> > + valid crop rectangle is equal to the pixel array portion that produces\n> > + the smallest frame size accepted by the ISP input. The rectangle\n> > + position, defined by its top-left corner, is not relevant and should be\n> > + set to (0, 0) by pipelines.\n>\n> Is the last sentence valid for both the non-upscaling and upscaling\n> cases ? It sounds like the latter, but I suppose it should be the\n> former. I don't think it's an implementation detail, I'd move it to the\n> first paragraph that could then be written\n>\n> The maximum value reflects the minimum mandatory cropping applied in the\n> camera sensor and the rest of the pipeline. It specifies the boundary\n> rectangle within which the ScalerCrop rectangle can be positioned. The\n> value is defined relatively to the sensor's active pixel array\n> (properties::PixelArrayActiveAreas)/\n>\n> The minimum value reflects the upscaling limit. It results from the\n> maximum scaling factor or the minimum scaler input size, and specifies\n> the minimum size for the ScalerCrop rectangle. The position of the\n> minmum rectangle isn't relevant and shall be set to (0, 0).\n>\n\nok\n\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> --\n> Regards,\n>\n> Laurent Pinchart","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 81F55BE177\n\tfor ;\n\tTue, 1 Dec 2020 17:01:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5E1EE63509;\n\tTue, 1 Dec 2020 18:01:53 +0100 (CET)","from relay5-d.mail.gandi.net (relay5-d.mail.gandi.net\n\t[217.70.183.197])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CC3D963460\n\tfor ;\n\tTue, 1 Dec 2020 18:01:51 +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 relay5-d.mail.gandi.net (Postfix) with ESMTPSA id 0B59D1C0004;\n\tTue, 1 Dec 2020 17:01:50 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Tue, 1 Dec 2020 18:01:57 +0100","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20201201170157.agpfi5j5heu4a3sy@uno.localdomain>","References":"<20201201100654.33552-1-jacopo@jmondi.org>\n\t<20201201135039.GG4569@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201201135039.GG4569@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3] 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@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\" "}},{"id":14098,"web_url":"https://patchwork.libcamera.org/comment/14098/","msgid":"<20201207112445.mggjeiegidb4bysb@uno.localdomain>","date":"2020-12-07T11:24:45","subject":"Re: [libcamera-devel] [PATCH v3] 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 Laurent,\n gentle ping\n\nOn Tue, Dec 01, 2020 at 06:01:57PM +0100, Jacopo Mondi wrote:\n> Hi Laurent,\n>\n> On Tue, Dec 01, 2020 at 03:50:39PM +0200, Laurent Pinchart wrote:\n> > Hi Jacopo,\n> >\n> > Thank you for the patch.\n> >\n\n[snip]\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> > > + the size of sensor pixel array portion used to produce the smallest\n> > > + available stream resolution, accounting for any applied pixel\n> > > + sub-sampling technique in use and including any border pixels required\n> > > + by the ISP for image processing.\n> >\n> > I'm having trouble parsing this. By \"smallest available stream\n> > resolution\", are you talking about the smallest resolution a camera can\n> > produce, or the smallest stream in the current configuration ? In the\n> > latter case, shouldn't it be the largest stream, as upscaling is not\n> > possible ?\n>\n> I meant the \"smallest resolution in the current configuration\"\n> and I think \"the smallest\" is right, but I get what you mean.\n>\n> It's not easy to express this with words but let me try.\n>\n> What I meant is that the min crop rectangle should be equal to the\n> analog crop rectangle of the smaller sensor mode the pipeline can\n> select, when considered in isolation.\n>\n> I thought at this a static property mostly, that will give you the\n> possibility to calculate the largest possible zoom factor, to be\n> populated at camera initialization time (to make it possible to\n> report ANDROID_SCALER_MAX_DIGITAL_ZOOM). When a mode is applied, the\n> pipeline will of course select a sensor mode large enough to\n> accommodate the largest stream configuration (assuming no\n> up-scaling) and update this. To me it was implied, but I agree it's\n> not clear.\n>\n> So, I guess the thing boils down to the fact I want to use this also\n> to report the max digital zoom factor to android, and this is an\n> absolute value, so I envisioned pipelines to initialize this at the\n> smallest selectable sensor mode.\n>\n> How should we reword this to make it fit for both usages ?\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 B6D36BDB1F\n\tfor ;\n\tMon, 7 Dec 2020 11:24:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4F5D9634C6;\n\tMon, 7 Dec 2020 12:24:38 +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 05B2860325\n\tfor ;\n\tMon, 7 Dec 2020 12:24:37 +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 relay3-d.mail.gandi.net (Postfix) with ESMTPSA id 55B306000A;\n\tMon, 7 Dec 2020 11:24:36 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Mon, 7 Dec 2020 12:24:45 +0100","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20201207112445.mggjeiegidb4bysb@uno.localdomain>","References":"<20201201100654.33552-1-jacopo@jmondi.org>\n\t<20201201135039.GG4569@pendragon.ideasonboard.com>\n\t<20201201170157.agpfi5j5heu4a3sy@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201201170157.agpfi5j5heu4a3sy@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v3] 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@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\" "}},{"id":14248,"web_url":"https://patchwork.libcamera.org/comment/14248/","msgid":"","date":"2020-12-15T04:38:33","subject":"Re: [libcamera-devel] [PATCH v3] libcamera: properties: Re-define\n\tScalerCropMaximum","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Tue, Dec 01, 2020 at 06:01:57PM +0100, Jacopo Mondi wrote:\n> On Tue, Dec 01, 2020 at 03:50:39PM +0200, Laurent Pinchart wrote:\n> > On Tue, Dec 01, 2020 at 11:06:54AM +0100, Jacopo Mondi wrote:\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> > > Reviewed-by: David Plowman \n> > > Signed-off-by: Jacopo Mondi \n> > > ---\n> > > v2 -> v3:\n> > > - Fix grammar issues pointed out by David\n> > > - Add David's tag\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> > > 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> > > \t\tLOG(RPI, Error) << \"Failed to configure the IPA: \" << ret;\n> > >\n> > > \t/*\n> > > -\t * Update the ScalerCropMaximum to the correct value for this camera mode.\n> > > -\t * For us, it's the same as the \"analogue crop\".\n> > > +\t * Update the ScalerCropLimits to the correct values for this camera\n> > > +\t * mode. For us, it's the same as the \"analogue crop\" and pixel array\n> > > +\t * portion used to produce the ispMinCropSize_;\n> > > \t *\n> > > \t * \\todo Make this property the ScalerCrop maximum value when dynamic\n> > > \t * controls are available and set it at validate() time\n> > > \t */\n> > > -\tdata->properties_.set(properties::ScalerCropMaximum, data->sensorInfo_.analogCrop);\n> > > +\tRectangle sensorMinCrop =\n> > > +\t\tRectangle(data->ispMinCropSize_).scaledBy(\n> > > +\t\t\t\tdata->sensorInfo_.analogCrop.size(),\n> > > +\t\t\t\tdata->sensorInfo_.outputSize);\n> > > +\tdata->properties_.set(properties::ScalerCropLimits,\n> > > +\t\t\t { data->sensorInfo_.analogCrop, sensorMinCrop });\n> > >\n> > > \treturn ret;\n> > > }\n> > > @@ -946,11 +952,12 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator)\n> > > \tdata->properties_ = data->sensor_->properties();\n> > >\n> > > \t/*\n> > > -\t * Set a default value for the ScalerCropMaximum property to show\n> > > +\t * Set default values for the ScalerCropLimits property to show\n> > > \t * that we support its use, however, initialise it to zero because\n> > > \t * it's not meaningful until a camera mode has been chosen.\n> > > \t */\n> > > -\tdata->properties_.set(properties::ScalerCropMaximum, Rectangle{});\n> > > +\tdata->properties_.set(properties::ScalerCropLimits,\n> > > +\t\t\t { Rectangle{}, Rectangle{} });\n> > >\n> > > \t/*\n> > > \t * 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..76474bfc5c31 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> > It's quite customary to report limits in the minimum, maximum order. Is\n> > there a reason to do it the other way around here ?\n> \n> No specific reasons, I can swap them\n> \n> > > +\n> > > + This two rectangles respectively reflect the minimum and maximum\n> >\n> > s/This/These/\n> >\n> > > + mandatory cropping applied in the camera sensor and the rest of the\n> >\n> > That's not quite right I think. It's true for the maximum value\n> > (reporting the minimum mandatory cropping), but for the minimum value,\n> > it's more about reporting the scaler limits (maximum scaling factor or\n> > minimum input size) than the \"maximum mandatory cropping\". How about the\n> > following ?\n> \n> Yes, \"max mandatory cropping doesn't sound right\".\n> \n> > The minimum value reflects the upscaling limit, resulting from the\n> > maximum scaling factor or the minimum scaler input size. The maximum\n> > value reflects the minimum mandatory cropping applied in the camera\n> > sensor and the rest of the pipeline. Both rectangles are defined\n> > relatively to the sensor's active pixel array\n> > (properties::PixelArrayActiveAreas)/\n> >\n> \n> Ack\n> \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> >\n> > How about\n> >\n> > \\par Implementation details\n> >\n> > with a white line just after to create a header ? Otherwise it looks a\n> > bit weird to have one single paragraph whose first sentence is\n> > \"Implementation details for ...\".\n> >\n> \n> Ack\n> \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 produce the final image streams. The largest possible crop\n> >\n> > \"frame size\" makes me think about binning/skipping. Maybe \"... pipelines\n> > may select different portions of the sensor's pixel array depending on\n> > the requested streams\" ? Up to you.\n> >\n> \n> Ack\n> \n> > > + rectangle is then limited by the size of the sensor's active pixel area\n> >\n> > s/then/thus/ ?\n> >\n> > I would drop \"the size of\", as the position of the maximum rectangle\n> > depends on the sensor crop configuration, it's not just the size.\n> >\n> \n> Ack\n> \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> > > + the size of sensor pixel array portion used to produce the smallest\n> > > + available stream resolution, accounting for any applied pixel\n> > > + sub-sampling technique in use and including any border pixels required\n> > > + by the ISP for image processing.\n> >\n> > I'm having trouble parsing this. By \"smallest available stream\n> > resolution\", are you talking about the smallest resolution a camera can\n> > produce, or the smallest stream in the current configuration ? In the\n> > latter case, shouldn't it be the largest stream, as upscaling is not\n> > possible ?\n> \n> I meant the \"smallest resolution in the current configuration\"\n> and I think \"the smallest\" is right, but I get what you mean.\n> \n> It's not easy to express this with words but let me try.\n> \n> What I meant is that the min crop rectangle should be equal to the\n> analog crop rectangle of the smaller sensor mode the pipeline can\n> select, when considered in isolation.\n\nI don't think that's correct. The minimum crop rectangle is indeed\ndefined in pixel array coordinates, but it corresponds to the smallest\ninput size for the scaler, when considering the scaler output size and\nits maximum upscaling factor, not the smallest analog crop rectangle the\nsensor can produce (otherwise, for non mode-based sensors, that would\nusually be a very very small value).\n\nThis can be a bit tricky, and easily misleading. Let's assume a sensor\nthat can bin, and a scaler at the very last stage of the ISP that can at\nmost downscale by 1/4 and upscale by x4. Let's further assume that\nnothing in the pipeline applies additional cropping. This is likely\nincorrect, but simplifies the example and doesn't affect the conclusion.\nFinally, let's consider we capture a 1920x1080 stream.\n\nThe minimum crop rectangle size at the scaler input is 480x270 (1/4 of\n1920x1080). Translated to sensor coordinates, this becomes 960x540 if\nthe sensor is configured with binning, or stays 480x270 otherwise. The\nmaximum digital zoom that the system can apply is thus the sensor\nresolution (note how we haven't defined it yet) divided by 960x540 or by\n480x270, depending on the sensor configuration selected by the pipeline\nhandler. We thus have a different maximum zoom depending on the camera\nconfiguration.\n\nNote that the maximium zoom factor is usually *not* x4 in this case,\neven if that's the maximum upscaling factor of the sensor. Let's assume\na sensor resolution larger than 1920x1080, for instance 4056x3042. What\nwill a pipeline handler typically do ? There are two options.\n\n- It can configure the sensor to output the full resolution. The aspect\n ratios don't match, so the image will be cropped to 4056x2282, and\n then *downscaled* to 1920x1080. When zooming in with the scaler, we\n can go down to a crop rectangle of 480x270. The maximum zoom factor is\n 4056/480 = 2282/270 = 8.45. Note that if the application had decided\n to capture 1280x720, the minimum scaler crop would have been 320x180,\n and the maximum zoom factor 4056/320 = 2282/180 = 12.675.\n\n- It can also configure the sensor to bin the image, for instance\n because the pipeline's bandwidth at full resolution would restrict the\n frame rate. The input to the scaler thus become 2028x1521, which would\n be cropped to 2028x1140 to product the correct aspect ratio. The image\n will again be downscaled to 1920x1080. When zomming in with the\n scaler, we can go down to a crop rectangle of 480x270. The maximum\n zoom factor is hus 2028/480 = 1140/270 = 4.225. If we translate the\n crop rectangle to sensor coordinates (as done by the scaler crop limit\n property), we get a rectangle of 960x540 pixels, but the maximum zoom\n factor is then calculated as 4056/960 = 2282/540 = 4.225, yielding the\n same value.\n\nWe assume there that the binning configuration of the sensor can't be\nchanged while streaming. Otherwise we would have two scalers that can be\ncombined, and the results would be different.\n\nWe also assume a sensor resolution larger than the stream size. This is\nrequired by Android (upscaling isn't allowed when no digital zoom is\napplied), but we don't have to set such a limitation in libcamera\nitself. If the sensor resolution was smaller than the stream size, we\nwould already scale up when no digital zoom is applied, and the maximum\ndigital zoom factor would then be smaller than x4.\n\nFinally, note that deciding whether to bin or not isn't always\ncompletely up to the pipeline handler. When the scaler has minimum\ndownscaling ratio, binning may need to be applied to achieve small\noutput resolutions. With the above example, if the application wants to\ncapture 640x480, binning will be required as it would otherwise exceed\nthe limits of the scaler (it would require downscaling by 6.3375). This\nis needed to fulfil Android's requirements (no digital zoom by default),\nbut is also not a limit that we need to enforce in libcamera (an\napplication may be interested in applying digital zoom between 1.584 and\n25.35, instead of between 1.0 and 12.675).\n\n> I thought at this a static property mostly, that will give you the\n> possibility to calculate the largest possible zoom factor, to be\n> populated at camera initialization time (to make it possible to\n> report ANDROID_SCALER_MAX_DIGITAL_ZOOM). When a mode is applied, the\n> pipeline will of course select a sensor mode large enough to\n> accommodate the largest stream configuration (assuming no\n> up-scaling) and update this. To me it was implied, but I agree it's\n> not clear.\n> \n> So, I guess the thing boils down to the fact I want to use this also\n> to report the max digital zoom factor to android, and this is an\n> absolute value, so I envisioned pipelines to initialize this at the\n> smallest selectable sensor mode.\n\nThere are a few challenges in doing so, given the above, as the maximum\nzoom factor depends on the camera configuration (it also gets more\ncomplicated when we consider multiple streams). This requires a bit more\nthinking. I'd be happy to brainstorm this further with you if that can\nhelp.\n\n> How should we reword this to make it fit for both usages ?\n\nLet's agree on the above first :-)\n\n> > > If a pipeline can up-scale, the minimum\n> > > + valid crop rectangle is equal to the pixel array portion that produces\n> > > + the smallest frame size accepted by the ISP input. The rectangle\n> > > + position, defined by its top-left corner, is not relevant and should be\n> > > + set to (0, 0) by pipelines.\n> >\n> > Is the last sentence valid for both the non-upscaling and upscaling\n> > cases ? It sounds like the latter, but I suppose it should be the\n> > former. I don't think it's an implementation detail, I'd move it to the\n> > first paragraph that could then be written\n> >\n> > The maximum value reflects the minimum mandatory cropping applied in the\n> > camera sensor and the rest of the pipeline. It specifies the boundary\n> > rectangle within which the ScalerCrop rectangle can be positioned. The\n> > value is defined relatively to the sensor's active pixel array\n> > (properties::PixelArrayActiveAreas)/\n> >\n> > The minimum value reflects the upscaling limit. It results from the\n> > maximum scaling factor or the minimum scaler input size, and specifies\n> > the minimum size for the ScalerCrop rectangle. The position of the\n> > minmum rectangle isn't relevant and shall be set to (0, 0).\n> >\n> \n> ok\n> \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> > > ...","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 A2DF1C0F1A\n\tfor ;\n\tTue, 15 Dec 2020 04:38:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2C3BD61590;\n\tTue, 15 Dec 2020 05:38:41 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D199F6052C\n\tfor ;\n\tTue, 15 Dec 2020 05:38:39 +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 48EC796;\n\tTue, 15 Dec 2020 05:38:39 +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=\"Y4UAGard\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1608007119;\n\tbh=9y8fXg6kfmwxGiZX/qsOYd7MBl39ZxYfQiTUKpvNbQQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Y4UAGard3nUjPaD/xJw5WDZk/Dpsr2LO4v4l2x/8Co0zmcr5wHgekUT8fbVlD5rPr\n\t7KeJFGDdWVbIx+rXELuYu7A3ssFJmN5pYoQZHWFP+PZ1RVbHpS44VOZhxyOYUMjsd5\n\tKF0Yc6rfk4e3ZwA2BTUYoh0I9vC0tllXh6XoXmlg=","Date":"Tue, 15 Dec 2020 06:38:33 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20201201100654.33552-1-jacopo@jmondi.org>\n\t<20201201135039.GG4569@pendragon.ideasonboard.com>\n\t<20201201170157.agpfi5j5heu4a3sy@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201201170157.agpfi5j5heu4a3sy@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v3] 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@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\" "}},{"id":14257,"web_url":"https://patchwork.libcamera.org/comment/14257/","msgid":"<20201217123603.oej6h3ixtdsizwof@uno.localdomain>","date":"2020-12-17T12:36:03","subject":"Re: [libcamera-devel] [PATCH v3] 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 Laurent,\n\nOn Tue, Dec 15, 2020 at 06:38:33AM +0200, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n\n[snip]\n\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> > > > + the size of sensor pixel array portion used to produce the smallest\n> > > > + available stream resolution, accounting for any applied pixel\n> > > > + sub-sampling technique in use and including any border pixels required\n> > > > + by the ISP for image processing.\n> > >\n> > > I'm having trouble parsing this. By \"smallest available stream\n> > > resolution\", are you talking about the smallest resolution a camera can\n> > > produce, or the smallest stream in the current configuration ? In the\n> > > latter case, shouldn't it be the largest stream, as upscaling is not\n> > > possible ?\n> >\n> > I meant the \"smallest resolution in the current configuration\"\n> > and I think \"the smallest\" is right, but I get what you mean.\n> >\n> > It's not easy to express this with words but let me try.\n> >\n> > What I meant is that the min crop rectangle should be equal to the\n> > analog crop rectangle of the smaller sensor mode the pipeline can\n> > select, when considered in isolation.\n>\n> I don't think that's correct. The minimum crop rectangle is indeed\n> defined in pixel array coordinates, but it corresponds to the smallest\n> input size for the scaler, when considering the scaler output size and\n> its maximum upscaling factor, not the smallest analog crop rectangle the\n> sensor can produce (otherwise, for non mode-based sensors, that would\n> usually be a very very small value).\n\nMaybe I don't fully get this but the text does not mention \"the\nsmallest analog crop the sensor can produce\" but rather\n\"the size of sensor pixel array portion used to produce the smallest\navailable stream resolution\". All of this prefixed by \"if a pipeline\ncannot up-scale\"\n\nProbably it's also questionable if the \"pipeline cannot up-scale\" case\nis really relevant when talking about digital zoom with one stream.\n\n>\n> This can be a bit tricky, and easily misleading. Let's assume a sensor\n> that can bin, and a scaler at the very last stage of the ISP that can at\n> most downscale by 1/4 and upscale by x4. Let's further assume that\n> nothing in the pipeline applies additional cropping. This is likely\n> incorrect, but simplifies the example and doesn't affect the conclusion.\n> Finally, let's consider we capture a 1920x1080 stream.\n>\n> The minimum crop rectangle size at the scaler input is 480x270 (1/4 of\n> 1920x1080). Translated to sensor coordinates, this becomes 960x540 if\n> the sensor is configured with binning, or stays 480x270 otherwise. The\n> maximum digital zoom that the system can apply is thus the sensor\n> resolution (note how we haven't defined it yet) divided by 960x540 or by\n> 480x270, depending on the sensor configuration selected by the pipeline\n> handler. We thus have a different maximum zoom depending on the camera\n> configuration.\n\nThe fact that it depends on the camera configuration to me was\nimplied, the current property definition is already dependent on the\nconfiguration and reports it with:\n\n \\todo Turn this property into a \"maximum control value\" for the\n ScalerCrop control once \"dynamic\" controls have been implemented.\n\nWhich I've replaced with\n\n \\todo Express this property as the limits for the ScalerCrop control\n once \"dynamic\" controls have been implemented.\n\ndo you think the new definition makes the situation worse ?\nI don't think it does for libcamera, but is problematic for android.\n\n>\n> Note that the maximium zoom factor is usually *not* x4 in this case,\n> even if that's the maximum upscaling factor of the sensor. Let's assume\n\ns/of the sensor/of the scaler/ right ? Or did I missed something ?\n\n> a sensor resolution larger than 1920x1080, for instance 4056x3042. What\n> will a pipeline handler typically do ? There are two options.\n>\n> - It can configure the sensor to output the full resolution. The aspect\n> ratios don't match, so the image will be cropped to 4056x2282, and\n> then *downscaled* to 1920x1080. When zooming in with the scaler, we\n> can go down to a crop rectangle of 480x270. The maximum zoom factor is\n> 4056/480 = 2282/270 = 8.45. Note that if the application had decided\n> to capture 1280x720, the minimum scaler crop would have been 320x180,\n> and the maximum zoom factor 4056/320 = 2282/180 = 12.675.\n>\n\nIndeed.\nDoesn't this example match the \"size of the pixel array portion used\nto produce the smallest available stream resolution\" definition I gave\nfor the minimum crop rectangle ?\n\nI think replacing 'smallest available stream resolution' with 'the smallest\nof the stream resolutions part of a configuration' would make it more\nclear ?\n\nIf I resume your examples here:\n1) output=1920x1080, minScalerInput=480x270\n if 480x720 is 2x2 binned -> analogCrop = 960x540\n zoomFactor = (active pixel array / analog crop) = 4.225\n2) output=1920x1080, minScalerInput=480x270\n if 480x720 is not binned -> analogCrop = 480x720\n zoomFactor = (active pixel array / analog crop) = 8.45\n3) output=1280x720, minScalerInput=320x180\n if 320x180 is not binned -> analogCrop = 320x180\n zoomFactor = (active pixel array / analog crop) = 12.675\n\n> - It can also configure the sensor to bin the image, for instance\n> because the pipeline's bandwidth at full resolution would restrict the\n> frame rate. The input to the scaler thus become 2028x1521, which would\n> be cropped to 2028x1140 to product the correct aspect ratio. The image\n> will again be downscaled to 1920x1080. When zomming in with the\n> scaler, we can go down to a crop rectangle of 480x270. The maximum\n> zoom factor is hus 2028/480 = 1140/270 = 4.225. If we translate the\n> crop rectangle to sensor coordinates (as done by the scaler crop limit\n> property), we get a rectangle of 960x540 pixels, but the maximum zoom\n> factor is then calculated as 4056/960 = 2282/540 = 4.225, yielding the\n> same value.\n\nIsn't this actually correct ? If we can only obtain 480x270 by binning\nand we translate it back to the sensor coordinates to 960x540 and we\ndo the same for the scaler input size (which is not relevant as we reason\nin terms of pixel array size 2028x1521 2x2 binned = 4056x2282)\n\nAgain, this changes with the camera configuration, and depends on a\ncombination of the sensor capabilities (both the device capabilities\nand the driver limitations) and the pipeline.\n\nIt doesn't seem contraditory to me, if not that establishing an\nabsolute zoom factor as requested by android is not possible or\neasy. This can be solved in the HAL by testing the list of supported\nconfigurations and inspecting the property to collect the maximum and\nreport it.\n\n>\n> We assume there that the binning configuration of the sensor can't be\n> changed while streaming. Otherwise we would have two scalers that can be\n> combined, and the results would be different.\n>\n> We also assume a sensor resolution larger than the stream size. This is\n> required by Android (upscaling isn't allowed when no digital zoom is\n> applied), but we don't have to set such a limitation in libcamera\n> itself. If the sensor resolution was smaller than the stream size, we\n> would already scale up when no digital zoom is applied, and the maximum\n> digital zoom factor would then be smaller than x4.\n\nCan we mark the \"sensor resolution smaller than the largest stream\"\nuse case as a corner case for devices that support android ?\nIt seems very unlikely to me that an Android device relies on\nup-scaling to produce its largest stream. Am I overlooking something ?\n\n>\n> Finally, note that deciding whether to bin or not isn't always\n> completely up to the pipeline handler. When the scaler has minimum\n> downscaling ratio, binning may need to be applied to achieve small\n> output resolutions. With the above example, if the application wants to\n> capture 640x480, binning will be required as it would otherwise exceed\n> the limits of the scaler (it would require downscaling by 6.3375). This\n> is needed to fulfil Android's requirements (no digital zoom by default),\n> but is also not a limit that we need to enforce in libcamera (an\n> application may be interested in applying digital zoom between 1.584 and\n> 25.35, instead of between 1.0 and 12.675).\n>\n> > I thought at this a static property mostly, that will give you the\n> > possibility to calculate the largest possible zoom factor, to be\n> > populated at camera initialization time (to make it possible to\n> > report ANDROID_SCALER_MAX_DIGITAL_ZOOM). When a mode is applied, the\n> > pipeline will of course select a sensor mode large enough to\n> > accommodate the largest stream configuration (assuming no\n> > up-scaling) and update this. To me it was implied, but I agree it's\n> > not clear.\n> >\n> > So, I guess the thing boils down to the fact I want to use this also\n> > to report the max digital zoom factor to android, and this is an\n> > absolute value, so I envisioned pipelines to initialize this at the\n> > smallest selectable sensor mode.\n>\n> There are a few challenges in doing so, given the above, as the maximum\n> zoom factor depends on the camera configuration (it also gets more\n> complicated when we consider multiple streams). This requires a bit more\n> thinking. I'd be happy to brainstorm this further with you if that can\n> help.\n>\n\nI think the definition for libcamera only is not worse than what we\nhad. It depends on the current configurtion indeed, but I don't see it\ntoo much controversial.\n\nFor the HAL side, it can't probably be used as it is. It can be worked\naround in the HAL (not easily, as static metadata should be reported\neven before the camera is acquired) or exposed through a draft\nproperty. Or we can decide to make regular property to allow pipeline\nhandlers to report their maximum absolute zoom factor if they have any\nknown and easily computable limits (unfortunately I doubt so).\n\nThis apart, I extending this property to make it useful to calculate\nthe maximum possible zoom factor -per camera configuration- still has\nmerits.\n\n> > How should we reword this to make it fit for both usages ?\n>\n> Let's agree on the above first :-)\n>\n> > > > If a pipeline can up-scale, the minimum\n> > > > + valid crop rectangle is equal to the pixel array portion that produces\n> > > > + the smallest frame size accepted by the ISP input. The rectangle\n> > > > + position, defined by its top-left corner, is not relevant and should be\n> > > > + set to (0, 0) by pipelines.\n> > >\n> > > Is the last sentence valid for both the non-upscaling and upscaling\n> > > cases ? It sounds like the latter, but I suppose it should be the\n> > > former. I don't think it's an implementation detail, I'd move it to the\n> > > first paragraph that could then be written\n> > >\n> > > The maximum value reflects the minimum mandatory cropping applied in the\n> > > camera sensor and the rest of the pipeline. It specifies the boundary\n> > > rectangle within which the ScalerCrop rectangle can be positioned. The\n> > > value is defined relatively to the sensor's active pixel array\n> > > (properties::PixelArrayActiveAreas)/\n> > >\n> > > The minimum value reflects the upscaling limit. It results from the\n> > > maximum scaling factor or the minimum scaler input size, and specifies\n> > > the minimum size for the ScalerCrop rectangle. The position of the\n> > > minmum rectangle isn't relevant and shall be set to (0, 0).\n> > >\n> >\n> > ok\n> >\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> --\n> Regards,\n>\n> Laurent Pinchart","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 880C5BD80A\n\tfor ;\n\tThu, 17 Dec 2020 12:35:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 11BFA61595;\n\tThu, 17 Dec 2020 13:35:56 +0100 (CET)","from relay1-d.mail.gandi.net (relay1-d.mail.gandi.net\n\t[217.70.183.193])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 693F761591\n\tfor ;\n\tThu, 17 Dec 2020 13:35:54 +0100 (CET)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay1-d.mail.gandi.net (Postfix) with ESMTPSA id 2B34624000A;\n\tThu, 17 Dec 2020 12:35:52 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Thu, 17 Dec 2020 13:36:03 +0100","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20201217123603.oej6h3ixtdsizwof@uno.localdomain>","References":"<20201201100654.33552-1-jacopo@jmondi.org>\n\t<20201201135039.GG4569@pendragon.ideasonboard.com>\n\t<20201201170157.agpfi5j5heu4a3sy@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v3] 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@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\" "}}]