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

[Jacopo Mondi]

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 at 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 at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
-------------- 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/20190527/5a76fac1/attachment.sig>