[{"id":12656,"web_url":"https://patchwork.libcamera.org/comment/12656/","msgid":"<20200923095134.qlos3gdjs2i5f34p@uno.localdomain>","date":"2020-09-23T09:51:34","subject":"Re: [libcamera-devel] [PATCH 5/6] libcamera: pipeline: raspberrypi:\n\tImplementation of digital zoom","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi David,\n\nOn Tue, Sep 22, 2020 at 11:03:59AM +0100, David Plowman wrote:\n> During configure() we update the SensorOutputSize to the correct value\n> for this camera mode (which we also note internally) and work out the\n> minimum crop size allowed by the ISP.\n>\n> Whenever a new IspCrop request is received we check it's valid and\n> apply it to the ISP V4L2 device. We also forward it to the IPA so that\n> the IPA can return the values used in the image metadata.\n>\n> Signed-off-by: David Plowman \n> ---\n> include/libcamera/ipa/raspberrypi.h | 1 +\n> src/ipa/raspberrypi/raspberrypi.cpp | 7 +++\n> .../pipeline/raspberrypi/raspberrypi.cpp | 47 +++++++++++++++++++\n> 3 files changed, 55 insertions(+)\n>\n> diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h\n> index dd6ebea..91a1892 100644\n> --- a/include/libcamera/ipa/raspberrypi.h\n> +++ b/include/libcamera/ipa/raspberrypi.h\n> @@ -58,6 +58,7 @@ static const ControlInfoMap RPiControls = {\n> \t{ &controls::Saturation, ControlInfo(0.0f, 32.0f) },\n> \t{ &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },\n> \t{ &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) },\n> +\t{ &controls::IspCrop, ControlInfo(Rectangle(0, 0, 0, 0), Rectangle(65535, 65535, 65535, 65535), Rectangle(0, 0, 0, 0)) },\n\nIf you feel it's more convenient, you can\ns/Rectangle(0, 0, 0, 0)/{}\n\nwhatever you like the most\n\n> };\n>\n> } /* namespace libcamera */\n> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> index 0555cc4..64f0e35 100644\n> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> @@ -687,6 +687,13 @@ void IPARPi::queueRequest(const ControlList &controls)\n> \t\t\tbreak;\n> \t\t}\n>\n> +\t\tcase controls::ISP_CROP: {\n> +\t\t\t/* Just copy the information back. */\n> +\t\t\tRectangle crop = ctrl.second.get();\n> +\t\t\tlibcameraMetadata_.set(controls::IspCrop, crop);\n> +\t\t\tbreak;\n> +\t\t}\n> +\n\nI wonder if this needs to go to the IPA at all.. I like the symmetry with\nother controls but the IPA is actually not involved at all in the process...\nDo you expect this to change ? Otherwise the metadata can be updated here\nwith the value of lastIspCrop_ ?\n\n> \t\tdefault:\n> \t\t\tLOG(IPARPI, Warning)\n> \t\t\t\t<< \"Ctrl \" << controls::controls.at(ctrl.first)->name()\n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index 50f0718..1674f8f 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -182,6 +182,11 @@ public:\n> \tstd::queue embeddedQueue_;\n> \tstd::deque requestQueue_;\n>\n> +\t/* For handling digital zoom. */\n> +\tSize ispMinSize_;\n> +\tSize sensorOutputSize_;\n> +\tRectangle lastIspCrop_;\n> +\n> \tunsigned int dropFrameCount_;\n>\n> private:\n> @@ -496,6 +501,14 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)\n> \tLOG(RPI, Info) << \"Sensor: \" << camera->id()\n> \t\t << \" - Selected mode: \" << sensorFormat.toString();\n>\n> +\t/*\n> +\t * Update the SensorOutputSize to the correct value for this camera mode.\n> +\t *\n> +\t * \\todo Move this property to CameraConfiguration when that\n> +\t * feature will be made available and set it at validate() time\n> +\t */\n> +\tdata->properties_.set(properties::SensorOutputSize, sensorFormat.size);\n\nThe next instruction is to apply 'sensorFormat' to the ISP input node.\nI don't know if this operation is expected to change the format, but I\nwould however update the property value after that.\n\n> +\n> \t/*\n> \t * This format may be reset on start() if the bayer order has changed\n> \t * because of flips in the sensor.\n> @@ -592,6 +605,16 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)\n> \t\treturn ret;\n> \t}\n>\n> +\t/*\n> +\t * Figure out the smallest selection the ISP will allow. We also store\n> +\t * the output image size, in pixels, from the sensor. These will be\n> +\t * used for digital zoom.\n> +\t */\n> +\tRectangle testCrop(0, 0, 1, 1);\n> +\tdata->isp_[Isp::Input].dev()->setSelection(V4L2_SEL_TGT_CROP, &testCrop);\n> +\tdata->ispMinSize_ = testCrop.size();\n> +\tdata->sensorOutputSize_ = sensorFormat.size;\n> +\n> \t/* Adjust aspect ratio by providing crops on the input image. */\n> \tRectangle crop{ 0, 0, sensorFormat.size };\n>\n> @@ -608,6 +631,8 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)\n> \tcrop.y = (sensorFormat.size.height - crop.height) >> 1;\n> \tdata->isp_[Isp::Input].dev()->setSelection(V4L2_SEL_TGT_CROP, &crop);\n>\n> +\tdata->lastIspCrop_ = crop;\n> +\n> \tret = data->configureIPA();\n> \tif (ret)\n> \t\tLOG(RPI, Error) << \"Failed to configure the IPA: \" << ret;\n> @@ -1449,6 +1474,28 @@ void RPiCameraData::tryRunPipeline()\n> \t/* Take the first request from the queue and action the IPA. */\n> \tRequest *request = requestQueue_.front();\n>\n> +\tif (request->controls().contains(controls::IspCrop)) {\n> +\t\tRectangle crop = request->controls().get(controls::IspCrop);\n> +\t\tif (crop.width && crop.height) {\n> +\t\t\t/*\n> +\t\t\t * The crop that we set must be:\n> +\t\t\t * 1. At least as big as ispMinSize_, once that's been\n> +\t\t\t * enlarged to the same aspect ratio.\n> +\t\t\t * 2. With the same mid-point, if possible.\n> +\t\t\t * 3. But it can't go outside the sensor area.\n> +\t\t\t */\n> +\t\t\tSize minSize = ispMinSize_.alignedUpToAspectRatio(crop.size());\n> +\t\t\tSize size = crop.size().expandedTo(minSize);\n> +\t\t\tcrop = size.centredTo(crop).boundedTo(Rectangle(sensorOutputSize_));\n\nI might again be missing something, but isn't this equal to:\n crop.size().expandTo(minSize).boundTo({sensorOutputSize_});\n\nI'm missing the need to centering, 'crop' has already it's own offsets\nset, so we just need to make sure it's larger that the minSize and\nsmaller than the sensor output size.\n\nGeneral question: how do we expect applications to reset crop ? By\nsetting it to the sensorOutputSize (or at any larger value). Do we\nwant to predispose a special case, like a {0, 0, 0, 0} crop rectangle\nresets the crop ?\n\n> +\n> +\t\t\tif (crop != lastIspCrop_)\n> +\t\t\t\tisp_[Isp::Input].dev()->setSelection(V4L2_SEL_TGT_CROP, &crop);\n> +\t\t\tlastIspCrop_ = crop;\n> +\t\t}\n> +\n> +\t\trequest->controls().set(controls::IspCrop, lastIspCrop_);\n> +\t}\n> +\n\nVery nice, I think as soon we can test this easily with qcam we'll see\ninteresting results!\n\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n> \t/*\n> \t * Process all the user controls by the IPA. Once this is complete, we\n> \t * queue the ISP output buffer listed in the request to start the HW\n> --\n> 2.20.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 62B7AC3B5B\n\tfor ;\n\tWed, 23 Sep 2020 09:47:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EC50362FDC;\n\tWed, 23 Sep 2020 11:47:42 +0200 (CEST)","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 615F360363\n\tfor ;\n\tWed, 23 Sep 2020 11:47:42 +0200 (CEST)","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 D694E60008;\n\tWed, 23 Sep 2020 09:47:41 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Wed, 23 Sep 2020 11:51:34 +0200","From":"Jacopo Mondi ","To":"David Plowman ","Message-ID":"<20200923095134.qlos3gdjs2i5f34p@uno.localdomain>","References":"<20200922100400.30766-1-david.plowman@raspberrypi.com>\n\t<20200922100400.30766-6-david.plowman@raspberrypi.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200922100400.30766-6-david.plowman@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH 5/6] libcamera: pipeline: raspberrypi:\n\tImplementation of digital zoom","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":12675,"web_url":"https://patchwork.libcamera.org/comment/12675/","msgid":"","date":"2020-09-23T13:29:05","subject":"Re: [libcamera-devel] [PATCH 5/6] libcamera: pipeline: raspberrypi:\n\tImplementation of digital zoom","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Jacopo\n\nThanks as always for the detailed review. Let me just go through a\ncouple of those points...!\n\nOn Wed, 23 Sep 2020 at 10:47, Jacopo Mondi wrote:\n>\n> Hi David,\n>\n> On Tue, Sep 22, 2020 at 11:03:59AM +0100, David Plowman wrote:\n> > During configure() we update the SensorOutputSize to the correct value\n> > for this camera mode (which we also note internally) and work out the\n> > minimum crop size allowed by the ISP.\n> >\n> > Whenever a new IspCrop request is received we check it's valid and\n> > apply it to the ISP V4L2 device. We also forward it to the IPA so that\n> > the IPA can return the values used in the image metadata.\n> >\n> > Signed-off-by: David Plowman \n> > ---\n> > include/libcamera/ipa/raspberrypi.h | 1 +\n> > src/ipa/raspberrypi/raspberrypi.cpp | 7 +++\n> > .../pipeline/raspberrypi/raspberrypi.cpp | 47 +++++++++++++++++++\n> > 3 files changed, 55 insertions(+)\n> >\n> > diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h\n> > index dd6ebea..91a1892 100644\n> > --- a/include/libcamera/ipa/raspberrypi.h\n> > +++ b/include/libcamera/ipa/raspberrypi.h\n> > @@ -58,6 +58,7 @@ static const ControlInfoMap RPiControls = {\n> > { &controls::Saturation, ControlInfo(0.0f, 32.0f) },\n> > { &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },\n> > { &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) },\n> > + { &controls::IspCrop, ControlInfo(Rectangle(0, 0, 0, 0), Rectangle(65535, 65535, 65535, 65535), Rectangle(0, 0, 0, 0)) },\n>\n> If you feel it's more convenient, you can\n> s/Rectangle(0, 0, 0, 0)/{}\n>\n> whatever you like the most\n\nYes, thanks, that looks nicer!\n\n>\n> > };\n> >\n> > } /* namespace libcamera */\n> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> > index 0555cc4..64f0e35 100644\n> > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > @@ -687,6 +687,13 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > break;\n> > }\n> >\n> > + case controls::ISP_CROP: {\n> > + /* Just copy the information back. */\n> > + Rectangle crop = ctrl.second.get();\n> > + libcameraMetadata_.set(controls::IspCrop, crop);\n> > + break;\n> > + }\n> > +\n>\n> I wonder if this needs to go to the IPA at all.. I like the symmetry with\n> other controls but the IPA is actually not involved at all in the process...\n> Do you expect this to change ? Otherwise the metadata can be updated here\n> with the value of lastIspCrop_ ?\n\nI remember thinking about this at the time. I couldn't actually think\nof a concrete reason why an IPA would want to know the crop, but I\nalways had the impression that it could possibly happen one day. For\nexample, might AWB ever want to give more importance to parts of the\nimage in the final result? Or might ALSC work less hard on parts of\nthe image outside the final crop? The current answer to all these is\nno, and I have no plans to change anything, but maybe that helps\nconvey why it's not such a strange thing to do.\n\nSo I'm inclined to leave this as it is unless there's still a strong\nfeeling to the contrary (in which case, please say!). Also, as you\npoint out, it's nice to treat everything the same.\n\n>\n> > default:\n> > LOG(IPARPI, Warning)\n> > << \"Ctrl \" << controls::controls.at(ctrl.first)->name()\n> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > index 50f0718..1674f8f 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > @@ -182,6 +182,11 @@ public:\n> > std::queue embeddedQueue_;\n> > std::deque requestQueue_;\n> >\n> > + /* For handling digital zoom. */\n> > + Size ispMinSize_;\n> > + Size sensorOutputSize_;\n> > + Rectangle lastIspCrop_;\n> > +\n> > unsigned int dropFrameCount_;\n> >\n> > private:\n> > @@ -496,6 +501,14 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)\n> > LOG(RPI, Info) << \"Sensor: \" << camera->id()\n> > << \" - Selected mode: \" << sensorFormat.toString();\n> >\n> > + /*\n> > + * Update the SensorOutputSize to the correct value for this camera mode.\n> > + *\n> > + * \\todo Move this property to CameraConfiguration when that\n> > + * feature will be made available and set it at validate() time\n> > + */\n> > + data->properties_.set(properties::SensorOutputSize, sensorFormat.size);\n>\n> The next instruction is to apply 'sensorFormat' to the ISP input node.\n> I don't know if this operation is expected to change the format, but I\n> would however update the property value after that.\n\nIndeed, I don't think it would change but it makes sense to move that\nlittle block to just after.\n\n>\n> > +\n> > /*\n> > * This format may be reset on start() if the bayer order has changed\n> > * because of flips in the sensor.\n> > @@ -592,6 +605,16 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)\n> > return ret;\n> > }\n> >\n> > + /*\n> > + * Figure out the smallest selection the ISP will allow. We also store\n> > + * the output image size, in pixels, from the sensor. These will be\n> > + * used for digital zoom.\n> > + */\n> > + Rectangle testCrop(0, 0, 1, 1);\n> > + data->isp_[Isp::Input].dev()->setSelection(V4L2_SEL_TGT_CROP, &testCrop);\n> > + data->ispMinSize_ = testCrop.size();\n> > + data->sensorOutputSize_ = sensorFormat.size;\n> > +\n> > /* Adjust aspect ratio by providing crops on the input image. */\n> > Rectangle crop{ 0, 0, sensorFormat.size };\n> >\n> > @@ -608,6 +631,8 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)\n> > crop.y = (sensorFormat.size.height - crop.height) >> 1;\n> > data->isp_[Isp::Input].dev()->setSelection(V4L2_SEL_TGT_CROP, &crop);\n> >\n> > + data->lastIspCrop_ = crop;\n> > +\n> > ret = data->configureIPA();\n> > if (ret)\n> > LOG(RPI, Error) << \"Failed to configure the IPA: \" << ret;\n> > @@ -1449,6 +1474,28 @@ void RPiCameraData::tryRunPipeline()\n> > /* Take the first request from the queue and action the IPA. */\n> > Request *request = requestQueue_.front();\n> >\n> > + if (request->controls().contains(controls::IspCrop)) {\n> > + Rectangle crop = request->controls().get(controls::IspCrop);\n> > + if (crop.width && crop.height) {\n> > + /*\n> > + * The crop that we set must be:\n> > + * 1. At least as big as ispMinSize_, once that's been\n> > + * enlarged to the same aspect ratio.\n> > + * 2. With the same mid-point, if possible.\n> > + * 3. But it can't go outside the sensor area.\n> > + */\n> > + Size minSize = ispMinSize_.alignedUpToAspectRatio(crop.size());\n> > + Size size = crop.size().expandedTo(minSize);\n> > + crop = size.centredTo(crop).boundedTo(Rectangle(sensorOutputSize_));\n>\n> I might again be missing something, but isn't this equal to:\n> crop.size().expandTo(minSize).boundTo({sensorOutputSize_});\n>\n> I'm missing the need to centering, 'crop' has already it's own offsets\n> set, so we just need to make sure it's larger that the minSize and\n> smaller than the sensor output size.\n\nSo I think there are some subtle differences.\n\ncrop.size().expandTo(minSize).boundTo({sensorOutputSize_}) has the\nfollowing behaviours (if I've understood it correctly):\n\n* I assumed this code is trying just to alter only the size of the\ncrop Rectangle? Rectangle::size() returns a temporary so I interpreted\nthe code as something like this:\n\nSize minSize = ispMinSize_.alignedUpToAspectRatio(crop.size());\nSize size = crop.size().expandedTo(minSize).boundedTo({sensorOutputSize_});\ncrop = Rectangle(crop.x, crop.y, size);\n\n* Size::boundedTo() doesn't see the crop offsets, so the final crop\nRectangle might go outside sensorOutputSize_.\n\n* changing the size of a Rectangle but not its offsets will cause its\ncentre-point to drift, and with digital zoom I think you'd expect the\ncentre-point of your window to remain the same.\n\nI think if you fix these things up you'd have the same code as me.\n\n>\n> General question: how do we expect applications to reset crop ? By\n> setting it to the sensorOutputSize (or at any larger value). Do we\n> want to predispose a special case, like a {0, 0, 0, 0} crop rectangle\n> resets the crop ?\n\nGood point, and there's a slight philosophical problem in that you\ndon't know what the pipeline handler's default behaviour is. I suppose\none answer is that the application would have to do something like\n\nsensorOutputSize.alignedDownToAspectRatio(streamConfig.size).centredTo({sensorOutputSize})\n\n(love the long names! but note how you do need to know what aspect\nratio you want, as there's no guarantee that the SensorOutputSize is\nnot different)\n\nThis all seems a bit long-winded, but then if an application is\ntouching this stuff it could presumably just set the pan to 0,0 and\nthe zoom to 1 and call its own pan/zoom function.\n\nAt the moment I think I just ignore requests with zero width/height\nbut yes, we could use them to \"reset\" the crop. I don't feel strongly\non the matter. I suppose weird things might happen if a pipeline\nhandler didn't do the \"obvious thing\" by default (i.e. get the right\naspect ratio and centre it), but it doesn't sound like a mainstream\nproblem (and we could always mandate the behaviour if we don't\nalready).\n\nBut anyway, I'm not entirely sure about all of this, so would be\ninterested to hear further discussion.\n\nI'll make all those other non-controversial changes in the meantime. Thanks!\n\nBest regards\nDavid\n\n>\n> > +\n> > + if (crop != lastIspCrop_)\n> > + isp_[Isp::Input].dev()->setSelection(V4L2_SEL_TGT_CROP, &crop);\n> > + lastIspCrop_ = crop;\n> > + }\n> > +\n> > + request->controls().set(controls::IspCrop, lastIspCrop_);\n> > + }\n> > +\n>\n> Very nice, I think as soon we can test this easily with qcam we'll see\n> interesting results!\n>\n> Reviewed-by: Jacopo Mondi \n>\n> Thanks\n> j\n>\n> > /*\n> > * Process all the user controls by the IPA. Once this is complete, we\n> > * queue the ISP output buffer listed in the request to start the HW\n> > --\n> > 2.20.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 48E55C3B5B\n\tfor ;\n\tWed, 23 Sep 2020 13:29:21 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D371962FDE;\n\tWed, 23 Sep 2020 15:29:20 +0200 (CEST)","from mail-oi1-x244.google.com (mail-oi1-x244.google.com\n\t[IPv6:2607:f8b0:4864:20::244])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0157160576\n\tfor ;\n\tWed, 23 Sep 2020 15:29:19 +0200 (CEST)","by mail-oi1-x244.google.com with SMTP id t76so25004534oif.7\n\tfor ;\n\tWed, 23 Sep 2020 06:29:19 -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=\"p/WyYRKC\"; 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=nyf8A8nO4zJ54dkfD0SlvKmgcmUeV22ySPJbGR0ydlA=;\n\tb=p/WyYRKChr7WPQz3r4Z9q7bg4dymqZwhIVMiKtoZRlCzJT0hlJ4KLq431BYFoW0J/n\n\tgnSwRh2QgHuVZ/YJtCClF6avCwRexkzlWzfVpyCTu3Ciaa/dsGP3yvlgG1qM+1QEjsam\n\tVEbQIvky7LDdOSET5z4/bPAIlJHB8inQGuIbINyjIgr77ckyn7HueqEAZh9nqTnWgH9b\n\tsB/evnGipprieLTvPsRB5LONSpTVHHpFbvxRvN1SPRtx9sa4/lh6wEGvXXUW6+eWbOBy\n\tnGG7+0bEldQD/blzEw9MCWejpebhRZBNwwjGfVCQKS/JPRsJdm3rnj8AdrG5XGNxfUSS\n\t6aIw==","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=nyf8A8nO4zJ54dkfD0SlvKmgcmUeV22ySPJbGR0ydlA=;\n\tb=CET/kt1gTgdCuFkog2L2nlSxA1vLqdssygiKBDZlKsREDTodtVnDPr0mDmvoJ7+/wB\n\t8Eu52c3OBVxkXc+j6koDeNjfnc5PDAJTj1iUmhyJeBMoeYdH2i0o90c+b09iHCB+B8FG\n\t/+hzjJiKARCuH165zy6a3p30vsJZj+dEVFawu4aKkdccx2KjM8mPdj8OyQoek2uD9eEC\n\tuqDXAnVY0sfyRyDAYEFDoqypL+pkE2/8SOd5L5dboxTQxubhZM0fcB7nd3uEzGrKvALX\n\tGW8cvffhv3oc83SCfj+UfgDUwwXdd+y2BrFeGdNtHUYYtYUiQvCOA7nosQOw9ajYwSFL\n\tHS0w==","X-Gm-Message-State":"AOAM5314fbG/XG9DyJNqQyMmITwRaBBRFw3RF9E24xeguE6m0qDSFVNE\n\trutw31LkCNuzodUOKX3GgxurJucTKikA3kvkLGd8Rw==","X-Google-Smtp-Source":"ABdhPJzz4gg5gFCr9Bq8XdA6IXzt6CfMzHPJWTQTKeQDFcIyJGGPi0bNkEqb61iNqHdrX43FA/wesB658c+yczhTGeA=","X-Received":"by 2002:aca:5546:: with SMTP id\n\tj67mr5942458oib.107.1600867757740; \n\tWed, 23 Sep 2020 06:29:17 -0700 (PDT)","MIME-Version":"1.0","References":"<20200922100400.30766-1-david.plowman@raspberrypi.com>\n\t<20200922100400.30766-6-david.plowman@raspberrypi.com>\n\t<20200923095134.qlos3gdjs2i5f34p@uno.localdomain>","In-Reply-To":"<20200923095134.qlos3gdjs2i5f34p@uno.localdomain>","From":"David Plowman ","Date":"Wed, 23 Sep 2020 14:29:05 +0100","Message-ID":"","To":"Jacopo Mondi ","Subject":"Re: [libcamera-devel] [PATCH 5/6] libcamera: pipeline: raspberrypi:\n\tImplementation of digital zoom","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":12682,"web_url":"https://patchwork.libcamera.org/comment/12682/","msgid":"<20200923171201.mpxosjgrb2aezuu5@uno.localdomain>","date":"2020-09-23T17:12:01","subject":"Re: [libcamera-devel] [PATCH 5/6] libcamera: pipeline: raspberrypi:\n\tImplementation of digital zoom","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi David,\n\nOn Wed, Sep 23, 2020 at 02:29:05PM +0100, David Plowman wrote:\n> Hi Jacopo\n>\n> Thanks as always for the detailed review. Let me just go through a\n> couple of those points...!\n>\n> On Wed, 23 Sep 2020 at 10:47, Jacopo Mondi wrote:\n> >\n> > Hi David,\n> >\n> > On Tue, Sep 22, 2020 at 11:03:59AM +0100, David Plowman wrote:\n> > > During configure() we update the SensorOutputSize to the correct value\n> > > for this camera mode (which we also note internally) and work out the\n> > > minimum crop size allowed by the ISP.\n> > >\n> > > Whenever a new IspCrop request is received we check it's valid and\n> > > apply it to the ISP V4L2 device. We also forward it to the IPA so that\n> > > the IPA can return the values used in the image metadata.\n> > >\n> > > Signed-off-by: David Plowman \n> > > ---\n> > > include/libcamera/ipa/raspberrypi.h | 1 +\n> > > src/ipa/raspberrypi/raspberrypi.cpp | 7 +++\n> > > .../pipeline/raspberrypi/raspberrypi.cpp | 47 +++++++++++++++++++\n> > > 3 files changed, 55 insertions(+)\n> > >\n> > > diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h\n> > > index dd6ebea..91a1892 100644\n> > > --- a/include/libcamera/ipa/raspberrypi.h\n> > > +++ b/include/libcamera/ipa/raspberrypi.h\n> > > @@ -58,6 +58,7 @@ static const ControlInfoMap RPiControls = {\n> > > { &controls::Saturation, ControlInfo(0.0f, 32.0f) },\n> > > { &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },\n> > > { &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) },\n> > > + { &controls::IspCrop, ControlInfo(Rectangle(0, 0, 0, 0), Rectangle(65535, 65535, 65535, 65535), Rectangle(0, 0, 0, 0)) },\n> >\n> > If you feel it's more convenient, you can\n> > s/Rectangle(0, 0, 0, 0)/{}\n> >\n> > whatever you like the most\n>\n> Yes, thanks, that looks nicer!\n>\n> >\n> > > };\n> > >\n> > > } /* namespace libcamera */\n> > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > index 0555cc4..64f0e35 100644\n> > > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > @@ -687,6 +687,13 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > > break;\n> > > }\n> > >\n> > > + case controls::ISP_CROP: {\n> > > + /* Just copy the information back. */\n> > > + Rectangle crop = ctrl.second.get();\n> > > + libcameraMetadata_.set(controls::IspCrop, crop);\n> > > + break;\n> > > + }\n> > > +\n> >\n> > I wonder if this needs to go to the IPA at all.. I like the symmetry with\n> > other controls but the IPA is actually not involved at all in the process...\n> > Do you expect this to change ? Otherwise the metadata can be updated here\n> > with the value of lastIspCrop_ ?\n>\n> I remember thinking about this at the time. I couldn't actually think\n> of a concrete reason why an IPA would want to know the crop, but I\n> always had the impression that it could possibly happen one day. For\n> example, might AWB ever want to give more importance to parts of the\n> image in the final result? Or might ALSC work less hard on parts of\n> the image outside the final crop? The current answer to all these is\n> no, and I have no plans to change anything, but maybe that helps\n> convey why it's not such a strange thing to do.\n>\n> So I'm inclined to leave this as it is unless there's still a strong\n> feeling to the contrary (in which case, please say!). Also, as you\n> point out, it's nice to treat everything the same.\n>\n\nIt's fine, there's no real overhead in sending it to the IPA in rpi\ncase, so I guess it's fine for now\n\n> >\n> > > default:\n> > > LOG(IPARPI, Warning)\n> > > << \"Ctrl \" << controls::controls.at(ctrl.first)->name()\n> > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > index 50f0718..1674f8f 100644\n> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > @@ -182,6 +182,11 @@ public:\n> > > std::queue embeddedQueue_;\n> > > std::deque requestQueue_;\n> > >\n> > > + /* For handling digital zoom. */\n> > > + Size ispMinSize_;\n> > > + Size sensorOutputSize_;\n> > > + Rectangle lastIspCrop_;\n> > > +\n> > > unsigned int dropFrameCount_;\n> > >\n> > > private:\n> > > @@ -496,6 +501,14 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)\n> > > LOG(RPI, Info) << \"Sensor: \" << camera->id()\n> > > << \" - Selected mode: \" << sensorFormat.toString();\n> > >\n> > > + /*\n> > > + * Update the SensorOutputSize to the correct value for this camera mode.\n> > > + *\n> > > + * \\todo Move this property to CameraConfiguration when that\n> > > + * feature will be made available and set it at validate() time\n> > > + */\n> > > + data->properties_.set(properties::SensorOutputSize, sensorFormat.size);\n> >\n> > The next instruction is to apply 'sensorFormat' to the ISP input node.\n> > I don't know if this operation is expected to change the format, but I\n> > would however update the property value after that.\n>\n> Indeed, I don't think it would change but it makes sense to move that\n> little block to just after.\n>\n> >\n> > > +\n> > > /*\n> > > * This format may be reset on start() if the bayer order has changed\n> > > * because of flips in the sensor.\n> > > @@ -592,6 +605,16 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)\n> > > return ret;\n> > > }\n> > >\n> > > + /*\n> > > + * Figure out the smallest selection the ISP will allow. We also store\n> > > + * the output image size, in pixels, from the sensor. These will be\n> > > + * used for digital zoom.\n> > > + */\n> > > + Rectangle testCrop(0, 0, 1, 1);\n> > > + data->isp_[Isp::Input].dev()->setSelection(V4L2_SEL_TGT_CROP, &testCrop);\n> > > + data->ispMinSize_ = testCrop.size();\n> > > + data->sensorOutputSize_ = sensorFormat.size;\n> > > +\n> > > /* Adjust aspect ratio by providing crops on the input image. */\n> > > Rectangle crop{ 0, 0, sensorFormat.size };\n> > >\n> > > @@ -608,6 +631,8 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)\n> > > crop.y = (sensorFormat.size.height - crop.height) >> 1;\n> > > data->isp_[Isp::Input].dev()->setSelection(V4L2_SEL_TGT_CROP, &crop);\n> > >\n> > > + data->lastIspCrop_ = crop;\n> > > +\n> > > ret = data->configureIPA();\n> > > if (ret)\n> > > LOG(RPI, Error) << \"Failed to configure the IPA: \" << ret;\n> > > @@ -1449,6 +1474,28 @@ void RPiCameraData::tryRunPipeline()\n> > > /* Take the first request from the queue and action the IPA. */\n> > > Request *request = requestQueue_.front();\n> > >\n> > > + if (request->controls().contains(controls::IspCrop)) {\n> > > + Rectangle crop = request->controls().get(controls::IspCrop);\n> > > + if (crop.width && crop.height) {\n> > > + /*\n> > > + * The crop that we set must be:\n> > > + * 1. At least as big as ispMinSize_, once that's been\n> > > + * enlarged to the same aspect ratio.\n> > > + * 2. With the same mid-point, if possible.\n> > > + * 3. But it can't go outside the sensor area.\n> > > + */\n> > > + Size minSize = ispMinSize_.alignedUpToAspectRatio(crop.size());\n> > > + Size size = crop.size().expandedTo(minSize);\n> > > + crop = size.centredTo(crop).boundedTo(Rectangle(sensorOutputSize_));\n> >\n> > I might again be missing something, but isn't this equal to:\n> > crop.size().expandTo(minSize).boundTo({sensorOutputSize_});\n> >\n> > I'm missing the need to centering, 'crop' has already it's own offsets\n> > set, so we just need to make sure it's larger that the minSize and\n> > smaller than the sensor output size.\n>\n> So I think there are some subtle differences.\n>\n> crop.size().expandTo(minSize).boundTo({sensorOutputSize_}) has the\n> following behaviours (if I've understood it correctly):\n>\n> * I assumed this code is trying just to alter only the size of the\n> crop Rectangle? Rectangle::size() returns a temporary so I interpreted\n> the code as something like this:\n>\n> Size minSize = ispMinSize_.alignedUpToAspectRatio(crop.size());\n> Size size = crop.size().expandedTo(minSize).boundedTo({sensorOutputSize_});\n> crop = Rectangle(crop.x, crop.y, size);\n>\n> * Size::boundedTo() doesn't see the crop offsets, so the final crop\n> Rectangle might go outside sensorOutputSize_.\n\nYes, it's a Size, not a Rectangle!\n\n>\n> * changing the size of a Rectangle but not its offsets will cause its\n> centre-point to drift, and with digital zoom I think you'd expect the\n> centre-point of your window to remain the same.\n>\n> I think if you fix these things up you'd have the same code as me.\n\nIndeed! thanks for confirming this!\n\n>\n> >\n> > General question: how do we expect applications to reset crop ? By\n> > setting it to the sensorOutputSize (or at any larger value). Do we\n> > want to predispose a special case, like a {0, 0, 0, 0} crop rectangle\n> > resets the crop ?\n>\n> Good point, and there's a slight philosophical problem in that you\n> don't know what the pipeline handler's default behaviour is. I suppose\n\nWell, we can establish that in the Control definition and pipeline\nwill have to follow...\n\n> one answer is that the application would have to do something like\n>\n> sensorOutputSize.alignedDownToAspectRatio(streamConfig.size).centredTo({sensorOutputSize})\n>\n> (love the long names! but note how you do need to know what aspect\n> ratio you want, as there's no guarantee that the SensorOutputSize is\n> not different)\n>\n> This all seems a bit long-winded, but then if an application is\n> touching this stuff it could presumably just set the pan to 0,0 and\n> the zoom to 1 and call its own pan/zoom function.\n>\n> At the moment I think I just ignore requests with zero width/height\n> but yes, we could use them to \"reset\" the crop. I don't feel strongly\n> on the matter. I suppose weird things might happen if a pipeline\n> handler didn't do the \"obvious thing\" by default (i.e. get the right\n> aspect ratio and centre it), but it doesn't sound like a mainstream\n> problem (and we could always mandate the behaviour if we don't\n> already).\n>\n> But anyway, I'm not entirely sure about all of this, so would be\n> interested to hear further discussion.\n\nI don't have a firm answer to this either.. I'm not concerned of\npipeline handler mis-beahving, as if they do so, it means they're not\ncompliant with out specs and shall be fixed... But as you say, an\napplication which goes to the extent of being able to implement\npan/zoom will probably like to handle it by itself... Anyway, that's\nnot related to this patch.. Maybe it's worth another \\todo entry in\nthe control definition?\n\nThanks\n j\n\n>\n> I'll make all those other non-controversial changes in the meantime. Thanks!\n>\n> Best regards\n> David\n>\n> >\n> > > +\n> > > + if (crop != lastIspCrop_)\n> > > + isp_[Isp::Input].dev()->setSelection(V4L2_SEL_TGT_CROP, &crop);\n> > > + lastIspCrop_ = crop;\n> > > + }\n> > > +\n> > > + request->controls().set(controls::IspCrop, lastIspCrop_);\n> > > + }\n> > > +\n> >\n> > Very nice, I think as soon we can test this easily with qcam we'll see\n> > interesting results!\n> >\n> > Reviewed-by: Jacopo Mondi \n> >\n> > Thanks\n> > j\n> >\n> > > /*\n> > > * Process all the user controls by the IPA. Once this is complete, we\n> > > * queue the ISP output buffer listed in the request to start the HW\n> > > --\n> > > 2.20.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 8855CC3B5C\n\tfor ;\n\tWed, 23 Sep 2020 17:08:11 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E6DD162FDE;\n\tWed, 23 Sep 2020 19:08:10 +0200 (CEST)","from relay4-d.mail.gandi.net (relay4-d.mail.gandi.net\n\t[217.70.183.196])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 952C760576\n\tfor ;\n\tWed, 23 Sep 2020 19:08:09 +0200 (CEST)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay4-d.mail.gandi.net (Postfix) with ESMTPSA id 0CDD5E0006;\n\tWed, 23 Sep 2020 17:08:08 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Wed, 23 Sep 2020 19:12:01 +0200","From":"Jacopo Mondi ","To":"David Plowman ","Message-ID":"<20200923171201.mpxosjgrb2aezuu5@uno.localdomain>","References":"<20200922100400.30766-1-david.plowman@raspberrypi.com>\n\t<20200922100400.30766-6-david.plowman@raspberrypi.com>\n\t<20200923095134.qlos3gdjs2i5f34p@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 5/6] libcamera: pipeline: raspberrypi:\n\tImplementation of digital zoom","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\" "}}]