[libcamera-devel] [PATCH v3 1/3] libcamera: Add MediaObject class hierarchy

[Jacopo Mondi]

Hi Laurent,

On Mon, Dec 31, 2018 at 02:39:54AM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Sunday, 30 December 2018 16:23:12 EET Jacopo Mondi wrote:
> > Add a class hierarcy to represent all media objects a media graph
> > represents. Add a base MediaObject class, which retains the global unique
> > object id, and define the derived MediaEntity, MediaLink and MediaPad
> > classes.
> >
> > This hierarchy will be used by the MediaDevice objects which represents and
> > handles the media graph.
> >
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> >  src/libcamera/include/media_object.h | 107 ++++++++++
> >  src/libcamera/media_object.cpp       | 281 +++++++++++++++++++++++++++
> >  src/libcamera/meson.build            |   2 +
> >  3 files changed, 390 insertions(+)
> >  create mode 100644 src/libcamera/include/media_object.h
> >  create mode 100644 src/libcamera/media_object.cpp
> >
> > diff --git a/src/libcamera/include/media_object.h
> > b/src/libcamera/include/media_object.h new file mode 100644
> > index 0000000..118d0d8
> > --- /dev/null
> > +++ b/src/libcamera/include/media_object.h
> > @@ -0,0 +1,107 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2018, Google Inc.
> > + *
> > + * media_object.h - Media Device objects: entities, pads and links.
> > + */
> > +#ifndef __LIBCAMERA_MEDIA_OBJECT_H__
> > +#define __LIBCAMERA_MEDIA_OBJECT_H__
> > +
> > +#include <string>
> > +#include <vector>
> > +
> > +#include <linux/media.h>
> > +
> > +namespace libcamera {
> > +
> > +class MediaDevice;
> > +class MediaEntity;
> > +
> > +class MediaObject
> > +{
> > +public:
> > +	MediaObject(unsigned int id) : id_(id) { }
> > +	virtual ~MediaObject() { }
> > +
> > +	unsigned int id() const { return id_; }
> > +
> > +protected:
> > +	unsigned int id_;
> > +};
> > +
> > +class MediaPad;
> > +class MediaLink : public MediaObject
> > +{
> > +	friend class MediaDevice;
> > +
> > +public:
> > +	~MediaLink() { }
> > +
> > +	MediaPad *source() const { return source_; }
> > +	MediaPad *sink() const { return sink_; }
> > +	unsigned int flags() const { return flags_; }
> > +	void setFlags(unsigned int flags) { flags_ = flags; }
> > +
> > +private:
> > +	MediaLink(const struct media_v2_link *link,
> > +		  MediaPad *source, MediaPad *sink);
> > +	MediaLink(const MediaLink &) = delete;
> > +
> > +	MediaPad *source_;
> > +	MediaPad *sink_;
> > +	unsigned int flags_;
> > +};
> > +
> > +class MediaEntity;
> > +class MediaPad : public MediaObject
> > +{
> > +	friend class MediaDevice;
> > +
> > +public:
> > +	~MediaPad();
> > +
> > +	unsigned int index() const { return index_; }
> > +	MediaEntity *entity() const { return entity_; }
> > +	unsigned int flags() const { return flags_; }
> > +	const std::vector<MediaLink *> &links() const { return links_; }
> > +
> > +	void addLink(MediaLink *link);
> > +
> > +private:
> > +	MediaPad(const struct media_v2_pad *pad, MediaEntity *entity);
> > +	MediaPad(const MediaPad &) = delete;
> > +
> > +	unsigned int index_;
> > +	MediaEntity *entity_;
> > +	unsigned int flags_;
> > +
> > +	std::vector<MediaLink *> links_;
> > +};
> > +
> > +class MediaEntity : public MediaObject
> > +{
> > +	friend class MediaDevice;
> > +
> > +public:
> > +	const std::string &name() const { return name_; }
> > +
> > +	const std::vector<MediaPad *> &pads() { return pads_; }
> > +
> > +	const MediaPad *getPadByIndex(unsigned int index);
> > +	const MediaPad *getPadById(unsigned int id);
> > +
> > +private:
> > +	MediaEntity(const struct media_v2_entity *entity);
> > +	MediaEntity(const MediaEntity &) = delete;
> > +	~MediaEntity();
> > +
> > +	std::string name_;
> > +	std::string devnode_;
> > +
> > +	std::vector<MediaPad *> pads_;
> > +
> > +	void addPad(MediaPad *pad);
> > +};
> > +
> > +} /* namespace libcamera */
> > +#endif /* __LIBCAMERA_MEDIA_OBJECT_H__ */
> > diff --git a/src/libcamera/media_object.cpp b/src/libcamera/media_object.cpp
> > new file mode 100644
> > index 0000000..f0906e8
> > --- /dev/null
> > +++ b/src/libcamera/media_object.cpp
> > @@ -0,0 +1,281 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2018, Google Inc.
> > + *
> > + * media_object.cpp - Media device objects: entities, pads and links
> > + */
> > +
> > +#include <errno.h>
> > +#include <fcntl.h>
> > +#include <string.h>
> > +#include <sys/stat.h>
> > +
> > +#include <string>
> > +#include <vector>
> > +
> > +#include <linux/media.h>
> > +
> > +#include "log.h"
> > +#include "media_object.h"
> > +
> > +/**
> > + * \file media_object.h
> > + *
> > + * In the media object file is implemented a class hierarchy that
> > + * represent the counterpart of media objects exposed by the kernel's
> > + * media controller APIs.
> > + *
> > + * Media Objects here represented are media entities, media pads and
> > + * media links, created with informations as obtained by the
> > + * MEDIA_IOC_G_TOPOLOGY ioctl call.
> > + *
> > + * MediaLink, MediaPad and MediaEntity are derived classes of the virtual
> > + * base class MediaObject, which maintains a globally unique id for each
> > object. + *
> > + * Media objects have private constructors to restrict the number of
> > classes + * that can instantiate them only to the 'friend' MediaDevice one.
> > Media + * objects are in facts created when a MediaDevice gets populated. +
> > */
> > +
> > +namespace libcamera {
> > +
> > +/**
> > + * \class MediaObject
> > + * \brief Base class for all media object types
> > + *
> > + * MediaObject is the virtual base class for all media objects in the
> > + * media graph. Each object has a globally unique id assigned, and this
> > + * base class provides that.
> > + *
> > + * MediaEntities, MediaPads and MediaLink are MediaObject derived classes.
> > + */
> > +
> > +/**
> > + * \fn MediaObject::MediaObject()
> > + * \brief Construct a MediaObject with \a id
> > + * \param id The globally unique object id
> > + */
> > +
> > +/**
> > + * \fn MediaObject::id()
> > + * \brief Return the object's globally unique id
> > + */
> > +
> > +/**
> > + * \var MediaObject::id_
> > + * \brief The MediaObject unique id
> > + */
> > +
> > +/**
> > + * \class MediaLink
> > + * \brief Media Link object
> > + *
> > + * A MediaLink represents a 'struct media_v2_link', with associated
> > + * references to the MediaPad it connects and an internal status defined
> > + * by the MEDIA_LNK_FL_* flags captured in flags_.
> > + *
> > + * MediaLink can be enabled enabled and disabled, with the exception of
> > + * immutable links (see MEDIA_LNK_FL_IMMUTABLE).
> > + *
> > + * MediaLinks are created between MediaPads inspecting the media graph
> > + * topology and are stored in both the source and sink MediaPad they
> > + * connect.
> > + */
> > +
> > +/**
> > + * \brief Construct a MediaLink
> > + * \param link The media link representation
> > + * \param source The MediaPad source
> > + * \param sink The MediaPad sink
> > + */
> > +MediaLink::MediaLink(const struct media_v2_link *link, MediaPad *source,
> > +		     MediaPad *sink)
> > +	: MediaObject(link->id), source_(source),
> > +	  sink_(sink), flags_(link->flags)
> > +{
> > +}
> > +
> > +/**
> > + * \fn MediaLink::source()
> > + * \brief Return the source MediaPad
> > + */
> > +
> > +/**
> > + * \fn MediaLink::sink()
> > + * \brief Return the sink MediaPad
> > + */
> > +
> > +/**
> > + * \fn MediaLink::flags()
> > + * \brief Return the link flags
> > + */
> > +
> > +/**
> > + * \fn MediaLink::setFlags()
> > + * \brief Set the link flags to \a flags
> > + * \param flags The flags to be applied to the link
> > + */
> > +
> > +/**
> > + * \class MediaPad
> > + * \brief Media Pad object
> > + *
> > + * MediaPads represent a 'struct media_v2_pad', with associated a list of
> > + * links and a reference to the entity they belong to.
> > + *
> > + * MediaPads are connected to other MediaPads by MediaLinks, created
> > + * inspecting the media graph topology. Data connection between pads can be
> > + * enabled or disabled operating on the link that connects the two. See +
> > * MediaLink.
> > + *
> > + * MediaPads are either 'source' pads, or 'sink' pads. This information is
> > + * captured by the MEDIA_PAD_FL_* flags as stored in the flags_ member
> > + * variable.
> > + *
> > + * Each MediaPad has a list of MediaLinks it is associated to. All links
> > + * in a source pad are outbound links, while all links in a sink pad are
> > + * inbound ones.
> > + *
> > + * Each MediaPad has a reference to the MediaEntity it belongs to.
> > + */
> > +
> > +/**
> > + * \brief Create a MediaPad
> > + * \param pad The media pad representation
> > + * \param entity The MediaEntity this pad belongs to
> > + */
> > +MediaPad::MediaPad(const struct media_v2_pad *pad, MediaEntity *entity)
> > +	: MediaObject(pad->id), index_(pad->index), entity_(entity),
> > +	  flags_(pad->flags)
> > +{
> > +}
> > +
> > +/**
> > + * \brief Delete pad.
> > + *
> > + * Delete the pad by deleting its media links. As a reference to the same
> > + * MediaLink gets stored in both the source and the sink pad, only source
> > + * ones actually delete the MediaLink.
> > + */
> > +MediaPad::~MediaPad()
> > +{
> > +	for (MediaLink *l : links_)
> > +		if (flags_ & MEDIA_PAD_FL_SOURCE)
> > +			delete l;
>
> This is dangerous. Let's store all media objects in one vector in MediaDevice
> and delete them all from there.
>

I still feel that deleting and accessing object following their
connections is a guarantee for correctness. But as the
MediaDevice has already a list of all objects, and it creates them
after all, I've now simplified all destructors.

> > +
> > +	links_.clear();
> > +}
> > +
> > +/**
> > + * \fn MediaPad::index()
> > + * \brief Return the 0-indexed pad index
> > + */
> > +
> > +/**
> > + * \fn MediaPad::entity()
> > + * \brief Return the MediaEntity this pad belongs to
> > + */
> > +
> > +/**
> > + * \fn MediaPad::flags()
> > + * \brief Return the pad flags (MEDIA_PAD_FL_*)
> > + */
> > +
> > +/**
> > + * \fn MediaPad::links()
> > + * \brief Return all links in this pad.
> > + */
> > +
> > +/**
> > + * \brief Add a new link to this pad.
> > + * \param link The new link to add
> > + */
> > +void MediaPad::addLink(MediaLink *link)
> > +{
> > +	links_.push_back(link);
> > +}
> > +
> > +/**
> > + * \class MediaEntity
> > + * \brief Media entity
> > + *
> > + * A MediaEntity represents a 'struct media_v2_entity' with an associated
> > + * list of MediaPads it contains.
> > + *
> > + * Entities are created inspecting the media graph topology, and MediaPads
> > + * gets added as they are discovered.
> > + *
> > + * TODO: Add support for associating a devnode to the entity when
> > integrating
> > + * with DeviceEnumerator.
> > + */
> > +
> > +/**
> > + * \fn MediaEntity::name()
> > + * \brief Return the entity name
> > + */
> > +
> > +/**
> > + * \fn MediaEntity::pads()
> > + * \brief Return all pads in this entity
> > + */
> > +
> > +/**
> > + * \fn MediaEntity::getPadByIndex(unsigned int index)
> > + * \brief Get a pad in this entity by its index
> > + * \return The pad with index \a index
> > + * \return nullptr if no pad is found at \index
> > + */
> > +const MediaPad *MediaEntity::getPadByIndex(unsigned int index)
> > +{
> > +	for (MediaPad *p : pads_)
> > +		if (p->index() == index)
> > +			return p;
> > +
> > +	return nullptr;
>
> You can just return pads_[index] (checking whether index is valid first of
> course).

That calls for storing the number of pads in the entity, which I've no
need of at the momement. I'll keep this as it is.

>
> > +}
> > +
> > +/**
> > + * \brief Get a pad in this entity by its id
> > + * \param id The pad globally unique id
> > + * \return The pad with id \a id
> > + * \return nullptr if not pad with \id is found
> > + */
> > +const MediaPad *MediaEntity::getPadById(unsigned int id)
> > +{
> > +	for (MediaPad *p : pads_)
> > +		if (p->id() == id)
> > +			return p;
> > +
> > +	return nullptr;
> > +}
> > +
> > +/**
> > + * \brief Construct a MediaEntity
> > + * \param entity The media entity representation
> > + */
> > +MediaEntity::MediaEntity(const struct media_v2_entity *entity)
> > +	: MediaObject(entity->id), name_(entity->name)
> > +{
> > +}
> > +
> > +/**
> > + * \fn MediaEntity::~MediaEntity()
> > + * \brief Delete all pads in the entity
> > + */
> > +MediaEntity::~MediaEntity()
> > +{
> > +	for (MediaPad *p : pads_)
> > +		delete p;
>
> Same comment as for links.
>
> Apart from that there's no blocking issue,
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>

Thanks, I'll send and push v4 shortly.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20181231/2477a5bf/attachment-0001.sig>