video: selection api (crop/compose) addition (api/shell/sample) + dcmipp implementation #91878

avolmat-st · 2025-06-18T22:09:38Z

This PR is still a WIP but should already give a good idea of the proposed SELECTION API allowing to the get capabilities of crop/compose as well as control it.
This is based on the Linux V4L2 SELECTION API (available in BSD-Clause): https://docs.kernel.org/6.6/userspace-api/media/v4l/selection-api.html

In order to give a better idea, I've also implemented the shell part as well as DCMIPP driver part (ongoing since only pixel pipes done for the time being, still need a few more code to also add the dump pipe support).

Basically, the setting a crop means that only part of the input is taken into account. The selected crop becomes the default compose size.
Setting a compose size means that the area selected via the crop would get modified (down or up) in order to lead to the rectangle defined by the compose. This lead to the output format at the end.

This leads for example to stuff as what can be seen in dcmipp modifications which is:

setting a crop lead to also setting the compose to the same rectangle and the output format width/height as well
setting a compose does not impact the crop but compose width/height also become the output format width/height
in such condition, set_fmt is mainly used to set the pixelformat, and the driver needs to check that the width/height of set_fmt do match with compose width/height.

In order to give a better idea I've modified the capture sample by adding new CONFIG_ in order to set the crop area and the sample app will, if available in the driver try to set the crop and do the compose to match with the requested output format.

Example of shell output, using DCMIPP driver on the STM32N6:

get output format upon start

uart:~$ video format endpoint@1 out
Current format: 'pRAA' 2592x1944 (0 bytes per frame)
min vbuf count: 1
min line count: 4294967295
max line count: 4294967295
	[0]: 'pRAA' (10 bits per pixel)
		Size: Discrete: 2592x1944 (6298560 bytes per frame)
			Interval: Discrete 33 ms (30 FPS)

Get current crop and apply a new one

uart:~$ video selection endpoint@1 out crop
	selection target: crop: (0,0)/2592x1944
uart:~$ video selection endpoint@1 out crop (10,10)/2400x1440
	selection target: crop: (10,10)/2400x1440
uart:~$ video selection endpoint@1 out crop
	selection target: crop: (10,10)/2400x1440
uart:~$ video format endpoint@1 out
Current format: 'pRAA' 2400x1440 (4331520 bytes per frame)
min vbuf count: 1
min line count: 4294967295
max line count: 4294967295
	[0]: 'pRAA' (10 bits per pixel)
		Size: Discrete: 2592x1944 (6298560 bytes per frame)
			Interval: Discrete 33 ms (30 FPS)

Set a compose to perform downscale

uart:~$ video selection endpoint@1 out compose
	selection target: compose: (10,10)/2400x1440
                                                     ^^^^^
                                                     this is actually wrong, a but within the DCMIPP code I'll fix it in the next push
uart:~$ video selection endpoint@1 out compose (0,0)/800x480
	selection target: compose: (0,0)/800x480
uart:~$ video selection endpoint@1 out compose
	selection target: compose: (0,0)/800x480
uart:~$ video format endpoint@1 out
Current format: 'pRAA' 800x480 (483840 bytes per frame)
min vbuf count: 1
min line count: 4294967295
max line count: 4294967295
	[0]: 'pRAA' (10 bits per pixel)
		Size: Discrete: 2592x1944 (6298560 bytes per frame)
			Interval: Discrete 33 ms (30 FPS)

Upon performing crop / compose the DCMIPP will now generate a 800x480 frame which is a downscale from the a rectangle of 2400x1440 starting at offset 10,10 of an original sensor frame of 2592x1944

Copilot

Pull Request Overview

Adds support for crop and compose selection in the video subsystem, with sample, shell, and DCMIPP driver updates.

Introduce video_selection API in video.h for crop/compose control
Extend the capture sample and Kconfig to configure source crop and compose
Implement selection handling in the shell and STM32 DCMIPP driver

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
samples/drivers/video/capture/src/main.c	Apply optional crop and compose settings in sample
samples/drivers/video/capture/Kconfig	Add `VIDEO_SOURCE_CROP_*` configuration options
include/zephyr/drivers/video.h	Define `video_selection` structs and APIs
drivers/video/video_stm32_dcmipp.c	Implement crop/compose logic and API callbacks
drivers/video/video_shell.c	Add shell commands and parsing for selection

Comments suppressed due to low confidence (2)

drivers/video/video_stm32_dcmipp.c:647

[nitpick] Grammar in error message: 'Failed to configuration pipe crop' should be 'Failed to configure pipe crop'.

		LOG_ERR("Failed to configuration pipe crop");

drivers/video/video_shell.c:1223

Help text and parser disagree on rectangle syntax: the code expects parentheses (<left>,<top>)/<width>x<height>, but the help message omits them. Align documentation with parser behavior.

	SHELL_CMD_ARG(selection, &dsub_video_format_dev,

samples/drivers/video/capture/src/main.c

include/zephyr/drivers/video.h

drivers/video/video_stm32_dcmipp.c

drivers/video/video_shell.c

kartben · 2025-06-19T07:39:39Z

drivers/video/video_shell.c

+
+	ret = video_shell_parse_in_out(sh, arg_in_out, &sel.type);
+	if (ret < 0) {
+		return -ret;


Suggested change

return -ret;

return ret;

Corrected. Same issue within the cmd_video_format function at same place (I copied code from there). I add a commit to fix this in the cmd_video_format function as well.

kartben · 2025-06-19T07:40:18Z

include/zephyr/drivers/video.h

+	__ASSERT_NO_MSG(sel != NULL);
+
+	api = (const struct video_driver_api *)dev->api;
+	if (api->set_selection == NULL) {


Suggested change

if (api->set_selection == NULL) {

if (api->get_selection == NULL) {

Oups, bad copy & paste. Corrected.

kartben · 2025-06-19T07:41:52Z

drivers/video/video_shell.c

+static int video_shell_selection_parse_rect(const struct shell *sh, char const *arg_rect,
+					    struct video_rect *rect)
+{
+	char *end_size = (char *)arg_rect;


please don't cast the const away, as per coding guidelines / MISRA

kartben · 2025-06-19T07:42:39Z

drivers/video/video_stm32_dcmipp.c

+
+	ret = HAL_DCMIPP_PIPE_SetCropConfig(&dcmipp->hdcmipp, pipe->id, &crop_cfg);
+	if (ret != HAL_OK) {
+		LOG_ERR("Failed to configuration pipe crop");


Suggested change

LOG_ERR("Failed to configuration pipe crop");

LOG_ERR("Failed to configure pipe crop");

josuah

Thank you for introducing this API! This will be convenient for image sensors as well, which I suppose will use the same API for their cropping region since it is not

I asked a few questions but it seems like being ready. Convenient to have V4L2 as a guideline!

I will give another cycle after your feedback, to make sure I understand what I review. Thank you!

include/zephyr/drivers/video.h

drivers/video/video_stm32_dcmipp.c

drivers/video/video_shell.c

samples/drivers/video/capture/src/main.c

josuah · 2025-06-19T10:14:14Z

For reference, the linux-side equivalent:

avolmat-st · 2025-06-19T20:53:43Z

First force-push to see comments update.
I will shortly push a new version with DCMIPP selection support on the dump pipe.

josuah · 2025-06-19T21:38:24Z

include/zephyr/drivers/video.h

+ *
+ * @param dev Pointer to the device structure for the driver instance.
+ * @param sel Pointer to a video selection structure
+ *


[non-blocking]

Suggested change

*

* Some targets are inter-dependents. For instance, setting a @ref VIDEO_SEL_TGT_CROP will

* reset @ref VIDEO_SEL_TGT_COMPOSE to the same size when both are supported.

Here I propose some minimum hint for users who can't figure out why their firmware work only when calling functions in different order leads to different results. Overkill? Better just point at the V4L2 doc?

Thanks for the explanations earlier!

Addition of selection APIs (set/get) in order to be able to control the crop and compose of a video device. Signed-off-by: Alain Volmat <[email protected]>

avolmat-st · 2025-06-20T11:59:59Z

Update the branch to take into consideration the comment from @josuah and also add support for the pipe0 (dump).

This update introduce an important modification in behavior compared to previous push in that set_selection(crop) and set_selection(compose) do not necessarily return an error if the parameters cannot be set and instead adjust those parameters. It would be later on up to the application to check the applied settings. This is I think an important behavior point to discuss about, what we would like to end up with. In this version, except when there is really an issue, the crop/compose do not return an error.
I thought this would be necessary especially in case of the pipe0 for which there are constraints in the crop value and I thought it would be easier for an application to get the setting updated rather than just having an error. However this is an open point, since here we are talking about FW for which usually the system is quite fixed so maybe there is no such need and it would be better to just send back an error.

avolmat-st · 2025-06-20T16:21:37Z

Thank you I understand better and agree.

If the information needed to make a choice is in the driver, then the driver has to expose that (is it even possible?) or choose and tell what it chose.

Yes, I think this depends on the HW so hard for an application to know that. That's why I chose to have the driver update the requested crop/compose and let the application know what has been set. But this implie that application check again after getting back the value and take some additional action if the applied setting aren't good for the application.

Such behavior is ok I think for a Linux application, but I am not sure if this match well what constraint of a Zephyr application, hence I am still wondering if this should be just "check if this can be applied to the HW and simply reject it if not".

josuah

Nothing blocking left I could find! Thank you for the modifications!

josuah · 2025-06-20T15:55:22Z

drivers/video/video_stm32_dcmipp.c

+		/* Compose not available on Pipe0 */
+		if (pipe->id == DCMIPP_PIPE0) {
+			sel->rect = pipe->crop;
+			goto out;


Should ret = -EINVAL be set here?

It depends, this is related to the discussion we are having about returning errors or not.
Here, since there is no compose, basically the applied compose will anyway always be the same as the crop.
A bit similar to when we do a set_frmival on a sensor which cannot change the framerate, we keep returning back the fixed framerate of the sensor.

josuah · 2025-06-20T15:56:33Z

drivers/video/video_stm32_dcmipp.c

+		if (sel->rect.left != 0) {
+			sel->rect.left = 0;
+		}
+		if (sel->rect.top != 0) {
+			sel->rect.top = 0;
+		}


[non-blocking]
Is the if needed? It seems like a no-op:
Calling sel->rect.left = 0; unconditionally would have the same effect.

it set left and/or top to 0 if it not 0. if it is already 0 it won't do anything. But yes, always setting left and top to 0 would make the code easier to read/understand I think.

josuah · 2025-06-20T16:20:12Z

samples/drivers/video/capture/src/main.c

+	if (err == 0) {
+		if (sel.rect.width != fmt.width || sel.rect.height != fmt.height) {
+			sel.target = VIDEO_SEL_TGT_COMPOSE;
+			sel.rect.left = 0;
+			sel.rect.top = 0;
+			sel.rect.width = fmt.width;
+			sel.rect.height = fmt.height;
+			err = video_set_selection(video_dev, &sel);
+			if (err != -ENOSYS && err < 0) {
+				LOG_ERR("Unable to set selection compose");
+				return 0;
+			}
+		}
+	} else if (err != -ENOSYS && err < 0) {
+		LOG_ERR("Unable to get selection crop");
+		return 0;
+	}


[non-blocking]
Proposal for making the error-handling idioms appear, just a minor nit:

err = video_get_selection(); // if (err < 0 && err != -ENOSYS) { // well-known C error handling idiom LOG_ERR("That did not work"); // return 0; // } // if (err == 0) { err = video_set_selection(); // if (err < 0 && err != -ENOSYS) { // well-known C error handling idiom LOG_ERR("That did not work"); // return 0; // } // }

yeah indeed, don't know why I came up with such hard to follow pattern ;(

Sometimes what is logical to write is not the same as what is logical to read. I got myself tricked in this exactly earlier.

samples/drivers/video/capture/src/main.c

ngphibang

LGTM except the constraint imposed on the sample where I think it should be up to the driver to decide.

Thanks for adding this API. The selection API looks simple enough but the complexity is left in the driver implementation and user's usage, especially its interaction with get/set_format(), e.g. what if format is set first, selection is set first, size does not mismatch, etc. It is left to the driver to decide to tightly couple with set_fmt() or simply return error. And users should always read back their original settings to decide ...

ngphibang · 2025-06-20T20:01:19Z

samples/drivers/video/capture/src/main.c

+		sel.rect.left, sel.rect.top, sel.rect.width, sel.rect.height);
+#endif
+
+#if CONFIG_VIDEO_FRAME_HEIGHT || CONFIG_VIDEO_FRAME_WIDTH


nit: In a function, it is recommended to use if (IS_ENABLED()) but it is currently not consistent across the sample

This would also solve the CI issue.

Done. Added an additionnal prior commit to use if (IS_ENABLED(CONFIG_VIDEO_FRAME_HEIGHT) ... instead of preprocessor.

samples/drivers/video/capture/src/main.c

Add possibility to perform crop on all pipes and compose (downscale) on pixel pipes (endpoint zephyrproject-rtos#1 and endpoint zephyrproject-rtos#2). Rework the code in order to move the downscale control from the set_fmt into the set_selection (compose). Signed-off-by: Alain Volmat <[email protected]>

Add shell handling in order to interact with drivers via the set/get_selection APIs allowing to get/set crop / compose and get HW capabilities such as crop bound / compose bound or native size. Signed-off-by: Alain Volmat <[email protected]>

Correct cmd_video_format return value in case of video_shell_parse_in_out returns a negative value. Signed-off-by: Alain Volmat <[email protected]>

avolmat-st · 2025-06-23T12:34:01Z

Gave up usage of if (IF_ENABLED(...)) since those CONFIG_VIDEO_ .... are not y/n config but values and using them would lead to just never entering into the if true statement.

Demonstrate the crop/compose API by introducing 4 new CONFIG options in order to define the crop area. Moreover, if the selection API is available and if the targetted size is different from the current crop size, then try to apply a compose in order to reach the targetted format size. Signed-off-by: Alain Volmat <[email protected]>

sonarqubecloud · 2025-06-23T13:03:42Z

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

comments addressed

drivers/video/video_stm32_dcmipp.c

GeorgeCGV · 2025-06-26T06:58:59Z

drivers/video/video_stm32_dcmipp.c

+	case VIDEO_SEL_TGT_COMPOSE:
+		sel->rect = pipe->compose;
+		break;
+	case VIDEO_SEL_TGT_CROP_BOUND:


Minor:

Suggested change

case VIDEO_SEL_TGT_CROP_BOUND:

case VIDEO_SEL_TGT_CROP_BOUND:

__fallthrough;

Add a mention to the video_set_selection() and video_get_selection() introduced in zephyrproject-rtos#91878. Signed-off-by: Josuah Demangeon <[email protected]>

Add a mention to the video_set_selection() and video_get_selection() introduced in #91878. Signed-off-by: Josuah Demangeon <[email protected]>

Add a mention to the video_set_selection() and video_get_selection() introduced in zephyrproject-rtos#91878. Signed-off-by: Josuah Demangeon <[email protected]>

Add a mention to the video_set_selection() and video_get_selection() introduced in zephyrproject-rtos#91878. (cherry picked from commit d491496) Original-Signed-off-by: Josuah Demangeon <[email protected]> GitOrigin-RevId: d491496 Cr-Build-Id: 8709689119827444849 Cr-Build-Url: https://cr-buildbucket.appspot.com/build/8709689119827444849 Copybot-Job-Name: zephyr-main-copybot-downstream Change-Id: I73af2c5c58232bf1086837f435bafd40928478e6 Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/zephyr/+/6725779 Commit-Queue: ChromeOS Copybot <[email protected]> Bot-Commit: ChromeOS Copybot <[email protected]> Tested-by: ChromeOS Copybot <[email protected]>

avolmat-st requested review from josuah and ngphibang June 18, 2025 22:10

kartben requested a review from Copilot June 19, 2025 07:37

Copilot AI reviewed Jun 19, 2025

View reviewed changes

kartben previously requested changes Jun 19, 2025

View reviewed changes

josuah reviewed Jun 19, 2025

View reviewed changes

avolmat-st force-pushed the pr_video_selection_api branch from c7987b2 to ade64c1 Compare June 19, 2025 20:52

josuah reviewed Jun 19, 2025

View reviewed changes

video: api: addition of video_set/get_selection APIs

b01240b

Addition of selection APIs (set/get) in order to be able to control the crop and compose of a video device. Signed-off-by: Alain Volmat <[email protected]>

avolmat-st force-pushed the pr_video_selection_api branch 2 times, most recently from 1b8693a to 122b221 Compare June 20, 2025 11:59

avolmat-st marked this pull request as ready for review June 20, 2025 11:59

github-actions bot added area: Samples Samples area: Video Video subsystem platform: STM32 ST Micro STM32 labels Jun 20, 2025

github-actions bot requested review from FRASTM, GeorgeCGV, djiatsaf-st, erwango, etienne-lms, gautierg-st, loicpoulain, marwaiehm-st, mathieuchopstm and nashif June 20, 2025 12:00

github-actions bot assigned josuah and ngphibang Jun 20, 2025

josuah requested a review from kartben June 20, 2025 15:08

josuah previously approved these changes Jun 20, 2025

View reviewed changes

josuah reviewed Jun 20, 2025

View reviewed changes

samples/drivers/video/capture/src/main.c Show resolved Hide resolved

ngphibang reviewed Jun 20, 2025

View reviewed changes

This was referenced Jun 23, 2025

Drivers:video:GC2145 - Wish there was more control over image resolution and windowing. #91968

Open

video: add more resolutions to GC2145 #91975

Closed

Alain Volmat added 3 commits June 23, 2025 13:00

video: shell: correct incorrect error return value in cmd_video_format

476f7af

Correct cmd_video_format return value in case of video_shell_parse_in_out returns a negative value. Signed-off-by: Alain Volmat <[email protected]>

avolmat-st dismissed josuah’s stale review via f1b088c June 23, 2025 12:32

avolmat-st force-pushed the pr_video_selection_api branch from fd2f0c7 to f1b088c Compare June 23, 2025 12:32

avolmat-st force-pushed the pr_video_selection_api branch from f1b088c to 0b9e6db Compare June 23, 2025 12:38

ngphibang approved these changes Jun 23, 2025

View reviewed changes

josuah approved these changes Jun 23, 2025

View reviewed changes

dkalowsk merged commit 174dead into zephyrproject-rtos:main Jun 26, 2025
26 checks passed

GeorgeCGV reviewed Jun 26, 2025

View reviewed changes

drivers/video/video_stm32_dcmipp.c Show resolved Hide resolved

GeorgeCGV reviewed Jun 26, 2025

View reviewed changes

josuah mentioned this pull request Jun 28, 2025

drivers: video: API enhancement #72959

Open

avolmat-st mentioned this pull request Jun 30, 2025

video: fix missing crop/compose handling in capture_to_lvgl sample & add missing conf files for stm32n657 fsbl variant #92410

Merged

danieldegrasse pushed a commit that referenced this pull request Jul 10, 2025

doc: releases: mention the video selection API functions

d491496

Add a mention to the video_set_selection() and video_get_selection() introduced in #91878. Signed-off-by: Josuah Demangeon <[email protected]>

	if (api->set_selection == NULL) {
	if (api->get_selection == NULL) {

	LOG_ERR("Failed to configuration pipe crop");
	LOG_ERR("Failed to configure pipe crop");

	*
	* Some targets are inter-dependents. For instance, setting a @ref VIDEO_SEL_TGT_CROP will
	* reset @ref VIDEO_SEL_TGT_COMPOSE to the same size when both are supported.

	case VIDEO_SEL_TGT_CROP_BOUND:
	case VIDEO_SEL_TGT_CROP_BOUND:
	__fallthrough;

video: selection api (crop/compose) addition (api/shell/sample) + dcmipp implementation #91878

video: selection api (crop/compose) addition (api/shell/sample) + dcmipp implementation #91878

Uh oh!

Conversation

avolmat-st commented Jun 18, 2025 • edited by josuah Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

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!

avolmat-st Jun 19, 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!

josuah left a comment

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!

Uh oh!

josuah commented Jun 19, 2025

Uh oh!

avolmat-st commented Jun 19, 2025

Uh oh!

josuah Jun 19, 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!

avolmat-st commented Jun 20, 2025

Uh oh!

avolmat-st commented Jun 20, 2025

Uh oh!

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!

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!

Uh oh!

ngphibang 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!

Choose a reason for hiding this comment

avolmat-st commented Jun 18, 2025 •

edited by josuah

Loading

avolmat-st Jun 19, 2025 •

edited

Loading

josuah Jun 19, 2025 •

edited

Loading

josuah left a comment •

edited

Loading

ngphibang left a comment •

edited

Loading