| Message ID | 20230928185537.20178-4-andrey.konovalov@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Andrey, Thank you for the patch On jeu., sept. 28, 2023 at 21:55, Andrey Konovalov <andrey.konovalov@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@linaro.org> Reviewed-by: Mattijs Korpershoek <mkorpershoek@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
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. */
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 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@linaro.org> --- 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(-)