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

[Kieran Bingham]

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)?

>>> +
>>>  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

-- 
Regards
--
Kieran