[{"id":200,"web_url":"https://patchwork.libcamera.org/comment/200/","msgid":"<20190104162456.GL22790@bigcity.dyn.berto.se>","date":"2019-01-04T16:24:57","subject":"Re: [libcamera-devel] [PATCH 1/5] libcamera: Add pointer to\n\tMediaDevice to MediaObject","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 2019-01-03 18:38:55 +0100, Jacopo Mondi wrote:\n> Add a MediaDevice member field to the MediaObject class hierarcy.\n> Each media object now has a reference to the media device it belongs to,\n> and which it has been created by.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> src/libcamera/include/media_object.h | 10 ++++++----\n> src/libcamera/media_device.cpp | 8 ++++----\n> src/libcamera/media_object.cpp | 30 +++++++++++++++++++---------\n> 3 files changed, 31 insertions(+), 17 deletions(-)\n> \n> diff --git a/src/libcamera/include/media_object.h b/src/libcamera/include/media_object.h\n> index 950a332..0f0bb29 100644\n> --- a/src/libcamera/include/media_object.h\n> +++ b/src/libcamera/include/media_object.h\n> @@ -26,10 +26,12 @@ public:\n> protected:\n> \tfriend class MediaDevice;\n> \n> -\tMediaObject(unsigned int id) : id_(id) { }\n> +\tMediaObject(MediaDevice *media, unsigned int id) :\n> +\t\tid_(id), media_(media) { }\n\nNit: I would swap the initialization settings if id_(), media_() to \nmedia_(), id_() in order to match the arguments to the constructor.\n\n> \tvirtual ~MediaObject() { }\n> \n> \tunsigned int id_;\n> +\tMediaDevice *media_;\n> };\n> \n> class MediaLink : public MediaObject\n> @@ -42,7 +44,7 @@ public:\n> private:\n> \tfriend class MediaDevice;\n> \n> -\tMediaLink(const struct media_v2_link *link,\n> +\tMediaLink(MediaDevice *media, const struct media_v2_link *link,\n> \t\t MediaPad *source, MediaPad *sink);\n> \tMediaLink(const MediaLink &) = delete;\n> \t~MediaLink() { }\n> @@ -65,7 +67,7 @@ public:\n> private:\n> \tfriend class MediaDevice;\n> \n> -\tMediaPad(const struct media_v2_pad *pad, MediaEntity *entity);\n> +\tMediaPad(MediaDevice *media, const struct media_v2_pad *pad, MediaEntity *entity);\n> \tMediaPad(const MediaPad &) = delete;\n> \t~MediaPad();\n> \n> @@ -93,7 +95,7 @@ public:\n> private:\n> \tfriend class MediaDevice;\n> \n> -\tMediaEntity(const struct media_v2_entity *entity,\n> +\tMediaEntity(MediaDevice *media, const struct media_v2_entity *entity,\n> \t\t unsigned int major = 0, unsigned int minor = 0);\n> \tMediaEntity(const MediaEntity &) = delete;\n> \t~MediaEntity();\n> diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp\n> index cf4ff90..2f9490c 100644\n> --- a/src/libcamera/media_device.cpp\n> +++ b/src/libcamera/media_device.cpp\n> @@ -430,11 +430,11 @@ bool MediaDevice::populateEntities(const struct media_v2_topology &topology)\n> \n> \t\tMediaEntity *entity;\n> \t\tif (iface)\n> -\t\t\tentity = new MediaEntity(&mediaEntities[i],\n> +\t\t\tentity = new MediaEntity(this, &mediaEntities[i],\n> \t\t\t\t\t\t iface->devnode.major,\n> \t\t\t\t\t\t iface->devnode.minor);\n> \t\telse\n> -\t\t\tentity = new MediaEntity(&mediaEntities[i]);\n> +\t\t\tentity = new MediaEntity(this, &mediaEntities[i]);\n> \n> \t\tif (!addObject(entity)) {\n> \t\t\tdelete entity;\n> @@ -464,7 +464,7 @@ bool MediaDevice::populatePads(const struct media_v2_topology &topology)\n> \t\t\treturn false;\n> \t\t}\n> \n> -\t\tMediaPad *pad = new MediaPad(&mediaPads[i], mediaEntity);\n> +\t\tMediaPad *pad = new MediaPad(this, &mediaPads[i], mediaEntity);\n> \t\tif (!addObject(pad)) {\n> \t\t\tdelete pad;\n> \t\t\treturn false;\n> @@ -509,7 +509,7 @@ bool MediaDevice::populateLinks(const struct media_v2_topology &topology)\n> \t\t\treturn false;\n> \t\t}\n> \n> -\t\tMediaLink *link = new MediaLink(&mediaLinks[i], source, sink);\n> +\t\tMediaLink *link = new MediaLink(this, &mediaLinks[i], source, sink);\n> \t\tif (!addObject(link)) {\n> \t\t\tdelete link;\n> \t\t\treturn false;\n> diff --git a/src/libcamera/media_object.cpp b/src/libcamera/media_object.cpp\n> index 581e1c0..f1535e6 100644\n> --- a/src/libcamera/media_object.cpp\n> +++ b/src/libcamera/media_object.cpp\n> @@ -44,14 +44,16 @@ namespace libcamera {\n> *\n> * MediaObject is an abstract base class for all media objects in the media\n> * graph. Every media graph object is identified by an id unique in the media\n> - * device context, and this base class provides that.\n> + * device context, and this base class provides both of them.\n\nBoth? It provides the unique id and what? Maybe I'm slow today :-) I \nassume the other thing is a reference to the MediaDevice? If so how \nabout.\n\n MediaObject is an abstract base class for all media objects in the \n media graph. Each object is identified by a reference to the media \n device it belongs to and a unique id withing that media devce.\n This base class provide helpers to media objects to keep track of \n these identifiers.\n\n> *\n> * \\sa MediaEntity, MediaPad, MediaLink\n> */\n> \n> /**\n> * \\fn MediaObject::MediaObject()\n> - * \\brief Construct a MediaObject with \\a id\n> + * \\brief Construct a MediaObject with \\a id, globally unique in the MediaDevice\n> + * \\a media\n> + * \\param media The media device this object belongs to\n> * \\param id The media object id\n> *\n> * The caller shall ensure unicity of the object id in the media device context.\n> @@ -69,6 +71,11 @@ namespace libcamera {\n> * \\brief The media object id\n> */\n> \n> +/**\n> + * \\var MediaObject::media_\n> + * \\brief The media device that constructed this object\n\nHow about\n\n The media device the media object belongs to\n\nSame comment bellow for documentation of the media argument. With this \naddressed.\n\nReviewed-by: Niklas Söderlund \n\n> + */\n> +\n> /**\n> * \\class MediaLink\n> * \\brief The MediaLink represents a link between two pads in the media graph.\n> @@ -82,13 +89,14 @@ namespace libcamera {\n> \n> /**\n> * \\brief Construct a MediaLink\n> + * \\param media The media device this entity belongs to\n> * \\param link The media link kernel data\n> * \\param source The source pad at the origin of the link\n> * \\param sink The sink pad at the destination of the link\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> +MediaLink::MediaLink(MediaDevice *media, const struct media_v2_link *link,\n> +\t\t MediaPad *source, MediaPad *sink)\n> +\t: MediaObject(media, link->id), source_(source),\n> \t sink_(sink), flags_(link->flags)\n> {\n> }\n> @@ -135,11 +143,13 @@ MediaLink::MediaLink(const struct media_v2_link *link, MediaPad *source,\n> \n> /**\n> * \\brief Construct a MediaPad\n> + * \\param media The media device this entity belongs to\n> * \\param pad The media pad kernel data\n> * \\param entity The entity the 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> +MediaPad::MediaPad(MediaDevice *media, const struct media_v2_pad *pad,\n> +\t\t MediaEntity *entity)\n> +\t: MediaObject(media, pad->id), index_(pad->index), entity_(entity),\n> \t flags_(pad->flags)\n> {\n> }\n> @@ -283,13 +293,15 @@ int MediaEntity::setDeviceNode(const std::string &devnode)\n> \n> /**\n> * \\brief Construct a MediaEntity\n> + * \\param media The media device this entity belongs to\n> * \\param entity The media entity kernel data\n> * \\param major The major number of the entity associated interface\n> * \\param minor The minor number of the entity associated interface\n> */\n> -MediaEntity::MediaEntity(const struct media_v2_entity *entity,\n> +MediaEntity::MediaEntity(MediaDevice *media,\n> +\t\t\t const struct media_v2_entity *entity,\n> \t\t\t unsigned int major, unsigned int minor)\n> -\t: MediaObject(entity->id), name_(entity->name),\n> +\t: MediaObject(media, entity->id), name_(entity->name),\n> \t major_(major), minor_(minor)\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-x244.google.com (mail-lj1-x244.google.com\n\t[IPv6:2a00:1450:4864:20::244])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8554360B0B\n\tfor ;\n\tFri, 4 Jan 2019 17:25:14 +0100 (CET)","by mail-lj1-x244.google.com with SMTP id k19-v6so32843741lji.11\n\tfor ;\n\tFri, 04 Jan 2019 08:25:14 -0800 (PST)","from localhost (89-233-230-99.cust.bredband2.com. [89.233.230.99])\n\tby smtp.gmail.com with ESMTPSA id\n\ts62sm11410406lfg.34.2019.01.04.08.24.57\n\t(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);\n\tFri, 04 Jan 2019 08:24:57 -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=YONV7dqPKzKrrhDFIcI5OmxCorx2Uz5Gr0Z0kwE1178=;\n\tb=LdJPxiEhyHb9ElUlIb9YIkObK0gEbUhzwOW/sbQDSOGadxnH+LXCYYFSToYgZ8FKRo\n\tY1DEwy57QXqrlOWqh6oDx9jQ/GDQkA+02e2q9oqqXWSruW6hubn2yjLXK7t+76nbdePG\n\tI/SsrpR2DASFprJG3iCYecwrtFloRyJ+5qwHymFLP0/r0PlahrfnNYSnLizWXcz6zcl6\n\tE3kQ6xO2fu9yXchyJlm95/PDixSPRFEJfs9MHBzfBFTAYXG+NwOZ6Au1CtFFdPn5HG1I\n\tGzEkIjDdPLJxo8SqCk7wMexdH7nDPUQllZ3QYZgPLC/oW9hRLW8YUhs+5oKiQYnktGA3\n\tcvTw==","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=YONV7dqPKzKrrhDFIcI5OmxCorx2Uz5Gr0Z0kwE1178=;\n\tb=NkkzlPG8IZ6yI0H+BpzjwEEp+l+FDJMHyLqIzBevOvRxjilCZdEKixp5t7JOI+qo2H\n\t9SpeLFnciAw0DAJ9SwdzTu+16VcA64jVr3ThPNV6LhK6Y3R+uQic5htO8cSOlyWX2L+I\n\t8OPphBfNrkVXtpHMCLd33TP8rIIwB3tEs2x6RE9/f+qRRbCVjG4YLsfBgoEQNj+c/Iy2\n\tUL8MFKwqIDsrqp6zaxbwZf3+KAzPjQLhozWCJnG0uXPLJbjoTCnS2wMgQnBgqFz5JOqy\n\t9Sgjqn6ByABWRUTw6M2iIkupkPBKaSDCg03mHp1CywB3PThVjsp/qImLlqvRe9/35iB4\n\tdz0Q==","X-Gm-Message-State":"AJcUukeoDN1wJMSoRRmMJxnfkbHxxC0T914mtnMlrmIDdKkNOyov9nwB\n\tX8bCCrUUhpWMEqT9z0Lb+nn8lw==","X-Google-Smtp-Source":"AFSGD/UtMSSGKDmDfaEY769uZW0bCnnextLewoTtOT1UedJCHnCRe6YMQzt7ApSwgalj9qRNOcM86A==","X-Received":"by 2002:a2e:5303:: with SMTP id\n\th3-v6mr31319111ljb.35.1546619098543; \n\tFri, 04 Jan 2019 08:24:58 -0800 (PST)","Date":"Fri, 4 Jan 2019 17:24:57 +0100","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190104162456.GL22790@bigcity.dyn.berto.se>","References":"<20190103173859.22624-1-jacopo@jmondi.org>\n\t<20190103173859.22624-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":"<20190103173859.22624-2-jacopo@jmondi.org>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 1/5] libcamera: Add pointer to\n\tMediaDevice to MediaObject","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Fri, 04 Jan 2019 16:25:14 -0000"}},{"id":216,"web_url":"https://patchwork.libcamera.org/comment/216/","msgid":"<20190107085255.xaklbccoliswwpgb@uno.localdomain>","date":"2019-01-07T08:52:55","subject":"Re: [libcamera-devel] [PATCH 1/5] libcamera: Add pointer to\n\tMediaDevice to MediaObject","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n\nOn Fri, Jan 04, 2019 at 05:24:57PM +0100, Niklas Söderlund wrote:\n> Hi Jacopo,\n>\n> Thanks for your patch.\n>\n> On 2019-01-03 18:38:55 +0100, Jacopo Mondi wrote:\n> > Add a MediaDevice member field to the MediaObject class hierarcy.\n> > Each media object now has a reference to the media device it belongs to,\n> > and which it has been created by.\n> >\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/libcamera/include/media_object.h | 10 ++++++----\n> > src/libcamera/media_device.cpp | 8 ++++----\n> > src/libcamera/media_object.cpp | 30 +++++++++++++++++++---------\n> > 3 files changed, 31 insertions(+), 17 deletions(-)\n> >\n> > diff --git a/src/libcamera/include/media_object.h b/src/libcamera/include/media_object.h\n> > index 950a332..0f0bb29 100644\n> > --- a/src/libcamera/include/media_object.h\n> > +++ b/src/libcamera/include/media_object.h\n> > @@ -26,10 +26,12 @@ public:\n> > protected:\n> > \tfriend class MediaDevice;\n> >\n> > -\tMediaObject(unsigned int id) : id_(id) { }\n> > +\tMediaObject(MediaDevice *media, unsigned int id) :\n> > +\t\tid_(id), media_(media) { }\n>\n> Nit: I would swap the initialization settings if id_(), media_() to\n> media_(), id_() in order to match the arguments to the constructor.\n>\n\nThe compiler complains if you do so, as 'id_' is declared before\n'media_'.\n\n../src/libcamera/include/media_object.h: In constructor ‘libcamera::MediaObject::MediaObject(libcamera::MediaDevice*, unsigned int)’:\n../src/libcamera/include/media_object.h:34:15: error: ‘libcamera::MediaObject::media_’ will be initialized after [-Werror=reorder]\n MediaDevice *media_;\n ^~~~~~\n../src/libcamera/include/media_object.h:33:15: error: ‘unsigned int libcamera::MediaObject::id_’ [-Werror=reorder]\n unsigned int id_;\n ^~~\n../src/libcamera/include/media_object.h:29:2: error: when initialized here [-Werror=reorder]\n MediaObject(MediaDevice *media, unsigned int id) :\n\nThat's possibly the most stupid compiler warning I ever met.\n\n> > \tvirtual ~MediaObject() { }\n> >\n> > \tunsigned int id_;\n> > +\tMediaDevice *media_;\n> > };\n> >\n> > class MediaLink : public MediaObject\n> > @@ -42,7 +44,7 @@ public:\n> > private:\n> > \tfriend class MediaDevice;\n> >\n> > -\tMediaLink(const struct media_v2_link *link,\n> > +\tMediaLink(MediaDevice *media, const struct media_v2_link *link,\n> > \t\t MediaPad *source, MediaPad *sink);\n> > \tMediaLink(const MediaLink &) = delete;\n> > \t~MediaLink() { }\n> > @@ -65,7 +67,7 @@ public:\n> > private:\n> > \tfriend class MediaDevice;\n> >\n> > -\tMediaPad(const struct media_v2_pad *pad, MediaEntity *entity);\n> > +\tMediaPad(MediaDevice *media, const struct media_v2_pad *pad, MediaEntity *entity);\n> > \tMediaPad(const MediaPad &) = delete;\n> > \t~MediaPad();\n> >\n> > @@ -93,7 +95,7 @@ public:\n> > private:\n> > \tfriend class MediaDevice;\n> >\n> > -\tMediaEntity(const struct media_v2_entity *entity,\n> > +\tMediaEntity(MediaDevice *media, const struct media_v2_entity *entity,\n> > \t\t unsigned int major = 0, unsigned int minor = 0);\n> > \tMediaEntity(const MediaEntity &) = delete;\n> > \t~MediaEntity();\n> > diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp\n> > index cf4ff90..2f9490c 100644\n> > --- a/src/libcamera/media_device.cpp\n> > +++ b/src/libcamera/media_device.cpp\n> > @@ -430,11 +430,11 @@ bool MediaDevice::populateEntities(const struct media_v2_topology &topology)\n> >\n> > \t\tMediaEntity *entity;\n> > \t\tif (iface)\n> > -\t\t\tentity = new MediaEntity(&mediaEntities[i],\n> > +\t\t\tentity = new MediaEntity(this, &mediaEntities[i],\n> > \t\t\t\t\t\t iface->devnode.major,\n> > \t\t\t\t\t\t iface->devnode.minor);\n> > \t\telse\n> > -\t\t\tentity = new MediaEntity(&mediaEntities[i]);\n> > +\t\t\tentity = new MediaEntity(this, &mediaEntities[i]);\n> >\n> > \t\tif (!addObject(entity)) {\n> > \t\t\tdelete entity;\n> > @@ -464,7 +464,7 @@ bool MediaDevice::populatePads(const struct media_v2_topology &topology)\n> > \t\t\treturn false;\n> > \t\t}\n> >\n> > -\t\tMediaPad *pad = new MediaPad(&mediaPads[i], mediaEntity);\n> > +\t\tMediaPad *pad = new MediaPad(this, &mediaPads[i], mediaEntity);\n> > \t\tif (!addObject(pad)) {\n> > \t\t\tdelete pad;\n> > \t\t\treturn false;\n> > @@ -509,7 +509,7 @@ bool MediaDevice::populateLinks(const struct media_v2_topology &topology)\n> > \t\t\treturn false;\n> > \t\t}\n> >\n> > -\t\tMediaLink *link = new MediaLink(&mediaLinks[i], source, sink);\n> > +\t\tMediaLink *link = new MediaLink(this, &mediaLinks[i], source, sink);\n> > \t\tif (!addObject(link)) {\n> > \t\t\tdelete link;\n> > \t\t\treturn false;\n> > diff --git a/src/libcamera/media_object.cpp b/src/libcamera/media_object.cpp\n> > index 581e1c0..f1535e6 100644\n> > --- a/src/libcamera/media_object.cpp\n> > +++ b/src/libcamera/media_object.cpp\n> > @@ -44,14 +44,16 @@ namespace libcamera {\n> > *\n> > * MediaObject is an abstract base class for all media objects in the media\n> > * graph. Every media graph object is identified by an id unique in the media\n> > - * device context, and this base class provides that.\n> > + * device context, and this base class provides both of them.\n>\n> Both? It provides the unique id and what? Maybe I'm slow today :-) I\n> assume the other thing is a reference to the MediaDevice? If so how\n> about.\n>\n> MediaObject is an abstract base class for all media objects in the\n> media graph. Each object is identified by a reference to the media\n> device it belongs to and a unique id withing that media devce.\n> This base class provide helpers to media objects to keep track of\n> these identifiers.\n\ns/withing/within, s/these identifiers/them, and I'll take your\nsuggestion in.\n>\n> > *\n> > * \\sa MediaEntity, MediaPad, MediaLink\n> > */\n> >\n> > /**\n> > * \\fn MediaObject::MediaObject()\n> > - * \\brief Construct a MediaObject with \\a id\n> > + * \\brief Construct a MediaObject with \\a id, globally unique in the MediaDevice\n> > + * \\a media\n> > + * \\param media The media device this object belongs to\n> > * \\param id The media object id\n> > *\n> > * The caller shall ensure unicity of the object id in the media device context.\n> > @@ -69,6 +71,11 @@ namespace libcamera {\n> > * \\brief The media object id\n> > */\n> >\n> > +/**\n> > + * \\var MediaObject::media_\n> > + * \\brief The media device that constructed this object\n>\n> How about\n>\n> The media device the media object belongs to\n>\n> Same comment bellow for documentation of the media argument. With this\n> addressed.\n\nSorry, I missed what you're referring to. Below here the documentation\nfor the media argument is:\n\n> > + * \\param media The media device this entity belongs to\n\nWhat would you change.\n\nThanks\n j\n\n\n>\n> Reviewed-by: Niklas Söderlund \n>\n> > + */\n> > +\n> > /**\n> > * \\class MediaLink\n> > * \\brief The MediaLink represents a link between two pads in the media graph.\n> > @@ -82,13 +89,14 @@ namespace libcamera {\n> >\n> > /**\n> > * \\brief Construct a MediaLink\n> > + * \\param media The media device this entity belongs to\n> > * \\param link The media link kernel data\n> > * \\param source The source pad at the origin of the link\n> > * \\param sink The sink pad at the destination of the link\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> > +MediaLink::MediaLink(MediaDevice *media, const struct media_v2_link *link,\n> > +\t\t MediaPad *source, MediaPad *sink)\n> > +\t: MediaObject(media, link->id), source_(source),\n> > \t sink_(sink), flags_(link->flags)\n> > {\n> > }\n> > @@ -135,11 +143,13 @@ MediaLink::MediaLink(const struct media_v2_link *link, MediaPad *source,\n> >\n> > /**\n> > * \\brief Construct a MediaPad\n> > + * \\param media The media device this entity belongs to\n> > * \\param pad The media pad kernel data\n> > * \\param entity The entity the 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> > +MediaPad::MediaPad(MediaDevice *media, const struct media_v2_pad *pad,\n> > +\t\t MediaEntity *entity)\n> > +\t: MediaObject(media, pad->id), index_(pad->index), entity_(entity),\n> > \t flags_(pad->flags)\n> > {\n> > }\n> > @@ -283,13 +293,15 @@ int MediaEntity::setDeviceNode(const std::string &devnode)\n> >\n> > /**\n> > * \\brief Construct a MediaEntity\n> > + * \\param media The media device this entity belongs to\n> > * \\param entity The media entity kernel data\n> > * \\param major The major number of the entity associated interface\n> > * \\param minor The minor number of the entity associated interface\n> > */\n> > -MediaEntity::MediaEntity(const struct media_v2_entity *entity,\n> > +MediaEntity::MediaEntity(MediaDevice *media,\n> > +\t\t\t const struct media_v2_entity *entity,\n> > \t\t\t unsigned int major, unsigned int minor)\n> > -\t: MediaObject(entity->id), name_(entity->name),\n> > +\t: MediaObject(media, entity->id), name_(entity->name),\n> > \t major_(major), minor_(minor)\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 080E160B0B\n\tfor ;\n\tMon, 7 Jan 2019 09:52:50 +0100 (CET)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay8-d.mail.gandi.net (Postfix) with ESMTPSA id 7DF5C1BF20F;\n\tMon, 7 Jan 2019 08:52:49 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Mon, 7 Jan 2019 09:52:55 +0100","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190107085255.xaklbccoliswwpgb@uno.localdomain>","References":"<20190103173859.22624-1-jacopo@jmondi.org>\n\t<20190103173859.22624-2-jacopo@jmondi.org>\n\t<20190104162456.GL22790@bigcity.dyn.berto.se>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"sslysixah6by37hj\"","Content-Disposition":"inline","In-Reply-To":"<20190104162456.GL22790@bigcity.dyn.berto.se>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH 1/5] libcamera: Add pointer to\n\tMediaDevice to MediaObject","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, 07 Jan 2019 08:52:50 -0000"}},{"id":235,"web_url":"https://patchwork.libcamera.org/comment/235/","msgid":"<3889882.S2iWa43fg7@avalon>","date":"2019-01-07T21:24:12","subject":"Re: [libcamera-devel] [PATCH 1/5] libcamera: Add pointer to\n\tMediaDevice to MediaObject","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello,\n\nOn Monday, 7 January 2019 10:52:55 EET Jacopo Mondi wrote:\n> On Fri, Jan 04, 2019 at 05:24:57PM +0100, Niklas Söderlund wrote:\n> > On 2019-01-03 18:38:55 +0100, Jacopo Mondi wrote:\n> >> Add a MediaDevice member field to the MediaObject class hierarcy.\n> >> Each media object now has a reference to the media device it belongs to,\n> >> and which it has been created by.\n> >> \n> >> Signed-off-by: Jacopo Mondi \n> >> ---\n> >> \n> >> src/libcamera/include/media_object.h | 10 ++++++----\n> >> src/libcamera/media_device.cpp | 8 ++++----\n> >> src/libcamera/media_object.cpp | 30 +++++++++++++++++++---------\n> >> 3 files changed, 31 insertions(+), 17 deletions(-)\n> >> \n> >> diff --git a/src/libcamera/include/media_object.h\n> >> b/src/libcamera/include/media_object.h index 950a332..0f0bb29 100644\n> >> --- a/src/libcamera/include/media_object.h\n> >> +++ b/src/libcamera/include/media_object.h\n> >> \n> >> @@ -26,10 +26,12 @@ public:\n> >> protected:\n> >> \tfriend class MediaDevice;\n> >> \n> >> -\tMediaObject(unsigned int id) : id_(id) { }\n> >> +\tMediaObject(MediaDevice *media, unsigned int id) :\n> >> +\t\tid_(id), media_(media) { }\n\nShould this be named dev_(dev) instead of media_(media) ?\n\n> > \n> > Nit: I would swap the initialization settings if id_(), media_() to\n> > media_(), id_() in order to match the arguments to the constructor.\n> \n> The compiler complains if you do so, as 'id_' is declared before\n> 'media_'.\n> \n> ../src/libcamera/include/media_object.h: In constructor\n> ‘libcamera::MediaObject::MediaObject(libcamera::MediaDevice*, unsigned\n> int)’: ../src/libcamera/include/media_object.h:34:15: error:\n> ‘libcamera::MediaObject::media_’ will be initialized after\n> [-Werror=reorder] MediaDevice *media_;\n> ^~~~~~\n> ../src/libcamera/include/media_object.h:33:15: error: ‘unsigned int\n> libcamera::MediaObject::id_’ [-Werror=reorder] unsigned int id_;\n> ^~~\n> ../src/libcamera/include/media_object.h:29:2: error: when initialized here\n> [-Werror=reorder] MediaObject(MediaDevice *media, unsigned int id) :\n\nYou can swap the id_ and media_ fields in the class to fix this.\n\n> That's possibly the most stupid compiler warning I ever met.\n\nhttps://github.com/isocpp/CppCoreGuidelines/blob/master/\nCppCoreGuidelines.md#c47-define-and-initialize-member-variables-in-the-order-\nof-member-declaration\n\nMembers are initialized in the order they are declared in the class. Without \nthe warning, the following code would compile, but would lead to confusing \nbehaviour.\n\nclass Foo\n{\npublic:\n\tFoo(int i) : a(i++), b(i++), c(i++) { }\n\n\tvoid print() {\n\t\tcout << \"a: \" << a << \", b: \" << b << \", c: \" << c << endl;\n\t}\n\nprivate:\n\tint a;\n\tint c;\n\tint b;\n};\n\nint main(int, char*[])\n{\n\tFoo f(0);\n\n\tf.print();\n\treturn 0\n}\n\nOutput:\na: 0, b: 2, c: 1\n\nThe warning prevents this kind of issue.\n\n> > > \tvirtual ~MediaObject() { }\n> > > \t\n> > > \tunsigned int id_;\n> > > +\tMediaDevice *media_;\n> > > };\n> > > \n> > > class MediaLink : public MediaObject\n> > > @@ -42,7 +44,7 @@ public:\n> > > private:\n> > > \tfriend class MediaDevice;\n> > > \n> > > -\tMediaLink(const struct media_v2_link *link,\n> > > +\tMediaLink(MediaDevice *media, const struct media_v2_link *link,\n> > > \t\t MediaPad *source, MediaPad *sink);\n> > > \tMediaLink(const MediaLink &) = delete;\n> > > \t~MediaLink() { }\n> > > @@ -65,7 +67,7 @@ public:\n> > > private:\n> > > \tfriend class MediaDevice;\n> > > \n> > > -\tMediaPad(const struct media_v2_pad *pad, MediaEntity *entity);\n> > > +\tMediaPad(MediaDevice *media, const struct media_v2_pad *pad,\n> > > MediaEntity *entity);\n\nShouldn't you break long lines ? You could also do\n\nMediaPad::MediaPad(const struct media_v2_pad *pad, MediaEntity *entity)\n\t: MediaObject(entity->media_, pad->id), index_(pad->index),\n\t entity_(entity), flags_(pad->flags)\n\nto avoid passing the media device pointer as an argument. MediaLink could do \nsomething similar. Up to you.\n\nAnother option would be to store the media device pointer in MediaEntity only \nand access it from there (link -> pad -> entity -> media device) to avoid \nstoring an extra pointer in objects that may not need it.\n\n> > > \t flags_(pad->flags)\n> > > \tMediaPad(const MediaPad &) = delete;\n> > > \t~MediaPad();\n> > > @@ -93,7 +95,7 @@ public:\n> > > private:\n> > > \tfriend class MediaDevice;\n> > > \n> > > -\tMediaEntity(const struct media_v2_entity *entity,\n> > > +\tMediaEntity(MediaDevice *media, const struct media_v2_entity \n*entity,\n> > > \t\t unsigned int major = 0, unsigned int minor = 0);\n> > > \tMediaEntity(const MediaEntity &) = delete;\n> > > \t~MediaEntity();\n> > > diff --git a/src/libcamera/media_device.cpp\n> > > b/src/libcamera/media_device.cpp index cf4ff90..2f9490c 100644\n> > > --- a/src/libcamera/media_device.cpp\n> > > +++ b/src/libcamera/media_device.cpp\n> > > @@ -430,11 +430,11 @@ bool MediaDevice::populateEntities(const struct\n> > > media_v2_topology &topology)\n> > > \t\tMediaEntity *entity;\n> > > \t\tif (iface)\n> > > -\t\t\tentity = new MediaEntity(&mediaEntities[i],\n> > > +\t\t\tentity = new MediaEntity(this, &mediaEntities[i],\n> > > \t\t\t\t\t\t iface->devnode.major,\n> > > \t\t\t\t\t\t iface->devnode.minor);\n> > > \t\telse\n> > > -\t\t\tentity = new MediaEntity(&mediaEntities[i]);\n> > > +\t\t\tentity = new MediaEntity(this, &mediaEntities[i]);\n> > > \n> > > \t\tif (!addObject(entity)) {\n> > > \t\t\tdelete entity;\n> > > @@ -464,7 +464,7 @@ bool MediaDevice::populatePads(const struct\n> > > media_v2_topology &topology)\n> > > \t\t\treturn false;\n> > > \t\t}\n> > > \n> > > -\t\tMediaPad *pad = new MediaPad(&mediaPads[i], mediaEntity);\n> > > +\t\tMediaPad *pad = new MediaPad(this, &mediaPads[i], mediaEntity);\n> > > \t\tif (!addObject(pad)) {\n> > > \t\t\tdelete pad;\n> > > \t\t\treturn false;\n> > > @@ -509,7 +509,7 @@ bool MediaDevice::populateLinks(const struct\n> > > media_v2_topology &topology)\n> > > \t\t\treturn false;\n> > > \t\t}\n> > > \n> > > -\t\tMediaLink *link = new MediaLink(&mediaLinks[i], source, sink);\n> > > +\t\tMediaLink *link = new MediaLink(this, &mediaLinks[i], source, \nsink);\n> > > \t\tif (!addObject(link)) {\n> > > \t\t\tdelete link;\n> > > \t\t\treturn false;\n> > > diff --git a/src/libcamera/media_object.cpp\n> > > b/src/libcamera/media_object.cpp index 581e1c0..f1535e6 100644\n> > > --- a/src/libcamera/media_object.cpp\n> > > +++ b/src/libcamera/media_object.cpp\n> > > @@ -44,14 +44,16 @@ namespace libcamera {\n> > > \n> > > *\n> > > * MediaObject is an abstract base class for all media objects in the\n> > > media\n> > > * graph. Every media graph object is identified by an id unique in the\n> > > media>\n> > > - * device context, and this base class provides that.\n> > > + * device context, and this base class provides both of them.\n> > \n> > Both? It provides the unique id and what? Maybe I'm slow today :-) I\n> > assume the other thing is a reference to the MediaDevice? If so how\n> > about.\n> > \n> > MediaObject is an abstract base class for all media objects in the\n> > media graph. Each object is identified by a reference to the media\n> > device it belongs to and a unique id withing that media devce.\n\ns/devce/device/\n\n> > This base class provide helpers to media objects to keep track of\n> > these identifiers.\n> \n> s/withing/within, s/these identifiers/them,\n\nI'd keep \"these identifies\" instead of \"them\", as the latter can be confusing \n(it could refer to \"helpers\" or \"media objects\" in that sentence).\n\n> and I'll take your suggestion in.\n> \n> > > *\n> > > * \\sa MediaEntity, MediaPad, MediaLink\n> > > */\n> > > \n> > > /**\n> > > * \\fn MediaObject::MediaObject()\n> > > \n> > > - * \\brief Construct a MediaObject with \\a id\n> > > + * \\brief Construct a MediaObject with \\a id, globally unique in the\n> > > MediaDevice\n\nTo me globally sounds like it is global, not local to a media device. I'd \nphrase this as \"Construct a MediaObject part of the MediaDevice \\a media, \nidentified by the \\a id unique in within the device\".\n\n> > > + * \\a media\n> > > + * \\param media The media device this object belongs to\n> > > * \\param id The media object id\n> > > *\n> > > * The caller shall ensure unicity of the object id in the media device\n> > > context.\n> > > @@ -69,6 +71,11 @@ namespace libcamera {\n> > > * \\brief The media object id\n> > > */\n> > > \n> > > +/**\n> > > + * \\var MediaObject::media_\n> > > + * \\brief The media device that constructed this object\n> > \n> > How about\n> > \n> > The media device the media object belongs to\n> > \n> > Same comment bellow for documentation of the media argument. With this\n> > addressed.\n> \n> Sorry, I missed what you're referring to. Below here the documentation\n> for the media argument is:\n> \n> > > + * \\param media The media device this entity belongs to\n> \n> What would you change.\n> \n> > Reviewed-by: Niklas Söderlund \n> > \n> > > + */\n> > > +\n> > > /**\n> > > * \\class MediaLink\n> > > * \\brief The MediaLink represents a link between two pads in the media\n> > > graph.\n> > > @@ -82,13 +89,14 @@ namespace libcamera {\n> > > \n> > > /**\n> > > * \\brief Construct a MediaLink\n> > > + * \\param media The media device this entity belongs to\n\ns/entity/link/\n\n> > > * \\param link The media link kernel data\n> > > * \\param source The source pad at the origin of the link\n> > > * \\param sink The sink pad at the destination of the link\n> > > */\n> > > -MediaLink::MediaLink(const struct media_v2_link *link, MediaPad\n> > > *source,\n> > > -\t\t MediaPad *sink)\n> > > -\t: MediaObject(link->id), source_(source),\n> > > +MediaLink::MediaLink(MediaDevice *media, const struct media_v2_link\n> > > *link,\n> > > +\t\t MediaPad *source, MediaPad *sink)\n> > > +\t: MediaObject(media, link->id), source_(source),\n> > > \t sink_(sink), flags_(link->flags)\n> > > {\n> > > }\n> > > @@ -135,11 +143,13 @@ MediaLink::MediaLink(const struct media_v2_link\n> > > *link, MediaPad *source,\n> > > \n> > > /**\n> > > * \\brief Construct a MediaPad\n> > > + * \\param media The media device this entity belongs to\n\ns/entity/pad/\n\n> > > * \\param pad The media pad kernel data\n> > > * \\param entity The entity the 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> > > +MediaPad::MediaPad(MediaDevice *media, const struct media_v2_pad *pad,\n> > > +\t\t MediaEntity *entity)\n> > > +\t: MediaObject(media, pad->id), index_(pad->index), entity_(entity),\n> > > \t flags_(pad->flags)\n> > > {\n> > > }\n> > > @@ -283,13 +293,15 @@ int MediaEntity::setDeviceNode(const std::string\n> > > &devnode)\n> > > \n> > > /**\n> > > * \\brief Construct a MediaEntity\n> > > + * \\param media The media device this entity belongs to\n\nThis one is correct :-)\n\n> > > * \\param entity The media entity kernel data\n> > > * \\param major The major number of the entity associated interface\n> > > * \\param minor The minor number of the entity associated interface\n> > > */\n> > > -MediaEntity::MediaEntity(const struct media_v2_entity *entity,\n> > > +MediaEntity::MediaEntity(MediaDevice *media,\n> > > +\t\t\t const struct media_v2_entity *entity,\n> > > \t\t\t unsigned int major, unsigned int minor)\n> > > -\t: MediaObject(entity->id), name_(entity->name),\n> > > +\t: MediaObject(media, entity->id), name_(entity->name),\n> > > \t major_(major), minor_(minor)\n> > > {\n> > > }","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BD420600CC\n\tfor ;\n\tMon, 7 Jan 2019 22:23:05 +0100 (CET)","from avalon.localnet (dfj612ybrt5fhg77mgycy-3.rev.dnainternet.fi\n\t[IPv6:2001:14ba:21f5:5b00:2e86:4862:ef6a:2804])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1B6CEE22;\n\tMon, 7 Jan 2019 22:23:05 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1546896185;\n\tbh=rbgLBjvzLUGKYTvEltd4jyCQbZXxRT2QiA+K5p5uddw=;\n\th=From:To:Cc:Subject:Date:In-Reply-To:References:From;\n\tb=huwBJzl3GFCH3fXKcyYFrAHVVbBJHpeLGhi4TmlRYHP2zEvsFq3UvQdAcjCpsgsW7\n\t3p6hhynbl9BYW41nQg5+gSe8XjM80p5/dw2FL4w2HJrag6SgM5X2DlGALUFR2EcRO3\n\tRcc7q9Ye98OOR0jUvhtRg43kuWOCkLjbqiJcg958=","From":"Laurent Pinchart ","To":"libcamera-devel@lists.libcamera.org","Date":"Mon, 07 Jan 2019 23:24:12 +0200","Message-ID":"<3889882.S2iWa43fg7@avalon>","Organization":"Ideas on Board Oy","In-Reply-To":"<20190107085255.xaklbccoliswwpgb@uno.localdomain>","References":"<20190103173859.22624-1-jacopo@jmondi.org>\n\t<20190104162456.GL22790@bigcity.dyn.berto.se>\n\t<20190107085255.xaklbccoliswwpgb@uno.localdomain>","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH 1/5] libcamera: Add pointer to\n\tMediaDevice to MediaObject","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, 07 Jan 2019 21:23:05 -0000"}}]