| Message ID | 20220802160136.63075-6-jacopo@jmondi.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, Thank you for the patch. On Tue, Aug 02, 2022 at 06:01:35PM +0200, Jacopo Mondi via libcamera-devel wrote: > Now that V4L2PixelFormat::fromPixelFormat() returns a list of formats > to chose from, select the one supported by the video device by matching > against the list of supported pixel formats. > > The first format found to match one of the device supported ones is > returned. > > As the list of pixel formats supported by the video device does not > change at run-time, cache it at device open() time. To maximize the > lookup efficiency store the list of supported V4L2PixelFormat in an > std::unordered_set<> which requires the V4L2PixelFormat class to be > associated with an hash function object and to support the comparison s/an hash/a hash/ > operator. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > include/libcamera/internal/v4l2_pixelformat.h | 15 +++++ > include/libcamera/internal/v4l2_videodevice.h | 4 ++ > src/libcamera/v4l2_pixelformat.cpp | 29 ++++++++++ > src/libcamera/v4l2_videodevice.cpp | 58 ++++++++++++++----- > 4 files changed, 92 insertions(+), 14 deletions(-) > > diff --git a/include/libcamera/internal/v4l2_pixelformat.h b/include/libcamera/internal/v4l2_pixelformat.h > index d5400f90a67e..05069a30304d 100644 > --- a/include/libcamera/internal/v4l2_pixelformat.h > +++ b/include/libcamera/internal/v4l2_pixelformat.h > @@ -8,6 +8,7 @@ > > #pragma once > > +#include <functional> > #include <ostream> > #include <stdint.h> > #include <string> > @@ -22,6 +23,15 @@ namespace libcamera { > class V4L2PixelFormat > { > public: > + class Hasher > + { > + public: > + std::size_t operator()(const V4L2PixelFormat &f) const > + { > + return f.fourcc(); > + } > + }; It would be simpler to inject a specialization of std::hash<> in the std namespace, that would allow declaring std::unordered_set and std::unordered_map instances without having to pass a hasher explicitly. I'll send a patch with this. > + > struct Info { > PixelFormat format; > const char *description; > @@ -37,6 +47,11 @@ public: > { > } > > + bool operator==(const V4L2PixelFormat &other) const > + { > + return fourcc_ == other.fourcc_; > + } This isn't needed, the default operator==() implementation generated by the compiler should be enough. > + > bool isValid() const { return fourcc_ != 0; } > uint32_t fourcc() const { return fourcc_; } > operator uint32_t() const { return fourcc_; } > diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h > index 29fa0bbaf670..c9064291a669 100644 > --- a/include/libcamera/internal/v4l2_videodevice.h > +++ b/include/libcamera/internal/v4l2_videodevice.h > @@ -14,6 +14,7 @@ > #include <ostream> > #include <stdint.h> > #include <string> > +#include <unordered_set> > #include <vector> > > #include <linux/videodev2.h> > @@ -242,6 +243,8 @@ private: > Stopped, > }; > > + int initFormats(); > + > int getFormatMeta(V4L2DeviceFormat *format); > int trySetFormatMeta(V4L2DeviceFormat *format, bool set); > > @@ -268,6 +271,7 @@ private: > V4L2Capability caps_; > V4L2DeviceFormat format_; > const PixelFormatInfo *formatInfo_; > + std::unordered_set<V4L2PixelFormat, V4L2PixelFormat::Hasher> pixelFormats_; > > enum v4l2_buf_type bufferType_; > enum v4l2_memory memoryType_; > diff --git a/src/libcamera/v4l2_pixelformat.cpp b/src/libcamera/v4l2_pixelformat.cpp > index 90c8fa8d9aae..264ca80aab27 100644 > --- a/src/libcamera/v4l2_pixelformat.cpp > +++ b/src/libcamera/v4l2_pixelformat.cpp > @@ -187,6 +187,28 @@ const std::map<V4L2PixelFormat, V4L2PixelFormat::Info> vpf2pf{ > > } /* namespace */ > > +/** > + * \class V4L2PixelFormat::Hasher > + * \brief Function object class that implement hash function for the > + * V4L2PixelFormat class > + * > + * The Hasher class is a function object class that that allows to use the > + * V4L2PixelFormat class with C++ STL unordered associative containers. > + * > + * The hash for a V4L2PixelFormat is obtained by using the function call > + * operator(). > + */ > + > +/** > + * \fn V4L2PixelFormat::Hasher::operator()(const V4L2PixelFormat &f) const > + * \brief Function call operator that computes the V4L2PixelFormat hash value > + * \param[in] f The V4L2PixelFormat to compute the hash on > + * > + * Compute the hash value of \a f, which simply is the numerical FourCC value. > + * > + * \return The numerical FourCC value for \a f > + */ > + > /** > * \struct V4L2PixelFormat::Info > * \brief Information about a V4L2 pixel format > @@ -208,6 +230,13 @@ const std::map<V4L2PixelFormat, V4L2PixelFormat::Info> vpf2pf{ > * invalid, calling the isValid() function returns false. > */ > > +/** > + * \fn V4L2PixelFormat::operator==() > + * \brief Compare two V4L2PixelFormat by comparing their FourCC codes > + * \param[in] other The other V4L2PixelFormat to compare with > + * \return True if the FourCC codes are the same, false otherwise > + */ > + > /** > * \fn V4L2PixelFormat::V4L2PixelFormat(uint32_t fourcc) > * \brief Construct a V4L2PixelFormat from a FourCC value > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp > index 2ca22f485d45..c3343ca7078b 100644 > --- a/src/libcamera/v4l2_videodevice.cpp > +++ b/src/libcamera/v4l2_videodevice.cpp > @@ -629,18 +629,14 @@ int V4L2VideoDevice::open() > fdBufferNotifier_->activated.connect(this, &V4L2VideoDevice::bufferAvailable); > fdBufferNotifier_->setEnabled(false); > > + ret = initFormats(); > + if (ret) > + return ret; Could you move this below the debug message to avoid changing the behaviour ? Same in the other open() function. > + > LOG(V4L2, Debug) > << "Opened device " << caps_.bus_info() << ": " > << caps_.driver() << ": " << caps_.card(); > > - ret = getFormat(&format_); > - if (ret) { > - LOG(V4L2, Error) << "Failed to get format"; > - return ret; > - } > - > - formatInfo_ = &PixelFormatInfo::info(format_.fourcc); > - > return 0; > } > > @@ -722,11 +718,25 @@ int V4L2VideoDevice::open(SharedFD handle, enum v4l2_buf_type type) > fdBufferNotifier_->activated.connect(this, &V4L2VideoDevice::bufferAvailable); > fdBufferNotifier_->setEnabled(false); > > + ret = initFormats(); > + if (ret) > + return ret; > + > LOG(V4L2, Debug) > << "Opened device " << caps_.bus_info() << ": " > << caps_.driver() << ": " << caps_.card(); > > - ret = getFormat(&format_); > + return 0; > +} > + > +int V4L2VideoDevice::initFormats() > +{ > + const std::vector<V4L2PixelFormat> &deviceFormats = enumPixelformats(0); No error checking ? > + pixelFormats_ = std::unordered_set<V4L2PixelFormat, > + V4L2PixelFormat::Hasher> > + (deviceFormats.begin(), deviceFormats.end()); The constructor you use here is not explicit, so you can write pixelFormats_ = { deviceFormats.begin(), deviceFormats.end() }; With these small issues addressed, and the V4L2PixelFormat changes dropped if you want to use the "[PATCH] libcamera: v4l2_pixelformat: Implement std::hash specialization" patch I've sent (and the commit message then updated accordingly), Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + > + int ret = getFormat(&format_); > if (ret) { > LOG(V4L2, Error) << "Failed to get format"; > return ret; > @@ -1990,17 +2000,37 @@ V4L2VideoDevice::fromEntityName(const MediaDevice *media, > } > > /** > - * \brief Convert \a PixelFormat to its corresponding V4L2 FourCC > + * \brief Convert \a PixelFormat to a V4L2PixelFormat supported by the device > * \param[in] pixelFormat The PixelFormat to convert > * > - * The V4L2 format variant the function returns the contiguous version > - * unconditionally. > + * Convert \a pixelformat to a V4L2 FourCC that is known to be supported by > + * the video device. > * > - * \return The V4L2_PIX_FMT_* pixel format code corresponding to \a pixelFormat > + * A V4L2VideoDevice may support different V4L2 pixel formats that map the same > + * PixelFormat. This is the case of the contiguous and non-contiguous variants > + * of multiplanar formats, and with the V4L2 MJPEG and JPEG pixel formats. > + * Converting a PixelFormat to a V4L2PixelFormat may thus have multiple answers. > + * > + * This function converts the \a pixelFormat using the list of V4L2 pixel > + * formats that the V4L2VideoDevice supports. This guarantees that the returned > + * V4L2PixelFormat will be valid for the device. If multiple matches are still > + * possible, contiguous variants are preferred. If the \a pixelFormat is not > + * supported by the device, the function returns an invalid V4L2PixelFormat. > + * > + * \return The V4L2PixelFormat corresponding to \a pixelFormat if supported by > + * the device, or an invalid V4L2PixelFormat otherwise > */ > V4L2PixelFormat V4L2VideoDevice::toV4L2PixelFormat(const PixelFormat &pixelFormat) const > { > - return V4L2PixelFormat::fromPixelFormat(pixelFormat)[0]; > + const std::vector<V4L2PixelFormat> &v4l2PixelFormats = > + V4L2PixelFormat::fromPixelFormat(pixelFormat); > + > + for (const V4L2PixelFormat &v4l2Format : v4l2PixelFormats) { > + if (pixelFormats_.count(v4l2Format)) > + return v4l2Format; > + } > + > + return {}; > } > > /**
diff --git a/include/libcamera/internal/v4l2_pixelformat.h b/include/libcamera/internal/v4l2_pixelformat.h index d5400f90a67e..05069a30304d 100644 --- a/include/libcamera/internal/v4l2_pixelformat.h +++ b/include/libcamera/internal/v4l2_pixelformat.h @@ -8,6 +8,7 @@ #pragma once +#include <functional> #include <ostream> #include <stdint.h> #include <string> @@ -22,6 +23,15 @@ namespace libcamera { class V4L2PixelFormat { public: + class Hasher + { + public: + std::size_t operator()(const V4L2PixelFormat &f) const + { + return f.fourcc(); + } + }; + struct Info { PixelFormat format; const char *description; @@ -37,6 +47,11 @@ public: { } + bool operator==(const V4L2PixelFormat &other) const + { + return fourcc_ == other.fourcc_; + } + bool isValid() const { return fourcc_ != 0; } uint32_t fourcc() const { return fourcc_; } operator uint32_t() const { return fourcc_; } diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h index 29fa0bbaf670..c9064291a669 100644 --- a/include/libcamera/internal/v4l2_videodevice.h +++ b/include/libcamera/internal/v4l2_videodevice.h @@ -14,6 +14,7 @@ #include <ostream> #include <stdint.h> #include <string> +#include <unordered_set> #include <vector> #include <linux/videodev2.h> @@ -242,6 +243,8 @@ private: Stopped, }; + int initFormats(); + int getFormatMeta(V4L2DeviceFormat *format); int trySetFormatMeta(V4L2DeviceFormat *format, bool set); @@ -268,6 +271,7 @@ private: V4L2Capability caps_; V4L2DeviceFormat format_; const PixelFormatInfo *formatInfo_; + std::unordered_set<V4L2PixelFormat, V4L2PixelFormat::Hasher> pixelFormats_; enum v4l2_buf_type bufferType_; enum v4l2_memory memoryType_; diff --git a/src/libcamera/v4l2_pixelformat.cpp b/src/libcamera/v4l2_pixelformat.cpp index 90c8fa8d9aae..264ca80aab27 100644 --- a/src/libcamera/v4l2_pixelformat.cpp +++ b/src/libcamera/v4l2_pixelformat.cpp @@ -187,6 +187,28 @@ const std::map<V4L2PixelFormat, V4L2PixelFormat::Info> vpf2pf{ } /* namespace */ +/** + * \class V4L2PixelFormat::Hasher + * \brief Function object class that implement hash function for the + * V4L2PixelFormat class + * + * The Hasher class is a function object class that that allows to use the + * V4L2PixelFormat class with C++ STL unordered associative containers. + * + * The hash for a V4L2PixelFormat is obtained by using the function call + * operator(). + */ + +/** + * \fn V4L2PixelFormat::Hasher::operator()(const V4L2PixelFormat &f) const + * \brief Function call operator that computes the V4L2PixelFormat hash value + * \param[in] f The V4L2PixelFormat to compute the hash on + * + * Compute the hash value of \a f, which simply is the numerical FourCC value. + * + * \return The numerical FourCC value for \a f + */ + /** * \struct V4L2PixelFormat::Info * \brief Information about a V4L2 pixel format @@ -208,6 +230,13 @@ const std::map<V4L2PixelFormat, V4L2PixelFormat::Info> vpf2pf{ * invalid, calling the isValid() function returns false. */ +/** + * \fn V4L2PixelFormat::operator==() + * \brief Compare two V4L2PixelFormat by comparing their FourCC codes + * \param[in] other The other V4L2PixelFormat to compare with + * \return True if the FourCC codes are the same, false otherwise + */ + /** * \fn V4L2PixelFormat::V4L2PixelFormat(uint32_t fourcc) * \brief Construct a V4L2PixelFormat from a FourCC value diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp index 2ca22f485d45..c3343ca7078b 100644 --- a/src/libcamera/v4l2_videodevice.cpp +++ b/src/libcamera/v4l2_videodevice.cpp @@ -629,18 +629,14 @@ int V4L2VideoDevice::open() fdBufferNotifier_->activated.connect(this, &V4L2VideoDevice::bufferAvailable); fdBufferNotifier_->setEnabled(false); + ret = initFormats(); + if (ret) + return ret; + LOG(V4L2, Debug) << "Opened device " << caps_.bus_info() << ": " << caps_.driver() << ": " << caps_.card(); - ret = getFormat(&format_); - if (ret) { - LOG(V4L2, Error) << "Failed to get format"; - return ret; - } - - formatInfo_ = &PixelFormatInfo::info(format_.fourcc); - return 0; } @@ -722,11 +718,25 @@ int V4L2VideoDevice::open(SharedFD handle, enum v4l2_buf_type type) fdBufferNotifier_->activated.connect(this, &V4L2VideoDevice::bufferAvailable); fdBufferNotifier_->setEnabled(false); + ret = initFormats(); + if (ret) + return ret; + LOG(V4L2, Debug) << "Opened device " << caps_.bus_info() << ": " << caps_.driver() << ": " << caps_.card(); - ret = getFormat(&format_); + return 0; +} + +int V4L2VideoDevice::initFormats() +{ + const std::vector<V4L2PixelFormat> &deviceFormats = enumPixelformats(0); + pixelFormats_ = std::unordered_set<V4L2PixelFormat, + V4L2PixelFormat::Hasher> + (deviceFormats.begin(), deviceFormats.end()); + + int ret = getFormat(&format_); if (ret) { LOG(V4L2, Error) << "Failed to get format"; return ret; @@ -1990,17 +2000,37 @@ V4L2VideoDevice::fromEntityName(const MediaDevice *media, } /** - * \brief Convert \a PixelFormat to its corresponding V4L2 FourCC + * \brief Convert \a PixelFormat to a V4L2PixelFormat supported by the device * \param[in] pixelFormat The PixelFormat to convert * - * The V4L2 format variant the function returns the contiguous version - * unconditionally. + * Convert \a pixelformat to a V4L2 FourCC that is known to be supported by + * the video device. * - * \return The V4L2_PIX_FMT_* pixel format code corresponding to \a pixelFormat + * A V4L2VideoDevice may support different V4L2 pixel formats that map the same + * PixelFormat. This is the case of the contiguous and non-contiguous variants + * of multiplanar formats, and with the V4L2 MJPEG and JPEG pixel formats. + * Converting a PixelFormat to a V4L2PixelFormat may thus have multiple answers. + * + * This function converts the \a pixelFormat using the list of V4L2 pixel + * formats that the V4L2VideoDevice supports. This guarantees that the returned + * V4L2PixelFormat will be valid for the device. If multiple matches are still + * possible, contiguous variants are preferred. If the \a pixelFormat is not + * supported by the device, the function returns an invalid V4L2PixelFormat. + * + * \return The V4L2PixelFormat corresponding to \a pixelFormat if supported by + * the device, or an invalid V4L2PixelFormat otherwise */ V4L2PixelFormat V4L2VideoDevice::toV4L2PixelFormat(const PixelFormat &pixelFormat) const { - return V4L2PixelFormat::fromPixelFormat(pixelFormat)[0]; + const std::vector<V4L2PixelFormat> &v4l2PixelFormats = + V4L2PixelFormat::fromPixelFormat(pixelFormat); + + for (const V4L2PixelFormat &v4l2Format : v4l2PixelFormats) { + if (pixelFormats_.count(v4l2Format)) + return v4l2Format; + } + + return {}; } /**
Now that V4L2PixelFormat::fromPixelFormat() returns a list of formats to chose from, select the one supported by the video device by matching against the list of supported pixel formats. The first format found to match one of the device supported ones is returned. As the list of pixel formats supported by the video device does not change at run-time, cache it at device open() time. To maximize the lookup efficiency store the list of supported V4L2PixelFormat in an std::unordered_set<> which requires the V4L2PixelFormat class to be associated with an hash function object and to support the comparison operator. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- include/libcamera/internal/v4l2_pixelformat.h | 15 +++++ include/libcamera/internal/v4l2_videodevice.h | 4 ++ src/libcamera/v4l2_pixelformat.cpp | 29 ++++++++++ src/libcamera/v4l2_videodevice.cpp | 58 ++++++++++++++----- 4 files changed, 92 insertions(+), 14 deletions(-)