[libcamera-devel] statistics cleanup was Re: ... libcamera: ipa: Soft IPA: add a Soft IPA implementation
Hans de Goede
hdegoede at redhat.com
Wed Dec 13 21:38:58 CET 2023
Hi,
On 12/13/23 00:00, Pavel Machek wrote:
> Hi!
>
>> ATM the statistics and debayer classes are mostly ready,
>> but I need to merge this with Andrey's v2 RFC and
>> then produce a clean new series from this
>> (assuming people like the approach).
>
> Could I get you to apply this?
>
> My camera is bayer8, so this is compile-tested only, but should give
> same performance with shorter and more readable source. (It will also
> allow me to add bayer8 statistics with little modifications).
>
> Thank you and best regards,
> Pavel
Thank you.
Note your implementation was broken because you were resetting
the per frame stats every line. The idea is to use a local variable
(hopefully a register) to gather the local r + g + b sums and
then at the end of processing the entire line increase
the per frame stats using the local variable.
I have merged / squashed a fixed version with some further cleanups:
>From ea1be60a181fad3a782fe970198906593601c3d3 Mon Sep 17 00:00:00 2001
From: Pavel Machek <pavel at ucw.cz>
Date: Wed, 13 Dec 2023 00:00:13 +0100
Subject: [PATCH] Pavel swstats changes
Signed-off-by: Hans de Goede <hdegoede at redhat.com>
---
.../internal/software_isp/swstats_cpu.h | 2 -
src/libcamera/software_isp/swstats_cpu.cpp | 120 +++++++-----------
2 files changed, 46 insertions(+), 76 deletions(-)
diff --git a/include/libcamera/internal/software_isp/swstats_cpu.h b/include/libcamera/internal/software_isp/swstats_cpu.h
index 993ade3e..0ceb995f 100644
--- a/include/libcamera/internal/software_isp/swstats_cpu.h
+++ b/include/libcamera/internal/software_isp/swstats_cpu.h
@@ -29,8 +29,6 @@ public:
/* FIXME this should be dropped once AWB has moved to the IPA */
SwIspStats getStats() { return *sharedStats_; }
private:
- void statsBGGR10PLine(const uint8_t *src0, const uint8_t *src1);
- void statsGBRG10PLine(const uint8_t *src0, const uint8_t *src1);
void statsBGGR10PLine0(const uint8_t *src, unsigned int stride);
void statsGBRG10PLine0(const uint8_t *src, unsigned int stride);
void statsGRBG10PLine0(const uint8_t *src, unsigned int stride);
diff --git a/src/libcamera/software_isp/swstats_cpu.cpp b/src/libcamera/software_isp/swstats_cpu.cpp
index 161dd033..84edadd8 100644
--- a/src/libcamera/software_isp/swstats_cpu.cpp
+++ b/src/libcamera/software_isp/swstats_cpu.cpp
@@ -36,102 +36,74 @@ static const unsigned int RED_Y_MUL = 77; /* 0.30 * 256 */
static const unsigned int GREEN_Y_MUL = 150; /* 0.59 * 256 */
static const unsigned int BLUE_Y_MUL = 29; /* 0.11 * 256 */
-/*
- * These need to be macros because it accesses a whole bunch of local
- * variables (and copy and pasting this x times is undesirable)
- */
-#define SWISP_LINARO_START_LINE_STATS() \
- uint8_t r, g, b; \
- unsigned int y_val; \
- \
- unsigned int sumR = 0; \
- unsigned int sumG = 0; \
- unsigned int sumB = 0; \
- \
- unsigned int bright_sum = 0; \
+static inline __attribute__((always_inline)) void
+statsBayer10P(const int width, const uint8_t *src0, const uint8_t *src1, bool bggr, unsigned int &bright_sum_, unsigned int &too_bright_sum_, SwIspStats &stats_)
+{
+ const int width_in_bytes = width * 5 / 4;
+ uint8_t r, g, b, g2;
+ unsigned int y_val;
+ unsigned int sumR = 0;
+ unsigned int sumG = 0;
+ unsigned int sumB = 0;
+
+ unsigned int bright_sum = 0;
unsigned int too_bright_sum = 0;
-#define SWISP_LINARO_ACCUMULATE_LINE_STATS() \
- sumR += r; \
- sumG += g; \
- sumB += b; \
- \
- y_val = r * RED_Y_MUL; \
- y_val += g * GREEN_Y_MUL; \
- y_val += b * BLUE_Y_MUL; \
- if (y_val > BRIGHT_LVL) ++bright_sum; \
- if (y_val > TOO_BRIGHT_LVL) ++too_bright_sum;
+ 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;
-#define SWISP_LINARO_FINISH_LINE_STATS() \
- stats_.sumR_ += sumR; \
- stats_.sumG_ += sumG; \
- stats_.sumB_ += sumB; \
- \
- bright_sum_ += bright_sum; \
+ sumR += r;
+ sumG += g;
+ sumB += b;
+
+ y_val = r * RED_Y_MUL;
+ y_val += g * GREEN_Y_MUL;
+ y_val += b * BLUE_Y_MUL;
+ if (y_val > BRIGHT_LVL) ++bright_sum;
+ if (y_val > TOO_BRIGHT_LVL) ++too_bright_sum;
+ }
+
+ stats_.sumR_ += sumR;
+ stats_.sumG_ += sumG;
+ stats_.sumB_ += sumB;
+
+ bright_sum_ += bright_sum;
too_bright_sum_ += too_bright_sum;
-
-void SwStatsCpu::statsBGGR10PLine(const uint8_t *src0, const uint8_t *src1)
-{
- const int width_in_bytes = window_.width * 5 / 4;
- uint8_t g2;
-
- SWISP_LINARO_START_LINE_STATS()
-
- for (int x = 0; x < width_in_bytes; x += 5) {
- /* BGGR */
- b = src0[x];
- g = src0[x + 1];
- g2 = src1[x];
- r = src1[x + 1];
-
- g = g + g2 / 2;
-
- SWISP_LINARO_ACCUMULATE_LINE_STATS()
- }
- SWISP_LINARO_FINISH_LINE_STATS()
-}
-
-void SwStatsCpu::statsGBRG10PLine(const uint8_t *src0, const uint8_t *src1)
-{
- const int width_in_bytes = window_.width * 5 / 4;
- uint8_t g2;
-
- SWISP_LINARO_START_LINE_STATS()
-
- for (int x = 0; x < width_in_bytes; x += 5) {
- /* GBRG */
- g = src0[x];
- b = src0[x + 1];
- r = src1[x];
- g2 = src1[x + 1];
-
- g = g + g2 / 2;
-
- SWISP_LINARO_ACCUMULATE_LINE_STATS()
- }
- SWISP_LINARO_FINISH_LINE_STATS()
}
void SwStatsCpu::statsBGGR10PLine0(const uint8_t *src, unsigned int stride)
{
- statsBGGR10PLine(src, src + stride);
+ statsBayer10P(window_.width, src, src + stride, true, bright_sum_, too_bright_sum_, stats_);
}
void SwStatsCpu::statsGBRG10PLine0(const uint8_t *src, unsigned int stride)
{
- statsGBRG10PLine(src, src + stride);
+ statsBayer10P(window_.width, src, src + stride, false, bright_sum_, too_bright_sum_, stats_);
}
void SwStatsCpu::statsGRBG10PLine0(const uint8_t *src, unsigned int stride)
{
/* GRBG is BGGR with the lines swapped */
- statsBGGR10PLine(src + stride, src);
+ statsBayer10P(window_.width, src + stride, src, true, bright_sum_, too_bright_sum_, stats_);
}
void SwStatsCpu::statsRGGB10PLine0(const uint8_t *src, unsigned int stride)
{
/* RGGB is GBRG with the lines swapped */
- statsGBRG10PLine(src + stride, src);
+ statsBayer10P(window_.width, src + stride, src, false, bright_sum_, too_bright_sum_, stats_);
}
void SwStatsCpu::resetStats(void)
--
2.41.0
Regards,
Hans
More information about the libcamera-devel
mailing list