[{"id":22483,"web_url":"https://patchwork.libcamera.org/comment/22483/","msgid":"<20220328132706.h5lcus7axzm3owzo@uno.localdomain>","date":"2022-03-28T13:27:06","subject":"Re: [libcamera-devel] [PATCH 1/4] libipa: camera_sensor_helper:\n\tReorganize gain constants","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent\n\nOn Mon, Mar 28, 2022 at 03:03:33PM +0300, Laurent Pinchart via libcamera-devel wrote:\n> To prepare for other gain models than the linear model, store the gain\n> constants in a union with per-model members. Due to the lack of\n> designated initializer support in gcc with C++17, initializing a single\n> complex structure that includes a union will be difficult. Split the\n> gain model type to a separate variable to work around this issue.\n\nLooking ahead, this makes sense.\n\nToo bad having something like\n\n struct {\n AnalogueGainType type_;\n AnalogueGainConstants constants_;\n } gain;\n\nmakes initialization more complex, so separate fields are ok\n\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n>\n> Signed-off-by: Laurent Pinchart \n> ---\n> src/ipa/libipa/camera_sensor_helper.cpp | 102 ++++++++++++++----------\n> src/ipa/libipa/camera_sensor_helper.h | 10 ++-\n> 2 files changed, 65 insertions(+), 47 deletions(-)\n>\n> diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp\n> index c953def04fd7..714cd86f039f 100644\n> --- a/src/ipa/libipa/camera_sensor_helper.cpp\n> +++ b/src/ipa/libipa/camera_sensor_helper.cpp\n> @@ -58,11 +58,13 @@ namespace ipa {\n> */\n> uint32_t CameraSensorHelper::gainCode(double gain) const\n> {\n> -\tASSERT(analogueGainConstants_.m0 == 0 || analogueGainConstants_.m1 == 0);\n> -\tASSERT(analogueGainConstants_.type == AnalogueGainLinear);\n> +\tconst AnalogueGainConstants &k = gainConstants_;\n>\n> -\treturn (analogueGainConstants_.c0 - analogueGainConstants_.c1 * gain) /\n> -\t (analogueGainConstants_.m1 * gain - analogueGainConstants_.m0);\n> +\tASSERT(gainType_ == AnalogueGainLinear);\n> +\tASSERT(k.linear.m0 == 0 || k.linear.m1 == 0);\n> +\n> +\treturn (k.linear.c0 - k.linear.c1 * gain) /\n> +\t (k.linear.m1 * gain - k.linear.m0);\n> }\n>\n> /**\n> @@ -80,11 +82,13 @@ uint32_t CameraSensorHelper::gainCode(double gain) const\n> */\n> double CameraSensorHelper::gain(uint32_t gainCode) const\n> {\n> -\tASSERT(analogueGainConstants_.m0 == 0 || analogueGainConstants_.m1 == 0);\n> -\tASSERT(analogueGainConstants_.type == AnalogueGainLinear);\n> +\tconst AnalogueGainConstants &k = gainConstants_;\n>\n> -\treturn (analogueGainConstants_.m0 * static_cast(gainCode) + analogueGainConstants_.c0) /\n> -\t (analogueGainConstants_.m1 * static_cast(gainCode) + analogueGainConstants_.c1);\n> +\tASSERT(gainType_ == AnalogueGainLinear);\n> +\tASSERT(k.linear.m0 == 0 || k.linear.m1 == 0);\n> +\n> +\treturn (k.linear.m0 * static_cast(gainCode) + k.linear.c0) /\n> +\t (k.linear.m1 * static_cast(gainCode) + k.linear.c1);\n> }\n>\n> /**\n> @@ -127,42 +131,45 @@ double CameraSensorHelper::gain(uint32_t gainCode) const\n> * \\todo not implemented in libipa\n> */\n>\n> +/**\n> + * \\struct CameraSensorHelper::AnalogueGainLinearConstants\n> + * \\brief Analogue gain constants for the linear gain model\n> + *\n> + * \\var CameraSensorHelper::AnalogueGainLinearConstants::m0\n> + * \\brief Constant used in the linear gain coding/decoding\n> + *\n> + * \\note Either m0 or m1 shall be zero.\n> + *\n> + * \\var CameraSensorHelper::AnalogueGainLinearConstants::c0\n> + * \\brief Constant used in the linear gain coding/decoding\n> + *\n> + * \\var CameraSensorHelper::AnalogueGainLinearConstants::m1\n> + * \\brief Constant used in the linear gain coding/decoding\n> + *\n> + * \\note Either m0 or m1 shall be zero.\n> + *\n> + * \\var CameraSensorHelper::AnalogueGainLinearConstants::c1\n> + * \\brief Constant used in the linear gain coding/decoding\n> + */\n> +\n> /**\n> * \\struct CameraSensorHelper::AnalogueGainConstants\n> - * \\brief Analogue gain constants used for gain calculation\n> - */\n> -\n> -/**\n> - * \\var CameraSensorHelper::AnalogueGainConstants::type\n> - * \\brief Analogue gain calculation mode\n> - */\n> -\n> -/**\n> - * \\var CameraSensorHelper::AnalogueGainConstants::m0\n> - * \\brief Constant used in the analogue Gain coding/decoding\n> + * \\brief Analogue gain model constants\n> *\n> - * \\note Either m0 or m1 shall be zero.\n> - */\n> -\n> -/**\n> - * \\var CameraSensorHelper::AnalogueGainConstants::c0\n> - * \\brief Constant used in the analogue gain coding/decoding\n> - */\n> -\n> -/**\n> - * \\var CameraSensorHelper::AnalogueGainConstants::m1\n> - * \\brief Constant used in the analogue gain coding/decoding\n> + * This union stores the constants used to calculate the analogue gain. The\n> + * CameraSensorHelper::gainType_ variable selects which union member is valid.\n> *\n> - * \\note Either m0 or m1 shall be zero.\n> + * \\var CameraSensorHelper::AnalogueGainConstants::linear\n> + * \\brief Constants for the linear gain model\n> */\n>\n> /**\n> - * \\var CameraSensorHelper::AnalogueGainConstants::c1\n> - * \\brief Constant used in the analogue gain coding/decoding\n> + * \\var CameraSensorHelper::gainType_\n> + * \\brief The analogue gain model type\n> */\n>\n> /**\n> - * \\var CameraSensorHelper::analogueGainConstants_\n> + * \\var CameraSensorHelper::gainConstants_\n> * \\brief The analogue gain parameters used for calculation\n> *\n> * The analogue gain is calculated through a formula, and its parameters are\n> @@ -290,7 +297,8 @@ class CameraSensorHelperImx219 : public CameraSensorHelper\n> public:\n> \tCameraSensorHelperImx219()\n> \t{\n> -\t\tanalogueGainConstants_ = { AnalogueGainLinear, 0, 256, -1, 256 };\n> +\t\tgainType_ = AnalogueGainLinear;\n> +\t\tgainConstants_.linear = { 0, 256, -1, 256 };\n> \t}\n> };\n> REGISTER_CAMERA_SENSOR_HELPER(\"imx219\", CameraSensorHelperImx219)\n> @@ -298,10 +306,11 @@ REGISTER_CAMERA_SENSOR_HELPER(\"imx219\", CameraSensorHelperImx219)\n> class CameraSensorHelperImx258 : public CameraSensorHelper\n> {\n> public:\n> - CameraSensorHelperImx258()\n> - {\n> - analogueGainConstants_ = { AnalogueGainLinear, 0, 512, -1, 512 };\n> - }\n> +\tCameraSensorHelperImx258()\n> +\t{\n> +\t\tgainType_ = AnalogueGainLinear;\n> +\t\tgainConstants_.linear = { 0, 512, -1, 512 };\n> +\t}\n> };\n> REGISTER_CAMERA_SENSOR_HELPER(\"imx258\", CameraSensorHelperImx258)\n>\n> @@ -310,7 +319,8 @@ class CameraSensorHelperOv2740 : public CameraSensorHelper\n> public:\n> \tCameraSensorHelperOv2740()\n> \t{\n> -\t\tanalogueGainConstants_ = { AnalogueGainLinear, 1, 0, 0, 128 };\n> +\t\tgainType_ = AnalogueGainLinear;\n> +\t\tgainConstants_.linear = { 1, 0, 0, 128 };\n> \t}\n> };\n> REGISTER_CAMERA_SENSOR_HELPER(\"ov2740\", CameraSensorHelperOv2740)\n> @@ -320,7 +330,8 @@ class CameraSensorHelperOv5670 : public CameraSensorHelper\n> public:\n> \tCameraSensorHelperOv5670()\n> \t{\n> -\t\tanalogueGainConstants_ = { AnalogueGainLinear, 1, 0, 0, 128 };\n> +\t\tgainType_ = AnalogueGainLinear;\n> +\t\tgainConstants_.linear = { 1, 0, 0, 128 };\n> \t}\n> };\n> REGISTER_CAMERA_SENSOR_HELPER(\"ov5670\", CameraSensorHelperOv5670)\n> @@ -330,7 +341,8 @@ class CameraSensorHelperOv5693 : public CameraSensorHelper\n> public:\n> \tCameraSensorHelperOv5693()\n> \t{\n> -\t\tanalogueGainConstants_ = { AnalogueGainLinear, 1, 0, 0, 16 };\n> +\t\tgainType_ = AnalogueGainLinear;\n> +\t\tgainConstants_.linear = { 1, 0, 0, 16 };\n> \t}\n> };\n> REGISTER_CAMERA_SENSOR_HELPER(\"ov5693\", CameraSensorHelperOv5693)\n> @@ -340,7 +352,8 @@ class CameraSensorHelperOv8865 : public CameraSensorHelper\n> public:\n> \tCameraSensorHelperOv8865()\n> \t{\n> -\t\tanalogueGainConstants_ = { AnalogueGainLinear, 1, 0, 0, 128 };\n> +\t\tgainType_ = AnalogueGainLinear;\n> +\t\tgainConstants_.linear = { 1, 0, 0, 128 };\n> \t}\n> };\n> REGISTER_CAMERA_SENSOR_HELPER(\"ov8865\", CameraSensorHelperOv8865)\n> @@ -350,7 +363,8 @@ class CameraSensorHelperOv13858 : public CameraSensorHelper\n> public:\n> \tCameraSensorHelperOv13858()\n> \t{\n> -\t\tanalogueGainConstants_ = { AnalogueGainLinear, 1, 0, 0, 128 };\n> +\t\tgainType_ = AnalogueGainLinear;\n> +\t\tgainConstants_.linear = { 1, 0, 0, 128 };\n> \t}\n> };\n> REGISTER_CAMERA_SENSOR_HELPER(\"ov13858\", CameraSensorHelperOv13858)\n> diff --git a/src/ipa/libipa/camera_sensor_helper.h b/src/ipa/libipa/camera_sensor_helper.h\n> index 26adfcb5955f..6b96520ba601 100644\n> --- a/src/ipa/libipa/camera_sensor_helper.h\n> +++ b/src/ipa/libipa/camera_sensor_helper.h\n> @@ -34,15 +34,19 @@ protected:\n> \t\tAnalogueGainExponential,\n> \t};\n>\n> -\tstruct AnalogueGainConstants {\n> -\t\tAnalogueGainType type;\n> +\tstruct AnalogueGainLinearConstants {\n> \t\tint16_t m0;\n> \t\tint16_t c0;\n> \t\tint16_t m1;\n> \t\tint16_t c1;\n> \t};\n>\n> -\tAnalogueGainConstants analogueGainConstants_;\n> +\tunion AnalogueGainConstants {\n> +\t\tAnalogueGainLinearConstants linear;\n> +\t};\n> +\n> +\tAnalogueGainType gainType_;\n> +\tAnalogueGainConstants gainConstants_;\n>\n> private:\n> \tLIBCAMERA_DISABLE_COPY_AND_MOVE(CameraSensorHelper)\n> --\n> Regards,\n>\n> Laurent Pinchart\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 12812C3256\n\tfor ;\n\tMon, 28 Mar 2022 13:27:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6BAC060135;\n\tMon, 28 Mar 2022 15:27:09 +0200 (CEST)","from relay12.mail.gandi.net (relay12.mail.gandi.net\n\t[IPv6:2001:4b98:dc4:8::232])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2E6C560135\n\tfor ;\n\tMon, 28 Mar 2022 15:27:08 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id B30B4200009;\n\tMon, 28 Mar 2022 13:27:07 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1648474029;\n\tbh=qCGrnchbqt7W521tKPW/tK+PmWzSt0fXAHS2KxI4vbg=;\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=EnNzq7o7UtJosxsjmFoReBEZ0sRK/j2nWfIT+Zz/ib7yQejQ5OfepVXov0c+vlEqN\n\tbv4v7PB+JWemEYLIQJMvuDxQ2ANLo9BFxND+80YnSL2NF9jkfrH0IYjz75zgoCARvx\n\tE0pVtd0PnWMoSDWkY4l4kWfN91cTLCw4lS8vYRizkQQAJucK0JPbiLDtTEWSyopUah\n\tYAEyjC1KXIp5vNVjcQh4rwgWv+WtbHJESw9rkeT15CWenBdozwZPSQh5eM35k4CU5i\n\tkO7LvVG/pjYs4F/kQiyNlmE42jKWNsLkgYyb4SnZ8R4lhWtN5KNmdeqrnKsyLdHzdf\n\ti3KwpAoL7V+0A==","Date":"Mon, 28 Mar 2022 15:27:06 +0200","To":"Laurent Pinchart ","Message-ID":"<20220328132706.h5lcus7axzm3owzo@uno.localdomain>","References":"<20220328120336.10834-1-laurent.pinchart@ideasonboard.com>\n\t<20220328120336.10834-2-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220328120336.10834-2-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 1/4] libipa: camera_sensor_helper:\n\tReorganize gain constants","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":22491,"web_url":"https://patchwork.libcamera.org/comment/22491/","msgid":"","date":"2022-03-28T16:27:51","subject":"Re: [libcamera-devel] [PATCH 1/4] libipa: camera_sensor_helper:\n\tReorganize gain constants","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Mon, Mar 28, 2022 at 03:27:06PM +0200, Jacopo Mondi wrote:\n> Hi Laurent\n> \n> On Mon, Mar 28, 2022 at 03:03:33PM +0300, Laurent Pinchart via libcamera-devel wrote:\n> > To prepare for other gain models than the linear model, store the gain\n> > constants in a union with per-model members. Due to the lack of\n> > designated initializer support in gcc with C++17, initializing a single\n> > complex structure that includes a union will be difficult. Split the\n> > gain model type to a separate variable to work around this issue.\n> \n> Looking ahead, this makes sense.\n> \n> Too bad having something like\n> \n> struct {\n> AnalogueGainType type_;\n> AnalogueGainConstants constants_;\n> } gain;\n> \n> makes initialization more complex, so separate fields are ok\n\nIt's the union in AnalogueGainConstants that messes it up :-S gcc has\npartial support of designated initializers in C++, it only accepts them\nwhen all the members are initialized by name (clang is happy with them,\nI wish there would be a gcc switch to enable them even with older C++\nstandards). We would need to switch to C++20 to have proper designated\ninitializers support with gcc, I don't think that an option.\n\n> Reviewed-by: Jacopo Mondi \n> \n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > src/ipa/libipa/camera_sensor_helper.cpp | 102 ++++++++++++++----------\n> > src/ipa/libipa/camera_sensor_helper.h | 10 ++-\n> > 2 files changed, 65 insertions(+), 47 deletions(-)\n> >\n> > diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp\n> > index c953def04fd7..714cd86f039f 100644\n> > --- a/src/ipa/libipa/camera_sensor_helper.cpp\n> > +++ b/src/ipa/libipa/camera_sensor_helper.cpp\n> > @@ -58,11 +58,13 @@ namespace ipa {\n> > */\n> > uint32_t CameraSensorHelper::gainCode(double gain) const\n> > {\n> > -\tASSERT(analogueGainConstants_.m0 == 0 || analogueGainConstants_.m1 == 0);\n> > -\tASSERT(analogueGainConstants_.type == AnalogueGainLinear);\n> > +\tconst AnalogueGainConstants &k = gainConstants_;\n> >\n> > -\treturn (analogueGainConstants_.c0 - analogueGainConstants_.c1 * gain) /\n> > -\t (analogueGainConstants_.m1 * gain - analogueGainConstants_.m0);\n> > +\tASSERT(gainType_ == AnalogueGainLinear);\n> > +\tASSERT(k.linear.m0 == 0 || k.linear.m1 == 0);\n> > +\n> > +\treturn (k.linear.c0 - k.linear.c1 * gain) /\n> > +\t (k.linear.m1 * gain - k.linear.m0);\n> > }\n> >\n> > /**\n> > @@ -80,11 +82,13 @@ uint32_t CameraSensorHelper::gainCode(double gain) const\n> > */\n> > double CameraSensorHelper::gain(uint32_t gainCode) const\n> > {\n> > -\tASSERT(analogueGainConstants_.m0 == 0 || analogueGainConstants_.m1 == 0);\n> > -\tASSERT(analogueGainConstants_.type == AnalogueGainLinear);\n> > +\tconst AnalogueGainConstants &k = gainConstants_;\n> >\n> > -\treturn (analogueGainConstants_.m0 * static_cast(gainCode) + analogueGainConstants_.c0) /\n> > -\t (analogueGainConstants_.m1 * static_cast(gainCode) + analogueGainConstants_.c1);\n> > +\tASSERT(gainType_ == AnalogueGainLinear);\n> > +\tASSERT(k.linear.m0 == 0 || k.linear.m1 == 0);\n> > +\n> > +\treturn (k.linear.m0 * static_cast(gainCode) + k.linear.c0) /\n> > +\t (k.linear.m1 * static_cast(gainCode) + k.linear.c1);\n> > }\n> >\n> > /**\n> > @@ -127,42 +131,45 @@ double CameraSensorHelper::gain(uint32_t gainCode) const\n> > * \\todo not implemented in libipa\n> > */\n> >\n> > +/**\n> > + * \\struct CameraSensorHelper::AnalogueGainLinearConstants\n> > + * \\brief Analogue gain constants for the linear gain model\n> > + *\n> > + * \\var CameraSensorHelper::AnalogueGainLinearConstants::m0\n> > + * \\brief Constant used in the linear gain coding/decoding\n> > + *\n> > + * \\note Either m0 or m1 shall be zero.\n> > + *\n> > + * \\var CameraSensorHelper::AnalogueGainLinearConstants::c0\n> > + * \\brief Constant used in the linear gain coding/decoding\n> > + *\n> > + * \\var CameraSensorHelper::AnalogueGainLinearConstants::m1\n> > + * \\brief Constant used in the linear gain coding/decoding\n> > + *\n> > + * \\note Either m0 or m1 shall be zero.\n> > + *\n> > + * \\var CameraSensorHelper::AnalogueGainLinearConstants::c1\n> > + * \\brief Constant used in the linear gain coding/decoding\n> > + */\n> > +\n> > /**\n> > * \\struct CameraSensorHelper::AnalogueGainConstants\n> > - * \\brief Analogue gain constants used for gain calculation\n> > - */\n> > -\n> > -/**\n> > - * \\var CameraSensorHelper::AnalogueGainConstants::type\n> > - * \\brief Analogue gain calculation mode\n> > - */\n> > -\n> > -/**\n> > - * \\var CameraSensorHelper::AnalogueGainConstants::m0\n> > - * \\brief Constant used in the analogue Gain coding/decoding\n> > + * \\brief Analogue gain model constants\n> > *\n> > - * \\note Either m0 or m1 shall be zero.\n> > - */\n> > -\n> > -/**\n> > - * \\var CameraSensorHelper::AnalogueGainConstants::c0\n> > - * \\brief Constant used in the analogue gain coding/decoding\n> > - */\n> > -\n> > -/**\n> > - * \\var CameraSensorHelper::AnalogueGainConstants::m1\n> > - * \\brief Constant used in the analogue gain coding/decoding\n> > + * This union stores the constants used to calculate the analogue gain. The\n> > + * CameraSensorHelper::gainType_ variable selects which union member is valid.\n> > *\n> > - * \\note Either m0 or m1 shall be zero.\n> > + * \\var CameraSensorHelper::AnalogueGainConstants::linear\n> > + * \\brief Constants for the linear gain model\n> > */\n> >\n> > /**\n> > - * \\var CameraSensorHelper::AnalogueGainConstants::c1\n> > - * \\brief Constant used in the analogue gain coding/decoding\n> > + * \\var CameraSensorHelper::gainType_\n> > + * \\brief The analogue gain model type\n> > */\n> >\n> > /**\n> > - * \\var CameraSensorHelper::analogueGainConstants_\n> > + * \\var CameraSensorHelper::gainConstants_\n> > * \\brief The analogue gain parameters used for calculation\n> > *\n> > * The analogue gain is calculated through a formula, and its parameters are\n> > @@ -290,7 +297,8 @@ class CameraSensorHelperImx219 : public CameraSensorHelper\n> > public:\n> > \tCameraSensorHelperImx219()\n> > \t{\n> > -\t\tanalogueGainConstants_ = { AnalogueGainLinear, 0, 256, -1, 256 };\n> > +\t\tgainType_ = AnalogueGainLinear;\n> > +\t\tgainConstants_.linear = { 0, 256, -1, 256 };\n> > \t}\n> > };\n> > REGISTER_CAMERA_SENSOR_HELPER(\"imx219\", CameraSensorHelperImx219)\n> > @@ -298,10 +306,11 @@ REGISTER_CAMERA_SENSOR_HELPER(\"imx219\", CameraSensorHelperImx219)\n> > class CameraSensorHelperImx258 : public CameraSensorHelper\n> > {\n> > public:\n> > - CameraSensorHelperImx258()\n> > - {\n> > - analogueGainConstants_ = { AnalogueGainLinear, 0, 512, -1, 512 };\n> > - }\n> > +\tCameraSensorHelperImx258()\n> > +\t{\n> > +\t\tgainType_ = AnalogueGainLinear;\n> > +\t\tgainConstants_.linear = { 0, 512, -1, 512 };\n> > +\t}\n> > };\n> > REGISTER_CAMERA_SENSOR_HELPER(\"imx258\", CameraSensorHelperImx258)\n> >\n> > @@ -310,7 +319,8 @@ class CameraSensorHelperOv2740 : public CameraSensorHelper\n> > public:\n> > \tCameraSensorHelperOv2740()\n> > \t{\n> > -\t\tanalogueGainConstants_ = { AnalogueGainLinear, 1, 0, 0, 128 };\n> > +\t\tgainType_ = AnalogueGainLinear;\n> > +\t\tgainConstants_.linear = { 1, 0, 0, 128 };\n> > \t}\n> > };\n> > REGISTER_CAMERA_SENSOR_HELPER(\"ov2740\", CameraSensorHelperOv2740)\n> > @@ -320,7 +330,8 @@ class CameraSensorHelperOv5670 : public CameraSensorHelper\n> > public:\n> > \tCameraSensorHelperOv5670()\n> > \t{\n> > -\t\tanalogueGainConstants_ = { AnalogueGainLinear, 1, 0, 0, 128 };\n> > +\t\tgainType_ = AnalogueGainLinear;\n> > +\t\tgainConstants_.linear = { 1, 0, 0, 128 };\n> > \t}\n> > };\n> > REGISTER_CAMERA_SENSOR_HELPER(\"ov5670\", CameraSensorHelperOv5670)\n> > @@ -330,7 +341,8 @@ class CameraSensorHelperOv5693 : public CameraSensorHelper\n> > public:\n> > \tCameraSensorHelperOv5693()\n> > \t{\n> > -\t\tanalogueGainConstants_ = { AnalogueGainLinear, 1, 0, 0, 16 };\n> > +\t\tgainType_ = AnalogueGainLinear;\n> > +\t\tgainConstants_.linear = { 1, 0, 0, 16 };\n> > \t}\n> > };\n> > REGISTER_CAMERA_SENSOR_HELPER(\"ov5693\", CameraSensorHelperOv5693)\n> > @@ -340,7 +352,8 @@ class CameraSensorHelperOv8865 : public CameraSensorHelper\n> > public:\n> > \tCameraSensorHelperOv8865()\n> > \t{\n> > -\t\tanalogueGainConstants_ = { AnalogueGainLinear, 1, 0, 0, 128 };\n> > +\t\tgainType_ = AnalogueGainLinear;\n> > +\t\tgainConstants_.linear = { 1, 0, 0, 128 };\n> > \t}\n> > };\n> > REGISTER_CAMERA_SENSOR_HELPER(\"ov8865\", CameraSensorHelperOv8865)\n> > @@ -350,7 +363,8 @@ class CameraSensorHelperOv13858 : public CameraSensorHelper\n> > public:\n> > \tCameraSensorHelperOv13858()\n> > \t{\n> > -\t\tanalogueGainConstants_ = { AnalogueGainLinear, 1, 0, 0, 128 };\n> > +\t\tgainType_ = AnalogueGainLinear;\n> > +\t\tgainConstants_.linear = { 1, 0, 0, 128 };\n> > \t}\n> > };\n> > REGISTER_CAMERA_SENSOR_HELPER(\"ov13858\", CameraSensorHelperOv13858)\n> > diff --git a/src/ipa/libipa/camera_sensor_helper.h b/src/ipa/libipa/camera_sensor_helper.h\n> > index 26adfcb5955f..6b96520ba601 100644\n> > --- a/src/ipa/libipa/camera_sensor_helper.h\n> > +++ b/src/ipa/libipa/camera_sensor_helper.h\n> > @@ -34,15 +34,19 @@ protected:\n> > \t\tAnalogueGainExponential,\n> > \t};\n> >\n> > -\tstruct AnalogueGainConstants {\n> > -\t\tAnalogueGainType type;\n> > +\tstruct AnalogueGainLinearConstants {\n> > \t\tint16_t m0;\n> > \t\tint16_t c0;\n> > \t\tint16_t m1;\n> > \t\tint16_t c1;\n> > \t};\n> >\n> > -\tAnalogueGainConstants analogueGainConstants_;\n> > +\tunion AnalogueGainConstants {\n> > +\t\tAnalogueGainLinearConstants linear;\n> > +\t};\n> > +\n> > +\tAnalogueGainType gainType_;\n> > +\tAnalogueGainConstants gainConstants_;\n> >\n> > private:\n> > \tLIBCAMERA_DISABLE_COPY_AND_MOVE(CameraSensorHelper)","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 5126FC3256\n\tfor ;\n\tMon, 28 Mar 2022 16:27:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BF7F3601F5;\n\tMon, 28 Mar 2022 18:27:55 +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 7026860135\n\tfor ;\n\tMon, 28 Mar 2022 18:27:54 +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 CBB9C2F7;\n\tMon, 28 Mar 2022 18:27:53 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1648484875;\n\tbh=Ur9FnT6V1nWk9H67dU1/J4G4n8PNXOXFqVLt2F1L+GE=;\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=nNxYrzKqGyCRxmBnqmzMpCxYgu9jXPh+hmkZAl/udhG/gjB7kEWA/GdUxPplJ/YB+\n\t/bicZxzOSOKM3G/vkmFqv2G0CaI968IZO5nwk8Nxh1UKbxbl3dddjjNIIEt59h3DML\n\t33lf1EjLQT7qFYYhRpS1cJDmgfLAYW2uJgsYhwp6zfLUYcfof/c29v3t/PDm5E4bVL\n\tO8JKL5qRWKVu9aB9ctqfWhlu07zQw6jNg+Sgi3x6JwpAVdSBKPX6HdX1m4J7MrLIHV\n\tBhvye9iR8xU/XNNZ/yi7ZpqFyE5nByklgrcGhgPk7wyLn0fTyqALwds/BZMJBJDyAE\n\tE6j8aEb+remxw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1648484874;\n\tbh=Ur9FnT6V1nWk9H67dU1/J4G4n8PNXOXFqVLt2F1L+GE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=aIdjTQXPo+jEUJ2wDsLOndd7/9ogypMVoRTxynI7+suHMIzkfRKwjvCo3gnF4j7t1\n\trhG3wmadQX/il83LzKz9bEyhsg5srXTqmkUC9u9sbd77q4j8f8MjNgwQWwDQSASh4b\n\tvB0ZU+XIz/syw70KTATyx6BB5WJTmVyoFLRDtYUs="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"aIdjTQXP\"; dkim-atps=neutral","Date":"Mon, 28 Mar 2022 19:27:51 +0300","To":"Jacopo Mondi ","Message-ID":"","References":"<20220328120336.10834-1-laurent.pinchart@ideasonboard.com>\n\t<20220328120336.10834-2-laurent.pinchart@ideasonboard.com>\n\t<20220328132706.h5lcus7axzm3owzo@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220328132706.h5lcus7axzm3owzo@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH 1/4] libipa: camera_sensor_helper:\n\tReorganize gain constants","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@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22521,"web_url":"https://patchwork.libcamera.org/comment/22521/","msgid":"","date":"2022-03-30T07:53:11","subject":"Re: [libcamera-devel] [PATCH 1/4] libipa: camera_sensor_helper:\n\tReorganize gain constants","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Laurent\n\nThank you for the patch\n\nOn 3/28/22 17:33, Laurent Pinchart via libcamera-devel wrote:\n> To prepare for other gain models than the linear model, store the gain\n> constants in a union with per-model members. Due to the lack of\n> designated initializer support in gcc with C++17, initializing a single\n> complex structure that includes a union will be difficult. Split the\n> gain model type to a separate variable to work around this issue.\n>\n> Signed-off-by: Laurent Pinchart \n\n\nReviewed-by: Umang Jain \n\n> ---\n> src/ipa/libipa/camera_sensor_helper.cpp | 102 ++++++++++++++----------\n> src/ipa/libipa/camera_sensor_helper.h | 10 ++-\n> 2 files changed, 65 insertions(+), 47 deletions(-)\n>\n> diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp\n> index c953def04fd7..714cd86f039f 100644\n> --- a/src/ipa/libipa/camera_sensor_helper.cpp\n> +++ b/src/ipa/libipa/camera_sensor_helper.cpp\n> @@ -58,11 +58,13 @@ namespace ipa {\n> */\n> uint32_t CameraSensorHelper::gainCode(double gain) const\n> {\n> -\tASSERT(analogueGainConstants_.m0 == 0 || analogueGainConstants_.m1 == 0);\n> -\tASSERT(analogueGainConstants_.type == AnalogueGainLinear);\n> +\tconst AnalogueGainConstants &k = gainConstants_;\n> \n> -\treturn (analogueGainConstants_.c0 - analogueGainConstants_.c1 * gain) /\n> -\t (analogueGainConstants_.m1 * gain - analogueGainConstants_.m0);\n> +\tASSERT(gainType_ == AnalogueGainLinear);\n> +\tASSERT(k.linear.m0 == 0 || k.linear.m1 == 0);\n> +\n> +\treturn (k.linear.c0 - k.linear.c1 * gain) /\n> +\t (k.linear.m1 * gain - k.linear.m0);\n> }\n> \n> /**\n> @@ -80,11 +82,13 @@ uint32_t CameraSensorHelper::gainCode(double gain) const\n> */\n> double CameraSensorHelper::gain(uint32_t gainCode) const\n> {\n> -\tASSERT(analogueGainConstants_.m0 == 0 || analogueGainConstants_.m1 == 0);\n> -\tASSERT(analogueGainConstants_.type == AnalogueGainLinear);\n> +\tconst AnalogueGainConstants &k = gainConstants_;\n> \n> -\treturn (analogueGainConstants_.m0 * static_cast(gainCode) + analogueGainConstants_.c0) /\n> -\t (analogueGainConstants_.m1 * static_cast(gainCode) + analogueGainConstants_.c1);\n> +\tASSERT(gainType_ == AnalogueGainLinear);\n> +\tASSERT(k.linear.m0 == 0 || k.linear.m1 == 0);\n> +\n> +\treturn (k.linear.m0 * static_cast(gainCode) + k.linear.c0) /\n> +\t (k.linear.m1 * static_cast(gainCode) + k.linear.c1);\n> }\n> \n> /**\n> @@ -127,42 +131,45 @@ double CameraSensorHelper::gain(uint32_t gainCode) const\n> * \\todo not implemented in libipa\n> */\n> \n> +/**\n> + * \\struct CameraSensorHelper::AnalogueGainLinearConstants\n> + * \\brief Analogue gain constants for the linear gain model\n> + *\n> + * \\var CameraSensorHelper::AnalogueGainLinearConstants::m0\n> + * \\brief Constant used in the linear gain coding/decoding\n> + *\n> + * \\note Either m0 or m1 shall be zero.\n> + *\n> + * \\var CameraSensorHelper::AnalogueGainLinearConstants::c0\n> + * \\brief Constant used in the linear gain coding/decoding\n> + *\n> + * \\var CameraSensorHelper::AnalogueGainLinearConstants::m1\n> + * \\brief Constant used in the linear gain coding/decoding\n> + *\n> + * \\note Either m0 or m1 shall be zero.\n> + *\n> + * \\var CameraSensorHelper::AnalogueGainLinearConstants::c1\n> + * \\brief Constant used in the linear gain coding/decoding\n> + */\n> +\n> /**\n> * \\struct CameraSensorHelper::AnalogueGainConstants\n> - * \\brief Analogue gain constants used for gain calculation\n> - */\n> -\n> -/**\n> - * \\var CameraSensorHelper::AnalogueGainConstants::type\n> - * \\brief Analogue gain calculation mode\n> - */\n> -\n> -/**\n> - * \\var CameraSensorHelper::AnalogueGainConstants::m0\n> - * \\brief Constant used in the analogue Gain coding/decoding\n> + * \\brief Analogue gain model constants\n> *\n> - * \\note Either m0 or m1 shall be zero.\n> - */\n> -\n> -/**\n> - * \\var CameraSensorHelper::AnalogueGainConstants::c0\n> - * \\brief Constant used in the analogue gain coding/decoding\n> - */\n> -\n> -/**\n> - * \\var CameraSensorHelper::AnalogueGainConstants::m1\n> - * \\brief Constant used in the analogue gain coding/decoding\n> + * This union stores the constants used to calculate the analogue gain. The\n> + * CameraSensorHelper::gainType_ variable selects which union member is valid.\n> *\n> - * \\note Either m0 or m1 shall be zero.\n> + * \\var CameraSensorHelper::AnalogueGainConstants::linear\n> + * \\brief Constants for the linear gain model\n> */\n> \n> /**\n> - * \\var CameraSensorHelper::AnalogueGainConstants::c1\n> - * \\brief Constant used in the analogue gain coding/decoding\n> + * \\var CameraSensorHelper::gainType_\n> + * \\brief The analogue gain model type\n> */\n> \n> /**\n> - * \\var CameraSensorHelper::analogueGainConstants_\n> + * \\var CameraSensorHelper::gainConstants_\n> * \\brief The analogue gain parameters used for calculation\n> *\n> * The analogue gain is calculated through a formula, and its parameters are\n> @@ -290,7 +297,8 @@ class CameraSensorHelperImx219 : public CameraSensorHelper\n> public:\n> \tCameraSensorHelperImx219()\n> \t{\n> -\t\tanalogueGainConstants_ = { AnalogueGainLinear, 0, 256, -1, 256 };\n> +\t\tgainType_ = AnalogueGainLinear;\n> +\t\tgainConstants_.linear = { 0, 256, -1, 256 };\n> \t}\n> };\n> REGISTER_CAMERA_SENSOR_HELPER(\"imx219\", CameraSensorHelperImx219)\n> @@ -298,10 +306,11 @@ REGISTER_CAMERA_SENSOR_HELPER(\"imx219\", CameraSensorHelperImx219)\n> class CameraSensorHelperImx258 : public CameraSensorHelper\n> {\n> public:\n> - CameraSensorHelperImx258()\n> - {\n> - analogueGainConstants_ = { AnalogueGainLinear, 0, 512, -1, 512 };\n> - }\n> +\tCameraSensorHelperImx258()\n> +\t{\n> +\t\tgainType_ = AnalogueGainLinear;\n> +\t\tgainConstants_.linear = { 0, 512, -1, 512 };\n> +\t}\n> };\n> REGISTER_CAMERA_SENSOR_HELPER(\"imx258\", CameraSensorHelperImx258)\n> \n> @@ -310,7 +319,8 @@ class CameraSensorHelperOv2740 : public CameraSensorHelper\n> public:\n> \tCameraSensorHelperOv2740()\n> \t{\n> -\t\tanalogueGainConstants_ = { AnalogueGainLinear, 1, 0, 0, 128 };\n> +\t\tgainType_ = AnalogueGainLinear;\n> +\t\tgainConstants_.linear = { 1, 0, 0, 128 };\n> \t}\n> };\n> REGISTER_CAMERA_SENSOR_HELPER(\"ov2740\", CameraSensorHelperOv2740)\n> @@ -320,7 +330,8 @@ class CameraSensorHelperOv5670 : public CameraSensorHelper\n> public:\n> \tCameraSensorHelperOv5670()\n> \t{\n> -\t\tanalogueGainConstants_ = { AnalogueGainLinear, 1, 0, 0, 128 };\n> +\t\tgainType_ = AnalogueGainLinear;\n> +\t\tgainConstants_.linear = { 1, 0, 0, 128 };\n> \t}\n> };\n> REGISTER_CAMERA_SENSOR_HELPER(\"ov5670\", CameraSensorHelperOv5670)\n> @@ -330,7 +341,8 @@ class CameraSensorHelperOv5693 : public CameraSensorHelper\n> public:\n> \tCameraSensorHelperOv5693()\n> \t{\n> -\t\tanalogueGainConstants_ = { AnalogueGainLinear, 1, 0, 0, 16 };\n> +\t\tgainType_ = AnalogueGainLinear;\n> +\t\tgainConstants_.linear = { 1, 0, 0, 16 };\n> \t}\n> };\n> REGISTER_CAMERA_SENSOR_HELPER(\"ov5693\", CameraSensorHelperOv5693)\n> @@ -340,7 +352,8 @@ class CameraSensorHelperOv8865 : public CameraSensorHelper\n> public:\n> \tCameraSensorHelperOv8865()\n> \t{\n> -\t\tanalogueGainConstants_ = { AnalogueGainLinear, 1, 0, 0, 128 };\n> +\t\tgainType_ = AnalogueGainLinear;\n> +\t\tgainConstants_.linear = { 1, 0, 0, 128 };\n> \t}\n> };\n> REGISTER_CAMERA_SENSOR_HELPER(\"ov8865\", CameraSensorHelperOv8865)\n> @@ -350,7 +363,8 @@ class CameraSensorHelperOv13858 : public CameraSensorHelper\n> public:\n> \tCameraSensorHelperOv13858()\n> \t{\n> -\t\tanalogueGainConstants_ = { AnalogueGainLinear, 1, 0, 0, 128 };\n> +\t\tgainType_ = AnalogueGainLinear;\n> +\t\tgainConstants_.linear = { 1, 0, 0, 128 };\n> \t}\n> };\n> REGISTER_CAMERA_SENSOR_HELPER(\"ov13858\", CameraSensorHelperOv13858)\n> diff --git a/src/ipa/libipa/camera_sensor_helper.h b/src/ipa/libipa/camera_sensor_helper.h\n> index 26adfcb5955f..6b96520ba601 100644\n> --- a/src/ipa/libipa/camera_sensor_helper.h\n> +++ b/src/ipa/libipa/camera_sensor_helper.h\n> @@ -34,15 +34,19 @@ protected:\n> \t\tAnalogueGainExponential,\n> \t};\n> \n> -\tstruct AnalogueGainConstants {\n> -\t\tAnalogueGainType type;\n> +\tstruct AnalogueGainLinearConstants {\n> \t\tint16_t m0;\n> \t\tint16_t c0;\n> \t\tint16_t m1;\n> \t\tint16_t c1;\n> \t};\n> \n> -\tAnalogueGainConstants analogueGainConstants_;\n> +\tunion AnalogueGainConstants {\n> +\t\tAnalogueGainLinearConstants linear;\n> +\t};\n> +\n> +\tAnalogueGainType gainType_;\n> +\tAnalogueGainConstants gainConstants_;\n> \n> private:\n> \tLIBCAMERA_DISABLE_COPY_AND_MOVE(CameraSensorHelper)","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 9A375C0F1B\n\tfor ;\n\tWed, 30 Mar 2022 07:53:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EB38D633A7;\n\tWed, 30 Mar 2022 09:53:23 +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 6621260397\n\tfor ;\n\tWed, 30 Mar 2022 09:53:22 +0200 (CEST)","from [192.168.1.105] (unknown [103.74.73.208])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 291EFE52;\n\tWed, 30 Mar 2022 09:53:19 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1648626804;\n\tbh=nzYjwdBR8AR3smqUzqQz0oCRYLo1nszZx3wXRDKdWw8=;\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:\n\tFrom;\n\tb=4D/8+EAkMDdh3C5DffnUG9nZE0KNPpGbjv3hb+WN33DYhlExSNvX3Q/lpbwVm5gx2\n\tZIUD7jNO1rRKIQdAAtkuk3Oelshllaw4oyiIurGr88Dyqz01L4nzrBBYrfFAOlYqWG\n\tJ71xuyYq64OKVyy2oY26R08hfgt0J0Y23Mv9tftW+pCz9n8A8FH+FZ+u5JQGwQCZES\n\tIJT1MYQNwExdUxptAvv6VO9nWt/P5IhiHwcDQTRGrHFVaDrP/A/4CVgnWSryJ96FF6\n\tnhgwYx0eokSAjHYvkwqLEepLCmSoLtMwIF/TsRVMX614+Hy3zZS490b34fwtP4PQSh\n\tFpTFuTGYGOh4A==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1648626802;\n\tbh=nzYjwdBR8AR3smqUzqQz0oCRYLo1nszZx3wXRDKdWw8=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=om3SmFprWkG7HLVmN7vd6yZt3TSFtX83wEGaFXTeQPm38MlGD4wO7e9YoPtlvJvZp\n\tPvfNM12u/dykbn/VO1aXJtkMJXS7L7QtMkzUwNWiI4IKz7PAXQrWT97Y9ugzs2S+13\n\tmtyJ7foBtP1H/dOTwzzQ6ZiFF+deRQtVSuVFpE0s="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"om3SmFpr\"; dkim-atps=neutral","Message-ID":"","Date":"Wed, 30 Mar 2022 13:23:11 +0530","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.4.1","Content-Language":"en-US","To":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20220328120336.10834-1-laurent.pinchart@ideasonboard.com>\n\t<20220328120336.10834-2-laurent.pinchart@ideasonboard.com>","In-Reply-To":"<20220328120336.10834-2-laurent.pinchart@ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH 1/4] libipa: camera_sensor_helper:\n\tReorganize gain constants","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":"Umang Jain via libcamera-devel ","Reply-To":"Umang Jain ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":22522,"web_url":"https://patchwork.libcamera.org/comment/22522/","msgid":"<20220330083922.GA3237525@pyrite.rasen.tech>","date":"2022-03-30T08:39:22","subject":"Re: [libcamera-devel] [PATCH 1/4] libipa: camera_sensor_helper:\n\tReorganize gain constants","submitter":{"id":97,"url":"https://patchwork.libcamera.org/api/people/97/","name":"Nicolas Dufresne via libcamera-devel","email":"libcamera-devel@lists.libcamera.org"},"content":"Hi Laurent,\n\nOn Mon, Mar 28, 2022 at 03:03:33PM +0300, Laurent Pinchart via libcamera-devel wrote:\n> To prepare for other gain models than the linear model, store the gain\n> constants in a union with per-model members. Due to the lack of\n> designated initializer support in gcc with C++17, initializing a single\n> complex structure that includes a union will be difficult. Split the\n> gain model type to a separate variable to work around this issue.\n> \n> Signed-off-by: Laurent Pinchart \n\nReviewed-by: Paul Elder \n\n> ---\n> src/ipa/libipa/camera_sensor_helper.cpp | 102 ++++++++++++++----------\n> src/ipa/libipa/camera_sensor_helper.h | 10 ++-\n> 2 files changed, 65 insertions(+), 47 deletions(-)\n> \n> diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp\n> index c953def04fd7..714cd86f039f 100644\n> --- a/src/ipa/libipa/camera_sensor_helper.cpp\n> +++ b/src/ipa/libipa/camera_sensor_helper.cpp\n> @@ -58,11 +58,13 @@ namespace ipa {\n> */\n> uint32_t CameraSensorHelper::gainCode(double gain) const\n> {\n> -\tASSERT(analogueGainConstants_.m0 == 0 || analogueGainConstants_.m1 == 0);\n> -\tASSERT(analogueGainConstants_.type == AnalogueGainLinear);\n> +\tconst AnalogueGainConstants &k = gainConstants_;\n> \n> -\treturn (analogueGainConstants_.c0 - analogueGainConstants_.c1 * gain) /\n> -\t (analogueGainConstants_.m1 * gain - analogueGainConstants_.m0);\n> +\tASSERT(gainType_ == AnalogueGainLinear);\n> +\tASSERT(k.linear.m0 == 0 || k.linear.m1 == 0);\n> +\n> +\treturn (k.linear.c0 - k.linear.c1 * gain) /\n> +\t (k.linear.m1 * gain - k.linear.m0);\n> }\n> \n> /**\n> @@ -80,11 +82,13 @@ uint32_t CameraSensorHelper::gainCode(double gain) const\n> */\n> double CameraSensorHelper::gain(uint32_t gainCode) const\n> {\n> -\tASSERT(analogueGainConstants_.m0 == 0 || analogueGainConstants_.m1 == 0);\n> -\tASSERT(analogueGainConstants_.type == AnalogueGainLinear);\n> +\tconst AnalogueGainConstants &k = gainConstants_;\n> \n> -\treturn (analogueGainConstants_.m0 * static_cast(gainCode) + analogueGainConstants_.c0) /\n> -\t (analogueGainConstants_.m1 * static_cast(gainCode) + analogueGainConstants_.c1);\n> +\tASSERT(gainType_ == AnalogueGainLinear);\n> +\tASSERT(k.linear.m0 == 0 || k.linear.m1 == 0);\n> +\n> +\treturn (k.linear.m0 * static_cast(gainCode) + k.linear.c0) /\n> +\t (k.linear.m1 * static_cast(gainCode) + k.linear.c1);\n> }\n> \n> /**\n> @@ -127,42 +131,45 @@ double CameraSensorHelper::gain(uint32_t gainCode) const\n> * \\todo not implemented in libipa\n> */\n> \n> +/**\n> + * \\struct CameraSensorHelper::AnalogueGainLinearConstants\n> + * \\brief Analogue gain constants for the linear gain model\n> + *\n> + * \\var CameraSensorHelper::AnalogueGainLinearConstants::m0\n> + * \\brief Constant used in the linear gain coding/decoding\n> + *\n> + * \\note Either m0 or m1 shall be zero.\n> + *\n> + * \\var CameraSensorHelper::AnalogueGainLinearConstants::c0\n> + * \\brief Constant used in the linear gain coding/decoding\n> + *\n> + * \\var CameraSensorHelper::AnalogueGainLinearConstants::m1\n> + * \\brief Constant used in the linear gain coding/decoding\n> + *\n> + * \\note Either m0 or m1 shall be zero.\n> + *\n> + * \\var CameraSensorHelper::AnalogueGainLinearConstants::c1\n> + * \\brief Constant used in the linear gain coding/decoding\n> + */\n> +\n> /**\n> * \\struct CameraSensorHelper::AnalogueGainConstants\n> - * \\brief Analogue gain constants used for gain calculation\n> - */\n> -\n> -/**\n> - * \\var CameraSensorHelper::AnalogueGainConstants::type\n> - * \\brief Analogue gain calculation mode\n> - */\n> -\n> -/**\n> - * \\var CameraSensorHelper::AnalogueGainConstants::m0\n> - * \\brief Constant used in the analogue Gain coding/decoding\n> + * \\brief Analogue gain model constants\n> *\n> - * \\note Either m0 or m1 shall be zero.\n> - */\n> -\n> -/**\n> - * \\var CameraSensorHelper::AnalogueGainConstants::c0\n> - * \\brief Constant used in the analogue gain coding/decoding\n> - */\n> -\n> -/**\n> - * \\var CameraSensorHelper::AnalogueGainConstants::m1\n> - * \\brief Constant used in the analogue gain coding/decoding\n> + * This union stores the constants used to calculate the analogue gain. The\n> + * CameraSensorHelper::gainType_ variable selects which union member is valid.\n> *\n> - * \\note Either m0 or m1 shall be zero.\n> + * \\var CameraSensorHelper::AnalogueGainConstants::linear\n> + * \\brief Constants for the linear gain model\n> */\n> \n> /**\n> - * \\var CameraSensorHelper::AnalogueGainConstants::c1\n> - * \\brief Constant used in the analogue gain coding/decoding\n> + * \\var CameraSensorHelper::gainType_\n> + * \\brief The analogue gain model type\n> */\n> \n> /**\n> - * \\var CameraSensorHelper::analogueGainConstants_\n> + * \\var CameraSensorHelper::gainConstants_\n> * \\brief The analogue gain parameters used for calculation\n> *\n> * The analogue gain is calculated through a formula, and its parameters are\n> @@ -290,7 +297,8 @@ class CameraSensorHelperImx219 : public CameraSensorHelper\n> public:\n> \tCameraSensorHelperImx219()\n> \t{\n> -\t\tanalogueGainConstants_ = { AnalogueGainLinear, 0, 256, -1, 256 };\n> +\t\tgainType_ = AnalogueGainLinear;\n> +\t\tgainConstants_.linear = { 0, 256, -1, 256 };\n> \t}\n> };\n> REGISTER_CAMERA_SENSOR_HELPER(\"imx219\", CameraSensorHelperImx219)\n> @@ -298,10 +306,11 @@ REGISTER_CAMERA_SENSOR_HELPER(\"imx219\", CameraSensorHelperImx219)\n> class CameraSensorHelperImx258 : public CameraSensorHelper\n> {\n> public:\n> - CameraSensorHelperImx258()\n> - {\n> - analogueGainConstants_ = { AnalogueGainLinear, 0, 512, -1, 512 };\n> - }\n> +\tCameraSensorHelperImx258()\n> +\t{\n> +\t\tgainType_ = AnalogueGainLinear;\n> +\t\tgainConstants_.linear = { 0, 512, -1, 512 };\n> +\t}\n> };\n> REGISTER_CAMERA_SENSOR_HELPER(\"imx258\", CameraSensorHelperImx258)\n> \n> @@ -310,7 +319,8 @@ class CameraSensorHelperOv2740 : public CameraSensorHelper\n> public:\n> \tCameraSensorHelperOv2740()\n> \t{\n> -\t\tanalogueGainConstants_ = { AnalogueGainLinear, 1, 0, 0, 128 };\n> +\t\tgainType_ = AnalogueGainLinear;\n> +\t\tgainConstants_.linear = { 1, 0, 0, 128 };\n> \t}\n> };\n> REGISTER_CAMERA_SENSOR_HELPER(\"ov2740\", CameraSensorHelperOv2740)\n> @@ -320,7 +330,8 @@ class CameraSensorHelperOv5670 : public CameraSensorHelper\n> public:\n> \tCameraSensorHelperOv5670()\n> \t{\n> -\t\tanalogueGainConstants_ = { AnalogueGainLinear, 1, 0, 0, 128 };\n> +\t\tgainType_ = AnalogueGainLinear;\n> +\t\tgainConstants_.linear = { 1, 0, 0, 128 };\n> \t}\n> };\n> REGISTER_CAMERA_SENSOR_HELPER(\"ov5670\", CameraSensorHelperOv5670)\n> @@ -330,7 +341,8 @@ class CameraSensorHelperOv5693 : public CameraSensorHelper\n> public:\n> \tCameraSensorHelperOv5693()\n> \t{\n> -\t\tanalogueGainConstants_ = { AnalogueGainLinear, 1, 0, 0, 16 };\n> +\t\tgainType_ = AnalogueGainLinear;\n> +\t\tgainConstants_.linear = { 1, 0, 0, 16 };\n> \t}\n> };\n> REGISTER_CAMERA_SENSOR_HELPER(\"ov5693\", CameraSensorHelperOv5693)\n> @@ -340,7 +352,8 @@ class CameraSensorHelperOv8865 : public CameraSensorHelper\n> public:\n> \tCameraSensorHelperOv8865()\n> \t{\n> -\t\tanalogueGainConstants_ = { AnalogueGainLinear, 1, 0, 0, 128 };\n> +\t\tgainType_ = AnalogueGainLinear;\n> +\t\tgainConstants_.linear = { 1, 0, 0, 128 };\n> \t}\n> };\n> REGISTER_CAMERA_SENSOR_HELPER(\"ov8865\", CameraSensorHelperOv8865)\n> @@ -350,7 +363,8 @@ class CameraSensorHelperOv13858 : public CameraSensorHelper\n> public:\n> \tCameraSensorHelperOv13858()\n> \t{\n> -\t\tanalogueGainConstants_ = { AnalogueGainLinear, 1, 0, 0, 128 };\n> +\t\tgainType_ = AnalogueGainLinear;\n> +\t\tgainConstants_.linear = { 1, 0, 0, 128 };\n> \t}\n> };\n> REGISTER_CAMERA_SENSOR_HELPER(\"ov13858\", CameraSensorHelperOv13858)\n> diff --git a/src/ipa/libipa/camera_sensor_helper.h b/src/ipa/libipa/camera_sensor_helper.h\n> index 26adfcb5955f..6b96520ba601 100644\n> --- a/src/ipa/libipa/camera_sensor_helper.h\n> +++ b/src/ipa/libipa/camera_sensor_helper.h\n> @@ -34,15 +34,19 @@ protected:\n> \t\tAnalogueGainExponential,\n> \t};\n> \n> -\tstruct AnalogueGainConstants {\n> -\t\tAnalogueGainType type;\n> +\tstruct AnalogueGainLinearConstants {\n> \t\tint16_t m0;\n> \t\tint16_t c0;\n> \t\tint16_t m1;\n> \t\tint16_t c1;\n> \t};\n> \n> -\tAnalogueGainConstants analogueGainConstants_;\n> +\tunion AnalogueGainConstants {\n> +\t\tAnalogueGainLinearConstants linear;\n> +\t};\n> +\n> +\tAnalogueGainType gainType_;\n> +\tAnalogueGainConstants gainConstants_;\n> \n> private:\n> \tLIBCAMERA_DISABLE_COPY_AND_MOVE(CameraSensorHelper)\n> -- \n> Regards,\n> \n> Laurent Pinchart\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 6E70AC3256\n\tfor ;\n\tWed, 30 Mar 2022 08:39:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B187165634;\n\tWed, 30 Mar 2022 10:39:32 +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 B2C8A60397\n\tfor ;\n\tWed, 30 Mar 2022 10:39:31 +0200 (CEST)","from pyrite.rasen.tech (h175-177-042-148.catv02.itscom.jp\n\t[175.177.42.148])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5FD9959D;\n\tWed, 30 Mar 2022 10:39:30 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1648629572;\n\tbh=MmYjlUgrU+6j54EInEGqnpn8uH9Gn56ak59xcyY4G4w=;\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=ViiDdQygzkVd93N7I0szzdeyU5ZrSf4tu4kLjHZqxXjZ+Cq159VIa6bhVpmGg4xWS\n\tHZ+Zp+Mkoj+NTExsyHORolQLFRlNRIBKMCvLzRnkeOz5UwYSNYLhQzEb+dAaAVN+CJ\n\tnd7RmOChEZpfbpwmt6pu3qFe32ZRz/RkaFTgWnxUYz14D9V8SCmH05UnerHy5DdpUL\n\tq5VHHyZN3jDzY1QYXHqjQzBIlMttHF3kL2ZCEgacut71i9hog0GLtpxwkXhNJp20lq\n\tvNGdPm6lcBt+5cqAsvlQ1YGrxZSEucAiKFfx29w8snyzGunp3C9yKeP15vQg1pNgqN\n\tRuJCkWHE5QPCA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1648629571;\n\tbh=MmYjlUgrU+6j54EInEGqnpn8uH9Gn56ak59xcyY4G4w=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=YjRP5gVAOZCSUCwRFC3UUQf6+ptchkoPZtrUCECpeJjsH5L/Al1bRNkeYJ9kT0YhD\n\tyaWAfntKLvGpn8WPEOCpr71TtiomJ6ccB+wBFIINDDTwgA35FKkzxy6MFG5XsbicdV\n\tsKqjh1pURJBZAwOsLzEW2JyEbbv4A4tHjBKrGHTU="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"YjRP5gVA\"; dkim-atps=neutral","Date":"Wed, 30 Mar 2022 17:39:22 +0900","To":"Laurent Pinchart ","Message-ID":"<20220330083922.GA3237525@pyrite.rasen.tech>","References":"<20220328120336.10834-1-laurent.pinchart@ideasonboard.com>\n\t<20220328120336.10834-2-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20220328120336.10834-2-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 1/4] libipa: camera_sensor_helper:\n\tReorganize gain constants","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":"Paul Elder via libcamera-devel ","Reply-To":"paul.elder@ideasonboard.com","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]