[{"id":16836,"web_url":"https://patchwork.libcamera.org/comment/16836/","msgid":"","date":"2021-05-07T22:20:39","subject":"Re: [libcamera-devel] [PATCH v4 1/2] ipa: raspberrypi: Make sensor\n\tembedded data parser use Span class","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi David,\n\nThank you for the patch.\n\nOn Fri, May 07, 2021 at 12:37:27PM +0100, David Plowman wrote:\n> Improve MdParser::Parse() by taking a Span, pointing to const data\n> that it should not change, as its input buffer.\n\nLooks good to me. I may have kept the variables named 'data' instead of\n'buffer', but it doesn't matter much.\n\n> Signed-off-by: David Plowman \n\nReviewed-by: Laurent Pinchart \n\n> ---\n> src/ipa/raspberrypi/cam_helper_imx219.cpp | 8 ++++----\n> src/ipa/raspberrypi/cam_helper_imx477.cpp | 8 ++++----\n> src/ipa/raspberrypi/md_parser.cpp | 23 ++++++++++++-----------\n> src/ipa/raspberrypi/md_parser.hpp | 20 ++++++++------------\n> 4 files changed, 28 insertions(+), 31 deletions(-)\n> \n> diff --git a/src/ipa/raspberrypi/cam_helper_imx219.cpp b/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> index 566ecbca..e550fba6 100644\n> --- a/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> +++ b/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> @@ -29,7 +29,7 @@ class MdParserImx219 : public MdParserSmia\n> {\n> public:\n> \tMdParserImx219();\n> -\tStatus Parse(void *data) override;\n> +\tStatus Parse(libcamera::Span buffer) override;\n> \tStatus GetExposureLines(unsigned int &lines) override;\n> \tStatus GetGainCode(unsigned int &gain_code) override;\n> private:\n> @@ -118,7 +118,7 @@ MdParserImx219::MdParserImx219()\n> \treg_offsets_[0] = reg_offsets_[1] = reg_offsets_[2] = -1;\n> }\n> \n> -MdParser::Status MdParserImx219::Parse(void *data)\n> +MdParser::Status MdParserImx219::Parse(libcamera::Span buffer)\n> {\n> \tbool try_again = false;\n> \n> @@ -132,7 +132,7 @@ MdParser::Status MdParserImx219::Parse(void *data)\n> \t\t/* Need to be ordered */\n> \t\tuint32_t regs[3] = { GAIN_REG, EXPHI_REG, EXPLO_REG };\n> \t\treg_offsets_[0] = reg_offsets_[1] = reg_offsets_[2] = -1;\n> -\t\tint ret = static_cast(findRegs(static_cast(data),\n> +\t\tint ret = static_cast(findRegs(buffer,\n> \t\t\t\t\t\t regs, reg_offsets_, 3));\n> \t\t/*\n> \t\t * > 0 means \"worked partially but parse again next time\",\n> @@ -148,7 +148,7 @@ MdParser::Status MdParserImx219::Parse(void *data)\n> \t\tif (reg_offsets_[i] == -1)\n> \t\t\tcontinue;\n> \n> -\t\treg_values_[i] = static_cast(data)[reg_offsets_[i]];\n> +\t\treg_values_[i] = buffer[reg_offsets_[i]];\n> \t}\n> \n> \t/* Re-parse next time if we were unhappy in some way. */\n> diff --git a/src/ipa/raspberrypi/cam_helper_imx477.cpp b/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> index 73a5ca7d..a4a58c15 100644\n> --- a/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> +++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> @@ -21,7 +21,7 @@ class MdParserImx477 : public MdParserSmia\n> {\n> public:\n> \tMdParserImx477();\n> -\tStatus Parse(void *data) override;\n> +\tStatus Parse(libcamera::Span buffer) override;\n> \tStatus GetExposureLines(unsigned int &lines) override;\n> \tStatus GetGainCode(unsigned int &gain_code) override;\n> private:\n> @@ -107,7 +107,7 @@ MdParserImx477::MdParserImx477()\n> \treg_offsets_[0] = reg_offsets_[1] = reg_offsets_[2] = reg_offsets_[3] = -1;\n> }\n> \n> -MdParser::Status MdParserImx477::Parse(void *data)\n> +MdParser::Status MdParserImx477::Parse(libcamera::Span buffer)\n> {\n> \tbool try_again = false;\n> \n> @@ -126,7 +126,7 @@ MdParser::Status MdParserImx477::Parse(void *data)\n> \t\t\tGAINLO_REG\n> \t\t};\n> \t\treg_offsets_[0] = reg_offsets_[1] = reg_offsets_[2] = reg_offsets_[3] = -1;\n> -\t\tint ret = static_cast(findRegs(static_cast(data),\n> +\t\tint ret = static_cast(findRegs(buffer,\n> \t\t\t\t\t\t regs, reg_offsets_, 4));\n> \t\t/*\n> \t\t * > 0 means \"worked partially but parse again next time\",\n> @@ -142,7 +142,7 @@ MdParser::Status MdParserImx477::Parse(void *data)\n> \t\tif (reg_offsets_[i] == -1)\n> \t\t\tcontinue;\n> \n> -\t\treg_values_[i] = static_cast(data)[reg_offsets_[i]];\n> +\t\treg_values_[i] = buffer[reg_offsets_[i]];\n> \t}\n> \n> \t/* Re-parse next time if we were unhappy in some way. */\n> diff --git a/src/ipa/raspberrypi/md_parser.cpp b/src/ipa/raspberrypi/md_parser.cpp\n> index d82c102c..852a1d34 100644\n> --- a/src/ipa/raspberrypi/md_parser.cpp\n> +++ b/src/ipa/raspberrypi/md_parser.cpp\n> @@ -27,12 +27,13 @@ using namespace RPiController;\n> #define REG_VALUE 0x5a\n> #define REG_SKIP 0x55\n> \n> -MdParserSmia::ParseStatus MdParserSmia::findRegs(unsigned char *data,\n> +MdParserSmia::ParseStatus MdParserSmia::findRegs(libcamera::Span buffer,\n> \t\t\t\t\t\t uint32_t regs[], int offsets[],\n> \t\t\t\t\t\t unsigned int num_regs)\n> {\n> \tassert(num_regs > 0);\n> -\tif (data[0] != LINE_START)\n> +\n> +\tif (buffer[0] != LINE_START)\n> \t\treturn NO_LINE_START;\n> \n> \tunsigned int current_offset = 1; // after the LINE_START\n> @@ -40,15 +41,15 @@ MdParserSmia::ParseStatus MdParserSmia::findRegs(unsigned char *data,\n> \tunsigned int reg_num = 0, first_reg = 0;\n> \tParseStatus retcode = PARSE_OK;\n> \twhile (1) {\n> -\t\tint tag = data[current_offset++];\n> +\t\tint tag = buffer[current_offset++];\n> \t\tif ((bits_per_pixel_ == 10 &&\n> \t\t (current_offset + 1 - current_line_start) % 5 == 0) ||\n> \t\t (bits_per_pixel_ == 12 &&\n> \t\t (current_offset + 1 - current_line_start) % 3 == 0)) {\n> -\t\t\tif (data[current_offset++] != REG_SKIP)\n> +\t\t\tif (buffer[current_offset++] != REG_SKIP)\n> \t\t\t\treturn BAD_DUMMY;\n> \t\t}\n> -\t\tint data_byte = data[current_offset++];\n> +\t\tint data_byte = buffer[current_offset++];\n> \t\t//printf(\"Offset %u, tag 0x%02x data_byte 0x%02x\\n\", current_offset-1, tag, data_byte);\n> \t\tif (tag == LINE_END_TAG) {\n> \t\t\tif (data_byte != LINE_END_TAG)\n> @@ -59,18 +60,18 @@ MdParserSmia::ParseStatus MdParserSmia::findRegs(unsigned char *data,\n> \t\t\t\tcurrent_offset =\n> \t\t\t\t\tcurrent_line_start + line_length_bytes_;\n> \t\t\t\t// Require whole line to be in the buffer (if buffer size set).\n> -\t\t\t\tif (buffer_size_bytes_ &&\n> +\t\t\t\tif (buffer.size() &&\n> \t\t\t\t current_offset + line_length_bytes_ >\n> -\t\t\t\t\t buffer_size_bytes_)\n> +\t\t\t\t\t buffer.size())\n> \t\t\t\t\treturn MISSING_REGS;\n> -\t\t\t\tif (data[current_offset] != LINE_START)\n> +\t\t\t\tif (buffer[current_offset] != LINE_START)\n> \t\t\t\t\treturn NO_LINE_START;\n> \t\t\t} else {\n> \t\t\t\t// allow a zero line length to mean \"hunt for the next line\"\n> -\t\t\t\twhile (data[current_offset] != LINE_START &&\n> -\t\t\t\t current_offset < buffer_size_bytes_)\n> +\t\t\t\twhile (buffer[current_offset] != LINE_START &&\n> +\t\t\t\t current_offset < buffer.size())\n> \t\t\t\t\tcurrent_offset++;\n> -\t\t\t\tif (current_offset == buffer_size_bytes_)\n> +\t\t\t\tif (current_offset == buffer.size())\n> \t\t\t\t\treturn NO_LINE_START;\n> \t\t\t}\n> \t\t\t// inc current_offset to after LINE_START\n> diff --git a/src/ipa/raspberrypi/md_parser.hpp b/src/ipa/raspberrypi/md_parser.hpp\n> index 926f676e..8e22b1d7 100644\n> --- a/src/ipa/raspberrypi/md_parser.hpp\n> +++ b/src/ipa/raspberrypi/md_parser.hpp\n> @@ -8,6 +8,8 @@\n> \n> #include \n> \n> +#include \n> +\n> /* Camera metadata parser class. Usage as shown below.\n> \n> Setup:\n> @@ -21,17 +23,15 @@ parser->SetBitsPerPixel(bpp);\n> parser->SetLineLengthBytes(pitch);\n> parser->SetNumLines(2);\n> \n> -Note 1: if you don't know how many lines there are, you can use SetBufferSize\n> -instead to limit the total buffer size.\n> +Note 1: if you don't know how many lines there are, the size of the input\n> +buffer is used as a limit instead.\n> \n> Note 2: if you don't know the line length, you can leave the line length unset\n> -(or set to zero) and the parser will hunt for the line start instead. In this\n> -case SetBufferSize *must* be used so that the parser won't run off the end of\n> -the buffer.\n> +(or set to zero) and the parser will hunt for the line start instead.\n> \n> Then on every frame:\n> \n> -if (parser->Parse(data) != MdParser::OK)\n> +if (parser->Parse(buffer) != MdParser::OK)\n> much badness;\n> unsigned int exposure_lines, gain_code\n> if (parser->GetExposureLines(exposure_lines) != MdParser::OK)\n> @@ -75,11 +75,7 @@ public:\n> \t{\n> \t\tline_length_bytes_ = num_bytes;\n> \t}\n> -\tvoid SetBufferSize(unsigned int num_bytes)\n> -\t{\n> -\t\tbuffer_size_bytes_ = num_bytes;\n> -\t}\n> -\tvirtual Status Parse(void *data) = 0;\n> +\tvirtual Status Parse(libcamera::Span buffer) = 0;\n> \tvirtual Status GetExposureLines(unsigned int &lines) = 0;\n> \tvirtual Status GetGainCode(unsigned int &gain_code) = 0;\n> \n> @@ -116,7 +112,7 @@ protected:\n> \t\tBAD_LINE_END = -4,\n> \t\tBAD_PADDING = -5\n> \t};\n> -\tParseStatus findRegs(unsigned char *data, uint32_t regs[],\n> +\tParseStatus findRegs(libcamera::Span buffer, uint32_t regs[],\n> \t\t\t int offsets[], unsigned int num_regs);\n> };\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 37F4EBF831\n\tfor ;\n\tFri, 7 May 2021 22:20:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B58036890C;\n\tSat, 8 May 2021 00:20:47 +0200 (CEST)","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 1EF1A68901\n\tfor ;\n\tSat, 8 May 2021 00:20:46 +0200 (CEST)","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 88BC13D7;\n\tSat, 8 May 2021 00:20:45 +0200 (CEST)"],"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=\"j0Efx7wu\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1620426045;\n\tbh=5iIGb/+tEhX7SGnPg5RNQQlVHRRpltBS9pF8bV8BQ1w=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=j0Efx7wuAR2b1WnI0YFyVB2P9OGzBX9kvpQ/lDCIrvLP/hVN29rXPu++uo6Rgizji\n\tIoZIr5+JxWwEdQD5mvT0a8md8rKux3iimPUMW2lvBiFvz4lXrlgCxCJnh4BUYaE9iG\n\t1qEzk+7Ch/Nz2hcsqff7dKcwRdxrngSOUTB44Vek=","Date":"Sat, 8 May 2021 01:20:39 +0300","From":"Laurent Pinchart ","To":"David Plowman ","Message-ID":"","References":"<20210507113728.14037-1-david.plowman@raspberrypi.com>\n\t<20210507113728.14037-2-david.plowman@raspberrypi.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210507113728.14037-2-david.plowman@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v4 1/2] ipa: raspberrypi: Make sensor\n\tembedded data parser use Span 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\" "}}]