[libcamera-devel] [PATCH v2 1/4] libcamera: Add pointer to MediaDevice to MediaObject

[Jacopo Mondi]

On Tue, Jan 08, 2019 at 08:07:59PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Tuesday, 8 January 2019 19:04:04 EET 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>
> > ---
> > v1->v2:
> > - Use the entity MediaDevice in MediaPad and MediaLink constructors
> > - Incorporate comments changes from Laurent and Niklas on v1
> >
> >  src/libcamera/include/media_object.h |  7 +++++--
> >  src/libcamera/media_device.cpp       |  4 ++--
> >  src/libcamera/media_object.cpp       | 27 +++++++++++++++++++--------
> >  3 files changed, 26 insertions(+), 12 deletions(-)
> >
> > diff --git a/src/libcamera/include/media_object.h
> > b/src/libcamera/include/media_object.h index 04b9a89..d92aab3 100644
> > --- a/src/libcamera/include/media_object.h
> > +++ b/src/libcamera/include/media_object.h
> > @@ -22,13 +22,16 @@ class MediaObject
> >  {
> >  public:
> >  	unsigned int id() const { return id_; }
> > +	MediaDevice *dev() const { return dev_; }
>
> For the reason explained in v1, I would make this either
>
> 	MediaDevice *dev() { return dev_; }
> 	const MediaDevice *dev() const { return dev_; }
>
> or just
>
> 	MediaDevice *dev() { return dev_; }

Ok, I might had missed that... a const function could be called on const
instances, and if you get a link, then you probably want to modify it,
and if you try to do so on a const MediaDevice instance, that will
fail... Is this what you're trying to protect?


>
> And I think you can spell the name of the function fully.
>

MediaDevice *device() {...}
?

> >  protected:
> >  	friend class MediaDevice;
> >
> > -	MediaObject(unsigned int id) : id_(id) { }
> > +	MediaObject(MediaDevice *dev, unsigned int id) :
> > +		dev_(dev), id_(id) { }
> >  	virtual ~MediaObject() { }
> >
> > +	MediaDevice *dev_;
> >  	unsigned int id_;
> >  };
> >
> > @@ -93,7 +96,7 @@ public:
> >  private:
> >  	friend class MediaDevice;
> >
> > -	MediaEntity(const struct media_v2_entity *entity,
> > +	MediaEntity(MediaDevice *media, const struct media_v2_entity *entity,
>
> If MediaObject is constructed with a MediaDevice *dev, should this be
> MediaDevice *dev too ?
>

Yes, I missed that

> >  		    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..b0d10ed 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;
> > diff --git a/src/libcamera/media_object.cpp b/src/libcamera/media_object.cpp
> > index 06a8d64..612550d 100644
> > --- a/src/libcamera/media_object.cpp
> > +++ b/src/libcamera/media_object.cpp
> > @@ -42,16 +42,20 @@ namespace libcamera {
> >   * \class MediaObject
> >   * \brief Base class for all media objects
> >   *
> > - * 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.
> > + * 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 within that media device.
> > + * 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 part of the MediaDevice \a dev,
> > + * identified by the \a id unique in within the device
>
> s/in within/within/
>
> > + * \param dev 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 +73,11 @@ namespace libcamera {
> >   * \brief The media object id
> >   */
> >
> > +/**
> > + * \var MediaObject::dev_
> > + * \brief The media device the media object belongs to
> > + */
> > +
>
> Could you keep this in the same order as in the class definition ?
>

Yes, I have not updated this...

> >  /**
> >   * \class MediaLink
> >   * \brief The MediaLink represents a link between two pads in the media
> > graph. @@ -88,7 +97,7 @@ namespace libcamera {
> >   */
> >  MediaLink::MediaLink(const struct media_v2_link *link, MediaPad *source,
> >  		     MediaPad *sink)
> > -	: MediaObject(link->id), source_(source),
> > +	: MediaObject(source->dev(), link->id), source_(source),
> >  	  sink_(sink), flags_(link->flags)
> >  {
> >  }
> > @@ -139,7 +148,7 @@ MediaLink::MediaLink(const struct media_v2_link *link,
> > MediaPad *source, * \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),
> > +	: MediaObject(entity->dev(), pad->id), index_(pad->index),
> > entity_(entity), flags_(pad->flags)
> >  {
> >  }
> > @@ -283,13 +292,15 @@ int MediaEntity::setDeviceNode(const std::string
> > &devnode)
> >
> >  /**
> >   * \brief Construct a MediaEntity
> > + * \param dev 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 *dev,
> > +			 const struct media_v2_entity *entity,
> >  			 unsigned int major, unsigned int minor)
> > -	: MediaObject(entity->id), name_(entity->name),
> > +	: MediaObject(dev, entity->id), name_(entity->name),
> >  	  major_(major), minor_(minor)
> >  {
> >  }
>
> With the small issues above fixed,
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>

Thanks
  j

> --
> Regards,
>
> Laurent Pinchart
>
>
>
-------------- 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/20190108/ac87607b/attachment-0001.sig>