[libcamera-devel] [PATCH v3 05/16] libcamera: formats: Add ImageFormats
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Jun 19 00:48:31 CEST 2019
Hi Niklas,
Thank you for the patch.
On Sun, Jun 16, 2019 at 03:33:51PM +0200, Niklas Söderlund wrote:
> Add a new class to hold format information for v4l2 devices and
s/v4l2/V4L2/ (and possibly "V4L2 video devices and subdevices" depending
on which series gets merged first)
> subdevices. The object describes the relationship between either pixel
> formats (v4l2 devices) or media bus codes (v4l2 subdevice) and a list of
V4L2 there too.
> image sizes which can be produced with that format.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
> src/libcamera/formats.cpp | 80 +++++++++++++++++++++++++++++++++
> src/libcamera/include/formats.h | 14 ++++++
> 2 files changed, 94 insertions(+)
>
> diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp
> index 56f4ddb51ffad4d3..2fd0c5480324ce33 100644
> --- a/src/libcamera/formats.cpp
> +++ b/src/libcamera/formats.cpp
> @@ -24,4 +24,84 @@ namespace libcamera {
> * resolutions represented by SizeRange items.
> */
>
> +/**
> + * \class ImageFormats
> + * \brief Describe V4L2Device and V4L2SubDevice image formats
> + *
> + * 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.
I would propose
"This class stores a list of image formats, each associated with a
corresponding set of image sizes. It is used to describe the formats and
sizes supported by a V4L2Device or V4L2Subdevice.
Formats are stored as an integer. When used for a V4L2Device, the image
formats are fourcc pixel formats. When used for a V4L2Subdevice they are
media bus codes. Both are defined by the V4L2 specification.
Sizes are stored as a list of SizeRange."
> + */
> +
> +/**
> + * \brief Add a format and sizes to the description
s/and sizes/and corresponding sizes/
> + * \param[in] format Pixel format or media bus code to describe
> + * \param[in] sizes List of supported sizes for the format
s/sizes/size ranges/ ?
> + *
> + * \return 0 on success or a negative error code otherwise
> + * \retval -EBUSY The format is already described
I wonder if we really need this, or if we should ignore the error
silently. If you think it should be kept, I would use -EEXIST.
> + */
> +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 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 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;
Do you think it would make sense to cache it internally and return a
const reference, or is it not worth it given the existing and foreseen
use cases ? If you think it could be useful you don't have to fix it
now, but a \todo would be useful.
> +}
> +
> +/**
> + * \brief Retrieve all sizes for a specific format
> + * \param[in] format A pixelformat or mbus code
s/A/The/
> + *
> + * Retrieve all SizeRanges for a specific format. For V4L2Device \a format is a
s/SizeRanges/size ranges/ or SizeRange instances (I would go for the
format). The return value type in doxygen will offer a clickable link to
SizeRange, so there's no strict need to mention SizeRange in the text
here.
> + * 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
s/he/The/
s/produced using/supported for/ ?
> + * format is not supported
s/format/the format/
> + */
> +std::vector<SizeRange> ImageFormats::sizes(unsigned int format) const
> +{
> + auto const &it = data_.find(format);
> + if (it == data_.end())
> + return {};
> +
> + return it->second;
In this case I think a copy is quite overkill. You could declare
static std::vector<SizeRange> empty;
replace return {} with return empty, and change the function type to
return a const reference.
> +}
> +
> +/**
> + * \brief Retrieve the map that associates formats to image sizes
> + * \return Map that associates formats to image sizes
s/Map/The map/
With these small issues addressed,
Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> + */
> +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..a49f83f3d8d60621 100644
> --- a/src/libcamera/include/formats.h
> +++ b/src/libcamera/include/formats.h
> @@ -17,6 +17,20 @@ namespace libcamera {
>
> typedef std::map<unsigned int, std::vector<SizeRange>> FormatEnum;
>
> +class ImageFormats
> +{
> +public:
> + int addFormat(unsigned int format, const std::vector<SizeRange> &sizes);
> +
> + bool isEmpty() const;
> + std::vector<unsigned int> formats() const;
> + std::vector<SizeRange> sizes(unsigned int format) const;
> + const std::map<unsigned int, std::vector<SizeRange>> &data() const;
> +
> +private:
> + std::map<unsigned int, std::vector<SizeRange>> data_;
> +};
> +
> } /* namespace libcamera */
>
> #endif /* __LIBCAMERA_FORMATS_H__ */
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list