[libcamera-devel] [PATCH v2 2/4] libcamera: converter: make using MediaDevice optional for the Converter
Jacopo Mondi
jacopo.mondi at ideasonboard.com
Thu Sep 21 10:51:44 CEST 2023
Hi Andrey
On Wed, Sep 20, 2023 at 06:19:19PM +0300, Andrey Konovalov via libcamera-devel wrote:
> Make Converter a bit more generic by making pointer to MediaDevice
> an optional argument for Converter::Converter(),
> ConverterFactoryBase::create(), ConverterFactoryBase::createInstance(),
> and ConverterFactory::createInstance() functions.
.. to prepare for adding support for coverters not backed by a media
device.
>
> Instead of the MediaDevice driver name, use a generic string to match
> against the convertor factory name and its compatibles list. For
s/convertor/converter
> MediaDevice based converters this string will be the MediaDevice driver
> name as before.
>
> Signed-off-by: Andrey Konovalov <andrey.konovalov at linaro.org>
> ---
> include/libcamera/internal/converter.h | 9 ++--
> src/libcamera/converter.cpp | 53 ++++++++++++++----------
> src/libcamera/pipeline/simple/simple.cpp | 33 +++++++++++----
> 3 files changed, 60 insertions(+), 35 deletions(-)
>
> diff --git a/include/libcamera/internal/converter.h b/include/libcamera/internal/converter.h
> index 834ec5ab..d0c70296 100644
> --- a/include/libcamera/internal/converter.h
> +++ b/include/libcamera/internal/converter.h
> @@ -2,6 +2,7 @@
> /*
> * Copyright (C) 2020, Laurent Pinchart
> * Copyright 2022 NXP
> + * Copyright 2023, Linaro Ltd
> *
> * converter.h - Generic format converter interface
> */
> @@ -31,7 +32,7 @@ struct StreamConfiguration;
> class Converter
> {
> public:
> - Converter(MediaDevice *media);
> + Converter(MediaDevice *media = nullptr);
> virtual ~Converter();
>
> virtual int loadConfiguration(const std::string &filename) = 0;
> @@ -72,7 +73,7 @@ public:
>
> const std::vector<std::string> &compatibles() const { return compatibles_; }
>
> - static std::unique_ptr<Converter> create(MediaDevice *media);
> + static std::unique_ptr<Converter> create(std::string name, MediaDevice *media = nullptr);
I'm debated. Should we overload create() instead ?
What's others opinion here ?
> static std::vector<ConverterFactoryBase *> &factories();
> static std::vector<std::string> names();
>
> @@ -81,7 +82,7 @@ private:
>
> static void registerType(ConverterFactoryBase *factory);
>
> - virtual std::unique_ptr<Converter> createInstance(MediaDevice *media) const = 0;
> + virtual std::unique_ptr<Converter> createInstance(MediaDevice *media = nullptr) const = 0;
>
> std::string name_;
> std::vector<std::string> compatibles_;
> @@ -96,7 +97,7 @@ public:
> {
> }
>
> - std::unique_ptr<Converter> createInstance(MediaDevice *media) const override
> + std::unique_ptr<Converter> createInstance(MediaDevice *media = nullptr) const override
> {
> return std::make_unique<_Converter>(media);
> }
> diff --git a/src/libcamera/converter.cpp b/src/libcamera/converter.cpp
> index 466f8421..5070eabc 100644
> --- a/src/libcamera/converter.cpp
> +++ b/src/libcamera/converter.cpp
> @@ -1,6 +1,7 @@
> /* SPDX-License-Identifier: LGPL-2.1-or-later */
> /*
> * Copyright 2022 NXP
> + * Copyright 2023 Linaro Ltd
> *
> * converter.cpp - Generic format converter interface
> */
> @@ -13,8 +14,6 @@
>
> #include "libcamera/internal/media_device.h"
>
> -#include "linux/media.h"
> -
unrelated to this change, isn't it ?
> /**
> * \file internal/converter.h
> * \brief Abstract converter
> @@ -38,26 +37,28 @@ LOG_DEFINE_CATEGORY(Converter)
>
> /**
> * \brief Construct a Converter instance
> - * \param[in] media The media device implementing the converter
> + * \param[in] media The media device implementing the converter (optional)
> *
> * 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;
> + if (media) {
or
if (!media)
return;
> + 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();
> }
> -
> - deviceNode_ = (*it)->deviceNode();
> }
>
> Converter::~Converter()
> @@ -162,7 +163,8 @@ Converter::~Converter()
> /**
> * \fn Converter::deviceNode()
> * \brief The converter device node attribute accessor
> - * \return The converter device node string
> + * \return The converter device node string. If there is no device node for
> + * the converter returnes an empty string.
s/returnes/returns/
> */
>
> /**
> @@ -203,8 +205,13 @@ ConverterFactoryBase::ConverterFactoryBase(const std::string name, std::initiali
> */
>
> /**
> - * \brief Create an instance of the converter corresponding to the media device
> - * \param[in] media the media device to create the converter for
> + * \brief Create an instance of the converter corresponding to the converter
> + * name
> + * \param[in] name the name of the converter to create
^ The
> + * \param[in] media the media device to create the converter for (optional)
> + *
> + * The converter \a name must match the name of the converter factory, or one
> + * of its compatibles.
> *
> * \return A unique pointer to a new instance of the converter subclass
> * corresponding to the media device. The converter is created by the factory
You should also modify this one
> @@ -212,22 +219,22 @@ ConverterFactoryBase::ConverterFactoryBase(const std::string name, std::initiali
> * If the media device driver name doesn't match anything a null pointer is
> * returned.
> */
> -std::unique_ptr<Converter> ConverterFactoryBase::create(MediaDevice *media)
> +std::unique_ptr<Converter> ConverterFactoryBase::create(std::string name, 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());
> + auto it = std::find(compatibles.begin(), compatibles.end(), name);
>
> - if (it == compatibles.end() && media->driver() != factory->name_)
> + if (it == compatibles.end() && name != factory->name_)
> continue;
>
> LOG(Converter, Debug)
> << "Creating converter from "
> << factory->name_ << " factory with "
> - << (it == compatibles.end() ? "no" : media->driver()) << " alias.";
> + << (it == compatibles.end() ? "no" : name) << " alias.";
>
> std::unique_ptr<Converter> converter = factory->createInstance(media);
> if (converter->isValid())
> @@ -318,7 +325,7 @@ std::vector<ConverterFactoryBase *> &ConverterFactoryBase::factories()
> /**
> * \fn ConverterFactory::createInstance() const
> * \brief Create an instance of the Converter corresponding to the factory
> - * \param[in] media Media device pointer
> + * \param[in] media Media device pointer (optional)
> * \return A unique pointer to a newly constructed instance of the Converter
> * subclass corresponding to the factory
> */
> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> index 05ba76bc..3f1b523a 100644
> --- a/src/libcamera/pipeline/simple/simple.cpp
> +++ b/src/libcamera/pipeline/simple/simple.cpp
> @@ -178,20 +178,26 @@ LOG_DEFINE_CATEGORY(SimplePipeline)
>
> class SimplePipelineHandler;
>
> +enum class ConverterFlag {
> + NoFlag = 0,
> + MediaDevice = (1 << 0),
> +};
> +using ConverterFlags = Flags<ConverterFlag>;
> +
Do you expect this to be used as a bitmask or a plain enum would be
enough ? Not that it makes that much difference after all..
> struct SimplePipelineInfo {
> const char *driver;
> /*
> * Each converter in the list contains the name
> * and the number of streams it supports.
> */
> - std::vector<std::pair<const char *, unsigned int>> converters;
> + std::vector<std::tuple<ConverterFlags, const char *, unsigned int>> converters;
> };
>
> namespace {
>
> static const SimplePipelineInfo supportedDevices[] = {
> { "dcmipp", {} },
> - { "imx7-csi", { { "pxp", 1 } } },
> + { "imx7-csi", { { ConverterFlag::MediaDevice, "pxp", 1 } } },
> { "j721e-csi2rx", {} },
> { "mxc-isi", {} },
> { "qcom-camss", {} },
> @@ -330,6 +336,7 @@ public:
>
> V4L2VideoDevice *video(const MediaEntity *entity);
> V4L2Subdevice *subdev(const MediaEntity *entity);
> + const char *converterName() { return converterName_; }
> MediaDevice *converter() { return converter_; }
>
> protected:
> @@ -358,6 +365,7 @@ private:
> MediaDevice *media_;
> std::map<const MediaEntity *, EntityData> entities_;
>
> + const char *converterName_;
> MediaDevice *converter_;
> };
>
> @@ -496,8 +504,10 @@ int SimpleCameraData::init()
>
> /* Open the converter, if any. */
> MediaDevice *converter = pipe->converter();
you could drop this
> - if (converter) {
> - converter_ = ConverterFactoryBase::create(converter);
> + const char *converterName = pipe->converterName();
> + if (converterName) {
> + LOG(SimplePipeline, Info) << "Creating converter '" << converterName << "'";
> + converter_ = ConverterFactoryBase::create(std::string(converterName), converter);
And
converter_ = ConverterFactoryBase::create(std::string(converterName),
pipe->converter());
> if (!converter_) {
> LOG(SimplePipeline, Warning)
> << "Failed to create converter, disabling format conversion";
> @@ -1409,10 +1419,17 @@ bool SimplePipelineHandler::match(DeviceEnumerator *enumerator)
> if (!media_)
> return false;
>
> - for (const auto &[name, streams] : info->converters) {
> - DeviceMatch converterMatch(name);
> - converter_ = acquireMediaDevice(enumerator, converterMatch);
> - if (converter_) {
> + for (const auto &[flags, name, streams] : info->converters) {
> + if (flags & ConverterFlag::MediaDevice) {
> + DeviceMatch converterMatch(name);
> + converter_ = acquireMediaDevice(enumerator, converterMatch);
> + if (converter_) {
> + converterName_ = name;
> + numStreams = streams;
> + break;
> + }
> + } else {
> + converterName_ = name;
> numStreams = streams;
> break;
> }
Isn't this equivalent to:
for (const auto &[flags, name, streams] : info->converters) {
if (flags & ConverterFlag::MediaDevice) {
DeviceMatch converterMatch(name);
converter_ = acquireMediaDevice(enumerator, converterMatch);
if (!converter_) {
LOG(SimplePipeline, Debug)
<< "Failed to acquire converter media device";
continue;
}
}
converterName_ = name;
numStreams = streams;
break;
}
?
> --
> 2.34.1
>
More information about the libcamera-devel
mailing list