[{"id":17683,"web_url":"https://patchwork.libcamera.org/comment/17683/","msgid":"","date":"2021-06-22T09:06:43","subject":"Re: [libcamera-devel] [PATCH 3/3] ipa: raspberrypi: Generalise the\n\tSMIA metadata parser","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi,\n\nGentle nudge on this as well please.\nIt's the last patch in the series that needs tags.\n\nThanks!\nNaush\n\n\nOn Tue, 15 Jun 2021 at 15:42, Naushir Patuck wrote:\n\n> Instead of having each CamHelper subclass the MdParserSmia, change the\n> implementation of MdParserSmia to be more generic. The MdParserSmia now\n> gets\n> given a list of registers to search for and helper functions are used to\n> compute\n> exposure lines and gain codes from these registers.\n>\n> Update the imx219 and imx477 CamHelpers by using this new mechanism.\n>\n> Signed-off-by: Naushir Patuck \n> ---\n> src/ipa/raspberrypi/cam_helper.cpp | 35 +++---\n> src/ipa/raspberrypi/cam_helper.hpp | 2 +\n> src/ipa/raspberrypi/cam_helper_imx219.cpp | 118 ++++----------------\n> src/ipa/raspberrypi/cam_helper_imx477.cpp | 124 ++++------------------\n> src/ipa/raspberrypi/md_parser.hpp | 42 +++++---\n> src/ipa/raspberrypi/md_parser_smia.cpp | 66 ++++++++++--\n> 6 files changed, 148 insertions(+), 239 deletions(-)\n>\n> diff --git a/src/ipa/raspberrypi/cam_helper.cpp\n> b/src/ipa/raspberrypi/cam_helper.cpp\n> index 1474464c9257..41e292ecbb61 100644\n> --- a/src/ipa/raspberrypi/cam_helper.cpp\n> +++ b/src/ipa/raspberrypi/cam_helper.cpp\n> @@ -159,32 +159,32 @@ unsigned int CamHelper::MistrustFramesModeSwitch()\n> const\n> void CamHelper::parseEmbeddedData(Span buffer,\n> Metadata &metadata)\n> {\n> + MdParser::RegisterMap map;\n> + Metadata parsedMetadata;\n> +\n> if (buffer.empty())\n> return;\n>\n> - uint32_t exposureLines, gainCode;\n> -\n> - if (parser_->Parse(buffer) != MdParser::Status::OK ||\n> - parser_->GetExposureLines(exposureLines) !=\n> MdParser::Status::OK ||\n> - parser_->GetGainCode(gainCode) != MdParser::Status::OK) {\n> + if (parser_->Parse(buffer, map) != MdParser::Status::OK) {\n> LOG(IPARPI, Error) << \"Embedded data buffer parsing\n> failed\";\n> return;\n> }\n>\n> + PopulateMetadata(map, parsedMetadata);\n> + metadata.Merge(parsedMetadata);\n> +\n> /*\n> - * Overwrite the exposure/gain values in the DeviceStatus, as\n> - * we know better. Fetch it first in case any other fields were\n> - * set meaningfully.\n> + * Overwrite the exposure/gain values in the existing DeviceStatus\n> with\n> + * values from the parsed embedded buffer. Fetch it first in case\n> any\n> + * other fields were set meaningfully.\n> */\n> - DeviceStatus deviceStatus;\n> -\n> - if (metadata.Get(\"device.status\", deviceStatus) != 0) {\n> - LOG(IPARPI, Error) << \"DeviceStatus not found\";\n> + DeviceStatus deviceStatus, parsedDeviceStatus;\n> + if (metadata.Get(\"device.status\", deviceStatus) ||\n> + parsedMetadata.Get(\"device.status\", parsedDeviceStatus))\n> return;\n> - }\n>\n> - deviceStatus.shutter_speed = Exposure(exposureLines);\n> - deviceStatus.analogue_gain = Gain(gainCode);\n> + deviceStatus.shutter_speed = parsedDeviceStatus.shutter_speed;\n> + deviceStatus.analogue_gain = parsedDeviceStatus.analogue_gain;\n>\n> LOG(IPARPI, Debug) << \"Metadata updated - Exposure : \"\n> << deviceStatus.shutter_speed\n> @@ -194,6 +194,11 @@ void CamHelper::parseEmbeddedData(Span\n> buffer,\n> metadata.Set(\"device.status\", deviceStatus);\n> }\n>\n> +void CamHelper::PopulateMetadata([[maybe_unused]] const\n> MdParser::RegisterMap &map,\n> + [[maybe_unused]] Metadata &metadata) const\n> +{\n> +}\n> +\n> RegisterCamHelper::RegisterCamHelper(char const *cam_name,\n> CamHelperCreateFunc create_func)\n> {\n> diff --git a/src/ipa/raspberrypi/cam_helper.hpp\n> b/src/ipa/raspberrypi/cam_helper.hpp\n> index f53f5c39b01c..30879e60b861 100644\n> --- a/src/ipa/raspberrypi/cam_helper.hpp\n> +++ b/src/ipa/raspberrypi/cam_helper.hpp\n> @@ -91,6 +91,8 @@ public:\n> protected:\n> void parseEmbeddedData(libcamera::Span buffer,\n> Metadata &metadata);\n> + virtual void PopulateMetadata([[maybe_unused]] const\n> MdParser::RegisterMap &map,\n> + [[maybe_unused]] Metadata &metadata)\n> const;\n>\n> MdParser *parser_;\n> CameraMode mode_;\n> diff --git a/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> b/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> index d951cd552a21..24818e9ec4de 100644\n> --- a/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> +++ b/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> @@ -23,21 +23,13 @@\n>\n> using namespace RPiController;\n>\n> -/* Metadata parser implementation specific to Sony IMX219 sensors. */\n> -\n> -class MdParserImx219 : public MdParserSmia\n> -{\n> -public:\n> - MdParserImx219();\n> - Status Parse(libcamera::Span buffer) override;\n> - Status GetExposureLines(unsigned int &lines) override;\n> - Status GetGainCode(unsigned int &gain_code) override;\n> -private:\n> - /* Offset of the register's value in the metadata block. */\n> - int reg_offsets_[3];\n> - /* Value of the register, once read from the metadata block. */\n> - int reg_values_[3];\n> -};\n> +/*\n> + * We care about one gain register and a pair of exposure registers.\n> Their I2C\n> + * addresses from the Sony IMX219 datasheet:\n> + */\n> +constexpr uint32_t gainReg = 0x157;\n> +constexpr uint32_t expHiReg = 0x15a;\n> +constexpr uint32_t expLoReg = 0x15b;\n>\n> class CamHelperImx219 : public CamHelper\n> {\n> @@ -55,15 +47,19 @@ private:\n> */\n> static constexpr int frameIntegrationDiff = 4;\n>\n> - MdParserImx219 imx219_parser;\n> + MdParserSmia imx219_parser;\n> +\n> + void PopulateMetadata(const MdParser::RegisterMap &map,\n> + Metadata &metadata) const override;\n> };\n>\n> CamHelperImx219::CamHelperImx219()\n> #if ENABLE_EMBEDDED_DATA\n> - : CamHelper(&imx219_parser, frameIntegrationDiff)\n> + : CamHelper(&imx219_parser, frameIntegrationDiff),\n> #else\n> - : CamHelper(nullptr, frameIntegrationDiff)\n> + : CamHelper(nullptr, frameIntegrationDiff),\n> #endif\n> + imx219_parser({ gainReg, expHiReg, expLoReg })\n> {\n> }\n>\n> @@ -92,88 +88,20 @@ bool CamHelperImx219::SensorEmbeddedDataPresent() const\n> return ENABLE_EMBEDDED_DATA;\n> }\n>\n> -static CamHelper *Create()\n> +void CamHelperImx219::PopulateMetadata(const MdParser::RegisterMap &map,\n> + Metadata &metadata) const\n> {\n> - return new CamHelperImx219();\n> -}\n> + DeviceStatus deviceStatus;\n>\n> -static RegisterCamHelper reg(\"imx219\", &Create);\n> + deviceStatus.shutter_speed = Exposure(map.at(expHiReg) * 256 +\n> map.at(expLoReg));\n> + deviceStatus.analogue_gain = Gain(map.at(gainReg));\n>\n> -/*\n> - * We care about one gain register and a pair of exposure registers.\n> Their I2C\n> - * addresses from the Sony IMX219 datasheet:\n> - */\n> -#define GAIN_REG 0x157\n> -#define EXPHI_REG 0x15A\n> -#define EXPLO_REG 0x15B\n> -\n> -/*\n> - * Index of each into the reg_offsets and reg_values arrays. Must be in\n> - * register address order.\n> - */\n> -#define GAIN_INDEX 0\n> -#define EXPHI_INDEX 1\n> -#define EXPLO_INDEX 2\n> -\n> -MdParserImx219::MdParserImx219()\n> -{\n> - reg_offsets_[0] = reg_offsets_[1] = reg_offsets_[2] = -1;\n> + metadata.Set(\"device.status\", deviceStatus);\n> }\n>\n> -MdParser::Status MdParserImx219::Parse(libcamera::Span\n> buffer)\n> -{\n> - bool try_again = false;\n> -\n> - if (reset_) {\n> - /*\n> - * Search again through the metadata for the gain and\n> exposure\n> - * registers.\n> - */\n> - assert(bits_per_pixel_);\n> - /* Need to be ordered */\n> - uint32_t regs[3] = { GAIN_REG, EXPHI_REG, EXPLO_REG };\n> - reg_offsets_[0] = reg_offsets_[1] = reg_offsets_[2] = -1;\n> - int ret = static_cast(findRegs(buffer,\n> - regs, reg_offsets_,\n> 3));\n> - /*\n> - * > 0 means \"worked partially but parse again next time\",\n> - * < 0 means \"hard error\".\n> - */\n> - if (ret > 0)\n> - try_again = true;\n> - else if (ret < 0)\n> - return ERROR;\n> - }\n> -\n> - for (int i = 0; i < 3; i++) {\n> - if (reg_offsets_[i] == -1)\n> - continue;\n> -\n> - reg_values_[i] = buffer[reg_offsets_[i]];\n> - }\n> -\n> - /* Re-parse next time if we were unhappy in some way. */\n> - reset_ = try_again;\n> -\n> - return OK;\n> -}\n> -\n> -MdParser::Status MdParserImx219::GetExposureLines(unsigned int &lines)\n> +static CamHelper *Create()\n> {\n> - if (reg_offsets_[EXPHI_INDEX] == -1 || reg_offsets_[EXPLO_INDEX]\n> == -1)\n> - return NOTFOUND;\n> -\n> - lines = reg_values_[EXPHI_INDEX] * 256 + reg_values_[EXPLO_INDEX];\n> -\n> - return OK;\n> + return new CamHelperImx219();\n> }\n>\n> -MdParser::Status MdParserImx219::GetGainCode(unsigned int &gain_code)\n> -{\n> - if (reg_offsets_[GAIN_INDEX] == -1)\n> - return NOTFOUND;\n> -\n> - gain_code = reg_values_[GAIN_INDEX];\n> -\n> - return OK;\n> -}\n> +static RegisterCamHelper reg(\"imx219\", &Create);\n> diff --git a/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> b/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> index 44f030ed7da9..7586e5f897d5 100644\n> --- a/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> +++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> @@ -15,21 +15,14 @@\n>\n> using namespace RPiController;\n>\n> -/* Metadata parser implementation specific to Sony IMX477 sensors. */\n> -\n> -class MdParserImx477 : public MdParserSmia\n> -{\n> -public:\n> - MdParserImx477();\n> - Status Parse(libcamera::Span buffer) override;\n> - Status GetExposureLines(unsigned int &lines) override;\n> - Status GetGainCode(unsigned int &gain_code) override;\n> -private:\n> - /* Offset of the register's value in the metadata block. */\n> - int reg_offsets_[4];\n> - /* Value of the register, once read from the metadata block. */\n> - int reg_values_[4];\n> -};\n> +/*\n> + * We care about two gain registers and a pair of exposure registers.\n> Their\n> + * I2C addresses from the Sony IMX477 datasheet:\n> + */\n> +constexpr uint32_t expHiReg = 0x0202;\n> +constexpr uint32_t expLoReg = 0x0203;\n> +constexpr uint32_t gainHiReg = 0x0204;\n> +constexpr uint32_t gainLoReg = 0x0205;\n>\n> class CamHelperImx477 : public CamHelper\n> {\n> @@ -48,11 +41,15 @@ private:\n> */\n> static constexpr int frameIntegrationDiff = 22;\n>\n> - MdParserImx477 imx477_parser;\n> + MdParserSmia imx477_parser;\n> +\n> + void PopulateMetadata(const MdParser::RegisterMap &map,\n> + Metadata &metadata) const override;\n> };\n>\n> CamHelperImx477::CamHelperImx477()\n> - : CamHelper(&imx477_parser, frameIntegrationDiff)\n> + : CamHelper(&imx477_parser, frameIntegrationDiff),\n> + imx477_parser({ expHiReg, expLoReg, gainHiReg, gainLoReg })\n> {\n> }\n>\n> @@ -79,95 +76,20 @@ bool CamHelperImx477::SensorEmbeddedDataPresent() const\n> return true;\n> }\n>\n> -static CamHelper *Create()\n> +void CamHelperImx477::PopulateMetadata(const MdParser::RegisterMap &map,\n> + Metadata &metadata) const\n> {\n> - return new CamHelperImx477();\n> -}\n> + DeviceStatus deviceStatus;\n>\n> -static RegisterCamHelper reg(\"imx477\", &Create);\n> + deviceStatus.shutter_speed = Exposure(map.at(expHiReg) * 256 +\n> map.at(expLoReg));\n> + deviceStatus.analogue_gain = Gain(map.at(gainHiReg) * 256 + map.at\n> (gainLoReg));\n>\n> -/*\n> - * We care about two gain registers and a pair of exposure registers.\n> Their\n> - * I2C addresses from the Sony IMX477 datasheet:\n> - */\n> -#define EXPHI_REG 0x0202\n> -#define EXPLO_REG 0x0203\n> -#define GAINHI_REG 0x0204\n> -#define GAINLO_REG 0x0205\n> -\n> -/*\n> - * Index of each into the reg_offsets and reg_values arrays. Must be in\n> register\n> - * address order.\n> - */\n> -#define EXPHI_INDEX 0\n> -#define EXPLO_INDEX 1\n> -#define GAINHI_INDEX 2\n> -#define GAINLO_INDEX 3\n> -\n> -MdParserImx477::MdParserImx477()\n> -{\n> - reg_offsets_[0] = reg_offsets_[1] = reg_offsets_[2] =\n> reg_offsets_[3] = -1;\n> + metadata.Set(\"device.status\", deviceStatus);\n> }\n>\n> -MdParser::Status MdParserImx477::Parse(libcamera::Span\n> buffer)\n> -{\n> - bool try_again = false;\n> -\n> - if (reset_) {\n> - /*\n> - * Search again through the metadata for the gain and\n> exposure\n> - * registers.\n> - */\n> - assert(bits_per_pixel_);\n> - /* Need to be ordered */\n> - uint32_t regs[4] = {\n> - EXPHI_REG,\n> - EXPLO_REG,\n> - GAINHI_REG,\n> - GAINLO_REG\n> - };\n> - reg_offsets_[0] = reg_offsets_[1] = reg_offsets_[2] =\n> reg_offsets_[3] = -1;\n> - int ret = static_cast(findRegs(buffer,\n> - regs, reg_offsets_,\n> 4));\n> - /*\n> - * > 0 means \"worked partially but parse again next time\",\n> - * < 0 means \"hard error\".\n> - */\n> - if (ret > 0)\n> - try_again = true;\n> - else if (ret < 0)\n> - return ERROR;\n> - }\n> -\n> - for (int i = 0; i < 4; i++) {\n> - if (reg_offsets_[i] == -1)\n> - continue;\n> -\n> - reg_values_[i] = buffer[reg_offsets_[i]];\n> - }\n> -\n> - /* Re-parse next time if we were unhappy in some way. */\n> - reset_ = try_again;\n> -\n> - return OK;\n> -}\n> -\n> -MdParser::Status MdParserImx477::GetExposureLines(unsigned int &lines)\n> +static CamHelper *Create()\n> {\n> - if (reg_offsets_[EXPHI_INDEX] == -1 || reg_offsets_[EXPLO_INDEX]\n> == -1)\n> - return NOTFOUND;\n> -\n> - lines = reg_values_[EXPHI_INDEX] * 256 + reg_values_[EXPLO_INDEX];\n> -\n> - return OK;\n> + return new CamHelperImx477();\n> }\n>\n> -MdParser::Status MdParserImx477::GetGainCode(unsigned int &gain_code)\n> -{\n> - if (reg_offsets_[GAINHI_INDEX] == -1 || reg_offsets_[GAINLO_INDEX]\n> == -1)\n> - return NOTFOUND;\n> -\n> - gain_code = reg_values_[GAINHI_INDEX] * 256 +\n> reg_values_[GAINLO_INDEX];\n> -\n> - return OK;\n> -}\n> +static RegisterCamHelper reg(\"imx477\", &Create);\n> diff --git a/src/ipa/raspberrypi/md_parser.hpp\n> b/src/ipa/raspberrypi/md_parser.hpp\n> index 65aab02d51b6..64f9bbb12845 100644\n> --- a/src/ipa/raspberrypi/md_parser.hpp\n> +++ b/src/ipa/raspberrypi/md_parser.hpp\n> @@ -8,6 +8,10 @@\n>\n> #include \n>\n> +#include \n> +#include \n> +#include \n> +\n> #include \n>\n> /*\n> @@ -19,7 +23,7 @@\n> * application code doesn't have to worry which kind to instantiate. But\n> for\n> * the sake of example let's suppose we're parsing imx219 metadata.\n> *\n> - * MdParser *parser = new MdParserImx219(); // for example\n> + * MdParser *parser = new MdParserSmia({ expHiReg, expLoReg, gainReg });\n> * parser->SetBitsPerPixel(bpp);\n> * parser->SetLineLengthBytes(pitch);\n> * parser->SetNumLines(2);\n> @@ -32,13 +36,11 @@\n> *\n> * Then on every frame:\n> *\n> - * if (parser->Parse(buffer) != MdParser::OK)\n> + * RegisterMap map;\n> + * if (parser->Parse(buffer, map) != 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> + * Metadata metadata;\n> + * CamHelper::PopulateMetadata(map, metadata);\n> *\n> * (Note that the CamHelper class converts to/from exposure lines and\n> time,\n> * and gain_code / actual gain.)\n> @@ -59,6 +61,8 @@ namespace RPiController {\n> class MdParser\n> {\n> public:\n> + using RegisterMap = std::map;\n> +\n> /*\n> * Parser status codes:\n> * OK - success\n> @@ -98,9 +102,8 @@ public:\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> + virtual Status Parse(libcamera::Span buffer,\n> + RegisterMap &map) = 0;\n>\n> protected:\n> bool reset_;\n> @@ -116,14 +119,18 @@ protected:\n> * md_parser_imx219.cpp for an example).\n> */\n>\n> -class MdParserSmia : public MdParser\n> +class MdParserSmia final : public MdParser\n> {\n> public:\n> - MdParserSmia() : MdParser()\n> - {\n> - }\n> + MdParserSmia(const std::vector ®s);\n> +\n> + MdParser::Status Parse(libcamera::Span buffer,\n> + RegisterMap &map) override;\n> +\n> +private:\n> + /* Maps register address to offset in the buffer. */\n> + using OffsetMap = std::map>;\n>\n> -protected:\n> /*\n> * Note that error codes > 0 are regarded as non-fatal; codes < 0\n> * indicate a bad data buffer. Status codes are:\n> @@ -141,8 +148,9 @@ protected:\n> BAD_PADDING = -5\n> };\n>\n> - ParseStatus findRegs(libcamera::Span buffer,\n> uint32_t regs[],\n> - int offsets[], unsigned int num_regs);\n> + ParseStatus findRegs(libcamera::Span buffer);\n> +\n> + OffsetMap offsets_;\n> };\n>\n> } // namespace RPi\n> diff --git a/src/ipa/raspberrypi/md_parser_smia.cpp\n> b/src/ipa/raspberrypi/md_parser_smia.cpp\n> index 0a14875575a2..9920699dfe25 100644\n> --- a/src/ipa/raspberrypi/md_parser_smia.cpp\n> +++ b/src/ipa/raspberrypi/md_parser_smia.cpp\n> @@ -6,9 +6,11 @@\n> */\n> #include \n>\n> +#include \"libcamera/internal/log.h\"\n> #include \"md_parser.hpp\"\n>\n> using namespace RPiController;\n> +using namespace libcamera;\n>\n> /*\n> * This function goes through the embedded data to find the offsets (not\n> @@ -26,18 +28,61 @@ constexpr unsigned int REG_LOW_BITS = 0xa5;\n> constexpr unsigned int REG_VALUE = 0x5a;\n> constexpr unsigned int REG_SKIP = 0x55;\n>\n> -MdParserSmia::ParseStatus MdParserSmia::findRegs(libcamera::Span uint8_t> buffer,\n> - uint32_t regs[], int\n> offsets[],\n> - unsigned int num_regs)\n> +MdParserSmia::MdParserSmia(const std::vector ®isters)\n> {\n> - assert(num_regs > 0);\n> + for (auto r : registers)\n> + offsets_[r] = {};\n> +}\n> +\n> +MdParser::Status MdParserSmia::Parse(libcamera::Span\n> buffer,\n> + RegisterMap &map)\n> +{\n> + if (reset_) {\n> + /*\n> + * Search again through the metadata for the gain and\n> exposure\n> + * registers.\n> + */\n> + ASSERT(bits_per_pixel_);\n> +\n> + for (auto &kv : offsets_)\n> + offsets_[kv.first] = {};\n> +\n> + ParseStatus ret = findRegs(buffer);\n> + /*\n> + * > 0 means \"worked partially but parse again next time\",\n> + * < 0 means \"hard error\".\n> + *\n> + * In either case, we retry parsing on the next frame.\n> + */\n> + if (ret != PARSE_OK)\n> + return ERROR;\n> +\n> + reset_ = false;\n> + }\n> +\n> + /* Populate the register values requested. */\n> + map.clear();\n> + for (auto &kv : offsets_) {\n> + if (!kv.second) {\n> + reset_ = true;\n> + return NOTFOUND;\n> + }\n> + map[kv.first] = buffer[kv.second.value()];\n> + }\n> +\n> + return OK;\n> +}\n> +\n> +MdParserSmia::ParseStatus MdParserSmia::findRegs(libcamera::Span uint8_t> buffer)\n> +{\n> + ASSERT(offsets_.size());\n>\n> if (buffer[0] != LINE_START)\n> return NO_LINE_START;\n>\n> unsigned int current_offset = 1; /* after the LINE_START */\n> unsigned int current_line_start = 0, current_line = 0;\n> - unsigned int reg_num = 0, first_reg = 0;\n> + unsigned int reg_num = 0, regs_done = 0;\n>\n> while (1) {\n> int tag = buffer[current_offset++];\n> @@ -89,13 +134,12 @@ MdParserSmia::ParseStatus\n> MdParserSmia::findRegs(libcamera::Span\n> else if (tag == REG_SKIP)\n> reg_num++;\n> else if (tag == REG_VALUE) {\n> - while (reg_num >=\n> - /* assumes registers are in\n> order... */\n> - regs[first_reg]) {\n> - if (reg_num == regs[first_reg])\n> - offsets[first_reg] =\n> current_offset - 1;\n> + auto reg = offsets_.find(reg_num);\n> +\n> + if (reg != offsets_.end()) {\n> + offsets_[reg_num] = current_offset\n> - 1;\n>\n> - if (++first_reg == num_regs)\n> + if (++regs_done == offsets_.size())\n> return PARSE_OK;\n> }\n> reg_num++;\n> --\n> 2.25.1\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 39144C321B\n\tfor ;\n\tTue, 22 Jun 2021 09:07:02 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9B0536893B;\n\tTue, 22 Jun 2021 11:07:01 +0200 (CEST)","from mail-lj1-x22e.google.com (mail-lj1-x22e.google.com\n\t[IPv6:2a00:1450:4864:20::22e])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 233906050B\n\tfor ;\n\tTue, 22 Jun 2021 11:07:00 +0200 (CEST)","by mail-lj1-x22e.google.com with SMTP id s22so29094680ljg.5\n\tfor ;\n\tTue, 22 Jun 2021 02:07:00 -0700 (PDT)"],"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=\"U+lsDj3+\"; 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\tbh=6f0A1hKxfRtpsfmOlVf7YC0mL9GeTcXZh/JtW17hul0=;\n\tb=U+lsDj3+d1I8hSuMFKjHUQ7dzNgbPgAeGJ+gBsq0VUBAtjghY/U3BBND1AXWD2clu1\n\tKMJjcctBNonCaMSG28Pq9m/oUaDnPPDEs17hHmadn1QNvFdkRq7Esrnd/QqF6HiS6T5P\n\tz4RsljyqUetx7YdPrEAHtFRYQ1F5Ygb407Ldg7D97CZSsJP783Ee71I8N6ymSbA8rGlE\n\tlGKqyj4Z5SZbf9ganE6+piP90mQ+F/RrjI2DfTj9PDMcdNDWnpAQSO+51w4rbTNCoUBN\n\tVv+DqvRYBsYnFrWRSWfPsP8YCzT6/YB3J+83zm8oMYhdUB0WGVCyOXPas7HN2cC/AbPQ\n\t77WA==","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;\n\tbh=6f0A1hKxfRtpsfmOlVf7YC0mL9GeTcXZh/JtW17hul0=;\n\tb=A4cxljbL5ixklUlhK+NRtc5QpXRJxb6M22csT0zaHeVMjQ8wQT2y0vx9HUaPfri90D\n\tjVKzk+HolhOKKXyqJvWD8tH9uxsbZ0ZEsXy5JV2yqwYQjPBo84W4ETlJvJVvsHoueFVB\n\t4ziL+j2dYKpus2dx/aWe7U6//r58hn+9gbDZ/AoCwwFh69td9G2gFzlkOGvhz0SUcrTh\n\tNin4WY8+v/hHAZgRwUI7Rk+9DAppqer3bNXW6sHsld1LeH2KVv6+u8NvvJUsL9dK40vD\n\tck0EUMCC2MzLS1iZ3nB7VtA9QtFY+/bT7RknzkHxRPtkXiZ6LNwK3OQizlPCXKGFlkKE\n\t/2Lg==","X-Gm-Message-State":"AOAM531ViZ+X5ryfdOYgAP7nMk7BBkTT7WCvnQKdUBuAb+/wwTWyUtxI\n\t6G+vEMjLRKmxgjRUwk+Pxks1JcrZLT2KNHZXdzB0g+PXwX8=","X-Google-Smtp-Source":"ABdhPJyEZoonvW/uascJRTbbWUMxrUiZ4T+AmyOMPoRL7If4aMQNxK8p3HnSrLbzYBEcnLCI5thVQmqLXK2CjBZ8zus=","X-Received":"by 2002:a2e:b711:: with SMTP id\n\tj17mr2307763ljo.329.1624352819244; \n\tTue, 22 Jun 2021 02:06:59 -0700 (PDT)","MIME-Version":"1.0","References":"<20210615144211.173047-1-naush@raspberrypi.com>\n\t<20210615144211.173047-4-naush@raspberrypi.com>","In-Reply-To":"<20210615144211.173047-4-naush@raspberrypi.com>","From":"Naushir Patuck ","Date":"Tue, 22 Jun 2021 10:06:43 +0100","Message-ID":"","To":"libcamera devel ","Content-Type":"multipart/alternative; boundary=\"000000000000c8041105c5571c70\"","Subject":"Re: [libcamera-devel] [PATCH 3/3] ipa: raspberrypi: Generalise the\n\tSMIA metadata parser","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17695,"web_url":"https://patchwork.libcamera.org/comment/17695/","msgid":"","date":"2021-06-22T11:12:43","subject":"Re: [libcamera-devel] [PATCH 3/3] ipa: raspberrypi: Generalise the\n\tSMIA metadata parser","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nThank you for the patch.\n\nOn Tue, Jun 15, 2021 at 03:42:11PM +0100, Naushir Patuck wrote:\n> Instead of having each CamHelper subclass the MdParserSmia, change the\n> implementation of MdParserSmia to be more generic. The MdParserSmia now gets\n> given a list of registers to search for and helper functions are used to compute\n> exposure lines and gain codes from these registers.\n> \n> Update the imx219 and imx477 CamHelpers by using this new mechanism.\n> \n> Signed-off-by: Naushir Patuck \n> ---\n> src/ipa/raspberrypi/cam_helper.cpp | 35 +++---\n> src/ipa/raspberrypi/cam_helper.hpp | 2 +\n> src/ipa/raspberrypi/cam_helper_imx219.cpp | 118 ++++----------------\n> src/ipa/raspberrypi/cam_helper_imx477.cpp | 124 ++++------------------\n> src/ipa/raspberrypi/md_parser.hpp | 42 +++++---\n> src/ipa/raspberrypi/md_parser_smia.cpp | 66 ++++++++++--\n> 6 files changed, 148 insertions(+), 239 deletions(-)\n> \n> diff --git a/src/ipa/raspberrypi/cam_helper.cpp b/src/ipa/raspberrypi/cam_helper.cpp\n> index 1474464c9257..41e292ecbb61 100644\n> --- a/src/ipa/raspberrypi/cam_helper.cpp\n> +++ b/src/ipa/raspberrypi/cam_helper.cpp\n> @@ -159,32 +159,32 @@ unsigned int CamHelper::MistrustFramesModeSwitch() const\n> void CamHelper::parseEmbeddedData(Span buffer,\n> \t\t\t\t Metadata &metadata)\n> {\n> +\tMdParser::RegisterMap map;\n> +\tMetadata parsedMetadata;\n> +\n> \tif (buffer.empty())\n> \t\treturn;\n> \n> -\tuint32_t exposureLines, gainCode;\n> -\n> -\tif (parser_->Parse(buffer) != MdParser::Status::OK ||\n> -\t parser_->GetExposureLines(exposureLines) != MdParser::Status::OK ||\n> -\t parser_->GetGainCode(gainCode) != MdParser::Status::OK) {\n> +\tif (parser_->Parse(buffer, map) != MdParser::Status::OK) {\n> \t\tLOG(IPARPI, Error) << \"Embedded data buffer parsing failed\";\n> \t\treturn;\n> \t}\n> \n> +\tPopulateMetadata(map, parsedMetadata);\n> +\tmetadata.Merge(parsedMetadata);\n> +\n> \t/*\n> -\t * Overwrite the exposure/gain values in the DeviceStatus, as\n> -\t * we know better. Fetch it first in case any other fields were\n> -\t * set meaningfully.\n> +\t * Overwrite the exposure/gain values in the existing DeviceStatus with\n> +\t * values from the parsed embedded buffer. Fetch it first in case any\n> +\t * other fields were set meaningfully.\n> \t */\n> -\tDeviceStatus deviceStatus;\n> -\n> -\tif (metadata.Get(\"device.status\", deviceStatus) != 0) {\n> -\t\tLOG(IPARPI, Error) << \"DeviceStatus not found\";\n\nNo log anymore ?\n\n> +\tDeviceStatus deviceStatus, parsedDeviceStatus;\n> +\tif (metadata.Get(\"device.status\", deviceStatus) ||\n> +\t parsedMetadata.Get(\"device.status\", parsedDeviceStatus))\n> \t\treturn;\n> -\t}\n> \n> -\tdeviceStatus.shutter_speed = Exposure(exposureLines);\n> -\tdeviceStatus.analogue_gain = Gain(gainCode);\n> +\tdeviceStatus.shutter_speed = parsedDeviceStatus.shutter_speed;\n> +\tdeviceStatus.analogue_gain = parsedDeviceStatus.analogue_gain;\n> \n> \tLOG(IPARPI, Debug) << \"Metadata updated - Exposure : \"\n> \t\t\t << deviceStatus.shutter_speed\n> @@ -194,6 +194,11 @@ void CamHelper::parseEmbeddedData(Span buffer,\n> \tmetadata.Set(\"device.status\", deviceStatus);\n> }\n> \n> +void CamHelper::PopulateMetadata([[maybe_unused]] const MdParser::RegisterMap &map,\n> +\t\t\t\t [[maybe_unused]] Metadata &metadata) const\n> +{\n> +}\n> +\n> RegisterCamHelper::RegisterCamHelper(char const *cam_name,\n> \t\t\t\t CamHelperCreateFunc create_func)\n> {\n> diff --git a/src/ipa/raspberrypi/cam_helper.hpp b/src/ipa/raspberrypi/cam_helper.hpp\n> index f53f5c39b01c..30879e60b861 100644\n> --- a/src/ipa/raspberrypi/cam_helper.hpp\n> +++ b/src/ipa/raspberrypi/cam_helper.hpp\n> @@ -91,6 +91,8 @@ public:\n> protected:\n> \tvoid parseEmbeddedData(libcamera::Span buffer,\n> \t\t\t Metadata &metadata);\n> +\tvirtual void PopulateMetadata([[maybe_unused]] const MdParser::RegisterMap &map,\n> +\t\t\t\t [[maybe_unused]] Metadata &metadata) const;\n> \n> \tMdParser *parser_;\n> \tCameraMode mode_;\n> diff --git a/src/ipa/raspberrypi/cam_helper_imx219.cpp b/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> index d951cd552a21..24818e9ec4de 100644\n> --- a/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> +++ b/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> @@ -23,21 +23,13 @@\n> \n> using namespace RPiController;\n> \n> -/* Metadata parser implementation specific to Sony IMX219 sensors. */\n> -\n> -class MdParserImx219 : public MdParserSmia\n> -{\n> -public:\n> -\tMdParserImx219();\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> -\t/* Offset of the register's value in the metadata block. */\n> -\tint reg_offsets_[3];\n> -\t/* Value of the register, once read from the metadata block. */\n> -\tint reg_values_[3];\n> -};\n> +/*\n> + * We care about one gain register and a pair of exposure registers. Their I2C\n> + * addresses from the Sony IMX219 datasheet:\n> + */\n> +constexpr uint32_t gainReg = 0x157;\n> +constexpr uint32_t expHiReg = 0x15a;\n> +constexpr uint32_t expLoReg = 0x15b;\n> \n> class CamHelperImx219 : public CamHelper\n> {\n> @@ -55,15 +47,19 @@ private:\n> \t */\n> \tstatic constexpr int frameIntegrationDiff = 4;\n> \n> -\tMdParserImx219 imx219_parser;\n> +\tMdParserSmia imx219_parser;\n\nCould you name this just \"parser\" ? It's a member of the CamHelperImx219\nclass, there's no need to repeat the prefix. Same for imx477.\n\n> +\n> +\tvoid PopulateMetadata(const MdParser::RegisterMap &map,\n> +\t\t\t Metadata &metadata) const override;\n> };\n> \n> CamHelperImx219::CamHelperImx219()\n> #if ENABLE_EMBEDDED_DATA\n> -\t: CamHelper(&imx219_parser, frameIntegrationDiff)\n> +\t: CamHelper(&imx219_parser, frameIntegrationDiff),\n\nYou're passing a pointer to an object that isn't constructed yet. The\nbase class only stores it so it should be OK, but it's a bit fragile.\n\n> #else\n> -\t: CamHelper(nullptr, frameIntegrationDiff)\n> +\t: CamHelper(nullptr, frameIntegrationDiff),\n> #endif\n> +\t imx219_parser({ gainReg, expHiReg, expLoReg })\n> {\n> }\n> \n> @@ -92,88 +88,20 @@ bool CamHelperImx219::SensorEmbeddedDataPresent() const\n> \treturn ENABLE_EMBEDDED_DATA;\n> }\n> \n> -static CamHelper *Create()\n> +void CamHelperImx219::PopulateMetadata(const MdParser::RegisterMap &map,\n> +\t\t\t\t Metadata &metadata) const\n> {\n> -\treturn new CamHelperImx219();\n> -}\n> +\tDeviceStatus deviceStatus;\n> \n> -static RegisterCamHelper reg(\"imx219\", &Create);\n> +\tdeviceStatus.shutter_speed = Exposure(map.at(expHiReg) * 256 + map.at(expLoReg));\n> +\tdeviceStatus.analogue_gain = Gain(map.at(gainReg));\n> \n> -/*\n> - * We care about one gain register and a pair of exposure registers. Their I2C\n> - * addresses from the Sony IMX219 datasheet:\n> - */\n> -#define GAIN_REG 0x157\n> -#define EXPHI_REG 0x15A\n> -#define EXPLO_REG 0x15B\n> -\n> -/*\n> - * Index of each into the reg_offsets and reg_values arrays. Must be in\n> - * register address order.\n> - */\n> -#define GAIN_INDEX 0\n> -#define EXPHI_INDEX 1\n> -#define EXPLO_INDEX 2\n> -\n> -MdParserImx219::MdParserImx219()\n> -{\n> -\treg_offsets_[0] = reg_offsets_[1] = reg_offsets_[2] = -1;\n> +\tmetadata.Set(\"device.status\", deviceStatus);\n> }\n> \n> -MdParser::Status MdParserImx219::Parse(libcamera::Span buffer)\n> -{\n> -\tbool try_again = false;\n> -\n> -\tif (reset_) {\n> -\t\t/*\n> -\t\t * Search again through the metadata for the gain and exposure\n> -\t\t * registers.\n> -\t\t */\n> -\t\tassert(bits_per_pixel_);\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(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> -\t\t * < 0 means \"hard error\".\n> -\t\t */\n> -\t\tif (ret > 0)\n> -\t\t\ttry_again = true;\n> -\t\telse if (ret < 0)\n> -\t\t\treturn ERROR;\n> -\t}\n> -\n> -\tfor (int i = 0; i < 3; i++) {\n> -\t\tif (reg_offsets_[i] == -1)\n> -\t\t\tcontinue;\n> -\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> -\treset_ = try_again;\n> -\n> -\treturn OK;\n> -}\n> -\n> -MdParser::Status MdParserImx219::GetExposureLines(unsigned int &lines)\n> +static CamHelper *Create()\n> {\n> -\tif (reg_offsets_[EXPHI_INDEX] == -1 || reg_offsets_[EXPLO_INDEX] == -1)\n> -\t\treturn NOTFOUND;\n> -\n> -\tlines = reg_values_[EXPHI_INDEX] * 256 + reg_values_[EXPLO_INDEX];\n> -\n> -\treturn OK;\n> +\treturn new CamHelperImx219();\n> }\n> \n> -MdParser::Status MdParserImx219::GetGainCode(unsigned int &gain_code)\n> -{\n> -\tif (reg_offsets_[GAIN_INDEX] == -1)\n> -\t\treturn NOTFOUND;\n> -\n> -\tgain_code = reg_values_[GAIN_INDEX];\n> -\n> -\treturn OK;\n> -}\n> +static RegisterCamHelper reg(\"imx219\", &Create);\n> diff --git a/src/ipa/raspberrypi/cam_helper_imx477.cpp b/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> index 44f030ed7da9..7586e5f897d5 100644\n> --- a/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> +++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> @@ -15,21 +15,14 @@\n> \n> using namespace RPiController;\n> \n> -/* Metadata parser implementation specific to Sony IMX477 sensors. */\n> -\n> -class MdParserImx477 : public MdParserSmia\n> -{\n> -public:\n> -\tMdParserImx477();\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> -\t/* Offset of the register's value in the metadata block. */\n> -\tint reg_offsets_[4];\n> -\t/* Value of the register, once read from the metadata block. */\n> -\tint reg_values_[4];\n> -};\n> +/*\n> + * We care about two gain registers and a pair of exposure registers. Their\n> + * I2C addresses from the Sony IMX477 datasheet:\n> + */\n> +constexpr uint32_t expHiReg = 0x0202;\n> +constexpr uint32_t expLoReg = 0x0203;\n> +constexpr uint32_t gainHiReg = 0x0204;\n> +constexpr uint32_t gainLoReg = 0x0205;\n> \n> class CamHelperImx477 : public CamHelper\n> {\n> @@ -48,11 +41,15 @@ private:\n> \t */\n> \tstatic constexpr int frameIntegrationDiff = 22;\n> \n> -\tMdParserImx477 imx477_parser;\n> +\tMdParserSmia imx477_parser;\n> +\n> +\tvoid PopulateMetadata(const MdParser::RegisterMap &map,\n> +\t\t\t Metadata &metadata) const override;\n> };\n> \n> CamHelperImx477::CamHelperImx477()\n> -\t: CamHelper(&imx477_parser, frameIntegrationDiff)\n> +\t: CamHelper(&imx477_parser, frameIntegrationDiff),\n> +\t imx477_parser({ expHiReg, expLoReg, gainHiReg, gainLoReg })\n> {\n> }\n> \n> @@ -79,95 +76,20 @@ bool CamHelperImx477::SensorEmbeddedDataPresent() const\n> \treturn true;\n> }\n> \n> -static CamHelper *Create()\n> +void CamHelperImx477::PopulateMetadata(const MdParser::RegisterMap &map,\n> +\t\t\t\t Metadata &metadata) const\n> {\n> -\treturn new CamHelperImx477();\n> -}\n> +\tDeviceStatus deviceStatus;\n> \n> -static RegisterCamHelper reg(\"imx477\", &Create);\n> +\tdeviceStatus.shutter_speed = Exposure(map.at(expHiReg) * 256 + map.at(expLoReg));\n> +\tdeviceStatus.analogue_gain = Gain(map.at(gainHiReg) * 256 + map.at(gainLoReg));\n> \n> -/*\n> - * We care about two gain registers and a pair of exposure registers. Their\n> - * I2C addresses from the Sony IMX477 datasheet:\n> - */\n> -#define EXPHI_REG 0x0202\n> -#define EXPLO_REG 0x0203\n> -#define GAINHI_REG 0x0204\n> -#define GAINLO_REG 0x0205\n> -\n> -/*\n> - * Index of each into the reg_offsets and reg_values arrays. Must be in register\n> - * address order.\n> - */\n> -#define EXPHI_INDEX 0\n> -#define EXPLO_INDEX 1\n> -#define GAINHI_INDEX 2\n> -#define GAINLO_INDEX 3\n> -\n> -MdParserImx477::MdParserImx477()\n> -{\n> -\treg_offsets_[0] = reg_offsets_[1] = reg_offsets_[2] = reg_offsets_[3] = -1;\n> +\tmetadata.Set(\"device.status\", deviceStatus);\n> }\n> \n> -MdParser::Status MdParserImx477::Parse(libcamera::Span buffer)\n> -{\n> -\tbool try_again = false;\n> -\n> -\tif (reset_) {\n> -\t\t/*\n> -\t\t * Search again through the metadata for the gain and exposure\n> -\t\t * registers.\n> -\t\t */\n> -\t\tassert(bits_per_pixel_);\n> -\t\t/* Need to be ordered */\n> -\t\tuint32_t regs[4] = {\n> -\t\t\tEXPHI_REG,\n> -\t\t\tEXPLO_REG,\n> -\t\t\tGAINHI_REG,\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(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> -\t\t * < 0 means \"hard error\".\n> -\t\t */\n> -\t\tif (ret > 0)\n> -\t\t\ttry_again = true;\n> -\t\telse if (ret < 0)\n> -\t\t\treturn ERROR;\n> -\t}\n> -\n> -\tfor (int i = 0; i < 4; i++) {\n> -\t\tif (reg_offsets_[i] == -1)\n> -\t\t\tcontinue;\n> -\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> -\treset_ = try_again;\n> -\n> -\treturn OK;\n> -}\n> -\n> -MdParser::Status MdParserImx477::GetExposureLines(unsigned int &lines)\n> +static CamHelper *Create()\n> {\n> -\tif (reg_offsets_[EXPHI_INDEX] == -1 || reg_offsets_[EXPLO_INDEX] == -1)\n> -\t\treturn NOTFOUND;\n> -\n> -\tlines = reg_values_[EXPHI_INDEX] * 256 + reg_values_[EXPLO_INDEX];\n> -\n> -\treturn OK;\n> +\treturn new CamHelperImx477();\n> }\n> \n> -MdParser::Status MdParserImx477::GetGainCode(unsigned int &gain_code)\n> -{\n> -\tif (reg_offsets_[GAINHI_INDEX] == -1 || reg_offsets_[GAINLO_INDEX] == -1)\n> -\t\treturn NOTFOUND;\n> -\n> -\tgain_code = reg_values_[GAINHI_INDEX] * 256 + reg_values_[GAINLO_INDEX];\n> -\n> -\treturn OK;\n> -}\n> +static RegisterCamHelper reg(\"imx477\", &Create);\n> diff --git a/src/ipa/raspberrypi/md_parser.hpp b/src/ipa/raspberrypi/md_parser.hpp\n> index 65aab02d51b6..64f9bbb12845 100644\n> --- a/src/ipa/raspberrypi/md_parser.hpp\n> +++ b/src/ipa/raspberrypi/md_parser.hpp\n> @@ -8,6 +8,10 @@\n> \n> #include \n> \n> +#include \n> +#include \n> +#include \n> +\n> #include \n> \n> /*\n> @@ -19,7 +23,7 @@\n> * application code doesn't have to worry which 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> + * MdParser *parser = new MdParserSmia({ expHiReg, expLoReg, gainReg });\n> * parser->SetBitsPerPixel(bpp);\n> * parser->SetLineLengthBytes(pitch);\n> * parser->SetNumLines(2);\n> @@ -32,13 +36,11 @@\n> *\n> * Then on every frame:\n> *\n> - * if (parser->Parse(buffer) != MdParser::OK)\n> + * RegisterMap map;\n> + * if (parser->Parse(buffer, map) != 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> + * Metadata metadata;\n> + * CamHelper::PopulateMetadata(map, metadata);\n> *\n> * (Note that the CamHelper class converts to/from exposure lines and time,\n> * and gain_code / actual gain.)\n> @@ -59,6 +61,8 @@ namespace RPiController {\n> class MdParser\n> {\n> public:\n> +\tusing RegisterMap = std::map;\n> +\n> \t/*\n> \t * Parser status codes:\n> \t * OK - success\n> @@ -98,9 +102,8 @@ public:\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> +\tvirtual Status Parse(libcamera::Span buffer,\n> +\t\t\t RegisterMap &map) = 0;\n\nI'd name the variable registers to tell what it contains (same in the\ncaller and in the example above).\n\n> \n> protected:\n> \tbool reset_;\n> @@ -116,14 +119,18 @@ protected:\n> * md_parser_imx219.cpp for an example).\n> */\n> \n> -class MdParserSmia : public MdParser\n> +class MdParserSmia final : public MdParser\n> {\n> public:\n> -\tMdParserSmia() : MdParser()\n> -\t{\n> -\t}\n> +\tMdParserSmia(const std::vector ®s);\n> +\n> +\tMdParser::Status Parse(libcamera::Span buffer,\n> +\t\t\t RegisterMap &map) override;\n> +\n> +private:\n> +\t/* Maps register address to offset in the buffer. */\n> +\tusing OffsetMap = std::map>;\n> \n> -protected:\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> @@ -141,8 +148,9 @@ protected:\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> +\tParseStatus findRegs(libcamera::Span buffer);\n> +\n> +\tOffsetMap offsets_;\n> };\n> \n> } // namespace RPi\n> diff --git a/src/ipa/raspberrypi/md_parser_smia.cpp b/src/ipa/raspberrypi/md_parser_smia.cpp\n> index 0a14875575a2..9920699dfe25 100644\n> --- a/src/ipa/raspberrypi/md_parser_smia.cpp\n> +++ b/src/ipa/raspberrypi/md_parser_smia.cpp\n> @@ -6,9 +6,11 @@\n> */\n> #include \n> \n> +#include \"libcamera/internal/log.h\"\n\nDo you need this header ?\n\n> #include \"md_parser.hpp\"\n> \n> using namespace RPiController;\n> +using namespace libcamera;\n> \n> /*\n> * This function goes through the embedded data to find the offsets (not\n> @@ -26,18 +28,61 @@ constexpr unsigned int REG_LOW_BITS = 0xa5;\n> constexpr unsigned int REG_VALUE = 0x5a;\n> constexpr unsigned int REG_SKIP = 0x55;\n> \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> +MdParserSmia::MdParserSmia(const std::vector ®isters)\n> {\n> -\tassert(num_regs > 0);\n> +\tfor (auto r : registers)\n> +\t\toffsets_[r] = {};\n> +}\n> +\n> +MdParser::Status MdParserSmia::Parse(libcamera::Span buffer,\n> +\t\t\t\t RegisterMap &map)\n> +{\n> +\tif (reset_) {\n> +\t\t/*\n> +\t\t * Search again through the metadata for the gain and exposure\n> +\t\t * registers.\n> +\t\t */\n\nIt's not just gain and exposure, this class is generic and can handle\nany register.\n\nI'm curious, is findRegs() that costly that you want to cache the\nresults ? Does SMIA++ guarantee that embedded data will be formatted the\nsame way for every frame ?\n\nI've pushed 2/3 already. 1/3 looks good to me, but I've held off in case\nsome of the review for 3/3 requires changing 1/3.\n\n> +\t\tASSERT(bits_per_pixel_);\n> +\n> +\t\tfor (auto &kv : offsets_)\n> +\t\t\toffsets_[kv.first] = {};\n> +\n> +\t\tParseStatus ret = findRegs(buffer);\n> +\t\t/*\n> +\t\t * > 0 means \"worked partially but parse again next time\",\n> +\t\t * < 0 means \"hard error\".\n> +\t\t *\n> +\t\t * In either case, we retry parsing on the next frame.\n> +\t\t */\n> +\t\tif (ret != PARSE_OK)\n> +\t\t\treturn ERROR;\n> +\n> +\t\treset_ = false;\n> +\t}\n> +\n> +\t/* Populate the register values requested. */\n> +\tmap.clear();\n> +\tfor (auto &kv : offsets_) {\n> +\t\tif (!kv.second) {\n> +\t\t\treset_ = true;\n> +\t\t\treturn NOTFOUND;\n> +\t\t}\n> +\t\tmap[kv.first] = buffer[kv.second.value()];\n> +\t}\n> +\n> +\treturn OK;\n> +}\n> +\n> +MdParserSmia::ParseStatus MdParserSmia::findRegs(libcamera::Span buffer)\n> +{\n> +\tASSERT(offsets_.size());\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> \tunsigned int current_line_start = 0, current_line = 0;\n> -\tunsigned int reg_num = 0, first_reg = 0;\n> +\tunsigned int reg_num = 0, regs_done = 0;\n> \n> \twhile (1) {\n> \t\tint tag = buffer[current_offset++];\n> @@ -89,13 +134,12 @@ MdParserSmia::ParseStatus MdParserSmia::findRegs(libcamera::Span\n> \t\t\telse if (tag == REG_SKIP)\n> \t\t\t\treg_num++;\n> \t\t\telse if (tag == REG_VALUE) {\n> -\t\t\t\twhile (reg_num >=\n> -\t\t\t\t /* assumes registers are in order... */\n> -\t\t\t\t regs[first_reg]) {\n> -\t\t\t\t\tif (reg_num == regs[first_reg])\n> -\t\t\t\t\t\toffsets[first_reg] = current_offset - 1;\n> +\t\t\t\tauto reg = offsets_.find(reg_num);\n> +\n> +\t\t\t\tif (reg != offsets_.end()) {\n> +\t\t\t\t\toffsets_[reg_num] = current_offset - 1;\n> \n> -\t\t\t\t\tif (++first_reg == num_regs)\n> +\t\t\t\t\tif (++regs_done == offsets_.size())\n> \t\t\t\t\t\treturn PARSE_OK;\n> \t\t\t\t}\n> \t\t\t\treg_num++;","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 43AFEC321A\n\tfor ;\n\tTue, 22 Jun 2021 11:13:13 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6BFD068932;\n\tTue, 22 Jun 2021 13:13:12 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CA19E60292\n\tfor ;\n\tTue, 22 Jun 2021 13:13:11 +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 3B010A66;\n\tTue, 22 Jun 2021 13:13:11 +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=\"FK0OFm6M\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1624360391;\n\tbh=EwM4nmtlsbmkVM2Ti0tAyuQ9GaD1YekdiT+OUt7Rjig=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=FK0OFm6M4LoL6MXd+q2g+Rhg8/OfDt40yLZ91rBwF37fKCj0hUgIZozW8rolJ3XLH\n\tGsqlLrGAkxyZWP6MAZ0Thj8JvNyrUlII6BhGcHVTOz4Q0N+ls8peMmsT90pylv4ki5\n\tHco7hqcbcE58gJFt2CEqBk1YT36ee8+32m5C1hjw=","Date":"Tue, 22 Jun 2021 14:12:43 +0300","From":"Laurent Pinchart ","To":"Naushir Patuck ","Message-ID":"","References":"<20210615144211.173047-1-naush@raspberrypi.com>\n\t<20210615144211.173047-4-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210615144211.173047-4-naush@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH 3/3] ipa: raspberrypi: Generalise the\n\tSMIA metadata parser","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":17696,"web_url":"https://patchwork.libcamera.org/comment/17696/","msgid":"","date":"2021-06-22T12:07:10","subject":"Re: [libcamera-devel] [PATCH 3/3] ipa: raspberrypi: Generalise the\n\tSMIA metadata parser","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Laurent,\n\nThank you for your feedback.\n\nOn Tue, 22 Jun 2021 at 12:13, Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi Naush,\n>\n> Thank you for the patch.\n>\n> On Tue, Jun 15, 2021 at 03:42:11PM +0100, Naushir Patuck wrote:\n> > Instead of having each CamHelper subclass the MdParserSmia, change the\n> > implementation of MdParserSmia to be more generic. The MdParserSmia now\n> gets\n> > given a list of registers to search for and helper functions are used to\n> compute\n> > exposure lines and gain codes from these registers.\n> >\n> > Update the imx219 and imx477 CamHelpers by using this new mechanism.\n> >\n> > Signed-off-by: Naushir Patuck \n> > ---\n> > src/ipa/raspberrypi/cam_helper.cpp | 35 +++---\n> > src/ipa/raspberrypi/cam_helper.hpp | 2 +\n> > src/ipa/raspberrypi/cam_helper_imx219.cpp | 118 ++++----------------\n> > src/ipa/raspberrypi/cam_helper_imx477.cpp | 124 ++++------------------\n> > src/ipa/raspberrypi/md_parser.hpp | 42 +++++---\n> > src/ipa/raspberrypi/md_parser_smia.cpp | 66 ++++++++++--\n> > 6 files changed, 148 insertions(+), 239 deletions(-)\n> >\n> > diff --git a/src/ipa/raspberrypi/cam_helper.cpp\n> b/src/ipa/raspberrypi/cam_helper.cpp\n> > index 1474464c9257..41e292ecbb61 100644\n> > --- a/src/ipa/raspberrypi/cam_helper.cpp\n> > +++ b/src/ipa/raspberrypi/cam_helper.cpp\n> > @@ -159,32 +159,32 @@ unsigned int CamHelper::MistrustFramesModeSwitch()\n> const\n> > void CamHelper::parseEmbeddedData(Span buffer,\n> > Metadata &metadata)\n> > {\n> > + MdParser::RegisterMap map;\n> > + Metadata parsedMetadata;\n> > +\n> > if (buffer.empty())\n> > return;\n> >\n> > - uint32_t exposureLines, gainCode;\n> > -\n> > - if (parser_->Parse(buffer) != MdParser::Status::OK ||\n> > - parser_->GetExposureLines(exposureLines) !=\n> MdParser::Status::OK ||\n> > - parser_->GetGainCode(gainCode) != MdParser::Status::OK) {\n> > + if (parser_->Parse(buffer, map) != MdParser::Status::OK) {\n> > LOG(IPARPI, Error) << \"Embedded data buffer parsing\n> failed\";\n> > return;\n> > }\n> >\n> > + PopulateMetadata(map, parsedMetadata);\n> > + metadata.Merge(parsedMetadata);\n> > +\n> > /*\n> > - * Overwrite the exposure/gain values in the DeviceStatus, as\n> > - * we know better. Fetch it first in case any other fields were\n> > - * set meaningfully.\n> > + * Overwrite the exposure/gain values in the existing DeviceStatus\n> with\n> > + * values from the parsed embedded buffer. Fetch it first in case\n> any\n> > + * other fields were set meaningfully.\n> > */\n> > - DeviceStatus deviceStatus;\n> > -\n> > - if (metadata.Get(\"device.status\", deviceStatus) != 0) {\n> > - LOG(IPARPI, Error) << \"DeviceStatus not found\";\n>\n> No log anymore ?\n>\n\nAck, I'll add it back below.\n\n\n>\n> > + DeviceStatus deviceStatus, parsedDeviceStatus;\n> > + if (metadata.Get(\"device.status\", deviceStatus) ||\n> > + parsedMetadata.Get(\"device.status\", parsedDeviceStatus))\n> > return;\n> > - }\n> >\n> > - deviceStatus.shutter_speed = Exposure(exposureLines);\n> > - deviceStatus.analogue_gain = Gain(gainCode);\n> > + deviceStatus.shutter_speed = parsedDeviceStatus.shutter_speed;\n> > + deviceStatus.analogue_gain = parsedDeviceStatus.analogue_gain;\n> >\n> > LOG(IPARPI, Debug) << \"Metadata updated - Exposure : \"\n> > << deviceStatus.shutter_speed\n> > @@ -194,6 +194,11 @@ void CamHelper::parseEmbeddedData(Span uint8_t> buffer,\n> > metadata.Set(\"device.status\", deviceStatus);\n> > }\n> >\n> > +void CamHelper::PopulateMetadata([[maybe_unused]] const\n> MdParser::RegisterMap &map,\n> > + [[maybe_unused]] Metadata &metadata) const\n> > +{\n> > +}\n> > +\n> > RegisterCamHelper::RegisterCamHelper(char const *cam_name,\n> > CamHelperCreateFunc create_func)\n> > {\n> > diff --git a/src/ipa/raspberrypi/cam_helper.hpp\n> b/src/ipa/raspberrypi/cam_helper.hpp\n> > index f53f5c39b01c..30879e60b861 100644\n> > --- a/src/ipa/raspberrypi/cam_helper.hpp\n> > +++ b/src/ipa/raspberrypi/cam_helper.hpp\n> > @@ -91,6 +91,8 @@ public:\n> > protected:\n> > void parseEmbeddedData(libcamera::Span buffer,\n> > Metadata &metadata);\n> > + virtual void PopulateMetadata([[maybe_unused]] const\n> MdParser::RegisterMap &map,\n> > + [[maybe_unused]] Metadata &metadata)\n> const;\n> >\n> > MdParser *parser_;\n> > CameraMode mode_;\n> > diff --git a/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> b/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> > index d951cd552a21..24818e9ec4de 100644\n> > --- a/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> > +++ b/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> > @@ -23,21 +23,13 @@\n> >\n> > using namespace RPiController;\n> >\n> > -/* Metadata parser implementation specific to Sony IMX219 sensors. */\n> > -\n> > -class MdParserImx219 : public MdParserSmia\n> > -{\n> > -public:\n> > - MdParserImx219();\n> > - Status Parse(libcamera::Span buffer) override;\n> > - Status GetExposureLines(unsigned int &lines) override;\n> > - Status GetGainCode(unsigned int &gain_code) override;\n> > -private:\n> > - /* Offset of the register's value in the metadata block. */\n> > - int reg_offsets_[3];\n> > - /* Value of the register, once read from the metadata block. */\n> > - int reg_values_[3];\n> > -};\n> > +/*\n> > + * We care about one gain register and a pair of exposure registers.\n> Their I2C\n> > + * addresses from the Sony IMX219 datasheet:\n> > + */\n> > +constexpr uint32_t gainReg = 0x157;\n> > +constexpr uint32_t expHiReg = 0x15a;\n> > +constexpr uint32_t expLoReg = 0x15b;\n> >\n> > class CamHelperImx219 : public CamHelper\n> > {\n> > @@ -55,15 +47,19 @@ private:\n> > */\n> > static constexpr int frameIntegrationDiff = 4;\n> >\n> > - MdParserImx219 imx219_parser;\n> > + MdParserSmia imx219_parser;\n>\n> Could you name this just \"parser\" ? It's a member of the CamHelperImx219\n> class, there's no need to repeat the prefix. Same for imx477.\n>\n\nAck.\n\n\n>\n> > +\n> > + void PopulateMetadata(const MdParser::RegisterMap &map,\n> > + Metadata &metadata) const override;\n> > };\n> >\n> > CamHelperImx219::CamHelperImx219()\n> > #if ENABLE_EMBEDDED_DATA\n> > - : CamHelper(&imx219_parser, frameIntegrationDiff)\n> > + : CamHelper(&imx219_parser, frameIntegrationDiff),\n>\n> You're passing a pointer to an object that isn't constructed yet. The\n> base class only stores it so it should be OK, but it's a bit fragile.\n>\n\nI agree, this is slightly naughty. Without relying in a dynamic\nallocation, there is no other way to construct the object before\npassing on the pointer to the base class.\n\nPerhaps I should replace patch 1/3 to use a shared_ptr instead of\nembedding the parser object in the derived class. This way, the memory\nis correctly managed, and the object will be constructed before returning\nit to the base class.\n\n\n>\n> > #else\n> > - : CamHelper(nullptr, frameIntegrationDiff)\n> > + : CamHelper(nullptr, frameIntegrationDiff),\n> > #endif\n> > + imx219_parser({ gainReg, expHiReg, expLoReg })\n> > {\n> > }\n> >\n> > @@ -92,88 +88,20 @@ bool CamHelperImx219::SensorEmbeddedDataPresent()\n> const\n> > return ENABLE_EMBEDDED_DATA;\n> > }\n> >\n> > -static CamHelper *Create()\n> > +void CamHelperImx219::PopulateMetadata(const MdParser::RegisterMap &map,\n> > + Metadata &metadata) const\n> > {\n> > - return new CamHelperImx219();\n> > -}\n> > + DeviceStatus deviceStatus;\n> >\n> > -static RegisterCamHelper reg(\"imx219\", &Create);\n> > + deviceStatus.shutter_speed = Exposure(map.at(expHiReg) * 256 +\n> map.at(expLoReg));\n> > + deviceStatus.analogue_gain = Gain(map.at(gainReg));\n> >\n> > -/*\n> > - * We care about one gain register and a pair of exposure registers.\n> Their I2C\n> > - * addresses from the Sony IMX219 datasheet:\n> > - */\n> > -#define GAIN_REG 0x157\n> > -#define EXPHI_REG 0x15A\n> > -#define EXPLO_REG 0x15B\n> > -\n> > -/*\n> > - * Index of each into the reg_offsets and reg_values arrays. Must be in\n> > - * register address order.\n> > - */\n> > -#define GAIN_INDEX 0\n> > -#define EXPHI_INDEX 1\n> > -#define EXPLO_INDEX 2\n> > -\n> > -MdParserImx219::MdParserImx219()\n> > -{\n> > - reg_offsets_[0] = reg_offsets_[1] = reg_offsets_[2] = -1;\n> > + metadata.Set(\"device.status\", deviceStatus);\n> > }\n> >\n> > -MdParser::Status MdParserImx219::Parse(libcamera::Span\n> buffer)\n> > -{\n> > - bool try_again = false;\n> > -\n> > - if (reset_) {\n> > - /*\n> > - * Search again through the metadata for the gain and\n> exposure\n> > - * registers.\n> > - */\n> > - assert(bits_per_pixel_);\n> > - /* Need to be ordered */\n> > - uint32_t regs[3] = { GAIN_REG, EXPHI_REG, EXPLO_REG };\n> > - reg_offsets_[0] = reg_offsets_[1] = reg_offsets_[2] = -1;\n> > - int ret = static_cast(findRegs(buffer,\n> > - regs, reg_offsets_,\n> 3));\n> > - /*\n> > - * > 0 means \"worked partially but parse again next time\",\n> > - * < 0 means \"hard error\".\n> > - */\n> > - if (ret > 0)\n> > - try_again = true;\n> > - else if (ret < 0)\n> > - return ERROR;\n> > - }\n> > -\n> > - for (int i = 0; i < 3; i++) {\n> > - if (reg_offsets_[i] == -1)\n> > - continue;\n> > -\n> > - reg_values_[i] = buffer[reg_offsets_[i]];\n> > - }\n> > -\n> > - /* Re-parse next time if we were unhappy in some way. */\n> > - reset_ = try_again;\n> > -\n> > - return OK;\n> > -}\n> > -\n> > -MdParser::Status MdParserImx219::GetExposureLines(unsigned int &lines)\n> > +static CamHelper *Create()\n> > {\n> > - if (reg_offsets_[EXPHI_INDEX] == -1 || reg_offsets_[EXPLO_INDEX]\n> == -1)\n> > - return NOTFOUND;\n> > -\n> > - lines = reg_values_[EXPHI_INDEX] * 256 + reg_values_[EXPLO_INDEX];\n> > -\n> > - return OK;\n> > + return new CamHelperImx219();\n> > }\n> >\n> > -MdParser::Status MdParserImx219::GetGainCode(unsigned int &gain_code)\n> > -{\n> > - if (reg_offsets_[GAIN_INDEX] == -1)\n> > - return NOTFOUND;\n> > -\n> > - gain_code = reg_values_[GAIN_INDEX];\n> > -\n> > - return OK;\n> > -}\n> > +static RegisterCamHelper reg(\"imx219\", &Create);\n> > diff --git a/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> b/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> > index 44f030ed7da9..7586e5f897d5 100644\n> > --- a/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> > +++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> > @@ -15,21 +15,14 @@\n> >\n> > using namespace RPiController;\n> >\n> > -/* Metadata parser implementation specific to Sony IMX477 sensors. */\n> > -\n> > -class MdParserImx477 : public MdParserSmia\n> > -{\n> > -public:\n> > - MdParserImx477();\n> > - Status Parse(libcamera::Span buffer) override;\n> > - Status GetExposureLines(unsigned int &lines) override;\n> > - Status GetGainCode(unsigned int &gain_code) override;\n> > -private:\n> > - /* Offset of the register's value in the metadata block. */\n> > - int reg_offsets_[4];\n> > - /* Value of the register, once read from the metadata block. */\n> > - int reg_values_[4];\n> > -};\n> > +/*\n> > + * We care about two gain registers and a pair of exposure registers.\n> Their\n> > + * I2C addresses from the Sony IMX477 datasheet:\n> > + */\n> > +constexpr uint32_t expHiReg = 0x0202;\n> > +constexpr uint32_t expLoReg = 0x0203;\n> > +constexpr uint32_t gainHiReg = 0x0204;\n> > +constexpr uint32_t gainLoReg = 0x0205;\n> >\n> > class CamHelperImx477 : public CamHelper\n> > {\n> > @@ -48,11 +41,15 @@ private:\n> > */\n> > static constexpr int frameIntegrationDiff = 22;\n> >\n> > - MdParserImx477 imx477_parser;\n> > + MdParserSmia imx477_parser;\n> > +\n> > + void PopulateMetadata(const MdParser::RegisterMap &map,\n> > + Metadata &metadata) const override;\n> > };\n> >\n> > CamHelperImx477::CamHelperImx477()\n> > - : CamHelper(&imx477_parser, frameIntegrationDiff)\n> > + : CamHelper(&imx477_parser, frameIntegrationDiff),\n> > + imx477_parser({ expHiReg, expLoReg, gainHiReg, gainLoReg })\n> > {\n> > }\n> >\n> > @@ -79,95 +76,20 @@ bool CamHelperImx477::SensorEmbeddedDataPresent()\n> const\n> > return true;\n> > }\n> >\n> > -static CamHelper *Create()\n> > +void CamHelperImx477::PopulateMetadata(const MdParser::RegisterMap &map,\n> > + Metadata &metadata) const\n> > {\n> > - return new CamHelperImx477();\n> > -}\n> > + DeviceStatus deviceStatus;\n> >\n> > -static RegisterCamHelper reg(\"imx477\", &Create);\n> > + deviceStatus.shutter_speed = Exposure(map.at(expHiReg) * 256 +\n> map.at(expLoReg));\n> > + deviceStatus.analogue_gain = Gain(map.at(gainHiReg) * 256 + map.at\n> (gainLoReg));\n> >\n> > -/*\n> > - * We care about two gain registers and a pair of exposure registers.\n> Their\n> > - * I2C addresses from the Sony IMX477 datasheet:\n> > - */\n> > -#define EXPHI_REG 0x0202\n> > -#define EXPLO_REG 0x0203\n> > -#define GAINHI_REG 0x0204\n> > -#define GAINLO_REG 0x0205\n> > -\n> > -/*\n> > - * Index of each into the reg_offsets and reg_values arrays. Must be in\n> register\n> > - * address order.\n> > - */\n> > -#define EXPHI_INDEX 0\n> > -#define EXPLO_INDEX 1\n> > -#define GAINHI_INDEX 2\n> > -#define GAINLO_INDEX 3\n> > -\n> > -MdParserImx477::MdParserImx477()\n> > -{\n> > - reg_offsets_[0] = reg_offsets_[1] = reg_offsets_[2] =\n> reg_offsets_[3] = -1;\n> > + metadata.Set(\"device.status\", deviceStatus);\n> > }\n> >\n> > -MdParser::Status MdParserImx477::Parse(libcamera::Span\n> buffer)\n> > -{\n> > - bool try_again = false;\n> > -\n> > - if (reset_) {\n> > - /*\n> > - * Search again through the metadata for the gain and\n> exposure\n> > - * registers.\n> > - */\n> > - assert(bits_per_pixel_);\n> > - /* Need to be ordered */\n> > - uint32_t regs[4] = {\n> > - EXPHI_REG,\n> > - EXPLO_REG,\n> > - GAINHI_REG,\n> > - GAINLO_REG\n> > - };\n> > - reg_offsets_[0] = reg_offsets_[1] = reg_offsets_[2] =\n> reg_offsets_[3] = -1;\n> > - int ret = static_cast(findRegs(buffer,\n> > - regs, reg_offsets_,\n> 4));\n> > - /*\n> > - * > 0 means \"worked partially but parse again next time\",\n> > - * < 0 means \"hard error\".\n> > - */\n> > - if (ret > 0)\n> > - try_again = true;\n> > - else if (ret < 0)\n> > - return ERROR;\n> > - }\n> > -\n> > - for (int i = 0; i < 4; i++) {\n> > - if (reg_offsets_[i] == -1)\n> > - continue;\n> > -\n> > - reg_values_[i] = buffer[reg_offsets_[i]];\n> > - }\n> > -\n> > - /* Re-parse next time if we were unhappy in some way. */\n> > - reset_ = try_again;\n> > -\n> > - return OK;\n> > -}\n> > -\n> > -MdParser::Status MdParserImx477::GetExposureLines(unsigned int &lines)\n> > +static CamHelper *Create()\n> > {\n> > - if (reg_offsets_[EXPHI_INDEX] == -1 || reg_offsets_[EXPLO_INDEX]\n> == -1)\n> > - return NOTFOUND;\n> > -\n> > - lines = reg_values_[EXPHI_INDEX] * 256 + reg_values_[EXPLO_INDEX];\n> > -\n> > - return OK;\n> > + return new CamHelperImx477();\n> > }\n> >\n> > -MdParser::Status MdParserImx477::GetGainCode(unsigned int &gain_code)\n> > -{\n> > - if (reg_offsets_[GAINHI_INDEX] == -1 || reg_offsets_[GAINLO_INDEX]\n> == -1)\n> > - return NOTFOUND;\n> > -\n> > - gain_code = reg_values_[GAINHI_INDEX] * 256 +\n> reg_values_[GAINLO_INDEX];\n> > -\n> > - return OK;\n> > -}\n> > +static RegisterCamHelper reg(\"imx477\", &Create);\n> > diff --git a/src/ipa/raspberrypi/md_parser.hpp\n> b/src/ipa/raspberrypi/md_parser.hpp\n> > index 65aab02d51b6..64f9bbb12845 100644\n> > --- a/src/ipa/raspberrypi/md_parser.hpp\n> > +++ b/src/ipa/raspberrypi/md_parser.hpp\n> > @@ -8,6 +8,10 @@\n> >\n> > #include \n> >\n> > +#include \n> > +#include \n> > +#include \n> > +\n> > #include \n> >\n> > /*\n> > @@ -19,7 +23,7 @@\n> > * application code doesn't have to worry which kind to instantiate.\n> But for\n> > * the sake of example let's suppose we're parsing imx219 metadata.\n> > *\n> > - * MdParser *parser = new MdParserImx219(); // for example\n> > + * MdParser *parser = new MdParserSmia({ expHiReg, expLoReg, gainReg });\n> > * parser->SetBitsPerPixel(bpp);\n> > * parser->SetLineLengthBytes(pitch);\n> > * parser->SetNumLines(2);\n> > @@ -32,13 +36,11 @@\n> > *\n> > * Then on every frame:\n> > *\n> > - * if (parser->Parse(buffer) != MdParser::OK)\n> > + * RegisterMap map;\n> > + * if (parser->Parse(buffer, map) != 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> > + * Metadata metadata;\n> > + * CamHelper::PopulateMetadata(map, metadata);\n> > *\n> > * (Note that the CamHelper class converts to/from exposure lines and\n> time,\n> > * and gain_code / actual gain.)\n> > @@ -59,6 +61,8 @@ namespace RPiController {\n> > class MdParser\n> > {\n> > public:\n> > + using RegisterMap = std::map;\n> > +\n> > /*\n> > * Parser status codes:\n> > * OK - success\n> > @@ -98,9 +102,8 @@ public:\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> > + virtual Status Parse(libcamera::Span buffer,\n> > + RegisterMap &map) = 0;\n>\n> I'd name the variable registers to tell what it contains (same in the\n> caller and in the example above).\n>\n\nAck.\n\n\n>\n> >\n> > protected:\n> > bool reset_;\n> > @@ -116,14 +119,18 @@ protected:\n> > * md_parser_imx219.cpp for an example).\n> > */\n> >\n> > -class MdParserSmia : public MdParser\n> > +class MdParserSmia final : public MdParser\n> > {\n> > public:\n> > - MdParserSmia() : MdParser()\n> > - {\n> > - }\n> > + MdParserSmia(const std::vector ®s);\n> > +\n> > + MdParser::Status Parse(libcamera::Span buffer,\n> > + RegisterMap &map) override;\n> > +\n> > +private:\n> > + /* Maps register address to offset in the buffer. */\n> > + using OffsetMap = std::map>;\n> >\n> > -protected:\n> > /*\n> > * Note that error codes > 0 are regarded as non-fatal; codes < 0\n> > * indicate a bad data buffer. Status codes are:\n> > @@ -141,8 +148,9 @@ protected:\n> > BAD_PADDING = -5\n> > };\n> >\n> > - ParseStatus findRegs(libcamera::Span buffer,\n> uint32_t regs[],\n> > - int offsets[], unsigned int num_regs);\n> > + ParseStatus findRegs(libcamera::Span buffer);\n> > +\n> > + OffsetMap offsets_;\n> > };\n> >\n> > } // namespace RPi\n> > diff --git a/src/ipa/raspberrypi/md_parser_smia.cpp\n> b/src/ipa/raspberrypi/md_parser_smia.cpp\n> > index 0a14875575a2..9920699dfe25 100644\n> > --- a/src/ipa/raspberrypi/md_parser_smia.cpp\n> > +++ b/src/ipa/raspberrypi/md_parser_smia.cpp\n> > @@ -6,9 +6,11 @@\n> > */\n> > #include \n> >\n> > +#include \"libcamera/internal/log.h\"\n>\n> Do you need this header ?\n>\n\nYes, I do for the ASSERT() macro below. I could use the standard assert()\nwithout\nthis header, but thought the libcamera version would be more consistent.\n\n\n>\n> > #include \"md_parser.hpp\"\n> >\n> > using namespace RPiController;\n> > +using namespace libcamera;\n> >\n> > /*\n> > * This function goes through the embedded data to find the offsets (not\n> > @@ -26,18 +28,61 @@ constexpr unsigned int REG_LOW_BITS = 0xa5;\n> > constexpr unsigned int REG_VALUE = 0x5a;\n> > constexpr unsigned int REG_SKIP = 0x55;\n> >\n> > -MdParserSmia::ParseStatus MdParserSmia::findRegs(libcamera::Span uint8_t> buffer,\n> > - uint32_t regs[], int\n> offsets[],\n> > - unsigned int num_regs)\n> > +MdParserSmia::MdParserSmia(const std::vector ®isters)\n> > {\n> > - assert(num_regs > 0);\n> > + for (auto r : registers)\n> > + offsets_[r] = {};\n> > +}\n> > +\n> > +MdParser::Status MdParserSmia::Parse(libcamera::Span\n> buffer,\n> > + RegisterMap &map)\n> > +{\n> > + if (reset_) {\n> > + /*\n> > + * Search again through the metadata for the gain and\n> exposure\n> > + * registers.\n> > + */\n>\n> It's not just gain and exposure, this class is generic and can handle\n> any register.\n>\n\nAck.\n\n\n>\n> I'm curious, is findRegs() that costly that you want to cache the\n> results ? Does SMIA++ guarantee that embedded data will be formatted the\n> same way for every frame ?\n>\n\nThe original SMIA spec guarantees that the register offsets will be the same\nwhile streaming. Not entirely sure about SMIA++, I have not looked at the\nupdated spec yet.\n\nIt is not too expensive to parse the buffer per-frame, but given that the\noffsets are fixed, I think it makes sense to cache the values as an\noptimisation.\n\n\n> I've pushed 2/3 already. 1/3 looks good to me, but I've held off in case\n> some of the review for 3/3 requires changing 1/3.\n>\n\nThanks, I'll submit a new version with the suggestions shortly.\n\nRegards,\nNaush\n\n\n\n> > + ASSERT(bits_per_pixel_);\n> > +\n> > + for (auto &kv : offsets_)\n> > + offsets_[kv.first] = {};\n> > +\n> > + ParseStatus ret = findRegs(buffer);\n> > + /*\n> > + * > 0 means \"worked partially but parse again next time\",\n> > + * < 0 means \"hard error\".\n> > + *\n> > + * In either case, we retry parsing on the next frame.\n> > + */\n> > + if (ret != PARSE_OK)\n> > + return ERROR;\n> > +\n> > + reset_ = false;\n> > + }\n> > +\n> > + /* Populate the register values requested. */\n> > + map.clear();\n> > + for (auto &kv : offsets_) {\n> > + if (!kv.second) {\n> > + reset_ = true;\n> > + return NOTFOUND;\n> > + }\n> > + map[kv.first] = buffer[kv.second.value()];\n> > + }\n> > +\n> > + return OK;\n> > +}\n> > +\n> > +MdParserSmia::ParseStatus MdParserSmia::findRegs(libcamera::Span uint8_t> buffer)\n> > +{\n> > + ASSERT(offsets_.size());\n> >\n> > if (buffer[0] != LINE_START)\n> > return NO_LINE_START;\n> >\n> > unsigned int current_offset = 1; /* after the LINE_START */\n> > unsigned int current_line_start = 0, current_line = 0;\n> > - unsigned int reg_num = 0, first_reg = 0;\n> > + unsigned int reg_num = 0, regs_done = 0;\n> >\n> > while (1) {\n> > int tag = buffer[current_offset++];\n> > @@ -89,13 +134,12 @@ MdParserSmia::ParseStatus\n> MdParserSmia::findRegs(libcamera::Span\n> > else if (tag == REG_SKIP)\n> > reg_num++;\n> > else if (tag == REG_VALUE) {\n> > - while (reg_num >=\n> > - /* assumes registers are in\n> order... */\n> > - regs[first_reg]) {\n> > - if (reg_num == regs[first_reg])\n> > - offsets[first_reg] =\n> current_offset - 1;\n> > + auto reg = offsets_.find(reg_num);\n> > +\n> > + if (reg != offsets_.end()) {\n> > + offsets_[reg_num] = current_offset\n> - 1;\n> >\n> > - if (++first_reg == num_regs)\n> > + if (++regs_done == offsets_.size())\n> > return PARSE_OK;\n> > }\n> > reg_num++;\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\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 D8B9CC321B\n\tfor ;\n\tTue, 22 Jun 2021 12:07:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 32B4F68935;\n\tTue, 22 Jun 2021 14:07:28 +0200 (CEST)","from mail-lj1-x231.google.com (mail-lj1-x231.google.com\n\t[IPv6:2a00:1450:4864:20::231])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5583C60292\n\tfor ;\n\tTue, 22 Jun 2021 14:07:27 +0200 (CEST)","by mail-lj1-x231.google.com with SMTP id d25so1706638lji.7\n\tfor ;\n\tTue, 22 Jun 2021 05:07:27 -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=\"N1U5e3qB\"; 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=dnpCjF48vo2ZISKF2Gb0OpBN/rajgg1TH1Fc+E88Qdk=;\n\tb=N1U5e3qBxffRkNMQ34UBttEy6/czqpznhHPA8Ar71kRKWBt8XVBuJr+k5te+ma+ThQ\n\ta2dAPiBDfhz/iHv/AbNW+A9CcGSOIUcB3qDMkIJkkD6jA1sNUIS9HG9lR1f7oxFWcTLp\n\toMeDsnD36+KgToBUq9JVqD6Rmrt9aL7a4etQfUuYL7N3cca2wb289h7E4OFv9Chz0uXh\n\tZPiDObpP5mqfYsc/HCTbtxRnTOC3/H6zlg6YSAfzxRygjM6xX4vke4/gAQ/VzA6TQDHj\n\ttW9Jay+LiMCs5N/rTucvk2o8t4+Ywsju6Aemdf6iViztx9BAivvGeRVQGltGaze84AcC\n\tClpw==","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=dnpCjF48vo2ZISKF2Gb0OpBN/rajgg1TH1Fc+E88Qdk=;\n\tb=kn4CEHDGMqkrhYpQiDffwXpDCTPhXoM4+hGo8IZ4jUien3Q0iPEyVoPMrgNEj1/eW3\n\t3X2K5SdOICrVREHKS9KHWpuZzAuiKfT5ilYBuFfSGIDuDfoYkLitE2KdeIhLvjO5Qkl9\n\tXhrmnAVY9Za8CeJhFQkEHDFONR6wYJjkWvprtPe5y30EaIMDELOCakdDe8uBmHia0pQS\n\tFmgVFJtP5AZxhf1FsSkXnLwfBVsKfrfA/1fQ0T43y3JkJLER6t979POGskfRNjYow0Pu\n\tXz55O170ytCXrtys8JhvQZZRnyvAZRHn2OvcpLNboS1E5qW85ciWGTM0iVSxFzWrAx9h\n\t3B6Q==","X-Gm-Message-State":"AOAM5334MOCCLMmfQzA0fS7UXRqVKLRyzjQC8hsnkAZELig5BxnQf2qK\n\tC2dv/5SFoo2bNERhNCLXuvq8zTykBaml5KIC4604dw==","X-Google-Smtp-Source":"ABdhPJzX11oAN5Xg7HHNuECDb0GvE0CJW/Njq4eR1fYNjECVyd4Io99D9o1EqpONSt51CohE3JMGqwVKIsrNmyjdQug=","X-Received":"by 2002:a2e:9207:: with SMTP id k7mr2968910ljg.253.1624363646355;\n\tTue, 22 Jun 2021 05:07:26 -0700 (PDT)","MIME-Version":"1.0","References":"<20210615144211.173047-1-naush@raspberrypi.com>\n\t<20210615144211.173047-4-naush@raspberrypi.com>\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Tue, 22 Jun 2021 13:07:10 +0100","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"multipart/alternative; boundary=\"000000000000209ed305c559a2b0\"","Subject":"Re: [libcamera-devel] [PATCH 3/3] ipa: raspberrypi: Generalise the\n\tSMIA metadata parser","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":17697,"web_url":"https://patchwork.libcamera.org/comment/17697/","msgid":"","date":"2021-06-22T12:25:37","subject":"Re: [libcamera-devel] [PATCH 3/3] ipa: raspberrypi: Generalise the\n\tSMIA metadata parser","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nOn Tue, Jun 22, 2021 at 01:07:10PM +0100, Naushir Patuck wrote:\n> On Tue, 22 Jun 2021 at 12:13, Laurent Pinchart wrote:\n> > On Tue, Jun 15, 2021 at 03:42:11PM +0100, Naushir Patuck wrote:\n> > > Instead of having each CamHelper subclass the MdParserSmia, change the\n> > > implementation of MdParserSmia to be more generic. The MdParserSmia now gets\n> > > given a list of registers to search for and helper functions are used to compute\n> > > exposure lines and gain codes from these registers.\n> > >\n> > > Update the imx219 and imx477 CamHelpers by using this new mechanism.\n> > >\n> > > Signed-off-by: Naushir Patuck \n> > > ---\n> > > src/ipa/raspberrypi/cam_helper.cpp | 35 +++---\n> > > src/ipa/raspberrypi/cam_helper.hpp | 2 +\n> > > src/ipa/raspberrypi/cam_helper_imx219.cpp | 118 ++++----------------\n> > > src/ipa/raspberrypi/cam_helper_imx477.cpp | 124 ++++------------------\n> > > src/ipa/raspberrypi/md_parser.hpp | 42 +++++---\n> > > src/ipa/raspberrypi/md_parser_smia.cpp | 66 ++++++++++--\n> > > 6 files changed, 148 insertions(+), 239 deletions(-)\n> > >\n> > > diff --git a/src/ipa/raspberrypi/cam_helper.cpp b/src/ipa/raspberrypi/cam_helper.cpp\n> > > index 1474464c9257..41e292ecbb61 100644\n> > > --- a/src/ipa/raspberrypi/cam_helper.cpp\n> > > +++ b/src/ipa/raspberrypi/cam_helper.cpp\n> > > @@ -159,32 +159,32 @@ unsigned int CamHelper::MistrustFramesModeSwitch() const\n> > > void CamHelper::parseEmbeddedData(Span buffer,\n> > > Metadata &metadata)\n> > > {\n> > > + MdParser::RegisterMap map;\n> > > + Metadata parsedMetadata;\n> > > +\n> > > if (buffer.empty())\n> > > return;\n> > >\n> > > - uint32_t exposureLines, gainCode;\n> > > -\n> > > - if (parser_->Parse(buffer) != MdParser::Status::OK ||\n> > > - parser_->GetExposureLines(exposureLines) != MdParser::Status::OK ||\n> > > - parser_->GetGainCode(gainCode) != MdParser::Status::OK) {\n> > > + if (parser_->Parse(buffer, map) != MdParser::Status::OK) {\n> > > LOG(IPARPI, Error) << \"Embedded data buffer parsing failed\";\n> > > return;\n> > > }\n> > >\n> > > + PopulateMetadata(map, parsedMetadata);\n> > > + metadata.Merge(parsedMetadata);\n> > > +\n> > > /*\n> > > - * Overwrite the exposure/gain values in the DeviceStatus, as\n> > > - * we know better. Fetch it first in case any other fields were\n> > > - * set meaningfully.\n> > > + * Overwrite the exposure/gain values in the existing DeviceStatus with\n> > > + * values from the parsed embedded buffer. Fetch it first in case any\n> > > + * other fields were set meaningfully.\n> > > */\n> > > - DeviceStatus deviceStatus;\n> > > -\n> > > - if (metadata.Get(\"device.status\", deviceStatus) != 0) {\n> > > - LOG(IPARPI, Error) << \"DeviceStatus not found\";\n> >\n> > No log anymore ?\n> \n> Ack, I'll add it back below.\n> \n> > > + DeviceStatus deviceStatus, parsedDeviceStatus;\n> > > + if (metadata.Get(\"device.status\", deviceStatus) ||\n> > > + parsedMetadata.Get(\"device.status\", parsedDeviceStatus))\n> > > return;\n> > > - }\n> > >\n> > > - deviceStatus.shutter_speed = Exposure(exposureLines);\n> > > - deviceStatus.analogue_gain = Gain(gainCode);\n> > > + deviceStatus.shutter_speed = parsedDeviceStatus.shutter_speed;\n> > > + deviceStatus.analogue_gain = parsedDeviceStatus.analogue_gain;\n> > >\n> > > LOG(IPARPI, Debug) << \"Metadata updated - Exposure : \"\n> > > << deviceStatus.shutter_speed\n> > > @@ -194,6 +194,11 @@ void CamHelper::parseEmbeddedData(Span buffer,\n> > > metadata.Set(\"device.status\", deviceStatus);\n> > > }\n> > >\n> > > +void CamHelper::PopulateMetadata([[maybe_unused]] const MdParser::RegisterMap &map,\n> > > + [[maybe_unused]] Metadata &metadata) const\n> > > +{\n> > > +}\n> > > +\n> > > RegisterCamHelper::RegisterCamHelper(char const *cam_name,\n> > > CamHelperCreateFunc create_func)\n> > > {\n> > > diff --git a/src/ipa/raspberrypi/cam_helper.hpp b/src/ipa/raspberrypi/cam_helper.hpp\n> > > index f53f5c39b01c..30879e60b861 100644\n> > > --- a/src/ipa/raspberrypi/cam_helper.hpp\n> > > +++ b/src/ipa/raspberrypi/cam_helper.hpp\n> > > @@ -91,6 +91,8 @@ public:\n> > > protected:\n> > > void parseEmbeddedData(libcamera::Span buffer,\n> > > Metadata &metadata);\n> > > + virtual void PopulateMetadata([[maybe_unused]] const MdParser::RegisterMap &map,\n> > > + [[maybe_unused]] Metadata &metadata) const;\n> > >\n> > > MdParser *parser_;\n> > > CameraMode mode_;\n> > > diff --git a/src/ipa/raspberrypi/cam_helper_imx219.cpp b/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> > > index d951cd552a21..24818e9ec4de 100644\n> > > --- a/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> > > +++ b/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> > > @@ -23,21 +23,13 @@\n> > >\n> > > using namespace RPiController;\n> > >\n> > > -/* Metadata parser implementation specific to Sony IMX219 sensors. */\n> > > -\n> > > -class MdParserImx219 : public MdParserSmia\n> > > -{\n> > > -public:\n> > > - MdParserImx219();\n> > > - Status Parse(libcamera::Span buffer) override;\n> > > - Status GetExposureLines(unsigned int &lines) override;\n> > > - Status GetGainCode(unsigned int &gain_code) override;\n> > > -private:\n> > > - /* Offset of the register's value in the metadata block. */\n> > > - int reg_offsets_[3];\n> > > - /* Value of the register, once read from the metadata block. */\n> > > - int reg_values_[3];\n> > > -};\n> > > +/*\n> > > + * We care about one gain register and a pair of exposure registers. Their I2C\n> > > + * addresses from the Sony IMX219 datasheet:\n> > > + */\n> > > +constexpr uint32_t gainReg = 0x157;\n> > > +constexpr uint32_t expHiReg = 0x15a;\n> > > +constexpr uint32_t expLoReg = 0x15b;\n> > >\n> > > class CamHelperImx219 : public CamHelper\n> > > {\n> > > @@ -55,15 +47,19 @@ private:\n> > > */\n> > > static constexpr int frameIntegrationDiff = 4;\n> > >\n> > > - MdParserImx219 imx219_parser;\n> > > + MdParserSmia imx219_parser;\n> >\n> > Could you name this just \"parser\" ? It's a member of the CamHelperImx219\n> > class, there's no need to repeat the prefix. Same for imx477.\n> \n> Ack.\n> \n> > > +\n> > > + void PopulateMetadata(const MdParser::RegisterMap &map,\n> > > + Metadata &metadata) const override;\n> > > };\n> > >\n> > > CamHelperImx219::CamHelperImx219()\n> > > #if ENABLE_EMBEDDED_DATA\n> > > - : CamHelper(&imx219_parser, frameIntegrationDiff)\n> > > + : CamHelper(&imx219_parser, frameIntegrationDiff),\n> >\n> > You're passing a pointer to an object that isn't constructed yet. The\n> > base class only stores it so it should be OK, but it's a bit fragile.\n> \n> I agree, this is slightly naughty. Without relying in a dynamic\n> allocation, there is no other way to construct the object before\n> passing on the pointer to the base class.\n> \n> Perhaps I should replace patch 1/3 to use a shared_ptr instead of\n\nA unique_ptr may be better.\n\n> embedding the parser object in the derived class. This way, the memory\n> is correctly managed, and the object will be constructed before returning\n> it to the base class.\n\nI'm fine either way.\n\n> > > #else\n> > > - : CamHelper(nullptr, frameIntegrationDiff)\n> > > + : CamHelper(nullptr, frameIntegrationDiff),\n> > > #endif\n> > > + imx219_parser({ gainReg, expHiReg, expLoReg })\n> > > {\n> > > }\n> > >\n> > > @@ -92,88 +88,20 @@ bool CamHelperImx219::SensorEmbeddedDataPresent() const\n> > > return ENABLE_EMBEDDED_DATA;\n> > > }\n> > >\n> > > -static CamHelper *Create()\n> > > +void CamHelperImx219::PopulateMetadata(const MdParser::RegisterMap &map,\n> > > + Metadata &metadata) const\n> > > {\n> > > - return new CamHelperImx219();\n> > > -}\n> > > + DeviceStatus deviceStatus;\n> > >\n> > > -static RegisterCamHelper reg(\"imx219\", &Create);\n> > > + deviceStatus.shutter_speed = Exposure(map.at(expHiReg) * 256 + map.at(expLoReg));\n> > > + deviceStatus.analogue_gain = Gain(map.at(gainReg));\n> > >\n> > > -/*\n> > > - * We care about one gain register and a pair of exposure registers. Their I2C\n> > > - * addresses from the Sony IMX219 datasheet:\n> > > - */\n> > > -#define GAIN_REG 0x157\n> > > -#define EXPHI_REG 0x15A\n> > > -#define EXPLO_REG 0x15B\n> > > -\n> > > -/*\n> > > - * Index of each into the reg_offsets and reg_values arrays. Must be in\n> > > - * register address order.\n> > > - */\n> > > -#define GAIN_INDEX 0\n> > > -#define EXPHI_INDEX 1\n> > > -#define EXPLO_INDEX 2\n> > > -\n> > > -MdParserImx219::MdParserImx219()\n> > > -{\n> > > - reg_offsets_[0] = reg_offsets_[1] = reg_offsets_[2] = -1;\n> > > + metadata.Set(\"device.status\", deviceStatus);\n> > > }\n> > >\n> > > -MdParser::Status MdParserImx219::Parse(libcamera::Span buffer)\n> > > -{\n> > > - bool try_again = false;\n> > > -\n> > > - if (reset_) {\n> > > - /*\n> > > - * Search again through the metadata for the gain and exposure\n> > > - * registers.\n> > > - */\n> > > - assert(bits_per_pixel_);\n> > > - /* Need to be ordered */\n> > > - uint32_t regs[3] = { GAIN_REG, EXPHI_REG, EXPLO_REG };\n> > > - reg_offsets_[0] = reg_offsets_[1] = reg_offsets_[2] = -1;\n> > > - int ret = static_cast(findRegs(buffer,\n> > > - regs, reg_offsets_, 3));\n> > > - /*\n> > > - * > 0 means \"worked partially but parse again next time\",\n> > > - * < 0 means \"hard error\".\n> > > - */\n> > > - if (ret > 0)\n> > > - try_again = true;\n> > > - else if (ret < 0)\n> > > - return ERROR;\n> > > - }\n> > > -\n> > > - for (int i = 0; i < 3; i++) {\n> > > - if (reg_offsets_[i] == -1)\n> > > - continue;\n> > > -\n> > > - reg_values_[i] = buffer[reg_offsets_[i]];\n> > > - }\n> > > -\n> > > - /* Re-parse next time if we were unhappy in some way. */\n> > > - reset_ = try_again;\n> > > -\n> > > - return OK;\n> > > -}\n> > > -\n> > > -MdParser::Status MdParserImx219::GetExposureLines(unsigned int &lines)\n> > > +static CamHelper *Create()\n> > > {\n> > > - if (reg_offsets_[EXPHI_INDEX] == -1 || reg_offsets_[EXPLO_INDEX] == -1)\n> > > - return NOTFOUND;\n> > > -\n> > > - lines = reg_values_[EXPHI_INDEX] * 256 + reg_values_[EXPLO_INDEX];\n> > > -\n> > > - return OK;\n> > > + return new CamHelperImx219();\n> > > }\n> > >\n> > > -MdParser::Status MdParserImx219::GetGainCode(unsigned int &gain_code)\n> > > -{\n> > > - if (reg_offsets_[GAIN_INDEX] == -1)\n> > > - return NOTFOUND;\n> > > -\n> > > - gain_code = reg_values_[GAIN_INDEX];\n> > > -\n> > > - return OK;\n> > > -}\n> > > +static RegisterCamHelper reg(\"imx219\", &Create);\n> > > diff --git a/src/ipa/raspberrypi/cam_helper_imx477.cpp b/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> > > index 44f030ed7da9..7586e5f897d5 100644\n> > > --- a/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> > > +++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> > > @@ -15,21 +15,14 @@\n> > >\n> > > using namespace RPiController;\n> > >\n> > > -/* Metadata parser implementation specific to Sony IMX477 sensors. */\n> > > -\n> > > -class MdParserImx477 : public MdParserSmia\n> > > -{\n> > > -public:\n> > > - MdParserImx477();\n> > > - Status Parse(libcamera::Span buffer) override;\n> > > - Status GetExposureLines(unsigned int &lines) override;\n> > > - Status GetGainCode(unsigned int &gain_code) override;\n> > > -private:\n> > > - /* Offset of the register's value in the metadata block. */\n> > > - int reg_offsets_[4];\n> > > - /* Value of the register, once read from the metadata block. */\n> > > - int reg_values_[4];\n> > > -};\n> > > +/*\n> > > + * We care about two gain registers and a pair of exposure registers. Their\n> > > + * I2C addresses from the Sony IMX477 datasheet:\n> > > + */\n> > > +constexpr uint32_t expHiReg = 0x0202;\n> > > +constexpr uint32_t expLoReg = 0x0203;\n> > > +constexpr uint32_t gainHiReg = 0x0204;\n> > > +constexpr uint32_t gainLoReg = 0x0205;\n> > >\n> > > class CamHelperImx477 : public CamHelper\n> > > {\n> > > @@ -48,11 +41,15 @@ private:\n> > > */\n> > > static constexpr int frameIntegrationDiff = 22;\n> > >\n> > > - MdParserImx477 imx477_parser;\n> > > + MdParserSmia imx477_parser;\n> > > +\n> > > + void PopulateMetadata(const MdParser::RegisterMap &map,\n> > > + Metadata &metadata) const override;\n> > > };\n> > >\n> > > CamHelperImx477::CamHelperImx477()\n> > > - : CamHelper(&imx477_parser, frameIntegrationDiff)\n> > > + : CamHelper(&imx477_parser, frameIntegrationDiff),\n> > > + imx477_parser({ expHiReg, expLoReg, gainHiReg, gainLoReg })\n> > > {\n> > > }\n> > >\n> > > @@ -79,95 +76,20 @@ bool CamHelperImx477::SensorEmbeddedDataPresent() const\n> > > return true;\n> > > }\n> > >\n> > > -static CamHelper *Create()\n> > > +void CamHelperImx477::PopulateMetadata(const MdParser::RegisterMap &map,\n> > > + Metadata &metadata) const\n> > > {\n> > > - return new CamHelperImx477();\n> > > -}\n> > > + DeviceStatus deviceStatus;\n> > >\n> > > -static RegisterCamHelper reg(\"imx477\", &Create);\n> > > + deviceStatus.shutter_speed = Exposure(map.at(expHiReg) * 256 + map.at(expLoReg));\n> > > + deviceStatus.analogue_gain = Gain(map.at(gainHiReg) * 256 + map.at (gainLoReg));\n> > >\n> > > -/*\n> > > - * We care about two gain registers and a pair of exposure registers. Their\n> > > - * I2C addresses from the Sony IMX477 datasheet:\n> > > - */\n> > > -#define EXPHI_REG 0x0202\n> > > -#define EXPLO_REG 0x0203\n> > > -#define GAINHI_REG 0x0204\n> > > -#define GAINLO_REG 0x0205\n> > > -\n> > > -/*\n> > > - * Index of each into the reg_offsets and reg_values arrays. Must be in register\n> > > - * address order.\n> > > - */\n> > > -#define EXPHI_INDEX 0\n> > > -#define EXPLO_INDEX 1\n> > > -#define GAINHI_INDEX 2\n> > > -#define GAINLO_INDEX 3\n> > > -\n> > > -MdParserImx477::MdParserImx477()\n> > > -{\n> > > - reg_offsets_[0] = reg_offsets_[1] = reg_offsets_[2] = reg_offsets_[3] = -1;\n> > > + metadata.Set(\"device.status\", deviceStatus);\n> > > }\n> > >\n> > > -MdParser::Status MdParserImx477::Parse(libcamera::Span buffer)\n> > > -{\n> > > - bool try_again = false;\n> > > -\n> > > - if (reset_) {\n> > > - /*\n> > > - * Search again through the metadata for the gain and exposure\n> > > - * registers.\n> > > - */\n> > > - assert(bits_per_pixel_);\n> > > - /* Need to be ordered */\n> > > - uint32_t regs[4] = {\n> > > - EXPHI_REG,\n> > > - EXPLO_REG,\n> > > - GAINHI_REG,\n> > > - GAINLO_REG\n> > > - };\n> > > - reg_offsets_[0] = reg_offsets_[1] = reg_offsets_[2] = reg_offsets_[3] = -1;\n> > > - int ret = static_cast(findRegs(buffer,\n> > > - regs, reg_offsets_, 4));\n> > > - /*\n> > > - * > 0 means \"worked partially but parse again next time\",\n> > > - * < 0 means \"hard error\".\n> > > - */\n> > > - if (ret > 0)\n> > > - try_again = true;\n> > > - else if (ret < 0)\n> > > - return ERROR;\n> > > - }\n> > > -\n> > > - for (int i = 0; i < 4; i++) {\n> > > - if (reg_offsets_[i] == -1)\n> > > - continue;\n> > > -\n> > > - reg_values_[i] = buffer[reg_offsets_[i]];\n> > > - }\n> > > -\n> > > - /* Re-parse next time if we were unhappy in some way. */\n> > > - reset_ = try_again;\n> > > -\n> > > - return OK;\n> > > -}\n> > > -\n> > > -MdParser::Status MdParserImx477::GetExposureLines(unsigned int &lines)\n> > > +static CamHelper *Create()\n> > > {\n> > > - if (reg_offsets_[EXPHI_INDEX] == -1 || reg_offsets_[EXPLO_INDEX] == -1)\n> > > - return NOTFOUND;\n> > > -\n> > > - lines = reg_values_[EXPHI_INDEX] * 256 + reg_values_[EXPLO_INDEX];\n> > > -\n> > > - return OK;\n> > > + return new CamHelperImx477();\n> > > }\n> > >\n> > > -MdParser::Status MdParserImx477::GetGainCode(unsigned int &gain_code)\n> > > -{\n> > > - if (reg_offsets_[GAINHI_INDEX] == -1 || reg_offsets_[GAINLO_INDEX] == -1)\n> > > - return NOTFOUND;\n> > > -\n> > > - gain_code = reg_values_[GAINHI_INDEX] * 256 + reg_values_[GAINLO_INDEX];\n> > > -\n> > > - return OK;\n> > > -}\n> > > +static RegisterCamHelper reg(\"imx477\", &Create);\n> > > diff --git a/src/ipa/raspberrypi/md_parser.hpp b/src/ipa/raspberrypi/md_parser.hpp\n> > > index 65aab02d51b6..64f9bbb12845 100644\n> > > --- a/src/ipa/raspberrypi/md_parser.hpp\n> > > +++ b/src/ipa/raspberrypi/md_parser.hpp\n> > > @@ -8,6 +8,10 @@\n> > >\n> > > #include \n> > >\n> > > +#include \n> > > +#include \n> > > +#include \n> > > +\n> > > #include \n> > >\n> > > /*\n> > > @@ -19,7 +23,7 @@\n> > > * application code doesn't have to worry which 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> > > + * MdParser *parser = new MdParserSmia({ expHiReg, expLoReg, gainReg });\n> > > * parser->SetBitsPerPixel(bpp);\n> > > * parser->SetLineLengthBytes(pitch);\n> > > * parser->SetNumLines(2);\n> > > @@ -32,13 +36,11 @@\n> > > *\n> > > * Then on every frame:\n> > > *\n> > > - * if (parser->Parse(buffer) != MdParser::OK)\n> > > + * RegisterMap map;\n> > > + * if (parser->Parse(buffer, map) != 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> > > + * Metadata metadata;\n> > > + * CamHelper::PopulateMetadata(map, metadata);\n> > > *\n> > > * (Note that the CamHelper class converts to/from exposure lines and time,\n> > > * and gain_code / actual gain.)\n> > > @@ -59,6 +61,8 @@ namespace RPiController {\n> > > class MdParser\n> > > {\n> > > public:\n> > > + using RegisterMap = std::map;\n> > > +\n> > > /*\n> > > * Parser status codes:\n> > > * OK - success\n> > > @@ -98,9 +102,8 @@ public:\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> > > + virtual Status Parse(libcamera::Span buffer,\n> > > + RegisterMap &map) = 0;\n> >\n> > I'd name the variable registers to tell what it contains (same in the\n> > caller and in the example above).\n> \n> Ack.\n> \n> > > protected:\n> > > bool reset_;\n> > > @@ -116,14 +119,18 @@ protected:\n> > > * md_parser_imx219.cpp for an example).\n> > > */\n> > >\n> > > -class MdParserSmia : public MdParser\n> > > +class MdParserSmia final : public MdParser\n> > > {\n> > > public:\n> > > - MdParserSmia() : MdParser()\n> > > - {\n> > > - }\n> > > + MdParserSmia(const std::vector ®s);\n> > > +\n> > > + MdParser::Status Parse(libcamera::Span buffer,\n> > > + RegisterMap &map) override;\n> > > +\n> > > +private:\n> > > + /* Maps register address to offset in the buffer. */\n> > > + using OffsetMap = std::map>;\n> > >\n> > > -protected:\n> > > /*\n> > > * Note that error codes > 0 are regarded as non-fatal; codes < 0\n> > > * indicate a bad data buffer. Status codes are:\n> > > @@ -141,8 +148,9 @@ protected:\n> > > BAD_PADDING = -5\n> > > };\n> > >\n> > > - ParseStatus findRegs(libcamera::Span buffer, uint32_t regs[],\n> > > - int offsets[], unsigned int num_regs);\n> > > + ParseStatus findRegs(libcamera::Span buffer);\n> > > +\n> > > + OffsetMap offsets_;\n> > > };\n> > >\n> > > } // namespace RPi\n> > > diff --git a/src/ipa/raspberrypi/md_parser_smia.cpp b/src/ipa/raspberrypi/md_parser_smia.cpp\n> > > index 0a14875575a2..9920699dfe25 100644\n> > > --- a/src/ipa/raspberrypi/md_parser_smia.cpp\n> > > +++ b/src/ipa/raspberrypi/md_parser_smia.cpp\n> > > @@ -6,9 +6,11 @@\n> > > */\n> > > #include \n> > >\n> > > +#include \"libcamera/internal/log.h\"\n> >\n> > Do you need this header ?\n> \n> Yes, I do for the ASSERT() macro below. I could use the standard assert() without\n> this header, but thought the libcamera version would be more consistent.\n\nAs yes I've missed that.\n\n> > > #include \"md_parser.hpp\"\n> > >\n> > > using namespace RPiController;\n> > > +using namespace libcamera;\n> > >\n> > > /*\n> > > * This function goes through the embedded data to find the offsets (not\n> > > @@ -26,18 +28,61 @@ constexpr unsigned int REG_LOW_BITS = 0xa5;\n> > > constexpr unsigned int REG_VALUE = 0x5a;\n> > > constexpr unsigned int REG_SKIP = 0x55;\n> > >\n> > > -MdParserSmia::ParseStatus MdParserSmia::findRegs(libcamera::Span buffer,\n> > > - uint32_t regs[], int offsets[],\n> > > - unsigned int num_regs)\n> > > +MdParserSmia::MdParserSmia(const std::vector ®isters)\n> > > {\n> > > - assert(num_regs > 0);\n> > > + for (auto r : registers)\n> > > + offsets_[r] = {};\n> > > +}\n> > > +\n> > > +MdParser::Status MdParserSmia::Parse(libcamera::Span buffer,\n> > > + RegisterMap &map)\n> > > +{\n> > > + if (reset_) {\n> > > + /*\n> > > + * Search again through the metadata for the gain and exposure\n> > > + * registers.\n> > > + */\n> >\n> > It's not just gain and exposure, this class is generic and can handle\n> > any register.\n> \n> Ack.\n> \n> > I'm curious, is findRegs() that costly that you want to cache the\n> > results ? Does SMIA++ guarantee that embedded data will be formatted the\n> > same way for every frame ?\n> \n> The original SMIA spec guarantees that the register offsets will be the same\n> while streaming. Not entirely sure about SMIA++, I have not looked at the\n> updated spec yet.\n\nCCS states\n\n\"The embedded data sequence of tags and the number of embedded data\nlines shall not vary dynamically from frame to frame. This is to speed\nup software-based systems, by allowing the Host to build up a positional\nmap (pixel number N = the value for CCI Register M) from the embedded\ndata lines in the first frame of image data received by the Host\nsystem.\"\n\nSounds good.\n\n> It is not too expensive to parse the buffer per-frame, but given that the\n> offsets are fixed, I think it makes sense to cache the values as an\n> optimisation.\n> \n> > I've pushed 2/3 already. 1/3 looks good to me, but I've held off in case\n> > some of the review for 3/3 requires changing 1/3.\n> \n> Thanks, I'll submit a new version with the suggestions shortly.\n> \n> > > + ASSERT(bits_per_pixel_);\n> > > +\n> > > + for (auto &kv : offsets_)\n> > > + offsets_[kv.first] = {};\n> > > +\n> > > + ParseStatus ret = findRegs(buffer);\n> > > + /*\n> > > + * > 0 means \"worked partially but parse again next time\",\n> > > + * < 0 means \"hard error\".\n> > > + *\n> > > + * In either case, we retry parsing on the next frame.\n> > > + */\n> > > + if (ret != PARSE_OK)\n> > > + return ERROR;\n> > > +\n> > > + reset_ = false;\n> > > + }\n> > > +\n> > > + /* Populate the register values requested. */\n> > > + map.clear();\n> > > + for (auto &kv : offsets_) {\n> > > + if (!kv.second) {\n> > > + reset_ = true;\n> > > + return NOTFOUND;\n> > > + }\n> > > + map[kv.first] = buffer[kv.second.value()];\n> > > + }\n> > > +\n> > > + return OK;\n> > > +}\n> > > +\n> > > +MdParserSmia::ParseStatus MdParserSmia::findRegs(libcamera::Span buffer)\n> > > +{\n> > > + ASSERT(offsets_.size());\n> > >\n> > > if (buffer[0] != LINE_START)\n> > > return NO_LINE_START;\n> > >\n> > > unsigned int current_offset = 1; /* after the LINE_START */\n> > > unsigned int current_line_start = 0, current_line = 0;\n> > > - unsigned int reg_num = 0, first_reg = 0;\n> > > + unsigned int reg_num = 0, regs_done = 0;\n> > >\n> > > while (1) {\n> > > int tag = buffer[current_offset++];\n> > > @@ -89,13 +134,12 @@ MdParserSmia::ParseStatus MdParserSmia::findRegs(libcamera::Span\n> > > else if (tag == REG_SKIP)\n> > > reg_num++;\n> > > else if (tag == REG_VALUE) {\n> > > - while (reg_num >=\n> > > - /* assumes registers are in order... */\n> > > - regs[first_reg]) {\n> > > - if (reg_num == regs[first_reg])\n> > > - offsets[first_reg] = current_offset - 1;\n> > > + auto reg = offsets_.find(reg_num);\n> > > +\n> > > + if (reg != offsets_.end()) {\n> > > + offsets_[reg_num] = current_offset - 1;\n> > >\n> > > - if (++first_reg == num_regs)\n> > > + if (++regs_done == offsets_.size())\n> > > return PARSE_OK;\n> > > }\n> > > reg_num++;","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 1670AC321A\n\tfor ;\n\tTue, 22 Jun 2021 12:26:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3475968932;\n\tTue, 22 Jun 2021 14:26:07 +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 BEAB660292\n\tfor ;\n\tTue, 22 Jun 2021 14:26:05 +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 346BFAD6;\n\tTue, 22 Jun 2021 14:26:05 +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=\"Gy+0DlzW\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1624364765;\n\tbh=DYXHVJtZAGV03nONGarYL6f7Ybj4r2O6yJoEnue3q4I=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Gy+0DlzWQSKHSq5PJtOx65ZVC8BcRIXiCovV1ODlvZ28VEwT5O95mAFqeqDSIE0jo\n\twSKjBqPxhRzQ7QCvxwIPoHUyrj5M86vLbIkSZAQTba8TYhdUhL3FHh5ogqIL+AbEC6\n\twIMpjfTse33ODjoiZlhPkQ1wpoEDOiE2+hqJ1DRU=","Date":"Tue, 22 Jun 2021 15:25:37 +0300","From":"Laurent Pinchart ","To":"Naushir Patuck ","Message-ID":"","References":"<20210615144211.173047-1-naush@raspberrypi.com>\n\t<20210615144211.173047-4-naush@raspberrypi.com>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 3/3] ipa: raspberrypi: Generalise the\n\tSMIA metadata parser","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\" "}}]