[libcamera-devel] [PATCH v2 2/4] libcamera: converter: make using MediaDevice optional for the Converter
Andrey Konovalov
andrey.konovalov at linaro.org
Thu Sep 21 20:52:13 CEST 2023
Hi Jacopo,
Thank you for the review!
On 21.09.2023 11:51, Jacopo Mondi wrote:
> 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.
Will add that.
>>
>> 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
Will fix.
>> 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 ?
Probably. But currently I am not sure.
> What's others opinion here ?
If there are no comments from the others I'll send the next version still using the default arg.
But we can always get back to this later (even in the next versions of this patch set).
>> 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 ?
I'll create a separate patch for this change.
What should I credit you with in the commit message as it was you who spotted this
issue and suggested to fix 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;
OK
>> + 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/
Will fix
>> */
>>
>> /**
>> @@ -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
Right. Will fix that
>> + * \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
Will do
>> @@ -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..
For now a plain enum (or even bool) would be enough.
But having a bitmask might become useful if in future some of different Converter
subclasses could share some features. And it doesn't add a lot of complexity, so
I'd probably keep it.
>> 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());
OK, sounds good.
>> 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;
> }
> ?
Yep, it is. Will update the patch.
Thanks,
Andrey
>> --
>> 2.34.1
>>
More information about the libcamera-devel
mailing list