[{"id":14326,"web_url":"https://patchwork.libcamera.org/comment/14326/","msgid":"","date":"2020-12-23T23:11:54","subject":"Re: [libcamera-devel] [PATCH 2/2] test: Add unit tests for the\n\tBayerFormat class","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Sebastian\n\nThanks for adding some tests to this class. Just a few comments below...\n\nOn Wed, 23 Dec 2020 at 12:11, Sebastian Fricke\n wrote:\n>\n> Test all of the present methods including the newly implemented\n> `fromV4L2PixelFormat`.\n>\n> Signed-off-by: Sebastian Fricke \n> ---\n> test/bayer_format.cpp | 154 ++++++++++++++++++++++++++++++++++++++++++\n> test/meson.build | 1 +\n> 2 files changed, 155 insertions(+)\n> create mode 100644 test/bayer_format.cpp\n>\n> diff --git a/test/bayer_format.cpp b/test/bayer_format.cpp\n> new file mode 100644\n> index 00000000..4d7c7ca1\n> --- /dev/null\n> +++ b/test/bayer_format.cpp\n> @@ -0,0 +1,154 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2020, Sebastian Fricke\n> + *\n> + * bayer_format.cpp - BayerFormat class tests\n> + */\n> +#include \n> +#include \n> +#include \n> +\n> +#include \"test.h\"\n> +\n> +using namespace std;\n> +using namespace libcamera;\n> +\n> +class BayerFormatTest : public Test\n> +{\n> +protected:\n> + int run()\n> + {\n> + /*\n> + * TEST 1: A empty bayer format has to be invalid.\n> + */\n> + BayerFormat bay_fmt = BayerFormat();\n\nThe libcamera style is normally camelCase, though I don't know to what\nextent people worry about that in test code - this would apply\nthroughout the patch, presumably. (I know I lapse frequently back into\nunderscores and have to be corrected...)\n\n> + if (bay_fmt.isValid()) {\n> + cout << \"TEST 1: FAIL: An empty bayer format \"\n> + << \"has to be invalid.\" << endl;\n> + return TestFail;\n> + }\n> +\n> + /*\n> + * TEST 2: A correct bayer format has to be valid.\n> + */\n> + bay_fmt = BayerFormat(BayerFormat::BGGR, 8, BayerFormat::None);\n> + if (!bay_fmt.isValid()) {\n> + cout << \"TEST 2: FAIL: A correct bayer format \"\n> + << \"has to be valid.\" << endl;\n> + return TestFail;\n> + }\n> +\n> + /*\n> + * TEST 3: Create a bayer format with a V4L2PixelFormat and\n> + * check if we get the same format after converting\n> + * back to the V4L2 Format.\n> + */\n> + V4L2PixelFormat pix_fmt = V4L2PixelFormat(V4L2_PIX_FMT_SBGGR8);\n> + bay_fmt = BayerFormat(pix_fmt);\n> + V4L2PixelFormat found_pix_fmt = bay_fmt.toV4L2PixelFormat();\n> + if (found_pix_fmt != pix_fmt) {\n> + cout << \"TEST 3: FAIL: expected: \"\n> + << pix_fmt.toString() << \" got: \"\n> + << found_pix_fmt.toString() << endl;\n> + return TestFail;\n> + }\n> +\n> + /*\n> + * TEST 4: Check if we get the expected bayer format BGGR8\n> + * when we convert the V4L2PixelFormat\n> + * (V4L2_PIX_FMT_SBGGR8) to a bayer format.\n> + */\n> + BayerFormat exp_bay_fmt = BayerFormat(BayerFormat::BGGR, 8,\n> + BayerFormat::None);\n> + BayerFormat found_bay_fmt = bay_fmt.fromV4L2PixelFormat(\n> + pix_fmt);\n\nSo here, I'm suspecting that\n\n BayerFormat found_bay_fmt = BayerFormat(pix_fmt);\n\nmay perform the same function. (Does this mean that the\nBayerFormat::fromV4L2PixelFormat method is in fact not required?)\n\n> + if (found_bay_fmt.order != exp_bay_fmt.order ||\n> + found_bay_fmt.bitDepth != exp_bay_fmt.bitDepth ||\n> + found_bay_fmt.packing != exp_bay_fmt.packing) {\n\nHmm, I wonder if we should add an operator==/operator!= for the\nclass... what do you think?\n\n> + cout << \"TEST 4: FAIL: Expected bayer format 'BGGR8',\"\n> + << \"got: \" << found_bay_fmt.toString() << endl;\n> + return TestFail;\n> + }\n> +\n> + /* TEST 5: Confirm that a V4L2PixelFormat that is not found in\n> + * the conversion table, doesn't yield a bayer format.\n> + */\n> + exp_bay_fmt = BayerFormat();\n> + found_bay_fmt = BayerFormat();\n> + V4L2PixelFormat unknownn_pix_fmt = V4L2PixelFormat(\n> + V4L2_PIX_FMT_BGRA444);\n> + found_bay_fmt = bay_fmt.fromV4L2PixelFormat(unknownn_pix_fmt);\n> + if (found_bay_fmt.order != exp_bay_fmt.order ||\n> + found_bay_fmt.bitDepth != exp_bay_fmt.bitDepth ||\n> + found_bay_fmt.packing != exp_bay_fmt.packing) {\n> + cout << \"TEST 5: FAIL: expected empty bayer format got: \"\n> + << found_bay_fmt.toString() << endl;\n> + return TestFail;\n> + }\n> +\n> + /*\n> + * TEST 6: Test if a valid bayer format can be converted to a\n> + * string representation.\n> + */\n> + if (bay_fmt.toString() != \"BGGR-8\") {\n> + cout << \"TEST 6: FAIL: String representation != 'BGGR8' (is: \"\n> + << bay_fmt.toString() << \" ) \" << endl;\n> + return TestFail;\n> + }\n> +\n> + /*\n> + * TEST 7: Determine if an empty bayer format results in no\n> + * string representation.\n> + */\n> + bay_fmt = BayerFormat();\n> + if (bay_fmt.toString() != \"INVALID\") {\n> + cout << \"TEST 7: FAIL: String representation != 'INVALID' (is: \"\n> + << bay_fmt.toString() << \" ) \" << endl;\n> + return TestFail;\n> + }\n> +\n> + /*\n> + * TEST 8: Perform a horizontal Flip and make sure that the\n> + * order is adjusted accordingly.\n> + */\n> + bay_fmt = BayerFormat(BayerFormat::BGGR, 8, BayerFormat::None);\n> + BayerFormat h_flip_fmt = bay_fmt.transform(Transform::HFlip);\n> + if (h_flip_fmt.toString() != \"GBRG-8\") {\n\nI wonder just a little bit about comparing to a string on the grounds\nthat you could possibly imagine the string representation changing one\nday, perhaps if the class were extended in some way. I guess this\nmaybe comes back to the question of adding an operator==?\n\n> + cout << \"TEST 8: FAIL: horizontal flip of 'BGGR-8' \"\n> + << \"should result in 'GBRG-8', got: \"\n> + << h_flip_fmt.toString() << endl;\n> + return TestFail;\n> + }\n> +\n> +\n> + /*\n> + * TEST 9: Perform a horizontal Flip and make sure that the\n\ns/horizontal/vertical/\n\n> + * order is adjusted accordingly.\n> + */\n> + bay_fmt = BayerFormat(BayerFormat::BGGR, 8, BayerFormat::None);\n> + BayerFormat v_flip_fmt = bay_fmt.transform(Transform::VFlip);\n> + if (v_flip_fmt.toString() != \"GRBG-8\") {\n> + cout << \"TEST 9: FAIL: vertical flip of 'BGGR-8' should \"\n> + << \"result in 'GRBG-8', got: \"\n> + << v_flip_fmt.toString() << endl;\n> + return TestFail;\n> + }\n> +\n> + /*\n> + * TEST 10: Perform a transposition and make sure that\n> + * nothing changes.\n> + */\n> + bay_fmt = BayerFormat(BayerFormat::BGGR, 8, BayerFormat::None);\n> + BayerFormat t_fmt = bay_fmt.transform(Transform::Transpose);\n> + if (t_fmt.toString() != \"BGGR-8\") {\n> + cout << \"TEST 10: FAIL: transposition not supported \"\n> + << \"format should be 'BGGR-8', got: \"\n> + << t_fmt.toString() << endl;\n> + return TestFail;\n> + }\n\nHaving gone to the trouble of adding a test for transpose where\nnothing changes, I wonder whether it might be worth adding one where\nit does change? So for instance, check that GBRG goes to GRBG.\n\nThanks again for adding this test code!\n\nBest regards\n\nDavid\n\n> +\n> + return TestPass;\n> + }\n> +};\n> +\n> +TEST_REGISTER(BayerFormatTest);\n> diff --git a/test/meson.build b/test/meson.build\n> index 0a1d434e..e985b0a0 100644\n> --- a/test/meson.build\n> +++ b/test/meson.build\n> @@ -23,6 +23,7 @@ public_tests = [\n> ]\n>\n> internal_tests = [\n> + ['bayer-format', 'bayer_format.cpp'],\n> ['byte-stream-buffer', 'byte-stream-buffer.cpp'],\n> ['camera-sensor', 'camera-sensor.cpp'],\n> ['event', 'event.cpp'],\n> --\n> 2.29.2\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 0FD79C0F1B\n\tfor ;\n\tWed, 23 Dec 2020 23:12:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 66C2C615AC;\n\tThu, 24 Dec 2020 00:12:09 +0100 (CET)","from mail-oi1-x231.google.com (mail-oi1-x231.google.com\n\t[IPv6:2607:f8b0:4864:20::231])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D744860528\n\tfor ;\n\tThu, 24 Dec 2020 00:12:06 +0100 (CET)","by mail-oi1-x231.google.com with SMTP id q25so725749oij.10\n\tfor ;\n\tWed, 23 Dec 2020 15:12:06 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"aEPrauqd\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=Txj8XXypz4T5CQeqbrjjTaIjp1JpUzMYDz4GJcgwXQs=;\n\tb=aEPrauqdzZFfCHmok7gXnCYwe68KMhEbJYg/D5OXz8Epk/d4nVMbboVEwF0cTEtbVN\n\toBaVrRUbVywJU3IIDc8QpZ+bf8aQGEskLmYxJ+hGWZ2RdyDjplo5NVGZ5z1q4gZixa3d\n\tsHNuIXnta7ucHffbHEJXicqWDbXG/2nfSQpR/TNR0X7QpqEHkoRBTFQDGhPUaGI3Eteq\n\tY7PpOCj6+K24HTWLB3COdH5+zyWUpam/hCksUehHx4B2ViFlaY/xWJ19vnOfUoxQLV0A\n\tdZsbJ0YXcH4c5skjZDegepd9HSggePv7oqXal8SSa37QJSWsgIJ+cR4gCgXlBE+JkHky\n\tWkRA==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=Txj8XXypz4T5CQeqbrjjTaIjp1JpUzMYDz4GJcgwXQs=;\n\tb=UCmSXlGC8f7E79KMzHEO2ZPLBqo2ANcbW4CrjW41vUadXEHOGwpaJuIc6BHubZf94P\n\t6s5bbVagRkEhFzggneZXxOikw+mFkPo2DFIDdkXtJ9iDFZ15D4q0LaNUlomF99hKqUGe\n\t67+NCJEceOr5ylsgFkNyk/lpCCpUmaGwmImMXUD1aqoJFSdrIk4Hdt+Hva+uNQNcower\n\t1Vg6UOj49mastm4G6pB4OO60799YIb3ihD16clcDqUMAsh41lVIsu0fcXgx7iAR9ZS+v\n\tE5nPFB0jPC2LjgGhx5hA8hwPIuNcrGaS7RbXBcSnkqxsqGOR5GufOkcInwXCn/A3y6OW\n\tLo/Q==","X-Gm-Message-State":"AOAM532ntAIp+klQC1GOfKF7rNQvp5wK+kC0nYhqIPbtiIlyNnrtIO0H\n\twBkO9DF+/I353+wuOW6+4JXMACIlm9OPqEYNyOse4A==","X-Google-Smtp-Source":"ABdhPJzlroMMOBL+Tgz9wWPGaRaVipGkyiY8fsYMnHK/0Mqh/2BxtB9M4K6d3UmB5jCYTeyPaGUtRQRfkFOddAH2yto=","X-Received":"by 2002:aca:4dc3:: with SMTP id\n\ta186mr1299148oib.107.1608765125547; \n\tWed, 23 Dec 2020 15:12:05 -0800 (PST)","MIME-Version":"1.0","References":"<20201223121055.14178-1-sebastian.fricke.linux@gmail.com>\n\t<20201223121055.14178-3-sebastian.fricke.linux@gmail.com>","In-Reply-To":"<20201223121055.14178-3-sebastian.fricke.linux@gmail.com>","From":"David Plowman ","Date":"Wed, 23 Dec 2020 23:11:54 +0000","Message-ID":"","To":"Sebastian Fricke ","Subject":"Re: [libcamera-devel] [PATCH 2/2] test: Add unit tests for the\n\tBayerFormat class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14331,"web_url":"https://patchwork.libcamera.org/comment/14331/","msgid":"","date":"2020-12-25T22:20:22","subject":"Re: [libcamera-devel] [PATCH 2/2] test: Add unit tests for the\n\tBayerFormat class","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Sebastian and David,\n\nOn Wed, Dec 23, 2020 at 11:11:54PM +0000, David Plowman wrote:\n> Hi Sebastian\n> \n> Thanks for adding some tests to this class. Just a few comments below...\n> \n> On Wed, 23 Dec 2020 at 12:11, Sebastian Fricke wrote:\n> >\n> > Test all of the present methods including the newly implemented\n> > `fromV4L2PixelFormat`.\n> >\n> > Signed-off-by: Sebastian Fricke \n> > ---\n> > test/bayer_format.cpp | 154 ++++++++++++++++++++++++++++++++++++++++++\n> > test/meson.build | 1 +\n> > 2 files changed, 155 insertions(+)\n> > create mode 100644 test/bayer_format.cpp\n> >\n> > diff --git a/test/bayer_format.cpp b/test/bayer_format.cpp\n> > new file mode 100644\n> > index 00000000..4d7c7ca1\n> > --- /dev/null\n> > +++ b/test/bayer_format.cpp\n> > @@ -0,0 +1,154 @@\n> > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> > +/*\n> > + * Copyright (C) 2020, Sebastian Fricke\n> > + *\n> > + * bayer_format.cpp - BayerFormat class tests\n> > + */\n\nA blank line is missing here.\n\n> > +#include \n\nAnd another one here (see the coding style document).\n\n> > +#include \n> > +#include \n> > +\n> > +#include \"test.h\"\n> > +\n> > +using namespace std;\n> > +using namespace libcamera;\n> > +\n> > +class BayerFormatTest : public Test\n> > +{\n> > +protected:\n> > + int run()\n> > + {\n> > + /*\n> > + * TEST 1: A empty bayer format has to be invalid.\n> > + */\n> > + BayerFormat bay_fmt = BayerFormat();\n> \n> The libcamera style is normally camelCase, though I don't know to what\n> extent people worry about that in test code - this would apply\n> throughout the patch, presumably. (I know I lapse frequently back into\n> underscores and have to be corrected...)\n\nThe coding style applies to test code too :-) \"bay\" sounds a bit weird\nas a prefix, I'd thus write bayerFmt, BayerFormat (my personal\npreference), or just format.\n\n> > + if (bay_fmt.isValid()) {\n> > + cout << \"TEST 1: FAIL: An empty bayer format \"\n> > + << \"has to be invalid.\" << endl;\n\nWe don't prefix failure messages with a test number. This is something\nwe may do in the future, but I think it should be addressed globally, as\npart of an effort to improve our test infrastructure. Similarly, \"FAIL:\"\nisn't required.\n\n> > + return TestFail;\n> > + }\n> > +\n> > + /*\n> > + * TEST 2: A correct bayer format has to be valid.\n> > + */\n> > + bay_fmt = BayerFormat(BayerFormat::BGGR, 8, BayerFormat::None);\n> > + if (!bay_fmt.isValid()) {\n> > + cout << \"TEST 2: FAIL: A correct bayer format \"\n> > + << \"has to be valid.\" << endl;\n> > + return TestFail;\n> > + }\n> > +\n> > + /*\n> > + * TEST 3: Create a bayer format with a V4L2PixelFormat and\n> > + * check if we get the same format after converting\n> > + * back to the V4L2 Format.\n> > + */\n> > + V4L2PixelFormat pix_fmt = V4L2PixelFormat(V4L2_PIX_FMT_SBGGR8);\n> > + bay_fmt = BayerFormat(pix_fmt);\n> > + V4L2PixelFormat found_pix_fmt = bay_fmt.toV4L2PixelFormat();\n> > + if (found_pix_fmt != pix_fmt) {\n> > + cout << \"TEST 3: FAIL: expected: \"\n> > + << pix_fmt.toString() << \" got: \"\n> > + << found_pix_fmt.toString() << endl;\n> > + return TestFail;\n> > + }\n> > +\n> > + /*\n> > + * TEST 4: Check if we get the expected bayer format BGGR8\n> > + * when we convert the V4L2PixelFormat\n> > + * (V4L2_PIX_FMT_SBGGR8) to a bayer format.\n> > + */\n> > + BayerFormat exp_bay_fmt = BayerFormat(BayerFormat::BGGR, 8,\n> > + BayerFormat::None);\n> > + BayerFormat found_bay_fmt = bay_fmt.fromV4L2PixelFormat(\n> > + pix_fmt);\n> \n> So here, I'm suspecting that\n> \n> BayerFormat found_bay_fmt = BayerFormat(pix_fmt);\n> \n> may perform the same function. (Does this mean that the\n> BayerFormat::fromV4L2PixelFormat method is in fact not required?)\n> \n> > + if (found_bay_fmt.order != exp_bay_fmt.order ||\n> > + found_bay_fmt.bitDepth != exp_bay_fmt.bitDepth ||\n> > + found_bay_fmt.packing != exp_bay_fmt.packing) {\n> \n> Hmm, I wonder if we should add an operator==/operator!= for the\n> class... what do you think?\n\nI think we should. operator!= should be defined as\n\nbool operator!=(const BayerFormat &lhs, const BayerFormat &rhs)\n{\n\treturn !(lhs == rhs);\n}\n\n(I like how C++20 groups all comparisons in a single operator<=>).\n\n> > + cout << \"TEST 4: FAIL: Expected bayer format 'BGGR8',\"\n> > + << \"got: \" << found_bay_fmt.toString() << endl;\n> > + return TestFail;\n> > + }\n> > +\n> > + /* TEST 5: Confirm that a V4L2PixelFormat that is not found in\n> > + * the conversion table, doesn't yield a bayer format.\n\nDoesn't checkstyle.py warn you that the comment should start with /* on\na line of its own ? I recommend enabling checkstyle.py as a post-commit\nhook with\n\ncp utils/hooks/post-commit .git/hooks/\n\n> > + */\n> > + exp_bay_fmt = BayerFormat();\n> > + found_bay_fmt = BayerFormat();\n> > + V4L2PixelFormat unknownn_pix_fmt = V4L2PixelFormat(\n> > + V4L2_PIX_FMT_BGRA444);\n> > + found_bay_fmt = bay_fmt.fromV4L2PixelFormat(unknownn_pix_fmt);\n> > + if (found_bay_fmt.order != exp_bay_fmt.order ||\n> > + found_bay_fmt.bitDepth != exp_bay_fmt.bitDepth ||\n> > + found_bay_fmt.packing != exp_bay_fmt.packing) {\n> > + cout << \"TEST 5: FAIL: expected empty bayer format got: \"\n> > + << found_bay_fmt.toString() << endl;\n> > + return TestFail;\n> > + }\n> > +\n> > + /*\n> > + * TEST 6: Test if a valid bayer format can be converted to a\n> > + * string representation.\n> > + */\n> > + if (bay_fmt.toString() != \"BGGR-8\") {\n> > + cout << \"TEST 6: FAIL: String representation != 'BGGR8' (is: \"\n> > + << bay_fmt.toString() << \" ) \" << endl;\n> > + return TestFail;\n> > + }\n> > +\n> > + /*\n> > + * TEST 7: Determine if an empty bayer format results in no\n> > + * string representation.\n> > + */\n> > + bay_fmt = BayerFormat();\n> > + if (bay_fmt.toString() != \"INVALID\") {\n> > + cout << \"TEST 7: FAIL: String representation != 'INVALID' (is: \"\n> > + << bay_fmt.toString() << \" ) \" << endl;\n> > + return TestFail;\n> > + }\n> > +\n> > + /*\n> > + * TEST 8: Perform a horizontal Flip and make sure that the\n> > + * order is adjusted accordingly.\n> > + */\n> > + bay_fmt = BayerFormat(BayerFormat::BGGR, 8, BayerFormat::None);\n> > + BayerFormat h_flip_fmt = bay_fmt.transform(Transform::HFlip);\n> > + if (h_flip_fmt.toString() != \"GBRG-8\") {\n> \n> I wonder just a little bit about comparing to a string on the grounds\n> that you could possibly imagine the string representation changing one\n> day, perhaps if the class were extended in some way. I guess this\n> maybe comes back to the question of adding an operator==?\n\nTesting .toSring() makes sense, for to test .transform(), I indeed think\nwe should compare two BayerFormat instances.\n\n> > + cout << \"TEST 8: FAIL: horizontal flip of 'BGGR-8' \"\n> > + << \"should result in 'GBRG-8', got: \"\n> > + << h_flip_fmt.toString() << endl;\n> > + return TestFail;\n> > + }\n> > +\n> > +\n> > + /*\n> > + * TEST 9: Perform a horizontal Flip and make sure that the\n> \n> s/horizontal/vertical/\n> \n> > + * order is adjusted accordingly.\n> > + */\n> > + bay_fmt = BayerFormat(BayerFormat::BGGR, 8, BayerFormat::None);\n> > + BayerFormat v_flip_fmt = bay_fmt.transform(Transform::VFlip);\n> > + if (v_flip_fmt.toString() != \"GRBG-8\") {\n> > + cout << \"TEST 9: FAIL: vertical flip of 'BGGR-8' should \"\n> > + << \"result in 'GRBG-8', got: \"\n> > + << v_flip_fmt.toString() << endl;\n> > + return TestFail;\n> > + }\n> > +\n> > + /*\n> > + * TEST 10: Perform a transposition and make sure that\n> > + * nothing changes.\n> > + */\n> > + bay_fmt = BayerFormat(BayerFormat::BGGR, 8, BayerFormat::None);\n> > + BayerFormat t_fmt = bay_fmt.transform(Transform::Transpose);\n> > + if (t_fmt.toString() != \"BGGR-8\") {\n> > + cout << \"TEST 10: FAIL: transposition not supported \"\n> > + << \"format should be 'BGGR-8', got: \"\n> > + << t_fmt.toString() << endl;\n> > + return TestFail;\n> > + }\n> \n> Having gone to the trouble of adding a test for transpose where\n> nothing changes, I wonder whether it might be worth adding one where\n> it does change? So for instance, check that GBRG goes to GRBG.\n> \n> Thanks again for adding this test code!\n> \n> > +\n> > + return TestPass;\n> > + }\n> > +};\n> > +\n> > +TEST_REGISTER(BayerFormatTest);\n> > diff --git a/test/meson.build b/test/meson.build\n> > index 0a1d434e..e985b0a0 100644\n> > --- a/test/meson.build\n> > +++ b/test/meson.build\n> > @@ -23,6 +23,7 @@ public_tests = [\n> > ]\n> >\n> > internal_tests = [\n> > + ['bayer-format', 'bayer_format.cpp'],\n> > ['byte-stream-buffer', 'byte-stream-buffer.cpp'],\n> > ['camera-sensor', 'camera-sensor.cpp'],\n> > ['event', 'event.cpp'],","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id CFB91C0F1A\n\tfor ;\n\tFri, 25 Dec 2020 22:20:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5188B615AC;\n\tFri, 25 Dec 2020 23:20:33 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C1A7F60525\n\tfor ;\n\tFri, 25 Dec 2020 23:20:31 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id DF2AB18B7;\n\tFri, 25 Dec 2020 23:20:30 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"YOmJOc8u\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1608934831;\n\tbh=ENygcSkHrZIdERv367OEOGDg8u1wNiNYwzrfNQOFJ9E=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=YOmJOc8u9r0jRWPfdeTDikSFnel63bnRloqMKccO9yC1HoTr2hs6m+bspTVEJh0jx\n\tegWN6vzmych2rF/pgqkvROpWZ4jM+3vrFHtJPQR3sDwhUTqFhHNwoOe6aEgJ+sbPpZ\n\tg8C42NbLWmzXiH6YQHHrqnEcdR005UHhVfkELcsU=","Date":"Sat, 26 Dec 2020 00:20:22 +0200","From":"Laurent Pinchart ","To":"David Plowman ","Message-ID":"","References":"<20201223121055.14178-1-sebastian.fricke.linux@gmail.com>\n\t<20201223121055.14178-3-sebastian.fricke.linux@gmail.com>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 2/2] test: Add unit tests for the\n\tBayerFormat class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14366,"web_url":"https://patchwork.libcamera.org/comment/14366/","msgid":"<20201228065229.poup4hinxvt3334p@basti-TUXEDO-Book-XA1510>","date":"2020-12-28T06:52:29","subject":"Re: [libcamera-devel] [PATCH 2/2] test: Add unit tests for the\n\tBayerFormat class","submitter":{"id":73,"url":"https://patchwork.libcamera.org/api/people/73/","name":"Sebastian Fricke","email":"sebastian.fricke.linux@gmail.com"},"content":"On 26.12.2020 00:20, Laurent Pinchart wrote:\n>Hi Sebastian and David,\n\nHello David and Laurent,\n\nThank you for the review, I will work on the next version and added a\nfew comments below.\n\n>\n>On Wed, Dec 23, 2020 at 11:11:54PM +0000, David Plowman wrote:\n>> Hi Sebastian\n>>\n>> Thanks for adding some tests to this class. Just a few comments below...\n>>\n>> On Wed, 23 Dec 2020 at 12:11, Sebastian Fricke wrote:\n>> >\n>> > Test all of the present methods including the newly implemented\n>> > `fromV4L2PixelFormat`.\n>> >\n>> > Signed-off-by: Sebastian Fricke \n>> > ---\n>> > test/bayer_format.cpp | 154 ++++++++++++++++++++++++++++++++++++++++++\n>> > test/meson.build | 1 +\n>> > 2 files changed, 155 insertions(+)\n>> > create mode 100644 test/bayer_format.cpp\n>> >\n>> > diff --git a/test/bayer_format.cpp b/test/bayer_format.cpp\n>> > new file mode 100644\n>> > index 00000000..4d7c7ca1\n>> > --- /dev/null\n>> > +++ b/test/bayer_format.cpp\n>> > @@ -0,0 +1,154 @@\n>> > +/* SPDX-License-Identifier: GPL-2.0-or-later */\n>> > +/*\n>> > + * Copyright (C) 2020, Sebastian Fricke\n>> > + *\n>> > + * bayer_format.cpp - BayerFormat class tests\n>> > + */\n>\n>A blank line is missing here.\n>\n>> > +#include \n>\n>And another one here (see the coding style document).\n>\n>> > +#include \n>> > +#include \n>> > +\n>> > +#include \"test.h\"\n>> > +\n>> > +using namespace std;\n>> > +using namespace libcamera;\n>> > +\n>> > +class BayerFormatTest : public Test\n>> > +{\n>> > +protected:\n>> > + int run()\n>> > + {\n>> > + /*\n>> > + * TEST 1: A empty bayer format has to be invalid.\n>> > + */\n>> > + BayerFormat bay_fmt = BayerFormat();\n>>\n>> The libcamera style is normally camelCase, though I don't know to what\n>> extent people worry about that in test code - this would apply\n>> throughout the patch, presumably. (I know I lapse frequently back into\n>> underscores and have to be corrected...)\n>\n>The coding style applies to test code too :-) \"bay\" sounds a bit weird\n>as a prefix, I'd thus write bayerFmt, BayerFormat (my personal\n>preference), or just format.\n\nYes, I should have known that already as Jacopo told me that some\npatches ago. The snake_case is just deeply embedded and I have to add\nthe checkstyle to my git hooks.\n\n>\n>> > + if (bay_fmt.isValid()) {\n>> > + cout << \"TEST 1: FAIL: An empty bayer format \"\n>> > + << \"has to be invalid.\" << endl;\n>\n>We don't prefix failure messages with a test number. This is something\n>we may do in the future, but I think it should be addressed globally, as\n>part of an effort to improve our test infrastructure. Similarly, \"FAIL:\"\n>isn't required.\n\nOK I adjust all failing test messages by removing the whole:\n'TEST \\d: FAIL:' part.\n\n>\n>> > + return TestFail;\n>> > + }\n>> > +\n>> > + /*\n>> > + * TEST 2: A correct bayer format has to be valid.\n>> > + */\n>> > + bay_fmt = BayerFormat(BayerFormat::BGGR, 8, BayerFormat::None);\n>> > + if (!bay_fmt.isValid()) {\n>> > + cout << \"TEST 2: FAIL: A correct bayer format \"\n>> > + << \"has to be valid.\" << endl;\n>> > + return TestFail;\n>> > + }\n>> > +\n>> > + /*\n>> > + * TEST 3: Create a bayer format with a V4L2PixelFormat and\n>> > + * check if we get the same format after converting\n>> > + * back to the V4L2 Format.\n>> > + */\n>> > + V4L2PixelFormat pix_fmt = V4L2PixelFormat(V4L2_PIX_FMT_SBGGR8);\n>> > + bay_fmt = BayerFormat(pix_fmt);\n>> > + V4L2PixelFormat found_pix_fmt = bay_fmt.toV4L2PixelFormat();\n>> > + if (found_pix_fmt != pix_fmt) {\n>> > + cout << \"TEST 3: FAIL: expected: \"\n>> > + << pix_fmt.toString() << \" got: \"\n>> > + << found_pix_fmt.toString() << endl;\n>> > + return TestFail;\n>> > + }\n>> > +\n>> > + /*\n>> > + * TEST 4: Check if we get the expected bayer format BGGR8\n>> > + * when we convert the V4L2PixelFormat\n>> > + * (V4L2_PIX_FMT_SBGGR8) to a bayer format.\n>> > + */\n>> > + BayerFormat exp_bay_fmt = BayerFormat(BayerFormat::BGGR, 8,\n>> > + BayerFormat::None);\n>> > + BayerFormat found_bay_fmt = bay_fmt.fromV4L2PixelFormat(\n>> > + pix_fmt);\n>>\n>> So here, I'm suspecting that\n>>\n>> BayerFormat found_bay_fmt = BayerFormat(pix_fmt);\n>>\n>> may perform the same function. (Does this mean that the\n>> BayerFormat::fromV4L2PixelFormat method is in fact not required?)\n>>\n\nI wanted to implement a similar interface to the BayerFormat class as it\nis present in the V4L2PixelFormat class. This idea was discussed here:\nhttps://patchwork.libcamera.org/patch/10684/\n\nAs suggested by Laurent, we should either go for the constructor method or do the\nstatic function way, so one of both will go probably.\n\n>> > + if (found_bay_fmt.order != exp_bay_fmt.order ||\n>> > + found_bay_fmt.bitDepth != exp_bay_fmt.bitDepth ||\n>> > + found_bay_fmt.packing != exp_bay_fmt.packing) {\n>>\n>> Hmm, I wonder if we should add an operator==/operator!= for the\n>> class... what do you think?\n>\n>I think we should. operator!= should be defined as\n>\n>bool operator!=(const BayerFormat &lhs, const BayerFormat &rhs)\n>{\n>\treturn !(lhs == rhs);\n>}\n>\n>(I like how C++20 groups all comparisons in a single operator<=>).\n\nThis will be implemented in the next version.\n\n>\n>> > + cout << \"TEST 4: FAIL: Expected bayer format 'BGGR8',\"\n>> > + << \"got: \" << found_bay_fmt.toString() << endl;\n>> > + return TestFail;\n>> > + }\n>> > +\n>> > + /* TEST 5: Confirm that a V4L2PixelFormat that is not found in\n>> > + * the conversion table, doesn't yield a bayer format.\n>\n>Doesn't checkstyle.py warn you that the comment should start with /* on\n>a line of its own ? I recommend enabling checkstyle.py as a post-commit\n>hook with\n>\n>cp utils/hooks/post-commit .git/hooks/\n\nThanks a lot, I will implement that hook.\n\n>\n>> > + */\n>> > + exp_bay_fmt = BayerFormat();\n>> > + found_bay_fmt = BayerFormat();\n>> > + V4L2PixelFormat unknownn_pix_fmt = V4L2PixelFormat(\n>> > + V4L2_PIX_FMT_BGRA444);\n>> > + found_bay_fmt = bay_fmt.fromV4L2PixelFormat(unknownn_pix_fmt);\n>> > + if (found_bay_fmt.order != exp_bay_fmt.order ||\n>> > + found_bay_fmt.bitDepth != exp_bay_fmt.bitDepth ||\n>> > + found_bay_fmt.packing != exp_bay_fmt.packing) {\n>> > + cout << \"TEST 5: FAIL: expected empty bayer format got: \"\n>> > + << found_bay_fmt.toString() << endl;\n>> > + return TestFail;\n>> > + }\n>> > +\n>> > + /*\n>> > + * TEST 6: Test if a valid bayer format can be converted to a\n>> > + * string representation.\n>> > + */\n>> > + if (bay_fmt.toString() != \"BGGR-8\") {\n>> > + cout << \"TEST 6: FAIL: String representation != 'BGGR8' (is: \"\n>> > + << bay_fmt.toString() << \" ) \" << endl;\n>> > + return TestFail;\n>> > + }\n>> > +\n>> > + /*\n>> > + * TEST 7: Determine if an empty bayer format results in no\n>> > + * string representation.\n>> > + */\n>> > + bay_fmt = BayerFormat();\n>> > + if (bay_fmt.toString() != \"INVALID\") {\n>> > + cout << \"TEST 7: FAIL: String representation != 'INVALID' (is: \"\n>> > + << bay_fmt.toString() << \" ) \" << endl;\n>> > + return TestFail;\n>> > + }\n>> > +\n>> > + /*\n>> > + * TEST 8: Perform a horizontal Flip and make sure that the\n>> > + * order is adjusted accordingly.\n>> > + */\n>> > + bay_fmt = BayerFormat(BayerFormat::BGGR, 8, BayerFormat::None);\n>> > + BayerFormat h_flip_fmt = bay_fmt.transform(Transform::HFlip);\n>> > + if (h_flip_fmt.toString() != \"GBRG-8\") {\n>>\n>> I wonder just a little bit about comparing to a string on the grounds\n>> that you could possibly imagine the string representation changing one\n>> day, perhaps if the class were extended in some way. I guess this\n>> maybe comes back to the question of adding an operator==?\n>\n>Testing .toSring() makes sense, for to test .transform(), I indeed think\n>we should compare two BayerFormat instances.\n\nWhat I liked about the string comparision is that the expected result\nalready describes quite acuratly what the performed operation should do\nand by looking at the actual result, one can spot where the operation\nfailed.\nBut I am totally on board with the maintainability aspect, so my next\nversion will contain the direct comparision between the formats.\n\n>\n>> > + cout << \"TEST 8: FAIL: horizontal flip of 'BGGR-8' \"\n>> > + << \"should result in 'GBRG-8', got: \"\n>> > + << h_flip_fmt.toString() << endl;\n>> > + return TestFail;\n>> > + }\n>> > +\n>> > +\n>> > + /*\n>> > + * TEST 9: Perform a horizontal Flip and make sure that the\n>>\n>> s/horizontal/vertical/\n>>\n>> > + * order is adjusted accordingly.\n>> > + */\n>> > + bay_fmt = BayerFormat(BayerFormat::BGGR, 8, BayerFormat::None);\n>> > + BayerFormat v_flip_fmt = bay_fmt.transform(Transform::VFlip);\n>> > + if (v_flip_fmt.toString() != \"GRBG-8\") {\n>> > + cout << \"TEST 9: FAIL: vertical flip of 'BGGR-8' should \"\n>> > + << \"result in 'GRBG-8', got: \"\n>> > + << v_flip_fmt.toString() << endl;\n>> > + return TestFail;\n>> > + }\n>> > +\n>> > + /*\n>> > + * TEST 10: Perform a transposition and make sure that\n>> > + * nothing changes.\n>> > + */\n>> > + bay_fmt = BayerFormat(BayerFormat::BGGR, 8, BayerFormat::None);\n>> > + BayerFormat t_fmt = bay_fmt.transform(Transform::Transpose);\n>> > + if (t_fmt.toString() != \"BGGR-8\") {\n>> > + cout << \"TEST 10: FAIL: transposition not supported \"\n>> > + << \"format should be 'BGGR-8', got: \"\n>> > + << t_fmt.toString() << endl;\n>> > + return TestFail;\n>> > + }\n>>\n>> Having gone to the trouble of adding a test for transpose where\n>> nothing changes, I wonder whether it might be worth adding one where\n>> it does change? So for instance, check that GBRG goes to GRBG.\n\nI'll give this a shot in the next version.\n\n>>\n>> Thanks again for adding this test code!\n>>\n\n:)\n\n>> > +\n>> > + return TestPass;\n>> > + }\n>> > +};\n>> > +\n>> > +TEST_REGISTER(BayerFormatTest);\n>> > diff --git a/test/meson.build b/test/meson.build\n>> > index 0a1d434e..e985b0a0 100644\n>> > --- a/test/meson.build\n>> > +++ b/test/meson.build\n>> > @@ -23,6 +23,7 @@ public_tests = [\n>> > ]\n>> >\n>> > internal_tests = [\n>> > + ['bayer-format', 'bayer_format.cpp'],\n>> > ['byte-stream-buffer', 'byte-stream-buffer.cpp'],\n>> > ['camera-sensor', 'camera-sensor.cpp'],\n>> > ['event', 'event.cpp'],\n>\n>-- \n>Regards,\n>\n>Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 06391C0F1A\n\tfor ;\n\tMon, 28 Dec 2020 06:52:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 73722615AF;\n\tMon, 28 Dec 2020 07:52:32 +0100 (CET)","from mail-ed1-x533.google.com (mail-ed1-x533.google.com\n\t[IPv6:2a00:1450:4864:20::533])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 851E26031B\n\tfor ;\n\tMon, 28 Dec 2020 07:52:31 +0100 (CET)","by mail-ed1-x533.google.com with SMTP id b2so8895602edm.3\n\tfor ;\n\tSun, 27 Dec 2020 22:52:31 -0800 (PST)","from localhost ([2001:16b8:577a:e001:7981:6f77:43f3:9d36])\n\tby smtp.gmail.com with ESMTPSA id\n\td22sm16680727eja.72.2020.12.27.22.52.30\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tSun, 27 Dec 2020 22:52:30 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"GVXUe22z\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:in-reply-to;\n\tbh=dcH5mlt9u5PE/fGzg5LClSS2xaiXgp7rM5DegMBE4Hg=;\n\tb=GVXUe22zAgyKZ/s3CRJ0No4j3OVaYe3U3CwxQ2qGwAv78a586tFv2s2GvyXsMkOveV\n\tukVlr6GhAh+zGvgYZl5mPB02ydyg4zgDaAA5IIz1Y8rRhgiV+At/m6EngLPV92GfHzOU\n\tJ/neZOhpkYE8PG0lvDAOi1R8efPVXV96/fHwNpNKXvbayDzzp1rrWX7fMUJhQZ0WoJKM\n\tYlBSfl04aU2ajkWuZe73l/mAasDcpKINXY76Z3NFikxan3Oud2LyqkIFgiWiNgCVmGH3\n\tTJ0q7kPb+tt2r+9sO+MYbUVAPJepDd9z22JSTPu3/xxtGYYbiPQPEijI0ypqMS5DWQEu\n\t53xw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:in-reply-to;\n\tbh=dcH5mlt9u5PE/fGzg5LClSS2xaiXgp7rM5DegMBE4Hg=;\n\tb=GyZk92NkX/OPtsYZoaKV1Nxhr20QzhntGr6QyMagEHCjFwCj2v0+u3hWQRyUovBA0/\n\t2D2reECeW21Gcyc4TVPs41MqkQzaQvkvDtaEpHi/EJlwjcmYBTxux34Sdhb8AyBI7Z2h\n\tzHmCBKyN0GA68OBFlbQwo+BrxQiYS1s4++AWDJeZCIq7gKzeqyVlq5tsp6fXCAewPdXF\n\tGHIPVpXYNFOUmv+Fmg7YM/SWVGTau6HkOBvbC2tSIOLK9s6M+5zezX9BRNRccLEWuvPd\n\tYHZrpDaSnIOzTQOlCaFbr3YXbO9SDtyFL0UXI6TxZ8Nzt5r27mOhKdHs4Mp9kWz8MJEk\n\tj98g==","X-Gm-Message-State":"AOAM532+tzTplSE1wY4pbuohOeZJ9QRrlh1Xg7U46cFn+LXzsXcChUuZ\n\txRL3OAigTrTPPaRt1geFWLryYFbVW6gCvw==","X-Google-Smtp-Source":"ABdhPJwSV6Tvjf1fKGmJQfivlrbCnA4nmEthM35A2jmBuqYdbEVoLdx34aapPsJw/QVQtju1JNrrKg==","X-Received":"by 2002:a05:6402:697:: with SMTP id\n\tf23mr41901479edy.318.1609138351006; \n\tSun, 27 Dec 2020 22:52:31 -0800 (PST)","Date":"Mon, 28 Dec 2020 07:52:29 +0100","From":"Sebastian Fricke ","To":"Laurent Pinchart ","Message-ID":"<20201228065229.poup4hinxvt3334p@basti-TUXEDO-Book-XA1510>","References":"<20201223121055.14178-1-sebastian.fricke.linux@gmail.com>\n\t<20201223121055.14178-3-sebastian.fricke.linux@gmail.com>\n\t\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 2/2] test: Add unit tests for the\n\tBayerFormat class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Content-Transfer-Encoding":"7bit","Content-Type":"text/plain; charset=\"us-ascii\"; Format=\"flowed\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]