[{"id":21727,"web_url":"https://patchwork.libcamera.org/comment/21727/","msgid":"","date":"2021-12-09T22:11:29","subject":"Re: [libcamera-devel] [PATCH v10 0/8] Colour spaces","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi David,\n\nOn Thu, Dec 09, 2021 at 10:12:37AM +0000, David Plowman wrote:\n> Hi everyone\n> \n> We're on to version 10 now!\n\n8 more versions to come of age :-D\n\n> Thanks for all the latest round of\n> reviews, that's much appreciated. I've adopted pretty much all of\n> those suggestions, but I'll try and round up the handful of discussion\n> points into this cover note.\n\nI should have read this before reviewing, looks like I've repeated\nquestions in 8/8 that you're answering below.\n\n> 1. Should we have a \"None\" Y'CbCr encoding?\n> \n> I have added one as it bothered me to have \"Rec601\" in ColorSpace::Raw\n> otherwise. I wonder slightly what you should do if a device returns\n> V4L2_COLORSPACE_RAW and a non-default YCbCr encoding. What would that\n> mean?\n\nProbably that the driver author didn't know what to do.\n\n> Should we turn it into ColorSpace::Raw regardless, or take it at\n> face value and let a pipeline handler complain (if it cares)?\n\nMaybe we could handle it when the situation arises.\n\n> For now I've taken the second approach, I suspect that in reality no\n> one is really going to care, but we could change that.\n> \n> 2. std::array in place of std::vector in color_space.cpp\n> \n> Unfortunately I still can't make the std::array constexpr, because the\n> named ColorSpaces within them aren't. And they can't be because\n> they're static class members which would have to be initialised within\n> the class, only you can't initialise the objects of the class from\n> within the declaration...\n\nOK, no worries.\n\n> But I have changed it to std::array anyway (just a const one), aren't\n> they in theory marginally cheaper than std::vectors?\n\nMarginally, yes, I think.\n\n> 3. Error messages in v4l2_videodevice.cpp etc.\n> \n> Should these be in to/fromColorSpace instead? I've left them where\n> they are because I can still imagine a pipeline handler calling these\n> functions to check the results for themselves, at which point\n> functions that spit out messages are just annoying.\n\nI agree that we shouldn't have warning or error messages for situations\nthat can arise under normal circumstances.\n\n> But I don't feel very strongly about this given that currently no one\n> else is calling them, so would be happy enough to change it!\n\nMaybe we could do so to avoid duplication of messages, and change it\nlater if needed ? The toColorSpace() and fromColorSpace() functions\nshould be made protected in that case, to make sure we noticed attempts\nto use them directly from pipeline handlers.\n\n> 4. Why should the largest output stream determine the color space?\n> \n> Because it's the biggest, therefore it's the most important. :)\n> \n> Though to be fair sometimes we use our low resolution output stream\n> for image analysis that users never see, so the colour space there\n> probably is less important. But in the end we have to pick one of\n> them and it doesn't really matter that much which.\n\nIf you could capture the rationale in a sentence or two in the patch or\ncommit message, that would be nice.\n\n> 5. Warnings in the RPi pipeline handler...\n> \n> I've removed one bunch of warnings on the assurance that validate()\n> has been called... Some others I've left in because they really\n> shouldn't happen and I am completely paranoid about colour spaces. If\n> I could be less anxious about them that would be nice...\n\nThere's just one warning message left, that tells if colour spaces have\nbeen adjusted. Isn't that allowed under normal circumstances ?\n\n> 6. Where to call validateColorSpaces?\n> \n> I've left this where it is. The one thing we're never going to do is\n> change raw pixel formats to non-raw ones, and vice versa. And mostly I\n> think pixel formats have little to do with colour spaces. There are\n> some exceptions (e.g. raw formats imply \"raw\" colour spaces), and YUV\n> type outputs imply using the Y'CbCr encoding matrix which is sort of\n> related, but apart from that I think they're mostly distinct.\n> (Everyone will now list more exceptions...)\n\nOK, sounds good to me.\n\n> Anyway, thanks again for all the effort that has gone into the many\n> reviews!\n> \n> David Plowman (8):\n> libcamera: Add ColorSpace class\n> libcamera: stream: Add ColorSpace fields to StreamConfiguration\n> libcamera: video_device: Convert between ColorSpace class and V4L2\n> formats\n> libcamera: video_device: Support passing ColorSpaces to V4L2 video\n> devices\n> libcamera: v4l2_subdevice: Add colorSpace field to V4L2SubdeviceFormat\n> libcamera: v4l2_subdevice: Support passing ColorSpaces to V4L2\n> subdevices\n> libcamera: Add validateColorSpaces to CameraConfiguration class\n> libcamera: pipeline: raspberrypi: Support color spaces\n> \n> include/libcamera/camera.h | 10 +\n> include/libcamera/color_space.h | 70 ++++\n> include/libcamera/internal/v4l2_device.h | 8 +\n> include/libcamera/internal/v4l2_subdevice.h | 3 +\n> include/libcamera/internal/v4l2_videodevice.h | 3 +\n> include/libcamera/meson.build | 1 +\n> include/libcamera/stream.h | 3 +\n> src/libcamera/camera.cpp | 84 +++++\n> src/libcamera/camera_sensor.cpp | 1 +\n> src/libcamera/color_space.cpp | 318 ++++++++++++++++++\n> src/libcamera/meson.build | 1 +\n> src/libcamera/pipeline/ipu3/cio2.cpp | 7 +-\n> .../pipeline/raspberrypi/raspberrypi.cpp | 40 +++\n> src/libcamera/pipeline/simple/simple.cpp | 8 +-\n> src/libcamera/stream.cpp | 20 ++\n> src/libcamera/v4l2_device.cpp | 194 +++++++++++\n> src/libcamera/v4l2_subdevice.cpp | 25 +-\n> src/libcamera/v4l2_videodevice.cpp | 32 ++\n> 18 files changed, 821 insertions(+), 7 deletions(-)\n> create mode 100644 include/libcamera/color_space.h\n> create mode 100644 src/libcamera/color_space.cpp","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 BB997BF415\n\tfor ;\n\tThu, 9 Dec 2021 22:12:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0CEC36087A;\n\tThu, 9 Dec 2021 23:12:01 +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 06AAA607DE\n\tfor ;\n\tThu, 9 Dec 2021 23:11:59 +0100 (CET)","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 5E09E501;\n\tThu, 9 Dec 2021 23:11:58 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"KCaZ2qQI\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1639087918;\n\tbh=atgYBd3WNZFCULeQIVtCBL0kJlV9/YpuXuGKyTr0M6I=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=KCaZ2qQI0U/t7m0RT4eDJhdbEQx6qsivcZEDj2K8k2h+MABF2J9GCN3e54G8jUSX+\n\tk+PUuLvLB4bc3peblhwNixYbW3akQkj16zte5adtHaTODOSQVtbXDId++Vh7TJu4yi\n\tyqWJbIFGRsJza9aogLCx+TPpd1ZIu5w7OXUEANmw=","Date":"Fri, 10 Dec 2021 00:11:29 +0200","From":"Laurent Pinchart ","To":"David Plowman ","Message-ID":"","References":"<20211209101245.6187-1-david.plowman@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211209101245.6187-1-david.plowman@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v10 0/8] Colour spaces","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 , libcamera-devel@lists.libcamera.org,\n\tHans Verkuil ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]