[libcamera-devel] [PATCH RESEND v2 1/4] libcamera: Add MediaObject class hierarchy

[Laurent Pinchart]

Hi Jacopo,

On Friday, 28 December 2018 15:11:15 EET Jacopo Mondi wrote:
> On Fri, Dec 28, 2018 at 12:51:23PM +0200, Laurent Pinchart wrote:
> > On Friday, 28 December 2018 09:57:40 EET Jacopo Mondi wrote:
> > > Add a class hierarcy to represent all media objects a media graph
> > > represents. Add a base MediaObject class, which retains the global
> > > unique object id, and define the derived MediaEntity, MediaLink and
> > > MediaPad classes.
> > > 
> > > This hierarchy will be used by the MediaDevice objects which represents
> > > and handles the media graph.
> > > 
> > > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > > ---
> > > 
> > >  src/libcamera/include/media_object.h | 117 +++++++++++
> > >  src/libcamera/media_object.cpp       | 302 +++++++++++++++++++++++++++
> > >  src/libcamera/meson.build            |   1 +
> > >  3 files changed, 420 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..99913eb
> > > --- /dev/null
> > > +++ b/src/libcamera/include/media_object.h
> > > @@ -0,0 +1,117 @@
> > > +/* 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 <functional>
> > > +#include <string>
> > > +#include <sstream>
> > 
> > s comes before t :-) I think you can remove <sstream> anyway, it doesn't
> > seem to be used.
> 
> Argh, could be a leftover! I'll remove (or re-sort at least)
> 
> > > +#include   <vector>
> > > +
> > > +#include   <linux/media.h>
> > 
> > If this is just for the definition of struct media_v2_entity, can't it be
> > forward-declared instead ?
> 
> Well, the media_object.cpp file that includes this header is going to
> include it anyhow, and I don't think it's bad to include it here to
> make clear where media_v2_entity comes from.

In this specific case it's not a big deal, but in general you should try to 
include as few headers as possible and use forward declarations instead. For 
that reason I would apply that rule globally, including here.

> > > +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 MediaLink : public MediaObject
> > > +{
> > > +	friend class MediaDevice;
> > > +
> > > +public:
> > > +	~MediaLink() { }
> > > +
> > > +	unsigned int source() const { return source_; }
> > > +	unsigned int 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);
> > > +	MediaLink(const MediaLink &) = delete;
> > > +
> > > +	unsigned int source_;
> > > +	unsigned int sink_;
> > > +	unsigned int flags_;
> > > +};
> > > +
> > > +class MediaPad : public MediaObject
> > > +{
> > > +	friend class MediaDevice;
> > > +
> > > +public:
> > > +	~MediaPad();
> > > +
> > > +	unsigned int index() const { return index_; }
> > > +	unsigned int 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);
> > > +	MediaPad(const MediaPad &) = delete;
> > > +
> > > +	unsigned int index_;
> > > +	unsigned int entity_;
> > > +	unsigned int flags_;
> > > +
> > > +	std::vector<MediaLink *> links_;
> > > +};
> > > +
> > > +class MediaEntity : public MediaObject
> > > +{
> > > +	friend class MediaDevice;
> > > +
> > > +public:
> > > +	bool operator==(unsigned int id) const { return this->id_ == id; }
> > > +	bool operator!=(unsigned int id) const { return this->id_ != id; }
> > > +	bool operator==(std::string name) const { return name_ == name; }
> > 
> > As stated before, I wouldn't define those operators. They're more
> > confusing
> > 
> > than typing code explicitly. Compare the two following implementations:
> > 	MediaEntity *entity = ...;
> > 	
> > 	if (entity == 3) {
> > 		...
> > 	} else if (entity == "foo") {
> > 		...
> > 	}
> > 
> > vs.
> > 
> > 	MediaEntity *entity = ...;
> > 	
> > 	if (entity->id() == 3) {
> > 		...
> > 	} else if (entity->name() == "foo") {
> > 		...
> > 	}
> 
> I see. They seemed quite natural to me, but I can remove them
> 
> > The second is easier to read and more self-explicit, without requiring
> > much
> > more code in the user of the class.
> > 
> > > +	const std::string name() const { return name_; }
> > 
> > You can return a const reference instead of a copy.
> 
> Right, this slipped through the cracks
> 
> > > +	const std::vector<MediaPad *> &sources() const { return sources_; }
> > > +	const std::vector<MediaPad *> &sinks() const { return sinks_; }
> > > +
> > > +	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 *> sources_;
> > > +	std::vector<MediaPad *> sinks_;
> > 
> > I think the implementation would be simpler if you merged all pads in a
> > single vector. The getPadByIndex() function would just be a vector
> > lookup. The sources() and sinks() functions would on the other hand need
> > to return std::vector<> instead of std::vector<>&, but I think that's
> > fine. Depending on how you use them in the callers, you could create a
> > pads() function instead. Given that pads could be bidirectional (unless
> > I'm mistaken there's nothing in the MC API that prevents the source and
> > sink flags being both set) I think this would be a better API.
> 
> I think you're right. When I started this I had much more use cases
> for retrieving sinks or sources only. Right now is just when dumping
> the media graph, but the caller can get all pads and skip the ones it
> is not interested in. Yes, that would really simplify the API.
> 
> > > +	int setDevice(const std::string &path);
> > > +
> > > +	void addPad(MediaPad *pad);
> > > +
> > > +	const MediaPad *__getPad(std::vector<MediaPad *> &v,
> > > +			   std::function<bool(MediaPad *)> f);
> > 
> > The __ prefix is reserved in C/C++.
> 
> ? For what ? I read online for "compiler vendors"... I used it in so
> many places... I'll drop

It's for compiler vendors indeed. A compiler (and the related standard 
libraries) can freely define names starting with two underscores, so they're 
reserved.

> > > +	const MediaPad *getPad(std::function<bool(MediaPad *)> f);
> > > +};
> > > +
> > > +} /* 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..65ed421
> > > --- /dev/null
> > > +++ b/src/libcamera/media_object.cpp
> > > @@ -0,0 +1,302 @@
> > > +/* 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 <functional>
> > > +#include <string>
> > > +#include <vector>
> > > +
> > > +#include <linux/media.h>
> > > +
> > > +#include "log.h"
> > > +#include "media_object.h"
> > > +
> > > +/**
> > > + * \file media_object.h
> > > + */
> > > +namespace libcamera {
> > > +
> > > +/**
> > > + * \class MediaObject
> > > + * \brief Base class for all media object types
> > > + *
> > > + * Defines a simple base class for all media objects with a simple
> > > + * unique id.
> > 
> > You may want to expand this a bit, or possibly the \file documentation
> > block, to explain that the media objects model their kernel-side
> > counterpart from the MC API.
> > 
> > > + */
> > > +
> > > +/**
> > > + * \fn MediaObject::MediaObject(unsigned int id)
> > > + * \brief Construct a MediaObject with id \a id
> > > + * \param id The globally unique object's id as returned by
> > > MEDIA_IOC_G_TOPOLOGY
> > 
> > Shouldn't it be "object id" ?
> > 
> > Maybe s/returned by/obtained from/ ?
> > 
> > If you document the fact that these objects model the MC objects in the
> > \file or \class, I think you can skip the "as returned by
> > MEDIA_IOC_G_TOPOLOGY" here and in all locations below.
> > 
> > > + */
> > > +
> > > +/**
> > > + * \fn MediaObject::id()
> > > + * \brief Return the object's globally unique id /ref id_
> > 
> > Ditto.
> > 
> > /ref ?
> 
> Ups, I'll drop
> 
> > > + */
> > > +
> > > +/**
> > > + * \var MediaObject::id_
> > > + * \brief The MediaObject unique id as returned by MEDIA_IOC_G_TOPOLOGY
> > > + */
> > 
> > Do we need to document the private data members ? I suppose it can be
> > useful,
> 
> Good question. We might comment if appropriate but leave the doxygen
> out?
> 
> > but there's lots of overlap between the constructor, id() and id_
> > documentation. I don't want to see documentation being written just to
> > tick a box, it should be useful if present.
> 
> I know this was over commented, but sometimes is difficult to
> find real value when you have to comment everything (if they were
> public, we should have documented that anyhow)

I agree, sometimes there's very little value in documenting some of the 
functions. I don't know how to solve this problem yet.

> > > +/**
> > > + * \class MediaLink
> > > + * \brief A Media Link object
> > > + *
> > > + * A MediaLink object represents a media link between two entities
> > 
> > Isn't it between two pads ?
> 
> Yap
> 
> > > + */
> > > +
> > > +/**
> > > + * \fn MediaLink::MediaLink(const struct media_v2_link *link)
> > > + * \brief Construct a MediaLink with informations from \a link
> > 
> > s/informations/information/
> > 
> > > + * \param link The media link representation as returned by
> > > + *	       MEDIA_IOC_G_TOPOLOGY
> > > + */
> > > +MediaLink::MediaLink(const struct media_v2_link *link) :
> > > MediaObject(link->id),
> > > +							 source_(link->source_id),
> > > +							 sink_(link->sink_id),
> > > +							 flags_(link->flags) { }
> > 
> > The indentation is quite peculiar :-) I think it should be (with spaces
> > instead of tabs to ensure it won't be mangled by mail clients)
> > 
> > MediaLink::MediaLink(const struct media_v2_link *link)
> > 
> >         : MediaObject(link->id), source_(link->source_id),
> >         : 
> >           sink_(link->sink_id), flags_(link->flags)
> > 
> > {
> 
> Eh, I had no clear idea how to deal with them.
> I'm not sure I quite like your proposed one neither, but it should at
> least silence the checkstyle tool, so I'm going for that.
> 
> > }
> > 
> > > +/**
> > > + * \fn MediaLink::source()
> > > + * \brief Return the source pad id
> > > + */
> > > +
> > > +/**
> > > + * \fn MediaLink::sink()
> > > + * \brief Return the sink pad id
> > > + */
> > > +
> > > +/**
> > > + * \fn MediaLink::flags()
> > > + * \brief Return the link flags
> > > + */
> > > +
> > > +/**
> > > + * \fn MediaLink::setFlags(unsigned int flags)
> > > + * \brief Set the link flags to \a flags
> > > + * \param flags The flags to be applied to the link
> > > + */
> > > +
> > > +/**
> > > + * \class MediaPad
> > > + * \brief Media Pad object
> > > + *
> > > + * A MediaPad object represents a media pad with its associated links
> > > + */
> > > +
> > > +/**
> > > + * \fn MediaPad::MediaPad(const struct media_v2_pad *pad)
> > > + * \brief Create a MediaPad object
> > > + * \param mediaPad The media pad representation as returned by
> > > + *		   MEDIA_IOC_G_TOPOLOGY
> > 
> > The argument name is pad, not mediaPad.
> 
> Thanks
> 
> > > + */
> > > +MediaPad::MediaPad(const struct media_v2_pad *pad) :
> > > MediaObject(pad->id),
> > > +						     index_(pad->index),
> > > +						     entity_(pad->entity_id),
> > > +						     flags_(pad->flags) { }
> > 
> > Indentation here too.
> > 
> > > +MediaPad::~MediaPad()
> > > +{
> > > +	links_.clear();
> > > +}
> > > +
> > > +/**
> > > + * \fn MediaPad::index()
> > > + * \brief Return the 0-indexed pad index
> > > + */
> > > +
> > > +/**
> > > + * \fn MediaPad::entity()
> > > + * \brief Return the entity id this pad belongs to
> > > + */
> > > +
> > > +/**
> > > + * \fn MediaPad::flags()
> > > + * \brief Return the pad flags (MEDIA_PAD_FL_*)
> > > + */
> > > +
> > > +/**
> > > + * \fn MediaPad::links()
> > > + * \brief Return all outbound and inbound links from/to this pad
> > > + */
> > > +
> > > +/**
> > > + * \fn MediaPad::addLink(MediaLink *link)
> > > + * \brief Add a new outbound or inbound link from/to this pad
> > > + * \param link The new link to add
> > > + */
> > > +void MediaPad::addLink(MediaLink *link)
> > > +{
> > > +	links_.push_back(link);
> > > +}
> > > +
> > > +/**
> > > + * \class MediaEntity
> > > + * \brief Media entity object
> > > + *
> > > + * A MediaEntity object represents a media entity with its id, name and
> > > its + * associated pads
> > > + */
> > > +
> > > +/**
> > > + * \fn MediaEntity::operator==(unsigned int id) const
> > > + * \brief Compare entities by id (check if they're equal)
> > > + * \param id The entity id to compare with
> > > + */
> > > +
> > > +/**
> > > + * \fn MediaEntity::operator!=(unsigned int id) const
> > > + * \brief Compare entities by id (check if they're not equal)
> > > + * \param id The entity id to compare with
> > > + */
> > > +
> > > +/**
> > > + * \fn MediaEntity::operator==(std::string name) const
> > > + * \brief Compare entities by name (check if they're equal)
> > > + * \param name The entity name to compare with
> > > + */
> > > +
> > > +/**
> > > + * \fn MediaEntity::name()
> > > + * \brief Return the entity name
> > > + */
> > > +
> > > +/**
> > > + * \fn MediaEntity::sources()
> > > + * \brief Get all source pads
> > 
> > Some getters are documented as "Return ...", others as "Get ...". Should
> > we standardize on one of the two ?
> 
> Yes, I would go for 'Return'
> 
> > > + */
> > > +
> > > +/**
> > > + * \fn MediaEntity::sinks()
> > > + * \brief Get all sink pads
> > > + */
> > > +
> > > +/**
> > > + * \fn MediaEntity::getPadByIndex(unsigned int index)
> > > + * \brief Get a pad in this entity by its index
> > > + * \param index The pad index (starting from 0)
> > > + */
> > > +const MediaPad *MediaEntity::getPadByIndex(unsigned int index)
> > > +{
> > > +	return getPad([&index](MediaPad *p) -> bool { return p->index() ==
> > > index;
> > > });
> > > +}
> > > +
> > > +/**
> > > + * \fn MediaEntity::getPadById(unsigned int id)
> > > + * \brief Get a pad in this entity by its id
> > > + * \param id The pad globally unique id
> > > + */
> > > +const MediaPad *MediaEntity::getPadById(unsigned int id)
> > > +{
> > > +	return getPad([&id](MediaPad *p) -> bool { return p->id() == id; });
> > 
> > Would it be more efficient to store all objects in a map in MediaObject
> > and have this function look up the pad from there ? Given the limited
> > number of pads it shouldn't matter too much so I'm OK with this
> > implementation. I don't like the __getPad() and getPad() with
> > std::function much though, but if you merge the sink and source vectors
> > like proposed above it would make all this simpler.
> 
> Actually, I don't think it's worth setting up a map for such a few
> entities.

I meant in MediaDevice, not MediaObject, sorry. There's already a 
mediaObjects_ map there, which we could use to look up the pad.

> On using lamdas, I don't think it's a bid deal here, it's
> quite clear what happens, but maybe it's just me wanting to play around
> with new toys, the actual gain in code size is negligible.
> 
> > > +}
> > > +
> > > +/**
> > > + * \fn MediaEntity::MediaEntity(const struct media_v2_entity *entity)
> > > + * \brief Construct a MediaEntity with informations from \a entity
> > > + * \param entity The media entity representation as returned by
> > > + *		 MEDIA_IOC_G_TOPOLOGY
> > > + */
> > > +MediaEntity::MediaEntity(const struct media_v2_entity *entity) :
> > > +							MediaObject(entity->id),
> > > +							name_(entity->name) { }
> > > +
> > 
> > Indentation.
> > 
> > > +/**
> > > + * \fn MediaEntity::~MediaEntity()
> > > + * \brief Release memory for all pads and links
> > > + */
> > > +MediaEntity::~MediaEntity()
> > > +{
> > > +	for (MediaPad *s : sources_)
> > > +		delete s;
> > > +	for (MediaPad *s : sinks_)
> > > +		delete s;
> > > +
> > > +	sources_.clear();
> > > +	sinks_.clear();
> > > +}
> > > +
> > > +/**
> > > + * \var MediaEntity::sources_
> > > + * \brief The MediaPad sources vector
> > > + */
> > > +
> > > +/**
> > > + * \var MediaEntity::sinks_
> > > + * \brief The MediaPad sinks vector
> > > + */
> > > +
> > > +/**
> > > + * \fn MediaEntity::setDevice(const std::string &path)
> > > + * \brief Set the entity video (sub)device node path
> > > + * \param path The video (sub)device node path associated with this
> > > entity
> > 
> > I'd talk about "device node" instead of "video (sub)device node" to keep
> > this generic, as entities could have non-video nodes. I would then name
> > the function setDeviceNode().
> > 
> > > + */
> > > +int MediaEntity::setDevice(const std::string &path)
> > 
> > s/path/devnode/ ?
> > 
> > > +{
> > > +	/* Make sure the path exists first. */
> > > +	struct stat pstat;
> > > +	int ret = ::stat(const_cast<const char *>(path.c_str()), &pstat);
> > 
> > Do you need the cast ?
> 
> I think g++ complains otherwise.

You don't case when calling ::open(), I wonder why one would succeed and the 
other one wouldn't. Could you please double-check ?

> > > +	if (ret < 0) {
> > > +		LOG(Error) << "Unable to open: " << path << ": "
> > > +			   << strerror(errno);
> > 
> > It's stat(), not open().
> > 
> > I wonder whether the check shouldn't be moved to the caller.
> > 
> > > +		return -errno;
> > > +	}
> > > +
> > > +	devnode_ = path;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +/**
> > > + * \fn MediaEntity::addPad(MediaPad *pad)
> > > + * \brief Add pad \a pad to \ref sources_ or \ref sinks_
> > 
> > You can write "Add \a pad", there's no need to duplicate the name. The
> > comment holds true for various other locations in this patch series.
> > 
> > > + * \param pad The pad to add
> > > + */
> > > +void MediaEntity::addPad(MediaPad *pad)
> > > +{
> > > +	std::vector<MediaPad *> *pads =
> > > +				 pad->flags() & MEDIA_PAD_FL_SOURCE ?
> > > +				 &sources_ : &sinks_;
> > > +	pads->push_back(pad);
> > > +}
> > > +
> > > +/**
> > > + * \fn MediaEntity::__getPad(std::vector<MediaPad *> &v,
> > > + *			     std::function<bool(MediaPad *)> f)
> > > + * \brief Find MediaPad the satisfies predicates \a f in the pad vector
> > > \v
> > > + * \param v The std::vector<MediaPad *> to search in
> > > + * \param f The predicate the pad has to satisfy
> > > + */
> > > +const MediaPad *MediaEntity::__getPad(std::vector<MediaPad *> &v,
> > > +				      std::function<bool(MediaPad *)> f)
> > > +{
> > > +	auto it = v.begin();
> > > +	while (it != sources_.end()) {
> > > +		if (f(*it))
> > > +			return *it;
> > > +		++it;
> > > +	}
> > 
> > How about
> > 
> > 	for (MediaPad *pad : v) {
> > 		if (f(pad))
> > 			return pad;
> > 	}
> 
> Not sure why I kept iterators...

New toy ? :-)

> > > +
> > > +	return nullptr;
> > > +}
> > > +
> > > +/**
> > > + * \fn MediaEntity::getPad(std::function<bool(MediaPad *)> f)
> > > + * \brief Run predicate \a f on both \ref sources_ and \ref sinks_
> > > + * \param f The predicate the pad has to satisfy
> > 
> > As stated above I find this a bit overengineered. Hopefully the code can
> > be simplified with merging the sink and source vectors. Otherwise you need
> > a more detailed documentation block to explain what this function does.
> > 
> > > + */
> > > +const MediaPad *MediaEntity::getPad(std::function<bool(MediaPad *)> f)
> > > +{
> > > +	const MediaPad *_p = __getPad(sources_, f);
> > > +	return (_p ? _p : __getPad(sinks_, f));
> > > +}
> 
> Maybe just keep a getPad() that goes on the now single pad vector but
> still using a predicate to do the matching?

If the getPadById() function is reworked to use the mediaObjects_ map, 
getPadByIndex() can be merged with getPad(), and you won't need a predicate.

> > > +
> > > +} /* namespace libcamera */
> > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > > index f632eb5..da06eba 100644
> > > --- a/src/libcamera/meson.build
> > > +++ b/src/libcamera/meson.build
> > > @@ -1,6 +1,7 @@
> > > 
> > >  libcamera_sources = files([
> > >  
> > >      'log.cpp',
> > >      'main.cpp',
> > > 
> > > +    'media_object.cpp',
> > > 
> > >  ])
> > >  
> > >  libcamera_headers = files([
> 
> I missed to add media_object.h (and media_device.h) here I guess...

-- 
Regards,

Laurent Pinchart