[{"id":31484,"web_url":"https://patchwork.libcamera.org/comment/31484/","msgid":"","date":"2024-10-01T07:37:42","subject":"Re: [PATCH v2 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 Mon, Sep 30, 2024 at 03:14:15PM 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 | 76 ++++++++++++++-----\n> 1 file changed, 59 insertions(+), 17 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 267e6bd9cd70..9393e79ae4c7 100644\n> --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> @@ -181,12 +181,14 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()\n>\n> \trawStreams_.clear();\n> \toutStreams_.clear();\n> +\tunsigned int rawStreamIndex = 0;\n> +\tunsigned int outStreamIndex = 0;\n>\n> -\tfor (const auto &[index, cfg] : utils::enumerate(config_)) {\n> +\tfor (auto &cfg : config_) {\n> \t\tif (PipelineHandlerBase::isRaw(cfg.pixelFormat))\n> -\t\t\trawStreams_.emplace_back(index, &cfg);\n> +\t\t\trawStreams_.emplace_back(rawStreamIndex++, &cfg);\n> \t\telse\n> -\t\t\toutStreams_.emplace_back(index, &cfg);\n> +\t\t\toutStreams_.emplace_back(outStreamIndex++, &cfg);\n> \t}\n>\n> \t/* Sort the streams so the highest resolution is first. */\n> @@ -565,10 +567,24 @@ int PipelineHandlerBase::configure(Camera *camera, CameraConfiguration *config)\n> \tconst auto cropParamsIt = data->cropParams_.find(0);\n> \tif (cropParamsIt != data->cropParams_.end()) {\n> \t\tconst CameraData::CropParams &cropParams = cropParamsIt->second;\n> -\t\t/* Add the ScalerCrop control limits based on the current mode. */\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(cropParams.ispMinCropSize));\n> \t\tctrlMap[&controls::ScalerCrop] = ControlInfo(ispMinCrop, data->sensorInfo_.analogCrop,\n> \t\t\t\t\t\t\t data->scaleIspCrop(cropParams.ispCrop));\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> +\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}\n> \t}\n>\n> \tdata->controlInfo_ = ControlInfoMap(std::move(ctrlMap), result.controlInfo.idmap());\n> @@ -1295,11 +1311,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> -\tconst auto cropParamsIt = cropParams_.find(0);\n> -\tif (scalerCrop && cropParamsIt != cropParams_.end()) {\n> -\t\tRectangle nativeCrop = *scalerCrop;\n> -\t\tCropParams &cropParams = cropParamsIt->second;\n> +\tconst auto &scalerCropRPi = controls.get>(controls::rpi::ScalerCrops);\n> +\tconst auto &scalerCropCore = controls.get(controls::ScalerCrop);\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\nI think you want to be stricter here and better defines what happens\nif\n1) ScalerCrops has less entries than the number of configured streams\n\n Your ScalerCrops documentation reports\n\n The order of rectangles passed into the control must match the order of\n streams configured by the application. The pipeline handler will only\n configure crop retangles up-to the number of output streams configured.\n All subsequent rectangles passed into this control are ignored by the\n pipeline handler.\n\n which addresses what happens if you have more ScalerCrops entries than\n Streams but not the other way around.\n\n2) Both ScalerCrop and ScalerCrops have been set by the application\n\nThe above code doesn't really handle the two above conditions if I got\nit right.\n\nAlso, but that's up to you to decide, on vc4 you always crop at the\nISP input, while on pisp you always crop on the outputs both for\nScalerCrop and ScalerCrops. Not sure if you need (or can) differentiate,\nin example, ScalerCrop goes to the ISP input crop and ScalerCrops goes\nto the ISP outputs. Up to you!\n\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> @@ -1315,13 +1349,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> \t\t}\n> \t}> }\n> @@ -1478,10 +1512,18 @@ 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> -\tconst auto cropParamsIt = cropParams_.find(0);\n> -\tif (cropParamsIt != cropParams_.end())\n> -\t\trequest->metadata().set(controls::ScalerCrop,\n> -\t\t\t\t\tscaleIspCrop(cropParamsIt->second.ispCrop));\n> +\tif (cropParams_.size()) {\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\tif (crops.size() > 1) {\n> +\t\t\trequest->metadata().set(controls::rpi::ScalerCrops,\n> +\t\t\t\t\t\tSpan(crops.data(), crops.size()));\n> +\t\t}\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 DC561C3257\n\tfor ;\n\tTue, 1 Oct 2024 07:37:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0FC0763512;\n\tTue, 1 Oct 2024 09:37:48 +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 B684860553\n\tfor ;\n\tTue, 1 Oct 2024 09:37:46 +0200 (CEST)","from ideasonboard.com (unknown [5.179.150.95])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 0DC0A669;\n\tTue, 1 Oct 2024 09:36:14 +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=\"cF8wdcAV\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1727768175;\n\tbh=mp5iQ2WbW9zXNPJn2iDtO/RiQjzRwyvWB2eV5hb2RUs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=cF8wdcAVac7WKmg9XXMAq30S7eUry7yblusi2czHsLqlkkuZeIpZ7TSKSZLMXH39O\n\tLpPVBXFpQ1W3ycNx8uhdvAcR/swds/bjAaj+FxdhMshoBTnhT3HgWSy95pXk8W/yil\n\t9NC+3x/+tgB2u3qzpxzxFDX/9cLY69IUxE19JyD8=","Date":"Tue, 1 Oct 2024 09:37:42 +0200","From":"Jacopo Mondi ","To":"Naushir Patuck ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v2 7/7] pipeline: rpi: Handler controls::rpi::ScalerCrops","Message-ID":"","References":"<20240930141415.8857-1-naush@raspberrypi.com>\n\t<20240930141415.8857-8-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240930141415.8857-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":31499,"web_url":"https://patchwork.libcamera.org/comment/31499/","msgid":"","date":"2024-10-01T11:20:17","subject":"Re: [PATCH v2 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\nOn Tue, 1 Oct 2024 at 08:37, Jacopo Mondi wrote:\n>\n> Hi Naush\n>\n> On Mon, Sep 30, 2024 at 03:14:15PM 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 | 76 ++++++++++++++-----\n> > 1 file changed, 59 insertions(+), 17 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 267e6bd9cd70..9393e79ae4c7 100644\n> > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> > @@ -181,12 +181,14 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()\n> >\n> > rawStreams_.clear();\n> > outStreams_.clear();\n> > + unsigned int rawStreamIndex = 0;\n> > + unsigned int outStreamIndex = 0;\n> >\n> > - for (const auto &[index, cfg] : utils::enumerate(config_)) {\n> > + for (auto &cfg : config_) {\n> > if (PipelineHandlerBase::isRaw(cfg.pixelFormat))\n> > - rawStreams_.emplace_back(index, &cfg);\n> > + rawStreams_.emplace_back(rawStreamIndex++, &cfg);\n> > else\n> > - outStreams_.emplace_back(index, &cfg);\n> > + outStreams_.emplace_back(outStreamIndex++, &cfg);\n> > }\n> >\n> > /* Sort the streams so the highest resolution is first. */\n> > @@ -565,10 +567,24 @@ int PipelineHandlerBase::configure(Camera *camera, CameraConfiguration *config)\n> > const auto cropParamsIt = data->cropParams_.find(0);\n> > if (cropParamsIt != data->cropParams_.end()) {\n> > const CameraData::CropParams &cropParams = cropParamsIt->second;\n> > - /* Add the ScalerCrop control limits based on the current mode. */\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(cropParams.ispMinCropSize));\n> > ctrlMap[&controls::ScalerCrop] = ControlInfo(ispMinCrop, data->sensorInfo_.analogCrop,\n> > data->scaleIspCrop(cropParams.ispCrop));\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> > + 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> > + }\n> > }\n> >\n> > data->controlInfo_ = ControlInfoMap(std::move(ctrlMap), result.controlInfo.idmap());\n> > @@ -1295,11 +1311,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> > - const auto cropParamsIt = cropParams_.find(0);\n> > - if (scalerCrop && cropParamsIt != cropParams_.end()) {\n> > - Rectangle nativeCrop = *scalerCrop;\n> > - CropParams &cropParams = cropParamsIt->second;\n> > + const auto &scalerCropRPi = controls.get>(controls::rpi::ScalerCrops);\n> > + const auto &scalerCropCore = controls.get(controls::ScalerCrop);\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> I think you want to be stricter here and better defines what happens\n> if\n> 1) ScalerCrops has less entries than the number of configured streams\n>\n> Your ScalerCrops documentation reports\n>\n> The order of rectangles passed into the control must match the order of\n> streams configured by the application. The pipeline handler will only\n> configure crop retangles up-to the number of output streams configured.\n> All subsequent rectangles passed into this control are ignored by the\n> pipeline handler.\n>\n> which addresses what happens if you have more ScalerCrops entries than\n> Streams but not the other way around.\n>\n> 2) Both ScalerCrop and ScalerCrops have been set by the application\n>\n> The above code doesn't really handle the two above conditions if I got\n> it right.\n\nIn the case of (2), we ignore ScalerCrop and use rpi::ScalerCrops. I\nwill explicitly mention this in the control documentation.\n\n>\n> Also, but that's up to you to decide, on vc4 you always crop at the\n> ISP input, while on pisp you always crop on the outputs both for\n> ScalerCrop and ScalerCrops. Not sure if you need (or can) differentiate,\n> in example, ScalerCrop goes to the ISP input crop and ScalerCrops goes\n> to the ISP outputs. Up to you!\n\nI think there might be some confusion. On both platforms, the crop is\nspecified on the full resolution input coordinate space - and this is\nthe same for both ScalerCrop and rpi::ScalerCrops controls. So they\nare pretty much interchanable if we only want a single crop. Where\nthe crop happens in the ISP HW pipeline is mostly irrelevant right?\n\nRegards,\nNaush\n\n\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> > @@ -1315,13 +1349,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> > }> }\n> > @@ -1478,10 +1512,18 @@ void CameraData::fillRequestMetadata(const ControlList &bufferControls, Request\n> > request->metadata().set(controls::SensorTimestamp,\n> > bufferControls.get(controls::SensorTimestamp).value_or(0));\n> >\n> > - const auto cropParamsIt = cropParams_.find(0);\n> > - if (cropParamsIt != cropParams_.end())\n> > - request->metadata().set(controls::ScalerCrop,\n> > - scaleIspCrop(cropParamsIt->second.ispCrop));\n> > + if (cropParams_.size()) {\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> > + if (crops.size() > 1) {\n> > + request->metadata().set(controls::rpi::ScalerCrops,\n> > + Span(crops.data(), crops.size()));\n> > + }\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 194E0BD80A\n\tfor ;\n\tTue, 1 Oct 2024 11:20:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id ACDF363523;\n\tTue, 1 Oct 2024 13:20:57 +0200 (CEST)","from mail-ed1-x52a.google.com (mail-ed1-x52a.google.com\n\t[IPv6:2a00:1450:4864:20::52a])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3390A60553\n\tfor ;\n\tTue, 1 Oct 2024 13:20:55 +0200 (CEST)","by mail-ed1-x52a.google.com with SMTP id\n\t4fb4d7f45d1cf-5c5cc5ef139so688272a12.0\n\tfor ;\n\tTue, 01 Oct 2024 04:20: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=\"YuahpRza\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1727781654; x=1728386454;\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=GdlwUDCw9+erPQCznkqFW7kuMf6ZuyeqTtmHmmPeY4Y=;\n\tb=YuahpRza1b7Hud4aTGsaUMFfNDYdTBGdbtmUZpmIJ0sD9BhH5VeqB6Bj3899Htt89z\n\tiRKGRe/9jP6NB5eXUFFYCtdyQlKuavn/2o7K3RJubOylgB0T62rIBKsE7fka1Hnw+A+h\n\t4HTR9BH7RIxO3EbNUPQgtMQZOHI/cWTKfTWdJntq57bU+0GjYdl5LG//qdbfC5xFhS8x\n\t3u3Sd3lFVfbQK6US5bGLqYUgPqjGHle0Eu2u/D0aBom+3qf5i+zLeAFoG0Kv55xNfunG\n\tJfMTcF6b0o0jSHWap50LNgrjwkKTSNJZY5ivSzs9Onkq8JN/eieivzF7RlGna4Dlr2Sg\n\t2aXg==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1727781654; x=1728386454;\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=GdlwUDCw9+erPQCznkqFW7kuMf6ZuyeqTtmHmmPeY4Y=;\n\tb=uhz3HBaRJoxXeIHj0f09oHgHllQVPdI/VWEQKTQkrd/wNxPw8GckpElsdarsAMUGCj\n\tHxkO5BsACk25SEgQBbKMOA5njQVnVFQna5FsG4QLBXLRM69Yw1uN06nxDwVr9DHQBaFI\n\tFGA0Gi92eyansxu9/wWXMnV3S1qkfd6W8ZUqy1VlRNwS/FC0Qaz7JZjj+r682Hc8rNYm\n\tDGRAGP4g2t3PJqslhTeh417O8d/EHlpMjNvkS38GDAP4NJtN7AzbjBvQGHe70t6ymFBv\n\t32aeFC0vCtvU3qWwO6GDzsx5sk2q66147DO62jd0ZcojKJTqYkegK+FuO2NzS3ZRhHDs\n\t6hnQ==","X-Gm-Message-State":"AOJu0Yxg9DqawGcsZnBxUHI2GUtkvpSoVPU6KB/M00QVfGIbGfYCWBkX\n\t/SQ48gmKopUND0aYWd9xnYGZjXQWj+85aa6Eh8HpzYz7WreNm0J9jsXy+C0lx2oxnLb93Eo226N\n\tHEV4ip2k7U8eD7SHnSFkrwxyT/XWMGXB0zMS3Cg==","X-Google-Smtp-Source":"AGHT+IFyEFo9mi7pEuOVRHhIUxyK/rWXtwVEeLCCWjFvxYhjM/57ZXvuviel3EOe7WIW1GaXacpB/hrbeGqSauCu0/U=","X-Received":"by 2002:a05:6402:50d0:b0:5c8:97b9:5863 with SMTP id\n\t4fb4d7f45d1cf-5c897b9588amr2497459a12.6.1727781654261;\n\tTue, 01 Oct 2024 04:20:54 -0700 (PDT)","MIME-Version":"1.0","References":"<20240930141415.8857-1-naush@raspberrypi.com>\n\t<20240930141415.8857-8-naush@raspberrypi.com>\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Tue, 1 Oct 2024 12:20:17 +0100","Message-ID":"","Subject":"Re: [PATCH v2 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":31507,"web_url":"https://patchwork.libcamera.org/comment/31507/","msgid":"","date":"2024-10-01T18:08:35","subject":"Re: [PATCH v2 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 Tue, Oct 01, 2024 at 12:20:17PM GMT, Naushir Patuck wrote:\n> Hi Jacopo,\n>\n> On Tue, 1 Oct 2024 at 08:37, Jacopo Mondi wrote:\n> >\n> > Hi Naush\n> >\n> > On Mon, Sep 30, 2024 at 03:14:15PM 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 | 76 ++++++++++++++-----\n> > > 1 file changed, 59 insertions(+), 17 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 267e6bd9cd70..9393e79ae4c7 100644\n> > > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> > > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> > > @@ -181,12 +181,14 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()\n> > >\n> > > rawStreams_.clear();\n> > > outStreams_.clear();\n> > > + unsigned int rawStreamIndex = 0;\n> > > + unsigned int outStreamIndex = 0;\n> > >\n> > > - for (const auto &[index, cfg] : utils::enumerate(config_)) {\n> > > + for (auto &cfg : config_) {\n> > > if (PipelineHandlerBase::isRaw(cfg.pixelFormat))\n> > > - rawStreams_.emplace_back(index, &cfg);\n> > > + rawStreams_.emplace_back(rawStreamIndex++, &cfg);\n> > > else\n> > > - outStreams_.emplace_back(index, &cfg);\n> > > + outStreams_.emplace_back(outStreamIndex++, &cfg);\n> > > }\n> > >\n> > > /* Sort the streams so the highest resolution is first. */\n> > > @@ -565,10 +567,24 @@ int PipelineHandlerBase::configure(Camera *camera, CameraConfiguration *config)\n> > > const auto cropParamsIt = data->cropParams_.find(0);\n> > > if (cropParamsIt != data->cropParams_.end()) {\n> > > const CameraData::CropParams &cropParams = cropParamsIt->second;\n> > > - /* Add the ScalerCrop control limits based on the current mode. */\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(cropParams.ispMinCropSize));\n> > > ctrlMap[&controls::ScalerCrop] = ControlInfo(ispMinCrop, data->sensorInfo_.analogCrop,\n> > > data->scaleIspCrop(cropParams.ispCrop));\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> > > + 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> > > + }\n> > > }\n> > >\n> > > data->controlInfo_ = ControlInfoMap(std::move(ctrlMap), result.controlInfo.idmap());\n> > > @@ -1295,11 +1311,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> > > - const auto cropParamsIt = cropParams_.find(0);\n> > > - if (scalerCrop && cropParamsIt != cropParams_.end()) {\n> > > - Rectangle nativeCrop = *scalerCrop;\n> > > - CropParams &cropParams = cropParamsIt->second;\n> > > + const auto &scalerCropRPi = controls.get>(controls::rpi::ScalerCrops);\n> > > + const auto &scalerCropCore = controls.get(controls::ScalerCrop);\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> > I think you want to be stricter here and better defines what happens\n> > if\n> > 1) ScalerCrops has less entries than the number of configured streams\n> >\n> > Your ScalerCrops documentation reports\n> >\n> > The order of rectangles passed into the control must match the order of\n> > streams configured by the application. The pipeline handler will only\n> > configure crop retangles up-to the number of output streams configured.\n> > All subsequent rectangles passed into this control are ignored by the\n> > pipeline handler.\n> >\n> > which addresses what happens if you have more ScalerCrops entries than\n> > Streams but not the other way around.\n> >\n> > 2) Both ScalerCrop and ScalerCrops have been set by the application\n> >\n> > The above code doesn't really handle the two above conditions if I got\n> > it right.\n>\n> In the case of (2), we ignore ScalerCrop and use rpi::ScalerCrops. I\n> will explicitly mention this in the control documentation.\n>\n\nThanks\n\n> >\n> > Also, but that's up to you to decide, on vc4 you always crop at the\n> > ISP input, while on pisp you always crop on the outputs both for\n> > ScalerCrop and ScalerCrops. Not sure if you need (or can) differentiate,\n> > in example, ScalerCrop goes to the ISP input crop and ScalerCrops goes\n> > to the ISP outputs. Up to you!\n>\n> I think there might be some confusion. On both platforms, the crop is\n> specified on the full resolution input coordinate space - and this is\n> the same for both ScalerCrop and rpi::ScalerCrops controls. So they\n> are pretty much interchanable if we only want a single crop. Where\n> the crop happens in the ISP HW pipeline is mostly irrelevant right?\n>\n\nWell you know, cropping at the ISP input might have an impact on the\nsuccessive scaling operations. I was thinking if it was of any use mentioning\nScalerCrop always apply before scaling and ScalerCrops apply on the\noutputs, but we can't make such assumption about ScalerCrop\ngenerically, as where to apply depends on the platform specificities.\n\nFine, please add\nReviewed-by: Jacopo Mondi \nto next version!\n\nThanks\n j\n> Regards,\n> Naush\n>\n>\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> > > @@ -1315,13 +1349,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> > > }> }\n> > > @@ -1478,10 +1512,18 @@ void CameraData::fillRequestMetadata(const ControlList &bufferControls, Request\n> > > request->metadata().set(controls::SensorTimestamp,\n> > > bufferControls.get(controls::SensorTimestamp).value_or(0));\n> > >\n> > > - const auto cropParamsIt = cropParams_.find(0);\n> > > - if (cropParamsIt != cropParams_.end())\n> > > - request->metadata().set(controls::ScalerCrop,\n> > > - scaleIspCrop(cropParamsIt->second.ispCrop));\n> > > + if (cropParams_.size()) {\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> > > + if (crops.size() > 1) {\n> > > + request->metadata().set(controls::rpi::ScalerCrops,\n> > > + Span(crops.data(), crops.size()));\n> > > + }\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 2A60BC3257\n\tfor ;\n\tTue, 1 Oct 2024 18:08:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3FDCD63512;\n\tTue, 1 Oct 2024 20:08:41 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8D77260553\n\tfor ;\n\tTue, 1 Oct 2024 20:08:39 +0200 (CEST)","from ideasonboard.com (unknown [5.77.89.72])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 05C3E2E3;\n\tTue, 1 Oct 2024 20:07:06 +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=\"EgOfPIc1\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1727806027;\n\tbh=Cak9u7jfeTOoMji1lrNuhaxy4GZx6DVyHrodYn+dnC4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=EgOfPIc1B8XVf+1NztcIGtVNfg1oXWuWNu/clruMaD8fqfcFgvM28Hoz5WAV9Rb4l\n\t7ShWpZ6o7rV529eVAp+IbEygrlCFTTnxwSnH4DHIAHCtO6yj8hRz7YRJMWnuL/Gu28\n\tjKyATbf2OMiDlytnfS24Dj6Xbw9jcccayBxu1qKQ=","Date":"Tue, 1 Oct 2024 20:08:35 +0200","From":"Jacopo Mondi ","To":"Naushir Patuck ","Cc":"Jacopo Mondi , \n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v2 7/7] pipeline: rpi: Handler controls::rpi::ScalerCrops","Message-ID":"","References":"<20240930141415.8857-1-naush@raspberrypi.com>\n\t<20240930141415.8857-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\" "}}]