| Message ID | 20190103173859.22624-2-jacopo@jmondi.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, Thanks for your patch. On 2019-01-03 18:38:55 +0100, Jacopo Mondi wrote: > Add a MediaDevice member field to the MediaObject class hierarcy. > Each media object now has a reference to the media device it belongs to, > and which it has been created by. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/libcamera/include/media_object.h | 10 ++++++---- > src/libcamera/media_device.cpp | 8 ++++---- > src/libcamera/media_object.cpp | 30 +++++++++++++++++++--------- > 3 files changed, 31 insertions(+), 17 deletions(-) > > diff --git a/src/libcamera/include/media_object.h b/src/libcamera/include/media_object.h > index 950a332..0f0bb29 100644 > --- a/src/libcamera/include/media_object.h > +++ b/src/libcamera/include/media_object.h > @@ -26,10 +26,12 @@ public: > protected: > friend class MediaDevice; > > - MediaObject(unsigned int id) : id_(id) { } > + MediaObject(MediaDevice *media, unsigned int id) : > + id_(id), media_(media) { } Nit: I would swap the initialization settings if id_(), media_() to media_(), id_() in order to match the arguments to the constructor. > virtual ~MediaObject() { } > > unsigned int id_; > + MediaDevice *media_; > }; > > class MediaLink : public MediaObject > @@ -42,7 +44,7 @@ public: > private: > friend class MediaDevice; > > - MediaLink(const struct media_v2_link *link, > + MediaLink(MediaDevice *media, const struct media_v2_link *link, > MediaPad *source, MediaPad *sink); > MediaLink(const MediaLink &) = delete; > ~MediaLink() { } > @@ -65,7 +67,7 @@ public: > private: > friend class MediaDevice; > > - MediaPad(const struct media_v2_pad *pad, MediaEntity *entity); > + MediaPad(MediaDevice *media, const struct media_v2_pad *pad, MediaEntity *entity); > MediaPad(const MediaPad &) = delete; > ~MediaPad(); > > @@ -93,7 +95,7 @@ public: > private: > friend class MediaDevice; > > - MediaEntity(const struct media_v2_entity *entity, > + MediaEntity(MediaDevice *media, const struct media_v2_entity *entity, > unsigned int major = 0, unsigned int minor = 0); > MediaEntity(const MediaEntity &) = delete; > ~MediaEntity(); > diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp > index cf4ff90..2f9490c 100644 > --- a/src/libcamera/media_device.cpp > +++ b/src/libcamera/media_device.cpp > @@ -430,11 +430,11 @@ bool MediaDevice::populateEntities(const struct media_v2_topology &topology) > > MediaEntity *entity; > if (iface) > - entity = new MediaEntity(&mediaEntities[i], > + entity = new MediaEntity(this, &mediaEntities[i], > iface->devnode.major, > iface->devnode.minor); > else > - entity = new MediaEntity(&mediaEntities[i]); > + entity = new MediaEntity(this, &mediaEntities[i]); > > if (!addObject(entity)) { > delete entity; > @@ -464,7 +464,7 @@ bool MediaDevice::populatePads(const struct media_v2_topology &topology) > return false; > } > > - MediaPad *pad = new MediaPad(&mediaPads[i], mediaEntity); > + MediaPad *pad = new MediaPad(this, &mediaPads[i], mediaEntity); > if (!addObject(pad)) { > delete pad; > return false; > @@ -509,7 +509,7 @@ bool MediaDevice::populateLinks(const struct media_v2_topology &topology) > return false; > } > > - MediaLink *link = new MediaLink(&mediaLinks[i], source, sink); > + MediaLink *link = new MediaLink(this, &mediaLinks[i], source, sink); > if (!addObject(link)) { > delete link; > return false; > diff --git a/src/libcamera/media_object.cpp b/src/libcamera/media_object.cpp > index 581e1c0..f1535e6 100644 > --- a/src/libcamera/media_object.cpp > +++ b/src/libcamera/media_object.cpp > @@ -44,14 +44,16 @@ namespace libcamera { > * > * MediaObject is an abstract base class for all media objects in the media > * graph. Every media graph object is identified by an id unique in the media > - * device context, and this base class provides that. > + * device context, and this base class provides both of them. Both? It provides the unique id and what? Maybe I'm slow today :-) I assume the other thing is a reference to the MediaDevice? If so how about. MediaObject is an abstract base class for all media objects in the media graph. Each object is identified by a reference to the media device it belongs to and a unique id withing that media devce. This base class provide helpers to media objects to keep track of these identifiers. > * > * \sa MediaEntity, MediaPad, MediaLink > */ > > /** > * \fn MediaObject::MediaObject() > - * \brief Construct a MediaObject with \a id > + * \brief Construct a MediaObject with \a id, globally unique in the MediaDevice > + * \a media > + * \param media The media device this object belongs to > * \param id The media object id > * > * The caller shall ensure unicity of the object id in the media device context. > @@ -69,6 +71,11 @@ namespace libcamera { > * \brief The media object id > */ > > +/** > + * \var MediaObject::media_ > + * \brief The media device that constructed this object How about The media device the media object belongs to Same comment bellow for documentation of the media argument. With this addressed. Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > + */ > + > /** > * \class MediaLink > * \brief The MediaLink represents a link between two pads in the media graph. > @@ -82,13 +89,14 @@ namespace libcamera { > > /** > * \brief Construct a MediaLink > + * \param media The media device this entity belongs to > * \param link The media link kernel data > * \param source The source pad at the origin of the link > * \param sink The sink pad at the destination of the link > */ > -MediaLink::MediaLink(const struct media_v2_link *link, MediaPad *source, > - MediaPad *sink) > - : MediaObject(link->id), source_(source), > +MediaLink::MediaLink(MediaDevice *media, const struct media_v2_link *link, > + MediaPad *source, MediaPad *sink) > + : MediaObject(media, link->id), source_(source), > sink_(sink), flags_(link->flags) > { > } > @@ -135,11 +143,13 @@ MediaLink::MediaLink(const struct media_v2_link *link, MediaPad *source, > > /** > * \brief Construct a MediaPad > + * \param media The media device this entity belongs to > * \param pad The media pad kernel data > * \param entity The entity the pad belongs to > */ > -MediaPad::MediaPad(const struct media_v2_pad *pad, MediaEntity *entity) > - : MediaObject(pad->id), index_(pad->index), entity_(entity), > +MediaPad::MediaPad(MediaDevice *media, const struct media_v2_pad *pad, > + MediaEntity *entity) > + : MediaObject(media, pad->id), index_(pad->index), entity_(entity), > flags_(pad->flags) > { > } > @@ -283,13 +293,15 @@ int MediaEntity::setDeviceNode(const std::string &devnode) > > /** > * \brief Construct a MediaEntity > + * \param media The media device this entity belongs to > * \param entity The media entity kernel data > * \param major The major number of the entity associated interface > * \param minor The minor number of the entity associated interface > */ > -MediaEntity::MediaEntity(const struct media_v2_entity *entity, > +MediaEntity::MediaEntity(MediaDevice *media, > + const struct media_v2_entity *entity, > unsigned int major, unsigned int minor) > - : MediaObject(entity->id), name_(entity->name), > + : MediaObject(media, entity->id), name_(entity->name), > major_(major), minor_(minor) > { > } > -- > 2.20.1 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Niklas, On Fri, Jan 04, 2019 at 05:24:57PM +0100, Niklas Söderlund wrote: > Hi Jacopo, > > Thanks for your patch. > > On 2019-01-03 18:38:55 +0100, Jacopo Mondi wrote: > > Add a MediaDevice member field to the MediaObject class hierarcy. > > Each media object now has a reference to the media device it belongs to, > > and which it has been created by. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > src/libcamera/include/media_object.h | 10 ++++++---- > > src/libcamera/media_device.cpp | 8 ++++---- > > src/libcamera/media_object.cpp | 30 +++++++++++++++++++--------- > > 3 files changed, 31 insertions(+), 17 deletions(-) > > > > diff --git a/src/libcamera/include/media_object.h b/src/libcamera/include/media_object.h > > index 950a332..0f0bb29 100644 > > --- a/src/libcamera/include/media_object.h > > +++ b/src/libcamera/include/media_object.h > > @@ -26,10 +26,12 @@ public: > > protected: > > friend class MediaDevice; > > > > - MediaObject(unsigned int id) : id_(id) { } > > + MediaObject(MediaDevice *media, unsigned int id) : > > + id_(id), media_(media) { } > > Nit: I would swap the initialization settings if id_(), media_() to > media_(), id_() in order to match the arguments to the constructor. > The compiler complains if you do so, as 'id_' is declared before 'media_'. ../src/libcamera/include/media_object.h: In constructor ‘libcamera::MediaObject::MediaObject(libcamera::MediaDevice*, unsigned int)’: ../src/libcamera/include/media_object.h:34:15: error: ‘libcamera::MediaObject::media_’ will be initialized after [-Werror=reorder] MediaDevice *media_; ^~~~~~ ../src/libcamera/include/media_object.h:33:15: error: ‘unsigned int libcamera::MediaObject::id_’ [-Werror=reorder] unsigned int id_; ^~~ ../src/libcamera/include/media_object.h:29:2: error: when initialized here [-Werror=reorder] MediaObject(MediaDevice *media, unsigned int id) : That's possibly the most stupid compiler warning I ever met. > > virtual ~MediaObject() { } > > > > unsigned int id_; > > + MediaDevice *media_; > > }; > > > > class MediaLink : public MediaObject > > @@ -42,7 +44,7 @@ public: > > private: > > friend class MediaDevice; > > > > - MediaLink(const struct media_v2_link *link, > > + MediaLink(MediaDevice *media, const struct media_v2_link *link, > > MediaPad *source, MediaPad *sink); > > MediaLink(const MediaLink &) = delete; > > ~MediaLink() { } > > @@ -65,7 +67,7 @@ public: > > private: > > friend class MediaDevice; > > > > - MediaPad(const struct media_v2_pad *pad, MediaEntity *entity); > > + MediaPad(MediaDevice *media, const struct media_v2_pad *pad, MediaEntity *entity); > > MediaPad(const MediaPad &) = delete; > > ~MediaPad(); > > > > @@ -93,7 +95,7 @@ public: > > private: > > friend class MediaDevice; > > > > - MediaEntity(const struct media_v2_entity *entity, > > + MediaEntity(MediaDevice *media, const struct media_v2_entity *entity, > > unsigned int major = 0, unsigned int minor = 0); > > MediaEntity(const MediaEntity &) = delete; > > ~MediaEntity(); > > diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp > > index cf4ff90..2f9490c 100644 > > --- a/src/libcamera/media_device.cpp > > +++ b/src/libcamera/media_device.cpp > > @@ -430,11 +430,11 @@ bool MediaDevice::populateEntities(const struct media_v2_topology &topology) > > > > MediaEntity *entity; > > if (iface) > > - entity = new MediaEntity(&mediaEntities[i], > > + entity = new MediaEntity(this, &mediaEntities[i], > > iface->devnode.major, > > iface->devnode.minor); > > else > > - entity = new MediaEntity(&mediaEntities[i]); > > + entity = new MediaEntity(this, &mediaEntities[i]); > > > > if (!addObject(entity)) { > > delete entity; > > @@ -464,7 +464,7 @@ bool MediaDevice::populatePads(const struct media_v2_topology &topology) > > return false; > > } > > > > - MediaPad *pad = new MediaPad(&mediaPads[i], mediaEntity); > > + MediaPad *pad = new MediaPad(this, &mediaPads[i], mediaEntity); > > if (!addObject(pad)) { > > delete pad; > > return false; > > @@ -509,7 +509,7 @@ bool MediaDevice::populateLinks(const struct media_v2_topology &topology) > > return false; > > } > > > > - MediaLink *link = new MediaLink(&mediaLinks[i], source, sink); > > + MediaLink *link = new MediaLink(this, &mediaLinks[i], source, sink); > > if (!addObject(link)) { > > delete link; > > return false; > > diff --git a/src/libcamera/media_object.cpp b/src/libcamera/media_object.cpp > > index 581e1c0..f1535e6 100644 > > --- a/src/libcamera/media_object.cpp > > +++ b/src/libcamera/media_object.cpp > > @@ -44,14 +44,16 @@ namespace libcamera { > > * > > * MediaObject is an abstract base class for all media objects in the media > > * graph. Every media graph object is identified by an id unique in the media > > - * device context, and this base class provides that. > > + * device context, and this base class provides both of them. > > Both? It provides the unique id and what? Maybe I'm slow today :-) I > assume the other thing is a reference to the MediaDevice? If so how > about. > > MediaObject is an abstract base class for all media objects in the > media graph. Each object is identified by a reference to the media > device it belongs to and a unique id withing that media devce. > This base class provide helpers to media objects to keep track of > these identifiers. s/withing/within, s/these identifiers/them, and I'll take your suggestion in. > > > * > > * \sa MediaEntity, MediaPad, MediaLink > > */ > > > > /** > > * \fn MediaObject::MediaObject() > > - * \brief Construct a MediaObject with \a id > > + * \brief Construct a MediaObject with \a id, globally unique in the MediaDevice > > + * \a media > > + * \param media The media device this object belongs to > > * \param id The media object id > > * > > * The caller shall ensure unicity of the object id in the media device context. > > @@ -69,6 +71,11 @@ namespace libcamera { > > * \brief The media object id > > */ > > > > +/** > > + * \var MediaObject::media_ > > + * \brief The media device that constructed this object > > How about > > The media device the media object belongs to > > Same comment bellow for documentation of the media argument. With this > addressed. Sorry, I missed what you're referring to. Below here the documentation for the media argument is: > > + * \param media The media device this entity belongs to What would you change. Thanks j > > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > > + */ > > + > > /** > > * \class MediaLink > > * \brief The MediaLink represents a link between two pads in the media graph. > > @@ -82,13 +89,14 @@ namespace libcamera { > > > > /** > > * \brief Construct a MediaLink > > + * \param media The media device this entity belongs to > > * \param link The media link kernel data > > * \param source The source pad at the origin of the link > > * \param sink The sink pad at the destination of the link > > */ > > -MediaLink::MediaLink(const struct media_v2_link *link, MediaPad *source, > > - MediaPad *sink) > > - : MediaObject(link->id), source_(source), > > +MediaLink::MediaLink(MediaDevice *media, const struct media_v2_link *link, > > + MediaPad *source, MediaPad *sink) > > + : MediaObject(media, link->id), source_(source), > > sink_(sink), flags_(link->flags) > > { > > } > > @@ -135,11 +143,13 @@ MediaLink::MediaLink(const struct media_v2_link *link, MediaPad *source, > > > > /** > > * \brief Construct a MediaPad > > + * \param media The media device this entity belongs to > > * \param pad The media pad kernel data > > * \param entity The entity the pad belongs to > > */ > > -MediaPad::MediaPad(const struct media_v2_pad *pad, MediaEntity *entity) > > - : MediaObject(pad->id), index_(pad->index), entity_(entity), > > +MediaPad::MediaPad(MediaDevice *media, const struct media_v2_pad *pad, > > + MediaEntity *entity) > > + : MediaObject(media, pad->id), index_(pad->index), entity_(entity), > > flags_(pad->flags) > > { > > } > > @@ -283,13 +293,15 @@ int MediaEntity::setDeviceNode(const std::string &devnode) > > > > /** > > * \brief Construct a MediaEntity > > + * \param media The media device this entity belongs to > > * \param entity The media entity kernel data > > * \param major The major number of the entity associated interface > > * \param minor The minor number of the entity associated interface > > */ > > -MediaEntity::MediaEntity(const struct media_v2_entity *entity, > > +MediaEntity::MediaEntity(MediaDevice *media, > > + const struct media_v2_entity *entity, > > unsigned int major, unsigned int minor) > > - : MediaObject(entity->id), name_(entity->name), > > + : MediaObject(media, entity->id), name_(entity->name), > > major_(major), minor_(minor) > > { > > } > > -- > > 2.20.1 > > > > _______________________________________________ > > libcamera-devel mailing list > > libcamera-devel@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel > > -- > Regards, > Niklas Söderlund
Hello, On Monday, 7 January 2019 10:52:55 EET Jacopo Mondi wrote: > On Fri, Jan 04, 2019 at 05:24:57PM +0100, Niklas Söderlund wrote: > > On 2019-01-03 18:38:55 +0100, Jacopo Mondi wrote: > >> Add a MediaDevice member field to the MediaObject class hierarcy. > >> Each media object now has a reference to the media device it belongs to, > >> and which it has been created by. > >> > >> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > >> --- > >> > >> src/libcamera/include/media_object.h | 10 ++++++---- > >> src/libcamera/media_device.cpp | 8 ++++---- > >> src/libcamera/media_object.cpp | 30 +++++++++++++++++++--------- > >> 3 files changed, 31 insertions(+), 17 deletions(-) > >> > >> diff --git a/src/libcamera/include/media_object.h > >> b/src/libcamera/include/media_object.h index 950a332..0f0bb29 100644 > >> --- a/src/libcamera/include/media_object.h > >> +++ b/src/libcamera/include/media_object.h > >> > >> @@ -26,10 +26,12 @@ public: > >> protected: > >> friend class MediaDevice; > >> > >> - MediaObject(unsigned int id) : id_(id) { } > >> + MediaObject(MediaDevice *media, unsigned int id) : > >> + id_(id), media_(media) { } Should this be named dev_(dev) instead of media_(media) ? > > > > Nit: I would swap the initialization settings if id_(), media_() to > > media_(), id_() in order to match the arguments to the constructor. > > The compiler complains if you do so, as 'id_' is declared before > 'media_'. > > ../src/libcamera/include/media_object.h: In constructor > ‘libcamera::MediaObject::MediaObject(libcamera::MediaDevice*, unsigned > int)’: ../src/libcamera/include/media_object.h:34:15: error: > ‘libcamera::MediaObject::media_’ will be initialized after > [-Werror=reorder] MediaDevice *media_; > ^~~~~~ > ../src/libcamera/include/media_object.h:33:15: error: ‘unsigned int > libcamera::MediaObject::id_’ [-Werror=reorder] unsigned int id_; > ^~~ > ../src/libcamera/include/media_object.h:29:2: error: when initialized here > [-Werror=reorder] MediaObject(MediaDevice *media, unsigned int id) : You can swap the id_ and media_ fields in the class to fix this. > That's possibly the most stupid compiler warning I ever met. https://github.com/isocpp/CppCoreGuidelines/blob/master/ CppCoreGuidelines.md#c47-define-and-initialize-member-variables-in-the-order- of-member-declaration Members are initialized in the order they are declared in the class. Without the warning, the following code would compile, but would lead to confusing behaviour. class Foo { public: Foo(int i) : a(i++), b(i++), c(i++) { } void print() { cout << "a: " << a << ", b: " << b << ", c: " << c << endl; } private: int a; int c; int b; }; int main(int, char*[]) { Foo f(0); f.print(); return 0 } Output: a: 0, b: 2, c: 1 The warning prevents this kind of issue. > > > virtual ~MediaObject() { } > > > > > > unsigned int id_; > > > + MediaDevice *media_; > > > }; > > > > > > class MediaLink : public MediaObject > > > @@ -42,7 +44,7 @@ public: > > > private: > > > friend class MediaDevice; > > > > > > - MediaLink(const struct media_v2_link *link, > > > + MediaLink(MediaDevice *media, const struct media_v2_link *link, > > > MediaPad *source, MediaPad *sink); > > > MediaLink(const MediaLink &) = delete; > > > ~MediaLink() { } > > > @@ -65,7 +67,7 @@ public: > > > private: > > > friend class MediaDevice; > > > > > > - MediaPad(const struct media_v2_pad *pad, MediaEntity *entity); > > > + MediaPad(MediaDevice *media, const struct media_v2_pad *pad, > > > MediaEntity *entity); Shouldn't you break long lines ? You could also do MediaPad::MediaPad(const struct media_v2_pad *pad, MediaEntity *entity) : MediaObject(entity->media_, pad->id), index_(pad->index), entity_(entity), flags_(pad->flags) to avoid passing the media device pointer as an argument. MediaLink could do something similar. Up to you. Another option would be to store the media device pointer in MediaEntity only and access it from there (link -> pad -> entity -> media device) to avoid storing an extra pointer in objects that may not need it. > > > flags_(pad->flags) > > > MediaPad(const MediaPad &) = delete; > > > ~MediaPad(); > > > @@ -93,7 +95,7 @@ public: > > > private: > > > friend class MediaDevice; > > > > > > - MediaEntity(const struct media_v2_entity *entity, > > > + MediaEntity(MediaDevice *media, const struct media_v2_entity *entity, > > > unsigned int major = 0, unsigned int minor = 0); > > > MediaEntity(const MediaEntity &) = delete; > > > ~MediaEntity(); > > > diff --git a/src/libcamera/media_device.cpp > > > b/src/libcamera/media_device.cpp index cf4ff90..2f9490c 100644 > > > --- a/src/libcamera/media_device.cpp > > > +++ b/src/libcamera/media_device.cpp > > > @@ -430,11 +430,11 @@ bool MediaDevice::populateEntities(const struct > > > media_v2_topology &topology) > > > MediaEntity *entity; > > > if (iface) > > > - entity = new MediaEntity(&mediaEntities[i], > > > + entity = new MediaEntity(this, &mediaEntities[i], > > > iface->devnode.major, > > > iface->devnode.minor); > > > else > > > - entity = new MediaEntity(&mediaEntities[i]); > > > + entity = new MediaEntity(this, &mediaEntities[i]); > > > > > > if (!addObject(entity)) { > > > delete entity; > > > @@ -464,7 +464,7 @@ bool MediaDevice::populatePads(const struct > > > media_v2_topology &topology) > > > return false; > > > } > > > > > > - MediaPad *pad = new MediaPad(&mediaPads[i], mediaEntity); > > > + MediaPad *pad = new MediaPad(this, &mediaPads[i], mediaEntity); > > > if (!addObject(pad)) { > > > delete pad; > > > return false; > > > @@ -509,7 +509,7 @@ bool MediaDevice::populateLinks(const struct > > > media_v2_topology &topology) > > > return false; > > > } > > > > > > - MediaLink *link = new MediaLink(&mediaLinks[i], source, sink); > > > + MediaLink *link = new MediaLink(this, &mediaLinks[i], source, sink); > > > if (!addObject(link)) { > > > delete link; > > > return false; > > > diff --git a/src/libcamera/media_object.cpp > > > b/src/libcamera/media_object.cpp index 581e1c0..f1535e6 100644 > > > --- a/src/libcamera/media_object.cpp > > > +++ b/src/libcamera/media_object.cpp > > > @@ -44,14 +44,16 @@ namespace libcamera { > > > > > > * > > > * MediaObject is an abstract base class for all media objects in the > > > media > > > * graph. Every media graph object is identified by an id unique in the > > > media> > > > - * device context, and this base class provides that. > > > + * device context, and this base class provides both of them. > > > > Both? It provides the unique id and what? Maybe I'm slow today :-) I > > assume the other thing is a reference to the MediaDevice? If so how > > about. > > > > MediaObject is an abstract base class for all media objects in the > > media graph. Each object is identified by a reference to the media > > device it belongs to and a unique id withing that media devce. s/devce/device/ > > This base class provide helpers to media objects to keep track of > > these identifiers. > > s/withing/within, s/these identifiers/them, I'd keep "these identifies" instead of "them", as the latter can be confusing (it could refer to "helpers" or "media objects" in that sentence). > and I'll take your suggestion in. > > > > * > > > * \sa MediaEntity, MediaPad, MediaLink > > > */ > > > > > > /** > > > * \fn MediaObject::MediaObject() > > > > > > - * \brief Construct a MediaObject with \a id > > > + * \brief Construct a MediaObject with \a id, globally unique in the > > > MediaDevice To me globally sounds like it is global, not local to a media device. I'd phrase this as "Construct a MediaObject part of the MediaDevice \a media, identified by the \a id unique in within the device". > > > + * \a media > > > + * \param media The media device this object belongs to > > > * \param id The media object id > > > * > > > * The caller shall ensure unicity of the object id in the media device > > > context. > > > @@ -69,6 +71,11 @@ namespace libcamera { > > > * \brief The media object id > > > */ > > > > > > +/** > > > + * \var MediaObject::media_ > > > + * \brief The media device that constructed this object > > > > How about > > > > The media device the media object belongs to > > > > Same comment bellow for documentation of the media argument. With this > > addressed. > > Sorry, I missed what you're referring to. Below here the documentation > for the media argument is: > > > > + * \param media The media device this entity belongs to > > What would you change. > > > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > > > > + */ > > > + > > > /** > > > * \class MediaLink > > > * \brief The MediaLink represents a link between two pads in the media > > > graph. > > > @@ -82,13 +89,14 @@ namespace libcamera { > > > > > > /** > > > * \brief Construct a MediaLink > > > + * \param media The media device this entity belongs to s/entity/link/ > > > * \param link The media link kernel data > > > * \param source The source pad at the origin of the link > > > * \param sink The sink pad at the destination of the link > > > */ > > > -MediaLink::MediaLink(const struct media_v2_link *link, MediaPad > > > *source, > > > - MediaPad *sink) > > > - : MediaObject(link->id), source_(source), > > > +MediaLink::MediaLink(MediaDevice *media, const struct media_v2_link > > > *link, > > > + MediaPad *source, MediaPad *sink) > > > + : MediaObject(media, link->id), source_(source), > > > sink_(sink), flags_(link->flags) > > > { > > > } > > > @@ -135,11 +143,13 @@ MediaLink::MediaLink(const struct media_v2_link > > > *link, MediaPad *source, > > > > > > /** > > > * \brief Construct a MediaPad > > > + * \param media The media device this entity belongs to s/entity/pad/ > > > * \param pad The media pad kernel data > > > * \param entity The entity the pad belongs to > > > */ > > > -MediaPad::MediaPad(const struct media_v2_pad *pad, MediaEntity *entity) > > > - : MediaObject(pad->id), index_(pad->index), entity_(entity), > > > +MediaPad::MediaPad(MediaDevice *media, const struct media_v2_pad *pad, > > > + MediaEntity *entity) > > > + : MediaObject(media, pad->id), index_(pad->index), entity_(entity), > > > flags_(pad->flags) > > > { > > > } > > > @@ -283,13 +293,15 @@ int MediaEntity::setDeviceNode(const std::string > > > &devnode) > > > > > > /** > > > * \brief Construct a MediaEntity > > > + * \param media The media device this entity belongs to This one is correct :-) > > > * \param entity The media entity kernel data > > > * \param major The major number of the entity associated interface > > > * \param minor The minor number of the entity associated interface > > > */ > > > -MediaEntity::MediaEntity(const struct media_v2_entity *entity, > > > +MediaEntity::MediaEntity(MediaDevice *media, > > > + const struct media_v2_entity *entity, > > > unsigned int major, unsigned int minor) > > > - : MediaObject(entity->id), name_(entity->name), > > > + : MediaObject(media, entity->id), name_(entity->name), > > > major_(major), minor_(minor) > > > { > > > }
diff --git a/src/libcamera/include/media_object.h b/src/libcamera/include/media_object.h index 950a332..0f0bb29 100644 --- a/src/libcamera/include/media_object.h +++ b/src/libcamera/include/media_object.h @@ -26,10 +26,12 @@ public: protected: friend class MediaDevice; - MediaObject(unsigned int id) : id_(id) { } + MediaObject(MediaDevice *media, unsigned int id) : + id_(id), media_(media) { } virtual ~MediaObject() { } unsigned int id_; + MediaDevice *media_; }; class MediaLink : public MediaObject @@ -42,7 +44,7 @@ public: private: friend class MediaDevice; - MediaLink(const struct media_v2_link *link, + MediaLink(MediaDevice *media, const struct media_v2_link *link, MediaPad *source, MediaPad *sink); MediaLink(const MediaLink &) = delete; ~MediaLink() { } @@ -65,7 +67,7 @@ public: private: friend class MediaDevice; - MediaPad(const struct media_v2_pad *pad, MediaEntity *entity); + MediaPad(MediaDevice *media, const struct media_v2_pad *pad, MediaEntity *entity); MediaPad(const MediaPad &) = delete; ~MediaPad(); @@ -93,7 +95,7 @@ public: private: friend class MediaDevice; - MediaEntity(const struct media_v2_entity *entity, + MediaEntity(MediaDevice *media, const struct media_v2_entity *entity, unsigned int major = 0, unsigned int minor = 0); MediaEntity(const MediaEntity &) = delete; ~MediaEntity(); diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp index cf4ff90..2f9490c 100644 --- a/src/libcamera/media_device.cpp +++ b/src/libcamera/media_device.cpp @@ -430,11 +430,11 @@ bool MediaDevice::populateEntities(const struct media_v2_topology &topology) MediaEntity *entity; if (iface) - entity = new MediaEntity(&mediaEntities[i], + entity = new MediaEntity(this, &mediaEntities[i], iface->devnode.major, iface->devnode.minor); else - entity = new MediaEntity(&mediaEntities[i]); + entity = new MediaEntity(this, &mediaEntities[i]); if (!addObject(entity)) { delete entity; @@ -464,7 +464,7 @@ bool MediaDevice::populatePads(const struct media_v2_topology &topology) return false; } - MediaPad *pad = new MediaPad(&mediaPads[i], mediaEntity); + MediaPad *pad = new MediaPad(this, &mediaPads[i], mediaEntity); if (!addObject(pad)) { delete pad; return false; @@ -509,7 +509,7 @@ bool MediaDevice::populateLinks(const struct media_v2_topology &topology) return false; } - MediaLink *link = new MediaLink(&mediaLinks[i], source, sink); + MediaLink *link = new MediaLink(this, &mediaLinks[i], source, sink); if (!addObject(link)) { delete link; return false; diff --git a/src/libcamera/media_object.cpp b/src/libcamera/media_object.cpp index 581e1c0..f1535e6 100644 --- a/src/libcamera/media_object.cpp +++ b/src/libcamera/media_object.cpp @@ -44,14 +44,16 @@ namespace libcamera { * * MediaObject is an abstract base class for all media objects in the media * graph. Every media graph object is identified by an id unique in the media - * device context, and this base class provides that. + * device context, and this base class provides both of them. * * \sa MediaEntity, MediaPad, MediaLink */ /** * \fn MediaObject::MediaObject() - * \brief Construct a MediaObject with \a id + * \brief Construct a MediaObject with \a id, globally unique in the MediaDevice + * \a media + * \param media The media device this object belongs to * \param id The media object id * * The caller shall ensure unicity of the object id in the media device context. @@ -69,6 +71,11 @@ namespace libcamera { * \brief The media object id */ +/** + * \var MediaObject::media_ + * \brief The media device that constructed this object + */ + /** * \class MediaLink * \brief The MediaLink represents a link between two pads in the media graph. @@ -82,13 +89,14 @@ namespace libcamera { /** * \brief Construct a MediaLink + * \param media The media device this entity belongs to * \param link The media link kernel data * \param source The source pad at the origin of the link * \param sink The sink pad at the destination of the link */ -MediaLink::MediaLink(const struct media_v2_link *link, MediaPad *source, - MediaPad *sink) - : MediaObject(link->id), source_(source), +MediaLink::MediaLink(MediaDevice *media, const struct media_v2_link *link, + MediaPad *source, MediaPad *sink) + : MediaObject(media, link->id), source_(source), sink_(sink), flags_(link->flags) { } @@ -135,11 +143,13 @@ MediaLink::MediaLink(const struct media_v2_link *link, MediaPad *source, /** * \brief Construct a MediaPad + * \param media The media device this entity belongs to * \param pad The media pad kernel data * \param entity The entity the pad belongs to */ -MediaPad::MediaPad(const struct media_v2_pad *pad, MediaEntity *entity) - : MediaObject(pad->id), index_(pad->index), entity_(entity), +MediaPad::MediaPad(MediaDevice *media, const struct media_v2_pad *pad, + MediaEntity *entity) + : MediaObject(media, pad->id), index_(pad->index), entity_(entity), flags_(pad->flags) { } @@ -283,13 +293,15 @@ int MediaEntity::setDeviceNode(const std::string &devnode) /** * \brief Construct a MediaEntity + * \param media The media device this entity belongs to * \param entity The media entity kernel data * \param major The major number of the entity associated interface * \param minor The minor number of the entity associated interface */ -MediaEntity::MediaEntity(const struct media_v2_entity *entity, +MediaEntity::MediaEntity(MediaDevice *media, + const struct media_v2_entity *entity, unsigned int major, unsigned int minor) - : MediaObject(entity->id), name_(entity->name), + : MediaObject(media, entity->id), name_(entity->name), major_(major), minor_(minor) { }
Add a MediaDevice member field to the MediaObject class hierarcy. Each media object now has a reference to the media device it belongs to, and which it has been created by. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- src/libcamera/include/media_object.h | 10 ++++++---- src/libcamera/media_device.cpp | 8 ++++---- src/libcamera/media_object.cpp | 30 +++++++++++++++++++--------- 3 files changed, 31 insertions(+), 17 deletions(-)