[{"id":21112,"web_url":"https://patchwork.libcamera.org/comment/21112/","msgid":"<519a4c4d-60c5-34af-b49f-4ee907436f19@ideasonboard.com>","date":"2021-11-23T08:00:59","subject":"Re: [libcamera-devel] [PATCH v6 4/7] libcamera: Support passing\n\tColorSpaces to V4L2 video devices","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi David,\n\nOn 11/18/21 8:49 PM, David Plowman wrote:\n> The ColorSpace from the StreamConfiguration is now handled\n> appropriately in the V4L2VideoDevice.\n>\n> Signed-off-by: David Plowman \n> ---\n> include/libcamera/internal/v4l2_videodevice.h | 2 +\n> src/libcamera/v4l2_videodevice.cpp | 53 +++++++++++++++++--\n> 2 files changed, 51 insertions(+), 4 deletions(-)\n>\n> diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h\n> index a1c458e4..00bc50e7 100644\n> --- a/include/libcamera/internal/v4l2_videodevice.h\n> +++ b/include/libcamera/internal/v4l2_videodevice.h\n> @@ -20,6 +20,7 @@\n> #include \n> #include \n> \n> +#include \n> #include \n> #include \n> #include \n> @@ -167,6 +168,7 @@ public:\n> \n> \tV4L2PixelFormat fourcc;\n> \tSize size;\n> +\tstd::optional colorSpace;\n> \n> \tstd::array planes;\n> \tunsigned int planesCount = 0;\n> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> index 4f04212d..e03ff8b5 100644\n> --- a/src/libcamera/v4l2_videodevice.cpp\n> +++ b/src/libcamera/v4l2_videodevice.cpp\n> @@ -370,17 +370,27 @@ bool V4L2BufferCache::Entry::operator==(const FrameBuffer &buffer) const\n> * \\brief The plane line stride (in bytes)\n> */\n> \n> +/**\n> + * \\var V4L2DeviceFormat::fourcc\n> + * \\brief The fourcc code describing the pixel encoding scheme\n> + *\n> + * The fourcc code, as defined by the V4L2 API with the V4L2_PIX_FMT_* macros,\n> + * that identifies the image format pixel encoding scheme.\n> + */\n> +\n> /**\n> * \\var V4L2DeviceFormat::size\n> * \\brief The image size in pixels\n> */\n> \n> /**\n> - * \\var V4L2DeviceFormat::fourcc\n> - * \\brief The fourcc code describing the pixel encoding scheme\n> + * \\var V4L2DeviceFormat::colorSpace\n> + * \\brief The color space of the pixels\n> *\n> - * The fourcc code, as defined by the V4L2 API with the V4L2_PIX_FMT_* macros,\n> - * that identifies the image format pixel encoding scheme.\n> + * The color space of the image. When setting the format this may be\n> + * unset, in which case the driver gets to use its default color space.\n> + * After being set, this value should contain the color space that the\n> + * was actually used.\n> */\n> \n> /**\n> @@ -879,6 +889,10 @@ int V4L2VideoDevice::getFormatMultiplane(V4L2DeviceFormat *format)\n> \tformat->fourcc = V4L2PixelFormat(pix->pixelformat);\n> \tformat->planesCount = pix->num_planes;\n> \n> +\tformat->colorSpace = toColorSpace(*pix);\n> +\tif (!format->colorSpace)\n> +\t\tLOG(V4L2, Error) << \"Retrieved unrecognised color space\";\n\n\nYou have marked this as an Error, should we be return here too?\n\n> +\n> \tfor (unsigned int i = 0; i < format->planesCount; ++i) {\n> \t\tformat->planes[i].bpl = pix->plane_fmt[i].bytesperline;\n> \t\tformat->planes[i].size = pix->plane_fmt[i].sizeimage;\n> @@ -893,6 +907,9 @@ int V4L2VideoDevice::trySetFormatMultiplane(V4L2DeviceFormat *format, bool set)\n> \tstruct v4l2_pix_format_mplane *pix = &v4l2Format.fmt.pix_mp;\n> \tint ret;\n> \n> +\tif (!format->colorSpace)\n> +\t\tLOG(V4L2, Warning) << \"Setting undefined color space\";\n> +\n> \tv4l2Format.type = bufferType_;\n> \tpix->width = format->size.width;\n> \tpix->height = format->size.height;\n> @@ -900,6 +917,12 @@ int V4L2VideoDevice::trySetFormatMultiplane(V4L2DeviceFormat *format, bool set)\n> \tpix->num_planes = format->planesCount;\n> \tpix->field = V4L2_FIELD_NONE;\n> \n> +\tret = fromColorSpace(format->colorSpace, *pix);\n> +\tif (ret < 0)\n> +\t\tLOG(V4L2, Warning)\n> +\t\t\t<< \"Setting color space unrecognised by V4L2: \"\n> +\t\t\t<< ColorSpace::toString(format->colorSpace);\n> +\n> \tASSERT(pix->num_planes <= std::size(pix->plane_fmt));\n> \n> \tfor (unsigned int i = 0; i < pix->num_planes; ++i) {\n> @@ -928,6 +951,10 @@ int V4L2VideoDevice::trySetFormatMultiplane(V4L2DeviceFormat *format, bool set)\n> \t\tformat->planes[i].size = pix->plane_fmt[i].sizeimage;\n> \t}\n> \n> +\tformat->colorSpace = toColorSpace(*pix);\n> +\tif (!format->colorSpace)\n> +\t\tLOG(V4L2, Warning) << \"Unrecognised color space has been set\";\n> +\n> \treturn 0;\n> }\n> \n> @@ -951,6 +978,10 @@ int V4L2VideoDevice::getFormatSingleplane(V4L2DeviceFormat *format)\n> \tformat->planes[0].bpl = pix->bytesperline;\n> \tformat->planes[0].size = pix->sizeimage;\n> \n> +\tformat->colorSpace = toColorSpace(*pix);\n> +\tif (!format->colorSpace)\n> +\t\tLOG(V4L2, Error) << \"Retrieved unrecognised color space\";\n> +\n> \treturn 0;\n> }\n> \n> @@ -960,12 +991,22 @@ int V4L2VideoDevice::trySetFormatSingleplane(V4L2DeviceFormat *format, bool set)\n> \tstruct v4l2_pix_format *pix = &v4l2Format.fmt.pix;\n> \tint ret;\n> \n> +\tif (!format->colorSpace)\n> +\t\tLOG(V4L2, Error) << \"Trying to set unrecognised color space\";\n> +\n> \tv4l2Format.type = bufferType_;\n> \tpix->width = format->size.width;\n> \tpix->height = format->size.height;\n> \tpix->pixelformat = format->fourcc;\n> \tpix->bytesperline = format->planes[0].bpl;\n> \tpix->field = V4L2_FIELD_NONE;\n> +\n> +\tret = fromColorSpace(format->colorSpace, *pix);\n> +\tif (ret < 0)\n> +\t\tLOG(V4L2, Warning)\n> +\t\t\t<< \"Set color space unrecognised by V4L2: \"\n> +\t\t\t<< ColorSpace::toString(format->colorSpace);\n> +\n> \tret = ioctl(set ? VIDIOC_S_FMT : VIDIOC_TRY_FMT, &v4l2Format);\n> \tif (ret) {\n> \t\tLOG(V4L2, Error)\n> @@ -985,6 +1026,10 @@ int V4L2VideoDevice::trySetFormatSingleplane(V4L2DeviceFormat *format, bool set)\n> \tformat->planes[0].bpl = pix->bytesperline;\n> \tformat->planes[0].size = pix->sizeimage;\n> \n> +\tformat->colorSpace = toColorSpace(*pix);\n> +\tif (!format->colorSpace)\n> +\t\tLOG(V4L2, Error) << \"Undefined color space has been set\";\n> +\n\n\nSimilar concern here.\n\nI am finding the Error/Warning strings to be a bit confusing. \nSet/Setting \"undefined/unrecognised\" color space = What does that mean \nactually? Is it about unsetting the color space so that the driver can \nuse and set it's default one?\n\n> \treturn 0;\n> }\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id D6F46BDB13\n\tfor ;\n\tTue, 23 Nov 2021 08:01:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 22A8F6036F;\n\tTue, 23 Nov 2021 09:01:06 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8A50260121\n\tfor ;\n\tTue, 23 Nov 2021 09:01:04 +0100 (CET)","from [192.168.1.106] (unknown [103.251.226.81])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D4AF4A1B\n\tfor ;\n\tTue, 23 Nov 2021 09:01:03 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Mw7psLw5\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1637654464;\n\tbh=n6zaSTchGX427uHdKJNW+6C8+WCpb7ByAZWsnLGXZBw=;\n\th=Subject:To:References:From:Date:In-Reply-To:From;\n\tb=Mw7psLw55sQl7ig27uvkyBHrg7dNMlxof2Xj4MWsHum9EGKnGRG9AigOk7dkmjMK5\n\t9uAH5E4mGNPZ8MESIRze9mfCNUZnUe2gsd7YrSN7hTpVDqWs5McJYQgk+KlYcxylow\n\tliFZnH/QUQrWSVbbGO4BjwmGPk/6tudAsXEFXsZU=","To":"libcamera-devel@lists.libcamera.org","References":"<20211118151933.15627-1-david.plowman@raspberrypi.com>\n\t<20211118151933.15627-5-david.plowman@raspberrypi.com>","From":"Umang Jain ","Message-ID":"<519a4c4d-60c5-34af-b49f-4ee907436f19@ideasonboard.com>","Date":"Tue, 23 Nov 2021 13:30:59 +0530","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.10.2","MIME-Version":"1.0","In-Reply-To":"<20211118151933.15627-5-david.plowman@raspberrypi.com>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"7bit","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH v6 4/7] libcamera: Support passing\n\tColorSpaces to V4L2 video devices","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":21192,"web_url":"https://patchwork.libcamera.org/comment/21192/","msgid":"","date":"2021-11-24T11:55:15","subject":"Re: [libcamera-devel] [PATCH v6 4/7] libcamera: Support passing\n\tColorSpaces to V4L2 video devices","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Umang\n\nThanks for looking at this!\n\nOn Tue, 23 Nov 2021 at 08:01, Umang Jain wrote:\n>\n> Hi David,\n>\n> On 11/18/21 8:49 PM, David Plowman wrote:\n> > The ColorSpace from the StreamConfiguration is now handled\n> > appropriately in the V4L2VideoDevice.\n> >\n> > Signed-off-by: David Plowman \n> > ---\n> > include/libcamera/internal/v4l2_videodevice.h | 2 +\n> > src/libcamera/v4l2_videodevice.cpp | 53 +++++++++++++++++--\n> > 2 files changed, 51 insertions(+), 4 deletions(-)\n> >\n> > diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h\n> > index a1c458e4..00bc50e7 100644\n> > --- a/include/libcamera/internal/v4l2_videodevice.h\n> > +++ b/include/libcamera/internal/v4l2_videodevice.h\n> > @@ -20,6 +20,7 @@\n> > #include \n> > #include \n> >\n> > +#include \n> > #include \n> > #include \n> > #include \n> > @@ -167,6 +168,7 @@ public:\n> >\n> > V4L2PixelFormat fourcc;\n> > Size size;\n> > + std::optional colorSpace;\n> >\n> > std::array planes;\n> > unsigned int planesCount = 0;\n> > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> > index 4f04212d..e03ff8b5 100644\n> > --- a/src/libcamera/v4l2_videodevice.cpp\n> > +++ b/src/libcamera/v4l2_videodevice.cpp\n> > @@ -370,17 +370,27 @@ bool V4L2BufferCache::Entry::operator==(const FrameBuffer &buffer) const\n> > * \\brief The plane line stride (in bytes)\n> > */\n> >\n> > +/**\n> > + * \\var V4L2DeviceFormat::fourcc\n> > + * \\brief The fourcc code describing the pixel encoding scheme\n> > + *\n> > + * The fourcc code, as defined by the V4L2 API with the V4L2_PIX_FMT_* macros,\n> > + * that identifies the image format pixel encoding scheme.\n> > + */\n> > +\n> > /**\n> > * \\var V4L2DeviceFormat::size\n> > * \\brief The image size in pixels\n> > */\n> >\n> > /**\n> > - * \\var V4L2DeviceFormat::fourcc\n> > - * \\brief The fourcc code describing the pixel encoding scheme\n> > + * \\var V4L2DeviceFormat::colorSpace\n> > + * \\brief The color space of the pixels\n> > *\n> > - * The fourcc code, as defined by the V4L2 API with the V4L2_PIX_FMT_* macros,\n> > - * that identifies the image format pixel encoding scheme.\n> > + * The color space of the image. When setting the format this may be\n> > + * unset, in which case the driver gets to use its default color space.\n> > + * After being set, this value should contain the color space that the\n> > + * was actually used.\n> > */\n> >\n> > /**\n> > @@ -879,6 +889,10 @@ int V4L2VideoDevice::getFormatMultiplane(V4L2DeviceFormat *format)\n> > format->fourcc = V4L2PixelFormat(pix->pixelformat);\n> > format->planesCount = pix->num_planes;\n> >\n> > + format->colorSpace = toColorSpace(*pix);\n> > + if (!format->colorSpace)\n> > + LOG(V4L2, Error) << \"Retrieved unrecognised color space\";\n>\n>\n> You have marked this as an Error, should we be return here too?\n\nI think the general consensus was that the ColorSpace should list all\nthe colour spaces that are ever used, and if we come across new ones\nthen we will need to add them.\n\nFor this reason I think \"Error\" is reasonable here to make the\ndeveloper take note. They will probably need to add their colour space\nto the ColorSpace class.\n\nHowever, returning with a \"hard error\" is unhelpful, in my view,\nbecause the code will run on and \"pretty much work\", just with\n(probably) some confusion about whether the colour space is right.\n\n>\n> > +\n> > for (unsigned int i = 0; i < format->planesCount; ++i) {\n> > format->planes[i].bpl = pix->plane_fmt[i].bytesperline;\n> > format->planes[i].size = pix->plane_fmt[i].sizeimage;\n> > @@ -893,6 +907,9 @@ int V4L2VideoDevice::trySetFormatMultiplane(V4L2DeviceFormat *format, bool set)\n> > struct v4l2_pix_format_mplane *pix = &v4l2Format.fmt.pix_mp;\n> > int ret;\n> >\n> > + if (!format->colorSpace)\n> > + LOG(V4L2, Warning) << \"Setting undefined color space\";\n> > +\n> > v4l2Format.type = bufferType_;\n> > pix->width = format->size.width;\n> > pix->height = format->size.height;\n> > @@ -900,6 +917,12 @@ int V4L2VideoDevice::trySetFormatMultiplane(V4L2DeviceFormat *format, bool set)\n> > pix->num_planes = format->planesCount;\n> > pix->field = V4L2_FIELD_NONE;\n> >\n> > + ret = fromColorSpace(format->colorSpace, *pix);\n> > + if (ret < 0)\n> > + LOG(V4L2, Warning)\n> > + << \"Setting color space unrecognised by V4L2: \"\n> > + << ColorSpace::toString(format->colorSpace);\n> > +\n> > ASSERT(pix->num_planes <= std::size(pix->plane_fmt));\n> >\n> > for (unsigned int i = 0; i < pix->num_planes; ++i) {\n> > @@ -928,6 +951,10 @@ int V4L2VideoDevice::trySetFormatMultiplane(V4L2DeviceFormat *format, bool set)\n> > format->planes[i].size = pix->plane_fmt[i].sizeimage;\n> > }\n> >\n> > + format->colorSpace = toColorSpace(*pix);\n> > + if (!format->colorSpace)\n> > + LOG(V4L2, Warning) << \"Unrecognised color space has been set\";\n\nActually I think this should be \"Error\", not \"Warning\", for the same\nreasons as above. It means the developer needs to add a new colour\nspace to the ColorSpace class.\n\n> > +\n> > return 0;\n> > }\n> >\n> > @@ -951,6 +978,10 @@ int V4L2VideoDevice::getFormatSingleplane(V4L2DeviceFormat *format)\n> > format->planes[0].bpl = pix->bytesperline;\n> > format->planes[0].size = pix->sizeimage;\n> >\n> > + format->colorSpace = toColorSpace(*pix);\n> > + if (!format->colorSpace)\n> > + LOG(V4L2, Error) << \"Retrieved unrecognised color space\";\n> > +\n> > return 0;\n> > }\n> >\n> > @@ -960,12 +991,22 @@ int V4L2VideoDevice::trySetFormatSingleplane(V4L2DeviceFormat *format, bool set)\n> > struct v4l2_pix_format *pix = &v4l2Format.fmt.pix;\n> > int ret;\n> >\n> > + if (!format->colorSpace)\n> > + LOG(V4L2, Error) << \"Trying to set unrecognised color space\";\n> > +\n> > v4l2Format.type = bufferType_;\n> > pix->width = format->size.width;\n> > pix->height = format->size.height;\n> > pix->pixelformat = format->fourcc;\n> > pix->bytesperline = format->planes[0].bpl;\n> > pix->field = V4L2_FIELD_NONE;\n> > +\n> > + ret = fromColorSpace(format->colorSpace, *pix);\n> > + if (ret < 0)\n> > + LOG(V4L2, Warning)\n> > + << \"Set color space unrecognised by V4L2: \"\n> > + << ColorSpace::toString(format->colorSpace);\n> > +\n> > ret = ioctl(set ? VIDIOC_S_FMT : VIDIOC_TRY_FMT, &v4l2Format);\n> > if (ret) {\n> > LOG(V4L2, Error)\n> > @@ -985,6 +1026,10 @@ int V4L2VideoDevice::trySetFormatSingleplane(V4L2DeviceFormat *format, bool set)\n> > format->planes[0].bpl = pix->bytesperline;\n> > format->planes[0].size = pix->sizeimage;\n> >\n> > + format->colorSpace = toColorSpace(*pix);\n> > + if (!format->colorSpace)\n> > + LOG(V4L2, Error) << \"Undefined color space has been set\";\n> > +\n>\n>\n> Similar concern here.\n\nThis should be \"Unrecognised\" rather than \"Undefined\". By\n\"unrecognised\" I mean that the ColorSpace class does not have an entry\nfor the colour space V4L2 is using. It's a perfectly good colour\nspace, just our ColorSpace class does not recognise it.\n\n>\n> I am finding the Error/Warning strings to be a bit confusing.\n> Set/Setting \"undefined/unrecognised\" color space = What does that mean\n> actually? Is it about unsetting the color space so that the driver can\n> use and set it's default one?\n\nYes, I'm sorry this is a bit confusing. Here are some of the things\nthat can happen:\n\n1. Application leaves the ColorSpace \"unset\" (it's actually a std::optional).\n\nThis is OK, you'll get the driver's default choice, but I print out a\n\"Warning\" just to make it clear that you should check what colour\nspace you are getting. I refer to this often as \"setting an undefined\ncolour space\". Note that you cannot \"set an unrecognised colour space\"\n- by \"unrecognised\" I mean specifically that a colour space is missing\nfrom the ColorSpace class - see below.\n\n2. Cannot turn ColorSpace into V4L2 enums\n\nActually this can't happen currently because the ColorSpace class is a\nsubset of V4L2 ones. But it might in the future. I print out a\n\"Warning\" because it would be something to investigate, but an\n\"Error\", especially a \"hard failure\" seems unhelpful. Again, things\nwill \"mostly work\", and in any case, the problem may not actually be\nfixable.\n\n3. Cannot turn V4L2 enums into a ColorSpace\n\nThis would be bad and the consensus the other day was that a new entry\nshould be added to the ColorSpace class to fix the problem. So I think\n\"Error\" is reasonable, though again, a \"hard failure\" seems unhelpful\nbecause, as before, everything will \"mostly work\". This is what I mean\nby \"unrecognised\".\n\nDoes that help at all?\n\nThanks again and best regards\nDavid\n\n>\n> > return 0;\n> > }\n> >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 3909BBDB13\n\tfor ;\n\tWed, 24 Nov 2021 11:55:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8D3E460371;\n\tWed, 24 Nov 2021 12:55:27 +0100 (CET)","from mail-wm1-x329.google.com (mail-wm1-x329.google.com\n\t[IPv6:2a00:1450:4864:20::329])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6A4FC60128\n\tfor ;\n\tWed, 24 Nov 2021 12:55:26 +0100 (CET)","by mail-wm1-x329.google.com with SMTP id y196so2051807wmc.3\n\tfor ;\n\tWed, 24 Nov 2021 03:55:26 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"nPrL+LUe\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=yT8xZIj8n0jTfoyd0Rm6cSl8aReafvjPKDlY34SxCc8=;\n\tb=nPrL+LUelsuxZuLCStpwgAXIjlOUxaHrKByxFMV12CWWSg3acbaugZ4d3TyToYbn4K\n\tcQ83I5bpSGcOtSCsZe0u0YSLPZ2df9PQtSmRU4J6jnQck/1MWCoF7I7CACZyPeZdqvEw\n\t1zv6n4nuih51Rk65EgWdky60eGloaFhNcyhUxkWGdr3ZV2w1laCpQoECy/JMkUee+L5Z\n\tyriBx4N1jqbnIWQst1TTAc0ywwzfPsyuM/sv5dJ1hcrA/idlHu773bOZvos20+c05pfb\n\tJ9Umsw+4e/atpgdYgQ0OqgsJVF8M6/qOW/y+10aYZ4zv3z/1G/sjimAfc5Ry6S8t9hfA\n\tPNPw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=yT8xZIj8n0jTfoyd0Rm6cSl8aReafvjPKDlY34SxCc8=;\n\tb=7bErv42ELdyfRiBLH/OxmGYR5kcmwWFDzkk8P9BXBX/CU7vzMeQfMZa308XFx3GVLH\n\tEM+B3+33QWd7LeMPBjV7Anw+SofoppgbK1jhGFyxgV5J3UIIiphqJyN9NG6vfb8IM5P/\n\t/tOVE88z0rdbFlDeC3NlGI5OJVpeQr+OxtEqhl7cjF9qnVM7LxQFSkgW4BblzGX14lsa\n\tcPhZL5XWu0+lj5dKpKx+gfnepxqTPdjZiZ4eVb8i1rtXyRgcQkUzivFFUUT9oA0BEgHT\n\tbJqONHAhoY2YVBK8p7Yx4etvuEY82ZQocMStTNCMB/zavsmX+Xl4VYvOUpoEFZBg4rmF\n\tdUrw==","X-Gm-Message-State":"AOAM531aFx7qNXnxCXZcazWIDyFT6L/iUw9ITaHKP3HdZQknpxojqqgY\n\togzpK39nprh0JgC8elgWzcMs1MdkjUMZ66ff5NDilMEQO8OI4A==","X-Google-Smtp-Source":"ABdhPJz13I7BcWTeWKfBe8T0T1hG49xx0SOzndYA1PfvHI5KMOEpkcgwQ74LrpTKgf909aMe9MOoV8PctoCJY06T2TM=","X-Received":"by 2002:a7b:ca54:: with SMTP id\n\tm20mr14559393wml.21.1637754925889; \n\tWed, 24 Nov 2021 03:55:25 -0800 (PST)","MIME-Version":"1.0","References":"<20211118151933.15627-1-david.plowman@raspberrypi.com>\n\t<20211118151933.15627-5-david.plowman@raspberrypi.com>\n\t<519a4c4d-60c5-34af-b49f-4ee907436f19@ideasonboard.com>","In-Reply-To":"<519a4c4d-60c5-34af-b49f-4ee907436f19@ideasonboard.com>","From":"David Plowman ","Date":"Wed, 24 Nov 2021 11:55:15 +0000","Message-ID":"","To":"Umang Jain ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v6 4/7] libcamera: Support passing\n\tColorSpaces to V4L2 video devices","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":21197,"web_url":"https://patchwork.libcamera.org/comment/21197/","msgid":"<163775882837.3059017.10170721954512813106@Monstersaurus>","date":"2021-11-24T13:00:28","subject":"Re: [libcamera-devel] [PATCH v6 4/7] libcamera: Support passing\n\tColorSpaces to V4L2 video devices","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting David Plowman (2021-11-24 11:55:15)\n> Hi Umang\n> \n> Thanks for looking at this!\n> \n> On Tue, 23 Nov 2021 at 08:01, Umang Jain wrote:\n> >\n> > Hi David,\n> >\n> > On 11/18/21 8:49 PM, David Plowman wrote:\n> > > The ColorSpace from the StreamConfiguration is now handled\n> > > appropriately in the V4L2VideoDevice.\n> > >\n> > > Signed-off-by: David Plowman \n> > > ---\n> > > include/libcamera/internal/v4l2_videodevice.h | 2 +\n> > > src/libcamera/v4l2_videodevice.cpp | 53 +++++++++++++++++--\n> > > 2 files changed, 51 insertions(+), 4 deletions(-)\n> > >\n> > > diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h\n> > > index a1c458e4..00bc50e7 100644\n> > > --- a/include/libcamera/internal/v4l2_videodevice.h\n> > > +++ b/include/libcamera/internal/v4l2_videodevice.h\n> > > @@ -20,6 +20,7 @@\n> > > #include \n> > > #include \n> > >\n> > > +#include \n> > > #include \n> > > #include \n> > > #include \n> > > @@ -167,6 +168,7 @@ public:\n> > >\n> > > V4L2PixelFormat fourcc;\n> > > Size size;\n> > > + std::optional colorSpace;\n> > >\n> > > std::array planes;\n> > > unsigned int planesCount = 0;\n> > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> > > index 4f04212d..e03ff8b5 100644\n> > > --- a/src/libcamera/v4l2_videodevice.cpp\n> > > +++ b/src/libcamera/v4l2_videodevice.cpp\n> > > @@ -370,17 +370,27 @@ bool V4L2BufferCache::Entry::operator==(const FrameBuffer &buffer) const\n> > > * \\brief The plane line stride (in bytes)\n> > > */\n> > >\n> > > +/**\n> > > + * \\var V4L2DeviceFormat::fourcc\n> > > + * \\brief The fourcc code describing the pixel encoding scheme\n> > > + *\n> > > + * The fourcc code, as defined by the V4L2 API with the V4L2_PIX_FMT_* macros,\n> > > + * that identifies the image format pixel encoding scheme.\n> > > + */\n> > > +\n> > > /**\n> > > * \\var V4L2DeviceFormat::size\n> > > * \\brief The image size in pixels\n> > > */\n> > >\n> > > /**\n> > > - * \\var V4L2DeviceFormat::fourcc\n> > > - * \\brief The fourcc code describing the pixel encoding scheme\n> > > + * \\var V4L2DeviceFormat::colorSpace\n> > > + * \\brief The color space of the pixels\n> > > *\n> > > - * The fourcc code, as defined by the V4L2 API with the V4L2_PIX_FMT_* macros,\n> > > - * that identifies the image format pixel encoding scheme.\n> > > + * The color space of the image. When setting the format this may be\n> > > + * unset, in which case the driver gets to use its default color space.\n> > > + * After being set, this value should contain the color space that the\n> > > + * was actually used.\n> > > */\n> > >\n> > > /**\n> > > @@ -879,6 +889,10 @@ int V4L2VideoDevice::getFormatMultiplane(V4L2DeviceFormat *format)\n> > > format->fourcc = V4L2PixelFormat(pix->pixelformat);\n> > > format->planesCount = pix->num_planes;\n> > >\n> > > + format->colorSpace = toColorSpace(*pix);\n> > > + if (!format->colorSpace)\n> > > + LOG(V4L2, Error) << \"Retrieved unrecognised color space\";\n> >\n> >\n> > You have marked this as an Error, should we be return here too?\n> \n> I think the general consensus was that the ColorSpace should list all\n> the colour spaces that are ever used, and if we come across new ones\n> then we will need to add them.\n\nYes, getting the format should still continue. We may still be able to\n'set' an appropriate one later... so this I think is part of the\nnegotiation phases.\n\n\nDelving further into the color-space issues, we'll need to consider\ngstreamer support too.\n\nThey define their colorspace support at:\n\nhttps://github.com/GStreamer/gst-plugins-base/blob/master/gst-libs/gst/video/video-color.h\n\nWhich may be useful reference.\n\nOur gstreamer element will need to convert between colorimetry and\ncolorspace, and negotiate between the two frameworks, so it likely\nprovides a really good test case for all of this.\n\n(Related bug, from a raspberry pi user)\n - https://bugs.libcamera.org/show_bug.cgi?id=75\n\n\n> For this reason I think \"Error\" is reasonable here to make the\n> developer take note. They will probably need to add their colour space\n> to the ColorSpace class.\n> \n> However, returning with a \"hard error\" is unhelpful, in my view,\n> because the code will run on and \"pretty much work\", just with\n> (probably) some confusion about whether the colour space is right.\n> \n> >\n> > > +\n> > > for (unsigned int i = 0; i < format->planesCount; ++i) {\n> > > format->planes[i].bpl = pix->plane_fmt[i].bytesperline;\n> > > format->planes[i].size = pix->plane_fmt[i].sizeimage;\n> > > @@ -893,6 +907,9 @@ int V4L2VideoDevice::trySetFormatMultiplane(V4L2DeviceFormat *format, bool set)\n> > > struct v4l2_pix_format_mplane *pix = &v4l2Format.fmt.pix_mp;\n> > > int ret;\n> > >\n> > > + if (!format->colorSpace)\n> > > + LOG(V4L2, Warning) << \"Setting undefined color space\";\n> > > +\n> > > v4l2Format.type = bufferType_;\n> > > pix->width = format->size.width;\n> > > pix->height = format->size.height;\n> > > @@ -900,6 +917,12 @@ int V4L2VideoDevice::trySetFormatMultiplane(V4L2DeviceFormat *format, bool set)\n> > > pix->num_planes = format->planesCount;\n> > > pix->field = V4L2_FIELD_NONE;\n> > >\n> > > + ret = fromColorSpace(format->colorSpace, *pix);\n> > > + if (ret < 0)\n> > > + LOG(V4L2, Warning)\n\nI think that can be Error too. This shouldn't happen, and if it does -\nit should be fixed (in the kernel?)\n\n> > > + << \"Setting color space unrecognised by V4L2: \"\n> > > + << ColorSpace::toString(format->colorSpace);\n> > > +\n> > > ASSERT(pix->num_planes <= std::size(pix->plane_fmt));\n> > >\n> > > for (unsigned int i = 0; i < pix->num_planes; ++i) {\n> > > @@ -928,6 +951,10 @@ int V4L2VideoDevice::trySetFormatMultiplane(V4L2DeviceFormat *format, bool set)\n> > > format->planes[i].size = pix->plane_fmt[i].sizeimage;\n> > > }\n> > >\n> > > + format->colorSpace = toColorSpace(*pix);\n> > > + if (!format->colorSpace)\n> > > + LOG(V4L2, Warning) << \"Unrecognised color space has been set\";\n> \n> Actually I think this should be \"Error\", not \"Warning\", for the same\n> reasons as above. It means the developer needs to add a new colour\n> space to the ColorSpace class.\n\nI agree here, I think Error makes sense.\n\n> \n> > > +\n> > > return 0;\n> > > }\n> > >\n> > > @@ -951,6 +978,10 @@ int V4L2VideoDevice::getFormatSingleplane(V4L2DeviceFormat *format)\n> > > format->planes[0].bpl = pix->bytesperline;\n> > > format->planes[0].size = pix->sizeimage;\n> > >\n> > > + format->colorSpace = toColorSpace(*pix);\n> > > + if (!format->colorSpace)\n> > > + LOG(V4L2, Error) << \"Retrieved unrecognised color space\";\n> > > +\n> > > return 0;\n> > > }\n> > >\n> > > @@ -960,12 +991,22 @@ int V4L2VideoDevice::trySetFormatSingleplane(V4L2DeviceFormat *format, bool set)\n> > > struct v4l2_pix_format *pix = &v4l2Format.fmt.pix;\n> > > int ret;\n> > >\n> > > + if (!format->colorSpace)\n> > > + LOG(V4L2, Error) << \"Trying to set unrecognised color space\";\n> > > +\n> > > v4l2Format.type = bufferType_;\n> > > pix->width = format->size.width;\n> > > pix->height = format->size.height;\n> > > pix->pixelformat = format->fourcc;\n> > > pix->bytesperline = format->planes[0].bpl;\n> > > pix->field = V4L2_FIELD_NONE;\n> > > +\n> > > + ret = fromColorSpace(format->colorSpace, *pix);\n> > > + if (ret < 0)\n> > > + LOG(V4L2, Warning)\n> > > + << \"Set color space unrecognised by V4L2: \"\n> > > + << ColorSpace::toString(format->colorSpace);\n> > > +\n> > > ret = ioctl(set ? VIDIOC_S_FMT : VIDIOC_TRY_FMT, &v4l2Format);\n> > > if (ret) {\n> > > LOG(V4L2, Error)\n> > > @@ -985,6 +1026,10 @@ int V4L2VideoDevice::trySetFormatSingleplane(V4L2DeviceFormat *format, bool set)\n> > > format->planes[0].bpl = pix->bytesperline;\n> > > format->planes[0].size = pix->sizeimage;\n> > >\n> > > + format->colorSpace = toColorSpace(*pix);\n> > > + if (!format->colorSpace)\n> > > + LOG(V4L2, Error) << \"Undefined color space has been set\";\n> > > +\n> >\n> >\n> > Similar concern here.\n> \n> This should be \"Unrecognised\" rather than \"Undefined\". By\n> \"unrecognised\" I mean that the ColorSpace class does not have an entry\n> for the colour space V4L2 is using. It's a perfectly good colour\n> space, just our ColorSpace class does not recognise it.\n> \n> >\n> > I am finding the Error/Warning strings to be a bit confusing.\n> > Set/Setting \"undefined/unrecognised\" color space = What does that mean\n> > actually? Is it about unsetting the color space so that the driver can\n> > use and set it's default one?\n> \n> Yes, I'm sorry this is a bit confusing. Here are some of the things\n> that can happen:\n> \n> 1. Application leaves the ColorSpace \"unset\" (it's actually a std::optional).\n> \n> This is OK, you'll get the driver's default choice, but I print out a\n> \"Warning\" just to make it clear that you should check what colour\n> space you are getting. I refer to this often as \"setting an undefined\n> colour space\". Note that you cannot \"set an unrecognised colour space\"\n> - by \"unrecognised\" I mean specifically that a colour space is missing\n> from the ColorSpace class - see below.\n> \n> 2. Cannot turn ColorSpace into V4L2 enums\n> \n> Actually this can't happen currently because the ColorSpace class is a\n> subset of V4L2 ones. But it might in the future. I print out a\n> \"Warning\" because it would be something to investigate, but an\n> \"Error\", especially a \"hard failure\" seems unhelpful. Again, things\n> will \"mostly work\", and in any case, the problem may not actually be\n> fixable.\n\nI think this should report as an Error - but not be a hard failure (i.e.\ncontinue).\n\nThe fix would be required on the kernel side in that case...\n\n\n> 3. Cannot turn V4L2 enums into a ColorSpace\n> \n> This would be bad and the consensus the other day was that a new entry\n> should be added to the ColorSpace class to fix the problem. So I think\n> \"Error\" is reasonable, though again, a \"hard failure\" seems unhelpful\n> because, as before, everything will \"mostly work\". This is what I mean\n> by \"unrecognised\".\n> \n> Does that help at all?\n> \n> Thanks again and best regards\n> David\n> \n> >\n> > > return 0;\n> > > }\n> > >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 6E8FDBF415\n\tfor ;\n\tWed, 24 Nov 2021 13:00:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B80FE60371;\n\tWed, 24 Nov 2021 14:00:33 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0012760128\n\tfor ;\n\tWed, 24 Nov 2021 14:00:31 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 94DAB993;\n\tWed, 24 Nov 2021 14:00:31 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"F9YoYcfO\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1637758831;\n\tbh=o0rb9F2BFRoZo8p/ji7eLoUD5LS0aSSGO3OWdgZJNeo=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=F9YoYcfO0vW6NikZFAkqfX2gKDHHz5IInnDcD5v+gk+UWO5qVAa1j7NcSEpHljpeS\n\tTGvzmDl2tr/zsKxFfMhnemqGfB4h1m4iayO4VLfaBEoSc9JwLfMqm8dqZ1RsswecTT\n\tuSzysfMi4fSp/SfDHekQWx4sPrE+78U+Y5Rry4Bc=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20211118151933.15627-1-david.plowman@raspberrypi.com>\n\t<20211118151933.15627-5-david.plowman@raspberrypi.com>\n\t<519a4c4d-60c5-34af-b49f-4ee907436f19@ideasonboard.com>\n\t","From":"Kieran Bingham ","To":"David Plowman ,\n\tUmang Jain ","Date":"Wed, 24 Nov 2021 13:00:28 +0000","Message-ID":"<163775882837.3059017.10170721954512813106@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v6 4/7] libcamera: Support passing\n\tColorSpaces to V4L2 video devices","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":21209,"web_url":"https://patchwork.libcamera.org/comment/21209/","msgid":"<20211124234258.vart2hjbrjlqwh3r@uno.localdomain>","date":"2021-11-24T23:42:58","subject":"Re: [libcamera-devel] [PATCH v6 4/7] libcamera: Support passing\n\tColorSpaces to V4L2 video devices","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi David,\n\nOn Thu, Nov 18, 2021 at 03:19:30PM +0000, David Plowman wrote:\n> The ColorSpace from the StreamConfiguration is now handled\n> appropriately in the V4L2VideoDevice.\n>\n> Signed-off-by: David Plowman \n> ---\n> include/libcamera/internal/v4l2_videodevice.h | 2 +\n> src/libcamera/v4l2_videodevice.cpp | 53 +++++++++++++++++--\n> 2 files changed, 51 insertions(+), 4 deletions(-)\n>\n> diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h\n> index a1c458e4..00bc50e7 100644\n> --- a/include/libcamera/internal/v4l2_videodevice.h\n> +++ b/include/libcamera/internal/v4l2_videodevice.h\n> @@ -20,6 +20,7 @@\n> #include \n> #include \n>\n> +#include \n> #include \n> #include \n> #include \n> @@ -167,6 +168,7 @@ public:\n>\n> \tV4L2PixelFormat fourcc;\n> \tSize size;\n> +\tstd::optional colorSpace;\n>\n> \tstd::array planes;\n> \tunsigned int planesCount = 0;\n> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> index 4f04212d..e03ff8b5 100644\n> --- a/src/libcamera/v4l2_videodevice.cpp\n> +++ b/src/libcamera/v4l2_videodevice.cpp\n> @@ -370,17 +370,27 @@ bool V4L2BufferCache::Entry::operator==(const FrameBuffer &buffer) const\n> * \\brief The plane line stride (in bytes)\n> */\n>\n> +/**\n> + * \\var V4L2DeviceFormat::fourcc\n> + * \\brief The fourcc code describing the pixel encoding scheme\n> + *\n> + * The fourcc code, as defined by the V4L2 API with the V4L2_PIX_FMT_* macros,\n> + * that identifies the image format pixel encoding scheme.\n> + */\n> +\n\nNice that the uncomplete documentation about color spaces has been\ndropped, but why moving the documentation block above ? Please keep\nthe same order as the declaration order in the header file.\n\nIt will also make it easier to diff it during review.\n\n> /**\n> * \\var V4L2DeviceFormat::size\n> * \\brief The image size in pixels\n> */\n>\n> /**\n> - * \\var V4L2DeviceFormat::fourcc\n> - * \\brief The fourcc code describing the pixel encoding scheme\n> + * \\var V4L2DeviceFormat::colorSpace\n> + * \\brief The color space of the pixels\n> *\n> - * The fourcc code, as defined by the V4L2 API with the V4L2_PIX_FMT_* macros,\n> - * that identifies the image format pixel encoding scheme.\n> + * The color space of the image. When setting the format this may be\n> + * unset, in which case the driver gets to use its default color space.\n> + * After being set, this value should contain the color space that the\n\ns/that the/that ?\n\n> + * was actually used.\n> */\n>\n> /**\n> @@ -879,6 +889,10 @@ int V4L2VideoDevice::getFormatMultiplane(V4L2DeviceFormat *format)\n> \tformat->fourcc = V4L2PixelFormat(pix->pixelformat);\n> \tformat->planesCount = pix->num_planes;\n>\n> +\tformat->colorSpace = toColorSpace(*pix);\n> +\tif (!format->colorSpace)\n> +\t\tLOG(V4L2, Error) << \"Retrieved unrecognised color space\";\n> +\n> \tfor (unsigned int i = 0; i < format->planesCount; ++i) {\n> \t\tformat->planes[i].bpl = pix->plane_fmt[i].bytesperline;\n> \t\tformat->planes[i].size = pix->plane_fmt[i].sizeimage;\n> @@ -893,6 +907,9 @@ int V4L2VideoDevice::trySetFormatMultiplane(V4L2DeviceFormat *format, bool set)\n> \tstruct v4l2_pix_format_mplane *pix = &v4l2Format.fmt.pix_mp;\n> \tint ret;\n>\n> +\tif (!format->colorSpace)\n> +\t\tLOG(V4L2, Warning) << \"Setting undefined color space\";\n> +\n> \tv4l2Format.type = bufferType_;\n> \tpix->width = format->size.width;\n> \tpix->height = format->size.height;\n> @@ -900,6 +917,12 @@ int V4L2VideoDevice::trySetFormatMultiplane(V4L2DeviceFormat *format, bool set)\n> \tpix->num_planes = format->planesCount;\n> \tpix->field = V4L2_FIELD_NONE;\n>\n> +\tret = fromColorSpace(format->colorSpace, *pix);\n> +\tif (ret < 0)\n> +\t\tLOG(V4L2, Warning)\n> +\t\t\t<< \"Setting color space unrecognised by V4L2: \"\n> +\t\t\t<< ColorSpace::toString(format->colorSpace);\n> +\n> \tASSERT(pix->num_planes <= std::size(pix->plane_fmt));\n>\n> \tfor (unsigned int i = 0; i < pix->num_planes; ++i) {\n> @@ -928,6 +951,10 @@ int V4L2VideoDevice::trySetFormatMultiplane(V4L2DeviceFormat *format, bool set)\n> \t\tformat->planes[i].size = pix->plane_fmt[i].sizeimage;\n> \t}\n>\n> +\tformat->colorSpace = toColorSpace(*pix);\n> +\tif (!format->colorSpace)\n> +\t\tLOG(V4L2, Warning) << \"Unrecognised color space has been set\";\n> +\n\nDoes colorspace handling stricly have to be tied to format\nconfiguration ? This would lead to undesired side effects, like the metadata pad\nbeing poked for colorspace and complaining as it doesn't really have\none.\n\nShould colorspace configuration be achieved with dedicated functions\nin the V4L2*Device classes so that pipeline handlers know what devices\nto apply/retrieve colorspace from, like they do with formats ?\n\n> \treturn 0;\n> }\n>\n> @@ -951,6 +978,10 @@ int V4L2VideoDevice::getFormatSingleplane(V4L2DeviceFormat *format)\n> \tformat->planes[0].bpl = pix->bytesperline;\n> \tformat->planes[0].size = pix->sizeimage;\n>\n> +\tformat->colorSpace = toColorSpace(*pix);\n> +\tif (!format->colorSpace)\n> +\t\tLOG(V4L2, Error) << \"Retrieved unrecognised color space\";\n> +\n> \treturn 0;\n> }\n>\n> @@ -960,12 +991,22 @@ int V4L2VideoDevice::trySetFormatSingleplane(V4L2DeviceFormat *format, bool set)\n> \tstruct v4l2_pix_format *pix = &v4l2Format.fmt.pix;\n> \tint ret;\n>\n> +\tif (!format->colorSpace)\n> +\t\tLOG(V4L2, Error) << \"Trying to set unrecognised color space\";\n> +\n> \tv4l2Format.type = bufferType_;\n> \tpix->width = format->size.width;\n> \tpix->height = format->size.height;\n> \tpix->pixelformat = format->fourcc;\n> \tpix->bytesperline = format->planes[0].bpl;\n> \tpix->field = V4L2_FIELD_NONE;\n> +\n> +\tret = fromColorSpace(format->colorSpace, *pix);\n> +\tif (ret < 0)\n> +\t\tLOG(V4L2, Warning)\n> +\t\t\t<< \"Set color space unrecognised by V4L2: \"\n> +\t\t\t<< ColorSpace::toString(format->colorSpace);\n> +\n> \tret = ioctl(set ? VIDIOC_S_FMT : VIDIOC_TRY_FMT, &v4l2Format);\n> \tif (ret) {\n> \t\tLOG(V4L2, Error)\n> @@ -985,6 +1026,10 @@ int V4L2VideoDevice::trySetFormatSingleplane(V4L2DeviceFormat *format, bool set)\n> \tformat->planes[0].bpl = pix->bytesperline;\n> \tformat->planes[0].size = pix->sizeimage;\n>\n> +\tformat->colorSpace = toColorSpace(*pix);\n> +\tif (!format->colorSpace)\n> +\t\tLOG(V4L2, Error) << \"Undefined color space has been set\";\n> +\n> \treturn 0;\n> }\n>\n> --\n> 2.20.1\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 0AF05BF415\n\tfor ;\n\tWed, 24 Nov 2021 23:42:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 19DE160371;\n\tThu, 25 Nov 2021 00:42:09 +0100 (CET)","from relay11.mail.gandi.net (relay11.mail.gandi.net\n\t[217.70.178.231])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id AF69760228\n\tfor ;\n\tThu, 25 Nov 2021 00:42:07 +0100 (CET)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay11.mail.gandi.net (Postfix) with ESMTPSA id 439C4100008;\n\tWed, 24 Nov 2021 23:42:06 +0000 (UTC)"],"Date":"Thu, 25 Nov 2021 00:42:58 +0100","From":"Jacopo Mondi ","To":"David Plowman ","Message-ID":"<20211124234258.vart2hjbrjlqwh3r@uno.localdomain>","References":"<20211118151933.15627-1-david.plowman@raspberrypi.com>\n\t<20211118151933.15627-5-david.plowman@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211118151933.15627-5-david.plowman@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v6 4/7] libcamera: Support passing\n\tColorSpaces to V4L2 video devices","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"tfiga@google.com, libcamera-devel@lists.libcamera.org,\n\thverkuil-cisco@xs4all.nl","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":21257,"web_url":"https://patchwork.libcamera.org/comment/21257/","msgid":"","date":"2021-11-25T16:36:40","subject":"Re: [libcamera-devel] [PATCH v6 4/7] libcamera: Support passing\n\tColorSpaces to V4L2 video devices","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Jacopo\n\nThanks for looking at this!\n\nOn Wed, 24 Nov 2021 at 23:42, Jacopo Mondi wrote:\n>\n> Hi David,\n>\n> On Thu, Nov 18, 2021 at 03:19:30PM +0000, David Plowman wrote:\n> > The ColorSpace from the StreamConfiguration is now handled\n> > appropriately in the V4L2VideoDevice.\n> >\n> > Signed-off-by: David Plowman \n> > ---\n> > include/libcamera/internal/v4l2_videodevice.h | 2 +\n> > src/libcamera/v4l2_videodevice.cpp | 53 +++++++++++++++++--\n> > 2 files changed, 51 insertions(+), 4 deletions(-)\n> >\n> > diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h\n> > index a1c458e4..00bc50e7 100644\n> > --- a/include/libcamera/internal/v4l2_videodevice.h\n> > +++ b/include/libcamera/internal/v4l2_videodevice.h\n> > @@ -20,6 +20,7 @@\n> > #include \n> > #include \n> >\n> > +#include \n> > #include \n> > #include \n> > #include \n> > @@ -167,6 +168,7 @@ public:\n> >\n> > V4L2PixelFormat fourcc;\n> > Size size;\n> > + std::optional colorSpace;\n> >\n> > std::array planes;\n> > unsigned int planesCount = 0;\n> > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> > index 4f04212d..e03ff8b5 100644\n> > --- a/src/libcamera/v4l2_videodevice.cpp\n> > +++ b/src/libcamera/v4l2_videodevice.cpp\n> > @@ -370,17 +370,27 @@ bool V4L2BufferCache::Entry::operator==(const FrameBuffer &buffer) const\n> > * \\brief The plane line stride (in bytes)\n> > */\n> >\n> > +/**\n> > + * \\var V4L2DeviceFormat::fourcc\n> > + * \\brief The fourcc code describing the pixel encoding scheme\n> > + *\n> > + * The fourcc code, as defined by the V4L2 API with the V4L2_PIX_FMT_* macros,\n> > + * that identifies the image format pixel encoding scheme.\n> > + */\n> > +\n>\n> Nice that the uncomplete documentation about color spaces has been\n> dropped, but why moving the documentation block above ? Please keep\n> the same order as the declaration order in the header file.\n>\n> It will also make it easier to diff it during review.\n>\n> > /**\n> > * \\var V4L2DeviceFormat::size\n> > * \\brief The image size in pixels\n> > */\n> >\n> > /**\n> > - * \\var V4L2DeviceFormat::fourcc\n> > - * \\brief The fourcc code describing the pixel encoding scheme\n> > + * \\var V4L2DeviceFormat::colorSpace\n> > + * \\brief The color space of the pixels\n> > *\n> > - * The fourcc code, as defined by the V4L2 API with the V4L2_PIX_FMT_* macros,\n> > - * that identifies the image format pixel encoding scheme.\n> > + * The color space of the image. When setting the format this may be\n> > + * unset, in which case the driver gets to use its default color space.\n> > + * After being set, this value should contain the color space that the\n>\n> s/that the/that ?\n>\n> > + * was actually used.\n> > */\n> >\n> > /**\n> > @@ -879,6 +889,10 @@ int V4L2VideoDevice::getFormatMultiplane(V4L2DeviceFormat *format)\n> > format->fourcc = V4L2PixelFormat(pix->pixelformat);\n> > format->planesCount = pix->num_planes;\n> >\n> > + format->colorSpace = toColorSpace(*pix);\n> > + if (!format->colorSpace)\n> > + LOG(V4L2, Error) << \"Retrieved unrecognised color space\";\n> > +\n> > for (unsigned int i = 0; i < format->planesCount; ++i) {\n> > format->planes[i].bpl = pix->plane_fmt[i].bytesperline;\n> > format->planes[i].size = pix->plane_fmt[i].sizeimage;\n> > @@ -893,6 +907,9 @@ int V4L2VideoDevice::trySetFormatMultiplane(V4L2DeviceFormat *format, bool set)\n> > struct v4l2_pix_format_mplane *pix = &v4l2Format.fmt.pix_mp;\n> > int ret;\n> >\n> > + if (!format->colorSpace)\n> > + LOG(V4L2, Warning) << \"Setting undefined color space\";\n> > +\n> > v4l2Format.type = bufferType_;\n> > pix->width = format->size.width;\n> > pix->height = format->size.height;\n> > @@ -900,6 +917,12 @@ int V4L2VideoDevice::trySetFormatMultiplane(V4L2DeviceFormat *format, bool set)\n> > pix->num_planes = format->planesCount;\n> > pix->field = V4L2_FIELD_NONE;\n> >\n> > + ret = fromColorSpace(format->colorSpace, *pix);\n> > + if (ret < 0)\n> > + LOG(V4L2, Warning)\n> > + << \"Setting color space unrecognised by V4L2: \"\n> > + << ColorSpace::toString(format->colorSpace);\n> > +\n> > ASSERT(pix->num_planes <= std::size(pix->plane_fmt));\n> >\n> > for (unsigned int i = 0; i < pix->num_planes; ++i) {\n> > @@ -928,6 +951,10 @@ int V4L2VideoDevice::trySetFormatMultiplane(V4L2DeviceFormat *format, bool set)\n> > format->planes[i].size = pix->plane_fmt[i].sizeimage;\n> > }\n> >\n> > + format->colorSpace = toColorSpace(*pix);\n> > + if (!format->colorSpace)\n> > + LOG(V4L2, Warning) << \"Unrecognised color space has been set\";\n> > +\n>\n> Does colorspace handling stricly have to be tied to format\n> configuration ? This would lead to undesired side effects, like the metadata pad\n> being poked for colorspace and complaining as it doesn't really have\n> one.\n>\n> Should colorspace configuration be achieved with dedicated functions\n> in the V4L2*Device classes so that pipeline handlers know what devices\n> to apply/retrieve colorspace from, like they do with formats ?\n\nYes, there is a problem here that I talked about in one of the earlier\nrevisions. Actually it's only an issue for subdevices, so you might\nnotice that there's a \"todo\" in v4l2_subdevice.cpp getFormat.\n\nThe problem is that I end up trying to convert the colour space on a\nmetadata pad - and of course that doesn't work. Nothing breaks, you\njust get a warning message. I'm open to suggestions on how to resolve\nthis. An extra parameter to the get/setFormat (\"I am not an image data\npad\"?). Separate functions get/setImageFormat and\nget/setMetadataFormat? Rely on the caller to check for a bad colour\nspace? (but will they bother?)\n\nThoughts, anyone?\n\nThanks!\nDavid\n\n>\n> > return 0;\n> > }\n> >\n> > @@ -951,6 +978,10 @@ int V4L2VideoDevice::getFormatSingleplane(V4L2DeviceFormat *format)\n> > format->planes[0].bpl = pix->bytesperline;\n> > format->planes[0].size = pix->sizeimage;\n> >\n> > + format->colorSpace = toColorSpace(*pix);\n> > + if (!format->colorSpace)\n> > + LOG(V4L2, Error) << \"Retrieved unrecognised color space\";\n> > +\n> > return 0;\n> > }\n> >\n> > @@ -960,12 +991,22 @@ int V4L2VideoDevice::trySetFormatSingleplane(V4L2DeviceFormat *format, bool set)\n> > struct v4l2_pix_format *pix = &v4l2Format.fmt.pix;\n> > int ret;\n> >\n> > + if (!format->colorSpace)\n> > + LOG(V4L2, Error) << \"Trying to set unrecognised color space\";\n> > +\n> > v4l2Format.type = bufferType_;\n> > pix->width = format->size.width;\n> > pix->height = format->size.height;\n> > pix->pixelformat = format->fourcc;\n> > pix->bytesperline = format->planes[0].bpl;\n> > pix->field = V4L2_FIELD_NONE;\n> > +\n> > + ret = fromColorSpace(format->colorSpace, *pix);\n> > + if (ret < 0)\n> > + LOG(V4L2, Warning)\n> > + << \"Set color space unrecognised by V4L2: \"\n> > + << ColorSpace::toString(format->colorSpace);\n> > +\n> > ret = ioctl(set ? VIDIOC_S_FMT : VIDIOC_TRY_FMT, &v4l2Format);\n> > if (ret) {\n> > LOG(V4L2, Error)\n> > @@ -985,6 +1026,10 @@ int V4L2VideoDevice::trySetFormatSingleplane(V4L2DeviceFormat *format, bool set)\n> > format->planes[0].bpl = pix->bytesperline;\n> > format->planes[0].size = pix->sizeimage;\n> >\n> > + format->colorSpace = toColorSpace(*pix);\n> > + if (!format->colorSpace)\n> > + LOG(V4L2, Error) << \"Undefined color space has been set\";\n> > +\n> > return 0;\n> > }\n> >\n> > --\n> > 2.20.1\n> >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 59531BF415\n\tfor ;\n\tThu, 25 Nov 2021 16:36:54 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6FCE56036F;\n\tThu, 25 Nov 2021 17:36:53 +0100 (CET)","from mail-wm1-x330.google.com (mail-wm1-x330.google.com\n\t[IPv6:2a00:1450:4864:20::330])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BDF9E60231\n\tfor ;\n\tThu, 25 Nov 2021 17:36:51 +0100 (CET)","by mail-wm1-x330.google.com with SMTP id\n\tj140-20020a1c2392000000b003399ae48f58so8762916wmj.5\n\tfor ;\n\tThu, 25 Nov 2021 08:36:51 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"JGGD+Ijo\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=EC6cthaE5O4YxuHAbgSDqbpjJSNZiyOHzS6/E/PJkMg=;\n\tb=JGGD+IjoE1j0KJ5OvP2gjobuUNUzW6VNdGFYgPI7e+5m7m/fK54EbVSZeqfrwfSpY6\n\tNqX55BzbownhNug3IpGGDG9mt2haH179sWSYMRVtkdj9BrPYzDNWnIVfnlvXOyzjUFLN\n\tMsAuMEiwgRBCIZPvl5S2S2P3PD9EJrClxQPJ44ggl3Cf0BslEgQ/peXo5+K6U1W41Ysu\n\tyXCAqGBXxR3tlWsWBeOBLpaXSNP4wBD6c/56QIMoSoxMJVdW2+0KVuDiAJnv11CJ8j56\n\t9oxlG/it6Nat2yNOP12ntiQ1bGSucFZs/bGVHMPmx/DBUR6LU6tfAA6eh4Bq88vXznPR\n\t5/Zw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=EC6cthaE5O4YxuHAbgSDqbpjJSNZiyOHzS6/E/PJkMg=;\n\tb=mSPBR0cGPA2nRhzI23L1D6Qyoc0rdiZtQ2/iqoq+Io29yMW6tca32xnidmAON/34KT\n\tbSwpLnXQHXWlgGENQajc6cbOM9Nwf3JrZOOCu0K8l6zcdDnJsmJZd4/91aOCM+Wq7Uiv\n\tRNJg+U5C38nuYpjNPcNdyfHnHod6Egsp/ampNNURV5z9+Ao1diz5BIBwopPg7KLS2wCx\n\tEzj6F97lzEKmEdw+vAIRjgw8eJI8UD2KKvnV4Gvh1zWEYjl1i/ED1rXkegrVSZvNCI8T\n\tCTI3isD4snyM4+iZyQcM36p8aOhBukkgFoQOqOBEvGboQV9kGAdQRlucYGli9ZY7yJYz\n\t3syg==","X-Gm-Message-State":"AOAM533/H/TsTvdrWzZkE7Ufm+KtoKwX21euQAyS5XDGDXuvizhjTnsO\n\t18SY+vA1lu7rtaG/Pt7yS+22uaGuzhkWQfyXxyxHlQ==","X-Google-Smtp-Source":"ABdhPJwRmVInF3rH8pDEpGy+MbGY7KDn137umAW3zZVbCoMPflH8DAK+fhpaMvG1mr6ExE230WwTyrU36/ctqKhgJ5w=","X-Received":"by 2002:a05:600c:c1:: with SMTP id\n\tu1mr8808317wmm.163.1637858211218; \n\tThu, 25 Nov 2021 08:36:51 -0800 (PST)","MIME-Version":"1.0","References":"<20211118151933.15627-1-david.plowman@raspberrypi.com>\n\t<20211118151933.15627-5-david.plowman@raspberrypi.com>\n\t<20211124234258.vart2hjbrjlqwh3r@uno.localdomain>","In-Reply-To":"<20211124234258.vart2hjbrjlqwh3r@uno.localdomain>","From":"David Plowman ","Date":"Thu, 25 Nov 2021 16:36:40 +0000","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v6 4/7] libcamera: Support passing\n\tColorSpaces to V4L2 video devices","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"Tomasz Figa ,\n\tlibcamera devel ,\n\tHans Verkuil ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":21258,"web_url":"https://patchwork.libcamera.org/comment/21258/","msgid":"<20211125171902.632gcjkwzaevxfc2@uno.localdomain>","date":"2021-11-25T17:19:02","subject":"Re: [libcamera-devel] [PATCH v6 4/7] libcamera: Support passing\n\tColorSpaces to V4L2 video devices","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi David,\n one more comment and a reply\n\nOn Thu, Nov 25, 2021 at 04:36:40PM +0000, David Plowman wrote:\n> Hi Jacopo\n>\n> Thanks for looking at this!\n>\n> On Wed, 24 Nov 2021 at 23:42, Jacopo Mondi wrote:\n> >\n> > Hi David,\n> >\n> > On Thu, Nov 18, 2021 at 03:19:30PM +0000, David Plowman wrote:\n> > > The ColorSpace from the StreamConfiguration is now handled\n> > > appropriately in the V4L2VideoDevice.\n> > >\n> > > Signed-off-by: David Plowman \n> > > ---\n> > > include/libcamera/internal/v4l2_videodevice.h | 2 +\n> > > src/libcamera/v4l2_videodevice.cpp | 53 +++++++++++++++++--\n> > > 2 files changed, 51 insertions(+), 4 deletions(-)\n> > >\n> > > diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h\n> > > index a1c458e4..00bc50e7 100644\n> > > --- a/include/libcamera/internal/v4l2_videodevice.h\n> > > +++ b/include/libcamera/internal/v4l2_videodevice.h\n> > > @@ -20,6 +20,7 @@\n> > > #include \n> > > #include \n> > >\n> > > +#include \n> > > #include \n> > > #include \n> > > #include \n> > > @@ -167,6 +168,7 @@ public:\n> > >\n> > > V4L2PixelFormat fourcc;\n> > > Size size;\n> > > + std::optional colorSpace;\n> > >\n> > > std::array planes;\n> > > unsigned int planesCount = 0;\n> > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> > > index 4f04212d..e03ff8b5 100644\n> > > --- a/src/libcamera/v4l2_videodevice.cpp\n> > > +++ b/src/libcamera/v4l2_videodevice.cpp\n> > > @@ -370,17 +370,27 @@ bool V4L2BufferCache::Entry::operator==(const FrameBuffer &buffer) const\n> > > * \\brief The plane line stride (in bytes)\n> > > */\n> > >\n> > > +/**\n> > > + * \\var V4L2DeviceFormat::fourcc\n> > > + * \\brief The fourcc code describing the pixel encoding scheme\n> > > + *\n> > > + * The fourcc code, as defined by the V4L2 API with the V4L2_PIX_FMT_* macros,\n> > > + * that identifies the image format pixel encoding scheme.\n> > > + */\n> > > +\n> >\n> > Nice that the uncomplete documentation about color spaces has been\n> > dropped, but why moving the documentation block above ? Please keep\n> > the same order as the declaration order in the header file.\n> >\n> > It will also make it easier to diff it during review.\n> >\n> > > /**\n> > > * \\var V4L2DeviceFormat::size\n> > > * \\brief The image size in pixels\n> > > */\n> > >\n> > > /**\n> > > - * \\var V4L2DeviceFormat::fourcc\n> > > - * \\brief The fourcc code describing the pixel encoding scheme\n> > > + * \\var V4L2DeviceFormat::colorSpace\n> > > + * \\brief The color space of the pixels\n> > > *\n> > > - * The fourcc code, as defined by the V4L2 API with the V4L2_PIX_FMT_* macros,\n> > > - * that identifies the image format pixel encoding scheme.\n> > > + * The color space of the image. When setting the format this may be\n> > > + * unset, in which case the driver gets to use its default color space.\n> > > + * After being set, this value should contain the color space that the\n> >\n> > s/that the/that ?\n> >\n> > > + * was actually used.\n> > > */\n> > >\n> > > /**\n> > > @@ -879,6 +889,10 @@ int V4L2VideoDevice::getFormatMultiplane(V4L2DeviceFormat *format)\n> > > format->fourcc = V4L2PixelFormat(pix->pixelformat);\n> > > format->planesCount = pix->num_planes;\n> > >\n> > > + format->colorSpace = toColorSpace(*pix);\n> > > + if (!format->colorSpace)\n> > > + LOG(V4L2, Error) << \"Retrieved unrecognised color space\";\n> > > +\n> > > for (unsigned int i = 0; i < format->planesCount; ++i) {\n> > > format->planes[i].bpl = pix->plane_fmt[i].bytesperline;\n> > > format->planes[i].size = pix->plane_fmt[i].sizeimage;\n> > > @@ -893,6 +907,9 @@ int V4L2VideoDevice::trySetFormatMultiplane(V4L2DeviceFormat *format, bool set)\n> > > struct v4l2_pix_format_mplane *pix = &v4l2Format.fmt.pix_mp;\n> > > int ret;\n> > >\n> > > + if (!format->colorSpace)\n> > > + LOG(V4L2, Warning) << \"Setting undefined color space\";\n> > > +\n> > > v4l2Format.type = bufferType_;\n> > > pix->width = format->size.width;\n> > > pix->height = format->size.height;\n> > > @@ -900,6 +917,12 @@ int V4L2VideoDevice::trySetFormatMultiplane(V4L2DeviceFormat *format, bool set)\n> > > pix->num_planes = format->planesCount;\n> > > pix->field = V4L2_FIELD_NONE;\n> > >\n> > > + ret = fromColorSpace(format->colorSpace, *pix);\n\nLooking at how this is used it really looks like this could be called\n\n ret = fillV4L2ColorSpace(pix, format->colorSpace);\n\n> > > + if (ret < 0)\n> > > + LOG(V4L2, Warning)\n> > > + << \"Setting color space unrecognised by V4L2: \"\n> > > + << ColorSpace::toString(format->colorSpace);\n> > > +\n> > > ASSERT(pix->num_planes <= std::size(pix->plane_fmt));\n> > >\n> > > for (unsigned int i = 0; i < pix->num_planes; ++i) {\n> > > @@ -928,6 +951,10 @@ int V4L2VideoDevice::trySetFormatMultiplane(V4L2DeviceFormat *format, bool set)\n> > > format->planes[i].size = pix->plane_fmt[i].sizeimage;\n> > > }\n> > >\n> > > + format->colorSpace = toColorSpace(*pix);\n> > > + if (!format->colorSpace)\n> > > + LOG(V4L2, Warning) << \"Unrecognised color space has been set\";\n> > > +\n> >\n> > Does colorspace handling stricly have to be tied to format\n> > configuration ? This would lead to undesired side effects, like the metadata pad\n> > being poked for colorspace and complaining as it doesn't really have\n> > one.\n> >\n> > Should colorspace configuration be achieved with dedicated functions\n> > in the V4L2*Device classes so that pipeline handlers know what devices\n> > to apply/retrieve colorspace from, like they do with formats ?\n>\n> Yes, there is a problem here that I talked about in one of the earlier\n> revisions. Actually it's only an issue for subdevices, so you might\n> notice that there's a \"todo\" in v4l2_subdevice.cpp getFormat.\n>\n> The problem is that I end up trying to convert the colour space on a\n> metadata pad - and of course that doesn't work. Nothing breaks, you\n\nI'm aware of the issue and it means to me the implemented API falls a\nbit short if it causes annoyances with a standard use case like this\none.\n\nMy proposal was to leave set/getFormat() in their current version in\nV4L2VideoDevice and V4L2Subdevice and add a new setColorSpace() or\nsimilar which the pipeline handlers would be in charge to call on\nthe devices it consider appropriate.\n\nThis would separate the ColorSpace configuration from the image format\nconfiguration. In V4L2 they go through the same IOCTL and the same\nstructure, but if that doesn't work well for us we can deviate from\nit.\n\nAlso looking at how colorspaces are validated in your last patch\n\n int ret = dev->tryFormat(&format);\n if (ret)\n return Invalid;\n+ if (!format.colorSpace || cfg.colorSpace != format.colorSpace) {\n+ status = Adjusted;\n+ LOG(RPI, Warning)\n+ << \"Color space changed from \"\n+ << ColorSpace::toString(cfg.colorSpace) << \" to \"\n+ << ColorSpace::toString(format.colorSpace);\n+ }\n+ cfg.colorSpace = format.colorSpace;\n\nThis really seems like it could be\n\n int ret = dev->tryFormat(&format);\n if (ret)\n return Invalid;\n\n ret = dev->setColorSpace(cfg.colorSpace);\n if (cfg.colorSpace != dev->getColorSpace()) {\n status = Adjusted;\n cfg.colorSpace = dev->getColorSpace();\n }\n\n\nTrue, setting an image format resets the colorspace maybe but I think\nit's fine as long as it is documented.\n\nIf in the setColorSpace() function you need a v4l2_format you can use\nthe currently applied format. Of course you would need to add the\nsingle planar/multiplanar V4L2 API behind the single setColorSpace()\nfunction.\n\nIn my mental picture there's nothing that prevents this from being\nimplemented (not that I'm saying it's a good idea, but it's doable at\nlast), am I wrong ?\n\n> just get a warning message. I'm open to suggestions on how to resolve\n> this. An extra parameter to the get/setFormat (\"I am not an image data\n> pad\"?). Separate functions get/setImageFormat and\n> get/setMetadataFormat? Rely on the caller to check for a bad colour\n> space? (but will they bother?)\n>\n> Thoughts, anyone?\n>\n> Thanks!\n> David\n>\n> >\n> > > return 0;\n> > > }\n> > >\n> > > @@ -951,6 +978,10 @@ int V4L2VideoDevice::getFormatSingleplane(V4L2DeviceFormat *format)\n> > > format->planes[0].bpl = pix->bytesperline;\n> > > format->planes[0].size = pix->sizeimage;\n> > >\n> > > + format->colorSpace = toColorSpace(*pix);\n> > > + if (!format->colorSpace)\n> > > + LOG(V4L2, Error) << \"Retrieved unrecognised color space\";\n> > > +\n> > > return 0;\n> > > }\n> > >\n> > > @@ -960,12 +991,22 @@ int V4L2VideoDevice::trySetFormatSingleplane(V4L2DeviceFormat *format, bool set)\n> > > struct v4l2_pix_format *pix = &v4l2Format.fmt.pix;\n> > > int ret;\n> > >\n> > > + if (!format->colorSpace)\n> > > + LOG(V4L2, Error) << \"Trying to set unrecognised color space\";\n> > > +\n> > > v4l2Format.type = bufferType_;\n> > > pix->width = format->size.width;\n> > > pix->height = format->size.height;\n> > > pix->pixelformat = format->fourcc;\n> > > pix->bytesperline = format->planes[0].bpl;\n> > > pix->field = V4L2_FIELD_NONE;\n> > > +\n> > > + ret = fromColorSpace(format->colorSpace, *pix);\n> > > + if (ret < 0)\n> > > + LOG(V4L2, Warning)\n> > > + << \"Set color space unrecognised by V4L2: \"\n> > > + << ColorSpace::toString(format->colorSpace);\n> > > +\n> > > ret = ioctl(set ? VIDIOC_S_FMT : VIDIOC_TRY_FMT, &v4l2Format);\n> > > if (ret) {\n> > > LOG(V4L2, Error)\n> > > @@ -985,6 +1026,10 @@ int V4L2VideoDevice::trySetFormatSingleplane(V4L2DeviceFormat *format, bool set)\n> > > format->planes[0].bpl = pix->bytesperline;\n> > > format->planes[0].size = pix->sizeimage;\n> > >\n> > > + format->colorSpace = toColorSpace(*pix);\n> > > + if (!format->colorSpace)\n> > > + LOG(V4L2, Error) << \"Undefined color space has been set\";\n> > > +\n> > > return 0;\n> > > }\n> > >\n> > > --\n> > > 2.20.1\n> > >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 0C895BDB13\n\tfor ;\n\tThu, 25 Nov 2021 17:18:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4D9D960376;\n\tThu, 25 Nov 2021 18:18:14 +0100 (CET)","from relay6-d.mail.gandi.net (relay6-d.mail.gandi.net\n\t[217.70.183.198])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BEC4F60231\n\tfor ;\n\tThu, 25 Nov 2021 18:18:12 +0100 (CET)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay6-d.mail.gandi.net (Postfix) with ESMTPSA id 1F031C000D;\n\tThu, 25 Nov 2021 17:18:10 +0000 (UTC)"],"Date":"Thu, 25 Nov 2021 18:19:02 +0100","From":"Jacopo Mondi ","To":"David Plowman ","Message-ID":"<20211125171902.632gcjkwzaevxfc2@uno.localdomain>","References":"<20211118151933.15627-1-david.plowman@raspberrypi.com>\n\t<20211118151933.15627-5-david.plowman@raspberrypi.com>\n\t<20211124234258.vart2hjbrjlqwh3r@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v6 4/7] libcamera: Support passing\n\tColorSpaces to V4L2 video devices","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"Tomasz Figa ,\n\tlibcamera devel ,\n\tHans Verkuil ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":21259,"web_url":"https://patchwork.libcamera.org/comment/21259/","msgid":"","date":"2021-11-25T20:38:25","subject":"Re: [libcamera-devel] [PATCH v6 4/7] libcamera: Support passing\n\tColorSpaces to V4L2 video devices","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Jacopo\n\nThanks for the suggestions.\n\nOn Thu, 25 Nov 2021 at 17:18, Jacopo Mondi wrote:\n>\n> Hi David,\n> one more comment and a reply\n>\n> On Thu, Nov 25, 2021 at 04:36:40PM +0000, David Plowman wrote:\n> > Hi Jacopo\n> >\n> > Thanks for looking at this!\n> >\n> > On Wed, 24 Nov 2021 at 23:42, Jacopo Mondi wrote:\n> > >\n> > > Hi David,\n> > >\n> > > On Thu, Nov 18, 2021 at 03:19:30PM +0000, David Plowman wrote:\n> > > > The ColorSpace from the StreamConfiguration is now handled\n> > > > appropriately in the V4L2VideoDevice.\n> > > >\n> > > > Signed-off-by: David Plowman \n> > > > ---\n> > > > include/libcamera/internal/v4l2_videodevice.h | 2 +\n> > > > src/libcamera/v4l2_videodevice.cpp | 53 +++++++++++++++++--\n> > > > 2 files changed, 51 insertions(+), 4 deletions(-)\n> > > >\n> > > > diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h\n> > > > index a1c458e4..00bc50e7 100644\n> > > > --- a/include/libcamera/internal/v4l2_videodevice.h\n> > > > +++ b/include/libcamera/internal/v4l2_videodevice.h\n> > > > @@ -20,6 +20,7 @@\n> > > > #include \n> > > > #include \n> > > >\n> > > > +#include \n> > > > #include \n> > > > #include \n> > > > #include \n> > > > @@ -167,6 +168,7 @@ public:\n> > > >\n> > > > V4L2PixelFormat fourcc;\n> > > > Size size;\n> > > > + std::optional colorSpace;\n> > > >\n> > > > std::array planes;\n> > > > unsigned int planesCount = 0;\n> > > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> > > > index 4f04212d..e03ff8b5 100644\n> > > > --- a/src/libcamera/v4l2_videodevice.cpp\n> > > > +++ b/src/libcamera/v4l2_videodevice.cpp\n> > > > @@ -370,17 +370,27 @@ bool V4L2BufferCache::Entry::operator==(const FrameBuffer &buffer) const\n> > > > * \\brief The plane line stride (in bytes)\n> > > > */\n> > > >\n> > > > +/**\n> > > > + * \\var V4L2DeviceFormat::fourcc\n> > > > + * \\brief The fourcc code describing the pixel encoding scheme\n> > > > + *\n> > > > + * The fourcc code, as defined by the V4L2 API with the V4L2_PIX_FMT_* macros,\n> > > > + * that identifies the image format pixel encoding scheme.\n> > > > + */\n> > > > +\n> > >\n> > > Nice that the uncomplete documentation about color spaces has been\n> > > dropped, but why moving the documentation block above ? Please keep\n> > > the same order as the declaration order in the header file.\n> > >\n> > > It will also make it easier to diff it during review.\n> > >\n> > > > /**\n> > > > * \\var V4L2DeviceFormat::size\n> > > > * \\brief The image size in pixels\n> > > > */\n> > > >\n> > > > /**\n> > > > - * \\var V4L2DeviceFormat::fourcc\n> > > > - * \\brief The fourcc code describing the pixel encoding scheme\n> > > > + * \\var V4L2DeviceFormat::colorSpace\n> > > > + * \\brief The color space of the pixels\n> > > > *\n> > > > - * The fourcc code, as defined by the V4L2 API with the V4L2_PIX_FMT_* macros,\n> > > > - * that identifies the image format pixel encoding scheme.\n> > > > + * The color space of the image. When setting the format this may be\n> > > > + * unset, in which case the driver gets to use its default color space.\n> > > > + * After being set, this value should contain the color space that the\n> > >\n> > > s/that the/that ?\n> > >\n> > > > + * was actually used.\n> > > > */\n> > > >\n> > > > /**\n> > > > @@ -879,6 +889,10 @@ int V4L2VideoDevice::getFormatMultiplane(V4L2DeviceFormat *format)\n> > > > format->fourcc = V4L2PixelFormat(pix->pixelformat);\n> > > > format->planesCount = pix->num_planes;\n> > > >\n> > > > + format->colorSpace = toColorSpace(*pix);\n> > > > + if (!format->colorSpace)\n> > > > + LOG(V4L2, Error) << \"Retrieved unrecognised color space\";\n> > > > +\n> > > > for (unsigned int i = 0; i < format->planesCount; ++i) {\n> > > > format->planes[i].bpl = pix->plane_fmt[i].bytesperline;\n> > > > format->planes[i].size = pix->plane_fmt[i].sizeimage;\n> > > > @@ -893,6 +907,9 @@ int V4L2VideoDevice::trySetFormatMultiplane(V4L2DeviceFormat *format, bool set)\n> > > > struct v4l2_pix_format_mplane *pix = &v4l2Format.fmt.pix_mp;\n> > > > int ret;\n> > > >\n> > > > + if (!format->colorSpace)\n> > > > + LOG(V4L2, Warning) << \"Setting undefined color space\";\n> > > > +\n> > > > v4l2Format.type = bufferType_;\n> > > > pix->width = format->size.width;\n> > > > pix->height = format->size.height;\n> > > > @@ -900,6 +917,12 @@ int V4L2VideoDevice::trySetFormatMultiplane(V4L2DeviceFormat *format, bool set)\n> > > > pix->num_planes = format->planesCount;\n> > > > pix->field = V4L2_FIELD_NONE;\n> > > >\n> > > > + ret = fromColorSpace(format->colorSpace, *pix);\n>\n> Looking at how this is used it really looks like this could be called\n>\n> ret = fillV4L2ColorSpace(pix, format->colorSpace);\n>\n> > > > + if (ret < 0)\n> > > > + LOG(V4L2, Warning)\n> > > > + << \"Setting color space unrecognised by V4L2: \"\n> > > > + << ColorSpace::toString(format->colorSpace);\n> > > > +\n> > > > ASSERT(pix->num_planes <= std::size(pix->plane_fmt));\n> > > >\n> > > > for (unsigned int i = 0; i < pix->num_planes; ++i) {\n> > > > @@ -928,6 +951,10 @@ int V4L2VideoDevice::trySetFormatMultiplane(V4L2DeviceFormat *format, bool set)\n> > > > format->planes[i].size = pix->plane_fmt[i].sizeimage;\n> > > > }\n> > > >\n> > > > + format->colorSpace = toColorSpace(*pix);\n> > > > + if (!format->colorSpace)\n> > > > + LOG(V4L2, Warning) << \"Unrecognised color space has been set\";\n> > > > +\n> > >\n> > > Does colorspace handling stricly have to be tied to format\n> > > configuration ? This would lead to undesired side effects, like the metadata pad\n> > > being poked for colorspace and complaining as it doesn't really have\n> > > one.\n> > >\n> > > Should colorspace configuration be achieved with dedicated functions\n> > > in the V4L2*Device classes so that pipeline handlers know what devices\n> > > to apply/retrieve colorspace from, like they do with formats ?\n> >\n> > Yes, there is a problem here that I talked about in one of the earlier\n> > revisions. Actually it's only an issue for subdevices, so you might\n> > notice that there's a \"todo\" in v4l2_subdevice.cpp getFormat.\n> >\n> > The problem is that I end up trying to convert the colour space on a\n> > metadata pad - and of course that doesn't work. Nothing breaks, you\n>\n> I'm aware of the issue and it means to me the implemented API falls a\n> bit short if it causes annoyances with a standard use case like this\n> one.\n>\n> My proposal was to leave set/getFormat() in their current version in\n> V4L2VideoDevice and V4L2Subdevice and add a new setColorSpace() or\n> similar which the pipeline handlers would be in charge to call on\n> the devices it consider appropriate.\n>\n> This would separate the ColorSpace configuration from the image format\n> configuration. In V4L2 they go through the same IOCTL and the same\n> structure, but if that doesn't work well for us we can deviate from\n> it.\n>\n> Also looking at how colorspaces are validated in your last patch\n>\n> int ret = dev->tryFormat(&format);\n> if (ret)\n> return Invalid;\n> + if (!format.colorSpace || cfg.colorSpace != format.colorSpace) {\n> + status = Adjusted;\n> + LOG(RPI, Warning)\n> + << \"Color space changed from \"\n> + << ColorSpace::toString(cfg.colorSpace) << \" to \"\n> + << ColorSpace::toString(format.colorSpace);\n> + }\n> + cfg.colorSpace = format.colorSpace;\n>\n> This really seems like it could be\n>\n> int ret = dev->tryFormat(&format);\n> if (ret)\n> return Invalid;\n>\n> ret = dev->setColorSpace(cfg.colorSpace);\n> if (cfg.colorSpace != dev->getColorSpace()) {\n> status = Adjusted;\n> cfg.colorSpace = dev->getColorSpace();\n> }\n>\n>\n> True, setting an image format resets the colorspace maybe but I think\n> it's fine as long as it is documented.\n>\n> If in the setColorSpace() function you need a v4l2_format you can use\n> the currently applied format. Of course you would need to add the\n> single planar/multiplanar V4L2 API behind the single setColorSpace()\n> function.\n>\n> In my mental picture there's nothing that prevents this from being\n> implemented (not that I'm saying it's a good idea, but it's doable at\n> last), am I wrong ?\n\nSo I agree that it could be done like this, but I guess the following\nthings bother me a bit.\n\nMainly it seems more complicated (you have to do 2 things all the\ntime, not just one - source of bugs right there). It confounds\nexisting expectations about V4L2 - why confuse people who thought they\nknew how V4L2 works? This is a \"helpful C++ layer\" on top of V4L2,\nright? And the actual mechanics of splitting this up - every \"set\"\noperation becomes a \"query-first-then-set\"- is not lovely.\n\nActually I think there's a perfectly good solution which is simply to\nlet the caller check the result. I know I get obsessed about colour\nspaces, but apparently not everyone does! This way everyone gets what\nthey want. I can check. Others can check if they want. All problems\nsolved.\n\nDavid\n\n>\n> > just get a warning message. I'm open to suggestions on how to resolve\n> > this. An extra parameter to the get/setFormat (\"I am not an image data\n> > pad\"?). Separate functions get/setImageFormat and\n> > get/setMetadataFormat? Rely on the caller to check for a bad colour\n> > space? (but will they bother?)\n> >\n> > Thoughts, anyone?\n> >\n> > Thanks!\n> > David\n> >\n> > >\n> > > > return 0;\n> > > > }\n> > > >\n> > > > @@ -951,6 +978,10 @@ int V4L2VideoDevice::getFormatSingleplane(V4L2DeviceFormat *format)\n> > > > format->planes[0].bpl = pix->bytesperline;\n> > > > format->planes[0].size = pix->sizeimage;\n> > > >\n> > > > + format->colorSpace = toColorSpace(*pix);\n> > > > + if (!format->colorSpace)\n> > > > + LOG(V4L2, Error) << \"Retrieved unrecognised color space\";\n> > > > +\n> > > > return 0;\n> > > > }\n> > > >\n> > > > @@ -960,12 +991,22 @@ int V4L2VideoDevice::trySetFormatSingleplane(V4L2DeviceFormat *format, bool set)\n> > > > struct v4l2_pix_format *pix = &v4l2Format.fmt.pix;\n> > > > int ret;\n> > > >\n> > > > + if (!format->colorSpace)\n> > > > + LOG(V4L2, Error) << \"Trying to set unrecognised color space\";\n> > > > +\n> > > > v4l2Format.type = bufferType_;\n> > > > pix->width = format->size.width;\n> > > > pix->height = format->size.height;\n> > > > pix->pixelformat = format->fourcc;\n> > > > pix->bytesperline = format->planes[0].bpl;\n> > > > pix->field = V4L2_FIELD_NONE;\n> > > > +\n> > > > + ret = fromColorSpace(format->colorSpace, *pix);\n> > > > + if (ret < 0)\n> > > > + LOG(V4L2, Warning)\n> > > > + << \"Set color space unrecognised by V4L2: \"\n> > > > + << ColorSpace::toString(format->colorSpace);\n> > > > +\n> > > > ret = ioctl(set ? VIDIOC_S_FMT : VIDIOC_TRY_FMT, &v4l2Format);\n> > > > if (ret) {\n> > > > LOG(V4L2, Error)\n> > > > @@ -985,6 +1026,10 @@ int V4L2VideoDevice::trySetFormatSingleplane(V4L2DeviceFormat *format, bool set)\n> > > > format->planes[0].bpl = pix->bytesperline;\n> > > > format->planes[0].size = pix->sizeimage;\n> > > >\n> > > > + format->colorSpace = toColorSpace(*pix);\n> > > > + if (!format->colorSpace)\n> > > > + LOG(V4L2, Error) << \"Undefined color space has been set\";\n> > > > +\n> > > > return 0;\n> > > > }\n> > > >\n> > > > --\n> > > > 2.20.1\n> > > >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id B55DCBF415\n\tfor ;\n\tThu, 25 Nov 2021 20:38:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DC5716036F;\n\tThu, 25 Nov 2021 21:38:38 +0100 (CET)","from mail-wr1-x42a.google.com (mail-wr1-x42a.google.com\n\t[IPv6:2a00:1450:4864:20::42a])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CC00C60231\n\tfor ;\n\tThu, 25 Nov 2021 21:38:36 +0100 (CET)","by mail-wr1-x42a.google.com with SMTP id s13so14135873wrb.3\n\tfor ;\n\tThu, 25 Nov 2021 12:38:36 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"PW5nOtY8\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=p89sCEW/yRFT0KsLQT+104dz9MutLw9+CWxiczgi1v8=;\n\tb=PW5nOtY8FcZPH9qvoH0RYSm6jSheS0mgN/4Ef2j9of8MjJiX/C0ktV/1IBquTgxOoo\n\tLrFpq1h3VcSw/gvhRRGeDRbvnHQwlwG4SeVD3SUNt26Ru1iilARawcXIOCldTfonylDU\n\t8oRXMmAQnHxochK4zvhvqDmAQM3UFBUGLZvPLIRMdcj8Lhw+o4OOwMP69ui1adqmribH\n\tr23tBp2ndvAfUn0LgKbBS8LQhLss95gvy8hrWxYy3ksIqxCvj0XcgagufSKpWa0nNB2R\n\t4nheUskPcR2aX07j4wvmQAv4Sn35n8h+tFjs3OSxt1PO01kOSctwoLUOo/P/d1idqJJK\n\tk9mQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=p89sCEW/yRFT0KsLQT+104dz9MutLw9+CWxiczgi1v8=;\n\tb=rdc/a1YSqQr+12BiXbomMjmiudASseS2tc2XztJrBDlSpdKTQ0pfbAc68STISjX3Es\n\ti5E0A15eqKkMXGOlFRC/mlHsmmEJLX4hT8yywPhaO5xI/ourIFgftuD/oMfpCJmNRkjP\n\t7ko64AsOeSm9Ockia9NJynfiPYeT7lh9iPD0/odNwHpnGxB0r5wsf8cmdVFnQ5ngyEqJ\n\tlT2BiKoWJm5um+ruzwuZLOUs1Yw6DF9JPFcVCkBs8DkaIIZSX4mYbVLLQHTOx2fiu8Ej\n\t5vnfBqCGuY6KnmD3siEJhUuB5aEbfXM8rUHygkFILZapVs1BxhOx9n7p/qY71Nam6RCE\n\tqz3w==","X-Gm-Message-State":"AOAM531OCPzq53vRMTa4VGBwOoHJIiicYhcmLkNsIT211hB2m8dCXD+l\n\tUracgfY0wxa8rNy631AJY+BArK2QM99ZHUDecBYM+g==","X-Google-Smtp-Source":"ABdhPJygI9XxPJhV6NIl2qPajQbOpn//eGbb8tU2fTc6DJ8DBE3DDugr4UvGwr8ZAcYQCMI1dUZZypkMUEIZySfO1I0=","X-Received":"by 2002:adf:f44e:: with SMTP id f14mr9938608wrp.37.1637872716225;\n\tThu, 25 Nov 2021 12:38:36 -0800 (PST)","MIME-Version":"1.0","References":"<20211118151933.15627-1-david.plowman@raspberrypi.com>\n\t<20211118151933.15627-5-david.plowman@raspberrypi.com>\n\t<20211124234258.vart2hjbrjlqwh3r@uno.localdomain>\n\t\n\t<20211125171902.632gcjkwzaevxfc2@uno.localdomain>","In-Reply-To":"<20211125171902.632gcjkwzaevxfc2@uno.localdomain>","From":"David Plowman ","Date":"Thu, 25 Nov 2021 20:38:25 +0000","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v6 4/7] libcamera: Support passing\n\tColorSpaces to V4L2 video devices","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"Tomasz Figa ,\n\tlibcamera devel ,\n\tHans Verkuil ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":21269,"web_url":"https://patchwork.libcamera.org/comment/21269/","msgid":"<20211126082104.rxuh2dzonao4wnat@uno.localdomain>","date":"2021-11-26T08:21:04","subject":"Re: [libcamera-devel] [PATCH v6 4/7] libcamera: Support passing\n\tColorSpaces to V4L2 video devices","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi David,\n\nOn Thu, Nov 25, 2021 at 08:38:25PM +0000, David Plowman wrote:\n> Hi Jacopo\n>\n> Thanks for the suggestions.\n>\n> On Thu, 25 Nov 2021 at 17:18, Jacopo Mondi wrote:\n> >\n> > Hi David,\n> > one more comment and a reply\n> >\n> > On Thu, Nov 25, 2021 at 04:36:40PM +0000, David Plowman wrote:\n> > > Hi Jacopo\n> > >\n> > > Thanks for looking at this!\n> > >\n> > > On Wed, 24 Nov 2021 at 23:42, Jacopo Mondi wrote:\n> > > >\n> > > > Hi David,\n> > > >\n> > > > On Thu, Nov 18, 2021 at 03:19:30PM +0000, David Plowman wrote:\n> > > > > The ColorSpace from the StreamConfiguration is now handled\n> > > > > appropriately in the V4L2VideoDevice.\n> > > > >\n> > > > > Signed-off-by: David Plowman \n> > > > > ---\n> > > > > include/libcamera/internal/v4l2_videodevice.h | 2 +\n> > > > > src/libcamera/v4l2_videodevice.cpp | 53 +++++++++++++++++--\n> > > > > 2 files changed, 51 insertions(+), 4 deletions(-)\n> > > > >\n> > > > > diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h\n> > > > > index a1c458e4..00bc50e7 100644\n> > > > > --- a/include/libcamera/internal/v4l2_videodevice.h\n> > > > > +++ b/include/libcamera/internal/v4l2_videodevice.h\n> > > > > @@ -20,6 +20,7 @@\n> > > > > #include \n> > > > > #include \n> > > > >\n> > > > > +#include \n> > > > > #include \n> > > > > #include \n> > > > > #include \n> > > > > @@ -167,6 +168,7 @@ public:\n> > > > >\n> > > > > V4L2PixelFormat fourcc;\n> > > > > Size size;\n> > > > > + std::optional colorSpace;\n> > > > >\n> > > > > std::array planes;\n> > > > > unsigned int planesCount = 0;\n> > > > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> > > > > index 4f04212d..e03ff8b5 100644\n> > > > > --- a/src/libcamera/v4l2_videodevice.cpp\n> > > > > +++ b/src/libcamera/v4l2_videodevice.cpp\n> > > > > @@ -370,17 +370,27 @@ bool V4L2BufferCache::Entry::operator==(const FrameBuffer &buffer) const\n> > > > > * \\brief The plane line stride (in bytes)\n> > > > > */\n> > > > >\n> > > > > +/**\n> > > > > + * \\var V4L2DeviceFormat::fourcc\n> > > > > + * \\brief The fourcc code describing the pixel encoding scheme\n> > > > > + *\n> > > > > + * The fourcc code, as defined by the V4L2 API with the V4L2_PIX_FMT_* macros,\n> > > > > + * that identifies the image format pixel encoding scheme.\n> > > > > + */\n> > > > > +\n> > > >\n> > > > Nice that the uncomplete documentation about color spaces has been\n> > > > dropped, but why moving the documentation block above ? Please keep\n> > > > the same order as the declaration order in the header file.\n> > > >\n> > > > It will also make it easier to diff it during review.\n> > > >\n> > > > > /**\n> > > > > * \\var V4L2DeviceFormat::size\n> > > > > * \\brief The image size in pixels\n> > > > > */\n> > > > >\n> > > > > /**\n> > > > > - * \\var V4L2DeviceFormat::fourcc\n> > > > > - * \\brief The fourcc code describing the pixel encoding scheme\n> > > > > + * \\var V4L2DeviceFormat::colorSpace\n> > > > > + * \\brief The color space of the pixels\n> > > > > *\n> > > > > - * The fourcc code, as defined by the V4L2 API with the V4L2_PIX_FMT_* macros,\n> > > > > - * that identifies the image format pixel encoding scheme.\n> > > > > + * The color space of the image. When setting the format this may be\n> > > > > + * unset, in which case the driver gets to use its default color space.\n> > > > > + * After being set, this value should contain the color space that the\n> > > >\n> > > > s/that the/that ?\n> > > >\n> > > > > + * was actually used.\n> > > > > */\n> > > > >\n> > > > > /**\n> > > > > @@ -879,6 +889,10 @@ int V4L2VideoDevice::getFormatMultiplane(V4L2DeviceFormat *format)\n> > > > > format->fourcc = V4L2PixelFormat(pix->pixelformat);\n> > > > > format->planesCount = pix->num_planes;\n> > > > >\n> > > > > + format->colorSpace = toColorSpace(*pix);\n> > > > > + if (!format->colorSpace)\n> > > > > + LOG(V4L2, Error) << \"Retrieved unrecognised color space\";\n> > > > > +\n> > > > > for (unsigned int i = 0; i < format->planesCount; ++i) {\n> > > > > format->planes[i].bpl = pix->plane_fmt[i].bytesperline;\n> > > > > format->planes[i].size = pix->plane_fmt[i].sizeimage;\n> > > > > @@ -893,6 +907,9 @@ int V4L2VideoDevice::trySetFormatMultiplane(V4L2DeviceFormat *format, bool set)\n> > > > > struct v4l2_pix_format_mplane *pix = &v4l2Format.fmt.pix_mp;\n> > > > > int ret;\n> > > > >\n> > > > > + if (!format->colorSpace)\n> > > > > + LOG(V4L2, Warning) << \"Setting undefined color space\";\n> > > > > +\n> > > > > v4l2Format.type = bufferType_;\n> > > > > pix->width = format->size.width;\n> > > > > pix->height = format->size.height;\n> > > > > @@ -900,6 +917,12 @@ int V4L2VideoDevice::trySetFormatMultiplane(V4L2DeviceFormat *format, bool set)\n> > > > > pix->num_planes = format->planesCount;\n> > > > > pix->field = V4L2_FIELD_NONE;\n> > > > >\n> > > > > + ret = fromColorSpace(format->colorSpace, *pix);\n> >\n> > Looking at how this is used it really looks like this could be called\n> >\n> > ret = fillV4L2ColorSpace(pix, format->colorSpace);\n> >\n> > > > > + if (ret < 0)\n> > > > > + LOG(V4L2, Warning)\n> > > > > + << \"Setting color space unrecognised by V4L2: \"\n> > > > > + << ColorSpace::toString(format->colorSpace);\n> > > > > +\n> > > > > ASSERT(pix->num_planes <= std::size(pix->plane_fmt));\n> > > > >\n> > > > > for (unsigned int i = 0; i < pix->num_planes; ++i) {\n> > > > > @@ -928,6 +951,10 @@ int V4L2VideoDevice::trySetFormatMultiplane(V4L2DeviceFormat *format, bool set)\n> > > > > format->planes[i].size = pix->plane_fmt[i].sizeimage;\n> > > > > }\n> > > > >\n> > > > > + format->colorSpace = toColorSpace(*pix);\n> > > > > + if (!format->colorSpace)\n> > > > > + LOG(V4L2, Warning) << \"Unrecognised color space has been set\";\n> > > > > +\n> > > >\n> > > > Does colorspace handling stricly have to be tied to format\n> > > > configuration ? This would lead to undesired side effects, like the metadata pad\n> > > > being poked for colorspace and complaining as it doesn't really have\n> > > > one.\n> > > >\n> > > > Should colorspace configuration be achieved with dedicated functions\n> > > > in the V4L2*Device classes so that pipeline handlers know what devices\n> > > > to apply/retrieve colorspace from, like they do with formats ?\n> > >\n> > > Yes, there is a problem here that I talked about in one of the earlier\n> > > revisions. Actually it's only an issue for subdevices, so you might\n> > > notice that there's a \"todo\" in v4l2_subdevice.cpp getFormat.\n> > >\n> > > The problem is that I end up trying to convert the colour space on a\n> > > metadata pad - and of course that doesn't work. Nothing breaks, you\n> >\n> > I'm aware of the issue and it means to me the implemented API falls a\n> > bit short if it causes annoyances with a standard use case like this\n> > one.\n> >\n> > My proposal was to leave set/getFormat() in their current version in\n> > V4L2VideoDevice and V4L2Subdevice and add a new setColorSpace() or\n> > similar which the pipeline handlers would be in charge to call on\n> > the devices it consider appropriate.\n> >\n> > This would separate the ColorSpace configuration from the image format\n> > configuration. In V4L2 they go through the same IOCTL and the same\n> > structure, but if that doesn't work well for us we can deviate from\n> > it.\n> >\n> > Also looking at how colorspaces are validated in your last patch\n> >\n> > int ret = dev->tryFormat(&format);\n> > if (ret)\n> > return Invalid;\n> > + if (!format.colorSpace || cfg.colorSpace != format.colorSpace) {\n> > + status = Adjusted;\n> > + LOG(RPI, Warning)\n> > + << \"Color space changed from \"\n> > + << ColorSpace::toString(cfg.colorSpace) << \" to \"\n> > + << ColorSpace::toString(format.colorSpace);\n> > + }\n> > + cfg.colorSpace = format.colorSpace;\n> >\n> > This really seems like it could be\n> >\n> > int ret = dev->tryFormat(&format);\n> > if (ret)\n> > return Invalid;\n> >\n> > ret = dev->setColorSpace(cfg.colorSpace);\n> > if (cfg.colorSpace != dev->getColorSpace()) {\n> > status = Adjusted;\n> > cfg.colorSpace = dev->getColorSpace();\n> > }\n> >\n> >\n> > True, setting an image format resets the colorspace maybe but I think\n> > it's fine as long as it is documented.\n> >\n> > If in the setColorSpace() function you need a v4l2_format you can use\n> > the currently applied format. Of course you would need to add the\n> > single planar/multiplanar V4L2 API behind the single setColorSpace()\n> > function.\n> >\n> > In my mental picture there's nothing that prevents this from being\n> > implemented (not that I'm saying it's a good idea, but it's doable at\n> > last), am I wrong ?\n>\n> So I agree that it could be done like this, but I guess the following\n> things bother me a bit.\n>\n> Mainly it seems more complicated (you have to do 2 things all the\n> time, not just one - source of bugs right there). It confounds\n\nI don't think it will be source of bugs, but rather something not all\npipeline handlers might do. But we'll have compliance test suites, and\nthis will be caught \"early\"\n\n> existing expectations about V4L2 - why confuse people who thought they\n> knew how V4L2 works? This is a \"helpful C++ layer\" on top of V4L2,\n\nWell, no :)\n\nlibcamera might theoretically exists on top of other abstractions and\nwhen it makes sense to deviate from v4l2 we're free to do so (again,\nif it makes sense)\n\n> right? And the actual mechanics of splitting this up - every \"set\"\n> operation becomes a \"query-first-then-set\"- is not lovely.\n\nThe \"query-first-then-set\" happens in the theoretical\nV4L2*Device::setColorSpace() function or in the pipeline handler code\n?\n\n>\n> Actually I think there's a perfectly good solution which is simply to\n> let the caller check the result. I know I get obsessed about colour\n> spaces, but apparently not everyone does! This way everyone gets what\n> they want. I can check. Others can check if they want. All problems\n> solved.\n\nNot really, as setting a format on a metadata pad creates an error\nmessage, and it doesn't anyway make much sense to configure a\ncolorspace there. As soon as we have devices producing data in\nnon-pixel formats (depth maps etc etc) we'll hit the same issue.\n\nThanks\n j\n\n>\n> David\n>\n> >\n> > > just get a warning message. I'm open to suggestions on how to resolve\n> > > this. An extra parameter to the get/setFormat (\"I am not an image data\n> > > pad\"?). Separate functions get/setImageFormat and\n> > > get/setMetadataFormat? Rely on the caller to check for a bad colour\n> > > space? (but will they bother?)\n> > >\n> > > Thoughts, anyone?\n> > >\n> > > Thanks!\n> > > David\n> > >\n> > > >\n> > > > > return 0;\n> > > > > }\n> > > > >\n> > > > > @@ -951,6 +978,10 @@ int V4L2VideoDevice::getFormatSingleplane(V4L2DeviceFormat *format)\n> > > > > format->planes[0].bpl = pix->bytesperline;\n> > > > > format->planes[0].size = pix->sizeimage;\n> > > > >\n> > > > > + format->colorSpace = toColorSpace(*pix);\n> > > > > + if (!format->colorSpace)\n> > > > > + LOG(V4L2, Error) << \"Retrieved unrecognised color space\";\n> > > > > +\n> > > > > return 0;\n> > > > > }\n> > > > >\n> > > > > @@ -960,12 +991,22 @@ int V4L2VideoDevice::trySetFormatSingleplane(V4L2DeviceFormat *format, bool set)\n> > > > > struct v4l2_pix_format *pix = &v4l2Format.fmt.pix;\n> > > > > int ret;\n> > > > >\n> > > > > + if (!format->colorSpace)\n> > > > > + LOG(V4L2, Error) << \"Trying to set unrecognised color space\";\n> > > > > +\n> > > > > v4l2Format.type = bufferType_;\n> > > > > pix->width = format->size.width;\n> > > > > pix->height = format->size.height;\n> > > > > pix->pixelformat = format->fourcc;\n> > > > > pix->bytesperline = format->planes[0].bpl;\n> > > > > pix->field = V4L2_FIELD_NONE;\n> > > > > +\n> > > > > + ret = fromColorSpace(format->colorSpace, *pix);\n> > > > > + if (ret < 0)\n> > > > > + LOG(V4L2, Warning)\n> > > > > + << \"Set color space unrecognised by V4L2: \"\n> > > > > + << ColorSpace::toString(format->colorSpace);\n> > > > > +\n> > > > > ret = ioctl(set ? VIDIOC_S_FMT : VIDIOC_TRY_FMT, &v4l2Format);\n> > > > > if (ret) {\n> > > > > LOG(V4L2, Error)\n> > > > > @@ -985,6 +1026,10 @@ int V4L2VideoDevice::trySetFormatSingleplane(V4L2DeviceFormat *format, bool set)\n> > > > > format->planes[0].bpl = pix->bytesperline;\n> > > > > format->planes[0].size = pix->sizeimage;\n> > > > >\n> > > > > + format->colorSpace = toColorSpace(*pix);\n> > > > > + if (!format->colorSpace)\n> > > > > + LOG(V4L2, Error) << \"Undefined color space has been set\";\n> > > > > +\n> > > > > return 0;\n> > > > > }\n> > > > >\n> > > > > --\n> > > > > 2.20.1\n> > > > >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 82964BDB13\n\tfor ;\n\tFri, 26 Nov 2021 08:20:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D46556049A;\n\tFri, 26 Nov 2021 09:20:14 +0100 (CET)","from relay6-d.mail.gandi.net (relay6-d.mail.gandi.net\n\t[217.70.183.198])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D59AC60227\n\tfor ;\n\tFri, 26 Nov 2021 09:20:13 +0100 (CET)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay6-d.mail.gandi.net (Postfix) with ESMTPSA id 40CEEC0011;\n\tFri, 26 Nov 2021 08:20:11 +0000 (UTC)"],"Date":"Fri, 26 Nov 2021 09:21:04 +0100","From":"Jacopo Mondi ","To":"David Plowman ","Message-ID":"<20211126082104.rxuh2dzonao4wnat@uno.localdomain>","References":"<20211118151933.15627-1-david.plowman@raspberrypi.com>\n\t<20211118151933.15627-5-david.plowman@raspberrypi.com>\n\t<20211124234258.vart2hjbrjlqwh3r@uno.localdomain>\n\t\n\t<20211125171902.632gcjkwzaevxfc2@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v6 4/7] libcamera: Support passing\n\tColorSpaces to V4L2 video devices","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"Tomasz Figa ,\n\tlibcamera devel ,\n\tHans Verkuil ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":21276,"web_url":"https://patchwork.libcamera.org/comment/21276/","msgid":"","date":"2021-11-26T09:34:04","subject":"Re: [libcamera-devel] [PATCH v6 4/7] libcamera: Support passing\n\tColorSpaces to V4L2 video devices","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Jacopo\n\nOn Fri, 26 Nov 2021 at 08:20, Jacopo Mondi wrote:\n>\n> Hi David,\n>\n> On Thu, Nov 25, 2021 at 08:38:25PM +0000, David Plowman wrote:\n> > Hi Jacopo\n> >\n> > Thanks for the suggestions.\n> >\n> > On Thu, 25 Nov 2021 at 17:18, Jacopo Mondi wrote:\n> > >\n> > > Hi David,\n> > > one more comment and a reply\n> > >\n> > > On Thu, Nov 25, 2021 at 04:36:40PM +0000, David Plowman wrote:\n> > > > Hi Jacopo\n> > > >\n> > > > Thanks for looking at this!\n> > > >\n> > > > On Wed, 24 Nov 2021 at 23:42, Jacopo Mondi wrote:\n> > > > >\n> > > > > Hi David,\n> > > > >\n> > > > > On Thu, Nov 18, 2021 at 03:19:30PM +0000, David Plowman wrote:\n> > > > > > The ColorSpace from the StreamConfiguration is now handled\n> > > > > > appropriately in the V4L2VideoDevice.\n> > > > > >\n> > > > > > Signed-off-by: David Plowman \n> > > > > > ---\n> > > > > > include/libcamera/internal/v4l2_videodevice.h | 2 +\n> > > > > > src/libcamera/v4l2_videodevice.cpp | 53 +++++++++++++++++--\n> > > > > > 2 files changed, 51 insertions(+), 4 deletions(-)\n> > > > > >\n> > > > > > diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h\n> > > > > > index a1c458e4..00bc50e7 100644\n> > > > > > --- a/include/libcamera/internal/v4l2_videodevice.h\n> > > > > > +++ b/include/libcamera/internal/v4l2_videodevice.h\n> > > > > > @@ -20,6 +20,7 @@\n> > > > > > #include \n> > > > > > #include \n> > > > > >\n> > > > > > +#include \n> > > > > > #include \n> > > > > > #include \n> > > > > > #include \n> > > > > > @@ -167,6 +168,7 @@ public:\n> > > > > >\n> > > > > > V4L2PixelFormat fourcc;\n> > > > > > Size size;\n> > > > > > + std::optional colorSpace;\n> > > > > >\n> > > > > > std::array planes;\n> > > > > > unsigned int planesCount = 0;\n> > > > > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> > > > > > index 4f04212d..e03ff8b5 100644\n> > > > > > --- a/src/libcamera/v4l2_videodevice.cpp\n> > > > > > +++ b/src/libcamera/v4l2_videodevice.cpp\n> > > > > > @@ -370,17 +370,27 @@ bool V4L2BufferCache::Entry::operator==(const FrameBuffer &buffer) const\n> > > > > > * \\brief The plane line stride (in bytes)\n> > > > > > */\n> > > > > >\n> > > > > > +/**\n> > > > > > + * \\var V4L2DeviceFormat::fourcc\n> > > > > > + * \\brief The fourcc code describing the pixel encoding scheme\n> > > > > > + *\n> > > > > > + * The fourcc code, as defined by the V4L2 API with the V4L2_PIX_FMT_* macros,\n> > > > > > + * that identifies the image format pixel encoding scheme.\n> > > > > > + */\n> > > > > > +\n> > > > >\n> > > > > Nice that the uncomplete documentation about color spaces has been\n> > > > > dropped, but why moving the documentation block above ? Please keep\n> > > > > the same order as the declaration order in the header file.\n> > > > >\n> > > > > It will also make it easier to diff it during review.\n> > > > >\n> > > > > > /**\n> > > > > > * \\var V4L2DeviceFormat::size\n> > > > > > * \\brief The image size in pixels\n> > > > > > */\n> > > > > >\n> > > > > > /**\n> > > > > > - * \\var V4L2DeviceFormat::fourcc\n> > > > > > - * \\brief The fourcc code describing the pixel encoding scheme\n> > > > > > + * \\var V4L2DeviceFormat::colorSpace\n> > > > > > + * \\brief The color space of the pixels\n> > > > > > *\n> > > > > > - * The fourcc code, as defined by the V4L2 API with the V4L2_PIX_FMT_* macros,\n> > > > > > - * that identifies the image format pixel encoding scheme.\n> > > > > > + * The color space of the image. When setting the format this may be\n> > > > > > + * unset, in which case the driver gets to use its default color space.\n> > > > > > + * After being set, this value should contain the color space that the\n> > > > >\n> > > > > s/that the/that ?\n> > > > >\n> > > > > > + * was actually used.\n> > > > > > */\n> > > > > >\n> > > > > > /**\n> > > > > > @@ -879,6 +889,10 @@ int V4L2VideoDevice::getFormatMultiplane(V4L2DeviceFormat *format)\n> > > > > > format->fourcc = V4L2PixelFormat(pix->pixelformat);\n> > > > > > format->planesCount = pix->num_planes;\n> > > > > >\n> > > > > > + format->colorSpace = toColorSpace(*pix);\n> > > > > > + if (!format->colorSpace)\n> > > > > > + LOG(V4L2, Error) << \"Retrieved unrecognised color space\";\n> > > > > > +\n> > > > > > for (unsigned int i = 0; i < format->planesCount; ++i) {\n> > > > > > format->planes[i].bpl = pix->plane_fmt[i].bytesperline;\n> > > > > > format->planes[i].size = pix->plane_fmt[i].sizeimage;\n> > > > > > @@ -893,6 +907,9 @@ int V4L2VideoDevice::trySetFormatMultiplane(V4L2DeviceFormat *format, bool set)\n> > > > > > struct v4l2_pix_format_mplane *pix = &v4l2Format.fmt.pix_mp;\n> > > > > > int ret;\n> > > > > >\n> > > > > > + if (!format->colorSpace)\n> > > > > > + LOG(V4L2, Warning) << \"Setting undefined color space\";\n> > > > > > +\n> > > > > > v4l2Format.type = bufferType_;\n> > > > > > pix->width = format->size.width;\n> > > > > > pix->height = format->size.height;\n> > > > > > @@ -900,6 +917,12 @@ int V4L2VideoDevice::trySetFormatMultiplane(V4L2DeviceFormat *format, bool set)\n> > > > > > pix->num_planes = format->planesCount;\n> > > > > > pix->field = V4L2_FIELD_NONE;\n> > > > > >\n> > > > > > + ret = fromColorSpace(format->colorSpace, *pix);\n> > >\n> > > Looking at how this is used it really looks like this could be called\n> > >\n> > > ret = fillV4L2ColorSpace(pix, format->colorSpace);\n> > >\n> > > > > > + if (ret < 0)\n> > > > > > + LOG(V4L2, Warning)\n> > > > > > + << \"Setting color space unrecognised by V4L2: \"\n> > > > > > + << ColorSpace::toString(format->colorSpace);\n> > > > > > +\n> > > > > > ASSERT(pix->num_planes <= std::size(pix->plane_fmt));\n> > > > > >\n> > > > > > for (unsigned int i = 0; i < pix->num_planes; ++i) {\n> > > > > > @@ -928,6 +951,10 @@ int V4L2VideoDevice::trySetFormatMultiplane(V4L2DeviceFormat *format, bool set)\n> > > > > > format->planes[i].size = pix->plane_fmt[i].sizeimage;\n> > > > > > }\n> > > > > >\n> > > > > > + format->colorSpace = toColorSpace(*pix);\n> > > > > > + if (!format->colorSpace)\n> > > > > > + LOG(V4L2, Warning) << \"Unrecognised color space has been set\";\n> > > > > > +\n> > > > >\n> > > > > Does colorspace handling stricly have to be tied to format\n> > > > > configuration ? This would lead to undesired side effects, like the metadata pad\n> > > > > being poked for colorspace and complaining as it doesn't really have\n> > > > > one.\n> > > > >\n> > > > > Should colorspace configuration be achieved with dedicated functions\n> > > > > in the V4L2*Device classes so that pipeline handlers know what devices\n> > > > > to apply/retrieve colorspace from, like they do with formats ?\n> > > >\n> > > > Yes, there is a problem here that I talked about in one of the earlier\n> > > > revisions. Actually it's only an issue for subdevices, so you might\n> > > > notice that there's a \"todo\" in v4l2_subdevice.cpp getFormat.\n> > > >\n> > > > The problem is that I end up trying to convert the colour space on a\n> > > > metadata pad - and of course that doesn't work. Nothing breaks, you\n> > >\n> > > I'm aware of the issue and it means to me the implemented API falls a\n> > > bit short if it causes annoyances with a standard use case like this\n> > > one.\n> > >\n> > > My proposal was to leave set/getFormat() in their current version in\n> > > V4L2VideoDevice and V4L2Subdevice and add a new setColorSpace() or\n> > > similar which the pipeline handlers would be in charge to call on\n> > > the devices it consider appropriate.\n> > >\n> > > This would separate the ColorSpace configuration from the image format\n> > > configuration. In V4L2 they go through the same IOCTL and the same\n> > > structure, but if that doesn't work well for us we can deviate from\n> > > it.\n> > >\n> > > Also looking at how colorspaces are validated in your last patch\n> > >\n> > > int ret = dev->tryFormat(&format);\n> > > if (ret)\n> > > return Invalid;\n> > > + if (!format.colorSpace || cfg.colorSpace != format.colorSpace) {\n> > > + status = Adjusted;\n> > > + LOG(RPI, Warning)\n> > > + << \"Color space changed from \"\n> > > + << ColorSpace::toString(cfg.colorSpace) << \" to \"\n> > > + << ColorSpace::toString(format.colorSpace);\n> > > + }\n> > > + cfg.colorSpace = format.colorSpace;\n> > >\n> > > This really seems like it could be\n> > >\n> > > int ret = dev->tryFormat(&format);\n> > > if (ret)\n> > > return Invalid;\n> > >\n> > > ret = dev->setColorSpace(cfg.colorSpace);\n> > > if (cfg.colorSpace != dev->getColorSpace()) {\n> > > status = Adjusted;\n> > > cfg.colorSpace = dev->getColorSpace();\n> > > }\n> > >\n> > >\n> > > True, setting an image format resets the colorspace maybe but I think\n> > > it's fine as long as it is documented.\n> > >\n> > > If in the setColorSpace() function you need a v4l2_format you can use\n> > > the currently applied format. Of course you would need to add the\n> > > single planar/multiplanar V4L2 API behind the single setColorSpace()\n> > > function.\n> > >\n> > > In my mental picture there's nothing that prevents this from being\n> > > implemented (not that I'm saying it's a good idea, but it's doable at\n> > > last), am I wrong ?\n> >\n> > So I agree that it could be done like this, but I guess the following\n> > things bother me a bit.\n> >\n> > Mainly it seems more complicated (you have to do 2 things all the\n> > time, not just one - source of bugs right there). It confounds\n>\n> I don't think it will be source of bugs, but rather something not all\n> pipeline handlers might do. But we'll have compliance test suites, and\n> this will be caught \"early\"\n>\n> > existing expectations about V4L2 - why confuse people who thought they\n> > knew how V4L2 works? This is a \"helpful C++ layer\" on top of V4L2,\n>\n> Well, no :)\n>\n> libcamera might theoretically exists on top of other abstractions and\n> when it makes sense to deviate from v4l2 we're free to do so (again,\n> if it makes sense)\n>\n> > right? And the actual mechanics of splitting this up - every \"set\"\n> > operation becomes a \"query-first-then-set\"- is not lovely.\n>\n> The \"query-first-then-set\" happens in the theoretical\n> V4L2*Device::setColorSpace() function or in the pipeline handler code\n> ?\n>\n> >\n> > Actually I think there's a perfectly good solution which is simply to\n> > let the caller check the result. I know I get obsessed about colour\n> > spaces, but apparently not everyone does! This way everyone gets what\n> > they want. I can check. Others can check if they want. All problems\n> > solved.\n>\n> Not really, as setting a format on a metadata pad creates an error\n> message, and it doesn't anyway make much sense to configure a\n> colorspace there. As soon as we have devices producing data in\n> non-pixel formats (depth maps etc etc) we'll hit the same issue.\n\nYes, I think I agree with this - and that's why it's best to leave the\ncheck to the caller. If it's image data, and they care about colour\nspaces, they can check. Otherwise they can leave it alone and all will\nbe fine. I'll prepare another version with these and other changes and\npost it shortly!\n\nThanks\nDavid\n\n>\n> Thanks\n> j\n>\n> >\n> > David\n> >\n> > >\n> > > > just get a warning message. I'm open to suggestions on how to resolve\n> > > > this. An extra parameter to the get/setFormat (\"I am not an image data\n> > > > pad\"?). Separate functions get/setImageFormat and\n> > > > get/setMetadataFormat? Rely on the caller to check for a bad colour\n> > > > space? (but will they bother?)\n> > > >\n> > > > Thoughts, anyone?\n> > > >\n> > > > Thanks!\n> > > > David\n> > > >\n> > > > >\n> > > > > > return 0;\n> > > > > > }\n> > > > > >\n> > > > > > @@ -951,6 +978,10 @@ int V4L2VideoDevice::getFormatSingleplane(V4L2DeviceFormat *format)\n> > > > > > format->planes[0].bpl = pix->bytesperline;\n> > > > > > format->planes[0].size = pix->sizeimage;\n> > > > > >\n> > > > > > + format->colorSpace = toColorSpace(*pix);\n> > > > > > + if (!format->colorSpace)\n> > > > > > + LOG(V4L2, Error) << \"Retrieved unrecognised color space\";\n> > > > > > +\n> > > > > > return 0;\n> > > > > > }\n> > > > > >\n> > > > > > @@ -960,12 +991,22 @@ int V4L2VideoDevice::trySetFormatSingleplane(V4L2DeviceFormat *format, bool set)\n> > > > > > struct v4l2_pix_format *pix = &v4l2Format.fmt.pix;\n> > > > > > int ret;\n> > > > > >\n> > > > > > + if (!format->colorSpace)\n> > > > > > + LOG(V4L2, Error) << \"Trying to set unrecognised color space\";\n> > > > > > +\n> > > > > > v4l2Format.type = bufferType_;\n> > > > > > pix->width = format->size.width;\n> > > > > > pix->height = format->size.height;\n> > > > > > pix->pixelformat = format->fourcc;\n> > > > > > pix->bytesperline = format->planes[0].bpl;\n> > > > > > pix->field = V4L2_FIELD_NONE;\n> > > > > > +\n> > > > > > + ret = fromColorSpace(format->colorSpace, *pix);\n> > > > > > + if (ret < 0)\n> > > > > > + LOG(V4L2, Warning)\n> > > > > > + << \"Set color space unrecognised by V4L2: \"\n> > > > > > + << ColorSpace::toString(format->colorSpace);\n> > > > > > +\n> > > > > > ret = ioctl(set ? VIDIOC_S_FMT : VIDIOC_TRY_FMT, &v4l2Format);\n> > > > > > if (ret) {\n> > > > > > LOG(V4L2, Error)\n> > > > > > @@ -985,6 +1026,10 @@ int V4L2VideoDevice::trySetFormatSingleplane(V4L2DeviceFormat *format, bool set)\n> > > > > > format->planes[0].bpl = pix->bytesperline;\n> > > > > > format->planes[0].size = pix->sizeimage;\n> > > > > >\n> > > > > > + format->colorSpace = toColorSpace(*pix);\n> > > > > > + if (!format->colorSpace)\n> > > > > > + LOG(V4L2, Error) << \"Undefined color space has been set\";\n> > > > > > +\n> > > > > > return 0;\n> > > > > > }\n> > > > > >\n> > > > > > --\n> > > > > > 2.20.1\n> > > > > >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 4A807BDB13\n\tfor ;\n\tFri, 26 Nov 2021 09:34:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8958F6049F;\n\tFri, 26 Nov 2021 10:34:17 +0100 (CET)","from mail-wr1-x42f.google.com (mail-wr1-x42f.google.com\n\t[IPv6:2a00:1450:4864:20::42f])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EB4D460227\n\tfor ;\n\tFri, 26 Nov 2021 10:34:15 +0100 (CET)","by mail-wr1-x42f.google.com with SMTP id l16so17277143wrp.11\n\tfor ;\n\tFri, 26 Nov 2021 01:34:15 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"fwoo1c90\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=tyZmiBe8yh3qhHHnOXXwx8Dp2Oppqr4rEefL26wNxDY=;\n\tb=fwoo1c90kZRIbEYh7bkqnh0YlmbczbpraVCTc3sXsqzFpIJpcva5ZtDkFllcwI33Yq\n\tVmUx2QXPj3WYhvpd7/SOOkpGj1UMCpe4MO3fpD1s3FkbhuC2DkK2Hx9tWAEP6icsDErF\n\tdJVOHInaB7kPgHkn8f4q1IGB1Lc0BOgETgZVt3aMxdOqxvHc0Hg+3pacgBdFVRQ1b0eO\n\t4Lq9W0mRzryp50EP3UTNZ9jIkBY5v99QQSqo7M7Er2GHt0hwnilkGmiDL1YjcUJ6Fxdc\n\ty1U9Nv5P/PIHNYoce4p9avlqwDrA7PQkbTDQijLWtH1DXi63P33RJkC6RYyEXj1/LxyU\n\tcZuw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=tyZmiBe8yh3qhHHnOXXwx8Dp2Oppqr4rEefL26wNxDY=;\n\tb=3JQhLjJJhXj6NpvGVHcRidpVJZPZzmOGM9ojUnNlSB+B2n/ulpOwLnJyDVr/e0knD9\n\t6LlC/0B9w2LjEbNcIuh7dxCM5/uggWTtXezluc96opPVu3Tvzdn08CL2n4btELfexcuz\n\tSq/vVk4Jqv4qhc2CIgg+2MTwM5+LxVcF5lZ5eA8AeeHVIr6s42Hbl6F2IY8d2IVLJpPv\n\tKmKIEOVLeIFVAy3c6kwVPIDYrqhzfDmXFDUbrVc0ryi42NXIIhV/q/TC1xXH+oKI+B7y\n\tMgOCdGQkTtL+fxfIJG2qRkh/8hVqpRoEFT2pbv9aqU1LHdwNV4ot241lvmUKi8Lgrtwq\n\tLL/w==","X-Gm-Message-State":"AOAM531WelovJ+bf5jN3vKP/eVXeoDco2FrgVGZ8s0K5bhX1MHgJlne/\n\tnujne3DaEbldGidX2Fw3KFlJgXNiYE+MiAC43l327Q==","X-Google-Smtp-Source":"ABdhPJxDi2zPQVRRsXi0Jf+e9nPjC/gBIzMVZIqIrbNyuwKVKZw2VLNU3wK2ROEHmD1+69LJZzbA7L7HjZzRKgT2Ks8=","X-Received":"by 2002:adf:f2ca:: with SMTP id\n\td10mr12707064wrp.79.1637919255339; \n\tFri, 26 Nov 2021 01:34:15 -0800 (PST)","MIME-Version":"1.0","References":"<20211118151933.15627-1-david.plowman@raspberrypi.com>\n\t<20211118151933.15627-5-david.plowman@raspberrypi.com>\n\t<20211124234258.vart2hjbrjlqwh3r@uno.localdomain>\n\t\n\t<20211125171902.632gcjkwzaevxfc2@uno.localdomain>\n\t\n\t<20211126082104.rxuh2dzonao4wnat@uno.localdomain>","In-Reply-To":"<20211126082104.rxuh2dzonao4wnat@uno.localdomain>","From":"David Plowman ","Date":"Fri, 26 Nov 2021 09:34:04 +0000","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v6 4/7] libcamera: Support passing\n\tColorSpaces to V4L2 video devices","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"Tomasz Figa ,\n\tlibcamera devel ,\n\tHans Verkuil ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]