[{"id":36886,"web_url":"https://patchwork.libcamera.org/comment/36886/","msgid":"<176346973997.880260.12551762065317130529@isaac-ThinkPad-T16-Gen-2>","date":"2025-11-18T12:42:19","subject":"Re: [PATCH v4 20/21] ipa: libipa: fixedpoint: Move float conversion\n\tinline","submitter":{"id":215,"url":"https://patchwork.libcamera.org/api/people/215/","name":"Isaac Scott","email":"isaac.scott@ideasonboard.com"},"content":"Hi Kieran,\n\nThank you for the patch!\n\nReviewed-by: Isaac Scott \n\nQuoting Kieran Bingham (2025-11-14 00:54:24)\n> With all users of the floatingToFixedPoint and fixedToFloatingPoint\n> calls converted to use the FixedPoint Quantized types, remove the calls\n> and documentation and move the implementation inline in the\n> FixedPointTraits implementation.\n> \n> Signed-off-by: Kieran Bingham \n> ---\n> src/ipa/libipa/fixedpoint.cpp | 22 ----------\n> src/ipa/libipa/fixedpoint.h | 76 +++++++++++------------------------\n> 2 files changed, 23 insertions(+), 75 deletions(-)\n> \n> diff --git a/src/ipa/libipa/fixedpoint.cpp b/src/ipa/libipa/fixedpoint.cpp\n> index 97cdd9b3f7aa..f2c93b1cf706 100644\n> --- a/src/ipa/libipa/fixedpoint.cpp\n> +++ b/src/ipa/libipa/fixedpoint.cpp\n> @@ -15,28 +15,6 @@ namespace libcamera {\n> \n> namespace ipa {\n> \n> -/**\n> - * \\fn R floatingToFixedPoint(T number)\n> - * \\brief Convert a floating point number to a fixed-point representation\n> - * \\tparam I Bit width of the integer part of the fixed-point\n> - * \\tparam F Bit width of the fractional part of the fixed-point\n> - * \\tparam R Return type of the fixed-point representation\n> - * \\tparam T Input type of the floating point representation\n> - * \\param number The floating point number to convert to fixed point\n> - * \\return The converted value\n> - */\n> -\n> -/**\n> - * \\fn R fixedToFloatingPoint(T number)\n> - * \\brief Convert a fixed-point number to a floating point representation\n> - * \\tparam I Bit width of the integer part of the fixed-point\n> - * \\tparam F Bit width of the fractional part of the fixed-point\n> - * \\tparam R Return type of the floating point representation\n> - * \\tparam T Input type of the fixed-point representation\n> - * \\param number The fixed point number to convert to floating point\n> - * \\return The converted value\n> - */\n> -\n> /**\n> * \\struct libcamera::ipa::FixedPointQTraits\n> * \\brief Traits type implementing fixed-point quantisation conversions\n> diff --git a/src/ipa/libipa/fixedpoint.h b/src/ipa/libipa/fixedpoint.h\n> index 1e984350111c..fe75af16bb54 100644\n> --- a/src/ipa/libipa/fixedpoint.h\n> +++ b/src/ipa/libipa/fixedpoint.h\n> @@ -16,55 +16,6 @@ namespace libcamera {\n> \n> namespace ipa {\n> \n> -#ifndef __DOXYGEN__\n> -template - std::enable_if_t &&\n> - std::is_floating_point_v> * = nullptr>\n> -#else\n> -template\n> -#endif\n> -constexpr R floatingToFixedPoint(T number)\n> -{\n> - static_assert(sizeof(int) >= sizeof(R));\n> - static_assert(I + F <= sizeof(R) * 8);\n> -\n> - /*\n> - * The intermediate cast to int is needed on arm platforms to properly\n> - * cast negative values. See\n> - * https://embeddeduse.com/2013/08/25/casting-a-negative-float-to-an-unsigned-int/\n> - */\n> - R mask = (1 << (F + I)) - 1;\n> - R frac = static_cast(static_cast(std::round(number * (1 << F)))) & mask;\n> -\n> - return frac;\n> -}\n> -\n> -#ifndef __DOXYGEN__\n> -template - std::enable_if_t &&\n> - std::is_integral_v> * = nullptr>\n> -#else\n> -template\n> -#endif\n> -constexpr R fixedToFloatingPoint(T number)\n> -{\n> - static_assert(sizeof(int) >= sizeof(T));\n> - static_assert(I + F <= sizeof(T) * 8);\n> -\n> - if constexpr (std::is_unsigned_v)\n> - return static_cast(number) / static_cast(1 << F);\n> -\n> - /*\n> - * Recreate the upper bits in case of a negative number by shifting the sign\n> - * bit from the fixed point to the first bit of the unsigned and then right shifting\n> - * by the same amount which keeps the sign bit in place.\n> - * This can be optimized by the compiler quite well.\n> - */\n> - int remaining_bits = sizeof(int) * 8 - (I + F);\n> - int t = static_cast(static_cast(number) << remaining_bits) >> remaining_bits;\n> - return static_cast(t) / static_cast(1 << F);\n> -}\n> -\n> template\n> struct FixedPointQTraits {\n> static_assert(std::is_integral_v, \"FixedPointQTraits: T must be integral\");\n> @@ -88,17 +39,36 @@ struct FixedPointQTraits {\n> \n> static constexpr float toFloat(QuantizedType q)\n> {\n> - return fixedToFloatingPoint(q);\n> + if constexpr (std::is_unsigned_v)\n> + return static_cast(q) / static_cast(1 << F);\n> +\n> + /*\n> + * Recreate the upper bits in case of a negative number by shifting the sign\n> + * bit from the fixed point to the first bit of the unsigned and then right shifting\n> + * by the same amount which keeps the sign bit in place.\n> + * This can be optimized by the compiler quite well.\n> + */\n> + static_assert(sizeof(int) >= sizeof(T));\n> +\n> + int remaining_bits = sizeof(int) * 8 - (I + F);\n> + int t = static_cast(static_cast(q) << remaining_bits) >> remaining_bits;\n> + return static_cast(t) / static_cast(1 << F);\n> }\n> \n> - static constexpr float min = fixedToFloatingPoint(qmin);\n> - static constexpr float max = fixedToFloatingPoint(qmax);\n> + static constexpr float min = toFloat(qmin);\n> + static constexpr float max = toFloat(qmax);\n> \n> /* Conversion functions required by Quantized */\n> static constexpr QuantizedType fromFloat(float v)\n> {\n> v = std::clamp(v, min, max);\n> - return floatingToFixedPoint(v);\n> +\n> + /*\n> + * The intermediate cast to int is needed on arm platforms to properly\n> + * cast negative values. See\n> + * https://embeddeduse.com/2013/08/25/casting-a-negative-float-to-an-unsigned-int/\n> + */\n> + return static_cast(static_cast(std::round(v * (1 << F)))) & BitMask;\n> }\n> };\n> \n> -- \n> 2.51.1\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 03126C0F1B\n\tfor ;\n\tTue, 18 Nov 2025 12:42:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B425D60AAE;\n\tTue, 18 Nov 2025 13:42:23 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9B843606D5\n\tfor ;\n\tTue, 18 Nov 2025 13:42:22 +0100 (CET)","from thinkpad.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D6991735;\n\tTue, 18 Nov 2025 13:40:18 +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=\"ENlg+XNH\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1763469619;\n\tbh=QOdLpIiCYQrqHK5ZNpx7Zq83MXlIN4wzCwXikxopdxg=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=ENlg+XNHw0roSb2pGirz+Ew3Rf0Xd0Ey2eUo1+IpA48nRN9Fk/kxnQM+AVvfg3gKo\n\tPzRtboC+bNjIamv43t97DUnHiM3O81dQSekwm5b2SM7pSkALXD7n/uw5cAM4fA51aM\n\tSlwnjjZyH1gDFH/0jOw9JOcFgNaB2xwHuHIJGWew=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20251114005428.90024-21-kieran.bingham@ideasonboard.com>","References":"<20251114005428.90024-1-kieran.bingham@ideasonboard.com>\n\t<20251114005428.90024-21-kieran.bingham@ideasonboard.com>","Subject":"Re: [PATCH v4 20/21] ipa: libipa: fixedpoint: Move float conversion\n\tinline","From":"Isaac Scott ","Cc":"Kieran Bingham ","To":"Kieran Bingham ,\n\tlibcamera devel ","Date":"Tue, 18 Nov 2025 12:42:19 +0000","Message-ID":"<176346973997.880260.12551762065317130529@isaac-ThinkPad-T16-Gen-2>","User-Agent":"alot/0.10","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":36908,"web_url":"https://patchwork.libcamera.org/comment/36908/","msgid":"<20251119043154.GG17526@pendragon.ideasonboard.com>","date":"2025-11-19T04:31:54","subject":"Re: [PATCH v4 20/21] ipa: libipa: fixedpoint: Move float conversion\n\tinline","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Fri, Nov 14, 2025 at 12:54:24AM +0000, Kieran Bingham wrote:\n> With all users of the floatingToFixedPoint and fixedToFloatingPoint\n> calls converted to use the FixedPoint Quantized types, remove the calls\n> and documentation and move the implementation inline in the\n> FixedPointTraits implementation.\n> \n> Signed-off-by: Kieran Bingham \n> ---\n> src/ipa/libipa/fixedpoint.cpp | 22 ----------\n> src/ipa/libipa/fixedpoint.h | 76 +++++++++++------------------------\n> 2 files changed, 23 insertions(+), 75 deletions(-)\n> \n> diff --git a/src/ipa/libipa/fixedpoint.cpp b/src/ipa/libipa/fixedpoint.cpp\n> index 97cdd9b3f7aa..f2c93b1cf706 100644\n> --- a/src/ipa/libipa/fixedpoint.cpp\n> +++ b/src/ipa/libipa/fixedpoint.cpp\n> @@ -15,28 +15,6 @@ namespace libcamera {\n> \n> namespace ipa {\n> \n> -/**\n> - * \\fn R floatingToFixedPoint(T number)\n> - * \\brief Convert a floating point number to a fixed-point representation\n> - * \\tparam I Bit width of the integer part of the fixed-point\n> - * \\tparam F Bit width of the fractional part of the fixed-point\n> - * \\tparam R Return type of the fixed-point representation\n> - * \\tparam T Input type of the floating point representation\n> - * \\param number The floating point number to convert to fixed point\n> - * \\return The converted value\n> - */\n> -\n> -/**\n> - * \\fn R fixedToFloatingPoint(T number)\n> - * \\brief Convert a fixed-point number to a floating point representation\n> - * \\tparam I Bit width of the integer part of the fixed-point\n> - * \\tparam F Bit width of the fractional part of the fixed-point\n> - * \\tparam R Return type of the floating point representation\n> - * \\tparam T Input type of the fixed-point representation\n> - * \\param number The fixed point number to convert to floating point\n> - * \\return The converted value\n> - */\n> -\n> /**\n> * \\struct libcamera::ipa::FixedPointQTraits\n> * \\brief Traits type implementing fixed-point quantisation conversions\n> diff --git a/src/ipa/libipa/fixedpoint.h b/src/ipa/libipa/fixedpoint.h\n> index 1e984350111c..fe75af16bb54 100644\n> --- a/src/ipa/libipa/fixedpoint.h\n> +++ b/src/ipa/libipa/fixedpoint.h\n> @@ -16,55 +16,6 @@ namespace libcamera {\n> \n> namespace ipa {\n> \n> -#ifndef __DOXYGEN__\n> -template -\t std::enable_if_t &&\n> -\t\t\t std::is_floating_point_v> * = nullptr>\n> -#else\n> -template\n> -#endif\n> -constexpr R floatingToFixedPoint(T number)\n> -{\n> -\tstatic_assert(sizeof(int) >= sizeof(R));\n> -\tstatic_assert(I + F <= sizeof(R) * 8);\n> -\n> -\t/*\n> -\t * The intermediate cast to int is needed on arm platforms to properly\n> -\t * cast negative values. See\n> -\t * https://embeddeduse.com/2013/08/25/casting-a-negative-float-to-an-unsigned-int/\n> -\t */\n> -\tR mask = (1 << (F + I)) - 1;\n> -\tR frac = static_cast(static_cast(std::round(number * (1 << F)))) & mask;\n> -\n> -\treturn frac;\n> -}\n> -\n> -#ifndef __DOXYGEN__\n> -template -\t std::enable_if_t &&\n> -\t\t\t std::is_integral_v> * = nullptr>\n> -#else\n> -template\n> -#endif\n> -constexpr R fixedToFloatingPoint(T number)\n> -{\n> -\tstatic_assert(sizeof(int) >= sizeof(T));\n> -\tstatic_assert(I + F <= sizeof(T) * 8);\n> -\n> -\tif constexpr (std::is_unsigned_v)\n> -\t\treturn static_cast(number) / static_cast(1 << F);\n> -\n> -\t/*\n> -\t * Recreate the upper bits in case of a negative number by shifting the sign\n> -\t * bit from the fixed point to the first bit of the unsigned and then right shifting\n> -\t * by the same amount which keeps the sign bit in place.\n> -\t * This can be optimized by the compiler quite well.\n> -\t */\n> -\tint remaining_bits = sizeof(int) * 8 - (I + F);\n> -\tint t = static_cast(static_cast(number) << remaining_bits) >> remaining_bits;\n> -\treturn static_cast(t) / static_cast(1 << F);\n> -}\n> -\n> template\n> struct FixedPointQTraits {\n> \tstatic_assert(std::is_integral_v, \"FixedPointQTraits: T must be integral\");\n> @@ -88,17 +39,36 @@ struct FixedPointQTraits {\n> \n> \tstatic constexpr float toFloat(QuantizedType q)\n> \t{\n> -\t\treturn fixedToFloatingPoint(q);\n> +\t\tif constexpr (std::is_unsigned_v)\n> +\t\t\treturn static_cast(q) / static_cast(1 << F);\n> +\n> +\t\t/*\n> +\t\t * Recreate the upper bits in case of a negative number by shifting the sign\n> +\t\t * bit from the fixed point to the first bit of the unsigned and then right shifting\n> +\t\t * by the same amount which keeps the sign bit in place.\n> +\t\t * This can be optimized by the compiler quite well.\n> +\t\t */\n\nWhile at it, please reflow to 80 columns.\n\n> +\t\tstatic_assert(sizeof(int) >= sizeof(T));\n> +\n> +\t\tint remaining_bits = sizeof(int) * 8 - (I + F);\n> +\t\tint t = static_cast(static_cast(q) << remaining_bits) >> remaining_bits;\n> +\t\treturn static_cast(t) / static_cast(1 << F);\n> \t}\n> \n> -\tstatic constexpr float min = fixedToFloatingPoint(qmin);\n> -\tstatic constexpr float max = fixedToFloatingPoint(qmax);\n> +\tstatic constexpr float min = toFloat(qmin);\n> +\tstatic constexpr float max = toFloat(qmax);\n> \n> \t/* Conversion functions required by Quantized */\n> \tstatic constexpr QuantizedType fromFloat(float v)\n> \t{\n> \t\tv = std::clamp(v, min, max);\n> -\t\treturn floatingToFixedPoint(v);\n> +\n> +\t\t/*\n> +\t\t * The intermediate cast to int is needed on arm platforms to properly\n> +\t\t * cast negative values. See\n\nSame here.\n\nReviewed-by: Laurent Pinchart \n\n> +\t\t * https://embeddeduse.com/2013/08/25/casting-a-negative-float-to-an-unsigned-int/\n> +\t\t */\n> +\t\treturn static_cast(static_cast(std::round(v * (1 << F)))) & BitMask;\n> \t}\n> };\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 329B2BD80A\n\tfor ;\n\tWed, 19 Nov 2025 04:32:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CFC4D60A8B;\n\tWed, 19 Nov 2025 05:32:32 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8B17160A7B\n\tfor ;\n\tWed, 19 Nov 2025 05:32:31 +0100 (CET)","from pendragon.ideasonboard.com (unknown [205.220.129.225])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id 5DB4FB5;\n\tWed, 19 Nov 2025 05:30:22 +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=\"EzUV8JkF\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1763526627;\n\tbh=2n9WjasVWPW37G5UjuKPXMKspAqaZ0en++9a5t8etyI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=EzUV8JkFT4mCRwV1T9God//HeosyDjVxFT/NLuZJxba6afv1L0y66dFpCg9XpUoea\n\tyIuhYWKw7+x3wKCxBMlf6Jhtef0wXpsfEyA2867BF0d1O1vvCGPmlUm4gTXiJGF5FV\n\tlYazelsh8R/ATFnfoYprKSSU1Ni4RO7Xs8Suxe2I=","Date":"Wed, 19 Nov 2025 13:31:54 +0900","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"libcamera devel ","Subject":"Re: [PATCH v4 20/21] ipa: libipa: fixedpoint: Move float conversion\n\tinline","Message-ID":"<20251119043154.GG17526@pendragon.ideasonboard.com>","References":"<20251114005428.90024-1-kieran.bingham@ideasonboard.com>\n\t<20251114005428.90024-21-kieran.bingham@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20251114005428.90024-21-kieran.bingham@ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":36955,"web_url":"https://patchwork.libcamera.org/comment/36955/","msgid":"<422fb917-5221-4f12-9c61-b6402f65ef4a@ideasonboard.com>","date":"2025-11-20T17:26:42","subject":"Re: [PATCH v4 20/21] ipa: libipa: fixedpoint: Move float conversion\n\tinline","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"2025. 11. 14. 1:54 keltezéssel, Kieran Bingham írta:\n> With all users of the floatingToFixedPoint and fixedToFloatingPoint\n> calls converted to use the FixedPoint Quantized types, remove the calls\n> and documentation and move the implementation inline in the\n> FixedPointTraits implementation.\n\nI'm wondering if we really want to remove them, the functions are somewhat\nmore flexible (e.g. allowing `double`). But arguably that is not used after\nthese changes.\n\nIf yes:\nReviewed-by: Barnabás Pőcze \n\n> \n> Signed-off-by: Kieran Bingham \n> ---\n> src/ipa/libipa/fixedpoint.cpp | 22 ----------\n> src/ipa/libipa/fixedpoint.h | 76 +++++++++++------------------------\n> 2 files changed, 23 insertions(+), 75 deletions(-)\n> \n> diff --git a/src/ipa/libipa/fixedpoint.cpp b/src/ipa/libipa/fixedpoint.cpp\n> index 97cdd9b3f7aa..f2c93b1cf706 100644\n> --- a/src/ipa/libipa/fixedpoint.cpp\n> +++ b/src/ipa/libipa/fixedpoint.cpp\n> @@ -15,28 +15,6 @@ namespace libcamera {\n> \n> namespace ipa {\n> \n> -/**\n> - * \\fn R floatingToFixedPoint(T number)\n> - * \\brief Convert a floating point number to a fixed-point representation\n> - * \\tparam I Bit width of the integer part of the fixed-point\n> - * \\tparam F Bit width of the fractional part of the fixed-point\n> - * \\tparam R Return type of the fixed-point representation\n> - * \\tparam T Input type of the floating point representation\n> - * \\param number The floating point number to convert to fixed point\n> - * \\return The converted value\n> - */\n> -\n> -/**\n> - * \\fn R fixedToFloatingPoint(T number)\n> - * \\brief Convert a fixed-point number to a floating point representation\n> - * \\tparam I Bit width of the integer part of the fixed-point\n> - * \\tparam F Bit width of the fractional part of the fixed-point\n> - * \\tparam R Return type of the floating point representation\n> - * \\tparam T Input type of the fixed-point representation\n> - * \\param number The fixed point number to convert to floating point\n> - * \\return The converted value\n> - */\n> -\n> /**\n> * \\struct libcamera::ipa::FixedPointQTraits\n> * \\brief Traits type implementing fixed-point quantisation conversions\n> diff --git a/src/ipa/libipa/fixedpoint.h b/src/ipa/libipa/fixedpoint.h\n> index 1e984350111c..fe75af16bb54 100644\n> --- a/src/ipa/libipa/fixedpoint.h\n> +++ b/src/ipa/libipa/fixedpoint.h\n> @@ -16,55 +16,6 @@ namespace libcamera {\n> \n> namespace ipa {\n> \n> -#ifndef __DOXYGEN__\n> -template -\t std::enable_if_t &&\n> -\t\t\t std::is_floating_point_v> * = nullptr>\n> -#else\n> -template\n> -#endif\n> -constexpr R floatingToFixedPoint(T number)\n> -{\n> -\tstatic_assert(sizeof(int) >= sizeof(R));\n> -\tstatic_assert(I + F <= sizeof(R) * 8);\n> -\n> -\t/*\n> -\t * The intermediate cast to int is needed on arm platforms to properly\n> -\t * cast negative values. See\n> -\t * https://embeddeduse.com/2013/08/25/casting-a-negative-float-to-an-unsigned-int/\n> -\t */\n> -\tR mask = (1 << (F + I)) - 1;\n> -\tR frac = static_cast(static_cast(std::round(number * (1 << F)))) & mask;\n> -\n> -\treturn frac;\n> -}\n> -\n> -#ifndef __DOXYGEN__\n> -template -\t std::enable_if_t &&\n> -\t\t\t std::is_integral_v> * = nullptr>\n> -#else\n> -template\n> -#endif\n> -constexpr R fixedToFloatingPoint(T number)\n> -{\n> -\tstatic_assert(sizeof(int) >= sizeof(T));\n> -\tstatic_assert(I + F <= sizeof(T) * 8);\n> -\n> -\tif constexpr (std::is_unsigned_v)\n> -\t\treturn static_cast(number) / static_cast(1 << F);\n> -\n> -\t/*\n> -\t * Recreate the upper bits in case of a negative number by shifting the sign\n> -\t * bit from the fixed point to the first bit of the unsigned and then right shifting\n> -\t * by the same amount which keeps the sign bit in place.\n> -\t * This can be optimized by the compiler quite well.\n> -\t */\n> -\tint remaining_bits = sizeof(int) * 8 - (I + F);\n> -\tint t = static_cast(static_cast(number) << remaining_bits) >> remaining_bits;\n> -\treturn static_cast(t) / static_cast(1 << F);\n> -}\n> -\n> template\n> struct FixedPointQTraits {\n> \tstatic_assert(std::is_integral_v, \"FixedPointQTraits: T must be integral\");\n> @@ -88,17 +39,36 @@ struct FixedPointQTraits {\n> \n> \tstatic constexpr float toFloat(QuantizedType q)\n> \t{\n> -\t\treturn fixedToFloatingPoint(q);\n> +\t\tif constexpr (std::is_unsigned_v)\n> +\t\t\treturn static_cast(q) / static_cast(1 << F);\n> +\n> +\t\t/*\n> +\t\t * Recreate the upper bits in case of a negative number by shifting the sign\n> +\t\t * bit from the fixed point to the first bit of the unsigned and then right shifting\n> +\t\t * by the same amount which keeps the sign bit in place.\n> +\t\t * This can be optimized by the compiler quite well.\n> +\t\t */\n> +\t\tstatic_assert(sizeof(int) >= sizeof(T));\n\nI'm wondering how easily this limitation can be lifted, e.g. is\n\n // fromFloat\n static_cast(static_cast>(std::round(v * (1 << F)))) & BitMask\n \n\n // toFloat\n static_cast(static_cast>(q) << remaining_bits) >> remaining_bits);\n\nsufficient?\n\n\n> +\n> +\t\tint remaining_bits = sizeof(int) * 8 - (I + F);\n\nThis could very well be a `constexpr` thing.\n\n\n> +\t\tint t = static_cast(static_cast(q) << remaining_bits) >> remaining_bits;\n> +\t\treturn static_cast(t) / static_cast(1 << F);\n> \t}\n> \n> -\tstatic constexpr float min = fixedToFloatingPoint(qmin);\n> -\tstatic constexpr float max = fixedToFloatingPoint(qmax);\n> +\tstatic constexpr float min = toFloat(qmin);\n> +\tstatic constexpr float max = toFloat(qmax);\n> \n> \t/* Conversion functions required by Quantized */\n> \tstatic constexpr QuantizedType fromFloat(float v)\n> \t{\n> \t\tv = std::clamp(v, min, max);\n> -\t\treturn floatingToFixedPoint(v);\n> +\n> +\t\t/*\n> +\t\t * The intermediate cast to int is needed on arm platforms to properly\n> +\t\t * cast negative values. See\n> +\t\t * https://embeddeduse.com/2013/08/25/casting-a-negative-float-to-an-unsigned-int/\n> +\t\t */\n> +\t\treturn static_cast(static_cast(std::round(v * (1 << F)))) & BitMask;\n> \t}\n> };\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 EFEA4BD80A\n\tfor ;\n\tThu, 20 Nov 2025 17:26:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3628A60A8B;\n\tThu, 20 Nov 2025 18:26:51 +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 1033F6069A\n\tfor ;\n\tThu, 20 Nov 2025 18:26:49 +0100 (CET)","from [192.168.33.37] (185.221.143.100.nat.pool.zt.hu\n\t[185.221.143.100])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 9B2F0C77;\n\tThu, 20 Nov 2025 18:24:40 +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=\"SmvUzrMR\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1763659483;\n\tbh=BjW3Pt94zJmxH2cS+GSKpqmzBT3CZDi6U+fzf2wbWuM=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=SmvUzrMR8iNGuzDfoiPLKaR8FzZtikSDe9bJBoxDpYU20IjeZ0rReTGnBpRDWwSib\n\teIkStThhgp6aEF67/P9f39n1A+rawpYXaieTiQZcmianKrR/mlj/1dqapprcoCRnP0\n\t38JkCTr1OuHmLqpq0kgqqmbKpYaxXmX0iGlkADe0=","Message-ID":"<422fb917-5221-4f12-9c61-b6402f65ef4a@ideasonboard.com>","Date":"Thu, 20 Nov 2025 18:26:42 +0100","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v4 20/21] ipa: libipa: fixedpoint: Move float conversion\n\tinline","To":"Kieran Bingham ,\n\tlibcamera devel ","References":"<20251114005428.90024-1-kieran.bingham@ideasonboard.com>\n\t<20251114005428.90024-21-kieran.bingham@ideasonboard.com>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"<20251114005428.90024-21-kieran.bingham@ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":36956,"web_url":"https://patchwork.libcamera.org/comment/36956/","msgid":"<176366105236.567526.7063638060431552764@ping.linuxembedded.co.uk>","date":"2025-11-20T17:50:52","subject":"Re: [PATCH v4 20/21] ipa: libipa: fixedpoint: Move float conversion\n\tinline","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Barnabás Pőcze (2025-11-20 17:26:42)\n> 2025. 11. 14. 1:54 keltezéssel, Kieran Bingham írta:\n> > With all users of the floatingToFixedPoint and fixedToFloatingPoint\n> > calls converted to use the FixedPoint Quantized types, remove the calls\n> > and documentation and move the implementation inline in the\n> > FixedPointTraits implementation.\n> \n> I'm wondering if we really want to remove them, the functions are somewhat\n> more flexible (e.g. allowing `double`). But arguably that is not used after\n> these changes.\n\nIf we get any users of types bigger than Q 32> perhaps we\ncould consider extending the Q type to be able to support double but I\ndon't see that being likely\n\n> \n> If yes:\n> Reviewed-by: Barnabás Pőcze \n\nThanks\n\nKieran\n\n> \n> > \n> > Signed-off-by: Kieran Bingham \n> > ---\n> > src/ipa/libipa/fixedpoint.cpp | 22 ----------\n> > src/ipa/libipa/fixedpoint.h | 76 +++++++++++------------------------\n> > 2 files changed, 23 insertions(+), 75 deletions(-)\n> > \n> > diff --git a/src/ipa/libipa/fixedpoint.cpp b/src/ipa/libipa/fixedpoint.cpp\n> > index 97cdd9b3f7aa..f2c93b1cf706 100644\n> > --- a/src/ipa/libipa/fixedpoint.cpp\n> > +++ b/src/ipa/libipa/fixedpoint.cpp\n> > @@ -15,28 +15,6 @@ namespace libcamera {\n> > \n> > namespace ipa {\n> > \n> > -/**\n> > - * \\fn R floatingToFixedPoint(T number)\n> > - * \\brief Convert a floating point number to a fixed-point representation\n> > - * \\tparam I Bit width of the integer part of the fixed-point\n> > - * \\tparam F Bit width of the fractional part of the fixed-point\n> > - * \\tparam R Return type of the fixed-point representation\n> > - * \\tparam T Input type of the floating point representation\n> > - * \\param number The floating point number to convert to fixed point\n> > - * \\return The converted value\n> > - */\n> > -\n> > -/**\n> > - * \\fn R fixedToFloatingPoint(T number)\n> > - * \\brief Convert a fixed-point number to a floating point representation\n> > - * \\tparam I Bit width of the integer part of the fixed-point\n> > - * \\tparam F Bit width of the fractional part of the fixed-point\n> > - * \\tparam R Return type of the floating point representation\n> > - * \\tparam T Input type of the fixed-point representation\n> > - * \\param number The fixed point number to convert to floating point\n> > - * \\return The converted value\n> > - */\n> > -\n> > /**\n> > * \\struct libcamera::ipa::FixedPointQTraits\n> > * \\brief Traits type implementing fixed-point quantisation conversions\n> > diff --git a/src/ipa/libipa/fixedpoint.h b/src/ipa/libipa/fixedpoint.h\n> > index 1e984350111c..fe75af16bb54 100644\n> > --- a/src/ipa/libipa/fixedpoint.h\n> > +++ b/src/ipa/libipa/fixedpoint.h\n> > @@ -16,55 +16,6 @@ namespace libcamera {\n> > \n> > namespace ipa {\n> > \n> > -#ifndef __DOXYGEN__\n> > -template > - std::enable_if_t &&\n> > - std::is_floating_point_v> * = nullptr>\n> > -#else\n> > -template\n> > -#endif\n> > -constexpr R floatingToFixedPoint(T number)\n> > -{\n> > - static_assert(sizeof(int) >= sizeof(R));\n> > - static_assert(I + F <= sizeof(R) * 8);\n> > -\n> > - /*\n> > - * The intermediate cast to int is needed on arm platforms to properly\n> > - * cast negative values. See\n> > - * https://embeddeduse.com/2013/08/25/casting-a-negative-float-to-an-unsigned-int/\n> > - */\n> > - R mask = (1 << (F + I)) - 1;\n> > - R frac = static_cast(static_cast(std::round(number * (1 << F)))) & mask;\n> > -\n> > - return frac;\n> > -}\n> > -\n> > -#ifndef __DOXYGEN__\n> > -template > - std::enable_if_t &&\n> > - std::is_integral_v> * = nullptr>\n> > -#else\n> > -template\n> > -#endif\n> > -constexpr R fixedToFloatingPoint(T number)\n> > -{\n> > - static_assert(sizeof(int) >= sizeof(T));\n> > - static_assert(I + F <= sizeof(T) * 8);\n> > -\n> > - if constexpr (std::is_unsigned_v)\n> > - return static_cast(number) / static_cast(1 << F);\n> > -\n> > - /*\n> > - * Recreate the upper bits in case of a negative number by shifting the sign\n> > - * bit from the fixed point to the first bit of the unsigned and then right shifting\n> > - * by the same amount which keeps the sign bit in place.\n> > - * This can be optimized by the compiler quite well.\n> > - */\n> > - int remaining_bits = sizeof(int) * 8 - (I + F);\n> > - int t = static_cast(static_cast(number) << remaining_bits) >> remaining_bits;\n> > - return static_cast(t) / static_cast(1 << F);\n> > -}\n> > -\n> > template\n> > struct FixedPointQTraits {\n> > static_assert(std::is_integral_v, \"FixedPointQTraits: T must be integral\");\n> > @@ -88,17 +39,36 @@ struct FixedPointQTraits {\n> > \n> > static constexpr float toFloat(QuantizedType q)\n> > {\n> > - return fixedToFloatingPoint(q);\n> > + if constexpr (std::is_unsigned_v)\n> > + return static_cast(q) / static_cast(1 << F);\n> > +\n> > + /*\n> > + * Recreate the upper bits in case of a negative number by shifting the sign\n> > + * bit from the fixed point to the first bit of the unsigned and then right shifting\n> > + * by the same amount which keeps the sign bit in place.\n> > + * This can be optimized by the compiler quite well.\n> > + */\n> > + static_assert(sizeof(int) >= sizeof(T));\n> \n> I'm wondering how easily this limitation can be lifted, e.g. is\n> \n> // fromFloat\n> static_cast(static_cast>(std::round(v * (1 << F)))) & BitMask\n> \n> \n> // toFloat\n> static_cast(static_cast>(q) << remaining_bits) >> remaining_bits);\n> \n> sufficient?\n> \n> \n> > +\n> > + int remaining_bits = sizeof(int) * 8 - (I + F);\n> \n> This could very well be a `constexpr` thing.\n> \n> \n> > + int t = static_cast(static_cast(q) << remaining_bits) >> remaining_bits;\n> > + return static_cast(t) / static_cast(1 << F);\n> > }\n> > \n> > - static constexpr float min = fixedToFloatingPoint(qmin);\n> > - static constexpr float max = fixedToFloatingPoint(qmax);\n> > + static constexpr float min = toFloat(qmin);\n> > + static constexpr float max = toFloat(qmax);\n> > \n> > /* Conversion functions required by Quantized */\n> > static constexpr QuantizedType fromFloat(float v)\n> > {\n> > v = std::clamp(v, min, max);\n> > - return floatingToFixedPoint(v);\n> > +\n> > + /*\n> > + * The intermediate cast to int is needed on arm platforms to properly\n> > + * cast negative values. See\n> > + * https://embeddeduse.com/2013/08/25/casting-a-negative-float-to-an-unsigned-int/\n> > + */\n> > + return static_cast(static_cast(std::round(v * (1 << F)))) & BitMask;\n> > }\n> > };\n> > \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 5452CC3330\n\tfor ;\n\tThu, 20 Nov 2025 17:50:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A46D26069A;\n\tThu, 20 Nov 2025 18:50:57 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 041866069A\n\tfor ;\n\tThu, 20 Nov 2025 18:50:56 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 68510C77;\n\tThu, 20 Nov 2025 18:48:50 +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=\"AbKhTDtm\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1763660930;\n\tbh=luDgko2rk6SrjE7KZg6iHUsfPeiGKtmWEhXXZt011+0=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=AbKhTDtmXyNxkazdOtkGxoe50gsbm5yomN0CqNGR7mtUxJdAsABNfVP1QPwcisRbS\n\tJh2phU7oBEZhuKAOQzzvPT5E43bk8VmJle3wF5sTFDubR9SEoVzS6AUUkvBAebwnJ9\n\t41BuArv55oJ8Y0DVMlJWAEpQBJ/+mTEz07CUprWc=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<422fb917-5221-4f12-9c61-b6402f65ef4a@ideasonboard.com>","References":"<20251114005428.90024-1-kieran.bingham@ideasonboard.com>\n\t<20251114005428.90024-21-kieran.bingham@ideasonboard.com>\n\t<422fb917-5221-4f12-9c61-b6402f65ef4a@ideasonboard.com>","Subject":"Re: [PATCH v4 20/21] ipa: libipa: fixedpoint: Move float conversion\n\tinline","From":"Kieran Bingham ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ,\n\tlibcamera devel ","Date":"Thu, 20 Nov 2025 17:50:52 +0000","Message-ID":"<176366105236.567526.7063638060431552764@ping.linuxembedded.co.uk>","User-Agent":"alot/0.9.1","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]