[RFC PATCH 1/2] libcamera: bitdepth: Add BitDepth implementation
Dan Scally
dan.scally at ideasonboard.com
Wed Dec 11 12:46:14 CET 2024
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 at 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 :)
More information about the libcamera-devel
mailing list