[libcamera-devel] pipeline: simple: Update documentation on pipeline setup

11629 diff mbox series

After 4671911df040, the explanation in the SimplePipeline documentation
how the handler tries to find a valid path to capture device doest not
reflect the reality anymore. Update the text to the new situation.

Fixes: 4671911df040 ("pipeline: simple: Use breadth-first search to
setup media pipeline")

Signed-off-by: Marian Cichy <m.cichy@pengutronix.de>
---
 src/libcamera/pipeline/simple/simple.cpp | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

Hey Marian,

Thank you for the patch.

On 19.03.2021 17:03, Marian Cichy wrote:
>After 4671911df040, the explanation in the SimplePipeline documentation
>how the handler tries to find a valid path to capture device doest not

s/doest not/doesn't/

>reflect the reality anymore. Update the text to the new situation.
>
>Fixes: 4671911df040 ("pipeline: simple: Use breadth-first search to
>setup media pipeline")
>
>Signed-off-by: Marian Cichy <m.cichy@pengutronix.de>
>---
> src/libcamera/pipeline/simple/simple.cpp | 11 ++++++-----
> 1 file changed, 6 insertions(+), 5 deletions(-)
>
>diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
>index 068fb454..39f3d019 100644
>--- a/src/libcamera/pipeline/simple/simple.cpp
>+++ b/src/libcamera/pipeline/simple/simple.cpp
>@@ -69,11 +69,12 @@ LOG_DEFINE_CATEGORY(SimplePipeline)
>  *
>  * When matching a device, the pipeline handler enumerates all camera sensors
>  * and attempts, for each of them, to find a path to a video capture video node.
>- * It does so by traversing the media graph, following the first non permanently
>- * disabled downstream link. If such a path is found, the pipeline handler
>- * creates a corresponding SimpleCameraData instance, and stores the media graph
>- * path in its entities_ list.
>- *
>+ * It does so by using a breadth-first search to find the shortest path from the
>+ * sensor device to a valid capture device. This is guaranteed to produce a
>+ * valid path on devices with one only option and is a good heuristic on more

s/with one only option/with a single option

>+ * complex devices to skip paths that aren't suitable for the simple pipeline
>+ * handler. For instance, on the IPU-based i.MX6, the shortest path will skip
>+ * encoders and image converts, and it will end in a CSI capture device.

s/image converts/image converters/

>  * A more complex graph search algorithm could be implemented if a device that
>  * would otherwise be compatible with the pipeline handler isn't correctly
>  * handled by this heuristic.
>-- 
>2.29.2

You can add:
Reviewed-by: Sebastian Fricke <sebastian.fricke@posteo.net>
if you like :).

Greetings,
Sebastian
>
>_______________________________________________
>libcamera-devel mailing list
>libcamera-devel@lists.libcamera.org
>https://lists.libcamera.org/listinfo/libcamera-devel

Hi Marian,

Thank you for the patch.

On Fri, Mar 19, 2021 at 05:44:29PM +0100, Sebastian Fricke wrote:
> On 19.03.2021 17:03, Marian Cichy wrote:
> > After 4671911df040, the explanation in the SimplePipeline documentation

To make it easier for the reader, commits should be quoted with their
subject line:

After commit 4671911df040 ("pipeline: simple: Use breadth-first search
to setup media pipeline"), the explanation ...

> > how the handler tries to find a valid path to capture device doest not
> 
> s/doest not/doesn't/

Or 'does not'.

> > reflect the reality anymore. Update the text to the new situation.
> > 
> > Fixes: 4671911df040 ("pipeline: simple: Use breadth-first search to
> > setup media pipeline")

No line break, you can go over the 72 columns limit for Fixes lines.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

I'll address these small issues when applying, no need to resubmit.

> > Signed-off-by: Marian Cichy <m.cichy@pengutronix.de>
> > ---
> >  src/libcamera/pipeline/simple/simple.cpp | 11 ++++++-----
> >  1 file changed, 6 insertions(+), 5 deletions(-)
> > 
> > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> > index 068fb454..39f3d019 100644
> > --- a/src/libcamera/pipeline/simple/simple.cpp
> > +++ b/src/libcamera/pipeline/simple/simple.cpp
> > @@ -69,11 +69,12 @@ LOG_DEFINE_CATEGORY(SimplePipeline)
> >   *
> >   * When matching a device, the pipeline handler enumerates all camera sensors
> >   * and attempts, for each of them, to find a path to a video capture video node.
> > - * It does so by traversing the media graph, following the first non permanently
> > - * disabled downstream link. If such a path is found, the pipeline handler
> > - * creates a corresponding SimpleCameraData instance, and stores the media graph
> > - * path in its entities_ list.
> > - *
> > + * It does so by using a breadth-first search to find the shortest path from the
> > + * sensor device to a valid capture device. This is guaranteed to produce a
> > + * valid path on devices with one only option and is a good heuristic on more
> 
> s/with one only option/with a single option
> 
> > + * complex devices to skip paths that aren't suitable for the simple pipeline
> > + * handler. For instance, on the IPU-based i.MX6, the shortest path will skip
> > + * encoders and image converts, and it will end in a CSI capture device.
> 
> s/image converts/image converters/
> 
> >   * A more complex graph search algorithm could be implemented if a device that
> >   * would otherwise be compatible with the pipeline handler isn't correctly
> >   * handled by this heuristic.
> 
> You can add:
> Reviewed-by: Sebastian Fricke <sebastian.fricke@posteo.net>
> if you like :).