[{"id":17533,"web_url":"https://patchwork.libcamera.org/comment/17533/","msgid":"","date":"2021-06-14T22:32:15","subject":"Re: [libcamera-devel] [PATCH v2] qcam: viewfinder: Maintain aspect\n\tratio","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nThank you for the patch.\n\nOn Mon, Jun 14, 2021 at 12:11:10PM +0100, Kieran Bingham wrote:\n> Keep the image aspect ratio when displaying in the viewfinder.\n> \n> When the window is adjusted to a size that differs in aspect ratio to\n> the image, keep the image centered in the main viewpoint.\n> \n> Signed-off-by: Kieran Bingham \n\nReviewed-by: Laurent Pinchart \n\n> ---\n> \n> v2: (fixed up from Laurent's review comments)\n> \n> - No longer accept event on an invalid size.\n> - centering simplifed\n> \n> \n> src/qcam/viewfinder_qt.cpp | 16 ++++++++++++++--\n> src/qcam/viewfinder_qt.h | 2 ++\n> 2 files changed, 16 insertions(+), 2 deletions(-)\n> \n> diff --git a/src/qcam/viewfinder_qt.cpp b/src/qcam/viewfinder_qt.cpp\n> index e436714c6bdb..08ac29e9fa99 100644\n> --- a/src/qcam/viewfinder_qt.cpp\n> +++ b/src/qcam/viewfinder_qt.cpp\n> @@ -15,6 +15,7 @@\n> #include \n> #include \n> #include \n> +#include \n> #include \n> \n> #include \n> @@ -34,7 +35,7 @@ static const QMap nativeFormats\n> };\n> \n> ViewFinderQt::ViewFinderQt(QWidget *parent)\n> -\t: QWidget(parent), buffer_(nullptr)\n> +\t: QWidget(parent), place_(rect()), buffer_(nullptr)\n> {\n> \ticon_ = QIcon(\":camera-off.svg\");\n> }\n> @@ -146,7 +147,7 @@ void ViewFinderQt::paintEvent(QPaintEvent *)\n> \n> \t/* If we have an image, draw it. */\n> \tif (!image_.isNull()) {\n> -\t\tpainter.drawImage(rect(), image_, image_.rect());\n> +\t\tpainter.drawImage(place_, image_, image_.rect());\n> \t\treturn;\n> \t}\n> \n> @@ -179,3 +180,14 @@ QSize ViewFinderQt::sizeHint() const\n> {\n> \treturn size_.isValid() ? size_ : QSize(640, 480);\n> }\n> +\n> +void ViewFinderQt::resizeEvent(QResizeEvent *event)\n> +{\n> +\tif (!size_.isValid())\n> +\t\treturn;\n> +\n> +\tplace_.setSize(size_.scaled(event->size(), Qt::KeepAspectRatio));\n> +\tplace_.moveCenter(rect().center());\n> +\n> +\tevent->accept();\n> +}\n> diff --git a/src/qcam/viewfinder_qt.h b/src/qcam/viewfinder_qt.h\n> index d755428887c0..bf31b81b3437 100644\n> --- a/src/qcam/viewfinder_qt.h\n> +++ b/src/qcam/viewfinder_qt.h\n> @@ -42,6 +42,7 @@ Q_SIGNALS:\n> \n> protected:\n> \tvoid paintEvent(QPaintEvent *) override;\n> +\tvoid resizeEvent(QResizeEvent *) override;\n> \tQSize sizeHint() const override;\n> \n> private:\n> @@ -49,6 +50,7 @@ private:\n> \n> \tlibcamera::PixelFormat format_;\n> \tQSize size_;\n> +\tQRect place_;\n> \n> \t/* Camera stopped icon */\n> \tQSize vfSize_;","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 D80E9BD78E\n\tfor ;\n\tMon, 14 Jun 2021 22:32:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4D9C368931;\n\tTue, 15 Jun 2021 00:32:37 +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 CBA03687EF\n\tfor ;\n\tTue, 15 Jun 2021 00:32:35 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 3DC2445E;\n\tTue, 15 Jun 2021 00:32:35 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"l5aMpZTM\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1623709955;\n\tbh=vEi6ObkDGypbXrpf/P23dEhuVu/KDwPVp9/aCunB26U=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=l5aMpZTM0OlXpTogBHbJel0PF7PTQlzIsd2nH2A5aPknLUF7nr2Yb1VWEw27LmTvJ\n\tZ8LpeBuVOW8WxglLPuEKYuxoxm2/zzn3FZbvIyI66Pe8mhwxdljV5nSP1FT4CvHkzi\n\tzW+EUJRNXcpoByxvfrGpte7JRrsnhccNYvtxaOCo=","Date":"Tue, 15 Jun 2021 01:32:15 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"","References":"<20210614111110.33324-1-kieran.bingham@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210614111110.33324-1-kieran.bingham@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2] qcam: viewfinder: Maintain aspect\n\tratio","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17535,"web_url":"https://patchwork.libcamera.org/comment/17535/","msgid":"","date":"2021-06-14T22:59:06","subject":"Re: [libcamera-devel] [PATCH v2] qcam: viewfinder: Maintain aspect\n\tratio","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, Jun 15, 2021 at 01:32:16AM +0300, Laurent Pinchart wrote:\n> Hi Kieran,\n> \n> Thank you for the patch.\n> \n> On Mon, Jun 14, 2021 at 12:11:10PM +0100, Kieran Bingham wrote:\n> > Keep the image aspect ratio when displaying in the viewfinder.\n> > \n> > When the window is adjusted to a size that differs in aspect ratio to\n> > the image, keep the image centered in the main viewpoint.\n> > \n> > Signed-off-by: Kieran Bingham \n> \n> Reviewed-by: Laurent Pinchart \n> \n> > ---\n> > \n> > v2: (fixed up from Laurent's review comments)\n> > \n> > - No longer accept event on an invalid size.\n> > - centering simplifed\n> > \n> > \n> > src/qcam/viewfinder_qt.cpp | 16 ++++++++++++++--\n> > src/qcam/viewfinder_qt.h | 2 ++\n> > 2 files changed, 16 insertions(+), 2 deletions(-)\n> > \n> > diff --git a/src/qcam/viewfinder_qt.cpp b/src/qcam/viewfinder_qt.cpp\n> > index e436714c6bdb..08ac29e9fa99 100644\n> > --- a/src/qcam/viewfinder_qt.cpp\n> > +++ b/src/qcam/viewfinder_qt.cpp\n> > @@ -15,6 +15,7 @@\n> > #include \n> > #include \n> > #include \n> > +#include \n> > #include \n> > \n> > #include \n> > @@ -34,7 +35,7 @@ static const QMap nativeFormats\n> > };\n> > \n> > ViewFinderQt::ViewFinderQt(QWidget *parent)\n> > -\t: QWidget(parent), buffer_(nullptr)\n> > +\t: QWidget(parent), place_(rect()), buffer_(nullptr)\n> > {\n> > \ticon_ = QIcon(\":camera-off.svg\");\n> > }\n> > @@ -146,7 +147,7 @@ void ViewFinderQt::paintEvent(QPaintEvent *)\n> > \n> > \t/* If we have an image, draw it. */\n> > \tif (!image_.isNull()) {\n> > -\t\tpainter.drawImage(rect(), image_, image_.rect());\n> > +\t\tpainter.drawImage(place_, image_, image_.rect());\n> > \t\treturn;\n> > \t}\n> > \n> > @@ -179,3 +180,14 @@ QSize ViewFinderQt::sizeHint() const\n> > {\n> > \treturn size_.isValid() ? size_ : QSize(640, 480);\n> > }\n> > +\n> > +void ViewFinderQt::resizeEvent(QResizeEvent *event)\n> > +{\n> > +\tif (!size_.isValid())\n> > +\t\treturn;\n> > +\n> > +\tplace_.setSize(size_.scaled(event->size(), Qt::KeepAspectRatio));\n> > +\tplace_.moveCenter(rect().center());\n> > +\n> > +\tevent->accept();\n\nAs far as I can tell, event->accept() isn't needed. It's however a good\npractice to call the base object's resizeEvent() function. I'd thus\nreplace this with\n\n\tQWidget::resizeEvent(event);\n\n> > +}\n> > diff --git a/src/qcam/viewfinder_qt.h b/src/qcam/viewfinder_qt.h\n> > index d755428887c0..bf31b81b3437 100644\n> > --- a/src/qcam/viewfinder_qt.h\n> > +++ b/src/qcam/viewfinder_qt.h\n> > @@ -42,6 +42,7 @@ Q_SIGNALS:\n> > \n> > protected:\n> > \tvoid paintEvent(QPaintEvent *) override;\n> > +\tvoid resizeEvent(QResizeEvent *) override;\n> > \tQSize sizeHint() const override;\n> > \n> > private:\n> > @@ -49,6 +50,7 @@ private:\n> > \n> > \tlibcamera::PixelFormat format_;\n> > \tQSize size_;\n> > +\tQRect place_;\n> > \n> > \t/* Camera stopped icon */\n> > \tQSize vfSize_;","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 ADE72BD78E\n\tfor ;\n\tMon, 14 Jun 2021 22:59:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 28F7068931;\n\tTue, 15 Jun 2021 00:59:28 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E0E99687EF\n\tfor ;\n\tTue, 15 Jun 2021 00:59:26 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5150F45E;\n\tTue, 15 Jun 2021 00:59:26 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"EBBPPNnL\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1623711566;\n\tbh=3d4DboYO8QAI9vg7pEPeoKamf/iOCKj/JgBu8KTtRDk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=EBBPPNnLnorCKPOPVDtvecQTjt3mtw+2Y2Oj2zmkZ1LUqcV2os1UrLpoq3/fM3bwq\n\t6RCoT1BzwFFE8cFLtbe2B8AyppHNZVR9aAJXE1wXkq/chnJK4BldtlXn1xEybnmIn1\n\t0qV9YABnkm3fWdK6SB+ADA2aF0O1QPHH0QBGsQqA=","Date":"Tue, 15 Jun 2021 01:59:06 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"","References":"<20210614111110.33324-1-kieran.bingham@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2] qcam: viewfinder: Maintain aspect\n\tratio","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":17536,"web_url":"https://patchwork.libcamera.org/comment/17536/","msgid":"","date":"2021-06-14T23:55:11","subject":"Re: [libcamera-devel] [PATCH v2] qcam: viewfinder: Maintain aspect\n\tratio","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"A few (last ?) comments.\n\nAs you only address the Qt-based viewfinder implementation,\ns/viewfinder/viewfinder_qt/ in the subject line would be appropriate.\n\nOn Tue, Jun 15, 2021 at 01:59:07AM +0300, Laurent Pinchart wrote:\n> On Tue, Jun 15, 2021 at 01:32:16AM +0300, Laurent Pinchart wrote:\n> > On Mon, Jun 14, 2021 at 12:11:10PM +0100, Kieran Bingham wrote:\n> > > Keep the image aspect ratio when displaying in the viewfinder.\n> > > \n> > > When the window is adjusted to a size that differs in aspect ratio to\n> > > the image, keep the image centered in the main viewpoint.\n\nDid you mean s/viewpoint/viewfinder/ ?\n\n> > > \n> > > Signed-off-by: Kieran Bingham \n> > \n> > Reviewed-by: Laurent Pinchart \n> > \n> > > ---\n> > > \n> > > v2: (fixed up from Laurent's review comments)\n> > > \n> > > - No longer accept event on an invalid size.\n> > > - centering simplifed\n> > > \n> > > \n> > > src/qcam/viewfinder_qt.cpp | 16 ++++++++++++++--\n> > > src/qcam/viewfinder_qt.h | 2 ++\n> > > 2 files changed, 16 insertions(+), 2 deletions(-)\n> > > \n> > > diff --git a/src/qcam/viewfinder_qt.cpp b/src/qcam/viewfinder_qt.cpp\n> > > index e436714c6bdb..08ac29e9fa99 100644\n> > > --- a/src/qcam/viewfinder_qt.cpp\n> > > +++ b/src/qcam/viewfinder_qt.cpp\n> > > @@ -15,6 +15,7 @@\n> > > #include \n> > > #include \n> > > #include \n> > > +#include \n> > > #include \n> > > \n> > > #include \n> > > @@ -34,7 +35,7 @@ static const QMap nativeFormats\n> > > };\n> > > \n> > > ViewFinderQt::ViewFinderQt(QWidget *parent)\n> > > -\t: QWidget(parent), buffer_(nullptr)\n> > > +\t: QWidget(parent), place_(rect()), buffer_(nullptr)\n> > > {\n> > > \ticon_ = QIcon(\":camera-off.svg\");\n> > > }\n> > > @@ -146,7 +147,7 @@ void ViewFinderQt::paintEvent(QPaintEvent *)\n> > > \n> > > \t/* If we have an image, draw it. */\n> > > \tif (!image_.isNull()) {\n> > > -\t\tpainter.drawImage(rect(), image_, image_.rect());\n> > > +\t\tpainter.drawImage(place_, image_, image_.rect());\n> > > \t\treturn;\n> > > \t}\n> > > \n> > > @@ -179,3 +180,14 @@ QSize ViewFinderQt::sizeHint() const\n> > > {\n> > > \treturn size_.isValid() ? size_ : QSize(640, 480);\n> > > }\n> > > +\n> > > +void ViewFinderQt::resizeEvent(QResizeEvent *event)\n> > > +{\n> > > +\tif (!size_.isValid())\n> > > +\t\treturn;\n> > > +\n> > > +\tplace_.setSize(size_.scaled(event->size(), Qt::KeepAspectRatio));\n> > > +\tplace_.moveCenter(rect().center());\n> > > +\n> > > +\tevent->accept();\n> \n> As far as I can tell, event->accept() isn't needed. It's however a good\n> practice to call the base object's resizeEvent() function. I'd thus\n> replace this with\n> \n> \tQWidget::resizeEvent(event);\n> \n> > > +}\n> > > diff --git a/src/qcam/viewfinder_qt.h b/src/qcam/viewfinder_qt.h\n> > > index d755428887c0..bf31b81b3437 100644\n> > > --- a/src/qcam/viewfinder_qt.h\n> > > +++ b/src/qcam/viewfinder_qt.h\n> > > @@ -42,6 +42,7 @@ Q_SIGNALS:\n> > > \n> > > protected:\n> > > \tvoid paintEvent(QPaintEvent *) override;\n> > > +\tvoid resizeEvent(QResizeEvent *) override;\n> > > \tQSize sizeHint() const override;\n> > > \n> > > private:\n> > > @@ -49,6 +50,7 @@ private:\n> > > \n> > > \tlibcamera::PixelFormat format_;\n> > > \tQSize size_;\n> > > +\tQRect place_;\n> > > \n> > > \t/* Camera stopped icon */\n> > > \tQSize vfSize_;","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 3A431C3218\n\tfor ;\n\tMon, 14 Jun 2021 23:55:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 883246892B;\n\tTue, 15 Jun 2021 01:55:33 +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 5EEF2687EF\n\tfor ;\n\tTue, 15 Jun 2021 01:55:32 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id BD6CC45E;\n\tTue, 15 Jun 2021 01:55:31 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"peBnsP3l\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1623714931;\n\tbh=P2863ZeNcSYu+sIHi3RxImfnkYfDfwp4iS8KMHdtyfs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=peBnsP3ly7jmxzzPVzjVdMsHTdb+2kCc/3vrADpSnOP+qUxfDz+gUztA1qJHzlmJV\n\t8kYsWdeCRsOiL/HVq1p7RFavXQkqIzgWWrVxBdE0Fd9zqPXPluO6LIpsyGwAnWSbvW\n\txHVhW1MlCRpOdUUYna9ELnVRbLdnGErNTGx7h92I=","Date":"Tue, 15 Jun 2021 02:55:11 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"","References":"<20210614111110.33324-1-kieran.bingham@ideasonboard.com>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2] qcam: viewfinder: Maintain aspect\n\tratio","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]