[{"id":4309,"web_url":"https://patchwork.libcamera.org/comment/4309/","msgid":"<41cc4f19-504e-3dcb-bcb2-d0cd6d86ab9f@ideasonboard.com>","date":"2020-03-26T15:37:05","subject":"Re: [libcamera-devel] [PATCH v3] libcamera: v4l2PixelFormat:\n\tReplace hex with fourCC","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Kaaira,\n\nOn 26/03/2020 09:33, 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\nThank you, This looks good to me.\n\nOnly a couple of quite minor formatting points, which if there are no\nfurther issues, and you're happy with; I can handle when applying.\n\nReviewed-by: Kieran Bingham \n\n> ---\n> \n> Changes since v2:\n> \t- reformatted the code\n> \t- added cerr messages before TestFail\n> \t- Changed test case to accomodate edge case\n> \t- Increased buffer size of char array\n> \t- Changed the maximum length of for loop.\n> \n> Changes since v1:\n> - Add tests for checking this function.\n> - use char[] instead of stringstream.\n> - add checks for default value.\n> - Print '.' for non-printable characters.\n> \n> src/libcamera/v4l2_videodevice.cpp | 19 ++++++++++++++++---\n> test/v4l2_videodevice/formats.cpp | 20 ++++++++++++++++++++\n> 2 files changed, 36 insertions(+), 3 deletions(-)\n> \n> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> index b778181..bb049ec 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[8] = { static_cast(fourcc_ & 0x7f),\n> +\t\t static_cast((fourcc_ >> 8) & 0x7f),\n> +\t\t static_cast((fourcc_ >> 16) & 0x7f),\n> +\t\t static_cast((fourcc_ >> 24) & 0x7f) };\n> +\n> +\tfor (unsigned int i = 0; i < 4; i++) {\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\nProbably a blank line here to make sure the return statement is distinct.\n\n> +\treturn ss;\n> }\n> \n> /**\n> diff --git a/test/v4l2_videodevice/formats.cpp b/test/v4l2_videodevice/formats.cpp\n> index d504d17..4e02afc 100644\n> --- a/test/v4l2_videodevice/formats.cpp\n> +++ b/test/v4l2_videodevice/formats.cpp\n> @@ -47,6 +47,26 @@ protected:\n> \t\t\treturn TestFail;\n> \t\t}\n> \n> +\t\tstd::vector> formats{\n\nMissing space between formats and {\n\n> +\t\t\t{ V4L2_PIX_FMT_YUYV, \"YUYV\" }, { 0, \"Invalid\" },\n> +\t\t\t{ v4l2_fourcc( 0, 1, 2, 3 ), \"....\" },\n\nBecause this is a table, I would probably put each entry on it's own\nline, rather than condensing to fit 80chars or such.\n\n\ni.e.\n\t\t\t{ V4L2_PIX_FMT_YUYV, \"YUYV\" },\n\t\t\t{ 0, \"Invalid\" },\n\t\t\t{ v4l2_fourcc( 0, 1, 2, 3 ), \"....\" },\n...\n\n> +\t\t\t{ V4L2_PIX_FMT_Y16_BE, \"Y16 -BE\" }\n> +\t\t};\n> +\n> +\t\tfor (const auto &format : formats) {\n> +\t\t\tif (V4L2PixelFormat(format.first).toString() != format.second) {\n> +\t\t\t\tcerr << \"Failed to convert V4L2PixelFormat to its fourcc\"\n> +\t\t\t\t << \" string\" << endl;\n> +\t\t\t\treturn TestFail;\n> +\t\t\t}\n> +\t\t}\n> +\n> +\t\tif (V4L2PixelFormat().toString() != \"Invalid\") {\n> +\t\t\tcerr << \"Failed to convert default V4L2PixelFormat to 'Invalid'\"\n> +\t\t\t << \" string\" << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> \t\treturn TestPass;\n> \t}\n> };\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 84C1360414\n\tfor ;\n\tThu, 26 Mar 2020 16:37:08 +0100 (CET)","from [192.168.0.20]\n\t(cpc89242-aztw30-2-0-cust488.18-1.cable.virginm.net [86.31.129.233])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id DC8602DC;\n\tThu, 26 Mar 2020 16:37:07 +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=\"f7zSgvtO\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1585237028;\n\tbh=QjOdRSU3bHXs69hfbQwmixhxD0GOpMPMJnS4ppHBZlU=;\n\th=Reply-To:Subject:To:References:From:Date:In-Reply-To:From;\n\tb=f7zSgvtOqhrEJYPRIhpcf6dTS2k7TDYQrXXSO1YTkeh/65KPWCxhCVTD7UBB0H32a\n\tIKyEevLD0oAiNac5ho+EaKGGib12Z4xGOE1zAliI5rCfDObxuDWjmbeLoa+h9gbFfJ\n\tX5k+tNaFeEJEOA89knqv8nWNkIACwLpcrcqxbOyw=","Reply-To":"kieran.bingham@ideasonboard.com","To":"Kaaira Gupta , libcamera-devel@lists.libcamera.org,\n\tHelen Koike ,\n\tVaishali Thakkar ","References":"<20200326093327.GA9015@kaaira-HP-Pavilion-Notebook>","From":"Kieran Bingham ","Openpgp":"preference=signencrypt","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"<41cc4f19-504e-3dcb-bcb2-d0cd6d86ab9f@ideasonboard.com>","Date":"Thu, 26 Mar 2020 15:37:05 +0000","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101\n\tThunderbird/60.9.1","MIME-Version":"1.0","In-Reply-To":"<20200326093327.GA9015@kaaira-HP-Pavilion-Notebook>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v3] 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":"Thu, 26 Mar 2020 15:37:08 -0000"}},{"id":4312,"web_url":"https://patchwork.libcamera.org/comment/4312/","msgid":"","date":"2020-03-26T15:49:04","subject":"Re: [libcamera-devel] [PATCH v3] libcamera: v4l2PixelFormat:\n\tReplace hex with fourCC","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi again :-)\n\nOn 26/03/2020 15:37, Kieran Bingham wrote:\n> Hi Kaaira,\n> \n> On 26/03/2020 09:33, 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> Thank you, This looks good to me.\n> \n> Only a couple of quite minor formatting points, which if there are no\n> further issues, and you're happy with; I can handle when applying.\n> \n> Reviewed-by: Kieran Bingham \n> \n>> ---\n>>\n>> Changes since v2:\n>> \t- reformatted the code\n>> \t- added cerr messages before TestFail\n>> \t- Changed test case to accomodate edge case\n>> \t- Increased buffer size of char array\n>> \t- Changed the maximum length of for loop.\n>>\n>> Changes since v1:\n>> - Add tests for checking this function.\n>> - use char[] instead of stringstream.\n>> - add checks for default value.\n>> - Print '.' for non-printable characters.\n>>\n>> src/libcamera/v4l2_videodevice.cpp | 19 ++++++++++++++++---\n>> test/v4l2_videodevice/formats.cpp | 20 ++++++++++++++++++++\n>> 2 files changed, 36 insertions(+), 3 deletions(-)\n>>\n>> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n>> index b778181..bb049ec 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[8] = { static_cast(fourcc_ & 0x7f),\n>> +\t\t static_cast((fourcc_ >> 8) & 0x7f),\n>> +\t\t static_cast((fourcc_ >> 16) & 0x7f),\n>> +\t\t static_cast((fourcc_ >> 24) & 0x7f) };\n>> +\n>> +\tfor (unsigned int i = 0; i < 4; i++) {\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> \n> Probably a blank line here to make sure the return statement is distinct.\n> \n>> +\treturn ss;\n>> }\n>> \n>> /**\n>> diff --git a/test/v4l2_videodevice/formats.cpp b/test/v4l2_videodevice/formats.cpp\n>> index d504d17..4e02afc 100644\n>> --- a/test/v4l2_videodevice/formats.cpp\n>> +++ b/test/v4l2_videodevice/formats.cpp\n>> @@ -47,6 +47,26 @@ protected:\n>> \t\t\treturn TestFail;\n>> \t\t}\n>> \n>> +\t\tstd::vector> formats{\n> \n> Missing space between formats and {\n\nAhh - checkstyle correctly points out I'm wrong here, as this is hugged\ndue to it being an initialiser, rather than a scope block.\n\nI guess the alternative is:\n\n formats = {\n\nBut it doesn't make much differnce I don't think.\n\nLet's keep it as you posted without the space.\n\n> \n>> +\t\t\t{ V4L2_PIX_FMT_YUYV, \"YUYV\" }, { 0, \"Invalid\" },\n>> +\t\t\t{ v4l2_fourcc( 0, 1, 2, 3 ), \"....\" },\n> \n> Because this is a table, I would probably put each entry on it's own\n> line, rather than condensing to fit 80chars or such.\n> \n> \n> i.e.\n> \t\t\t{ V4L2_PIX_FMT_YUYV, \"YUYV\" },\n> \t\t\t{ 0, \"Invalid\" },\n> \t\t\t{ v4l2_fourcc( 0, 1, 2, 3 ), \"....\" },\n> ...\n> \n>> +\t\t\t{ V4L2_PIX_FMT_Y16_BE, \"Y16 -BE\" }\n>> +\t\t};\n>> +\n>> +\t\tfor (const auto &format : formats) {\n>> +\t\t\tif (V4L2PixelFormat(format.first).toString() != format.second) {\n>> +\t\t\t\tcerr << \"Failed to convert V4L2PixelFormat to its fourcc\"\n>> +\t\t\t\t << \" string\" << endl;\n>> +\t\t\t\treturn TestFail;\n>> +\t\t\t}\n>> +\t\t}\n>> +\n>> +\t\tif (V4L2PixelFormat().toString() != \"Invalid\") {\n>> +\t\t\tcerr << \"Failed to convert default V4L2PixelFormat to 'Invalid'\"\n>> +\t\t\t << \" string\" << endl;\n>> +\t\t\treturn TestFail;\n>> +\t\t}\n>> +\n>> \t\treturn TestPass;\n>> \t}\n>> };\n>>\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 DFCAD60414\n\tfor ;\n\tThu, 26 Mar 2020 16:49:06 +0100 (CET)","from [192.168.0.20]\n\t(cpc89242-aztw30-2-0-cust488.18-1.cable.virginm.net [86.31.129.233])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 3DC952DC;\n\tThu, 26 Mar 2020 16:49:06 +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=\"U7uTafPR\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1585237746;\n\tbh=2gHBk44OlDnk3CSBMZn8m/y1ClKUfuhOu5E7JDMcPrE=;\n\th=Reply-To:Subject:From:To:References:Date:In-Reply-To:From;\n\tb=U7uTafPRy/fScpZntYFQpqUav5VfJuuyfGmQbvn/dFd8O7cTj4MpIGksNXjB+yi4b\n\tcmc19TmGQBeZAFhjzoIU4g/jVDvcgrG4ku9QszxwoC0eE+KWEbJzLs0R3W555mAjee\n\tzkCvRn4JhaRbY3UpdlXvK8wW0Fnx6t9Emvw7ugcM=","Reply-To":"kieran.bingham@ideasonboard.com","From":"Kieran Bingham ","To":"Kaaira Gupta , libcamera-devel@lists.libcamera.org,\n\tHelen Koike ,\n\tVaishali Thakkar ","References":"<20200326093327.GA9015@kaaira-HP-Pavilion-Notebook>\n\t<41cc4f19-504e-3dcb-bcb2-d0cd6d86ab9f@ideasonboard.com>","Openpgp":"preference=signencrypt","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"","Date":"Thu, 26 Mar 2020 15:49:04 +0000","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101\n\tThunderbird/60.9.1","MIME-Version":"1.0","In-Reply-To":"<41cc4f19-504e-3dcb-bcb2-d0cd6d86ab9f@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH v3] 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":"Thu, 26 Mar 2020 15:49:07 -0000"}},{"id":4313,"web_url":"https://patchwork.libcamera.org/comment/4313/","msgid":"<20200326160615.GB14181@pendragon.ideasonboard.com>","date":"2020-03-26T16:06:15","subject":"Re: [libcamera-devel] [PATCH v3] 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 Kieran,\n\nOn Thu, Mar 26, 2020 at 03:49:04PM +0000, Kieran Bingham wrote:\n> On 26/03/2020 15:37, Kieran Bingham wrote:\n> > On 26/03/2020 09:33, 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> > Thank you, This looks good to me.\n> > \n> > Only a couple of quite minor formatting points, which if there are no\n> > further issues, and you're happy with; I can handle when applying.\n> > \n> > Reviewed-by: Kieran Bingham \n> > \n> >> ---\n> >>\n> >> Changes since v2:\n> >> \t- reformatted the code\n> >> \t- added cerr messages before TestFail\n> >> \t- Changed test case to accomodate edge case\n> >> \t- Increased buffer size of char array\n> >> \t- Changed the maximum length of for loop.\n> >>\n> >> Changes since v1:\n> >> - Add tests for checking this function.\n> >> - use char[] instead of stringstream.\n> >> - add checks for default value.\n> >> - Print '.' for non-printable characters.\n> >>\n> >> src/libcamera/v4l2_videodevice.cpp | 19 ++++++++++++++++---\n> >> test/v4l2_videodevice/formats.cpp | 20 ++++++++++++++++++++\n> >> 2 files changed, 36 insertions(+), 3 deletions(-)\n> >>\n> >> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> >> index b778181..bb049ec 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\nThis will result in messages such as \"Pixel format set to Invalid\". I\nwonder if \"INVALID\", or maybe something even more distinct from\nsurrounding text such as \"[INV]\" or \"\" would make sense.\n\n> >> +\n> >> +\tchar ss[8] = { static_cast(fourcc_ & 0x7f),\n> >> +\t\t static_cast((fourcc_ >> 8) & 0x7f),\n> >> +\t\t static_cast((fourcc_ >> 16) & 0x7f),\n> >> +\t\t static_cast((fourcc_ >> 24) & 0x7f) };\n> >> +\n> >> +\tfor (unsigned int i = 0; i < 4; i++) {\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> > \n> > Probably a blank line here to make sure the return statement is distinct.\n> > \n> >> +\treturn ss;\n> >> }\n> >> \n> >> /**\n> >> diff --git a/test/v4l2_videodevice/formats.cpp b/test/v4l2_videodevice/formats.cpp\n> >> index d504d17..4e02afc 100644\n> >> --- a/test/v4l2_videodevice/formats.cpp\n> >> +++ b/test/v4l2_videodevice/formats.cpp\n> >> @@ -47,6 +47,26 @@ protected:\n> >> \t\t\treturn TestFail;\n> >> \t\t}\n> >> \n> >> +\t\tstd::vector> formats{\n> > \n> > Missing space between formats and {\n> \n> Ahh - checkstyle correctly points out I'm wrong here, as this is hugged\n> due to it being an initialiser, rather than a scope block.\n> \n> I guess the alternative is:\n> \n> formats = {\n> \n> But it doesn't make much differnce I don't think.\n> \n> Let's keep it as you posted without the space.\n\nJust wanted to note that we'll likely need to go through the code to\nmake consistent usage of different initialization types at some point.\nThis is certainly fine for now.\n\n> >> +\t\t\t{ V4L2_PIX_FMT_YUYV, \"YUYV\" }, { 0, \"Invalid\" },\n> >> +\t\t\t{ v4l2_fourcc( 0, 1, 2, 3 ), \"....\" },\n> > \n> > Because this is a table, I would probably put each entry on it's own\n> > line, rather than condensing to fit 80chars or such.\n> > \n> > \n> > i.e.\n> > \t\t\t{ V4L2_PIX_FMT_YUYV, \"YUYV\" },\n> > \t\t\t{ 0, \"Invalid\" },\n> > \t\t\t{ v4l2_fourcc( 0, 1, 2, 3 ), \"....\" },\n> > ...\n\nI was about to propose that too :-)\n\n> >> +\t\t\t{ V4L2_PIX_FMT_Y16_BE, \"Y16 -BE\" }\n> >> +\t\t};\n> >> +\n> >> +\t\tfor (const auto &format : formats) {\n> >> +\t\t\tif (V4L2PixelFormat(format.first).toString() != format.second) {\n> >> +\t\t\t\tcerr << \"Failed to convert V4L2PixelFormat to its fourcc\"\n> >> +\t\t\t\t << \" string\" << endl;\n\nHow about\n\t\t\t\tcerr << \"Failed to convert V4L2PixelFormat \"\n\t\t\t\t << utils::hex(format.first) << \"to string\"\n\t\t\t\t << endl;\n\nto tell which one failed ?\n\n> >> +\t\t\t\treturn TestFail;\n> >> +\t\t\t}\n> >> +\t\t}\n> >> +\n> >> +\t\tif (V4L2PixelFormat().toString() != \"Invalid\") {\n> >> +\t\t\tcerr << \"Failed to convert default V4L2PixelFormat to 'Invalid'\"\n> >> +\t\t\t << \" string\" << endl;\n\nThis could then be\n\n\t\t\tcerr << \"Failed to convert default V4L2PixelFormat to string\"\n\t\t\t << endl;\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 ED13660414\n\tfor ;\n\tThu, 26 Mar 2020 17:06:19 +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 565C42DC;\n\tThu, 26 Mar 2020 17:06:19 +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=\"oI1FEqK2\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1585238779;\n\tbh=nr0CqntvcRTienQRDhQfoMY1XvLVufyQs5doNHH0Sic=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=oI1FEqK2t8QiMywKgttWQ8XKqoT2kKhfIzqmve7DuyHixu78qCVQi1koCB5t57ioM\n\tYSRRdMEKTgbhPv/UFLErSG+uDhpNq7K6WQk4FylsnFEvpmULouOimnnRN1TvxBmusQ\n\tsPTr0Lw1igFfjFIvfEzrnj4R+17RdxRTBCHiSfDg=","Date":"Thu, 26 Mar 2020 18:06:15 +0200","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"Kaaira Gupta , libcamera-devel@lists.libcamera.org,\n\tHelen Koike ,\n\tVaishali Thakkar ","Message-ID":"<20200326160615.GB14181@pendragon.ideasonboard.com>","References":"<20200326093327.GA9015@kaaira-HP-Pavilion-Notebook>\n\t<41cc4f19-504e-3dcb-bcb2-d0cd6d86ab9f@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v3] 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":"Thu, 26 Mar 2020 16:06:20 -0000"}}]