[libcamera-devel,2/5] libcamera: media_device: Add function to get a MediaLink

146 diff mbox series

Add three overloaded functions 'link()' to retrieve a link between two
pads. Each overloaded implementation exposes a different method to
identify the source and sink entities.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 src/libcamera/include/media_device.h |   6 ++
 src/libcamera/media_device.cpp       | 119 +++++++++++++++++++++++++++
 2 files changed, 125 insertions(+)

Hi Jacopo,

Thanks for your work.

On 2019-01-03 18:38:56 +0100, Jacopo Mondi wrote:
> Add three overloaded functions 'link()' to retrieve a link between two
> pads. Each overloaded implementation exposes a different method to
> identify the source and sink entities.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  src/libcamera/include/media_device.h |   6 ++
>  src/libcamera/media_device.cpp       | 119 +++++++++++++++++++++++++++
>  2 files changed, 125 insertions(+)
> 
> diff --git a/src/libcamera/include/media_device.h b/src/libcamera/include/media_device.h
> index 9f45fc7..3228ad5 100644
> --- a/src/libcamera/include/media_device.h
> +++ b/src/libcamera/include/media_device.h
> @@ -40,6 +40,12 @@ public:
>  	const std::vector<MediaEntity *> &entities() const { return entities_; }
>  	MediaEntity *getEntityByName(const std::string &name) const;
>  
> +	MediaLink *link(const std::string sourceName, unsigned int sourceIdx,
> +			const std::string sinkName, unsigned int sinkIdx) const;
> +	MediaLink *link(const MediaEntity *source, unsigned int sourceIdx,
> +			const MediaEntity *sink, unsigned int sinkIdx) const;
> +	MediaLink *link(const MediaPad *source, const MediaPad *sink) const;
> +
>  private:
>  	std::string driver_;
>  	std::string devnode_;
> diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp
> index 2f9490c..6892300 100644
> --- a/src/libcamera/media_device.cpp
> +++ b/src/libcamera/media_device.cpp
> @@ -306,6 +306,125 @@ MediaEntity *MediaDevice::getEntityByName(const std::string &name) const
>  	return nullptr;
>  }
>  
> +/**
> + * \brief Return the MediaLink that connects two entities identified by name
> + * \param sourceName The source entity name
> + * \param sourceIdx The index of the source pad
> + * \param sinkName The sink entity name
> + * \param sinkIdx The index of the sink pad
> + *
> + * Find link that connects the pads at index \a sourceIdx of the source
> + * entity with name \a sourceName, to the pad at index \a sinkIdx of the
> + * sink entity with name \a sinkName, if any.
> + *
> + * \sa MediaDevice::link(const MediaEntity *source, unsigned int sourceIdx, const MediaEntity *sink, unsigned int sinkIdx) const
> + * \sa MediaDevice::link(const MediaPad *source, const MediaPad *sink) const
> + *
> + * \return The link that connects the two entities, nullptr otherwise
> + */
> +MediaLink *MediaDevice::link(const std::string sourceName, unsigned int sourceIdx,
> +			     const std::string sinkName, unsigned int sinkIdx) const
> +{
> +	const MediaEntity *source = getEntityByName(sourceName);
> +	if (!source) {
> +		LOG(Error) << "Failed to find entity with name: "
> +			   << sourceName << "\n";
> +		return nullptr;
> +	}
> +
> +	const MediaPad *sourcePad = source->getPadByIndex(sourceIdx);
> +	if (!sourcePad) {
> +		LOG(Error) << "Entity \"" << sourceName << "\" "
> +			   << "has no pad at index " << sourceIdx << "\n";
> +		return nullptr;
> +	}
> +
> +	const MediaEntity *sink = getEntityByName(sinkName);
> +	if (!sink) {
> +		LOG(Error) << "Failed to find entity with name: "
> +			   << sinkName << "\n";
> +		return nullptr;
> +	}
> +
> +	const MediaPad *sinkPad = sink->getPadByIndex(sinkIdx);
> +	if (!sinkPad) {
> +		LOG(Error) << "Entity \"" << sinkName << "\" "
> +			   << "has no pad at index " << sinkIdx << "\n";
> +		return nullptr;
> +	}
> +
> +	return link(sourcePad, sinkPad);

How about just resolving the source and sink MediaEntity's here and then

    return link(source, sourceIdx, sink, sinkIdx);

This would reduce the code duplication.

> +}
> +
> +/**
> + * \brief Return the MediaLink that connects two entities
> + * \param source The source entity
> + * \param sourceIdx The index of the source pad
> + * \param sink The sink entity
> + * \param sinkIdx The index of the sink pad
> + *
> + * Find link that connects the pads at index \a sourceIdx of the source
> + * entity \a source, to the pad at index \a sinkIdx of the sink entity \a
> + * sink, if any.
> + *
> + * \sa MediaDevice::link(const std::string sourceName, unsigned int sourceIdx, const std::string sinkName, unsigned int sinkIdx) const
> + * \sa MediaDevice::link(const MediaPad *source, const MediaPad *sink) const
> + *
> + * \return The link that connects the two entities, nullptr otherwise
> + */
> +MediaLink *MediaDevice::link(const MediaEntity *source, unsigned int sourceIdx,
> +			     const MediaEntity *sink, unsigned int sinkIdx) const
> +{
> +
> +	const MediaPad *sourcePad = source->getPadByIndex(sourceIdx);
> +	if (!sourcePad) {
> +		LOG(Error) << "Entity \"" << source->name() << "\" "
> +			   << "has no pad at index " << sourceIdx << "\n";
> +		return nullptr;
> +	}
> +
> +	const MediaPad *sinkPad = sink->getPadByIndex(sinkIdx);
> +	if (!sinkPad) {
> +		LOG(Error) << "Entity \"" << sink->name() << "\" "
> +			   << "has no pad at index " << sinkIdx << "\n";
> +		return nullptr;
> +	}
> +
> +	return link(sourcePad, sinkPad);
> +}
> +
> +/**
> + * \brief Return the MediaLink that connects two pads
> + * \param source The source pad
> + * \param sink The sink pad
> + *
> + * \sa MediaDevice::link(const std::string sourceName, unsigned int sourceIdx, const std::string sinkName, unsigned int sinkIdx) const
> + * \sa MediaDevice::link(const MediaEntity *source, unsigned int sourceIdx, const MediaEntity *sink, unsigned int sinkIdx) const
> + *
> + * \return The link that connects the two pads, nullptr otherwise
> + */
> +MediaLink *MediaDevice::link(const MediaPad *source, const MediaPad *sink) const
> +{
> +	if (!source || !sink)
> +		return nullptr;
> +
> +	/*
> +	 * Now that we have made sure all instances are valid, compare
> +	 * their ids to find a matching link.
> +	 */

I like comments in the code it self. I don't like (but I still write 
them my self) comments that are written in this tens, how about.

    Compare pad ids to find the link which connects the source and sink 
    pads.

> +	for (MediaLink *link : source->links()) {
> +		if (link->sink()->id() != sink->id())
> +			continue;
> +
> +		if (link->sink()->entity()->id() != sink->entity()->id())
> +			continue;
> +
> +		return link;
> +	}
> +
> +	return nullptr;
> +}
> +
>  /**
>   * \var MediaDevice::objects_
>   * \brief Global map of media objects (entities, pads, links) keyed by their
> -- 
> 2.20.1
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Hi Niklas,

On Fri, Jan 04, 2019 at 05:33:16PM +0100, Niklas Söderlund wrote:
> Hi Jacopo,
>
> Thanks for your work.
>
> On 2019-01-03 18:38:56 +0100, Jacopo Mondi wrote:
> > Add three overloaded functions 'link()' to retrieve a link between two
> > pads. Each overloaded implementation exposes a different method to
> > identify the source and sink entities.
> >
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  src/libcamera/include/media_device.h |   6 ++
> >  src/libcamera/media_device.cpp       | 119 +++++++++++++++++++++++++++
> >  2 files changed, 125 insertions(+)
> >
> > diff --git a/src/libcamera/include/media_device.h b/src/libcamera/include/media_device.h
> > index 9f45fc7..3228ad5 100644
> > --- a/src/libcamera/include/media_device.h
> > +++ b/src/libcamera/include/media_device.h
> > @@ -40,6 +40,12 @@ public:
> >  	const std::vector<MediaEntity *> &entities() const { return entities_; }
> >  	MediaEntity *getEntityByName(const std::string &name) const;
> >
> > +	MediaLink *link(const std::string sourceName, unsigned int sourceIdx,
> > +			const std::string sinkName, unsigned int sinkIdx) const;
> > +	MediaLink *link(const MediaEntity *source, unsigned int sourceIdx,
> > +			const MediaEntity *sink, unsigned int sinkIdx) const;
> > +	MediaLink *link(const MediaPad *source, const MediaPad *sink) const;
> > +
> >  private:
> >  	std::string driver_;
> >  	std::string devnode_;
> > diff --git a/src/libcamera/media_device.cpp b/src/libcamera/media_device.cpp
> > index 2f9490c..6892300 100644
> > --- a/src/libcamera/media_device.cpp
> > +++ b/src/libcamera/media_device.cpp
> > @@ -306,6 +306,125 @@ MediaEntity *MediaDevice::getEntityByName(const std::string &name) const
> >  	return nullptr;
> >  }
> >
> > +/**
> > + * \brief Return the MediaLink that connects two entities identified by name
> > + * \param sourceName The source entity name
> > + * \param sourceIdx The index of the source pad
> > + * \param sinkName The sink entity name
> > + * \param sinkIdx The index of the sink pad
> > + *
> > + * Find link that connects the pads at index \a sourceIdx of the source
> > + * entity with name \a sourceName, to the pad at index \a sinkIdx of the
> > + * sink entity with name \a sinkName, if any.
> > + *
> > + * \sa MediaDevice::link(const MediaEntity *source, unsigned int sourceIdx, const MediaEntity *sink, unsigned int sinkIdx) const
> > + * \sa MediaDevice::link(const MediaPad *source, const MediaPad *sink) const
> > + *
> > + * \return The link that connects the two entities, nullptr otherwise
> > + */
> > +MediaLink *MediaDevice::link(const std::string sourceName, unsigned int sourceIdx,
> > +			     const std::string sinkName, unsigned int sinkIdx) const
> > +{
> > +	const MediaEntity *source = getEntityByName(sourceName);
> > +	if (!source) {
> > +		LOG(Error) << "Failed to find entity with name: "
> > +			   << sourceName << "\n";
> > +		return nullptr;
> > +	}
> > +
> > +	const MediaPad *sourcePad = source->getPadByIndex(sourceIdx);
> > +	if (!sourcePad) {
> > +		LOG(Error) << "Entity \"" << sourceName << "\" "
> > +			   << "has no pad at index " << sourceIdx << "\n";
> > +		return nullptr;
> > +	}
> > +
> > +	const MediaEntity *sink = getEntityByName(sinkName);
> > +	if (!sink) {
> > +		LOG(Error) << "Failed to find entity with name: "
> > +			   << sinkName << "\n";
> > +		return nullptr;
> > +	}
> > +
> > +	const MediaPad *sinkPad = sink->getPadByIndex(sinkIdx);
> > +	if (!sinkPad) {
> > +		LOG(Error) << "Entity \"" << sinkName << "\" "
> > +			   << "has no pad at index " << sinkIdx << "\n";
> > +		return nullptr;
> > +	}
> > +
> > +	return link(sourcePad, sinkPad);
>
> How about just resolving the source and sink MediaEntity's here and then
>
>     return link(source, sourceIdx, sink, sinkIdx);
>
> This would reduce the code duplication.
>

Ah, right, no need to search for the source and sink pads here.
I'll fix.

> > +}
> > +
> > +/**
> > + * \brief Return the MediaLink that connects two entities
> > + * \param source The source entity
> > + * \param sourceIdx The index of the source pad
> > + * \param sink The sink entity
> > + * \param sinkIdx The index of the sink pad
> > + *
> > + * Find link that connects the pads at index \a sourceIdx of the source
> > + * entity \a source, to the pad at index \a sinkIdx of the sink entity \a
> > + * sink, if any.
> > + *
> > + * \sa MediaDevice::link(const std::string sourceName, unsigned int sourceIdx, const std::string sinkName, unsigned int sinkIdx) const
> > + * \sa MediaDevice::link(const MediaPad *source, const MediaPad *sink) const
> > + *
> > + * \return The link that connects the two entities, nullptr otherwise
> > + */
> > +MediaLink *MediaDevice::link(const MediaEntity *source, unsigned int sourceIdx,
> > +			     const MediaEntity *sink, unsigned int sinkIdx) const
> > +{
> > +
> > +	const MediaPad *sourcePad = source->getPadByIndex(sourceIdx);
> > +	if (!sourcePad) {
> > +		LOG(Error) << "Entity \"" << source->name() << "\" "
> > +			   << "has no pad at index " << sourceIdx << "\n";
> > +		return nullptr;
> > +	}
> > +
> > +	const MediaPad *sinkPad = sink->getPadByIndex(sinkIdx);
> > +	if (!sinkPad) {
> > +		LOG(Error) << "Entity \"" << sink->name() << "\" "
> > +			   << "has no pad at index " << sinkIdx << "\n";
> > +		return nullptr;
> > +	}
> > +
> > +	return link(sourcePad, sinkPad);
> > +}
> > +
> > +/**
> > + * \brief Return the MediaLink that connects two pads
> > + * \param source The source pad
> > + * \param sink The sink pad
> > + *
> > + * \sa MediaDevice::link(const std::string sourceName, unsigned int sourceIdx, const std::string sinkName, unsigned int sinkIdx) const
> > + * \sa MediaDevice::link(const MediaEntity *source, unsigned int sourceIdx, const MediaEntity *sink, unsigned int sinkIdx) const
> > + *
> > + * \return The link that connects the two pads, nullptr otherwise
> > + */
> > +MediaLink *MediaDevice::link(const MediaPad *source, const MediaPad *sink) const
> > +{
> > +	if (!source || !sink)
> > +		return nullptr;
> > +
> > +	/*
> > +	 * Now that we have made sure all instances are valid, compare
> > +	 * their ids to find a matching link.
> > +	 */
>
> I like comments in the code it self. I don't like (but I still write
> them my self) comments that are written in this tens, how about.
>
>     Compare pad ids to find the link which connects the source and sink
>     pads.
>

Ack for this and for the comments style in general.

Thanks
  j

> > +	for (MediaLink *link : source->links()) {
> > +		if (link->sink()->id() != sink->id())
> > +			continue;
> > +
> > +		if (link->sink()->entity()->id() != sink->entity()->id())
> > +			continue;
> > +
> > +		return link;
> > +	}
> > +
> > +	return nullptr;
> > +}
> > +
> >  /**
> >   * \var MediaDevice::objects_
> >   * \brief Global map of media objects (entities, pads, links) keyed by their
> > --
> > 2.20.1
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel@lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
>
> --
> Regards,
> Niklas Söderlund

Hello,

On Monday, 7 January 2019 10:57:30 EET Jacopo Mondi wrote:
> On Fri, Jan 04, 2019 at 05:33:16PM +0100, Niklas Söderlund wrote:
> > On 2019-01-03 18:38:56 +0100, Jacopo Mondi wrote:
> > > Add three overloaded functions 'link()' to retrieve a link between two
> > > pads. Each overloaded implementation exposes a different method to
> > > identify the source and sink entities.
> > > 
> > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > ---
> > > 
> > >  src/libcamera/include/media_device.h |   6 ++
> > >  src/libcamera/media_device.cpp       | 119 +++++++++++++++++++++++++++
> > >  2 files changed, 125 insertions(+)

[snip]

> > > diff --git a/src/libcamera/media_device.cpp
> > > b/src/libcamera/media_device.cpp index 2f9490c..6892300 100644
> > > --- a/src/libcamera/media_device.cpp
> > > +++ b/src/libcamera/media_device.cpp
> > > @@ -306,6 +306,125 @@ MediaEntity *MediaDevice::getEntityByName(const
> > > std::string &name) const
> > >  	return nullptr;
> > >  }
> > > 
> > > +/**
> > > + * \brief Return the MediaLink that connects two entities identified by
> > > name

s/Return/Retrieve/

The link doesn't actually connect the two entities directly, it connects two 
pads. I'd write this as

"Retrieve the MediaLink connecting two pads, identified by entity name and pad 
index."

> > > + * \param sourceName The source entity name
> > > + * \param sourceIdx The index of the source pad
> > > + * \param sinkName The sink entity name
> > > + * \param sinkIdx The index of the sink pad
> > > + *
> > > + * Find link that connects the pads at index \a sourceIdx of the source

s/Find/Find the/

> > > + * entity with name \a sourceName, to the pad at index \a sinkIdx of
> > > the
> > > + * sink entity with name \a sinkName, if any.
> > > + *
> > > + * \sa MediaDevice::link(const MediaEntity *source, unsigned int
> > > sourceIdx, const MediaEntity *sink, unsigned int sinkIdx) const + * \sa
> > > MediaDevice::link(const MediaPad *source, const MediaPad *sink) const +
> > > *
> > > + * \return The link that connects the two entities, nullptr otherwise

", or nullptr if no such link exists"

> > > + */
> > > +MediaLink *MediaDevice::link(const std::string sourceName, unsigned int
> > > sourceIdx,
> > > +			     const std::string sinkName, unsigned int sinkIdx)
> > > const

Isn't it dangerous to create a function that allows modifying a link from a 
const MediaDevice ? Should you provide both

	MediaLink *MediaDevice::link(...)
	const MediaLink *MediaDevice::link(...) const

? If you don't want to provide both I think you could go for the former only, 
as I don't think we'll have to deal with const MediaDevice pointers.

> > > +{
> > > +	const MediaEntity *source = getEntityByName(sourceName);
> > > +	if (!source) {
> > > +		LOG(Error) << "Failed to find entity with name: "
> > > +			   << sourceName << "\n";

I'd go for LOG(Debug) at most. I think it's a valid use case for a user to 
call this function to get a link without knowing whether it is present in the 
graph. This can be used to test for the availability of a link in a media 
device that supports multiple configurations. Even as a debug message I'm not 
sure how valuable this is. Same for the messages below.

All these comments apply to the other two functions below.

> > > +		return nullptr;
> > > +	}
> > > +
> > > +	const MediaPad *sourcePad = source->getPadByIndex(sourceIdx);
> > > +	if (!sourcePad) {
> > > +		LOG(Error) << "Entity \"" << sourceName << "\" "
> > > +			   << "has no pad at index " << sourceIdx << "\n";
> > > +		return nullptr;
> > > +	}
> > > +
> > > +	const MediaEntity *sink = getEntityByName(sinkName);
> > > +	if (!sink) {
> > > +		LOG(Error) << "Failed to find entity with name: "
> > > +			   << sinkName << "\n";
> > > +		return nullptr;
> > > +	}
> > > +
> > > +	const MediaPad *sinkPad = sink->getPadByIndex(sinkIdx);
> > > +	if (!sinkPad) {
> > > +		LOG(Error) << "Entity \"" << sinkName << "\" "
> > > +			   << "has no pad at index " << sinkIdx << "\n";
> > > +		return nullptr;
> > > +	}
> > > +
> > > +	return link(sourcePad, sinkPad);
> > 
> > How about just resolving the source and sink MediaEntity's here and then
> > 
> >     return link(source, sourceIdx, sink, sinkIdx);
> > 
> > This would reduce the code duplication.
> 
> Ah, right, no need to search for the source and sink pads here.
> I'll fix.
> 
> > > +}
> > > +
> > > +/**
> > > + * \brief Return the MediaLink that connects two entities
> > > + * \param source The source entity
> > > + * \param sourceIdx The index of the source pad
> > > + * \param sink The sink entity
> > > + * \param sinkIdx The index of the sink pad
> > > + *
> > > + * Find link that connects the pads at index \a sourceIdx of the source
> > > + * entity \a source, to the pad at index \a sinkIdx of the sink entity
> > > \a
> > > + * sink, if any.
> > > + *
> > > + * \sa MediaDevice::link(const std::string sourceName, unsigned int
> > > sourceIdx, const std::string sinkName, unsigned int sinkIdx) const + *
> > > \sa MediaDevice::link(const MediaPad *source, const MediaPad *sink)
> > > const + *
> > > + * \return The link that connects the two entities, nullptr otherwise
> > > + */
> > > +MediaLink *MediaDevice::link(const MediaEntity *source, unsigned int
> > > sourceIdx,
> > > +			     const MediaEntity *sink, unsigned int sinkIdx)
> > > const
> > > +{
> > > +
> > > +	const MediaPad *sourcePad = source->getPadByIndex(sourceIdx);
> > > +	if (!sourcePad) {
> > > +		LOG(Error) << "Entity \"" << source->name() << "\" "
> > > +			   << "has no pad at index " << sourceIdx << "\n";
> > > +		return nullptr;
> > > +	}
> > > +
> > > +	const MediaPad *sinkPad = sink->getPadByIndex(sinkIdx);
> > > +	if (!sinkPad) {
> > > +		LOG(Error) << "Entity \"" << sink->name() << "\" "
> > > +			   << "has no pad at index " << sinkIdx << "\n";
> > > +		return nullptr;
> > > +	}
> > > +
> > > +	return link(sourcePad, sinkPad);
> > > +}
> > > +
> > > +/**
> > > + * \brief Return the MediaLink that connects two pads
> > > + * \param source The source pad
> > > + * \param sink The sink pad
> > > + *
> > > + * \sa MediaDevice::link(const std::string sourceName, unsigned int
> > > sourceIdx, const std::string sinkName, unsigned int sinkIdx) const + *
> > > \sa MediaDevice::link(const MediaEntity *source, unsigned int
> > > sourceIdx, const MediaEntity *sink, unsigned int sinkIdx) const
> > > + *
> > > + * \return The link that connects the two pads, nullptr otherwise
> > > + */
> > > +MediaLink *MediaDevice::link(const MediaPad *source, const MediaPad
> > > *sink) const
> > > +{
> > > +	if (!source || !sink)
> > > +		return nullptr;

Can this happen ?

> > > +	/*
> > > +	 * Now that we have made sure all instances are valid, compare
> > > +	 * their ids to find a matching link.
> > > +	 */
> > 
> > I like comments in the code it self. I don't like (but I still write
> > them my self) comments that are written in this tens, how about.
> > 
> >     Compare pad ids to find the link which connects the source and sink
> >     pads.
> 
> Ack for this and for the comments style in general.
> 
> > > +	for (MediaLink *link : source->links()) {
> > > +		if (link->sink()->id() != sink->id())
> > > +			continue;
> > > +
> > > +		if (link->sink()->entity()->id() != sink->entity()->id())
> > > +			continue;

Do you need this check given that the id is unique ? It seems to be only 
needed if you search based on pad index, not when searching on pad id. You 
could thus write

		if (link->sink()->id() == sink->id())
			return link;

> > > +		return link;
> > > +	}
> > > +
> > > +	return nullptr;
> > > +}
> > > +
> > >  /**
> > >   * \var MediaDevice::objects_
> > >   * \brief Global map of media objects (entities, pads, links) keyed by
> > >   their

Hi Laurent,

On Mon, Jan 07, 2019 at 11:39:43PM +0200, Laurent Pinchart wrote:
> Hello,
>
[snip]

> > > > + */
> > > > +MediaLink *MediaDevice::link(const std::string sourceName, unsigned int
> > > > sourceIdx,
> > > > +			     const std::string sinkName, unsigned int sinkIdx)
> > > > const
>
> Isn't it dangerous to create a function that allows modifying a link from a
> const MediaDevice ? Should you provide both
>
> 	MediaLink *MediaDevice::link(...)
> 	const MediaLink *MediaDevice::link(...) const
>
> ? If you don't want to provide both I think you could go for the former only,
> as I don't think we'll have to deal with const MediaDevice pointers.

The function is declared as:
MediaLink *MediaDevice::link(...) const
and not
const MediaLink *MediaDevice::link(...) const

and indeed the function itself does not change the MediaDevice
instance state. Would you drop the const at end of the declaration?

Thanks
   j


>
> > > > +{
> > > > +	const MediaEntity *source = getEntityByName(sourceName);
> > > > +	if (!source) {
> > > > +		LOG(Error) << "Failed to find entity with name: "
> > > > +			   << sourceName << "\n";
>
> I'd go for LOG(Debug) at most. I think it's a valid use case for a user to
> call this function to get a link without knowing whether it is present in the
> graph. This can be used to test for the availability of a link in a media
> device that supports multiple configurations. Even as a debug message I'm not
> sure how valuable this is. Same for the messages below.
>
> All these comments apply to the other two functions below.
>
> > > > +		return nullptr;
> > > > +	}
> > > > +
> > > > +	const MediaPad *sourcePad = source->getPadByIndex(sourceIdx);
> > > > +	if (!sourcePad) {
> > > > +		LOG(Error) << "Entity \"" << sourceName << "\" "
> > > > +			   << "has no pad at index " << sourceIdx << "\n";
> > > > +		return nullptr;
> > > > +	}
> > > > +
> > > > +	const MediaEntity *sink = getEntityByName(sinkName);
> > > > +	if (!sink) {
> > > > +		LOG(Error) << "Failed to find entity with name: "
> > > > +			   << sinkName << "\n";
> > > > +		return nullptr;
> > > > +	}
> > > > +
> > > > +	const MediaPad *sinkPad = sink->getPadByIndex(sinkIdx);
> > > > +	if (!sinkPad) {
> > > > +		LOG(Error) << "Entity \"" << sinkName << "\" "
> > > > +			   << "has no pad at index " << sinkIdx << "\n";
> > > > +		return nullptr;
> > > > +	}
> > > > +
> > > > +	return link(sourcePad, sinkPad);
> > >
> > > How about just resolving the source and sink MediaEntity's here and then
> > >
> > >     return link(source, sourceIdx, sink, sinkIdx);
> > >
> > > This would reduce the code duplication.
> >
> > Ah, right, no need to search for the source and sink pads here.
> > I'll fix.
> >
> > > > +}
> > > > +
> > > > +/**
> > > > + * \brief Return the MediaLink that connects two entities
> > > > + * \param source The source entity
> > > > + * \param sourceIdx The index of the source pad
> > > > + * \param sink The sink entity
> > > > + * \param sinkIdx The index of the sink pad
> > > > + *
> > > > + * Find link that connects the pads at index \a sourceIdx of the source
> > > > + * entity \a source, to the pad at index \a sinkIdx of the sink entity
> > > > \a
> > > > + * sink, if any.
> > > > + *
> > > > + * \sa MediaDevice::link(const std::string sourceName, unsigned int
> > > > sourceIdx, const std::string sinkName, unsigned int sinkIdx) const + *
> > > > \sa MediaDevice::link(const MediaPad *source, const MediaPad *sink)
> > > > const + *
> > > > + * \return The link that connects the two entities, nullptr otherwise
> > > > + */
> > > > +MediaLink *MediaDevice::link(const MediaEntity *source, unsigned int
> > > > sourceIdx,
> > > > +			     const MediaEntity *sink, unsigned int sinkIdx)
> > > > const
> > > > +{
> > > > +
> > > > +	const MediaPad *sourcePad = source->getPadByIndex(sourceIdx);
> > > > +	if (!sourcePad) {
> > > > +		LOG(Error) << "Entity \"" << source->name() << "\" "
> > > > +			   << "has no pad at index " << sourceIdx << "\n";
> > > > +		return nullptr;
> > > > +	}
> > > > +
> > > > +	const MediaPad *sinkPad = sink->getPadByIndex(sinkIdx);
> > > > +	if (!sinkPad) {
> > > > +		LOG(Error) << "Entity \"" << sink->name() << "\" "
> > > > +			   << "has no pad at index " << sinkIdx << "\n";
> > > > +		return nullptr;
> > > > +	}
> > > > +
> > > > +	return link(sourcePad, sinkPad);
> > > > +}
> > > > +
> > > > +/**
> > > > + * \brief Return the MediaLink that connects two pads
> > > > + * \param source The source pad
> > > > + * \param sink The sink pad
> > > > + *
> > > > + * \sa MediaDevice::link(const std::string sourceName, unsigned int
> > > > sourceIdx, const std::string sinkName, unsigned int sinkIdx) const + *
> > > > \sa MediaDevice::link(const MediaEntity *source, unsigned int
> > > > sourceIdx, const MediaEntity *sink, unsigned int sinkIdx) const
> > > > + *
> > > > + * \return The link that connects the two pads, nullptr otherwise
> > > > + */
> > > > +MediaLink *MediaDevice::link(const MediaPad *source, const MediaPad
> > > > *sink) const
> > > > +{
> > > > +	if (!source || !sink)
> > > > +		return nullptr;
>
> Can this happen ?
>
> > > > +	/*
> > > > +	 * Now that we have made sure all instances are valid, compare
> > > > +	 * their ids to find a matching link.
> > > > +	 */
> > >
> > > I like comments in the code it self. I don't like (but I still write
> > > them my self) comments that are written in this tens, how about.
> > >
> > >     Compare pad ids to find the link which connects the source and sink
> > >     pads.
> >
> > Ack for this and for the comments style in general.
> >
> > > > +	for (MediaLink *link : source->links()) {
> > > > +		if (link->sink()->id() != sink->id())
> > > > +			continue;
> > > > +
> > > > +		if (link->sink()->entity()->id() != sink->entity()->id())
> > > > +			continue;
>
> Do you need this check given that the id is unique ? It seems to be only
> needed if you search based on pad index, not when searching on pad id. You
> could thus write
>
> 		if (link->sink()->id() == sink->id())
> 			return link;
>
> > > > +		return link;
> > > > +	}
> > > > +
> > > > +	return nullptr;
> > > > +}
> > > > +
> > > >  /**
> > > >   * \var MediaDevice::objects_
> > > >   * \brief Global map of media objects (entities, pads, links) keyed by
> > > >   their
>
> --
> Regards,
>
> Laurent Pinchart
>
>
>

Hi Jacopo,

On Tuesday, 8 January 2019 15:49:33 EET Jacopo Mondi wrote:
> On Mon, Jan 07, 2019 at 11:39:43PM +0200, Laurent Pinchart wrote:
> > Hello,
> 
> [snip]
> 
> >>>> + */
> >>>> +MediaLink *MediaDevice::link(const std::string sourceName, unsigned
> >>>> int sourceIdx,
> >>>> +			     const std::string sinkName, unsigned int sinkIdx)
> >>>> const
> > 
> > Isn't it dangerous to create a function that allows modifying a link from
> > a const MediaDevice ? Should you provide both
> > 
> > 	MediaLink *MediaDevice::link(...)
> > 	const MediaLink *MediaDevice::link(...) const
> > 
> > ? If you don't want to provide both I think you could go for the former
> > only, as I don't think we'll have to deal with const MediaDevice
> > pointers.
> 
> The function is declared as:
> MediaLink *MediaDevice::link(...) const
> and not
> const MediaLink *MediaDevice::link(...) const
> 
> and indeed the function itself does not change the MediaDevice
> instance state. Would you drop the const at end of the declaration?

I think I would do so, as it's a bit misleading otherwise. True, the function 
doesn't change the MediaDevice instance itself, but it allows non-const access 
to a link, which is part of the graph. If we give a const MediaDevice * 
pointer to someone, I wouldn't expect the API to allow changing links.

[snip]