[libcamera-devel,4/4] libcamera: mediadevice: Improve documentation

120 diff mbox series

Improve the documentation of the media device operation, including how
it handles the lifetime of media objects.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 src/libcamera/include/media_device.h |   2 +-
 src/libcamera/media_device.cpp       | 113 ++++++++++++++++-----------
 2 files changed, 70 insertions(+), 45 deletions(-)

Hi Laurent,

On 01/01/2019 21:23, Laurent Pinchart wrote:
> Improve the documentation of the media device operation, including how
> it handles the lifetime of media objects.
> 

Some potential issues to consider inline.

With those fixed,

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  src/libcamera/include/media_device.h |   2 +-
>  src/libcamera/media_device.cpp       | 113 ++++++++++++++++-----------
>  2 files changed, 70 insertions(+), 45 deletions(-)
> 
> diff --git a/src/libcamera/include/media_device.h b/src/libcamera/include/media_device.h
> index fd1e12d1bbcb..d787be391882 100644
> --- a/src/libcamera/include/media_device.h
> +++ b/src/libcamera/include/media_device.h
> @@ -44,7 +44,7 @@ private:
>  
>  	std::map<unsigned int, MediaObject *> objects_;
>  	MediaObject *object(unsigned int id);
> -	bool addObject(MediaObject *obj);
> +	bool addObject(MediaObject *object);
>  	void clear();
>  
>  	std::vector<MediaEntity *> entities_;
> diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp
> index 4c77d3787391..2470c0be806e 100644
> --- a/src/libcamera/media_device.cpp
> +++ b/src/libcamera/media_device.cpp
> @@ -32,37 +32,41 @@ namespace libcamera {
>   * \brief The MediaDevice represents a Media Controller device with its full
>   * graph of connected objects.
>   *
> - * Media devices are created with an empty graph, which must be populated from
> + * A MediaDevice instance is associated with a media controller device node when
> + * created, and that association is kept for the lifetime of the MediaDevice
> + * instance.
>   *
> + * The instance is created with an empty media graph. Before performing any
> + * other operation, it must be opened with the open() function and the media
> + * graph populated by calling populate(). Instances of MediaEntity, MediaPad and
> + * MediaLink are created to model the media graph, and stored in a map indexed
> + * by object id.
>   *
> - * The caller is responsible for opening the MediaDevice explicitly before
> - * operating on it, and shall close it when not needed anymore, as access
> - * to the MediaDevice is exclusive.

> + * Once successfully populated the graph is valid, as reported by the valid()

"Once successfully populated the graph is valid" sounds like it needs a
breather comma after populated.

Alternatively - re-order as:

The graph is valid once successfully populated, as reported by the
valid() function.



> + * function. It can be queried to list all entities(), or entities can be
> + * looked up by name with getEntityByName(). The graph can be traversed from
> + * entity to entity through pads and links as exposed by the corresponding
> + * classes.
>   *
> - * A MediaDevice is created empty and gets populated by inspecting the media
> - * graph topology using the MEDIA_IOC_G_TOPOLOGY ioctls. Representation
> - * of each entity, pad and link described are created using MediaObject
> - * derived classes.
> - *
> - * All MediaObject are stored in a global pool, where they could be retrieved
> - * from by their globally unique id.
> - *
> - * References to MediaEntity registered in the graph are stored in a vector
> - * to allow easier by-name lookup, and the list of MediaEntities is accessible.
> + * An open media device will keep an open file handle for the underlying media
> + * controller device node. It can be closed at any time with a call to close().
> + * This will not invalidate the media graph and all cached media object remain

s/media object/media objects/

> + * valid and can be accessed normally. The device can then be later reopened if
> + * needed to perform other operations that interect with the device node.

s/interect/interact/ ?

>   */
>  
>  /**
>   * \brief Construct a MediaDevice
>   * \param devnode The media device node path
> + *
> + * Once constructed the media device is invalid, and must be opened and
> + * populated with open() and populate() before the media graph can be queried.
>   */
>  MediaDevice::MediaDevice(const std::string &devnode)
>  	: devnode_(devnode), fd_(-1), valid_(false)
>  {
>  }
>  
> -/**
> - * \brief Close the media device file descriptor and delete all object
> - */
>  MediaDevice::~MediaDevice()
>  {
>  	if (fd_ != -1)
> @@ -71,11 +75,18 @@ MediaDevice::~MediaDevice()
>  }
>  
>  /**
> - * \brief Open a media device and retrieve informations from it
> + * \brief Open a media device and retrieve device information
>   *
> - * The function fails if the media device is already open or if either
> - * open or the media device information retrieval operations fail.
> - * \return 0 for success or a negative error number otherwise
> + * Before populating the media graph or performing any operation that interact
> + * with the device node associated with the media device, the device node must
> + * be opened.
> + *
> + * This function also retrieves media device information from the device node,
> + * which can be queried through driver().
> + *
> + * If the device is already open the function returns -EBUSY.
> + *
> + * \return 0 on success or a negative error code otherwise
>   */
>  int MediaDevice::open()
>  {
> @@ -108,10 +119,17 @@ int MediaDevice::open()
>  }
>  
>  /**
> - * \brief Close the file descriptor associated with the media device.
> + * \brief Close the media device
> + *
> + * This function closes the media device node. It does not invalidate the media
> + * graph and all cached media object remain valid and can be accessed normally.

s/object/objects/

> + * Once closed no operation interacting with the media device node can be
> + * performed until the device is opened again.
>   *
> - * After this function has been called, for the MediaDevice to be operated on,
> - * the caller shall open it again.
> + * Closing an already closed device is allowed and will not performed any

s/performed/perform/

> + * operation.
> + *
> + * \sa open()
>   */
>  void MediaDevice::close()
>  {
> @@ -130,9 +148,9 @@ void MediaDevice::close()
>   * stored as MediaEntity, MediaPad and MediaLink respectively, with cross-
>   * references between objects. Interfaces are not processed.
>   *
> - * MediaEntities are stored in a global list in the MediaDevice itself to ease
> - * lookup, while MediaPads are accessible from the MediaEntity they belong
> - * to only and MediaLinks from the MediaPad they connect.
> + * Entities are stored in a separate list in the MediaDevice to ease lookup,
> + * while pads are accessible from the entity they belong to and links from the
> + * pad they connect.

s/pad/pads/

>   *
>   * \return 0 on success, a negative error code otherwise
>   */
> @@ -241,8 +259,9 @@ MediaEntity *MediaDevice::getEntityByName(const std::string &name)
>   * object id.
>   */
>  
> -/*
> - * MediaObject pool lookup by id.
> +/**
> + * \brief Retrieve the media graph object specified by \a id
> + * \return The graph object, or nullptr if no object with \a id is found
>   */
>  MediaObject *MediaDevice::object(unsigned int id)
>  {
> @@ -250,33 +269,40 @@ MediaObject *MediaDevice::object(unsigned int id)
>  	return (it == objects_.end()) ? nullptr : it->second;
>  }
>  
> -/*
> - * Add a new object to the global objects pool and fail if the object
> - * has already been registered.
> +/**
> + * \brief Add a media object to the media graph
> + *
> + * If the \a object has a unique id it is added to the media graph, and its
> + * lifetime will be managed by the media device. Otherwise the object isn't
> + * added to the graph and the caller must delete it.
> + *
> + * \return true if the object was successfully added to the graph and false
> + * otherwise
>   */
> -bool MediaDevice::addObject(MediaObject *obj)
> +bool MediaDevice::addObject(MediaObject *object)
>  {
>  
> -	if (objects_.find(obj->id()) != objects_.end()) {
> -		LOG(Error) << "Element with id " << obj->id()
> +	if (objects_.find(object->id()) != objects_.end()) {
> +		LOG(Error) << "Element with id " << object->id()
>  			   << " already enumerated.";
>  		return false;
>  	}
>  
> -	objects_[obj->id()] = obj;
> +	objects_[object->id()] = object;
>  
>  	return true;
>  }
>  
>  /**
> - * \brief Delete all media objects in the MediaDevice.
> + * \brief Delete all graph objects in the media device
> + *
> + * Clear the media graph and delete all the objects it contains. After this
> + * function returns any previously obtained pointer to a media graph object
> + * becomes invalid.
>   *
> - * Delete all MediaEntities; entities will then delete their pads,
> - * and each source pad will delete links.
> + * The media device graph state is reset to invalid when the graph is cleared.
>   *
> - * After this function has been called, the media graph will be unpopulated
> - * and its media objects deleted. The media device has to be populated
> - * before it could be used again.
> + * \sa valid()

What does \sa valid() do here?

>   */
>  void MediaDevice::clear()
>  {
> @@ -295,8 +321,7 @@ void MediaDevice::clear()
>  
>  /*
>   * For each entity in the media graph create a MediaEntity and store a
> - * reference in the MediaObject global pool and in the global vector of
> - * entities.
> + * reference in the media device objects map and entities list.
>   */
>  bool MediaDevice::populateEntities(const struct media_v2_topology &topology)
>  {
>

Hi Kieran,

On Wednesday, 2 January 2019 00:06:14 EET Kieran Bingham wrote:
> On 01/01/2019 21:23, Laurent Pinchart wrote:
> > Improve the documentation of the media device operation, including how
> > it handles the lifetime of media objects.
> 
> Some potential issues to consider inline.
> 
> With those fixed,
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > 
> >  src/libcamera/include/media_device.h |   2 +-
> >  src/libcamera/media_device.cpp       | 113 ++++++++++++++++-----------
> >  2 files changed, 70 insertions(+), 45 deletions(-)

[snip]

> > diff --git a/src/libcamera/media_device.cpp
> > b/src/libcamera/media_device.cpp index 4c77d3787391..2470c0be806e 100644
> > --- a/src/libcamera/media_device.cpp
> > +++ b/src/libcamera/media_device.cpp

[snip]

> >  /**
> > - * \brief Delete all media objects in the MediaDevice.
> > + * \brief Delete all graph objects in the media device
> > + *
> > + * Clear the media graph and delete all the objects it contains. After
> > this + * function returns any previously obtained pointer to a media
> > graph object + * becomes invalid.
> >   *
> > - * Delete all MediaEntities; entities will then delete their pads,
> > - * and each source pad will delete links.
> > + * The media device graph state is reset to invalid when the graph is
> > cleared.
> >   *
> > - * After this function has been called, the media graph will be
> > unpopulated - * and its media objects deleted. The media device has to be
> > populated - * before it could be used again.
> > + * \sa valid()
> 
> What does \sa valid() do here?

It links to the valid() function (sa stands for see also).

> >   */
> >  void MediaDevice::clear()
> >  {

Hi Laurent
   thanks for the patch.

The documentation is now better, thanks. Writing proper doc is harder
than writing proper code :/

On Tue, Jan 01, 2019 at 11:23:28PM +0200, Laurent Pinchart wrote:
> Improve the documentation of the media device operation, including how
> it handles the lifetime of media objects.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  src/libcamera/include/media_device.h |   2 +-
>  src/libcamera/media_device.cpp       | 113 ++++++++++++++++-----------
>  2 files changed, 70 insertions(+), 45 deletions(-)
>
> diff --git a/src/libcamera/include/media_device.h b/src/libcamera/include/media_device.h
> index fd1e12d1bbcb..d787be391882 100644
> --- a/src/libcamera/include/media_device.h
> +++ b/src/libcamera/include/media_device.h
> @@ -44,7 +44,7 @@ private:
>
>  	std::map<unsigned int, MediaObject *> objects_;
>  	MediaObject *object(unsigned int id);
> -	bool addObject(MediaObject *obj);
> +	bool addObject(MediaObject *object);
>  	void clear();
>
>  	std::vector<MediaEntity *> entities_;
> diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp
> index 4c77d3787391..2470c0be806e 100644
> --- a/src/libcamera/media_device.cpp
> +++ b/src/libcamera/media_device.cpp
> @@ -32,37 +32,41 @@ namespace libcamera {
>   * \brief The MediaDevice represents a Media Controller device with its full
>   * graph of connected objects.
>   *
> - * Media devices are created with an empty graph, which must be populated from
> + * A MediaDevice instance is associated with a media controller device node when
> + * created, and that association is kept for the lifetime of the MediaDevice
> + * instance.
>   *
> + * The instance is created with an empty media graph. Before performing any
> + * other operation, it must be opened with the open() function and the media
> + * graph populated by calling populate(). Instances of MediaEntity, MediaPad and
> + * MediaLink are created to model the media graph, and stored in a map indexed
> + * by object id.
>   *
> - * The caller is responsible for opening the MediaDevice explicitly before
> - * operating on it, and shall close it when not needed anymore, as access
> - * to the MediaDevice is exclusive.
> + * Once successfully populated the graph is valid, as reported by the valid()
> + * function. It can be queried to list all entities(), or entities can be
> + * looked up by name with getEntityByName(). The graph can be traversed from
> + * entity to entity through pads and links as exposed by the corresponding
> + * classes.
>   *
> - * A MediaDevice is created empty and gets populated by inspecting the media
> - * graph topology using the MEDIA_IOC_G_TOPOLOGY ioctls. Representation
> - * of each entity, pad and link described are created using MediaObject
> - * derived classes.
> - *
> - * All MediaObject are stored in a global pool, where they could be retrieved
> - * from by their globally unique id.
> - *
> - * References to MediaEntity registered in the graph are stored in a vector
> - * to allow easier by-name lookup, and the list of MediaEntities is accessible.
> + * An open media device will keep an open file handle for the underlying media
> + * controller device node. It can be closed at any time with a call to close().
> + * This will not invalidate the media graph and all cached media object remain

s/cached media object/cached media objects/

> + * valid and can be accessed normally. The device can then be later reopened if
> + * needed to perform other operations that interect with the device node.

s/interect/interact


>   */
>
>  /**
>   * \brief Construct a MediaDevice
>   * \param devnode The media device node path
> + *
> + * Once constructed the media device is invalid, and must be opened and
> + * populated with open() and populate() before the media graph can be queried.
>   */
>  MediaDevice::MediaDevice(const std::string &devnode)
>  	: devnode_(devnode), fd_(-1), valid_(false)
>  {
>  }
>
> -/**
> - * \brief Close the media device file descriptor and delete all object
> - */
>  MediaDevice::~MediaDevice()
>  {
>  	if (fd_ != -1)
> @@ -71,11 +75,18 @@ MediaDevice::~MediaDevice()
>  }
>
>  /**
> - * \brief Open a media device and retrieve informations from it
> + * \brief Open a media device and retrieve device information
>   *
> - * The function fails if the media device is already open or if either
> - * open or the media device information retrieval operations fail.
> - * \return 0 for success or a negative error number otherwise
> + * Before populating the media graph or performing any operation that interact
> + * with the device node associated with the media device, the device node must
> + * be opened.
> + *
> + * This function also retrieves media device information from the device node,
> + * which can be queried through driver().
> + *
> + * If the device is already open the function returns -EBUSY.
> + *
> + * \return 0 on success or a negative error code otherwise
>   */
>  int MediaDevice::open()
>  {
> @@ -108,10 +119,17 @@ int MediaDevice::open()
>  }
>
>  /**
> - * \brief Close the file descriptor associated with the media device.
> + * \brief Close the media device
> + *
> + * This function closes the media device node. It does not invalidate the media
> + * graph and all cached media object remain valid and can be accessed normally.

s/cached media object/cached media objects/

> + * Once closed no operation interacting with the media device node can be
> + * performed until the device is opened again.
>   *
> - * After this function has been called, for the MediaDevice to be operated on,
> - * the caller shall open it again.
> + * Closing an already closed device is allowed and will not performed any
> + * operation.

s/perform/performed

Even if I would have said "Closing an already closed device is a safe non-op".

> + *
> + * \sa open()
>   */
>  void MediaDevice::close()
>  {
> @@ -130,9 +148,9 @@ void MediaDevice::close()
>   * stored as MediaEntity, MediaPad and MediaLink respectively, with cross-
>   * references between objects. Interfaces are not processed.
>   *
> - * MediaEntities are stored in a global list in the MediaDevice itself to ease
> - * lookup, while MediaPads are accessible from the MediaEntity they belong
> - * to only and MediaLinks from the MediaPad they connect.
> + * Entities are stored in a separate list in the MediaDevice to ease lookup,
> + * while pads are accessible from the entity they belong to and links from the
> + * pad they connect.

We should establish a rule when to use the type name (eg. MediaEntity)
and when a generic name (eg Entities) in documentation.

>   *
>   * \return 0 on success, a negative error code otherwise
>   */
> @@ -241,8 +259,9 @@ MediaEntity *MediaDevice::getEntityByName(const std::string &name)
>   * object id.
>   */
>
> -/*
> - * MediaObject pool lookup by id.
> +/**
> + * \brief Retrieve the media graph object specified by \a id
> + * \return The graph object, or nullptr if no object with \a id is found
>   */

This is a private method. Should we doxygen them? I originally
commented them without the starting /**

afaict documentation on private methods and fields does not show up in the
generated output even if I'm sure this can be changed.

>  MediaObject *MediaDevice::object(unsigned int id)
>  {
> @@ -250,33 +269,40 @@ MediaObject *MediaDevice::object(unsigned int id)
>  	return (it == objects_.end()) ? nullptr : it->second;
>  }
>
> -/*
> - * Add a new object to the global objects pool and fail if the object
> - * has already been registered.
> +/**
> + * \brief Add a media object to the media graph
> + *
> + * If the \a object has a unique id it is added to the media graph, and its
> + * lifetime will be managed by the media device. Otherwise the object isn't
> + * added to the graph and the caller must delete it.
> + *
> + * \return true if the object was successfully added to the graph and false
> + * otherwise

Same question as above.

Thanks
  j


>   */
> -bool MediaDevice::addObject(MediaObject *obj)
> +bool MediaDevice::addObject(MediaObject *object)
>  {
>
> -	if (objects_.find(obj->id()) != objects_.end()) {
> -		LOG(Error) << "Element with id " << obj->id()
> +	if (objects_.find(object->id()) != objects_.end()) {
> +		LOG(Error) << "Element with id " << object->id()
>  			   << " already enumerated.";
>  		return false;
>  	}
>
> -	objects_[obj->id()] = obj;
> +	objects_[object->id()] = object;
>
>  	return true;
>  }
>
>  /**
> - * \brief Delete all media objects in the MediaDevice.
> + * \brief Delete all graph objects in the media device
> + *
> + * Clear the media graph and delete all the objects it contains. After this
> + * function returns any previously obtained pointer to a media graph object
> + * becomes invalid.
>   *
> - * Delete all MediaEntities; entities will then delete their pads,
> - * and each source pad will delete links.
> + * The media device graph state is reset to invalid when the graph is cleared.
>   *
> - * After this function has been called, the media graph will be unpopulated
> - * and its media objects deleted. The media device has to be populated
> - * before it could be used again.
> + * \sa valid()
>   */
>  void MediaDevice::clear()
>  {
> @@ -295,8 +321,7 @@ void MediaDevice::clear()
>
>  /*
>   * For each entity in the media graph create a MediaEntity and store a
> - * reference in the MediaObject global pool and in the global vector of
> - * entities.
> + * reference in the media device objects map and entities list.
>   */
>  bool MediaDevice::populateEntities(const struct media_v2_topology &topology)
>  {
> --
> Regards,
>
> Laurent Pinchart
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Hi Jacopo,

On Wednesday, 2 January 2019 11:29:23 EET Jacopo Mondi wrote:
> Hi Laurent
>    thanks for the patch.
> 
> The documentation is now better, thanks. Writing proper doc is harder
> than writing proper code :/

Tell me about it :-S

> On Tue, Jan 01, 2019 at 11:23:28PM +0200, Laurent Pinchart wrote:
> > Improve the documentation of the media device operation, including how
> > it handles the lifetime of media objects.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > 
> >  src/libcamera/include/media_device.h |   2 +-
> >  src/libcamera/media_device.cpp       | 113 ++++++++++++++++-----------
> >  2 files changed, 70 insertions(+), 45 deletions(-)
> > 
> > diff --git a/src/libcamera/include/media_device.h
> > b/src/libcamera/include/media_device.h index fd1e12d1bbcb..d787be391882
> > 100644
> > --- a/src/libcamera/include/media_device.h
> > +++ b/src/libcamera/include/media_device.h
> > @@ -44,7 +44,7 @@ private:
> >  	std::map<unsigned int, MediaObject *> objects_;
> >  	MediaObject *object(unsigned int id);
> > -	bool addObject(MediaObject *obj);
> > +	bool addObject(MediaObject *object);
> >  	void clear();
> >  	
> >  	std::vector<MediaEntity *> entities_;
> > diff --git a/src/libcamera/media_device.cpp
> > b/src/libcamera/media_device.cpp index 4c77d3787391..2470c0be806e 100644
> > --- a/src/libcamera/media_device.cpp
> > +++ b/src/libcamera/media_device.cpp
> > @@ -32,37 +32,41 @@ namespace libcamera {
> >   * \brief The MediaDevice represents a Media Controller device with its
> >   full
> >   * graph of connected objects.
> >   *
> > - * Media devices are created with an empty graph, which must be populated
> > from + * A MediaDevice instance is associated with a media controller
> > device node when + * created, and that association is kept for the
> > lifetime of the MediaDevice + * instance.
> >   *
> > + * The instance is created with an empty media graph. Before performing
> > any + * other operation, it must be opened with the open() function and
> > the media + * graph populated by calling populate(). Instances of
> > MediaEntity, MediaPad and + * MediaLink are created to model the media
> > graph, and stored in a map indexed + * by object id.
> >   *
> > - * The caller is responsible for opening the MediaDevice explicitly
> > before
> > - * operating on it, and shall close it when not needed anymore, as access
> > - * to the MediaDevice is exclusive.
> > + * Once successfully populated the graph is valid, as reported by the
> > valid() + * function. It can be queried to list all entities(), or
> > entities can be + * looked up by name with getEntityByName(). The graph
> > can be traversed from + * entity to entity through pads and links as
> > exposed by the corresponding + * classes.
> >   *
> > - * A MediaDevice is created empty and gets populated by inspecting the
> > media - * graph topology using the MEDIA_IOC_G_TOPOLOGY ioctls.
> > Representation - * of each entity, pad and link described are created
> > using MediaObject - * derived classes.
> > - *
> > - * All MediaObject are stored in a global pool, where they could be
> > retrieved - * from by their globally unique id.
> > - *
> > - * References to MediaEntity registered in the graph are stored in a
> > vector - * to allow easier by-name lookup, and the list of MediaEntities
> > is accessible. + * An open media device will keep an open file handle for
> > the underlying media + * controller device node. It can be closed at any
> > time with a call to close(). + * This will not invalidate the media graph
> > and all cached media object remain
> 
> s/cached media object/cached media objects/

Fixed already.

> > + * valid and can be accessed normally. The device can then be later
> > reopened if + * needed to perform other operations that interect with the
> > device node.
> 
> s/interect/interact

Ditto.

> >   */

[snip]

> >  /**
> > - * \brief Close the file descriptor associated with the media device.
> > + * \brief Close the media device
> > + *
> > + * This function closes the media device node. It does not invalidate the
> > media + * graph and all cached media object remain valid and can be
> > accessed normally.
> 
> s/cached media object/cached media objects/

And here too.

> > + * Once closed no operation interacting with the media device node can be
> > + * performed until the device is opened again.
> >   *
> > - * After this function has been called, for the MediaDevice to be
> > operated on, - * the caller shall open it again.
> > + * Closing an already closed device is allowed and will not performed any
> > + * operation.
> 
> s/perform/performed

Same.

> Even if I would have said "Closing an already closed device is a safe
> non-op".

It's usually spelled no-op :-)

> > + *
> > + * \sa open()
> >   */
> >  void MediaDevice::close()
> >  {
> > @@ -130,9 +148,9 @@ void MediaDevice::close()
> >   * stored as MediaEntity, MediaPad and MediaLink respectively, with
> >   cross-
> >   * references between objects. Interfaces are not processed.
> >   *
> > - * MediaEntities are stored in a global list in the MediaDevice itself to
> > ease - * lookup, while MediaPads are accessible from the MediaEntity they
> > belong - * to only and MediaLinks from the MediaPad they connect.
> > + * Entities are stored in a separate list in the MediaDevice to ease
> > lookup, + * while pads are accessible from the entity they belong to and
> > links from the + * pad they connect.
> 
> We should establish a rule when to use the type name (eg. MediaEntity)
> and when a generic name (eg Entities) in documentation.

I agree. When the term is used as a noun I think a generic name should be 
used. This is the case here in my opinion because you use a plural form.

> >   *
> >   * \return 0 on success, a negative error code otherwise
> >   */
> > @@ -241,8 +259,9 @@ MediaEntity *MediaDevice::getEntityByName(const
> > std::string &name)
> >   * object id.
> >   */
> > 
> > -/*
> > - * MediaObject pool lookup by id.
> > +/**
> > + * \brief Retrieve the media graph object specified by \a id
> > + * \return The graph object, or nullptr if no object with \a id is found
> >   */
> 
> This is a private method. Should we doxygen them? I originally
> commented them without the starting /**
> 
> afaict documentation on private methods and fields does not show up in the
> generated output even if I'm sure this can be changed.

I think using the doxygen syntax to document private methods makes sense to be 
consistent. Whether private methods should be documented or not can be 
debated. In general I believe they should when documentation has added value, 
especially when the methods are complex and information not immediately 
visible from reading the code should be conveyed to the library developers. 
I'm fine skipping documentation for simple private methods.

I wish doxygen would still check syntax of comment blocks for private methods, 
even if it doesn't output documentation.

> >  MediaObject *MediaDevice::object(unsigned int id)
> >  {
> > @@ -250,33 +269,40 @@ MediaObject *MediaDevice::object(unsigned int id)
> >  	return (it == objects_.end()) ? nullptr : it->second;
> >  }
> > 
> > -/*
> > - * Add a new object to the global objects pool and fail if the object
> > - * has already been registered.
> > +/**
> > + * \brief Add a media object to the media graph
> > + *
> > + * If the \a object has a unique id it is added to the media graph, and
> > its + * lifetime will be managed by the media device. Otherwise the
> > object isn't + * added to the graph and the caller must delete it.
> > + *
> > + * \return true if the object was successfully added to the graph and
> > false + * otherwise
> 
> Same question as above.

In this case I think it's important to state that a failed call to addObject() 
leaves the responsibility of deleting the object to the caller. The object() 
method above could be left undocumented as it's quite straightforward.

> >   */
> > -bool MediaDevice::addObject(MediaObject *obj)
> > +bool MediaDevice::addObject(MediaObject *object)
> >  {
> > -	if (objects_.find(obj->id()) != objects_.end()) {
> > -		LOG(Error) << "Element with id " << obj->id()
> > +	if (objects_.find(object->id()) != objects_.end()) {
> > +		LOG(Error) << "Element with id " << object->id()
> >  			   << " already enumerated.";
> >  		return false;
> >  	}
> > 
> > -	objects_[obj->id()] = obj;
> > +	objects_[object->id()] = object;
> > 
> >  	return true;
> >  }

[snip]