[libcamera-devel] [PATCH 4/4] libcamera: mediadevice: Improve documentation
Jacopo Mondi
jacopo at jmondi.org
Wed Jan 2 10:29:23 CET 2019
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 at 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 at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
-------------- 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/20190102/ea164841/attachment.sig>
More information about the libcamera-devel
mailing list