| Message ID | 20190312121242.2253-3-jacopo@jmondi.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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@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? > + > 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? > + */ > + > /** > * \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; > >
Hi Jacopo, Thank you for the patch. On Tue, Mar 12, 2019 at 01:12:30PM +0100, 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@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; > + > 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); While at it you could drop the const. > > 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. > + */ > + > /** > * \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; >
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@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? > > > + > > 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? 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
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@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
Hi Laurent, On Wed, Mar 13, 2019 at 07:14:22PM +0200, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > On Tue, Mar 12, 2019 at 01:12:30PM +0100, 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@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; > > + > > 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); > > While at it you could drop the const. > Not just could, but should. As SubdevFormatEnum is returned by value, returning it as const prevents the compiler from invoking the move constructor of the variable the return value of formats() is assigned to, if I got this right... I'll change this. Thanks j > > > > 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. > > + */ > > + > > /** > > * \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, > > Laurent Pinchart
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@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
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@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; >>>>> >>>>>
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; + 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. + */ + /** * \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;
Provide a type definition for the map used to enumerate the subdevice image formats and associated sizes. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- src/libcamera/include/v4l2_subdevice.h | 5 +++-- src/libcamera/v4l2_subdevice.cpp | 14 +++++++++++--- 2 files changed, 14 insertions(+), 5 deletions(-)