[{"id":4295,"web_url":"https://patchwork.libcamera.org/comment/4295/","msgid":"<20200325225436.GZ19171@pendragon.ideasonboard.com>","date":"2020-03-25T22:54:36","subject":"Re: [libcamera-devel] [PATCH v2] libcamera: v4l2PixelFormat:\n\tReplace hex with fourCC","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kaaira,\n\nThank you for the patch.\n\nOn Thu, Mar 26, 2020 at 03:32:52AM +0530, Kaaira Gupta wrote:\n> Print fourCC characters instead of the hex value in toString() as they are\n> more informative. Also, write the tests for this in formats.cpp\n> \n> Signed-off-by: Kaaira Gupta \n> ---\n> \n> Changes since v1:\n> \t- Add tests for checking this function.\n> \t- use char[] instead of stringstream.\n> \t- add checks for default value.\n> \t- Print '.' for non-printable characters.\n> \n> src/libcamera/v4l2_videodevice.cpp | 19 ++++++++++++++++---\n> test/v4l2_videodevice/formats.cpp | 15 +++++++++++++++\n> 2 files changed, 31 insertions(+), 3 deletions(-)\n> \n> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> index b778181..f5c14aa 100644\n> --- a/src/libcamera/v4l2_videodevice.cpp\n> +++ b/src/libcamera/v4l2_videodevice.cpp\n> @@ -336,9 +336,22 @@ bool V4L2BufferCache::Entry::operator==(const FrameBuffer &buffer) const\n> */\n> std::string V4L2PixelFormat::toString() const\n> {\n> -\tchar str[11];\n> -\tsnprintf(str, 11, \"0x%08x\", fourcc_);\n> -\treturn str;\n> +\tif (fourcc_ == 0)\n> +\t\treturn \"Invalid\";\n> +\n> +\tchar ss[7] = {static_cast(fourcc_ & 0x7f),\n> +\t\t\t\t static_cast((fourcc_ >> 8) & 0x7f),\n> +\t\t\t\t static_cast((fourcc_ >> 16) & 0x7f),\n> +\t\t\t\t static_cast((fourcc_ >> 24) & 0x7f)};\n> +\n> +\tfor (unsigned int i = 0; i < strlen(ss); i++){\n\nThis will stop at the first 0 byte. If you replace the badfourcc below\nwith (0, 1, 2, 3) you will see the test failing.\n\nWe consider any issue in the code that isn't caught by a test to imply\nthat there's a missing test. In this case I think using (0, 1, 2, 3)\ninstead of (1, 2, 3, 4) in the test case is a good test coverage\nextension. As for the fix in the code, i < 4 seems to be the simplest\nsolution.\n\n> +\t\tif (!isprint(ss[i]))\n> +\t\t\tss[i]= '.';\n> +\t}\n> +\n> +\tif (fourcc_ & (1 << 31))\n> +\t\tstrcat(ss, \"-BE\");\n\nThis overflows the buffer, as you have space for 7 characters only, and\nneed an eight character for the termating '\\0'.\n\n> +\treturn ss;\n> }\n\nThere are lots of formatting issues here and below that are caught by\ncheckstyle.py. Could you install it as a post-commit git hook ?\nInstructions are available in utils/hooks/post-commit. You don't have to\ntake all the flagged issues into account (in particular we haven't found\na way yet to tell the underlying code formatter, clang-format, about the\n80 columns soft limit and the 120 columns hard limit, so it will often\nunwrap lines up to the 120 columns limit).\n\n> \n> /**\n> diff --git a/test/v4l2_videodevice/formats.cpp b/test/v4l2_videodevice/formats.cpp\n> index d504d17..f129c5c 100644\n> --- a/test/v4l2_videodevice/formats.cpp\n> +++ b/test/v4l2_videodevice/formats.cpp\n> @@ -47,6 +47,21 @@ protected:\n> \t\t\treturn TestFail;\n> \t\t}\n> \n> +\t\tauto badfourcc = v4l2_fourcc( 1, 2, 3, 4 );\n\nWe usually discourage the usage of auto when the type is simple, to\navoid moving towards an untyped language. auto can introduce subtle bugs\nwhen the developers and reviewers don't have all the C++ type deduction\nrules in memory. In this case uin32_t is an appropriate type. Or,\npossibly even better, you could use the v4l2_fourcc macro directly below\ninstead of badfourcc.\n\n> +\t\tstd::vector> formats{\n> +\t\t\t{V4L2_PIX_FMT_YUYV,\"YUYV\"},{0, \"Invalid\"}, {badfourcc, \"....\"},\n> +\t\t\t{V4L2_PIX_FMT_Y16_BE,\"Y16 -BE\"}};\n> +\n> +\t\tfor (const auto &format :formats){\n> +\t\t\tif (V4L2PixelFormat(format.first).toString()!=format.second){\n\nPlease print a message to cerr to tell what failed, otherwise test\nfailures are difficult to investigate.\n\n> +\t\t\t\treturn TestFail;\n> +\t\t\t}\n> +\t\t}\n> +\n> +\t\tif (V4L2PixelFormat().toString() != \"Invalid\"){\n\nSame here.\n\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> \t\treturn TestPass;\n> \t}\n> };","headers":{"Return-Path":"","Received":["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 BC99E60413\n\tfor ;\n\tWed, 25 Mar 2020 23:54:41 +0100 (CET)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B86D580C;\n\tWed, 25 Mar 2020 23:54:40 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"YymnhEY0\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1585176881;\n\tbh=gNkgUTam8e7ZY2Iu4n+JaoQvun/Rg5O15rYERzXI7b4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=YymnhEY0bJuOOGE0+Lu32jJu4+q8LMc20RtLDjkphnKBSIrg1Z9DyCbzevIRUDmIB\n\tfX/k4LQwg+YJtTLGUu4w7u7KJGkCD0oONsZs3+ZL6rxedMpK73sUA/Un3mTt5oi4c6\n\tb8G1mYLbkruAWMMC60u1xe+XWu24jLlJC+SS+aqg=","Date":"Thu, 26 Mar 2020 00:54:36 +0200","From":"Laurent Pinchart ","To":"Kaaira Gupta ","Cc":"libcamera-devel@lists.libcamera.org,\n\tKieran Bingham ,\n\tHelen Koike ,\n\tVaishali Thakkar ","Message-ID":"<20200325225436.GZ19171@pendragon.ideasonboard.com>","References":"<20200325220252.GA14374@kaaira-HP-Pavilion-Notebook>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20200325220252.GA14374@kaaira-HP-Pavilion-Notebook>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v2] libcamera: v4l2PixelFormat:\n\tReplace hex with fourCC","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","X-List-Received-Date":"Wed, 25 Mar 2020 22:54:42 -0000"}}]