[libcamera-devel] [PATCH 04/14] libcamera: Declare generic size and format converter interface
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Oct 4 00:08:38 CEST 2022
Hi Xavier,
On Sun, Sep 18, 2022 at 10:39:32AM +0200, Xavier Roumegue (OSS) via libcamera-devel wrote:
> On 9/14/22 12:46, Jacopo Mondi wrote:
> > Hi Xavier
> > I would drop "size and format" and use "Converter" in the subject
> > title.
> >
> > On Thu, Sep 08, 2022 at 08:48:40PM +0200, Xavier Roumegue via libcamera-devel wrote:
> >> Declare a converter Abstract Base Class intented to provide generic
s/intented/intended/
> >> 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 load_configuration() method which
s/load_configuration() method/loadConfigure() function/
"method" is a term from Java, it's not used by the C++ specification
(which uses "member function" instead).
> >> can be used by the concrete implementation to load hardware specific
> >> runtime parameters defined by the application.
> >>
> >> Signed-off-by: Xavier Roumegue <xavier.roumegue at oss.nxp.com>
> >> ---
> >> include/libcamera/internal/converter.h | 101 ++++++++++++++++++++++++
> >> include/libcamera/internal/meson.build | 1 +
> >> src/libcamera/converter.cpp | 102 +++++++++++++++++++++++++
> >> src/libcamera/meson.build | 1 +
> >> 4 files changed, 205 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..e2237c57
> >> --- /dev/null
> >> +++ b/include/libcamera/internal/converter.h
> >> @@ -0,0 +1,101 @@
> >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> >> +/*
> >> + * Copyright (C) 2020, Laurent Pinchart
> >> + * Copyright 2022 NXP
> >> + *
> >> + * converter.h - Generic stream converter infrastructure
> >> + */
> >> +
> >> +#pragma once
> >> +
> >> +#include <functional>
> >
> > I might have missed what is functional included for
>
> Right, I will remove it.
It's for std::reference_wrapper.
> >> +#include <map>
> >> +#include <memory>
> >> +#include <string>
> >> +#include <tuple>
> >> +#include <vector>
> >> +
> >> +#include <libcamera/base/class.h>
> >> +#include <libcamera/base/signal.h>
> >> +
> >> +#include <libcamera/geometry.h>
> >> +#include <libcamera/pixel_format.h>
> >> +
> >> +namespace libcamera {
> >> +
> >> +class FrameBuffer;
> >> +class MediaDevice;
> >> +class Size;
> >> +class SizeRange;
> >> +struct StreamConfiguration;
> >> +
> >> +class Converter
> >> +{
> >> +public:
> >> + Converter(MediaDevice *media);
> >> + virtual ~Converter();
> >
> > Empty line maybe ?
> >
> >> + 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;
This was a bit of an ad-hoc API for the simple pipeline handler. We can
keep it as-is for now, and rework it on top to improve the converter
API.
> >> +
> >> + virtual int configure(const StreamConfiguration &inputCfg,
> >> + const std::vector<std::reference_wrapper<StreamConfiguration>> &outputCfg) = 0;
> >> + virtual int exportBuffers(unsigned int ouput, 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;
> >> +
> >> + std::string deviceNode_;
This should be private, with an accessor function.
> >> + Signal<FrameBuffer *> inputBufferReady;
> >> + Signal<FrameBuffer *> outputBufferReady;
> >> +};
> >> +
> >> +class ConverterFactory
> >> +{
> >> +public:
> >> + ConverterFactory(const std::string name);
> >> + virtual ~ConverterFactory() = default;
> >> +
> >> + static std::unique_ptr<Converter> create(MediaDevice *media);
> >> +
> >> + static void registerType(ConverterFactory *factory);
> >> + static std::vector<ConverterFactory *> &factories();
> >> + static std::vector<std::string> names();
> >> +
> >> +protected:
> >> + virtual Converter *createInstance(MediaDevice *media) = 0;
> >> + virtual const std::vector<std::string> aliases() const = 0;
> >> +
> >> +private:
> >> + LIBCAMERA_DISABLE_COPY_AND_MOVE(ConverterFactory)
> >> +
> >> + std::string name_;
> >> +};
> >> +
> >> +#define REGISTER_CONVERTER(name, converter, ...) \
> >> + class converter##Factory final : public ConverterFactory \
> >> + { \
> >> + public: \
> >> + converter##Factory() : ConverterFactory(name) {} \
> >> + \
> >> + private: \
> >> + Converter *createInstance(MediaDevice *media) \
> >> + { \
> >> + return new converter(media); \
> >> + } \
> >> + std::vector<std::string> aliases_ = { __VA_ARGS__ }; \
> >> + const std::vector<std::string> aliases() const { return aliases_; } \
This should return a reference.
> >> + }; \
> >> + static converter##Factory global_##converter##Factory;
I've sent a patch series that moved the CameraSensorHelper and
PipelineHandler factories to using a class template for registration
("[PATCH 0/8] libcamera: Use class templates for auto-registration").
Could you base this code on that design ? It would result in
class ConverterFactoryBase
{
public:
ConverterFactoryBase(const char *name, std::initializer_list<std::string> aliases);
virtual ~ConverterFactoryBase() = default;
const std::vector<std::string> &aliases() const { return aliases_; }
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> aliases_;
};
template<typename _Converter>
class ConverterFactory : public ConverterFactoryBase
{
public:
ConverterFactory(const char *name, std::initializer_list<std::string> aliases)
: ConverterFactoryBase(name, aliases)
{
}
std::unique_ptr<Converter> createInstance(MediaDevice *media) const override
{
return std::make_unique<_Converter>(media);
}
};
#define REGISTER_CONVERTER(name, converter, ...) \
static ConverterFactory<converter> global_##converter##Factory(name, { __VA_ARGS__ });
> >> +
> >> +} /* namespace libcamera */
> >> diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build
> >> index 7a780d48..8f50d755 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..89a594d1
> >> --- /dev/null
> >> +++ b/src/libcamera/converter.cpp
> >> @@ -0,0 +1,102 @@
> >> +
> >
> > Rogue empty line
> >
> >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> >> +/*
> >> + * Copyright 2022 NXP
> >> + *
> >> + * converter.cpp - Generic Format converter interface
> >> + */
> >> +
> >> +#include <algorithm>
> >> +
> >> +#include <libcamera/base/log.h>
> >> +
> >> +#include "libcamera/internal/converter.h"
> >> +#include "libcamera/internal/media_device.h"
> >> +
> >> +namespace libcamera {
> >> +
> >> +LOG_DEFINE_CATEGORY(Converter)
> >> +
Documentation will be needed ;-)
> >> +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())
> >> + return;
> >
> > Should we complain loud and set some state variable that can be
> > inspected by the isValid() function already ?
>
> Complaining loud can not hurt. An empty deviceNode_ string can be used
> to indicate the instance is invalid in case the concrete class relies on
> a device node. Adding a kind of isValid boolean attribute would likely
> be redundant with isValid().
>
> >> +
> >> + deviceNode_ = (*it)->deviceNode();
> >
> > I'm not sure storing a string here is any useful if not fo debugging
> > purposes. What if you store a pointer to the entity instead, so
> > derived classes can use the static fromMediaEntity() function (which
> > probably have to be added to the M2M device) to open the device node ?
>
> The ambition of this abstract class is to be framework/hardware
> agnostic. For instance, the converter could sit on top on a DRM device,
> or any hardware compute backend (dsp, cpu, etc..) to implement the
> interface.
> Hence, the design choice to not embed specialized framework in the
> abstract class except in the constructor.
I agree that's the way to go, but there will be no media device in that
case. We can specialize the class for now if that brings any
improvement, and rework it later.
> >> +}
> >> +
> >> +Converter::~Converter()
> >> +{
> >> +}
> >> +
> >> +ConverterFactory::ConverterFactory(const std::string name)
> >> + : name_(name)
> >> +{
> >> + registerType(this);
> >> +}
> >> +
> >> +std::unique_ptr<Converter> ConverterFactory::create(MediaDevice *media)
> >> +{
> >> + std::vector<ConverterFactory *> &factories =
You can make this const, as well as the factory pointer below.
> >> + ConverterFactory::factories();
> >
> > Oh, this is different from the other favtories we have.
> > The usage here is:
> >
> > ConverterFavtory::create()
> > for (factory: factories())
> > factory->create();
> >
> > While other factories (looking at the pipeline handler factory) have a
> > static method to retrieve factories, and then call create() on them.
> >
> > My understanding is that this shouldn't change much in regard to the
> > link order protection realized by declaring factories_ inside the
> > factories() function, just pointing it out to see if someone else see
> > an issue here.
> >
> >> +
> >> + for (ConverterFactory *factory : factories) {
> >> + std::vector<std::string> aliases = factory->aliases();
> >> + auto it = std::find(aliases.begin(), aliases.end(), media->driver());
> >
> > How do you envision aliases to be used ? I mean, do you expect a
> > Converter instance to handle different devices ? Probably yes, as a
> > generic M2M converter will follow in the series and could handle
> > multiple devices...
>
> Yes, the factory name would be a framework name in this case and the
> aliases would point to driver names libcamera could use as converters.
>
> >> +src/libcamera/pipeline_handler.cpp
> >> + if (it == aliases.end() && media->driver() != factory->name_)
> >
> > what is "media->driver() != factory->name_)" for ? Shouldn't the media
> > driver name be part of aliases only ?
>
> In case the media driver name should directly be used as binding string.
> Yes, the driver name should be part of the aliases, but that's a way to
> enforce it.
I'm not too fond of the alias mechanism, especially with the separate
name, but that can be handled later (or maybe I'll make a proposal with
fixup patches on top after reviewing the whole series, I don't know yet
how I would like the code to be structured).
> >> + continue;
> >> +
> >> + LOG(Converter, Debug)
> >> + << "Creating converter from "
> >> + << factory->name_ << " factory with "
> >> + << (it == aliases.end() ? "no" : media->driver()) << " alias.";
> >
> > How long is this message ? Can it be shortened or the factory name
> > here is relevant ?
>
> "Creating converter from dw100 with no alias"
> That's not so long but can be removed as this just serve debug purpose.
>
> >> +
> >> + Converter *converter = factory->createInstance(media);
> >> + return std::unique_ptr<Converter>(converter);
> >> + }
> >> +
> >> + return nullptr;
> >> +}
> >> +
> >> +void ConverterFactory::registerType(ConverterFactory *factory)
> >> +{
> >> + std::vector<ConverterFactory *> &factories =
> >> + ConverterFactory::factories();
> >> +
> >> + factories.push_back(factory);
> >> +}
> >> +
> >> +std::vector<std::string> ConverterFactory::names()
> >> +{
> >> + std::vector<std::string> list;
> >> +
> >> + std::vector<ConverterFactory *> &factories =
> >> + ConverterFactory::factories();
> >> +
> >> + for (ConverterFactory *factory : factories) {
> >> + list.push_back(factory->name_);
> >> + for (auto alias : factory->aliases())
> >> + list.push_back(alias);
> >> + }
> >> +
> >> + return list;
> >> +}
This I'm not very fond of either, I'll check how it's used.
> >> +
> >> +std::vector<ConverterFactory *> &ConverterFactory::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<ConverterFactory *> factories;
> >> + return factories;
> >> +}
> >> +
> >> +} /* namespace libcamera */
> >> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> >> index 63b47b17..a261d4b4 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',
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list