[libcamera-devel] [PATCH 2/2] libcamera: pipeline: simple: converter: Use generic converter interface
Xavier Roumegue (OSS)
xavier.roumegue at oss.nxp.com
Tue Nov 15 11:15:18 CET 2022
Hi Jacopo,
On 11/9/22 08:54, Jacopo Mondi wrote:
> Hi Xavier
>
> On Mon, Oct 10, 2022 at 03:17:44PM +0200, Xavier Roumegue (OSS) wrote:
>> From: Xavier Roumegue <xavier.roumegue at oss.nxp.com>
>>
>> Move the simple converter implementation to a generic V4L2 M2M class
>> derived from the converter interface. This latter could be used by
>> other pipeline implementations and as base class for customized V4L2 M2M
>> converters.
>>
>> Signed-off-by: Xavier Roumegue <xavier.roumegue at oss.nxp.com>
>> ---
>> .../internal/converter/converter_v4l2_m2m.h | 18 +--
>> .../libcamera/internal/converter/meson.build | 5 +
>> include/libcamera/internal/meson.build | 2 +
>> .../converter_v4l2_m2m.cpp} | 149 +++++++++++-------
>> src/libcamera/converter/meson.build | 5 +
>> src/libcamera/meson.build | 1 +
>> src/libcamera/pipeline/simple/meson.build | 1 -
>> src/libcamera/pipeline/simple/simple.cpp | 6 +-
>> 8 files changed, 119 insertions(+), 68 deletions(-)
>> rename src/libcamera/pipeline/simple/converter.h => include/libcamera/internal/converter/converter_v4l2_m2m.h (83%)
>> create mode 100644 include/libcamera/internal/converter/meson.build
>> rename src/libcamera/{pipeline/simple/converter.cpp => converter/converter_v4l2_m2m.cpp} (69%)
>> create mode 100644 src/libcamera/converter/meson.build
>>
>> diff --git a/src/libcamera/pipeline/simple/converter.h b/include/libcamera/internal/converter/converter_v4l2_m2m.h
>> similarity index 83%
>> rename from src/libcamera/pipeline/simple/converter.h
>> rename to include/libcamera/internal/converter/converter_v4l2_m2m.h
>> index f0ebe2e0..ef31eeba 100644
>> --- a/src/libcamera/pipeline/simple/converter.h
>> +++ b/include/libcamera/internal/converter/converter_v4l2_m2m.h
>> @@ -1,8 +1,9 @@
>> /* SPDX-License-Identifier: LGPL-2.1-or-later */
>> /*
>> * Copyright (C) 2020, Laurent Pinchart
>> + * Copyright 2022 NXP
>> *
>> - * converter.h - Format converter for simple pipeline handler
>> + * converter_v4l2_m2m.h - V4l2 M2M Format converter interface
>> */
>>
>> #pragma once
>> @@ -19,6 +20,8 @@
>> #include <libcamera/base/log.h>
>> #include <libcamera/base/signal.h>
>>
>> +#include "libcamera/internal/converter.h"
>> +
>> namespace libcamera {
>>
>> class FrameBuffer;
>> @@ -28,11 +31,12 @@ class SizeRange;
>> struct StreamConfiguration;
>> class V4L2M2MDevice;
>>
>> -class SimpleConverter
>> +class V4L2M2MConverter : public Converter
>> {
>> public:
>> - SimpleConverter(MediaDevice *media);
>> + V4L2M2MConverter(MediaDevice *media);
>>
>> + int loadConfiguration([[maybe_unused]] const std::string &filename) { return 0; }
>
> How do you envision this function to be called ?
Should be called from the pipeline handlers as the configuration file depends on
the pipeline handler.
> If it is only your derived class that calls this, should this be part
> of the abstract base class ?
>
>> bool isValid() const { return m2m_ != nullptr; }
>>
>> std::vector<PixelFormat> formats(PixelFormat input);
>> @@ -52,14 +56,11 @@ public:
>> int queueBuffers(FrameBuffer *input,
>> const std::map<unsigned int, FrameBuffer *> &outputs);
>>
>> - Signal<FrameBuffer *> inputBufferReady;
>> - Signal<FrameBuffer *> outputBufferReady;
>> -
>> private:
>> class Stream : protected Loggable
>> {
>> public:
>> - Stream(SimpleConverter *converter, unsigned int index);
>> + Stream(V4L2M2MConverter *converter, unsigned int index);
>>
>> bool isValid() const { return m2m_ != nullptr; }
>>
>> @@ -80,7 +81,7 @@ private:
>> void captureBufferReady(FrameBuffer *buffer);
>> void outputBufferReady(FrameBuffer *buffer);
>>
>> - SimpleConverter *converter_;
>> + V4L2M2MConverter *converter_;
>> unsigned int index_;
>> std::unique_ptr<V4L2M2MDevice> m2m_;
>>
>> @@ -88,7 +89,6 @@ private:
>> unsigned int outputBufferCount_;
>> };
>>
>> - std::string deviceNode_;
>> std::unique_ptr<V4L2M2MDevice> m2m_;
>>
>> std::vector<Stream> streams_;
>> diff --git a/include/libcamera/internal/converter/meson.build b/include/libcamera/internal/converter/meson.build
>> new file mode 100644
>> index 00000000..891e79e7
>> --- /dev/null
>> +++ b/include/libcamera/internal/converter/meson.build
>> @@ -0,0 +1,5 @@
>> +# SPDX-License-Identifier: CC0-1.0
>> +
>> +libcamera_internal_headers += files([
>> + 'converter_v4l2_m2m.h',
>> +])
>> diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build
>> index 8f50d755..b9db5a8c 100644
>> --- a/include/libcamera/internal/meson.build
>> +++ b/include/libcamera/internal/meson.build
>> @@ -45,3 +45,5 @@ libcamera_internal_headers = files([
>> 'v4l2_videodevice.h',
>> 'yaml_parser.h',
>> ])
>> +
>> +subdir('converter')
>> diff --git a/src/libcamera/pipeline/simple/converter.cpp b/src/libcamera/converter/converter_v4l2_m2m.cpp
>> similarity index 69%
>> rename from src/libcamera/pipeline/simple/converter.cpp
>> rename to src/libcamera/converter/converter_v4l2_m2m.cpp
>> index acaaa64c..5af054be 100644
>> --- a/src/libcamera/pipeline/simple/converter.cpp
>> +++ b/src/libcamera/converter/converter_v4l2_m2m.cpp
>> @@ -1,12 +1,11 @@
>> /* SPDX-License-Identifier: LGPL-2.1-or-later */
>> /*
>> * Copyright (C) 2020, Laurent Pinchart
>> + * Copyright 2022 NXP
>> *
>> - * converter.cpp - Format converter for simple pipeline handler
>> + * converter_v4l2_m2m.cpp - V4L2 M2M Format converter
>> */
>>
>> -#include "converter.h"
>> -
>> #include <algorithm>
>> #include <limits.h>
>>
>> @@ -20,19 +19,32 @@
>>
>> #include "libcamera/internal/media_device.h"
>> #include "libcamera/internal/v4l2_videodevice.h"
>> +#include "libcamera/internal/converter/converter_v4l2_m2m.h"
>
> Shouldn't this be included first ?
>
>> +
>> +/**
>> + * \file internal/converter/converter_v4l2_m2m.h
>> + * \brief V4L2 M2M based converter
>> + */
>>
>> namespace libcamera {
>>
>> -LOG_DECLARE_CATEGORY(SimplePipeline)
>> +LOG_DECLARE_CATEGORY(Converter)
>> +
>> +/**
>> + * \class V4L2M2MConverter
>> + * \brief V4L2 M2M based converter
>> + *
>> + * The V4L2 M2M converter implements the converter interface based on V4L2 M2M device.
>> + */
>>
>> /* -----------------------------------------------------------------------------
>> - * SimpleConverter::Stream
>> + * V4L2M2MConverter::Stream
>> */
>>
>> -SimpleConverter::Stream::Stream(SimpleConverter *converter, unsigned int index)
>> +V4L2M2MConverter::Stream::Stream(V4L2M2MConverter *converter, unsigned int index)
>> : converter_(converter), index_(index)
>> {
>> - m2m_ = std::make_unique<V4L2M2MDevice>(converter->deviceNode_);
>> + m2m_ = std::make_unique<V4L2M2MDevice>(converter->deviceNode());
>
> isValid() simply checks for m2m_ != nullptr.
> The m2m video dev might fail at open() time.
> I would use a valid_ flag, to be set to true at the end of the
> constructor
>
>>
>> m2m_->output()->bufferReady.connect(this, &Stream::outputBufferReady);
>> m2m_->capture()->bufferReady.connect(this, &Stream::captureBufferReady);
>> @@ -42,8 +54,8 @@ SimpleConverter::Stream::Stream(SimpleConverter *converter, unsigned int index)
>> m2m_.reset();
>> }
>>
>> -int SimpleConverter::Stream::configure(const StreamConfiguration &inputCfg,
>> - const StreamConfiguration &outputCfg)
>> +int V4L2M2MConverter::Stream::configure(const StreamConfiguration &inputCfg,
>> + const StreamConfiguration &outputCfg)
>> {
>> V4L2PixelFormat videoFormat =
>> m2m_->output()->toV4L2PixelFormat(inputCfg.pixelFormat);
>> @@ -56,14 +68,14 @@ int SimpleConverter::Stream::configure(const StreamConfiguration &inputCfg,
>>
>> int ret = m2m_->output()->setFormat(&format);
>> if (ret < 0) {
>> - LOG(SimplePipeline, Error)
>> + LOG(Converter, Error)
>> << "Failed to set input format: " << strerror(-ret);
>> return ret;
>> }
>>
>> if (format.fourcc != videoFormat || format.size != inputCfg.size ||
>> format.planes[0].bpl != inputCfg.stride) {
>> - LOG(SimplePipeline, Error)
>> + LOG(Converter, Error)
>> << "Input format not supported (requested "
>> << inputCfg.size << "-" << videoFormat
>> << ", got " << format << ")";
>> @@ -78,13 +90,13 @@ int SimpleConverter::Stream::configure(const StreamConfiguration &inputCfg,
>>
>> ret = m2m_->capture()->setFormat(&format);
>> if (ret < 0) {
>> - LOG(SimplePipeline, Error)
>> + LOG(Converter, Error)
>> << "Failed to set output format: " << strerror(-ret);
>> return ret;
>> }
>>
>> if (format.fourcc != videoFormat || format.size != outputCfg.size) {
>> - LOG(SimplePipeline, Error)
>> + LOG(Converter, Error)
>> << "Output format not supported";
>> return -EINVAL;
>> }
>> @@ -95,13 +107,13 @@ int SimpleConverter::Stream::configure(const StreamConfiguration &inputCfg,
>> return 0;
>> }
>>
>> -int SimpleConverter::Stream::exportBuffers(unsigned int count,
>> - std::vector<std::unique_ptr<FrameBuffer>> *buffers)
>> +int V4L2M2MConverter::Stream::exportBuffers(unsigned int count,
>> + std::vector<std::unique_ptr<FrameBuffer>> *buffers)
>> {
>> return m2m_->capture()->exportBuffers(count, buffers);
>> }
>>
>> -int SimpleConverter::Stream::start()
>> +int V4L2M2MConverter::Stream::start()
>> {
>> int ret = m2m_->output()->importBuffers(inputBufferCount_);
>> if (ret < 0)
>> @@ -128,7 +140,7 @@ int SimpleConverter::Stream::start()
>> return 0;
>> }
>>
>> -void SimpleConverter::Stream::stop()
>> +void V4L2M2MConverter::Stream::stop()
>> {
>> m2m_->capture()->streamOff();
>> m2m_->output()->streamOff();
>> @@ -136,8 +148,7 @@ void SimpleConverter::Stream::stop()
>> m2m_->output()->releaseBuffers();
>> }
>>
>> -int SimpleConverter::Stream::queueBuffers(FrameBuffer *input,
>> - FrameBuffer *output)
>> +int V4L2M2MConverter::Stream::queueBuffers(FrameBuffer *input, FrameBuffer *output)
>> {
>> int ret = m2m_->output()->queueBuffer(input);
>> if (ret < 0)
>> @@ -150,12 +161,12 @@ int SimpleConverter::Stream::queueBuffers(FrameBuffer *input,
>> return 0;
>> }
>>
>> -std::string SimpleConverter::Stream::logPrefix() const
>> +std::string V4L2M2MConverter::Stream::logPrefix() const
>> {
>> return "stream" + std::to_string(index_);
>> }
>>
>> -void SimpleConverter::Stream::outputBufferReady(FrameBuffer *buffer)
>> +void V4L2M2MConverter::Stream::outputBufferReady(FrameBuffer *buffer)
>> {
>> auto it = converter_->queue_.find(buffer);
>> if (it == converter_->queue_.end())
>> @@ -167,32 +178,26 @@ void SimpleConverter::Stream::outputBufferReady(FrameBuffer *buffer)
>> }
>> }
>>
>> -void SimpleConverter::Stream::captureBufferReady(FrameBuffer *buffer)
>> +void V4L2M2MConverter::Stream::captureBufferReady(FrameBuffer *buffer)
>> {
>> converter_->outputBufferReady.emit(buffer);
>> }
>>
>> /* -----------------------------------------------------------------------------
>> - * SimpleConverter
>> + * V4L2M2MConverter
>> */
>>
>> -SimpleConverter::SimpleConverter(MediaDevice *media)
>> +/**
>> + * \brief Construct a V4L2M2MConverter instance
>> + * \param[in] media The media device implementing the converter
>> + */
>> +V4L2M2MConverter::V4L2M2MConverter(MediaDevice *media)
>> + : Converter(media)
>> {
>> - /*
>> - * Locate the video node. There's no need to validate the pipeline
>> - * further, the caller guarantees that this is a V4L2 mem2mem device.
>> - */
>> - const std::vector<MediaEntity *> &entities = media->entities();
>> - auto it = std::find_if(entities.begin(), entities.end(),
>> - [](MediaEntity *entity) {
>> - return entity->function() == MEDIA_ENT_F_IO_V4L;
>> - });
>> - if (it == entities.end())
>> + if (deviceNode().empty())
>> return;
>>
>> - deviceNode_ = (*it)->deviceNode();
>> -
>> - m2m_ = std::make_unique<V4L2M2MDevice>(deviceNode_);
>> + m2m_ = std::make_unique<V4L2M2MDevice>(deviceNode());
>> int ret = m2m_->open();
>> if (ret < 0) {
>> m2m_.reset();
>> @@ -200,7 +205,18 @@ SimpleConverter::SimpleConverter(MediaDevice *media)
>> }
>> }
>>
>> -std::vector<PixelFormat> SimpleConverter::formats(PixelFormat input)
>> +/**
>> + * \copydoc libcamera::Converter::loadConfiguration
>> + */
>> +
>> +/**
>> + * \copydoc libcamera::Converter::isValid
>> + */
>> +
>> +/**
>> + * \copydoc libcamera::Converter::formats
>> + */
>> +std::vector<PixelFormat> V4L2M2MConverter::formats(PixelFormat input)
>> {
>> if (!m2m_)
>> return {};
>> @@ -215,13 +231,13 @@ std::vector<PixelFormat> SimpleConverter::formats(PixelFormat input)
>>
>> int ret = m2m_->output()->setFormat(&v4l2Format);
>> if (ret < 0) {
>> - LOG(SimplePipeline, Error)
>> + LOG(Converter, Error)
>> << "Failed to set format: " << strerror(-ret);
>> return {};
>> }
>>
>> if (v4l2Format.fourcc != m2m_->output()->toV4L2PixelFormat(input)) {
>> - LOG(SimplePipeline, Debug)
>> + LOG(Converter, Debug)
>> << "Input format " << input << " not supported.";
>> return {};
>> }
>> @@ -237,7 +253,10 @@ std::vector<PixelFormat> SimpleConverter::formats(PixelFormat input)
>> return pixelFormats;
>> }
>>
>> -SizeRange SimpleConverter::sizes(const Size &input)
>> +/**
>> + * \copydoc libcamera::Converter::sizes
>> + */
>> +SizeRange V4L2M2MConverter::sizes(const Size &input)
>> {
>> if (!m2m_)
>> return {};
>> @@ -252,7 +271,7 @@ SizeRange SimpleConverter::sizes(const Size &input)
>>
>> int ret = m2m_->output()->setFormat(&format);
>> if (ret < 0) {
>> - LOG(SimplePipeline, Error)
>> + LOG(Converter, Error)
>> << "Failed to set format: " << strerror(-ret);
>> return {};
>> }
>> @@ -262,7 +281,7 @@ SizeRange SimpleConverter::sizes(const Size &input)
>> format.size = { 1, 1 };
>> ret = m2m_->capture()->setFormat(&format);
>> if (ret < 0) {
>> - LOG(SimplePipeline, Error)
>> + LOG(Converter, Error)
>> << "Failed to set format: " << strerror(-ret);
>> return {};
>> }
>> @@ -272,7 +291,7 @@ SizeRange SimpleConverter::sizes(const Size &input)
>> format.size = { UINT_MAX, UINT_MAX };
>> ret = m2m_->capture()->setFormat(&format);
>> if (ret < 0) {
>> - LOG(SimplePipeline, Error)
>> + LOG(Converter, Error)
>> << "Failed to set format: " << strerror(-ret);
>> return {};
>> }
>> @@ -282,9 +301,12 @@ SizeRange SimpleConverter::sizes(const Size &input)
>> return sizes;
>> }
>>
>> +/**
>> + * \copydoc libcamera::Converter::strideAndFrameSize
>> + */
>> std::tuple<unsigned int, unsigned int>
>> -SimpleConverter::strideAndFrameSize(const PixelFormat &pixelFormat,
>> - const Size &size)
>> +V4L2M2MConverter::strideAndFrameSize(const PixelFormat &pixelFormat,
>> + const Size &size)
>> {
>> V4L2DeviceFormat format;
>> format.fourcc = m2m_->capture()->toV4L2PixelFormat(pixelFormat);
>> @@ -297,8 +319,11 @@ SimpleConverter::strideAndFrameSize(const PixelFormat &pixelFormat,
>> return std::make_tuple(format.planes[0].bpl, format.planes[0].size);
>> }
>>
>> -int SimpleConverter::configure(const StreamConfiguration &inputCfg,
>> - const std::vector<std::reference_wrapper<StreamConfiguration>> &outputCfgs)
>> +/**
>> + * \copydoc libcamera::Converter::configure
>> + */
>> +int V4L2M2MConverter::configure(const StreamConfiguration &inputCfg,
>> + const std::vector<std::reference_wrapper<StreamConfiguration>> &outputCfgs)
>> {
>> int ret = 0;
>>
>> @@ -309,7 +334,7 @@ int SimpleConverter::configure(const StreamConfiguration &inputCfg,
>> Stream &stream = streams_.emplace_back(this, i);
>>
>> if (!stream.isValid()) {
>> - LOG(SimplePipeline, Error)
>> + LOG(Converter, Error)
>> << "Failed to create stream " << i;
>> ret = -EINVAL;
>> break;
>> @@ -328,8 +353,11 @@ int SimpleConverter::configure(const StreamConfiguration &inputCfg,
>> return 0;
>> }
>>
>> -int SimpleConverter::exportBuffers(unsigned int output, unsigned int count,
>> - std::vector<std::unique_ptr<FrameBuffer>> *buffers)
>> +/**
>> + * \copydoc libcamera::Converter::exportBuffers
>> + */
>> +int V4L2M2MConverter::exportBuffers(unsigned int output, unsigned int count,
>> + std::vector<std::unique_ptr<FrameBuffer>> *buffers)
>> {
>> if (output >= streams_.size())
>> return -EINVAL;
>> @@ -337,7 +365,10 @@ int SimpleConverter::exportBuffers(unsigned int output, unsigned int count,
>> return streams_[output].exportBuffers(count, buffers);
>> }
>>
>> -int SimpleConverter::start()
>> +/**
>> + * \copydoc libcamera::Converter::start
>> + */
>> +int V4L2M2MConverter::start()
>> {
>> int ret;
>>
>> @@ -352,14 +383,20 @@ int SimpleConverter::start()
>> return 0;
>> }
>>
>> -void SimpleConverter::stop()
>> +/**
>> + * \copydoc libcamera::Converter::stop
>> + */
>> +void V4L2M2MConverter::stop()
>> {
>> for (Stream &stream : utils::reverse(streams_))
>> stream.stop();
>> }
>>
>> -int SimpleConverter::queueBuffers(FrameBuffer *input,
>> - const std::map<unsigned int, FrameBuffer *> &outputs)
>> +/**
>> + * \copydoc libcamera::Converter::queueBuffers
>> + */
>> +int V4L2M2MConverter::queueBuffers(FrameBuffer *input,
>> + const std::map<unsigned int, FrameBuffer *> &outputs)
>> {
>> unsigned int mask = 0;
>> int ret;
>> @@ -402,4 +439,6 @@ int SimpleConverter::queueBuffers(FrameBuffer *input,
>> return 0;
>> }
>>
>> +REGISTER_CONVERTER("v4l2_m2m", V4L2M2MConverter, "pxp")
>> +
>> } /* namespace libcamera */
>> diff --git a/src/libcamera/converter/meson.build b/src/libcamera/converter/meson.build
>> new file mode 100644
>> index 00000000..2aa72fe4
>> --- /dev/null
>> +++ b/src/libcamera/converter/meson.build
>> @@ -0,0 +1,5 @@
>> +# SPDX-License-Identifier: CC0-1.0
>> +
>> +libcamera_sources += files([
>> + 'converter_v4l2_m2m.cpp'
>> +])
>> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
>> index 3a9fc31f..d2f75741 100644
>> --- a/src/libcamera/meson.build
>> +++ b/src/libcamera/meson.build
>> @@ -62,6 +62,7 @@ libatomic = cc.find_library('atomic', required : false)
>> libthreads = dependency('threads')
>>
>> subdir('base')
>> +subdir('converter')
>> subdir('ipa')
>> subdir('pipeline')
>> subdir('proxy')
>> diff --git a/src/libcamera/pipeline/simple/meson.build b/src/libcamera/pipeline/simple/meson.build
>> index 9c99b32f..42b0896d 100644
>> --- a/src/libcamera/pipeline/simple/meson.build
>> +++ b/src/libcamera/pipeline/simple/meson.build
>> @@ -1,6 +1,5 @@
>> # SPDX-License-Identifier: CC0-1.0
>>
>> libcamera_sources += files([
>> - 'converter.cpp',
>> 'simple.cpp',
>> ])
>> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
>> index 37b3e7ac..5cc6bc6a 100644
>> --- a/src/libcamera/pipeline/simple/simple.cpp
>> +++ b/src/libcamera/pipeline/simple/simple.cpp
>> @@ -30,13 +30,13 @@
>>
>> #include "libcamera/internal/camera.h"
>> #include "libcamera/internal/camera_sensor.h"
>> +#include "libcamera/internal/converter.h"
>> #include "libcamera/internal/device_enumerator.h"
>> #include "libcamera/internal/media_device.h"
>> #include "libcamera/internal/pipeline_handler.h"
>> #include "libcamera/internal/v4l2_subdevice.h"
>> #include "libcamera/internal/v4l2_videodevice.h"
>>
>> -#include "converter.h"
>>
>> namespace libcamera {
>>
>> @@ -266,7 +266,7 @@ public:
>> std::vector<Configuration> configs_;
>> std::map<PixelFormat, std::vector<const Configuration *>> formats_;
>>
>> - std::unique_ptr<SimpleConverter> converter_;
>> + std::unique_ptr<Converter> converter_;
>> std::vector<std::unique_ptr<FrameBuffer>> converterBuffers_;
>> bool useConverter_;
>> std::queue<std::map<unsigned int, FrameBuffer *>> converterQueue_;
>> @@ -492,7 +492,7 @@ int SimpleCameraData::init()
>> /* Open the converter, if any. */
>> MediaDevice *converter = pipe->converter();
>> if (converter) {
>> - converter_ = std::make_unique<SimpleConverter>(converter);
>> + converter_ = ConverterFactoryBase::create(converter);
>> if (!converter_->isValid()) {
>> LOG(SimplePipeline, Warning)
>> << "Failed to create converter, disabling format conversion";
>
> All minors:
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
>
> Thanks
> j
>
>> --
>> 2.37.3
>>
More information about the libcamera-devel
mailing list