[v9,05/10] libcamera: simple: Identify requested stream roles
diff mbox series

Message ID 20250707155856.33436-6-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
Currently, raw streams don't work in the simple pipeline and the
requested stream roles are ignored.  In order to support raw streams, we
now track in SimpleCameraConfiguration whether raw and/or processed
streams are requested.  We also check that at most one raw stream is
requested, there is no reason to have more.

That information will be used in the followup patches.

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

Comments

Umang Jain July 11, 2025, 5:08 a.m. UTC | #1
On Mon, Jul 07, 2025 at 05:58:50PM +0200, Milan Zamazal wrote:
> Currently, raw streams don't work in the simple pipeline and the
> requested stream roles are ignored.  In order to support raw streams, we
> now track in SimpleCameraConfiguration whether raw and/or processed
> streams are requested.  We also check that at most one raw stream is
> requested, there is no reason to have more.
> 
> That information will be used in the followup patches.
> 
> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
> ---
>  src/libcamera/pipeline/simple/simple.cpp | 20 +++++++++++++++++++-
>  1 file changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> index 1a7474228..6330afeb5 100644
> --- a/src/libcamera/pipeline/simple/simple.cpp
> +++ b/src/libcamera/pipeline/simple/simple.cpp
> @@ -384,6 +384,9 @@ public:
>  	bool needConversion() const { return needConversion_; }
>  	const Transform &combinedTransform() const { return combinedTransform_; }
>  
> +	bool processedRequested_;
> +	bool rawRequested_;
> +
>  private:
>  	/*
>  	 * The SimpleCameraData instance is guaranteed to be valid as long as
> @@ -1311,12 +1314,27 @@ std::unique_ptr<CameraConfiguration>
>  SimplePipelineHandler::generateConfiguration(Camera *camera, Span<const StreamRole> roles)
>  {
>  	SimpleCameraData *data = cameraData(camera);
> -	std::unique_ptr<CameraConfiguration> config =
> +	std::unique_ptr<SimpleCameraConfiguration> config =
>  		std::make_unique<SimpleCameraConfiguration>(camera, data);
>  
> +	config->processedRequested_ = false;
> +	config->rawRequested_ = false;

Should be defaulted at construction time?
> +
>  	if (roles.empty())
>  		return config;
>  
> +	for (const auto &role : roles)
> +		if (role == StreamRole::Raw) {
> +			if (config->rawRequested_) {
> +				LOG(SimplePipeline, Error)
> +					<< "Can't capture multiple raw streams";
> +				return nullptr;

Should this be a candidate of
CameraConfiguration::generateConfiguration() instead?  Requesting multiple
role=raw streams for a single CameraConfiguration, should be invalid anyway
on all platform (unless I am missing something).

> +			}
> +			config->rawRequested_ = true;


> +		} else {
> +			config->processedRequested_ = true;
> +		}
> +
>  	/* Create the formats map. */
>  	std::map<PixelFormat, std::vector<SizeRange>> formats;
>  
> -- 
> 2.50.0
>
Umang Jain July 11, 2025, 6:17 a.m. UTC | #2
Hi again,

On Fri, Jul 11, 2025 at 10:38:02AM +0530, Umang Jain wrote:
> On Mon, Jul 07, 2025 at 05:58:50PM +0200, Milan Zamazal wrote:
> > Currently, raw streams don't work in the simple pipeline and the
> > requested stream roles are ignored.  In order to support raw streams, we
> > now track in SimpleCameraConfiguration whether raw and/or processed
> > streams are requested.  We also check that at most one raw stream is
> > requested, there is no reason to have more.
> > 
> > That information will be used in the followup patches.
> > 
> > Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
> > ---
> >  src/libcamera/pipeline/simple/simple.cpp | 20 +++++++++++++++++++-
> >  1 file changed, 19 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> > index 1a7474228..6330afeb5 100644
> > --- a/src/libcamera/pipeline/simple/simple.cpp
> > +++ b/src/libcamera/pipeline/simple/simple.cpp
> > @@ -384,6 +384,9 @@ public:
> >  	bool needConversion() const { return needConversion_; }
> >  	const Transform &combinedTransform() const { return combinedTransform_; }
> >  
> > +	bool processedRequested_;
> > +	bool rawRequested_;
> > +
> >  private:
> >  	/*
> >  	 * The SimpleCameraData instance is guaranteed to be valid as long as
> > @@ -1311,12 +1314,27 @@ std::unique_ptr<CameraConfiguration>
> >  SimplePipelineHandler::generateConfiguration(Camera *camera, Span<const StreamRole> roles)
> >  {
> >  	SimpleCameraData *data = cameraData(camera);
> > -	std::unique_ptr<CameraConfiguration> config =
> > +	std::unique_ptr<SimpleCameraConfiguration> config =
> >  		std::make_unique<SimpleCameraConfiguration>(camera, data);
> >  
> > +	config->processedRequested_ = false;
> > +	config->rawRequested_ = false;
> 
> Should be defaulted at construction time?
> > +
> >  	if (roles.empty())
> >  		return config;
> >  
> > +	for (const auto &role : roles)
> > +		if (role == StreamRole::Raw) {
> > +			if (config->rawRequested_) {
> > +				LOG(SimplePipeline, Error)
> > +					<< "Can't capture multiple raw streams";
> > +				return nullptr;
> 
> Should this be a candidate of
> CameraConfiguration::generateConfiguration() instead?  Requesting multiple
> role=raw streams for a single CameraConfiguration, should be invalid anyway
> on all platform (unless I am missing something).
> 
> > +			}
> > +			config->rawRequested_ = true;
> 
> 
> > +		} else {
> > +			config->processedRequested_ = true;
> > +		}
> > +

I believe these members rawRequested_ and processedRequests_ should be
set based on streams configuration / pixel formats  requested and not on
roles passed.

The application can request an empty CameraConfiguration by not
passing any role in generateConfiguration() and then pass its desired stream
configuration(s) by CameraConfiguration::addConfiguration(). That would
completely bypass setting of these variables and probably not
configure() things properly.

I would rather inspect the streamConfiguration in validate() ... and set
rawRequested_ and processedRequestsed_ there. There will be more corner
cases to deal with (like multiple raw StreamConfiguration added in
Camera Configuration) which can be easily caught in validate().

> >  	/* Create the formats map. */
> >  	std::map<PixelFormat, std::vector<SizeRange>> formats;
> >  
> > -- 
> > 2.50.0
> >
Pőcze Barnabás July 11, 2025, 10:07 a.m. UTC | #3
Hi

2025. 07. 11. 8:17 keltezéssel, Umang Jain írta:
> Hi again,
> 
> On Fri, Jul 11, 2025 at 10:38:02AM +0530, Umang Jain wrote:
>> On Mon, Jul 07, 2025 at 05:58:50PM +0200, Milan Zamazal wrote:
>>> Currently, raw streams don't work in the simple pipeline and the
>>> requested stream roles are ignored.  In order to support raw streams, we
>>> now track in SimpleCameraConfiguration whether raw and/or processed
>>> streams are requested.  We also check that at most one raw stream is
>>> requested, there is no reason to have more.
>>>
>>> That information will be used in the followup patches.
>>>
>>> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
>>> ---
>>>   src/libcamera/pipeline/simple/simple.cpp | 20 +++++++++++++++++++-
>>>   1 file changed, 19 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
>>> index 1a7474228..6330afeb5 100644
>>> --- a/src/libcamera/pipeline/simple/simple.cpp
>>> +++ b/src/libcamera/pipeline/simple/simple.cpp
>>> @@ -384,6 +384,9 @@ public:
>>>   	bool needConversion() const { return needConversion_; }
>>>   	const Transform &combinedTransform() const { return combinedTransform_; }
>>>
>>> +	bool processedRequested_;
>>> +	bool rawRequested_;
>>> +
>>>   private:
>>>   	/*
>>>   	 * The SimpleCameraData instance is guaranteed to be valid as long as
>>> @@ -1311,12 +1314,27 @@ std::unique_ptr<CameraConfiguration>
>>>   SimplePipelineHandler::generateConfiguration(Camera *camera, Span<const StreamRole> roles)
>>>   {
>>>   	SimpleCameraData *data = cameraData(camera);
>>> -	std::unique_ptr<CameraConfiguration> config =
>>> +	std::unique_ptr<SimpleCameraConfiguration> config =
>>>   		std::make_unique<SimpleCameraConfiguration>(camera, data);
>>>
>>> +	config->processedRequested_ = false;
>>> +	config->rawRequested_ = false;
>>
>> Should be defaulted at construction time?

I agree, just having ` = false` in the type definition would probably be best.


>>> +
>>>   	if (roles.empty())
>>>   		return config;
>>>
>>> +	for (const auto &role : roles)
>>> +		if (role == StreamRole::Raw) {
>>> +			if (config->rawRequested_) {
>>> +				LOG(SimplePipeline, Error)
>>> +					<< "Can't capture multiple raw streams";
>>> +				return nullptr;
>>
>> Should this be a candidate of
>> CameraConfiguration::generateConfiguration() instead?  Requesting multiple
>> role=raw streams for a single CameraConfiguration, should be invalid anyway
>> on all platform (unless I am missing something).
>>
>>> +			}
>>> +			config->rawRequested_ = true;
>>
>>
>>> +		} else {
>>> +			config->processedRequested_ = true;
>>> +		}
>>> +
> 
> I believe these members rawRequested_ and processedRequests_ should be
> set based on streams configuration / pixel formats  requested and not on
> roles passed.
> 
> The application can request an empty CameraConfiguration by not
> passing any role in generateConfiguration() and then pass its desired stream
> configuration(s) by CameraConfiguration::addConfiguration(). That would
> completely bypass setting of these variables and probably not
> configure() things properly.

I think this is already taken care of, at least in some capacity; these two flags
are updated in `SimpleCameraConfiguration::validate()` in patch 07.


Regards,
Barnabás Pőcze


> 
> I would rather inspect the streamConfiguration in validate() ... and set
> rawRequested_ and processedRequestsed_ there. There will be more corner
> cases to deal with (like multiple raw StreamConfiguration added in
> Camera Configuration) which can be easily caught in validate().
> 
>>>   	/* Create the formats map. */
>>>   	std::map<PixelFormat, std::vector<SizeRange>> formats;
>>>
>>> --
>>> 2.50.0
>>>
Milan Zamazal July 11, 2025, 3:25 p.m. UTC | #4
Hi,

thank you for reviews.

Pőcze Barnabás <pobrn@protonmail.com> writes:

> Hi
>
> 2025. 07. 11. 8:17 keltezéssel, Umang Jain írta:
>> Hi again,
>> 
>> On Fri, Jul 11, 2025 at 10:38:02AM +0530, Umang Jain wrote:
>>> On Mon, Jul 07, 2025 at 05:58:50PM +0200, Milan Zamazal wrote:
>>>> Currently, raw streams don't work in the simple pipeline and the
>>>> requested stream roles are ignored.  In order to support raw streams, we
>>>> now track in SimpleCameraConfiguration whether raw and/or processed
>>>> streams are requested.  We also check that at most one raw stream is
>>>> requested, there is no reason to have more.
>>>>
>>>> That information will be used in the followup patches.
>>>>
>>>> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
>>>> ---
>>>>   src/libcamera/pipeline/simple/simple.cpp | 20 +++++++++++++++++++-
>>>>   1 file changed, 19 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
>>>> index 1a7474228..6330afeb5 100644
>>>> --- a/src/libcamera/pipeline/simple/simple.cpp
>>>> +++ b/src/libcamera/pipeline/simple/simple.cpp
>>>> @@ -384,6 +384,9 @@ public:
>>>>   	bool needConversion() const { return needConversion_; }
>>>>   	const Transform &combinedTransform() const { return combinedTransform_; }
>>>>
>>>> +	bool processedRequested_;
>>>> +	bool rawRequested_;
>>>> +
>>>>   private:
>>>>   	/*
>>>>   	 * The SimpleCameraData instance is guaranteed to be valid as long as
>>>> @@ -1311,12 +1314,27 @@ std::unique_ptr<CameraConfiguration>
>>>>   SimplePipelineHandler::generateConfiguration(Camera *camera, Span<const StreamRole> roles)
>>>>   {
>>>>   	SimpleCameraData *data = cameraData(camera);
>>>> -	std::unique_ptr<CameraConfiguration> config =
>>>> +	std::unique_ptr<SimpleCameraConfiguration> config =
>>>>   		std::make_unique<SimpleCameraConfiguration>(camera, data);
>>>>
>>>> +	config->processedRequested_ = false;
>>>> +	config->rawRequested_ = false;
>>>
>>> Should be defaulted at construction time?
>
> I agree, just having ` = false` in the type definition would probably be best.
>
>
>>>> +
>>>>   	if (roles.empty())
>>>>   		return config;
>>>>
>>>> +	for (const auto &role : roles)
>>>> +		if (role == StreamRole::Raw) {
>>>> +			if (config->rawRequested_) {
>>>> +				LOG(SimplePipeline, Error)
>>>> +					<< "Can't capture multiple raw streams";
>>>> +				return nullptr;
>>>
>>> Should this be a candidate of
>>> CameraConfiguration::generateConfiguration() instead?  Requesting multiple
>>> role=raw streams for a single CameraConfiguration, should be invalid anyway
>>> on all platform (unless I am missing something).

I'm not sure, I think I've seen support for two raw streams in some
pipeline, but I may be mistaken.  Anyway, I wouldn't like to make
changes with impact on other pipelines in this series.

>>>> +			}
>>>> +			config->rawRequested_ = true;
>>>
>>>
>>>> +		} else {
>>>> +			config->processedRequested_ = true;
>>>> +		}
>>>> +
>> 
>> I believe these members rawRequested_ and processedRequests_ should be
>> set based on streams configuration / pixel formats  requested and not on
>> roles passed.

I think I can drop rawRequested_ and processedRequests_ and set
corresponding local variables independently in both
generateConfiguration() and validate().  This should solve the issue you
describe below; the role-based checks in generateConfiguration() should
remain in place as asking for multiple raw roles or other mismatches
(like asking for a raw role with a non-raw format) are simply invalid.

>> The application can request an empty CameraConfiguration by not
>> passing any role in generateConfiguration() and then pass its desired stream
>> configuration(s) by CameraConfiguration::addConfiguration(). That would
>> completely bypass setting of these variables and probably not
>> configure() things properly.
>
> I think this is already taken care of, at least in some capacity; these two flags
> are updated in `SimpleCameraConfiguration::validate()` in patch 07.

Indeed, but a check for multiple raw streams should be added there, will
do in v10.

> Regards,
> Barnabás Pőcze
>
>
>> 
>> I would rather inspect the streamConfiguration in validate() ... and set
>> rawRequested_ and processedRequestsed_ there. There will be more corner
>> cases to deal with (like multiple raw StreamConfiguration added in
>> Camera Configuration) which can be easily caught in validate().
>> 
>>>>   	/* Create the formats map. */
>>>>   	std::map<PixelFormat, std::vector<SizeRange>> formats;
>>>>
>>>> --
>>>> 2.50.0
>>>>

Patch
diff mbox series

diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
index 1a7474228..6330afeb5 100644
--- a/src/libcamera/pipeline/simple/simple.cpp
+++ b/src/libcamera/pipeline/simple/simple.cpp
@@ -384,6 +384,9 @@  public:
 	bool needConversion() const { return needConversion_; }
 	const Transform &combinedTransform() const { return combinedTransform_; }
 
+	bool processedRequested_;
+	bool rawRequested_;
+
 private:
 	/*
 	 * The SimpleCameraData instance is guaranteed to be valid as long as
@@ -1311,12 +1314,27 @@  std::unique_ptr<CameraConfiguration>
 SimplePipelineHandler::generateConfiguration(Camera *camera, Span<const StreamRole> roles)
 {
 	SimpleCameraData *data = cameraData(camera);
-	std::unique_ptr<CameraConfiguration> config =
+	std::unique_ptr<SimpleCameraConfiguration> config =
 		std::make_unique<SimpleCameraConfiguration>(camera, data);
 
+	config->processedRequested_ = false;
+	config->rawRequested_ = false;
+
 	if (roles.empty())
 		return config;
 
+	for (const auto &role : roles)
+		if (role == StreamRole::Raw) {
+			if (config->rawRequested_) {
+				LOG(SimplePipeline, Error)
+					<< "Can't capture multiple raw streams";
+				return nullptr;
+			}
+			config->rawRequested_ = true;
+		} else {
+			config->processedRequested_ = true;
+		}
+
 	/* Create the formats map. */
 	std::map<PixelFormat, std::vector<SizeRange>> formats;