[libcamera-devel] [PATCH v2 02/14] libcamera: v4l2_subdevice: Define format enumeration type

[Kieran Bingham]

Hi Jacopo,

On 14/03/2019 10:46, Jacopo Mondi wrote:
> Hi Kieran,
> 
> On Thu, Mar 14, 2019 at 10:30:04AM +0000, Kieran Bingham wrote:
>> Heya,
>>
>> On 14/03/2019 10:10, Jacopo Mondi wrote:
>>> Hi Kieran,
>>>
>>> On Tue, Mar 12, 2019 at 12:22:46PM +0000, Kieran Bingham wrote:
>>>> Hi Jacopo,
>>>>
>>>> On 12/03/2019 12:12, Jacopo Mondi wrote:
>>>>> Provide a type definition for the map used to enumerate the subdevice
>>>>> image formats and associated sizes.
>>>>>
>>>>> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
>>>>> ---
>>>>>  src/libcamera/include/v4l2_subdevice.h |  5 +++--
>>>>>  src/libcamera/v4l2_subdevice.cpp       | 14 +++++++++++---
>>>>>  2 files changed, 14 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/src/libcamera/include/v4l2_subdevice.h b/src/libcamera/include/v4l2_subdevice.h
>>>>> index 700e66bcddac..15add95a825c 100644
>>>>> --- a/src/libcamera/include/v4l2_subdevice.h
>>>>> +++ b/src/libcamera/include/v4l2_subdevice.h
>>>>> @@ -17,6 +17,8 @@
>>>>>
>>>>>  namespace libcamera {
>>>>>
>>>>> +typedef std::map<unsigned int, std::vector<SizeRange>> SubdevFormatEnum;
>>>>
>>>> Should this have a V4L2 prefix? or is that overkill?
>>>>
>>>
>>> I accept suggestions. To me it's an overkill, but I understand formats
>>> and V4L2-related APIs are "V4L2" prefixed.
>>>
>>> What would you do here?
>>
>> Not sure - it just stood out as not having the same prefix :)
>>
>> Will the V4L2Device need a similiar formatEnum type?
>>
>> If so (or if hypothetically so), how would we distinguish between them.
>>
>> Or - Would the same 'type' be usable for both? - in which case this
>> would become V4L2FormatEnum perhaps? (i.e. it's not specific to subdevs)?
>>
> 
> Good question. V4L2 devices equally have ENUM_FMT and ENUM_FRAMESIZES,
> so they could use the same type we use for subdevices.
> 
> I would move this to geometry, but then I would drop any V4L2
> reference and use FormatEnum as a type name. Or better, tackle this
> when we'll eventually handle formats, and store this with the other
> format-related types in a dedicated header, and leave it here with a
> \todo for later.

I guess it depends on how much time you have for this right now.

Either way, it sounds like this 'type' should be "FormatEnum" without
the prefix if it is generic (and essentially a libcamera type, rather
than a V4L2{Device,Subdev} type.)

I'd say it can live here if it doesn't yet have a real home, with a
todo: that it will move to 'formats.h' when it exists.


> Not sure what to do actually. I would leave it here with a \todo note,
> and move it wither to a format header or to geometry if we implement
> enum formats for V4L2 devices too.

Is geometry a suitable place for it at the moment? or would it also need
a todo: move to formats.h if it was there?


How about keep it here, with the todo: move to formats.h - and that will
force us to handle formats.h when we tackle formats on the V4L2Device?

--
KB



> Thanks
>   j
> 
>>>>> +
>>>>>  struct V4L2SubdeviceFormat {
>>>>>  	uint32_t mbus_code;
>>>>>  	uint32_t width;
>>>>> @@ -42,8 +44,7 @@ public:
>>>>>  	int setCrop(unsigned int pad, Rectangle *rect);
>>>>>  	int setCompose(unsigned int pad, Rectangle *rect);
>>>>>
>>>>> -	const std::map<unsigned int, std::vector<SizeRange>>
>>>>> -						formats(unsigned int pad);
>>>>> +	const SubdevFormatEnum formats(unsigned int pad);
>>>>>
>>>>>  	int getFormat(unsigned int pad, V4L2SubdeviceFormat *format);
>>>>>  	int setFormat(unsigned int pad, V4L2SubdeviceFormat *format);
>>>>> diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp
>>>>> index 7c56ba3ba510..153330bccc97 100644
>>>>> --- a/src/libcamera/v4l2_subdevice.cpp
>>>>> +++ b/src/libcamera/v4l2_subdevice.cpp
>>>>> @@ -28,6 +28,15 @@ namespace libcamera {
>>>>>
>>>>>  LOG_DEFINE_CATEGORY(V4L2Subdev)
>>>>>
>>>>> +/**
>>>>> + * \typedef SubdevFormatEnum
>>>>> + * \brief Type definition for the map of image formats and associated sizes
>>>>> + *
>>>>> + * Type definition for the map of media bus codes and associated image
>>>>> + * resolutions used by V4L2Subdevice to report the enumeration of supported
>>>>> + * image formats.
>>>>
>>>> Should/Could we document how to use this new 'type' in here somehow as
>>>> the underlying type is now a bit obfuscated?
>>>>
>>>
>>> Not sure heither how to explain this better than looking at the type,
>>> and seeing it's a map which associates media bus code to a vector of
>>> image resolutions. Ah, see! I just explained what the new type
>>> obscures. Should I add this?
>>
>> Aha - now that's sounding clearer :)
>>
>>
>>> Thanks
>>>   j
>>>
>>>>
>>>>> + */
>>>>> +
>>>>>  /**
>>>>>   * \struct V4L2SubdeviceFormat
>>>>>   * \brief The V4L2 sub-device image format and sizes
>>>>> @@ -210,10 +219,9 @@ int V4L2Subdevice::setCompose(unsigned int pad, Rectangle *rect)
>>>>>   * \return A map of image formats associated with a list of image sizes, or
>>>>>   * an empty map on error or if the pad does not exist
>>>>>   */
>>>>> -const std::map<unsigned int, std::vector<SizeRange>>
>>>>> -V4L2Subdevice::formats(unsigned int pad)
>>>>> +const SubdevFormatEnum V4L2Subdevice::formats(unsigned int pad)
>>>>>  {
>>>>> -	std::map<unsigned int, std::vector<SizeRange>> formatMap = {};
>>>>> +	SubdevFormatEnum formatMap = {};
>>>>>  	struct v4l2_subdev_mbus_code_enum mbusEnum = {};
>>>>>  	int ret;
>>>>>
>>>>>


-- 
Regards
--
Kieran