[libcamera-devel] [PATCH v2 4/4] test: Add unit tests for the BayerFormat class
David Plowman
david.plowman at raspberrypi.com
Tue Jan 12 09:53:54 CET 2021
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 at 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 at gmail.com>
> > > ---
> > > test/bayer_format.cpp | 202 ++++++++++++++++++++++++++++++++++++++++++
> > > test/meson.build | 1 +
> > > 2 files changed, 203 insertions(+)
> > > create mode 100644 test/bayer_format.cpp
> > >
> > > diff --git a/test/bayer_format.cpp b/test/bayer_format.cpp
> > > new file mode 100644
> > > index 00000000..dd7aa8cb
> > > --- /dev/null
> > > +++ b/test/bayer_format.cpp
> > > @@ -0,0 +1,202 @@
> > > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > > +/*
> > > + * Copyright (C) 2020, Sebastian Fricke
> > > + *
> > > + * bayer_format.cpp - BayerFormat class tests
> > > + */
> > > +
> > > +#include <iostream>
> > > +
> > > +#include <libcamera/internal/bayer_format.h>
> > > +#include <libcamera/transform.h>
> > > +
> > > +#include "test.h"
> > > +
> > > +using namespace std;
> > > +using namespace libcamera;
> > > +
> > > +class BayerFormatTest : public Test
> > > +{
> > > +protected:
> > > + int run()
> > > + {
> > > + /*
> > > + * An empty Bayer format has to be invalid.
> > > + */
> >
> > Fits on 1 line (also other comments)
> >
> > > + BayerFormat bayerFmt = BayerFormat();
> >
> > Just
> > BayerFormat bayerFmt;
> > ?
> >
> > > + if (bayerFmt.isValid()) {
> > > + cout << "An empty BayerFormat "
> >
> > We have a mixed usage of cout/cerr in test/
> > But in new tests I would use cerr.
> >
> > > + << "has to be invalid." << endl;
>
> I'd avoid splitting strings to make it easier to grep for error
> messages. This means that it would be nice to shorten a few of the
> strings below if possible.
>
> > > + 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 at raspberrypi.com>
Thanks again and best regards
David
>
> > Mostly minors so please add
> > Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at 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
More information about the libcamera-devel
mailing list