[RFC,v2,02/13] libcamera: simple: Increase the default number of streams to 2

22641 diff mbox series

In order to allow providing a raw stream together with a processed
stream, we need two streams.  If only one stream is eventually
requested, it doesn't harm to allocate two.

This is a hack for the lack of a better easy option.  The number of
streams is determined as a camera property in the pipeline matching.
The actual number of streams needed (one or two) is determined only when
examining roles in SimplePipelineHandler::generateConfiguration.

It's not obvious what to do better about the number of the streams.  It
seems that hardware pipelines utilize separate hardware streams while
with software ISP we have only a single input for multiple outputs.  A
fixed number of streams also doesn't scale but is good enough for our
current use case, which is supporting a single processed + a single raw
stream, not more.

Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
---
 src/libcamera/pipeline/simple/simple.cpp | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Hi Milan,

Thank you for the patch.

On Fri, Jan 24, 2025 at 10:57:53PM +0100, Milan Zamazal wrote:
> In order to allow providing a raw stream together with a processed
> stream, we need two streams.  If only one stream is eventually
> requested, it doesn't harm to allocate two.
> 
> This is a hack for the lack of a better easy option.  The number of
> streams is determined as a camera property in the pipeline matching.
> The actual number of streams needed (one or two) is determined only when
> examining roles in SimplePipelineHandler::generateConfiguration.
> 
> It's not obvious what to do better about the number of the streams.  It
> seems that hardware pipelines utilize separate hardware streams while
> with software ISP we have only a single input for multiple outputs.  A
> fixed number of streams also doesn't scale but is good enough for our
> current use case, which is supporting a single processed + a single raw
> stream, not more.

You also need to consider the case where the software ISP isn't enabled.
The camera won't be able to produce multiple streams in that case
(assuming there's no hardware converter), so creating the camera with
two streams is wrong.

The simple pipeline handler assumes there's a linear pipeline from the
camera sensor to a video capture device, and only supports a single
stream. Branches in the hardware pipeline that would allow capturing
multiple streams from the same camera sensor are not supported. We have
no plan to change that, as a device that can produce multiple streams
will likely be better supported by a dedicated pipeline handler.

In addition to the capture pipeline, the simple pipeline handler has
supported hardware converters that operate in a memory to memory
fashion. The allows producing multiple streams by running the converter
multiple times on the same captured image. When a converter is available
we don't support capturing the stream produced by the capture pipeline.
That's an artificial restriction to simplify the implementation,
conceptually we could think of it as a "raw" stream and expose it to
applications. It will most likely not have a raw format though, as none
of the converters support raw inputs, but it could deliver an additional
stream at no cost.

The software ISP is similar to a converter. We could run it multiple
times to produce multiple processed streams (at an obvious cost), and we
can also expose the images at the output of the hardware capture
pipeline as an additional stream (which, unlike for converters, is
guaranteed to be a raw stream as the software ISP supports raw inputs
only).

Given those similarities between converters and the software ISP, I
would like to see a common abstraction being developed. The simple
pipeline handler shouldn't care about whether the captured frames are
processed with a hardware converter or with the software ISP. That will
take more work, and I'm fine with a step by step approach. I would
however like those steps to keep the final goal in mind.

For this specific patch, it means that the number of streams should
remain one when no software ISP is present. Something along these lines
could do:

diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
index 8ac24e6e3423..c8d801636c62 100644
--- a/src/libcamera/pipeline/simple/simple.cpp
+++ b/src/libcamera/pipeline/simple/simple.cpp
@@ -1573,7 +1573,16 @@ bool SimplePipelineHandler::match(DeviceEnumerator *enumerator)
 		}
 	}
 
-	swIspEnabled_ = info->swIspEnabled;
+	if (info->swIspEnabled) {
+		/*
+		 * When the software ISP is enabled, the simple pipeline handler
+		 * exposes the raw stream, giving a total of two streams. This
+		 * is mutally exclusive with the presence of a converter.
+		 */
+		ASSERT(!converter_);
+		numStreams = 2;
+		swIspEnabled_ = true;
+	}
 
 	/* Locate the sensors. */
 	std::vector<MediaEntity *> sensors = locateSensors(media);

> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
> ---
>  src/libcamera/pipeline/simple/simple.cpp | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> index 8ac24e6e..6e9bc630 100644
> --- a/src/libcamera/pipeline/simple/simple.cpp
> +++ b/src/libcamera/pipeline/simple/simple.cpp
> @@ -1549,7 +1549,12 @@ int SimplePipelineHandler::resetRoutingTable(V4L2Subdevice *subdev)
>  bool SimplePipelineHandler::match(DeviceEnumerator *enumerator)
>  {
>  	const SimplePipelineInfo *info = nullptr;
> -	unsigned int numStreams = 1;
> +	/*
> +	 * Let's allocate two streams, for processed + raw streams.
> +	 * It's OK if there is only a single stream.
> +	 * TODO: This should be more flexible.

We use

	 * \todo This should be more flexible.

'\todo' is a doxygen tag. It won't get parsed here as the comment block
doesn't start with '/**', but we try to standardize on '\todo' to
facilitate grepping.

> +	 */
> +	unsigned int numStreams = 2;
>  	MediaDevice *media;
>  
>  	for (const SimplePipelineInfo &inf : supportedDevices) {