[{"id":27712,"web_url":"https://patchwork.libcamera.org/comment/27712/","msgid":"","date":"2023-08-30T08:13:03","subject":"Re: [libcamera-devel] [PATCH v2 4/5] ipa: rpi: agc: Add\n\tAgcChannelConstraint class","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi David\n\nOn Wed, Aug 23, 2023 at 01:09:14PM +0100, David Plowman via libcamera-devel wrote:\n> A channel constraint is somewhat similar to the upper/lower bound\n> constraints that we use elsewhere, but these constraints apply between\n> multiple AGC channels. For example, it lets you say things like \"don't\n> let the channel 1 total exposure be more than 8x that of channel 0\",\n\nI haven't seen an example of a config file, can you make an example of\nhow you would express the above with UPPER and LOWER ?\n\n> and so on. By using both an upper and lower bound constraint, you\n> could fix one AGC channel always to be a fixed ratio of another.\n>\n> Also read a vector of them (if present) when loading the tuning file.\n>\n> Signed-off-by: David Plowman \n> Reviewed-by: Naushir Patuck \n> ---\n> src/ipa/rpi/controller/rpi/agc_channel.cpp | 42 ++++++++++++++++++++++\n> src/ipa/rpi/controller/rpi/agc_channel.h | 10 ++++++\n> 2 files changed, 52 insertions(+)\n>\n> diff --git a/src/ipa/rpi/controller/rpi/agc_channel.cpp b/src/ipa/rpi/controller/rpi/agc_channel.cpp\n> index ddec611f..44198c2c 100644\n> --- a/src/ipa/rpi/controller/rpi/agc_channel.cpp\n> +++ b/src/ipa/rpi/controller/rpi/agc_channel.cpp\n> @@ -173,6 +173,42 @@ readConstraintModes(std::map &constraintModes,\n> \treturn { 0, first };\n> }\n>\n> +int AgcChannelConstraint::read(const libcamera::YamlObject ¶ms)\n> +{\n> +\tstd::string boundString = params[\"bound\"].get(\"\");\n> +\ttransform(boundString.begin(), boundString.end(),\n> +\t\t boundString.begin(), ::toupper);\n\nwould this allow \"upper\" and \"UPPER\" ?\n\n> +\tif (boundString != \"UPPER\" && boundString != \"LOWER\") {\n> +\t\tLOG(RPiAgc, Error) << \"AGC channelconstraint type should be UPPER or LOWER\";\n> +\t\treturn -EINVAL;\n> +\t}\n> +\tbound = boundString == \"UPPER\" ? Bound::UPPER : Bound::LOWER;\n> +\n> +\tauto value = params[\"factor\"].get();\n> +\tif (!value)\n> +\t\treturn -EINVAL;\n\nDoes this deserve an error message ?\n\n> +\tfactor = *value;\n> +\n> +\treturn 0;\n> +}\n> +\n> +static int readChannelConstraints(std::vector &channelConstraints,\n> +\t\t\t\t const libcamera::YamlObject ¶ms)\n> +{\n> +\tint ret = 0;\n\nno need\n\n> +\n> +\tfor (const auto &p : params.asList()) {\n> +\t\tAgcChannelConstraint constraint;\n> +\t\tret = constraint.read(p);\n\nint ret = ..\n\n> +\t\tif (ret)\n> +\t\t\treturn ret;\n> +\n> +\t\tchannelConstraints.push_back(constraint);\n> +\t}\n> +\n> +\treturn ret;\n\nand return 0\n\n> +}\n> +\n> int AgcConfig::read(const libcamera::YamlObject ¶ms)\n> {\n> \tLOG(RPiAgc, Debug) << \"AgcConfig\";\n> @@ -191,6 +227,12 @@ int AgcConfig::read(const libcamera::YamlObject ¶ms)\n> \tif (ret)\n> \t\treturn ret;\n>\n> +\tif (params.contains(\"channel_constraints\")) {\n> +\t\tret = readChannelConstraints(channelConstraints, params[\"channel_constraints\"]);\n> +\t\tif (ret)\n> +\t\t\treturn ret;\n> +\t}\n> +\n> \tret = yTarget.read(params[\"y_target\"]);\n> \tif (ret)\n> \t\treturn ret;\n> diff --git a/src/ipa/rpi/controller/rpi/agc_channel.h b/src/ipa/rpi/controller/rpi/agc_channel.h\n> index 0e3d44b0..446125ef 100644\n> --- a/src/ipa/rpi/controller/rpi/agc_channel.h\n> +++ b/src/ipa/rpi/controller/rpi/agc_channel.h\n> @@ -42,11 +42,21 @@ struct AgcConstraint {\n>\n> typedef std::vector AgcConstraintMode;\n>\n> +struct AgcChannelConstraint {\n> +\tenum class Bound { LOWER = 0,\n> +\t\t\t UPPER = 1 };\n> +\tBound bound;\n> +\tunsigned int channel;\n> +\tdouble factor;\n> +\tint read(const libcamera::YamlObject ¶ms);\n> +};\n> +\n> struct AgcConfig {\n> \tint read(const libcamera::YamlObject ¶ms);\n> \tstd::map meteringModes;\n> \tstd::map exposureModes;\n> \tstd::map constraintModes;\n> +\tstd::vector channelConstraints;\n> \tPwl yTarget;\n> \tdouble speed;\n> \tuint16_t startupFrames;\n> --\n> 2.30.2\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 593E3BE080\n\tfor ;\n\tWed, 30 Aug 2023 08:13:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7FBD461F19;\n\tWed, 30 Aug 2023 10:13:08 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BF8CC61E00\n\tfor ;\n\tWed, 30 Aug 2023 10:13:06 +0200 (CEST)","from ideasonboard.com (mob-5-90-48-184.net.vodafone.it\n\t[5.90.48.184])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 48BB1814;\n\tWed, 30 Aug 2023 10:11:44 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1693383188;\n\tbh=dArlKQM5ZiCa/Dzl15RXWWgmqSsEErB/udBpLv3sOTg=;\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=gqJ/jDCqsIJ5nFIDDo6DUevukgZK10PzplJLmNGUJLM+CG8J3WrPQlnpdi4NOwCHR\n\t+Guaz+wKTU529IFSpAs/NwlcNipAP+FvZ/Y1deoBOGPnIBJkyVOKeNBUo3hu88op8o\n\tJMBRG1Q2VRVsxbRJZyMMl32LBLJVhBzv+sEKfLq5WgQ3H4GonUOkPomfpXeGfNOWmk\n\tzqGSM8LakXT3mEhcas0ZVnKe8kjGKHNwPaFaEgyvOWb1kHpE2bCkiHKASTSJZbnyJW\n\tzzrJB+/QoHtNwX4Xgv/kqjs77wYuV40xiBlYvOMdwoq9owamDcuI50nAhKnPsFqx7H\n\txb1eQgzHkMcmw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1693383104;\n\tbh=dArlKQM5ZiCa/Dzl15RXWWgmqSsEErB/udBpLv3sOTg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=YMWBvzrQlb6z4W5DWEWOPPlD+P73ZXSXPNe5xTY9T0lxfOAyNrLrKaDLL5c19iNul\n\tGlhaZvSlQNJeB8iCNW4WF6cRZaN5t7n7RkoalPb9o+eTD4f4YacAcF+cdgaaotV0jH\n\twr/UQw2qg8M6BNnwhZrG5rHUBS7aKAwgiPGfCQDc="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"YMWBvzrQ\"; dkim-atps=neutral","Date":"Wed, 30 Aug 2023 10:13:03 +0200","To":"David Plowman ","Message-ID":"","References":"<20230823120915.18621-1-david.plowman@raspberrypi.com>\n\t<20230823120915.18621-5-david.plowman@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20230823120915.18621-5-david.plowman@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v2 4/5] ipa: rpi: agc: Add\n\tAgcChannelConstraint class","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":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27752,"web_url":"https://patchwork.libcamera.org/comment/27752/","msgid":"","date":"2023-09-11T14:43:40","subject":"Re: [libcamera-devel] [PATCH v2 4/5] ipa: rpi: agc: Add\n\tAgcChannelConstraint class","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Jacopo\n\nThanks for the review.\n\nOn Wed, 30 Aug 2023 at 09:13, Jacopo Mondi\n wrote:\n>\n> Hi David\n>\n> On Wed, Aug 23, 2023 at 01:09:14PM +0100, David Plowman via libcamera-devel wrote:\n> > A channel constraint is somewhat similar to the upper/lower bound\n> > constraints that we use elsewhere, but these constraints apply between\n> > multiple AGC channels. For example, it lets you say things like \"don't\n> > let the channel 1 total exposure be more than 8x that of channel 0\",\n>\n> I haven't seen an example of a config file, can you make an example of\n> how you would express the above with UPPER and LOWER ?\n\nYes, given that I'm not using this feature anywhere (though I expect\nother people may want to), then I should supply something. What would\nbe most appropriate do you think, extending the commit message,\ncomments in the source file or something different?\n\n>\n> > and so on. By using both an upper and lower bound constraint, you\n> > could fix one AGC channel always to be a fixed ratio of another.\n> >\n> > Also read a vector of them (if present) when loading the tuning file.\n> >\n> > Signed-off-by: David Plowman \n> > Reviewed-by: Naushir Patuck \n> > ---\n> > src/ipa/rpi/controller/rpi/agc_channel.cpp | 42 ++++++++++++++++++++++\n> > src/ipa/rpi/controller/rpi/agc_channel.h | 10 ++++++\n> > 2 files changed, 52 insertions(+)\n> >\n> > diff --git a/src/ipa/rpi/controller/rpi/agc_channel.cpp b/src/ipa/rpi/controller/rpi/agc_channel.cpp\n> > index ddec611f..44198c2c 100644\n> > --- a/src/ipa/rpi/controller/rpi/agc_channel.cpp\n> > +++ b/src/ipa/rpi/controller/rpi/agc_channel.cpp\n> > @@ -173,6 +173,42 @@ readConstraintModes(std::map &constraintModes,\n> > return { 0, first };\n> > }\n> >\n> > +int AgcChannelConstraint::read(const libcamera::YamlObject ¶ms)\n> > +{\n> > + std::string boundString = params[\"bound\"].get(\"\");\n> > + transform(boundString.begin(), boundString.end(),\n> > + boundString.begin(), ::toupper);\n>\n> would this allow \"upper\" and \"UPPER\" ?\n\nYes, or \"Upper\" or even \"uPpEr\" if you want! But failing just on\naccount of the case doesn't seem very helpful.\n\n>\n> > + if (boundString != \"UPPER\" && boundString != \"LOWER\") {\n> > + LOG(RPiAgc, Error) << \"AGC channelconstraint type should be UPPER or LOWER\";\n> > + return -EINVAL;\n> > + }\n> > + bound = boundString == \"UPPER\" ? Bound::UPPER : Bound::LOWER;\n> > +\n> > + auto value = params[\"factor\"].get();\n> > + if (!value)\n> > + return -EINVAL;\n>\n> Does this deserve an error message ?\n\nGood point, I think it should.\n\n>\n> > + factor = *value;\n> > +\n> > + return 0;\n> > +}\n> > +\n> > +static int readChannelConstraints(std::vector &channelConstraints,\n> > + const libcamera::YamlObject ¶ms)\n> > +{\n> > + int ret = 0;\n>\n> no need\n>\n> > +\n> > + for (const auto &p : params.asList()) {\n> > + AgcChannelConstraint constraint;\n> > + ret = constraint.read(p);\n>\n> int ret = ..\n>\n> > + if (ret)\n> > + return ret;\n> > +\n> > + channelConstraints.push_back(constraint);\n> > + }\n> > +\n> > + return ret;\n>\n> and return 0\n\nYep, I can make those changes!\n\nThanks\nDavid\n\n>\n> > +}\n> > +\n> > int AgcConfig::read(const libcamera::YamlObject ¶ms)\n> > {\n> > LOG(RPiAgc, Debug) << \"AgcConfig\";\n> > @@ -191,6 +227,12 @@ int AgcConfig::read(const libcamera::YamlObject ¶ms)\n> > if (ret)\n> > return ret;\n> >\n> > + if (params.contains(\"channel_constraints\")) {\n> > + ret = readChannelConstraints(channelConstraints, params[\"channel_constraints\"]);\n> > + if (ret)\n> > + return ret;\n> > + }\n> > +\n> > ret = yTarget.read(params[\"y_target\"]);\n> > if (ret)\n> > return ret;\n> > diff --git a/src/ipa/rpi/controller/rpi/agc_channel.h b/src/ipa/rpi/controller/rpi/agc_channel.h\n> > index 0e3d44b0..446125ef 100644\n> > --- a/src/ipa/rpi/controller/rpi/agc_channel.h\n> > +++ b/src/ipa/rpi/controller/rpi/agc_channel.h\n> > @@ -42,11 +42,21 @@ struct AgcConstraint {\n> >\n> > typedef std::vector AgcConstraintMode;\n> >\n> > +struct AgcChannelConstraint {\n> > + enum class Bound { LOWER = 0,\n> > + UPPER = 1 };\n> > + Bound bound;\n> > + unsigned int channel;\n> > + double factor;\n> > + int read(const libcamera::YamlObject ¶ms);\n> > +};\n> > +\n> > struct AgcConfig {\n> > int read(const libcamera::YamlObject ¶ms);\n> > std::map meteringModes;\n> > std::map exposureModes;\n> > std::map constraintModes;\n> > + std::vector channelConstraints;\n> > Pwl yTarget;\n> > double speed;\n> > uint16_t startupFrames;\n> > --\n> > 2.30.2\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 D8730BE080\n\tfor ;\n\tMon, 11 Sep 2023 14:43:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4999F628F2;\n\tMon, 11 Sep 2023 16:43:53 +0200 (CEST)","from mail-qt1-x82d.google.com (mail-qt1-x82d.google.com\n\t[IPv6:2607:f8b0:4864:20::82d])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5DAFE61DF0\n\tfor ;\n\tMon, 11 Sep 2023 16:43:52 +0200 (CEST)","by mail-qt1-x82d.google.com with SMTP id\n\td75a77b69052e-4121130e7afso31944841cf.2\n\tfor ;\n\tMon, 11 Sep 2023 07:43:52 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1694443433;\n\tbh=qpjpI9n3lxzZuUtsUf3YOtvfp5wnTHoT/PZqKy43Frc=;\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=t6sFk9VMzSiAPdvNPJJAJNu+VLGZmU/tKS/7kdGw2R+tZXI4EpvSMk0AYynv5tTn/\n\tchQJjY7Bg6RJ7lQht+IvWXuoMow9yblWPw8+NJclxQwA448t9nSUR85gegQaomG/cp\n\tNxyLpU308mRfYWP19orl/he4Yox/RC83UqRt2OVXzCbAPpWyLKXlsZ3nMwMiCklotB\n\tNnLNELJeZR2U3or5X2dRm52BmJvp3iV8MeFpi9Y158IhScy6XDGCtsoZsEabWlMeIL\n\t041MbHBlT6W+uOQNcWfRRwGu3h53NaaEugEwGyD6wkfIUsCnxOcncpfGAhKDSQwugf\n\tu2Qb+7SgMouEA==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1694443431; x=1695048231;\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=DiA9snVUdujpdGBukL0cXItSTemGpU3x6E9DF5kNE+0=;\n\tb=A7xSx8akz13goUALdwRnAPIkrDOpp7SeRvwlwmdjc2d4buK1gNDNjSzgtFO4cocnqm\n\tV1BTyIBrCn8DVN8Ap7Ty0pi6G13qHV5PO1KkMvT+oerb25uW9MUSBJAV2phDhdIaeRW4\n\tiTNTE2dwE/iwyAg+aEi7rSDeGiFFjV0EwkW3XAFlxTScAQJ9/9HuLVkJ6lU/Ib9m7Yvc\n\tpbdazkv17PeoM+Acq4x+omYQUmjaPukzDXaVBSajMnULn3MEaqP7He/RdV5k34C3X0U5\n\tFUpPzIKt2DOHdDDC+dq5mN3ZfWBBbiZNWpQbFbQx1Rq4GV5agNk/LHe/MhwmPIqsFGKJ\n\tREMw=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"A7xSx8ak\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1694443431; x=1695048231;\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=DiA9snVUdujpdGBukL0cXItSTemGpU3x6E9DF5kNE+0=;\n\tb=jbFj4HDx2cInn21b0ZO8mSAx1sV55ZwWh1wKM18LKBiQ2/toWVHeTCmS3q37b1Y5+1\n\tv86bQ+tcKYbv/8qqnXtHi6zVBFazI/t0qz4vjdvog3zgII8KHJUu/VjJFypTtUi8GuMg\n\tq/4BhnhJyZtGzJlGAhSALVCeW+X3a3nCuZ33tc/YtEdv9OseZ9TMhG2mpnGxcCaOoDy/\n\tLqJiTuMfoHL5LFmgAkCCW3PQn647woCRMHq9xnZONgnzYIbamIbPjKI47ubaAxbbi337\n\ttTzifkyb31JrlHgEGjbgd+GwJF7BthASYrbB2zD4EvMisudSwwffVxJhFx1j+NI1BDoA\n\tg+Ww==","X-Gm-Message-State":"AOJu0YwNfy9iWopwa5RskOdUhCS7PWP2HmROtgvEKq7XfX99nbV3D7Yb\n\tDc+OyrczgXx2SkJsXUgbDQj4pAIysueJyJwZeBxpZYvoQDr9QUY5Pr8=","X-Google-Smtp-Source":"AGHT+IEwn+LuvJ1oQL8vyfr56gj/dlZiDmoTQ9vnunZ/7/7wSl8qwnjDcBxsEfs94bZCNyYItVSXzy4fGkCBkH3DwTk=","X-Received":"by 2002:a05:622a:248:b0:410:961:668a with SMTP id\n\tc8-20020a05622a024800b004100961668amr9931498qtx.26.1694443431180;\n\tMon, 11 Sep 2023 07:43:51 -0700 (PDT)","MIME-Version":"1.0","References":"<20230823120915.18621-1-david.plowman@raspberrypi.com>\n\t<20230823120915.18621-5-david.plowman@raspberrypi.com>\n\t","In-Reply-To":"","Date":"Mon, 11 Sep 2023 15:43:40 +0100","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v2 4/5] ipa: rpi: agc: Add\n\tAgcChannelConstraint class","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":"David Plowman via libcamera-devel ","Reply-To":"David Plowman ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]