[libcamera-devel] [PATCH 2/2] test: Add unit tests for the BayerFormat class
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Dec 25 23:20:22 CET 2020
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 at 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'],
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list