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

[Laurent Pinchart]

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

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 at ideasonboard.com>

-- 
Regards,

Laurent Pinchart