[PATCH resend 5/5] libcamera: swstats_cpu: Add processFrame() method
Milan Zamazal
mzamazal at redhat.com
Fri Dec 6 14:56:00 CET 2024
Laurent Pinchart <laurent.pinchart at 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 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 */
More information about the libcamera-devel
mailing list