| Message ID | 20200316234649.2545-2-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, Thanks for your work. On 2020-03-17 01:46:48 +0200, Laurent Pinchart wrote: > The V4L2PixelFormat class describes the pixel format of a V4L2 buffer. > It wraps the V4L2 numerical FourCC, and shall be used in all APIs that > deal with V4L2 pixel formats. Its purpose is to prevent unintentional > confusion of V4L2 and DRM FourCCs in code by catching implicit > conversion attempts at compile time. > > The constructor taking a V4L2 FourCC integer value will be made explicit > in a further commit to minimize the size of this change and keep it > reviewable. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > src/libcamera/include/v4l2_videodevice.h | 35 ++++++++++--- > src/libcamera/v4l2_videodevice.cpp | 62 +++++++++++++++++++----- > 2 files changed, 79 insertions(+), 18 deletions(-) > > diff --git a/src/libcamera/include/v4l2_videodevice.h b/src/libcamera/include/v4l2_videodevice.h > index b3000f3c5133..49d2ca357efa 100644 > --- a/src/libcamera/include/v4l2_videodevice.h > +++ b/src/libcamera/include/v4l2_videodevice.h > @@ -149,10 +149,31 @@ private: > unsigned int missCounter_; > }; > > +class V4L2PixelFormat > +{ > +public: > + V4L2PixelFormat() > + : fourcc_(0) > + { > + } > + > + V4L2PixelFormat(uint32_t fourcc) > + : fourcc_(fourcc) > + { > + } > + > + bool isValid() const { return fourcc_ != 0; } > + uint32_t value() const { return fourcc_; } > + operator uint32_t() const { return fourcc_; } > + > +private: > + uint32_t fourcc_; > +}; > + > class V4L2DeviceFormat > { > public: > - uint32_t fourcc; > + V4L2PixelFormat fourcc; > Size size; > > struct { > @@ -184,7 +205,7 @@ public: > > int getFormat(V4L2DeviceFormat *format); > int setFormat(V4L2DeviceFormat *format); > - std::map<unsigned int, std::vector<SizeRange>> formats(); > + std::map<V4L2PixelFormat, std::vector<SizeRange>> formats(); > > int setCrop(Rectangle *rect); > int setCompose(Rectangle *rect); > @@ -203,9 +224,9 @@ public: > static V4L2VideoDevice *fromEntityName(const MediaDevice *media, > const std::string &entity); > > - static PixelFormat toPixelFormat(uint32_t v4l2Fourcc); > - uint32_t toV4L2Fourcc(PixelFormat pixelFormat); > - static uint32_t toV4L2Fourcc(PixelFormat pixelFormat, bool multiplanar); > + static PixelFormat toPixelFormat(V4L2PixelFormat v4l2Fourcc); > + V4L2PixelFormat toV4L2Fourcc(PixelFormat pixelFormat); > + static V4L2PixelFormat toV4L2Fourcc(PixelFormat pixelFormat, bool multiplanar); > > protected: > std::string logPrefix() const; > @@ -220,8 +241,8 @@ private: > int getFormatSingleplane(V4L2DeviceFormat *format); > int setFormatSingleplane(V4L2DeviceFormat *format); > > - std::vector<unsigned int> enumPixelformats(); > - std::vector<SizeRange> enumSizes(unsigned int pixelFormat); > + std::vector<V4L2PixelFormat> enumPixelformats(); > + std::vector<SizeRange> enumSizes(V4L2PixelFormat pixelFormat); > > int setSelection(unsigned int target, Rectangle *rect); > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp > index 81911e764fde..40396c22aa45 100644 > --- a/src/libcamera/v4l2_videodevice.cpp > +++ b/src/libcamera/v4l2_videodevice.cpp > @@ -277,6 +277,46 @@ bool V4L2BufferCache::Entry::operator==(const FrameBuffer &buffer) const > return true; > } > > +/** > + * \class V4L2PixelFormat > + * \brief V4L2 pixel format wrapper > + * > + * The V4L2PixelFormat class describes the pixel format of a V4L2 buffer. It > + * wraps the V4L2 numerical FourCC, and shall be used in all APIs that deal with > + * V4L2 pixel formats. Its purpose is to prevent unintentional confusion of > + * V4L2 and DRM FourCCs in code by catching implicit conversion attempts at > + * compile time. > + */ > + > +/** > + * \fn V4L2PixelFormat::V4L2PixelFormat() > + * \brief Construct an invalid V4L2 pixel format with a numerical value of 0 > + */ > + > +/** > + * \fn V4L2PixelFormat::V4L2PixelFormat(uint32_t fourcc) > + * \brief Construct a V4L2 pixel format from a FourCC value > + * \param[in] fourcc The pixel format numerical value > + */ > + > +/** > + * \fn bool V4L2PixelFormat::isValid() const > + * \brief Check if the pixel format is valid > + * \return True if the pixel format has a non-zero value, false otherwise > + */ > + > +/** > + * \fn uint32_t V4L2PixelFormat::value() const > + * \brief Retrieve the pixel format numerical value > + * \return The pixel format numerical value > + */ > + > +/** > + * \fn V4L2PixelFormat::operator uint32_t() const > + * \brief Convert the the pixel format numerical value > + * \return The pixel format numerical value > + */ > + > /** > * \class V4L2DeviceFormat > * \brief The V4L2 video device image format and sizes > @@ -385,7 +425,7 @@ bool V4L2BufferCache::Entry::operator==(const FrameBuffer &buffer) const > const std::string V4L2DeviceFormat::toString() const > { > std::stringstream ss; > - ss << size.toString() << "-" << utils::hex(fourcc); > + ss << size.toString() << "-" << utils::hex(fourcc.value()); As you state in the cover letter a toString() method should be added. I think for now it can just convert the FourCC to a hex value and return it as a string. The important thing I think is to convert this and the other utils::hex() callsite bellow to use a toString() method. Whit this fixed, Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > return ss.str(); > } > > @@ -844,11 +884,11 @@ int V4L2VideoDevice::setFormatSingleplane(V4L2DeviceFormat *format) > * > * \return A list of the supported video device formats > */ > -std::map<unsigned int, std::vector<SizeRange>> V4L2VideoDevice::formats() > +std::map<V4L2PixelFormat, std::vector<SizeRange>> V4L2VideoDevice::formats() > { > - std::map<unsigned int, std::vector<SizeRange>> formats; > + std::map<V4L2PixelFormat, std::vector<SizeRange>> formats; > > - for (unsigned int pixelformat : enumPixelformats()) { > + for (V4L2PixelFormat pixelformat : enumPixelformats()) { > std::vector<SizeRange> sizes = enumSizes(pixelformat); > if (sizes.empty()) > return {}; > @@ -859,9 +899,9 @@ std::map<unsigned int, std::vector<SizeRange>> V4L2VideoDevice::formats() > return formats; > } > > -std::vector<unsigned int> V4L2VideoDevice::enumPixelformats() > +std::vector<V4L2PixelFormat> V4L2VideoDevice::enumPixelformats() > { > - std::vector<unsigned int> formats; > + std::vector<V4L2PixelFormat> formats; > int ret; > > for (unsigned int index = 0; ; index++) { > @@ -886,7 +926,7 @@ std::vector<unsigned int> V4L2VideoDevice::enumPixelformats() > return formats; > } > > -std::vector<SizeRange> V4L2VideoDevice::enumSizes(unsigned int pixelFormat) > +std::vector<SizeRange> V4L2VideoDevice::enumSizes(V4L2PixelFormat pixelFormat) > { > std::vector<SizeRange> sizes; > int ret; > @@ -1417,7 +1457,7 @@ V4L2VideoDevice *V4L2VideoDevice::fromEntityName(const MediaDevice *media, > * \param[in] v4l2Fourcc The V4L2 pixel format (V4L2_PIX_FORMAT_*) > * \return The PixelFormat corresponding to \a v4l2Fourcc > */ > -PixelFormat V4L2VideoDevice::toPixelFormat(uint32_t v4l2Fourcc) > +PixelFormat V4L2VideoDevice::toPixelFormat(V4L2PixelFormat v4l2Fourcc) > { > switch (v4l2Fourcc) { > /* RGB formats. */ > @@ -1466,7 +1506,7 @@ PixelFormat V4L2VideoDevice::toPixelFormat(uint32_t v4l2Fourcc) > libcamera::_log(__FILE__, __LINE__, _LOG_CATEGORY(V4L2)(), > LogError).stream() > << "Unsupported V4L2 pixel format " > - << utils::hex(v4l2Fourcc); > + << utils::hex(v4l2Fourcc.value()); > return PixelFormat(); > } > } > @@ -1482,7 +1522,7 @@ PixelFormat V4L2VideoDevice::toPixelFormat(uint32_t v4l2Fourcc) > * > * \return The V4L2_PIX_FMT_* pixel format code corresponding to \a pixelFormat > */ > -uint32_t V4L2VideoDevice::toV4L2Fourcc(PixelFormat pixelFormat) > +V4L2PixelFormat V4L2VideoDevice::toV4L2Fourcc(PixelFormat pixelFormat) > { > return V4L2VideoDevice::toV4L2Fourcc(pixelFormat, caps_.isMultiplanar()); > } > @@ -1500,7 +1540,7 @@ uint32_t V4L2VideoDevice::toV4L2Fourcc(PixelFormat pixelFormat) > * > * \return The V4L2_PIX_FMT_* pixel format code corresponding to \a pixelFormat > */ > -uint32_t V4L2VideoDevice::toV4L2Fourcc(PixelFormat pixelFormat, bool multiplanar) > +V4L2PixelFormat V4L2VideoDevice::toV4L2Fourcc(PixelFormat pixelFormat, bool multiplanar) > { > switch (pixelFormat.fourcc()) { > /* RGB formats. */ > -- > Regards, > > Laurent Pinchart > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Laurent, thanks for the patch On Tue, Mar 17, 2020 at 01:46:48AM +0200, Laurent Pinchart wrote: > The V4L2PixelFormat class describes the pixel format of a V4L2 buffer. > It wraps the V4L2 numerical FourCC, and shall be used in all APIs that > deal with V4L2 pixel formats. Its purpose is to prevent unintentional > confusion of V4L2 and DRM FourCCs in code by catching implicit > conversion attempts at compile time. > > The constructor taking a V4L2 FourCC integer value will be made explicit > in a further commit to minimize the size of this change and keep it > reviewable. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > src/libcamera/include/v4l2_videodevice.h | 35 ++++++++++--- > src/libcamera/v4l2_videodevice.cpp | 62 +++++++++++++++++++----- > 2 files changed, 79 insertions(+), 18 deletions(-) > > diff --git a/src/libcamera/include/v4l2_videodevice.h b/src/libcamera/include/v4l2_videodevice.h > index b3000f3c5133..49d2ca357efa 100644 > --- a/src/libcamera/include/v4l2_videodevice.h > +++ b/src/libcamera/include/v4l2_videodevice.h > @@ -149,10 +149,31 @@ private: > unsigned int missCounter_; > }; > > +class V4L2PixelFormat > +{ > +public: > + V4L2PixelFormat() > + : fourcc_(0) > + { > + } > + > + V4L2PixelFormat(uint32_t fourcc) > + : fourcc_(fourcc) > + { > + } > + > + bool isValid() const { return fourcc_ != 0; } > + uint32_t value() const { return fourcc_; } s/value/fourcc ? > + operator uint32_t() const { return fourcc_; } If I remove this I get errors in all pipelines when comparing V4L2PixelFormat instances. Is this a shortcut to allow implicit cast to uin32_t so that operator==() and operator!=() are happy ? Isn't it better to defined those operator instead ? > + > +private: > + uint32_t fourcc_; > +}; > + > class V4L2DeviceFormat > { > public: > - uint32_t fourcc; > + V4L2PixelFormat fourcc; > Size size; > > struct { > @@ -184,7 +205,7 @@ public: > > int getFormat(V4L2DeviceFormat *format); > int setFormat(V4L2DeviceFormat *format); > - std::map<unsigned int, std::vector<SizeRange>> formats(); > + std::map<V4L2PixelFormat, std::vector<SizeRange>> formats(); > > int setCrop(Rectangle *rect); > int setCompose(Rectangle *rect); > @@ -203,9 +224,9 @@ public: > static V4L2VideoDevice *fromEntityName(const MediaDevice *media, > const std::string &entity); > > - static PixelFormat toPixelFormat(uint32_t v4l2Fourcc); > - uint32_t toV4L2Fourcc(PixelFormat pixelFormat); > - static uint32_t toV4L2Fourcc(PixelFormat pixelFormat, bool multiplanar); > + static PixelFormat toPixelFormat(V4L2PixelFormat v4l2Fourcc); > + V4L2PixelFormat toV4L2Fourcc(PixelFormat pixelFormat); > + static V4L2PixelFormat toV4L2Fourcc(PixelFormat pixelFormat, bool multiplanar); s/toV4L2PixelFormat ? to be consistent with the new class name and leave froucc out of the API > > protected: > std::string logPrefix() const; > @@ -220,8 +241,8 @@ private: > int getFormatSingleplane(V4L2DeviceFormat *format); > int setFormatSingleplane(V4L2DeviceFormat *format); > > - std::vector<unsigned int> enumPixelformats(); > - std::vector<SizeRange> enumSizes(unsigned int pixelFormat); > + std::vector<V4L2PixelFormat> enumPixelformats(); > + std::vector<SizeRange> enumSizes(V4L2PixelFormat pixelFormat); > > int setSelection(unsigned int target, Rectangle *rect); > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp > index 81911e764fde..40396c22aa45 100644 > --- a/src/libcamera/v4l2_videodevice.cpp > +++ b/src/libcamera/v4l2_videodevice.cpp > @@ -277,6 +277,46 @@ bool V4L2BufferCache::Entry::operator==(const FrameBuffer &buffer) const > return true; > } > > +/** > + * \class V4L2PixelFormat > + * \brief V4L2 pixel format wrapper In documentation I would instead mention fourcc. Seems there is a bit of confusion between fourcc and pixel format. To me v4l2 pixel format wraps a V4L2 fourcc code Here v4l2 pixel format wraps a v4l2 pixel format :) > + * > + * The V4L2PixelFormat class describes the pixel format of a V4L2 buffer. It > + * wraps the V4L2 numerical FourCC, and shall be used in all APIs that deal with > + * V4L2 pixel formats. Its purpose is to prevent unintentional confusion of > + * V4L2 and DRM FourCCs in code by catching implicit conversion attempts at > + * compile time. > + */ > + > +/** > + * \fn V4L2PixelFormat::V4L2PixelFormat() > + * \brief Construct an invalid V4L2 pixel format with a numerical value of 0 Also here and below, the "numerical values" are fourcc codes, which are indeed numerical values, but from a specific range :) All minors though Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thanks j > + */ > + > +/** > + * \fn V4L2PixelFormat::V4L2PixelFormat(uint32_t fourcc) > + * \brief Construct a V4L2 pixel format from a FourCC value > + * \param[in] fourcc The pixel format numerical value > + */ > + > +/** > + * \fn bool V4L2PixelFormat::isValid() const > + * \brief Check if the pixel format is valid > + * \return True if the pixel format has a non-zero value, false otherwise > + */ > + > +/** > + * \fn uint32_t V4L2PixelFormat::value() const > + * \brief Retrieve the pixel format numerical value > + * \return The pixel format numerical value > + */ > + > +/** > + * \fn V4L2PixelFormat::operator uint32_t() const > + * \brief Convert the the pixel format numerical value > + * \return The pixel format numerical value > + */ > + > /** > * \class V4L2DeviceFormat > * \brief The V4L2 video device image format and sizes > @@ -385,7 +425,7 @@ bool V4L2BufferCache::Entry::operator==(const FrameBuffer &buffer) const > const std::string V4L2DeviceFormat::toString() const > { > std::stringstream ss; > - ss << size.toString() << "-" << utils::hex(fourcc); > + ss << size.toString() << "-" << utils::hex(fourcc.value()); > return ss.str(); > } > > @@ -844,11 +884,11 @@ int V4L2VideoDevice::setFormatSingleplane(V4L2DeviceFormat *format) > * > * \return A list of the supported video device formats > */ > -std::map<unsigned int, std::vector<SizeRange>> V4L2VideoDevice::formats() > +std::map<V4L2PixelFormat, std::vector<SizeRange>> V4L2VideoDevice::formats() > { > - std::map<unsigned int, std::vector<SizeRange>> formats; > + std::map<V4L2PixelFormat, std::vector<SizeRange>> formats; > > - for (unsigned int pixelformat : enumPixelformats()) { > + for (V4L2PixelFormat pixelformat : enumPixelformats()) { > std::vector<SizeRange> sizes = enumSizes(pixelformat); > if (sizes.empty()) > return {}; > @@ -859,9 +899,9 @@ std::map<unsigned int, std::vector<SizeRange>> V4L2VideoDevice::formats() > return formats; > } > > -std::vector<unsigned int> V4L2VideoDevice::enumPixelformats() > +std::vector<V4L2PixelFormat> V4L2VideoDevice::enumPixelformats() > { > - std::vector<unsigned int> formats; > + std::vector<V4L2PixelFormat> formats; > int ret; > > for (unsigned int index = 0; ; index++) { > @@ -886,7 +926,7 @@ std::vector<unsigned int> V4L2VideoDevice::enumPixelformats() > return formats; > } > > -std::vector<SizeRange> V4L2VideoDevice::enumSizes(unsigned int pixelFormat) > +std::vector<SizeRange> V4L2VideoDevice::enumSizes(V4L2PixelFormat pixelFormat) > { > std::vector<SizeRange> sizes; > int ret; > @@ -1417,7 +1457,7 @@ V4L2VideoDevice *V4L2VideoDevice::fromEntityName(const MediaDevice *media, > * \param[in] v4l2Fourcc The V4L2 pixel format (V4L2_PIX_FORMAT_*) > * \return The PixelFormat corresponding to \a v4l2Fourcc > */ > -PixelFormat V4L2VideoDevice::toPixelFormat(uint32_t v4l2Fourcc) > +PixelFormat V4L2VideoDevice::toPixelFormat(V4L2PixelFormat v4l2Fourcc) > { > switch (v4l2Fourcc) { > /* RGB formats. */ > @@ -1466,7 +1506,7 @@ PixelFormat V4L2VideoDevice::toPixelFormat(uint32_t v4l2Fourcc) > libcamera::_log(__FILE__, __LINE__, _LOG_CATEGORY(V4L2)(), > LogError).stream() > << "Unsupported V4L2 pixel format " > - << utils::hex(v4l2Fourcc); > + << utils::hex(v4l2Fourcc.value()); > return PixelFormat(); > } > } > @@ -1482,7 +1522,7 @@ PixelFormat V4L2VideoDevice::toPixelFormat(uint32_t v4l2Fourcc) > * > * \return The V4L2_PIX_FMT_* pixel format code corresponding to \a pixelFormat > */ > -uint32_t V4L2VideoDevice::toV4L2Fourcc(PixelFormat pixelFormat) > +V4L2PixelFormat V4L2VideoDevice::toV4L2Fourcc(PixelFormat pixelFormat) > { > return V4L2VideoDevice::toV4L2Fourcc(pixelFormat, caps_.isMultiplanar()); > } > @@ -1500,7 +1540,7 @@ uint32_t V4L2VideoDevice::toV4L2Fourcc(PixelFormat pixelFormat) > * > * \return The V4L2_PIX_FMT_* pixel format code corresponding to \a pixelFormat > */ > -uint32_t V4L2VideoDevice::toV4L2Fourcc(PixelFormat pixelFormat, bool multiplanar) > +V4L2PixelFormat V4L2VideoDevice::toV4L2Fourcc(PixelFormat pixelFormat, bool multiplanar) > { > switch (pixelFormat.fourcc()) { > /* RGB formats. */ > -- > Regards, > > Laurent Pinchart > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Jacopo, On Thu, Mar 19, 2020 at 01:32:41PM +0100, Jacopo Mondi wrote: > On Tue, Mar 17, 2020 at 01:46:48AM +0200, Laurent Pinchart wrote: > > The V4L2PixelFormat class describes the pixel format of a V4L2 buffer. > > It wraps the V4L2 numerical FourCC, and shall be used in all APIs that > > deal with V4L2 pixel formats. Its purpose is to prevent unintentional > > confusion of V4L2 and DRM FourCCs in code by catching implicit > > conversion attempts at compile time. > > > > The constructor taking a V4L2 FourCC integer value will be made explicit > > in a further commit to minimize the size of this change and keep it > > reviewable. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > src/libcamera/include/v4l2_videodevice.h | 35 ++++++++++--- > > src/libcamera/v4l2_videodevice.cpp | 62 +++++++++++++++++++----- > > 2 files changed, 79 insertions(+), 18 deletions(-) > > > > diff --git a/src/libcamera/include/v4l2_videodevice.h b/src/libcamera/include/v4l2_videodevice.h > > index b3000f3c5133..49d2ca357efa 100644 > > --- a/src/libcamera/include/v4l2_videodevice.h > > +++ b/src/libcamera/include/v4l2_videodevice.h > > @@ -149,10 +149,31 @@ private: > > unsigned int missCounter_; > > }; > > > > +class V4L2PixelFormat > > +{ > > +public: > > + V4L2PixelFormat() > > + : fourcc_(0) > > + { > > + } > > + > > + V4L2PixelFormat(uint32_t fourcc) > > + : fourcc_(fourcc) > > + { > > + } > > + > > + bool isValid() const { return fourcc_ != 0; } > > + uint32_t value() const { return fourcc_; } > > s/value/fourcc ? I've considered that, I found pros and cons, and would have preferred using value() for PixelFormat too, but we have modifiers too there, so that's not an option. I'll change it to fourcc() here. > > + operator uint32_t() const { return fourcc_; } > > If I remove this I get errors in all pipelines when comparing > V4L2PixelFormat instances. Is this a shortcut to allow implicit cast > to uin32_t so that operator==() and operator!=() are happy ? Isn't it > better to defined those operator instead ? It's not just comparison, it also makes it possible to assign a V4L2PixelFormat to a uint32_t, as well as pass a V4L2PixelFormat to a function that expects a uint32_t. Please see the related discussions on the PixelFormat series. > > + > > +private: > > + uint32_t fourcc_; > > +}; > > + > > class V4L2DeviceFormat > > { > > public: > > - uint32_t fourcc; > > + V4L2PixelFormat fourcc; > > Size size; > > > > struct { > > @@ -184,7 +205,7 @@ public: > > > > int getFormat(V4L2DeviceFormat *format); > > int setFormat(V4L2DeviceFormat *format); > > - std::map<unsigned int, std::vector<SizeRange>> formats(); > > + std::map<V4L2PixelFormat, std::vector<SizeRange>> formats(); > > > > int setCrop(Rectangle *rect); > > int setCompose(Rectangle *rect); > > @@ -203,9 +224,9 @@ public: > > static V4L2VideoDevice *fromEntityName(const MediaDevice *media, > > const std::string &entity); > > > > - static PixelFormat toPixelFormat(uint32_t v4l2Fourcc); > > - uint32_t toV4L2Fourcc(PixelFormat pixelFormat); > > - static uint32_t toV4L2Fourcc(PixelFormat pixelFormat, bool multiplanar); > > + static PixelFormat toPixelFormat(V4L2PixelFormat v4l2Fourcc); > > + V4L2PixelFormat toV4L2Fourcc(PixelFormat pixelFormat); > > + static V4L2PixelFormat toV4L2Fourcc(PixelFormat pixelFormat, bool multiplanar); > > s/toV4L2PixelFormat ? > > to be consistent with the new class name and leave froucc out of the > API Good point, I'll fix that. > > > > protected: > > std::string logPrefix() const; > > @@ -220,8 +241,8 @@ private: > > int getFormatSingleplane(V4L2DeviceFormat *format); > > int setFormatSingleplane(V4L2DeviceFormat *format); > > > > - std::vector<unsigned int> enumPixelformats(); > > - std::vector<SizeRange> enumSizes(unsigned int pixelFormat); > > + std::vector<V4L2PixelFormat> enumPixelformats(); > > + std::vector<SizeRange> enumSizes(V4L2PixelFormat pixelFormat); > > > > int setSelection(unsigned int target, Rectangle *rect); > > > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp > > index 81911e764fde..40396c22aa45 100644 > > --- a/src/libcamera/v4l2_videodevice.cpp > > +++ b/src/libcamera/v4l2_videodevice.cpp > > @@ -277,6 +277,46 @@ bool V4L2BufferCache::Entry::operator==(const FrameBuffer &buffer) const > > return true; > > } > > > > +/** > > + * \class V4L2PixelFormat > > + * \brief V4L2 pixel format wrapper > > In documentation I would instead mention fourcc. Seems there is a bit > of confusion between fourcc and pixel format. Good idea, I'll fix that by writing "V4L2 pixel format FourCC wrapper". > To me v4l2 pixel format wraps a V4L2 fourcc code > Here v4l2 pixel format wraps a v4l2 pixel format :) > > > + * > > + * The V4L2PixelFormat class describes the pixel format of a V4L2 buffer. It > > + * wraps the V4L2 numerical FourCC, and shall be used in all APIs that deal with > > + * V4L2 pixel formats. Its purpose is to prevent unintentional confusion of > > + * V4L2 and DRM FourCCs in code by catching implicit conversion attempts at > > + * compile time. > > + */ > > + > > +/** > > + * \fn V4L2PixelFormat::V4L2PixelFormat() > > + * \brief Construct an invalid V4L2 pixel format with a numerical value of 0 > > Also here and below, the "numerical values" are fourcc codes, which > are indeed numerical values, but from a specific range :) > > All minors though > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > > + */ > > + > > +/** > > + * \fn V4L2PixelFormat::V4L2PixelFormat(uint32_t fourcc) > > + * \brief Construct a V4L2 pixel format from a FourCC value > > + * \param[in] fourcc The pixel format numerical value > > + */ > > + > > +/** > > + * \fn bool V4L2PixelFormat::isValid() const > > + * \brief Check if the pixel format is valid > > + * \return True if the pixel format has a non-zero value, false otherwise > > + */ > > + > > +/** > > + * \fn uint32_t V4L2PixelFormat::value() const > > + * \brief Retrieve the pixel format numerical value > > + * \return The pixel format numerical value > > + */ > > + > > +/** > > + * \fn V4L2PixelFormat::operator uint32_t() const > > + * \brief Convert the the pixel format numerical value > > + * \return The pixel format numerical value > > + */ > > + > > /** > > * \class V4L2DeviceFormat > > * \brief The V4L2 video device image format and sizes > > @@ -385,7 +425,7 @@ bool V4L2BufferCache::Entry::operator==(const FrameBuffer &buffer) const > > const std::string V4L2DeviceFormat::toString() const > > { > > std::stringstream ss; > > - ss << size.toString() << "-" << utils::hex(fourcc); > > + ss << size.toString() << "-" << utils::hex(fourcc.value()); > > return ss.str(); > > } > > > > @@ -844,11 +884,11 @@ int V4L2VideoDevice::setFormatSingleplane(V4L2DeviceFormat *format) > > * > > * \return A list of the supported video device formats > > */ > > -std::map<unsigned int, std::vector<SizeRange>> V4L2VideoDevice::formats() > > +std::map<V4L2PixelFormat, std::vector<SizeRange>> V4L2VideoDevice::formats() > > { > > - std::map<unsigned int, std::vector<SizeRange>> formats; > > + std::map<V4L2PixelFormat, std::vector<SizeRange>> formats; > > > > - for (unsigned int pixelformat : enumPixelformats()) { > > + for (V4L2PixelFormat pixelformat : enumPixelformats()) { > > std::vector<SizeRange> sizes = enumSizes(pixelformat); > > if (sizes.empty()) > > return {}; > > @@ -859,9 +899,9 @@ std::map<unsigned int, std::vector<SizeRange>> V4L2VideoDevice::formats() > > return formats; > > } > > > > -std::vector<unsigned int> V4L2VideoDevice::enumPixelformats() > > +std::vector<V4L2PixelFormat> V4L2VideoDevice::enumPixelformats() > > { > > - std::vector<unsigned int> formats; > > + std::vector<V4L2PixelFormat> formats; > > int ret; > > > > for (unsigned int index = 0; ; index++) { > > @@ -886,7 +926,7 @@ std::vector<unsigned int> V4L2VideoDevice::enumPixelformats() > > return formats; > > } > > > > -std::vector<SizeRange> V4L2VideoDevice::enumSizes(unsigned int pixelFormat) > > +std::vector<SizeRange> V4L2VideoDevice::enumSizes(V4L2PixelFormat pixelFormat) > > { > > std::vector<SizeRange> sizes; > > int ret; > > @@ -1417,7 +1457,7 @@ V4L2VideoDevice *V4L2VideoDevice::fromEntityName(const MediaDevice *media, > > * \param[in] v4l2Fourcc The V4L2 pixel format (V4L2_PIX_FORMAT_*) > > * \return The PixelFormat corresponding to \a v4l2Fourcc > > */ > > -PixelFormat V4L2VideoDevice::toPixelFormat(uint32_t v4l2Fourcc) > > +PixelFormat V4L2VideoDevice::toPixelFormat(V4L2PixelFormat v4l2Fourcc) > > { > > switch (v4l2Fourcc) { > > /* RGB formats. */ > > @@ -1466,7 +1506,7 @@ PixelFormat V4L2VideoDevice::toPixelFormat(uint32_t v4l2Fourcc) > > libcamera::_log(__FILE__, __LINE__, _LOG_CATEGORY(V4L2)(), > > LogError).stream() > > << "Unsupported V4L2 pixel format " > > - << utils::hex(v4l2Fourcc); > > + << utils::hex(v4l2Fourcc.value()); > > return PixelFormat(); > > } > > } > > @@ -1482,7 +1522,7 @@ PixelFormat V4L2VideoDevice::toPixelFormat(uint32_t v4l2Fourcc) > > * > > * \return The V4L2_PIX_FMT_* pixel format code corresponding to \a pixelFormat > > */ > > -uint32_t V4L2VideoDevice::toV4L2Fourcc(PixelFormat pixelFormat) > > +V4L2PixelFormat V4L2VideoDevice::toV4L2Fourcc(PixelFormat pixelFormat) > > { > > return V4L2VideoDevice::toV4L2Fourcc(pixelFormat, caps_.isMultiplanar()); > > } > > @@ -1500,7 +1540,7 @@ uint32_t V4L2VideoDevice::toV4L2Fourcc(PixelFormat pixelFormat) > > * > > * \return The V4L2_PIX_FMT_* pixel format code corresponding to \a pixelFormat > > */ > > -uint32_t V4L2VideoDevice::toV4L2Fourcc(PixelFormat pixelFormat, bool multiplanar) > > +V4L2PixelFormat V4L2VideoDevice::toV4L2Fourcc(PixelFormat pixelFormat, bool multiplanar) > > { > > switch (pixelFormat.fourcc()) { > > /* RGB formats. */
diff --git a/src/libcamera/include/v4l2_videodevice.h b/src/libcamera/include/v4l2_videodevice.h index b3000f3c5133..49d2ca357efa 100644 --- a/src/libcamera/include/v4l2_videodevice.h +++ b/src/libcamera/include/v4l2_videodevice.h @@ -149,10 +149,31 @@ private: unsigned int missCounter_; }; +class V4L2PixelFormat +{ +public: + V4L2PixelFormat() + : fourcc_(0) + { + } + + V4L2PixelFormat(uint32_t fourcc) + : fourcc_(fourcc) + { + } + + bool isValid() const { return fourcc_ != 0; } + uint32_t value() const { return fourcc_; } + operator uint32_t() const { return fourcc_; } + +private: + uint32_t fourcc_; +}; + class V4L2DeviceFormat { public: - uint32_t fourcc; + V4L2PixelFormat fourcc; Size size; struct { @@ -184,7 +205,7 @@ public: int getFormat(V4L2DeviceFormat *format); int setFormat(V4L2DeviceFormat *format); - std::map<unsigned int, std::vector<SizeRange>> formats(); + std::map<V4L2PixelFormat, std::vector<SizeRange>> formats(); int setCrop(Rectangle *rect); int setCompose(Rectangle *rect); @@ -203,9 +224,9 @@ public: static V4L2VideoDevice *fromEntityName(const MediaDevice *media, const std::string &entity); - static PixelFormat toPixelFormat(uint32_t v4l2Fourcc); - uint32_t toV4L2Fourcc(PixelFormat pixelFormat); - static uint32_t toV4L2Fourcc(PixelFormat pixelFormat, bool multiplanar); + static PixelFormat toPixelFormat(V4L2PixelFormat v4l2Fourcc); + V4L2PixelFormat toV4L2Fourcc(PixelFormat pixelFormat); + static V4L2PixelFormat toV4L2Fourcc(PixelFormat pixelFormat, bool multiplanar); protected: std::string logPrefix() const; @@ -220,8 +241,8 @@ private: int getFormatSingleplane(V4L2DeviceFormat *format); int setFormatSingleplane(V4L2DeviceFormat *format); - std::vector<unsigned int> enumPixelformats(); - std::vector<SizeRange> enumSizes(unsigned int pixelFormat); + std::vector<V4L2PixelFormat> enumPixelformats(); + std::vector<SizeRange> enumSizes(V4L2PixelFormat pixelFormat); int setSelection(unsigned int target, Rectangle *rect); diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp index 81911e764fde..40396c22aa45 100644 --- a/src/libcamera/v4l2_videodevice.cpp +++ b/src/libcamera/v4l2_videodevice.cpp @@ -277,6 +277,46 @@ bool V4L2BufferCache::Entry::operator==(const FrameBuffer &buffer) const return true; } +/** + * \class V4L2PixelFormat + * \brief V4L2 pixel format wrapper + * + * The V4L2PixelFormat class describes the pixel format of a V4L2 buffer. It + * wraps the V4L2 numerical FourCC, and shall be used in all APIs that deal with + * V4L2 pixel formats. Its purpose is to prevent unintentional confusion of + * V4L2 and DRM FourCCs in code by catching implicit conversion attempts at + * compile time. + */ + +/** + * \fn V4L2PixelFormat::V4L2PixelFormat() + * \brief Construct an invalid V4L2 pixel format with a numerical value of 0 + */ + +/** + * \fn V4L2PixelFormat::V4L2PixelFormat(uint32_t fourcc) + * \brief Construct a V4L2 pixel format from a FourCC value + * \param[in] fourcc The pixel format numerical value + */ + +/** + * \fn bool V4L2PixelFormat::isValid() const + * \brief Check if the pixel format is valid + * \return True if the pixel format has a non-zero value, false otherwise + */ + +/** + * \fn uint32_t V4L2PixelFormat::value() const + * \brief Retrieve the pixel format numerical value + * \return The pixel format numerical value + */ + +/** + * \fn V4L2PixelFormat::operator uint32_t() const + * \brief Convert the the pixel format numerical value + * \return The pixel format numerical value + */ + /** * \class V4L2DeviceFormat * \brief The V4L2 video device image format and sizes @@ -385,7 +425,7 @@ bool V4L2BufferCache::Entry::operator==(const FrameBuffer &buffer) const const std::string V4L2DeviceFormat::toString() const { std::stringstream ss; - ss << size.toString() << "-" << utils::hex(fourcc); + ss << size.toString() << "-" << utils::hex(fourcc.value()); return ss.str(); } @@ -844,11 +884,11 @@ int V4L2VideoDevice::setFormatSingleplane(V4L2DeviceFormat *format) * * \return A list of the supported video device formats */ -std::map<unsigned int, std::vector<SizeRange>> V4L2VideoDevice::formats() +std::map<V4L2PixelFormat, std::vector<SizeRange>> V4L2VideoDevice::formats() { - std::map<unsigned int, std::vector<SizeRange>> formats; + std::map<V4L2PixelFormat, std::vector<SizeRange>> formats; - for (unsigned int pixelformat : enumPixelformats()) { + for (V4L2PixelFormat pixelformat : enumPixelformats()) { std::vector<SizeRange> sizes = enumSizes(pixelformat); if (sizes.empty()) return {}; @@ -859,9 +899,9 @@ std::map<unsigned int, std::vector<SizeRange>> V4L2VideoDevice::formats() return formats; } -std::vector<unsigned int> V4L2VideoDevice::enumPixelformats() +std::vector<V4L2PixelFormat> V4L2VideoDevice::enumPixelformats() { - std::vector<unsigned int> formats; + std::vector<V4L2PixelFormat> formats; int ret; for (unsigned int index = 0; ; index++) { @@ -886,7 +926,7 @@ std::vector<unsigned int> V4L2VideoDevice::enumPixelformats() return formats; } -std::vector<SizeRange> V4L2VideoDevice::enumSizes(unsigned int pixelFormat) +std::vector<SizeRange> V4L2VideoDevice::enumSizes(V4L2PixelFormat pixelFormat) { std::vector<SizeRange> sizes; int ret; @@ -1417,7 +1457,7 @@ V4L2VideoDevice *V4L2VideoDevice::fromEntityName(const MediaDevice *media, * \param[in] v4l2Fourcc The V4L2 pixel format (V4L2_PIX_FORMAT_*) * \return The PixelFormat corresponding to \a v4l2Fourcc */ -PixelFormat V4L2VideoDevice::toPixelFormat(uint32_t v4l2Fourcc) +PixelFormat V4L2VideoDevice::toPixelFormat(V4L2PixelFormat v4l2Fourcc) { switch (v4l2Fourcc) { /* RGB formats. */ @@ -1466,7 +1506,7 @@ PixelFormat V4L2VideoDevice::toPixelFormat(uint32_t v4l2Fourcc) libcamera::_log(__FILE__, __LINE__, _LOG_CATEGORY(V4L2)(), LogError).stream() << "Unsupported V4L2 pixel format " - << utils::hex(v4l2Fourcc); + << utils::hex(v4l2Fourcc.value()); return PixelFormat(); } } @@ -1482,7 +1522,7 @@ PixelFormat V4L2VideoDevice::toPixelFormat(uint32_t v4l2Fourcc) * * \return The V4L2_PIX_FMT_* pixel format code corresponding to \a pixelFormat */ -uint32_t V4L2VideoDevice::toV4L2Fourcc(PixelFormat pixelFormat) +V4L2PixelFormat V4L2VideoDevice::toV4L2Fourcc(PixelFormat pixelFormat) { return V4L2VideoDevice::toV4L2Fourcc(pixelFormat, caps_.isMultiplanar()); } @@ -1500,7 +1540,7 @@ uint32_t V4L2VideoDevice::toV4L2Fourcc(PixelFormat pixelFormat) * * \return The V4L2_PIX_FMT_* pixel format code corresponding to \a pixelFormat */ -uint32_t V4L2VideoDevice::toV4L2Fourcc(PixelFormat pixelFormat, bool multiplanar) +V4L2PixelFormat V4L2VideoDevice::toV4L2Fourcc(PixelFormat pixelFormat, bool multiplanar) { switch (pixelFormat.fourcc()) { /* RGB formats. */
The V4L2PixelFormat class describes the pixel format of a V4L2 buffer. It wraps the V4L2 numerical FourCC, and shall be used in all APIs that deal with V4L2 pixel formats. Its purpose is to prevent unintentional confusion of V4L2 and DRM FourCCs in code by catching implicit conversion attempts at compile time. The constructor taking a V4L2 FourCC integer value will be made explicit in a further commit to minimize the size of this change and keep it reviewable. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- src/libcamera/include/v4l2_videodevice.h | 35 ++++++++++--- src/libcamera/v4l2_videodevice.cpp | 62 +++++++++++++++++++----- 2 files changed, 79 insertions(+), 18 deletions(-)