drivers: video: controls: add the BASE and CAMERA controls by josuah · Pull Request #88924 · zephyrproject-rtos/zephyr

josuah · 2025-04-22T23:17:02Z

Downstream:

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

It seemed easier to batch-import controls than add them one by one in a hundred of small pull requests. Is this the right approach? Controls from the "BASE" and "CAMERA" that Zephyr APIs can support, (not audio and not video compression) are chosen.

P.S.: Clang-format is ignored for video_get_std_menu_ctrl() in particular, as its heuristic was picking a different strategy for every line.

josuah · 2025-04-23T00:38:58Z

drivers/video/video_ctrls.c

-	static const char *const camera_power_line_frequency[] = {"Disabled", "50 Hz", "60 Hz",
-								  "Auto", NULL};
-	static const char *const camera_exposure_auto[] = {"Auto Mode", "Manual Mode",
-							   "Shutter Priority Mode",
-							   "Aperture Priority Mode", NULL};
-


Proposing a slightly different strategy aiming to reduce effort when adding new controls.

I think with the old static array definition, the data is stored once and persist to the end of the program. By changing to anonymous literal like that, it needs to create temporal char arrays each time the function is called, no ?

Right, they are not static, so it is less obvious for average C developer like me what happens in such case, I'd have to look at the standard to know, while static const char *const names[]; is obvious.

josuah · 2025-04-23T00:40:59Z

include/zephyr/drivers/video-controls.h

-enum video_exposure_auto_type {
+enum video_exposure_auto {


Maybe I should do it the other way around: keep the _type suffix every time...

enum video_exposure_auto is confused with one of the exposure type: VIDEO_EXPOSURE_AUTO = 0,
I think it should rather be enum video_exposure_type as the type suffice is important to say that, for example in a function foo(enum video_exposure_type type) we should put one of the exposure types : auto or manual or shutter, etc.

include/zephyr/drivers/video-controls.h

loicpoulain · 2025-04-23T07:00:28Z

The new control framework is merged! 🎉

It seemed easier to batch-import controls than add them one by one in a hundred of small pull requests. Is this the right approach? Controls from the "BASE" and "CAMERA" that Zephyr APIs can support, (not audio and not video compression) are chosen.

P.S.: Clang-format is ignored for video_get_std_menu_ctrl() in particular, as its heuristic was picking a different strategy for every line.

LGTM, it always good to have new controls support ready, though we should keep in mind that some platforms will be very low in resources and may only want to act as dumb image capture, without embedded control or processing (general comment for the whole video subsystem). So maybe at some point we should think about optimizing this and have unused stuff stripped, either via the preprocessor or linker.

ngphibang

nits:

commit title: drivers: video: controls: add more BASE and CAMERA control IDs
commit message:
"Buttons are left as integer for now": I don't see where is the button stuffs ?
"For the minimum number of buffer, Zephyr uses a different mechanism": I don't see where is the buffer stuffs ?

ngphibang · 2025-04-23T09:01:03Z

drivers/video/video_ctrls.c

-	static const char *const camera_power_line_frequency[] = {"Disabled", "50 Hz", "60 Hz",
-								  "Auto", NULL};
-	static const char *const camera_exposure_auto[] = {"Auto Mode", "Manual Mode",
-							   "Shutter Priority Mode",
-							   "Aperture Priority Mode", NULL};
-


I think with the old static array definition, the data is stored once and persist to the end of the program. By changing to anonymous literal like that, it needs to create temporal char arrays each time the function is called, no ?

ngphibang · 2025-04-23T09:08:01Z

include/zephyr/drivers/video-controls.h

-enum video_exposure_auto_type {
+enum video_exposure_auto {


enum video_exposure_auto is confused with one of the exposure type: VIDEO_EXPOSURE_AUTO = 0,
I think it should rather be enum video_exposure_type as the type suffice is important to say that, for example in a function foo(enum video_exposure_type type) we should put one of the exposure types : auto or manual or shutter, etc.

ngphibang · 2025-04-23T09:29:20Z

Thank you for adding many more control IDs.

It seemed easier to batch-import controls than add them one by one in a hundred of small pull requests. Is this the right approach?

In general, not specific to this PR, I think incremental development is a better approach. We should only add what we need (and even more most of people need, for example, in Linux there are a number of extensions proposed by vendors not go to upstream due to this reason) to avoid dead/unused code.

we should keep in mind that some platforms will be very low in resources and may only want to act as dumb image capture, without embedded control or processing (general comment for the whole video subsystem). So maybe at some point we should think about optimizing this and have unused stuff stripped, either via the preprocessor or linker.

Agreed. The controls IDs do not take much space. But in other cases, apart from resources concern, code should be added if having its usage (e.g. utility functions, etc.). This is not only for the resources concerns but for clarity (people may ask why it is there but have no usage) and consistency (code can be added without proving its usage). When unused code added, I think it will stay there for a very long time because people think "it's better not to touch them maybe someone else need them ...".

Just some thought in general :-). About the PR, it's fine to me to add all the "common" control IDs that we think "will be used". Some controls seems very rarely used.

josuah · 2025-04-23T13:27:42Z

<@loicpoulain> we should keep in mind that some platforms will be very low in resources

Big tables of strings can be big in resource use, I will compare the size before/after for the capture example of various boards and compare it there.

<@ngphibang> incremental development is a better approach

I will go check what the image sensor and UVC could implement and whitelist these controls.
Those that are likely not needed can be filtered away.
Those that are left would be "grey list" and we can choose where to split.

This should help with code size too.

Thank you all for the feedback! :)

josuah · 2025-04-23T13:33:17Z

<@ngphibang> "Buttons are left as integer for now": I don't see where is the button stuffs ?

I noticed that on the Linux doc site:

V4L2_CID_PAN_RESET (button)
When this control is set, the camera moves horizontally to the default position.
-- https://docs.kernel.org/userspace-api/media/v4l/ext-ctrls-camera.html

josuah · 2025-04-23T13:33:42Z

<@ngphibang> "For the minimum number of buffer, Zephyr uses a different mechanism": I don't see where is the buffer stuffs ?

This was about these controls in Linux:

V4L2_CID_MIN_BUFFERS_FOR_CAPTURE (integer)
This is a read-only control that can be read by the application and used as a hint to determine the number of CAPTURE buffers to pass to REQBUFS. The value is the minimum number of CAPTURE buffers that is necessary for hardware to work. This control is required for stateful decoders.

V4L2_CID_MIN_BUFFERS_FOR_OUTPUT (integer)
This is a read-only control that can be read by the application and used as a hint to determine the number of OUTPUT buffers to pass to REQBUFS. The value is the minimum number of OUTPUT buffers that is necessary for hardware to work. > This control is required for stateful encoders.

-- https://docs.kernel.org/userspace-api/media/v4l/control.html

In Zephyr this seems to be how it is currently done:

zephyr/include/zephyr/drivers/video.h

Line 101 in 4680590

uint8_t min_vbuf_count;

josuah

Here are comments for each control about whether to keep or remove them.

If you wish, it is possible to use 👍 and 👎 to quickly annotate.
For now I will keep using this version (with everything) for UVC rebasing.

include/zephyr/drivers/video-controls.h

josuah

I went ahead and marked as 'resolved' each of these:

KEEP everything used by uvc or libcamera or ov5640 or multiple sensors
REMOVE everything used by a one or two ISP chips or CPUs

Remain only the special cases.

ngphibang · 2025-04-26T20:34:44Z

I made a choice for each of remaining cases. Thanks

josuah

Thank you! All ready to go then.

include/zephyr/drivers/video-controls.h

Add all the base controls present like they are in Linux into Zephyr, limited to those that can apply in the current system: - Buttons are left as integer for now. - Some description is modified to fit the Zephyr situation. - For the minimum number of buffer, Zephyr uses a different mechanism. - No audio support through the video subsystem. - Homogenize the wording Signed-off-by: Josuah Demangeon <me@josuah.net>

josuah added area: Video Video subsystem area: camera labels Apr 22, 2025

github-actions bot requested review from loicpoulain and ngphibang April 22, 2025 23:17

josuah force-pushed the pr-video-more-ctrls branch 2 times, most recently from 5bb4384 to bb9f7c0 Compare April 22, 2025 23:44

josuah commented Apr 23, 2025

View reviewed changes

include/zephyr/drivers/video-controls.h Outdated Show resolved Hide resolved

loicpoulain previously approved these changes Apr 23, 2025

View reviewed changes

ngphibang reviewed Apr 23, 2025

View reviewed changes

josuah dismissed loicpoulain’s stale review via f74cc53 April 24, 2025 20:42

josuah force-pushed the pr-video-more-ctrls branch from bb9f7c0 to f74cc53 Compare April 24, 2025 20:42

josuah commented Apr 24, 2025

View reviewed changes

josuah commented Apr 26, 2025

View reviewed changes

josuah mentioned this pull request Apr 26, 2025

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

Merged

josuah added the In progress For PRs: is work in progress and should not be merged yet. For issues: Is being worked on label Apr 26, 2025

josuah commented Apr 27, 2025

View reviewed changes

include/zephyr/drivers/video-controls.h Outdated Show resolved Hide resolved

include/zephyr/drivers/video-controls.h Outdated Show resolved Hide resolved

include/zephyr/drivers/video-controls.h Outdated Show resolved Hide resolved

josuah force-pushed the pr-video-more-ctrls branch from f74cc53 to ab35f97 Compare April 27, 2025 01:07

josuah removed the In progress For PRs: is work in progress and should not be merged yet. For issues: Is being worked on label Apr 27, 2025

ngphibang approved these changes Apr 27, 2025

View reviewed changes

josuah requested a review from loicpoulain April 28, 2025 00:31

tmon-nordic approved these changes May 16, 2025

View reviewed changes

kartben merged commit 084f0ac into zephyrproject-rtos:main May 16, 2025
32 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drivers: video: controls: add the BASE and CAMERA controls#88924

drivers: video: controls: add the BASE and CAMERA controls#88924
kartben merged 1 commit intozephyrproject-rtos:mainfrom
tinyvision-ai-inc:pr-video-more-ctrls

josuah commented Apr 22, 2025 •

edited

Loading

josuah Apr 23, 2025

ngphibang Apr 23, 2025

josuah Apr 23, 2025 •

edited

Loading

josuah Apr 23, 2025

ngphibang Apr 23, 2025

Uh oh!

loicpoulain commented Apr 23, 2025

ngphibang left a comment

ngphibang Apr 23, 2025

ngphibang Apr 23, 2025

ngphibang commented Apr 23, 2025 •

edited

Loading

josuah commented Apr 23, 2025

josuah commented Apr 23, 2025

josuah commented Apr 23, 2025

josuah left a comment •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

josuah left a comment

ngphibang commented Apr 26, 2025

josuah left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Labels

5 participants

		enum video_exposure_auto_type {
		enum video_exposure_auto {

		enum video_exposure_auto_type {
		enum video_exposure_auto {

Conversation

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

Uh oh!

josuah Apr 23, 2025

Choose a reason for hiding this comment

ngphibang Apr 23, 2025

Choose a reason for hiding this comment

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

Uh oh!

Choose a reason for hiding this comment

josuah Apr 23, 2025

Choose a reason for hiding this comment

ngphibang Apr 23, 2025

Choose a reason for hiding this comment

Uh oh!

loicpoulain commented Apr 23, 2025

ngphibang left a comment

Choose a reason for hiding this comment

ngphibang Apr 23, 2025

Choose a reason for hiding this comment

ngphibang Apr 23, 2025

Choose a reason for hiding this comment

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

Uh oh!

josuah commented Apr 23, 2025

josuah commented Apr 23, 2025

josuah commented Apr 23, 2025

josuah left a comment • 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

josuah left a comment

Choose a reason for hiding this comment

ngphibang commented Apr 26, 2025

josuah left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Labels

5 participants

josuah commented Apr 22, 2025 •

edited

Loading

josuah Apr 23, 2025 •

edited

Loading

ngphibang commented Apr 23, 2025 •

edited

Loading

josuah left a comment •

edited

Loading