[libcamera-devel] [PATCH 1/5] libcamera: formats: Make ImageFormats a templated class
Niklas Söderlund
niklas.soderlund at ragnatech.se
Fri Jun 5 21:36:43 CEST 2020
Hi Jacopo,
Thanks for your work.
On 2020-05-29 13:03:31 +0200, Jacopo Mondi wrote:
> The ImageFormats class was originally designed to be used by both
> V4L2VideoDevices and V4L2Subdevices. As video devices enumerates their
> image formats using V4L2PixelFormat instances, the ImageFormats class
> cannot be used there anymore and it has been replaced by raw maps.
>
> Prepare to re-introduce usage of ImageFormats in the V4L2VideoDevice
> class and its users by making ImageFormats a templated class that
> indexes the image sizes using on keys of variadic type.
This is similar to '[RFC 2/6] libcamera: formats: Turn ImageFormats into
a template' which was dropped in favor of trying to refactor away
ImageFormats going forward.
I'm not against backtracking on that decision as it's been a few months
since then. But I think we all need to agree what option is the best way
forward.
>
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
> include/libcamera/internal/camera_sensor.h | 2 +-
> include/libcamera/internal/formats.h | 64 ++++++++++-
> include/libcamera/internal/v4l2_subdevice.h | 2 +-
> src/libcamera/formats.cpp | 117 +++++++++++++-------
> src/libcamera/v4l2_subdevice.cpp | 6 +-
> test/v4l2_subdevice/list_formats.cpp | 2 +-
> 6 files changed, 138 insertions(+), 55 deletions(-)
>
> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
> index d79bd9ce9d58..5c1d5789fe79 100644
> --- a/include/libcamera/internal/camera_sensor.h
> +++ b/include/libcamera/internal/camera_sensor.h
> @@ -75,7 +75,7 @@ private:
>
> std::string model_;
>
> - ImageFormats formats_;
> + ImageFormats<uint32_t> formats_;
> Size resolution_;
> std::vector<unsigned int> mbusCodes_;
> std::vector<Size> sizes_;
> diff --git a/include/libcamera/internal/formats.h b/include/libcamera/internal/formats.h
> index 4092a93ef973..e20c031e857f 100644
> --- a/include/libcamera/internal/formats.h
> +++ b/include/libcamera/internal/formats.h
> @@ -18,18 +18,70 @@
>
> namespace libcamera {
>
> +template<typename T>
> class ImageFormats
> {
> public:
> - int addFormat(unsigned int format, const std::vector<SizeRange> &sizes);
> + using iterator = typename std::map<T, std::vector<SizeRange>>::iterator;
> + using const_iterator = typename std::map<T, std::vector<SizeRange>>::const_iterator;
> + using value_type = typename std::map<T, std::vector<SizeRange>>::value_type;
>
> - bool isEmpty() const;
> - std::vector<unsigned int> formats() const;
> - const std::vector<SizeRange> &sizes(unsigned int format) const;
> - const std::map<unsigned int, std::vector<SizeRange>> &data() const;
> + iterator begin() { return data_.begin(); }
> + const_iterator begin() const { return data_.begin(); }
> + iterator end() { return data_.end(); }
> + const_iterator end() const { return data_.end(); }
> +
> + iterator find(const T format) { return data_.find(format); }
> + const iterator find(T format) const { return data_.find(format); }
> +
> + template<class... Args>
> + std::pair<iterator, bool> emplace(Args&&... args)
> + {
> + return data_.emplace(args...);
> + }
> +
> + int addFormat(T format, const std::vector<SizeRange> &sizes)
> + {
> + if (data_.find(format) != data_.end())
> + return -EEXIST;
> +
> + data_[format] = sizes;
> +
> + return 0;
> + }
> +
> + bool isEmpty() const { return data_.empty(); }
> +
> + std::vector<T> formats() const
> + {
> + std::vector<T> formats;
> + formats.reserve(data_.size());
> +
> + /* \todo: Should this be cached instead of computed each time? */
> + for (auto const &it : data_)
> + formats.push_back(it.first);
> +
> + return formats;
> + }
> +
> + const std::vector<SizeRange> &sizes(T format) const
> + {
> + static const std::vector<SizeRange> empty;
> +
> + auto const &it = data_.find(format);
> + if (it == data_.end())
> + return empty;
> +
> + return it->second;
> + }
> +
> + const std::map<T, std::vector<SizeRange>> &data() const
> + {
> + return data_;
> + }
>
> private:
> - std::map<unsigned int, std::vector<SizeRange>> data_;
> + std::map<T, std::vector<SizeRange>> data_;
> };
>
> class PixelFormatInfo
> diff --git a/include/libcamera/internal/v4l2_subdevice.h b/include/libcamera/internal/v4l2_subdevice.h
> index 1be454f0ddda..0ce6da48f58a 100644
> --- a/include/libcamera/internal/v4l2_subdevice.h
> +++ b/include/libcamera/internal/v4l2_subdevice.h
> @@ -51,7 +51,7 @@ public:
> int setSelection(unsigned int pad, unsigned int target,
> Rectangle *rect);
>
> - ImageFormats formats(unsigned int pad);
> + ImageFormats<uint32_t> formats(unsigned int pad);
>
> int getFormat(unsigned int pad, V4L2SubdeviceFormat *format,
> Whence whence = ActiveFormat);
> diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp
> index 2ac3b412ecdb..a7922077d9c5 100644
> --- a/src/libcamera/formats.cpp
> +++ b/src/libcamera/formats.cpp
> @@ -22,20 +22,85 @@ LOG_DEFINE_CATEGORY(Formats)
>
> /**
> * \class ImageFormats
> - * \brief Describe V4L2Device and V4L2SubDevice image formats
> + * \brief Describe V4L2Device and V4L2SubDevice image formats and associated
> + * image resolutions
> *
> - * This class stores a list of image formats, each associated with a
> + * This class stores a list of image formats, each of them 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.
> + * When used for a V4L2Device, the image formats are V4L2PixelFormat instances.
> + * When used for a V4L2Subdevice formats are integer media bus codes.
> *
> * Sizes are stored as a list of SizeRange.
> */
>
> /**
> + * \typedef ImageFormats::iterator
> + * \brief Iterator for the formats map
> + */
> +
> +/**
> + * \typedef ImageFormats::const_iterator
> + * \brief Const iterator for the formats map
> + */
> +
> +/**
> + * \typedef ImageFormats::value_type
> + * \brief Value type of the entries in the formats map
> + */
> +
> +/**
> + * \fn iterator ImageFormats<T>::begin()
> + * \brief Retrieve an iterator to the first element in the formats map
> + * \return An iterator to the first format map
> + */
> +
> +/**
> + * \fn const_iterator ImageFormats<T>::begin() const
> + * \brief Retrieve an const iterator to the first element in the formats map
> + * \return A const iterator to the first format map
> + */
> +
> +/**
> + * \fn iterator ImageFormats<T>::end()
> + * \brief Retrieve an iterator pointing to the past-the-end element in the
> + * formats map
> + * \return An iterator to the element following the last format
> + */
> +
> +/**
> + * \fn const_iterator ImageFormats<T>::end() const
> + * \brief Retrieve a const iterator pointing to the past-the-end element in the
> + * formats map
> + * \return A const iterator to the element following the last format
> + */
> +
> +/**
> + * \fn iterator ImageFormats::find(const T format)
> + * \brief Find an element with key equal to \a format
> + * \param[in] format The format to search for
> + * \return An iterator to the vector of sizes associated with \a format
> + */
> +
> +/**
> + * \fn const_iterator ImageFormats::find(const T format) const
> + * \brief Find a const element with key equal to \a format
> + * \param[in] format The format to search for
> + * \return An const iterator to the vector of sizes associated with \a format
> + */
> +
> +/**
> + * \fn std::pair<iterator, bool> ImageFormats::emplace(Args&&... args)
> + * \brief Insert a new element in the formats map constructed in place with the
> + * given \a args
> + * \param[in] args The argument pack used to construct the new entry in place
> + * \return A pair consisting of an iterator to the inserted element, and a bool
> + * denoting whether the insertion took place
> + */
> +
> +/**
> + * \fn ImageFormats<T>::addFormat(T format, const std::vector<SizeRange> &sizes)
> * \brief Add a format and corresponding sizes to the description
> * \param[in] format Pixel format or media bus code to describe
> * \param[in] sizes List of supported size ranges for the format
> @@ -43,42 +108,21 @@ LOG_DEFINE_CATEGORY(Formats)
> * \return 0 on success or a negative error code otherwise
> * \retval -EEXIST The format is already described
> */
> -int ImageFormats::addFormat(unsigned int format, const std::vector<SizeRange> &sizes)
> -{
> - if (data_.find(format) != data_.end())
> - return -EEXIST;
> -
> - data_[format] = sizes;
> -
> - return 0;
> -}
>
> /**
> + * \fn ImageFormats<T>::isEmpty() const
> * \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();
> -}
>
> /**
> + * \fn ImageFormats<T>::formats() const
> * \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());
> -
> - /* \todo: Should this be cached instead of computed each time? */
> - for (auto const &it : data_)
> - formats.push_back(it.first);
> -
> - return formats;
> -}
>
> /**
> + * \fn ImageFormats<T>::sizes(T format) const
> * \brief Retrieve all sizes for a specific format
> * \param[in] format The pixel format or mbus code
> *
> @@ -88,25 +132,12 @@ std::vector<unsigned int> ImageFormats::formats() const
> * \return The list of image sizes supported for \a format, or an empty list if
> * the format is not supported
> */
> -const std::vector<SizeRange> &ImageFormats::sizes(unsigned int format) const
> -{
> - static const std::vector<SizeRange> empty;
> -
> - auto const &it = data_.find(format);
> - if (it == data_.end())
> - return empty;
> -
> - return it->second;
> -}
>
> /**
> + * \fn ImageFormats<T>::data() const
> * \brief Retrieve the map that associates formats to image sizes
> * \return The map that associates formats to image sizes
> */
> -const std::map<unsigned int, std::vector<SizeRange>> &ImageFormats::data() const
> -{
> - return data_;
> -}
>
> /**
> * \class PixelFormatInfo
> diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp
> index 7aefc1be032d..9fa20e84a904 100644
> --- a/src/libcamera/v4l2_subdevice.cpp
> +++ b/src/libcamera/v4l2_subdevice.cpp
> @@ -320,16 +320,16 @@ int V4L2Subdevice::setSelection(unsigned int pad, unsigned int target,
> *
> * \return A list of the supported device formats
> */
> -ImageFormats V4L2Subdevice::formats(unsigned int pad)
> +ImageFormats<uint32_t> V4L2Subdevice::formats(unsigned int pad)
> {
> - ImageFormats formats;
> + ImageFormats<uint32_t> formats;
>
> if (pad >= entity_->pads().size()) {
> LOG(V4L2, Error) << "Invalid pad: " << pad;
> return {};
> }
>
> - for (unsigned int code : enumPadCodes(pad)) {
> + for (uint32_t code : enumPadCodes(pad)) {
> std::vector<SizeRange> sizes = enumPadSizes(pad, code);
> if (sizes.empty())
> return {};
> diff --git a/test/v4l2_subdevice/list_formats.cpp b/test/v4l2_subdevice/list_formats.cpp
> index 25503c3334e5..f66bb633fb00 100644
> --- a/test/v4l2_subdevice/list_formats.cpp
> +++ b/test/v4l2_subdevice/list_formats.cpp
> @@ -48,7 +48,7 @@ void ListFormatsTest::printFormats(unsigned int pad,
> int ListFormatsTest::run()
> {
> /* List all formats available on existing "Scaler" pads. */
> - ImageFormats formats;
> + ImageFormats<uint32_t> formats;
>
> formats = scaler_->formats(0);
> if (formats.isEmpty()) {
> --
> 2.26.2
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list