[{"id":4252,"web_url":"https://patchwork.libcamera.org/comment/4252/","msgid":"","date":"2020-03-24T08:44:47","subject":"Re: [libcamera-devel] [PATCH v2.2 21/21] qcam: viewfinder: Add\n\tsupport for more native formats","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 23/03/2020 23:28, Laurent Pinchart wrote:\n> Qt supports more 24-bit and 32-bit RGB formats for native painting. If\n> the frame buffer pixel format matches any of them, disable the converter\n> and create a QImage in the appropriate format.\n> \n> Signed-off-by: Laurent Pinchart \n> Reviewed-by: Kieran Bingham \n> ---\n> Changes since v2:\n> \n> - Add more formats\n> - Remove unnecessary parentheses\n> \n> Changes since v1:\n> \n> - Rename formats to nativeFormats\n> ---\n> src/qcam/viewfinder.cpp | 25 ++++++++++++++++++++++---\n> 1 file changed, 22 insertions(+), 3 deletions(-)\n> \n> diff --git a/src/qcam/viewfinder.cpp b/src/qcam/viewfinder.cpp\n> index 45e226b58135..7e7407f56e4c 100644\n> --- a/src/qcam/viewfinder.cpp\n> +++ b/src/qcam/viewfinder.cpp\n> @@ -7,16 +7,34 @@\n> \n> #include \"viewfinder.h\"\n> \n> +#include \n> #include \n> \n> #include \n> #include \n> +#include \n> #include \n> #include \n> #include \n> \n> #include \"format_converter.h\"\n> \n> +static const QMap nativeFormats\n> +{\n> +#if QT_VERSION >= QT_VERSION_CHECK(5, 2, 0)\n> +#if QBYTE_ORDER == Q_LITTLE_ENDIAN\n> +\t{ DRM_FORMAT_ABGR8888, QImage::Format_RGBA8888 },\n> +#else\n> +\t{ DRM_FORMAT_RGBA8888, QImage::Format_RGBA8888 },\n> +#endif\n\nThere's no else attributed to the QT_VERSION_CHECK, so we will not\nsupport QImage::Format_RGBA8888 at all on version < 5.2.0...\n\nPresumably that's not an issue.\n\n> +#endif\n\n\nCan we explain (in a comment perhaps) why we need to handle byte order\nfor QImage::Format_RGBA8888, but not QImage::Format_RGB32 ?\n\n\n> +\t{ DRM_FORMAT_ARGB8888, QImage::Format_RGB32 },\n\n\n> +#if QT_VERSION >= QT_VERSION_CHECK(5, 14, 0)\n> +\t{ DRM_FORMAT_BGR888, QImage::Format_BGR888 },\n> +#endif\n> +\t{ DRM_FORMAT_RGB888, QImage::Format_RGB888 },\n> +};\n> +\n> ViewFinder::ViewFinder(QWidget *parent)\n> \t: QWidget(parent), buffer_(nullptr)\n> {\n> @@ -36,7 +54,7 @@ int ViewFinder::setFormat(const libcamera::PixelFormat &format,\n> \t * If format conversion is needed, configure the converter and allocate\n> \t * the destination image.\n> \t */\n> -\tif (format != DRM_FORMAT_ARGB8888) {\n> +\tif (!nativeFormats.contains(format)) {\n> \t\tint ret = converter_.configure(format, size);\n> \t\tif (ret < 0)\n> \t\t\treturn ret;\n> @@ -64,7 +82,7 @@ void ViewFinder::render(libcamera::FrameBuffer *buffer, MappedBuffer *map)\n> \t{\n> \t\tQMutexLocker locker(&mutex_);\n> \n> -\t\tif (format_ == DRM_FORMAT_ARGB8888) {\n> +\t\tif (nativeFormats.contains(format_)) {\n> \t\t\t/*\n> \t\t\t * If the frame format is identical to the display\n> \t\t\t * format, create a QImage that references the frame\n> @@ -76,7 +94,8 @@ void ViewFinder::render(libcamera::FrameBuffer *buffer, MappedBuffer *map)\n> \t\t\t * computing it naively\n> \t\t\t */\n> \t\t\timage_ = QImage(memory, size_.width(), size_.height(),\n> -\t\t\t\t\tsize / size_.height(), QImage::Format_RGB32);\n> +\t\t\t\t\tsize / size_.height(),\n> +\t\t\t\t\tnativeFormats[format_]);\n> \t\t\tstd::swap(buffer, buffer_);\n> \t\t} else {\n> \t\t\t/*\n>","headers":{"Return-Path":"","Received":["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 9AD2860411\n\tfor ;\n\tTue, 24 Mar 2020 09:44:51 +0100 (CET)","from [192.168.0.20]\n\t(cpc89242-aztw30-2-0-cust488.18-1.cable.virginm.net [86.31.129.233])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 03E64308;\n\tTue, 24 Mar 2020 09:44:50 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1585039491;\n\tbh=buM5A7pterU39dQIwB9KrfxXovZIju/L76aqRTSXZNs=;\n\th=Reply-To:Subject:To:References:From:Date:In-Reply-To:From;\n\tb=oAO1uRTv1v5TVYvyF+12gWAZDort13vQf8xFrrQ5KTk19XGlo1RbHTh0WpSXo0u1X\n\tm8vEpH6DVz06MQdX1VvpsYI6lPMj28vc18j/+hDVoaoDRU8qIRhfmZBUPVisdJn0nP\n\tjy7u7akw+oYID8b51NkbkgBijbZlWpKFTR9dzyPQ=","Reply-To":"kieran.bingham@ideasonboard.com","To":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20200323232801.12010-1-laurent.pinchart@ideasonboard.com>","From":"Kieran Bingham ","Openpgp":"preference=signencrypt","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"","Date":"Tue, 24 Mar 2020 08:44:47 +0000","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101\n\tThunderbird/60.9.1","MIME-Version":"1.0","In-Reply-To":"<20200323232801.12010-1-laurent.pinchart@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v2.2 21/21] qcam: viewfinder: Add\n\tsupport for more native formats","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","X-List-Received-Date":"Tue, 24 Mar 2020 08:44:51 -0000"}},{"id":4253,"web_url":"https://patchwork.libcamera.org/comment/4253/","msgid":"<20200324090932.GA24971@pendragon.ideasonboard.com>","date":"2020-03-24T09:09:32","subject":"Re: [libcamera-devel] [PATCH v2.2 21/21] qcam: viewfinder: Add\n\tsupport for more native formats","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Tue, Mar 24, 2020 at 08:44:47AM +0000, Kieran Bingham wrote:\n> On 23/03/2020 23:28, Laurent Pinchart wrote:\n> > Qt supports more 24-bit and 32-bit RGB formats for native painting. If\n> > the frame buffer pixel format matches any of them, disable the converter\n> > and create a QImage in the appropriate format.\n> > \n> > Signed-off-by: Laurent Pinchart \n> > Reviewed-by: Kieran Bingham \n> > ---\n> > Changes since v2:\n> > \n> > - Add more formats\n> > - Remove unnecessary parentheses\n> > \n> > Changes since v1:\n> > \n> > - Rename formats to nativeFormats\n> > ---\n> > src/qcam/viewfinder.cpp | 25 ++++++++++++++++++++++---\n> > 1 file changed, 22 insertions(+), 3 deletions(-)\n> > \n> > diff --git a/src/qcam/viewfinder.cpp b/src/qcam/viewfinder.cpp\n> > index 45e226b58135..7e7407f56e4c 100644\n> > --- a/src/qcam/viewfinder.cpp\n> > +++ b/src/qcam/viewfinder.cpp\n> > @@ -7,16 +7,34 @@\n> > \n> > #include \"viewfinder.h\"\n> > \n> > +#include \n> > #include \n> > \n> > #include \n> > #include \n> > +#include \n> > #include \n> > #include \n> > #include \n> > \n> > #include \"format_converter.h\"\n> > \n> > +static const QMap nativeFormats\n> > +{\n> > +#if QT_VERSION >= QT_VERSION_CHECK(5, 2, 0)\n> > +#if QBYTE_ORDER == Q_LITTLE_ENDIAN\n> > +\t{ DRM_FORMAT_ABGR8888, QImage::Format_RGBA8888 },\n> > +#else\n> > +\t{ DRM_FORMAT_RGBA8888, QImage::Format_RGBA8888 },\n> > +#endif\n> \n> There's no else attributed to the QT_VERSION_CHECK, so we will not\n> support QImage::Format_RGBA8888 at all on version < 5.2.0...\n> \n> Presumably that's not an issue.\n\nWe don't really have a choice though:\n\n\"QImage::Format_RGBA8888\tThe image is stored using a 32-bit\nbyte-ordered RGBA format (8-8-8-8). Unlike ARGB32 this is a byte-ordered\nformat, which means the 32bit encoding differs between big endian and\nlittle endian architectures, being respectively (0xRRGGBBAA) and\n(0xAABBGGRR). The order of the colors is the same on any architecture if\nread as bytes 0xRR,0xGG,0xBB,0xAA. (added in Qt 5.2)\"\n\n> > +#endif\n> \n> Can we explain (in a comment perhaps) why we need to handle byte order\n> for QImage::Format_RGBA8888, but not QImage::Format_RGB32 ?\n\nAfter thinking more about it, I think the implementation here is\nincorrect. This particular format does not depend on byte ordering,\nwhile all the other do. I'll drop big endian handling for now, we can\nadd that later for all the formats after researching it.\n\n> > +\t{ DRM_FORMAT_ARGB8888, QImage::Format_RGB32 },\n> \n> > +#if QT_VERSION >= QT_VERSION_CHECK(5, 14, 0)\n> > +\t{ DRM_FORMAT_BGR888, QImage::Format_BGR888 },\n> > +#endif\n> > +\t{ DRM_FORMAT_RGB888, QImage::Format_RGB888 },\n> > +};\n> > +\n> > ViewFinder::ViewFinder(QWidget *parent)\n> > \t: QWidget(parent), buffer_(nullptr)\n> > {\n> > @@ -36,7 +54,7 @@ int ViewFinder::setFormat(const libcamera::PixelFormat &format,\n> > \t * If format conversion is needed, configure the converter and allocate\n> > \t * the destination image.\n> > \t */\n> > -\tif (format != DRM_FORMAT_ARGB8888) {\n> > +\tif (!nativeFormats.contains(format)) {\n> > \t\tint ret = converter_.configure(format, size);\n> > \t\tif (ret < 0)\n> > \t\t\treturn ret;\n> > @@ -64,7 +82,7 @@ void ViewFinder::render(libcamera::FrameBuffer *buffer, MappedBuffer *map)\n> > \t{\n> > \t\tQMutexLocker locker(&mutex_);\n> > \n> > -\t\tif (format_ == DRM_FORMAT_ARGB8888) {\n> > +\t\tif (nativeFormats.contains(format_)) {\n> > \t\t\t/*\n> > \t\t\t * If the frame format is identical to the display\n> > \t\t\t * format, create a QImage that references the frame\n> > @@ -76,7 +94,8 @@ void ViewFinder::render(libcamera::FrameBuffer *buffer, MappedBuffer *map)\n> > \t\t\t * computing it naively\n> > \t\t\t */\n> > \t\t\timage_ = QImage(memory, size_.width(), size_.height(),\n> > -\t\t\t\t\tsize / size_.height(), QImage::Format_RGB32);\n> > +\t\t\t\t\tsize / size_.height(),\n> > +\t\t\t\t\tnativeFormats[format_]);\n> > \t\t\tstd::swap(buffer, buffer_);\n> > \t\t} else {\n> > \t\t\t/*","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 059B460411\n\tfor ;\n\tTue, 24 Mar 2020 10:09:35 +0100 (CET)","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 678E9308;\n\tTue, 24 Mar 2020 10:09:34 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1585040974;\n\tbh=kTeohMUJf94ffGQ46pInHWmCCUveRdvgkYY982+6p1I=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=siNWBrSTRhiRmpoqdvtOPoZYKF4o2ND6pHoFjJJr3Yj8xEC9JMHjsrJBtrTPnpQoK\n\t3JWrLb2qvwoQOYdG6FdS5pST1abuDPtz6BIocSnsOlgbwyW1u9HwrcIRGbOT8kJx+1\n\t7M2E4qPV62JFV07zvMxVp0nX9z6hyuYEHgxhrpuw=","Date":"Tue, 24 Mar 2020 11:09:32 +0200","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200324090932.GA24971@pendragon.ideasonboard.com>","References":"<20200323232801.12010-1-laurent.pinchart@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v2.2 21/21] qcam: viewfinder: Add\n\tsupport for more native formats","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","X-List-Received-Date":"Tue, 24 Mar 2020 09:09:35 -0000"}}]