[{"id":33331,"web_url":"https://patchwork.libcamera.org/comment/33331/","msgid":"<173927656369.2293020.2111425735382346526@ping.linuxembedded.co.uk>","date":"2025-02-11T12:22:43","subject":"Re: [PATCH v3] ipa: rpi: Apply default ControlInfo values for sensor\n\tcontrols","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Naushir Patuck (2025-02-11 10:20:02)\n> The existing IPA initialisation code did not set default values for\n> some sensor related controls. This caused a crash using libcamerify\n> when the it was trying to access the default value for\n> controls::FrameDurationLimits as part of a recent change.\n> \n> Ensure controls::FrameDurationLimits, controls::AnalogueGain and\n> controls::ExposureTime advertise default values along with the existing\n> min/max values. The default is set to the defaults defined in the IPA\n> set during initialisation.\n> \n> Bug: https://bugs.libcamera.org/show_bug.cgi?id=253\n> Signed-off-by: Naushir Patuck \n> Reviewed-by: David Plowman \n\nReviewed-by: Kieran Bingham \n\nIf you're ready with this I can merge it.\n\nI saw some githubs fly by about the default values of AWB/AGC enable.\nShould they be set to report they are enabled by default too?\n\n(Let's treat that as a separate topic from this patch though).\n\n\n\n> ---\n> \n> Changes in v3:\n> \n> Need to apply the same default in the init phase since the test in the\n> V4L2CameraProxy::setFmtFromConfig() happens before calling ipa::configure().\n> \n> ---\n> src/ipa/rpi/common/ipa_base.cpp | 18 ++++++++++++------\n> 1 file changed, 12 insertions(+), 6 deletions(-)\n> \n> diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp\n> index bd3c22000df5..5d2c6d40da3e 100644\n> --- a/src/ipa/rpi/common/ipa_base.cpp\n> +++ b/src/ipa/rpi/common/ipa_base.cpp\n> @@ -61,12 +61,13 @@ const ControlInfoMap::Map ipaControls{\n> ControlInfo(static_cast(controls::ExposureTimeModeAuto),\n> static_cast(controls::ExposureTimeModeManual),\n> static_cast(controls::ExposureTimeModeAuto)) },\n> - { &controls::ExposureTime, ControlInfo(0, 66666) },\n> + { &controls::ExposureTime,\n> + ControlInfo(1, 66666, static_cast(defaultExposureTime.get())) },\n> { &controls::AnalogueGainMode,\n> ControlInfo(static_cast(controls::AnalogueGainModeAuto),\n> static_cast(controls::AnalogueGainModeManual),\n> static_cast(controls::AnalogueGainModeAuto)) },\n> - { &controls::AnalogueGain, ControlInfo(1.0f, 16.0f) },\n> + { &controls::AnalogueGain, ControlInfo(1.0f, 16.0f, 1.0f) },\n> { &controls::AeMeteringMode, ControlInfo(controls::AeMeteringModeValues) },\n> { &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },\n> { &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) },\n> @@ -80,7 +81,9 @@ const ControlInfoMap::Map ipaControls{\n> { &controls::HdrMode, ControlInfo(controls::HdrModeValues) },\n> { &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },\n> { &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },\n> - { &controls::FrameDurationLimits, ControlInfo(INT64_C(33333), INT64_C(120000)) },\n> + { &controls::FrameDurationLimits,\n> + ControlInfo(INT64_C(33333), INT64_C(120000),\n> + static_cast(defaultMinFrameDuration.get())) },\n> { &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) },\n> { &controls::rpi::StatsOutputEnable, ControlInfo(false, true, false) },\n> };\n> @@ -259,15 +262,18 @@ int32_t IpaBase::configure(const IPACameraSensorInfo &sensorInfo, const ConfigPa\n> ControlInfoMap::Map ctrlMap = ipaControls;\n> ctrlMap[&controls::FrameDurationLimits] =\n> ControlInfo(static_cast(mode_.minFrameDuration.get()),\n> - static_cast(mode_.maxFrameDuration.get()));\n> + static_cast(mode_.maxFrameDuration.get()),\n> + static_cast(defaultMinFrameDuration.get()));\n> \n> ctrlMap[&controls::AnalogueGain] =\n> ControlInfo(static_cast(mode_.minAnalogueGain),\n> - static_cast(mode_.maxAnalogueGain));\n> + static_cast(mode_.maxAnalogueGain),\n> + static_cast(defaultAnalogueGain));\n> \n> ctrlMap[&controls::ExposureTime] =\n> ControlInfo(static_cast(mode_.minExposureTime.get()),\n> - static_cast(mode_.maxExposureTime.get()));\n> + static_cast(mode_.maxExposureTime.get()),\n> + static_cast(defaultExposureTime.get()));\n> \n> /* Declare colour processing related controls for non-mono sensors. */\n> if (!monoSensor_)\n> -- \n> 2.43.0\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 A39AFC32F5\n\tfor ;\n\tTue, 11 Feb 2025 12:22:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BBA1268621;\n\tTue, 11 Feb 2025 13:22:48 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0FA38685BE\n\tfor ;\n\tTue, 11 Feb 2025 13:22:47 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id CE95B6DF;\n\tTue, 11 Feb 2025 13:21:29 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"mtNsDqCH\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1739276489;\n\tbh=ExYpSMi1NCcjyRRi0QkFBpZVp0DSNzesacvf30xPkYg=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=mtNsDqCH/f1UnyRv1oYgkt5/gGN/WsGq2dhWjfEEK9e/VAiVxsnJk2q0lmC561Tpa\n\tWKrQwYTy/KsbmAtWQf/K9/ukfOyUCeUHMn/W53zJiDW2faVrLBzli93yGeqbwUK6jP\n\tAtkVVig6vHnjCNQFG9j3I+x/K8zY1EXZcuvinJqc=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20250211102327.55021-1-naush@raspberrypi.com>","References":"<20250211102327.55021-1-naush@raspberrypi.com>","Subject":"Re: [PATCH v3] ipa: rpi: Apply default ControlInfo values for sensor\n\tcontrols","From":"Kieran Bingham ","Cc":"Naushir Patuck ,\n\tDavid Plowman ","To":"Naushir Patuck ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Tue, 11 Feb 2025 12:22:43 +0000","Message-ID":"<173927656369.2293020.2111425735382346526@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","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":33332,"web_url":"https://patchwork.libcamera.org/comment/33332/","msgid":"","date":"2025-02-11T12:28:44","subject":"Re: [PATCH v3] ipa: rpi: Apply default ControlInfo values for sensor\n\tcontrols","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Kieran,\n\nOn Tue, 11 Feb 2025 at 12:22, Kieran Bingham\n wrote:\n>\n> Quoting Naushir Patuck (2025-02-11 10:20:02)\n> > The existing IPA initialisation code did not set default values for\n> > some sensor related controls. This caused a crash using libcamerify\n> > when the it was trying to access the default value for\n> > controls::FrameDurationLimits as part of a recent change.\n> >\n> > Ensure controls::FrameDurationLimits, controls::AnalogueGain and\n> > controls::ExposureTime advertise default values along with the existing\n> > min/max values. The default is set to the defaults defined in the IPA\n> > set during initialisation.\n> >\n> > Bug: https://bugs.libcamera.org/show_bug.cgi?id=253\n> > Signed-off-by: Naushir Patuck \n> > Reviewed-by: David Plowman \n>\n> Reviewed-by: Kieran Bingham \n>\n> If you're ready with this I can merge it.\n>\n> I saw some githubs fly by about the default values of AWB/AGC enable.\n> Should they be set to report they are enabled by default too?\n\nActually some of those have already been fixed upstream by Paul, but I\nhaven't rebased our downstream tree with the commits yet. There are\nstill others that I have questions above.\n\n>\n> (Let's treat that as a separate topic from this patch though).\n\nAgreed, feel free to merge this patch now.\n\nRegards,\nNaush\n\n>\n>\n>\n> > ---\n> >\n> > Changes in v3:\n> >\n> > Need to apply the same default in the init phase since the test in the\n> > V4L2CameraProxy::setFmtFromConfig() happens before calling ipa::configure().\n> >\n> > ---\n> > src/ipa/rpi/common/ipa_base.cpp | 18 ++++++++++++------\n> > 1 file changed, 12 insertions(+), 6 deletions(-)\n> >\n> > diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp\n> > index bd3c22000df5..5d2c6d40da3e 100644\n> > --- a/src/ipa/rpi/common/ipa_base.cpp\n> > +++ b/src/ipa/rpi/common/ipa_base.cpp\n> > @@ -61,12 +61,13 @@ const ControlInfoMap::Map ipaControls{\n> > ControlInfo(static_cast(controls::ExposureTimeModeAuto),\n> > static_cast(controls::ExposureTimeModeManual),\n> > static_cast(controls::ExposureTimeModeAuto)) },\n> > - { &controls::ExposureTime, ControlInfo(0, 66666) },\n> > + { &controls::ExposureTime,\n> > + ControlInfo(1, 66666, static_cast(defaultExposureTime.get())) },\n> > { &controls::AnalogueGainMode,\n> > ControlInfo(static_cast(controls::AnalogueGainModeAuto),\n> > static_cast(controls::AnalogueGainModeManual),\n> > static_cast(controls::AnalogueGainModeAuto)) },\n> > - { &controls::AnalogueGain, ControlInfo(1.0f, 16.0f) },\n> > + { &controls::AnalogueGain, ControlInfo(1.0f, 16.0f, 1.0f) },\n> > { &controls::AeMeteringMode, ControlInfo(controls::AeMeteringModeValues) },\n> > { &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },\n> > { &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) },\n> > @@ -80,7 +81,9 @@ const ControlInfoMap::Map ipaControls{\n> > { &controls::HdrMode, ControlInfo(controls::HdrModeValues) },\n> > { &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },\n> > { &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },\n> > - { &controls::FrameDurationLimits, ControlInfo(INT64_C(33333), INT64_C(120000)) },\n> > + { &controls::FrameDurationLimits,\n> > + ControlInfo(INT64_C(33333), INT64_C(120000),\n> > + static_cast(defaultMinFrameDuration.get())) },\n> > { &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) },\n> > { &controls::rpi::StatsOutputEnable, ControlInfo(false, true, false) },\n> > };\n> > @@ -259,15 +262,18 @@ int32_t IpaBase::configure(const IPACameraSensorInfo &sensorInfo, const ConfigPa\n> > ControlInfoMap::Map ctrlMap = ipaControls;\n> > ctrlMap[&controls::FrameDurationLimits] =\n> > ControlInfo(static_cast(mode_.minFrameDuration.get()),\n> > - static_cast(mode_.maxFrameDuration.get()));\n> > + static_cast(mode_.maxFrameDuration.get()),\n> > + static_cast(defaultMinFrameDuration.get()));\n> >\n> > ctrlMap[&controls::AnalogueGain] =\n> > ControlInfo(static_cast(mode_.minAnalogueGain),\n> > - static_cast(mode_.maxAnalogueGain));\n> > + static_cast(mode_.maxAnalogueGain),\n> > + static_cast(defaultAnalogueGain));\n> >\n> > ctrlMap[&controls::ExposureTime] =\n> > ControlInfo(static_cast(mode_.minExposureTime.get()),\n> > - static_cast(mode_.maxExposureTime.get()));\n> > + static_cast(mode_.maxExposureTime.get()),\n> > + static_cast(defaultExposureTime.get()));\n> >\n> > /* Declare colour processing related controls for non-mono sensors. */\n> > if (!monoSensor_)\n> > --\n> > 2.43.0\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 D2E2ABD1F1\n\tfor ;\n\tTue, 11 Feb 2025 12:29:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C82F868626;\n\tTue, 11 Feb 2025 13:29:22 +0100 (CET)","from mail-yw1-x1136.google.com (mail-yw1-x1136.google.com\n\t[IPv6:2607:f8b0:4864:20::1136])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 838A5685BE\n\tfor ;\n\tTue, 11 Feb 2025 13:29:20 +0100 (CET)","by mail-yw1-x1136.google.com with SMTP id\n\t00721157ae682-6f4a9164e67so5072957b3.1\n\tfor ;\n\tTue, 11 Feb 2025 04:29:20 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"ViqDBf7F\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1739276959; x=1739881759;\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=6GszI1NApZ8BCNvRUNCx44E7blak5TrLaaaxprTmpu0=;\n\tb=ViqDBf7Fj3YAja4oPU/o9fjDDTZXLkFxxrbhhgfKc2qUuCAVLKSTJtEMCxIESgq/hQ\n\t+AWHqq1fPPrLeS+Pja8U0EIsQQIjRnn1rBr5kplheejqI+GRGcB+lyAq2s/WC97gLGvz\n\ty1fIxdsV6ArrYmpfJy8YPgBK3h5+JTbP83A3G9PvUzyoSbBa14R9PLFZ0dpvL3I9yYD6\n\tyQl7A0uk6dd33VU4affgLlaqnB68vu/XqTSYbjdtLH0eVd2QmpGm7BTV9VGO9gj7WYNV\n\tkCRY4SfEfS9Iy7aYdB2fWffjb2ddHwjNHZWUal+scll+kw3YSASU0lUyKAKTQsBCTUKP\n\tnEWQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1739276959; x=1739881759;\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=6GszI1NApZ8BCNvRUNCx44E7blak5TrLaaaxprTmpu0=;\n\tb=YKr6hoIgPixYLglDx0K3gt5jjN1LIH8K1qhLcCEbPaouIpCrvJx5UIjadvXbuhf1Zs\n\tiVPEVI5gnNYhlB2H71lRvtB4YEqIj9xioCkAT99A47NGPscImxQ4z+4O7giimAKX/s6g\n\tlTDIhdAhDJzpcE14wqh85Y/9+OcovBwTuhV9TPJYql8ZXMqOMmelAUcb9m+rwIwKMXpe\n\th3ChOlwwOdnCzoqGNC2ktlZ1nncO+6Er38jHOPt/5VxTYQSRb85bDxD7uTK721gYvK5G\n\t7Pi8lVu5NE7zUb2cDZp1qHMc0jGYzFb6iazso8dSgSBlfvTr8LtZDTYnzzQ7tt5s/d7X\n\tfFMg==","X-Gm-Message-State":"AOJu0YyPlNjZpIl1AKWvgKTJPduZYXLMUEs76K/H3IGwwNbseU7mOtVY\n\tRKJKcsf2kDEMqa8xNq5cdHwrZhY54TXacZh8tWtjtH9SZenVxglzyWXR38z4pAnKQhEVI0512aJ\n\tCxgDXDEM/iB6WU6RFDs2acfNgKn6pCPGRGcy7kMZCzgMUyVdKQe0=","X-Gm-Gg":"ASbGncuWO0Ghqo244wcPwqJ2eAK1RWX09gD+PL7mAa0nSmdjOGfQR9W8EjXX5dl7Zrr\n\tSKqKaHgbQINNusWM2toJDRI16eYbgjssEhHMh2IBF5fhTsDrcS6LbOOsOQacjCh9k03NCE8vwwm\n\tmEIPkoGuZsK/mgdBGwkMukWc3T+OhX","X-Google-Smtp-Source":"AGHT+IEWq2l22VdNEWksIO4XOL8vYrxFZ6bWxMm3wgT6rvifel5qmEWFVtcj6mzfeq8UEI7fIA8jzXUVjLPOof1cMEs=","X-Received":"by 2002:a05:690c:d94:b0:6ea:4d3f:df8c with SMTP id\n\t00721157ae682-6f9b27ff034mr63988567b3.1.1739276959326;\n\tTue, 11 Feb 2025 04:29:19 -0800 (PST)","MIME-Version":"1.0","References":"<20250211102327.55021-1-naush@raspberrypi.com>\n\t<173927656369.2293020.2111425735382346526@ping.linuxembedded.co.uk>","In-Reply-To":"<173927656369.2293020.2111425735382346526@ping.linuxembedded.co.uk>","From":"Naushir Patuck ","Date":"Tue, 11 Feb 2025 12:28:44 +0000","X-Gm-Features":"AWEUYZlKukDI24-EFknDEptIU3QHaWFcs264uSdpwdxR6g69ZT4iRCOSYzZpvPY","Message-ID":"","Subject":"Re: [PATCH v3] ipa: rpi: Apply default ControlInfo values for sensor\n\tcontrols","To":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org, \n\tDavid Plowman ","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\" "}}]