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

[Kieran Bingham]

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 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
>>
>> _______________________________________________
>> libcamera-devel mailing list
>> libcamera-devel at lists.libcamera.org
>> https://lists.libcamera.org/listinfo/libcamera-devel

-- 
Regards
--
Kieran