[libcamera-devel] [PATCH 1/5] libcamera: formats: Make ImageFormats a templated class
Jacopo Mondi
jacopo at jmondi.org
Mon Jun 8 23:30:13 CEST 2020
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 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 ?
>
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
More information about the libcamera-devel
mailing list