<div dir="ltr"><div dir="ltr">Hi Laurent,<div><br></div><div>Thank you for your feedback.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, 22 Jun 2021 at 12:13, 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">Hi Naush,<br>
<br>
Thank you for the patch.<br>
<br>
On Tue, Jun 15, 2021 at 03:42:11PM +0100, Naushir Patuck wrote:<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>
> ---<br>
> src/ipa/raspberrypi/cam_helper.cpp | 35 +++---<br>
> src/ipa/raspberrypi/cam_helper.hpp | 2 +<br>
> src/ipa/raspberrypi/cam_helper_imx219.cpp | 118 ++++----------------<br>
> src/ipa/raspberrypi/cam_helper_imx477.cpp | 124 ++++------------------<br>
> src/ipa/raspberrypi/md_parser.hpp | 42 +++++---<br>
> src/ipa/raspberrypi/md_parser_smia.cpp | 66 ++++++++++--<br>
> 6 files changed, 148 insertions(+), 239 deletions(-)<br>
> <br>
> diff --git a/src/ipa/raspberrypi/cam_helper.cpp b/src/ipa/raspberrypi/cam_helper.cpp<br>
> index 1474464c9257..41e292ecbb61 100644<br>
> --- a/src/ipa/raspberrypi/cam_helper.cpp<br>
> +++ b/src/ipa/raspberrypi/cam_helper.cpp<br>
> @@ -159,32 +159,32 @@ unsigned int CamHelper::MistrustFramesModeSwitch() const<br>
> void CamHelper::parseEmbeddedData(Span<const uint8_t> buffer,<br>
> Metadata &metadata)<br>
> {<br>
> + MdParser::RegisterMap map;<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, map) != MdParser::Status::OK) {<br>
> LOG(IPARPI, Error) << "Embedded data buffer parsing failed";<br>
> return;<br>
> }<br>
> <br>
> + PopulateMetadata(map, 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>
> - LOG(IPARPI, Error) << "DeviceStatus not found";<br>
<br>
No log anymore ?<br></blockquote><div><br></div><div>Ack, I'll add it back below.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> + DeviceStatus deviceStatus, parsedDeviceStatus;<br>
> + if (metadata.Get("device.status", deviceStatus) ||<br>
> + parsedMetadata.Get("device.status", parsedDeviceStatus))<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 +194,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 &map,<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 f53f5c39b01c..30879e60b861 100644<br>
> --- a/src/ipa/raspberrypi/cam_helper.hpp<br>
> +++ b/src/ipa/raspberrypi/cam_helper.hpp<br>
> @@ -91,6 +91,8 @@ public:<br>
> protected:<br>
> void parseEmbeddedData(libcamera::Span<const uint8_t> buffer,<br>
> Metadata &metadata);<br>
> + virtual void PopulateMetadata([[maybe_unused]] const MdParser::RegisterMap &map,<br>
> + [[maybe_unused]] Metadata &metadata) const;<br>
> <br>
> 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 d951cd552a21..24818e9ec4de 100644<br>
> --- a/src/ipa/raspberrypi/cam_helper_imx219.cpp<br>
> +++ b/src/ipa/raspberrypi/cam_helper_imx219.cpp<br>
> @@ -23,21 +23,13 @@<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>
> <br>
> class CamHelperImx219 : public CamHelper<br>
> {<br>
> @@ -55,15 +47,19 @@ private:<br>
> */<br>
> static constexpr int frameIntegrationDiff = 4;<br>
> <br>
> - MdParserImx219 imx219_parser;<br>
> + MdParserSmia imx219_parser;<br>
<br>
Could you name this just "parser" ? It's a member of the CamHelperImx219<br>
class, there's no need to repeat the prefix. Same for imx477.<br></blockquote><div><br></div><div>Ack.</div><div> </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>
> + void PopulateMetadata(const MdParser::RegisterMap &map,<br>
> + Metadata &metadata) const override;<br>
> };<br>
> <br>
> CamHelperImx219::CamHelperImx219()<br>
> #if ENABLE_EMBEDDED_DATA<br>
> - : CamHelper(&imx219_parser, frameIntegrationDiff)<br>
> + : CamHelper(&imx219_parser, frameIntegrationDiff),<br>
<br>
You're passing a pointer to an object that isn't constructed yet. The<br>
base class only stores it so it should be OK, but it's a bit fragile.<br></blockquote><div><br></div><div>I agree, this is slightly naughty. Without relying in a dynamic</div><div>allocation, there is no other way to construct the object before</div><div>passing on the pointer to the base class.</div><div><br></div><div>Perhaps I should replace patch 1/3 to use a shared_ptr instead of</div><div>embedding the parser object in the derived class. This way, the memory</div><div>is correctly managed, and the object will be constructed before returning</div><div>it to the base class.</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>
> #else<br>
> - : CamHelper(nullptr, frameIntegrationDiff)<br>
> + : CamHelper(nullptr, frameIntegrationDiff),<br>
> #endif<br>
> + imx219_parser({ gainReg, expHiReg, expLoReg })<br>
> {<br>
> }<br>
> <br>
> @@ -92,88 +88,20 @@ bool CamHelperImx219::SensorEmbeddedDataPresent() const<br>
> return ENABLE_EMBEDDED_DATA;<br>
> }<br>
> <br>
> -static CamHelper *Create()<br>
> +void CamHelperImx219::PopulateMetadata(const MdParser::RegisterMap &map,<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://map.at" rel="noreferrer" target="_blank">map.at</a>(expHiReg) * 256 + <a href="http://map.at" rel="noreferrer" target="_blank">map.at</a>(expLoReg));<br>
> + deviceStatus.analogue_gain = Gain(<a href="http://map.at" rel="noreferrer" target="_blank">map.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 44f030ed7da9..7586e5f897d5 100644<br>
> --- a/src/ipa/raspberrypi/cam_helper_imx477.cpp<br>
> +++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp<br>
> @@ -15,21 +15,14 @@<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>
> <br>
> class CamHelperImx477 : public CamHelper<br>
> {<br>
> @@ -48,11 +41,15 @@ private:<br>
> */<br>
> static constexpr int frameIntegrationDiff = 22;<br>
> <br>
> - MdParserImx477 imx477_parser;<br>
> + MdParserSmia imx477_parser;<br>
> +<br>
> + void PopulateMetadata(const MdParser::RegisterMap &map,<br>
> + Metadata &metadata) const override;<br>
> };<br>
> <br>
> CamHelperImx477::CamHelperImx477()<br>
> - : CamHelper(&imx477_parser, frameIntegrationDiff)<br>
> + : CamHelper(&imx477_parser, frameIntegrationDiff),<br>
> + imx477_parser({ expHiReg, expLoReg, gainHiReg, gainLoReg })<br>
> {<br>
> }<br>
> <br>
> @@ -79,95 +76,20 @@ bool CamHelperImx477::SensorEmbeddedDataPresent() const<br>
> return true;<br>
> }<br>
> <br>
> -static CamHelper *Create()<br>
> +void CamHelperImx477::PopulateMetadata(const MdParser::RegisterMap &map,<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://map.at" rel="noreferrer" target="_blank">map.at</a>(expHiReg) * 256 + <a href="http://map.at" rel="noreferrer" target="_blank">map.at</a>(expLoReg));<br>
> + deviceStatus.analogue_gain = Gain(<a href="http://map.at" rel="noreferrer" target="_blank">map.at</a>(gainHiReg) * 256 + <a href="http://map.at" rel="noreferrer" target="_blank">map.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 + 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 65aab02d51b6..64f9bbb12845 100644<br>
> --- a/src/ipa/raspberrypi/md_parser.hpp<br>
> +++ b/src/ipa/raspberrypi/md_parser.hpp<br>
> @@ -8,6 +8,10 @@<br>
> <br>
> #include <stdint.h><br>
> <br>
> +#include <map><br>
> +#include <optional><br>
> +#include <vector><br>
> +<br>
> #include <libcamera/span.h><br>
> <br>
> /*<br>
> @@ -19,7 +23,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 +36,11 @@<br>
> *<br>
> * Then on every frame:<br>
> *<br>
> - * if (parser->Parse(buffer) != MdParser::OK)<br>
> + * RegisterMap map;<br>
> + * if (parser->Parse(buffer, map) != 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(map, metadata);<br>
> *<br>
> * (Note that the CamHelper class converts to/from exposure lines and time,<br>
> * and gain_code / actual gain.)<br>
> @@ -59,6 +61,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 +102,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 &map) = 0;<br>
<br>
I'd name the variable registers to tell what it contains (same in the<br>
caller and in the example above).<br></blockquote><div><br></div><div>Ack.</div><div> </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>
> protected:<br>
> bool reset_;<br>
> @@ -116,14 +119,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(const std::vector<uint32_t> ®s);<br>
> +<br>
> + MdParser::Status Parse(libcamera::Span<const uint8_t> buffer,<br>
> + RegisterMap &map) 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 +148,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..9920699dfe25 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>
> +#include "libcamera/internal/log.h"<br>
<br>
Do you need this header ?<br></blockquote><div><br></div><div>Yes, I do for the ASSERT() macro below. I could use the standard assert() without</div><div>this header, but thought the libcamera version would be more consistent.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<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(const std::vector<uint32_t> ®isters)<br>
> {<br>
> - assert(num_regs > 0);<br>
> + for (auto r : registers)<br>
> + offsets_[r] = {};<br>
> +}<br>
> +<br>
> +MdParser::Status MdParserSmia::Parse(libcamera::Span<const uint8_t> buffer,<br>
> + RegisterMap &map)<br>
> +{<br>
> + if (reset_) {<br>
> + /*<br>
> + * Search again through the metadata for the gain and exposure<br>
> + * registers.<br>
> + */<br>
<br>
It's not just gain and exposure, this class is generic and can handle<br>
any register.<br></blockquote><div><br></div><div>Ack.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
I'm curious, is findRegs() that costly that you want to cache the<br>
results ? Does SMIA++ guarantee that embedded data will be formatted the<br>
same way for every frame ?<br></blockquote><div><br></div><div>The original SMIA spec guarantees that the register offsets will be the same</div><div>while streaming. Not entirely sure about SMIA++, I have not looked at the</div><div>updated spec yet.</div><div><br></div><div>It is not too expensive to parse the buffer per-frame, but given that the</div><div>offsets are fixed, I think it makes sense to cache the values as an</div><div>optimisation.</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>
I've pushed 2/3 already. 1/3 looks good to me, but I've held off in case<br>
some of the review for 3/3 requires changing 1/3.<br></blockquote><div><br></div><div>Thanks, I'll submit a new version with the suggestions shortly.</div><div><br></div><div>Regards,</div><div>Naush</div><div> </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> + ASSERT(bits_per_pixel_);<br>
> +<br>
> + for (auto &kv : offsets_)<br>
> + offsets_[kv.first] = {};<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>
> + map.clear();<br>
> + for (auto &kv : offsets_) {<br>
> + if (!kv.second) {<br>
> + reset_ = true;<br>
> + return NOTFOUND;<br>
> + }<br>
> + map[kv.first] = buffer[kv.second.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>