[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 :)