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

ngphibang · 2025-03-17T17:46:58Z

drivers/video/video_sw_generator.c

@@ -10,6 +10,8 @@
 #include <zephyr/drivers/video.h>
 #include <zephyr/drivers/video-controls.h>
 #include <zephyr/logging/log.h>
+#include <zephyr/sys/util.h>
+#include <zephyr/sys/byteorder.h>


You didn't use any thing in this byteorder.h header. I think you need it in another commit.

Note: this change is now in the commit that uses sys_cpu_to_le32(). Github moved the comment to a different commit it seems.

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.

Copilot · 2025-04-18T15:00:10Z

drivers/video/video_sw_generator.c

+	{0xFF, 0x00, 0xFF}, {0xFF, 0x00, 0x00}, {0x00, 0x00, 0xFF}, {0x00, 0x00, 0x00},
+};
+
+static int video_sw_generator_fill_yuyv(uint8_t *buffer, uint16_t width)
 {


The loop in video_sw_generator_fill_yuyv assumes that width is even. To avoid potential out-of-bound writes when width is odd, consider adding a check or enforcing even width via an assertion.

Suggested change

{

{

__ASSERT((width % 2) == 0, "Width must be even to avoid out-of-bound writes");

Let's increase assert density then!

josuah · 2025-04-18T19:34:16Z

force-push:

Split the commit discussed here in two, fix minor off-by-one in the process (ARRAY_SIZE vs ARRAY_SIZE - 1)
Add extra assert() as suggested by bot review here
rebase on main()

…ions In order to help debugging through GDB and other error messages and debug tools, convert the __xxx prefix to video_sw_generator_xxx full prefix. To help keep function names short, use slightly shorter sufixes. Signed-off-by: Josuah Demangeon <[email protected]>

Help with maintainance and possibly readability by using a more regular layout for various tables of numbers. This adds a comma on the last element to help with formatters like clang-format. Signed-off-by: Josuah Demangeon <[email protected]>

Use the IN_RANGE() macro to compare a format against the format caps width/height_min and width/height_max fields. Signed-off-by: Josuah Demangeon <[email protected]>

Fix video_sw_generator_enum_frmival() only matching the first size of a pixel format. The selection loop was stopping at the first matching pixel format rather than pixel format + size pair. Signed-off-by: Josuah Demangeon <[email protected]>

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 <[email protected]>

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 <[email protected]>

This complements the 32-bit RGB (XRGB32) test pattern with an equivalent 24-bit RGB (RGB24) implementation. Signed-off-by: Josuah Demangeon <[email protected]>

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 <[email protected]>

The struct video_format_caps fmts[] is an array terminated by an empty entry which should not be used while searching or comparing the formats. If using ARRAY_SIZE(), this terminator entry should be subtracted from the list. Signed-off-by: Josuah Demangeon <[email protected]>

josuah · 2025-04-27T13:40:43Z

Force-push:

Updated on latest main
Integrated the hflip control everywhere

On native_sim with the SDL display:

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

Draft

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

Draft

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

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 force-pushed the pr-video-sw-generator branch from 11b85d5 to b749191 Compare April 18, 2025 19:30

josuah added 4 commits April 27, 2025 12:42

drivers: video: sw_generator: use IN_RANGE() when possible

e0e302e

Use the IN_RANGE() macro to compare a format against the format caps width/height_min and width/height_max fields. Signed-off-by: Josuah Demangeon <[email protected]>

josuah force-pushed the pr-video-sw-generator branch from fb15800 to 25982fa Compare April 27, 2025 13:06

josuah added 2 commits April 27, 2025 15:27

josuah force-pushed the pr-video-sw-generator branch from 25982fa to d7605bf Compare April 27, 2025 13:32

josuah added 3 commits April 27, 2025 15:33

drivers: video: sw_generator: introduce the RGB24 test pattern

edc2413

This complements the 32-bit RGB (XRGB32) test pattern with an equivalent 24-bit RGB (RGB24) implementation. Signed-off-by: Josuah Demangeon <[email protected]>

josuah force-pushed the pr-video-sw-generator branch from d7605bf to 4b08f4b Compare April 27, 2025 13:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

josuah commented Feb 18, 2025 •

edited

Loading

josuah commented Feb 19, 2025

josuah commented Feb 23, 2025 •

edited

Loading

josuah commented Mar 8, 2025

ngphibang Mar 17, 2025

josuah Apr 17, 2025

ngphibang Mar 17, 2025

josuah Mar 17, 2025

ngphibang Mar 17, 2025

josuah Mar 17, 2025

ngphibang Mar 17, 2025

josuah Mar 17, 2025

ngphibang Apr 18, 2025

josuah Apr 18, 2025 •

edited

Loading

josuah Apr 18, 2025

ngphibang Apr 18, 2025

Copilot AI left a comment

Copilot AI Apr 18, 2025

josuah Apr 18, 2025

josuah commented Apr 18, 2025 •

edited

Loading

josuah commented Apr 27, 2025

	{
	{
	__ASSERT((width % 2) == 0, "Width must be even to avoid out-of-bound writes");

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

Are you sure you want to change the base?

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

Conversation

josuah commented Feb 18, 2025 • edited Loading

josuah commented Feb 19, 2025

josuah commented Feb 23, 2025 • edited Loading

josuah commented Mar 8, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josuah Apr 18, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Copilot AI Apr 18, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josuah commented Apr 18, 2025 • edited Loading

josuah commented Apr 27, 2025

josuah commented Feb 18, 2025 •

edited

Loading

josuah commented Feb 23, 2025 •

edited

Loading

josuah Apr 18, 2025 •

edited

Loading

josuah commented Apr 18, 2025 •

edited

Loading