[libcamera-devel] [PATCH v2] qcam: viewfinder: Maintain aspect ratio

[Laurent Pinchart]

A few (last ?) comments.

As you only address the Qt-based viewfinder implementation,
s/viewfinder/viewfinder_qt/ in the subject line would be appropriate.

On Tue, Jun 15, 2021 at 01:59:07AM +0300, Laurent Pinchart wrote:
> On Tue, Jun 15, 2021 at 01:32:16AM +0300, Laurent Pinchart wrote:
> > On Mon, Jun 14, 2021 at 12:11:10PM +0100, Kieran Bingham wrote:
> > > Keep the image aspect ratio when displaying in the viewfinder.
> > > 
> > > When the window is adjusted to a size that differs in aspect ratio to
> > > the image, keep the image centered in the main viewpoint.

Did you mean s/viewpoint/viewfinder/ ?

> > > 
> > > Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > 
> > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > 
> > > ---
> > > 
> > > v2: (fixed up from Laurent's review comments)
> > > 
> > >  - No longer accept event on an invalid size.
> > >  - centering simplifed
> > > 
> > > 
> > >  src/qcam/viewfinder_qt.cpp | 16 ++++++++++++++--
> > >  src/qcam/viewfinder_qt.h   |  2 ++
> > >  2 files changed, 16 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/src/qcam/viewfinder_qt.cpp b/src/qcam/viewfinder_qt.cpp
> > > index e436714c6bdb..08ac29e9fa99 100644
> > > --- a/src/qcam/viewfinder_qt.cpp
> > > +++ b/src/qcam/viewfinder_qt.cpp
> > > @@ -15,6 +15,7 @@
> > >  #include <QMap>
> > >  #include <QMutexLocker>
> > >  #include <QPainter>
> > > +#include <QResizeEvent>
> > >  #include <QtDebug>
> > >  
> > >  #include <libcamera/formats.h>
> > > @@ -34,7 +35,7 @@ static const QMap<libcamera::PixelFormat, QImage::Format> nativeFormats
> > >  };
> > >  
> > >  ViewFinderQt::ViewFinderQt(QWidget *parent)
> > > -	: QWidget(parent), buffer_(nullptr)
> > > +	: QWidget(parent), place_(rect()), buffer_(nullptr)
> > >  {
> > >  	icon_ = QIcon(":camera-off.svg");
> > >  }
> > > @@ -146,7 +147,7 @@ void ViewFinderQt::paintEvent(QPaintEvent *)
> > >  
> > >  	/* If we have an image, draw it. */
> > >  	if (!image_.isNull()) {
> > > -		painter.drawImage(rect(), image_, image_.rect());
> > > +		painter.drawImage(place_, image_, image_.rect());
> > >  		return;
> > >  	}
> > >  
> > > @@ -179,3 +180,14 @@ QSize ViewFinderQt::sizeHint() const
> > >  {
> > >  	return size_.isValid() ? size_ : QSize(640, 480);
> > >  }
> > > +
> > > +void ViewFinderQt::resizeEvent(QResizeEvent *event)
> > > +{
> > > +	if (!size_.isValid())
> > > +		return;
> > > +
> > > +	place_.setSize(size_.scaled(event->size(), Qt::KeepAspectRatio));
> > > +	place_.moveCenter(rect().center());
> > > +
> > > +	event->accept();
> 
> As far as I can tell, event->accept() isn't needed. It's however a good
> practice to call the base object's resizeEvent() function. I'd thus
> replace this with
> 
> 	QWidget::resizeEvent(event);
> 
> > > +}
> > > diff --git a/src/qcam/viewfinder_qt.h b/src/qcam/viewfinder_qt.h
> > > index d755428887c0..bf31b81b3437 100644
> > > --- a/src/qcam/viewfinder_qt.h
> > > +++ b/src/qcam/viewfinder_qt.h
> > > @@ -42,6 +42,7 @@ Q_SIGNALS:
> > >  
> > >  protected:
> > >  	void paintEvent(QPaintEvent *) override;
> > > +	void resizeEvent(QResizeEvent *) override;
> > >  	QSize sizeHint() const override;
> > >  
> > >  private:
> > > @@ -49,6 +50,7 @@ private:
> > >  
> > >  	libcamera::PixelFormat format_;
> > >  	QSize size_;
> > > +	QRect place_;
> > >  
> > >  	/* Camera stopped icon */
> > >  	QSize vfSize_;

-- 
Regards,

Laurent Pinchart