[{"id":26668,"web_url":"https://patchwork.libcamera.org/comment/26668/","msgid":"<20230319195139.GE13726@pendragon.ideasonboard.com>","date":"2023-03-19T19:51:39","subject":"Re: [libcamera-devel] [PATCH v3 1/4] libcamera: rkisp1: Generate\n\tconfig using main path","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the patch.\n\nOn Tue, Mar 07, 2023 at 12:48:01PM +0100, Jacopo Mondi via libcamera-devel wrote:\n> The generateConfiguration() implementation in the Rockchip RkISP1\n> pipeline handler uses by default the self path (if available) for\n> the Viewfinder and VideoRecording StreamRoles.\n> \n> The validate() implementation, at the contrary, prefers using the main\n> path, when available, for all streams.\n> \n> As the self-path is limited in output resolution to 1920x1920,\n> generating a configuration using the self path limits the maximum\n> stream size to 1920x1920, while higher resolutions can be obtained by\n> using the main path.\n> \n> Align the generateConfiguration() implementation to the validate() one\n> by using the main path by default if available.\n>\n> Bug: https://bugs.libcamera.org/show_bug.cgi?id=180\n> Reported-by: libcamera@luigi311.com\n> Signed-off-by: Jacopo Mondi \n> Reviewed-by: Paul Elder \n\nThere's a reason we prefer the self path for some of the roles, as\nexplained in the git history:\n\ncommit 921c0cdc6a0c486c9b53af5746b1cce6a2501b3e\nAuthor: Niklas Söderlund \nDate: Thu Aug 13 01:51:21 2020 +0200\n\n libcamera: pipeline: rkisp1: Expose self path stream\n\n Expose the self stream to applications and prefers it for the viewfinder\n and video roles as it can be extended to produce RGB. Keep preferring\n the main path for still capture as it could be extended to support RAW\n formats which makes most sense for still capture.\n\n With this change the self path becomes available to applications and a\n camera backed by this pipeline can produce two streams simultaneously.\n\n Signed-off-by: Niklas Söderlund \n Reviewed-by: Jacopo Mondi \n Reviewed-by: Laurent Pinchart \n\nSwitching to the main path unconditionally, while making higher\nresolutions available, prevents capturing RGB formats.\n\nI'm fine with this patch, but it shows we have an API problem as we can\nexpose the capabilities of the device correctly to the application.\nCould you start thinking how this could be improved ?\n\n> ---\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 9 +--------\n> 1 file changed, 1 insertion(+), 8 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index 096c9cca3a0a..ebc9bef8688a 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -647,23 +647,19 @@ PipelineHandlerRkISP1::generateConfiguration(Camera *camera,\n> \t * first stream and use it for all streams.\n> \t */\n> \tstd::optional colorSpace;\n> -\n> \tbool mainPathAvailable = true;\n> -\tbool selfPathAvailable = data->selfPath_;\n> \n> \tfor (const StreamRole role : roles) {\n> -\t\tbool useMainPath;\n> +\t\tbool useMainPath = mainPathAvailable;\n\nYou can drop the useMainPath variable and use mainPathAvailable in the\nonly location where useMainPath is still used. With this change,\n\nReviewed-by: Laurent Pinchart \n\n> \n> \t\tswitch (role) {\n> \t\tcase StreamRole::StillCapture:\n> -\t\t\tuseMainPath = mainPathAvailable;\n> \t\t\t/* JPEG encoders typically expect sYCC. */\n> \t\t\tif (!colorSpace)\n> \t\t\t\tcolorSpace = ColorSpace::Sycc;\n> \t\t\tbreak;\n> \n> \t\tcase StreamRole::Viewfinder:\n> -\t\t\tuseMainPath = !selfPathAvailable;\n> \t\t\t/*\n> \t\t\t * sYCC is the YCbCr encoding of sRGB, which is commonly\n> \t\t\t * used by displays.\n> @@ -673,7 +669,6 @@ PipelineHandlerRkISP1::generateConfiguration(Camera *camera,\n> \t\t\tbreak;\n> \n> \t\tcase StreamRole::VideoRecording:\n> -\t\t\tuseMainPath = !selfPathAvailable;\n> \t\t\t/* Rec. 709 is a good default for HD video recording. */\n> \t\t\tif (!colorSpace)\n> \t\t\t\tcolorSpace = ColorSpace::Rec709;\n> @@ -686,7 +681,6 @@ PipelineHandlerRkISP1::generateConfiguration(Camera *camera,\n> \t\t\t\treturn nullptr;\n> \t\t\t}\n> \n> -\t\t\tuseMainPath = true;\n> \t\t\tcolorSpace = ColorSpace::Raw;\n> \t\t\tbreak;\n> \n> @@ -703,7 +697,6 @@ PipelineHandlerRkISP1::generateConfiguration(Camera *camera,\n> \t\t\tmainPathAvailable = false;\n> \t\t} else {\n> \t\t\tpath = data->selfPath_;\n> -\t\t\tselfPathAvailable = false;\n> \t\t}\n> \n> \t\tStreamConfiguration cfg =","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 05FC5BD80A\n\tfor ;\n\tSun, 19 Mar 2023 19:51:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 42946626CA;\n\tSun, 19 Mar 2023 20:51:41 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0122E626CA\n\tfor ;\n\tSun, 19 Mar 2023 20:51:38 +0100 (CET)","from pendragon.ideasonboard.com (213-209-177-213.ip.skylogicnet.com\n\t[213.209.177.213])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 35F7B189A;\n\tSun, 19 Mar 2023 20:51:36 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1679255501;\n\tbh=rLoB4Q+54sSRmOvYUBBgW4yYsugdCZV9oMp6TWc76t4=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=Thd/FC/XzmobzfeJXp4ChFICmHzJuwKACnYdDyk0D8JeyY/+GoQ0St/vsBgWixnS8\n\t0ugeuhQ6KUkdmlSjxNod8MQrTFOkkQUcN4CeHvFw92WgfSG1aSPkzL4FqQfsRTgF50\n\tUpEubu/vKoMYUW0hsIRaxYwjA4ha2Eylfsvh1uHzPvR2WvZDCFSwyMBUZJXGTddeMN\n\tTutFMtmz6v30loiFfbcD37BPxn6E99a22Uayz2RK5fSIhKxsRNT1ugsvrOSAK6YHXH\n\tc1xu3GBUZTk3t+2ob4twdHoCbcFw4WmTad9PkGdFg/5iegN3ZSXHXvkqqy8ZaBQMIY\n\ty7tbM5kUZq/nA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1679255498;\n\tbh=rLoB4Q+54sSRmOvYUBBgW4yYsugdCZV9oMp6TWc76t4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=B7JX0qrwyC2G4E4Hu5fewPUHZVUc2Do4JoLS1TB0BBy3cQzbUyifzRbDyw9eK6DWm\n\tQAF1R8ID8MbPFkYcDBqALCfgXlvJgYqSaCXHh5wFcsHZ3q86Ngk6/8yGW9fTaLK7r6\n\tHlPETIuLAW7a/mFT6a1tcLkVRdhViWoyd67r+ltU="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"B7JX0qrw\"; dkim-atps=neutral","Date":"Sun, 19 Mar 2023 21:51:39 +0200","To":"Jacopo Mondi ","Message-ID":"<20230319195139.GE13726@pendragon.ideasonboard.com>","References":"<20230307114804.42291-1-jacopo.mondi@ideasonboard.com>\n\t<20230307114804.42291-2-jacopo.mondi@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20230307114804.42291-2-jacopo.mondi@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 1/4] libcamera: rkisp1: Generate\n\tconfig using main path","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","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org, libcamera@luigi311.com","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]