[libcamera-devel,v2,05/16] libcamera: formats: Add ImageFormats

1401 diff mbox series

Add a new class to hold format information for v4l2 devices and
subdevices. The object describes the relationship between either pixel
formats (v4l2 devices) or media bus codes (v4l2 subdevice) and a list of
image sizes which can be produced with that format.

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

Hi Niklas,

On Wed, Jun 12, 2019 at 02:43:48AM +0200, Niklas Söderlund wrote:
> Add a new class to hold format information for v4l2 devices and
> subdevices. The object describes the relationship between either pixel
> formats (v4l2 devices) or media bus codes (v4l2 subdevice) and a list of
> image sizes which can be produced with that format.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> ---
>  src/libcamera/formats.cpp       | 83 +++++++++++++++++++++++++++++++++
>  src/libcamera/include/formats.h | 17 +++++++
>  2 files changed, 100 insertions(+)
>
> diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp
> index 56f4ddb51ffad4d3..ab4bae0a5928d9ba 100644
> --- a/src/libcamera/formats.cpp
> +++ b/src/libcamera/formats.cpp
> @@ -24,4 +24,87 @@ namespace libcamera {
>   * resolutions represented by SizeRange items.
>   */
>
> +/**
> + * \class ImageFormats

I'm a bit undecided here... This represents a V4L2 device or subdevice
format, and I would keep the "ImageFormats" name for the application
facing image format, which might end up being different than the V4L2
defined one.

Now that we have a V4L2Device I think V4L2Format might be considered
as an alternative name..

> + * \brief Describe V4L2Device and V4L2SubDevice image formats
> + *
> + * The class describes information about image formats and supported sizes. If
> + * the ImageFormat describe a V4L2Device the formats described are pixel formats
> + * while if it describes a V4L2SubDevice the formats are media bus codes.

I would:
 The class describes information about image formats and supported sizes. If
 the ImageFormat describe a V4L2Device the image formats are described
 with a fourcc pixel format code, while if it describes a V4L2SubDevice
 the formats are described with media bus codes, both defined by the
 V4L2 specification.

Is fine if you keep yours...

> + */
> +
> +ImageFormats::ImageFormats()
> +{
> +}

Do you need this? The compiler generates a default contructor for
you..

> +
> +/**
> + * \brief Add a format and sizes to the description
> + * \param[in] format Pixel format or media bus code to describe
> + * \param[in] sizes List of supported sizes for the format
> + *
> + * \return 0 on success or a negative error code otherwise
> + * \retval -EBUSY The format is already described
> + */
> +int ImageFormats::addFormat(unsigned int format, const std::vector<SizeRange> &sizes)
> +{
> +	if (data_.find(format) != data_.end())
> +		return -EBUSY;
> +
> +	data_[format] = sizes;
> +
> +	return 0;
> +}
> +
> +/**
> + * \brief Retrieve a list of all supported image formats
> + * \return List of pixel formats or media bus codes
> + */
> +std::vector<unsigned int> ImageFormats::formats() const
> +{
> +	std::vector<unsigned int> formats;
> +	formats.reserve(data_.size());
> +
> +	for (auto const &it : data_)
> +		formats.push_back(it.first);
> +
> +	return formats;
> +}
> +
> +/**
> + * \brief Retrieve all sizes for a specific format
> + * \param[in] format A pixelformat or mbus code
> + *
> + * Retrieve all SizeRanges for a specific format. For V4L2Device \a format is a
> + * pixel format while for a V4L2Subdevice \a format is a media bus code.
> + *
> + * \return he list of image sizes produced using \a format, or an empty list if
> + * format is not supported
> + */
> +std::vector<SizeRange> ImageFormats::sizes(unsigned int format) const
> +{
> +	auto const &it = data_.find(format);
> +	if (it == data_.end())
> +		return {};
> +
> +	return it->second;
> +}
> +
> +/**
> + * \brief Check if the list of devices supported formats is empty
> + * \return True if the list of supported formats is empty
> + */
> +bool ImageFormats::isEmpty() const
> +{
> +	return data_.empty();
> +}
> +
> +/**
> + * \brief Retrieve the map that associates formats to image sizes
> + * \return Map that associates formats to image sizes
> + */
> +const std::map<unsigned int, std::vector<SizeRange>> &ImageFormats::data() const
> +{
> +	return data_;
> +}
> +
>  } /* namespace libcamera */
> diff --git a/src/libcamera/include/formats.h b/src/libcamera/include/formats.h
> index a73772b1eda068b4..a31aa42bcb4742ac 100644
> --- a/src/libcamera/include/formats.h
> +++ b/src/libcamera/include/formats.h
> @@ -17,6 +17,23 @@ namespace libcamera {
>
>  typedef std::map<unsigned int, std::vector<SizeRange>> FormatEnum;

Do we want to use this in this class?

>
> +class ImageFormats
> +{
> +public:
> +	ImageFormats();
> +
> +	int addFormat(unsigned int format, const std::vector<SizeRange> &sizes);
> +
> +	std::vector<unsigned int> formats() const;
> +	std::vector<SizeRange> sizes(unsigned int format) const;

I would return a const & here

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

and group the three data accessors together. Maybe you can move
isEmpty() up as the first method?

Thanks
   j

> +
> +private:
> +	std::map<unsigned int, std::vector<SizeRange>> data_;
> +};
> +
>  } /* 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 Jacopo,

Thanks for your feedback.

On 2019-06-13 17:15:33 +0200, Jacopo Mondi wrote:
> Hi Niklas,
> 
> On Wed, Jun 12, 2019 at 02:43:48AM +0200, Niklas Söderlund wrote:
> > Add a new class to hold format information for v4l2 devices and
> > subdevices. The object describes the relationship between either pixel
> > formats (v4l2 devices) or media bus codes (v4l2 subdevice) and a list of
> > image sizes which can be produced with that format.
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > ---
> >  src/libcamera/formats.cpp       | 83 +++++++++++++++++++++++++++++++++
> >  src/libcamera/include/formats.h | 17 +++++++
> >  2 files changed, 100 insertions(+)
> >
> > diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp
> > index 56f4ddb51ffad4d3..ab4bae0a5928d9ba 100644
> > --- a/src/libcamera/formats.cpp
> > +++ b/src/libcamera/formats.cpp
> > @@ -24,4 +24,87 @@ namespace libcamera {
> >   * resolutions represented by SizeRange items.
> >   */
> >
> > +/**
> > + * \class ImageFormats
> 
> I'm a bit undecided here... This represents a V4L2 device or subdevice
> format, and I would keep the "ImageFormats" name for the application
> facing image format, which might end up being different than the V4L2
> defined one.

I'm open to renaming it, I'm bad at naming so I picked ImageFormats as 
you suggested it in your last review ;-P I wait for what others think 
and do the popular thing.

> 
> Now that we have a V4L2Device I think V4L2Format might be considered
> as an alternative name..
> 
> > + * \brief Describe V4L2Device and V4L2SubDevice image formats
> > + *
> > + * The class describes information about image formats and supported sizes. If
> > + * the ImageFormat describe a V4L2Device the formats described are pixel formats
> > + * while if it describes a V4L2SubDevice the formats are media bus codes.
> 
> I would:
>  The class describes information about image formats and supported sizes. If
>  the ImageFormat describe a V4L2Device the image formats are described
>  with a fourcc pixel format code, while if it describes a V4L2SubDevice
>  the formats are described with media bus codes, both defined by the
>  V4L2 specification.
> 
> Is fine if you keep yours...
> 
> > + */
> > +
> > +ImageFormats::ImageFormats()
> > +{
> > +}
> 
> Do you need this? The compiler generates a default contructor for
> you..
> 
> > +
> > +/**
> > + * \brief Add a format and sizes to the description
> > + * \param[in] format Pixel format or media bus code to describe
> > + * \param[in] sizes List of supported sizes for the format
> > + *
> > + * \return 0 on success or a negative error code otherwise
> > + * \retval -EBUSY The format is already described
> > + */
> > +int ImageFormats::addFormat(unsigned int format, const std::vector<SizeRange> &sizes)
> > +{
> > +	if (data_.find(format) != data_.end())
> > +		return -EBUSY;
> > +
> > +	data_[format] = sizes;
> > +
> > +	return 0;
> > +}
> > +
> > +/**
> > + * \brief Retrieve a list of all supported image formats
> > + * \return List of pixel formats or media bus codes
> > + */
> > +std::vector<unsigned int> ImageFormats::formats() const
> > +{
> > +	std::vector<unsigned int> formats;
> > +	formats.reserve(data_.size());
> > +
> > +	for (auto const &it : data_)
> > +		formats.push_back(it.first);
> > +
> > +	return formats;
> > +}
> > +
> > +/**
> > + * \brief Retrieve all sizes for a specific format
> > + * \param[in] format A pixelformat or mbus code
> > + *
> > + * Retrieve all SizeRanges for a specific format. For V4L2Device \a format is a
> > + * pixel format while for a V4L2Subdevice \a format is a media bus code.
> > + *
> > + * \return he list of image sizes produced using \a format, or an empty list if
> > + * format is not supported
> > + */
> > +std::vector<SizeRange> ImageFormats::sizes(unsigned int format) const
> > +{
> > +	auto const &it = data_.find(format);
> > +	if (it == data_.end())
> > +		return {};
> > +
> > +	return it->second;
> > +}
> > +
> > +/**
> > + * \brief Check if the list of devices supported formats is empty
> > + * \return True if the list of supported formats is empty
> > + */
> > +bool ImageFormats::isEmpty() const
> > +{
> > +	return data_.empty();
> > +}
> > +
> > +/**
> > + * \brief Retrieve the map that associates formats to image sizes
> > + * \return Map that associates formats to image sizes
> > + */
> > +const std::map<unsigned int, std::vector<SizeRange>> &ImageFormats::data() const
> > +{
> > +	return data_;
> > +}
> > +
> >  } /* namespace libcamera */
> > diff --git a/src/libcamera/include/formats.h b/src/libcamera/include/formats.h
> > index a73772b1eda068b4..a31aa42bcb4742ac 100644
> > --- a/src/libcamera/include/formats.h
> > +++ b/src/libcamera/include/formats.h
> > @@ -17,6 +17,23 @@ namespace libcamera {
> >
> >  typedef std::map<unsigned int, std::vector<SizeRange>> FormatEnum;
> 
> Do we want to use this in this class?

I don't want to use it and it's removed in a later patch in this series.

> 
> >
> > +class ImageFormats
> > +{
> > +public:
> > +	ImageFormats();
> > +
> > +	int addFormat(unsigned int format, const std::vector<SizeRange> &sizes);
> > +
> > +	std::vector<unsigned int> formats() const;
> > +	std::vector<SizeRange> sizes(unsigned int format) const;
> 
> I would return a const & here

I would to if it where possible, as we need to generate the vector from 
the map we can't return a reference. Laurent suggested to compute and 
cache the vectors which would allow this. I'm not against doing that but 
it could be done at a later time as we optimize.

> 
> > +
> > +	bool isEmpty() const;
> > +	const std::map<unsigned int, std::vector<SizeRange>> &data() const;
> 
> and group the three data accessors together. Maybe you can move
> isEmpty() up as the first method?

Sure, I have no preference.

> 
> Thanks
>    j
> 
> > +
> > +private:
> > +	std::map<unsigned int, std::vector<SizeRange>> data_;
> > +};
> > +
> >  } /* 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