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

12197 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>
Reviewed-by: Naushir Patuck <naush@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,

Thanks for your work on this

Am Mi., 5. Mai 2021 um 16:04 Uhr schrieb David Plowman
<david.plowman@raspberrypi.com>:
>
> 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>
> Reviewed-by: Naushir Patuck <naush@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()) {

You can use `Span::empty()` to check if the span is... empty ;-)

Since the function can't do much without some data, I would return
immediately in that case
and save a level of indentation:

    if (buffer.empty())
        return;

    // do real work

> +               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;
> +

I would probably check `success` right here and return early if it
wasn't successful

> +               /*
> +                * 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;

The `struct` isn't needed. And just for good measure, I would
zero-initialize it with `= {}`

> +
> +               if (success &&
> +                   metadata.Get("device.status", deviceStatus) == 0) {
> +                       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";

If we check `success` right after the parsing step and bail out if it
wasn't successful, this could be written
like:

    if (metadata.Get("device.status", deviceStatus) != 0) {
        LOG(IPARPI, Error) << "Could not get device status from metadata";
        return;
    }

    deviceStatus.shutter_speed = Exposure(exposureLines);
    deviceStatus.analogue_gain = Gain(gainCode);
    [...]

I guess it's good to have a different error message for the
`metadata.Get("device.status", deviceStatus) != 0` case
anyway, since it's not really related to the parsing of the embedded data?

> +       }
> +}
> +
>  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 c3ed5362..6ee5051c 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);

The CppCoreGuidelines suggest passing spans by value:

    Note: A span<T> object does not own its elements and is so small
that it can be passed by value.
    Passing a span object as an argument is exactly as efficient as
passing a pair of pointer arguments
    or passing a pointer and an integer count.

https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#f24-use-a-spant-or-a-span_pt-to-designate-a-half-open-sequence

So this could be `libcamera::Span<const uint8_t> buffer` since the
function just needs read-only access to the
buffer data. This would require changing the signature of
`MdParser::Parse()` though. Either to `const void *` or
better yet `libcamera::Span<const uint8_t>` as well. Which would allow
removing the `static_cast<uint8_t *>(data)`
inside the `MdParser::Parse()` implementations.

> +       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 dad6395f..b0f61d35 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);
> @@ -894,35 +892,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;

This can be `Span<const uint8_t>`

> +
> +       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. */
> @@ -972,41 +969,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 = {};

`struct` can be removed here as well

>
> -       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);
>
> @@ -1014,6 +983,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)
> @@ -1027,6 +998,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 Marvin

Thanks for the feedback!

On Thu, 6 May 2021 at 18:18, Marvin Schmidt
<marvin.schmidt1987@gmail.com> wrote:
>
> Hi David,
>
> Thanks for your work on this
>
> Am Mi., 5. Mai 2021 um 16:04 Uhr schrieb David Plowman
> <david.plowman@raspberrypi.com>:
> >
> > 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>
> > Reviewed-by: Naushir Patuck <naush@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()) {
>
> You can use `Span::empty()` to check if the span is... empty ;-)

Yes, thanks, that's nicer!

>
> Since the function can't do much without some data, I would return
> immediately in that case
> and save a level of indentation:
>
>     if (buffer.empty())
>         return;
>
>     // do real work

Sure, I think that makes sense too.

>
> > +               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;
> > +
>
> I would probably check `success` right here and return early if it
> wasn't successful
>
> > +               /*
> > +                * 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;
>
> The `struct` isn't needed. And just for good measure, I would
> zero-initialize it with `= {}`

I tend to put "struct" in to remind myself that something is a vanilla
C data type. But I agree, I can just as easily take it out. I'm
slightly disinclined to zero-initialise things when it's not necessary
- if you do it all the time then it can cost actual CPU cycles. But I
don't particularly mind if there's a general preference for
zero-initialising stuff...

>
> > +
> > +               if (success &&
> > +                   metadata.Get("device.status", deviceStatus) == 0) {
> > +                       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";
>
> If we check `success` right after the parsing step and bail out if it
> wasn't successful, this could be written
> like:
>
>     if (metadata.Get("device.status", deviceStatus) != 0) {
>         LOG(IPARPI, Error) << "Could not get device status from metadata";
>         return;
>     }
>
>     deviceStatus.shutter_speed = Exposure(exposureLines);
>     deviceStatus.analogue_gain = Gain(gainCode);
>     [...]
>
> I guess it's good to have a different error message for the
> `metadata.Get("device.status", deviceStatus) != 0` case
> anyway, since it's not really related to the parsing of the embedded data?
>
> > +       }
> > +}
> > +
> >  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 c3ed5362..6ee5051c 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);
>
> The CppCoreGuidelines suggest passing spans by value:
>
>     Note: A span<T> object does not own its elements and is so small
> that it can be passed by value.
>     Passing a span object as an argument is exactly as efficient as
> passing a pair of pointer arguments
>     or passing a pointer and an integer count.
>
> https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#f24-use-a-spant-or-a-span_pt-to-designate-a-half-open-sequence

Thanks, I hadn't read that. I guess it doesn't formally tell us about
the merits of passing a Span by reference vs. passing it by value, but
it's tiny so hardly matters. Looking through libcamera, I would say
they're mostly passed by value so I can copy that pattern too.

Anyway, thanks for all those suggestions. I'll make those various
changes and submit another version.

Best regards
David

>
> So this could be `libcamera::Span<const uint8_t> buffer` since the
> function just needs read-only access to the
> buffer data. This would require changing the signature of
> `MdParser::Parse()` though. Either to `const void *` or
> better yet `libcamera::Span<const uint8_t>` as well. Which would allow
> removing the `static_cast<uint8_t *>(data)`
> inside the `MdParser::Parse()` implementations.
>
> > +       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 dad6395f..b0f61d35 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);
> > @@ -894,35 +892,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;
>
> This can be `Span<const uint8_t>`
>
> > +
> > +       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. */
> > @@ -972,41 +969,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 = {};
>
> `struct` can be removed here as well
>
> >
> > -       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);
> >
> > @@ -1014,6 +983,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)
> > @@ -1027,6 +998,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