[v9,02/10] libcamera: simple: Set the number of software ISP streams to 2
diff mbox series

Message ID 20250707155856.33436-3-mzamazal@redhat.com
State Superseded
Headers show
Series
  • Enable raw streams with software ISP
Related show

Commit Message

Milan Zamazal July 7, 2025, 3:58 p.m. UTC
When software ISP is enabled, we want to be able to provide a raw stream
in addition to the processed stream.  For this purpose, we need two
streams.  If only the processed stream is 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.

In theory, software ISP could produce multiple processed streams but
this is out of scope of this patch series.  Hence two streams are
sufficient at the moment.

When software ISP is not enabled, the camera won't be able to produce
multiple streams (assuming there's no hardware converter) and only
single stream should be allocated as before.  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.

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

Comments

Umang Jain July 10, 2025, 2:21 p.m. UTC | #1
Hi Milan,

Thank you for the patch.

On Mon, Jul 07, 2025 at 05:58:47PM +0200, Milan Zamazal wrote:
> When software ISP is enabled, we want to be able to provide a raw stream
> in addition to the processed stream.  For this purpose, we need two
> streams.  If only the processed stream is 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.

It is determined only for the converters case right? For
SoftwareISP=true, it can be determined later in the stage, no? Are there
any downsides of doing that?

> The actual number of streams needed (one or two) is determined only when
> examining roles in SimplePipelineHandler::generateConfiguration.

I don't think numStreams it's set in stone from the start, except for
the converters case. If SoftISP is enabled, we should be able to
resize the streams_ vector / determine the actual no. of streams
required.

> 
> In theory, software ISP could produce multiple processed streams but
> this is out of scope of this patch series.  Hence two streams are
> sufficient at the moment.
> 
> When software ISP is not enabled, the camera won't be able to produce
> multiple streams (assuming there's no hardware converter) and only
> single stream should be allocated as before.  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.
> 
> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
> ---
>  src/libcamera/pipeline/simple/simple.cpp | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> index d45480fe7..a900918db 100644
> --- a/src/libcamera/pipeline/simple/simple.cpp
> +++ b/src/libcamera/pipeline/simple/simple.cpp
> @@ -1723,7 +1723,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;
> +	}

Won't swIspEnabled_ will remain un-initialized if, info->swIspEnabled=false ?
I don't see getting it initialised elsewhere.

>  
>  	/* Locate the sensors. */
>  	std::vector<MediaEntity *> sensors = locateSensors(media);
> -- 
> 2.50.0
>
Milan Zamazal July 10, 2025, 6:03 p.m. UTC | #2
Hi Umang,

thank you for review.

Umang Jain <uajain@igalia.com> writes:

> Hi Milan,
>
> Thank you for the patch.
>
> On Mon, Jul 07, 2025 at 05:58:47PM +0200, Milan Zamazal wrote:
>> When software ISP is enabled, we want to be able to provide a raw stream
>> in addition to the processed stream.  For this purpose, we need two
>> streams.  If only the processed stream is 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.
>
> It is determined only for the converters case right? For
> SoftwareISP=true, it can be determined later in the stage, no? Are there
> any downsides of doing that?

It's a tempting idea but I'm not sure it can be done this way.
Currently, it fails in Camera::generateConfiguration on

  if (roles.size() > streams().size())
  	return nullptr;

>> The actual number of streams needed (one or two) is determined only when
>> examining roles in SimplePipelineHandler::generateConfiguration.
>
> I don't think numStreams it's set in stone from the start, except for
> the converters case. If SoftISP is enabled, we should be able to
> resize the streams_ vector / determine the actual no. of streams
> required.
>
>> 
>> In theory, software ISP could produce multiple processed streams but
>> this is out of scope of this patch series.  Hence two streams are
>> sufficient at the moment.
>> 
>> When software ISP is not enabled, the camera won't be able to produce
>> multiple streams (assuming there's no hardware converter) and only
>> single stream should be allocated as before.  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.
>> 
>> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
>> ---
>>  src/libcamera/pipeline/simple/simple.cpp | 11 ++++++++++-
>>  1 file changed, 10 insertions(+), 1 deletion(-)
>> 
>> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
>> index d45480fe7..a900918db 100644
>> --- a/src/libcamera/pipeline/simple/simple.cpp
>> +++ b/src/libcamera/pipeline/simple/simple.cpp
>> @@ -1723,7 +1723,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;
>> +	}
>
> Won't swIspEnabled_ will remain un-initialized if, info->swIspEnabled=false ?
> I don't see getting it initialised elsewhere.

Right, it should be initialised here, will fix.

>>  
>>  	/* Locate the sensors. */
>>  	std::vector<MediaEntity *> sensors = locateSensors(media);
>> -- 
>> 2.50.0
>>
Umang Jain July 11, 2025, 7:34 a.m. UTC | #3
On Thu, Jul 10, 2025 at 08:03:37PM +0200, Milan Zamazal wrote:
> Hi Umang,
> 
> thank you for review.
> 
> Umang Jain <uajain@igalia.com> writes:
> 
> > Hi Milan,
> >
> > Thank you for the patch.
> >
> > On Mon, Jul 07, 2025 at 05:58:47PM +0200, Milan Zamazal wrote:
> >> When software ISP is enabled, we want to be able to provide a raw stream
> >> in addition to the processed stream.  For this purpose, we need two
> >> streams.  If only the processed stream is 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.
> >
> > It is determined only for the converters case right? For
> > SoftwareISP=true, it can be determined later in the stage, no? Are there
> > any downsides of doing that?
> 
> It's a tempting idea but I'm not sure it can be done this way.
> Currently, it fails in Camera::generateConfiguration on
> 
>   if (roles.size() > streams().size())
>   	return nullptr;
> 

Oh yes - I see it now, it is a sticky situation on this.
streams_ is a bit static parameter while creating the camera.
For Simple<>SoftISP configuration, one can theoretically have as
many streams they want, right? The check seems to be coming from a time
where software-ISP was proabably not been conceived.


> >> The actual number of streams needed (one or two) is determined only when
> >> examining roles in SimplePipelineHandler::generateConfiguration.
> >
> > I don't think numStreams it's set in stone from the start, except for
> > the converters case. If SoftISP is enabled, we should be able to
> > resize the streams_ vector / determine the actual no. of streams
> > required.
> >
> >> 
> >> In theory, software ISP could produce multiple processed streams but
> >> this is out of scope of this patch series.  Hence two streams are
> >> sufficient at the moment.
> >> 
> >> When software ISP is not enabled, the camera won't be able to produce
> >> multiple streams (assuming there's no hardware converter) and only
> >> single stream should be allocated as before.  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.
> >> 
> >> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
> >> ---
> >>  src/libcamera/pipeline/simple/simple.cpp | 11 ++++++++++-
> >>  1 file changed, 10 insertions(+), 1 deletion(-)
> >> 
> >> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> >> index d45480fe7..a900918db 100644
> >> --- a/src/libcamera/pipeline/simple/simple.cpp
> >> +++ b/src/libcamera/pipeline/simple/simple.cpp
> >> @@ -1723,7 +1723,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;
> >> +	}
> >
> > Won't swIspEnabled_ will remain un-initialized if, info->swIspEnabled=false ?
> > I don't see getting it initialised elsewhere.
> 
> Right, it should be initialised here, will fix.
> 
> >>  
> >>  	/* Locate the sensors. */
> >>  	std::vector<MediaEntity *> sensors = locateSensors(media);
> >> -- 
> >> 2.50.0
> >> 
>
Milan Zamazal July 11, 2025, 12:23 p.m. UTC | #4
Umang Jain <uajain@igalia.com> writes:

> On Thu, Jul 10, 2025 at 08:03:37PM +0200, Milan Zamazal wrote:
>> Hi Umang,
>> 
>
>> thank you for review.
>> 
>> Umang Jain <uajain@igalia.com> writes:
>> 
>> > Hi Milan,
>> >
>> > Thank you for the patch.
>> >
>> > On Mon, Jul 07, 2025 at 05:58:47PM +0200, Milan Zamazal wrote:
>> >> When software ISP is enabled, we want to be able to provide a raw stream
>> >> in addition to the processed stream.  For this purpose, we need two
>> >> streams.  If only the processed stream is 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.
>> >
>> > It is determined only for the converters case right? For
>> > SoftwareISP=true, it can be determined later in the stage, no? Are there
>> > any downsides of doing that?
>> 
>> It's a tempting idea but I'm not sure it can be done this way.
>> Currently, it fails in Camera::generateConfiguration on
>> 
>>   if (roles.size() > streams().size())
>>   	return nullptr;
>> 
>
> Oh yes - I see it now, it is a sticky situation on this.
> streams_ is a bit static parameter while creating the camera.
> For Simple<>SoftISP configuration, one can theoretically have as
> many streams they want, right? 

I think so.

> The check seems to be coming from a time where software-ISP was
> proabably not been conceived.

It does but it doesn't prevent having more streams.  I'm not keen on
touching it, it could cause problems in other pipelines.  Considering
the raw stream effort for simple pipeline has been in review with slow
progress for half a year, I'd keep this series as simple as possible.
Setting numStreams to 2 is certainly a hack but it serves the purpose
for now and can be improved later if needed.

>> >> The actual number of streams needed (one or two) is determined only when
>> >> examining roles in SimplePipelineHandler::generateConfiguration.
>> >
>> > I don't think numStreams it's set in stone from the start, except for
>> > the converters case. If SoftISP is enabled, we should be able to
>> > resize the streams_ vector / determine the actual no. of streams
>> > required.
>> >
>> >> 
>> >> In theory, software ISP could produce multiple processed streams but
>> >> this is out of scope of this patch series.  Hence two streams are
>> >> sufficient at the moment.
>> >> 
>> >> When software ISP is not enabled, the camera won't be able to produce
>> >> multiple streams (assuming there's no hardware converter) and only
>> >> single stream should be allocated as before.  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.
>> >> 
>> >> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
>> >> ---
>> >>  src/libcamera/pipeline/simple/simple.cpp | 11 ++++++++++-
>> >>  1 file changed, 10 insertions(+), 1 deletion(-)
>> >> 
>> >> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
>> >> index d45480fe7..a900918db 100644
>> >> --- a/src/libcamera/pipeline/simple/simple.cpp
>> >> +++ b/src/libcamera/pipeline/simple/simple.cpp
>> >> @@ -1723,7 +1723,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;
>> >> +	}
>> >
>> > Won't swIspEnabled_ will remain un-initialized if, info->swIspEnabled=false ?
>> > I don't see getting it initialised elsewhere.
>> 
>> Right, it should be initialised here, will fix.
>> 
>> >>  
>> >>  	/* Locate the sensors. */
>> >>  	std::vector<MediaEntity *> sensors = locateSensors(media);
>> >> -- 
>> >> 2.50.0
>> >> 
>>

Patch
diff mbox series

diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
index d45480fe7..a900918db 100644
--- a/src/libcamera/pipeline/simple/simple.cpp
+++ b/src/libcamera/pipeline/simple/simple.cpp
@@ -1723,7 +1723,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);