From patchwork Tue Oct 25 01:21:31 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Laurent Pinchart X-Patchwork-Id: 17689 Return-Path: X-Original-To: parsemail@patchwork.libcamera.org Delivered-To: parsemail@patchwork.libcamera.org Received: from lancelot.ideasonboard.com (lancelot.ideasonboard.com [92.243.16.209]) by patchwork.libcamera.org (Postfix) with ESMTPS id 66FF0BD16B for ; Tue, 25 Oct 2022 01:22:00 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id CFE3A62F2C; Tue, 25 Oct 2022 03:21:59 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org; s=mail; t=1666660919; bh=GuyCrJs8jIgN4JZOHr421WT3T/c3MnSoVFNfD2uLprk=; h=To:Date:Subject:List-Id:List-Unsubscribe:List-Archive:List-Post: List-Help:List-Subscribe:From:Reply-To:From; b=Hy00GaGKoFKWPrNJFSFb0SohfQMr0DuA8/Od0rnYJhlV2o0hgu14Jlqg6rtBrKUx9 ZoPcs8+ceGMOR3FtWRJjuCzmsxYLtGTUxGshHUZ83fTzqulsjmvJW6kX4kgvg8IWTO /ZLy2rR7l4oD5B+uHXul0R9ot+S2c3LYt2L+iol19Nb8GGdjAlNppzMO4NUnmVrkhQ tRp46NF0rsUdJ8toePgrxQL2dM/yaiueZn7FHSd49Z8maUkCY/NXk/mYu1vyL9gDmp 7HfS6rkwguHi484+WUI/O/NR9u1G7QbNFtuNJaJpzxOwCkrZ786XB4zGoA+KMXRMY3 yAaWsMnIb7Pnw== Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id B8DC262EAE for ; Tue, 25 Oct 2022 03:21:57 +0200 (CEST) Authentication-Results: lancelot.ideasonboard.com; dkim=pass (1024-bit key; unprotected) header.d=ideasonboard.com header.i=@ideasonboard.com header.b="JCc3NrTM"; dkim-atps=neutral Received: from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi [62.78.145.57]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 1EEBCD3C; Tue, 25 Oct 2022 03:21:57 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1666660917; bh=GuyCrJs8jIgN4JZOHr421WT3T/c3MnSoVFNfD2uLprk=; h=From:To:Cc:Subject:Date:From; b=JCc3NrTMhWrDixxNqMVuwLCRsSdoo8NCvcNXtw+vp3rq5JeDB82Wp+RdDx/fJADhW NOiy2QsrbOhytUBZ4jQG+exsa+OWDArOv+FKSM57xNAwBxv2cqrf3uCNcuXP+kE5Ez uUbzGiwOVVsL7rks+DtuPAZPLGTFHMUccnstNphY= To: libcamera-devel@lists.libcamera.org Date: Tue, 25 Oct 2022 04:21:31 +0300 Message-Id: <20221025012131.12943-1-laurent.pinchart@ideasonboard.com> X-Mailer: git-send-email 2.37.4 MIME-Version: 1.0 Subject: [libcamera-devel] [RFC PATCH] libcamera: v4l2_device: Always set all four V4L2 colorspace fields X-BeenThere: libcamera-devel@lists.libcamera.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-Patchwork-Original-From: Laurent Pinchart via libcamera-devel From: Laurent Pinchart Reply-To: Laurent Pinchart Errors-To: libcamera-devel-bounces@lists.libcamera.org Sender: "libcamera-devel" When setting a colorspace on a V4L2 video capture device or a subdev source pad, the kernel considers the colorspace, xfer_func, ycbcr_enc and quantization fields as four independent values. Any field set to V4L2_*_DEFAULT by userspace is ignored, and the driver keeps its current configuration for that field. This behaviour, while not necessarily well known, is clearly documented in [1] and [2], and is implemented by the rkisp1 driver. The V4L2Device::fromColorSpace() function, when converting from a libcamera ColorSpace to the V4L2 format colorspace-related fields, first attempts to find a match for a colorspace preset. If found, it only sets the colorspace field, and leaves the xfer_func, ycbcr_enc and quantization fields to V4L2_*_DEFAULT. This contradicts the V4L2 specification, and prevents setting the transfer function, YCbCr encoding and quantization on at least rkisp1. Fix this issue by dropping the preset lookup, and set all four colorspace-related fields explicitly. [1] https://linuxtv.org/downloads/v4l-dvb-apis/userspace-api/v4l/pixfmt-v4l2.html#id2 [2] https://linuxtv.org/downloads/v4l-dvb-apis/userspace-api/v4l/subdev-formats.html#id2 Signed-off-by: Laurent Pinchart Tested-by: Jacopo Mondi --- I expect this change to cause breakages on Raspberry Pi, hence the RFC. The bcm2835-isp driver doesn't comply with the V4L2 API specification. I certainly won't blame it, as I implemented the same non-compliant behaviour in the rkisp1 driver initially, before realizing that V4L2 doesn't consider the colorspace field as a preset in set format calls (this has been confirmed by Hans). The question is how to move forward here. Fixing the driver to make it compliant with V4L2 wouldn't be difficult, but I expect non-libcamera users to be surprised. I don't expect many such users though, so if that's fine, the only possible issue would be synchronizing the changes in the driver and in libcamera. Other options may be possible. I don't think updating the V4L2 API specification to consider the colorspace field as a preset will be well received upstream (and I don't think I would like that much myself to be honest). Keeping a non-compliant implementation in the bcm2835-isp driver with specific handling in libcamera could be done, but I don't think it would be upstreamable. --- src/libcamera/v4l2_device.cpp | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp index c17b323f8af0..e60a9ae217de 100644 --- a/src/libcamera/v4l2_device.cpp +++ b/src/libcamera/v4l2_device.cpp @@ -915,20 +915,6 @@ int V4L2Device::fromColorSpace(const std::optional &colorSpace, T &v if (!colorSpace) return 0; - auto itColor = std::find_if(colorSpaceToV4l2.begin(), colorSpaceToV4l2.end(), - [&colorSpace](const auto &item) { - return colorSpace == item.first; - }); - if (itColor != colorSpaceToV4l2.end()) { - v4l2Format.colorspace = itColor->second; - /* Leaving all the other fields as "default" should be fine. */ - return 0; - } - - /* - * If the colorSpace doesn't precisely match a standard color space, - * then we must choose a V4L2 colorspace with matching primaries. - */ int ret = 0; auto itPrimaries = primariesToV4l2.find(colorSpace->primaries);