[libcamera-devel] [PATCH v2 2/4] libcamera: converter: make using MediaDevice optional for the Converter
Jacopo Mondi
jacopo.mondi at ideasonboard.com
Fri Sep 22 09:25:12 CEST 2023
HI Andrey
On Thu, Sep 21, 2023 at 09:52:13PM +0300, Andrey Konovalov wrote:
> 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?
>
That would be a Suggested-by: tag probably, but it's not that
necessary in this case. Up to you :)
Thanks
j
> > > /**
> > > * \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