[{"id":93,"web_url":"https://patchwork.libcamera.org/comment/93/","msgid":"<7331052.KbqZa54csX@avalon>","date":"2018-12-28T10:51:23","subject":"Re: [libcamera-devel] [PATCH RESEND v2 1/4] libcamera: Add\n\tMediaObject class hierarchy","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:40 EET Jacopo Mondi wrote:\n> Add a class hierarcy to represent all media objects a media graph\n> represents. Add a base MediaObject class, which retains the global unique\n> object id, and define the derived MediaEntity, MediaLink and MediaPad\n> classes.\n> \n> This hierarchy will be used by the MediaDevice objects which represents and\n> handles the media graph.\n> \n> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>\n> ---\n>  src/libcamera/include/media_object.h | 117 +++++++++++\n>  src/libcamera/media_object.cpp       | 302 +++++++++++++++++++++++++++\n>  src/libcamera/meson.build            |   1 +\n>  3 files changed, 420 insertions(+)\n>  create mode 100644 src/libcamera/include/media_object.h\n>  create mode 100644 src/libcamera/media_object.cpp\n> \n> diff --git a/src/libcamera/include/media_object.h\n> b/src/libcamera/include/media_object.h new file mode 100644\n> index 0000000..99913eb\n> --- /dev/null\n> +++ b/src/libcamera/include/media_object.h\n> @@ -0,0 +1,117 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2018, Google Inc.\n> + *\n> + * media_object.h - Media Device objects: entities, pads and links.\n> + */\n> +#ifndef __LIBCAMERA_MEDIA_OBJECT_H__\n> +#define __LIBCAMERA_MEDIA_OBJECT_H__\n> +\n> +#include <functional>\n> +#include <string>\n> +#include <sstream>\n\ns comes before t :-) I think you can remove <sstream> anyway, it doesn't seem \nto be used.\n\n> +#include <vector>\n> +\n> +#include <linux/media.h>\n\nIf this is just for the definition of struct media_v2_entity, can't it be \nforward-declared instead ?\n\n> +namespace libcamera {\n> +\n> +class MediaDevice;\n> +class MediaEntity;\n> +\n> +class MediaObject\n> +{\n> +public:\n> +\tMediaObject(unsigned int id) : id_(id) { }\n> +\tvirtual ~MediaObject() { }\n> +\n> +\tunsigned int id() const { return id_; }\n> +\n> +protected:\n> +\tunsigned int id_;\n> +};\n> +\n> +class MediaLink : public MediaObject\n> +{\n> +\tfriend class MediaDevice;\n> +\n> +public:\n> +\t~MediaLink() { }\n> +\n> +\tunsigned int source() const { return source_; }\n> +\tunsigned int sink() const { return sink_; }\n> +\tunsigned int flags() const { return flags_; }\n> +\tvoid setFlags(unsigned int flags) { flags_ = flags; }\n> +\n> +private:\n> +\tMediaLink(const struct media_v2_link *link);\n> +\tMediaLink(const MediaLink &) = delete;\n> +\n> +\tunsigned int source_;\n> +\tunsigned int sink_;\n> +\tunsigned int flags_;\n> +};\n> +\n> +class MediaPad : public MediaObject\n> +{\n> +\tfriend class MediaDevice;\n> +\n> +public:\n> +\t~MediaPad();\n> +\n> +\tunsigned int index() const { return index_; }\n> +\tunsigned int entity() const { return entity_; }\n> +\tunsigned int flags() const { return flags_; }\n> +\tconst std::vector<MediaLink *> &links() const { return links_; }\n> +\n> +\tvoid addLink(MediaLink *link);\n> +\n> +private:\n> +\tMediaPad(const struct media_v2_pad *pad);\n> +\tMediaPad(const MediaPad &) = delete;\n> +\n> +\tunsigned int index_;\n> +\tunsigned int entity_;\n> +\tunsigned int flags_;\n> +\n> +\tstd::vector<MediaLink *> links_;\n> +};\n> +\n> +class MediaEntity : public MediaObject\n> +{\n> +\tfriend class MediaDevice;\n> +\n> +public:\n> +\tbool operator==(unsigned int id) const { return this->id_ == id; }\n> +\tbool operator!=(unsigned int id) const { return this->id_ != id; }\n> +\tbool operator==(std::string name) const { return name_ == name; }\n\nAs stated before, I wouldn't define those operators. They're more confusing \nthan typing code explicitly. Compare the two following implementations:\n\n\tMediaEntity *entity = ...;\n\n\tif (entity == 3) {\n\t\t...\n\t} else if (entity == \"foo\") {\n\t\t...\n\t}\n\nvs.\n\n\tMediaEntity *entity = ...;\n\n\tif (entity->id() == 3) {\n\t\t...\n\t} else if (entity->name() == \"foo\") {\n\t\t...\n\t}\n\nThe second is easier to read and more self-explicit, without requiring much \nmore code in the user of the class.\n\n> +\tconst std::string name() const { return name_; }\n\nYou can return a const reference instead of a copy.\n\n> +\tconst std::vector<MediaPad *> &sources() const { return sources_; }\n> +\tconst std::vector<MediaPad *> &sinks() const { return sinks_; }\n> +\n> +\tconst MediaPad *getPadByIndex(unsigned int index);\n> +\tconst MediaPad *getPadById(unsigned int id);\n> +\n> +private:\n> +\tMediaEntity(const struct media_v2_entity *entity);\n> +\tMediaEntity(const MediaEntity &) = delete;\n> +\t~MediaEntity();\n> +\n> +\tstd::string name_;\n> +\tstd::string devnode_;\n> +\n> +\tstd::vector<MediaPad *> sources_;\n> +\tstd::vector<MediaPad *> sinks_;\n\nI think the implementation would be simpler if you merged all pads in a single \nvector. The getPadByIndex() function would just be a vector lookup. The \nsources() and sinks() functions would on the other hand need to return \nstd::vector<> instead of std::vector<>&, but I think that's fine. Depending on \nhow you use them in the callers, you could create a pads() function instead. \nGiven that pads could be bidirectional (unless I'm mistaken there's nothing in \nthe MC API that prevents the source and sink flags being both set) I think \nthis would be a better API.\n\n> +\tint setDevice(const std::string &path);\n> +\n> +\tvoid addPad(MediaPad *pad);\n> +\n> +\tconst MediaPad *__getPad(std::vector<MediaPad *> &v,\n> +\t\t\t   std::function<bool(MediaPad *)> f);\n\nThe __ prefix is reserved in C/C++.\n\n> +\tconst MediaPad *getPad(std::function<bool(MediaPad *)> f);\n> +};\n> +\n> +} /* namespace libcamera */\n> +#endif /* __LIBCAMERA_MEDIA_OBJECT_H__ */\n> diff --git a/src/libcamera/media_object.cpp b/src/libcamera/media_object.cpp\n> new file mode 100644\n> index 0000000..65ed421\n> --- /dev/null\n> +++ b/src/libcamera/media_object.cpp\n> @@ -0,0 +1,302 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2018, Google Inc.\n> + *\n> + * media_object.cpp - Media device objects: entities, pads and links\n> + */\n> +\n> +#include <errno.h>\n> +#include <fcntl.h>\n> +#include <string.h>\n> +#include <sys/stat.h>\n> +\n> +#include <functional>\n> +#include <string>\n> +#include <vector>\n> +\n> +#include <linux/media.h>\n> +\n> +#include \"log.h\"\n> +#include \"media_object.h\"\n> +\n> +/**\n> + * \\file media_object.h\n> + */\n> +namespace libcamera {\n> +\n> +/**\n> + * \\class MediaObject\n> + * \\brief Base class for all media object types\n> + *\n> + * Defines a simple base class for all media objects with a simple\n> + * unique id.\n\nYou may want to expand this a bit, or possibly the \\file documentation block, \nto explain that the media objects model their kernel-side counterpart from the \nMC API.\n\n> + */\n> +\n> +/**\n> + * \\fn MediaObject::MediaObject(unsigned int id)\n> + * \\brief Construct a MediaObject with id \\a id\n> + * \\param id The globally unique object's id as returned by\n> MEDIA_IOC_G_TOPOLOGY\n\nShouldn't it be \"object id\" ?\n\nMaybe s/returned by/obtained from/ ?\n\nIf you document the fact that these objects model the MC objects in the \\file \nor \\class, I think you can skip the \"as returned by MEDIA_IOC_G_TOPOLOGY\" here \nand in all locations below.\n\n> + */\n> +\n> +/**\n> + * \\fn MediaObject::id()\n> + * \\brief Return the object's globally unique id /ref id_\n\nDitto.\n\n/ref ?\n\n> + */\n> +\n> +/**\n> + * \\var MediaObject::id_\n> + * \\brief The MediaObject unique id as returned by MEDIA_IOC_G_TOPOLOGY\n> + */\n\nDo we need to document the private data members ? I suppose it can be useful, \nbut there's lots of overlap between the constructor, id() and id_ \ndocumentation. I don't want to see documentation being written just to tick a \nbox, it should be useful if present.\n\n> +/**\n> + * \\class MediaLink\n> + * \\brief A Media Link object\n> + *\n> + * A MediaLink object represents a media link between two entities\n\nIsn't it between two pads ?\n\n> + */\n> +\n> +/**\n> + * \\fn MediaLink::MediaLink(const struct media_v2_link *link)\n> + * \\brief Construct a MediaLink with informations from \\a link\n\ns/informations/information/\n\n> + * \\param link The media link representation as returned by\n> + *\t       MEDIA_IOC_G_TOPOLOGY\n> + */\n> +MediaLink::MediaLink(const struct media_v2_link *link) :\n> MediaObject(link->id),\n> +\t\t\t\t\t\t\t source_(link->source_id),\n> +\t\t\t\t\t\t\t sink_(link->sink_id),\n> +\t\t\t\t\t\t\t flags_(link->flags) { }\n\nThe indentation is quite peculiar :-) I think it should be (with spaces \ninstead of tabs to ensure it won't be mangled by mail clients)\n\nMediaLink::MediaLink(const struct media_v2_link *link)\n        : MediaObject(link->id), source_(link->source_id),\n          sink_(link->sink_id), flags_(link->flags)\n{\n}\n\n> +/**\n> + * \\fn MediaLink::source()\n> + * \\brief Return the source pad id\n> + */\n> +\n> +/**\n> + * \\fn MediaLink::sink()\n> + * \\brief Return the sink pad id\n> + */\n> +\n> +/**\n> + * \\fn MediaLink::flags()\n> + * \\brief Return the link flags\n> + */\n> +\n> +/**\n> + * \\fn MediaLink::setFlags(unsigned int flags)\n> + * \\brief Set the link flags to \\a flags\n> + * \\param flags The flags to be applied to the link\n> + */\n> +\n> +/**\n> + * \\class MediaPad\n> + * \\brief Media Pad object\n> + *\n> + * A MediaPad object represents a media pad with its associated links\n> + */\n> +\n> +/**\n> + * \\fn MediaPad::MediaPad(const struct media_v2_pad *pad)\n> + * \\brief Create a MediaPad object\n> + * \\param mediaPad The media pad representation as returned by\n> + *\t\t   MEDIA_IOC_G_TOPOLOGY\n\nThe argument name is pad, not mediaPad.\n\n> + */\n> +MediaPad::MediaPad(const struct media_v2_pad *pad) : MediaObject(pad->id),\n> +\t\t\t\t\t\t     index_(pad->index),\n> +\t\t\t\t\t\t     entity_(pad->entity_id),\n> +\t\t\t\t\t\t     flags_(pad->flags) { }\n\nIndentation here too.\n\n> +MediaPad::~MediaPad()\n> +{\n> +\tlinks_.clear();\n> +}\n> +\n> +/**\n> + * \\fn MediaPad::index()\n> + * \\brief Return the 0-indexed pad index\n> + */\n> +\n> +/**\n> + * \\fn MediaPad::entity()\n> + * \\brief Return the entity id this pad belongs to\n> + */\n> +\n> +/**\n> + * \\fn MediaPad::flags()\n> + * \\brief Return the pad flags (MEDIA_PAD_FL_*)\n> + */\n> +\n> +/**\n> + * \\fn MediaPad::links()\n> + * \\brief Return all outbound and inbound links from/to this pad\n> + */\n> +\n> +/**\n> + * \\fn MediaPad::addLink(MediaLink *link)\n> + * \\brief Add a new outbound or inbound link from/to this pad\n> + * \\param link The new link to add\n> + */\n> +void MediaPad::addLink(MediaLink *link)\n> +{\n> +\tlinks_.push_back(link);\n> +}\n> +\n> +/**\n> + * \\class MediaEntity\n> + * \\brief Media entity object\n> + *\n> + * A MediaEntity object represents a media entity with its id, name and its\n> + * associated pads\n> + */\n> +\n> +/**\n> + * \\fn MediaEntity::operator==(unsigned int id) const\n> + * \\brief Compare entities by id (check if they're equal)\n> + * \\param id The entity id to compare with\n> + */\n> +\n> +/**\n> + * \\fn MediaEntity::operator!=(unsigned int id) const\n> + * \\brief Compare entities by id (check if they're not equal)\n> + * \\param id The entity id to compare with\n> + */\n> +\n> +/**\n> + * \\fn MediaEntity::operator==(std::string name) const\n> + * \\brief Compare entities by name (check if they're equal)\n> + * \\param name The entity name to compare with\n> + */\n> +\n> +/**\n> + * \\fn MediaEntity::name()\n> + * \\brief Return the entity name\n> + */\n> +\n> +/**\n> + * \\fn MediaEntity::sources()\n> + * \\brief Get all source pads\n\nSome getters are documented as \"Return ...\", others as \"Get ...\". Should we \nstandardize on one of the two ?\n\n> + */\n> +\n> +/**\n> + * \\fn MediaEntity::sinks()\n> + * \\brief Get all sink pads\n> + */\n> +\n> +/**\n> + * \\fn MediaEntity::getPadByIndex(unsigned int index)\n> + * \\brief Get a pad in this entity by its index\n> + * \\param index The pad index (starting from 0)\n> + */\n> +const MediaPad *MediaEntity::getPadByIndex(unsigned int index)\n> +{\n> +\treturn getPad([&index](MediaPad *p) -> bool { return p->index() == index;\n> });\n> +}\n> +\n> +/**\n> + * \\fn MediaEntity::getPadById(unsigned int id)\n> + * \\brief Get a pad in this entity by its id\n> + * \\param id The pad globally unique id\n> + */\n> +const MediaPad *MediaEntity::getPadById(unsigned int id)\n> +{\n> +\treturn getPad([&id](MediaPad *p) -> bool { return p->id() == id; });\n\nWould it be more efficient to store all objects in a map in MediaObject and \nhave this function look up the pad from there ? Given the limited number of \npads it shouldn't matter too much so I'm OK with this implementation. I don't \nlike the __getPad() and getPad() with std::function much though, but if you \nmerge the sink and source vectors like proposed above it would make all this \nsimpler.\n\n> +}\n> +\n> +/**\n> + * \\fn MediaEntity::MediaEntity(const struct media_v2_entity *entity)\n> + * \\brief Construct a MediaEntity with informations from \\a entity\n> + * \\param entity The media entity representation as returned by\n> + *\t\t MEDIA_IOC_G_TOPOLOGY\n> + */\n> +MediaEntity::MediaEntity(const struct media_v2_entity *entity) :\n> +\t\t\t\t\t\t\tMediaObject(entity->id),\n> +\t\t\t\t\t\t\tname_(entity->name) { }\n> +\n\nIndentation.\n\n> +/**\n> + * \\fn MediaEntity::~MediaEntity()\n> + * \\brief Release memory for all pads and links\n> + */\n> +MediaEntity::~MediaEntity()\n> +{\n> +\tfor (MediaPad *s : sources_)\n> +\t\tdelete s;\n> +\tfor (MediaPad *s : sinks_)\n> +\t\tdelete s;\n> +\n> +\tsources_.clear();\n> +\tsinks_.clear();\n> +}\n> +\n> +/**\n> + * \\var MediaEntity::sources_\n> + * \\brief The MediaPad sources vector\n> + */\n> +\n> +/**\n> + * \\var MediaEntity::sinks_\n> + * \\brief The MediaPad sinks vector\n> + */\n> +\n> +/**\n> + * \\fn MediaEntity::setDevice(const std::string &path)\n> + * \\brief Set the entity video (sub)device node path\n> + * \\param path The video (sub)device node path associated with this entity\n\nI'd talk about \"device node\" instead of \"video (sub)device node\" to keep this \ngeneric, as entities could have non-video nodes. I would then name the \nfunction setDeviceNode().\n\n> + */\n> +int MediaEntity::setDevice(const std::string &path)\n\ns/path/devnode/ ?\n\n> +{\n> +\t/* Make sure the path exists first. */\n> +\tstruct stat pstat;\n> +\tint ret = ::stat(const_cast<const char *>(path.c_str()), &pstat);\n\nDo you need the cast ?\n\n> +\tif (ret < 0) {\n> +\t\tLOG(Error) << \"Unable to open: \" << path << \": \"\n> +\t\t\t   << strerror(errno);\n\nIt's stat(), not open().\n\nI wonder whether the check shouldn't be moved to the caller.\n\n> +\t\treturn -errno;\n> +\t}\n> +\n> +\tdevnode_ = path;\n> +\n> +\treturn 0;\n> +}\n> +\n> +/**\n> + * \\fn MediaEntity::addPad(MediaPad *pad)\n> + * \\brief Add pad \\a pad to \\ref sources_ or \\ref sinks_\n\nYou can write \"Add \\a pad\", there's no need to duplicate the name. The comment \nholds true for various other locations in this patch series.\n\n> + * \\param pad The pad to add\n> + */\n> +void MediaEntity::addPad(MediaPad *pad)\n> +{\n> +\tstd::vector<MediaPad *> *pads =\n> +\t\t\t\t pad->flags() & MEDIA_PAD_FL_SOURCE ?\n> +\t\t\t\t &sources_ : &sinks_;\n> +\tpads->push_back(pad);\n> +}\n> +\n> +/**\n> + * \\fn MediaEntity::__getPad(std::vector<MediaPad *> &v,\n> + *\t\t\t     std::function<bool(MediaPad *)> f)\n> + * \\brief Find MediaPad the satisfies predicates \\a f in the pad vector \\v\n> + * \\param v The std::vector<MediaPad *> to search in\n> + * \\param f The predicate the pad has to satisfy\n> + */\n> +const MediaPad *MediaEntity::__getPad(std::vector<MediaPad *> &v,\n> +\t\t\t\t      std::function<bool(MediaPad *)> f)\n> +{\n> +\tauto it = v.begin();\n> +\twhile (it != sources_.end()) {\n> +\t\tif (f(*it))\n> +\t\t\treturn *it;\n> +\t\t++it;\n> +\t}\n\nHow about\n\n\tfor (MediaPad *pad : v) {\n\t\tif (f(pad))\n\t\t\treturn pad;\n\t}\n\n> +\n> +\treturn nullptr;\n> +}\n> +\n> +/**\n> + * \\fn MediaEntity::getPad(std::function<bool(MediaPad *)> f)\n> + * \\brief Run predicate \\a f on both \\ref sources_ and \\ref sinks_\n> + * \\param f The predicate the pad has to satisfy\n\nAs stated above I find this a bit overengineered. Hopefully the code can be \nsimplified with merging the sink and source vectors. Otherwise you need a more \ndetailed documentation block to explain what this function does.\n\n> + */\n> +const MediaPad *MediaEntity::getPad(std::function<bool(MediaPad *)> f)\n> +{\n> +\tconst MediaPad *_p = __getPad(sources_, f);\n> +\treturn (_p ? _p : __getPad(sinks_, f));\n> +}\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index f632eb5..da06eba 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -1,6 +1,7 @@\n>  libcamera_sources = files([\n>      'log.cpp',\n>      'main.cpp',\n> +    'media_object.cpp',\n>  ])\n> \n>  libcamera_headers = files([","headers":{"Return-Path":"<laurent.pinchart@ideasonboard.com>","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 121ED60B2F\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 28 Dec 2018 11:50:27 +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 7CDA0DF;\n\tFri, 28 Dec 2018 11:50:26 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1545994226;\n\tbh=oZmMiUC8qRBUEDX/pAA0u/8zD1D8UUK6h7fUuLRC7/4=;\n\th=From:To:Cc:Subject:Date:In-Reply-To:References:From;\n\tb=QNJ+qWMbGgozCy3q1CA0dEUou9nVnabmhQZFpyvcK65rC3nYiVIDLpC+ybIXlfnUk\n\tny+a51SoiBv+29xvv2/enVAzJExa2zkWCI6GiwtX0kecdzzi1uZBIefYVnL42+3exS\n\t7US9FCQwvcmirs9o9sKL2Z1SESpf3oj+V/i68SNg=","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"libcamera-devel@lists.libcamera.org","Date":"Fri, 28 Dec 2018 12:51:23 +0200","Message-ID":"<7331052.KbqZa54csX@avalon>","Organization":"Ideas on Board Oy","In-Reply-To":"<20181228075743.28637-2-jacopo@jmondi.org>","References":"<20181228075743.28637-1-jacopo@jmondi.org>\n\t<20181228075743.28637-2-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 1/4] libcamera: Add\n\tMediaObject class hierarchy","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Fri, 28 Dec 2018 10:50:27 -0000"}},{"id":94,"web_url":"https://patchwork.libcamera.org/comment/94/","msgid":"<20181228131115.GG909@uno.localdomain>","date":"2018-12-28T13:11:15","subject":"Re: [libcamera-devel] [PATCH RESEND v2 1/4] libcamera: Add\n\tMediaObject class hierarchy","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"HI Laurent,\n  thanks for review.\n\nOn Fri, Dec 28, 2018 at 12:51:23PM +0200, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Friday, 28 December 2018 09:57:40 EET Jacopo Mondi wrote:\n> > Add a class hierarcy to represent all media objects a media graph\n> > represents. Add a base MediaObject class, which retains the global unique\n> > object id, and define the derived MediaEntity, MediaLink and MediaPad\n> > classes.\n> >\n> > This hierarchy will be used by the MediaDevice objects which represents and\n> > handles the media graph.\n> >\n> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>\n> > ---\n> >  src/libcamera/include/media_object.h | 117 +++++++++++\n> >  src/libcamera/media_object.cpp       | 302 +++++++++++++++++++++++++++\n> >  src/libcamera/meson.build            |   1 +\n> >  3 files changed, 420 insertions(+)\n> >  create mode 100644 src/libcamera/include/media_object.h\n> >  create mode 100644 src/libcamera/media_object.cpp\n> >\n> > diff --git a/src/libcamera/include/media_object.h\n> > b/src/libcamera/include/media_object.h new file mode 100644\n> > index 0000000..99913eb\n> > --- /dev/null\n> > +++ b/src/libcamera/include/media_object.h\n> > @@ -0,0 +1,117 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2018, Google Inc.\n> > + *\n> > + * media_object.h - Media Device objects: entities, pads and links.\n> > + */\n> > +#ifndef __LIBCAMERA_MEDIA_OBJECT_H__\n> > +#define __LIBCAMERA_MEDIA_OBJECT_H__\n> > +\n> > +#include <functional>\n> > +#include <string>\n> > +#include <sstream>\n>\n> s comes before t :-) I think you can remove <sstream> anyway, it doesn't seem\n> to be used.\n\nArgh, could be a leftover! I'll remove (or re-sort at least)\n>\n> > +#include   <vector>\n> > +\n> > +#include   <linux/media.h>\n>\n> If this is just for the definition of struct media_v2_entity, can't it be\n> forward-declared instead ?\n\nWell, the media_object.cpp file that includes this header is going to\ninclude it anyhow, and I don't think it's bad to include it here to\nmake clear where media_v2_entity comes from.\n>\n> > +namespace libcamera {\n> > +\n> > +class MediaDevice;\n> > +class MediaEntity;\n> > +\n> > +class MediaObject\n> > +{\n> > +public:\n> > +\tMediaObject(unsigned int id) : id_(id) { }\n> > +\tvirtual ~MediaObject() { }\n> > +\n> > +\tunsigned int id() const { return id_; }\n> > +\n> > +protected:\n> > +\tunsigned int id_;\n> > +};\n> > +\n> > +class MediaLink : public MediaObject\n> > +{\n> > +\tfriend class MediaDevice;\n> > +\n> > +public:\n> > +\t~MediaLink() { }\n> > +\n> > +\tunsigned int source() const { return source_; }\n> > +\tunsigned int sink() const { return sink_; }\n> > +\tunsigned int flags() const { return flags_; }\n> > +\tvoid setFlags(unsigned int flags) { flags_ = flags; }\n> > +\n> > +private:\n> > +\tMediaLink(const struct media_v2_link *link);\n> > +\tMediaLink(const MediaLink &) = delete;\n> > +\n> > +\tunsigned int source_;\n> > +\tunsigned int sink_;\n> > +\tunsigned int flags_;\n> > +};\n> > +\n> > +class MediaPad : public MediaObject\n> > +{\n> > +\tfriend class MediaDevice;\n> > +\n> > +public:\n> > +\t~MediaPad();\n> > +\n> > +\tunsigned int index() const { return index_; }\n> > +\tunsigned int entity() const { return entity_; }\n> > +\tunsigned int flags() const { return flags_; }\n> > +\tconst std::vector<MediaLink *> &links() const { return links_; }\n> > +\n> > +\tvoid addLink(MediaLink *link);\n> > +\n> > +private:\n> > +\tMediaPad(const struct media_v2_pad *pad);\n> > +\tMediaPad(const MediaPad &) = delete;\n> > +\n> > +\tunsigned int index_;\n> > +\tunsigned int entity_;\n> > +\tunsigned int flags_;\n> > +\n> > +\tstd::vector<MediaLink *> links_;\n> > +};\n> > +\n> > +class MediaEntity : public MediaObject\n> > +{\n> > +\tfriend class MediaDevice;\n> > +\n> > +public:\n> > +\tbool operator==(unsigned int id) const { return this->id_ == id; }\n> > +\tbool operator!=(unsigned int id) const { return this->id_ != id; }\n> > +\tbool operator==(std::string name) const { return name_ == name; }\n>\n> As stated before, I wouldn't define those operators. They're more confusing\n> than typing code explicitly. Compare the two following implementations:\n>\n> \tMediaEntity *entity = ...;\n>\n> \tif (entity == 3) {\n> \t\t...\n> \t} else if (entity == \"foo\") {\n> \t\t...\n> \t}\n>\n> vs.\n>\n> \tMediaEntity *entity = ...;\n>\n> \tif (entity->id() == 3) {\n> \t\t...\n> \t} else if (entity->name() == \"foo\") {\n> \t\t...\n> \t}\n>\n\nI see. They seemed quite natural to me, but I can remove them\n\n> The second is easier to read and more self-explicit, without requiring much\n> more code in the user of the class.\n>\n> > +\tconst std::string name() const { return name_; }\n>\n> You can return a const reference instead of a copy.\n>\n\nRight, this slipped through the cracks\n\n> > +\tconst std::vector<MediaPad *> &sources() const { return sources_; }\n> > +\tconst std::vector<MediaPad *> &sinks() const { return sinks_; }\n> > +\n> > +\tconst MediaPad *getPadByIndex(unsigned int index);\n> > +\tconst MediaPad *getPadById(unsigned int id);\n> > +\n> > +private:\n> > +\tMediaEntity(const struct media_v2_entity *entity);\n> > +\tMediaEntity(const MediaEntity &) = delete;\n> > +\t~MediaEntity();\n> > +\n> > +\tstd::string name_;\n> > +\tstd::string devnode_;\n> > +\n> > +\tstd::vector<MediaPad *> sources_;\n> > +\tstd::vector<MediaPad *> sinks_;\n>\n> I think the implementation would be simpler if you merged all pads in a single\n> vector. The getPadByIndex() function would just be a vector lookup. The\n> sources() and sinks() functions would on the other hand need to return\n> std::vector<> instead of std::vector<>&, but I think that's fine. Depending on\n> how you use them in the callers, you could create a pads() function instead.\n> Given that pads could be bidirectional (unless I'm mistaken there's nothing in\n> the MC API that prevents the source and sink flags being both set) I think\n> this would be a better API.\n\nI think you're right. When I started this I had much more use cases\nfor retrieving sinks or sources only. Right now is just when dumping\nthe media graph, but the caller can get all pads and skip the ones it\nis not interested in. Yes, that would really simplify the API.\n\n>\n> > +\tint setDevice(const std::string &path);\n> > +\n> > +\tvoid addPad(MediaPad *pad);\n> > +\n> > +\tconst MediaPad *__getPad(std::vector<MediaPad *> &v,\n> > +\t\t\t   std::function<bool(MediaPad *)> f);\n>\n> The __ prefix is reserved in C/C++.\n>\n\n? For what ? I read online for \"compiler vendors\"... I used it in so\nmany places... I'll drop\n\n> > +\tconst MediaPad *getPad(std::function<bool(MediaPad *)> f);\n> > +};\n> > +\n> > +} /* namespace libcamera */\n> > +#endif /* __LIBCAMERA_MEDIA_OBJECT_H__ */\n> > diff --git a/src/libcamera/media_object.cpp b/src/libcamera/media_object.cpp\n> > new file mode 100644\n> > index 0000000..65ed421\n> > --- /dev/null\n> > +++ b/src/libcamera/media_object.cpp\n> > @@ -0,0 +1,302 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2018, Google Inc.\n> > + *\n> > + * media_object.cpp - Media device objects: entities, pads and links\n> > + */\n> > +\n> > +#include <errno.h>\n> > +#include <fcntl.h>\n> > +#include <string.h>\n> > +#include <sys/stat.h>\n> > +\n> > +#include <functional>\n> > +#include <string>\n> > +#include <vector>\n> > +\n> > +#include <linux/media.h>\n> > +\n> > +#include \"log.h\"\n> > +#include \"media_object.h\"\n> > +\n> > +/**\n> > + * \\file media_object.h\n> > + */\n> > +namespace libcamera {\n> > +\n> > +/**\n> > + * \\class MediaObject\n> > + * \\brief Base class for all media object types\n> > + *\n> > + * Defines a simple base class for all media objects with a simple\n> > + * unique id.\n>\n> You may want to expand this a bit, or possibly the \\file documentation block,\n> to explain that the media objects model their kernel-side counterpart from the\n> MC API.\n>\n> > + */\n> > +\n> > +/**\n> > + * \\fn MediaObject::MediaObject(unsigned int id)\n> > + * \\brief Construct a MediaObject with id \\a id\n> > + * \\param id The globally unique object's id as returned by\n> > MEDIA_IOC_G_TOPOLOGY\n>\n> Shouldn't it be \"object id\" ?\n>\n> Maybe s/returned by/obtained from/ ?\n>\n> If you document the fact that these objects model the MC objects in the \\file\n> or \\class, I think you can skip the \"as returned by MEDIA_IOC_G_TOPOLOGY\" here\n> and in all locations below.\n>\n> > + */\n> > +\n> > +/**\n> > + * \\fn MediaObject::id()\n> > + * \\brief Return the object's globally unique id /ref id_\n>\n> Ditto.\n>\n> /ref ?\n>\n\nUps, I'll drop\n\n> > + */\n> > +\n> > +/**\n> > + * \\var MediaObject::id_\n> > + * \\brief The MediaObject unique id as returned by MEDIA_IOC_G_TOPOLOGY\n> > + */\n>\n> Do we need to document the private data members ? I suppose it can be useful,\n\nGood question. We might comment if appropriate but leave the doxygen\nout?\n\n> but there's lots of overlap between the constructor, id() and id_\n> documentation. I don't want to see documentation being written just to tick a\n> box, it should be useful if present.\n\nI know this was over commented, but sometimes is difficult to\nfind real value when you have to comment everything (if they were\npublic, we should have documented that anyhow)\n>\n> > +/**\n> > + * \\class MediaLink\n> > + * \\brief A Media Link object\n> > + *\n> > + * A MediaLink object represents a media link between two entities\n>\n> Isn't it between two pads ?\n>\n\nYap\n\n> > + */\n> > +\n> > +/**\n> > + * \\fn MediaLink::MediaLink(const struct media_v2_link *link)\n> > + * \\brief Construct a MediaLink with informations from \\a link\n>\n> s/informations/information/\n>\n> > + * \\param link The media link representation as returned by\n> > + *\t       MEDIA_IOC_G_TOPOLOGY\n> > + */\n> > +MediaLink::MediaLink(const struct media_v2_link *link) :\n> > MediaObject(link->id),\n> > +\t\t\t\t\t\t\t source_(link->source_id),\n> > +\t\t\t\t\t\t\t sink_(link->sink_id),\n> > +\t\t\t\t\t\t\t flags_(link->flags) { }\n>\n> The indentation is quite peculiar :-) I think it should be (with spaces\n> instead of tabs to ensure it won't be mangled by mail clients)\n>\n> MediaLink::MediaLink(const struct media_v2_link *link)\n>         : MediaObject(link->id), source_(link->source_id),\n>           sink_(link->sink_id), flags_(link->flags)\n> {\n\nEh, I had no clear idea how to deal with them.\nI'm not sure I quite like your proposed one neither, but it should at\nleast silence the checkstyle tool, so I'm going for that.\n\n> }\n>\n> > +/**\n> > + * \\fn MediaLink::source()\n> > + * \\brief Return the source pad id\n> > + */\n> > +\n> > +/**\n> > + * \\fn MediaLink::sink()\n> > + * \\brief Return the sink pad id\n> > + */\n> > +\n> > +/**\n> > + * \\fn MediaLink::flags()\n> > + * \\brief Return the link flags\n> > + */\n> > +\n> > +/**\n> > + * \\fn MediaLink::setFlags(unsigned int flags)\n> > + * \\brief Set the link flags to \\a flags\n> > + * \\param flags The flags to be applied to the link\n> > + */\n> > +\n> > +/**\n> > + * \\class MediaPad\n> > + * \\brief Media Pad object\n> > + *\n> > + * A MediaPad object represents a media pad with its associated links\n> > + */\n> > +\n> > +/**\n> > + * \\fn MediaPad::MediaPad(const struct media_v2_pad *pad)\n> > + * \\brief Create a MediaPad object\n> > + * \\param mediaPad The media pad representation as returned by\n> > + *\t\t   MEDIA_IOC_G_TOPOLOGY\n>\n> The argument name is pad, not mediaPad.\n>\n\nThanks\n\n> > + */\n> > +MediaPad::MediaPad(const struct media_v2_pad *pad) : MediaObject(pad->id),\n> > +\t\t\t\t\t\t     index_(pad->index),\n> > +\t\t\t\t\t\t     entity_(pad->entity_id),\n> > +\t\t\t\t\t\t     flags_(pad->flags) { }\n>\n> Indentation here too.\n>\n> > +MediaPad::~MediaPad()\n> > +{\n> > +\tlinks_.clear();\n> > +}\n> > +\n> > +/**\n> > + * \\fn MediaPad::index()\n> > + * \\brief Return the 0-indexed pad index\n> > + */\n> > +\n> > +/**\n> > + * \\fn MediaPad::entity()\n> > + * \\brief Return the entity id this pad belongs to\n> > + */\n> > +\n> > +/**\n> > + * \\fn MediaPad::flags()\n> > + * \\brief Return the pad flags (MEDIA_PAD_FL_*)\n> > + */\n> > +\n> > +/**\n> > + * \\fn MediaPad::links()\n> > + * \\brief Return all outbound and inbound links from/to this pad\n> > + */\n> > +\n> > +/**\n> > + * \\fn MediaPad::addLink(MediaLink *link)\n> > + * \\brief Add a new outbound or inbound link from/to this pad\n> > + * \\param link The new link to add\n> > + */\n> > +void MediaPad::addLink(MediaLink *link)\n> > +{\n> > +\tlinks_.push_back(link);\n> > +}\n> > +\n> > +/**\n> > + * \\class MediaEntity\n> > + * \\brief Media entity object\n> > + *\n> > + * A MediaEntity object represents a media entity with its id, name and its\n> > + * associated pads\n> > + */\n> > +\n> > +/**\n> > + * \\fn MediaEntity::operator==(unsigned int id) const\n> > + * \\brief Compare entities by id (check if they're equal)\n> > + * \\param id The entity id to compare with\n> > + */\n> > +\n> > +/**\n> > + * \\fn MediaEntity::operator!=(unsigned int id) const\n> > + * \\brief Compare entities by id (check if they're not equal)\n> > + * \\param id The entity id to compare with\n> > + */\n> > +\n> > +/**\n> > + * \\fn MediaEntity::operator==(std::string name) const\n> > + * \\brief Compare entities by name (check if they're equal)\n> > + * \\param name The entity name to compare with\n> > + */\n> > +\n> > +/**\n> > + * \\fn MediaEntity::name()\n> > + * \\brief Return the entity name\n> > + */\n> > +\n> > +/**\n> > + * \\fn MediaEntity::sources()\n> > + * \\brief Get all source pads\n>\n> Some getters are documented as \"Return ...\", others as \"Get ...\". Should we\n> standardize on one of the two ?\n>\n\nYes, I would go for 'Return'\n\n> > + */\n> > +\n> > +/**\n> > + * \\fn MediaEntity::sinks()\n> > + * \\brief Get all sink pads\n> > + */\n> > +\n> > +/**\n> > + * \\fn MediaEntity::getPadByIndex(unsigned int index)\n> > + * \\brief Get a pad in this entity by its index\n> > + * \\param index The pad index (starting from 0)\n> > + */\n> > +const MediaPad *MediaEntity::getPadByIndex(unsigned int index)\n> > +{\n> > +\treturn getPad([&index](MediaPad *p) -> bool { return p->index() == index;\n> > });\n> > +}\n> > +\n> > +/**\n> > + * \\fn MediaEntity::getPadById(unsigned int id)\n> > + * \\brief Get a pad in this entity by its id\n> > + * \\param id The pad globally unique id\n> > + */\n> > +const MediaPad *MediaEntity::getPadById(unsigned int id)\n> > +{\n> > +\treturn getPad([&id](MediaPad *p) -> bool { return p->id() == id; });\n>\n> Would it be more efficient to store all objects in a map in MediaObject and\n> have this function look up the pad from there ? Given the limited number of\n> pads it shouldn't matter too much so I'm OK with this implementation. I don't\n> like the __getPad() and getPad() with std::function much though, but if you\n> merge the sink and source vectors like proposed above it would make all this\n> simpler.\n\nActually, I don't think it's worth setting up a map for such a few\nentities. On using lamdas, I don't think it's a bid deal here, it's\nquite clear what happens, but maybe it's just me wanting to play around\nwith new toys, the actual gain in code size is negligible.\n\n>\n> > +}\n> > +\n> > +/**\n> > + * \\fn MediaEntity::MediaEntity(const struct media_v2_entity *entity)\n> > + * \\brief Construct a MediaEntity with informations from \\a entity\n> > + * \\param entity The media entity representation as returned by\n> > + *\t\t MEDIA_IOC_G_TOPOLOGY\n> > + */\n> > +MediaEntity::MediaEntity(const struct media_v2_entity *entity) :\n> > +\t\t\t\t\t\t\tMediaObject(entity->id),\n> > +\t\t\t\t\t\t\tname_(entity->name) { }\n> > +\n>\n> Indentation.\n>\n> > +/**\n> > + * \\fn MediaEntity::~MediaEntity()\n> > + * \\brief Release memory for all pads and links\n> > + */\n> > +MediaEntity::~MediaEntity()\n> > +{\n> > +\tfor (MediaPad *s : sources_)\n> > +\t\tdelete s;\n> > +\tfor (MediaPad *s : sinks_)\n> > +\t\tdelete s;\n> > +\n> > +\tsources_.clear();\n> > +\tsinks_.clear();\n> > +}\n> > +\n> > +/**\n> > + * \\var MediaEntity::sources_\n> > + * \\brief The MediaPad sources vector\n> > + */\n> > +\n> > +/**\n> > + * \\var MediaEntity::sinks_\n> > + * \\brief The MediaPad sinks vector\n> > + */\n> > +\n> > +/**\n> > + * \\fn MediaEntity::setDevice(const std::string &path)\n> > + * \\brief Set the entity video (sub)device node path\n> > + * \\param path The video (sub)device node path associated with this entity\n>\n> I'd talk about \"device node\" instead of \"video (sub)device node\" to keep this\n> generic, as entities could have non-video nodes. I would then name the\n> function setDeviceNode().\n>\n> > + */\n> > +int MediaEntity::setDevice(const std::string &path)\n>\n> s/path/devnode/ ?\n>\n> > +{\n> > +\t/* Make sure the path exists first. */\n> > +\tstruct stat pstat;\n> > +\tint ret = ::stat(const_cast<const char *>(path.c_str()), &pstat);\n>\n> Do you need the cast ?\n>\n\nI think g++ complains otherwise.\n\n> > +\tif (ret < 0) {\n> > +\t\tLOG(Error) << \"Unable to open: \" << path << \": \"\n> > +\t\t\t   << strerror(errno);\n>\n> It's stat(), not open().\n>\n> I wonder whether the check shouldn't be moved to the caller.\n>\n> > +\t\treturn -errno;\n> > +\t}\n> > +\n> > +\tdevnode_ = path;\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > +/**\n> > + * \\fn MediaEntity::addPad(MediaPad *pad)\n> > + * \\brief Add pad \\a pad to \\ref sources_ or \\ref sinks_\n>\n> You can write \"Add \\a pad\", there's no need to duplicate the name. The comment\n> holds true for various other locations in this patch series.\n>\n> > + * \\param pad The pad to add\n> > + */\n> > +void MediaEntity::addPad(MediaPad *pad)\n> > +{\n> > +\tstd::vector<MediaPad *> *pads =\n> > +\t\t\t\t pad->flags() & MEDIA_PAD_FL_SOURCE ?\n> > +\t\t\t\t &sources_ : &sinks_;\n> > +\tpads->push_back(pad);\n> > +}\n> > +\n> > +/**\n> > + * \\fn MediaEntity::__getPad(std::vector<MediaPad *> &v,\n> > + *\t\t\t     std::function<bool(MediaPad *)> f)\n> > + * \\brief Find MediaPad the satisfies predicates \\a f in the pad vector \\v\n> > + * \\param v The std::vector<MediaPad *> to search in\n> > + * \\param f The predicate the pad has to satisfy\n> > + */\n> > +const MediaPad *MediaEntity::__getPad(std::vector<MediaPad *> &v,\n> > +\t\t\t\t      std::function<bool(MediaPad *)> f)\n> > +{\n> > +\tauto it = v.begin();\n> > +\twhile (it != sources_.end()) {\n> > +\t\tif (f(*it))\n> > +\t\t\treturn *it;\n> > +\t\t++it;\n> > +\t}\n>\n> How about\n>\n> \tfor (MediaPad *pad : v) {\n> \t\tif (f(pad))\n> \t\t\treturn pad;\n> \t}\n>\n\nNot sure why I kept iterators...\n\n> > +\n> > +\treturn nullptr;\n> > +}\n> > +\n> > +/**\n> > + * \\fn MediaEntity::getPad(std::function<bool(MediaPad *)> f)\n> > + * \\brief Run predicate \\a f on both \\ref sources_ and \\ref sinks_\n> > + * \\param f The predicate the pad has to satisfy\n>\n> As stated above I find this a bit overengineered. Hopefully the code can be\n> simplified with merging the sink and source vectors. Otherwise you need a more\n> detailed documentation block to explain what this function does.\n>\n> > + */\n> > +const MediaPad *MediaEntity::getPad(std::function<bool(MediaPad *)> f)\n> > +{\n> > +\tconst MediaPad *_p = __getPad(sources_, f);\n> > +\treturn (_p ? _p : __getPad(sinks_, f));\n> > +}\n\nMaybe just keep a getPad() that goes on the now single pad vector but\nstill using a predicate to do the matching?\n\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> > index f632eb5..da06eba 100644\n> > --- a/src/libcamera/meson.build\n> > +++ b/src/libcamera/meson.build\n> > @@ -1,6 +1,7 @@\n> >  libcamera_sources = files([\n> >      'log.cpp',\n> >      'main.cpp',\n> > +    'media_object.cpp',\n> >  ])\n> >\n> >  libcamera_headers = files([\n\nI missed to add media_object.h (and media_device.h) here I guess...\n\n>\n\nThanks\n   j\n\n> --\n> Regards,\n>\n> Laurent Pinchart\n>\n>\n>","headers":{"Return-Path":"<jacopo@jmondi.org>","Received":["from relay10.mail.gandi.net (relay10.mail.gandi.net\n\t[217.70.178.230])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7A12060B2E\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 28 Dec 2018 14:11:15 +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 relay10.mail.gandi.net (Postfix) with ESMTPSA id E4005240009;\n\tFri, 28 Dec 2018 13:11:14 +0000 (UTC)"],"Date":"Fri, 28 Dec 2018 14:11:15 +0100","From":"Jacopo Mondi <jacopo@jmondi.org>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20181228131115.GG909@uno.localdomain>","References":"<20181228075743.28637-1-jacopo@jmondi.org>\n\t<20181228075743.28637-2-jacopo@jmondi.org>\n\t<7331052.KbqZa54csX@avalon>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"aFi3jz1oiPowsTUB\"","Content-Disposition":"inline","In-Reply-To":"<7331052.KbqZa54csX@avalon>","User-Agent":"Mutt/1.11.1 (2018-12-01)","Subject":"Re: [libcamera-devel] [PATCH RESEND v2 1/4] libcamera: Add\n\tMediaObject class hierarchy","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Fri, 28 Dec 2018 13:11:15 -0000"}},{"id":96,"web_url":"https://patchwork.libcamera.org/comment/96/","msgid":"<3401878.rr1as6qmyV@avalon>","date":"2018-12-28T13:50:27","subject":"Re: [libcamera-devel] [PATCH RESEND v2 1/4] libcamera: Add\n\tMediaObject class hierarchy","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 15:11:15 EET Jacopo Mondi wrote:\n> On Fri, Dec 28, 2018 at 12:51:23PM +0200, Laurent Pinchart wrote:\n> > On Friday, 28 December 2018 09:57:40 EET Jacopo Mondi wrote:\n> > > Add a class hierarcy to represent all media objects a media graph\n> > > represents. Add a base MediaObject class, which retains the global\n> > > unique object id, and define the derived MediaEntity, MediaLink and\n> > > MediaPad classes.\n> > > \n> > > This hierarchy will be used by the MediaDevice objects which represents\n> > > and handles the media graph.\n> > > \n> > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>\n> > > ---\n> > > \n> > >  src/libcamera/include/media_object.h | 117 +++++++++++\n> > >  src/libcamera/media_object.cpp       | 302 +++++++++++++++++++++++++++\n> > >  src/libcamera/meson.build            |   1 +\n> > >  3 files changed, 420 insertions(+)\n> > >  create mode 100644 src/libcamera/include/media_object.h\n> > >  create mode 100644 src/libcamera/media_object.cpp\n> > > \n> > > diff --git a/src/libcamera/include/media_object.h\n> > > b/src/libcamera/include/media_object.h new file mode 100644\n> > > index 0000000..99913eb\n> > > --- /dev/null\n> > > +++ b/src/libcamera/include/media_object.h\n> > > @@ -0,0 +1,117 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2018, Google Inc.\n> > > + *\n> > > + * media_object.h - Media Device objects: entities, pads and links.\n> > > + */\n> > > +#ifndef __LIBCAMERA_MEDIA_OBJECT_H__\n> > > +#define __LIBCAMERA_MEDIA_OBJECT_H__\n> > > +\n> > > +#include <functional>\n> > > +#include <string>\n> > > +#include <sstream>\n> > \n> > s comes before t :-) I think you can remove <sstream> anyway, it doesn't\n> > seem to be used.\n> \n> Argh, could be a leftover! I'll remove (or re-sort at least)\n> \n> > > +#include   <vector>\n> > > +\n> > > +#include   <linux/media.h>\n> > \n> > If this is just for the definition of struct media_v2_entity, can't it be\n> > forward-declared instead ?\n> \n> Well, the media_object.cpp file that includes this header is going to\n> include it anyhow, and I don't think it's bad to include it here to\n> make clear where media_v2_entity comes from.\n\nIn this specific case it's not a big deal, but in general you should try to \ninclude as few headers as possible and use forward declarations instead. For \nthat reason I would apply that rule globally, including here.\n\n> > > +namespace libcamera {\n> > > +\n> > > +class MediaDevice;\n> > > +class MediaEntity;\n> > > +\n> > > +class MediaObject\n> > > +{\n> > > +public:\n> > > +\tMediaObject(unsigned int id) : id_(id) { }\n> > > +\tvirtual ~MediaObject() { }\n> > > +\n> > > +\tunsigned int id() const { return id_; }\n> > > +\n> > > +protected:\n> > > +\tunsigned int id_;\n> > > +};\n> > > +\n> > > +class MediaLink : public MediaObject\n> > > +{\n> > > +\tfriend class MediaDevice;\n> > > +\n> > > +public:\n> > > +\t~MediaLink() { }\n> > > +\n> > > +\tunsigned int source() const { return source_; }\n> > > +\tunsigned int sink() const { return sink_; }\n> > > +\tunsigned int flags() const { return flags_; }\n> > > +\tvoid setFlags(unsigned int flags) { flags_ = flags; }\n> > > +\n> > > +private:\n> > > +\tMediaLink(const struct media_v2_link *link);\n> > > +\tMediaLink(const MediaLink &) = delete;\n> > > +\n> > > +\tunsigned int source_;\n> > > +\tunsigned int sink_;\n> > > +\tunsigned int flags_;\n> > > +};\n> > > +\n> > > +class MediaPad : public MediaObject\n> > > +{\n> > > +\tfriend class MediaDevice;\n> > > +\n> > > +public:\n> > > +\t~MediaPad();\n> > > +\n> > > +\tunsigned int index() const { return index_; }\n> > > +\tunsigned int entity() const { return entity_; }\n> > > +\tunsigned int flags() const { return flags_; }\n> > > +\tconst std::vector<MediaLink *> &links() const { return links_; }\n> > > +\n> > > +\tvoid addLink(MediaLink *link);\n> > > +\n> > > +private:\n> > > +\tMediaPad(const struct media_v2_pad *pad);\n> > > +\tMediaPad(const MediaPad &) = delete;\n> > > +\n> > > +\tunsigned int index_;\n> > > +\tunsigned int entity_;\n> > > +\tunsigned int flags_;\n> > > +\n> > > +\tstd::vector<MediaLink *> links_;\n> > > +};\n> > > +\n> > > +class MediaEntity : public MediaObject\n> > > +{\n> > > +\tfriend class MediaDevice;\n> > > +\n> > > +public:\n> > > +\tbool operator==(unsigned int id) const { return this->id_ == id; }\n> > > +\tbool operator!=(unsigned int id) const { return this->id_ != id; }\n> > > +\tbool operator==(std::string name) const { return name_ == name; }\n> > \n> > As stated before, I wouldn't define those operators. They're more\n> > confusing\n> > \n> > than typing code explicitly. Compare the two following implementations:\n> > \tMediaEntity *entity = ...;\n> > \t\n> > \tif (entity == 3) {\n> > \t\t...\n> > \t} else if (entity == \"foo\") {\n> > \t\t...\n> > \t}\n> > \n> > vs.\n> > \n> > \tMediaEntity *entity = ...;\n> > \t\n> > \tif (entity->id() == 3) {\n> > \t\t...\n> > \t} else if (entity->name() == \"foo\") {\n> > \t\t...\n> > \t}\n> \n> I see. They seemed quite natural to me, but I can remove them\n> \n> > The second is easier to read and more self-explicit, without requiring\n> > much\n> > more code in the user of the class.\n> > \n> > > +\tconst std::string name() const { return name_; }\n> > \n> > You can return a const reference instead of a copy.\n> \n> Right, this slipped through the cracks\n> \n> > > +\tconst std::vector<MediaPad *> &sources() const { return sources_; }\n> > > +\tconst std::vector<MediaPad *> &sinks() const { return sinks_; }\n> > > +\n> > > +\tconst MediaPad *getPadByIndex(unsigned int index);\n> > > +\tconst MediaPad *getPadById(unsigned int id);\n> > > +\n> > > +private:\n> > > +\tMediaEntity(const struct media_v2_entity *entity);\n> > > +\tMediaEntity(const MediaEntity &) = delete;\n> > > +\t~MediaEntity();\n> > > +\n> > > +\tstd::string name_;\n> > > +\tstd::string devnode_;\n> > > +\n> > > +\tstd::vector<MediaPad *> sources_;\n> > > +\tstd::vector<MediaPad *> sinks_;\n> > \n> > I think the implementation would be simpler if you merged all pads in a\n> > single vector. The getPadByIndex() function would just be a vector\n> > lookup. The sources() and sinks() functions would on the other hand need\n> > to return std::vector<> instead of std::vector<>&, but I think that's\n> > fine. Depending on how you use them in the callers, you could create a\n> > pads() function instead. Given that pads could be bidirectional (unless\n> > I'm mistaken there's nothing in the MC API that prevents the source and\n> > sink flags being both set) I think this would be a better API.\n> \n> I think you're right. When I started this I had much more use cases\n> for retrieving sinks or sources only. Right now is just when dumping\n> the media graph, but the caller can get all pads and skip the ones it\n> is not interested in. Yes, that would really simplify the API.\n> \n> > > +\tint setDevice(const std::string &path);\n> > > +\n> > > +\tvoid addPad(MediaPad *pad);\n> > > +\n> > > +\tconst MediaPad *__getPad(std::vector<MediaPad *> &v,\n> > > +\t\t\t   std::function<bool(MediaPad *)> f);\n> > \n> > The __ prefix is reserved in C/C++.\n> \n> ? For what ? I read online for \"compiler vendors\"... I used it in so\n> many places... I'll drop\n\nIt's for compiler vendors indeed. A compiler (and the related standard \nlibraries) can freely define names starting with two underscores, so they're \nreserved.\n\n> > > +\tconst MediaPad *getPad(std::function<bool(MediaPad *)> f);\n> > > +};\n> > > +\n> > > +} /* namespace libcamera */\n> > > +#endif /* __LIBCAMERA_MEDIA_OBJECT_H__ */\n> > > diff --git a/src/libcamera/media_object.cpp\n> > > b/src/libcamera/media_object.cpp new file mode 100644\n> > > index 0000000..65ed421\n> > > --- /dev/null\n> > > +++ b/src/libcamera/media_object.cpp\n> > > @@ -0,0 +1,302 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2018, Google Inc.\n> > > + *\n> > > + * media_object.cpp - Media device objects: entities, pads and links\n> > > + */\n> > > +\n> > > +#include <errno.h>\n> > > +#include <fcntl.h>\n> > > +#include <string.h>\n> > > +#include <sys/stat.h>\n> > > +\n> > > +#include <functional>\n> > > +#include <string>\n> > > +#include <vector>\n> > > +\n> > > +#include <linux/media.h>\n> > > +\n> > > +#include \"log.h\"\n> > > +#include \"media_object.h\"\n> > > +\n> > > +/**\n> > > + * \\file media_object.h\n> > > + */\n> > > +namespace libcamera {\n> > > +\n> > > +/**\n> > > + * \\class MediaObject\n> > > + * \\brief Base class for all media object types\n> > > + *\n> > > + * Defines a simple base class for all media objects with a simple\n> > > + * unique id.\n> > \n> > You may want to expand this a bit, or possibly the \\file documentation\n> > block, to explain that the media objects model their kernel-side\n> > counterpart from the MC API.\n> > \n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn MediaObject::MediaObject(unsigned int id)\n> > > + * \\brief Construct a MediaObject with id \\a id\n> > > + * \\param id The globally unique object's id as returned by\n> > > MEDIA_IOC_G_TOPOLOGY\n> > \n> > Shouldn't it be \"object id\" ?\n> > \n> > Maybe s/returned by/obtained from/ ?\n> > \n> > If you document the fact that these objects model the MC objects in the\n> > \\file or \\class, I think you can skip the \"as returned by\n> > MEDIA_IOC_G_TOPOLOGY\" here and in all locations below.\n> > \n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn MediaObject::id()\n> > > + * \\brief Return the object's globally unique id /ref id_\n> > \n> > Ditto.\n> > \n> > /ref ?\n> \n> Ups, I'll drop\n> \n> > > + */\n> > > +\n> > > +/**\n> > > + * \\var MediaObject::id_\n> > > + * \\brief The MediaObject unique id as returned by MEDIA_IOC_G_TOPOLOGY\n> > > + */\n> > \n> > Do we need to document the private data members ? I suppose it can be\n> > useful,\n> \n> Good question. We might comment if appropriate but leave the doxygen\n> out?\n> \n> > but there's lots of overlap between the constructor, id() and id_\n> > documentation. I don't want to see documentation being written just to\n> > tick a box, it should be useful if present.\n> \n> I know this was over commented, but sometimes is difficult to\n> find real value when you have to comment everything (if they were\n> public, we should have documented that anyhow)\n\nI agree, sometimes there's very little value in documenting some of the \nfunctions. I don't know how to solve this problem yet.\n\n> > > +/**\n> > > + * \\class MediaLink\n> > > + * \\brief A Media Link object\n> > > + *\n> > > + * A MediaLink object represents a media link between two entities\n> > \n> > Isn't it between two pads ?\n> \n> Yap\n> \n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn MediaLink::MediaLink(const struct media_v2_link *link)\n> > > + * \\brief Construct a MediaLink with informations from \\a link\n> > \n> > s/informations/information/\n> > \n> > > + * \\param link The media link representation as returned by\n> > > + *\t       MEDIA_IOC_G_TOPOLOGY\n> > > + */\n> > > +MediaLink::MediaLink(const struct media_v2_link *link) :\n> > > MediaObject(link->id),\n> > > +\t\t\t\t\t\t\t source_(link->source_id),\n> > > +\t\t\t\t\t\t\t sink_(link->sink_id),\n> > > +\t\t\t\t\t\t\t flags_(link->flags) { }\n> > \n> > The indentation is quite peculiar :-) I think it should be (with spaces\n> > instead of tabs to ensure it won't be mangled by mail clients)\n> > \n> > MediaLink::MediaLink(const struct media_v2_link *link)\n> > \n> >         : MediaObject(link->id), source_(link->source_id),\n> >         : \n> >           sink_(link->sink_id), flags_(link->flags)\n> > \n> > {\n> \n> Eh, I had no clear idea how to deal with them.\n> I'm not sure I quite like your proposed one neither, but it should at\n> least silence the checkstyle tool, so I'm going for that.\n> \n> > }\n> > \n> > > +/**\n> > > + * \\fn MediaLink::source()\n> > > + * \\brief Return the source pad id\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn MediaLink::sink()\n> > > + * \\brief Return the sink pad id\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn MediaLink::flags()\n> > > + * \\brief Return the link flags\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn MediaLink::setFlags(unsigned int flags)\n> > > + * \\brief Set the link flags to \\a flags\n> > > + * \\param flags The flags to be applied to the link\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\class MediaPad\n> > > + * \\brief Media Pad object\n> > > + *\n> > > + * A MediaPad object represents a media pad with its associated links\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn MediaPad::MediaPad(const struct media_v2_pad *pad)\n> > > + * \\brief Create a MediaPad object\n> > > + * \\param mediaPad The media pad representation as returned by\n> > > + *\t\t   MEDIA_IOC_G_TOPOLOGY\n> > \n> > The argument name is pad, not mediaPad.\n> \n> Thanks\n> \n> > > + */\n> > > +MediaPad::MediaPad(const struct media_v2_pad *pad) :\n> > > MediaObject(pad->id),\n> > > +\t\t\t\t\t\t     index_(pad->index),\n> > > +\t\t\t\t\t\t     entity_(pad->entity_id),\n> > > +\t\t\t\t\t\t     flags_(pad->flags) { }\n> > \n> > Indentation here too.\n> > \n> > > +MediaPad::~MediaPad()\n> > > +{\n> > > +\tlinks_.clear();\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\fn MediaPad::index()\n> > > + * \\brief Return the 0-indexed pad index\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn MediaPad::entity()\n> > > + * \\brief Return the entity id this pad belongs to\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn MediaPad::flags()\n> > > + * \\brief Return the pad flags (MEDIA_PAD_FL_*)\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn MediaPad::links()\n> > > + * \\brief Return all outbound and inbound links from/to this pad\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn MediaPad::addLink(MediaLink *link)\n> > > + * \\brief Add a new outbound or inbound link from/to this pad\n> > > + * \\param link The new link to add\n> > > + */\n> > > +void MediaPad::addLink(MediaLink *link)\n> > > +{\n> > > +\tlinks_.push_back(link);\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\class MediaEntity\n> > > + * \\brief Media entity object\n> > > + *\n> > > + * A MediaEntity object represents a media entity with its id, name and\n> > > its + * associated pads\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn MediaEntity::operator==(unsigned int id) const\n> > > + * \\brief Compare entities by id (check if they're equal)\n> > > + * \\param id The entity id to compare with\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn MediaEntity::operator!=(unsigned int id) const\n> > > + * \\brief Compare entities by id (check if they're not equal)\n> > > + * \\param id The entity id to compare with\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn MediaEntity::operator==(std::string name) const\n> > > + * \\brief Compare entities by name (check if they're equal)\n> > > + * \\param name The entity name to compare with\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn MediaEntity::name()\n> > > + * \\brief Return the entity name\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn MediaEntity::sources()\n> > > + * \\brief Get all source pads\n> > \n> > Some getters are documented as \"Return ...\", others as \"Get ...\". Should\n> > we standardize on one of the two ?\n> \n> Yes, I would go for 'Return'\n> \n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn MediaEntity::sinks()\n> > > + * \\brief Get all sink pads\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn MediaEntity::getPadByIndex(unsigned int index)\n> > > + * \\brief Get a pad in this entity by its index\n> > > + * \\param index The pad index (starting from 0)\n> > > + */\n> > > +const MediaPad *MediaEntity::getPadByIndex(unsigned int index)\n> > > +{\n> > > +\treturn getPad([&index](MediaPad *p) -> bool { return p->index() ==\n> > > index;\n> > > });\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\fn MediaEntity::getPadById(unsigned int id)\n> > > + * \\brief Get a pad in this entity by its id\n> > > + * \\param id The pad globally unique id\n> > > + */\n> > > +const MediaPad *MediaEntity::getPadById(unsigned int id)\n> > > +{\n> > > +\treturn getPad([&id](MediaPad *p) -> bool { return p->id() == id; });\n> > \n> > Would it be more efficient to store all objects in a map in MediaObject\n> > and have this function look up the pad from there ? Given the limited\n> > number of pads it shouldn't matter too much so I'm OK with this\n> > implementation. I don't like the __getPad() and getPad() with\n> > std::function much though, but if you merge the sink and source vectors\n> > like proposed above it would make all this simpler.\n> \n> Actually, I don't think it's worth setting up a map for such a few\n> entities.\n\nI meant in MediaDevice, not MediaObject, sorry. There's already a \nmediaObjects_ map there, which we could use to look up the pad.\n\n> On using lamdas, I don't think it's a bid deal here, it's\n> quite clear what happens, but maybe it's just me wanting to play around\n> with new toys, the actual gain in code size is negligible.\n> \n> > > +}\n> > > +\n> > > +/**\n> > > + * \\fn MediaEntity::MediaEntity(const struct media_v2_entity *entity)\n> > > + * \\brief Construct a MediaEntity with informations from \\a entity\n> > > + * \\param entity The media entity representation as returned by\n> > > + *\t\t MEDIA_IOC_G_TOPOLOGY\n> > > + */\n> > > +MediaEntity::MediaEntity(const struct media_v2_entity *entity) :\n> > > +\t\t\t\t\t\t\tMediaObject(entity->id),\n> > > +\t\t\t\t\t\t\tname_(entity->name) { }\n> > > +\n> > \n> > Indentation.\n> > \n> > > +/**\n> > > + * \\fn MediaEntity::~MediaEntity()\n> > > + * \\brief Release memory for all pads and links\n> > > + */\n> > > +MediaEntity::~MediaEntity()\n> > > +{\n> > > +\tfor (MediaPad *s : sources_)\n> > > +\t\tdelete s;\n> > > +\tfor (MediaPad *s : sinks_)\n> > > +\t\tdelete s;\n> > > +\n> > > +\tsources_.clear();\n> > > +\tsinks_.clear();\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\var MediaEntity::sources_\n> > > + * \\brief The MediaPad sources vector\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\var MediaEntity::sinks_\n> > > + * \\brief The MediaPad sinks vector\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn MediaEntity::setDevice(const std::string &path)\n> > > + * \\brief Set the entity video (sub)device node path\n> > > + * \\param path The video (sub)device node path associated with this\n> > > entity\n> > \n> > I'd talk about \"device node\" instead of \"video (sub)device node\" to keep\n> > this generic, as entities could have non-video nodes. I would then name\n> > the function setDeviceNode().\n> > \n> > > + */\n> > > +int MediaEntity::setDevice(const std::string &path)\n> > \n> > s/path/devnode/ ?\n> > \n> > > +{\n> > > +\t/* Make sure the path exists first. */\n> > > +\tstruct stat pstat;\n> > > +\tint ret = ::stat(const_cast<const char *>(path.c_str()), &pstat);\n> > \n> > Do you need the cast ?\n> \n> I think g++ complains otherwise.\n\nYou don't case when calling ::open(), I wonder why one would succeed and the \nother one wouldn't. Could you please double-check ?\n\n> > > +\tif (ret < 0) {\n> > > +\t\tLOG(Error) << \"Unable to open: \" << path << \": \"\n> > > +\t\t\t   << strerror(errno);\n> > \n> > It's stat(), not open().\n> > \n> > I wonder whether the check shouldn't be moved to the caller.\n> > \n> > > +\t\treturn -errno;\n> > > +\t}\n> > > +\n> > > +\tdevnode_ = path;\n> > > +\n> > > +\treturn 0;\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\fn MediaEntity::addPad(MediaPad *pad)\n> > > + * \\brief Add pad \\a pad to \\ref sources_ or \\ref sinks_\n> > \n> > You can write \"Add \\a pad\", there's no need to duplicate the name. The\n> > comment holds true for various other locations in this patch series.\n> > \n> > > + * \\param pad The pad to add\n> > > + */\n> > > +void MediaEntity::addPad(MediaPad *pad)\n> > > +{\n> > > +\tstd::vector<MediaPad *> *pads =\n> > > +\t\t\t\t pad->flags() & MEDIA_PAD_FL_SOURCE ?\n> > > +\t\t\t\t &sources_ : &sinks_;\n> > > +\tpads->push_back(pad);\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\fn MediaEntity::__getPad(std::vector<MediaPad *> &v,\n> > > + *\t\t\t     std::function<bool(MediaPad *)> f)\n> > > + * \\brief Find MediaPad the satisfies predicates \\a f in the pad vector\n> > > \\v\n> > > + * \\param v The std::vector<MediaPad *> to search in\n> > > + * \\param f The predicate the pad has to satisfy\n> > > + */\n> > > +const MediaPad *MediaEntity::__getPad(std::vector<MediaPad *> &v,\n> > > +\t\t\t\t      std::function<bool(MediaPad *)> f)\n> > > +{\n> > > +\tauto it = v.begin();\n> > > +\twhile (it != sources_.end()) {\n> > > +\t\tif (f(*it))\n> > > +\t\t\treturn *it;\n> > > +\t\t++it;\n> > > +\t}\n> > \n> > How about\n> > \n> > \tfor (MediaPad *pad : v) {\n> > \t\tif (f(pad))\n> > \t\t\treturn pad;\n> > \t}\n> \n> Not sure why I kept iterators...\n\nNew toy ? :-)\n\n> > > +\n> > > +\treturn nullptr;\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\fn MediaEntity::getPad(std::function<bool(MediaPad *)> f)\n> > > + * \\brief Run predicate \\a f on both \\ref sources_ and \\ref sinks_\n> > > + * \\param f The predicate the pad has to satisfy\n> > \n> > As stated above I find this a bit overengineered. Hopefully the code can\n> > be simplified with merging the sink and source vectors. Otherwise you need\n> > a more detailed documentation block to explain what this function does.\n> > \n> > > + */\n> > > +const MediaPad *MediaEntity::getPad(std::function<bool(MediaPad *)> f)\n> > > +{\n> > > +\tconst MediaPad *_p = __getPad(sources_, f);\n> > > +\treturn (_p ? _p : __getPad(sinks_, f));\n> > > +}\n> \n> Maybe just keep a getPad() that goes on the now single pad vector but\n> still using a predicate to do the matching?\n\nIf the getPadById() function is reworked to use the mediaObjects_ map, \ngetPadByIndex() can be merged with getPad(), and you won't need a predicate.\n\n> > > +\n> > > +} /* namespace libcamera */\n> > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> > > index f632eb5..da06eba 100644\n> > > --- a/src/libcamera/meson.build\n> > > +++ b/src/libcamera/meson.build\n> > > @@ -1,6 +1,7 @@\n> > > \n> > >  libcamera_sources = files([\n> > >  \n> > >      'log.cpp',\n> > >      'main.cpp',\n> > > \n> > > +    'media_object.cpp',\n> > > \n> > >  ])\n> > >  \n> > >  libcamera_headers = files([\n> \n> I missed to add media_object.h (and media_device.h) here I guess...","headers":{"Return-Path":"<laurent.pinchart@ideasonboard.com>","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2AAF060B2E\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 28 Dec 2018 14:49:31 +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 5AE7BDF;\n\tFri, 28 Dec 2018 14:49:30 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1546004970;\n\tbh=aE0yV9S6voLq+FeUuDOb2JC/UmxEr9GaHXvcnrw20wY=;\n\th=From:To:Cc:Subject:Date:In-Reply-To:References:From;\n\tb=NQyFljluMD+DUhbzXJekByM2Z3Kk54wvFZlBJ6vxnMZ6QB9P8YexyVcpNbOAjQCUl\n\tYr74iK2eEnhVeDCamsdr08q5ybs4NdeEe7VsPCeFP7+CWD27fs9ag0DTNGJqb1QWa+\n\tJ9AAv7iT2qpiWuYR4w0+3UsNCv0OehhbP9r9cx10=","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Jacopo Mondi <jacopo@jmondi.org>","Cc":"libcamera-devel@lists.libcamera.org","Date":"Fri, 28 Dec 2018 15:50:27 +0200","Message-ID":"<3401878.rr1as6qmyV@avalon>","Organization":"Ideas on Board Oy","In-Reply-To":"<20181228131115.GG909@uno.localdomain>","References":"<20181228075743.28637-1-jacopo@jmondi.org>\n\t<7331052.KbqZa54csX@avalon> <20181228131115.GG909@uno.localdomain>","MIME-Version":"1.0","Content-Transfer-Encoding":"7Bit","Content-Type":"text/plain; charset=\"us-ascii\"","Subject":"Re: [libcamera-devel] [PATCH RESEND v2 1/4] libcamera: Add\n\tMediaObject class hierarchy","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Fri, 28 Dec 2018 13:49:31 -0000"}},{"id":97,"web_url":"https://patchwork.libcamera.org/comment/97/","msgid":"<20181228142648.GA6300@uno.localdomain>","date":"2018-12-28T14:26:48","subject":"Re: [libcamera-devel] [PATCH RESEND v2 1/4] libcamera: Add\n\tMediaObject class hierarchy","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent\n\nOn Fri, Dec 28, 2018 at 03:50:27PM +0200, Laurent Pinchart wrote:\n\n[snip]\n\n> > > > + */\n> > > > +const MediaPad *MediaEntity::getPad(std::function<bool(MediaPad *)> f)\n> > > > +{\n> > > > +\tconst MediaPad *_p = __getPad(sources_, f);\n> > > > +\treturn (_p ? _p : __getPad(sinks_, f));\n> > > > +}\n> >\n> > Maybe just keep a getPad() that goes on the now single pad vector but\n> > still using a predicate to do the matching?\n>\n> If the getPadById() function is reworked to use the mediaObjects_ map,\n> getPadByIndex() can be merged with getPad(), and you won't need a predicate.\n>\n\nI really don't want to break the encapsulation this class hierarchy\nprovides. MediaDevice creates and manages a hierarchy of MediaObjects,\nbut MediaObjects should not go and call into the media device. If it\nhappens there's something fishy in the design and I would like to keep\nthis rule firm. Also the mediaObjects_ array contains all objects, entities,\npads and links, as \"MediaObject *\". I should walk all of it every time\nto match ids.\n\nI'm fine with a single mediaObject_ vector in MediaDevice as a general\npool where to retrieve stuff from by id (in the media device itself), but\nfor this classes I would like to keep the current structure, where\nentities have pads, and pads have links. They should provide accessors\nto those, but that's it, I don't want them to fish from the global\nobject pool.\n\nAm I overthinking it?","headers":{"Return-Path":"<jacopo@jmondi.org>","Received":["from relay12.mail.gandi.net (relay12.mail.gandi.net\n\t[217.70.178.232])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 75DD060B2E\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 28 Dec 2018 15:26: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 relay12.mail.gandi.net (Postfix) with ESMTPSA id 05969200009;\n\tFri, 28 Dec 2018 14:26:51 +0000 (UTC)"],"Date":"Fri, 28 Dec 2018 15:26:48 +0100","From":"Jacopo Mondi <jacopo@jmondi.org>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20181228142648.GA6300@uno.localdomain>","References":"<20181228075743.28637-1-jacopo@jmondi.org>\n\t<7331052.KbqZa54csX@avalon> <20181228131115.GG909@uno.localdomain>\n\t<3401878.rr1as6qmyV@avalon>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"fUYQa+Pmc3FrFX/N\"","Content-Disposition":"inline","In-Reply-To":"<3401878.rr1as6qmyV@avalon>","User-Agent":"Mutt/1.11.1 (2018-12-01)","Subject":"Re: [libcamera-devel] [PATCH RESEND v2 1/4] libcamera: Add\n\tMediaObject class hierarchy","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Fri, 28 Dec 2018 14:26:52 -0000"}}]