[{"id":32675,"web_url":"https://patchwork.libcamera.org/comment/32675/","msgid":"<33dd713b-e910-4932-8028-a540de6183cd@ideasonboard.com>","date":"2024-12-11T11:46:14","subject":"Re: [RFC PATCH 1/2] libcamera: bitdepth: Add BitDepth implementation","submitter":{"id":156,"url":"https://patchwork.libcamera.org/api/people/156/","name":"Dan Scally","email":"dan.scally@ideasonboard.com"},"content":"Hi Isaac, thanks for the patches\n\nOn 27/11/2024 14:46, Isaac Scott wrote:\n> Add a class template that simplifies bit depth conversion. This\n> functionality allows users to avoid having to perform inline bitshifting\n> to convert values of one bit depth to another.\n>\n> For example, this makes it easier to input values from datasheets, where\n> a value may be expressed in a certain bit depth. It also removes the\n> potential for human errors when making these conversions manually, and\n> in a lot of cases makes bit depth conversions more readable.\n>\n> Signed-off-by: Isaac Scott \n> ---\n> src/ipa/libipa/bitdepth.h | 86 ++++++++++++++++++++++++++++\n> test/ipa/libipa/bitdepth.cpp | 107 +++++++++++++++++++++++++++++++++++\n> 2 files changed, 193 insertions(+)\n> create mode 100644 src/ipa/libipa/bitdepth.h\n> create mode 100644 test/ipa/libipa/bitdepth.cpp\n>\n> diff --git a/src/ipa/libipa/bitdepth.h b/src/ipa/libipa/bitdepth.h\n> new file mode 100644\n> index 00000000..145ee093\n> --- /dev/null\n> +++ b/src/ipa/libipa/bitdepth.h\n> @@ -0,0 +1,86 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2024 Ideas On Board Oy.\n> + *\n> + * BitDepth class to abstract bit shift operations\n> + */\n> +\n> +#pragma once\n> +\n> +template\n> +class BitDepthValue\n> +{\n> +public:\n> +\tstatic_assert(BitDepth > 0, \"Bit depth must be positive\");\n> +\n> +\tBitDepthValue()\n> +\t\t: value_(0) {}\n> +\n> +\tBitDepthValue(int value)\n> +\t\t: value_(value) {}\n> +\n> +\tBitDepthValue(unsigned int value)\n> +\t\t: value_(value) {}\n> +\n> +\ttemplate\n> +\tBitDepthValue convert() const\n> +\t{\n> +\t\tstatic_assert(TargetBitDepth > 0, \"Bit depth must be positive\");\n> +\n> +\t\tunsigned int shift;\n> +\n> +\t\tif constexpr (BitDepth > TargetBitDepth) {\n> +\t\t\tshift = BitDepth - TargetBitDepth;\n> +\t\t\treturn BitDepthValue(value_ >> shift);\n> +\t\t} else if constexpr (BitDepth < TargetBitDepth) {\n> +\t\t\tshift = TargetBitDepth - BitDepth;\n> +\t\t\treturn BitDepthValue(value_ << shift);\n> +\t\t} else {\n> +\t\t\treturn BitDepthValue(value_);\n> +\t\t}\n> +\t}\n> +\n> +\tunsigned int value() const\n> +\t{\n> +\t\treturn value_;\n> +\t}\n> +\n> +\ttemplate\n> +\toperator BitDepthValue() const\n> +\t{\n> +\t\treturn convert();\n> +\t}\n> +\n> +\toperator unsigned int() const\n> +\t{\n> +\t\treturn value_;\n> +\t}\n> +\n> +private:\n> +\tunsigned int value_;\n> +};\n> +\n> +inline BitDepthValue<8> operator\"\" _8bit(unsigned long long value)\n> +{\n> +\treturn BitDepthValue<8>(static_cast(value));\n> +}\n> +\n> +inline BitDepthValue<10> operator\"\" _10bit(unsigned long long value)\n> +{\n> +\treturn BitDepthValue<10>(static_cast(value));\n> +}\n> +\n> +inline BitDepthValue<12> operator\"\" _12bit(unsigned long long value)\n> +{\n> +\treturn BitDepthValue<12>(static_cast(value));\n> +}\n> +\n> +inline BitDepthValue<14> operator\"\" _14bit(unsigned long long value)\n> +{\n> +\treturn BitDepthValue<14>(static_cast(value));\n> +}\n> +\n> +inline BitDepthValue<16> operator\"\" _16bit(unsigned long long value)\n> +{\n> +\treturn BitDepthValue<16>(static_cast(value));\n> +}\n\n\nI'm not sure that I like the API, to be honest. To me it seems weird to be declaring a value at a \ngiven bitdepth... I think I'd rather the class store all values at a defined number of bits \n(probably 16, to match CameraSensorHelper) with a helper function to shift it to a different \nBitDepth if needed (and from patch 2 it looks like that would just be soft ISP which wants 8 bits). \nThe operator functions I quite like, and those could perform the shift to 16 bits to instantiate the \ninstance. What do you think?\n\n> diff --git a/test/ipa/libipa/bitdepth.cpp b/test/ipa/libipa/bitdepth.cpp\n> new file mode 100644\n> index 00000000..d854637d\n> --- /dev/null\n> +++ b/test/ipa/libipa/bitdepth.cpp\n> @@ -0,0 +1,107 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2024 Ideas On Board Oy.\n> + *\n> + * BitDepth converter tests\n> + */\n> +\n> +#include \"../../../src/ipa/libipa/bitdepth.h\"\n> +\n> +#include \n> +#include \n> +#include \nProbably don't need map or iostream here\n> +#include \n> +#include \n> +#include \n> +\n> +#include \nOr this one as far as I can tell\n> +\n> +#include \"../../libtest/test.h\"\n> +using namespace std;\n> +\n> +#define ASSERT_EQ(a, b) \\\n> +\tif ((a) != (b)) { \\\n> +\t\tprintf(#a \" != \" #b \"\\n\"); \\\n> +\t\treturn TestFail; \\\n> +\t}\n> +\n> +class BitDepthConverterTest : public Test\n> +{\n> +protected:\n> +\tint run()\n> +\t{\n> +\t\t/* 10-bit to 16-bit conversion */\n> +\t\tBitDepthValue<10> value10bit = 64_10bit;\n> +\t\tBitDepthValue<16> value16bit = value10bit.convert<16>();\n> +\t\tASSERT_EQ(value16bit.value(), 4096);\n> +\n> +\t\t/* Convert implicitly from another BitDepthValue */\n> +\t\tvalue10bit = BitDepthValue<8>(16);\n> +\t\tASSERT_EQ(value10bit.value(), 64);\n> +\t\tvalue10bit = 1_8bit;\n> +\t\tASSERT_EQ(value10bit.value(), 4);\n> +\n> +\t\t/* Read value explicity and implicitly */\n> +\t\tvalue10bit = BitDepthValue<10>(64);\n> +\t\tASSERT_EQ(value10bit.value(), 64);\n> +\t\tASSERT_EQ(value10bit, 64);\n> +\n> +\t\t/* 12-bit to 8-bit conversion */\n> +\t\tBitDepthValue<12> value12bit = 4096_12bit;\n> +\t\tBitDepthValue<8> value8bit = value12bit.convert<8>();\n> +\t\tASSERT_EQ(value8bit.value(), 256);\n> +\n> +\t\t/* Explicit bit depth assignment and conversion */\n> +\t\tvalue16bit = 32768_16bit;\n> +\t\tvalue12bit = value16bit.convert<12>();\n> +\t\tASSERT_EQ(value12bit.value(), 2048);\n> +\n> +\t\t/* Test hex conversion */\n> +\t\tvalue8bit = 0xFF_8bit;\n> +\t\tASSERT_EQ(value8bit, 255);\n> +\n> +\t\t/* Test conversion to same bit depth makes no difference */\n> +\t\tvalue16bit = 255_16bit;\n> +\t\tvalue16bit = value16bit.convert<16>();\n> +\t\tASSERT_EQ(value16bit, 255);\n> +\n> +\t\t/* Implicit bit depth assignment */\n> +\t\tvalue12bit = 10;\n> +\t\tASSERT_EQ(value12bit.value(), 10);\n> +\n> +\t\t/* 8-bit to 12-bit conversion */\n> +\t\tvalue8bit = 128_8bit;\n> +\t\tvalue12bit = value8bit.convert<12>();\n> +\t\tASSERT_EQ(value12bit.value(), 2048);\n> +\n> +\t\t/* 16-bit to 8-bit conversion */\n> +\t\tvalue16bit = 65535_16bit;\n> +\t\tvalue8bit = value16bit.convert<8>();\n> +\t\tASSERT_EQ(value8bit.value(), 255);\n> +\n> +\t\t/* Implicit assignment with int */\n> +\t\tvalue8bit = 200;\n> +\t\tASSERT_EQ(value8bit.value(), 200);\n> +\n> +\t\t/* 8-bit to 16-bit and back again */\n> +\t\tvalue8bit = 150_8bit;\n> +\t\tvalue16bit = value8bit.convert<16>();\n> +\t\tvalue8bit = value16bit.convert<8>();\n> +\t\tASSERT_EQ(value8bit.value(), 150);\n> +\n> +\t\t/* 12-bit to 16-bit and back again */\n> +\t\tvalue12bit = 3000_12bit;\n> +\t\tvalue16bit = value12bit.convert<16>();\n> +\t\tASSERT_EQ(value12bit, 3000);\n> +\t\tASSERT_EQ(value16bit, 48000)\n> +\t\tvalue12bit = value16bit.convert<12>();\n> +\t\tASSERT_EQ(value12bit.value(), 3000);\n> +\n> +\t\t/* Test negatives fail */\n> +\t\t//value12bit = BitDepthValue<-1>;\nThis shouldn't be commented out.\n> +\n> +\t\treturn TestPass;\n> +\t}\n> +};\n> +\n> +TEST_REGISTER(BitDepthConverterTest)\nThe file isn't actually added to meson.build, so this doesn't actually generate a test. If I add it \nthough the tests pass :)","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 144D4BD80A\n\tfor ;\n\tWed, 11 Dec 2024 11:46:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 291AF67EAC;\n\tWed, 11 Dec 2024 12:46:19 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5E3D8618AE\n\tfor ;\n\tWed, 11 Dec 2024 12:46:17 +0100 (CET)","from [192.168.0.43]\n\t(cpc141996-chfd3-2-0-cust928.12-3.cable.virginm.net [86.13.91.161])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 66C34352\n\tfor ;\n\tWed, 11 Dec 2024 12:45:44 +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=\"jXkSsZQX\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1733917544;\n\tbh=b9gcUCFb9k92FQLTctgxcEGcWfnDIQYBEh+wU5//vWA=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=jXkSsZQXv0l9EdnMB9C+YLyhR8PMYSn9eRn/aEqTXTl1DLLldS0KnKEvF8IJLccBw\n\t/rsPH40k/cwvEWamj/d//3kqIaSh/XHT45a1cYFIq/XN/SE+tbeSCtVkkufpf8Vads\n\tL9PJhGaEXP3pKEIykYkCYK6cXypG7bMmv+itKx2g=","Message-ID":"<33dd713b-e910-4932-8028-a540de6183cd@ideasonboard.com>","Date":"Wed, 11 Dec 2024 11:46:14 +0000","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [RFC PATCH 1/2] libcamera: bitdepth: Add BitDepth implementation","To":"libcamera-devel@lists.libcamera.org","References":"<20241127144655.1074720-1-isaac.scott@ideasonboard.com>\n\t<20241127144655.1074720-2-isaac.scott@ideasonboard.com>","Content-Language":"en-US","From":"Dan Scally ","Autocrypt":"addr=dan.scally@ideasonboard.com; keydata=\n\txsFNBGLydlEBEADa5O2s0AbUguprfvXOQun/0a8y2Vk6BqkQALgeD6KnXSWwaoCULp18etYW\n\tB31bfgrdphXQ5kUQibB0ADK8DERB4wrzrUb5CMxLBFE7mQty+v5NsP0OFNK9XTaAOcmD+Ove\n\teIjYvqurAaro91jrRVrS1gBRxIFqyPgNvwwL+alMZhn3/2jU2uvBmuRrgnc/e9cHKiuT3Dtq\n\tMHGPKL2m+plk+7tjMoQFfexoQ1JKugHAjxAhJfrkXh6uS6rc01bYCyo7ybzg53m1HLFJdNGX\n\tsUKR+dQpBs3SY4s66tc1sREJqdYyTsSZf80HjIeJjU/hRunRo4NjRIJwhvnK1GyjOvvuCKVU\n\tRWpY8dNjNu5OeAfdrlvFJOxIE9M8JuYCQTMULqd1NuzbpFMjc9524U3Cngs589T7qUMPb1H1\n\tNTA81LmtJ6Y+IV5/kiTUANflpzBwhu18Ok7kGyCq2a2jsOcVmk8gZNs04gyjuj8JziYwwLbf\n\tvzABwpFVcS8aR+nHIZV1HtOzyw8CsL8OySc3K9y+Y0NRpziMRvutrppzgyMb9V+N31mK9Mxl\n\t1YkgaTl4ciNWpdfUe0yxH03OCuHi3922qhPLF4XX5LN+NaVw5Xz2o3eeWklXdouxwV7QlN33\n\tu4+u2FWzKxDqO6WLQGjxPE0mVB4Gh5Pa1Vb0ct9Ctg0qElvtGQARAQABzShEYW4gU2NhbGx5\n\tIDxkYW4uc2NhbGx5QGlkZWFzb25ib2FyZC5jb20+wsGNBBMBCAA3FiEEsdtt8OWP7+8SNfQe\n\tkiQuh/L+GMQFAmLydlIFCQWjmoACGwMECwkIBwUVCAkKCwUWAgMBAAAKCRCSJC6H8v4YxDI2\n\tEAC2Gz0iyaXJkPInyshrREEWbo0CA6v5KKf3I/HlMPqkZ48bmGoYm4mEQGFWZJAT3K4ir8bg\n\tcEfs9V54gpbrZvdwS4abXbUK4WjKwEs8HK3XJv1WXUN2bsz5oEJWZUImh9gD3naiLLI9QMMm\n\tw/aZkT+NbN5/2KvChRWhdcha7+2Te4foOY66nIM+pw2FZM6zIkInLLUik2zXOhaZtqdeJZQi\n\tHSPU9xu7TRYN4cvdZAnSpG7gQqmLm5/uGZN1/sB3kHTustQtSXKMaIcD/DMNI3JN/t+RJVS7\n\tc0Jh/ThzTmhHyhxx3DRnDIy7kwMI4CFvmhkVC2uNs9kWsj1DuX5kt8513mvfw2OcX9UnNKmZ\n\tnhNCuF6DxVrL8wjOPuIpiEj3V+K7DFF1Cxw1/yrLs8dYdYh8T8vCY2CHBMsqpESROnTazboh\n\tAiQ2xMN1cyXtX11Qwqm5U3sykpLbx2BcmUUUEAKNsM//Zn81QXKG8vOx0ZdMfnzsCaCzt8f6\n\t9dcDBBI3tJ0BI9ByiocqUoL6759LM8qm18x3FYlxvuOs4wSGPfRVaA4yh0pgI+ModVC2Pu3y\n\tejE/IxeatGqJHh6Y+iJzskdi27uFkRixl7YJZvPJAbEn7kzSi98u/5ReEA8Qhc8KO/B7wprj\n\txjNMZNYd0Eth8+WkixHYj752NT5qshKJXcyUU87BTQRi8nZSARAAx0BJayh1Fhwbf4zoY56x\n\txHEpT6DwdTAYAetd3yiKClLVJadYxOpuqyWa1bdfQWPb+h4MeXbWw/53PBgn7gI2EA7ebIRC\n\tPJJhAIkeym7hHZoxqDQTGDJjxFEL11qF+U3rhWiL2Zt0Pl+zFq0eWYYVNiXjsIS4FI2+4m16\n\ttPbDWZFJnSZ828VGtRDQdhXfx3zyVX21lVx1bX4/OZvIET7sVUufkE4hrbqrrufre7wsjD1t\n\t8MQKSapVrr1RltpzPpScdoxknOSBRwOvpp57pJJe5A0L7+WxJ+vQoQXj0j+5tmIWOAV1qBQp\n\thyoyUk9JpPfntk2EKnZHWaApFp5TcL6c5LhUvV7F6XwOjGPuGlZQCWXee9dr7zym8iR3irWT\n\t+49bIh5PMlqSLXJDYbuyFQHFxoiNdVvvf7etvGfqFYVMPVjipqfEQ38ST2nkzx+KBICz7uwj\n\tJwLBdTXzGFKHQNckGMl7F5QdO/35An/QcxBnHVMXqaSd12tkJmoRVWduwuuoFfkTY5mUV3uX\n\txGj3iVCK4V+ezOYA7c2YolfRCNMTza6vcK/P4tDjjsyBBZrCCzhBvd4VVsnnlZhVaIxoky4K\n\taL+AP+zcQrUZmXmgZjXOLryGnsaeoVrIFyrU6ly90s1y3KLoPsDaTBMtnOdwxPmo1xisH8oL\n\ta/VRgpFBfojLPxMAEQEAAcLBfAQYAQgAJhYhBLHbbfDlj+/vEjX0HpIkLofy/hjEBQJi8nZT\n\tBQkFo5qAAhsMAAoJEJIkLofy/hjEXPcQAMIPNqiWiz/HKu9W4QIf1OMUpKn3YkVIj3p3gvfM\n\tRes4fGX94Ji599uLNrPoxKyaytC4R6BTxVriTJjWK8mbo9jZIRM4vkwkZZ2bu98EweSucxbp\n\tvjESsvMXGgxniqV/RQ/3T7LABYRoIUutARYq58p5HwSP0frF0fdFHYdTa2g7MYZl1ur2JzOC\n\tFHRpGadlNzKDE3fEdoMobxHB3Lm6FDml5GyBAA8+dQYVI0oDwJ3gpZPZ0J5Vx9RbqXe8RDuR\n\tdu90hvCJkq7/tzSQ0GeD3BwXb9/R/A4dVXhaDd91Q1qQXidI+2jwhx8iqiYxbT+DoAUkQRQy\n\txBtoCM1CxH7u45URUgD//fxYr3D4B1SlonA6vdaEdHZOGwECnDpTxecENMbz/Bx7qfrmd901\n\tD+N9SjIwrbVhhSyUXYnSUb8F+9g2RDY42Sk7GcYxIeON4VzKqWM7hpkXZ47pkK0YodO+dRKM\n\tyMcoUWrTK0Uz6UzUGKoJVbxmSW/EJLEGoI5p3NWxWtScEVv8mO49gqQdrRIOheZycDmHnItt\n\t9Qjv00uFhEwv2YfiyGk6iGF2W40s2pH2t6oeuGgmiZ7g6d0MEK8Ql/4zPItvr1c1rpwpXUC1\n\tu1kQWgtnNjFHX3KiYdqjcZeRBiry1X0zY+4Y24wUU0KsEewJwjhmCKAsju1RpdlPg2kC","In-Reply-To":"<20241127144655.1074720-2-isaac.scott@ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","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":32923,"web_url":"https://patchwork.libcamera.org/comment/32923/","msgid":"<854j2g3s4h.fsf@mzamazal-thinkpadp1gen3.tpbc.csb>","date":"2025-01-03T14:10:38","subject":"Re: [RFC PATCH 1/2] libcamera: bitdepth: Add BitDepth implementation","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Hi,\n\nDan Scally writes:\n\n> Hi Isaac, thanks for the patches\n>\n> On 27/11/2024 14:46, Isaac Scott wrote:\n>> Add a class template that simplifies bit depth conversion. This\n>> functionality allows users to avoid having to perform inline bitshifting\n>> to convert values of one bit depth to another.\n>>\n>> For example, this makes it easier to input values from datasheets, where\n>> a value may be expressed in a certain bit depth. It also removes the\n>> potential for human errors when making these conversions manually, and\n>> in a lot of cases makes bit depth conversions more readable.\n>>\n>> Signed-off-by: Isaac Scott \n>> ---\n>> src/ipa/libipa/bitdepth.h | 86 ++++++++++++++++++++++++++++\n>> test/ipa/libipa/bitdepth.cpp | 107 +++++++++++++++++++++++++++++++++++\n>> 2 files changed, 193 insertions(+)\n>> create mode 100644 src/ipa/libipa/bitdepth.h\n>> create mode 100644 test/ipa/libipa/bitdepth.cpp\n>>\n>> diff --git a/src/ipa/libipa/bitdepth.h b/src/ipa/libipa/bitdepth.h\n>> new file mode 100644\n>> index 00000000..145ee093\n>> --- /dev/null\n>> +++ b/src/ipa/libipa/bitdepth.h\n>> @@ -0,0 +1,86 @@\n>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>> +/*\n>> + * Copyright (C) 2024 Ideas On Board Oy.\n>> + *\n>> + * BitDepth class to abstract bit shift operations\n>> + */\n>> +\n>> +#pragma once\n>> +\n>> +template\n>> +class BitDepthValue\n>> +{\n>> +public:\n>> +\tstatic_assert(BitDepth > 0, \"Bit depth must be positive\");\n>> +\n>> +\tBitDepthValue()\n>> +\t\t: value_(0) {}\n>> +\n>> +\tBitDepthValue(int value)\n>> +\t\t: value_(value) {}\n>> +\n>> +\tBitDepthValue(unsigned int value)\n>> +\t\t: value_(value) {}\n>> +\n>> +\ttemplate\n>> +\tBitDepthValue convert() const\n>> +\t{\n>> +\t\tstatic_assert(TargetBitDepth > 0, \"Bit depth must be positive\");\n>> +\n>> +\t\tunsigned int shift;\n>> +\n>> +\t\tif constexpr (BitDepth > TargetBitDepth) {\n>> +\t\t\tshift = BitDepth - TargetBitDepth;\n>> +\t\t\treturn BitDepthValue(value_ >> shift);\n>> +\t\t} else if constexpr (BitDepth < TargetBitDepth) {\n>> +\t\t\tshift = TargetBitDepth - BitDepth;\n>> +\t\t\treturn BitDepthValue(value_ << shift);\n>> +\t\t} else {\n>> +\t\t\treturn BitDepthValue(value_);\n>> +\t\t}\n>> +\t}\n>> +\n>> +\tunsigned int value() const\n>> +\t{\n>> +\t\treturn value_;\n>> +\t}\n>> +\n>> +\ttemplate\n>> +\toperator BitDepthValue() const\n>> +\t{\n>> +\t\treturn convert();\n>> +\t}\n>> +\n>> +\toperator unsigned int() const\n>> +\t{\n>> +\t\treturn value_;\n>> +\t}\n>> +\n>> +private:\n>> +\tunsigned int value_;\n>> +};\n>> +\n>> +inline BitDepthValue<8> operator\"\" _8bit(unsigned long long value)\n>> +{\n>> +\treturn BitDepthValue<8>(static_cast(value));\n>> +}\n>> +\n>> +inline BitDepthValue<10> operator\"\" _10bit(unsigned long long value)\n>> +{\n>> +\treturn BitDepthValue<10>(static_cast(value));\n>> +}\n>> +\n>> +inline BitDepthValue<12> operator\"\" _12bit(unsigned long long value)\n>> +{\n>> +\treturn BitDepthValue<12>(static_cast(value));\n>> +}\n>> +\n>> +inline BitDepthValue<14> operator\"\" _14bit(unsigned long long value)\n>> +{\n>> +\treturn BitDepthValue<14>(static_cast(value));\n>> +}\n>> +\n>> +inline BitDepthValue<16> operator\"\" _16bit(unsigned long long value)\n>> +{\n>> +\treturn BitDepthValue<16>(static_cast(value));\n>> +}\n>\n>\n> I'm not sure that I like the API, to be honest. To me it seems weird to be declaring a value at a given\n> bitdepth... I think I'd rather the class store all values at a defined number of bits (probably 16, to match\n> CameraSensorHelper) with a helper function to shift it to a different BitDepth if needed (and from patch 2 it looks\n> like that would just be soft ISP which wants 8 bits). The operator functions I quite like, and those could perform\n> the shift to 16 bits to instantiate the instance. What do you think?\n\nI tend to agree that storing values at a fixed depth would be better.\nMy thoughts about the API:\n\n- BitDepthValue and BitDepthValue are fully interchangeable, there\n is no need for mutual type checking. They are both integers, just\n with a different internal value, which is better not to ever expose\n directly. Which means having a fixed internal value would be simpler\n and with one template less.\n\n- There is apparently no need to save space by having specific internal\n representations.\n\n- The current BitDepthValue::value() looks dangerous, it can easily be\n assigned to an integer of a different depth by mistake, I would remove\n the method. Always using BitDepthValue::convert() is clearer;\n perhaps it should be named BitDepthValue::value().\n\n- If the internal representation was of a fixed depth, it'd be necessary\n to make clear in the constructors what's the bit depth of the provided\n value. Not only to avoid mistakes when passing values to the\n constructor but also to be able to change the internal depth to a\n different one if needed.\n\nIn either case, preventing bit depth confusions by introducing BitDepthValue looks like a good idea to me.\n\n>> diff --git a/test/ipa/libipa/bitdepth.cpp b/test/ipa/libipa/bitdepth.cpp\n>> new file mode 100644\n>> index 00000000..d854637d\n>> --- /dev/null\n>> +++ b/test/ipa/libipa/bitdepth.cpp\n>> @@ -0,0 +1,107 @@\n>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>> +/*\n>> + * Copyright (C) 2024 Ideas On Board Oy.\n>> + *\n>> + * BitDepth converter tests\n>> + */\n>> +\n>> +#include \"../../../src/ipa/libipa/bitdepth.h\"\n>> +\n>> +#include \n>> +#include \n>> +#include \n> Probably don't need map or iostream here\n>> +#include \n>> +#include \n>> +#include \n>> +\n>> +#include \n> Or this one as far as I can tell\n>> +\n>> +#include \"../../libtest/test.h\"\n>> +using namespace std;\n>> +\n>> +#define ASSERT_EQ(a, b) \\\n>> +\tif ((a) != (b)) { \\\n>> +\t\tprintf(#a \" != \" #b \"\\n\"); \\\n>> +\t\treturn TestFail; \\\n>> +\t}\n>> +\n>> +class BitDepthConverterTest : public Test\n>> +{\n>> +protected:\n>> +\tint run()\n>> +\t{\n>> +\t\t/* 10-bit to 16-bit conversion */\n>> +\t\tBitDepthValue<10> value10bit = 64_10bit;\n>> +\t\tBitDepthValue<16> value16bit = value10bit.convert<16>();\n>> +\t\tASSERT_EQ(value16bit.value(), 4096);\n>> +\n>> +\t\t/* Convert implicitly from another BitDepthValue */\n>> +\t\tvalue10bit = BitDepthValue<8>(16);\n>> +\t\tASSERT_EQ(value10bit.value(), 64);\n>> +\t\tvalue10bit = 1_8bit;\n>> +\t\tASSERT_EQ(value10bit.value(), 4);\n>> +\n>> +\t\t/* Read value explicity and implicitly */\n>> +\t\tvalue10bit = BitDepthValue<10>(64);\n>> +\t\tASSERT_EQ(value10bit.value(), 64);\n>> +\t\tASSERT_EQ(value10bit, 64);\n>> +\n>> +\t\t/* 12-bit to 8-bit conversion */\n>> +\t\tBitDepthValue<12> value12bit = 4096_12bit;\n>> +\t\tBitDepthValue<8> value8bit = value12bit.convert<8>();\n>> +\t\tASSERT_EQ(value8bit.value(), 256);\n>> +\n>> +\t\t/* Explicit bit depth assignment and conversion */\n>> +\t\tvalue16bit = 32768_16bit;\n>> +\t\tvalue12bit = value16bit.convert<12>();\n>> +\t\tASSERT_EQ(value12bit.value(), 2048);\n>> +\n>> +\t\t/* Test hex conversion */\n>> +\t\tvalue8bit = 0xFF_8bit;\n>> +\t\tASSERT_EQ(value8bit, 255);\n>> +\n>> +\t\t/* Test conversion to same bit depth makes no difference */\n>> +\t\tvalue16bit = 255_16bit;\n>> +\t\tvalue16bit = value16bit.convert<16>();\n>> +\t\tASSERT_EQ(value16bit, 255);\n>> +\n>> +\t\t/* Implicit bit depth assignment */\n>> +\t\tvalue12bit = 10;\n>> +\t\tASSERT_EQ(value12bit.value(), 10);\n>> +\n>> +\t\t/* 8-bit to 12-bit conversion */\n>> +\t\tvalue8bit = 128_8bit;\n>> +\t\tvalue12bit = value8bit.convert<12>();\n>> +\t\tASSERT_EQ(value12bit.value(), 2048);\n>> +\n>> +\t\t/* 16-bit to 8-bit conversion */\n>> +\t\tvalue16bit = 65535_16bit;\n>> +\t\tvalue8bit = value16bit.convert<8>();\n>> +\t\tASSERT_EQ(value8bit.value(), 255);\n>> +\n>> +\t\t/* Implicit assignment with int */\n>> +\t\tvalue8bit = 200;\n>> +\t\tASSERT_EQ(value8bit.value(), 200);\n>> +\n>> +\t\t/* 8-bit to 16-bit and back again */\n>> +\t\tvalue8bit = 150_8bit;\n>> +\t\tvalue16bit = value8bit.convert<16>();\n>> +\t\tvalue8bit = value16bit.convert<8>();\n>> +\t\tASSERT_EQ(value8bit.value(), 150);\n>> +\n>> +\t\t/* 12-bit to 16-bit and back again */\n>> +\t\tvalue12bit = 3000_12bit;\n>> +\t\tvalue16bit = value12bit.convert<16>();\n>> +\t\tASSERT_EQ(value12bit, 3000);\n>> +\t\tASSERT_EQ(value16bit, 48000)\n>> +\t\tvalue12bit = value16bit.convert<12>();\n>> +\t\tASSERT_EQ(value12bit.value(), 3000);\n>> +\n>> +\t\t/* Test negatives fail */\n>> +\t\t//value12bit = BitDepthValue<-1>;\n> This shouldn't be commented out.\n>> +\n>> +\t\treturn TestPass;\n>> +\t}\n>> +};\n>> +\n>> +TEST_REGISTER(BitDepthConverterTest)\n> The file isn't actually added to meson.build, so this doesn't actually generate a test. If I add it though the\n> tests pass :)","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 D78ABC324C\n\tfor ;\n\tFri, 3 Jan 2025 14:10:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E7A9F684E1;\n\tFri, 3 Jan 2025 15:10:47 +0100 (CET)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.133.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BABE1684DF\n\tfor ;\n\tFri, 3 Jan 2025 15:10:45 +0100 (CET)","from mail-wr1-f69.google.com (mail-wr1-f69.google.com\n\t[209.85.221.69]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-658-442Gu_sNORqxwV0SmbYhcg-1; Fri, 03 Jan 2025 09:10:43 -0500","by mail-wr1-f69.google.com with SMTP id\n\tffacd0b85a97d-38a684a096eso335840f8f.2\n\tfor ;\n\tFri, 03 Jan 2025 06:10:42 -0800 (PST)","from mzamazal-thinkpadp1gen3.tpbc.csb\n\t(ip-77-48-47-2.net.vodafone.cz. [77.48.47.2])\n\tby smtp.gmail.com with ESMTPSA id\n\t5b1f17b1804b1-43661200ae5sm483546415e9.17.2025.01.03.06.10.39\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tFri, 03 Jan 2025 06:10:39 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"QXHj1JOg\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1735913444;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=Vcq58HeaHfInqzN0nvle1pN44kZf77egnjM2RuFpB30=;\n\tb=QXHj1JOgYccF1vGOFV49+zPirLohCh7CcsG8kBCMPc1DbWFFivuiy1rC5bS1NEq6LR7ign\n\tjNM6K+RKseRVPeEnnYkRPpRUAzglkA/HnYhsiJhfHHYhoiae4V5CmJzdEImCT6Y4VRQi0m\n\tDWHKEtoPet6jbagrofu7a/P/w9wXevY=","X-MC-Unique":"442Gu_sNORqxwV0SmbYhcg-1","X-Mimecast-MFC-AGG-ID":"442Gu_sNORqxwV0SmbYhcg","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1735913441; x=1736518241;\n\th=mime-version:user-agent:message-id:date:references:in-reply-to\n\t:subject:cc:to:from:x-gm-message-state:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=Vcq58HeaHfInqzN0nvle1pN44kZf77egnjM2RuFpB30=;\n\tb=VdlkFZigYMDBeG8XnnX6sweE+xXBlxBQPcLVe+P2InE5771u/NYtKI+NfmWsXFANoR\n\tDIiDkH2OVHREUsA8PajTY5MjPy4se6LyWjg0nF8/QOk3JHS+uMGCOz0YDTXQnbImtMzI\n\tg1WkxTRHAp7N6zpifktTiKPhyGYhZqyIfXnOrer2NWqGyTaslu+cHI9NdVVvuVz+BDIi\n\tZOSHPAZzoyex0vs9oZUhK5GN5IVdwnp1lQ/xqC/qw8rI49rzXD5h94CUuRdh012LUsLI\n\teDOCahSsT7dACJGDLaEG8zTcX8BHNlZY71XtmfOfzKEFDbvmRsv5nQ/Kh1NFePZBc4QI\n\tYtVw==","X-Forwarded-Encrypted":"i=1;\n\tAJvYcCWkRL7NIfck0fALGwaV55wait/Dmhj/EJ+N7Y3G83r9hruxiKO9plik6UtYKSaLyar34mdTeto4k4hHvx6xHqs=@lists.libcamera.org","X-Gm-Message-State":"AOJu0YyrWgesJktEq5bVnewj70B6HwhQjm9uwdg8IkhizwgntFocaA88\n\tzbSXkaUdiNlfR86mJhOjPrFsQ/qFo/rr/0VQtAQsNmVR433M8KOOT3ok2BVLwrwAX9Ha1oHcNQO\n\tdsPvDigjWmDrSgzMWG91MwnQ9I62S9hb0x2J2P9xol/gbEEaKMjbnThlM2a6NRMLJ7ILiGZOpE9\n\t+UZRBAaf9FQ6MgCKpnp5eANm3IzjntjPWl9BAtk33w61ghT96QhFCqi6I=","X-Gm-Gg":"ASbGncu7yvtH6sYYbNE0qvZitp9qfRHNcgMIwbVxYXze+uWbj5thNqy5mXWg9gkBjnH\n\tSuCeekIyYXs1/nmbh02a8CDZEzCt+kGyHTu3srQ62xoFCCnvbrEoiHuqhCHERGClrE+aasdY3Sm\n\tEE7HekFBKfSn7UJvvDTP8o80Li07s8UFN2NHd1rd1KPW2Ta6yyiYbgZDerqvP34/99tNf5eM5fU\n\tLPavCWOEgG6O4ApCLabpClhWNP5C7+ES+1Go1qja+Cs+aFEcM3hdhWTGLV3SO0cXdrZiBI2iwtg\n\tQ0h7F8U7hUw0qcJexyOgbyJ2/VqB2OLFUA==","X-Received":["by 2002:a05:6000:1a8d:b0:385:e22e:288b with SMTP id\n\tffacd0b85a97d-38a2240a0c9mr44292600f8f.59.1735913441466; \n\tFri, 03 Jan 2025 06:10:41 -0800 (PST)","by 2002:a05:6000:1a8d:b0:385:e22e:288b with SMTP id\n\tffacd0b85a97d-38a2240a0c9mr44292561f8f.59.1735913440951; \n\tFri, 03 Jan 2025 06:10:40 -0800 (PST)"],"X-Google-Smtp-Source":"AGHT+IHdggUEtPty37/42UJfUW7v2KH8TmzZ0p+O7j0zD5/lMQ6RBa24U1faz0bx7697c9LU/vso7w==","From":"Milan Zamazal ","To":"Dan Scally ","Cc":"Isaac Scott ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [RFC PATCH 1/2] libcamera: bitdepth: Add BitDepth implementation","In-Reply-To":"<33dd713b-e910-4932-8028-a540de6183cd@ideasonboard.com> (Dan\n\tScally's message of \"Wed, 11 Dec 2024 11:46:14 +0000\")","References":"<20241127144655.1074720-1-isaac.scott@ideasonboard.com>\n\t<20241127144655.1074720-2-isaac.scott@ideasonboard.com>\n\t<33dd713b-e910-4932-8028-a540de6183cd@ideasonboard.com>","Date":"Fri, 03 Jan 2025 15:10:38 +0100","Message-ID":"<854j2g3s4h.fsf@mzamazal-thinkpadp1gen3.tpbc.csb>","User-Agent":"Gnus/5.13 (Gnus v5.13)","MIME-Version":"1.0","X-Mimecast-Spam-Score":"0","X-Mimecast-MFC-PROC-ID":"2rSSnROeR_f4ReJrO9rkfV4ryNWgN1gST2WiS9zl8JE_1735913442","X-Mimecast-Originator":"redhat.com","Content-Type":"text/plain","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\" "}}]