[{"id":24378,"web_url":"https://patchwork.libcamera.org/comment/24378/","msgid":"","date":"2022-08-04T17:39:32","subject":"Re: [libcamera-devel] [PATCH v2] libcamera: v4l2: Set colorspace\n\tflags","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Umang,\n\nThank you for the patch.\n\nOn Mon, Aug 01, 2022 at 01:54:20PM +0530, Umang Jain via libcamera-devel wrote:\n> The colorspace fields are read-only from an application point of view,\n> both on video devices and on subdevs, unless the\n> V4L2_PIX_FMT_FLAG_SET_CSC or V4L2_MBUS_FRAMEFMT_SET_CSC flags\n> (respectively) are set when calling the S_FMT ioctl.\n> \n> Signed-off-by: Umang Jain \n> ---\n> changes in v2:\n> - Set V4L2_MBUS_FRAMEFMT_SET_CSC flag only on source pad of\n> media entity\n> - Set V4L2_PIX_FMT_FLAG_SET_CSC on video capture devices only\n> ---\n> src/libcamera/v4l2_subdevice.cpp | 6 ++++--\n> src/libcamera/v4l2_videodevice.cpp | 8 ++++++--\n> 2 files changed, 10 insertions(+), 4 deletions(-)\n> \n> diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp\n> index 98a3911a..8f6b327f 100644\n> --- a/src/libcamera/v4l2_subdevice.cpp\n> +++ b/src/libcamera/v4l2_subdevice.cpp\n> @@ -459,9 +459,11 @@ int V4L2Subdevice::setFormat(unsigned int pad, V4L2SubdeviceFormat *format,\n> \tsubdevFmt.format.height = format->size.height;\n> \tsubdevFmt.format.code = format->mbus_code;\n> \tsubdevFmt.format.field = V4L2_FIELD_NONE;\n> -\tfromColorSpace(format->colorSpace, subdevFmt.format);\n> +\tint ret = fromColorSpace(format->colorSpace, subdevFmt.format);\n> +\tif (ret == 0 && (entity_->pads()[pad]->flags() & MEDIA_PAD_FL_SOURCE))\n> +\t\tsubdevFmt.format.flags |= V4L2_MBUS_FRAMEFMT_SET_CSC;\n\nI don't think that's the right condition, as from ColorSpace returns 0\nif colorSpace is nullopt. I'd write it as\n\n\tif (format->colorSpace) {\n\t\tfromColorSpace(format->colorSpace, subdevFmt.format);\n\n\t\t/* The CSC flag is only applicable to source pads. */\n\t\tif (entity_->pads()[pad]->flags() & MEDIA_PAD_FL_SOURCE)\n\t\t\tsubdevFmt.format.flags |= V4L2_MBUS_FRAMEFMT_SET_CSC;\n\t}\n\nYou could possibly move the fromColorSpace() call outside of the if (),\nas colorSpace is checked in the function itself, but given that an if ()\nis needed anyway, I'd leave it in there.\n\nSame below.\n\n> \n> -\tint ret = ioctl(VIDIOC_SUBDEV_S_FMT, &subdevFmt);\n> +\tret = ioctl(VIDIOC_SUBDEV_S_FMT, &subdevFmt);\n> \tif (ret) {\n> \t\tLOG(V4L2, Error)\n> \t\t\t<< \"Unable to set format on pad \" << pad\n> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> index 63911339..f3651740 100644\n> --- a/src/libcamera/v4l2_videodevice.cpp\n> +++ b/src/libcamera/v4l2_videodevice.cpp\n> @@ -940,7 +940,9 @@ int V4L2VideoDevice::trySetFormatMultiplane(V4L2DeviceFormat *format, bool set)\n> \tpix->pixelformat = format->fourcc;\n> \tpix->num_planes = format->planesCount;\n> \tpix->field = V4L2_FIELD_NONE;\n> -\tfromColorSpace(format->colorSpace, *pix);\n> +\tret = fromColorSpace(format->colorSpace, *pix);\n> +\tif (ret == 0 && caps_.isVideoCapture())\n> +\t\tpix->flags |= V4L2_PIX_FMT_FLAG_SET_CSC;\n> \n> \tASSERT(pix->num_planes <= std::size(pix->plane_fmt));\n> \n> @@ -1010,7 +1012,9 @@ int V4L2VideoDevice::trySetFormatSingleplane(V4L2DeviceFormat *format, bool set)\n> \tpix->pixelformat = format->fourcc;\n> \tpix->bytesperline = format->planes[0].bpl;\n> \tpix->field = V4L2_FIELD_NONE;\n> -\tfromColorSpace(format->colorSpace, *pix);\n> +\tret = fromColorSpace(format->colorSpace, *pix);\n> +\tif (ret == 0 && caps_.isVideoCapture())\n> +\t\tpix->flags |= V4L2_PIX_FMT_FLAG_SET_CSC;\n> \n> \tret = ioctl(set ? VIDIOC_S_FMT : VIDIOC_TRY_FMT, &v4l2Format);\n> \tif (ret) {","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 16CA6BE173\n\tfor ;\n\tThu, 4 Aug 2022 17:39:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 69B6A6332B;\n\tThu, 4 Aug 2022 19:39:42 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 924B86330D\n\tfor ;\n\tThu, 4 Aug 2022 19:39:40 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E3FDC481;\n\tThu, 4 Aug 2022 19:39:39 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1659634782;\n\tbh=tBuSwS/uMw16xgJqOPqMRvJW5pFNmUmlThsdae5jeH8=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=aOhETJrieUmrC+XNC5LNYKD0GKoFltwYSl/OvxNP4O1pzkddxHux3t/KDDSw1fKlU\n\tvvcAFuWY3xOuR/DN5dU1lRwhBdnjf6S7hMzpx4HSA8mpSWAzXPnBjyxagsxWxvyZal\n\tsqRvREHB3M6wRafpehqPc6/Sw0gceVBqskTCGjAOms1sUaKHwOfx5b9TOFFEW/2m5b\n\tX5q1AZGsDIlUqAmCMA7yuW/xe3rM3foyxS1OIsGBPIc7CeojhTLeVCuZxTW9kAh+mZ\n\tPBuDSn3t0UO6hxqP2Z95F7vHHlDoFF+5/Gt5H69Bg62DsK4MnF8601Jyr011wVAyY1\n\tgNvuEn7b7Epwg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1659634780;\n\tbh=tBuSwS/uMw16xgJqOPqMRvJW5pFNmUmlThsdae5jeH8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=FIKLSfw9goeIuV1qfJbVQ/D5WZL3YzfVZhJ2m6UTAz+qEXnVO2NQOkuJPNg8TAscy\n\t7kSIWAuGvLqoc0yrxZMH21anSJdbYCc33vNsTAV7+8ZFVBLQpy6oaqFPjGk0z7MjAb\n\tP+zOpayii/SzPgWS3q+4NsMYUWcJO0jgGMN0F0lo="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"FIKLSfw9\"; dkim-atps=neutral","Date":"Thu, 4 Aug 2022 20:39:32 +0300","To":"Umang Jain ","Message-ID":"","References":"<20220801082420.68120-1-umang.jain@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220801082420.68120-1-umang.jain@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2] libcamera: v4l2: Set colorspace\n\tflags","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"rishikeshdonadkar@gmail.com, libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24387,"web_url":"https://patchwork.libcamera.org/comment/24387/","msgid":"","date":"2022-08-05T04:52:20","subject":"Re: [libcamera-devel] [PATCH v2] libcamera: v4l2: Set colorspace\n\tflags","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 8/4/22 23:09, Laurent Pinchart wrote:\n> Hi Umang,\n>\n> Thank you for the patch.\n>\n> On Mon, Aug 01, 2022 at 01:54:20PM +0530, Umang Jain via libcamera-devel wrote:\n>> The colorspace fields are read-only from an application point of view,\n>> both on video devices and on subdevs, unless the\n>> V4L2_PIX_FMT_FLAG_SET_CSC or V4L2_MBUS_FRAMEFMT_SET_CSC flags\n>> (respectively) are set when calling the S_FMT ioctl.\n>>\n>> Signed-off-by: Umang Jain \n>> ---\n>> changes in v2:\n>> - Set V4L2_MBUS_FRAMEFMT_SET_CSC flag only on source pad of\n>> media entity\n>> - Set V4L2_PIX_FMT_FLAG_SET_CSC on video capture devices only\n>> ---\n>> src/libcamera/v4l2_subdevice.cpp | 6 ++++--\n>> src/libcamera/v4l2_videodevice.cpp | 8 ++++++--\n>> 2 files changed, 10 insertions(+), 4 deletions(-)\n>>\n>> diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp\n>> index 98a3911a..8f6b327f 100644\n>> --- a/src/libcamera/v4l2_subdevice.cpp\n>> +++ b/src/libcamera/v4l2_subdevice.cpp\n>> @@ -459,9 +459,11 @@ int V4L2Subdevice::setFormat(unsigned int pad, V4L2SubdeviceFormat *format,\n>> \tsubdevFmt.format.height = format->size.height;\n>> \tsubdevFmt.format.code = format->mbus_code;\n>> \tsubdevFmt.format.field = V4L2_FIELD_NONE;\n>> -\tfromColorSpace(format->colorSpace, subdevFmt.format);\n>> +\tint ret = fromColorSpace(format->colorSpace, subdevFmt.format);\n>> +\tif (ret == 0 && (entity_->pads()[pad]->flags() & MEDIA_PAD_FL_SOURCE))\n>> +\t\tsubdevFmt.format.flags |= V4L2_MBUS_FRAMEFMT_SET_CSC;\n> I don't think that's the right condition, as from ColorSpace returns 0\n> if colorSpace is nullopt. I'd write it as\n\n\nWhen colorSpace is nullopt, v4l2Format colorspace identifiers set to \n*_DEFAULT. Doesn't that count?\n\n/me *grins*\n\n>\n> \tif (format->colorSpace) {\n> \t\tfromColorSpace(format->colorSpace, subdevFmt.format);\n>\n> \t\t/* The CSC flag is only applicable to source pads. */\n> \t\tif (entity_->pads()[pad]->flags() & MEDIA_PAD_FL_SOURCE)\n> \t\t\tsubdevFmt.format.flags |= V4L2_MBUS_FRAMEFMT_SET_CSC;\n> \t}\n>\n> You could possibly move the fromColorSpace() call outside of the if (),\n> as colorSpace is checked in the function itself, but given that an if ()\n> is needed anyway, I'd leave it in there.\n>\n> Same below.\n>\n>> \n>> -\tint ret = ioctl(VIDIOC_SUBDEV_S_FMT, &subdevFmt);\n>> +\tret = ioctl(VIDIOC_SUBDEV_S_FMT, &subdevFmt);\n>> \tif (ret) {\n>> \t\tLOG(V4L2, Error)\n>> \t\t\t<< \"Unable to set format on pad \" << pad\n>> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n>> index 63911339..f3651740 100644\n>> --- a/src/libcamera/v4l2_videodevice.cpp\n>> +++ b/src/libcamera/v4l2_videodevice.cpp\n>> @@ -940,7 +940,9 @@ int V4L2VideoDevice::trySetFormatMultiplane(V4L2DeviceFormat *format, bool set)\n>> \tpix->pixelformat = format->fourcc;\n>> \tpix->num_planes = format->planesCount;\n>> \tpix->field = V4L2_FIELD_NONE;\n>> -\tfromColorSpace(format->colorSpace, *pix);\n>> +\tret = fromColorSpace(format->colorSpace, *pix);\n>> +\tif (ret == 0 && caps_.isVideoCapture())\n>> +\t\tpix->flags |= V4L2_PIX_FMT_FLAG_SET_CSC;\n>> \n>> \tASSERT(pix->num_planes <= std::size(pix->plane_fmt));\n>> \n>> @@ -1010,7 +1012,9 @@ int V4L2VideoDevice::trySetFormatSingleplane(V4L2DeviceFormat *format, bool set)\n>> \tpix->pixelformat = format->fourcc;\n>> \tpix->bytesperline = format->planes[0].bpl;\n>> \tpix->field = V4L2_FIELD_NONE;\n>> -\tfromColorSpace(format->colorSpace, *pix);\n>> +\tret = fromColorSpace(format->colorSpace, *pix);\n>> +\tif (ret == 0 && caps_.isVideoCapture())\n>> +\t\tpix->flags |= V4L2_PIX_FMT_FLAG_SET_CSC;\n>> \n>> \tret = ioctl(set ? VIDIOC_S_FMT : VIDIOC_TRY_FMT, &v4l2Format);\n>> \tif (ret) {","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 6ED23BE173\n\tfor ;\n\tFri, 5 Aug 2022 04:52:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 811916332B;\n\tFri, 5 Aug 2022 06:52:27 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id AB02561FAC\n\tfor ;\n\tFri, 5 Aug 2022 06:52:25 +0200 (CEST)","from [IPV6:2401:4900:1f3e:69e0:a479:4ccc:ab2a:bf48] (unknown\n\t[IPv6:2401:4900:1f3e:69e0:a479:4ccc:ab2a:bf48])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 529ED3F1;\n\tFri, 5 Aug 2022 06:52:24 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1659675147;\n\tbh=BgaV0uW9vgfQH3+iMPJe0wsn9dB3vDqR5GzZiPzqwTo=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=PDxbS2zjCyTmHYdPzvZn+8AF11kg977R/DiUxtclbohFaNSqduQXj+NbdO55+Vt8v\n\tDs3KFXTa8l3Mq84J2EDQKgZPOLSBObT+Fmzv4YSGqoaBVlXbzNW7kAv3ekObnnmtgL\n\tVvnfeF4SfjMHxBvjquRJXL61KPs3Zzh0Gw3WBuqJdRByvcguHtzpebvK8vB7BKMqCN\n\t6MIiyXhGf9TjUx17FkZzVndm97wy5qsXtjrijgdmnIGlGtrMGcdPfOTVVYUeHXdKep\n\thmee0lErX5a24/NCxX3xhuy+jgUv6GzbzRL1950reybth0E4ZWfS2qa+XYg+D7Hv/1\n\tSMyzy41PBW2DA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1659675145;\n\tbh=BgaV0uW9vgfQH3+iMPJe0wsn9dB3vDqR5GzZiPzqwTo=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=M9CNjqXufP6S11mxqqavX+W0k3UH55buNzRZQ+DrREmOpV+0okFvgmUaf7f8k68WM\n\tGKxXQ39QLJ9HH/5DXv71BM4bI3JDFYG1zF+2aLM7jY8Qg/3mClZ7wGqBpZzcvdv5MK\n\t5LKS3QnPeJmeYNZikThX5fX7lhDG5qxopmNxgCM0="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"M9CNjqXu\"; dkim-atps=neutral","Message-ID":"","Date":"Fri, 5 Aug 2022 10:22:20 +0530","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.4.1","Content-Language":"en-US","To":"Laurent Pinchart ","References":"<20220801082420.68120-1-umang.jain@ideasonboard.com>\n\t","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH v2] libcamera: v4l2: Set colorspace\n\tflags","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Umang Jain via libcamera-devel ","Reply-To":"Umang Jain ","Cc":"rishikeshdonadkar@gmail.com, libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]