[{"id":14476,"web_url":"https://patchwork.libcamera.org/comment/14476/","msgid":"<20210108114654.g63ock4nbtiwlhpc@uno.localdomain>","date":"2021-01-08T11:46:54","subject":"Re: [libcamera-devel] [PATCH v2 4/4] test: Add unit tests for the\n\tBayerFormat class","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Sebastian,\n\nOn Thu, Dec 31, 2020 at 04:53:36PM +0100, Sebastian Fricke wrote:\n> Test all of the present methods including the newly implemented\n> `fromV4L2PixelFormat`, as well as the new operators `==/!=`.\n>\n> Signed-off-by: Sebastian Fricke \n> ---\n> test/bayer_format.cpp | 202 ++++++++++++++++++++++++++++++++++++++++++\n> test/meson.build | 1 +\n> 2 files changed, 203 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..dd7aa8cb\n> --- /dev/null\n> +++ b/test/bayer_format.cpp\n> @@ -0,0 +1,202 @@\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> +#include \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> +\tint run()\n> +\t{\n> +\t\t/*\n> +\t\t * An empty Bayer format has to be invalid.\n> +\t\t */\n\nFits on 1 line (also other comments)\n\n> +\t\tBayerFormat bayerFmt = BayerFormat();\n\nJust\n BayerFormat bayerFmt;\n?\n\n> +\t\tif (bayerFmt.isValid()) {\n> +\t\t\tcout << \"An empty BayerFormat \"\n\nWe have a mixed usage of cout/cerr in test/\nBut in new tests I would use cerr.\n\n> +\t\t\t << \"has to be invalid.\" << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\t/*\n> +\t\t * A correct Bayer format has to be valid.\n> +\t\t */\n> +\t\tbayerFmt = BayerFormat(BayerFormat::BGGR, 8, BayerFormat::None);\n> +\t\tif (!bayerFmt.isValid()) {\n> +\t\t\tcout << \"A correct BayerFormat \"\n> +\t\t\t << \"has to be valid.\" << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\t/*\n> +\t\t * Two bayer formats created with the same order and bit depth\n> +\t\t * have to be equal.\n> +\t\t */\n> +\t\tbayerFmt = BayerFormat(BayerFormat::BGGR, 8, BayerFormat::None);\n> +\t\tBayerFormat bayerFmtExpect = BayerFormat(BayerFormat::BGGR, 8,\n> +\t\t\t\t\t\t\t BayerFormat::None);\n> +\t\tif (bayerFmt != bayerFmtExpect) {\n> +\t\t\tcout << \"Two BayerFormat object created with the same \"\n> +\t\t\t << \"order and bitDepth must be equal.\" << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\t/*\n> +\t\t * Two Bayer formats created with the same order but with a\n> +\t\t * different bitDepth are not equal.\n> +\t\t */\n> +\t\tbayerFmt = BayerFormat(BayerFormat::BGGR, 8, BayerFormat::None);\n> +\t\tbayerFmtExpect = BayerFormat(BayerFormat::BGGR, 12,\n> +\t\t\t\t\t BayerFormat::None);\n> +\t\tif (bayerFmt == bayerFmtExpect) {\n> +\t\t\tcout << \"Two BayerFormat object created with the same \"\n> +\t\t\t << \"order, but different bit depths are not equal.\"\n> +\t\t\t << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\t/*\n> +\t\t * Create a Bayer format with a V4L2PixelFormat and check if we\n> +\t\t * get the same format after converting back to the V4L2 Format.\n> +\t\t */\n> +\t\tV4L2PixelFormat v4l2FmtExpect = V4L2PixelFormat(\n> +\t\t\tV4L2_PIX_FMT_SBGGR8);\n> +\t\tbayerFmt = BayerFormat::fromV4L2PixelFormat(v4l2FmtExpect);\n> +\t\tV4L2PixelFormat v4l2Fmt = bayerFmt.toV4L2PixelFormat();\n> +\t\tif (v4l2Fmt != v4l2FmtExpect) {\n> +\t\t\tcout << \"Expected: '\" << v4l2FmtExpect.toString()\n> +\t\t\t << \"' got: '\" << v4l2Fmt.toString() << \"'\" << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\t/*\n> +\t\t * Use a Bayer format that is not found in the mapping table\n> +\t\t * and verify that no matching V4L2PixelFormat is found.\n> +\t\t */\n> +\t\tv4l2FmtExpect = V4L2PixelFormat();\n> +\t\tbayerFmt = BayerFormat(BayerFormat::BGGR, 24,\n> +\t\t\t\t BayerFormat::None);\n> +\t\tv4l2Fmt = bayerFmt.toV4L2PixelFormat();\n> +\t\tif (v4l2Fmt != v4l2FmtExpect) {\n> +\t\t\tcout << \"Expected: empty V4L2PixelFormat got: '\"\n> +\t\t\t << v4l2Fmt.toString() << \"'\" << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n\nMmm, not sure...\nIf such a format is ever added this will fail. And bayerFmt should not\nbe valid in first place, so accessing .toV4L2PixelFormat() is not of\nmuch value... Do you think it's a valuable test case ?\n\n> +\n> +\t\t/*\n> +\t\t * Check if we get the expected Bayer format BGGR8\n> +\t\t * when we convert the V4L2PixelFormat (V4L2_PIX_FMT_SBGGR8)\n> +\t\t * to a Bayer format.\n> +\t\t */\n> +\t\tbayerFmtExpect = BayerFormat(BayerFormat::BGGR, 8,\n> +\t\t\t\t\t BayerFormat::None);\n> +\t\tv4l2Fmt = V4L2PixelFormat(V4L2_PIX_FMT_SBGGR8);\n> +\t\tbayerFmt = BayerFormat::fromV4L2PixelFormat(v4l2Fmt);\n> +\t\tif (bayerFmt != bayerFmtExpect) {\n> +\t\t\tcout << \"Expected BayerFormat '\"\n> +\t\t\t << bayerFmtExpect.toString() << \"',\"\n> +\t\t\t << \"got: '\" << bayerFmt.toString() << \"'\" << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\t/*\n> +\t\t * Confirm that a V4L2PixelFormat that is not found in\n> +\t\t * the conversion table, doesn't yield a Bayer format.\n> +\t\t */\n> +\t\tbayerFmtExpect = BayerFormat();\n> +\t\tV4L2PixelFormat v4l2FmtUnknown = V4L2PixelFormat(\n> +\t\t\tV4L2_PIX_FMT_BGRA444);\n> +\t\tbayerFmt = BayerFormat::fromV4L2PixelFormat(v4l2FmtUnknown);\n> +\t\tif (bayerFmt != bayerFmtExpect) {\n> +\t\t\tcout << \"Expected empty BayerFormat got: '\"\n> +\t\t\t << bayerFmt.toString() << \"'\" << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\t/*\n> +\t\t * Test if a valid Bayer format can be converted to a\n> +\t\t * string representation.\n> +\t\t */\n> +\t\tbayerFmt = BayerFormat(BayerFormat::BGGR, 8, BayerFormat::None);\n> +\t\tif (bayerFmt.toString() != \"BGGR-8\") {\n> +\t\t\tcout << \"String representation != 'BGGR8' (got: '\"\n> +\t\t\t << bayerFmt.toString() << \"' ) \" << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\t/*\n> +\t\t * Determine if an empty Bayer format results in no\n> +\t\t * string representation.\n> +\t\t */\n> +\t\tbayerFmt = BayerFormat();\n> +\t\tif (bayerFmt.toString() != \"INVALID\") {\n> +\t\t\tcout << \"String representation != 'INVALID' (got: '\"\n> +\t\t\t << bayerFmt.toString() << \"' ) \" << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\t/*\n> +\t\t * Perform a horizontal Flip and make sure that the\n> +\t\t * order is adjusted accordingly.\n> +\t\t */\n> +\t\tbayerFmt = BayerFormat(BayerFormat::BGGR, 8, BayerFormat::None);\n> +\t\tbayerFmtExpect = BayerFormat(BayerFormat::GBRG, 8,\n> +\t\t\t\t\t BayerFormat::None);\n> +\t\tBayerFormat hFlipFmt = bayerFmt.transform(Transform::HFlip);\n> +\t\tif (hFlipFmt != bayerFmtExpect) {\n> +\t\t\tcout << \"horizontal flip of 'BGGR-8' \"\n> +\t\t\t << \"should result in '\"\n> +\t\t\t << bayerFmtExpect.toString() << \"', got: '\"\n> +\t\t\t << hFlipFmt.toString() << \"'\" << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n\nnice !\n\n> +\n> +\t\t/*\n> +\t\t * Perform a vertical Flip and make sure that\n> +\t\t * the order is adjusted accordingly.\n> +\t\t */\n> +\t\tbayerFmt = BayerFormat(BayerFormat::BGGR, 8, BayerFormat::None);\n> +\t\tbayerFmtExpect = BayerFormat(BayerFormat::GRBG, 8,\n> +\t\t\t\t\t BayerFormat::None);\n> +\t\tBayerFormat vFlipFmt = bayerFmt.transform(Transform::VFlip);\n> +\t\tif (vFlipFmt != bayerFmtExpect) {\n> +\t\t\tcout << \"vertical flip of 'BGGR-8' \"\n> +\t\t\t << \"should result in '\"\n> +\t\t\t << bayerFmtExpect.toString() << \"', got: '\"\n> +\t\t\t << vFlipFmt.toString() << \"'\" << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n\nnice too!\n\n> +\n> +\t\t/*\n> +\t\t * Perform a transposition and make sure that nothing changes.\n> +\t\t * Transpositions are not implemented as sensors are not\n> +\t\t * expected to support this functionality.\n> +\t\t */\n> +\t\tbayerFmt = BayerFormat(BayerFormat::BGGR, 8, BayerFormat::None);\n> +\t\tBayerFormat transposeFmt = bayerFmt.transform(\n> +\t\t\tTransform::Transpose);\n> +\t\tif (transposeFmt.toString() != \"BGGR-8\") {\n> +\t\t\tcout << \"Transposition is not supported \"\n> +\t\t\t << \"format should be 'BGGR-8', got: '\"\n> +\t\t\t << transposeFmt.toString() << \"'\" << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n\nI would drop this last case as it is not used in practice, as you have\nsaid.\n\nMostly minors so please add\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n> +\n> +\t\treturn TestPass;\n> +\t}\n> +};\n> +\n> +TEST_REGISTER(BayerFormatTest);\n> +\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.25.1\n>","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 5F8C7C0F1A\n\tfor ;\n\tFri, 8 Jan 2021 11:46:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CC2EC635B0;\n\tFri, 8 Jan 2021 12:46:40 +0100 (CET)","from relay5-d.mail.gandi.net (relay5-d.mail.gandi.net\n\t[217.70.183.197])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id AD41463138\n\tfor ;\n\tFri, 8 Jan 2021 12:46:39 +0100 (CET)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay5-d.mail.gandi.net (Postfix) with ESMTPSA id A43661C0004;\n\tFri, 8 Jan 2021 11:46:38 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Fri, 8 Jan 2021 12:46:54 +0100","From":"Jacopo Mondi ","To":"Sebastian Fricke ","Message-ID":"<20210108114654.g63ock4nbtiwlhpc@uno.localdomain>","References":"<20201231155336.7058-1-sebastian.fricke.linux@gmail.com>\n\t<20201231155336.7058-5-sebastian.fricke.linux@gmail.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201231155336.7058-5-sebastian.fricke.linux@gmail.com>","Subject":"Re: [libcamera-devel] [PATCH v2 4/4] 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@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14517,"web_url":"https://patchwork.libcamera.org/comment/14517/","msgid":"","date":"2021-01-11T01:06:30","subject":"Re: [libcamera-devel] [PATCH v2 4/4] 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,\n\nThank you for the patch.\n\nOn Fri, Jan 08, 2021 at 12:46:54PM +0100, Jacopo Mondi wrote:\n> On Thu, Dec 31, 2020 at 04:53:36PM +0100, Sebastian Fricke wrote:\n> > Test all of the present methods including the newly implemented\n> > `fromV4L2PixelFormat`, as well as the new operators `==/!=`.\n> >\n> > Signed-off-by: Sebastian Fricke \n> > ---\n> > test/bayer_format.cpp | 202 ++++++++++++++++++++++++++++++++++++++++++\n> > test/meson.build | 1 +\n> > 2 files changed, 203 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..dd7aa8cb\n> > --- /dev/null\n> > +++ b/test/bayer_format.cpp\n> > @@ -0,0 +1,202 @@\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> > +#include \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> > +\tint run()\n> > +\t{\n> > +\t\t/*\n> > +\t\t * An empty Bayer format has to be invalid.\n> > +\t\t */\n> \n> Fits on 1 line (also other comments)\n> \n> > +\t\tBayerFormat bayerFmt = BayerFormat();\n> \n> Just\n> BayerFormat bayerFmt;\n> ?\n> \n> > +\t\tif (bayerFmt.isValid()) {\n> > +\t\t\tcout << \"An empty BayerFormat \"\n> \n> We have a mixed usage of cout/cerr in test/\n> But in new tests I would use cerr.\n> \n> > +\t\t\t << \"has to be invalid.\" << endl;\n\nI'd avoid splitting strings to make it easier to grep for error\nmessages. This means that it would be nice to shorten a few of the\nstrings below if possible.\n\n> > +\t\t\treturn TestFail;\n> > +\t\t}\n> > +\n> > +\t\t/*\n> > +\t\t * A correct Bayer format has to be valid.\n> > +\t\t */\n> > +\t\tbayerFmt = BayerFormat(BayerFormat::BGGR, 8, BayerFormat::None);\n> > +\t\tif (!bayerFmt.isValid()) {\n> > +\t\t\tcout << \"A correct BayerFormat \"\n> > +\t\t\t << \"has to be valid.\" << endl;\n> > +\t\t\treturn TestFail;\n> > +\t\t}\n> > +\n> > +\t\t/*\n> > +\t\t * Two bayer formats created with the same order and bit depth\n> > +\t\t * have to be equal.\n> > +\t\t */\n> > +\t\tbayerFmt = BayerFormat(BayerFormat::BGGR, 8, BayerFormat::None);\n> > +\t\tBayerFormat bayerFmtExpect = BayerFormat(BayerFormat::BGGR, 8,\n> > +\t\t\t\t\t\t\t BayerFormat::None);\n> > +\t\tif (bayerFmt != bayerFmtExpect) {\n> > +\t\t\tcout << \"Two BayerFormat object created with the same \"\n> > +\t\t\t << \"order and bitDepth must be equal.\" << endl;\n\nFor instance this could become\n\n\t\t\tcout << \"Comparison if identical formats failed\" << endl;\n\n> > +\t\t\treturn TestFail;\n> > +\t\t}\n> > +\n> > +\t\t/*\n> > +\t\t * Two Bayer formats created with the same order but with a\n> > +\t\t * different bitDepth are not equal.\n> > +\t\t */\n> > +\t\tbayerFmt = BayerFormat(BayerFormat::BGGR, 8, BayerFormat::None);\n> > +\t\tbayerFmtExpect = BayerFormat(BayerFormat::BGGR, 12,\n> > +\t\t\t\t\t BayerFormat::None);\n> > +\t\tif (bayerFmt == bayerFmtExpect) {\n> > +\t\t\tcout << \"Two BayerFormat object created with the same \"\n> > +\t\t\t << \"order, but different bit depths are not equal.\"\n> > +\t\t\t << endl;\n> > +\t\t\treturn TestFail;\n> > +\t\t}\n> > +\n> > +\t\t/*\n> > +\t\t * Create a Bayer format with a V4L2PixelFormat and check if we\n> > +\t\t * get the same format after converting back to the V4L2 Format.\n> > +\t\t */\n> > +\t\tV4L2PixelFormat v4l2FmtExpect = V4L2PixelFormat(\n> > +\t\t\tV4L2_PIX_FMT_SBGGR8);\n> > +\t\tbayerFmt = BayerFormat::fromV4L2PixelFormat(v4l2FmtExpect);\n> > +\t\tV4L2PixelFormat v4l2Fmt = bayerFmt.toV4L2PixelFormat();\n> > +\t\tif (v4l2Fmt != v4l2FmtExpect) {\n> > +\t\t\tcout << \"Expected: '\" << v4l2FmtExpect.toString()\n> > +\t\t\t << \"' got: '\" << v4l2Fmt.toString() << \"'\" << endl;\n> > +\t\t\treturn TestFail;\n> > +\t\t}\n> > +\n> > +\t\t/*\n> > +\t\t * Use a Bayer format that is not found in the mapping table\n> > +\t\t * and verify that no matching V4L2PixelFormat is found.\n> > +\t\t */\n> > +\t\tv4l2FmtExpect = V4L2PixelFormat();\n> > +\t\tbayerFmt = BayerFormat(BayerFormat::BGGR, 24,\n> > +\t\t\t\t BayerFormat::None);\n> > +\t\tv4l2Fmt = bayerFmt.toV4L2PixelFormat();\n> > +\t\tif (v4l2Fmt != v4l2FmtExpect) {\n> > +\t\t\tcout << \"Expected: empty V4L2PixelFormat got: '\"\n> > +\t\t\t << v4l2Fmt.toString() << \"'\" << endl;\n> > +\t\t\treturn TestFail;\n> > +\t\t}\n> \n> Mmm, not sure...\n> If such a format is ever added this will fail. And bayerFmt should not\n> be valid in first place, so accessing .toV4L2PixelFormat() is not of\n> much value... Do you think it's a valuable test case ?\n> \n> > +\n> > +\t\t/*\n> > +\t\t * Check if we get the expected Bayer format BGGR8\n> > +\t\t * when we convert the V4L2PixelFormat (V4L2_PIX_FMT_SBGGR8)\n> > +\t\t * to a Bayer format.\n> > +\t\t */\n> > +\t\tbayerFmtExpect = BayerFormat(BayerFormat::BGGR, 8,\n> > +\t\t\t\t\t BayerFormat::None);\n> > +\t\tv4l2Fmt = V4L2PixelFormat(V4L2_PIX_FMT_SBGGR8);\n> > +\t\tbayerFmt = BayerFormat::fromV4L2PixelFormat(v4l2Fmt);\n> > +\t\tif (bayerFmt != bayerFmtExpect) {\n> > +\t\t\tcout << \"Expected BayerFormat '\"\n> > +\t\t\t << bayerFmtExpect.toString() << \"',\"\n> > +\t\t\t << \"got: '\" << bayerFmt.toString() << \"'\" << endl;\n\nYou can squash the two string into \"', got: '\".\n\n> > +\t\t\treturn TestFail;\n> > +\t\t}\n> > +\n> > +\t\t/*\n> > +\t\t * Confirm that a V4L2PixelFormat that is not found in\n> > +\t\t * the conversion table, doesn't yield a Bayer format.\n> > +\t\t */\n> > +\t\tbayerFmtExpect = BayerFormat();\n> > +\t\tV4L2PixelFormat v4l2FmtUnknown = V4L2PixelFormat(\n> > +\t\t\tV4L2_PIX_FMT_BGRA444);\n> > +\t\tbayerFmt = BayerFormat::fromV4L2PixelFormat(v4l2FmtUnknown);\n> > +\t\tif (bayerFmt != bayerFmtExpect) {\n\nI would simply check if (bayerFmt.isValid()).\n\n> > +\t\t\tcout << \"Expected empty BayerFormat got: '\"\n> > +\t\t\t << bayerFmt.toString() << \"'\" << endl;\n> > +\t\t\treturn TestFail;\n> > +\t\t}\n> > +\n> > +\t\t/*\n> > +\t\t * Test if a valid Bayer format can be converted to a\n> > +\t\t * string representation.\n> > +\t\t */\n> > +\t\tbayerFmt = BayerFormat(BayerFormat::BGGR, 8, BayerFormat::None);\n> > +\t\tif (bayerFmt.toString() != \"BGGR-8\") {\n> > +\t\t\tcout << \"String representation != 'BGGR8' (got: '\"\n\ns/BGGR8/BGGR-8/\n\n> > +\t\t\t << bayerFmt.toString() << \"' ) \" << endl;\n> > +\t\t\treturn TestFail;\n> > +\t\t}\n> > +\n> > +\t\t/*\n> > +\t\t * Determine if an empty Bayer format results in no\n> > +\t\t * string representation.\n> > +\t\t */\n> > +\t\tbayerFmt = BayerFormat();\n> > +\t\tif (bayerFmt.toString() != \"INVALID\") {\n> > +\t\t\tcout << \"String representation != 'INVALID' (got: '\"\n> > +\t\t\t << bayerFmt.toString() << \"' ) \" << endl;\n> > +\t\t\treturn TestFail;\n> > +\t\t}\n> > +\n> > +\t\t/*\n> > +\t\t * Perform a horizontal Flip and make sure that the\n> > +\t\t * order is adjusted accordingly.\n> > +\t\t */\n> > +\t\tbayerFmt = BayerFormat(BayerFormat::BGGR, 8, BayerFormat::None);\n> > +\t\tbayerFmtExpect = BayerFormat(BayerFormat::GBRG, 8,\n> > +\t\t\t\t\t BayerFormat::None);\n> > +\t\tBayerFormat hFlipFmt = bayerFmt.transform(Transform::HFlip);\n> > +\t\tif (hFlipFmt != bayerFmtExpect) {\n> > +\t\t\tcout << \"horizontal flip of 'BGGR-8' \"\n> > +\t\t\t << \"should result in '\"\n> > +\t\t\t << bayerFmtExpect.toString() << \"', got: '\"\n> > +\t\t\t << hFlipFmt.toString() << \"'\" << endl;\n> > +\t\t\treturn TestFail;\n> > +\t\t}\n> \n> nice !\n> \n> > +\n> > +\t\t/*\n> > +\t\t * Perform a vertical Flip and make sure that\n> > +\t\t * the order is adjusted accordingly.\n> > +\t\t */\n> > +\t\tbayerFmt = BayerFormat(BayerFormat::BGGR, 8, BayerFormat::None);\n> > +\t\tbayerFmtExpect = BayerFormat(BayerFormat::GRBG, 8,\n> > +\t\t\t\t\t BayerFormat::None);\n> > +\t\tBayerFormat vFlipFmt = bayerFmt.transform(Transform::VFlip);\n> > +\t\tif (vFlipFmt != bayerFmtExpect) {\n> > +\t\t\tcout << \"vertical flip of 'BGGR-8' \"\n> > +\t\t\t << \"should result in '\"\n> > +\t\t\t << bayerFmtExpect.toString() << \"', got: '\"\n> > +\t\t\t << vFlipFmt.toString() << \"'\" << endl;\n> > +\t\t\treturn TestFail;\n> > +\t\t}\n> \n> nice too!\n> \n> > +\n> > +\t\t/*\n> > +\t\t * Perform a transposition and make sure that nothing changes.\n> > +\t\t * Transpositions are not implemented as sensors are not\n> > +\t\t * expected to support this functionality.\n> > +\t\t */\n> > +\t\tbayerFmt = BayerFormat(BayerFormat::BGGR, 8, BayerFormat::None);\n> > +\t\tBayerFormat transposeFmt = bayerFmt.transform(\n> > +\t\t\tTransform::Transpose);\n> > +\t\tif (transposeFmt.toString() != \"BGGR-8\") {\n> > +\t\t\tcout << \"Transposition is not supported \"\n> > +\t\t\t << \"format should be 'BGGR-8', got: '\"\n> > +\t\t\t << transposeFmt.toString() << \"'\" << endl;\n> > +\t\t\treturn TestFail;\n> > +\t\t}\n> \n> I would drop this last case as it is not used in practice, as you have\n> said.\n\nEspecially given that the test passes with the missing implementation,\nwhile it should fail.\n\n> Mostly minors so please add\n> Reviewed-by: Jacopo Mondi \n\nReviewed-by: Laurent Pinchart \n\n> > +\n> > +\t\treturn TestPass;\n> > +\t}\n> > +};\n> > +\n> > +TEST_REGISTER(BayerFormatTest);\n> > +\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 7BD60BD808\n\tfor ;\n\tMon, 11 Jan 2021 01:06:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D05F36809E;\n\tMon, 11 Jan 2021 02:06:47 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 689F760317\n\tfor ;\n\tMon, 11 Jan 2021 02:06:45 +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 80630EC;\n\tMon, 11 Jan 2021 02:06:44 +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=\"RsFUx7M1\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1610327204;\n\tbh=cevqm043MEhJ0x4INUtuK10Onri0Kp7hASp4aurGqMk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=RsFUx7M1pq1X2bWG/HzEMfipH1X55mP9uWR2/4ox88lWOhw1TdwD4OvWBxJVZ90s5\n\tW2rTjWIgR2qaneyjeY/DCZ7ezeNLTzG+znnE7aa/2LBgzjCqAV9bxFZNwXf5tIUMvL\n\ty+YieIJSsVCBo2aXPjw7ANaIkAKXFS7DV1fAt1YU=","Date":"Mon, 11 Jan 2021 03:06:30 +0200","From":"Laurent Pinchart ","To":"Sebastian Fricke ","Message-ID":"","References":"<20201231155336.7058-1-sebastian.fricke.linux@gmail.com>\n\t<20201231155336.7058-5-sebastian.fricke.linux@gmail.com>\n\t<20210108114654.g63ock4nbtiwlhpc@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210108114654.g63ock4nbtiwlhpc@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v2 4/4] 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@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14536,"web_url":"https://patchwork.libcamera.org/comment/14536/","msgid":"","date":"2021-01-12T08:53:54","subject":"Re: [libcamera-devel] [PATCH v2 4/4] 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 this patch set. I think it's excellent to have some unit\ntests for the BayerFormat class, especially as the original author of\nthe class should probably have done it... :)\n\nJust one comment/question below:\n\nOn Mon, 11 Jan 2021 at 01:06, Laurent Pinchart\n wrote:\n>\n> Hi Sebastian,\n>\n> Thank you for the patch.\n>\n> On Fri, Jan 08, 2021 at 12:46:54PM +0100, Jacopo Mondi wrote:\n> > On Thu, Dec 31, 2020 at 04:53:36PM +0100, Sebastian Fricke wrote:\n> > > Test all of the present methods including the newly implemented\n> > > `fromV4L2PixelFormat`, as well as the new operators `==/!=`.\n> > >\n> > > Signed-off-by: Sebastian Fricke \n> > > ---\n> > > test/bayer_format.cpp | 202 ++++++++++++++++++++++++++++++++++++++++++\n> > > test/meson.build | 1 +\n> > > 2 files changed, 203 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..dd7aa8cb\n> > > --- /dev/null\n> > > +++ b/test/bayer_format.cpp\n> > > @@ -0,0 +1,202 @@\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> > > +#include \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> > > + * An empty Bayer format has to be invalid.\n> > > + */\n> >\n> > Fits on 1 line (also other comments)\n> >\n> > > + BayerFormat bayerFmt = BayerFormat();\n> >\n> > Just\n> > BayerFormat bayerFmt;\n> > ?\n> >\n> > > + if (bayerFmt.isValid()) {\n> > > + cout << \"An empty BayerFormat \"\n> >\n> > We have a mixed usage of cout/cerr in test/\n> > But in new tests I would use cerr.\n> >\n> > > + << \"has to be invalid.\" << endl;\n>\n> I'd avoid splitting strings to make it easier to grep for error\n> messages. This means that it would be nice to shorten a few of the\n> strings below if possible.\n>\n> > > + return TestFail;\n> > > + }\n> > > +\n> > > + /*\n> > > + * A correct Bayer format has to be valid.\n> > > + */\n> > > + bayerFmt = BayerFormat(BayerFormat::BGGR, 8, BayerFormat::None);\n> > > + if (!bayerFmt.isValid()) {\n> > > + cout << \"A correct BayerFormat \"\n> > > + << \"has to be valid.\" << endl;\n> > > + return TestFail;\n> > > + }\n> > > +\n> > > + /*\n> > > + * Two bayer formats created with the same order and bit depth\n> > > + * have to be equal.\n> > > + */\n> > > + bayerFmt = BayerFormat(BayerFormat::BGGR, 8, BayerFormat::None);\n> > > + BayerFormat bayerFmtExpect = BayerFormat(BayerFormat::BGGR, 8,\n> > > + BayerFormat::None);\n> > > + if (bayerFmt != bayerFmtExpect) {\n> > > + cout << \"Two BayerFormat object created with the same \"\n> > > + << \"order and bitDepth must be equal.\" << endl;\n>\n> For instance this could become\n>\n> cout << \"Comparison if identical formats failed\" << endl;\n>\n> > > + return TestFail;\n> > > + }\n> > > +\n> > > + /*\n> > > + * Two Bayer formats created with the same order but with a\n> > > + * different bitDepth are not equal.\n> > > + */\n> > > + bayerFmt = BayerFormat(BayerFormat::BGGR, 8, BayerFormat::None);\n> > > + bayerFmtExpect = BayerFormat(BayerFormat::BGGR, 12,\n> > > + BayerFormat::None);\n> > > + if (bayerFmt == bayerFmtExpect) {\n> > > + cout << \"Two BayerFormat object created with the same \"\n> > > + << \"order, but different bit depths are not equal.\"\n> > > + << endl;\n> > > + return TestFail;\n> > > + }\n> > > +\n> > > + /*\n> > > + * Create a Bayer format with a V4L2PixelFormat and check if we\n> > > + * get the same format after converting back to the V4L2 Format.\n> > > + */\n> > > + V4L2PixelFormat v4l2FmtExpect = V4L2PixelFormat(\n> > > + V4L2_PIX_FMT_SBGGR8);\n> > > + bayerFmt = BayerFormat::fromV4L2PixelFormat(v4l2FmtExpect);\n> > > + V4L2PixelFormat v4l2Fmt = bayerFmt.toV4L2PixelFormat();\n> > > + if (v4l2Fmt != v4l2FmtExpect) {\n> > > + cout << \"Expected: '\" << v4l2FmtExpect.toString()\n> > > + << \"' got: '\" << v4l2Fmt.toString() << \"'\" << endl;\n> > > + return TestFail;\n> > > + }\n> > > +\n> > > + /*\n> > > + * Use a Bayer format that is not found in the mapping table\n> > > + * and verify that no matching V4L2PixelFormat is found.\n> > > + */\n> > > + v4l2FmtExpect = V4L2PixelFormat();\n> > > + bayerFmt = BayerFormat(BayerFormat::BGGR, 24,\n> > > + BayerFormat::None);\n> > > + v4l2Fmt = bayerFmt.toV4L2PixelFormat();\n> > > + if (v4l2Fmt != v4l2FmtExpect) {\n> > > + cout << \"Expected: empty V4L2PixelFormat got: '\"\n> > > + << v4l2Fmt.toString() << \"'\" << endl;\n> > > + return TestFail;\n> > > + }\n> >\n> > Mmm, not sure...\n> > If such a format is ever added this will fail. And bayerFmt should not\n> > be valid in first place, so accessing .toV4L2PixelFormat() is not of\n> > much value... Do you think it's a valuable test case ?\n> >\n> > > +\n> > > + /*\n> > > + * Check if we get the expected Bayer format BGGR8\n> > > + * when we convert the V4L2PixelFormat (V4L2_PIX_FMT_SBGGR8)\n> > > + * to a Bayer format.\n> > > + */\n> > > + bayerFmtExpect = BayerFormat(BayerFormat::BGGR, 8,\n> > > + BayerFormat::None);\n> > > + v4l2Fmt = V4L2PixelFormat(V4L2_PIX_FMT_SBGGR8);\n> > > + bayerFmt = BayerFormat::fromV4L2PixelFormat(v4l2Fmt);\n> > > + if (bayerFmt != bayerFmtExpect) {\n> > > + cout << \"Expected BayerFormat '\"\n> > > + << bayerFmtExpect.toString() << \"',\"\n> > > + << \"got: '\" << bayerFmt.toString() << \"'\" << endl;\n>\n> You can squash the two string into \"', got: '\".\n>\n> > > + return TestFail;\n> > > + }\n> > > +\n> > > + /*\n> > > + * Confirm that a V4L2PixelFormat that is not found in\n> > > + * the conversion table, doesn't yield a Bayer format.\n> > > + */\n> > > + bayerFmtExpect = BayerFormat();\n> > > + V4L2PixelFormat v4l2FmtUnknown = V4L2PixelFormat(\n> > > + V4L2_PIX_FMT_BGRA444);\n> > > + bayerFmt = BayerFormat::fromV4L2PixelFormat(v4l2FmtUnknown);\n> > > + if (bayerFmt != bayerFmtExpect) {\n>\n> I would simply check if (bayerFmt.isValid()).\n>\n> > > + cout << \"Expected empty BayerFormat got: '\"\n> > > + << bayerFmt.toString() << \"'\" << endl;\n> > > + return TestFail;\n> > > + }\n> > > +\n> > > + /*\n> > > + * Test if a valid Bayer format can be converted to a\n> > > + * string representation.\n> > > + */\n> > > + bayerFmt = BayerFormat(BayerFormat::BGGR, 8, BayerFormat::None);\n> > > + if (bayerFmt.toString() != \"BGGR-8\") {\n> > > + cout << \"String representation != 'BGGR8' (got: '\"\n>\n> s/BGGR8/BGGR-8/\n>\n> > > + << bayerFmt.toString() << \"' ) \" << endl;\n> > > + return TestFail;\n> > > + }\n> > > +\n> > > + /*\n> > > + * Determine if an empty Bayer format results in no\n> > > + * string representation.\n> > > + */\n> > > + bayerFmt = BayerFormat();\n> > > + if (bayerFmt.toString() != \"INVALID\") {\n> > > + cout << \"String representation != 'INVALID' (got: '\"\n> > > + << bayerFmt.toString() << \"' ) \" << endl;\n> > > + return TestFail;\n> > > + }\n> > > +\n> > > + /*\n> > > + * Perform a horizontal Flip and make sure that the\n> > > + * order is adjusted accordingly.\n> > > + */\n> > > + bayerFmt = BayerFormat(BayerFormat::BGGR, 8, BayerFormat::None);\n> > > + bayerFmtExpect = BayerFormat(BayerFormat::GBRG, 8,\n> > > + BayerFormat::None);\n> > > + BayerFormat hFlipFmt = bayerFmt.transform(Transform::HFlip);\n> > > + if (hFlipFmt != bayerFmtExpect) {\n> > > + cout << \"horizontal flip of 'BGGR-8' \"\n> > > + << \"should result in '\"\n> > > + << bayerFmtExpect.toString() << \"', got: '\"\n> > > + << hFlipFmt.toString() << \"'\" << endl;\n> > > + return TestFail;\n> > > + }\n> >\n> > nice !\n> >\n> > > +\n> > > + /*\n> > > + * Perform a vertical Flip and make sure that\n> > > + * the order is adjusted accordingly.\n> > > + */\n> > > + bayerFmt = BayerFormat(BayerFormat::BGGR, 8, BayerFormat::None);\n> > > + bayerFmtExpect = BayerFormat(BayerFormat::GRBG, 8,\n> > > + BayerFormat::None);\n> > > + BayerFormat vFlipFmt = bayerFmt.transform(Transform::VFlip);\n> > > + if (vFlipFmt != bayerFmtExpect) {\n> > > + cout << \"vertical flip of 'BGGR-8' \"\n> > > + << \"should result in '\"\n> > > + << bayerFmtExpect.toString() << \"', got: '\"\n> > > + << vFlipFmt.toString() << \"'\" << endl;\n> > > + return TestFail;\n> > > + }\n> >\n> > nice too!\n> >\n> > > +\n> > > + /*\n> > > + * Perform a transposition and make sure that nothing changes.\n> > > + * Transpositions are not implemented as sensors are not\n> > > + * expected to support this functionality.\n> > > + */\n> > > + bayerFmt = BayerFormat(BayerFormat::BGGR, 8, BayerFormat::None);\n> > > + BayerFormat transposeFmt = bayerFmt.transform(\n> > > + Transform::Transpose);\n> > > + if (transposeFmt.toString() != \"BGGR-8\") {\n> > > + cout << \"Transposition is not supported \"\n> > > + << \"format should be 'BGGR-8', got: '\"\n> > > + << transposeFmt.toString() << \"'\" << endl;\n> > > + return TestFail;\n> > > + }\n> >\n> > I would drop this last case as it is not used in practice, as you have\n> > said.\n>\n> Especially given that the test passes with the missing implementation,\n> while it should fail.\n\nI'm not quite sure if I understand this, I'm under the impression that\nbeing able to transpose a BayerFormat is required to work correctly,\nand does not depend on whether a particular sensor supports it (which\nthey don't).\n\nAs such I'd quite like to keep this test, though I'd prefer to avoid\ntesting for equality by comparing strings. I'd also quite like to see\nanother transpose test but where the Bayer format does change (e.g.\nGRBG).\n\nBut I don't feel too strongly, and perhaps I've misunderstood, so\nplease add this regardless:\n\nReviewed-by: David Plowman \n\nThanks again and best regards\nDavid\n\n>\n> > Mostly minors so please add\n> > Reviewed-by: Jacopo Mondi \n>\n> Reviewed-by: Laurent Pinchart \n>\n> > > +\n> > > + return TestPass;\n> > > + }\n> > > +};\n> > > +\n> > > +TEST_REGISTER(BayerFormatTest);\n> > > +\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 5D60DBD808\n\tfor ;\n\tTue, 12 Jan 2021 08:54:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D05A3680CF;\n\tTue, 12 Jan 2021 09:54:07 +0100 (CET)","from mail-oi1-x232.google.com (mail-oi1-x232.google.com\n\t[IPv6:2607:f8b0:4864:20::232])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id F2B056010A\n\tfor ;\n\tTue, 12 Jan 2021 09:54:05 +0100 (CET)","by mail-oi1-x232.google.com with SMTP id q205so1560118oig.13\n\tfor ;\n\tTue, 12 Jan 2021 00:54:05 -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=\"mwP3Xu7v\"; 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=88X3lnpWQAyegH828q7rnWhXSKf9w8teadT9Egi0KdM=;\n\tb=mwP3Xu7vHvfotZg5fgtwqQUoY6OzmKIHxMfQ79ugDw2E53Erx0oz6LqbmHqYEBEzPK\n\tf30egAIqQTCAhWz9J9bdBXheqF3XSrt7JqmbaclGebA+wbeu00m4mAg+wj55WsnLoLvV\n\tKQZWjZOrhzd4f1cDuuwvLyJE/u39VxBqF2DE6AMHLOiZYCZ8BUTQkfic/uTtMCbVLlOq\n\tYTcG6tAN0ko7CaRakZYiYr2rkIrXPhMPtLHsP72tH22bP72HfZ2M9JDLKmlCcuvy4Jhu\n\t81mKYo9LvV+31sujIDS8ZlkUu3kWPMTIoA1KfBoBxJpH7nNEf5NMtmB1eKcURpnN5GQN\n\tZs3g==","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=88X3lnpWQAyegH828q7rnWhXSKf9w8teadT9Egi0KdM=;\n\tb=N9v+KTIkhG3JY1yT8QdgyuOOcNEVb8oh/ptYvD1gy8lZkWzvM4HY+PHvZbH+Xkf59Z\n\tJC/lU4LY74lu4KmKSm4R8SmPqACXJ9oR+gMqjQBN+7ii47pArEYe9lLQi5UKej5isASQ\n\t3P/kMtIiLdo8l7d9ExwPiB3VocXOcrp91nlO+LQOVl6nWAMhR0cVU2Z89QOJbV3sxPWQ\n\tlaNUghmhc+XIerbhjR1nDlM1tycGwjr1yMdLf1CfbSC5gOvAG98zfPXnU/aeIrfLQddk\n\tQKfcJ3qHo7FTDel0++EpKUGsaiF11FG4a//npx9kqlRbQhZunnI0a4LNJDOiX7sox6ds\n\tsfLg==","X-Gm-Message-State":"AOAM530j/jTCovV1yfKRwhy2oixrMqesqechjQhzBl5In1I4E8ewMl4a\n\tEodZnKkNXarlkzfjZwFsDNMqz1yOQ7HH7logDno4Xg==","X-Google-Smtp-Source":"ABdhPJyex2nZTscJpOd1DNt03BJqlCeYYxqesXjE4GDtexataT8J/yp5grTiGFN7SEIx7lUPPjoaHS6jMG2HxxkjEws=","X-Received":"by 2002:aca:4dc3:: with SMTP id\n\ta186mr1666506oib.107.1610441644560; \n\tTue, 12 Jan 2021 00:54:04 -0800 (PST)","MIME-Version":"1.0","References":"<20201231155336.7058-1-sebastian.fricke.linux@gmail.com>\n\t<20201231155336.7058-5-sebastian.fricke.linux@gmail.com>\n\t<20210108114654.g63ock4nbtiwlhpc@uno.localdomain>\n\t","In-Reply-To":"","From":"David Plowman ","Date":"Tue, 12 Jan 2021 08:53:54 +0000","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH v2 4/4] 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":14736,"web_url":"https://patchwork.libcamera.org/comment/14736/","msgid":"<20210125062206.e2i53mur5zv55gyp@basti-TUXEDO-Book-XA1510>","date":"2021-01-25T06:22:06","subject":"Re: [libcamera-devel] [PATCH v2 4/4] 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 08.01.2021 12:46, Jacopo Mondi wrote:\n>Hi Sebastian,\n\nHey Jacopo,\n\nThank you for your review.\n\n>\n>On Thu, Dec 31, 2020 at 04:53:36PM +0100, Sebastian Fricke wrote:\n>> Test all of the present methods including the newly implemented\n>> `fromV4L2PixelFormat`, as well as the new operators `==/!=`.\n>>\n>> Signed-off-by: Sebastian Fricke \n>> ---\n>> test/bayer_format.cpp | 202 ++++++++++++++++++++++++++++++++++++++++++\n>> test/meson.build | 1 +\n>> 2 files changed, 203 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..dd7aa8cb\n>> --- /dev/null\n>> +++ b/test/bayer_format.cpp\n>> @@ -0,0 +1,202 @@\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>> +#include \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>> +\tint run()\n>> +\t{\n>> +\t\t/*\n>> +\t\t * An empty Bayer format has to be invalid.\n>> +\t\t */\n>\n>Fits on 1 line (also other comments)\n>\n>> +\t\tBayerFormat bayerFmt = BayerFormat();\n>\n>Just\n> BayerFormat bayerFmt;\n>?\n>\n>> +\t\tif (bayerFmt.isValid()) {\n>> +\t\t\tcout << \"An empty BayerFormat \"\n>\n>We have a mixed usage of cout/cerr in test/\n>But in new tests I would use cerr.\n\nFixed in my next version.\n\n>\n>> +\t\t\t << \"has to be invalid.\" << endl;\n>> +\t\t\treturn TestFail;\n>> +\t\t}\n>> +\n>> +\t\t/*\n>> +\t\t * A correct Bayer format has to be valid.\n>> +\t\t */\n>> +\t\tbayerFmt = BayerFormat(BayerFormat::BGGR, 8, BayerFormat::None);\n>> +\t\tif (!bayerFmt.isValid()) {\n>> +\t\t\tcout << \"A correct BayerFormat \"\n>> +\t\t\t << \"has to be valid.\" << endl;\n>> +\t\t\treturn TestFail;\n>> +\t\t}\n>> +\n>> +\t\t/*\n>> +\t\t * Two bayer formats created with the same order and bit depth\n>> +\t\t * have to be equal.\n>> +\t\t */\n>> +\t\tbayerFmt = BayerFormat(BayerFormat::BGGR, 8, BayerFormat::None);\n>> +\t\tBayerFormat bayerFmtExpect = BayerFormat(BayerFormat::BGGR, 8,\n>> +\t\t\t\t\t\t\t BayerFormat::None);\n>> +\t\tif (bayerFmt != bayerFmtExpect) {\n>> +\t\t\tcout << \"Two BayerFormat object created with the same \"\n>> +\t\t\t << \"order and bitDepth must be equal.\" << endl;\n>> +\t\t\treturn TestFail;\n>> +\t\t}\n>> +\n>> +\t\t/*\n>> +\t\t * Two Bayer formats created with the same order but with a\n>> +\t\t * different bitDepth are not equal.\n>> +\t\t */\n>> +\t\tbayerFmt = BayerFormat(BayerFormat::BGGR, 8, BayerFormat::None);\n>> +\t\tbayerFmtExpect = BayerFormat(BayerFormat::BGGR, 12,\n>> +\t\t\t\t\t BayerFormat::None);\n>> +\t\tif (bayerFmt == bayerFmtExpect) {\n>> +\t\t\tcout << \"Two BayerFormat object created with the same \"\n>> +\t\t\t << \"order, but different bit depths are not equal.\"\n>> +\t\t\t << endl;\n>> +\t\t\treturn TestFail;\n>> +\t\t}\n>> +\n>> +\t\t/*\n>> +\t\t * Create a Bayer format with a V4L2PixelFormat and check if we\n>> +\t\t * get the same format after converting back to the V4L2 Format.\n>> +\t\t */\n>> +\t\tV4L2PixelFormat v4l2FmtExpect = V4L2PixelFormat(\n>> +\t\t\tV4L2_PIX_FMT_SBGGR8);\n>> +\t\tbayerFmt = BayerFormat::fromV4L2PixelFormat(v4l2FmtExpect);\n>> +\t\tV4L2PixelFormat v4l2Fmt = bayerFmt.toV4L2PixelFormat();\n>> +\t\tif (v4l2Fmt != v4l2FmtExpect) {\n>> +\t\t\tcout << \"Expected: '\" << v4l2FmtExpect.toString()\n>> +\t\t\t << \"' got: '\" << v4l2Fmt.toString() << \"'\" << endl;\n>> +\t\t\treturn TestFail;\n>> +\t\t}\n>> +\n>> +\t\t/*\n>> +\t\t * Use a Bayer format that is not found in the mapping table\n>> +\t\t * and verify that no matching V4L2PixelFormat is found.\n>> +\t\t */\n>> +\t\tv4l2FmtExpect = V4L2PixelFormat();\n>> +\t\tbayerFmt = BayerFormat(BayerFormat::BGGR, 24,\n>> +\t\t\t\t BayerFormat::None);\n>> +\t\tv4l2Fmt = bayerFmt.toV4L2PixelFormat();\n>> +\t\tif (v4l2Fmt != v4l2FmtExpect) {\n>> +\t\t\tcout << \"Expected: empty V4L2PixelFormat got: '\"\n>> +\t\t\t << v4l2Fmt.toString() << \"'\" << endl;\n>> +\t\t\treturn TestFail;\n>> +\t\t}\n>\n>Mmm, not sure...\n>If such a format is ever added this will fail. And bayerFmt should not\n>be valid in first place, so accessing .toV4L2PixelFormat() is not of\n>much value... Do you think it's a valuable test case ?\n\nMy goal was to prove that using invalid input for the function results\nin an empty result. In my new version, I have replaced this imaginary\ncase with a simple check if the usage of this static member function on\nan empty BayerFormat results in an empty V4L2PixelFormat.\n\n>\n>> +\n>> +\t\t/*\n>> +\t\t * Check if we get the expected Bayer format BGGR8\n>> +\t\t * when we convert the V4L2PixelFormat (V4L2_PIX_FMT_SBGGR8)\n>> +\t\t * to a Bayer format.\n>> +\t\t */\n>> +\t\tbayerFmtExpect = BayerFormat(BayerFormat::BGGR, 8,\n>> +\t\t\t\t\t BayerFormat::None);\n>> +\t\tv4l2Fmt = V4L2PixelFormat(V4L2_PIX_FMT_SBGGR8);\n>> +\t\tbayerFmt = BayerFormat::fromV4L2PixelFormat(v4l2Fmt);\n>> +\t\tif (bayerFmt != bayerFmtExpect) {\n>> +\t\t\tcout << \"Expected BayerFormat '\"\n>> +\t\t\t << bayerFmtExpect.toString() << \"',\"\n>> +\t\t\t << \"got: '\" << bayerFmt.toString() << \"'\" << endl;\n>> +\t\t\treturn TestFail;\n>> +\t\t}\n>> +\n>> +\t\t/*\n>> +\t\t * Confirm that a V4L2PixelFormat that is not found in\n>> +\t\t * the conversion table, doesn't yield a Bayer format.\n>> +\t\t */\n>> +\t\tbayerFmtExpect = BayerFormat();\n>> +\t\tV4L2PixelFormat v4l2FmtUnknown = V4L2PixelFormat(\n>> +\t\t\tV4L2_PIX_FMT_BGRA444);\n>> +\t\tbayerFmt = BayerFormat::fromV4L2PixelFormat(v4l2FmtUnknown);\n>> +\t\tif (bayerFmt != bayerFmtExpect) {\n>> +\t\t\tcout << \"Expected empty BayerFormat got: '\"\n>> +\t\t\t << bayerFmt.toString() << \"'\" << endl;\n>> +\t\t\treturn TestFail;\n>> +\t\t}\n>> +\n>> +\t\t/*\n>> +\t\t * Test if a valid Bayer format can be converted to a\n>> +\t\t * string representation.\n>> +\t\t */\n>> +\t\tbayerFmt = BayerFormat(BayerFormat::BGGR, 8, BayerFormat::None);\n>> +\t\tif (bayerFmt.toString() != \"BGGR-8\") {\n>> +\t\t\tcout << \"String representation != 'BGGR8' (got: '\"\n>> +\t\t\t << bayerFmt.toString() << \"' ) \" << endl;\n>> +\t\t\treturn TestFail;\n>> +\t\t}\n>> +\n>> +\t\t/*\n>> +\t\t * Determine if an empty Bayer format results in no\n>> +\t\t * string representation.\n>> +\t\t */\n>> +\t\tbayerFmt = BayerFormat();\n>> +\t\tif (bayerFmt.toString() != \"INVALID\") {\n>> +\t\t\tcout << \"String representation != 'INVALID' (got: '\"\n>> +\t\t\t << bayerFmt.toString() << \"' ) \" << endl;\n>> +\t\t\treturn TestFail;\n>> +\t\t}\n>> +\n>> +\t\t/*\n>> +\t\t * Perform a horizontal Flip and make sure that the\n>> +\t\t * order is adjusted accordingly.\n>> +\t\t */\n>> +\t\tbayerFmt = BayerFormat(BayerFormat::BGGR, 8, BayerFormat::None);\n>> +\t\tbayerFmtExpect = BayerFormat(BayerFormat::GBRG, 8,\n>> +\t\t\t\t\t BayerFormat::None);\n>> +\t\tBayerFormat hFlipFmt = bayerFmt.transform(Transform::HFlip);\n>> +\t\tif (hFlipFmt != bayerFmtExpect) {\n>> +\t\t\tcout << \"horizontal flip of 'BGGR-8' \"\n>> +\t\t\t << \"should result in '\"\n>> +\t\t\t << bayerFmtExpect.toString() << \"', got: '\"\n>> +\t\t\t << hFlipFmt.toString() << \"'\" << endl;\n>> +\t\t\treturn TestFail;\n>> +\t\t}\n>\n>nice !\n>\n>> +\n>> +\t\t/*\n>> +\t\t * Perform a vertical Flip and make sure that\n>> +\t\t * the order is adjusted accordingly.\n>> +\t\t */\n>> +\t\tbayerFmt = BayerFormat(BayerFormat::BGGR, 8, BayerFormat::None);\n>> +\t\tbayerFmtExpect = BayerFormat(BayerFormat::GRBG, 8,\n>> +\t\t\t\t\t BayerFormat::None);\n>> +\t\tBayerFormat vFlipFmt = bayerFmt.transform(Transform::VFlip);\n>> +\t\tif (vFlipFmt != bayerFmtExpect) {\n>> +\t\t\tcout << \"vertical flip of 'BGGR-8' \"\n>> +\t\t\t << \"should result in '\"\n>> +\t\t\t << bayerFmtExpect.toString() << \"', got: '\"\n>> +\t\t\t << vFlipFmt.toString() << \"'\" << endl;\n>> +\t\t\treturn TestFail;\n>> +\t\t}\n>\n>nice too!\n>\n>> +\n>> +\t\t/*\n>> +\t\t * Perform a transposition and make sure that nothing changes.\n>> +\t\t * Transpositions are not implemented as sensors are not\n>> +\t\t * expected to support this functionality.\n>> +\t\t */\n>> +\t\tbayerFmt = BayerFormat(BayerFormat::BGGR, 8, BayerFormat::None);\n>> +\t\tBayerFormat transposeFmt = bayerFmt.transform(\n>> +\t\t\tTransform::Transpose);\n>> +\t\tif (transposeFmt.toString() != \"BGGR-8\") {\n>> +\t\t\tcout << \"Transposition is not supported \"\n>> +\t\t\t << \"format should be 'BGGR-8', got: '\"\n>> +\t\t\t << transposeFmt.toString() << \"'\" << endl;\n>> +\t\t\treturn TestFail;\n>> +\t\t}\n>\n>I would drop this last case as it is not used in practice, as you have\n>said.\n\nI will comment on this discussion within David's review.\n\n>\n>Mostly minors so please add\n>Reviewed-by: Jacopo Mondi \n>\n>Thanks\n> j\n\nThank you and Greetings,\nSebastian\n\n>\n>> +\n>> +\t\treturn TestPass;\n>> +\t}\n>> +};\n>> +\n>> +TEST_REGISTER(BayerFormatTest);\n>> +\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.25.1\n>>","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 8DD88C0F2B\n\tfor ;\n\tMon, 25 Jan 2021 09:33:02 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D131A682B5;\n\tMon, 25 Jan 2021 10:33:01 +0100 (CET)","from mail-wm1-x32b.google.com (mail-wm1-x32b.google.com\n\t[IPv6:2a00:1450:4864:20::32b])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 570EA6030B\n\tfor ;\n\tMon, 25 Jan 2021 07:22:09 +0100 (CET)","by mail-wm1-x32b.google.com with SMTP id u14so2883490wml.4\n\tfor ;\n\tSun, 24 Jan 2021 22:22:09 -0800 (PST)","from localhost\n\t(p200300d1ff07ec002b02c4c10cf04b20.dip0.t-ipconnect.de.\n\t[2003:d1:ff07:ec00:2b02:c4c1:cf0:4b20])\n\tby smtp.gmail.com with ESMTPSA id\n\tn6sm19339873wmi.23.2021.01.24.22.22.07\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tSun, 24 Jan 2021 22:22:08 -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=\"lXUP8ZDL\"; 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=zzRTAdEctruFGlMsQ2RIpwp/7TOtn9gtGHakdbXTGVk=;\n\tb=lXUP8ZDLDRWTlFG46kLf5z/EU8N1jJJzUz3NtgTxLsUY2dICJgr8uIRdkg++3mf6aW\n\tj6duG+cqXiGALRLJQ/qHKe8KhrAViTS6tcpLn7QACyZwfkotxmsVZ1BiLLZmbnWUgJCh\n\tg7RG1OHskOb+SneNTngJRsMWHDQYi8pepms3PcLw5UyQ+lWRo/Ku2wJA2ZTus2RuWnBT\n\tsmJWbB01XijBlZsu+UV7gUCx8tVSFbscXGQRXZrWQi74jTgWRwXxwGi+ueOs/9J1k95I\n\tJak4Uq5J/QVQOld8Bn4uGbP1X0QJijXrCcuj1A0rrJ4B2kH+uzI3ogYkr5LBoyIRIpJb\n\tJxxQ==","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=zzRTAdEctruFGlMsQ2RIpwp/7TOtn9gtGHakdbXTGVk=;\n\tb=EHhePlkExEwTqGj5Ysy/+gtzIDeezOtLAhvmRsGN7zdtu4IrpzVzFfFkFa+vqPGAOB\n\t+JW6LPMqBjFavux+t0H76/VAHRPK1GoxNh1qmkGFohDfX950alroaUEXsiTz7X6ZMFSF\n\tp6Cl2sC9pidploguFQEc6lClVje43FeNg97yR5niYvPwKwp94qWikXVhAC1iWDxv6JHc\n\tkc2fRN2et1Hs9u1DfVIxjxPBdbHuooen/5cftoeP2ezgXaASfDIYj0lQWO8+j2lYbRMy\n\tVoOcQ19JfnV8f5weL0FCVwKX7wuOsuBMofVfIdjJa/tK+84WqU5R6aZiKfYDU4dQEwHg\n\tgTuA==","X-Gm-Message-State":"AOAM532AOCC+pymejTHd7pObKf1Ir2zOEK0UXKCOeWg9JlhJnkmfRj1U\n\tSRM+MrkFf1XSTTg7sRKRtmqcmF564q4=","X-Google-Smtp-Source":"ABdhPJx2EdJ/7uQzOzB4TFGvnozIzgoTMWhDy5MVd59JbsCHK1vx1/KSd8iEjaooco6a/Zg+9bxI3g==","X-Received":"by 2002:a7b:c5cc:: with SMTP id n12mr629130wmk.123.1611555728996;\n\tSun, 24 Jan 2021 22:22:08 -0800 (PST)","Date":"Mon, 25 Jan 2021 07:22:06 +0100","From":"Sebastian Fricke ","To":"Jacopo Mondi ","Message-ID":"<20210125062206.e2i53mur5zv55gyp@basti-TUXEDO-Book-XA1510>","References":"<20201231155336.7058-1-sebastian.fricke.linux@gmail.com>\n\t<20201231155336.7058-5-sebastian.fricke.linux@gmail.com>\n\t<20210108114654.g63ock4nbtiwlhpc@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210108114654.g63ock4nbtiwlhpc@uno.localdomain>","X-Mailman-Approved-At":"Mon, 25 Jan 2021 10:32:59 +0100","Subject":"Re: [libcamera-devel] [PATCH v2 4/4] 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@lists.libcamera.org","Content-Transfer-Encoding":"7bit","Content-Type":"text/plain; charset=\"us-ascii\"; Format=\"flowed\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14737,"web_url":"https://patchwork.libcamera.org/comment/14737/","msgid":"<20210125062403.zkp6fvlm4lpnku3w@basti-TUXEDO-Book-XA1510>","date":"2021-01-25T06:24:03","subject":"Re: [libcamera-devel] [PATCH v2 4/4] 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 11.01.2021 03:06, Laurent Pinchart wrote:\n>Hi Sebastian,\n\nHey Laurent,\n\n>\n>Thank you for the patch.\n\nThank you for the review.\n\n>\n>On Fri, Jan 08, 2021 at 12:46:54PM +0100, Jacopo Mondi wrote:\n>> On Thu, Dec 31, 2020 at 04:53:36PM +0100, Sebastian Fricke wrote:\n>> > Test all of the present methods including the newly implemented\n>> > `fromV4L2PixelFormat`, as well as the new operators `==/!=`.\n>> >\n>> > Signed-off-by: Sebastian Fricke \n>> > ---\n>> > test/bayer_format.cpp | 202 ++++++++++++++++++++++++++++++++++++++++++\n>> > test/meson.build | 1 +\n>> > 2 files changed, 203 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..dd7aa8cb\n>> > --- /dev/null\n>> > +++ b/test/bayer_format.cpp\n>> > @@ -0,0 +1,202 @@\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>> > +#include \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>> > +\tint run()\n>> > +\t{\n>> > +\t\t/*\n>> > +\t\t * An empty Bayer format has to be invalid.\n>> > +\t\t */\n>>\n>> Fits on 1 line (also other comments)\n>>\n>> > +\t\tBayerFormat bayerFmt = BayerFormat();\n>>\n>> Just\n>> BayerFormat bayerFmt;\n>> ?\n>>\n>> > +\t\tif (bayerFmt.isValid()) {\n>> > +\t\t\tcout << \"An empty BayerFormat \"\n>>\n>> We have a mixed usage of cout/cerr in test/\n>> But in new tests I would use cerr.\n>>\n>> > +\t\t\t << \"has to be invalid.\" << endl;\n>\n>I'd avoid splitting strings to make it easier to grep for error\n>messages. This means that it would be nice to shorten a few of the\n>strings below if possible.\n\nI have tried my best to makes this possible in my next version, I hope\nyou will like it.\n\n>\n>> > +\t\t\treturn TestFail;\n>> > +\t\t}\n>> > +\n>> > +\t\t/*\n>> > +\t\t * A correct Bayer format has to be valid.\n>> > +\t\t */\n>> > +\t\tbayerFmt = BayerFormat(BayerFormat::BGGR, 8, BayerFormat::None);\n>> > +\t\tif (!bayerFmt.isValid()) {\n>> > +\t\t\tcout << \"A correct BayerFormat \"\n>> > +\t\t\t << \"has to be valid.\" << endl;\n>> > +\t\t\treturn TestFail;\n>> > +\t\t}\n>> > +\n>> > +\t\t/*\n>> > +\t\t * Two bayer formats created with the same order and bit depth\n>> > +\t\t * have to be equal.\n>> > +\t\t */\n>> > +\t\tbayerFmt = BayerFormat(BayerFormat::BGGR, 8, BayerFormat::None);\n>> > +\t\tBayerFormat bayerFmtExpect = BayerFormat(BayerFormat::BGGR, 8,\n>> > +\t\t\t\t\t\t\t BayerFormat::None);\n>> > +\t\tif (bayerFmt != bayerFmtExpect) {\n>> > +\t\t\tcout << \"Two BayerFormat object created with the same \"\n>> > +\t\t\t << \"order and bitDepth must be equal.\" << endl;\n>\n>For instance this could become\n>\n>\t\t\tcout << \"Comparison if identical formats failed\" << endl;\n>\n>> > +\t\t\treturn TestFail;\n>> > +\t\t}\n>> > +\n>> > +\t\t/*\n>> > +\t\t * Two Bayer formats created with the same order but with a\n>> > +\t\t * different bitDepth are not equal.\n>> > +\t\t */\n>> > +\t\tbayerFmt = BayerFormat(BayerFormat::BGGR, 8, BayerFormat::None);\n>> > +\t\tbayerFmtExpect = BayerFormat(BayerFormat::BGGR, 12,\n>> > +\t\t\t\t\t BayerFormat::None);\n>> > +\t\tif (bayerFmt == bayerFmtExpect) {\n>> > +\t\t\tcout << \"Two BayerFormat object created with the same \"\n>> > +\t\t\t << \"order, but different bit depths are not equal.\"\n>> > +\t\t\t << endl;\n>> > +\t\t\treturn TestFail;\n>> > +\t\t}\n>> > +\n>> > +\t\t/*\n>> > +\t\t * Create a Bayer format with a V4L2PixelFormat and check if we\n>> > +\t\t * get the same format after converting back to the V4L2 Format.\n>> > +\t\t */\n>> > +\t\tV4L2PixelFormat v4l2FmtExpect = V4L2PixelFormat(\n>> > +\t\t\tV4L2_PIX_FMT_SBGGR8);\n>> > +\t\tbayerFmt = BayerFormat::fromV4L2PixelFormat(v4l2FmtExpect);\n>> > +\t\tV4L2PixelFormat v4l2Fmt = bayerFmt.toV4L2PixelFormat();\n>> > +\t\tif (v4l2Fmt != v4l2FmtExpect) {\n>> > +\t\t\tcout << \"Expected: '\" << v4l2FmtExpect.toString()\n>> > +\t\t\t << \"' got: '\" << v4l2Fmt.toString() << \"'\" << endl;\n>> > +\t\t\treturn TestFail;\n>> > +\t\t}\n>> > +\n>> > +\t\t/*\n>> > +\t\t * Use a Bayer format that is not found in the mapping table\n>> > +\t\t * and verify that no matching V4L2PixelFormat is found.\n>> > +\t\t */\n>> > +\t\tv4l2FmtExpect = V4L2PixelFormat();\n>> > +\t\tbayerFmt = BayerFormat(BayerFormat::BGGR, 24,\n>> > +\t\t\t\t BayerFormat::None);\n>> > +\t\tv4l2Fmt = bayerFmt.toV4L2PixelFormat();\n>> > +\t\tif (v4l2Fmt != v4l2FmtExpect) {\n>> > +\t\t\tcout << \"Expected: empty V4L2PixelFormat got: '\"\n>> > +\t\t\t << v4l2Fmt.toString() << \"'\" << endl;\n>> > +\t\t\treturn TestFail;\n>> > +\t\t}\n>>\n>> Mmm, not sure...\n>> If such a format is ever added this will fail. And bayerFmt should not\n>> be valid in first place, so accessing .toV4L2PixelFormat() is not of\n>> much value... Do you think it's a valuable test case ?\n>>\n>> > +\n>> > +\t\t/*\n>> > +\t\t * Check if we get the expected Bayer format BGGR8\n>> > +\t\t * when we convert the V4L2PixelFormat (V4L2_PIX_FMT_SBGGR8)\n>> > +\t\t * to a Bayer format.\n>> > +\t\t */\n>> > +\t\tbayerFmtExpect = BayerFormat(BayerFormat::BGGR, 8,\n>> > +\t\t\t\t\t BayerFormat::None);\n>> > +\t\tv4l2Fmt = V4L2PixelFormat(V4L2_PIX_FMT_SBGGR8);\n>> > +\t\tbayerFmt = BayerFormat::fromV4L2PixelFormat(v4l2Fmt);\n>> > +\t\tif (bayerFmt != bayerFmtExpect) {\n>> > +\t\t\tcout << \"Expected BayerFormat '\"\n>> > +\t\t\t << bayerFmtExpect.toString() << \"',\"\n>> > +\t\t\t << \"got: '\" << bayerFmt.toString() << \"'\" << endl;\n>\n>You can squash the two string into \"', got: '\".\n>\n>> > +\t\t\treturn TestFail;\n>> > +\t\t}\n>> > +\n>> > +\t\t/*\n>> > +\t\t * Confirm that a V4L2PixelFormat that is not found in\n>> > +\t\t * the conversion table, doesn't yield a Bayer format.\n>> > +\t\t */\n>> > +\t\tbayerFmtExpect = BayerFormat();\n>> > +\t\tV4L2PixelFormat v4l2FmtUnknown = V4L2PixelFormat(\n>> > +\t\t\tV4L2_PIX_FMT_BGRA444);\n>> > +\t\tbayerFmt = BayerFormat::fromV4L2PixelFormat(v4l2FmtUnknown);\n>> > +\t\tif (bayerFmt != bayerFmtExpect) {\n>\n>I would simply check if (bayerFmt.isValid()).\n>\n>> > +\t\t\tcout << \"Expected empty BayerFormat got: '\"\n>> > +\t\t\t << bayerFmt.toString() << \"'\" << endl;\n>> > +\t\t\treturn TestFail;\n>> > +\t\t}\n>> > +\n>> > +\t\t/*\n>> > +\t\t * Test if a valid Bayer format can be converted to a\n>> > +\t\t * string representation.\n>> > +\t\t */\n>> > +\t\tbayerFmt = BayerFormat(BayerFormat::BGGR, 8, BayerFormat::None);\n>> > +\t\tif (bayerFmt.toString() != \"BGGR-8\") {\n>> > +\t\t\tcout << \"String representation != 'BGGR8' (got: '\"\n>\n>s/BGGR8/BGGR-8/\n>\n>> > +\t\t\t << bayerFmt.toString() << \"' ) \" << endl;\n>> > +\t\t\treturn TestFail;\n>> > +\t\t}\n>> > +\n>> > +\t\t/*\n>> > +\t\t * Determine if an empty Bayer format results in no\n>> > +\t\t * string representation.\n>> > +\t\t */\n>> > +\t\tbayerFmt = BayerFormat();\n>> > +\t\tif (bayerFmt.toString() != \"INVALID\") {\n>> > +\t\t\tcout << \"String representation != 'INVALID' (got: '\"\n>> > +\t\t\t << bayerFmt.toString() << \"' ) \" << endl;\n>> > +\t\t\treturn TestFail;\n>> > +\t\t}\n>> > +\n>> > +\t\t/*\n>> > +\t\t * Perform a horizontal Flip and make sure that the\n>> > +\t\t * order is adjusted accordingly.\n>> > +\t\t */\n>> > +\t\tbayerFmt = BayerFormat(BayerFormat::BGGR, 8, BayerFormat::None);\n>> > +\t\tbayerFmtExpect = BayerFormat(BayerFormat::GBRG, 8,\n>> > +\t\t\t\t\t BayerFormat::None);\n>> > +\t\tBayerFormat hFlipFmt = bayerFmt.transform(Transform::HFlip);\n>> > +\t\tif (hFlipFmt != bayerFmtExpect) {\n>> > +\t\t\tcout << \"horizontal flip of 'BGGR-8' \"\n>> > +\t\t\t << \"should result in '\"\n>> > +\t\t\t << bayerFmtExpect.toString() << \"', got: '\"\n>> > +\t\t\t << hFlipFmt.toString() << \"'\" << endl;\n>> > +\t\t\treturn TestFail;\n>> > +\t\t}\n>>\n>> nice !\n>>\n>> > +\n>> > +\t\t/*\n>> > +\t\t * Perform a vertical Flip and make sure that\n>> > +\t\t * the order is adjusted accordingly.\n>> > +\t\t */\n>> > +\t\tbayerFmt = BayerFormat(BayerFormat::BGGR, 8, BayerFormat::None);\n>> > +\t\tbayerFmtExpect = BayerFormat(BayerFormat::GRBG, 8,\n>> > +\t\t\t\t\t BayerFormat::None);\n>> > +\t\tBayerFormat vFlipFmt = bayerFmt.transform(Transform::VFlip);\n>> > +\t\tif (vFlipFmt != bayerFmtExpect) {\n>> > +\t\t\tcout << \"vertical flip of 'BGGR-8' \"\n>> > +\t\t\t << \"should result in '\"\n>> > +\t\t\t << bayerFmtExpect.toString() << \"', got: '\"\n>> > +\t\t\t << vFlipFmt.toString() << \"'\" << endl;\n>> > +\t\t\treturn TestFail;\n>> > +\t\t}\n>>\n>> nice too!\n>>\n>> > +\n>> > +\t\t/*\n>> > +\t\t * Perform a transposition and make sure that nothing changes.\n>> > +\t\t * Transpositions are not implemented as sensors are not\n>> > +\t\t * expected to support this functionality.\n>> > +\t\t */\n>> > +\t\tbayerFmt = BayerFormat(BayerFormat::BGGR, 8, BayerFormat::None);\n>> > +\t\tBayerFormat transposeFmt = bayerFmt.transform(\n>> > +\t\t\tTransform::Transpose);\n>> > +\t\tif (transposeFmt.toString() != \"BGGR-8\") {\n>> > +\t\t\tcout << \"Transposition is not supported \"\n>> > +\t\t\t << \"format should be 'BGGR-8', got: '\"\n>> > +\t\t\t << transposeFmt.toString() << \"'\" << endl;\n>> > +\t\t\treturn TestFail;\n>> > +\t\t}\n>>\n>> I would drop this last case as it is not used in practice, as you have\n>> said.\n>\n>Especially given that the test passes with the missing implementation,\n>while it should fail.\n\nMore on this in David's review.\n\n>\n>> Mostly minors so please add\n>> Reviewed-by: Jacopo Mondi \n>\n>Reviewed-by: Laurent Pinchart \n\nThank you,\nSebastian\n\n>\n>> > +\n>> > +\t\treturn TestPass;\n>> > +\t}\n>> > +};\n>> > +\n>> > +TEST_REGISTER(BayerFormatTest);\n>> > +\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 CC15EC0F2B\n\tfor ;\n\tMon, 25 Jan 2021 09:33:03 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AAC6B682B4;\n\tMon, 25 Jan 2021 10:33:02 +0100 (CET)","from mail-wr1-x429.google.com (mail-wr1-x429.google.com\n\t[IPv6:2a00:1450:4864:20::429])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 383A0681DC\n\tfor ;\n\tMon, 25 Jan 2021 07:24:05 +0100 (CET)","by mail-wr1-x429.google.com with SMTP id a1so11028274wrq.6\n\tfor ;\n\tSun, 24 Jan 2021 22:24:05 -0800 (PST)","from localhost\n\t(p200300d1ff07ec002b02c4c10cf04b20.dip0.t-ipconnect.de.\n\t[2003:d1:ff07:ec00:2b02:c4c1:cf0:4b20])\n\tby smtp.gmail.com with ESMTPSA id\n\ti131sm19257557wmi.25.2021.01.24.22.24.03\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tSun, 24 Jan 2021 22:24:04 -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=\"fGHz2a71\"; 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=Ui5qewWYiGIFWSukAcEkkDZMbsOjpB6V6C/3w/RO7aA=;\n\tb=fGHz2a71/mENLEWBmngVSvzpWT/zR3oM+JCZHq+AmUjO9tEU574GRtNl5TBdOnheZA\n\tnBR845bDgkg7DMpNIMD1rsnBBdCZPuPCmSdqWSJAlUmrAIVo+/C/XcMCu+9Py+pOPlq/\n\tfXvBvY0xxTcn1Z1JmhCuo2KkEXxg1mbEu3aXqchSPluMbNo7GCEic7BWIETgI0FP5xJJ\n\tStyRmWIlsnO+EUk1CNhu3ZVNPqAFhF10Jlv8f7BeK362LVZki1JfeyNnoZhlpnGVA5CG\n\t5ThGcp0TAHjkrTLPRyZIPw1peoTKvviuUTG6qVZpqnEq+pRc73jdNDqHNKzEBmapHiCQ\n\teX1Q==","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=Ui5qewWYiGIFWSukAcEkkDZMbsOjpB6V6C/3w/RO7aA=;\n\tb=azK4qsFG/WM0UNFeTqQEZdLThkQVOHaOSh5Ogtq+tSGPotrUkvmgt0nSVrKJkE8mgl\n\tzG//w/iY0O9fthVFFHsRsOsnom4X7W8/Y4iE+V8WJrKKcYHbbwJQs7Q/DCyT1D0Ffk/h\n\tkEUjgwMJmC1Y/j/+JxjPhJZo2rKuocjZWlsrsAWzliN97xbtF2D6TQvhcPFbvwmaV1DH\n\tIBeYg/Alo9S8p2XIXR9fdzgTt/qYaRcAhnKX7/f7xxvk30N9+bvLXe0PXlQR8K/6HIj2\n\tdx81ZAK54MPLlmq/OCywDCbhTE8Q9zS6jAeIAgnc5J29qAaW1g9ax0W5e5oR+NkecW14\n\tYFnA==","X-Gm-Message-State":"AOAM530Hjb/PMuMXfC3z63Tm5mn1QW2j+ZNwtJusAsh3uP6US4xupZs4\n\tP+nri3amTy8tXi3nMi3cbuM=","X-Google-Smtp-Source":"ABdhPJzvNlGjXt4cXdeslL4fSPV+ZWeqRHL8A9g1ZPpP34SdXV2OgFoIAFWbkZ2W1NeBPE80JIR5YA==","X-Received":"by 2002:a5d:6347:: with SMTP id\n\tb7mr15266746wrw.233.1611555844869; \n\tSun, 24 Jan 2021 22:24:04 -0800 (PST)","Date":"Mon, 25 Jan 2021 07:24:03 +0100","From":"Sebastian Fricke ","To":"Laurent Pinchart ","Message-ID":"<20210125062403.zkp6fvlm4lpnku3w@basti-TUXEDO-Book-XA1510>","References":"<20201231155336.7058-1-sebastian.fricke.linux@gmail.com>\n\t<20201231155336.7058-5-sebastian.fricke.linux@gmail.com>\n\t<20210108114654.g63ock4nbtiwlhpc@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","X-Mailman-Approved-At":"Mon, 25 Jan 2021 10:32:59 +0100","Subject":"Re: [libcamera-devel] [PATCH v2 4/4] 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@lists.libcamera.org","Content-Transfer-Encoding":"7bit","Content-Type":"text/plain; charset=\"us-ascii\"; Format=\"flowed\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14738,"web_url":"https://patchwork.libcamera.org/comment/14738/","msgid":"<20210125064932.uihimfipdaf224a3@basti-TUXEDO-Book-XA1510>","date":"2021-01-25T06:49:32","subject":"Re: [libcamera-devel] [PATCH v2 4/4] 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 12.01.2021 08:53, David Plowman wrote:\n>Hi Sebastian\n\nHey David, Jacopo & Laurent,\n\n>\n>Thanks for this patch set. I think it's excellent to have some unit\n>tests for the BayerFormat class, especially as the original author of\n>the class should probably have done it... :)\n>\n>Just one comment/question below:\n>\n>On Mon, 11 Jan 2021 at 01:06, Laurent Pinchart\n> wrote:\n>>\n>> Hi Sebastian,\n>>\n>> Thank you for the patch.\n>>\n>> On Fri, Jan 08, 2021 at 12:46:54PM +0100, Jacopo Mondi wrote:\n>> > On Thu, Dec 31, 2020 at 04:53:36PM +0100, Sebastian Fricke wrote:\n>> > > Test all of the present methods including the newly implemented\n>> > > `fromV4L2PixelFormat`, as well as the new operators `==/!=`.\n>> > >\n>> > > Signed-off-by: Sebastian Fricke \n>> > > ---\n>> > > test/bayer_format.cpp | 202 ++++++++++++++++++++++++++++++++++++++++++\n>> > > test/meson.build | 1 +\n>> > > 2 files changed, 203 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..dd7aa8cb\n>> > > --- /dev/null\n>> > > +++ b/test/bayer_format.cpp\n>> > > @@ -0,0 +1,202 @@\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>> > > +#include \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>> > > + * An empty Bayer format has to be invalid.\n>> > > + */\n>> >\n>> > Fits on 1 line (also other comments)\n>> >\n>> > > + BayerFormat bayerFmt = BayerFormat();\n>> >\n>> > Just\n>> > BayerFormat bayerFmt;\n>> > ?\n>> >\n>> > > + if (bayerFmt.isValid()) {\n>> > > + cout << \"An empty BayerFormat \"\n>> >\n>> > We have a mixed usage of cout/cerr in test/\n>> > But in new tests I would use cerr.\n>> >\n>> > > + << \"has to be invalid.\" << endl;\n>>\n>> I'd avoid splitting strings to make it easier to grep for error\n>> messages. This means that it would be nice to shorten a few of the\n>> strings below if possible.\n>>\n>> > > + return TestFail;\n>> > > + }\n>> > > +\n>> > > + /*\n>> > > + * A correct Bayer format has to be valid.\n>> > > + */\n>> > > + bayerFmt = BayerFormat(BayerFormat::BGGR, 8, BayerFormat::None);\n>> > > + if (!bayerFmt.isValid()) {\n>> > > + cout << \"A correct BayerFormat \"\n>> > > + << \"has to be valid.\" << endl;\n>> > > + return TestFail;\n>> > > + }\n>> > > +\n>> > > + /*\n>> > > + * Two bayer formats created with the same order and bit depth\n>> > > + * have to be equal.\n>> > > + */\n>> > > + bayerFmt = BayerFormat(BayerFormat::BGGR, 8, BayerFormat::None);\n>> > > + BayerFormat bayerFmtExpect = BayerFormat(BayerFormat::BGGR, 8,\n>> > > + BayerFormat::None);\n>> > > + if (bayerFmt != bayerFmtExpect) {\n>> > > + cout << \"Two BayerFormat object created with the same \"\n>> > > + << \"order and bitDepth must be equal.\" << endl;\n>>\n>> For instance this could become\n>>\n>> cout << \"Comparison if identical formats failed\" << endl;\n>>\n>> > > + return TestFail;\n>> > > + }\n>> > > +\n>> > > + /*\n>> > > + * Two Bayer formats created with the same order but with a\n>> > > + * different bitDepth are not equal.\n>> > > + */\n>> > > + bayerFmt = BayerFormat(BayerFormat::BGGR, 8, BayerFormat::None);\n>> > > + bayerFmtExpect = BayerFormat(BayerFormat::BGGR, 12,\n>> > > + BayerFormat::None);\n>> > > + if (bayerFmt == bayerFmtExpect) {\n>> > > + cout << \"Two BayerFormat object created with the same \"\n>> > > + << \"order, but different bit depths are not equal.\"\n>> > > + << endl;\n>> > > + return TestFail;\n>> > > + }\n>> > > +\n>> > > + /*\n>> > > + * Create a Bayer format with a V4L2PixelFormat and check if we\n>> > > + * get the same format after converting back to the V4L2 Format.\n>> > > + */\n>> > > + V4L2PixelFormat v4l2FmtExpect = V4L2PixelFormat(\n>> > > + V4L2_PIX_FMT_SBGGR8);\n>> > > + bayerFmt = BayerFormat::fromV4L2PixelFormat(v4l2FmtExpect);\n>> > > + V4L2PixelFormat v4l2Fmt = bayerFmt.toV4L2PixelFormat();\n>> > > + if (v4l2Fmt != v4l2FmtExpect) {\n>> > > + cout << \"Expected: '\" << v4l2FmtExpect.toString()\n>> > > + << \"' got: '\" << v4l2Fmt.toString() << \"'\" << endl;\n>> > > + return TestFail;\n>> > > + }\n>> > > +\n>> > > + /*\n>> > > + * Use a Bayer format that is not found in the mapping table\n>> > > + * and verify that no matching V4L2PixelFormat is found.\n>> > > + */\n>> > > + v4l2FmtExpect = V4L2PixelFormat();\n>> > > + bayerFmt = BayerFormat(BayerFormat::BGGR, 24,\n>> > > + BayerFormat::None);\n>> > > + v4l2Fmt = bayerFmt.toV4L2PixelFormat();\n>> > > + if (v4l2Fmt != v4l2FmtExpect) {\n>> > > + cout << \"Expected: empty V4L2PixelFormat got: '\"\n>> > > + << v4l2Fmt.toString() << \"'\" << endl;\n>> > > + return TestFail;\n>> > > + }\n>> >\n>> > Mmm, not sure...\n>> > If such a format is ever added this will fail. And bayerFmt should not\n>> > be valid in first place, so accessing .toV4L2PixelFormat() is not of\n>> > much value... Do you think it's a valuable test case ?\n>> >\n>> > > +\n>> > > + /*\n>> > > + * Check if we get the expected Bayer format BGGR8\n>> > > + * when we convert the V4L2PixelFormat (V4L2_PIX_FMT_SBGGR8)\n>> > > + * to a Bayer format.\n>> > > + */\n>> > > + bayerFmtExpect = BayerFormat(BayerFormat::BGGR, 8,\n>> > > + BayerFormat::None);\n>> > > + v4l2Fmt = V4L2PixelFormat(V4L2_PIX_FMT_SBGGR8);\n>> > > + bayerFmt = BayerFormat::fromV4L2PixelFormat(v4l2Fmt);\n>> > > + if (bayerFmt != bayerFmtExpect) {\n>> > > + cout << \"Expected BayerFormat '\"\n>> > > + << bayerFmtExpect.toString() << \"',\"\n>> > > + << \"got: '\" << bayerFmt.toString() << \"'\" << endl;\n>>\n>> You can squash the two string into \"', got: '\".\n>>\n>> > > + return TestFail;\n>> > > + }\n>> > > +\n>> > > + /*\n>> > > + * Confirm that a V4L2PixelFormat that is not found in\n>> > > + * the conversion table, doesn't yield a Bayer format.\n>> > > + */\n>> > > + bayerFmtExpect = BayerFormat();\n>> > > + V4L2PixelFormat v4l2FmtUnknown = V4L2PixelFormat(\n>> > > + V4L2_PIX_FMT_BGRA444);\n>> > > + bayerFmt = BayerFormat::fromV4L2PixelFormat(v4l2FmtUnknown);\n>> > > + if (bayerFmt != bayerFmtExpect) {\n>>\n>> I would simply check if (bayerFmt.isValid()).\n>>\n>> > > + cout << \"Expected empty BayerFormat got: '\"\n>> > > + << bayerFmt.toString() << \"'\" << endl;\n>> > > + return TestFail;\n>> > > + }\n>> > > +\n>> > > + /*\n>> > > + * Test if a valid Bayer format can be converted to a\n>> > > + * string representation.\n>> > > + */\n>> > > + bayerFmt = BayerFormat(BayerFormat::BGGR, 8, BayerFormat::None);\n>> > > + if (bayerFmt.toString() != \"BGGR-8\") {\n>> > > + cout << \"String representation != 'BGGR8' (got: '\"\n>>\n>> s/BGGR8/BGGR-8/\n>>\n>> > > + << bayerFmt.toString() << \"' ) \" << endl;\n>> > > + return TestFail;\n>> > > + }\n>> > > +\n>> > > + /*\n>> > > + * Determine if an empty Bayer format results in no\n>> > > + * string representation.\n>> > > + */\n>> > > + bayerFmt = BayerFormat();\n>> > > + if (bayerFmt.toString() != \"INVALID\") {\n>> > > + cout << \"String representation != 'INVALID' (got: '\"\n>> > > + << bayerFmt.toString() << \"' ) \" << endl;\n>> > > + return TestFail;\n>> > > + }\n>> > > +\n>> > > + /*\n>> > > + * Perform a horizontal Flip and make sure that the\n>> > > + * order is adjusted accordingly.\n>> > > + */\n>> > > + bayerFmt = BayerFormat(BayerFormat::BGGR, 8, BayerFormat::None);\n>> > > + bayerFmtExpect = BayerFormat(BayerFormat::GBRG, 8,\n>> > > + BayerFormat::None);\n>> > > + BayerFormat hFlipFmt = bayerFmt.transform(Transform::HFlip);\n>> > > + if (hFlipFmt != bayerFmtExpect) {\n>> > > + cout << \"horizontal flip of 'BGGR-8' \"\n>> > > + << \"should result in '\"\n>> > > + << bayerFmtExpect.toString() << \"', got: '\"\n>> > > + << hFlipFmt.toString() << \"'\" << endl;\n>> > > + return TestFail;\n>> > > + }\n>> >\n>> > nice !\n>> >\n>> > > +\n>> > > + /*\n>> > > + * Perform a vertical Flip and make sure that\n>> > > + * the order is adjusted accordingly.\n>> > > + */\n>> > > + bayerFmt = BayerFormat(BayerFormat::BGGR, 8, BayerFormat::None);\n>> > > + bayerFmtExpect = BayerFormat(BayerFormat::GRBG, 8,\n>> > > + BayerFormat::None);\n>> > > + BayerFormat vFlipFmt = bayerFmt.transform(Transform::VFlip);\n>> > > + if (vFlipFmt != bayerFmtExpect) {\n>> > > + cout << \"vertical flip of 'BGGR-8' \"\n>> > > + << \"should result in '\"\n>> > > + << bayerFmtExpect.toString() << \"', got: '\"\n>> > > + << vFlipFmt.toString() << \"'\" << endl;\n>> > > + return TestFail;\n>> > > + }\n>> >\n>> > nice too!\n>> >\n>> > > +\n>> > > + /*\n>> > > + * Perform a transposition and make sure that nothing changes.\n>> > > + * Transpositions are not implemented as sensors are not\n>> > > + * expected to support this functionality.\n>> > > + */\n>> > > + bayerFmt = BayerFormat(BayerFormat::BGGR, 8, BayerFormat::None);\n>> > > + BayerFormat transposeFmt = bayerFmt.transform(\n>> > > + Transform::Transpose);\n>> > > + if (transposeFmt.toString() != \"BGGR-8\") {\n>> > > + cout << \"Transposition is not supported \"\n>> > > + << \"format should be 'BGGR-8', got: '\"\n>> > > + << transposeFmt.toString() << \"'\" << endl;\n>> > > + return TestFail;\n>> > > + }\n>> >\n>> > I would drop this last case as it is not used in practice, as you have\n>> > said.\n>>\n>> Especially given that the test passes with the missing implementation,\n>> while it should fail.\n>\n>I'm not quite sure if I understand this, I'm under the impression that\n>being able to transpose a BayerFormat is required to work correctly,\n>and does not depend on whether a particular sensor supports it (which\n>they don't).\n\nI have looked into implementing the transpose functionality for the\nBayerFormat but I noticed that this is not as easy as the horizontal and\nvertical flip.\nBefore I add more code to the transform method, I would like to make\nsure, that this is something that the team believes to be useful.\nSo, @everyone, I would love to hear your thoughts about this.\n\n>\n>As such I'd quite like to keep this test, though I'd prefer to avoid\n>testing for equality by comparing strings.\n\nYes I can change that.\n\n>I'd also quite like to see\n>another transpose test but where the Bayer format does change (e.g.\n>GRBG).\n>\n>But I don't feel too strongly, and perhaps I've misunderstood, so\n>please add this regardless:\n>\n>Reviewed-by: David Plowman \n>\n>Thanks again and best regards\n>David\n\nThank you,\nSebastian\n\n>\n>>\n>> > Mostly minors so please add\n>> > Reviewed-by: Jacopo Mondi \n>>\n>> Reviewed-by: Laurent Pinchart \n>>\n>> > > +\n>> > > + return TestPass;\n>> > > + }\n>> > > +};\n>> > > +\n>> > > +TEST_REGISTER(BayerFormatTest);\n>> > > +\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 B7483C0F2B\n\tfor ;\n\tMon, 25 Jan 2021 09:33:04 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9FF3B682BA;\n\tMon, 25 Jan 2021 10:33:03 +0100 (CET)","from mail-wr1-x434.google.com (mail-wr1-x434.google.com\n\t[IPv6:2a00:1450:4864:20::434])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id F2DBB6829E\n\tfor ;\n\tMon, 25 Jan 2021 07:49:34 +0100 (CET)","by mail-wr1-x434.google.com with SMTP id b5so11049294wrr.10\n\tfor ;\n\tSun, 24 Jan 2021 22:49:34 -0800 (PST)","from localhost\n\t(p200300d1ff07ec002b02c4c10cf04b20.dip0.t-ipconnect.de.\n\t[2003:d1:ff07:ec00:2b02:c4c1:cf0:4b20])\n\tby smtp.gmail.com with ESMTPSA id\n\tx81sm15539835wmg.40.2021.01.24.22.49.33\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tSun, 24 Jan 2021 22:49:34 -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=\"pxsKcu0j\"; 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=srOWVriBBTw6DDniiOjsdlwKM4/mTEtE3Pd+Wasxm8o=;\n\tb=pxsKcu0jKIFXg87lPXrzKpNswaHzVQ4eF5HkJALWUBJKNUyLbv2JxxpgYJHM8b2yTc\n\tstGnuk6kNZRMG16T92jaQ6ejzsROAqk2QxgoC+Z4dezG36xrJLJIzRb+dL9rYwKOPzMF\n\tvxrdTJPOoDkv0rWZdWPSLarNFQ44DHWh4eDzS32Dv8+iaQpQOab3n9kQG3Ryfuxc8cUl\n\t2D/Z/HXTIj9HHPyuZ4ZjCwIvClSGCHxxOHNFh0kLudsXUztuVCewf5at+zEYpb4LnUTZ\n\tSJtxzvwKai0Gnrn44s9DjVT3RHupHTstJrz5W55oWabCU5mlgM42L1K4F5l1EoAgB8Ha\n\tZnbg==","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=srOWVriBBTw6DDniiOjsdlwKM4/mTEtE3Pd+Wasxm8o=;\n\tb=bkjmtygJgJ+E08V+/fkE/d/vGEA8bk7BfC3tc+gCbarRR8zomqox+Wwe7X/5FrddX4\n\tOabcdtBb0KFFNlmQPOGyiUMrRB/AvAUswx4/mnF4sGKnaHfQFi2tIrYkd4a/64s5v+b9\n\tr+hgW03b0IVpnaYYngzRDiCWy0hP0iy1OnpT2dHMZER8oYK1UNzxqlpw2b9tklX9SmLW\n\t8RElWfNH0M6GWOxlqg9m507YTbgcuSU1SHnzJJ0nhiUA2nbLP90YFurwffgjQ6zoV/pQ\n\tnjcb0DTUvzEwgmGkvzGbX2ZgP6MGP9diOYjH16ZXXHHk2UlSNBJlCGoS9cbp0RgHvINC\n\tkBBw==","X-Gm-Message-State":"AOAM5338W+i/Bq/6w9NF1ztxcvyqGq8uOb9xnagTvdaj847gtZOmxnWQ\n\tx/YyDGxtYndW541noqOvLuY=","X-Google-Smtp-Source":"ABdhPJzB9VLzco8yooPUksu7hHMaZabsA3W79DjY0mmYG0eyksgf1YclunkN/5ZkwoSG1dREb+8Z+A==","X-Received":"by 2002:adf:a11d:: with SMTP id o29mr564609wro.45.1611557374623; \n\tSun, 24 Jan 2021 22:49:34 -0800 (PST)","Date":"Mon, 25 Jan 2021 07:49:32 +0100","From":"Sebastian Fricke ","To":"David Plowman ","Message-ID":"<20210125064932.uihimfipdaf224a3@basti-TUXEDO-Book-XA1510>","References":"<20201231155336.7058-1-sebastian.fricke.linux@gmail.com>\n\t<20201231155336.7058-5-sebastian.fricke.linux@gmail.com>\n\t<20210108114654.g63ock4nbtiwlhpc@uno.localdomain>\n\t\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","X-Mailman-Approved-At":"Mon, 25 Jan 2021 10:33:00 +0100","Subject":"Re: [libcamera-devel] [PATCH v2 4/4] 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\" "}}]