[RFC,1/2] libcamera: bitdepth: Add BitDepth implementation

22125 diff mbox series

Add a class template that simplifies bit depth conversion. This
functionality allows users to avoid having to perform inline bitshifting
to convert values of one bit depth to another.

For example, this makes it easier to input values from datasheets, where
a value may be expressed in a certain bit depth. It also removes the
potential for human errors when making these conversions manually, and
in a lot of cases makes bit depth conversions more readable.

Signed-off-by: Isaac Scott <isaac.scott@ideasonboard.com>
---
 src/ipa/libipa/bitdepth.h    |  86 ++++++++++++++++++++++++++++
 test/ipa/libipa/bitdepth.cpp | 107 +++++++++++++++++++++++++++++++++++
 2 files changed, 193 insertions(+)
 create mode 100644 src/ipa/libipa/bitdepth.h
 create mode 100644 test/ipa/libipa/bitdepth.cpp

Hi Isaac, thanks for the patches

On 27/11/2024 14:46, Isaac Scott wrote:
> Add a class template that simplifies bit depth conversion. This
> functionality allows users to avoid having to perform inline bitshifting
> to convert values of one bit depth to another.
>
> For example, this makes it easier to input values from datasheets, where
> a value may be expressed in a certain bit depth. It also removes the
> potential for human errors when making these conversions manually, and
> in a lot of cases makes bit depth conversions more readable.
>
> Signed-off-by: Isaac Scott <isaac.scott@ideasonboard.com>
> ---
>   src/ipa/libipa/bitdepth.h    |  86 ++++++++++++++++++++++++++++
>   test/ipa/libipa/bitdepth.cpp | 107 +++++++++++++++++++++++++++++++++++
>   2 files changed, 193 insertions(+)
>   create mode 100644 src/ipa/libipa/bitdepth.h
>   create mode 100644 test/ipa/libipa/bitdepth.cpp
>
> diff --git a/src/ipa/libipa/bitdepth.h b/src/ipa/libipa/bitdepth.h
> new file mode 100644
> index 00000000..145ee093
> --- /dev/null
> +++ b/src/ipa/libipa/bitdepth.h
> @@ -0,0 +1,86 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2024 Ideas On Board Oy.
> + *
> + * BitDepth class to abstract bit shift operations
> + */
> +
> +#pragma once
> +
> +template<unsigned int BitDepth>
> +class BitDepthValue
> +{
> +public:
> +	static_assert(BitDepth > 0, "Bit depth must be positive");
> +
> +	BitDepthValue()
> +		: value_(0) {}
> +
> +	BitDepthValue(int value)
> +		: value_(value) {}
> +
> +	BitDepthValue(unsigned int value)
> +		: value_(value) {}
> +
> +	template<unsigned int TargetBitDepth>
> +	BitDepthValue<TargetBitDepth> convert() const
> +	{
> +		static_assert(TargetBitDepth > 0, "Bit depth must be positive");
> +
> +		unsigned int shift;
> +
> +		if constexpr (BitDepth > TargetBitDepth) {
> +			shift = BitDepth - TargetBitDepth;
> +			return BitDepthValue<TargetBitDepth>(value_ >> shift);
> +		} else if constexpr (BitDepth < TargetBitDepth) {
> +			shift = TargetBitDepth - BitDepth;
> +			return BitDepthValue<TargetBitDepth>(value_ << shift);
> +		} else {
> +			return BitDepthValue<TargetBitDepth>(value_);
> +		}
> +	}
> +
> +	unsigned int value() const
> +	{
> +		return value_;
> +	}
> +
> +	template<unsigned int TargetBitDepth>
> +	operator BitDepthValue<TargetBitDepth>() const
> +	{
> +		return convert<TargetBitDepth>();
> +	}
> +
> +	operator unsigned int() const
> +	{
> +		return value_;
> +	}
> +
> +private:
> +	unsigned int value_;
> +};
> +
> +inline BitDepthValue<8> operator"" _8bit(unsigned long long value)
> +{
> +	return BitDepthValue<8>(static_cast<unsigned int>(value));
> +}
> +
> +inline BitDepthValue<10> operator"" _10bit(unsigned long long value)
> +{
> +	return BitDepthValue<10>(static_cast<unsigned int>(value));
> +}
> +
> +inline BitDepthValue<12> operator"" _12bit(unsigned long long value)
> +{
> +	return BitDepthValue<12>(static_cast<unsigned int>(value));
> +}
> +
> +inline BitDepthValue<14> operator"" _14bit(unsigned long long value)
> +{
> +	return BitDepthValue<14>(static_cast<unsigned int>(value));
> +}
> +
> +inline BitDepthValue<16> operator"" _16bit(unsigned long long value)
> +{
> +	return BitDepthValue<16>(static_cast<unsigned int>(value));
> +}


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 bitdepth... I think I'd rather the class store all values at a defined number of bits 
(probably 16, to match CameraSensorHelper) with a helper function to shift it to a different 
BitDepth if needed (and from patch 2 it looks like that would just be soft ISP which wants 8 bits). 
The operator functions I quite like, and those could perform the shift to 16 bits to instantiate the 
instance. What do you think?

> diff --git a/test/ipa/libipa/bitdepth.cpp b/test/ipa/libipa/bitdepth.cpp
> new file mode 100644
> index 00000000..d854637d
> --- /dev/null
> +++ b/test/ipa/libipa/bitdepth.cpp
> @@ -0,0 +1,107 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2024 Ideas On Board Oy.
> + *
> + * BitDepth converter tests
> + */
> +
> +#include "../../../src/ipa/libipa/bitdepth.h"
> +
> +#include <cmath>
> +#include <iostream>
> +#include <map>
Probably don't need map or iostream here
> +#include <stdint.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +
> +#include <libcamera/base/log.h>
Or this one as far as I can tell
> +
> +#include "../../libtest/test.h"
> +using namespace std;
> +
> +#define ASSERT_EQ(a, b)                    \
> +	if ((a) != (b)) {                  \
> +		printf(#a " != " #b "\n"); \
> +		return TestFail;           \
> +	}
> +
> +class BitDepthConverterTest : public Test
> +{
> +protected:
> +	int run()
> +	{
> +		/* 10-bit to 16-bit conversion */
> +		BitDepthValue<10> value10bit = 64_10bit;
> +		BitDepthValue<16> value16bit = value10bit.convert<16>();
> +		ASSERT_EQ(value16bit.value(), 4096);
> +
> +		/* Convert implicitly from another BitDepthValue */
> +		value10bit = BitDepthValue<8>(16);
> +		ASSERT_EQ(value10bit.value(), 64);
> +		value10bit = 1_8bit;
> +		ASSERT_EQ(value10bit.value(), 4);
> +
> +		/* Read value explicity and implicitly */
> +		value10bit = BitDepthValue<10>(64);
> +		ASSERT_EQ(value10bit.value(), 64);
> +		ASSERT_EQ(value10bit, 64);
> +
> +		/* 12-bit to 8-bit conversion */
> +		BitDepthValue<12> value12bit = 4096_12bit;
> +		BitDepthValue<8> value8bit = value12bit.convert<8>();
> +		ASSERT_EQ(value8bit.value(), 256);
> +
> +		/* Explicit bit depth assignment and conversion */
> +		value16bit = 32768_16bit;
> +		value12bit = value16bit.convert<12>();
> +		ASSERT_EQ(value12bit.value(), 2048);
> +
> +		/* Test hex conversion */
> +		value8bit = 0xFF_8bit;
> +		ASSERT_EQ(value8bit, 255);
> +
> +		/* Test conversion to same bit depth makes no difference */
> +		value16bit = 255_16bit;
> +		value16bit = value16bit.convert<16>();
> +		ASSERT_EQ(value16bit, 255);
> +
> +		/* Implicit bit depth assignment */
> +		value12bit = 10;
> +		ASSERT_EQ(value12bit.value(), 10);
> +
> +		/* 8-bit to 12-bit conversion */
> +		value8bit = 128_8bit;
> +		value12bit = value8bit.convert<12>();
> +		ASSERT_EQ(value12bit.value(), 2048);
> +
> +		/* 16-bit to 8-bit conversion */
> +		value16bit = 65535_16bit;
> +		value8bit = value16bit.convert<8>();
> +		ASSERT_EQ(value8bit.value(), 255);
> +
> +		/* Implicit assignment with int */
> +		value8bit = 200;
> +		ASSERT_EQ(value8bit.value(), 200);
> +
> +		/* 8-bit to 16-bit and back again */
> +		value8bit = 150_8bit;
> +		value16bit = value8bit.convert<16>();
> +		value8bit = value16bit.convert<8>();
> +		ASSERT_EQ(value8bit.value(), 150);
> +
> +		/* 12-bit to 16-bit and back again */
> +		value12bit = 3000_12bit;
> +		value16bit = value12bit.convert<16>();
> +		ASSERT_EQ(value12bit, 3000);
> +		ASSERT_EQ(value16bit, 48000)
> +		value12bit = value16bit.convert<12>();
> +		ASSERT_EQ(value12bit.value(), 3000);
> +
> +		/* Test negatives fail */
> +		//value12bit = BitDepthValue<-1>;
This shouldn't be commented out.
> +
> +		return TestPass;
> +	}
> +};
> +
> +TEST_REGISTER(BitDepthConverterTest)
The file isn't actually added to meson.build, so this doesn't actually generate a test. If I add it 
though the tests pass :)

Hi,

Dan Scally <dan.scally@ideasonboard.com> writes:

> Hi Isaac, thanks for the patches
>
> On 27/11/2024 14:46, Isaac Scott wrote:
>> Add a class template that simplifies bit depth conversion. This
>> functionality allows users to avoid having to perform inline bitshifting
>> to convert values of one bit depth to another.
>>
>> For example, this makes it easier to input values from datasheets, where
>> a value may be expressed in a certain bit depth. It also removes the
>> potential for human errors when making these conversions manually, and
>> in a lot of cases makes bit depth conversions more readable.
>>
>> Signed-off-by: Isaac Scott <isaac.scott@ideasonboard.com>
>> ---
>>   src/ipa/libipa/bitdepth.h    |  86 ++++++++++++++++++++++++++++
>>   test/ipa/libipa/bitdepth.cpp | 107 +++++++++++++++++++++++++++++++++++
>>   2 files changed, 193 insertions(+)
>>   create mode 100644 src/ipa/libipa/bitdepth.h
>>   create mode 100644 test/ipa/libipa/bitdepth.cpp
>>
>> diff --git a/src/ipa/libipa/bitdepth.h b/src/ipa/libipa/bitdepth.h
>> new file mode 100644
>> index 00000000..145ee093
>> --- /dev/null
>> +++ b/src/ipa/libipa/bitdepth.h
>> @@ -0,0 +1,86 @@
>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>> +/*
>> + * Copyright (C) 2024 Ideas On Board Oy.
>> + *
>> + * BitDepth class to abstract bit shift operations
>> + */
>> +
>> +#pragma once
>> +
>> +template<unsigned int BitDepth>
>> +class BitDepthValue
>> +{
>> +public:
>> +	static_assert(BitDepth > 0, "Bit depth must be positive");
>> +
>> +	BitDepthValue()
>> +		: value_(0) {}
>> +
>> +	BitDepthValue(int value)
>> +		: value_(value) {}
>> +
>> +	BitDepthValue(unsigned int value)
>> +		: value_(value) {}
>> +
>> +	template<unsigned int TargetBitDepth>
>> +	BitDepthValue<TargetBitDepth> convert() const
>> +	{
>> +		static_assert(TargetBitDepth > 0, "Bit depth must be positive");
>> +
>> +		unsigned int shift;
>> +
>> +		if constexpr (BitDepth > TargetBitDepth) {
>> +			shift = BitDepth - TargetBitDepth;
>> +			return BitDepthValue<TargetBitDepth>(value_ >> shift);
>> +		} else if constexpr (BitDepth < TargetBitDepth) {
>> +			shift = TargetBitDepth - BitDepth;
>> +			return BitDepthValue<TargetBitDepth>(value_ << shift);
>> +		} else {
>> +			return BitDepthValue<TargetBitDepth>(value_);
>> +		}
>> +	}
>> +
>> +	unsigned int value() const
>> +	{
>> +		return value_;
>> +	}
>> +
>> +	template<unsigned int TargetBitDepth>
>> +	operator BitDepthValue<TargetBitDepth>() const
>> +	{
>> +		return convert<TargetBitDepth>();
>> +	}
>> +
>> +	operator unsigned int() const
>> +	{
>> +		return value_;
>> +	}
>> +
>> +private:
>> +	unsigned int value_;
>> +};
>> +
>> +inline BitDepthValue<8> operator"" _8bit(unsigned long long value)
>> +{
>> +	return BitDepthValue<8>(static_cast<unsigned int>(value));
>> +}
>> +
>> +inline BitDepthValue<10> operator"" _10bit(unsigned long long value)
>> +{
>> +	return BitDepthValue<10>(static_cast<unsigned int>(value));
>> +}
>> +
>> +inline BitDepthValue<12> operator"" _12bit(unsigned long long value)
>> +{
>> +	return BitDepthValue<12>(static_cast<unsigned int>(value));
>> +}
>> +
>> +inline BitDepthValue<14> operator"" _14bit(unsigned long long value)
>> +{
>> +	return BitDepthValue<14>(static_cast<unsigned int>(value));
>> +}
>> +
>> +inline BitDepthValue<16> operator"" _16bit(unsigned long long value)
>> +{
>> +	return BitDepthValue<16>(static_cast<unsigned int>(value));
>> +}
>
>
> 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
> bitdepth... I think I'd rather the class store all values at a defined number of bits (probably 16, to match
> CameraSensorHelper) with a helper function to shift it to a different BitDepth if needed (and from patch 2 it looks
> like that would just be soft ISP which wants 8 bits). The operator functions I quite like, and those could perform
> the shift to 16 bits to instantiate the instance. What do you think?

I tend to agree that storing values at a fixed depth would be better.
My thoughts about the API:

- BitDepthValue<M> and BitDepthValue<N> are fully interchangeable, there
  is no need for mutual type checking.  They are both integers, just
  with a different internal value, which is better not to ever expose
  directly.  Which means having a fixed internal value would be simpler
  and with one template less.

- There is apparently no need to save space by having specific internal
  representations.

- The current BitDepthValue::value() looks dangerous, it can easily be
  assigned to an integer of a different depth by mistake, I would remove
  the method.  Always using BitDepthValue::convert<N>() is clearer;
  perhaps it should be named BitDepthValue::value<N>().

- If the internal representation was of a fixed depth, it'd be necessary
  to make clear in the constructors what's the bit depth of the provided
  value.  Not only to avoid mistakes when passing values to the
  constructor but also to be able to change the internal depth to a
  different one if needed.

In either case, preventing bit depth confusions by introducing BitDepthValue looks like a good idea to me.

>> diff --git a/test/ipa/libipa/bitdepth.cpp b/test/ipa/libipa/bitdepth.cpp
>> new file mode 100644
>> index 00000000..d854637d
>> --- /dev/null
>> +++ b/test/ipa/libipa/bitdepth.cpp
>> @@ -0,0 +1,107 @@
>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>> +/*
>> + * Copyright (C) 2024 Ideas On Board Oy.
>> + *
>> + * BitDepth converter tests
>> + */
>> +
>> +#include "../../../src/ipa/libipa/bitdepth.h"
>> +
>> +#include <cmath>
>> +#include <iostream>
>> +#include <map>
> Probably don't need map or iostream here
>> +#include <stdint.h>
>> +#include <stdio.h>
>> +#include <stdlib.h>
>> +
>> +#include <libcamera/base/log.h>
> Or this one as far as I can tell
>> +
>> +#include "../../libtest/test.h"
>> +using namespace std;
>> +
>> +#define ASSERT_EQ(a, b)                    \
>> +	if ((a) != (b)) {                  \
>> +		printf(#a " != " #b "\n"); \
>> +		return TestFail;           \
>> +	}
>> +
>> +class BitDepthConverterTest : public Test
>> +{
>> +protected:
>> +	int run()
>> +	{
>> +		/* 10-bit to 16-bit conversion */
>> +		BitDepthValue<10> value10bit = 64_10bit;
>> +		BitDepthValue<16> value16bit = value10bit.convert<16>();
>> +		ASSERT_EQ(value16bit.value(), 4096);
>> +
>> +		/* Convert implicitly from another BitDepthValue */
>> +		value10bit = BitDepthValue<8>(16);
>> +		ASSERT_EQ(value10bit.value(), 64);
>> +		value10bit = 1_8bit;
>> +		ASSERT_EQ(value10bit.value(), 4);
>> +
>> +		/* Read value explicity and implicitly */
>> +		value10bit = BitDepthValue<10>(64);
>> +		ASSERT_EQ(value10bit.value(), 64);
>> +		ASSERT_EQ(value10bit, 64);
>> +
>> +		/* 12-bit to 8-bit conversion */
>> +		BitDepthValue<12> value12bit = 4096_12bit;
>> +		BitDepthValue<8> value8bit = value12bit.convert<8>();
>> +		ASSERT_EQ(value8bit.value(), 256);
>> +
>> +		/* Explicit bit depth assignment and conversion */
>> +		value16bit = 32768_16bit;
>> +		value12bit = value16bit.convert<12>();
>> +		ASSERT_EQ(value12bit.value(), 2048);
>> +
>> +		/* Test hex conversion */
>> +		value8bit = 0xFF_8bit;
>> +		ASSERT_EQ(value8bit, 255);
>> +
>> +		/* Test conversion to same bit depth makes no difference */
>> +		value16bit = 255_16bit;
>> +		value16bit = value16bit.convert<16>();
>> +		ASSERT_EQ(value16bit, 255);
>> +
>> +		/* Implicit bit depth assignment */
>> +		value12bit = 10;
>> +		ASSERT_EQ(value12bit.value(), 10);
>> +
>> +		/* 8-bit to 12-bit conversion */
>> +		value8bit = 128_8bit;
>> +		value12bit = value8bit.convert<12>();
>> +		ASSERT_EQ(value12bit.value(), 2048);
>> +
>> +		/* 16-bit to 8-bit conversion */
>> +		value16bit = 65535_16bit;
>> +		value8bit = value16bit.convert<8>();
>> +		ASSERT_EQ(value8bit.value(), 255);
>> +
>> +		/* Implicit assignment with int */
>> +		value8bit = 200;
>> +		ASSERT_EQ(value8bit.value(), 200);
>> +
>> +		/* 8-bit to 16-bit and back again */
>> +		value8bit = 150_8bit;
>> +		value16bit = value8bit.convert<16>();
>> +		value8bit = value16bit.convert<8>();
>> +		ASSERT_EQ(value8bit.value(), 150);
>> +
>> +		/* 12-bit to 16-bit and back again */
>> +		value12bit = 3000_12bit;
>> +		value16bit = value12bit.convert<16>();
>> +		ASSERT_EQ(value12bit, 3000);
>> +		ASSERT_EQ(value16bit, 48000)
>> +		value12bit = value16bit.convert<12>();
>> +		ASSERT_EQ(value12bit.value(), 3000);
>> +
>> +		/* Test negatives fail */
>> +		//value12bit = BitDepthValue<-1>;
> This shouldn't be commented out.
>> +
>> +		return TestPass;
>> +	}
>> +};
>> +
>> +TEST_REGISTER(BitDepthConverterTest)
> The file isn't actually added to meson.build, so this doesn't actually generate a test. If I add it though the
> tests pass :)