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

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

--
2.20.1

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

HI Laurent,
  thanks for review.

On Fri, Dec 28, 2018 at 12:51:23PM +0200, Laurent Pinchart wrote:
> 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@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.
>
> > +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

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

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

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

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

>

Thanks
   j

> --
> Regards,
>
> 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@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...

Hi Laurent

On Fri, Dec 28, 2018 at 03:50:27PM +0200, Laurent Pinchart wrote:

[snip]

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

I really don't want to break the encapsulation this class hierarchy
provides. MediaDevice creates and manages a hierarchy of MediaObjects,
but MediaObjects should not go and call into the media device. If it
happens there's something fishy in the design and I would like to keep
this rule firm. Also the mediaObjects_ array contains all objects, entities,
pads and links, as "MediaObject *". I should walk all of it every time
to match ids.

I'm fine with a single mediaObject_ vector in MediaDevice as a general
pool where to retrieve stuff from by id (in the media device itself), but
for this classes I would like to keep the current structure, where
entities have pads, and pads have links. They should provide accessors
to those, but that's it, I don't want them to fish from the global
object pool.

Am I overthinking it?