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