drivers: video: sw_generator: add more formats and try to simplify #85968

josuah · 2025-02-18T23:13:37Z

This includes several incremental, small changes to the software generator.

drivers: video: sw_generator: modify video_sw_generator_fill_colorbar()
drivers: video: sw_generator: refactor fmt/frmival selection
drivers: video: sw_generator: refactor: flatten arrays
drivers: video: sw_generator: preserve full prefix for internal functions

It was not broken on ideal conditions: this only help it be more robust during misuse.

josuah · 2025-02-19T23:15:30Z

force-push:

Added 4 different bayer formats
Added YUYV format
Refactored the pattern generator loop

This is a draft until I can properly test every format, i.e. with UVC or other ways.

Ideally this would be having unit tests like done by the capture sample, but this would need to come in another PR.

josuah · 2025-02-23T15:04:48Z

Force-push:

The following formats could be tested with external tooling, not part of upstream Zephyr: YUYV, RGB24, RGGB8
Simplification of how struct k_work_sync is used

There is a challenge though: the YUYV is actually YUV BT.709 (used by sRGB and HDTV), not BT.601 (used by JPEG).
So using the pattern generator and piping it into a JPEG library gives a test pattern with wrong contrast.

EDIT: Linux has a separate format field that every driver can state to tell "If you see YUV value coming from me, mind that these were obtained from RGB using this tranfer function: we would need extra fields on struct video_format to handle this...

I suppose a Kconfig option could act as temporary workaround. Until there is a system using both BT.601 and BT.709, this might work.

josuah · 2025-03-08T22:04:15Z

force-push:

Test the formats using the lib/pixel, and bugfix them.
A bit more input validation

drivers/video/video_sw_generator.c

ngphibang · 2025-03-17T18:32:29Z

drivers/video/video_sw_generator.c

+	vbuf->bytesused = 0;
+
+	if (vbuf->size < data->fmt.pitch) {
+		LOG_WRN("Buffer %p has only %u bytes, shorter than pitch %u",
+			vbuf, vbuf->size, data->fmt.pitch);
+		return;
+	}
+
+	/* Generate the first line of data */
+	for (int w = 0, i = 0; w < data->fmt.width; w++) {
+		int color_idx = data->ctrl_vflip ? 7 - w / bw : w / bw;
+
+		if (data->fmt.pixelformat == VIDEO_PIX_FMT_RGB565) {
+			uint16_t *pixel = (uint16_t *)&vbuf->buffer[i];
+			*pixel = sys_cpu_to_le16(rgb565_colorbar_value[color_idx]);
+		} else if (data->fmt.pixelformat == VIDEO_PIX_FMT_XRGB32) {
+			uint32_t *pixel = (uint32_t *)&vbuf->buffer[i];
+			*pixel = sys_cpu_to_le32(xrgb32_colorbar_value[color_idx]);
+		}
+		i += video_bits_per_pixel(data->fmt.pixelformat) / BITS_PER_BYTE;
+	}
+
+	/* Duplicate the first line all over the buffer */
+	for (int h = 1; h < data->fmt.height; h++) {
+		if (vbuf->size < vbuf->bytesused + data->fmt.pitch) {
+			break;
 		}
+
+		memcpy(vbuf->buffer + h * data->fmt.pitch, vbuf->buffer, data->fmt.pitch);
+		vbuf->bytesused += data->fmt.pitch;


I think the old code is OK. I think we don't need to check the buffer against the format's pitch because

In the current v4z framework, when application allocates the video buffer, it is supposed to allocate sufficient memory for the requested format, if not, it will simply not work.

Otherwise, there is a problem in the current framework which will be pointed out in my upcomming buffer management PR:

the application should not specify the format->pitch as it has no knowdlege about this (this should be set by the driver, e.g. driver can add padding to the width line, etc.)

the application also should not specify the buffer size, it just set the format, the number of buffers and request the framework / driver to alloc the buffers on the video buffer heap.

I replaced data->fmt.pitch by a local pitch variable that is computed locally: the driver knows what it generates, let me know if that does not solve the concerns fully.

(2.) takes the entire buffer management out of the application developer way! 😯

ngphibang · 2025-03-17T18:38:37Z

drivers/video/video_sw_generator.c

+	if (fmt->pitch != fmt->width * video_bits_per_pixel(fmt->pixelformat) / BITS_PER_BYTE) {
+		LOG_ERR("The pitch is not consistent with the other parameters");
+		return -EINVAL;
+	}


Same comment here. This reveals the weak point of the current framework : application should not set the format's pitch.

drivers/video/video_sw_generator.c

ngphibang · 2025-03-17T18:51:14Z

drivers/video/video_sw_generator.c

-	while (fmts[i].pixelformat && (fmts[i].pixelformat != fie->format->pixelformat)) {
-		i++;
+	for (i = 0; fmts[i].pixelformat != 0; ++i) {
+		if (fie->format->pixelformat == fmts[i].pixelformat &&
+		    IN_RANGE(fie->format->width, fmts[i].width_min, fmts[i].width_max) &&
+		    IN_RANGE(fie->format->height, fmts[i].height_min, fmts[i].height_max)) {
+			break;
+		}
 	}

-	if ((i == ARRAY_SIZE(fmts)) || (fie->format->width > fmts[i].width_max) ||
-	    (fie->format->width < fmts[i].width_min) ||
-	    (fie->format->height > fmts[i].height_max) ||
-	    (fie->format->height < fmts[i].height_min)) {
+	if (fmts[i].pixelformat == 0) {
+		LOG_ERR("Nothing matching the requested format was found");


I think the change is the same as the old code except the use of IN_RANGE(). But well, both are fine to me.

This is just an attempt at using idioms.
I reverted fmts[i].pixelformat == 0 back to i == ARRAY_SIZE(fmts) to avoid too many distracting changes.

It seems unchanged since last time, but it is ok to me

I think I forgot to change it there... I notice that maybe ARRAY_SIZE(fmts) needs to be ARRAY_SIZE(fmts) - 1 to exclude the {0} terminator from the comparisons. Same as here.

For this reason along with avoiding memory allocation, it could be convenient to switch to video_enum_format() like video_enum_frmival(). For another RFC/PR maybe!

ngphibang · 2025-04-18T07:35:42Z

drivers/video/video_sw_generator.c

-	while (fmts[i].pixelformat && (fmts[i].pixelformat != fie->format->pixelformat)) {
-		i++;
+	for (i = 0; fmts[i].pixelformat != 0; ++i) {
+		if (fie->format->pixelformat == fmts[i].pixelformat &&
+		    IN_RANGE(fie->format->width, fmts[i].width_min, fmts[i].width_max) &&
+		    IN_RANGE(fie->format->height, fmts[i].height_min, fmts[i].height_max)) {
+			break;
+		}
 	}

-	if ((i == ARRAY_SIZE(fmts)) || (fie->format->width > fmts[i].width_max) ||
-	    (fie->format->width < fmts[i].width_min) ||
-	    (fie->format->height > fmts[i].height_max) ||
-	    (fie->format->height < fmts[i].height_min)) {
+	if (fmts[i].pixelformat == 0) {
+		LOG_ERR("Nothing matching the requested format was found");


It seems unchanged since last time, but it is ok to me

Copilot

Pull Request Overview

This PR enhances the video software generator by adding support for additional pixel formats and simplifying several functions. Key changes include updating header comments and macros to support the new VIDEO_PIX_FMT_RGB24 format, refactoring the format capabilities array using a macro, and splitting the fill_colorbar logic into multiple dedicated fill functions.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
include/zephyr/drivers/video.h	Updated comments and added definitions for the new RGB24 pixel format.
drivers/video/video_sw_generator.c	Refactored work handling, format selection, and added new fill routines for various pixel formats.

drivers/video/video_sw_generator.c

josuah · 2025-05-19T15:33:48Z

Force-push:

CI fixes
Typo fixes
Fixes fixes (fix bugs introduced by previous fixes 😅)

avolmat-st

Very minor points, appart from that LGTM

avolmat-st · 2025-05-19T18:52:49Z

drivers/video/video_sw_generator.c

@@ -274,6 +274,10 @@ static int video_sw_generator_enum_frmival(const struct device *dev, struct vide
 	int idx;
 	int ret;

+	if (fie->index >= 1) {


very minor point, should match this line with the commit log which mentions fie.index > 1

Good catch!

avolmat-st · 2025-05-19T19:01:59Z

drivers/video/video_sw_generator.c

+	return 1;
+}
+
+static uint16_t video_sw_generator_fill_bayer(uint8_t *buffer, uint16_t width, bool hflip,


maybe this should be named fill_bayer8 or something like that since it produce 8bit unpacked bayer.

I removed the 8 from pattern_rgbr8_idx as these are bit-depth agnostic, and added it to fill_bayer.
Thank you!

Hum, I am not sure about that (bit-depth agnostic) but maybe I misunderstood what is being generated.
For a 8bit Bayer, indeed, that would generate B G G R for example, each letter being here 8bit. But for other bayers (10 - 12 - 14- 16 etc), each letter should be on 2 bytes, as described within the BGGR10 etc format description in video.h.
That is the reason why I mentioned that this is creating a 8bit bayer pattern, not 10-12-14-16 etc. Right ?

Maybe this can be reused for 10-bit, 12-bit, 14-bit, 16-bit patterns too?

static const uint8_t pattern_rggb_idx[] = {0, 1, 1, 2}; static const uint8_t pattern_bggr_idx[] = {2, 1, 1, 0}; static const uint8_t pattern_gbrg_idx[] = {1, 2, 0, 1}; static const uint8_t pattern_grbg_idx[] = {1, 0, 2, 1}; static const uint8_t pattern_8bars_yuv_rgb24[8][3] = {...}; static const uint16_t pattern_8bars_yuv_rgb48[8][3] = {...}; // 8-bit row0[w + 0] = pattern_8bars_yuv_rgb24[color_idx][pattern_rggb_idx[0]]; row0[w + 1] = pattern_8bars_yuv_rgb24[color_idx][pattern_rggb_idx[1]]; row1[w + 0] = pattern_8bars_yuv_rgb24[color_idx][pattern_rggb_idx[2]]; row1[w + 1] = pattern_8bars_yuv_rgb24[color_idx][pattern_rggb_idx[3]]; // 16-bit row0[w + 0] = pattern_8bars_yuv_rgb48[color_idx][pattern_rggb_idx[0]]; row0[w + 1] = pattern_8bars_yuv_rgb48[color_idx][pattern_rggb_idx[1]]; row1[w + 0] = pattern_8bars_yuv_rgb48[color_idx][pattern_rggb_idx[2]]; row1[w + 1] = pattern_8bars_yuv_rgb48[color_idx][pattern_rggb_idx[3]];

Side question: did you need 16-bit test pattern generators? We might need that too at some point at tinyVision.ai so I can add it in another PR. for 12-bit and 14-bit, would that be the MIPI packed formats or the padded formats (16-bit per pixel, only 12/14-bit populated with data).

That also made me spot a small issue that I'm fixing now.

Yeah I think that if addition is done, this is to be done within another PR. Here my point was only to mentions that it was handling 8 bit bayer hence need to clarify that.
I think usually that is unpacked formats (aka 10-12-14 bayers are all stored into 16bits with 0 padding). I don't know if someone has a need to get the still packed version.

Use the video_format_caps_index() function to check if a format matches any entry of the format caps. Signed-off-by: Josuah Demangeon <me@josuah.net>

Return an error on fie.index >= 1 as there is only one framerate entry per pixelformat, this prevents an endless loop. Signed-off-by: Josuah Demangeon <me@josuah.net>

Add a check for the array size to avoid overwriting unrelated memory when the buffer is too small for the full format. It first check if there is enough buffer for one line, and fill it programmatically. Then, it will try to duplicate that line over the entire buffer, in the limit of the room available. Signed-off-by: Josuah Demangeon <me@josuah.net>

This refactors the pattern generator functions to also offer bayer formats as input. 4 variant of bayer formats are proposed. The pixel packing is also now split from the color selection: only a single RGB and single YUV array used by all the pattern generators. Signed-off-by: Josuah Demangeon <me@josuah.net>

This complements the 32-bit RGB (XRGB32) test pattern with an equivalent 24-bit RGB (RGB24) implementation. Signed-off-by: Josuah Demangeon <me@josuah.net>

The documentation of k_work_cancel_delayable_sync() states that the input k_work_sync parameter needs to be valid until the function call returns, so there is no need to preserve the state across successive calls. Now that there is a single work-related field in the struct, rename it to simply "work". Signed-off-by: Josuah Demangeon <me@josuah.net>

When the FPS value stored in data->frame_rate is zero, a division by zero occurs. Fix it by clamping the frame rate between min and max values. Signed-off-by: Josuah Demangeon <me@josuah.net>

Sort alphabetically the header added in the previous few commits as well as the original implementation. Signed-off-by: Josuah Demangeon <me@josuah.net>

Provide an initial video buffer in the first call to video_dequeue(). It is used by the driver for choosing between the input or output queue. Applied to capture, capture_to_lvgl and tcpserversink samples. Signed-off-by: Josuah Demangeon <me@josuah.net>

Remove trailing spaces at the end of log string. Move arguments on the next line for readability. Align arguments to first line. Signed-off-by: Josuah Demangeon <me@josuah.net>

josuah · 2025-05-20T11:37:22Z

Force-push:

adjust the numbers: _fill_bayer() -> _fill_bayer8(), pattern_rggb8_idx -> pattern_rggb_idx, rgb/yuv -> rgb24/yuv24
Only tested with RGB565 for the capture and capture_to_lgl samples. I wanted to test everything with UVC (which supports bayer8 formats too) but need to rebase it first...

sonarqubecloud · 2025-05-20T12:01:33Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

ngphibang · 2025-05-21T12:51:16Z

This PR contains the commit 55eb71f which could be considered as a hot fix. I tried to assigned @avolmat-st with hope that it can be merged quickly. Correct me if I did it wrong.

josuah · 2025-05-21T13:22:01Z

@ngphibang apparently, just 3h left to show-up as "ready" in the merge-list:

https://merge-list.zephyrproject.io/ (at the bottom, Ctrl+F sw_generator)

ngphibang · 2025-05-21T13:26:10Z

@josuah That's right, I forgot to check this. Thanks !

zephyrbot added the area: Video Video subsystem label Feb 18, 2025

zephyrbot requested review from loicpoulain and ngphibang February 18, 2025 23:14

josuah force-pushed the pr-video-sw-generator branch from 948ae06 to d803873 Compare February 19, 2025 00:24

josuah marked this pull request as draft February 19, 2025 23:13

josuah force-pushed the pr-video-sw-generator branch 8 times, most recently from 584c08c to ff7d7f6 Compare February 23, 2025 12:47

josuah mentioned this pull request Mar 5, 2025

usb: device_next: new USB Video Class (UVC) implementation #76798

Merged

josuah force-pushed the pr-video-sw-generator branch from 8caaa0d to 334c03f Compare March 8, 2025 22:02

josuah marked this pull request as ready for review March 8, 2025 22:04

josuah mentioned this pull request Mar 13, 2025

drivers: video: sw_isp: software-based image processing #87007

Closed

josuah force-pushed the pr-video-sw-generator branch from 334c03f to 80be4fd Compare March 13, 2025 12:26

ngphibang reviewed Mar 17, 2025

View reviewed changes

josuah force-pushed the pr-video-sw-generator branch from 80be4fd to 3cfe8af Compare March 17, 2025 21:34

josuah changed the title ~~drivers: video: sw_generator: add more checks and try to simplify~~ drivers: video: sw_generator: add more formats and try to simplify Mar 17, 2025

josuah requested a review from ngphibang April 17, 2025 17:08

ngphibang previously approved these changes Apr 18, 2025

View reviewed changes

kartben requested a review from Copilot April 18, 2025 14:59

Copilot AI reviewed Apr 18, 2025

View reviewed changes

drivers/video/video_sw_generator.c Show resolved Hide resolved

josuah dismissed ngphibang’s stale review via 11b85d5 April 18, 2025 19:30

josuah force-pushed the pr-video-sw-generator branch from 3cfe8af to 11b85d5 Compare April 18, 2025 19:30

josuah marked this pull request as ready for review May 19, 2025 15:26

avolmat-st requested changes May 19, 2025

View reviewed changes

drivers: video: sw_generator: use video_format_caps_index()

606ecd7

Use the video_format_caps_index() function to check if a format matches any entry of the format caps. Signed-off-by: Josuah Demangeon <me@josuah.net>

josuah force-pushed the pr-video-sw-generator branch from 8cbe90c to 9080391 Compare May 19, 2025 20:16

kartben removed their assignment May 20, 2025

josuah added 9 commits May 20, 2025 10:33

drivers: video: sw_generator: fix video_sw_generator_enum_frmival()

f80515e

Return an error on fie.index >= 1 as there is only one framerate entry per pixelformat, this prevents an endless loop. Signed-off-by: Josuah Demangeon <me@josuah.net>

drivers: video: sw_generator: introduce the RGB24 test pattern

43f9e32

This complements the 32-bit RGB (XRGB32) test pattern with an equivalent 24-bit RGB (RGB24) implementation. Signed-off-by: Josuah Demangeon <me@josuah.net>

drivers: video: sw_generator: fix div by zero on very low framerate

e9ce587

When the FPS value stored in data->frame_rate is zero, a division by zero occurs. Fix it by clamping the frame rate between min and max values. Signed-off-by: Josuah Demangeon <me@josuah.net>

drivers: video: sw_generator: sort header alphabetically

dabaae9

Sort alphabetically the header added in the previous few commits as well as the original implementation. Signed-off-by: Josuah Demangeon <me@josuah.net>

samples: drivers: video: capture: logging improvements

0850ee8

Remove trailing spaces at the end of log string. Move arguments on the next line for readability. Align arguments to first line. Signed-off-by: Josuah Demangeon <me@josuah.net>

josuah force-pushed the pr-video-sw-generator branch from 9080391 to 0850ee8 Compare May 20, 2025 11:33

github-actions bot assigned kartben May 20, 2025

josuah requested a review from avolmat-st May 20, 2025 11:37

avolmat-st approved these changes May 20, 2025

View reviewed changes

josuah unassigned kartben May 20, 2025

ngphibang approved these changes May 20, 2025

View reviewed changes

ngphibang assigned avolmat-st May 21, 2025

josuah added the bug The issue is a bug, or the PR is fixing a bug label May 21, 2025

kartben merged commit b95d7e2 into zephyrproject-rtos:main May 21, 2025
29 checks passed

josuah mentioned this pull request May 22, 2025

drivers: video: CONFIG_VIDEO_SW_GENERATOR -> zephyr,video-sw-generator #88780

Merged

drivers: video: sw_generator: add more formats and try to simplify #85968

drivers: video: sw_generator: add more formats and try to simplify #85968

Uh oh!

Conversation

josuah commented Feb 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

josuah commented Feb 19, 2025

Uh oh!

josuah commented Feb 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

josuah commented Mar 8, 2025

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!

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!

josuah Apr 18, 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!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

josuah commented May 19, 2025

Uh oh!

avolmat-st 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!

josuah May 20, 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!

josuah commented May 20, 2025

Uh oh!

sonarqubecloud bot commented May 20, 2025

Quality Gate passed

Uh oh!

ngphibang commented May 21, 2025

Uh oh!

josuah commented May 21, 2025

Uh oh!

ngphibang commented May 21, 2025

Uh oh!

Uh oh!

Uh oh!

josuah commented Feb 18, 2025 •

edited

Loading

josuah commented Feb 23, 2025 •

edited

Loading

josuah Apr 18, 2025 •

edited

Loading

josuah May 20, 2025 •

edited

Loading