[libcamera-devel] [PATCH 3/3] ipa: raspberrypi: Generalise the SMIA metadata parser
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Jun 22 13:12:43 CEST 2021
Hi Naush,
Thank you for the patch.
On Tue, Jun 15, 2021 at 03:42:11PM +0100, 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 at raspberrypi.com>
> ---
> src/ipa/raspberrypi/cam_helper.cpp | 35 +++---
> src/ipa/raspberrypi/cam_helper.hpp | 2 +
> src/ipa/raspberrypi/cam_helper_imx219.cpp | 118 ++++----------------
> src/ipa/raspberrypi/cam_helper_imx477.cpp | 124 ++++------------------
> src/ipa/raspberrypi/md_parser.hpp | 42 +++++---
> src/ipa/raspberrypi/md_parser_smia.cpp | 66 ++++++++++--
> 6 files changed, 148 insertions(+), 239 deletions(-)
>
> diff --git a/src/ipa/raspberrypi/cam_helper.cpp b/src/ipa/raspberrypi/cam_helper.cpp
> index 1474464c9257..41e292ecbb61 100644
> --- a/src/ipa/raspberrypi/cam_helper.cpp
> +++ b/src/ipa/raspberrypi/cam_helper.cpp
> @@ -159,32 +159,32 @@ unsigned int CamHelper::MistrustFramesModeSwitch() const
> void CamHelper::parseEmbeddedData(Span<const uint8_t> buffer,
> Metadata &metadata)
> {
> + MdParser::RegisterMap map;
> + 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, map) != MdParser::Status::OK) {
> LOG(IPARPI, Error) << "Embedded data buffer parsing failed";
> return;
> }
>
> + PopulateMetadata(map, 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) {
> - LOG(IPARPI, Error) << "DeviceStatus not found";
No log anymore ?
> + DeviceStatus deviceStatus, parsedDeviceStatus;
> + if (metadata.Get("device.status", deviceStatus) ||
> + parsedMetadata.Get("device.status", parsedDeviceStatus))
> 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 +194,11 @@ void CamHelper::parseEmbeddedData(Span<const uint8_t> buffer,
> metadata.Set("device.status", deviceStatus);
> }
>
> +void CamHelper::PopulateMetadata([[maybe_unused]] const MdParser::RegisterMap &map,
> + [[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 f53f5c39b01c..30879e60b861 100644
> --- a/src/ipa/raspberrypi/cam_helper.hpp
> +++ b/src/ipa/raspberrypi/cam_helper.hpp
> @@ -91,6 +91,8 @@ public:
> protected:
> void parseEmbeddedData(libcamera::Span<const uint8_t> buffer,
> Metadata &metadata);
> + virtual void PopulateMetadata([[maybe_unused]] const MdParser::RegisterMap &map,
> + [[maybe_unused]] Metadata &metadata) const;
>
> MdParser *parser_;
> CameraMode mode_;
> diff --git a/src/ipa/raspberrypi/cam_helper_imx219.cpp b/src/ipa/raspberrypi/cam_helper_imx219.cpp
> index d951cd552a21..24818e9ec4de 100644
> --- a/src/ipa/raspberrypi/cam_helper_imx219.cpp
> +++ b/src/ipa/raspberrypi/cam_helper_imx219.cpp
> @@ -23,21 +23,13 @@
>
> 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;
>
> class CamHelperImx219 : public CamHelper
> {
> @@ -55,15 +47,19 @@ private:
> */
> static constexpr int frameIntegrationDiff = 4;
>
> - MdParserImx219 imx219_parser;
> + MdParserSmia imx219_parser;
Could you name this just "parser" ? It's a member of the CamHelperImx219
class, there's no need to repeat the prefix. Same for imx477.
> +
> + void PopulateMetadata(const MdParser::RegisterMap &map,
> + Metadata &metadata) const override;
> };
>
> CamHelperImx219::CamHelperImx219()
> #if ENABLE_EMBEDDED_DATA
> - : CamHelper(&imx219_parser, frameIntegrationDiff)
> + : CamHelper(&imx219_parser, frameIntegrationDiff),
You're passing a pointer to an object that isn't constructed yet. The
base class only stores it so it should be OK, but it's a bit fragile.
> #else
> - : CamHelper(nullptr, frameIntegrationDiff)
> + : CamHelper(nullptr, frameIntegrationDiff),
> #endif
> + imx219_parser({ gainReg, expHiReg, expLoReg })
> {
> }
>
> @@ -92,88 +88,20 @@ bool CamHelperImx219::SensorEmbeddedDataPresent() const
> return ENABLE_EMBEDDED_DATA;
> }
>
> -static CamHelper *Create()
> +void CamHelperImx219::PopulateMetadata(const MdParser::RegisterMap &map,
> + Metadata &metadata) const
> {
> - return new CamHelperImx219();
> -}
> + DeviceStatus deviceStatus;
>
> -static RegisterCamHelper reg("imx219", &Create);
> + deviceStatus.shutter_speed = Exposure(map.at(expHiReg) * 256 + map.at(expLoReg));
> + deviceStatus.analogue_gain = Gain(map.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 44f030ed7da9..7586e5f897d5 100644
> --- a/src/ipa/raspberrypi/cam_helper_imx477.cpp
> +++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp
> @@ -15,21 +15,14 @@
>
> 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;
>
> class CamHelperImx477 : public CamHelper
> {
> @@ -48,11 +41,15 @@ private:
> */
> static constexpr int frameIntegrationDiff = 22;
>
> - MdParserImx477 imx477_parser;
> + MdParserSmia imx477_parser;
> +
> + void PopulateMetadata(const MdParser::RegisterMap &map,
> + Metadata &metadata) const override;
> };
>
> CamHelperImx477::CamHelperImx477()
> - : CamHelper(&imx477_parser, frameIntegrationDiff)
> + : CamHelper(&imx477_parser, frameIntegrationDiff),
> + imx477_parser({ expHiReg, expLoReg, gainHiReg, gainLoReg })
> {
> }
>
> @@ -79,95 +76,20 @@ bool CamHelperImx477::SensorEmbeddedDataPresent() const
> return true;
> }
>
> -static CamHelper *Create()
> +void CamHelperImx477::PopulateMetadata(const MdParser::RegisterMap &map,
> + Metadata &metadata) const
> {
> - return new CamHelperImx477();
> -}
> + DeviceStatus deviceStatus;
>
> -static RegisterCamHelper reg("imx477", &Create);
> + deviceStatus.shutter_speed = Exposure(map.at(expHiReg) * 256 + map.at(expLoReg));
> + deviceStatus.analogue_gain = Gain(map.at(gainHiReg) * 256 + map.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 65aab02d51b6..64f9bbb12845 100644
> --- a/src/ipa/raspberrypi/md_parser.hpp
> +++ b/src/ipa/raspberrypi/md_parser.hpp
> @@ -8,6 +8,10 @@
>
> #include <stdint.h>
>
> +#include <map>
> +#include <optional>
> +#include <vector>
> +
> #include <libcamera/span.h>
>
> /*
> @@ -19,7 +23,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 +36,11 @@
> *
> * Then on every frame:
> *
> - * if (parser->Parse(buffer) != MdParser::OK)
> + * RegisterMap map;
> + * if (parser->Parse(buffer, map) != 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(map, metadata);
> *
> * (Note that the CamHelper class converts to/from exposure lines and time,
> * and gain_code / actual gain.)
> @@ -59,6 +61,8 @@ namespace RPiController {
> class MdParser
> {
> public:
> + using RegisterMap = std::map<uint32_t, uint32_t>;
> +
> /*
> * Parser status codes:
> * OK - success
> @@ -98,9 +102,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 &map) = 0;
I'd name the variable registers to tell what it contains (same in the
caller and in the example above).
>
> protected:
> bool reset_;
> @@ -116,14 +119,18 @@ protected:
> * md_parser_imx219.cpp for an example).
> */
>
> -class MdParserSmia : public MdParser
> +class MdParserSmia final : public MdParser
> {
> public:
> - MdParserSmia() : MdParser()
> - {
> - }
> + MdParserSmia(const std::vector<uint32_t> ®s);
> +
> + MdParser::Status Parse(libcamera::Span<const uint8_t> buffer,
> + RegisterMap &map) 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 +148,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..9920699dfe25 100644
> --- a/src/ipa/raspberrypi/md_parser_smia.cpp
> +++ b/src/ipa/raspberrypi/md_parser_smia.cpp
> @@ -6,9 +6,11 @@
> */
> #include <assert.h>
>
> +#include "libcamera/internal/log.h"
Do you need this header ?
> #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(const std::vector<uint32_t> ®isters)
> {
> - assert(num_regs > 0);
> + for (auto r : registers)
> + offsets_[r] = {};
> +}
> +
> +MdParser::Status MdParserSmia::Parse(libcamera::Span<const uint8_t> buffer,
> + RegisterMap &map)
> +{
> + if (reset_) {
> + /*
> + * Search again through the metadata for the gain and exposure
> + * registers.
> + */
It's not just gain and exposure, this class is generic and can handle
any register.
I'm curious, is findRegs() that costly that you want to cache the
results ? Does SMIA++ guarantee that embedded data will be formatted the
same way for every frame ?
I've pushed 2/3 already. 1/3 looks good to me, but I've held off in case
some of the review for 3/3 requires changing 1/3.
> + ASSERT(bits_per_pixel_);
> +
> + for (auto &kv : offsets_)
> + offsets_[kv.first] = {};
> +
> + 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. */
> + map.clear();
> + for (auto &kv : offsets_) {
> + if (!kv.second) {
> + reset_ = true;
> + return NOTFOUND;
> + }
> + map[kv.first] = buffer[kv.second.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
More information about the libcamera-devel
mailing list