[libcamera-devel] [PATCH v2 07/18] libcamera: software_isp: Add SwStats base class

[Jacopo Mondi]

Hi Hans

On Thu, Feb 08, 2024 at 05:13:58PM +0100, Hans de Goede wrote:
> Hi Jacopo,
>
> Thank you for the review.
>
> On 1/31/24 13:44, Jacopo Mondi wrote:
>
> <snip>
>
> >> new file mode 100644
> >> index 00000000..dcac7064
> >> --- /dev/null
> >> +++ b/include/libcamera/internal/software_isp/swstats.h
>
> <snip>
>
> >> +protected:
> >> +	/**
> >> +	 * \brief Called when there is data to get statistics from.
> >> +	 * \param[in] src The input data
> >> +	 */
> >> +	typedef void (SwStats::*statsProcessFn)(const uint8_t *src[]);
> >
> > I'll see how this is used later, but passing in an unbound pointer to
> > memory seems suspicious. Span<> can help maybe or maybe just passing in a
> > size ?
>
> The size is given by the rectanlge in window_ which is
> set by calling setWindow()
>

mkey, it's just that unbounded memory passed to a function looks not
great

> >> +	/**
> >> +	 * \brief Called when the statistics gathering is done or when a new frame starts.
> >> +	 */
> >> +	typedef void (SwStats::*statsVoidFn)();
> >> +
> >> +	/* Variables set by configure(), used every line */
> >> +	/**
> >> +	 * \brief The function called when a line is ready for statistics processing.
> >> +	 *
> >> +	 * Used for line 0 and 1, repeating if there isn't a 3rd and a 4th line in the bayer order.
> >> +	 */
> >> +	statsProcessFn stats0_;
> >> +	/**
> >> +	 * \brief The function called when a line is ready for statistics processing.
> >> +	 *
> >> +	 * Used for line 3 and 4, only needed if the bayer order has 4 different lines.
> >> +	 */
> >> +	statsProcessFn stats2_;
> >
> > Is the idea that derived classes shall provide these ? Isn't a pure
> > virtual function better ?
> >
> > ...
> >
> > Ok, I've now read the next patches, and you set stat0_ and stat2_
> > conditionally on the bitdepth
> >
> > 		switch (bayerFormat.bitDepth) {
> > 		case 8:
> > 			stats0_ = (SwStats::statsProcessFn)&SwStatsCpu::statsBGGR8Line0;
> > 			return 0;
> > 		case 10:
> > 			stats0_ = (SwStats::statsProcessFn)&SwStatsCpu::statsBGGR10Line0;
> > 			return 0;
> > 		}
> >
> > stats2_ seems not used in you implementation
> >
> > This makes me wonder if the 8 and 10 bits implementation shouldn't
> > actually be two dervied classes, each one implementing their own
> > stats0() functions. It's indeed more verbose though...
>
> The bit you found is just for the unpacked 8 and 10 bpp support,
> there is another function for 10bpp packed and v3 will add
> 12 bpp unpacked and I also have patches adding IGIG_GBGR_IGIG_GRGB
> support. This is found on the ov01a1s, which has the following
> bayer pattern (4x4 tile repeating):
>
> IGIG
> GBGR
> IGIG
> GRGB
>
> And I also plan to use swstats + the softIPA together with
> the atomisp, which has a working HW ISP, but we have no idea
> what the statistics mean so my plan so to use swstats
> for autogain + awb.
>
> This will mean running swstats after debayering, so on
> say RGB888 or YUV420, so yet more formats to support.
>
> And who knows what the future brings (e.g. packed 12bpp bayer?)
> adding a derived class for each new supported pixelformat
> seems like a lot of extra code / overhead.
>
> Which is why I went with old-school C-style function
> pointers for this.
>

I understand. I'm not a fanatic of OOP when old-school C-style methods
make the code more concise, and I don't have time resources to try
propose something different, so I guess what you have is good :)

> Note the IGIG_GBGR_IGIG_GRGB case is also what the
> stats2_ function pointer is for. stats0_ processes
> lines 0 and 1 of a 2 or 4 line block and stats2_
> processes lines 2 and 3 of a 4 line block.
>

Yep, thanks

>
> >> +	/**
> >> +	 * \brief The memory used per pixel in bits.
> >
> > Usually called 'bitdepth' in other parts of the library
>
> The way bitDepth is used in e.g. bayerFormat is different
> from this though.
>
> This is not the number of significant bits per pixel,
> this is the number of buts used in memory.

Thanks for the typo, made me laugh

>
> So for e.g. bayerFormat.bitDepth = 10, this is 16 for
> unpacked data (1 pixel per 16 bit memory word) and
> 10 for packed data (4 pixels per 5 bytes of memory).

Oh, I see, so it's not the size on the wire, but the size in memory
once unpacked.

>
> I'm open to a better name then bpp and also I'm open
> to improve the brief, since I was hoping that the brief
> explained (already) how this is different from e.g.
> bayerFormat.bitDepth .
>

Stupid question, will this ever be != 16 ?

You can expand the description by saying this value represents the
size of the memory word where one pixel sample is stored in and
explicitly state this is different from the raw image bitdepth (the
bit size of the sample).

Quoting the v4l2 documentation for the memory RAW formats for
more inspiration:

        These four pixel formats are raw sRGB / Bayer formats with 10
        bits per sample. Each sample is stored in a 16-bit word, with
        6 unused high bits filled with zeros. Each n-pixel row
        contains n/2 green samples and n/2 blue or red samples, with
        alternating red and blue rows. Bytes are stored in memory in
        little endian order. They are conventionally described as
        GRGR... BGBG..., RGRG... GBGB..., etc. Below is an example of
        one of these formats:



> >> +	 */
> >> +	unsigned int bpp_;
> >> +	/**
> >> +	 * \brief Skip lines where this bitmask is set in y.
> >> +	 */
> >> +	unsigned int y_skip_mask_;
> >> +
> >> +	/**
> >> +	 * \brief Statistics window, set by setWindow(), used ever line.
> >> +	 */
> >> +	Rectangle window_;
> >> +
> >> +	/**
> >> +	 * \brief The function called at the start of a frame.
> >> +	 */
> >> +	statsVoidFn startFrame_;
> >> +	/**
> >> +	 * \brief The function called at the end of a frame.
> >> +	 */
> >> +	statsVoidFn finishFrame_;
> >> +	/**
> >> +	 * \brief The size of the bayer pattern.
> >> +	 */
> >> +	Size patternSize_;
> >
> > Do you have a use case already for pattern sizes larger than the usual
> > 2x2 ? Mostly out of curiosity
>
> Yes, the IGIG_GBGR_IGIG_GRGB pattern mentioned above.
> I already have this working, but I first need to upstream
> the kernel support for the ov01a1s (and finalize the
> V4L2_PIXFMT for it) before this can be upstreamed into
> libcamera.

nice!

>
> <snip>
>
>
> >> +	/**
> >> +	 * \brief Specify window coordinates over which to gather statistics.
> >> +	 * \param[in] window The window object.
> >> +	 */
> >> +	void setWindow(Rectangle window)
> >> +	{
> >> +		window_ = window;
> >> +
> >> +		window_.x &= ~(patternSize_.width - 1);
> >> +		window_.x += x_shift_;
> >> +		window_.y &= ~(patternSize_.height - 1);
> >> +
> >> +		/* width_ - x_shift_ to make sure the window fits */
> >> +		window_.width -= x_shift_;
> >> +		window_.width &= ~(patternSize_.width - 1);
> >> +		window_.height &= ~(patternSize_.height - 1);
> >> +	}
> >
> > The usage of x_shift (in conjunction with swap_lines (btw, we use
> > camelCase and not snake_case)) seems to depend on the fact the SwStatCpu
> > implementation uses a single function to deal with the 4 bayer
> > permutations. A different implementation might not use the same. I
> > wonder if all of this shouldn't be moved to SwStatCpu..
>
> Ack, Laurent suggested just dropping the base class altogether and
> only have a SwStatsCpu class for now. OTOH Bryan mentioned that
> he wanted to move more stuff into the base-class to share it
> for his GLES work.
>
> So my plan for now is to leave the split as is and figure this out
> once we also have some GLES POC / MVP code.
>

Makes sense, let's keep this in mind!

> <snip>
>
> >> +	/**
> >> +	 * \brief Finish statistics calculation for the current frame.
> >> +	 *
> >> +	 * This may only be called after a successful setWindow() call.
> >> +	 */
> >> +	void finishFrame()
> >> +	{
> >> +		(this->*finishFrame_)();
> >> +	}
> >
> > This seems like a potential candidate for a pure virtual function ?
>
> Ack and the same for startFrame().
>
> <snip>
>
> >> diff --git a/src/libcamera/software_isp/swstats.cpp b/src/libcamera/software_isp/swstats.cpp
> >> new file mode 100644
> >> index 00000000..e65a7ada
> >> --- /dev/null
> >> +++ b/src/libcamera/software_isp/swstats.cpp
> >> @@ -0,0 +1,22 @@
> >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> >> +/*
> >> + * Copyright (C) 2023, Linaro Ltd
> >> + * Copyright (C) 2023, Red Hat Inc.
> >> + *
> >> + * Authors:
> >> + * Hans de Goede <hdegoede at redhat.com>
> >> + *
> >> + * swstats.cpp - software statistics base class
> >> + */
> >> +
> >> +#include "libcamera/internal/software_isp/swstats.h"
> >> +
> >> +namespace libcamera {
> >> +
> >> +LOG_DEFINE_CATEGORY(SwStats)
> >> +
> >> +SwStats::~SwStats()
> >> +{
> >> +}
> >
> > You'll need a .cpp file anyway for comments, but why having a lot of
> > inline code in the header (which I would move to the .cpp file, as it
> > doesn't depend on any template resolution) and have the empty
> > constructor here ?
>
> Note this is an empty destructor not constructor and
> the destructor is declared virtual (pure virtual even)
> in the .h file.

yeah sorry, typo

>
> So I did not expect to have to implement a destructor in the base-class
> at all, but even with it being pure-virtual the derived SwStatsCpu class
> destructor will chain up to the base-class destructor leading to a
> linker error about that missing from the vtable for the base class
> unless I add this.
>
> This might just be me being inexperienced with c++, so if there
> is another way to solve this suggestions are welcome.
>

My point was that most if not all functions implementation should go
the .cpp file and not in the header. But maybe I missed something ?

> Regards,
>
> Hans
>
>