[{"id":21199,"web_url":"https://patchwork.libcamera.org/comment/21199/","msgid":"<163775998961.3059017.4992064762819465621@Monstersaurus>","date":"2021-11-24T13:19:49","subject":"Re: [libcamera-devel] [PATCH v6 5/7] libcamera: Support passing\n\tColorSpaces to V4L2 subdevices","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting David Plowman (2021-11-18 15:19:31)\n> The ColorSpace from the StreamConfiguration is now handled\n> appropriately in the V4L2Subdevice.\n> \n> Signed-off-by: David Plowman \n> ---\n> include/libcamera/internal/v4l2_subdevice.h | 2 ++\n> src/libcamera/camera_sensor.cpp | 1 +\n> src/libcamera/v4l2_subdevice.cpp | 35 ++++++++++++++++++++-\n> 3 files changed, 37 insertions(+), 1 deletion(-)\n> \n> diff --git a/include/libcamera/internal/v4l2_subdevice.h b/include/libcamera/internal/v4l2_subdevice.h\n> index 97b89fb9..a6a0a870 100644\n> --- a/include/libcamera/internal/v4l2_subdevice.h\n> +++ b/include/libcamera/internal/v4l2_subdevice.h\n> @@ -14,6 +14,7 @@\n> #include \n> #include \n> \n> +#include \n> #include \n> \n> #include \"libcamera/internal/formats.h\"\n> @@ -27,6 +28,7 @@ class MediaDevice;\n> struct V4L2SubdeviceFormat {\n> uint32_t mbus_code;\n> Size size;\n> + std::optional colorSpace;\n> \n> const std::string toString() const;\n> uint8_t bitsPerPixel() const;\n> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> index 9fdb8c09..6fcd1c1d 100644\n> --- a/src/libcamera/camera_sensor.cpp\n> +++ b/src/libcamera/camera_sensor.cpp\n> @@ -613,6 +613,7 @@ V4L2SubdeviceFormat CameraSensor::getFormat(const std::vector &mbu\n> V4L2SubdeviceFormat format{\n> .mbus_code = bestCode,\n> .size = *bestSize,\n> + .colorSpace = ColorSpace::Raw,\n> };\n> \n> return format;\n> diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp\n> index 023e2328..b60ab69a 100644\n> --- a/src/libcamera/v4l2_subdevice.cpp\n> +++ b/src/libcamera/v4l2_subdevice.cpp\n> @@ -168,6 +168,16 @@ const std::map formatInfoMap = {\n> * \\brief The image size in pixels\n> */\n> \n> +/**\n> + * \\var V4L2SubdeviceFormat::colorSpace\n> + * \\brief The color space of the pixels\n> + *\n> + * The color space of the image. When setting the format this may be\n> + * unset, in which case the driver gets to use its default color space.\n> + * After being set, this value should contain the color space that the\n\nshould ? or will? (or ... shall?)\n\n/that the was/that was/\n\n\n> + * was actually used.\n\nI'd put something in about how it is represented when invalid too:\n\nPinch or adapt from below if there's anything useful there...\n\n\"\"\"\nThe color space of the image. When setting the format this may be left\nunset, which will request the default color space from the driver. When\na format is returned from a device, this will contain the color space\nthat is used or become unset if an invalid or unsupported color space\nwas provided by the driver.\n\"\"\"\n\n\n> + */\n> +\n> /**\n> * \\brief Assemble and return a string describing the format\n> * \\return A string describing the V4L2SubdeviceFormat\n> @@ -400,6 +410,16 @@ int V4L2Subdevice::getFormat(unsigned int pad, V4L2SubdeviceFormat *format,\n> format->size.height = subdevFmt.format.height;\n> format->mbus_code = subdevFmt.format.code;\n> \n> + format->colorSpace = toColorSpace(subdevFmt.format);\n> + /*\n> + * This warning can be ignored on metadata pads. These are normally\n> + * pads other than zero.\n> + * \\todo find a way to detect metadata pads and ignore them\n> + */\n> + if (!format->colorSpace)\n> + LOG(V4L2, Warning)\n> + << \"Retrieved unrecognised color space on pad \" << pad;\n> +\n> return 0;\n> }\n> \n> @@ -418,6 +438,9 @@ int V4L2Subdevice::getFormat(unsigned int pad, V4L2SubdeviceFormat *format,\n> int V4L2Subdevice::setFormat(unsigned int pad, V4L2SubdeviceFormat *format,\n> Whence whence)\n> {\n> + if (!format->colorSpace)\n> + LOG(V4L2, Error) << \"Setting an undefined color space\";\n> +\n\nFrom my understanding now, this isn't an error as it will request the\ncolor space from the driver?\n\n> struct v4l2_subdev_format subdevFmt = {};\n> subdevFmt.which = whence == ActiveFormat ? V4L2_SUBDEV_FORMAT_ACTIVE\n> : V4L2_SUBDEV_FORMAT_TRY;\n> @@ -427,7 +450,13 @@ int V4L2Subdevice::setFormat(unsigned int pad, V4L2SubdeviceFormat *format,\n> subdevFmt.format.code = format->mbus_code;\n> subdevFmt.format.field = V4L2_FIELD_NONE;\n> \n> - int ret = ioctl(VIDIOC_SUBDEV_S_FMT, &subdevFmt);\n> + int ret = fromColorSpace(format->colorSpace, subdevFmt.format);\n> + if (ret < 0)\n> + LOG(V4L2, Warning)\n> + << \"Setting color space unrecognised by V4L2: \"\n\nBut this should be ... as this is where we need to know what was/is to\nbe used...?\n\n> + << ColorSpace::toString(format->colorSpace);\n> +\n> + ret = ioctl(VIDIOC_SUBDEV_S_FMT, &subdevFmt);\n> if (ret) {\n> LOG(V4L2, Error)\n> << \"Unable to set format on pad \" << pad\n> @@ -439,6 +468,10 @@ int V4L2Subdevice::setFormat(unsigned int pad, V4L2SubdeviceFormat *format,\n> format->size.height = subdevFmt.format.height;\n> format->mbus_code = subdevFmt.format.code;\n> \n> + format->colorSpace = toColorSpace(subdevFmt.format);\n> + if (!format->colorSpace)\n> + LOG(V4L2, Warning) << \"Unrecognised color space has been set\";\n\nHere too should be Error?\n\n> +\n> return 0;\n> }\n> \n> -- \n> 2.20.1\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 25F98BF415\n\tfor ;\n\tWed, 24 Nov 2021 13:19:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 75D0A60371;\n\tWed, 24 Nov 2021 14:19:54 +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 E685B60128\n\tfor ;\n\tWed, 24 Nov 2021 14:19:52 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 6C0C1993;\n\tWed, 24 Nov 2021 14:19:52 +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=\"G7l0LKGm\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1637759992;\n\tbh=1a3Q9Fu/aAmcIMTtC/AgGQKm5brJ3rawvbdLLbaBVCA=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=G7l0LKGmXnXG+iHP7x708/z8DsxmlIM+UbH2b2okIjWjaSEey6BhFH8aW98ot2LUj\n\t8wMyNNUnRkO9Bf+G82znSL/w3HimPKYpfZkc7USFTqpAUMXlpFSOWy+9x/lsMEbTj2\n\t2sx/2aJpxCiSCl8ZcvAdz2SdH7vuCKLkZKm8nrbU=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20211118151933.15627-6-david.plowman@raspberrypi.com>","References":"<20211118151933.15627-1-david.plowman@raspberrypi.com>\n\t<20211118151933.15627-6-david.plowman@raspberrypi.com>","From":"Kieran Bingham ","To":"David Plowman , hverkuil-cisco@xs4all.nl, \n\tjacopo@jmondi.org, laurent.pinchart@ideasonboard.com,\n\tlibcamera-devel@lists.libcamera.org, naush@raspberrypi.com,\n\ttfiga@google.com","Date":"Wed, 24 Nov 2021 13:19:49 +0000","Message-ID":"<163775998961.3059017.4992064762819465621@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v6 5/7] libcamera: Support passing\n\tColorSpaces to V4L2 subdevices","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\" "}}]