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

[Laurent Pinchart]

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.

> +
> +	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).

> +}
> +
> +/**
> + * \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>

> +	pads_.clear();
> +}
> +
> +/**
> + * \brief Add \a pad to list of entity's pads
> + * \param pad The pad to add
> + */
> +void MediaEntity::addPad(MediaPad *pad)
> +{
> +	pads_.push_back(pad);
> +}
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index f632eb5..01d321c 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -1,10 +1,12 @@
>  libcamera_sources = files([
>      'log.cpp',
>      'main.cpp',
> +    'media_object.cpp',
>  ])
> 
>  libcamera_headers = files([
>      'include/log.h',
> +    'include/media_object.h',
>      'include/utils.h',
>  ])

-- 
Regards,

Laurent Pinchart