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

[Kieran Bingham]

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.

> 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).

--

Kieran

> +       finishFrame(frame, bufferId);
> +       bench_.finishFrame();
> +}
> +
>  } /* namespace libcamera */
> -- 
> 2.47.0
>