[{"id":13461,"web_url":"https://patchwork.libcamera.org/comment/13461/","msgid":"<20201023233109.GA13857@pendragon.ideasonboard.com>","date":"2020-10-23T23:31:09","subject":"Re: [libcamera-devel] [PATCH v5 5/5] libcamera: pipeline:\n\traspberrypi: Implementation of digital zoom","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi David,\n\nThank you for the patch.\n\nOn Fri, Oct 23, 2020 at 11:21:59AM +0100, David Plowman wrote:\n> During configure() we update the ScalerCropMaximum to the correct\n> value for this camera mode and work out the minimum crop size allowed\n> by the ISP.\n> \n> Whenever a new ScalerCrop request is received we check it's valid and\n> apply it to the ISP V4L2 device. When the IPA returns its metadata to\n> us we add the ScalerCrop information, rescaled to sensor native\n> pixels.\n> \n> Signed-off-by: David Plowman \n> ---\n> include/libcamera/ipa/raspberrypi.h | 1 +\n> src/ipa/raspberrypi/raspberrypi.cpp | 5 ++\n> .../pipeline/raspberrypi/raspberrypi.cpp | 82 +++++++++++++++----\n> 3 files changed, 72 insertions(+), 16 deletions(-)\n> \n> diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h\n> index b23baf2f..ff2faf86 100644\n> --- a/include/libcamera/ipa/raspberrypi.h\n> +++ b/include/libcamera/ipa/raspberrypi.h\n> @@ -62,6 +62,7 @@ static const ControlInfoMap Controls = {\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::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },\n> };\n> \n> } /* namespace RPi */\n> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> index 1c255aa2..f338ff8b 100644\n> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> @@ -699,6 +699,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> \t\t\tbreak;\n> \t\t}\n> \n> +\t\tcase controls::SCALER_CROP: {\n> +\t\t\t/* We do nothing with this, but should avoid the warning below. */\n> +\t\t\tbreak;\n> +\t\t}\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 763451a8..83e91576 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -193,6 +193,11 @@ public:\n> \tbool flipsAlterBayerOrder_;\n> \tBayerFormat::Order nativeBayerOrder_;\n> \n> +\t/* For handling digital zoom. */\n> +\tCameraSensorInfo sensorInfo_;\n> +\tRectangle lastIspCrop_;\n> +\tSize ispMinSize_;\n\nMaybe ispMinCropSize_ ?\n\n> +\n> \tunsigned int dropFrameCount_;\n> \n> private:\n> @@ -677,26 +682,31 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)\n> \t\treturn ret;\n> \t}\n> \n> -\t/* Adjust aspect ratio by providing crops on the input image. */\n> -\tRectangle crop{ 0, 0, sensorFormat.size };\n> -\n> -\tint ar = maxSize.height * sensorFormat.size.width - maxSize.width * sensorFormat.size.height;\n> -\tif (ar > 0)\n> -\t\tcrop.width = maxSize.width * sensorFormat.size.height / maxSize.height;\n> -\telse if (ar < 0)\n> -\t\tcrop.height = maxSize.height * sensorFormat.size.width / maxSize.width;\n> +\t/* Figure out the smallest selection the ISP will allow. */\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> \n> -\tcrop.width &= ~1;\n> -\tcrop.height &= ~1;\n> +\t/* Adjust aspect ratio by providing crops on the input image. */\n> +\tSize size = sensorFormat.size.boundedToAspectRatio(maxSize);\n> +\tRectangle crop = size.centeredTo(sensorFormat.size.center());\n> +\tdata->lastIspCrop_ = crop;\n> \n> -\tcrop.x = (sensorFormat.size.width - crop.width) >> 1;\n> -\tcrop.y = (sensorFormat.size.height - crop.height) >> 1;\n> \tdata->isp_[Isp::Input].dev()->setSelection(V4L2_SEL_TGT_CROP, &crop);\n> \n> \tret = data->configureIPA(config);\n> \tif (ret)\n> \t\tLOG(RPI, Error) << \"Failed to configure the IPA: \" << ret;\n> \n> +\t/*\n> +\t * Update the ScalerCropMaximum to the correct value for this camera mode.\n> +\t * For us, it's the same as the \"analogue crop\".\n> +\t *\n> +\t * \\todo Make this property the ScalerCrop maximum value when dynamic\n> +\t * controls are available and set it at validate() time\n> +\t */\n> +\tdata->properties_.set(properties::ScalerCropMaximum, data->sensorInfo_.analogCrop);\n> +\n> \treturn ret;\n> }\n> \n> @@ -1154,8 +1164,8 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)\n> \t\tipaConfig.data.push_back(static_cast(lsTable_.fd()));\n> \t}\n> \n> -\tCameraSensorInfo sensorInfo = {};\n> -\tint ret = sensor_->sensorInfo(&sensorInfo);\n> +\t/* We store the CameraSensorInfo for digital zoom calculations. */\n> +\tint ret = sensor_->sensorInfo(&sensorInfo_);\n> \tif (ret) {\n> \t\tLOG(RPI, Error) << \"Failed to retrieve camera sensor info\";\n> \t\treturn ret;\n> @@ -1164,7 +1174,7 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)\n> \t/* Ready the IPA - it must know about the sensor resolution. */\n> \tIPAOperationData result;\n> \n> -\tipa_->configure(sensorInfo, streamConfig, entityControls, ipaConfig,\n> +\tipa_->configure(sensorInfo_, streamConfig, entityControls, ipaConfig,\n> \t\t\t&result);\n> \n> \tunsigned int resultIdx = 0;\n> @@ -1243,8 +1253,23 @@ void RPiCameraData::queueFrameAction([[maybe_unused]] unsigned int frame,\n> \t\tFrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(bufferId);\n> \n> \t\thandleStreamBuffer(buffer, &isp_[Isp::Stats]);\n> +\n> \t\t/* Fill the Request metadata buffer with what the IPA has provided */\n> -\t\trequestQueue_.front()->metadata() = std::move(action.controls[0]);\n> +\t\tRequest *request = requestQueue_.front();\n> +\t\trequest->metadata() = std::move(action.controls[0]);\n> +\n> +\t\t/*\n> +\t\t * Also update the ScalerCrop in the metadata with what we actually\n> +\t\t * used. But we must first rescale that from ISP (camera mode) pixels\n> +\t\t * back into sensor native pixels.\n> +\t\t *\n> +\t\t * Sending this information on every frame may be helpful.\n> +\t\t */\n> +\t\tRectangle crop = lastIspCrop_.scaledBy(sensorInfo_.analogCrop.size(),\n> +\t\t\t\t\t\t sensorInfo_.outputSize);\n> +\t\tcrop.translateBy(sensorInfo_.analogCrop.topLeft());\n\nWould it make sense to store this in lastCrop_, along with lastIspCrop_,\nto avoid recomputing it in every frame ?\n\n> +\t\trequest->metadata().set(controls::ScalerCrop, crop);\n> +\n> \t\tstate_ = State::IpaComplete;\n> \t\tbreak;\n> \t}\n> @@ -1595,6 +1620,31 @@ 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::ScalerCrop)) {\n> +\t\tRectangle crop = request->controls().get(controls::ScalerCrop);\n> +\n> +\t\tif (crop.width && crop.height) {\n\nSomething we can address on top of this series, but I'd like to\nexplicitly document how this case is to be handled, from an API point of\nview. The question goes beyond digital zoom: how should libcamera handle\ninvalid control values ?\n\nIn this specific case, the value can't be considered invalid as\ninclude/libcamera/ipa/raspberrypi.h sets the minimum to Rectangle{}, so\nwidth == 0 || height == 0 is valid from that point of view. Practically\nspeaking that's of course not valid, and I think it would make sense to\nset the minimum to the hardware limit if possible.\n\nThe question still holds, how should we react to invalid control values\n? Should Camera::queueRequest() return an error ? That may be possible\nin some cases, such as checking against the boundaries set by the\npipeline handler in the ControlInfoMap (and we'll possibly have a tiny\nrace condition to handle there if we allow pipeline handlers to update\nthe ControlInfoMap after start()), but not in all cases as the pipeline\nhandler isn't involved in the synchronous part of\nCamera::queueRequest(). We could of course extend the pipeline handler\nAPI with a function to validate controls synchronously.\n\nAnother option is to fail the request asynchronously, reporting an\nerror. A third option is to proceeds with the control being either\nignored, or set to the nearest acceptable value. I'm sure there could be\nother options, such as picking a default value for instance.\n\n> +\t\t\t/* First scale the crop from sensor native to camera mode pixels. */\n> +\t\t\tcrop.translateBy(-sensorInfo_.analogCrop.topLeft());\n> +\t\t\tcrop.scaleBy(sensorInfo_.outputSize, sensorInfo_.analogCrop.size());\n> +\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_.expandedToAspectRatio(crop.size());\n> +\t\t\tSize size = crop.size().expandedTo(minSize);\n> +\t\t\tcrop = size.centeredTo(crop.center()).enclosedIn(Rectangle(sensorInfo_.outputSize));\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> +\t}\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","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 8ECE7C3D3C\n\tfor ;\n\tFri, 23 Oct 2020 23:31:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0DF2A615D2;\n\tSat, 24 Oct 2020 01:31:58 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5104F615D2\n\tfor ;\n\tSat, 24 Oct 2020 01:31:56 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id BBB059BF;\n\tSat, 24 Oct 2020 01:31:55 +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=\"eellf0GA\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1603495915;\n\tbh=3iRSW4zw4t8AwyGpBz9DODWkalSrOhCuzg/t0WZRKYw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=eellf0GAbjVjC6wTq6Zh/3oSyWnw69zYBaRNdpgFP19ZcpEqivcsFVDkvGEts83rS\n\ty+qBLNPu0S79bnl8Dboj8JVlbAndsgVgt3rqQCGLJRpQGIOPP9y4gkDt2OA0/NE46Z\n\t/wzVyMxK314ulJpjX+31tw4ukViyLmiNcXM51KKo=","Date":"Sat, 24 Oct 2020 02:31:09 +0300","From":"Laurent Pinchart ","To":"David Plowman ","Message-ID":"<20201023233109.GA13857@pendragon.ideasonboard.com>","References":"<20201023102159.26274-1-david.plowman@raspberrypi.com>\n\t<20201023102159.26274-6-david.plowman@raspberrypi.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201023102159.26274-6-david.plowman@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v5 5/5] libcamera: pipeline:\n\traspberrypi: Implementation 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":13468,"web_url":"https://patchwork.libcamera.org/comment/13468/","msgid":"<20201024165627.y2pexfy26d24e3qb@uno.localdomain>","date":"2020-10-24T16:56:27","subject":"Re: [libcamera-devel] [PATCH v5 5/5] libcamera: pipeline:\n\traspberrypi: Implementation of digital zoom","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi David, Laurent,\n\nOn Sat, Oct 24, 2020 at 02:31:09AM +0300, Laurent Pinchart wrote:\n> Hi David,\n>\n> Thank you for the patch.\n>\n> On Fri, Oct 23, 2020 at 11:21:59AM +0100, David Plowman wrote:\n> > During configure() we update the ScalerCropMaximum to the correct\n> > value for this camera mode and work out the minimum crop size allowed\n> > by the ISP.\n> >\n> > Whenever a new ScalerCrop request is received we check it's valid and\n> > apply it to the ISP V4L2 device. When the IPA returns its metadata to\n> > us we add the ScalerCrop information, rescaled to sensor native\n> > pixels.\n> >\n> > Signed-off-by: David Plowman \n\nFor the patch\nReviewed-by: Jacopo Mondi \n\nbut to add to the discussion\n\n> > ---\n> > include/libcamera/ipa/raspberrypi.h | 1 +\n> > src/ipa/raspberrypi/raspberrypi.cpp | 5 ++\n> > .../pipeline/raspberrypi/raspberrypi.cpp | 82 +++++++++++++++----\n> > 3 files changed, 72 insertions(+), 16 deletions(-)\n> >\n> > diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h\n> > index b23baf2f..ff2faf86 100644\n> > --- a/include/libcamera/ipa/raspberrypi.h\n> > +++ b/include/libcamera/ipa/raspberrypi.h\n> > @@ -62,6 +62,7 @@ static const ControlInfoMap Controls = {\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::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },\n> > };\n> >\n> > } /* namespace RPi */\n> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> > index 1c255aa2..f338ff8b 100644\n> > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > @@ -699,6 +699,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > \t\t\tbreak;\n> > \t\t}\n> >\n> > +\t\tcase controls::SCALER_CROP: {\n> > +\t\t\t/* We do nothing with this, but should avoid the warning below. */\n> > +\t\t\tbreak;\n> > +\t\t}\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 763451a8..83e91576 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > @@ -193,6 +193,11 @@ public:\n> > \tbool flipsAlterBayerOrder_;\n> > \tBayerFormat::Order nativeBayerOrder_;\n> >\n> > +\t/* For handling digital zoom. */\n> > +\tCameraSensorInfo sensorInfo_;\n> > +\tRectangle lastIspCrop_;\n> > +\tSize ispMinSize_;\n>\n> Maybe ispMinCropSize_ ?\n>\n> > +\n> > \tunsigned int dropFrameCount_;\n> >\n> > private:\n> > @@ -677,26 +682,31 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)\n> > \t\treturn ret;\n> > \t}\n> >\n> > -\t/* Adjust aspect ratio by providing crops on the input image. */\n> > -\tRectangle crop{ 0, 0, sensorFormat.size };\n> > -\n> > -\tint ar = maxSize.height * sensorFormat.size.width - maxSize.width * sensorFormat.size.height;\n> > -\tif (ar > 0)\n> > -\t\tcrop.width = maxSize.width * sensorFormat.size.height / maxSize.height;\n> > -\telse if (ar < 0)\n> > -\t\tcrop.height = maxSize.height * sensorFormat.size.width / maxSize.width;\n> > +\t/* Figure out the smallest selection the ISP will allow. */\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> >\n> > -\tcrop.width &= ~1;\n> > -\tcrop.height &= ~1;\n> > +\t/* Adjust aspect ratio by providing crops on the input image. */\n> > +\tSize size = sensorFormat.size.boundedToAspectRatio(maxSize);\n> > +\tRectangle crop = size.centeredTo(sensorFormat.size.center());\n> > +\tdata->lastIspCrop_ = crop;\n> >\n> > -\tcrop.x = (sensorFormat.size.width - crop.width) >> 1;\n> > -\tcrop.y = (sensorFormat.size.height - crop.height) >> 1;\n> > \tdata->isp_[Isp::Input].dev()->setSelection(V4L2_SEL_TGT_CROP, &crop);\n> >\n> > \tret = data->configureIPA(config);\n> > \tif (ret)\n> > \t\tLOG(RPI, Error) << \"Failed to configure the IPA: \" << ret;\n> >\n> > +\t/*\n> > +\t * Update the ScalerCropMaximum to the correct value for this camera mode.\n> > +\t * For us, it's the same as the \"analogue crop\".\n> > +\t *\n> > +\t * \\todo Make this property the ScalerCrop maximum value when dynamic\n> > +\t * controls are available and set it at validate() time\n> > +\t */\n> > +\tdata->properties_.set(properties::ScalerCropMaximum, data->sensorInfo_.analogCrop);\n> > +\n> > \treturn ret;\n> > }\n> >\n> > @@ -1154,8 +1164,8 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)\n> > \t\tipaConfig.data.push_back(static_cast(lsTable_.fd()));\n> > \t}\n> >\n> > -\tCameraSensorInfo sensorInfo = {};\n> > -\tint ret = sensor_->sensorInfo(&sensorInfo);\n> > +\t/* We store the CameraSensorInfo for digital zoom calculations. */\n> > +\tint ret = sensor_->sensorInfo(&sensorInfo_);\n> > \tif (ret) {\n> > \t\tLOG(RPI, Error) << \"Failed to retrieve camera sensor info\";\n> > \t\treturn ret;\n> > @@ -1164,7 +1174,7 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)\n> > \t/* Ready the IPA - it must know about the sensor resolution. */\n> > \tIPAOperationData result;\n> >\n> > -\tipa_->configure(sensorInfo, streamConfig, entityControls, ipaConfig,\n> > +\tipa_->configure(sensorInfo_, streamConfig, entityControls, ipaConfig,\n> > \t\t\t&result);\n> >\n> > \tunsigned int resultIdx = 0;\n> > @@ -1243,8 +1253,23 @@ void RPiCameraData::queueFrameAction([[maybe_unused]] unsigned int frame,\n> > \t\tFrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(bufferId);\n> >\n> > \t\thandleStreamBuffer(buffer, &isp_[Isp::Stats]);\n> > +\n> > \t\t/* Fill the Request metadata buffer with what the IPA has provided */\n> > -\t\trequestQueue_.front()->metadata() = std::move(action.controls[0]);\n> > +\t\tRequest *request = requestQueue_.front();\n> > +\t\trequest->metadata() = std::move(action.controls[0]);\n> > +\n> > +\t\t/*\n> > +\t\t * Also update the ScalerCrop in the metadata with what we actually\n> > +\t\t * used. But we must first rescale that from ISP (camera mode) pixels\n> > +\t\t * back into sensor native pixels.\n> > +\t\t *\n> > +\t\t * Sending this information on every frame may be helpful.\n> > +\t\t */\n> > +\t\tRectangle crop = lastIspCrop_.scaledBy(sensorInfo_.analogCrop.size(),\n> > +\t\t\t\t\t\t sensorInfo_.outputSize);\n> > +\t\tcrop.translateBy(sensorInfo_.analogCrop.topLeft());\n>\n> Would it make sense to store this in lastCrop_, along with lastIspCrop_,\n> to avoid recomputing it in every frame ?\n>\n> > +\t\trequest->metadata().set(controls::ScalerCrop, crop);\n> > +\n> > \t\tstate_ = State::IpaComplete;\n> > \t\tbreak;\n> > \t}\n> > @@ -1595,6 +1620,31 @@ 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::ScalerCrop)) {\n> > +\t\tRectangle crop = request->controls().get(controls::ScalerCrop);\n> > +\n> > +\t\tif (crop.width && crop.height) {\n>\n> Something we can address on top of this series, but I'd like to\n> explicitly document how this case is to be handled, from an API point of\n> view. The question goes beyond digital zoom: how should libcamera handle\n> invalid control values ?\n>\n> In this specific case, the value can't be considered invalid as\n> include/libcamera/ipa/raspberrypi.h sets the minimum to Rectangle{}, so\n> width == 0 || height == 0 is valid from that point of view. Practically\n> speaking that's of course not valid, and I think it would make sense to\n> set the minimum to the hardware limit if possible.\n>\n> The question still holds, how should we react to invalid control values\n> ? Should Camera::queueRequest() return an error ? That may be possible\n> in some cases, such as checking against the boundaries set by the\n> pipeline handler in the ControlInfoMap (and we'll possibly have a tiny\n> race condition to handle there if we allow pipeline handlers to update\n> the ControlInfoMap after start()), but not in all cases as the pipeline\n> handler isn't involved in the synchronous part of\n> Camera::queueRequest(). We could of course extend the pipeline handler\n> API with a function to validate controls synchronously.\n>\n> Another option is to fail the request asynchronously, reporting an\n> error. A third option is to proceeds with the control being either\n> ignored, or set to the nearest acceptable value. I'm sure there could be\n> other options, such as picking a default value for instance.\n>\n\nHow is digital zoom supposed to be reset ? Should the application\nreset it to the full active pixel array size ? Should a (0, 0)\nRectangle reset the scaler crop otherwise ?\n\nThanks\n j\n\n> > +\t\t\t/* First scale the crop from sensor native to camera mode pixels. */\n> > +\t\t\tcrop.translateBy(-sensorInfo_.analogCrop.topLeft());\n> > +\t\t\tcrop.scaleBy(sensorInfo_.outputSize, sensorInfo_.analogCrop.size());\n> > +\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_.expandedToAspectRatio(crop.size());\n> > +\t\t\tSize size = crop.size().expandedTo(minSize);\n> > +\t\t\tcrop = size.centeredTo(crop.center()).enclosedIn(Rectangle(sensorInfo_.outputSize));\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> > +\t}\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> --\n> Regards,\n>\n> Laurent Pinchart\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 7538CC3B5C\n\tfor ;\n\tSat, 24 Oct 2020 16:56:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1875C61D64;\n\tSat, 24 Oct 2020 18:56:31 +0200 (CEST)","from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net\n\t[217.70.183.194])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C4CD0605BF\n\tfor ;\n\tSat, 24 Oct 2020 18:56:29 +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 relay2-d.mail.gandi.net (Postfix) with ESMTPSA id A2C544000B;\n\tSat, 24 Oct 2020 16:56:28 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Sat, 24 Oct 2020 18:56:27 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20201024165627.y2pexfy26d24e3qb@uno.localdomain>","References":"<20201023102159.26274-1-david.plowman@raspberrypi.com>\n\t<20201023102159.26274-6-david.plowman@raspberrypi.com>\n\t<20201023233109.GA13857@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201023233109.GA13857@pendragon.ideasonboard.com>","X-Spam-Flag":"yes","X-Spam-Level":"**************************","X-GND-Spam-Score":"400","X-GND-Status":"SPAM","Subject":"Re: [libcamera-devel] [PATCH v5 5/5] libcamera: pipeline:\n\traspberrypi: Implementation 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":13471,"web_url":"https://patchwork.libcamera.org/comment/13471/","msgid":"","date":"2020-10-24T21:51:47","subject":"Re: [libcamera-devel] [PATCH v5 5/5] libcamera: pipeline:\n\traspberrypi: Implementation 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 for the comments. Yes, this is an interesting point (see below...)\n\nOn Sat, 24 Oct 2020 at 17:56, Jacopo Mondi wrote:\n>\n> Hi David, Laurent,\n>\n> On Sat, Oct 24, 2020 at 02:31:09AM +0300, Laurent Pinchart wrote:\n> > Hi David,\n> >\n> > Thank you for the patch.\n> >\n> > On Fri, Oct 23, 2020 at 11:21:59AM +0100, David Plowman wrote:\n> > > During configure() we update the ScalerCropMaximum to the correct\n> > > value for this camera mode and work out the minimum crop size allowed\n> > > by the ISP.\n> > >\n> > > Whenever a new ScalerCrop request is received we check it's valid and\n> > > apply it to the ISP V4L2 device. When the IPA returns its metadata to\n> > > us we add the ScalerCrop information, rescaled to sensor native\n> > > pixels.\n> > >\n> > > Signed-off-by: David Plowman \n>\n> For the patch\n> Reviewed-by: Jacopo Mondi \n>\n> but to add to the discussion\n>\n> > > ---\n> > > include/libcamera/ipa/raspberrypi.h | 1 +\n> > > src/ipa/raspberrypi/raspberrypi.cpp | 5 ++\n> > > .../pipeline/raspberrypi/raspberrypi.cpp | 82 +++++++++++++++----\n> > > 3 files changed, 72 insertions(+), 16 deletions(-)\n> > >\n> > > diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h\n> > > index b23baf2f..ff2faf86 100644\n> > > --- a/include/libcamera/ipa/raspberrypi.h\n> > > +++ b/include/libcamera/ipa/raspberrypi.h\n> > > @@ -62,6 +62,7 @@ static const ControlInfoMap Controls = {\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::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },\n> > > };\n> > >\n> > > } /* namespace RPi */\n> > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > index 1c255aa2..f338ff8b 100644\n> > > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > @@ -699,6 +699,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > > break;\n> > > }\n> > >\n> > > + case controls::SCALER_CROP: {\n> > > + /* We do nothing with this, but should avoid the warning below. */\n> > > + break;\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 763451a8..83e91576 100644\n> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > @@ -193,6 +193,11 @@ public:\n> > > bool flipsAlterBayerOrder_;\n> > > BayerFormat::Order nativeBayerOrder_;\n> > >\n> > > + /* For handling digital zoom. */\n> > > + CameraSensorInfo sensorInfo_;\n> > > + Rectangle lastIspCrop_;\n> > > + Size ispMinSize_;\n> >\n> > Maybe ispMinCropSize_ ?\n> >\n> > > +\n> > > unsigned int dropFrameCount_;\n> > >\n> > > private:\n> > > @@ -677,26 +682,31 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)\n> > > return ret;\n> > > }\n> > >\n> > > - /* Adjust aspect ratio by providing crops on the input image. */\n> > > - Rectangle crop{ 0, 0, sensorFormat.size };\n> > > -\n> > > - int ar = maxSize.height * sensorFormat.size.width - maxSize.width * sensorFormat.size.height;\n> > > - if (ar > 0)\n> > > - crop.width = maxSize.width * sensorFormat.size.height / maxSize.height;\n> > > - else if (ar < 0)\n> > > - crop.height = maxSize.height * sensorFormat.size.width / maxSize.width;\n> > > + /* Figure out the smallest selection the ISP will allow. */\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> > >\n> > > - crop.width &= ~1;\n> > > - crop.height &= ~1;\n> > > + /* Adjust aspect ratio by providing crops on the input image. */\n> > > + Size size = sensorFormat.size.boundedToAspectRatio(maxSize);\n> > > + Rectangle crop = size.centeredTo(sensorFormat.size.center());\n> > > + data->lastIspCrop_ = crop;\n> > >\n> > > - crop.x = (sensorFormat.size.width - crop.width) >> 1;\n> > > - crop.y = (sensorFormat.size.height - crop.height) >> 1;\n> > > data->isp_[Isp::Input].dev()->setSelection(V4L2_SEL_TGT_CROP, &crop);\n> > >\n> > > ret = data->configureIPA(config);\n> > > if (ret)\n> > > LOG(RPI, Error) << \"Failed to configure the IPA: \" << ret;\n> > >\n> > > + /*\n> > > + * Update the ScalerCropMaximum to the correct value for this camera mode.\n> > > + * For us, it's the same as the \"analogue crop\".\n> > > + *\n> > > + * \\todo Make this property the ScalerCrop maximum value when dynamic\n> > > + * controls are available and set it at validate() time\n> > > + */\n> > > + data->properties_.set(properties::ScalerCropMaximum, data->sensorInfo_.analogCrop);\n> > > +\n> > > return ret;\n> > > }\n> > >\n> > > @@ -1154,8 +1164,8 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)\n> > > ipaConfig.data.push_back(static_cast(lsTable_.fd()));\n> > > }\n> > >\n> > > - CameraSensorInfo sensorInfo = {};\n> > > - int ret = sensor_->sensorInfo(&sensorInfo);\n> > > + /* We store the CameraSensorInfo for digital zoom calculations. */\n> > > + int ret = sensor_->sensorInfo(&sensorInfo_);\n> > > if (ret) {\n> > > LOG(RPI, Error) << \"Failed to retrieve camera sensor info\";\n> > > return ret;\n> > > @@ -1164,7 +1174,7 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)\n> > > /* Ready the IPA - it must know about the sensor resolution. */\n> > > IPAOperationData result;\n> > >\n> > > - ipa_->configure(sensorInfo, streamConfig, entityControls, ipaConfig,\n> > > + ipa_->configure(sensorInfo_, streamConfig, entityControls, ipaConfig,\n> > > &result);\n> > >\n> > > unsigned int resultIdx = 0;\n> > > @@ -1243,8 +1253,23 @@ void RPiCameraData::queueFrameAction([[maybe_unused]] unsigned int frame,\n> > > FrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(bufferId);\n> > >\n> > > handleStreamBuffer(buffer, &isp_[Isp::Stats]);\n> > > +\n> > > /* Fill the Request metadata buffer with what the IPA has provided */\n> > > - requestQueue_.front()->metadata() = std::move(action.controls[0]);\n> > > + Request *request = requestQueue_.front();\n> > > + request->metadata() = std::move(action.controls[0]);\n> > > +\n> > > + /*\n> > > + * Also update the ScalerCrop in the metadata with what we actually\n> > > + * used. But we must first rescale that from ISP (camera mode) pixels\n> > > + * back into sensor native pixels.\n> > > + *\n> > > + * Sending this information on every frame may be helpful.\n> > > + */\n> > > + Rectangle crop = lastIspCrop_.scaledBy(sensorInfo_.analogCrop.size(),\n> > > + sensorInfo_.outputSize);\n> > > + crop.translateBy(sensorInfo_.analogCrop.topLeft());\n> >\n> > Would it make sense to store this in lastCrop_, along with lastIspCrop_,\n> > to avoid recomputing it in every frame ?\n> >\n> > > + request->metadata().set(controls::ScalerCrop, crop);\n> > > +\n> > > state_ = State::IpaComplete;\n> > > break;\n> > > }\n> > > @@ -1595,6 +1620,31 @@ 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::ScalerCrop)) {\n> > > + Rectangle crop = request->controls().get(controls::ScalerCrop);\n> > > +\n> > > + if (crop.width && crop.height) {\n> >\n> > Something we can address on top of this series, but I'd like to\n> > explicitly document how this case is to be handled, from an API point of\n> > view. The question goes beyond digital zoom: how should libcamera handle\n> > invalid control values ?\n> >\n> > In this specific case, the value can't be considered invalid as\n> > include/libcamera/ipa/raspberrypi.h sets the minimum to Rectangle{}, so\n> > width == 0 || height == 0 is valid from that point of view. Practically\n> > speaking that's of course not valid, and I think it would make sense to\n> > set the minimum to the hardware limit if possible.\n> >\n> > The question still holds, how should we react to invalid control values\n> > ? Should Camera::queueRequest() return an error ? That may be possible\n> > in some cases, such as checking against the boundaries set by the\n> > pipeline handler in the ControlInfoMap (and we'll possibly have a tiny\n> > race condition to handle there if we allow pipeline handlers to update\n> > the ControlInfoMap after start()), but not in all cases as the pipeline\n> > handler isn't involved in the synchronous part of\n> > Camera::queueRequest(). We could of course extend the pipeline handler\n> > API with a function to validate controls synchronously.\n> >\n> > Another option is to fail the request asynchronously, reporting an\n> > error. A third option is to proceeds with the control being either\n> > ignored, or set to the nearest acceptable value. I'm sure there could be\n> > other options, such as picking a default value for instance.\n> >\n>\n> How is digital zoom supposed to be reset ? Should the application\n> reset it to the full active pixel array size ? Should a (0, 0)\n> Rectangle reset the scaler crop otherwise ?\n>\n\nSetting the ScalerCrop to the ScalerCropMaximum might seem the\n\"obvious\" thing to do, but in general it wouldn't have the right\naspect ratio. So we could go with a rectangle full of zeroes (or at\nleast width and height zero) to mean \"the default, please\".\n\nOf course, we haven't defined what the default is. You might expect to\ncrop from the middle of the ScalerCropMaximum, but we don't mandate\nthat. Nor is it clear how you would find out. (Although in the Pi\nversion I always return the ScalerCrop - it seems to me like a useful\nthing. But I expect that behaviour is not guaranteed for other\npipelines, even if they support the ScalerCrop.)\n\nHowever, I suspect that applications that want to take control of the\nzoom will probably do so completely. They'll implement their own zoom\ncode and may as well implement their own default by setting their zoom\nfactor to 1, regardless of any pipeline default. So I think it may be\nless important in practice, even if it does seem like a bit of an\nomission.\n\nNevertheless, having all-zeroes be a request for the default seems\nreasonable enough to me. If there are no objections I could add that\ninto the next version of the patches.\n\nThanks!\nDavid\n\n> Thanks\n> j\n>\n> > > + /* First scale the crop from sensor native to camera mode pixels. */\n> > > + crop.translateBy(-sensorInfo_.analogCrop.topLeft());\n> > > + crop.scaleBy(sensorInfo_.outputSize, sensorInfo_.analogCrop.size());\n> > > +\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_.expandedToAspectRatio(crop.size());\n> > > + Size size = crop.size().expandedTo(minSize);\n> > > + crop = size.centeredTo(crop.center()).enclosedIn(Rectangle(sensorInfo_.outputSize));\n> > > +\n> > > + if (crop != lastIspCrop_)\n> > > + isp_[Isp::Input].dev()->setSelection(V4L2_SEL_TGT_CROP, &crop);\n> > > + lastIspCrop_ = crop;\n> > > + }\n> > > + }\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> > --\n> > Regards,\n> >\n> > Laurent Pinchart\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 8840DC3B5C\n\tfor ;\n\tSat, 24 Oct 2020 21:52:02 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E219961DD3;\n\tSat, 24 Oct 2020 23:52:01 +0200 (CEST)","from mail-oo1-xc34.google.com (mail-oo1-xc34.google.com\n\t[IPv6:2607:f8b0:4864:20::c34])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 76785605BF\n\tfor ;\n\tSat, 24 Oct 2020 23:52:00 +0200 (CEST)","by mail-oo1-xc34.google.com with SMTP id c10so1467860oon.6\n\tfor ;\n\tSat, 24 Oct 2020 14:52:00 -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=\"poiNNDqO\"; 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=naOy2YZL1oUWUP2y4bxnXXMLw0lUDL+MU+NTL/U/Nug=;\n\tb=poiNNDqOIDwD4uNW3kwIcJ8/tSnME12q0MczBbUoIyWqAJiJijL0dGBHZjGjeo95e+\n\t3rkg2rBV8nH5tY0Vkp+S+aZmp148spQ8admHJHAHYkP/CoDbNd4jUgmYtTOxl/YxXO9m\n\tkxbUxk59OQfw8jz1sTP6Z0imjt94Mj9OlNdBC4Ukkqbx9iNg+97aPWl4H9o+d2YQwd6q\n\tGU1N3EXzuslyFgrRJMBnXgLROfQ5wnAbHeeNRYwrvL+5QRvZDUQ32FSmV5zmzr1LznXb\n\tGp+OcnzInVN7NOV840f8iX1Vqx+sw71mJSC+09XHhLfE/TqJPg+v+34iQwymELdmmqRO\n\tHj1Q==","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=naOy2YZL1oUWUP2y4bxnXXMLw0lUDL+MU+NTL/U/Nug=;\n\tb=cb2JaihSz4KyhulPwUvTa2fU7+TGPMtH3je+kc4sw6MiWrE+D34vSnEMS3bbVIaaU8\n\t4dHbNMSWzOCQTTwFTonURcyx2jUGOxu9UXJ8ZTML1oF2q93x9UZlWcOl1JZCskqPswgq\n\tqDrYHSbztSULSdEYRRjn2NGiKoXcods4MsAQqJGCTK8brQ+gThUWrrmnSS026rCrfAi2\n\tmICdBIcKXN/ZC76KEoPuhlx9I6SwaZvD85fIc9mYrB6/9XVjWeWFB7/NOjGzYgAf/OnJ\n\tPNWKnGl+dOM/26Kvt72UGBW1rX0B6itondIXzF2Kx91KyZxRioZ7MJH0su7RB3HNWG5K\n\tFy2Q==","X-Gm-Message-State":"AOAM531h1p7mOMdBy9Y3W4yBWEfoAgseGXuhn4Nly5qPo9h655dZstAI\n\tZF+/RVQ9gZbihIRGiHS58Pdxn/5g7kpv6vA9uCzOHw==","X-Google-Smtp-Source":"ABdhPJykPuzJ49weW1dGWXsK45/UbfY17JRaOWVP7Pba/bwVpcqafohS/aVsvW+CyudbCztLg1H1OQdwuqZznHtYp1c=","X-Received":"by 2002:a4a:83d7:: with SMTP id r23mr2675912oog.5.1603576318671; \n\tSat, 24 Oct 2020 14:51:58 -0700 (PDT)","MIME-Version":"1.0","References":"<20201023102159.26274-1-david.plowman@raspberrypi.com>\n\t<20201023102159.26274-6-david.plowman@raspberrypi.com>\n\t<20201023233109.GA13857@pendragon.ideasonboard.com>\n\t<20201024165627.y2pexfy26d24e3qb@uno.localdomain>","In-Reply-To":"<20201024165627.y2pexfy26d24e3qb@uno.localdomain>","From":"David Plowman ","Date":"Sat, 24 Oct 2020 22:51:47 +0100","Message-ID":"","To":"Jacopo Mondi ","Subject":"Re: [libcamera-devel] [PATCH v5 5/5] libcamera: pipeline:\n\traspberrypi: Implementation 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 ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":13473,"web_url":"https://patchwork.libcamera.org/comment/13473/","msgid":"<20201024223143.GF3943@pendragon.ideasonboard.com>","date":"2020-10-24T22:31:43","subject":"Re: [libcamera-devel] [PATCH v5 5/5] libcamera: pipeline:\n\traspberrypi: Implementation of digital zoom","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi David,\n\nOn Sat, Oct 24, 2020 at 10:51:47PM +0100, David Plowman wrote:\n> Hi Jacopo\n> \n> Thanks for the comments. Yes, this is an interesting point (see below...)\n> \n> On Sat, 24 Oct 2020 at 17:56, Jacopo Mondi wrote:\n> > On Sat, Oct 24, 2020 at 02:31:09AM +0300, Laurent Pinchart wrote:\n> > > On Fri, Oct 23, 2020 at 11:21:59AM +0100, David Plowman wrote:\n> > > > During configure() we update the ScalerCropMaximum to the correct\n> > > > value for this camera mode and work out the minimum crop size allowed\n> > > > by the ISP.\n> > > >\n> > > > Whenever a new ScalerCrop request is received we check it's valid and\n> > > > apply it to the ISP V4L2 device. When the IPA returns its metadata to\n> > > > us we add the ScalerCrop information, rescaled to sensor native\n> > > > pixels.\n> > > >\n> > > > Signed-off-by: David Plowman \n> >\n> > For the patch\n> > Reviewed-by: Jacopo Mondi \n> >\n> > but to add to the discussion\n> >\n> > > > ---\n> > > > include/libcamera/ipa/raspberrypi.h | 1 +\n> > > > src/ipa/raspberrypi/raspberrypi.cpp | 5 ++\n> > > > .../pipeline/raspberrypi/raspberrypi.cpp | 82 +++++++++++++++----\n> > > > 3 files changed, 72 insertions(+), 16 deletions(-)\n> > > >\n> > > > diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h\n> > > > index b23baf2f..ff2faf86 100644\n> > > > --- a/include/libcamera/ipa/raspberrypi.h\n> > > > +++ b/include/libcamera/ipa/raspberrypi.h\n> > > > @@ -62,6 +62,7 @@ static const ControlInfoMap Controls = {\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::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },\n> > > > };\n> > > >\n> > > > } /* namespace RPi */\n> > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > > index 1c255aa2..f338ff8b 100644\n> > > > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > > @@ -699,6 +699,11 @@ void IPARPi::queueRequest(const ControlList &controls)\n> > > > break;\n> > > > }\n> > > >\n> > > > + case controls::SCALER_CROP: {\n> > > > + /* We do nothing with this, but should avoid the warning below. */\n> > > > + break;\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 763451a8..83e91576 100644\n> > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > @@ -193,6 +193,11 @@ public:\n> > > > bool flipsAlterBayerOrder_;\n> > > > BayerFormat::Order nativeBayerOrder_;\n> > > >\n> > > > + /* For handling digital zoom. */\n> > > > + CameraSensorInfo sensorInfo_;\n> > > > + Rectangle lastIspCrop_;\n> > > > + Size ispMinSize_;\n> > >\n> > > Maybe ispMinCropSize_ ?\n> > >\n> > > > +\n> > > > unsigned int dropFrameCount_;\n> > > >\n> > > > private:\n> > > > @@ -677,26 +682,31 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)\n> > > > return ret;\n> > > > }\n> > > >\n> > > > - /* Adjust aspect ratio by providing crops on the input image. */\n> > > > - Rectangle crop{ 0, 0, sensorFormat.size };\n> > > > -\n> > > > - int ar = maxSize.height * sensorFormat.size.width - maxSize.width * sensorFormat.size.height;\n> > > > - if (ar > 0)\n> > > > - crop.width = maxSize.width * sensorFormat.size.height / maxSize.height;\n> > > > - else if (ar < 0)\n> > > > - crop.height = maxSize.height * sensorFormat.size.width / maxSize.width;\n> > > > + /* Figure out the smallest selection the ISP will allow. */\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> > > >\n> > > > - crop.width &= ~1;\n> > > > - crop.height &= ~1;\n> > > > + /* Adjust aspect ratio by providing crops on the input image. */\n> > > > + Size size = sensorFormat.size.boundedToAspectRatio(maxSize);\n> > > > + Rectangle crop = size.centeredTo(sensorFormat.size.center());\n> > > > + data->lastIspCrop_ = crop;\n> > > >\n> > > > - crop.x = (sensorFormat.size.width - crop.width) >> 1;\n> > > > - crop.y = (sensorFormat.size.height - crop.height) >> 1;\n> > > > data->isp_[Isp::Input].dev()->setSelection(V4L2_SEL_TGT_CROP, &crop);\n> > > >\n> > > > ret = data->configureIPA(config);\n> > > > if (ret)\n> > > > LOG(RPI, Error) << \"Failed to configure the IPA: \" << ret;\n> > > >\n> > > > + /*\n> > > > + * Update the ScalerCropMaximum to the correct value for this camera mode.\n> > > > + * For us, it's the same as the \"analogue crop\".\n> > > > + *\n> > > > + * \\todo Make this property the ScalerCrop maximum value when dynamic\n> > > > + * controls are available and set it at validate() time\n> > > > + */\n> > > > + data->properties_.set(properties::ScalerCropMaximum, data->sensorInfo_.analogCrop);\n> > > > +\n> > > > return ret;\n> > > > }\n> > > >\n> > > > @@ -1154,8 +1164,8 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)\n> > > > ipaConfig.data.push_back(static_cast(lsTable_.fd()));\n> > > > }\n> > > >\n> > > > - CameraSensorInfo sensorInfo = {};\n> > > > - int ret = sensor_->sensorInfo(&sensorInfo);\n> > > > + /* We store the CameraSensorInfo for digital zoom calculations. */\n> > > > + int ret = sensor_->sensorInfo(&sensorInfo_);\n> > > > if (ret) {\n> > > > LOG(RPI, Error) << \"Failed to retrieve camera sensor info\";\n> > > > return ret;\n> > > > @@ -1164,7 +1174,7 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)\n> > > > /* Ready the IPA - it must know about the sensor resolution. */\n> > > > IPAOperationData result;\n> > > >\n> > > > - ipa_->configure(sensorInfo, streamConfig, entityControls, ipaConfig,\n> > > > + ipa_->configure(sensorInfo_, streamConfig, entityControls, ipaConfig,\n> > > > &result);\n> > > >\n> > > > unsigned int resultIdx = 0;\n> > > > @@ -1243,8 +1253,23 @@ void RPiCameraData::queueFrameAction([[maybe_unused]] unsigned int frame,\n> > > > FrameBuffer *buffer = isp_[Isp::Stats].getBuffers().at(bufferId);\n> > > >\n> > > > handleStreamBuffer(buffer, &isp_[Isp::Stats]);\n> > > > +\n> > > > /* Fill the Request metadata buffer with what the IPA has provided */\n> > > > - requestQueue_.front()->metadata() = std::move(action.controls[0]);\n> > > > + Request *request = requestQueue_.front();\n> > > > + request->metadata() = std::move(action.controls[0]);\n> > > > +\n> > > > + /*\n> > > > + * Also update the ScalerCrop in the metadata with what we actually\n> > > > + * used. But we must first rescale that from ISP (camera mode) pixels\n> > > > + * back into sensor native pixels.\n> > > > + *\n> > > > + * Sending this information on every frame may be helpful.\n> > > > + */\n> > > > + Rectangle crop = lastIspCrop_.scaledBy(sensorInfo_.analogCrop.size(),\n> > > > + sensorInfo_.outputSize);\n> > > > + crop.translateBy(sensorInfo_.analogCrop.topLeft());\n> > >\n> > > Would it make sense to store this in lastCrop_, along with lastIspCrop_,\n> > > to avoid recomputing it in every frame ?\n> > >\n> > > > + request->metadata().set(controls::ScalerCrop, crop);\n> > > > +\n> > > > state_ = State::IpaComplete;\n> > > > break;\n> > > > }\n> > > > @@ -1595,6 +1620,31 @@ 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::ScalerCrop)) {\n> > > > + Rectangle crop = request->controls().get(controls::ScalerCrop);\n> > > > +\n> > > > + if (crop.width && crop.height) {\n> > >\n> > > Something we can address on top of this series, but I'd like to\n> > > explicitly document how this case is to be handled, from an API point of\n> > > view. The question goes beyond digital zoom: how should libcamera handle\n> > > invalid control values ?\n> > >\n> > > In this specific case, the value can't be considered invalid as\n> > > include/libcamera/ipa/raspberrypi.h sets the minimum to Rectangle{}, so\n> > > width == 0 || height == 0 is valid from that point of view. Practically\n> > > speaking that's of course not valid, and I think it would make sense to\n> > > set the minimum to the hardware limit if possible.\n> > >\n> > > The question still holds, how should we react to invalid control values\n> > > ? Should Camera::queueRequest() return an error ? That may be possible\n> > > in some cases, such as checking against the boundaries set by the\n> > > pipeline handler in the ControlInfoMap (and we'll possibly have a tiny\n> > > race condition to handle there if we allow pipeline handlers to update\n> > > the ControlInfoMap after start()), but not in all cases as the pipeline\n> > > handler isn't involved in the synchronous part of\n> > > Camera::queueRequest(). We could of course extend the pipeline handler\n> > > API with a function to validate controls synchronously.\n> > >\n> > > Another option is to fail the request asynchronously, reporting an\n> > > error. A third option is to proceeds with the control being either\n> > > ignored, or set to the nearest acceptable value. I'm sure there could be\n> > > other options, such as picking a default value for instance.\n> >\n> > How is digital zoom supposed to be reset ? Should the application\n> > reset it to the full active pixel array size ? Should a (0, 0)\n> > Rectangle reset the scaler crop otherwise ?\n> \n> Setting the ScalerCrop to the ScalerCropMaximum might seem the\n> \"obvious\" thing to do, but in general it wouldn't have the right\n> aspect ratio. So we could go with a rectangle full of zeroes (or at\n> least width and height zero) to mean \"the default, please\".\n> \n> Of course, we haven't defined what the default is. You might expect to\n> crop from the middle of the ScalerCropMaximum, but we don't mandate\n> that. Nor is it clear how you would find out. (Although in the Pi\n> version I always return the ScalerCrop - it seems to me like a useful\n> thing. But I expect that behaviour is not guaranteed for other\n> pipelines, even if they support the ScalerCrop.)\n\nI agree it's a useful thing, and I think we should mandate ScalerCrop to\nbe returned in metadata for all pipelines that support it.\n\n> However, I suspect that applications that want to take control of the\n> zoom will probably do so completely. They'll implement their own zoom\n> code and may as well implement their own default by setting their zoom\n> factor to 1, regardless of any pipeline default. So I think it may be\n> less important in practice, even if it does seem like a bit of an\n> omission.\n> \n> Nevertheless, having all-zeroes be a request for the default seems\n> reasonable enough to me. If there are no objections I could add that\n> into the next version of the patches.\n\nThe same way we plan to replace ScalerCropMaximum with a dynamic\nControlInfo, I think the default should also be reported through\nControlInfo once it will be made dynamic. Applications can then just\ngrab the default from there, and use it to reset the scaler crop\nrectangle. I would thus prefer avoiding addition of a special case with\nal all 0s rectangle to perform the same operation.\n\n> > > > + /* First scale the crop from sensor native to camera mode pixels. */\n> > > > + crop.translateBy(-sensorInfo_.analogCrop.topLeft());\n> > > > + crop.scaleBy(sensorInfo_.outputSize, sensorInfo_.analogCrop.size());\n> > > > +\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_.expandedToAspectRatio(crop.size());\n> > > > + Size size = crop.size().expandedTo(minSize);\n> > > > + crop = size.centeredTo(crop.center()).enclosedIn(Rectangle(sensorInfo_.outputSize));\n> > > > +\n> > > > + if (crop != lastIspCrop_)\n> > > > + isp_[Isp::Input].dev()->setSelection(V4L2_SEL_TGT_CROP, &crop);\n> > > > + lastIspCrop_ = crop;\n> > > > + }\n> > > > + }\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","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 5A4D2C3B5C\n\tfor ;\n\tSat, 24 Oct 2020 22:32:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CD1BA61DD3;\n\tSun, 25 Oct 2020 00:32:31 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6BFF061CB4\n\tfor ;\n\tSun, 25 Oct 2020 00:32:30 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E07C3A19;\n\tSun, 25 Oct 2020 00:32:29 +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=\"TZeesUi4\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1603578750;\n\tbh=PUbF4mXwQm6JL4/sSVSZqes1W4+/lH3L/4f3j6jeUqw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=TZeesUi4Xvl1Fq97WYV9cprRzQhtBt3Xdyquy/tXSyyVI5/trHznuqrBitJPGVevy\n\tC4AQQJ2A+YD+YA5iIfRtq3Z3jCmF9axGUr8HyI7xh/rYv7fzf1O9SERXkVGYoM6ykT\n\txAXiF1PWXjQQCImQD/E0V711zkZ7K8g2jRSZ8Ox8=","Date":"Sun, 25 Oct 2020 01:31:43 +0300","From":"Laurent Pinchart ","To":"David Plowman ","Message-ID":"<20201024223143.GF3943@pendragon.ideasonboard.com>","References":"<20201023102159.26274-1-david.plowman@raspberrypi.com>\n\t<20201023102159.26274-6-david.plowman@raspberrypi.com>\n\t<20201023233109.GA13857@pendragon.ideasonboard.com>\n\t<20201024165627.y2pexfy26d24e3qb@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v5 5/5] libcamera: pipeline:\n\traspberrypi: Implementation 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 ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]