[libcamera-devel] [PATCH v2 4/4] test: Add unit tests for the BayerFormat class

[Sebastian Fricke]

On 08.01.2021 12:46, Jacopo Mondi wrote:
>Hi Sebastian,

Hey Jacopo,

Thank you for your review.

>
>On Thu, Dec 31, 2020 at 04:53:36PM +0100, Sebastian Fricke wrote:
>> Test all of the present methods including the newly implemented
>> `fromV4L2PixelFormat`, as well as the new operators `==/!=`.
>>
>> Signed-off-by: Sebastian Fricke <sebastian.fricke.linux at gmail.com>
>> ---
>>  test/bayer_format.cpp | 202 ++++++++++++++++++++++++++++++++++++++++++
>>  test/meson.build      |   1 +
>>  2 files changed, 203 insertions(+)
>>  create mode 100644 test/bayer_format.cpp
>>
>> diff --git a/test/bayer_format.cpp b/test/bayer_format.cpp
>> new file mode 100644
>> index 00000000..dd7aa8cb
>> --- /dev/null
>> +++ b/test/bayer_format.cpp
>> @@ -0,0 +1,202 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +/*
>> + * Copyright (C) 2020, Sebastian Fricke
>> + *
>> + * bayer_format.cpp - BayerFormat class tests
>> + */
>> +
>> +#include <iostream>
>> +
>> +#include <libcamera/internal/bayer_format.h>
>> +#include <libcamera/transform.h>
>> +
>> +#include "test.h"
>> +
>> +using namespace std;
>> +using namespace libcamera;
>> +
>> +class BayerFormatTest : public Test
>> +{
>> +protected:
>> +	int run()
>> +	{
>> +		/*
>> +		 * An empty Bayer format has to be invalid.
>> +		 */
>
>Fits on 1 line (also other comments)
>
>> +		BayerFormat bayerFmt = BayerFormat();
>
>Just
>                BayerFormat bayerFmt;
>?
>
>> +		if (bayerFmt.isValid()) {
>> +			cout << "An empty BayerFormat "
>
>We have a mixed usage of cout/cerr in test/
>But in new tests I would use cerr.

Fixed in my next version.

>
>> +			     << "has to be invalid." << endl;
>> +			return TestFail;
>> +		}
>> +
>> +		/*
>> +		 * A correct Bayer format has to be valid.
>> +		 */
>> +		bayerFmt = BayerFormat(BayerFormat::BGGR, 8, BayerFormat::None);
>> +		if (!bayerFmt.isValid()) {
>> +			cout << "A correct BayerFormat "
>> +			     << "has to be valid." << endl;
>> +			return TestFail;
>> +		}
>> +
>> +		/*
>> +		 * Two bayer formats created with the same order and bit depth
>> +		 * have to be equal.
>> +		 */
>> +		bayerFmt = BayerFormat(BayerFormat::BGGR, 8, BayerFormat::None);
>> +		BayerFormat bayerFmtExpect = BayerFormat(BayerFormat::BGGR, 8,
>> +							 BayerFormat::None);
>> +		if (bayerFmt != bayerFmtExpect) {
>> +			cout << "Two BayerFormat object created with the same "
>> +			     << "order and bitDepth must be equal." << endl;
>> +			return TestFail;
>> +		}
>> +
>> +		/*
>> +		 * Two Bayer formats created with the same order but with a
>> +		 * different bitDepth are not equal.
>> +		 */
>> +		bayerFmt = BayerFormat(BayerFormat::BGGR, 8, BayerFormat::None);
>> +		bayerFmtExpect = BayerFormat(BayerFormat::BGGR, 12,
>> +					     BayerFormat::None);
>> +		if (bayerFmt == bayerFmtExpect) {
>> +			cout << "Two BayerFormat object created with the same "
>> +			     << "order, but different bit depths are not equal."
>> +			     << endl;
>> +			return TestFail;
>> +		}
>> +
>> +		/*
>> +		 * Create a Bayer format with a V4L2PixelFormat and check if we
>> +		 * get the same format after converting back to the V4L2 Format.
>> +		 */
>> +		V4L2PixelFormat v4l2FmtExpect = V4L2PixelFormat(
>> +			V4L2_PIX_FMT_SBGGR8);
>> +		bayerFmt = BayerFormat::fromV4L2PixelFormat(v4l2FmtExpect);
>> +		V4L2PixelFormat v4l2Fmt = bayerFmt.toV4L2PixelFormat();
>> +		if (v4l2Fmt != v4l2FmtExpect) {
>> +			cout << "Expected: '" << v4l2FmtExpect.toString()
>> +			     << "' got: '" << v4l2Fmt.toString() << "'" << endl;
>> +			return TestFail;
>> +		}
>> +
>> +		/*
>> +		 * Use a Bayer format that is not found in the mapping table
>> +		 * and verify that no matching V4L2PixelFormat is found.
>> +		 */
>> +		v4l2FmtExpect = V4L2PixelFormat();
>> +		bayerFmt = BayerFormat(BayerFormat::BGGR, 24,
>> +				       BayerFormat::None);
>> +		v4l2Fmt = bayerFmt.toV4L2PixelFormat();
>> +		if (v4l2Fmt != v4l2FmtExpect) {
>> +			cout << "Expected: empty V4L2PixelFormat got: '"
>> +			     << v4l2Fmt.toString() << "'" << endl;
>> +			return TestFail;
>> +		}
>
>Mmm, not sure...
>If such a format is ever added this will fail. And bayerFmt should not
>be valid in first place, so accessing .toV4L2PixelFormat() is not of
>much value... Do you think it's a valuable test case ?

My goal was to prove that using invalid input for the function results
in an empty result. In my new version, I have replaced this imaginary
case with a simple check if the usage of this static member function on
an empty BayerFormat results in an empty V4L2PixelFormat.

>
>> +
>> +		/*
>> +		 * Check if we get the expected Bayer format BGGR8
>> +		 * when we convert the V4L2PixelFormat (V4L2_PIX_FMT_SBGGR8)
>> +		 * to a Bayer format.
>> +		 */
>> +		bayerFmtExpect = BayerFormat(BayerFormat::BGGR, 8,
>> +					     BayerFormat::None);
>> +		v4l2Fmt = V4L2PixelFormat(V4L2_PIX_FMT_SBGGR8);
>> +		bayerFmt = BayerFormat::fromV4L2PixelFormat(v4l2Fmt);
>> +		if (bayerFmt != bayerFmtExpect) {
>> +			cout << "Expected BayerFormat '"
>> +			     << bayerFmtExpect.toString() << "',"
>> +			     << "got: '" << bayerFmt.toString() << "'" << endl;
>> +			return TestFail;
>> +		}
>> +
>> +		/*
>> +		 * Confirm that a V4L2PixelFormat that is not found in
>> +		 * the conversion table, doesn't yield a Bayer format.
>> +		 */
>> +		bayerFmtExpect = BayerFormat();
>> +		V4L2PixelFormat v4l2FmtUnknown = V4L2PixelFormat(
>> +			V4L2_PIX_FMT_BGRA444);
>> +		bayerFmt = BayerFormat::fromV4L2PixelFormat(v4l2FmtUnknown);
>> +		if (bayerFmt != bayerFmtExpect) {
>> +			cout << "Expected empty BayerFormat got: '"
>> +			     << bayerFmt.toString() << "'" << endl;
>> +			return TestFail;
>> +		}
>> +
>> +		/*
>> +		 * Test if a valid Bayer format can be converted to a
>> +		 * string representation.
>> +		 */
>> +		bayerFmt = BayerFormat(BayerFormat::BGGR, 8, BayerFormat::None);
>> +		if (bayerFmt.toString() != "BGGR-8") {
>> +			cout << "String representation != 'BGGR8' (got: '"
>> +			     << bayerFmt.toString() << "' ) " << endl;
>> +			return TestFail;
>> +		}
>> +
>> +		/*
>> +		 * Determine if an empty Bayer format results in no
>> +		 * string representation.
>> +		 */
>> +		bayerFmt = BayerFormat();
>> +		if (bayerFmt.toString() != "INVALID") {
>> +			cout << "String representation != 'INVALID' (got: '"
>> +			     << bayerFmt.toString() << "' ) " << endl;
>> +			return TestFail;
>> +		}
>> +
>> +		/*
>> +		 * Perform a horizontal Flip and make sure that the
>> +		 * order is adjusted accordingly.
>> +		 */
>> +		bayerFmt = BayerFormat(BayerFormat::BGGR, 8, BayerFormat::None);
>> +		bayerFmtExpect = BayerFormat(BayerFormat::GBRG, 8,
>> +					     BayerFormat::None);
>> +		BayerFormat hFlipFmt = bayerFmt.transform(Transform::HFlip);
>> +		if (hFlipFmt != bayerFmtExpect) {
>> +			cout << "horizontal flip of 'BGGR-8' "
>> +			     << "should result in '"
>> +			     << bayerFmtExpect.toString() << "', got: '"
>> +			     << hFlipFmt.toString() << "'" << endl;
>> +			return TestFail;
>> +		}
>
>nice !
>
>> +
>> +		/*
>> +		 * Perform a vertical Flip and make sure that
>> +		 * the order is adjusted accordingly.
>> +		 */
>> +		bayerFmt = BayerFormat(BayerFormat::BGGR, 8, BayerFormat::None);
>> +		bayerFmtExpect = BayerFormat(BayerFormat::GRBG, 8,
>> +					     BayerFormat::None);
>> +		BayerFormat vFlipFmt = bayerFmt.transform(Transform::VFlip);
>> +		if (vFlipFmt != bayerFmtExpect) {
>> +			cout << "vertical flip of 'BGGR-8' "
>> +			     << "should result in '"
>> +			     << bayerFmtExpect.toString() << "', got: '"
>> +			     << vFlipFmt.toString() << "'" << endl;
>> +			return TestFail;
>> +		}
>
>nice too!
>
>> +
>> +		/*
>> +		 * Perform a transposition and make sure that nothing changes.
>> +		 * Transpositions are not implemented as sensors are not
>> +		 * expected to support this functionality.
>> +		 */
>> +		bayerFmt = BayerFormat(BayerFormat::BGGR, 8, BayerFormat::None);
>> +		BayerFormat transposeFmt = bayerFmt.transform(
>> +			Transform::Transpose);
>> +		if (transposeFmt.toString() != "BGGR-8") {
>> +			cout << "Transposition is not supported "
>> +			     << "format should be 'BGGR-8', got: '"
>> +			     << transposeFmt.toString() << "'" << endl;
>> +			return TestFail;
>> +		}
>
>I would drop this last case as it is not used in practice, as you have
>said.

I will comment on this discussion within David's review.

>
>Mostly minors so please add
>Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
>
>Thanks
>  j

Thank you and Greetings,
Sebastian

>
>> +
>> +		return TestPass;
>> +	}
>> +};
>> +
>> +TEST_REGISTER(BayerFormatTest);
>> +
>> diff --git a/test/meson.build b/test/meson.build
>> index 0a1d434e..e985b0a0 100644
>> --- a/test/meson.build
>> +++ b/test/meson.build
>> @@ -23,6 +23,7 @@ public_tests = [
>>  ]
>>
>>  internal_tests = [
>> +    ['bayer-format',                    'bayer_format.cpp'],
>>      ['byte-stream-buffer',              'byte-stream-buffer.cpp'],
>>      ['camera-sensor',                   'camera-sensor.cpp'],
>>      ['event',                           'event.cpp'],
>> --
>> 2.25.1
>>