[{"id":11246,"web_url":"https://patchwork.libcamera.org/comment/11246/","msgid":"<20200708145210.GC20298@pendragon.ideasonboard.com>","date":"2020-07-08T14:52:10","subject":"Re: [libcamera-devel] [PATCH v4 06/21] libcamera: PixelFormatInfo:\n\tAdd functions stride and frameSize","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Paul,\n\nThank you for the patch.\n\nOn Wed, Jul 08, 2020 at 10:44:02PM +0900, Paul Elder wrote:\n> Add member functions to PixelFormatInfo for calculating stride and frame\n> size. This will simplify existing code that calculates these things.\n> \n> Signed-off-by: Paul Elder \n> \n> ---\n> Changes in v4:\n> - add overloaded frameSize() that takes array of strides\n> - add optional parameter to stride() for byte alignment\n> \n> Changes in v3:\n> - rename functions to stride and frameSize, from bytesPerLine and\n> imageSize, respectively\n> \n> Changes in v2:\n> - make these functions const\n> - add documentation\n> - inline DIV_ROUND_UP\n> ---\n> include/libcamera/internal/formats.h | 6 +++\n> src/libcamera/formats.cpp | 73 ++++++++++++++++++++++++++++\n> 2 files changed, 79 insertions(+)\n> \n> diff --git a/include/libcamera/internal/formats.h b/include/libcamera/internal/formats.h\n> index 054be37..e347a46 100644\n> --- a/include/libcamera/internal/formats.h\n> +++ b/include/libcamera/internal/formats.h\n> @@ -52,6 +52,12 @@ public:\n> \tstatic const PixelFormatInfo &info(const PixelFormat &format);\n> \tstatic const PixelFormatInfo &info(const V4L2PixelFormat &format);\n> \n> +\tunsigned int stride(unsigned int width, unsigned int plane,\n> +\t\t\t unsigned int align = 0) const;\n> +\tunsigned int frameSize(const Size &size) const;\n> +\tunsigned int frameSize(const Size &size,\n> +\t\t\t const std::array &strides) const;\n> +\n> \t/* \\todo Add support for non-contiguous memory planes */\n> \tconst char *name;\n> \tPixelFormat format;\n> diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp\n> index 6d96055..c355e57 100644\n> --- a/src/libcamera/formats.cpp\n> +++ b/src/libcamera/formats.cpp\n> @@ -732,4 +732,77 @@ const PixelFormatInfo &PixelFormatInfo::info(const V4L2PixelFormat &format)\n> \treturn info->second;\n> }\n> \n> +/**\n> + * \\brief Compute the stride\n> + * \\param[in] width The width of the line, in pixels\n> + * \\param[in] plane The index of the plane whose stride is to be computed\n> + * \\param[in] align The number of bytes to align to (0 for default alignment)\n> + * \\return The number of bytes necessary to store a line, or 0 if the\n> + * PixelFormatInfo instance or the \\a plane is not valid\n> + */\n> +unsigned int PixelFormatInfo::stride(unsigned int width, unsigned int plane,\n> +\t\t\t\t unsigned int align) const\n> +{\n> +\tif (!isValid())\n> +\t\treturn 0;\n> +\n> +\tif (plane > planes.size() || !planes[plane].bytesPerGroup)\n> +\t\treturn 0;\n> +\n> +\t/* ceil(width / pixelsPerGroup) * bytesPerGroup */\n> +\tunsigned int stride = (width + pixelsPerGroup - 1) / pixelsPerGroup\n> +\t\t\t * planes[plane].bytesPerGroup;\n\n* should be under =\n\n> +\n> +\tif (!align)\n> +\t\treturn stride;\n\nI wonder if we should default align to 1 instead of 0 and drop this.\n\n> +\n> +\t/* ceil(stride / align) * align */\n> +\treturn (stride + align - 1) / align * align;\n> +}\n> +\n> +/**\n> + * \\brief Compute the bytes necessary to store the frame\n\ns/the bytes/the number of bytes/\ns/the frame/a frame/\n\nSame for the next function\n\n> + * \\param[in] size The size of the frame, in pixels\n> + * \\return The number of bytes necessary to store the frame, or 0 if the\n> + * PixelFormatInfo instance is not valid\n> + */\n> +unsigned int PixelFormatInfo::frameSize(const Size &size) const\n\nWould it be useful to add an align parameter to this function, that\nwould default to 0 (or 1 depending on the outcome of the discussion\nabove) ? That should cover most pipeline handlers, with the next\nfunction being for really odd cases.\n\n> +{\n> +\t/* stride * ceil(height / verticalSubSampling) */\n> +\tunsigned int sum = 0;\n> +\tfor (unsigned int i = 0; i < 3; i++) {\n> +\t\tunsigned int vertSubSample = planes[i].verticalSubSampling;\n> +\t\tif (!vertSubSample)\n> +\t\t\tcontinue;\n> +\t\tsum += stride(size.width, i)\n> +\t\t * ((size.height + vertSubSample - 1) / vertSubSample);\n> +\t}\n> +\n> +\treturn sum;\n> +}\n> +\n> +/**\n> + * \\brief Compute the bytes necessary to store the frame\n> + * \\param[in] size The size of the frame, in pixels\n> + * \\param[in] strides The strides to use for each plane\n\n *\n * This function is an overloaded version that takes custom strides for each\n * plane, to be used when the device has custom alignment constraints that\n * can't be described by just an alignment value.\n *\n\n?\n\nReviewed-by: Laurent Pinchart \n\n> + * \\return The number of bytes necessary to store the frame, or 0 if the\n> + * PixelFormatInfo instance is not valid\n> + */\n> +unsigned int\n> +PixelFormatInfo::frameSize(const Size &size,\n> +\t\t\t const std::array &strides) const\n> +{\n> +\t/* stride * ceil(height / verticalSubSampling) */\n> +\tunsigned int sum = 0;\n> +\tfor (unsigned int i = 0; i < 3; i++) {\n> +\t\tunsigned int vertSubSample = planes[i].verticalSubSampling;\n> +\t\tif (!vertSubSample)\n> +\t\t\tcontinue;\n> +\t\tsum += strides[i]\n> +\t\t * ((size.height + vertSubSample - 1) / vertSubSample);\n> +\t}\n> +\n> +\treturn sum;\n> +}\n> +\n> } /* namespace libcamera */","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 792EBBD790\n\tfor ;\n\tWed, 8 Jul 2020 14:52:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 03E386114B;\n\tWed, 8 Jul 2020 16:52:18 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 39E75610F4\n\tfor ;\n\tWed, 8 Jul 2020 16:52:17 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A630C51B;\n\tWed, 8 Jul 2020 16:52:16 +0200 (CEST)"],"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=\"TVHbDRk9\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1594219936;\n\tbh=qto5zYSU0vUrzV7bL9cEXFXH3yNxZqEY040xeKlqhIU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=TVHbDRk9KDL3MPDs8JEUECq7F8SPGC56X3eX0/8FgFkUclDO1CtM1KdiaF2IGFoHb\n\tIipkYSMp339jsMUgwtNxkWBLkHATClKrAdi/Q/oWur8/bwmM0P92WAnlqRIFCPBCtq\n\tSx1zrWkFnmAMX/DdMcQLuFOyWrE1OmUWzjNJgH5U=","Date":"Wed, 8 Jul 2020 17:52:10 +0300","From":"Laurent Pinchart ","To":"Paul Elder ","Message-ID":"<20200708145210.GC20298@pendragon.ideasonboard.com>","References":"<20200708134417.67747-1-paul.elder@ideasonboard.com>\n\t<20200708134417.67747-7-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200708134417.67747-7-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v4 06/21] libcamera: PixelFormatInfo:\n\tAdd functions stride and frameSize","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@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":11260,"web_url":"https://patchwork.libcamera.org/comment/11260/","msgid":"<20200708211045.tpmbiytzihp3epul@uno.localdomain>","date":"2020-07-08T21:10:45","subject":"Re: [libcamera-devel] [PATCH v4 06/21] libcamera: PixelFormatInfo:\n\tAdd functions stride and frameSize","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Paul,\n\nOn Wed, Jul 08, 2020 at 10:44:02PM +0900, Paul Elder wrote:\n> Add member functions to PixelFormatInfo for calculating stride and frame\n> size. This will simplify existing code that calculates these things.\n>\n> Signed-off-by: Paul Elder \n>\n> ---\n> Changes in v4:\n> - add overloaded frameSize() that takes array of strides\n> - add optional parameter to stride() for byte alignment\n>\n> Changes in v3:\n> - rename functions to stride and frameSize, from bytesPerLine and\n> imageSize, respectively\n>\n> Changes in v2:\n> - make these functions const\n> - add documentation\n> - inline DIV_ROUND_UP\n> ---\n> include/libcamera/internal/formats.h | 6 +++\n> src/libcamera/formats.cpp | 73 ++++++++++++++++++++++++++++\n> 2 files changed, 79 insertions(+)\n>\n> diff --git a/include/libcamera/internal/formats.h b/include/libcamera/internal/formats.h\n> index 054be37..e347a46 100644\n> --- a/include/libcamera/internal/formats.h\n> +++ b/include/libcamera/internal/formats.h\n> @@ -52,6 +52,12 @@ public:\n> \tstatic const PixelFormatInfo &info(const PixelFormat &format);\n> \tstatic const PixelFormatInfo &info(const V4L2PixelFormat &format);\n>\n> +\tunsigned int stride(unsigned int width, unsigned int plane,\n> +\t\t\t unsigned int align = 0) const;\n> +\tunsigned int frameSize(const Size &size) const;\n> +\tunsigned int frameSize(const Size &size,\n> +\t\t\t const std::array &strides) const;\n> +\n> \t/* \\todo Add support for non-contiguous memory planes */\n> \tconst char *name;\n> \tPixelFormat format;\n> diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp\n> index 6d96055..c355e57 100644\n> --- a/src/libcamera/formats.cpp\n> +++ b/src/libcamera/formats.cpp\n> @@ -732,4 +732,77 @@ const PixelFormatInfo &PixelFormatInfo::info(const V4L2PixelFormat &format)\n> \treturn info->second;\n> }\n>\n> +/**\n> + * \\brief Compute the stride\n> + * \\param[in] width The width of the line, in pixels\n> + * \\param[in] plane The index of the plane whose stride is to be computed\n\ns/is/has ? (if this wasn't intentional, your English is slightly\nbetter than mine so it might be correct :)\n\n> + * \\param[in] align The number of bytes to align to (0 for default alignment)\n\nI would then make this a default parameter\n\n> + * \\return The number of bytes necessary to store a line, or 0 if the\n\nHow is likely be used stride by applications ? Could it be used as\ndivisor ? silent divisios by 0 are nasty to debug, is it worth an Info\nmessage ? Alternatively you could return an int with an error code,\nbut I wonder how many would actually check that, so an error message\nmight be better.\n\n> + * PixelFormatInfo instance or the \\a plane is not valid\n> + */\n> +unsigned int PixelFormatInfo::stride(unsigned int width, unsigned int plane,\n> +\t\t\t\t unsigned int align) const\n ^ allignment\n> +{\n> +\tif (!isValid())\n> +\t\treturn 0;\n> +\n> +\tif (plane > planes.size() || !planes[plane].bytesPerGroup)\n> +\t\treturn 0;\n> +\n> +\t/* ceil(width / pixelsPerGroup) * bytesPerGroup */\n> +\tunsigned int stride = (width + pixelsPerGroup - 1) / pixelsPerGroup\n> +\t\t\t * planes[plane].bytesPerGroup;\n> +\n> +\tif (!align)\n> +\t\treturn stride;\n\nyes, maybe a default parameter is better, and maybe\n> +\n\t/* ceil(stride / align) * align */\n return !align ? stride\n : (stride + align - 1) / align * align;\n\n> +\treturn (stride + align - 1) / align * align;\n> +}\n> +\n> +/**\n> + * \\brief Compute the bytes necessary to store the frame\n> + * \\param[in] size The size of the frame, in pixels\n> + * \\return The number of bytes necessary to store the frame, or 0 if the\n> + * PixelFormatInfo instance is not valid\n> + */\n> +unsigned int PixelFormatInfo::frameSize(const Size &size) const\n> +{\n> +\t/* stride * ceil(height / verticalSubSampling) */\n> +\tunsigned int sum = 0;\n> +\tfor (unsigned int i = 0; i < 3; i++) {\n> +\t\tunsigned int vertSubSample = planes[i].verticalSubSampling;\n> +\t\tif (!vertSubSample)\n> +\t\t\tcontinue;\n> +\t\tsum += stride(size.width, i)\n> +\t\t * ((size.height + vertSubSample - 1) / vertSubSample);\n> +\t}\n> +\n> +\treturn sum;\n> +}\n> +\n> +/**\n> + * \\brief Compute the bytes necessary to store the frame\n> + * \\param[in] size The size of the frame, in pixels\n> + * \\param[in] strides The strides to use for each plane\n> + * \\return The number of bytes necessary to store the frame, or 0 if the\n> + * PixelFormatInfo instance is not valid\n> + */\n> +unsigned int\n> +PixelFormatInfo::frameSize(const Size &size,\n> +\t\t\t const std::array &strides) const\n ^ alignment (doesn't checkstyle complains?)\n\n> +{\n> +\t/* stride * ceil(height / verticalSubSampling) */\n> +\tunsigned int sum = 0;\n> +\tfor (unsigned int i = 0; i < 3; i++) {\n> +\t\tunsigned int vertSubSample = planes[i].verticalSubSampling;\n> +\t\tif (!vertSubSample)\n> +\t\t\tcontinue;\n> +\t\tsum += strides[i]\n> +\t\t * ((size.height + vertSubSample - 1) / vertSubSample);\n\nIf you make of the stride stride a parameter defaulted to and empty\nvector you could merge the two functions here. Just\n\n unsigned int s = ((size.height + vertSubSample - 1)/ vertSubSample);\n if (!strides.empty())\n s *= stride(size.width, i);\n else\n s *= strides[i];\n\n sum += s;\n\nor something similar\n\nThe rest of the function seems identical to me.\n\n> +\t}\n> +\n> +\treturn sum;\n> +}\n> +\n> } /* namespace libcamera */\n> --\n> 2.27.0\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","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 50C9DBD792\n\tfor ;\n\tWed, 8 Jul 2020 21:07:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E442461167;\n\tWed, 8 Jul 2020 23:07:13 +0200 (CEST)","from relay5-d.mail.gandi.net (relay5-d.mail.gandi.net\n\t[217.70.183.197])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5376B61163\n\tfor ;\n\tWed, 8 Jul 2020 23:07:12 +0200 (CEST)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay5-d.mail.gandi.net (Postfix) with ESMTPSA id 94EAE1C0004;\n\tWed, 8 Jul 2020 21:07:11 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Wed, 8 Jul 2020 23:10:45 +0200","From":"Jacopo Mondi ","To":"Paul Elder ","Message-ID":"<20200708211045.tpmbiytzihp3epul@uno.localdomain>","References":"<20200708134417.67747-1-paul.elder@ideasonboard.com>\n\t<20200708134417.67747-7-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200708134417.67747-7-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v4 06/21] libcamera: PixelFormatInfo:\n\tAdd functions stride and frameSize","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@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":11273,"web_url":"https://patchwork.libcamera.org/comment/11273/","msgid":"<20200709074024.GB66701@pyrite.rasen.tech>","date":"2020-07-09T07:40:24","subject":"Re: [libcamera-devel] [PATCH v4 06/21] libcamera: PixelFormatInfo:\n\tAdd functions stride and frameSize","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Laurent,\n\nThank you for the review.\n\nOn Wed, Jul 08, 2020 at 05:52:10PM +0300, Laurent Pinchart wrote:\n> Hi Paul,\n> \n> Thank you for the patch.\n> \n> On Wed, Jul 08, 2020 at 10:44:02PM +0900, Paul Elder wrote:\n> > Add member functions to PixelFormatInfo for calculating stride and frame\n> > size. This will simplify existing code that calculates these things.\n> > \n> > Signed-off-by: Paul Elder \n> > \n> > ---\n> > Changes in v4:\n> > - add overloaded frameSize() that takes array of strides\n> > - add optional parameter to stride() for byte alignment\n> > \n> > Changes in v3:\n> > - rename functions to stride and frameSize, from bytesPerLine and\n> > imageSize, respectively\n> > \n> > Changes in v2:\n> > - make these functions const\n> > - add documentation\n> > - inline DIV_ROUND_UP\n> > ---\n> > include/libcamera/internal/formats.h | 6 +++\n> > src/libcamera/formats.cpp | 73 ++++++++++++++++++++++++++++\n> > 2 files changed, 79 insertions(+)\n> > \n> > diff --git a/include/libcamera/internal/formats.h b/include/libcamera/internal/formats.h\n> > index 054be37..e347a46 100644\n> > --- a/include/libcamera/internal/formats.h\n> > +++ b/include/libcamera/internal/formats.h\n> > @@ -52,6 +52,12 @@ public:\n> > \tstatic const PixelFormatInfo &info(const PixelFormat &format);\n> > \tstatic const PixelFormatInfo &info(const V4L2PixelFormat &format);\n> > \n> > +\tunsigned int stride(unsigned int width, unsigned int plane,\n> > +\t\t\t unsigned int align = 0) const;\n> > +\tunsigned int frameSize(const Size &size) const;\n> > +\tunsigned int frameSize(const Size &size,\n> > +\t\t\t const std::array &strides) const;\n> > +\n> > \t/* \\todo Add support for non-contiguous memory planes */\n> > \tconst char *name;\n> > \tPixelFormat format;\n> > diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp\n> > index 6d96055..c355e57 100644\n> > --- a/src/libcamera/formats.cpp\n> > +++ b/src/libcamera/formats.cpp\n> > @@ -732,4 +732,77 @@ const PixelFormatInfo &PixelFormatInfo::info(const V4L2PixelFormat &format)\n> > \treturn info->second;\n> > }\n> > \n> > +/**\n> > + * \\brief Compute the stride\n> > + * \\param[in] width The width of the line, in pixels\n> > + * \\param[in] plane The index of the plane whose stride is to be computed\n> > + * \\param[in] align The number of bytes to align to (0 for default alignment)\n> > + * \\return The number of bytes necessary to store a line, or 0 if the\n> > + * PixelFormatInfo instance or the \\a plane is not valid\n> > + */\n> > +unsigned int PixelFormatInfo::stride(unsigned int width, unsigned int plane,\n> > +\t\t\t\t unsigned int align) const\n> > +{\n> > +\tif (!isValid())\n> > +\t\treturn 0;\n> > +\n> > +\tif (plane > planes.size() || !planes[plane].bytesPerGroup)\n> > +\t\treturn 0;\n> > +\n> > +\t/* ceil(width / pixelsPerGroup) * bytesPerGroup */\n> > +\tunsigned int stride = (width + pixelsPerGroup - 1) / pixelsPerGroup\n> > +\t\t\t * planes[plane].bytesPerGroup;\n> \n> * should be under =\n> \n> > +\n> > +\tif (!align)\n> > +\t\treturn stride;\n> \n> I wonder if we should default align to 1 instead of 0 and drop this.\n\nWell at first I thought that align to 1 bytes doesn't make sense, until\nI realized that stride must be a multiple of align, so 1 does actually\nmake sense, both semantically and mathematically. So yeah, I'll change\nit to 1.\n\n> > +\n> > +\t/* ceil(stride / align) * align */\n> > +\treturn (stride + align - 1) / align * align;\n> > +}\n> > +\n> > +/**\n> > + * \\brief Compute the bytes necessary to store the frame\n> \n> s/the bytes/the number of bytes/\n> s/the frame/a frame/\n> \n> Same for the next function\n> \n> > + * \\param[in] size The size of the frame, in pixels\n> > + * \\return The number of bytes necessary to store the frame, or 0 if the\n> > + * PixelFormatInfo instance is not valid\n> > + */\n> > +unsigned int PixelFormatInfo::frameSize(const Size &size) const\n> \n> Would it be useful to add an align parameter to this function, that\n> would default to 0 (or 1 depending on the outcome of the discussion\n> above) ? That should cover most pipeline handlers, with the next\n> function being for really odd cases.\n\nYes, I think it would be useful.\n\n> > +{\n> > +\t/* stride * ceil(height / verticalSubSampling) */\n> > +\tunsigned int sum = 0;\n> > +\tfor (unsigned int i = 0; i < 3; i++) {\n> > +\t\tunsigned int vertSubSample = planes[i].verticalSubSampling;\n> > +\t\tif (!vertSubSample)\n> > +\t\t\tcontinue;\n> > +\t\tsum += stride(size.width, i)\n> > +\t\t * ((size.height + vertSubSample - 1) / vertSubSample);\n> > +\t}\n> > +\n> > +\treturn sum;\n> > +}\n> > +\n> > +/**\n> > + * \\brief Compute the bytes necessary to store the frame\n> > + * \\param[in] size The size of the frame, in pixels\n> > + * \\param[in] strides The strides to use for each plane\n> \n> *\n> * This function is an overloaded version that takes custom strides for each\n> * plane, to be used when the device has custom alignment constraints that\n> * can't be described by just an alignment value.\n> *\n> \n> ?\n> \n> Reviewed-by: Laurent Pinchart \n> \n> > + * \\return The number of bytes necessary to store the frame, or 0 if the\n> > + * PixelFormatInfo instance is not valid\n> > + */\n> > +unsigned int\n> > +PixelFormatInfo::frameSize(const Size &size,\n> > +\t\t\t const std::array &strides) const\n> > +{\n> > +\t/* stride * ceil(height / verticalSubSampling) */\n> > +\tunsigned int sum = 0;\n> > +\tfor (unsigned int i = 0; i < 3; i++) {\n> > +\t\tunsigned int vertSubSample = planes[i].verticalSubSampling;\n> > +\t\tif (!vertSubSample)\n> > +\t\t\tcontinue;\n> > +\t\tsum += strides[i]\n> > +\t\t * ((size.height + vertSubSample - 1) / vertSubSample);\n> > +\t}\n> > +\n> > +\treturn sum;\n> > +}\n> > +\n> > } /* namespace libcamera */\n\n\nThanks,\n\nPaul","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 8CDDDBD792\n\tfor ;\n\tThu, 9 Jul 2020 07:40:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 06F8161179;\n\tThu, 9 Jul 2020 09:40:35 +0200 (CEST)","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 2479C61158\n\tfor ;\n\tThu, 9 Jul 2020 09:40:33 +0200 (CEST)","from pyrite.rasen.tech (unknown\n\t[IPv6:2400:4051:61:600:2c71:1b79:d06d:5032])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 79E52289;\n\tThu, 9 Jul 2020 09:40:31 +0200 (CEST)"],"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=\"d6do8Q5n\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1594280432;\n\tbh=ApIIUaK//NYNQzHCaoKWCQQ01uaoiU5mrEO+exUSK2M=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=d6do8Q5nj6TKc80KTXX1p7Kp3TuLRhNenCj7k5KFEL+hx2htSDF1M3xicVhHTrSpX\n\tg1n71io1vWMcjXEtWEG++RMW+RHRa47uzQs+8Q0e2y8YkAb+9oAX51+B7I0XgYPAmU\n\tzJdiI5+ud4cUsu4j7vQgXLXkQ6CdzjAKMMlOET1U=","Date":"Thu, 9 Jul 2020 16:40:24 +0900","From":"paul.elder@ideasonboard.com","To":"Laurent Pinchart ","Message-ID":"<20200709074024.GB66701@pyrite.rasen.tech>","References":"<20200708134417.67747-1-paul.elder@ideasonboard.com>\n\t<20200708134417.67747-7-paul.elder@ideasonboard.com>\n\t<20200708145210.GC20298@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200708145210.GC20298@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v4 06/21] libcamera: PixelFormatInfo:\n\tAdd functions stride and frameSize","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@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":11274,"web_url":"https://patchwork.libcamera.org/comment/11274/","msgid":"<20200709074845.GC66701@pyrite.rasen.tech>","date":"2020-07-09T07:48:45","subject":"Re: [libcamera-devel] [PATCH v4 06/21] libcamera: PixelFormatInfo:\n\tAdd functions stride and frameSize","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the review.\n\nOn Wed, Jul 08, 2020 at 11:10:45PM +0200, Jacopo Mondi wrote:\n> Hi Paul,\n> \n> On Wed, Jul 08, 2020 at 10:44:02PM +0900, Paul Elder wrote:\n> > Add member functions to PixelFormatInfo for calculating stride and frame\n> > size. This will simplify existing code that calculates these things.\n> >\n> > Signed-off-by: Paul Elder \n> >\n> > ---\n> > Changes in v4:\n> > - add overloaded frameSize() that takes array of strides\n> > - add optional parameter to stride() for byte alignment\n> >\n> > Changes in v3:\n> > - rename functions to stride and frameSize, from bytesPerLine and\n> > imageSize, respectively\n> >\n> > Changes in v2:\n> > - make these functions const\n> > - add documentation\n> > - inline DIV_ROUND_UP\n> > ---\n> > include/libcamera/internal/formats.h | 6 +++\n> > src/libcamera/formats.cpp | 73 ++++++++++++++++++++++++++++\n> > 2 files changed, 79 insertions(+)\n> >\n> > diff --git a/include/libcamera/internal/formats.h b/include/libcamera/internal/formats.h\n> > index 054be37..e347a46 100644\n> > --- a/include/libcamera/internal/formats.h\n> > +++ b/include/libcamera/internal/formats.h\n> > @@ -52,6 +52,12 @@ public:\n> > \tstatic const PixelFormatInfo &info(const PixelFormat &format);\n> > \tstatic const PixelFormatInfo &info(const V4L2PixelFormat &format);\n> >\n> > +\tunsigned int stride(unsigned int width, unsigned int plane,\n> > +\t\t\t unsigned int align = 0) const;\n> > +\tunsigned int frameSize(const Size &size) const;\n> > +\tunsigned int frameSize(const Size &size,\n> > +\t\t\t const std::array &strides) const;\n> > +\n> > \t/* \\todo Add support for non-contiguous memory planes */\n> > \tconst char *name;\n> > \tPixelFormat format;\n> > diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp\n> > index 6d96055..c355e57 100644\n> > --- a/src/libcamera/formats.cpp\n> > +++ b/src/libcamera/formats.cpp\n> > @@ -732,4 +732,77 @@ const PixelFormatInfo &PixelFormatInfo::info(const V4L2PixelFormat &format)\n> > \treturn info->second;\n> > }\n> >\n> > +/**\n> > + * \\brief Compute the stride\n> > + * \\param[in] width The width of the line, in pixels\n> > + * \\param[in] plane The index of the plane whose stride is to be computed\n> \n> s/is/has ? (if this wasn't intentional, your English is slightly\n> better than mine so it might be correct :)\n\nThis is fine as-is :) We /at the moment/ want to compute the stride of a\nplane, so we can refer to the stride in present tense.\n\n> > + * \\param[in] align The number of bytes to align to (0 for default alignment)\n> \n> I would then make this a default parameter\n\nIt is, in the header.\n\n> > + * \\return The number of bytes necessary to store a line, or 0 if the\n> \n> How is likely be used stride by applications ? Could it be used as\n> divisor ? silent divisios by 0 are nasty to debug, is it worth an Info\n> message ? Alternatively you could return an int with an error code,\n> but I wonder how many would actually check that, so an error message\n> might be better.\n\nYeah, that's true. I'll add an Info message at least, then. We return\nunsigned int for stride (as does struct v4l2_format.fmt.pix.bytesperline),\nso we can't really return negative error code.\n\n> > + * PixelFormatInfo instance or the \\a plane is not valid\n> > + */\n> > +unsigned int PixelFormatInfo::stride(unsigned int width, unsigned int plane,\n> > +\t\t\t\t unsigned int align) const\n> ^ allignment\n\nThis, and the other alignments that you point out, are fine. Maybe\nsomething happened in the mail client :/\n\n> > +{\n> > +\tif (!isValid())\n> > +\t\treturn 0;\n> > +\n> > +\tif (plane > planes.size() || !planes[plane].bytesPerGroup)\n> > +\t\treturn 0;\n> > +\n> > +\t/* ceil(width / pixelsPerGroup) * bytesPerGroup */\n> > +\tunsigned int stride = (width + pixelsPerGroup - 1) / pixelsPerGroup\n> > +\t\t\t * planes[plane].bytesPerGroup;\n> > +\n> > +\tif (!align)\n> > +\t\treturn stride;\n> \n> yes, maybe a default parameter is better, and maybe\n> > +\n> \t/* ceil(stride / align) * align */\n> return !align ? stride\n> : (stride + align - 1) / align * align;\n\nAfter making the default alignment 1 instead of 0, the branch won't be\nnecessary anymore.\n\n> > +\treturn (stride + align - 1) / align * align;\n> > +}\n> > +\n> > +/**\n> > + * \\brief Compute the bytes necessary to store the frame\n> > + * \\param[in] size The size of the frame, in pixels\n> > + * \\return The number of bytes necessary to store the frame, or 0 if the\n> > + * PixelFormatInfo instance is not valid\n> > + */\n> > +unsigned int PixelFormatInfo::frameSize(const Size &size) const\n> > +{\n> > +\t/* stride * ceil(height / verticalSubSampling) */\n> > +\tunsigned int sum = 0;\n> > +\tfor (unsigned int i = 0; i < 3; i++) {\n> > +\t\tunsigned int vertSubSample = planes[i].verticalSubSampling;\n> > +\t\tif (!vertSubSample)\n> > +\t\t\tcontinue;\n> > +\t\tsum += stride(size.width, i)\n> > +\t\t * ((size.height + vertSubSample - 1) / vertSubSample);\n> > +\t}\n> > +\n> > +\treturn sum;\n> > +}\n> > +\n> > +/**\n> > + * \\brief Compute the bytes necessary to store the frame\n> > + * \\param[in] size The size of the frame, in pixels\n> > + * \\param[in] strides The strides to use for each plane\n> > + * \\return The number of bytes necessary to store the frame, or 0 if the\n> > + * PixelFormatInfo instance is not valid\n> > + */\n> > +unsigned int\n> > +PixelFormatInfo::frameSize(const Size &size,\n> > +\t\t\t const std::array &strides) const\n> ^ alignment (doesn't checkstyle complains?)\n> \n> > +{\n> > +\t/* stride * ceil(height / verticalSubSampling) */\n> > +\tunsigned int sum = 0;\n> > +\tfor (unsigned int i = 0; i < 3; i++) {\n> > +\t\tunsigned int vertSubSample = planes[i].verticalSubSampling;\n> > +\t\tif (!vertSubSample)\n> > +\t\t\tcontinue;\n> > +\t\tsum += strides[i]\n> > +\t\t * ((size.height + vertSubSample - 1) / vertSubSample);\n> \n> If you make of the stride stride a parameter defaulted to and empty\n> vector you could merge the two functions here. Just\n> \n> unsigned int s = ((size.height + vertSubSample - 1)/ vertSubSample);\n> if (!strides.empty())\n> s *= stride(size.width, i);\n> else\n> s *= strides[i];\n> \n> sum += s;\n> \n> or something similar\n\nOh yeah, that's better.\n\n> The rest of the function seems identical to me.\n\nIt is :)\n\n> > +\t}\n> > +\n> > +\treturn sum;\n> > +}\n> > +\n> > } /* namespace libcamera */\n> > --\n> > 2.27.0\n\n\nThanks,\n\nPaul","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 ADB4ABD790\n\tfor ;\n\tThu, 9 Jul 2020 07:48:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2407261179;\n\tThu, 9 Jul 2020 09:48:55 +0200 (CEST)","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 19F9361158\n\tfor ;\n\tThu, 9 Jul 2020 09:48:54 +0200 (CEST)","from pyrite.rasen.tech (unknown\n\t[IPv6:2400:4051:61:600:2c71:1b79:d06d:5032])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 70490289;\n\tThu, 9 Jul 2020 09:48:52 +0200 (CEST)"],"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=\"MTYZH7NX\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1594280933;\n\tbh=Z/aOa3dOQpc/xlpmLrRIuTXPZnfvXRqXd2D2FVwoIF0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=MTYZH7NXnb7NNml06b7CuILrymKxRifWZzl8eJZg88LKqTVLpkXwpZ/9JQcCtMHAi\n\tuUO46RdW1Tcw3VPPwx3+6NrHV7TqCzzlL6fE3XBzi1R+JxhkNEc2CLOGxPiOoAGK5S\n\tOMWHUZCkiJ2V94MaXCgQZBDoLZyMhmpDaT5ZOb3g=","Date":"Thu, 9 Jul 2020 16:48:45 +0900","From":"paul.elder@ideasonboard.com","To":"Jacopo Mondi ","Message-ID":"<20200709074845.GC66701@pyrite.rasen.tech>","References":"<20200708134417.67747-1-paul.elder@ideasonboard.com>\n\t<20200708134417.67747-7-paul.elder@ideasonboard.com>\n\t<20200708211045.tpmbiytzihp3epul@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200708211045.tpmbiytzihp3epul@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v4 06/21] libcamera: PixelFormatInfo:\n\tAdd functions stride and frameSize","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@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]