| Message ID | 20241030024307.1114787-3-ming.qian@oss.nxp.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Support V4L2_CTRL_TYPE_RECT and V4L2_CTRL_WHICH_MIN/MAX_VAL | expand |
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);
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); >
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); >
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); >> >
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); >> >
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);
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); >
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);
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); >
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); >>
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);
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); >
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);
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);