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

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

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.

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

> +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") {
		...
	}

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.

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

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

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

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

> +/**
> + * \class MediaLink
> + * \brief A Media Link object
> + *
> + * A MediaLink object represents a media link between two entities

Isn't it between two pads ?

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

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

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

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

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

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

> +
> +	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));
> +}
> +
> +} /* 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([

-- 
Regards,

Laurent Pinchart