Re: [PATCH v15 18/19] media: uvcvideo: implement UVC v1.5 ROI

[Hans de Goede <hdegoede@redhat.com>]

Hi Ricardo,

On 18-Nov-24 5:16 PM, Ricardo Ribalda Delgado wrote:
> Hi
> 
> On Mon, Nov 18, 2024 at 4:59 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Hi Ricardo,
>>
>> On 14-Nov-24 9:03 PM, Ricardo Ribalda wrote:
>>> Hi Gergo
>>>
>>> Sorry, I forgot to reply to your comment in v14.
>>>
>>> On Thu, 14 Nov 2024 at 20:53, Gergo Koteles <soyer@irl.hu> wrote:
>>>>
>>>> Hi Ricardo,
>>>>
>>>> On Thu, 2024-11-14 at 19:10 +0000, Ricardo Ribalda wrote:
>>>>>
>>>>> +     },
>>>>> +     {
>>>>> +             .id             = V4L2_CID_UVC_REGION_OF_INTEREST_AUTO,
>>>>> +             .entity         = UVC_GUID_UVC_CAMERA,
>>>>> +             .selector       = UVC_CT_REGION_OF_INTEREST_CONTROL,
>>>>> +             .size           = 16,
>>>>> +             .offset         = 64,
>>>>> +             .v4l2_type      = V4L2_CTRL_TYPE_BITMASK,
>>>>> +             .data_type      = UVC_CTRL_DATA_TYPE_BITMASK,
>>>>> +             .name           = "Region Of Interest Auto Controls",
>>>>> +     },
>>>>>  };
>>>>>
>>>>
>>>> Wouldn't be better to use 8 V4L2_CTRL_TYPE_BOOLEAN controls for this?
>>>
>>> If I create 8 Booleans, they will always be shown in the device. And
>>> the user will not have a way to know which values are available and
>>> which are not.
>>>
>>> We will also fail the v4l2-compliance test, because there will be up
>>> to 7 boolean controls that will not be able to be set to 1, eventhough
>>> they are writable.
>>
>> So why can't these other controls be set to 1? Because only one
>> of the options in the bitmask can be selected at a time ?
>>
>> If only 1 bit in the UVC_CTRL_DATA_TYPE_BITMASK for this can be one
>> at the time, then this should be mapped to a V4L2_CTRL_TYPE_MENU
>> just like how that is done for V4L2_CID_EXPOSURE_AUTO already.
>>
>> Actually looking at existing comments about UVC_CTRL_DATA_TYPE_BITMASK
>> in the driver there is this comment on top of uvc_mapping_get_menu_value()
>>
>>  * For controls of type UVC_CTRL_DATA_TYPE_BITMASK, the UVC control value is
>>  * expressed as a bitmask and is thus guaranteed to have a single bit set.
>>
>> Assuming this "guaranteed to have a single bit set" comment is valid for
>> the V4L2_CID_UVC_REGION_OF_INTEREST_AUTO part of UVC_CT_REGION_OF_INTEREST_CONTROL
>> too then I think we should simply map this to a menu similar to how
>> this is done for V4L2_CID_EXPOSURE_AUTO.
>>
>> Note V4L2_CID_EXPOSURE_AUTO is the only existing user of UVC_CTRL_DATA_TYPE_BITMASK
>> at the moment.
>>
>> Mapping this to a menu should nicely address Gergo's concerns here.
> 
> The UVC standard is not very clear re bmAutoControls. It says:
> """
> The bmAutoControls bitmask determines which, if any, on board features
> should track to the region of interest. To detect if a device supports
> a particular Auto Control, use GET_MAX which returns a mask indicating
> all supported Auto Controls.
> GET_CUR returns the current Region of Interest (RoI) being employed by
> the device. This RoI should be the same as specified in most recent
> SET_CUR except in the case where the ‘Auto Detect and Track’ and/or
> ‘Image Stabilization’ bit have been set.
> """
> 
> Which makes me believe that you can set another Auto value + one of
> these ones. So I do not think that we can assume "guaranteed to have a
> single bit set".

I see I already was afraid it would be something like this but
it would have been nice if we could have turned this into a menu control.

> The behaviour will vary module to module. So I'd rather take a
> conservative approach here and let the hardware clamp the value and
> not us.

Agreed.

Regards,

Hans