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

22191 diff mbox series

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.

Reviewed-by: Milan Zamazal <mzamazal@redhat.com>
Signed-off-by: Hans de Goede <hdegoede@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(+)

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@redhat.com>
> Signed-off-by: Hans de Goede <hdegoede@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
>

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@redhat.com>
> > Signed-off-by: Hans de Goede <hdegoede@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 */

Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes:

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

Another use, discussed in one of the recent software ISP syncs, would be
to get statistics from CPU when doing debayering on a GPU.

And when all I need is getting raw frames, let's say from `simple'
pipeline for debugging purposes or something similar, getting them
reasonably exposed out of the box may be convenient.

>> > Reviewed-by: Milan Zamazal <mzamazal@redhat.com>
>> > Signed-off-by: Hans de Goede <hdegoede@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 */

On Fri, Dec 06, 2024 at 02:56:00PM +0100, Milan Zamazal wrote:
> Laurent Pinchart writes:
> > 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.
> 
> Another use, discussed in one of the recent software ISP syncs, would be
> to get statistics from CPU when doing debayering on a GPU.

That use case is an important one. I hope we'll eventualy get the GPU to
calculates stats too, but in the interim period the CPU is useful.

> And when all I need is getting raw frames, let's say from `simple'
> pipeline for debugging purposes or something similar, getting them
> reasonably exposed out of the box may be convenient.

In that case I'd expect the frames to go through the software ISP, and
if the user only wants raw frames, we would "configure" the software ISP
to only produce statistics, not processed frames.

> >> > Reviewed-by: Milan Zamazal <mzamazal@redhat.com>
> >> > Signed-off-by: Hans de Goede <hdegoede@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 */