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

[Hans de Goede]

Hi Kieran,

I realize this series / your comments are quite old now,
but I finally have some time to rebase this and work on
it more.

On 6-Dec-24 2:14 PM, 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.
> 
>> 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() ?

The "2" refers to the number of lines in the bayer pattern
before it repeats itself. There are also some RGB+Ir combined
sensors where the pattern repeats every 4 lines, these will
get a processBayerFrame4() function.

And likewise there also will be separate process frame functions
for planar YUV vs packed YUV/RGB.

...

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

...

>> +/**
>> + * \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?

Yes, as mentioned above there will be separate processFrame_ functions
for planar YUV + packed YUV/RGB. The full atomisp patch-series this
is part of adds a processYUV420Frame() function and configure()
will set this pointer to one or the other depending on the pixelfmt.

So I plan to keep this as is for the new atomisp pipelinehandler
series which I hope to post soon.

Regards,

Hans