[{"id":34897,"web_url":"https://patchwork.libcamera.org/comment/34897/","msgid":"<85o6tllvj6.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","date":"2025-07-15T08:15:25","subject":"Re: [PATCH] libcamera: pipeline: Always generate valid camera\n\tconfigurations","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Umang Jain <uajain@igalia.com> writes:\n\n> PipelineHandler::generateConfiguration() documentation states the\n> following:\n>\n>  * \\return A valid CameraConfiguration if the requested roles can be\n>  *  satisfied, or a null pointer otherwise.\n>  */\n>\n> Hence, always check the return status of config->validate(), during\n> generateConfiguration() call in each pipeline handler.\n>\n> Signed-off-by: Umang Jain <uajain@igalia.com>\n\nReviewed-by: Milan Zamazal <mzamazal@redhat.com>\n\n> ---\n>  src/libcamera/pipeline/imx8-isi/imx8-isi.cpp | 3 ++-\n>  src/libcamera/pipeline/rkisp1/rkisp1.cpp     | 3 ++-\n>  src/libcamera/pipeline/simple/simple.cpp     | 3 ++-\n>  src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 3 ++-\n>  src/libcamera/pipeline/vimc/vimc.cpp         | 3 ++-\n>  5 files changed, 10 insertions(+), 5 deletions(-)\n>\n> diff --git a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp\n> index f4014b95..c96fefa4 100644\n> --- a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp\n> +++ b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp\n> @@ -798,7 +798,8 @@ PipelineHandlerISI::generateConfiguration(Camera *camera,\n>  \t\tconfig->addConfiguration(cfg);\n>  \t}\n>  \n> -\tconfig->validate();\n> +\tif (config->validate() == CameraConfiguration::Invalid)\n> +\t\treturn nullptr;\n>  \n>  \treturn config;\n>  }\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index 675f0a74..fca1c74e 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -787,7 +787,8 @@ PipelineHandlerRkISP1::generateConfiguration(Camera *camera,\n>  \t\tconfig->addConfiguration(cfg);\n>  \t}\n>  \n> -\tconfig->validate();\n> +\tif (config->validate() == CameraConfiguration::Invalid)\n> +\t\treturn nullptr;\n>  \n>  \treturn config;\n>  }\n> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> index efb07051..f994106e 100644\n> --- a/src/libcamera/pipeline/simple/simple.cpp\n> +++ b/src/libcamera/pipeline/simple/simple.cpp\n> @@ -1322,7 +1322,8 @@ SimplePipelineHandler::generateConfiguration(Camera *camera, Span<const StreamRo\n>  \t\tconfig->addConfiguration(cfg);\n>  \t}\n>  \n> -\tconfig->validate();\n> +\tif (config->validate() == CameraConfiguration::Invalid)\n> +\t\treturn nullptr;\n>  \n>  \treturn config;\n>  }\n> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> index 4b5816df..9ed85f3b 100644\n> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> @@ -250,7 +250,8 @@ PipelineHandlerUVC::generateConfiguration(Camera *camera,\n>  \n>  \tconfig->addConfiguration(cfg);\n>  \n> -\tconfig->validate();\n> +\tif (config->validate() == CameraConfiguration::Invalid)\n> +\t\treturn nullptr;\n>  \n>  \treturn config;\n>  }\n> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp\n> index 07273bd2..56719fa3 100644\n> --- a/src/libcamera/pipeline/vimc/vimc.cpp\n> +++ b/src/libcamera/pipeline/vimc/vimc.cpp\n> @@ -245,7 +245,8 @@ PipelineHandlerVimc::generateConfiguration(Camera *camera,\n>  \n>  \tconfig->addConfiguration(cfg);\n>  \n> -\tconfig->validate();\n> +\tif (config->validate() == CameraConfiguration::Invalid)\n> +\t\treturn nullptr;\n>  \n>  \treturn config;\n>  }","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 8E13DBE175\n\tfor <parsemail@patchwork.libcamera.org>;\n\tTue, 15 Jul 2025 08:15:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0C06368F51;\n\tTue, 15 Jul 2025 10:15:34 +0200 (CEST)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.133.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BFF4D61517\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 15 Jul 2025 10:15:31 +0200 (CEST)","from mail-wm1-f72.google.com (mail-wm1-f72.google.com\n\t[209.85.128.72]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-394-QYREfVRkPi23WkxLE6Zefg-1; Tue, 15 Jul 2025 04:15:29 -0400","by mail-wm1-f72.google.com with SMTP id\n\t5b1f17b1804b1-4560a30a793so11731415e9.3\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 15 Jul 2025 01:15:28 -0700 (PDT)","from mzamazal-thinkpadp1gen7.tpbc.csb ([85.93.96.130])\n\tby smtp.gmail.com with ESMTPSA id\n\t5b1f17b1804b1-454dd540b52sm154265425e9.28.2025.07.15.01.15.26\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tTue, 15 Jul 2025 01:15:26 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"U8XpxHOm\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1752567330;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=ftisPSwOJdN/ug2qkfRg7j/InCFs9gbqDFpCamiXztE=;\n\tb=U8XpxHOmVON/NMDbA3i6iBlnk87Owe6hwrvc/y+RJaaZMbvFs97vjmcSwwfH9uHRfO8rhZ\n\tnt3jDjvCNg3zgboNqmRICDWojLkSD8nvAu6YKpGLDt9+4BMKhzAccIK/AgWn07Be4hzPyT\n\tLmvNq7wvgk69Q0mO/lPfOltVJWPAD14=","X-MC-Unique":"QYREfVRkPi23WkxLE6Zefg-1","X-Mimecast-MFC-AGG-ID":"QYREfVRkPi23WkxLE6Zefg_1752567328","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1752567327; x=1753172127;\n\th=mime-version:user-agent:message-id:date:references:in-reply-to\n\t:subject:cc:to:from:x-gm-message-state:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=ftisPSwOJdN/ug2qkfRg7j/InCFs9gbqDFpCamiXztE=;\n\tb=m2g9tyXE33/KVKVSNWDnDJ9grLRAk35fIqHIaaHGRSAAfv32dnN2OSa2ULC6q+r+Gl\n\tEyVS8LhBvVCaP/rWsFnHcoT9K7iFQixEo4R7Bz0mayB9PxgYw7jH4csVIhzFGM5MT1Mv\n\tWiscJmx/vcg6YkaRpkvlFv+FRx3F7J5fGgxqWy/lCqm5CVQqZxfyCu2OtQ3EE7prsE4b\n\t+tQjnfuYEZaPbwKZyEcX1hMBXKPGzPvjgypeOLqz1MvtayOT4lp4Hjvp5aj4/ezrKHyM\n\t7SWhBCgfhV0P9saOJthuI1WSPxkCQkG0BY4anqxup4Sy5Jf9yuhQnSuuM1Sqam8+Ixfe\n\t2JnQ==","X-Gm-Message-State":"AOJu0YzsFwbRozRYN25KVAiWbv3NJByBtibdZZG+kit8fQzn/6xwtum1\n\tOpueh8jai5Fj3/e10cUS6xeEAS4Uq/v319ouCKub+TdYLBOXHFdcmb5dcyIqVWCKjMANuBf7Frp\n\tEPJzAnCKn3aIk0Sw5ZcFkqtaX+eRA4rHuVQ2UeZMDGXDPIkmevMgXRhKuRLuJzSOXRiPGAeg8bo\n\t3U3y4kNXzVVsT66SHrcVPQTatT39s+CWdKJFPZmhVRylx1H2mhPtaYr1P4N+Q=","X-Gm-Gg":"ASbGncuiLQm/yT2Q9+Jel9pthP+q6jZP8PEim0ypO8vn7OMKzHS5JtsGS6YDh7EFBtV\n\tZMRcxIkUtw4n8d1E33JLf19Asd9I5QkHRZ/2WMrysR8NO7AIDRvdzcXJjIytZ9Y6cDfoc0kudrp\n\tXpln9nI3aT+nE2NdwgdSXuE4Bz9xfN0SUmViTdfhdqZLwi1JVPCmyj53rPu8/N+ZbMceXNc2Y9K\n\tbQlnv3EX7VeFdkVFqzBdKiQDjMesN9yzxFw/fmAgeRq56XyQnUD5HhJ3Pd8hGTwp+/Dz7ogzwpc\n\tLDw25AZX6R/+oLUl3bSdZproDga9+T7SOOswdFj/6DLf5mXwXai7wVxdOpTbTrgC","X-Received":["by 2002:a05:600c:1f95:b0:456:58e:3192 with SMTP id\n\t5b1f17b1804b1-456058e32ebmr83851575e9.1.1752567327449; \n\tTue, 15 Jul 2025 01:15:27 -0700 (PDT)","by 2002:a05:600c:1f95:b0:456:58e:3192 with SMTP id\n\t5b1f17b1804b1-456058e32ebmr83851195e9.1.1752567326889; \n\tTue, 15 Jul 2025 01:15:26 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IGMvrIF4HXEZMRAdOD5qi/fIgWHT0P8NsoD0QUc6BUS+O80P7uP6osC/ic9an2HCSf1xfPaZQ==","From":"Milan Zamazal <mzamazal@redhat.com>","To":"Umang Jain <uajain@igalia.com>","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH] libcamera: pipeline: Always generate valid camera\n\tconfigurations","In-Reply-To":"<20250715023013.16435-1-uajain@igalia.com> (Umang Jain's message\n\tof \"Tue, 15 Jul 2025 08:00:13 +0530\")","References":"<20250715023013.16435-1-uajain@igalia.com>","Date":"Tue, 15 Jul 2025 10:15:25 +0200","Message-ID":"<85o6tllvj6.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","User-Agent":"Gnus/5.13 (Gnus v5.13)","MIME-Version":"1.0","X-Mimecast-Spam-Score":"0","X-Mimecast-MFC-PROC-ID":"ZJaMwBzuv6KS02p-C5k49yVy7GMURvWVty7FWXzdMmo_1752567328","X-Mimecast-Originator":"redhat.com","Content-Type":"text/plain","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":34898,"web_url":"https://patchwork.libcamera.org/comment/34898/","msgid":"<402e1b09-a9ff-458e-8847-d2dacced2c4b@ideasonboard.com>","date":"2025-07-15T09:00:22","subject":"Re: [PATCH] libcamera: pipeline: Always generate valid camera\n\tconfigurations","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"Hi\n\n2025. 07. 15. 4:30 keltezéssel, Umang Jain írta:\n> PipelineHandler::generateConfiguration() documentation states the\n> following:\n> \n>   * \\return A valid CameraConfiguration if the requested roles can be\n>   *  satisfied, or a null pointer otherwise.\n>   */\n> \n> Hence, always check the return status of config->validate(), during\n> generateConfiguration() call in each pipeline handler.\n\nThis is already part of the ipu3 and mali-c55 pipeline handlers.\nSo I guess consistency is good; any reason is it not part of\n`Camera::generateConfiguration()` ?\n\n\nRegards,\nBarnabás Pőcze\n\n\n> \n> Signed-off-by: Umang Jain <uajain@igalia.com>\n> ---\n>   src/libcamera/pipeline/imx8-isi/imx8-isi.cpp | 3 ++-\n>   src/libcamera/pipeline/rkisp1/rkisp1.cpp     | 3 ++-\n>   src/libcamera/pipeline/simple/simple.cpp     | 3 ++-\n>   src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 3 ++-\n>   src/libcamera/pipeline/vimc/vimc.cpp         | 3 ++-\n>   5 files changed, 10 insertions(+), 5 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp\n> index f4014b95..c96fefa4 100644\n> --- a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp\n> +++ b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp\n> @@ -798,7 +798,8 @@ PipelineHandlerISI::generateConfiguration(Camera *camera,\n>   \t\tconfig->addConfiguration(cfg);\n>   \t}\n>   \n> -\tconfig->validate();\n> +\tif (config->validate() == CameraConfiguration::Invalid)\n> +\t\treturn nullptr;\n>   \n>   \treturn config;\n>   }\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index 675f0a74..fca1c74e 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -787,7 +787,8 @@ PipelineHandlerRkISP1::generateConfiguration(Camera *camera,\n>   \t\tconfig->addConfiguration(cfg);\n>   \t}\n>   \n> -\tconfig->validate();\n> +\tif (config->validate() == CameraConfiguration::Invalid)\n> +\t\treturn nullptr;\n>   \n>   \treturn config;\n>   }\n> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> index efb07051..f994106e 100644\n> --- a/src/libcamera/pipeline/simple/simple.cpp\n> +++ b/src/libcamera/pipeline/simple/simple.cpp\n> @@ -1322,7 +1322,8 @@ SimplePipelineHandler::generateConfiguration(Camera *camera, Span<const StreamRo\n>   \t\tconfig->addConfiguration(cfg);\n>   \t}\n>   \n> -\tconfig->validate();\n> +\tif (config->validate() == CameraConfiguration::Invalid)\n> +\t\treturn nullptr;\n>   \n>   \treturn config;\n>   }\n> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> index 4b5816df..9ed85f3b 100644\n> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> @@ -250,7 +250,8 @@ PipelineHandlerUVC::generateConfiguration(Camera *camera,\n>   \n>   \tconfig->addConfiguration(cfg);\n>   \n> -\tconfig->validate();\n> +\tif (config->validate() == CameraConfiguration::Invalid)\n> +\t\treturn nullptr;\n>   \n>   \treturn config;\n>   }\n> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp\n> index 07273bd2..56719fa3 100644\n> --- a/src/libcamera/pipeline/vimc/vimc.cpp\n> +++ b/src/libcamera/pipeline/vimc/vimc.cpp\n> @@ -245,7 +245,8 @@ PipelineHandlerVimc::generateConfiguration(Camera *camera,\n>   \n>   \tconfig->addConfiguration(cfg);\n>   \n> -\tconfig->validate();\n> +\tif (config->validate() == CameraConfiguration::Invalid)\n> +\t\treturn nullptr;\n>   \n>   \treturn config;\n>   }","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 5BDDFBE175\n\tfor <parsemail@patchwork.libcamera.org>;\n\tTue, 15 Jul 2025 09:00:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D6FE961517;\n\tTue, 15 Jul 2025 11:00:29 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 61D2261517\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 15 Jul 2025 11:00:27 +0200 (CEST)","from [192.168.33.25] (185.221.140.39.nat.pool.zt.hu\n\t[185.221.140.39])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 711B37E4;\n\tTue, 15 Jul 2025 10:59:54 +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=\"ef42k3lf\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1752569994;\n\tbh=XkdsA7e6+vqm3Z3iE8bJSLCW40br42n3+EigomqcA18=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=ef42k3lfmMAGyIMhXOm/QfUvdIkeHd1fAfQjKbMxhMTx3MiWaVuMBGuW/sMlN/gww\n\tjSMxvujUjbVZUv2XL6FmnogEFhzUy6RTD/76n8mO1ZOH3EN23fkhrcdksCPPlZZuUK\n\tvgMczyGr/DEjtmxKnJ8m4lqf4B8VBIbBB0mpeBkw=","Message-ID":"<402e1b09-a9ff-458e-8847-d2dacced2c4b@ideasonboard.com>","Date":"Tue, 15 Jul 2025 11:00:22 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH] libcamera: pipeline: Always generate valid camera\n\tconfigurations","To":"Umang Jain <uajain@igalia.com>, libcamera-devel@lists.libcamera.org","References":"<20250715023013.16435-1-uajain@igalia.com>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= <barnabas.pocze@ideasonboard.com>","Content-Language":"en-US, hu-HU","In-Reply-To":"<20250715023013.16435-1-uajain@igalia.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":34899,"web_url":"https://patchwork.libcamera.org/comment/34899/","msgid":"<20250715091644.GG20231@pendragon.ideasonboard.com>","date":"2025-07-15T09:16:44","subject":"Re: [PATCH] libcamera: pipeline: Always generate valid camera\n\tconfigurations","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Umang,\n\nThank you for the patch.\n\nOn Tue, Jul 15, 2025 at 08:00:13AM +0530, Umang Jain wrote:\n> PipelineHandler::generateConfiguration() documentation states the\n> following:\n> \n>  * \\return A valid CameraConfiguration if the requested roles can be\n>  *  satisfied, or a null pointer otherwise.\n>  */\n> \n> Hence, always check the return status of config->validate(), during\n> generateConfiguration() call in each pipeline handler.\n> \n> Signed-off-by: Umang Jain <uajain@igalia.com>\n> ---\n>  src/libcamera/pipeline/imx8-isi/imx8-isi.cpp | 3 ++-\n>  src/libcamera/pipeline/rkisp1/rkisp1.cpp     | 3 ++-\n>  src/libcamera/pipeline/simple/simple.cpp     | 3 ++-\n>  src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 3 ++-\n>  src/libcamera/pipeline/vimc/vimc.cpp         | 3 ++-\n>  5 files changed, 10 insertions(+), 5 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp\n> index f4014b95..c96fefa4 100644\n> --- a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp\n> +++ b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp\n> @@ -798,7 +798,8 @@ PipelineHandlerISI::generateConfiguration(Camera *camera,\n>  \t\tconfig->addConfiguration(cfg);\n>  \t}\n>  \n> -\tconfig->validate();\n> +\tif (config->validate() == CameraConfiguration::Invalid)\n> +\t\treturn nullptr;\n\nReturning nullptr from generateConfiguration() is meant to cover cases\nwhere the requested roles really can't be met. For instance, this can be\nthe case when requesting more streams than the hardware supports, or\nwhen one of the stream roles has an unsupported value.\n\nThe call to validate() in generateConfiguration() was initially added in\na pipeline handler that filled part of the configuration in\ngenerateConfiguration() and relied on validate() to fill the rest,\ninstead of duplicating the same logic in both functions. It then got\ncopied blindly to other pipeline handlers. The validate() call is not\nsupposed to return Invalid here, that would be a bug in the pipeline\nhandler. Blindly returning nullptr, and worse, returning it without any\nindication to the user as to what went wrong, doesn't seem desirable to\nme.\n\n>  \n>  \treturn config;\n>  }\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index 675f0a74..fca1c74e 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -787,7 +787,8 @@ PipelineHandlerRkISP1::generateConfiguration(Camera *camera,\n>  \t\tconfig->addConfiguration(cfg);\n>  \t}\n>  \n> -\tconfig->validate();\n> +\tif (config->validate() == CameraConfiguration::Invalid)\n> +\t\treturn nullptr;\n>  \n>  \treturn config;\n>  }\n> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> index efb07051..f994106e 100644\n> --- a/src/libcamera/pipeline/simple/simple.cpp\n> +++ b/src/libcamera/pipeline/simple/simple.cpp\n> @@ -1322,7 +1322,8 @@ SimplePipelineHandler::generateConfiguration(Camera *camera, Span<const StreamRo\n>  \t\tconfig->addConfiguration(cfg);\n>  \t}\n>  \n> -\tconfig->validate();\n> +\tif (config->validate() == CameraConfiguration::Invalid)\n> +\t\treturn nullptr;\n>  \n>  \treturn config;\n>  }\n> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> index 4b5816df..9ed85f3b 100644\n> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> @@ -250,7 +250,8 @@ PipelineHandlerUVC::generateConfiguration(Camera *camera,\n>  \n>  \tconfig->addConfiguration(cfg);\n>  \n> -\tconfig->validate();\n> +\tif (config->validate() == CameraConfiguration::Invalid)\n> +\t\treturn nullptr;\n>  \n>  \treturn config;\n>  }\n> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp\n> index 07273bd2..56719fa3 100644\n> --- a/src/libcamera/pipeline/vimc/vimc.cpp\n> +++ b/src/libcamera/pipeline/vimc/vimc.cpp\n> @@ -245,7 +245,8 @@ PipelineHandlerVimc::generateConfiguration(Camera *camera,\n>  \n>  \tconfig->addConfiguration(cfg);\n>  \n> -\tconfig->validate();\n> +\tif (config->validate() == CameraConfiguration::Invalid)\n> +\t\treturn nullptr;\n>  \n>  \treturn config;\n>  }","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 54260C3237\n\tfor <parsemail@patchwork.libcamera.org>;\n\tTue, 15 Jul 2025 09:16:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D88E168F58;\n\tTue, 15 Jul 2025 11:16:47 +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 EC8A161517\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 15 Jul 2025 11:16:45 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id 354737E4;\n\tTue, 15 Jul 2025 11:16:13 +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=\"SkgypnBw\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1752570973;\n\tbh=9kkWA8UdXZIN6sjbkj5nLq32mqWLgQsMaI2mIng0Wl0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=SkgypnBwlg1vv3TAbKQo5Ut6JjFxswx5AzhgITI/dPXi0lHmb5xO8lD/dHSxWreUz\n\tiYWoJtnYVuuOwFRloC/GBsK5qc0jfgbBK2neD4TA3E7zC0CFMS1nl4BuAXzWfxGESc\n\ttvJKgd1zcQdmZ5n/7G234KN4sWPPA+xRuWhtpas4=","Date":"Tue, 15 Jul 2025 12:16:44 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Umang Jain <uajain@igalia.com>","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH] libcamera: pipeline: Always generate valid camera\n\tconfigurations","Message-ID":"<20250715091644.GG20231@pendragon.ideasonboard.com>","References":"<20250715023013.16435-1-uajain@igalia.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20250715023013.16435-1-uajain@igalia.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":34900,"web_url":"https://patchwork.libcamera.org/comment/34900/","msgid":"<85h5zdlq7r.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","date":"2025-07-15T10:10:16","subject":"Re: [PATCH] libcamera: pipeline: Always generate valid camera\n\tconfigurations","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Hi Laurent,\n\nLaurent Pinchart <laurent.pinchart@ideasonboard.com> writes:\n\n> Hi Umang,\n>\n> Thank you for the patch.\n>\n> On Tue, Jul 15, 2025 at 08:00:13AM +0530, Umang Jain wrote:\n>> PipelineHandler::generateConfiguration() documentation states the\n>> following:\n>> \n>>  * \\return A valid CameraConfiguration if the requested roles can be\n>>  *  satisfied, or a null pointer otherwise.\n>>  */\n>> \n>> Hence, always check the return status of config->validate(), during\n>> generateConfiguration() call in each pipeline handler.\n>> \n>> Signed-off-by: Umang Jain <uajain@igalia.com>\n>> ---\n>>  src/libcamera/pipeline/imx8-isi/imx8-isi.cpp | 3 ++-\n>>  src/libcamera/pipeline/rkisp1/rkisp1.cpp     | 3 ++-\n>>  src/libcamera/pipeline/simple/simple.cpp     | 3 ++-\n>>  src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 3 ++-\n>>  src/libcamera/pipeline/vimc/vimc.cpp         | 3 ++-\n>>  5 files changed, 10 insertions(+), 5 deletions(-)\n>> \n>> diff --git a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp\n>> index f4014b95..c96fefa4 100644\n>> --- a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp\n>> +++ b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp\n>> @@ -798,7 +798,8 @@ PipelineHandlerISI::generateConfiguration(Camera *camera,\n>>  \t\tconfig->addConfiguration(cfg);\n>>  \t}\n>>  \n>> -\tconfig->validate();\n>> +\tif (config->validate() == CameraConfiguration::Invalid)\n>> +\t\treturn nullptr;\n>\n> Returning nullptr from generateConfiguration() is meant to cover cases\n> where the requested roles really can't be met. For instance, this can be\n> the case when requesting more streams than the hardware supports, or\n> when one of the stream roles has an unsupported value.\n>\n> The call to validate() in generateConfiguration() was initially added in\n> a pipeline handler that filled part of the configuration in\n> generateConfiguration() and relied on validate() to fill the rest,\n> instead of duplicating the same logic in both functions. It then got\n> copied blindly to other pipeline handlers. The validate() call is not\n> supposed to return Invalid here, that would be a bug in the pipeline\n> handler. \n\nDo we want to check that then in a different way?  At least by replacing\nthe check above with an assertion?\n\n> Blindly returning nullptr, and worse, returning it without any\n> indication to the user as to what went wrong, doesn't seem desirable\n> to me.\n>\n>>  \n>>  \treturn config;\n>>  }\n>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>> index 675f0a74..fca1c74e 100644\n>> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>> @@ -787,7 +787,8 @@ PipelineHandlerRkISP1::generateConfiguration(Camera *camera,\n>>  \t\tconfig->addConfiguration(cfg);\n>>  \t}\n>>  \n>> -\tconfig->validate();\n>> +\tif (config->validate() == CameraConfiguration::Invalid)\n>> +\t\treturn nullptr;\n>>  \n>>  \treturn config;\n>>  }\n>> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n>> index efb07051..f994106e 100644\n>> --- a/src/libcamera/pipeline/simple/simple.cpp\n>> +++ b/src/libcamera/pipeline/simple/simple.cpp\n>> @@ -1322,7 +1322,8 @@ SimplePipelineHandler::generateConfiguration(Camera *camera, Span<const StreamRo\n>>  \t\tconfig->addConfiguration(cfg);\n>>  \t}\n>>  \n>> -\tconfig->validate();\n>> +\tif (config->validate() == CameraConfiguration::Invalid)\n>> +\t\treturn nullptr;\n>>  \n>>  \treturn config;\n>>  }\n>> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n>> index 4b5816df..9ed85f3b 100644\n>> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n>> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n>> @@ -250,7 +250,8 @@ PipelineHandlerUVC::generateConfiguration(Camera *camera,\n>>  \n>>  \tconfig->addConfiguration(cfg);\n>>  \n>> -\tconfig->validate();\n>> +\tif (config->validate() == CameraConfiguration::Invalid)\n>> +\t\treturn nullptr;\n>>  \n>>  \treturn config;\n>>  }\n>> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp\n>> index 07273bd2..56719fa3 100644\n>> --- a/src/libcamera/pipeline/vimc/vimc.cpp\n>> +++ b/src/libcamera/pipeline/vimc/vimc.cpp\n>> @@ -245,7 +245,8 @@ PipelineHandlerVimc::generateConfiguration(Camera *camera,\n>>  \n>>  \tconfig->addConfiguration(cfg);\n>>  \n>> -\tconfig->validate();\n>> +\tif (config->validate() == CameraConfiguration::Invalid)\n>> +\t\treturn nullptr;\n>>  \n>>  \treturn config;\n>>  }","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 D382EC3237\n\tfor <parsemail@patchwork.libcamera.org>;\n\tTue, 15 Jul 2025 10:10:26 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7ACC668F58;\n\tTue, 15 Jul 2025 12:10:25 +0200 (CEST)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.129.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3766E6186B\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 15 Jul 2025 12:10:23 +0200 (CEST)","from mail-wr1-f69.google.com (mail-wr1-f69.google.com\n\t[209.85.221.69]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-595-TcsjxtrYOLe5FCqPj6Eglg-1; Tue, 15 Jul 2025 06:10:20 -0400","by mail-wr1-f69.google.com with SMTP id\n\tffacd0b85a97d-3a503f28b09so3318287f8f.0\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 15 Jul 2025 03:10:20 -0700 (PDT)","from mzamazal-thinkpadp1gen7.tpbc.csb ([85.93.96.130])\n\tby smtp.gmail.com with ESMTPSA id\n\tffacd0b85a97d-3b5e8dc1f70sm14574022f8f.27.2025.07.15.03.10.17\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tTue, 15 Jul 2025 03:10:17 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"di6GjB2+\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1752574222;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=dsySLBvV9gjVSJeXNlKq7Fa3g2FjBEw7EXbj5o6Fqvk=;\n\tb=di6GjB2+psXjgc4SeVIpmGAuTAX+FL3B/7j6UlR9VmRLZsRG/NBf9cdyX5gOVogA+k3fiB\n\t/FEd2VicQa01HF/Ys2LFEubnUivraN/AL2wc/GN7MMoLeH7+3/59VmSnxuwR8YRihIUBm+\n\tw/ntYpoHv6ZX/LiTnkc79FmUnZaETjI=","X-MC-Unique":"TcsjxtrYOLe5FCqPj6Eglg-1","X-Mimecast-MFC-AGG-ID":"TcsjxtrYOLe5FCqPj6Eglg_1752574219","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1752574219; x=1753179019;\n\th=mime-version:user-agent:message-id:date:references:in-reply-to\n\t:subject:cc:to:from:x-gm-message-state:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=dsySLBvV9gjVSJeXNlKq7Fa3g2FjBEw7EXbj5o6Fqvk=;\n\tb=dMC3snnSWJibD8hpw/xw2qJ01cqAgj8ii2B2n4t0iJYj+8fjGIG615+BT9bqPNuW0w\n\t5DSOrMZZV412Hqj0KtR8ixcOj9z17fMGpMAYwYCtUxMyA+hgUMCgZTE24m87vhcQz9vS\n\tvciOKEacxRjcTftVXAj/2qKI44aZ6xUBmCAKVGxxGbpQqV99xTDNuB6bhylnPlBeYV4h\n\tqbpDXacQzqDF97oU3SUfulbNwO6o2M74QA2FSUOHurY+fMhm6YdTUNtBc3j9oq4bE/cG\n\tsD0ZAS8fMlL6q0mJDiegZ8+vuLnFGMKAaCSA7qcY0WGWsWzh67Jthjep2pOEEw+RYh/x\n\tdr9g==","X-Forwarded-Encrypted":"i=1;\n\tAJvYcCWwOmGo+1XINqZbIPMMueggMQpu9gw0BeJ05q7HLCh1ozQebluuxwH/V5BgOiutLOAhQxsPD8VtfSmYTjITDCA=@lists.libcamera.org","X-Gm-Message-State":"AOJu0Yzdv9gOjwrX+rTEj9nAxNNU9dFRBxtze2D4VaUhon4AtM+sux88\n\tyOaALIOSjyiu16a9hvdQXjkEq7k0KfRAFg+XBCjzU2f4GXCsxNZ8drwAXNkvrdN81G5HRLOB+kl\n\tl6VpizT2OVasbMhqqWlfG3bKLvg1szqOWCnIzMpq/Bvv6k1gptVMPfECx0LZK6ZrsCx0AJTuBBH\n\tsBEUSLBN4d+w0sCYHcs9gTcf9D7hlhl5e6bA9Tv5AWc33Rz54QmQO7oab6JK0=","X-Gm-Gg":"ASbGncv7fTDL7UAiMfqJo5He25scf+mMh31b/R1florI2DYc0xsiqxB6i3pOa9QW0v4\n\tYZDd1cFKFpsqUeJ48sGRUWiU3Qq3VWNBkXaxVpmecgAN7qxDWSJCs9GrjEfSvx+Ys5zRRRaeyIg\n\t4KKARDbQ7uusELG4SKAXYAWuPgSxW5yooI4Z6Mpm4JR0gGD0332kdRgxytp3tM+inKwue4EUT6d\n\tLZZEgu1mcxV4AvnBVPk2qSzjRVm8czjtHlRBbAmadGxd/XnFtlGh9UJfQ0Aj/ZteVHwuRPZoORC\n\tHfTuFhbsSgBbDeceG7s4oBPU8DD6fF+E56fnCH9HSNx8EGbkSu9jUOqrwaSDaAlB","X-Received":["by 2002:a05:6000:2307:b0:3a5:8934:4959 with SMTP id\n\tffacd0b85a97d-3b609544322mr2130389f8f.27.1752574218704; \n\tTue, 15 Jul 2025 03:10:18 -0700 (PDT)","by 2002:a05:6000:2307:b0:3a5:8934:4959 with SMTP id\n\tffacd0b85a97d-3b609544322mr2130360f8f.27.1752574218086; \n\tTue, 15 Jul 2025 03:10:18 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IH+zy3T4qNSUxY7mSTCPY+cslDyVR2/wOZK6I6b4AioxWfKGnivGEO/HemgNf/wE06NTUNtYQ==","From":"Milan Zamazal <mzamazal@redhat.com>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Cc":"Umang Jain <uajain@igalia.com>,  libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH] libcamera: pipeline: Always generate valid camera\n\tconfigurations","In-Reply-To":"<20250715091644.GG20231@pendragon.ideasonboard.com> (Laurent\n\tPinchart's message of \"Tue, 15 Jul 2025 12:16:44 +0300\")","References":"<20250715023013.16435-1-uajain@igalia.com>\n\t<20250715091644.GG20231@pendragon.ideasonboard.com>","Date":"Tue, 15 Jul 2025 12:10:16 +0200","Message-ID":"<85h5zdlq7r.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","User-Agent":"Gnus/5.13 (Gnus v5.13)","MIME-Version":"1.0","X-Mimecast-Spam-Score":"0","X-Mimecast-MFC-PROC-ID":"edCYRmoCsAKX-1SeXYJ9jvPHnmc4aX4QaUX0XDra38A_1752574219","X-Mimecast-Originator":"redhat.com","Content-Type":"text/plain","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":34908,"web_url":"https://patchwork.libcamera.org/comment/34908/","msgid":"<wmfru2qvsz7jfg6zb433mz2rt57pancgtj4slbkbdkmcn3huat@suqqfarzxis7>","date":"2025-07-16T04:52:34","subject":"Re: [PATCH] libcamera: pipeline: Always generate valid camera\n\tconfigurations","submitter":{"id":232,"url":"https://patchwork.libcamera.org/api/people/232/","name":"Umang Jain","email":"uajain@igalia.com"},"content":"On Tue, Jul 15, 2025 at 12:10:16PM +0200, Milan Zamazal wrote:\n> Hi Laurent,\n> \n> Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes:\n> \n> > Hi Umang,\n> >\n> > Thank you for the patch.\n> >\n> > On Tue, Jul 15, 2025 at 08:00:13AM +0530, Umang Jain wrote:\n> >> PipelineHandler::generateConfiguration() documentation states the\n> >> following:\n> >> \n> >>  * \\return A valid CameraConfiguration if the requested roles can be\n> >>  *  satisfied, or a null pointer otherwise.\n> >>  */\n> >> \n> >> Hence, always check the return status of config->validate(), during\n> >> generateConfiguration() call in each pipeline handler.\n> >> \n> >> Signed-off-by: Umang Jain <uajain@igalia.com>\n> >> ---\n> >>  src/libcamera/pipeline/imx8-isi/imx8-isi.cpp | 3 ++-\n> >>  src/libcamera/pipeline/rkisp1/rkisp1.cpp     | 3 ++-\n> >>  src/libcamera/pipeline/simple/simple.cpp     | 3 ++-\n> >>  src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 3 ++-\n> >>  src/libcamera/pipeline/vimc/vimc.cpp         | 3 ++-\n> >>  5 files changed, 10 insertions(+), 5 deletions(-)\n> >> \n> >> diff --git a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp\n> >> index f4014b95..c96fefa4 100644\n> >> --- a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp\n> >> +++ b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp\n> >> @@ -798,7 +798,8 @@ PipelineHandlerISI::generateConfiguration(Camera *camera,\n> >>  \t\tconfig->addConfiguration(cfg);\n> >>  \t}\n> >>  \n> >> -\tconfig->validate();\n> >> +\tif (config->validate() == CameraConfiguration::Invalid)\n> >> +\t\treturn nullptr;\n> >\n> > Returning nullptr from generateConfiguration() is meant to cover cases\n> > where the requested roles really can't be met. For instance, this can be\n> > the case when requesting more streams than the hardware supports, or\n> > when one of the stream roles has an unsupported value.\n> >\n> > The call to validate() in generateConfiguration() was initially added in\n> > a pipeline handler that filled part of the configuration in\n> > generateConfiguration() and relied on validate() to fill the rest,\n> > instead of duplicating the same logic in both functions. It then got\n> > copied blindly to other pipeline handlers. The validate() call is not\n> > supposed to return Invalid here, that would be a bug in the pipeline\n> > handler. \n> \n> Do we want to check that then in a different way?  At least by replacing\n> the check above with an assertion?\n\nI second this, along with a documentation extension of stating the logic\nof using validate() in generateConfiguration().\n\n> \n> > Blindly returning nullptr, and worse, returning it without any\n> > indication to the user as to what went wrong, doesn't seem desirable\n> > to me.\n> >\n> >>  \n> >>  \treturn config;\n> >>  }\n> >> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> >> index 675f0a74..fca1c74e 100644\n> >> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> >> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> >> @@ -787,7 +787,8 @@ PipelineHandlerRkISP1::generateConfiguration(Camera *camera,\n> >>  \t\tconfig->addConfiguration(cfg);\n> >>  \t}\n> >>  \n> >> -\tconfig->validate();\n> >> +\tif (config->validate() == CameraConfiguration::Invalid)\n> >> +\t\treturn nullptr;\n> >>  \n> >>  \treturn config;\n> >>  }\n> >> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> >> index efb07051..f994106e 100644\n> >> --- a/src/libcamera/pipeline/simple/simple.cpp\n> >> +++ b/src/libcamera/pipeline/simple/simple.cpp\n> >> @@ -1322,7 +1322,8 @@ SimplePipelineHandler::generateConfiguration(Camera *camera, Span<const StreamRo\n> >>  \t\tconfig->addConfiguration(cfg);\n> >>  \t}\n> >>  \n> >> -\tconfig->validate();\n> >> +\tif (config->validate() == CameraConfiguration::Invalid)\n> >> +\t\treturn nullptr;\n> >>  \n> >>  \treturn config;\n> >>  }\n> >> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> >> index 4b5816df..9ed85f3b 100644\n> >> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> >> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> >> @@ -250,7 +250,8 @@ PipelineHandlerUVC::generateConfiguration(Camera *camera,\n> >>  \n> >>  \tconfig->addConfiguration(cfg);\n> >>  \n> >> -\tconfig->validate();\n> >> +\tif (config->validate() == CameraConfiguration::Invalid)\n> >> +\t\treturn nullptr;\n> >>  \n> >>  \treturn config;\n> >>  }\n> >> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp\n> >> index 07273bd2..56719fa3 100644\n> >> --- a/src/libcamera/pipeline/vimc/vimc.cpp\n> >> +++ b/src/libcamera/pipeline/vimc/vimc.cpp\n> >> @@ -245,7 +245,8 @@ PipelineHandlerVimc::generateConfiguration(Camera *camera,\n> >>  \n> >>  \tconfig->addConfiguration(cfg);\n> >>  \n> >> -\tconfig->validate();\n> >> +\tif (config->validate() == CameraConfiguration::Invalid)\n> >> +\t\treturn nullptr;\n> >>  \n> >>  \treturn config;\n> >>  }\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 E21DEC3237\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed, 16 Jul 2025 04:52:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4B59168F67;\n\tWed, 16 Jul 2025 06:52:35 +0200 (CEST)","from fanzine2.igalia.com (fanzine2.igalia.com [213.97.179.56])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C98A26150A\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 16 Jul 2025 06:52:32 +0200 (CEST)","from [49.36.69.57] (helo=uajain) by fanzine2.igalia.com with\n\tesmtpsa \n\t(Cipher TLS1.3:ECDHE_SECP256R1__RSA_PSS_RSAE_SHA256__AES_256_GCM:256)\n\t(Exim) id 1ubu83-00H7Tt-Dh; Wed, 16 Jul 2025 06:52:27 +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=\"Qr7md7S0\"; 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=uQG0uTTQ9rx2anriUtKB+FJ2HkcqQcw+5DAiLhfkPjU=;\n\tb=Qr7md7S0SMjUZyWHPmh3uNmAAL\n\tId9jogFyqY+9n8PVUH2vj7Ts2L68rZ3/xQPBDL+kUBZ56g9uKXHxbdYtfR8GQFqFhCHi/ibiUeQyk\n\thdhS94qTSNjrjJIXer0mj3fzthTCF7js6JPUNuPNfps9ji0kTaHLVTfaslUhSjeB+pYQ3c6enq6Ey\n\tgf+cfPsctUj4+yWkIXfArApaWdK8NY324fn98a57Y8MZj1yU7IcKWykSpYr+woLkKf7q4bMsQCCHh\n\tSTKTMaw823gs/qriYVaeo9iTdqNKcAERz+zxuZX6Ytqiup5dBiuO6wYItZhODUW3/VmNRQFNpporQ\n\tmMDGYBlA==;","Date":"Wed, 16 Jul 2025 10:22:34 +0530","From":"Umang Jain <uajain@igalia.com>","To":"Milan Zamazal <mzamazal@redhat.com>","Cc":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>, \n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH] libcamera: pipeline: Always generate valid camera\n\tconfigurations","Message-ID":"<wmfru2qvsz7jfg6zb433mz2rt57pancgtj4slbkbdkmcn3huat@suqqfarzxis7>","References":"<20250715023013.16435-1-uajain@igalia.com>\n\t<20250715091644.GG20231@pendragon.ideasonboard.com>\n\t<85h5zdlq7r.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<85h5zdlq7r.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","User-Agent":"NeoMutt/20250510-dirty","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":34968,"web_url":"https://patchwork.libcamera.org/comment/34968/","msgid":"<175309768386.50296.16135851073111593986@ping.linuxembedded.co.uk>","date":"2025-07-21T11:34:43","subject":"Re: [PATCH] libcamera: pipeline: Always generate valid camera\n\tconfigurations","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Umang Jain (2025-07-16 05:52:34)\n> On Tue, Jul 15, 2025 at 12:10:16PM +0200, Milan Zamazal wrote:\n> > Hi Laurent,\n> > \n> > Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes:\n> > \n> > > Hi Umang,\n> > >\n> > > Thank you for the patch.\n> > >\n> > > On Tue, Jul 15, 2025 at 08:00:13AM +0530, Umang Jain wrote:\n> > >> PipelineHandler::generateConfiguration() documentation states the\n> > >> following:\n> > >> \n> > >>  * \\return A valid CameraConfiguration if the requested roles can be\n> > >>  *  satisfied, or a null pointer otherwise.\n> > >>  */\n> > >> \n> > >> Hence, always check the return status of config->validate(), during\n> > >> generateConfiguration() call in each pipeline handler.\n> > >> \n> > >> Signed-off-by: Umang Jain <uajain@igalia.com>\n> > >> ---\n> > >>  src/libcamera/pipeline/imx8-isi/imx8-isi.cpp | 3 ++-\n> > >>  src/libcamera/pipeline/rkisp1/rkisp1.cpp     | 3 ++-\n> > >>  src/libcamera/pipeline/simple/simple.cpp     | 3 ++-\n> > >>  src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 3 ++-\n> > >>  src/libcamera/pipeline/vimc/vimc.cpp         | 3 ++-\n> > >>  5 files changed, 10 insertions(+), 5 deletions(-)\n> > >> \n> > >> diff --git a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp\n> > >> index f4014b95..c96fefa4 100644\n> > >> --- a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp\n> > >> +++ b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp\n> > >> @@ -798,7 +798,8 @@ PipelineHandlerISI::generateConfiguration(Camera *camera,\n> > >>            config->addConfiguration(cfg);\n> > >>    }\n> > >>  \n> > >> -  config->validate();\n> > >> +  if (config->validate() == CameraConfiguration::Invalid)\n> > >> +          return nullptr;\n> > >\n> > > Returning nullptr from generateConfiguration() is meant to cover cases\n> > > where the requested roles really can't be met. For instance, this can be\n> > > the case when requesting more streams than the hardware supports, or\n> > > when one of the stream roles has an unsupported value.\n> > >\n> > > The call to validate() in generateConfiguration() was initially added in\n> > > a pipeline handler that filled part of the configuration in\n> > > generateConfiguration() and relied on validate() to fill the rest,\n> > > instead of duplicating the same logic in both functions. It then got\n> > > copied blindly to other pipeline handlers. The validate() call is not\n> > > supposed to return Invalid here, that would be a bug in the pipeline\n> > > handler. \n> > \n> > Do we want to check that then in a different way?  At least by replacing\n> > the check above with an assertion?\n> \n> I second this, along with a documentation extension of stating the logic\n> of using validate() in generateConfiguration().\n\n\nIf there's an assertion it should be in a common place. Perhaps at the\nCamera class directly ?\n\n--\nKieran\n\n\n> \n> > \n> > > Blindly returning nullptr, and worse, returning it without any\n> > > indication to the user as to what went wrong, doesn't seem desirable\n> > > to me.\n> > >\n> > >>  \n> > >>    return config;\n> > >>  }\n> > >> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > >> index 675f0a74..fca1c74e 100644\n> > >> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > >> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > >> @@ -787,7 +787,8 @@ PipelineHandlerRkISP1::generateConfiguration(Camera *camera,\n> > >>            config->addConfiguration(cfg);\n> > >>    }\n> > >>  \n> > >> -  config->validate();\n> > >> +  if (config->validate() == CameraConfiguration::Invalid)\n> > >> +          return nullptr;\n> > >>  \n> > >>    return config;\n> > >>  }\n> > >> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> > >> index efb07051..f994106e 100644\n> > >> --- a/src/libcamera/pipeline/simple/simple.cpp\n> > >> +++ b/src/libcamera/pipeline/simple/simple.cpp\n> > >> @@ -1322,7 +1322,8 @@ SimplePipelineHandler::generateConfiguration(Camera *camera, Span<const StreamRo\n> > >>            config->addConfiguration(cfg);\n> > >>    }\n> > >>  \n> > >> -  config->validate();\n> > >> +  if (config->validate() == CameraConfiguration::Invalid)\n> > >> +          return nullptr;\n> > >>  \n> > >>    return config;\n> > >>  }\n> > >> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > >> index 4b5816df..9ed85f3b 100644\n> > >> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > >> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > >> @@ -250,7 +250,8 @@ PipelineHandlerUVC::generateConfiguration(Camera *camera,\n> > >>  \n> > >>    config->addConfiguration(cfg);\n> > >>  \n> > >> -  config->validate();\n> > >> +  if (config->validate() == CameraConfiguration::Invalid)\n> > >> +          return nullptr;\n> > >>  \n> > >>    return config;\n> > >>  }\n> > >> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp\n> > >> index 07273bd2..56719fa3 100644\n> > >> --- a/src/libcamera/pipeline/vimc/vimc.cpp\n> > >> +++ b/src/libcamera/pipeline/vimc/vimc.cpp\n> > >> @@ -245,7 +245,8 @@ PipelineHandlerVimc::generateConfiguration(Camera *camera,\n> > >>  \n> > >>    config->addConfiguration(cfg);\n> > >>  \n> > >> -  config->validate();\n> > >> +  if (config->validate() == CameraConfiguration::Invalid)\n> > >> +          return nullptr;\n> > >>  \n> > >>    return config;\n> > >>  }\n> >","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 29544C3237\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 21 Jul 2025 11:34:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 59DD468FED;\n\tMon, 21 Jul 2025 13:34:51 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 68ED268FD0\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 21 Jul 2025 13:34:49 +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 9BA81243C;\n\tMon, 21 Jul 2025 13:34:10 +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=\"i5twNXZP\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1753097650;\n\tbh=ErLbvHZH31IgWE6AUlRV2Iix/lXU/S7yiGtUD6yBTAE=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=i5twNXZPilANF/g/tyCPp8uM6O+YY06XF/d0kwltAt0ikiQKScv6B+FBt+9PCy2hu\n\t+U10TBWWdFqHiBOoP3dQ/z+nTLTjrLZSWbBFjRFtIpQoLhrgIKQ6Qh0MuVjvLR0MH7\n\t2I2gQ4KmoaHdNaD+1PzaXKbH9viIs0u8bnRjrWxw=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<wmfru2qvsz7jfg6zb433mz2rt57pancgtj4slbkbdkmcn3huat@suqqfarzxis7>","References":"<20250715023013.16435-1-uajain@igalia.com>\n\t<20250715091644.GG20231@pendragon.ideasonboard.com>\n\t<85h5zdlq7r.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>\n\t<wmfru2qvsz7jfg6zb433mz2rt57pancgtj4slbkbdkmcn3huat@suqqfarzxis7>","Subject":"Re: [PATCH] libcamera: pipeline: Always generate valid camera\n\tconfigurations","From":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Cc":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","To":"Milan Zamazal <mzamazal@redhat.com>, Umang Jain <uajain@igalia.com>","Date":"Mon, 21 Jul 2025 12:34:43 +0100","Message-ID":"<175309768386.50296.16135851073111593986@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":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":34971,"web_url":"https://patchwork.libcamera.org/comment/34971/","msgid":"<pkenvn5f22lm3yzsb7gvcpzk5orkrnne4bogkru6yn5ubnqnbz@726iex5b5vhe>","date":"2025-07-21T13:40:11","subject":"Re: [PATCH] libcamera: pipeline: Always generate valid camera\n\tconfigurations","submitter":{"id":232,"url":"https://patchwork.libcamera.org/api/people/232/","name":"Umang Jain","email":"uajain@igalia.com"},"content":"On Mon, Jul 21, 2025 at 12:34:43PM +0100, Kieran Bingham wrote:\n> Quoting Umang Jain (2025-07-16 05:52:34)\n> > On Tue, Jul 15, 2025 at 12:10:16PM +0200, Milan Zamazal wrote:\n> > > Hi Laurent,\n> > > \n> > > Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes:\n> > > \n> > > > Hi Umang,\n> > > >\n> > > > Thank you for the patch.\n> > > >\n> > > > On Tue, Jul 15, 2025 at 08:00:13AM +0530, Umang Jain wrote:\n> > > >> PipelineHandler::generateConfiguration() documentation states the\n> > > >> following:\n> > > >> \n> > > >>  * \\return A valid CameraConfiguration if the requested roles can be\n> > > >>  *  satisfied, or a null pointer otherwise.\n> > > >>  */\n> > > >> \n> > > >> Hence, always check the return status of config->validate(), during\n> > > >> generateConfiguration() call in each pipeline handler.\n> > > >> \n> > > >> Signed-off-by: Umang Jain <uajain@igalia.com>\n> > > >> ---\n> > > >>  src/libcamera/pipeline/imx8-isi/imx8-isi.cpp | 3 ++-\n> > > >>  src/libcamera/pipeline/rkisp1/rkisp1.cpp     | 3 ++-\n> > > >>  src/libcamera/pipeline/simple/simple.cpp     | 3 ++-\n> > > >>  src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 3 ++-\n> > > >>  src/libcamera/pipeline/vimc/vimc.cpp         | 3 ++-\n> > > >>  5 files changed, 10 insertions(+), 5 deletions(-)\n> > > >> \n> > > >> diff --git a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp\n> > > >> index f4014b95..c96fefa4 100644\n> > > >> --- a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp\n> > > >> +++ b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp\n> > > >> @@ -798,7 +798,8 @@ PipelineHandlerISI::generateConfiguration(Camera *camera,\n> > > >>            config->addConfiguration(cfg);\n> > > >>    }\n> > > >>  \n> > > >> -  config->validate();\n> > > >> +  if (config->validate() == CameraConfiguration::Invalid)\n> > > >> +          return nullptr;\n> > > >\n> > > > Returning nullptr from generateConfiguration() is meant to cover cases\n> > > > where the requested roles really can't be met. For instance, this can be\n> > > > the case when requesting more streams than the hardware supports, or\n> > > > when one of the stream roles has an unsupported value.\n> > > >\n> > > > The call to validate() in generateConfiguration() was initially added in\n> > > > a pipeline handler that filled part of the configuration in\n> > > > generateConfiguration() and relied on validate() to fill the rest,\n> > > > instead of duplicating the same logic in both functions. It then got\n> > > > copied blindly to other pipeline handlers. The validate() call is not\n> > > > supposed to return Invalid here, that would be a bug in the pipeline\n> > > > handler. \n> > > \n> > > Do we want to check that then in a different way?  At least by replacing\n> > > the check above with an assertion?\n> > \n> > I second this, along with a documentation extension of stating the logic\n> > of using validate() in generateConfiguration().\n> \n> \n> If there's an assertion it should be in a common place. Perhaps at the\n> Camera class directly ?\n\nThat would be ideal, yes!\n\nBut that would also need to bring up the validate() call from individual\nPH to the camera class and check the return value enclosed in the\nASSERT. Alternative to that would be to have an out parameter in the\nfunction, to retrieve the validate() return value and then check in\nCamera class.\n\nCurrently we have all kind of offenders for generateConfiguration():\n\n- IPU3 + Mali-C55:\n          if (config->validate() == CameraConfiguration::Invalid)                 \n                return {}; \n- virtual:\n\tASSERT(config->validate() != CameraConfiguration::Invalid);\n\nSo indeed, some streamlining can be done.\n\n> \n> --\n> Kieran\n> \n> \n> > \n> > > \n> > > > Blindly returning nullptr, and worse, returning it without any\n> > > > indication to the user as to what went wrong, doesn't seem desirable\n> > > > to me.\n> > > >\n> > > >>  \n> > > >>    return config;\n> > > >>  }\n> > > >> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > >> index 675f0a74..fca1c74e 100644\n> > > >> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > >> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > >> @@ -787,7 +787,8 @@ PipelineHandlerRkISP1::generateConfiguration(Camera *camera,\n> > > >>            config->addConfiguration(cfg);\n> > > >>    }\n> > > >>  \n> > > >> -  config->validate();\n> > > >> +  if (config->validate() == CameraConfiguration::Invalid)\n> > > >> +          return nullptr;\n> > > >>  \n> > > >>    return config;\n> > > >>  }\n> > > >> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> > > >> index efb07051..f994106e 100644\n> > > >> --- a/src/libcamera/pipeline/simple/simple.cpp\n> > > >> +++ b/src/libcamera/pipeline/simple/simple.cpp\n> > > >> @@ -1322,7 +1322,8 @@ SimplePipelineHandler::generateConfiguration(Camera *camera, Span<const StreamRo\n> > > >>            config->addConfiguration(cfg);\n> > > >>    }\n> > > >>  \n> > > >> -  config->validate();\n> > > >> +  if (config->validate() == CameraConfiguration::Invalid)\n> > > >> +          return nullptr;\n> > > >>  \n> > > >>    return config;\n> > > >>  }\n> > > >> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > > >> index 4b5816df..9ed85f3b 100644\n> > > >> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > > >> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > > >> @@ -250,7 +250,8 @@ PipelineHandlerUVC::generateConfiguration(Camera *camera,\n> > > >>  \n> > > >>    config->addConfiguration(cfg);\n> > > >>  \n> > > >> -  config->validate();\n> > > >> +  if (config->validate() == CameraConfiguration::Invalid)\n> > > >> +          return nullptr;\n> > > >>  \n> > > >>    return config;\n> > > >>  }\n> > > >> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp\n> > > >> index 07273bd2..56719fa3 100644\n> > > >> --- a/src/libcamera/pipeline/vimc/vimc.cpp\n> > > >> +++ b/src/libcamera/pipeline/vimc/vimc.cpp\n> > > >> @@ -245,7 +245,8 @@ PipelineHandlerVimc::generateConfiguration(Camera *camera,\n> > > >>  \n> > > >>    config->addConfiguration(cfg);\n> > > >>  \n> > > >> -  config->validate();\n> > > >> +  if (config->validate() == CameraConfiguration::Invalid)\n> > > >> +          return nullptr;\n> > > >>  \n> > > >>    return config;\n> > > >>  }\n> > >","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 26092BDCC1\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 21 Jul 2025 13:40:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D8B0668FE9;\n\tMon, 21 Jul 2025 15:40:13 +0200 (CEST)","from fanzine2.igalia.com (fanzine2.igalia.com [213.97.179.56])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4316B68FB1\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 21 Jul 2025 15:40:10 +0200 (CEST)","from [49.36.71.87] (helo=uajain) by fanzine2.igalia.com with\n\tesmtpsa \n\t(Cipher TLS1.3:ECDHE_SECP256R1__RSA_PSS_RSAE_SHA256__AES_256_GCM:256)\n\t(Exim) id 1udqkQ-001itK-2O; Mon, 21 Jul 2025 15:40:06 +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=\"fMVsPlwz\"; 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=VvgMEU+20L7BZVwF0OJ7scIdDr0C6jU0RCDRczyNfLs=;\n\tb=fMVsPlwzut4w3Y/NyA46CYRXa1\n\tqLPFFPvPTovNlUKfOdho5/Mddv2boS7onAdxBiPRzvo+a2kx5DlKXkTRtuZ9nMjUUOHvjTIt1TS7b\n\tpQCv9tijBrPzwyF6uHzTAJ+IknpvIPrJRa4wBWmyo/BdWv9w+cFty5av75h1zgrdngjwOlLPLTGM1\n\tstUb4kUPbMJzzoCDauCHVYm8oSsWsvr+JCXqL048reXXrSv2VhJAgrLxM4wTc7xRDbwvv4d0ZCIdi\n\tLW56nHfuPfnNa8M5DB6OcbzDE6NPTfJwMgORPIMJ2Dd6rYmq7ef8kabnO7GihSCdaGzmj+KhGUICe\n\tcNGkssDA==;","Date":"Mon, 21 Jul 2025 19:10:11 +0530","From":"Umang Jain <uajain@igalia.com>","To":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Cc":"Milan Zamazal <mzamazal@redhat.com>, \n\tLaurent Pinchart <laurent.pinchart@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH] libcamera: pipeline: Always generate valid camera\n\tconfigurations","Message-ID":"<pkenvn5f22lm3yzsb7gvcpzk5orkrnne4bogkru6yn5ubnqnbz@726iex5b5vhe>","References":"<20250715023013.16435-1-uajain@igalia.com>\n\t<20250715091644.GG20231@pendragon.ideasonboard.com>\n\t<85h5zdlq7r.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>\n\t<wmfru2qvsz7jfg6zb433mz2rt57pancgtj4slbkbdkmcn3huat@suqqfarzxis7>\n\t<175309768386.50296.16135851073111593986@ping.linuxembedded.co.uk>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<175309768386.50296.16135851073111593986@ping.linuxembedded.co.uk>","User-Agent":"NeoMutt/20250510-dirty","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]