Re: [REVIEWv2 PATCH 24/34] v4l2-ctrls/videodev2.h: add u8 and u16 types.

[Hans Verkuil <hansverk@cisco.com>]

On 02/12/14 14:40, Ricardo Ribalda Delgado wrote:
> Hello Hans
> 
> 
> 
> On Wed, Feb 12, 2014 at 2:26 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>> On 02/12/14 14:13, Ricardo Ribalda Delgado wrote:
>>> Hello Hans
>>>
>>> Thanks for you promptly response
>>>
>>> On Wed, Feb 12, 2014 at 1:40 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>>> On 02/12/14 13:11, Ricardo Ribalda Delgado wrote:
>>>>> Hi Hans
>>>>>
>>>>> Thanks for your reply
>>>>>
>>>>> On Wed, Feb 12, 2014 at 12:20 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>>>>> Hi Ricardo,
>>>>>>
>>>>>> On 02/12/14 11:44, Ricardo Ribalda Delgado wrote:
>>>>>>> Hello Hans
>>>>>>>
>>>>>>> In the case of U8 and U16 data types. Why dont you fill the elem_size
>>>>>>> automatically in v4l2_ctrl and request the driver to fill the field?
>>>>>>
>>>>>> When you create the control the control framework has to know the element
>>>>>> size beforehand as it will use that to allocate the memory containing the
>>>>>> control's value. The control framework is aware of the 'old' control types
>>>>>> and will fill in the elem_size accordingly, but it cannot do that in the
>>>>>> general case for these complex types. I guess it could be filled in by the
>>>>>> framework for the more common types (U8, U16) but I felt it was more
>>>>>> consistent to just require drivers to fill it in manually, rather than have
>>>>>> it set for some types but not for others.
>>>>>>
>>>>>>>
>>>>>>> Other option would be not declaring the basic data types (U8, U16,
>>>>>>> U32...) and use elem_size. Ie. If type==V4L2_CTRL_COMPLEX_TYPES, then
>>>>>>> the type is basic and elem_size is the size of the type. If the type
>>>>>>>> V4L2_CTRL_COMPLEX_TYPES the type is not basic.
>>>>>>
>>>>>> You still need to know the type. Applications have to be able to check for
>>>>>> the type, the element size by itself doesn't tell you how to interpret the
>>>>>> data, you need the type identifier as well.
>>>>>
>>>>> I think that the driver is setting twice the same info. I see no gain
>>>>> in declaring U8, U16 types etc if we still have to set the element
>>>>> size. This is why I believe that we should only declare the "structs".
>>>>
>>>> Just to make sure I understand you: for simple types like U8/U16 you want
>>>> the control framework to fill in elem_size, for more complex types (structs)
>>>> you want the driver to fill in elem_size?
>>>
>>> I dont like that the type contains the size of the element, and then I
>>> have to provide the size again. (Hungarian notation)
>>>
>>> Instead, I think it is better:
>>>
>>> Defines ONLY this two types for simple types:
>>> V4L2_CTRL_COMPLEX_TYPE_SIGNED_INTEGER and
>>> V4L2_CTRL_COMPLEX_TYPE_UNSIGNED_INTEGER and use elem_size to determine
>>> the size.
>>
>> It sounds great, but it isn't in practice because this will produce awful
>> code like this:
>>
>> switch (type) {
>> case V4L2_CTRL_COMPLEX_TYPE_SIGNED_INTEGER:
>>         switch (elem_size) {
>>         case 1: // it's a u8!
>>                 break;
>>         case 2: // it's a u16!
>>                 break;
>>         }
>> etc.
>> }
> 
> I slightly disagree here. Your proposal will produce this code:
> 
> 
> case V4L2_CTRL_TYPE_U8:
> ptr.p_u8[idx] = ctrl->default_value;
> break;
> case V4L2_CTRL_TYPE_U16:
> ptr.p_u16[idx] = ctrl->default_value;
> break;
> case V4L2_CTRL_TYPE_U32:
> ptr.p_s32[idx] = ctrl->default_value;
> break;
> case V4L2_CTRL_TYPE_U64:
> ptr.p_s64[idx] = ctrl->default_value;
> break;
> case V4L2_CTRL_TYPE_S8:
> ptr.p_s8[idx] = ctrl->default_value;
> break;
> case V4L2_CTRL_TYPE_S16:
> ptr.p_s16[idx] = ctrl->default_value;
> break;
> case V4L2_CTRL_TYPE_S32:
> ptr.p_s32[idx] = ctrl->default_value;
> break;
> case V4L2_CTRL_TYPE_S64:
> ptr.p_s64[idx] = ctrl->default_value;
> break;
> 
> instead of:
> 
> 
> case V4L2_CTRL_COMPLEX_TYPE_SIGNED_INTEGER:
> case V4L2_CTRL_COMPLEX_TYPE_UNSIGNED_INTEGER:
> memcpy(&ptr.p[idx],&ctrl->default_value,ctrl->elem_size)

Well, no. default_value is a 64-bit value that you can't just memcpy to
a u8 (certainly not on a big-endian system). It's basically why I split
it up, otherwise I would have used the same trick instead of writing it
out for every type.

Regards,

	Hans