[{"id":23501,"web_url":"https://patchwork.libcamera.org/comment/23501/","msgid":"<165581370622.1149771.1806782484227783430@Monstersaurus>","date":"2022-06-21T12:15:06","subject":"Re: [libcamera-devel] [PATCH 2/3] pipeline: ipa: raspberrypi:\n\tCorrectly report available control limits","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Naushir Patuck via libcamera-devel (2022-06-10 13:25:32)\n> The ipa currently advertises a static ControlInfoMap to specify the available\n> controls and their limits. Fix this limitation by having the IPA populate a new\n> ControlInfoMap with updated limits for ExposureTime, AnalogueGain, and\n> FrameDurationLimits controls based on the current sensor mode. This new\n> ControlInfoMap is then returned back to the pipeline handler and available to\n> the application after a successful Camera::Configure() call.\n> \n> Signed-off-by: Naushir Patuck \n> ---\n> include/libcamera/ipa/raspberrypi.mojom | 1 +\n> src/ipa/raspberrypi/raspberrypi.cpp | 24 ++++++++++++++++---\n> .../pipeline/raspberrypi/raspberrypi.cpp | 3 +++\n> 3 files changed, 25 insertions(+), 3 deletions(-)\n> \n> diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom\n> index 77f52c282b0f..c0de435b7b33 100644\n> --- a/include/libcamera/ipa/raspberrypi.mojom\n> +++ b/include/libcamera/ipa/raspberrypi.mojom\n> @@ -45,6 +45,7 @@ struct IPAConfig {\n> \n> struct IPAConfigResult {\n\nAha, I see the precedent for \"Result\" now. so perhaps it's more suitable\nin the previous patch than I realised.\n\n\n> float modeSensitivity;\n> + libcamera.ControlInfoMap controlInfo;\n> };\n> \n> struct StartConfig {\n> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> index 089528f5e126..295f6b735dc0 100644\n> --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> @@ -74,8 +74,6 @@ constexpr Duration controllerMinFrameDuration = 1.0s / 30.0;\n> /* List of controls handled by the Raspberry Pi IPA */\n> static const ControlInfoMap::Map ipaControls{\n> { &controls::AeEnable, ControlInfo(false, true) },\n> - { &controls::ExposureTime, ControlInfo(0, 999999) },\n> - { &controls::AnalogueGain, ControlInfo(1.0f, 32.0f) },\n> { &controls::AeMeteringMode, ControlInfo(controls::AeMeteringModeValues) },\n> { &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },\n> { &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) },\n> @@ -89,7 +87,6 @@ static const ControlInfoMap::Map ipaControls{\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> - { &controls::FrameDurationLimits, ControlInfo(INT64_C(1000), INT64_C(1000000000)) },\n> { &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) }\n> };\n> \n> @@ -446,6 +443,27 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo,\n> ASSERT(controls);\n> *controls = std::move(ctrls);\n> \n> + /*\n> + * Apply the correct limits to the exposure, gain and frame duration controls\n> + * based on the current sensor mode.\n> + */\n> + ControlInfoMap::Map ctrlMap = ipaControls;\n> + const Duration minSensorFrameDuration = mode_.min_frame_length * mode_.line_length;\n> + const Duration maxSensorFrameDuration = mode_.max_frame_length * mode_.line_length;\n> + ctrlMap.emplace(&controls::FrameDurationLimits,\n> + ControlInfo(static_cast(minSensorFrameDuration.get()),\n> + static_cast(maxSensorFrameDuration.get())));\n> +\n> + ctrlMap.emplace(&controls::AnalogueGain,\n> + ControlInfo(1.0f, static_cast(helper_->Gain(maxSensorGainCode_))));\n> +\n> + const uint32_t exposureMin = sensorCtrls_.at(V4L2_CID_EXPOSURE).min().get();\n> + const uint32_t exposureMax = sensorCtrls_.at(V4L2_CID_EXPOSURE).max().get();\n> + ctrlMap.emplace(&controls::ExposureTime,\n> + ControlInfo(static_cast(helper_->Exposure(exposureMin).get()),\n> + static_cast(helper_->Exposure(exposureMax).get())));\n> +\n> + result->controlInfo = ControlInfoMap(std::move(ctrlMap), controls::controls);\n\n\naha, this means applications can't read the exposure time / gain\ncontrols before the camera is configured.\n\nI think that sounds 'correct' - but I wonder if we need to make that\nclear in some documentation somewhere, or somehow in an extension of\n'simple-cam' showing how to manage camera controls. (Not to block this\npatch of course).\n\n\nWe should make sure applications realise they have to recheck controls\nafter a configuration operation. (Maybe that's obvious .. I'm not sure).\n\nI was dwelling on if we should only send the updates to the\nControlInfoMap over the IPC mechanism, but I don't think this is\ncritical path, and it's simpler to send the whole updated map - so:\n\n\nReviewed-by: Kieran Bingham \n\n> return 0;\n> }\n> \n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index d980c1a71dd8..4596f2babcea 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -940,6 +940,9 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)\n> /* Store the mode sensitivity for the application. */\n> data->properties_.set(properties::SensorSensitivity, result.modeSensitivity);\n> \n> + /* Update the controls that the Raspberry Pi IPA can handle. */\n> + data->controlInfo_ = result.controlInfo;\n> +\n> /* Setup the Video Mux/Bridge entities. */\n> for (auto &[device, link] : data->bridgeDevices_) {\n> /*\n> -- \n> 2.25.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 5139DBE173\n\tfor ;\n\tTue, 21 Jun 2022 12:15:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1E8BC65638;\n\tTue, 21 Jun 2022 14:15:10 +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 4E92261FB2\n\tfor ;\n\tTue, 21 Jun 2022 14:15:09 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 037D2576;\n\tTue, 21 Jun 2022 14:15:08 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1655813710;\n\tbh=eIhcSin7LfUmQtYD6WdLLbJqdTGvAVMDHBRRUYKhISk=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:\n\tFrom;\n\tb=Nr8fzGR8C2emWpYIYThvLTQcjz9KCmVQQTS55IRwd62kfQR0xpBl/tO+XVrQJBn/s\n\tYnWYjqsIyvzi4VR7mKmoUQxYGKsyZ7bEufmD6TQXE4ezdfGArWv6m0CKErK02HW9fx\n\tBQUBt4FNBrWx2henvkvDNv5Ny6+gZnaFkS0MoisB0yXXlvbHF+KggRwwjuJwzWbrC7\n\tud8E3+WetGm+gtPJ1rpn50exyGtqojaRPUxGYgBb+aq2YgtbWC0qQIizz5etY43wvs\n\tgcuKOnVvMJ5nvdYzaF5e6S32Z1GOrGIRFztmzrCaWS2KF6ngI1ObZy7qNe+GYLtHzE\n\tWBDJUCVdTiMeg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1655813709;\n\tbh=eIhcSin7LfUmQtYD6WdLLbJqdTGvAVMDHBRRUYKhISk=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=MyqgGplV5398G5tlt7HYPUKlqxqSS/GaQlOkWVRv56irv4yhAkE90fVMp3n9MIsTx\n\tp+hA8cmn/LkZc18vcJoTxoQJQSI5yHKM6g/bojRQ5HhjOVvAzs3zOHESDdH2hiCO3u\n\tT1awMpvnTjcfTHZ7mbB+CnDWCTcsMYBXE4Dk63Os="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"MyqgGplV\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20220610122533.11888-2-naush@raspberrypi.com>","References":"<20220610122533.11888-1-naush@raspberrypi.com>\n\t<20220610122533.11888-2-naush@raspberrypi.com>","To":"Naushir Patuck ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Tue, 21 Jun 2022 13:15:06 +0100","Message-ID":"<165581370622.1149771.1806782484227783430@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH 2/3] pipeline: ipa: raspberrypi:\n\tCorrectly report available control limits","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","From":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23503,"web_url":"https://patchwork.libcamera.org/comment/23503/","msgid":"","date":"2022-06-21T12:36:09","subject":"Re: [libcamera-devel] [PATCH 2/3] pipeline: ipa: raspberrypi:\n\tCorrectly report available control limits","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Kieran,\n\nThank you for your feedback.\n\nOn Tue, 21 Jun 2022 at 13:15, Kieran Bingham <\nkieran.bingham@ideasonboard.com> wrote:\n\n> Quoting Naushir Patuck via libcamera-devel (2022-06-10 13:25:32)\n> > The ipa currently advertises a static ControlInfoMap to specify the\n> available\n> > controls and their limits. Fix this limitation by having the IPA\n> populate a new\n> > ControlInfoMap with updated limits for ExposureTime, AnalogueGain, and\n> > FrameDurationLimits controls based on the current sensor mode. This new\n> > ControlInfoMap is then returned back to the pipeline handler and\n> available to\n> > the application after a successful Camera::Configure() call.\n> >\n> > Signed-off-by: Naushir Patuck \n> > ---\n> > include/libcamera/ipa/raspberrypi.mojom | 1 +\n> > src/ipa/raspberrypi/raspberrypi.cpp | 24 ++++++++++++++++---\n> > .../pipeline/raspberrypi/raspberrypi.cpp | 3 +++\n> > 3 files changed, 25 insertions(+), 3 deletions(-)\n> >\n> > diff --git a/include/libcamera/ipa/raspberrypi.mojom\n> b/include/libcamera/ipa/raspberrypi.mojom\n> > index 77f52c282b0f..c0de435b7b33 100644\n> > --- a/include/libcamera/ipa/raspberrypi.mojom\n> > +++ b/include/libcamera/ipa/raspberrypi.mojom\n> > @@ -45,6 +45,7 @@ struct IPAConfig {\n> >\n> > struct IPAConfigResult {\n>\n> Aha, I see the precedent for \"Result\" now. so perhaps it's more suitable\n> in the previous patch than I realised.\n>\n>\n> > float modeSensitivity;\n> > + libcamera.ControlInfoMap controlInfo;\n> > };\n> >\n> > struct StartConfig {\n> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp\n> b/src/ipa/raspberrypi/raspberrypi.cpp\n> > index 089528f5e126..295f6b735dc0 100644\n> > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > @@ -74,8 +74,6 @@ constexpr Duration controllerMinFrameDuration = 1.0s /\n> 30.0;\n> > /* List of controls handled by the Raspberry Pi IPA */\n> > static const ControlInfoMap::Map ipaControls{\n> > { &controls::AeEnable, ControlInfo(false, true) },\n> > - { &controls::ExposureTime, ControlInfo(0, 999999) },\n> > - { &controls::AnalogueGain, ControlInfo(1.0f, 32.0f) },\n> > { &controls::AeMeteringMode,\n> ControlInfo(controls::AeMeteringModeValues) },\n> > { &controls::AeConstraintMode,\n> ControlInfo(controls::AeConstraintModeValues) },\n> > { &controls::AeExposureMode,\n> ControlInfo(controls::AeExposureModeValues) },\n> > @@ -89,7 +87,6 @@ static const ControlInfoMap::Map ipaControls{\n> > { &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },\n> > { &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f)\n> },\n> > { &controls::ScalerCrop, ControlInfo(Rectangle{},\n> Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },\n> > - { &controls::FrameDurationLimits, ControlInfo(INT64_C(1000),\n> INT64_C(1000000000)) },\n> > { &controls::draft::NoiseReductionMode,\n> ControlInfo(controls::draft::NoiseReductionModeValues) }\n> > };\n> >\n> > @@ -446,6 +443,27 @@ int IPARPi::configure(const IPACameraSensorInfo\n> &sensorInfo,\n> > ASSERT(controls);\n> > *controls = std::move(ctrls);\n> >\n> > + /*\n> > + * Apply the correct limits to the exposure, gain and frame\n> duration controls\n> > + * based on the current sensor mode.\n> > + */\n> > + ControlInfoMap::Map ctrlMap = ipaControls;\n> > + const Duration minSensorFrameDuration = mode_.min_frame_length *\n> mode_.line_length;\n> > + const Duration maxSensorFrameDuration = mode_.max_frame_length *\n> mode_.line_length;\n> > + ctrlMap.emplace(&controls::FrameDurationLimits,\n> > +\n> ControlInfo(static_cast(minSensorFrameDuration.get()),\n> > +\n> static_cast(maxSensorFrameDuration.get())));\n> > +\n> > + ctrlMap.emplace(&controls::AnalogueGain,\n> > + ControlInfo(1.0f,\n> static_cast(helper_->Gain(maxSensorGainCode_))));\n> > +\n> > + const uint32_t exposureMin =\n> sensorCtrls_.at(V4L2_CID_EXPOSURE).min().get();\n> > + const uint32_t exposureMax =\n> sensorCtrls_.at(V4L2_CID_EXPOSURE).max().get();\n> > + ctrlMap.emplace(&controls::ExposureTime,\n> > +\n> ControlInfo(static_cast(helper_->Exposure(exposureMin).get()),\n> > +\n> static_cast(helper_->Exposure(exposureMax).get())));\n> > +\n> > + result->controlInfo = ControlInfoMap(std::move(ctrlMap),\n> controls::controls);\n>\n>\n> aha, this means applications can't read the exposure time / gain\n> controls before the camera is configured.\n>\n> I think that sounds 'correct' - but I wonder if we need to make that\n> clear in some documentation somewhere, or somehow in an extension of\n> 'simple-cam' showing how to manage camera controls. (Not to block this\n> patch of course).\n>\n\nI did stop to think what to do here. We could add ControlInfo for\nExposureTime and AnalogueGain for the \"default\" sensor mode, but\ngetting to the sensor mode is a bit more difficult in ipa::init() as things\nstand. Additionally, these values would likely have to be ignored by\nthe application. It has to do a camera::Configure() call before starting,\nso the values might have changed, rendering the previous values as\ninvalid.\n\n\n>\n>\n> We should make sure applications realise they have to recheck controls\n> after a configuration operation. (Maybe that's obvious .. I'm not sure).\n>\n\nDocumenting this is probably a good thing so as to avoid application writers\nfrom tripping up!\n\nRegards,\nNaush\n\n\n>\n> I was dwelling on if we should only send the updates to the\n> ControlInfoMap over the IPC mechanism, but I don't think this is\n> critical path, and it's simpler to send the whole updated map - so:\n>\n>\n> Reviewed-by: Kieran Bingham \n>\n> > return 0;\n> > }\n> >\n> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > index d980c1a71dd8..4596f2babcea 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > @@ -940,6 +940,9 @@ int PipelineHandlerRPi::configure(Camera *camera,\n> CameraConfiguration *config)\n> > /* Store the mode sensitivity for the application. */\n> > data->properties_.set(properties::SensorSensitivity,\n> result.modeSensitivity);\n> >\n> > + /* Update the controls that the Raspberry Pi IPA can handle. */\n> > + data->controlInfo_ = result.controlInfo;\n> > +\n> > /* Setup the Video Mux/Bridge entities. */\n> > for (auto &[device, link] : data->bridgeDevices_) {\n> > /*\n> > --\n> > 2.25.1\n> >\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 8055BBE173\n\tfor ;\n\tTue, 21 Jun 2022 12:36:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7674365637;\n\tTue, 21 Jun 2022 14:36:16 +0200 (CEST)","from mail-lf1-x132.google.com (mail-lf1-x132.google.com\n\t[IPv6:2a00:1450:4864:20::132])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A146D61FB2\n\tfor ;\n\tTue, 21 Jun 2022 14:36:14 +0200 (CEST)","by mail-lf1-x132.google.com with SMTP id i18so8487412lfu.8\n\tfor ;\n\tTue, 21 Jun 2022 05:36:14 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1655814976;\n\tbh=vH7E5Kab1rSBlKYh1rY1xO9ZAtrHbhlFvC8OlMm3gFc=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=hJ0zO/4aPlT0FfKd0ShmTlHGjFR9Ds3vxFBkV/caEHIfmwlZpSKRMAvAJcHBE8bH3\n\tOWR8totnyn24in5/RImuhKQcfZ1trAxNg14VU16felv2tRVPS+wVTjtNtWGladYeuO\n\tNVrGINzw1tNJUqsq3mM7MPL92Haqzg2J2xDo8Aqw8y9xUhjwS0oZ47LmGXo2EWX6bt\n\tE9w9IwD5VGougfifCadDxEQRoO4k5Tju6T5M6nvVsp6DjBZLDzJ9m97rqgnXJMUfnW\n\tcZvnopvYyNv6QQNdv69XpAFXT7dwj86+ZNb0VoDbZaIJiTIlSzhRSdUyrC7S+Ats7q\n\t6c03WhVLxJIIw==","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=AxgePpUrRf7eLqUNE70DKiMEV6gl8ivKJVCPfRcsxbU=;\n\tb=R5KImtVAEKrnbeEqQRe/DaoxneIa5H2FhtvUurMvmY7+gNuGffqTiLFqNz58yljEum\n\tv5YDV4Zj77BcATcAFzyuRkYuzkbAUcBgNEu3Z1wS78XQCcLv8Wt75LxPkGw2ow7i4eeH\n\tNJqD0EbNnB9WEylHc00ytnz05YnXSuijDF2wNfKYjCaeDPvYdm25jpDYwyBUTAkvNbjM\n\tqQT/FVj4hXEll+EOEV31kwUrJZ+ca1FFMmgDKv2Ro3hzJOLcZdOrDCypYpACg8nNGiqV\n\tWIjLJm7OWZ+75IFFn5bl8dQRusjYBf46TcBmv+XVu1Q6OX8mErSW0jOyFk4ffe+P4La9\n\tMCCg=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"R5KImtVA\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=AxgePpUrRf7eLqUNE70DKiMEV6gl8ivKJVCPfRcsxbU=;\n\tb=2PDjIA95Z7KniNdIvln3+i2QoDGPJkrdLXBTKx9KtY+LEMMIWZh3g3bBga/4tvD83m\n\tpDg5HFlXSWQIMzVDx7ApTssX75yhYSxSbss4BVa1v/jai4jrOlIIUcTgeTLZ052njSkT\n\tYqtcQ07IM0YXap+uKibaMkTTunxfyyxivT8nXDrVLzqSUtUe1s5b8qoEAdZR8vqyNUMh\n\tBoVPVBHtBuiC2uC7hMa6Cal8ca98bdkk7o6K1iZxckeRFxzyY+Fc43kvAqU/zOsFqBcR\n\tWJx5eTFrGWSMTezbmDITLofpRgLrheQ5Vd5AQQ4frRCXPmN2/w8QYxFJ5EDPxNf6xx7V\n\tqlaw==","X-Gm-Message-State":"AJIora9c7YpdmAs4VTRS9WNw9ibIdn02yXK2h53+A9YLe2/MfiAVej6C\n\tsGKCxXb98AP+G0dO+b/QtYfz82bU5FHgJ2Plhr4y/Wpa93oBowuT","X-Google-Smtp-Source":"AGRyM1u85FmYOU7k1K43YVmFkj0V21Ox2MMz170VW38NCJhJIsNqq5rRCNiEz/p+18+q5B4k3MwRuNmFUlYMnQuN3ig=","X-Received":"by 2002:a05:6512:12c5:b0:479:28ec:c04a with SMTP id\n\tp5-20020a05651212c500b0047928ecc04amr16530208lfg.510.1655814973864;\n\tTue, 21 Jun 2022 05:36:13 -0700 (PDT)","MIME-Version":"1.0","References":"<20220610122533.11888-1-naush@raspberrypi.com>\n\t<20220610122533.11888-2-naush@raspberrypi.com>\n\t<165581370622.1149771.1806782484227783430@Monstersaurus>","In-Reply-To":"<165581370622.1149771.1806782484227783430@Monstersaurus>","Date":"Tue, 21 Jun 2022 13:36:09 +0100","Message-ID":"","To":"Kieran Bingham ","Content-Type":"multipart/alternative; boundary=\"00000000000054d4a605e1f47784\"","Subject":"Re: [libcamera-devel] [PATCH 2/3] pipeline: ipa: raspberrypi:\n\tCorrectly report available control limits","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","From":"Naushir Patuck via libcamera-devel\n\t","Reply-To":"Naushir Patuck ","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23506,"web_url":"https://patchwork.libcamera.org/comment/23506/","msgid":"","date":"2022-06-21T12:47:47","subject":"Re: [libcamera-devel] [PATCH 2/3] pipeline: ipa: raspberrypi:\n\tCorrectly report available control limits","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nThank you for the patch.\n\nOn Tue, Jun 21, 2022 at 01:36:09PM +0100, Naushir Patuck via libcamera-devel wrote:\n> On Tue, 21 Jun 2022 at 13:15, Kieran Bingham wrote:\n> > Quoting Naushir Patuck via libcamera-devel (2022-06-10 13:25:32)\n> > > The ipa currently advertises a static ControlInfoMap to specify the available\n> > > controls and their limits. Fix this limitation by having the IPA populate a new\n> > > ControlInfoMap with updated limits for ExposureTime, AnalogueGain, and\n> > > FrameDurationLimits controls based on the current sensor mode. This new\n> > > ControlInfoMap is then returned back to the pipeline handler and available to\n> > > the application after a successful Camera::Configure() call.\n\ns/Configure/configure/\n\n> > >\n> > > Signed-off-by: Naushir Patuck \n> > > ---\n> > > include/libcamera/ipa/raspberrypi.mojom | 1 +\n> > > src/ipa/raspberrypi/raspberrypi.cpp | 24 ++++++++++++++++---\n> > > .../pipeline/raspberrypi/raspberrypi.cpp | 3 +++\n> > > 3 files changed, 25 insertions(+), 3 deletions(-)\n> > >\n> > > diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom\n> > > index 77f52c282b0f..c0de435b7b33 100644\n> > > --- a/include/libcamera/ipa/raspberrypi.mojom\n> > > +++ b/include/libcamera/ipa/raspberrypi.mojom\n> > > @@ -45,6 +45,7 @@ struct IPAConfig {\n> > >\n> > > struct IPAConfigResult {\n> >\n> > Aha, I see the precedent for \"Result\" now. so perhaps it's more suitable\n> > in the previous patch than I realised.\n> >\n> > > float modeSensitivity;\n> > > + libcamera.ControlInfoMap controlInfo;\n> > > };\n> > >\n> > > struct StartConfig {\n> > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > index 089528f5e126..295f6b735dc0 100644\n> > > --- a/src/ipa/raspberrypi/raspberrypi.cpp\n> > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp\n> > > @@ -74,8 +74,6 @@ constexpr Duration controllerMinFrameDuration = 1.0s / 30.0;\n> > > /* List of controls handled by the Raspberry Pi IPA */\n> > > static const ControlInfoMap::Map ipaControls{\n> > > { &controls::AeEnable, ControlInfo(false, true) },\n> > > - { &controls::ExposureTime, ControlInfo(0, 999999) },\n> > > - { &controls::AnalogueGain, ControlInfo(1.0f, 32.0f) },\n> > > { &controls::AeMeteringMode, ControlInfo(controls::AeMeteringModeValues) },\n> > > { &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },\n> > > { &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) },\n> > > @@ -89,7 +87,6 @@ static const ControlInfoMap::Map ipaControls{\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> > > - { &controls::FrameDurationLimits, ControlInfo(INT64_C(1000), INT64_C(1000000000)) },\n> > > { &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) }\n> > > };\n> > >\n> > > @@ -446,6 +443,27 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo,\n> > > ASSERT(controls);\n> > > *controls = std::move(ctrls);\n> > >\n> > > + /*\n> > > + * Apply the correct limits to the exposure, gain and frame duration controls\n> > > + * based on the current sensor mode.\n> > > + */\n> > > + ControlInfoMap::Map ctrlMap = ipaControls;\n> > > + const Duration minSensorFrameDuration = mode_.min_frame_length * mode_.line_length;\n> > > + const Duration maxSensorFrameDuration = mode_.max_frame_length * mode_.line_length;\n> > > + ctrlMap.emplace(&controls::FrameDurationLimits,\n> > > + ControlInfo(static_cast(minSensorFrameDuration.get()),\n> > > + static_cast(maxSensorFrameDuration.get())));\n> > > +\n> > > + ctrlMap.emplace(&controls::AnalogueGain,\n> > > + ControlInfo(1.0f, static_cast(helper_->Gain(maxSensorGainCode_))));\n> > > +\n> > > + const uint32_t exposureMin = sensorCtrls_.at(V4L2_CID_EXPOSURE).min().get();\n> > > + const uint32_t exposureMax = sensorCtrls_.at(V4L2_CID_EXPOSURE).max().get();\n> > > + ctrlMap.emplace(&controls::ExposureTime,\n> > > + ControlInfo(static_cast(helper_->Exposure(exposureMin).get()),\n> > > + static_cast(helper_->Exposure(exposureMax).get())));\n> > > +\n> > > + result->controlInfo = ControlInfoMap(std::move(ctrlMap), controls::controls);\n> >\n> > aha, this means applications can't read the exposure time / gain\n> > controls before the camera is configured.\n> >\n> > I think that sounds 'correct' - but I wonder if we need to make that\n> > clear in some documentation somewhere, or somehow in an extension of\n> > 'simple-cam' showing how to manage camera controls. (Not to block this\n> > patch of course).\n> \n> I did stop to think what to do here. We could add ControlInfo for\n> ExposureTime and AnalogueGain for the \"default\" sensor mode, but\n> getting to the sensor mode is a bit more difficult in ipa::init() as things\n> stand. Additionally, these values would likely have to be ignored by\n> the application. It has to do a camera::Configure() call before starting,\n> so the values might have changed, rendering the previous values as\n> invalid.\n> \n> > We should make sure applications realise they have to recheck controls\n> > after a configuration operation. (Maybe that's obvious .. I'm not sure).\n> \n> Documenting this is probably a good thing so as to avoid application writers\n> from tripping up!\n\nThis API is way too fragile, even with documentation. That's fine, we'll\nfix it later, but for now I'd like to report *something* even before the\nfirst configure() call. Let's make it as meaningful as reasonable\npossible, that will be good enough.\n\n> > I was dwelling on if we should only send the updates to the\n> > ControlInfoMap over the IPC mechanism, but I don't think this is\n> > critical path, and it's simpler to send the whole updated map - so:\n> >\n> > Reviewed-by: Kieran Bingham \n> >\n> > > return 0;\n> > > }\n> > >\n> > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > index d980c1a71dd8..4596f2babcea 100644\n> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > @@ -940,6 +940,9 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)\n> > > /* Store the mode sensitivity for the application. */\n> > > data->properties_.set(properties::SensorSensitivity, result.modeSensitivity);\n> > >\n> > > + /* Update the controls that the Raspberry Pi IPA can handle. */\n> > > + data->controlInfo_ = result.controlInfo;\n> > > +\n> > > /* Setup the Video Mux/Bridge entities. */\n> > > for (auto &[device, link] : data->bridgeDevices_) {\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 A39F6BE173\n\tfor ;\n\tTue, 21 Jun 2022 12:48:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 182BA65634;\n\tTue, 21 Jun 2022 14:48:05 +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 8E92C61FB2\n\tfor ;\n\tTue, 21 Jun 2022 14:48:03 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 11CCB104;\n\tTue, 21 Jun 2022 14:48:03 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1655815685;\n\tbh=xypfcKy5ZSFg2tf7MWm0rotZIGvT2RfjwCCmC6kQDu0=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=eOgES5wh1qsUE/EkxFMO8+GU1aQSrxcrZ9he/UAkYu63IX9uF2PwDkFHuZFXS8N8J\n\tI+e5b3jPsY5Qxl5QHh55qgwydnyL4AWUnDsxV3/ftWrQqFJBI9agED6HqMRnYdQQAJ\n\twcShVZ1HCnK3wcb147V7q0AhnbluRiJdKAhQTNZfHyUYXOTtUQAx4gJthfQV1Xg9CU\n\t5aHVdIh2Qb6A8dF7u43HdHI0H3IM+5bKckJ+oVoUk8usP95MZiqUlFOuJm0WGXFTFX\n\t25tEVcDPmXcY8fJzQGfgiRHpGexyQZrjeiqI2+TBBzck3IUuSrgSexOT2F6AMKihaS\n\tXH3sVdOBdUPog==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1655815683;\n\tbh=xypfcKy5ZSFg2tf7MWm0rotZIGvT2RfjwCCmC6kQDu0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Yw43yUpeHo+7u9+BIgzBxestTE1PEcn/8amkN/LwCXK/nKKQfMbKke+j8AudGrJrx\n\t9xSWxGA+O7Ru1Sn6/0lh0oOgIiYp5BVxDn4kPqz+0TxBo2gXtpCC0at4IqzMsv+00i\n\tdxjGZ4su/pXo7RnL+fvJU4uMTKM3RsFtzi93e2/c="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"Yw43yUpe\"; dkim-atps=neutral","Date":"Tue, 21 Jun 2022 15:47:47 +0300","To":"Naushir Patuck ","Message-ID":"","References":"<20220610122533.11888-1-naush@raspberrypi.com>\n\t<20220610122533.11888-2-naush@raspberrypi.com>\n\t<165581370622.1149771.1806782484227783430@Monstersaurus>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 2/3] pipeline: ipa: raspberrypi:\n\tCorrectly report available control limits","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","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]