[libcamera-devel] [PATCH RESEND v2 1/4] libcamera: Add MediaObject class hierarchy
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Dec 28 14:50:27 CET 2018
Hi Jacopo,
On Friday, 28 December 2018 15:11:15 EET Jacopo Mondi wrote:
> On Fri, Dec 28, 2018 at 12:51:23PM +0200, Laurent Pinchart wrote:
> > On Friday, 28 December 2018 09:57:40 EET Jacopo Mondi wrote:
> > > Add a class hierarcy to represent all media objects a media graph
> > > represents. Add a base MediaObject class, which retains the global
> > > unique object id, and define the derived MediaEntity, MediaLink and
> > > MediaPad classes.
> > >
> > > This hierarchy will be used by the MediaDevice objects which represents
> > > and handles the media graph.
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > > ---
> > >
> > > src/libcamera/include/media_object.h | 117 +++++++++++
> > > src/libcamera/media_object.cpp | 302 +++++++++++++++++++++++++++
> > > src/libcamera/meson.build | 1 +
> > > 3 files changed, 420 insertions(+)
> > > create mode 100644 src/libcamera/include/media_object.h
> > > create mode 100644 src/libcamera/media_object.cpp
> > >
> > > diff --git a/src/libcamera/include/media_object.h
> > > b/src/libcamera/include/media_object.h new file mode 100644
> > > index 0000000..99913eb
> > > --- /dev/null
> > > +++ b/src/libcamera/include/media_object.h
> > > @@ -0,0 +1,117 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2018, Google Inc.
> > > + *
> > > + * media_object.h - Media Device objects: entities, pads and links.
> > > + */
> > > +#ifndef __LIBCAMERA_MEDIA_OBJECT_H__
> > > +#define __LIBCAMERA_MEDIA_OBJECT_H__
> > > +
> > > +#include <functional>
> > > +#include <string>
> > > +#include <sstream>
> >
> > s comes before t :-) I think you can remove <sstream> anyway, it doesn't
> > seem to be used.
>
> Argh, could be a leftover! I'll remove (or re-sort at least)
>
> > > +#include <vector>
> > > +
> > > +#include <linux/media.h>
> >
> > If this is just for the definition of struct media_v2_entity, can't it be
> > forward-declared instead ?
>
> Well, the media_object.cpp file that includes this header is going to
> include it anyhow, and I don't think it's bad to include it here to
> make clear where media_v2_entity comes from.
In this specific case it's not a big deal, but in general you should try to
include as few headers as possible and use forward declarations instead. For
that reason I would apply that rule globally, including here.
> > > +namespace libcamera {
> > > +
> > > +class MediaDevice;
> > > +class MediaEntity;
> > > +
> > > +class MediaObject
> > > +{
> > > +public:
> > > + MediaObject(unsigned int id) : id_(id) { }
> > > + virtual ~MediaObject() { }
> > > +
> > > + unsigned int id() const { return id_; }
> > > +
> > > +protected:
> > > + unsigned int id_;
> > > +};
> > > +
> > > +class MediaLink : public MediaObject
> > > +{
> > > + friend class MediaDevice;
> > > +
> > > +public:
> > > + ~MediaLink() { }
> > > +
> > > + unsigned int source() const { return source_; }
> > > + unsigned int sink() const { return sink_; }
> > > + unsigned int flags() const { return flags_; }
> > > + void setFlags(unsigned int flags) { flags_ = flags; }
> > > +
> > > +private:
> > > + MediaLink(const struct media_v2_link *link);
> > > + MediaLink(const MediaLink &) = delete;
> > > +
> > > + unsigned int source_;
> > > + unsigned int sink_;
> > > + unsigned int flags_;
> > > +};
> > > +
> > > +class MediaPad : public MediaObject
> > > +{
> > > + friend class MediaDevice;
> > > +
> > > +public:
> > > + ~MediaPad();
> > > +
> > > + unsigned int index() const { return index_; }
> > > + unsigned int entity() const { return entity_; }
> > > + unsigned int flags() const { return flags_; }
> > > + const std::vector<MediaLink *> &links() const { return links_; }
> > > +
> > > + void addLink(MediaLink *link);
> > > +
> > > +private:
> > > + MediaPad(const struct media_v2_pad *pad);
> > > + MediaPad(const MediaPad &) = delete;
> > > +
> > > + unsigned int index_;
> > > + unsigned int entity_;
> > > + unsigned int flags_;
> > > +
> > > + std::vector<MediaLink *> links_;
> > > +};
> > > +
> > > +class MediaEntity : public MediaObject
> > > +{
> > > + friend class MediaDevice;
> > > +
> > > +public:
> > > + bool operator==(unsigned int id) const { return this->id_ == id; }
> > > + bool operator!=(unsigned int id) const { return this->id_ != id; }
> > > + bool operator==(std::string name) const { return name_ == name; }
> >
> > As stated before, I wouldn't define those operators. They're more
> > confusing
> >
> > than typing code explicitly. Compare the two following implementations:
> > MediaEntity *entity = ...;
> >
> > if (entity == 3) {
> > ...
> > } else if (entity == "foo") {
> > ...
> > }
> >
> > vs.
> >
> > MediaEntity *entity = ...;
> >
> > if (entity->id() == 3) {
> > ...
> > } else if (entity->name() == "foo") {
> > ...
> > }
>
> I see. They seemed quite natural to me, but I can remove them
>
> > The second is easier to read and more self-explicit, without requiring
> > much
> > more code in the user of the class.
> >
> > > + const std::string name() const { return name_; }
> >
> > You can return a const reference instead of a copy.
>
> Right, this slipped through the cracks
>
> > > + const std::vector<MediaPad *> &sources() const { return sources_; }
> > > + const std::vector<MediaPad *> &sinks() const { return sinks_; }
> > > +
> > > + const MediaPad *getPadByIndex(unsigned int index);
> > > + const MediaPad *getPadById(unsigned int id);
> > > +
> > > +private:
> > > + MediaEntity(const struct media_v2_entity *entity);
> > > + MediaEntity(const MediaEntity &) = delete;
> > > + ~MediaEntity();
> > > +
> > > + std::string name_;
> > > + std::string devnode_;
> > > +
> > > + std::vector<MediaPad *> sources_;
> > > + std::vector<MediaPad *> sinks_;
> >
> > I think the implementation would be simpler if you merged all pads in a
> > single vector. The getPadByIndex() function would just be a vector
> > lookup. The sources() and sinks() functions would on the other hand need
> > to return std::vector<> instead of std::vector<>&, but I think that's
> > fine. Depending on how you use them in the callers, you could create a
> > pads() function instead. Given that pads could be bidirectional (unless
> > I'm mistaken there's nothing in the MC API that prevents the source and
> > sink flags being both set) I think this would be a better API.
>
> I think you're right. When I started this I had much more use cases
> for retrieving sinks or sources only. Right now is just when dumping
> the media graph, but the caller can get all pads and skip the ones it
> is not interested in. Yes, that would really simplify the API.
>
> > > + int setDevice(const std::string &path);
> > > +
> > > + void addPad(MediaPad *pad);
> > > +
> > > + const MediaPad *__getPad(std::vector<MediaPad *> &v,
> > > + std::function<bool(MediaPad *)> f);
> >
> > The __ prefix is reserved in C/C++.
>
> ? For what ? I read online for "compiler vendors"... I used it in so
> many places... I'll drop
It's for compiler vendors indeed. A compiler (and the related standard
libraries) can freely define names starting with two underscores, so they're
reserved.
> > > + const MediaPad *getPad(std::function<bool(MediaPad *)> f);
> > > +};
> > > +
> > > +} /* namespace libcamera */
> > > +#endif /* __LIBCAMERA_MEDIA_OBJECT_H__ */
> > > diff --git a/src/libcamera/media_object.cpp
> > > b/src/libcamera/media_object.cpp new file mode 100644
> > > index 0000000..65ed421
> > > --- /dev/null
> > > +++ b/src/libcamera/media_object.cpp
> > > @@ -0,0 +1,302 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2018, Google Inc.
> > > + *
> > > + * media_object.cpp - Media device objects: entities, pads and links
> > > + */
> > > +
> > > +#include <errno.h>
> > > +#include <fcntl.h>
> > > +#include <string.h>
> > > +#include <sys/stat.h>
> > > +
> > > +#include <functional>
> > > +#include <string>
> > > +#include <vector>
> > > +
> > > +#include <linux/media.h>
> > > +
> > > +#include "log.h"
> > > +#include "media_object.h"
> > > +
> > > +/**
> > > + * \file media_object.h
> > > + */
> > > +namespace libcamera {
> > > +
> > > +/**
> > > + * \class MediaObject
> > > + * \brief Base class for all media object types
> > > + *
> > > + * Defines a simple base class for all media objects with a simple
> > > + * unique id.
> >
> > You may want to expand this a bit, or possibly the \file documentation
> > block, to explain that the media objects model their kernel-side
> > counterpart from the MC API.
> >
> > > + */
> > > +
> > > +/**
> > > + * \fn MediaObject::MediaObject(unsigned int id)
> > > + * \brief Construct a MediaObject with id \a id
> > > + * \param id The globally unique object's id as returned by
> > > MEDIA_IOC_G_TOPOLOGY
> >
> > Shouldn't it be "object id" ?
> >
> > Maybe s/returned by/obtained from/ ?
> >
> > If you document the fact that these objects model the MC objects in the
> > \file or \class, I think you can skip the "as returned by
> > MEDIA_IOC_G_TOPOLOGY" here and in all locations below.
> >
> > > + */
> > > +
> > > +/**
> > > + * \fn MediaObject::id()
> > > + * \brief Return the object's globally unique id /ref id_
> >
> > Ditto.
> >
> > /ref ?
>
> Ups, I'll drop
>
> > > + */
> > > +
> > > +/**
> > > + * \var MediaObject::id_
> > > + * \brief The MediaObject unique id as returned by MEDIA_IOC_G_TOPOLOGY
> > > + */
> >
> > Do we need to document the private data members ? I suppose it can be
> > useful,
>
> Good question. We might comment if appropriate but leave the doxygen
> out?
>
> > but there's lots of overlap between the constructor, id() and id_
> > documentation. I don't want to see documentation being written just to
> > tick a box, it should be useful if present.
>
> I know this was over commented, but sometimes is difficult to
> find real value when you have to comment everything (if they were
> public, we should have documented that anyhow)
I agree, sometimes there's very little value in documenting some of the
functions. I don't know how to solve this problem yet.
> > > +/**
> > > + * \class MediaLink
> > > + * \brief A Media Link object
> > > + *
> > > + * A MediaLink object represents a media link between two entities
> >
> > Isn't it between two pads ?
>
> Yap
>
> > > + */
> > > +
> > > +/**
> > > + * \fn MediaLink::MediaLink(const struct media_v2_link *link)
> > > + * \brief Construct a MediaLink with informations from \a link
> >
> > s/informations/information/
> >
> > > + * \param link The media link representation as returned by
> > > + * MEDIA_IOC_G_TOPOLOGY
> > > + */
> > > +MediaLink::MediaLink(const struct media_v2_link *link) :
> > > MediaObject(link->id),
> > > + source_(link->source_id),
> > > + sink_(link->sink_id),
> > > + flags_(link->flags) { }
> >
> > The indentation is quite peculiar :-) I think it should be (with spaces
> > instead of tabs to ensure it won't be mangled by mail clients)
> >
> > MediaLink::MediaLink(const struct media_v2_link *link)
> >
> > : MediaObject(link->id), source_(link->source_id),
> > :
> > sink_(link->sink_id), flags_(link->flags)
> >
> > {
>
> Eh, I had no clear idea how to deal with them.
> I'm not sure I quite like your proposed one neither, but it should at
> least silence the checkstyle tool, so I'm going for that.
>
> > }
> >
> > > +/**
> > > + * \fn MediaLink::source()
> > > + * \brief Return the source pad id
> > > + */
> > > +
> > > +/**
> > > + * \fn MediaLink::sink()
> > > + * \brief Return the sink pad id
> > > + */
> > > +
> > > +/**
> > > + * \fn MediaLink::flags()
> > > + * \brief Return the link flags
> > > + */
> > > +
> > > +/**
> > > + * \fn MediaLink::setFlags(unsigned int flags)
> > > + * \brief Set the link flags to \a flags
> > > + * \param flags The flags to be applied to the link
> > > + */
> > > +
> > > +/**
> > > + * \class MediaPad
> > > + * \brief Media Pad object
> > > + *
> > > + * A MediaPad object represents a media pad with its associated links
> > > + */
> > > +
> > > +/**
> > > + * \fn MediaPad::MediaPad(const struct media_v2_pad *pad)
> > > + * \brief Create a MediaPad object
> > > + * \param mediaPad The media pad representation as returned by
> > > + * MEDIA_IOC_G_TOPOLOGY
> >
> > The argument name is pad, not mediaPad.
>
> Thanks
>
> > > + */
> > > +MediaPad::MediaPad(const struct media_v2_pad *pad) :
> > > MediaObject(pad->id),
> > > + index_(pad->index),
> > > + entity_(pad->entity_id),
> > > + flags_(pad->flags) { }
> >
> > Indentation here too.
> >
> > > +MediaPad::~MediaPad()
> > > +{
> > > + links_.clear();
> > > +}
> > > +
> > > +/**
> > > + * \fn MediaPad::index()
> > > + * \brief Return the 0-indexed pad index
> > > + */
> > > +
> > > +/**
> > > + * \fn MediaPad::entity()
> > > + * \brief Return the entity id this pad belongs to
> > > + */
> > > +
> > > +/**
> > > + * \fn MediaPad::flags()
> > > + * \brief Return the pad flags (MEDIA_PAD_FL_*)
> > > + */
> > > +
> > > +/**
> > > + * \fn MediaPad::links()
> > > + * \brief Return all outbound and inbound links from/to this pad
> > > + */
> > > +
> > > +/**
> > > + * \fn MediaPad::addLink(MediaLink *link)
> > > + * \brief Add a new outbound or inbound link from/to this pad
> > > + * \param link The new link to add
> > > + */
> > > +void MediaPad::addLink(MediaLink *link)
> > > +{
> > > + links_.push_back(link);
> > > +}
> > > +
> > > +/**
> > > + * \class MediaEntity
> > > + * \brief Media entity object
> > > + *
> > > + * A MediaEntity object represents a media entity with its id, name and
> > > its + * associated pads
> > > + */
> > > +
> > > +/**
> > > + * \fn MediaEntity::operator==(unsigned int id) const
> > > + * \brief Compare entities by id (check if they're equal)
> > > + * \param id The entity id to compare with
> > > + */
> > > +
> > > +/**
> > > + * \fn MediaEntity::operator!=(unsigned int id) const
> > > + * \brief Compare entities by id (check if they're not equal)
> > > + * \param id The entity id to compare with
> > > + */
> > > +
> > > +/**
> > > + * \fn MediaEntity::operator==(std::string name) const
> > > + * \brief Compare entities by name (check if they're equal)
> > > + * \param name The entity name to compare with
> > > + */
> > > +
> > > +/**
> > > + * \fn MediaEntity::name()
> > > + * \brief Return the entity name
> > > + */
> > > +
> > > +/**
> > > + * \fn MediaEntity::sources()
> > > + * \brief Get all source pads
> >
> > Some getters are documented as "Return ...", others as "Get ...". Should
> > we standardize on one of the two ?
>
> Yes, I would go for 'Return'
>
> > > + */
> > > +
> > > +/**
> > > + * \fn MediaEntity::sinks()
> > > + * \brief Get all sink pads
> > > + */
> > > +
> > > +/**
> > > + * \fn MediaEntity::getPadByIndex(unsigned int index)
> > > + * \brief Get a pad in this entity by its index
> > > + * \param index The pad index (starting from 0)
> > > + */
> > > +const MediaPad *MediaEntity::getPadByIndex(unsigned int index)
> > > +{
> > > + return getPad([&index](MediaPad *p) -> bool { return p->index() ==
> > > index;
> > > });
> > > +}
> > > +
> > > +/**
> > > + * \fn MediaEntity::getPadById(unsigned int id)
> > > + * \brief Get a pad in this entity by its id
> > > + * \param id The pad globally unique id
> > > + */
> > > +const MediaPad *MediaEntity::getPadById(unsigned int id)
> > > +{
> > > + return getPad([&id](MediaPad *p) -> bool { return p->id() == id; });
> >
> > Would it be more efficient to store all objects in a map in MediaObject
> > and have this function look up the pad from there ? Given the limited
> > number of pads it shouldn't matter too much so I'm OK with this
> > implementation. I don't like the __getPad() and getPad() with
> > std::function much though, but if you merge the sink and source vectors
> > like proposed above it would make all this simpler.
>
> Actually, I don't think it's worth setting up a map for such a few
> entities.
I meant in MediaDevice, not MediaObject, sorry. There's already a
mediaObjects_ map there, which we could use to look up the pad.
> On using lamdas, I don't think it's a bid deal here, it's
> quite clear what happens, but maybe it's just me wanting to play around
> with new toys, the actual gain in code size is negligible.
>
> > > +}
> > > +
> > > +/**
> > > + * \fn MediaEntity::MediaEntity(const struct media_v2_entity *entity)
> > > + * \brief Construct a MediaEntity with informations from \a entity
> > > + * \param entity The media entity representation as returned by
> > > + * MEDIA_IOC_G_TOPOLOGY
> > > + */
> > > +MediaEntity::MediaEntity(const struct media_v2_entity *entity) :
> > > + MediaObject(entity->id),
> > > + name_(entity->name) { }
> > > +
> >
> > Indentation.
> >
> > > +/**
> > > + * \fn MediaEntity::~MediaEntity()
> > > + * \brief Release memory for all pads and links
> > > + */
> > > +MediaEntity::~MediaEntity()
> > > +{
> > > + for (MediaPad *s : sources_)
> > > + delete s;
> > > + for (MediaPad *s : sinks_)
> > > + delete s;
> > > +
> > > + sources_.clear();
> > > + sinks_.clear();
> > > +}
> > > +
> > > +/**
> > > + * \var MediaEntity::sources_
> > > + * \brief The MediaPad sources vector
> > > + */
> > > +
> > > +/**
> > > + * \var MediaEntity::sinks_
> > > + * \brief The MediaPad sinks vector
> > > + */
> > > +
> > > +/**
> > > + * \fn MediaEntity::setDevice(const std::string &path)
> > > + * \brief Set the entity video (sub)device node path
> > > + * \param path The video (sub)device node path associated with this
> > > entity
> >
> > I'd talk about "device node" instead of "video (sub)device node" to keep
> > this generic, as entities could have non-video nodes. I would then name
> > the function setDeviceNode().
> >
> > > + */
> > > +int MediaEntity::setDevice(const std::string &path)
> >
> > s/path/devnode/ ?
> >
> > > +{
> > > + /* Make sure the path exists first. */
> > > + struct stat pstat;
> > > + int ret = ::stat(const_cast<const char *>(path.c_str()), &pstat);
> >
> > Do you need the cast ?
>
> I think g++ complains otherwise.
You don't case when calling ::open(), I wonder why one would succeed and the
other one wouldn't. Could you please double-check ?
> > > + if (ret < 0) {
> > > + LOG(Error) << "Unable to open: " << path << ": "
> > > + << strerror(errno);
> >
> > It's stat(), not open().
> >
> > I wonder whether the check shouldn't be moved to the caller.
> >
> > > + return -errno;
> > > + }
> > > +
> > > + devnode_ = path;
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +/**
> > > + * \fn MediaEntity::addPad(MediaPad *pad)
> > > + * \brief Add pad \a pad to \ref sources_ or \ref sinks_
> >
> > You can write "Add \a pad", there's no need to duplicate the name. The
> > comment holds true for various other locations in this patch series.
> >
> > > + * \param pad The pad to add
> > > + */
> > > +void MediaEntity::addPad(MediaPad *pad)
> > > +{
> > > + std::vector<MediaPad *> *pads =
> > > + pad->flags() & MEDIA_PAD_FL_SOURCE ?
> > > + &sources_ : &sinks_;
> > > + pads->push_back(pad);
> > > +}
> > > +
> > > +/**
> > > + * \fn MediaEntity::__getPad(std::vector<MediaPad *> &v,
> > > + * std::function<bool(MediaPad *)> f)
> > > + * \brief Find MediaPad the satisfies predicates \a f in the pad vector
> > > \v
> > > + * \param v The std::vector<MediaPad *> to search in
> > > + * \param f The predicate the pad has to satisfy
> > > + */
> > > +const MediaPad *MediaEntity::__getPad(std::vector<MediaPad *> &v,
> > > + std::function<bool(MediaPad *)> f)
> > > +{
> > > + auto it = v.begin();
> > > + while (it != sources_.end()) {
> > > + if (f(*it))
> > > + return *it;
> > > + ++it;
> > > + }
> >
> > How about
> >
> > for (MediaPad *pad : v) {
> > if (f(pad))
> > return pad;
> > }
>
> Not sure why I kept iterators...
New toy ? :-)
> > > +
> > > + return nullptr;
> > > +}
> > > +
> > > +/**
> > > + * \fn MediaEntity::getPad(std::function<bool(MediaPad *)> f)
> > > + * \brief Run predicate \a f on both \ref sources_ and \ref sinks_
> > > + * \param f The predicate the pad has to satisfy
> >
> > As stated above I find this a bit overengineered. Hopefully the code can
> > be simplified with merging the sink and source vectors. Otherwise you need
> > a more detailed documentation block to explain what this function does.
> >
> > > + */
> > > +const MediaPad *MediaEntity::getPad(std::function<bool(MediaPad *)> f)
> > > +{
> > > + const MediaPad *_p = __getPad(sources_, f);
> > > + return (_p ? _p : __getPad(sinks_, f));
> > > +}
>
> Maybe just keep a getPad() that goes on the now single pad vector but
> still using a predicate to do the matching?
If the getPadById() function is reworked to use the mediaObjects_ map,
getPadByIndex() can be merged with getPad(), and you won't need a predicate.
> > > +
> > > +} /* namespace libcamera */
> > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > > index f632eb5..da06eba 100644
> > > --- a/src/libcamera/meson.build
> > > +++ b/src/libcamera/meson.build
> > > @@ -1,6 +1,7 @@
> > >
> > > libcamera_sources = files([
> > >
> > > 'log.cpp',
> > > 'main.cpp',
> > >
> > > + 'media_object.cpp',
> > >
> > > ])
> > >
> > > libcamera_headers = files([
>
> I missed to add media_object.h (and media_device.h) here I guess...
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list