| Message ID | 20200529110335.620503-2-jacopo@jmondi.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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@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@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Jacopo and Niklas, On Fri, Jun 05, 2020 at 09:36:43PM +0200, Niklas Söderlund wrote: > 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. I agree with you. I still believe we'll be able to find a better abstraction in the long term. If there's value in refactoring, given how long it's taking us to come up with a better solution, I think that's a good intermediate step. Sorry for rejecting it initially, I thought we could converge on a better solution faster. > > Signed-off-by: Jacopo Mondi <jacopo@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); } I'd split this to a separate patch. How much of the std::map API do we want/need to expose ? > > + > > + 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; > > + } Given that we'll have only two specializations of this class, could we avoid inlining the larger functions (that is all but isEmpty() and data()) ? You can move them to formats.cpp, for instance template<typename T> std::vector<T> ImageFormats<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; } and at the end add template class ImageFormats<uint32_t>; template class ImageFormats<V4L2PixelFormat>; > > + > > + const std::map<T, std::vector<SizeRange>> &data() const > > + { > > + return data_; > > + } I would then make this a single line, or make isEmpty() multi-line. > > > > 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. s/integer/uint32_t/ > > * > > * 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()) {
Hi Niklas, On Sat, Jun 06, 2020 at 02:18:40AM +0300, Laurent Pinchart wrote: > Hi Jacopo and Niklas, > > On Fri, Jun 05, 2020 at 09:36:43PM +0200, Niklas Söderlund wrote: > > 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. > > I agree with you. I still believe we'll be able to find a better > abstraction in the long term. If there's value in refactoring, given how > long it's taking us to come up with a better solution, I think that's a > good intermediate step. Sorry for rejecting it initially, I thought we > could converge on a better solution faster. Yeah, this is very similar, and I'm aware I contributed to shot that proposal down, sorry about this. Howevere, we still have ImageFormats around, and pipeline handlers are starting to abuse it, so as it's still around, I think it's worth trying to make it nicer to use > > > > Signed-off-by: Jacopo Mondi <jacopo@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); } > > I'd split this to a separate patch. How much of the std::map API do we > want/need to expose ? > > > > + > > > + 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; > > > + } > > Given that we'll have only two specializations of this class, could we > avoid inlining the larger functions (that is all but isEmpty() and > data()) ? You can move them to formats.cpp, for instance > > template<typename T> > std::vector<T> ImageFormats<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; > } > > and at the end add > > template class ImageFormats<uint32_t>; > template class ImageFormats<V4L2PixelFormat>; > > > > + > > > + const std::map<T, std::vector<SizeRange>> &data() const > > > + { > > > + return data_; > > > + } > > I would then make this a single line, or make isEmpty() multi-line. > > > > > > > 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. > > s/integer/uint32_t/ > > > > * > > > * 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()) { > > -- > Regards, > > Laurent Pinchart
Hi Laurent, On Sat, Jun 06, 2020 at 02:18:40AM +0300, Laurent Pinchart wrote: > Hi Jacopo and Niklas, > > On Fri, Jun 05, 2020 at 09:36:43PM +0200, Niklas Söderlund wrote: > > 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. > > I agree with you. I still believe we'll be able to find a better > abstraction in the long term. If there's value in refactoring, given how > long it's taking us to come up with a better solution, I think that's a > good intermediate step. Sorry for rejecting it initially, I thought we > could converge on a better solution faster. > > > > Signed-off-by: Jacopo Mondi <jacopo@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); } > > I'd split this to a separate patch. How much of the std::map API do we > want/need to expose ? > Iterators, begin and end for sure. Not sure about emplace and find, which are not used... I'll leave them out, but I'm not sure it's worth a separate patch to add iterators :/ > > > + > > > + 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; > > > + } > > Given that we'll have only two specializations of this class, could we > avoid inlining the larger functions (that is all but isEmpty() and > data()) ? You can move them to formats.cpp, for instance > > template<typename T> > std::vector<T> ImageFormats<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; > } > > and at the end add > > template class ImageFormats<uint32_t>; > template class ImageFormats<V4L2PixelFormat>; > Sure thing, I had it this way as I had the idea to unify this with StreamFormats, which is a very similar construct.. But then I gave up as the idea of making image format public would have required to convince too many people :) Thanks j > > > + > > > + const std::map<T, std::vector<SizeRange>> &data() const > > > + { > > > + return data_; > > > + } > > I would then make this a single line, or make isEmpty() multi-line. > > > > > > > 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. > > s/integer/uint32_t/ > > > > * > > > * 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()) { > > -- > Regards, > > Laurent Pinchart
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()) {
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. Signed-off-by: Jacopo Mondi <jacopo@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(-)