[libcamera-devel,v3,2/2] ipa: raspberrypi: Generalise the SMIA metadata parser

12745 diff mbox series

Instead of having each CamHelper subclass the MdParserSmia, change the
implementation of MdParserSmia to be more generic. The MdParserSmia now gets
given a list of registers to search for and helper functions are used to compute
exposure lines and gain codes from these registers.

Update the imx219 and imx477 CamHelpers by using this new mechanism.

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 src/ipa/raspberrypi/cam_helper.cpp        |  33 +++---
 src/ipa/raspberrypi/cam_helper.hpp        |   2 +
 src/ipa/raspberrypi/cam_helper_imx219.cpp | 114 ++++----------------
 src/ipa/raspberrypi/cam_helper_imx477.cpp | 122 ++++------------------
 src/ipa/raspberrypi/md_parser.hpp         |  41 +++++---
 src/ipa/raspberrypi/md_parser_smia.cpp    |  66 ++++++++++--
 6 files changed, 144 insertions(+), 234 deletions(-)

On Tue, 29 Jun 2021 at 11:45, Naushir Patuck <naush@raspberrypi.com> wrote:

> Instead of having each CamHelper subclass the MdParserSmia, change the
> implementation of MdParserSmia to be more generic. The MdParserSmia now
> gets
> given a list of registers to search for and helper functions are used to
> compute
> exposure lines and gain codes from these registers.
>
> Update the imx219 and imx477 CamHelpers by using this new mechanism.
>
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  src/ipa/raspberrypi/cam_helper.cpp        |  33 +++---
>  src/ipa/raspberrypi/cam_helper.hpp        |   2 +
>  src/ipa/raspberrypi/cam_helper_imx219.cpp | 114 ++++----------------
>  src/ipa/raspberrypi/cam_helper_imx477.cpp | 122 ++++------------------
>  src/ipa/raspberrypi/md_parser.hpp         |  41 +++++---
>  src/ipa/raspberrypi/md_parser_smia.cpp    |  66 ++++++++++--
>  6 files changed, 144 insertions(+), 234 deletions(-)
>
> diff --git a/src/ipa/raspberrypi/cam_helper.cpp
> b/src/ipa/raspberrypi/cam_helper.cpp
> index 90498c37af98..e6d2258c66d7 100644
> --- a/src/ipa/raspberrypi/cam_helper.cpp
> +++ b/src/ipa/raspberrypi/cam_helper.cpp
> @@ -159,32 +159,34 @@ unsigned int CamHelper::MistrustFramesModeSwitch()
> const
>  void CamHelper::parseEmbeddedData(Span<const uint8_t> buffer,
>                                   Metadata &metadata)
>  {
> +       MdParser::RegisterMap registers;
> +       Metadata parsedMetadata;
> +
>         if (buffer.empty())
>                 return;
>
> -       uint32_t exposureLines, gainCode;
> -
> -       if (parser_->Parse(buffer) != MdParser::Status::OK ||
> -           parser_->GetExposureLines(exposureLines) !=
> MdParser::Status::OK ||
> -           parser_->GetGainCode(gainCode) != MdParser::Status::OK) {
> +       if (parser_->Parse(buffer, registers) != MdParser::Status::OK) {
>                 LOG(IPARPI, Error) << "Embedded data buffer parsing
> failed";
>                 return;
>         }
>
> +       PopulateMetadata(registers, parsedMetadata);
> +       metadata.Merge(parsedMetadata);
> +
>         /*
> -        * Overwrite the exposure/gain values in the DeviceStatus, as
> -        * we know better. Fetch it first in case any other fields were
> -        * set meaningfully.
> +        * Overwrite the exposure/gain values in the existing DeviceStatus
> with
> +        * values from the parsed embedded buffer. Fetch it first in case
> any
> +        * other fields were set meaningfully.
>          */
> -       DeviceStatus deviceStatus;
> -
> -       if (metadata.Get("device.status", deviceStatus) != 0) {
> +       DeviceStatus deviceStatus, parsedDeviceStatus;
> +       if (metadata.Get("device.status", deviceStatus) ||
> +           parsedMetadata.Get("device.status", parsedDeviceStatus)) {
>                 LOG(IPARPI, Error) << "DeviceStatus not found";
>                 return;
>         }
>
> -       deviceStatus.shutter_speed = Exposure(exposureLines);
> -       deviceStatus.analogue_gain = Gain(gainCode);
> +       deviceStatus.shutter_speed = parsedDeviceStatus.shutter_speed;
> +       deviceStatus.analogue_gain = parsedDeviceStatus.analogue_gain;
>
>         LOG(IPARPI, Debug) << "Metadata updated - Exposure : "
>                            << deviceStatus.shutter_speed
> @@ -194,6 +196,11 @@ void CamHelper::parseEmbeddedData(Span<const uint8_t>
> buffer,
>         metadata.Set("device.status", deviceStatus);
>  }
>
> +void CamHelper::PopulateMetadata([[maybe_unused]] const
> MdParser::RegisterMap &registers,
> +                                [[maybe_unused]] Metadata &metadata) const
> +{
> +}
> +
>  RegisterCamHelper::RegisterCamHelper(char const *cam_name,
>                                      CamHelperCreateFunc create_func)
>  {
> diff --git a/src/ipa/raspberrypi/cam_helper.hpp
> b/src/ipa/raspberrypi/cam_helper.hpp
> index fc3139e22be0..200cc83f3872 100644
> --- a/src/ipa/raspberrypi/cam_helper.hpp
> +++ b/src/ipa/raspberrypi/cam_helper.hpp
> @@ -92,6 +92,8 @@ public:
>  protected:
>         void parseEmbeddedData(libcamera::Span<const uint8_t> buffer,
>                                Metadata &metadata);
> +       virtual void PopulateMetadata(const MdParser::RegisterMap
> &registers,
> +                                     Metadata &metadata) const;
>
>         std::unique_ptr<MdParser> parser_;
>         CameraMode mode_;
> diff --git a/src/ipa/raspberrypi/cam_helper_imx219.cpp
> b/src/ipa/raspberrypi/cam_helper_imx219.cpp
> index c85044a5fa6d..4e4994188ff3 100644
> --- a/src/ipa/raspberrypi/cam_helper_imx219.cpp
> +++ b/src/ipa/raspberrypi/cam_helper_imx219.cpp
> @@ -23,21 +23,14 @@
>
>  using namespace RPiController;
>
> -/* Metadata parser implementation specific to Sony IMX219 sensors. */
> -
> -class MdParserImx219 : public MdParserSmia
> -{
> -public:
> -       MdParserImx219();
> -       Status Parse(libcamera::Span<const uint8_t> buffer) override;
> -       Status GetExposureLines(unsigned int &lines) override;
> -       Status GetGainCode(unsigned int &gain_code) override;
> -private:
> -       /* Offset of the register's value in the metadata block. */
> -       int reg_offsets_[3];
> -       /* Value of the register, once read from the metadata block. */
> -       int reg_values_[3];
> -};
> +/*
> + * We care about one gain register and a pair of exposure registers.
> Their I2C
> + * addresses from the Sony IMX219 datasheet:
> + */
> +constexpr uint32_t gainReg = 0x157;
> +constexpr uint32_t expHiReg = 0x15a;
> +constexpr uint32_t expLoReg = 0x15b;
> +constexpr std::initializer_list<uint32_t> registerList = { expHiReg,
> expLoReg, gainReg };
>
>  class CamHelperImx219 : public CamHelper
>  {
> @@ -54,11 +47,14 @@ private:
>          * in units of lines.
>          */
>         static constexpr int frameIntegrationDiff = 4;
> +
> +       void PopulateMetadata(const MdParser::RegisterMap &registers,
> +                             Metadata &metadata) const override;
>  };
>
>  CamHelperImx219::CamHelperImx219()
>  #if ENABLE_EMBEDDED_DATA
> -       : CamHelper(std::make_unique<MdParserImx219>(),
> frameIntegrationDiff)
> +       : CamHelper(std::make_unique<MdParserSmia>(registerList),
> frameIntegrationDiff)
>  #else
>         : CamHelper({}, frameIntegrationDiff)
>  #endif
> @@ -90,88 +86,20 @@ bool CamHelperImx219::SensorEmbeddedDataPresent() const
>         return ENABLE_EMBEDDED_DATA;
>  }
>
> -static CamHelper *Create()
> +void CamHelperImx219::PopulateMetadata(const MdParser::RegisterMap
> &registers,
> +                                      Metadata &metadata) const
>  {
> -       return new CamHelperImx219();
> -}
> +       DeviceStatus deviceStatus;
>
> -static RegisterCamHelper reg("imx219", &Create);
> +       deviceStatus.shutter_speed = Exposure(registers.at(expHiReg) *
> 256 + registers.at(expLoReg));
> +       deviceStatus.analogue_gain = Gain(registers.at(gainReg));
>
> -/*
> - * We care about one gain register and a pair of exposure registers.
> Their I2C
> - * addresses from the Sony IMX219 datasheet:
> - */
> -#define GAIN_REG 0x157
> -#define EXPHI_REG 0x15A
> -#define EXPLO_REG 0x15B
> -
> -/*
> - * Index of each into the reg_offsets and reg_values arrays. Must be in
> - * register address order.
> - */
> -#define GAIN_INDEX 0
> -#define EXPHI_INDEX 1
> -#define EXPLO_INDEX 2
> -
> -MdParserImx219::MdParserImx219()
> -{
> -       reg_offsets_[0] = reg_offsets_[1] = reg_offsets_[2] = -1;
> +       metadata.Set("device.status", deviceStatus);
>  }
>
> -MdParser::Status MdParserImx219::Parse(libcamera::Span<const uint8_t>
> buffer)
> -{
> -       bool try_again = false;
> -
> -       if (reset_) {
> -               /*
> -                * Search again through the metadata for the gain and
> exposure
> -                * registers.
> -                */
> -               assert(bits_per_pixel_);
> -               /* Need to be ordered */
> -               uint32_t regs[3] = { GAIN_REG, EXPHI_REG, EXPLO_REG };
> -               reg_offsets_[0] = reg_offsets_[1] = reg_offsets_[2] = -1;
> -               int ret = static_cast<int>(findRegs(buffer,
> -                                                   regs, reg_offsets_,
> 3));
> -               /*
> -                * > 0 means "worked partially but parse again next time",
> -                * < 0 means "hard error".
> -                */
> -               if (ret > 0)
> -                       try_again = true;
> -               else if (ret < 0)
> -                       return ERROR;
> -       }
> -
> -       for (int i = 0; i < 3; i++) {
> -               if (reg_offsets_[i] == -1)
> -                       continue;
> -
> -               reg_values_[i] = buffer[reg_offsets_[i]];
> -       }
> -
> -       /* Re-parse next time if we were unhappy in some way. */
> -       reset_ = try_again;
> -
> -       return OK;
> -}
> -
> -MdParser::Status MdParserImx219::GetExposureLines(unsigned int &lines)
> +static CamHelper *Create()
>  {
> -       if (reg_offsets_[EXPHI_INDEX] == -1 || reg_offsets_[EXPLO_INDEX]
> == -1)
> -               return NOTFOUND;
> -
> -       lines = reg_values_[EXPHI_INDEX] * 256 + reg_values_[EXPLO_INDEX];
> -
> -       return OK;
> +       return new CamHelperImx219();
>  }
>
> -MdParser::Status MdParserImx219::GetGainCode(unsigned int &gain_code)
> -{
> -       if (reg_offsets_[GAIN_INDEX] == -1)
> -               return NOTFOUND;
> -
> -       gain_code = reg_values_[GAIN_INDEX];
> -
> -       return OK;
> -}
> +static RegisterCamHelper reg("imx219", &Create);
> diff --git a/src/ipa/raspberrypi/cam_helper_imx477.cpp
> b/src/ipa/raspberrypi/cam_helper_imx477.cpp
> index d72a9be0438e..efd1a5893db8 100644
> --- a/src/ipa/raspberrypi/cam_helper_imx477.cpp
> +++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp
> @@ -15,21 +15,15 @@
>
>  using namespace RPiController;
>
> -/* Metadata parser implementation specific to Sony IMX477 sensors. */
> -
> -class MdParserImx477 : public MdParserSmia
> -{
> -public:
> -       MdParserImx477();
> -       Status Parse(libcamera::Span<const uint8_t> buffer) override;
> -       Status GetExposureLines(unsigned int &lines) override;
> -       Status GetGainCode(unsigned int &gain_code) override;
> -private:
> -       /* Offset of the register's value in the metadata block. */
> -       int reg_offsets_[4];
> -       /* Value of the register, once read from the metadata block. */
> -       int reg_values_[4];
> -};
> +/*
> + * We care about two gain registers and a pair of exposure registers.
> Their
> + * I2C addresses from the Sony IMX477 datasheet:
> + */
> +constexpr uint32_t expHiReg = 0x0202;
> +constexpr uint32_t expLoReg = 0x0203;
> +constexpr uint32_t gainHiReg = 0x0204;
> +constexpr uint32_t gainLoReg = 0x0205;
> +constexpr std::initializer_list<uint32_t> registerList = { expHiReg,
> expLoReg, gainHiReg, gainLoReg };
>
>  class CamHelperImx477 : public CamHelper
>  {
> @@ -47,10 +41,13 @@ private:
>          * in units of lines.
>          */
>         static constexpr int frameIntegrationDiff = 22;
> +
> +       void PopulateMetadata(const MdParser::RegisterMap &registers,
> +                             Metadata &metadata) const override;
>  };
>
>  CamHelperImx477::CamHelperImx477()
> -       : CamHelper(std::make_unique<MdParserImx477>(),
> frameIntegrationDiff)
> +       : CamHelper(std::make_unique<MdParserSmia>(registerList),
> frameIntegrationDiff)
>  {
>  }
>
> @@ -77,95 +74,20 @@ bool CamHelperImx477::SensorEmbeddedDataPresent() const
>         return true;
>  }
>
> -static CamHelper *Create()
> +void CamHelperImx477::PopulateMetadata(const MdParser::RegisterMap
> &registers,
> +                                      Metadata &metadata) const
>  {
> -       return new CamHelperImx477();
> -}
> +       DeviceStatus deviceStatus;
>
> -static RegisterCamHelper reg("imx477", &Create);
> +       deviceStatus.shutter_speed = Exposure(registers.at(expHiReg) *
> 256 + registers.at(expLoReg));
> +       deviceStatus.analogue_gain = Gain(registers.at(gainHiReg) * 256 +
> registers.at(gainLoReg));
>
> -/*
> - * We care about two gain registers and a pair of exposure registers.
> Their
> - * I2C addresses from the Sony IMX477 datasheet:
> - */
> -#define EXPHI_REG 0x0202
> -#define EXPLO_REG 0x0203
> -#define GAINHI_REG 0x0204
> -#define GAINLO_REG 0x0205
> -
> -/*
> - * Index of each into the reg_offsets and reg_values arrays. Must be in
> register
> - * address order.
> - */
> -#define EXPHI_INDEX 0
> -#define EXPLO_INDEX 1
> -#define GAINHI_INDEX 2
> -#define GAINLO_INDEX 3
> -
> -MdParserImx477::MdParserImx477()
> -{
> -       reg_offsets_[0] = reg_offsets_[1] = reg_offsets_[2] =
> reg_offsets_[3] = -1;
> +       metadata.Set("device.status", deviceStatus);
>  }
>
> -MdParser::Status MdParserImx477::Parse(libcamera::Span<const uint8_t>
> buffer)
> -{
> -       bool try_again = false;
> -
> -       if (reset_) {
> -               /*
> -                * Search again through the metadata for the gain and
> exposure
> -                * registers.
> -                */
> -               assert(bits_per_pixel_);
> -               /* Need to be ordered */
> -               uint32_t regs[4] = {
> -                       EXPHI_REG,
> -                       EXPLO_REG,
> -                       GAINHI_REG,
> -                       GAINLO_REG
> -               };
> -               reg_offsets_[0] = reg_offsets_[1] = reg_offsets_[2] =
> reg_offsets_[3] = -1;
> -               int ret = static_cast<int>(findRegs(buffer,
> -                                                   regs, reg_offsets_,
> 4));
> -               /*
> -                * > 0 means "worked partially but parse again next time",
> -                * < 0 means "hard error".
> -                */
> -               if (ret > 0)
> -                       try_again = true;
> -               else if (ret < 0)
> -                       return ERROR;
> -       }
> -
> -       for (int i = 0; i < 4; i++) {
> -               if (reg_offsets_[i] == -1)
> -                       continue;
> -
> -               reg_values_[i] = buffer[reg_offsets_[i]];
> -       }
> -
> -       /* Re-parse next time if we were unhappy in some way. */
> -       reset_ = try_again;
> -
> -       return OK;
> -}
> -
> -MdParser::Status MdParserImx477::GetExposureLines(unsigned int &lines)
> +static CamHelper *Create()
>  {
> -       if (reg_offsets_[EXPHI_INDEX] == -1 || reg_offsets_[EXPLO_INDEX]
> == -1)
> -               return NOTFOUND;
> -
> -       lines = reg_values_[EXPHI_INDEX] * 256 + reg_values_[EXPLO_INDEX];
> -
> -       return OK;
> +       return new CamHelperImx477();
>  }
>
> -MdParser::Status MdParserImx477::GetGainCode(unsigned int &gain_code)
> -{
> -       if (reg_offsets_[GAINHI_INDEX] == -1 || reg_offsets_[GAINLO_INDEX]
> == -1)
> -               return NOTFOUND;
> -
> -       gain_code = reg_values_[GAINHI_INDEX] * 256 +
> reg_values_[GAINLO_INDEX];
> -
> -       return OK;
> -}
> +static RegisterCamHelper reg("imx477", &Create);
> diff --git a/src/ipa/raspberrypi/md_parser.hpp
> b/src/ipa/raspberrypi/md_parser.hpp
> index 8497216f8db7..e3e2738537a8 100644
> --- a/src/ipa/raspberrypi/md_parser.hpp
> +++ b/src/ipa/raspberrypi/md_parser.hpp
> @@ -6,6 +6,9 @@
>   */
>  #pragma once
>
> +#include <initializer_list>
> +#include <map>
> +#include <optional>
>  #include <stdint.h>
>
>  #include <libcamera/base/span.h>
> @@ -19,7 +22,7 @@
>   * application code doesn't have to worry which kind to instantiate. But
> for
>   * the sake of example let's suppose we're parsing imx219 metadata.
>   *
> - * MdParser *parser = new MdParserImx219();  // for example
> + * MdParser *parser = new MdParserSmia({ expHiReg, expLoReg, gainReg });
>   * parser->SetBitsPerPixel(bpp);
>   * parser->SetLineLengthBytes(pitch);
>   * parser->SetNumLines(2);
> @@ -32,13 +35,11 @@
>   *
>   * Then on every frame:
>   *
> - * if (parser->Parse(buffer) != MdParser::OK)
> + * RegisterMap registers;
> + * if (parser->Parse(buffer, registers) != MdParser::OK)
>   *     much badness;
> - * unsigned int exposure_lines, gain_code
> - * if (parser->GetExposureLines(exposure_lines) != MdParser::OK)
> - *     exposure was not found;
> - * if (parser->GetGainCode(parser, gain_code) != MdParser::OK)
> - *     gain code was not found;
> + * Metadata metadata;
> + * CamHelper::PopulateMetadata(registers, metadata);
>   *
>   * (Note that the CamHelper class converts to/from exposure lines and
> time,
>   * and gain_code / actual gain.)
> @@ -59,6 +60,8 @@ namespace RPiController {
>  class MdParser
>  {
>  public:
> +       using RegisterMap = std::map<uint32_t, uint32_t>;
> +
>         /*
>          * Parser status codes:
>          * OK       - success
> @@ -98,9 +101,8 @@ public:
>                 line_length_bytes_ = num_bytes;
>         }
>
> -       virtual Status Parse(libcamera::Span<const uint8_t> buffer) = 0;
> -       virtual Status GetExposureLines(unsigned int &lines) = 0;
> -       virtual Status GetGainCode(unsigned int &gain_code) = 0;
> +       virtual Status Parse(libcamera::Span<const uint8_t> buffer,
> +                            RegisterMap &registers) = 0;
>
>  protected:
>         bool reset_;
> @@ -116,14 +118,18 @@ protected:
>   * md_parser_imx219.cpp for an example).
>   */
>
> -class MdParserSmia : public MdParser
> +class MdParserSmia final : public MdParser
>  {
>  public:
> -       MdParserSmia() : MdParser()
> -       {
> -       }
> +       MdParserSmia(std::initializer_list<uint32_t> registerList);
> +
> +       MdParser::Status Parse(libcamera::Span<const uint8_t> buffer,
> +                              RegisterMap &registers) override;
> +
> +private:
> +       /* Maps register address to offset in the buffer. */
> +       using OffsetMap = std::map<uint32_t, std::optional<uint32_t>>;
>
> -protected:
>         /*
>          * Note that error codes > 0 are regarded as non-fatal; codes < 0
>          * indicate a bad data buffer. Status codes are:
> @@ -141,8 +147,9 @@ protected:
>                 BAD_PADDING   = -5
>         };
>
> -       ParseStatus findRegs(libcamera::Span<const uint8_t> buffer,
> uint32_t regs[],
> -                            int offsets[], unsigned int num_regs);
> +       ParseStatus findRegs(libcamera::Span<const uint8_t> buffer);
> +
> +       OffsetMap offsets_;
>  };
>
>  } // namespace RPi
> diff --git a/src/ipa/raspberrypi/md_parser_smia.cpp
> b/src/ipa/raspberrypi/md_parser_smia.cpp
> index 0a14875575a2..7f72fc03948f 100644
> --- a/src/ipa/raspberrypi/md_parser_smia.cpp
> +++ b/src/ipa/raspberrypi/md_parser_smia.cpp
> @@ -6,9 +6,11 @@
>   */
>  #include <assert.h>
>

Oops, this header is not needed any more!



>
> +#include "libcamera/base/log.h"
>  #include "md_parser.hpp"
>
>  using namespace RPiController;
> +using namespace libcamera;
>
>  /*
>   * This function goes through the embedded data to find the offsets (not
> @@ -26,18 +28,61 @@ constexpr unsigned int REG_LOW_BITS = 0xa5;
>  constexpr unsigned int REG_VALUE = 0x5a;
>  constexpr unsigned int REG_SKIP = 0x55;
>
> -MdParserSmia::ParseStatus MdParserSmia::findRegs(libcamera::Span<const
> uint8_t> buffer,
> -                                                uint32_t regs[], int
> offsets[],
> -                                                unsigned int num_regs)
> +MdParserSmia::MdParserSmia(std::initializer_list<uint32_t> registerList)
>  {
> -       assert(num_regs > 0);
> +       for (auto r : registerList)
> +               offsets_[r] = {};
> +}
> +
> +MdParser::Status MdParserSmia::Parse(libcamera::Span<const uint8_t>
> buffer,
> +                                    RegisterMap &registers)
> +{
> +       if (reset_) {
> +               /*
> +                * Search again through the metadata for all the registers
> +                * requested.
> +                */
> +               ASSERT(bits_per_pixel_);
> +
> +               for (const auto &[reg, offset] : offsets_)
> +                       offsets_[reg] = {};
> +
> +               ParseStatus ret = findRegs(buffer);
> +               /*
> +                * > 0 means "worked partially but parse again next time",
> +                * < 0 means "hard error".
> +                *
> +                * In either case, we retry parsing on the next frame.
> +                */
> +               if (ret != PARSE_OK)
> +                       return ERROR;
> +
> +               reset_ = false;
> +       }
> +
> +       /* Populate the register values requested. */
> +       registers.clear();
> +       for (const auto &[reg, offset] : offsets_) {
> +               if (!offset) {
> +                       reset_ = true;
> +                       return NOTFOUND;
> +               }
> +               registers[reg] = buffer[offset.value()];
> +       }
> +
> +       return OK;
> +}
> +
> +MdParserSmia::ParseStatus MdParserSmia::findRegs(libcamera::Span<const
> uint8_t> buffer)
> +{
> +       ASSERT(offsets_.size());
>
>         if (buffer[0] != LINE_START)
>                 return NO_LINE_START;
>
>         unsigned int current_offset = 1; /* after the LINE_START */
>         unsigned int current_line_start = 0, current_line = 0;
> -       unsigned int reg_num = 0, first_reg = 0;
> +       unsigned int reg_num = 0, regs_done = 0;
>
>         while (1) {
>                 int tag = buffer[current_offset++];
> @@ -89,13 +134,12 @@ MdParserSmia::ParseStatus
> MdParserSmia::findRegs(libcamera::Span<const uint8_t>
>                         else if (tag == REG_SKIP)
>                                 reg_num++;
>                         else if (tag == REG_VALUE) {
> -                               while (reg_num >=
> -                                      /* assumes registers are in
> order... */
> -                                      regs[first_reg]) {
> -                                       if (reg_num == regs[first_reg])
> -                                               offsets[first_reg] =
> current_offset - 1;
> +                               auto reg = offsets_.find(reg_num);
> +
> +                               if (reg != offsets_.end()) {
> +                                       offsets_[reg_num] = current_offset
> - 1;
>
> -                                       if (++first_reg == num_regs)
> +                                       if (++regs_done == offsets_.size())
>                                                 return PARSE_OK;
>                                 }
>                                 reg_num++;
> --
> 2.25.1
>
>

Hi Naush,

On Tue, Jun 29, 2021 at 02:53:50PM +0100, Naushir Patuck wrote:
> On Tue, 29 Jun 2021 at 11:45, Naushir Patuck <naush@raspberrypi.com> wrote:
> 
> > Instead of having each CamHelper subclass the MdParserSmia, change the
> > implementation of MdParserSmia to be more generic. The MdParserSmia now gets
> > given a list of registers to search for and helper functions are used to compute
> > exposure lines and gain codes from these registers.
> >
> > Update the imx219 and imx477 CamHelpers by using this new mechanism.
> >
> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  src/ipa/raspberrypi/cam_helper.cpp        |  33 +++---
> >  src/ipa/raspberrypi/cam_helper.hpp        |   2 +
> >  src/ipa/raspberrypi/cam_helper_imx219.cpp | 114 ++++----------------
> >  src/ipa/raspberrypi/cam_helper_imx477.cpp | 122 ++++------------------
> >  src/ipa/raspberrypi/md_parser.hpp         |  41 +++++---
> >  src/ipa/raspberrypi/md_parser_smia.cpp    |  66 ++++++++++--
> >  6 files changed, 144 insertions(+), 234 deletions(-)
> >
> > diff --git a/src/ipa/raspberrypi/cam_helper.cpp b/src/ipa/raspberrypi/cam_helper.cpp
> > index 90498c37af98..e6d2258c66d7 100644
> > --- a/src/ipa/raspberrypi/cam_helper.cpp
> > +++ b/src/ipa/raspberrypi/cam_helper.cpp
> > @@ -159,32 +159,34 @@ unsigned int CamHelper::MistrustFramesModeSwitch() const
> >  void CamHelper::parseEmbeddedData(Span<const uint8_t> buffer,
> >                                   Metadata &metadata)
> >  {
> > +       MdParser::RegisterMap registers;
> > +       Metadata parsedMetadata;
> > +
> >         if (buffer.empty())
> >                 return;
> >
> > -       uint32_t exposureLines, gainCode;
> > -
> > -       if (parser_->Parse(buffer) != MdParser::Status::OK ||
> > -           parser_->GetExposureLines(exposureLines) != MdParser::Status::OK ||
> > -           parser_->GetGainCode(gainCode) != MdParser::Status::OK) {
> > +       if (parser_->Parse(buffer, registers) != MdParser::Status::OK) {
> >                 LOG(IPARPI, Error) << "Embedded data buffer parsing failed";
> >                 return;
> >         }
> >
> > +       PopulateMetadata(registers, parsedMetadata);
> > +       metadata.Merge(parsedMetadata);
> > +
> >         /*
> > -        * Overwrite the exposure/gain values in the DeviceStatus, as
> > -        * we know better. Fetch it first in case any other fields were
> > -        * set meaningfully.
> > +        * Overwrite the exposure/gain values in the existing DeviceStatus with
> > +        * values from the parsed embedded buffer. Fetch it first in case any
> > +        * other fields were set meaningfully.
> >          */
> > -       DeviceStatus deviceStatus;
> > -
> > -       if (metadata.Get("device.status", deviceStatus) != 0) {
> > +       DeviceStatus deviceStatus, parsedDeviceStatus;
> > +       if (metadata.Get("device.status", deviceStatus) ||
> > +           parsedMetadata.Get("device.status", parsedDeviceStatus)) {
> >                 LOG(IPARPI, Error) << "DeviceStatus not found";
> >                 return;
> >         }
> >
> > -       deviceStatus.shutter_speed = Exposure(exposureLines);
> > -       deviceStatus.analogue_gain = Gain(gainCode);
> > +       deviceStatus.shutter_speed = parsedDeviceStatus.shutter_speed;
> > +       deviceStatus.analogue_gain = parsedDeviceStatus.analogue_gain;
> >
> >         LOG(IPARPI, Debug) << "Metadata updated - Exposure : "
> >                            << deviceStatus.shutter_speed
> > @@ -194,6 +196,11 @@ void CamHelper::parseEmbeddedData(Span<const uint8_t> buffer,
> >         metadata.Set("device.status", deviceStatus);
> >  }
> >
> > +void CamHelper::PopulateMetadata([[maybe_unused]] const MdParser::RegisterMap &registers,
> > +                                [[maybe_unused]] Metadata &metadata) const
> > +{
> > +}
> > +
> >  RegisterCamHelper::RegisterCamHelper(char const *cam_name,
> >                                      CamHelperCreateFunc create_func)
> >  {
> > diff --git a/src/ipa/raspberrypi/cam_helper.hpp b/src/ipa/raspberrypi/cam_helper.hpp
> > index fc3139e22be0..200cc83f3872 100644
> > --- a/src/ipa/raspberrypi/cam_helper.hpp
> > +++ b/src/ipa/raspberrypi/cam_helper.hpp
> > @@ -92,6 +92,8 @@ public:
> >  protected:
> >         void parseEmbeddedData(libcamera::Span<const uint8_t> buffer,
> >                                Metadata &metadata);
> > +       virtual void PopulateMetadata(const MdParser::RegisterMap &registers,
> > +                                     Metadata &metadata) const;
> >
> >         std::unique_ptr<MdParser> parser_;
> >         CameraMode mode_;
> > diff --git a/src/ipa/raspberrypi/cam_helper_imx219.cpp b/src/ipa/raspberrypi/cam_helper_imx219.cpp
> > index c85044a5fa6d..4e4994188ff3 100644
> > --- a/src/ipa/raspberrypi/cam_helper_imx219.cpp
> > +++ b/src/ipa/raspberrypi/cam_helper_imx219.cpp
> > @@ -23,21 +23,14 @@
> >
> >  using namespace RPiController;
> >
> > -/* Metadata parser implementation specific to Sony IMX219 sensors. */
> > -
> > -class MdParserImx219 : public MdParserSmia
> > -{
> > -public:
> > -       MdParserImx219();
> > -       Status Parse(libcamera::Span<const uint8_t> buffer) override;
> > -       Status GetExposureLines(unsigned int &lines) override;
> > -       Status GetGainCode(unsigned int &gain_code) override;
> > -private:
> > -       /* Offset of the register's value in the metadata block. */
> > -       int reg_offsets_[3];
> > -       /* Value of the register, once read from the metadata block. */
> > -       int reg_values_[3];
> > -};
> > +/*
> > + * We care about one gain register and a pair of exposure registers. Their I2C
> > + * addresses from the Sony IMX219 datasheet:
> > + */
> > +constexpr uint32_t gainReg = 0x157;
> > +constexpr uint32_t expHiReg = 0x15a;
> > +constexpr uint32_t expLoReg = 0x15b;
> > +constexpr std::initializer_list<uint32_t> registerList = { expHiReg, expLoReg, gainReg };
> >
> >  class CamHelperImx219 : public CamHelper
> >  {
> > @@ -54,11 +47,14 @@ private:
> >          * in units of lines.
> >          */
> >         static constexpr int frameIntegrationDiff = 4;
> > +
> > +       void PopulateMetadata(const MdParser::RegisterMap &registers,
> > +                             Metadata &metadata) const override;
> >  };
> >
> >  CamHelperImx219::CamHelperImx219()
> >  #if ENABLE_EMBEDDED_DATA
> > -       : CamHelper(std::make_unique<MdParserImx219>(), frameIntegrationDiff)
> > +       : CamHelper(std::make_unique<MdParserSmia>(registerList), frameIntegrationDiff)
> >  #else
> >         : CamHelper({}, frameIntegrationDiff)
> >  #endif
> > @@ -90,88 +86,20 @@ bool CamHelperImx219::SensorEmbeddedDataPresent() const
> >         return ENABLE_EMBEDDED_DATA;
> >  }
> >
> > -static CamHelper *Create()
> > +void CamHelperImx219::PopulateMetadata(const MdParser::RegisterMap &registers,
> > +                                      Metadata &metadata) const
> >  {
> > -       return new CamHelperImx219();
> > -}
> > +       DeviceStatus deviceStatus;
> >
> > -static RegisterCamHelper reg("imx219", &Create);
> > +       deviceStatus.shutter_speed = Exposure(registers.at(expHiReg) * 256 + registers.at(expLoReg));
> > +       deviceStatus.analogue_gain = Gain(registers.at(gainReg));
> >
> > -/*
> > - * We care about one gain register and a pair of exposure registers. Their I2C
> > - * addresses from the Sony IMX219 datasheet:
> > - */
> > -#define GAIN_REG 0x157
> > -#define EXPHI_REG 0x15A
> > -#define EXPLO_REG 0x15B
> > -
> > -/*
> > - * Index of each into the reg_offsets and reg_values arrays. Must be in
> > - * register address order.
> > - */
> > -#define GAIN_INDEX 0
> > -#define EXPHI_INDEX 1
> > -#define EXPLO_INDEX 2
> > -
> > -MdParserImx219::MdParserImx219()
> > -{
> > -       reg_offsets_[0] = reg_offsets_[1] = reg_offsets_[2] = -1;
> > +       metadata.Set("device.status", deviceStatus);
> >  }
> >
> > -MdParser::Status MdParserImx219::Parse(libcamera::Span<const uint8_t> buffer)
> > -{
> > -       bool try_again = false;
> > -
> > -       if (reset_) {
> > -               /*
> > -                * Search again through the metadata for the gain and exposure
> > -                * registers.
> > -                */
> > -               assert(bits_per_pixel_);
> > -               /* Need to be ordered */
> > -               uint32_t regs[3] = { GAIN_REG, EXPHI_REG, EXPLO_REG };
> > -               reg_offsets_[0] = reg_offsets_[1] = reg_offsets_[2] = -1;
> > -               int ret = static_cast<int>(findRegs(buffer,
> > -                                                   regs, reg_offsets_, 3));
> > -               /*
> > -                * > 0 means "worked partially but parse again next time",
> > -                * < 0 means "hard error".
> > -                */
> > -               if (ret > 0)
> > -                       try_again = true;
> > -               else if (ret < 0)
> > -                       return ERROR;
> > -       }
> > -
> > -       for (int i = 0; i < 3; i++) {
> > -               if (reg_offsets_[i] == -1)
> > -                       continue;
> > -
> > -               reg_values_[i] = buffer[reg_offsets_[i]];
> > -       }
> > -
> > -       /* Re-parse next time if we were unhappy in some way. */
> > -       reset_ = try_again;
> > -
> > -       return OK;
> > -}
> > -
> > -MdParser::Status MdParserImx219::GetExposureLines(unsigned int &lines)
> > +static CamHelper *Create()
> >  {
> > -       if (reg_offsets_[EXPHI_INDEX] == -1 || reg_offsets_[EXPLO_INDEX] == -1)
> > -               return NOTFOUND;
> > -
> > -       lines = reg_values_[EXPHI_INDEX] * 256 + reg_values_[EXPLO_INDEX];
> > -
> > -       return OK;
> > +       return new CamHelperImx219();
> >  }
> >
> > -MdParser::Status MdParserImx219::GetGainCode(unsigned int &gain_code)
> > -{
> > -       if (reg_offsets_[GAIN_INDEX] == -1)
> > -               return NOTFOUND;
> > -
> > -       gain_code = reg_values_[GAIN_INDEX];
> > -
> > -       return OK;
> > -}
> > +static RegisterCamHelper reg("imx219", &Create);
> > diff --git a/src/ipa/raspberrypi/cam_helper_imx477.cpp b/src/ipa/raspberrypi/cam_helper_imx477.cpp
> > index d72a9be0438e..efd1a5893db8 100644
> > --- a/src/ipa/raspberrypi/cam_helper_imx477.cpp
> > +++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp
> > @@ -15,21 +15,15 @@
> >
> >  using namespace RPiController;
> >
> > -/* Metadata parser implementation specific to Sony IMX477 sensors. */
> > -
> > -class MdParserImx477 : public MdParserSmia
> > -{
> > -public:
> > -       MdParserImx477();
> > -       Status Parse(libcamera::Span<const uint8_t> buffer) override;
> > -       Status GetExposureLines(unsigned int &lines) override;
> > -       Status GetGainCode(unsigned int &gain_code) override;
> > -private:
> > -       /* Offset of the register's value in the metadata block. */
> > -       int reg_offsets_[4];
> > -       /* Value of the register, once read from the metadata block. */
> > -       int reg_values_[4];
> > -};
> > +/*
> > + * We care about two gain registers and a pair of exposure registers. Their
> > + * I2C addresses from the Sony IMX477 datasheet:
> > + */
> > +constexpr uint32_t expHiReg = 0x0202;
> > +constexpr uint32_t expLoReg = 0x0203;
> > +constexpr uint32_t gainHiReg = 0x0204;
> > +constexpr uint32_t gainLoReg = 0x0205;
> > +constexpr std::initializer_list<uint32_t> registerList = { expHiReg, expLoReg, gainHiReg, gainLoReg };
> >
> >  class CamHelperImx477 : public CamHelper
> >  {
> > @@ -47,10 +41,13 @@ private:
> >          * in units of lines.
> >          */
> >         static constexpr int frameIntegrationDiff = 22;
> > +
> > +       void PopulateMetadata(const MdParser::RegisterMap &registers,
> > +                             Metadata &metadata) const override;
> >  };
> >
> >  CamHelperImx477::CamHelperImx477()
> > -       : CamHelper(std::make_unique<MdParserImx477>(), frameIntegrationDiff)
> > +       : CamHelper(std::make_unique<MdParserSmia>(registerList), frameIntegrationDiff)
> >  {
> >  }
> >
> > @@ -77,95 +74,20 @@ bool CamHelperImx477::SensorEmbeddedDataPresent() const
> >         return true;
> >  }
> >
> > -static CamHelper *Create()
> > +void CamHelperImx477::PopulateMetadata(const MdParser::RegisterMap &registers,
> > +                                      Metadata &metadata) const
> >  {
> > -       return new CamHelperImx477();
> > -}
> > +       DeviceStatus deviceStatus;
> >
> > -static RegisterCamHelper reg("imx477", &Create);
> > +       deviceStatus.shutter_speed = Exposure(registers.at(expHiReg) * 256 + registers.at(expLoReg));
> > +       deviceStatus.analogue_gain = Gain(registers.at(gainHiReg) * 256 + registers.at(gainLoReg));
> >
> > -/*
> > - * We care about two gain registers and a pair of exposure registers. Their
> > - * I2C addresses from the Sony IMX477 datasheet:
> > - */
> > -#define EXPHI_REG 0x0202
> > -#define EXPLO_REG 0x0203
> > -#define GAINHI_REG 0x0204
> > -#define GAINLO_REG 0x0205
> > -
> > -/*
> > - * Index of each into the reg_offsets and reg_values arrays. Must be in register
> > - * address order.
> > - */
> > -#define EXPHI_INDEX 0
> > -#define EXPLO_INDEX 1
> > -#define GAINHI_INDEX 2
> > -#define GAINLO_INDEX 3
> > -
> > -MdParserImx477::MdParserImx477()
> > -{
> > -       reg_offsets_[0] = reg_offsets_[1] = reg_offsets_[2] = reg_offsets_[3] = -1;
> > +       metadata.Set("device.status", deviceStatus);
> >  }
> >
> > -MdParser::Status MdParserImx477::Parse(libcamera::Span<const uint8_t> buffer)
> > -{
> > -       bool try_again = false;
> > -
> > -       if (reset_) {
> > -               /*
> > -                * Search again through the metadata for the gain and exposure
> > -                * registers.
> > -                */
> > -               assert(bits_per_pixel_);
> > -               /* Need to be ordered */
> > -               uint32_t regs[4] = {
> > -                       EXPHI_REG,
> > -                       EXPLO_REG,
> > -                       GAINHI_REG,
> > -                       GAINLO_REG
> > -               };
> > -               reg_offsets_[0] = reg_offsets_[1] = reg_offsets_[2] = reg_offsets_[3] = -1;
> > -               int ret = static_cast<int>(findRegs(buffer,
> > -                                                   regs, reg_offsets_, 4));
> > -               /*
> > -                * > 0 means "worked partially but parse again next time",
> > -                * < 0 means "hard error".
> > -                */
> > -               if (ret > 0)
> > -                       try_again = true;
> > -               else if (ret < 0)
> > -                       return ERROR;
> > -       }
> > -
> > -       for (int i = 0; i < 4; i++) {
> > -               if (reg_offsets_[i] == -1)
> > -                       continue;
> > -
> > -               reg_values_[i] = buffer[reg_offsets_[i]];
> > -       }
> > -
> > -       /* Re-parse next time if we were unhappy in some way. */
> > -       reset_ = try_again;
> > -
> > -       return OK;
> > -}
> > -
> > -MdParser::Status MdParserImx477::GetExposureLines(unsigned int &lines)
> > +static CamHelper *Create()
> >  {
> > -       if (reg_offsets_[EXPHI_INDEX] == -1 || reg_offsets_[EXPLO_INDEX] == -1)
> > -               return NOTFOUND;
> > -
> > -       lines = reg_values_[EXPHI_INDEX] * 256 + reg_values_[EXPLO_INDEX];
> > -
> > -       return OK;
> > +       return new CamHelperImx477();
> >  }
> >
> > -MdParser::Status MdParserImx477::GetGainCode(unsigned int &gain_code)
> > -{
> > -       if (reg_offsets_[GAINHI_INDEX] == -1 || reg_offsets_[GAINLO_INDEX] == -1)
> > -               return NOTFOUND;
> > -
> > -       gain_code = reg_values_[GAINHI_INDEX] * 256 +
> > reg_values_[GAINLO_INDEX];
> > -
> > -       return OK;
> > -}
> > +static RegisterCamHelper reg("imx477", &Create);
> > diff --git a/src/ipa/raspberrypi/md_parser.hpp b/src/ipa/raspberrypi/md_parser.hpp
> > index 8497216f8db7..e3e2738537a8 100644
> > --- a/src/ipa/raspberrypi/md_parser.hpp
> > +++ b/src/ipa/raspberrypi/md_parser.hpp
> > @@ -6,6 +6,9 @@
> >   */
> >  #pragma once
> >
> > +#include <initializer_list>
> > +#include <map>
> > +#include <optional>
> >  #include <stdint.h>
> >
> >  #include <libcamera/base/span.h>
> > @@ -19,7 +22,7 @@
> >   * application code doesn't have to worry which kind to instantiate. But for
> >   * the sake of example let's suppose we're parsing imx219 metadata.
> >   *
> > - * MdParser *parser = new MdParserImx219();  // for example
> > + * MdParser *parser = new MdParserSmia({ expHiReg, expLoReg, gainReg });
> >   * parser->SetBitsPerPixel(bpp);
> >   * parser->SetLineLengthBytes(pitch);
> >   * parser->SetNumLines(2);
> > @@ -32,13 +35,11 @@
> >   *
> >   * Then on every frame:
> >   *
> > - * if (parser->Parse(buffer) != MdParser::OK)
> > + * RegisterMap registers;
> > + * if (parser->Parse(buffer, registers) != MdParser::OK)
> >   *     much badness;
> > - * unsigned int exposure_lines, gain_code
> > - * if (parser->GetExposureLines(exposure_lines) != MdParser::OK)
> > - *     exposure was not found;
> > - * if (parser->GetGainCode(parser, gain_code) != MdParser::OK)
> > - *     gain code was not found;
> > + * Metadata metadata;
> > + * CamHelper::PopulateMetadata(registers, metadata);
> >   *
> >   * (Note that the CamHelper class converts to/from exposure lines and time,
> >   * and gain_code / actual gain.)
> > @@ -59,6 +60,8 @@ namespace RPiController {
> >  class MdParser
> >  {
> >  public:
> > +       using RegisterMap = std::map<uint32_t, uint32_t>;
> > +
> >         /*
> >          * Parser status codes:
> >          * OK       - success
> > @@ -98,9 +101,8 @@ public:
> >                 line_length_bytes_ = num_bytes;
> >         }
> >
> > -       virtual Status Parse(libcamera::Span<const uint8_t> buffer) = 0;
> > -       virtual Status GetExposureLines(unsigned int &lines) = 0;
> > -       virtual Status GetGainCode(unsigned int &gain_code) = 0;
> > +       virtual Status Parse(libcamera::Span<const uint8_t> buffer,
> > +                            RegisterMap &registers) = 0;
> >
> >  protected:
> >         bool reset_;
> > @@ -116,14 +118,18 @@ protected:
> >   * md_parser_imx219.cpp for an example).
> >   */
> >
> > -class MdParserSmia : public MdParser
> > +class MdParserSmia final : public MdParser
> >  {
> >  public:
> > -       MdParserSmia() : MdParser()
> > -       {
> > -       }
> > +       MdParserSmia(std::initializer_list<uint32_t> registerList);
> > +
> > +       MdParser::Status Parse(libcamera::Span<const uint8_t> buffer,
> > +                              RegisterMap &registers) override;
> > +
> > +private:
> > +       /* Maps register address to offset in the buffer. */
> > +       using OffsetMap = std::map<uint32_t, std::optional<uint32_t>>;
> >
> > -protected:
> >         /*
> >          * Note that error codes > 0 are regarded as non-fatal; codes < 0
> >          * indicate a bad data buffer. Status codes are:
> > @@ -141,8 +147,9 @@ protected:
> >                 BAD_PADDING   = -5
> >         };
> >
> > -       ParseStatus findRegs(libcamera::Span<const uint8_t> buffer, uint32_t regs[],
> > -                            int offsets[], unsigned int num_regs);
> > +       ParseStatus findRegs(libcamera::Span<const uint8_t> buffer);
> > +
> > +       OffsetMap offsets_;
> >  };
> >
> >  } // namespace RPi
> > diff --git a/src/ipa/raspberrypi/md_parser_smia.cpp b/src/ipa/raspberrypi/md_parser_smia.cpp
> > index 0a14875575a2..7f72fc03948f 100644
> > --- a/src/ipa/raspberrypi/md_parser_smia.cpp
> > +++ b/src/ipa/raspberrypi/md_parser_smia.cpp
> > @@ -6,9 +6,11 @@
> >   */
> >  #include <assert.h>
> >
> 
> Oops, this header is not needed any more!
> 
> > +#include "libcamera/base/log.h"

And this should be <libcamera/base/log.h>. I'll fix when applying.

> >  #include "md_parser.hpp"
> >
> >  using namespace RPiController;
> > +using namespace libcamera;
> >
> >  /*
> >   * This function goes through the embedded data to find the offsets (not
> > @@ -26,18 +28,61 @@ constexpr unsigned int REG_LOW_BITS = 0xa5;
> >  constexpr unsigned int REG_VALUE = 0x5a;
> >  constexpr unsigned int REG_SKIP = 0x55;
> >
> > -MdParserSmia::ParseStatus MdParserSmia::findRegs(libcamera::Span<const uint8_t> buffer,
> > -                                                uint32_t regs[], int offsets[],
> > -                                                unsigned int num_regs)
> > +MdParserSmia::MdParserSmia(std::initializer_list<uint32_t> registerList)
> >  {
> > -       assert(num_regs > 0);
> > +       for (auto r : registerList)
> > +               offsets_[r] = {};
> > +}
> > +
> > +MdParser::Status MdParserSmia::Parse(libcamera::Span<const uint8_t> buffer,
> > +                                    RegisterMap &registers)
> > +{
> > +       if (reset_) {
> > +               /*
> > +                * Search again through the metadata for all the registers
> > +                * requested.
> > +                */
> > +               ASSERT(bits_per_pixel_);
> > +
> > +               for (const auto &[reg, offset] : offsets_)
> > +                       offsets_[reg] = {};
> > +
> > +               ParseStatus ret = findRegs(buffer);
> > +               /*
> > +                * > 0 means "worked partially but parse again next time",
> > +                * < 0 means "hard error".
> > +                *
> > +                * In either case, we retry parsing on the next frame.
> > +                */
> > +               if (ret != PARSE_OK)
> > +                       return ERROR;
> > +
> > +               reset_ = false;
> > +       }
> > +
> > +       /* Populate the register values requested. */
> > +       registers.clear();
> > +       for (const auto &[reg, offset] : offsets_) {
> > +               if (!offset) {
> > +                       reset_ = true;
> > +                       return NOTFOUND;
> > +               }
> > +               registers[reg] = buffer[offset.value()];
> > +       }
> > +
> > +       return OK;
> > +}
> > +
> > +MdParserSmia::ParseStatus MdParserSmia::findRegs(libcamera::Span<const uint8_t> buffer)
> > +{
> > +       ASSERT(offsets_.size());
> >
> >         if (buffer[0] != LINE_START)
> >                 return NO_LINE_START;
> >
> >         unsigned int current_offset = 1; /* after the LINE_START */
> >         unsigned int current_line_start = 0, current_line = 0;
> > -       unsigned int reg_num = 0, first_reg = 0;
> > +       unsigned int reg_num = 0, regs_done = 0;
> >
> >         while (1) {
> >                 int tag = buffer[current_offset++];
> > @@ -89,13 +134,12 @@ MdParserSmia::ParseStatus MdParserSmia::findRegs(libcamera::Span<const uint8_t>
> >                         else if (tag == REG_SKIP)
> >                                 reg_num++;
> >                         else if (tag == REG_VALUE) {
> > -                               while (reg_num >=
> > -                                      /* assumes registers are in order... */
> > -                                      regs[first_reg]) {
> > -                                       if (reg_num == regs[first_reg])
> > -                                               offsets[first_reg] = current_offset - 1;
> > +                               auto reg = offsets_.find(reg_num);
> > +
> > +                               if (reg != offsets_.end()) {
> > +                                       offsets_[reg_num] = current_offset - 1;
> >
> > -                                       if (++first_reg == num_regs)
> > +                                       if (++regs_done == offsets_.size())
> >                                                 return PARSE_OK;
> >                                 }
> >                                 reg_num++;

On Tue, 29 Jun 2021 at 15:44, Laurent Pinchart <
laurent.pinchart@ideasonboard.com> wrote:

> Hi Naush,
>
> On Tue, Jun 29, 2021 at 02:53:50PM +0100, Naushir Patuck wrote:
> > On Tue, 29 Jun 2021 at 11:45, Naushir Patuck <naush@raspberrypi.com>
> wrote:
> >
> > > Instead of having each CamHelper subclass the MdParserSmia, change the
> > > implementation of MdParserSmia to be more generic. The MdParserSmia
> now gets
> > > given a list of registers to search for and helper functions are used
> to compute
> > > exposure lines and gain codes from these registers.
> > >
> > > Update the imx219 and imx477 CamHelpers by using this new mechanism.
> > >
> > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > ---
> > >  src/ipa/raspberrypi/cam_helper.cpp        |  33 +++---
> > >  src/ipa/raspberrypi/cam_helper.hpp        |   2 +
> > >  src/ipa/raspberrypi/cam_helper_imx219.cpp | 114 ++++----------------
> > >  src/ipa/raspberrypi/cam_helper_imx477.cpp | 122 ++++------------------
> > >  src/ipa/raspberrypi/md_parser.hpp         |  41 +++++---
> > >  src/ipa/raspberrypi/md_parser_smia.cpp    |  66 ++++++++++--
> > >  6 files changed, 144 insertions(+), 234 deletions(-)
> > >
> > > diff --git a/src/ipa/raspberrypi/cam_helper.cpp
> b/src/ipa/raspberrypi/cam_helper.cpp
> > > index 90498c37af98..e6d2258c66d7 100644
> > > --- a/src/ipa/raspberrypi/cam_helper.cpp
> > > +++ b/src/ipa/raspberrypi/cam_helper.cpp
> > > @@ -159,32 +159,34 @@ unsigned int
> CamHelper::MistrustFramesModeSwitch() const
> > >  void CamHelper::parseEmbeddedData(Span<const uint8_t> buffer,
> > >                                   Metadata &metadata)
> > >  {
> > > +       MdParser::RegisterMap registers;
> > > +       Metadata parsedMetadata;
> > > +
> > >         if (buffer.empty())
> > >                 return;
> > >
> > > -       uint32_t exposureLines, gainCode;
> > > -
> > > -       if (parser_->Parse(buffer) != MdParser::Status::OK ||
> > > -           parser_->GetExposureLines(exposureLines) !=
> MdParser::Status::OK ||
> > > -           parser_->GetGainCode(gainCode) != MdParser::Status::OK) {
> > > +       if (parser_->Parse(buffer, registers) != MdParser::Status::OK)
> {
> > >                 LOG(IPARPI, Error) << "Embedded data buffer parsing
> failed";
> > >                 return;
> > >         }
> > >
> > > +       PopulateMetadata(registers, parsedMetadata);
> > > +       metadata.Merge(parsedMetadata);
> > > +
> > >         /*
> > > -        * Overwrite the exposure/gain values in the DeviceStatus, as
> > > -        * we know better. Fetch it first in case any other fields were
> > > -        * set meaningfully.
> > > +        * Overwrite the exposure/gain values in the existing
> DeviceStatus with
> > > +        * values from the parsed embedded buffer. Fetch it first in
> case any
> > > +        * other fields were set meaningfully.
> > >          */
> > > -       DeviceStatus deviceStatus;
> > > -
> > > -       if (metadata.Get("device.status", deviceStatus) != 0) {
> > > +       DeviceStatus deviceStatus, parsedDeviceStatus;
> > > +       if (metadata.Get("device.status", deviceStatus) ||
> > > +           parsedMetadata.Get("device.status", parsedDeviceStatus)) {
> > >                 LOG(IPARPI, Error) << "DeviceStatus not found";
> > >                 return;
> > >         }
> > >
> > > -       deviceStatus.shutter_speed = Exposure(exposureLines);
> > > -       deviceStatus.analogue_gain = Gain(gainCode);
> > > +       deviceStatus.shutter_speed = parsedDeviceStatus.shutter_speed;
> > > +       deviceStatus.analogue_gain = parsedDeviceStatus.analogue_gain;
> > >
> > >         LOG(IPARPI, Debug) << "Metadata updated - Exposure : "
> > >                            << deviceStatus.shutter_speed
> > > @@ -194,6 +196,11 @@ void CamHelper::parseEmbeddedData(Span<const
> uint8_t> buffer,
> > >         metadata.Set("device.status", deviceStatus);
> > >  }
> > >
> > > +void CamHelper::PopulateMetadata([[maybe_unused]] const
> MdParser::RegisterMap &registers,
> > > +                                [[maybe_unused]] Metadata &metadata)
> const
> > > +{
> > > +}
> > > +
> > >  RegisterCamHelper::RegisterCamHelper(char const *cam_name,
> > >                                      CamHelperCreateFunc create_func)
> > >  {
> > > diff --git a/src/ipa/raspberrypi/cam_helper.hpp
> b/src/ipa/raspberrypi/cam_helper.hpp
> > > index fc3139e22be0..200cc83f3872 100644
> > > --- a/src/ipa/raspberrypi/cam_helper.hpp
> > > +++ b/src/ipa/raspberrypi/cam_helper.hpp
> > > @@ -92,6 +92,8 @@ public:
> > >  protected:
> > >         void parseEmbeddedData(libcamera::Span<const uint8_t> buffer,
> > >                                Metadata &metadata);
> > > +       virtual void PopulateMetadata(const MdParser::RegisterMap
> &registers,
> > > +                                     Metadata &metadata) const;
> > >
> > >         std::unique_ptr<MdParser> parser_;
> > >         CameraMode mode_;
> > > diff --git a/src/ipa/raspberrypi/cam_helper_imx219.cpp
> b/src/ipa/raspberrypi/cam_helper_imx219.cpp
> > > index c85044a5fa6d..4e4994188ff3 100644
> > > --- a/src/ipa/raspberrypi/cam_helper_imx219.cpp
> > > +++ b/src/ipa/raspberrypi/cam_helper_imx219.cpp
> > > @@ -23,21 +23,14 @@
> > >
> > >  using namespace RPiController;
> > >
> > > -/* Metadata parser implementation specific to Sony IMX219 sensors. */
> > > -
> > > -class MdParserImx219 : public MdParserSmia
> > > -{
> > > -public:
> > > -       MdParserImx219();
> > > -       Status Parse(libcamera::Span<const uint8_t> buffer) override;
> > > -       Status GetExposureLines(unsigned int &lines) override;
> > > -       Status GetGainCode(unsigned int &gain_code) override;
> > > -private:
> > > -       /* Offset of the register's value in the metadata block. */
> > > -       int reg_offsets_[3];
> > > -       /* Value of the register, once read from the metadata block. */
> > > -       int reg_values_[3];
> > > -};
> > > +/*
> > > + * We care about one gain register and a pair of exposure registers.
> Their I2C
> > > + * addresses from the Sony IMX219 datasheet:
> > > + */
> > > +constexpr uint32_t gainReg = 0x157;
> > > +constexpr uint32_t expHiReg = 0x15a;
> > > +constexpr uint32_t expLoReg = 0x15b;
> > > +constexpr std::initializer_list<uint32_t> registerList = { expHiReg,
> expLoReg, gainReg };
> > >
> > >  class CamHelperImx219 : public CamHelper
> > >  {
> > > @@ -54,11 +47,14 @@ private:
> > >          * in units of lines.
> > >          */
> > >         static constexpr int frameIntegrationDiff = 4;
> > > +
> > > +       void PopulateMetadata(const MdParser::RegisterMap &registers,
> > > +                             Metadata &metadata) const override;
> > >  };
> > >
> > >  CamHelperImx219::CamHelperImx219()
> > >  #if ENABLE_EMBEDDED_DATA
> > > -       : CamHelper(std::make_unique<MdParserImx219>(),
> frameIntegrationDiff)
> > > +       : CamHelper(std::make_unique<MdParserSmia>(registerList),
> frameIntegrationDiff)
> > >  #else
> > >         : CamHelper({}, frameIntegrationDiff)
> > >  #endif
> > > @@ -90,88 +86,20 @@ bool CamHelperImx219::SensorEmbeddedDataPresent()
> const
> > >         return ENABLE_EMBEDDED_DATA;
> > >  }
> > >
> > > -static CamHelper *Create()
> > > +void CamHelperImx219::PopulateMetadata(const MdParser::RegisterMap
> &registers,
> > > +                                      Metadata &metadata) const
> > >  {
> > > -       return new CamHelperImx219();
> > > -}
> > > +       DeviceStatus deviceStatus;
> > >
> > > -static RegisterCamHelper reg("imx219", &Create);
> > > +       deviceStatus.shutter_speed = Exposure(registers.at(expHiReg)
> * 256 + registers.at(expLoReg));
> > > +       deviceStatus.analogue_gain = Gain(registers.at(gainReg));
> > >
> > > -/*
> > > - * We care about one gain register and a pair of exposure registers.
> Their I2C
> > > - * addresses from the Sony IMX219 datasheet:
> > > - */
> > > -#define GAIN_REG 0x157
> > > -#define EXPHI_REG 0x15A
> > > -#define EXPLO_REG 0x15B
> > > -
> > > -/*
> > > - * Index of each into the reg_offsets and reg_values arrays. Must be
> in
> > > - * register address order.
> > > - */
> > > -#define GAIN_INDEX 0
> > > -#define EXPHI_INDEX 1
> > > -#define EXPLO_INDEX 2
> > > -
> > > -MdParserImx219::MdParserImx219()
> > > -{
> > > -       reg_offsets_[0] = reg_offsets_[1] = reg_offsets_[2] = -1;
> > > +       metadata.Set("device.status", deviceStatus);
> > >  }
> > >
> > > -MdParser::Status MdParserImx219::Parse(libcamera::Span<const uint8_t>
> buffer)
> > > -{
> > > -       bool try_again = false;
> > > -
> > > -       if (reset_) {
> > > -               /*
> > > -                * Search again through the metadata for the gain and
> exposure
> > > -                * registers.
> > > -                */
> > > -               assert(bits_per_pixel_);
> > > -               /* Need to be ordered */
> > > -               uint32_t regs[3] = { GAIN_REG, EXPHI_REG, EXPLO_REG };
> > > -               reg_offsets_[0] = reg_offsets_[1] = reg_offsets_[2] =
> -1;
> > > -               int ret = static_cast<int>(findRegs(buffer,
> > > -                                                   regs,
> reg_offsets_, 3));
> > > -               /*
> > > -                * > 0 means "worked partially but parse again next
> time",
> > > -                * < 0 means "hard error".
> > > -                */
> > > -               if (ret > 0)
> > > -                       try_again = true;
> > > -               else if (ret < 0)
> > > -                       return ERROR;
> > > -       }
> > > -
> > > -       for (int i = 0; i < 3; i++) {
> > > -               if (reg_offsets_[i] == -1)
> > > -                       continue;
> > > -
> > > -               reg_values_[i] = buffer[reg_offsets_[i]];
> > > -       }
> > > -
> > > -       /* Re-parse next time if we were unhappy in some way. */
> > > -       reset_ = try_again;
> > > -
> > > -       return OK;
> > > -}
> > > -
> > > -MdParser::Status MdParserImx219::GetExposureLines(unsigned int &lines)
> > > +static CamHelper *Create()
> > >  {
> > > -       if (reg_offsets_[EXPHI_INDEX] == -1 ||
> reg_offsets_[EXPLO_INDEX] == -1)
> > > -               return NOTFOUND;
> > > -
> > > -       lines = reg_values_[EXPHI_INDEX] * 256 +
> reg_values_[EXPLO_INDEX];
> > > -
> > > -       return OK;
> > > +       return new CamHelperImx219();
> > >  }
> > >
> > > -MdParser::Status MdParserImx219::GetGainCode(unsigned int &gain_code)
> > > -{
> > > -       if (reg_offsets_[GAIN_INDEX] == -1)
> > > -               return NOTFOUND;
> > > -
> > > -       gain_code = reg_values_[GAIN_INDEX];
> > > -
> > > -       return OK;
> > > -}
> > > +static RegisterCamHelper reg("imx219", &Create);
> > > diff --git a/src/ipa/raspberrypi/cam_helper_imx477.cpp
> b/src/ipa/raspberrypi/cam_helper_imx477.cpp
> > > index d72a9be0438e..efd1a5893db8 100644
> > > --- a/src/ipa/raspberrypi/cam_helper_imx477.cpp
> > > +++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp
> > > @@ -15,21 +15,15 @@
> > >
> > >  using namespace RPiController;
> > >
> > > -/* Metadata parser implementation specific to Sony IMX477 sensors. */
> > > -
> > > -class MdParserImx477 : public MdParserSmia
> > > -{
> > > -public:
> > > -       MdParserImx477();
> > > -       Status Parse(libcamera::Span<const uint8_t> buffer) override;
> > > -       Status GetExposureLines(unsigned int &lines) override;
> > > -       Status GetGainCode(unsigned int &gain_code) override;
> > > -private:
> > > -       /* Offset of the register's value in the metadata block. */
> > > -       int reg_offsets_[4];
> > > -       /* Value of the register, once read from the metadata block. */
> > > -       int reg_values_[4];
> > > -};
> > > +/*
> > > + * We care about two gain registers and a pair of exposure registers.
> Their
> > > + * I2C addresses from the Sony IMX477 datasheet:
> > > + */
> > > +constexpr uint32_t expHiReg = 0x0202;
> > > +constexpr uint32_t expLoReg = 0x0203;
> > > +constexpr uint32_t gainHiReg = 0x0204;
> > > +constexpr uint32_t gainLoReg = 0x0205;
> > > +constexpr std::initializer_list<uint32_t> registerList = { expHiReg,
> expLoReg, gainHiReg, gainLoReg };
> > >
> > >  class CamHelperImx477 : public CamHelper
> > >  {
> > > @@ -47,10 +41,13 @@ private:
> > >          * in units of lines.
> > >          */
> > >         static constexpr int frameIntegrationDiff = 22;
> > > +
> > > +       void PopulateMetadata(const MdParser::RegisterMap &registers,
> > > +                             Metadata &metadata) const override;
> > >  };
> > >
> > >  CamHelperImx477::CamHelperImx477()
> > > -       : CamHelper(std::make_unique<MdParserImx477>(),
> frameIntegrationDiff)
> > > +       : CamHelper(std::make_unique<MdParserSmia>(registerList),
> frameIntegrationDiff)
> > >  {
> > >  }
> > >
> > > @@ -77,95 +74,20 @@ bool CamHelperImx477::SensorEmbeddedDataPresent()
> const
> > >         return true;
> > >  }
> > >
> > > -static CamHelper *Create()
> > > +void CamHelperImx477::PopulateMetadata(const MdParser::RegisterMap
> &registers,
> > > +                                      Metadata &metadata) const
> > >  {
> > > -       return new CamHelperImx477();
> > > -}
> > > +       DeviceStatus deviceStatus;
> > >
> > > -static RegisterCamHelper reg("imx477", &Create);
> > > +       deviceStatus.shutter_speed = Exposure(registers.at(expHiReg)
> * 256 + registers.at(expLoReg));
> > > +       deviceStatus.analogue_gain = Gain(registers.at(gainHiReg) *
> 256 + registers.at(gainLoReg));
> > >
> > > -/*
> > > - * We care about two gain registers and a pair of exposure registers.
> Their
> > > - * I2C addresses from the Sony IMX477 datasheet:
> > > - */
> > > -#define EXPHI_REG 0x0202
> > > -#define EXPLO_REG 0x0203
> > > -#define GAINHI_REG 0x0204
> > > -#define GAINLO_REG 0x0205
> > > -
> > > -/*
> > > - * Index of each into the reg_offsets and reg_values arrays. Must be
> in register
> > > - * address order.
> > > - */
> > > -#define EXPHI_INDEX 0
> > > -#define EXPLO_INDEX 1
> > > -#define GAINHI_INDEX 2
> > > -#define GAINLO_INDEX 3
> > > -
> > > -MdParserImx477::MdParserImx477()
> > > -{
> > > -       reg_offsets_[0] = reg_offsets_[1] = reg_offsets_[2] =
> reg_offsets_[3] = -1;
> > > +       metadata.Set("device.status", deviceStatus);
> > >  }
> > >
> > > -MdParser::Status MdParserImx477::Parse(libcamera::Span<const uint8_t>
> buffer)
> > > -{
> > > -       bool try_again = false;
> > > -
> > > -       if (reset_) {
> > > -               /*
> > > -                * Search again through the metadata for the gain and
> exposure
> > > -                * registers.
> > > -                */
> > > -               assert(bits_per_pixel_);
> > > -               /* Need to be ordered */
> > > -               uint32_t regs[4] = {
> > > -                       EXPHI_REG,
> > > -                       EXPLO_REG,
> > > -                       GAINHI_REG,
> > > -                       GAINLO_REG
> > > -               };
> > > -               reg_offsets_[0] = reg_offsets_[1] = reg_offsets_[2] =
> reg_offsets_[3] = -1;
> > > -               int ret = static_cast<int>(findRegs(buffer,
> > > -                                                   regs,
> reg_offsets_, 4));
> > > -               /*
> > > -                * > 0 means "worked partially but parse again next
> time",
> > > -                * < 0 means "hard error".
> > > -                */
> > > -               if (ret > 0)
> > > -                       try_again = true;
> > > -               else if (ret < 0)
> > > -                       return ERROR;
> > > -       }
> > > -
> > > -       for (int i = 0; i < 4; i++) {
> > > -               if (reg_offsets_[i] == -1)
> > > -                       continue;
> > > -
> > > -               reg_values_[i] = buffer[reg_offsets_[i]];
> > > -       }
> > > -
> > > -       /* Re-parse next time if we were unhappy in some way. */
> > > -       reset_ = try_again;
> > > -
> > > -       return OK;
> > > -}
> > > -
> > > -MdParser::Status MdParserImx477::GetExposureLines(unsigned int &lines)
> > > +static CamHelper *Create()
> > >  {
> > > -       if (reg_offsets_[EXPHI_INDEX] == -1 ||
> reg_offsets_[EXPLO_INDEX] == -1)
> > > -               return NOTFOUND;
> > > -
> > > -       lines = reg_values_[EXPHI_INDEX] * 256 +
> reg_values_[EXPLO_INDEX];
> > > -
> > > -       return OK;
> > > +       return new CamHelperImx477();
> > >  }
> > >
> > > -MdParser::Status MdParserImx477::GetGainCode(unsigned int &gain_code)
> > > -{
> > > -       if (reg_offsets_[GAINHI_INDEX] == -1 ||
> reg_offsets_[GAINLO_INDEX] == -1)
> > > -               return NOTFOUND;
> > > -
> > > -       gain_code = reg_values_[GAINHI_INDEX] * 256 +
> > > reg_values_[GAINLO_INDEX];
> > > -
> > > -       return OK;
> > > -}
> > > +static RegisterCamHelper reg("imx477", &Create);
> > > diff --git a/src/ipa/raspberrypi/md_parser.hpp
> b/src/ipa/raspberrypi/md_parser.hpp
> > > index 8497216f8db7..e3e2738537a8 100644
> > > --- a/src/ipa/raspberrypi/md_parser.hpp
> > > +++ b/src/ipa/raspberrypi/md_parser.hpp
> > > @@ -6,6 +6,9 @@
> > >   */
> > >  #pragma once
> > >
> > > +#include <initializer_list>
> > > +#include <map>
> > > +#include <optional>
> > >  #include <stdint.h>
> > >
> > >  #include <libcamera/base/span.h>
> > > @@ -19,7 +22,7 @@
> > >   * application code doesn't have to worry which kind to instantiate.
> But for
> > >   * the sake of example let's suppose we're parsing imx219 metadata.
> > >   *
> > > - * MdParser *parser = new MdParserImx219();  // for example
> > > + * MdParser *parser = new MdParserSmia({ expHiReg, expLoReg, gainReg
> });
> > >   * parser->SetBitsPerPixel(bpp);
> > >   * parser->SetLineLengthBytes(pitch);
> > >   * parser->SetNumLines(2);
> > > @@ -32,13 +35,11 @@
> > >   *
> > >   * Then on every frame:
> > >   *
> > > - * if (parser->Parse(buffer) != MdParser::OK)
> > > + * RegisterMap registers;
> > > + * if (parser->Parse(buffer, registers) != MdParser::OK)
> > >   *     much badness;
> > > - * unsigned int exposure_lines, gain_code
> > > - * if (parser->GetExposureLines(exposure_lines) != MdParser::OK)
> > > - *     exposure was not found;
> > > - * if (parser->GetGainCode(parser, gain_code) != MdParser::OK)
> > > - *     gain code was not found;
> > > + * Metadata metadata;
> > > + * CamHelper::PopulateMetadata(registers, metadata);
> > >   *
> > >   * (Note that the CamHelper class converts to/from exposure lines and
> time,
> > >   * and gain_code / actual gain.)
> > > @@ -59,6 +60,8 @@ namespace RPiController {
> > >  class MdParser
> > >  {
> > >  public:
> > > +       using RegisterMap = std::map<uint32_t, uint32_t>;
> > > +
> > >         /*
> > >          * Parser status codes:
> > >          * OK       - success
> > > @@ -98,9 +101,8 @@ public:
> > >                 line_length_bytes_ = num_bytes;
> > >         }
> > >
> > > -       virtual Status Parse(libcamera::Span<const uint8_t> buffer) =
> 0;
> > > -       virtual Status GetExposureLines(unsigned int &lines) = 0;
> > > -       virtual Status GetGainCode(unsigned int &gain_code) = 0;
> > > +       virtual Status Parse(libcamera::Span<const uint8_t> buffer,
> > > +                            RegisterMap &registers) = 0;
> > >
> > >  protected:
> > >         bool reset_;
> > > @@ -116,14 +118,18 @@ protected:
> > >   * md_parser_imx219.cpp for an example).
> > >   */
> > >
> > > -class MdParserSmia : public MdParser
> > > +class MdParserSmia final : public MdParser
> > >  {
> > >  public:
> > > -       MdParserSmia() : MdParser()
> > > -       {
> > > -       }
> > > +       MdParserSmia(std::initializer_list<uint32_t> registerList);
> > > +
> > > +       MdParser::Status Parse(libcamera::Span<const uint8_t> buffer,
> > > +                              RegisterMap &registers) override;
> > > +
> > > +private:
> > > +       /* Maps register address to offset in the buffer. */
> > > +       using OffsetMap = std::map<uint32_t, std::optional<uint32_t>>;
> > >
> > > -protected:
> > >         /*
> > >          * Note that error codes > 0 are regarded as non-fatal; codes
> < 0
> > >          * indicate a bad data buffer. Status codes are:
> > > @@ -141,8 +147,9 @@ protected:
> > >                 BAD_PADDING   = -5
> > >         };
> > >
> > > -       ParseStatus findRegs(libcamera::Span<const uint8_t> buffer,
> uint32_t regs[],
> > > -                            int offsets[], unsigned int num_regs);
> > > +       ParseStatus findRegs(libcamera::Span<const uint8_t> buffer);
> > > +
> > > +       OffsetMap offsets_;
> > >  };
> > >
> > >  } // namespace RPi
> > > diff --git a/src/ipa/raspberrypi/md_parser_smia.cpp
> b/src/ipa/raspberrypi/md_parser_smia.cpp
> > > index 0a14875575a2..7f72fc03948f 100644
> > > --- a/src/ipa/raspberrypi/md_parser_smia.cpp
> > > +++ b/src/ipa/raspberrypi/md_parser_smia.cpp
> > > @@ -6,9 +6,11 @@
> > >   */
> > >  #include <assert.h>
> > >
> >
> > Oops, this header is not needed any more!
> >
> > > +#include "libcamera/base/log.h"
>
> And this should be <libcamera/base/log.h>. I'll fix when applying.
>

Thanks Laurent!


>
> > >  #include "md_parser.hpp"
> > >
> > >  using namespace RPiController;
> > > +using namespace libcamera;
> > >
> > >  /*
> > >   * This function goes through the embedded data to find the offsets
> (not
> > > @@ -26,18 +28,61 @@ constexpr unsigned int REG_LOW_BITS = 0xa5;
> > >  constexpr unsigned int REG_VALUE = 0x5a;
> > >  constexpr unsigned int REG_SKIP = 0x55;
> > >
> > > -MdParserSmia::ParseStatus
> MdParserSmia::findRegs(libcamera::Span<const uint8_t> buffer,
> > > -                                                uint32_t regs[], int
> offsets[],
> > > -                                                unsigned int num_regs)
> > > +MdParserSmia::MdParserSmia(std::initializer_list<uint32_t>
> registerList)
> > >  {
> > > -       assert(num_regs > 0);
> > > +       for (auto r : registerList)
> > > +               offsets_[r] = {};
> > > +}
> > > +
> > > +MdParser::Status MdParserSmia::Parse(libcamera::Span<const uint8_t>
> buffer,
> > > +                                    RegisterMap &registers)
> > > +{
> > > +       if (reset_) {
> > > +               /*
> > > +                * Search again through the metadata for all the
> registers
> > > +                * requested.
> > > +                */
> > > +               ASSERT(bits_per_pixel_);
> > > +
> > > +               for (const auto &[reg, offset] : offsets_)
> > > +                       offsets_[reg] = {};
> > > +
> > > +               ParseStatus ret = findRegs(buffer);
> > > +               /*
> > > +                * > 0 means "worked partially but parse again next
> time",
> > > +                * < 0 means "hard error".
> > > +                *
> > > +                * In either case, we retry parsing on the next frame.
> > > +                */
> > > +               if (ret != PARSE_OK)
> > > +                       return ERROR;
> > > +
> > > +               reset_ = false;
> > > +       }
> > > +
> > > +       /* Populate the register values requested. */
> > > +       registers.clear();
> > > +       for (const auto &[reg, offset] : offsets_) {
> > > +               if (!offset) {
> > > +                       reset_ = true;
> > > +                       return NOTFOUND;
> > > +               }
> > > +               registers[reg] = buffer[offset.value()];
> > > +       }
> > > +
> > > +       return OK;
> > > +}
> > > +
> > > +MdParserSmia::ParseStatus
> MdParserSmia::findRegs(libcamera::Span<const uint8_t> buffer)
> > > +{
> > > +       ASSERT(offsets_.size());
> > >
> > >         if (buffer[0] != LINE_START)
> > >                 return NO_LINE_START;
> > >
> > >         unsigned int current_offset = 1; /* after the LINE_START */
> > >         unsigned int current_line_start = 0, current_line = 0;
> > > -       unsigned int reg_num = 0, first_reg = 0;
> > > +       unsigned int reg_num = 0, regs_done = 0;
> > >
> > >         while (1) {
> > >                 int tag = buffer[current_offset++];
> > > @@ -89,13 +134,12 @@ MdParserSmia::ParseStatus
> MdParserSmia::findRegs(libcamera::Span<const uint8_t>
> > >                         else if (tag == REG_SKIP)
> > >                                 reg_num++;
> > >                         else if (tag == REG_VALUE) {
> > > -                               while (reg_num >=
> > > -                                      /* assumes registers are in
> order... */
> > > -                                      regs[first_reg]) {
> > > -                                       if (reg_num == regs[first_reg])
> > > -                                               offsets[first_reg] =
> current_offset - 1;
> > > +                               auto reg = offsets_.find(reg_num);
> > > +
> > > +                               if (reg != offsets_.end()) {
> > > +                                       offsets_[reg_num] =
> current_offset - 1;
> > >
> > > -                                       if (++first_reg == num_regs)
> > > +                                       if (++regs_done ==
> offsets_.size())
> > >                                                 return PARSE_OK;
> > >                                 }
> > >                                 reg_num++;
>
> --
> Regards,
>
> Laurent Pinchart
>

Hello again,

On Tue, Jun 29, 2021 at 05:44:14PM +0300, Laurent Pinchart wrote:
> On Tue, Jun 29, 2021 at 02:53:50PM +0100, Naushir Patuck wrote:
> > On Tue, 29 Jun 2021 at 11:45, Naushir Patuck <naush@raspberrypi.com> wrote:
> > 
> > > Instead of having each CamHelper subclass the MdParserSmia, change the
> > > implementation of MdParserSmia to be more generic. The MdParserSmia now gets
> > > given a list of registers to search for and helper functions are used to compute
> > > exposure lines and gain codes from these registers.
> > >
> > > Update the imx219 and imx477 CamHelpers by using this new mechanism.
> > >
> > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > ---
> > >  src/ipa/raspberrypi/cam_helper.cpp        |  33 +++---
> > >  src/ipa/raspberrypi/cam_helper.hpp        |   2 +
> > >  src/ipa/raspberrypi/cam_helper_imx219.cpp | 114 ++++----------------
> > >  src/ipa/raspberrypi/cam_helper_imx477.cpp | 122 ++++------------------
> > >  src/ipa/raspberrypi/md_parser.hpp         |  41 +++++---
> > >  src/ipa/raspberrypi/md_parser_smia.cpp    |  66 ++++++++++--
> > >  6 files changed, 144 insertions(+), 234 deletions(-)
> > >
> > > diff --git a/src/ipa/raspberrypi/cam_helper.cpp b/src/ipa/raspberrypi/cam_helper.cpp
> > > index 90498c37af98..e6d2258c66d7 100644
> > > --- a/src/ipa/raspberrypi/cam_helper.cpp
> > > +++ b/src/ipa/raspberrypi/cam_helper.cpp
> > > @@ -159,32 +159,34 @@ unsigned int CamHelper::MistrustFramesModeSwitch() const
> > >  void CamHelper::parseEmbeddedData(Span<const uint8_t> buffer,
> > >                                   Metadata &metadata)
> > >  {
> > > +       MdParser::RegisterMap registers;
> > > +       Metadata parsedMetadata;
> > > +
> > >         if (buffer.empty())
> > >                 return;
> > >
> > > -       uint32_t exposureLines, gainCode;
> > > -
> > > -       if (parser_->Parse(buffer) != MdParser::Status::OK ||
> > > -           parser_->GetExposureLines(exposureLines) != MdParser::Status::OK ||
> > > -           parser_->GetGainCode(gainCode) != MdParser::Status::OK) {
> > > +       if (parser_->Parse(buffer, registers) != MdParser::Status::OK) {
> > >                 LOG(IPARPI, Error) << "Embedded data buffer parsing failed";
> > >                 return;
> > >         }
> > >
> > > +       PopulateMetadata(registers, parsedMetadata);
> > > +       metadata.Merge(parsedMetadata);
> > > +
> > >         /*
> > > -        * Overwrite the exposure/gain values in the DeviceStatus, as
> > > -        * we know better. Fetch it first in case any other fields were
> > > -        * set meaningfully.
> > > +        * Overwrite the exposure/gain values in the existing DeviceStatus with
> > > +        * values from the parsed embedded buffer. Fetch it first in case any
> > > +        * other fields were set meaningfully.
> > >          */
> > > -       DeviceStatus deviceStatus;
> > > -
> > > -       if (metadata.Get("device.status", deviceStatus) != 0) {
> > > +       DeviceStatus deviceStatus, parsedDeviceStatus;
> > > +       if (metadata.Get("device.status", deviceStatus) ||
> > > +           parsedMetadata.Get("device.status", parsedDeviceStatus)) {
> > >                 LOG(IPARPI, Error) << "DeviceStatus not found";
> > >                 return;
> > >         }
> > >
> > > -       deviceStatus.shutter_speed = Exposure(exposureLines);
> > > -       deviceStatus.analogue_gain = Gain(gainCode);
> > > +       deviceStatus.shutter_speed = parsedDeviceStatus.shutter_speed;
> > > +       deviceStatus.analogue_gain = parsedDeviceStatus.analogue_gain;
> > >
> > >         LOG(IPARPI, Debug) << "Metadata updated - Exposure : "
> > >                            << deviceStatus.shutter_speed
> > > @@ -194,6 +196,11 @@ void CamHelper::parseEmbeddedData(Span<const uint8_t> buffer,
> > >         metadata.Set("device.status", deviceStatus);
> > >  }
> > >
> > > +void CamHelper::PopulateMetadata([[maybe_unused]] const MdParser::RegisterMap &registers,
> > > +                                [[maybe_unused]] Metadata &metadata) const
> > > +{
> > > +}
> > > +
> > >  RegisterCamHelper::RegisterCamHelper(char const *cam_name,
> > >                                      CamHelperCreateFunc create_func)
> > >  {
> > > diff --git a/src/ipa/raspberrypi/cam_helper.hpp b/src/ipa/raspberrypi/cam_helper.hpp
> > > index fc3139e22be0..200cc83f3872 100644
> > > --- a/src/ipa/raspberrypi/cam_helper.hpp
> > > +++ b/src/ipa/raspberrypi/cam_helper.hpp
> > > @@ -92,6 +92,8 @@ public:
> > >  protected:
> > >         void parseEmbeddedData(libcamera::Span<const uint8_t> buffer,
> > >                                Metadata &metadata);
> > > +       virtual void PopulateMetadata(const MdParser::RegisterMap &registers,
> > > +                                     Metadata &metadata) const;
> > >
> > >         std::unique_ptr<MdParser> parser_;
> > >         CameraMode mode_;
> > > diff --git a/src/ipa/raspberrypi/cam_helper_imx219.cpp b/src/ipa/raspberrypi/cam_helper_imx219.cpp
> > > index c85044a5fa6d..4e4994188ff3 100644
> > > --- a/src/ipa/raspberrypi/cam_helper_imx219.cpp
> > > +++ b/src/ipa/raspberrypi/cam_helper_imx219.cpp
> > > @@ -23,21 +23,14 @@
> > >
> > >  using namespace RPiController;
> > >
> > > -/* Metadata parser implementation specific to Sony IMX219 sensors. */
> > > -
> > > -class MdParserImx219 : public MdParserSmia
> > > -{
> > > -public:
> > > -       MdParserImx219();
> > > -       Status Parse(libcamera::Span<const uint8_t> buffer) override;
> > > -       Status GetExposureLines(unsigned int &lines) override;
> > > -       Status GetGainCode(unsigned int &gain_code) override;
> > > -private:
> > > -       /* Offset of the register's value in the metadata block. */
> > > -       int reg_offsets_[3];
> > > -       /* Value of the register, once read from the metadata block. */
> > > -       int reg_values_[3];
> > > -};
> > > +/*
> > > + * We care about one gain register and a pair of exposure registers. Their I2C
> > > + * addresses from the Sony IMX219 datasheet:
> > > + */
> > > +constexpr uint32_t gainReg = 0x157;
> > > +constexpr uint32_t expHiReg = 0x15a;
> > > +constexpr uint32_t expLoReg = 0x15b;
> > > +constexpr std::initializer_list<uint32_t> registerList = { expHiReg, expLoReg, gainReg };

This needs a [[maybe_unused]] in case ENABLE_EMBEDDED_DATA isn't
defined.

> > >
> > >  class CamHelperImx219 : public CamHelper
> > >  {
> > > @@ -54,11 +47,14 @@ private:
> > >          * in units of lines.
> > >          */
> > >         static constexpr int frameIntegrationDiff = 4;
> > > +
> > > +       void PopulateMetadata(const MdParser::RegisterMap &registers,
> > > +                             Metadata &metadata) const override;
> > >  };
> > >
> > >  CamHelperImx219::CamHelperImx219()
> > >  #if ENABLE_EMBEDDED_DATA
> > > -       : CamHelper(std::make_unique<MdParserImx219>(), frameIntegrationDiff)
> > > +       : CamHelper(std::make_unique<MdParserSmia>(registerList), frameIntegrationDiff)
> > >  #else
> > >         : CamHelper({}, frameIntegrationDiff)
> > >  #endif
> > > @@ -90,88 +86,20 @@ bool CamHelperImx219::SensorEmbeddedDataPresent() const
> > >         return ENABLE_EMBEDDED_DATA;
> > >  }
> > >
> > > -static CamHelper *Create()
> > > +void CamHelperImx219::PopulateMetadata(const MdParser::RegisterMap &registers,
> > > +                                      Metadata &metadata) const
> > >  {
> > > -       return new CamHelperImx219();
> > > -}
> > > +       DeviceStatus deviceStatus;
> > >
> > > -static RegisterCamHelper reg("imx219", &Create);
> > > +       deviceStatus.shutter_speed = Exposure(registers.at(expHiReg) * 256 + registers.at(expLoReg));
> > > +       deviceStatus.analogue_gain = Gain(registers.at(gainReg));
> > >
> > > -/*
> > > - * We care about one gain register and a pair of exposure registers. Their I2C
> > > - * addresses from the Sony IMX219 datasheet:
> > > - */
> > > -#define GAIN_REG 0x157
> > > -#define EXPHI_REG 0x15A
> > > -#define EXPLO_REG 0x15B
> > > -
> > > -/*
> > > - * Index of each into the reg_offsets and reg_values arrays. Must be in
> > > - * register address order.
> > > - */
> > > -#define GAIN_INDEX 0
> > > -#define EXPHI_INDEX 1
> > > -#define EXPLO_INDEX 2
> > > -
> > > -MdParserImx219::MdParserImx219()
> > > -{
> > > -       reg_offsets_[0] = reg_offsets_[1] = reg_offsets_[2] = -1;
> > > +       metadata.Set("device.status", deviceStatus);
> > >  }
> > >
> > > -MdParser::Status MdParserImx219::Parse(libcamera::Span<const uint8_t> buffer)
> > > -{
> > > -       bool try_again = false;
> > > -
> > > -       if (reset_) {
> > > -               /*
> > > -                * Search again through the metadata for the gain and exposure
> > > -                * registers.
> > > -                */
> > > -               assert(bits_per_pixel_);
> > > -               /* Need to be ordered */
> > > -               uint32_t regs[3] = { GAIN_REG, EXPHI_REG, EXPLO_REG };
> > > -               reg_offsets_[0] = reg_offsets_[1] = reg_offsets_[2] = -1;
> > > -               int ret = static_cast<int>(findRegs(buffer,
> > > -                                                   regs, reg_offsets_, 3));
> > > -               /*
> > > -                * > 0 means "worked partially but parse again next time",
> > > -                * < 0 means "hard error".
> > > -                */
> > > -               if (ret > 0)
> > > -                       try_again = true;
> > > -               else if (ret < 0)
> > > -                       return ERROR;
> > > -       }
> > > -
> > > -       for (int i = 0; i < 3; i++) {
> > > -               if (reg_offsets_[i] == -1)
> > > -                       continue;
> > > -
> > > -               reg_values_[i] = buffer[reg_offsets_[i]];
> > > -       }
> > > -
> > > -       /* Re-parse next time if we were unhappy in some way. */
> > > -       reset_ = try_again;
> > > -
> > > -       return OK;
> > > -}
> > > -
> > > -MdParser::Status MdParserImx219::GetExposureLines(unsigned int &lines)
> > > +static CamHelper *Create()
> > >  {
> > > -       if (reg_offsets_[EXPHI_INDEX] == -1 || reg_offsets_[EXPLO_INDEX] == -1)
> > > -               return NOTFOUND;
> > > -
> > > -       lines = reg_values_[EXPHI_INDEX] * 256 + reg_values_[EXPLO_INDEX];
> > > -
> > > -       return OK;
> > > +       return new CamHelperImx219();
> > >  }
> > >
> > > -MdParser::Status MdParserImx219::GetGainCode(unsigned int &gain_code)
> > > -{
> > > -       if (reg_offsets_[GAIN_INDEX] == -1)
> > > -               return NOTFOUND;
> > > -
> > > -       gain_code = reg_values_[GAIN_INDEX];
> > > -
> > > -       return OK;
> > > -}
> > > +static RegisterCamHelper reg("imx219", &Create);
> > > diff --git a/src/ipa/raspberrypi/cam_helper_imx477.cpp b/src/ipa/raspberrypi/cam_helper_imx477.cpp
> > > index d72a9be0438e..efd1a5893db8 100644
> > > --- a/src/ipa/raspberrypi/cam_helper_imx477.cpp
> > > +++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp
> > > @@ -15,21 +15,15 @@
> > >
> > >  using namespace RPiController;
> > >
> > > -/* Metadata parser implementation specific to Sony IMX477 sensors. */
> > > -
> > > -class MdParserImx477 : public MdParserSmia
> > > -{
> > > -public:
> > > -       MdParserImx477();
> > > -       Status Parse(libcamera::Span<const uint8_t> buffer) override;
> > > -       Status GetExposureLines(unsigned int &lines) override;
> > > -       Status GetGainCode(unsigned int &gain_code) override;
> > > -private:
> > > -       /* Offset of the register's value in the metadata block. */
> > > -       int reg_offsets_[4];
> > > -       /* Value of the register, once read from the metadata block. */
> > > -       int reg_values_[4];
> > > -};
> > > +/*
> > > + * We care about two gain registers and a pair of exposure registers. Their
> > > + * I2C addresses from the Sony IMX477 datasheet:
> > > + */
> > > +constexpr uint32_t expHiReg = 0x0202;
> > > +constexpr uint32_t expLoReg = 0x0203;
> > > +constexpr uint32_t gainHiReg = 0x0204;
> > > +constexpr uint32_t gainLoReg = 0x0205;
> > > +constexpr std::initializer_list<uint32_t> registerList = { expHiReg, expLoReg, gainHiReg, gainLoReg };

Same here.

> > >
> > >  class CamHelperImx477 : public CamHelper
> > >  {
> > > @@ -47,10 +41,13 @@ private:
> > >          * in units of lines.
> > >          */
> > >         static constexpr int frameIntegrationDiff = 22;
> > > +
> > > +       void PopulateMetadata(const MdParser::RegisterMap &registers,
> > > +                             Metadata &metadata) const override;
> > >  };
> > >
> > >  CamHelperImx477::CamHelperImx477()
> > > -       : CamHelper(std::make_unique<MdParserImx477>(), frameIntegrationDiff)
> > > +       : CamHelper(std::make_unique<MdParserSmia>(registerList), frameIntegrationDiff)
> > >  {
> > >  }
> > >
> > > @@ -77,95 +74,20 @@ bool CamHelperImx477::SensorEmbeddedDataPresent() const
> > >         return true;
> > >  }
> > >
> > > -static CamHelper *Create()
> > > +void CamHelperImx477::PopulateMetadata(const MdParser::RegisterMap &registers,
> > > +                                      Metadata &metadata) const
> > >  {
> > > -       return new CamHelperImx477();
> > > -}
> > > +       DeviceStatus deviceStatus;
> > >
> > > -static RegisterCamHelper reg("imx477", &Create);
> > > +       deviceStatus.shutter_speed = Exposure(registers.at(expHiReg) * 256 + registers.at(expLoReg));
> > > +       deviceStatus.analogue_gain = Gain(registers.at(gainHiReg) * 256 + registers.at(gainLoReg));
> > >
> > > -/*
> > > - * We care about two gain registers and a pair of exposure registers. Their
> > > - * I2C addresses from the Sony IMX477 datasheet:
> > > - */
> > > -#define EXPHI_REG 0x0202
> > > -#define EXPLO_REG 0x0203
> > > -#define GAINHI_REG 0x0204
> > > -#define GAINLO_REG 0x0205
> > > -
> > > -/*
> > > - * Index of each into the reg_offsets and reg_values arrays. Must be in register
> > > - * address order.
> > > - */
> > > -#define EXPHI_INDEX 0
> > > -#define EXPLO_INDEX 1
> > > -#define GAINHI_INDEX 2
> > > -#define GAINLO_INDEX 3
> > > -
> > > -MdParserImx477::MdParserImx477()
> > > -{
> > > -       reg_offsets_[0] = reg_offsets_[1] = reg_offsets_[2] = reg_offsets_[3] = -1;
> > > +       metadata.Set("device.status", deviceStatus);
> > >  }
> > >
> > > -MdParser::Status MdParserImx477::Parse(libcamera::Span<const uint8_t> buffer)
> > > -{
> > > -       bool try_again = false;
> > > -
> > > -       if (reset_) {
> > > -               /*
> > > -                * Search again through the metadata for the gain and exposure
> > > -                * registers.
> > > -                */
> > > -               assert(bits_per_pixel_);
> > > -               /* Need to be ordered */
> > > -               uint32_t regs[4] = {
> > > -                       EXPHI_REG,
> > > -                       EXPLO_REG,
> > > -                       GAINHI_REG,
> > > -                       GAINLO_REG
> > > -               };
> > > -               reg_offsets_[0] = reg_offsets_[1] = reg_offsets_[2] = reg_offsets_[3] = -1;
> > > -               int ret = static_cast<int>(findRegs(buffer,
> > > -                                                   regs, reg_offsets_, 4));
> > > -               /*
> > > -                * > 0 means "worked partially but parse again next time",
> > > -                * < 0 means "hard error".
> > > -                */
> > > -               if (ret > 0)
> > > -                       try_again = true;
> > > -               else if (ret < 0)
> > > -                       return ERROR;
> > > -       }
> > > -
> > > -       for (int i = 0; i < 4; i++) {
> > > -               if (reg_offsets_[i] == -1)
> > > -                       continue;
> > > -
> > > -               reg_values_[i] = buffer[reg_offsets_[i]];
> > > -       }
> > > -
> > > -       /* Re-parse next time if we were unhappy in some way. */
> > > -       reset_ = try_again;
> > > -
> > > -       return OK;
> > > -}
> > > -
> > > -MdParser::Status MdParserImx477::GetExposureLines(unsigned int &lines)
> > > +static CamHelper *Create()
> > >  {
> > > -       if (reg_offsets_[EXPHI_INDEX] == -1 || reg_offsets_[EXPLO_INDEX] == -1)
> > > -               return NOTFOUND;
> > > -
> > > -       lines = reg_values_[EXPHI_INDEX] * 256 + reg_values_[EXPLO_INDEX];
> > > -
> > > -       return OK;
> > > +       return new CamHelperImx477();
> > >  }
> > >
> > > -MdParser::Status MdParserImx477::GetGainCode(unsigned int &gain_code)
> > > -{
> > > -       if (reg_offsets_[GAINHI_INDEX] == -1 || reg_offsets_[GAINLO_INDEX] == -1)
> > > -               return NOTFOUND;
> > > -
> > > -       gain_code = reg_values_[GAINHI_INDEX] * 256 +
> > > reg_values_[GAINLO_INDEX];
> > > -
> > > -       return OK;
> > > -}
> > > +static RegisterCamHelper reg("imx477", &Create);
> > > diff --git a/src/ipa/raspberrypi/md_parser.hpp b/src/ipa/raspberrypi/md_parser.hpp
> > > index 8497216f8db7..e3e2738537a8 100644
> > > --- a/src/ipa/raspberrypi/md_parser.hpp
> > > +++ b/src/ipa/raspberrypi/md_parser.hpp
> > > @@ -6,6 +6,9 @@
> > >   */
> > >  #pragma once
> > >
> > > +#include <initializer_list>
> > > +#include <map>
> > > +#include <optional>
> > >  #include <stdint.h>
> > >
> > >  #include <libcamera/base/span.h>
> > > @@ -19,7 +22,7 @@
> > >   * application code doesn't have to worry which kind to instantiate. But for
> > >   * the sake of example let's suppose we're parsing imx219 metadata.
> > >   *
> > > - * MdParser *parser = new MdParserImx219();  // for example
> > > + * MdParser *parser = new MdParserSmia({ expHiReg, expLoReg, gainReg });
> > >   * parser->SetBitsPerPixel(bpp);
> > >   * parser->SetLineLengthBytes(pitch);
> > >   * parser->SetNumLines(2);
> > > @@ -32,13 +35,11 @@
> > >   *
> > >   * Then on every frame:
> > >   *
> > > - * if (parser->Parse(buffer) != MdParser::OK)
> > > + * RegisterMap registers;
> > > + * if (parser->Parse(buffer, registers) != MdParser::OK)
> > >   *     much badness;
> > > - * unsigned int exposure_lines, gain_code
> > > - * if (parser->GetExposureLines(exposure_lines) != MdParser::OK)
> > > - *     exposure was not found;
> > > - * if (parser->GetGainCode(parser, gain_code) != MdParser::OK)
> > > - *     gain code was not found;
> > > + * Metadata metadata;
> > > + * CamHelper::PopulateMetadata(registers, metadata);
> > >   *
> > >   * (Note that the CamHelper class converts to/from exposure lines and time,
> > >   * and gain_code / actual gain.)
> > > @@ -59,6 +60,8 @@ namespace RPiController {
> > >  class MdParser
> > >  {
> > >  public:
> > > +       using RegisterMap = std::map<uint32_t, uint32_t>;
> > > +
> > >         /*
> > >          * Parser status codes:
> > >          * OK       - success
> > > @@ -98,9 +101,8 @@ public:
> > >                 line_length_bytes_ = num_bytes;
> > >         }
> > >
> > > -       virtual Status Parse(libcamera::Span<const uint8_t> buffer) = 0;
> > > -       virtual Status GetExposureLines(unsigned int &lines) = 0;
> > > -       virtual Status GetGainCode(unsigned int &gain_code) = 0;
> > > +       virtual Status Parse(libcamera::Span<const uint8_t> buffer,
> > > +                            RegisterMap &registers) = 0;
> > >
> > >  protected:
> > >         bool reset_;
> > > @@ -116,14 +118,18 @@ protected:
> > >   * md_parser_imx219.cpp for an example).
> > >   */
> > >
> > > -class MdParserSmia : public MdParser
> > > +class MdParserSmia final : public MdParser
> > >  {
> > >  public:
> > > -       MdParserSmia() : MdParser()
> > > -       {
> > > -       }
> > > +       MdParserSmia(std::initializer_list<uint32_t> registerList);
> > > +
> > > +       MdParser::Status Parse(libcamera::Span<const uint8_t> buffer,
> > > +                              RegisterMap &registers) override;
> > > +
> > > +private:
> > > +       /* Maps register address to offset in the buffer. */
> > > +       using OffsetMap = std::map<uint32_t, std::optional<uint32_t>>;
> > >
> > > -protected:
> > >         /*
> > >          * Note that error codes > 0 are regarded as non-fatal; codes < 0
> > >          * indicate a bad data buffer. Status codes are:
> > > @@ -141,8 +147,9 @@ protected:
> > >                 BAD_PADDING   = -5
> > >         };
> > >
> > > -       ParseStatus findRegs(libcamera::Span<const uint8_t> buffer, uint32_t regs[],
> > > -                            int offsets[], unsigned int num_regs);
> > > +       ParseStatus findRegs(libcamera::Span<const uint8_t> buffer);
> > > +
> > > +       OffsetMap offsets_;
> > >  };
> > >
> > >  } // namespace RPi
> > > diff --git a/src/ipa/raspberrypi/md_parser_smia.cpp b/src/ipa/raspberrypi/md_parser_smia.cpp
> > > index 0a14875575a2..7f72fc03948f 100644
> > > --- a/src/ipa/raspberrypi/md_parser_smia.cpp
> > > +++ b/src/ipa/raspberrypi/md_parser_smia.cpp
> > > @@ -6,9 +6,11 @@
> > >   */
> > >  #include <assert.h>
> > >
> > 
> > Oops, this header is not needed any more!
> > 
> > > +#include "libcamera/base/log.h"
> 
> And this should be <libcamera/base/log.h>. I'll fix when applying.
> 
> > >  #include "md_parser.hpp"
> > >
> > >  using namespace RPiController;
> > > +using namespace libcamera;
> > >
> > >  /*
> > >   * This function goes through the embedded data to find the offsets (not
> > > @@ -26,18 +28,61 @@ constexpr unsigned int REG_LOW_BITS = 0xa5;
> > >  constexpr unsigned int REG_VALUE = 0x5a;
> > >  constexpr unsigned int REG_SKIP = 0x55;
> > >
> > > -MdParserSmia::ParseStatus MdParserSmia::findRegs(libcamera::Span<const uint8_t> buffer,
> > > -                                                uint32_t regs[], int offsets[],
> > > -                                                unsigned int num_regs)
> > > +MdParserSmia::MdParserSmia(std::initializer_list<uint32_t> registerList)
> > >  {
> > > -       assert(num_regs > 0);
> > > +       for (auto r : registerList)
> > > +               offsets_[r] = {};
> > > +}
> > > +
> > > +MdParser::Status MdParserSmia::Parse(libcamera::Span<const uint8_t> buffer,
> > > +                                    RegisterMap &registers)
> > > +{
> > > +       if (reset_) {
> > > +               /*
> > > +                * Search again through the metadata for all the registers
> > > +                * requested.
> > > +                */
> > > +               ASSERT(bits_per_pixel_);
> > > +
> > > +               for (const auto &[reg, offset] : offsets_)
> > > +                       offsets_[reg] = {};

I'm afraid this causes compilation failures with gcc 7 and gcc 8, due to
offset being unused. We can't throw a [[maybe_unused]] here, that's not
supported, so we need to revert to the syntax from v2.

		for (const auto &kv : offsets_)
			offsets_[kv.first] = {};

I'll post a v3.1, Naush, could you give it a try ?

> > > +
> > > +               ParseStatus ret = findRegs(buffer);
> > > +               /*
> > > +                * > 0 means "worked partially but parse again next time",
> > > +                * < 0 means "hard error".
> > > +                *
> > > +                * In either case, we retry parsing on the next frame.
> > > +                */
> > > +               if (ret != PARSE_OK)
> > > +                       return ERROR;
> > > +
> > > +               reset_ = false;
> > > +       }
> > > +
> > > +       /* Populate the register values requested. */
> > > +       registers.clear();
> > > +       for (const auto &[reg, offset] : offsets_) {
> > > +               if (!offset) {
> > > +                       reset_ = true;
> > > +                       return NOTFOUND;
> > > +               }
> > > +               registers[reg] = buffer[offset.value()];
> > > +       }
> > > +
> > > +       return OK;
> > > +}
> > > +
> > > +MdParserSmia::ParseStatus MdParserSmia::findRegs(libcamera::Span<const uint8_t> buffer)
> > > +{
> > > +       ASSERT(offsets_.size());
> > >
> > >         if (buffer[0] != LINE_START)
> > >                 return NO_LINE_START;
> > >
> > >         unsigned int current_offset = 1; /* after the LINE_START */
> > >         unsigned int current_line_start = 0, current_line = 0;
> > > -       unsigned int reg_num = 0, first_reg = 0;
> > > +       unsigned int reg_num = 0, regs_done = 0;
> > >
> > >         while (1) {
> > >                 int tag = buffer[current_offset++];
> > > @@ -89,13 +134,12 @@ MdParserSmia::ParseStatus MdParserSmia::findRegs(libcamera::Span<const uint8_t>
> > >                         else if (tag == REG_SKIP)
> > >                                 reg_num++;
> > >                         else if (tag == REG_VALUE) {
> > > -                               while (reg_num >=
> > > -                                      /* assumes registers are in order... */
> > > -                                      regs[first_reg]) {
> > > -                                       if (reg_num == regs[first_reg])
> > > -                                               offsets[first_reg] = current_offset - 1;
> > > +                               auto reg = offsets_.find(reg_num);
> > > +
> > > +                               if (reg != offsets_.end()) {
> > > +                                       offsets_[reg_num] = current_offset - 1;
> > >
> > > -                                       if (++first_reg == num_regs)
> > > +                                       if (++regs_done == offsets_.size())
> > >                                                 return PARSE_OK;
> > >                                 }
> > >                                 reg_num++;

Hi Laurent,

On Tue, 29 Jun 2021 at 16:47, Laurent Pinchart <
laurent.pinchart@ideasonboard.com> wrote:

> Hello again,
>
> On Tue, Jun 29, 2021 at 05:44:14PM +0300, Laurent Pinchart wrote:
> > On Tue, Jun 29, 2021 at 02:53:50PM +0100, Naushir Patuck wrote:
> > > On Tue, 29 Jun 2021 at 11:45, Naushir Patuck <naush@raspberrypi.com>
> wrote:
> > >
> > > > Instead of having each CamHelper subclass the MdParserSmia, change
> the
> > > > implementation of MdParserSmia to be more generic. The MdParserSmia
> now gets
> > > > given a list of registers to search for and helper functions are
> used to compute
> > > > exposure lines and gain codes from these registers.
> > > >
> > > > Update the imx219 and imx477 CamHelpers by using this new mechanism.
> > > >
> > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > ---
> > > >  src/ipa/raspberrypi/cam_helper.cpp        |  33 +++---
> > > >  src/ipa/raspberrypi/cam_helper.hpp        |   2 +
> > > >  src/ipa/raspberrypi/cam_helper_imx219.cpp | 114 ++++----------------
> > > >  src/ipa/raspberrypi/cam_helper_imx477.cpp | 122
> ++++------------------
> > > >  src/ipa/raspberrypi/md_parser.hpp         |  41 +++++---
> > > >  src/ipa/raspberrypi/md_parser_smia.cpp    |  66 ++++++++++--
> > > >  6 files changed, 144 insertions(+), 234 deletions(-)
> > > >
> > > > diff --git a/src/ipa/raspberrypi/cam_helper.cpp
> b/src/ipa/raspberrypi/cam_helper.cpp
> > > > index 90498c37af98..e6d2258c66d7 100644
> > > > --- a/src/ipa/raspberrypi/cam_helper.cpp
> > > > +++ b/src/ipa/raspberrypi/cam_helper.cpp
> > > > @@ -159,32 +159,34 @@ unsigned int
> CamHelper::MistrustFramesModeSwitch() const
> > > >  void CamHelper::parseEmbeddedData(Span<const uint8_t> buffer,
> > > >                                   Metadata &metadata)
> > > >  {
> > > > +       MdParser::RegisterMap registers;
> > > > +       Metadata parsedMetadata;
> > > > +
> > > >         if (buffer.empty())
> > > >                 return;
> > > >
> > > > -       uint32_t exposureLines, gainCode;
> > > > -
> > > > -       if (parser_->Parse(buffer) != MdParser::Status::OK ||
> > > > -           parser_->GetExposureLines(exposureLines) !=
> MdParser::Status::OK ||
> > > > -           parser_->GetGainCode(gainCode) != MdParser::Status::OK) {
> > > > +       if (parser_->Parse(buffer, registers) !=
> MdParser::Status::OK) {
> > > >                 LOG(IPARPI, Error) << "Embedded data buffer parsing
> failed";
> > > >                 return;
> > > >         }
> > > >
> > > > +       PopulateMetadata(registers, parsedMetadata);
> > > > +       metadata.Merge(parsedMetadata);
> > > > +
> > > >         /*
> > > > -        * Overwrite the exposure/gain values in the DeviceStatus, as
> > > > -        * we know better. Fetch it first in case any other fields
> were
> > > > -        * set meaningfully.
> > > > +        * Overwrite the exposure/gain values in the existing
> DeviceStatus with
> > > > +        * values from the parsed embedded buffer. Fetch it first in
> case any
> > > > +        * other fields were set meaningfully.
> > > >          */
> > > > -       DeviceStatus deviceStatus;
> > > > -
> > > > -       if (metadata.Get("device.status", deviceStatus) != 0) {
> > > > +       DeviceStatus deviceStatus, parsedDeviceStatus;
> > > > +       if (metadata.Get("device.status", deviceStatus) ||
> > > > +           parsedMetadata.Get("device.status", parsedDeviceStatus))
> {
> > > >                 LOG(IPARPI, Error) << "DeviceStatus not found";
> > > >                 return;
> > > >         }
> > > >
> > > > -       deviceStatus.shutter_speed = Exposure(exposureLines);
> > > > -       deviceStatus.analogue_gain = Gain(gainCode);
> > > > +       deviceStatus.shutter_speed =
> parsedDeviceStatus.shutter_speed;
> > > > +       deviceStatus.analogue_gain =
> parsedDeviceStatus.analogue_gain;
> > > >
> > > >         LOG(IPARPI, Debug) << "Metadata updated - Exposure : "
> > > >                            << deviceStatus.shutter_speed
> > > > @@ -194,6 +196,11 @@ void CamHelper::parseEmbeddedData(Span<const
> uint8_t> buffer,
> > > >         metadata.Set("device.status", deviceStatus);
> > > >  }
> > > >
> > > > +void CamHelper::PopulateMetadata([[maybe_unused]] const
> MdParser::RegisterMap &registers,
> > > > +                                [[maybe_unused]] Metadata
> &metadata) const
> > > > +{
> > > > +}
> > > > +
> > > >  RegisterCamHelper::RegisterCamHelper(char const *cam_name,
> > > >                                      CamHelperCreateFunc create_func)
> > > >  {
> > > > diff --git a/src/ipa/raspberrypi/cam_helper.hpp
> b/src/ipa/raspberrypi/cam_helper.hpp
> > > > index fc3139e22be0..200cc83f3872 100644
> > > > --- a/src/ipa/raspberrypi/cam_helper.hpp
> > > > +++ b/src/ipa/raspberrypi/cam_helper.hpp
> > > > @@ -92,6 +92,8 @@ public:
> > > >  protected:
> > > >         void parseEmbeddedData(libcamera::Span<const uint8_t> buffer,
> > > >                                Metadata &metadata);
> > > > +       virtual void PopulateMetadata(const MdParser::RegisterMap
> &registers,
> > > > +                                     Metadata &metadata) const;
> > > >
> > > >         std::unique_ptr<MdParser> parser_;
> > > >         CameraMode mode_;
> > > > diff --git a/src/ipa/raspberrypi/cam_helper_imx219.cpp
> b/src/ipa/raspberrypi/cam_helper_imx219.cpp
> > > > index c85044a5fa6d..4e4994188ff3 100644
> > > > --- a/src/ipa/raspberrypi/cam_helper_imx219.cpp
> > > > +++ b/src/ipa/raspberrypi/cam_helper_imx219.cpp
> > > > @@ -23,21 +23,14 @@
> > > >
> > > >  using namespace RPiController;
> > > >
> > > > -/* Metadata parser implementation specific to Sony IMX219 sensors.
> */
> > > > -
> > > > -class MdParserImx219 : public MdParserSmia
> > > > -{
> > > > -public:
> > > > -       MdParserImx219();
> > > > -       Status Parse(libcamera::Span<const uint8_t> buffer) override;
> > > > -       Status GetExposureLines(unsigned int &lines) override;
> > > > -       Status GetGainCode(unsigned int &gain_code) override;
> > > > -private:
> > > > -       /* Offset of the register's value in the metadata block. */
> > > > -       int reg_offsets_[3];
> > > > -       /* Value of the register, once read from the metadata block.
> */
> > > > -       int reg_values_[3];
> > > > -};
> > > > +/*
> > > > + * We care about one gain register and a pair of exposure
> registers. Their I2C
> > > > + * addresses from the Sony IMX219 datasheet:
> > > > + */
> > > > +constexpr uint32_t gainReg = 0x157;
> > > > +constexpr uint32_t expHiReg = 0x15a;
> > > > +constexpr uint32_t expLoReg = 0x15b;
> > > > +constexpr std::initializer_list<uint32_t> registerList = {
> expHiReg, expLoReg, gainReg };
>
> This needs a [[maybe_unused]] in case ENABLE_EMBEDDED_DATA isn't
> defined.
>
> > > >
> > > >  class CamHelperImx219 : public CamHelper
> > > >  {
> > > > @@ -54,11 +47,14 @@ private:
> > > >          * in units of lines.
> > > >          */
> > > >         static constexpr int frameIntegrationDiff = 4;
> > > > +
> > > > +       void PopulateMetadata(const MdParser::RegisterMap &registers,
> > > > +                             Metadata &metadata) const override;
> > > >  };
> > > >
> > > >  CamHelperImx219::CamHelperImx219()
> > > >  #if ENABLE_EMBEDDED_DATA
> > > > -       : CamHelper(std::make_unique<MdParserImx219>(),
> frameIntegrationDiff)
> > > > +       : CamHelper(std::make_unique<MdParserSmia>(registerList),
> frameIntegrationDiff)
> > > >  #else
> > > >         : CamHelper({}, frameIntegrationDiff)
> > > >  #endif
> > > > @@ -90,88 +86,20 @@ bool
> CamHelperImx219::SensorEmbeddedDataPresent() const
> > > >         return ENABLE_EMBEDDED_DATA;
> > > >  }
> > > >
> > > > -static CamHelper *Create()
> > > > +void CamHelperImx219::PopulateMetadata(const MdParser::RegisterMap
> &registers,
> > > > +                                      Metadata &metadata) const
> > > >  {
> > > > -       return new CamHelperImx219();
> > > > -}
> > > > +       DeviceStatus deviceStatus;
> > > >
> > > > -static RegisterCamHelper reg("imx219", &Create);
> > > > +       deviceStatus.shutter_speed = Exposure(registers.at(expHiReg)
> * 256 + registers.at(expLoReg));
> > > > +       deviceStatus.analogue_gain = Gain(registers.at(gainReg));
> > > >
> > > > -/*
> > > > - * We care about one gain register and a pair of exposure
> registers. Their I2C
> > > > - * addresses from the Sony IMX219 datasheet:
> > > > - */
> > > > -#define GAIN_REG 0x157
> > > > -#define EXPHI_REG 0x15A
> > > > -#define EXPLO_REG 0x15B
> > > > -
> > > > -/*
> > > > - * Index of each into the reg_offsets and reg_values arrays. Must
> be in
> > > > - * register address order.
> > > > - */
> > > > -#define GAIN_INDEX 0
> > > > -#define EXPHI_INDEX 1
> > > > -#define EXPLO_INDEX 2
> > > > -
> > > > -MdParserImx219::MdParserImx219()
> > > > -{
> > > > -       reg_offsets_[0] = reg_offsets_[1] = reg_offsets_[2] = -1;
> > > > +       metadata.Set("device.status", deviceStatus);
> > > >  }
> > > >
> > > > -MdParser::Status MdParserImx219::Parse(libcamera::Span<const
> uint8_t> buffer)
> > > > -{
> > > > -       bool try_again = false;
> > > > -
> > > > -       if (reset_) {
> > > > -               /*
> > > > -                * Search again through the metadata for the gain
> and exposure
> > > > -                * registers.
> > > > -                */
> > > > -               assert(bits_per_pixel_);
> > > > -               /* Need to be ordered */
> > > > -               uint32_t regs[3] = { GAIN_REG, EXPHI_REG, EXPLO_REG
> };
> > > > -               reg_offsets_[0] = reg_offsets_[1] = reg_offsets_[2]
> = -1;
> > > > -               int ret = static_cast<int>(findRegs(buffer,
> > > > -                                                   regs,
> reg_offsets_, 3));
> > > > -               /*
> > > > -                * > 0 means "worked partially but parse again next
> time",
> > > > -                * < 0 means "hard error".
> > > > -                */
> > > > -               if (ret > 0)
> > > > -                       try_again = true;
> > > > -               else if (ret < 0)
> > > > -                       return ERROR;
> > > > -       }
> > > > -
> > > > -       for (int i = 0; i < 3; i++) {
> > > > -               if (reg_offsets_[i] == -1)
> > > > -                       continue;
> > > > -
> > > > -               reg_values_[i] = buffer[reg_offsets_[i]];
> > > > -       }
> > > > -
> > > > -       /* Re-parse next time if we were unhappy in some way. */
> > > > -       reset_ = try_again;
> > > > -
> > > > -       return OK;
> > > > -}
> > > > -
> > > > -MdParser::Status MdParserImx219::GetExposureLines(unsigned int
> &lines)
> > > > +static CamHelper *Create()
> > > >  {
> > > > -       if (reg_offsets_[EXPHI_INDEX] == -1 ||
> reg_offsets_[EXPLO_INDEX] == -1)
> > > > -               return NOTFOUND;
> > > > -
> > > > -       lines = reg_values_[EXPHI_INDEX] * 256 +
> reg_values_[EXPLO_INDEX];
> > > > -
> > > > -       return OK;
> > > > +       return new CamHelperImx219();
> > > >  }
> > > >
> > > > -MdParser::Status MdParserImx219::GetGainCode(unsigned int
> &gain_code)
> > > > -{
> > > > -       if (reg_offsets_[GAIN_INDEX] == -1)
> > > > -               return NOTFOUND;
> > > > -
> > > > -       gain_code = reg_values_[GAIN_INDEX];
> > > > -
> > > > -       return OK;
> > > > -}
> > > > +static RegisterCamHelper reg("imx219", &Create);
> > > > diff --git a/src/ipa/raspberrypi/cam_helper_imx477.cpp
> b/src/ipa/raspberrypi/cam_helper_imx477.cpp
> > > > index d72a9be0438e..efd1a5893db8 100644
> > > > --- a/src/ipa/raspberrypi/cam_helper_imx477.cpp
> > > > +++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp
> > > > @@ -15,21 +15,15 @@
> > > >
> > > >  using namespace RPiController;
> > > >
> > > > -/* Metadata parser implementation specific to Sony IMX477 sensors.
> */
> > > > -
> > > > -class MdParserImx477 : public MdParserSmia
> > > > -{
> > > > -public:
> > > > -       MdParserImx477();
> > > > -       Status Parse(libcamera::Span<const uint8_t> buffer) override;
> > > > -       Status GetExposureLines(unsigned int &lines) override;
> > > > -       Status GetGainCode(unsigned int &gain_code) override;
> > > > -private:
> > > > -       /* Offset of the register's value in the metadata block. */
> > > > -       int reg_offsets_[4];
> > > > -       /* Value of the register, once read from the metadata block.
> */
> > > > -       int reg_values_[4];
> > > > -};
> > > > +/*
> > > > + * We care about two gain registers and a pair of exposure
> registers. Their
> > > > + * I2C addresses from the Sony IMX477 datasheet:
> > > > + */
> > > > +constexpr uint32_t expHiReg = 0x0202;
> > > > +constexpr uint32_t expLoReg = 0x0203;
> > > > +constexpr uint32_t gainHiReg = 0x0204;
> > > > +constexpr uint32_t gainLoReg = 0x0205;
> > > > +constexpr std::initializer_list<uint32_t> registerList = {
> expHiReg, expLoReg, gainHiReg, gainLoReg };
>
> Same here.
>
> > > >
> > > >  class CamHelperImx477 : public CamHelper
> > > >  {
> > > > @@ -47,10 +41,13 @@ private:
> > > >          * in units of lines.
> > > >          */
> > > >         static constexpr int frameIntegrationDiff = 22;
> > > > +
> > > > +       void PopulateMetadata(const MdParser::RegisterMap &registers,
> > > > +                             Metadata &metadata) const override;
> > > >  };
> > > >
> > > >  CamHelperImx477::CamHelperImx477()
> > > > -       : CamHelper(std::make_unique<MdParserImx477>(),
> frameIntegrationDiff)
> > > > +       : CamHelper(std::make_unique<MdParserSmia>(registerList),
> frameIntegrationDiff)
> > > >  {
> > > >  }
> > > >
> > > > @@ -77,95 +74,20 @@ bool
> CamHelperImx477::SensorEmbeddedDataPresent() const
> > > >         return true;
> > > >  }
> > > >
> > > > -static CamHelper *Create()
> > > > +void CamHelperImx477::PopulateMetadata(const MdParser::RegisterMap
> &registers,
> > > > +                                      Metadata &metadata) const
> > > >  {
> > > > -       return new CamHelperImx477();
> > > > -}
> > > > +       DeviceStatus deviceStatus;
> > > >
> > > > -static RegisterCamHelper reg("imx477", &Create);
> > > > +       deviceStatus.shutter_speed = Exposure(registers.at(expHiReg)
> * 256 + registers.at(expLoReg));
> > > > +       deviceStatus.analogue_gain = Gain(registers.at(gainHiReg) *
> 256 + registers.at(gainLoReg));
> > > >
> > > > -/*
> > > > - * We care about two gain registers and a pair of exposure
> registers. Their
> > > > - * I2C addresses from the Sony IMX477 datasheet:
> > > > - */
> > > > -#define EXPHI_REG 0x0202
> > > > -#define EXPLO_REG 0x0203
> > > > -#define GAINHI_REG 0x0204
> > > > -#define GAINLO_REG 0x0205
> > > > -
> > > > -/*
> > > > - * Index of each into the reg_offsets and reg_values arrays. Must
> be in register
> > > > - * address order.
> > > > - */
> > > > -#define EXPHI_INDEX 0
> > > > -#define EXPLO_INDEX 1
> > > > -#define GAINHI_INDEX 2
> > > > -#define GAINLO_INDEX 3
> > > > -
> > > > -MdParserImx477::MdParserImx477()
> > > > -{
> > > > -       reg_offsets_[0] = reg_offsets_[1] = reg_offsets_[2] =
> reg_offsets_[3] = -1;
> > > > +       metadata.Set("device.status", deviceStatus);
> > > >  }
> > > >
> > > > -MdParser::Status MdParserImx477::Parse(libcamera::Span<const
> uint8_t> buffer)
> > > > -{
> > > > -       bool try_again = false;
> > > > -
> > > > -       if (reset_) {
> > > > -               /*
> > > > -                * Search again through the metadata for the gain
> and exposure
> > > > -                * registers.
> > > > -                */
> > > > -               assert(bits_per_pixel_);
> > > > -               /* Need to be ordered */
> > > > -               uint32_t regs[4] = {
> > > > -                       EXPHI_REG,
> > > > -                       EXPLO_REG,
> > > > -                       GAINHI_REG,
> > > > -                       GAINLO_REG
> > > > -               };
> > > > -               reg_offsets_[0] = reg_offsets_[1] = reg_offsets_[2]
> = reg_offsets_[3] = -1;
> > > > -               int ret = static_cast<int>(findRegs(buffer,
> > > > -                                                   regs,
> reg_offsets_, 4));
> > > > -               /*
> > > > -                * > 0 means "worked partially but parse again next
> time",
> > > > -                * < 0 means "hard error".
> > > > -                */
> > > > -               if (ret > 0)
> > > > -                       try_again = true;
> > > > -               else if (ret < 0)
> > > > -                       return ERROR;
> > > > -       }
> > > > -
> > > > -       for (int i = 0; i < 4; i++) {
> > > > -               if (reg_offsets_[i] == -1)
> > > > -                       continue;
> > > > -
> > > > -               reg_values_[i] = buffer[reg_offsets_[i]];
> > > > -       }
> > > > -
> > > > -       /* Re-parse next time if we were unhappy in some way. */
> > > > -       reset_ = try_again;
> > > > -
> > > > -       return OK;
> > > > -}
> > > > -
> > > > -MdParser::Status MdParserImx477::GetExposureLines(unsigned int
> &lines)
> > > > +static CamHelper *Create()
> > > >  {
> > > > -       if (reg_offsets_[EXPHI_INDEX] == -1 ||
> reg_offsets_[EXPLO_INDEX] == -1)
> > > > -               return NOTFOUND;
> > > > -
> > > > -       lines = reg_values_[EXPHI_INDEX] * 256 +
> reg_values_[EXPLO_INDEX];
> > > > -
> > > > -       return OK;
> > > > +       return new CamHelperImx477();
> > > >  }
> > > >
> > > > -MdParser::Status MdParserImx477::GetGainCode(unsigned int
> &gain_code)
> > > > -{
> > > > -       if (reg_offsets_[GAINHI_INDEX] == -1 ||
> reg_offsets_[GAINLO_INDEX] == -1)
> > > > -               return NOTFOUND;
> > > > -
> > > > -       gain_code = reg_values_[GAINHI_INDEX] * 256 +
> > > > reg_values_[GAINLO_INDEX];
> > > > -
> > > > -       return OK;
> > > > -}
> > > > +static RegisterCamHelper reg("imx477", &Create);
> > > > diff --git a/src/ipa/raspberrypi/md_parser.hpp
> b/src/ipa/raspberrypi/md_parser.hpp
> > > > index 8497216f8db7..e3e2738537a8 100644
> > > > --- a/src/ipa/raspberrypi/md_parser.hpp
> > > > +++ b/src/ipa/raspberrypi/md_parser.hpp
> > > > @@ -6,6 +6,9 @@
> > > >   */
> > > >  #pragma once
> > > >
> > > > +#include <initializer_list>
> > > > +#include <map>
> > > > +#include <optional>
> > > >  #include <stdint.h>
> > > >
> > > >  #include <libcamera/base/span.h>
> > > > @@ -19,7 +22,7 @@
> > > >   * application code doesn't have to worry which kind to
> instantiate. But for
> > > >   * the sake of example let's suppose we're parsing imx219 metadata.
> > > >   *
> > > > - * MdParser *parser = new MdParserImx219();  // for example
> > > > + * MdParser *parser = new MdParserSmia({ expHiReg, expLoReg,
> gainReg });
> > > >   * parser->SetBitsPerPixel(bpp);
> > > >   * parser->SetLineLengthBytes(pitch);
> > > >   * parser->SetNumLines(2);
> > > > @@ -32,13 +35,11 @@
> > > >   *
> > > >   * Then on every frame:
> > > >   *
> > > > - * if (parser->Parse(buffer) != MdParser::OK)
> > > > + * RegisterMap registers;
> > > > + * if (parser->Parse(buffer, registers) != MdParser::OK)
> > > >   *     much badness;
> > > > - * unsigned int exposure_lines, gain_code
> > > > - * if (parser->GetExposureLines(exposure_lines) != MdParser::OK)
> > > > - *     exposure was not found;
> > > > - * if (parser->GetGainCode(parser, gain_code) != MdParser::OK)
> > > > - *     gain code was not found;
> > > > + * Metadata metadata;
> > > > + * CamHelper::PopulateMetadata(registers, metadata);
> > > >   *
> > > >   * (Note that the CamHelper class converts to/from exposure lines
> and time,
> > > >   * and gain_code / actual gain.)
> > > > @@ -59,6 +60,8 @@ namespace RPiController {
> > > >  class MdParser
> > > >  {
> > > >  public:
> > > > +       using RegisterMap = std::map<uint32_t, uint32_t>;
> > > > +
> > > >         /*
> > > >          * Parser status codes:
> > > >          * OK       - success
> > > > @@ -98,9 +101,8 @@ public:
> > > >                 line_length_bytes_ = num_bytes;
> > > >         }
> > > >
> > > > -       virtual Status Parse(libcamera::Span<const uint8_t> buffer)
> = 0;
> > > > -       virtual Status GetExposureLines(unsigned int &lines) = 0;
> > > > -       virtual Status GetGainCode(unsigned int &gain_code) = 0;
> > > > +       virtual Status Parse(libcamera::Span<const uint8_t> buffer,
> > > > +                            RegisterMap &registers) = 0;
> > > >
> > > >  protected:
> > > >         bool reset_;
> > > > @@ -116,14 +118,18 @@ protected:
> > > >   * md_parser_imx219.cpp for an example).
> > > >   */
> > > >
> > > > -class MdParserSmia : public MdParser
> > > > +class MdParserSmia final : public MdParser
> > > >  {
> > > >  public:
> > > > -       MdParserSmia() : MdParser()
> > > > -       {
> > > > -       }
> > > > +       MdParserSmia(std::initializer_list<uint32_t> registerList);
> > > > +
> > > > +       MdParser::Status Parse(libcamera::Span<const uint8_t> buffer,
> > > > +                              RegisterMap &registers) override;
> > > > +
> > > > +private:
> > > > +       /* Maps register address to offset in the buffer. */
> > > > +       using OffsetMap = std::map<uint32_t,
> std::optional<uint32_t>>;
> > > >
> > > > -protected:
> > > >         /*
> > > >          * Note that error codes > 0 are regarded as non-fatal;
> codes < 0
> > > >          * indicate a bad data buffer. Status codes are:
> > > > @@ -141,8 +147,9 @@ protected:
> > > >                 BAD_PADDING   = -5
> > > >         };
> > > >
> > > > -       ParseStatus findRegs(libcamera::Span<const uint8_t> buffer,
> uint32_t regs[],
> > > > -                            int offsets[], unsigned int num_regs);
> > > > +       ParseStatus findRegs(libcamera::Span<const uint8_t> buffer);
> > > > +
> > > > +       OffsetMap offsets_;
> > > >  };
> > > >
> > > >  } // namespace RPi
> > > > diff --git a/src/ipa/raspberrypi/md_parser_smia.cpp
> b/src/ipa/raspberrypi/md_parser_smia.cpp
> > > > index 0a14875575a2..7f72fc03948f 100644
> > > > --- a/src/ipa/raspberrypi/md_parser_smia.cpp
> > > > +++ b/src/ipa/raspberrypi/md_parser_smia.cpp
> > > > @@ -6,9 +6,11 @@
> > > >   */
> > > >  #include <assert.h>
> > > >
> > >
> > > Oops, this header is not needed any more!
> > >
> > > > +#include "libcamera/base/log.h"
> >
> > And this should be <libcamera/base/log.h>. I'll fix when applying.
> >
> > > >  #include "md_parser.hpp"
> > > >
> > > >  using namespace RPiController;
> > > > +using namespace libcamera;
> > > >
> > > >  /*
> > > >   * This function goes through the embedded data to find the offsets
> (not
> > > > @@ -26,18 +28,61 @@ constexpr unsigned int REG_LOW_BITS = 0xa5;
> > > >  constexpr unsigned int REG_VALUE = 0x5a;
> > > >  constexpr unsigned int REG_SKIP = 0x55;
> > > >
> > > > -MdParserSmia::ParseStatus
> MdParserSmia::findRegs(libcamera::Span<const uint8_t> buffer,
> > > > -                                                uint32_t regs[],
> int offsets[],
> > > > -                                                unsigned int
> num_regs)
> > > > +MdParserSmia::MdParserSmia(std::initializer_list<uint32_t>
> registerList)
> > > >  {
> > > > -       assert(num_regs > 0);
> > > > +       for (auto r : registerList)
> > > > +               offsets_[r] = {};
> > > > +}
> > > > +
> > > > +MdParser::Status MdParserSmia::Parse(libcamera::Span<const uint8_t>
> buffer,
> > > > +                                    RegisterMap &registers)
> > > > +{
> > > > +       if (reset_) {
> > > > +               /*
> > > > +                * Search again through the metadata for all the
> registers
> > > > +                * requested.
> > > > +                */
> > > > +               ASSERT(bits_per_pixel_);
> > > > +
> > > > +               for (const auto &[reg, offset] : offsets_)
> > > > +                       offsets_[reg] = {};
>
> I'm afraid this causes compilation failures with gcc 7 and gcc 8, due to
> offset being unused. We can't throw a [[maybe_unused]] here, that's not
> supported, so we need to revert to the syntax from v2.
>
>                 for (const auto &kv : offsets_)
>                         offsets_[kv.first] = {};
>
> I'll post a v3.1, Naush, could you give it a try ?
>

Thank you for the fixups!  I'm a bit curious why my compiler did not throw
out warnings
for these though...?

I'll test and report back first thing tomorrow.

Regards,
Naush


> > > > +
> > > > +               ParseStatus ret = findRegs(buffer);
> > > > +               /*
> > > > +                * > 0 means "worked partially but parse again next
> time",
> > > > +                * < 0 means "hard error".
> > > > +                *
> > > > +                * In either case, we retry parsing on the next
> frame.
> > > > +                */
> > > > +               if (ret != PARSE_OK)
> > > > +                       return ERROR;
> > > > +
> > > > +               reset_ = false;
> > > > +       }
> > > > +
> > > > +       /* Populate the register values requested. */
> > > > +       registers.clear();
> > > > +       for (const auto &[reg, offset] : offsets_) {
> > > > +               if (!offset) {
> > > > +                       reset_ = true;
> > > > +                       return NOTFOUND;
> > > > +               }
> > > > +               registers[reg] = buffer[offset.value()];
> > > > +       }
> > > > +
> > > > +       return OK;
> > > > +}
> > > > +
> > > > +MdParserSmia::ParseStatus
> MdParserSmia::findRegs(libcamera::Span<const uint8_t> buffer)
> > > > +{
> > > > +       ASSERT(offsets_.size());
> > > >
> > > >         if (buffer[0] != LINE_START)
> > > >                 return NO_LINE_START;
> > > >
> > > >         unsigned int current_offset = 1; /* after the LINE_START */
> > > >         unsigned int current_line_start = 0, current_line = 0;
> > > > -       unsigned int reg_num = 0, first_reg = 0;
> > > > +       unsigned int reg_num = 0, regs_done = 0;
> > > >
> > > >         while (1) {
> > > >                 int tag = buffer[current_offset++];
> > > > @@ -89,13 +134,12 @@ MdParserSmia::ParseStatus
> MdParserSmia::findRegs(libcamera::Span<const uint8_t>
> > > >                         else if (tag == REG_SKIP)
> > > >                                 reg_num++;
> > > >                         else if (tag == REG_VALUE) {
> > > > -                               while (reg_num >=
> > > > -                                      /* assumes registers are in
> order... */
> > > > -                                      regs[first_reg]) {
> > > > -                                       if (reg_num ==
> regs[first_reg])
> > > > -                                               offsets[first_reg] =
> current_offset - 1;
> > > > +                               auto reg = offsets_.find(reg_num);
> > > > +
> > > > +                               if (reg != offsets_.end()) {
> > > > +                                       offsets_[reg_num] =
> current_offset - 1;
> > > >
> > > > -                                       if (++first_reg == num_regs)
> > > > +                                       if (++regs_done ==
> offsets_.size())
> > > >                                                 return PARSE_OK;
> > > >                                 }
> > > >                                 reg_num++;
>
> --
> Regards,
>
> Laurent Pinchart
>

Hi Naush,

On Tue, Jun 29, 2021 at 07:20:13PM +0100, Naushir Patuck wrote:
> On Tue, 29 Jun 2021 at 16:47, Laurent Pinchart wrote:
> > On Tue, Jun 29, 2021 at 05:44:14PM +0300, Laurent Pinchart wrote:
> > > On Tue, Jun 29, 2021 at 02:53:50PM +0100, Naushir Patuck wrote:
> > > > On Tue, 29 Jun 2021 at 11:45, Naushir Patuck wrote:
> > > >
> > > > > Instead of having each CamHelper subclass the MdParserSmia, change the
> > > > > implementation of MdParserSmia to be more generic. The MdParserSmia now gets
> > > > > given a list of registers to search for and helper functions are used to compute
> > > > > exposure lines and gain codes from these registers.
> > > > >
> > > > > Update the imx219 and imx477 CamHelpers by using this new mechanism.
> > > > >
> > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > > ---
> > > > >  src/ipa/raspberrypi/cam_helper.cpp        |  33 +++---
> > > > >  src/ipa/raspberrypi/cam_helper.hpp        |   2 +
> > > > >  src/ipa/raspberrypi/cam_helper_imx219.cpp | 114 ++++----------------
> > > > >  src/ipa/raspberrypi/cam_helper_imx477.cpp | 122 ++++------------------
> > > > >  src/ipa/raspberrypi/md_parser.hpp         |  41 +++++---
> > > > >  src/ipa/raspberrypi/md_parser_smia.cpp    |  66 ++++++++++--
> > > > >  6 files changed, 144 insertions(+), 234 deletions(-)
> > > > >
> > > > > diff --git a/src/ipa/raspberrypi/cam_helper.cpp b/src/ipa/raspberrypi/cam_helper.cpp
> > > > > index 90498c37af98..e6d2258c66d7 100644
> > > > > --- a/src/ipa/raspberrypi/cam_helper.cpp
> > > > > +++ b/src/ipa/raspberrypi/cam_helper.cpp
> > > > > @@ -159,32 +159,34 @@ unsigned int CamHelper::MistrustFramesModeSwitch() const
> > > > >  void CamHelper::parseEmbeddedData(Span<const uint8_t> buffer,
> > > > >                                   Metadata &metadata)
> > > > >  {
> > > > > +       MdParser::RegisterMap registers;
> > > > > +       Metadata parsedMetadata;
> > > > > +
> > > > >         if (buffer.empty())
> > > > >                 return;
> > > > >
> > > > > -       uint32_t exposureLines, gainCode;
> > > > > -
> > > > > -       if (parser_->Parse(buffer) != MdParser::Status::OK ||
> > > > > -           parser_->GetExposureLines(exposureLines) != MdParser::Status::OK ||
> > > > > -           parser_->GetGainCode(gainCode) != MdParser::Status::OK) {
> > > > > +       if (parser_->Parse(buffer, registers) != MdParser::Status::OK) {
> > > > >                 LOG(IPARPI, Error) << "Embedded data buffer parsing failed";
> > > > >                 return;
> > > > >         }
> > > > >
> > > > > +       PopulateMetadata(registers, parsedMetadata);
> > > > > +       metadata.Merge(parsedMetadata);
> > > > > +
> > > > >         /*
> > > > > -        * Overwrite the exposure/gain values in the DeviceStatus, as
> > > > > -        * we know better. Fetch it first in case any other fields were
> > > > > -        * set meaningfully.
> > > > > +        * Overwrite the exposure/gain values in the existing DeviceStatus with
> > > > > +        * values from the parsed embedded buffer. Fetch it first in case any
> > > > > +        * other fields were set meaningfully.
> > > > >          */
> > > > > -       DeviceStatus deviceStatus;
> > > > > -
> > > > > -       if (metadata.Get("device.status", deviceStatus) != 0) {
> > > > > +       DeviceStatus deviceStatus, parsedDeviceStatus;
> > > > > +       if (metadata.Get("device.status", deviceStatus) ||
> > > > > +           parsedMetadata.Get("device.status", parsedDeviceStatus)) {
> > > > >                 LOG(IPARPI, Error) << "DeviceStatus not found";
> > > > >                 return;
> > > > >         }
> > > > >
> > > > > -       deviceStatus.shutter_speed = Exposure(exposureLines);
> > > > > -       deviceStatus.analogue_gain = Gain(gainCode);
> > > > > +       deviceStatus.shutter_speed = parsedDeviceStatus.shutter_speed;
> > > > > +       deviceStatus.analogue_gain = parsedDeviceStatus.analogue_gain;
> > > > >
> > > > >         LOG(IPARPI, Debug) << "Metadata updated - Exposure : "
> > > > >                            << deviceStatus.shutter_speed
> > > > > @@ -194,6 +196,11 @@ void CamHelper::parseEmbeddedData(Span<const uint8_t> buffer,
> > > > >         metadata.Set("device.status", deviceStatus);
> > > > >  }
> > > > >
> > > > > +void CamHelper::PopulateMetadata([[maybe_unused]] const MdParser::RegisterMap &registers,
> > > > > +                                [[maybe_unused]] Metadata &metadata) const
> > > > > +{
> > > > > +}
> > > > > +
> > > > >  RegisterCamHelper::RegisterCamHelper(char const *cam_name,
> > > > >                                      CamHelperCreateFunc create_func)
> > > > >  {
> > > > > diff --git a/src/ipa/raspberrypi/cam_helper.hpp b/src/ipa/raspberrypi/cam_helper.hpp
> > > > > index fc3139e22be0..200cc83f3872 100644
> > > > > --- a/src/ipa/raspberrypi/cam_helper.hpp
> > > > > +++ b/src/ipa/raspberrypi/cam_helper.hpp
> > > > > @@ -92,6 +92,8 @@ public:
> > > > >  protected:
> > > > >         void parseEmbeddedData(libcamera::Span<const uint8_t> buffer,
> > > > >                                Metadata &metadata);
> > > > > +       virtual void PopulateMetadata(const MdParser::RegisterMap &registers,
> > > > > +                                     Metadata &metadata) const;
> > > > >
> > > > >         std::unique_ptr<MdParser> parser_;
> > > > >         CameraMode mode_;
> > > > > diff --git a/src/ipa/raspberrypi/cam_helper_imx219.cpp b/src/ipa/raspberrypi/cam_helper_imx219.cpp
> > > > > index c85044a5fa6d..4e4994188ff3 100644
> > > > > --- a/src/ipa/raspberrypi/cam_helper_imx219.cpp
> > > > > +++ b/src/ipa/raspberrypi/cam_helper_imx219.cpp
> > > > > @@ -23,21 +23,14 @@
> > > > >
> > > > >  using namespace RPiController;
> > > > >
> > > > > -/* Metadata parser implementation specific to Sony IMX219 sensors. */
> > > > > -
> > > > > -class MdParserImx219 : public MdParserSmia
> > > > > -{
> > > > > -public:
> > > > > -       MdParserImx219();
> > > > > -       Status Parse(libcamera::Span<const uint8_t> buffer) override;
> > > > > -       Status GetExposureLines(unsigned int &lines) override;
> > > > > -       Status GetGainCode(unsigned int &gain_code) override;
> > > > > -private:
> > > > > -       /* Offset of the register's value in the metadata block. */
> > > > > -       int reg_offsets_[3];
> > > > > -       /* Value of the register, once read from the metadata block. */
> > > > > -       int reg_values_[3];
> > > > > -};
> > > > > +/*
> > > > > + * We care about one gain register and a pair of exposure registers. Their I2C
> > > > > + * addresses from the Sony IMX219 datasheet:
> > > > > + */
> > > > > +constexpr uint32_t gainReg = 0x157;
> > > > > +constexpr uint32_t expHiReg = 0x15a;
> > > > > +constexpr uint32_t expLoReg = 0x15b;
> > > > > +constexpr std::initializer_list<uint32_t> registerList = { expHiReg, expLoReg, gainReg };
> >
> > This needs a [[maybe_unused]] in case ENABLE_EMBEDDED_DATA isn't defined.
> >
> > > > >
> > > > >  class CamHelperImx219 : public CamHelper
> > > > >  {
> > > > > @@ -54,11 +47,14 @@ private:
> > > > >          * in units of lines.
> > > > >          */
> > > > >         static constexpr int frameIntegrationDiff = 4;
> > > > > +
> > > > > +       void PopulateMetadata(const MdParser::RegisterMap &registers,
> > > > > +                             Metadata &metadata) const override;
> > > > >  };
> > > > >
> > > > >  CamHelperImx219::CamHelperImx219()
> > > > >  #if ENABLE_EMBEDDED_DATA
> > > > > -       : CamHelper(std::make_unique<MdParserImx219>(), frameIntegrationDiff)
> > > > > +       : CamHelper(std::make_unique<MdParserSmia>(registerList), frameIntegrationDiff)
> > > > >  #else
> > > > >         : CamHelper({}, frameIntegrationDiff)
> > > > >  #endif
> > > > > @@ -90,88 +86,20 @@ bool CamHelperImx219::SensorEmbeddedDataPresent() const
> > > > >         return ENABLE_EMBEDDED_DATA;
> > > > >  }
> > > > >
> > > > > -static CamHelper *Create()
> > > > > +void CamHelperImx219::PopulateMetadata(const MdParser::RegisterMap &registers,
> > > > > +                                      Metadata &metadata) const
> > > > >  {
> > > > > -       return new CamHelperImx219();
> > > > > -}
> > > > > +       DeviceStatus deviceStatus;
> > > > >
> > > > > -static RegisterCamHelper reg("imx219", &Create);
> > > > > +       deviceStatus.shutter_speed = Exposure(registers.at(expHiReg) * 256 + registers.at(expLoReg));
> > > > > +       deviceStatus.analogue_gain = Gain(registers.at(gainReg));
> > > > >
> > > > > -/*
> > > > > - * We care about one gain register and a pair of exposure registers. Their I2C
> > > > > - * addresses from the Sony IMX219 datasheet:
> > > > > - */
> > > > > -#define GAIN_REG 0x157
> > > > > -#define EXPHI_REG 0x15A
> > > > > -#define EXPLO_REG 0x15B
> > > > > -
> > > > > -/*
> > > > > - * Index of each into the reg_offsets and reg_values arrays. Must be in
> > > > > - * register address order.
> > > > > - */
> > > > > -#define GAIN_INDEX 0
> > > > > -#define EXPHI_INDEX 1
> > > > > -#define EXPLO_INDEX 2
> > > > > -
> > > > > -MdParserImx219::MdParserImx219()
> > > > > -{
> > > > > -       reg_offsets_[0] = reg_offsets_[1] = reg_offsets_[2] = -1;
> > > > > +       metadata.Set("device.status", deviceStatus);
> > > > >  }
> > > > >
> > > > > -MdParser::Status MdParserImx219::Parse(libcamera::Span<const uint8_t> buffer)
> > > > > -{
> > > > > -       bool try_again = false;
> > > > > -
> > > > > -       if (reset_) {
> > > > > -               /*
> > > > > -                * Search again through the metadata for the gain and exposure
> > > > > -                * registers.
> > > > > -                */
> > > > > -               assert(bits_per_pixel_);
> > > > > -               /* Need to be ordered */
> > > > > -               uint32_t regs[3] = { GAIN_REG, EXPHI_REG, EXPLO_REG };
> > > > > -               reg_offsets_[0] = reg_offsets_[1] = reg_offsets_[2] = -1;
> > > > > -               int ret = static_cast<int>(findRegs(buffer,
> > > > > -                                                   regs, reg_offsets_, 3));
> > > > > -               /*
> > > > > -                * > 0 means "worked partially but parse again next time",
> > > > > -                * < 0 means "hard error".
> > > > > -                */
> > > > > -               if (ret > 0)
> > > > > -                       try_again = true;
> > > > > -               else if (ret < 0)
> > > > > -                       return ERROR;
> > > > > -       }
> > > > > -
> > > > > -       for (int i = 0; i < 3; i++) {
> > > > > -               if (reg_offsets_[i] == -1)
> > > > > -                       continue;
> > > > > -
> > > > > -               reg_values_[i] = buffer[reg_offsets_[i]];
> > > > > -       }
> > > > > -
> > > > > -       /* Re-parse next time if we were unhappy in some way. */
> > > > > -       reset_ = try_again;
> > > > > -
> > > > > -       return OK;
> > > > > -}
> > > > > -
> > > > > -MdParser::Status MdParserImx219::GetExposureLines(unsigned int &lines)
> > > > > +static CamHelper *Create()
> > > > >  {
> > > > > -       if (reg_offsets_[EXPHI_INDEX] == -1 || reg_offsets_[EXPLO_INDEX] == -1)
> > > > > -               return NOTFOUND;
> > > > > -
> > > > > -       lines = reg_values_[EXPHI_INDEX] * 256 + reg_values_[EXPLO_INDEX];
> > > > > -
> > > > > -       return OK;
> > > > > +       return new CamHelperImx219();
> > > > >  }
> > > > >
> > > > > -MdParser::Status MdParserImx219::GetGainCode(unsigned int &gain_code)
> > > > > -{
> > > > > -       if (reg_offsets_[GAIN_INDEX] == -1)
> > > > > -               return NOTFOUND;
> > > > > -
> > > > > -       gain_code = reg_values_[GAIN_INDEX];
> > > > > -
> > > > > -       return OK;
> > > > > -}
> > > > > +static RegisterCamHelper reg("imx219", &Create);
> > > > > diff --git a/src/ipa/raspberrypi/cam_helper_imx477.cpp b/src/ipa/raspberrypi/cam_helper_imx477.cpp
> > > > > index d72a9be0438e..efd1a5893db8 100644
> > > > > --- a/src/ipa/raspberrypi/cam_helper_imx477.cpp
> > > > > +++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp
> > > > > @@ -15,21 +15,15 @@
> > > > >
> > > > >  using namespace RPiController;
> > > > >
> > > > > -/* Metadata parser implementation specific to Sony IMX477 sensors. */
> > > > > -
> > > > > -class MdParserImx477 : public MdParserSmia
> > > > > -{
> > > > > -public:
> > > > > -       MdParserImx477();
> > > > > -       Status Parse(libcamera::Span<const uint8_t> buffer) override;
> > > > > -       Status GetExposureLines(unsigned int &lines) override;
> > > > > -       Status GetGainCode(unsigned int &gain_code) override;
> > > > > -private:
> > > > > -       /* Offset of the register's value in the metadata block. */
> > > > > -       int reg_offsets_[4];
> > > > > -       /* Value of the register, once read from the metadata block. */
> > > > > -       int reg_values_[4];
> > > > > -};
> > > > > +/*
> > > > > + * We care about two gain registers and a pair of exposure registers. Their
> > > > > + * I2C addresses from the Sony IMX477 datasheet:
> > > > > + */
> > > > > +constexpr uint32_t expHiReg = 0x0202;
> > > > > +constexpr uint32_t expLoReg = 0x0203;
> > > > > +constexpr uint32_t gainHiReg = 0x0204;
> > > > > +constexpr uint32_t gainLoReg = 0x0205;
> > > > > +constexpr std::initializer_list<uint32_t> registerList = { expHiReg, expLoReg, gainHiReg, gainLoReg };
> >
> > Same here.
> >
> > > > >
> > > > >  class CamHelperImx477 : public CamHelper
> > > > >  {
> > > > > @@ -47,10 +41,13 @@ private:
> > > > >          * in units of lines.
> > > > >          */
> > > > >         static constexpr int frameIntegrationDiff = 22;
> > > > > +
> > > > > +       void PopulateMetadata(const MdParser::RegisterMap &registers,
> > > > > +                             Metadata &metadata) const override;
> > > > >  };
> > > > >
> > > > >  CamHelperImx477::CamHelperImx477()
> > > > > -       : CamHelper(std::make_unique<MdParserImx477>(), frameIntegrationDiff)
> > > > > +       : CamHelper(std::make_unique<MdParserSmia>(registerList), frameIntegrationDiff)
> > > > >  {
> > > > >  }
> > > > >
> > > > > @@ -77,95 +74,20 @@ bool CamHelperImx477::SensorEmbeddedDataPresent() const
> > > > >         return true;
> > > > >  }
> > > > >
> > > > > -static CamHelper *Create()
> > > > > +void CamHelperImx477::PopulateMetadata(const MdParser::RegisterMap &registers,
> > > > > +                                      Metadata &metadata) const
> > > > >  {
> > > > > -       return new CamHelperImx477();
> > > > > -}
> > > > > +       DeviceStatus deviceStatus;
> > > > >
> > > > > -static RegisterCamHelper reg("imx477", &Create);
> > > > > +       deviceStatus.shutter_speed = Exposure(registers.at(expHiReg) * 256 + registers.at(expLoReg));
> > > > > +       deviceStatus.analogue_gain = Gain(registers.at(gainHiReg) * 256 + registers.at(gainLoReg));
> > > > >
> > > > > -/*
> > > > > - * We care about two gain registers and a pair of exposure registers. Their
> > > > > - * I2C addresses from the Sony IMX477 datasheet:
> > > > > - */
> > > > > -#define EXPHI_REG 0x0202
> > > > > -#define EXPLO_REG 0x0203
> > > > > -#define GAINHI_REG 0x0204
> > > > > -#define GAINLO_REG 0x0205
> > > > > -
> > > > > -/*
> > > > > - * Index of each into the reg_offsets and reg_values arrays. Must be in register
> > > > > - * address order.
> > > > > - */
> > > > > -#define EXPHI_INDEX 0
> > > > > -#define EXPLO_INDEX 1
> > > > > -#define GAINHI_INDEX 2
> > > > > -#define GAINLO_INDEX 3
> > > > > -
> > > > > -MdParserImx477::MdParserImx477()
> > > > > -{
> > > > > -       reg_offsets_[0] = reg_offsets_[1] = reg_offsets_[2] = reg_offsets_[3] = -1;
> > > > > +       metadata.Set("device.status", deviceStatus);
> > > > >  }
> > > > >
> > > > > -MdParser::Status MdParserImx477::Parse(libcamera::Span<const uint8_t> buffer)
> > > > > -{
> > > > > -       bool try_again = false;
> > > > > -
> > > > > -       if (reset_) {
> > > > > -               /*
> > > > > -                * Search again through the metadata for the gain and exposure
> > > > > -                * registers.
> > > > > -                */
> > > > > -               assert(bits_per_pixel_);
> > > > > -               /* Need to be ordered */
> > > > > -               uint32_t regs[4] = {
> > > > > -                       EXPHI_REG,
> > > > > -                       EXPLO_REG,
> > > > > -                       GAINHI_REG,
> > > > > -                       GAINLO_REG
> > > > > -               };
> > > > > -               reg_offsets_[0] = reg_offsets_[1] = reg_offsets_[2] = reg_offsets_[3] = -1;
> > > > > -               int ret = static_cast<int>(findRegs(buffer,
> > > > > -                                                   regs, reg_offsets_, 4));
> > > > > -               /*
> > > > > -                * > 0 means "worked partially but parse again next time",
> > > > > -                * < 0 means "hard error".
> > > > > -                */
> > > > > -               if (ret > 0)
> > > > > -                       try_again = true;
> > > > > -               else if (ret < 0)
> > > > > -                       return ERROR;
> > > > > -       }
> > > > > -
> > > > > -       for (int i = 0; i < 4; i++) {
> > > > > -               if (reg_offsets_[i] == -1)
> > > > > -                       continue;
> > > > > -
> > > > > -               reg_values_[i] = buffer[reg_offsets_[i]];
> > > > > -       }
> > > > > -
> > > > > -       /* Re-parse next time if we were unhappy in some way. */
> > > > > -       reset_ = try_again;
> > > > > -
> > > > > -       return OK;
> > > > > -}
> > > > > -
> > > > > -MdParser::Status MdParserImx477::GetExposureLines(unsigned int &lines)
> > > > > +static CamHelper *Create()
> > > > >  {
> > > > > -       if (reg_offsets_[EXPHI_INDEX] == -1 || reg_offsets_[EXPLO_INDEX] == -1)
> > > > > -               return NOTFOUND;
> > > > > -
> > > > > -       lines = reg_values_[EXPHI_INDEX] * 256 + reg_values_[EXPLO_INDEX];
> > > > > -
> > > > > -       return OK;
> > > > > +       return new CamHelperImx477();
> > > > >  }
> > > > >
> > > > > -MdParser::Status MdParserImx477::GetGainCode(unsigned int &gain_code)
> > > > > -{
> > > > > -       if (reg_offsets_[GAINHI_INDEX] == -1 || reg_offsets_[GAINLO_INDEX] == -1)
> > > > > -               return NOTFOUND;
> > > > > -
> > > > > -       gain_code = reg_values_[GAINHI_INDEX] * 256 +
> > > > > reg_values_[GAINLO_INDEX];
> > > > > -
> > > > > -       return OK;
> > > > > -}
> > > > > +static RegisterCamHelper reg("imx477", &Create);
> > > > > diff --git a/src/ipa/raspberrypi/md_parser.hpp b/src/ipa/raspberrypi/md_parser.hpp
> > > > > index 8497216f8db7..e3e2738537a8 100644
> > > > > --- a/src/ipa/raspberrypi/md_parser.hpp
> > > > > +++ b/src/ipa/raspberrypi/md_parser.hpp
> > > > > @@ -6,6 +6,9 @@
> > > > >   */
> > > > >  #pragma once
> > > > >
> > > > > +#include <initializer_list>
> > > > > +#include <map>
> > > > > +#include <optional>
> > > > >  #include <stdint.h>
> > > > >
> > > > >  #include <libcamera/base/span.h>
> > > > > @@ -19,7 +22,7 @@
> > > > >   * application code doesn't have to worry which kind to instantiate. But for
> > > > >   * the sake of example let's suppose we're parsing imx219 metadata.
> > > > >   *
> > > > > - * MdParser *parser = new MdParserImx219();  // for example
> > > > > + * MdParser *parser = new MdParserSmia({ expHiReg, expLoReg, gainReg });
> > > > >   * parser->SetBitsPerPixel(bpp);
> > > > >   * parser->SetLineLengthBytes(pitch);
> > > > >   * parser->SetNumLines(2);
> > > > > @@ -32,13 +35,11 @@
> > > > >   *
> > > > >   * Then on every frame:
> > > > >   *
> > > > > - * if (parser->Parse(buffer) != MdParser::OK)
> > > > > + * RegisterMap registers;
> > > > > + * if (parser->Parse(buffer, registers) != MdParser::OK)
> > > > >   *     much badness;
> > > > > - * unsigned int exposure_lines, gain_code
> > > > > - * if (parser->GetExposureLines(exposure_lines) != MdParser::OK)
> > > > > - *     exposure was not found;
> > > > > - * if (parser->GetGainCode(parser, gain_code) != MdParser::OK)
> > > > > - *     gain code was not found;
> > > > > + * Metadata metadata;
> > > > > + * CamHelper::PopulateMetadata(registers, metadata);
> > > > >   *
> > > > >   * (Note that the CamHelper class converts to/from exposure lines and time,
> > > > >   * and gain_code / actual gain.)
> > > > > @@ -59,6 +60,8 @@ namespace RPiController {
> > > > >  class MdParser
> > > > >  {
> > > > >  public:
> > > > > +       using RegisterMap = std::map<uint32_t, uint32_t>;
> > > > > +
> > > > >         /*
> > > > >          * Parser status codes:
> > > > >          * OK       - success
> > > > > @@ -98,9 +101,8 @@ public:
> > > > >                 line_length_bytes_ = num_bytes;
> > > > >         }
> > > > >
> > > > > -       virtual Status Parse(libcamera::Span<const uint8_t> buffer) = 0;
> > > > > -       virtual Status GetExposureLines(unsigned int &lines) = 0;
> > > > > -       virtual Status GetGainCode(unsigned int &gain_code) = 0;
> > > > > +       virtual Status Parse(libcamera::Span<const uint8_t> buffer,
> > > > > +                            RegisterMap &registers) = 0;
> > > > >
> > > > >  protected:
> > > > >         bool reset_;
> > > > > @@ -116,14 +118,18 @@ protected:
> > > > >   * md_parser_imx219.cpp for an example).
> > > > >   */
> > > > >
> > > > > -class MdParserSmia : public MdParser
> > > > > +class MdParserSmia final : public MdParser
> > > > >  {
> > > > >  public:
> > > > > -       MdParserSmia() : MdParser()
> > > > > -       {
> > > > > -       }
> > > > > +       MdParserSmia(std::initializer_list<uint32_t> registerList);
> > > > > +
> > > > > +       MdParser::Status Parse(libcamera::Span<const uint8_t> buffer,
> > > > > +                              RegisterMap &registers) override;
> > > > > +
> > > > > +private:
> > > > > +       /* Maps register address to offset in the buffer. */
> > > > > +       using OffsetMap = std::map<uint32_t, std::optional<uint32_t>>;
> > > > >
> > > > > -protected:
> > > > >         /*
> > > > >          * Note that error codes > 0 are regarded as non-fatal; codes < 0
> > > > >          * indicate a bad data buffer. Status codes are:
> > > > > @@ -141,8 +147,9 @@ protected:
> > > > >                 BAD_PADDING   = -5
> > > > >         };
> > > > >
> > > > > -       ParseStatus findRegs(libcamera::Span<const uint8_t> buffer, uint32_t regs[],
> > > > > -                            int offsets[], unsigned int num_regs);
> > > > > +       ParseStatus findRegs(libcamera::Span<const uint8_t> buffer);
> > > > > +
> > > > > +       OffsetMap offsets_;
> > > > >  };
> > > > >
> > > > >  } // namespace RPi
> > > > > diff --git a/src/ipa/raspberrypi/md_parser_smia.cpp b/src/ipa/raspberrypi/md_parser_smia.cpp
> > > > > index 0a14875575a2..7f72fc03948f 100644
> > > > > --- a/src/ipa/raspberrypi/md_parser_smia.cpp
> > > > > +++ b/src/ipa/raspberrypi/md_parser_smia.cpp
> > > > > @@ -6,9 +6,11 @@
> > > > >   */
> > > > >  #include <assert.h>
> > > > >
> > > >
> > > > Oops, this header is not needed any more!
> > > >
> > > > > +#include "libcamera/base/log.h"
> > >
> > > And this should be <libcamera/base/log.h>. I'll fix when applying.
> > >
> > > > >  #include "md_parser.hpp"
> > > > >
> > > > >  using namespace RPiController;
> > > > > +using namespace libcamera;
> > > > >
> > > > >  /*
> > > > >   * This function goes through the embedded data to find the offsets (not
> > > > > @@ -26,18 +28,61 @@ constexpr unsigned int REG_LOW_BITS = 0xa5;
> > > > >  constexpr unsigned int REG_VALUE = 0x5a;
> > > > >  constexpr unsigned int REG_SKIP = 0x55;
> > > > >
> > > > > -MdParserSmia::ParseStatus MdParserSmia::findRegs(libcamera::Span<const uint8_t> buffer,
> > > > > -                                                uint32_t regs[], int offsets[],
> > > > > -                                                unsigned int num_regs)
> > > > > +MdParserSmia::MdParserSmia(std::initializer_list<uint32_t> registerList)
> > > > >  {
> > > > > -       assert(num_regs > 0);
> > > > > +       for (auto r : registerList)
> > > > > +               offsets_[r] = {};
> > > > > +}
> > > > > +
> > > > > +MdParser::Status MdParserSmia::Parse(libcamera::Span<const uint8_t> buffer,
> > > > > +                                    RegisterMap &registers)
> > > > > +{
> > > > > +       if (reset_) {
> > > > > +               /*
> > > > > +                * Search again through the metadata for all the registers
> > > > > +                * requested.
> > > > > +                */
> > > > > +               ASSERT(bits_per_pixel_);
> > > > > +
> > > > > +               for (const auto &[reg, offset] : offsets_)
> > > > > +                       offsets_[reg] = {};
> >
> > I'm afraid this causes compilation failures with gcc 7 and gcc 8, due to
> > offset being unused. We can't throw a [[maybe_unused]] here, that's not
> > supported, so we need to revert to the syntax from v2.
> >
> >                 for (const auto &kv : offsets_)
> >                         offsets_[kv.first] = {};
> >
> > I'll post a v3.1, Naush, could you give it a try ?
> 
> Thank you for the fixups!  I'm a bit curious why my compiler did not throw out warnings
> for these though...?

It got fixed in gcc 9, maybe that's why ?

> I'll test and report back first thing tomorrow.

Thank you.

> > > > > +
> > > > > +               ParseStatus ret = findRegs(buffer);
> > > > > +               /*
> > > > > +                * > 0 means "worked partially but parse again next time",
> > > > > +                * < 0 means "hard error".
> > > > > +                *
> > > > > +                * In either case, we retry parsing on the next frame.
> > > > > +                */
> > > > > +               if (ret != PARSE_OK)
> > > > > +                       return ERROR;
> > > > > +
> > > > > +               reset_ = false;
> > > > > +       }
> > > > > +
> > > > > +       /* Populate the register values requested. */
> > > > > +       registers.clear();
> > > > > +       for (const auto &[reg, offset] : offsets_) {
> > > > > +               if (!offset) {
> > > > > +                       reset_ = true;
> > > > > +                       return NOTFOUND;
> > > > > +               }
> > > > > +               registers[reg] = buffer[offset.value()];
> > > > > +       }
> > > > > +
> > > > > +       return OK;
> > > > > +}
> > > > > +
> > > > > +MdParserSmia::ParseStatus MdParserSmia::findRegs(libcamera::Span<const uint8_t> buffer)
> > > > > +{
> > > > > +       ASSERT(offsets_.size());
> > > > >
> > > > >         if (buffer[0] != LINE_START)
> > > > >                 return NO_LINE_START;
> > > > >
> > > > >         unsigned int current_offset = 1; /* after the LINE_START */
> > > > >         unsigned int current_line_start = 0, current_line = 0;
> > > > > -       unsigned int reg_num = 0, first_reg = 0;
> > > > > +       unsigned int reg_num = 0, regs_done = 0;
> > > > >
> > > > >         while (1) {
> > > > >                 int tag = buffer[current_offset++];
> > > > > @@ -89,13 +134,12 @@ MdParserSmia::ParseStatus MdParserSmia::findRegs(libcamera::Span<const uint8_t>
> > > > >                         else if (tag == REG_SKIP)
> > > > >                                 reg_num++;
> > > > >                         else if (tag == REG_VALUE) {
> > > > > -                               while (reg_num >=
> > > > > -                                      /* assumes registers are in order... */
> > > > > -                                      regs[first_reg]) {
> > > > > -                                       if (reg_num == regs[first_reg])
> > > > > -                                               offsets[first_reg] = current_offset - 1;
> > > > > +                               auto reg = offsets_.find(reg_num);
> > > > > +
> > > > > +                               if (reg != offsets_.end()) {
> > > > > +                                       offsets_[reg_num] = current_offset - 1;
> > > > >
> > > > > -                                       if (++first_reg == num_regs)
> > > > > +                                       if (++regs_done == offsets_.size())
> > > > >                                                 return PARSE_OK;
> > > > >                                 }
> > > > >                                 reg_num++;