| Message ID | 20220801000543.3501-11-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent 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@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. > --- > 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 ? THanks j > > - 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 >
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@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. 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. > > > > - 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;
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@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@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
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; + } - 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;
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@ideasonboard.com> --- src/libcamera/pipeline/simple/simple.cpp | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-)