[libcamera-devel] [PATCH v3 1/3] libcamera: Add MediaObject class hierarchy
Niklas Söderlund
niklas.soderlund at ragnatech.se
Sun Dec 30 22:50:15 CET 2018
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?
Same question for other constructors in this patch.
> + 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/
> + * 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.
> + */
> +
> +/**
> + * \var MediaObject::id_
> + * \brief The MediaObject unique id
> + */
Should we document private variables which do not require special
explanations for one reason or another?
> +
> +/**
> + * \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?
> +
> +/**
> + * \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?
> +
> +/**
> + * \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?
> + * \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.
> + */
> +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.
> + * \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
> + */
> +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
More information about the libcamera-devel
mailing list