[libcamera-devel] [PATCH v1 2/2] ipa: vc4: Implement the StatsOutputEnable vendor control

[Kieran Bingham]

Quoting David Plowman via libcamera-devel (2023-12-06 17:00:11)
> Hi Naush
> 
> Thanks for this. Looks good!
> 
> On Mon, 4 Dec 2023 at 16:18, Naushir Patuck via libcamera-devel
> <libcamera-devel at lists.libcamera.org> wrote:
> >
> > Implement the StatsOutputEnable control for the VC4 IPA. When set,
> > this outputs the ISP statistics as a uint8_t span through the Bcm2835StatsOutput
> > metadata control.
> >
> > To get this working, IpaBase::libcameraMetadata_ is moved from a private
> > to a protected member variable. This makes it accessable to the VC4
> > derived IPA class.
> >
> > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > ---
> >  src/ipa/rpi/common/ipa_base.cpp | 12 +++++++++---
> >  src/ipa/rpi/common/ipa_base.h   |  3 ++-
> >  src/ipa/rpi/vc4/vc4.cpp         |  6 ++++++
> >  3 files changed, 17 insertions(+), 4 deletions(-)
> >
> > diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp
> > index 6ac9d5de2f88..c4d17a2130c8 100644
> > --- a/src/ipa/rpi/common/ipa_base.cpp
> > +++ b/src/ipa/rpi/common/ipa_base.cpp
> > @@ -70,7 +70,8 @@ const ControlInfoMap::Map ipaControls{
> >         { &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },
> >         { &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },
> >         { &controls::FrameDurationLimits, ControlInfo(INT64_C(33333), INT64_C(120000)) },
> > -       { &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) }
> > +       { &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) },
> > +       { &controls::rpi::StatsOutputEnable, ControlInfo(false, true) },
> >  };
> >
> >  /* IPA controls handled conditionally, if the sensor is not mono */
> > @@ -100,8 +101,9 @@ LOG_DEFINE_CATEGORY(IPARPI)
> >  namespace ipa::RPi {
> >
> >  IpaBase::IpaBase()
> > -       : controller_(), frameLengths_(FrameLengthsQueueSize, 0s), frameCount_(0),
> > -         mistrustCount_(0), lastRunTimestamp_(0), firstStart_(true), flickerState_({ 0, 0s })
> > +       : controller_(), frameLengths_(FrameLengthsQueueSize, 0s), statsMetadataOutput_(false),
> > +         frameCount_(0), mistrustCount_(0), lastRunTimestamp_(0), firstStart_(true),
> > +         flickerState_({ 0, 0s })
> >  {
> >  }
> >
> > @@ -1135,6 +1137,10 @@ void IpaBase::applyControls(const ControlList &controls)
> >                         break;
> >                 }
> >
> > +               case controls::rpi::STATS_OUTPUT_ENABLE:
> > +                       statsMetadataOutput_ = ctrl.second.get<bool>();
> > +                       break;
> > +
> >                 default:
> >                         LOG(IPARPI, Warning)
> >                                 << "Ctrl " << controls::controls.at(ctrl.first)->name()
> > diff --git a/src/ipa/rpi/common/ipa_base.h b/src/ipa/rpi/common/ipa_base.h
> > index eaa9f71182ed..4db4411eed7c 100644
> > --- a/src/ipa/rpi/common/ipa_base.h
> > +++ b/src/ipa/rpi/common/ipa_base.h
> > @@ -61,6 +61,8 @@ protected:
> >         /* Track the frame length times over FrameLengthsQueueSize frames. */
> >         std::deque<utils::Duration> frameLengths_;
> >         utils::Duration lastTimeout_;
> > +       ControlList libcameraMetadata_;
> > +       bool statsMetadataOutput_;
> >
> >  private:
> >         /* Number of metadata objects available in the context list. */
> > @@ -89,7 +91,6 @@ private:
> >
> >         bool lensPresent_;
> >         bool monoSensor_;
> > -       ControlList libcameraMetadata_;
> >
> >         std::array<RPiController::Metadata, numMetadataContexts> rpiMetadata_;
> >
> > diff --git a/src/ipa/rpi/vc4/vc4.cpp b/src/ipa/rpi/vc4/vc4.cpp
> > index c165a5b8b0b6..ccd4141943dd 100644
> > --- a/src/ipa/rpi/vc4/vc4.cpp
> > +++ b/src/ipa/rpi/vc4/vc4.cpp
> > @@ -11,6 +11,7 @@
> >  #include <linux/bcm2835-isp.h>
> >
> >  #include <libcamera/base/log.h>
> > +#include <libcamera/base/span.h>
> >  #include <libcamera/control_ids.h>
> >  #include <libcamera/ipa/ipa_module_info.h>
> >
> > @@ -245,6 +246,11 @@ RPiController::StatisticsPtr IpaVc4::platformProcessStats(Span<uint8_t> mem)
> >                                                   stats->focus_stats[i].contrast_val_num[1][1],
> >                                                   stats->focus_stats[i].contrast_val_num[1][0] });
> >
> > +       if (statsMetadataOutput_) {
> > +               Span<uint8_t> statsSpan((uint8_t *)(stats), sizeof(bcm2835_isp_stats));
> 
> I thought folks preferred C++ style casts these days, but honestly...
> I really don't care!!
> 
> Reviewed-by: David Plowman <david.plowman at raspberrypi.com>

Yes, any objection to changing this to the following?:

		Span<uint8_t> statsSpan(static_cast<uint8_t *>(stats),
					sizeOf(bcm2835_isp_stats));

And then:


Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

> 
> Thanks!
> David
> 
> > +               libcameraMetadata_.set(controls::rpi::Bcm2835StatsOutput, statsSpan);
> > +       }
> > +
> >         return statistics;
> >  }
> >
> > --
> > 2.34.1
> >