[{"id":34725,"web_url":"https://patchwork.libcamera.org/comment/34725/","msgid":"<175127196610.2340557.4001158828547058310@ping.linuxembedded.co.uk>","date":"2025-06-30T08:26:06","subject":"Re: [PATCH] lc-compliance: Ensure stream's colorspace is set after\n\tvalidate()","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Umang Jain (2025-06-30 08:42:16)\n> StreamConfiguration::colorspace is a std::optional<> and if unset by\n> the user, it should be populated by the pipeline handler after the\n> CameraConfiguration::validate().\n> \n> Add a EXPECT_TRUE() check to ensure that each stream in the\n> CameraConfiguration has a colorspace set.\n> \n> Signed-off-by: Umang Jain \n> ---\n> Question for reviewers: Should setting the colorspace rule be applied\n> to all pipeline handlers, or do we have exceptions? For e.g. Laurent's\n> comment: https://patchwork.libcamera.org/patch/22373/#32812 suggests\n> to set ColorSpace::Raw if the colorspace is still unknown after\n> validate() - which will lead to pass this lc-compliance test in those\n> cases as well.\n> \n> Also I preferred EXPECT_TRUE() deliberately as it is non-fatal and\n> all such violations can be caught in a single lc-compliance run for a\n> platform.\n\nEXPECT_TRUE looks fine to me as long as that's essentially mapping to\nthe underlying 'is the optional set' which I think it does.\n\nAnd indeed, this should report as a test failure - but then continue to\ntest as much as possible.\n\n\nReviewed-by: Kieran Bingham \n\n\n> ---\n> src/apps/lc-compliance/helpers/capture.cpp | 3 +++\n> 1 file changed, 3 insertions(+)\n> \n> diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp\n> index 2a3fa3b6..e767e45e 100644\n> --- a/src/apps/lc-compliance/helpers/capture.cpp\n> +++ b/src/apps/lc-compliance/helpers/capture.cpp\n> @@ -52,6 +52,9 @@ void Capture::configure(libcamera::Span roles)\n> FAIL() << \"Configuration not valid\";\n> }\n> \n> + for (auto &cfg : *config_)\n> + EXPECT_TRUE(cfg.colorSpace) << cfg << \" - colorspace not set.\";\n> +\n> if (camera_->configure(config_.get())) {\n> config_.reset();\n> FAIL() << \"Failed to configure camera\";\n> -- \n> 2.50.0\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 549C9C3237\n\tfor ;\n\tMon, 30 Jun 2025 08:26:12 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0C54568E06;\n\tMon, 30 Jun 2025 10:26:11 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DFDC468E00\n\tfor ;\n\tMon, 30 Jun 2025 10:26:08 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 275BE6BE;\n\tMon, 30 Jun 2025 10:25:47 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"YhaJsVq6\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1751271947;\n\tbh=j9eZ/MbfwpOMPQDlLIU0Kt3ctQBjVrV1imNCxoIlgAc=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=YhaJsVq6a7bHEOYYSlm893VdKohyYiVIDl6okuhOg9/nuhoUjCb6/VemCX81+voLy\n\tNiI+VPRAOQ3EUPgBbEjgnztnJBTNhxPFrY5A+aJutFYxuvkqHaduk+5+TRRsQxwbA6\n\tSk3wT1tbQrgz9nths7jI0c86YeCT6yf5g9UMsohg=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20250630074216.13731-1-uajain@igalia.com>","References":"<20250630074216.13731-1-uajain@igalia.com>","Subject":"Re: [PATCH] lc-compliance: Ensure stream's colorspace is set after\n\tvalidate()","From":"Kieran Bingham ","Cc":"Antoine Bouyer ,\n\tMilan Zamazal , Umang Jain ","To":"Umang Jain , libcamera-devel@lists.libcamera.org","Date":"Mon, 30 Jun 2025 09:26:06 +0100","Message-ID":"<175127196610.2340557.4001158828547058310@ping.linuxembedded.co.uk>","User-Agent":"alot/0.9.1","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":34735,"web_url":"https://patchwork.libcamera.org/comment/34735/","msgid":"<175128006787.2538045.9831690463552621775@ping.linuxembedded.co.uk>","date":"2025-06-30T10:41:07","subject":"Re: [PATCH] lc-compliance: Ensure stream's colorspace is set after\n\tvalidate()","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Umang Jain (2025-06-30 08:42:16)\n> StreamConfiguration::colorspace is a std::optional<> and if unset by\n> the user, it should be populated by the pipeline handler after the\n> CameraConfiguration::validate().\n> \n> Add a EXPECT_TRUE() check to ensure that each stream in the\n> CameraConfiguration has a colorspace set.\n> \n> Signed-off-by: Umang Jain \n> ---\n> Question for reviewers: Should setting the colorspace rule be applied\n> to all pipeline handlers, or do we have exceptions? For e.g. Laurent's\n> comment: https://patchwork.libcamera.org/patch/22373/#32812 suggests\n> to set ColorSpace::Raw if the colorspace is still unknown after\n> validate() - which will lead to pass this lc-compliance test in those\n> cases as well.\n> \n> Also I preferred EXPECT_TRUE() deliberately as it is non-fatal and\n> all such violations can be caught in a single lc-compliance run for a\n> platform.\n> ---\n> src/apps/lc-compliance/helpers/capture.cpp | 3 +++\n> 1 file changed, 3 insertions(+)\n> \n> diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp\n> index 2a3fa3b6..e767e45e 100644\n> --- a/src/apps/lc-compliance/helpers/capture.cpp\n> +++ b/src/apps/lc-compliance/helpers/capture.cpp\n> @@ -52,6 +52,9 @@ void Capture::configure(libcamera::Span roles)\n> FAIL() << \"Configuration not valid\";\n> }\n> \n> + for (auto &cfg : *config_)\n> + EXPECT_TRUE(cfg.colorSpace) << cfg << \" - colorspace not set.\";\n> +\n\nThe good news is your patch works.\n\nThe bad news is - the virtual pipeline handler does not set the\ncolorSpace correctly - so this halts in the CI which runs lc-compliance\non the virtual pipeline handler - so that handler needs to be 'fixed' to\nbe able to merge this.\n\nhttps://gitlab.freedesktop.org/camera/libcamera/-/jobs/79412875\n\n--\nKieran\n\n\n> if (camera_->configure(config_.get())) {\n> config_.reset();\n> FAIL() << \"Failed to configure camera\";\n> -- \n> 2.50.0\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 6F40EC3237\n\tfor ;\n\tMon, 30 Jun 2025 10:41:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DAE2B68E04;\n\tMon, 30 Jun 2025 12:41:12 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9391161527\n\tfor ;\n\tMon, 30 Jun 2025 12:41:10 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id DF9111352;\n\tMon, 30 Jun 2025 12:40:48 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"V/ti9ZfJ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1751280049;\n\tbh=0V0aS7KDoiB8RA0YB34JGMec/aW2IEjzO6ZrwRCcq5Q=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=V/ti9ZfJZ/j8Qt/c0Y1DS4HAIq9neA76HCu6l+MydPJs/Nrw0G5zM82IMqK9jp8lC\n\tN/hjz4csES2/7Ti8bPI5nd4j6wDoxCZ185ctk0qA1gYZAh3nxVChw5WL+p4ztzlwvh\n\trwXTmHGhnX/+6pwkLlmoDamE1z/bM3EmiRZo+9Vc=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20250630074216.13731-1-uajain@igalia.com>","References":"<20250630074216.13731-1-uajain@igalia.com>","Subject":"Re: [PATCH] lc-compliance: Ensure stream's colorspace is set after\n\tvalidate()","From":"Kieran Bingham ","Cc":"Antoine Bouyer ,\n\tMilan Zamazal , Umang Jain ","To":"Umang Jain , libcamera-devel@lists.libcamera.org","Date":"Mon, 30 Jun 2025 11:41:07 +0100","Message-ID":"<175128006787.2538045.9831690463552621775@ping.linuxembedded.co.uk>","User-Agent":"alot/0.9.1","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":34740,"web_url":"https://patchwork.libcamera.org/comment/34740/","msgid":"","date":"2025-06-30T11:49:46","subject":"Re: [PATCH] lc-compliance: Ensure stream's colorspace is set after\n\tvalidate()","submitter":{"id":232,"url":"https://patchwork.libcamera.org/api/people/232/","name":"Umang Jain","email":"uajain@igalia.com"},"content":"On Mon, Jun 30, 2025 at 11:41:07AM +0100, Kieran Bingham wrote:\n> Quoting Umang Jain (2025-06-30 08:42:16)\n> > StreamConfiguration::colorspace is a std::optional<> and if unset by\n> > the user, it should be populated by the pipeline handler after the\n> > CameraConfiguration::validate().\n> > \n> > Add a EXPECT_TRUE() check to ensure that each stream in the\n> > CameraConfiguration has a colorspace set.\n> > \n> > Signed-off-by: Umang Jain \n> > ---\n> > Question for reviewers: Should setting the colorspace rule be applied\n> > to all pipeline handlers, or do we have exceptions? For e.g. Laurent's\n> > comment: https://patchwork.libcamera.org/patch/22373/#32812 suggests\n> > to set ColorSpace::Raw if the colorspace is still unknown after\n> > validate() - which will lead to pass this lc-compliance test in those\n> > cases as well.\n> > \n> > Also I preferred EXPECT_TRUE() deliberately as it is non-fatal and\n> > all such violations can be caught in a single lc-compliance run for a\n> > platform.\n> > ---\n> > src/apps/lc-compliance/helpers/capture.cpp | 3 +++\n> > 1 file changed, 3 insertions(+)\n> > \n> > diff --git a/src/apps/lc-compliance/helpers/capture.cpp b/src/apps/lc-compliance/helpers/capture.cpp\n> > index 2a3fa3b6..e767e45e 100644\n> > --- a/src/apps/lc-compliance/helpers/capture.cpp\n> > +++ b/src/apps/lc-compliance/helpers/capture.cpp\n> > @@ -52,6 +52,9 @@ void Capture::configure(libcamera::Span roles)\n> > FAIL() << \"Configuration not valid\";\n> > }\n> > \n> > + for (auto &cfg : *config_)\n> > + EXPECT_TRUE(cfg.colorSpace) << cfg << \" - colorspace not set.\";\n> > +\n> \n> The good news is your patch works.\n> \n> The bad news is - the virtual pipeline handler does not set the\n> colorSpace correctly - so this halts in the CI which runs lc-compliance\n> on the virtual pipeline handler - so that handler needs to be 'fixed' to\n> be able to merge this.\n> \n> https://gitlab.freedesktop.org/camera/libcamera/-/jobs/79412875\n\nAha, wonderful! Already doing it's thing..\n> \n> --\n> Kieran\n> \n> \n> > if (camera_->configure(config_.get())) {\n> > config_.reset();\n> > FAIL() << \"Failed to configure camera\";\n> > -- \n> > 2.50.0\n> >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 006B1C3237\n\tfor ;\n\tMon, 30 Jun 2025 11:49:46 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B8E0268E04;\n\tMon, 30 Jun 2025 13:49:45 +0200 (CEST)","from fanzine2.igalia.com (fanzine2.igalia.com [213.97.179.56])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4478261527\n\tfor ;\n\tMon, 30 Jun 2025 13:49:44 +0200 (CEST)","from [49.36.69.141] (helo=uajain)\n\tby fanzine2.igalia.com with esmtpsa \n\t(Cipher TLS1.3:ECDHE_SECP256R1__RSA_PSS_RSAE_SHA256__AES_256_GCM:256)\n\t(Exim) id 1uWD14-00ASVZ-KJ; Mon, 30 Jun 2025 13:49:43 +0200"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=igalia.com header.i=@igalia.com\n\theader.b=\"UHJJCF92\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=igalia.com;\n\ts=20170329;\n\th=In-Reply-To:Content-Type:MIME-Version:References:Message-ID:\n\tSubject:Cc:To:From:Date:Sender:Reply-To:Content-Transfer-Encoding:Content-ID:\n\tContent-Description:Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc\n\t:Resent-Message-ID:List-Id:List-Help:List-Unsubscribe:List-Subscribe:\n\tList-Post:List-Owner:List-Archive;\n\tbh=KYk3i0/jhaKZV9wyVdTwFZUvGXALBUKl3PbGLiRwizE=;\n\tb=UHJJCF92soaUHUcdhUhxl7IPFr\n\tusPSC/+VHYuPJ62963JNvG/E0XQWmBoEwjgIjtaH/jzuuuwcv7eeSoHHQbCQyWaACquw9XeZO2vFU\n\tYIkSIqSOOo/tOXPj4vvfUxI/wJZDdGae28wydIKth9ZV/Nn099uQSzalzeBwgDmelAyg2/iV+DBIM\n\tHldmBS6SZ8iaUbG1akyq5z5crekonafEjP8xP/JWX1RiXYgc3NigopB9CMXNdiCLUGDbFk3zOlSWc\n\tayy+enxyq1olGILDn9GG98BArbAuaCOAvlxPCPVN1yCzN0sNug+/oCLpln6LZsR28t/KEhAkPxuTH\n\tP5hc19+A==;","Date":"Mon, 30 Jun 2025 17:19:46 +0530","From":"Umang Jain ","To":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org, \n\tAntoine Bouyer ,\n\tMilan Zamazal ","Subject":"Re: [PATCH] lc-compliance: Ensure stream's colorspace is set after\n\tvalidate()","Message-ID":"","References":"<20250630074216.13731-1-uajain@igalia.com>\n\t<175128006787.2538045.9831690463552621775@ping.linuxembedded.co.uk>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<175128006787.2538045.9831690463552621775@ping.linuxembedded.co.uk>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]