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

[Hans de Goede]

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

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

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.


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

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

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 .

>> +	 */
>> +	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.

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

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

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.

Regards,

Hans