[libcamera-devel] [PATCH v2 03/27] libcamera: v4l2_videodevice: Drop toV4L2PixelFormat()
Hirokazu Honda
hiroh at chromium.org
Mon Sep 6 14:32:04 CEST 2021
Hi Laurent, thank you for the patch.
On Mon, Sep 6, 2021 at 7:39 PM Kieran Bingham
<kieran.bingham at ideasonboard.com> wrote:
>
> 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>
Reviewed-by: Hirokazu Honda <hiroh at chromium.org>
> > ---
> > 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