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

[Jacopo Mondi]

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 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.
>
> > +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
>
>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20181228/4ec86003/attachment.sig>