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

10699 diff mbox series

Test all of the present methods including the newly implemented
`fromV4L2PixelFormat`.

Signed-off-by: Sebastian Fricke <sebastian.fricke.linux@gmail.com>
---
 test/bayer_format.cpp | 154 ++++++++++++++++++++++++++++++++++++++++++
 test/meson.build      |   1 +
 2 files changed, 155 insertions(+)
 create mode 100644 test/bayer_format.cpp

Hi Sebastian

Thanks for adding some tests to this class. Just  a few comments below...

On Wed, 23 Dec 2020 at 12:11, Sebastian Fricke
<sebastian.fricke.linux@gmail.com> wrote:
>
> Test all of the present methods including the newly implemented
> `fromV4L2PixelFormat`.
>
> Signed-off-by: Sebastian Fricke <sebastian.fricke.linux@gmail.com>
> ---
>  test/bayer_format.cpp | 154 ++++++++++++++++++++++++++++++++++++++++++
>  test/meson.build      |   1 +
>  2 files changed, 155 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..4d7c7ca1
> --- /dev/null
> +++ b/test/bayer_format.cpp
> @@ -0,0 +1,154 @@
> +/* 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()
> +        {
> +                /*
> +                 * TEST 1: A empty bayer format has to be invalid.
> +                 */
> +                BayerFormat bay_fmt = BayerFormat();

The libcamera style is normally camelCase, though I don't know to what
extent people worry about that in test code - this would apply
throughout the patch, presumably. (I know I lapse frequently back into
underscores and have to be corrected...)

> +                if (bay_fmt.isValid()) {
> +                        cout << "TEST 1: FAIL: An empty bayer format "
> +                             << "has to be invalid." << endl;
> +                        return TestFail;
> +                }
> +
> +                /*
> +                 * TEST 2: A correct bayer format has to be valid.
> +                 */
> +                bay_fmt = BayerFormat(BayerFormat::BGGR, 8, BayerFormat::None);
> +                if (!bay_fmt.isValid()) {
> +                        cout << "TEST 2: FAIL: A correct bayer format "
> +                             << "has to be valid." << endl;
> +                        return TestFail;
> +                }
> +
> +                /*
> +                 * TEST 3: Create a bayer format with a V4L2PixelFormat and
> +                 *         check if we get the same format after converting
> +                 *         back to the V4L2 Format.
> +                 */
> +                V4L2PixelFormat pix_fmt = V4L2PixelFormat(V4L2_PIX_FMT_SBGGR8);
> +                bay_fmt = BayerFormat(pix_fmt);
> +                V4L2PixelFormat found_pix_fmt = bay_fmt.toV4L2PixelFormat();
> +                if (found_pix_fmt != pix_fmt) {
> +                        cout << "TEST 3: FAIL: expected: "
> +                             << pix_fmt.toString() << " got: "
> +                             << found_pix_fmt.toString() << endl;
> +                        return TestFail;
> +                }
> +
> +                /*
> +                 * TEST 4: Check if we get the expected bayer format BGGR8
> +                 *         when we convert the V4L2PixelFormat
> +                 *         (V4L2_PIX_FMT_SBGGR8) to a bayer format.
> +                 */
> +                BayerFormat exp_bay_fmt = BayerFormat(BayerFormat::BGGR, 8,
> +                                                BayerFormat::None);
> +                BayerFormat found_bay_fmt = bay_fmt.fromV4L2PixelFormat(
> +                                                pix_fmt);

So here, I'm suspecting that

                BayerFormat found_bay_fmt = BayerFormat(pix_fmt);

may perform the same function. (Does this mean that the
BayerFormat::fromV4L2PixelFormat method is in fact not required?)

> +                if (found_bay_fmt.order != exp_bay_fmt.order ||
> +                    found_bay_fmt.bitDepth != exp_bay_fmt.bitDepth ||
> +                    found_bay_fmt.packing != exp_bay_fmt.packing) {

Hmm, I wonder if we should add an operator==/operator!= for the
class... what do you think?

> +                        cout << "TEST 4: FAIL: Expected bayer format 'BGGR8',"
> +                             << "got: " << found_bay_fmt.toString() << endl;
> +                        return TestFail;
> +                }
> +
> +                /* TEST 5: Confirm that a V4L2PixelFormat that is not found in
> +                 *         the conversion table, doesn't yield a bayer format.
> +                 */
> +                exp_bay_fmt = BayerFormat();
> +                found_bay_fmt = BayerFormat();
> +                V4L2PixelFormat unknownn_pix_fmt = V4L2PixelFormat(
> +                        V4L2_PIX_FMT_BGRA444);
> +                found_bay_fmt = bay_fmt.fromV4L2PixelFormat(unknownn_pix_fmt);
> +                if (found_bay_fmt.order != exp_bay_fmt.order ||
> +                    found_bay_fmt.bitDepth != exp_bay_fmt.bitDepth ||
> +                    found_bay_fmt.packing != exp_bay_fmt.packing) {
> +                        cout << "TEST 5: FAIL: expected empty bayer format got: "
> +                             << found_bay_fmt.toString() << endl;
> +                        return TestFail;
> +                }
> +
> +                /*
> +                 * TEST 6: Test if a valid bayer format can be converted to a
> +                 *         string representation.
> +                 */
> +                if (bay_fmt.toString() != "BGGR-8") {
> +                        cout << "TEST 6: FAIL: String representation != 'BGGR8' (is: "
> +                             << bay_fmt.toString() << " ) " << endl;
> +                        return TestFail;
> +                }
> +
> +                /*
> +                 * TEST 7: Determine if an empty bayer format results in no
> +                 *         string representation.
> +                 */
> +                bay_fmt = BayerFormat();
> +                if (bay_fmt.toString() != "INVALID") {
> +                        cout << "TEST 7: FAIL: String representation != 'INVALID' (is: "
> +                             << bay_fmt.toString() << " ) " << endl;
> +                        return TestFail;
> +                }
> +
> +                /*
> +                 * TEST 8: Perform a horizontal Flip and make sure that the
> +                 *         order is adjusted accordingly.
> +                 */
> +                bay_fmt = BayerFormat(BayerFormat::BGGR, 8, BayerFormat::None);
> +                BayerFormat h_flip_fmt = bay_fmt.transform(Transform::HFlip);
> +                if (h_flip_fmt.toString() != "GBRG-8") {

I wonder just a little bit about comparing to a string on the grounds
that you could possibly imagine the string representation changing one
day, perhaps if the class were extended in some way. I guess this
maybe comes back to the question of adding an operator==?

> +                        cout << "TEST 8: FAIL: horizontal flip of 'BGGR-8' "
> +                             << "should result in 'GBRG-8', got: "
> +                             << h_flip_fmt.toString() << endl;
> +                        return TestFail;
> +                }
> +
> +
> +                /*
> +                 * TEST 9: Perform a horizontal Flip and make sure that the

s/horizontal/vertical/

> +                 *         order is adjusted accordingly.
> +                 */
> +                bay_fmt = BayerFormat(BayerFormat::BGGR, 8, BayerFormat::None);
> +                BayerFormat v_flip_fmt = bay_fmt.transform(Transform::VFlip);
> +                if (v_flip_fmt.toString() != "GRBG-8") {
> +                        cout << "TEST 9: FAIL: vertical flip of 'BGGR-8' should "
> +                             << "result in 'GRBG-8', got: "
> +                             << v_flip_fmt.toString() << endl;
> +                        return TestFail;
> +                }
> +
> +                /*
> +                 * TEST 10: Perform a transposition and make sure that
> +                 *          nothing changes.
> +                 */
> +                bay_fmt = BayerFormat(BayerFormat::BGGR, 8, BayerFormat::None);
> +                BayerFormat t_fmt = bay_fmt.transform(Transform::Transpose);
> +                if (t_fmt.toString() != "BGGR-8") {
> +                        cout << "TEST 10: FAIL: transposition not supported "
> +                             << "format should be 'BGGR-8', got: "
> +                             << t_fmt.toString() << endl;
> +                        return TestFail;
> +                }

Having gone to the trouble of adding a test for transpose where
nothing changes, I wonder whether it might be worth adding one where
it does change? So for instance, check that GBRG goes to GRBG.

Thanks again for adding this test code!

Best regards

David

> +
> +                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.29.2
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Hi Sebastian and David,

On Wed, Dec 23, 2020 at 11:11:54PM +0000, David Plowman wrote:
> Hi Sebastian
> 
> Thanks for adding some tests to this class. Just  a few comments below...
> 
> On Wed, 23 Dec 2020 at 12:11, Sebastian Fricke wrote:
> >
> > Test all of the present methods including the newly implemented
> > `fromV4L2PixelFormat`.
> >
> > Signed-off-by: Sebastian Fricke <sebastian.fricke.linux@gmail.com>
> > ---
> >  test/bayer_format.cpp | 154 ++++++++++++++++++++++++++++++++++++++++++
> >  test/meson.build      |   1 +
> >  2 files changed, 155 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..4d7c7ca1
> > --- /dev/null
> > +++ b/test/bayer_format.cpp
> > @@ -0,0 +1,154 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +/*
> > + * Copyright (C) 2020, Sebastian Fricke
> > + *
> > + * bayer_format.cpp - BayerFormat class tests
> > + */

A blank line is missing here.

> > +#include <iostream>

And another one here (see the coding style document).

> > +#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()
> > +        {
> > +                /*
> > +                 * TEST 1: A empty bayer format has to be invalid.
> > +                 */
> > +                BayerFormat bay_fmt = BayerFormat();
> 
> The libcamera style is normally camelCase, though I don't know to what
> extent people worry about that in test code - this would apply
> throughout the patch, presumably. (I know I lapse frequently back into
> underscores and have to be corrected...)

The coding style applies to test code too :-) "bay" sounds a bit weird
as a prefix, I'd thus write bayerFmt, BayerFormat (my personal
preference), or just format.

> > +                if (bay_fmt.isValid()) {
> > +                        cout << "TEST 1: FAIL: An empty bayer format "
> > +                             << "has to be invalid." << endl;

We don't prefix failure messages with a test number. This is something
we may do in the future, but I think it should be addressed globally, as
part of an effort to improve our test infrastructure. Similarly, "FAIL:"
isn't required.

> > +                        return TestFail;
> > +                }
> > +
> > +                /*
> > +                 * TEST 2: A correct bayer format has to be valid.
> > +                 */
> > +                bay_fmt = BayerFormat(BayerFormat::BGGR, 8, BayerFormat::None);
> > +                if (!bay_fmt.isValid()) {
> > +                        cout << "TEST 2: FAIL: A correct bayer format "
> > +                             << "has to be valid." << endl;
> > +                        return TestFail;
> > +                }
> > +
> > +                /*
> > +                 * TEST 3: Create a bayer format with a V4L2PixelFormat and
> > +                 *         check if we get the same format after converting
> > +                 *         back to the V4L2 Format.
> > +                 */
> > +                V4L2PixelFormat pix_fmt = V4L2PixelFormat(V4L2_PIX_FMT_SBGGR8);
> > +                bay_fmt = BayerFormat(pix_fmt);
> > +                V4L2PixelFormat found_pix_fmt = bay_fmt.toV4L2PixelFormat();
> > +                if (found_pix_fmt != pix_fmt) {
> > +                        cout << "TEST 3: FAIL: expected: "
> > +                             << pix_fmt.toString() << " got: "
> > +                             << found_pix_fmt.toString() << endl;
> > +                        return TestFail;
> > +                }
> > +
> > +                /*
> > +                 * TEST 4: Check if we get the expected bayer format BGGR8
> > +                 *         when we convert the V4L2PixelFormat
> > +                 *         (V4L2_PIX_FMT_SBGGR8) to a bayer format.
> > +                 */
> > +                BayerFormat exp_bay_fmt = BayerFormat(BayerFormat::BGGR, 8,
> > +                                                BayerFormat::None);
> > +                BayerFormat found_bay_fmt = bay_fmt.fromV4L2PixelFormat(
> > +                                                pix_fmt);
> 
> So here, I'm suspecting that
> 
>                 BayerFormat found_bay_fmt = BayerFormat(pix_fmt);
> 
> may perform the same function. (Does this mean that the
> BayerFormat::fromV4L2PixelFormat method is in fact not required?)
> 
> > +                if (found_bay_fmt.order != exp_bay_fmt.order ||
> > +                    found_bay_fmt.bitDepth != exp_bay_fmt.bitDepth ||
> > +                    found_bay_fmt.packing != exp_bay_fmt.packing) {
> 
> Hmm, I wonder if we should add an operator==/operator!= for the
> class... what do you think?

I think we should. operator!= should be defined as

bool operator!=(const BayerFormat &lhs, const BayerFormat &rhs)
{
	return !(lhs == rhs);
}

(I like how C++20 groups all comparisons in a single operator<=>).

> > +                        cout << "TEST 4: FAIL: Expected bayer format 'BGGR8',"
> > +                             << "got: " << found_bay_fmt.toString() << endl;
> > +                        return TestFail;
> > +                }
> > +
> > +                /* TEST 5: Confirm that a V4L2PixelFormat that is not found in
> > +                 *         the conversion table, doesn't yield a bayer format.

Doesn't checkstyle.py warn you that the comment should start with /* on
a line of its own ? I recommend enabling checkstyle.py as a post-commit
hook with

cp utils/hooks/post-commit .git/hooks/

> > +                 */
> > +                exp_bay_fmt = BayerFormat();
> > +                found_bay_fmt = BayerFormat();
> > +                V4L2PixelFormat unknownn_pix_fmt = V4L2PixelFormat(
> > +                        V4L2_PIX_FMT_BGRA444);
> > +                found_bay_fmt = bay_fmt.fromV4L2PixelFormat(unknownn_pix_fmt);
> > +                if (found_bay_fmt.order != exp_bay_fmt.order ||
> > +                    found_bay_fmt.bitDepth != exp_bay_fmt.bitDepth ||
> > +                    found_bay_fmt.packing != exp_bay_fmt.packing) {
> > +                        cout << "TEST 5: FAIL: expected empty bayer format got: "
> > +                             << found_bay_fmt.toString() << endl;
> > +                        return TestFail;
> > +                }
> > +
> > +                /*
> > +                 * TEST 6: Test if a valid bayer format can be converted to a
> > +                 *         string representation.
> > +                 */
> > +                if (bay_fmt.toString() != "BGGR-8") {
> > +                        cout << "TEST 6: FAIL: String representation != 'BGGR8' (is: "
> > +                             << bay_fmt.toString() << " ) " << endl;
> > +                        return TestFail;
> > +                }
> > +
> > +                /*
> > +                 * TEST 7: Determine if an empty bayer format results in no
> > +                 *         string representation.
> > +                 */
> > +                bay_fmt = BayerFormat();
> > +                if (bay_fmt.toString() != "INVALID") {
> > +                        cout << "TEST 7: FAIL: String representation != 'INVALID' (is: "
> > +                             << bay_fmt.toString() << " ) " << endl;
> > +                        return TestFail;
> > +                }
> > +
> > +                /*
> > +                 * TEST 8: Perform a horizontal Flip and make sure that the
> > +                 *         order is adjusted accordingly.
> > +                 */
> > +                bay_fmt = BayerFormat(BayerFormat::BGGR, 8, BayerFormat::None);
> > +                BayerFormat h_flip_fmt = bay_fmt.transform(Transform::HFlip);
> > +                if (h_flip_fmt.toString() != "GBRG-8") {
> 
> I wonder just a little bit about comparing to a string on the grounds
> that you could possibly imagine the string representation changing one
> day, perhaps if the class were extended in some way. I guess this
> maybe comes back to the question of adding an operator==?

Testing .toSring() makes sense, for to test .transform(), I indeed think
we should compare two BayerFormat instances.

> > +                        cout << "TEST 8: FAIL: horizontal flip of 'BGGR-8' "
> > +                             << "should result in 'GBRG-8', got: "
> > +                             << h_flip_fmt.toString() << endl;
> > +                        return TestFail;
> > +                }
> > +
> > +
> > +                /*
> > +                 * TEST 9: Perform a horizontal Flip and make sure that the
> 
> s/horizontal/vertical/
> 
> > +                 *         order is adjusted accordingly.
> > +                 */
> > +                bay_fmt = BayerFormat(BayerFormat::BGGR, 8, BayerFormat::None);
> > +                BayerFormat v_flip_fmt = bay_fmt.transform(Transform::VFlip);
> > +                if (v_flip_fmt.toString() != "GRBG-8") {
> > +                        cout << "TEST 9: FAIL: vertical flip of 'BGGR-8' should "
> > +                             << "result in 'GRBG-8', got: "
> > +                             << v_flip_fmt.toString() << endl;
> > +                        return TestFail;
> > +                }
> > +
> > +                /*
> > +                 * TEST 10: Perform a transposition and make sure that
> > +                 *          nothing changes.
> > +                 */
> > +                bay_fmt = BayerFormat(BayerFormat::BGGR, 8, BayerFormat::None);
> > +                BayerFormat t_fmt = bay_fmt.transform(Transform::Transpose);
> > +                if (t_fmt.toString() != "BGGR-8") {
> > +                        cout << "TEST 10: FAIL: transposition not supported "
> > +                             << "format should be 'BGGR-8', got: "
> > +                             << t_fmt.toString() << endl;
> > +                        return TestFail;
> > +                }
> 
> Having gone to the trouble of adding a test for transpose where
> nothing changes, I wonder whether it might be worth adding one where
> it does change? So for instance, check that GBRG goes to GRBG.
> 
> Thanks again for adding this test code!
> 
> > +
> > +                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'],

On 26.12.2020 00:20, Laurent Pinchart wrote:
>Hi Sebastian and David,

Hello David and Laurent,

Thank you for the review, I will work on the next version and added a
few comments below.

>
>On Wed, Dec 23, 2020 at 11:11:54PM +0000, David Plowman wrote:
>> Hi Sebastian
>>
>> Thanks for adding some tests to this class. Just  a few comments below...
>>
>> On Wed, 23 Dec 2020 at 12:11, Sebastian Fricke wrote:
>> >
>> > Test all of the present methods including the newly implemented
>> > `fromV4L2PixelFormat`.
>> >
>> > Signed-off-by: Sebastian Fricke <sebastian.fricke.linux@gmail.com>
>> > ---
>> >  test/bayer_format.cpp | 154 ++++++++++++++++++++++++++++++++++++++++++
>> >  test/meson.build      |   1 +
>> >  2 files changed, 155 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..4d7c7ca1
>> > --- /dev/null
>> > +++ b/test/bayer_format.cpp
>> > @@ -0,0 +1,154 @@
>> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> > +/*
>> > + * Copyright (C) 2020, Sebastian Fricke
>> > + *
>> > + * bayer_format.cpp - BayerFormat class tests
>> > + */
>
>A blank line is missing here.
>
>> > +#include <iostream>
>
>And another one here (see the coding style document).
>
>> > +#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()
>> > +        {
>> > +                /*
>> > +                 * TEST 1: A empty bayer format has to be invalid.
>> > +                 */
>> > +                BayerFormat bay_fmt = BayerFormat();
>>
>> The libcamera style is normally camelCase, though I don't know to what
>> extent people worry about that in test code - this would apply
>> throughout the patch, presumably. (I know I lapse frequently back into
>> underscores and have to be corrected...)
>
>The coding style applies to test code too :-) "bay" sounds a bit weird
>as a prefix, I'd thus write bayerFmt, BayerFormat (my personal
>preference), or just format.

Yes, I should have known that already as Jacopo told me that some
patches ago. The snake_case is just deeply embedded and I have to add
the checkstyle to my git hooks.

>
>> > +                if (bay_fmt.isValid()) {
>> > +                        cout << "TEST 1: FAIL: An empty bayer format "
>> > +                             << "has to be invalid." << endl;
>
>We don't prefix failure messages with a test number. This is something
>we may do in the future, but I think it should be addressed globally, as
>part of an effort to improve our test infrastructure. Similarly, "FAIL:"
>isn't required.

OK I adjust all failing test messages by removing the whole:
'TEST \d: FAIL:' part.

>
>> > +                        return TestFail;
>> > +                }
>> > +
>> > +                /*
>> > +                 * TEST 2: A correct bayer format has to be valid.
>> > +                 */
>> > +                bay_fmt = BayerFormat(BayerFormat::BGGR, 8, BayerFormat::None);
>> > +                if (!bay_fmt.isValid()) {
>> > +                        cout << "TEST 2: FAIL: A correct bayer format "
>> > +                             << "has to be valid." << endl;
>> > +                        return TestFail;
>> > +                }
>> > +
>> > +                /*
>> > +                 * TEST 3: Create a bayer format with a V4L2PixelFormat and
>> > +                 *         check if we get the same format after converting
>> > +                 *         back to the V4L2 Format.
>> > +                 */
>> > +                V4L2PixelFormat pix_fmt = V4L2PixelFormat(V4L2_PIX_FMT_SBGGR8);
>> > +                bay_fmt = BayerFormat(pix_fmt);
>> > +                V4L2PixelFormat found_pix_fmt = bay_fmt.toV4L2PixelFormat();
>> > +                if (found_pix_fmt != pix_fmt) {
>> > +                        cout << "TEST 3: FAIL: expected: "
>> > +                             << pix_fmt.toString() << " got: "
>> > +                             << found_pix_fmt.toString() << endl;
>> > +                        return TestFail;
>> > +                }
>> > +
>> > +                /*
>> > +                 * TEST 4: Check if we get the expected bayer format BGGR8
>> > +                 *         when we convert the V4L2PixelFormat
>> > +                 *         (V4L2_PIX_FMT_SBGGR8) to a bayer format.
>> > +                 */
>> > +                BayerFormat exp_bay_fmt = BayerFormat(BayerFormat::BGGR, 8,
>> > +                                                BayerFormat::None);
>> > +                BayerFormat found_bay_fmt = bay_fmt.fromV4L2PixelFormat(
>> > +                                                pix_fmt);
>>
>> So here, I'm suspecting that
>>
>>                 BayerFormat found_bay_fmt = BayerFormat(pix_fmt);
>>
>> may perform the same function. (Does this mean that the
>> BayerFormat::fromV4L2PixelFormat method is in fact not required?)
>>

I wanted to implement a similar interface to the BayerFormat class as it
is present in the V4L2PixelFormat class. This idea was discussed here:
https://patchwork.libcamera.org/patch/10684/

As suggested by Laurent, we should either go for the constructor method or do the
static function way, so one of both will go probably.

>> > +                if (found_bay_fmt.order != exp_bay_fmt.order ||
>> > +                    found_bay_fmt.bitDepth != exp_bay_fmt.bitDepth ||
>> > +                    found_bay_fmt.packing != exp_bay_fmt.packing) {
>>
>> Hmm, I wonder if we should add an operator==/operator!= for the
>> class... what do you think?
>
>I think we should. operator!= should be defined as
>
>bool operator!=(const BayerFormat &lhs, const BayerFormat &rhs)
>{
>	return !(lhs == rhs);
>}
>
>(I like how C++20 groups all comparisons in a single operator<=>).

This will be implemented in the next version.

>
>> > +                        cout << "TEST 4: FAIL: Expected bayer format 'BGGR8',"
>> > +                             << "got: " << found_bay_fmt.toString() << endl;
>> > +                        return TestFail;
>> > +                }
>> > +
>> > +                /* TEST 5: Confirm that a V4L2PixelFormat that is not found in
>> > +                 *         the conversion table, doesn't yield a bayer format.
>
>Doesn't checkstyle.py warn you that the comment should start with /* on
>a line of its own ? I recommend enabling checkstyle.py as a post-commit
>hook with
>
>cp utils/hooks/post-commit .git/hooks/

Thanks a lot, I will implement that hook.

>
>> > +                 */
>> > +                exp_bay_fmt = BayerFormat();
>> > +                found_bay_fmt = BayerFormat();
>> > +                V4L2PixelFormat unknownn_pix_fmt = V4L2PixelFormat(
>> > +                        V4L2_PIX_FMT_BGRA444);
>> > +                found_bay_fmt = bay_fmt.fromV4L2PixelFormat(unknownn_pix_fmt);
>> > +                if (found_bay_fmt.order != exp_bay_fmt.order ||
>> > +                    found_bay_fmt.bitDepth != exp_bay_fmt.bitDepth ||
>> > +                    found_bay_fmt.packing != exp_bay_fmt.packing) {
>> > +                        cout << "TEST 5: FAIL: expected empty bayer format got: "
>> > +                             << found_bay_fmt.toString() << endl;
>> > +                        return TestFail;
>> > +                }
>> > +
>> > +                /*
>> > +                 * TEST 6: Test if a valid bayer format can be converted to a
>> > +                 *         string representation.
>> > +                 */
>> > +                if (bay_fmt.toString() != "BGGR-8") {
>> > +                        cout << "TEST 6: FAIL: String representation != 'BGGR8' (is: "
>> > +                             << bay_fmt.toString() << " ) " << endl;
>> > +                        return TestFail;
>> > +                }
>> > +
>> > +                /*
>> > +                 * TEST 7: Determine if an empty bayer format results in no
>> > +                 *         string representation.
>> > +                 */
>> > +                bay_fmt = BayerFormat();
>> > +                if (bay_fmt.toString() != "INVALID") {
>> > +                        cout << "TEST 7: FAIL: String representation != 'INVALID' (is: "
>> > +                             << bay_fmt.toString() << " ) " << endl;
>> > +                        return TestFail;
>> > +                }
>> > +
>> > +                /*
>> > +                 * TEST 8: Perform a horizontal Flip and make sure that the
>> > +                 *         order is adjusted accordingly.
>> > +                 */
>> > +                bay_fmt = BayerFormat(BayerFormat::BGGR, 8, BayerFormat::None);
>> > +                BayerFormat h_flip_fmt = bay_fmt.transform(Transform::HFlip);
>> > +                if (h_flip_fmt.toString() != "GBRG-8") {
>>
>> I wonder just a little bit about comparing to a string on the grounds
>> that you could possibly imagine the string representation changing one
>> day, perhaps if the class were extended in some way. I guess this
>> maybe comes back to the question of adding an operator==?
>
>Testing .toSring() makes sense, for to test .transform(), I indeed think
>we should compare two BayerFormat instances.

What I liked about the string comparision is that the expected result
already describes quite acuratly what the performed operation should do
and by looking at the actual result, one can spot where the operation
failed.
But I am totally on board with the maintainability aspect, so my next
version will contain the direct comparision between the formats.

>
>> > +                        cout << "TEST 8: FAIL: horizontal flip of 'BGGR-8' "
>> > +                             << "should result in 'GBRG-8', got: "
>> > +                             << h_flip_fmt.toString() << endl;
>> > +                        return TestFail;
>> > +                }
>> > +
>> > +
>> > +                /*
>> > +                 * TEST 9: Perform a horizontal Flip and make sure that the
>>
>> s/horizontal/vertical/
>>
>> > +                 *         order is adjusted accordingly.
>> > +                 */
>> > +                bay_fmt = BayerFormat(BayerFormat::BGGR, 8, BayerFormat::None);
>> > +                BayerFormat v_flip_fmt = bay_fmt.transform(Transform::VFlip);
>> > +                if (v_flip_fmt.toString() != "GRBG-8") {
>> > +                        cout << "TEST 9: FAIL: vertical flip of 'BGGR-8' should "
>> > +                             << "result in 'GRBG-8', got: "
>> > +                             << v_flip_fmt.toString() << endl;
>> > +                        return TestFail;
>> > +                }
>> > +
>> > +                /*
>> > +                 * TEST 10: Perform a transposition and make sure that
>> > +                 *          nothing changes.
>> > +                 */
>> > +                bay_fmt = BayerFormat(BayerFormat::BGGR, 8, BayerFormat::None);
>> > +                BayerFormat t_fmt = bay_fmt.transform(Transform::Transpose);
>> > +                if (t_fmt.toString() != "BGGR-8") {
>> > +                        cout << "TEST 10: FAIL: transposition not supported "
>> > +                             << "format should be 'BGGR-8', got: "
>> > +                             << t_fmt.toString() << endl;
>> > +                        return TestFail;
>> > +                }
>>
>> Having gone to the trouble of adding a test for transpose where
>> nothing changes, I wonder whether it might be worth adding one where
>> it does change? So for instance, check that GBRG goes to GRBG.

I'll give this a shot in the next version.

>>
>> Thanks again for adding this test code!
>>

:)

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