[libcamera-devel,v2,1/1] ipa: raspberrypi: Use CamHelpers to generalise sensor metadata parsing

11730 diff mbox series

CamHelpers get virtual Prepare() and Process() methods, running just
before and just after the ISP, just like Raspberry Pi Algorithms.

The Prepare() method is able to parse the register dumps in embedded
data buffers, and can be specialised to perform custom processing when
necessary.

The IPA itself only needs to call the new Prepare() and Process()
methods. It must fill in the DeviceStatus from the controls first, in
case the Prepare() method does not supply its own values.

Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
---
 src/ipa/raspberrypi/cam_helper.cpp  | 51 ++++++++++++++++++
 src/ipa/raspberrypi/cam_helper.hpp  | 11 +++-
 src/ipa/raspberrypi/raspberrypi.cpp | 80 ++++++++++-------------------
 3 files changed, 87 insertions(+), 55 deletions(-)

Hi David,


On Fri, 26 Mar 2021 at 12:43, David Plowman <david.plowman@raspberrypi.com>
wrote:

> CamHelpers get virtual Prepare() and Process() methods, running just
> before and just after the ISP, just like Raspberry Pi Algorithms.
>
> The Prepare() method is able to parse the register dumps in embedded
> data buffers, and can be specialised to perform custom processing when
> necessary.
>
> The IPA itself only needs to call the new Prepare() and Process()
> methods. It must fill in the DeviceStatus from the controls first, in
> case the Prepare() method does not supply its own values.
>
> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> ---
>  src/ipa/raspberrypi/cam_helper.cpp  | 51 ++++++++++++++++++
>  src/ipa/raspberrypi/cam_helper.hpp  | 11 +++-
>  src/ipa/raspberrypi/raspberrypi.cpp | 80 ++++++++++-------------------
>  3 files changed, 87 insertions(+), 55 deletions(-)
>
> diff --git a/src/ipa/raspberrypi/cam_helper.cpp
> b/src/ipa/raspberrypi/cam_helper.cpp
> index 0ae0baa0..5af73c9c 100644
> --- a/src/ipa/raspberrypi/cam_helper.cpp
> +++ b/src/ipa/raspberrypi/cam_helper.cpp
> @@ -17,6 +17,11 @@
>  #include "md_parser.hpp"
>
>  using namespace RPiController;
> +using namespace libcamera;
> +
> +namespace libcamera {
> +LOG_DECLARE_CATEGORY(IPARPI)
> +}
>
>  static std::map<std::string, CamHelperCreateFunc> cam_helpers;
>
> @@ -45,6 +50,17 @@ CamHelper::~CamHelper()
>         delete parser_;
>  }
>
> +void CamHelper::Prepare(const Span<uint8_t> &buffer,
> +                       Metadata &metadata)
> +{
> +       parseEmbeddedData(buffer, metadata);
> +}
> +
> +void CamHelper::Process([[maybe_unused]] StatisticsPtr &stats,
> +                       [[maybe_unused]] Metadata &metadata)
> +{
> +}
> +
>  uint32_t CamHelper::ExposureLines(double exposure_us) const
>  {
>         assert(initialized_);
> @@ -139,6 +155,41 @@ unsigned int CamHelper::MistrustFramesModeSwitch()
> const
>         return 0;
>  }
>
> +void CamHelper::parseEmbeddedData(const Span<uint8_t> &buffer,
> +                                 Metadata &metadata)
> +{
> +       if (buffer.size()) {
> +               bool success = false;
> +               uint32_t exposureLines, gainCode;
> +
> +               parser_->SetBufferSize(buffer.size());
> +               success = parser_->Parse(buffer.data()) ==
> MdParser::Status::OK &&
> +                         parser_->GetExposureLines(exposureLines) ==
> MdParser::Status::OK &&
> +                         parser_->GetGainCode(gainCode) ==
> MdParser::Status::OK;
> +
> +               /*
> +                * Overwrite the exposure/gain values in the DeviceStatus,
> as
> +                * we know better. Fetch it first in case any other fields
> were
> +                * set meaningfully.
> +                */
> +               struct DeviceStatus deviceStatus;
> +
> +               if (success &&
> +                   metadata.Get("device.status", deviceStatus) == 0) {
>

I'm trying to work out if the second clause is needed?  Shouldn't we write
device.status
unconditionally, whether it is present in the metadata or not?  I suppose
it does not matter,
it is guaranteed to be there from the IPA.

Reviewed-by: Naushir Patuck <naush@raspberrypi.com>



> +                       deviceStatus.shutter_speed =
> Exposure(exposureLines);
> +                       deviceStatus.analogue_gain = Gain(gainCode);
> +
> +                       LOG(IPARPI, Debug) << "Metadata updated - Exposure
> : "
> +                                          << deviceStatus.shutter_speed
> +                                          << " Gain : "
> +                                          << deviceStatus.analogue_gain;
> +
> +                       metadata.Set("device.status", deviceStatus);
> +               } else
> +                       LOG(IPARPI, Error) << "Embedded buffer parsing
> failed";
> +       }
> +}
> +
>  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 1b2d6eec..930ea39a 100644
> --- a/src/ipa/raspberrypi/cam_helper.hpp
> +++ b/src/ipa/raspberrypi/cam_helper.hpp
> @@ -8,7 +8,11 @@
>
>  #include <string>
>
> +#include <libcamera/span.h>
> +
>  #include "camera_mode.h"
> +#include "controller/controller.hpp"
> +#include "controller/metadata.hpp"
>  #include "md_parser.hpp"
>
>  #include "libcamera/internal/v4l2_videodevice.h"
> @@ -65,7 +69,9 @@ public:
>         CamHelper(MdParser *parser, unsigned int frameIntegrationDiff);
>         virtual ~CamHelper();
>         void SetCameraMode(const CameraMode &mode);
> -       MdParser &Parser() const { return *parser_; }
> +       virtual void Prepare(const libcamera::Span<uint8_t> &buffer,
> +                            Metadata &metadata);
> +       virtual void Process(StatisticsPtr &stats, Metadata &metadata);
>         uint32_t ExposureLines(double exposure_us) const;
>         double Exposure(uint32_t exposure_lines) const; // in us
>         virtual uint32_t GetVBlanking(double &exposure_us, double
> minFrameDuration,
> @@ -81,6 +87,9 @@ public:
>         virtual unsigned int MistrustFramesModeSwitch() const;
>
>  protected:
> +       void parseEmbeddedData(const libcamera::Span<uint8_t> &buffer,
> +                              Metadata &metadata);
> +
>         MdParser *parser_;
>         CameraMode mode_;
>
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp
> b/src/ipa/raspberrypi/raspberrypi.cpp
> index 1c928b72..32ecbdcd 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -101,9 +101,7 @@ private:
>         void returnEmbeddedBuffer(unsigned int bufferId);
>         void prepareISP(const ipa::RPi::ISPConfig &data);
>         void reportMetadata();
> -       bool parseEmbeddedData(unsigned int bufferId, struct DeviceStatus
> &deviceStatus);
> -       void fillDeviceStatus(uint32_t exposureLines, uint32_t gainCode,
> -                             struct DeviceStatus &deviceStatus);
> +       void fillDeviceStatus(const ControlList &sensorControls);
>         void processStats(unsigned int bufferId);
>         void applyFrameDurations(double minFrameDuration, double
> maxFrameDuration);
>         void applyAGC(const struct AgcStatus *agcStatus, ControlList
> &ctrls);
> @@ -895,35 +893,34 @@ void IPARPi::returnEmbeddedBuffer(unsigned int
> bufferId)
>
>  void IPARPi::prepareISP(const ipa::RPi::ISPConfig &data)
>  {
> -       struct DeviceStatus deviceStatus = {};
> -       bool success = false;
> +       Span<uint8_t> embeddedBuffer;
> +
> +       rpiMetadata_.Clear();
> +
> +       fillDeviceStatus(data.controls);
>
>         if (data.embeddedBufferPresent) {
>                 /*
>                  * Pipeline handler has supplied us with an embedded data
> buffer,
> -                * so parse it and extract the exposure and gain.
> +                * we must pass it to the CamHelper for parsing.
>                  */
> -               success = parseEmbeddedData(data.embeddedBufferId,
> deviceStatus);
> -
> -               /* Done with embedded data now, return to pipeline handler
> asap. */
> -               returnEmbeddedBuffer(data.embeddedBufferId);
> +               auto it = buffers_.find(data.embeddedBufferId);
> +               ASSERT(it != buffers_.end());
> +               embeddedBuffer = it->second.maps()[0];
>         }
>
> -       if (!success) {
> -               /*
> -                * Pipeline handler has not supplied an embedded data
> buffer,
> -                * or embedded data buffer parsing has failed for some
> reason,
> -                * so pull the exposure and gain values from the control
> list.
> -                */
> -               int32_t exposureLines =
> data.controls.get(V4L2_CID_EXPOSURE).get<int32_t>();
> -               int32_t gainCode =
> data.controls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>();
> -               fillDeviceStatus(exposureLines, gainCode, deviceStatus);
> -       }
> +       /*
> +        * This may overwrite the DeviceStatus using values from the sensor
> +        * metadata, and may also do additional custom processing.
> +        */
> +       helper_->Prepare(embeddedBuffer, rpiMetadata_);
> +
> +       /* Done with embedded data now, return to pipeline handler asap. */
> +       if (data.embeddedBufferPresent)
> +               returnEmbeddedBuffer(data.embeddedBufferId);
>
>         ControlList ctrls(ispCtrls_);
>
> -       rpiMetadata_.Clear();
> -       rpiMetadata_.Set("device.status", deviceStatus);
>         controller_.Prepare(&rpiMetadata_);
>
>         /* Lock the metadata buffer to avoid constant locks/unlocks. */
> @@ -973,41 +970,13 @@ void IPARPi::prepareISP(const ipa::RPi::ISPConfig
> &data)
>                 setIspControls.emit(ctrls);
>  }
>
> -bool IPARPi::parseEmbeddedData(unsigned int bufferId, struct DeviceStatus
> &deviceStatus)
> +void IPARPi::fillDeviceStatus(const ControlList &sensorControls)
>  {
> -       auto it = buffers_.find(bufferId);
> -       if (it == buffers_.end()) {
> -               LOG(IPARPI, Error) << "Could not find embedded buffer!";
> -               return false;
> -       }
> -
> -       Span<uint8_t> mem = it->second.maps()[0];
> -       helper_->Parser().SetBufferSize(mem.size());
> -       RPiController::MdParser::Status status =
> helper_->Parser().Parse(mem.data());
> -       if (status != RPiController::MdParser::Status::OK) {
> -               LOG(IPARPI, Error) << "Embedded Buffer parsing failed,
> error " << status;
> -               return false;
> -       } else {
> -               uint32_t exposureLines, gainCode;
> -               if (helper_->Parser().GetExposureLines(exposureLines) !=
> RPiController::MdParser::Status::OK) {
> -                       LOG(IPARPI, Error) << "Exposure time failed";
> -                       return false;
> -               }
> -
> -               if (helper_->Parser().GetGainCode(gainCode) !=
> RPiController::MdParser::Status::OK) {
> -                       LOG(IPARPI, Error) << "Gain failed";
> -                       return false;
> -               }
> -
> -               fillDeviceStatus(exposureLines, gainCode, deviceStatus);
> -       }
> +       struct DeviceStatus deviceStatus = {};
>
> -       return true;
> -}
> +       int32_t exposureLines =
> sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();
> +       int32_t gainCode =
> sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>();
>
> -void IPARPi::fillDeviceStatus(uint32_t exposureLines, uint32_t gainCode,
> -                             struct DeviceStatus &deviceStatus)
> -{
>         deviceStatus.shutter_speed = helper_->Exposure(exposureLines);
>         deviceStatus.analogue_gain = helper_->Gain(gainCode);
>
> @@ -1015,6 +984,8 @@ void IPARPi::fillDeviceStatus(uint32_t exposureLines,
> uint32_t gainCode,
>                            << deviceStatus.shutter_speed
>                            << " Gain : "
>                            << deviceStatus.analogue_gain;
> +
> +       rpiMetadata_.Set("device.status", deviceStatus);
>  }
>
>  void IPARPi::processStats(unsigned int bufferId)
> @@ -1028,6 +999,7 @@ void IPARPi::processStats(unsigned int bufferId)
>         Span<uint8_t> mem = it->second.maps()[0];
>         bcm2835_isp_stats *stats = reinterpret_cast<bcm2835_isp_stats
> *>(mem.data());
>         RPiController::StatisticsPtr statistics =
> std::make_shared<bcm2835_isp_stats>(*stats);
> +       helper_->Process(statistics, rpiMetadata_);
>         controller_.Process(statistics, &rpiMetadata_);
>
>         struct AgcStatus agcStatus;
> --
> 2.20.1
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
>

Hi everyone

I was wondering if I could apply a little nudge to this one too as
there are some other patches in the system relying on it. Does this
need more thought, do we think?

Thanks!
David

On Fri, 26 Mar 2021 at 13:43, Naushir Patuck <naush@raspberrypi.com> wrote:
>
> Hi David,
>
>
> On Fri, 26 Mar 2021 at 12:43, David Plowman <david.plowman@raspberrypi.com> wrote:
>>
>> CamHelpers get virtual Prepare() and Process() methods, running just
>> before and just after the ISP, just like Raspberry Pi Algorithms.
>>
>> The Prepare() method is able to parse the register dumps in embedded
>> data buffers, and can be specialised to perform custom processing when
>> necessary.
>>
>> The IPA itself only needs to call the new Prepare() and Process()
>> methods. It must fill in the DeviceStatus from the controls first, in
>> case the Prepare() method does not supply its own values.
>>
>> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
>> ---
>>  src/ipa/raspberrypi/cam_helper.cpp  | 51 ++++++++++++++++++
>>  src/ipa/raspberrypi/cam_helper.hpp  | 11 +++-
>>  src/ipa/raspberrypi/raspberrypi.cpp | 80 ++++++++++-------------------
>>  3 files changed, 87 insertions(+), 55 deletions(-)
>>
>> diff --git a/src/ipa/raspberrypi/cam_helper.cpp b/src/ipa/raspberrypi/cam_helper.cpp
>> index 0ae0baa0..5af73c9c 100644
>> --- a/src/ipa/raspberrypi/cam_helper.cpp
>> +++ b/src/ipa/raspberrypi/cam_helper.cpp
>> @@ -17,6 +17,11 @@
>>  #include "md_parser.hpp"
>>
>>  using namespace RPiController;
>> +using namespace libcamera;
>> +
>> +namespace libcamera {
>> +LOG_DECLARE_CATEGORY(IPARPI)
>> +}
>>
>>  static std::map<std::string, CamHelperCreateFunc> cam_helpers;
>>
>> @@ -45,6 +50,17 @@ CamHelper::~CamHelper()
>>         delete parser_;
>>  }
>>
>> +void CamHelper::Prepare(const Span<uint8_t> &buffer,
>> +                       Metadata &metadata)
>> +{
>> +       parseEmbeddedData(buffer, metadata);
>> +}
>> +
>> +void CamHelper::Process([[maybe_unused]] StatisticsPtr &stats,
>> +                       [[maybe_unused]] Metadata &metadata)
>> +{
>> +}
>> +
>>  uint32_t CamHelper::ExposureLines(double exposure_us) const
>>  {
>>         assert(initialized_);
>> @@ -139,6 +155,41 @@ unsigned int CamHelper::MistrustFramesModeSwitch() const
>>         return 0;
>>  }
>>
>> +void CamHelper::parseEmbeddedData(const Span<uint8_t> &buffer,
>> +                                 Metadata &metadata)
>> +{
>> +       if (buffer.size()) {
>> +               bool success = false;
>> +               uint32_t exposureLines, gainCode;
>> +
>> +               parser_->SetBufferSize(buffer.size());
>> +               success = parser_->Parse(buffer.data()) == MdParser::Status::OK &&
>> +                         parser_->GetExposureLines(exposureLines) == MdParser::Status::OK &&
>> +                         parser_->GetGainCode(gainCode) == MdParser::Status::OK;
>> +
>> +               /*
>> +                * Overwrite the exposure/gain values in the DeviceStatus, as
>> +                * we know better. Fetch it first in case any other fields were
>> +                * set meaningfully.
>> +                */
>> +               struct DeviceStatus deviceStatus;
>> +
>> +               if (success &&
>> +                   metadata.Get("device.status", deviceStatus) == 0) {
>
>
> I'm trying to work out if the second clause is needed?  Shouldn't we write device.status
> unconditionally, whether it is present in the metadata or not?  I suppose it does not matter,
> it is guaranteed to be there from the IPA.
>
> Reviewed-by: Naushir Patuck <naush@raspberrypi.com>
>
>
>>
>> +                       deviceStatus.shutter_speed = Exposure(exposureLines);
>> +                       deviceStatus.analogue_gain = Gain(gainCode);
>> +
>> +                       LOG(IPARPI, Debug) << "Metadata updated - Exposure : "
>> +                                          << deviceStatus.shutter_speed
>> +                                          << " Gain : "
>> +                                          << deviceStatus.analogue_gain;
>> +
>> +                       metadata.Set("device.status", deviceStatus);
>> +               } else
>> +                       LOG(IPARPI, Error) << "Embedded buffer parsing failed";
>> +       }
>> +}
>> +
>>  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 1b2d6eec..930ea39a 100644
>> --- a/src/ipa/raspberrypi/cam_helper.hpp
>> +++ b/src/ipa/raspberrypi/cam_helper.hpp
>> @@ -8,7 +8,11 @@
>>
>>  #include <string>
>>
>> +#include <libcamera/span.h>
>> +
>>  #include "camera_mode.h"
>> +#include "controller/controller.hpp"
>> +#include "controller/metadata.hpp"
>>  #include "md_parser.hpp"
>>
>>  #include "libcamera/internal/v4l2_videodevice.h"
>> @@ -65,7 +69,9 @@ public:
>>         CamHelper(MdParser *parser, unsigned int frameIntegrationDiff);
>>         virtual ~CamHelper();
>>         void SetCameraMode(const CameraMode &mode);
>> -       MdParser &Parser() const { return *parser_; }
>> +       virtual void Prepare(const libcamera::Span<uint8_t> &buffer,
>> +                            Metadata &metadata);
>> +       virtual void Process(StatisticsPtr &stats, Metadata &metadata);
>>         uint32_t ExposureLines(double exposure_us) const;
>>         double Exposure(uint32_t exposure_lines) const; // in us
>>         virtual uint32_t GetVBlanking(double &exposure_us, double minFrameDuration,
>> @@ -81,6 +87,9 @@ public:
>>         virtual unsigned int MistrustFramesModeSwitch() const;
>>
>>  protected:
>> +       void parseEmbeddedData(const libcamera::Span<uint8_t> &buffer,
>> +                              Metadata &metadata);
>> +
>>         MdParser *parser_;
>>         CameraMode mode_;
>>
>> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
>> index 1c928b72..32ecbdcd 100644
>> --- a/src/ipa/raspberrypi/raspberrypi.cpp
>> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
>> @@ -101,9 +101,7 @@ private:
>>         void returnEmbeddedBuffer(unsigned int bufferId);
>>         void prepareISP(const ipa::RPi::ISPConfig &data);
>>         void reportMetadata();
>> -       bool parseEmbeddedData(unsigned int bufferId, struct DeviceStatus &deviceStatus);
>> -       void fillDeviceStatus(uint32_t exposureLines, uint32_t gainCode,
>> -                             struct DeviceStatus &deviceStatus);
>> +       void fillDeviceStatus(const ControlList &sensorControls);
>>         void processStats(unsigned int bufferId);
>>         void applyFrameDurations(double minFrameDuration, double maxFrameDuration);
>>         void applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls);
>> @@ -895,35 +893,34 @@ void IPARPi::returnEmbeddedBuffer(unsigned int bufferId)
>>
>>  void IPARPi::prepareISP(const ipa::RPi::ISPConfig &data)
>>  {
>> -       struct DeviceStatus deviceStatus = {};
>> -       bool success = false;
>> +       Span<uint8_t> embeddedBuffer;
>> +
>> +       rpiMetadata_.Clear();
>> +
>> +       fillDeviceStatus(data.controls);
>>
>>         if (data.embeddedBufferPresent) {
>>                 /*
>>                  * Pipeline handler has supplied us with an embedded data buffer,
>> -                * so parse it and extract the exposure and gain.
>> +                * we must pass it to the CamHelper for parsing.
>>                  */
>> -               success = parseEmbeddedData(data.embeddedBufferId, deviceStatus);
>> -
>> -               /* Done with embedded data now, return to pipeline handler asap. */
>> -               returnEmbeddedBuffer(data.embeddedBufferId);
>> +               auto it = buffers_.find(data.embeddedBufferId);
>> +               ASSERT(it != buffers_.end());
>> +               embeddedBuffer = it->second.maps()[0];
>>         }
>>
>> -       if (!success) {
>> -               /*
>> -                * Pipeline handler has not supplied an embedded data buffer,
>> -                * or embedded data buffer parsing has failed for some reason,
>> -                * so pull the exposure and gain values from the control list.
>> -                */
>> -               int32_t exposureLines = data.controls.get(V4L2_CID_EXPOSURE).get<int32_t>();
>> -               int32_t gainCode = data.controls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>();
>> -               fillDeviceStatus(exposureLines, gainCode, deviceStatus);
>> -       }
>> +       /*
>> +        * This may overwrite the DeviceStatus using values from the sensor
>> +        * metadata, and may also do additional custom processing.
>> +        */
>> +       helper_->Prepare(embeddedBuffer, rpiMetadata_);
>> +
>> +       /* Done with embedded data now, return to pipeline handler asap. */
>> +       if (data.embeddedBufferPresent)
>> +               returnEmbeddedBuffer(data.embeddedBufferId);
>>
>>         ControlList ctrls(ispCtrls_);
>>
>> -       rpiMetadata_.Clear();
>> -       rpiMetadata_.Set("device.status", deviceStatus);
>>         controller_.Prepare(&rpiMetadata_);
>>
>>         /* Lock the metadata buffer to avoid constant locks/unlocks. */
>> @@ -973,41 +970,13 @@ void IPARPi::prepareISP(const ipa::RPi::ISPConfig &data)
>>                 setIspControls.emit(ctrls);
>>  }
>>
>> -bool IPARPi::parseEmbeddedData(unsigned int bufferId, struct DeviceStatus &deviceStatus)
>> +void IPARPi::fillDeviceStatus(const ControlList &sensorControls)
>>  {
>> -       auto it = buffers_.find(bufferId);
>> -       if (it == buffers_.end()) {
>> -               LOG(IPARPI, Error) << "Could not find embedded buffer!";
>> -               return false;
>> -       }
>> -
>> -       Span<uint8_t> mem = it->second.maps()[0];
>> -       helper_->Parser().SetBufferSize(mem.size());
>> -       RPiController::MdParser::Status status = helper_->Parser().Parse(mem.data());
>> -       if (status != RPiController::MdParser::Status::OK) {
>> -               LOG(IPARPI, Error) << "Embedded Buffer parsing failed, error " << status;
>> -               return false;
>> -       } else {
>> -               uint32_t exposureLines, gainCode;
>> -               if (helper_->Parser().GetExposureLines(exposureLines) != RPiController::MdParser::Status::OK) {
>> -                       LOG(IPARPI, Error) << "Exposure time failed";
>> -                       return false;
>> -               }
>> -
>> -               if (helper_->Parser().GetGainCode(gainCode) != RPiController::MdParser::Status::OK) {
>> -                       LOG(IPARPI, Error) << "Gain failed";
>> -                       return false;
>> -               }
>> -
>> -               fillDeviceStatus(exposureLines, gainCode, deviceStatus);
>> -       }
>> +       struct DeviceStatus deviceStatus = {};
>>
>> -       return true;
>> -}
>> +       int32_t exposureLines = sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();
>> +       int32_t gainCode = sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>();
>>
>> -void IPARPi::fillDeviceStatus(uint32_t exposureLines, uint32_t gainCode,
>> -                             struct DeviceStatus &deviceStatus)
>> -{
>>         deviceStatus.shutter_speed = helper_->Exposure(exposureLines);
>>         deviceStatus.analogue_gain = helper_->Gain(gainCode);
>>
>> @@ -1015,6 +984,8 @@ void IPARPi::fillDeviceStatus(uint32_t exposureLines, uint32_t gainCode,
>>                            << deviceStatus.shutter_speed
>>                            << " Gain : "
>>                            << deviceStatus.analogue_gain;
>> +
>> +       rpiMetadata_.Set("device.status", deviceStatus);
>>  }
>>
>>  void IPARPi::processStats(unsigned int bufferId)
>> @@ -1028,6 +999,7 @@ void IPARPi::processStats(unsigned int bufferId)
>>         Span<uint8_t> mem = it->second.maps()[0];
>>         bcm2835_isp_stats *stats = reinterpret_cast<bcm2835_isp_stats *>(mem.data());
>>         RPiController::StatisticsPtr statistics = std::make_shared<bcm2835_isp_stats>(*stats);
>> +       helper_->Process(statistics, rpiMetadata_);
>>         controller_.Process(statistics, &rpiMetadata_);
>>
>>         struct AgcStatus agcStatus;
>> --
>> 2.20.1
>>
>> _______________________________________________
>> libcamera-devel mailing list
>> libcamera-devel@lists.libcamera.org
>> https://lists.libcamera.org/listinfo/libcamera-devel

Hi again everyone

Can I give this one another little prod too, please?

Thanks!
David

On Wed, 21 Apr 2021 at 09:59, David Plowman
<david.plowman@raspberrypi.com> wrote:
>
> Hi everyone
>
> I was wondering if I could apply a little nudge to this one too as
> there are some other patches in the system relying on it. Does this
> need more thought, do we think?
>
> Thanks!
> David
>
> On Fri, 26 Mar 2021 at 13:43, Naushir Patuck <naush@raspberrypi.com> wrote:
> >
> > Hi David,
> >
> >
> > On Fri, 26 Mar 2021 at 12:43, David Plowman <david.plowman@raspberrypi.com> wrote:
> >>
> >> CamHelpers get virtual Prepare() and Process() methods, running just
> >> before and just after the ISP, just like Raspberry Pi Algorithms.
> >>
> >> The Prepare() method is able to parse the register dumps in embedded
> >> data buffers, and can be specialised to perform custom processing when
> >> necessary.
> >>
> >> The IPA itself only needs to call the new Prepare() and Process()
> >> methods. It must fill in the DeviceStatus from the controls first, in
> >> case the Prepare() method does not supply its own values.
> >>
> >> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> >> ---
> >>  src/ipa/raspberrypi/cam_helper.cpp  | 51 ++++++++++++++++++
> >>  src/ipa/raspberrypi/cam_helper.hpp  | 11 +++-
> >>  src/ipa/raspberrypi/raspberrypi.cpp | 80 ++++++++++-------------------
> >>  3 files changed, 87 insertions(+), 55 deletions(-)
> >>
> >> diff --git a/src/ipa/raspberrypi/cam_helper.cpp b/src/ipa/raspberrypi/cam_helper.cpp
> >> index 0ae0baa0..5af73c9c 100644
> >> --- a/src/ipa/raspberrypi/cam_helper.cpp
> >> +++ b/src/ipa/raspberrypi/cam_helper.cpp
> >> @@ -17,6 +17,11 @@
> >>  #include "md_parser.hpp"
> >>
> >>  using namespace RPiController;
> >> +using namespace libcamera;
> >> +
> >> +namespace libcamera {
> >> +LOG_DECLARE_CATEGORY(IPARPI)
> >> +}
> >>
> >>  static std::map<std::string, CamHelperCreateFunc> cam_helpers;
> >>
> >> @@ -45,6 +50,17 @@ CamHelper::~CamHelper()
> >>         delete parser_;
> >>  }
> >>
> >> +void CamHelper::Prepare(const Span<uint8_t> &buffer,
> >> +                       Metadata &metadata)
> >> +{
> >> +       parseEmbeddedData(buffer, metadata);
> >> +}
> >> +
> >> +void CamHelper::Process([[maybe_unused]] StatisticsPtr &stats,
> >> +                       [[maybe_unused]] Metadata &metadata)
> >> +{
> >> +}
> >> +
> >>  uint32_t CamHelper::ExposureLines(double exposure_us) const
> >>  {
> >>         assert(initialized_);
> >> @@ -139,6 +155,41 @@ unsigned int CamHelper::MistrustFramesModeSwitch() const
> >>         return 0;
> >>  }
> >>
> >> +void CamHelper::parseEmbeddedData(const Span<uint8_t> &buffer,
> >> +                                 Metadata &metadata)
> >> +{
> >> +       if (buffer.size()) {
> >> +               bool success = false;
> >> +               uint32_t exposureLines, gainCode;
> >> +
> >> +               parser_->SetBufferSize(buffer.size());
> >> +               success = parser_->Parse(buffer.data()) == MdParser::Status::OK &&
> >> +                         parser_->GetExposureLines(exposureLines) == MdParser::Status::OK &&
> >> +                         parser_->GetGainCode(gainCode) == MdParser::Status::OK;
> >> +
> >> +               /*
> >> +                * Overwrite the exposure/gain values in the DeviceStatus, as
> >> +                * we know better. Fetch it first in case any other fields were
> >> +                * set meaningfully.
> >> +                */
> >> +               struct DeviceStatus deviceStatus;
> >> +
> >> +               if (success &&
> >> +                   metadata.Get("device.status", deviceStatus) == 0) {
> >
> >
> > I'm trying to work out if the second clause is needed?  Shouldn't we write device.status
> > unconditionally, whether it is present in the metadata or not?  I suppose it does not matter,
> > it is guaranteed to be there from the IPA.
> >
> > Reviewed-by: Naushir Patuck <naush@raspberrypi.com>
> >
> >
> >>
> >> +                       deviceStatus.shutter_speed = Exposure(exposureLines);
> >> +                       deviceStatus.analogue_gain = Gain(gainCode);
> >> +
> >> +                       LOG(IPARPI, Debug) << "Metadata updated - Exposure : "
> >> +                                          << deviceStatus.shutter_speed
> >> +                                          << " Gain : "
> >> +                                          << deviceStatus.analogue_gain;
> >> +
> >> +                       metadata.Set("device.status", deviceStatus);
> >> +               } else
> >> +                       LOG(IPARPI, Error) << "Embedded buffer parsing failed";
> >> +       }
> >> +}
> >> +
> >>  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 1b2d6eec..930ea39a 100644
> >> --- a/src/ipa/raspberrypi/cam_helper.hpp
> >> +++ b/src/ipa/raspberrypi/cam_helper.hpp
> >> @@ -8,7 +8,11 @@
> >>
> >>  #include <string>
> >>
> >> +#include <libcamera/span.h>
> >> +
> >>  #include "camera_mode.h"
> >> +#include "controller/controller.hpp"
> >> +#include "controller/metadata.hpp"
> >>  #include "md_parser.hpp"
> >>
> >>  #include "libcamera/internal/v4l2_videodevice.h"
> >> @@ -65,7 +69,9 @@ public:
> >>         CamHelper(MdParser *parser, unsigned int frameIntegrationDiff);
> >>         virtual ~CamHelper();
> >>         void SetCameraMode(const CameraMode &mode);
> >> -       MdParser &Parser() const { return *parser_; }
> >> +       virtual void Prepare(const libcamera::Span<uint8_t> &buffer,
> >> +                            Metadata &metadata);
> >> +       virtual void Process(StatisticsPtr &stats, Metadata &metadata);
> >>         uint32_t ExposureLines(double exposure_us) const;
> >>         double Exposure(uint32_t exposure_lines) const; // in us
> >>         virtual uint32_t GetVBlanking(double &exposure_us, double minFrameDuration,
> >> @@ -81,6 +87,9 @@ public:
> >>         virtual unsigned int MistrustFramesModeSwitch() const;
> >>
> >>  protected:
> >> +       void parseEmbeddedData(const libcamera::Span<uint8_t> &buffer,
> >> +                              Metadata &metadata);
> >> +
> >>         MdParser *parser_;
> >>         CameraMode mode_;
> >>
> >> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> >> index 1c928b72..32ecbdcd 100644
> >> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> >> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> >> @@ -101,9 +101,7 @@ private:
> >>         void returnEmbeddedBuffer(unsigned int bufferId);
> >>         void prepareISP(const ipa::RPi::ISPConfig &data);
> >>         void reportMetadata();
> >> -       bool parseEmbeddedData(unsigned int bufferId, struct DeviceStatus &deviceStatus);
> >> -       void fillDeviceStatus(uint32_t exposureLines, uint32_t gainCode,
> >> -                             struct DeviceStatus &deviceStatus);
> >> +       void fillDeviceStatus(const ControlList &sensorControls);
> >>         void processStats(unsigned int bufferId);
> >>         void applyFrameDurations(double minFrameDuration, double maxFrameDuration);
> >>         void applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls);
> >> @@ -895,35 +893,34 @@ void IPARPi::returnEmbeddedBuffer(unsigned int bufferId)
> >>
> >>  void IPARPi::prepareISP(const ipa::RPi::ISPConfig &data)
> >>  {
> >> -       struct DeviceStatus deviceStatus = {};
> >> -       bool success = false;
> >> +       Span<uint8_t> embeddedBuffer;
> >> +
> >> +       rpiMetadata_.Clear();
> >> +
> >> +       fillDeviceStatus(data.controls);
> >>
> >>         if (data.embeddedBufferPresent) {
> >>                 /*
> >>                  * Pipeline handler has supplied us with an embedded data buffer,
> >> -                * so parse it and extract the exposure and gain.
> >> +                * we must pass it to the CamHelper for parsing.
> >>                  */
> >> -               success = parseEmbeddedData(data.embeddedBufferId, deviceStatus);
> >> -
> >> -               /* Done with embedded data now, return to pipeline handler asap. */
> >> -               returnEmbeddedBuffer(data.embeddedBufferId);
> >> +               auto it = buffers_.find(data.embeddedBufferId);
> >> +               ASSERT(it != buffers_.end());
> >> +               embeddedBuffer = it->second.maps()[0];
> >>         }
> >>
> >> -       if (!success) {
> >> -               /*
> >> -                * Pipeline handler has not supplied an embedded data buffer,
> >> -                * or embedded data buffer parsing has failed for some reason,
> >> -                * so pull the exposure and gain values from the control list.
> >> -                */
> >> -               int32_t exposureLines = data.controls.get(V4L2_CID_EXPOSURE).get<int32_t>();
> >> -               int32_t gainCode = data.controls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>();
> >> -               fillDeviceStatus(exposureLines, gainCode, deviceStatus);
> >> -       }
> >> +       /*
> >> +        * This may overwrite the DeviceStatus using values from the sensor
> >> +        * metadata, and may also do additional custom processing.
> >> +        */
> >> +       helper_->Prepare(embeddedBuffer, rpiMetadata_);
> >> +
> >> +       /* Done with embedded data now, return to pipeline handler asap. */
> >> +       if (data.embeddedBufferPresent)
> >> +               returnEmbeddedBuffer(data.embeddedBufferId);
> >>
> >>         ControlList ctrls(ispCtrls_);
> >>
> >> -       rpiMetadata_.Clear();
> >> -       rpiMetadata_.Set("device.status", deviceStatus);
> >>         controller_.Prepare(&rpiMetadata_);
> >>
> >>         /* Lock the metadata buffer to avoid constant locks/unlocks. */
> >> @@ -973,41 +970,13 @@ void IPARPi::prepareISP(const ipa::RPi::ISPConfig &data)
> >>                 setIspControls.emit(ctrls);
> >>  }
> >>
> >> -bool IPARPi::parseEmbeddedData(unsigned int bufferId, struct DeviceStatus &deviceStatus)
> >> +void IPARPi::fillDeviceStatus(const ControlList &sensorControls)
> >>  {
> >> -       auto it = buffers_.find(bufferId);
> >> -       if (it == buffers_.end()) {
> >> -               LOG(IPARPI, Error) << "Could not find embedded buffer!";
> >> -               return false;
> >> -       }
> >> -
> >> -       Span<uint8_t> mem = it->second.maps()[0];
> >> -       helper_->Parser().SetBufferSize(mem.size());
> >> -       RPiController::MdParser::Status status = helper_->Parser().Parse(mem.data());
> >> -       if (status != RPiController::MdParser::Status::OK) {
> >> -               LOG(IPARPI, Error) << "Embedded Buffer parsing failed, error " << status;
> >> -               return false;
> >> -       } else {
> >> -               uint32_t exposureLines, gainCode;
> >> -               if (helper_->Parser().GetExposureLines(exposureLines) != RPiController::MdParser::Status::OK) {
> >> -                       LOG(IPARPI, Error) << "Exposure time failed";
> >> -                       return false;
> >> -               }
> >> -
> >> -               if (helper_->Parser().GetGainCode(gainCode) != RPiController::MdParser::Status::OK) {
> >> -                       LOG(IPARPI, Error) << "Gain failed";
> >> -                       return false;
> >> -               }
> >> -
> >> -               fillDeviceStatus(exposureLines, gainCode, deviceStatus);
> >> -       }
> >> +       struct DeviceStatus deviceStatus = {};
> >>
> >> -       return true;
> >> -}
> >> +       int32_t exposureLines = sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>();
> >> +       int32_t gainCode = sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>();
> >>
> >> -void IPARPi::fillDeviceStatus(uint32_t exposureLines, uint32_t gainCode,
> >> -                             struct DeviceStatus &deviceStatus)
> >> -{
> >>         deviceStatus.shutter_speed = helper_->Exposure(exposureLines);
> >>         deviceStatus.analogue_gain = helper_->Gain(gainCode);
> >>
> >> @@ -1015,6 +984,8 @@ void IPARPi::fillDeviceStatus(uint32_t exposureLines, uint32_t gainCode,
> >>                            << deviceStatus.shutter_speed
> >>                            << " Gain : "
> >>                            << deviceStatus.analogue_gain;
> >> +
> >> +       rpiMetadata_.Set("device.status", deviceStatus);
> >>  }
> >>
> >>  void IPARPi::processStats(unsigned int bufferId)
> >> @@ -1028,6 +999,7 @@ void IPARPi::processStats(unsigned int bufferId)
> >>         Span<uint8_t> mem = it->second.maps()[0];
> >>         bcm2835_isp_stats *stats = reinterpret_cast<bcm2835_isp_stats *>(mem.data());
> >>         RPiController::StatisticsPtr statistics = std::make_shared<bcm2835_isp_stats>(*stats);
> >> +       helper_->Process(statistics, rpiMetadata_);
> >>         controller_.Process(statistics, &rpiMetadata_);
> >>
> >>         struct AgcStatus agcStatus;
> >> --
> >> 2.20.1
> >>
> >> _______________________________________________
> >> libcamera-devel mailing list
> >> libcamera-devel@lists.libcamera.org
> >> https://lists.libcamera.org/listinfo/libcamera-devel