[libcamera-devel] [PATCH 04/14] libcamera: Declare generic size and format converter interface
Xavier Roumegue (OSS)
xavier.roumegue at oss.nxp.com
Sun Sep 18 10:39:32 CEST 2022
Hi Jacopo,
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
>> 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
>> 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.
>
>> +#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;
>> +
>> + 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_;
>> + 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_; } \
>> + }; \
>> + static converter##Factory global_##converter##Factory;
>> +
>> +} /* 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)
>> +
>> +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.
>
>> +}
>> +
>> +Converter::~Converter()
>> +{
>> +}
>> +
>> +ConverterFactory::ConverterFactory(const std::string name)
>> + : name_(name)
>> +{
>> + registerType(this);
>> +}
>> +
>> +std::unique_ptr<Converter> ConverterFactory::create(MediaDevice *media)
>> +{
>> + std::vector<ConverterFactory *> &factories =
>> + 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.
>
>> + 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;
>> +}
>> +
>> +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',
>> --
>> 2.37.3
>>
More information about the libcamera-devel
mailing list