[{"id":20420,"web_url":"https://patchwork.libcamera.org/comment/20420/","msgid":"","date":"2021-10-25T08:34:10","subject":"Re: [libcamera-devel] [PATCH v3 6/7] libcamera: Add\n\tvalidateColorSpaces to CameraConfiguration class","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 work.\n\n\nOn Wed, 20 Oct 2021 at 12:08, David Plowman \nwrote:\n\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> ---\n> include/libcamera/camera.h | 2 ++\n> src/libcamera/camera.cpp | 53 ++++++++++++++++++++++++++++++++++++++\n> 2 files changed, 55 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 71809bcd..79451ef4 100644\n> --- a/src/libcamera/camera.cpp\n> +++ b/src/libcamera/camera.cpp\n> @@ -19,6 +19,7 @@\n> #include \n>\n> #include \"libcamera/internal/camera.h\"\n> +#include \"libcamera/internal/formats.h\"\n> #include \"libcamera/internal/pipeline_handler.h\"\n>\n> /**\n> @@ -313,6 +314,58 @@ std::size_t CameraConfiguration::size() const\n> return config_.size();\n> }\n>\n> +static 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\nNot as part of this series, but we ought to move this to the\nPixelFomat class,\nthere is an identical function in our pipeline handler :-)\n\n\n> +\n> +CameraConfiguration::Status CameraConfiguration::validateColorSpaces(bool\n> 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) &&\n> + (index < 0 ||\n> + cfg.size.width * cfg.size.height >\n> config_[i].size.width * config_[i].size.height))\n>\n\nYou can replace this with \"cfg.size > config_[i].size\". Apart from that:\n\nReviewed-by: Naushir Patuck \n\n\n> + index = i;\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\n> space\";\n> + cfg.requestedColorSpace = ColorSpace::Jpeg;\n> + } else if (sharedColorSpace)\n> + cfg.requestedColorSpace =\n> config_[index].requestedColorSpace;\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>\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 AD130BF415\n\tfor ;\n\tMon, 25 Oct 2021 08:34:29 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1FEA760124;\n\tMon, 25 Oct 2021 10:34:29 +0200 (CEST)","from mail-lf1-x12d.google.com (mail-lf1-x12d.google.com\n\t[IPv6:2a00:1450:4864:20::12d])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5A7FF60124\n\tfor ;\n\tMon, 25 Oct 2021 10:34:27 +0200 (CEST)","by mail-lf1-x12d.google.com with SMTP id u11so2045819lfs.1\n\tfor ;\n\tMon, 25 Oct 2021 01:34:27 -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=\"NIk5YrQG\"; 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=LvbLIhO6pTJv9dRTKaD8iC4sg6SoEds3BkvxR+sAEtI=;\n\tb=NIk5YrQGvulgsNdf/aW9tzhlLz1Ftr+MkUF1iDFnepbJmeMB5zJgsGSkQhIybm0T+G\n\tCpDBuGYhun8gUuAMA3H/aQKTHRyl6OiHcva6oYiquzang8bNRqYhQ4Xex2FIpveW25g4\n\t2kZOyhUxu+kdcT3qHe2aW2qDeL+Y0lY4IgEIVWzLDV17y4q+KvE1901cL+4wxFdEJjx5\n\t0hZX2RFCbQZCre2++vKv4amBp7p1qAWrw2bX39DGUkDncIY5FNsP1se/NpiU7CwMDexm\n\thbe/+Ha4VUgsI8W7F3ucTXjXFdlgZVVCLpHvphSJc6o36zE74Q8waVqr7AxirHp0Ys6B\n\tNR7A==","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=LvbLIhO6pTJv9dRTKaD8iC4sg6SoEds3BkvxR+sAEtI=;\n\tb=1laKQ38/CWDEmihsCEZ9aY3Gd424efB1sePA60BQtaDMf9ksZYw4ZxjeiThVdtSZ9y\n\tP8V2x8WV/p++KfHLqum/Tm/rWaFTSra09LQeUyuK99l5N3NbEG9J3rPwMr5jCV+bmeSx\n\twqsNopuCdOrs3gljgLOLCQbBhvQdUc10djYD0qy/9DjpUqvzcNIPYw290pHh/iA7lkvk\n\t5GQ4uEmox53Ew9jDKoxD+iFbMygoBLU5asgEjgtytrSSo67KN0FrvM0dYV6AV7TP7LQ5\n\tEJIBH0RgY8L7PRZFWy4aSem8ANbseVF5tsnzgJEKzjvraiOrMOsF23cJhJKZWwoD5oZ/\n\td5DQ==","X-Gm-Message-State":"AOAM533BZ+YFZaMZwBFIrlUoyiNIRl1XKHzMTzocDDKS+mN+9odoztEJ\n\tPO6cvtBJOxRmG4MvEJC3Sf7TGkn0OrmsI1rQVxnAh269t+4YpQ==","X-Google-Smtp-Source":"ABdhPJzLMuCQ2VkxtXpU151q7HfNrv0f4xcHckmkAqxpCLAtmarifqxg6jTnuovV0AO9EzImFcWc5choNTpDz3eZpTc=","X-Received":"by 2002:a19:ad0d:: with SMTP id\n\tt13mr16017530lfc.161.1635150866691; \n\tMon, 25 Oct 2021 01:34:26 -0700 (PDT)","MIME-Version":"1.0","References":"<20211020110825.12902-1-david.plowman@raspberrypi.com>\n\t<20211020110825.12902-7-david.plowman@raspberrypi.com>","In-Reply-To":"<20211020110825.12902-7-david.plowman@raspberrypi.com>","From":"Naushir Patuck ","Date":"Mon, 25 Oct 2021 09:34:10 +0100","Message-ID":"","To":"David Plowman ","Content-Type":"multipart/alternative; boundary=\"00000000000090493805cf293a8f\"","Subject":"Re: [libcamera-devel] [PATCH v3 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","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20427,"web_url":"https://patchwork.libcamera.org/comment/20427/","msgid":"","date":"2021-10-25T09:56:32","subject":"Re: [libcamera-devel] [PATCH v3 6/7] 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 Naush\n\nThanks for the review.\n\nOn Mon, 25 Oct 2021 at 09:34, Naushir Patuck wrote:\n>\n> Hi David,\n>\n> Thank you for your work.\n>\n>\n> On Wed, 20 Oct 2021 at 12:08, David Plowman wrote:\n>>\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>> ---\n>> include/libcamera/camera.h | 2 ++\n>> src/libcamera/camera.cpp | 53 ++++++++++++++++++++++++++++++++++++++\n>> 2 files changed, 55 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 71809bcd..79451ef4 100644\n>> --- a/src/libcamera/camera.cpp\n>> +++ b/src/libcamera/camera.cpp\n>> @@ -19,6 +19,7 @@\n>> #include \n>>\n>> #include \"libcamera/internal/camera.h\"\n>> +#include \"libcamera/internal/formats.h\"\n>> #include \"libcamera/internal/pipeline_handler.h\"\n>>\n>> /**\n>> @@ -313,6 +314,58 @@ std::size_t CameraConfiguration::size() const\n>> return config_.size();\n>> }\n>>\n>> +static 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>\n> Not as part of this series, but we ought to move this to the PixelFomat class,\n> there is an identical function in our pipeline handler :-)\n\nHehe. I think I made the same comment about one of your patches recently!\n\n>\n>>\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) &&\n>> + (index < 0 ||\n>> + cfg.size.width * cfg.size.height > config_[i].size.width * config_[i].size.height))\n>\n>\n> You can replace this with \"cfg.size > config_[i].size\". Apart from that:\n\nWow, you can do that? How cool, I never knew!\n\nThanks\n\nDavid\n\n>\n> Reviewed-by: Naushir Patuck \n>\n>>\n>> + index = i;\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>> +\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 48FAFBDB1C\n\tfor ;\n\tMon, 25 Oct 2021 09:56:45 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A179664872;\n\tMon, 25 Oct 2021 11:56:44 +0200 (CEST)","from mail-wr1-x42d.google.com (mail-wr1-x42d.google.com\n\t[IPv6:2a00:1450:4864:20::42d])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 840BB60124\n\tfor ;\n\tMon, 25 Oct 2021 11:56:43 +0200 (CEST)","by mail-wr1-x42d.google.com with SMTP id d10so7682315wrb.1\n\tfor ;\n\tMon, 25 Oct 2021 02:56:43 -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=\"sSQ98b1w\"; 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=qYtIW0lJ5+1SayhrZTC/+zlovIZAkESgAL1MuCb9MO4=;\n\tb=sSQ98b1wiRFR/aEp2YXFv/TJYCj5iXW9wNQB55SgmZxAThprXD8R9yWHcSBtQyGZ57\n\t3/KV87kBaoDxnUWjKqPULSC27ksHV0P4GEuPldM+PkdchH8IwamR4GSqSkvvF4rSCvi5\n\tYdSPmd2S6TGVSP74PriCf5TIQ2P5AYvRn1nH+Njxet0pN12Jf2yokZIz/qQTHMJdZiNL\n\tApSpnwkZyAWmML8ykhzy98H8OffX04h72GT0tuL3X0joK3KOR5dYfKPMWhECu1QuoPRq\n\tK0PbSXoqlzFrtcQzdYIF+92F+57ov7ynfi6SfwdsIf+rug+ZPBC5gZ8eWfJiht5UjphG\n\trG3A==","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=qYtIW0lJ5+1SayhrZTC/+zlovIZAkESgAL1MuCb9MO4=;\n\tb=ivO8cmfXBpbpzRdkl/B8DKXQRf4Oq7P5DYPRV4AUI+VvRzJNR6F5Xr5fs5QX31Rb3R\n\tt9IfdvZy7TDGXaPB7ZQfhXDDBZ3hCfmg/lCUV9s8yB9JKP/fNZbUtDtoaJs+ikSiOYm5\n\tLTDzdKOSZMfX6VxGqTk+GMpcNjcdeOlkwGUG79vHhOw0s/IuuvSYbUNgq8O7jCNMyZP6\n\t3IXqWsPoun2OvEovbPn7QihRVoS5ctKblafTMu3xpKF0j2UOXoiOa6SoP8S8DFEE9fSJ\n\t/UtR91e+igqRAiPuGcpIfHYbwX0hg49uUI8aSq0434o6e7nqFtTn3TEG+CJ8T6i91NbO\n\tqaCg==","X-Gm-Message-State":"AOAM5339b/FiF+r5BZO386F4BmWG/6Ns4Y5wb4n+RDIZHMFDAfSG5Z9+\n\tmdHLw2AnIP4Js3r3WkRxuCQjb+1rXEIbk02gHwiaaYaX","X-Google-Smtp-Source":"ABdhPJwjJKRG8WlP1m08VXUrsDxhVCRtix+QuH9jM8Afmjt1O0XeonwgcMmnyK9WYycxkvPbCVLjdNv7EJrDTMvFwKw=","X-Received":"by 2002:adf:e38a:: with SMTP id\n\te10mr21774038wrm.162.1635155803078; \n\tMon, 25 Oct 2021 02:56:43 -0700 (PDT)","MIME-Version":"1.0","References":"<20211020110825.12902-1-david.plowman@raspberrypi.com>\n\t<20211020110825.12902-7-david.plowman@raspberrypi.com>\n\t","In-Reply-To":"","From":"David Plowman ","Date":"Mon, 25 Oct 2021 10:56:32 +0100","Message-ID":"","To":"Naushir Patuck ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v3 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","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]