[libcamera-devel] [PATCH 2/2] libcamera: v4l2_videodevice: Make V4L2PixelFormat constructor explicit
Niklas Söderlund
niklas.soderlund at ragnatech.se
Thu Mar 19 01:04:14 CET 2020
Hi Laurent,
Thanks for your work.
On 2020-03-17 01:46:49 +0200, Laurent Pinchart wrote:
> To achieve the goal of preventing unwanted conversion between a DRM and
> a V4L2 FourCC, make the V4L2PixelFormat constructor that takes an
> integer value explicit. All users of V4L2 pixel formats flagged by the
> compiler are fixed.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
> src/libcamera/include/v4l2_videodevice.h | 2 +-
> src/libcamera/pipeline/ipu3/ipu3.cpp | 14 +++----
> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 4 +-
> src/libcamera/pipeline/vimc.cpp | 2 +-
> src/libcamera/v4l2_videodevice.cpp | 42 ++++++++++---------
> .../v4l2_videodevice_test.cpp | 2 +-
> 6 files changed, 35 insertions(+), 31 deletions(-)
>
> diff --git a/src/libcamera/include/v4l2_videodevice.h b/src/libcamera/include/v4l2_videodevice.h
> index 49d2ca357efa..9a123ce8c50e 100644
> --- a/src/libcamera/include/v4l2_videodevice.h
> +++ b/src/libcamera/include/v4l2_videodevice.h
> @@ -157,7 +157,7 @@ public:
> {
> }
>
> - V4L2PixelFormat(uint32_t fourcc)
> + explicit V4L2PixelFormat(uint32_t fourcc)
> : fourcc_(fourcc)
> {
> }
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 90de7749f623..123b184023c3 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -121,7 +121,7 @@ public:
> int start();
> int stop();
>
> - static int mediaBusToFormat(unsigned int code);
> + static V4L2PixelFormat mediaBusToFormat(unsigned int code);
>
> V4L2VideoDevice *output_;
> V4L2Subdevice *csi2_;
> @@ -1456,19 +1456,19 @@ int CIO2Device::stop()
> return output_->streamOff();
> }
>
> -int CIO2Device::mediaBusToFormat(unsigned int code)
> +V4L2PixelFormat CIO2Device::mediaBusToFormat(unsigned int code)
> {
> switch (code) {
> case MEDIA_BUS_FMT_SBGGR10_1X10:
> - return V4L2_PIX_FMT_IPU3_SBGGR10;
> + return V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SBGGR10);
> case MEDIA_BUS_FMT_SGBRG10_1X10:
> - return V4L2_PIX_FMT_IPU3_SGBRG10;
> + return V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGBRG10);
> case MEDIA_BUS_FMT_SGRBG10_1X10:
> - return V4L2_PIX_FMT_IPU3_SGRBG10;
> + return V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGRBG10);
> case MEDIA_BUS_FMT_SRGGB10_1X10:
> - return V4L2_PIX_FMT_IPU3_SRGGB10;
> + return V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SRGGB10);
> default:
> - return -EINVAL;
> + return {};
> }
> }
>
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index beed38956662..96ab8ea45931 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -645,13 +645,13 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)
> }
>
> V4L2DeviceFormat paramFormat = {};
> - paramFormat.fourcc = V4L2_META_FMT_RK_ISP1_PARAMS;
> + paramFormat.fourcc = V4L2PixelFormat(V4L2_META_FMT_RK_ISP1_PARAMS);
> ret = param_->setFormat(¶mFormat);
> if (ret)
> return ret;
>
> V4L2DeviceFormat statFormat = {};
> - statFormat.fourcc = V4L2_META_FMT_RK_ISP1_STAT_3A;
> + statFormat.fourcc = V4L2PixelFormat(V4L2_META_FMT_RK_ISP1_STAT_3A);
> ret = stat_->setFormat(&statFormat);
> if (ret)
> return ret;
> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp
> index eedef85866a1..83ffb5f16efa 100644
> --- a/src/libcamera/pipeline/vimc.cpp
> +++ b/src/libcamera/pipeline/vimc.cpp
> @@ -247,7 +247,7 @@ int PipelineHandlerVimc::configure(Camera *camera, CameraConfiguration *config)
> * Format has to be set on the raw capture video node, otherwise the
> * vimc driver will fail pipeline validation.
> */
> - format.fourcc = V4L2_PIX_FMT_SGRBG8;
> + format.fourcc = V4L2PixelFormat(V4L2_PIX_FMT_SGRBG8);
> format.size = { cfg.size.width / 3, cfg.size.height / 3 };
>
> ret = data->raw_->setFormat(&format);
> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> index 40396c22aa45..6f59487593ae 100644
> --- a/src/libcamera/v4l2_videodevice.cpp
> +++ b/src/libcamera/v4l2_videodevice.cpp
> @@ -286,6 +286,10 @@ bool V4L2BufferCache::Entry::operator==(const FrameBuffer &buffer) const
> * 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.
> + *
> + * To achieve this goal, construction of a V4L2PixelFormat from an integer value
> + * is explicit. To retrieve the integer value of a V4L2PixelFormat, both the
> + * explicit value() and implicit uint32_t conversion operators may be used.
> */
>
> /**
> @@ -719,7 +723,7 @@ int V4L2VideoDevice::getFormatMeta(V4L2DeviceFormat *format)
>
> format->size.width = 0;
> format->size.height = 0;
> - format->fourcc = pix->dataformat;
> + format->fourcc = V4L2PixelFormat(pix->dataformat);
> format->planesCount = 1;
> format->planes[0].bpl = pix->buffersize;
> format->planes[0].size = pix->buffersize;
> @@ -771,7 +775,7 @@ int V4L2VideoDevice::getFormatMultiplane(V4L2DeviceFormat *format)
>
> format->size.width = pix->width;
> format->size.height = pix->height;
> - format->fourcc = pix->pixelformat;
> + format->fourcc = V4L2PixelFormat(pix->pixelformat);
> format->planesCount = pix->num_planes;
>
> for (unsigned int i = 0; i < format->planesCount; ++i) {
> @@ -812,7 +816,7 @@ int V4L2VideoDevice::setFormatMultiplane(V4L2DeviceFormat *format)
> */
> format->size.width = pix->width;
> format->size.height = pix->height;
> - format->fourcc = pix->pixelformat;
> + format->fourcc = V4L2PixelFormat(pix->pixelformat);
> format->planesCount = pix->num_planes;
> for (unsigned int i = 0; i < format->planesCount; ++i) {
> format->planes[i].bpl = pix->plane_fmt[i].bytesperline;
> @@ -837,7 +841,7 @@ int V4L2VideoDevice::getFormatSingleplane(V4L2DeviceFormat *format)
>
> format->size.width = pix->width;
> format->size.height = pix->height;
> - format->fourcc = pix->pixelformat;
> + format->fourcc = V4L2PixelFormat(pix->pixelformat);
> format->planesCount = 1;
> format->planes[0].bpl = pix->bytesperline;
> format->planes[0].size = pix->sizeimage;
> @@ -869,7 +873,7 @@ int V4L2VideoDevice::setFormatSingleplane(V4L2DeviceFormat *format)
> */
> format->size.width = pix->width;
> format->size.height = pix->height;
> - format->fourcc = pix->pixelformat;
> + format->fourcc = V4L2PixelFormat(pix->pixelformat);
> format->planesCount = 1;
> format->planes[0].bpl = pix->bytesperline;
> format->planes[0].size = pix->sizeimage;
> @@ -913,7 +917,7 @@ std::vector<V4L2PixelFormat> V4L2VideoDevice::enumPixelformats()
> if (ret)
> break;
>
> - formats.push_back(pixelformatEnum.pixelformat);
> + formats.push_back(V4L2PixelFormat(pixelformatEnum.pixelformat));
> }
>
> if (ret && ret != -EINVAL) {
> @@ -1545,21 +1549,21 @@ V4L2PixelFormat V4L2VideoDevice::toV4L2Fourcc(PixelFormat pixelFormat, bool mult
> switch (pixelFormat.fourcc()) {
> /* RGB formats. */
> case DRM_FORMAT_BGR888:
> - return V4L2_PIX_FMT_RGB24;
> + return V4L2PixelFormat(V4L2_PIX_FMT_RGB24);
> case DRM_FORMAT_RGB888:
> - return V4L2_PIX_FMT_BGR24;
> + return V4L2PixelFormat(V4L2_PIX_FMT_BGR24);
> case DRM_FORMAT_BGRA8888:
> - return V4L2_PIX_FMT_ARGB32;
> + return V4L2PixelFormat(V4L2_PIX_FMT_ARGB32);
>
> /* YUV packed formats. */
> case DRM_FORMAT_YUYV:
> - return V4L2_PIX_FMT_YUYV;
> + return V4L2PixelFormat(V4L2_PIX_FMT_YUYV);
> case DRM_FORMAT_YVYU:
> - return V4L2_PIX_FMT_YVYU;
> + return V4L2PixelFormat(V4L2_PIX_FMT_YVYU);
> case DRM_FORMAT_UYVY:
> - return V4L2_PIX_FMT_UYVY;
> + return V4L2PixelFormat(V4L2_PIX_FMT_UYVY);
> case DRM_FORMAT_VYUY:
> - return V4L2_PIX_FMT_VYUY;
> + return V4L2PixelFormat(V4L2_PIX_FMT_VYUY);
>
> /*
> * YUY planar formats.
> @@ -1568,17 +1572,17 @@ V4L2PixelFormat V4L2VideoDevice::toV4L2Fourcc(PixelFormat pixelFormat, bool mult
> * also take into account the formats supported by the device.
> */
> case DRM_FORMAT_NV16:
> - return V4L2_PIX_FMT_NV16;
> + return V4L2PixelFormat(V4L2_PIX_FMT_NV16);
> case DRM_FORMAT_NV61:
> - return V4L2_PIX_FMT_NV61;
> + return V4L2PixelFormat(V4L2_PIX_FMT_NV61);
> case DRM_FORMAT_NV12:
> - return V4L2_PIX_FMT_NV12;
> + return V4L2PixelFormat(V4L2_PIX_FMT_NV12);
> case DRM_FORMAT_NV21:
> - return V4L2_PIX_FMT_NV21;
> + return V4L2PixelFormat(V4L2_PIX_FMT_NV21);
>
> /* Compressed formats. */
> case DRM_FORMAT_MJPEG:
> - return V4L2_PIX_FMT_MJPEG;
> + return V4L2PixelFormat(V4L2_PIX_FMT_MJPEG);
> }
>
> /*
> @@ -1587,7 +1591,7 @@ V4L2PixelFormat V4L2VideoDevice::toV4L2Fourcc(PixelFormat pixelFormat, bool mult
> */
> libcamera::_log(__FILE__, __LINE__, _LOG_CATEGORY(V4L2)(), LogError).stream()
> << "Unsupported V4L2 pixel format " << pixelFormat.toString();
> - return 0;
> + return {};
> }
>
> /**
> diff --git a/test/v4l2_videodevice/v4l2_videodevice_test.cpp b/test/v4l2_videodevice/v4l2_videodevice_test.cpp
> index 577da4cb601c..93b9e72da5b4 100644
> --- a/test/v4l2_videodevice/v4l2_videodevice_test.cpp
> +++ b/test/v4l2_videodevice/v4l2_videodevice_test.cpp
> @@ -69,7 +69,7 @@ int V4L2VideoDeviceTest::init()
> if (debayer_->open())
> return TestFail;
>
> - format.fourcc = V4L2_PIX_FMT_SBGGR8;
> + format.fourcc = V4L2PixelFormat(V4L2_PIX_FMT_SBGGR8);
>
> V4L2SubdeviceFormat subformat = {};
> subformat.mbus_code = MEDIA_BUS_FMT_SBGGR8_1X8;
> --
> Regards,
>
> Laurent Pinchart
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list