[libcamera-devel] [PATCH v2 4/4] test: Add unit tests for the BayerFormat class
Sebastian Fricke
sebastian.fricke.linux at gmail.com
Mon Jan 25 07:49:32 CET 2021
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 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).
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 at raspberrypi.com>
>
>Thanks again and best regards
>David
Thank you,
Sebastian
>
>>
>> > 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