[{"id":32212,"web_url":"https://patchwork.libcamera.org/comment/32212/","msgid":"<87wmh0iz3u.fsf@redhat.com>","date":"2024-11-18T11:02:29","subject":"Re: [RFC PATCH v2 02/12] ipa: libipa: vector: Add r(), g() and b()\n\taccessors","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Hi Laurent,\n\nthank you for the patches.\n\nLaurent Pinchart <laurent.pinchart@ideasonboard.com> writes:\n\n> The Vector class can be useful to represent RGB pixel values. Add r(),\n> g() and b() accessors, similar to x(), y() and z(), along with an RGB\n> type that aliases Vector<T, 3>.\n\nGood thing to have, including RGB alias.\n\nIs this expected to have the same access time as with e.g.\n\n  struct RGB {\n    T r, g, b;\n  };\n\n?  This is important in places where each CPU cycle counts, like\nsoftware ISP debayering.\n\n> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n\nReviewed-by: Milan Zamazal <mzamazal@redhat.com>\n\n> ---\n>  src/ipa/libipa/vector.cpp | 38 ++++++++++++++++++++++++++++++++++++++\n>  src/ipa/libipa/vector.h   | 28 ++++++++++++++++++++++++++++\n>  2 files changed, 66 insertions(+)\n>\n> diff --git a/src/ipa/libipa/vector.cpp b/src/ipa/libipa/vector.cpp\n> index 8a39bfd27f90..f14f155216f3 100644\n> --- a/src/ipa/libipa/vector.cpp\n> +++ b/src/ipa/libipa/vector.cpp\n> @@ -126,6 +126,39 @@ namespace ipa {\n>   * \\copydoc Vector::z()\n>   */\n>  \n> +/**\n> + * \\fn T &Vector::r()\n> + * \\brief Convenience function to access the first element of the vector\n> + * \\return The first element of the vector\n> + */\n> +\n> +/**\n> + * \\fn T &Vector::g()\n> + * \\brief Convenience function to access the second element of the vector\n> + * \\return The second element of the vector\n> + */\n> +\n> +/**\n> + * \\fn T &Vector::b()\n> + * \\brief Convenience function to access the third element of the vector\n> + * \\return The third element of the vector\n> + */\n> +\n> +/**\n> + * \\fn constexpr const T &Vector::r() const\n> + * \\copydoc Vector::r()\n> + */\n> +\n> +/**\n> + * \\fn constexpr const T &Vector::g() const\n> + * \\copydoc Vector::g()\n> + */\n> +\n> +/**\n> + * \\fn constexpr const T &Vector::b() const\n> + * \\copydoc Vector::b()\n> + */\n> +\n>  /**\n>   * \\fn Vector::length2()\n>   * \\brief Get the squared length of the vector\n> @@ -149,6 +182,11 @@ namespace ipa {\n>   * \\return Product of matrix \\a m and vector \\a v\n>   */\n>  \n> +/**\n> + * \\typedef RGB\n> + * \\brief A Vector of 3 elements representing an RGB pixel value\n> + */\n> +\n>  /**\n>   * \\fn bool operator==(const Vector<T, Rows> &lhs, const Vector<T, Rows> &rhs)\n>   * \\brief Compare vectors for equality\n> diff --git a/src/ipa/libipa/vector.h b/src/ipa/libipa/vector.h\n> index 1b11a34deee4..b72ab9851aa3 100644\n> --- a/src/ipa/libipa/vector.h\n> +++ b/src/ipa/libipa/vector.h\n> @@ -126,6 +126,31 @@ public:\n>  #endif /* __DOXYGEN__ */\n>  \tT &z() { return data_[2]; }\n>  \n> +#ifndef __DOXYGEN__\n> +\ttemplate<bool Dependent = false, typename = std::enable_if_t<Dependent || Rows >= 1>>\n> +#endif /* __DOXYGEN__ */\n> +\tconstexpr const T &r() const { return data_[0]; }\n> +#ifndef __DOXYGEN__\n> +\ttemplate<bool Dependent = false, typename = std::enable_if_t<Dependent || Rows >= 2>>\n> +#endif /* __DOXYGEN__ */\n> +\tconstexpr const T &g() const { return data_[1]; }\n> +#ifndef __DOXYGEN__\n> +\ttemplate<bool Dependent = false, typename = std::enable_if_t<Dependent || Rows >= 3>>\n> +#endif /* __DOXYGEN__ */\n> +\tconstexpr const T &b() const { return data_[2]; }\n> +#ifndef __DOXYGEN__\n> +\ttemplate<bool Dependent = false, typename = std::enable_if_t<Dependent || Rows >= 1>>\n> +#endif /* __DOXYGEN__ */\n> +\tT &r() { return data_[0]; }\n> +#ifndef __DOXYGEN__\n> +\ttemplate<bool Dependent = false, typename = std::enable_if_t<Dependent || Rows >= 2>>\n> +#endif /* __DOXYGEN__ */\n> +\tT &g() { return data_[1]; }\n> +#ifndef __DOXYGEN__\n> +\ttemplate<bool Dependent = false, typename = std::enable_if_t<Dependent || Rows >= 3>>\n> +#endif /* __DOXYGEN__ */\n> +\tT &b() { return data_[2]; }\n> +\n>  \tconstexpr double length2() const\n>  \t{\n>  \t\tdouble ret = 0;\n> @@ -143,6 +168,9 @@ private:\n>  \tstd::array<T, Rows> data_;\n>  };\n>  \n> +template<typename T>\n> +using RGB = Vector<T, 3>;\n> +\n>  template<typename T, unsigned int Rows, unsigned int Cols>\n>  Vector<T, Rows> operator*(const Matrix<T, Rows, Cols> &m, const Vector<T, Cols> &v)\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 92DF6C32EA\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 18 Nov 2024 11:02:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D3BE6658AD;\n\tMon, 18 Nov 2024 12:02:38 +0100 (CET)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.129.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0C58260532\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 18 Nov 2024 12:02:36 +0100 (CET)","from mail-wm1-f71.google.com (mail-wm1-f71.google.com\n\t[209.85.128.71]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-351-Uj5pjmgaNPOXaOPmQjEJvg-1; Mon, 18 Nov 2024 06:02:34 -0500","by mail-wm1-f71.google.com with SMTP id\n\t5b1f17b1804b1-4314f023f55so27829275e9.2\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 18 Nov 2024 03:02:34 -0800 (PST)","from nuthatch ([2a00:102a:400a:489a:ffe9:aa41:63cd:fc2b])\n\tby smtp.gmail.com with ESMTPSA id\n\tffacd0b85a97d-3824aad691csm690398f8f.1.2024.11.18.03.02.30\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tMon, 18 Nov 2024 03:02:31 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"XkrIaxhO\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1731927755;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=YVJFW1Mo5J5XI5BfMGKIq1uLJo4YhEB/6G2i64ap52s=;\n\tb=XkrIaxhORAdTFB6UqZnJvbZyUozy2bnCkDixMKdJb1YxDJFq7tgkDxWJ5FB+jCYfv/VoPe\n\t/cEsM4EIVd2InrAmwpERL12KxfRxydZFtEh2+YD/jWDTK44JunmpanpHxiGFeSNnKGAMr+\n\tQQtlonWF61Fg/r5qoFsSG+RH9lgCyF4=","X-MC-Unique":"Uj5pjmgaNPOXaOPmQjEJvg-1","X-Mimecast-MFC-AGG-ID":"Uj5pjmgaNPOXaOPmQjEJvg","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1731927753; x=1732532553;\n\th=mime-version:user-agent:message-id:date:references:in-reply-to\n\t:subject:cc:to:from:x-gm-message-state:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=YVJFW1Mo5J5XI5BfMGKIq1uLJo4YhEB/6G2i64ap52s=;\n\tb=QuZN/DdcKaTS+MMRsdQCoa+xOElvTixc8tuiMIn9aUTWGgjaXvcvpSv0Fl3Ga2WiTA\n\t2Bddk+hLnpjXdsqtjAQt94XZybGyUh8j9skufwp3S96MxoSyrObkP9g7EvcnL0tNkaGV\n\tjuLSSptqYJudkoq1vP9XPW88cj4L8C9r26kS7EWa0OvwmIBLI1UC/ISZXVHXIdRyUvpw\n\tCiCefM6wQFxKQPtkACTGJcC4G6Ce+kK4THSEklfH8LQg54JOpR5z04/QzFUZJE8YV4Jh\n\tqVCswh3OFO2fLQDa36guH2JvOI9Enysd/QYpoUxiQASvM9doLuUN6OAkSUgs4i0VXGnE\n\tL3Sg==","X-Gm-Message-State":"AOJu0YwxTm4ZpbBkIBSIxyojFq2K3nA1IZ87Xs+Xt6/9tFWIjr/BK1y3\n\tBkk8KML2t/SEXQvUv4W6OdTfuHD5tyzYxR8CE+n1BOmFjZGhDVVgl2Cb1uaUA6ki1u0+dA6jKNZ\n\teJFHKyLf/l6fmsCguhh9CIsnL8Dnzk/Vm3utE3QItffQbYQeg7H0xrMVESYqlPVdBPAuhBDir+f\n\txURGoId5kaXIhrwqtDMZm6/CLi5EMOuv1KXAy/8VirzJzs3diorjqeiaQ=","X-Received":["by 2002:a05:600c:314c:b0:42c:c003:edd1 with SMTP id\n\t5b1f17b1804b1-432df728f88mr115415825e9.10.1731927752449; \n\tMon, 18 Nov 2024 03:02:32 -0800 (PST)","by 2002:a05:600c:314c:b0:42c:c003:edd1 with SMTP id\n\t5b1f17b1804b1-432df728f88mr115415445e9.10.1731927751986; \n\tMon, 18 Nov 2024 03:02:31 -0800 (PST)"],"X-Google-Smtp-Source":"AGHT+IH9maemnf+mvaJcbQwOneloW+VmL+egUiHrBUPV7Cg3OaouOKEKmEegvstDxJTBaPAJ3q7VYQ==","From":"Milan Zamazal <mzamazal@redhat.com>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [RFC PATCH v2 02/12] ipa: libipa: vector: Add r(), g() and b()\n\taccessors","In-Reply-To":"<20241118000738.18977-3-laurent.pinchart@ideasonboard.com>\n\t(Laurent Pinchart's message of \"Mon, 18 Nov 2024 02:07:28 +0200\")","References":"<20241118000738.18977-1-laurent.pinchart@ideasonboard.com>\n\t<20241118000738.18977-3-laurent.pinchart@ideasonboard.com>","Date":"Mon, 18 Nov 2024 12:02:29 +0100","Message-ID":"<87wmh0iz3u.fsf@redhat.com>","User-Agent":"Gnus/5.13 (Gnus v5.13)","MIME-Version":"1.0","X-Mimecast-Spam-Score":"0","X-Mimecast-MFC-PROC-ID":"bVLn5nbNHh2Q2Uiv2A1eNj-UlQDBOwowDe-f-RgEmoo_1731927753","X-Mimecast-Originator":"redhat.com","Content-Type":"text/plain","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>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":32215,"web_url":"https://patchwork.libcamera.org/comment/32215/","msgid":"<20241118112305.GM31681@pendragon.ideasonboard.com>","date":"2024-11-18T11:23:05","subject":"Re: [RFC PATCH v2 02/12] ipa: libipa: vector: Add r(), g() and b()\n\taccessors","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Milan,\n\nOn Mon, Nov 18, 2024 at 12:02:29PM +0100, Milan Zamazal wrote:\n> Hi Laurent,\n> \n> thank you for the patches.\n> \n> Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes:\n> \n> > The Vector class can be useful to represent RGB pixel values. Add r(),\n> > g() and b() accessors, similar to x(), y() and z(), along with an RGB\n> > type that aliases Vector<T, 3>.\n> \n> Good thing to have, including RGB alias.\n\nThere are a few things that bother me with the RGB alias. One is that\nwe don't have an equivalent alias for YUV. We could add that, but then\ndo we also need one for XYZ (see patch 12/12) ? And for other triplets\nthat represent pixels or pixel-related values, such as gains ?\n\nI've also considered making RGB<T> inherit from Vector<T, 3>, and only\ndefine the r(), g() and b() accessors in the RGB class. The proble with\nthat approach is that the operators return a Vector, so doing something\nlike\n\n\tRGB<double> pixel;\n\tRGB<double> gains;\n\n\tauto result = pixel * gains;\n\treturn result.r()\n\nwould fail to compile as result would be a Vector<double, 3> instance.\nThere could be ways around that but I'm not sure they're worth it.\n\n> Is this expected to have the same access time as with e.g.\n> \n>   struct RGB {\n>     T r, g, b;\n>   };\n> \n> ?  This is important in places where each CPU cycle counts, like\n> software ISP debayering.\n\nGiven that all the functions are inline, I believe the compiler should\nbe able to generate the same code. It should be measured though.\n\n> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> \n> Reviewed-by: Milan Zamazal <mzamazal@redhat.com>\n> \n> > ---\n> >  src/ipa/libipa/vector.cpp | 38 ++++++++++++++++++++++++++++++++++++++\n> >  src/ipa/libipa/vector.h   | 28 ++++++++++++++++++++++++++++\n> >  2 files changed, 66 insertions(+)\n> >\n> > diff --git a/src/ipa/libipa/vector.cpp b/src/ipa/libipa/vector.cpp\n> > index 8a39bfd27f90..f14f155216f3 100644\n> > --- a/src/ipa/libipa/vector.cpp\n> > +++ b/src/ipa/libipa/vector.cpp\n> > @@ -126,6 +126,39 @@ namespace ipa {\n> >   * \\copydoc Vector::z()\n> >   */\n> >  \n> > +/**\n> > + * \\fn T &Vector::r()\n> > + * \\brief Convenience function to access the first element of the vector\n> > + * \\return The first element of the vector\n> > + */\n> > +\n> > +/**\n> > + * \\fn T &Vector::g()\n> > + * \\brief Convenience function to access the second element of the vector\n> > + * \\return The second element of the vector\n> > + */\n> > +\n> > +/**\n> > + * \\fn T &Vector::b()\n> > + * \\brief Convenience function to access the third element of the vector\n> > + * \\return The third element of the vector\n> > + */\n> > +\n> > +/**\n> > + * \\fn constexpr const T &Vector::r() const\n> > + * \\copydoc Vector::r()\n> > + */\n> > +\n> > +/**\n> > + * \\fn constexpr const T &Vector::g() const\n> > + * \\copydoc Vector::g()\n> > + */\n> > +\n> > +/**\n> > + * \\fn constexpr const T &Vector::b() const\n> > + * \\copydoc Vector::b()\n> > + */\n> > +\n> >  /**\n> >   * \\fn Vector::length2()\n> >   * \\brief Get the squared length of the vector\n> > @@ -149,6 +182,11 @@ namespace ipa {\n> >   * \\return Product of matrix \\a m and vector \\a v\n> >   */\n> >  \n> > +/**\n> > + * \\typedef RGB\n> > + * \\brief A Vector of 3 elements representing an RGB pixel value\n> > + */\n> > +\n> >  /**\n> >   * \\fn bool operator==(const Vector<T, Rows> &lhs, const Vector<T, Rows> &rhs)\n> >   * \\brief Compare vectors for equality\n> > diff --git a/src/ipa/libipa/vector.h b/src/ipa/libipa/vector.h\n> > index 1b11a34deee4..b72ab9851aa3 100644\n> > --- a/src/ipa/libipa/vector.h\n> > +++ b/src/ipa/libipa/vector.h\n> > @@ -126,6 +126,31 @@ public:\n> >  #endif /* __DOXYGEN__ */\n> >  \tT &z() { return data_[2]; }\n> >  \n> > +#ifndef __DOXYGEN__\n> > +\ttemplate<bool Dependent = false, typename = std::enable_if_t<Dependent || Rows >= 1>>\n> > +#endif /* __DOXYGEN__ */\n> > +\tconstexpr const T &r() const { return data_[0]; }\n> > +#ifndef __DOXYGEN__\n> > +\ttemplate<bool Dependent = false, typename = std::enable_if_t<Dependent || Rows >= 2>>\n> > +#endif /* __DOXYGEN__ */\n> > +\tconstexpr const T &g() const { return data_[1]; }\n> > +#ifndef __DOXYGEN__\n> > +\ttemplate<bool Dependent = false, typename = std::enable_if_t<Dependent || Rows >= 3>>\n> > +#endif /* __DOXYGEN__ */\n> > +\tconstexpr const T &b() const { return data_[2]; }\n> > +#ifndef __DOXYGEN__\n> > +\ttemplate<bool Dependent = false, typename = std::enable_if_t<Dependent || Rows >= 1>>\n> > +#endif /* __DOXYGEN__ */\n> > +\tT &r() { return data_[0]; }\n> > +#ifndef __DOXYGEN__\n> > +\ttemplate<bool Dependent = false, typename = std::enable_if_t<Dependent || Rows >= 2>>\n> > +#endif /* __DOXYGEN__ */\n> > +\tT &g() { return data_[1]; }\n> > +#ifndef __DOXYGEN__\n> > +\ttemplate<bool Dependent = false, typename = std::enable_if_t<Dependent || Rows >= 3>>\n> > +#endif /* __DOXYGEN__ */\n> > +\tT &b() { return data_[2]; }\n> > +\n> >  \tconstexpr double length2() const\n> >  \t{\n> >  \t\tdouble ret = 0;\n> > @@ -143,6 +168,9 @@ private:\n> >  \tstd::array<T, Rows> data_;\n> >  };\n> >  \n> > +template<typename T>\n> > +using RGB = Vector<T, 3>;\n> > +\n> >  template<typename T, unsigned int Rows, unsigned int Cols>\n> >  Vector<T, Rows> operator*(const Matrix<T, Rows, Cols> &m, const Vector<T, Cols> &v)\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 D54B5C32DD\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 18 Nov 2024 11:23:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 88C44658C4;\n\tMon, 18 Nov 2024 12:23:15 +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 F3D9260532\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 18 Nov 2024 12:23:13 +0100 (CET)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 418EC5B3;\n\tMon, 18 Nov 2024 12:22:57 +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=\"QDwndvhy\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1731928977;\n\tbh=LAKmOexvU3oPfBv3dgixErw8TOBBV2b8tk5Lpx1wuMI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=QDwndvhy+GCb3d4XysqOwX2D/ZCR6Maj2jq5tnHiDvEPJBbKRS2n6wOTYZlNwDXDP\n\tMqF7jtDtDRdpOY/2FdEupN7TRCrgpOWGhMnQN4ccNsF3cYaVDYHkPNpd3xrGp9bH8I\n\toKal1s7kkvt1TDqnSlqt3xUUlVSInXuyKnJ1tZwo=","Date":"Mon, 18 Nov 2024 13:23:05 +0200","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Milan Zamazal <mzamazal@redhat.com>","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [RFC PATCH v2 02/12] ipa: libipa: vector: Add r(), g() and b()\n\taccessors","Message-ID":"<20241118112305.GM31681@pendragon.ideasonboard.com>","References":"<20241118000738.18977-1-laurent.pinchart@ideasonboard.com>\n\t<20241118000738.18977-3-laurent.pinchart@ideasonboard.com>\n\t<87wmh0iz3u.fsf@redhat.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<87wmh0iz3u.fsf@redhat.com>","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>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":32216,"web_url":"https://patchwork.libcamera.org/comment/32216/","msgid":"<87seroivrz.fsf@redhat.com>","date":"2024-11-18T12:14:24","subject":"Re: [RFC PATCH v2 02/12] ipa: libipa: vector: Add r(), g() and b()\n\taccessors","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes:\n\n> Hi Milan,\n>\n> On Mon, Nov 18, 2024 at 12:02:29PM +0100, Milan Zamazal wrote:\n>> Hi Laurent,\n>> \n>> thank you for the patches.\n>> \n>> Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes:\n>> \n>> > The Vector class can be useful to represent RGB pixel values. Add r(),\n>> > g() and b() accessors, similar to x(), y() and z(), along with an RGB\n>> > type that aliases Vector<T, 3>.\n>> \n>> Good thing to have, including RGB alias.\n>\n> There are a few things that bother me with the RGB alias. One is that\n> we don't have an equivalent alias for YUV. We could add that, but then\n> do we also need one for XYZ (see patch 12/12) ? And for other triplets\n> that represent pixels or pixel-related values, such as gains ?\n\nI'd say it should be approached reasonably.  RGB is such a widespread\nthing.  And I prefer\n\n  RGB<double> gains;\n\nover\n\n  /* Three items for red, green, blue. */\n  Vector<double, 3> gains;\n\nor\n\n  double redGain, greenGain, blueGain;\n\nFor YUV, maybe.  The other cases are probably not worth their own\naliases.\n\n> I've also considered making RGB<T> inherit from Vector<T, 3>, and only\n> define the r(), g() and b() accessors in the RGB class. The proble with\n> that approach is that the operators return a Vector, so doing something\n> like\n>\n> \tRGB<double> pixel;\n> \tRGB<double> gains;\n>\n> \tauto result = pixel * gains;\n> \treturn result.r()\n>\n> would fail to compile as result would be a Vector<double, 3> instance.\n> There could be ways around that but I'm not sure they're worth it.\n\nIt depends how much we care about type checking here (I mean\ne.g. mistakenly using RGB in place of YUV or so).\n\n>> Is this expected to have the same access time as with e.g.\n>> \n>>   struct RGB {\n>>     T r, g, b;\n>>   };\n>> \n>> ?  This is important in places where each CPU cycle counts, like\n>> software ISP debayering.\n>\n> Given that all the functions are inline, I believe the compiler should\n> be able to generate the same code. It should be measured though.\n>\n>> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n>> \n>> Reviewed-by: Milan Zamazal <mzamazal@redhat.com>\n>> \n>> > ---\n>> >  src/ipa/libipa/vector.cpp | 38 ++++++++++++++++++++++++++++++++++++++\n>> >  src/ipa/libipa/vector.h   | 28 ++++++++++++++++++++++++++++\n>> >  2 files changed, 66 insertions(+)\n>> >\n>> > diff --git a/src/ipa/libipa/vector.cpp b/src/ipa/libipa/vector.cpp\n>> > index 8a39bfd27f90..f14f155216f3 100644\n>> > --- a/src/ipa/libipa/vector.cpp\n>> > +++ b/src/ipa/libipa/vector.cpp\n>> > @@ -126,6 +126,39 @@ namespace ipa {\n>> >   * \\copydoc Vector::z()\n>> >   */\n>> >  \n>> > +/**\n>> > + * \\fn T &Vector::r()\n>> > + * \\brief Convenience function to access the first element of the vector\n>> > + * \\return The first element of the vector\n>> > + */\n>> > +\n>> > +/**\n>> > + * \\fn T &Vector::g()\n>> > + * \\brief Convenience function to access the second element of the vector\n>> > + * \\return The second element of the vector\n>> > + */\n>> > +\n>> > +/**\n>> > + * \\fn T &Vector::b()\n>> > + * \\brief Convenience function to access the third element of the vector\n>> > + * \\return The third element of the vector\n>> > + */\n>> > +\n>> > +/**\n>> > + * \\fn constexpr const T &Vector::r() const\n>> > + * \\copydoc Vector::r()\n>> > + */\n>> > +\n>> > +/**\n>> > + * \\fn constexpr const T &Vector::g() const\n>> > + * \\copydoc Vector::g()\n>> > + */\n>> > +\n>> > +/**\n>> > + * \\fn constexpr const T &Vector::b() const\n>> > + * \\copydoc Vector::b()\n>> > + */\n>> > +\n>> >  /**\n>> >   * \\fn Vector::length2()\n>> >   * \\brief Get the squared length of the vector\n>> > @@ -149,6 +182,11 @@ namespace ipa {\n>> >   * \\return Product of matrix \\a m and vector \\a v\n>> >   */\n>> >  \n>> > +/**\n>> > + * \\typedef RGB\n>> > + * \\brief A Vector of 3 elements representing an RGB pixel value\n>> > + */\n>> > +\n>> >  /**\n>> >   * \\fn bool operator==(const Vector<T, Rows> &lhs, const Vector<T, Rows> &rhs)\n>> >   * \\brief Compare vectors for equality\n>> > diff --git a/src/ipa/libipa/vector.h b/src/ipa/libipa/vector.h\n>> > index 1b11a34deee4..b72ab9851aa3 100644\n>> > --- a/src/ipa/libipa/vector.h\n>> > +++ b/src/ipa/libipa/vector.h\n>> > @@ -126,6 +126,31 @@ public:\n>> >  #endif /* __DOXYGEN__ */\n>> >  \tT &z() { return data_[2]; }\n>> >  \n>> > +#ifndef __DOXYGEN__\n>> > +\ttemplate<bool Dependent = false, typename = std::enable_if_t<Dependent || Rows >= 1>>\n>> > +#endif /* __DOXYGEN__ */\n>> > +\tconstexpr const T &r() const { return data_[0]; }\n>> > +#ifndef __DOXYGEN__\n>> > +\ttemplate<bool Dependent = false, typename = std::enable_if_t<Dependent || Rows >= 2>>\n>> > +#endif /* __DOXYGEN__ */\n>> > +\tconstexpr const T &g() const { return data_[1]; }\n>> > +#ifndef __DOXYGEN__\n>> > +\ttemplate<bool Dependent = false, typename = std::enable_if_t<Dependent || Rows >= 3>>\n>> > +#endif /* __DOXYGEN__ */\n>> > +\tconstexpr const T &b() const { return data_[2]; }\n>> > +#ifndef __DOXYGEN__\n>> > +\ttemplate<bool Dependent = false, typename = std::enable_if_t<Dependent || Rows >= 1>>\n>> > +#endif /* __DOXYGEN__ */\n>> > +\tT &r() { return data_[0]; }\n>> > +#ifndef __DOXYGEN__\n>> > +\ttemplate<bool Dependent = false, typename = std::enable_if_t<Dependent || Rows >= 2>>\n>> > +#endif /* __DOXYGEN__ */\n>> > +\tT &g() { return data_[1]; }\n>> > +#ifndef __DOXYGEN__\n>> > +\ttemplate<bool Dependent = false, typename = std::enable_if_t<Dependent || Rows >= 3>>\n>> > +#endif /* __DOXYGEN__ */\n>> > +\tT &b() { return data_[2]; }\n>> > +\n>> >  \tconstexpr double length2() const\n>> >  \t{\n>> >  \t\tdouble ret = 0;\n>> > @@ -143,6 +168,9 @@ private:\n>> >  \tstd::array<T, Rows> data_;\n>> >  };\n>> >  \n>> > +template<typename T>\n>> > +using RGB = Vector<T, 3>;\n>> > +\n>> >  template<typename T, unsigned int Rows, unsigned int Cols>\n>> >  Vector<T, Rows> operator*(const Matrix<T, Rows, Cols> &m, const Vector<T, Cols> &v)\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 029D3C32EA\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 18 Nov 2024 12:14:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 253CE658C4;\n\tMon, 18 Nov 2024 13:14:32 +0100 (CET)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.129.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CC68A60532\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 18 Nov 2024 13:14:30 +0100 (CET)","from mail-wr1-f69.google.com (mail-wr1-f69.google.com\n\t[209.85.221.69]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-677-k4sca054NGSrnGy21ghmYQ-1; Mon, 18 Nov 2024 07:14:28 -0500","by mail-wr1-f69.google.com with SMTP id\n\tffacd0b85a97d-3824b5c4e65so69757f8f.1\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 18 Nov 2024 04:14:28 -0800 (PST)","from nuthatch ([2a00:102a:400a:489a:ffe9:aa41:63cd:fc2b])\n\tby smtp.gmail.com with ESMTPSA id\n\tffacd0b85a97d-382327e8140sm7767726f8f.60.2024.11.18.04.14.25\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tMon, 18 Nov 2024 04:14:25 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"MpleO8T+\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1731932069;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=mLt2RSf2aYmMVkamGUjoaLj8qYhfSh3PleGputQAWp4=;\n\tb=MpleO8T+IAIKtvejwA0e9Gy8LONyYilXLJFxOcQjjotf1ktEK5bwqyC8RTyFI3ARTBywn7\n\tMkN1zwsarLlEGufz+R/fKtpiht7ViG+7wnlw3kDuEDLUz6ChdXkJWroCDz7/G9S54nVm1p\n\t3BfQXpNFEROlYwjOjTSWurxHj9eMjPA=","X-MC-Unique":"k4sca054NGSrnGy21ghmYQ-1","X-Mimecast-MFC-AGG-ID":"k4sca054NGSrnGy21ghmYQ","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1731932067; x=1732536867;\n\th=mime-version:user-agent:message-id:date:references:in-reply-to\n\t:subject:cc:to:from:x-gm-message-state:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=mLt2RSf2aYmMVkamGUjoaLj8qYhfSh3PleGputQAWp4=;\n\tb=CDVDrRxPaZ9NapYeIz7Edr6GG9MTVeRVadTB9DyhG6JpExkUvl2zC6HZ1Tdm2KmNlV\n\tS1ka6t1tOj0pF5Sipqf2FYIvizf5+AdyW0e7xJXVTmCa215Xgc2wnQaXHMXp9M+LDh41\n\tuEP9aTkNbw+xNPpb9YWfRBtUyW0khZmclYav9Tf3wxeccy0cbH1DDjXKjnnE3NzfyFLP\n\tYYP7UvDuIWzXlYAW2ceeElm/03GA9D9e44DnwqDuHMUG4g1Lu2cL2/YJBVmkSXhTSHYl\n\tG7F3k4ylxtlp6mueVvVhO6KYU9OFxbvzL0bFa1JSC9BptY5XnovMfC3dksRtH8J2+Vht\n\tPxnQ==","X-Gm-Message-State":"AOJu0YxpZoGfqr3XAPvKzTH3KS+7TX+Wh/EjPo/EPrbbCzuf2LzB1ZMH\n\tqBhBuKH5mTX74lTKeK7ZZuYRER+Yaj0DOk6O9g/aDM5gWQwRi+H6krQ2R2jP7Yo4HWtNdsHpJ38\n\tR4KSJld5mgaYE20v+loKBCosLu77JO5QH1c9Q6PjWIzgGzi+VAHWnnE50rCnXshf4tBLy1SxICB\n\tcCAigYQVlI/2JhXQWXFT87TM3KhotVa1XdA3gTc+qb/f0VqUCirFrPxA0=","X-Received":["by 2002:a5d:5889:0:b0:367:8e57:8 with SMTP id\n\tffacd0b85a97d-38214064639mr15063006f8f.19.1731932066853; \n\tMon, 18 Nov 2024 04:14:26 -0800 (PST)","by 2002:a5d:5889:0:b0:367:8e57:8 with SMTP id\n\tffacd0b85a97d-38214064639mr15062964f8f.19.1731932066228; \n\tMon, 18 Nov 2024 04:14:26 -0800 (PST)"],"X-Google-Smtp-Source":"AGHT+IHEBmFJm1LqiJ8b7qR/H0Kj+1Vm9CV1+htsWdxNbfpf7mqj2Mbc6D/9G20q46VhVfF+I/Md6Q==","From":"Milan Zamazal <mzamazal@redhat.com>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [RFC PATCH v2 02/12] ipa: libipa: vector: Add r(), g() and b()\n\taccessors","In-Reply-To":"<20241118112305.GM31681@pendragon.ideasonboard.com> (Laurent\n\tPinchart's message of \"Mon, 18 Nov 2024 13:23:05 +0200\")","References":"<20241118000738.18977-1-laurent.pinchart@ideasonboard.com>\n\t<20241118000738.18977-3-laurent.pinchart@ideasonboard.com>\n\t<87wmh0iz3u.fsf@redhat.com>\n\t<20241118112305.GM31681@pendragon.ideasonboard.com>","Date":"Mon, 18 Nov 2024 13:14:24 +0100","Message-ID":"<87seroivrz.fsf@redhat.com>","User-Agent":"Gnus/5.13 (Gnus v5.13)","MIME-Version":"1.0","X-Mimecast-Spam-Score":"0","X-Mimecast-MFC-PROC-ID":"5Ln0iAM6U14IUME8GkI6jc5J5EiFDFYQiQ--C9qzf6M_1731932067","X-Mimecast-Originator":"redhat.com","Content-Type":"text/plain","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>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]