diff mbox series

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

Message ID 20241030024307.1114787-3-ming.qian@oss.nxp.com
State New
Headers show
Series Support V4L2_CTRL_TYPE_RECT and V4L2_CTRL_WHICH_MIN/MAX_VAL | expand

Commit Message

Ming Qian(OSS) Oct. 30, 2024, 2:43 a.m. UTC 
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(+)

Comments

Ming Qian(OSS) Oct. 30, 2024, 9:21 a.m. UTC | #1 
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);
>
Laurent Pinchart Oct. 31, 2024, 9:34 a.m. UTC | #2 
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);
Ming Qian(OSS) Oct. 31, 2024, 9:46 a.m. UTC | #3 
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);
>
Laurent Pinchart Oct. 31, 2024, 10:09 a.m. UTC | #4 
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);
diff mbox series

Patch

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);