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

[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 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);
> >
> > 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 at lists.libcamera.org
> > > https://lists.libcamera.org/listinfo/libcamera-devel
> >
> > --
> > Regards,
> > Niklas Söderlund



-- 
Regards,
Niklas Söderlund