[libcamera-devel] [PATCH 1/5] libcamera: formats: Make ImageFormats a templated class
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sat Jun 6 01:18:40 CEST 2020
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 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); }
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
More information about the libcamera-devel
mailing list