[{"id":21746,"web_url":"https://patchwork.libcamera.org/comment/21746/","msgid":"","date":"2021-12-10T12:49:47","subject":"Re: [libcamera-devel] [PATCH v11 7/8] libcamera: Add\n\tvalidateColorSpaces to CameraConfiguration class","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi David,\n\nThank you for the patch.\n\nOn Fri, Dec 10, 2021 at 11:21:41AM +0000, David Plowman wrote:\n> This function forces raw streams to have the \"raw\" color space, and\n> also optionally makes all non-raw output streams to share the same\n> color space as some platforms may require this.\n> \n> When sharing color spaces we take the shared value to be the one from\n> the largest of these streams. This choice is ultimately arbitrary, but\n> can be appropriate if smaller output streams are used for image\n> analysis rather than human consumption, when the precise colours may\n> be less important.\n> \n> Signed-off-by: David Plowman \n> ---\n> include/libcamera/camera.h | 10 +++++\n> src/libcamera/camera.cpp | 80 ++++++++++++++++++++++++++++++++++++++\n> 2 files changed, 90 insertions(+)\n> \n> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> index a7759ccb..5bb06584 100644\n> --- a/include/libcamera/camera.h\n> +++ b/include/libcamera/camera.h\n> @@ -13,6 +13,7 @@\n> #include \n> \n> #include \n> +#include \n> #include \n> #include \n> \n> @@ -69,6 +70,15 @@ public:\n> protected:\n> \tCameraConfiguration();\n> \n> +\tenum class ColorSpaceFlag {\n> +\t\tNone,\n> +\t\tStreamsShareColorSpace,\n> +\t};\n> +\n> +\tusing ColorSpaceFlags = Flags;\n> +\n> +\tStatus validateColorSpaces(ColorSpaceFlags flags = ColorSpaceFlag::None);\n> +\n> \tstd::vector config_;\n> };\n> \n> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> index 400a7cf0..5f8533e8 100644\n> --- a/src/libcamera/camera.cpp\n> +++ b/src/libcamera/camera.cpp\n> @@ -14,12 +14,14 @@\n> #include \n> #include \n> \n> +#include \n> #include \n> #include \n> #include \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 +316,84 @@ std::size_t CameraConfiguration::size() const\n> \treturn config_.size();\n> }\n> \n> +namespace {\n> +\n> +bool isRaw(const PixelFormat &pixFmt)\n> +{\n> +\tconst PixelFormatInfo &info = PixelFormatInfo::info(pixFmt);\n> +\treturn info.isValid() &&\n> +\t info.colourEncoding == PixelFormatInfo::ColourEncodingRAW;\n> +}\n> +\n> +} /* namespace */\n> +\n> +/**\n> + * \\enum CameraConfiguration::ColorSpaceFlag\n> + * \\brief Specify the behaviour of validateColorSpaces\n> + * \\var CameraConfiguration::ColorSpaceFlag::None\n> + * \\brief No extra validation of color spaces is required\n> + * \\var CameraConfiguration::ColorSpaceFlag::StreamsShareColorSpace\n> + * \\brief Non-raw output streams must share the same color space\n> + */\n> +\n> +/**\n> + * \\typedef CameraConfiguration::ColorSpaceFlags\n> + * \\brief A bitwise combination of ColorSpaceFlag values\n> + */\n> +\n> +/**\n> + * \\brief Check the color spaces requested for each stream\n> + * \\param[in] flags Flags to control the behaviour of this function\n> + *\n> + * This function performs certain consistency checks on the color spaces of\n> + * the streams and may adjust them so that:\n> + *\n> + * - Any raw streams have the Raw color space\n> + * - If the StreamsShareColorSpace flag is set, all output streams are forced\n> + * to share the same color space (this may be a constraint on some platforms).\n> + *\n> + * It is optional for a pipeline handler to use this function.\n> + *\n> + * \\return A CameraConfiguration::Status value that describes the validation\n> + * status.\n> + * \\retval CameraConfigutation::Adjusted The configuration has been adjusted\n> + * and is now valid. The color space of some or all of the streams may bave\n> + * benn changed. The caller shall check the color spaces carefully.\n> + * \\retval CameraConfiguration::Valid The configuration was already valid and\n> + * hasn't been adjusted.\n> + */\n> +CameraConfiguration::Status CameraConfiguration::validateColorSpaces(ColorSpaceFlags flags)\n> +{\n> +\tStatus status = Valid;\n> +\n> +\t/*\n> +\t * Set all raw streams to the Raw color space, and make a note of the largest\n> +\t * non-raw stream with a defined color space (if there is one).\n> +\t */\n> +\tint index = -1;\n> +\tfor (auto [i, cfg] : utils::enumerate(config_)) {\n> +\t\tif (isRaw(cfg.pixelFormat) && cfg.colorSpace != ColorSpace::Raw) {\n> +\t\t\tcfg.colorSpace = ColorSpace::Raw;\n> +\t\t\tstatus = Adjusted;\n> +\t\t} else if (cfg.colorSpace && (index == -1 || cfg.size > config_[i].size))\n> +\t\t\tindex = i;\n\n\t\t} else if (cfg.colorSpace && (index == -1 || cfg.size > config_[i].size)) {\n\t\t\tindex = i;\n\t\t}\n\nReviewed-by: Laurent Pinchart \n\n> +\t}\n> +\n> +\tif (index < 0 || !(flags & ColorSpaceFlag::StreamsShareColorSpace))\n> +\t\treturn status;\n> +\n> +\t/* Make all output color spaces the same, if requested. */\n> +\tfor (auto &cfg : config_) {\n> +\t\tif (!isRaw(cfg.pixelFormat) &&\n> +\t\t cfg.colorSpace != config_[index].colorSpace) {\n> +\t\t\tcfg.colorSpace = config_[index].colorSpace;\n> +\t\t\tstatus = Adjusted;\n> +\t\t}\n> +\t}\n> +\n> +\treturn status;\n> +}\n> +\n> /**\n> * \\var CameraConfiguration::transform\n> * \\brief User-specified transform to be applied to the image","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 9572CBDB13\n\tfor ;\n\tFri, 10 Dec 2021 12:50:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4E7B760890;\n\tFri, 10 Dec 2021 13:50:18 +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 4B8B960868\n\tfor ;\n\tFri, 10 Dec 2021 13:50:17 +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 BA833F84;\n\tFri, 10 Dec 2021 13:50:16 +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=\"uslWbK1c\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1639140617;\n\tbh=h8VoMo6a9YWZsya55F7zJ440M0hlDmg++WfehM9uz+c=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=uslWbK1c14s/omVOwLkUlNst2wv5NQYEjSEnrbs1dR0CBJqFNAHRLSZ4WpeUZOgGY\n\tsBwuhpll3UzvDG8WD9+PcHjDuZ1UeCiL9fFIQGdjdh7WqlVvRhAJTrIKxLx0xhJyma\n\tkqMNRwFTD7dD0bUyIcnp786Dg6unFIvHWF7y9rZY=","Date":"Fri, 10 Dec 2021 14:49:47 +0200","From":"Laurent Pinchart ","To":"David Plowman ","Message-ID":"","References":"<20211210112142.18441-1-david.plowman@raspberrypi.com>\n\t<20211210112142.18441-8-david.plowman@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211210112142.18441-8-david.plowman@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v11 7/8] 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","Cc":"Tomasz Figa , libcamera-devel@lists.libcamera.org,\n\tHans Verkuil ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":21749,"web_url":"https://patchwork.libcamera.org/comment/21749/","msgid":"","date":"2021-12-10T13:10:02","subject":"Re: [libcamera-devel] [PATCH v11 7/8] libcamera: Add\n\tvalidateColorSpaces to CameraConfiguration class","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Laurent\n\nThanks for the review.\n\nOn Fri, 10 Dec 2021 at 12:50, Laurent Pinchart\n wrote:\n>\n> Hi David,\n>\n> Thank you for the patch.\n>\n> On Fri, Dec 10, 2021 at 11:21:41AM +0000, David Plowman wrote:\n> > This function forces raw streams to have the \"raw\" color space, and\n> > also optionally makes all non-raw output streams to share the same\n> > color space as some platforms may require this.\n> >\n> > When sharing color spaces we take the shared value to be the one from\n> > the largest of these streams. This choice is ultimately arbitrary, but\n> > can be appropriate if smaller output streams are used for image\n> > analysis rather than human consumption, when the precise colours may\n> > be less important.\n> >\n> > Signed-off-by: David Plowman \n> > ---\n> > include/libcamera/camera.h | 10 +++++\n> > src/libcamera/camera.cpp | 80 ++++++++++++++++++++++++++++++++++++++\n> > 2 files changed, 90 insertions(+)\n> >\n> > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> > index a7759ccb..5bb06584 100644\n> > --- a/include/libcamera/camera.h\n> > +++ b/include/libcamera/camera.h\n> > @@ -13,6 +13,7 @@\n> > #include \n> >\n> > #include \n> > +#include \n> > #include \n> > #include \n> >\n> > @@ -69,6 +70,15 @@ public:\n> > protected:\n> > CameraConfiguration();\n> >\n> > + enum class ColorSpaceFlag {\n> > + None,\n> > + StreamsShareColorSpace,\n> > + };\n> > +\n> > + using ColorSpaceFlags = Flags;\n> > +\n> > + Status validateColorSpaces(ColorSpaceFlags flags = ColorSpaceFlag::None);\n> > +\n> > std::vector config_;\n> > };\n> >\n> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > index 400a7cf0..5f8533e8 100644\n> > --- a/src/libcamera/camera.cpp\n> > +++ b/src/libcamera/camera.cpp\n> > @@ -14,12 +14,14 @@\n> > #include \n> > #include \n> >\n> > +#include \n> > #include \n> > #include \n> > #include \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 +316,84 @@ std::size_t CameraConfiguration::size() const\n> > return config_.size();\n> > }\n> >\n> > +namespace {\n> > +\n> > +bool isRaw(const PixelFormat &pixFmt)\n> > +{\n> > + const PixelFormatInfo &info = PixelFormatInfo::info(pixFmt);\n> > + return info.isValid() &&\n> > + info.colourEncoding == PixelFormatInfo::ColourEncodingRAW;\n> > +}\n> > +\n> > +} /* namespace */\n> > +\n> > +/**\n> > + * \\enum CameraConfiguration::ColorSpaceFlag\n> > + * \\brief Specify the behaviour of validateColorSpaces\n> > + * \\var CameraConfiguration::ColorSpaceFlag::None\n> > + * \\brief No extra validation of color spaces is required\n> > + * \\var CameraConfiguration::ColorSpaceFlag::StreamsShareColorSpace\n> > + * \\brief Non-raw output streams must share the same color space\n> > + */\n> > +\n> > +/**\n> > + * \\typedef CameraConfiguration::ColorSpaceFlags\n> > + * \\brief A bitwise combination of ColorSpaceFlag values\n> > + */\n> > +\n> > +/**\n> > + * \\brief Check the color spaces requested for each stream\n> > + * \\param[in] flags Flags to control the behaviour of this function\n> > + *\n> > + * This function performs certain consistency checks on the color spaces of\n> > + * the streams and may adjust them so that:\n> > + *\n> > + * - Any raw streams have the Raw color space\n> > + * - If the StreamsShareColorSpace flag is set, all output streams are forced\n> > + * to share the same color space (this may be a constraint on some platforms).\n> > + *\n> > + * It is optional for a pipeline handler to use this function.\n> > + *\n> > + * \\return A CameraConfiguration::Status value that describes the validation\n> > + * status.\n> > + * \\retval CameraConfigutation::Adjusted The configuration has been adjusted\n> > + * and is now valid. The color space of some or all of the streams may bave\n> > + * benn changed. The caller shall check the color spaces carefully.\n> > + * \\retval CameraConfiguration::Valid The configuration was already valid and\n> > + * hasn't been adjusted.\n> > + */\n> > +CameraConfiguration::Status CameraConfiguration::validateColorSpaces(ColorSpaceFlags flags)\n> > +{\n> > + Status status = Valid;\n> > +\n> > + /*\n> > + * Set all raw streams to the Raw color space, and make a note of the largest\n> > + * non-raw stream with a defined color space (if there is one).\n> > + */\n> > + int index = -1;\n> > + for (auto [i, cfg] : utils::enumerate(config_)) {\n> > + if (isRaw(cfg.pixelFormat) && cfg.colorSpace != ColorSpace::Raw) {\n> > + cfg.colorSpace = ColorSpace::Raw;\n> > + status = Adjusted;\n> > + } else if (cfg.colorSpace && (index == -1 || cfg.size > config_[i].size))\n> > + index = i;\n>\n> } else if (cfg.colorSpace && (index == -1 || cfg.size > config_[i].size)) {\n> index = i;\n> }\n\nYes, will add that!\n\nBest regards\n\nDavid\n\n>\n> Reviewed-by: Laurent Pinchart \n>\n> > + }\n> > +\n> > + if (index < 0 || !(flags & ColorSpaceFlag::StreamsShareColorSpace))\n> > + return status;\n> > +\n> > + /* Make all output color spaces the same, if requested. */\n> > + for (auto &cfg : config_) {\n> > + if (!isRaw(cfg.pixelFormat) &&\n> > + cfg.colorSpace != config_[index].colorSpace) {\n> > + cfg.colorSpace = config_[index].colorSpace;\n> > + status = Adjusted;\n> > + }\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> --\n> Regards,\n>\n> Laurent Pinchart","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 89709BF415\n\tfor ;\n\tFri, 10 Dec 2021 13:10:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D2F9160890;\n\tFri, 10 Dec 2021 14:10:14 +0100 (CET)","from mail-wr1-x42a.google.com (mail-wr1-x42a.google.com\n\t[IPv6:2a00:1450:4864:20::42a])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3569460868\n\tfor ;\n\tFri, 10 Dec 2021 14:10:13 +0100 (CET)","by mail-wr1-x42a.google.com with SMTP id v11so14808990wrw.10\n\tfor ;\n\tFri, 10 Dec 2021 05:10:13 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"GS8Mj0Pc\"; 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=yYZSTlpbw1fXQkiJJ0aiZiEfLoYqvzgWWng93k/nYzQ=;\n\tb=GS8Mj0PcZBzzQTa5mbesKcVv4vigiRnyoqfgJsNnqRKZQcJ2sAwU961+yCqcN76unu\n\tHmSr5zRsqFsiIQo/e1eHJ8fLZOfjmpV7mOff6dZn0/5WKzdtGbS6TcCRQKtUkh8ZxxLM\n\tOC2vQ/MeNnQlbmwf7/A4KOTNYUNg4LraLsDGYxswa+ajEBg/8WvUfEgphTI+H+yyCqXz\n\tOiNxgbQB2mTptV0oGtvjz8RSWBcTxmRr4r93szVYVsFAhCPmpcdK3UTj81yEUaBOngRl\n\tgwLhvFEfGKdmMo8W9LskOAKmdL9BsO2cd9cOuxPnv7omnaXYxkqLEAm7i1MB48sFj4NP\n\twwdQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=yYZSTlpbw1fXQkiJJ0aiZiEfLoYqvzgWWng93k/nYzQ=;\n\tb=kfHCiqsy2Z31Wxe6kFPo97cpY5j8jmBJK9yQyoPc0Ofx2AOKpF41IPm7YBlEYAQ9Qq\n\tofomj7gP1w+1rEQT9ABNwqa2COMrKT5+nAhWwOBRkaEukkle2IYUQyvW75V42KCNCzxe\n\tGAs/8RiZpfZDZNKRCNYauduP3JUD+zdlMEOLIjmxsOfCeNgfxeObmDCTWKHLSrdQyuA/\n\tUeZXAUIW07TrdhXiTt8Sx5XSfVMgSukU3z03QXkMQr7Z0ggyZ5yoa2MN8X1YryLNQ+tw\n\tMoDTKIJyYAWvZ4A+TBXSCpHcXuW4x1LdKczvxSSepUwebIoGbtsTcGW80lLDB3l72UGu\n\t1VYQ==","X-Gm-Message-State":"AOAM532+8mD1ZgIlMHMahQxXkHOs+zL84Nu0a7dD9dma5ZdrfT32UGgQ\n\tn1zjqJQOalN2FGJqfkbiENtv4HQeGxRBsUzcYdGLtA==","X-Google-Smtp-Source":"ABdhPJy0W8/LP2N+ZYC6PGtSwQ2ZMnoT7Dz8QXYXzwXQOgSdxcZam79q75MaIn+/2T+VJnHJ0xEI/4m+rWF8zj7vuRM=","X-Received":"by 2002:adf:f2ca:: with SMTP id\n\td10mr13615533wrp.79.1639141812751; \n\tFri, 10 Dec 2021 05:10:12 -0800 (PST)","MIME-Version":"1.0","References":"<20211210112142.18441-1-david.plowman@raspberrypi.com>\n\t<20211210112142.18441-8-david.plowman@raspberrypi.com>\n\t","In-Reply-To":"","From":"David Plowman ","Date":"Fri, 10 Dec 2021 13:10:02 +0000","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v11 7/8] 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","Cc":"Tomasz Figa ,\n\tlibcamera devel ,\n\tHans Verkuil ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]