[{"id":11336,"web_url":"https://patchwork.libcamera.org/comment/11336/","msgid":"","date":"2020-07-10T13:12:19","subject":"Re: [libcamera-devel] [PATCH v2 1/2] libcamera: Implement digital\n\tzoom","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi David,\n\nOn 09/07/2020 10:15, David Plowman wrote:\n> These changes add a Digital Zoom control, tacking a rectangle as its\n\ns/tacking/taking/\n\n> argument, indicating the region of the sensor output that will be\n> zoomed up to the final output size.\n> \n> Additionally, we need have a method returning the \"sensorCrop\" which\n\ns/need have/need to have/\n\n> gives the dimensions of the sensor output within which we can pan and\n> zoom.\n\n\nThis commit message describes implementing digital zoom, but *this*\ncommit is only dealing with the representaion of a crop region in the\n(CameraSensor) which can be used to implement digital zoom.\n\nPerhaps this commit should be titled:\n \"libcamera: camera_sensor: Support cropping regions\"\n\n(with that being the assumption that my comments below about moving this\nto the CameraSensor class are correct)\n\n\n\n> ---\n> include/libcamera/camera.h | 2 ++\n> include/libcamera/internal/pipeline_handler.h | 4 +++\n> src/libcamera/camera.cpp | 26 +++++++++++++++++++\n> src/libcamera/control_ids.yaml | 10 +++++++\n> 4 files changed, 42 insertions(+)\n> \n> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> index 4d1a4a9..dd07f7a 100644\n> --- a/include/libcamera/camera.h\n> +++ b/include/libcamera/camera.h\n> @@ -92,6 +92,8 @@ public:\n> \tstd::unique_ptr generateConfiguration(const StreamRoles &roles = {});\n> \tint configure(CameraConfiguration *config);\n> \n> +\tSize const &getSensorCrop() const;\n> +\n> \tRequest *createRequest(uint64_t cookie = 0);\n> \tint queueRequest(Request *request);\n> \n> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> index 22e629a..37e069a 100644\n> --- a/include/libcamera/internal/pipeline_handler.h\n> +++ b/include/libcamera/internal/pipeline_handler.h\n> @@ -89,6 +89,8 @@ public:\n> \n> \tconst char *name() const { return name_; }\n> \n> +\tSize const &getSensorCrop() const { return sensorCrop_; }\n> +\n> protected:\n> \tvoid registerCamera(std::shared_ptr camera,\n> \t\t\t std::unique_ptr data);\n> @@ -100,6 +102,8 @@ protected:\n> \n> \tCameraManager *manager_;\n> \n> +\tSize sensorCrop_;\n> +\n\nI think this needs to go in the CameraSensor class...\nIt's 'per sensor' not 'per pipeline handler' right?\n\n\n> private:\n> \tvoid mediaDeviceDisconnected(MediaDevice *media);\n> \tvirtual void disconnect();\n> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> index 69a1b44..6614c93 100644\n> --- a/src/libcamera/camera.cpp\n> +++ b/src/libcamera/camera.cpp\n> @@ -793,6 +793,32 @@ int Camera::configure(CameraConfiguration *config)\n> \treturn 0;\n> }\n> \n> +/**\n> + * \\brief Return the size of the sensor image being used to form the output\n> + *\n> + * This method returns the size, in pixels, of the raw image read from the\n> + * sensor and which is used to form the output image(s). Note that these\n> + * values take account of any cropping performed on the sensor output so\n> + * as to produce the correct aspect ratio. It would normally be necessary\n> + * to retrieve these values in order to calculate correct parameters for\n> + * digital zoom.\n> + *\n> + * Example: a sensor mode may produce a 1920x1440 output image. But if an\n> + * application has requested a 16:9 image, the values returned here would\n> + * be 1920x1080 - the largest portion of the sensor output that provides\n> + * the correct aspect ratio.\n\nShould this be a Rectangle? Can the crop region be positioned\narbitrarily or is it always centered?\n\n\nWhat is the relationship between this and the analogCrop in\nCameraSensorInfo? I 'think' that one is more about what the actual valid\ndata region is from the camera, so I presume the analogCrop is sort of\nthe 'maximum' image size?\n\n\n\n\n> + *\n> + * \\context This function is \\threadsafe. It will only return valid\n> + * (non-zero) values when the camera has been configured.\n> + *\n> + * \\return The dimensions of the sensor image in use.\n> + */\n> +\n> +Size const &Camera::getSensorCrop() const\n> +{\n> +\treturn p_->pipe_->getSensorCrop();\n\nAha, more indication to me that this should go in to the Camera class,\nand it would save the indirection required here...\n\n\n> +}\n> +\n> /**\n> * \\brief Create a request object for the camera\n> * \\param[in] cookie Opaque cookie for application use\n> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml\n> index 988b501..5a099d5 100644\n> --- a/src/libcamera/control_ids.yaml\n> +++ b/src/libcamera/control_ids.yaml\n> @@ -262,4 +262,14 @@ controls:\n> In this respect, it is not necessarily aimed at providing a way to\n> implement a focus algorithm by the application, rather an indication of\n> how in-focus a frame is.\n> +\n> + - DigitalZoom:\n> + type: Rectangle\n> + description: |\n> + Sets the portion of the full sensor image, in pixels, that will be\n> + used for digital zoom. That is, this part of the sensor output will\n> + be scaled up to make the full size output image (and everything else\n> + discarded). To obtain the \"full sensor image\" that is available, the\n> + method Camera::getOutputCrop() should be called once the camera is\n> + configured. An application may pan and zoom within this rectangle.\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 EDDA0BD792\n\tfor ;\n\tFri, 10 Jul 2020 13:12:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7E022613D4;\n\tFri, 10 Jul 2020 15:12:24 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 747C061186\n\tfor ;\n\tFri, 10 Jul 2020 15:12:23 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89242-aztw30-2-0-cust488.18-1.cable.virginm.net [86.31.129.233])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id DC70D2C0;\n\tFri, 10 Jul 2020 15:12:22 +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=\"T9sZ6yng\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1594386743;\n\tbh=yAoBT2GT5PFa2GH9IdL4boj8CDSIxXAO/SdLuiAMnu4=;\n\th=Reply-To:Subject:To:References:From:Date:In-Reply-To:From;\n\tb=T9sZ6yngLHjqnnNP2yfks2pnN7UYnDirpNxCmBayENIe+TgjCY4SXzXgQ+wYJiqcX\n\t2Tr3ezAA0+gOglWiX8qP/NXLeQC9nDXk8tf8XJ0J3Zw9pmmXNCohx/JGC68u/mouBo\n\tOGRfOoaGHsFMjY4IH3dUQJbaiak8q7tg/xxpwmAk=","To":"David Plowman ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20200709091555.1617-1-david.plowman@raspberrypi.com>\n\t<20200709091555.1617-2-david.plowman@raspberrypi.com>","From":"Kieran Bingham ","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"","Date":"Fri, 10 Jul 2020 14:12:19 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.8.0","MIME-Version":"1.0","In-Reply-To":"<20200709091555.1617-2-david.plowman@raspberrypi.com>","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH v2 1/2] libcamera: Implement digital\n\tzoom","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","Reply-To":"kieran.bingham@ideasonboard.com","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":11432,"web_url":"https://patchwork.libcamera.org/comment/11432/","msgid":"","date":"2020-07-20T07:19:29","subject":"Re: [libcamera-devel] [PATCH v2 1/2] libcamera: Implement digital\n\tzoom","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Kieran\n\nThanks for your feedback. Apologies for the delay in replying - I've\nbeen away for the past week!\n\nI agree there's still some stuff to figure out here! Let me try and\nanswer some of the questions (and probably pose some more!):\n\nOn Fri, 10 Jul 2020 at 14:12, Kieran Bingham\n wrote:\n>\n> Hi David,\n>\n> On 09/07/2020 10:15, David Plowman wrote:\n> > These changes add a Digital Zoom control, tacking a rectangle as its\n>\n> s/tacking/taking/\n>\n> > argument, indicating the region of the sensor output that will be\n> > zoomed up to the final output size.\n> >\n> > Additionally, we need have a method returning the \"sensorCrop\" which\n>\n> s/need have/need to have/\n>\n> > gives the dimensions of the sensor output within which we can pan and\n> > zoom.\n>\n>\n> This commit message describes implementing digital zoom, but *this*\n> commit is only dealing with the representaion of a crop region in the\n> (CameraSensor) which can be used to implement digital zoom.\n>\n> Perhaps this commit should be titled:\n> \"libcamera: camera_sensor: Support cropping regions\"\n>\n> (with that being the assumption that my comments below about moving this\n> to the CameraSensor class are correct)\n>\n>\n>\n> > ---\n> > include/libcamera/camera.h | 2 ++\n> > include/libcamera/internal/pipeline_handler.h | 4 +++\n> > src/libcamera/camera.cpp | 26 +++++++++++++++++++\n> > src/libcamera/control_ids.yaml | 10 +++++++\n> > 4 files changed, 42 insertions(+)\n> >\n> > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> > index 4d1a4a9..dd07f7a 100644\n> > --- a/include/libcamera/camera.h\n> > +++ b/include/libcamera/camera.h\n> > @@ -92,6 +92,8 @@ public:\n> > std::unique_ptr generateConfiguration(const StreamRoles &roles = {});\n> > int configure(CameraConfiguration *config);\n> >\n> > + Size const &getSensorCrop() const;\n> > +\n> > Request *createRequest(uint64_t cookie = 0);\n> > int queueRequest(Request *request);\n> >\n> > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> > index 22e629a..37e069a 100644\n> > --- a/include/libcamera/internal/pipeline_handler.h\n> > +++ b/include/libcamera/internal/pipeline_handler.h\n> > @@ -89,6 +89,8 @@ public:\n> >\n> > const char *name() const { return name_; }\n> >\n> > + Size const &getSensorCrop() const { return sensorCrop_; }\n> > +\n> > protected:\n> > void registerCamera(std::shared_ptr camera,\n> > std::unique_ptr data);\n> > @@ -100,6 +102,8 @@ protected:\n> >\n> > CameraManager *manager_;\n> >\n> > + Size sensorCrop_;\n> > +\n>\n> I think this needs to go in the CameraSensor class...\n> It's 'per sensor' not 'per pipeline handler' right?\n\nMaybe this is the billion dollar question. See below...!\n\n>\n>\n> > private:\n> > void mediaDeviceDisconnected(MediaDevice *media);\n> > virtual void disconnect();\n> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > index 69a1b44..6614c93 100644\n> > --- a/src/libcamera/camera.cpp\n> > +++ b/src/libcamera/camera.cpp\n> > @@ -793,6 +793,32 @@ int Camera::configure(CameraConfiguration *config)\n> > return 0;\n> > }\n> >\n> > +/**\n> > + * \\brief Return the size of the sensor image being used to form the output\n> > + *\n> > + * This method returns the size, in pixels, of the raw image read from the\n> > + * sensor and which is used to form the output image(s). Note that these\n> > + * values take account of any cropping performed on the sensor output so\n> > + * as to produce the correct aspect ratio. It would normally be necessary\n> > + * to retrieve these values in order to calculate correct parameters for\n> > + * digital zoom.\n> > + *\n> > + * Example: a sensor mode may produce a 1920x1440 output image. But if an\n> > + * application has requested a 16:9 image, the values returned here would\n> > + * be 1920x1080 - the largest portion of the sensor output that provides\n> > + * the correct aspect ratio.\n>\n> Should this be a Rectangle? Can the crop region be positioned\n> arbitrarily or is it always centered?\n\nAs I've implemented this, it is just a pair of dimensions. The\npipeline handler may position it anywhere it likes within the array of\npixels output by the sensor (and takes care of any offset within\nthat), but the application doesn't need to know. The idea being that\nthe application gets to choose a *rectangle* from within the\ndimensions it is given and doesn't have to bother itself with how it's\noffset in the pixel array beyond that.\n\n>\n>\n> What is the relationship between this and the analogCrop in\n> CameraSensorInfo? I 'think' that one is more about what the actual valid\n> data region is from the camera, so I presume the analogCrop is sort of\n> the 'maximum' image size?\n\nYes, I think the analogCrop is the maximum usable pixel area from the\nsensor. The crop here is what the pipeline handler decides it wants to\nuse. It's adjusted for aspect ratio (of the requested output), the\nactual size that it wants to use (and hence the scaling). I certainly\nagree that it might want putting somewhere else... yet it's only the\npipeline handler that can calculate it. It depends on the sensor, but\nit also depends on the mode selected, the output size requested, and\n\"arbitrary\" decisions made in the pipeline handler (for example, they\nmay wish to avoid marginal scaling and prefer 1:1 input to output\npixels).\n\n>\n>\n>\n>\n> > + *\n> > + * \\context This function is \\threadsafe. It will only return valid\n> > + * (non-zero) values when the camera has been configured.\n> > + *\n> > + * \\return The dimensions of the sensor image in use.\n> > + */\n> > +\n> > +Size const &Camera::getSensorCrop() const\n> > +{\n> > + return p_->pipe_->getSensorCrop();\n>\n> Aha, more indication to me that this should go in to the Camera class,\n> and it would save the indirection required here...\n\nIndeed. But given that the pipeline handler calculates it, can we have\nit poke its value into the Camera class? Or is that weird if the\nnumber depends on rather more than just the camera? (And will change\nevery time we start the sensor in a different mode.)\n\nAnother solution to this that I've talked about previously is simply\nto go use ratios, and avoid pixel coordinates altogether. Then you\njust don't need this function. The counter argument is that an\napplication can't control digital pan/zoom down to the precise pixel -\nwhich sounds appealing. Yet I think real applications will have in\nmind a _rate_ at which they wish to work (e.g. pan across half the\nfield of view in 5 seconds) at which point ratios are quite natural.\n\nBest regards\nDavid\n\n>\n>\n> > +}\n> > +\n> > /**\n> > * \\brief Create a request object for the camera\n> > * \\param[in] cookie Opaque cookie for application use\n> > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml\n> > index 988b501..5a099d5 100644\n> > --- a/src/libcamera/control_ids.yaml\n> > +++ b/src/libcamera/control_ids.yaml\n> > @@ -262,4 +262,14 @@ controls:\n> > In this respect, it is not necessarily aimed at providing a way to\n> > implement a focus algorithm by the application, rather an indication of\n> > how in-focus a frame is.\n> > +\n> > + - DigitalZoom:\n> > + type: Rectangle\n> > + description: |\n> > + Sets the portion of the full sensor image, in pixels, that will be\n> > + used for digital zoom. That is, this part of the sensor output will\n> > + be scaled up to make the full size output image (and everything else\n> > + discarded). To obtain the \"full sensor image\" that is available, the\n> > + method Camera::getOutputCrop() should be called once the camera is\n> > + configured. An application may pan and zoom within this rectangle.\n> > ...\n> >\n>\n> --\n> Regards\n> --\n> Kieran","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 8E823C0109\n\tfor ;\n\tMon, 20 Jul 2020 07:19:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id F26CF605A8;\n\tMon, 20 Jul 2020 09:19:42 +0200 (CEST)","from mail-oi1-x243.google.com (mail-oi1-x243.google.com\n\t[IPv6:2607:f8b0:4864:20::243])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8BCDE6039E\n\tfor ;\n\tMon, 20 Jul 2020 09:19:41 +0200 (CEST)","by mail-oi1-x243.google.com with SMTP id h17so13705804oie.3\n\tfor ;\n\tMon, 20 Jul 2020 00:19:41 -0700 (PDT)"],"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=\"d7GVYhP+\"; 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=n9Fi1Xop5+WHgDke98dbWUcMW+NJf4//jazVq4MFh7M=;\n\tb=d7GVYhP+D9TZCFt/uhqoeXK5kR+D8gOcP90ouxY3tIVEzb6WJMZ5FEW8freKW/tL5H\n\tKvY4KgDtDe123RzIXYZIMOeRTuMi0+0vAthL9xzlIkRxw1p3U1ed2d3G4PfnpVDyyuU9\n\tlYb++JOSS1ui7UDQwzB7Trq8avVQWGZ/AphWHOhhOfVAK7k4cYh5Zlva8p+gy5hKcBgz\n\tBQ8oz4pTWR4nW5iGy/678baUoF1o3JOu/sNLZvayj+HZNIRwpBxbQbFrnr3ZV0h2kuuF\n\twrVN9+V33y0FtmRdm9xAPmaop0+Ye3CWpBvyZ9Oa3Xj7PduN1EBVSxIvM7EXGsF+/jPq\n\tnizA==","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=n9Fi1Xop5+WHgDke98dbWUcMW+NJf4//jazVq4MFh7M=;\n\tb=PUlpYAHahB28NXyN1V2uiDYLUXBJBDLW+IbVDBRrX0/ofXyL5wAgGgQAA6RMibwOsy\n\tZuU5CyY0H9RfzNKw1Ch8L9pdhRJNxBtX9POvSdMOdNmOcS14EtRnOrysOUMT8QM4df+l\n\tvA9DjGeCaVRCT7kpyuLSS9BfV1SXPeW2zfWoLvEpAIV5cnJDZmq0yC6oetBwPWHJawmx\n\t3XX87e3KcQdHQiuqC/wWnAiONVTXC4XPPakuAl7PvqaRwQtU5oKdbVE99/vcO/tccKM/\n\tQpDfcAlvJVHw6arbHi9inZZxGKoGd8waSPgeW3yY/Nb34wh/oEy1qVhUlQ/v/+rjVICP\n\tpwXQ==","X-Gm-Message-State":"AOAM533mHofw+7B96vlfXcweKN22IYEGJ1XHd4uAhcqGypBKr5KAuE0e\n\tuZdTr38dALzoVWyE7rsTkdLNcZDO15SGLHmiXtFI5Q==","X-Google-Smtp-Source":"ABdhPJwGyhD1XOx6W45H8TJ8ZjJg+jY5gPHlSrG5I0BMvESEN+IBiFvRaVv+HOTLuCJf933VvQMNAqc6LMfmNQZs+Uc=","X-Received":"by 2002:aca:84e:: with SMTP id 75mr17474193oii.107.1595229579980;\n\tMon, 20 Jul 2020 00:19:39 -0700 (PDT)","MIME-Version":"1.0","References":"<20200709091555.1617-1-david.plowman@raspberrypi.com>\n\t<20200709091555.1617-2-david.plowman@raspberrypi.com>\n\t","In-Reply-To":"","From":"David Plowman ","Date":"Mon, 20 Jul 2020 08:19:29 +0100","Message-ID":"","To":"Kieran Bingham ","Subject":"Re: [libcamera-devel] [PATCH v2 1/2] libcamera: Implement digital\n\tzoom","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":11454,"web_url":"https://patchwork.libcamera.org/comment/11454/","msgid":"","date":"2020-07-21T11:27:40","subject":"Re: [libcamera-devel] [PATCH v2 1/2] libcamera: Implement digital\n\tzoom","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi again Kieran, everyone\n\nSo I've been contemplating some of this a bit more since yesterday,\nand I wonder if I'm confusing everything by giving that contentious\nfunction the name \"getSensorCrop\". It makes it sound like it's a\nproperty of the sensor when it really isn't - it can change whenever\nthe sensor mode chosen changes, or the size of the requested output\nchanges. It's an interaction between the sensor mode and the behaviour\nof the pipeline handler, so I wonder if we should be coining a\ndifferent name for it?\n\nHow about....\n\ngetInputCrop\ngetPipelineCrop\ngetPipelineSensorCrop\ngetPipelineSensorArea\n\nOf those I think I prefer the last one, it's the area of the sensor\nthat the pipeline handler has chosen that we can use. But does anyone\nhave any better suggestions?\n\nI guess I'm also still unclear as to where to store it - I'm assuming\nthat the SensorInfo isn't the right place for something that can\nchange like this. (?)\n\nThanks!\nDavid\n\nOn Mon, 20 Jul 2020 at 08:19, David Plowman\n wrote:\n>\n> Hi Kieran\n>\n> Thanks for your feedback. Apologies for the delay in replying - I've\n> been away for the past week!\n>\n> I agree there's still some stuff to figure out here! Let me try and\n> answer some of the questions (and probably pose some more!):\n>\n> On Fri, 10 Jul 2020 at 14:12, Kieran Bingham\n> wrote:\n> >\n> > Hi David,\n> >\n> > On 09/07/2020 10:15, David Plowman wrote:\n> > > These changes add a Digital Zoom control, tacking a rectangle as its\n> >\n> > s/tacking/taking/\n> >\n> > > argument, indicating the region of the sensor output that will be\n> > > zoomed up to the final output size.\n> > >\n> > > Additionally, we need have a method returning the \"sensorCrop\" which\n> >\n> > s/need have/need to have/\n> >\n> > > gives the dimensions of the sensor output within which we can pan and\n> > > zoom.\n> >\n> >\n> > This commit message describes implementing digital zoom, but *this*\n> > commit is only dealing with the representaion of a crop region in the\n> > (CameraSensor) which can be used to implement digital zoom.\n> >\n> > Perhaps this commit should be titled:\n> > \"libcamera: camera_sensor: Support cropping regions\"\n> >\n> > (with that being the assumption that my comments below about moving this\n> > to the CameraSensor class are correct)\n> >\n> >\n> >\n> > > ---\n> > > include/libcamera/camera.h | 2 ++\n> > > include/libcamera/internal/pipeline_handler.h | 4 +++\n> > > src/libcamera/camera.cpp | 26 +++++++++++++++++++\n> > > src/libcamera/control_ids.yaml | 10 +++++++\n> > > 4 files changed, 42 insertions(+)\n> > >\n> > > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> > > index 4d1a4a9..dd07f7a 100644\n> > > --- a/include/libcamera/camera.h\n> > > +++ b/include/libcamera/camera.h\n> > > @@ -92,6 +92,8 @@ public:\n> > > std::unique_ptr generateConfiguration(const StreamRoles &roles = {});\n> > > int configure(CameraConfiguration *config);\n> > >\n> > > + Size const &getSensorCrop() const;\n> > > +\n> > > Request *createRequest(uint64_t cookie = 0);\n> > > int queueRequest(Request *request);\n> > >\n> > > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> > > index 22e629a..37e069a 100644\n> > > --- a/include/libcamera/internal/pipeline_handler.h\n> > > +++ b/include/libcamera/internal/pipeline_handler.h\n> > > @@ -89,6 +89,8 @@ public:\n> > >\n> > > const char *name() const { return name_; }\n> > >\n> > > + Size const &getSensorCrop() const { return sensorCrop_; }\n> > > +\n> > > protected:\n> > > void registerCamera(std::shared_ptr camera,\n> > > std::unique_ptr data);\n> > > @@ -100,6 +102,8 @@ protected:\n> > >\n> > > CameraManager *manager_;\n> > >\n> > > + Size sensorCrop_;\n> > > +\n> >\n> > I think this needs to go in the CameraSensor class...\n> > It's 'per sensor' not 'per pipeline handler' right?\n>\n> Maybe this is the billion dollar question. See below...!\n>\n> >\n> >\n> > > private:\n> > > void mediaDeviceDisconnected(MediaDevice *media);\n> > > virtual void disconnect();\n> > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > > index 69a1b44..6614c93 100644\n> > > --- a/src/libcamera/camera.cpp\n> > > +++ b/src/libcamera/camera.cpp\n> > > @@ -793,6 +793,32 @@ int Camera::configure(CameraConfiguration *config)\n> > > return 0;\n> > > }\n> > >\n> > > +/**\n> > > + * \\brief Return the size of the sensor image being used to form the output\n> > > + *\n> > > + * This method returns the size, in pixels, of the raw image read from the\n> > > + * sensor and which is used to form the output image(s). Note that these\n> > > + * values take account of any cropping performed on the sensor output so\n> > > + * as to produce the correct aspect ratio. It would normally be necessary\n> > > + * to retrieve these values in order to calculate correct parameters for\n> > > + * digital zoom.\n> > > + *\n> > > + * Example: a sensor mode may produce a 1920x1440 output image. But if an\n> > > + * application has requested a 16:9 image, the values returned here would\n> > > + * be 1920x1080 - the largest portion of the sensor output that provides\n> > > + * the correct aspect ratio.\n> >\n> > Should this be a Rectangle? Can the crop region be positioned\n> > arbitrarily or is it always centered?\n>\n> As I've implemented this, it is just a pair of dimensions. The\n> pipeline handler may position it anywhere it likes within the array of\n> pixels output by the sensor (and takes care of any offset within\n> that), but the application doesn't need to know. The idea being that\n> the application gets to choose a *rectangle* from within the\n> dimensions it is given and doesn't have to bother itself with how it's\n> offset in the pixel array beyond that.\n>\n> >\n> >\n> > What is the relationship between this and the analogCrop in\n> > CameraSensorInfo? I 'think' that one is more about what the actual valid\n> > data region is from the camera, so I presume the analogCrop is sort of\n> > the 'maximum' image size?\n>\n> Yes, I think the analogCrop is the maximum usable pixel area from the\n> sensor. The crop here is what the pipeline handler decides it wants to\n> use. It's adjusted for aspect ratio (of the requested output), the\n> actual size that it wants to use (and hence the scaling). I certainly\n> agree that it might want putting somewhere else... yet it's only the\n> pipeline handler that can calculate it. It depends on the sensor, but\n> it also depends on the mode selected, the output size requested, and\n> \"arbitrary\" decisions made in the pipeline handler (for example, they\n> may wish to avoid marginal scaling and prefer 1:1 input to output\n> pixels).\n>\n> >\n> >\n> >\n> >\n> > > + *\n> > > + * \\context This function is \\threadsafe. It will only return valid\n> > > + * (non-zero) values when the camera has been configured.\n> > > + *\n> > > + * \\return The dimensions of the sensor image in use.\n> > > + */\n> > > +\n> > > +Size const &Camera::getSensorCrop() const\n> > > +{\n> > > + return p_->pipe_->getSensorCrop();\n> >\n> > Aha, more indication to me that this should go in to the Camera class,\n> > and it would save the indirection required here...\n>\n> Indeed. But given that the pipeline handler calculates it, can we have\n> it poke its value into the Camera class? Or is that weird if the\n> number depends on rather more than just the camera? (And will change\n> every time we start the sensor in a different mode.)\n>\n> Another solution to this that I've talked about previously is simply\n> to go use ratios, and avoid pixel coordinates altogether. Then you\n> just don't need this function. The counter argument is that an\n> application can't control digital pan/zoom down to the precise pixel -\n> which sounds appealing. Yet I think real applications will have in\n> mind a _rate_ at which they wish to work (e.g. pan across half the\n> field of view in 5 seconds) at which point ratios are quite natural.\n>\n> Best regards\n> David\n>\n> >\n> >\n> > > +}\n> > > +\n> > > /**\n> > > * \\brief Create a request object for the camera\n> > > * \\param[in] cookie Opaque cookie for application use\n> > > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml\n> > > index 988b501..5a099d5 100644\n> > > --- a/src/libcamera/control_ids.yaml\n> > > +++ b/src/libcamera/control_ids.yaml\n> > > @@ -262,4 +262,14 @@ controls:\n> > > In this respect, it is not necessarily aimed at providing a way to\n> > > implement a focus algorithm by the application, rather an indication of\n> > > how in-focus a frame is.\n> > > +\n> > > + - DigitalZoom:\n> > > + type: Rectangle\n> > > + description: |\n> > > + Sets the portion of the full sensor image, in pixels, that will be\n> > > + used for digital zoom. That is, this part of the sensor output will\n> > > + be scaled up to make the full size output image (and everything else\n> > > + discarded). To obtain the \"full sensor image\" that is available, the\n> > > + method Camera::getOutputCrop() should be called once the camera is\n> > > + configured. An application may pan and zoom within this rectangle.\n> > > ...\n> > >\n> >\n> > --\n> > Regards\n> > --\n> > Kieran","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 51551BD792\n\tfor ;\n\tTue, 21 Jul 2020 11:27:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D9DF560832;\n\tTue, 21 Jul 2020 13:27:54 +0200 (CEST)","from mail-ot1-x341.google.com (mail-ot1-x341.google.com\n\t[IPv6:2607:f8b0:4864:20::341])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B2F8D60496\n\tfor ;\n\tTue, 21 Jul 2020 13:27:53 +0200 (CEST)","by mail-ot1-x341.google.com with SMTP id d4so14807028otk.2\n\tfor ;\n\tTue, 21 Jul 2020 04:27:53 -0700 (PDT)"],"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=\"Wt+8At0l\"; 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=u2phyJ6LTc/qXna8XdcUd7JfH9P4qWKl0KzSI7YvPHQ=;\n\tb=Wt+8At0lHI1Pjec/7bCZWNTsox5jyMF2zWOiCwWheT8S1J25YaYzIzgKTeFRbcvg+o\n\t268qqj4nbx8pDO1Z5VquqyjcigK2Gi5nMc2Y+WpteegfDPQigToVGmIKBx7vUMMiFTIv\n\tCO/c+Gz3zQSK+eIkTcWSj5BB5xehQqfQUWGSFzqKJf+ZFnOjbt8b9yOERxuBwQh0Mtp+\n\tRRugt+r3xLYdnCbL1H86IaqjwyvVQFz0jRumCOc1DV11KKoDC12bXKEUWO5p+EYAPKaq\n\tUY/NK9ssXwvoE4BM+ufSPUTDNfvH2hOHWD7QynBhTxytrm/rtJ+DRv47x03CF2503lyb\n\t3o6Q==","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=u2phyJ6LTc/qXna8XdcUd7JfH9P4qWKl0KzSI7YvPHQ=;\n\tb=sn9McpuFvjuh0Qs69ACerdb9tIQdX/sVi1SexqbavG/o7WwjT9KwOSC+SUqVlOc0aM\n\t6Y/O29MthtGxfuPRstAaw/Ztb9D1yVF9cWjHY1fh+1awPouuE6YpGPLh7cXDHUNSUPnD\n\ty4yLMpgpjc903UwZlU9CPm0LVBQwzLPCVD50SAs+9lUtK1sc1npIS/0B6DY6SJanrATP\n\tuxUfLmQt3R/OjdOtdz+1xcnR8txxADOQUu0ji2g7TcYStUUAIBTUHhD0ZdwEdOWQjl8+\n\tIxs97OSiGEMx1mibrx1R5nJy8826EB7YfvUpBI8qw1CeUYhJUyUhtL9Llso42FWMX3d9\n\tjtyA==","X-Gm-Message-State":"AOAM531KM13DrQ9ra0ch4Z0QtCojAFG64SKkNYetJSMgTOsJXIZH79xn\n\tNXpyHIntc2FYm1W4aSokpXCS+lEHF82HGDryDJ9caw==","X-Google-Smtp-Source":"ABdhPJwe+Eb/VNHcvfriZWKXTfjaYX2FGMfDryCj86NRmSU9faImfaDxDJTZDirb+O7v7edhRBhZJKgcKZdHJHhlHPE=","X-Received":"by 2002:a05:6830:14d4:: with SMTP id\n\tt20mr23755000otq.166.1595330872440; \n\tTue, 21 Jul 2020 04:27:52 -0700 (PDT)","MIME-Version":"1.0","References":"<20200709091555.1617-1-david.plowman@raspberrypi.com>\n\t<20200709091555.1617-2-david.plowman@raspberrypi.com>\n\t\n\t","In-Reply-To":"","From":"David Plowman ","Date":"Tue, 21 Jul 2020 12:27:40 +0100","Message-ID":"","To":"Kieran Bingham ","Subject":"Re: [libcamera-devel] [PATCH v2 1/2] libcamera: Implement digital\n\tzoom","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":11489,"web_url":"https://patchwork.libcamera.org/comment/11489/","msgid":"<48caac22-eafa-36ef-4486-cd3a50b43415@ideasonboard.com>","date":"2020-07-22T13:35:25","subject":"Re: [libcamera-devel] [PATCH v2 1/2] libcamera: Implement digital\n\tzoom","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi David,\n\nOn 21/07/2020 12:27, David Plowman wrote:\n> Hi again Kieran, everyone\n> \n> So I've been contemplating some of this a bit more since yesterday,\n\n\nI've been trying to give this a bit more thought too... more discussion\nbelow:\n\n\n> and I wonder if I'm confusing everything by giving that contentious\n> function the name \"getSensorCrop\". It makes it sound like it's a\n> property of the sensor when it really isn't - it can change whenever\n> the sensor mode chosen changes, or the size of the requested output\n> changes. It's an interaction between the sensor mode and the behaviour\n> of the pipeline handler, so I wonder if we should be coining a\n> different name for it?\n> \n> How about....\n> \n> getInputCrop\n> getPipelineCrop\n> getPipelineSensorCrop\n> getPipelineSensorArea\n> \n> Of those I think I prefer the last one, it's the area of the sensor\n> that the pipeline handler has chosen that we can use. But does anyone\n> have any better suggestions?\n> \n\n\nI think I actually like getInputCrop(or is it setInputCrop?)\n\nPerhaps I'm getting confused as to whether this addition is to set or\nget the zoom window.\n\n\n\n> I guess I'm also still unclear as to where to store it - I'm assuming\n> that the SensorInfo isn't the right place for something that can\n> change like this. (?)\n\n\nI seem to still be confused as to what data this is representing.\n\nI.e. is it:\n\nA) A crop which reduces the output of the sensor (sensorOutputCrop) and\nfurther restricts the data output by the sensor reducing the overall\nwindow further than the analogCrop.\n\nB) A crop which restricts the input to the ISP/Scaler (scalerInputCrop)\nand must be smaller than the analogCrop from the sensor\n\n\n\nAlso, I think I have been confused by the fact that there is a Rectangle\nDigitalZoom control added in this patch, which should come later, and\nwill 'use'/'set' the crop being defined by this patch?\n\n\nMaybe a video call to discuss this sometime might help me understand\nwhat parts you are referring to.\n\n\n\n> Thanks!\n> David\n> \n> On Mon, 20 Jul 2020 at 08:19, David Plowman\n> wrote:\n>>\n>> Hi Kieran\n>>\n>> Thanks for your feedback. Apologies for the delay in replying - I've\n>> been away for the past week!\n>>\n>> I agree there's still some stuff to figure out here! Let me try and\n>> answer some of the questions (and probably pose some more!):\n>>\n>> On Fri, 10 Jul 2020 at 14:12, Kieran Bingham\n>> wrote:\n>>>\n>>> Hi David,\n>>>\n>>> On 09/07/2020 10:15, David Plowman wrote:\n>>>> These changes add a Digital Zoom control, tacking a rectangle as its\n>>>\n>>> s/tacking/taking/\n>>>\n>>>> argument, indicating the region of the sensor output that will be\n>>>> zoomed up to the final output size.\n>>>>\n>>>> Additionally, we need have a method returning the \"sensorCrop\" which\n>>>\n>>> s/need have/need to have/\n>>>\n>>>> gives the dimensions of the sensor output within which we can pan and\n>>>> zoom.\n>>>\n>>>\n>>> This commit message describes implementing digital zoom, but *this*\n>>> commit is only dealing with the representaion of a crop region in the\n>>> (CameraSensor) which can be used to implement digital zoom.\n>>>\n>>> Perhaps this commit should be titled:\n>>> \"libcamera: camera_sensor: Support cropping regions\"\n>>>\n>>> (with that being the assumption that my comments below about moving this\n>>> to the CameraSensor class are correct)\n>>>\n>>>\n>>>\n>>>> ---\n>>>> include/libcamera/camera.h | 2 ++\n>>>> include/libcamera/internal/pipeline_handler.h | 4 +++\n>>>> src/libcamera/camera.cpp | 26 +++++++++++++++++++\n>>>> src/libcamera/control_ids.yaml | 10 +++++++\n>>>> 4 files changed, 42 insertions(+)\n>>>>\n>>>> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n>>>> index 4d1a4a9..dd07f7a 100644\n>>>> --- a/include/libcamera/camera.h\n>>>> +++ b/include/libcamera/camera.h\n>>>> @@ -92,6 +92,8 @@ public:\n>>>> std::unique_ptr generateConfiguration(const StreamRoles &roles = {});\n>>>> int configure(CameraConfiguration *config);\n>>>>\n>>>> + Size const &getSensorCrop() const;\n>>>> +\n>>>> Request *createRequest(uint64_t cookie = 0);\n>>>> int queueRequest(Request *request);\n>>>>\n>>>> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n>>>> index 22e629a..37e069a 100644\n>>>> --- a/include/libcamera/internal/pipeline_handler.h\n>>>> +++ b/include/libcamera/internal/pipeline_handler.h\n>>>> @@ -89,6 +89,8 @@ public:\n>>>>\n>>>> const char *name() const { return name_; }\n>>>>\n>>>> + Size const &getSensorCrop() const { return sensorCrop_; }\n>>>> +\n>>>> protected:\n>>>> void registerCamera(std::shared_ptr camera,\n>>>> std::unique_ptr data);\n>>>> @@ -100,6 +102,8 @@ protected:\n>>>>\n>>>> CameraManager *manager_;\n>>>>\n>>>> + Size sensorCrop_;\n>>>> +\n>>>\n>>> I think this needs to go in the CameraSensor class...\n>>> It's 'per sensor' not 'per pipeline handler' right?\n>>\n>> Maybe this is the billion dollar question. See below...!\n\n\nI might also have meant CameraData ... but it depends still...\n\n\n>>>> private:\n>>>> void mediaDeviceDisconnected(MediaDevice *media);\n>>>> virtual void disconnect();\n>>>> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n>>>> index 69a1b44..6614c93 100644\n>>>> --- a/src/libcamera/camera.cpp\n>>>> +++ b/src/libcamera/camera.cpp\n>>>> @@ -793,6 +793,32 @@ int Camera::configure(CameraConfiguration *config)\n>>>> return 0;\n>>>> }\n>>>>\n>>>> +/**\n>>>> + * \\brief Return the size of the sensor image being used to form the output\n>>>> + *\n>>>> + * This method returns the size, in pixels, of the raw image read from the\n>>>> + * sensor and which is used to form the output image(s). Note that these\n>>>> + * values take account of any cropping performed on the sensor output so\n>>>> + * as to produce the correct aspect ratio. It would normally be necessary\n>>>> + * to retrieve these values in order to calculate correct parameters for\n>>>> + * digital zoom.\n>>>> + *\n>>>> + * Example: a sensor mode may produce a 1920x1440 output image. But if an\n>>>> + * application has requested a 16:9 image, the values returned here would\n>>>> + * be 1920x1080 - the largest portion of the sensor output that provides\n>>>> + * the correct aspect ratio.\n>>>\n>>> Should this be a Rectangle? Can the crop region be positioned\n>>> arbitrarily or is it always centered?\n>>\n>> As I've implemented this, it is just a pair of dimensions. The\n>> pipeline handler may position it anywhere it likes within the array of\n>> pixels output by the sensor (and takes care of any offset within\n>> that), but the application doesn't need to know. The idea being that\n>> the application gets to choose a *rectangle* from within the\n>> dimensions it is given and doesn't have to bother itself with how it's\n>> offset in the pixel array beyond that.\n\nSurely the position is quite essential to define somehow though,\notherwise we'll get some zoom which will zoom in on the bottom right\ncorner and be perfectly valid, while another zoom chooses the centre,\nand another the top left ... ?\n\nI assume centreing is usually the right thing to do - but I don't know ...\n\nIf it is defined as always zooming in on the centre point then indeed a\nSize would be the only thing needed to store.\n\n\n\n\n\n\n>>> What is the relationship between this and the analogCrop in\n>>> CameraSensorInfo? I 'think' that one is more about what the actual valid\n>>> data region is from the camera, so I presume the analogCrop is sort of\n>>> the 'maximum' image size?\n>>\n>> Yes, I think the analogCrop is the maximum usable pixel area from the\n>> sensor. The crop here is what the pipeline handler decides it wants to\n>> use. It's adjusted for aspect ratio (of the requested output), the\n>> actual size that it wants to use (and hence the scaling). I certainly\n>> agree that it might want putting somewhere else... yet it's only the\n>> pipeline handler that can calculate it. It depends on the sensor, but\n>> it also depends on the mode selected, the output size requested, and\n>> \"arbitrary\" decisions made in the pipeline handler (for example, they\n>> may wish to avoid marginal scaling and prefer 1:1 input to output\n>> pixels).\n\n\nJust to clarify further, do you expect that this control changes the\nmode of the camera at all? I.e. does it crop the data coming from the\nsensor? Or is this purely applying the input crop to the rescaler ?\n\n\n>>>> + *\n>>>> + * \\context This function is \\threadsafe. It will only return valid\n>>>> + * (non-zero) values when the camera has been configured.\n>>>> + *\n>>>> + * \\return The dimensions of the sensor image in use.\n>>>> + */\n>>>> +\n>>>> +Size const &Camera::getSensorCrop() const\n>>>> +{\n>>>> + return p_->pipe_->getSensorCrop();\n>>>\n>>> Aha, more indication to me that this should go in to the Camera class,\n>>> and it would save the indirection required here...\n>>\n>> Indeed. But given that the pipeline handler calculates it, can we have\n>> it poke its value into the Camera class? Or is that weird if the\n>> number depends on rather more than just the camera? (And will change\n>> every time we start the sensor in a different mode.)\n\nHrm - ok so that depends on something I assumed. I assumed that this\nwould be setting a property on the sensor as well, but now I realise I\nthink this is just defining a crop which will determine what window to\nread from at the input to the rescaler.\n\n\n>> Another solution to this that I've talked about previously is simply\n>> to go use ratios, and avoid pixel coordinates altogether. Then you\n>> just don't need this function. The counter argument is that an\n>> application can't control digital pan/zoom down to the precise pixel -\n>> which sounds appealing. Yet I think real applications will have in\n>> mind a _rate_ at which they wish to work (e.g. pan across half the\n>> field of view in 5 seconds) at which point ratios are quite natural.\n\nYes, I think I am used to seeing zoom in terms of a multiplier, but that\nwon't allow positioning/panning within the region.\n\n\n\n\n\n>>\n>> Best regards\n>> David\n>>\n>>>\n>>>\n>>>> +}\n>>>> +\n>>>> /**\n>>>> * \\brief Create a request object for the camera\n>>>> * \\param[in] cookie Opaque cookie for application use\n>>>> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml\n>>>> index 988b501..5a099d5 100644\n>>>> --- a/src/libcamera/control_ids.yaml\n>>>> +++ b/src/libcamera/control_ids.yaml\n>>>> @@ -262,4 +262,14 @@ controls:\n>>>> In this respect, it is not necessarily aimed at providing a way to\n>>>> implement a focus algorithm by the application, rather an indication of\n>>>> how in-focus a frame is.\n>>>> +\n>>>> + - DigitalZoom:\n>>>> + type: Rectangle\n>>>> + description: |\n>>>> + Sets the portion of the full sensor image, in pixels, that will be\n>>>> + used for digital zoom. That is, this part of the sensor output will\n>>>> + be scaled up to make the full size output image (and everything else\n>>>> + discarded). To obtain the \"full sensor image\" that is available, the\n>>>> + method Camera::getOutputCrop() should be called once the camera is\n\nAha, so the DigitalZoom is a rectangle that it can zoom in on... I think\nI confused this DigitalZoom rectangle with the getSensorCrop() above.\n\nBut that also highlights a confusion in this control documentation, Did\nyou mean getSensorCrop when you stated getOutputCrop above?\n\n\nI think this actual DigitalZoom control addition perhaps shouldn't be in\nthis patch, and should be in a patch on it's own after, or in patch 2/2\nin the series?\n\nOh, and now I see both patches have the same title - That might need\nfixing too to clarify intent of each patch ;-) It probably is also why I\nhave been confused. I thought this patch was implementing more of the\ndigital zoom than I realised, and actually the zooming part is in the\nnext patch...\n\n\n\n\n\n>>>> + configured. An application may pan and zoom within this rectangle.\n>>>> ...\n>>>>\n>>>\n>>> --\n>>> Regards\n>>> --\n>>> Kieran","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 6BFB8C2E68\n\tfor ;\n\tWed, 22 Jul 2020 13:35:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D019160983;\n\tWed, 22 Jul 2020 15:35:31 +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 0523C6053C\n\tfor ;\n\tWed, 22 Jul 2020 15:35:31 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89242-aztw30-2-0-cust488.18-1.cable.virginm.net [86.31.129.233])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 84047329;\n\tWed, 22 Jul 2020 15:35:28 +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=\"c0HH9z2W\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1595424928;\n\tbh=WX64jEDJ0M+4ozyLezn8XeTsz9WHENkh+VbT2vdeUOo=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=c0HH9z2WJ6CNU+15wWv4EP6P58goB1bL0lnkwZka4yWhtg652hdChOW2cm19VhV7/\n\tGrHOxFFi8k9P+hH9niPDQkcIbro4QGR3Iyba9Y2+goIdH4Ae9APnquMombCgBBqdQb\n\tNhLwbg4QOQkxPw108pE//7Jngebq2V7a8U6vKmMg=","To":"David Plowman ","References":"<20200709091555.1617-1-david.plowman@raspberrypi.com>\n\t<20200709091555.1617-2-david.plowman@raspberrypi.com>\n\t\n\t\n\t","From":"Kieran Bingham ","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"<48caac22-eafa-36ef-4486-cd3a50b43415@ideasonboard.com>","Date":"Wed, 22 Jul 2020 14:35:25 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.10.0","MIME-Version":"1.0","In-Reply-To":"","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH v2 1/2] libcamera: Implement digital\n\tzoom","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","Reply-To":"kieran.bingham@ideasonboard.com","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":11517,"web_url":"https://patchwork.libcamera.org/comment/11517/","msgid":"","date":"2020-07-22T17:58:23","subject":"Re: [libcamera-devel] [PATCH v2 1/2] libcamera: Implement digital\n\tzoom","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Kieran and everyone\n\nThanks for sticking with this gnarly subject! Perhaps I haven't helped\nmyself with some poor naming choices. You may be right that it\nactually wants a \"face to face\" discussion, but before that maybe I'll\nhave one more go at an explanation, and then I'll post a new version\nof the patches with some tidy-ups and improved nomenclature.\n\nSo there are lots of pixels and crops flying about. There may well be\nsome going on in the sensor, but here I'm talking about the crop that\nis applied within the ISP, before it all gets rescaled to create the\noutput images. (Our pipeline handler shovels all the pixels we receive\nfrom the sensor into the ISP; even those that get discarded are still\nuseful for things like statistics.)\n\nFrom now on (and until someone changes my mind again) I'm going to\ncall this the *pipeline crop*. How is the \"pipeline crop\" decided?\n\nLet's suppose we're using the imx477 sensor and we want 1080p\noutput. The pipeline handler will probably choose the 2x2 binned mode,\ngiving us 2028x1520 pixels from the sensor.\n\nWe're going to feed all of this into the ISP. But the ISP has to apply\na crop before it rescales everything to the output size of 1920x1080 in\nour case.\n\nOne obvious choice would be to take the largest rectangle from the\n2028x1520 pixels that matches the output aspect ratio, and then to\ncentre this rectangle within the 2028x1520 pixels. This would give us\na \"pipeline crop\" of 2028x1140 pixels, with the top left pixel taken\nfrom (0,190) in the 2028x1520 array from the sensor.\n\nBut the pipeline handler could have chosen something different. If the\nthought of not-quite-1-to-1 scaling gives the pipeline handler the\ncreeps, it might have chosen a \"pipeline crop\" of 1920x1080, taken\nfrom (54,220).\n\nSo the \"pipeline crop\" depends on: the sensor, the current sensor\nmode, the aspect ratio of the output images, and basically what mood\nthe pipeline handler was in at that moment.\n\nThe idea is then that the pipeline crop is given to the application,\nwhich, in our example, was either 2028x1140 or 1920x1080. The\napplication pans and zooms about within there. Notice how the offsets\n- (0,190) or (54,220) - are immaterial to the application; the\npipeline handler will add them on transparently before setting the\nV4L2 \"selection\".\n\nSome additional notes:\n\n1. There's also a policy decision here that you can't pan into regions\nof image that were not selected by the pipeline crop. This seems\nreasonable to me; allowing that to happen would create something both\nslightly surprising and fiddly, and I don't really see any upside.\n\n2. You could implement digital zoom by cropping in the sensor itself\nrather than the ISP, though I think this would be unusual. But the\ndiscussion above works just the same, with the pipeline crop meaning\n\"the largest crop we're prepared to take from the sensor\". So it does\nmuddy the meaning a bit, but I'm stuck as to what else to call it!\n\nAnyway, thanks again everyone and please watch out for a revised\npatch set tomorrow.\n\nDavid\n\nOn Wed, 22 Jul 2020 at 14:35, Kieran Bingham\n wrote:\n>\n> Hi David,\n>\n> On 21/07/2020 12:27, David Plowman wrote:\n> > Hi again Kieran, everyone\n> >\n> > So I've been contemplating some of this a bit more since yesterday,\n>\n>\n> I've been trying to give this a bit more thought too... more discussion\n> below:\n>\n>\n> > and I wonder if I'm confusing everything by giving that contentious\n> > function the name \"getSensorCrop\". It makes it sound like it's a\n> > property of the sensor when it really isn't - it can change whenever\n> > the sensor mode chosen changes, or the size of the requested output\n> > changes. It's an interaction between the sensor mode and the behaviour\n> > of the pipeline handler, so I wonder if we should be coining a\n> > different name for it?\n> >\n> > How about....\n> >\n> > getInputCrop\n> > getPipelineCrop\n> > getPipelineSensorCrop\n> > getPipelineSensorArea\n> >\n> > Of those I think I prefer the last one, it's the area of the sensor\n> > that the pipeline handler has chosen that we can use. But does anyone\n> > have any better suggestions?\n> >\n>\n>\n> I think I actually like getInputCrop(or is it setInputCrop?)\n>\n> Perhaps I'm getting confused as to whether this addition is to set or\n> get the zoom window.\n>\n>\n>\n> > I guess I'm also still unclear as to where to store it - I'm assuming\n> > that the SensorInfo isn't the right place for something that can\n> > change like this. (?)\n>\n>\n> I seem to still be confused as to what data this is representing.\n>\n> I.e. is it:\n>\n> A) A crop which reduces the output of the sensor (sensorOutputCrop) and\n> further restricts the data output by the sensor reducing the overall\n> window further than the analogCrop.\n>\n> B) A crop which restricts the input to the ISP/Scaler (scalerInputCrop)\n> and must be smaller than the analogCrop from the sensor\n>\n>\n>\n> Also, I think I have been confused by the fact that there is a Rectangle\n> DigitalZoom control added in this patch, which should come later, and\n> will 'use'/'set' the crop being defined by this patch?\n>\n>\n> Maybe a video call to discuss this sometime might help me understand\n> what parts you are referring to.\n>\n>\n>\n> > Thanks!\n> > David\n> >\n> > On Mon, 20 Jul 2020 at 08:19, David Plowman\n> > wrote:\n> >>\n> >> Hi Kieran\n> >>\n> >> Thanks for your feedback. Apologies for the delay in replying - I've\n> >> been away for the past week!\n> >>\n> >> I agree there's still some stuff to figure out here! Let me try and\n> >> answer some of the questions (and probably pose some more!):\n> >>\n> >> On Fri, 10 Jul 2020 at 14:12, Kieran Bingham\n> >> wrote:\n> >>>\n> >>> Hi David,\n> >>>\n> >>> On 09/07/2020 10:15, David Plowman wrote:\n> >>>> These changes add a Digital Zoom control, tacking a rectangle as its\n> >>>\n> >>> s/tacking/taking/\n> >>>\n> >>>> argument, indicating the region of the sensor output that will be\n> >>>> zoomed up to the final output size.\n> >>>>\n> >>>> Additionally, we need have a method returning the \"sensorCrop\" which\n> >>>\n> >>> s/need have/need to have/\n> >>>\n> >>>> gives the dimensions of the sensor output within which we can pan and\n> >>>> zoom.\n> >>>\n> >>>\n> >>> This commit message describes implementing digital zoom, but *this*\n> >>> commit is only dealing with the representaion of a crop region in the\n> >>> (CameraSensor) which can be used to implement digital zoom.\n> >>>\n> >>> Perhaps this commit should be titled:\n> >>> \"libcamera: camera_sensor: Support cropping regions\"\n> >>>\n> >>> (with that being the assumption that my comments below about moving this\n> >>> to the CameraSensor class are correct)\n> >>>\n> >>>\n> >>>\n> >>>> ---\n> >>>> include/libcamera/camera.h | 2 ++\n> >>>> include/libcamera/internal/pipeline_handler.h | 4 +++\n> >>>> src/libcamera/camera.cpp | 26 +++++++++++++++++++\n> >>>> src/libcamera/control_ids.yaml | 10 +++++++\n> >>>> 4 files changed, 42 insertions(+)\n> >>>>\n> >>>> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> >>>> index 4d1a4a9..dd07f7a 100644\n> >>>> --- a/include/libcamera/camera.h\n> >>>> +++ b/include/libcamera/camera.h\n> >>>> @@ -92,6 +92,8 @@ public:\n> >>>> std::unique_ptr generateConfiguration(const StreamRoles &roles = {});\n> >>>> int configure(CameraConfiguration *config);\n> >>>>\n> >>>> + Size const &getSensorCrop() const;\n> >>>> +\n> >>>> Request *createRequest(uint64_t cookie = 0);\n> >>>> int queueRequest(Request *request);\n> >>>>\n> >>>> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h\n> >>>> index 22e629a..37e069a 100644\n> >>>> --- a/include/libcamera/internal/pipeline_handler.h\n> >>>> +++ b/include/libcamera/internal/pipeline_handler.h\n> >>>> @@ -89,6 +89,8 @@ public:\n> >>>>\n> >>>> const char *name() const { return name_; }\n> >>>>\n> >>>> + Size const &getSensorCrop() const { return sensorCrop_; }\n> >>>> +\n> >>>> protected:\n> >>>> void registerCamera(std::shared_ptr camera,\n> >>>> std::unique_ptr data);\n> >>>> @@ -100,6 +102,8 @@ protected:\n> >>>>\n> >>>> CameraManager *manager_;\n> >>>>\n> >>>> + Size sensorCrop_;\n> >>>> +\n> >>>\n> >>> I think this needs to go in the CameraSensor class...\n> >>> It's 'per sensor' not 'per pipeline handler' right?\n> >>\n> >> Maybe this is the billion dollar question. See below...!\n>\n>\n> I might also have meant CameraData ... but it depends still...\n>\n>\n> >>>> private:\n> >>>> void mediaDeviceDisconnected(MediaDevice *media);\n> >>>> virtual void disconnect();\n> >>>> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> >>>> index 69a1b44..6614c93 100644\n> >>>> --- a/src/libcamera/camera.cpp\n> >>>> +++ b/src/libcamera/camera.cpp\n> >>>> @@ -793,6 +793,32 @@ int Camera::configure(CameraConfiguration *config)\n> >>>> return 0;\n> >>>> }\n> >>>>\n> >>>> +/**\n> >>>> + * \\brief Return the size of the sensor image being used to form the output\n> >>>> + *\n> >>>> + * This method returns the size, in pixels, of the raw image read from the\n> >>>> + * sensor and which is used to form the output image(s). Note that these\n> >>>> + * values take account of any cropping performed on the sensor output so\n> >>>> + * as to produce the correct aspect ratio. It would normally be necessary\n> >>>> + * to retrieve these values in order to calculate correct parameters for\n> >>>> + * digital zoom.\n> >>>> + *\n> >>>> + * Example: a sensor mode may produce a 1920x1440 output image. But if an\n> >>>> + * application has requested a 16:9 image, the values returned here would\n> >>>> + * be 1920x1080 - the largest portion of the sensor output that provides\n> >>>> + * the correct aspect ratio.\n> >>>\n> >>> Should this be a Rectangle? Can the crop region be positioned\n> >>> arbitrarily or is it always centered?\n> >>\n> >> As I've implemented this, it is just a pair of dimensions. The\n> >> pipeline handler may position it anywhere it likes within the array of\n> >> pixels output by the sensor (and takes care of any offset within\n> >> that), but the application doesn't need to know. The idea being that\n> >> the application gets to choose a *rectangle* from within the\n> >> dimensions it is given and doesn't have to bother itself with how it's\n> >> offset in the pixel array beyond that.\n>\n> Surely the position is quite essential to define somehow though,\n> otherwise we'll get some zoom which will zoom in on the bottom right\n> corner and be perfectly valid, while another zoom chooses the centre,\n> and another the top left ... ?\n>\n> I assume centreing is usually the right thing to do - but I don't know ...\n>\n> If it is defined as always zooming in on the centre point then indeed a\n> Size would be the only thing needed to store.\n>\n>\n>\n>\n>\n>\n> >>> What is the relationship between this and the analogCrop in\n> >>> CameraSensorInfo? I 'think' that one is more about what the actual valid\n> >>> data region is from the camera, so I presume the analogCrop is sort of\n> >>> the 'maximum' image size?\n> >>\n> >> Yes, I think the analogCrop is the maximum usable pixel area from the\n> >> sensor. The crop here is what the pipeline handler decides it wants to\n> >> use. It's adjusted for aspect ratio (of the requested output), the\n> >> actual size that it wants to use (and hence the scaling). I certainly\n> >> agree that it might want putting somewhere else... yet it's only the\n> >> pipeline handler that can calculate it. It depends on the sensor, but\n> >> it also depends on the mode selected, the output size requested, and\n> >> \"arbitrary\" decisions made in the pipeline handler (for example, they\n> >> may wish to avoid marginal scaling and prefer 1:1 input to output\n> >> pixels).\n>\n>\n> Just to clarify further, do you expect that this control changes the\n> mode of the camera at all? I.e. does it crop the data coming from the\n> sensor? Or is this purely applying the input crop to the rescaler ?\n>\n>\n> >>>> + *\n> >>>> + * \\context This function is \\threadsafe. It will only return valid\n> >>>> + * (non-zero) values when the camera has been configured.\n> >>>> + *\n> >>>> + * \\return The dimensions of the sensor image in use.\n> >>>> + */\n> >>>> +\n> >>>> +Size const &Camera::getSensorCrop() const\n> >>>> +{\n> >>>> + return p_->pipe_->getSensorCrop();\n> >>>\n> >>> Aha, more indication to me that this should go in to the Camera class,\n> >>> and it would save the indirection required here...\n> >>\n> >> Indeed. But given that the pipeline handler calculates it, can we have\n> >> it poke its value into the Camera class? Or is that weird if the\n> >> number depends on rather more than just the camera? (And will change\n> >> every time we start the sensor in a different mode.)\n>\n> Hrm - ok so that depends on something I assumed. I assumed that this\n> would be setting a property on the sensor as well, but now I realise I\n> think this is just defining a crop which will determine what window to\n> read from at the input to the rescaler.\n>\n>\n> >> Another solution to this that I've talked about previously is simply\n> >> to go use ratios, and avoid pixel coordinates altogether. Then you\n> >> just don't need this function. The counter argument is that an\n> >> application can't control digital pan/zoom down to the precise pixel -\n> >> which sounds appealing. Yet I think real applications will have in\n> >> mind a _rate_ at which they wish to work (e.g. pan across half the\n> >> field of view in 5 seconds) at which point ratios are quite natural.\n>\n> Yes, I think I am used to seeing zoom in terms of a multiplier, but that\n> won't allow positioning/panning within the region.\n>\n>\n>\n>\n>\n> >>\n> >> Best regards\n> >> David\n> >>\n> >>>\n> >>>\n> >>>> +}\n> >>>> +\n> >>>> /**\n> >>>> * \\brief Create a request object for the camera\n> >>>> * \\param[in] cookie Opaque cookie for application use\n> >>>> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml\n> >>>> index 988b501..5a099d5 100644\n> >>>> --- a/src/libcamera/control_ids.yaml\n> >>>> +++ b/src/libcamera/control_ids.yaml\n> >>>> @@ -262,4 +262,14 @@ controls:\n> >>>> In this respect, it is not necessarily aimed at providing a way to\n> >>>> implement a focus algorithm by the application, rather an indication of\n> >>>> how in-focus a frame is.\n> >>>> +\n> >>>> + - DigitalZoom:\n> >>>> + type: Rectangle\n> >>>> + description: |\n> >>>> + Sets the portion of the full sensor image, in pixels, that will be\n> >>>> + used for digital zoom. That is, this part of the sensor output will\n> >>>> + be scaled up to make the full size output image (and everything else\n> >>>> + discarded). To obtain the \"full sensor image\" that is available, the\n> >>>> + method Camera::getOutputCrop() should be called once the camera is\n>\n> Aha, so the DigitalZoom is a rectangle that it can zoom in on... I think\n> I confused this DigitalZoom rectangle with the getSensorCrop() above.\n>\n> But that also highlights a confusion in this control documentation, Did\n> you mean getSensorCrop when you stated getOutputCrop above?\n>\n>\n> I think this actual DigitalZoom control addition perhaps shouldn't be in\n> this patch, and should be in a patch on it's own after, or in patch 2/2\n> in the series?\n>\n> Oh, and now I see both patches have the same title - That might need\n> fixing too to clarify intent of each patch ;-) It probably is also why I\n> have been confused. I thought this patch was implementing more of the\n> digital zoom than I realised, and actually the zooming part is in the\n> next patch...\n>\n>\n>\n>\n>\n> >>>> + configured. An application may pan and zoom within this rectangle.\n> >>>> ...\n> >>>>\n> >>>\n> >>> --\n> >>> Regards\n> >>> --\n> >>> Kieran\n>\n> --\n> Regards\n> --\n> Kieran","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 45EA3C2E68\n\tfor ;\n\tWed, 22 Jul 2020 17:58:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A6ACF60C34;\n\tWed, 22 Jul 2020 19:58:37 +0200 (CEST)","from mail-oi1-x242.google.com (mail-oi1-x242.google.com\n\t[IPv6:2607:f8b0:4864:20::242])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7922260540\n\tfor ;\n\tWed, 22 Jul 2020 19:58:36 +0200 (CEST)","by mail-oi1-x242.google.com with SMTP id x83so2617870oif.10\n\tfor ;\n\tWed, 22 Jul 2020 10:58:36 -0700 (PDT)"],"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=\"k+miYiiQ\"; 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=//Wc3KMGBlunsMuYjPeL9GkPu4i9NvCvUN7EegD3IUI=;\n\tb=k+miYiiQWCSM54IlzfGbRS7sDX2Zzm91IpQmjhtmHyrQN61PfhDaqZ3IeMQx3nyFw6\n\tQ6EB0aKrz4J0x5iTjcjxt21xaOP7r0U/MqJBKAIezfYcqQ1mizOHJCLLPN5LX2zx03u1\n\tz3brBhNTTlwWGy8Zx/zH5f/nX9q3DQBDq2LKgzl4hjKt3NI/sjIJJ4fd2ddCIr9A6KK2\n\t5+5g3rGjE8ehW9RrdHNOTCgcoNmgVyW1DQ2hRk46Hx7aNEeSKIqJ9BHfAPkUF+tzHY+6\n\trsRr9j6xB9FuHylx8JV7qGkGC3e7w3gz+lAXAInWINFMpV/FxZpwwUOIKlA7luEXdLWp\n\t4yjQ==","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=//Wc3KMGBlunsMuYjPeL9GkPu4i9NvCvUN7EegD3IUI=;\n\tb=Tu1I2P9cJKczf3/vEnyVTSUsTf+zyVo4KQVgABGPcmrj7MxUNmay5q/Jg3LLgY5Qzn\n\t12u6vkZRCWcmJgSOPyaxBJWl2dn4fKNb+1kE4Xe7vnaeINTBi/Gz6TrU8y9cba5BU8gd\n\tGEvI9v2xZHnYAHffYziFMXu0fW2yTt4iGOARuuIqPcpwjrg0DG0KbtNNRBUwF8uOgGhL\n\tFJTVa2QmE76Y7/pvMoV0mYawNGh966SLuX3cuuXbzV0dRQUJ3k4q5zfxlgRO+18PZGgC\n\t5W+wDzjme5oahs3l/YneCo1Ba6m16hBB+Y3oD3HVTl0KSCh6wo1cxqo8H9fV/Pl0Cg4/\n\t/bTw==","X-Gm-Message-State":"AOAM530Laalb/GMh7R9QzEU1Rj9W4YXH4tmUaaWTkvRUOXnYc0BmFneH\n\tcXqVmTxELCglbTeV7wpuS6gJxEnhMh5VkwviiHWwcg==","X-Google-Smtp-Source":"ABdhPJz5TsUNrMfK0CtgwAogMKtwPyzjI+bzKv/Vobnq9+2AxbK4GZJ+sknNiibprPTOPrEeI/CWZYy64OaTbvDTw1c=","X-Received":"by 2002:aca:ac01:: with SMTP id v1mr709900oie.22.1595440714675; \n\tWed, 22 Jul 2020 10:58:34 -0700 (PDT)","MIME-Version":"1.0","References":"<20200709091555.1617-1-david.plowman@raspberrypi.com>\n\t<20200709091555.1617-2-david.plowman@raspberrypi.com>\n\t\n\t\n\t\n\t<48caac22-eafa-36ef-4486-cd3a50b43415@ideasonboard.com>","In-Reply-To":"<48caac22-eafa-36ef-4486-cd3a50b43415@ideasonboard.com>","From":"David Plowman ","Date":"Wed, 22 Jul 2020 18:58:23 +0100","Message-ID":"","To":"Kieran Bingham ","Subject":"Re: [libcamera-devel] [PATCH v2 1/2] libcamera: Implement digital\n\tzoom","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\" "}}]