[libcamera-devel] [PATCH 1/5] libcamera: Add pointer to MediaDevice to MediaObject
Jacopo Mondi
jacopo at jmondi.org
Mon Jan 7 09:52:55 CET 2019
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 at 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 at 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 at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
>
> --
> Regards,
> Niklas Söderlund
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20190107/5ccd1a6c/attachment-0001.sig>
More information about the libcamera-devel
mailing list