[libcamera-devel,RFC,1/1] ipa: raspberrypi: Use CamHelpers to generalise embedded data parsing

11546 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 no longer performs this register parsing, it just has
to call the new Prepare() and Process() methods.
---
 src/ipa/raspberrypi/cam_helper.cpp  | 49 ++++++++++++++++
 src/ipa/raspberrypi/cam_helper.hpp  | 14 ++++-
 src/ipa/raspberrypi/raspberrypi.cpp | 88 ++++++++---------------------
 3 files changed, 84 insertions(+), 67 deletions(-)

Hi David,

Thank you for your work.

On Wed, 10 Mar 2021 at 17:23, 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 no longer performs this register parsing, it just has
> to call the new Prepare() and Process() methods.
> ---
>  src/ipa/raspberrypi/cam_helper.cpp  | 49 ++++++++++++++++
>  src/ipa/raspberrypi/cam_helper.hpp  | 14 ++++-
>  src/ipa/raspberrypi/raspberrypi.cpp | 88 ++++++++---------------------
>  3 files changed, 84 insertions(+), 67 deletions(-)
>
> diff --git a/src/ipa/raspberrypi/cam_helper.cpp
> b/src/ipa/raspberrypi/cam_helper.cpp
> index 0ae0baa0..fc69f4cb 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)
> +}
>

Is this namespace enclosure needed for some reason?
Seems to compile well enough without it.  I wonder if we should add a
category for cam_helper based logging?


>
>  static std::map<std::string, CamHelperCreateFunc> cam_helpers;
>
> @@ -45,6 +50,17 @@ CamHelper::~CamHelper()
>         delete parser_;
>  }
>
> +void CamHelper::Prepare(const Span<uint8_t> &buffer,
> +                       const ControlList &controls, Metadata &metadata)
> +{
> +       parseRegisters(buffer, controls, 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,39 @@ unsigned int CamHelper::MistrustFramesModeSwitch()
> const
>         return 0;
>  }
>
> +void CamHelper::parseRegisters(const Span<uint8_t> &buffer,
> +                              const ControlList &controls, Metadata
> &metadata)
> +{
>

Bit of a nit-pick (sorry) , but could you consider a few renames:

s/parseRegisters/parseEmbeddedData/
s/controls/sensorControls/
s/metadata/controllerMetadata/

Took me a bit of inspection to remind myself what those variables
represented.
Not too fussed either way though.

+       struct DeviceStatus deviceStatus = {};
> +       bool success = false;
> +       uint32_t exposureLines, gainCode;
> +
> +       if (buffer.size()) {
> +               parser_->SetBufferSize(buffer.size());
> +               success = parser_->Parse(buffer.data()) ==
> MdParser::Status::OK &&
> +                         parser_->GetExposureLines(exposureLines) ==
> MdParser::Status::OK &&
> +                         parser_->GetGainCode(gainCode) ==
> MdParser::Status::OK;
> +
> +               if (!success)
> +                       LOG(IPARPI, Error) << "Embedded buffer parsing
> failed";
> +       }
> +
> +       if (!success) {
> +               exposureLines =
> controls.get(V4L2_CID_EXPOSURE).get<int32_t>();
> +               gainCode =
> controls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>();
> +       }
> +
> +       deviceStatus.shutter_speed = Exposure(exposureLines);
> +       deviceStatus.analogue_gain = Gain(gainCode);
> +
> +       LOG(IPARPI, Debug) << "Metadata - Exposure : "
> +                          << deviceStatus.shutter_speed
> +                          << " Gain : "
> +                          << deviceStatus.analogue_gain;
> +
> +       metadata.Set("device.status", deviceStatus);
> +}
> +
>  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..ce5b82d2 100644
> --- a/src/ipa/raspberrypi/cam_helper.hpp
> +++ b/src/ipa/raspberrypi/cam_helper.hpp
> @@ -8,8 +8,13 @@
>
>  #include <string>
>
> +#include <libcamera/controls.h>
> +#include <libcamera/span.h>
> +
>  #include "camera_mode.h"
> +#include "controller/controller.hpp"
>  #include "md_parser.hpp"
> +#include "controller/metadata.hpp"
>

This will need to be ordered alphabetically.


>
>  #include "libcamera/internal/v4l2_videodevice.h"
>
> @@ -65,7 +70,10 @@ 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,
> +                            const libcamera::ControlList &controls,
> +                            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 +89,10 @@ public:
>         virtual unsigned int MistrustFramesModeSwitch() const;
>
>  protected:
> +       void parseRegisters(const libcamera::Span<uint8_t> &buffer,
> +                           const libcamera::ControlList &controls,
> +                           Metadata &metadata);
> +
>         MdParser *parser_;
>         CameraMode mode_;
>
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp
> b/src/ipa/raspberrypi/raspberrypi.cpp
> index 7904225a..d699540a 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -103,9 +103,6 @@ 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 processStats(unsigned int bufferId);
>         void applyFrameDurations(double minFrameDuration, double
> maxFrameDuration);
>         void applyAGC(const struct AgcStatus *agcStatus, ControlList
> &ctrls);
> @@ -913,35 +910,37 @@ void IPARPi::returnEmbeddedBuffer(unsigned int
> bufferId)
>
>  void IPARPi::prepareISP(const ipa::RPi::ISPConfig &data)
>  {
> -       struct DeviceStatus deviceStatus = {};
> -       bool success = false;
> +       Span<uint8_t> embeddedBuffer;
> +       bool returnEmbedded = false;
> +
> +       rpiMetadata_.Clear();
>
>         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);
> +               if (it == buffers_.end())
> +                       LOG(IPARPI, Error) << "Could not find embedded
> buffer!";
>

This really should never happen, perhaps an assert in its place?


> +               else {
> +                       embeddedBuffer = it->second.maps()[0];
> +                       returnEmbedded = true;
> +               }
>         }
>
> -       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 will add the DeviceStatus to the metadata, and depending
> on the
> +        * sensor, may do additional custom processing.
> +        */
> +       helper_->Prepare(embeddedBuffer, data.controls, rpiMetadata_);
> +
> +       /* Done with embedded data now, return to pipeline handler asap. */
> +       if (returnEmbedded)
> +               returnEmbeddedBuffer(data.embeddedBufferId);
>

I think this should happen unconditionally - but as before, returnEmbedded
should never
be false.   If the earlier code was changed to an assert, you could
remove returnEmbedded.

Regards,
Naush



>         ControlList ctrls(ispCtrls_);
>
> -       rpiMetadata_.Clear();
> -       rpiMetadata_.Set("device.status", deviceStatus);
>         controller_.Prepare(&rpiMetadata_);
>
>         /* Lock the metadata buffer to avoid constant locks/unlocks. */
> @@ -991,50 +990,6 @@ void IPARPi::prepareISP(const ipa::RPi::ISPConfig
> &data)
>                 setIspControls.emit(ctrls);
>  }
>
> -bool IPARPi::parseEmbeddedData(unsigned int bufferId, struct DeviceStatus
> &deviceStatus)
> -{
> -       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);
> -       }
> -
> -       return true;
> -}
> -
> -void IPARPi::fillDeviceStatus(uint32_t exposureLines, uint32_t gainCode,
> -                             struct DeviceStatus &deviceStatus)
> -{
> -       deviceStatus.shutter_speed = helper_->Exposure(exposureLines);
> -       deviceStatus.analogue_gain = helper_->Gain(gainCode);
> -
> -       LOG(IPARPI, Debug) << "Metadata - Exposure : "
> -                          << deviceStatus.shutter_speed
> -                          << " Gain : "
> -                          << deviceStatus.analogue_gain;
> -}
> -
>  void IPARPi::processStats(unsigned int bufferId)
>  {
>         auto it = buffers_.find(bufferId);
> @@ -1046,6 +1001,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
>