[{"id":20691,"web_url":"https://patchwork.libcamera.org/comment/20691/","msgid":"<163611684736.275423.1727129360493341563@Monstersaurus>","date":"2021-11-05T12:54:07","subject":"Re: [libcamera-devel] [PATCH v5 6/7] libcamera: Add\n\tvalidateColorSpaces to CameraConfiguration class","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-04 13:58:04)\n> This method checks that the requested color spaces are sensible and do\n> not contain any undefined enum values. It also initialises the\n> \"actual\" color space field to the same value as the one requested, in\n> the expectation that the rest of the validate() method will be able to\n> check this.\n> \n> Signed-off-by: David Plowman \n> Reviewed-by: Naushir Patuck \n> ---\n> include/libcamera/camera.h | 2 ++\n> src/libcamera/camera.cpp | 51 ++++++++++++++++++++++++++++++++++++++\n> 2 files changed, 53 insertions(+)\n> \n> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> index 601ee46e..fdab4410 100644\n> --- a/include/libcamera/camera.h\n> +++ b/include/libcamera/camera.h\n> @@ -69,6 +69,8 @@ public:\n> protected:\n> CameraConfiguration();\n> \n> + Status validateColorSpaces(bool sharedColorSpace);\n> +\n> std::vector config_;\n> };\n> \n> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> index 400a7cf0..90e9460b 100644\n> --- a/src/libcamera/camera.cpp\n> +++ b/src/libcamera/camera.cpp\n> @@ -20,6 +20,7 @@\n> \n> #include \"libcamera/internal/camera.h\"\n> #include \"libcamera/internal/camera_controls.h\"\n> +#include \"libcamera/internal/formats.h\"\n> #include \"libcamera/internal/pipeline_handler.h\"\n> \n> /**\n> @@ -314,6 +315,56 @@ std::size_t CameraConfiguration::size() const\n> return config_.size();\n> }\n> \n> +static bool isRaw(const PixelFormat &pixFmt)\n\nThis looks like it should be a helper added to the PixelFormat class as\na patch of it's own? I'm not sure it belongs here in the camera class.\n\n> +{\n> + const PixelFormatInfo &info = PixelFormatInfo::info(pixFmt);\n> + return info.isValid() &&\n> + info.colourEncoding == PixelFormatInfo::ColourEncodingRAW;\n> +}\n> +\n\nI think this is missing doxygen documentation.\n\nWhat's the definition of a 'sharedColorSpace' ?\n\n> +CameraConfiguration::Status CameraConfiguration::validateColorSpaces(bool sharedColorSpace)\n> +{\n> + Status status = Valid;\n> +\n> + /* Find the largest non-raw stream (if any). */\n> + int index = -1;\n> + for (unsigned int i = 0; i < config_.size(); i++) {\n> + const StreamConfiguration &cfg = config_[i];\n> + if (!isRaw(cfg.pixelFormat) && (index < 0 || cfg.size > config_[i].size))\n> + index = i;\n> + }\n> +\n\nGiven the below usage of index, I'd be tempted to put an \n\n\tASSERT(index != -1);\nhere.\n\n\n> + /*\n> + * Here we force raw streams to the correct color space and signal\n> + * an error if we encounter anything undefined. We handle the case\n> + * where all output streams are to share a color space, which we\n> + * choose to be the color space of the largest stream.\n> + */\n> + for (auto &cfg : config_) {\n> + ColorSpace initialColorSpace = cfg.requestedColorSpace;\n> +\n> + if (isRaw(cfg.pixelFormat))\n> + cfg.requestedColorSpace = ColorSpace::Raw;\n> + else if (!cfg.requestedColorSpace.isFullyDefined()) {\n> + LOG(Camera, Error) << \"Stream has undefined color space\";\n> + cfg.requestedColorSpace = ColorSpace::Jpeg;\n> + } else if (sharedColorSpace)\n> + cfg.requestedColorSpace = config_[index].requestedColorSpace;\n\nCan index ever be -1 here?\n\n\n> +\n> + if (cfg.requestedColorSpace != initialColorSpace)\n> + status = Adjusted;\n> +\n> + /*\n> + * We also initialise the actual color space as if the\n> + * hardware can do what we want. But note that the rest\n> + * of the validate() method may change this.\n> + */\n> + cfg.actualColorSpace = cfg.requestedColorSpace;\n> + }\n> +\n> + return status;\n> +}\n> +\n> /**\n> * \\var CameraConfiguration::transform\n> * \\brief User-specified transform to be applied to the image\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 7AE25BDB1C\n\tfor ;\n\tFri, 5 Nov 2021 12:54:11 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E12B56034A;\n\tFri, 5 Nov 2021 13:54:10 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3ED2D600B8\n\tfor ;\n\tFri, 5 Nov 2021 13:54:10 +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 D02281908;\n\tFri, 5 Nov 2021 13:54:09 +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=\"g+Drxmmu\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1636116849;\n\tbh=eXaqveCoWacA9mQzO+n1rReGHTw5jvg4T3OE+c7Stik=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=g+DrxmmuNMRXbWiJqzmlysItczcNMnfCMuMpHtduMVn0DtH/saWVwyMcYILSlC2o5\n\tMNMurNyY3SNfNeoLEy8GbFx8qtoUOvTFRcEs7dL8wDdDoQ1B7PGIt0jML8h9Jp4OfO\n\thEB6K6TrSGbGYwmq+rak/ZFoy94zpbkTJYdSzxHg=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20211104135805.5269-7-david.plowman@raspberrypi.com>","References":"<20211104135805.5269-1-david.plowman@raspberrypi.com>\n\t<20211104135805.5269-7-david.plowman@raspberrypi.com>","From":"Kieran Bingham ","To":"David Plowman ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Fri, 05 Nov 2021 12:54:07 +0000","Message-ID":"<163611684736.275423.1727129360493341563@Monstersaurus>","User-Agent":"alot/0.9.1","Subject":"Re: [libcamera-devel] [PATCH v5 6/7] libcamera: Add\n\tvalidateColorSpaces to CameraConfiguration class","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\" "}}]