[{"id":17526,"web_url":"https://patchwork.libcamera.org/comment/17526/","msgid":"<CAEmqJPraO6Fkmeiy=JD5Eyop9LDa9ikbiBAe_VBqBWaUJ0gXMA@mail.gmail.com>","date":"2021-06-14T11:04:32","subject":"Re: [libcamera-devel] [RFC PATCH 3/3] libcamera: pipeline:\n\traspberrypi: Support colour spaces","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi David,\n\nThank you for your patch.\n\nOn Tue, 8 Jun 2021 at 15:44, David Plowman <david.plowman@raspberrypi.com>\nwrote:\n\n> The Raspberry Pi pipeline handler now sets colour spaces correctly.\n>\n> In generateConfiguration() is sets them to reasonable default values\n>\n\nTypo:\ns\\is\\it\\\n\n\n> based on the stream role. Note how video recording streams use the\n> \"VIDEO\" YCbCr encoding, which will later be fixed up according to the\n> requested resolution.\n>\n> validate() and configure() check the colour space is valid, and also\n> force all (non-raw) output streams to share the same colour space\n> (which is a hardware restriction). Finally, the \"VIDEO\" YCbCr encoding\n> is corrected by configure() to be one of REC601, REC709 or REC2020.\n> ---\n>  .../pipeline/raspberrypi/raspberrypi.cpp      | 84 +++++++++++++++++++\n>  1 file changed, 84 insertions(+)\n>\n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index a65b4568..acb4a1c3 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -279,6 +279,24 @@ RPiCameraConfiguration::RPiCameraConfiguration(const\n> RPiCameraData *data)\n>  {\n>  }\n>\n> +static bool validateColorSpace(ColorSpace &colorSpace)\n> +{\n> +       const std::vector<ColorSpace> validColorSpaces = {\n> +               ColorSpace::JFIF,\n> +               ColorSpace::SMPTE170M,\n> +               ColorSpace::REC709,\n> +               ColorSpace::REC2020,\n> +               ColorSpace::VIDEO,\n> +       };\n> +       auto it = std::find_if(validColorSpaces.begin(),\n> validColorSpaces.end(),\n> +                              [&colorSpace](const ColorSpace &item) {\n> return colorSpace == item; });\n>\n\nYou could use std::find() for fewer characters to type :-)\n\n\n> +       if (it != validColorSpaces.end())\n> +               return true;\n> +\n> +       colorSpace = ColorSpace::JFIF;\n> +       return false;\n> +}\n> +\n>  CameraConfiguration::Status RPiCameraConfiguration::validate()\n>  {\n>         Status status = Valid;\n> @@ -344,6 +362,7 @@ CameraConfiguration::Status\n> RPiCameraConfiguration::validate()\n>         unsigned int rawCount = 0, outCount = 0, count = 0, maxIndex = 0;\n>         std::pair<int, Size> outSize[2];\n>         Size maxSize;\n> +       ColorSpace colorSpace; /* colour space for all non-raw output\n> streams */\n>         for (StreamConfiguration &cfg : config_) {\n>                 if (isRaw(cfg.pixelFormat)) {\n>                         /*\n> @@ -352,6 +371,7 @@ CameraConfiguration::Status\n> RPiCameraConfiguration::validate()\n>                          */\n>                         V4L2VideoDevice::Formats fmts =\n> data_->unicam_[Unicam::Image].dev()->formats();\n>                         V4L2DeviceFormat sensorFormat = findBestMode(fmts,\n> cfg.size);\n> +                       sensorFormat.colorSpace = ColorSpace::RAW;\n>                         int ret =\n> data_->unicam_[Unicam::Image].dev()->tryFormat(&sensorFormat);\n>                         if (ret)\n>                                 return Invalid;\n> @@ -390,6 +410,7 @@ CameraConfiguration::Status\n> RPiCameraConfiguration::validate()\n>                         if (maxSize < cfg.size) {\n>                                 maxSize = cfg.size;\n>                                 maxIndex = outCount;\n> +                               colorSpace = cfg.colorSpace;\n>                         }\n>                         outCount++;\n>                 }\n> @@ -403,6 +424,10 @@ CameraConfiguration::Status\n> RPiCameraConfiguration::validate()\n>                 }\n>         }\n>\n> +       /* Ensure the output colour space (if present) is one we handle. */\n> +       if (outCount)\n> +               validateColorSpace(colorSpace);\n> +\n>         /*\n>          * Now do any fixups needed. For the two ISP outputs, one stream\n> must be\n>          * equal or smaller than the other in all dimensions.\n> @@ -444,10 +469,26 @@ CameraConfiguration::Status\n> RPiCameraConfiguration::validate()\n>                         status = Adjusted;\n>                 }\n>\n> +               /* Output streams must share the same colour space. */\n> +               if (cfg.colorSpace != colorSpace) {\n> +                       LOG(RPI, Warning) << \"Output stream \" << (i ==\n> maxIndex ? 0 : 1)\n> +                                         << \" colour space changed\";\n> +                       cfg.colorSpace = colorSpace;\n> +                       status = Adjusted;\n> +               }\n> +\n>                 V4L2DeviceFormat format;\n>                 format.fourcc = dev->toV4L2PixelFormat(cfg.pixelFormat);\n>                 format.size = cfg.size;\n>\n> +               /*\n> +                * Request a sensible colour space. Note that \"VIDEO\"\n> isn't a real\n> +                * encoding, so substitute something else sensible.\n> +                */\n> +               format.colorSpace = colorSpace;\n> +               if (format.colorSpace.encoding ==\n> ColorSpace::Encoding::VIDEO)\n> +                       format.colorSpace.encoding =\n> ColorSpace::Encoding::REC601;\n> +\n>                 int ret = dev->tryFormat(&format);\n>                 if (ret)\n>                         return Invalid;\n> @@ -473,6 +514,7 @@ CameraConfiguration\n> *PipelineHandlerRPi::generateConfiguration(Camera *camera,\n>         V4L2DeviceFormat sensorFormat;\n>         unsigned int bufferCount;\n>         PixelFormat pixelFormat;\n> +       ColorSpace colorSpace;\n>         V4L2VideoDevice::Formats fmts;\n>         Size size;\n>\n> @@ -488,6 +530,7 @@ CameraConfiguration\n> *PipelineHandlerRPi::generateConfiguration(Camera *camera,\n>                         fmts =\n> data->unicam_[Unicam::Image].dev()->formats();\n>                         sensorFormat = findBestMode(fmts, size);\n>                         pixelFormat = sensorFormat.fourcc.toPixelFormat();\n> +                       colorSpace = ColorSpace::RAW;\n>                         ASSERT(pixelFormat.isValid());\n>                         bufferCount = 2;\n>                         rawCount++;\n> @@ -499,6 +542,7 @@ CameraConfiguration\n> *PipelineHandlerRPi::generateConfiguration(Camera *camera,\n>                         /* Return the largest sensor resolution. */\n>                         size = data->sensor_->resolution();\n>                         bufferCount = 1;\n> +                       colorSpace = ColorSpace::JFIF;\n>                         outCount++;\n>                         break;\n>\n> @@ -515,6 +559,7 @@ CameraConfiguration\n> *PipelineHandlerRPi::generateConfiguration(Camera *camera,\n>                         pixelFormat = formats::YUV420;\n>                         size = { 1920, 1080 };\n>                         bufferCount = 4;\n> +                       colorSpace = ColorSpace::VIDEO;\n>                         outCount++;\n>                         break;\n>\n> @@ -523,6 +568,7 @@ CameraConfiguration\n> *PipelineHandlerRPi::generateConfiguration(Camera *camera,\n>                         pixelFormat = formats::ARGB8888;\n>                         size = { 800, 600 };\n>                         bufferCount = 4;\n> +                       colorSpace = ColorSpace::JFIF;\n>                         outCount++;\n>                         break;\n>\n> @@ -552,6 +598,7 @@ CameraConfiguration\n> *PipelineHandlerRPi::generateConfiguration(Camera *camera,\n>                 StreamConfiguration cfg(formats);\n>                 cfg.size = size;\n>                 cfg.pixelFormat = pixelFormat;\n> +               cfg.colorSpace = colorSpace;\n>                 cfg.bufferCount = bufferCount;\n>                 config->addConfiguration(cfg);\n>         }\n> @@ -573,6 +620,7 @@ int PipelineHandlerRPi::configure(Camera *camera,\n> CameraConfiguration *config)\n>         Size maxSize, sensorSize;\n>         unsigned int maxIndex = 0;\n>         bool rawStream = false;\n> +       ColorSpace colorSpace; /* colour space for all non-raw output\n> streams */\n>\n>         /*\n>          * Look for the RAW stream (if given) size as well as the largest\n> @@ -591,14 +639,40 @@ int PipelineHandlerRPi::configure(Camera *camera,\n> CameraConfiguration *config)\n>                 } else {\n>                         if (cfg.size > maxSize) {\n>                                 maxSize = config->at(i).size;\n> +                               colorSpace = config->at(i).colorSpace;\n>                                 maxIndex = i;\n>                         }\n>                 }\n>         }\n>\n> +       if (maxSize.isNull()) {\n> +               /*\n> +                * No non-raw streams, so some will get made below.\n> Doesn't matter\n> +                * what colour space we assign to them.\n> +                */\n> +               colorSpace = ColorSpace::JFIF;\n> +       } else {\n> +               /* Make sure we can handle this colour space. */\n> +               validateColorSpace(colorSpace);\n> +\n> +               /*\n> +                * The \"VIDEO\" colour encoding means that we choose one of\n> REC601,\n> +                * REC709 or REC2020 automatically according to the\n> resolution.\n> +                */\n> +               if (colorSpace.encoding == ColorSpace::Encoding::VIDEO) {\n> +                       if (maxSize.width >= 3840)\n> +                               colorSpace.encoding =\n> ColorSpace::Encoding::REC2020;\n> +                       else if (maxSize.width >= 1280)\n> +                               colorSpace.encoding =\n> ColorSpace::Encoding::REC709;\n> +                       else\n> +                               colorSpace.encoding =\n> ColorSpace::Encoding::REC601;\n> +               }\n> +       }\n> +\n>         /* First calculate the best sensor mode we can use based on the\n> user request. */\n>         V4L2VideoDevice::Formats fmts =\n> data->unicam_[Unicam::Image].dev()->formats();\n>         V4L2DeviceFormat sensorFormat = findBestMode(fmts, rawStream ?\n> sensorSize : maxSize);\n> +       sensorFormat.colorSpace = ColorSpace::RAW;\n>\n>         /*\n>          * Unicam image output format. The ISP input format gets set at\n> start,\n> @@ -636,11 +710,18 @@ int PipelineHandlerRPi::configure(Camera *camera,\n> CameraConfiguration *config)\n>                 StreamConfiguration &cfg = config->at(i);\n>\n>                 if (isRaw(cfg.pixelFormat)) {\n> +                       cfg.colorSpace = ColorSpace::RAW;\n>                         cfg.setStream(&data->unicam_[Unicam::Image]);\n>                         data->unicam_[Unicam::Image].setExternal(true);\n>                         continue;\n>                 }\n>\n> +               /* All other streams share the same colour space. */\n> +               if (cfg.colorSpace != colorSpace) {\n> +                       LOG(RPI, Warning) << \"Stream \" << i << \" colour\n> space changed\";\n>\n\nPerhaps this should be a Info log level?\n\nAll things here are minors so\n\nReviewed-by: Naushir Patuck <naush@raspberrypi.com>\n\n\n> +                       cfg.colorSpace = colorSpace;\n> +               }\n> +\n>                 /* The largest resolution gets routed to the ISP Output 0\n> node. */\n>                 RPi::Stream *stream = i == maxIndex ?\n> &data->isp_[Isp::Output0]\n>                                                     :\n> &data->isp_[Isp::Output1];\n> @@ -648,6 +729,7 @@ int PipelineHandlerRPi::configure(Camera *camera,\n> CameraConfiguration *config)\n>                 V4L2PixelFormat fourcc =\n> stream->dev()->toV4L2PixelFormat(cfg.pixelFormat);\n>                 format.size = cfg.size;\n>                 format.fourcc = fourcc;\n> +               format.colorSpace = cfg.colorSpace;\n>\n>                 LOG(RPI, Debug) << \"Setting \" << stream->name() << \" to \"\n>                                 << format.toString();\n> @@ -687,6 +769,7 @@ int PipelineHandlerRPi::configure(Camera *camera,\n> CameraConfiguration *config)\n>                 format = {};\n>                 format.size = maxSize;\n>                 format.fourcc =\n> V4L2PixelFormat::fromPixelFormat(formats::YUV420, false);\n> +               format.colorSpace = colorSpace;\n>                 ret = data->isp_[Isp::Output0].dev()->setFormat(&format);\n>                 if (ret) {\n>                         LOG(RPI, Error)\n> @@ -716,6 +799,7 @@ int PipelineHandlerRPi::configure(Camera *camera,\n> CameraConfiguration *config)\n>                 const Size limit =\n> maxDimensions.boundedToAspectRatio(format.size);\n>\n>                 output1Format.size = (format.size /\n> 2).boundedTo(limit).alignedDownTo(2, 2);\n> +               output1Format.colorSpace = colorSpace;\n>\n>                 LOG(RPI, Debug) << \"Setting ISP Output1 (internal) to \"\n>                                 << output1Format.toString();\n> --\n> 2.20.1\n>\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 3AAE6BD78E\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 14 Jun 2021 11:04:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A937B68932;\n\tMon, 14 Jun 2021 13:04:50 +0200 (CEST)","from mail-lf1-x129.google.com (mail-lf1-x129.google.com\n\t[IPv6:2a00:1450:4864:20::129])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B86806029D\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 14 Jun 2021 13:04:49 +0200 (CEST)","by mail-lf1-x129.google.com with SMTP id v22so20484009lfa.3\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 14 Jun 2021 04:04:49 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"QD5eySlz\"; 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=SoINVl3t1KJTxGheqFWVyLK01MXyRvXsfnwFCv/4dc4=;\n\tb=QD5eySlzCvQXEth3tu6IuadZpudq9v2XaoIViIrUj3aIy07xDpqUBvihteuJQjNG/L\n\tkPOnjlz0lLU9mnpyFjp9KxS0eLoD44JWgAiwtahQg3BOpnf+K1ikhuXAVCSVUQ15+0Hh\n\t0yniudlFepVlJmtSThumHizQwy4Idm1Ed1h/6cFsDjWglfd0Rhqsyu8cGHSzCGCFIzUf\n\tv60Ch0DfhuEi0TjUFqaxpLSLNUT3F4LsCKSWZUcaMHRT1L+0p+pFfIYef95+T540Xs+5\n\t+6Z++yoUsh/tLyBc3l/2yyqqWppExZieQ4/09/Sqo1i4Mww3DPVA1AR4pmuBAJnEIx9u\n\tkJ/A==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=SoINVl3t1KJTxGheqFWVyLK01MXyRvXsfnwFCv/4dc4=;\n\tb=Jzp0ZOzyOwc7c9c4H6SWHvCrOWrJ40W2skqdafGHd2JD7xN6RlZMGGZh2puFPDHtYt\n\tYrlx1kZ6H5N+T35KFWu73mx2AnSQLbibmrnxWuuKklUG89tzNhTkXknz4ebJlZeLQqb2\n\tg4nibJbIsfGJ1uqbKldP04/YDNFIamk8ovBITIFoKrhJAWVK15nhJ7La8HfRXGQ7yDvn\n\tFOwhW8nqXOcLFRKIwDcDjOo/ZpCMfFRQVzHVIGF3+EhgOpiD0YopNs70f0wdxS59/FUC\n\toWNLM+A5W0bNKfLWmXjH5rMIBks98UguFvHzVy9AjAvV6mcNSz8bqTzIKSbMUw7fJY0B\n\tbfFQ==","X-Gm-Message-State":"AOAM531ZH490/Z0yEFrX6yZaeqqzha1FOUcR1WogaPsYpHCyobDkOKbp\n\tU+1Dl2mn4tf/LXqwL0oVkfKy+XNFWhZeybLHP6fqeg==","X-Google-Smtp-Source":"ABdhPJz1+L5/y1PYyhscWLvyFxfaws11harUJ6PB021LFnDCLFJuEUhqEpIdx0IkK4MV2o4iIRbgplbiilLXNIvRLBc=","X-Received":"by 2002:a05:6512:3054:: with SMTP id\n\tb20mr11457850lfb.375.1623668689049; \n\tMon, 14 Jun 2021 04:04:49 -0700 (PDT)","MIME-Version":"1.0","References":"<20210608144413.1529-1-david.plowman@raspberrypi.com>\n\t<20210608144413.1529-4-david.plowman@raspberrypi.com>","In-Reply-To":"<20210608144413.1529-4-david.plowman@raspberrypi.com>","From":"Naushir Patuck <naush@raspberrypi.com>","Date":"Mon, 14 Jun 2021 12:04:32 +0100","Message-ID":"<CAEmqJPraO6Fkmeiy=JD5Eyop9LDa9ikbiBAe_VBqBWaUJ0gXMA@mail.gmail.com>","To":"David Plowman <david.plowman@raspberrypi.com>","Content-Type":"multipart/alternative; boundary=\"00000000000071a1e205c4b7d3e0\"","Subject":"Re: [libcamera-devel] [RFC PATCH 3/3] libcamera: pipeline:\n\traspberrypi: Support colour spaces","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera devel <libcamera-devel@lists.libcamera.org>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]