[libcamera-devel] [PATCH v4 1/2] libcamera: Declare generic converter interface
Kieran Bingham
kieran.bingham at ideasonboard.com
Wed Dec 14 15:23:45 CET 2022
Quoting Xavier Roumegue (OSS) (2022-12-14 14:11:49)
> Hi Kieran,
>
> On 12/14/22 13:05, Kieran Bingham wrote:
> > Hi Xavier,
> >
> > Hrm ... sorry - doing my merge checks a few more issues came up.
> >
> > Could you add the checkstyle script as a post-commit hook please?
> >
> > cp utils/hooks/post-commit .git/hooks/post-commit
> >
> > Then you'll see these warnings yourself as you develop.
> >
> >
> >> ---------------------------------------------------------------------------------------
> >> 56fbecabbb548c8031914ef53ffb73306a9a745d libcamera: Declare generic converter interface
> >> ---------------------------------------------------------------------------------------
> >> --- include/libcamera/internal/converter.h
> >> +++ include/libcamera/internal/converter.h
> >> @@ -8,8 +8,8 @@
> >>
> >> #pragma once
> >>
> >> +#include <functional>
> >> #include <initializer_list>
> >> -#include <functional>
> >> #include <map>
> >> #include <memory>
> >> #include <string>
> >> ---
> >> 1 potential issue detected, please review
> >
> >
> > I think that one was pre-existing. We could
> > A) Ignore it, as this is just a code move
> > B) Fix it before moving it
> > C) Fix it after moving it.
> Went for C) with the fix as part of the move. ok ?
Certainly! Thanks for tackling it.
> >
> >
> >> ----------------------------------------------------------------------------------------------------------------
> >> 5c32397540488420daf1b8d4b11e26d936a5e2ee libcamera: pipeline: simple: converter: Use generic converter interface
> >> ----------------------------------------------------------------------------------------------------------------
> >> Header include/libcamera/internal/converter/converter_v4l2_m2m.h added without corresponding update to include/libcamera/internal/converter/meson.build
> >
> >
> > This one is a blocker to merge. We must keep the headers tracked in
> > meson, as that's how compile time dependencies are handled.
> The meson file is actually added, not updated. This rather sounds as a
> bug in utils/checkstyle.sh
>
> With the following patch, this quiets the check
>
> diff --git a/utils/checkstyle.py b/utils/checkstyle.py
> index f0248d65..a11d95cc 100755
> --- a/utils/checkstyle.py
> +++ b/utils/checkstyle.py
> @@ -313,7 +313,7 @@ class HeaderAddChecker(CommitChecker):
> def check(cls, commit, top_level):
> issues = []
>
> - meson_files = [f for f in commit.files('M')
> + meson_files = [f for f in commit.files()
> if os.path.basename(f) == 'meson.build']
>
> for filename in commit.files('AR'):
>
aha, sorry I'd missed that it was added there. Sounds like a fixup to
checkstyle indeed, but I haven't looked at how commit.files() is being
parsed, so I suspect that's a separate patch for Laurent to review.
> >
> >
> >
> >> --- include/libcamera/internal/converter/converter_v4l2_m2m.h
> >> +++ include/libcamera/internal/converter/converter_v4l2_m2m.h
> >> @@ -15,10 +15,10 @@
> >> #include <tuple>
> >> #include <vector>
> >>
> >> -#include <libcamera/pixel_format.h>
> >> -
> >> #include <libcamera/base/log.h>
> >> #include <libcamera/base/signal.h>
> >> +
> >> +#include <libcamera/pixel_format.h>
> >>
> >> #include "libcamera/internal/converter.h"
> >
> >
> > That one looks trivial.
> >
> >
> >> --- src/libcamera/converter/converter_v4l2_m2m.cpp
> >> +++ src/libcamera/converter/converter_v4l2_m2m.cpp
> >> @@ -19,7 +21,6 @@
> >>
> >> #include "libcamera/internal/media_device.h"
> >> #include "libcamera/internal/v4l2_videodevice.h"
> >> -#include "libcamera/internal/converter/converter_v4l2_m2m.h"
> >>
> >
> > This looks like clang-format moving the line, but checkstyle not quite
> > realising where it went...
> >
> > You can run clang-format directly to see what it did I think.
> Ok, applied the clang-format fix inplace
Great.
> >
> >
> >
> >
> >> /**
> >> * \file internal/converter/converter_v4l2_m2m.h
> >> ---
> >> 3 potential issues detected, please review
> >>
> >> --------------------------------------------------------------------------------------
> >> 4d65923f5190407ebbeb6144fe04a0226432b7ce libcamera: coverter: Fix clang-11 build issue
> >> --------------------------------------------------------------------------------------
> >> No issue detected
> >
> >
> > With those and the clang-11 ; issue I think we can merge this series.
> > --
> > Kieran
> >
> >
> >
> > Quoting Xavier Roumegue (2022-12-14 10:34:42)
> >> Declare a converter Abstract Base Class intended to provide generic
> >> interfaces to hardware offering size and format conversion services on
> >> streams. This is mainly based on the public interfaces of the current
> >> converter class implementation found in the simple pipeline handler.
> >>
> >> The main change is the introduction of loadConfiguration() function
> >> which can be used by the concrete implementation to load hardware
> >> specific runtime parameters defined by the application.
> >>
> >> Signed-off-by: Xavier Roumegue <xavier.roumegue at oss.nxp.com>
> >> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> >> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> >> ---
> >> include/libcamera/internal/converter.h | 108 ++++++++
> >> include/libcamera/internal/meson.build | 1 +
> >> src/libcamera/converter.cpp | 332 +++++++++++++++++++++++++
> >> src/libcamera/meson.build | 1 +
> >> 4 files changed, 442 insertions(+)
> >> create mode 100644 include/libcamera/internal/converter.h
> >> create mode 100644 src/libcamera/converter.cpp
> >>
> >> diff --git a/include/libcamera/internal/converter.h b/include/libcamera/internal/converter.h
> >> new file mode 100644
> >> index 00000000..ca2e6846
> >> --- /dev/null
> >> +++ b/include/libcamera/internal/converter.h
> >> @@ -0,0 +1,108 @@
> >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> >> +/*
> >> + * Copyright (C) 2020, Laurent Pinchart
> >> + * Copyright 2022 NXP
> >> + *
> >> + * converter.h - Generic format converter interface
> >> + */
> >> +
> >> +#pragma once
> >> +
> >> +#include <initializer_list>
> >> +#include <functional>
> >> +#include <map>
> >> +#include <memory>
> >> +#include <string>
> >> +#include <tuple>
> >> +#include <vector>
> >> +
> >> +#include <libcamera/base/class.h>
> >> +#include <libcamera/base/signal.h>
> >> +
> >> +#include <libcamera/geometry.h>
> >> +
> >> +namespace libcamera {
> >> +
> >> +class FrameBuffer;
> >> +class MediaDevice;
> >> +class PixelFormat;
> >> +struct StreamConfiguration;
> >> +
> >> +class Converter
> >> +{
> >> +public:
> >> + Converter(MediaDevice *media);
> >> + virtual ~Converter();
> >> +
> >> + virtual int loadConfiguration(const std::string &filename) = 0;
> >> +
> >> + virtual bool isValid() const = 0;
> >> +
> >> + virtual std::vector<PixelFormat> formats(PixelFormat input) = 0;
> >> + virtual SizeRange sizes(const Size &input) = 0;
> >> +
> >> + virtual std::tuple<unsigned int, unsigned int>
> >> + strideAndFrameSize(const PixelFormat &pixelFormat, const Size &size) = 0;
> >> +
> >> + virtual int configure(const StreamConfiguration &inputCfg,
> >> + const std::vector<std::reference_wrapper<StreamConfiguration>> &outputCfgs) = 0;
> >> + virtual int exportBuffers(unsigned int output, unsigned int count,
> >> + std::vector<std::unique_ptr<FrameBuffer>> *buffers) = 0;
> >> +
> >> + virtual int start() = 0;
> >> + virtual void stop() = 0;
> >> +
> >> + virtual int queueBuffers(FrameBuffer *input,
> >> + const std::map<unsigned int, FrameBuffer *> &outputs) = 0;
> >> +
> >> + Signal<FrameBuffer *> inputBufferReady;
> >> + Signal<FrameBuffer *> outputBufferReady;
> >> +
> >> + const std::string &deviceNode() const { return deviceNode_; };
> >> +
> >> +private:
> >> + std::string deviceNode_;
> >> +};
> >> +
> >> +class ConverterFactoryBase
> >> +{
> >> +public:
> >> + ConverterFactoryBase(const std::string name, std::initializer_list<std::string> compatibles);
> >> + virtual ~ConverterFactoryBase() = default;
> >> +
> >> + const std::vector<std::string> &compatibles() const { return compatibles_; }
> >> +
> >> + static std::unique_ptr<Converter> create(MediaDevice *media);
> >> + static std::vector<ConverterFactoryBase *> &factories();
> >> + static std::vector<std::string> names();
> >> +
> >> +private:
> >> + LIBCAMERA_DISABLE_COPY_AND_MOVE(ConverterFactoryBase)
> >> +
> >> + static void registerType(ConverterFactoryBase *factory);
> >> +
> >> + virtual std::unique_ptr<Converter> createInstance(MediaDevice *media) const = 0;
> >> +
> >> + std::string name_;
> >> + std::vector<std::string> compatibles_;
> >> +};
> >> +
> >> +template<typename _Converter>
> >> +class ConverterFactory : public ConverterFactoryBase
> >> +{
> >> +public:
> >> + ConverterFactory(const char *name, std::initializer_list<std::string> compatibles)
> >> + : ConverterFactoryBase(name, compatibles)
> >> + {
> >> + }
> >> +
> >> + std::unique_ptr<Converter> createInstance(MediaDevice *media) const override
> >> + {
> >> + return std::make_unique<_Converter>(media);
> >> + }
> >> +};
> >> +
> >> +#define REGISTER_CONVERTER(name, converter, compatibles) \
> >> + static ConverterFactory<converter> global_##converter##Factory(name, compatibles);
> >> +
> >> +} /* namespace libcamera */
> >> diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build
> >> index f8be86e0..341af8a2 100644
> >> --- a/include/libcamera/internal/meson.build
> >> +++ b/include/libcamera/internal/meson.build
> >> @@ -19,6 +19,7 @@ libcamera_internal_headers = files([
> >> 'camera_sensor_properties.h',
> >> 'control_serializer.h',
> >> 'control_validator.h',
> >> + 'converter.h',
> >> 'delayed_controls.h',
> >> 'device_enumerator.h',
> >> 'device_enumerator_sysfs.h',
> >> diff --git a/src/libcamera/converter.cpp b/src/libcamera/converter.cpp
> >> new file mode 100644
> >> index 00000000..3de39cff
> >> --- /dev/null
> >> +++ b/src/libcamera/converter.cpp
> >> @@ -0,0 +1,332 @@
> >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> >> +/*
> >> + * Copyright 2022 NXP
> >> + *
> >> + * converter.cpp - Generic format converter interface
> >> + */
> >> +
> >> +#include "libcamera/internal/converter.h"
> >> +
> >> +#include <algorithm>
> >> +
> >> +#include <libcamera/base/log.h>
> >> +
> >> +#include "libcamera/internal/media_device.h"
> >> +
> >> +#include "linux/media.h"
> >> +
> >> +/**
> >> + * \file internal/converter.h
> >> + * \brief Abstract converter
> >> + */
> >> +
> >> +namespace libcamera {
> >> +
> >> +LOG_DEFINE_CATEGORY(Converter)
> >> +
> >> +/**
> >> + * \class Converter
> >> + * \brief Abstract Base Class for converter
> >> + *
> >> + * The Converter class is an Abstract Base Class defining the interfaces of
> >> + * converter implementations.
> >> + *
> >> + * Converters offer scaling and pixel format conversion services on an input
> >> + * stream. The converter can output multiple streams with individual conversion
> >> + * parameters from the same input stream.
> >> + */
> >> +
> >> +/**
> >> + * \brief Construct a Converter instance
> >> + * \param[in] media The media device implementing the converter
> >> + *
> >> + * This searches for the entity implementing the data streaming function in the
> >> + * media graph entities and use its device node as the converter device node.
> >> + */
> >> +Converter::Converter(MediaDevice *media)
> >> +{
> >> + 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()) {
> >> + LOG(Converter, Error)
> >> + << "No entity suitable for implementing a converter in "
> >> + << media->driver() << " entities list.";
> >> + return;
> >> + }
> >> +
> >> + deviceNode_ = (*it)->deviceNode();
> >> +}
> >> +
> >> +Converter::~Converter()
> >> +{
> >> +}
> >> +
> >> +/**
> >> + * \fn Converter::loadConfiguration()
> >> + * \brief Load converter configuration from file
> >> + * \param[in] filename The file name path
> >> + *
> >> + * Load converter dependent configuration parameters to apply on the hardware.
> >> + *
> >> + * \return 0 on success or a negative error code otherwise
> >> + */
> >> +
> >> +/**
> >> + * \fn Converter::isValid()
> >> + * \brief Check if the converter configuration is valid
> >> + * \return True is the converter is valid, false otherwise
> >> + */
> >> +
> >> +/**
> >> + * \fn Converter::formats()
> >> + * \brief Retrieve the list of supported pixel formats for an input pixel format
> >> + * \param[in] input Input pixel format to retrieve output pixel format list for
> >> + * \return The list of supported output pixel formats
> >> + */
> >> +
> >> +/**
> >> + * \fn Converter::sizes()
> >> + * \brief Retrieve the range of minimum and maximum output sizes for an input size
> >> + * \param[in] input Input stream size to retrieve range for
> >> + * \return A range of output image sizes
> >> + */
> >> +
> >> +/**
> >> + * \fn Converter::strideAndFrameSize()
> >> + * \brief Retrieve the output stride and frame size for an input configutation
> >> + * \param[in] pixelFormat Input stream pixel format
> >> + * \param[in] size Input stream size
> >> + * \return A tuple indicating the stride and frame size or an empty tuple on error
> >> + */
> >> +
> >> +/**
> >> + * \fn Converter::configure()
> >> + * \brief Configure a set of output stream conversion from an input stream
> >> + * \param[in] inputCfg Input stream configuration
> >> + * \param[out] outputCfgs A list of output stream configurations
> >> + * \return 0 on success or a negative error code otherwise
> >> + */
> >> +
> >> +/**
> >> + * \fn Converter::exportBuffers()
> >> + * \brief Export buffers from the converter device
> >> + * \param[in] output Output stream index exporting the buffers
> >> + * \param[in] count Number of buffers to allocate
> >> + * \param[out] buffers Vector to store allocated buffers
> >> + *
> >> + * This function operates similarly to V4L2VideoDevice::exportBuffers() on the
> >> + * output stream indicated by the \a output index.
> >> + *
> >> + * \return The number of allocated buffers on success or a negative error code
> >> + * otherwise
> >> + */
> >> +
> >> +/**
> >> + * \fn Converter::start()
> >> + * \brief Start the converter streaming operation
> >> + * \return 0 on success or a negative error code otherwise
> >> + */
> >> +
> >> +/**
> >> + * \fn Converter::stop()
> >> + * \brief Stop the converter streaming operation
> >> + */
> >> +
> >> +/**
> >> + * \fn Converter::queueBuffers()
> >> + * \brief Queue buffers to converter device
> >> + * \param[in] input The frame buffer to apply the conversion
> >> + * \param[out] outputs The container holding the output stream indexes and
> >> + * their respective frame buffer outputs.
> >> + *
> >> + * This function queues the \a input frame buffer on the output streams of the
> >> + * \a outputs map key and retrieve the output frame buffer indicated by the
> >> + * buffer map value.
> >> + *
> >> + * \return 0 on success or a negative error code otherwise
> >> + */
> >> +
> >> +/**
> >> + * \var Converter::inputBufferReady
> >> + * \brief A signal emitted when the input frame buffer completes
> >> + */
> >> +
> >> +/**
> >> + * \var Converter::outputBufferReady
> >> + * \brief A signal emitted on each frame buffer completion of the output queue
> >> + */
> >> +
> >> +/**
> >> + * \fn Converter::deviceNode()
> >> + * \brief The converter device node attribute accessor
> >> + * \return The converter device node string
> >> + */
> >> +
> >> +/**
> >> + * \class ConverterFactoryBase
> >> + * \brief Base class for converter factories
> >> + *
> >> + * The ConverterFactoryBase class is the base of all specializations of the
> >> + * ConverterFactory class template. It implements the factory registration,
> >> + * maintains a registry of factories, and provides access to the registered
> >> + * factories.
> >> + */
> >> +
> >> +/**
> >> + * \brief Construct a converter factory base
> >> + * \param[in] name Name of the converter class
> >> + * \param[in] compatibles Name aliases of the converter class
> >> + *
> >> + * Creating an instance of the factory base registers it with the global list of
> >> + * factories, accessible through the factories() function.
> >> + *
> >> + * The factory \a name is used as unique identifier. If the converter
> >> + * implementation fully relies on a generic framework, the name should be the
> >> + * same as the framework. Otherwise, if the implementation is specialized, the
> >> + * factory name should match the driver name implementing the function.
> >> + *
> >> + * The factory \a compatibles holds a list of driver names implementing a generic
> >> + * subsystem without any personalizations.
> >> + */
> >> +ConverterFactoryBase::ConverterFactoryBase(const std::string name, std::initializer_list<std::string> compatibles)
> >> + : name_(name), compatibles_(compatibles)
> >> +{
> >> + registerType(this);
> >> +}
> >> +
> >> +/**
> >> + * \fn ConverterFactoryBase::compatibles()
> >> + * \return The names compatibles
> >> + */
> >> +
> >> +/**
> >> + * \brief Create an instance of the converter corresponding to a named factory
> >> + * \param[in] media Name of the factory
> >> + *
> >> + * \return A unique pointer to a new instance of the converter subclass
> >> + * corresponding to the named factory or one of its alias. Otherwise a null
> >> + * pointer if no such factory exists
> >> + */
> >> +std::unique_ptr<Converter> ConverterFactoryBase::create(MediaDevice *media)
> >> +{
> >> + const std::vector<ConverterFactoryBase *> &factories =
> >> + ConverterFactoryBase::factories();
> >> +
> >> + for (const ConverterFactoryBase *factory : factories) {
> >> + const std::vector<std::string> &compatibles = factory->compatibles();
> >> + auto it = std::find(compatibles.begin(), compatibles.end(), media->driver());
> >> +
> >> + if (it == compatibles.end() && media->driver() != factory->name_)
> >> + continue;
> >> +
> >> + LOG(Converter, Debug)
> >> + << "Creating converter from "
> >> + << factory->name_ << " factory with "
> >> + << (it == compatibles.end() ? "no" : media->driver()) << " alias.";
> >> +
> >> + return factory->createInstance(media);
> >> + }
> >> +
> >> + return nullptr;
> >> +}
> >> +
> >> +/**
> >> + * \brief Add a converter class to the registry
> >> + * \param[in] factory Factory to use to construct the converter class
> >> + *
> >> + * The caller is responsible to guarantee the uniqueness of the converter name.
> >> + */
> >> +void ConverterFactoryBase::registerType(ConverterFactoryBase *factory)
> >> +{
> >> + std::vector<ConverterFactoryBase *> &factories =
> >> + ConverterFactoryBase::factories();
> >> +
> >> + factories.push_back(factory);
> >> +}
> >> +
> >> +/**
> >> + * \brief Retrieve the list of all converter factory names
> >> + * \return The list of all converter factory names
> >> + */
> >> +std::vector<std::string> ConverterFactoryBase::names()
> >> +{
> >> + std::vector<std::string> list;
> >> +
> >> + std::vector<ConverterFactoryBase *> &factories =
> >> + ConverterFactoryBase::factories();
> >> +
> >> + for (ConverterFactoryBase *factory : factories) {
> >> + list.push_back(factory->name_);
> >> + for (auto alias : factory->compatibles())
> >> + list.push_back(alias);
> >> + }
> >> +
> >> + return list;
> >> +}
> >> +
> >> +/**
> >> + * \brief Retrieve the list of all converter factories
> >> + * \return The list of converter factories
> >> + */
> >> +std::vector<ConverterFactoryBase *> &ConverterFactoryBase::factories()
> >> +{
> >> + /*
> >> + * The static factories map is defined inside the function to ensure
> >> + * it gets initialized on first use, without any dependency on link
> >> + * order.
> >> + */
> >> + static std::vector<ConverterFactoryBase *> factories;
> >> + return factories;
> >> +}
> >> +
> >> +/**
> >> + * \var ConverterFactoryBase::name_
> >> + * \brief The name of the factory
> >> + */
> >> +
> >> +/**
> >> + * \var ConverterFactoryBase::compatibles_
> >> + * \brief The list holding the factory compatibles
> >> + */
> >> +
> >> +/**
> >> + * \class ConverterFactory
> >> + * \brief Registration of ConverterFactory classes and creation of instances
> >> + * \param _Converter The converter class type for this factory
> >> + *
> >> + * To facilitate discovery and instantiation of Converter classes, the
> >> + * ConverterFactory class implements auto-registration of converter helpers.
> >> + * Each Converter subclass shall register itself using the REGISTER_CONVERTER()
> >> + * macro, which will create a corresponding instance of a ConverterFactory
> >> + * subclass and register it with the static list of factories.
> >> + */
> >> +
> >> +/**
> >> + * \fn ConverterFactory::ConverterFactory(const char *name, std::initializer_list<std::string> compatibles)
> >> + * \brief Construct a converter factory
> >> + * \details \copydetails ConverterFactoryBase::ConverterFactoryBase
> >> + */
> >> +
> >> +/**
> >> + * \fn ConverterFactory::createInstance() const
> >> + * \brief Create an instance of the Converter corresponding to the factory
> >> + * \param[in] media Media device pointer
> >> + * \return A unique pointer to a newly constructed instance of the Converter
> >> + * subclass corresponding to the factory
> >> + */
> >> +
> >> +/**
> >> + * \def REGISTER_CONVERTER
> >> + * \brief Register a converter with the Converter factory
> >> + * \param[in] name Converter name used to register the class
> >> + * \param[in] converter Class name of Converter derived class to register
> >> + * \param[in] compatibles List of compatible names
> >> + *
> >> + * Register a Converter subclass with the factory and make it available to try
> >> + * and match converters.
> >> + */
> >> +
> >> +} /* namespace libcamera */
> >> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> >> index 0494e808..e9d0324e 100644
> >> --- a/src/libcamera/meson.build
> >> +++ b/src/libcamera/meson.build
> >> @@ -13,6 +13,7 @@ libcamera_sources = files([
> >> 'controls.cpp',
> >> 'control_serializer.cpp',
> >> 'control_validator.cpp',
> >> + 'converter.cpp',
> >> 'delayed_controls.cpp',
> >> 'device_enumerator.cpp',
> >> 'device_enumerator_sysfs.cpp',
> >> --
> >> 2.38.1
> >>
More information about the libcamera-devel
mailing list