[{"id":30944,"web_url":"https://patchwork.libcamera.org/comment/30944/","msgid":"","date":"2024-08-28T10:07:46","subject":"Re: [PATCH v1 7/7] pipeline: rpi: Handler controls::rpi::ScalerCrops","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Naush\n\nOn Thu, Aug 08, 2024 at 11:23:46AM GMT, Naushir Patuck wrote:\n> Handle multiple scaler crops being set through the rpi::ScalerCrops\n> control. We now populate the cropParams_ map in the loop where we handle\n> the output stream configuration items. The key of this map is the index\n> of the stream configuration structure set by the application. This will\n> also be the same index used to specify the crop rectangles through the\n> ScalerCrops control.\n>\n> CameraData::applyScalerCrop() has been adapted to look at either\n> controls::ScalerCrop or controls::rpi::ScalerCrops. The former takes\n> priority over the latter, and if present, will apply the same scaler\n> crop to all output streams.\n>\n> Finally return all crops through the same ScalerCrops control via\n> request metadata. The first configure stream's crop rectangle is also\n> returned via the ScalerCrop control in the request metadata.\n>\n> Signed-off-by: Naushir Patuck \n> ---\n> .../pipeline/rpi/common/pipeline_base.cpp | 69 +++++++++++++++----\n> 1 file changed, 56 insertions(+), 13 deletions(-)\n>\n> diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> index a6ea4e9c47dd..b9759b682f0d 100644\n> --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> @@ -561,11 +561,28 @@ int PipelineHandlerBase::configure(Camera *camera, CameraConfiguration *config)\n> \tfor (auto const &c : result.controlInfo)\n> \t\tctrlMap.emplace(c.first, c.second);\n>\n> -\tif (data->cropParams_.count(0)) {\n> -\t\t/* Add the ScalerCrop control limits based on the current mode. */\n> +\tif (data->cropParams_.size()) {\n\nThis comment applies to the previous patch, but is there a case where\ndata->cropParams_ is not populated at all ?\n\n> +\t\t/*\n> +\t\t * Add the ScalerCrop control limits based on the current mode and\n> +\t\t * the first configured stream.\n> +\t\t */\n> \t\tRectangle ispMinCrop = data->scaleIspCrop(Rectangle(data->cropParams_[0].ispMinCropSize));\n> \t\tctrlMap[&controls::ScalerCrop] = ControlInfo(ispMinCrop, data->sensorInfo_.analogCrop,\n> \t\t\t\t\t\t\t data->scaleIspCrop(data->cropParams_[0].ispCrop));\n\nempty line maybe ?\n\n> +\t\tif (data->cropParams_.size() == 2) {\n> +\t\t\t/*\n> +\t\t\t * The control map for rpi::ScalerCrops has the min value\n> +\t\t\t * as the default crop for stream 0, max value as the default\n> +\t\t\t * value for stream 1.\n\nmmm, I guess this is some internal agreement between pipeline_base and\npipeline_pisp that initializes cropParams in this way, right ?\n\n\n> +\t\t\t */\n> +\t\t\tctrlMap[&controls::rpi::ScalerCrops] =\n> +\t\t\t\tControlInfo(data->scaleIspCrop(data->cropParams_[0].ispCrop),\n> +\t\t\t\t\t data->scaleIspCrop(data->cropParams_[1].ispCrop),\n> +\t\t\t\t\t ctrlMap[&controls::ScalerCrop].def());\n> +\t\t} else {\n> +\t\t\t/* Match the same ControlInfo for rpi::ScalerCrops. */\n> +\t\t\tctrlMap[&controls::rpi::ScalerCrops] = ctrlMap[&controls::ScalerCrop];\n> +\t\t}\n> \t}\n>\n> \tdata->controlInfo_ = ControlInfoMap(std::move(ctrlMap), result.controlInfo.idmap());\n> @@ -1292,10 +1309,29 @@ Rectangle CameraData::scaleIspCrop(const Rectangle &ispCrop) const\n>\n> void CameraData::applyScalerCrop(const ControlList &controls)\n> {\n> -\tconst auto &scalerCrop = controls.get(controls::ScalerCrop);\n> -\tif (scalerCrop && cropParams_.count(0)) {\n> -\t\tCropParams &cropParams = cropParams_[0];\n> -\t\tRectangle nativeCrop = *scalerCrop;\n> +\tconst auto &scalerCropRPi = controls.get>(controls::rpi::ScalerCrops);\n> +\tconst auto &scalerCropCore = controls.get(controls::ScalerCrop);\n\ncan both be specified in the same requests ? Does the usage of\nScalerCrops make ScalerCrop invalid ?\n\nIt feels to me like pips should only register ScalerCrops and vc4 only\nScalerCrop. Yes, the application has to adapt to this in this case,\nnot sure how bad that would be\n\n> +\tstd::vector scalerCrops;\n> +\n> +\t/*\n> +\t * First thing to do is create a vector of crops to apply to each ISP output\n> +\t * based on either controls::ScalerCrop or controls::rpi::ScalerCrops if\n> +\t * present.\n> +\t *\n> +\t * If controls::ScalerCrop is present, apply the same crop to all ISP output\n> +\t * streams. Otherwise if controls::rpi::ScalerCrops, apply the given crops\n> +\t * to the ISP output streams, indexed by the same order in which they had\n> +\t * been configured. This is not the same as the ISP output index.\n> +\t */\n> +\tfor (unsigned int i = 0; i < cropParams_.size(); i++) {\n> +\t\tif (scalerCropRPi && i < scalerCropRPi->size())\n> +\t\t\tscalerCrops.push_back(scalerCropRPi->data()[i]);\n> +\t\telse if (scalerCropCore)\n> +\t\t\tscalerCrops.push_back(*scalerCropCore);\n> +\t}\n> +\n> +\tfor (auto const &[i, scalerCrop] : utils::enumerate(scalerCrops)) {\n> +\t\tRectangle nativeCrop = scalerCrop;\n>\n> \t\tif (!nativeCrop.width || !nativeCrop.height)\n> \t\t\tnativeCrop = { 0, 0, 1, 1 };\n> @@ -1311,13 +1347,13 @@ void CameraData::applyScalerCrop(const ControlList &controls)\n> \t\t * 2. With the same mid-point, if possible.\n> \t\t * 3. But it can't go outside the sensor area.\n> \t\t */\n> -\t\tSize minSize = cropParams.ispMinCropSize.expandedToAspectRatio(nativeCrop.size());\n> +\t\tSize minSize = cropParams_[i].ispMinCropSize.expandedToAspectRatio(nativeCrop.size());\n> \t\tSize size = ispCrop.size().expandedTo(minSize);\n> \t\tispCrop = size.centeredTo(ispCrop.center()).enclosedIn(Rectangle(sensorInfo_.outputSize));\n>\n> -\t\tif (ispCrop != cropParams.ispCrop) {\n> -\t\t\tcropParams.ispCrop = ispCrop;\n> -\t\t\tplatformSetIspCrop(cropParams.ispIndex, ispCrop);\n> +\t\tif (ispCrop != cropParams_[i].ispCrop) {\n> +\t\t\tcropParams_[i].ispCrop = ispCrop;\n> +\t\t\tplatformSetIspCrop(cropParams_[i].ispIndex, ispCrop);\n\nYou can really just pass cropsParams_[i] to platformSetIspCrop. It\ncontains the index and the updated ispCrop.\n\n> \t\t}\n> \t}\n> }\n> @@ -1474,9 +1510,16 @@ void CameraData::fillRequestMetadata(const ControlList &bufferControls, Request\n> \trequest->metadata().set(controls::SensorTimestamp,\n> \t\t\t\tbufferControls.get(controls::SensorTimestamp).value_or(0));\n>\n> -\tif (cropParams_.count(0))\n> -\t\trequest->metadata().set(controls::ScalerCrop,\n> -\t\t\t\t\tscaleIspCrop(cropParams_[0].ispCrop));\n> +\tif (cropParams_.size()) {\n\nAgain I might have missed in which case cropParams_ is not populated\nat all\n\n> +\t\tstd::vector crops;\n> +\n> +\t\tfor (auto const &[k, v] : cropParams_)\n> +\t\t\tcrops.push_back(scaleIspCrop(v.ispCrop));\n> +\n> +\t\trequest->metadata().set(controls::ScalerCrop, crops[0]);\n> +\t\trequest->metadata().set(controls::rpi::ScalerCrops,\n> +\t\t\t\t\tSpan(crops.data(), crops.size()));\n> +\t}\n> }\n>\n> } /* namespace libcamera */\n> --\n> 2.34.1\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 9EEEFC323E\n\tfor ;\n\tWed, 28 Aug 2024 10:07:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 68D75633CD;\n\tWed, 28 Aug 2024 12:07:52 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4214E61903\n\tfor ;\n\tWed, 28 Aug 2024 12:07:50 +0200 (CEST)","from ideasonboard.com (mob-5-90-141-165.net.vodafone.it\n\t[5.90.141.165])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 67672220;\n\tWed, 28 Aug 2024 12:06:42 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"HS+Uu5OE\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1724839602;\n\tbh=bDP5/U/Eb1D0IjMRClb+MHUly/x620rUSMtQLkMJK1A=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=HS+Uu5OEThVXA4XXBgjPIfWpUL/zlciiZWOMqw9jX6nUiA5fujDS2H2oLCYvmC4b/\n\tQdWpRuvG5awbQKHTHnwnsub21G1t59+/nm+foXSE1wMlzKbaKfbs4RtVlT/9WtZr8R\n\taTuybXpQgK/+Mjig1sLXPr0E/LtZkoZK48W+qgCk=","Date":"Wed, 28 Aug 2024 12:07:46 +0200","From":"Jacopo Mondi ","To":"Naushir Patuck ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v1 7/7] pipeline: rpi: Handler controls::rpi::ScalerCrops","Message-ID":"","References":"<20240808102346.13065-1-naush@raspberrypi.com>\n\t<20240808102346.13065-8-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240808102346.13065-8-naush@raspberrypi.com>","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":30945,"web_url":"https://patchwork.libcamera.org/comment/30945/","msgid":"","date":"2024-08-28T11:10:18","subject":"Re: [PATCH v1 7/7] pipeline: rpi: Handler controls::rpi::ScalerCrops","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Jacopo,\n\nThanks for the review.\n\nOn Wed, 28 Aug 2024 at 11:07, Jacopo Mondi\n wrote:\n>\n> Hi Naush\n>\n> On Thu, Aug 08, 2024 at 11:23:46AM GMT, Naushir Patuck wrote:\n> > Handle multiple scaler crops being set through the rpi::ScalerCrops\n> > control. We now populate the cropParams_ map in the loop where we handle\n> > the output stream configuration items. The key of this map is the index\n> > of the stream configuration structure set by the application. This will\n> > also be the same index used to specify the crop rectangles through the\n> > ScalerCrops control.\n> >\n> > CameraData::applyScalerCrop() has been adapted to look at either\n> > controls::ScalerCrop or controls::rpi::ScalerCrops. The former takes\n> > priority over the latter, and if present, will apply the same scaler\n> > crop to all output streams.\n> >\n> > Finally return all crops through the same ScalerCrops control via\n> > request metadata. The first configure stream's crop rectangle is also\n> > returned via the ScalerCrop control in the request metadata.\n> >\n> > Signed-off-by: Naushir Patuck \n> > ---\n> > .../pipeline/rpi/common/pipeline_base.cpp | 69 +++++++++++++++----\n> > 1 file changed, 56 insertions(+), 13 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> > index a6ea4e9c47dd..b9759b682f0d 100644\n> > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> > @@ -561,11 +561,28 @@ int PipelineHandlerBase::configure(Camera *camera, CameraConfiguration *config)\n> > for (auto const &c : result.controlInfo)\n> > ctrlMap.emplace(c.first, c.second);\n> >\n> > - if (data->cropParams_.count(0)) {\n> > - /* Add the ScalerCrop control limits based on the current mode. */\n> > + if (data->cropParams_.size()) {\n>\n> This comment applies to the previous patch, but is there a case where\n> data->cropParams_ is not populated at all ?\n\nBoth vc4 and pisp will populate this. I was being a bit too cautious\nhere perhaps. I'll remove the check.\n\n>\n> > + /*\n> > + * Add the ScalerCrop control limits based on the current mode and\n> > + * the first configured stream.\n> > + */\n> > Rectangle ispMinCrop = data->scaleIspCrop(Rectangle(data->cropParams_[0].ispMinCropSize));\n> > ctrlMap[&controls::ScalerCrop] = ControlInfo(ispMinCrop, data->sensorInfo_.analogCrop,\n> > data->scaleIspCrop(data->cropParams_[0].ispCrop));\n>\n> empty line maybe ?\n\nack\n\n>\n> > + if (data->cropParams_.size() == 2) {\n> > + /*\n> > + * The control map for rpi::ScalerCrops has the min value\n> > + * as the default crop for stream 0, max value as the default\n> > + * value for stream 1.\n>\n> mmm, I guess this is some internal agreement between pipeline_base and\n> pipeline_pisp that initializes cropParams in this way, right ?\n>\n>\n> > + */\n> > + ctrlMap[&controls::rpi::ScalerCrops] =\n> > + ControlInfo(data->scaleIspCrop(data->cropParams_[0].ispCrop),\n> > + data->scaleIspCrop(data->cropParams_[1].ispCrop),\n> > + ctrlMap[&controls::ScalerCrop].def());\n> > + } else {\n> > + /* Match the same ControlInfo for rpi::ScalerCrops. */\n> > + ctrlMap[&controls::rpi::ScalerCrops] = ctrlMap[&controls::ScalerCrop];\n> > + }\n> > }\n> >\n> > data->controlInfo_ = ControlInfoMap(std::move(ctrlMap), result.controlInfo.idmap());\n> > @@ -1292,10 +1309,29 @@ Rectangle CameraData::scaleIspCrop(const Rectangle &ispCrop) const\n> >\n> > void CameraData::applyScalerCrop(const ControlList &controls)\n> > {\n> > - const auto &scalerCrop = controls.get(controls::ScalerCrop);\n> > - if (scalerCrop && cropParams_.count(0)) {\n> > - CropParams &cropParams = cropParams_[0];\n> > - Rectangle nativeCrop = *scalerCrop;\n> > + const auto &scalerCropRPi = controls.get>(controls::rpi::ScalerCrops);\n> > + const auto &scalerCropCore = controls.get(controls::ScalerCrop);\n>\n> can both be specified in the same requests ? Does the usage of\n> ScalerCrops make ScalerCrop invalid ?\n\nIf they are both specified in the same request,\ncontrols::rpi::ScalerCrops will be used instead of\ncontrols::ScalerCrop.\n\n>\n> It feels to me like pips should only register ScalerCrops and vc4 only\n> ScalerCrop. Yes, the application has to adapt to this in this case,\n> not sure how bad that would be\n\nI think that would be workable.\n\n>\n> > + std::vector scalerCrops;\n> > +\n> > + /*\n> > + * First thing to do is create a vector of crops to apply to each ISP output\n> > + * based on either controls::ScalerCrop or controls::rpi::ScalerCrops if\n> > + * present.\n> > + *\n> > + * If controls::ScalerCrop is present, apply the same crop to all ISP output\n> > + * streams. Otherwise if controls::rpi::ScalerCrops, apply the given crops\n> > + * to the ISP output streams, indexed by the same order in which they had\n> > + * been configured. This is not the same as the ISP output index.\n> > + */\n> > + for (unsigned int i = 0; i < cropParams_.size(); i++) {\n> > + if (scalerCropRPi && i < scalerCropRPi->size())\n> > + scalerCrops.push_back(scalerCropRPi->data()[i]);\n> > + else if (scalerCropCore)\n> > + scalerCrops.push_back(*scalerCropCore);\n> > + }\n> > +\n> > + for (auto const &[i, scalerCrop] : utils::enumerate(scalerCrops)) {\n> > + Rectangle nativeCrop = scalerCrop;\n> >\n> > if (!nativeCrop.width || !nativeCrop.height)\n> > nativeCrop = { 0, 0, 1, 1 };\n> > @@ -1311,13 +1347,13 @@ void CameraData::applyScalerCrop(const ControlList &controls)\n> > * 2. With the same mid-point, if possible.\n> > * 3. But it can't go outside the sensor area.\n> > */\n> > - Size minSize = cropParams.ispMinCropSize.expandedToAspectRatio(nativeCrop.size());\n> > + Size minSize = cropParams_[i].ispMinCropSize.expandedToAspectRatio(nativeCrop.size());\n> > Size size = ispCrop.size().expandedTo(minSize);\n> > ispCrop = size.centeredTo(ispCrop.center()).enclosedIn(Rectangle(sensorInfo_.outputSize));\n> >\n> > - if (ispCrop != cropParams.ispCrop) {\n> > - cropParams.ispCrop = ispCrop;\n> > - platformSetIspCrop(cropParams.ispIndex, ispCrop);\n> > + if (ispCrop != cropParams_[i].ispCrop) {\n> > + cropParams_[i].ispCrop = ispCrop;\n> > + platformSetIspCrop(cropParams_[i].ispIndex, ispCrop);\n>\n> You can really just pass cropsParams_[i] to platformSetIspCrop. It\n> contains the index and the updated ispCrop.\n\nAck.\n\n>\n> > }\n> > }\n> > }\n> > @@ -1474,9 +1510,16 @@ void CameraData::fillRequestMetadata(const ControlList &bufferControls, Request\n> > request->metadata().set(controls::SensorTimestamp,\n> > bufferControls.get(controls::SensorTimestamp).value_or(0));\n> >\n> > - if (cropParams_.count(0))\n> > - request->metadata().set(controls::ScalerCrop,\n> > - scaleIspCrop(cropParams_[0].ispCrop));\n> > + if (cropParams_.size()) {\n>\n> Again I might have missed in which case cropParams_ is not populated\n> at all\n\nAs above, I can remove the check.\n\nRegards,\nNaush\n\n>\n> > + std::vector crops;\n> > +\n> > + for (auto const &[k, v] : cropParams_)\n> > + crops.push_back(scaleIspCrop(v.ispCrop));\n> > +\n> > + request->metadata().set(controls::ScalerCrop, crops[0]);\n> > + request->metadata().set(controls::rpi::ScalerCrops,\n> > + Span(crops.data(), crops.size()));\n> > + }\n> > }\n> >\n> > } /* namespace libcamera */\n> > --\n> > 2.34.1\n> >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 9E702C324C\n\tfor ;\n\tWed, 28 Aug 2024 11:10:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2020A633CD;\n\tWed, 28 Aug 2024 13:10:58 +0200 (CEST)","from mail-yb1-xb2d.google.com (mail-yb1-xb2d.google.com\n\t[IPv6:2607:f8b0:4864:20::b2d])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 06DBC61903\n\tfor ;\n\tWed, 28 Aug 2024 13:10:55 +0200 (CEST)","by mail-yb1-xb2d.google.com with SMTP id\n\t3f1490d57ef6-dfdcda2bdbcso211630276.1\n\tfor ;\n\tWed, 28 Aug 2024 04:10:55 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"sn4LnEal\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1724843455; x=1725448255;\n\tdarn=lists.libcamera.org; \n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=nchd3Z5alzZSOWbPjv3t1ZDke/YSogt64pILwEyufWk=;\n\tb=sn4LnEalul8E+fHYXDKhHHjvAP+KTT8zMG1DIrivHNWcmUeZn3H1EH/gwl6kwxSWXe\n\tZyZ6wdlI63PrQvKB7pIuHFtzvUG2hZ/vFt8ZBmXwxzBfntVpYMWhVBrJBzlQBVd8yfDw\n\tNnA3f4RfRzjmZe8NE5c7STI7E9Ro1L9bKhDrvCllKVU583TnfNPnBVWuE8m7kC9VM3yu\n\tuhzVKWvUwl8v1dplJj8EeFKb+X0g0RT6hRZibsakBriKIV8cc7LqTuFxCO8KhdpCqpPO\n\tTm6uoZbm1Y9gNthVeoCCuAZKud6mcFc/LqobH+XdH7wBn29YpMCol3Imkvu5yck/Mdyw\n\tHnIw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1724843455; x=1725448255;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=nchd3Z5alzZSOWbPjv3t1ZDke/YSogt64pILwEyufWk=;\n\tb=CqGdNt0BU2zAPUNFNhZnHnvzcH3yIHKYpAQ+RyKGSIRfhP9AdkWV1kTDAwqqyXGBTj\n\tTzntSavGFAfLQHq5pU3sgUyWH0mwbVe6YVId945f9A4Od0Ta7U8IMutxvh6X3JivfRKE\n\t9BYm4AX40E2//SXq5KFfhG1ht0uXZJOrtZYqLuMcrx74A5xqEsyf9/F/S8KyaJc7/QMX\n\tRx4MbNam7LFI1xKu/j2x5dy8ffpvf+jg7eX7FtmcuVRJJXtwQ36xaejyAJRALpJhqjes\n\tY6/Eixl0ly6g7zbKn1mgJwTn27m6D6ex1n9vHCL1t1lCdr0SQHw1RN/O1rc1Xvti3Yl+\n\tpgYQ==","X-Gm-Message-State":"AOJu0YxJLALcUXNPrP5L/I7b5E0sKz1db8sURFyuPWp+Zjinbimg/CxT\n\tJxRK7fg2k4zPNhPJF9p+dWf0s/uOzrJDIUBby5Tqg3zsoJZKhTX0VRRQWoKeQ2I8bdkecD34Bj1\n\tNUjhU7G5HgT/r5aBaAgM1Pq5DaW3V4jwdR08/4k9cF4bpb6CwZbw=","X-Google-Smtp-Source":"AGHT+IGLXJHyr2uSYcraWpB6mXKwC3qrSKANxrAj1cXHOBMMOG/Q1F/7PTlyKDpHx/rqv3m2GEVc0Wgy/wxYVuNc5lY=","X-Received":"by 2002:a05:690c:95:b0:6b4:b3bf:c34 with SMTP id\n\t00721157ae682-6c6243209d4mr101155817b3.1.1724843454638;\n\tWed, 28 Aug 2024 04:10:54 -0700 (PDT)","MIME-Version":"1.0","References":"<20240808102346.13065-1-naush@raspberrypi.com>\n\t<20240808102346.13065-8-naush@raspberrypi.com>\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Wed, 28 Aug 2024 12:10:18 +0100","Message-ID":"","Subject":"Re: [PATCH v1 7/7] pipeline: rpi: Handler controls::rpi::ScalerCrops","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"UTF-8\"","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":30953,"web_url":"https://patchwork.libcamera.org/comment/30953/","msgid":"<20240828170154.GA26812@pendragon.ideasonboard.com>","date":"2024-08-28T17:01:54","subject":"Re: [PATCH v1 7/7] pipeline: rpi: Handler controls::rpi::ScalerCrops","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello,\n\nOn Wed, Aug 28, 2024 at 12:10:18PM +0100, Naushir Patuck wrote:\n> On Wed, 28 Aug 2024 at 11:07, Jacopo Mondi wrote:\n> > On Thu, Aug 08, 2024 at 11:23:46AM GMT, Naushir Patuck wrote:\n> > > Handle multiple scaler crops being set through the rpi::ScalerCrops\n> > > control. We now populate the cropParams_ map in the loop where we handle\n> > > the output stream configuration items. The key of this map is the index\n> > > of the stream configuration structure set by the application. This will\n> > > also be the same index used to specify the crop rectangles through the\n> > > ScalerCrops control.\n> > >\n> > > CameraData::applyScalerCrop() has been adapted to look at either\n> > > controls::ScalerCrop or controls::rpi::ScalerCrops. The former takes\n> > > priority over the latter, and if present, will apply the same scaler\n> > > crop to all output streams.\n\nThis should be documented in the controls::rpi::ScalerCrops definition.\n\n> > >\n> > > Finally return all crops through the same ScalerCrops control via\n> > > request metadata. The first configure stream's crop rectangle is also\n> > > returned via the ScalerCrop control in the request metadata.\n> > >\n> > > Signed-off-by: Naushir Patuck \n> > > ---\n> > > .../pipeline/rpi/common/pipeline_base.cpp | 69 +++++++++++++++----\n> > > 1 file changed, 56 insertions(+), 13 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> > > index a6ea4e9c47dd..b9759b682f0d 100644\n> > > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> > > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> > > @@ -561,11 +561,28 @@ int PipelineHandlerBase::configure(Camera *camera, CameraConfiguration *config)\n> > > for (auto const &c : result.controlInfo)\n> > > ctrlMap.emplace(c.first, c.second);\n> > >\n> > > - if (data->cropParams_.count(0)) {\n> > > - /* Add the ScalerCrop control limits based on the current mode. */\n> > > + if (data->cropParams_.size()) {\n> >\n> > This comment applies to the previous patch, but is there a case where\n> > data->cropParams_ is not populated at all ?\n> \n> Both vc4 and pisp will populate this. I was being a bit too cautious\n> here perhaps. I'll remove the check.\n> \n> > > + /*\n> > > + * Add the ScalerCrop control limits based on the current mode and\n> > > + * the first configured stream.\n> > > + */\n> > > Rectangle ispMinCrop = data->scaleIspCrop(Rectangle(data->cropParams_[0].ispMinCropSize));\n> > > ctrlMap[&controls::ScalerCrop] = ControlInfo(ispMinCrop, data->sensorInfo_.analogCrop,\n> > > data->scaleIspCrop(data->cropParams_[0].ispCrop));\n> >\n> > empty line maybe ?\n> \n> ack\n> \n> > > + if (data->cropParams_.size() == 2) {\n> > > + /*\n> > > + * The control map for rpi::ScalerCrops has the min value\n> > > + * as the default crop for stream 0, max value as the default\n> > > + * value for stream 1.\n> >\n> > mmm, I guess this is some internal agreement between pipeline_base and\n> > pipeline_pisp that initializes cropParams in this way, right ?\n\nCould this be handled in a bit cleaner way ?\n\n> > > + */\n> > > + ctrlMap[&controls::rpi::ScalerCrops] =\n> > > + ControlInfo(data->scaleIspCrop(data->cropParams_[0].ispCrop),\n> > > + data->scaleIspCrop(data->cropParams_[1].ispCrop),\n> > > + ctrlMap[&controls::ScalerCrop].def());\n> > > + } else {\n> > > + /* Match the same ControlInfo for rpi::ScalerCrops. */\n> > > + ctrlMap[&controls::rpi::ScalerCrops] = ctrlMap[&controls::ScalerCrop];\n> > > + }\n> > > }\n> > >\n> > > data->controlInfo_ = ControlInfoMap(std::move(ctrlMap), result.controlInfo.idmap());\n> > > @@ -1292,10 +1309,29 @@ Rectangle CameraData::scaleIspCrop(const Rectangle &ispCrop) const\n> > >\n> > > void CameraData::applyScalerCrop(const ControlList &controls)\n> > > {\n> > > - const auto &scalerCrop = controls.get(controls::ScalerCrop);\n> > > - if (scalerCrop && cropParams_.count(0)) {\n> > > - CropParams &cropParams = cropParams_[0];\n> > > - Rectangle nativeCrop = *scalerCrop;\n> > > + const auto &scalerCropRPi = controls.get>(controls::rpi::ScalerCrops);\n> > > + const auto &scalerCropCore = controls.get(controls::ScalerCrop);\n> >\n> > can both be specified in the same requests ? Does the usage of\n> > ScalerCrops make ScalerCrop invalid ?\n> \n> If they are both specified in the same request,\n> controls::rpi::ScalerCrops will be used instead of\n> controls::ScalerCrop.\n\nThis contradicts the commit message which specifies the opposite order.\n\n> > It feels to me like pips should only register ScalerCrops and vc4 only\n> > ScalerCrop. Yes, the application has to adapt to this in this case,\n> > not sure how bad that would be\n> \n> I think that would be workable.\n> \n> > > + std::vector scalerCrops;\n> > > +\n> > > + /*\n> > > + * First thing to do is create a vector of crops to apply to each ISP output\n> > > + * based on either controls::ScalerCrop or controls::rpi::ScalerCrops if\n> > > + * present.\n> > > + *\n> > > + * If controls::ScalerCrop is present, apply the same crop to all ISP output\n> > > + * streams. Otherwise if controls::rpi::ScalerCrops, apply the given crops\n> > > + * to the ISP output streams, indexed by the same order in which they had\n> > > + * been configured. This is not the same as the ISP output index.\n> > > + */\n> > > + for (unsigned int i = 0; i < cropParams_.size(); i++) {\n> > > + if (scalerCropRPi && i < scalerCropRPi->size())\n> > > + scalerCrops.push_back(scalerCropRPi->data()[i]);\n> > > + else if (scalerCropCore)\n> > > + scalerCrops.push_back(*scalerCropCore);\n> > > + }\n> > > +\n> > > + for (auto const &[i, scalerCrop] : utils::enumerate(scalerCrops)) {\n> > > + Rectangle nativeCrop = scalerCrop;\n> > >\n> > > if (!nativeCrop.width || !nativeCrop.height)\n> > > nativeCrop = { 0, 0, 1, 1 };\n> > > @@ -1311,13 +1347,13 @@ void CameraData::applyScalerCrop(const ControlList &controls)\n> > > * 2. With the same mid-point, if possible.\n> > > * 3. But it can't go outside the sensor area.\n> > > */\n> > > - Size minSize = cropParams.ispMinCropSize.expandedToAspectRatio(nativeCrop.size());\n> > > + Size minSize = cropParams_[i].ispMinCropSize.expandedToAspectRatio(nativeCrop.size());\n> > > Size size = ispCrop.size().expandedTo(minSize);\n> > > ispCrop = size.centeredTo(ispCrop.center()).enclosedIn(Rectangle(sensorInfo_.outputSize));\n> > >\n> > > - if (ispCrop != cropParams.ispCrop) {\n> > > - cropParams.ispCrop = ispCrop;\n> > > - platformSetIspCrop(cropParams.ispIndex, ispCrop);\n> > > + if (ispCrop != cropParams_[i].ispCrop) {\n> > > + cropParams_[i].ispCrop = ispCrop;\n> > > + platformSetIspCrop(cropParams_[i].ispIndex, ispCrop);\n> >\n> > You can really just pass cropsParams_[i] to platformSetIspCrop. It\n> > contains the index and the updated ispCrop.\n> \n> Ack.\n> \n> > > }\n> > > }\n> > > }\n> > > @@ -1474,9 +1510,16 @@ void CameraData::fillRequestMetadata(const ControlList &bufferControls, Request\n> > > request->metadata().set(controls::SensorTimestamp,\n> > > bufferControls.get(controls::SensorTimestamp).value_or(0));\n> > >\n> > > - if (cropParams_.count(0))\n> > > - request->metadata().set(controls::ScalerCrop,\n> > > - scaleIspCrop(cropParams_[0].ispCrop));\n> > > + if (cropParams_.size()) {\n> >\n> > Again I might have missed in which case cropParams_ is not populated\n> > at all\n> \n> As above, I can remove the check.\n> \n> > > + std::vector crops;\n> > > +\n> > > + for (auto const &[k, v] : cropParams_)\n> > > + crops.push_back(scaleIspCrop(v.ispCrop));\n> > > +\n> > > + request->metadata().set(controls::ScalerCrop, crops[0]);\n> > > + request->metadata().set(controls::rpi::ScalerCrops,\n> > > + Span(crops.data(), crops.size()));\n> > > + }\n> > > }\n> > >\n> > > } /* namespace libcamera */","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 BA7CFC324C\n\tfor ;\n\tWed, 28 Aug 2024 17:02:02 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A7FD9633CD;\n\tWed, 28 Aug 2024 19:02:01 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3FF6A61903\n\tfor ;\n\tWed, 28 Aug 2024 19:01:59 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 2E01F220;\n\tWed, 28 Aug 2024 19:00:51 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"qec52IN2\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1724864451;\n\tbh=f6eO6TzKs8Kb/RJRsDvPahBsEbERmasZs4NCMQY3AlY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=qec52IN2rztcEhvFkgr2zl1Sp3hWSpGi4qbKhEa6504ewowLAZ3OdPjlPJakzQNxy\n\ts5aaSu6uS5w4mErbIMHJ7TWbG+emGbLRERCPJGjaeH0e7y1OyWKiHDTz2YYQRB50m3\n\tysGNgQNz9BuicWvtsqfJ68GZa36Y8onGZz5vrj+s=","Date":"Wed, 28 Aug 2024 20:01:54 +0300","From":"Laurent Pinchart ","To":"Naushir Patuck ","Cc":"Jacopo Mondi ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v1 7/7] pipeline: rpi: Handler controls::rpi::ScalerCrops","Message-ID":"<20240828170154.GA26812@pendragon.ideasonboard.com>","References":"<20240808102346.13065-1-naush@raspberrypi.com>\n\t<20240808102346.13065-8-naush@raspberrypi.com>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]