[libcamera-devel] [PATCH RESEND v2 2/4] libcamera: Add MediaDevice class
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Dec 28 14:27:21 CET 2018
Hi Jacopo,
Thank you for the patch.
On Friday, 28 December 2018 09:57:41 EET Jacopo Mondi wrote:
> The MediaDevice object implements handling and configuration of the media
> graph associated with a V4L2 media device.
Let's not talk about V4L2 here, it's just MC, not V4L2.
> The class allows enumeration of all pads, links and entities registered in
> the media graph, and provides methods to setup and reset media links.
>
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
> src/libcamera/include/media_device.h | 71 ++++
> src/libcamera/media_device.cpp | 596 +++++++++++++++++++++++++++
> src/libcamera/meson.build | 1 +
> 3 files changed, 668 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..642eea9
> --- /dev/null
> +++ b/src/libcamera/include/media_device.h
> @@ -0,0 +1,71 @@
> +/* 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 <string>
> +#include <sstream>
sort
> +#include <vector>
> +
> +#include <linux/media.h>
> +
> +#include "log.h"
Is this needed ?
> +#include "media_object.h"
> +
> +namespace libcamera {
> +
> +class MediaDevice
> +{
> +public:
> + MediaDevice() : fd_(-1) { };
> + ~MediaDevice();
> +
> + const std::string name() const { return name_; }
> + const std::string path() const { return path_; }
> +
> + int open(const std::string &path);
> + int close();
> + int enumerate(std::map<std::string, std::string> &entitiesMap);
> + void dumpGraph(std::ostream &os);
> +
> + int resetLinks();
> + int link(const std::string &source, unsigned int sourceIdx,
> + const std::string &sink, unsigned int sinkIdx,
> + unsigned int flags);
> +
> +private:
> + /** The media device file descriptor */
Wrong file ?
> + int fd_;
> + /** The media device name as returned by MEDIA_IOC_DEVICE_INFO */
Please take my comments on 1/4 into account regarding wording of documentation
here.
This field stores the driver name, I would thus name it driver_ (or possibly
driverName_).
> + std::string name_;
> + /** The media device path */
> + std::string path_;
> +
> + std::map<unsigned int, MediaObject *> mediaObjects_;
s/mediaObjects_/objects_/ ?
> + MediaObject *getObject(unsigned int id);
> + void addObject(MediaObject *obj);
> + void deleteObjects();
> +
> + std::vector<MediaEntity *> entities_;
> + MediaEntity *getEntityByName(const std::string &name);
> +
> + int enumerateEntities(std::map<std::string, std::string> &entitiesMap,
> + struct media_v2_topology &topology);
> + int enumeratePads(struct media_v2_topology &topology);
> + int enumerateLinks(struct media_v2_topology &topology);
> +
> + int setupLink(const MediaPad *source, const MediaPad *sink,
> + MediaLink *link, unsigned int flags);
> +
> + void dumpLocal(MediaEntity *e, MediaPad *p, std::ostream &os);
> + void dumpRemote(MediaLink *l, std::ostream &os);
> + void dumpLink(MediaLink *l, std::ostream &os);
> +};
> +
> +} /* 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..b98d62f
> --- /dev/null
> +++ b/src/libcamera/media_device.cpp
> @@ -0,0 +1,596 @@
> +/* 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 handles the media graph associated with a V4L2 media device.
> + */
> +
> +/**
> + * \fn MediaDevice::~MediaDevice()
> + * \brief Close the media device file descriptor and release entities
> + */
> +MediaDevice::~MediaDevice()
> +{
> + if (fd_ > -1)
I would have written != -1.
> + ::close(fd_);
> + deleteObjects();
> +}
> +
> +/**
> + * \fn MediaDevice::name()
> + * \brief Return the media device name
> + */
> +
> +/**
> + * \fn MediaDevice::path()
> + * \brief Return the media device path node associated with this
> MediaDevice
I'd name this devnode() instead of path().
> + */
> +
> +/**
> + * \fn MediaDevice::deleteObjects()
> + * \brief Delete all registered entities in the MediaDevice object
> + */
> +void MediaDevice::deleteObjects()
> +{
> + for (auto const &e : mediaObjects_)
> + delete e.second;
> +
> + mediaObjects_.clear();
> + entities_.clear();
> +}
> +
> +/**
> + * \fn int MediaDevice::open(std::string)
Do you need to specify the arguments when there's a single function by that
name ? If so, shouldn't it be (const std::string &) ?
Same comment for other functions in this patch series.
> + * \brief Open a media device and initialize its components.
> + * \param path The media device path
> + */
> +int MediaDevice::open(const std::string &path)
s/path/devnode/ ?
> +{
What if the device is already open ? It would be useful to add some
documentation to explain the "life cycle" of the object, as in how it is
supposed to be used (and what happens when it's incorrectly used). This can be
done in the open() documentation (and referenced from close() and possibly
other functions), or in the \class documentation.
> + fd_ = ::open(path.c_str(), O_RDWR);
> + if (fd_ < 0) {
> + LOG(Error) << "Failed to open media device at " << path
> + << ": " << strerror(errno);
> + return -errno;
The LOG() call may overwrite errno. You will need a local variable. How about
int ret;
ret = ::open(path.c_str(), O_RDWR);
if (ret < 0) {
ret = -errno;
LOG(Error) << "Failed to open media device at " << path
<< ": " << strerror(-ret);
return ret;
}
fd_ = ret;
path_ = path;
> + }
> + path_ = path;
> +
> + struct media_device_info info = { };
> + int ret = ioctl(fd_, MEDIA_IOC_DEVICE_INFO, &info);
> + if (ret) {
> + LOG(Error) << "Failed to get media device info "
> + << ": " << strerror(errno);
> + return -errno;
> + }
> +
> + name_ = info.driver;
> +
> + return 0;
> +}
> +
> +/**
> + * \fn MediaDevice::close()
> + * \brief Close the file descriptor associated with the media device
> + */
> +int MediaDevice::close()
> +{
> + if (fd_ > -1)
I'd write this != -1 too.
> + return ::close(fd_);
> +
> + return 0;
Do you use the return value ?
> +}
> +
> +void MediaDevice::addObject(MediaObject *obj)
> +{
> +
> + if (mediaObjects_.find(obj->id()) != mediaObjects_.end()) {
> + LOG(Error) << "Element with id " << obj->id()
> + << " already enumerated.";
> + return;
Shouldn't you propagate the error to the caller ? This seems pretty fatal to
me.
> + }
> +
> + mediaObjects_[obj->id()] = obj;
> +}
> +
> +MediaObject *MediaDevice::getObject(unsigned int id)
> +{
> + auto it = mediaObjects_.find(id);
> + return (it == mediaObjects_.end()) ?
> + nullptr : it->second;
> +}
> +
> +/**
> + * \fn MediaDevice::getEntityByName(std::string)
> + * \brief Return entity with name \a name
> + * \param name The entity name
Please expand documentation a little bit. For instance you should explain that
this function returns nullptr if the entity can't be found. In general a one-
line documentation that just duplicates the function name isn't very useful,
the real value comes from what isn't expressed in the name.
Documentation is hard :-)
> + */
> +MediaEntity *MediaDevice::getEntityByName(const std::string &name)
> +{
> + auto it = entities_.begin();
> +
> + while (it != entities_.end()) {
> + MediaEntity *e = *it;
> + if (!(e->name().compare(name)))
> + return e;
> + it++;
> + }
> +
> + return nullptr;
> +}
> +
> +/**
> + * \fn MediaDevice::enumerateLinks(struct media_v2_topology &topology)
> + * \brief Enumerate all links in the system and associate them with their
> + * source and sink pads
> + * \param topology The media topology as returned by MEDIA_IOC_G_TOPOLOGY
> + */
> +int MediaDevice::enumerateLinks(struct media_v2_topology &topology)
Shouldn't the argument be const ? I would have used a pointer instead of a
reference, but I'm not sure why.
> +{
> + struct media_v2_link *link = reinterpret_cast<struct media_v2_link *>
> + (topology.ptr_links);
> +
> + for (unsigned int i = 0; i < topology.num_links; i++, link++) {
> + /*
> + * Skip links between entities and interfaces: we only care
> + * about pad-2-pad links here.
> + */
> + if ((link->flags & MEDIA_LNK_FL_LINK_TYPE) ==
> + MEDIA_LNK_FL_INTERFACE_LINK)
> + continue;
> +
> + MediaLink *mediaLink = new MediaLink(link);
s/mediaLink/link/ ?
You would need to rename the link variable above though, I'm not sure which
one is best.
> + addObject(mediaLink);
> +
> + /* Store reference to this mediaLink in the link's source pad. */
> + MediaPad *mediaPad = dynamic_cast<MediaPad *>
> + (getObject(mediaLink->source()));
s/mediaPad/pad/ ?
> + if (!mediaPad) {
> + LOG(Error) << "Failed to find pad with id: "
Nitpicking, s/://.
> + << mediaLink->source();
> + return -ENODEV;
> + }
> + mediaPad->addLink(mediaLink);
> +
> + /* Store reference to this mediaLink in the link's sink pad. */
> + mediaPad = dynamic_cast<MediaPad *>(getObject(mediaLink->sink()));
> + if (!mediaPad) {
> + LOG(Error) << "Failed to find pad with id: "
> + << mediaLink->sink();
> + return -ENODEV;
> + }
> + mediaPad->addLink(mediaLink);
> + }
> +
> + return 0;
> +}
> +
> +/**
> + * \fn MediaDevice::enumeratePads(struct media_v2_topology &topology)
> + * \brief Enumerate all pads in the system and associate them with the
> + * entity they belong to
> + * \param topology The media topology as returned by MEDIA_IOC_G_TOPOLOGY
> + */
> +int MediaDevice::enumeratePads(struct media_v2_topology &topology)
> +{
> + struct media_v2_pad *pad = reinterpret_cast<struct media_v2_pad *>
> + (topology.ptr_pads);
> +
> + for (unsigned int i = 0; i < topology.num_pads; i++, pad++) {
> + MediaPad *mediaPad = new MediaPad(pad);
> + addObject(mediaPad);
> +
> + /* Store a reference to this MediaPad in pad's entity. */
> + MediaEntity *mediaEntity = dynamic_cast<MediaEntity *>
> + (getObject(mediaPad->entity()));
s/mediaPad/pad/ and s/mediaEntity/entity/ ?
> + if (!mediaEntity) {
> + LOG(Error) << "Failed to find entity with id: "
> + << mediaPad->entity();
> + return -ENODEV;
> + }
> +
> + mediaEntity->addPad(mediaPad);
> + }
> +
> + return 0;
> +}
> +
> +/**
> + * \fn MediaDevice::enumerateEntities(std::map<std::string, std::string> &,
> + * struct media_v2_topology &topology)
> + * \brief Enumerate and initialize entities in the media graph
> + * \param entitiesMap Map entities names to their video (sub)device node
> + * \param topology The media topology as returned by MEDIA_IOC_G_TOPOLOGY
> + *
> + * Associate the video (sub)device path to the entity name as returned by
As stated before, I'd talk about device nodes, as this isn't V4L2-specific.
> + * MEDIA_IOC_G_TOPOLOGY
> + */
> +int MediaDevice::enumerateEntities(std::map<std::string, std::string>
> &entitiesMap,
> + struct media_v2_topology &topology)
> +{
> + struct media_v2_entity *entities = reinterpret_cast<struct media_v2_entity
> *>
> + (topology.ptr_entities);
> +
> + for (unsigned int i = 0; i < topology.num_entities; ++i) {
> + auto it = entitiesMap.find(entities[i].name);
> + if (it == entitiesMap.end()) {
> + LOG(Error) << "Entity " << entities[i].name
> + << " not found in media entities map";
> + return -ENOENT;
> + }
> +
> + MediaEntity *entity = new MediaEntity(&entities[i]);
> + if (entity->setDevice(it->second)) {
> + delete entity;
> + goto delete_entities;
You could call deleteObjects() and return here directly.
> + }
> +
> + addObject(entity);
> + entities_.push_back(entity);
> + }
> +
> + return 0;
> +
> +delete_entities:
> + deleteObjects();
> +
> + return -errno;
-errno isn't correct. You should assign the return value of setDevice() to a
ret variable and return ret.
> +}
> +
> +/**
> + * \fn MediaDevice::enumerate(std::map<std::string, std::string>)
> + * \brief Enumerate the media graph topology
> + * \param entitiesMap Map entities names to their video (sub)device node
> + * FIXME: this is statically provided by the caller at the moment.
> + *
> + * This functions enumerates all media objects, registered in the media
> graph,
s/functions/function/
s/,//g
> + * through the MEDIA_IOC_G_TOPOLOGY ioctl. For each returned entity,
> + * it creates and store its representation for later reuse.
Let's expand this a little.
"This function enumerates all media graph objects in the media device and
populates the internal list of objects. All entities, pads and links are
stored as MediaEntity, MediaPad and MediaLink respectively, with cross-
references between objects. Media interfaces are not processed.
For entities paired with a device node, the path to the device node is stored
in the MediaEntity object, based on the associations set in the \a entitiesMap
parameter.
\return Return 0 on success or a negative error code on error."
> + */
> +int MediaDevice::enumerate(std::map<std::string, std::string> &entitiesMap)
> +{
> + struct media_v2_topology topology = { };
> + unsigned int num_interfaces;
> + unsigned int num_links;
> + unsigned int num_pads;
> + unsigned int num_ent;
> +
> + do {
> + num_ent = topology.num_entities;
> + num_pads = topology.num_pads;
> + num_links = topology.num_links;
> + num_interfaces = topology.num_interfaces;
> +
> + /* Call G_TOPOLOGY the first time here to enumerate .*/
> + if (ioctl(fd_, MEDIA_IOC_G_TOPOLOGY, &topology)) {
> + LOG(Error) << "Failed to enumerate media topology on"
> + << path_ << ": " << strerror(errno);
> + return -errno;
> + }
> +
> + /*
> + * Repeat the call until we don't get a 'stable' number
> + * of media objects.
> + */
> + } while (num_ent != topology.num_entities ||
> + num_pads != topology.num_pads ||
> + num_links != topology.num_links ||
> + num_interfaces != topology.num_interfaces);
That's not very useful, as the need to ensure that the topology doesn't change
stems from the fact we call MEDIA_IOC_G_TOPOLOGY (at least) twice, once to
retrieve the number of entities, and a second time to retrieve the graph
itself. The graph could be updated in the kernel after this loop and before
the next MEDIA_IOC_G_TOPOLOGY.
I proposed a way to call MEDIA_IOC_G_TOPOLOGY from a single location in a
previous e-mail. We can fix this on top of this series, or:
struct media_v2_topology topology = { };
struct media_v2_entity *entities;
struct media_v2_pad *pads;
struct media_v2_link *links;
struct media_v2_interface *interfaces;
unsigned int num_entities;
unsigned int num_pads;
unsigned int num_links;
unsigned int num_interfaces;
int ret;
while (1) {
num_entitites = topology.num_entities;
num_pads = topology.num_pads;
num_links = topology.num_links;
num_interfaces = topology.num_interfaces;
pads = new media_v2_pad[topology.num_pads];
entities = new media_v2_entity[topology.num_entities];
topology.ptr_entities = reinterpret_cast<__u64>(entities);
topology.ptr_pads = reinterpret_cast<__u64>(pads);
links = new media_v2_link[topology.num_links];
topology.ptr_links = reinterpret_cast<__u64>(links);
links = new media_v2_interface[topology.num_interfaces];
topology.ptr_interfaces = reinterpret_cast<__u64>(interfaces);
ret = ioctl(fd_, MEDIA_IOC_G_TOPOLOGY, &topology);
if (ret < 0) {
ret = -errno;
LOG(Error) << "Failed to enumerate media topology on"
<< path_ << ": " << strerror(-ret);
goto done;
}
/*
* If all objects have been enumerated, stop now. If the number
* of objects has increased (after the first call or due to a
* topology change, retry the enumeration.
*/
if (num_entities >= topology.num_entities &&
num_pads >= topology.num_pads &&
num_links >= topology.num_links &&
num_interfaces >= topology.num_interfaces)
break;
}
ret = enumerateEntities(entitiesMap, topology);
if (ret)
goto done;
ret = enumeratePads(topology);
if (ret)
goto done;
ret = enumerateLinks(topology);
if (ret)
goto done;
done:
if (ret < 0)
deleteObjects();
delete[] entities;
delete[] links;
delete[] pads;
delete[] interfaces;
return ret;
(untested)
I wonder if enumerateEntities, enumeratePads and enumerateLinks should be
renamed as s/enumerate/populate/.
> + auto *_ptr_e = new media_v2_entity[topology.num_entities];
> + topology.ptr_entities = reinterpret_cast<__u64>(_ptr_e);
> +
> + auto *_ptr_p = new media_v2_pad[topology.num_pads];
> + topology.ptr_pads = reinterpret_cast<__u64>(_ptr_p);
> +
> + auto *_ptr_l = new media_v2_link[topology.num_links];
> + topology.ptr_links = reinterpret_cast<__u64>(_ptr_l);
> +
> + /* Call G_TOPOLOGY again, this time with memory reserved. */
> + int ret = ioctl(fd_, MEDIA_IOC_G_TOPOLOGY, &topology);
> + if (ret < 0) {
> + LOG(Error) << "Failed to enumerate media topology on " << path_
> + << ": " << strerror(errno);
> + ret = -errno;
> + goto error_free_mem;
> + }
> +
> + ret = enumerateEntities(entitiesMap, topology);
> + if (ret)
> + goto error_free_mem;
> +
> + ret = enumeratePads(topology);
> + if (ret)
> + goto error_free_objs;
> +
> + ret = enumerateLinks(topology);
> + if (ret)
> + goto error_free_objs;
> +
> + delete[] _ptr_e;
> + delete[] _ptr_p;
> + delete[] _ptr_l;
> +
> + return 0;
> +
> +error_free_objs:
> + deleteObjects();
> +
> +error_free_mem:
> + delete[] _ptr_e;
> + delete[] _ptr_p;
> + delete[] _ptr_l;
> +
> + return ret;
> +}
> +
> +void MediaDevice::dumpLocal(MediaEntity *e, MediaPad *p, std::ostream &os)
s/p/pad/
Unless names get really long, let's try to spell them out, as it increases
readability.
> +{
> + os << "\t \"" << e->name() << "\"["
> + << p->index() << "]";
> +}
> +
> +void MediaDevice::dumpRemote(MediaLink *l, std::ostream &os)
s/l/link/
> +{
> +
Stray blank line.
> + MediaPad *remotePad = dynamic_cast<MediaPad *>
> + (getObject(l->sink()));
Would it make sense for the sink() and source() methods to return pointers to
MediaPad instead of an id ? I would in that case store the pointers internally
in MediaLink, to avoid a call to getObject() every time.
> + if (!remotePad)
> + return;
This check wouldn't be needed then.
> +
> + MediaEntity *remoteEntity = dynamic_cast<MediaEntity *>
> + (getObject(remotePad->entity()));
> + if (!remoteEntity)
> + return;
Same here, returning a MediaEntity pointer, and removing the check.
> + os << "\"" << remoteEntity->name() << "\"["
> + << remotePad->index() << "]";
> +}
> +
> +void MediaDevice::dumpLink(MediaLink *l, std::ostream &os)
> +{
> + unsigned int flags = l->flags();
> +
> + os << " [";
> + if (flags) {
> + os << (flags & MEDIA_LNK_FL_ENABLED ? "ENABLED," : "")
> + << (flags & MEDIA_LNK_FL_IMMUTABLE ? "IMMUTABLE" : "");
> + }
> + os << "]\n";
> +}
> +
> +/**
> + * \fn MediaDevice::dumpGraph(std::ostream)
> + * \brief Dump the media device topology in textual form to an output
> stream
> + * \param os The output stream where to append the printed topology to
s/append/write/ ?
> + */
> +void MediaDevice::dumpGraph(std::ostream &os)
> +{
> + os << "\n" << name_ << " - " << path_ << "\n\n";
> +
> + for (auto const &e : entities_) {
s/auto const &e/const struct MediaEntity */
Let's restrict the use of auto to cases where it would be impractical to write
the type out explicitly, as auto reduces readability and removes compile-time
checks.
> + os << "\"" << e->name() << "\"\n";
> +
> + for (auto const &p : e->sinks()) {
> + os << " [" << p->index() << "]" << ": Sink\n";
> + for (auto const &l : p->links()) {
> + dumpLocal(e, p, os);
> + os << " <- ";
> + dumpRemote(l, os);
> + dumpLink(l, os);
> + }
> + os << "\n";
> + }
> +
> + for (auto const &p : e->sources()) {
> + os << " [" << p->index() << "]" << ": Source\n";
> + for (auto const &l : p->links()) {
> + dumpLocal(e, p, os);
> + os << " -> ";
> + dumpRemote(l, os);
> + dumpLink(l, os);
> + }
> + os << "\n";
> + }
> + }
> +}
I wonder whether dumpGraph() belongs here, or whether it should be moved to a
test. MediaDevice isn't exposed through the public API, how do you foresee
dumpGraph being used ? If it can never be called from the public API there's
no point in having it in the library :-)
> +/**
> + * \fn MediaDevice::setupLink(MediaPad *source, MediaPad *sink)
you're missing const (assuming we need to list the parameters explicitly here,
see the comment above).
> + * \brief Apply \a flags to the link between \a source and \a sink pads
> + * \param source The source MediaPad
> + * \param sink The sink MediaPad
> + * \param link The MediaLink to operate on
Why do you need to specify both source and sink, and the link ? The link
should identify the source and sink already. If you don't expect the caller to
have the link pointer you should look it up internally, otherwise you
shouldn't pass the source and sink. And if you expect the caller to have the
link pointer, maybe you could make this function a member of the MediaLink
class ?
> + * \param flags Flags to be applied to the link (MEDIA_LNK_FL_*)
> + */
> +int MediaDevice::setupLink(const MediaPad *source, const MediaPad *sink,
> + MediaLink *link, unsigned int flags)
> +{
> + struct media_link_desc linkDesc = { };
> +
> + linkDesc.source.entity = source->entity();
> + linkDesc.source.index = source->index();
> + linkDesc.source.flags = MEDIA_PAD_FL_SOURCE;
> +
> + linkDesc.sink.entity = sink->entity();
> + linkDesc.sink.index = sink->index();
> + linkDesc.sink.flags = MEDIA_PAD_FL_SINK;
> +
> + linkDesc.flags = flags;
> +
> + if (ioctl(fd_, MEDIA_IOC_SETUP_LINK, &linkDesc)) {
> + LOG(Error) << "Failed to setup link: "
> + << strerror(errno);
> + return -errno;
> + }
> +
> + link->setFlags(0);
> +
> + return 0;
> +}
> +
> +/**
> + * \fn MediaDevice::resetLinks()
> + * \brief Reset all links on the media graph
Reset to what ? :-)
> + *
> + * Walk all registered entities, and disable all links from their
> + * source pads to other pads.
Wouldn't it be more efficient to walk links instead of entities and pads ?
Could you expand the documentation to explain the use cases for this function
? Please also document the return value (this holds true for all other
functions returning a value in the whole series).
> + */
> +int MediaDevice::resetLinks()
> +{
> + for (MediaEntity *e : entities_) {
> + for (MediaPad *sourcePad : e->sources()) {
> + for (MediaLink *l : sourcePad->links()) {
> + /*
> + * Do not reset links that are not enabled
> + * or immutable.
> + */
> + if (l->flags() & MEDIA_LNK_FL_IMMUTABLE)
> + continue;
> +
> + if (!(l->flags() & MEDIA_LNK_FL_ENABLED))
> + continue;
> +
> + /* Get the remote sink pad. */
> + MediaPad *sinkPad = dynamic_cast<MediaPad *>
> + (getObject(l->sink()));
> + if (!sinkPad)
> + return -ENOENT;
> +
> + /* Also get entity to make sure IDs are ok. */
> + MediaEntity *sinkEntity =
> + dynamic_cast<MediaEntity *>
> + (getObject(sinkPad->entity()));
> + if (!sinkEntity)
> + return -ENOENT;
> +
> + int ret = setupLink(sourcePad, sinkPad, l, 0);
> + if (ret) {
> + LOG(Error) << "Link reset failed: "
> + << e->name() << "["
> + << sourcePad->index()
> + << "] -> "
> + << sinkEntity->name() << "["
> + << sinkPad->index() << "]";
> + return ret;
> + }
> +
> + LOG(Info) << "Link reset: "
> + << e->name() << "["
> + << sourcePad->index()
> + << "] -> "
> + << sinkEntity->name() << "["
> + << sinkPad->index() << "]";
> + }
> + }
> + }
> +
> + return 0;
> +}
> +
> +/**
> + * \fn MediaDevice::link(std::string, unsigned int, std::string, unsigned
> int)
> + * \brief Setup a link identified by the entities name and their source and
> + * sink pad indexes
> + * \param source The source entity name
> + * \param sourceIdx The source pad index
> + * \param sink The sink entity name
> + * \param sinkIdx The sink pad index
> + * \param flags The link setup flag (see MEDIA_LNK_FL_*)
If I understand the purpose of this function properly (and the fact that I'm
not sure I do means the documentation should be improved :-)), it serves the
same purpose as MediaDevice::setupLink(), but with different arguments. I
would thus name the two functions setupLink(), as C++ allows overloading
functions.
I would go one step further though, at least if you agree with my proposition
to move setupLink to MediaLink, and turn this function into a link lookup. The
callers would then do something similar to
MediaDevice *dev = ...;
MediaLink *link = dev->link("source", 0, "sink", 3);
if (!link)
return -ENODEV;
link->setup(flags);
and possibly cache the link pointer internally to avoid looking up links all
the time.
> + */
> +int MediaDevice::link(const std::string &source, unsigned int sourceIdx,
> + const std::string &sink, unsigned int sinkIdx,
At the cost of longer names, sourcePadIndex and sinkPadIndex ?
> + unsigned int flags)
> +{
> +
Stray blank line.
> + /* Make sure the supplied link is well formed. */
> + MediaEntity *sourceEntity = getEntityByName(source);
> + if (!sourceEntity) {
> + LOG(Error) << "Entity name: " << source << "not found";
> + return -ENOENT;
> + }
> +
> + MediaEntity *sinkEntity = getEntityByName(sink);
> + if (!sinkEntity) {
> + LOG(Error) << "Entity name: " << source << "not found";
s/source/sink/
s/Entity name:/Entity/
> + return -ENOENT;
> + }
> +
> + const MediaPad *sourcePad = sourceEntity->getPadByIndex(sourceIdx);
> + if (!sourcePad) {
> + LOG(Error) << "Pad " << sourceIdx << "not found in entity "
> + << sourceEntity->name();
> + return -ENOENT;
> + }
> +
> + const MediaPad *sinkPad = sinkEntity->getPadByIndex(sinkIdx);
> + if (!sinkPad) {
> + LOG(Error) << "Pad " << sinkIdx << "not found in entity "
> + << sinkEntity->name();
> + return -ENOENT;
> + }
> +
> + /*
> + * Walk all links in the source and search for an entry matching the
> + * pad ids. If none, the requested link does not exists.
> + */
> + MediaLink *validLink = nullptr;
> + for (MediaLink *link : sourcePad->links()) {
> + if (link->source() != sourcePad->id())
> + continue;
> +
> + if (link->sink() != sinkPad->id())
> + continue;
> +
> + validLink = link;
> + break;
> + }
> +
> + if (!validLink) {
> + LOG(Error) << "Link not found"
> + << "\"" << sourceEntity->name() << "\"["
> + << sourcePad->index() << "] -> "
> + << "\"" << sinkEntity->name() << "\"["
> + << sinkPad->index() << "]";
> + return -EINVAL;
> + }
> +
> + int ret = setupLink(sourcePad, sinkPad, validLink, flags);
> + if (ret)
> + return ret;
> +
> + LOG(Info) << "Setup link: "
LOG(Debug) at most.
> + << "\"" << sourceEntity->name() << "\"["
> + << sourcePad->index() << "] -> "
> + << "\"" << sinkEntity->name() << "\"["
> + << sinkPad->index() << "]"
> + << " [" << flags << "]";
> +
> + return 0;
> +}
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index da06eba..4cac687 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -2,6 +2,7 @@ libcamera_sources = files([
> 'log.cpp',
> 'main.cpp',
> 'media_object.cpp',
> + 'media_device.cpp',
sort
> ])
>
> libcamera_headers = files([
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list