[{"id":27229,"web_url":"https://patchwork.libcamera.org/comment/27229/","msgid":"<20230602155154.GM26944@pendragon.ideasonboard.com>","date":"2023-06-02T15:51:54","subject":"Re: [libcamera-devel] [PATCH v2 2/2] ipa: rpi: Handle controls for\n\tmono variant sensors","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 Fri, Jun 02, 2023 at 02:23:58PM +0100, Naushir Patuck wrote:\n> Do not advertise colour related controls (i.e. [A]WB, colour saturation)\n> in the ControlInfoMap of available controls returned out to the\n> application.\n> \n> Silently ignore these controls in the control handler in case applications\n> don't use the advertised ControlInfoMap to validate controls.\n> \n> As a drive-by, don't advertise controls::ColourCorrectionMatrix in the\n> ControlInfoMap as it is not handled by the IPA.\n> \n> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>\n> Reviewed-by: David Plowman <david.plowman@raspberrypi.com>\n> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> ---\n>  src/ipa/rpi/common/ipa_base.cpp | 35 ++++++++++++++++++++++++++++-----\n>  src/ipa/rpi/common/ipa_base.h   |  1 +\n>  2 files changed, 31 insertions(+), 5 deletions(-)\n> \n> diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp\n> index db7a0eb3a1ca..813e01fa5cfd 100644\n> --- a/src/ipa/rpi/common/ipa_base.cpp\n> +++ b/src/ipa/rpi/common/ipa_base.cpp\n> @@ -12,6 +12,7 @@\n>  #include <libcamera/base/log.h>\n>  #include <libcamera/base/span.h>\n>  #include <libcamera/control_ids.h>\n> +#include <libcamera/property_ids.h>\n>  \n>  #include \"controller/af_algorithm.h\"\n>  #include \"controller/af_status.h\"\n> @@ -60,19 +61,22 @@ const ControlInfoMap::Map ipaControls{\n>  \t{ &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },\n>  \t{ &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) },\n>  \t{ &controls::ExposureValue, ControlInfo(-8.0f, 8.0f, 0.0f) },\n> -\t{ &controls::AwbEnable, ControlInfo(false, true) },\n> -\t{ &controls::ColourGains, ControlInfo(0.0f, 32.0f) },\n> -\t{ &controls::AwbMode, ControlInfo(controls::AwbModeValues) },\n>  \t{ &controls::Brightness, ControlInfo(-1.0f, 1.0f, 0.0f) },\n>  \t{ &controls::Contrast, ControlInfo(0.0f, 32.0f, 1.0f) },\n> -\t{ &controls::Saturation, ControlInfo(0.0f, 32.0f, 1.0f) },\n>  \t{ &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },\n> -\t{ &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) },\n>  \t{ &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },\n>  \t{ &controls::FrameDurationLimits, ControlInfo(INT64_C(33333), INT64_C(120000)) },\n>  \t{ &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) }\n>  };\n>  \n> +/* IPA controls handled conditionally, if the sensor is not mono */\n> +const ControlInfoMap::Map ipaColourControls{\n> +\t{ &controls::AwbEnable, ControlInfo(false, true) },\n> +\t{ &controls::AwbMode, ControlInfo(controls::AwbModeValues) },\n> +\t{ &controls::ColourGains, ControlInfo(0.0f, 32.0f) },\n> +\t{ &controls::Saturation, ControlInfo(0.0f, 32.0f, 1.0f) },\n> +};\n> +\n>  /* IPA controls handled conditionally, if the lens has a focus control */\n>  const ControlInfoMap::Map ipaAfControls{\n>  \t{ &controls::AfMode, ControlInfo(controls::AfModeValues) },\n> @@ -220,6 +224,11 @@ int32_t IpaBase::configure(const IPACameraSensorInfo &sensorInfo, const ConfigPa\n>  \t\tControlInfo(static_cast<int32_t>(mode_.minShutter.get<std::micro>()),\n>  \t\t\t    static_cast<int32_t>(mode_.maxShutter.get<std::micro>()));\n>  \n> +\t/* Declare colour processing related controls for non-mono sensors. */\n> +\tmonoSensor_ = sensorInfo.cfaPattern == properties::draft::ColorFilterArrangementEnum::MONO;\n> +\tif (!monoSensor_)\n> +\t\tctrlMap.merge(ControlInfoMap::Map(ipaColourControls));\n> +\n\nThis means that the camera won't report the colour-related controls\nuntil it gets configured. Could we fix that by passing the IPASensorInfo\nto the init() function as well ?\n\n>  \t/* Declare Autofocus controls, only if we have a controllable lens */\n>  \tif (lensPresent_)\n>  \t\tctrlMap.merge(ControlInfoMap::Map(ipaAfControls));\n> @@ -780,6 +789,10 @@ void IpaBase::applyControls(const ControlList &controls)\n>  \t\t}\n>  \n>  \t\tcase controls::AWB_ENABLE: {\n> +\t\t\t/* Silently ignore this control for a mono sensor. */\n> +\t\t\tif (monoSensor_)\n> +\t\t\t\tbreak;\n> +\n>  \t\t\tRPiController::AwbAlgorithm *awb = dynamic_cast<RPiController::AwbAlgorithm *>(\n>  \t\t\t\tcontroller_.getAlgorithm(\"awb\"));\n>  \t\t\tif (!awb) {\n> @@ -799,6 +812,10 @@ void IpaBase::applyControls(const ControlList &controls)\n>  \t\t}\n>  \n>  \t\tcase controls::AWB_MODE: {\n> +\t\t\t/* Silently ignore this control for a mono sensor. */\n> +\t\t\tif (monoSensor_)\n> +\t\t\t\tbreak;\n> +\n>  \t\t\tRPiController::AwbAlgorithm *awb = dynamic_cast<RPiController::AwbAlgorithm *>(\n>  \t\t\t\tcontroller_.getAlgorithm(\"awb\"));\n>  \t\t\tif (!awb) {\n> @@ -819,6 +836,10 @@ void IpaBase::applyControls(const ControlList &controls)\n>  \t\t}\n>  \n>  \t\tcase controls::COLOUR_GAINS: {\n> +\t\t\t/* Silently ignore this control for a mono sensor. */\n> +\t\t\tif (monoSensor_)\n> +\t\t\t\tbreak;\n> +\n>  \t\t\tauto gains = ctrl.second.get<Span<const float>>();\n>  \t\t\tRPiController::AwbAlgorithm *awb = dynamic_cast<RPiController::AwbAlgorithm *>(\n>  \t\t\t\tcontroller_.getAlgorithm(\"awb\"));\n> @@ -867,6 +888,10 @@ void IpaBase::applyControls(const ControlList &controls)\n>  \t\t}\n>  \n>  \t\tcase controls::SATURATION: {\n> +\t\t\t/* Silently ignore this control for a mono sensor. */\n> +\t\t\tif (monoSensor_)\n> +\t\t\t\tbreak;\n> +\n>  \t\t\tRPiController::CcmAlgorithm *ccm = dynamic_cast<RPiController::CcmAlgorithm *>(\n>  \t\t\t\tcontroller_.getAlgorithm(\"ccm\"));\n>  \t\t\tif (!ccm) {\n> diff --git a/src/ipa/rpi/common/ipa_base.h b/src/ipa/rpi/common/ipa_base.h\n> index 6f9c46bb16b1..39d00760d012 100644\n> --- a/src/ipa/rpi/common/ipa_base.h\n> +++ b/src/ipa/rpi/common/ipa_base.h\n> @@ -87,6 +87,7 @@ private:\n>  \tstd::map<unsigned int, MappedFrameBuffer> buffers_;\n>  \n>  \tbool lensPresent_;\n> +\tbool monoSensor_;\n>  \tControlList libcameraMetadata_;\n>  \n>  \tstd::array<RPiController::Metadata, numMetadataContexts> rpiMetadata_;","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 11852C31E9\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri,  2 Jun 2023 15:51:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7779562754;\n\tFri,  2 Jun 2023 17:51:57 +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 AB97D626FA\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri,  2 Jun 2023 17:51:56 +0200 (CEST)","from pendragon.ideasonboard.com (om126156168104.26.openmobile.ne.jp\n\t[126.156.168.104])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 09731844;\n\tFri,  2 Jun 2023 17:51:32 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1685721117;\n\tbh=vHcA5lXvfzVTkio0TPFkrw8OmaJbPFRQ2RyI9FPX4eg=;\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=fTtsPAZ+VdWrBaoeha1dhyMQTeHLz/lmEjMS8/8gIoC7SHeY+9oFSHoi6vJdb8PQt\n\tU7Uzfb4qJdfLF0+5GhWmaOf69FuVymrOujhFglg8t0g88OIqhDpej+UbW70zVAzOxh\n\tr5DQ2DaCLVNDCsjinTBQYVFyIMWUhsDOnrq1iSmk2+DxYVlsR8jxBMzaHX2GCg/Lrw\n\ti+7N7cMk/bwWADsZTg/zLotMHxeg37RKR1ul5d6o5qe3eHHYiBI/JieVo+np/DwZ0W\n\tuNYXFEyx7JE+wnR/xndbrDFg/dhyDPf90Od85Bv15wd+FT92XgIua7SGF8Z1YkrEyz\n\tmtyx1V9MI2PTg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1685721093;\n\tbh=vHcA5lXvfzVTkio0TPFkrw8OmaJbPFRQ2RyI9FPX4eg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Xb+CqgmcC5hJQKwxhXmLsF+N7C9XXNZSoxhNlCBQ7NxYtctoTCjijX7K6xCdgXoSu\n\tPgfgcFlEhFYB358QxqbzhZuTh/YPj8cIL8tIvjBWD0vWq7t0Boe0Il1Yc2U1A82ez0\n\txz/isH5vNAFV/pihRleR5MWRCamYnLlDRJZ4pySY="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"Xb+Cqgmc\"; dkim-atps=neutral","Date":"Fri, 2 Jun 2023 18:51:54 +0300","To":"Naushir Patuck <naush@raspberrypi.com>","Message-ID":"<20230602155154.GM26944@pendragon.ideasonboard.com>","References":"<20230602132358.16314-1-naush@raspberrypi.com>\n\t<20230602132358.16314-3-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20230602132358.16314-3-naush@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] ipa: rpi: Handle controls for\n\tmono variant sensors","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","From":"Laurent Pinchart via libcamera-devel\n\t<libcamera-devel@lists.libcamera.org>","Reply-To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":27242,"web_url":"https://patchwork.libcamera.org/comment/27242/","msgid":"<CAEmqJPq9xxq=f3+DyBoNrZhT4_RY2ZHDkieXrumVkz70mzE0og@mail.gmail.com>","date":"2023-06-05T07:20:28","subject":"Re: [libcamera-devel] [PATCH v2 2/2] ipa: rpi: Handle controls for\n\tmono variant sensors","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Laurent,\n\nOn Fri, 2 Jun 2023 at 16:51, Laurent Pinchart\n<laurent.pinchart@ideasonboard.com> wrote:\n>\n> Hi Naush,\n>\n> Thank you for the patch.\n>\n> On Fri, Jun 02, 2023 at 02:23:58PM +0100, Naushir Patuck wrote:\n> > Do not advertise colour related controls (i.e. [A]WB, colour saturation)\n> > in the ControlInfoMap of available controls returned out to the\n> > application.\n> >\n> > Silently ignore these controls in the control handler in case applications\n> > don't use the advertised ControlInfoMap to validate controls.\n> >\n> > As a drive-by, don't advertise controls::ColourCorrectionMatrix in the\n> > ControlInfoMap as it is not handled by the IPA.\n> >\n> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>\n> > Reviewed-by: David Plowman <david.plowman@raspberrypi.com>\n> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> > ---\n> >  src/ipa/rpi/common/ipa_base.cpp | 35 ++++++++++++++++++++++++++++-----\n> >  src/ipa/rpi/common/ipa_base.h   |  1 +\n> >  2 files changed, 31 insertions(+), 5 deletions(-)\n> >\n> > diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp\n> > index db7a0eb3a1ca..813e01fa5cfd 100644\n> > --- a/src/ipa/rpi/common/ipa_base.cpp\n> > +++ b/src/ipa/rpi/common/ipa_base.cpp\n> > @@ -12,6 +12,7 @@\n> >  #include <libcamera/base/log.h>\n> >  #include <libcamera/base/span.h>\n> >  #include <libcamera/control_ids.h>\n> > +#include <libcamera/property_ids.h>\n> >\n> >  #include \"controller/af_algorithm.h\"\n> >  #include \"controller/af_status.h\"\n> > @@ -60,19 +61,22 @@ const ControlInfoMap::Map ipaControls{\n> >       { &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },\n> >       { &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) },\n> >       { &controls::ExposureValue, ControlInfo(-8.0f, 8.0f, 0.0f) },\n> > -     { &controls::AwbEnable, ControlInfo(false, true) },\n> > -     { &controls::ColourGains, ControlInfo(0.0f, 32.0f) },\n> > -     { &controls::AwbMode, ControlInfo(controls::AwbModeValues) },\n> >       { &controls::Brightness, ControlInfo(-1.0f, 1.0f, 0.0f) },\n> >       { &controls::Contrast, ControlInfo(0.0f, 32.0f, 1.0f) },\n> > -     { &controls::Saturation, ControlInfo(0.0f, 32.0f, 1.0f) },\n> >       { &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },\n> > -     { &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) },\n> >       { &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },\n> >       { &controls::FrameDurationLimits, ControlInfo(INT64_C(33333), INT64_C(120000)) },\n> >       { &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) }\n> >  };\n> >\n> > +/* IPA controls handled conditionally, if the sensor is not mono */\n> > +const ControlInfoMap::Map ipaColourControls{\n> > +     { &controls::AwbEnable, ControlInfo(false, true) },\n> > +     { &controls::AwbMode, ControlInfo(controls::AwbModeValues) },\n> > +     { &controls::ColourGains, ControlInfo(0.0f, 32.0f) },\n> > +     { &controls::Saturation, ControlInfo(0.0f, 32.0f, 1.0f) },\n> > +};\n> > +\n> >  /* IPA controls handled conditionally, if the lens has a focus control */\n> >  const ControlInfoMap::Map ipaAfControls{\n> >       { &controls::AfMode, ControlInfo(controls::AfModeValues) },\n> > @@ -220,6 +224,11 @@ int32_t IpaBase::configure(const IPACameraSensorInfo &sensorInfo, const ConfigPa\n> >               ControlInfo(static_cast<int32_t>(mode_.minShutter.get<std::micro>()),\n> >                           static_cast<int32_t>(mode_.maxShutter.get<std::micro>()));\n> >\n> > +     /* Declare colour processing related controls for non-mono sensors. */\n> > +     monoSensor_ = sensorInfo.cfaPattern == properties::draft::ColorFilterArrangementEnum::MONO;\n> > +     if (!monoSensor_)\n> > +             ctrlMap.merge(ControlInfoMap::Map(ipaColourControls));\n> > +\n>\n> This means that the camera won't report the colour-related controls\n> until it gets configured. Could we fix that by passing the IPASensorInfo\n> to the init() function as well ?\n\nYes this is true.  I've always felt that the ControlInfoMap should not be\nadvertised at all during ipa->init(), since some controls limits (e.g.\nAnalogueGain\nand ExposureTime) have invalid min/max/default values.  They only get correct\nvalues set after ipa->configure().\n\nRather than passing IPASensorInfo to init(), I would probably prefer to entirely\nremove setting up the ControlInfoMap there.  What are your thoughts?\n\nRegards,\nNaush\n\n>\n> >       /* Declare Autofocus controls, only if we have a controllable lens */\n> >       if (lensPresent_)\n> >               ctrlMap.merge(ControlInfoMap::Map(ipaAfControls));\n> > @@ -780,6 +789,10 @@ void IpaBase::applyControls(const ControlList &controls)\n> >               }\n> >\n> >               case controls::AWB_ENABLE: {\n> > +                     /* Silently ignore this control for a mono sensor. */\n> > +                     if (monoSensor_)\n> > +                             break;\n> > +\n> >                       RPiController::AwbAlgorithm *awb = dynamic_cast<RPiController::AwbAlgorithm *>(\n> >                               controller_.getAlgorithm(\"awb\"));\n> >                       if (!awb) {\n> > @@ -799,6 +812,10 @@ void IpaBase::applyControls(const ControlList &controls)\n> >               }\n> >\n> >               case controls::AWB_MODE: {\n> > +                     /* Silently ignore this control for a mono sensor. */\n> > +                     if (monoSensor_)\n> > +                             break;\n> > +\n> >                       RPiController::AwbAlgorithm *awb = dynamic_cast<RPiController::AwbAlgorithm *>(\n> >                               controller_.getAlgorithm(\"awb\"));\n> >                       if (!awb) {\n> > @@ -819,6 +836,10 @@ void IpaBase::applyControls(const ControlList &controls)\n> >               }\n> >\n> >               case controls::COLOUR_GAINS: {\n> > +                     /* Silently ignore this control for a mono sensor. */\n> > +                     if (monoSensor_)\n> > +                             break;\n> > +\n> >                       auto gains = ctrl.second.get<Span<const float>>();\n> >                       RPiController::AwbAlgorithm *awb = dynamic_cast<RPiController::AwbAlgorithm *>(\n> >                               controller_.getAlgorithm(\"awb\"));\n> > @@ -867,6 +888,10 @@ void IpaBase::applyControls(const ControlList &controls)\n> >               }\n> >\n> >               case controls::SATURATION: {\n> > +                     /* Silently ignore this control for a mono sensor. */\n> > +                     if (monoSensor_)\n> > +                             break;\n> > +\n> >                       RPiController::CcmAlgorithm *ccm = dynamic_cast<RPiController::CcmAlgorithm *>(\n> >                               controller_.getAlgorithm(\"ccm\"));\n> >                       if (!ccm) {\n> > diff --git a/src/ipa/rpi/common/ipa_base.h b/src/ipa/rpi/common/ipa_base.h\n> > index 6f9c46bb16b1..39d00760d012 100644\n> > --- a/src/ipa/rpi/common/ipa_base.h\n> > +++ b/src/ipa/rpi/common/ipa_base.h\n> > @@ -87,6 +87,7 @@ private:\n> >       std::map<unsigned int, MappedFrameBuffer> buffers_;\n> >\n> >       bool lensPresent_;\n> > +     bool monoSensor_;\n> >       ControlList libcameraMetadata_;\n> >\n> >       std::array<RPiController::Metadata, numMetadataContexts> rpiMetadata_;\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id B81A9C3200\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon,  5 Jun 2023 07:20:47 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E3C206287D;\n\tMon,  5 Jun 2023 09:20:46 +0200 (CEST)","from mail-yw1-x1132.google.com (mail-yw1-x1132.google.com\n\t[IPv6:2607:f8b0:4864:20::1132])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8AC7660579\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon,  5 Jun 2023 09:20:45 +0200 (CEST)","by mail-yw1-x1132.google.com with SMTP id\n\t00721157ae682-568f9caff33so43373827b3.2\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 05 Jun 2023 00:20:45 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1685949647;\n\tbh=9qlKDwpuBNph7YLKfKFmoWkQiT/soQpnXOmxnFqd/vM=;\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=VH06t9On2Jf7hjj6B1orsc6yWbjI6QLjQlHMqZELDWDn/2UGymhCh0zCwlVpVbsM2\n\tni7VvIUFUA+RTJXukp/9UeCXxzOwZkViiUQH2Qv+coSynQYmC6XGIy/0VSv2KG+HGk\n\t3MIJuIp+8larL+J1s9crMX67AZ76CBnA1waHCM5SZagnit6ag1vmjSsXBe4oQNiu6K\n\t0fKFLASLOIRwjRlbPNHFAGh5JyudF000ITA+l2Yc9XXWzHUOX7yM+IncQN0toq6PMS\n\tBuVhzLFPiE9HAXEzp8Sx4pC/XTApcg2IoNFPTaJPCLE0Azhn57hb6VQ1BJ9eXgVXu6\n\tEQHuCDg2Ldu0g==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1685949644; x=1688541644;\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=wT2Jbtd/0MOfC1gyyA/Hju4QIBd0InblIeZX3Z5ZU4A=;\n\tb=snBJvsn6nqz6iCKT5Dvm74OsXGyBDh8CB1DZXujknX2Auplyl6TppZZyXTu5UpZvlp\n\tEZHrFnLDDOUg+CrG8a9V5H1944KO6aOkVvAizNp8kdqJO74sUmDNVboRxmdL2twBwnAL\n\tHE3bnvwJYwN5eYSoXRuVUFWx/Js7QyMlLrRHAlFEmemZpzz6dLl/TYWlyP0tJJGGtZUl\n\t7yhFPOgPaBHcK10AlM1397jQHitCBXB7rhMfS+GepPwd2Hze6hvmthzS2+GlKP2qWLBA\n\tIroexEMV1t0rLaAhocHaPOAXjdyLFf2NC2p3KgiSL66UPlKM2MgsGHrYJfpqXfVFUPa3\n\tQ+NA=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"snBJvsn6\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20221208; t=1685949644; x=1688541644;\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=wT2Jbtd/0MOfC1gyyA/Hju4QIBd0InblIeZX3Z5ZU4A=;\n\tb=P40nsvQL+QbcKZ17DzmyHtcEDyiCG7xTdQZgSNO46JPFhpGoSjSsMai5Hxp50C7wA1\n\tnLh6pXiAqtM/1NvGrLO9Pm2PYx2AQKY0rUgKzEMD1+WQ1kMw3HwmTFhEcL4DHbHWGHIB\n\tTVfvi4bJn3hclGnUa9NINZ8Lyh3O7pshBthUXgIJxgEnHr3omAiZ1Hd52yB8bw+W0m4X\n\tlyGc9AKCISd8Xk0AsKx6N1/KhyHQvdM1eEf/NGkYyFewBDW0OceTkJNoJqQmrY4D11a0\n\tiI0iWB40AarRY6wUMskVxTdfpVRydSIqw+CCGARrrIq1S3YlIVBGgEBafCCG1Pp8v9tj\n\t1tFw==","X-Gm-Message-State":"AC+VfDybqiIvCbZbk/pcOqoyHN/ENJoYzxzc7NZ8ONmV+s0gEfOaCaqR\n\tiJnpCdI7qrmPfeJbJ6zyNkNXniHzAW0DrR73zzGsfg==","X-Google-Smtp-Source":"ACHHUZ5I/5lQlbntROoXky858mQp1xFb+ulmvlECyt5cykvdLGcKJz1a2iP2gRM1H1rhYRTfNWuH5hNvloKfB5dPRJA=","X-Received":"by 2002:a0d:df93:0:b0:55a:26cf:33e with SMTP id\n\ti141-20020a0ddf93000000b0055a26cf033emr7610821ywe.42.1685949644337;\n\tMon, 05 Jun 2023 00:20:44 -0700 (PDT)","MIME-Version":"1.0","References":"<20230602132358.16314-1-naush@raspberrypi.com>\n\t<20230602132358.16314-3-naush@raspberrypi.com>\n\t<20230602155154.GM26944@pendragon.ideasonboard.com>","In-Reply-To":"<20230602155154.GM26944@pendragon.ideasonboard.com>","Date":"Mon, 5 Jun 2023 08:20:28 +0100","Message-ID":"<CAEmqJPq9xxq=f3+DyBoNrZhT4_RY2ZHDkieXrumVkz70mzE0og@mail.gmail.com>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] ipa: rpi: Handle controls for\n\tmono variant sensors","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","From":"Naushir Patuck via libcamera-devel\n\t<libcamera-devel@lists.libcamera.org>","Reply-To":"Naushir Patuck <naush@raspberrypi.com>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":27245,"web_url":"https://patchwork.libcamera.org/comment/27245/","msgid":"<20230605074325.GH22604@pendragon.ideasonboard.com>","date":"2023-06-05T07:43:25","subject":"Re: [libcamera-devel] [PATCH v2 2/2] ipa: rpi: Handle controls for\n\tmono variant sensors","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nOn Mon, Jun 05, 2023 at 08:20:28AM +0100, Naushir Patuck wrote:\n> On Fri, 2 Jun 2023 at 16:51, Laurent Pinchart wrote:\n> > On Fri, Jun 02, 2023 at 02:23:58PM +0100, Naushir Patuck wrote:\n> > > Do not advertise colour related controls (i.e. [A]WB, colour saturation)\n> > > in the ControlInfoMap of available controls returned out to the\n> > > application.\n> > >\n> > > Silently ignore these controls in the control handler in case applications\n> > > don't use the advertised ControlInfoMap to validate controls.\n> > >\n> > > As a drive-by, don't advertise controls::ColourCorrectionMatrix in the\n> > > ControlInfoMap as it is not handled by the IPA.\n> > >\n> > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>\n> > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com>\n> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> > > ---\n> > >  src/ipa/rpi/common/ipa_base.cpp | 35 ++++++++++++++++++++++++++++-----\n> > >  src/ipa/rpi/common/ipa_base.h   |  1 +\n> > >  2 files changed, 31 insertions(+), 5 deletions(-)\n> > >\n> > > diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp\n> > > index db7a0eb3a1ca..813e01fa5cfd 100644\n> > > --- a/src/ipa/rpi/common/ipa_base.cpp\n> > > +++ b/src/ipa/rpi/common/ipa_base.cpp\n> > > @@ -12,6 +12,7 @@\n> > >  #include <libcamera/base/log.h>\n> > >  #include <libcamera/base/span.h>\n> > >  #include <libcamera/control_ids.h>\n> > > +#include <libcamera/property_ids.h>\n> > >\n> > >  #include \"controller/af_algorithm.h\"\n> > >  #include \"controller/af_status.h\"\n> > > @@ -60,19 +61,22 @@ const ControlInfoMap::Map ipaControls{\n> > >       { &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },\n> > >       { &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) },\n> > >       { &controls::ExposureValue, ControlInfo(-8.0f, 8.0f, 0.0f) },\n> > > -     { &controls::AwbEnable, ControlInfo(false, true) },\n> > > -     { &controls::ColourGains, ControlInfo(0.0f, 32.0f) },\n> > > -     { &controls::AwbMode, ControlInfo(controls::AwbModeValues) },\n> > >       { &controls::Brightness, ControlInfo(-1.0f, 1.0f, 0.0f) },\n> > >       { &controls::Contrast, ControlInfo(0.0f, 32.0f, 1.0f) },\n> > > -     { &controls::Saturation, ControlInfo(0.0f, 32.0f, 1.0f) },\n> > >       { &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },\n> > > -     { &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) },\n> > >       { &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },\n> > >       { &controls::FrameDurationLimits, ControlInfo(INT64_C(33333), INT64_C(120000)) },\n> > >       { &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) }\n> > >  };\n> > >\n> > > +/* IPA controls handled conditionally, if the sensor is not mono */\n> > > +const ControlInfoMap::Map ipaColourControls{\n> > > +     { &controls::AwbEnable, ControlInfo(false, true) },\n> > > +     { &controls::AwbMode, ControlInfo(controls::AwbModeValues) },\n> > > +     { &controls::ColourGains, ControlInfo(0.0f, 32.0f) },\n> > > +     { &controls::Saturation, ControlInfo(0.0f, 32.0f, 1.0f) },\n> > > +};\n> > > +\n> > >  /* IPA controls handled conditionally, if the lens has a focus control */\n> > >  const ControlInfoMap::Map ipaAfControls{\n> > >       { &controls::AfMode, ControlInfo(controls::AfModeValues) },\n> > > @@ -220,6 +224,11 @@ int32_t IpaBase::configure(const IPACameraSensorInfo &sensorInfo, const ConfigPa\n> > >               ControlInfo(static_cast<int32_t>(mode_.minShutter.get<std::micro>()),\n> > >                           static_cast<int32_t>(mode_.maxShutter.get<std::micro>()));\n> > >\n> > > +     /* Declare colour processing related controls for non-mono sensors. */\n> > > +     monoSensor_ = sensorInfo.cfaPattern == properties::draft::ColorFilterArrangementEnum::MONO;\n> > > +     if (!monoSensor_)\n> > > +             ctrlMap.merge(ControlInfoMap::Map(ipaColourControls));\n> > > +\n> >\n> > This means that the camera won't report the colour-related controls\n> > until it gets configured. Could we fix that by passing the IPASensorInfo\n> > to the init() function as well ?\n> \n> Yes this is true.  I've always felt that the ControlInfoMap should not be\n> advertised at all during ipa->init(), since some controls limits (e.g.\n> AnalogueGain and ExposureTime) have invalid min/max/default values.\n> They only get correct values set after ipa->configure().\n>\n> Rather than passing IPASensorInfo to init(), I would probably prefer to entirely\n> remove setting up the ControlInfoMap there.  What are your thoughts?\n\nThis is an issue we'll need to tackle, and I don't know yet how to do\nso. Not reporting properties and controls until after configure() is an\neasy option, but it would make it more difficult for applications to\nfigure out if a camera is suitable for their needs, or just adapt their\nbehaviour based on the camera features. I don't want to drop reporting\nsupported controls at init time completely until we have a proper,\nwell-thought replacement.\n\n> > >       /* Declare Autofocus controls, only if we have a controllable lens */\n> > >       if (lensPresent_)\n> > >               ctrlMap.merge(ControlInfoMap::Map(ipaAfControls));\n> > > @@ -780,6 +789,10 @@ void IpaBase::applyControls(const ControlList &controls)\n> > >               }\n> > >\n> > >               case controls::AWB_ENABLE: {\n> > > +                     /* Silently ignore this control for a mono sensor. */\n> > > +                     if (monoSensor_)\n> > > +                             break;\n> > > +\n> > >                       RPiController::AwbAlgorithm *awb = dynamic_cast<RPiController::AwbAlgorithm *>(\n> > >                               controller_.getAlgorithm(\"awb\"));\n> > >                       if (!awb) {\n> > > @@ -799,6 +812,10 @@ void IpaBase::applyControls(const ControlList &controls)\n> > >               }\n> > >\n> > >               case controls::AWB_MODE: {\n> > > +                     /* Silently ignore this control for a mono sensor. */\n> > > +                     if (monoSensor_)\n> > > +                             break;\n> > > +\n> > >                       RPiController::AwbAlgorithm *awb = dynamic_cast<RPiController::AwbAlgorithm *>(\n> > >                               controller_.getAlgorithm(\"awb\"));\n> > >                       if (!awb) {\n> > > @@ -819,6 +836,10 @@ void IpaBase::applyControls(const ControlList &controls)\n> > >               }\n> > >\n> > >               case controls::COLOUR_GAINS: {\n> > > +                     /* Silently ignore this control for a mono sensor. */\n> > > +                     if (monoSensor_)\n> > > +                             break;\n> > > +\n> > >                       auto gains = ctrl.second.get<Span<const float>>();\n> > >                       RPiController::AwbAlgorithm *awb = dynamic_cast<RPiController::AwbAlgorithm *>(\n> > >                               controller_.getAlgorithm(\"awb\"));\n> > > @@ -867,6 +888,10 @@ void IpaBase::applyControls(const ControlList &controls)\n> > >               }\n> > >\n> > >               case controls::SATURATION: {\n> > > +                     /* Silently ignore this control for a mono sensor. */\n> > > +                     if (monoSensor_)\n> > > +                             break;\n> > > +\n> > >                       RPiController::CcmAlgorithm *ccm = dynamic_cast<RPiController::CcmAlgorithm *>(\n> > >                               controller_.getAlgorithm(\"ccm\"));\n> > >                       if (!ccm) {\n> > > diff --git a/src/ipa/rpi/common/ipa_base.h b/src/ipa/rpi/common/ipa_base.h\n> > > index 6f9c46bb16b1..39d00760d012 100644\n> > > --- a/src/ipa/rpi/common/ipa_base.h\n> > > +++ b/src/ipa/rpi/common/ipa_base.h\n> > > @@ -87,6 +87,7 @@ private:\n> > >       std::map<unsigned int, MappedFrameBuffer> buffers_;\n> > >\n> > >       bool lensPresent_;\n> > > +     bool monoSensor_;\n> > >       ControlList libcameraMetadata_;\n> > >\n> > >       std::array<RPiController::Metadata, numMetadataContexts> rpiMetadata_;","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 0B5E3C31E9\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon,  5 Jun 2023 07:43:29 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4F70E61EA2;\n\tMon,  5 Jun 2023 09:43:28 +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 EA7B460579\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon,  5 Jun 2023 09:43:26 +0200 (CEST)","from pendragon.ideasonboard.com (om126156242094.26.openmobile.ne.jp\n\t[126.156.242.94])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 711C42BC;\n\tMon,  5 Jun 2023 09:43:01 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1685951008;\n\tbh=j7UoMJ/WKe4AAo1sAeknQJ/wWuEmgZmhBWHOWMnuWBk=;\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=qSWN3PGDalJY+AhOV/TpDll9A9cTkZUAQxJQt3ILaZlEk4oHj95HLhdpKLrEJobRv\n\t7KJTs/+kJzztFj/PM90yBFO7CJDubsqlzipj8Cp5876vSWz5GJ7SZFClcy3zDIPA0o\n\t5DtRZWaXc3CvhwXdrw+bx0jSDvOoyW2IN62nTTbZwtDuDGISxex3J1g5/ifwHQjsZ3\n\t4cQIi8c0YlM3oIVD6xBGw3w0Hrq5xEWQuhSgPqbgjMbKf2Hfxp7/Ljf//GjgcCr/BM\n\tK2KmOVVTU9gmjrvGKF1cB0KoOId3zHt5mOeHW/N6ERW0zOTQeM1FlJXh+/SC81xo2Y\n\tW5rIv9rZQCRvQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1685950982;\n\tbh=j7UoMJ/WKe4AAo1sAeknQJ/wWuEmgZmhBWHOWMnuWBk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=UBNR9eK1wIo8/7CYzNnm5gMVTyw/BtXSeOr0tbE1B4EUBn3m8oYUO2VWb4XfiDh4u\n\tM3PEeMU9pTcJJrz0yLSoamk9K9Ak/oWPYZCkHM6FbJI9ux3+fTsgSEvv5rtTaiiy5s\n\tj3olKvE0+DdnDBmRq3eAVLy/gyib/tGrbcmj2USI="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"UBNR9eK1\"; dkim-atps=neutral","Date":"Mon, 5 Jun 2023 10:43:25 +0300","To":"Naushir Patuck <naush@raspberrypi.com>","Message-ID":"<20230605074325.GH22604@pendragon.ideasonboard.com>","References":"<20230602132358.16314-1-naush@raspberrypi.com>\n\t<20230602132358.16314-3-naush@raspberrypi.com>\n\t<20230602155154.GM26944@pendragon.ideasonboard.com>\n\t<CAEmqJPq9xxq=f3+DyBoNrZhT4_RY2ZHDkieXrumVkz70mzE0og@mail.gmail.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<CAEmqJPq9xxq=f3+DyBoNrZhT4_RY2ZHDkieXrumVkz70mzE0og@mail.gmail.com>","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] ipa: rpi: Handle controls for\n\tmono variant sensors","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","From":"Laurent Pinchart via libcamera-devel\n\t<libcamera-devel@lists.libcamera.org>","Reply-To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":27247,"web_url":"https://patchwork.libcamera.org/comment/27247/","msgid":"<CAEmqJPqZkDpE3694YrhvYq8kVcOkK=mTubpG0hHnww3-pcSaow@mail.gmail.com>","date":"2023-06-05T07:52:15","subject":"Re: [libcamera-devel] [PATCH v2 2/2] ipa: rpi: Handle controls for\n\tmono variant sensors","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Laurent,\n\nOn Mon, 5 Jun 2023 at 08:43, Laurent Pinchart\n<laurent.pinchart@ideasonboard.com> wrote:\n>\n> Hi Naush,\n>\n> On Mon, Jun 05, 2023 at 08:20:28AM +0100, Naushir Patuck wrote:\n> > On Fri, 2 Jun 2023 at 16:51, Laurent Pinchart wrote:\n> > > On Fri, Jun 02, 2023 at 02:23:58PM +0100, Naushir Patuck wrote:\n> > > > Do not advertise colour related controls (i.e. [A]WB, colour saturation)\n> > > > in the ControlInfoMap of available controls returned out to the\n> > > > application.\n> > > >\n> > > > Silently ignore these controls in the control handler in case applications\n> > > > don't use the advertised ControlInfoMap to validate controls.\n> > > >\n> > > > As a drive-by, don't advertise controls::ColourCorrectionMatrix in the\n> > > > ControlInfoMap as it is not handled by the IPA.\n> > > >\n> > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>\n> > > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com>\n> > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> > > > ---\n> > > >  src/ipa/rpi/common/ipa_base.cpp | 35 ++++++++++++++++++++++++++++-----\n> > > >  src/ipa/rpi/common/ipa_base.h   |  1 +\n> > > >  2 files changed, 31 insertions(+), 5 deletions(-)\n> > > >\n> > > > diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp\n> > > > index db7a0eb3a1ca..813e01fa5cfd 100644\n> > > > --- a/src/ipa/rpi/common/ipa_base.cpp\n> > > > +++ b/src/ipa/rpi/common/ipa_base.cpp\n> > > > @@ -12,6 +12,7 @@\n> > > >  #include <libcamera/base/log.h>\n> > > >  #include <libcamera/base/span.h>\n> > > >  #include <libcamera/control_ids.h>\n> > > > +#include <libcamera/property_ids.h>\n> > > >\n> > > >  #include \"controller/af_algorithm.h\"\n> > > >  #include \"controller/af_status.h\"\n> > > > @@ -60,19 +61,22 @@ const ControlInfoMap::Map ipaControls{\n> > > >       { &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },\n> > > >       { &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) },\n> > > >       { &controls::ExposureValue, ControlInfo(-8.0f, 8.0f, 0.0f) },\n> > > > -     { &controls::AwbEnable, ControlInfo(false, true) },\n> > > > -     { &controls::ColourGains, ControlInfo(0.0f, 32.0f) },\n> > > > -     { &controls::AwbMode, ControlInfo(controls::AwbModeValues) },\n> > > >       { &controls::Brightness, ControlInfo(-1.0f, 1.0f, 0.0f) },\n> > > >       { &controls::Contrast, ControlInfo(0.0f, 32.0f, 1.0f) },\n> > > > -     { &controls::Saturation, ControlInfo(0.0f, 32.0f, 1.0f) },\n> > > >       { &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },\n> > > > -     { &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) },\n> > > >       { &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },\n> > > >       { &controls::FrameDurationLimits, ControlInfo(INT64_C(33333), INT64_C(120000)) },\n> > > >       { &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) }\n> > > >  };\n> > > >\n> > > > +/* IPA controls handled conditionally, if the sensor is not mono */\n> > > > +const ControlInfoMap::Map ipaColourControls{\n> > > > +     { &controls::AwbEnable, ControlInfo(false, true) },\n> > > > +     { &controls::AwbMode, ControlInfo(controls::AwbModeValues) },\n> > > > +     { &controls::ColourGains, ControlInfo(0.0f, 32.0f) },\n> > > > +     { &controls::Saturation, ControlInfo(0.0f, 32.0f, 1.0f) },\n> > > > +};\n> > > > +\n> > > >  /* IPA controls handled conditionally, if the lens has a focus control */\n> > > >  const ControlInfoMap::Map ipaAfControls{\n> > > >       { &controls::AfMode, ControlInfo(controls::AfModeValues) },\n> > > > @@ -220,6 +224,11 @@ int32_t IpaBase::configure(const IPACameraSensorInfo &sensorInfo, const ConfigPa\n> > > >               ControlInfo(static_cast<int32_t>(mode_.minShutter.get<std::micro>()),\n> > > >                           static_cast<int32_t>(mode_.maxShutter.get<std::micro>()));\n> > > >\n> > > > +     /* Declare colour processing related controls for non-mono sensors. */\n> > > > +     monoSensor_ = sensorInfo.cfaPattern == properties::draft::ColorFilterArrangementEnum::MONO;\n> > > > +     if (!monoSensor_)\n> > > > +             ctrlMap.merge(ControlInfoMap::Map(ipaColourControls));\n> > > > +\n> > >\n> > > This means that the camera won't report the colour-related controls\n> > > until it gets configured. Could we fix that by passing the IPASensorInfo\n> > > to the init() function as well ?\n> >\n> > Yes this is true.  I've always felt that the ControlInfoMap should not be\n> > advertised at all during ipa->init(), since some controls limits (e.g.\n> > AnalogueGain and ExposureTime) have invalid min/max/default values.\n> > They only get correct values set after ipa->configure().\n> >\n> > Rather than passing IPASensorInfo to init(), I would probably prefer to entirely\n> > remove setting up the ControlInfoMap there.  What are your thoughts?\n>\n> This is an issue we'll need to tackle, and I don't know yet how to do\n> so. Not reporting properties and controls until after configure() is an\n> easy option, but it would make it more difficult for applications to\n> figure out if a camera is suitable for their needs, or just adapt their\n> behaviour based on the camera features. I don't want to drop reporting\n> supported controls at init time completely until we have a proper,\n> well-thought replacement.\n\nOk, I'll post an update with the IPASensorInfo passed into init() for now until\nwe have a better solution.\n\nRegards,\nNaush\n\n>\n> > > >       /* Declare Autofocus controls, only if we have a controllable lens */\n> > > >       if (lensPresent_)\n> > > >               ctrlMap.merge(ControlInfoMap::Map(ipaAfControls));\n> > > > @@ -780,6 +789,10 @@ void IpaBase::applyControls(const ControlList &controls)\n> > > >               }\n> > > >\n> > > >               case controls::AWB_ENABLE: {\n> > > > +                     /* Silently ignore this control for a mono sensor. */\n> > > > +                     if (monoSensor_)\n> > > > +                             break;\n> > > > +\n> > > >                       RPiController::AwbAlgorithm *awb = dynamic_cast<RPiController::AwbAlgorithm *>(\n> > > >                               controller_.getAlgorithm(\"awb\"));\n> > > >                       if (!awb) {\n> > > > @@ -799,6 +812,10 @@ void IpaBase::applyControls(const ControlList &controls)\n> > > >               }\n> > > >\n> > > >               case controls::AWB_MODE: {\n> > > > +                     /* Silently ignore this control for a mono sensor. */\n> > > > +                     if (monoSensor_)\n> > > > +                             break;\n> > > > +\n> > > >                       RPiController::AwbAlgorithm *awb = dynamic_cast<RPiController::AwbAlgorithm *>(\n> > > >                               controller_.getAlgorithm(\"awb\"));\n> > > >                       if (!awb) {\n> > > > @@ -819,6 +836,10 @@ void IpaBase::applyControls(const ControlList &controls)\n> > > >               }\n> > > >\n> > > >               case controls::COLOUR_GAINS: {\n> > > > +                     /* Silently ignore this control for a mono sensor. */\n> > > > +                     if (monoSensor_)\n> > > > +                             break;\n> > > > +\n> > > >                       auto gains = ctrl.second.get<Span<const float>>();\n> > > >                       RPiController::AwbAlgorithm *awb = dynamic_cast<RPiController::AwbAlgorithm *>(\n> > > >                               controller_.getAlgorithm(\"awb\"));\n> > > > @@ -867,6 +888,10 @@ void IpaBase::applyControls(const ControlList &controls)\n> > > >               }\n> > > >\n> > > >               case controls::SATURATION: {\n> > > > +                     /* Silently ignore this control for a mono sensor. */\n> > > > +                     if (monoSensor_)\n> > > > +                             break;\n> > > > +\n> > > >                       RPiController::CcmAlgorithm *ccm = dynamic_cast<RPiController::CcmAlgorithm *>(\n> > > >                               controller_.getAlgorithm(\"ccm\"));\n> > > >                       if (!ccm) {\n> > > > diff --git a/src/ipa/rpi/common/ipa_base.h b/src/ipa/rpi/common/ipa_base.h\n> > > > index 6f9c46bb16b1..39d00760d012 100644\n> > > > --- a/src/ipa/rpi/common/ipa_base.h\n> > > > +++ b/src/ipa/rpi/common/ipa_base.h\n> > > > @@ -87,6 +87,7 @@ private:\n> > > >       std::map<unsigned int, MappedFrameBuffer> buffers_;\n> > > >\n> > > >       bool lensPresent_;\n> > > > +     bool monoSensor_;\n> > > >       ControlList libcameraMetadata_;\n> > > >\n> > > >       std::array<RPiController::Metadata, numMetadataContexts> rpiMetadata_;\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 5D559C31E9\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon,  5 Jun 2023 07:52:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2A2C262754;\n\tMon,  5 Jun 2023 09:52:34 +0200 (CEST)","from mail-yw1-x1130.google.com (mail-yw1-x1130.google.com\n\t[IPv6:2607:f8b0:4864:20::1130])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 328AC60579\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon,  5 Jun 2023 09:52:32 +0200 (CEST)","by mail-yw1-x1130.google.com with SMTP id\n\t00721157ae682-5664b14966bso56780267b3.1\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 05 Jun 2023 00:52:32 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1685951554;\n\tbh=lBnifF35GcuhrxC2keEGzDak20AR5B2hiOzi9u66GUA=;\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=ftq01Tb0+6iMiOKuHQmujDmPqIUjqeGNf9iPZrxo+5+hGyBsY2mOlPmLidLVTc63T\n\tq/VWi5r50BOBzlRqS4UVS05PmUCw7YjWgBf40Ihrhca9LqoA/bdVvc9Yac2lNabs3E\n\t1irwNjoWwW0XyAzVgYjYgtFs5VHb2p1Md110Gz3SXCyyDOIjfN9XC9qKttPzx6cLNM\n\tdifLUnr4hK3u23ZSIkZakyNjcw6VzGzmLuudDwl5djfWmky1uizFuP3qzB9iHeL2f+\n\tZTuKlfhFrI3IDuHErJwyGq37SHNJx3ynkXhITRq3v+V/pyDPBARHYtNeauLiG3oQif\n\tLqmU+4ZYwh6sg==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1685951551; x=1688543551;\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=0fMNMU+3AcsDLfWVjglAdfUk3VsrvyzpKGkiYFu4IFY=;\n\tb=KMaemSqhQumrLP2/Ionik+htA3YaJR6ZbFnFf6SoP6qLUEoSQZA+wqQV+g27McNtUQ\n\tfxjohvzEBhyrRHD9qdo+5ZmO2gKFaa08Q1xi5si21iY0AgdgmL0zket9nn1tXZf1u8Uf\n\ttXwMD43rAKHxCAFH9TavhRE00j05BYQ//ONDngbPAodbZahKRCfih6/z1W+m7X9CdREV\n\teyWMTzJ8yDSFUNSFF/OmRkaNnjY44z/dIp7AyHOPFNCRzMaERlprlrlQRUhilHlu+89R\n\txVK0G1PVRN4WLNZd0oll7xoB3LIp1qEcECa5k1HDR+jEs/CniCc3GIhGp5tv394a6MsH\n\t8WEA=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"KMaemSqh\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20221208; t=1685951551; x=1688543551;\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=0fMNMU+3AcsDLfWVjglAdfUk3VsrvyzpKGkiYFu4IFY=;\n\tb=lg5GA05dR5qpukp7BCBJdKB2Idwnl7WTz16STc+mxT/ytWBE02kJDqMTBW2OBYyy6W\n\ttA4xAqFG1efPWcLDLzxaA1/anUGXLSQ9QSQ+uLA4CMN88s9eE0o/s8+/W/5lqAodGFNU\n\tdjarc3DJY+vQ59dK1XS1+5ggzUQUgrttKTRGH0X5J1Hs5hKJScVoDXLD/eU9cipHqS6J\n\tgDGY/3x8zLPMWQCf17JIbbtNWuOZXPYhaLx27hdK+rKthizDZw7mqVWcYjPXlDnjgHHQ\n\tQo0/L7fGSOvdXesLqOW0Z0ynCtu8XxMCUZCUq04NPW+r1PkVcfmq6k3cGSDVYSIFYz7V\n\t0Cng==","X-Gm-Message-State":"AC+VfDxOZ1h5Pze6VUT3k5oVBKFpMpUkjXhMWt1IYxG/oTMQy3IGhXou\n\tQ/m3u2GLYkaHAhie084qht66ZmEzD3H8ChSH3sMxYr4AlpJ5SjvlmInUOQ==","X-Google-Smtp-Source":"ACHHUZ7vQBRCvD7fF/tF7LBxdV2EcsZnewI+QM1VeEFDS+tZ8UnUgWWZcParyg1gNnQfKzqZ0ejORWOX1ZbqHa8JUvE=","X-Received":"by 2002:a0d:ea85:0:b0:559:deed:f363 with SMTP id\n\tt127-20020a0dea85000000b00559deedf363mr10980480ywe.2.1685951551106;\n\tMon, 05 Jun 2023 00:52:31 -0700 (PDT)","MIME-Version":"1.0","References":"<20230602132358.16314-1-naush@raspberrypi.com>\n\t<20230602132358.16314-3-naush@raspberrypi.com>\n\t<20230602155154.GM26944@pendragon.ideasonboard.com>\n\t<CAEmqJPq9xxq=f3+DyBoNrZhT4_RY2ZHDkieXrumVkz70mzE0og@mail.gmail.com>\n\t<20230605074325.GH22604@pendragon.ideasonboard.com>","In-Reply-To":"<20230605074325.GH22604@pendragon.ideasonboard.com>","Date":"Mon, 5 Jun 2023 08:52:15 +0100","Message-ID":"<CAEmqJPqZkDpE3694YrhvYq8kVcOkK=mTubpG0hHnww3-pcSaow@mail.gmail.com>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] ipa: rpi: Handle controls for\n\tmono variant sensors","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","From":"Naushir Patuck via libcamera-devel\n\t<libcamera-devel@lists.libcamera.org>","Reply-To":"Naushir Patuck <naush@raspberrypi.com>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]