[libcamera-devel,v4,1/2] libcamera: Declare generic converter interface

18009 diff mbox series

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@oss.nxp.com>
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
Reviewed-by: Kieran Bingham <kieran.bingham@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

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@oss.nxp.com>
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> Reviewed-by: Kieran Bingham <kieran.bingham@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_; };

This causes a compiler warning in clang-11.

Removing the semi-colon at the end is all it takes, so this could be
fixed while applying.

With that - it builds fine.
--
Kieran


> +
> +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
>

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.


> ----------------------------------------------------------------------------------------------------------------
> 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.



> --- 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.




>  /**
>   * \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@oss.nxp.com>
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> Reviewed-by: Kieran Bingham <kieran.bingham@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
>

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 ?
> 
> 
>> ----------------------------------------------------------------------------------------------------------------
>> 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'):

> 
> 
> 
>> --- 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
> 
> 
> 
> 
>>   /**
>>    * \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@oss.nxp.com>
>> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
>> Reviewed-by: Kieran Bingham <kieran.bingham@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
>>

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@oss.nxp.com>
> >> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> >> Reviewed-by: Kieran Bingham <kieran.bingham@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
> >>

Quoting Kieran Bingham (2022-12-14 14:23:45)
> 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. 

Oh! Now I see the patch, this was bad advice from me. The
convertor_v4l2_m2m.h is the interface header for this compile unit. That
always goes first, but I can't figure out how to tell clang-format that
when the .h headers are not located next to the .cpp file ... so it
doesn't know that it's supposed to be first.

I'll fix this while applying, and get this series in.

--
Kieran


> 
> > > 
> > > 
> > > 
> > > 
> > >>   /**
> > >>    * \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@oss.nxp.com>
> > >> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> > >> Reviewed-by: Kieran Bingham <kieran.bingham@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
> > >>

Quoting Kieran Bingham (2022-12-14 16:25:27)
> > > >> --- 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. 
> 
> Oh! Now I see the patch, this was bad advice from me. The
> convertor_v4l2_m2m.h is the interface header for this compile unit. That
> always goes first, but I can't figure out how to tell clang-format that
> when the .h headers are not located next to the .cpp file ... so it
> doesn't know that it's supposed to be first.
> 
> I'll fix this while applying, and get this series in.

FTR: I was confused above, and misread the diff between the version. v5
was/is absolutely fine! (and I've merged it).

--
Kieran