[{"id":17935,"web_url":"https://patchwork.libcamera.org/comment/17935/","msgid":"<CAEmqJPq68nFaRH5FXgcyv_NR4X_=2bSZM5X1N2VKOWB+Y26s_g@mail.gmail.com>","date":"2021-06-30T08:35:54","subject":"Re: [libcamera-devel] [PATCH v3.1 2/2] ipa: raspberrypi: Generalise\n\tthe SMIA 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 these fixups!\nApart from one minor thing below, all looks good.\n\nOn Tue, 29 Jun 2021 at 16:48, Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> From: Naushir Patuck <naush@raspberrypi.com>\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 <naush@raspberrypi.com>\n> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.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 | 118 ++++----------------\n>  src/ipa/raspberrypi/cam_helper_imx477.cpp | 126 +++++-----------------\n>  src/ipa/raspberrypi/md_parser.hpp         |  41 ++++---\n>  src/ipa/raspberrypi/md_parser_smia.cpp    |  67 +++++++++---\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 90498c37af98..e6d2258c66d7 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()\n> const\n>  void CamHelper::parseEmbeddedData(Span<const uint8_t> buffer,\n>                                   Metadata &metadata)\n>  {\n> +       MdParser::RegisterMap registers;\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, registers) != MdParser::Status::OK) {\n>                 LOG(IPARPI, Error) << \"Embedded data buffer parsing\n> failed\";\n>                 return;\n>         }\n>\n> +       PopulateMetadata(registers, 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> +       DeviceStatus deviceStatus, parsedDeviceStatus;\n> +       if (metadata.Get(\"device.status\", deviceStatus) ||\n> +           parsedMetadata.Get(\"device.status\", parsedDeviceStatus)) {\n>                 LOG(IPARPI, Error) << \"DeviceStatus not found\";\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 +196,11 @@ void CamHelper::parseEmbeddedData(Span<const uint8_t>\n> buffer,\n>         metadata.Set(\"device.status\", deviceStatus);\n>  }\n>\n> +void CamHelper::PopulateMetadata([[maybe_unused]] const\n> MdParser::RegisterMap &registers,\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 fc3139e22be0..200cc83f3872 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>         void parseEmbeddedData(libcamera::Span<const uint8_t> buffer,\n>                                Metadata &metadata);\n> +       virtual void PopulateMetadata(const MdParser::RegisterMap\n> &registers,\n> +                                     Metadata &metadata) const;\n>\n>         std::unique_ptr<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 c85044a5fa6d..4d68e01fce71 100644\n> --- a/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> +++ b/src/ipa/raspberrypi/cam_helper_imx219.cpp\n> @@ -23,21 +23,14 @@\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<const uint8_t> 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> +constexpr std::initializer_list<uint32_t> registerList [[maybe_unused]] =\n> { expHiReg, expLoReg, gainReg };\n>\n>  class CamHelperImx219 : public CamHelper\n>  {\n> @@ -54,11 +47,14 @@ private:\n>          * in units of lines.\n>          */\n>         static constexpr int frameIntegrationDiff = 4;\n> +\n> +       void PopulateMetadata(const MdParser::RegisterMap &registers,\n> +                             Metadata &metadata) const override;\n>  };\n>\n>  CamHelperImx219::CamHelperImx219()\n>  #if ENABLE_EMBEDDED_DATA\n> -       : CamHelper(std::make_unique<MdParserImx219>(),\n> frameIntegrationDiff)\n> +       : CamHelper(std::make_unique<MdParserSmia>(registerList),\n> frameIntegrationDiff)\n>  #else\n>         : CamHelper({}, frameIntegrationDiff)\n>  #endif\n> @@ -90,88 +86,20 @@ bool CamHelperImx219::SensorEmbeddedDataPresent() const\n>         return ENABLE_EMBEDDED_DATA;\n>  }\n>\n> +void CamHelperImx219::PopulateMetadata(const MdParser::RegisterMap\n> &registers,\n> +                                      Metadata &metadata) const\n> +{\n> +       DeviceStatus deviceStatus;\n> +\n> +       deviceStatus.shutter_speed = Exposure(registers.at(expHiReg) *\n> 256 + registers.at(expLoReg));\n> +       deviceStatus.analogue_gain = Gain(registers.at(gainReg));\n> +\n> +       metadata.Set(\"device.status\", deviceStatus);\n> +}\n> +\n>  static CamHelper *Create()\n>  {\n>         return new CamHelperImx219();\n>  }\n>\n>  static RegisterCamHelper reg(\"imx219\", &Create);\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> -}\n> -\n> -MdParser::Status MdParserImx219::Parse(libcamera::Span<const uint8_t>\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<int>(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> -{\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> -}\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> diff --git a/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> b/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> index d72a9be0438e..4098fde6f322 100644\n> --- a/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> +++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp\n> @@ -15,21 +15,15 @@\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<const uint8_t> 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> +constexpr std::initializer_list<uint32_t> registerList [[maybe_unused]] =\n> { expHiReg, expLoReg, gainHiReg, gainLoReg };\n>\n\nThis does not need to have [[maybe_unused]] as we do not switch metadata\nparsing off\nin the imx477.\n\nNot sure I am able to tag my own patch, but...\n\nReviewed-by: Naushir Patuck <naush@raspberrypi.com>\n\n:)\n\n\n>\n>  class CamHelperImx477 : public CamHelper\n>  {\n> @@ -47,10 +41,13 @@ private:\n>          * in units of lines.\n>          */\n>         static constexpr int frameIntegrationDiff = 22;\n> +\n> +       void PopulateMetadata(const MdParser::RegisterMap &registers,\n> +                             Metadata &metadata) const override;\n>  };\n>\n>  CamHelperImx477::CamHelperImx477()\n> -       : CamHelper(std::make_unique<MdParserImx477>(),\n> frameIntegrationDiff)\n> +       : CamHelper(std::make_unique<MdParserSmia>(registerList),\n> frameIntegrationDiff)\n>  {\n>  }\n>\n> @@ -77,95 +74,20 @@ bool CamHelperImx477::SensorEmbeddedDataPresent() const\n>         return true;\n>  }\n>\n> +void CamHelperImx477::PopulateMetadata(const MdParser::RegisterMap\n> &registers,\n> +                                      Metadata &metadata) const\n> +{\n> +       DeviceStatus deviceStatus;\n> +\n> +       deviceStatus.shutter_speed = Exposure(registers.at(expHiReg) *\n> 256 + registers.at(expLoReg));\n> +       deviceStatus.analogue_gain = Gain(registers.at(gainHiReg) * 256 +\n> registers.at(gainLoReg));\n> +\n> +       metadata.Set(\"device.status\", deviceStatus);\n> +}\n> +\n>  static CamHelper *Create()\n>  {\n>         return new CamHelperImx477();\n>  }\n>\n>  static RegisterCamHelper reg(\"imx477\", &Create);\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> -}\n> -\n> -MdParser::Status MdParserImx477::Parse(libcamera::Span<const uint8_t>\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<int>(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> -{\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> -}\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> diff --git a/src/ipa/raspberrypi/md_parser.hpp\n> b/src/ipa/raspberrypi/md_parser.hpp\n> index 8497216f8db7..e3e2738537a8 100644\n> --- a/src/ipa/raspberrypi/md_parser.hpp\n> +++ b/src/ipa/raspberrypi/md_parser.hpp\n> @@ -6,6 +6,9 @@\n>   */\n>  #pragma once\n>\n> +#include <initializer_list>\n> +#include <map>\n> +#include <optional>\n>  #include <stdint.h>\n>\n>  #include <libcamera/base/span.h>\n> @@ -19,7 +22,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 +35,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\n> time,\n>   * and gain_code / actual gain.)\n> @@ -59,6 +60,8 @@ namespace RPiController {\n>  class MdParser\n>  {\n>  public:\n> +       using RegisterMap = std::map<uint32_t, uint32_t>;\n> +\n>         /*\n>          * Parser status codes:\n>          * OK       - success\n> @@ -98,9 +101,8 @@ public:\n>                 line_length_bytes_ = num_bytes;\n>         }\n>\n> -       virtual Status Parse(libcamera::Span<const uint8_t> 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<const uint8_t> buffer,\n> +                            RegisterMap &registers) = 0;\n>\n>  protected:\n>         bool reset_;\n> @@ -116,14 +118,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(std::initializer_list<uint32_t> registerList);\n> +\n> +       MdParser::Status Parse(libcamera::Span<const uint8_t> buffer,\n> +                              RegisterMap &registers) override;\n> +\n> +private:\n> +       /* Maps register address to offset in the buffer. */\n> +       using OffsetMap = std::map<uint32_t, std::optional<uint32_t>>;\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 +147,9 @@ protected:\n>                 BAD_PADDING   = -5\n>         };\n>\n> -       ParseStatus findRegs(libcamera::Span<const uint8_t> buffer,\n> uint32_t regs[],\n> -                            int offsets[], unsigned int num_regs);\n> +       ParseStatus findRegs(libcamera::Span<const uint8_t> 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..ea5eac414b36 100644\n> --- a/src/ipa/raspberrypi/md_parser_smia.cpp\n> +++ b/src/ipa/raspberrypi/md_parser_smia.cpp\n> @@ -4,11 +4,12 @@\n>   *\n>   * md_parser_smia.cpp - SMIA specification based embedded data parser\n>   */\n> -#include <assert.h>\n>\n> +#include <libcamera/base/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 +27,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\n> uint8_t> buffer,\n> -                                                uint32_t regs[], int\n> offsets[],\n> -                                                unsigned int num_regs)\n> +MdParserSmia::MdParserSmia(std::initializer_list<uint32_t> registerList)\n>  {\n> -       assert(num_regs > 0);\n> +       for (auto r : registerList)\n> +               offsets_[r] = {};\n> +}\n> +\n> +MdParser::Status MdParserSmia::Parse(libcamera::Span<const uint8_t>\n> buffer,\n> +                                    RegisterMap &registers)\n> +{\n> +       if (reset_) {\n> +               /*\n> +                * Search again through the metadata for all the registers\n> +                * requested.\n> +                */\n> +               ASSERT(bits_per_pixel_);\n> +\n> +               for (const 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> +       registers.clear();\n> +       for (const auto &[reg, offset] : offsets_) {\n> +               if (!offset) {\n> +                       reset_ = true;\n> +                       return NOTFOUND;\n> +               }\n> +               registers[reg] = buffer[offset.value()];\n> +       }\n> +\n> +       return OK;\n> +}\n> +\n> +MdParserSmia::ParseStatus MdParserSmia::findRegs(libcamera::Span<const\n> 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 +133,12 @@ MdParserSmia::ParseStatus\n> MdParserSmia::findRegs(libcamera::Span<const uint8_t>\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 (++first_reg == num_regs)\n> +                               if (reg != offsets_.end()) {\n> +                                       offsets_[reg_num] = current_offset\n> - 1;\n> +\n> +                                       if (++regs_done == offsets_.size())\n>                                                 return PARSE_OK;\n>                                 }\n>                                 reg_num++;\n> --\n> Regards,\n>\n> Laurent Pinchart\n>\n>","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 2A551C321F\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed, 30 Jun 2021 08:36:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 67F06684EB;\n\tWed, 30 Jun 2021 10:36:13 +0200 (CEST)","from mail-lf1-x12f.google.com (mail-lf1-x12f.google.com\n\t[IPv6:2a00:1450:4864:20::12f])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7837A6028E\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 30 Jun 2021 10:36:11 +0200 (CEST)","by mail-lf1-x12f.google.com with SMTP id t17so3739516lfq.0\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 30 Jun 2021 01:36:11 -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=\"aQy4aIuG\"; 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=FtVmRSd07BG5pttfKHAtbl8S9GPGN6i/o4rW+2c7ImA=;\n\tb=aQy4aIuGEyJP6yowmhs3b9ziaNsN6drRYOaW3dT2CO8LiuX2ezxpFLOLvCyBA8JfY3\n\t8FM0o+7QZaO7lbLbNJcYuid29v8MTRdR7/vOVyn5WnIuVKVp4cuaCwNzuVb6Eq5GycqF\n\tqR+P630F2XVhoZYA0HnXoeR3oZAgIFytGuiVrb2q2sa243n4wNpxpdv8z4YlEw9QYGT0\n\tq55sPyVRmwuqV/iBJYfn8vYnXBSaT7y4b04uPOiqKDVSO+Dqp0E8lw0uxFUNOnziJBN2\n\tJhxMtZcknfd3KV5SWwzoXhtArTlxyjIW8T4uaNzUnKtRz0aD3Rns5YCV4ZHA2nDEYv7S\n\tRr4g==","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=FtVmRSd07BG5pttfKHAtbl8S9GPGN6i/o4rW+2c7ImA=;\n\tb=Yj1sBeK5YzesTTsVp+O1gcjVqUpvr7HLKnwOaqSWr3SeipdR+TZ2Y88OjF8QVrmNpr\n\t92tNb0GMyL5cdWdL8mn3uB8aKWkD9KJysrtMOulDEOLOw1TcT0ZsGSNRtbJftuGC9T17\n\tBu6xcmKGIzfOPIqUcW/UQ6/Eg7zO3vBQQPdP7EjSwrlxflin/S74LEnDgE0g46mKJRbS\n\tLC1Ybe2vZ3nv0NUKwCxsVfF6lDylYC25eJhPRbwoolglZo3zhlpieXriZ4hCDj9qLdQW\n\tQqH7P3ZO0bXEeaQpuVJGSwmEwJSELRI9TWA+cvp4+u6WWlRomYa0WpCln78jwN15tl1e\n\tyGiQ==","X-Gm-Message-State":"AOAM532xv+FD2YjrWhPCqt+DdTcK6Z7GigR70Vsgu0atcSmWt7xy8CzW\n\t/oCj/Nu+bs5jLvY+PCILSwOEC5EQurY/b2VJcXmXUg==","X-Google-Smtp-Source":"ABdhPJxs1fAiql/sk69Dpsu5HEqk1lyBI6rLjxGcJuJ9dBGX1IosFQC1EN26o0XrsOIn5NW/N2bjqyQ2i5MeYjdoed4=","X-Received":"by 2002:ac2:4342:: with SMTP id o2mr9834360lfl.375.1625042170493;\n\tWed, 30 Jun 2021 01:36:10 -0700 (PDT)","MIME-Version":"1.0","References":"<20210629104500.51672-3-naush@raspberrypi.com>\n\t<20210629154806.32341-1-laurent.pinchart@ideasonboard.com>","In-Reply-To":"<20210629154806.32341-1-laurent.pinchart@ideasonboard.com>","From":"Naushir Patuck <naush@raspberrypi.com>","Date":"Wed, 30 Jun 2021 09:35:54 +0100","Message-ID":"<CAEmqJPq68nFaRH5FXgcyv_NR4X_=2bSZM5X1N2VKOWB+Y26s_g@mail.gmail.com>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Content-Type":"multipart/alternative; boundary=\"000000000000514e9405c5f79d48\"","Subject":"Re: [libcamera-devel] [PATCH v3.1 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 <libcamera-devel@lists.libcamera.org>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]