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

[Jacopo Mondi]

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.

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.

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
>
> --
> Regards
> --
> Kieran
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20190314/54ab4101/attachment.sig>