[libcamera-devel] [RFC 2/6] libcamera: formats: Turn ImageFormats into a template
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sat Feb 29 11:43:01 CET 2020
Hi Niklas,
Thank you for the patch.
On Fri, Feb 28, 2020 at 10:46:51AM +0100, Jacopo Mondi wrote:
> On Fri, Feb 28, 2020 at 04:29:09AM +0100, Niklas Söderlund wrote:
> > The ImageFormats class is used to carry both V4L2 pixelformats and mbus
> > codes as keys to lookup different supported sizes. The reason for this
> > is that both V4L2 pixelformats and mbus codes are unsigned integers and
> > the keyword PixelFormat is currently defined as such.
> >
> > Going forward PixelFormat will be reworked into a class so their is a
s/their/there/
> > need to have ImageFormats instances with two different key types, turn
> > it into an template and update all sites to use unsigned int where a
> > mbus code is the key and PixelFormat otherwise.
>
> Sorry, but I'm not sure to follow.
>
> ImageFormats is said to report image formats for V4L2Device and
> V4L2Subdevices at the moment and indeed it uses an unsigned int as key
> which could be interpreted as either a v4l2 fourcc in case we're
> dealing with a video device, or as an mbus media code if we're dealing
> with a v4l2 subdev.
>
> PixelFormat is designed to be exposed to applications, and I would
> really avoid create dependencies between the V4L2VideoDevice 4cc
> codes and the application facing one.
>
> I agree ImageFormats is not a descriptive name, and I would be in
> favour of renaming it to, say, DeviceFormat or whatever, but I'm not
> sure what we gain by making v4l2 devices tied to the same class we use
> to report image formats to application.
>
> Will I find it out later in the series ?
I'm with Jacopo on this one, I don't like it much. This will essentially
duplicate the whole class at compilation time, with lots of code that
shouldn't depend on T. I think refactoring is needed. I'll comment
further on patch 3/6.
> > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > ---
> > src/libcamera/camera_sensor.cpp | 2 +-
> > src/libcamera/formats.cpp | 19 +++++++++++++------
> > src/libcamera/include/formats.h | 11 ++++++-----
> > src/libcamera/include/v4l2_subdevice.h | 2 +-
> > src/libcamera/include/v4l2_videodevice.h | 2 +-
> > src/libcamera/pipeline/uvcvideo.cpp | 2 +-
> > src/libcamera/pipeline/vimc.cpp | 2 +-
> > src/libcamera/v4l2_subdevice.cpp | 4 ++--
> > src/libcamera/v4l2_videodevice.cpp | 4 ++--
> > test/v4l2_subdevice/list_formats.cpp | 2 +-
> > 10 files changed, 29 insertions(+), 21 deletions(-)
> >
> > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> > index 2219a43074362834..23fdafb690117d91 100644
> > --- a/src/libcamera/camera_sensor.cpp
> > +++ b/src/libcamera/camera_sensor.cpp
> > @@ -131,7 +131,7 @@ int CameraSensor::init()
> > properties_.set(properties::Rotation, propertyValue);
> >
> > /* Enumerate and cache media bus codes and sizes. */
> > - const ImageFormats formats = subdev_->formats(0);
> > + const ImageFormats<unsigned int> formats = subdev_->formats(0);
> > if (formats.isEmpty()) {
> > LOG(CameraSensor, Error) << "No image format found";
> > return -EINVAL;
> > diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp
> > index 5f6552a4e06c5231..98817deee2b54c84 100644
> > --- a/src/libcamera/formats.cpp
> > +++ b/src/libcamera/formats.cpp
> > @@ -39,7 +39,8 @@ namespace libcamera {
> > * \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)
> > +template<typename T>
> > +int ImageFormats<T>::addFormat(T format, const std::vector<SizeRange> &sizes)
> > {
> > if (data_.find(format) != data_.end())
> > return -EEXIST;
> > @@ -53,7 +54,8 @@ int ImageFormats::addFormat(unsigned int format, const std::vector<SizeRange> &s
> > * \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
> > +template<typename T>
> > +bool ImageFormats<T>::isEmpty() const
> > {
> > return data_.empty();
> > }
> > @@ -62,9 +64,10 @@ bool ImageFormats::isEmpty() 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
> > +template<typename T>
> > +std::vector<T> ImageFormats<T>::formats() const
> > {
> > - std::vector<unsigned int> formats;
> > + std::vector<T> formats;
> > formats.reserve(data_.size());
> >
> > /* \todo: Should this be cached instead of computed each time? */
> > @@ -84,7 +87,8 @@ 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
> > +template<typename T>
> > +const std::vector<SizeRange> &ImageFormats<T>::sizes(T format) const
> > {
> > static const std::vector<SizeRange> empty;
> >
> > @@ -99,9 +103,12 @@ const std::vector<SizeRange> &ImageFormats::sizes(unsigned int format) 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
> > +template<typename T>
> > +const std::map<T, std::vector<SizeRange>> &ImageFormats<T>::data() const
> > {
> > return data_;
> > }
> >
> > +template class ImageFormats<unsigned int>;
> > +
> > } /* namespace libcamera */
> > diff --git a/src/libcamera/include/formats.h b/src/libcamera/include/formats.h
> > index f43bc8c004f690b4..566be9c09550a830 100644
> > --- a/src/libcamera/include/formats.h
> > +++ b/src/libcamera/include/formats.h
> > @@ -15,18 +15,19 @@
> >
> > namespace libcamera {
> >
> > +template<typename T>
> > class ImageFormats
> > {
> > public:
> > - int addFormat(unsigned int format, const std::vector<SizeRange> &sizes);
> > + int addFormat(T format, const std::vector<SizeRange> &sizes);
> >
> > 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;
> > + std::vector<T> formats() const;
> > + const std::vector<SizeRange> &sizes(T format) const;
> > + const std::map<T, std::vector<SizeRange>> &data() const;
> >
> > private:
> > - std::map<unsigned int, std::vector<SizeRange>> data_;
> > + std::map<T, std::vector<SizeRange>> data_;
> > };
> >
> > } /* namespace libcamera */
> > diff --git a/src/libcamera/include/v4l2_subdevice.h b/src/libcamera/include/v4l2_subdevice.h
> > index 9c077674f997dae6..53c73617efe4b860 100644
> > --- a/src/libcamera/include/v4l2_subdevice.h
> > +++ b/src/libcamera/include/v4l2_subdevice.h
> > @@ -44,7 +44,7 @@ public:
> > int setCrop(unsigned int pad, Rectangle *rect);
> > int setCompose(unsigned int pad, Rectangle *rect);
> >
> > - ImageFormats formats(unsigned int pad);
> > + ImageFormats<unsigned int> formats(unsigned int pad);
> >
> > int getFormat(unsigned int pad, V4L2SubdeviceFormat *format);
> > int setFormat(unsigned int pad, V4L2SubdeviceFormat *format);
> > diff --git a/src/libcamera/include/v4l2_videodevice.h b/src/libcamera/include/v4l2_videodevice.h
> > index fcf072641420dacf..982df9d2f918e49c 100644
> > --- a/src/libcamera/include/v4l2_videodevice.h
> > +++ b/src/libcamera/include/v4l2_videodevice.h
> > @@ -180,7 +180,7 @@ public:
> >
> > int getFormat(V4L2DeviceFormat *format);
> > int setFormat(V4L2DeviceFormat *format);
> > - ImageFormats formats();
> > + ImageFormats<PixelFormat> formats();
> >
> > int setCrop(Rectangle *rect);
> > int setCompose(Rectangle *rect);
> > diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp
> > index cf419e8b25a87389..8efd188e75a3d135 100644
> > --- a/src/libcamera/pipeline/uvcvideo.cpp
> > +++ b/src/libcamera/pipeline/uvcvideo.cpp
> > @@ -155,7 +155,7 @@ CameraConfiguration *PipelineHandlerUVC::generateConfiguration(Camera *camera,
> > if (roles.empty())
> > return config;
> >
> > - ImageFormats v4l2formats = data->video_->formats();
> > + ImageFormats<PixelFormat> v4l2formats = data->video_->formats();
> > StreamFormats formats(v4l2formats.data());
> > StreamConfiguration cfg(formats);
> >
> > diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp
> > index 93d89729e1faa29f..9fbe33c626e327d4 100644
> > --- a/src/libcamera/pipeline/vimc.cpp
> > +++ b/src/libcamera/pipeline/vimc.cpp
> > @@ -175,7 +175,7 @@ CameraConfiguration *PipelineHandlerVimc::generateConfiguration(Camera *camera,
> > if (roles.empty())
> > return config;
> >
> > - ImageFormats formats;
> > + ImageFormats<PixelFormat> formats;
> >
> > for (PixelFormat pixelformat : pixelformats) {
> > /* The scaler hardcodes a x3 scale-up ratio. */
> > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp
> > index f2bcd7f73c5c75a1..bec14f80c960d854 100644
> > --- a/src/libcamera/v4l2_subdevice.cpp
> > +++ b/src/libcamera/v4l2_subdevice.cpp
> > @@ -155,9 +155,9 @@ int V4L2Subdevice::setCompose(unsigned int pad, Rectangle *rect)
> > *
> > * \return A list of the supported device formats
> > */
> > -ImageFormats V4L2Subdevice::formats(unsigned int pad)
> > +ImageFormats<unsigned int> V4L2Subdevice::formats(unsigned int pad)
> > {
> > - ImageFormats formats;
> > + ImageFormats<unsigned int> formats;
> >
> > if (pad >= entity_->pads().size()) {
> > LOG(V4L2, Error) << "Invalid pad: " << pad;
> > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> > index 99470ce11421c77c..f84bd00570afa38c 100644
> > --- a/src/libcamera/v4l2_videodevice.cpp
> > +++ b/src/libcamera/v4l2_videodevice.cpp
> > @@ -837,9 +837,9 @@ int V4L2VideoDevice::setFormatSingleplane(V4L2DeviceFormat *format)
> > *
> > * \return A list of the supported video device formats
> > */
> > -ImageFormats V4L2VideoDevice::formats()
> > +ImageFormats<PixelFormat> V4L2VideoDevice::formats()
> > {
> > - ImageFormats formats;
> > + ImageFormats<PixelFormat> formats;
> >
> > for (unsigned int pixelformat : enumPixelformats()) {
> > std::vector<SizeRange> sizes = enumSizes(pixelformat);
> > diff --git a/test/v4l2_subdevice/list_formats.cpp b/test/v4l2_subdevice/list_formats.cpp
> > index 067dc5ed30f4edd9..201a294c72a09b4d 100644
> > --- a/test/v4l2_subdevice/list_formats.cpp
> > +++ b/test/v4l2_subdevice/list_formats.cpp
> > @@ -47,7 +47,7 @@ void ListFormatsTest::printFormats(unsigned int pad,
> > int ListFormatsTest::run()
> > {
> > /* List all formats available on existing "Scaler" pads. */
> > - ImageFormats formats;
> > + ImageFormats<unsigned int> formats;
> >
> > formats = scaler_->formats(0);
> > if (formats.isEmpty()) {
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list