[{"id":95,"web_url":"https://patchwork.libcamera.org/comment/95/","msgid":"<1597883.HRDAXlUJTF@avalon>","date":"2018-12-28T13:27:21","subject":"Re: [libcamera-devel] [PATCH RESEND v2 2/4] libcamera: Add\n\tMediaDevice class","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the patch.\n\nOn Friday, 28 December 2018 09:57:41 EET Jacopo Mondi wrote:\n> The MediaDevice object implements handling and configuration of the media\n> graph associated with a V4L2 media device.\n\nLet's not talk about V4L2 here, it's just MC, not V4L2.\n\n> The class allows enumeration of all pads, links and entities registered in\n> the media graph, and provides methods to setup and reset media links.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> src/libcamera/include/media_device.h | 71 ++++\n> src/libcamera/media_device.cpp | 596 +++++++++++++++++++++++++++\n> src/libcamera/meson.build | 1 +\n> 3 files changed, 668 insertions(+)\n> create mode 100644 src/libcamera/include/media_device.h\n> create mode 100644 src/libcamera/media_device.cpp\n> \n> diff --git a/src/libcamera/include/media_device.h\n> b/src/libcamera/include/media_device.h new file mode 100644\n> index 0000000..642eea9\n> --- /dev/null\n> +++ b/src/libcamera/include/media_device.h\n> @@ -0,0 +1,71 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2018, Google Inc.\n> + *\n> + * media_device.h - Media device handler\n> + */\n> +#ifndef __LIBCAMERA_MEDIA_DEVICE_H__\n> +#define __LIBCAMERA_MEDIA_DEVICE_H__\n> +\n> +#include \n> +#include \n> +#include \n\nsort\n\n> +#include \n> +\n> +#include \n> +\n> +#include \"log.h\"\n\nIs this needed ?\n\n> +#include \"media_object.h\"\n> +\n> +namespace libcamera {\n> +\n> +class MediaDevice\n> +{\n> +public:\n> +\tMediaDevice() : fd_(-1) { };\n> +\t~MediaDevice();\n> +\n> +\tconst std::string name() const { return name_; }\n> +\tconst std::string path() const { return path_; }\n> +\n> +\tint open(const std::string &path);\n> +\tint close();\n> +\tint enumerate(std::map &entitiesMap);\n> +\tvoid dumpGraph(std::ostream &os);\n> +\n> +\tint resetLinks();\n> +\tint link(const std::string &source, unsigned int sourceIdx,\n> +\t\t const std::string &sink, unsigned int sinkIdx,\n> +\t\t unsigned int flags);\n> +\n> +private:\n> +\t/** The media device file descriptor */\n\nWrong file ?\n\n> +\tint fd_;\n> +\t/** The media device name as returned by MEDIA_IOC_DEVICE_INFO */\n\nPlease take my comments on 1/4 into account regarding wording of documentation \nhere.\n\nThis field stores the driver name, I would thus name it driver_ (or possibly \ndriverName_).\n\n> +\tstd::string name_;\n> +\t/** The media device path */\n> +\tstd::string path_;\n> +\n> +\tstd::map mediaObjects_;\n\ns/mediaObjects_/objects_/ ?\n\n> +\tMediaObject *getObject(unsigned int id);\n> +\tvoid addObject(MediaObject *obj);\n> +\tvoid deleteObjects();\n> +\n> +\tstd::vector entities_;\n> +\tMediaEntity *getEntityByName(const std::string &name);\n> +\n> +\tint enumerateEntities(std::map &entitiesMap,\n> +\t\t\t struct media_v2_topology &topology);\n> +\tint enumeratePads(struct media_v2_topology &topology);\n> +\tint enumerateLinks(struct media_v2_topology &topology);\n> +\n> +\tint setupLink(const MediaPad *source, const MediaPad *sink,\n> +\t\t MediaLink *link, unsigned int flags);\n> +\n> +\tvoid dumpLocal(MediaEntity *e, MediaPad *p, std::ostream &os);\n> +\tvoid dumpRemote(MediaLink *l, std::ostream &os);\n> +\tvoid dumpLink(MediaLink *l, std::ostream &os);\n> +};\n> +\n> +} /* namespace libcamera */\n> +#endif /* __LIBCAMERA_MEDIA_DEVICE_H__ */\n> diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp\n> new file mode 100644\n> index 0000000..b98d62f\n> --- /dev/null\n> +++ b/src/libcamera/media_device.cpp\n> @@ -0,0 +1,596 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2018, Google Inc.\n> + *\n> + * media_device.cpp - Media device handler\n> + */\n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +#include \n> +\n> +#include \n> +\n> +#include \"log.h\"\n> +#include \"media_device.h\"\n> +\n> +/**\n> + * \\file media_device.h\n> + */\n> +namespace libcamera {\n> +\n> +/**\n> + * \\class MediaDevice\n> + * \\brief Media device handler\n> + *\n> + * MediaDevice handles the media graph associated with a V4L2 media device.\n> + */\n> +\n> +/**\n> + * \\fn MediaDevice::~MediaDevice()\n> + * \\brief Close the media device file descriptor and release entities\n> + */\n> +MediaDevice::~MediaDevice()\n> +{\n> +\tif (fd_ > -1)\n\nI would have written != -1.\n\n> +\t\t::close(fd_);\n> +\tdeleteObjects();\n> +}\n> +\n> +/**\n> + * \\fn MediaDevice::name()\n> + * \\brief Return the media device name\n> + */\n> +\n> +/**\n> + * \\fn MediaDevice::path()\n> + * \\brief Return the media device path node associated with this\n> MediaDevice\n\nI'd name this devnode() instead of path().\n\n> + */\n> +\n> +/**\n> + * \\fn MediaDevice::deleteObjects()\n> + * \\brief Delete all registered entities in the MediaDevice object\n> + */\n> +void MediaDevice::deleteObjects()\n> +{\n> +\tfor (auto const &e : mediaObjects_)\n> +\t\tdelete e.second;\n> +\n> +\tmediaObjects_.clear();\n> +\tentities_.clear();\n> +}\n> +\n> +/**\n> + * \\fn int MediaDevice::open(std::string)\n\nDo you need to specify the arguments when there's a single function by that \nname ? If so, shouldn't it be (const std::string &) ?\n\nSame comment for other functions in this patch series.\n\n> + * \\brief Open a media device and initialize its components.\n> + * \\param path The media device path\n> + */\n> +int MediaDevice::open(const std::string &path)\n\ns/path/devnode/ ?\n\n> +{\n\nWhat if the device is already open ? It would be useful to add some \ndocumentation to explain the \"life cycle\" of the object, as in how it is \nsupposed to be used (and what happens when it's incorrectly used). This can be \ndone in the open() documentation (and referenced from close() and possibly \nother functions), or in the \\class documentation.\n\n> +\tfd_ = ::open(path.c_str(), O_RDWR);\n> +\tif (fd_ < 0) {\n> +\t\tLOG(Error) << \"Failed to open media device at \" << path\n> +\t\t\t << \": \" << strerror(errno);\n> +\t\treturn -errno;\n\nThe LOG() call may overwrite errno. You will need a local variable. How about\n\n\tint ret;\n\n\tret = ::open(path.c_str(), O_RDWR);\n\tif (ret < 0) {\n\t\tret = -errno;\n\t\tLOG(Error) << \"Failed to open media device at \" << path\n\t\t\t << \": \" << strerror(-ret);\n\t\treturn ret;\n\t}\n\n\tfd_ = ret;\n\tpath_ = path;\n\n> +\t}\n> +\tpath_ = path;\n> +\n> +\tstruct media_device_info info = { };\n> +\tint ret = ioctl(fd_, MEDIA_IOC_DEVICE_INFO, &info);\n> +\tif (ret) {\n> +\t\tLOG(Error) << \"Failed to get media device info \"\n> +\t\t\t << \": \" << strerror(errno);\n> +\t\treturn -errno;\n> +\t}\n> +\n> +\tname_ = info.driver;\n> +\n> +\treturn 0;\n> +}\n> +\n> +/**\n> + * \\fn MediaDevice::close()\n> + * \\brief Close the file descriptor associated with the media device\n> + */\n> +int MediaDevice::close()\n> +{\n> +\tif (fd_ > -1)\n\nI'd write this != -1 too.\n\n> +\t\treturn ::close(fd_);\n> +\n> +\treturn 0;\n\nDo you use the return value ?\n\n> +}\n> +\n> +void MediaDevice::addObject(MediaObject *obj)\n> +{\n> +\n> +\tif (mediaObjects_.find(obj->id()) != mediaObjects_.end()) {\n> +\t\tLOG(Error) << \"Element with id \" << obj->id()\n> +\t\t\t << \" already enumerated.\";\n> +\t\treturn;\n\nShouldn't you propagate the error to the caller ? This seems pretty fatal to \nme.\n\n> +\t}\n> +\n> +\tmediaObjects_[obj->id()] = obj;\n> +}\n> +\n> +MediaObject *MediaDevice::getObject(unsigned int id)\n> +{\n> +\tauto it = mediaObjects_.find(id);\n> +\treturn (it == mediaObjects_.end()) ?\n> +\t nullptr : it->second;\n> +}\n> +\n> +/**\n> + * \\fn MediaDevice::getEntityByName(std::string)\n> + * \\brief Return entity with name \\a name\n> + * \\param name The entity name\n\nPlease expand documentation a little bit. For instance you should explain that \nthis function returns nullptr if the entity can't be found. In general a one-\nline documentation that just duplicates the function name isn't very useful, \nthe real value comes from what isn't expressed in the name.\n\nDocumentation is hard :-)\n\n> + */\n> +MediaEntity *MediaDevice::getEntityByName(const std::string &name)\n> +{\n> +\tauto it = entities_.begin();\n> +\n> +\twhile (it != entities_.end()) {\n> +\t\tMediaEntity *e = *it;\n> +\t\tif (!(e->name().compare(name)))\n> +\t\t\treturn e;\n> +\t\tit++;\n> +\t}\n> +\n> +\treturn nullptr;\n> +}\n> +\n> +/**\n> + * \\fn MediaDevice::enumerateLinks(struct media_v2_topology &topology)\n> + * \\brief Enumerate all links in the system and associate them with their\n> + * source and sink pads\n> + * \\param topology The media topology as returned by MEDIA_IOC_G_TOPOLOGY\n> + */\n> +int MediaDevice::enumerateLinks(struct media_v2_topology &topology)\n\nShouldn't the argument be const ? I would have used a pointer instead of a \nreference, but I'm not sure why.\n\n> +{\n> +\tstruct media_v2_link *link = reinterpret_cast\n> +\t\t\t\t (topology.ptr_links);\n> +\n> +\tfor (unsigned int i = 0; i < topology.num_links; i++, link++) {\n> +\t\t/*\n> +\t\t * Skip links between entities and interfaces: we only care\n> +\t\t * about pad-2-pad links here.\n> +\t\t */\n> +\t\tif ((link->flags & MEDIA_LNK_FL_LINK_TYPE) ==\n> +\t\t MEDIA_LNK_FL_INTERFACE_LINK)\n> +\t\t\tcontinue;\n> +\n> +\t\tMediaLink *mediaLink = new MediaLink(link);\n\ns/mediaLink/link/ ?\n\nYou would need to rename the link variable above though, I'm not sure which \none is best.\n\n> +\t\taddObject(mediaLink);\n> +\n> +\t\t/* Store reference to this mediaLink in the link's source pad. */\n> +\t\tMediaPad *mediaPad = dynamic_cast\n> +\t\t\t\t (getObject(mediaLink->source()));\n\ns/mediaPad/pad/ ?\n\n> +\t\tif (!mediaPad) {\n> +\t\t\tLOG(Error) << \"Failed to find pad with id: \"\n\nNitpicking, s/://.\n\n> +\t\t\t\t << mediaLink->source();\n> +\t\t\treturn -ENODEV;\n> +\t\t}\n> +\t\tmediaPad->addLink(mediaLink);\n> +\n> +\t\t/* Store reference to this mediaLink in the link's sink pad. */\n> +\t\tmediaPad = dynamic_cast(getObject(mediaLink->sink()));\n> +\t\tif (!mediaPad) {\n> +\t\t\tLOG(Error) << \"Failed to find pad with id: \"\n> +\t\t\t\t << mediaLink->sink();\n> +\t\t\treturn -ENODEV;\n> +\t\t}\n> +\t\tmediaPad->addLink(mediaLink);\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> +/**\n> + * \\fn MediaDevice::enumeratePads(struct media_v2_topology &topology)\n> + * \\brief Enumerate all pads in the system and associate them with the\n> + * entity they belong to\n> + * \\param topology The media topology as returned by MEDIA_IOC_G_TOPOLOGY\n> + */\n> +int MediaDevice::enumeratePads(struct media_v2_topology &topology)\n> +{\n> +\tstruct media_v2_pad *pad = reinterpret_cast\n> +\t\t\t\t (topology.ptr_pads);\n> +\n> +\tfor (unsigned int i = 0; i < topology.num_pads; i++, pad++) {\n> +\t\tMediaPad *mediaPad = new MediaPad(pad);\n> +\t\taddObject(mediaPad);\n> +\n> +\t\t/* Store a reference to this MediaPad in pad's entity. */\n> +\t\tMediaEntity *mediaEntity = dynamic_cast\n> +\t\t\t\t\t (getObject(mediaPad->entity()));\n\ns/mediaPad/pad/ and s/mediaEntity/entity/ ?\n\n> +\t\tif (!mediaEntity) {\n> +\t\t\tLOG(Error) << \"Failed to find entity with id: \"\n> +\t\t\t\t << mediaPad->entity();\n> +\t\t\treturn -ENODEV;\n> +\t\t}\n> +\n> +\t\tmediaEntity->addPad(mediaPad);\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> +/**\n> + * \\fn MediaDevice::enumerateEntities(std::map &,\n> + * struct media_v2_topology &topology)\n> + * \\brief Enumerate and initialize entities in the media graph\n> + * \\param entitiesMap Map entities names to their video (sub)device node\n> + * \\param topology The media topology as returned by MEDIA_IOC_G_TOPOLOGY\n> + *\n> + * Associate the video (sub)device path to the entity name as returned by\n\nAs stated before, I'd talk about device nodes, as this isn't V4L2-specific.\n\n> + * MEDIA_IOC_G_TOPOLOGY\n> + */\n> +int MediaDevice::enumerateEntities(std::map\n> &entitiesMap,\n> +\t\t\t\t struct media_v2_topology &topology)\n> +{\n> +\tstruct media_v2_entity *entities = reinterpret_cast *>\n> +\t\t\t\t\t (topology.ptr_entities);\n> +\n> +\tfor (unsigned int i = 0; i < topology.num_entities; ++i) {\n> +\t\tauto it = entitiesMap.find(entities[i].name);\n> +\t\tif (it == entitiesMap.end()) {\n> +\t\t\tLOG(Error) << \"Entity \" << entities[i].name\n> +\t\t\t\t << \" not found in media entities map\";\n> +\t\t\treturn -ENOENT;\n> +\t\t}\n> +\n> +\t\tMediaEntity *entity = new MediaEntity(&entities[i]);\n> +\t\tif (entity->setDevice(it->second)) {\n> +\t\t\tdelete entity;\n> +\t\t\tgoto delete_entities;\n\nYou could call deleteObjects() and return here directly.\n\n> +\t\t}\n> +\n> +\t\taddObject(entity);\n> +\t\tentities_.push_back(entity);\n> +\t}\n> +\n> +\treturn 0;\n> +\n> +delete_entities:\n> +\tdeleteObjects();\n> +\n> +\treturn -errno;\n\n-errno isn't correct. You should assign the return value of setDevice() to a \nret variable and return ret.\n\n> +}\n> +\n> +/**\n> + * \\fn MediaDevice::enumerate(std::map)\n> + * \\brief Enumerate the media graph topology\n> + * \\param entitiesMap Map entities names to their video (sub)device node\n> + * FIXME: this is statically provided by the caller at the moment.\n> + *\n> + * This functions enumerates all media objects, registered in the media\n> graph,\n\ns/functions/function/\ns/,//g\n\n> + * through the MEDIA_IOC_G_TOPOLOGY ioctl. For each returned entity,\n> + * it creates and store its representation for later reuse.\n\nLet's expand this a little.\n\n\"This function enumerates all media graph objects in the media device and \npopulates the internal list of objects. All entities, pads and links are \nstored as MediaEntity, MediaPad and MediaLink respectively, with cross-\nreferences between objects. Media interfaces are not processed.\n\nFor entities paired with a device node, the path to the device node is stored \nin the MediaEntity object, based on the associations set in the \\a entitiesMap \nparameter.\n\n\\return Return 0 on success or a negative error code on error.\"\n\n> + */\n> +int MediaDevice::enumerate(std::map &entitiesMap)\n> +{\n> +\tstruct media_v2_topology topology = { };\n> +\tunsigned int num_interfaces;\n> +\tunsigned int num_links;\n> +\tunsigned int num_pads;\n> +\tunsigned int num_ent;\n> +\n> +\tdo {\n> +\t\tnum_ent = topology.num_entities;\n> +\t\tnum_pads = topology.num_pads;\n> +\t\tnum_links = topology.num_links;\n> +\t\tnum_interfaces = topology.num_interfaces;\n> +\n> +\t\t/* Call G_TOPOLOGY the first time here to enumerate .*/\n> +\t\tif (ioctl(fd_, MEDIA_IOC_G_TOPOLOGY, &topology)) {\n> +\t\t\tLOG(Error) << \"Failed to enumerate media topology on\"\n> +\t\t\t\t << path_ << \": \" << strerror(errno);\n> +\t\t\treturn -errno;\n> +\t\t}\n> +\n> +\t\t/*\n> +\t\t * Repeat the call until we don't get a 'stable' number\n> +\t\t * of media objects.\n> +\t\t */\n> +\t} while (num_ent != topology.num_entities ||\n> +\t\t num_pads != topology.num_pads ||\n> +\t\t num_links != topology.num_links ||\n> +\t\t num_interfaces != topology.num_interfaces);\n\nThat's not very useful, as the need to ensure that the topology doesn't change \nstems from the fact we call MEDIA_IOC_G_TOPOLOGY (at least) twice, once to \nretrieve the number of entities, and a second time to retrieve the graph \nitself. The graph could be updated in the kernel after this loop and before \nthe next MEDIA_IOC_G_TOPOLOGY.\n\nI proposed a way to call MEDIA_IOC_G_TOPOLOGY from a single location in a \nprevious e-mail. We can fix this on top of this series, or:\n\n\n\tstruct media_v2_topology topology = { };\n\tstruct media_v2_entity *entities;\n\tstruct media_v2_pad *pads;\n\tstruct media_v2_link *links;\n\tstruct media_v2_interface *interfaces;\n\tunsigned int num_entities;\n\tunsigned int num_pads;\n\tunsigned int num_links;\n\tunsigned int num_interfaces;\n\tint ret;\n\n\twhile (1) {\n\t\tnum_entitites = topology.num_entities;\n\t\tnum_pads = topology.num_pads;\n\t\tnum_links = topology.num_links;\n\t\tnum_interfaces = topology.num_interfaces;\n\n\t\tpads = new media_v2_pad[topology.num_pads];\n\t\tentities = new media_v2_entity[topology.num_entities];\n\t\ttopology.ptr_entities = reinterpret_cast<__u64>(entities);\n\n\t\ttopology.ptr_pads = reinterpret_cast<__u64>(pads);\n\n\t\tlinks = new media_v2_link[topology.num_links];\n\t\ttopology.ptr_links = reinterpret_cast<__u64>(links);\n\t\tlinks = new media_v2_interface[topology.num_interfaces];\n\t\ttopology.ptr_interfaces = reinterpret_cast<__u64>(interfaces);\n\n\t\tret = ioctl(fd_, MEDIA_IOC_G_TOPOLOGY, &topology);\n\t\tif (ret < 0) {\n\t\t\tret = -errno;\n\t\t\tLOG(Error) << \"Failed to enumerate media topology on\"\n\t\t\t\t << path_ << \": \" << strerror(-ret);\n\t\t\tgoto done;\n\t\t}\n\n\t\t/*\n\t\t * If all objects have been enumerated, stop now. If the number\n\t\t * of objects has increased (after the first call or due to a\n\t\t * topology change, retry the enumeration.\n\t\t */\n\t\tif (num_entities >= topology.num_entities &&\n\t\t num_pads >= topology.num_pads &&\n\t\t num_links >= topology.num_links &&\n\t\t num_interfaces >= topology.num_interfaces)\n\t\t\tbreak;\n\t}\n\n\tret = enumerateEntities(entitiesMap, topology);\n\tif (ret)\n\t\tgoto done;\n\n\tret = enumeratePads(topology);\n\tif (ret)\n\t\tgoto done;\n\n\tret = enumerateLinks(topology);\n\tif (ret)\n\t\tgoto done;\n\ndone:\n\tif (ret < 0)\n\t\tdeleteObjects();\n\n\tdelete[] entities;\n\tdelete[] links;\n\tdelete[] pads;\n\tdelete[] interfaces;\n\n\treturn ret;\n\n(untested)\n\nI wonder if enumerateEntities, enumeratePads and enumerateLinks should be \nrenamed as s/enumerate/populate/.\n\n> +\tauto *_ptr_e = new media_v2_entity[topology.num_entities];\n> +\ttopology.ptr_entities = reinterpret_cast<__u64>(_ptr_e);\n> +\n> +\tauto *_ptr_p = new media_v2_pad[topology.num_pads];\n> +\ttopology.ptr_pads = reinterpret_cast<__u64>(_ptr_p);\n> +\n> +\tauto *_ptr_l = new media_v2_link[topology.num_links];\n> +\ttopology.ptr_links = reinterpret_cast<__u64>(_ptr_l);\n> +\n> +\t/* Call G_TOPOLOGY again, this time with memory reserved. */\n> +\tint ret = ioctl(fd_, MEDIA_IOC_G_TOPOLOGY, &topology);\n> +\tif (ret < 0) {\n> +\t\tLOG(Error) << \"Failed to enumerate media topology on \" << path_\n> +\t\t\t << \": \" << strerror(errno);\n> +\t\tret = -errno;\n> +\t\tgoto error_free_mem;\n> +\t}\n> + \n> +\tret = enumerateEntities(entitiesMap, topology);\n> +\tif (ret)\n> +\t\tgoto error_free_mem;\n> +\n> +\tret = enumeratePads(topology);\n> +\tif (ret)\n> +\t\tgoto error_free_objs;\n> +\n> +\tret = enumerateLinks(topology);\n> +\tif (ret)\n> +\t\tgoto error_free_objs;\n> +\n> +\tdelete[] _ptr_e;\n> +\tdelete[] _ptr_p;\n> +\tdelete[] _ptr_l;\n> +\n> +\treturn 0;\n> +\n> +error_free_objs:\n> +\tdeleteObjects();\n> +\n> +error_free_mem:\n> +\tdelete[] _ptr_e;\n> +\tdelete[] _ptr_p;\n> +\tdelete[] _ptr_l;\n> +\n> +\treturn ret;\n> +}\n> +\n> +void MediaDevice::dumpLocal(MediaEntity *e, MediaPad *p, std::ostream &os)\n\ns/p/pad/\n\nUnless names get really long, let's try to spell them out, as it increases \nreadability.\n\n> +{\n> +\tos << \"\\t \\\"\" << e->name() << \"\\\"[\"\n> +\t << p->index() << \"]\";\n> +}\n> +\n> +void MediaDevice::dumpRemote(MediaLink *l, std::ostream &os)\n\ns/l/link/\n\n> +{\n> +\n\nStray blank line.\n\n> +\tMediaPad *remotePad = dynamic_cast\n> +\t\t\t (getObject(l->sink()));\n\nWould it make sense for the sink() and source() methods to return pointers to \nMediaPad instead of an id ? I would in that case store the pointers internally \nin MediaLink, to avoid a call to getObject() every time.\n\n> +\tif (!remotePad)\n> +\t\treturn;\n\nThis check wouldn't be needed then.\n\n> +\n> +\tMediaEntity *remoteEntity = dynamic_cast\n> +\t\t\t\t (getObject(remotePad->entity()));\n> +\tif (!remoteEntity)\n> +\t\treturn;\n\nSame here, returning a MediaEntity pointer, and removing the check.\n\n> +\tos << \"\\\"\" << remoteEntity->name() << \"\\\"[\"\n> +\t << remotePad->index() << \"]\";\n> +}\n> +\n> +void MediaDevice::dumpLink(MediaLink *l, std::ostream &os)\n> +{\n> +\tunsigned int flags = l->flags();\n> +\n> +\tos << \" [\";\n> +\tif (flags) {\n> +\t\tos << (flags & MEDIA_LNK_FL_ENABLED ? \"ENABLED,\" : \"\")\n> +\t\t << (flags & MEDIA_LNK_FL_IMMUTABLE ? \"IMMUTABLE\" : \"\");\n> +\t}\n> +\tos << \"]\\n\";\n> +}\n> +\n> +/**\n> + * \\fn MediaDevice::dumpGraph(std::ostream)\n> + * \\brief Dump the media device topology in textual form to an output\n> stream\n> + * \\param os The output stream where to append the printed topology to\n\ns/append/write/ ?\n\n> + */\n> +void MediaDevice::dumpGraph(std::ostream &os)\n> +{\n> +\tos << \"\\n\" << name_ << \" - \" << path_ << \"\\n\\n\";\n> +\n> +\tfor (auto const &e : entities_) {\n\ns/auto const &e/const struct MediaEntity */\n\nLet's restrict the use of auto to cases where it would be impractical to write \nthe type out explicitly, as auto reduces readability and removes compile-time \nchecks.\n\n> +\t\tos << \"\\\"\" << e->name() << \"\\\"\\n\";\n> +\n> +\t\tfor (auto const &p : e->sinks()) {\n> +\t\t\tos << \" [\" << p->index() << \"]\" << \": Sink\\n\";\n> +\t\t\tfor (auto const &l : p->links()) {\n> +\t\t\t\tdumpLocal(e, p, os);\n> +\t\t\t\tos << \" <- \";\n> +\t\t\t\tdumpRemote(l, os);\n> +\t\t\t\tdumpLink(l, os);\n> +\t\t\t}\n> +\t\t\tos << \"\\n\";\n> +\t\t}\n> +\n> +\t\tfor (auto const &p : e->sources()) {\n> +\t\t\tos << \" [\" << p->index() << \"]\" << \": Source\\n\";\n> +\t\t\tfor (auto const &l : p->links()) {\n> +\t\t\t\tdumpLocal(e, p, os);\n> +\t\t\t\tos << \" -> \";\n> +\t\t\t\tdumpRemote(l, os);\n> +\t\t\t\tdumpLink(l, os);\n> +\t\t\t}\n> +\t\t\tos << \"\\n\";\n> +\t\t}\n> +\t}\n> +}\n\nI wonder whether dumpGraph() belongs here, or whether it should be moved to a \ntest. MediaDevice isn't exposed through the public API, how do you foresee \ndumpGraph being used ? If it can never be called from the public API there's \nno point in having it in the library :-)\n\n> +/**\n> + * \\fn MediaDevice::setupLink(MediaPad *source, MediaPad *sink)\n\nyou're missing const (assuming we need to list the parameters explicitly here, \nsee the comment above).\n\n> + * \\brief Apply \\a flags to the link between \\a source and \\a sink pads\n> + * \\param source The source MediaPad\n> + * \\param sink The sink MediaPad\n> + * \\param link The MediaLink to operate on\n\nWhy do you need to specify both source and sink, and the link ? The link \nshould identify the source and sink already. If you don't expect the caller to \nhave the link pointer you should look it up internally, otherwise you \nshouldn't pass the source and sink. And if you expect the caller to have the \nlink pointer, maybe you could make this function a member of the MediaLink \nclass ?\n\n> + * \\param flags Flags to be applied to the link (MEDIA_LNK_FL_*)\n> + */\n> +int MediaDevice::setupLink(const MediaPad *source, const MediaPad *sink,\n> +\t\t\t MediaLink *link, unsigned int flags)\n> +{\n> +\tstruct media_link_desc linkDesc = { };\n> +\n> +\tlinkDesc.source.entity = source->entity();\n> +\tlinkDesc.source.index = source->index();\n> +\tlinkDesc.source.flags = MEDIA_PAD_FL_SOURCE;\n> +\n> +\tlinkDesc.sink.entity = sink->entity();\n> +\tlinkDesc.sink.index = sink->index();\n> +\tlinkDesc.sink.flags = MEDIA_PAD_FL_SINK;\n> +\n> +\tlinkDesc.flags = flags;\n> +\n> +\tif (ioctl(fd_, MEDIA_IOC_SETUP_LINK, &linkDesc)) {\n> +\t\tLOG(Error) << \"Failed to setup link: \"\n> +\t\t\t << strerror(errno);\n> +\t\treturn -errno;\n> +\t}\n> +\n> +\tlink->setFlags(0);\n> +\n> +\treturn 0;\n> +}\n> +\n> +/**\n> + * \\fn MediaDevice::resetLinks()\n> + * \\brief Reset all links on the media graph\n\nReset to what ? :-)\n\n> + *\n> + * Walk all registered entities, and disable all links from their\n> + * source pads to other pads.\n\nWouldn't it be more efficient to walk links instead of entities and pads ?\n\nCould you expand the documentation to explain the use cases for this function \n? Please also document the return value (this holds true for all other \nfunctions returning a value in the whole series).\n\n> + */\n> +int MediaDevice::resetLinks()\n> +{\n> +\tfor (MediaEntity *e : entities_) {\n> +\t\tfor (MediaPad *sourcePad : e->sources()) {\n> +\t\t\tfor (MediaLink *l : sourcePad->links()) {\n> +\t\t\t\t/*\n> +\t\t\t\t * Do not reset links that are not enabled\n> +\t\t\t\t * or immutable.\n> +\t\t\t\t */\n> +\t\t\t\tif (l->flags() & MEDIA_LNK_FL_IMMUTABLE)\n> +\t\t\t\t\tcontinue;\n> +\n> +\t\t\t\tif (!(l->flags() & MEDIA_LNK_FL_ENABLED))\n> +\t\t\t\t\tcontinue;\n> +\n> +\t\t\t\t/* Get the remote sink pad. */\n> +\t\t\t\tMediaPad *sinkPad = dynamic_cast\n> +\t\t\t\t\t\t (getObject(l->sink()));\n> +\t\t\t\tif (!sinkPad)\n> +\t\t\t\t\treturn -ENOENT;\n> +\n> +\t\t\t\t/* Also get entity to make sure IDs are ok. */\n> +\t\t\t\tMediaEntity *sinkEntity =\n> +\t\t\t\t\t\tdynamic_cast\n> +\t\t\t\t\t\t(getObject(sinkPad->entity()));\n> +\t\t\t\tif (!sinkEntity)\n> +\t\t\t\t\treturn -ENOENT;\n> +\n> +\t\t\t\tint ret = setupLink(sourcePad, sinkPad, l, 0);\n> +\t\t\t\tif (ret) {\n> +\t\t\t\t\tLOG(Error) << \"Link reset failed: \"\n> +\t\t\t\t\t\t << e->name() << \"[\"\n> +\t\t\t\t\t\t << sourcePad->index()\n> +\t\t\t\t\t\t << \"] -> \"\n> +\t\t\t\t\t\t << sinkEntity->name() << \"[\"\n> +\t\t\t\t\t\t << sinkPad->index() << \"]\";\n> +\t\t\t\t\treturn ret;\n> +\t\t\t\t}\n> +\n> +\t\t\t\tLOG(Info) << \"Link reset: \"\n> +\t\t\t\t\t << e->name() << \"[\"\n> +\t\t\t\t\t << sourcePad->index()\n> +\t\t\t\t\t << \"] -> \"\n> +\t\t\t\t\t << sinkEntity->name() << \"[\"\n> +\t\t\t\t\t << sinkPad->index() << \"]\";\n> +\t\t\t}\n> +\t\t}\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> +/**\n> + * \\fn MediaDevice::link(std::string, unsigned int, std::string, unsigned\n> int)\n> + * \\brief Setup a link identified by the entities name and their source and\n> + * sink pad indexes\n> + * \\param source The source entity name\n> + * \\param sourceIdx The source pad index\n> + * \\param sink The sink entity name\n> + * \\param sinkIdx The sink pad index\n> + * \\param flags The link setup flag (see MEDIA_LNK_FL_*)\n\nIf I understand the purpose of this function properly (and the fact that I'm \nnot sure I do means the documentation should be improved :-)), it serves the \nsame purpose as MediaDevice::setupLink(), but with different arguments. I \nwould thus name the two functions setupLink(), as C++ allows overloading \nfunctions.\n\nI would go one step further though, at least if you agree with my proposition \nto move setupLink to MediaLink, and turn this function into a link lookup. The \ncallers would then do something similar to\n\n\tMediaDevice *dev = ...;\n\tMediaLink *link = dev->link(\"source\", 0, \"sink\", 3);\n\tif (!link)\n\t\treturn -ENODEV;\n\tlink->setup(flags);\n\nand possibly cache the link pointer internally to avoid looking up links all \nthe time.\n\n> + */\n> +int MediaDevice::link(const std::string &source, unsigned int sourceIdx,\n> +\t\t const std::string &sink, unsigned int sinkIdx,\n\nAt the cost of longer names, sourcePadIndex and sinkPadIndex ?\n\n> +\t\t unsigned int flags)\n> +{\n> +\n\nStray blank line.\n\n> +\t/* Make sure the supplied link is well formed. */\n> +\tMediaEntity *sourceEntity = getEntityByName(source);\n> +\tif (!sourceEntity) {\n> +\t\tLOG(Error) << \"Entity name: \" << source << \"not found\";\n> +\t\treturn -ENOENT;\n> +\t}\n> +\n> +\tMediaEntity *sinkEntity = getEntityByName(sink);\n> +\tif (!sinkEntity) {\n> +\t\tLOG(Error) << \"Entity name: \" << source << \"not found\";\n\ns/source/sink/\ns/Entity name:/Entity/\n\n> +\t\treturn -ENOENT;\n> +\t}\n> +\n> +\tconst MediaPad *sourcePad = sourceEntity->getPadByIndex(sourceIdx);\n> +\tif (!sourcePad) {\n> +\t\tLOG(Error) << \"Pad \" << sourceIdx << \"not found in entity \"\n> +\t\t\t << sourceEntity->name();\n> +\t\treturn -ENOENT;\n> +\t}\n> +\n> +\tconst MediaPad *sinkPad = sinkEntity->getPadByIndex(sinkIdx);\n> +\tif (!sinkPad) {\n> +\t\tLOG(Error) << \"Pad \" << sinkIdx << \"not found in entity \"\n> +\t\t\t << sinkEntity->name();\n> +\t\treturn -ENOENT;\n> +\t}\n> +\n> +\t/*\n> +\t * Walk all links in the source and search for an entry matching the\n> +\t * pad ids. If none, the requested link does not exists.\n> +\t */\n> +\tMediaLink *validLink = nullptr;\n> +\tfor (MediaLink *link : sourcePad->links()) {\n> +\t\tif (link->source() != sourcePad->id())\n> +\t\t\tcontinue;\n> +\n> +\t\tif (link->sink() != sinkPad->id())\n> +\t\t\tcontinue;\n> +\n> +\t\tvalidLink = link;\n> +\t\tbreak;\n> +\t}\n> +\n> +\tif (!validLink) {\n> +\t\tLOG(Error) << \"Link not found\"\n> +\t\t\t << \"\\\"\" << sourceEntity->name() << \"\\\"[\"\n> +\t\t\t << sourcePad->index() << \"] -> \"\n> +\t\t\t << \"\\\"\" << sinkEntity->name() << \"\\\"[\"\n> +\t\t\t << sinkPad->index() << \"]\";\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tint ret = setupLink(sourcePad, sinkPad, validLink, flags);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\tLOG(Info) << \"Setup link: \"\n\nLOG(Debug) at most.\n\n> +\t\t << \"\\\"\" << sourceEntity->name() << \"\\\"[\"\n> +\t\t << sourcePad->index() << \"] -> \"\n> +\t\t << \"\\\"\" << sinkEntity->name() << \"\\\"[\"\n> +\t\t << sinkPad->index() << \"]\"\n> +\t\t << \" [\" << flags << \"]\";\n> +\n> +\treturn 0;\n> +}\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index da06eba..4cac687 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -2,6 +2,7 @@ libcamera_sources = files([\n> 'log.cpp',\n> 'main.cpp',\n> 'media_object.cpp',\n> + 'media_device.cpp',\n\nsort\n\n> ])\n> \n> libcamera_headers = files([","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 67D1F60B2E\n\tfor ;\n\tFri, 28 Dec 2018 14:26:24 +0100 (CET)","from avalon.localnet (unknown\n\t[IPv6:2a02:2788:66a:3eb:2624:a446:f4b7:b19d])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 9C11FDF;\n\tFri, 28 Dec 2018 14:26:23 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1546003583;\n\tbh=xTY+vEChCUva4uejCx1bDnlYpqg4hRdo1WoXxea9hi8=;\n\th=From:To:Cc:Subject:Date:In-Reply-To:References:From;\n\tb=OfCndZVdoQkN0DmQ7WusTijvZI1UynSH5fwi7KwgZQQSqGAR8Ln4WL2hdzmvQpxGN\n\tOcU4aBhKkAhIyZ9MHe65o9q/c113Sdz9kigYkO1ox+HRugZyOl2lfvvSOsx0/KzMwu\n\twYOz1EFNKwBI3OC4fADUIumyeQOX4V1LQovwEmeM=","From":"Laurent Pinchart ","To":"libcamera-devel@lists.libcamera.org","Date":"Fri, 28 Dec 2018 15:27:21 +0200","Message-ID":"<1597883.HRDAXlUJTF@avalon>","Organization":"Ideas on Board Oy","In-Reply-To":"<20181228075743.28637-3-jacopo@jmondi.org>","References":"<20181228075743.28637-1-jacopo@jmondi.org>\n\t<20181228075743.28637-3-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Transfer-Encoding":"7Bit","Content-Type":"text/plain; charset=\"us-ascii\"","Subject":"Re: [libcamera-devel] [PATCH RESEND v2 2/4] libcamera: Add\n\tMediaDevice class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Fri, 28 Dec 2018 13:26:24 -0000"}},{"id":98,"web_url":"https://patchwork.libcamera.org/comment/98/","msgid":"<20181228143652.GH909@uno.localdomain>","date":"2018-12-28T14:36:52","subject":"Re: [libcamera-devel] [PATCH RESEND v2 2/4] libcamera: Add\n\tMediaDevice class","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"HI Laurent, quite a few comments...\n\nOn Fri, Dec 28, 2018 at 03:27:21PM +0200, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Friday, 28 December 2018 09:57:41 EET Jacopo Mondi wrote:\n> > The MediaDevice object implements handling and configuration of the media\n> > graph associated with a V4L2 media device.\n>\n> Let's not talk about V4L2 here, it's just MC, not V4L2.\n>\n> > The class allows enumeration of all pads, links and entities registered in\n> > the media graph, and provides methods to setup and reset media links.\n> >\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/libcamera/include/media_device.h | 71 ++++\n> > src/libcamera/media_device.cpp | 596 +++++++++++++++++++++++++++\n> > src/libcamera/meson.build | 1 +\n> > 3 files changed, 668 insertions(+)\n> > create mode 100644 src/libcamera/include/media_device.h\n> > create mode 100644 src/libcamera/media_device.cpp\n> >\n> > diff --git a/src/libcamera/include/media_device.h\n> > b/src/libcamera/include/media_device.h new file mode 100644\n> > index 0000000..642eea9\n> > --- /dev/null\n> > +++ b/src/libcamera/include/media_device.h\n> > @@ -0,0 +1,71 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2018, Google Inc.\n> > + *\n> > + * media_device.h - Media device handler\n> > + */\n> > +#ifndef __LIBCAMERA_MEDIA_DEVICE_H__\n> > +#define __LIBCAMERA_MEDIA_DEVICE_H__\n> > +\n> > +#include \n> > +#include \n> > +#include \n>\n> sort\n>\n> > +#include \n> > +\n> > +#include \n> > +\n> > +#include \"log.h\"\n>\n> Is this needed ?\n>\n> > +#include \"media_object.h\"\n> > +\n> > +namespace libcamera {\n> > +\n> > +class MediaDevice\n> > +{\n> > +public:\n> > +\tMediaDevice() : fd_(-1) { };\n> > +\t~MediaDevice();\n> > +\n> > +\tconst std::string name() const { return name_; }\n> > +\tconst std::string path() const { return path_; }\n> > +\n> > +\tint open(const std::string &path);\n> > +\tint close();\n> > +\tint enumerate(std::map &entitiesMap);\n> > +\tvoid dumpGraph(std::ostream &os);\n> > +\n> > +\tint resetLinks();\n> > +\tint link(const std::string &source, unsigned int sourceIdx,\n> > +\t\t const std::string &sink, unsigned int sinkIdx,\n> > +\t\t unsigned int flags);\n> > +\n> > +private:\n> > +\t/** The media device file descriptor */\n>\n> Wrong file ?\n>\nSorry, I didn't get this.\n\n> > +\tint fd_;\n> > +\t/** The media device name as returned by MEDIA_IOC_DEVICE_INFO */\n>\n> Please take my comments on 1/4 into account regarding wording of documentation\n> here.\n\nI'll drop documentation from private members\n\n>\n> This field stores the driver name, I would thus name it driver_ (or possibly\n> driverName_).\n>\n> > +\tstd::string name_;\n> > +\t/** The media device path */\n> > +\tstd::string path_;\n> > +\n> > +\tstd::map mediaObjects_;\n>\n> s/mediaObjects_/objects_/ ?\n>\n\nActually I don't agree, here and all other places in the patch where\nan s/mediaSomething/something/ has been suggested.\n\nThis class deals both with struct_v2_something and MediaSomething. I\nwould like to keep clear when we're operating on the object\nrepresentation and when on the kernel provided structure.\n\n> > +\tMediaObject *getObject(unsigned int id);\n> > +\tvoid addObject(MediaObject *obj);\n> > +\tvoid deleteObjects();\n> > +\n> > +\tstd::vector entities_;\n> > +\tMediaEntity *getEntityByName(const std::string &name);\n> > +\n> > +\tint enumerateEntities(std::map &entitiesMap,\n> > +\t\t\t struct media_v2_topology &topology);\n> > +\tint enumeratePads(struct media_v2_topology &topology);\n> > +\tint enumerateLinks(struct media_v2_topology &topology);\n> > +\n> > +\tint setupLink(const MediaPad *source, const MediaPad *sink,\n> > +\t\t MediaLink *link, unsigned int flags);\n> > +\n> > +\tvoid dumpLocal(MediaEntity *e, MediaPad *p, std::ostream &os);\n> > +\tvoid dumpRemote(MediaLink *l, std::ostream &os);\n> > +\tvoid dumpLink(MediaLink *l, std::ostream &os);\n> > +};\n> > +\n> > +} /* namespace libcamera */\n> > +#endif /* __LIBCAMERA_MEDIA_DEVICE_H__ */\n> > diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp\n> > new file mode 100644\n> > index 0000000..b98d62f\n> > --- /dev/null\n> > +++ b/src/libcamera/media_device.cpp\n> > @@ -0,0 +1,596 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2018, Google Inc.\n> > + *\n> > + * media_device.cpp - Media device handler\n> > + */\n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +\n> > +#include \"log.h\"\n> > +#include \"media_device.h\"\n> > +\n> > +/**\n> > + * \\file media_device.h\n> > + */\n> > +namespace libcamera {\n> > +\n> > +/**\n> > + * \\class MediaDevice\n> > + * \\brief Media device handler\n> > + *\n> > + * MediaDevice handles the media graph associated with a V4L2 media device.\n> > + */\n> > +\n> > +/**\n> > + * \\fn MediaDevice::~MediaDevice()\n> > + * \\brief Close the media device file descriptor and release entities\n> > + */\n> > +MediaDevice::~MediaDevice()\n> > +{\n> > +\tif (fd_ > -1)\n>\n> I would have written != -1.\n>\n\nAny reason why it's better?\n\n> > +\t\t::close(fd_);\n> > +\tdeleteObjects();\n> > +}\n> > +\n> > +/**\n> > + * \\fn MediaDevice::name()\n> > + * \\brief Return the media device name\n> > + */\n> > +\n> > +/**\n> > + * \\fn MediaDevice::path()\n> > + * \\brief Return the media device path node associated with this\n> > MediaDevice\n>\n> I'd name this devnode() instead of path().\n>\n\nI already though about chnging this, right...\n\n> > + */\n> > +\n> > +/**\n> > + * \\fn MediaDevice::deleteObjects()\n> > + * \\brief Delete all registered entities in the MediaDevice object\n> > + */\n> > +void MediaDevice::deleteObjects()\n> > +{\n> > +\tfor (auto const &e : mediaObjects_)\n> > +\t\tdelete e.second;\n> > +\n> > +\tmediaObjects_.clear();\n> > +\tentities_.clear();\n> > +}\n> > +\n> > +/**\n> > + * \\fn int MediaDevice::open(std::string)\n>\n> Do you need to specify the arguments when there's a single function by that\n> name ? If so, shouldn't it be (const std::string &) ?\n>\n> Same comment for other functions in this patch series.\n\nI think I could drop the function name completely when the comment\nblock is just before the function definition.\n\n>\n> > + * \\brief Open a media device and initialize its components.\n> > + * \\param path The media device path\n> > + */\n> > +int MediaDevice::open(const std::string &path)\n>\n> s/path/devnode/ ?\n>\n> > +{\n>\n> What if the device is already open ? It would be useful to add some\n> documentation to explain the \"life cycle\" of the object, as in how it is\n> supposed to be used (and what happens when it's incorrectly used). This can be\n> done in the open() documentation (and referenced from close() and possibly\n> other functions), or in the \\class documentation.\n>\n\nRight, currently there's no protection for that in this\nimplementation. I should return an error if (fd != -1).\n\nI could just state the caller is responsible for opening and closing\nthe device and that access is exclusive. In the future, this might be\nsuperseded by ref-counted acquire/release operations, with open() and\nclose() then made private.\n\n> > +\tfd_ = ::open(path.c_str(), O_RDWR);\n> > +\tif (fd_ < 0) {\n> > +\t\tLOG(Error) << \"Failed to open media device at \" << path\n> > +\t\t\t << \": \" << strerror(errno);\n> > +\t\treturn -errno;\n>\n> The LOG() call may overwrite errno. You will need a local variable. How about\n>\n> \tint ret;\n>\n> \tret = ::open(path.c_str(), O_RDWR);\n> \tif (ret < 0) {\n> \t\tret = -errno;\n> \t\tLOG(Error) << \"Failed to open media device at \" << path\n> \t\t\t << \": \" << strerror(-ret);\n> \t\treturn ret;\n> \t}\n>\n> \tfd_ = ret;\n> \tpath_ = path;\n>\n\nRight. Thanks, this is important, I would like to see all error\nhandling standardized in the library.\n\n> > +\t}\n> > +\tpath_ = path;\n> > +\n> > +\tstruct media_device_info info = { };\n> > +\tint ret = ioctl(fd_, MEDIA_IOC_DEVICE_INFO, &info);\n> > +\tif (ret) {\n> > +\t\tLOG(Error) << \"Failed to get media device info \"\n> > +\t\t\t << \": \" << strerror(errno);\n> > +\t\treturn -errno;\n> > +\t}\n> > +\n> > +\tname_ = info.driver;\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +/**\n> > + * \\fn MediaDevice::close()\n> > + * \\brief Close the file descriptor associated with the media device\n> > + */\n> > +int MediaDevice::close()\n> > +{\n> > +\tif (fd_ > -1)\n>\n> I'd write this != -1 too.\n>\n> > +\t\treturn ::close(fd_);\n> > +\n> > +\treturn 0;\n>\n> Do you use the return value ?\n>\n> > +}\n> > +\n> > +void MediaDevice::addObject(MediaObject *obj)\n> > +{\n> > +\n> > +\tif (mediaObjects_.find(obj->id()) != mediaObjects_.end()) {\n> > +\t\tLOG(Error) << \"Element with id \" << obj->id()\n> > +\t\t\t << \" already enumerated.\";\n> > +\t\treturn;\n>\n> Shouldn't you propagate the error to the caller ? This seems pretty fatal to\n> me.\n>\n\nThis should not happen of course. In case it does, I'll return\n-EEXIST.\n\n> > +\t}\n> > +\n> > +\tmediaObjects_[obj->id()] = obj;\n> > +}\n> > +\n> > +MediaObject *MediaDevice::getObject(unsigned int id)\n> > +{\n> > +\tauto it = mediaObjects_.find(id);\n> > +\treturn (it == mediaObjects_.end()) ?\n> > +\t nullptr : it->second;\n> > +}\n> > +\n> > +/**\n> > + * \\fn MediaDevice::getEntityByName(std::string)\n> > + * \\brief Return entity with name \\a name\n> > + * \\param name The entity name\n>\n> Please expand documentation a little bit. For instance you should explain that\n> this function returns nullptr if the entity can't be found. In general a one-\n> line documentation that just duplicates the function name isn't very useful,\n> the real value comes from what isn't expressed in the name.\n>\n\nI still feel that a function named getEntityByName() is pretty clear\nwithout having to document it in detail. The return value though might\nbe pointed out, even if, it's exactly what one would expect, the\nentity or nullptr.\n\n> Documentation is hard :-)\n>\n\nDrawing the line between useful documentation and writing comments to\nsilence doxygen \"not documented\" warning is. I clearly went for the second\noption and documented everything doxygen complained about but that's\nclearly not useful if not for ticking a box. I would be happy to chop\ndocumentation parts here and there.\n\n> > + */\n> > +MediaEntity *MediaDevice::getEntityByName(const std::string &name)\n> > +{\n> > +\tauto it = entities_.begin();\n> > +\n> > +\twhile (it != entities_.end()) {\n> > +\t\tMediaEntity *e = *it;\n> > +\t\tif (!(e->name().compare(name)))\n> > +\t\t\treturn e;\n> > +\t\tit++;\n> > +\t}\n> > +\n> > +\treturn nullptr;\n> > +}\n> > +\n> > +/**\n> > + * \\fn MediaDevice::enumerateLinks(struct media_v2_topology &topology)\n> > + * \\brief Enumerate all links in the system and associate them with their\n> > + * source and sink pads\n> > + * \\param topology The media topology as returned by MEDIA_IOC_G_TOPOLOGY\n> > + */\n> > +int MediaDevice::enumerateLinks(struct media_v2_topology &topology)\n>\n> Shouldn't the argument be const ? I would have used a pointer instead of a\n> reference, but I'm not sure why.\n>\n> > +{\n> > +\tstruct media_v2_link *link = reinterpret_cast\n> > +\t\t\t\t (topology.ptr_links);\n> > +\n> > +\tfor (unsigned int i = 0; i < topology.num_links; i++, link++) {\n> > +\t\t/*\n> > +\t\t * Skip links between entities and interfaces: we only care\n> > +\t\t * about pad-2-pad links here.\n> > +\t\t */\n> > +\t\tif ((link->flags & MEDIA_LNK_FL_LINK_TYPE) ==\n> > +\t\t MEDIA_LNK_FL_INTERFACE_LINK)\n> > +\t\t\tcontinue;\n> > +\n> > +\t\tMediaLink *mediaLink = new MediaLink(link);\n>\n> s/mediaLink/link/ ?\n>\n> You would need to rename the link variable above though, I'm not sure which\n> one is best.\n\nThat's what I meant. link for media_v2_link, and mediaLink for\nMediaLink. It makes sense to me.\n\n>\n> > +\t\taddObject(mediaLink);\n> > +\n> > +\t\t/* Store reference to this mediaLink in the link's source pad. */\n> > +\t\tMediaPad *mediaPad = dynamic_cast\n> > +\t\t\t\t (getObject(mediaLink->source()));\n>\n> s/mediaPad/pad/ ?\n>\n\nditto\n\n> > +\t\tif (!mediaPad) {\n> > +\t\t\tLOG(Error) << \"Failed to find pad with id: \"\n>\n> Nitpicking, s/://.\n>\n> > +\t\t\t\t << mediaLink->source();\n> > +\t\t\treturn -ENODEV;\n> > +\t\t}\n> > +\t\tmediaPad->addLink(mediaLink);\n> > +\n> > +\t\t/* Store reference to this mediaLink in the link's sink pad. */\n> > +\t\tmediaPad = dynamic_cast(getObject(mediaLink->sink()));\n> > +\t\tif (!mediaPad) {\n> > +\t\t\tLOG(Error) << \"Failed to find pad with id: \"\n> > +\t\t\t\t << mediaLink->sink();\n> > +\t\t\treturn -ENODEV;\n> > +\t\t}\n> > +\t\tmediaPad->addLink(mediaLink);\n> > +\t}\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +/**\n> > + * \\fn MediaDevice::enumeratePads(struct media_v2_topology &topology)\n> > + * \\brief Enumerate all pads in the system and associate them with the\n> > + * entity they belong to\n> > + * \\param topology The media topology as returned by MEDIA_IOC_G_TOPOLOGY\n> > + */\n> > +int MediaDevice::enumeratePads(struct media_v2_topology &topology)\n> > +{\n> > +\tstruct media_v2_pad *pad = reinterpret_cast\n> > +\t\t\t\t (topology.ptr_pads);\n> > +\n> > +\tfor (unsigned int i = 0; i < topology.num_pads; i++, pad++) {\n> > +\t\tMediaPad *mediaPad = new MediaPad(pad);\n> > +\t\taddObject(mediaPad);\n> > +\n> > +\t\t/* Store a reference to this MediaPad in pad's entity. */\n> > +\t\tMediaEntity *mediaEntity = dynamic_cast\n> > +\t\t\t\t\t (getObject(mediaPad->entity()));\n>\n> s/mediaPad/pad/ and s/mediaEntity/entity/ ?\n>\n\nditto ditto ;)\n\n> > +\t\tif (!mediaEntity) {\n> > +\t\t\tLOG(Error) << \"Failed to find entity with id: \"\n> > +\t\t\t\t << mediaPad->entity();\n> > +\t\t\treturn -ENODEV;\n> > +\t\t}\n> > +\n> > +\t\tmediaEntity->addPad(mediaPad);\n> > +\t}\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +/**\n> > + * \\fn MediaDevice::enumerateEntities(std::map &,\n> > + * struct media_v2_topology &topology)\n> > + * \\brief Enumerate and initialize entities in the media graph\n> > + * \\param entitiesMap Map entities names to their video (sub)device node\n> > + * \\param topology The media topology as returned by MEDIA_IOC_G_TOPOLOGY\n> > + *\n> > + * Associate the video (sub)device path to the entity name as returned by\n>\n> As stated before, I'd talk about device nodes, as this isn't V4L2-specific.\n>\n> > + * MEDIA_IOC_G_TOPOLOGY\n> > + */\n> > +int MediaDevice::enumerateEntities(std::map\n> > &entitiesMap,\n> > +\t\t\t\t struct media_v2_topology &topology)\n> > +{\n> > +\tstruct media_v2_entity *entities = reinterpret_cast > *>\n> > +\t\t\t\t\t (topology.ptr_entities);\n> > +\n> > +\tfor (unsigned int i = 0; i < topology.num_entities; ++i) {\n> > +\t\tauto it = entitiesMap.find(entities[i].name);\n> > +\t\tif (it == entitiesMap.end()) {\n> > +\t\t\tLOG(Error) << \"Entity \" << entities[i].name\n> > +\t\t\t\t << \" not found in media entities map\";\n> > +\t\t\treturn -ENOENT;\n> > +\t\t}\n> > +\n> > +\t\tMediaEntity *entity = new MediaEntity(&entities[i]);\n> > +\t\tif (entity->setDevice(it->second)) {\n> > +\t\t\tdelete entity;\n> > +\t\t\tgoto delete_entities;\n>\n> You could call deleteObjects() and return here directly.\n>\n> > +\t\t}\n> > +\n> > +\t\taddObject(entity);\n> > +\t\tentities_.push_back(entity);\n> > +\t}\n> > +\n> > +\treturn 0;\n> > +\n> > +delete_entities:\n> > +\tdeleteObjects();\n> > +\n> > +\treturn -errno;\n>\n> -errno isn't correct. You should assign the return value of setDevice() to a\n> ret variable and return ret.\n>\n> > +}\n> > +\n> > +/**\n> > + * \\fn MediaDevice::enumerate(std::map)\n> > + * \\brief Enumerate the media graph topology\n> > + * \\param entitiesMap Map entities names to their video (sub)device node\n> > + * FIXME: this is statically provided by the caller at the moment.\n> > + *\n> > + * This functions enumerates all media objects, registered in the media\n> > graph,\n>\n> s/functions/function/\n> s/,//g\n>\n> > + * through the MEDIA_IOC_G_TOPOLOGY ioctl. For each returned entity,\n> > + * it creates and store its representation for later reuse.\n>\n> Let's expand this a little.\n>\n> \"This function enumerates all media graph objects in the media device and\n> populates the internal list of objects. All entities, pads and links are\n> stored as MediaEntity, MediaPad and MediaLink respectively, with cross-\n> references between objects. Media interfaces are not processed.\n>\n> For entities paired with a device node, the path to the device node is stored\n> in the MediaEntity object, based on the associations set in the \\a entitiesMap\n> parameter.\n>\n> \\return Return 0 on success or a negative error code on error.\"\n>\n> > + */\n> > +int MediaDevice::enumerate(std::map &entitiesMap)\n> > +{\n> > +\tstruct media_v2_topology topology = { };\n> > +\tunsigned int num_interfaces;\n> > +\tunsigned int num_links;\n> > +\tunsigned int num_pads;\n> > +\tunsigned int num_ent;\n> > +\n> > +\tdo {\n> > +\t\tnum_ent = topology.num_entities;\n> > +\t\tnum_pads = topology.num_pads;\n> > +\t\tnum_links = topology.num_links;\n> > +\t\tnum_interfaces = topology.num_interfaces;\n> > +\n> > +\t\t/* Call G_TOPOLOGY the first time here to enumerate .*/\n> > +\t\tif (ioctl(fd_, MEDIA_IOC_G_TOPOLOGY, &topology)) {\n> > +\t\t\tLOG(Error) << \"Failed to enumerate media topology on\"\n> > +\t\t\t\t << path_ << \": \" << strerror(errno);\n> > +\t\t\treturn -errno;\n> > +\t\t}\n> > +\n> > +\t\t/*\n> > +\t\t * Repeat the call until we don't get a 'stable' number\n> > +\t\t * of media objects.\n> > +\t\t */\n> > +\t} while (num_ent != topology.num_entities ||\n> > +\t\t num_pads != topology.num_pads ||\n> > +\t\t num_links != topology.num_links ||\n> > +\t\t num_interfaces != topology.num_interfaces);\n>\n> That's not very useful, as the need to ensure that the topology doesn't change\n> stems from the fact we call MEDIA_IOC_G_TOPOLOGY (at least) twice, once to\n> retrieve the number of entities, and a second time to retrieve the graph\n> itself. The graph could be updated in the kernel after this loop and before\n> the next MEDIA_IOC_G_TOPOLOGY.\n>\n> I proposed a way to call MEDIA_IOC_G_TOPOLOGY from a single location in a\n> previous e-mail. We can fix this on top of this series, or:\n>\n>\n> \tstruct media_v2_topology topology = { };\n> \tstruct media_v2_entity *entities;\n> \tstruct media_v2_pad *pads;\n> \tstruct media_v2_link *links;\n> \tstruct media_v2_interface *interfaces;\n> \tunsigned int num_entities;\n> \tunsigned int num_pads;\n> \tunsigned int num_links;\n> \tunsigned int num_interfaces;\n> \tint ret;\n>\n> \twhile (1) {\n> \t\tnum_entitites = topology.num_entities;\n> \t\tnum_pads = topology.num_pads;\n> \t\tnum_links = topology.num_links;\n> \t\tnum_interfaces = topology.num_interfaces;\n>\n> \t\tpads = new media_v2_pad[topology.num_pads];\n> \t\tentities = new media_v2_entity[topology.num_entities];\n> \t\ttopology.ptr_entities = reinterpret_cast<__u64>(entities);\n>\n> \t\ttopology.ptr_pads = reinterpret_cast<__u64>(pads);\n>\n> \t\tlinks = new media_v2_link[topology.num_links];\n> \t\ttopology.ptr_links = reinterpret_cast<__u64>(links);\n> \t\tlinks = new media_v2_interface[topology.num_interfaces];\n> \t\ttopology.ptr_interfaces = reinterpret_cast<__u64>(interfaces);\n>\n> \t\tret = ioctl(fd_, MEDIA_IOC_G_TOPOLOGY, &topology);\n> \t\tif (ret < 0) {\n> \t\t\tret = -errno;\n> \t\t\tLOG(Error) << \"Failed to enumerate media topology on\"\n> \t\t\t\t << path_ << \": \" << strerror(-ret);\n> \t\t\tgoto done;\n> \t\t}\n>\n> \t\t/*\n> \t\t * If all objects have been enumerated, stop now. If the number\n> \t\t * of objects has increased (after the first call or due to a\n> \t\t * topology change, retry the enumeration.\n> \t\t */\n> \t\tif (num_entities >= topology.num_entities &&\n> \t\t num_pads >= topology.num_pads &&\n> \t\t num_links >= topology.num_links &&\n> \t\t num_interfaces >= topology.num_interfaces)\n> \t\t\tbreak;\n> \t}\n\nAccording to documentation (and Niklas' implementation I have copied,\nchecking that topology->version does not change should be enough. Do\nyou agree?\n\nBy the way, I appreciate the concern regarding dynamic media graphs,\nbut we're here checking that media graph does not change in between\ntwo ioctl calls, which is quite a strict race window, while it could\nchange at any other point in time after this call, and we won't\nnotice. I wonder if this is worth, but since I have code from you a\nNiklas, I'll simply copy it in.\n>\n> \tret = enumerateEntities(entitiesMap, topology);\n> \tif (ret)\n> \t\tgoto done;\n>\n> \tret = enumeratePads(topology);\n> \tif (ret)\n> \t\tgoto done;\n>\n> \tret = enumerateLinks(topology);\n> \tif (ret)\n> \t\tgoto done;\n>\n> done:\n> \tif (ret < 0)\n> \t\tdeleteObjects();\n>\n> \tdelete[] entities;\n> \tdelete[] links;\n> \tdelete[] pads;\n> \tdelete[] interfaces;\n>\n> \treturn ret;\n>\n> (untested)\n>\n> I wonder if enumerateEntities, enumeratePads and enumerateLinks should be\n> renamed as s/enumerate/populate/.\n>\n> > +\tauto *_ptr_e = new media_v2_entity[topology.num_entities];\n> > +\ttopology.ptr_entities = reinterpret_cast<__u64>(_ptr_e);\n> > +\n> > +\tauto *_ptr_p = new media_v2_pad[topology.num_pads];\n> > +\ttopology.ptr_pads = reinterpret_cast<__u64>(_ptr_p);\n> > +\n> > +\tauto *_ptr_l = new media_v2_link[topology.num_links];\n> > +\ttopology.ptr_links = reinterpret_cast<__u64>(_ptr_l);\n> > +\n> > +\t/* Call G_TOPOLOGY again, this time with memory reserved. */\n> > +\tint ret = ioctl(fd_, MEDIA_IOC_G_TOPOLOGY, &topology);\n> > +\tif (ret < 0) {\n> > +\t\tLOG(Error) << \"Failed to enumerate media topology on \" << path_\n> > +\t\t\t << \": \" << strerror(errno);\n> > +\t\tret = -errno;\n> > +\t\tgoto error_free_mem;\n> > +\t}\n> > +\n> > +\tret = enumerateEntities(entitiesMap, topology);\n> > +\tif (ret)\n> > +\t\tgoto error_free_mem;\n> > +\n> > +\tret = enumeratePads(topology);\n> > +\tif (ret)\n> > +\t\tgoto error_free_objs;\n> > +\n> > +\tret = enumerateLinks(topology);\n> > +\tif (ret)\n> > +\t\tgoto error_free_objs;\n> > +\n> > +\tdelete[] _ptr_e;\n> > +\tdelete[] _ptr_p;\n> > +\tdelete[] _ptr_l;\n> > +\n> > +\treturn 0;\n> > +\n> > +error_free_objs:\n> > +\tdeleteObjects();\n> > +\n> > +error_free_mem:\n> > +\tdelete[] _ptr_e;\n> > +\tdelete[] _ptr_p;\n> > +\tdelete[] _ptr_l;\n> > +\n> > +\treturn ret;\n> > +}\n> > +\n> > +void MediaDevice::dumpLocal(MediaEntity *e, MediaPad *p, std::ostream &os)\n>\n> s/p/pad/\n>\n> Unless names get really long, let's try to spell them out, as it increases\n> readability.\n>\n> > +{\n> > +\tos << \"\\t \\\"\" << e->name() << \"\\\"[\"\n> > +\t << p->index() << \"]\";\n> > +}\n> > +\n> > +void MediaDevice::dumpRemote(MediaLink *l, std::ostream &os)\n>\n> s/l/link/\n>\n> > +{\n> > +\n>\n> Stray blank line.\n>\n> > +\tMediaPad *remotePad = dynamic_cast\n> > +\t\t\t (getObject(l->sink()));\n>\n> Would it make sense for the sink() and source() methods to return pointers to\n> MediaPad instead of an id ? I would in that case store the pointers internally\n> in MediaLink, to avoid a call to getObject() every time.\n>\n\nThat calls for initializing two pointers at MediaLink creation time.\nThe same could happen for MediaPads, instead of storing their entity\nID they could store a pointer to the MediaEntity... I'll see how it\nmight look.\n\n> > +\tif (!remotePad)\n> > +\t\treturn;\n>\n> This check wouldn't be needed then.\n>\n> > +\n> > +\tMediaEntity *remoteEntity = dynamic_cast\n> > +\t\t\t\t (getObject(remotePad->entity()));\n> > +\tif (!remoteEntity)\n> > +\t\treturn;\n>\n> Same here, returning a MediaEntity pointer, and removing the check.\n>\n> > +\tos << \"\\\"\" << remoteEntity->name() << \"\\\"[\"\n> > +\t << remotePad->index() << \"]\";\n> > +}\n> > +\n> > +void MediaDevice::dumpLink(MediaLink *l, std::ostream &os)\n> > +{\n> > +\tunsigned int flags = l->flags();\n> > +\n> > +\tos << \" [\";\n> > +\tif (flags) {\n> > +\t\tos << (flags & MEDIA_LNK_FL_ENABLED ? \"ENABLED,\" : \"\")\n> > +\t\t << (flags & MEDIA_LNK_FL_IMMUTABLE ? \"IMMUTABLE\" : \"\");\n> > +\t}\n> > +\tos << \"]\\n\";\n> > +}\n> > +\n> > +/**\n> > + * \\fn MediaDevice::dumpGraph(std::ostream)\n> > + * \\brief Dump the media device topology in textual form to an output\n> > stream\n> > + * \\param os The output stream where to append the printed topology to\n>\n> s/append/write/ ?\n>\n\nAppend is more appropriate, as the stream could already have content.\n\n> > + */\n> > +void MediaDevice::dumpGraph(std::ostream &os)\n> > +{\n> > +\tos << \"\\n\" << name_ << \" - \" << path_ << \"\\n\\n\";\n> > +\n> > +\tfor (auto const &e : entities_) {\n>\n> s/auto const &e/const struct MediaEntity */\n>\n> Let's restrict the use of auto to cases where it would be impractical to write\n> the type out explicitly, as auto reduces readability and removes compile-time\n> checks.\n>\n\nAgreed\n\n> > +\t\tos << \"\\\"\" << e->name() << \"\\\"\\n\";\n> > +\n> > +\t\tfor (auto const &p : e->sinks()) {\n> > +\t\t\tos << \" [\" << p->index() << \"]\" << \": Sink\\n\";\n> > +\t\t\tfor (auto const &l : p->links()) {\n> > +\t\t\t\tdumpLocal(e, p, os);\n> > +\t\t\t\tos << \" <- \";\n> > +\t\t\t\tdumpRemote(l, os);\n> > +\t\t\t\tdumpLink(l, os);\n> > +\t\t\t}\n> > +\t\t\tos << \"\\n\";\n> > +\t\t}\n> > +\n> > +\t\tfor (auto const &p : e->sources()) {\n> > +\t\t\tos << \" [\" << p->index() << \"]\" << \": Source\\n\";\n> > +\t\t\tfor (auto const &l : p->links()) {\n> > +\t\t\t\tdumpLocal(e, p, os);\n> > +\t\t\t\tos << \" -> \";\n> > +\t\t\t\tdumpRemote(l, os);\n> > +\t\t\t\tdumpLink(l, os);\n> > +\t\t\t}\n> > +\t\t\tos << \"\\n\";\n> > +\t\t}\n> > +\t}\n> > +}\n>\n> I wonder whether dumpGraph() belongs here, or whether it should be moved to a\n> test. MediaDevice isn't exposed through the public API, how do you foresee\n> dumpGraph being used ? If it can never be called from the public API there's\n> no point in having it in the library :-)\n>\n\nYou are right. This was my validation test to make sure things where\nworking properly, so it's probably better to make a test out of this.\n\n> > +/**\n> > + * \\fn MediaDevice::setupLink(MediaPad *source, MediaPad *sink)\n>\n> you're missing const (assuming we need to list the parameters explicitly here,\n> see the comment above).\n>\n> > + * \\brief Apply \\a flags to the link between \\a source and \\a sink pads\n> > + * \\param source The source MediaPad\n> > + * \\param sink The sink MediaPad\n> > + * \\param link The MediaLink to operate on\n>\n> Why do you need to specify both source and sink, and the link ? The link\n> should identify the source and sink already. If you don't expect the caller to\n> have the link pointer you should look it up internally, otherwise you\n> shouldn't pass the source and sink. And if you expect the caller to have the\n> link pointer, maybe you could make this function a member of the MediaLink\n> class ?\n\nI'll consider making something like links->setup()\n\n>\n> > + * \\param flags Flags to be applied to the link (MEDIA_LNK_FL_*)\n> > + */\n> > +int MediaDevice::setupLink(const MediaPad *source, const MediaPad *sink,\n> > +\t\t\t MediaLink *link, unsigned int flags)\n> > +{\n> > +\tstruct media_link_desc linkDesc = { };\n> > +\n> > +\tlinkDesc.source.entity = source->entity();\n> > +\tlinkDesc.source.index = source->index();\n> > +\tlinkDesc.source.flags = MEDIA_PAD_FL_SOURCE;\n> > +\n> > +\tlinkDesc.sink.entity = sink->entity();\n> > +\tlinkDesc.sink.index = sink->index();\n> > +\tlinkDesc.sink.flags = MEDIA_PAD_FL_SINK;\n> > +\n> > +\tlinkDesc.flags = flags;\n> > +\n> > +\tif (ioctl(fd_, MEDIA_IOC_SETUP_LINK, &linkDesc)) {\n> > +\t\tLOG(Error) << \"Failed to setup link: \"\n> > +\t\t\t << strerror(errno);\n> > +\t\treturn -errno;\n> > +\t}\n> > +\n> > +\tlink->setFlags(0);\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +/**\n> > + * \\fn MediaDevice::resetLinks()\n> > + * \\brief Reset all links on the media graph\n>\n> Reset to what ? :-)\n\nOk... \"Clear the ENABLE flags\" ? \"Reset a link to the non-enabled\nstate?\"\n>\n> > + *\n> > + * Walk all registered entities, and disable all links from their\n> > + * source pads to other pads.\n>\n> Wouldn't it be more efficient to walk links instead of entities and pads ?\n>\n\nEntities have pads\npads have links\n\nI could create a global list of links in the media device, but if it's\njust for this, I don't think it's worth.\n\n> Could you expand the documentation to explain the use cases for this function\n> ? Please also document the return value (this holds true for all other\n> functions returning a value in the whole series).\n>\n\nWhat do you mean by use cases? Before operating on a media graph, or\nwhenever the pipeline handlers wants to reset enabled but not\nimmutable links to the non-enabled state, they have a utlity function\nto do that. Where and how to use it it's up to the pipeline handler,\neven if it were for me, that should happen before doing any operation\non the media graph.\n\nAh, you possibly meant what I just wrote? Damn documentation...\n\n> > + */\n> > +int MediaDevice::resetLinks()\n> > +{\n> > +\tfor (MediaEntity *e : entities_) {\n> > +\t\tfor (MediaPad *sourcePad : e->sources()) {\n> > +\t\t\tfor (MediaLink *l : sourcePad->links()) {\n> > +\t\t\t\t/*\n> > +\t\t\t\t * Do not reset links that are not enabled\n> > +\t\t\t\t * or immutable.\n> > +\t\t\t\t */\n> > +\t\t\t\tif (l->flags() & MEDIA_LNK_FL_IMMUTABLE)\n> > +\t\t\t\t\tcontinue;\n> > +\n> > +\t\t\t\tif (!(l->flags() & MEDIA_LNK_FL_ENABLED))\n> > +\t\t\t\t\tcontinue;\n> > +\n> > +\t\t\t\t/* Get the remote sink pad. */\n> > +\t\t\t\tMediaPad *sinkPad = dynamic_cast\n> > +\t\t\t\t\t\t (getObject(l->sink()));\n> > +\t\t\t\tif (!sinkPad)\n> > +\t\t\t\t\treturn -ENOENT;\n> > +\n> > +\t\t\t\t/* Also get entity to make sure IDs are ok. */\n> > +\t\t\t\tMediaEntity *sinkEntity =\n> > +\t\t\t\t\t\tdynamic_cast\n> > +\t\t\t\t\t\t(getObject(sinkPad->entity()));\n> > +\t\t\t\tif (!sinkEntity)\n> > +\t\t\t\t\treturn -ENOENT;\n> > +\n> > +\t\t\t\tint ret = setupLink(sourcePad, sinkPad, l, 0);\n> > +\t\t\t\tif (ret) {\n> > +\t\t\t\t\tLOG(Error) << \"Link reset failed: \"\n> > +\t\t\t\t\t\t << e->name() << \"[\"\n> > +\t\t\t\t\t\t << sourcePad->index()\n> > +\t\t\t\t\t\t << \"] -> \"\n> > +\t\t\t\t\t\t << sinkEntity->name() << \"[\"\n> > +\t\t\t\t\t\t << sinkPad->index() << \"]\";\n> > +\t\t\t\t\treturn ret;\n> > +\t\t\t\t}\n> > +\n> > +\t\t\t\tLOG(Info) << \"Link reset: \"\n> > +\t\t\t\t\t << e->name() << \"[\"\n> > +\t\t\t\t\t << sourcePad->index()\n> > +\t\t\t\t\t << \"] -> \"\n> > +\t\t\t\t\t << sinkEntity->name() << \"[\"\n> > +\t\t\t\t\t << sinkPad->index() << \"]\";\n> > +\t\t\t}\n> > +\t\t}\n> > +\t}\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +/**\n> > + * \\fn MediaDevice::link(std::string, unsigned int, std::string, unsigned\n> > int)\n> > + * \\brief Setup a link identified by the entities name and their source and\n> > + * sink pad indexes\n> > + * \\param source The source entity name\n> > + * \\param sourceIdx The source pad index\n> > + * \\param sink The sink entity name\n> > + * \\param sinkIdx The sink pad index\n> > + * \\param flags The link setup flag (see MEDIA_LNK_FL_*)\n>\n> If I understand the purpose of this function properly (and the fact that I'm\n> not sure I do means the documentation should be improved :-)), it serves the\n> same purpose as MediaDevice::setupLink(), but with different arguments. I\n> would thus name the two functions setupLink(), as C++ allows overloading\n> functions.\n\nNo, they're different. setupLink() wants pads and a link where to\noperate on, as is private, designed to back up this function which is\ninstead public and wants entities names and pad ids. I agree I should\nlook if I could make setupLink a link operation and how ita would look\nlike.\n\n>\n> I would go one step further though, at least if you agree with my proposition\n> to move setupLink to MediaLink, and turn this function into a link lookup. The\n> callers would then do something similar to\n>\n> \tMediaDevice *dev = ...;\n> \tMediaLink *link = dev->link(\"source\", 0, \"sink\", 3);\n ^- why three?\n\nCan links be set as immutable by userspace, or do they get created\nimmutable from the driver only?\n\n> \tif (!link)\n> \t\treturn -ENODEV;\n> \tlink->setup(flags);\n>\n> and possibly cache the link pointer internally to avoid looking up links all\n> the time.\n\nHere I don't agree actually. I think there should be a MediaDevice\nprovided \"link()\" operation that might use the MediaLink \"setupLink\"\nbut from the caller, nothing should be seen that it's not the\nMediaDevice itself. From the caller I would just like to see\n\n ret = mediaDev->link(\"source\", 0, \"sink\", 1, 1);\n\nThere is no reason (as I immagined this) for the caller to deal with\nlinks, and pads itself. Entities are an exception maybe, as we might\nneed to configure format and controls on their subdevices, but that's\nit.\n\nDon't you think this provides a better isolation?\n\n>\n> > + */\n> > +int MediaDevice::link(const std::string &source, unsigned int sourceIdx,\n> > +\t\t const std::string &sink, unsigned int sinkIdx,\n>\n> At the cost of longer names, sourcePadIndex and sinkPadIndex ?\n>\n> > +\t\t unsigned int flags)\n> > +{\n> > +\n>\n> Stray blank line.\n>\n> > +\t/* Make sure the supplied link is well formed. */\n> > +\tMediaEntity *sourceEntity = getEntityByName(source);\n> > +\tif (!sourceEntity) {\n> > +\t\tLOG(Error) << \"Entity name: \" << source << \"not found\";\n> > +\t\treturn -ENOENT;\n> > +\t}\n> > +\n> > +\tMediaEntity *sinkEntity = getEntityByName(sink);\n> > +\tif (!sinkEntity) {\n> > +\t\tLOG(Error) << \"Entity name: \" << source << \"not found\";\n>\n> s/source/sink/\n> s/Entity name:/Entity/\n>\n> > +\t\treturn -ENOENT;\n> > +\t}\n> > +\n> > +\tconst MediaPad *sourcePad = sourceEntity->getPadByIndex(sourceIdx);\n> > +\tif (!sourcePad) {\n> > +\t\tLOG(Error) << \"Pad \" << sourceIdx << \"not found in entity \"\n> > +\t\t\t << sourceEntity->name();\n> > +\t\treturn -ENOENT;\n> > +\t}\n> > +\n> > +\tconst MediaPad *sinkPad = sinkEntity->getPadByIndex(sinkIdx);\n> > +\tif (!sinkPad) {\n> > +\t\tLOG(Error) << \"Pad \" << sinkIdx << \"not found in entity \"\n> > +\t\t\t << sinkEntity->name();\n> > +\t\treturn -ENOENT;\n> > +\t}\n> > +\n> > +\t/*\n> > +\t * Walk all links in the source and search for an entry matching the\n> > +\t * pad ids. If none, the requested link does not exists.\n> > +\t */\n> > +\tMediaLink *validLink = nullptr;\n> > +\tfor (MediaLink *link : sourcePad->links()) {\n> > +\t\tif (link->source() != sourcePad->id())\n> > +\t\t\tcontinue;\n> > +\n> > +\t\tif (link->sink() != sinkPad->id())\n> > +\t\t\tcontinue;\n> > +\n> > +\t\tvalidLink = link;\n> > +\t\tbreak;\n> > +\t}\n> > +\n> > +\tif (!validLink) {\n> > +\t\tLOG(Error) << \"Link not found\"\n> > +\t\t\t << \"\\\"\" << sourceEntity->name() << \"\\\"[\"\n> > +\t\t\t << sourcePad->index() << \"] -> \"\n> > +\t\t\t << \"\\\"\" << sinkEntity->name() << \"\\\"[\"\n> > +\t\t\t << sinkPad->index() << \"]\";\n> > +\t\treturn -EINVAL;\n> > +\t}\n> > +\n> > +\tint ret = setupLink(sourcePad, sinkPad, validLink, flags);\n> > +\tif (ret)\n> > +\t\treturn ret;\n> > +\n> > +\tLOG(Info) << \"Setup link: \"\n>\n> LOG(Debug) at most.\n>\n> > +\t\t << \"\\\"\" << sourceEntity->name() << \"\\\"[\"\n> > +\t\t << sourcePad->index() << \"] -> \"\n> > +\t\t << \"\\\"\" << sinkEntity->name() << \"\\\"[\"\n> > +\t\t << sinkPad->index() << \"]\"\n> > +\t\t << \" [\" << flags << \"]\";\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> > index da06eba..4cac687 100644\n> > --- a/src/libcamera/meson.build\n> > +++ b/src/libcamera/meson.build\n> > @@ -2,6 +2,7 @@ libcamera_sources = files([\n> > 'log.cpp',\n> > 'main.cpp',\n> > 'media_object.cpp',\n> > + 'media_device.cpp',\n>\n> sort\n>\n> > ])\n> >\n> > libcamera_headers = files([\n\n\nThanks\n j\n\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\n>\n>\n>","headers":{"Return-Path":"","Received":["from relay8-d.mail.gandi.net (relay8-d.mail.gandi.net\n\t[217.70.183.201])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 30A2060B2E\n\tfor ;\n\tFri, 28 Dec 2018 15:36:52 +0100 (CET)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay8-d.mail.gandi.net (Postfix) with ESMTPSA id 8D40F1BF208;\n\tFri, 28 Dec 2018 14:36:51 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Fri, 28 Dec 2018 15:36:52 +0100","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20181228143652.GH909@uno.localdomain>","References":"<20181228075743.28637-1-jacopo@jmondi.org>\n\t<20181228075743.28637-3-jacopo@jmondi.org>\n\t<1597883.HRDAXlUJTF@avalon>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"07FIeBX8hApXX6Bi\"","Content-Disposition":"inline","In-Reply-To":"<1597883.HRDAXlUJTF@avalon>","User-Agent":"Mutt/1.11.1 (2018-12-01)","Subject":"Re: [libcamera-devel] [PATCH RESEND v2 2/4] libcamera: Add\n\tMediaDevice class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Fri, 28 Dec 2018 14:36:52 -0000"}},{"id":99,"web_url":"https://patchwork.libcamera.org/comment/99/","msgid":"<4498295.DGJ6F74ZFY@avalon>","date":"2018-12-28T16:40:51","subject":"Re: [libcamera-devel] [PATCH RESEND v2 2/4] libcamera: Add\n\tMediaDevice class","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Friday, 28 December 2018 16:36:52 EET Jacopo Mondi wrote:\n> On Fri, Dec 28, 2018 at 03:27:21PM +0200, Laurent Pinchart wrote:\n> > On Friday, 28 December 2018 09:57:41 EET Jacopo Mondi wrote:\n> > > The MediaDevice object implements handling and configuration of the\n> > > media graph associated with a V4L2 media device.\n> > \n> > Let's not talk about V4L2 here, it's just MC, not V4L2.\n> > \n> > > The class allows enumeration of all pads, links and entities registered\n> > > in the media graph, and provides methods to setup and reset media links.\n> > > \n> > > Signed-off-by: Jacopo Mondi \n> > > ---\n> > > \n> > > src/libcamera/include/media_device.h | 71 ++++\n> > > src/libcamera/media_device.cpp | 596 +++++++++++++++++++++++++++\n> > > src/libcamera/meson.build | 1 +\n> > > 3 files changed, 668 insertions(+)\n> > > create mode 100644 src/libcamera/include/media_device.h\n> > > create mode 100644 src/libcamera/media_device.cpp\n> > > \n> > > diff --git a/src/libcamera/include/media_device.h\n> > > b/src/libcamera/include/media_device.h new file mode 100644\n> > > index 0000000..642eea9\n> > > --- /dev/null\n> > > +++ b/src/libcamera/include/media_device.h\n> > > @@ -0,0 +1,71 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2018, Google Inc.\n> > > + *\n> > > + * media_device.h - Media device handler\n> > > + */\n> > > +#ifndef __LIBCAMERA_MEDIA_DEVICE_H__\n> > > +#define __LIBCAMERA_MEDIA_DEVICE_H__\n> > > +\n> > > +#include \n> > > +#include \n> > > +#include \n> > \n> > sort\n> > \n> > > +#include \n> > > +\n> > > +#include \n> > > +\n> > > +#include \"log.h\"\n> > \n> > Is this needed ?\n> > \n> > > +#include \"media_object.h\"\n> > > +\n> > > +namespace libcamera {\n> > > +\n> > > +class MediaDevice\n> > > +{\n> > > +public:\n> > > +\tMediaDevice() : fd_(-1) { };\n> > > +\t~MediaDevice();\n> > > +\n> > > +\tconst std::string name() const { return name_; }\n> > > +\tconst std::string path() const { return path_; }\n> > > +\n> > > +\tint open(const std::string &path);\n> > > +\tint close();\n> > > +\tint enumerate(std::map &entitiesMap);\n> > > +\tvoid dumpGraph(std::ostream &os);\n> > > +\n> > > +\tint resetLinks();\n> > > +\tint link(const std::string &source, unsigned int sourceIdx,\n> > > +\t\t const std::string &sink, unsigned int sinkIdx,\n> > > +\t\t unsigned int flags);\n> > > +\n> > > +private:\n> > > +\t/** The media device file descriptor */\n> > \n> > Wrong file ?\n> \n> Sorry, I didn't get this.\n\nI meant that the doxygen comments are placed in the .cpp file.\n\n> > > +\tint fd_;\n> > > +\t/** The media device name as returned by MEDIA_IOC_DEVICE_INFO */\n> > \n> > Please take my comments on 1/4 into account regarding wording of\n> > documentation here.\n> \n> I'll drop documentation from private members\n> \n> > This field stores the driver name, I would thus name it driver_ (or\n> > possibly driverName_).\n> > \n> > > +\tstd::string name_;\n> > > +\t/** The media device path */\n> > > +\tstd::string path_;\n> > > +\n> > > +\tstd::map mediaObjects_;\n> > \n> > s/mediaObjects_/objects_/ ?\n> \n> Actually I don't agree, here and all other places in the patch where\n> an s/mediaSomething/something/ has been suggested.\n> \n> This class deals both with struct_v2_something and MediaSomething. I\n> would like to keep clear when we're operating on the object\n> representation and when on the kernel provided structure.\n\nI understand, and I agree that there's a naming issue when both types of \nobjects are needed. Given that the kernel structures should all be \nencapsulated here and the Media* objects used everywhere else, I would go for \nshorter names for Media* (e.g. MediaPad *pad) and longer names for the kernel \nstructures.\n\n> > > +\tMediaObject *getObject(unsigned int id);\n> > > +\tvoid addObject(MediaObject *obj);\n> > > +\tvoid deleteObjects();\n> > > +\n> > > +\tstd::vector entities_;\n> > > +\tMediaEntity *getEntityByName(const std::string &name);\n> > > +\n> > > +\tint enumerateEntities(std::map \n&entitiesMap,\n> > > +\t\t\t struct media_v2_topology &topology);\n> > > +\tint enumeratePads(struct media_v2_topology &topology);\n> > > +\tint enumerateLinks(struct media_v2_topology &topology);\n> > > +\n> > > +\tint setupLink(const MediaPad *source, const MediaPad *sink,\n> > > +\t\t MediaLink *link, unsigned int flags);\n> > > +\n> > > +\tvoid dumpLocal(MediaEntity *e, MediaPad *p, std::ostream &os);\n> > > +\tvoid dumpRemote(MediaLink *l, std::ostream &os);\n> > > +\tvoid dumpLink(MediaLink *l, std::ostream &os);\n> > > +};\n> > > +\n> > > +} /* namespace libcamera */\n> > > +#endif /* __LIBCAMERA_MEDIA_DEVICE_H__ */\n> > > diff --git a/src/libcamera/media_device.cpp\n> > > b/src/libcamera/media_device.cpp new file mode 100644\n> > > index 0000000..b98d62f\n> > > --- /dev/null\n> > > +++ b/src/libcamera/media_device.cpp\n> > > @@ -0,0 +1,596 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2018, Google Inc.\n> > > + *\n> > > + * media_device.cpp - Media device handler\n> > > + */\n> > > +\n> > > +#include \n> > > +#include \n> > > +#include \n> > > +#include \n> > > +#include \n> > > +\n> > > +#include \n> > > +#include \n> > > +\n> > > +#include \n> > > +\n> > > +#include \"log.h\"\n> > > +#include \"media_device.h\"\n> > > +\n> > > +/**\n> > > + * \\file media_device.h\n> > > + */\n> > > +namespace libcamera {\n> > > +\n> > > +/**\n> > > + * \\class MediaDevice\n> > > + * \\brief Media device handler\n> > > + *\n> > > + * MediaDevice handles the media graph associated with a V4L2 media\n> > > device. + */\n> > > +\n> > > +/**\n> > > + * \\fn MediaDevice::~MediaDevice()\n> > > + * \\brief Close the media device file descriptor and release entities\n> > > + */\n> > > +MediaDevice::~MediaDevice()\n> > > +{\n> > > +\tif (fd_ > -1)\n> > \n> > I would have written != -1.\n> \n> Any reason why it's better?\n\nProbably not :-) We should never have negative values other than -1.\n\n> > > +\t\t::close(fd_);\n> > > +\tdeleteObjects();\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\fn MediaDevice::name()\n> > > + * \\brief Return the media device name\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn MediaDevice::path()\n> > > + * \\brief Return the media device path node associated with this\n> > > MediaDevice\n> > \n> > I'd name this devnode() instead of path().\n> \n> I already though about chnging this, right...\n> \n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn MediaDevice::deleteObjects()\n> > > + * \\brief Delete all registered entities in the MediaDevice object\n> > > + */\n> > > +void MediaDevice::deleteObjects()\n> > > +{\n> > > +\tfor (auto const &e : mediaObjects_)\n> > > +\t\tdelete e.second;\n> > > +\n> > > +\tmediaObjects_.clear();\n> > > +\tentities_.clear();\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\fn int MediaDevice::open(std::string)\n> > \n> > Do you need to specify the arguments when there's a single function by\n> > that name ? If so, shouldn't it be (const std::string &) ?\n> > \n> > Same comment for other functions in this patch series.\n> \n> I think I could drop the function name completely when the comment\n> block is just before the function definition.\n> \n> > > + * \\brief Open a media device and initialize its components.\n> > > + * \\param path The media device path\n> > > + */\n> > > +int MediaDevice::open(const std::string &path)\n> > \n> > s/path/devnode/ ?\n> > \n> > > +{\n> > \n> > What if the device is already open ? It would be useful to add some\n> > documentation to explain the \"life cycle\" of the object, as in how it is\n> > supposed to be used (and what happens when it's incorrectly used). This\n> > can be done in the open() documentation (and referenced from close() and\n> > possibly other functions), or in the \\class documentation.\n> \n> Right, currently there's no protection for that in this\n> implementation. I should return an error if (fd != -1).\n> \n> I could just state the caller is responsible for opening and closing\n> the device and that access is exclusive. In the future, this might be\n> superseded by ref-counted acquire/release operations, with open() and\n> close() then made private.\n\nAs long as you document the expected behaviour, that's fine with me.\n\n> > > +\tfd_ = ::open(path.c_str(), O_RDWR);\n> > > +\tif (fd_ < 0) {\n> > > +\t\tLOG(Error) << \"Failed to open media device at \" << path\n> > > +\t\t\t << \": \" << strerror(errno);\n> > > +\t\treturn -errno;\n> > \n> > The LOG() call may overwrite errno. You will need a local variable. How\n> > about> \n> > \tint ret;\n> > \t\n> > \tret = ::open(path.c_str(), O_RDWR);\n> > \tif (ret < 0) {\n> > \t\tret = -errno;\n> > \t\tLOG(Error) << \"Failed to open media device at \" << path\n> > \t\t\t << \": \" << strerror(-ret);\n> > \t\treturn ret;\n> > \t}\n> > \t\n> > \tfd_ = ret;\n> > \tpath_ = path;\n> \n> Right. Thanks, this is important, I would like to see all error\n> handling standardized in the library.\n\nSo would I. I think the above construct could be used through the library.\n\n> > > +\t}\n> > > +\tpath_ = path;\n> > > +\n> > > +\tstruct media_device_info info = { };\n> > > +\tint ret = ioctl(fd_, MEDIA_IOC_DEVICE_INFO, &info);\n> > > +\tif (ret) {\n> > > +\t\tLOG(Error) << \"Failed to get media device info \"\n> > > +\t\t\t << \": \" << strerror(errno);\n> > > +\t\treturn -errno;\n> > > +\t}\n> > > +\n> > > +\tname_ = info.driver;\n> > > +\n> > > +\treturn 0;\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\fn MediaDevice::close()\n> > > + * \\brief Close the file descriptor associated with the media device\n> > > + */\n> > > +int MediaDevice::close()\n> > > +{\n> > > +\tif (fd_ > -1)\n> > \n> > I'd write this != -1 too.\n> > \n> > > +\t\treturn ::close(fd_);\n> > > +\n> > > +\treturn 0;\n> > \n> > Do you use the return value ?\n> > \n> > > +}\n> > > +\n> > > +void MediaDevice::addObject(MediaObject *obj)\n> > > +{\n> > > +\n> > > +\tif (mediaObjects_.find(obj->id()) != mediaObjects_.end()) {\n> > > +\t\tLOG(Error) << \"Element with id \" << obj->id()\n> > > +\t\t\t << \" already enumerated.\";\n> > > +\t\treturn;\n> > \n> > Shouldn't you propagate the error to the caller ? This seems pretty fatal\n> > to me.\n> \n> This should not happen of course. In case it does, I'll return\n> -EEXIST.\n> \n> > > +\t}\n> > > +\n> > > +\tmediaObjects_[obj->id()] = obj;\n> > > +}\n> > > +\n> > > +MediaObject *MediaDevice::getObject(unsigned int id)\n> > > +{\n> > > +\tauto it = mediaObjects_.find(id);\n> > > +\treturn (it == mediaObjects_.end()) ?\n> > > +\t nullptr : it->second;\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\fn MediaDevice::getEntityByName(std::string)\n> > > + * \\brief Return entity with name \\a name\n> > > + * \\param name The entity name\n> > \n> > Please expand documentation a little bit. For instance you should explain\n> > that this function returns nullptr if the entity can't be found. In\n> > general a one- line documentation that just duplicates the function name\n> > isn't very useful, the real value comes from what isn't expressed in the\n> > name.\n> \n> I still feel that a function named getEntityByName() is pretty clear\n> without having to document it in detail. The return value though might\n> be pointed out, even if, it's exactly what one would expect, the\n> entity or nullptr.\n> \n> > Documentation is hard :-)\n> \n> Drawing the line between useful documentation and writing comments to\n> silence doxygen \"not documented\" warning is. I clearly went for the second\n> option and documented everything doxygen complained about but that's\n> clearly not useful if not for ticking a box. I would be happy to chop\n> documentation parts here and there.\n\nTODO: Check whether we could silence warnings in a better way when no \ndocumentation is needed.\n\n> > > + */\n> > > +MediaEntity *MediaDevice::getEntityByName(const std::string &name)\n> > > +{\n> > > +\tauto it = entities_.begin();\n> > > +\n> > > +\twhile (it != entities_.end()) {\n> > > +\t\tMediaEntity *e = *it;\n> > > +\t\tif (!(e->name().compare(name)))\n> > > +\t\t\treturn e;\n> > > +\t\tit++;\n> > > +\t}\n> > > +\n> > > +\treturn nullptr;\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\fn MediaDevice::enumerateLinks(struct media_v2_topology &topology)\n> > > + * \\brief Enumerate all links in the system and associate them with\n> > > their\n> > > + * source and sink pads\n> > > + * \\param topology The media topology as returned by\n> > > MEDIA_IOC_G_TOPOLOGY\n> > > + */\n> > > +int MediaDevice::enumerateLinks(struct media_v2_topology &topology)\n> > \n> > Shouldn't the argument be const ? I would have used a pointer instead of a\n> > reference, but I'm not sure why.\n> > \n> > > +{\n> > > +\tstruct media_v2_link *link = reinterpret_cast\n> > > +\t\t\t\t (topology.ptr_links);\n> > > +\n> > > +\tfor (unsigned int i = 0; i < topology.num_links; i++, link++) {\n> > > +\t\t/*\n> > > +\t\t * Skip links between entities and interfaces: we only care\n> > > +\t\t * about pad-2-pad links here.\n> > > +\t\t */\n> > > +\t\tif ((link->flags & MEDIA_LNK_FL_LINK_TYPE) ==\n> > > +\t\t MEDIA_LNK_FL_INTERFACE_LINK)\n> > > +\t\t\tcontinue;\n> > > +\n> > > +\t\tMediaLink *mediaLink = new MediaLink(link);\n> > \n> > s/mediaLink/link/ ?\n> > \n> > You would need to rename the link variable above though, I'm not sure\n> > which one is best.\n> \n> That's what I meant. link for media_v2_link, and mediaLink for\n> MediaLink. It makes sense to me.\n\nI would have gone for the opposite, as explained above, but that's OK too I \nsuppose. We should however go for shorter names (no media prefix) in the API \nand in the code using those classes. The media prefix, if used for the Media* \nclasses, should only appear in the media_object.cpp and media_device.cpp \nfiles.\n\n> > > +\t\taddObject(mediaLink);\n> > > +\n> > > +\t\t/* Store reference to this mediaLink in the link's source pad. */\n> > > +\t\tMediaPad *mediaPad = dynamic_cast\n> > > +\t\t\t\t (getObject(mediaLink->source()));\n> > \n> > s/mediaPad/pad/ ?\n> \n> ditto\n> \n> > > +\t\tif (!mediaPad) {\n> > > +\t\t\tLOG(Error) << \"Failed to find pad with id: \"\n> > \n> > Nitpicking, s/://.\n> > \n> > > +\t\t\t\t << mediaLink->source();\n> > > +\t\t\treturn -ENODEV;\n> > > +\t\t}\n> > > +\t\tmediaPad->addLink(mediaLink);\n> > > +\n> > > +\t\t/* Store reference to this mediaLink in the link's sink pad. */\n> > > +\t\tmediaPad = dynamic_cast(getObject(mediaLink-\n>sink()));\n> > > +\t\tif (!mediaPad) {\n> > > +\t\t\tLOG(Error) << \"Failed to find pad with id: \"\n> > > +\t\t\t\t << mediaLink->sink();\n> > > +\t\t\treturn -ENODEV;\n> > > +\t\t}\n> > > +\t\tmediaPad->addLink(mediaLink);\n> > > +\t}\n> > > +\n> > > +\treturn 0;\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\fn MediaDevice::enumeratePads(struct media_v2_topology &topology)\n> > > + * \\brief Enumerate all pads in the system and associate them with the\n> > > + * entity they belong to\n> > > + * \\param topology The media topology as returned by\n> > > MEDIA_IOC_G_TOPOLOGY\n> > > + */\n> > > +int MediaDevice::enumeratePads(struct media_v2_topology &topology)\n> > > +{\n> > > +\tstruct media_v2_pad *pad = reinterpret_cast\n> > > +\t\t\t\t (topology.ptr_pads);\n> > > +\n> > > +\tfor (unsigned int i = 0; i < topology.num_pads; i++, pad++) {\n> > > +\t\tMediaPad *mediaPad = new MediaPad(pad);\n> > > +\t\taddObject(mediaPad);\n> > > +\n> > > +\t\t/* Store a reference to this MediaPad in pad's entity. */\n> > > +\t\tMediaEntity *mediaEntity = dynamic_cast\n> > > +\t\t\t\t\t (getObject(mediaPad->entity()));\n> > \n> > s/mediaPad/pad/ and s/mediaEntity/entity/ ?\n> \n> ditto ditto ;)\n> \n> > > +\t\tif (!mediaEntity) {\n> > > +\t\t\tLOG(Error) << \"Failed to find entity with id: \"\n> > > +\t\t\t\t << mediaPad->entity();\n> > > +\t\t\treturn -ENODEV;\n> > > +\t\t}\n> > > +\n> > > +\t\tmediaEntity->addPad(mediaPad);\n> > > +\t}\n> > > +\n> > > +\treturn 0;\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\fn MediaDevice::enumerateEntities(std::map > > std::string> &, + * struct\n> > > media_v2_topology &topology) + * \\brief Enumerate and initialize\n> > > entities in the media graph\n> > > + * \\param entitiesMap Map entities names to their video (sub)device\n> > > node\n> > > + * \\param topology The media topology as returned by\n> > > MEDIA_IOC_G_TOPOLOGY\n> > > + *\n> > > + * Associate the video (sub)device path to the entity name as returned\n> > > by\n> > \n> > As stated before, I'd talk about device nodes, as this isn't\n> > V4L2-specific.\n> > \n> > > + * MEDIA_IOC_G_TOPOLOGY\n> > > + */\n> > > +int MediaDevice::enumerateEntities(std::map\n> > > &entitiesMap,\n> > > +\t\t\t\t struct media_v2_topology &topology)\n> > > +{\n> > > +\tstruct media_v2_entity *entities = reinterpret_cast > > media_v2_entity *>\n> > > +\t\t\t\t\t (topology.ptr_entities);\n> > > +\n> > > +\tfor (unsigned int i = 0; i < topology.num_entities; ++i) {\n> > > +\t\tauto it = entitiesMap.find(entities[i].name);\n> > > +\t\tif (it == entitiesMap.end()) {\n> > > +\t\t\tLOG(Error) << \"Entity \" << entities[i].name\n> > > +\t\t\t\t << \" not found in media entities map\";\n> > > +\t\t\treturn -ENOENT;\n> > > +\t\t}\n> > > +\n> > > +\t\tMediaEntity *entity = new MediaEntity(&entities[i]);\n> > > +\t\tif (entity->setDevice(it->second)) {\n> > > +\t\t\tdelete entity;\n> > > +\t\t\tgoto delete_entities;\n> > \n> > You could call deleteObjects() and return here directly.\n> > \n> > > +\t\t}\n> > > +\n> > > +\t\taddObject(entity);\n> > > +\t\tentities_.push_back(entity);\n> > > +\t}\n> > > +\n> > > +\treturn 0;\n> > > +\n> > > +delete_entities:\n> > > +\tdeleteObjects();\n> > > +\n> > > +\treturn -errno;\n> > \n> > -errno isn't correct. You should assign the return value of setDevice() to\n> > a ret variable and return ret.\n> > \n> > > +}\n> > > +\n> > > +/**\n> > > + * \\fn MediaDevice::enumerate(std::map)\n> > > + * \\brief Enumerate the media graph topology\n> > > + * \\param entitiesMap Map entities names to their video (sub)device\n> > > node\n> > > + * FIXME: this is statically provided by the caller at the moment.\n> > > + *\n> > > + * This functions enumerates all media objects, registered in the media\n> > > graph,\n> > \n> > s/functions/function/\n> > s/,//g\n> > \n> > > + * through the MEDIA_IOC_G_TOPOLOGY ioctl. For each returned entity,\n> > > + * it creates and store its representation for later reuse.\n> > \n> > Let's expand this a little.\n> > \n> > \"This function enumerates all media graph objects in the media device and\n> > populates the internal list of objects. All entities, pads and links are\n> > stored as MediaEntity, MediaPad and MediaLink respectively, with cross-\n> > references between objects. Media interfaces are not processed.\n> > \n> > For entities paired with a device node, the path to the device node is\n> > stored in the MediaEntity object, based on the associations set in the \\a\n> > entitiesMap parameter.\n> > \n> > \\return Return 0 on success or a negative error code on error.\"\n> > \n> > > + */\n> > > +int MediaDevice::enumerate(std::map\n> > > &entitiesMap) +{\n> > > +\tstruct media_v2_topology topology = { };\n> > > +\tunsigned int num_interfaces;\n> > > +\tunsigned int num_links;\n> > > +\tunsigned int num_pads;\n> > > +\tunsigned int num_ent;\n> > > +\n> > > +\tdo {\n> > > +\t\tnum_ent = topology.num_entities;\n> > > +\t\tnum_pads = topology.num_pads;\n> > > +\t\tnum_links = topology.num_links;\n> > > +\t\tnum_interfaces = topology.num_interfaces;\n> > > +\n> > > +\t\t/* Call G_TOPOLOGY the first time here to enumerate .*/\n> > > +\t\tif (ioctl(fd_, MEDIA_IOC_G_TOPOLOGY, &topology)) {\n> > > +\t\t\tLOG(Error) << \"Failed to enumerate media topology on\"\n> > > +\t\t\t\t << path_ << \": \" << strerror(errno);\n> > > +\t\t\treturn -errno;\n> > > +\t\t}\n> > > +\n> > > +\t\t/*\n> > > +\t\t * Repeat the call until we don't get a 'stable' number\n> > > +\t\t * of media objects.\n> > > +\t\t */\n> > > +\t} while (num_ent != topology.num_entities ||\n> > > +\t\t num_pads != topology.num_pads ||\n> > > +\t\t num_links != topology.num_links ||\n> > > +\t\t num_interfaces != topology.num_interfaces);\n> > \n> > That's not very useful, as the need to ensure that the topology doesn't\n> > change stems from the fact we call MEDIA_IOC_G_TOPOLOGY (at least) twice,\n> > once to retrieve the number of entities, and a second time to retrieve\n> > the graph itself. The graph could be updated in the kernel after this\n> > loop and before the next MEDIA_IOC_G_TOPOLOGY.\n> > \n> > I proposed a way to call MEDIA_IOC_G_TOPOLOGY from a single location in a\n> > previous e-mail. We can fix this on top of this series, or:\n> > \n> > \tstruct media_v2_topology topology = { };\n> > \tstruct media_v2_entity *entities;\n> > \tstruct media_v2_pad *pads;\n> > \tstruct media_v2_link *links;\n> > \tstruct media_v2_interface *interfaces;\n> > \tunsigned int num_entities;\n> > \tunsigned int num_pads;\n> > \tunsigned int num_links;\n> > \tunsigned int num_interfaces;\n> > \tint ret;\n> > \t\n> > \twhile (1) {\n> > \t\tnum_entitites = topology.num_entities;\n> > \t\tnum_pads = topology.num_pads;\n> > \t\tnum_links = topology.num_links;\n> > \t\tnum_interfaces = topology.num_interfaces;\n> > \t\t\n> > \t\tpads = new media_v2_pad[topology.num_pads];\n> > \t\tentities = new media_v2_entity[topology.num_entities];\n> > \t\ttopology.ptr_entities = reinterpret_cast<__u64>(entities);\n> > \t\t\n> > \t\ttopology.ptr_pads = reinterpret_cast<__u64>(pads);\n> > \t\t\n> > \t\tlinks = new media_v2_link[topology.num_links];\n> > \t\ttopology.ptr_links = reinterpret_cast<__u64>(links);\n> > \t\tlinks = new media_v2_interface[topology.num_interfaces];\n> > \t\ttopology.ptr_interfaces = reinterpret_cast<__u64>(interfaces);\n> > \t\t\n> > \t\tret = ioctl(fd_, MEDIA_IOC_G_TOPOLOGY, &topology);\n> > \t\tif (ret < 0) {\n> > \t\t\tret = -errno;\n> > \t\t\tLOG(Error) << \"Failed to enumerate media topology on\"\n> > \t\t\t\t << path_ << \": \" << strerror(-ret);\n> > \t\t\tgoto done;\n> > \t\t}\n> > \t\t\n> > \t\t/*\n> > \t\t * If all objects have been enumerated, stop now. If the number\n> > \t\t * of objects has increased (after the first call or due to a\n> > \t\t * topology change, retry the enumeration.\n> > \t\t */\n> > \t\tif (num_entities >= topology.num_entities &&\n> > \t\t num_pads >= topology.num_pads &&\n> > \t\t num_links >= topology.num_links &&\n> > \t\t num_interfaces >= topology.num_interfaces)\n> > \t\t\tbreak;\n> > \t}\n> \n> According to documentation (and Niklas' implementation I have copied,\n> checking that topology->version does not change should be enough. Do\n> you agree?\n\nYes, that would work too. My implementation avoids an extra iteration in case \nthe topology changes but the number of objects doesn't increase, but that's \nreally a micro-optimization that will likely never matter in practice.\n\n> By the way, I appreciate the concern regarding dynamic media graphs,\n> but we're here checking that media graph does not change in between\n> two ioctl calls, which is quite a strict race window, while it could\n> change at any other point in time after this call, and we won't\n> notice. I wonder if this is worth, but since I have code from you a\n> Niklas, I'll simply copy it in.\n\nYou're right that other changes will be needed, but as you've pointed out, the \ncode exists, so let's use it.\n\n> > \tret = enumerateEntities(entitiesMap, topology);\n> > \tif (ret)\n> > \t\tgoto done;\n> > \t\n> > \tret = enumeratePads(topology);\n> > \tif (ret)\n> > \t\tgoto done;\n> > \t\n> > \tret = enumerateLinks(topology);\n> > \tif (ret)\n> > \t\tgoto done;\n> > \n> > done:\n> > \tif (ret < 0)\n> > \t\tdeleteObjects();\n> > \t\n> > \tdelete[] entities;\n> > \tdelete[] links;\n> > \tdelete[] pads;\n> > \tdelete[] interfaces;\n> > \t\n> > \treturn ret;\n> > \n> > (untested)\n> > \n> > I wonder if enumerateEntities, enumeratePads and enumerateLinks should be\n> > renamed as s/enumerate/populate/.\n> > \n> > > +\tauto *_ptr_e = new media_v2_entity[topology.num_entities];\n> > > +\ttopology.ptr_entities = reinterpret_cast<__u64>(_ptr_e);\n> > > +\n> > > +\tauto *_ptr_p = new media_v2_pad[topology.num_pads];\n> > > +\ttopology.ptr_pads = reinterpret_cast<__u64>(_ptr_p);\n> > > +\n> > > +\tauto *_ptr_l = new media_v2_link[topology.num_links];\n> > > +\ttopology.ptr_links = reinterpret_cast<__u64>(_ptr_l);\n> > > +\n> > > +\t/* Call G_TOPOLOGY again, this time with memory reserved. */\n> > > +\tint ret = ioctl(fd_, MEDIA_IOC_G_TOPOLOGY, &topology);\n> > > +\tif (ret < 0) {\n> > > +\t\tLOG(Error) << \"Failed to enumerate media topology on \" << path_\n> > > +\t\t\t << \": \" << strerror(errno);\n> > > +\t\tret = -errno;\n> > > +\t\tgoto error_free_mem;\n> > > +\t}\n> > > +\n> > > +\tret = enumerateEntities(entitiesMap, topology);\n> > > +\tif (ret)\n> > > +\t\tgoto error_free_mem;\n> > > +\n> > > +\tret = enumeratePads(topology);\n> > > +\tif (ret)\n> > > +\t\tgoto error_free_objs;\n> > > +\n> > > +\tret = enumerateLinks(topology);\n> > > +\tif (ret)\n> > > +\t\tgoto error_free_objs;\n> > > +\n> > > +\tdelete[] _ptr_e;\n> > > +\tdelete[] _ptr_p;\n> > > +\tdelete[] _ptr_l;\n> > > +\n> > > +\treturn 0;\n> > > +\n> > > +error_free_objs:\n> > > +\tdeleteObjects();\n> > > +\n> > > +error_free_mem:\n> > > +\tdelete[] _ptr_e;\n> > > +\tdelete[] _ptr_p;\n> > > +\tdelete[] _ptr_l;\n> > > +\n> > > +\treturn ret;\n> > > +}\n> > > +\n> > > +void MediaDevice::dumpLocal(MediaEntity *e, MediaPad *p, std::ostream\n> > > &os)\n> > \n> > s/p/pad/\n> > \n> > Unless names get really long, let's try to spell them out, as it increases\n> > readability.\n> > \n> > > +{\n> > > +\tos << \"\\t \\\"\" << e->name() << \"\\\"[\"\n> > > +\t << p->index() << \"]\";\n> > > +}\n> > > +\n> > > +void MediaDevice::dumpRemote(MediaLink *l, std::ostream &os)\n> > \n> > s/l/link/\n> > \n> > > +{\n> > > +\n> > \n> > Stray blank line.\n> > \n> > > +\tMediaPad *remotePad = dynamic_cast\n> > > +\t\t\t (getObject(l->sink()));\n> > \n> > Would it make sense for the sink() and source() methods to return pointers\n> > to MediaPad instead of an id ? I would in that case store the pointers\n> > internally in MediaLink, to avoid a call to getObject() every time.\n> \n> That calls for initializing two pointers at MediaLink creation time.\n> The same could happen for MediaPads, instead of storing their entity\n> ID they could store a pointer to the MediaEntity... I'll see how it\n> might look.\n\nInitialization will be a bit more complex, but usage should be simpler.\n\n> > > +\tif (!remotePad)\n> > > +\t\treturn;\n> > \n> > This check wouldn't be needed then.\n> > \n> > > +\n> > > +\tMediaEntity *remoteEntity = dynamic_cast\n> > > +\t\t\t\t (getObject(remotePad->entity()));\n> > > +\tif (!remoteEntity)\n> > > +\t\treturn;\n> > \n> > Same here, returning a MediaEntity pointer, and removing the check.\n> > \n> > > +\tos << \"\\\"\" << remoteEntity->name() << \"\\\"[\"\n> > > +\t << remotePad->index() << \"]\";\n> > > +}\n> > > +\n> > > +void MediaDevice::dumpLink(MediaLink *l, std::ostream &os)\n> > > +{\n> > > +\tunsigned int flags = l->flags();\n> > > +\n> > > +\tos << \" [\";\n> > > +\tif (flags) {\n> > > +\t\tos << (flags & MEDIA_LNK_FL_ENABLED ? \"ENABLED,\" : \"\")\n> > > +\t\t << (flags & MEDIA_LNK_FL_IMMUTABLE ? \"IMMUTABLE\" : \"\");\n> > > +\t}\n> > > +\tos << \"]\\n\";\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\fn MediaDevice::dumpGraph(std::ostream)\n> > > + * \\brief Dump the media device topology in textual form to an output\n> > > stream\n> > > + * \\param os The output stream where to append the printed topology to\n> > \n> > s/append/write/ ?\n> \n> Append is more appropriate, as the stream could already have content.\n> \n> > > + */\n> > > +void MediaDevice::dumpGraph(std::ostream &os)\n> > > +{\n> > > +\tos << \"\\n\" << name_ << \" - \" << path_ << \"\\n\\n\";\n> > > +\n> > > +\tfor (auto const &e : entities_) {\n> > \n> > s/auto const &e/const struct MediaEntity */\n> > \n> > Let's restrict the use of auto to cases where it would be impractical to\n> > write the type out explicitly, as auto reduces readability and removes\n> > compile-time checks.\n> \n> Agreed\n> \n> > > +\t\tos << \"\\\"\" << e->name() << \"\\\"\\n\";\n> > > +\n> > > +\t\tfor (auto const &p : e->sinks()) {\n> > > +\t\t\tos << \" [\" << p->index() << \"]\" << \": Sink\\n\";\n> > > +\t\t\tfor (auto const &l : p->links()) {\n> > > +\t\t\t\tdumpLocal(e, p, os);\n> > > +\t\t\t\tos << \" <- \";\n> > > +\t\t\t\tdumpRemote(l, os);\n> > > +\t\t\t\tdumpLink(l, os);\n> > > +\t\t\t}\n> > > +\t\t\tos << \"\\n\";\n> > > +\t\t}\n> > > +\n> > > +\t\tfor (auto const &p : e->sources()) {\n> > > +\t\t\tos << \" [\" << p->index() << \"]\" << \": Source\\n\";\n> > > +\t\t\tfor (auto const &l : p->links()) {\n> > > +\t\t\t\tdumpLocal(e, p, os);\n> > > +\t\t\t\tos << \" -> \";\n> > > +\t\t\t\tdumpRemote(l, os);\n> > > +\t\t\t\tdumpLink(l, os);\n> > > +\t\t\t}\n> > > +\t\t\tos << \"\\n\";\n> > > +\t\t}\n> > > +\t}\n> > > +}\n> > \n> > I wonder whether dumpGraph() belongs here, or whether it should be moved\n> > to a test. MediaDevice isn't exposed through the public API, how do you\n> > foresee dumpGraph being used ? If it can never be called from the public\n> > API there's no point in having it in the library :-)\n> \n> You are right. This was my validation test to make sure things where\n> working properly, so it's probably better to make a test out of this.\n\nAnd it would be a test producing nicely looking results :-)\n\n> > > +/**\n> > > + * \\fn MediaDevice::setupLink(MediaPad *source, MediaPad *sink)\n> > \n> > you're missing const (assuming we need to list the parameters explicitly\n> > here, see the comment above).\n> > \n> > > + * \\brief Apply \\a flags to the link between \\a source and \\a sink pads\n> > > + * \\param source The source MediaPad\n> > > + * \\param sink The sink MediaPad\n> > > + * \\param link The MediaLink to operate on\n> > \n> > Why do you need to specify both source and sink, and the link ? The link\n> > should identify the source and sink already. If you don't expect the\n> > caller to have the link pointer you should look it up internally,\n> > otherwise you shouldn't pass the source and sink. And if you expect the\n> > caller to have the link pointer, maybe you could make this function a\n> > member of the MediaLink class ?\n> \n> I'll consider making something like links->setup()\n> \n> > > + * \\param flags Flags to be applied to the link (MEDIA_LNK_FL_*)\n> > > + */\n> > > +int MediaDevice::setupLink(const MediaPad *source, const MediaPad\n> > > *sink,\n> > > +\t\t\t MediaLink *link, unsigned int flags)\n> > > +{\n> > > +\tstruct media_link_desc linkDesc = { };\n> > > +\n> > > +\tlinkDesc.source.entity = source->entity();\n> > > +\tlinkDesc.source.index = source->index();\n> > > +\tlinkDesc.source.flags = MEDIA_PAD_FL_SOURCE;\n> > > +\n> > > +\tlinkDesc.sink.entity = sink->entity();\n> > > +\tlinkDesc.sink.index = sink->index();\n> > > +\tlinkDesc.sink.flags = MEDIA_PAD_FL_SINK;\n> > > +\n> > > +\tlinkDesc.flags = flags;\n> > > +\n> > > +\tif (ioctl(fd_, MEDIA_IOC_SETUP_LINK, &linkDesc)) {\n> > > +\t\tLOG(Error) << \"Failed to setup link: \"\n> > > +\t\t\t << strerror(errno);\n> > > +\t\treturn -errno;\n> > > +\t}\n> > > +\n> > > +\tlink->setFlags(0);\n> > > +\n> > > +\treturn 0;\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\fn MediaDevice::resetLinks()\n> > > + * \\brief Reset all links on the media graph\n> > \n> > Reset to what ? :-)\n> \n> Ok... \"Clear the ENABLE flags\" ? \"Reset a link to the non-enabled\n> state?\"\n\nMaybe \"reset all mutable links to disabled state\" ?\n\n> > > + *\n> > > + * Walk all registered entities, and disable all links from their\n> > > + * source pads to other pads.\n> > \n> > Wouldn't it be more efficient to walk links instead of entities and pads ?\n> \n> Entities have pads\n> pads have links\n> \n> I could create a global list of links in the media device, but if it's\n> just for this, I don't think it's worth.\n\nWe could walk all objects and skip non-link objects. I'll let you decide what \nis simpler. As the function resets all links I find a single loop easier.\n\n> > Could you expand the documentation to explain the use cases for this\n> > function ? Please also document the return value (this holds true for all\n> > other functions returning a value in the whole series).\n> \n> What do you mean by use cases? Before operating on a media graph, or\n> whenever the pipeline handlers wants to reset enabled but not\n> immutable links to the non-enabled state, they have a utlity function\n> to do that. Where and how to use it it's up to the pipeline handler,\n> even if it were for me, that should happen before doing any operation\n> on the media graph.\n> \n> Ah, you possibly meant what I just wrote? Damn documentation...\n\n:-)\n\n> > > + */\n> > > +int MediaDevice::resetLinks()\n> > > +{\n> > > +\tfor (MediaEntity *e : entities_) {\n> > > +\t\tfor (MediaPad *sourcePad : e->sources()) {\n> > > +\t\t\tfor (MediaLink *l : sourcePad->links()) {\n> > > +\t\t\t\t/*\n> > > +\t\t\t\t * Do not reset links that are not enabled\n> > > +\t\t\t\t * or immutable.\n> > > +\t\t\t\t */\n> > > +\t\t\t\tif (l->flags() & MEDIA_LNK_FL_IMMUTABLE)\n> > > +\t\t\t\t\tcontinue;\n> > > +\n> > > +\t\t\t\tif (!(l->flags() & MEDIA_LNK_FL_ENABLED))\n> > > +\t\t\t\t\tcontinue;\n> > > +\n> > > +\t\t\t\t/* Get the remote sink pad. */\n> > > +\t\t\t\tMediaPad *sinkPad = dynamic_cast\n> > > +\t\t\t\t\t\t (getObject(l->sink()));\n> > > +\t\t\t\tif (!sinkPad)\n> > > +\t\t\t\t\treturn -ENOENT;\n> > > +\n> > > +\t\t\t\t/* Also get entity to make sure IDs are ok. */\n> > > +\t\t\t\tMediaEntity *sinkEntity =\n> > > +\t\t\t\t\t\tdynamic_cast\n> > > +\t\t\t\t\t\t(getObject(sinkPad->entity()));\n> > > +\t\t\t\tif (!sinkEntity)\n> > > +\t\t\t\t\treturn -ENOENT;\n> > > +\n> > > +\t\t\t\tint ret = setupLink(sourcePad, sinkPad, l, 0);\n> > > +\t\t\t\tif (ret) {\n> > > +\t\t\t\t\tLOG(Error) << \"Link reset failed: \"\n> > > +\t\t\t\t\t\t << e->name() << \"[\"\n> > > +\t\t\t\t\t\t << sourcePad->index()\n> > > +\t\t\t\t\t\t << \"] -> \"\n> > > +\t\t\t\t\t\t << sinkEntity->name() << \"[\"\n> > > +\t\t\t\t\t\t << sinkPad->index() << \"]\";\n> > > +\t\t\t\t\treturn ret;\n> > > +\t\t\t\t}\n> > > +\n> > > +\t\t\t\tLOG(Info) << \"Link reset: \"\n> > > +\t\t\t\t\t << e->name() << \"[\"\n> > > +\t\t\t\t\t << sourcePad->index()\n> > > +\t\t\t\t\t << \"] -> \"\n> > > +\t\t\t\t\t << sinkEntity->name() << \"[\"\n> > > +\t\t\t\t\t << sinkPad->index() << \"]\";\n> > > +\t\t\t}\n> > > +\t\t}\n> > > +\t}\n> > > +\n> > > +\treturn 0;\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\fn MediaDevice::link(std::string, unsigned int, std::string,\n> > > unsigned int)\n> > > + * \\brief Setup a link identified by the entities name and their source\n> > > and\n> > > + * sink pad indexes\n> > > + * \\param source The source entity name\n> > > + * \\param sourceIdx The source pad index\n> > > + * \\param sink The sink entity name\n> > > + * \\param sinkIdx The sink pad index\n> > > + * \\param flags The link setup flag (see MEDIA_LNK_FL_*)\n> > \n> > If I understand the purpose of this function properly (and the fact that\n> > I'm not sure I do means the documentation should be improved :-)), it\n> > serves the same purpose as MediaDevice::setupLink(), but with different\n> > arguments. I would thus name the two functions setupLink(), as C++ allows\n> > overloading functions.\n> \n> No, they're different. setupLink() wants pads and a link where to\n> operate on, as is private, designed to back up this function which is\n> instead public and wants entities names and pad ids. I agree I should\n> look if I could make setupLink a link operation and how ita would look\n> like.\n> \n> > I would go one step further though, at least if you agree with my\n> > proposition to move setupLink to MediaLink, and turn this function into a\n> > link lookup. The callers would then do something similar to\n> > \n> > \tMediaDevice *dev = ...;\n> > \tMediaLink *link = dev->link(\"source\", 0, \"sink\", 3);\n> \n> ^- why three?\n\nIt was just an example :-)\n\n> Can links be set as immutable by userspace, or do they get created\n> immutable from the driver only?\n\nThe immutable flag is set by drivers and not modifiable by userspace.\n\n> > \tif (!link)\n> > \t\treturn -ENODEV;\n> > \t\n> > \tlink->setup(flags);\n> > \n> > and possibly cache the link pointer internally to avoid looking up links\n> > all the time.\n> \n> Here I don't agree actually. I think there should be a MediaDevice\n> provided \"link()\" operation that might use the MediaLink \"setupLink\"\n\nBy the way the operation on MediaLink should be called setup(), not \nsetupLink().\n\n> but from the caller, nothing should be seen that it's not the\n> MediaDevice itself. From the caller I would just like to see\n> \n> ret = mediaDev->link(\"source\", 0, \"sink\", 1, 1);\n> \n> There is no reason (as I immagined this) for the caller to deal with\n> links, and pads itself. Entities are an exception maybe, as we might\n> need to configure format and controls on their subdevices, but that's\n> it.\n> \n> Don't you think this provides a better isolation?\n\nThe reason is that the caller could look up the few links it's interested in \nat init time, and then use them at runtime.\n\n> > > + */\n> > > +int MediaDevice::link(const std::string &source, unsigned int\n> > > sourceIdx,\n> > > +\t\t const std::string &sink, unsigned int sinkIdx,\n> > \n> > At the cost of longer names, sourcePadIndex and sinkPadIndex ?\n> > \n> > > +\t\t unsigned int flags)\n> > > +{\n> > > +\n> > \n> > Stray blank line.\n> > \n> > > +\t/* Make sure the supplied link is well formed. */\n> > > +\tMediaEntity *sourceEntity = getEntityByName(source);\n> > > +\tif (!sourceEntity) {\n> > > +\t\tLOG(Error) << \"Entity name: \" << source << \"not found\";\n> > > +\t\treturn -ENOENT;\n> > > +\t}\n> > > +\n> > > +\tMediaEntity *sinkEntity = getEntityByName(sink);\n> > > +\tif (!sinkEntity) {\n> > > +\t\tLOG(Error) << \"Entity name: \" << source << \"not found\";\n> > \n> > s/source/sink/\n> > s/Entity name:/Entity/\n> > \n> > > +\t\treturn -ENOENT;\n> > > +\t}\n> > > +\n> > > +\tconst MediaPad *sourcePad = sourceEntity->getPadByIndex(sourceIdx);\n> > > +\tif (!sourcePad) {\n> > > +\t\tLOG(Error) << \"Pad \" << sourceIdx << \"not found in entity \"\n> > > +\t\t\t << sourceEntity->name();\n> > > +\t\treturn -ENOENT;\n> > > +\t}\n> > > +\n> > > +\tconst MediaPad *sinkPad = sinkEntity->getPadByIndex(sinkIdx);\n> > > +\tif (!sinkPad) {\n> > > +\t\tLOG(Error) << \"Pad \" << sinkIdx << \"not found in entity \"\n> > > +\t\t\t << sinkEntity->name();\n> > > +\t\treturn -ENOENT;\n> > > +\t}\n> > > +\n> > > +\t/*\n> > > +\t * Walk all links in the source and search for an entry matching the\n> > > +\t * pad ids. If none, the requested link does not exists.\n> > > +\t */\n> > > +\tMediaLink *validLink = nullptr;\n> > > +\tfor (MediaLink *link : sourcePad->links()) {\n> > > +\t\tif (link->source() != sourcePad->id())\n> > > +\t\t\tcontinue;\n> > > +\n> > > +\t\tif (link->sink() != sinkPad->id())\n> > > +\t\t\tcontinue;\n> > > +\n> > > +\t\tvalidLink = link;\n> > > +\t\tbreak;\n> > > +\t}\n> > > +\n> > > +\tif (!validLink) {\n> > > +\t\tLOG(Error) << \"Link not found\"\n> > > +\t\t\t << \"\\\"\" << sourceEntity->name() << \"\\\"[\"\n> > > +\t\t\t << sourcePad->index() << \"] -> \"\n> > > +\t\t\t << \"\\\"\" << sinkEntity->name() << \"\\\"[\"\n> > > +\t\t\t << sinkPad->index() << \"]\";\n> > > +\t\treturn -EINVAL;\n> > > +\t}\n> > > +\n> > > +\tint ret = setupLink(sourcePad, sinkPad, validLink, flags);\n> > > +\tif (ret)\n> > > +\t\treturn ret;\n> > > +\n> > > +\tLOG(Info) << \"Setup link: \"\n> > \n> > LOG(Debug) at most.\n> > \n> > > +\t\t << \"\\\"\" << sourceEntity->name() << \"\\\"[\"\n> > > +\t\t << sourcePad->index() << \"] -> \"\n> > > +\t\t << \"\\\"\" << sinkEntity->name() << \"\\\"[\"\n> > > +\t\t << sinkPad->index() << \"]\"\n> > > +\t\t << \" [\" << flags << \"]\";\n> > > +\n> > > +\treturn 0;\n> > > +}\n> > > +\n> > > +} /* namespace libcamera */\n> > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> > > index da06eba..4cac687 100644\n> > > --- a/src/libcamera/meson.build\n> > > +++ b/src/libcamera/meson.build\n> > > @@ -2,6 +2,7 @@ libcamera_sources = files([\n> > > \n> > > 'log.cpp',\n> > > 'main.cpp',\n> > > 'media_object.cpp',\n> > > \n> > > + 'media_device.cpp',\n> > \n> > sort\n> > \n> > > ])\n> > > \n> > > libcamera_headers = files([","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 80D5860B2E\n\tfor ;\n\tFri, 28 Dec 2018 17:39:54 +0100 (CET)","from avalon.localnet (unknown\n\t[IPv6:2a02:2788:66a:3eb:2624:a446:f4b7:b19d])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id AE7F4DF;\n\tFri, 28 Dec 2018 17:39:53 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1546015193;\n\tbh=0z53lgT5H66NTmv7gu9wFswTObXvBqyRb8FHDnubW6A=;\n\th=From:To:Cc:Subject:Date:In-Reply-To:References:From;\n\tb=lxuFyT7Hz9oblvTJvlwYgUt9tNUbbekwvv+4VV2R9G2JkhDa5kXcPkOIIgFW6ibJ+\n\tNkouu7qh2qj7iGzvFh1YhqBMw+nOysPXUT/T3cMQeroy9FJ1z3A5kKZUxOAeEaiBOD\n\t491lrWZ1ihadtU8wbn9m1hF5kk5H5sFtUCwIgdk4=","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Date":"Fri, 28 Dec 2018 18:40:51 +0200","Message-ID":"<4498295.DGJ6F74ZFY@avalon>","Organization":"Ideas on Board Oy","In-Reply-To":"<20181228143652.GH909@uno.localdomain>","References":"<20181228075743.28637-1-jacopo@jmondi.org>\n\t<1597883.HRDAXlUJTF@avalon> <20181228143652.GH909@uno.localdomain>","MIME-Version":"1.0","Content-Transfer-Encoding":"7Bit","Content-Type":"text/plain; charset=\"us-ascii\"","Subject":"Re: [libcamera-devel] [PATCH RESEND v2 2/4] libcamera: Add\n\tMediaDevice class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Fri, 28 Dec 2018 16:39:54 -0000"}}]