[{"id":11036,"web_url":"https://patchwork.libcamera.org/comment/11036/","msgid":"<20200701104827.GQ5963@pendragon.ideasonboard.com>","date":"2020-07-01T10:48:27","subject":"Re: [libcamera-devel] [PATCH v2 4/6] libcamera: PixelFormatInfo:\n\tAdd functions bytesPerLine and imageSize","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 Tue, Jun 30, 2020 at 11:58:06PM +0900, Paul Elder wrote:\n> Add member functions to PixelFormatInfo for calculating bytes-per-line\n> and image size. This will simplify existing code that calculates these\n> things.\n> \n> Signed-off-by: Paul Elder \n> \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 | 3 +++\n> src/libcamera/formats.cpp | 27 +++++++++++++++++++++++++++\n> 2 files changed, 30 insertions(+)\n> \n> diff --git a/include/libcamera/internal/formats.h b/include/libcamera/internal/formats.h\n> index 3c7440a..af0f1ac 100644\n> --- a/include/libcamera/internal/formats.h\n> +++ b/include/libcamera/internal/formats.h\n> @@ -46,6 +46,9 @@ public:\n> \tstatic const PixelFormatInfo &info(const PixelFormat &format);\n> \tstatic const PixelFormatInfo &info(const V4L2PixelFormat &format);\n> \n> +\tunsigned int bytesPerLine(unsigned int width) const;\n> +\tunsigned int imageSize(const Size &size) const;\n> +\n> \tconst char *name;\n> \tPixelFormat format;\n> \tV4L2PixelFormat v4l2Format;\n> diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp\n> index 4d18825..698c905 100644\n> --- a/src/libcamera/formats.cpp\n> +++ b/src/libcamera/formats.cpp\n> @@ -693,4 +693,31 @@ const PixelFormatInfo &PixelFormatInfo::info(const V4L2PixelFormat &format)\n> \treturn info->second;\n> }\n> \n> +/**\n> + * \\brief Compute the bytes per line\n> + * \\param[in] width The width of the line, in pixels\n> + * \\return The number of bytes necessary to store the line, or 0 if the\n> + * PixelFormatInfo instance not valid\n> + */\n> +unsigned int PixelFormatInfo::bytesPerLine(unsigned int width) const\n\nI think the name (and documentation) should be updated based on the\nnaming discussions in 2/6 v1 (stride or lineStride ?).\n\n> +{\n> +\tif (!isValid())\n> +\t\treturn 0;\n> +\n> +\t/* ceil(width / pixelsPerGroup) * bytesPerGroup */\n> +\treturn ((width + pixelsPerGroup - 1) / pixelsPerGroup) * bytesPerGroup;\n\nYou could remove the outer parentheses.\n\n> +\n\nExtra blank line.\n\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::imageSize(const Size &size) const\n> +{\n> +\treturn size.height * bytesPerLine(size.width);\n> +}\n\nThis one is a bit of an issue. The calculation is fine assuming the\ndevice doesn't have particular requirements on the stride alignment.\nHowever, if you have a device that requires the stride to be aligned on\na multiple of 32 bytes, the bytes per line value will be aligned by the\npipeline handler manually after calling PixelFormatInfo::bytesPerLine(),\nbut this won't be taken into account in PixelFormatInfo::imageSize(). I\nwould either pass the stride and height instead of the size to\nPixelFormatInfo::imageSize(), or drop the function altogether as all it\ndoes is a multiplication that doesn't depend on the PixelFormatInfo and\nthat can be easily done in the callers.\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 6A389BFFE2\n\tfor ;\n\tWed, 1 Jul 2020 10:48:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EAD4160C56;\n\tWed, 1 Jul 2020 12:48:32 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id AF48B603B4\n\tfor ;\n\tWed, 1 Jul 2020 12:48:31 +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 20B5A556;\n\tWed, 1 Jul 2020 12:48: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=\"gs6GbHkW\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1593600511;\n\tbh=urwOFktZ5G8MHUlalhpRy9k2pqDyp/wQQJGGRnK+7/M=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=gs6GbHkWimm6KVIUAy4A8c7zZrV3uRDwQCCeogaPUbwyeqguG8J/XYPrJLeVOS0zf\n\t6+vXIAFuhzKzhXAGyEWRCcnbJacZrTOQVHCgl62HrB2lCbOC4TmUiyj/uGaFsb9uGM\n\tXoFHUX5CR5s2uHYCAxwrCFRaeDgHrHbff9rHa3R8=","Date":"Wed, 1 Jul 2020 13:48:27 +0300","From":"Laurent Pinchart ","To":"Paul Elder ","Message-ID":"<20200701104827.GQ5963@pendragon.ideasonboard.com>","References":"<20200630145808.2976956-1-paul.elder@ideasonboard.com>\n\t<20200630145808.2976956-5-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200630145808.2976956-5-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 4/6] libcamera: PixelFormatInfo:\n\tAdd functions bytesPerLine and imageSize","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":11037,"web_url":"https://patchwork.libcamera.org/comment/11037/","msgid":"<20200701105852.GA23039@pendragon.ideasonboard.com>","date":"2020-07-01T10:58:52","subject":"Re: [libcamera-devel] [PATCH v2 4/6] libcamera: PixelFormatInfo:\n\tAdd functions bytesPerLine and imageSize","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi again,\n\nOn Wed, Jul 01, 2020 at 01:48:28PM +0300, Laurent Pinchart wrote:\n> On Tue, Jun 30, 2020 at 11:58:06PM +0900, Paul Elder wrote:\n> > Add member functions to PixelFormatInfo for calculating bytes-per-line\n> > and image size. This will simplify existing code that calculates these\n> > things.\n> > \n> > Signed-off-by: Paul Elder \n> > \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 | 3 +++\n> > src/libcamera/formats.cpp | 27 +++++++++++++++++++++++++++\n> > 2 files changed, 30 insertions(+)\n> > \n> > diff --git a/include/libcamera/internal/formats.h b/include/libcamera/internal/formats.h\n> > index 3c7440a..af0f1ac 100644\n> > --- a/include/libcamera/internal/formats.h\n> > +++ b/include/libcamera/internal/formats.h\n> > @@ -46,6 +46,9 @@ public:\n> > \tstatic const PixelFormatInfo &info(const PixelFormat &format);\n> > \tstatic const PixelFormatInfo &info(const V4L2PixelFormat &format);\n> > \n> > +\tunsigned int bytesPerLine(unsigned int width) const;\n> > +\tunsigned int imageSize(const Size &size) const;\n> > +\n> > \tconst char *name;\n> > \tPixelFormat format;\n> > \tV4L2PixelFormat v4l2Format;\n> > diff --git a/src/libcamera/formats.cpp b/src/libcamera/formats.cpp\n> > index 4d18825..698c905 100644\n> > --- a/src/libcamera/formats.cpp\n> > +++ b/src/libcamera/formats.cpp\n> > @@ -693,4 +693,31 @@ const PixelFormatInfo &PixelFormatInfo::info(const V4L2PixelFormat &format)\n> > \treturn info->second;\n> > }\n> > \n> > +/**\n> > + * \\brief Compute the bytes per line\n> > + * \\param[in] width The width of the line, in pixels\n> > + * \\return The number of bytes necessary to store the line, or 0 if the\n> > + * PixelFormatInfo instance not valid\n> > + */\n> > +unsigned int PixelFormatInfo::bytesPerLine(unsigned int width) const\n> \n> I think the name (and documentation) should be updated based on the\n> naming discussions in 2/6 v1 (stride or lineStride ?).\n> \n> > +{\n> > +\tif (!isValid())\n> > +\t\treturn 0;\n> > +\n> > +\t/* ceil(width / pixelsPerGroup) * bytesPerGroup */\n> > +\treturn ((width + pixelsPerGroup - 1) / pixelsPerGroup) * bytesPerGroup;\n> \n> You could remove the outer parentheses.\n> \n> > +\n> \n> Extra blank line.\n> \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::imageSize(const Size &size) const\n> > +{\n> > +\treturn size.height * bytesPerLine(size.width);\n> > +}\n> \n> This one is a bit of an issue. The calculation is fine assuming the\n> device doesn't have particular requirements on the stride alignment.\n> However, if you have a device that requires the stride to be aligned on\n> a multiple of 32 bytes, the bytes per line value will be aligned by the\n> pipeline handler manually after calling PixelFormatInfo::bytesPerLine(),\n> but this won't be taken into account in PixelFormatInfo::imageSize(). I\n> would either pass the stride and height instead of the size to\n> PixelFormatInfo::imageSize(), or drop the function altogether as all it\n> does is a multiplication that doesn't depend on the PixelFormatInfo and\n> that can be easily done in the callers.\n\nActually I think we should keep the function, but we need to make it\nmore complicated. This won't work well for multi-planar formats, as\nbytesPerLine() returns the stride for the first plane only. You need to\ntake the number of planes and the vertical subsampling into account.\nThen there will be the question of whether we can assume that the\nalignment restrictions of the hardware will be the same for all planes.\nI wonder if the function should take an array of strides, with one value\nper plane. We need to think a bit about how a good API would look like.\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 75BFDBF415\n\tfor ;\n\tWed, 1 Jul 2020 10:58:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0C1E8609A9;\n\tWed, 1 Jul 2020 12:58:58 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C91D6603B4\n\tfor ;\n\tWed, 1 Jul 2020 12:58:56 +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 49D63556;\n\tWed, 1 Jul 2020 12:58:56 +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=\"LUgUPo/N\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1593601136;\n\tbh=N8JTp6BaoCxUV8CnC7nFoF18wNkgIDtOC9BJAIfASaw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=LUgUPo/NFohN0yPEb8hk02kE9dtUdHP8C2kikBYH+i7RsECK03q7lrxCZMwNaEeHL\n\trpGlqI/if8cjLR/2+7P8CCoHX9nvNR0IgYw2mhhcHQbd9MaW8zblV+x1T4V53afCv7\n\t62WdYNRkZt4HP5A3RFWgwZXKOif18LStVpUz12Kk=","Date":"Wed, 1 Jul 2020 13:58:52 +0300","From":"Laurent Pinchart ","To":"Paul Elder ","Message-ID":"<20200701105852.GA23039@pendragon.ideasonboard.com>","References":"<20200630145808.2976956-1-paul.elder@ideasonboard.com>\n\t<20200630145808.2976956-5-paul.elder@ideasonboard.com>\n\t<20200701104827.GQ5963@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200701104827.GQ5963@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 4/6] libcamera: PixelFormatInfo:\n\tAdd functions bytesPerLine and imageSize","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\" "}}]