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

Mon Jan 25 07:24:03 CET 2021

On 11.01.2021 03:06, Laurent Pinchart wrote:
>Hi Sebastian,

Hey Laurent,

>
>Thank you for the patch.

Thank you for the review.

>
>On Fri, Jan 08, 2021 at 12:46:54PM +0100, Jacopo Mondi wrote:
>> 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.
>>
>> > +			     << "has to be invalid." << endl;
>
>I'd avoid splitting strings to make it easier to grep for error
>messages. This means that it would be nice to shorten a few of the
>strings below if possible.

I have tried my best to makes this possible in my next version, I hope
you will like it.

>
>> > +			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;
>
>For instance this could become
>
>			cout << "Comparison if identical formats failed" << 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 ?
>>
>> > +
>> > +		/*
>> > +		 * 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;
>
>You can squash the two string into "', got: '".
>
>> > +			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) {
>
>I would simply check if (bayerFmt.isValid()).
>
>> > +			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: '"
>
>s/BGGR8/BGGR-8/
>
>> > +			     << 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.
>
>Especially given that the test passes with the missing implementation,
>while it should fail.