[libcamera-devel] [PATCH 1/2] libcamera: v4l2_videodevice: Add V4L2PixelFormat class
Jacopo Mondi
jacopo at jmondi.org
Thu Mar 19 13:32:41 CET 2020
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 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 ?
> + 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 at 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 at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
More information about the libcamera-devel
mailing list