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@xxxxxxxxxxx wrote:
>>>>>>>> From: Yunke Cao <yunkec@xxxxxxxxxx>
>>>>>>>>
>>>>>>>> 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@xxxxxxxxxx>
>>>>>>>> Signed-off-by: Ming Qian <ming.qian@xxxxxxxxxxx>
>>>>>>>> ---
>>>>>>>> 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);
>>
