[libcamera-devel] [PATCH 10/13] libcamera: pipeline: simple: Setup links in the context of sink entities

[Jacopo Mondi]

On Mon, Aug 01, 2022 at 10:55:56PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Mon, Aug 01, 2022 at 05:54:13PM +0200, Jacopo Mondi wrote:
> > On Mon, Aug 01, 2022 at 03:05:40AM +0300, Laurent Pinchart via libcamera-devel wrote:
> > > To setup links the pipeline handler iterates over all entities in the
> > > pipeline and disables all links but the ones we want to enable. Some
> > > entities don't allow multiple sink links to be enabled, so the iteration
> > > needs to be based on sink entities, disabling all their links first and
> > > then enabling the sink link that is part of the pipeline.
> > >
> > > The loop implementation iterates over all Entity instances, and uses
> > > their source link to locate the MediaEntity for the connected sink. The
> > > sink entity is then processed in the context of the source's loop
> > > iteration. This prevents the code from being able to access extra
> > > information about the sink entity, as we only have access to the
> > > MediaEntity, not the Entity.
> > >
> > > To prepare for subdev routing support that will require accessing
> > > additional entity information, refactor the loop to process the sink
> > > entity in the context of its Entity instance.
> > >
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> >
> > I guess that's easier than storing the sink link along with the source
> > one in SimpleCameraData(), just checking if you think there will be
> > more use cases where the sink link will be usefull.
>
> I can't rule that out, but I don't have any such use case in mind.
>
> > > ---
> > >  src/libcamera/pipeline/simple/simple.cpp | 24 +++++++++++++++++-------
> > >  1 file changed, 17 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> > > index 731d355efda6..4bde9caa7254 100644
> > > --- a/src/libcamera/pipeline/simple/simple.cpp
> > > +++ b/src/libcamera/pipeline/simple/simple.cpp
> > > @@ -578,15 +578,23 @@ int SimpleCameraData::setupLinks()
> > >  	 * multiple sink links to be enabled together, even on different sink
> > >  	 * pads. We must thus start by disabling all sink links (but the one we
> > >  	 * want to enable) before enabling the pipeline link.
> > > +	 *
> > > +	 * The entities_ list stores entities along with their source link. We
> > > +	 * need to process the link in the context of the sink entity, so
> > > +	 * record the source link of the current entity as the sink link of the
> > > +	 * next entity, and skip the first entity in the loop.
> > >  	 */
> > > +	MediaLink *sinkLink = nullptr;
> > > +
> > >  	for (SimpleCameraData::Entity &e : entities_) {
> > > -		if (!e.sourceLink)
> > > -			break;
> > > +		if (!sinkLink) {
> > > +			sinkLink = e.sourceLink;
> > > +			continue;
> > > +		}
> >
> > Do we assume to start from the sensor (which has no sink pads to
> > disable links on) ?
> >
> > If that's the case, for my understanding, how is entities_ ordered ?
> > It is populated at SimpleCameraData construction time, by iterating
> > over "parents" which is an unordered map.
> >
> > Is there a risk we actually start from a different entity and not the
> > sensor here ?
>
> We indeed start from the sensor. The entities_ list is built in
> SimpleCameraData::SimpleCameraData(). The first large while () loop
> performs a breath-first search starting from the camera sensor, as
> passed to the constructor. At each step, how an entity was reached is
> recorded in the parents unordered map, whose key is the sink and value
> is the source (a.k.a. parent). The loop stops when it reaches a video
> capture device.
>

So far so good

> Then, we create the entities_ list by first adding the video device, and
> then walking from sink to source using the parents map and pushing each
> entity in turn at the *front* of the list. Note that we don't iterate
> over the parents map, we use it to retrieve the parent (a.k.a. source)
> for a given sink. The list is thus guaranteed to start with the sensor
> and be ordered from source to sink, ending with the video device.

I missed the fact we're not iterating, but -searching- on the map.

That solves it, thanks

Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>

>
> > >
> > > -		MediaEntity *remote = e.sourceLink->sink()->entity();
> > > -		for (MediaPad *pad : remote->pads()) {
> > > +		for (MediaPad *pad : e.entity->pads()) {
> > >  			for (MediaLink *link : pad->links()) {
> > > -				if (link == e.sourceLink)
> > > +				if (link == sinkLink)
> > >  					continue;
> > >
> > >  				if ((link->flags() & MEDIA_LNK_FL_ENABLED) &&
> > > @@ -598,11 +606,13 @@ int SimpleCameraData::setupLinks()
> > >  			}
> > >  		}
> > >
> > > -		if (!(e.sourceLink->flags() & MEDIA_LNK_FL_ENABLED)) {
> > > -			ret = e.sourceLink->setEnabled(true);
> > > +		if (!(sinkLink->flags() & MEDIA_LNK_FL_ENABLED)) {
> > > +			ret = sinkLink->setEnabled(true);
> > >  			if (ret < 0)
> > >  				return ret;
> > >  		}
> > > +
> > > +		sinkLink = e.sourceLink;
> > >  	}
> > >
> > >  	return 0;
>
> --
> Regards,
>
> Laurent Pinchart