[libcamera-devel] [PATCH 2/2] test: Add unit tests for the BayerFormat class
David Plowman
david.plowman at raspberrypi.com
Thu Dec 24 00:11:54 CET 2020
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 at gmail.com> 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
> + */
> +#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 at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
More information about the libcamera-devel
mailing list