| Message ID | 20210907002044.7319-2-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, On 07/09/2021 01:20, Laurent Pinchart wrote: > qcam currently assumes that no padding is used at end of lines, and uses > the image width as the stride. This leads to rendering failures with > some formats on some platforms. To prepare for stride support, add a > stride parameter to the ViewFinder::setFormat() function to pass the > stride from the stream configuration to the viewfinder. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > src/qcam/main_window.cpp | 3 ++- > src/qcam/viewfinder.h | 3 ++- > src/qcam/viewfinder_gl.cpp | 7 ++----- > src/qcam/viewfinder_gl.h | 3 ++- > src/qcam/viewfinder_qt.cpp | 3 ++- > src/qcam/viewfinder_qt.h | 3 ++- > 6 files changed, 12 insertions(+), 10 deletions(-) > > diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp > index 168dd5ce30e3..bb6b03993add 100644 > --- a/src/qcam/main_window.cpp > +++ b/src/qcam/main_window.cpp > @@ -448,7 +448,8 @@ int MainWindow::startCapture() > > /* Configure the viewfinder. */ > ret = viewfinder_->setFormat(vfConfig.pixelFormat, > - QSize(vfConfig.size.width, vfConfig.size.height)); > + QSize(vfConfig.size.width, vfConfig.size.height), > + vfConfig.stride); > if (ret < 0) { > qInfo() << "Failed to set viewfinder format"; > return ret; > diff --git a/src/qcam/viewfinder.h b/src/qcam/viewfinder.h > index fb462835fb5f..4c2102a6ed04 100644 > --- a/src/qcam/viewfinder.h > +++ b/src/qcam/viewfinder.h > @@ -23,7 +23,8 @@ public: > > virtual const QList<libcamera::PixelFormat> &nativeFormats() const = 0; > > - virtual int setFormat(const libcamera::PixelFormat &format, const QSize &size) = 0; > + virtual int setFormat(const libcamera::PixelFormat &format, const QSize &size, > + unsigned int stride) = 0; > virtual void render(libcamera::FrameBuffer *buffer, Image *image) = 0; > virtual void stop() = 0; > > diff --git a/src/qcam/viewfinder_gl.cpp b/src/qcam/viewfinder_gl.cpp > index 32232faa2ad8..aeb1ea02d2d5 100644 > --- a/src/qcam/viewfinder_gl.cpp > +++ b/src/qcam/viewfinder_gl.cpp > @@ -72,7 +72,7 @@ const QList<libcamera::PixelFormat> &ViewFinderGL::nativeFormats() const > } > > int ViewFinderGL::setFormat(const libcamera::PixelFormat &format, > - const QSize &size) > + const QSize &size, unsigned int stride) > { > if (format != format_) { > /* > @@ -92,6 +92,7 @@ int ViewFinderGL::setFormat(const libcamera::PixelFormat &format, > } > > size_ = size; > + stride_ = stride; > > updateGeometry(); > return 0; > @@ -119,10 +120,6 @@ void ViewFinderGL::render(libcamera::FrameBuffer *buffer, Image *image) > renderComplete(buffer_); > > image_ = image; > - /* > - * \todo Get the stride from the buffer instead of computing it naively > - */ We're getting it from the StreamConfiguration instead of the PixelFormatInfo, or the buffer ... Are they (sufficiently) equivalent? The documentation suggests subtle differences. Is this patch 'all' that is required to ensure the strides are passed correctly, or do we need to keep/add a todo to make it clear that there is further work up at "Configure the viewfinder" to handle plane specific strides? I guess that would become evident if StreamConfiguration::stride became a span of strides... so perhaps the compiler will act as a todo if that ever happens. I don't want to complain too much though.. Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > - stride_ = buffer->metadata().planes()[0].bytesused / size_.height(); > update(); > buffer_ = buffer; > } > diff --git a/src/qcam/viewfinder_gl.h b/src/qcam/viewfinder_gl.h > index 72a60ecb9159..2b2b1e86035a 100644 > --- a/src/qcam/viewfinder_gl.h > +++ b/src/qcam/viewfinder_gl.h > @@ -38,7 +38,8 @@ public: > > const QList<libcamera::PixelFormat> &nativeFormats() const override; > > - int setFormat(const libcamera::PixelFormat &format, const QSize &size) override; > + int setFormat(const libcamera::PixelFormat &format, const QSize &size, > + unsigned int stride) override; > void render(libcamera::FrameBuffer *buffer, Image *image) override; > void stop() override; > > diff --git a/src/qcam/viewfinder_qt.cpp b/src/qcam/viewfinder_qt.cpp > index 0d357d860014..cd051760160c 100644 > --- a/src/qcam/viewfinder_qt.cpp > +++ b/src/qcam/viewfinder_qt.cpp > @@ -52,7 +52,8 @@ const QList<libcamera::PixelFormat> &ViewFinderQt::nativeFormats() const > } > > int ViewFinderQt::setFormat(const libcamera::PixelFormat &format, > - const QSize &size) > + const QSize &size, > + [[maybe_unused]] unsigned int stride) > { > image_ = QImage(); > > diff --git a/src/qcam/viewfinder_qt.h b/src/qcam/viewfinder_qt.h > index 6b48ef48a7d1..756f3fa33055 100644 > --- a/src/qcam/viewfinder_qt.h > +++ b/src/qcam/viewfinder_qt.h > @@ -31,7 +31,8 @@ public: > > const QList<libcamera::PixelFormat> &nativeFormats() const override; > > - int setFormat(const libcamera::PixelFormat &format, const QSize &size) override; > + int setFormat(const libcamera::PixelFormat &format, const QSize &size, > + unsigned int stride) override; > void render(libcamera::FrameBuffer *buffer, Image *image) override; > void stop() override; > >
Hi Kieran, On Tue, Sep 21, 2021 at 03:43:12PM +0100, Kieran Bingham wrote: > On 07/09/2021 01:20, Laurent Pinchart wrote: > > qcam currently assumes that no padding is used at end of lines, and uses > > the image width as the stride. This leads to rendering failures with > > some formats on some platforms. To prepare for stride support, add a > > stride parameter to the ViewFinder::setFormat() function to pass the > > stride from the stream configuration to the viewfinder. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > src/qcam/main_window.cpp | 3 ++- > > src/qcam/viewfinder.h | 3 ++- > > src/qcam/viewfinder_gl.cpp | 7 ++----- > > src/qcam/viewfinder_gl.h | 3 ++- > > src/qcam/viewfinder_qt.cpp | 3 ++- > > src/qcam/viewfinder_qt.h | 3 ++- > > 6 files changed, 12 insertions(+), 10 deletions(-) > > > > diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp > > index 168dd5ce30e3..bb6b03993add 100644 > > --- a/src/qcam/main_window.cpp > > +++ b/src/qcam/main_window.cpp > > @@ -448,7 +448,8 @@ int MainWindow::startCapture() > > > > /* Configure the viewfinder. */ > > ret = viewfinder_->setFormat(vfConfig.pixelFormat, > > - QSize(vfConfig.size.width, vfConfig.size.height)); > > + QSize(vfConfig.size.width, vfConfig.size.height), > > + vfConfig.stride); > > if (ret < 0) { > > qInfo() << "Failed to set viewfinder format"; > > return ret; > > diff --git a/src/qcam/viewfinder.h b/src/qcam/viewfinder.h > > index fb462835fb5f..4c2102a6ed04 100644 > > --- a/src/qcam/viewfinder.h > > +++ b/src/qcam/viewfinder.h > > @@ -23,7 +23,8 @@ public: > > > > virtual const QList<libcamera::PixelFormat> &nativeFormats() const = 0; > > > > - virtual int setFormat(const libcamera::PixelFormat &format, const QSize &size) = 0; > > + virtual int setFormat(const libcamera::PixelFormat &format, const QSize &size, > > + unsigned int stride) = 0; > > virtual void render(libcamera::FrameBuffer *buffer, Image *image) = 0; > > virtual void stop() = 0; > > > > diff --git a/src/qcam/viewfinder_gl.cpp b/src/qcam/viewfinder_gl.cpp > > index 32232faa2ad8..aeb1ea02d2d5 100644 > > --- a/src/qcam/viewfinder_gl.cpp > > +++ b/src/qcam/viewfinder_gl.cpp > > @@ -72,7 +72,7 @@ const QList<libcamera::PixelFormat> &ViewFinderGL::nativeFormats() const > > } > > > > int ViewFinderGL::setFormat(const libcamera::PixelFormat &format, > > - const QSize &size) > > + const QSize &size, unsigned int stride) > > { > > if (format != format_) { > > /* > > @@ -92,6 +92,7 @@ int ViewFinderGL::setFormat(const libcamera::PixelFormat &format, > > } > > > > size_ = size; > > + stride_ = stride; > > > > updateGeometry(); > > return 0; > > @@ -119,10 +120,6 @@ void ViewFinderGL::render(libcamera::FrameBuffer *buffer, Image *image) > > renderComplete(buffer_); > > > > image_ = image; > > - /* > > - * \todo Get the stride from the buffer instead of computing it naively > > - */ > > We're getting it from the StreamConfiguration instead of the > PixelFormatInfo, or the buffer ... Are they (sufficiently) equivalent? Mentioning "buffer" here was likely a mistake. Our current model sets the stride at configuration time, and it's then considered to be fixed for the duration of the session. The stride from the pixel format is different, as a pixel format can only provide information about possible strides, while the stream configuration contains the actual stride. > The documentation suggests subtle differences. > > Is this patch 'all' that is required to ensure the strides are passed > correctly, or do we need to keep/add a todo to make it clear that there > is further work up at "Configure the viewfinder" to handle plane > specific strides? That's all there is to handle the stride correctly with the current API. We may need to negotiate strides between the source and sink at some point, in case the sink can't work with the default stride from the source, but that's not covered by this \todo anyway. > I guess that would become evident if StreamConfiguration::stride became > a span of strides... so perhaps the compiler will act as a todo if that > ever happens. Yes, if the API changes, then the compiler will let us know :-) I'm not entirely sure how to handle that though, as we could support per-plane strides, but most devices won't support that. > I don't want to complain too much though.. > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > - stride_ = buffer->metadata().planes()[0].bytesused / size_.height(); > > update(); > > buffer_ = buffer; > > } > > diff --git a/src/qcam/viewfinder_gl.h b/src/qcam/viewfinder_gl.h > > index 72a60ecb9159..2b2b1e86035a 100644 > > --- a/src/qcam/viewfinder_gl.h > > +++ b/src/qcam/viewfinder_gl.h > > @@ -38,7 +38,8 @@ public: > > > > const QList<libcamera::PixelFormat> &nativeFormats() const override; > > > > - int setFormat(const libcamera::PixelFormat &format, const QSize &size) override; > > + int setFormat(const libcamera::PixelFormat &format, const QSize &size, > > + unsigned int stride) override; > > void render(libcamera::FrameBuffer *buffer, Image *image) override; > > void stop() override; > > > > diff --git a/src/qcam/viewfinder_qt.cpp b/src/qcam/viewfinder_qt.cpp > > index 0d357d860014..cd051760160c 100644 > > --- a/src/qcam/viewfinder_qt.cpp > > +++ b/src/qcam/viewfinder_qt.cpp > > @@ -52,7 +52,8 @@ const QList<libcamera::PixelFormat> &ViewFinderQt::nativeFormats() const > > } > > > > int ViewFinderQt::setFormat(const libcamera::PixelFormat &format, > > - const QSize &size) > > + const QSize &size, > > + [[maybe_unused]] unsigned int stride) > > { > > image_ = QImage(); > > > > diff --git a/src/qcam/viewfinder_qt.h b/src/qcam/viewfinder_qt.h > > index 6b48ef48a7d1..756f3fa33055 100644 > > --- a/src/qcam/viewfinder_qt.h > > +++ b/src/qcam/viewfinder_qt.h > > @@ -31,7 +31,8 @@ public: > > > > const QList<libcamera::PixelFormat> &nativeFormats() const override; > > > > - int setFormat(const libcamera::PixelFormat &format, const QSize &size) override; > > + int setFormat(const libcamera::PixelFormat &format, const QSize &size, > > + unsigned int stride) override; > > void render(libcamera::FrameBuffer *buffer, Image *image) override; > > void stop() override; > >
Hi Laurent, On Tue, Sep 07, 2021 at 03:20:40AM +0300, Laurent Pinchart wrote: > qcam currently assumes that no padding is used at end of lines, and uses > the image width as the stride. This leads to rendering failures with > some formats on some platforms. To prepare for stride support, add a > stride parameter to the ViewFinder::setFormat() function to pass the > stride from the stream configuration to the viewfinder. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > --- > src/qcam/main_window.cpp | 3 ++- > src/qcam/viewfinder.h | 3 ++- > src/qcam/viewfinder_gl.cpp | 7 ++----- > src/qcam/viewfinder_gl.h | 3 ++- > src/qcam/viewfinder_qt.cpp | 3 ++- > src/qcam/viewfinder_qt.h | 3 ++- > 6 files changed, 12 insertions(+), 10 deletions(-) > > diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp > index 168dd5ce30e3..bb6b03993add 100644 > --- a/src/qcam/main_window.cpp > +++ b/src/qcam/main_window.cpp > @@ -448,7 +448,8 @@ int MainWindow::startCapture() > > /* Configure the viewfinder. */ > ret = viewfinder_->setFormat(vfConfig.pixelFormat, > - QSize(vfConfig.size.width, vfConfig.size.height)); > + QSize(vfConfig.size.width, vfConfig.size.height), > + vfConfig.stride); > if (ret < 0) { > qInfo() << "Failed to set viewfinder format"; > return ret; > diff --git a/src/qcam/viewfinder.h b/src/qcam/viewfinder.h > index fb462835fb5f..4c2102a6ed04 100644 > --- a/src/qcam/viewfinder.h > +++ b/src/qcam/viewfinder.h > @@ -23,7 +23,8 @@ public: > > virtual const QList<libcamera::PixelFormat> &nativeFormats() const = 0; > > - virtual int setFormat(const libcamera::PixelFormat &format, const QSize &size) = 0; > + virtual int setFormat(const libcamera::PixelFormat &format, const QSize &size, > + unsigned int stride) = 0; > virtual void render(libcamera::FrameBuffer *buffer, Image *image) = 0; > virtual void stop() = 0; > > diff --git a/src/qcam/viewfinder_gl.cpp b/src/qcam/viewfinder_gl.cpp > index 32232faa2ad8..aeb1ea02d2d5 100644 > --- a/src/qcam/viewfinder_gl.cpp > +++ b/src/qcam/viewfinder_gl.cpp > @@ -72,7 +72,7 @@ const QList<libcamera::PixelFormat> &ViewFinderGL::nativeFormats() const > } > > int ViewFinderGL::setFormat(const libcamera::PixelFormat &format, > - const QSize &size) > + const QSize &size, unsigned int stride) > { > if (format != format_) { > /* > @@ -92,6 +92,7 @@ int ViewFinderGL::setFormat(const libcamera::PixelFormat &format, > } > > size_ = size; > + stride_ = stride; > > updateGeometry(); > return 0; > @@ -119,10 +120,6 @@ void ViewFinderGL::render(libcamera::FrameBuffer *buffer, Image *image) > renderComplete(buffer_); > > image_ = image; > - /* > - * \todo Get the stride from the buffer instead of computing it naively > - */ > - stride_ = buffer->metadata().planes()[0].bytesused / size_.height(); > update(); > buffer_ = buffer; > } > diff --git a/src/qcam/viewfinder_gl.h b/src/qcam/viewfinder_gl.h > index 72a60ecb9159..2b2b1e86035a 100644 > --- a/src/qcam/viewfinder_gl.h > +++ b/src/qcam/viewfinder_gl.h > @@ -38,7 +38,8 @@ public: > > const QList<libcamera::PixelFormat> &nativeFormats() const override; > > - int setFormat(const libcamera::PixelFormat &format, const QSize &size) override; > + int setFormat(const libcamera::PixelFormat &format, const QSize &size, > + unsigned int stride) override; > void render(libcamera::FrameBuffer *buffer, Image *image) override; > void stop() override; > > diff --git a/src/qcam/viewfinder_qt.cpp b/src/qcam/viewfinder_qt.cpp > index 0d357d860014..cd051760160c 100644 > --- a/src/qcam/viewfinder_qt.cpp > +++ b/src/qcam/viewfinder_qt.cpp > @@ -52,7 +52,8 @@ const QList<libcamera::PixelFormat> &ViewFinderQt::nativeFormats() const > } > > int ViewFinderQt::setFormat(const libcamera::PixelFormat &format, > - const QSize &size) > + const QSize &size, > + [[maybe_unused]] unsigned int stride) > { > image_ = QImage(); > > diff --git a/src/qcam/viewfinder_qt.h b/src/qcam/viewfinder_qt.h > index 6b48ef48a7d1..756f3fa33055 100644 > --- a/src/qcam/viewfinder_qt.h > +++ b/src/qcam/viewfinder_qt.h > @@ -31,7 +31,8 @@ public: > > const QList<libcamera::PixelFormat> &nativeFormats() const override; > > - int setFormat(const libcamera::PixelFormat &format, const QSize &size) override; > + int setFormat(const libcamera::PixelFormat &format, const QSize &size, > + unsigned int stride) override; > void render(libcamera::FrameBuffer *buffer, Image *image) override; > void stop() override; > > -- > Regards, > > Laurent Pinchart >
diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp index 168dd5ce30e3..bb6b03993add 100644 --- a/src/qcam/main_window.cpp +++ b/src/qcam/main_window.cpp @@ -448,7 +448,8 @@ int MainWindow::startCapture() /* Configure the viewfinder. */ ret = viewfinder_->setFormat(vfConfig.pixelFormat, - QSize(vfConfig.size.width, vfConfig.size.height)); + QSize(vfConfig.size.width, vfConfig.size.height), + vfConfig.stride); if (ret < 0) { qInfo() << "Failed to set viewfinder format"; return ret; diff --git a/src/qcam/viewfinder.h b/src/qcam/viewfinder.h index fb462835fb5f..4c2102a6ed04 100644 --- a/src/qcam/viewfinder.h +++ b/src/qcam/viewfinder.h @@ -23,7 +23,8 @@ public: virtual const QList<libcamera::PixelFormat> &nativeFormats() const = 0; - virtual int setFormat(const libcamera::PixelFormat &format, const QSize &size) = 0; + virtual int setFormat(const libcamera::PixelFormat &format, const QSize &size, + unsigned int stride) = 0; virtual void render(libcamera::FrameBuffer *buffer, Image *image) = 0; virtual void stop() = 0; diff --git a/src/qcam/viewfinder_gl.cpp b/src/qcam/viewfinder_gl.cpp index 32232faa2ad8..aeb1ea02d2d5 100644 --- a/src/qcam/viewfinder_gl.cpp +++ b/src/qcam/viewfinder_gl.cpp @@ -72,7 +72,7 @@ const QList<libcamera::PixelFormat> &ViewFinderGL::nativeFormats() const } int ViewFinderGL::setFormat(const libcamera::PixelFormat &format, - const QSize &size) + const QSize &size, unsigned int stride) { if (format != format_) { /* @@ -92,6 +92,7 @@ int ViewFinderGL::setFormat(const libcamera::PixelFormat &format, } size_ = size; + stride_ = stride; updateGeometry(); return 0; @@ -119,10 +120,6 @@ void ViewFinderGL::render(libcamera::FrameBuffer *buffer, Image *image) renderComplete(buffer_); image_ = image; - /* - * \todo Get the stride from the buffer instead of computing it naively - */ - stride_ = buffer->metadata().planes()[0].bytesused / size_.height(); update(); buffer_ = buffer; } diff --git a/src/qcam/viewfinder_gl.h b/src/qcam/viewfinder_gl.h index 72a60ecb9159..2b2b1e86035a 100644 --- a/src/qcam/viewfinder_gl.h +++ b/src/qcam/viewfinder_gl.h @@ -38,7 +38,8 @@ public: const QList<libcamera::PixelFormat> &nativeFormats() const override; - int setFormat(const libcamera::PixelFormat &format, const QSize &size) override; + int setFormat(const libcamera::PixelFormat &format, const QSize &size, + unsigned int stride) override; void render(libcamera::FrameBuffer *buffer, Image *image) override; void stop() override; diff --git a/src/qcam/viewfinder_qt.cpp b/src/qcam/viewfinder_qt.cpp index 0d357d860014..cd051760160c 100644 --- a/src/qcam/viewfinder_qt.cpp +++ b/src/qcam/viewfinder_qt.cpp @@ -52,7 +52,8 @@ const QList<libcamera::PixelFormat> &ViewFinderQt::nativeFormats() const } int ViewFinderQt::setFormat(const libcamera::PixelFormat &format, - const QSize &size) + const QSize &size, + [[maybe_unused]] unsigned int stride) { image_ = QImage(); diff --git a/src/qcam/viewfinder_qt.h b/src/qcam/viewfinder_qt.h index 6b48ef48a7d1..756f3fa33055 100644 --- a/src/qcam/viewfinder_qt.h +++ b/src/qcam/viewfinder_qt.h @@ -31,7 +31,8 @@ public: const QList<libcamera::PixelFormat> &nativeFormats() const override; - int setFormat(const libcamera::PixelFormat &format, const QSize &size) override; + int setFormat(const libcamera::PixelFormat &format, const QSize &size, + unsigned int stride) override; void render(libcamera::FrameBuffer *buffer, Image *image) override; void stop() override;
qcam currently assumes that no padding is used at end of lines, and uses the image width as the stride. This leads to rendering failures with some formats on some platforms. To prepare for stride support, add a stride parameter to the ViewFinder::setFormat() function to pass the stride from the stream configuration to the viewfinder. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- src/qcam/main_window.cpp | 3 ++- src/qcam/viewfinder.h | 3 ++- src/qcam/viewfinder_gl.cpp | 7 ++----- src/qcam/viewfinder_gl.h | 3 ++- src/qcam/viewfinder_qt.cpp | 3 ++- src/qcam/viewfinder_qt.h | 3 ++- 6 files changed, 12 insertions(+), 10 deletions(-)