[{"id":32552,"web_url":"https://patchwork.libcamera.org/comment/32552/","msgid":"<20241205195815.GF21014@pendragon.ideasonboard.com>","date":"2024-12-05T19:58:15","subject":"Re: [PATCH v1] libcamera: libipa: camera_sensor_helper: Use\n\t`variant` instead of `union`","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Barnabás,\n\nThank you for the patch.\n\nOn Thu, Dec 05, 2024 at 12:27:05PM +0000, Barnabás Pőcze wrote:\n> Use an `std::variant` to store the analogue gain instead of a bare union + tag.\n\nSounds like the right tool for the job.\n\n> Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>\n\nReviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n\n> ---\n>  src/ipa/libipa/camera_sensor_helper.cpp | 198 ++++++++----------------\n>  src/ipa/libipa/camera_sensor_helper.h   |  18 +--\n>  2 files changed, 70 insertions(+), 146 deletions(-)\n> \n> diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp\n> index a76e5e75..ac3d712b 100644\n> --- a/src/ipa/libipa/camera_sensor_helper.cpp\n> +++ b/src/ipa/libipa/camera_sensor_helper.cpp\n> @@ -87,21 +87,18 @@ namespace ipa {\n>   */\n>  uint32_t CameraSensorHelper::gainCode(double gain) const\n>  {\n> -\tconst AnalogueGainConstants &k = gainConstants_;\n> +\tif (auto *l = std::get_if<AnalogueGainLinear>(&gain_)) {\n> +\t\tASSERT(l->m0 == 0 || l->m1 == 0);\n>  \n> -\tswitch (gainType_) {\n> -\tcase AnalogueGainLinear:\n> -\t\tASSERT(k.linear.m0 == 0 || k.linear.m1 == 0);\n> -\n> -\t\treturn (k.linear.c0 - k.linear.c1 * gain) /\n> -\t\t       (k.linear.m1 * gain - k.linear.m0);\n> -\n> -\tcase AnalogueGainExponential:\n> -\t\tASSERT(k.exp.a != 0 && k.exp.m != 0);\n> -\n> -\t\treturn std::log2(gain / k.exp.a) / k.exp.m;\n> +\t\treturn (l->c0 - l->c1 * gain) /\n> +\t\t       (l->m1 * gain - l->m0);\n> +\t}\n> +\telse if (auto *e = std::get_if<AnalogueGainExp>(&gain_)) {\n> +\t\tASSERT(e->a != 0 && e->m != 0);\n>  \n> -\tdefault:\n> +\t\treturn std::log2(gain / e->a) / e->m;\n> +\t}\n> +\telse {\n>  \t\tASSERT(false);\n>  \t\treturn 0;\n>  \t}\n> @@ -119,38 +116,28 @@ uint32_t CameraSensorHelper::gainCode(double gain) const\n>   */\n>  double CameraSensorHelper::gain(uint32_t gainCode) const\n>  {\n> -\tconst AnalogueGainConstants &k = gainConstants_;\n>  \tdouble gain = static_cast<double>(gainCode);\n>  \n> -\tswitch (gainType_) {\n> -\tcase AnalogueGainLinear:\n> -\t\tASSERT(k.linear.m0 == 0 || k.linear.m1 == 0);\n> -\n> -\t\treturn (k.linear.m0 * gain + k.linear.c0) /\n> -\t\t       (k.linear.m1 * gain + k.linear.c1);\n> +\tif (auto *l = std::get_if<AnalogueGainLinear>(&gain_)) {\n> +\t\tASSERT(l->m0 == 0 || l->m1 == 0);\n>  \n> -\tcase AnalogueGainExponential:\n> -\t\tASSERT(k.exp.a != 0 && k.exp.m != 0);\n> -\n> -\t\treturn k.exp.a * std::exp2(k.exp.m * gain);\n> +\t\treturn (l->m0 * gain + l->c0) /\n> +\t\t       (l->m1 * gain + l->c1);\n> +\t}\n> +\telse if (auto *e = std::get_if<AnalogueGainExp>(&gain_)) {\n> +\t\tASSERT(e->a != 0 && e->m != 0);\n>  \n> -\tdefault:\n> +\t\treturn e->a * std::exp2(e->m * gain);\n> +\t}\n> +\telse {\n>  \t\tASSERT(false);\n>  \t\treturn 0.0;\n>  \t}\n>  }\n>  \n>  /**\n> - * \\enum CameraSensorHelper::AnalogueGainType\n> - * \\brief The gain calculation modes as defined by the MIPI CCS\n> - *\n> - * Describes the image sensor analogue gain capabilities.\n> - * Two modes are possible, depending on the sensor: Linear and Exponential.\n> - */\n> -\n> -/**\n> - * \\var CameraSensorHelper::AnalogueGainLinear\n> - * \\brief Gain is computed using linear gain estimation\n> + * \\struct CameraSensorHelper::AnalogueGainLinear\n> + * \\brief Analogue gain constants for the linear gain model\n>   *\n>   * The relationship between the integer gain parameter and the resulting gain\n>   * multiplier is given by the following equation:\n> @@ -165,11 +152,27 @@ double CameraSensorHelper::gain(uint32_t gainCode) const\n>   * The full Gain equation therefore reduces to either:\n>   *\n>   * \\f$gain=\\frac{c0}{m1x+c1}\\f$ or \\f$\\frac{m0x+c0}{c1}\\f$\n> + *\n> + * \\var CameraSensorHelper::AnalogueGainLinear::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::AnalogueGainLinear::c0\n> + * \\brief Constant used in the linear gain coding/decoding\n> + *\n> + * \\var CameraSensorHelper::AnalogueGainLinear::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::AnalogueGainLinear::c1\n> + * \\brief Constant used in the linear gain coding/decoding\n>   */\n>  \n>  /**\n> - * \\var CameraSensorHelper::AnalogueGainExponential\n> - * \\brief Gain is expressed using an exponential model\n> + * \\struct CameraSensorHelper::AnalogueGainExp\n> + * \\brief Analogue gain constants for the exponential gain model\n>   *\n>   * The relationship between the integer gain parameter and the resulting gain\n>   * multiplier is given by the following equation:\n> @@ -185,54 +188,14 @@ double CameraSensorHelper::gain(uint32_t gainCode) const\n>   *\n>   * When the gain is expressed in dB, 'a' is equal to 1 and 'm' to\n>   * \\f$log_{2}{10^{\\frac{1}{20}}}\\f$.\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::AnalogueGainExpConstants\n> - * \\brief Analogue gain constants for the exponential gain model\n>   *\n> - * \\var CameraSensorHelper::AnalogueGainExpConstants::a\n> + * \\var CameraSensorHelper::AnalogueGainExp::a\n>   * \\brief Constant used in the exponential gain coding/decoding\n>   *\n> - * \\var CameraSensorHelper::AnalogueGainExpConstants::m\n> + * \\var CameraSensorHelper::AnalogueGainExp::m\n>   * \\brief Constant used in the exponential gain coding/decoding\n>   */\n>  \n> -/**\n> - * \\struct CameraSensorHelper::AnalogueGainConstants\n> - * \\brief Analogue gain model constants\n> - *\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> - * \\var CameraSensorHelper::AnalogueGainConstants::linear\n> - * \\brief Constants for the linear gain model\n> - *\n> - * \\var CameraSensorHelper::AnalogueGainConstants::exp\n> - * \\brief Constants for the exponential gain model\n> - */\n> -\n>  /**\n>   * \\var CameraSensorHelper::blackLevel_\n>   * \\brief The black level of the sensor\n> @@ -240,12 +203,7 @@ double CameraSensorHelper::gain(uint32_t gainCode) const\n>   */\n>  \n>  /**\n> - * \\var CameraSensorHelper::gainType_\n> - * \\brief The analogue gain model type\n> - */\n> -\n> -/**\n> - * \\var CameraSensorHelper::gainConstants_\n> + * \\var CameraSensorHelper::gain_\n>   * \\brief The analogue gain parameters used for calculation\n>   *\n>   * The analogue gain is calculated through a formula, and its parameters are\n> @@ -526,8 +484,7 @@ public:\n>  \t{\n>  \t\t/* From datasheet: 64 at 10bits. */\n>  \t\tblackLevel_ = 4096;\n> -\t\tgainType_ = AnalogueGainLinear;\n> -\t\tgainConstants_.linear = { 100, 0, 0, 1024 };\n> +\t\tgain_ = AnalogueGainLinear{ 100, 0, 0, 1024 };\n>  \t}\n>  };\n>  REGISTER_CAMERA_SENSOR_HELPER(\"gc05a2\", CameraSensorHelperGc05a2)\n> @@ -539,8 +496,7 @@ public:\n>  \t{\n>  \t\t/* From datasheet: 64 at 10bits. */\n>  \t\tblackLevel_ = 4096;\n> -\t\tgainType_ = AnalogueGainLinear;\n> -\t\tgainConstants_.linear = { 100, 0, 0, 1024 };\n> +\t\tgain_ = AnalogueGainLinear{ 100, 0, 0, 1024 };\n>  \t}\n>  };\n>  REGISTER_CAMERA_SENSOR_HELPER(\"gc08a3\", CameraSensorHelperGc08a3)\n> @@ -552,8 +508,7 @@ public:\n>  \t{\n>  \t\t/* From datasheet: 64 at 10bits. */\n>  \t\tblackLevel_ = 4096;\n> -\t\tgainType_ = AnalogueGainLinear;\n> -\t\tgainConstants_.linear = { 0, 512, -1, 512 };\n> +\t\tgain_ = AnalogueGainLinear{ 0, 512, -1, 512 };\n>  \t}\n>  };\n>  REGISTER_CAMERA_SENSOR_HELPER(\"imx214\", CameraSensorHelperImx214)\n> @@ -565,8 +520,7 @@ public:\n>  \t{\n>  \t\t/* From datasheet: 64 at 10bits. */\n>  \t\tblackLevel_ = 4096;\n> -\t\tgainType_ = AnalogueGainLinear;\n> -\t\tgainConstants_.linear = { 0, 256, -1, 256 };\n> +\t\tgain_ = AnalogueGainLinear{ 0, 256, -1, 256 };\n>  \t}\n>  };\n>  REGISTER_CAMERA_SENSOR_HELPER(\"imx219\", CameraSensorHelperImx219)\n> @@ -578,8 +532,7 @@ public:\n>  \t{\n>  \t\t/* From datasheet: 0x40 at 10bits. */\n>  \t\tblackLevel_ = 4096;\n> -\t\tgainType_ = AnalogueGainLinear;\n> -\t\tgainConstants_.linear = { 0, 512, -1, 512 };\n> +\t\tgain_ = AnalogueGainLinear{ 0, 512, -1, 512 };\n>  \t}\n>  };\n>  REGISTER_CAMERA_SENSOR_HELPER(\"imx258\", CameraSensorHelperImx258)\n> @@ -591,8 +544,7 @@ public:\n>  \t{\n>  \t\t/* From datasheet: 0x32 at 10bits. */\n>  \t\tblackLevel_ = 3200;\n> -\t\tgainType_ = AnalogueGainLinear;\n> -\t\tgainConstants_.linear = { 0, 2048, -1, 2048 };\n> +\t\tgain_ = AnalogueGainLinear{ 0, 2048, -1, 2048 };\n>  \t}\n>  };\n>  REGISTER_CAMERA_SENSOR_HELPER(\"imx283\", CameraSensorHelperImx283)\n> @@ -604,8 +556,7 @@ public:\n>  \t{\n>  \t\t/* From datasheet: 0xf0 at 12bits. */\n>  \t\tblackLevel_ = 3840;\n> -\t\tgainType_ = AnalogueGainExponential;\n> -\t\tgainConstants_.exp = { 1.0, expGainDb(0.3) };\n> +\t\tgain_ = AnalogueGainExp{ 1.0, expGainDb(0.3) };\n>  \t}\n>  };\n>  REGISTER_CAMERA_SENSOR_HELPER(\"imx290\", CameraSensorHelperImx290)\n> @@ -615,8 +566,7 @@ class CameraSensorHelperImx296 : public CameraSensorHelper\n>  public:\n>  \tCameraSensorHelperImx296()\n>  \t{\n> -\t\tgainType_ = AnalogueGainExponential;\n> -\t\tgainConstants_.exp = { 1.0, expGainDb(0.1) };\n> +\t\tgain_ = AnalogueGainExp{ 1.0, expGainDb(0.1) };\n>  \t}\n>  };\n>  REGISTER_CAMERA_SENSOR_HELPER(\"imx296\", CameraSensorHelperImx296)\n> @@ -633,8 +583,7 @@ public:\n>  \t{\n>  \t\t/* From datasheet: 0x32 at 10bits. */\n>  \t\tblackLevel_ = 3200;\n> -\t\tgainType_ = AnalogueGainExponential;\n> -\t\tgainConstants_.exp = { 1.0, expGainDb(0.3) };\n> +\t\tgain_ = AnalogueGainExp{ 1.0, expGainDb(0.3) };\n>  \t}\n>  };\n>  REGISTER_CAMERA_SENSOR_HELPER(\"imx335\", CameraSensorHelperImx335)\n> @@ -644,8 +593,7 @@ class CameraSensorHelperImx415 : public CameraSensorHelper\n>  public:\n>  \tCameraSensorHelperImx415()\n>  \t{\n> -\t\tgainType_ = AnalogueGainExponential;\n> -\t\tgainConstants_.exp = { 1.0, expGainDb(0.3) };\n> +\t\tgain_ = AnalogueGainExp{ 1.0, expGainDb(0.3) };\n>  \t}\n>  };\n>  REGISTER_CAMERA_SENSOR_HELPER(\"imx415\", CameraSensorHelperImx415)\n> @@ -660,8 +608,7 @@ class CameraSensorHelperImx477 : public CameraSensorHelper\n>  public:\n>  \tCameraSensorHelperImx477()\n>  \t{\n> -\t\tgainType_ = AnalogueGainLinear;\n> -\t\tgainConstants_.linear = { 0, 1024, -1, 1024 };\n> +\t\tgain_ = AnalogueGainLinear{ 0, 1024, -1, 1024 };\n>  \t}\n>  };\n>  REGISTER_CAMERA_SENSOR_HELPER(\"imx477\", CameraSensorHelperImx477)\n> @@ -675,8 +622,7 @@ public:\n>  \t\t * The Sensor Manual doesn't appear to document the gain model.\n>  \t\t * This has been validated with some empirical testing only.\n>  \t\t */\n> -\t\tgainType_ = AnalogueGainLinear;\n> -\t\tgainConstants_.linear = { 1, 0, 0, 128 };\n> +\t\tgain_ = AnalogueGainLinear{ 1, 0, 0, 128 };\n>  \t}\n>  };\n>  REGISTER_CAMERA_SENSOR_HELPER(\"ov2685\", CameraSensorHelperOv2685)\n> @@ -686,8 +632,7 @@ class CameraSensorHelperOv2740 : public CameraSensorHelper\n>  public:\n>  \tCameraSensorHelperOv2740()\n>  \t{\n> -\t\tgainType_ = AnalogueGainLinear;\n> -\t\tgainConstants_.linear = { 1, 0, 0, 128 };\n> +\t\tgain_ = AnalogueGainLinear{ 1, 0, 0, 128 };\n>  \t}\n>  };\n>  REGISTER_CAMERA_SENSOR_HELPER(\"ov2740\", CameraSensorHelperOv2740)\n> @@ -699,8 +644,7 @@ public:\n>  \t{\n>  \t\t/* From datasheet: 0x40 at 12bits. */\n>  \t\tblackLevel_ = 1024;\n> -\t\tgainType_ = AnalogueGainLinear;\n> -\t\tgainConstants_.linear = { 1, 0, 0, 128 };\n> +\t\tgain_ = AnalogueGainLinear{ 1, 0, 0, 128 };\n>  \t}\n>  };\n>  REGISTER_CAMERA_SENSOR_HELPER(\"ov4689\", CameraSensorHelperOv4689)\n> @@ -712,8 +656,7 @@ public:\n>  \t{\n>  \t\t/* From datasheet: 0x10 at 10bits. */\n>  \t\tblackLevel_ = 1024;\n> -\t\tgainType_ = AnalogueGainLinear;\n> -\t\tgainConstants_.linear = { 1, 0, 0, 16 };\n> +\t\tgain_ = AnalogueGainLinear{ 1, 0, 0, 16 };\n>  \t}\n>  };\n>  REGISTER_CAMERA_SENSOR_HELPER(\"ov5640\", CameraSensorHelperOv5640)\n> @@ -723,8 +666,7 @@ class CameraSensorHelperOv5647 : public CameraSensorHelper\n>  public:\n>  \tCameraSensorHelperOv5647()\n>  \t{\n> -\t\tgainType_ = AnalogueGainLinear;\n> -\t\tgainConstants_.linear = { 1, 0, 0, 16 };\n> +\t\tgain_ = AnalogueGainLinear{ 1, 0, 0, 16 };\n>  \t}\n>  };\n>  REGISTER_CAMERA_SENSOR_HELPER(\"ov5647\", CameraSensorHelperOv5647)\n> @@ -734,8 +676,7 @@ class CameraSensorHelperOv5670 : public CameraSensorHelper\n>  public:\n>  \tCameraSensorHelperOv5670()\n>  \t{\n> -\t\tgainType_ = AnalogueGainLinear;\n> -\t\tgainConstants_.linear = { 1, 0, 0, 128 };\n> +\t\tgain_ = AnalogueGainLinear{ 1, 0, 0, 128 };\n>  \t}\n>  };\n>  REGISTER_CAMERA_SENSOR_HELPER(\"ov5670\", CameraSensorHelperOv5670)\n> @@ -747,8 +688,7 @@ public:\n>  \t{\n>  \t\t/* From Linux kernel driver: 0x40 at 10bits. */\n>  \t\tblackLevel_ = 4096;\n> -\t\tgainType_ = AnalogueGainLinear;\n> -\t\tgainConstants_.linear = { 1, 0, 0, 128 };\n> +\t\tgain_ = AnalogueGainLinear{ 1, 0, 0, 128 };\n>  \t}\n>  };\n>  REGISTER_CAMERA_SENSOR_HELPER(\"ov5675\", CameraSensorHelperOv5675)\n> @@ -758,8 +698,7 @@ class CameraSensorHelperOv5693 : public CameraSensorHelper\n>  public:\n>  \tCameraSensorHelperOv5693()\n>  \t{\n> -\t\tgainType_ = AnalogueGainLinear;\n> -\t\tgainConstants_.linear = { 1, 0, 0, 16 };\n> +\t\tgain_ = AnalogueGainLinear{ 1, 0, 0, 16 };\n>  \t}\n>  };\n>  REGISTER_CAMERA_SENSOR_HELPER(\"ov5693\", CameraSensorHelperOv5693)\n> @@ -769,8 +708,7 @@ class CameraSensorHelperOv64a40 : public CameraSensorHelper\n>  public:\n>  \tCameraSensorHelperOv64a40()\n>  \t{\n> -\t\tgainType_ = AnalogueGainLinear;\n> -\t\tgainConstants_.linear = { 1, 0, 0, 128 };\n> +\t\tgain_ = AnalogueGainLinear{ 1, 0, 0, 128 };\n>  \t}\n>  };\n>  REGISTER_CAMERA_SENSOR_HELPER(\"ov64a40\", CameraSensorHelperOv64a40)\n> @@ -780,15 +718,13 @@ class CameraSensorHelperOv8858 : public CameraSensorHelper\n>  public:\n>  \tCameraSensorHelperOv8858()\n>  \t{\n> -\t\tgainType_ = AnalogueGainLinear;\n> -\n>  \t\t/*\n>  \t\t * \\todo Validate the selected 1/128 step value as it differs\n>  \t\t * from what the sensor manual describes.\n>  \t\t *\n>  \t\t * See: https://patchwork.linuxtv.org/project/linux-media/patch/20221106171129.166892-2-nicholas@rothemail.net/#142267\n>  \t\t */\n> -\t\tgainConstants_.linear = { 1, 0, 0, 128 };\n> +\t\tgain_ = AnalogueGainLinear{ 1, 0, 0, 128 };\n>  \t}\n>  };\n>  REGISTER_CAMERA_SENSOR_HELPER(\"ov8858\", CameraSensorHelperOv8858)\n> @@ -798,8 +734,7 @@ class CameraSensorHelperOv8865 : public CameraSensorHelper\n>  public:\n>  \tCameraSensorHelperOv8865()\n>  \t{\n> -\t\tgainType_ = AnalogueGainLinear;\n> -\t\tgainConstants_.linear = { 1, 0, 0, 128 };\n> +\t\tgain_ = AnalogueGainLinear{ 1, 0, 0, 128 };\n>  \t}\n>  };\n>  REGISTER_CAMERA_SENSOR_HELPER(\"ov8865\", CameraSensorHelperOv8865)\n> @@ -809,8 +744,7 @@ class CameraSensorHelperOv13858 : public CameraSensorHelper\n>  public:\n>  \tCameraSensorHelperOv13858()\n>  \t{\n> -\t\tgainType_ = AnalogueGainLinear;\n> -\t\tgainConstants_.linear = { 1, 0, 0, 128 };\n> +\t\tgain_ = AnalogueGainLinear{ 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 75868205..a9300a64 100644\n> --- a/src/ipa/libipa/camera_sensor_helper.h\n> +++ b/src/ipa/libipa/camera_sensor_helper.h\n> @@ -11,6 +11,7 @@\n>  #include <optional>\n>  #include <stdint.h>\n>  #include <string>\n> +#include <variant>\n>  #include <vector>\n>  \n>  #include <libcamera/base/class.h>\n> @@ -30,31 +31,20 @@ public:\n>  \tvirtual double gain(uint32_t gainCode) const;\n>  \n>  protected:\n> -\tenum AnalogueGainType {\n> -\t\tAnalogueGainLinear,\n> -\t\tAnalogueGainExponential,\n> -\t};\n> -\n> -\tstruct AnalogueGainLinearConstants {\n> +\tstruct AnalogueGainLinear {\n>  \t\tint16_t m0;\n>  \t\tint16_t c0;\n>  \t\tint16_t m1;\n>  \t\tint16_t c1;\n>  \t};\n>  \n> -\tstruct AnalogueGainExpConstants {\n> +\tstruct AnalogueGainExp {\n>  \t\tdouble a;\n>  \t\tdouble m;\n>  \t};\n>  \n> -\tunion AnalogueGainConstants {\n> -\t\tAnalogueGainLinearConstants linear;\n> -\t\tAnalogueGainExpConstants exp;\n> -\t};\n> -\n>  \tstd::optional<int16_t> blackLevel_;\n> -\tAnalogueGainType gainType_;\n> -\tAnalogueGainConstants gainConstants_;\n> +\tstd::variant<std::monostate, AnalogueGainLinear, AnalogueGainExp> gain_;\n>  \n>  private:\n>  \tLIBCAMERA_DISABLE_COPY_AND_MOVE(CameraSensorHelper)","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 21B98C323E\n\tfor <parsemail@patchwork.libcamera.org>;\n\tThu,  5 Dec 2024 19:58:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4C62F66114;\n\tThu,  5 Dec 2024 20:58:30 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CED296608C\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu,  5 Dec 2024 20:58:27 +0100 (CET)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B0F202B3;\n\tThu,  5 Dec 2024 20:57:58 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"T5QFdwKi\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1733428678;\n\tbh=IcpmkCBO/dKBpLGc8O1DkNgOFk69ZlenvNHuRLSJzrM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=T5QFdwKiiqOfCW0/RDm59JLjcWOmMQgg4m2k2Tg0FiFU/WUuQHRddRyS7Fj2Ho8/h\n\teIX1JFGkaz38Jt4l0p75/pjrBA7fz6g1KCGXvi+X4kJgbldbrEdO5gHBRI9bJVyhHe\n\tFCUb//RG/kr0Bod79rrYghSA6w+rmpAf34Hvxv+c=","Date":"Thu, 5 Dec 2024 21:58:15 +0200","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= <pobrn@protonmail.com>","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v1] libcamera: libipa: camera_sensor_helper: Use\n\t`variant` instead of `union`","Message-ID":"<20241205195815.GF21014@pendragon.ideasonboard.com>","References":"<20241205122702.1105835-1-pobrn@protonmail.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20241205122702.1105835-1-pobrn@protonmail.com>","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>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":32619,"web_url":"https://patchwork.libcamera.org/comment/32619/","msgid":"<20241209022855.GA11343@pendragon.ideasonboard.com>","date":"2024-12-09T02:28:55","subject":"Re: [PATCH v1] libcamera: libipa: camera_sensor_helper: Use\n\t`variant` instead of `union`","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Thu, Dec 05, 2024 at 09:58:15PM +0200, Laurent Pinchart wrote:\n> Hi Barnabás,\n> \n> Thank you for the patch.\n> \n> On Thu, Dec 05, 2024 at 12:27:05PM +0000, Barnabás Pőcze wrote:\n> > Use an `std::variant` to store the analogue gain instead of a bare union + tag.\n> \n> Sounds like the right tool for the job.\n> \n> > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>\n> \n> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> \n> > ---\n> >  src/ipa/libipa/camera_sensor_helper.cpp | 198 ++++++++----------------\n> >  src/ipa/libipa/camera_sensor_helper.h   |  18 +--\n> >  2 files changed, 70 insertions(+), 146 deletions(-)\n> > \n> > diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp\n> > index a76e5e75..ac3d712b 100644\n> > --- a/src/ipa/libipa/camera_sensor_helper.cpp\n> > +++ b/src/ipa/libipa/camera_sensor_helper.cpp\n> > @@ -87,21 +87,18 @@ namespace ipa {\n> >   */\n> >  uint32_t CameraSensorHelper::gainCode(double gain) const\n> >  {\n> > -\tconst AnalogueGainConstants &k = gainConstants_;\n> > +\tif (auto *l = std::get_if<AnalogueGainLinear>(&gain_)) {\n> > +\t\tASSERT(l->m0 == 0 || l->m1 == 0);\n> >  \n> > -\tswitch (gainType_) {\n> > -\tcase AnalogueGainLinear:\n> > -\t\tASSERT(k.linear.m0 == 0 || k.linear.m1 == 0);\n> > -\n> > -\t\treturn (k.linear.c0 - k.linear.c1 * gain) /\n> > -\t\t       (k.linear.m1 * gain - k.linear.m0);\n> > -\n> > -\tcase AnalogueGainExponential:\n> > -\t\tASSERT(k.exp.a != 0 && k.exp.m != 0);\n> > -\n> > -\t\treturn std::log2(gain / k.exp.a) / k.exp.m;\n> > +\t\treturn (l->c0 - l->c1 * gain) /\n> > +\t\t       (l->m1 * gain - l->m0);\n> > +\t}\n> > +\telse if (auto *e = std::get_if<AnalogueGainExp>(&gain_)) {\n\nActually checkstyle.py reports an issue here and in a few other places.\nYou can keep my Rb tag for v2 after fixing that.\n\n> > +\t\tASSERT(e->a != 0 && e->m != 0);\n> >  \n> > -\tdefault:\n> > +\t\treturn std::log2(gain / e->a) / e->m;\n> > +\t}\n> > +\telse {\n> >  \t\tASSERT(false);\n> >  \t\treturn 0;\n> >  \t}\n> > @@ -119,38 +116,28 @@ uint32_t CameraSensorHelper::gainCode(double gain) const\n> >   */\n> >  double CameraSensorHelper::gain(uint32_t gainCode) const\n> >  {\n> > -\tconst AnalogueGainConstants &k = gainConstants_;\n> >  \tdouble gain = static_cast<double>(gainCode);\n> >  \n> > -\tswitch (gainType_) {\n> > -\tcase AnalogueGainLinear:\n> > -\t\tASSERT(k.linear.m0 == 0 || k.linear.m1 == 0);\n> > -\n> > -\t\treturn (k.linear.m0 * gain + k.linear.c0) /\n> > -\t\t       (k.linear.m1 * gain + k.linear.c1);\n> > +\tif (auto *l = std::get_if<AnalogueGainLinear>(&gain_)) {\n> > +\t\tASSERT(l->m0 == 0 || l->m1 == 0);\n> >  \n> > -\tcase AnalogueGainExponential:\n> > -\t\tASSERT(k.exp.a != 0 && k.exp.m != 0);\n> > -\n> > -\t\treturn k.exp.a * std::exp2(k.exp.m * gain);\n> > +\t\treturn (l->m0 * gain + l->c0) /\n> > +\t\t       (l->m1 * gain + l->c1);\n> > +\t}\n> > +\telse if (auto *e = std::get_if<AnalogueGainExp>(&gain_)) {\n> > +\t\tASSERT(e->a != 0 && e->m != 0);\n> >  \n> > -\tdefault:\n> > +\t\treturn e->a * std::exp2(e->m * gain);\n> > +\t}\n> > +\telse {\n> >  \t\tASSERT(false);\n> >  \t\treturn 0.0;\n> >  \t}\n> >  }\n> >  \n> >  /**\n> > - * \\enum CameraSensorHelper::AnalogueGainType\n> > - * \\brief The gain calculation modes as defined by the MIPI CCS\n> > - *\n> > - * Describes the image sensor analogue gain capabilities.\n> > - * Two modes are possible, depending on the sensor: Linear and Exponential.\n> > - */\n> > -\n> > -/**\n> > - * \\var CameraSensorHelper::AnalogueGainLinear\n> > - * \\brief Gain is computed using linear gain estimation\n> > + * \\struct CameraSensorHelper::AnalogueGainLinear\n> > + * \\brief Analogue gain constants for the linear gain model\n> >   *\n> >   * The relationship between the integer gain parameter and the resulting gain\n> >   * multiplier is given by the following equation:\n> > @@ -165,11 +152,27 @@ double CameraSensorHelper::gain(uint32_t gainCode) const\n> >   * The full Gain equation therefore reduces to either:\n> >   *\n> >   * \\f$gain=\\frac{c0}{m1x+c1}\\f$ or \\f$\\frac{m0x+c0}{c1}\\f$\n> > + *\n> > + * \\var CameraSensorHelper::AnalogueGainLinear::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::AnalogueGainLinear::c0\n> > + * \\brief Constant used in the linear gain coding/decoding\n> > + *\n> > + * \\var CameraSensorHelper::AnalogueGainLinear::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::AnalogueGainLinear::c1\n> > + * \\brief Constant used in the linear gain coding/decoding\n> >   */\n> >  \n> >  /**\n> > - * \\var CameraSensorHelper::AnalogueGainExponential\n> > - * \\brief Gain is expressed using an exponential model\n> > + * \\struct CameraSensorHelper::AnalogueGainExp\n> > + * \\brief Analogue gain constants for the exponential gain model\n> >   *\n> >   * The relationship between the integer gain parameter and the resulting gain\n> >   * multiplier is given by the following equation:\n> > @@ -185,54 +188,14 @@ double CameraSensorHelper::gain(uint32_t gainCode) const\n> >   *\n> >   * When the gain is expressed in dB, 'a' is equal to 1 and 'm' to\n> >   * \\f$log_{2}{10^{\\frac{1}{20}}}\\f$.\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::AnalogueGainExpConstants\n> > - * \\brief Analogue gain constants for the exponential gain model\n> >   *\n> > - * \\var CameraSensorHelper::AnalogueGainExpConstants::a\n> > + * \\var CameraSensorHelper::AnalogueGainExp::a\n> >   * \\brief Constant used in the exponential gain coding/decoding\n> >   *\n> > - * \\var CameraSensorHelper::AnalogueGainExpConstants::m\n> > + * \\var CameraSensorHelper::AnalogueGainExp::m\n> >   * \\brief Constant used in the exponential gain coding/decoding\n> >   */\n> >  \n> > -/**\n> > - * \\struct CameraSensorHelper::AnalogueGainConstants\n> > - * \\brief Analogue gain model constants\n> > - *\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> > - * \\var CameraSensorHelper::AnalogueGainConstants::linear\n> > - * \\brief Constants for the linear gain model\n> > - *\n> > - * \\var CameraSensorHelper::AnalogueGainConstants::exp\n> > - * \\brief Constants for the exponential gain model\n> > - */\n> > -\n> >  /**\n> >   * \\var CameraSensorHelper::blackLevel_\n> >   * \\brief The black level of the sensor\n> > @@ -240,12 +203,7 @@ double CameraSensorHelper::gain(uint32_t gainCode) const\n> >   */\n> >  \n> >  /**\n> > - * \\var CameraSensorHelper::gainType_\n> > - * \\brief The analogue gain model type\n> > - */\n> > -\n> > -/**\n> > - * \\var CameraSensorHelper::gainConstants_\n> > + * \\var CameraSensorHelper::gain_\n> >   * \\brief The analogue gain parameters used for calculation\n> >   *\n> >   * The analogue gain is calculated through a formula, and its parameters are\n> > @@ -526,8 +484,7 @@ public:\n> >  \t{\n> >  \t\t/* From datasheet: 64 at 10bits. */\n> >  \t\tblackLevel_ = 4096;\n> > -\t\tgainType_ = AnalogueGainLinear;\n> > -\t\tgainConstants_.linear = { 100, 0, 0, 1024 };\n> > +\t\tgain_ = AnalogueGainLinear{ 100, 0, 0, 1024 };\n> >  \t}\n> >  };\n> >  REGISTER_CAMERA_SENSOR_HELPER(\"gc05a2\", CameraSensorHelperGc05a2)\n> > @@ -539,8 +496,7 @@ public:\n> >  \t{\n> >  \t\t/* From datasheet: 64 at 10bits. */\n> >  \t\tblackLevel_ = 4096;\n> > -\t\tgainType_ = AnalogueGainLinear;\n> > -\t\tgainConstants_.linear = { 100, 0, 0, 1024 };\n> > +\t\tgain_ = AnalogueGainLinear{ 100, 0, 0, 1024 };\n> >  \t}\n> >  };\n> >  REGISTER_CAMERA_SENSOR_HELPER(\"gc08a3\", CameraSensorHelperGc08a3)\n> > @@ -552,8 +508,7 @@ public:\n> >  \t{\n> >  \t\t/* From datasheet: 64 at 10bits. */\n> >  \t\tblackLevel_ = 4096;\n> > -\t\tgainType_ = AnalogueGainLinear;\n> > -\t\tgainConstants_.linear = { 0, 512, -1, 512 };\n> > +\t\tgain_ = AnalogueGainLinear{ 0, 512, -1, 512 };\n> >  \t}\n> >  };\n> >  REGISTER_CAMERA_SENSOR_HELPER(\"imx214\", CameraSensorHelperImx214)\n> > @@ -565,8 +520,7 @@ public:\n> >  \t{\n> >  \t\t/* From datasheet: 64 at 10bits. */\n> >  \t\tblackLevel_ = 4096;\n> > -\t\tgainType_ = AnalogueGainLinear;\n> > -\t\tgainConstants_.linear = { 0, 256, -1, 256 };\n> > +\t\tgain_ = AnalogueGainLinear{ 0, 256, -1, 256 };\n> >  \t}\n> >  };\n> >  REGISTER_CAMERA_SENSOR_HELPER(\"imx219\", CameraSensorHelperImx219)\n> > @@ -578,8 +532,7 @@ public:\n> >  \t{\n> >  \t\t/* From datasheet: 0x40 at 10bits. */\n> >  \t\tblackLevel_ = 4096;\n> > -\t\tgainType_ = AnalogueGainLinear;\n> > -\t\tgainConstants_.linear = { 0, 512, -1, 512 };\n> > +\t\tgain_ = AnalogueGainLinear{ 0, 512, -1, 512 };\n> >  \t}\n> >  };\n> >  REGISTER_CAMERA_SENSOR_HELPER(\"imx258\", CameraSensorHelperImx258)\n> > @@ -591,8 +544,7 @@ public:\n> >  \t{\n> >  \t\t/* From datasheet: 0x32 at 10bits. */\n> >  \t\tblackLevel_ = 3200;\n> > -\t\tgainType_ = AnalogueGainLinear;\n> > -\t\tgainConstants_.linear = { 0, 2048, -1, 2048 };\n> > +\t\tgain_ = AnalogueGainLinear{ 0, 2048, -1, 2048 };\n> >  \t}\n> >  };\n> >  REGISTER_CAMERA_SENSOR_HELPER(\"imx283\", CameraSensorHelperImx283)\n> > @@ -604,8 +556,7 @@ public:\n> >  \t{\n> >  \t\t/* From datasheet: 0xf0 at 12bits. */\n> >  \t\tblackLevel_ = 3840;\n> > -\t\tgainType_ = AnalogueGainExponential;\n> > -\t\tgainConstants_.exp = { 1.0, expGainDb(0.3) };\n> > +\t\tgain_ = AnalogueGainExp{ 1.0, expGainDb(0.3) };\n> >  \t}\n> >  };\n> >  REGISTER_CAMERA_SENSOR_HELPER(\"imx290\", CameraSensorHelperImx290)\n> > @@ -615,8 +566,7 @@ class CameraSensorHelperImx296 : public CameraSensorHelper\n> >  public:\n> >  \tCameraSensorHelperImx296()\n> >  \t{\n> > -\t\tgainType_ = AnalogueGainExponential;\n> > -\t\tgainConstants_.exp = { 1.0, expGainDb(0.1) };\n> > +\t\tgain_ = AnalogueGainExp{ 1.0, expGainDb(0.1) };\n> >  \t}\n> >  };\n> >  REGISTER_CAMERA_SENSOR_HELPER(\"imx296\", CameraSensorHelperImx296)\n> > @@ -633,8 +583,7 @@ public:\n> >  \t{\n> >  \t\t/* From datasheet: 0x32 at 10bits. */\n> >  \t\tblackLevel_ = 3200;\n> > -\t\tgainType_ = AnalogueGainExponential;\n> > -\t\tgainConstants_.exp = { 1.0, expGainDb(0.3) };\n> > +\t\tgain_ = AnalogueGainExp{ 1.0, expGainDb(0.3) };\n> >  \t}\n> >  };\n> >  REGISTER_CAMERA_SENSOR_HELPER(\"imx335\", CameraSensorHelperImx335)\n> > @@ -644,8 +593,7 @@ class CameraSensorHelperImx415 : public CameraSensorHelper\n> >  public:\n> >  \tCameraSensorHelperImx415()\n> >  \t{\n> > -\t\tgainType_ = AnalogueGainExponential;\n> > -\t\tgainConstants_.exp = { 1.0, expGainDb(0.3) };\n> > +\t\tgain_ = AnalogueGainExp{ 1.0, expGainDb(0.3) };\n> >  \t}\n> >  };\n> >  REGISTER_CAMERA_SENSOR_HELPER(\"imx415\", CameraSensorHelperImx415)\n> > @@ -660,8 +608,7 @@ class CameraSensorHelperImx477 : public CameraSensorHelper\n> >  public:\n> >  \tCameraSensorHelperImx477()\n> >  \t{\n> > -\t\tgainType_ = AnalogueGainLinear;\n> > -\t\tgainConstants_.linear = { 0, 1024, -1, 1024 };\n> > +\t\tgain_ = AnalogueGainLinear{ 0, 1024, -1, 1024 };\n> >  \t}\n> >  };\n> >  REGISTER_CAMERA_SENSOR_HELPER(\"imx477\", CameraSensorHelperImx477)\n> > @@ -675,8 +622,7 @@ public:\n> >  \t\t * The Sensor Manual doesn't appear to document the gain model.\n> >  \t\t * This has been validated with some empirical testing only.\n> >  \t\t */\n> > -\t\tgainType_ = AnalogueGainLinear;\n> > -\t\tgainConstants_.linear = { 1, 0, 0, 128 };\n> > +\t\tgain_ = AnalogueGainLinear{ 1, 0, 0, 128 };\n> >  \t}\n> >  };\n> >  REGISTER_CAMERA_SENSOR_HELPER(\"ov2685\", CameraSensorHelperOv2685)\n> > @@ -686,8 +632,7 @@ class CameraSensorHelperOv2740 : public CameraSensorHelper\n> >  public:\n> >  \tCameraSensorHelperOv2740()\n> >  \t{\n> > -\t\tgainType_ = AnalogueGainLinear;\n> > -\t\tgainConstants_.linear = { 1, 0, 0, 128 };\n> > +\t\tgain_ = AnalogueGainLinear{ 1, 0, 0, 128 };\n> >  \t}\n> >  };\n> >  REGISTER_CAMERA_SENSOR_HELPER(\"ov2740\", CameraSensorHelperOv2740)\n> > @@ -699,8 +644,7 @@ public:\n> >  \t{\n> >  \t\t/* From datasheet: 0x40 at 12bits. */\n> >  \t\tblackLevel_ = 1024;\n> > -\t\tgainType_ = AnalogueGainLinear;\n> > -\t\tgainConstants_.linear = { 1, 0, 0, 128 };\n> > +\t\tgain_ = AnalogueGainLinear{ 1, 0, 0, 128 };\n> >  \t}\n> >  };\n> >  REGISTER_CAMERA_SENSOR_HELPER(\"ov4689\", CameraSensorHelperOv4689)\n> > @@ -712,8 +656,7 @@ public:\n> >  \t{\n> >  \t\t/* From datasheet: 0x10 at 10bits. */\n> >  \t\tblackLevel_ = 1024;\n> > -\t\tgainType_ = AnalogueGainLinear;\n> > -\t\tgainConstants_.linear = { 1, 0, 0, 16 };\n> > +\t\tgain_ = AnalogueGainLinear{ 1, 0, 0, 16 };\n> >  \t}\n> >  };\n> >  REGISTER_CAMERA_SENSOR_HELPER(\"ov5640\", CameraSensorHelperOv5640)\n> > @@ -723,8 +666,7 @@ class CameraSensorHelperOv5647 : public CameraSensorHelper\n> >  public:\n> >  \tCameraSensorHelperOv5647()\n> >  \t{\n> > -\t\tgainType_ = AnalogueGainLinear;\n> > -\t\tgainConstants_.linear = { 1, 0, 0, 16 };\n> > +\t\tgain_ = AnalogueGainLinear{ 1, 0, 0, 16 };\n> >  \t}\n> >  };\n> >  REGISTER_CAMERA_SENSOR_HELPER(\"ov5647\", CameraSensorHelperOv5647)\n> > @@ -734,8 +676,7 @@ class CameraSensorHelperOv5670 : public CameraSensorHelper\n> >  public:\n> >  \tCameraSensorHelperOv5670()\n> >  \t{\n> > -\t\tgainType_ = AnalogueGainLinear;\n> > -\t\tgainConstants_.linear = { 1, 0, 0, 128 };\n> > +\t\tgain_ = AnalogueGainLinear{ 1, 0, 0, 128 };\n> >  \t}\n> >  };\n> >  REGISTER_CAMERA_SENSOR_HELPER(\"ov5670\", CameraSensorHelperOv5670)\n> > @@ -747,8 +688,7 @@ public:\n> >  \t{\n> >  \t\t/* From Linux kernel driver: 0x40 at 10bits. */\n> >  \t\tblackLevel_ = 4096;\n> > -\t\tgainType_ = AnalogueGainLinear;\n> > -\t\tgainConstants_.linear = { 1, 0, 0, 128 };\n> > +\t\tgain_ = AnalogueGainLinear{ 1, 0, 0, 128 };\n> >  \t}\n> >  };\n> >  REGISTER_CAMERA_SENSOR_HELPER(\"ov5675\", CameraSensorHelperOv5675)\n> > @@ -758,8 +698,7 @@ class CameraSensorHelperOv5693 : public CameraSensorHelper\n> >  public:\n> >  \tCameraSensorHelperOv5693()\n> >  \t{\n> > -\t\tgainType_ = AnalogueGainLinear;\n> > -\t\tgainConstants_.linear = { 1, 0, 0, 16 };\n> > +\t\tgain_ = AnalogueGainLinear{ 1, 0, 0, 16 };\n> >  \t}\n> >  };\n> >  REGISTER_CAMERA_SENSOR_HELPER(\"ov5693\", CameraSensorHelperOv5693)\n> > @@ -769,8 +708,7 @@ class CameraSensorHelperOv64a40 : public CameraSensorHelper\n> >  public:\n> >  \tCameraSensorHelperOv64a40()\n> >  \t{\n> > -\t\tgainType_ = AnalogueGainLinear;\n> > -\t\tgainConstants_.linear = { 1, 0, 0, 128 };\n> > +\t\tgain_ = AnalogueGainLinear{ 1, 0, 0, 128 };\n> >  \t}\n> >  };\n> >  REGISTER_CAMERA_SENSOR_HELPER(\"ov64a40\", CameraSensorHelperOv64a40)\n> > @@ -780,15 +718,13 @@ class CameraSensorHelperOv8858 : public CameraSensorHelper\n> >  public:\n> >  \tCameraSensorHelperOv8858()\n> >  \t{\n> > -\t\tgainType_ = AnalogueGainLinear;\n> > -\n> >  \t\t/*\n> >  \t\t * \\todo Validate the selected 1/128 step value as it differs\n> >  \t\t * from what the sensor manual describes.\n> >  \t\t *\n> >  \t\t * See: https://patchwork.linuxtv.org/project/linux-media/patch/20221106171129.166892-2-nicholas@rothemail.net/#142267\n> >  \t\t */\n> > -\t\tgainConstants_.linear = { 1, 0, 0, 128 };\n> > +\t\tgain_ = AnalogueGainLinear{ 1, 0, 0, 128 };\n> >  \t}\n> >  };\n> >  REGISTER_CAMERA_SENSOR_HELPER(\"ov8858\", CameraSensorHelperOv8858)\n> > @@ -798,8 +734,7 @@ class CameraSensorHelperOv8865 : public CameraSensorHelper\n> >  public:\n> >  \tCameraSensorHelperOv8865()\n> >  \t{\n> > -\t\tgainType_ = AnalogueGainLinear;\n> > -\t\tgainConstants_.linear = { 1, 0, 0, 128 };\n> > +\t\tgain_ = AnalogueGainLinear{ 1, 0, 0, 128 };\n> >  \t}\n> >  };\n> >  REGISTER_CAMERA_SENSOR_HELPER(\"ov8865\", CameraSensorHelperOv8865)\n> > @@ -809,8 +744,7 @@ class CameraSensorHelperOv13858 : public CameraSensorHelper\n> >  public:\n> >  \tCameraSensorHelperOv13858()\n> >  \t{\n> > -\t\tgainType_ = AnalogueGainLinear;\n> > -\t\tgainConstants_.linear = { 1, 0, 0, 128 };\n> > +\t\tgain_ = AnalogueGainLinear{ 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 75868205..a9300a64 100644\n> > --- a/src/ipa/libipa/camera_sensor_helper.h\n> > +++ b/src/ipa/libipa/camera_sensor_helper.h\n> > @@ -11,6 +11,7 @@\n> >  #include <optional>\n> >  #include <stdint.h>\n> >  #include <string>\n> > +#include <variant>\n> >  #include <vector>\n> >  \n> >  #include <libcamera/base/class.h>\n> > @@ -30,31 +31,20 @@ public:\n> >  \tvirtual double gain(uint32_t gainCode) const;\n> >  \n> >  protected:\n> > -\tenum AnalogueGainType {\n> > -\t\tAnalogueGainLinear,\n> > -\t\tAnalogueGainExponential,\n> > -\t};\n> > -\n> > -\tstruct AnalogueGainLinearConstants {\n> > +\tstruct AnalogueGainLinear {\n> >  \t\tint16_t m0;\n> >  \t\tint16_t c0;\n> >  \t\tint16_t m1;\n> >  \t\tint16_t c1;\n> >  \t};\n> >  \n> > -\tstruct AnalogueGainExpConstants {\n> > +\tstruct AnalogueGainExp {\n> >  \t\tdouble a;\n> >  \t\tdouble m;\n> >  \t};\n> >  \n> > -\tunion AnalogueGainConstants {\n> > -\t\tAnalogueGainLinearConstants linear;\n> > -\t\tAnalogueGainExpConstants exp;\n> > -\t};\n> > -\n> >  \tstd::optional<int16_t> blackLevel_;\n> > -\tAnalogueGainType gainType_;\n> > -\tAnalogueGainConstants gainConstants_;\n> > +\tstd::variant<std::monostate, AnalogueGainLinear, AnalogueGainExp> gain_;\n> >  \n> >  private:\n> >  \tLIBCAMERA_DISABLE_COPY_AND_MOVE(CameraSensorHelper)","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 5C9A0C3259\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon,  9 Dec 2024 02:29:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6551D67E49;\n\tMon,  9 Dec 2024 03:29:13 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 80C60618AD\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon,  9 Dec 2024 03:29:11 +0100 (CET)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 03312502;\n\tMon,  9 Dec 2024 03:28:39 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"QlfLpFpX\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1733711320;\n\tbh=3PjddGtwOesmVgAz97pvqJKJyxbxJeH4Y7QGbAZLTmM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=QlfLpFpX0RX6/6//uaFJRCCM4FXDpGrKS9xKw5EVYChAqPfAoGTpA6TRSoWha9DBk\n\tItuiVgvqiVSmyVRsnOcD3PIr9amOmJ8yWaqUYmmMvWq/B4zt2BpNiIahohzIqTx4PW\n\tRgUdimMNYoiKmwTa5nJbYDb7nLxyYtenbEeXv7kE=","Date":"Mon, 9 Dec 2024 04:28:55 +0200","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= <pobrn@protonmail.com>","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v1] libcamera: libipa: camera_sensor_helper: Use\n\t`variant` instead of `union`","Message-ID":"<20241209022855.GA11343@pendragon.ideasonboard.com>","References":"<20241205122702.1105835-1-pobrn@protonmail.com>\n\t<20241205195815.GF21014@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20241205195815.GF21014@pendragon.ideasonboard.com>","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>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]