[{"id":13496,"web_url":"https://patchwork.libcamera.org/comment/13496/","msgid":"<20201026230658.GJ3756@pendragon.ideasonboard.com>","date":"2020-10-26T23:06:58","subject":"Re: [libcamera-devel] [PATCH v6 5/6] 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 Mon, Oct 26, 2020 at 05:19:07PM +0000, 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 <david.plowman@raspberrypi.com>\n> ---\n>  include/libcamera/ipa/raspberrypi.h           |  1 +\n>  src/ipa/raspberrypi/raspberrypi.cpp           |  5 +\n>  .../pipeline/raspberrypi/raspberrypi.cpp      | 92 +++++++++++++++----\n>  3 files changed, 81 insertions(+), 17 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..b9d74a81 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -135,7 +135,7 @@ public:\n>  \tRPiCameraData(PipelineHandler *pipe)\n>  \t\t: CameraData(pipe), sensor_(nullptr), state_(State::Stopped),\n>  \t\t  supportsFlips_(false), flipsAlterBayerOrder_(false),\n> -\t\t  dropFrameCount_(0), ispOutputCount_(0)\n> +\t\t  updateScalerCrop_(true), dropFrameCount_(0), ispOutputCount_(0)\n>  \t{\n>  \t}\n>  \n> @@ -193,6 +193,13 @@ public:\n>  \tbool flipsAlterBayerOrder_;\n>  \tBayerFormat::Order nativeBayerOrder_;\n>  \n> +\t/* For handling digital zoom. */\n> +\tCameraSensorInfo sensorInfo_;\n> +\tRectangle ispCrop_; /* crop in ISP (camera mode) pixels */\n> +\tRectangle scalerCrop_; /* crop in sensor native pixels */\n> +\tbool updateScalerCrop_;\n> +\tSize ispMinCropSize_;\n> +\n>  \tunsigned int dropFrameCount_;\n>  \n>  private:\n> @@ -677,26 +684,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->ispMinCropSize_ = 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(Rectangle(sensorFormat.size).center());\n> +\tdata->ispCrop_ = 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 +1166,8 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)\n>  \t\tipaConfig.data.push_back(static_cast<unsigned int>(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 +1176,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 +1255,26 @@ 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\tif (updateScalerCrop_) {\n> +\t\t\tupdateScalerCrop_ = false;\n> +\t\t\tscalerCrop_ = ispCrop_.scaledBy(sensorInfo_.analogCrop.size(),\n> +\t\t\t\t\t\t\tsensorInfo_.outputSize);\n> +\t\t\tscalerCrop_.translateBy(sensorInfo_.analogCrop.topLeft());\n\nI initially thought this would be done in\nRPiCameraData::tryRunPipeline(), but here is fine too.\n\n> +\t\t}\n> +\t\trequest->metadata().set(controls::ScalerCrop, scalerCrop_);\n> +\n>  \t\tstate_ = State::IpaComplete;\n>  \t\tbreak;\n>  \t}\n> @@ -1595,6 +1625,34 @@ 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<Rectangle>(controls::ScalerCrop);\n> +\n> +\t\tif (crop.width && crop.height) {\n\nThis is the only part that I think needs a fix. If the crop rectangle is\ninvalid, I think it should be set to the closed valid value. How about\n\n\t\tif (!crop.width || !crop.height)\n\t\t\tcrop = { 0, 0, 1, 1 };\n\nand lowering the indentation of the next block of code ? Unless I'm\nmistaken this will avoid a crash and produce a valid crop rectangle. We\ncan also try to make it a bit more fancy to use the same aspect ratio as\nthe analog crop rectangle, but I think that's overkill, if the crop\nrectangle is invalid in the first place, it's the application's fault.\n\nI can fix this when applying if you're fine with the change (it would be\nnice if you could test it though).\n\nWith this addressed,\n\nReviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\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 ispMinCropSize_, 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 = ispMinCropSize_.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 != ispCrop_) {\n> +\t\t\t\tisp_[Isp::Input].dev()->setSelection(V4L2_SEL_TGT_CROP, &crop);\n> +\t\t\t\tispCrop_ = crop;\n> +\t\t\t\t/* queueFrameAction will have to update its scalerCrop_ */\n> +\t\t\t\tupdateScalerCrop_ = true;\n> +\t\t\t}\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":"<libcamera-devel-bounces@lists.libcamera.org>","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 24854C3B5C\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 26 Oct 2020 23:07:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 85FA562064;\n\tTue, 27 Oct 2020 00:07:48 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 99CD162061\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 27 Oct 2020 00:07:46 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C70AC81;\n\tTue, 27 Oct 2020 00:07:45 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"LocemcEX\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1603753666;\n\tbh=UaxHV5sBrpGKdJXR7eEE/taPY0TSbVg2HQzKNFDj7Ic=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=LocemcEX+dbijHP7Bh0krkzPD4ULFSZgW0EhmUGeV4TdhLiKqnC4v2qNWo9OiZ9gp\n\ti9LOP4dH3YHDJAI1XqVGVwmgtwGwT3WwHQjwFzj4KwWFalG3x+XCTWSBCNpitPf01Y\n\tEeZhC+R2ShGSoepZk0yfT3x4wA2TERiEXJCLTtYo=","Date":"Tue, 27 Oct 2020 01:06:58 +0200","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"David Plowman <david.plowman@raspberrypi.com>","Message-ID":"<20201026230658.GJ3756@pendragon.ideasonboard.com>","References":"<20201026171908.21463-1-david.plowman@raspberrypi.com>\n\t<20201026171908.21463-6-david.plowman@raspberrypi.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201026171908.21463-6-david.plowman@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v6 5/6] 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":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","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\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":13511,"web_url":"https://patchwork.libcamera.org/comment/13511/","msgid":"<CAHW6GYL-FLwyivLbELZf_NWvfecNwBC-zZnz6pWw5w0zt20xhQ@mail.gmail.com>","date":"2020-10-27T10:35:52","subject":"Re: [libcamera-devel] [PATCH v6 5/6] 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 Laurent\n\nThanks for looking through this!\n\nOn Mon, 26 Oct 2020 at 23:07, Laurent Pinchart\n<laurent.pinchart@ideasonboard.com> wrote:\n>\n> Hi David,\n>\n> Thank you for the patch.\n>\n> On Mon, Oct 26, 2020 at 05:19:07PM +0000, 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 <david.plowman@raspberrypi.com>\n> > ---\n> >  include/libcamera/ipa/raspberrypi.h           |  1 +\n> >  src/ipa/raspberrypi/raspberrypi.cpp           |  5 +\n> >  .../pipeline/raspberrypi/raspberrypi.cpp      | 92 +++++++++++++++----\n> >  3 files changed, 81 insertions(+), 17 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..b9d74a81 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > @@ -135,7 +135,7 @@ public:\n> >       RPiCameraData(PipelineHandler *pipe)\n> >               : CameraData(pipe), sensor_(nullptr), state_(State::Stopped),\n> >                 supportsFlips_(false), flipsAlterBayerOrder_(false),\n> > -               dropFrameCount_(0), ispOutputCount_(0)\n> > +               updateScalerCrop_(true), dropFrameCount_(0), ispOutputCount_(0)\n> >       {\n> >       }\n> >\n> > @@ -193,6 +193,13 @@ public:\n> >       bool flipsAlterBayerOrder_;\n> >       BayerFormat::Order nativeBayerOrder_;\n> >\n> > +     /* For handling digital zoom. */\n> > +     CameraSensorInfo sensorInfo_;\n> > +     Rectangle ispCrop_; /* crop in ISP (camera mode) pixels */\n> > +     Rectangle scalerCrop_; /* crop in sensor native pixels */\n> > +     bool updateScalerCrop_;\n> > +     Size ispMinCropSize_;\n> > +\n> >       unsigned int dropFrameCount_;\n> >\n> >  private:\n> > @@ -677,26 +684,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->ispMinCropSize_ = 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(Rectangle(sensorFormat.size).center());\n> > +     data->ispCrop_ = 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 +1166,8 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)\n> >               ipaConfig.data.push_back(static_cast<unsigned int>(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 +1176,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 +1255,26 @@ 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> > +             if (updateScalerCrop_) {\n> > +                     updateScalerCrop_ = false;\n> > +                     scalerCrop_ = ispCrop_.scaledBy(sensorInfo_.analogCrop.size(),\n> > +                                                     sensorInfo_.outputSize);\n> > +                     scalerCrop_.translateBy(sensorInfo_.analogCrop.topLeft());\n>\n> I initially thought this would be done in\n> RPiCameraData::tryRunPipeline(), but here is fine too.\n>\n> > +             }\n> > +             request->metadata().set(controls::ScalerCrop, scalerCrop_);\n> > +\n> >               state_ = State::IpaComplete;\n> >               break;\n> >       }\n> > @@ -1595,6 +1625,34 @@ 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<Rectangle>(controls::ScalerCrop);\n> > +\n> > +             if (crop.width && crop.height) {\n>\n> This is the only part that I think needs a fix. If the crop rectangle is\n> invalid, I think it should be set to the closed valid value. How about\n>\n>                 if (!crop.width || !crop.height)\n>                         crop = { 0, 0, 1, 1 };\n>\n> and lowering the indentation of the next block of code ? Unless I'm\n> mistaken this will avoid a crash and produce a valid crop rectangle. We\n> can also try to make it a bit more fancy to use the same aspect ratio as\n> the analog crop rectangle, but I think that's overkill, if the crop\n> rectangle is invalid in the first place, it's the application's fault.\n\nI wasn't quite clear why this would crash as it stands. As far as I\ncan see it would avoid the \"if (crop.width && crop.height)\" block,\nleaving ispCrop_ unchanged (initially it has the default value). But\nplease let me know if I've missed something there.\n\nHowever, this did alert me to a real problem, namely that crop.width\nand crop.height might both be non-zero, but they could go to zero now\nthat we rescale from native to ISP pixels (we didn't do this before,\nof course). This was revealed by that { 0, 0, 1, 1 } rectangle that\nyou used, and really does cause a crash! (It asserts in\nexpandedToAspectRatio - so good idea to put the assert in there!!)\n\nAnyway, I'll fix that up and send a revised version of this single\npatch (if I can figure out how to do that...)\n\nThanks\nDavid\n\n\n\n>\n> I can fix this when applying if you're fine with the change (it would be\n> nice if you could test it though).\n>\n> With this addressed,\n>\n> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\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 ispMinCropSize_, 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 = ispMinCropSize_.expandedToAspectRatio(crop.size());\n> > +                     Size size = crop.size().expandedTo(minSize);\n> > +                     crop = size.centeredTo(crop.center()).enclosedIn(Rectangle(sensorInfo_.outputSize));\n> > +\n> > +                     if (crop != ispCrop_) {\n> > +                             isp_[Isp::Input].dev()->setSelection(V4L2_SEL_TGT_CROP, &crop);\n> > +                             ispCrop_ = crop;\n> > +                             /* queueFrameAction will have to update its scalerCrop_ */\n> > +                             updateScalerCrop_ = true;\n> > +                     }\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","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 2234EC3B5C\n\tfor <parsemail@patchwork.libcamera.org>;\n\tTue, 27 Oct 2020 10:36:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 840C0620AE;\n\tTue, 27 Oct 2020 11:36:07 +0100 (CET)","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 B5F086034C\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 27 Oct 2020 11:36:05 +0100 (CET)","by mail-oi1-x244.google.com with SMTP id u127so776546oib.6\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 27 Oct 2020 03:36:05 -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=\"siYis5fK\"; 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=JrrMwZDaM2aNdBhZdKnLxVF0pH1sRPnRD/xP/KVjUcA=;\n\tb=siYis5fK8ZukYsyB5oyd1/7vcVDD8hNMuO63zoknZ2iqVn69VAGbyM9X9cNxLESRVp\n\tl4RpRyHIbuDRyC4mxZtltZQBK74iErvyjzXLPzuVj2+efwzza4hDz2ivdnoKgP/JUy0H\n\t43exHtfb1mi2IQJrrIe0VhjiGeXykuGmRz+FoS1RYSBhdUnA2l0yNQ86Ea98DUy8SwMJ\n\tqqw5/SC9c6aaeHC5wpx2qr+c8bChnH7Uev/ACfmhhOvmfoJiyvgDM5t2DvwNg5Ax7U5E\n\tuePQlRwyvfKNhVXSEPkOM5m3lDyTmWeXv670l7X9O7Dcs1sq6dq3bHeDhgrVzjDjDf7b\n\tHpiQ==","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=JrrMwZDaM2aNdBhZdKnLxVF0pH1sRPnRD/xP/KVjUcA=;\n\tb=Knj+dlvulKiqZ1KISncwsKDF6DtIg51DqdALETUW83S1i3z4o/Ds7JmN0/hUsrAaij\n\tv60pcgfpUlZHYmu4WzHRMVF9MpflEH3nMawYg3RG5N8c3yB5MIz0mUQ/lufHntCxmv7S\n\tylTzD0IBwrzFpq9lM3kxDO9hDVIccK7H48gLvc3Nl5WJfVVE0yrgYOWtx/mgU0oG2KhH\n\tlrfHDBx1VdX7JOFRRrWXOL+8z15W0uN3XKxql8BWREG4Exj+p1AhDr5iPwnVEIFApGk8\n\t2P9CP3G2eEXaZ/ZBgHW1Q0OnlIpVK1rWrvNO/nER/t2DlkeyeaPOyEI/kYpugsYVhswE\n\t19Zw==","X-Gm-Message-State":"AOAM5325i2F5P0yyfkSRS0PvZQG7Lm1L+RUdGcovDlfq8lUkElY9+VR0\n\tpbOpCwCUmg0+oRbKFWccJ7cqCSZjTFqKaZ8oLSorCg==","X-Google-Smtp-Source":"ABdhPJxRtlCa1Lc31TQZCkQlm7W8TUI+WHocP2OlImJMGBpMcHPYLtcxYaGSZ9iW2g3EnRWYQAVHgf1xxsRp4JPvzB0=","X-Received":"by 2002:a05:6808:696:: with SMTP id\n\tk22mr981731oig.107.1603794964056; \n\tTue, 27 Oct 2020 03:36:04 -0700 (PDT)","MIME-Version":"1.0","References":"<20201026171908.21463-1-david.plowman@raspberrypi.com>\n\t<20201026171908.21463-6-david.plowman@raspberrypi.com>\n\t<20201026230658.GJ3756@pendragon.ideasonboard.com>","In-Reply-To":"<20201026230658.GJ3756@pendragon.ideasonboard.com>","From":"David Plowman <david.plowman@raspberrypi.com>","Date":"Tue, 27 Oct 2020 10:35:52 +0000","Message-ID":"<CAHW6GYL-FLwyivLbELZf_NWvfecNwBC-zZnz6pWw5w0zt20xhQ@mail.gmail.com>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v6 5/6] 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":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera devel <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\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":13512,"web_url":"https://patchwork.libcamera.org/comment/13512/","msgid":"<20201027104126.GC3967@pendragon.ideasonboard.com>","date":"2020-10-27T10:41:26","subject":"Re: [libcamera-devel] [PATCH v6 5/6] 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 Tue, Oct 27, 2020 at 10:35:52AM +0000, David Plowman wrote:\n> Hi Laurent\n> \n> Thanks for looking through this!\n> \n> On Mon, 26 Oct 2020 at 23:07, Laurent Pinchart wrote:\n> > On Mon, Oct 26, 2020 at 05:19:07PM +0000, 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 <david.plowman@raspberrypi.com>\n> > > ---\n> > >  include/libcamera/ipa/raspberrypi.h           |  1 +\n> > >  src/ipa/raspberrypi/raspberrypi.cpp           |  5 +\n> > >  .../pipeline/raspberrypi/raspberrypi.cpp      | 92 +++++++++++++++----\n> > >  3 files changed, 81 insertions(+), 17 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..b9d74a81 100644\n> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > @@ -135,7 +135,7 @@ public:\n> > >       RPiCameraData(PipelineHandler *pipe)\n> > >               : CameraData(pipe), sensor_(nullptr), state_(State::Stopped),\n> > >                 supportsFlips_(false), flipsAlterBayerOrder_(false),\n> > > -               dropFrameCount_(0), ispOutputCount_(0)\n> > > +               updateScalerCrop_(true), dropFrameCount_(0), ispOutputCount_(0)\n> > >       {\n> > >       }\n> > >\n> > > @@ -193,6 +193,13 @@ public:\n> > >       bool flipsAlterBayerOrder_;\n> > >       BayerFormat::Order nativeBayerOrder_;\n> > >\n> > > +     /* For handling digital zoom. */\n> > > +     CameraSensorInfo sensorInfo_;\n> > > +     Rectangle ispCrop_; /* crop in ISP (camera mode) pixels */\n> > > +     Rectangle scalerCrop_; /* crop in sensor native pixels */\n> > > +     bool updateScalerCrop_;\n> > > +     Size ispMinCropSize_;\n> > > +\n> > >       unsigned int dropFrameCount_;\n> > >\n> > >  private:\n> > > @@ -677,26 +684,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->ispMinCropSize_ = 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(Rectangle(sensorFormat.size).center());\n> > > +     data->ispCrop_ = 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 +1166,8 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)\n> > >               ipaConfig.data.push_back(static_cast<unsigned int>(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 +1176,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 +1255,26 @@ 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> > > +             if (updateScalerCrop_) {\n> > > +                     updateScalerCrop_ = false;\n> > > +                     scalerCrop_ = ispCrop_.scaledBy(sensorInfo_.analogCrop.size(),\n> > > +                                                     sensorInfo_.outputSize);\n> > > +                     scalerCrop_.translateBy(sensorInfo_.analogCrop.topLeft());\n> >\n> > I initially thought this would be done in\n> > RPiCameraData::tryRunPipeline(), but here is fine too.\n> >\n> > > +             }\n> > > +             request->metadata().set(controls::ScalerCrop, scalerCrop_);\n> > > +\n> > >               state_ = State::IpaComplete;\n> > >               break;\n> > >       }\n> > > @@ -1595,6 +1625,34 @@ 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<Rectangle>(controls::ScalerCrop);\n> > > +\n> > > +             if (crop.width && crop.height) {\n> >\n> > This is the only part that I think needs a fix. If the crop rectangle is\n> > invalid, I think it should be set to the closed valid value. How about\n> >\n> >                 if (!crop.width || !crop.height)\n> >                         crop = { 0, 0, 1, 1 };\n> >\n> > and lowering the indentation of the next block of code ? Unless I'm\n> > mistaken this will avoid a crash and produce a valid crop rectangle. We\n> > can also try to make it a bit more fancy to use the same aspect ratio as\n> > the analog crop rectangle, but I think that's overkill, if the crop\n> > rectangle is invalid in the first place, it's the application's fault.\n> \n> I wasn't quite clear why this would crash as it stands. As far as I\n> can see it would avoid the \"if (crop.width && crop.height)\" block,\n> leaving ispCrop_ unchanged (initially it has the default value). But\n> please let me know if I've missed something there.\n\nMy proposal is that when the crop rectangle is invalid, it shouldn't be\nignored, but should be replaced by the closest valid value.\n\n> However, this did alert me to a real problem, namely that crop.width\n> and crop.height might both be non-zero, but they could go to zero now\n> that we rescale from native to ISP pixels (we didn't do this before,\n> of course). This was revealed by that { 0, 0, 1, 1 } rectangle that\n> you used, and really does cause a crash! (It asserts in\n> expandedToAspectRatio - so good idea to put the assert in there!!)\n\nGood point !\n\n> Anyway, I'll fix that up and send a revised version of this single\n> patch (if I can figure out how to do that...)\n\nThanks. Please let me know if you need help.\n\n> > I can fix this when applying if you're fine with the change (it would be\n> > nice if you could test it though).\n> >\n> > With this addressed,\n> >\n> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\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 ispMinCropSize_, 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 = ispMinCropSize_.expandedToAspectRatio(crop.size());\n> > > +                     Size size = crop.size().expandedTo(minSize);\n> > > +                     crop = size.centeredTo(crop.center()).enclosedIn(Rectangle(sensorInfo_.outputSize));\n> > > +\n> > > +                     if (crop != ispCrop_) {\n> > > +                             isp_[Isp::Input].dev()->setSelection(V4L2_SEL_TGT_CROP, &crop);\n> > > +                             ispCrop_ = crop;\n> > > +                             /* queueFrameAction will have to update its scalerCrop_ */\n> > > +                             updateScalerCrop_ = true;\n> > > +                     }\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":"<libcamera-devel-bounces@lists.libcamera.org>","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 2713FBDB1E\n\tfor <parsemail@patchwork.libcamera.org>;\n\tTue, 27 Oct 2020 10:42:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 96794620AE;\n\tTue, 27 Oct 2020 11:42:14 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B8AF36034C\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 27 Oct 2020 11:42:13 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 3B56981;\n\tTue, 27 Oct 2020 11:42:13 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Yqo7QxTU\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1603795333;\n\tbh=boViU31/SyZe3EK/R6P/gstcErf5z8ro2BxCzxZiY78=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Yqo7QxTUVx7Y/8A1ocIfyGq3AzeRkZHy/hH7U11Wtm2oRKZmXY8bmBvvukIhFgiAc\n\tMWGFBeDumgEI1CIUe8TH5PNcXtanh9Z6VRYPm6I+uVbDVhZDfPYwLEWYNHyRAY9xw0\n\tBNIQ/gLlXiyIez0AaoKz1rlKOn700gCquTIT1d8I=","Date":"Tue, 27 Oct 2020 12:41:26 +0200","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"David Plowman <david.plowman@raspberrypi.com>","Message-ID":"<20201027104126.GC3967@pendragon.ideasonboard.com>","References":"<20201026171908.21463-1-david.plowman@raspberrypi.com>\n\t<20201026171908.21463-6-david.plowman@raspberrypi.com>\n\t<20201026230658.GJ3756@pendragon.ideasonboard.com>\n\t<CAHW6GYL-FLwyivLbELZf_NWvfecNwBC-zZnz6pWw5w0zt20xhQ@mail.gmail.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<CAHW6GYL-FLwyivLbELZf_NWvfecNwBC-zZnz6pWw5w0zt20xhQ@mail.gmail.com>","Subject":"Re: [libcamera-devel] [PATCH v6 5/6] 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":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera devel <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\" <libcamera-devel-bounces@lists.libcamera.org>"}}]