[{"id":33358,"web_url":"https://patchwork.libcamera.org/comment/33358/","msgid":"<20250214162616.GA18264@pendragon.ideasonboard.com>","date":"2025-02-14T16:26:16","subject":"Re: [PATCH v2 2/2] libcamera: formats: Deduplicate\n\tPixelFormatInfo::info() code","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Stefan,\n\nOn Fri, Feb 14, 2025 at 05:09:48PM +0100, Stefan Klug wrote:\n> Use PixelFormatInfo::info(const PixelFormat &format) to implement\n> PixelFormatInfo::info(const V4L2PixelFormat &format).\n> \n> This has one noteworthy side effect: If info(V4L2PixelFormat &format) is\n> called with a valid but unsupported (by libcamera) format, we now get\n> the same warning as in the info(PixelFormat &format) case.\n\nNow that you state it that way, can it actually ever happen ? For the\nwarning to be printed, we would need a V4L2PixelFormat that the vpf2pf\ntable (in v4l2_pixelformat.cpp) knows about, but that is unknown to\npixelFormatInfo (in formats.cpp). Give that every entry in vpf2pf\nrequires a PixelFormat, I would expect it to be present in\npixelFormatInfo.\n\nIf we hit the warning, it means there would be a clear bug in libcamera.\nI initially thought this patch could introduce verbose warnings that we\nmay not want to see, and may need to be reverted in the future. It now\nsounds like the warning will only be printed when something goes really\nwrong. That lowers my concern.\n\nYou may want to indicate in the commit message that we expect the\nwarning only in case of a bug in the libcamera core. Apart from that,\nthe patch looks good, you can keep my Rb tag.\n\n> Signed-off-by: Stefan Klug \n> Reviewed-by: Kieran Bingham \n> Reviewed-by: Laurent Pinchart \n> ---\n> src/libcamera/formats.cpp | 9 +--------\n> 1 file changed, 1 insertion(+), 8 deletions(-)\n> \n> diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp\n> index b4518e61d04a..df7413f58ba8 100644\n> --- a/src/libcamera/formats.cpp\n> +++ b/src/libcamera/formats.cpp\n> @@ -1024,14 +1024,7 @@ const PixelFormatInfo &PixelFormatInfo::info(const PixelFormat &format)\n> const PixelFormatInfo &PixelFormatInfo::info(const V4L2PixelFormat &format)\n> {\n> \tPixelFormat pixelFormat = format.toPixelFormat(false);\n> -\tif (!pixelFormat.isValid())\n> -\t\treturn pixelFormatInfoInvalid;\n> -\n> -\tconst auto iter = pixelFormatInfo.find(pixelFormat);\n> -\tif (iter == pixelFormatInfo.end())\n> -\t\treturn pixelFormatInfoInvalid;\n> -\n> -\treturn iter->second;\n> +\treturn info(pixelFormat);\n> }\n> \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 36B51C32A3\n\tfor ;\n\tFri, 14 Feb 2025 16:26:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 295C06864D;\n\tFri, 14 Feb 2025 17:26:31 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7B5796862A\n\tfor ;\n\tFri, 14 Feb 2025 17:26:29 +0100 (CET)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C509E710;\n\tFri, 14 Feb 2025 17:25:09 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"gLgeKqqL\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1739550310;\n\tbh=Js9R6xomwSM5S01qSjmA3zoYGvgmuJtIesRNzOaRJZ0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=gLgeKqqL2DRB8Y5TjHb+6l1njuUeAidluUTgFYsLi7tp7WLZp+DCgyUEBIVeiKWW0\n\tOc2OEzAQIt/6JHDKU+Bgf8CyGtLWwI/s5ZkJtxfBqBkELmpa8bFW4NMW0yUE5V3ZYA\n\t8Vtl/5cpgFJ+2VhPGdJ+YjLu0d0//fmOyzjWlesk=","Date":"Fri, 14 Feb 2025 18:26:16 +0200","From":"Laurent Pinchart ","To":"Stefan Klug ","Cc":"libcamera-devel@lists.libcamera.org,\n\tKieran Bingham ","Subject":"Re: [PATCH v2 2/2] libcamera: formats: Deduplicate\n\tPixelFormatInfo::info() code","Message-ID":"<20250214162616.GA18264@pendragon.ideasonboard.com>","References":"<20250214161031.240481-1-stefan.klug@ideasonboard.com>\n\t<20250214161031.240481-3-stefan.klug@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20250214161031.240481-3-stefan.klug@ideasonboard.com>","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\" "}}]