[{"id":24765,"web_url":"https://patchwork.libcamera.org/comment/24765/","msgid":"","date":"2022-08-24T23:05:57","subject":"Re: [libcamera-devel] [PATCH v2 2/6] libcamera: v4l2_device: Adjust\n\tcolorspace if pixel format is RGB","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Umang,\n\nThank you for the patch.\n\nOn Wed, Aug 24, 2022 at 09:54:21PM +0530, Umang Jain via libcamera-devel wrote:\n> The V4L2_COLORSPACE_SRGB colorspace maps to ColorSpace::Srgb.\n> This is wrong as the V4L2_COLORSPACE_SRGB is ill-defined (defines\n> Rec 601 Y'CbCr encoding and limited range) in the kernel.\n> \n> The RGB pixel formats should not use any Y'CbCr encoding and is always\n> full range. Adjust the colorspace before reporting back to the\n> userspace in such a situation.\n> \n> Moving forwards, the ColorSpace::Srgb will be defined in the true sense\n> for RGB pixel formats.\n\nMentioning sRGB here is a bit confusing I think. Let's focus on what\nthis patch does (and this will also help making sure I understood this\ncorrectly):\n\nV4L2 has no \"none\" YCbCr encoding, and thus reports an encoding for all\nformats, including RGB and raw formats. This causes the libcamera\nColorSpace to report incorrect encodings for non-YUV formats. Fix it by\noverriding the encoding reported by the kernel to YCbCrEncoding::None\nfor non-YUV pixel formats and media bus formats.\n\nSimilarly, override the quantization range of non-YUV formats to full\nrange, as limited range isn't used for RGB and raw formats.\n\n> Signed-off-by: Umang Jain \n> ---\n> include/libcamera/internal/v4l2_device.h | 5 ++++-\n> src/libcamera/v4l2_device.cpp | 19 +++++++++++++++----\n> src/libcamera/v4l2_subdevice.cpp | 10 ++++++++--\n> src/libcamera/v4l2_videodevice.cpp | 12 ++++++++----\n> 4 files changed, 35 insertions(+), 11 deletions(-)\n> \n> diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h\n> index a52a5f2c..5ae2ef8a 100644\n> --- a/include/libcamera/internal/v4l2_device.h\n> +++ b/include/libcamera/internal/v4l2_device.h\n> @@ -22,6 +22,8 @@\n> #include \n> #include \n> \n> +#include \"libcamera/internal/formats.h\"\n> +\n> namespace libcamera {\n> \n> class EventNotifier;\n> @@ -59,7 +61,8 @@ protected:\n> \tint fd() const { return fd_.get(); }\n> \n> \ttemplate\n> -\tstatic std::optional toColorSpace(const T &v4l2Format);\n> +\tstatic std::optional toColorSpace(const T &v4l2Format,\n> +\t\t\t\t\t\t const PixelFormatInfo::ColourEncoding &colourEncoding);\n\n\t\t\t\t\t\t PixelFormatInfo::ColourEncoding colourEncoding);\nSame below.\n\n> \n> \ttemplate\n> \tstatic int fromColorSpace(const std::optional &colorSpace, T &v4l2Format);\n> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> index b22a981f..1fb08b9d 100644\n> --- a/src/libcamera/v4l2_device.cpp\n> +++ b/src/libcamera/v4l2_device.cpp\n> @@ -24,6 +24,7 @@\n> #include \n> #include \n> \n> +#include \"libcamera/internal/formats.h\"\n> #include \"libcamera/internal/sysfs.h\"\n> \n> /**\n> @@ -816,7 +817,8 @@ static const std::map rangeToV4l2 = {\n> * \\retval std::nullopt One or more V4L2 color space fields were not recognised\n> */\n> template\n> -std::optional V4L2Device::toColorSpace(const T &v4l2Format)\n> +std::optional V4L2Device::toColorSpace(const T &v4l2Format,\n> +\t\t\t\t\t\t const PixelFormatInfo::ColourEncoding &colourEncoding)\n> {\n> \tauto itColor = v4l2ToColorSpace.find(v4l2Format.colorspace);\n> \tif (itColor == v4l2ToColorSpace.end())\n> @@ -839,6 +841,9 @@ std::optional V4L2Device::toColorSpace(const T &v4l2Format)\n> \t\t\treturn std::nullopt;\n> \n> \t\tcolorSpace.ycbcrEncoding = itYcbcrEncoding->second;\n> +\n\nI'd add a comment here, or we'll forget why.\n\n\t\t/*\n\t\t * V4L2 has no \"none\" encoding, override the value returned by\n\t\t * the kernel for non-YUV formats as YCbCr encoding isn't\n\t\t * applicable in that case..\n\t\t */\n\n> +\t\tif (colourEncoding == PixelFormatInfo::ColourEncodingRGB)\n\nShouldn't this be\n\n\t\tif (colourEncoding != PixelFormatInfo::ColourEncodingYUV)\n\n? Raw formats have no YCbCr encoding either. The subject line would need\nto be updated accordingly, I'd write \"Adjust colorspace based on pixel\nformat\".\n\n> +\t\t\tcolorSpace.ycbcrEncoding = ColorSpace::YcbcrEncoding::None;\n> \t}\n> \n> \tif (v4l2Format.quantization != V4L2_QUANTIZATION_DEFAULT) {\n> @@ -847,14 +852,20 @@ std::optional V4L2Device::toColorSpace(const T &v4l2Format)\n> \t\t\treturn std::nullopt;\n> \n> \t\tcolorSpace.range = itRange->second;\n> +\n> +\t\tif (colourEncoding == PixelFormatInfo::ColourEncodingRGB)\n\nSame here.\n\n> +\t\t\tcolorSpace.range = ColorSpace::Range::Full;\n> \t}\n> \n> \treturn colorSpace;\n> }\n> \n> -template std::optional V4L2Device::toColorSpace(const struct v4l2_pix_format &);\n> -template std::optional V4L2Device::toColorSpace(const struct v4l2_pix_format_mplane &);\n> -template std::optional V4L2Device::toColorSpace(const struct v4l2_mbus_framefmt &);\n> +template std::optional V4L2Device::toColorSpace(const struct v4l2_pix_format &,\n> +\t\t\t\t\t\t\t const PixelFormatInfo::ColourEncoding &);\n> +template std::optional V4L2Device::toColorSpace(const struct v4l2_pix_format_mplane &,\n> +\t\t\t\t\t\t\t const PixelFormatInfo::ColourEncoding &);\n> +template std::optional V4L2Device::toColorSpace(const struct v4l2_mbus_framefmt &,\n> +\t\t\t\t\t\t\t const PixelFormatInfo::ColourEncoding &);\n> \n> /**\n> * \\brief Fill in the color space fields of a V4L2 format from a ColorSpace\n> diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp\n> index ab74b9f0..a52c414f 100644\n> --- a/src/libcamera/v4l2_subdevice.cpp\n> +++ b/src/libcamera/v4l2_subdevice.cpp\n> @@ -502,7 +502,10 @@ int V4L2Subdevice::getFormat(unsigned int pad, V4L2SubdeviceFormat *format,\n> \tformat->size.width = subdevFmt.format.width;\n> \tformat->size.height = subdevFmt.format.height;\n> \tformat->mbus_code = subdevFmt.format.code;\n> -\tformat->colorSpace = toColorSpace(subdevFmt.format);\n> +\tauto iter = formatInfoMap.find(format->mbus_code);\n> +\tif (iter == formatInfoMap.end())\n> +\t\treturn -EINVAL;\n\nThat seems a bit harsh. I'm thinking about the simple pipeline handler\nin particular, which will propagate media bus formats through the\npipeline without caring much about the format itself. Would it better to\ninstead pick a default ?\n\n> +\tformat->colorSpace = toColorSpace(subdevFmt.format, iter->second.colourEncoding);\n> \n> \treturn 0;\n> }\n> @@ -548,7 +551,10 @@ int V4L2Subdevice::setFormat(unsigned int pad, V4L2SubdeviceFormat *format,\n> \tformat->size.width = subdevFmt.format.width;\n> \tformat->size.height = subdevFmt.format.height;\n> \tformat->mbus_code = subdevFmt.format.code;\n> -\tformat->colorSpace = toColorSpace(subdevFmt.format);\n> +\tauto iter = formatInfoMap.find(format->mbus_code);\n> +\tif (iter == formatInfoMap.end())\n> +\t\treturn -EINVAL;\n\nSame here.\n\n> +\tformat->colorSpace = toColorSpace(subdevFmt.format, iter->second.colourEncoding);\n> \n> \treturn 0;\n> }\n> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> index 5a2d0e5b..0e3f5436 100644\n> --- a/src/libcamera/v4l2_videodevice.cpp\n> +++ b/src/libcamera/v4l2_videodevice.cpp\n> @@ -931,7 +931,8 @@ int V4L2VideoDevice::getFormatMultiplane(V4L2DeviceFormat *format)\n\nI'm tempted to add a helper just above this function:\n\nnamespace {\n\nstd::optional toColorSpace(const struct v4l2_pix_format_mplane &pix)\n{\n\tV4L2PixelFormat fourcc{ pix.pixelformat };\n\treturn toColorSpace(pix, PixelFormatInfo::info(fourcc).colourEncoding);\n}\n\n} /* namespace */\n\nto make the color below a bit nicer (it would actually not need to be\nchanged at all). Up to you.\n\n> \tformat->size.height = pix->height;\n> \tformat->fourcc = V4L2PixelFormat(pix->pixelformat);\n> \tformat->planesCount = pix->num_planes;\n> -\tformat->colorSpace = toColorSpace(*pix);\n> +\tformat->colorSpace =\n> +\t\ttoColorSpace(*pix, PixelFormatInfo::info(format->fourcc).colourEncoding);\n> \n> \tfor (unsigned int i = 0; i < format->planesCount; ++i) {\n> \t\tformat->planes[i].bpl = pix->plane_fmt[i].bytesperline;\n> @@ -987,7 +988,8 @@ 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 = toColorSpace(*pix);\n> +\tformat->colorSpace =\n> +\t\ttoColorSpace(*pix, PixelFormatInfo::info(format->fourcc).colourEncoding);\n> \n> \treturn 0;\n> }\n> @@ -1011,7 +1013,8 @@ int V4L2VideoDevice::getFormatSingleplane(V4L2DeviceFormat *format)\n> \tformat->planesCount = 1;\n> \tformat->planes[0].bpl = pix->bytesperline;\n> \tformat->planes[0].size = pix->sizeimage;\n> -\tformat->colorSpace = toColorSpace(*pix);\n> +\tformat->colorSpace =\n> +\t\ttoColorSpace(*pix, PixelFormatInfo::info(format->fourcc).colourEncoding);\n> \n> \treturn 0;\n> }\n> @@ -1053,7 +1056,8 @@ 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 = toColorSpace(*pix);\n> +\tformat->colorSpace =\n> +\t\ttoColorSpace(*pix, PixelFormatInfo::info(format->fourcc).colourEncoding);\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 82DFAC0DA4\n\tfor ;\n\tWed, 24 Aug 2022 23:06:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CB8F161F9F;\n\tThu, 25 Aug 2022 01:06:05 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 30C1E61F9F\n\tfor ;\n\tThu, 25 Aug 2022 01:06:04 +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 8A2662B3;\n\tThu, 25 Aug 2022 01:06:03 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1661382365;\n\tbh=J0NarLVjIb21MrwHuwcJPJW7mqCX2VfOyTiFV4Z7ZFQ=;\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=j58bR5s4BEVfUeU1m+BWrQa3w0T/6Hy84XOBtYmzyVOzhQMJ/mKnowt0AeLZkN7UI\n\tBMnYRO8+tErVL9n11wsIfxWsNicGG5653BQcyrXcF0SXU79OdxGhguGeqSWN+3cwKc\n\t0oQL6TcVxaowb8LKT0HpSfjSMFdU2OpDlXgCHY8IUs51PEFWvRj8GbVArRwfg6CRYO\n\tpFlHaTDjZG6x/OkC+V/tQZpQr8dnE9fx3KhraXXyyU1/EEKM8/k7jHLGrRb4Aq7hJ4\n\tASkYDlAKHdl4XOVm0cYZNtMLmsurcZjnwt4hY3W5EewPXellsm42hQInGfWFHdrkT/\n\tP4MSLPFqzT6+A==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1661382363;\n\tbh=J0NarLVjIb21MrwHuwcJPJW7mqCX2VfOyTiFV4Z7ZFQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=KfUuin73svoB7Pt5F0QWBMeDTuT7rP1p4ddYcnTk7zwGVlH5PtYBnwLLwBzCMZuxG\n\txcKmvkTfa1z1ueKYcsJ/Rq3kTqkrHeNIxegXe/eKFg6h0bdBx6Sq9ptidN+Je35Bta\n\t5LYZGIfDrpCmJs8FwLEaW6ZOsBcrTAujr1mVt/UM="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"KfUuin73\"; dkim-atps=neutral","Date":"Thu, 25 Aug 2022 02:05:57 +0300","To":"Umang Jain ","Message-ID":"","References":"<20220824162425.71087-1-umang.jain@ideasonboard.com>\n\t<20220824162425.71087-3-umang.jain@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220824162425.71087-3-umang.jain@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 2/6] libcamera: v4l2_device: Adjust\n\tcolorspace if pixel format is RGB","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24771,"web_url":"https://patchwork.libcamera.org/comment/24771/","msgid":"","date":"2022-08-25T15:09:43","subject":"Re: [libcamera-devel] [PATCH v2 2/6] libcamera: v4l2_device: Adjust\n\tcolorspace if pixel format is RGB","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Another comment.\n\nOn Thu, Aug 25, 2022 at 02:05:57AM +0300, Laurent Pinchart via libcamera-devel wrote:\n> Hi Umang,\n> \n> Thank you for the patch.\n> \n> On Wed, Aug 24, 2022 at 09:54:21PM +0530, Umang Jain via libcamera-devel wrote:\n> > The V4L2_COLORSPACE_SRGB colorspace maps to ColorSpace::Srgb.\n> > This is wrong as the V4L2_COLORSPACE_SRGB is ill-defined (defines\n> > Rec 601 Y'CbCr encoding and limited range) in the kernel.\n> > \n> > The RGB pixel formats should not use any Y'CbCr encoding and is always\n> > full range. Adjust the colorspace before reporting back to the\n> > userspace in such a situation.\n> > \n> > Moving forwards, the ColorSpace::Srgb will be defined in the true sense\n> > for RGB pixel formats.\n> \n> Mentioning sRGB here is a bit confusing I think. Let's focus on what\n> this patch does (and this will also help making sure I understood this\n> correctly):\n> \n> V4L2 has no \"none\" YCbCr encoding, and thus reports an encoding for all\n> formats, including RGB and raw formats. This causes the libcamera\n> ColorSpace to report incorrect encodings for non-YUV formats. Fix it by\n> overriding the encoding reported by the kernel to YCbCrEncoding::None\n> for non-YUV pixel formats and media bus formats.\n> \n> Similarly, override the quantization range of non-YUV formats to full\n> range, as limited range isn't used for RGB and raw formats.\n> \n> > Signed-off-by: Umang Jain \n> > ---\n> > include/libcamera/internal/v4l2_device.h | 5 ++++-\n> > src/libcamera/v4l2_device.cpp | 19 +++++++++++++++----\n> > src/libcamera/v4l2_subdevice.cpp | 10 ++++++++--\n> > src/libcamera/v4l2_videodevice.cpp | 12 ++++++++----\n> > 4 files changed, 35 insertions(+), 11 deletions(-)\n> > \n> > diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h\n> > index a52a5f2c..5ae2ef8a 100644\n> > --- a/include/libcamera/internal/v4l2_device.h\n> > +++ b/include/libcamera/internal/v4l2_device.h\n> > @@ -22,6 +22,8 @@\n> > #include \n> > #include \n> > \n> > +#include \"libcamera/internal/formats.h\"\n> > +\n> > namespace libcamera {\n> > \n> > class EventNotifier;\n> > @@ -59,7 +61,8 @@ protected:\n> > \tint fd() const { return fd_.get(); }\n> > \n> > \ttemplate\n> > -\tstatic std::optional toColorSpace(const T &v4l2Format);\n> > +\tstatic std::optional toColorSpace(const T &v4l2Format,\n> > +\t\t\t\t\t\t const PixelFormatInfo::ColourEncoding &colourEncoding);\n> \n> \t\t\t\t\t\t PixelFormatInfo::ColourEncoding colourEncoding);\n> Same below.\n> \n> > \n> > \ttemplate\n> > \tstatic int fromColorSpace(const std::optional &colorSpace, T &v4l2Format);\n> > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> > index b22a981f..1fb08b9d 100644\n> > --- a/src/libcamera/v4l2_device.cpp\n> > +++ b/src/libcamera/v4l2_device.cpp\n> > @@ -24,6 +24,7 @@\n> > #include \n> > #include \n> > \n> > +#include \"libcamera/internal/formats.h\"\n> > #include \"libcamera/internal/sysfs.h\"\n> > \n> > /**\n> > @@ -816,7 +817,8 @@ static const std::map rangeToV4l2 = {\n> > * \\retval std::nullopt One or more V4L2 color space fields were not recognised\n> > */\n\nThe documentation needs to be updated with the new parameter.\n\n> > template\n> > -std::optional V4L2Device::toColorSpace(const T &v4l2Format)\n> > +std::optional V4L2Device::toColorSpace(const T &v4l2Format,\n> > +\t\t\t\t\t\t const PixelFormatInfo::ColourEncoding &colourEncoding)\n> > {\n> > \tauto itColor = v4l2ToColorSpace.find(v4l2Format.colorspace);\n> > \tif (itColor == v4l2ToColorSpace.end())\n> > @@ -839,6 +841,9 @@ std::optional V4L2Device::toColorSpace(const T &v4l2Format)\n> > \t\t\treturn std::nullopt;\n> > \n> > \t\tcolorSpace.ycbcrEncoding = itYcbcrEncoding->second;\n> > +\n> \n> I'd add a comment here, or we'll forget why.\n> \n> \t\t/*\n> \t\t * V4L2 has no \"none\" encoding, override the value returned by\n> \t\t * the kernel for non-YUV formats as YCbCr encoding isn't\n> \t\t * applicable in that case..\n> \t\t */\n> \n> > +\t\tif (colourEncoding == PixelFormatInfo::ColourEncodingRGB)\n> \n> Shouldn't this be\n> \n> \t\tif (colourEncoding != PixelFormatInfo::ColourEncodingYUV)\n> \n> ? Raw formats have no YCbCr encoding either. The subject line would need\n> to be updated accordingly, I'd write \"Adjust colorspace based on pixel\n> format\".\n> \n> > +\t\t\tcolorSpace.ycbcrEncoding = ColorSpace::YcbcrEncoding::None;\n> > \t}\n> > \n> > \tif (v4l2Format.quantization != V4L2_QUANTIZATION_DEFAULT) {\n> > @@ -847,14 +852,20 @@ std::optional V4L2Device::toColorSpace(const T &v4l2Format)\n> > \t\t\treturn std::nullopt;\n> > \n> > \t\tcolorSpace.range = itRange->second;\n> > +\n> > +\t\tif (colourEncoding == PixelFormatInfo::ColourEncodingRGB)\n> \n> Same here.\n> \n> > +\t\t\tcolorSpace.range = ColorSpace::Range::Full;\n> > \t}\n> > \n> > \treturn colorSpace;\n> > }\n> > \n> > -template std::optional V4L2Device::toColorSpace(const struct v4l2_pix_format &);\n> > -template std::optional V4L2Device::toColorSpace(const struct v4l2_pix_format_mplane &);\n> > -template std::optional V4L2Device::toColorSpace(const struct v4l2_mbus_framefmt &);\n> > +template std::optional V4L2Device::toColorSpace(const struct v4l2_pix_format &,\n> > +\t\t\t\t\t\t\t const PixelFormatInfo::ColourEncoding &);\n> > +template std::optional V4L2Device::toColorSpace(const struct v4l2_pix_format_mplane &,\n> > +\t\t\t\t\t\t\t const PixelFormatInfo::ColourEncoding &);\n> > +template std::optional V4L2Device::toColorSpace(const struct v4l2_mbus_framefmt &,\n> > +\t\t\t\t\t\t\t const PixelFormatInfo::ColourEncoding &);\n> > \n> > /**\n> > * \\brief Fill in the color space fields of a V4L2 format from a ColorSpace\n> > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp\n> > index ab74b9f0..a52c414f 100644\n> > --- a/src/libcamera/v4l2_subdevice.cpp\n> > +++ b/src/libcamera/v4l2_subdevice.cpp\n> > @@ -502,7 +502,10 @@ int V4L2Subdevice::getFormat(unsigned int pad, V4L2SubdeviceFormat *format,\n> > \tformat->size.width = subdevFmt.format.width;\n> > \tformat->size.height = subdevFmt.format.height;\n> > \tformat->mbus_code = subdevFmt.format.code;\n> > -\tformat->colorSpace = toColorSpace(subdevFmt.format);\n> > +\tauto iter = formatInfoMap.find(format->mbus_code);\n> > +\tif (iter == formatInfoMap.end())\n> > +\t\treturn -EINVAL;\n> \n> That seems a bit harsh. I'm thinking about the simple pipeline handler\n> in particular, which will propagate media bus formats through the\n> pipeline without caring much about the format itself. Would it better to\n> instead pick a default ?\n> \n> > +\tformat->colorSpace = toColorSpace(subdevFmt.format, iter->second.colourEncoding);\n> > \n> > \treturn 0;\n> > }\n> > @@ -548,7 +551,10 @@ int V4L2Subdevice::setFormat(unsigned int pad, V4L2SubdeviceFormat *format,\n> > \tformat->size.width = subdevFmt.format.width;\n> > \tformat->size.height = subdevFmt.format.height;\n> > \tformat->mbus_code = subdevFmt.format.code;\n> > -\tformat->colorSpace = toColorSpace(subdevFmt.format);\n> > +\tauto iter = formatInfoMap.find(format->mbus_code);\n> > +\tif (iter == formatInfoMap.end())\n> > +\t\treturn -EINVAL;\n> \n> Same here.\n> \n> > +\tformat->colorSpace = toColorSpace(subdevFmt.format, iter->second.colourEncoding);\n> > \n> > \treturn 0;\n> > }\n> > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> > index 5a2d0e5b..0e3f5436 100644\n> > --- a/src/libcamera/v4l2_videodevice.cpp\n> > +++ b/src/libcamera/v4l2_videodevice.cpp\n> > @@ -931,7 +931,8 @@ int V4L2VideoDevice::getFormatMultiplane(V4L2DeviceFormat *format)\n> \n> I'm tempted to add a helper just above this function:\n> \n> namespace {\n> \n> std::optional toColorSpace(const struct v4l2_pix_format_mplane &pix)\n> {\n> \tV4L2PixelFormat fourcc{ pix.pixelformat };\n> \treturn toColorSpace(pix, PixelFormatInfo::info(fourcc).colourEncoding);\n> }\n> \n> } /* namespace */\n> \n> to make the color below a bit nicer (it would actually not need to be\n> changed at all). Up to you.\n> \n> > \tformat->size.height = pix->height;\n> > \tformat->fourcc = V4L2PixelFormat(pix->pixelformat);\n> > \tformat->planesCount = pix->num_planes;\n> > -\tformat->colorSpace = toColorSpace(*pix);\n> > +\tformat->colorSpace =\n> > +\t\ttoColorSpace(*pix, PixelFormatInfo::info(format->fourcc).colourEncoding);\n> > \n> > \tfor (unsigned int i = 0; i < format->planesCount; ++i) {\n> > \t\tformat->planes[i].bpl = pix->plane_fmt[i].bytesperline;\n> > @@ -987,7 +988,8 @@ 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 = toColorSpace(*pix);\n> > +\tformat->colorSpace =\n> > +\t\ttoColorSpace(*pix, PixelFormatInfo::info(format->fourcc).colourEncoding);\n> > \n> > \treturn 0;\n> > }\n> > @@ -1011,7 +1013,8 @@ int V4L2VideoDevice::getFormatSingleplane(V4L2DeviceFormat *format)\n> > \tformat->planesCount = 1;\n> > \tformat->planes[0].bpl = pix->bytesperline;\n> > \tformat->planes[0].size = pix->sizeimage;\n> > -\tformat->colorSpace = toColorSpace(*pix);\n> > +\tformat->colorSpace =\n> > +\t\ttoColorSpace(*pix, PixelFormatInfo::info(format->fourcc).colourEncoding);\n> > \n> > \treturn 0;\n> > }\n> > @@ -1053,7 +1056,8 @@ 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 = toColorSpace(*pix);\n> > +\tformat->colorSpace =\n> > +\t\ttoColorSpace(*pix, PixelFormatInfo::info(format->fourcc).colourEncoding);\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 35F65C0DA4\n\tfor ;\n\tThu, 25 Aug 2022 15:09:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8228F61FBF;\n\tThu, 25 Aug 2022 17:09:52 +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 0106161FA0\n\tfor ;\n\tThu, 25 Aug 2022 17:09:50 +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 50E602B3;\n\tThu, 25 Aug 2022 17:09:50 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1661440192;\n\tbh=ceqQhfvwIKVAXPfBEf26+dCZDnA4ezeEPTqns6RQViA=;\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:\n\tFrom;\n\tb=mZOABtoEajbZQ6Ikrzsl6y1g6bGXRipxuYp4YvKgXptQawUFCkVotXI5Mdl0zWoHL\n\tyuNfKiVOhT3SIARfHPj18zb3r6NBXtC1QwnFHoQjLtQ9CVmX7ffYkKqRnMqPZBur2t\n\ttNSEVZtO+4gVYmB4ZfYsetATk51NALc3GuzmJqx1Q3Fzu7yNl1BjWlEzKqhj+qc7ZK\n\tpsgd5ZUcz10eZBHXs8OMtAN1wuZLjOu6ngzAZdyAgwbOylx4a5yn9B+so6dIfIU1Sf\n\tMA8xydWMyQ2oYbR/GfYp3/PdaQm2Pi8Imr8qTkzizNGNS7Zs+qjrh/diBJxk6+KTC/\n\tYcScynmniSlBA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1661440190;\n\tbh=ceqQhfvwIKVAXPfBEf26+dCZDnA4ezeEPTqns6RQViA=;\n\th=Date:From:To:Subject:References:In-Reply-To:From;\n\tb=czFbjYMnIc5u/WWfbemgQS550M2Hs82AimRQ+oyA7A2i1Cvsv73mWD+fUNtS31SQY\n\trF5OLQFDGxG7eV51iDU8lWUeqNka2ny2l1aXTil4JB2EPXm9ioH1PbRsosXCUzrQ66\n\tV2LvJkNZS4xlwP3JpHWrwunHtwiZVOQAUFJxf0m0="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"czFbjYMn\"; dkim-atps=neutral","Date":"Thu, 25 Aug 2022 18:09:43 +0300","To":"Umang Jain ,\n\tlibcamera-devel ","Message-ID":"","References":"<20220824162425.71087-1-umang.jain@ideasonboard.com>\n\t<20220824162425.71087-3-umang.jain@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2 2/6] libcamera: v4l2_device: Adjust\n\tcolorspace if pixel format is RGB","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24787,"web_url":"https://patchwork.libcamera.org/comment/24787/","msgid":"","date":"2022-08-26T06:16:58","subject":"Re: [libcamera-devel] [PATCH v2 2/6] libcamera: v4l2_device: Adjust\n\tcolorspace if pixel format is RGB","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Laurent,\n\nThank you for the review.\n\nOn 8/25/22 4:35 AM, Laurent Pinchart wrote:\n> Hi Umang,\n>\n> Thank you for the patch.\n>\n> On Wed, Aug 24, 2022 at 09:54:21PM +0530, Umang Jain via libcamera-devel wrote:\n>> The V4L2_COLORSPACE_SRGB colorspace maps to ColorSpace::Srgb.\n>> This is wrong as the V4L2_COLORSPACE_SRGB is ill-defined (defines\n>> Rec 601 Y'CbCr encoding and limited range) in the kernel.\n>>\n>> The RGB pixel formats should not use any Y'CbCr encoding and is always\n>> full range. Adjust the colorspace before reporting back to the\n>> userspace in such a situation.\n>>\n>> Moving forwards, the ColorSpace::Srgb will be defined in the true sense\n>> for RGB pixel formats.\n> Mentioning sRGB here is a bit confusing I think. Let's focus on what\n> this patch does (and this will also help making sure I understood this\n> correctly):\n>\n> V4L2 has no \"none\" YCbCr encoding, and thus reports an encoding for all\n> formats, including RGB and raw formats. This causes the libcamera\n> ColorSpace to report incorrect encodings for non-YUV formats. Fix it by\n> overriding the encoding reported by the kernel to YCbCrEncoding::None\n> for non-YUV pixel formats and media bus formats.\n\nCorrect.\n>\n> Similarly, override the quantization range of non-YUV formats to full\n> range, as limited range isn't used for RGB and raw formats.\n\nSeems correct too, I agree my commit message was not at it's best.\n>\n>> Signed-off-by: Umang Jain \n>> ---\n>> include/libcamera/internal/v4l2_device.h | 5 ++++-\n>> src/libcamera/v4l2_device.cpp | 19 +++++++++++++++----\n>> src/libcamera/v4l2_subdevice.cpp | 10 ++++++++--\n>> src/libcamera/v4l2_videodevice.cpp | 12 ++++++++----\n>> 4 files changed, 35 insertions(+), 11 deletions(-)\n>>\n>> diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h\n>> index a52a5f2c..5ae2ef8a 100644\n>> --- a/include/libcamera/internal/v4l2_device.h\n>> +++ b/include/libcamera/internal/v4l2_device.h\n>> @@ -22,6 +22,8 @@\n>> #include \n>> #include \n>> \n>> +#include \"libcamera/internal/formats.h\"\n>> +\n>> namespace libcamera {\n>> \n>> class EventNotifier;\n>> @@ -59,7 +61,8 @@ protected:\n>> \tint fd() const { return fd_.get(); }\n>> \n>> \ttemplate\n>> -\tstatic std::optional toColorSpace(const T &v4l2Format);\n>> +\tstatic std::optional toColorSpace(const T &v4l2Format,\n>> +\t\t\t\t\t\t const PixelFormatInfo::ColourEncoding &colourEncoding);\n> \t\t\t\t\t\t PixelFormatInfo::ColourEncoding colourEncoding);\n> Same below.\n>\n>> \n>> \ttemplate\n>> \tstatic int fromColorSpace(const std::optional &colorSpace, T &v4l2Format);\n>> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n>> index b22a981f..1fb08b9d 100644\n>> --- a/src/libcamera/v4l2_device.cpp\n>> +++ b/src/libcamera/v4l2_device.cpp\n>> @@ -24,6 +24,7 @@\n>> #include \n>> #include \n>> \n>> +#include \"libcamera/internal/formats.h\"\n>> #include \"libcamera/internal/sysfs.h\"\n>> \n>> /**\n>> @@ -816,7 +817,8 @@ static const std::map rangeToV4l2 = {\n>> * \\retval std::nullopt One or more V4L2 color space fields were not recognised\n>> */\n>> template\n>> -std::optional V4L2Device::toColorSpace(const T &v4l2Format)\n>> +std::optional V4L2Device::toColorSpace(const T &v4l2Format,\n>> +\t\t\t\t\t\t const PixelFormatInfo::ColourEncoding &colourEncoding)\n>> {\n>> \tauto itColor = v4l2ToColorSpace.find(v4l2Format.colorspace);\n>> \tif (itColor == v4l2ToColorSpace.end())\n>> @@ -839,6 +841,9 @@ std::optional V4L2Device::toColorSpace(const T &v4l2Format)\n>> \t\t\treturn std::nullopt;\n>> \n>> \t\tcolorSpace.ycbcrEncoding = itYcbcrEncoding->second;\n>> +\n> I'd add a comment here, or we'll forget why.\n>\n> \t\t/*\n> \t\t * V4L2 has no \"none\" encoding, override the value returned by\n> \t\t * the kernel for non-YUV formats as YCbCr encoding isn't\n> \t\t * applicable in that case..\n> \t\t */\n>\n>> +\t\tif (colourEncoding == PixelFormatInfo::ColourEncodingRGB)\n> Shouldn't this be\n>\n> \t\tif (colourEncoding != PixelFormatInfo::ColourEncodingYUV)\n>\n> ? Raw formats have no YCbCr encoding either. The subject line would need\n> to be updated accordingly, I'd write \"Adjust colorspace based on pixel\n> format\".\n>\n>> +\t\t\tcolorSpace.ycbcrEncoding = ColorSpace::YcbcrEncoding::None;\n>> \t}\n>> \n>> \tif (v4l2Format.quantization != V4L2_QUANTIZATION_DEFAULT) {\n>> @@ -847,14 +852,20 @@ std::optional V4L2Device::toColorSpace(const T &v4l2Format)\n>> \t\t\treturn std::nullopt;\n>> \n>> \t\tcolorSpace.range = itRange->second;\n>> +\n>> +\t\tif (colourEncoding == PixelFormatInfo::ColourEncodingRGB)\n> Same here.\n>\n>> +\t\t\tcolorSpace.range = ColorSpace::Range::Full;\n>> \t}\n>> \n>> \treturn colorSpace;\n>> }\n>> \n>> -template std::optional V4L2Device::toColorSpace(const struct v4l2_pix_format &);\n>> -template std::optional V4L2Device::toColorSpace(const struct v4l2_pix_format_mplane &);\n>> -template std::optional V4L2Device::toColorSpace(const struct v4l2_mbus_framefmt &);\n>> +template std::optional V4L2Device::toColorSpace(const struct v4l2_pix_format &,\n>> +\t\t\t\t\t\t\t const PixelFormatInfo::ColourEncoding &);\n>> +template std::optional V4L2Device::toColorSpace(const struct v4l2_pix_format_mplane &,\n>> +\t\t\t\t\t\t\t const PixelFormatInfo::ColourEncoding &);\n>> +template std::optional V4L2Device::toColorSpace(const struct v4l2_mbus_framefmt &,\n>> +\t\t\t\t\t\t\t const PixelFormatInfo::ColourEncoding &);\n>> \n>> /**\n>> * \\brief Fill in the color space fields of a V4L2 format from a ColorSpace\n>> diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp\n>> index ab74b9f0..a52c414f 100644\n>> --- a/src/libcamera/v4l2_subdevice.cpp\n>> +++ b/src/libcamera/v4l2_subdevice.cpp\n>> @@ -502,7 +502,10 @@ int V4L2Subdevice::getFormat(unsigned int pad, V4L2SubdeviceFormat *format,\n>> \tformat->size.width = subdevFmt.format.width;\n>> \tformat->size.height = subdevFmt.format.height;\n>> \tformat->mbus_code = subdevFmt.format.code;\n>> -\tformat->colorSpace = toColorSpace(subdevFmt.format);\n>> +\tauto iter = formatInfoMap.find(format->mbus_code);\n>> +\tif (iter == formatInfoMap.end())\n>> +\t\treturn -EINVAL;\n> That seems a bit harsh. I'm thinking about the simple pipeline handler\n> in particular, which will propagate media bus formats through the\n> pipeline without caring much about the format itself. Would it better to\n> instead pick a default ?\n\nI wasn't aware of that. I assumed (formatInfoMap.find(format->mbus_code) \n== formatInfoMap.end()) would be very hard to trigger and if it does at \nall, we want to fail loudly.\n\nI am not much aware of simple pipeline handler format's passing - I'll \ntake a look to answer the question - \"Which default to pick\"\n>> +\tformat->colorSpace = toColorSpace(subdevFmt.format, iter->second.colourEncoding);\n>> \n>> \treturn 0;\n>> }\n>> @@ -548,7 +551,10 @@ int V4L2Subdevice::setFormat(unsigned int pad, V4L2SubdeviceFormat *format,\n>> \tformat->size.width = subdevFmt.format.width;\n>> \tformat->size.height = subdevFmt.format.height;\n>> \tformat->mbus_code = subdevFmt.format.code;\n>> -\tformat->colorSpace = toColorSpace(subdevFmt.format);\n>> +\tauto iter = formatInfoMap.find(format->mbus_code);\n>> +\tif (iter == formatInfoMap.end())\n>> +\t\treturn -EINVAL;\n> Same here.\n>\n>> +\tformat->colorSpace = toColorSpace(subdevFmt.format, iter->second.colourEncoding);\n>> \n>> \treturn 0;\n>> }\n>> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n>> index 5a2d0e5b..0e3f5436 100644\n>> --- a/src/libcamera/v4l2_videodevice.cpp\n>> +++ b/src/libcamera/v4l2_videodevice.cpp\n>> @@ -931,7 +931,8 @@ int V4L2VideoDevice::getFormatMultiplane(V4L2DeviceFormat *format)\n> I'm tempted to add a helper just above this function:\n>\n> namespace {\n>\n> std::optional toColorSpace(const struct v4l2_pix_format_mplane &pix)\n> {\n> \tV4L2PixelFormat fourcc{ pix.pixelformat };\n> \treturn toColorSpace(pix, PixelFormatInfo::info(fourcc).colourEncoding);\n> }\n>\n> } /* namespace */\n>\n> to make the color below a bit nicer (it would actually not need to be\n> changed at all). Up to you.\n\nAck\n>\n>> \tformat->size.height = pix->height;\n>> \tformat->fourcc = V4L2PixelFormat(pix->pixelformat);\n>> \tformat->planesCount = pix->num_planes;\n>> -\tformat->colorSpace = toColorSpace(*pix);\n>> +\tformat->colorSpace =\n>> +\t\ttoColorSpace(*pix, PixelFormatInfo::info(format->fourcc).colourEncoding);\n>> \n>> \tfor (unsigned int i = 0; i < format->planesCount; ++i) {\n>> \t\tformat->planes[i].bpl = pix->plane_fmt[i].bytesperline;\n>> @@ -987,7 +988,8 @@ 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 = toColorSpace(*pix);\n>> +\tformat->colorSpace =\n>> +\t\ttoColorSpace(*pix, PixelFormatInfo::info(format->fourcc).colourEncoding);\n>> \n>> \treturn 0;\n>> }\n>> @@ -1011,7 +1013,8 @@ int V4L2VideoDevice::getFormatSingleplane(V4L2DeviceFormat *format)\n>> \tformat->planesCount = 1;\n>> \tformat->planes[0].bpl = pix->bytesperline;\n>> \tformat->planes[0].size = pix->sizeimage;\n>> -\tformat->colorSpace = toColorSpace(*pix);\n>> +\tformat->colorSpace =\n>> +\t\ttoColorSpace(*pix, PixelFormatInfo::info(format->fourcc).colourEncoding);\n>> \n>> \treturn 0;\n>> }\n>> @@ -1053,7 +1056,8 @@ 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 = toColorSpace(*pix);\n>> +\tformat->colorSpace =\n>> +\t\ttoColorSpace(*pix, PixelFormatInfo::info(format->fourcc).colourEncoding);\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 4A5FAC3272\n\tfor ;\n\tFri, 26 Aug 2022 06:17:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6903F61FC0;\n\tFri, 26 Aug 2022 08:17:06 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 42B8261FBA\n\tfor ;\n\tFri, 26 Aug 2022 08:17:04 +0200 (CEST)","from [IPV6:2401:4900:1f3f:806e:6647:8e5c:f441:ca9a] (unknown\n\t[IPv6:2401:4900:1f3f:806e:6647:8e5c:f441:ca9a])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1876D2B3;\n\tFri, 26 Aug 2022 08:17:02 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1661494626;\n\tbh=8Horlz2H2xIsZ3BKNbPI/nUs079bn0EgSrgLlJpdn1M=;\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=0en7DZ40VvdpnMFiluNZlDz14HAfQ4TDazRw2hxJ0BTP6O19tZFBu8pJrey4J+0ve\n\tuhOLS3NnjQ8+Bg+WP6gEO0En9Vh51m7vjMpoUbZgukHhI5zHRY10wdtTiXHt7aZuwv\n\t+nYUzK30nOpx3u8tjoqaZHXaf+ORnibTZ9qTOliV2DH0ryDNefhackbRhxhUtKjjf2\n\tOS17cYu0++zrxd2bJ6OhzOnADuTLdNRpedPYe4D969807PLsioB/otx+z151JkbHHr\n\ts9WZuFKaWzejKsztTCovJ39QQOGV4wRxZTM7LCzurBwd0rHqtEXWSchzetrf/WtPxF\n\tvP15GyZ1+S0VQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1661494623;\n\tbh=8Horlz2H2xIsZ3BKNbPI/nUs079bn0EgSrgLlJpdn1M=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=VB/DVFbBqNkzO7YXjemSYR7gxrwzIx3+hmMtvYAw7doctIEAAM7TtrPbXJpggftpu\n\trI2EEYJXnncRWxz/hCKCdO4pb2N1yBOW0v6fTHm6xxNIXa/sG3NLrH4Ly64uOcd+Kr\n\tpyLRmFaKxTJeCJlVNB7sZlBkgf9+DX6eQSuVGKX0="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"VB/DVFbB\"; dkim-atps=neutral","Message-ID":"","Date":"Fri, 26 Aug 2022 11:46:58 +0530","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.12.0","Content-Language":"en-US","To":"Laurent Pinchart ","References":"<20220824162425.71087-1-umang.jain@ideasonboard.com>\n\t<20220824162425.71087-3-umang.jain@ideasonboard.com>\n\t","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH v2 2/6] libcamera: v4l2_device: Adjust\n\tcolorspace if pixel format is RGB","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":"Umang Jain via libcamera-devel ","Reply-To":"Umang Jain ","Cc":"libcamera-devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24788,"web_url":"https://patchwork.libcamera.org/comment/24788/","msgid":"","date":"2022-08-26T08:26:20","subject":"Re: [libcamera-devel] [PATCH v2 2/6] libcamera: v4l2_device: Adjust\n\tcolorspace if pixel format is RGB","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 8/25/22 8:39 PM, Laurent Pinchart wrote:\n> Another comment.\n>\n> On Thu, Aug 25, 2022 at 02:05:57AM +0300, Laurent Pinchart via libcamera-devel wrote:\n>> Hi Umang,\n>>\n>> Thank you for the patch.\n>>\n>> On Wed, Aug 24, 2022 at 09:54:21PM +0530, Umang Jain via libcamera-devel wrote:\n>>> The V4L2_COLORSPACE_SRGB colorspace maps to ColorSpace::Srgb.\n>>> This is wrong as the V4L2_COLORSPACE_SRGB is ill-defined (defines\n>>> Rec 601 Y'CbCr encoding and limited range) in the kernel.\n>>>\n>>> The RGB pixel formats should not use any Y'CbCr encoding and is always\n>>> full range. Adjust the colorspace before reporting back to the\n>>> userspace in such a situation.\n>>>\n>>> Moving forwards, the ColorSpace::Srgb will be defined in the true sense\n>>> for RGB pixel formats.\n>> Mentioning sRGB here is a bit confusing I think. Let's focus on what\n>> this patch does (and this will also help making sure I understood this\n>> correctly):\n>>\n>> V4L2 has no \"none\" YCbCr encoding, and thus reports an encoding for all\n>> formats, including RGB and raw formats. This causes the libcamera\n>> ColorSpace to report incorrect encodings for non-YUV formats. Fix it by\n>> overriding the encoding reported by the kernel to YCbCrEncoding::None\n>> for non-YUV pixel formats and media bus formats.\n>>\n>> Similarly, override the quantization range of non-YUV formats to full\n>> range, as limited range isn't used for RGB and raw formats.\n>>\n>>> Signed-off-by: Umang Jain \n>>> ---\n>>> include/libcamera/internal/v4l2_device.h | 5 ++++-\n>>> src/libcamera/v4l2_device.cpp | 19 +++++++++++++++----\n>>> src/libcamera/v4l2_subdevice.cpp | 10 ++++++++--\n>>> src/libcamera/v4l2_videodevice.cpp | 12 ++++++++----\n>>> 4 files changed, 35 insertions(+), 11 deletions(-)\n>>>\n>>> diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h\n>>> index a52a5f2c..5ae2ef8a 100644\n>>> --- a/include/libcamera/internal/v4l2_device.h\n>>> +++ b/include/libcamera/internal/v4l2_device.h\n>>> @@ -22,6 +22,8 @@\n>>> #include \n>>> #include \n>>> \n>>> +#include \"libcamera/internal/formats.h\"\n>>> +\n>>> namespace libcamera {\n>>> \n>>> class EventNotifier;\n>>> @@ -59,7 +61,8 @@ protected:\n>>> \tint fd() const { return fd_.get(); }\n>>> \n>>> \ttemplate\n>>> -\tstatic std::optional toColorSpace(const T &v4l2Format);\n>>> +\tstatic std::optional toColorSpace(const T &v4l2Format,\n>>> +\t\t\t\t\t\t const PixelFormatInfo::ColourEncoding &colourEncoding);\n>> \t\t\t\t\t\t PixelFormatInfo::ColourEncoding colourEncoding);\n>> Same below.\n>>\n>>> \n>>> \ttemplate\n>>> \tstatic int fromColorSpace(const std::optional &colorSpace, T &v4l2Format);\n>>> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n>>> index b22a981f..1fb08b9d 100644\n>>> --- a/src/libcamera/v4l2_device.cpp\n>>> +++ b/src/libcamera/v4l2_device.cpp\n>>> @@ -24,6 +24,7 @@\n>>> #include \n>>> #include \n>>> \n>>> +#include \"libcamera/internal/formats.h\"\n>>> #include \"libcamera/internal/sysfs.h\"\n>>> \n>>> /**\n>>> @@ -816,7 +817,8 @@ static const std::map rangeToV4l2 = {\n>>> * \\retval std::nullopt One or more V4L2 color space fields were not recognised\n>>> */\n> The documentation needs to be updated with the new parameter.\n>\n>>> template\n>>> -std::optional V4L2Device::toColorSpace(const T &v4l2Format)\n>>> +std::optional V4L2Device::toColorSpace(const T &v4l2Format,\n>>> +\t\t\t\t\t\t const PixelFormatInfo::ColourEncoding &colourEncoding)\n>>> {\n>>> \tauto itColor = v4l2ToColorSpace.find(v4l2Format.colorspace);\n>>> \tif (itColor == v4l2ToColorSpace.end())\n>>> @@ -839,6 +841,9 @@ std::optional V4L2Device::toColorSpace(const T &v4l2Format)\n>>> \t\t\treturn std::nullopt;\n>>> \n>>> \t\tcolorSpace.ycbcrEncoding = itYcbcrEncoding->second;\n>>> +\n>> I'd add a comment here, or we'll forget why.\n>>\n>> \t\t/*\n>> \t\t * V4L2 has no \"none\" encoding, override the value returned by\n>> \t\t * the kernel for non-YUV formats as YCbCr encoding isn't\n>> \t\t * applicable in that case..\n>> \t\t */\n>>\n>>> +\t\tif (colourEncoding == PixelFormatInfo::ColourEncodingRGB)\n>> Shouldn't this be\n>>\n>> \t\tif (colourEncoding != PixelFormatInfo::ColourEncodingYUV)\n>>\n>> ? Raw formats have no YCbCr encoding either. The subject line would need\n>> to be updated accordingly, I'd write \"Adjust colorspace based on pixel\n>> format\".\n>>\n>>> +\t\t\tcolorSpace.ycbcrEncoding = ColorSpace::YcbcrEncoding::None;\n>>> \t}\n>>> \n>>> \tif (v4l2Format.quantization != V4L2_QUANTIZATION_DEFAULT) {\n>>> @@ -847,14 +852,20 @@ std::optional V4L2Device::toColorSpace(const T &v4l2Format)\n>>> \t\t\treturn std::nullopt;\n>>> \n>>> \t\tcolorSpace.range = itRange->second;\n>>> +\n>>> +\t\tif (colourEncoding == PixelFormatInfo::ColourEncodingRGB)\n>> Same here.\n>>\n>>> +\t\t\tcolorSpace.range = ColorSpace::Range::Full;\n>>> \t}\n>>> \n>>> \treturn colorSpace;\n>>> }\n>>> \n>>> -template std::optional V4L2Device::toColorSpace(const struct v4l2_pix_format &);\n>>> -template std::optional V4L2Device::toColorSpace(const struct v4l2_pix_format_mplane &);\n>>> -template std::optional V4L2Device::toColorSpace(const struct v4l2_mbus_framefmt &);\n>>> +template std::optional V4L2Device::toColorSpace(const struct v4l2_pix_format &,\n>>> +\t\t\t\t\t\t\t const PixelFormatInfo::ColourEncoding &);\n>>> +template std::optional V4L2Device::toColorSpace(const struct v4l2_pix_format_mplane &,\n>>> +\t\t\t\t\t\t\t const PixelFormatInfo::ColourEncoding &);\n>>> +template std::optional V4L2Device::toColorSpace(const struct v4l2_mbus_framefmt &,\n>>> +\t\t\t\t\t\t\t const PixelFormatInfo::ColourEncoding &);\n>>> \n>>> /**\n>>> * \\brief Fill in the color space fields of a V4L2 format from a ColorSpace\n>>> diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp\n>>> index ab74b9f0..a52c414f 100644\n>>> --- a/src/libcamera/v4l2_subdevice.cpp\n>>> +++ b/src/libcamera/v4l2_subdevice.cpp\n>>> @@ -502,7 +502,10 @@ int V4L2Subdevice::getFormat(unsigned int pad, V4L2SubdeviceFormat *format,\n>>> \tformat->size.width = subdevFmt.format.width;\n>>> \tformat->size.height = subdevFmt.format.height;\n>>> \tformat->mbus_code = subdevFmt.format.code;\n>>> -\tformat->colorSpace = toColorSpace(subdevFmt.format);\n>>> +\tauto iter = formatInfoMap.find(format->mbus_code);\n>>> +\tif (iter == formatInfoMap.end())\n>>> +\t\treturn -EINVAL;\n>> That seems a bit harsh. I'm thinking about the simple pipeline handler\n>> in particular, which will propagate media bus formats through the\n>> pipeline without caring much about the format itself. Would it better to\n>> instead pick a default ?\n>>\n>>> +\tformat->colorSpace = toColorSpace(subdevFmt.format, iter->second.colourEncoding);\n>>> \n>>> \treturn 0;\n>>> }\n>>> @@ -548,7 +551,10 @@ int V4L2Subdevice::setFormat(unsigned int pad, V4L2SubdeviceFormat *format,\n>>> \tformat->size.width = subdevFmt.format.width;\n>>> \tformat->size.height = subdevFmt.format.height;\n>>> \tformat->mbus_code = subdevFmt.format.code;\n>>> -\tformat->colorSpace = toColorSpace(subdevFmt.format);\n>>> +\tauto iter = formatInfoMap.find(format->mbus_code);\n>>> +\tif (iter == formatInfoMap.end())\n>>> +\t\treturn -EINVAL;\n>> Same here.\n\nEsp in V4l2Subdevice::setFormat() , picking a default and reporting that \nback makes me a bit unconformtable ....\n\nshould we chose to fail hard here?\n\ndiff --git a/src/libcamera/v4l2_subdevice.cpp \nb/src/libcamera/v4l2_subdevice.cpp\nindex 70f179c2..d415a738 100644\n--- a/src/libcamera/v4l2_subdevice.cpp\n+++ b/src/libcamera/v4l2_subdevice.cpp\n@@ -502,10 +502,19 @@ int V4L2Subdevice::getFormat(unsigned int pad, \nV4L2SubdeviceFormat *format,\n\n         format->size.width = subdevFmt.format.width;\n         format->size.height = subdevFmt.format.height;\n+\n+       auto iter = formatInfoMap.find(subdevFmt.format.code);\n+       if (iter == formatInfoMap.end()) {\n+               /* Pick a default format to continue */\n+               uint32_t defCode = MEDIA_BUS_FMT_RGB888_1X24;\n+               iter = formatInfoMap.find(defCode);\n+               LOG(V4L2, Error)\n+                        << \"Mapping for subdev format \" << \nsubdevFmt.format.code\n+                        << \" is missing, falling back to \" << \niter->second.name;\n+\n+               subdevFmt.format.code = defCode;\n+       }\n         format->mbus_code = subdevFmt.format.code;\n-       auto iter = formatInfoMap.find(format->mbus_code);\n-       if (iter == formatInfoMap.end())\n-               return -EINVAL;\n         format->colorSpace = toColorSpace(subdevFmt.format, \niter->second.colourEncoding);\n\n         return 0;\n@@ -551,10 +560,20 @@ int V4L2Subdevice::setFormat(unsigned int pad, \nV4L2SubdeviceFormat *format,\n\n         format->size.width = subdevFmt.format.width;\n         format->size.height = subdevFmt.format.height;\n+\n+       auto iter = formatInfoMap.find(subdevFmt.format.code);\n+       if (iter == formatInfoMap.end()) {\n+               /* Pick a default format to continue */\n+               uint32_t defCode = MEDIA_BUS_FMT_RGB888_1X24;\n+               iter = formatInfoMap.find(defCode);\n+\n+               LOG(V4L2, Error)\n+                       << \"Mapping for subdev format \" << \nsubdevFmt.format.code\n+                       << \" is missing, falling back to \" << \niter->second.name;\n+\n+               subdevFmt.format.code = defCode;\n+       }\n         format->mbus_code = subdevFmt.format.code;\n-       auto iter = formatInfoMap.find(format->mbus_code);\n-       if (iter == formatInfoMap.end())\n-               return -EINVAL;\n         format->colorSpace = toColorSpace(subdevFmt.format, \niter->second.colourEncoding);\n\n         return 0;\n\n>>\n>>> +\tformat->colorSpace = toColorSpace(subdevFmt.format, iter->second.colourEncoding);\n>>> \n>>> \treturn 0;\n>>> }\n>>> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n>>> index 5a2d0e5b..0e3f5436 100644\n>>> --- a/src/libcamera/v4l2_videodevice.cpp\n>>> +++ b/src/libcamera/v4l2_videodevice.cpp\n>>> @@ -931,7 +931,8 @@ int V4L2VideoDevice::getFormatMultiplane(V4L2DeviceFormat *format)\n>> I'm tempted to add a helper just above this function:\n>>\n>> namespace {\n>>\n>> std::optional toColorSpace(const struct v4l2_pix_format_mplane &pix)\n>> {\n>> \tV4L2PixelFormat fourcc{ pix.pixelformat };\n>> \treturn toColorSpace(pix, PixelFormatInfo::info(fourcc).colourEncoding);\n>> }\n>>\n>> } /* namespace */\n>>\n>> to make the color below a bit nicer (it would actually not need to be\n>> changed at all). Up to you.\n>>\n>>> \tformat->size.height = pix->height;\n>>> \tformat->fourcc = V4L2PixelFormat(pix->pixelformat);\n>>> \tformat->planesCount = pix->num_planes;\n>>> -\tformat->colorSpace = toColorSpace(*pix);\n>>> +\tformat->colorSpace =\n>>> +\t\ttoColorSpace(*pix, PixelFormatInfo::info(format->fourcc).colourEncoding);\n>>> \n>>> \tfor (unsigned int i = 0; i < format->planesCount; ++i) {\n>>> \t\tformat->planes[i].bpl = pix->plane_fmt[i].bytesperline;\n>>> @@ -987,7 +988,8 @@ 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 = toColorSpace(*pix);\n>>> +\tformat->colorSpace =\n>>> +\t\ttoColorSpace(*pix, PixelFormatInfo::info(format->fourcc).colourEncoding);\n>>> \n>>> \treturn 0;\n>>> }\n>>> @@ -1011,7 +1013,8 @@ int V4L2VideoDevice::getFormatSingleplane(V4L2DeviceFormat *format)\n>>> \tformat->planesCount = 1;\n>>> \tformat->planes[0].bpl = pix->bytesperline;\n>>> \tformat->planes[0].size = pix->sizeimage;\n>>> -\tformat->colorSpace = toColorSpace(*pix);\n>>> +\tformat->colorSpace =\n>>> +\t\ttoColorSpace(*pix, PixelFormatInfo::info(format->fourcc).colourEncoding);\n>>> \n>>> \treturn 0;\n>>> }\n>>> @@ -1053,7 +1056,8 @@ 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 = toColorSpace(*pix);\n>>> +\tformat->colorSpace =\n>>> +\t\ttoColorSpace(*pix, PixelFormatInfo::info(format->fourcc).colourEncoding);\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 03570C0DA4\n\tfor ;\n\tFri, 26 Aug 2022 08:26:30 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6AAF061FC1;\n\tFri, 26 Aug 2022 10:26:29 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 535BD61FB9\n\tfor ;\n\tFri, 26 Aug 2022 10:26:28 +0200 (CEST)","from [IPV6:2401:4900:1f3f:806e:6647:8e5c:f441:ca9a] (unknown\n\t[IPv6:2401:4900:1f3f:806e:6647:8e5c:f441:ca9a])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 982822B3;\n\tFri, 26 Aug 2022 10:26:26 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1661502389;\n\tbh=mD+DHis1/jWjHSydakr5Au+inU684bv8+J4HUvF++ww=;\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:\n\tFrom;\n\tb=jxODXGpykJVDvTiMxApxe+h1GMCfWbpNMJzVW11p7ZoEJibwh8FFCjA7HI7ymJIZf\n\tFNNAl+42BAhpK9uceFNhn4mrWFUvNFxf7O5y4l6065SQOHd+MdFD/aUGr8h83yTEr+\n\t1FaW/W8+7cakACzWJ221nnSqexyjOCz4ahsSZaRTsWad05TVkrnqUZXbL4/15YOcDx\n\t6nIS2H82bvklp6vs+lmVUQqj1wJSdGNT87RA+uopB0V5ByyDOlTXxhLwQb+B1s9LuM\n\tBeMfvs/ifioQaXoylPigJUOD8SgydqaK6mPlL90WJQFQpZq9O2IR/LtLHMy/mTXoLf\n\tpMrY5W0Rf8gbA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1661502387;\n\tbh=mD+DHis1/jWjHSydakr5Au+inU684bv8+J4HUvF++ww=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=nE+fdOpPHZYU3GCkfcvEeJSxf0or/BzLLcl4BSsICBh4JLgTvDiCMMppbNa0nRmUs\n\tgQvoY/V4tXGpKlvCfkBCujnoNFQ2lqnv9T8lSWeobDgJBKv7Hq9hNtCRNeYn2ruWjM\n\thRXo9rTAMNUe50Jo1jx84xqGduYhjC8uhhSdmubY="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"nE+fdOpP\"; dkim-atps=neutral","Message-ID":"","Date":"Fri, 26 Aug 2022 13:56:20 +0530","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.12.0","Content-Language":"en-US","To":"Laurent Pinchart ,\n\tlibcamera-devel ","References":"<20220824162425.71087-1-umang.jain@ideasonboard.com>\n\t<20220824162425.71087-3-umang.jain@ideasonboard.com>\n\t\n\t","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v2 2/6] libcamera: v4l2_device: Adjust\n\tcolorspace if pixel format is RGB","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":"Umang Jain via libcamera-devel ","Reply-To":"Umang Jain ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24795,"web_url":"https://patchwork.libcamera.org/comment/24795/","msgid":"","date":"2022-08-26T13:23:09","subject":"Re: [libcamera-devel] [PATCH v2 2/6] libcamera: v4l2_device: Adjust\n\tcolorspace if pixel format is RGB","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Umang,\n\nOn Fri, Aug 26, 2022 at 11:46:58AM +0530, Umang Jain wrote:\n> On 8/25/22 4:35 AM, Laurent Pinchart wrote:\n> > On Wed, Aug 24, 2022 at 09:54:21PM +0530, Umang Jain via libcamera-devel wrote:\n> >> The V4L2_COLORSPACE_SRGB colorspace maps to ColorSpace::Srgb.\n> >> This is wrong as the V4L2_COLORSPACE_SRGB is ill-defined (defines\n> >> Rec 601 Y'CbCr encoding and limited range) in the kernel.\n> >>\n> >> The RGB pixel formats should not use any Y'CbCr encoding and is always\n> >> full range. Adjust the colorspace before reporting back to the\n> >> userspace in such a situation.\n> >>\n> >> Moving forwards, the ColorSpace::Srgb will be defined in the true sense\n> >> for RGB pixel formats.\n> > \n> > Mentioning sRGB here is a bit confusing I think. Let's focus on what\n> > this patch does (and this will also help making sure I understood this\n> > correctly):\n> >\n> > V4L2 has no \"none\" YCbCr encoding, and thus reports an encoding for all\n> > formats, including RGB and raw formats. This causes the libcamera\n> > ColorSpace to report incorrect encodings for non-YUV formats. Fix it by\n> > overriding the encoding reported by the kernel to YCbCrEncoding::None\n> > for non-YUV pixel formats and media bus formats.\n> \n> Correct.\n> \n> > Similarly, override the quantization range of non-YUV formats to full\n> > range, as limited range isn't used for RGB and raw formats.\n> \n> Seems correct too, I agree my commit message was not at it's best.\n> \n> >> Signed-off-by: Umang Jain \n> >> ---\n> >> include/libcamera/internal/v4l2_device.h | 5 ++++-\n> >> src/libcamera/v4l2_device.cpp | 19 +++++++++++++++----\n> >> src/libcamera/v4l2_subdevice.cpp | 10 ++++++++--\n> >> src/libcamera/v4l2_videodevice.cpp | 12 ++++++++----\n> >> 4 files changed, 35 insertions(+), 11 deletions(-)\n> >>\n> >> diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h\n> >> index a52a5f2c..5ae2ef8a 100644\n> >> --- a/include/libcamera/internal/v4l2_device.h\n> >> +++ b/include/libcamera/internal/v4l2_device.h\n> >> @@ -22,6 +22,8 @@\n> >> #include \n> >> #include \n> >> \n> >> +#include \"libcamera/internal/formats.h\"\n> >> +\n> >> namespace libcamera {\n> >> \n> >> class EventNotifier;\n> >> @@ -59,7 +61,8 @@ protected:\n> >> \tint fd() const { return fd_.get(); }\n> >> \n> >> \ttemplate\n> >> -\tstatic std::optional toColorSpace(const T &v4l2Format);\n> >> +\tstatic std::optional toColorSpace(const T &v4l2Format,\n> >> +\t\t\t\t\t\t const PixelFormatInfo::ColourEncoding &colourEncoding);\n> > \t\t\t\t\t\t PixelFormatInfo::ColourEncoding colourEncoding);\n> > Same below.\n> >\n> >> \n> >> \ttemplate\n> >> \tstatic int fromColorSpace(const std::optional &colorSpace, T &v4l2Format);\n> >> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> >> index b22a981f..1fb08b9d 100644\n> >> --- a/src/libcamera/v4l2_device.cpp\n> >> +++ b/src/libcamera/v4l2_device.cpp\n> >> @@ -24,6 +24,7 @@\n> >> #include \n> >> #include \n> >> \n> >> +#include \"libcamera/internal/formats.h\"\n> >> #include \"libcamera/internal/sysfs.h\"\n> >> \n> >> /**\n> >> @@ -816,7 +817,8 @@ static const std::map rangeToV4l2 = {\n> >> * \\retval std::nullopt One or more V4L2 color space fields were not recognised\n> >> */\n> >> template\n> >> -std::optional V4L2Device::toColorSpace(const T &v4l2Format)\n> >> +std::optional V4L2Device::toColorSpace(const T &v4l2Format,\n> >> +\t\t\t\t\t\t const PixelFormatInfo::ColourEncoding &colourEncoding)\n> >> {\n> >> \tauto itColor = v4l2ToColorSpace.find(v4l2Format.colorspace);\n> >> \tif (itColor == v4l2ToColorSpace.end())\n> >> @@ -839,6 +841,9 @@ std::optional V4L2Device::toColorSpace(const T &v4l2Format)\n> >> \t\t\treturn std::nullopt;\n> >> \n> >> \t\tcolorSpace.ycbcrEncoding = itYcbcrEncoding->second;\n> >> +\n> > \n> > I'd add a comment here, or we'll forget why.\n> >\n> > \t\t/*\n> > \t\t * V4L2 has no \"none\" encoding, override the value returned by\n> > \t\t * the kernel for non-YUV formats as YCbCr encoding isn't\n> > \t\t * applicable in that case..\n> > \t\t */\n> >\n> >> +\t\tif (colourEncoding == PixelFormatInfo::ColourEncodingRGB)\n> > \n> > Shouldn't this be\n> >\n> > \t\tif (colourEncoding != PixelFormatInfo::ColourEncodingYUV)\n> >\n> > ? Raw formats have no YCbCr encoding either. The subject line would need\n> > to be updated accordingly, I'd write \"Adjust colorspace based on pixel\n> > format\".\n> >\n> >> +\t\t\tcolorSpace.ycbcrEncoding = ColorSpace::YcbcrEncoding::None;\n> >> \t}\n> >> \n> >> \tif (v4l2Format.quantization != V4L2_QUANTIZATION_DEFAULT) {\n> >> @@ -847,14 +852,20 @@ std::optional V4L2Device::toColorSpace(const T &v4l2Format)\n> >> \t\t\treturn std::nullopt;\n> >> \n> >> \t\tcolorSpace.range = itRange->second;\n> >> +\n> >> +\t\tif (colourEncoding == PixelFormatInfo::ColourEncodingRGB)\n> > \n> > Same here.\n> >\n> >> +\t\t\tcolorSpace.range = ColorSpace::Range::Full;\n> >> \t}\n> >> \n> >> \treturn colorSpace;\n> >> }\n> >> \n> >> -template std::optional V4L2Device::toColorSpace(const struct v4l2_pix_format &);\n> >> -template std::optional V4L2Device::toColorSpace(const struct v4l2_pix_format_mplane &);\n> >> -template std::optional V4L2Device::toColorSpace(const struct v4l2_mbus_framefmt &);\n> >> +template std::optional V4L2Device::toColorSpace(const struct v4l2_pix_format &,\n> >> +\t\t\t\t\t\t\t const PixelFormatInfo::ColourEncoding &);\n> >> +template std::optional V4L2Device::toColorSpace(const struct v4l2_pix_format_mplane &,\n> >> +\t\t\t\t\t\t\t const PixelFormatInfo::ColourEncoding &);\n> >> +template std::optional V4L2Device::toColorSpace(const struct v4l2_mbus_framefmt &,\n> >> +\t\t\t\t\t\t\t const PixelFormatInfo::ColourEncoding &);\n> >> \n> >> /**\n> >> * \\brief Fill in the color space fields of a V4L2 format from a ColorSpace\n> >> diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp\n> >> index ab74b9f0..a52c414f 100644\n> >> --- a/src/libcamera/v4l2_subdevice.cpp\n> >> +++ b/src/libcamera/v4l2_subdevice.cpp\n> >> @@ -502,7 +502,10 @@ int V4L2Subdevice::getFormat(unsigned int pad, V4L2SubdeviceFormat *format,\n> >> \tformat->size.width = subdevFmt.format.width;\n> >> \tformat->size.height = subdevFmt.format.height;\n> >> \tformat->mbus_code = subdevFmt.format.code;\n> >> -\tformat->colorSpace = toColorSpace(subdevFmt.format);\n> >> +\tauto iter = formatInfoMap.find(format->mbus_code);\n> >> +\tif (iter == formatInfoMap.end())\n> >> +\t\treturn -EINVAL;\n> > \n> > That seems a bit harsh. I'm thinking about the simple pipeline handler\n> > in particular, which will propagate media bus formats through the\n> > pipeline without caring much about the format itself. Would it better to\n> > instead pick a default ?\n> \n> I wasn't aware of that. I assumed (formatInfoMap.find(format->mbus_code) \n> == formatInfoMap.end()) would be very hard to trigger and if it does at \n> all, we want to fail loudly.\n\nCurrently, we only use formatInfoMap in two places:\n\n- in V4L2SubdeviceFormat::bitsPerPixel()\n- in the << operator\n\nThe former will complain with an error message and return 0 if the media\nbus code is unknown, the latter will print the hex value.\n\nbitsPerPixel() is used in CameraSensor::sensorInfo() only. We thus\ncomplain already if a sensor has an unknown output format. We however\ndon't complain about unknown formats anywhere else, and there may thus\nbe places where we already handle them silently. I'm thinking in\nparticular about format propagation in the simple pipeline handler,\nwhere we just get the format on the output of a subdevice and set it on\nthe input of the connected subdevice, without caring about the exact\nformat. We may also be use MEDIA_BUS_FMT_METADATA_FIXED without knowing\nit, with the change here an attempt to call getFormat() on a subdev pad\nthat carries metadata will return in -EINVAL.\n\nI would prefer keeping the current behaviour of selecting a reasonable\ndefault in this patch series, and then add a stricter check on top if we\nwant to go that way.\n\n> I am not much aware of simple pipeline handler format's passing - I'll \n> take a look to answer the question - \"Which default to pick\"\n> \n> >> +\tformat->colorSpace = toColorSpace(subdevFmt.format, iter->second.colourEncoding);\n> >> \n> >> \treturn 0;\n> >> }\n> >> @@ -548,7 +551,10 @@ int V4L2Subdevice::setFormat(unsigned int pad, V4L2SubdeviceFormat *format,\n> >> \tformat->size.width = subdevFmt.format.width;\n> >> \tformat->size.height = subdevFmt.format.height;\n> >> \tformat->mbus_code = subdevFmt.format.code;\n> >> -\tformat->colorSpace = toColorSpace(subdevFmt.format);\n> >> +\tauto iter = formatInfoMap.find(format->mbus_code);\n> >> +\tif (iter == formatInfoMap.end())\n> >> +\t\treturn -EINVAL;\n> > \n> > Same here.\n> >\n> >> +\tformat->colorSpace = toColorSpace(subdevFmt.format, iter->second.colourEncoding);\n> >> \n> >> \treturn 0;\n> >> }\n> >> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> >> index 5a2d0e5b..0e3f5436 100644\n> >> --- a/src/libcamera/v4l2_videodevice.cpp\n> >> +++ b/src/libcamera/v4l2_videodevice.cpp\n> >> @@ -931,7 +931,8 @@ int V4L2VideoDevice::getFormatMultiplane(V4L2DeviceFormat *format)\n> > \n> > I'm tempted to add a helper just above this function:\n> >\n> > namespace {\n> >\n> > std::optional toColorSpace(const struct v4l2_pix_format_mplane &pix)\n> > {\n> > \tV4L2PixelFormat fourcc{ pix.pixelformat };\n> > \treturn toColorSpace(pix, PixelFormatInfo::info(fourcc).colourEncoding);\n> > }\n> >\n> > } /* namespace */\n> >\n> > to make the color below a bit nicer (it would actually not need to be\n> > changed at all). Up to you.\n> \n> Ack\n> \n> >> \tformat->size.height = pix->height;\n> >> \tformat->fourcc = V4L2PixelFormat(pix->pixelformat);\n> >> \tformat->planesCount = pix->num_planes;\n> >> -\tformat->colorSpace = toColorSpace(*pix);\n> >> +\tformat->colorSpace =\n> >> +\t\ttoColorSpace(*pix, PixelFormatInfo::info(format->fourcc).colourEncoding);\n> >> \n> >> \tfor (unsigned int i = 0; i < format->planesCount; ++i) {\n> >> \t\tformat->planes[i].bpl = pix->plane_fmt[i].bytesperline;\n> >> @@ -987,7 +988,8 @@ 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 = toColorSpace(*pix);\n> >> +\tformat->colorSpace =\n> >> +\t\ttoColorSpace(*pix, PixelFormatInfo::info(format->fourcc).colourEncoding);\n> >> \n> >> \treturn 0;\n> >> }\n> >> @@ -1011,7 +1013,8 @@ int V4L2VideoDevice::getFormatSingleplane(V4L2DeviceFormat *format)\n> >> \tformat->planesCount = 1;\n> >> \tformat->planes[0].bpl = pix->bytesperline;\n> >> \tformat->planes[0].size = pix->sizeimage;\n> >> -\tformat->colorSpace = toColorSpace(*pix);\n> >> +\tformat->colorSpace =\n> >> +\t\ttoColorSpace(*pix, PixelFormatInfo::info(format->fourcc).colourEncoding);\n> >> \n> >> \treturn 0;\n> >> }\n> >> @@ -1053,7 +1056,8 @@ 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 = toColorSpace(*pix);\n> >> +\tformat->colorSpace =\n> >> +\t\ttoColorSpace(*pix, PixelFormatInfo::info(format->fourcc).colourEncoding);\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 65C57C0DA4\n\tfor ;\n\tFri, 26 Aug 2022 13:23:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9113561FC0;\n\tFri, 26 Aug 2022 15:23:19 +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 33A3861FA2\n\tfor ;\n\tFri, 26 Aug 2022 15:23:18 +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 64F902B3;\n\tFri, 26 Aug 2022 15:23:17 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1661520199;\n\tbh=CSbnYoOVHRgLAkQaI+FeLeO1TrpoM3baFmslSIv4bVk=;\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=d33XUKPCvRnaUC94HfYgQ91IpmA0Y3xwPsrj4YAMC0mcJWtIIbZ3CzQ+tlS44oCD8\n\t7nJv9+/eIVHI8cKrmbverwfvaPIfxxL8VKa3ETfz95KIFyClELv0Ie0g5Z2EXExKTl\n\tqHr62g4OUcVOjA+m4tEPU0Izgute++ry/WMKIP/kgho3Jp2lxIwO9Mop3a5gTyPrwO\n\t1jmSf80sgmWVhRse3PcPb0c0tsolV4Bg1zQP0pOU0lMZujxCUL8Sjml7dg8XdmALSL\n\tOgSlQSsBitAywfjBxc4qBGimcFl2rl6UlqanZkXemwb5CIj8NTt5GxDapmzZQ4LWib\n\tyltSCcan5JWVg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1661520197;\n\tbh=CSbnYoOVHRgLAkQaI+FeLeO1TrpoM3baFmslSIv4bVk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=vw1XSFBQo2j8yK6QNBOqqYyXQHHzO+KUFjMYPTBlMaWARsklVP/uEOycR7AapNZVq\n\tUi60XMgo6o6Ufi2wlPl0xSdwvDHrSfCbtHqy2WaoeGeHcwkQUogga40E9Z2ZbyP/f1\n\tIk1ufCtiK3WMZUd5efF0nYJRiIPjK+S/K32uDOuw="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"vw1XSFBQ\"; dkim-atps=neutral","Date":"Fri, 26 Aug 2022 16:23:09 +0300","To":"Umang Jain ","Message-ID":"","References":"<20220824162425.71087-1-umang.jain@ideasonboard.com>\n\t<20220824162425.71087-3-umang.jain@ideasonboard.com>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2 2/6] libcamera: v4l2_device: Adjust\n\tcolorspace if pixel format is RGB","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24797,"web_url":"https://patchwork.libcamera.org/comment/24797/","msgid":"","date":"2022-08-26T13:36:35","subject":"Re: [libcamera-devel] [PATCH v2 2/6] libcamera: v4l2_device: Adjust\n\tcolorspace if pixel format is RGB","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Umang,\n\nOn Fri, Aug 26, 2022 at 01:56:20PM +0530, Umang Jain wrote:\n> On 8/25/22 8:39 PM, Laurent Pinchart wrote:\n> > On Thu, Aug 25, 2022 at 02:05:57AM +0300, Laurent Pinchart via libcamera-devel wrote:\n> >> On Wed, Aug 24, 2022 at 09:54:21PM +0530, Umang Jain via libcamera-devel wrote:\n> >>> The V4L2_COLORSPACE_SRGB colorspace maps to ColorSpace::Srgb.\n> >>> This is wrong as the V4L2_COLORSPACE_SRGB is ill-defined (defines\n> >>> Rec 601 Y'CbCr encoding and limited range) in the kernel.\n> >>>\n> >>> The RGB pixel formats should not use any Y'CbCr encoding and is always\n> >>> full range. Adjust the colorspace before reporting back to the\n> >>> userspace in such a situation.\n> >>>\n> >>> Moving forwards, the ColorSpace::Srgb will be defined in the true sense\n> >>> for RGB pixel formats.\n> >>\n> >> Mentioning sRGB here is a bit confusing I think. Let's focus on what\n> >> this patch does (and this will also help making sure I understood this\n> >> correctly):\n> >>\n> >> V4L2 has no \"none\" YCbCr encoding, and thus reports an encoding for all\n> >> formats, including RGB and raw formats. This causes the libcamera\n> >> ColorSpace to report incorrect encodings for non-YUV formats. Fix it by\n> >> overriding the encoding reported by the kernel to YCbCrEncoding::None\n> >> for non-YUV pixel formats and media bus formats.\n> >>\n> >> Similarly, override the quantization range of non-YUV formats to full\n> >> range, as limited range isn't used for RGB and raw formats.\n> >>\n> >>> Signed-off-by: Umang Jain \n> >>> ---\n> >>> include/libcamera/internal/v4l2_device.h | 5 ++++-\n> >>> src/libcamera/v4l2_device.cpp | 19 +++++++++++++++----\n> >>> src/libcamera/v4l2_subdevice.cpp | 10 ++++++++--\n> >>> src/libcamera/v4l2_videodevice.cpp | 12 ++++++++----\n> >>> 4 files changed, 35 insertions(+), 11 deletions(-)\n> >>>\n> >>> diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h\n> >>> index a52a5f2c..5ae2ef8a 100644\n> >>> --- a/include/libcamera/internal/v4l2_device.h\n> >>> +++ b/include/libcamera/internal/v4l2_device.h\n> >>> @@ -22,6 +22,8 @@\n> >>> #include \n> >>> #include \n> >>> \n> >>> +#include \"libcamera/internal/formats.h\"\n> >>> +\n> >>> namespace libcamera {\n> >>> \n> >>> class EventNotifier;\n> >>> @@ -59,7 +61,8 @@ protected:\n> >>> \tint fd() const { return fd_.get(); }\n> >>> \n> >>> \ttemplate\n> >>> -\tstatic std::optional toColorSpace(const T &v4l2Format);\n> >>> +\tstatic std::optional toColorSpace(const T &v4l2Format,\n> >>> +\t\t\t\t\t\t const PixelFormatInfo::ColourEncoding &colourEncoding);\n> >>\n> >> \t\t\t\t\t\t PixelFormatInfo::ColourEncoding colourEncoding);\n> >> Same below.\n> >>\n> >>> \n> >>> \ttemplate\n> >>> \tstatic int fromColorSpace(const std::optional &colorSpace, T &v4l2Format);\n> >>> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> >>> index b22a981f..1fb08b9d 100644\n> >>> --- a/src/libcamera/v4l2_device.cpp\n> >>> +++ b/src/libcamera/v4l2_device.cpp\n> >>> @@ -24,6 +24,7 @@\n> >>> #include \n> >>> #include \n> >>> \n> >>> +#include \"libcamera/internal/formats.h\"\n> >>> #include \"libcamera/internal/sysfs.h\"\n> >>> \n> >>> /**\n> >>> @@ -816,7 +817,8 @@ static const std::map rangeToV4l2 = {\n> >>> * \\retval std::nullopt One or more V4L2 color space fields were not recognised\n> >>> */\n> > The documentation needs to be updated with the new parameter.\n> >\n> >>> template\n> >>> -std::optional V4L2Device::toColorSpace(const T &v4l2Format)\n> >>> +std::optional V4L2Device::toColorSpace(const T &v4l2Format,\n> >>> +\t\t\t\t\t\t const PixelFormatInfo::ColourEncoding &colourEncoding)\n> >>> {\n> >>> \tauto itColor = v4l2ToColorSpace.find(v4l2Format.colorspace);\n> >>> \tif (itColor == v4l2ToColorSpace.end())\n> >>> @@ -839,6 +841,9 @@ std::optional V4L2Device::toColorSpace(const T &v4l2Format)\n> >>> \t\t\treturn std::nullopt;\n> >>> \n> >>> \t\tcolorSpace.ycbcrEncoding = itYcbcrEncoding->second;\n> >>> +\n> >>\n> >> I'd add a comment here, or we'll forget why.\n> >>\n> >> \t\t/*\n> >> \t\t * V4L2 has no \"none\" encoding, override the value returned by\n> >> \t\t * the kernel for non-YUV formats as YCbCr encoding isn't\n> >> \t\t * applicable in that case..\n> >> \t\t */\n> >>\n> >>> +\t\tif (colourEncoding == PixelFormatInfo::ColourEncodingRGB)\n> >>\n> >> Shouldn't this be\n> >>\n> >> \t\tif (colourEncoding != PixelFormatInfo::ColourEncodingYUV)\n> >>\n> >> ? Raw formats have no YCbCr encoding either. The subject line would need\n> >> to be updated accordingly, I'd write \"Adjust colorspace based on pixel\n> >> format\".\n> >>\n> >>> +\t\t\tcolorSpace.ycbcrEncoding = ColorSpace::YcbcrEncoding::None;\n> >>> \t}\n> >>> \n> >>> \tif (v4l2Format.quantization != V4L2_QUANTIZATION_DEFAULT) {\n> >>> @@ -847,14 +852,20 @@ std::optional V4L2Device::toColorSpace(const T &v4l2Format)\n> >>> \t\t\treturn std::nullopt;\n> >>> \n> >>> \t\tcolorSpace.range = itRange->second;\n> >>> +\n> >>> +\t\tif (colourEncoding == PixelFormatInfo::ColourEncodingRGB)\n> >>\n> >> Same here.\n> >>\n> >>> +\t\t\tcolorSpace.range = ColorSpace::Range::Full;\n> >>> \t}\n> >>> \n> >>> \treturn colorSpace;\n> >>> }\n> >>> \n> >>> -template std::optional V4L2Device::toColorSpace(const struct v4l2_pix_format &);\n> >>> -template std::optional V4L2Device::toColorSpace(const struct v4l2_pix_format_mplane &);\n> >>> -template std::optional V4L2Device::toColorSpace(const struct v4l2_mbus_framefmt &);\n> >>> +template std::optional V4L2Device::toColorSpace(const struct v4l2_pix_format &,\n> >>> +\t\t\t\t\t\t\t const PixelFormatInfo::ColourEncoding &);\n> >>> +template std::optional V4L2Device::toColorSpace(const struct v4l2_pix_format_mplane &,\n> >>> +\t\t\t\t\t\t\t const PixelFormatInfo::ColourEncoding &);\n> >>> +template std::optional V4L2Device::toColorSpace(const struct v4l2_mbus_framefmt &,\n> >>> +\t\t\t\t\t\t\t const PixelFormatInfo::ColourEncoding &);\n> >>> \n> >>> /**\n> >>> * \\brief Fill in the color space fields of a V4L2 format from a ColorSpace\n> >>> diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp\n> >>> index ab74b9f0..a52c414f 100644\n> >>> --- a/src/libcamera/v4l2_subdevice.cpp\n> >>> +++ b/src/libcamera/v4l2_subdevice.cpp\n> >>> @@ -502,7 +502,10 @@ int V4L2Subdevice::getFormat(unsigned int pad, V4L2SubdeviceFormat *format,\n> >>> \tformat->size.width = subdevFmt.format.width;\n> >>> \tformat->size.height = subdevFmt.format.height;\n> >>> \tformat->mbus_code = subdevFmt.format.code;\n> >>> -\tformat->colorSpace = toColorSpace(subdevFmt.format);\n> >>> +\tauto iter = formatInfoMap.find(format->mbus_code);\n> >>> +\tif (iter == formatInfoMap.end())\n> >>> +\t\treturn -EINVAL;\n> >>\n> >> That seems a bit harsh. I'm thinking about the simple pipeline handler\n> >> in particular, which will propagate media bus formats through the\n> >> pipeline without caring much about the format itself. Would it better to\n> >> instead pick a default ?\n> >>\n> >>> +\tformat->colorSpace = toColorSpace(subdevFmt.format, iter->second.colourEncoding);\n> >>> \n> >>> \treturn 0;\n> >>> }\n> >>> @@ -548,7 +551,10 @@ int V4L2Subdevice::setFormat(unsigned int pad, V4L2SubdeviceFormat *format,\n> >>> \tformat->size.width = subdevFmt.format.width;\n> >>> \tformat->size.height = subdevFmt.format.height;\n> >>> \tformat->mbus_code = subdevFmt.format.code;\n> >>> -\tformat->colorSpace = toColorSpace(subdevFmt.format);\n> >>> +\tauto iter = formatInfoMap.find(format->mbus_code);\n> >>> +\tif (iter == formatInfoMap.end())\n> >>> +\t\treturn -EINVAL;\n> >>\n> >> Same here.\n> \n> Esp in V4l2Subdevice::setFormat() , picking a default and reporting that \n> back makes me a bit unconformtable ....\n> \n> should we chose to fail hard here?\n\nFor the reasons explained in another reply in this thread, I'd rather\nadd hard failures separately, if we decide it's the way to go. We would\nthen need to consider the impact on different parts of libcamera (simple\npipeline handler, metadata formats, ...). Even an error/warning message\nis something I would introduce in a separate patch on top, to be able to\ntest it thoroughly without blocking the rest of the series.\n\n> diff --git a/src/libcamera/v4l2_subdevice.cpp \n> b/src/libcamera/v4l2_subdevice.cpp\n> index 70f179c2..d415a738 100644\n> --- a/src/libcamera/v4l2_subdevice.cpp\n> +++ b/src/libcamera/v4l2_subdevice.cpp\n> @@ -502,10 +502,19 @@ int V4L2Subdevice::getFormat(unsigned int pad, V4L2SubdeviceFormat *format,\n> \n>         format->size.width = subdevFmt.format.width;\n>         format->size.height = subdevFmt.format.height;\n> +\n> +       auto iter = formatInfoMap.find(subdevFmt.format.code);\n> +       if (iter == formatInfoMap.end()) {\n> +               /* Pick a default format to continue */\n> +               uint32_t defCode = MEDIA_BUS_FMT_RGB888_1X24;\n> +               iter = formatInfoMap.find(defCode);\n\nYou can avoid the lookup there and just use the RGB encoding:\n\n\tPixelFormatInfo::ColourEncoding colourEncoding;\n\tauto iter = formatInfoMap.find(subdevFmt.format.code);\n\tif (iter != formatInfoMap.end()) {\n\t\tcolourEncoding = iter->second.colourEncoding;\n\t} else {\n\t\tLOG(V4L2, Warning)\n\t\t\t<< \"Unknown subdev format \"\n\t\t\t<< utils::hex(subdevFmt.format.code, 4)\n\t\t\t<< \", defaulting to RGB encoding\";\n\n\t\tcolourEncoding = PixelFormatInfo::ColourEncodingRGB;\n\t}\n\n> +               LOG(V4L2, Error)\n\nMake it a warning.\n\n> +                        << \"Mapping for subdev format \" << subdevFmt.format.code\n\nPrint the code in hex.\n\n> +                        << \" is missing, falling back to \" << iter->second.name;\n> +\n> +               subdevFmt.format.code = defCode;\n\nWhy do you override the media bus code returned by the driver ?\n\nAll these comments apply below.\n\n> +       }\n>         format->mbus_code = subdevFmt.format.code;\n> -       auto iter = formatInfoMap.find(format->mbus_code);\n> -       if (iter == formatInfoMap.end())\n> -               return -EINVAL;\n>         format->colorSpace = toColorSpace(subdevFmt.format, iter->second.colourEncoding);\n> \n>         return 0;\n> @@ -551,10 +560,20 @@ int V4L2Subdevice::setFormat(unsigned int pad, V4L2SubdeviceFormat *format,\n> \n>         format->size.width = subdevFmt.format.width;\n>         format->size.height = subdevFmt.format.height;\n> +\n> +       auto iter = formatInfoMap.find(subdevFmt.format.code);\n> +       if (iter == formatInfoMap.end()) {\n> +               /* Pick a default format to continue */\n> +               uint32_t defCode = MEDIA_BUS_FMT_RGB888_1X24;\n> +               iter = formatInfoMap.find(defCode);\n> +\n> +               LOG(V4L2, Error)\n> +                       << \"Mapping for subdev format \" << subdevFmt.format.code\n> +                       << \" is missing, falling back to \" << iter->second.name;\n> +\n> +               subdevFmt.format.code = defCode;\n> +       }\n>         format->mbus_code = subdevFmt.format.code;\n> -       auto iter = formatInfoMap.find(format->mbus_code);\n> -       if (iter == formatInfoMap.end())\n> -               return -EINVAL;\n>         format->colorSpace = toColorSpace(subdevFmt.format, iter->second.colourEncoding);\n> \n>         return 0;\n> \n> >>> +\tformat->colorSpace = toColorSpace(subdevFmt.format, iter->second.colourEncoding);\n> >>> \n> >>> \treturn 0;\n> >>> }\n> >>> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> >>> index 5a2d0e5b..0e3f5436 100644\n> >>> --- a/src/libcamera/v4l2_videodevice.cpp\n> >>> +++ b/src/libcamera/v4l2_videodevice.cpp\n> >>> @@ -931,7 +931,8 @@ int V4L2VideoDevice::getFormatMultiplane(V4L2DeviceFormat *format)\n> >>\n> >> I'm tempted to add a helper just above this function:\n> >>\n> >> namespace {\n> >>\n> >> std::optional toColorSpace(const struct v4l2_pix_format_mplane &pix)\n> >> {\n> >> \tV4L2PixelFormat fourcc{ pix.pixelformat };\n> >> \treturn toColorSpace(pix, PixelFormatInfo::info(fourcc).colourEncoding);\n> >> }\n> >>\n> >> } /* namespace */\n> >>\n> >> to make the color below a bit nicer (it would actually not need to be\n> >> changed at all). Up to you.\n> >>\n> >>> \tformat->size.height = pix->height;\n> >>> \tformat->fourcc = V4L2PixelFormat(pix->pixelformat);\n> >>> \tformat->planesCount = pix->num_planes;\n> >>> -\tformat->colorSpace = toColorSpace(*pix);\n> >>> +\tformat->colorSpace =\n> >>> +\t\ttoColorSpace(*pix, PixelFormatInfo::info(format->fourcc).colourEncoding);\n> >>> \n> >>> \tfor (unsigned int i = 0; i < format->planesCount; ++i) {\n> >>> \t\tformat->planes[i].bpl = pix->plane_fmt[i].bytesperline;\n> >>> @@ -987,7 +988,8 @@ 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 = toColorSpace(*pix);\n> >>> +\tformat->colorSpace =\n> >>> +\t\ttoColorSpace(*pix, PixelFormatInfo::info(format->fourcc).colourEncoding);\n> >>> \n> >>> \treturn 0;\n> >>> }\n> >>> @@ -1011,7 +1013,8 @@ int V4L2VideoDevice::getFormatSingleplane(V4L2DeviceFormat *format)\n> >>> \tformat->planesCount = 1;\n> >>> \tformat->planes[0].bpl = pix->bytesperline;\n> >>> \tformat->planes[0].size = pix->sizeimage;\n> >>> -\tformat->colorSpace = toColorSpace(*pix);\n> >>> +\tformat->colorSpace =\n> >>> +\t\ttoColorSpace(*pix, PixelFormatInfo::info(format->fourcc).colourEncoding);\n> >>> \n> >>> \treturn 0;\n> >>> }\n> >>> @@ -1053,7 +1056,8 @@ 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 = toColorSpace(*pix);\n> >>> +\tformat->colorSpace =\n> >>> +\t\ttoColorSpace(*pix, PixelFormatInfo::info(format->fourcc).colourEncoding);\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 4354DC0DA4\n\tfor ;\n\tFri, 26 Aug 2022 13:36:46 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 96C8A61FC0;\n\tFri, 26 Aug 2022 15:36:45 +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 7DF1061FA2\n\tfor ;\n\tFri, 26 Aug 2022 15:36:43 +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 BF14C2B3;\n\tFri, 26 Aug 2022 15:36:42 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1661521005;\n\tbh=LBZBbFnFErwK8LuSYh3uu6IOI91RVMfmJhyBySh2jF4=;\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=pMT1Ezpb51W2LI03sCn7t5vex3VgE+7rTzYWAQ8WW2EFR+nJvoJnl/BW1FJd1po0/\n\tIlgSLjene4O7m3+7b+L5/tGHcksvfp6hbRRmZUt/YY0rKghqHxwjiO2hv89RaWKCMf\n\tWvt/TVHB79TwTrGCYxjjTEkvOMhxGOg7sxsiSg73p1a3Sksh4hl2D9G1yTzK20u1sJ\n\t9crWpnmyhjqYQQEBA8UEm0MnCdEkVfTpHHO47RAc4Gly+aKH9+sUg6eaXL2ncFMsVu\n\ta3Ay+hhRhdBE3/rpWsy0lgf9nSSFbDDVf9pX/gBPUobwL2ItN/8rzE2MZBNF+SsBQM\n\t+kbK9FzHwvNaw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1661521003;\n\tbh=LBZBbFnFErwK8LuSYh3uu6IOI91RVMfmJhyBySh2jF4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=l2TnlinYxflZwRNkhAXMRMPcJVeJB9Re6kySRq1HHpNod7w4kKIuT8neZttRWfC0z\n\tZxCQRzDZTox1wn1GD24mp5PUE2yK/JqfmytVGSTziDEPRbNACZNMinkFdcozKNu6AU\n\tVNRsFTv45NPy1hI5MHVBm3ptv/9K/d+j3IwzOBco="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"l2TnlinY\"; dkim-atps=neutral","Date":"Fri, 26 Aug 2022 16:36:35 +0300","To":"Umang Jain ","Message-ID":"","References":"<20220824162425.71087-1-umang.jain@ideasonboard.com>\n\t<20220824162425.71087-3-umang.jain@ideasonboard.com>\n\t\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2 2/6] libcamera: v4l2_device: Adjust\n\tcolorspace if pixel format is RGB","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24799,"web_url":"https://patchwork.libcamera.org/comment/24799/","msgid":"<99c36566-f1ae-6808-e3f0-c6281dd3f6d6@ideasonboard.com>","date":"2022-08-26T17:33:03","subject":"Re: [libcamera-devel] [PATCH v2 2/6] libcamera: v4l2_device: Adjust\n\tcolorspace if pixel format is RGB","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 8/26/22 7:06 PM, Laurent Pinchart wrote:\n> Hi Umang,\n>\n> On Fri, Aug 26, 2022 at 01:56:20PM +0530, Umang Jain wrote:\n>> On 8/25/22 8:39 PM, Laurent Pinchart wrote:\n>>> On Thu, Aug 25, 2022 at 02:05:57AM +0300, Laurent Pinchart via libcamera-devel wrote:\n>>>> On Wed, Aug 24, 2022 at 09:54:21PM +0530, Umang Jain via libcamera-devel wrote:\n>>>>> The V4L2_COLORSPACE_SRGB colorspace maps to ColorSpace::Srgb.\n>>>>> This is wrong as the V4L2_COLORSPACE_SRGB is ill-defined (defines\n>>>>> Rec 601 Y'CbCr encoding and limited range) in the kernel.\n>>>>>\n>>>>> The RGB pixel formats should not use any Y'CbCr encoding and is always\n>>>>> full range. Adjust the colorspace before reporting back to the\n>>>>> userspace in such a situation.\n>>>>>\n>>>>> Moving forwards, the ColorSpace::Srgb will be defined in the true sense\n>>>>> for RGB pixel formats.\n>>>> Mentioning sRGB here is a bit confusing I think. Let's focus on what\n>>>> this patch does (and this will also help making sure I understood this\n>>>> correctly):\n>>>>\n>>>> V4L2 has no \"none\" YCbCr encoding, and thus reports an encoding for all\n>>>> formats, including RGB and raw formats. This causes the libcamera\n>>>> ColorSpace to report incorrect encodings for non-YUV formats. Fix it by\n>>>> overriding the encoding reported by the kernel to YCbCrEncoding::None\n>>>> for non-YUV pixel formats and media bus formats.\n>>>>\n>>>> Similarly, override the quantization range of non-YUV formats to full\n>>>> range, as limited range isn't used for RGB and raw formats.\n>>>>\n>>>>> Signed-off-by: Umang Jain \n>>>>> ---\n>>>>> include/libcamera/internal/v4l2_device.h | 5 ++++-\n>>>>> src/libcamera/v4l2_device.cpp | 19 +++++++++++++++----\n>>>>> src/libcamera/v4l2_subdevice.cpp | 10 ++++++++--\n>>>>> src/libcamera/v4l2_videodevice.cpp | 12 ++++++++----\n>>>>> 4 files changed, 35 insertions(+), 11 deletions(-)\n>>>>>\n>>>>> diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h\n>>>>> index a52a5f2c..5ae2ef8a 100644\n>>>>> --- a/include/libcamera/internal/v4l2_device.h\n>>>>> +++ b/include/libcamera/internal/v4l2_device.h\n>>>>> @@ -22,6 +22,8 @@\n>>>>> #include \n>>>>> #include \n>>>>> \n>>>>> +#include \"libcamera/internal/formats.h\"\n>>>>> +\n>>>>> namespace libcamera {\n>>>>> \n>>>>> class EventNotifier;\n>>>>> @@ -59,7 +61,8 @@ protected:\n>>>>> \tint fd() const { return fd_.get(); }\n>>>>> \n>>>>> \ttemplate\n>>>>> -\tstatic std::optional toColorSpace(const T &v4l2Format);\n>>>>> +\tstatic std::optional toColorSpace(const T &v4l2Format,\n>>>>> +\t\t\t\t\t\t const PixelFormatInfo::ColourEncoding &colourEncoding);\n>>>> \t\t\t\t\t\t PixelFormatInfo::ColourEncoding colourEncoding);\n>>>> Same below.\n>>>>\n>>>>> \n>>>>> \ttemplate\n>>>>> \tstatic int fromColorSpace(const std::optional &colorSpace, T &v4l2Format);\n>>>>> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n>>>>> index b22a981f..1fb08b9d 100644\n>>>>> --- a/src/libcamera/v4l2_device.cpp\n>>>>> +++ b/src/libcamera/v4l2_device.cpp\n>>>>> @@ -24,6 +24,7 @@\n>>>>> #include \n>>>>> #include \n>>>>> \n>>>>> +#include \"libcamera/internal/formats.h\"\n>>>>> #include \"libcamera/internal/sysfs.h\"\n>>>>> \n>>>>> /**\n>>>>> @@ -816,7 +817,8 @@ static const std::map rangeToV4l2 = {\n>>>>> * \\retval std::nullopt One or more V4L2 color space fields were not recognised\n>>>>> */\n>>> The documentation needs to be updated with the new parameter.\n>>>\n>>>>> template\n>>>>> -std::optional V4L2Device::toColorSpace(const T &v4l2Format)\n>>>>> +std::optional V4L2Device::toColorSpace(const T &v4l2Format,\n>>>>> +\t\t\t\t\t\t const PixelFormatInfo::ColourEncoding &colourEncoding)\n>>>>> {\n>>>>> \tauto itColor = v4l2ToColorSpace.find(v4l2Format.colorspace);\n>>>>> \tif (itColor == v4l2ToColorSpace.end())\n>>>>> @@ -839,6 +841,9 @@ std::optional V4L2Device::toColorSpace(const T &v4l2Format)\n>>>>> \t\t\treturn std::nullopt;\n>>>>> \n>>>>> \t\tcolorSpace.ycbcrEncoding = itYcbcrEncoding->second;\n>>>>> +\n>>>> I'd add a comment here, or we'll forget why.\n>>>>\n>>>> \t\t/*\n>>>> \t\t * V4L2 has no \"none\" encoding, override the value returned by\n>>>> \t\t * the kernel for non-YUV formats as YCbCr encoding isn't\n>>>> \t\t * applicable in that case..\n>>>> \t\t */\n>>>>\n>>>>> +\t\tif (colourEncoding == PixelFormatInfo::ColourEncodingRGB)\n>>>> Shouldn't this be\n>>>>\n>>>> \t\tif (colourEncoding != PixelFormatInfo::ColourEncodingYUV)\n>>>>\n>>>> ? Raw formats have no YCbCr encoding either. The subject line would need\n>>>> to be updated accordingly, I'd write \"Adjust colorspace based on pixel\n>>>> format\".\n>>>>\n>>>>> +\t\t\tcolorSpace.ycbcrEncoding = ColorSpace::YcbcrEncoding::None;\n>>>>> \t}\n>>>>> \n>>>>> \tif (v4l2Format.quantization != V4L2_QUANTIZATION_DEFAULT) {\n>>>>> @@ -847,14 +852,20 @@ std::optional V4L2Device::toColorSpace(const T &v4l2Format)\n>>>>> \t\t\treturn std::nullopt;\n>>>>> \n>>>>> \t\tcolorSpace.range = itRange->second;\n>>>>> +\n>>>>> +\t\tif (colourEncoding == PixelFormatInfo::ColourEncodingRGB)\n>>>> Same here.\n>>>>\n>>>>> +\t\t\tcolorSpace.range = ColorSpace::Range::Full;\n>>>>> \t}\n>>>>> \n>>>>> \treturn colorSpace;\n>>>>> }\n>>>>> \n>>>>> -template std::optional V4L2Device::toColorSpace(const struct v4l2_pix_format &);\n>>>>> -template std::optional V4L2Device::toColorSpace(const struct v4l2_pix_format_mplane &);\n>>>>> -template std::optional V4L2Device::toColorSpace(const struct v4l2_mbus_framefmt &);\n>>>>> +template std::optional V4L2Device::toColorSpace(const struct v4l2_pix_format &,\n>>>>> +\t\t\t\t\t\t\t const PixelFormatInfo::ColourEncoding &);\n>>>>> +template std::optional V4L2Device::toColorSpace(const struct v4l2_pix_format_mplane &,\n>>>>> +\t\t\t\t\t\t\t const PixelFormatInfo::ColourEncoding &);\n>>>>> +template std::optional V4L2Device::toColorSpace(const struct v4l2_mbus_framefmt &,\n>>>>> +\t\t\t\t\t\t\t const PixelFormatInfo::ColourEncoding &);\n>>>>> \n>>>>> /**\n>>>>> * \\brief Fill in the color space fields of a V4L2 format from a ColorSpace\n>>>>> diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp\n>>>>> index ab74b9f0..a52c414f 100644\n>>>>> --- a/src/libcamera/v4l2_subdevice.cpp\n>>>>> +++ b/src/libcamera/v4l2_subdevice.cpp\n>>>>> @@ -502,7 +502,10 @@ int V4L2Subdevice::getFormat(unsigned int pad, V4L2SubdeviceFormat *format,\n>>>>> \tformat->size.width = subdevFmt.format.width;\n>>>>> \tformat->size.height = subdevFmt.format.height;\n>>>>> \tformat->mbus_code = subdevFmt.format.code;\n>>>>> -\tformat->colorSpace = toColorSpace(subdevFmt.format);\n>>>>> +\tauto iter = formatInfoMap.find(format->mbus_code);\n>>>>> +\tif (iter == formatInfoMap.end())\n>>>>> +\t\treturn -EINVAL;\n>>>> That seems a bit harsh. I'm thinking about the simple pipeline handler\n>>>> in particular, which will propagate media bus formats through the\n>>>> pipeline without caring much about the format itself. Would it better to\n>>>> instead pick a default ?\n>>>>\n>>>>> +\tformat->colorSpace = toColorSpace(subdevFmt.format, iter->second.colourEncoding);\n>>>>> \n>>>>> \treturn 0;\n>>>>> }\n>>>>> @@ -548,7 +551,10 @@ int V4L2Subdevice::setFormat(unsigned int pad, V4L2SubdeviceFormat *format,\n>>>>> \tformat->size.width = subdevFmt.format.width;\n>>>>> \tformat->size.height = subdevFmt.format.height;\n>>>>> \tformat->mbus_code = subdevFmt.format.code;\n>>>>> -\tformat->colorSpace = toColorSpace(subdevFmt.format);\n>>>>> +\tauto iter = formatInfoMap.find(format->mbus_code);\n>>>>> +\tif (iter == formatInfoMap.end())\n>>>>> +\t\treturn -EINVAL;\n>>>> Same here.\n>> Esp in V4l2Subdevice::setFormat() , picking a default and reporting that\n>> back makes me a bit unconformtable ....\n>>\n>> should we chose to fail hard here?\n> For the reasons explained in another reply in this thread, I'd rather\n> add hard failures separately, if we decide it's the way to go. We would\n> then need to consider the impact on different parts of libcamera (simple\n> pipeline handler, metadata formats, ...). Even an error/warning message\n> is something I would introduce in a separate patch on top, to be able to\n> test it thoroughly without blocking the rest of the series.\n\nOk.\n>\n>> diff --git a/src/libcamera/v4l2_subdevice.cpp\n>> b/src/libcamera/v4l2_subdevice.cpp\n>> index 70f179c2..d415a738 100644\n>> --- a/src/libcamera/v4l2_subdevice.cpp\n>> +++ b/src/libcamera/v4l2_subdevice.cpp\n>> @@ -502,10 +502,19 @@ int V4L2Subdevice::getFormat(unsigned int pad, V4L2SubdeviceFormat *format,\n>>\n>>         format->size.width = subdevFmt.format.width;\n>>         format->size.height = subdevFmt.format.height;\n>> +\n>> +       auto iter = formatInfoMap.find(subdevFmt.format.code);\n>> +       if (iter == formatInfoMap.end()) {\n>> +               /* Pick a default format to continue */\n>> +               uint32_t defCode = MEDIA_BUS_FMT_RGB888_1X24;\n>> +               iter = formatInfoMap.find(defCode);\n> You can avoid the lookup there and just use the RGB encoding:\n>\n> \tPixelFormatInfo::ColourEncoding colourEncoding;\n> \tauto iter = formatInfoMap.find(subdevFmt.format.code);\n> \tif (iter != formatInfoMap.end()) {\n> \t\tcolourEncoding = iter->second.colourEncoding;\n> \t} else {\n> \t\tLOG(V4L2, Warning)\n> \t\t\t<< \"Unknown subdev format \"\n> \t\t\t<< utils::hex(subdevFmt.format.code, 4)\n> \t\t\t<< \", defaulting to RGB encoding\";\n>\n> \t\tcolourEncoding = PixelFormatInfo::ColourEncodingRGB;\n> \t}\n>\n>> +               LOG(V4L2, Error)\n> Make it a warning.\n>\n>> +                        << \"Mapping for subdev format \" << subdevFmt.format.code\n> Print the code in hex.\n>\n>> +                        << \" is missing, falling back to \" << iter->second.name;\n>> +\n>> +               subdevFmt.format.code = defCode;\n> Why do you override the media bus code returned by the driver ?\n\nIt wasn't explicit in the previous conversation that we need to pick a \ndefault colorspace /only/\n\nMy thought process went on the lines \"we need to pick a default media \nbus format\"  (if the returned code mapping is missing..)\n>\n> All these comments apply below.\n>\n>> +       }\n>>         format->mbus_code = subdevFmt.format.code;\n>> -       auto iter = formatInfoMap.find(format->mbus_code);\n>> -       if (iter == formatInfoMap.end())\n>> -               return -EINVAL;\n>>         format->colorSpace = toColorSpace(subdevFmt.format, iter->second.colourEncoding);\n>>\n>>         return 0;\n>> @@ -551,10 +560,20 @@ int V4L2Subdevice::setFormat(unsigned int pad, V4L2SubdeviceFormat *format,\n>>\n>>         format->size.width = subdevFmt.format.width;\n>>         format->size.height = subdevFmt.format.height;\n>> +\n>> +       auto iter = formatInfoMap.find(subdevFmt.format.code);\n>> +       if (iter == formatInfoMap.end()) {\n>> +               /* Pick a default format to continue */\n>> +               uint32_t defCode = MEDIA_BUS_FMT_RGB888_1X24;\n>> +               iter = formatInfoMap.find(defCode);\n>> +\n>> +               LOG(V4L2, Error)\n>> +                       << \"Mapping for subdev format \" << subdevFmt.format.code\n>> +                       << \" is missing, falling back to \" << iter->second.name;\n>> +\n>> +               subdevFmt.format.code = defCode;\n>> +       }\n>>         format->mbus_code = subdevFmt.format.code;\n>> -       auto iter = formatInfoMap.find(format->mbus_code);\n>> -       if (iter == formatInfoMap.end())\n>> -               return -EINVAL;\n>>         format->colorSpace = toColorSpace(subdevFmt.format, iter->second.colourEncoding);\n>>\n>>         return 0;\n>>\n>>>>> +\tformat->colorSpace = toColorSpace(subdevFmt.format, iter->second.colourEncoding);\n>>>>> \n>>>>> \treturn 0;\n>>>>> }\n>>>>> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n>>>>> index 5a2d0e5b..0e3f5436 100644\n>>>>> --- a/src/libcamera/v4l2_videodevice.cpp\n>>>>> +++ b/src/libcamera/v4l2_videodevice.cpp\n>>>>> @@ -931,7 +931,8 @@ int V4L2VideoDevice::getFormatMultiplane(V4L2DeviceFormat *format)\n>>>> I'm tempted to add a helper just above this function:\n>>>>\n>>>> namespace {\n>>>>\n>>>> std::optional toColorSpace(const struct v4l2_pix_format_mplane &pix)\n>>>> {\n>>>> \tV4L2PixelFormat fourcc{ pix.pixelformat };\n>>>> \treturn toColorSpace(pix, PixelFormatInfo::info(fourcc).colourEncoding);\n>>>> }\n>>>>\n>>>> } /* namespace */\n>>>>\n>>>> to make the color below a bit nicer (it would actually not need to be\n>>>> changed at all). Up to you.\n>>>>\n>>>>> \tformat->size.height = pix->height;\n>>>>> \tformat->fourcc = V4L2PixelFormat(pix->pixelformat);\n>>>>> \tformat->planesCount = pix->num_planes;\n>>>>> -\tformat->colorSpace = toColorSpace(*pix);\n>>>>> +\tformat->colorSpace =\n>>>>> +\t\ttoColorSpace(*pix, PixelFormatInfo::info(format->fourcc).colourEncoding);\n>>>>> \n>>>>> \tfor (unsigned int i = 0; i < format->planesCount; ++i) {\n>>>>> \t\tformat->planes[i].bpl = pix->plane_fmt[i].bytesperline;\n>>>>> @@ -987,7 +988,8 @@ 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 = toColorSpace(*pix);\n>>>>> +\tformat->colorSpace =\n>>>>> +\t\ttoColorSpace(*pix, PixelFormatInfo::info(format->fourcc).colourEncoding);\n>>>>> \n>>>>> \treturn 0;\n>>>>> }\n>>>>> @@ -1011,7 +1013,8 @@ int V4L2VideoDevice::getFormatSingleplane(V4L2DeviceFormat *format)\n>>>>> \tformat->planesCount = 1;\n>>>>> \tformat->planes[0].bpl = pix->bytesperline;\n>>>>> \tformat->planes[0].size = pix->sizeimage;\n>>>>> -\tformat->colorSpace = toColorSpace(*pix);\n>>>>> +\tformat->colorSpace =\n>>>>> +\t\ttoColorSpace(*pix, PixelFormatInfo::info(format->fourcc).colourEncoding);\n>>>>> \n>>>>> \treturn 0;\n>>>>> }\n>>>>> @@ -1053,7 +1056,8 @@ 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 = toColorSpace(*pix);\n>>>>> +\tformat->colorSpace =\n>>>>> +\t\ttoColorSpace(*pix, PixelFormatInfo::info(format->fourcc).colourEncoding);\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 5FC2FC0DA4\n\tfor ;\n\tFri, 26 Aug 2022 17:33:12 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7D66361FC0;\n\tFri, 26 Aug 2022 19:33:11 +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 6186061FA2\n\tfor ;\n\tFri, 26 Aug 2022 19:33:09 +0200 (CEST)","from [IPV6:2401:4900:1f3f:806e:6647:8e5c:f441:ca9a] (unknown\n\t[IPv6:2401:4900:1f3f:806e:6647:8e5c:f441:ca9a])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1F6C62B3;\n\tFri, 26 Aug 2022 19:33:07 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1661535191;\n\tbh=tiLTWxOkTcmM35qUEid/xBwD0nbvjWRbfgst8z4diOg=;\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=4KEi37wKM3udycef1ROV4Jd3j3OqjeuCgIukp9lnjaXFNbFbbQ/gFTe+zV0zzRoF1\n\tEy1DBhP1mZqr7B0M5gKXGqnVlrVQ67D0FkfcfCyS3RZFdsdiXEy0DD03v3cqNxjWo8\n\twIF7FLbj32OVRMLKuVePIWz3yQ16ofHt6Y6JcHgO09+PqiVdxQbHHuf0pvXRDRGZiY\n\ti2azvwtb2kO4/kj+N2U9xY2QJwarkuSh8dtMOOKqDghYmdXoPv+kjtY+HGR/ro/QBn\n\t+1TtK1s1AQWwcHsa+p/LEqGPVl1VPkYJ+mCL+bR/Ld/95JpU3y8jaYghuxS3rwSFFL\n\tWTeM9JrEgS/CA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1661535188;\n\tbh=tiLTWxOkTcmM35qUEid/xBwD0nbvjWRbfgst8z4diOg=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=MP1YCnSo0Y/HA1gO9IEy3NUuNEKU9kthtBGRNWJaRQVtNVIX9pcwSMrabHan3yguB\n\tZqGwNJwgmmpn9wCELBU6+JK6fdpwWcHX0hJk5qoMs3HZbjx6pgILalQIK+PU3NUPZR\n\tot05DvPCA8gdJEZ97RdZgaIqE6siNfdAduuSpGD8="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"MP1YCnSo\"; dkim-atps=neutral","Message-ID":"<99c36566-f1ae-6808-e3f0-c6281dd3f6d6@ideasonboard.com>","Date":"Fri, 26 Aug 2022 23:03:03 +0530","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.12.0","Content-Language":"en-US","To":"Laurent Pinchart ","References":"<20220824162425.71087-1-umang.jain@ideasonboard.com>\n\t<20220824162425.71087-3-umang.jain@ideasonboard.com>\n\t\n\t\n\t\n\t","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v2 2/6] libcamera: v4l2_device: Adjust\n\tcolorspace if pixel format is RGB","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":"Umang Jain via libcamera-devel ","Reply-To":"Umang Jain ","Cc":"libcamera-devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24800,"web_url":"https://patchwork.libcamera.org/comment/24800/","msgid":"","date":"2022-08-26T20:40:45","subject":"Re: [libcamera-devel] [PATCH v2 2/6] libcamera: v4l2_device: Adjust\n\tcolorspace if pixel format is RGB","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Umang,\n\nOn Fri, Aug 26, 2022 at 11:03:03PM +0530, Umang Jain wrote:\n> On 8/26/22 7:06 PM, Laurent Pinchart wrote:\n> > On Fri, Aug 26, 2022 at 01:56:20PM +0530, Umang Jain wrote:\n> >> On 8/25/22 8:39 PM, Laurent Pinchart wrote:\n> >>> On Thu, Aug 25, 2022 at 02:05:57AM +0300, Laurent Pinchart via libcamera-devel wrote:\n> >>>> On Wed, Aug 24, 2022 at 09:54:21PM +0530, Umang Jain via libcamera-devel wrote:\n> >>>>> The V4L2_COLORSPACE_SRGB colorspace maps to ColorSpace::Srgb.\n> >>>>> This is wrong as the V4L2_COLORSPACE_SRGB is ill-defined (defines\n> >>>>> Rec 601 Y'CbCr encoding and limited range) in the kernel.\n> >>>>>\n> >>>>> The RGB pixel formats should not use any Y'CbCr encoding and is always\n> >>>>> full range. Adjust the colorspace before reporting back to the\n> >>>>> userspace in such a situation.\n> >>>>>\n> >>>>> Moving forwards, the ColorSpace::Srgb will be defined in the true sense\n> >>>>> for RGB pixel formats.\n> >>>> Mentioning sRGB here is a bit confusing I think. Let's focus on what\n> >>>> this patch does (and this will also help making sure I understood this\n> >>>> correctly):\n> >>>>\n> >>>> V4L2 has no \"none\" YCbCr encoding, and thus reports an encoding for all\n> >>>> formats, including RGB and raw formats. This causes the libcamera\n> >>>> ColorSpace to report incorrect encodings for non-YUV formats. Fix it by\n> >>>> overriding the encoding reported by the kernel to YCbCrEncoding::None\n> >>>> for non-YUV pixel formats and media bus formats.\n> >>>>\n> >>>> Similarly, override the quantization range of non-YUV formats to full\n> >>>> range, as limited range isn't used for RGB and raw formats.\n> >>>>\n> >>>>> Signed-off-by: Umang Jain \n> >>>>> ---\n> >>>>> include/libcamera/internal/v4l2_device.h | 5 ++++-\n> >>>>> src/libcamera/v4l2_device.cpp | 19 +++++++++++++++----\n> >>>>> src/libcamera/v4l2_subdevice.cpp | 10 ++++++++--\n> >>>>> src/libcamera/v4l2_videodevice.cpp | 12 ++++++++----\n> >>>>> 4 files changed, 35 insertions(+), 11 deletions(-)\n> >>>>>\n> >>>>> diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h\n> >>>>> index a52a5f2c..5ae2ef8a 100644\n> >>>>> --- a/include/libcamera/internal/v4l2_device.h\n> >>>>> +++ b/include/libcamera/internal/v4l2_device.h\n> >>>>> @@ -22,6 +22,8 @@\n> >>>>> #include \n> >>>>> #include \n> >>>>> \n> >>>>> +#include \"libcamera/internal/formats.h\"\n> >>>>> +\n> >>>>> namespace libcamera {\n> >>>>> \n> >>>>> class EventNotifier;\n> >>>>> @@ -59,7 +61,8 @@ protected:\n> >>>>> \tint fd() const { return fd_.get(); }\n> >>>>> \n> >>>>> \ttemplate\n> >>>>> -\tstatic std::optional toColorSpace(const T &v4l2Format);\n> >>>>> +\tstatic std::optional toColorSpace(const T &v4l2Format,\n> >>>>> +\t\t\t\t\t\t const PixelFormatInfo::ColourEncoding &colourEncoding);\n> >>>> \t\t\t\t\t\t PixelFormatInfo::ColourEncoding colourEncoding);\n> >>>> Same below.\n> >>>>\n> >>>>> \n> >>>>> \ttemplate\n> >>>>> \tstatic int fromColorSpace(const std::optional &colorSpace, T &v4l2Format);\n> >>>>> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> >>>>> index b22a981f..1fb08b9d 100644\n> >>>>> --- a/src/libcamera/v4l2_device.cpp\n> >>>>> +++ b/src/libcamera/v4l2_device.cpp\n> >>>>> @@ -24,6 +24,7 @@\n> >>>>> #include \n> >>>>> #include \n> >>>>> \n> >>>>> +#include \"libcamera/internal/formats.h\"\n> >>>>> #include \"libcamera/internal/sysfs.h\"\n> >>>>> \n> >>>>> /**\n> >>>>> @@ -816,7 +817,8 @@ static const std::map rangeToV4l2 = {\n> >>>>> * \\retval std::nullopt One or more V4L2 color space fields were not recognised\n> >>>>> */\n> >>> The documentation needs to be updated with the new parameter.\n> >>>\n> >>>>> template\n> >>>>> -std::optional V4L2Device::toColorSpace(const T &v4l2Format)\n> >>>>> +std::optional V4L2Device::toColorSpace(const T &v4l2Format,\n> >>>>> +\t\t\t\t\t\t const PixelFormatInfo::ColourEncoding &colourEncoding)\n> >>>>> {\n> >>>>> \tauto itColor = v4l2ToColorSpace.find(v4l2Format.colorspace);\n> >>>>> \tif (itColor == v4l2ToColorSpace.end())\n> >>>>> @@ -839,6 +841,9 @@ std::optional V4L2Device::toColorSpace(const T &v4l2Format)\n> >>>>> \t\t\treturn std::nullopt;\n> >>>>> \n> >>>>> \t\tcolorSpace.ycbcrEncoding = itYcbcrEncoding->second;\n> >>>>> +\n> >>>> I'd add a comment here, or we'll forget why.\n> >>>>\n> >>>> \t\t/*\n> >>>> \t\t * V4L2 has no \"none\" encoding, override the value returned by\n> >>>> \t\t * the kernel for non-YUV formats as YCbCr encoding isn't\n> >>>> \t\t * applicable in that case..\n> >>>> \t\t */\n> >>>>\n> >>>>> +\t\tif (colourEncoding == PixelFormatInfo::ColourEncodingRGB)\n> >>>> Shouldn't this be\n> >>>>\n> >>>> \t\tif (colourEncoding != PixelFormatInfo::ColourEncodingYUV)\n> >>>>\n> >>>> ? Raw formats have no YCbCr encoding either. The subject line would need\n> >>>> to be updated accordingly, I'd write \"Adjust colorspace based on pixel\n> >>>> format\".\n> >>>>\n> >>>>> +\t\t\tcolorSpace.ycbcrEncoding = ColorSpace::YcbcrEncoding::None;\n> >>>>> \t}\n> >>>>> \n> >>>>> \tif (v4l2Format.quantization != V4L2_QUANTIZATION_DEFAULT) {\n> >>>>> @@ -847,14 +852,20 @@ std::optional V4L2Device::toColorSpace(const T &v4l2Format)\n> >>>>> \t\t\treturn std::nullopt;\n> >>>>> \n> >>>>> \t\tcolorSpace.range = itRange->second;\n> >>>>> +\n> >>>>> +\t\tif (colourEncoding == PixelFormatInfo::ColourEncodingRGB)\n> >>>> Same here.\n> >>>>\n> >>>>> +\t\t\tcolorSpace.range = ColorSpace::Range::Full;\n> >>>>> \t}\n> >>>>> \n> >>>>> \treturn colorSpace;\n> >>>>> }\n> >>>>> \n> >>>>> -template std::optional V4L2Device::toColorSpace(const struct v4l2_pix_format &);\n> >>>>> -template std::optional V4L2Device::toColorSpace(const struct v4l2_pix_format_mplane &);\n> >>>>> -template std::optional V4L2Device::toColorSpace(const struct v4l2_mbus_framefmt &);\n> >>>>> +template std::optional V4L2Device::toColorSpace(const struct v4l2_pix_format &,\n> >>>>> +\t\t\t\t\t\t\t const PixelFormatInfo::ColourEncoding &);\n> >>>>> +template std::optional V4L2Device::toColorSpace(const struct v4l2_pix_format_mplane &,\n> >>>>> +\t\t\t\t\t\t\t const PixelFormatInfo::ColourEncoding &);\n> >>>>> +template std::optional V4L2Device::toColorSpace(const struct v4l2_mbus_framefmt &,\n> >>>>> +\t\t\t\t\t\t\t const PixelFormatInfo::ColourEncoding &);\n> >>>>> \n> >>>>> /**\n> >>>>> * \\brief Fill in the color space fields of a V4L2 format from a ColorSpace\n> >>>>> diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp\n> >>>>> index ab74b9f0..a52c414f 100644\n> >>>>> --- a/src/libcamera/v4l2_subdevice.cpp\n> >>>>> +++ b/src/libcamera/v4l2_subdevice.cpp\n> >>>>> @@ -502,7 +502,10 @@ int V4L2Subdevice::getFormat(unsigned int pad, V4L2SubdeviceFormat *format,\n> >>>>> \tformat->size.width = subdevFmt.format.width;\n> >>>>> \tformat->size.height = subdevFmt.format.height;\n> >>>>> \tformat->mbus_code = subdevFmt.format.code;\n> >>>>> -\tformat->colorSpace = toColorSpace(subdevFmt.format);\n> >>>>> +\tauto iter = formatInfoMap.find(format->mbus_code);\n> >>>>> +\tif (iter == formatInfoMap.end())\n> >>>>> +\t\treturn -EINVAL;\n> >>>> That seems a bit harsh. I'm thinking about the simple pipeline handler\n> >>>> in particular, which will propagate media bus formats through the\n> >>>> pipeline without caring much about the format itself. Would it better to\n> >>>> instead pick a default ?\n> >>>>\n> >>>>> +\tformat->colorSpace = toColorSpace(subdevFmt.format, iter->second.colourEncoding);\n> >>>>> \n> >>>>> \treturn 0;\n> >>>>> }\n> >>>>> @@ -548,7 +551,10 @@ int V4L2Subdevice::setFormat(unsigned int pad, V4L2SubdeviceFormat *format,\n> >>>>> \tformat->size.width = subdevFmt.format.width;\n> >>>>> \tformat->size.height = subdevFmt.format.height;\n> >>>>> \tformat->mbus_code = subdevFmt.format.code;\n> >>>>> -\tformat->colorSpace = toColorSpace(subdevFmt.format);\n> >>>>> +\tauto iter = formatInfoMap.find(format->mbus_code);\n> >>>>> +\tif (iter == formatInfoMap.end())\n> >>>>> +\t\treturn -EINVAL;\n> >>>> Same here.\n> >> Esp in V4l2Subdevice::setFormat() , picking a default and reporting that\n> >> back makes me a bit unconformtable ....\n> >>\n> >> should we chose to fail hard here?\n> > For the reasons explained in another reply in this thread, I'd rather\n> > add hard failures separately, if we decide it's the way to go. We would\n> > then need to consider the impact on different parts of libcamera (simple\n> > pipeline handler, metadata formats, ...). Even an error/warning message\n> > is something I would introduce in a separate patch on top, to be able to\n> > test it thoroughly without blocking the rest of the series.\n> \n> Ok.\n> >\n> >> diff --git a/src/libcamera/v4l2_subdevice.cpp\n> >> b/src/libcamera/v4l2_subdevice.cpp\n> >> index 70f179c2..d415a738 100644\n> >> --- a/src/libcamera/v4l2_subdevice.cpp\n> >> +++ b/src/libcamera/v4l2_subdevice.cpp\n> >> @@ -502,10 +502,19 @@ int V4L2Subdevice::getFormat(unsigned int pad, V4L2SubdeviceFormat *format,\n> >>\n> >>         format->size.width = subdevFmt.format.width;\n> >>         format->size.height = subdevFmt.format.height;\n> >> +\n> >> +       auto iter = formatInfoMap.find(subdevFmt.format.code);\n> >> +       if (iter == formatInfoMap.end()) {\n> >> +               /* Pick a default format to continue */\n> >> +               uint32_t defCode = MEDIA_BUS_FMT_RGB888_1X24;\n> >> +               iter = formatInfoMap.find(defCode);\n> > You can avoid the lookup there and just use the RGB encoding:\n> >\n> > \tPixelFormatInfo::ColourEncoding colourEncoding;\n> > \tauto iter = formatInfoMap.find(subdevFmt.format.code);\n> > \tif (iter != formatInfoMap.end()) {\n> > \t\tcolourEncoding = iter->second.colourEncoding;\n> > \t} else {\n> > \t\tLOG(V4L2, Warning)\n> > \t\t\t<< \"Unknown subdev format \"\n> > \t\t\t<< utils::hex(subdevFmt.format.code, 4)\n> > \t\t\t<< \", defaulting to RGB encoding\";\n> >\n> > \t\tcolourEncoding = PixelFormatInfo::ColourEncodingRGB;\n> > \t}\n> >\n> >> +               LOG(V4L2, Error)\n> > Make it a warning.\n> >\n> >> +                        << \"Mapping for subdev format \" << subdevFmt.format.code\n> > Print the code in hex.\n> >\n> >> +                        << \" is missing, falling back to \" << iter->second.name;\n> >> +\n> >> +               subdevFmt.format.code = defCode;\n> > Why do you override the media bus code returned by the driver ?\n> \n> It wasn't explicit in the previous conversation that we need to pick a \n> default colorspace /only/\n> \n> My thought process went on the lines \"we need to pick a default media \n> bus format\"  (if the returned code mapping is missing..)\n\nI should have been clearer, sorry. A default color space should be fine\nI think, because we need to do something, and we can't do better, but\noverriding an unknown format will lead to possible problems.\n\n> > All these comments apply below.\n> >\n> >> +       }\n> >>         format->mbus_code = subdevFmt.format.code;\n> >> -       auto iter = formatInfoMap.find(format->mbus_code);\n> >> -       if (iter == formatInfoMap.end())\n> >> -               return -EINVAL;\n> >>         format->colorSpace = toColorSpace(subdevFmt.format, iter->second.colourEncoding);\n> >>\n> >>         return 0;\n> >> @@ -551,10 +560,20 @@ int V4L2Subdevice::setFormat(unsigned int pad, V4L2SubdeviceFormat *format,\n> >>\n> >>         format->size.width = subdevFmt.format.width;\n> >>         format->size.height = subdevFmt.format.height;\n> >> +\n> >> +       auto iter = formatInfoMap.find(subdevFmt.format.code);\n> >> +       if (iter == formatInfoMap.end()) {\n> >> +               /* Pick a default format to continue */\n> >> +               uint32_t defCode = MEDIA_BUS_FMT_RGB888_1X24;\n> >> +               iter = formatInfoMap.find(defCode);\n> >> +\n> >> +               LOG(V4L2, Error)\n> >> +                       << \"Mapping for subdev format \" << subdevFmt.format.code\n> >> +                       << \" is missing, falling back to \" << iter->second.name;\n> >> +\n> >> +               subdevFmt.format.code = defCode;\n> >> +       }\n> >>         format->mbus_code = subdevFmt.format.code;\n> >> -       auto iter = formatInfoMap.find(format->mbus_code);\n> >> -       if (iter == formatInfoMap.end())\n> >> -               return -EINVAL;\n> >>         format->colorSpace = toColorSpace(subdevFmt.format, iter->second.colourEncoding);\n> >>\n> >>         return 0;\n> >>\n> >>>>> +\tformat->colorSpace = toColorSpace(subdevFmt.format, iter->second.colourEncoding);\n> >>>>> \n> >>>>> \treturn 0;\n> >>>>> }\n> >>>>> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> >>>>> index 5a2d0e5b..0e3f5436 100644\n> >>>>> --- a/src/libcamera/v4l2_videodevice.cpp\n> >>>>> +++ b/src/libcamera/v4l2_videodevice.cpp\n> >>>>> @@ -931,7 +931,8 @@ int V4L2VideoDevice::getFormatMultiplane(V4L2DeviceFormat *format)\n> >>>> I'm tempted to add a helper just above this function:\n> >>>>\n> >>>> namespace {\n> >>>>\n> >>>> std::optional toColorSpace(const struct v4l2_pix_format_mplane &pix)\n> >>>> {\n> >>>> \tV4L2PixelFormat fourcc{ pix.pixelformat };\n> >>>> \treturn toColorSpace(pix, PixelFormatInfo::info(fourcc).colourEncoding);\n> >>>> }\n> >>>>\n> >>>> } /* namespace */\n> >>>>\n> >>>> to make the color below a bit nicer (it would actually not need to be\n> >>>> changed at all). Up to you.\n> >>>>\n> >>>>> \tformat->size.height = pix->height;\n> >>>>> \tformat->fourcc = V4L2PixelFormat(pix->pixelformat);\n> >>>>> \tformat->planesCount = pix->num_planes;\n> >>>>> -\tformat->colorSpace = toColorSpace(*pix);\n> >>>>> +\tformat->colorSpace =\n> >>>>> +\t\ttoColorSpace(*pix, PixelFormatInfo::info(format->fourcc).colourEncoding);\n> >>>>> \n> >>>>> \tfor (unsigned int i = 0; i < format->planesCount; ++i) {\n> >>>>> \t\tformat->planes[i].bpl = pix->plane_fmt[i].bytesperline;\n> >>>>> @@ -987,7 +988,8 @@ 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 = toColorSpace(*pix);\n> >>>>> +\tformat->colorSpace =\n> >>>>> +\t\ttoColorSpace(*pix, PixelFormatInfo::info(format->fourcc).colourEncoding);\n> >>>>> \n> >>>>> \treturn 0;\n> >>>>> }\n> >>>>> @@ -1011,7 +1013,8 @@ int V4L2VideoDevice::getFormatSingleplane(V4L2DeviceFormat *format)\n> >>>>> \tformat->planesCount = 1;\n> >>>>> \tformat->planes[0].bpl = pix->bytesperline;\n> >>>>> \tformat->planes[0].size = pix->sizeimage;\n> >>>>> -\tformat->colorSpace = toColorSpace(*pix);\n> >>>>> +\tformat->colorSpace =\n> >>>>> +\t\ttoColorSpace(*pix, PixelFormatInfo::info(format->fourcc).colourEncoding);\n> >>>>> \n> >>>>> \treturn 0;\n> >>>>> }\n> >>>>> @@ -1053,7 +1056,8 @@ 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 = toColorSpace(*pix);\n> >>>>> +\tformat->colorSpace =\n> >>>>> +\t\ttoColorSpace(*pix, PixelFormatInfo::info(format->fourcc).colourEncoding);\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 37AEEC0DA4\n\tfor ;\n\tFri, 26 Aug 2022 20:40:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8A88C61FC0;\n\tFri, 26 Aug 2022 22:40:55 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BE66561FA2\n\tfor ;\n\tFri, 26 Aug 2022 22:40:53 +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 00F982B3;\n\tFri, 26 Aug 2022 22:40:52 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1661546455;\n\tbh=XuN9yfKCxUDZ8K2MieGc0h9xqLhNT7g2I0elhmiyDH0=;\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=tc253v6VtRyX7vDhM44oJAv8owLXfneY0VONYjukMXT40cI/0cEEV4mZxHl+UVB3M\n\t+nszKIu3ayKeYou6lIHM+rYz3KOsJ7AwUlKt98uGD9d8ueCv2I2Hp9PKUwY7MGRAvB\n\tKJcvgGAvF/kLTWvLY9sk2aXu4UIBgcSIVPM2hpD/kgOMFn5rIzCsCed4FLSGZZAjBh\n\t34i3ZSqFFk4KhlijxO7lARyvRhp5MM/mapjLTM9Rkb7xaUOq6roSnFUQV2BKaDmmT4\n\taRe0/jBmaKaBXgX5l1yudIycB4P0bJ5P+hfJf9yi3tDdD9nJcBqpWWJkH0hGbWV5Zd\n\t66gf9X8QFLvag==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1661546453;\n\tbh=XuN9yfKCxUDZ8K2MieGc0h9xqLhNT7g2I0elhmiyDH0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=CgenQ/ClGPwi3gWuNbuSxn+WjSH0oJihzqpc93fvfoXf6Vh6pE45GBGureAdDvju+\n\tmQwEO5j4oRNbbABF3Y5GCbnr9TiWK59VdrhrbiPOYdG6o4AoKRp4UP/hHU69bQG4mc\n\tHboXPcSakM0XSWAuic2yh30AAaKD7Lbv/1VHdnqM="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"CgenQ/Cl\"; dkim-atps=neutral","Date":"Fri, 26 Aug 2022 23:40:45 +0300","To":"Umang Jain ","Message-ID":"","References":"<20220824162425.71087-1-umang.jain@ideasonboard.com>\n\t<20220824162425.71087-3-umang.jain@ideasonboard.com>\n\t\n\t\n\t\n\t\n\t<99c36566-f1ae-6808-e3f0-c6281dd3f6d6@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<99c36566-f1ae-6808-e3f0-c6281dd3f6d6@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 2/6] libcamera: v4l2_device: Adjust\n\tcolorspace if pixel format is RGB","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]