[libcamera-devel] [PATCH v2 03/27] libcamera: v4l2_videodevice: Drop toV4L2PixelFormat()
Kieran Bingham
kieran.bingham at ideasonboard.com
Mon Sep 6 12:39:50 CEST 2021
Hi Laurent,
On 06/09/2021 03:00, Laurent Pinchart wrote:
> The V4L2VideoDevice::toV4L2PixelFormat() function is incorrectly
> implemented, as it will pick a multi-planar format if the device
> supports the multi-planar API, even if only single-planar formats are
> supported. This currently works because the implementation calls
> V4L2PixelFormat::fromPixelFormat(), which ignores the multiplanar
> argument and always returns a single-planar format.
>
> Fixing this isn't trivial. As we don't need to support multi-planar V4L2
> formats at this point, drop the function instead of pretending
> everything is fine, and call V4L2PixelFormat::fromPixelFormat() directly
> from pipeline handlers. As the single-planar case is the most common,
> set the multiplanar argument to false by default to avoid long lines.
Seems fine to me.
Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
> include/libcamera/internal/v4l2_pixelformat.h | 2 +-
> include/libcamera/internal/v4l2_videodevice.h | 2 --
> src/libcamera/pipeline/ipu3/imgu.cpp | 2 +-
> .../pipeline/raspberrypi/raspberrypi.cpp | 8 ++++----
> src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 6 +++---
> src/libcamera/pipeline/simple/converter.cpp | 8 ++++----
> src/libcamera/pipeline/simple/simple.cpp | 4 ++--
> src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 6 +++---
> src/libcamera/pipeline/vimc/vimc.cpp | 8 ++++----
> src/libcamera/v4l2_videodevice.cpp | 17 -----------------
> test/libtest/buffer_source.cpp | 3 +--
> 11 files changed, 23 insertions(+), 43 deletions(-)
>
> diff --git a/include/libcamera/internal/v4l2_pixelformat.h b/include/libcamera/internal/v4l2_pixelformat.h
> index 9bfd81ad6651..560c5c53c0c3 100644
> --- a/include/libcamera/internal/v4l2_pixelformat.h
> +++ b/include/libcamera/internal/v4l2_pixelformat.h
> @@ -38,7 +38,7 @@ public:
>
> PixelFormat toPixelFormat() const;
> static V4L2PixelFormat fromPixelFormat(const PixelFormat &pixelFormat,
> - bool multiplanar);
> + bool multiplanar = false);
>
> private:
> uint32_t fourcc_;
> diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h
> index 7a145f608a5b..087ad067e37e 100644
> --- a/include/libcamera/internal/v4l2_videodevice.h
> +++ b/include/libcamera/internal/v4l2_videodevice.h
> @@ -212,8 +212,6 @@ public:
> static std::unique_ptr<V4L2VideoDevice>
> fromEntityName(const MediaDevice *media, const std::string &entity);
>
> - V4L2PixelFormat toV4L2PixelFormat(const PixelFormat &pixelFormat);
> -
> protected:
> std::string logPrefix() const override;
>
> diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp
> index 317e482a1498..3e1ef645ec93 100644
> --- a/src/libcamera/pipeline/ipu3/imgu.cpp
> +++ b/src/libcamera/pipeline/ipu3/imgu.cpp
> @@ -575,7 +575,7 @@ int ImgUDevice::configureVideoDevice(V4L2VideoDevice *dev, unsigned int pad,
> return 0;
>
> *outputFormat = {};
> - outputFormat->fourcc = dev->toV4L2PixelFormat(formats::NV12);
> + outputFormat->fourcc = V4L2PixelFormat::fromPixelFormat(formats::NV12);
> outputFormat->size = cfg.size;
> outputFormat->planesCount = 2;
>
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index b2674ac02109..0bdfa7273ce0 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -440,14 +440,14 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()
>
> V4L2VideoDevice::Formats fmts = dev->formats();
>
> - if (fmts.find(V4L2PixelFormat::fromPixelFormat(cfgPixFmt, false)) == fmts.end()) {
> + if (fmts.find(V4L2PixelFormat::fromPixelFormat(cfgPixFmt)) == fmts.end()) {
> /* If we cannot find a native format, use a default one. */
> cfgPixFmt = formats::NV12;
> status = Adjusted;
> }
>
> V4L2DeviceFormat format;
> - format.fourcc = dev->toV4L2PixelFormat(cfg.pixelFormat);
> + format.fourcc = V4L2PixelFormat::fromPixelFormat(cfg.pixelFormat);
> format.size = cfg.size;
>
> int ret = dev->tryFormat(&format);
> @@ -647,7 +647,7 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
> RPi::Stream *stream = i == maxIndex ? &data->isp_[Isp::Output0]
> : &data->isp_[Isp::Output1];
>
> - V4L2PixelFormat fourcc = stream->dev()->toV4L2PixelFormat(cfg.pixelFormat);
> + V4L2PixelFormat fourcc = V4L2PixelFormat::fromPixelFormat(cfg.pixelFormat);
> format.size = cfg.size;
> format.fourcc = fourcc;
>
> @@ -688,7 +688,7 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
> maxSize = Size(320, 240);
> format = {};
> format.size = maxSize;
> - format.fourcc = V4L2PixelFormat::fromPixelFormat(formats::YUV420, false);
> + format.fourcc = V4L2PixelFormat::fromPixelFormat(formats::YUV420);
> ret = data->isp_[Isp::Output0].dev()->setFormat(&format);
> if (ret) {
> LOG(RPI, Error)
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> index 25f482eb8d8e..f8d471204d2e 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> @@ -80,7 +80,7 @@ CameraConfiguration::Status RkISP1Path::validate(StreamConfiguration *cfg)
> cfg->bufferCount = RKISP1_BUFFER_COUNT;
>
> V4L2DeviceFormat format;
> - format.fourcc = video_->toV4L2PixelFormat(cfg->pixelFormat);
> + format.fourcc = V4L2PixelFormat::fromPixelFormat(cfg->pixelFormat);
> format.size = cfg->size;
>
> int ret = video_->tryFormat(&format);
> @@ -146,7 +146,7 @@ int RkISP1Path::configure(const StreamConfiguration &config,
>
> const PixelFormatInfo &info = PixelFormatInfo::info(config.pixelFormat);
> V4L2DeviceFormat outputFormat;
> - outputFormat.fourcc = video_->toV4L2PixelFormat(config.pixelFormat);
> + outputFormat.fourcc = V4L2PixelFormat::fromPixelFormat(config.pixelFormat);
> outputFormat.size = config.size;
> outputFormat.planesCount = info.numPlanes();
>
> @@ -155,7 +155,7 @@ int RkISP1Path::configure(const StreamConfiguration &config,
> return ret;
>
> if (outputFormat.size != config.size ||
> - outputFormat.fourcc != video_->toV4L2PixelFormat(config.pixelFormat)) {
> + outputFormat.fourcc != V4L2PixelFormat::fromPixelFormat(config.pixelFormat)) {
> LOG(RkISP1, Error)
> << "Unable to configure capture in " << config.toString();
> return -EINVAL;
> diff --git a/src/libcamera/pipeline/simple/converter.cpp b/src/libcamera/pipeline/simple/converter.cpp
> index b5e34c4cd0c5..9cbc6ee30ce4 100644
> --- a/src/libcamera/pipeline/simple/converter.cpp
> +++ b/src/libcamera/pipeline/simple/converter.cpp
> @@ -46,7 +46,7 @@ int SimpleConverter::Stream::configure(const StreamConfiguration &inputCfg,
> const StreamConfiguration &outputCfg)
> {
> V4L2PixelFormat videoFormat =
> - m2m_->output()->toV4L2PixelFormat(inputCfg.pixelFormat);
> + V4L2PixelFormat::fromPixelFormat(inputCfg.pixelFormat);
>
> V4L2DeviceFormat format;
> format.fourcc = videoFormat;
> @@ -71,7 +71,7 @@ int SimpleConverter::Stream::configure(const StreamConfiguration &inputCfg,
> }
>
> /* Set the pixel format and size on the output. */
> - videoFormat = m2m_->capture()->toV4L2PixelFormat(outputCfg.pixelFormat);
> + videoFormat = V4L2PixelFormat::fromPixelFormat(outputCfg.pixelFormat);
> format = {};
> format.fourcc = videoFormat;
> format.size = outputCfg.size;
> @@ -210,7 +210,7 @@ std::vector<PixelFormat> SimpleConverter::formats(PixelFormat input)
> * enumerate the conversion capabilities on its output (V4L2 capture).
> */
> V4L2DeviceFormat v4l2Format;
> - v4l2Format.fourcc = m2m_->output()->toV4L2PixelFormat(input);
> + v4l2Format.fourcc = V4L2PixelFormat::fromPixelFormat(input);
> v4l2Format.size = { 1, 1 };
>
> int ret = m2m_->output()->setFormat(&v4l2Format);
> @@ -281,7 +281,7 @@ SimpleConverter::strideAndFrameSize(const PixelFormat &pixelFormat,
> const Size &size)
> {
> V4L2DeviceFormat format;
> - format.fourcc = m2m_->capture()->toV4L2PixelFormat(pixelFormat);
> + format.fourcc = V4L2PixelFormat::fromPixelFormat(pixelFormat);
> format.size = size;
>
> int ret = m2m_->capture()->tryFormat(&format);
> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> index cadaf5d030ab..701fb4be0b71 100644
> --- a/src/libcamera/pipeline/simple/simple.cpp
> +++ b/src/libcamera/pipeline/simple/simple.cpp
> @@ -826,7 +826,7 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate()
> return Invalid;
> } else {
> V4L2DeviceFormat format;
> - format.fourcc = data_->video_->toV4L2PixelFormat(cfg.pixelFormat);
> + format.fourcc = V4L2PixelFormat::fromPixelFormat(cfg.pixelFormat);
> format.size = cfg.size;
>
> int ret = data_->video_->tryFormat(&format);
> @@ -915,7 +915,7 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)
> return ret;
>
> /* Configure the video node. */
> - V4L2PixelFormat videoFormat = video->toV4L2PixelFormat(pipeConfig->captureFormat);
> + V4L2PixelFormat videoFormat = V4L2PixelFormat::fromPixelFormat(pipeConfig->captureFormat);
>
> V4L2DeviceFormat captureFormat;
> captureFormat.fourcc = videoFormat;
> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> index 973ecd5b835e..264f5370cf32 100644
> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> @@ -151,7 +151,7 @@ CameraConfiguration::Status UVCCameraConfiguration::validate()
> cfg.bufferCount = 4;
>
> V4L2DeviceFormat format;
> - format.fourcc = data_->video_->toV4L2PixelFormat(cfg.pixelFormat);
> + format.fourcc = V4L2PixelFormat::fromPixelFormat(cfg.pixelFormat);
> format.size = cfg.size;
>
> int ret = data_->video_->tryFormat(&format);
> @@ -207,7 +207,7 @@ int PipelineHandlerUVC::configure(Camera *camera, CameraConfiguration *config)
> int ret;
>
> V4L2DeviceFormat format;
> - format.fourcc = data->video_->toV4L2PixelFormat(cfg.pixelFormat);
> + format.fourcc = V4L2PixelFormat::fromPixelFormat(cfg.pixelFormat);
> format.size = cfg.size;
>
> ret = data->video_->setFormat(&format);
> @@ -215,7 +215,7 @@ int PipelineHandlerUVC::configure(Camera *camera, CameraConfiguration *config)
> return ret;
>
> if (format.size != cfg.size ||
> - format.fourcc != data->video_->toV4L2PixelFormat(cfg.pixelFormat))
> + format.fourcc != V4L2PixelFormat::fromPixelFormat(cfg.pixelFormat))
> return -EINVAL;
>
> cfg.setStream(&data->stream_);
> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp
> index baeb6a7e6fa6..e453091da4b2 100644
> --- a/src/libcamera/pipeline/vimc/vimc.cpp
> +++ b/src/libcamera/pipeline/vimc/vimc.cpp
> @@ -170,7 +170,7 @@ CameraConfiguration::Status VimcCameraConfiguration::validate()
> cfg.bufferCount = 4;
>
> V4L2DeviceFormat format;
> - format.fourcc = data_->video_->toV4L2PixelFormat(cfg.pixelFormat);
> + format.fourcc = V4L2PixelFormat::fromPixelFormat(cfg.pixelFormat);
> format.size = cfg.size;
>
> int ret = data_->video_->tryFormat(&format);
> @@ -274,7 +274,7 @@ int PipelineHandlerVimc::configure(Camera *camera, CameraConfiguration *config)
> return ret;
>
> V4L2DeviceFormat format;
> - format.fourcc = data->video_->toV4L2PixelFormat(cfg.pixelFormat);
> + format.fourcc = V4L2PixelFormat::fromPixelFormat(cfg.pixelFormat);
> format.size = cfg.size;
>
> ret = data->video_->setFormat(&format);
> @@ -282,7 +282,7 @@ int PipelineHandlerVimc::configure(Camera *camera, CameraConfiguration *config)
> return ret;
>
> if (format.size != cfg.size ||
> - format.fourcc != data->video_->toV4L2PixelFormat(cfg.pixelFormat))
> + format.fourcc != V4L2PixelFormat::fromPixelFormat(cfg.pixelFormat))
> return -EINVAL;
>
> /*
> @@ -597,7 +597,7 @@ int VimcCameraData::allocateMockIPABuffers()
> constexpr unsigned int kBufCount = 2;
>
> V4L2DeviceFormat format;
> - format.fourcc = video_->toV4L2PixelFormat(formats::BGR888);
> + format.fourcc = V4L2PixelFormat::fromPixelFormat(formats::BGR888);
> format.size = Size (160, 120);
>
> int ret = video_->setFormat(&format);
> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> index 4e1c2b7cef5e..84ccb97495f5 100644
> --- a/src/libcamera/v4l2_videodevice.cpp
> +++ b/src/libcamera/v4l2_videodevice.cpp
> @@ -1687,23 +1687,6 @@ V4L2VideoDevice::fromEntityName(const MediaDevice *media,
> return std::make_unique<V4L2VideoDevice>(mediaEntity);
> }
>
> -/**
> - * \brief Convert \a PixelFormat to its corresponding V4L2 FourCC
> - * \param[in] pixelFormat The PixelFormat to convert
> - *
> - * For multiplanar formats, the V4L2 format variant (contiguous or
> - * non-contiguous planes) is selected automatically based on the capabilities
> - * of the video device. If the video device supports the V4L2 multiplanar API,
> - * non-contiguous formats are preferred.
> - *
> - * \return The V4L2_PIX_FMT_* pixel format code corresponding to \a pixelFormat
> - */
> -V4L2PixelFormat V4L2VideoDevice::toV4L2PixelFormat(const PixelFormat &pixelFormat)
> -{
> - return V4L2PixelFormat::fromPixelFormat(pixelFormat,
> - caps_.isMultiplanar());
> -}
> -
> /**
> * \class V4L2M2MDevice
> * \brief Memory-to-Memory video device
> diff --git a/test/libtest/buffer_source.cpp b/test/libtest/buffer_source.cpp
> index 73563f2fc39d..64e7376ad575 100644
> --- a/test/libtest/buffer_source.cpp
> +++ b/test/libtest/buffer_source.cpp
> @@ -70,8 +70,7 @@ int BufferSource::allocate(const StreamConfiguration &config)
> }
>
> format.size = config.size;
> - format.fourcc = V4L2PixelFormat::fromPixelFormat(config.pixelFormat,
> - false);
> + format.fourcc = V4L2PixelFormat::fromPixelFormat(config.pixelFormat);
> if (video->setFormat(&format)) {
> std::cout << "Failed to set format on output device" << std::endl;
> return TestFail;
>
More information about the libcamera-devel
mailing list