[PATCH v2 0/2] Support V4L2_CTRL_TYPE_RECT and V4L2_CTRL_WHICH_MIN/MAX_VAL

[PATCH v2 0/2] Support V4L2_CTRL_TYPE_RECT and V4L2_CTRL_WHICH_MIN/MAX_VAL

[ming.qian]

From: Ming Qian <ming.qian@oss.nxp.com>

Hi!

This patchset adds basic support for V4L2_CTRL_TYPE_RECT
and V4L2_CTRL_WHICH_MIN/MAX_VAL in v4l2-ctl and v4l2-compliance.

The corresponding linux kernel patchset can be found at
https://lore.kernel.org/lkml/20241030022134.1098589-1-ming.qian@oss.nxp.com/

Ming Qian (1):
  v4l-utils: Define V4L2_CTRL_TYPE_RECT

Yunke Cao (2):
  v4l2-ctl: Support V4L2_CTRL_TYPE_RECT
  v4l2-utils: Support V4L2_CTRL_WHICH_MIN/MAX_VAL

 include/linux/videodev2.h                    |  5 ++++
 utils/common/v4l2-info.cpp                   |  1 +
 utils/v4l2-compliance/v4l2-test-controls.cpp | 24 ++++++++++++++++++++
 utils/v4l2-ctl/v4l2-ctl-common.cpp           | 12 ++++++++++
 4 files changed, 42 insertions(+)

-- 
2.43.0-rc1

[PATCH v2 1/3] v4l-utils: Define V4L2_CTRL_TYPE_RECT

[ming.qian]

From: Ming Qian <ming.qian@oss.nxp.com>

Define V4L2_CTRL_TYPE_RECT and V4L2_CTRL_WHICH_MIN/MAX_VAL
in videodev2.h
This is a temporary patch

Signed-off-by: Ming Qian <ming.qian@oss.nxp.com>
---
 include/linux/videodev2.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
index 23db72afdf60..6aae7e10e90c 100644
--- a/include/linux/videodev2.h
+++ b/include/linux/videodev2.h
@@ -1816,6 +1816,7 @@ struct v4l2_ext_control {
 		struct v4l2_ctrl_av1_film_grain *p_av1_film_grain;
 		struct v4l2_ctrl_hdr10_cll_info *p_hdr10_cll_info;
 		struct v4l2_ctrl_hdr10_mastering_display *p_hdr10_mastering_display;
+		struct v4l2_rect *p_rect;
 		void *ptr;
 	} __attribute__ ((packed));
 } __attribute__ ((packed));
@@ -1840,6 +1841,8 @@ struct v4l2_ext_controls {
 #define V4L2_CTRL_WHICH_CUR_VAL   0
 #define V4L2_CTRL_WHICH_DEF_VAL   0x0f000000
 #define V4L2_CTRL_WHICH_REQUEST_VAL 0x0f010000
+#define V4L2_CTRL_WHICH_MIN_VAL   0x0f020000
+#define V4L2_CTRL_WHICH_MAX_VAL   0x0f030000
 
 enum v4l2_ctrl_type {
 	V4L2_CTRL_TYPE_INTEGER	     = 1,
@@ -1858,6 +1861,7 @@ enum v4l2_ctrl_type {
 	V4L2_CTRL_TYPE_U16	     = 0x0101,
 	V4L2_CTRL_TYPE_U32	     = 0x0102,
 	V4L2_CTRL_TYPE_AREA          = 0x0106,
+	V4L2_CTRL_TYPE_RECT	     = 0x0107,
 
 	V4L2_CTRL_TYPE_HDR10_CLL_INFO		= 0x0110,
 	V4L2_CTRL_TYPE_HDR10_MASTERING_DISPLAY	= 0x0111,
@@ -1946,6 +1950,7 @@ struct v4l2_querymenu {
 #define V4L2_CTRL_FLAG_EXECUTE_ON_WRITE	0x0200
 #define V4L2_CTRL_FLAG_MODIFY_LAYOUT	0x0400
 #define V4L2_CTRL_FLAG_DYNAMIC_ARRAY	0x0800
+#define V4L2_CTRL_FLAG_HAS_WHICH_MIN_MAX 0x1000
 
 /*  Query flags, to be ORed with the control ID */
 #define V4L2_CTRL_FLAG_NEXT_CTRL	0x80000000
-- 
2.43.0-rc1

[PATCH v2 2/3] v4l2-ctl: Support V4L2_CTRL_TYPE_RECT

[ming.qian]

From: Yunke Cao <yunkec@google.com>

Tested with VIVID

 ./v4l2-ctl -C rect -d 0
rect: 300x400@200x100

 ./v4l2-ctl -c rect=1000x2000@0x0
 ./v4l2-ctl -C rect -d 0
rect: 1000x2000@0x0

Signed-off-by: Yunke Cao <yunkec@google.com>
Signed-off-by: Ming Qian <ming.qian@oss.nxp.com>
---
 utils/v4l2-ctl/v4l2-ctl-common.cpp | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/utils/v4l2-ctl/v4l2-ctl-common.cpp b/utils/v4l2-ctl/v4l2-ctl-common.cpp
index 40667575fcc7..538e1951cf81 100644
--- a/utils/v4l2-ctl/v4l2-ctl-common.cpp
+++ b/utils/v4l2-ctl/v4l2-ctl-common.cpp
@@ -614,6 +614,10 @@ static void print_value(int fd, const v4l2_query_ext_ctrl &qc, const v4l2_ext_co
 		case V4L2_CTRL_TYPE_AREA:
 			printf("%dx%d", ctrl.p_area->width, ctrl.p_area->height);
 			break;
+		case V4L2_CTRL_TYPE_RECT:
+			printf("%ux%u@%dx%d", ctrl.p_rect->width, ctrl.p_rect->height,
+			       ctrl.p_rect->left, ctrl.p_rect->top);
+			break;
 		default:
 			printf("unsupported payload type");
 			break;
@@ -702,6 +706,9 @@ static void print_qctrl(int fd, const v4l2_query_ext_ctrl &qc,
 	case V4L2_CTRL_TYPE_AREA:
 		printf("%31s %#8.8x (area)   :", s.c_str(), qc.id);
 		break;
+	case V4L2_CTRL_TYPE_RECT:
+		printf("%31s %#8.8x (rect)   :", s.c_str(), qc.id);
+		break;
 	case V4L2_CTRL_TYPE_HDR10_CLL_INFO:
 		printf("%31s %#8.8x (hdr10-cll-info):", s.c_str(), qc.id);
 		break;
@@ -1279,6 +1286,11 @@ void common_set(cv4l_fd &_fd)
 					sscanf(set_ctrl.second.c_str(), "%ux%u",
 					       &ctrl.p_area->width, &ctrl.p_area->height);
 					break;
+				case V4L2_CTRL_TYPE_RECT:
+					sscanf(set_ctrl.second.c_str(), "%ux%u@%dx%d",
+					       &ctrl.p_rect->width, &ctrl.p_rect->height,
+					       &ctrl.p_rect->left, &ctrl.p_rect->top);
+					break;
 				default:
 					fprintf(stderr, "%s: unsupported payload type\n",
 							qc.name);
-- 
2.43.0-rc1

[PATCH v2 3/3] v4l2-utils: Support V4L2_CTRL_WHICH_MIN/MAX_VAL

[ming.qian]

From: Yunke Cao <yunkec@google.com>

Add string name for V4L2_CTRL_FLAG_HAS_WHICH_MIN_MAX.

Test that V4L2_CTRL_WHICH_MIN/MAX_VAL behaves as expected.
1. Calling G_EXT_CTRLS successes when V4L2_CTRL_FLAG_HAS_WHICH_MIN_MAX is set
   and returns -ENVALID if the flag is not set.
2. S_EXT_CTRLS and TRY_EXT_CTRLS always return fail.

Tested with VIVID:

./v4l2-ctl -l -d 0
...
		     area 0x0098f90b (area)   : value=1000x2000 flags=has-payload
read_only_integer_32_bits 0x0098f90c (int)    : min=0 max=255 step=1 default=0 value=0 flags=read-only, has-min-max
        u32_dynamic_array 0x0098f90d (u32)    : min=10 max=90 step=1 default=50 elems=1 dims=[100] flags=has-payload, dynamic-array
	   u8_pixel_array 0x0098f90e (u8)     : min=0 max=255 step=1 default=128 dims=[640][368] flags=has-payload
      s32_2_element_array 0x0098f90f (int)    : min=-10 max=10 step=1 default=2 dims=[2] flags=has-payload, has-min-max
      s64_5_element_array 0x0098f910 (int64)  : min=-10 max=10 step=1 default=4 dims=[5] flags=has-payload, has-min-max
		     rect 0x0098f911 (rect)   : value=300x400@200x100 flags=has-payload, has-min-max
...
./v4l2-compliance -d 0
...
Total for vivid device /dev/video0: 125, Succeeded: 125, Failed: 0, Warnings: 0

Signed-off-by: Yunke Cao <yunkec@google.com>
Signed-off-by: Ming Qian <ming.qian@oss.nxp.com>
---
 utils/common/v4l2-info.cpp                   |  1 +
 utils/v4l2-compliance/v4l2-test-controls.cpp | 24 ++++++++++++++++++++
 2 files changed, 25 insertions(+)

diff --git a/utils/common/v4l2-info.cpp b/utils/common/v4l2-info.cpp
index 7dd7e708eb5e..02dfd6da0248 100644
--- a/utils/common/v4l2-info.cpp
+++ b/utils/common/v4l2-info.cpp
@@ -538,6 +538,7 @@ std::string ctrlflags2s(__u32 flags)
 		{ V4L2_CTRL_FLAG_EXECUTE_ON_WRITE, "execute-on-write" },
 		{ V4L2_CTRL_FLAG_MODIFY_LAYOUT, "modify-layout" },
 		{ V4L2_CTRL_FLAG_DYNAMIC_ARRAY, "dynamic-array" },
+		{ V4L2_CTRL_FLAG_HAS_WHICH_MIN_MAX, "has-min-max" },
 		{ 0, nullptr }
 	};
 	return flags2s(flags, def);
diff --git a/utils/v4l2-compliance/v4l2-test-controls.cpp b/utils/v4l2-compliance/v4l2-test-controls.cpp
index e87a1af96406..a0e8bc19aafd 100644
--- a/utils/v4l2-compliance/v4l2-test-controls.cpp
+++ b/utils/v4l2-compliance/v4l2-test-controls.cpp
@@ -971,6 +971,21 @@ int testExtendedControls(struct node *node)
 		if (is_vivid && ctrl.id == VIVID_CID_U32_DYN_ARRAY &&
 		    checkVividDynArray(node, ctrl, qctrl))
 			return fail("dynamic array tests failed\n");
+
+		ctrls.which = V4L2_CTRL_WHICH_MIN_VAL;
+		ret = doioctl(node, VIDIOC_G_EXT_CTRLS, &ctrls);
+		if (qctrl.flags & V4L2_CTRL_FLAG_HAS_WHICH_MIN_MAX)
+			fail_on_test_val(ret, ret);
+		else
+			fail_on_test_val(ret != EINVAL, ret);
+
+		ctrls.which = V4L2_CTRL_WHICH_MAX_VAL;
+		ret = doioctl(node, VIDIOC_G_EXT_CTRLS, &ctrls);
+		if (qctrl.flags & V4L2_CTRL_FLAG_HAS_WHICH_MIN_MAX)
+			fail_on_test_val(ret, ret);
+		else
+			fail_on_test_val(ret != EINVAL, ret);
+
 		if (qctrl.flags & V4L2_CTRL_FLAG_HAS_PAYLOAD)
 			delete [] ctrl.string;
 		ctrl.string = nullptr;
@@ -1082,6 +1097,15 @@ int testExtendedControls(struct node *node)
 	fail_on_test(!doioctl(node, VIDIOC_S_EXT_CTRLS, &ctrls));
 	fail_on_test(!doioctl(node, VIDIOC_TRY_EXT_CTRLS, &ctrls));
 	fail_on_test(doioctl(node, VIDIOC_G_EXT_CTRLS, &ctrls));
+
+	ctrls.which = V4L2_CTRL_WHICH_MIN_VAL;
+	fail_on_test(!doioctl(node, VIDIOC_S_EXT_CTRLS, &ctrls));
+	fail_on_test(!doioctl(node, VIDIOC_TRY_EXT_CTRLS, &ctrls));
+
+	ctrls.which = V4L2_CTRL_WHICH_MAX_VAL;
+	fail_on_test(!doioctl(node, VIDIOC_S_EXT_CTRLS, &ctrls));
+	fail_on_test(!doioctl(node, VIDIOC_TRY_EXT_CTRLS, &ctrls));
+
 	return 0;
 }
 
-- 
2.43.0-rc1

Re: [PATCH v2 2/3] v4l2-ctl: Support V4L2_CTRL_TYPE_RECT

[Laurent Pinchart]

On Wed, Oct 30, 2024 at 11:43:06AM +0900, ming.qian@oss.nxp.com wrote:
> From: Yunke Cao <yunkec@google.com>
> 
> Tested with VIVID
> 
>  ./v4l2-ctl -C rect -d 0
> rect: 300x400@200x100
> 
>  ./v4l2-ctl -c rect=1000x2000@0x0
>  ./v4l2-ctl -C rect -d 0
> rect: 1000x2000@0x0
> 
> Signed-off-by: Yunke Cao <yunkec@google.com>
> Signed-off-by: Ming Qian <ming.qian@oss.nxp.com>
> ---
>  utils/v4l2-ctl/v4l2-ctl-common.cpp | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/utils/v4l2-ctl/v4l2-ctl-common.cpp b/utils/v4l2-ctl/v4l2-ctl-common.cpp
> index 40667575fcc7..538e1951cf81 100644
> --- a/utils/v4l2-ctl/v4l2-ctl-common.cpp
> +++ b/utils/v4l2-ctl/v4l2-ctl-common.cpp
> @@ -614,6 +614,10 @@ static void print_value(int fd, const v4l2_query_ext_ctrl &qc, const v4l2_ext_co
>  		case V4L2_CTRL_TYPE_AREA:
>  			printf("%dx%d", ctrl.p_area->width, ctrl.p_area->height);
>  			break;
> +		case V4L2_CTRL_TYPE_RECT:
> +			printf("%ux%u@%dx%d", ctrl.p_rect->width, ctrl.p_rect->height,

I find this notation ambiguous, it's not immediately clear when reading
10x10@20x20 if we're looking at a 10x10 rectangle positioned at (20,20)
or the other way around. media-ctl use (20,20)/10x10 which I think would
be a better notation.

> +			       ctrl.p_rect->left, ctrl.p_rect->top);
> +			break;
>  		default:
>  			printf("unsupported payload type");
>  			break;
> @@ -702,6 +706,9 @@ static void print_qctrl(int fd, const v4l2_query_ext_ctrl &qc,
>  	case V4L2_CTRL_TYPE_AREA:
>  		printf("%31s %#8.8x (area)   :", s.c_str(), qc.id);
>  		break;
> +	case V4L2_CTRL_TYPE_RECT:
> +		printf("%31s %#8.8x (rect)   :", s.c_str(), qc.id);
> +		break;
>  	case V4L2_CTRL_TYPE_HDR10_CLL_INFO:
>  		printf("%31s %#8.8x (hdr10-cll-info):", s.c_str(), qc.id);
>  		break;
> @@ -1279,6 +1286,11 @@ void common_set(cv4l_fd &_fd)
>  					sscanf(set_ctrl.second.c_str(), "%ux%u",
>  					       &ctrl.p_area->width, &ctrl.p_area->height);
>  					break;
> +				case V4L2_CTRL_TYPE_RECT:
> +					sscanf(set_ctrl.second.c_str(), "%ux%u@%dx%d",
> +					       &ctrl.p_rect->width, &ctrl.p_rect->height,
> +					       &ctrl.p_rect->left, &ctrl.p_rect->top);
> +					break;
>  				default:
>  					fprintf(stderr, "%s: unsupported payload type\n",
>  							qc.name);

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 2/3] v4l2-ctl: Support V4L2_CTRL_TYPE_RECT

[Hans Verkuil]

On 30/10/2024 10:03, Laurent Pinchart wrote:
> On Wed, Oct 30, 2024 at 11:43:06AM +0900, ming.qian@oss.nxp.com wrote:
>> From: Yunke Cao <yunkec@google.com>
>>
>> Tested with VIVID
>>
>>  ./v4l2-ctl -C rect -d 0
>> rect: 300x400@200x100
>>
>>  ./v4l2-ctl -c rect=1000x2000@0x0
>>  ./v4l2-ctl -C rect -d 0
>> rect: 1000x2000@0x0
>>
>> Signed-off-by: Yunke Cao <yunkec@google.com>
>> Signed-off-by: Ming Qian <ming.qian@oss.nxp.com>
>> ---
>>  utils/v4l2-ctl/v4l2-ctl-common.cpp | 12 ++++++++++++
>>  1 file changed, 12 insertions(+)
>>
>> diff --git a/utils/v4l2-ctl/v4l2-ctl-common.cpp b/utils/v4l2-ctl/v4l2-ctl-common.cpp
>> index 40667575fcc7..538e1951cf81 100644
>> --- a/utils/v4l2-ctl/v4l2-ctl-common.cpp
>> +++ b/utils/v4l2-ctl/v4l2-ctl-common.cpp
>> @@ -614,6 +614,10 @@ static void print_value(int fd, const v4l2_query_ext_ctrl &qc, const v4l2_ext_co
>>  		case V4L2_CTRL_TYPE_AREA:
>>  			printf("%dx%d", ctrl.p_area->width, ctrl.p_area->height);
>>  			break;
>> +		case V4L2_CTRL_TYPE_RECT:
>> +			printf("%ux%u@%dx%d", ctrl.p_rect->width, ctrl.p_rect->height,
> 
> I find this notation ambiguous, it's not immediately clear when reading
> 10x10@20x20 if we're looking at a 10x10 rectangle positioned at (20,20)
> or the other way around. media-ctl use (20,20)/10x10 which I think would
> be a better notation.

Good point, I agree.

Ming Qian, can you also update patch 1/4 of the kernel patch series to
use the same formatting when logging the V4L2_CTRL_TYPE_RECT value?

Regards,

	Hans

> 
>> +			       ctrl.p_rect->left, ctrl.p_rect->top);
>> +			break;
>>  		default:
>>  			printf("unsupported payload type");
>>  			break;
>> @@ -702,6 +706,9 @@ static void print_qctrl(int fd, const v4l2_query_ext_ctrl &qc,
>>  	case V4L2_CTRL_TYPE_AREA:
>>  		printf("%31s %#8.8x (area)   :", s.c_str(), qc.id);
>>  		break;
>> +	case V4L2_CTRL_TYPE_RECT:
>> +		printf("%31s %#8.8x (rect)   :", s.c_str(), qc.id);
>> +		break;
>>  	case V4L2_CTRL_TYPE_HDR10_CLL_INFO:
>>  		printf("%31s %#8.8x (hdr10-cll-info):", s.c_str(), qc.id);
>>  		break;
>> @@ -1279,6 +1286,11 @@ void common_set(cv4l_fd &_fd)
>>  					sscanf(set_ctrl.second.c_str(), "%ux%u",
>>  					       &ctrl.p_area->width, &ctrl.p_area->height);
>>  					break;
>> +				case V4L2_CTRL_TYPE_RECT:
>> +					sscanf(set_ctrl.second.c_str(), "%ux%u@%dx%d",
>> +					       &ctrl.p_rect->width, &ctrl.p_rect->height,
>> +					       &ctrl.p_rect->left, &ctrl.p_rect->top);
>> +					break;
>>  				default:
>>  					fprintf(stderr, "%s: unsupported payload type\n",
>>  							qc.name);
> 

Re: [PATCH v2 2/3] v4l2-ctl: Support V4L2_CTRL_TYPE_RECT

[Ming Qian(OSS)]

Hi Laurent,

On 2024/10/30 17:03, Laurent Pinchart wrote:
> On Wed, Oct 30, 2024 at 11:43:06AM +0900, ming.qian@oss.nxp.com wrote:
>> From: Yunke Cao <yunkec@google.com>
>>
>> Tested with VIVID
>>
>>   ./v4l2-ctl -C rect -d 0
>> rect: 300x400@200x100
>>
>>   ./v4l2-ctl -c rect=1000x2000@0x0
>>   ./v4l2-ctl -C rect -d 0
>> rect: 1000x2000@0x0
>>
>> Signed-off-by: Yunke Cao <yunkec@google.com>
>> Signed-off-by: Ming Qian <ming.qian@oss.nxp.com>
>> ---
>>   utils/v4l2-ctl/v4l2-ctl-common.cpp | 12 ++++++++++++
>>   1 file changed, 12 insertions(+)
>>
>> diff --git a/utils/v4l2-ctl/v4l2-ctl-common.cpp b/utils/v4l2-ctl/v4l2-ctl-common.cpp
>> index 40667575fcc7..538e1951cf81 100644
>> --- a/utils/v4l2-ctl/v4l2-ctl-common.cpp
>> +++ b/utils/v4l2-ctl/v4l2-ctl-common.cpp
>> @@ -614,6 +614,10 @@ static void print_value(int fd, const v4l2_query_ext_ctrl &qc, const v4l2_ext_co
>>   		case V4L2_CTRL_TYPE_AREA:
>>   			printf("%dx%d", ctrl.p_area->width, ctrl.p_area->height);
>>   			break;
>> +		case V4L2_CTRL_TYPE_RECT:
>> +			printf("%ux%u@%dx%d", ctrl.p_rect->width, ctrl.p_rect->height,
> 
> I find this notation ambiguous, it's not immediately clear when reading
> 10x10@20x20 if we're looking at a 10x10 rectangle positioned at (20,20)
> or the other way around. media-ctl use (20,20)/10x10 which I think would
> be a better notation.
> 

Thanks for the suggestions, I'll go ahead with this approach.

>> +			       ctrl.p_rect->left, ctrl.p_rect->top);
>> +			break;
>>   		default:
>>   			printf("unsupported payload type");
>>   			break;
>> @@ -702,6 +706,9 @@ static void print_qctrl(int fd, const v4l2_query_ext_ctrl &qc,
>>   	case V4L2_CTRL_TYPE_AREA:
>>   		printf("%31s %#8.8x (area)   :", s.c_str(), qc.id);
>>   		break;
>> +	case V4L2_CTRL_TYPE_RECT:
>> +		printf("%31s %#8.8x (rect)   :", s.c_str(), qc.id);
>> +		break;
>>   	case V4L2_CTRL_TYPE_HDR10_CLL_INFO:
>>   		printf("%31s %#8.8x (hdr10-cll-info):", s.c_str(), qc.id);
>>   		break;
>> @@ -1279,6 +1286,11 @@ void common_set(cv4l_fd &_fd)
>>   					sscanf(set_ctrl.second.c_str(), "%ux%u",
>>   					       &ctrl.p_area->width, &ctrl.p_area->height);
>>   					break;
>> +				case V4L2_CTRL_TYPE_RECT:
>> +					sscanf(set_ctrl.second.c_str(), "%ux%u@%dx%d",
>> +					       &ctrl.p_rect->width, &ctrl.p_rect->height,
>> +					       &ctrl.p_rect->left, &ctrl.p_rect->top);
>> +					break;
>>   				default:
>>   					fprintf(stderr, "%s: unsupported payload type\n",
>>   							qc.name);
> 

Re: [PATCH v2 2/3] v4l2-ctl: Support V4L2_CTRL_TYPE_RECT

[Ming Qian(OSS)]

On 2024/10/30 17:19, Hans Verkuil wrote:
> On 30/10/2024 10:03, Laurent Pinchart wrote:
>> On Wed, Oct 30, 2024 at 11:43:06AM +0900, ming.qian@oss.nxp.com wrote:
>>> From: Yunke Cao <yunkec@google.com>
>>>
>>> Tested with VIVID
>>>
>>>   ./v4l2-ctl -C rect -d 0
>>> rect: 300x400@200x100
>>>
>>>   ./v4l2-ctl -c rect=1000x2000@0x0
>>>   ./v4l2-ctl -C rect -d 0
>>> rect: 1000x2000@0x0
>>>
>>> Signed-off-by: Yunke Cao <yunkec@google.com>
>>> Signed-off-by: Ming Qian <ming.qian@oss.nxp.com>
>>> ---
>>>   utils/v4l2-ctl/v4l2-ctl-common.cpp | 12 ++++++++++++
>>>   1 file changed, 12 insertions(+)
>>>
>>> diff --git a/utils/v4l2-ctl/v4l2-ctl-common.cpp b/utils/v4l2-ctl/v4l2-ctl-common.cpp
>>> index 40667575fcc7..538e1951cf81 100644
>>> --- a/utils/v4l2-ctl/v4l2-ctl-common.cpp
>>> +++ b/utils/v4l2-ctl/v4l2-ctl-common.cpp
>>> @@ -614,6 +614,10 @@ static void print_value(int fd, const v4l2_query_ext_ctrl &qc, const v4l2_ext_co
>>>   		case V4L2_CTRL_TYPE_AREA:
>>>   			printf("%dx%d", ctrl.p_area->width, ctrl.p_area->height);
>>>   			break;
>>> +		case V4L2_CTRL_TYPE_RECT:
>>> +			printf("%ux%u@%dx%d", ctrl.p_rect->width, ctrl.p_rect->height,
>>
>> I find this notation ambiguous, it's not immediately clear when reading
>> 10x10@20x20 if we're looking at a 10x10 rectangle positioned at (20,20)
>> or the other way around. media-ctl use (20,20)/10x10 which I think would
>> be a better notation.
> 
> Good point, I agree.
> 
> Ming Qian, can you also update patch 1/4 of the kernel patch series to
> use the same formatting when logging the V4L2_CTRL_TYPE_RECT value?
> 
> Regards,
> 
> 	Hans

Yes, I will

> 
>>
>>> +			       ctrl.p_rect->left, ctrl.p_rect->top);
>>> +			break;
>>>   		default:
>>>   			printf("unsupported payload type");
>>>   			break;
>>> @@ -702,6 +706,9 @@ static void print_qctrl(int fd, const v4l2_query_ext_ctrl &qc,
>>>   	case V4L2_CTRL_TYPE_AREA:
>>>   		printf("%31s %#8.8x (area)   :", s.c_str(), qc.id);
>>>   		break;
>>> +	case V4L2_CTRL_TYPE_RECT:
>>> +		printf("%31s %#8.8x (rect)   :", s.c_str(), qc.id);
>>> +		break;
>>>   	case V4L2_CTRL_TYPE_HDR10_CLL_INFO:
>>>   		printf("%31s %#8.8x (hdr10-cll-info):", s.c_str(), qc.id);
>>>   		break;
>>> @@ -1279,6 +1286,11 @@ void common_set(cv4l_fd &_fd)
>>>   					sscanf(set_ctrl.second.c_str(), "%ux%u",
>>>   					       &ctrl.p_area->width, &ctrl.p_area->height);
>>>   					break;
>>> +				case V4L2_CTRL_TYPE_RECT:
>>> +					sscanf(set_ctrl.second.c_str(), "%ux%u@%dx%d",
>>> +					       &ctrl.p_rect->width, &ctrl.p_rect->height,
>>> +					       &ctrl.p_rect->left, &ctrl.p_rect->top);
>>> +					break;
>>>   				default:
>>>   					fprintf(stderr, "%s: unsupported payload type\n",
>>>   							qc.name);
>>
> 

Re: [PATCH v2 2/3] v4l2-ctl: Support V4L2_CTRL_TYPE_RECT

[Ming Qian(OSS)]

Hi Hans,


On 2024/10/30 17:19, Hans Verkuil wrote:
> On 30/10/2024 10:03, Laurent Pinchart wrote:
>> On Wed, Oct 30, 2024 at 11:43:06AM +0900, ming.qian@oss.nxp.com wrote:
>>> From: Yunke Cao <yunkec@google.com>
>>>
>>> Tested with VIVID
>>>
>>>   ./v4l2-ctl -C rect -d 0
>>> rect: 300x400@200x100
>>>
>>>   ./v4l2-ctl -c rect=1000x2000@0x0
>>>   ./v4l2-ctl -C rect -d 0
>>> rect: 1000x2000@0x0
>>>
>>> Signed-off-by: Yunke Cao <yunkec@google.com>
>>> Signed-off-by: Ming Qian <ming.qian@oss.nxp.com>
>>> ---
>>>   utils/v4l2-ctl/v4l2-ctl-common.cpp | 12 ++++++++++++
>>>   1 file changed, 12 insertions(+)
>>>
>>> diff --git a/utils/v4l2-ctl/v4l2-ctl-common.cpp b/utils/v4l2-ctl/v4l2-ctl-common.cpp
>>> index 40667575fcc7..538e1951cf81 100644
>>> --- a/utils/v4l2-ctl/v4l2-ctl-common.cpp
>>> +++ b/utils/v4l2-ctl/v4l2-ctl-common.cpp
>>> @@ -614,6 +614,10 @@ static void print_value(int fd, const v4l2_query_ext_ctrl &qc, const v4l2_ext_co
>>>   		case V4L2_CTRL_TYPE_AREA:
>>>   			printf("%dx%d", ctrl.p_area->width, ctrl.p_area->height);
>>>   			break;
>>> +		case V4L2_CTRL_TYPE_RECT:
>>> +			printf("%ux%u@%dx%d", ctrl.p_rect->width, ctrl.p_rect->height,
>>
>> I find this notation ambiguous, it's not immediately clear when reading
>> 10x10@20x20 if we're looking at a 10x10 rectangle positioned at (20,20)
>> or the other way around. media-ctl use (20,20)/10x10 which I think would
>> be a better notation.
> 
> Good point, I agree.
> 
> Ming Qian, can you also update patch 1/4 of the kernel patch series to
> use the same formatting when logging the V4L2_CTRL_TYPE_RECT value?
> 
> Regards,
> 
> 	Hans

There is a issue in v4l2-utils, that ',' is the ending flag in 
v4l_getsubopt(), then I can't set the rect control,
for example:

$v4l2-ctl -d 0 -c rect="(0,0)/1000x2000"
control '0)/1000x2000' without '='

Thanks,
Ming


> 
>>
>>> +			       ctrl.p_rect->left, ctrl.p_rect->top);
>>> +			break;
>>>   		default:
>>>   			printf("unsupported payload type");
>>>   			break;
>>> @@ -702,6 +706,9 @@ static void print_qctrl(int fd, const v4l2_query_ext_ctrl &qc,
>>>   	case V4L2_CTRL_TYPE_AREA:
>>>   		printf("%31s %#8.8x (area)   :", s.c_str(), qc.id);
>>>   		break;
>>> +	case V4L2_CTRL_TYPE_RECT:
>>> +		printf("%31s %#8.8x (rect)   :", s.c_str(), qc.id);
>>> +		break;
>>>   	case V4L2_CTRL_TYPE_HDR10_CLL_INFO:
>>>   		printf("%31s %#8.8x (hdr10-cll-info):", s.c_str(), qc.id);
>>>   		break;
>>> @@ -1279,6 +1286,11 @@ void common_set(cv4l_fd &_fd)
>>>   					sscanf(set_ctrl.second.c_str(), "%ux%u",
>>>   					       &ctrl.p_area->width, &ctrl.p_area->height);
>>>   					break;
>>> +				case V4L2_CTRL_TYPE_RECT:
>>> +					sscanf(set_ctrl.second.c_str(), "%ux%u@%dx%d",
>>> +					       &ctrl.p_rect->width, &ctrl.p_rect->height,
>>> +					       &ctrl.p_rect->left, &ctrl.p_rect->top);
>>> +					break;
>>>   				default:
>>>   					fprintf(stderr, "%s: unsupported payload type\n",
>>>   							qc.name);
>>
> 

Re: [PATCH v2 2/3] v4l2-ctl: Support V4L2_CTRL_TYPE_RECT

[Laurent Pinchart]

On Thu, Oct 31, 2024 at 05:19:02PM +0800, Ming Qian(OSS) wrote:
> On 2024/10/30 17:19, Hans Verkuil wrote:
> > On 30/10/2024 10:03, Laurent Pinchart wrote:
> >> On Wed, Oct 30, 2024 at 11:43:06AM +0900, ming.qian@oss.nxp.com wrote:
> >>> From: Yunke Cao <yunkec@google.com>
> >>>
> >>> Tested with VIVID
> >>>
> >>>   ./v4l2-ctl -C rect -d 0
> >>> rect: 300x400@200x100
> >>>
> >>>   ./v4l2-ctl -c rect=1000x2000@0x0
> >>>   ./v4l2-ctl -C rect -d 0
> >>> rect: 1000x2000@0x0
> >>>
> >>> Signed-off-by: Yunke Cao <yunkec@google.com>
> >>> Signed-off-by: Ming Qian <ming.qian@oss.nxp.com>
> >>> ---
> >>>   utils/v4l2-ctl/v4l2-ctl-common.cpp | 12 ++++++++++++
> >>>   1 file changed, 12 insertions(+)
> >>>
> >>> diff --git a/utils/v4l2-ctl/v4l2-ctl-common.cpp b/utils/v4l2-ctl/v4l2-ctl-common.cpp
> >>> index 40667575fcc7..538e1951cf81 100644
> >>> --- a/utils/v4l2-ctl/v4l2-ctl-common.cpp
> >>> +++ b/utils/v4l2-ctl/v4l2-ctl-common.cpp
> >>> @@ -614,6 +614,10 @@ static void print_value(int fd, const v4l2_query_ext_ctrl &qc, const v4l2_ext_co
> >>>   		case V4L2_CTRL_TYPE_AREA:
> >>>   			printf("%dx%d", ctrl.p_area->width, ctrl.p_area->height);
> >>>   			break;
> >>> +		case V4L2_CTRL_TYPE_RECT:
> >>> +			printf("%ux%u@%dx%d", ctrl.p_rect->width, ctrl.p_rect->height,
> >>
> >> I find this notation ambiguous, it's not immediately clear when reading
> >> 10x10@20x20 if we're looking at a 10x10 rectangle positioned at (20,20)
> >> or the other way around. media-ctl use (20,20)/10x10 which I think would
> >> be a better notation.
> > 
> > Good point, I agree.
> > 
> > Ming Qian, can you also update patch 1/4 of the kernel patch series to
> > use the same formatting when logging the V4L2_CTRL_TYPE_RECT value?
> > 
> > Regards,
> > 
> > 	Hans
> 
> There is a issue in v4l2-utils, that ',' is the ending flag in 
> v4l_getsubopt(), then I can't set the rect control,
> for example:
> 
> $v4l2-ctl -d 0 -c rect="(0,0)/1000x2000"
> control '0)/1000x2000' without '='

The should be fixable in v4l_getsubopt().

> >>> +			       ctrl.p_rect->left, ctrl.p_rect->top);
> >>> +			break;
> >>>   		default:
> >>>   			printf("unsupported payload type");
> >>>   			break;
> >>> @@ -702,6 +706,9 @@ static void print_qctrl(int fd, const v4l2_query_ext_ctrl &qc,
> >>>   	case V4L2_CTRL_TYPE_AREA:
> >>>   		printf("%31s %#8.8x (area)   :", s.c_str(), qc.id);
> >>>   		break;
> >>> +	case V4L2_CTRL_TYPE_RECT:
> >>> +		printf("%31s %#8.8x (rect)   :", s.c_str(), qc.id);
> >>> +		break;
> >>>   	case V4L2_CTRL_TYPE_HDR10_CLL_INFO:
> >>>   		printf("%31s %#8.8x (hdr10-cll-info):", s.c_str(), qc.id);
> >>>   		break;
> >>> @@ -1279,6 +1286,11 @@ void common_set(cv4l_fd &_fd)
> >>>   					sscanf(set_ctrl.second.c_str(), "%ux%u",
> >>>   					       &ctrl.p_area->width, &ctrl.p_area->height);
> >>>   					break;
> >>> +				case V4L2_CTRL_TYPE_RECT:
> >>> +					sscanf(set_ctrl.second.c_str(), "%ux%u@%dx%d",
> >>> +					       &ctrl.p_rect->width, &ctrl.p_rect->height,
> >>> +					       &ctrl.p_rect->left, &ctrl.p_rect->top);
> >>> +					break;
> >>>   				default:
> >>>   					fprintf(stderr, "%s: unsupported payload type\n",
> >>>   							qc.name);

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 2/3] v4l2-ctl: Support V4L2_CTRL_TYPE_RECT

[Ming Qian(OSS)]

Hi Laurent,

On 2024/10/31 17:34, Laurent Pinchart wrote:
> On Thu, Oct 31, 2024 at 05:19:02PM +0800, Ming Qian(OSS) wrote:
>> On 2024/10/30 17:19, Hans Verkuil wrote:
>>> On 30/10/2024 10:03, Laurent Pinchart wrote:
>>>> On Wed, Oct 30, 2024 at 11:43:06AM +0900, ming.qian@oss.nxp.com wrote:
>>>>> From: Yunke Cao <yunkec@google.com>
>>>>>
>>>>> Tested with VIVID
>>>>>
>>>>>    ./v4l2-ctl -C rect -d 0
>>>>> rect: 300x400@200x100
>>>>>
>>>>>    ./v4l2-ctl -c rect=1000x2000@0x0
>>>>>    ./v4l2-ctl -C rect -d 0
>>>>> rect: 1000x2000@0x0
>>>>>
>>>>> Signed-off-by: Yunke Cao <yunkec@google.com>
>>>>> Signed-off-by: Ming Qian <ming.qian@oss.nxp.com>
>>>>> ---
>>>>>    utils/v4l2-ctl/v4l2-ctl-common.cpp | 12 ++++++++++++
>>>>>    1 file changed, 12 insertions(+)
>>>>>
>>>>> diff --git a/utils/v4l2-ctl/v4l2-ctl-common.cpp b/utils/v4l2-ctl/v4l2-ctl-common.cpp
>>>>> index 40667575fcc7..538e1951cf81 100644
>>>>> --- a/utils/v4l2-ctl/v4l2-ctl-common.cpp
>>>>> +++ b/utils/v4l2-ctl/v4l2-ctl-common.cpp
>>>>> @@ -614,6 +614,10 @@ static void print_value(int fd, const v4l2_query_ext_ctrl &qc, const v4l2_ext_co
>>>>>    		case V4L2_CTRL_TYPE_AREA:
>>>>>    			printf("%dx%d", ctrl.p_area->width, ctrl.p_area->height);
>>>>>    			break;
>>>>> +		case V4L2_CTRL_TYPE_RECT:
>>>>> +			printf("%ux%u@%dx%d", ctrl.p_rect->width, ctrl.p_rect->height,
>>>>
>>>> I find this notation ambiguous, it's not immediately clear when reading
>>>> 10x10@20x20 if we're looking at a 10x10 rectangle positioned at (20,20)
>>>> or the other way around. media-ctl use (20,20)/10x10 which I think would
>>>> be a better notation.
>>>
>>> Good point, I agree.
>>>
>>> Ming Qian, can you also update patch 1/4 of the kernel patch series to
>>> use the same formatting when logging the V4L2_CTRL_TYPE_RECT value?
>>>
>>> Regards,
>>>
>>> 	Hans
>>
>> There is a issue in v4l2-utils, that ',' is the ending flag in
>> v4l_getsubopt(), then I can't set the rect control,
>> for example:
>>
>> $v4l2-ctl -d 0 -c rect="(0,0)/1000x2000"
>> control '0)/1000x2000' without '='
> 
> The should be fixable in v4l_getsubopt().
> 

I can see the following comments of v4l_getsubopt(),

    Parse comma separated suboption from *OPTIONP and match against
    strings in TOKENS.

I am not sure if we can change it.

Thanks,
Ming

>>>>> +			       ctrl.p_rect->left, ctrl.p_rect->top);
>>>>> +			break;
>>>>>    		default:
>>>>>    			printf("unsupported payload type");
>>>>>    			break;
>>>>> @@ -702,6 +706,9 @@ static void print_qctrl(int fd, const v4l2_query_ext_ctrl &qc,
>>>>>    	case V4L2_CTRL_TYPE_AREA:
>>>>>    		printf("%31s %#8.8x (area)   :", s.c_str(), qc.id);
>>>>>    		break;
>>>>> +	case V4L2_CTRL_TYPE_RECT:
>>>>> +		printf("%31s %#8.8x (rect)   :", s.c_str(), qc.id);
>>>>> +		break;
>>>>>    	case V4L2_CTRL_TYPE_HDR10_CLL_INFO:
>>>>>    		printf("%31s %#8.8x (hdr10-cll-info):", s.c_str(), qc.id);
>>>>>    		break;
>>>>> @@ -1279,6 +1286,11 @@ void common_set(cv4l_fd &_fd)
>>>>>    					sscanf(set_ctrl.second.c_str(), "%ux%u",
>>>>>    					       &ctrl.p_area->width, &ctrl.p_area->height);
>>>>>    					break;
>>>>> +				case V4L2_CTRL_TYPE_RECT:
>>>>> +					sscanf(set_ctrl.second.c_str(), "%ux%u@%dx%d",
>>>>> +					       &ctrl.p_rect->width, &ctrl.p_rect->height,
>>>>> +					       &ctrl.p_rect->left, &ctrl.p_rect->top);
>>>>> +					break;
>>>>>    				default:
>>>>>    					fprintf(stderr, "%s: unsupported payload type\n",
>>>>>    							qc.name);
> 

Re: [PATCH v2 2/3] v4l2-ctl: Support V4L2_CTRL_TYPE_RECT

[Laurent Pinchart]

On Thu, Oct 31, 2024 at 05:46:49PM +0800, Ming Qian(OSS) wrote:
> On 2024/10/31 17:34, Laurent Pinchart wrote:
> > On Thu, Oct 31, 2024 at 05:19:02PM +0800, Ming Qian(OSS) wrote:
> >> On 2024/10/30 17:19, Hans Verkuil wrote:
> >>> On 30/10/2024 10:03, Laurent Pinchart wrote:
> >>>> On Wed, Oct 30, 2024 at 11:43:06AM +0900, ming.qian@oss.nxp.com wrote:
> >>>>> From: Yunke Cao <yunkec@google.com>
> >>>>>
> >>>>> Tested with VIVID
> >>>>>
> >>>>>    ./v4l2-ctl -C rect -d 0
> >>>>> rect: 300x400@200x100
> >>>>>
> >>>>>    ./v4l2-ctl -c rect=1000x2000@0x0
> >>>>>    ./v4l2-ctl -C rect -d 0
> >>>>> rect: 1000x2000@0x0
> >>>>>
> >>>>> Signed-off-by: Yunke Cao <yunkec@google.com>
> >>>>> Signed-off-by: Ming Qian <ming.qian@oss.nxp.com>
> >>>>> ---
> >>>>>    utils/v4l2-ctl/v4l2-ctl-common.cpp | 12 ++++++++++++
> >>>>>    1 file changed, 12 insertions(+)
> >>>>>
> >>>>> diff --git a/utils/v4l2-ctl/v4l2-ctl-common.cpp b/utils/v4l2-ctl/v4l2-ctl-common.cpp
> >>>>> index 40667575fcc7..538e1951cf81 100644
> >>>>> --- a/utils/v4l2-ctl/v4l2-ctl-common.cpp
> >>>>> +++ b/utils/v4l2-ctl/v4l2-ctl-common.cpp
> >>>>> @@ -614,6 +614,10 @@ static void print_value(int fd, const v4l2_query_ext_ctrl &qc, const v4l2_ext_co
> >>>>>    		case V4L2_CTRL_TYPE_AREA:
> >>>>>    			printf("%dx%d", ctrl.p_area->width, ctrl.p_area->height);
> >>>>>    			break;
> >>>>> +		case V4L2_CTRL_TYPE_RECT:
> >>>>> +			printf("%ux%u@%dx%d", ctrl.p_rect->width, ctrl.p_rect->height,
> >>>>
> >>>> I find this notation ambiguous, it's not immediately clear when reading
> >>>> 10x10@20x20 if we're looking at a 10x10 rectangle positioned at (20,20)
> >>>> or the other way around. media-ctl use (20,20)/10x10 which I think would
> >>>> be a better notation.
> >>>
> >>> Good point, I agree.
> >>>
> >>> Ming Qian, can you also update patch 1/4 of the kernel patch series to
> >>> use the same formatting when logging the V4L2_CTRL_TYPE_RECT value?
> >>>
> >>> Regards,
> >>>
> >>> 	Hans
> >>
> >> There is a issue in v4l2-utils, that ',' is the ending flag in
> >> v4l_getsubopt(), then I can't set the rect control,
> >> for example:
> >>
> >> $v4l2-ctl -d 0 -c rect="(0,0)/1000x2000"
> >> control '0)/1000x2000' without '='
> > 
> > The should be fixable in v4l_getsubopt().
> > 
> 
> I can see the following comments of v4l_getsubopt(),
> 
>     Parse comma separated suboption from *OPTIONP and match against
>     strings in TOKENS.
> 
> I am not sure if we can change it.

I think we can improve quotes handling by considering quoted substrings
as a single value, ignoring commas. Hans any opinion ?

> >>>>> +			       ctrl.p_rect->left, ctrl.p_rect->top);
> >>>>> +			break;
> >>>>>    		default:
> >>>>>    			printf("unsupported payload type");
> >>>>>    			break;
> >>>>> @@ -702,6 +706,9 @@ static void print_qctrl(int fd, const v4l2_query_ext_ctrl &qc,
> >>>>>    	case V4L2_CTRL_TYPE_AREA:
> >>>>>    		printf("%31s %#8.8x (area)   :", s.c_str(), qc.id);
> >>>>>    		break;
> >>>>> +	case V4L2_CTRL_TYPE_RECT:
> >>>>> +		printf("%31s %#8.8x (rect)   :", s.c_str(), qc.id);
> >>>>> +		break;
> >>>>>    	case V4L2_CTRL_TYPE_HDR10_CLL_INFO:
> >>>>>    		printf("%31s %#8.8x (hdr10-cll-info):", s.c_str(), qc.id);
> >>>>>    		break;
> >>>>> @@ -1279,6 +1286,11 @@ void common_set(cv4l_fd &_fd)
> >>>>>    					sscanf(set_ctrl.second.c_str(), "%ux%u",
> >>>>>    					       &ctrl.p_area->width, &ctrl.p_area->height);
> >>>>>    					break;
> >>>>> +				case V4L2_CTRL_TYPE_RECT:
> >>>>> +					sscanf(set_ctrl.second.c_str(), "%ux%u@%dx%d",
> >>>>> +					       &ctrl.p_rect->width, &ctrl.p_rect->height,
> >>>>> +					       &ctrl.p_rect->left, &ctrl.p_rect->top);
> >>>>> +					break;
> >>>>>    				default:
> >>>>>    					fprintf(stderr, "%s: unsupported payload type\n",
> >>>>>    							qc.name);

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 2/3] v4l2-ctl: Support V4L2_CTRL_TYPE_RECT

[Ming Qian(OSS)]

Hi Laurent and Hans,

On 2024/10/31 18:09, Laurent Pinchart wrote:
> On Thu, Oct 31, 2024 at 05:46:49PM +0800, Ming Qian(OSS) wrote:
>> On 2024/10/31 17:34, Laurent Pinchart wrote:
>>> On Thu, Oct 31, 2024 at 05:19:02PM +0800, Ming Qian(OSS) wrote:
>>>> On 2024/10/30 17:19, Hans Verkuil wrote:
>>>>> On 30/10/2024 10:03, Laurent Pinchart wrote:
>>>>>> On Wed, Oct 30, 2024 at 11:43:06AM +0900, ming.qian@oss.nxp.com wrote:
>>>>>>> From: Yunke Cao <yunkec@google.com>
>>>>>>>
>>>>>>> Tested with VIVID
>>>>>>>
>>>>>>>     ./v4l2-ctl -C rect -d 0
>>>>>>> rect: 300x400@200x100
>>>>>>>
>>>>>>>     ./v4l2-ctl -c rect=1000x2000@0x0
>>>>>>>     ./v4l2-ctl -C rect -d 0
>>>>>>> rect: 1000x2000@0x0
>>>>>>>
>>>>>>> Signed-off-by: Yunke Cao <yunkec@google.com>
>>>>>>> Signed-off-by: Ming Qian <ming.qian@oss.nxp.com>
>>>>>>> ---
>>>>>>>     utils/v4l2-ctl/v4l2-ctl-common.cpp | 12 ++++++++++++
>>>>>>>     1 file changed, 12 insertions(+)
>>>>>>>
>>>>>>> diff --git a/utils/v4l2-ctl/v4l2-ctl-common.cpp b/utils/v4l2-ctl/v4l2-ctl-common.cpp
>>>>>>> index 40667575fcc7..538e1951cf81 100644
>>>>>>> --- a/utils/v4l2-ctl/v4l2-ctl-common.cpp
>>>>>>> +++ b/utils/v4l2-ctl/v4l2-ctl-common.cpp
>>>>>>> @@ -614,6 +614,10 @@ static void print_value(int fd, const v4l2_query_ext_ctrl &qc, const v4l2_ext_co
>>>>>>>     		case V4L2_CTRL_TYPE_AREA:
>>>>>>>     			printf("%dx%d", ctrl.p_area->width, ctrl.p_area->height);
>>>>>>>     			break;
>>>>>>> +		case V4L2_CTRL_TYPE_RECT:
>>>>>>> +			printf("%ux%u@%dx%d", ctrl.p_rect->width, ctrl.p_rect->height,
>>>>>>
>>>>>> I find this notation ambiguous, it's not immediately clear when reading
>>>>>> 10x10@20x20 if we're looking at a 10x10 rectangle positioned at (20,20)
>>>>>> or the other way around. media-ctl use (20,20)/10x10 which I think would
>>>>>> be a better notation.
>>>>>
>>>>> Good point, I agree.
>>>>>
>>>>> Ming Qian, can you also update patch 1/4 of the kernel patch series to
>>>>> use the same formatting when logging the V4L2_CTRL_TYPE_RECT value?
>>>>>
>>>>> Regards,
>>>>>
>>>>> 	Hans
>>>>
>>>> There is a issue in v4l2-utils, that ',' is the ending flag in
>>>> v4l_getsubopt(), then I can't set the rect control,
>>>> for example:
>>>>
>>>> $v4l2-ctl -d 0 -c rect="(0,0)/1000x2000"
>>>> control '0)/1000x2000' without '='
>>>
>>> The should be fixable in v4l_getsubopt().
>>>
>>
>> I can see the following comments of v4l_getsubopt(),
>>
>>      Parse comma separated suboption from *OPTIONP and match against
>>      strings in TOKENS.
>>
>> I am not sure if we can change it.
> 
> I think we can improve quotes handling by considering quoted substrings
> as a single value, ignoring commas. Hans any opinion ?
> 

How about omitting the commas between the brackets when parsing subopt?


>>>>>>> +			       ctrl.p_rect->left, ctrl.p_rect->top);
>>>>>>> +			break;
>>>>>>>     		default:
>>>>>>>     			printf("unsupported payload type");
>>>>>>>     			break;
>>>>>>> @@ -702,6 +706,9 @@ static void print_qctrl(int fd, const v4l2_query_ext_ctrl &qc,
>>>>>>>     	case V4L2_CTRL_TYPE_AREA:
>>>>>>>     		printf("%31s %#8.8x (area)   :", s.c_str(), qc.id);
>>>>>>>     		break;
>>>>>>> +	case V4L2_CTRL_TYPE_RECT:
>>>>>>> +		printf("%31s %#8.8x (rect)   :", s.c_str(), qc.id);
>>>>>>> +		break;
>>>>>>>     	case V4L2_CTRL_TYPE_HDR10_CLL_INFO:
>>>>>>>     		printf("%31s %#8.8x (hdr10-cll-info):", s.c_str(), qc.id);
>>>>>>>     		break;
>>>>>>> @@ -1279,6 +1286,11 @@ void common_set(cv4l_fd &_fd)
>>>>>>>     					sscanf(set_ctrl.second.c_str(), "%ux%u",
>>>>>>>     					       &ctrl.p_area->width, &ctrl.p_area->height);
>>>>>>>     					break;
>>>>>>> +				case V4L2_CTRL_TYPE_RECT:
>>>>>>> +					sscanf(set_ctrl.second.c_str(), "%ux%u@%dx%d",
>>>>>>> +					       &ctrl.p_rect->width, &ctrl.p_rect->height,
>>>>>>> +					       &ctrl.p_rect->left, &ctrl.p_rect->top);
>>>>>>> +					break;
>>>>>>>     				default:
>>>>>>>     					fprintf(stderr, "%s: unsupported payload type\n",
>>>>>>>     							qc.name);
> 

Re: [PATCH v2 2/3] v4l2-ctl: Support V4L2_CTRL_TYPE_RECT

[Hans Verkuil]

On 04/11/2024 02:24, Ming Qian(OSS) wrote:
> Hi Laurent and Hans,
> 
> On 2024/10/31 18:09, Laurent Pinchart wrote:
>> On Thu, Oct 31, 2024 at 05:46:49PM +0800, Ming Qian(OSS) wrote:
>>> On 2024/10/31 17:34, Laurent Pinchart wrote:
>>>> On Thu, Oct 31, 2024 at 05:19:02PM +0800, Ming Qian(OSS) wrote:
>>>>> On 2024/10/30 17:19, Hans Verkuil wrote:
>>>>>> On 30/10/2024 10:03, Laurent Pinchart wrote:
>>>>>>> On Wed, Oct 30, 2024 at 11:43:06AM +0900, ming.qian@oss.nxp.com wrote:
>>>>>>>> From: Yunke Cao <yunkec@google.com>
>>>>>>>>
>>>>>>>> Tested with VIVID
>>>>>>>>
>>>>>>>>     ./v4l2-ctl -C rect -d 0
>>>>>>>> rect: 300x400@200x100
>>>>>>>>
>>>>>>>>     ./v4l2-ctl -c rect=1000x2000@0x0
>>>>>>>>     ./v4l2-ctl -C rect -d 0
>>>>>>>> rect: 1000x2000@0x0
>>>>>>>>
>>>>>>>> Signed-off-by: Yunke Cao <yunkec@google.com>
>>>>>>>> Signed-off-by: Ming Qian <ming.qian@oss.nxp.com>
>>>>>>>> ---
>>>>>>>>     utils/v4l2-ctl/v4l2-ctl-common.cpp | 12 ++++++++++++
>>>>>>>>     1 file changed, 12 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/utils/v4l2-ctl/v4l2-ctl-common.cpp b/utils/v4l2-ctl/v4l2-ctl-common.cpp
>>>>>>>> index 40667575fcc7..538e1951cf81 100644
>>>>>>>> --- a/utils/v4l2-ctl/v4l2-ctl-common.cpp
>>>>>>>> +++ b/utils/v4l2-ctl/v4l2-ctl-common.cpp
>>>>>>>> @@ -614,6 +614,10 @@ static void print_value(int fd, const v4l2_query_ext_ctrl &qc, const v4l2_ext_co
>>>>>>>>     		case V4L2_CTRL_TYPE_AREA:
>>>>>>>>     			printf("%dx%d", ctrl.p_area->width, ctrl.p_area->height);
>>>>>>>>     			break;
>>>>>>>> +		case V4L2_CTRL_TYPE_RECT:
>>>>>>>> +			printf("%ux%u@%dx%d", ctrl.p_rect->width, ctrl.p_rect->height,
>>>>>>>
>>>>>>> I find this notation ambiguous, it's not immediately clear when reading
>>>>>>> 10x10@20x20 if we're looking at a 10x10 rectangle positioned at (20,20)
>>>>>>> or the other way around. media-ctl use (20,20)/10x10 which I think would
>>>>>>> be a better notation.
>>>>>>
>>>>>> Good point, I agree.
>>>>>>
>>>>>> Ming Qian, can you also update patch 1/4 of the kernel patch series to
>>>>>> use the same formatting when logging the V4L2_CTRL_TYPE_RECT value?
>>>>>>
>>>>>> Regards,
>>>>>>
>>>>>> 	Hans
>>>>>
>>>>> There is a issue in v4l2-utils, that ',' is the ending flag in
>>>>> v4l_getsubopt(), then I can't set the rect control,
>>>>> for example:
>>>>>
>>>>> $v4l2-ctl -d 0 -c rect="(0,0)/1000x2000"
>>>>> control '0)/1000x2000' without '='
>>>>
>>>> The should be fixable in v4l_getsubopt().
>>>>
>>>
>>> I can see the following comments of v4l_getsubopt(),
>>>
>>>      Parse comma separated suboption from *OPTIONP and match against
>>>      strings in TOKENS.
>>>
>>> I am not sure if we can change it.
>>
>> I think we can improve quotes handling by considering quoted substrings
>> as a single value, ignoring commas. Hans any opinion ?

I think commas are hard to parse. Note that v4l_getsubopt is normally a
#define for getsubopt from glibc. So you can't change the behavior of
that function.

I propose this format for parsing instead:

widthxheight@(top;left)

e.g.: 1000x2000@(0;0)

According to this:
https://www.dr-aart.nl/Geometry-coordinates.html

the ';' is the separator in countries where a decimal comma is used
instead of a decimal point.

I prefer to have the position after the size of the rectangle, for two
reasons: it feels more natural to talk about a 'rectangle of size S at position
P', and it also makes it possible to allow a variant where only the size
is given and the position will default to (0;0). I.e., we can support
parsing either "widthxheight" or "widthxheight@(top;left)".

However, logging rectangles in the kernel should use a comma instead of a
semicolon. Inside v4l-utils just consistently use the semicolon.

What do you think, Laurent?

Regards,

	Hans

>>
> 
> How about omitting the commas between the brackets when parsing subopt?
> 
> 
>>>>>>>> +			       ctrl.p_rect->left, ctrl.p_rect->top);
>>>>>>>> +			break;
>>>>>>>>     		default:
>>>>>>>>     			printf("unsupported payload type");
>>>>>>>>     			break;
>>>>>>>> @@ -702,6 +706,9 @@ static void print_qctrl(int fd, const v4l2_query_ext_ctrl &qc,
>>>>>>>>     	case V4L2_CTRL_TYPE_AREA:
>>>>>>>>     		printf("%31s %#8.8x (area)   :", s.c_str(), qc.id);
>>>>>>>>     		break;
>>>>>>>> +	case V4L2_CTRL_TYPE_RECT:
>>>>>>>> +		printf("%31s %#8.8x (rect)   :", s.c_str(), qc.id);
>>>>>>>> +		break;
>>>>>>>>     	case V4L2_CTRL_TYPE_HDR10_CLL_INFO:
>>>>>>>>     		printf("%31s %#8.8x (hdr10-cll-info):", s.c_str(), qc.id);
>>>>>>>>     		break;
>>>>>>>> @@ -1279,6 +1286,11 @@ void common_set(cv4l_fd &_fd)
>>>>>>>>     					sscanf(set_ctrl.second.c_str(), "%ux%u",
>>>>>>>>     					       &ctrl.p_area->width, &ctrl.p_area->height);
>>>>>>>>     					break;
>>>>>>>> +				case V4L2_CTRL_TYPE_RECT:
>>>>>>>> +					sscanf(set_ctrl.second.c_str(), "%ux%u@%dx%d",
>>>>>>>> +					       &ctrl.p_rect->width, &ctrl.p_rect->height,
>>>>>>>> +					       &ctrl.p_rect->left, &ctrl.p_rect->top);
>>>>>>>> +					break;
>>>>>>>>     				default:
>>>>>>>>     					fprintf(stderr, "%s: unsupported payload type\n",
>>>>>>>>     							qc.name);
>>

Re: [PATCH v2 2/3] v4l2-ctl: Support V4L2_CTRL_TYPE_RECT

[Laurent Pinchart]

Hi Hans,

On Tue, Nov 05, 2024 at 09:30:43AM +0100, Hans Verkuil wrote:
> On 04/11/2024 02:24, Ming Qian(OSS) wrote:
> > On 2024/10/31 18:09, Laurent Pinchart wrote:
> >> On Thu, Oct 31, 2024 at 05:46:49PM +0800, Ming Qian(OSS) wrote:
> >>> On 2024/10/31 17:34, Laurent Pinchart wrote:
> >>>> On Thu, Oct 31, 2024 at 05:19:02PM +0800, Ming Qian(OSS) wrote:
> >>>>> On 2024/10/30 17:19, Hans Verkuil wrote:
> >>>>>> On 30/10/2024 10:03, Laurent Pinchart wrote:
> >>>>>>> On Wed, Oct 30, 2024 at 11:43:06AM +0900, ming.qian@oss.nxp.com wrote:
> >>>>>>>> From: Yunke Cao <yunkec@google.com>
> >>>>>>>>
> >>>>>>>> Tested with VIVID
> >>>>>>>>
> >>>>>>>>     ./v4l2-ctl -C rect -d 0
> >>>>>>>> rect: 300x400@200x100
> >>>>>>>>
> >>>>>>>>     ./v4l2-ctl -c rect=1000x2000@0x0
> >>>>>>>>     ./v4l2-ctl -C rect -d 0
> >>>>>>>> rect: 1000x2000@0x0
> >>>>>>>>
> >>>>>>>> Signed-off-by: Yunke Cao <yunkec@google.com>
> >>>>>>>> Signed-off-by: Ming Qian <ming.qian@oss.nxp.com>
> >>>>>>>> ---
> >>>>>>>>     utils/v4l2-ctl/v4l2-ctl-common.cpp | 12 ++++++++++++
> >>>>>>>>     1 file changed, 12 insertions(+)
> >>>>>>>>
> >>>>>>>> diff --git a/utils/v4l2-ctl/v4l2-ctl-common.cpp b/utils/v4l2-ctl/v4l2-ctl-common.cpp
> >>>>>>>> index 40667575fcc7..538e1951cf81 100644
> >>>>>>>> --- a/utils/v4l2-ctl/v4l2-ctl-common.cpp
> >>>>>>>> +++ b/utils/v4l2-ctl/v4l2-ctl-common.cpp
> >>>>>>>> @@ -614,6 +614,10 @@ static void print_value(int fd, const v4l2_query_ext_ctrl &qc, const v4l2_ext_co
> >>>>>>>>     		case V4L2_CTRL_TYPE_AREA:
> >>>>>>>>     			printf("%dx%d", ctrl.p_area->width, ctrl.p_area->height);
> >>>>>>>>     			break;
> >>>>>>>> +		case V4L2_CTRL_TYPE_RECT:
> >>>>>>>> +			printf("%ux%u@%dx%d", ctrl.p_rect->width, ctrl.p_rect->height,
> >>>>>>>
> >>>>>>> I find this notation ambiguous, it's not immediately clear when reading
> >>>>>>> 10x10@20x20 if we're looking at a 10x10 rectangle positioned at (20,20)
> >>>>>>> or the other way around. media-ctl use (20,20)/10x10 which I think would
> >>>>>>> be a better notation.
> >>>>>>
> >>>>>> Good point, I agree.
> >>>>>>
> >>>>>> Ming Qian, can you also update patch 1/4 of the kernel patch series to
> >>>>>> use the same formatting when logging the V4L2_CTRL_TYPE_RECT value?
> >>>>>>
> >>>>>> Regards,
> >>>>>>
> >>>>>> 	Hans
> >>>>>
> >>>>> There is a issue in v4l2-utils, that ',' is the ending flag in
> >>>>> v4l_getsubopt(), then I can't set the rect control,
> >>>>> for example:
> >>>>>
> >>>>> $v4l2-ctl -d 0 -c rect="(0,0)/1000x2000"
> >>>>> control '0)/1000x2000' without '='
> >>>>
> >>>> The should be fixable in v4l_getsubopt().
> >>>>
> >>>
> >>> I can see the following comments of v4l_getsubopt(),
> >>>
> >>>      Parse comma separated suboption from *OPTIONP and match against
> >>>      strings in TOKENS.
> >>>
> >>> I am not sure if we can change it.
> >>
> >> I think we can improve quotes handling by considering quoted substrings
> >> as a single value, ignoring commas. Hans any opinion ?
> 
> I think commas are hard to parse. Note that v4l_getsubopt is normally a
> #define for getsubopt from glibc. So you can't change the behavior of
> that function.

Can't we ? Isn't it an internal function ?

> I propose this format for parsing instead:
> 
> widthxheight@(top;left)
> 
> e.g.: 1000x2000@(0;0)
> 
> According to this:
> https://www.dr-aart.nl/Geometry-coordinates.html
> 
> the ';' is the separator in countries where a decimal comma is used
> instead of a decimal point.
> 
> I prefer to have the position after the size of the rectangle, for two
> reasons: it feels more natural to talk about a 'rectangle of size S at position
> P', and it also makes it possible to allow a variant where only the size
> is given and the position will default to (0;0). I.e., we can support
> parsing either "widthxheight" or "widthxheight@(top;left)".
> 
> However, logging rectangles in the kernel should use a comma instead of a
> semicolon. Inside v4l-utils just consistently use the semicolon.
> 
> What do you think, Laurent?

We have a precedent of using (x,y)/WxH , both in the kernel and in
media-ctl. Breaking that with another syntax would cause trouble,
especially having different syntaxes between media-ctl and v4l2-ctl.
Think about the shell scripts that would need to convert from one syntax
to another for instance. I would very strongly like to avoid that.

> > How about omitting the commas between the brackets when parsing subopt?
> > 
> > 
> >>>>>>>> +			       ctrl.p_rect->left, ctrl.p_rect->top);
> >>>>>>>> +			break;
> >>>>>>>>     		default:
> >>>>>>>>     			printf("unsupported payload type");
> >>>>>>>>     			break;
> >>>>>>>> @@ -702,6 +706,9 @@ static void print_qctrl(int fd, const v4l2_query_ext_ctrl &qc,
> >>>>>>>>     	case V4L2_CTRL_TYPE_AREA:
> >>>>>>>>     		printf("%31s %#8.8x (area)   :", s.c_str(), qc.id);
> >>>>>>>>     		break;
> >>>>>>>> +	case V4L2_CTRL_TYPE_RECT:
> >>>>>>>> +		printf("%31s %#8.8x (rect)   :", s.c_str(), qc.id);
> >>>>>>>> +		break;
> >>>>>>>>     	case V4L2_CTRL_TYPE_HDR10_CLL_INFO:
> >>>>>>>>     		printf("%31s %#8.8x (hdr10-cll-info):", s.c_str(), qc.id);
> >>>>>>>>     		break;
> >>>>>>>> @@ -1279,6 +1286,11 @@ void common_set(cv4l_fd &_fd)
> >>>>>>>>     					sscanf(set_ctrl.second.c_str(), "%ux%u",
> >>>>>>>>     					       &ctrl.p_area->width, &ctrl.p_area->height);
> >>>>>>>>     					break;
> >>>>>>>> +				case V4L2_CTRL_TYPE_RECT:
> >>>>>>>> +					sscanf(set_ctrl.second.c_str(), "%ux%u@%dx%d",
> >>>>>>>> +					       &ctrl.p_rect->width, &ctrl.p_rect->height,
> >>>>>>>> +					       &ctrl.p_rect->left, &ctrl.p_rect->top);
> >>>>>>>> +					break;
> >>>>>>>>     				default:
> >>>>>>>>     					fprintf(stderr, "%s: unsupported payload type\n",
> >>>>>>>>     							qc.name);

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 2/3] v4l2-ctl: Support V4L2_CTRL_TYPE_RECT

[Hans Verkuil]

On 05/11/2024 09:51, Laurent Pinchart wrote:
> Hi Hans,
> 
> On Tue, Nov 05, 2024 at 09:30:43AM +0100, Hans Verkuil wrote:
>> On 04/11/2024 02:24, Ming Qian(OSS) wrote:
>>> On 2024/10/31 18:09, Laurent Pinchart wrote:
>>>> On Thu, Oct 31, 2024 at 05:46:49PM +0800, Ming Qian(OSS) wrote:
>>>>> On 2024/10/31 17:34, Laurent Pinchart wrote:
>>>>>> On Thu, Oct 31, 2024 at 05:19:02PM +0800, Ming Qian(OSS) wrote:
>>>>>>> On 2024/10/30 17:19, Hans Verkuil wrote:
>>>>>>>> On 30/10/2024 10:03, Laurent Pinchart wrote:
>>>>>>>>> On Wed, Oct 30, 2024 at 11:43:06AM +0900, ming.qian@oss.nxp.com wrote:
>>>>>>>>>> From: Yunke Cao <yunkec@google.com>
>>>>>>>>>>
>>>>>>>>>> Tested with VIVID
>>>>>>>>>>
>>>>>>>>>>     ./v4l2-ctl -C rect -d 0
>>>>>>>>>> rect: 300x400@200x100
>>>>>>>>>>
>>>>>>>>>>     ./v4l2-ctl -c rect=1000x2000@0x0
>>>>>>>>>>     ./v4l2-ctl -C rect -d 0
>>>>>>>>>> rect: 1000x2000@0x0
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Yunke Cao <yunkec@google.com>
>>>>>>>>>> Signed-off-by: Ming Qian <ming.qian@oss.nxp.com>
>>>>>>>>>> ---
>>>>>>>>>>     utils/v4l2-ctl/v4l2-ctl-common.cpp | 12 ++++++++++++
>>>>>>>>>>     1 file changed, 12 insertions(+)
>>>>>>>>>>
>>>>>>>>>> diff --git a/utils/v4l2-ctl/v4l2-ctl-common.cpp b/utils/v4l2-ctl/v4l2-ctl-common.cpp
>>>>>>>>>> index 40667575fcc7..538e1951cf81 100644
>>>>>>>>>> --- a/utils/v4l2-ctl/v4l2-ctl-common.cpp
>>>>>>>>>> +++ b/utils/v4l2-ctl/v4l2-ctl-common.cpp
>>>>>>>>>> @@ -614,6 +614,10 @@ static void print_value(int fd, const v4l2_query_ext_ctrl &qc, const v4l2_ext_co
>>>>>>>>>>     		case V4L2_CTRL_TYPE_AREA:
>>>>>>>>>>     			printf("%dx%d", ctrl.p_area->width, ctrl.p_area->height);
>>>>>>>>>>     			break;
>>>>>>>>>> +		case V4L2_CTRL_TYPE_RECT:
>>>>>>>>>> +			printf("%ux%u@%dx%d", ctrl.p_rect->width, ctrl.p_rect->height,
>>>>>>>>>
>>>>>>>>> I find this notation ambiguous, it's not immediately clear when reading
>>>>>>>>> 10x10@20x20 if we're looking at a 10x10 rectangle positioned at (20,20)
>>>>>>>>> or the other way around. media-ctl use (20,20)/10x10 which I think would
>>>>>>>>> be a better notation.
>>>>>>>>
>>>>>>>> Good point, I agree.
>>>>>>>>
>>>>>>>> Ming Qian, can you also update patch 1/4 of the kernel patch series to
>>>>>>>> use the same formatting when logging the V4L2_CTRL_TYPE_RECT value?
>>>>>>>>
>>>>>>>> Regards,
>>>>>>>>
>>>>>>>> 	Hans
>>>>>>>
>>>>>>> There is a issue in v4l2-utils, that ',' is the ending flag in
>>>>>>> v4l_getsubopt(), then I can't set the rect control,
>>>>>>> for example:
>>>>>>>
>>>>>>> $v4l2-ctl -d 0 -c rect="(0,0)/1000x2000"
>>>>>>> control '0)/1000x2000' without '='
>>>>>>
>>>>>> The should be fixable in v4l_getsubopt().
>>>>>>
>>>>>
>>>>> I can see the following comments of v4l_getsubopt(),
>>>>>
>>>>>      Parse comma separated suboption from *OPTIONP and match against
>>>>>      strings in TOKENS.
>>>>>
>>>>> I am not sure if we can change it.
>>>>
>>>> I think we can improve quotes handling by considering quoted substrings
>>>> as a single value, ignoring commas. Hans any opinion ?
>>
>> I think commas are hard to parse. Note that v4l_getsubopt is normally a
>> #define for getsubopt from glibc. So you can't change the behavior of
>> that function.
> 
> Can't we ? Isn't it an internal function ?

No. It's there when it is compiled for systems without glibc.

But I guess we can just use our copy all the time.

> 
>> I propose this format for parsing instead:
>>
>> widthxheight@(top;left)
>>
>> e.g.: 1000x2000@(0;0)
>>
>> According to this:
>> https://www.dr-aart.nl/Geometry-coordinates.html
>>
>> the ';' is the separator in countries where a decimal comma is used
>> instead of a decimal point.
>>
>> I prefer to have the position after the size of the rectangle, for two
>> reasons: it feels more natural to talk about a 'rectangle of size S at position
>> P', and it also makes it possible to allow a variant where only the size
>> is given and the position will default to (0;0). I.e., we can support
>> parsing either "widthxheight" or "widthxheight@(top;left)".
>>
>> However, logging rectangles in the kernel should use a comma instead of a
>> semicolon. Inside v4l-utils just consistently use the semicolon.
>>
>> What do you think, Laurent?
> 
> We have a precedent of using (x,y)/WxH , both in the kernel and in
> media-ctl. Breaking that with another syntax would cause trouble,
> especially having different syntaxes between media-ctl and v4l2-ctl.
> Think about the shell scripts that would need to convert from one syntax
> to another for instance. I would very strongly like to avoid that.

Have we used that notation in the kernel? Where?

I will admit that I am not a fan of the media-ctl notation, I think it is
weird. I think WxH@(x,y) is much more natural. But if v4l_getsubopt can be
adapted for this, then I'm fine with it.

Regards,

	Hans

> 
>>> How about omitting the commas between the brackets when parsing subopt?
>>>
>>>
>>>>>>>>>> +			       ctrl.p_rect->left, ctrl.p_rect->top);
>>>>>>>>>> +			break;
>>>>>>>>>>     		default:
>>>>>>>>>>     			printf("unsupported payload type");
>>>>>>>>>>     			break;
>>>>>>>>>> @@ -702,6 +706,9 @@ static void print_qctrl(int fd, const v4l2_query_ext_ctrl &qc,
>>>>>>>>>>     	case V4L2_CTRL_TYPE_AREA:
>>>>>>>>>>     		printf("%31s %#8.8x (area)   :", s.c_str(), qc.id);
>>>>>>>>>>     		break;
>>>>>>>>>> +	case V4L2_CTRL_TYPE_RECT:
>>>>>>>>>> +		printf("%31s %#8.8x (rect)   :", s.c_str(), qc.id);
>>>>>>>>>> +		break;
>>>>>>>>>>     	case V4L2_CTRL_TYPE_HDR10_CLL_INFO:
>>>>>>>>>>     		printf("%31s %#8.8x (hdr10-cll-info):", s.c_str(), qc.id);
>>>>>>>>>>     		break;
>>>>>>>>>> @@ -1279,6 +1286,11 @@ void common_set(cv4l_fd &_fd)
>>>>>>>>>>     					sscanf(set_ctrl.second.c_str(), "%ux%u",
>>>>>>>>>>     					       &ctrl.p_area->width, &ctrl.p_area->height);
>>>>>>>>>>     					break;
>>>>>>>>>> +				case V4L2_CTRL_TYPE_RECT:
>>>>>>>>>> +					sscanf(set_ctrl.second.c_str(), "%ux%u@%dx%d",
>>>>>>>>>> +					       &ctrl.p_rect->width, &ctrl.p_rect->height,
>>>>>>>>>> +					       &ctrl.p_rect->left, &ctrl.p_rect->top);
>>>>>>>>>> +					break;
>>>>>>>>>>     				default:
>>>>>>>>>>     					fprintf(stderr, "%s: unsupported payload type\n",
>>>>>>>>>>     							qc.name);
> 

Re: [PATCH v2 2/3] v4l2-ctl: Support V4L2_CTRL_TYPE_RECT

[Laurent Pinchart]

On Tue, Nov 05, 2024 at 10:01:32AM +0100, Hans Verkuil wrote:
> On 05/11/2024 09:51, Laurent Pinchart wrote:
> > On Tue, Nov 05, 2024 at 09:30:43AM +0100, Hans Verkuil wrote:
> >> On 04/11/2024 02:24, Ming Qian(OSS) wrote:
> >>> On 2024/10/31 18:09, Laurent Pinchart wrote:
> >>>> On Thu, Oct 31, 2024 at 05:46:49PM +0800, Ming Qian(OSS) wrote:
> >>>>> On 2024/10/31 17:34, Laurent Pinchart wrote:
> >>>>>> On Thu, Oct 31, 2024 at 05:19:02PM +0800, Ming Qian(OSS) wrote:
> >>>>>>> On 2024/10/30 17:19, Hans Verkuil wrote:
> >>>>>>>> On 30/10/2024 10:03, Laurent Pinchart wrote:
> >>>>>>>>> On Wed, Oct 30, 2024 at 11:43:06AM +0900, ming.qian@oss.nxp.com wrote:
> >>>>>>>>>> From: Yunke Cao <yunkec@google.com>
> >>>>>>>>>>
> >>>>>>>>>> Tested with VIVID
> >>>>>>>>>>
> >>>>>>>>>>     ./v4l2-ctl -C rect -d 0
> >>>>>>>>>> rect: 300x400@200x100
> >>>>>>>>>>
> >>>>>>>>>>     ./v4l2-ctl -c rect=1000x2000@0x0
> >>>>>>>>>>     ./v4l2-ctl -C rect -d 0
> >>>>>>>>>> rect: 1000x2000@0x0
> >>>>>>>>>>
> >>>>>>>>>> Signed-off-by: Yunke Cao <yunkec@google.com>
> >>>>>>>>>> Signed-off-by: Ming Qian <ming.qian@oss.nxp.com>
> >>>>>>>>>> ---
> >>>>>>>>>>     utils/v4l2-ctl/v4l2-ctl-common.cpp | 12 ++++++++++++
> >>>>>>>>>>     1 file changed, 12 insertions(+)
> >>>>>>>>>>
> >>>>>>>>>> diff --git a/utils/v4l2-ctl/v4l2-ctl-common.cpp b/utils/v4l2-ctl/v4l2-ctl-common.cpp
> >>>>>>>>>> index 40667575fcc7..538e1951cf81 100644
> >>>>>>>>>> --- a/utils/v4l2-ctl/v4l2-ctl-common.cpp
> >>>>>>>>>> +++ b/utils/v4l2-ctl/v4l2-ctl-common.cpp
> >>>>>>>>>> @@ -614,6 +614,10 @@ static void print_value(int fd, const v4l2_query_ext_ctrl &qc, const v4l2_ext_co
> >>>>>>>>>>     		case V4L2_CTRL_TYPE_AREA:
> >>>>>>>>>>     			printf("%dx%d", ctrl.p_area->width, ctrl.p_area->height);
> >>>>>>>>>>     			break;
> >>>>>>>>>> +		case V4L2_CTRL_TYPE_RECT:
> >>>>>>>>>> +			printf("%ux%u@%dx%d", ctrl.p_rect->width, ctrl.p_rect->height,
> >>>>>>>>>
> >>>>>>>>> I find this notation ambiguous, it's not immediately clear when reading
> >>>>>>>>> 10x10@20x20 if we're looking at a 10x10 rectangle positioned at (20,20)
> >>>>>>>>> or the other way around. media-ctl use (20,20)/10x10 which I think would
> >>>>>>>>> be a better notation.
> >>>>>>>>
> >>>>>>>> Good point, I agree.
> >>>>>>>>
> >>>>>>>> Ming Qian, can you also update patch 1/4 of the kernel patch series to
> >>>>>>>> use the same formatting when logging the V4L2_CTRL_TYPE_RECT value?
> >>>>>>>>
> >>>>>>>> Regards,
> >>>>>>>>
> >>>>>>>> 	Hans
> >>>>>>>
> >>>>>>> There is a issue in v4l2-utils, that ',' is the ending flag in
> >>>>>>> v4l_getsubopt(), then I can't set the rect control,
> >>>>>>> for example:
> >>>>>>>
> >>>>>>> $v4l2-ctl -d 0 -c rect="(0,0)/1000x2000"
> >>>>>>> control '0)/1000x2000' without '='
> >>>>>>
> >>>>>> The should be fixable in v4l_getsubopt().
> >>>>>>
> >>>>>
> >>>>> I can see the following comments of v4l_getsubopt(),
> >>>>>
> >>>>>      Parse comma separated suboption from *OPTIONP and match against
> >>>>>      strings in TOKENS.
> >>>>>
> >>>>> I am not sure if we can change it.
> >>>>
> >>>> I think we can improve quotes handling by considering quoted substrings
> >>>> as a single value, ignoring commas. Hans any opinion ?
> >>
> >> I think commas are hard to parse. Note that v4l_getsubopt is normally a
> >> #define for getsubopt from glibc. So you can't change the behavior of
> >> that function.
> > 
> > Can't we ? Isn't it an internal function ?
> 
> No. It's there when it is compiled for systems without glibc.
> 
> But I guess we can just use our copy all the time.
> 
> >> I propose this format for parsing instead:
> >>
> >> widthxheight@(top;left)
> >>
> >> e.g.: 1000x2000@(0;0)
> >>
> >> According to this:
> >> https://www.dr-aart.nl/Geometry-coordinates.html
> >>
> >> the ';' is the separator in countries where a decimal comma is used
> >> instead of a decimal point.
> >>
> >> I prefer to have the position after the size of the rectangle, for two
> >> reasons: it feels more natural to talk about a 'rectangle of size S at position
> >> P', and it also makes it possible to allow a variant where only the size
> >> is given and the position will default to (0;0). I.e., we can support
> >> parsing either "widthxheight" or "widthxheight@(top;left)".
> >>
> >> However, logging rectangles in the kernel should use a comma instead of a
> >> semicolon. Inside v4l-utils just consistently use the semicolon.
> >>
> >> What do you think, Laurent?
> > 
> > We have a precedent of using (x,y)/WxH , both in the kernel and in
> > media-ctl. Breaking that with another syntax would cause trouble,
> > especially having different syntaxes between media-ctl and v4l2-ctl.
> > Think about the shell scripts that would need to convert from one syntax
> > to another for instance. I would very strongly like to avoid that.
> 
> Have we used that notation in the kernel? Where?

In kernel log message. We have quite a few occurences of "(%d,%d)/%ux%u"
(or the less correct "(%d,%d)/%dx%d"). That's not an ABI, but it's still
used.

For what it's worth, we're also using the same notation in libcamera
(not necessarily as a result of a careful design decision, but likely
more because it was already used by media-ctl and nobody thought twice).

> I will admit that I am not a fan of the media-ctl notation, I think it is
> weird. I think WxH@(x,y) is much more natural. But if v4l_getsubopt can be
> adapted for this, then I'm fine with it.

I don't have anything against WxH@(x,y) per-se, but having different
notations in different tools is in my opinion a big enough annoyance
that a new notation shouldn't be introduced without very compeling
reasons.

> >>> How about omitting the commas between the brackets when parsing subopt?
> >>>
> >>>
> >>>>>>>>>> +			       ctrl.p_rect->left, ctrl.p_rect->top);
> >>>>>>>>>> +			break;
> >>>>>>>>>>     		default:
> >>>>>>>>>>     			printf("unsupported payload type");
> >>>>>>>>>>     			break;
> >>>>>>>>>> @@ -702,6 +706,9 @@ static void print_qctrl(int fd, const v4l2_query_ext_ctrl &qc,
> >>>>>>>>>>     	case V4L2_CTRL_TYPE_AREA:
> >>>>>>>>>>     		printf("%31s %#8.8x (area)   :", s.c_str(), qc.id);
> >>>>>>>>>>     		break;
> >>>>>>>>>> +	case V4L2_CTRL_TYPE_RECT:
> >>>>>>>>>> +		printf("%31s %#8.8x (rect)   :", s.c_str(), qc.id);
> >>>>>>>>>> +		break;
> >>>>>>>>>>     	case V4L2_CTRL_TYPE_HDR10_CLL_INFO:
> >>>>>>>>>>     		printf("%31s %#8.8x (hdr10-cll-info):", s.c_str(), qc.id);
> >>>>>>>>>>     		break;
> >>>>>>>>>> @@ -1279,6 +1286,11 @@ void common_set(cv4l_fd &_fd)
> >>>>>>>>>>     					sscanf(set_ctrl.second.c_str(), "%ux%u",
> >>>>>>>>>>     					       &ctrl.p_area->width, &ctrl.p_area->height);
> >>>>>>>>>>     					break;
> >>>>>>>>>> +				case V4L2_CTRL_TYPE_RECT:
> >>>>>>>>>> +					sscanf(set_ctrl.second.c_str(), "%ux%u@%dx%d",
> >>>>>>>>>> +					       &ctrl.p_rect->width, &ctrl.p_rect->height,
> >>>>>>>>>> +					       &ctrl.p_rect->left, &ctrl.p_rect->top);
> >>>>>>>>>> +					break;
> >>>>>>>>>>     				default:
> >>>>>>>>>>     					fprintf(stderr, "%s: unsupported payload type\n",
> >>>>>>>>>>     							qc.name);

-- 
Regards,

Laurent Pinchart