[{"id":13399,"web_url":"https://patchwork.libcamera.org/comment/13399/","msgid":"<20201022060428.GL3942@pendragon.ideasonboard.com>","date":"2020-10-22T06:04:28","subject":"Re: [libcamera-devel] [PATCH v4 1/5] libcamera: Add\n\tSensorCropMaximum property","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi David,\n\nThank you for the patch.\n\nOn Mon, Oct 19, 2020 at 01:51:52PM +0100, David Plowman wrote:\n> The SensorCropMaximum camera property reports the location of that\n> part of the image sensor array that is scaled to produce the output\n> images, given in native sensor pixels. It will normally change when a\n> new camera mode is selected, and can be used to implement digital\n> zoom.\n>\n> Signed-off-by: David Plowman \n> ---\n> src/libcamera/property_ids.yaml | 20 ++++++++++++++++++++\n> 1 file changed, 20 insertions(+)\n> \n> diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml\n> index 7261263a..022cf65d 100644\n> --- a/src/libcamera/property_ids.yaml\n> +++ b/src/libcamera/property_ids.yaml\n> @@ -663,4 +663,24 @@ controls:\n> \\todo Rename this property to ActiveAreas once we will have property\n> categories (i.e. Properties::PixelArray::ActiveAreas)\n> \n> + - ScalerCropMaximum:\n> + type: Rectangle\n> + description: |\n> + The size and location, in native sensor pixels, of the part of the\n> + sensor that is rescaled to produce the output images. Note that the\n> + units remain native sensor pixels, even if the sensor is being used in\n> + a binning skipping or scaling mode.\n\nNow that I've reviewed 5/5 I think we agree on the meaning of this\nproperty, but the text here doesn't seem to correctly match what I\nunderstand is our agreement :-) The above paragraph is actually what I\nwas expecting for the ScalerCrop control, not the ScalerCropMaximum\nproperty.\n\nHow about simply defining this property as the maximum value for\nScalerCrop ? It could be written as just\n\n The maximum valid rectangle for the controls::ScalerCrop control.\n\nwith the last two paragraphs kept. The rest would then be moved to the\ndocumentation of ScalerCrop (if they're not there already, I'll review\nthat patch next).\n\nIf you agree with this, and with the commit message updated too,\n\nReviewed-by: Laurent Pinchart \n\n> +\n> + The (x,y) location of this rectangle is relative to the\n> + PixelArrayActiveArea that is being used. The property also takes into\n> + account any further cropping being done by the CSI-2 receiver or\n> + elsewhere.\n> +\n> + This property is valid only after the Camera has been successfully\n> + configured and its value changes 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> +\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 A465BC3D3C\n\tfor ;\n\tThu, 22 Oct 2020 06:05:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 456F361059;\n\tThu, 22 Oct 2020 08:05:16 +0200 (CEST)","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 EF7046034F\n\tfor ;\n\tThu, 22 Oct 2020 08:05:14 +0200 (CEST)","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 762AB53;\n\tThu, 22 Oct 2020 08:05:14 +0200 (CEST)"],"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=\"N9w68Nxr\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1603346714;\n\tbh=fRlyfEkzUYs2p1k2Q65bfurxb18wpYR+UEZ8c3CyKo4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=N9w68Nxr7m7mo8FWCx1WTh5wYRwlurjEaQPCE6SNUZPqcnw7eJunIjA/ZQYVzZfz9\n\t+ezlkik1fwJTw1kbqQnP+hOBmBuhL9K/vuJSp06N2O317lWbNEZWjIDtH15dIHCUJD\n\tXT27h2SLe0jFOAl0GXmHlZwhsXT3kHzdaZPD2148=","Date":"Thu, 22 Oct 2020 09:04:28 +0300","From":"Laurent Pinchart ","To":"David Plowman ","Message-ID":"<20201022060428.GL3942@pendragon.ideasonboard.com>","References":"<20201019125156.26751-1-david.plowman@raspberrypi.com>\n\t<20201019125156.26751-2-david.plowman@raspberrypi.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201019125156.26751-2-david.plowman@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v4 1/5] libcamera: Add\n\tSensorCropMaximum property","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":13401,"web_url":"https://patchwork.libcamera.org/comment/13401/","msgid":"<20201022061752.GB4816@pendragon.ideasonboard.com>","date":"2020-10-22T06:17:52","subject":"Re: [libcamera-devel] [PATCH v4 1/5] libcamera: Add\n\tSensorCropMaximum property","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi David,\n\nAn additional comment.\n\nOn Thu, Oct 22, 2020 at 09:04:28AM +0300, Laurent Pinchart wrote:\n> Hi David,\n> \n> Thank you for the patch.\n> \n> On Mon, Oct 19, 2020 at 01:51:52PM +0100, David Plowman wrote:\n> > The SensorCropMaximum camera property reports the location of that\n> > part of the image sensor array that is scaled to produce the output\n> > images, given in native sensor pixels. It will normally change when a\n> > new camera mode is selected, and can be used to implement digital\n> > zoom.\n> >\n> > Signed-off-by: David Plowman \n> > ---\n> > src/libcamera/property_ids.yaml | 20 ++++++++++++++++++++\n> > 1 file changed, 20 insertions(+)\n> > \n> > diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml\n> > index 7261263a..022cf65d 100644\n> > --- a/src/libcamera/property_ids.yaml\n> > +++ b/src/libcamera/property_ids.yaml\n> > @@ -663,4 +663,24 @@ controls:\n> > \\todo Rename this property to ActiveAreas once we will have property\n> > categories (i.e. Properties::PixelArray::ActiveAreas)\n> > \n> > + - ScalerCropMaximum:\n> > + type: Rectangle\n> > + description: |\n> > + The size and location, in native sensor pixels, of the part of the\n> > + sensor that is rescaled to produce the output images. Note that the\n> > + units remain native sensor pixels, even if the sensor is being used in\n> > + a binning skipping or scaling mode.\n> \n> Now that I've reviewed 5/5 I think we agree on the meaning of this\n> property, but the text here doesn't seem to correctly match what I\n> understand is our agreement :-) The above paragraph is actually what I\n> was expecting for the ScalerCrop control, not the ScalerCropMaximum\n> property.\n> \n> How about simply defining this property as the maximum value for\n> ScalerCrop ? It could be written as just\n> \n> The maximum valid rectangle for the controls::ScalerCrop control.\n\nThis should be expanded a little bit, as, after reviewing the ScalerCrop\ncontrol, the second paragraph of this property doesn't fit there.\n\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.\n\n(I wouldn't mention CSI-2 receiver explicitly as we may be dealing with\na parallel sensor.)\n\n> with the last two paragraphs kept. The rest would then be moved to the\n> documentation of ScalerCrop (if they're not there already, I'll review\n> that patch next).\n> \n> If you agree with this, and with the commit message updated too,\n> \n> Reviewed-by: Laurent Pinchart \n> \n> > +\n> > + The (x,y) location of this rectangle is relative to the\n> > + PixelArrayActiveArea that is being used. The property also takes into\n> > + account any further cropping being done by the CSI-2 receiver or\n> > + elsewhere.\n> > +\n> > + This property is valid only after the Camera has been successfully\n> > + configured and its value changes 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> > +\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 C0D60C3D3C\n\tfor ;\n\tThu, 22 Oct 2020 06:18:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2B54961059;\n\tThu, 22 Oct 2020 08:18:41 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2EF106034F\n\tfor ;\n\tThu, 22 Oct 2020 08:18:39 +0200 (CEST)","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 A218053;\n\tThu, 22 Oct 2020 08:18:38 +0200 (CEST)"],"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=\"L36YNwzA\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1603347518;\n\tbh=H8xrm0+OMZxSydvq3tMsI3Zr+UHsS7rOk0KZio0JwQI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=L36YNwzASr9w664R6BU0FXBsON0QvmmdZUaZyY5CYA7RizO3W9xfFzRVao7bxi3os\n\trSN8eZzqwM8EWYofS9eCcfCHQSIv8pyLZf8CiW+e5vEcyhoNsIN1qQcbAsYr2epuI9\n\tcUtDweDHlUJHqjTEP6c/QSUCo3/J3l1heflbPIJs=","Date":"Thu, 22 Oct 2020 09:17:52 +0300","From":"Laurent Pinchart ","To":"David Plowman ","Message-ID":"<20201022061752.GB4816@pendragon.ideasonboard.com>","References":"<20201019125156.26751-1-david.plowman@raspberrypi.com>\n\t<20201019125156.26751-2-david.plowman@raspberrypi.com>\n\t<20201022060428.GL3942@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201022060428.GL3942@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v4 1/5] libcamera: Add\n\tSensorCropMaximum property","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\" "}}]