Skip to content

drivers: video: sw_generator: Hue Saturation Brightness controls #79458

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 4 commits into from
Closed

drivers: video: sw_generator: Hue Saturation Brightness controls #79458

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
87b82f5
sys: util: add SCALE() to change a value range
josuah Oct 5, 2024
408884d
zdsp: add a file with macro implementation of fixed point functions
josuah Oct 5, 2024
d65fbf3
sys: util: add a MAP_VALUE to change a value range
josuah Oct 5, 2024
638fa35
drivers: video: sw_generator: add hue, saturation, brightness controls
josuah Oct 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion drivers/video/Kconfig.sw_generator
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
# MT9m114
# Software-based pattern generator

# Copyright (c) 2016 Linaro Limited
# SPDX-License-Identifier: Apache-2.0
Expand Down
243 changes: 198 additions & 45 deletions drivers/video/video_sw_generator.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
/*
* Copyright (c) 2019, Linaro Limited
* Copyright (c) 2024, tinyVision.ai Inc.
*
* SPDX-License-Identifier: Apache-2.0
*/
Expand All @@ -9,6 +10,11 @@
#include <zephyr/kernel.h>
#include <zephyr/drivers/video.h>
#include <zephyr/logging/log.h>
#include <zephyr/dsp/print_format.h>
#include <zephyr/dsp/types.h>
#include <zephyr/dsp/macros.h>

#include <video_common.h>

LOG_MODULE_REGISTER(video_sw_generator, CONFIG_VIDEO_LOG_LEVEL);

Expand All @@ -22,6 +28,15 @@
* format. 60 fps is therefore chosen as a common value in practice.
*/
#define MAX_FRAME_RATE 60
#define HUE(a) Q15f((double)(a) / 360.)
#define SMPTE_NUM 7

static const q15_t smpte_colorbar_hsv[3][SMPTE_NUM] = {
/* white, yellow, cyan, green, magenta, red, blue */
{HUE(0), HUE(60), HUE(180), HUE(120), HUE(300), HUE(0), HUE(240)},
{Q15f(0.), Q15f(1.), Q15f(1.), Q15f(1.), Q15f(1.), Q15f(1.), Q15f(1.)},

Check notice on line 37 in drivers/video/video_sw_generator.c

View workflow job for this annotation

GitHub Actions / Run compliance checks on patch series (PR)

You may want to run clang-format on this change 

drivers/video/video_sw_generator.c:37 
-#define HUE(a)		        Q15f((double)(a) / 360.)
+#define HUE(a)                  Q15f((double)(a) / 360.)
 #define SMPTE_NUM               7
 
 static const q15_t smpte_colorbar_hsv[3][SMPTE_NUM] = {
 	/* white,  yellow,   cyan,     green,    magenta,  red,      blue */
-	{HUE(0),   HUE(60),  HUE(180), HUE(120), HUE(300), HUE(0),   HUE(240)},
+	{HUE(0), HUE(60), HUE(180), HUE(120), HUE(300), HUE(0), HUE(240)},
{Q15f(1.), Q15f(1.), Q15f(1.), Q15f(1.), Q15f(1.), Q15f(1.), Q15f(1.)},
};

struct video_sw_generator_data {
const struct device *dev;
Expand All @@ -31,30 +46,27 @@
struct k_work_delayable buf_work;
struct k_work_sync work_sync;
int pattern;
uint8_t colorbar_rgb565[SMPTE_NUM][2];
uint8_t colorbar_xrgb32[SMPTE_NUM][4];
uint8_t colorbar_yuyv[SMPTE_NUM][4];
q7_t ctrl_hsv_q7[3];
bool ctrl_hflip;
bool ctrl_vflip;
struct k_poll_signal *signal;
uint32_t frame_rate;
};

static const struct video_format_cap fmts[] = {{
.pixelformat = VIDEO_PIX_FMT_RGB565,
.width_min = 64,
.width_max = 1920,
.height_min = 64,
.height_max = 1080,
.width_step = 1,
.height_step = 1,
}, {
.pixelformat = VIDEO_PIX_FMT_XRGB32,
.width_min = 64,
.width_max = 1920,
.height_min = 64,
.height_max = 1080,
.width_step = 1,
.height_step = 1,
},
{0}};
#define VIDEO_SW_GENERATOR_FORMAT_CAP(fourcc) \
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code optimization should go into a separate commit.

{ \
.pixelformat = (fourcc), .width_min = 64, .width_max = 1920, .height_min = 1, \
.height_max = 1080, .width_step = 1, .height_step = 1, \
}

Check notice on line 62 in drivers/video/video_sw_generator.c

View workflow job for this annotation

GitHub Actions / Run compliance checks on patch series (PR)

You may want to run clang-format on this change 

drivers/video/video_sw_generator.c:62 
-		.pixelformat = (fourcc), .width_min = 64, .width_max = 1920, .height_min = 1,      \
-		.height_max = 1080, .width_step = 1, .height_step = 1,                             \
+		.pixelformat = (fourcc),                                                           \
+		.width_min = 64,                                                                   \
+		.width_max = 1920,                                                                 \
+		.height_min = 1,                                                                   \
+		.height_max = 1080,                                                                \
+		.width_step = 1,                                                                   \
+		.height_step = 1,                                                                  \

static const struct video_format_cap fmts[] = {
VIDEO_SW_GENERATOR_FORMAT_CAP(VIDEO_PIX_FMT_RGB565),
VIDEO_SW_GENERATOR_FORMAT_CAP(VIDEO_PIX_FMT_XRGB32),
VIDEO_SW_GENERATOR_FORMAT_CAP(VIDEO_PIX_FMT_YUYV),
{0},
};

static int video_sw_generator_set_fmt(const struct device *dev, enum video_endpoint_id ep,
struct video_format *fmt)
Expand Down Expand Up @@ -116,28 +128,145 @@
return 0;
}

/* Black, Blue, Red, Purple, Green, Aqua, Yellow, White */
uint16_t rgb565_colorbar_value[] = {0x0000, 0x001F, 0xF800, 0xF81F, 0x07E0, 0x07FF, 0xFFE0, 0xFFFF};
static void hsv_to_rgb(q15_t h, q15_t s, q15_t v, q15_t *r, q15_t *g, q15_t *b)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think color related stuffs (macro, functions, etc.) is generic and should go into a separate .h file to enhance readabitlity and reusability.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, I will do this!

{
q15_t chroma = MULq15(s, v);
q15_t x;

if (h < Q15f(1. / 6.)) {
x = SUBq15(h, Q15f(0. / 6.)) * 6;
*r = chroma;
*g = MULq15(chroma, x);
*b = Q15f(0.);
} else if (h < Q15f(2. / 6.)) {
x = SUBq15(h, Q15f(1. / 6.)) * 6;
*r = MULq15(chroma, SUBq15(Q15f(1.), x));
*g = chroma;
*b = Q15f(0.);
} else if (h < Q15f(3. / 6.)) {
x = SUBq15(h, Q15f(2. / 6.)) * 6;
*r = Q15f(0.);
*g = chroma;
*b = MULq15(chroma, x);
} else if (h < Q15f(4. / 6.)) {
x = SUBq15(h, Q15f(3. / 6.)) * 6;
*r = Q15f(0.);
*g = MULq15(chroma, SUBq15(Q15f(1), x));
*b = chroma;
} else if (h < Q15f(5. / 6.)) {
x = SUBq15(h, Q15f(4. / 6.)) * 6;
*r = MULq15(chroma, x);
*g = Q15f(0.);
*b = chroma;
} else {
x = SUBq15(h, Q15f(5. / 6.)) * 6;
*r = chroma;
*g = Q15f(0.);
*b = MULq15(chroma, SUBq15(Q15f(1), x));
}

*r = ADDq15(SUBq15(v, chroma), *r);
*g = ADDq15(SUBq15(v, chroma), *g);
*b = ADDq15(SUBq15(v, chroma), *b);
}

#define BT709_WR 0.2126
#define BT709_WG 0.7152
#define BT709_WB 0.0722
#define BT709_UMAX 0.436
#define BT709_VMAX 0.615

static void rgb_to_yuv(q15_t r, q15_t g, q15_t b, q15_t *y, q15_t *u, q15_t *v)
{
q15_t ux = Q15f(BT709_UMAX / (1. - BT709_WB));
q15_t vx = Q15f(BT709_VMAX / (1. - BT709_WR));

/* Using BT.709 coefficients */
*y = 0;
*y = ADDq15(*y, MULq15(Q15f(BT709_WR), r));
*y = ADDq15(*y, MULq15(Q15f(BT709_WG), g));
*y = ADDq15(*y, MULq15(Q15f(BT709_WB), b));
*u = MULq15(ux, SUBq15(b, *y));
*v = MULq15(vx, SUBq15(r, *y));
}

static void hsv_adjust(q15_t *hue, q15_t *sat, q15_t *val, q15_t h, q15_t s, q15_t v)
{
*hue = MODq15((q31_t)MAXq15 + (q31_t)*hue + (q31_t)h, MAXq15);
*sat = CLAMP((q31_t)*sat + (q31_t)s, 0, MAXq15);
*val = CLAMP((q31_t)*val + (q31_t)v, 0, MAXq15);
}

static void init_colors(const struct device *dev)
{
struct video_sw_generator_data *data = dev->data;

uint32_t xrgb32_colorbar_value[] = {0xFF000000, 0xFF0000FF, 0xFFFF0000, 0xFFFF00FF,
0xFF00FF00, 0xFF00FFFF, 0xFFFFFF00, 0xFFFFFFFF};
for (int i = 0; i < SMPTE_NUM; i++) {
q15_t r, g, b;
q15_t y, u, v;
q15_t hue = smpte_colorbar_hsv[0][i];
q15_t sat = smpte_colorbar_hsv[1][i];
q15_t val = smpte_colorbar_hsv[2][i];
uint16_t u16;

hsv_adjust(&hue, &sat, &val, Q15q7(data->ctrl_hsv_q7[0]),
Q15q7(data->ctrl_hsv_q7[1]), Q15q7(data->ctrl_hsv_q7[2]));
hsv_to_rgb(hue, sat, val, &r, &g, &b);
rgb_to_yuv(r, g, b, &y, &u, &v);

LOG_DBG("H%1"PRIq(3)" S%1"PRIq(3)" V%1"PRIq(3)", "
"R%1"PRIq(3)" G%1"PRIq(3)" B%1"PRIq(3)", "
"Y%1"PRIq(3)" U%1"PRIq(3)" V%1"PRIq(3),
PRIq_arg(hue, 3, 0), PRIq_arg(sat, 3, 0), PRIq_arg(val, 3, 0),
PRIq_arg(r, 3, 0), PRIq_arg(g, 3, 0), PRIq_arg(b, 3, 0),
PRIq_arg(y, 3, 0), PRIq_arg(u, 3, 0), PRIq_arg(v, 3, 0));

Check notice on line 223 in drivers/video/video_sw_generator.c

View workflow job for this annotation

GitHub Actions / Run compliance checks on patch series (PR)

You may want to run clang-format on this change 

drivers/video/video_sw_generator.c:223 
-		LOG_DBG("H%1"PRIq(3)" S%1"PRIq(3)" V%1"PRIq(3)", "
-			"R%1"PRIq(3)" G%1"PRIq(3)" B%1"PRIq(3)", "
-			"Y%1"PRIq(3)" U%1"PRIq(3)" V%1"PRIq(3),
+		LOG_DBG("H%1" PRIq(3) " S%1" PRIq(3) " V%1" PRIq(
+				3) ", "
+				   "R%1" PRIq(3) " G%1" PRIq(3) " B%1" PRIq(
+					   3) ", "
+					      "Y%1" PRIq(3) " U%1" PRIq(3) " V%1" PRIq(3),
 			PRIq_arg(hue, 3, 0), PRIq_arg(sat, 3, 0), PRIq_arg(val, 3, 0),
-			PRIq_arg(r, 3, 0), PRIq_arg(g, 3, 0), PRIq_arg(b, 3, 0),
-			PRIq_arg(y, 3, 0), PRIq_arg(u, 3, 0), PRIq_arg(v, 3, 0));
+			PRIq_arg(r, 3, 0), PRIq_arg(g, 3, 0), PRIq_arg(b, 3, 0), PRIq_arg(y, 3, 0),
+			PRIq_arg(u, 3, 0), PRIq_arg(v, 3, 0));
u16 = BITSq15(r, 5) | (BITSq15(g, 6) << 5) | (BITSq15(b, 5) << 11);
data->colorbar_rgb565[i][0] = u16 >> 0;
data->colorbar_rgb565[i][1] = u16 >> 8;

data->colorbar_xrgb32[i][0] = 0x00;
data->colorbar_xrgb32[i][1] = BITSq15(r, 8);
data->colorbar_xrgb32[i][2] = BITSq15(g, 8);
data->colorbar_xrgb32[i][3] = BITSq15(g, 8);

data->colorbar_yuyv[i][0] = BITSq15(y, 8);
data->colorbar_yuyv[i][1] = BITSq15(ADDq15(Q15f(0.5), u), 8);
data->colorbar_yuyv[i][2] = BITSq15(y, 8);
data->colorbar_yuyv[i][3] = BITSq15(ADDq15(Q15f(0.5), v), 8);
}
}

static void __fill_buffer_colorbar(struct video_sw_generator_data *data, struct video_buffer *vbuf)
{
int bw = data->fmt.width / 8;
int h, w, i = 0;

for (h = 0; h < data->fmt.height; h++) {
for (w = 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 = rgb565_colorbar_value[color_idx];
for (w = 0; w < data->fmt.width;) {
int color_idx = w * SMPTE_NUM / data->fmt.width;

if (data->ctrl_hflip) {
color_idx = SMPTE_NUM - 1 - color_idx;
}

switch (data->fmt.pixelformat) {
case VIDEO_PIX_FMT_RGB565:
memcpy(&vbuf->buffer[i], data->colorbar_rgb565[color_idx], 2);
i += 2;
} else if (data->fmt.pixelformat == VIDEO_PIX_FMT_XRGB32) {
uint32_t *pixel = (uint32_t *)&vbuf->buffer[i];
*pixel = xrgb32_colorbar_value[color_idx];
w += 1;
break;
case VIDEO_PIX_FMT_XRGB32:
memcpy(&vbuf->buffer[i], data->colorbar_xrgb32[color_idx], 4);
i += 4;
w += 1;
break;
case VIDEO_PIX_FMT_YUYV:
memcpy(&vbuf->buffer[i], data->colorbar_yuyv[color_idx], 4);
i += 4;
w += 2;
break;
default:
__ASSERT_NO_MSG(false);
}
}
}
Expand Down Expand Up @@ -259,10 +388,30 @@
void *value)
{
struct video_sw_generator_data *data = dev->data;
uint32_t u32 = (uint32_t)value;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

u32 seems not a good name, confused to the type.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, I realize this is not a good name for a variable.

int ret;

ret = video_check_range_u32(dev, cid, u32);
if (ret < 0) {
LOG_ERR("value %u not in range", u32);
return ret;
}

switch (cid) {
case VIDEO_CID_VFLIP:
data->ctrl_vflip = (bool)value;
case VIDEO_CID_HFLIP:
data->ctrl_hflip = (bool)value;
break;
case VIDEO_CID_HUE:
data->ctrl_hsv_q7[0] = u32 - MINq7;
init_colors(dev);
break;
case VIDEO_CID_SATURATION:
data->ctrl_hsv_q7[1] = u32 - MINq7;
init_colors(dev);
break;
case VIDEO_CID_BRIGHTNESS:
data->ctrl_hsv_q7[2] = u32 - MINq7;
init_colors(dev);
break;
default:
return -ENOTSUP;
Expand Down Expand Up @@ -350,18 +499,12 @@
#endif
};

static struct video_sw_generator_data video_sw_generator_data_0 = {
.fmt.width = 320,
.fmt.height = 160,
.fmt.pitch = 320 * 2,
.fmt.pixelformat = VIDEO_PIX_FMT_RGB565,
.frame_rate = DEFAULT_FRAME_RATE,
};

static int video_sw_generator_init(const struct device *dev)
{
struct video_sw_generator_data *data = dev->data;

init_colors(dev);

data->dev = dev;
k_fifo_init(&data->fifo_in);
k_fifo_init(&data->fifo_out);
Expand All @@ -370,6 +513,16 @@
return 0;
}

DEVICE_DEFINE(video_sw_generator, "VIDEO_SW_GENERATOR", &video_sw_generator_init, NULL,
&video_sw_generator_data_0, NULL, POST_KERNEL, CONFIG_VIDEO_INIT_PRIORITY,
&video_sw_generator_driver_api);
#define VIDEO_SW_GENERATOR_DEFINE(inst) \
static struct video_sw_generator_data video_sw_generator_data_##inst = { \
.fmt.width = 320, \
.fmt.height = 160, \
.fmt.pitch = 320 * 2, \
.fmt.pixelformat = VIDEO_PIX_FMT_RGB565, \
}; \
\
DEVICE_DT_INST_DEFINE(inst, &video_sw_generator_init, NULL, \
&video_sw_generator_data_##inst, NULL, POST_KERNEL, \
CONFIG_VIDEO_INIT_PRIORITY, &video_sw_generator_driver_api);

DT_INST_FOREACH_STATUS_OKAY(VIDEO_SW_GENERATOR_DEFINE)
11 changes: 11 additions & 0 deletions dts/bindings/video/zephyr,sw-generator.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
# Copyright 2024 tinyVision.ai Inc.
# SPDX-License-Identifier: Apache-2.0

compatible: "zephyr,sw-generator"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why does this need to be in DT?

Copy link
Collaborator Author

@josuah josuah Oct 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I personally needed it for exposing the hue/saturation/brightness controls over USB Video.
https://github.com/zephyrproject-rtos/zephyr/pull/76798/files#diff-47c7b3f77fda7a7bc55ec72fb80259f4e7b7111fd1f20e0653f28ea90d663b76R13

This permit to refer to this "virtual hardware" as a target for video-controls, using a phandle and avoid a special-case in this driver, or in the various samples.

It sounds a bit of a deformation of the purpose of devicetree describing actual hardware.
Having it on the devicetree was a method to avoid making a special case of it in samples
and every driver to be tested with a software-based back-end.

I'd be glad to search an alternative method if it is more suited!

Copy link
Collaborator

@ngphibang ngphibang Oct 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have the same question. It seems still unclear why we need to have this in DT. The video capture sample currently fallbacks by default to video sw generator if we don't specify any shield.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the use-case ever shows-up, then I can make a PR to introduce it, but it looks like there is no use for it currently. I will close this PR and reopen others for the points suggested.

Very grateful for the feedback from both of you!


description: |
Software-based video pattern generator for testing purpose.
This generates an image pattern in the RAM buffer passed to it,
at the specified dimension.

include: base.yaml
Loading
Loading