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

110 diff mbox series

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@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

Hi Jacopo,

Thanks for your patch.

On 2018-12-30 15:23:12 +0100, 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@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);

Why not use references here? Would it be valid to create a MediaLink 
where any of the parameters are nullptr?

Same question for other constructors in this patch.

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

Same here why not use a possibly const reference?

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

s/In the media object file is implemented a/Provide a/

> + * represent the counterpart of media objects exposed by the kernel's

s/counterpart of/corresponding/

> + * media controller APIs.
> + *
> + * Media Objects here represented are media entities, media pads and

s/here represented/represented here/

> + * 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

Only asking if the \return key word should not be used what is returned 
by the function and \brief describing the action performed.?

\brief Retrieve the media objects global id
\returns Media objects global id

I know it seems silly for such a simple function but still I like the 
\return keyword as it's easy to spot when reading the documentation.

Same comment on all functions returning something which is described 
using the \brief keyword.

> + */
> +
> +/**
> + * \var MediaObject::id_
> + * \brief The MediaObject unique id
> + */

Should we document private variables which do not require special 
explanations for one reason or another?

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

s/enabled enabled/enabled/

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

Is it useful to add documentation for these trivial getters?

> +
> +/**
> + * \fn MediaLink::setFlags()
> + * \brief Set the link flags to \a flags
> + * \param flags The flags to be applied to the link
> + */

I'm not sure why this function is need? I can find no references to it 
on the rest of this series. Maybe it's used by code yet to be posted.  
Could you drop it from this series or expand the documentation to it's 
intended purpose?

> +
> +/**
> + * \class MediaPad
> + * \brief Media Pad object
> + *
> + * MediaPads represent a 'struct media_v2_pad', with associated a list of

s/associated a/associated/

> + * 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 representationo

Maybe make it clear that it's the kernels representation of the pad and 
not the libraries?

> + * \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.

Can this create race conditions? If the source pad is deleted before the 
sink there is a dangling pointer alive. Would reference counting be 
needed here or is this a none issue?

I'm thinking of when we get hotplug support and entities in the media 
graph can come and go. If the entity proving the source pad is removed 
the link would be deleted while the sink pad is still alive and pointing 
to the now deleted link.

Can be addressed on top of this if deemed at all to be a issue.

> + */
> +MediaPad::~MediaPad()
> +{
> +	for (MediaLink *l : links_)
> +		if (flags_ & MEDIA_PAD_FL_SOURCE)
> +			delete l;
> +
> +	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.
> + */

Is it useful to document these trivial getters?

> +
> +/**
> + * \brief Add a new link to this pad.

You have not ended other \brief with a period before. Not sure what is 
best, but it should be the same.

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

Same as above.

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

@s@\index@\a index@

> + */
> +const MediaPad *MediaEntity::getPadByIndex(unsigned int index)
> +{
> +	for (MediaPad *p : pads_)
> +		if (p->index() == index)
> +			return p;
> +
> +	return nullptr;
> +}
> +
> +/**
> + * \brief Get a pad in this entity by its id

I would add the words 'globally unique id' in the \brief

> + * \param id The pad globally unique id
> + * \return The pad with id \a id
> + * \return nullptr if not pad with \id is found

@s@\id@\a id@

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

Same as for MediaPad::MediaPad, mention the parameter is the kernels 
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

How about:

\brief: Cleans up all resources allocated by the MediaEntity

> + */
> +MediaEntity::~MediaEntity()
> +{
> +	for (MediaPad *p : pads_)
> +		delete p;
> +	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',
>  ])
>  
> -- 
> 2.20.1
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Hi Niklas,
   thanks for review

On Sun, Dec 30, 2018 at 10:50:15PM +0100, Niklas Söderlund wrote:
> Hi Jacopo,
>
> Thanks for your patch.
>
> On 2018-12-30 15:23:12 +0100, 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@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);
>
> Why not use references here? Would it be valid to create a MediaLink
> where any of the parameters are nullptr?

I feel it is better to use pointers for dynamically allocated objects
as this class entities are. The caller will have pointers to them, and
I should dereference them to access their value and pass it by
reference.

As I've replied to your series, I have not found this stated anywhere,
so it might just be my preference, but I agree this protects us from
nullptr values.

>
> Same question for other constructors in this patch.
>

Could I keep them like this until we don't try establish a firm rule?

> > +	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);
>
> Same here why not use a possibly const reference?
>
> > +
> > +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
>
> s/In the media object file is implemented a/Provide a/
>
> > + * represent the counterpart of media objects exposed by the kernel's
>
> s/counterpart of/corresponding/
>
> > + * media controller APIs.
> > + *
> > + * Media Objects here represented are media entities, media pads and
>
> s/here represented/represented here/
>

Thanks

> > + * 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
>
> Only asking if the \return key word should not be used what is returned
> by the function and \brief describing the action performed.?
>
> \brief Retrieve the media objects global id
> \returns Media objects global id
>
> I know it seems silly for such a simple function but still I like the
> \return keyword as it's easy to spot when reading the documentation.
>
> Same comment on all functions returning something which is described
> using the \brief keyword.
>

I agree on both points, it's better, but still silly for such simply
methods :)

> > + */
> > +
> > +/**
> > + * \var MediaObject::id_
> > + * \brief The MediaObject unique id
> > + */
>
> Should we document private variables which do not require special
> explanations for one reason or another?
>

It's protected, I think it's worth documenting it (doxygen wants it to
be commented...)

> > +
> > +/**
> > + * \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
>
> s/enabled enabled/enabled/
>
> > + * 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
> > + */
>
> Is it useful to add documentation for these trivial getters?
>

If doxygen compiler is happy, I'm happy :)
I agree going forward we could relax this, but since I already wrote
it I would keep it here.

> > +
> > +/**
> > + * \fn MediaLink::setFlags()
> > + * \brief Set the link flags to \a flags
> > + * \param flags The flags to be applied to the link
> > + */
>
> I'm not sure why this function is need? I can find no references to it
> on the rest of this series. Maybe it's used by code yet to be posted.
> Could you drop it from this series or expand the documentation to it's
> intended purpose?
>

Leftover and not used here, you're right. I'll drop.

> > +
> > +/**
> > + * \class MediaPad
> > + * \brief Media Pad object
> > + *
> > + * MediaPads represent a 'struct media_v2_pad', with associated a list of
>
> s/associated a/associated/
>
> > + * 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 representationo
>
> Maybe make it clear that it's the kernels representation of the pad and
> not the libraries?

"The kernel media_v2_pad representation" ?
And I've miss-spelled representationo

>
> > + * \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.
>
> Can this create race conditions? If the source pad is deleted before the
> sink there is a dangling pointer alive. Would reference counting be
> needed here or is this a none issue?
>
> I'm thinking of when we get hotplug support and entities in the media
> graph can come and go. If the entity proving the source pad is removed
> the link would be deleted while the sink pad is still alive and pointing
> to the now deleted link.
>
> Can be addressed on top of this if deemed at all to be a issue.
>

I do expect we handle link deletion as well when adding hot-plug support.
For now, I don't think it's an issue though.

> > + */
> > +MediaPad::~MediaPad()
> > +{
> > +	for (MediaLink *l : links_)
> > +		if (flags_ & MEDIA_PAD_FL_SOURCE)
> > +			delete l;
> > +
> > +	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.
> > + */
>
> Is it useful to document these trivial getters?
>
> > +
> > +/**
> > + * \brief Add a new link to this pad.
>
> You have not ended other \brief with a period before. Not sure what is
> best, but it should be the same.

Right, not sure what to do. I'll drop dots, as you've not been using
them in your series.

>
> > + * \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
> > + */
>
> Same as above.
>
> > +
> > +/**
> > + * \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
>
> @s@\index@\a index@
>
> > + */
> > +const MediaPad *MediaEntity::getPadByIndex(unsigned int index)
> > +{
> > +	for (MediaPad *p : pads_)
> > +		if (p->index() == index)
> > +			return p;
> > +
> > +	return nullptr;
> > +}
> > +
> > +/**
> > + * \brief Get a pad in this entity by its id
>
> I would add the words 'globally unique id' in the \brief
>
> > + * \param id The pad globally unique id
> > + * \return The pad with id \a id
> > + * \return nullptr if not pad with \id is found
>
> @s@\id@\a id@
>
> > + */
> > +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
>
> Same as for MediaPad::MediaPad, mention the parameter is the kernels
> 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
>
> How about:
>
> \brief: Cleans up all resources allocated by the MediaEntity
>

Yes, better (this and all the above comments)

> > + */
> > +MediaEntity::~MediaEntity()
> > +{
> > +	for (MediaPad *p : pads_)
> > +		delete p;
> > +	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',
> >  ])
> >
> > --
> > 2.20.1
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel@lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
>
> --
> Regards,
> Niklas Söderlund

Hi Jacopo,

On 2018-12-30 23:10:57 +0100, Jacopo Mondi wrote:
> Hi Niklas,
>    thanks for review
> 
> On Sun, Dec 30, 2018 at 10:50:15PM +0100, Niklas Söderlund wrote:
> > Hi Jacopo,
> >
> > Thanks for your patch.
> >
> > On 2018-12-30 15:23:12 +0100, 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@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);
> >
> > Why not use references here? Would it be valid to create a MediaLink
> > where any of the parameters are nullptr?
> 
> I feel it is better to use pointers for dynamically allocated objects
> as this class entities are. The caller will have pointers to them, and
> I should dereference them to access their value and pass it by
> reference.
> 
> As I've replied to your series, I have not found this stated anywhere,
> so it might just be my preference, but I agree this protects us from
> nullptr values.
> 
> >
> > Same question for other constructors in this patch.
> >
> 
> Could I keep them like this until we don't try establish a firm rule?

I have no problem keeping them, but if you want to keep them you should 
ensure they are not null before dereferencing them.

> 
> > > +	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);
> >
> > Same here why not use a possibly const reference?
> >
> > > +
> > > +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
> >
> > s/In the media object file is implemented a/Provide a/
> >
> > > + * represent the counterpart of media objects exposed by the kernel's
> >
> > s/counterpart of/corresponding/
> >
> > > + * media controller APIs.
> > > + *
> > > + * Media Objects here represented are media entities, media pads and
> >
> > s/here represented/represented here/
> >
> 
> Thanks
> 
> > > + * 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
> >
> > Only asking if the \return key word should not be used what is returned
> > by the function and \brief describing the action performed.?
> >
> > \brief Retrieve the media objects global id
> > \returns Media objects global id
> >
> > I know it seems silly for such a simple function but still I like the
> > \return keyword as it's easy to spot when reading the documentation.
> >
> > Same comment on all functions returning something which is described
> > using the \brief keyword.
> >
> 
> I agree on both points, it's better, but still silly for such simply
> methods :)
> 
> > > + */
> > > +
> > > +/**
> > > + * \var MediaObject::id_
> > > + * \brief The MediaObject unique id
> > > + */
> >
> > Should we document private variables which do not require special
> > explanations for one reason or another?
> >
> 
> It's protected, I think it's worth documenting it (doxygen wants it to
> be commented...)
> 
> > > +
> > > +/**
> > > + * \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
> >
> > s/enabled enabled/enabled/
> >
> > > + * 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
> > > + */
> >
> > Is it useful to add documentation for these trivial getters?
> >
> 
> If doxygen compiler is happy, I'm happy :)
> I agree going forward we could relax this, but since I already wrote
> it I would keep it here.
> 
> > > +
> > > +/**
> > > + * \fn MediaLink::setFlags()
> > > + * \brief Set the link flags to \a flags
> > > + * \param flags The flags to be applied to the link
> > > + */
> >
> > I'm not sure why this function is need? I can find no references to it
> > on the rest of this series. Maybe it's used by code yet to be posted.
> > Could you drop it from this series or expand the documentation to it's
> > intended purpose?
> >
> 
> Leftover and not used here, you're right. I'll drop.
> 
> > > +
> > > +/**
> > > + * \class MediaPad
> > > + * \brief Media Pad object
> > > + *
> > > + * MediaPads represent a 'struct media_v2_pad', with associated a list of
> >
> > s/associated a/associated/
> >
> > > + * 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 representationo
> >
> > Maybe make it clear that it's the kernels representation of the pad and
> > not the libraries?
> 
> "The kernel media_v2_pad representation" ?
> And I've miss-spelled representationo
> 
> >
> > > + * \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.
> >
> > Can this create race conditions? If the source pad is deleted before the
> > sink there is a dangling pointer alive. Would reference counting be
> > needed here or is this a none issue?
> >
> > I'm thinking of when we get hotplug support and entities in the media
> > graph can come and go. If the entity proving the source pad is removed
> > the link would be deleted while the sink pad is still alive and pointing
> > to the now deleted link.
> >
> > Can be addressed on top of this if deemed at all to be a issue.
> >
> 
> I do expect we handle link deletion as well when adding hot-plug support.
> For now, I don't think it's an issue though.
> 
> > > + */
> > > +MediaPad::~MediaPad()
> > > +{
> > > +	for (MediaLink *l : links_)
> > > +		if (flags_ & MEDIA_PAD_FL_SOURCE)
> > > +			delete l;
> > > +
> > > +	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.
> > > + */
> >
> > Is it useful to document these trivial getters?
> >
> > > +
> > > +/**
> > > + * \brief Add a new link to this pad.
> >
> > You have not ended other \brief with a period before. Not sure what is
> > best, but it should be the same.
> 
> Right, not sure what to do. I'll drop dots, as you've not been using
> them in your series.
> 
> >
> > > + * \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
> > > + */
> >
> > Same as above.
> >
> > > +
> > > +/**
> > > + * \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
> >
> > @s@\index@\a index@
> >
> > > + */
> > > +const MediaPad *MediaEntity::getPadByIndex(unsigned int index)
> > > +{
> > > +	for (MediaPad *p : pads_)
> > > +		if (p->index() == index)
> > > +			return p;
> > > +
> > > +	return nullptr;
> > > +}
> > > +
> > > +/**
> > > + * \brief Get a pad in this entity by its id
> >
> > I would add the words 'globally unique id' in the \brief
> >
> > > + * \param id The pad globally unique id
> > > + * \return The pad with id \a id
> > > + * \return nullptr if not pad with \id is found
> >
> > @s@\id@\a id@
> >
> > > + */
> > > +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
> >
> > Same as for MediaPad::MediaPad, mention the parameter is the kernels
> > 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
> >
> > How about:
> >
> > \brief: Cleans up all resources allocated by the MediaEntity
> >
> 
> Yes, better (this and all the above comments)
> 
> > > + */
> > > +MediaEntity::~MediaEntity()
> > > +{
> > > +	for (MediaPad *p : pads_)
> > > +		delete p;
> > > +	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',
> > >  ])
> > >
> > > --
> > > 2.20.1
> > >
> > > _______________________________________________
> > > libcamera-devel mailing list
> > > libcamera-devel@lists.libcamera.org
> > > https://lists.libcamera.org/listinfo/libcamera-devel
> >
> > --
> > Regards,
> > Niklas Söderlund

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@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@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',
>  ])

Hi Niklas,

On Sun, Dec 30, 2018 at 11:47:56PM +0100, Niklas Söderlund wrote:
> Hi Jacopo,
>
> On 2018-12-30 23:10:57 +0100, Jacopo Mondi wrote:
> > Hi Niklas,
> >    thanks for review
> >
> > On Sun, Dec 30, 2018 at 10:50:15PM +0100, Niklas Söderlund wrote:
> > > Hi Jacopo,
> > >
> > > Thanks for your patch.
> > >
> > > On 2018-12-30 15:23:12 +0100, 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@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);
> > >
> > > Why not use references here? Would it be valid to create a MediaLink
> > > where any of the parameters are nullptr?
> >
> > I feel it is better to use pointers for dynamically allocated objects
> > as this class entities are. The caller will have pointers to them, and
> > I should dereference them to access their value and pass it by
> > reference.
> >
> > As I've replied to your series, I have not found this stated anywhere,
> > so it might just be my preference, but I agree this protects us from
> > nullptr values.
> >
> > >
> > > Same question for other constructors in this patch.
> > >
> >
> > Could I keep them like this until we don't try establish a firm rule?
>
> I have no problem keeping them, but if you want to keep them you should
> ensure they are not null before dereferencing them.
>

The caller checks for those pointers validity, and makes sure all
these objects are initialized with valid pointers.

Let me point out that using references, and thus accessing the
pointer's value, if the caller provides a nullptr won't help:

                MediaPad *source == nullprt;
                MediaPad *source == nullprt;
                new MediaLink(link, *source, *sink)

Will obviusly segault when the function is called.

Thanks
  j

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@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@ideasonboard.com>
>

Thanks, I'll send and push v4 shortly.