[libcamera-devel] [PATCH 2/2] test: Add unit tests for the BayerFormat class
Sebastian Fricke
sebastian.fricke.linux at gmail.com
Mon Dec 28 07:52:29 CET 2020
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 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.
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
More information about the libcamera-devel
mailing list