[{"id":20074,"web_url":"https://patchwork.libcamera.org/comment/20074/","msgid":"","date":"2021-10-05T22:47:21","subject":"Re: [libcamera-devel] [PATCH v2 2/3] libcamera: Support passing\n\tColorSpaces to V4L2 drivers","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi David,\n\n(CC'ing Hans)\n\nThank you for the patch.\n\nOn Mon, Sep 27, 2021 at 01:33:26PM +0100, David Plowman wrote:\n> The ColorSpace class is added to the StreamConfiguration, and is now\n> passed to V4L2 devices where it is handled appropriately. Note how\n> this means that the colour space is configured per-stream (though\n> platforms may restrict this).\n> \n> Signed-off-by: David Plowman \n> ---\n> include/libcamera/internal/v4l2_videodevice.h | 2 +\n> include/libcamera/stream.h | 3 +\n> src/libcamera/v4l2_videodevice.cpp | 119 ++++++++++++++++++\n\nI'd split this patch in two if you post a v3, as the change to\nStreamConfiguration is not directly related to the colour space support\nin V4L2VideoDevice.\n\n> 3 files changed, 124 insertions(+)\n> \n> diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h\n> index efe34d47..34cc9cdd 100644\n> --- a/include/libcamera/internal/v4l2_videodevice.h\n> +++ b/include/libcamera/internal/v4l2_videodevice.h\n> @@ -20,6 +20,7 @@\n> #include \n> #include \n> \n> +#include \n> #include \n> #include \n> #include \n> @@ -163,6 +164,7 @@ public:\n> \n> \tV4L2PixelFormat fourcc;\n> \tSize size;\n> +\tColorSpace colorSpace;\n\nWe'll need to same change in V4L2SubdeviceFormat, right ?\n\n> \n> \tstd::array planes;\n> \tunsigned int planesCount = 0;\n> diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h\n> index 0c55e716..131f7733 100644\n> --- a/include/libcamera/stream.h\n> +++ b/include/libcamera/stream.h\n> @@ -12,6 +12,7 @@\n> #include \n> #include \n> \n> +#include \n> #include \n> #include \n> #include \n> @@ -47,6 +48,8 @@ struct StreamConfiguration {\n> \n> \tunsigned int bufferCount;\n> \n> +\tColorSpace colorSpace;\n\nThis is missing documentation. It's probably the most crucial part as we\nneed to define the behaviour (such as how pipeline handlers should\nadjust unsupported values, but also how to handle inter-stream\nconstraints for instance).\n\n> +\n> \tStream *stream() const { return stream_; }\n> \tvoid setStream(Stream *stream) { stream_ = stream; }\n> \tconst StreamFormats &formats() const { return formats_; }\n> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> index ba5f88cd..3e6c8b87 100644\n> --- a/src/libcamera/v4l2_videodevice.cpp\n> +++ b/src/libcamera/v4l2_videodevice.cpp\n> @@ -369,6 +369,11 @@ bool V4L2BufferCache::Entry::operator==(const FrameBuffer &buffer) const\n> * \\brief The image size in pixels\n> */\n> \n> +/**\n> + * \\var V4L2DeviceFormat::colorSpace\n> + * \\brief The color space of the pixels\n\nMore documentation would be useful here too. In particular, the VIDEO\nencoding is not valid in V4L2DeviceFormat as it's meant to be handled by\npipeline handler.\n\n> + */\n> +\n> /**\n> * \\var V4L2DeviceFormat::fourcc\n> * \\brief The fourcc code describing the pixel encoding scheme\n\nWhile at it, could you move the documentation of fourcc before size ?\n\n> @@ -752,6 +757,114 @@ int V4L2VideoDevice::getFormat(V4L2DeviceFormat *format)\n> \t\treturn getFormatSingleplane(format);\n> }\n> \n> +static const std::vector> colorSpaceToV4l2 = {\n\nAs mentioned separately, I think you could also use a\nstd::unordered_map<> to have better lookup performance without having to\ndefine an order for colour spaces.\n\nOn the other hand, given that you also define maps in the other\ndirection, maybe we could just use vectors of pairs and always look up\nusing the vectors (possibly with a small helper function template to\nmake the syntax less cumbersome). O(n) may be more efficient than\naverage O(1) for small values of n, and there will then be less risks of\nthe two maps getting out of sync. Up to you.\n\n> +\t{ ColorSpace::RAW, V4L2_COLORSPACE_RAW },\n> +\t{ ColorSpace::JFIF, V4L2_COLORSPACE_JPEG },\n> +\t{ ColorSpace::SMPTE170M, V4L2_COLORSPACE_SMPTE170M },\n> +\t{ ColorSpace::REC709, V4L2_COLORSPACE_REC709 },\n> +\t{ ColorSpace::REC2020, V4L2_COLORSPACE_BT2020 },\n> +};\n> +\n> +static const std::map encodingToV4l2 = {\n> +\t{ ColorSpace::Encoding::REC601, V4L2_YCBCR_ENC_601 },\n> +\t{ ColorSpace::Encoding::REC709, V4L2_YCBCR_ENC_709 },\n> +\t{ ColorSpace::Encoding::REC2020, V4L2_YCBCR_ENC_BT2020 },\n> +};\n> +\n> +static const std::map transferFunctionToV4l2 = {\n> +\t{ ColorSpace::TransferFunction::IDENTITY, V4L2_XFER_FUNC_NONE },\n> +\t{ ColorSpace::TransferFunction::SRGB, V4L2_XFER_FUNC_SRGB },\n> +\t{ ColorSpace::TransferFunction::REC709, V4L2_XFER_FUNC_709 },\n> +};\n> +\n> +static const std::map rangeToV4l2 = {\n> +\t{ ColorSpace::Range::FULL, V4L2_QUANTIZATION_FULL_RANGE },\n> +\t{ ColorSpace::Range::LIMITED, V4L2_QUANTIZATION_LIM_RANGE },\n> +};\n> +\n> +static const std::map v4l2ToColorSpace = {\n> +\t{ V4L2_COLORSPACE_RAW, ColorSpace::RAW },\n> +\t{ V4L2_COLORSPACE_JPEG, ColorSpace::JFIF },\n> +\t{ V4L2_COLORSPACE_SRGB, ColorSpace::JFIF },\n> +\t{ V4L2_COLORSPACE_SMPTE170M, ColorSpace::SMPTE170M },\n> +\t{ V4L2_COLORSPACE_REC709, ColorSpace::REC709 },\n> +\t{ V4L2_COLORSPACE_BT2020, ColorSpace::REC2020 },\n> +};\n> +\n> +static const std::map v4l2ToEncoding = {\n> +\t{ V4L2_YCBCR_ENC_601, ColorSpace::Encoding::REC601 },\n> +\t{ V4L2_YCBCR_ENC_709, ColorSpace::Encoding::REC709 },\n> +\t{ V4L2_YCBCR_ENC_BT2020, ColorSpace::Encoding::REC2020 },\n> +};\n> +\n> +static const std::map v4l2ToTransferFunction = {\n> +\t{ V4L2_XFER_FUNC_NONE, ColorSpace::TransferFunction::IDENTITY },\n> +\t{ V4L2_XFER_FUNC_SRGB, ColorSpace::TransferFunction::SRGB },\n> +\t{ V4L2_XFER_FUNC_709, ColorSpace::TransferFunction::REC709 },\n> +};\n> +\n> +static const std::map v4l2ToRange = {\n> +\t{ V4L2_QUANTIZATION_FULL_RANGE, ColorSpace::Range::FULL },\n> +\t{ V4L2_QUANTIZATION_LIM_RANGE, ColorSpace::Range::LIMITED },\n> +};\n> +\n> +template\n> +static void setColorSpace(const ColorSpace &colorSpace, T &v4l2PixFormat)\n> +{\n> +\tif (!colorSpace.isFullyDefined())\n> +\t\tLOG(V4L2, Warning) << \"Setting non-fully defined colour space\"\n> +\t\t\t\t << colorSpace.toString();\n> +\n> +\tv4l2PixFormat.colorspace = V4L2_COLORSPACE_DEFAULT;\n> +\tv4l2PixFormat.ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;\n> +\tv4l2PixFormat.xfer_func = V4L2_XFER_FUNC_DEFAULT;\n> +\tv4l2PixFormat.quantization = V4L2_QUANTIZATION_DEFAULT;\n> +\n> +\tauto itColor = std::find_if(colorSpaceToV4l2.begin(), colorSpaceToV4l2.end(),\n> +\t\t\t\t [&colorSpace](const std::pair &item) { return colorSpace == item.first; });\n\nLet's shorten this a bit:\n\n\tauto itColor = std::find_if(colorSpaceToV4l2.begin(), colorSpaceToV4l2.end(),\n\t\t\t\t [&colorSpace](const std::pair &item) {\n\t\t\t\t\t return colorSpace == item.first;\n\t\t\t\t });\n\n> +\tif (itColor != colorSpaceToV4l2.end())\n> +\t\tv4l2PixFormat.colorspace = itColor->second;\n> +\n> +\tauto itEncoding = encodingToV4l2.find(colorSpace.encoding);\n> +\tif (itEncoding != encodingToV4l2.end())\n> +\t\tv4l2PixFormat.ycbcr_enc = itEncoding->second;\n> +\n> +\tauto itTransfer = transferFunctionToV4l2.find(colorSpace.transferFunction);\n> +\tif (itTransfer != transferFunctionToV4l2.end())\n> +\t\tv4l2PixFormat.xfer_func = itTransfer->second;\n> +\n> +\tauto itRange = rangeToV4l2.find(colorSpace.range);\n> +\tif (itRange != rangeToV4l2.end())\n> +\t\tv4l2PixFormat.quantization = itRange->second;\n> +}\n> +\n> +template\n> +static ColorSpace getColorSpace(const T &v4l2PixFormat)\n> +{\n> +\tColorSpace colorSpace;\n> +\n> +\tauto itColor = v4l2ToColorSpace.find(v4l2PixFormat.colorspace);\n> +\tif (itColor != v4l2ToColorSpace.end())\n> +\t\tcolorSpace = itColor->second;\n> +\n> +\tauto itEncoding = v4l2ToEncoding.find(v4l2PixFormat.ycbcr_enc);\n> +\tif (itEncoding != v4l2ToEncoding.end())\n> +\t\tcolorSpace.encoding = itEncoding->second;\n> +\n> +\tauto itTransfer = v4l2ToTransferFunction.find(v4l2PixFormat.xfer_func);\n> +\tif (itTransfer != v4l2ToTransferFunction.end())\n> +\t\tcolorSpace.transferFunction = itTransfer->second;\n> +\n> +\tauto itRange = v4l2ToRange.find(v4l2PixFormat.quantization);\n> +\tif (itRange != v4l2ToRange.end())\n> +\t\tcolorSpace.range = itRange->second;\n> +\n> +\tif (!colorSpace.isFullyDefined())\n> +\t\tLOG(V4L2, Warning) << \"Returning non-fully defined colour space\"\n> +\t\t\t\t << colorSpace.toString();\n> +\treturn colorSpace;\n> +}\n> +\n> /**\n> * \\brief Try an image format on the V4L2 video device\n> * \\param[inout] format The image format to test applicability to the video device\n> @@ -871,6 +984,7 @@ int V4L2VideoDevice::getFormatMultiplane(V4L2DeviceFormat *format)\n> \tformat->size.width = pix->width;\n> \tformat->size.height = pix->height;\n> \tformat->fourcc = V4L2PixelFormat(pix->pixelformat);\n> +\tformat->colorSpace = getColorSpace(*pix);\n> \tformat->planesCount = pix->num_planes;\n> \n> \tfor (unsigned int i = 0; i < format->planesCount; ++i) {\n> @@ -893,6 +1007,7 @@ int V4L2VideoDevice::trySetFormatMultiplane(V4L2DeviceFormat *format, bool set)\n> \tpix->pixelformat = format->fourcc;\n> \tpix->num_planes = format->planesCount;\n> \tpix->field = V4L2_FIELD_NONE;\n> +\tsetColorSpace(format->colorSpace, *pix);\n> \n> \tASSERT(pix->num_planes <= std::size(pix->plane_fmt));\n> \n> @@ -921,6 +1036,7 @@ int V4L2VideoDevice::trySetFormatMultiplane(V4L2DeviceFormat *format, bool set)\n> \t\tformat->planes[i].bpl = pix->plane_fmt[i].bytesperline;\n> \t\tformat->planes[i].size = pix->plane_fmt[i].sizeimage;\n> \t}\n> +\tformat->colorSpace = getColorSpace(*pix);\n> \n> \treturn 0;\n> }\n> @@ -941,6 +1057,7 @@ int V4L2VideoDevice::getFormatSingleplane(V4L2DeviceFormat *format)\n> \tformat->size.width = pix->width;\n> \tformat->size.height = pix->height;\n> \tformat->fourcc = V4L2PixelFormat(pix->pixelformat);\n> +\tformat->colorSpace = getColorSpace(*pix);\n> \tformat->planesCount = 1;\n> \tformat->planes[0].bpl = pix->bytesperline;\n> \tformat->planes[0].size = pix->sizeimage;\n> @@ -960,6 +1077,7 @@ int V4L2VideoDevice::trySetFormatSingleplane(V4L2DeviceFormat *format, bool set)\n> \tpix->pixelformat = format->fourcc;\n> \tpix->bytesperline = format->planes[0].bpl;\n> \tpix->field = V4L2_FIELD_NONE;\n> +\tsetColorSpace(format->colorSpace, *pix);\n> \tret = ioctl(set ? VIDIOC_S_FMT : VIDIOC_TRY_FMT, &v4l2Format);\n> \tif (ret) {\n> \t\tLOG(V4L2, Error)\n> @@ -978,6 +1096,7 @@ int V4L2VideoDevice::trySetFormatSingleplane(V4L2DeviceFormat *format, bool set)\n> \tformat->planesCount = 1;\n> \tformat->planes[0].bpl = pix->bytesperline;\n> \tformat->planes[0].size = pix->sizeimage;\n> +\tformat->colorSpace = getColorSpace(*pix);\n> \n> \treturn 0;\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 6A6E7C3243\n\tfor ;\n\tTue, 5 Oct 2021 22:47:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C4D33691B7;\n\tWed, 6 Oct 2021 00:47:32 +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 1E3056023F\n\tfor ;\n\tWed, 6 Oct 2021 00:47:29 +0200 (CEST)","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 8CAFF581;\n\tWed, 6 Oct 2021 00:47:28 +0200 (CEST)"],"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=\"hni5Sadu\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1633474048;\n\tbh=grNVCwS+eRYPhsx9REyQW0U4GJcNb6+ZVs1SHfyrdIw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=hni5SaduTjdk16v9JNE4z/nPEVJOZb9Gh7y/V1yglOiqcbqIBc68GcefvX5vO3m+J\n\tsF+qqAZHoPl0mtoBw2Ci9Q2yoZp1KQpth4p+LbkpwngWqJBH8kOufbI2nuK7EXSRvA\n\t3Rmo5EadEcKyGaXa9U/ioe2gRY0aLvWSKGaE9Ef4=","Date":"Wed, 6 Oct 2021 01:47:21 +0300","From":"Laurent Pinchart ","To":"David Plowman ","Message-ID":"","References":"<20210927123327.14554-1-david.plowman@raspberrypi.com>\n\t<20210927123327.14554-3-david.plowman@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210927123327.14554-3-david.plowman@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v2 2/3] libcamera: Support passing\n\tColorSpaces to V4L2 drivers","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":"Hans Verkuil ,\n\tlibcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]