[{"id":21726,"web_url":"https://patchwork.libcamera.org/comment/21726/","msgid":"","date":"2021-12-09T22:02:42","subject":"Re: [libcamera-devel] [PATCH v10 8/8] libcamera: pipeline:\n\traspberrypi: Support color spaces","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi David,\n\nThank you for the patch.\n\nOn Thu, Dec 09, 2021 at 10:12:45AM +0000, David Plowman wrote:\n> The Raspberry Pi pipeline handler now sets color spaces correctly.\n> \n> In generateConfiguration() it sets them to reasonable default values\n> based on the stream role.\n> \n> validate() now calls validateColorSpaces() to ensure that the\n> requested color spaces are sensible, before proceeding to check what\n> the hardware can deliver.\n\nOverall this looks good, but there are two questions in v9 for which I\nhaven't seen an answer. It doesn't mean there are issues to be\naddressed, I'd just like to make sure. I've copied the questions below.\n\n> Signed-off-by: David Plowman \n> ---\n> .../pipeline/raspberrypi/raspberrypi.cpp | 40 +++++++++++++++++++\n> 1 file changed, 40 insertions(+)\n> \n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index 22c8ee68..368953e1 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -96,6 +96,7 @@ V4L2DeviceFormat toV4L2DeviceFormat(const V4L2SubdeviceFormat &format,\n> \n> \tdeviceFormat.fourcc = V4L2PixelFormat::fromPixelFormat(pix);\n> \tdeviceFormat.size = format.size;\n> +\tdeviceFormat.colorSpace = format.colorSpace;\n> \treturn deviceFormat;\n> }\n> \n> @@ -132,6 +133,7 @@ V4L2SubdeviceFormat findBestFormat(const SensorFormats &formatsMap, const Size &\n> {\n> \tdouble bestScore = std::numeric_limits::max(), score;\n> \tV4L2SubdeviceFormat bestFormat;\n> +\tbestFormat.colorSpace = ColorSpace::Raw;\n> \n> \tconstexpr float penaltyAr = 1500.0;\n> \tconstexpr float penaltyBitDepth = 500.0;\n> @@ -329,6 +331,8 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()\n> \tif (config_.empty())\n> \t\treturn Invalid;\n> \n> +\tstatus = validateColorSpaces(ColorSpaceFlag::StreamsShareColorSpace);\n\nWouldn't it be better to validate colour spaces after pixel formats have\nbeen validated ? Otherwise your isRaw() check during colour space\nvalidation may not match with the pixel format after validation.\n\n> +\n> \t/*\n> \t * What if the platform has a non-90 degree rotation? We can't even\n> \t * \"adjust\" the configuration and carry on. Alternatively, raising an\n> @@ -496,11 +500,25 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()\n> \t\tV4L2DeviceFormat format;\n> \t\tformat.fourcc = V4L2PixelFormat::fromPixelFormat(cfg.pixelFormat);\n> \t\tformat.size = cfg.size;\n> +\t\tformat.colorSpace = cfg.colorSpace;\n> +\n> +\t\tLOG(RPI, Debug)\n> +\t\t\t<< \"Try color space \" << ColorSpace::toString(cfg.colorSpace);\n> \n> \t\tint ret = dev->tryFormat(&format);\n> \t\tif (ret)\n> \t\t\treturn Invalid;\n> \n> +\t\tif (!format.colorSpace || cfg.colorSpace != format.colorSpace) {\n> +\t\t\tstatus = Adjusted;\n> +\t\t\tLOG(RPI, Warning)\n> +\t\t\t\t<< \"Color space changed from \"\n> +\t\t\t\t<< ColorSpace::toString(cfg.colorSpace) << \" to \"\n> +\t\t\t\t<< ColorSpace::toString(format.colorSpace);\n\nWarning or Debug ? Adjusting a configuration is a normal use case.\n\n> +\t\t}\n> +\n> +\t\tcfg.colorSpace = format.colorSpace;\n> +\n> \t\tcfg.stride = format.planes[0].bpl;\n> \t\tcfg.frameSize = format.planes[0].size;\n> \n> @@ -524,6 +542,7 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,\n> \tPixelFormat pixelFormat;\n> \tV4L2VideoDevice::Formats fmts;\n> \tSize size;\n> +\tstd::optional colorSpace;\n> \n> \tif (roles.empty())\n> \t\treturn config;\n> @@ -538,6 +557,7 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,\n> \t\t\tpixelFormat = mbusCodeToPixelFormat(sensorFormat.mbus_code,\n> \t\t\t\t\t\t\t BayerFormat::Packing::CSI2);\n> \t\t\tASSERT(pixelFormat.isValid());\n> +\t\t\tcolorSpace = ColorSpace::Raw;\n> \t\t\tbufferCount = 2;\n> \t\t\trawCount++;\n> \t\t\tbreak;\n> @@ -545,6 +565,12 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,\n> \t\tcase StreamRole::StillCapture:\n> \t\t\tfmts = data->isp_[Isp::Output0].dev()->formats();\n> \t\t\tpixelFormat = formats::NV12;\n> +\t\t\t/*\n> +\t\t\t * Still image codecs usually expect the JPEG color space.\n> +\t\t\t * Even RGB codecs will be fine as the RGB we get with the\n> +\t\t\t * JPEG color space is the same as sRGB.\n> +\t\t\t */\n> +\t\t\tcolorSpace = ColorSpace::Jpeg;\n> \t\t\t/* Return the largest sensor resolution. */\n> \t\t\tsize = data->sensor_->resolution();\n> \t\t\tbufferCount = 1;\n> @@ -562,6 +588,11 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,\n> \t\t\t */\n> \t\t\tfmts = data->isp_[Isp::Output0].dev()->formats();\n> \t\t\tpixelFormat = formats::YUV420;\n> +\t\t\t/*\n> +\t\t\t * Choose a color space appropriate for video recording.\n> +\t\t\t * Rec.709 will be a good default for HD resolutions.\n> +\t\t\t */\n> +\t\t\tcolorSpace = ColorSpace::Rec709;\n> \t\t\tsize = { 1920, 1080 };\n> \t\t\tbufferCount = 4;\n> \t\t\toutCount++;\n> @@ -570,6 +601,7 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,\n> \t\tcase StreamRole::Viewfinder:\n> \t\t\tfmts = data->isp_[Isp::Output0].dev()->formats();\n> \t\t\tpixelFormat = formats::ARGB8888;\n> +\t\t\tcolorSpace = ColorSpace::Jpeg;\n> \t\t\tsize = { 800, 600 };\n> \t\t\tbufferCount = 4;\n> \t\t\toutCount++;\n> @@ -612,6 +644,7 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,\n> \t\tStreamConfiguration cfg(formats);\n> \t\tcfg.size = size;\n> \t\tcfg.pixelFormat = pixelFormat;\n> +\t\tcfg.colorSpace = colorSpace;\n> \t\tcfg.bufferCount = bufferCount;\n> \t\tconfig->addConfiguration(cfg);\n> \t}\n> @@ -719,6 +752,7 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)\n> \t\tV4L2PixelFormat fourcc = V4L2PixelFormat::fromPixelFormat(cfg.pixelFormat);\n> \t\tformat.size = cfg.size;\n> \t\tformat.fourcc = fourcc;\n> +\t\tformat.colorSpace = cfg.colorSpace;\n> \n> \t\tLOG(RPI, Debug) << \"Setting \" << stream->name() << \" to \"\n> \t\t\t\t<< format.toString();\n> @@ -734,6 +768,10 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)\n> \t\t\treturn -EINVAL;\n> \t\t}\n> \n> +\t\tLOG(RPI, Debug)\n> +\t\t\t<< \"Stream \" << stream->name() << \" has color space \"\n> +\t\t\t<< ColorSpace::toString(cfg.colorSpace);\n> +\n> \t\tcfg.setStream(stream);\n> \t\tstream->setExternal(true);\n> \n> @@ -758,6 +796,8 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)\n> \t\tformat = {};\n> \t\tformat.size = maxSize;\n> \t\tformat.fourcc = V4L2PixelFormat::fromPixelFormat(formats::YUV420);\n> +\t\t/* No one asked for output, so the color space doesn't matter. */\n> +\t\tformat.colorSpace = ColorSpace::Jpeg;\n> \t\tret = data->isp_[Isp::Output0].dev()->setFormat(&format);\n> \t\tif (ret) {\n> \t\t\tLOG(RPI, Error)","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 9CE1DBDB13\n\tfor ;\n\tThu, 9 Dec 2021 22:03:13 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DC7416087A;\n\tThu, 9 Dec 2021 23:03:12 +0100 (CET)","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 D583F607DE\n\tfor ;\n\tThu, 9 Dec 2021 23:03:11 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 2D1EA501;\n\tThu, 9 Dec 2021 23:03:11 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"su0xbZ5i\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1639087391;\n\tbh=yJFbESVQfmpScl8yF09oQ/NEX0DiEQd9gW2Oq4q9r8k=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=su0xbZ5i27V5wgBZGH562JauHnz+XqBWzXE0vo0nPGisgxwNtoBHZWPiF9QvmrRD+\n\tTR6vaoDOjzWkpw8v4XtjrY3/H8rIkOTZykTE/TtwxBVxCuAImJWB9/gPj29sDQJDM/\n\tvf2065GU53tcs1pqFAOtlBhUT2D94psWXsCKCmHs=","Date":"Fri, 10 Dec 2021 00:02:42 +0200","From":"Laurent Pinchart ","To":"David Plowman ","Message-ID":"","References":"<20211209101245.6187-1-david.plowman@raspberrypi.com>\n\t<20211209101245.6187-9-david.plowman@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211209101245.6187-9-david.plowman@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v10 8/8] libcamera: pipeline:\n\traspberrypi: Support color spaces","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","Cc":"Tomasz Figa , libcamera-devel@lists.libcamera.org,\n\tHans Verkuil ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]