| Message ID | 20250630074216.13731-1-uajain@igalia.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Quoting Umang Jain (2025-06-30 08:42:16) > StreamConfiguration::colorspace is a std::optional<> and if unset by > the user, it should be populated by the pipeline handler after the > CameraConfiguration::validate(). > > Add a EXPECT_TRUE() check to ensure that each stream in the > CameraConfiguration has a colorspace set. > > Signed-off-by: Umang Jain <uajain@igalia.com> > --- > Question for reviewers: Should setting the colorspace rule be applied > to all pipeline handlers, or do we have exceptions? For e.g. Laurent's > comment: https://patchwork.libcamera.org/patch/22373/#32812 suggests > to set ColorSpace::Raw if the colorspace is still unknown after > validate() - which will lead to pass this lc-compliance test in those > cases as well. > > Also I preferred EXPECT_TRUE() deliberately as it is non-fatal and > all such violations can be caught in a single lc-compliance run for a > platform. EXPECT_TRUE looks fine to me as long as that's essentially mapping to the underlying 'is the optional set' which I think it does. And indeed, this should report as a test failure - but then continue to test as much as possible. Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > src/apps/lc-compliance/helpers/capture.cpp | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp > index 2a3fa3b6..e767e45e 100644 > --- a/src/apps/lc-compliance/helpers/capture.cpp > +++ b/src/apps/lc-compliance/helpers/capture.cpp > @@ -52,6 +52,9 @@ void Capture::configure(libcamera::Span<const libcamera::StreamRole> roles) > FAIL() << "Configuration not valid"; > } > > + for (auto &cfg : *config_) > + EXPECT_TRUE(cfg.colorSpace) << cfg << " - colorspace not set."; > + > if (camera_->configure(config_.get())) { > config_.reset(); > FAIL() << "Failed to configure camera"; > -- > 2.50.0 >
Quoting Umang Jain (2025-06-30 08:42:16) > StreamConfiguration::colorspace is a std::optional<> and if unset by > the user, it should be populated by the pipeline handler after the > CameraConfiguration::validate(). > > Add a EXPECT_TRUE() check to ensure that each stream in the > CameraConfiguration has a colorspace set. > > Signed-off-by: Umang Jain <uajain@igalia.com> > --- > Question for reviewers: Should setting the colorspace rule be applied > to all pipeline handlers, or do we have exceptions? For e.g. Laurent's > comment: https://patchwork.libcamera.org/patch/22373/#32812 suggests > to set ColorSpace::Raw if the colorspace is still unknown after > validate() - which will lead to pass this lc-compliance test in those > cases as well. > > Also I preferred EXPECT_TRUE() deliberately as it is non-fatal and > all such violations can be caught in a single lc-compliance run for a > platform. > --- > src/apps/lc-compliance/helpers/capture.cpp | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp > index 2a3fa3b6..e767e45e 100644 > --- a/src/apps/lc-compliance/helpers/capture.cpp > +++ b/src/apps/lc-compliance/helpers/capture.cpp > @@ -52,6 +52,9 @@ void Capture::configure(libcamera::Span<const libcamera::StreamRole> roles) > FAIL() << "Configuration not valid"; > } > > + for (auto &cfg : *config_) > + EXPECT_TRUE(cfg.colorSpace) << cfg << " - colorspace not set."; > + The good news is your patch works. The bad news is - the virtual pipeline handler does not set the colorSpace correctly - so this halts in the CI which runs lc-compliance on the virtual pipeline handler - so that handler needs to be 'fixed' to be able to merge this. https://gitlab.freedesktop.org/camera/libcamera/-/jobs/79412875 -- Kieran > if (camera_->configure(config_.get())) { > config_.reset(); > FAIL() << "Failed to configure camera"; > -- > 2.50.0 >
On Mon, Jun 30, 2025 at 11:41:07AM +0100, Kieran Bingham wrote: > Quoting Umang Jain (2025-06-30 08:42:16) > > StreamConfiguration::colorspace is a std::optional<> and if unset by > > the user, it should be populated by the pipeline handler after the > > CameraConfiguration::validate(). > > > > Add a EXPECT_TRUE() check to ensure that each stream in the > > CameraConfiguration has a colorspace set. > > > > Signed-off-by: Umang Jain <uajain@igalia.com> > > --- > > Question for reviewers: Should setting the colorspace rule be applied > > to all pipeline handlers, or do we have exceptions? For e.g. Laurent's > > comment: https://patchwork.libcamera.org/patch/22373/#32812 suggests > > to set ColorSpace::Raw if the colorspace is still unknown after > > validate() - which will lead to pass this lc-compliance test in those > > cases as well. > > > > Also I preferred EXPECT_TRUE() deliberately as it is non-fatal and > > all such violations can be caught in a single lc-compliance run for a > > platform. > > --- > > src/apps/lc-compliance/helpers/capture.cpp | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp > > index 2a3fa3b6..e767e45e 100644 > > --- a/src/apps/lc-compliance/helpers/capture.cpp > > +++ b/src/apps/lc-compliance/helpers/capture.cpp > > @@ -52,6 +52,9 @@ void Capture::configure(libcamera::Span<const libcamera::StreamRole> roles) > > FAIL() << "Configuration not valid"; > > } > > > > + for (auto &cfg : *config_) > > + EXPECT_TRUE(cfg.colorSpace) << cfg << " - colorspace not set."; > > + > > The good news is your patch works. > > The bad news is - the virtual pipeline handler does not set the > colorSpace correctly - so this halts in the CI which runs lc-compliance > on the virtual pipeline handler - so that handler needs to be 'fixed' to > be able to merge this. > > https://gitlab.freedesktop.org/camera/libcamera/-/jobs/79412875 Aha, wonderful! Already doing it's thing.. > > -- > Kieran > > > > if (camera_->configure(config_.get())) { > > config_.reset(); > > FAIL() << "Failed to configure camera"; > > -- > > 2.50.0 > >
diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp index 2a3fa3b6..e767e45e 100644 --- a/src/apps/lc-compliance/helpers/capture.cpp +++ b/src/apps/lc-compliance/helpers/capture.cpp @@ -52,6 +52,9 @@ void Capture::configure(libcamera::Span<const libcamera::StreamRole> roles) FAIL() << "Configuration not valid"; } + for (auto &cfg : *config_) + EXPECT_TRUE(cfg.colorSpace) << cfg << " - colorspace not set."; + if (camera_->configure(config_.get())) { config_.reset(); FAIL() << "Failed to configure camera";
StreamConfiguration::colorspace is a std::optional<> and if unset by the user, it should be populated by the pipeline handler after the CameraConfiguration::validate(). Add a EXPECT_TRUE() check to ensure that each stream in the CameraConfiguration has a colorspace set. Signed-off-by: Umang Jain <uajain@igalia.com> --- Question for reviewers: Should setting the colorspace rule be applied to all pipeline handlers, or do we have exceptions? For e.g. Laurent's comment: https://patchwork.libcamera.org/patch/22373/#32812 suggests to set ColorSpace::Raw if the colorspace is still unknown after validate() - which will lead to pass this lc-compliance test in those cases as well. Also I preferred EXPECT_TRUE() deliberately as it is non-fatal and all such violations can be caught in a single lc-compliance run for a platform. --- src/apps/lc-compliance/helpers/capture.cpp | 3 +++ 1 file changed, 3 insertions(+)