[{"id":17903,"web_url":"https://patchwork.libcamera.org/comment/17903/","msgid":"<YNoQLmLDgYc9b6q4@pendragon.ideasonboard.com>","date":"2021-06-28T18:08:46","subject":"Re: [libcamera-devel] [PATCH v2 2/2] ipa: raspberrypi: Generalise\n\tthe SMIA 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 22, 2021 at 02:20:14PM +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 <naush@raspberrypi.com>\n> ---\n>  src/ipa/raspberrypi/cam_helper.cpp        |  33 +++---\n>  src/ipa/raspberrypi/cam_helper.hpp        |   2 +\n>  src/ipa/raspberrypi/cam_helper_imx219.cpp | 115 ++++----------------\n>  src/ipa/raspberrypi/cam_helper_imx477.cpp | 123 ++++------------------\n>  src/ipa/raspberrypi/md_parser.hpp         |  42 +++++---\n>  src/ipa/raspberrypi/md_parser_smia.cpp    |  66 ++++++++++--\n>  6 files changed, 147 insertions(+), 234 deletions(-)\n> \n> diff --git a/src/ipa/raspberrypi/cam_helper.cpp b/src/ipa/raspberrypi/cam_helper.cpp\n> index 90498c37af98..7cfced2d6c1f 100644\n> --- a/src/ipa/raspberrypi/cam_helper.cpp\n> +++ b/src/ipa/raspberrypi/cam_helper.cpp\n> @@ -159,32 +159,34 @@ unsigned int CamHelper::MistrustFramesModeSwitch() const\n>  void CamHelper::parseEmbeddedData(Span<const uint8_t> buffer,\n>  \t\t\t\t  Metadata &metadata)\n>  {\n> +\tMdParser::RegisterMap registers;\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, registers) != MdParser::Status::OK) {\n>  \t\tLOG(IPARPI, Error) << \"Embedded data buffer parsing failed\";\n>  \t\treturn;\n>  \t}\n>  \n> +\tPopulateMetadata(registers, 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> +\tDeviceStatus deviceStatus, parsedDeviceStatus;\n> +\tif (metadata.Get(\"device.status\", deviceStatus) ||\n> +\t    parsedMetadata.Get(\"device.status\", parsedDeviceStatus)) {\n>  \t\tLOG(IPARPI, Error) << \"DeviceStatus not found\";\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 +196,11 @@ void CamHelper::parseEmbeddedData(Span<const uint8_t> 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 7371b9d97f66..81ac9d39dc46 100644\n> --- a/src/ipa/raspberrypi/cam_helper.hpp\n> +++ b/src/ipa/raspberrypi/cam_helper.hpp\n> @@ -92,6 +92,8 @@ public:\n>  protected:\n>  \tvoid parseEmbeddedData(libcamera::Span<const uint8_t> 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\nYou don't need [[maybe_unused]] in the function declaration.\n\n>  \n>  \tstd::unique_ptr<MdParser> 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 c85044a5fa6d..18f5c3e7e520 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<const uint8_t> 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> @@ -54,11 +46,16 @@ private:\n>  \t * in units of lines.\n>  \t */\n>  \tstatic constexpr int frameIntegrationDiff = 4;\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(std::make_unique<MdParserImx219>(), frameIntegrationDiff)\n> +\t: CamHelper(std::make_unique<MdParserSmia>\n> +\t\t\t(std::initializer_list<uint32_t>({ expHiReg, expLoReg, gainReg })),\n> +\t\t    frameIntegrationDiff)\n>  #else\n>  \t: CamHelper({}, frameIntegrationDiff)\n>  #endif\n> @@ -90,88 +87,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<const uint8_t> 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<int>(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 d72a9be0438e..8869af6620cf 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<const uint8_t> 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> @@ -47,10 +40,15 @@ private:\n>  \t * in units of lines.\n>  \t */\n>  \tstatic constexpr int frameIntegrationDiff = 22;\n> +\n> +\tvoid PopulateMetadata(const MdParser::RegisterMap &map,\n> +\t\t\t      Metadata &metadata) const override;\n>  };\n>  \n>  CamHelperImx477::CamHelperImx477()\n> -\t: CamHelper(std::make_unique<MdParserImx477>(), frameIntegrationDiff)\n> +\t: CamHelper(std::make_unique<MdParserSmia>\n> +\t\t\t(std::initializer_list<uint32_t>({ expHiReg, expLoReg, gainHiReg, gainLoReg })),\n> +\t\t    frameIntegrationDiff)\n>  {\n>  }\n>  \n> @@ -77,95 +75,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<const uint8_t> 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<int>(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..05c1a060e26e 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 <stdint.h>\n>  \n> +#include <initializer_list>\n> +#include <map>\n> +#include <optional>\n\nYou can group this with stdint.h.\n\n> +\n>  #include <libcamera/span.h>\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 registers;\n> + * if (parser->Parse(buffer, registers) != 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(registers, 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<uint32_t, uint32_t>;\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<const uint8_t> 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<const uint8_t> buffer,\n> +\t\t\t     RegisterMap &map) = 0;\n\ns/map/registers/ here too ?\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(std::initializer_list<uint32_t> registerList);\n> +\n> +\tMdParser::Status Parse(libcamera::Span<const uint8_t> buffer,\n> +\t\t\t       RegisterMap &registers) override;\n> +\n> +private:\n> +\t/* Maps register address to offset in the buffer. */\n> +\tusing OffsetMap = std::map<uint32_t, std::optional<uint32_t>>;\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<const uint8_t> buffer, uint32_t regs[],\n> -\t\t\t     int offsets[], unsigned int num_regs);\n> +\tParseStatus findRegs(libcamera::Span<const uint8_t> 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..d6900328a992 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 <assert.h>\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<const uint8_t> 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(std::initializer_list<uint32_t> registerList)\n>  {\n> -\tassert(num_regs > 0);\n> +\tfor (auto r : registerList)\n> +\t\toffsets_[r] = {};\n> +}\n> +\n> +MdParser::Status MdParserSmia::Parse(libcamera::Span<const uint8_t> buffer,\n> +\t\t\t\t     RegisterMap &registers)\n> +{\n> +\tif (reset_) {\n> +\t\t/*\n> +\t\t * Search again through the metadata for all the registers\n> +\t\t * requested.\n> +\t\t */\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> +\tregisters.clear();\n> +\tfor (auto &kv : offsets_) {\n\nYou could write this\n\n\tfor (const auto &[register, offset] : offsets_) {\n\nand use register and offset instead of kv.first and kv.second below.\n\nReviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n\n> +\t\tif (!kv.second) {\n> +\t\t\treset_ = true;\n> +\t\t\treturn NOTFOUND;\n> +\t\t}\n> +\t\tregisters[kv.first] = buffer[kv.second.value()];\n> +\t}\n> +\n> +\treturn OK;\n> +}\n> +\n> +MdParserSmia::ParseStatus MdParserSmia::findRegs(libcamera::Span<const uint8_t> 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<const uint8_t>\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":"<libcamera-devel-bounces@lists.libcamera.org>","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 869F5C321F\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 28 Jun 2021 18:08:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C7FDA684D4;\n\tMon, 28 Jun 2021 20:08:49 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 507D96028C\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 28 Jun 2021 20:08:48 +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 C7FFEB8A;\n\tMon, 28 Jun 2021 20:08:47 +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=\"jOiQsaPe\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1624903728;\n\tbh=Peu73u/E4vnd99Kk+dd+fq+7POa3+210wPNZ6KkWEIo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=jOiQsaPet4Dq9tswdFu37xa6+cYQhZ73lFSRZ2U1T+c7PDzj5dFbxOEanc8ePvyVh\n\tTmGMAoW3JPvNtEZXZ7CIQa5r875IU2ipbOtZTtkajOpGZ86Nv1vytcHFyggWlaRBb5\n\tqKzOwYgf3CRWflkd7JboQQqhUVmDyoETfLB4bT6o=","Date":"Mon, 28 Jun 2021 21:08:46 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Naushir Patuck <naush@raspberrypi.com>","Message-ID":"<YNoQLmLDgYc9b6q4@pendragon.ideasonboard.com>","References":"<20210622132014.949961-1-naush@raspberrypi.com>\n\t<20210622132014.949961-3-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210622132014.949961-3-naush@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v2 2/2] ipa: raspberrypi: Generalise\n\tthe SMIA metadata parser","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]