[libcamera-devel,v2,1/4] libcamera: Add pointer to MediaDevice to MediaObject
diff mbox series

Message ID 20190108170407.4770-2-jacopo@jmondi.org
State Superseded
Headers show
Series
  • libcamera: media device: Add link handling
Related show

Commit Message

Jacopo Mondi Jan. 8, 2019, 5:04 p.m. UTC 
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>
---
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(-)

--
2.20.1

Comments

Laurent Pinchart Jan. 8, 2019, 6:07 p.m. UTC | #1 
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@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_; }

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

>  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 ?

>  		    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 ?

>  /**
>   * \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@ideasonboard.com>
Jacopo Mondi Jan. 8, 2019, 7:33 p.m. UTC | #2 
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@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@ideasonboard.com>
>

Thanks
  j

> --
> Regards,
>
> Laurent Pinchart
>
>
>
Laurent Pinchart Jan. 8, 2019, 8:20 p.m. UTC | #3 
Hi Jacopo,

On Tuesday, 8 January 2019 21:33:51 EET Jacopo Mondi wrote:
> On Tue, Jan 08, 2019 at 08:07:59PM +0200, Laurent Pinchart wrote:
> > 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@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?

Using the proposed API, you can do MediaDevice::link(...) ->setEnabled() and 
that would modify the state of the MediaDevice, even if the MediaDevice 
instance was originally const. I don't think that's a good idea.

> > And I think you can spell the name of the function fully.
> 
> MediaDevice *device() {...}
> ?

Yes.

> >>  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();
> >> 

[snip]

Patch
diff mbox series 

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_; }

 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,
 		    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
+ * \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
+ */
+
 /**
  * \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)
 {
 }