[{"id":141,"web_url":"https://patchwork.libcamera.org/comment/141/","msgid":"<20181230215015.GF31866@bigcity.dyn.berto.se>","date":"2018-12-30T21:50:15","subject":"Re: [libcamera-devel] [PATCH v3 1/3] libcamera: Add MediaObject\n\tclass hierarchy","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Jacopo,\n\nThanks for your patch.\n\nOn 2018-12-30 15:23:12 +0100, Jacopo Mondi wrote:\n> Add a class hierarcy to represent all media objects a media graph represents.\n> Add a base MediaObject class, which retains the global unique object id,\n> and define the derived MediaEntity, MediaLink and MediaPad 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 \n> ---\n> src/libcamera/include/media_object.h | 107 ++++++++++\n> src/libcamera/media_object.cpp | 281 +++++++++++++++++++++++++++\n> src/libcamera/meson.build | 2 +\n> 3 files changed, 390 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 b/src/libcamera/include/media_object.h\n> new file mode 100644\n> index 0000000..118d0d8\n> --- /dev/null\n> +++ b/src/libcamera/include/media_object.h\n> @@ -0,0 +1,107 @@\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 \n> +#include \n> +\n> +#include \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 MediaPad;\n> +class MediaLink : public MediaObject\n> +{\n> +\tfriend class MediaDevice;\n> +\n> +public:\n> +\t~MediaLink() { }\n> +\n> +\tMediaPad *source() const { return source_; }\n> +\tMediaPad *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> +\t\t MediaPad *source, MediaPad *sink);\n\nWhy not use references here? Would it be valid to create a MediaLink \nwhere any of the parameters are nullptr?\n\nSame question for other constructors in this patch.\n\n> +\tMediaLink(const MediaLink &) = delete;\n> +\n> +\tMediaPad *source_;\n> +\tMediaPad *sink_;\n> +\tunsigned int flags_;\n> +};\n> +\n> +class MediaEntity;\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> +\tMediaEntity *entity() const { return entity_; }\n> +\tunsigned int flags() const { return flags_; }\n> +\tconst std::vector &links() const { return links_; }\n> +\n> +\tvoid addLink(MediaLink *link);\n\nSame here why not use a possibly const reference?\n\n> +\n> +private:\n> +\tMediaPad(const struct media_v2_pad *pad, MediaEntity *entity);\n> +\tMediaPad(const MediaPad &) = delete;\n> +\n> +\tunsigned int index_;\n> +\tMediaEntity *entity_;\n> +\tunsigned int flags_;\n> +\n> +\tstd::vector links_;\n> +};\n> +\n> +class MediaEntity : public MediaObject\n> +{\n> +\tfriend class MediaDevice;\n> +\n> +public:\n> +\tconst std::string &name() const { return name_; }\n> +\n> +\tconst std::vector &pads() { return pads_; }\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 pads_;\n> +\n> +\tvoid addPad(MediaPad *pad);\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..f0906e8\n> --- /dev/null\n> +++ b/src/libcamera/media_object.cpp\n> @@ -0,0 +1,281 @@\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 \n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +#include \n> +\n> +#include \n> +\n> +#include \"log.h\"\n> +#include \"media_object.h\"\n> +\n> +/**\n> + * \\file media_object.h\n> + *\n> + * In the media object file is implemented a class hierarchy that\n\ns/In the media object file is implemented a/Provide a/\n\n> + * represent the counterpart of media objects exposed by the kernel's\n\ns/counterpart of/corresponding/\n\n> + * media controller APIs.\n> + *\n> + * Media Objects here represented are media entities, media pads and\n\ns/here represented/represented here/\n\n> + * media links, created with informations as obtained by the\n> + * MEDIA_IOC_G_TOPOLOGY ioctl call.\n> + *\n> + * MediaLink, MediaPad and MediaEntity are derived classes of the virtual\n> + * base class MediaObject, which maintains a globally unique id for each object.\n> + *\n> + * Media objects have private constructors to restrict the number of classes\n> + * that can instantiate them only to the 'friend' MediaDevice one. Media\n> + * objects are in facts created when a MediaDevice gets populated.\n> + */\n> +\n> +namespace libcamera {\n> +\n> +/**\n> + * \\class MediaObject\n> + * \\brief Base class for all media object types\n> + *\n> + * MediaObject is the virtual base class for all media objects in the\n> + * media graph. Each object has a globally unique id assigned, and this\n> + * base class provides that.\n> + *\n> + * MediaEntities, MediaPads and MediaLink are MediaObject derived classes.\n> + */\n> +\n> +/**\n> + * \\fn MediaObject::MediaObject()\n> + * \\brief Construct a MediaObject with \\a id\n> + * \\param id The globally unique object id\n> + */\n> +\n> +/**\n> + * \\fn MediaObject::id()\n> + * \\brief Return the object's globally unique id\n\nOnly asking if the \\return key word should not be used what is returned \nby the function and \\brief describing the action performed.?\n\n\\brief Retrieve the media objects global id\n\\returns Media objects global id\n\nI know it seems silly for such a simple function but still I like the \n\\return keyword as it's easy to spot when reading the documentation.\n\nSame comment on all functions returning something which is described \nusing the \\brief keyword.\n\n> + */\n> +\n> +/**\n> + * \\var MediaObject::id_\n> + * \\brief The MediaObject unique id\n> + */\n\nShould we document private variables which do not require special \nexplanations for one reason or another?\n\n> +\n> +/**\n> + * \\class MediaLink\n> + * \\brief Media Link object\n> + *\n> + * A MediaLink represents a 'struct media_v2_link', with associated\n> + * references to the MediaPad it connects and an internal status defined\n> + * by the MEDIA_LNK_FL_* flags captured in flags_.\n> + *\n> + * MediaLink can be enabled enabled and disabled, with the exception of\n\ns/enabled enabled/enabled/\n\n> + * immutable links (see MEDIA_LNK_FL_IMMUTABLE).\n> + *\n> + * MediaLinks are created between MediaPads inspecting the media graph\n> + * topology and are stored in both the source and sink MediaPad they\n> + * connect.\n> + */\n> +\n> +/**\n> + * \\brief Construct a MediaLink\n> + * \\param link The media link representation\n> + * \\param source The MediaPad source\n> + * \\param sink The MediaPad sink\n> + */\n> +MediaLink::MediaLink(const struct media_v2_link *link, MediaPad *source,\n> +\t\t MediaPad *sink)\n> +\t: MediaObject(link->id), source_(source),\n> +\t sink_(sink), flags_(link->flags)\n> +{\n> +}\n> +\n> +/**\n> + * \\fn MediaLink::source()\n> + * \\brief Return the source MediaPad\n> + */\n> +\n> +/**\n> + * \\fn MediaLink::sink()\n> + * \\brief Return the sink MediaPad\n> + */\n> +\n> +/**\n> + * \\fn MediaLink::flags()\n> + * \\brief Return the link flags\n> + */\n\nIs it useful to add documentation for these trivial getters?\n\n> +\n> +/**\n> + * \\fn MediaLink::setFlags()\n> + * \\brief Set the link flags to \\a flags\n> + * \\param flags The flags to be applied to the link\n> + */\n\nI'm not sure why this function is need? I can find no references to it \non the rest of this series. Maybe it's used by code yet to be posted. \nCould you drop it from this series or expand the documentation to it's \nintended purpose?\n\n> +\n> +/**\n> + * \\class MediaPad\n> + * \\brief Media Pad object\n> + *\n> + * MediaPads represent a 'struct media_v2_pad', with associated a list of\n\ns/associated a/associated/\n\n> + * links and a reference to the entity they belong to.\n> + *\n> + * MediaPads are connected to other MediaPads by MediaLinks, created\n> + * inspecting the media graph topology. Data connection between pads can be\n> + * enabled or disabled operating on the link that connects the two. See\n> + * MediaLink.\n> + *\n> + * MediaPads are either 'source' pads, or 'sink' pads. This information is\n> + * captured by the MEDIA_PAD_FL_* flags as stored in the flags_ member\n> + * variable.\n> + *\n> + * Each MediaPad has a list of MediaLinks it is associated to. All links\n> + * in a source pad are outbound links, while all links in a sink pad are\n> + * inbound ones.\n> + *\n> + * Each MediaPad has a reference to the MediaEntity it belongs to.\n> + */\n> +\n> +/**\n> + * \\brief Create a MediaPad\n> + * \\param pad The media pad representationo\n\nMaybe make it clear that it's the kernels representation of the pad and \nnot the libraries?\n\n> + * \\param entity The MediaEntity this pad belongs to\n> + */\n> +MediaPad::MediaPad(const struct media_v2_pad *pad, MediaEntity *entity)\n> +\t: MediaObject(pad->id), index_(pad->index), entity_(entity),\n> +\t flags_(pad->flags)\n> +{\n> +}\n> +\n> +/**\n> + * \\brief Delete pad.\n> + *\n> + * Delete the pad by deleting its media links. As a reference to the same\n> + * MediaLink gets stored in both the source and the sink pad, only source\n> + * ones actually delete the MediaLink.\n\nCan this create race conditions? If the source pad is deleted before the \nsink there is a dangling pointer alive. Would reference counting be \nneeded here or is this a none issue?\n\nI'm thinking of when we get hotplug support and entities in the media \ngraph can come and go. If the entity proving the source pad is removed \nthe link would be deleted while the sink pad is still alive and pointing \nto the now deleted link.\n\nCan be addressed on top of this if deemed at all to be a issue.\n\n> + */\n> +MediaPad::~MediaPad()\n> +{\n> +\tfor (MediaLink *l : links_)\n> +\t\tif (flags_ & MEDIA_PAD_FL_SOURCE)\n> +\t\t\tdelete l;\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 MediaEntity 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 links in this pad.\n> + */\n\nIs it useful to document these trivial getters?\n\n> +\n> +/**\n> + * \\brief Add a new link to this pad.\n\nYou have not ended other \\brief with a period before. Not sure what is \nbest, but it should be the same.\n\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\n> + *\n> + * A MediaEntity represents a 'struct media_v2_entity' with an associated\n> + * list of MediaPads it contains.\n> + *\n> + * Entities are created inspecting the media graph topology, and MediaPads\n> + * gets added as they are discovered.\n> + *\n> + * TODO: Add support for associating a devnode to the entity when integrating\n> + * with DeviceEnumerator.\n> + */\n> +\n> +/**\n> + * \\fn MediaEntity::name()\n> + * \\brief Return the entity name\n> + */\n> +\n> +/**\n> + * \\fn MediaEntity::pads()\n> + * \\brief Return all pads in this entity\n> + */\n\nSame as above.\n\n> +\n> +/**\n> + * \\fn MediaEntity::getPadByIndex(unsigned int index)\n> + * \\brief Get a pad in this entity by its index\n> + * \\return The pad with index \\a index\n> + * \\return nullptr if no pad is found at \\index\n\n@s@\\index@\\a index@\n\n> + */\n> +const MediaPad *MediaEntity::getPadByIndex(unsigned int index)\n> +{\n> +\tfor (MediaPad *p : pads_)\n> +\t\tif (p->index() == index)\n> +\t\t\treturn p;\n> +\n> +\treturn nullptr;\n> +}\n> +\n> +/**\n> + * \\brief Get a pad in this entity by its id\n\nI would add the words 'globally unique id' in the \\brief\n\n> + * \\param id The pad globally unique id\n> + * \\return The pad with id \\a id\n> + * \\return nullptr if not pad with \\id is found\n\n@s@\\id@\\a id@\n\n> + */\n> +const MediaPad *MediaEntity::getPadById(unsigned int id)\n> +{\n> +\tfor (MediaPad *p : pads_)\n> +\t\tif (p->id() == id)\n> +\t\t\treturn p;\n> +\n> +\treturn nullptr;\n> +}\n> +\n> +/**\n> + * \\brief Construct a MediaEntity\n> + * \\param entity The media entity representation\n\nSame as for MediaPad::MediaPad, mention the parameter is the kernels \nrepresentation.\n\n> + */\n> +MediaEntity::MediaEntity(const struct media_v2_entity *entity)\n> +\t: MediaObject(entity->id), name_(entity->name)\n> +{\n> +}\n> +\n> +/**\n> + * \\fn MediaEntity::~MediaEntity()\n> + * \\brief Delete all pads in the entity\n\nHow about:\n\n\\brief: Cleans up all resources allocated by the MediaEntity\n\n> + */\n> +MediaEntity::~MediaEntity()\n> +{\n> +\tfor (MediaPad *p : pads_)\n> +\t\tdelete p;\n> +\tpads_.clear();\n> +}\n> +\n> +/**\n> + * \\brief Add \\a pad to list of entity's pads\n> + * \\param pad The pad to add\n> + */\n> +void MediaEntity::addPad(MediaPad *pad)\n> +{\n> +\tpads_.push_back(pad);\n> +}\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index f632eb5..01d321c 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -1,10 +1,12 @@\n> libcamera_sources = files([\n> 'log.cpp',\n> 'main.cpp',\n> + 'media_object.cpp',\n> ])\n> \n> libcamera_headers = files([\n> 'include/log.h',\n> + 'include/media_object.h',\n> 'include/utils.h',\n> ])\n> \n> -- \n> 2.20.1\n> \n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","Received":["from mail-lj1-x241.google.com (mail-lj1-x241.google.com\n\t[IPv6:2a00:1450:4864:20::241])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 20D75600CC\n\tfor ;\n\tSun, 30 Dec 2018 22:50:18 +0100 (CET)","by mail-lj1-x241.google.com with SMTP id t18-v6so22585901ljd.4\n\tfor ;\n\tSun, 30 Dec 2018 13:50:18 -0800 (PST)","from localhost (89-233-230-99.cust.bredband2.com. [89.233.230.99])\n\tby smtp.gmail.com with ESMTPSA id\n\tu79-v6sm10852904lje.36.2018.12.30.13.50.16\n\t(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);\n\tSun, 30 Dec 2018 13:50:16 -0800 (PST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to\n\t:user-agent; bh=mXJCssUGO/CO1kp6+pyXfQqtiHm8ZnNkBZ4EZYqYQ9A=;\n\tb=UmDGBiLj5wO2jZ2OG+K+3skZglo5tP5eiGfgKErTqKQci5+uhzhzmuOLsC/lSYm3sp\n\tVTmqEk3g6z0f9HMkYb2dRGqnrSIhixEgGlqBbukrLeGRXZbA3Yjj5FS8UqrX9s6yDXTg\n\tFClNLoVtjaz9d2zvE5A2lZO+16IxCKRMy56QZLCqV8ROtnO3YNKD4nWkVaDKTr06ghe3\n\ttH6VhDWh/38KwBvtryJlbF6G2M/u3Sewy/nVEX+6g6frGNBmM8RCC1/sXoqk0JO/Y3Eb\n\t0hlfP6pMMpZFEvd6X+b2QIzsIAcG8HJB9NOhsmNGe+dZPMk7Ue+wxNOblwnDjU7nGcIC\n\tIJjg==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to:user-agent;\n\tbh=mXJCssUGO/CO1kp6+pyXfQqtiHm8ZnNkBZ4EZYqYQ9A=;\n\tb=SeZb5Ot8gHFGiR+q2S+Wett0C7BsZmcVT6uZxDrW8oL/XmDvKGoQAmu9jdmghz+GEK\n\tg4FoCB1S4F2M0wco9/KYSFa32IEoubTM/ueAvHV66PXCtidvbb2s9yFmUfsbA10Jh6jx\n\tOAvMBxarc6zfIIWvXGwdUaOUyCpljssjjkajzI2nQcBDdNfVKrpJszbQvILd/Pfu270m\n\tQpRu+RxOycEQ/PG7Z2NFC2hDAsmERk1HnQPMqMDehR7l8pCGG+2tFwiEys5Ce5TdztDK\n\tcirOEtS3cj13TeIkMXEH0M84bvodEmSnRjXRjkfcD0LfSj4acsaBKoVEGAfKZAiZjjg8\n\tt3Eg==","X-Gm-Message-State":"AJcUukcs55QsuQNktu5+D0aCMQO62PBFRvcQXfpuBX/Jk6ho/SWMD549\n\tzoItNQfej1xW2gsQDGQQJBn4Mw==","X-Google-Smtp-Source":"ALg8bN7Cd0f6eMa4AnVfz7vQz8gOAxporSvGlZs9p2FIC80qW50P4aQVFMPaOK2Ck0RNo1grxiBzPw==","X-Received":"by 2002:a2e:2b85:: with SMTP id\n\tr5-v6mr19581659ljr.91.1546206617283; \n\tSun, 30 Dec 2018 13:50:17 -0800 (PST)","Date":"Sun, 30 Dec 2018 22:50:15 +0100","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20181230215015.GF31866@bigcity.dyn.berto.se>","References":"<20181230142314.16263-1-jacopo@jmondi.org>\n\t<20181230142314.16263-2-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20181230142314.16263-2-jacopo@jmondi.org>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v3 1/3] libcamera: Add MediaObject\n\tclass hierarchy","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":"Sun, 30 Dec 2018 21:50:18 -0000"}},{"id":142,"web_url":"https://patchwork.libcamera.org/comment/142/","msgid":"<20181230221057.ckxmjw52nvjveu4s@uno.localdomain>","date":"2018-12-30T22:10:57","subject":"Re: [libcamera-devel] [PATCH v3 1/3] libcamera: Add MediaObject\n\tclass hierarchy","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n thanks for review\n\nOn Sun, Dec 30, 2018 at 10:50:15PM +0100, Niklas Söderlund wrote:\n> Hi Jacopo,\n>\n> Thanks for your patch.\n>\n> On 2018-12-30 15:23:12 +0100, Jacopo Mondi wrote:\n> > Add a class hierarcy to represent all media objects a media graph represents.\n> > Add a base MediaObject class, which retains the global unique object id,\n> > and define the derived MediaEntity, MediaLink and MediaPad 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 \n> > ---\n> > src/libcamera/include/media_object.h | 107 ++++++++++\n> > src/libcamera/media_object.cpp | 281 +++++++++++++++++++++++++++\n> > src/libcamera/meson.build | 2 +\n> > 3 files changed, 390 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 b/src/libcamera/include/media_object.h\n> > new file mode 100644\n> > index 0000000..118d0d8\n> > --- /dev/null\n> > +++ b/src/libcamera/include/media_object.h\n> > @@ -0,0 +1,107 @@\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 \n> > +#include \n> > +\n> > +#include \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 MediaPad;\n> > +class MediaLink : public MediaObject\n> > +{\n> > +\tfriend class MediaDevice;\n> > +\n> > +public:\n> > +\t~MediaLink() { }\n> > +\n> > +\tMediaPad *source() const { return source_; }\n> > +\tMediaPad *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> > +\t\t MediaPad *source, MediaPad *sink);\n>\n> Why not use references here? Would it be valid to create a MediaLink\n> where any of the parameters are nullptr?\n\nI feel it is better to use pointers for dynamically allocated objects\nas this class entities are. The caller will have pointers to them, and\nI should dereference them to access their value and pass it by\nreference.\n\nAs I've replied to your series, I have not found this stated anywhere,\nso it might just be my preference, but I agree this protects us from\nnullptr values.\n\n>\n> Same question for other constructors in this patch.\n>\n\nCould I keep them like this until we don't try establish a firm rule?\n\n> > +\tMediaLink(const MediaLink &) = delete;\n> > +\n> > +\tMediaPad *source_;\n> > +\tMediaPad *sink_;\n> > +\tunsigned int flags_;\n> > +};\n> > +\n> > +class MediaEntity;\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> > +\tMediaEntity *entity() const { return entity_; }\n> > +\tunsigned int flags() const { return flags_; }\n> > +\tconst std::vector &links() const { return links_; }\n> > +\n> > +\tvoid addLink(MediaLink *link);\n>\n> Same here why not use a possibly const reference?\n>\n> > +\n> > +private:\n> > +\tMediaPad(const struct media_v2_pad *pad, MediaEntity *entity);\n> > +\tMediaPad(const MediaPad &) = delete;\n> > +\n> > +\tunsigned int index_;\n> > +\tMediaEntity *entity_;\n> > +\tunsigned int flags_;\n> > +\n> > +\tstd::vector links_;\n> > +};\n> > +\n> > +class MediaEntity : public MediaObject\n> > +{\n> > +\tfriend class MediaDevice;\n> > +\n> > +public:\n> > +\tconst std::string &name() const { return name_; }\n> > +\n> > +\tconst std::vector &pads() { return pads_; }\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 pads_;\n> > +\n> > +\tvoid addPad(MediaPad *pad);\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..f0906e8\n> > --- /dev/null\n> > +++ b/src/libcamera/media_object.cpp\n> > @@ -0,0 +1,281 @@\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 \n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +\n> > +#include \"log.h\"\n> > +#include \"media_object.h\"\n> > +\n> > +/**\n> > + * \\file media_object.h\n> > + *\n> > + * In the media object file is implemented a class hierarchy that\n>\n> s/In the media object file is implemented a/Provide a/\n>\n> > + * represent the counterpart of media objects exposed by the kernel's\n>\n> s/counterpart of/corresponding/\n>\n> > + * media controller APIs.\n> > + *\n> > + * Media Objects here represented are media entities, media pads and\n>\n> s/here represented/represented here/\n>\n\nThanks\n\n> > + * media links, created with informations as obtained by the\n> > + * MEDIA_IOC_G_TOPOLOGY ioctl call.\n> > + *\n> > + * MediaLink, MediaPad and MediaEntity are derived classes of the virtual\n> > + * base class MediaObject, which maintains a globally unique id for each object.\n> > + *\n> > + * Media objects have private constructors to restrict the number of classes\n> > + * that can instantiate them only to the 'friend' MediaDevice one. Media\n> > + * objects are in facts created when a MediaDevice gets populated.\n> > + */\n> > +\n> > +namespace libcamera {\n> > +\n> > +/**\n> > + * \\class MediaObject\n> > + * \\brief Base class for all media object types\n> > + *\n> > + * MediaObject is the virtual base class for all media objects in the\n> > + * media graph. Each object has a globally unique id assigned, and this\n> > + * base class provides that.\n> > + *\n> > + * MediaEntities, MediaPads and MediaLink are MediaObject derived classes.\n> > + */\n> > +\n> > +/**\n> > + * \\fn MediaObject::MediaObject()\n> > + * \\brief Construct a MediaObject with \\a id\n> > + * \\param id The globally unique object id\n> > + */\n> > +\n> > +/**\n> > + * \\fn MediaObject::id()\n> > + * \\brief Return the object's globally unique id\n>\n> Only asking if the \\return key word should not be used what is returned\n> by the function and \\brief describing the action performed.?\n>\n> \\brief Retrieve the media objects global id\n> \\returns Media objects global id\n>\n> I know it seems silly for such a simple function but still I like the\n> \\return keyword as it's easy to spot when reading the documentation.\n>\n> Same comment on all functions returning something which is described\n> using the \\brief keyword.\n>\n\nI agree on both points, it's better, but still silly for such simply\nmethods :)\n\n> > + */\n> > +\n> > +/**\n> > + * \\var MediaObject::id_\n> > + * \\brief The MediaObject unique id\n> > + */\n>\n> Should we document private variables which do not require special\n> explanations for one reason or another?\n>\n\nIt's protected, I think it's worth documenting it (doxygen wants it to\nbe commented...)\n\n> > +\n> > +/**\n> > + * \\class MediaLink\n> > + * \\brief Media Link object\n> > + *\n> > + * A MediaLink represents a 'struct media_v2_link', with associated\n> > + * references to the MediaPad it connects and an internal status defined\n> > + * by the MEDIA_LNK_FL_* flags captured in flags_.\n> > + *\n> > + * MediaLink can be enabled enabled and disabled, with the exception of\n>\n> s/enabled enabled/enabled/\n>\n> > + * immutable links (see MEDIA_LNK_FL_IMMUTABLE).\n> > + *\n> > + * MediaLinks are created between MediaPads inspecting the media graph\n> > + * topology and are stored in both the source and sink MediaPad they\n> > + * connect.\n> > + */\n> > +\n> > +/**\n> > + * \\brief Construct a MediaLink\n> > + * \\param link The media link representation\n> > + * \\param source The MediaPad source\n> > + * \\param sink The MediaPad sink\n> > + */\n> > +MediaLink::MediaLink(const struct media_v2_link *link, MediaPad *source,\n> > +\t\t MediaPad *sink)\n> > +\t: MediaObject(link->id), source_(source),\n> > +\t sink_(sink), flags_(link->flags)\n> > +{\n> > +}\n> > +\n> > +/**\n> > + * \\fn MediaLink::source()\n> > + * \\brief Return the source MediaPad\n> > + */\n> > +\n> > +/**\n> > + * \\fn MediaLink::sink()\n> > + * \\brief Return the sink MediaPad\n> > + */\n> > +\n> > +/**\n> > + * \\fn MediaLink::flags()\n> > + * \\brief Return the link flags\n> > + */\n>\n> Is it useful to add documentation for these trivial getters?\n>\n\nIf doxygen compiler is happy, I'm happy :)\nI agree going forward we could relax this, but since I already wrote\nit I would keep it here.\n\n> > +\n> > +/**\n> > + * \\fn MediaLink::setFlags()\n> > + * \\brief Set the link flags to \\a flags\n> > + * \\param flags The flags to be applied to the link\n> > + */\n>\n> I'm not sure why this function is need? I can find no references to it\n> on the rest of this series. Maybe it's used by code yet to be posted.\n> Could you drop it from this series or expand the documentation to it's\n> intended purpose?\n>\n\nLeftover and not used here, you're right. I'll drop.\n\n> > +\n> > +/**\n> > + * \\class MediaPad\n> > + * \\brief Media Pad object\n> > + *\n> > + * MediaPads represent a 'struct media_v2_pad', with associated a list of\n>\n> s/associated a/associated/\n>\n> > + * links and a reference to the entity they belong to.\n> > + *\n> > + * MediaPads are connected to other MediaPads by MediaLinks, created\n> > + * inspecting the media graph topology. Data connection between pads can be\n> > + * enabled or disabled operating on the link that connects the two. See\n> > + * MediaLink.\n> > + *\n> > + * MediaPads are either 'source' pads, or 'sink' pads. This information is\n> > + * captured by the MEDIA_PAD_FL_* flags as stored in the flags_ member\n> > + * variable.\n> > + *\n> > + * Each MediaPad has a list of MediaLinks it is associated to. All links\n> > + * in a source pad are outbound links, while all links in a sink pad are\n> > + * inbound ones.\n> > + *\n> > + * Each MediaPad has a reference to the MediaEntity it belongs to.\n> > + */\n> > +\n> > +/**\n> > + * \\brief Create a MediaPad\n> > + * \\param pad The media pad representationo\n>\n> Maybe make it clear that it's the kernels representation of the pad and\n> not the libraries?\n\n\"The kernel media_v2_pad representation\" ?\nAnd I've miss-spelled representationo\n\n>\n> > + * \\param entity The MediaEntity this pad belongs to\n> > + *\n> > +MediaPad::MediaPad(const struct media_v2_pad *pad, MediaEntity *entity)\n> > +\t: MediaObject(pad->id), index_(pad->index), entity_(entity),\n> > +\t flags_(pad->flags)\n> > +{\n> > +}\n> > +\n> > +/**\n> > + * \\brief Delete pad.\n> > + *\n> > + * Delete the pad by deleting its media links. As a reference to the same\n> > + * MediaLink gets stored in both the source and the sink pad, only source\n> > + * ones actually delete the MediaLink.\n>\n> Can this create race conditions? If the source pad is deleted before the\n> sink there is a dangling pointer alive. Would reference counting be\n> needed here or is this a none issue?\n>\n> I'm thinking of when we get hotplug support and entities in the media\n> graph can come and go. If the entity proving the source pad is removed\n> the link would be deleted while the sink pad is still alive and pointing\n> to the now deleted link.\n>\n> Can be addressed on top of this if deemed at all to be a issue.\n>\n\nI do expect we handle link deletion as well when adding hot-plug support.\nFor now, I don't think it's an issue though.\n\n> > + */\n> > +MediaPad::~MediaPad()\n> > +{\n> > +\tfor (MediaLink *l : links_)\n> > +\t\tif (flags_ & MEDIA_PAD_FL_SOURCE)\n> > +\t\t\tdelete l;\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 MediaEntity 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 links in this pad.\n> > + */\n>\n> Is it useful to document these trivial getters?\n>\n> > +\n> > +/**\n> > + * \\brief Add a new link to this pad.\n>\n> You have not ended other \\brief with a period before. Not sure what is\n> best, but it should be the same.\n\nRight, not sure what to do. I'll drop dots, as you've not been using\nthem in your series.\n\n>\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\n> > + *\n> > + * A MediaEntity represents a 'struct media_v2_entity' with an associated\n> > + * list of MediaPads it contains.\n> > + *\n> > + * Entities are created inspecting the media graph topology, and MediaPads\n> > + * gets added as they are discovered.\n> > + *\n> > + * TODO: Add support for associating a devnode to the entity when integrating\n> > + * with DeviceEnumerator.\n> > + */\n> > +\n> > +/**\n> > + * \\fn MediaEntity::name()\n> > + * \\brief Return the entity name\n> > + */\n> > +\n> > +/**\n> > + * \\fn MediaEntity::pads()\n> > + * \\brief Return all pads in this entity\n> > + */\n>\n> Same as above.\n>\n> > +\n> > +/**\n> > + * \\fn MediaEntity::getPadByIndex(unsigned int index)\n> > + * \\brief Get a pad in this entity by its index\n> > + * \\return The pad with index \\a index\n> > + * \\return nullptr if no pad is found at \\index\n>\n> @s@\\index@\\a index@\n>\n> > + */\n> > +const MediaPad *MediaEntity::getPadByIndex(unsigned int index)\n> > +{\n> > +\tfor (MediaPad *p : pads_)\n> > +\t\tif (p->index() == index)\n> > +\t\t\treturn p;\n> > +\n> > +\treturn nullptr;\n> > +}\n> > +\n> > +/**\n> > + * \\brief Get a pad in this entity by its id\n>\n> I would add the words 'globally unique id' in the \\brief\n>\n> > + * \\param id The pad globally unique id\n> > + * \\return The pad with id \\a id\n> > + * \\return nullptr if not pad with \\id is found\n>\n> @s@\\id@\\a id@\n>\n> > + */\n> > +const MediaPad *MediaEntity::getPadById(unsigned int id)\n> > +{\n> > +\tfor (MediaPad *p : pads_)\n> > +\t\tif (p->id() == id)\n> > +\t\t\treturn p;\n> > +\n> > +\treturn nullptr;\n> > +}\n> > +\n> > +/**\n> > + * \\brief Construct a MediaEntity\n> > + * \\param entity The media entity representation\n>\n> Same as for MediaPad::MediaPad, mention the parameter is the kernels\n> representation.\n>\n> > + */\n> > +MediaEntity::MediaEntity(const struct media_v2_entity *entity)\n> > +\t: MediaObject(entity->id), name_(entity->name)\n> > +{\n> > +}\n> > +\n> > +/**\n> > + * \\fn MediaEntity::~MediaEntity()\n> > + * \\brief Delete all pads in the entity\n>\n> How about:\n>\n> \\brief: Cleans up all resources allocated by the MediaEntity\n>\n\nYes, better (this and all the above comments)\n\n> > + */\n> > +MediaEntity::~MediaEntity()\n> > +{\n> > +\tfor (MediaPad *p : pads_)\n> > +\t\tdelete p;\n> > +\tpads_.clear();\n> > +}\n> > +\n> > +/**\n> > + * \\brief Add \\a pad to list of entity's pads\n> > + * \\param pad The pad to add\n> > + */\n> > +void MediaEntity::addPad(MediaPad *pad)\n> > +{\n> > +\tpads_.push_back(pad);\n> > +}\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> > index f632eb5..01d321c 100644\n> > --- a/src/libcamera/meson.build\n> > +++ b/src/libcamera/meson.build\n> > @@ -1,10 +1,12 @@\n> > libcamera_sources = files([\n> > 'log.cpp',\n> > 'main.cpp',\n> > + 'media_object.cpp',\n> > ])\n> >\n> > libcamera_headers = files([\n> > 'include/log.h',\n> > + 'include/media_object.h',\n> > 'include/utils.h',\n> > ])\n> >\n> > --\n> > 2.20.1\n> >\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel\n>\n> --\n> Regards,\n> Niklas Söderlund","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 21B6D600CC\n\tfor ;\n\tSun, 30 Dec 2018 23:11:06 +0100 (CET)","from uno.localdomain\n\t(host54-51-dynamic.16-87-r.retail.telecomitalia.it [87.16.51.54])\n\t(Authenticated sender: jacopo@jmondi.org)\n\tby relay8-d.mail.gandi.net (Postfix) with ESMTPSA id D6AA81BF203;\n\tSun, 30 Dec 2018 22:11:04 +0000 (UTC)"],"X-Originating-IP":"87.16.51.54","Date":"Sun, 30 Dec 2018 23:10:57 +0100","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20181230221057.ckxmjw52nvjveu4s@uno.localdomain>","References":"<20181230142314.16263-1-jacopo@jmondi.org>\n\t<20181230142314.16263-2-jacopo@jmondi.org>\n\t<20181230215015.GF31866@bigcity.dyn.berto.se>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"rszxv3c5sue7mpj5\"","Content-Disposition":"inline","In-Reply-To":"<20181230215015.GF31866@bigcity.dyn.berto.se>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH v3 1/3] libcamera: Add MediaObject\n\tclass hierarchy","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":"Sun, 30 Dec 2018 22:11:06 -0000"}},{"id":145,"web_url":"https://patchwork.libcamera.org/comment/145/","msgid":"<20181230224756.GA10929@bigcity.dyn.berto.se>","date":"2018-12-30T22:47:56","subject":"Re: [libcamera-devel] [PATCH v3 1/3] libcamera: Add MediaObject\n\tclass hierarchy","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Jacopo,\n\nOn 2018-12-30 23:10:57 +0100, Jacopo Mondi wrote:\n> Hi Niklas,\n> thanks for review\n> \n> On Sun, Dec 30, 2018 at 10:50:15PM +0100, Niklas Söderlund wrote:\n> > Hi Jacopo,\n> >\n> > Thanks for your patch.\n> >\n> > On 2018-12-30 15:23:12 +0100, Jacopo Mondi wrote:\n> > > Add a class hierarcy to represent all media objects a media graph represents.\n> > > Add a base MediaObject class, which retains the global unique object id,\n> > > and define the derived MediaEntity, MediaLink and MediaPad 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 \n> > > ---\n> > > src/libcamera/include/media_object.h | 107 ++++++++++\n> > > src/libcamera/media_object.cpp | 281 +++++++++++++++++++++++++++\n> > > src/libcamera/meson.build | 2 +\n> > > 3 files changed, 390 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 b/src/libcamera/include/media_object.h\n> > > new file mode 100644\n> > > index 0000000..118d0d8\n> > > --- /dev/null\n> > > +++ b/src/libcamera/include/media_object.h\n> > > @@ -0,0 +1,107 @@\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 \n> > > +#include \n> > > +\n> > > +#include \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 MediaPad;\n> > > +class MediaLink : public MediaObject\n> > > +{\n> > > +\tfriend class MediaDevice;\n> > > +\n> > > +public:\n> > > +\t~MediaLink() { }\n> > > +\n> > > +\tMediaPad *source() const { return source_; }\n> > > +\tMediaPad *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> > > +\t\t MediaPad *source, MediaPad *sink);\n> >\n> > Why not use references here? Would it be valid to create a MediaLink\n> > where any of the parameters are nullptr?\n> \n> I feel it is better to use pointers for dynamically allocated objects\n> as this class entities are. The caller will have pointers to them, and\n> I should dereference them to access their value and pass it by\n> reference.\n> \n> As I've replied to your series, I have not found this stated anywhere,\n> so it might just be my preference, but I agree this protects us from\n> nullptr values.\n> \n> >\n> > Same question for other constructors in this patch.\n> >\n> \n> Could I keep them like this until we don't try establish a firm rule?\n\nI have no problem keeping them, but if you want to keep them you should \nensure they are not null before dereferencing them.\n\n> \n> > > +\tMediaLink(const MediaLink &) = delete;\n> > > +\n> > > +\tMediaPad *source_;\n> > > +\tMediaPad *sink_;\n> > > +\tunsigned int flags_;\n> > > +};\n> > > +\n> > > +class MediaEntity;\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> > > +\tMediaEntity *entity() const { return entity_; }\n> > > +\tunsigned int flags() const { return flags_; }\n> > > +\tconst std::vector &links() const { return links_; }\n> > > +\n> > > +\tvoid addLink(MediaLink *link);\n> >\n> > Same here why not use a possibly const reference?\n> >\n> > > +\n> > > +private:\n> > > +\tMediaPad(const struct media_v2_pad *pad, MediaEntity *entity);\n> > > +\tMediaPad(const MediaPad &) = delete;\n> > > +\n> > > +\tunsigned int index_;\n> > > +\tMediaEntity *entity_;\n> > > +\tunsigned int flags_;\n> > > +\n> > > +\tstd::vector links_;\n> > > +};\n> > > +\n> > > +class MediaEntity : public MediaObject\n> > > +{\n> > > +\tfriend class MediaDevice;\n> > > +\n> > > +public:\n> > > +\tconst std::string &name() const { return name_; }\n> > > +\n> > > +\tconst std::vector &pads() { return pads_; }\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 pads_;\n> > > +\n> > > +\tvoid addPad(MediaPad *pad);\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..f0906e8\n> > > --- /dev/null\n> > > +++ b/src/libcamera/media_object.cpp\n> > > @@ -0,0 +1,281 @@\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 \n> > > +#include \n> > > +#include \n> > > +#include \n> > > +\n> > > +#include \n> > > +#include \n> > > +\n> > > +#include \n> > > +\n> > > +#include \"log.h\"\n> > > +#include \"media_object.h\"\n> > > +\n> > > +/**\n> > > + * \\file media_object.h\n> > > + *\n> > > + * In the media object file is implemented a class hierarchy that\n> >\n> > s/In the media object file is implemented a/Provide a/\n> >\n> > > + * represent the counterpart of media objects exposed by the kernel's\n> >\n> > s/counterpart of/corresponding/\n> >\n> > > + * media controller APIs.\n> > > + *\n> > > + * Media Objects here represented are media entities, media pads and\n> >\n> > s/here represented/represented here/\n> >\n> \n> Thanks\n> \n> > > + * media links, created with informations as obtained by the\n> > > + * MEDIA_IOC_G_TOPOLOGY ioctl call.\n> > > + *\n> > > + * MediaLink, MediaPad and MediaEntity are derived classes of the virtual\n> > > + * base class MediaObject, which maintains a globally unique id for each object.\n> > > + *\n> > > + * Media objects have private constructors to restrict the number of classes\n> > > + * that can instantiate them only to the 'friend' MediaDevice one. Media\n> > > + * objects are in facts created when a MediaDevice gets populated.\n> > > + */\n> > > +\n> > > +namespace libcamera {\n> > > +\n> > > +/**\n> > > + * \\class MediaObject\n> > > + * \\brief Base class for all media object types\n> > > + *\n> > > + * MediaObject is the virtual base class for all media objects in the\n> > > + * media graph. Each object has a globally unique id assigned, and this\n> > > + * base class provides that.\n> > > + *\n> > > + * MediaEntities, MediaPads and MediaLink are MediaObject derived classes.\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn MediaObject::MediaObject()\n> > > + * \\brief Construct a MediaObject with \\a id\n> > > + * \\param id The globally unique object id\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn MediaObject::id()\n> > > + * \\brief Return the object's globally unique id\n> >\n> > Only asking if the \\return key word should not be used what is returned\n> > by the function and \\brief describing the action performed.?\n> >\n> > \\brief Retrieve the media objects global id\n> > \\returns Media objects global id\n> >\n> > I know it seems silly for such a simple function but still I like the\n> > \\return keyword as it's easy to spot when reading the documentation.\n> >\n> > Same comment on all functions returning something which is described\n> > using the \\brief keyword.\n> >\n> \n> I agree on both points, it's better, but still silly for such simply\n> methods :)\n> \n> > > + */\n> > > +\n> > > +/**\n> > > + * \\var MediaObject::id_\n> > > + * \\brief The MediaObject unique id\n> > > + */\n> >\n> > Should we document private variables which do not require special\n> > explanations for one reason or another?\n> >\n> \n> It's protected, I think it's worth documenting it (doxygen wants it to\n> be commented...)\n> \n> > > +\n> > > +/**\n> > > + * \\class MediaLink\n> > > + * \\brief Media Link object\n> > > + *\n> > > + * A MediaLink represents a 'struct media_v2_link', with associated\n> > > + * references to the MediaPad it connects and an internal status defined\n> > > + * by the MEDIA_LNK_FL_* flags captured in flags_.\n> > > + *\n> > > + * MediaLink can be enabled enabled and disabled, with the exception of\n> >\n> > s/enabled enabled/enabled/\n> >\n> > > + * immutable links (see MEDIA_LNK_FL_IMMUTABLE).\n> > > + *\n> > > + * MediaLinks are created between MediaPads inspecting the media graph\n> > > + * topology and are stored in both the source and sink MediaPad they\n> > > + * connect.\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\brief Construct a MediaLink\n> > > + * \\param link The media link representation\n> > > + * \\param source The MediaPad source\n> > > + * \\param sink The MediaPad sink\n> > > + */\n> > > +MediaLink::MediaLink(const struct media_v2_link *link, MediaPad *source,\n> > > +\t\t MediaPad *sink)\n> > > +\t: MediaObject(link->id), source_(source),\n> > > +\t sink_(sink), flags_(link->flags)\n> > > +{\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\fn MediaLink::source()\n> > > + * \\brief Return the source MediaPad\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn MediaLink::sink()\n> > > + * \\brief Return the sink MediaPad\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn MediaLink::flags()\n> > > + * \\brief Return the link flags\n> > > + */\n> >\n> > Is it useful to add documentation for these trivial getters?\n> >\n> \n> If doxygen compiler is happy, I'm happy :)\n> I agree going forward we could relax this, but since I already wrote\n> it I would keep it here.\n> \n> > > +\n> > > +/**\n> > > + * \\fn MediaLink::setFlags()\n> > > + * \\brief Set the link flags to \\a flags\n> > > + * \\param flags The flags to be applied to the link\n> > > + */\n> >\n> > I'm not sure why this function is need? I can find no references to it\n> > on the rest of this series. Maybe it's used by code yet to be posted.\n> > Could you drop it from this series or expand the documentation to it's\n> > intended purpose?\n> >\n> \n> Leftover and not used here, you're right. I'll drop.\n> \n> > > +\n> > > +/**\n> > > + * \\class MediaPad\n> > > + * \\brief Media Pad object\n> > > + *\n> > > + * MediaPads represent a 'struct media_v2_pad', with associated a list of\n> >\n> > s/associated a/associated/\n> >\n> > > + * links and a reference to the entity they belong to.\n> > > + *\n> > > + * MediaPads are connected to other MediaPads by MediaLinks, created\n> > > + * inspecting the media graph topology. Data connection between pads can be\n> > > + * enabled or disabled operating on the link that connects the two. See\n> > > + * MediaLink.\n> > > + *\n> > > + * MediaPads are either 'source' pads, or 'sink' pads. This information is\n> > > + * captured by the MEDIA_PAD_FL_* flags as stored in the flags_ member\n> > > + * variable.\n> > > + *\n> > > + * Each MediaPad has a list of MediaLinks it is associated to. All links\n> > > + * in a source pad are outbound links, while all links in a sink pad are\n> > > + * inbound ones.\n> > > + *\n> > > + * Each MediaPad has a reference to the MediaEntity it belongs to.\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\brief Create a MediaPad\n> > > + * \\param pad The media pad representationo\n> >\n> > Maybe make it clear that it's the kernels representation of the pad and\n> > not the libraries?\n> \n> \"The kernel media_v2_pad representation\" ?\n> And I've miss-spelled representationo\n> \n> >\n> > > + * \\param entity The MediaEntity this pad belongs to\n> > > + *\n> > > +MediaPad::MediaPad(const struct media_v2_pad *pad, MediaEntity *entity)\n> > > +\t: MediaObject(pad->id), index_(pad->index), entity_(entity),\n> > > +\t flags_(pad->flags)\n> > > +{\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\brief Delete pad.\n> > > + *\n> > > + * Delete the pad by deleting its media links. As a reference to the same\n> > > + * MediaLink gets stored in both the source and the sink pad, only source\n> > > + * ones actually delete the MediaLink.\n> >\n> > Can this create race conditions? If the source pad is deleted before the\n> > sink there is a dangling pointer alive. Would reference counting be\n> > needed here or is this a none issue?\n> >\n> > I'm thinking of when we get hotplug support and entities in the media\n> > graph can come and go. If the entity proving the source pad is removed\n> > the link would be deleted while the sink pad is still alive and pointing\n> > to the now deleted link.\n> >\n> > Can be addressed on top of this if deemed at all to be a issue.\n> >\n> \n> I do expect we handle link deletion as well when adding hot-plug support.\n> For now, I don't think it's an issue though.\n> \n> > > + */\n> > > +MediaPad::~MediaPad()\n> > > +{\n> > > +\tfor (MediaLink *l : links_)\n> > > +\t\tif (flags_ & MEDIA_PAD_FL_SOURCE)\n> > > +\t\t\tdelete l;\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 MediaEntity 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 links in this pad.\n> > > + */\n> >\n> > Is it useful to document these trivial getters?\n> >\n> > > +\n> > > +/**\n> > > + * \\brief Add a new link to this pad.\n> >\n> > You have not ended other \\brief with a period before. Not sure what is\n> > best, but it should be the same.\n> \n> Right, not sure what to do. I'll drop dots, as you've not been using\n> them in your series.\n> \n> >\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\n> > > + *\n> > > + * A MediaEntity represents a 'struct media_v2_entity' with an associated\n> > > + * list of MediaPads it contains.\n> > > + *\n> > > + * Entities are created inspecting the media graph topology, and MediaPads\n> > > + * gets added as they are discovered.\n> > > + *\n> > > + * TODO: Add support for associating a devnode to the entity when integrating\n> > > + * with DeviceEnumerator.\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn MediaEntity::name()\n> > > + * \\brief Return the entity name\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn MediaEntity::pads()\n> > > + * \\brief Return all pads in this entity\n> > > + */\n> >\n> > Same as above.\n> >\n> > > +\n> > > +/**\n> > > + * \\fn MediaEntity::getPadByIndex(unsigned int index)\n> > > + * \\brief Get a pad in this entity by its index\n> > > + * \\return The pad with index \\a index\n> > > + * \\return nullptr if no pad is found at \\index\n> >\n> > @s@\\index@\\a index@\n> >\n> > > + */\n> > > +const MediaPad *MediaEntity::getPadByIndex(unsigned int index)\n> > > +{\n> > > +\tfor (MediaPad *p : pads_)\n> > > +\t\tif (p->index() == index)\n> > > +\t\t\treturn p;\n> > > +\n> > > +\treturn nullptr;\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\brief Get a pad in this entity by its id\n> >\n> > I would add the words 'globally unique id' in the \\brief\n> >\n> > > + * \\param id The pad globally unique id\n> > > + * \\return The pad with id \\a id\n> > > + * \\return nullptr if not pad with \\id is found\n> >\n> > @s@\\id@\\a id@\n> >\n> > > + */\n> > > +const MediaPad *MediaEntity::getPadById(unsigned int id)\n> > > +{\n> > > +\tfor (MediaPad *p : pads_)\n> > > +\t\tif (p->id() == id)\n> > > +\t\t\treturn p;\n> > > +\n> > > +\treturn nullptr;\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\brief Construct a MediaEntity\n> > > + * \\param entity The media entity representation\n> >\n> > Same as for MediaPad::MediaPad, mention the parameter is the kernels\n> > representation.\n> >\n> > > + */\n> > > +MediaEntity::MediaEntity(const struct media_v2_entity *entity)\n> > > +\t: MediaObject(entity->id), name_(entity->name)\n> > > +{\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\fn MediaEntity::~MediaEntity()\n> > > + * \\brief Delete all pads in the entity\n> >\n> > How about:\n> >\n> > \\brief: Cleans up all resources allocated by the MediaEntity\n> >\n> \n> Yes, better (this and all the above comments)\n> \n> > > + */\n> > > +MediaEntity::~MediaEntity()\n> > > +{\n> > > +\tfor (MediaPad *p : pads_)\n> > > +\t\tdelete p;\n> > > +\tpads_.clear();\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\brief Add \\a pad to list of entity's pads\n> > > + * \\param pad The pad to add\n> > > + */\n> > > +void MediaEntity::addPad(MediaPad *pad)\n> > > +{\n> > > +\tpads_.push_back(pad);\n> > > +}\n> > > +\n> > > +} /* namespace libcamera */\n> > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> > > index f632eb5..01d321c 100644\n> > > --- a/src/libcamera/meson.build\n> > > +++ b/src/libcamera/meson.build\n> > > @@ -1,10 +1,12 @@\n> > > libcamera_sources = files([\n> > > 'log.cpp',\n> > > 'main.cpp',\n> > > + 'media_object.cpp',\n> > > ])\n> > >\n> > > libcamera_headers = files([\n> > > 'include/log.h',\n> > > + 'include/media_object.h',\n> > > 'include/utils.h',\n> > > ])\n> > >\n> > > --\n> > > 2.20.1\n> > >\n> > > _______________________________________________\n> > > libcamera-devel mailing list\n> > > libcamera-devel@lists.libcamera.org\n> > > https://lists.libcamera.org/listinfo/libcamera-devel\n> >\n> > --\n> > Regards,\n> > Niklas Söderlund","headers":{"Return-Path":"","Received":["from mail-lj1-x243.google.com (mail-lj1-x243.google.com\n\t[IPv6:2a00:1450:4864:20::243])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2D1B5600CC\n\tfor ;\n\tSun, 30 Dec 2018 23:47:58 +0100 (CET)","by mail-lj1-x243.google.com with SMTP id q2-v6so22625479lji.10\n\tfor ;\n\tSun, 30 Dec 2018 14:47:58 -0800 (PST)","from localhost (89-233-230-99.cust.bredband2.com. [89.233.230.99])\n\tby smtp.gmail.com with ESMTPSA id\n\tg70-v6sm9938075ljg.92.2018.12.30.14.47.56\n\t(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);\n\tSun, 30 Dec 2018 14:47:56 -0800 (PST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to\n\t:user-agent; bh=J2SAHantuh+Ysthhj3kmUVvXrcmBD6UmcsU6EehkHko=;\n\tb=vF7LYiqbidEAazv10U78b5ZSckQKRJssUk8HUqdxyScvakiHlD5cua4VWLKahDGV8N\n\tBpuR6csR+kuFbMS8Mb9FALM+PktLKZrFKWlGzvqkVy90l1HaaTq1oxGzTEIqe9HiS3o3\n\tWrD6FTjq3o18SZhCXn1P+9/ddLyYdjnoV29d+6vQoEJ5flBxWKdzNatjwzvHkn83zfSw\n\tJLuUrybzwur5HUyxkT0a5KhXY0Mb/eyhNtYA2/js+JvirX0mhTEaKfZ5HHYxfl/lBLUq\n\t0oos6Y4mH07Rqe/lmBFhFtMmy7ozIac2gH0CWw57JnENutToTu0/+FFz/8r/do9d4qgj\n\tkGMw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to:user-agent;\n\tbh=J2SAHantuh+Ysthhj3kmUVvXrcmBD6UmcsU6EehkHko=;\n\tb=A/ss/emTmozGT97LOM+0mOBaUm3UmpWc9+s11zjjXKaX3zJ6nf9drEspK2rzvLViC1\n\thsz+gCuRKeDrOOFjhk/s4nENWgD/vF2FBO0GzJxdLEmh7XXKA8wrmCxNQs6VQKgOEHyZ\n\tfCvhuKwBaY+1Xzxq3Tt1ko6eCSJ/b0g1kkjZaQsm0rpgy7tBcQtcvsaeedxVko7pswmv\n\tbz7KV2k+ziOocW7pmED7kxVuzRJ6+ZZgKJFzFQGu343cQuI/5Ub4UlKO4WgfuHCStbtE\n\taAcxYBShpReFJEv+41UaLG6KR8TWo6M/Dh9cUQabzUQhouNUeA88GvIt1vZgoefBGrea\n\tDycw==","X-Gm-Message-State":"AJcUukdnCBDDk7/XZcxM1+Zy1ZAvM4QMRbmLtH9giz5DRl4oWDke+jk+\n\tLrRZvYIIygvKdfYVL1jMokCcHA==","X-Google-Smtp-Source":"ALg8bN4G8jyoAeFsSFpDQQDTr9yM8wy0dfOqpatJWY4hjXrrqEdK5LKsekJwYAj8DONaxO7RhFg9kg==","X-Received":"by 2002:a2e:9957:: with SMTP id\n\tr23-v6mr19303524ljj.98.1546210077347; \n\tSun, 30 Dec 2018 14:47:57 -0800 (PST)","Date":"Sun, 30 Dec 2018 23:47:56 +0100","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20181230224756.GA10929@bigcity.dyn.berto.se>","References":"<20181230142314.16263-1-jacopo@jmondi.org>\n\t<20181230142314.16263-2-jacopo@jmondi.org>\n\t<20181230215015.GF31866@bigcity.dyn.berto.se>\n\t<20181230221057.ckxmjw52nvjveu4s@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20181230221057.ckxmjw52nvjveu4s@uno.localdomain>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v3 1/3] libcamera: Add MediaObject\n\tclass hierarchy","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":"Sun, 30 Dec 2018 22:47:58 -0000"}},{"id":152,"web_url":"https://patchwork.libcamera.org/comment/152/","msgid":"<5999023.SlpS090CIt@avalon>","date":"2018-12-31T00:39:54","subject":"Re: [libcamera-devel] [PATCH v3 1/3] libcamera: Add MediaObject\n\tclass 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 Sunday, 30 December 2018 16:23:12 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 \n> ---\n> src/libcamera/include/media_object.h | 107 ++++++++++\n> src/libcamera/media_object.cpp | 281 +++++++++++++++++++++++++++\n> src/libcamera/meson.build | 2 +\n> 3 files changed, 390 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..118d0d8\n> --- /dev/null\n> +++ b/src/libcamera/include/media_object.h\n> @@ -0,0 +1,107 @@\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 \n> +#include \n> +\n> +#include \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 MediaPad;\n> +class MediaLink : public MediaObject\n> +{\n> +\tfriend class MediaDevice;\n> +\n> +public:\n> +\t~MediaLink() { }\n> +\n> +\tMediaPad *source() const { return source_; }\n> +\tMediaPad *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> +\t\t MediaPad *source, MediaPad *sink);\n> +\tMediaLink(const MediaLink &) = delete;\n> +\n> +\tMediaPad *source_;\n> +\tMediaPad *sink_;\n> +\tunsigned int flags_;\n> +};\n> +\n> +class MediaEntity;\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> +\tMediaEntity *entity() const { return entity_; }\n> +\tunsigned int flags() const { return flags_; }\n> +\tconst std::vector &links() const { return links_; }\n> +\n> +\tvoid addLink(MediaLink *link);\n> +\n> +private:\n> +\tMediaPad(const struct media_v2_pad *pad, MediaEntity *entity);\n> +\tMediaPad(const MediaPad &) = delete;\n> +\n> +\tunsigned int index_;\n> +\tMediaEntity *entity_;\n> +\tunsigned int flags_;\n> +\n> +\tstd::vector links_;\n> +};\n> +\n> +class MediaEntity : public MediaObject\n> +{\n> +\tfriend class MediaDevice;\n> +\n> +public:\n> +\tconst std::string &name() const { return name_; }\n> +\n> +\tconst std::vector &pads() { return pads_; }\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 pads_;\n> +\n> +\tvoid addPad(MediaPad *pad);\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..f0906e8\n> --- /dev/null\n> +++ b/src/libcamera/media_object.cpp\n> @@ -0,0 +1,281 @@\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 \n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +#include \n> +\n> +#include \n> +\n> +#include \"log.h\"\n> +#include \"media_object.h\"\n> +\n> +/**\n> + * \\file media_object.h\n> + *\n> + * In the media object file is implemented a class hierarchy that\n> + * represent the counterpart of media objects exposed by the kernel's\n> + * media controller APIs.\n> + *\n> + * Media Objects here represented are media entities, media pads and\n> + * media links, created with informations as obtained by the\n> + * MEDIA_IOC_G_TOPOLOGY ioctl call.\n> + *\n> + * MediaLink, MediaPad and MediaEntity are derived classes of the virtual\n> + * base class MediaObject, which maintains a globally unique id for each\n> object. + *\n> + * Media objects have private constructors to restrict the number of\n> classes + * that can instantiate them only to the 'friend' MediaDevice one.\n> Media + * objects are in facts created when a MediaDevice gets populated. +\n> */\n> +\n> +namespace libcamera {\n> +\n> +/**\n> + * \\class MediaObject\n> + * \\brief Base class for all media object types\n> + *\n> + * MediaObject is the virtual base class for all media objects in the\n> + * media graph. Each object has a globally unique id assigned, and this\n> + * base class provides that.\n> + *\n> + * MediaEntities, MediaPads and MediaLink are MediaObject derived classes.\n> + */\n> +\n> +/**\n> + * \\fn MediaObject::MediaObject()\n> + * \\brief Construct a MediaObject with \\a id\n> + * \\param id The globally unique object id\n> + */\n> +\n> +/**\n> + * \\fn MediaObject::id()\n> + * \\brief Return the object's globally unique id\n> + */\n> +\n> +/**\n> + * \\var MediaObject::id_\n> + * \\brief The MediaObject unique id\n> + */\n> +\n> +/**\n> + * \\class MediaLink\n> + * \\brief Media Link object\n> + *\n> + * A MediaLink represents a 'struct media_v2_link', with associated\n> + * references to the MediaPad it connects and an internal status defined\n> + * by the MEDIA_LNK_FL_* flags captured in flags_.\n> + *\n> + * MediaLink can be enabled enabled and disabled, with the exception of\n> + * immutable links (see MEDIA_LNK_FL_IMMUTABLE).\n> + *\n> + * MediaLinks are created between MediaPads inspecting the media graph\n> + * topology and are stored in both the source and sink MediaPad they\n> + * connect.\n> + */\n> +\n> +/**\n> + * \\brief Construct a MediaLink\n> + * \\param link The media link representation\n> + * \\param source The MediaPad source\n> + * \\param sink The MediaPad sink\n> + */\n> +MediaLink::MediaLink(const struct media_v2_link *link, MediaPad *source,\n> +\t\t MediaPad *sink)\n> +\t: MediaObject(link->id), source_(source),\n> +\t sink_(sink), flags_(link->flags)\n> +{\n> +}\n> +\n> +/**\n> + * \\fn MediaLink::source()\n> + * \\brief Return the source MediaPad\n> + */\n> +\n> +/**\n> + * \\fn MediaLink::sink()\n> + * \\brief Return the sink MediaPad\n> + */\n> +\n> +/**\n> + * \\fn MediaLink::flags()\n> + * \\brief Return the link flags\n> + */\n> +\n> +/**\n> + * \\fn MediaLink::setFlags()\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> + * MediaPads represent a 'struct media_v2_pad', with associated a list of\n> + * links and a reference to the entity they belong to.\n> + *\n> + * MediaPads are connected to other MediaPads by MediaLinks, created\n> + * inspecting the media graph topology. Data connection between pads can be\n> + * enabled or disabled operating on the link that connects the two. See +\n> * MediaLink.\n> + *\n> + * MediaPads are either 'source' pads, or 'sink' pads. This information is\n> + * captured by the MEDIA_PAD_FL_* flags as stored in the flags_ member\n> + * variable.\n> + *\n> + * Each MediaPad has a list of MediaLinks it is associated to. All links\n> + * in a source pad are outbound links, while all links in a sink pad are\n> + * inbound ones.\n> + *\n> + * Each MediaPad has a reference to the MediaEntity it belongs to.\n> + */\n> +\n> +/**\n> + * \\brief Create a MediaPad\n> + * \\param pad The media pad representation\n> + * \\param entity The MediaEntity this pad belongs to\n> + */\n> +MediaPad::MediaPad(const struct media_v2_pad *pad, MediaEntity *entity)\n> +\t: MediaObject(pad->id), index_(pad->index), entity_(entity),\n> +\t flags_(pad->flags)\n> +{\n> +}\n> +\n> +/**\n> + * \\brief Delete pad.\n> + *\n> + * Delete the pad by deleting its media links. As a reference to the same\n> + * MediaLink gets stored in both the source and the sink pad, only source\n> + * ones actually delete the MediaLink.\n> + */\n> +MediaPad::~MediaPad()\n> +{\n> +\tfor (MediaLink *l : links_)\n> +\t\tif (flags_ & MEDIA_PAD_FL_SOURCE)\n> +\t\t\tdelete l;\n\nThis is dangerous. Let's store all media objects in one vector in MediaDevice \nand delete them all from there.\n\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 MediaEntity 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 links in this pad.\n> + */\n> +\n> +/**\n> + * \\brief Add a new link 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\n> + *\n> + * A MediaEntity represents a 'struct media_v2_entity' with an associated\n> + * list of MediaPads it contains.\n> + *\n> + * Entities are created inspecting the media graph topology, and MediaPads\n> + * gets added as they are discovered.\n> + *\n> + * TODO: Add support for associating a devnode to the entity when\n> integrating\n> + * with DeviceEnumerator.\n> + */\n> +\n> +/**\n> + * \\fn MediaEntity::name()\n> + * \\brief Return the entity name\n> + */\n> +\n> +/**\n> + * \\fn MediaEntity::pads()\n> + * \\brief Return all pads in this entity\n> + */\n> +\n> +/**\n> + * \\fn MediaEntity::getPadByIndex(unsigned int index)\n> + * \\brief Get a pad in this entity by its index\n> + * \\return The pad with index \\a index\n> + * \\return nullptr if no pad is found at \\index\n> + */\n> +const MediaPad *MediaEntity::getPadByIndex(unsigned int index)\n> +{\n> +\tfor (MediaPad *p : pads_)\n> +\t\tif (p->index() == index)\n> +\t\t\treturn p;\n> +\n> +\treturn nullptr;\n\nYou can just return pads_[index] (checking whether index is valid first of \ncourse).\n\n> +}\n> +\n> +/**\n> + * \\brief Get a pad in this entity by its id\n> + * \\param id The pad globally unique id\n> + * \\return The pad with id \\a id\n> + * \\return nullptr if not pad with \\id is found\n> + */\n> +const MediaPad *MediaEntity::getPadById(unsigned int id)\n> +{\n> +\tfor (MediaPad *p : pads_)\n> +\t\tif (p->id() == id)\n> +\t\t\treturn p;\n> +\n> +\treturn nullptr;\n> +}\n> +\n> +/**\n> + * \\brief Construct a MediaEntity\n> + * \\param entity The media entity representation\n> + */\n> +MediaEntity::MediaEntity(const struct media_v2_entity *entity)\n> +\t: MediaObject(entity->id), name_(entity->name)\n> +{\n> +}\n> +\n> +/**\n> + * \\fn MediaEntity::~MediaEntity()\n> + * \\brief Delete all pads in the entity\n> + */\n> +MediaEntity::~MediaEntity()\n> +{\n> +\tfor (MediaPad *p : pads_)\n> +\t\tdelete p;\n\nSame comment as for links.\n\nApart from that there's no blocking issue,\n\nReviewed-by: Laurent Pinchart \n\n> +\tpads_.clear();\n> +}\n> +\n> +/**\n> + * \\brief Add \\a pad to list of entity's pads\n> + * \\param pad The pad to add\n> + */\n> +void MediaEntity::addPad(MediaPad *pad)\n> +{\n> +\tpads_.push_back(pad);\n> +}\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index f632eb5..01d321c 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -1,10 +1,12 @@\n> libcamera_sources = files([\n> 'log.cpp',\n> 'main.cpp',\n> + 'media_object.cpp',\n> ])\n> \n> libcamera_headers = files([\n> 'include/log.h',\n> + 'include/media_object.h',\n> 'include/utils.h',\n> ])","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 6B03A60B23\n\tfor ;\n\tMon, 31 Dec 2018 01:38:58 +0100 (CET)","from avalon.localnet (unknown\n\t[IPv6:2a02:a03f:3ad5:900:7d1d:858b:75d5:534d])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id AF1C450A;\n\tMon, 31 Dec 2018 01:38:56 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1546216736;\n\tbh=f2CAvF+lEshnOzLf26wF5Y9343Nzban2oFWTDzjRmdw=;\n\th=From:To:Cc:Subject:Date:In-Reply-To:References:From;\n\tb=F7nQtzRm91vFVdF412jmbLlxgCIbI2OVpPmunsjOA+DQQAg+wxp9zcqDjKfIkmEf3\n\t3mBaSfH0qWKQ8KtNBIoI1BJpK2vMOOzWi4KpUQQB1DzKjc3tYO+mMMiu9sZjI1HP8u\n\t72kGYxS3CiVyqF9Jo0gczM0WuTgQRP9YfKp3mET8=","From":"Laurent Pinchart ","To":"libcamera-devel@lists.libcamera.org","Date":"Mon, 31 Dec 2018 02:39:54 +0200","Message-ID":"<5999023.SlpS090CIt@avalon>","Organization":"Ideas on Board Oy","In-Reply-To":"<20181230142314.16263-2-jacopo@jmondi.org>","References":"<20181230142314.16263-1-jacopo@jmondi.org>\n\t<20181230142314.16263-2-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Transfer-Encoding":"7Bit","Content-Type":"text/plain; charset=\"us-ascii\"","Subject":"Re: [libcamera-devel] [PATCH v3 1/3] libcamera: Add MediaObject\n\tclass hierarchy","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":"Mon, 31 Dec 2018 00:38:58 -0000"}},{"id":153,"web_url":"https://patchwork.libcamera.org/comment/153/","msgid":"<20181231080425.GA997@uno.localdomain>","date":"2018-12-31T08:04:34","subject":"Re: [libcamera-devel] [PATCH v3 1/3] libcamera: Add MediaObject\n\tclass hierarchy","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n\nOn Sun, Dec 30, 2018 at 11:47:56PM +0100, Niklas Söderlund wrote:\n> Hi Jacopo,\n>\n> On 2018-12-30 23:10:57 +0100, Jacopo Mondi wrote:\n> > Hi Niklas,\n> > thanks for review\n> >\n> > On Sun, Dec 30, 2018 at 10:50:15PM +0100, Niklas Söderlund wrote:\n> > > Hi Jacopo,\n> > >\n> > > Thanks for your patch.\n> > >\n> > > On 2018-12-30 15:23:12 +0100, Jacopo Mondi wrote:\n> > > > Add a class hierarcy to represent all media objects a media graph represents.\n> > > > Add a base MediaObject class, which retains the global unique object id,\n> > > > and define the derived MediaEntity, MediaLink and MediaPad 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 \n> > > > ---\n> > > > src/libcamera/include/media_object.h | 107 ++++++++++\n> > > > src/libcamera/media_object.cpp | 281 +++++++++++++++++++++++++++\n> > > > src/libcamera/meson.build | 2 +\n> > > > 3 files changed, 390 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 b/src/libcamera/include/media_object.h\n> > > > new file mode 100644\n> > > > index 0000000..118d0d8\n> > > > --- /dev/null\n> > > > +++ b/src/libcamera/include/media_object.h\n> > > > @@ -0,0 +1,107 @@\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 \n> > > > +#include \n> > > > +\n> > > > +#include \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 MediaPad;\n> > > > +class MediaLink : public MediaObject\n> > > > +{\n> > > > +\tfriend class MediaDevice;\n> > > > +\n> > > > +public:\n> > > > +\t~MediaLink() { }\n> > > > +\n> > > > +\tMediaPad *source() const { return source_; }\n> > > > +\tMediaPad *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> > > > +\t\t MediaPad *source, MediaPad *sink);\n> > >\n> > > Why not use references here? Would it be valid to create a MediaLink\n> > > where any of the parameters are nullptr?\n> >\n> > I feel it is better to use pointers for dynamically allocated objects\n> > as this class entities are. The caller will have pointers to them, and\n> > I should dereference them to access their value and pass it by\n> > reference.\n> >\n> > As I've replied to your series, I have not found this stated anywhere,\n> > so it might just be my preference, but I agree this protects us from\n> > nullptr values.\n> >\n> > >\n> > > Same question for other constructors in this patch.\n> > >\n> >\n> > Could I keep them like this until we don't try establish a firm rule?\n>\n> I have no problem keeping them, but if you want to keep them you should\n> ensure they are not null before dereferencing them.\n>\n\nThe caller checks for those pointers validity, and makes sure all\nthese objects are initialized with valid pointers.\n\nLet me point out that using references, and thus accessing the\npointer's value, if the caller provides a nullptr won't help:\n\n MediaPad *source == nullprt;\n MediaPad *source == nullprt;\n new MediaLink(link, *source, *sink)\n\nWill obviusly segault when the function is called.\n\nThanks\n j","headers":{"Return-Path":"","Received":["from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net\n\t[217.70.183.194])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C255E60B2E\n\tfor ;\n\tMon, 31 Dec 2018 09:04:37 +0100 (CET)","from uno.localdomain\n\t(host54-51-dynamic.16-87-r.retail.telecomitalia.it [87.16.51.54])\n\t(Authenticated sender: jacopo@jmondi.org)\n\tby relay2-d.mail.gandi.net (Postfix) with ESMTPSA id F127B40003;\n\tMon, 31 Dec 2018 08:04:36 +0000 (UTC)"],"X-Originating-IP":"87.16.51.54","Date":"Mon, 31 Dec 2018 09:04:34 +0100","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20181231080425.GA997@uno.localdomain>","References":"<20181230142314.16263-1-jacopo@jmondi.org>\n\t<20181230142314.16263-2-jacopo@jmondi.org>\n\t<20181230215015.GF31866@bigcity.dyn.berto.se>\n\t<20181230221057.ckxmjw52nvjveu4s@uno.localdomain>\n\t<20181230224756.GA10929@bigcity.dyn.berto.se>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"kXdP64Ggrk/fb43R\"","Content-Disposition":"inline","In-Reply-To":"<20181230224756.GA10929@bigcity.dyn.berto.se>","User-Agent":"Mutt/1.11.1 (2018-12-01)","Subject":"Re: [libcamera-devel] [PATCH v3 1/3] libcamera: Add MediaObject\n\tclass hierarchy","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":"Mon, 31 Dec 2018 08:04:37 -0000"}},{"id":155,"web_url":"https://patchwork.libcamera.org/comment/155/","msgid":"<20181231085951.GC997@uno.localdomain>","date":"2018-12-31T08:59:51","subject":"Re: [libcamera-devel] [PATCH v3 1/3] libcamera: Add MediaObject\n\tclass hierarchy","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Mon, Dec 31, 2018 at 02:39:54AM +0200, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Sunday, 30 December 2018 16:23:12 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 \n> > ---\n> > src/libcamera/include/media_object.h | 107 ++++++++++\n> > src/libcamera/media_object.cpp | 281 +++++++++++++++++++++++++++\n> > src/libcamera/meson.build | 2 +\n> > 3 files changed, 390 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..118d0d8\n> > --- /dev/null\n> > +++ b/src/libcamera/include/media_object.h\n> > @@ -0,0 +1,107 @@\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 \n> > +#include \n> > +\n> > +#include \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 MediaPad;\n> > +class MediaLink : public MediaObject\n> > +{\n> > +\tfriend class MediaDevice;\n> > +\n> > +public:\n> > +\t~MediaLink() { }\n> > +\n> > +\tMediaPad *source() const { return source_; }\n> > +\tMediaPad *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> > +\t\t MediaPad *source, MediaPad *sink);\n> > +\tMediaLink(const MediaLink &) = delete;\n> > +\n> > +\tMediaPad *source_;\n> > +\tMediaPad *sink_;\n> > +\tunsigned int flags_;\n> > +};\n> > +\n> > +class MediaEntity;\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> > +\tMediaEntity *entity() const { return entity_; }\n> > +\tunsigned int flags() const { return flags_; }\n> > +\tconst std::vector &links() const { return links_; }\n> > +\n> > +\tvoid addLink(MediaLink *link);\n> > +\n> > +private:\n> > +\tMediaPad(const struct media_v2_pad *pad, MediaEntity *entity);\n> > +\tMediaPad(const MediaPad &) = delete;\n> > +\n> > +\tunsigned int index_;\n> > +\tMediaEntity *entity_;\n> > +\tunsigned int flags_;\n> > +\n> > +\tstd::vector links_;\n> > +};\n> > +\n> > +class MediaEntity : public MediaObject\n> > +{\n> > +\tfriend class MediaDevice;\n> > +\n> > +public:\n> > +\tconst std::string &name() const { return name_; }\n> > +\n> > +\tconst std::vector &pads() { return pads_; }\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 pads_;\n> > +\n> > +\tvoid addPad(MediaPad *pad);\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..f0906e8\n> > --- /dev/null\n> > +++ b/src/libcamera/media_object.cpp\n> > @@ -0,0 +1,281 @@\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 \n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +\n> > +#include \"log.h\"\n> > +#include \"media_object.h\"\n> > +\n> > +/**\n> > + * \\file media_object.h\n> > + *\n> > + * In the media object file is implemented a class hierarchy that\n> > + * represent the counterpart of media objects exposed by the kernel's\n> > + * media controller APIs.\n> > + *\n> > + * Media Objects here represented are media entities, media pads and\n> > + * media links, created with informations as obtained by the\n> > + * MEDIA_IOC_G_TOPOLOGY ioctl call.\n> > + *\n> > + * MediaLink, MediaPad and MediaEntity are derived classes of the virtual\n> > + * base class MediaObject, which maintains a globally unique id for each\n> > object. + *\n> > + * Media objects have private constructors to restrict the number of\n> > classes + * that can instantiate them only to the 'friend' MediaDevice one.\n> > Media + * objects are in facts created when a MediaDevice gets populated. +\n> > */\n> > +\n> > +namespace libcamera {\n> > +\n> > +/**\n> > + * \\class MediaObject\n> > + * \\brief Base class for all media object types\n> > + *\n> > + * MediaObject is the virtual base class for all media objects in the\n> > + * media graph. Each object has a globally unique id assigned, and this\n> > + * base class provides that.\n> > + *\n> > + * MediaEntities, MediaPads and MediaLink are MediaObject derived classes.\n> > + */\n> > +\n> > +/**\n> > + * \\fn MediaObject::MediaObject()\n> > + * \\brief Construct a MediaObject with \\a id\n> > + * \\param id The globally unique object id\n> > + */\n> > +\n> > +/**\n> > + * \\fn MediaObject::id()\n> > + * \\brief Return the object's globally unique id\n> > + */\n> > +\n> > +/**\n> > + * \\var MediaObject::id_\n> > + * \\brief The MediaObject unique id\n> > + */\n> > +\n> > +/**\n> > + * \\class MediaLink\n> > + * \\brief Media Link object\n> > + *\n> > + * A MediaLink represents a 'struct media_v2_link', with associated\n> > + * references to the MediaPad it connects and an internal status defined\n> > + * by the MEDIA_LNK_FL_* flags captured in flags_.\n> > + *\n> > + * MediaLink can be enabled enabled and disabled, with the exception of\n> > + * immutable links (see MEDIA_LNK_FL_IMMUTABLE).\n> > + *\n> > + * MediaLinks are created between MediaPads inspecting the media graph\n> > + * topology and are stored in both the source and sink MediaPad they\n> > + * connect.\n> > + */\n> > +\n> > +/**\n> > + * \\brief Construct a MediaLink\n> > + * \\param link The media link representation\n> > + * \\param source The MediaPad source\n> > + * \\param sink The MediaPad sink\n> > + */\n> > +MediaLink::MediaLink(const struct media_v2_link *link, MediaPad *source,\n> > +\t\t MediaPad *sink)\n> > +\t: MediaObject(link->id), source_(source),\n> > +\t sink_(sink), flags_(link->flags)\n> > +{\n> > +}\n> > +\n> > +/**\n> > + * \\fn MediaLink::source()\n> > + * \\brief Return the source MediaPad\n> > + */\n> > +\n> > +/**\n> > + * \\fn MediaLink::sink()\n> > + * \\brief Return the sink MediaPad\n> > + */\n> > +\n> > +/**\n> > + * \\fn MediaLink::flags()\n> > + * \\brief Return the link flags\n> > + */\n> > +\n> > +/**\n> > + * \\fn MediaLink::setFlags()\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> > + * MediaPads represent a 'struct media_v2_pad', with associated a list of\n> > + * links and a reference to the entity they belong to.\n> > + *\n> > + * MediaPads are connected to other MediaPads by MediaLinks, created\n> > + * inspecting the media graph topology. Data connection between pads can be\n> > + * enabled or disabled operating on the link that connects the two. See +\n> > * MediaLink.\n> > + *\n> > + * MediaPads are either 'source' pads, or 'sink' pads. This information is\n> > + * captured by the MEDIA_PAD_FL_* flags as stored in the flags_ member\n> > + * variable.\n> > + *\n> > + * Each MediaPad has a list of MediaLinks it is associated to. All links\n> > + * in a source pad are outbound links, while all links in a sink pad are\n> > + * inbound ones.\n> > + *\n> > + * Each MediaPad has a reference to the MediaEntity it belongs to.\n> > + */\n> > +\n> > +/**\n> > + * \\brief Create a MediaPad\n> > + * \\param pad The media pad representation\n> > + * \\param entity The MediaEntity this pad belongs to\n> > + */\n> > +MediaPad::MediaPad(const struct media_v2_pad *pad, MediaEntity *entity)\n> > +\t: MediaObject(pad->id), index_(pad->index), entity_(entity),\n> > +\t flags_(pad->flags)\n> > +{\n> > +}\n> > +\n> > +/**\n> > + * \\brief Delete pad.\n> > + *\n> > + * Delete the pad by deleting its media links. As a reference to the same\n> > + * MediaLink gets stored in both the source and the sink pad, only source\n> > + * ones actually delete the MediaLink.\n> > + */\n> > +MediaPad::~MediaPad()\n> > +{\n> > +\tfor (MediaLink *l : links_)\n> > +\t\tif (flags_ & MEDIA_PAD_FL_SOURCE)\n> > +\t\t\tdelete l;\n>\n> This is dangerous. Let's store all media objects in one vector in MediaDevice\n> and delete them all from there.\n>\n\nI still feel that deleting and accessing object following their\nconnections is a guarantee for correctness. But as the\nMediaDevice has already a list of all objects, and it creates them\nafter all, I've now simplified all destructors.\n\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 MediaEntity 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 links in this pad.\n> > + */\n> > +\n> > +/**\n> > + * \\brief Add a new link 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\n> > + *\n> > + * A MediaEntity represents a 'struct media_v2_entity' with an associated\n> > + * list of MediaPads it contains.\n> > + *\n> > + * Entities are created inspecting the media graph topology, and MediaPads\n> > + * gets added as they are discovered.\n> > + *\n> > + * TODO: Add support for associating a devnode to the entity when\n> > integrating\n> > + * with DeviceEnumerator.\n> > + */\n> > +\n> > +/**\n> > + * \\fn MediaEntity::name()\n> > + * \\brief Return the entity name\n> > + */\n> > +\n> > +/**\n> > + * \\fn MediaEntity::pads()\n> > + * \\brief Return all pads in this entity\n> > + */\n> > +\n> > +/**\n> > + * \\fn MediaEntity::getPadByIndex(unsigned int index)\n> > + * \\brief Get a pad in this entity by its index\n> > + * \\return The pad with index \\a index\n> > + * \\return nullptr if no pad is found at \\index\n> > + */\n> > +const MediaPad *MediaEntity::getPadByIndex(unsigned int index)\n> > +{\n> > +\tfor (MediaPad *p : pads_)\n> > +\t\tif (p->index() == index)\n> > +\t\t\treturn p;\n> > +\n> > +\treturn nullptr;\n>\n> You can just return pads_[index] (checking whether index is valid first of\n> course).\n\nThat calls for storing the number of pads in the entity, which I've no\nneed of at the momement. I'll keep this as it is.\n\n>\n> > +}\n> > +\n> > +/**\n> > + * \\brief Get a pad in this entity by its id\n> > + * \\param id The pad globally unique id\n> > + * \\return The pad with id \\a id\n> > + * \\return nullptr if not pad with \\id is found\n> > + */\n> > +const MediaPad *MediaEntity::getPadById(unsigned int id)\n> > +{\n> > +\tfor (MediaPad *p : pads_)\n> > +\t\tif (p->id() == id)\n> > +\t\t\treturn p;\n> > +\n> > +\treturn nullptr;\n> > +}\n> > +\n> > +/**\n> > + * \\brief Construct a MediaEntity\n> > + * \\param entity The media entity representation\n> > + */\n> > +MediaEntity::MediaEntity(const struct media_v2_entity *entity)\n> > +\t: MediaObject(entity->id), name_(entity->name)\n> > +{\n> > +}\n> > +\n> > +/**\n> > + * \\fn MediaEntity::~MediaEntity()\n> > + * \\brief Delete all pads in the entity\n> > + */\n> > +MediaEntity::~MediaEntity()\n> > +{\n> > +\tfor (MediaPad *p : pads_)\n> > +\t\tdelete p;\n>\n> Same comment as for links.\n>\n> Apart from that there's no blocking issue,\n>\n> Reviewed-by: Laurent Pinchart \n>\n\nThanks, I'll send and push v4 shortly.","headers":{"Return-Path":"","Received":["from relay3-d.mail.gandi.net (relay3-d.mail.gandi.net\n\t[217.70.183.195])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D3B7460B2E\n\tfor ;\n\tMon, 31 Dec 2018 09:59:50 +0100 (CET)","from uno.localdomain\n\t(host54-51-dynamic.16-87-r.retail.telecomitalia.it [87.16.51.54])\n\t(Authenticated sender: jacopo@jmondi.org)\n\tby relay3-d.mail.gandi.net (Postfix) with ESMTPSA id BAC4760002;\n\tMon, 31 Dec 2018 08:59:49 +0000 (UTC)"],"X-Originating-IP":"87.16.51.54","Date":"Mon, 31 Dec 2018 09:59:51 +0100","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20181231085951.GC997@uno.localdomain>","References":"<20181230142314.16263-1-jacopo@jmondi.org>\n\t<20181230142314.16263-2-jacopo@jmondi.org>\n\t<5999023.SlpS090CIt@avalon>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"76DTJ5CE0DCVQemd\"","Content-Disposition":"inline","In-Reply-To":"<5999023.SlpS090CIt@avalon>","User-Agent":"Mutt/1.11.1 (2018-12-01)","Subject":"Re: [libcamera-devel] [PATCH v3 1/3] libcamera: Add MediaObject\n\tclass hierarchy","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":"Mon, 31 Dec 2018 08:59:51 -0000"}}]