[libcamera-devel] [PATCH v2 08/18] libcamera: software_isp: Add SwStatsCpu class
Hans de Goede
hdegoede at redhat.com
Thu Feb 8 18:18:09 CET 2024
Hi Laurent,
Thank you for the review.
On 1/23/24 18:16, Laurent Pinchart wrote:
<snip>
>> +#define SWISP_LINARO_START_LINE_STATS(pixel_t) \
>> + pixel_t r, g, g2, b; \
>> + unsigned int y_val; \
>> + \
>> + unsigned int sumR = 0; \
>> + unsigned int sumG = 0; \
>> + unsigned int sumB = 0;
>> +
>> +#define SWISP_LINARO_ACCUMULATE_LINE_STATS(div) \
>> + sumR += r; \
>> + sumG += g; \
>> + sumB += b; \
>> + \
>> + y_val = r * RED_Y_MUL; \
>> + y_val += g * GREEN_Y_MUL; \
>> + y_val += b * BLUE_Y_MUL; \
>> + stats_.y_histogram[y_val / (256 * 16 * (div))]++;
>> +
>> +#define SWISP_LINARO_FINISH_LINE_STATS() \
>> + stats_.sumR_ += sumR; \
>> + stats_.sumG_ += sumG; \
>> + stats_.sumB_ += sumB;
>
> As these macros are only used in a single location, wouldn't the code be
> more readable if you just inlined them ?
These get used to avoid copy and pasting the above
over and over when added 8, 10 and 12bpp unpacked
bayer data format in a later patch. This initial patch
only adds 10bpp packed support. Since that is what
both the Qualcomm and IPU6 initial test-cases used.
> [[gnu::always_inline]]
> static inline void statsBayer10P(const int width, const uint8_t *src0,
> const uint8_t *src1, bool bggr,
> SwIspStats &stats_)
>
>> +{
>> + const int width_in_bytes = width * 5 / 4;
>> +
>> + SWISP_LINARO_START_LINE_STATS(uint8_t)
>> +
>> + for (int x = 0; x < width_in_bytes; x += 5) {
>> + if (bggr) {
>> + /* BGGR */
>> + b = src0[x];
>> + g = src0[x + 1];
>> + g2 = src1[x];
>> + r = src1[x + 1];
>> + } else {
>> + /* GBRG */
>> + g = src0[x];
>> + b = src0[x + 1];
>> + r = src1[x];
>> + g2 = src1[x + 1];
>> + }
>> + g = (g + g2) / 2;
>> +
>> + SWISP_LINARO_ACCUMULATE_LINE_STATS(1)
>> + }
>> +
>> + SWISP_LINARO_FINISH_LINE_STATS()
>> +}
>> +
>> +void SwStatsCpu::statsBGGR10PLine0(const uint8_t *src[])
>> +{
>> + const uint8_t *src0 = src[1] + window_.x * 5 / 4;
>> + const uint8_t *src1 = src[2] + window_.x * 5 / 4;
>> +
>> + if (swap_lines_)
>> + std::swap(src0, src1);
>> +
>> + statsBayer10P(window_.width, src0, src1, true, stats_);
>> +}
>> +
>> +void SwStatsCpu::statsGBRG10PLine0(const uint8_t *src[])
>> +{
>> + const uint8_t *src0 = src[1] + window_.x * 5 / 4;
>> + const uint8_t *src1 = src[2] + window_.x * 5 / 4;
>> +
>> + if (swap_lines_)
>> + std::swap(src0, src1);
>> +
>> + statsBayer10P(window_.width, src0, src1, false, stats_);
>> +}
>
> These two functions are identical except for the bggr argument they pass
> to statsBayer10P(). Can't we store the bayer order in a member variable
> and just use statsBayer10P() directly ?
At first these were 2 different functions with the loop from
statsBayer10P() copies in both functions and then either
the if or the else part of the if (bggr) check in statsBayer10P()
inside the loop.
Pavel suggested adding statsBayer10P() with the parameter
(and always inline it) for some code-reduction.
By having bggr be a const value passed in these 2 wrappers
we allow the compiler to optimize away the if (bggr).
If we replace the if (bggr) where bggr is a const value passed by
the wrappers, with some check based on data stored in the object
we add an extra if *per pixel* and modern pipelined CPUs suffer
greatly from this.
Note when Pavel made this change he also dropped the macros
because back then we still had only 10bpp packed support so
this change reduced the users of the macros to only a single
caller as you already pointed out.
With the macros (which we need to avoid duplication when adding
more formats) the loop is not that big.
I just tried moving the loop back inside statsBGGR10PLine0 /
statsGBRG10PLine0 with the if (bggr) removed and that actually
does not change the line count.
This also make the code more readable and more like the unpacked
bayer fmt support added later (and this also gets rid of the always
inline), so I will go with just removing the statsBayer10P() helper
for v3.
>> +
>> +void SwStatsCpu::resetStats(void)
>> +{
>> + stats_.sumR_ = 0;
>> + stats_.sumB_ = 0;
>> + stats_.sumG_ = 0;
>> + std::fill_n(stats_.y_histogram, 16, 0);
>> +}
>> +
>> +void SwStatsCpu::finishStats(void)
>> +{
>> + *sharedStats_ = stats_;
>> + statsReady.emit(0);
>
> If you always emit this signal with 0, you don't have to give it an
> argument.
The Signal<> template requires a dummy argument in this case,
I just tried:
--- a/src/libcamera/software_isp/swstats.h
+++ b/src/libcamera/software_isp/swstats.h
@@ -213,7 +213,7 @@ public:
*
* The int parameter isn't actually used.
*/
- Signal<int> statsReady;
+ Signal<void> statsReady;
};
} /* namespace libcamera */
And this leads to:
In file included from ../src/libcamera/software_isp/swstats.h:17,
from ../src/libcamera/software_isp/swstats.cpp:12:
../include/libcamera/base/signal.h: In instantiation of ‘class libcamera::Signal<void>’:
../src/libcamera/software_isp/swstats.h:216:15: required from here
../include/libcamera/base/signal.h:49:14: error: invalid parameter type ‘void’
49 | void connect(T *obj, R (T::*func)(Args...),
| ^~~~~~~
../include/libcamera/base/signal.h:49:14: error: in declaration ‘void libcamera::Signal<Args>::connect(T*, R (T::*)(Args ...), libcamera::ConnectionType)’
../include/libcamera/base/signal.h:60:14: error: invalid parameter type ‘void’
60 | void connect(T *obj, R (T::*func)(Args...))
| ^~~~~~~
../include/libcamera/base/signal.h:60:14: error: in declaration ‘void libcamera::Signal<Args>::connect(T*, R (T::*)(Args ...))’
../include/libcamera/base/signal.h:93:14: error: invalid parameter type ‘void’
93 | void connect(R (*func)(Args...))
| ^~~~~~~
../include/libcamera/base/signal.h:93:14: error: in declaration ‘void libcamera::Signal<Args>::connect(R (*)(Args ...))’
../include/libcamera/base/signal.h:114:14: error: invalid parameter type ‘void’
114 | void disconnect(T *obj, R (T::*func)(Args...))
| ^~~~~~~~~~
and then much more errors ...
<snip>
>> + if (bayerFormat.bitDepth == 10 &&
>> + bayerFormat.packing == BayerFormat::Packing::CSI2) {
>> + bpp_ = 10;
>> + patternSize_.height = 2;
>> + patternSize_.width = 4; /* 5 bytes per *4* pixels */
>> + y_skip_mask_ = 0x02; /* Skip every 3th and 4th line */
>> + x_shift_ = 0;
>> +
>> + switch (bayerFormat.order) {
>> + case BayerFormat::BGGR:
>> + case BayerFormat::GRBG:
>> + stats0_ = (SwStats::statsProcessFn)&SwStatsCpu::statsBGGR10PLine0;
>> + swap_lines_ = bayerFormat.order == BayerFormat::GRBG;
>> + return 0;
>> + case BayerFormat::GBRG:
>> + case BayerFormat::RGGB:
>> + stats0_ = (SwStats::statsProcessFn)&SwStatsCpu::statsGBRG10PLine0;
>> + swap_lines_ = bayerFormat.order == BayerFormat::RGGB;
>> + return 0;
>> + default:
>> + break;
>> + }
>> + }
>> +
>> + LOG(SwStats, Info)
>
> Sounds like an error.
>
>> + << "Unsupported input format " << inputCfg.pixelFormat.toString();
>> + return -EINVAL;
This may get hit when the simplepipeline is enumerating and a CSI receiver
has an unsupported format. E.g. the IPU6 supports both 10bpp packed / unpacked
and when testing with only the initial patches adding only unpacked support this
gets hit.
Regards,
Hans
More information about the libcamera-devel
mailing list