[libcamera-devel] [PATCH 1/2] libcamera: v4l2_videodevice: Add V4L2PixelFormat class
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Mar 19 13:56:11 CET 2020
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 at 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 at 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. */
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list