[{"id":17527,"web_url":"https://patchwork.libcamera.org/comment/17527/","msgid":"","date":"2021-06-14T16:26:04","subject":"Re: [libcamera-devel] [PATCH 1/6] ipa: raspberrypi: Non-functional\n\tformatting fixes to md_parser.hpp","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Naush\n\nThanks for tidying this up!\n\nOn Mon, 14 Jun 2021 at 10:53, Naushir Patuck wrote:\n>\n> Adjust source formatting to closer match libcamera guidelines:\n>\n> - Remove unused header files.\n> - Switch to C style comments.\n> - Add whitespace for readability.\n>\n> There are no functional changes in this commit.\n>\n> Signed-off-by: Naushir Patuck \n> ---\n> src/ipa/raspberrypi/md_parser.hpp | 144 ++++++++++++++++++------------\n> 1 file changed, 85 insertions(+), 59 deletions(-)\n>\n> diff --git a/src/ipa/raspberrypi/md_parser.hpp b/src/ipa/raspberrypi/md_parser.hpp\n> index 8e22b1d7ca43..86e0577614e0 100644\n> --- a/src/ipa/raspberrypi/md_parser.hpp\n> +++ b/src/ipa/raspberrypi/md_parser.hpp\n> @@ -6,75 +6,94 @@\n> */\n> #pragma once\n>\n> -#include \n> -\n> #include \n>\n> /* Camera metadata parser class. Usage as shown below.\n> -\n> -Setup:\n> -\n> -Usually the metadata parser will be made as part of the CamHelper class so\n> -application code doesn't have to worry which to kind to instantiate. But for\n> -the sake of example let's suppose we're parsing imx219 metadata.\n> -\n> -MdParser *parser = new MdParserImx219(); // for example\n> -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, 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.\n> -\n> -Then on every frame:\n> -\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> - exposure was not found;\n> -if (parser->GetGainCode(parser, gain_code) != MdParser::OK)\n> - gain code was not found;\n> -\n> -(Note that the CamHelper class converts to/from exposure lines and time,\n> -and gain_code / actual gain.)\n> -\n> -If you suspect your embedded data may have changed its layout, change any line\n> -lengths, number of lines, bits per pixel etc. that are different, and\n> -then:\n> -\n> -parser->Reset();\n> -\n> -before calling Parse again. */\n> + *\n> + * Setup:\n> + *\n> + * Usually the metadata parser will be made as part of the CamHelper class so\n> + * application code doesn't have to worry which to kind to instantiate. But for\n\ns/which to kind/which kind/\nI suppose we could have done a drive-by typo fix here! But otherwise\nall good, so:\n\nReviewed-by: David Plowman \n\nThanks\nDavid\n\n> + * the sake of example let's suppose we're parsing imx219 metadata.\n> + *\n> + * MdParser *parser = new MdParserImx219(); // for example\n> + * 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, 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.\n> + *\n> + * Then on every frame:\n> + *\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> + * exposure was not found;\n> + * if (parser->GetGainCode(parser, gain_code) != MdParser::OK)\n> + * gain code was not found;\n> + *\n> + * (Note that the CamHelper class converts to/from exposure lines and time,\n> + * and gain_code / actual gain.)\n> + *\n> + * If you suspect your embedded data may have changed its layout, change any line\n> + * lengths, number of lines, bits per pixel etc. that are different, and\n> + * then:\n> + *\n> + * parser->Reset();\n> + *\n> + * before calling Parse again.\n> + */\n>\n> namespace RPiController {\n>\n> -// Abstract base class from which other metadata parsers are derived.\n> +/* Abstract base class from which other metadata parsers are derived. */\n>\n> class MdParser\n> {\n> public:\n> - // Parser status codes:\n> - // OK - success\n> - // NOTFOUND - value such as exposure or gain was not found\n> - // ERROR - all other errors\n> + /*\n> + * Parser status codes:\n> + * OK - success\n> + * NOTFOUND - value such as exposure or gain was not found\n> + * ERROR - all other errors\n> + */\n> enum Status {\n> OK = 0,\n> NOTFOUND = 1,\n> ERROR = 2\n> };\n> - MdParser() : reset_(true) {}\n> +\n> + MdParser() : reset_(true)\n> + {\n> + }\n> +\n> virtual ~MdParser() = default;\n> - void Reset() { reset_ = true; }\n> - void SetBitsPerPixel(int bpp) { bits_per_pixel_ = bpp; }\n> - void SetNumLines(unsigned int num_lines) { num_lines_ = num_lines; }\n> +\n> + void Reset()\n> + {\n> + reset_ = true;\n> + }\n> +\n> + void SetBitsPerPixel(int bpp)\n> + {\n> + bits_per_pixel_ = bpp;\n> + }\n> +\n> + void SetNumLines(unsigned int num_lines)\n> + {\n> + num_lines_ = num_lines;\n> + }\n> +\n> void SetLineLengthBytes(unsigned int num_bytes)\n> {\n> line_length_bytes_ = num_bytes;\n> }\n> +\n> virtual Status Parse(libcamera::Span buffer) = 0;\n> virtual Status GetExposureLines(unsigned int &lines) = 0;\n> virtual Status GetGainCode(unsigned int &gain_code) = 0;\n> @@ -87,22 +106,28 @@ protected:\n> unsigned int buffer_size_bytes_;\n> };\n>\n> -// This isn't a full implementation of a metadata parser for SMIA sensors,\n> -// however, it does provide the findRegs method which will prove useful and make\n> -// it easier to implement parsers for other SMIA-like sensors (see\n> -// md_parser_imx219.cpp for an example).\n> +/*\n> + * This isn't a full implementation of a metadata parser for SMIA sensors,\n> + * however, it does provide the findRegs method which will prove useful and make\n> + * it easier to implement parsers for other SMIA-like sensors (see\n> + * md_parser_imx219.cpp for an example).\n> + */\n>\n> class MdParserSmia : public MdParser\n> {\n> public:\n> - MdParserSmia() : MdParser() {}\n> + MdParserSmia() : MdParser()\n> + {\n> + }\n>\n> protected:\n> - // Note that error codes > 0 are regarded as non-fatal; codes < 0\n> - // indicate a bad data buffer. Status codes are:\n> - // PARSE_OK - found all registers, much happiness\n> - // MISSING_REGS - some registers found; should this be a hard error?\n> - // The remaining codes are all hard errors.\n> + /*\n> + * Note that error codes > 0 are regarded as non-fatal; codes < 0\n> + * indicate a bad data buffer. Status codes are:\n> + * PARSE_OK - found all registers, much happiness\n> + * MISSING_REGS - some registers found; should this be a hard error?\n> + * The remaining codes are all hard errors.\n> + */\n> enum ParseStatus {\n> PARSE_OK = 0,\n> MISSING_REGS = 1,\n> @@ -112,6 +137,7 @@ protected:\n> BAD_LINE_END = -4,\n> BAD_PADDING = -5\n> };\n> +\n> ParseStatus findRegs(libcamera::Span buffer, uint32_t regs[],\n> int offsets[], unsigned int num_regs);\n> };\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 52690C3218\n\tfor ;\n\tMon, 14 Jun 2021 16:26:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9D83768934;\n\tMon, 14 Jun 2021 18:26:17 +0200 (CEST)","from mail-wr1-x430.google.com (mail-wr1-x430.google.com\n\t[IPv6:2a00:1450:4864:20::430])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C205B602A1\n\tfor ;\n\tMon, 14 Jun 2021 18:26:16 +0200 (CEST)","by mail-wr1-x430.google.com with SMTP id c5so15178092wrq.9\n\tfor ;\n\tMon, 14 Jun 2021 09:26:16 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"mKrBkJXv\"; 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=GizAICl0ieUdCokDVfRzUCNOU2LdSm5H7dvItlU12Sw=;\n\tb=mKrBkJXvLJ1Pv8QDKgnZWAdHd9ouWsTJzj7p+zOgLOMBxf+p93nf1xtnLfcy739NQT\n\tDQ/jKrTLogxC3pezbXgdGGUUQj+CWFsZEk4lrMUy7PSSyksy23y+U1wQnbxVFh1xKcUC\n\t7JlF+V9QNS92LEwnZ+XNQF57rkyL/jr/DWDNVltt8iJVw9Vq8IRBp7ndmQjy7dJwcdh1\n\tneWS+6r8iEWP2vQw2Mu+CnEP7xzcX51slaNOYxqD8bySDRlB/UqTUyWdce+forTaBx1X\n\tscW3j45tFo0c4PZ9dte09mLuRTiBxiCffGcU+rPCNXeiE2y+ShcJja17DKGe/pq6QQmz\n\t/p0Q==","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=GizAICl0ieUdCokDVfRzUCNOU2LdSm5H7dvItlU12Sw=;\n\tb=jUnKdQpXwRcuIkKRIveSwGQKcFux1nrt30Sxs26uzsD+oRjEUgcOKvpSETBh4SLF14\n\tFV2jLgI+BNnQWkzGOY7MSoNjKZ7xWX6BLmbsbtAudmixRkDbUhcpLbAChhQBeO18NrwB\n\t1SlVQN2VKcz/OIys5Wvb6FROBLdF4ixkAnnozwSzHxb0AOKJYGdodTRfVECxqf6i28Xg\n\tbl040wChSIVY+fendIDlsrVSL7KO9kdkMkp+vIWeJtYchx/CBSzvy0pMtOwbEjdtZ9wv\n\tTgjGAyDz5nOiUCBDa8dhA41Q/CGxq24oBwIO9IPKbE8cab4KMilRf1S033xlkFTtiqlH\n\t4Zwg==","X-Gm-Message-State":"AOAM531670OuYqXzpKoJxGc/rPpmluTTfbXl/S/wa0hGa2v1iQNZ3gXO\n\tX2q7GLhLLhttWXtK4WOiXzj28MiTiucZzWmmKbXLAg==","X-Google-Smtp-Source":"ABdhPJwD1ftc5fPNY9NriMSN8HyNx+k/hpThGksSs7hF8Lc0mvxHCMuiEuZVWDde0Dy4p1EFzzOsi93p9BCIBmk2rMY=","X-Received":"by 2002:a5d:59a9:: with SMTP id p9mr19327670wrr.86.1623687976414;\n\tMon, 14 Jun 2021 09:26:16 -0700 (PDT)","MIME-Version":"1.0","References":"<20210614095340.3051816-1-naush@raspberrypi.com>\n\t<20210614095340.3051816-2-naush@raspberrypi.com>","In-Reply-To":"<20210614095340.3051816-2-naush@raspberrypi.com>","From":"David Plowman ","Date":"Mon, 14 Jun 2021 17:26:04 +0100","Message-ID":"","To":"Naushir Patuck ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH 1/6] ipa: raspberrypi: Non-functional\n\tformatting fixes to md_parser.hpp","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17541,"web_url":"https://patchwork.libcamera.org/comment/17541/","msgid":"","date":"2021-06-15T02:35:40","subject":"Re: [libcamera-devel] [PATCH 1/6] ipa: raspberrypi: Non-functional\n\tformatting fixes to md_parser.hpp","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Mon, Jun 14, 2021 at 10:53:35AM +0100, Naushir Patuck wrote:\n> Adjust source formatting to closer match libcamera guidelines:\n> \n> - Remove unused header files.\n> - Switch to C style comments.\n> - Add whitespace for readability.\n> \n> There are no functional changes in this commit.\n> \n> Signed-off-by: Naushir Patuck \n> ---\n> src/ipa/raspberrypi/md_parser.hpp | 144 ++++++++++++++++++------------\n> 1 file changed, 85 insertions(+), 59 deletions(-)\n> \n> diff --git a/src/ipa/raspberrypi/md_parser.hpp b/src/ipa/raspberrypi/md_parser.hpp\n> index 8e22b1d7ca43..86e0577614e0 100644\n> --- a/src/ipa/raspberrypi/md_parser.hpp\n> +++ b/src/ipa/raspberrypi/md_parser.hpp\n> @@ -6,75 +6,94 @@\n> */\n> #pragma once\n> \n> -#include \n> -\n> #include \n> \n> /* Camera metadata parser class. Usage as shown below.\n\nThis should be\n\n/*\n * Camera metadata parser class. Usage as shown below.\n\nI'll fix when applying.\n\nReviewed-by: Laurent Pinchart \n\n> -\n> -Setup:\n> -\n> -Usually the metadata parser will be made as part of the CamHelper class so\n> -application code doesn't have to worry which to kind to instantiate. But for\n> -the sake of example let's suppose we're parsing imx219 metadata.\n> -\n> -MdParser *parser = new MdParserImx219(); // for example\n> -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, 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.\n> -\n> -Then on every frame:\n> -\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> - exposure was not found;\n> -if (parser->GetGainCode(parser, gain_code) != MdParser::OK)\n> - gain code was not found;\n> -\n> -(Note that the CamHelper class converts to/from exposure lines and time,\n> -and gain_code / actual gain.)\n> -\n> -If you suspect your embedded data may have changed its layout, change any line\n> -lengths, number of lines, bits per pixel etc. that are different, and\n> -then:\n> -\n> -parser->Reset();\n> -\n> -before calling Parse again. */\n> + *\n> + * Setup:\n> + *\n> + * Usually the metadata parser will be made as part of the CamHelper class so\n> + * application code doesn't have to worry which to kind to instantiate. But for\n> + * the sake of example let's suppose we're parsing imx219 metadata.\n> + *\n> + * MdParser *parser = new MdParserImx219(); // for example\n> + * 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, 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.\n> + *\n> + * Then on every frame:\n> + *\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> + * exposure was not found;\n> + * if (parser->GetGainCode(parser, gain_code) != MdParser::OK)\n> + * gain code was not found;\n> + *\n> + * (Note that the CamHelper class converts to/from exposure lines and time,\n> + * and gain_code / actual gain.)\n> + *\n> + * If you suspect your embedded data may have changed its layout, change any line\n> + * lengths, number of lines, bits per pixel etc. that are different, and\n> + * then:\n> + *\n> + * parser->Reset();\n> + *\n> + * before calling Parse again.\n> + */\n> \n> namespace RPiController {\n> \n> -// Abstract base class from which other metadata parsers are derived.\n> +/* Abstract base class from which other metadata parsers are derived. */\n> \n> class MdParser\n> {\n> public:\n> -\t// Parser status codes:\n> -\t// OK - success\n> -\t// NOTFOUND - value such as exposure or gain was not found\n> -\t// ERROR - all other errors\n> +\t/*\n> +\t * Parser status codes:\n> +\t * OK - success\n> +\t * NOTFOUND - value such as exposure or gain was not found\n> +\t * ERROR - all other errors\n> +\t */\n> \tenum Status {\n> \t\tOK = 0,\n> \t\tNOTFOUND = 1,\n> \t\tERROR = 2\n> \t};\n> -\tMdParser() : reset_(true) {}\n> +\n> +\tMdParser() : reset_(true)\n> +\t{\n> +\t}\n> +\n> \tvirtual ~MdParser() = default;\n> -\tvoid Reset() { reset_ = true; }\n> -\tvoid SetBitsPerPixel(int bpp) { bits_per_pixel_ = bpp; }\n> -\tvoid SetNumLines(unsigned int num_lines) { num_lines_ = num_lines; }\n> +\n> +\tvoid Reset()\n> +\t{\n> +\t\treset_ = true;\n> +\t}\n> +\n> +\tvoid SetBitsPerPixel(int bpp)\n> +\t{\n> +\t\tbits_per_pixel_ = bpp;\n> +\t}\n> +\n> +\tvoid SetNumLines(unsigned int num_lines)\n> +\t{\n> +\t\tnum_lines_ = num_lines;\n> +\t}\n> +\n> \tvoid SetLineLengthBytes(unsigned int num_bytes)\n> \t{\n> \t\tline_length_bytes_ = num_bytes;\n> \t}\n> +\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> @@ -87,22 +106,28 @@ protected:\n> \tunsigned int buffer_size_bytes_;\n> };\n> \n> -// This isn't a full implementation of a metadata parser for SMIA sensors,\n> -// however, it does provide the findRegs method which will prove useful and make\n> -// it easier to implement parsers for other SMIA-like sensors (see\n> -// md_parser_imx219.cpp for an example).\n> +/*\n> + * This isn't a full implementation of a metadata parser for SMIA sensors,\n> + * however, it does provide the findRegs method which will prove useful and make\n> + * it easier to implement parsers for other SMIA-like sensors (see\n> + * md_parser_imx219.cpp for an example).\n> + */\n> \n> class MdParserSmia : public MdParser\n> {\n> public:\n> -\tMdParserSmia() : MdParser() {}\n> +\tMdParserSmia() : MdParser()\n> +\t{\n> +\t}\n> \n> protected:\n> -\t// Note that error codes > 0 are regarded as non-fatal; codes < 0\n> -\t// indicate a bad data buffer. Status codes are:\n> -\t// PARSE_OK - found all registers, much happiness\n> -\t// MISSING_REGS - some registers found; should this be a hard error?\n> -\t// The remaining codes are all hard errors.\n> +\t/*\n> +\t * Note that error codes > 0 are regarded as non-fatal; codes < 0\n> +\t * indicate a bad data buffer. Status codes are:\n> +\t * PARSE_OK - found all registers, much happiness\n> +\t * MISSING_REGS - some registers found; should this be a hard error?\n> +\t * The remaining codes are all hard errors.\n> +\t */\n> \tenum ParseStatus {\n> \t\tPARSE_OK = 0,\n> \t\tMISSING_REGS = 1,\n> @@ -112,6 +137,7 @@ protected:\n> \t\tBAD_LINE_END = -4,\n> \t\tBAD_PADDING = -5\n> \t};\n> +\n> \tParseStatus findRegs(libcamera::Span buffer, uint32_t regs[],\n> \t\t\t int offsets[], unsigned int num_regs);\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 1542ABD78E\n\tfor ;\n\tTue, 15 Jun 2021 02:36:03 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6A5B56892B;\n\tTue, 15 Jun 2021 04:36:02 +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 712B268925\n\tfor ;\n\tTue, 15 Jun 2021 04:36:01 +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 DFFCCE9;\n\tTue, 15 Jun 2021 04:36:00 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"bEA6yrM2\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1623724561;\n\tbh=20+dbDS1QRWdDaWL6kOcxNyPbWKrGBhs7rQRS3ElqjE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=bEA6yrM2CCx+dVZgaKDdzMnseWASD7mL3zvpFDKkjxh8Q7OETcuhAVFVERhDFiyJ8\n\tDUg4ajI5Jcs+qsQ/hCcfr7YaU9NDsSRlbJ3LqoJ/u9UZdoKWEnbwF/r7a3SCKjcsRV\n\tUvDPuKfckuhTPhiWzy95eN3ZimH7lj4P9D2B9jVk=","Date":"Tue, 15 Jun 2021 05:35:40 +0300","From":"Laurent Pinchart ","To":"Naushir Patuck ","Message-ID":"","References":"<20210614095340.3051816-1-naush@raspberrypi.com>\n\t<20210614095340.3051816-2-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210614095340.3051816-2-naush@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH 1/6] ipa: raspberrypi: Non-functional\n\tformatting fixes to md_parser.hpp","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17542,"web_url":"https://patchwork.libcamera.org/comment/17542/","msgid":"","date":"2021-06-15T02:36:50","subject":"Re: [libcamera-devel] [PATCH 1/6] ipa: raspberrypi: Non-functional\n\tformatting fixes to md_parser.hpp","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Tue, Jun 15, 2021 at 05:35:42AM +0300, Laurent Pinchart wrote:\n> On Mon, Jun 14, 2021 at 10:53:35AM +0100, Naushir Patuck wrote:\n> > Adjust source formatting to closer match libcamera guidelines:\n> > \n> > - Remove unused header files.\n> > - Switch to C style comments.\n> > - Add whitespace for readability.\n> > \n> > There are no functional changes in this commit.\n> > \n> > Signed-off-by: Naushir Patuck \n> > ---\n> > src/ipa/raspberrypi/md_parser.hpp | 144 ++++++++++++++++++------------\n> > 1 file changed, 85 insertions(+), 59 deletions(-)\n> > \n> > diff --git a/src/ipa/raspberrypi/md_parser.hpp b/src/ipa/raspberrypi/md_parser.hpp\n> > index 8e22b1d7ca43..86e0577614e0 100644\n> > --- a/src/ipa/raspberrypi/md_parser.hpp\n> > +++ b/src/ipa/raspberrypi/md_parser.hpp\n> > @@ -6,75 +6,94 @@\n> > */\n> > #pragma once\n> > \n> > -#include \n> > -\n\nOh, and this should be kept, as the header uses uint*_t types.\n\n> > #include \n> > \n> > /* Camera metadata parser class. Usage as shown below.\n> \n> This should be\n> \n> /*\n> * Camera metadata parser class. Usage as shown below.\n> \n> I'll fix when applying.\n> \n> Reviewed-by: Laurent Pinchart \n> \n> > -\n> > -Setup:\n> > -\n> > -Usually the metadata parser will be made as part of the CamHelper class so\n> > -application code doesn't have to worry which to kind to instantiate. But for\n> > -the sake of example let's suppose we're parsing imx219 metadata.\n> > -\n> > -MdParser *parser = new MdParserImx219(); // for example\n> > -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, 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.\n> > -\n> > -Then on every frame:\n> > -\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> > - exposure was not found;\n> > -if (parser->GetGainCode(parser, gain_code) != MdParser::OK)\n> > - gain code was not found;\n> > -\n> > -(Note that the CamHelper class converts to/from exposure lines and time,\n> > -and gain_code / actual gain.)\n> > -\n> > -If you suspect your embedded data may have changed its layout, change any line\n> > -lengths, number of lines, bits per pixel etc. that are different, and\n> > -then:\n> > -\n> > -parser->Reset();\n> > -\n> > -before calling Parse again. */\n> > + *\n> > + * Setup:\n> > + *\n> > + * Usually the metadata parser will be made as part of the CamHelper class so\n> > + * application code doesn't have to worry which to kind to instantiate. But for\n> > + * the sake of example let's suppose we're parsing imx219 metadata.\n> > + *\n> > + * MdParser *parser = new MdParserImx219(); // for example\n> > + * 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, 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.\n> > + *\n> > + * Then on every frame:\n> > + *\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> > + * exposure was not found;\n> > + * if (parser->GetGainCode(parser, gain_code) != MdParser::OK)\n> > + * gain code was not found;\n> > + *\n> > + * (Note that the CamHelper class converts to/from exposure lines and time,\n> > + * and gain_code / actual gain.)\n> > + *\n> > + * If you suspect your embedded data may have changed its layout, change any line\n> > + * lengths, number of lines, bits per pixel etc. that are different, and\n> > + * then:\n> > + *\n> > + * parser->Reset();\n> > + *\n> > + * before calling Parse again.\n> > + */\n> > \n> > namespace RPiController {\n> > \n> > -// Abstract base class from which other metadata parsers are derived.\n> > +/* Abstract base class from which other metadata parsers are derived. */\n> > \n> > class MdParser\n> > {\n> > public:\n> > -\t// Parser status codes:\n> > -\t// OK - success\n> > -\t// NOTFOUND - value such as exposure or gain was not found\n> > -\t// ERROR - all other errors\n> > +\t/*\n> > +\t * Parser status codes:\n> > +\t * OK - success\n> > +\t * NOTFOUND - value such as exposure or gain was not found\n> > +\t * ERROR - all other errors\n> > +\t */\n> > \tenum Status {\n> > \t\tOK = 0,\n> > \t\tNOTFOUND = 1,\n> > \t\tERROR = 2\n> > \t};\n> > -\tMdParser() : reset_(true) {}\n> > +\n> > +\tMdParser() : reset_(true)\n> > +\t{\n> > +\t}\n> > +\n> > \tvirtual ~MdParser() = default;\n> > -\tvoid Reset() { reset_ = true; }\n> > -\tvoid SetBitsPerPixel(int bpp) { bits_per_pixel_ = bpp; }\n> > -\tvoid SetNumLines(unsigned int num_lines) { num_lines_ = num_lines; }\n> > +\n> > +\tvoid Reset()\n> > +\t{\n> > +\t\treset_ = true;\n> > +\t}\n> > +\n> > +\tvoid SetBitsPerPixel(int bpp)\n> > +\t{\n> > +\t\tbits_per_pixel_ = bpp;\n> > +\t}\n> > +\n> > +\tvoid SetNumLines(unsigned int num_lines)\n> > +\t{\n> > +\t\tnum_lines_ = num_lines;\n> > +\t}\n> > +\n> > \tvoid SetLineLengthBytes(unsigned int num_bytes)\n> > \t{\n> > \t\tline_length_bytes_ = num_bytes;\n> > \t}\n> > +\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> > @@ -87,22 +106,28 @@ protected:\n> > \tunsigned int buffer_size_bytes_;\n> > };\n> > \n> > -// This isn't a full implementation of a metadata parser for SMIA sensors,\n> > -// however, it does provide the findRegs method which will prove useful and make\n> > -// it easier to implement parsers for other SMIA-like sensors (see\n> > -// md_parser_imx219.cpp for an example).\n> > +/*\n> > + * This isn't a full implementation of a metadata parser for SMIA sensors,\n> > + * however, it does provide the findRegs method which will prove useful and make\n> > + * it easier to implement parsers for other SMIA-like sensors (see\n> > + * md_parser_imx219.cpp for an example).\n> > + */\n> > \n> > class MdParserSmia : public MdParser\n> > {\n> > public:\n> > -\tMdParserSmia() : MdParser() {}\n> > +\tMdParserSmia() : MdParser()\n> > +\t{\n> > +\t}\n> > \n> > protected:\n> > -\t// Note that error codes > 0 are regarded as non-fatal; codes < 0\n> > -\t// indicate a bad data buffer. Status codes are:\n> > -\t// PARSE_OK - found all registers, much happiness\n> > -\t// MISSING_REGS - some registers found; should this be a hard error?\n> > -\t// The remaining codes are all hard errors.\n> > +\t/*\n> > +\t * Note that error codes > 0 are regarded as non-fatal; codes < 0\n> > +\t * indicate a bad data buffer. Status codes are:\n> > +\t * PARSE_OK - found all registers, much happiness\n> > +\t * MISSING_REGS - some registers found; should this be a hard error?\n> > +\t * The remaining codes are all hard errors.\n> > +\t */\n> > \tenum ParseStatus {\n> > \t\tPARSE_OK = 0,\n> > \t\tMISSING_REGS = 1,\n> > @@ -112,6 +137,7 @@ protected:\n> > \t\tBAD_LINE_END = -4,\n> > \t\tBAD_PADDING = -5\n> > \t};\n> > +\n> > \tParseStatus findRegs(libcamera::Span buffer, uint32_t regs[],\n> > \t\t\t int offsets[], unsigned int num_regs);\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 2175FBD78E\n\tfor ;\n\tTue, 15 Jun 2021 02:37:13 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D14C868932;\n\tTue, 15 Jun 2021 04:37:12 +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 E942968925\n\tfor ;\n\tTue, 15 Jun 2021 04:37:10 +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 6742FE9;\n\tTue, 15 Jun 2021 04:37:10 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"CIhhIFs9\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1623724630;\n\tbh=bWwGZXHjLip/zNP7nTFd6w5jI2u7+ohD8K1H4mi87sI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=CIhhIFs9lFeKDIqazCgfScRgDHqD7xuqnPvmEI4dYRopCK9FPesjdDKHiE4hbiZCj\n\tEzECyfmy3l9gekP1WgzmD6jR8LPk6JTLazyDHHA1KCXsp1PURqZjiv1YT/La1ux6Pu\n\td9BQ7ke7j1UMJbPKLt1DpsLrhngwsWJExdLjSZi4=","Date":"Tue, 15 Jun 2021 05:36:50 +0300","From":"Laurent Pinchart ","To":"Naushir Patuck ","Message-ID":"","References":"<20210614095340.3051816-1-naush@raspberrypi.com>\n\t<20210614095340.3051816-2-naush@raspberrypi.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 1/6] ipa: raspberrypi: Non-functional\n\tformatting fixes to md_parser.hpp","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]