[libcamera-devel] [PATCH v3 1/3] libcamera: Add MediaObject class hierarchy
Jacopo Mondi
jacopo at jmondi.org
Sun Dec 30 23:10:57 CET 2018
Hi Niklas,
thanks for review
On Sun, Dec 30, 2018 at 10:50:15PM +0100, Niklas Söderlund wrote:
> Hi Jacopo,
>
> Thanks for your patch.
>
> On 2018-12-30 15:23:12 +0100, 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 | 107 ++++++++++
> > src/libcamera/media_object.cpp | 281 +++++++++++++++++++++++++++
> > src/libcamera/meson.build | 2 +
> > 3 files changed, 390 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..118d0d8
> > --- /dev/null
> > +++ b/src/libcamera/include/media_object.h
> > @@ -0,0 +1,107 @@
> > +/* 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 <string>
> > +#include <vector>
> > +
> > +#include <linux/media.h>
> > +
> > +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 MediaPad;
> > +class MediaLink : public MediaObject
> > +{
> > + friend class MediaDevice;
> > +
> > +public:
> > + ~MediaLink() { }
> > +
> > + MediaPad *source() const { return source_; }
> > + MediaPad *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,
> > + MediaPad *source, MediaPad *sink);
>
> Why not use references here? Would it be valid to create a MediaLink
> where any of the parameters are nullptr?
I feel it is better to use pointers for dynamically allocated objects
as this class entities are. The caller will have pointers to them, and
I should dereference them to access their value and pass it by
reference.
As I've replied to your series, I have not found this stated anywhere,
so it might just be my preference, but I agree this protects us from
nullptr values.
>
> Same question for other constructors in this patch.
>
Could I keep them like this until we don't try establish a firm rule?
> > + MediaLink(const MediaLink &) = delete;
> > +
> > + MediaPad *source_;
> > + MediaPad *sink_;
> > + unsigned int flags_;
> > +};
> > +
> > +class MediaEntity;
> > +class MediaPad : public MediaObject
> > +{
> > + friend class MediaDevice;
> > +
> > +public:
> > + ~MediaPad();
> > +
> > + unsigned int index() const { return index_; }
> > + MediaEntity *entity() const { return entity_; }
> > + unsigned int flags() const { return flags_; }
> > + const std::vector<MediaLink *> &links() const { return links_; }
> > +
> > + void addLink(MediaLink *link);
>
> Same here why not use a possibly const reference?
>
> > +
> > +private:
> > + MediaPad(const struct media_v2_pad *pad, MediaEntity *entity);
> > + MediaPad(const MediaPad &) = delete;
> > +
> > + unsigned int index_;
> > + MediaEntity *entity_;
> > + unsigned int flags_;
> > +
> > + std::vector<MediaLink *> links_;
> > +};
> > +
> > +class MediaEntity : public MediaObject
> > +{
> > + friend class MediaDevice;
> > +
> > +public:
> > + const std::string &name() const { return name_; }
> > +
> > + const std::vector<MediaPad *> &pads() { return pads_; }
> > +
> > + 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 *> pads_;
> > +
> > + void addPad(MediaPad *pad);
> > +};
> > +
> > +} /* 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..f0906e8
> > --- /dev/null
> > +++ b/src/libcamera/media_object.cpp
> > @@ -0,0 +1,281 @@
> > +/* 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 <string>
> > +#include <vector>
> > +
> > +#include <linux/media.h>
> > +
> > +#include "log.h"
> > +#include "media_object.h"
> > +
> > +/**
> > + * \file media_object.h
> > + *
> > + * In the media object file is implemented a class hierarchy that
>
> s/In the media object file is implemented a/Provide a/
>
> > + * represent the counterpart of media objects exposed by the kernel's
>
> s/counterpart of/corresponding/
>
> > + * media controller APIs.
> > + *
> > + * Media Objects here represented are media entities, media pads and
>
> s/here represented/represented here/
>
Thanks
> > + * media links, created with informations as obtained by the
> > + * MEDIA_IOC_G_TOPOLOGY ioctl call.
> > + *
> > + * MediaLink, MediaPad and MediaEntity are derived classes of the virtual
> > + * base class MediaObject, which maintains a globally unique id for each object.
> > + *
> > + * Media objects have private constructors to restrict the number of classes
> > + * that can instantiate them only to the 'friend' MediaDevice one. Media
> > + * objects are in facts created when a MediaDevice gets populated.
> > + */
> > +
> > +namespace libcamera {
> > +
> > +/**
> > + * \class MediaObject
> > + * \brief Base class for all media object types
> > + *
> > + * MediaObject is the virtual base class for all media objects in the
> > + * media graph. Each object has a globally unique id assigned, and this
> > + * base class provides that.
> > + *
> > + * MediaEntities, MediaPads and MediaLink are MediaObject derived classes.
> > + */
> > +
> > +/**
> > + * \fn MediaObject::MediaObject()
> > + * \brief Construct a MediaObject with \a id
> > + * \param id The globally unique object id
> > + */
> > +
> > +/**
> > + * \fn MediaObject::id()
> > + * \brief Return the object's globally unique id
>
> Only asking if the \return key word should not be used what is returned
> by the function and \brief describing the action performed.?
>
> \brief Retrieve the media objects global id
> \returns Media objects global id
>
> I know it seems silly for such a simple function but still I like the
> \return keyword as it's easy to spot when reading the documentation.
>
> Same comment on all functions returning something which is described
> using the \brief keyword.
>
I agree on both points, it's better, but still silly for such simply
methods :)
> > + */
> > +
> > +/**
> > + * \var MediaObject::id_
> > + * \brief The MediaObject unique id
> > + */
>
> Should we document private variables which do not require special
> explanations for one reason or another?
>
It's protected, I think it's worth documenting it (doxygen wants it to
be commented...)
> > +
> > +/**
> > + * \class MediaLink
> > + * \brief Media Link object
> > + *
> > + * A MediaLink represents a 'struct media_v2_link', with associated
> > + * references to the MediaPad it connects and an internal status defined
> > + * by the MEDIA_LNK_FL_* flags captured in flags_.
> > + *
> > + * MediaLink can be enabled enabled and disabled, with the exception of
>
> s/enabled enabled/enabled/
>
> > + * immutable links (see MEDIA_LNK_FL_IMMUTABLE).
> > + *
> > + * MediaLinks are created between MediaPads inspecting the media graph
> > + * topology and are stored in both the source and sink MediaPad they
> > + * connect.
> > + */
> > +
> > +/**
> > + * \brief Construct a MediaLink
> > + * \param link The media link representation
> > + * \param source The MediaPad source
> > + * \param sink The MediaPad sink
> > + */
> > +MediaLink::MediaLink(const struct media_v2_link *link, MediaPad *source,
> > + MediaPad *sink)
> > + : MediaObject(link->id), source_(source),
> > + sink_(sink), flags_(link->flags)
> > +{
> > +}
> > +
> > +/**
> > + * \fn MediaLink::source()
> > + * \brief Return the source MediaPad
> > + */
> > +
> > +/**
> > + * \fn MediaLink::sink()
> > + * \brief Return the sink MediaPad
> > + */
> > +
> > +/**
> > + * \fn MediaLink::flags()
> > + * \brief Return the link flags
> > + */
>
> Is it useful to add documentation for these trivial getters?
>
If doxygen compiler is happy, I'm happy :)
I agree going forward we could relax this, but since I already wrote
it I would keep it here.
> > +
> > +/**
> > + * \fn MediaLink::setFlags()
> > + * \brief Set the link flags to \a flags
> > + * \param flags The flags to be applied to the link
> > + */
>
> I'm not sure why this function is need? I can find no references to it
> on the rest of this series. Maybe it's used by code yet to be posted.
> Could you drop it from this series or expand the documentation to it's
> intended purpose?
>
Leftover and not used here, you're right. I'll drop.
> > +
> > +/**
> > + * \class MediaPad
> > + * \brief Media Pad object
> > + *
> > + * MediaPads represent a 'struct media_v2_pad', with associated a list of
>
> s/associated a/associated/
>
> > + * links and a reference to the entity they belong to.
> > + *
> > + * MediaPads are connected to other MediaPads by MediaLinks, created
> > + * inspecting the media graph topology. Data connection between pads can be
> > + * enabled or disabled operating on the link that connects the two. See
> > + * MediaLink.
> > + *
> > + * MediaPads are either 'source' pads, or 'sink' pads. This information is
> > + * captured by the MEDIA_PAD_FL_* flags as stored in the flags_ member
> > + * variable.
> > + *
> > + * Each MediaPad has a list of MediaLinks it is associated to. All links
> > + * in a source pad are outbound links, while all links in a sink pad are
> > + * inbound ones.
> > + *
> > + * Each MediaPad has a reference to the MediaEntity it belongs to.
> > + */
> > +
> > +/**
> > + * \brief Create a MediaPad
> > + * \param pad The media pad representationo
>
> Maybe make it clear that it's the kernels representation of the pad and
> not the libraries?
"The kernel media_v2_pad representation" ?
And I've miss-spelled representationo
>
> > + * \param entity The MediaEntity this pad belongs to
> > + *
> > +MediaPad::MediaPad(const struct media_v2_pad *pad, MediaEntity *entity)
> > + : MediaObject(pad->id), index_(pad->index), entity_(entity),
> > + flags_(pad->flags)
> > +{
> > +}
> > +
> > +/**
> > + * \brief Delete pad.
> > + *
> > + * Delete the pad by deleting its media links. As a reference to the same
> > + * MediaLink gets stored in both the source and the sink pad, only source
> > + * ones actually delete the MediaLink.
>
> Can this create race conditions? If the source pad is deleted before the
> sink there is a dangling pointer alive. Would reference counting be
> needed here or is this a none issue?
>
> I'm thinking of when we get hotplug support and entities in the media
> graph can come and go. If the entity proving the source pad is removed
> the link would be deleted while the sink pad is still alive and pointing
> to the now deleted link.
>
> Can be addressed on top of this if deemed at all to be a issue.
>
I do expect we handle link deletion as well when adding hot-plug support.
For now, I don't think it's an issue though.
> > + */
> > +MediaPad::~MediaPad()
> > +{
> > + for (MediaLink *l : links_)
> > + if (flags_ & MEDIA_PAD_FL_SOURCE)
> > + delete l;
> > +
> > + links_.clear();
> > +}
> > +
> > +/**
> > + * \fn MediaPad::index()
> > + * \brief Return the 0-indexed pad index
> > + */
> > +
> > +/**
> > + * \fn MediaPad::entity()
> > + * \brief Return the MediaEntity this pad belongs to
> > + */
> > +
> > +/**
> > + * \fn MediaPad::flags()
> > + * \brief Return the pad flags (MEDIA_PAD_FL_*)
> > + */
> > +
> > +/**
> > + * \fn MediaPad::links()
> > + * \brief Return all links in this pad.
> > + */
>
> Is it useful to document these trivial getters?
>
> > +
> > +/**
> > + * \brief Add a new link to this pad.
>
> You have not ended other \brief with a period before. Not sure what is
> best, but it should be the same.
Right, not sure what to do. I'll drop dots, as you've not been using
them in your series.
>
> > + * \param link The new link to add
> > + */
> > +void MediaPad::addLink(MediaLink *link)
> > +{
> > + links_.push_back(link);
> > +}
> > +
> > +/**
> > + * \class MediaEntity
> > + * \brief Media entity
> > + *
> > + * A MediaEntity represents a 'struct media_v2_entity' with an associated
> > + * list of MediaPads it contains.
> > + *
> > + * Entities are created inspecting the media graph topology, and MediaPads
> > + * gets added as they are discovered.
> > + *
> > + * TODO: Add support for associating a devnode to the entity when integrating
> > + * with DeviceEnumerator.
> > + */
> > +
> > +/**
> > + * \fn MediaEntity::name()
> > + * \brief Return the entity name
> > + */
> > +
> > +/**
> > + * \fn MediaEntity::pads()
> > + * \brief Return all pads in this entity
> > + */
>
> Same as above.
>
> > +
> > +/**
> > + * \fn MediaEntity::getPadByIndex(unsigned int index)
> > + * \brief Get a pad in this entity by its index
> > + * \return The pad with index \a index
> > + * \return nullptr if no pad is found at \index
>
> @s@\index@\a index@
>
> > + */
> > +const MediaPad *MediaEntity::getPadByIndex(unsigned int index)
> > +{
> > + for (MediaPad *p : pads_)
> > + if (p->index() == index)
> > + return p;
> > +
> > + return nullptr;
> > +}
> > +
> > +/**
> > + * \brief Get a pad in this entity by its id
>
> I would add the words 'globally unique id' in the \brief
>
> > + * \param id The pad globally unique id
> > + * \return The pad with id \a id
> > + * \return nullptr if not pad with \id is found
>
> @s@\id@\a id@
>
> > + */
> > +const MediaPad *MediaEntity::getPadById(unsigned int id)
> > +{
> > + for (MediaPad *p : pads_)
> > + if (p->id() == id)
> > + return p;
> > +
> > + return nullptr;
> > +}
> > +
> > +/**
> > + * \brief Construct a MediaEntity
> > + * \param entity The media entity representation
>
> Same as for MediaPad::MediaPad, mention the parameter is the kernels
> representation.
>
> > + */
> > +MediaEntity::MediaEntity(const struct media_v2_entity *entity)
> > + : MediaObject(entity->id), name_(entity->name)
> > +{
> > +}
> > +
> > +/**
> > + * \fn MediaEntity::~MediaEntity()
> > + * \brief Delete all pads in the entity
>
> How about:
>
> \brief: Cleans up all resources allocated by the MediaEntity
>
Yes, better (this and all the above comments)
> > + */
> > +MediaEntity::~MediaEntity()
> > +{
> > + for (MediaPad *p : pads_)
> > + delete p;
> > + pads_.clear();
> > +}
> > +
> > +/**
> > + * \brief Add \a pad to list of entity's pads
> > + * \param pad The pad to add
> > + */
> > +void MediaEntity::addPad(MediaPad *pad)
> > +{
> > + pads_.push_back(pad);
> > +}
> > +
> > +} /* namespace libcamera */
> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > index f632eb5..01d321c 100644
> > --- a/src/libcamera/meson.build
> > +++ b/src/libcamera/meson.build
> > @@ -1,10 +1,12 @@
> > libcamera_sources = files([
> > 'log.cpp',
> > 'main.cpp',
> > + 'media_object.cpp',
> > ])
> >
> > libcamera_headers = files([
> > 'include/log.h',
> > + 'include/media_object.h',
> > 'include/utils.h',
> > ])
> >
> > --
> > 2.20.1
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
>
> --
> Regards,
> Niklas Söderlund
-------------- 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/20181230/26165d6b/attachment.sig>
More information about the libcamera-devel
mailing list