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

10798 diff mbox series

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@gmail.com>
---
 test/bayer_format.cpp | 202 ++++++++++++++++++++++++++++++++++++++++++
 test/meson.build      |   1 +
 2 files changed, 203 insertions(+)
 create mode 100644 test/bayer_format.cpp

Hi Sebastian,

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@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;
> +			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 ?

> +
> +		/*
> +		 * 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.

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

Thanks
  j

> +
> +		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
>

Hi Sebastian,

Thank you for the patch.

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@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.

> > +			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.

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

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> > +
> > +		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'],

Hi Sebastian

Thanks for this patch set. I think it's excellent to have some unit
tests for the BayerFormat class, especially as the original author of
the class should probably have done it... :)

Just one comment/question below:

On Mon, 11 Jan 2021 at 01:06, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Sebastian,
>
> Thank you for the patch.
>
> 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@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.
>
> > > +                   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.

I'm not quite sure if I understand this, I'm under the impression that
being able to transpose a BayerFormat is required to work correctly,
and does not depend on whether a particular sensor supports it (which
they don't).

As such I'd quite like to keep this test, though I'd prefer to avoid
testing for equality by comparing strings. I'd also quite like to see
another transpose test but where the Bayer format does change (e.g.
GRBG).

But I don't feel too strongly, and perhaps I've misunderstood, so
please add this regardless:

Reviewed-by: David Plowman <david.plowman@raspberrypi.com>

Thanks again and best regards
David

>
> > Mostly minors so please add
> > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> > > +
> > > +           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'],
>
> --
> Regards,
>
> Laurent Pinchart

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@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@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
>>

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@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.

More on this in David's review.

>
>> Mostly minors so please add
>> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
>
>Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Thank you,
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'],
>
>-- 
>Regards,
>
>Laurent Pinchart

On 12.01.2021 08:53, David Plowman wrote:
>Hi Sebastian

Hey David, Jacopo & Laurent,

>
>Thanks for this patch set. I think it's excellent to have some unit
>tests for the BayerFormat class, especially as the original author of
>the class should probably have done it... :)
>
>Just one comment/question below:
>
>On Mon, 11 Jan 2021 at 01:06, Laurent Pinchart
><laurent.pinchart@ideasonboard.com> wrote:
>>
>> Hi Sebastian,
>>
>> Thank you for the patch.
>>
>> 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@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.
>>
>> > > +                   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.
>
>I'm not quite sure if I understand this, I'm under the impression that
>being able to transpose a BayerFormat is required to work correctly,
>and does not depend on whether a particular sensor supports it (which
>they don't).

I have looked into implementing the transpose functionality for the
BayerFormat but I noticed that this is not as easy as the horizontal and
vertical flip.
Before I add more code to the transform method, I would like to make
sure, that this is something that the team believes to be useful.
So, @everyone, I would love to hear your thoughts about this.

>
>As such I'd quite like to keep this test, though I'd prefer to avoid
>testing for equality by comparing strings.

Yes I can change that.

>I'd also quite like to see
>another transpose test but where the Bayer format does change (e.g.
>GRBG).
>
>But I don't feel too strongly, and perhaps I've misunderstood, so
>please add this regardless:
>
>Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
>
>Thanks again and best regards
>David

Thank you,
Sebastian

>
>>
>> > Mostly minors so please add
>> > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
>>
>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>
>> > > +
>> > > +           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'],
>>
>> --
>> Regards,
>>
>> Laurent Pinchart