[libcamera-devel,05/17] libcamera: formats: Add V4L2DeviceFormats and V4L2DeviceFormats

1298 diff mbox series

Add two new objects to hold v4l2 format information for v4l2 devices and
subdevices. There is much code which can be shared between the two so
have both of them inherit from a base class to reduce code duplication.

Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
---
 src/libcamera/formats.cpp       | 158 ++++++++++++++++++++++++++++++++
 src/libcamera/include/formats.h |  35 +++++++
 2 files changed, 193 insertions(+)

Hi Niklas,

On Mon, May 27, 2019 at 02:15:31AM +0200, Niklas Söderlund wrote:
> Add two new objects to hold v4l2 format information for v4l2 devices and
> subdevices. There is much code which can be shared between the two so
> have both of them inherit from a base class to reduce code duplication.
>

I still think we should have gone this way from the very beginning, so
I think this is the direction to go.

One question:
1) looking at the patch, it seems to me now V4L2DeviceFormats and
V4L2SubdeviceFormats only differns in the name of the formats map key
(pixelcode and mbus code). I wonder if we shouldn't better unify those
(maybe renaming DeviceFormats in just ImageFormats).

> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> ---
>  src/libcamera/formats.cpp       | 158 ++++++++++++++++++++++++++++++++
>  src/libcamera/include/formats.h |  35 +++++++
>  2 files changed, 193 insertions(+)
>
> diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp
> index 56f4ddb51ffad4d3..f5841b38cea5686c 100644
> --- a/src/libcamera/formats.cpp
> +++ b/src/libcamera/formats.cpp
> @@ -24,4 +24,162 @@ namespace libcamera {
>   * resolutions represented by SizeRange items.
>   */
>
> +
> +/**
> + * \class DeviceFormats
> + * \brief Base class for V4L2Device and V4L2SubDevice Formats
> + *
> + * The base class holds common functionallity between V4L2DeviceFormats and

s/functionallity/functionalities/

> + * V4L2SubDeviceForamts.

s/Foramats/Formats/ here an in other places
> + *
> + * \sa V4L2DeviceFormats
> + * \sa V4L2SubDeviceForamts
> + */
> +
> +/**
> + * \brief Create an empty DeviceFormats
> + */
> +DeviceFormats::DeviceFormats()
> +{
> +}
> +
> +/**
> + * \brief Create a DeviceFormats with data

with data/using the formats and size provided by \a data

missing the data argument documentation.

Documentation apart, we're now creating a DeviceFormats from an
already built map, which require users of this class to build the map
by themselves

Have you considered the possibility of creating an empty DeviceFormats
and later populated it with:

        addSizes(unsigned int fmt, std::vector<SizeRange> &sizes)

Otherwise the interface to populate the formats stays as awkward to
use as it is today.

> + */
> +DeviceFormats::DeviceFormats(const std::map<unsigned int, std::vector<SizeRange>> &data)
> +	: data_(data)

s/data/map ?

> +{
> +}
> +
> +DeviceFormats::~DeviceFormats()
> +{
> +}
> +
> +/**
> + * \brief Retrieve all sizes for a specific format
> + * \param[in] format A pixelformat or mbus code
> + *
> + * Retrieves all SizeRanges for a specific format. For V4L2Device \a format is a

We usually use "Retrieve"

> + * pixelformoat while for a V4L2Subdevice \a format is a media bus code.

pixelformoat/pixel format code/

As the difference is the name of the field only, I would rather unify
the two derived classes.

> + *
> + * \return List of SizeRanges for \a format, empty list if format is not valid

The list of image sizes produced using \a format, or an empty list if format is not supported.

> + */
> +std::vector<SizeRange> DeviceFormats::sizes(unsigned int format) const
> +{
> +	auto const &it = data_.find(format);
> +	if (it == data_.end())
> +		return {};
> +
> +	return it->second;
> +}
> +
> +/**
> + * \brief Check if the device formats is empty

Check if the list of devices supported formats is empty

> + * \return True if the formats are empty

"True if the list of supported formats is empty"

> + */
> +bool DeviceFormats::empty() const
> +{
> +	return data_.empty();
> +}
> +
> +/**
> + * \brief Retrieve the raw dataA

s/dataA/data
and I would
s/data/the map that associates formats to image sizes/

I would rather not let users access the whole map. Do you have an use
case for this?

> + * \return Raw map containgin formats to SizeRanges

The map that associates formats to image sizes

> + */
> +const std::map<unsigned int, std::vector<SizeRange>> &DeviceFormats::data()
> +{
> +	return data_;
> +}
> +
> +/**
> + * \brief Retrieve a list all contained formats

Retrieve a list of all supported image formats

> + *
> + * This is a helper function intended to be used by V4L2DeviceFormats and
> + * V4L2SubdeviceFormats.
> + *
> + * \return A list of formats contained

"of supported formats"

> + */
> +std::vector<unsigned int> DeviceFormats::formats() const
> +{
> +	std::vector<unsigned int> formats;
> +
> +	for (auto const &it : data_)
> +		formats.push_back(it.first);
> +
> +	return formats;

I'm very disappointed std::map does not have a keys() nor a values()
method.

> +}
> +
> +/**
> + * \var DeviceFormats::data_
> + * \brief The map holding format and SizeRange information

Associating image formats to sizes.

> + */
> +
> +/**
> + * \class V4L2SubdeviceFormats
> + * \brief Holds media bus codes to frame sizes information for a v4l2 subdevice
> + *
> + * Hold media bus codes and frame sizes which describes a v4l2 subdevice. The
> + * intended user of this object is pipeline handlers which can create it
> + * from a V4L2Subdevice and use it to describe and validate formats.
> + */
> +
> +/**
> + * \brief Create an empty V4L2SubdeviceFormats
> + */
> +V4L2SubdeviceFormats::V4L2SubdeviceFormats()
> +	: DeviceFormats()
> +{
> +}

How do you fill in an empty V4L2SubdeviceFormats ?

> +
> +/**
> + * \brief Create an V4L2SubdeviceFormats with data
> + */
> +V4L2SubdeviceFormats::V4L2SubdeviceFormats(const std::map<unsigned int, std::vector<SizeRange>> &data)
> +	: DeviceFormats(data)
> +{
> +}
> +
> +/**
> + * \brief Retrieve media bus codes which are described
which are described ... ?

I would just "Retrieve the list of supported media bus codes"

> + * \return List of media bus codes
> + */
> +std::vector<unsigned int> V4L2SubdeviceFormats::codes() const
> +{
> +	return formats();
> +}
> +
> +/**
> + * \class V4L2DeviceFormats
> + * \brief Holds pixelformats to frame sizes information for a v4l2 device
> + *
> + * Hold pixelformats and frame sizes which describes a v4l2 device. The
> + * intended user of this object is pipeline handlers which can create it
> + * from a V4L2Device and use it to describe and validate formats.
> + */
> +
> +/**
> + * \brief Create an empty V4L2DeviceFormats
> + */
> +V4L2DeviceFormats::V4L2DeviceFormats()
> +	: DeviceFormats()
> +{
> +}

Same question

> +
> +/**
> + * \brief Create an V4L2DeviceFormats with data
> + */
> +V4L2DeviceFormats::V4L2DeviceFormats(const std::map<unsigned int, std::vector<SizeRange>> &data)
> +	: DeviceFormats(data)
> +{
> +}
> +
> +/**
> + * \brief Retrieve pixelformats which are described
> + * \return List of pixelformats
> + */
> +std::vector<unsigned int> V4L2DeviceFormats::pixelformats() const
> +{
> +	return formats();
> +}

And same comment again: do we now need V4L2Device and V4L2Subdevice
formats? I think it's easier for pipeline handler to deal with formats
and sizes through a single interface, even if we loose the
pixelcode/media bus code/ distinction, which is by the way only
specified at documentation level.

Thanks
  j


> +
>  } /* namespace libcamera */
> diff --git a/src/libcamera/include/formats.h b/src/libcamera/include/formats.h
> index a73772b1eda068b4..372f6e6d71b236dd 100644
> --- a/src/libcamera/include/formats.h
> +++ b/src/libcamera/include/formats.h
> @@ -17,6 +17,41 @@ namespace libcamera {
>
>  typedef std::map<unsigned int, std::vector<SizeRange>> FormatEnum;
>
> +class DeviceFormats
> +{
> +public:
> +	virtual ~DeviceFormats();
> +
> +	std::vector<SizeRange> sizes(unsigned int format) const;
> +	bool empty() const;
> +	const std::map<unsigned int, std::vector<SizeRange>> &data();
> +
> +protected:
> +	DeviceFormats();
> +	DeviceFormats(const std::map<unsigned int, std::vector<SizeRange>> &data);
> +	std::vector<unsigned int> formats() const;
> +
> +	std::map<unsigned int, std::vector<SizeRange>> data_;
> +};
> +
> +class V4L2SubdeviceFormats : public DeviceFormats
> +{
> +public:
> +	V4L2SubdeviceFormats();
> +	V4L2SubdeviceFormats(const std::map<unsigned int, std::vector<SizeRange>> &data);
> +
> +	std::vector<unsigned int> codes() const;
> +};
> +
> +class V4L2DeviceFormats : public DeviceFormats
> +{
> +public:
> +	V4L2DeviceFormats();
> +	V4L2DeviceFormats(const std::map<unsigned int, std::vector<SizeRange>> &data);
> +
> +	std::vector<unsigned int> pixelformats() const;
> +};
> +
>  } /* namespace libcamera */
>
>  #endif /* __LIBCAMERA_FORMATS_H__ */
> --
> 2.21.0
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Hi Niklas,

On 27/05/2019 11:05, Jacopo Mondi wrote:
> Hi Niklas,
> 
> On Mon, May 27, 2019 at 02:15:31AM +0200, Niklas Söderlund wrote:
>> Add two new objects to hold v4l2 format information for v4l2 devices and
>> subdevices. There is much code which can be shared between the two so
>> have both of them inherit from a base class to reduce code duplication.
>>
> 
> I still think we should have gone this way from the very beginning, so
> I think this is the direction to go.
> 
> One question:
> 1) looking at the patch, it seems to me now V4L2DeviceFormats and
> V4L2SubdeviceFormats only differns in the name of the formats map key
> (pixelcode and mbus code). I wonder if we shouldn't better unify those
> (maybe renaming DeviceFormats in just ImageFormats).
> 

Also, I'm a bit weary that we will now have V4L2DeviceFormat and
V4L2DeviceFormats which are quite different

 (xxxxFormats is not a direct array of xxxFormat)

Same for both V4L2DeviceFormats and V4L2SubdeviceFormats.

Do you think this is a problem? Or will this be ok?

Or perhaps it's a moot point, as I think Jacopo's comments look like
suggesting unifying this some how so the naming will be less ambiguous
then anyway.

--
Kieran


>> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
>> ---
>>  src/libcamera/formats.cpp       | 158 ++++++++++++++++++++++++++++++++
>>  src/libcamera/include/formats.h |  35 +++++++
>>  2 files changed, 193 insertions(+)
>>
>> diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp
>> index 56f4ddb51ffad4d3..f5841b38cea5686c 100644
>> --- a/src/libcamera/formats.cpp
>> +++ b/src/libcamera/formats.cpp
>> @@ -24,4 +24,162 @@ namespace libcamera {
>>   * resolutions represented by SizeRange items.
>>   */
>>
>> +
>> +/**
>> + * \class DeviceFormats
>> + * \brief Base class for V4L2Device and V4L2SubDevice Formats
>> + *
>> + * The base class holds common functionallity between V4L2DeviceFormats and
> 
> s/functionallity/functionalities/
> 
>> + * V4L2SubDeviceForamts.
> 
> s/Foramats/Formats/ here an in other places
>> + *
>> + * \sa V4L2DeviceFormats
>> + * \sa V4L2SubDeviceForamts
>> + */
>> +
>> +/**
>> + * \brief Create an empty DeviceFormats
>> + */
>> +DeviceFormats::DeviceFormats()
>> +{
>> +}
>> +
>> +/**
>> + * \brief Create a DeviceFormats with data
> 
> with data/using the formats and size provided by \a data
> 
> missing the data argument documentation.
> 
> Documentation apart, we're now creating a DeviceFormats from an
> already built map, which require users of this class to build the map
> by themselves
> 
> Have you considered the possibility of creating an empty DeviceFormats
> and later populated it with:
> 
>         addSizes(unsigned int fmt, std::vector<SizeRange> &sizes)
> 
> Otherwise the interface to populate the formats stays as awkward to
> use as it is today.
> 
>> + */
>> +DeviceFormats::DeviceFormats(const std::map<unsigned int, std::vector<SizeRange>> &data)
>> +	: data_(data)
> 
> s/data/map ?
> 
>> +{
>> +}
>> +
>> +DeviceFormats::~DeviceFormats()
>> +{
>> +}
>> +
>> +/**
>> + * \brief Retrieve all sizes for a specific format
>> + * \param[in] format A pixelformat or mbus code
>> + *
>> + * Retrieves all SizeRanges for a specific format. For V4L2Device \a format is a
> 
> We usually use "Retrieve"
> 
>> + * pixelformoat while for a V4L2Subdevice \a format is a media bus code.
> 
> pixelformoat/pixel format code/
> 
> As the difference is the name of the field only, I would rather unify
> the two derived classes.
> 
>> + *
>> + * \return List of SizeRanges for \a format, empty list if format is not valid
> 
> The list of image sizes produced using \a format, or an empty list if format is not supported.
> 
>> + */
>> +std::vector<SizeRange> DeviceFormats::sizes(unsigned int format) const
>> +{
>> +	auto const &it = data_.find(format);
>> +	if (it == data_.end())
>> +		return {};
>> +
>> +	return it->second;
>> +}
>> +
>> +/**
>> + * \brief Check if the device formats is empty
> 
> Check if the list of devices supported formats is empty
> 
>> + * \return True if the formats are empty
> 
> "True if the list of supported formats is empty"
> 
>> + */
>> +bool DeviceFormats::empty() const
>> +{
>> +	return data_.empty();
>> +}
>> +
>> +/**
>> + * \brief Retrieve the raw dataA
> 
> s/dataA/data
> and I would
> s/data/the map that associates formats to image sizes/
> 
> I would rather not let users access the whole map. Do you have an use
> case for this?
> 
>> + * \return Raw map containgin formats to SizeRanges
> 
> The map that associates formats to image sizes
> 
>> + */
>> +const std::map<unsigned int, std::vector<SizeRange>> &DeviceFormats::data()
>> +{
>> +	return data_;
>> +}
>> +
>> +/**
>> + * \brief Retrieve a list all contained formats
> 
> Retrieve a list of all supported image formats
> 
>> + *
>> + * This is a helper function intended to be used by V4L2DeviceFormats and
>> + * V4L2SubdeviceFormats.
>> + *
>> + * \return A list of formats contained
> 
> "of supported formats"
> 
>> + */
>> +std::vector<unsigned int> DeviceFormats::formats() const
>> +{
>> +	std::vector<unsigned int> formats;
>> +
>> +	for (auto const &it : data_)
>> +		formats.push_back(it.first);
>> +
>> +	return formats;
> 
> I'm very disappointed std::map does not have a keys() nor a values()
> method.
> 
>> +}
>> +
>> +/**
>> + * \var DeviceFormats::data_
>> + * \brief The map holding format and SizeRange information
> 
> Associating image formats to sizes.
> 
>> + */
>> +
>> +/**
>> + * \class V4L2SubdeviceFormats
>> + * \brief Holds media bus codes to frame sizes information for a v4l2 subdevice
>> + *
>> + * Hold media bus codes and frame sizes which describes a v4l2 subdevice. The
>> + * intended user of this object is pipeline handlers which can create it
>> + * from a V4L2Subdevice and use it to describe and validate formats.
>> + */
>> +
>> +/**
>> + * \brief Create an empty V4L2SubdeviceFormats
>> + */
>> +V4L2SubdeviceFormats::V4L2SubdeviceFormats()
>> +	: DeviceFormats()
>> +{
>> +}
> 
> How do you fill in an empty V4L2SubdeviceFormats ?
> 
>> +
>> +/**
>> + * \brief Create an V4L2SubdeviceFormats with data
>> + */
>> +V4L2SubdeviceFormats::V4L2SubdeviceFormats(const std::map<unsigned int, std::vector<SizeRange>> &data)
>> +	: DeviceFormats(data)
>> +{
>> +}
>> +
>> +/**
>> + * \brief Retrieve media bus codes which are described
> which are described ... ?
> 
> I would just "Retrieve the list of supported media bus codes"
> 
>> + * \return List of media bus codes
>> + */
>> +std::vector<unsigned int> V4L2SubdeviceFormats::codes() const
>> +{
>> +	return formats();
>> +}
>> +
>> +/**
>> + * \class V4L2DeviceFormats
>> + * \brief Holds pixelformats to frame sizes information for a v4l2 device
>> + *
>> + * Hold pixelformats and frame sizes which describes a v4l2 device. The
>> + * intended user of this object is pipeline handlers which can create it
>> + * from a V4L2Device and use it to describe and validate formats.
>> + */
>> +
>> +/**
>> + * \brief Create an empty V4L2DeviceFormats
>> + */
>> +V4L2DeviceFormats::V4L2DeviceFormats()
>> +	: DeviceFormats()
>> +{
>> +}
> 
> Same question
> 
>> +
>> +/**
>> + * \brief Create an V4L2DeviceFormats with data
>> + */
>> +V4L2DeviceFormats::V4L2DeviceFormats(const std::map<unsigned int, std::vector<SizeRange>> &data)
>> +	: DeviceFormats(data)
>> +{
>> +}
>> +
>> +/**
>> + * \brief Retrieve pixelformats which are described
>> + * \return List of pixelformats
>> + */
>> +std::vector<unsigned int> V4L2DeviceFormats::pixelformats() const
>> +{
>> +	return formats();
>> +}
> 
> And same comment again: do we now need V4L2Device and V4L2Subdevice
> formats? I think it's easier for pipeline handler to deal with formats
> and sizes through a single interface, even if we loose the
> pixelcode/media bus code/ distinction, which is by the way only
> specified at documentation level.
> 
> Thanks
>   j
> 
> 
>> +
>>  } /* namespace libcamera */
>> diff --git a/src/libcamera/include/formats.h b/src/libcamera/include/formats.h
>> index a73772b1eda068b4..372f6e6d71b236dd 100644
>> --- a/src/libcamera/include/formats.h
>> +++ b/src/libcamera/include/formats.h
>> @@ -17,6 +17,41 @@ namespace libcamera {
>>
>>  typedef std::map<unsigned int, std::vector<SizeRange>> FormatEnum;
>>
>> +class DeviceFormats
>> +{
>> +public:
>> +	virtual ~DeviceFormats();
>> +
>> +	std::vector<SizeRange> sizes(unsigned int format) const;
>> +	bool empty() const;
>> +	const std::map<unsigned int, std::vector<SizeRange>> &data();
>> +
>> +protected:
>> +	DeviceFormats();
>> +	DeviceFormats(const std::map<unsigned int, std::vector<SizeRange>> &data);
>> +	std::vector<unsigned int> formats() const;
>> +
>> +	std::map<unsigned int, std::vector<SizeRange>> data_;
>> +};
>> +
>> +class V4L2SubdeviceFormats : public DeviceFormats
>> +{
>> +public:
>> +	V4L2SubdeviceFormats();
>> +	V4L2SubdeviceFormats(const std::map<unsigned int, std::vector<SizeRange>> &data);
>> +
>> +	std::vector<unsigned int> codes() const;
>> +};
>> +
>> +class V4L2DeviceFormats : public DeviceFormats
>> +{
>> +public:
>> +	V4L2DeviceFormats();
>> +	V4L2DeviceFormats(const std::map<unsigned int, std::vector<SizeRange>> &data);
>> +
>> +	std::vector<unsigned int> pixelformats() const;
>> +};
>> +
>>  } /* namespace libcamera */
>>
>>  #endif /* __LIBCAMERA_FORMATS_H__ */
>> --
>> 2.21.0
>>
>> _______________________________________________
>> libcamera-devel mailing list
>> libcamera-devel@lists.libcamera.org
>> https://lists.libcamera.org/listinfo/libcamera-devel
>>
>> _______________________________________________
>> libcamera-devel mailing list
>> libcamera-devel@lists.libcamera.org
>> https://lists.libcamera.org/listinfo/libcamera-devel

Hi Niklas,

On Wed, May 29, 2019 at 10:36:56PM +0100, Kieran Bingham wrote:
> On 27/05/2019 11:05, Jacopo Mondi wrote:
> > On Mon, May 27, 2019 at 02:15:31AM +0200, Niklas Söderlund wrote:
> >> Add two new objects to hold v4l2 format information for v4l2 devices and
> >> subdevices. There is much code which can be shared between the two so
> >> have both of them inherit from a base class to reduce code duplication.
> > 
> > I still think we should have gone this way from the very beginning, so
> > I think this is the direction to go.
> > 
> > One question:
> > 1) looking at the patch, it seems to me now V4L2DeviceFormats and
> > V4L2SubdeviceFormats only differns in the name of the formats map key
> > (pixelcode and mbus code). I wonder if we shouldn't better unify those
> > (maybe renaming DeviceFormats in just ImageFormats).
> 
> Also, I'm a bit weary that we will now have V4L2DeviceFormat and
> V4L2DeviceFormats which are quite different
> 
>  (xxxxFormats is not a direct array of xxxFormat)
> 
> Same for both V4L2DeviceFormats and V4L2SubdeviceFormats.
> 
> Do you think this is a problem? Or will this be ok?
> 
> Or perhaps it's a moot point, as I think Jacopo's comments look like
> suggesting unifying this some how so the naming will be less ambiguous
> then anyway.

I also agree that, given how similar the two classes are, unifying them
would be a good idea.

> >> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> >> ---
> >>  src/libcamera/formats.cpp       | 158 ++++++++++++++++++++++++++++++++
> >>  src/libcamera/include/formats.h |  35 +++++++
> >>  2 files changed, 193 insertions(+)
> >>
> >> diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp
> >> index 56f4ddb51ffad4d3..f5841b38cea5686c 100644
> >> --- a/src/libcamera/formats.cpp
> >> +++ b/src/libcamera/formats.cpp
> >> @@ -24,4 +24,162 @@ namespace libcamera {
> >>   * resolutions represented by SizeRange items.
> >>   */
> >>
> >> +

Extra blank line ?

> >> +/**
> >> + * \class DeviceFormats
> >> + * \brief Base class for V4L2Device and V4L2SubDevice Formats
> >> + *
> >> + * The base class holds common functionallity between V4L2DeviceFormats and
> > 
> > s/functionallity/functionalities/
> > 
> >> + * V4L2SubDeviceForamts.
> > 
> > s/Foramats/Formats/ here an in other places
> >> + *
> >> + * \sa V4L2DeviceFormats
> >> + * \sa V4L2SubDeviceForamts
> >> + */
> >> +
> >> +/**
> >> + * \brief Create an empty DeviceFormats
> >> + */
> >> +DeviceFormats::DeviceFormats()
> >> +{
> >> +}
> >> +
> >> +/**
> >> + * \brief Create a DeviceFormats with data
> > 
> > with data/using the formats and size provided by \a data
> > 
> > missing the data argument documentation.
> > 
> > Documentation apart, we're now creating a DeviceFormats from an
> > already built map, which require users of this class to build the map
> > by themselves
> > 
> > Have you considered the possibility of creating an empty DeviceFormats
> > and later populated it with:
> > 
> >         addSizes(unsigned int fmt, std::vector<SizeRange> &sizes)
> > 
> > Otherwise the interface to populate the formats stays as awkward to
> > use as it is today.

I have yet to review the patches that make use of this class, but I
agree that this constructor is a bit awkward to use.

> >> + */
> >> +DeviceFormats::DeviceFormats(const std::map<unsigned int, std::vector<SizeRange>> &data)
> >> +	: data_(data)
> > 
> > s/data/map ?
> > 
> >> +{
> >> +}
> >> +
> >> +DeviceFormats::~DeviceFormats()
> >> +{
> >> +}
> >> +
> >> +/**
> >> + * \brief Retrieve all sizes for a specific format
> >> + * \param[in] format A pixelformat or mbus code
> >> + *
> >> + * Retrieves all SizeRanges for a specific format. For V4L2Device \a format is a
> > 
> > We usually use "Retrieve"
> > 
> >> + * pixelformoat while for a V4L2Subdevice \a format is a media bus code.
> > 
> > pixelformoat/pixel format code/
> > 
> > As the difference is the name of the field only, I would rather unify
> > the two derived classes.
> > 
> >> + *
> >> + * \return List of SizeRanges for \a format, empty list if format is not valid
> > 
> > The list of image sizes produced using \a format, or an empty list if format is not supported.
> > 
> >> + */
> >> +std::vector<SizeRange> DeviceFormats::sizes(unsigned int format) const
> >> +{
> >> +	auto const &it = data_.find(format);
> >> +	if (it == data_.end())
> >> +		return {};
> >> +
> >> +	return it->second;
> >> +}
> >> +
> >> +/**
> >> + * \brief Check if the device formats is empty
> > 
> > Check if the list of devices supported formats is empty
> > 
> >> + * \return True if the formats are empty
> > 
> > "True if the list of supported formats is empty"
> > 
> >> + */
> >> +bool DeviceFormats::empty() const
> >> +{
> >> +	return data_.empty();
> >> +}
> >> +
> >> +/**
> >> + * \brief Retrieve the raw dataA
> > 
> > s/dataA/data
> > and I would
> > s/data/the map that associates formats to image sizes/
> > 
> > I would rather not let users access the whole map. Do you have an use
> > case for this?

Why would you prefer not exposing it ?

An alternative could also be to expose custom iterators on the
DeviceFormats class.

> >> + * \return Raw map containgin formats to SizeRanges
> > 
> > The map that associates formats to image sizes
> > 
> >> + */
> >> +const std::map<unsigned int, std::vector<SizeRange>> &DeviceFormats::data()
> >> +{
> >> +	return data_;
> >> +}
> >> +
> >> +/**
> >> + * \brief Retrieve a list all contained formats
> > 
> > Retrieve a list of all supported image formats
> > 
> >> + *
> >> + * This is a helper function intended to be used by V4L2DeviceFormats and
> >> + * V4L2SubdeviceFormats.
> >> + *
> >> + * \return A list of formats contained
> > 
> > "of supported formats"
> > 
> >> + */
> >> +std::vector<unsigned int> DeviceFormats::formats() const
> >> +{
> >> +	std::vector<unsigned int> formats;

You should call formats.reserve(data_.size()); to optimise memory
allocation.

> >> +
> >> +	for (auto const &it : data_)
> >> +		formats.push_back(it.first);
> >> +
> >> +	return formats;
> > 
> > I'm very disappointed std::map does not have a keys() nor a values()
> > method.

Another option is

	std::transform(data_.begin(), data_.end(), std::back_inserter(formats),
		       [](decltype(data_)::value_type const &pair) {
			       return pair.first;
		       });

I'm not sure which one is more efficient.

> >> +}
> >> +
> >> +/**
> >> + * \var DeviceFormats::data_
> >> + * \brief The map holding format and SizeRange information
> > 
> > Associating image formats to sizes.
> > 
> >> + */
> >> +
> >> +/**
> >> + * \class V4L2SubdeviceFormats
> >> + * \brief Holds media bus codes to frame sizes information for a v4l2 subdevice
> >> + *
> >> + * Hold media bus codes and frame sizes which describes a v4l2 subdevice. The
> >> + * intended user of this object is pipeline handlers which can create it
> >> + * from a V4L2Subdevice and use it to describe and validate formats.
> >> + */
> >> +
> >> +/**
> >> + * \brief Create an empty V4L2SubdeviceFormats
> >> + */
> >> +V4L2SubdeviceFormats::V4L2SubdeviceFormats()
> >> +	: DeviceFormats()
> >> +{
> >> +}
> > 
> > How do you fill in an empty V4L2SubdeviceFormats ?
> > 
> >> +
> >> +/**
> >> + * \brief Create an V4L2SubdeviceFormats with data
> >> + */
> >> +V4L2SubdeviceFormats::V4L2SubdeviceFormats(const std::map<unsigned int, std::vector<SizeRange>> &data)
> >> +	: DeviceFormats(data)
> >> +{
> >> +}
> >> +
> >> +/**
> >> + * \brief Retrieve media bus codes which are described
> > which are described ... ?
> > 
> > I would just "Retrieve the list of supported media bus codes"
> > 
> >> + * \return List of media bus codes
> >> + */
> >> +std::vector<unsigned int> V4L2SubdeviceFormats::codes() const
> >> +{
> >> +	return formats();
> >> +}
> >> +
> >> +/**
> >> + * \class V4L2DeviceFormats
> >> + * \brief Holds pixelformats to frame sizes information for a v4l2 device
> >> + *
> >> + * Hold pixelformats and frame sizes which describes a v4l2 device. The
> >> + * intended user of this object is pipeline handlers which can create it
> >> + * from a V4L2Device and use it to describe and validate formats.
> >> + */
> >> +
> >> +/**
> >> + * \brief Create an empty V4L2DeviceFormats
> >> + */
> >> +V4L2DeviceFormats::V4L2DeviceFormats()
> >> +	: DeviceFormats()
> >> +{
> >> +}
> > 
> > Same question
> > 
> >> +
> >> +/**
> >> + * \brief Create an V4L2DeviceFormats with data
> >> + */
> >> +V4L2DeviceFormats::V4L2DeviceFormats(const std::map<unsigned int, std::vector<SizeRange>> &data)
> >> +	: DeviceFormats(data)
> >> +{
> >> +}
> >> +
> >> +/**
> >> + * \brief Retrieve pixelformats which are described
> >> + * \return List of pixelformats
> >> + */
> >> +std::vector<unsigned int> V4L2DeviceFormats::pixelformats() const
> >> +{
> >> +	return formats();
> >> +}
> > 
> > And same comment again: do we now need V4L2Device and V4L2Subdevice
> > formats? I think it's easier for pipeline handler to deal with formats
> > and sizes through a single interface, even if we loose the
> > pixelcode/media bus code/ distinction, which is by the way only
> > specified at documentation level.
> > 
> >> +
> >>  } /* namespace libcamera */
> >> diff --git a/src/libcamera/include/formats.h b/src/libcamera/include/formats.h
> >> index a73772b1eda068b4..372f6e6d71b236dd 100644
> >> --- a/src/libcamera/include/formats.h
> >> +++ b/src/libcamera/include/formats.h
> >> @@ -17,6 +17,41 @@ namespace libcamera {
> >>
> >>  typedef std::map<unsigned int, std::vector<SizeRange>> FormatEnum;
> >>
> >> +class DeviceFormats
> >> +{
> >> +public:
> >> +	virtual ~DeviceFormats();
> >> +
> >> +	std::vector<SizeRange> sizes(unsigned int format) const;

Should this return a const reference instead of a copy ?

> >> +	bool empty() const;

isEmpty() ?

> >> +	const std::map<unsigned int, std::vector<SizeRange>> &data();

I think you can inline this function.

> >> +
> >> +protected:
> >> +	DeviceFormats();
> >> +	DeviceFormats(const std::map<unsigned int, std::vector<SizeRange>> &data);
> >> +	std::vector<unsigned int> formats() const;
> >> +
> >> +	std::map<unsigned int, std::vector<SizeRange>> data_;
> >> +};
> >> +
> >> +class V4L2SubdeviceFormats : public DeviceFormats
> >> +{
> >> +public:
> >> +	V4L2SubdeviceFormats();
> >> +	V4L2SubdeviceFormats(const std::map<unsigned int, std::vector<SizeRange>> &data);
> >> +
> >> +	std::vector<unsigned int> codes() const;
> >> +};
> >> +
> >> +class V4L2DeviceFormats : public DeviceFormats
> >> +{
> >> +public:
> >> +	V4L2DeviceFormats();
> >> +	V4L2DeviceFormats(const std::map<unsigned int, std::vector<SizeRange>> &data);
> >> +
> >> +	std::vector<unsigned int> pixelformats() const;
> >> +};
> >> +
> >>  } /* namespace libcamera */
> >>
> >>  #endif /* __LIBCAMERA_FORMATS_H__ */