[libcamera-devel] [PATCH v3 2/3] libcamera: Add MediaDevice class
Niklas Söderlund
niklas.soderlund at ragnatech.se
Sun Dec 30 23:39:48 CET 2018
Hi Jacopo,
Thanks for your work.
On 2018-12-30 15:23:13 +0100, Jacopo Mondi wrote:
> The MediaDevice object implements handling and configuration of the media
> graph associated with a media device.
>
> The class allows enumeration of all pads, links and entities registered in
> the media graph
>
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
> src/libcamera/include/media_device.h | 61 +++++
> src/libcamera/media_device.cpp | 370 +++++++++++++++++++++++++++
> src/libcamera/meson.build | 2 +
> 3 files changed, 433 insertions(+)
> create mode 100644 src/libcamera/include/media_device.h
> create mode 100644 src/libcamera/media_device.cpp
>
> diff --git a/src/libcamera/include/media_device.h b/src/libcamera/include/media_device.h
> new file mode 100644
> index 0000000..6a9260e
> --- /dev/null
> +++ b/src/libcamera/include/media_device.h
> @@ -0,0 +1,61 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2018, Google Inc.
> + *
> + * media_device.h - Media device handler
> + */
> +#ifndef __LIBCAMERA_MEDIA_DEVICE_H__
> +#define __LIBCAMERA_MEDIA_DEVICE_H__
> +
> +#include <map>
> +#include <sstream>
> +#include <string>
> +#include <vector>
> +
> +#include <linux/media.h>
> +
> +#include "media_object.h"
> +
> +namespace libcamera {
> +
> +class MediaDevice
> +{
> +public:
> + MediaDevice() : fd_(-1) { };
> + ~MediaDevice();
> +
> + const std::string driver() const { return driver_; }
> + const std::string devnode() const { return devnode_; }
> +
> + int open(const std::string &devnode);
> + void close();
This bothers me somewhat. Would it be a valid use-case for a MediaDevice
object to be around while the fd is closed after it have been open()?
Why not rename open() to init() and move all of close() to
~MediaDevice()? Looking at the code bellow there seems to be overlap
between the two.
> +
> + const std::vector<MediaEntity *> &entities() const { return entities_; }
> +
> + int populate();
> +
> +private:
> + std::string driver_;
> + std::string devnode_;
> + int fd_;
> +
> + /*
> + * Global map of media objects (entities, pads, links) associated to
> + * their globally unique id.
> + */
> + std::map<unsigned int, MediaObject *> objects_;
> + MediaObject *getObject(unsigned int id);
> + int addObject(MediaObject *obj);
> + void deleteObjects();
> +
> + /* Global list of media entities in the media graph: lookup by name. */
> + std::vector<MediaEntity *> entities_;
> + MediaEntity *getEntityByName(const std::string &name);
> +
> + int populateEntities(const struct media_v2_topology &topology);
> + int populatePads(const struct media_v2_topology &topology);
> + int populateLinks(const struct media_v2_topology &topology);
> +};
> +
> +} /* namespace libcamera */
> +#endif /* __LIBCAMERA_MEDIA_DEVICE_H__ */
> diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp
> new file mode 100644
> index 0000000..497b12d
> --- /dev/null
> +++ b/src/libcamera/media_device.cpp
> @@ -0,0 +1,370 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2018, Google Inc.
> + *
> + * media_device.cpp - Media device handler
> + */
> +
> +#include <errno.h>
> +#include <fcntl.h>
> +#include <string.h>
> +#include <sys/ioctl.h>
> +#include <unistd.h>
> +
> +#include <string>
> +#include <vector>
> +
> +#include <linux/media.h>
> +
> +#include "log.h"
> +#include "media_device.h"
> +
> +/**
> + * \file media_device.h
> + */
> +namespace libcamera {
> +
> +/**
> + * \class MediaDevice
> + * \brief Media device handler
> + *
> + * MediaDevice represents the graph of media objects associated with a
> + * media device as exposed by the kernel through the media controller APIs.
> + *
> + * The caller is responsible for opening the MediaDevice explicitly before
> + * operating on it, and shall close it when not needed anymore, as access
> + * to the MediaDevice is exclusive.
> + *
> + * A MediaDevice is created empty and gets populated by inspecting the media
> + * graph topology using the MEDIA_IOC_G_TOPOLOGY ioctls. Representation
> + * of each entity, pad and link described are created using MediaObject
> + * derived classes.
> + *
> + * All MediaObject are stored in a global pool, where they could be retrieved
> + * from by their globally unique id.
> + *
> + * References to MediaEntity registered in the graph are stored in a vector
> + * to allow easier by-name lookup, and the list of MediaEntities is accessible.
Accessible for whom?
> + */
> +
> +/**
> + * \brief Close the media device file descriptor and delete all
> object
> + */
> +MediaDevice::~MediaDevice()
> +{
> + if (fd_ != -1)
> + ::close(fd_);
> + deleteObjects();
> +}
> +
> +/**
> + * \fn MediaDevice::driver()
> + * \brief Return the driver name that handles the media graph this object
> + * represents.
> + */
> +
> +/**
> + * \fn MediaDevice::devnode()
> + * \brief Return the media device devnode node associated with this MediaDevice.
> + */
Is it useful to document these two simple getters?
> +
> +/**
> + * \brief Delete all media objects in the MediaDevice.
> + *
> + * Delete all MediaEntities; entities will then delete their pads,
> + * and each source pad will delete links.
> + *
> + * After this function has been called, the media graph will be unpopulated
> + * and its media objects deleted. The media device has to be populated
> + * before it could be used again.
Is there a use-case to delete all objects and re-populate() the graph?
If not can this not be moved to the destructor?
> + */
> +void MediaDevice::deleteObjects()
> +{
> + for (auto const &e : entities_)
> + delete e;
> +
> + objects_.clear();
> + entities_.clear();
> +}
> +
> +/**
> + * \brief Open a media device and retrieve informations from it.
> + * \param devnode The media device node path.
> + *
> + * The function fails if the media device is already open or if either
> + * open or the media device information retrieval operations fail.
> + *
> + * \return Returns 0 for success or a negative error number otherwise.
> + */
> +int MediaDevice::open(const std::string &devnode)
> +{
> + if (fd_ != -1) {
> + LOG(Error) << "MediaDevice already open";
> + return -EBUSY;
> + }
> +
> + int ret = ::open(devnode.c_str(), O_RDWR);
> + if (ret < 0) {
> + ret = -errno;
> + LOG(Error) << "Failed to open media device at " << devnode
> + << ": " << strerror(-ret);
> + return ret;
> + }
> + fd_ = ret;
> + devnode_ = devnode;
> +
> + struct media_device_info info = { };
> + ret = ioctl(fd_, MEDIA_IOC_DEVICE_INFO, &info);
> + if (ret) {
> + ret = -errno;
> + LOG(Error) << "Failed to get media device info "
> + << ": " << strerror(-ret);
As you have easy access to devnode here I would add it to the error
message.
> + return ret;
> + }
> +
> + driver_ = info.driver;
> +
> + return 0;
> +}
> +
> +/**
> + * \brief Close the file descriptor associated with the media device.
> + */
> +void MediaDevice::close()
> +{
> + if (fd_ == -1)
> + return;
> +
> + ::close(fd_);
> + fd_ = -1;
> +}
I think this should be moved/merged to the destructor and this function
removed. If not possible/desirable the code duplication should be
reduced by calling close() in the destructor.
> +
> +/**
> + * \fn MediaDevice::entities()
> + * \brief Return the list of MediaEntity references.
> + */
Needed?
> +
> +/*
> + * Add a new object to the global objects pool and fail if the object
> + * has already been registered.
> + */
> +int MediaDevice::addObject(MediaObject *obj)
> +{
> +
> + if (objects_.find(obj->id()) != objects_.end()) {
> + LOG(Error) << "Element with id " << obj->id()
> + << " already enumerated.";
> + return -EEXIST;
> + }
> +
> + objects_[obj->id()] = obj;
> +
> + return 0;
> +}
> +
> +/*
> + * MediaObject pool lookup by id.
> + */
> +MediaObject *MediaDevice::getObject(unsigned int id)
> +{
> + auto it = objects_.find(id);
> + return (it == objects_.end()) ? nullptr : it->second;
> +}
> +
> +/**
> + * \brief Return the MediaEntity with name \a name.
> + * \param name The entity name.
> + * \return The entity with \a name.
> + * \return nullptr if no entity with \a name is found.
> + */
> +MediaEntity *MediaDevice::getEntityByName(const std::string &name)
> +{
> + for (MediaEntity *e : entities_)
> + if (e->name() == name)
> + return e;
> +
> + return nullptr;
> +}
> +
> +int MediaDevice::populateLinks(const struct media_v2_topology &topology)
> +{
> + media_v2_link *mediaLinks = reinterpret_cast<media_v2_link *>
> + (topology.ptr_links);
> +
> + for (unsigned int i = 0; i < topology.num_links; ++i) {
> + /*
> + * Skip links between entities and interfaces: we only care
> + * about pad-2-pad links here.
> + */
> + if ((mediaLinks[i].flags & MEDIA_LNK_FL_LINK_TYPE) ==
> + MEDIA_LNK_FL_INTERFACE_LINK)
> + continue;
> +
> + /* Store references to source and sink pads in the link. */
> + unsigned int source_id = mediaLinks[i].source_id;
> + MediaPad *source = dynamic_cast<MediaPad *>
> + (getObject(source_id));
> + if (!source) {
> + LOG(Error) << "Failed to find pad with id: "
> + << source_id;
> + return -ENODEV;
> + }
> +
> + unsigned int sink_id = mediaLinks[i].sink_id;
> + MediaPad *sink = dynamic_cast<MediaPad *>
> + (getObject(sink_id));
> + if (!sink) {
> + LOG(Error) << "Failed to find pad with id: "
> + << sink_id;
> + return -ENODEV;
> + }
> +
> + MediaLink *link = new MediaLink(&mediaLinks[i], source, sink);
> + source->addLink(link);
> + sink->addLink(link);
> +
> + addObject(link);
I would put addObject() before foo->addLink();
> + }
> +
> + return 0;
> +}
> +
> +int MediaDevice::populatePads(const struct media_v2_topology &topology)
> +{
> + media_v2_pad *mediaPads = reinterpret_cast<media_v2_pad *>
> + (topology.ptr_pads);
> +
> + for (unsigned int i = 0; i < topology.num_pads; ++i) {
> + unsigned int entity_id = mediaPads[i].entity_id;
> +
> + /* Store a reference to this MediaPad in entity. */
> + MediaEntity *mediaEntity = dynamic_cast<MediaEntity *>
> + (getObject(entity_id));
> + if (!mediaEntity) {
> + LOG(Error) << "Failed to find entity with id: "
> + << entity_id;
> + return -ENODEV;
> + }
> +
> + MediaPad *pad = new MediaPad(&mediaPads[i], mediaEntity);
> + mediaEntity->addPad(pad);
> +
> + addObject(pad);
I would put addObject() before foo->addPad();
> + }
> +
> + return 0;
> +}
> +
> +/*
> + * For each entity in the media graph create a MediaEntity and store a
> + * reference in the MediaObject global pool and in the global vector of
> + * entities.
> + */
> +int MediaDevice::populateEntities(const struct media_v2_topology &topology)
This function can't fail maybe make it void?
> +{
> + media_v2_entity *mediaEntities = reinterpret_cast<media_v2_entity *>
> + (topology.ptr_entities);
> +
> + for (unsigned int i = 0; i < topology.num_entities; ++i) {
> + MediaEntity *entity = new MediaEntity(&mediaEntities[i]);
> +
> + addObject(entity);
> + entities_.push_back(entity);
> + }
> +
> + return 0;
> +}
> +
> +/**
> + * \brief Populate the media graph with media objects.
> + *
> + * This function enumerates all media objects in the media device graph and
> + * creates their MediaObject representations. All entities, pads and links are
> + * stored as MediaEntity, MediaPad and MediaLink respectively, with cross-
> + * references between objects. Interfaces are not processed.
> + *
> + * MediaEntities are stored in a global list in the MediaDevice itself to ease
> + * lookup, while MediaPads are accessible from the MediaEntity they belong
> + * to only and MediaLinks from the MediaPad they connect.
> + *
> + * \return Return 0 on success or a negative error code on error.
s/Return//
> + */
> +int MediaDevice::populate()
> +{
> + struct media_v2_topology topology;
> + struct media_v2_entity *ents;
> + struct media_v2_link *links;
> + struct media_v2_pad *pads;
> + int ret;
> +
> + /*
> + * Keep calling G_TOPOLOGY until the version number stays stable.
> + */
> + while (true) {
> + topology = {};
> +
> + ret = ioctl(fd_, MEDIA_IOC_G_TOPOLOGY, &topology);
> + if (ret < 0) {
> + ret = -errno;
> + LOG(Error) << "Failed to enumerate topology: "
> + << strerror(-ret);
> + return ret;
> + }
> +
> + __u64 version = topology.topology_version;
> +
> + ents = new media_v2_entity[topology.num_entities];
> + links = new media_v2_link[topology.num_links];
> + pads = new media_v2_pad[topology.num_pads];
> +
> + topology.ptr_entities = reinterpret_cast<__u64>(ents);
> + topology.ptr_links = reinterpret_cast<__u64>(links);
> + topology.ptr_pads = reinterpret_cast<__u64>(pads);
> +
> + ret = ioctl(fd_, MEDIA_IOC_G_TOPOLOGY, &topology);
> + if (ret < 0) {
> + ret = -errno;
> + LOG(Error) << "Failed to enumerate topology: "
> + << strerror(-ret);
> + goto error_free_mem;
> + }
> +
> + if (version == topology.topology_version)
> + break;
> +
> + delete[] links;
> + delete[] ents;
> + delete[] pads;
> + }
> +
> + /* Populate entities, pads and links. */
> + ret = populateEntities(topology);
> + if (ret)
> + goto error_free_mem;
populateEntities() can't fail.
> +
> + ret = populatePads(topology);
> + if (ret)
> + goto error_free_objs;
> +
> + ret = populateLinks(topology);
> + if (ret)
> + goto error_free_objs;
> +
Code bellow here can be reduced.
> + delete[] links;
> + delete[] ents;
> + delete[] pads;
> +
> + return 0;
> +
> +error_free_objs:
> + deleteObjects();
> +
> +error_free_mem:
> + delete[] links;
> + delete[] ents;
> + delete[] pads;
> +
> + return ret;
As there is no harm in calling deleteObjects() even if no object have
been added. How about using only one error label:
error:
if (ret)
deleteObjects();
delete[] links;
delete[] ents;
delete[] pads;
return ret;
> +}
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index 01d321c..39a0464 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -2,10 +2,12 @@ libcamera_sources = files([
> 'log.cpp',
> 'main.cpp',
> 'media_object.cpp',
> + 'media_device.cpp',
> ])
>
> libcamera_headers = files([
> 'include/log.h',
> + 'include/media_device.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