[libcamera-devel] [PATCH v3 3/5] libcamera: converter: make using MediaDevice optional for the Converter
Mattijs Korpershoek
mkorpershoek at baylibre.com
Sat Sep 30 10:59:46 CEST 2023
Hi Andrey,
Thank you for the patch
On jeu., sept. 28, 2023 at 21:55, Andrey Konovalov <andrey.konovalov at linaro.org> 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.
nit: s/coverters/converters
>
> Instead of the MediaDevice driver name, use a generic string to match
> against the converter factory name and its compatibles list. For
> MediaDevice based converters this string will be the MediaDevice driver
> name as before.
>
> Signed-off-by: Andrey Konovalov <andrey.konovalov at linaro.org>
Reviewed-by: Mattijs Korpershoek <mkorpershoek at baylibre.com>
> ---
> include/libcamera/internal/converter.h | 9 +++---
> src/libcamera/converter.cpp | 40 +++++++++++++++---------
> src/libcamera/pipeline/simple/simple.cpp | 39 ++++++++++++++++-------
> 3 files changed, 58 insertions(+), 30 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);
> 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 aca4fbc7..fe073bf2 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
> */
> @@ -36,13 +37,16 @@ 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)
> {
> + if (!media)
> + return;
> +
> const std::vector<MediaEntity *> &entities = media->entities();
> auto it = std::find_if(entities.begin(), entities.end(),
> [](MediaEntity *entity) {
> @@ -160,7 +164,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 returns an empty string.
> */
>
> /**
> @@ -201,32 +206,37 @@ 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
> + * \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. For media device based converters the converter \a name
> + * is the media device driver name.
> + *
> + * \return A unique pointer to a new instance of the converter subclass.
> + * The converter is created by matching the factory name or any of its
> + * compatible aliases with the converter name.
> *
> - * \return A unique pointer to a new instance of the converter subclass
> - * corresponding to the media device. The converter is created by matching
> - * the factory name or any of its compatible aliases with the media device
> - * driver name.
> - * If the media device driver name doesn't match anything a null pointer is
> - * returned.
> + * If the converter 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())
> @@ -317,7 +327,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..c7da700b 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>;
> +
> 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_;
> };
>
> @@ -495,9 +503,10 @@ int SimpleCameraData::init()
> int ret;
>
> /* Open the converter, if any. */
> - MediaDevice *converter = pipe->converter();
> - 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), pipe->converter());
> if (!converter_) {
> LOG(SimplePipeline, Warning)
> << "Failed to create converter, disabling format conversion";
> @@ -1409,13 +1418,21 @@ 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_) {
> - numStreams = streams;
> - break;
> + 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 '"
> + << name << "'";
> + continue;
> + }
> }
> +
> + converterName_ = name;
> + numStreams = streams;
> + break;
> }
>
> /* Locate the sensors. */
> --
> 2.34.1
More information about the libcamera-devel
mailing list