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

[Laurent Pinchart]

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 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.
> >>   */
> >>
> >> +

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__ */

-- 
Regards,

Laurent Pinchart