[PATCH resend 5/5] libcamera: swstats_cpu: Add processFrame() method

[Laurent Pinchart]

On Fri, Dec 06, 2024 at 01:14:35PM +0000, Kieran Bingham wrote:
> Quoting Hans de Goede (2024-12-05 19:25:19)
> > Add a method to the SwstatsCpu class to process a whole Framebuffer in
> > one go, rather then line by line. This is useful for gathering stats
> > when debayering is not necessary or is not done on the CPU.
> 
> This is the bit I can see will be useful for any scenario where we don't
> have stats. I think for instance on RKISP1 when running raw mode we
> don't get stats, so something like this would be helpful to be able to
> support some AEGC control which would likely be beneficial for tuning or
> capturing raw streams.

I'm in two minds about that. If it's only about tuning with ISPs that
can't generate statistics when capturing raw frames, an application-side
AE could be a better option. We should discuss it when the need arises
before writing code.

> > Reviewed-by: Milan Zamazal <mzamazal at redhat.com>
> > Signed-off-by: Hans de Goede <hdegoede at redhat.com>
> > ---
> > Changes since the RFC:
> > - Make processFrame() call startFrame() and finishFrame() rather then
> >   making the caller do this
> > - Add doxygen documentation for processFrame()
> > ---
> >  .../internal/software_isp/swstats_cpu.h       | 12 +++++
> >  src/libcamera/software_isp/swstats_cpu.cpp    | 51 +++++++++++++++++++
> >  2 files changed, 63 insertions(+)
> > 
> > diff --git a/include/libcamera/internal/software_isp/swstats_cpu.h b/include/libcamera/internal/software_isp/swstats_cpu.h
> > index 26a2f462..fa47cec9 100644
> > --- a/include/libcamera/internal/software_isp/swstats_cpu.h
> > +++ b/include/libcamera/internal/software_isp/swstats_cpu.h
> > @@ -18,12 +18,16 @@
> >  #include <libcamera/geometry.h>
> >  
> >  #include "libcamera/internal/bayer_format.h"
> > +#include "libcamera/internal/framebuffer.h"
> >  #include "libcamera/internal/shared_mem_object.h"
> >  #include "libcamera/internal/software_isp/swisp_stats.h"
> >  
> > +#include "benchmark.h"
> > +
> >  namespace libcamera {
> >  
> >  class PixelFormat;
> > +class MappedFrameBuffer;
> >  struct StreamConfiguration;
> >  
> >  class SwStatsCpu
> > @@ -42,6 +46,7 @@ public:
> >         void setWindow(const Rectangle &window);
> >         void startFrame();
> >         void finishFrame(uint32_t frame, uint32_t bufferId);
> > +       void processFrame(uint32_t frame, uint32_t bufferId, FrameBuffer *input);
> >  
> >         void processLine0(unsigned int y, const uint8_t *src[])
> >         {
> > @@ -65,6 +70,7 @@ public:
> >  
> >  private:
> >         using statsProcessFn = void (SwStatsCpu::*)(const uint8_t *src[]);
> > +       using processFrameFn = void (SwStatsCpu::*)(MappedFrameBuffer &in);
> >  
> >         int setupStandardBayerOrder(BayerFormat::Order order);
> >         /* Bayer 8 bpp unpacked */
> > @@ -77,6 +83,10 @@ private:
> >         void statsBGGR10PLine0(const uint8_t *src[]);
> >         void statsGBRG10PLine0(const uint8_t *src[]);
> >  
> > +       void processBayerFrame2(MappedFrameBuffer &in);
> 
> Ayeee ... I shudder a little seeing 'Thing2'.
> 
> Would this really just be processMappedBayerFrame() ?
> 
> > +
> > +       processFrameFn processFrame_;
> > +
> >         /* Variables set by configure(), used every line */
> >         statsProcessFn stats0_;
> >         statsProcessFn stats2_;
> > @@ -89,9 +99,11 @@ private:
> >         Size patternSize_;
> >  
> >         unsigned int xShift_;
> > +       unsigned int stride_;
> >  
> >         SharedMemObject<SwIspStats> sharedStats_;
> >         SwIspStats stats_;
> > +       Benchmark bench_;
> >  };
> >  
> >  } /* namespace libcamera */
> > diff --git a/src/libcamera/software_isp/swstats_cpu.cpp b/src/libcamera/software_isp/swstats_cpu.cpp
> > index aa5654dc..1ff15f5b 100644
> > --- a/src/libcamera/software_isp/swstats_cpu.cpp
> > +++ b/src/libcamera/software_isp/swstats_cpu.cpp
> > @@ -16,6 +16,7 @@
> >  #include <libcamera/stream.h>
> >  
> >  #include "libcamera/internal/bayer_format.h"
> > +#include "libcamera/internal/mapped_framebuffer.h"
> >  
> >  namespace libcamera {
> >  
> > @@ -360,11 +361,14 @@ int SwStatsCpu::setupStandardBayerOrder(BayerFormat::Order order)
> >   */
> >  int SwStatsCpu::configure(const StreamConfiguration &inputCfg)
> >  {
> > +       stride_ = inputCfg.stride;
> > +
> >         BayerFormat bayerFormat =
> >                 BayerFormat::fromPixelFormat(inputCfg.pixelFormat);
> >  
> >         if (bayerFormat.packing == BayerFormat::Packing::None &&
> >             setupStandardBayerOrder(bayerFormat.order) == 0) {
> > +               processFrame_ = &SwStatsCpu::processBayerFrame2;
> >                 switch (bayerFormat.bitDepth) {
> >                 case 8:
> >                         stats0_ = &SwStatsCpu::statsBGGR8Line0;
> > @@ -385,6 +389,7 @@ int SwStatsCpu::configure(const StreamConfiguration &inputCfg)
> >                 /* Skip every 3th and 4th line, sample every other 2x2 block */
> >                 ySkipMask_ = 0x02;
> >                 xShift_ = 0;
> > +               processFrame_ = &SwStatsCpu::processBayerFrame2;
> >  
> >                 switch (bayerFormat.order) {
> >                 case BayerFormat::BGGR:
> > @@ -425,4 +430,50 @@ void SwStatsCpu::setWindow(const Rectangle &window)
> >         window_.height &= ~(patternSize_.height - 1);
> >  }
> >  
> > +void SwStatsCpu::processBayerFrame2(MappedFrameBuffer &in)
> > +{
> > +       const uint8_t *src = in.planes()[0].data();
> > +       const uint8_t *linePointers[3];
> > +
> > +       /* Adjust src for starting at window_.y */
> > +       src += window_.y * stride_;
> > +
> > +       for (unsigned int y = 0; y < window_.height; y += 2) {
> > +               if (y & ySkipMask_) {
> > +                       src += stride_ * 2;
> > +                       continue;
> > +               }
> > +
> > +               /* linePointers[0] is not used by any stats0_ functions */
> > +               linePointers[1] = src;
> > +               linePointers[2] = src + stride_;
> > +               (this->*stats0_)(linePointers);
> > +               src += stride_ * 2;
> > +       }
> > +}
> > +
> > +/**
> > + * \brief Calculate statistics for a frame in one go
> > + * \param[in] frame The frame number
> > + * \param[in] bufferId ID of the statistics buffer
> > + * \param[in] input The frame to process
> > + *
> > + * This may only be called after a successful setWindow() call.
> > + */
> > +void SwStatsCpu::processFrame(uint32_t frame, uint32_t bufferId, FrameBuffer *input)
> > +{
> > +       bench_.startFrame();
> > +       startFrame();
> > +
> > +       MappedFrameBuffer in(input, MappedFrameBuffer::MapFlag::Read);
> > +       if (!in.isValid()) {
> > +               LOG(SwStatsCpu, Error) << "mmap-ing buffer(s) failed";
> > +               return;
> > +       }
> > +
> > +       (this->*processFrame_)(in);
> 
> I can't see why this goes through a function pointer at the moment. Is
> there some later patches that will have 'different' processFrame_
> implemenations?
> 
> Or could SwStatsCpu::processBayerFrame2 just be inlined here ?
> 
> (I'm fine keeping it separate if something else is about to use it, or
> change it soon).
> 
> > +       finishFrame(frame, bufferId);
> > +       bench_.finishFrame();
> > +}
> > +
> >  } /* namespace libcamera */

-- 
Regards,

Laurent Pinchart