ports/alif: Improve PAG7936 Image Quality. #2911

kwagyeman · 2025-11-03T06:15:25Z

Fixes issues with the speed of the AWB algorithm and enables controls to enable/disable it.

github-actions · 2025-11-03T06:39:54Z

Code Size Report:

Firmware	Text Diff	Data Diff	BSS Diff
OPENMV_AE3/firmware_M55_HP.elf	🔺0.02% (+448)	➖0.00% (+0)	🔺0.00% (+8)
OPENMV_N6/firmware.elf	🔺0.04% (+656)	➖0.00% (+0)	➖0.00% (+0)

iabdalkader

Mostly okay. Main change: Rework the API..

common/average.h

iabdalkader · 2025-11-03T19:31:35Z

common/average.c

@@ -0,0 +1,81 @@
+/*


I don't like this API, it should be omv_csi_x. For example, omv_csi_stats_t, and functions moved to common/omv_csi.c: omv_csi_stats_update etc...

Done, renamed it to awb as it's controlling that.

drivers/sensors/pag7936.c

iabdalkader · 2025-11-03T19:37:24Z

lib/imlib/bayer.c


+    for (int i = 0; i < 256; i++) {
+        int v = fast_roundf(((fast_powf(i * (1.0f / 255.0f), gamma) * contrast) + brightness) * 255.0f);
+        gamma_lut[i] = IM_CLAMP(v, 0, 255);


Should this be recalculated on every call or when those values change?

I was going to expose the brightness/contrast controls to the sensor API later on. It only needs to be done when the value changes, however, the lut part is an implementation detail. So, it shouldn't leave bayer.c. I can move the table update to another function call that's only done on value changes.

ports/alif/omv_portconfig.h

iabdalkader · 2025-11-03T19:47:59Z

common/omv_csi.c

        NULL, NULL
    };

+    #if defined(OMV_CSI_RGB_AVG_SIZE)


Suggested change

#if defined(OMV_CSI_RGB_AVG_SIZE)

#if defined(OMV_CSI_STATS_ENABLE)

kwagyeman · 2025-11-03T19:57:25Z

lib/imlib/bayer.c

+// FIXME: This needs to be placed in the DTCM. .bss is in the DTCM on Alif. But, this is not portable.
+// Move this to a proper DTCM section for other platforms.
+static uint8_t gamma_lut[256];


@iabdalkader - Ideas on how to solve this? It needs to be in the DTCM because that has the quad read port on the AE3.

If you want a DTCM section, just add one (e.g, OMV_FAST_MEMORY), what's the problem?

Moved to the other PR with fast memory.

iabdalkader

There might be more comments, you should start with the gamma lut PR then we'll have another look at this.

iabdalkader · 2025-11-09T07:53:35Z

boards/OPENMV_AE3/omv_boardconfig.h

 // Camera interface
 #define OMV_CSI_BASE                    ((CPI_Type *) CPI_BASE)
 #define OMV_CSI_CLK_FREQUENCY           (24000000)
+#define OMV_CSI_AWB_ENABLE


Suggested change

#define OMV_CSI_AWB_ENABLE

#define OMV_CSI_STATS_ENABLE (1)

There are two different things here: the CSI collects stats, and the image library does something with the stats (AWB). This enables collecting stats, what you do with those stats is a different story. omv_csi_awb_get doesn't make sense.

iabdalkader · 2025-11-09T07:57:22Z

common/omv_csi.h

+#if defined(OMV_CSI_AWB_ENABLE)
+typedef struct omv_csi_awb {
+    bool enable;
+    bool initialized;
+    uint32_t last_ms;
+    float r_ema;
+    float g_ema;
+    float b_ema;
+} omv_csi_awb_t;
+#endif // defined(OMV_CSI_AWB_ENABLE)


Suggested change

#if defined(OMV_CSI_AWB_ENABLE)

typedef struct omv_csi_awb {

bool enable;

bool initialized;

uint32_t last_ms;

float r_ema;

float g_ema;

float b_ema;

} omv_csi_awb_t;

#endif // defined(OMV_CSI_AWB_ENABLE)

#if OMV_CSI_STATS_ENABLE

typedef struct omv_csi_stats {

bool enable;

bool initialized;

uint32_t last_ms;

float r_avg;

float g_avg;

float b_avg;

} omv_csi_stats_t;

#endif // OMV_CSI_STATS_ENABLE

iabdalkader · 2025-11-09T08:08:38Z

common/omv_csi.h

+#if defined(OMV_CSI_AWB_ENABLE)
+// Update RGB moving average struct.
+void omv_csi_awb_update(omv_csi_t *csi, uint32_t *r, uint32_t *g, uint32_t *b, uint32_t ms);
+
+// Get RGB moving average values.
+void omv_csi_awb_get(omv_csi_t *csi, uint32_t *r, uint32_t *g, uint32_t *b);
+#endif // defined(OMV_CSI_AWB_ENABLE)


Suggested change

#if defined(OMV_CSI_AWB_ENABLE)

// Update RGB moving average struct.

void omv_csi_awb_update(omv_csi_t *csi, uint32_t *r, uint32_t *g, uint32_t *b, uint32_t ms);

// Get RGB moving average values.

void omv_csi_awb_get(omv_csi_t *csi, uint32_t *r, uint32_t *g, uint32_t *b);

#endif // defined(OMV_CSI_AWB_ENABLE)

#if OMV_CSI_STATS_ENABLE

// Update RGB stats.

void omv_csi_stats_update(omv_csi_t *csi, uint32_t *r, uint32_t *g, uint32_t *b, uint32_t ms);

// Get RGB stats.

void omv_csi_get_stats(omv_csi_t *csi, uint32_t *r, uint32_t *g, uint32_t *b);

#endif // OMV_CSI_STATS_EMA_ALPHA

iabdalkader · 2025-11-09T08:20:11Z

lib/imlib/bayer.c

+// FIXME: This needs to be placed in the DTCM. .bss is in the DTCM on Alif. But, this is not portable.
+// Move this to a proper DTCM section for other platforms.
+static uint8_t gamma_lut[256];


If you want a DTCM section, just add one (e.g, OMV_FAST_MEMORY), what's the problem?

iabdalkader · 2025-11-09T08:23:05Z

lib/imlib/bayer.c


+// FIXME: This needs to be placed in the DTCM. .bss is in the DTCM on Alif. But, this is not portable.
+// Move this to a proper DTCM section for other platforms.
+static uint8_t gamma_lut[256];


Here's my suggestion: Move this to imlib.c, initialize in init0, while we're at it, please rename imlib_init/deinit_all to just imlib_init/deinit.

Please send this in a new PR first.

I'd also consider mapping the input from an easier to work with range, like: 0.0f..1.0f or 0..255, to make it easier to pass down from Python. This can also be done in the Python module.

We can later implement set_brightness/contrast/gamma, have them call this function to update the table.

Suggested change

static uint8_t gamma_lut[256];

#if ENABLE_GAMMA_LUT

// extern in header

uint8_t gamma_lut[256];

#endif

void imlib_init0() {

#if ENABLE_GAMMA_LUT

imlib_update_gamma_table(-0.2f, 1.0f, 2.2f);

#endif

}

#if ENABLE_GAMMA_LUT

// brightness: -1.0 to 1.0, contrast: 0.0 to 2.0, gamma: 0.0 to inf.

void imlib_update_gamma_table(float brightness, float contrast, float gamma) {

for (int i = 0; i < 256; i++) {

int v = fast_roundf(((fast_powf(i * (1.0f / 255.0f), (1.0f / gamma)) * contrast) + brightness) * 255.0f);

gamma_lut[i] = IM_CLAMP(v, 0, 255);

}

}

#endif

Moved to: #2914

I'd also consider mapping the input from an easier to work with range, like: 0.0f..1.0f or 0..255, to make it easier to pass down from Python. This can also be done in the Python module.

I tried to match the way arguments are passed to the function gamma().

We can later implement set_brightness/contrast/gamma, have them call this function to update the table.

Yes, but, note, the original code with it just recalculates every time was the easiest for handle future updates. This table will need to be moved to the CSI struct after brightness/contrast controls are exposed, as it technically needs to be reset on sensor reset and varies per sensor, since sensors can be controlled independently.

Anyway, the current method is the best for it being static at the moment, but, expect it to be refactored again shortly.

I tried to match the way arguments are passed to the function gamma().

That function applies gamma correction to an image, right? I think we actually need to match these functions instead https://docs.openmv.io/library/omv.sensor.html#sensor.set_contrast

But the -3 to 3 range is also a really horrible artifact from the past. Perhaps we should refactor those to accept 0..100, or floats.

I like the current float format for gamma() as it's normalized and can easily map to anything.

-3 to 3 range is also a really horrible artifact from the past

Agreeded :)

iabdalkader · 2025-11-09T08:47:08Z

common/omv_csi.c

+
+    // Return the current average.
+    *r = fast_roundf(awb->r_ema);
+    *g = fast_roundf(awb->g_ema);
+    *b = fast_roundf(awb->b_ema);


The function below, already does that, just call it.

Suggested change

// Return the current average.

*r = fast_roundf(awb->r_ema);

*g = fast_roundf(awb->g_ema);

*b = fast_roundf(awb->b_ema);

iabdalkader · 2025-11-10T10:52:30Z

common/omv_csi.h

+    bool enable;
+    bool initialized;
+    uint32_t last_ms;
+    float r_avg;
+    float g_avg;
+    float b_avg;


Suggested change

bool enable;

bool initialized;

uint32_t last_ms;

float r_avg;

float g_avg;

float b_avg;

float r_avg;

float g_avg;

float b_avg;

uint32_t last_ms;

Can we use r_avg==0 to detect if it's initialized or not? Or bad idea?
Also please add bool stats_enabled to csi_t directly.

Can we use r_avg==0 to detect if it's initialized or not? Or bad idea?

No, we don't want to do that as once I add black line calibration (which subtracts the value of pixels when seeing pure black, like when the lens cap is on), then it's quite possible to have the image go to all 0 in a dark scene.

Also please add bool stats_enabled to csi_t directly.

Done

The image turning green previously was caused by the rgb stats not being sampled continously (only every 100 frames) coupled with only happening on snapshot. As such, any algorithm causing the fps to slow down would lower the update freq. The new average solution ensures that the rgb average is smoothed over many frames and only keeps the last 250ms of history.

This also resolves issues with the luminace target flickering as the average cannot update instantly which filters out flicking on the PS5520 updating its settings quickly.

kwagyeman requested a review from iabdalkader November 3, 2025 06:15

kwagyeman force-pushed the kwabena/improve_pag7936_image_quality branch from a785163 to c41fc4b Compare November 3, 2025 06:29

kwagyeman changed the title ~~Kwabena/improve pag7936 image quality~~ ports/alif: Improve PAG7936 Image Quality. Nov 3, 2025

kwagyeman force-pushed the kwabena/improve_pag7936_image_quality branch from c41fc4b to 0c1d134 Compare November 3, 2025 06:36

kwagyeman added the enhancement label Nov 3, 2025

iabdalkader added this to the v5.0.0 milestone Nov 3, 2025

iabdalkader requested changes Nov 3, 2025

View reviewed changes

kwagyeman commented Nov 3, 2025

View reviewed changes

kwagyeman force-pushed the kwabena/improve_pag7936_image_quality branch 3 times, most recently from d9198cf to 6ebfde0 Compare November 9, 2025 06:20

iabdalkader requested changes Nov 9, 2025

View reviewed changes

kwagyeman force-pushed the kwabena/improve_pag7936_image_quality branch 3 times, most recently from 924c56b to ecc3781 Compare November 10, 2025 01:56

iabdalkader reviewed Nov 10, 2025

View reviewed changes

kwagyeman added 4 commits November 10, 2025 13:02

common: Add AWB averaging.

d8f92a4

ports/stm32: Use 250ms rgb moving average for stats.

5bff664

This also resolves issues with the luminace target flickering as the average cannot update instantly which filters out flicking on the PS5520 updating its settings quickly.

drivers/sensors: Add basic awb controls for sensors using an isp.

bf0d931

kwagyeman force-pushed the kwabena/improve_pag7936_image_quality branch from ecc3781 to bf0d931 Compare November 10, 2025 21:25

	#if defined(OMV_CSI_RGB_AVG_SIZE)
	#if defined(OMV_CSI_STATS_ENABLE)

-static uint8_t gamma_lut[256];
+#if ENABLE_GAMMA_LUT
+// extern in header
+uint8_t gamma_lut[256];
+#endif
+void imlib_init0() {
+#if ENABLE_GAMMA_LUT
+   imlib_update_gamma_table(-0.2f, 1.0f, 2.2f);
+#endif
+}
+#if ENABLE_GAMMA_LUT
+// brightness: -1.0 to 1.0, contrast: 0.0 to 2.0, gamma: 0.0 to inf.
+void imlib_update_gamma_table(float brightness, float contrast, float gamma) {
+    for (int i = 0; i < 256; i++) {
+        int v = fast_roundf(((fast_powf(i * (1.0f / 255.0f), (1.0f / gamma)) * contrast) + brightness) * 255.0f);
+        gamma_lut[i] = IM_CLAMP(v, 0, 255);
+    }
+}
+#endif

ports/alif: Improve PAG7936 Image Quality. #2911

Are you sure you want to change the base?

ports/alif: Improve PAG7936 Image Quality. #2911

Uh oh!

Conversation

kwagyeman commented Nov 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Nov 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iabdalkader left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iabdalkader left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iabdalkader Nov 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iabdalkader Nov 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

kwagyeman commented Nov 3, 2025 •

edited

Loading

github-actions bot commented Nov 3, 2025 •

edited

Loading

iabdalkader Nov 9, 2025 •

edited

Loading

iabdalkader Nov 10, 2025 •

edited

Loading