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

[Naushir Patuck]

Hi Kieran and David,

On Mon, 8 Jan 2024 at 17:42, Kieran Bingham
<kieran.bingham at ideasonboard.com> wrote:
>
> 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));

No objections to doing that.

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

Thanks!
Naush

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