| Message ID | 20250707155856.33436-6-mzamazal@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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 >
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 > >
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 >>>
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 >>>>
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;
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(-)