[libcamera-devel] [PATCH v3 2/3] libcamera: Add MediaDevice class

[Jacopo Mondi]

Hi Niklas,

On Sun, Dec 30, 2018 at 11:39:48PM +0100, Niklas Söderlund wrote:
> 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.
>

I think it is a valid use case. In example the enumerator might
open/populate the media device and then close it (wihtout deleting its
media objects) and open it again once a match is requested or once the
media device is given to the pipeline handler that operates on it.

> > +
> > +	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?
>

I meant that a public method to access entities is available.

> > + */
> > +
> > +/**
> > + * \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?
>

deleteObjects() is private anyhow, so that's just an internal
convenience function, which I can remove if it bothers you.

> > + */
> > +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.
>

For now (aka we don't merge with the enumerator) I would keep this
open/close interfce. If we find out this is not required we can merge
this in a single init() that does open+enumeration.

> > +
> > +/**
> > + * \fn MediaDevice::entities()
> > + * \brief Return the list of MediaEntity references.
> > + */
>
> Needed?
>

Yes, there is not way to access entities otherwise, and if I want to
do any testing I had to do so.

Me and Laurent briefly discussed this, I don't like to much giving all
entities away and I would have liked a parametrized interface to access
entities (byName, byId etc). But he convinced me there are a lot of use
cases where the list of entities is needed (the point that convinced
me the most is that a pipeline handler might not know the name of the
image sensor installed, and to find it out, it has to access all
entities).

> > +
> > +/*
> > + * 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();
>

As that's just a matter of tastes, I'll keep it as it is.

> > +	}
> > +
> > +	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;

This might simplify the code, thanks.

Thanks
   j

>
> > +}
> > +
> > +} /* 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
-------------- 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/20181231/a6903947/attachment.sig>