[{"id":19742,"web_url":"https://patchwork.libcamera.org/comment/19742/","msgid":"<e6474433-ecca-4b38-db83-1e99505b512f@ideasonboard.com>","date":"2021-09-21T14:43:12","subject":"Re: [libcamera-devel] [PATCH v2 1/5] qcam: viewfinder: Pass stride\n\tvalue to viewfinder","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 07/09/2021 01:20, Laurent Pinchart wrote:\n> qcam currently assumes that no padding is used at end of lines, and uses\n> the image width as the stride. This leads to rendering failures with\n> some formats on some platforms. To prepare for stride support, add a\n> stride parameter to the ViewFinder::setFormat() function to pass the\n> stride from the stream configuration to the viewfinder.\n>\n> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> ---\n>  src/qcam/main_window.cpp   | 3 ++-\n>  src/qcam/viewfinder.h      | 3 ++-\n>  src/qcam/viewfinder_gl.cpp | 7 ++-----\n>  src/qcam/viewfinder_gl.h   | 3 ++-\n>  src/qcam/viewfinder_qt.cpp | 3 ++-\n>  src/qcam/viewfinder_qt.h   | 3 ++-\n>  6 files changed, 12 insertions(+), 10 deletions(-)\n> \n> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp\n> index 168dd5ce30e3..bb6b03993add 100644\n> --- a/src/qcam/main_window.cpp\n> +++ b/src/qcam/main_window.cpp\n> @@ -448,7 +448,8 @@ int MainWindow::startCapture()\n>  \n>  \t/* Configure the viewfinder. */\n>  \tret = viewfinder_->setFormat(vfConfig.pixelFormat,\n> -\t\t\t\t     QSize(vfConfig.size.width, vfConfig.size.height));\n> +\t\t\t\t     QSize(vfConfig.size.width, vfConfig.size.height),\n> +\t\t\t\t     vfConfig.stride);\n>  \tif (ret < 0) {\n>  \t\tqInfo() << \"Failed to set viewfinder format\";\n>  \t\treturn ret;\n> diff --git a/src/qcam/viewfinder.h b/src/qcam/viewfinder.h\n> index fb462835fb5f..4c2102a6ed04 100644\n> --- a/src/qcam/viewfinder.h\n> +++ b/src/qcam/viewfinder.h\n> @@ -23,7 +23,8 @@ public:\n>  \n>  \tvirtual const QList<libcamera::PixelFormat> &nativeFormats() const = 0;\n>  \n> -\tvirtual int setFormat(const libcamera::PixelFormat &format, const QSize &size) = 0;\n> +\tvirtual int setFormat(const libcamera::PixelFormat &format, const QSize &size,\n> +\t\t\t      unsigned int stride) = 0;\n>  \tvirtual void render(libcamera::FrameBuffer *buffer, Image *image) = 0;\n>  \tvirtual void stop() = 0;\n>  \n> diff --git a/src/qcam/viewfinder_gl.cpp b/src/qcam/viewfinder_gl.cpp\n> index 32232faa2ad8..aeb1ea02d2d5 100644\n> --- a/src/qcam/viewfinder_gl.cpp\n> +++ b/src/qcam/viewfinder_gl.cpp\n> @@ -72,7 +72,7 @@ const QList<libcamera::PixelFormat> &ViewFinderGL::nativeFormats() const\n>  }\n>  \n>  int ViewFinderGL::setFormat(const libcamera::PixelFormat &format,\n> -\t\t\t    const QSize &size)\n> +\t\t\t    const QSize &size, unsigned int stride)\n>  {\n>  \tif (format != format_) {\n>  \t\t/*\n> @@ -92,6 +92,7 @@ int ViewFinderGL::setFormat(const libcamera::PixelFormat &format,\n>  \t}\n>  \n>  \tsize_ = size;\n> +\tstride_ = stride;\n>  \n>  \tupdateGeometry();\n>  \treturn 0;\n> @@ -119,10 +120,6 @@ void ViewFinderGL::render(libcamera::FrameBuffer *buffer, Image *image)\n>  \t\trenderComplete(buffer_);\n>  \n>  \timage_ = image;\n> -\t/*\n> -\t * \\todo Get the stride from the buffer instead of computing it naively\n> -\t */\n\nWe're getting it from the StreamConfiguration instead of the\nPixelFormatInfo, or the buffer ... Are they (sufficiently) equivalent?\n\nThe documentation suggests subtle differences.\n\nIs this patch 'all' that is required to ensure the strides are passed\ncorrectly, or do we need to keep/add a todo to make it clear that there\nis further work up at \"Configure the viewfinder\" to handle plane\nspecific strides?\n\nI guess that would become evident if StreamConfiguration::stride became\na span of strides... so perhaps the compiler will act as a todo if that\never happens.\n\n\nI don't want to complain too much though..\n\nReviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n\n\n> -\tstride_ = buffer->metadata().planes()[0].bytesused / size_.height();\n>  \tupdate();\n>  \tbuffer_ = buffer;\n>  }\n> diff --git a/src/qcam/viewfinder_gl.h b/src/qcam/viewfinder_gl.h\n> index 72a60ecb9159..2b2b1e86035a 100644\n> --- a/src/qcam/viewfinder_gl.h\n> +++ b/src/qcam/viewfinder_gl.h\n> @@ -38,7 +38,8 @@ public:\n>  \n>  \tconst QList<libcamera::PixelFormat> &nativeFormats() const override;\n>  \n> -\tint setFormat(const libcamera::PixelFormat &format, const QSize &size) override;\n> +\tint setFormat(const libcamera::PixelFormat &format, const QSize &size,\n> +\t\t      unsigned int stride) override;\n>  \tvoid render(libcamera::FrameBuffer *buffer, Image *image) override;\n>  \tvoid stop() override;\n>  \n> diff --git a/src/qcam/viewfinder_qt.cpp b/src/qcam/viewfinder_qt.cpp\n> index 0d357d860014..cd051760160c 100644\n> --- a/src/qcam/viewfinder_qt.cpp\n> +++ b/src/qcam/viewfinder_qt.cpp\n> @@ -52,7 +52,8 @@ const QList<libcamera::PixelFormat> &ViewFinderQt::nativeFormats() const\n>  }\n>  \n>  int ViewFinderQt::setFormat(const libcamera::PixelFormat &format,\n> -\t\t\t    const QSize &size)\n> +\t\t\t    const QSize &size,\n> +\t\t\t    [[maybe_unused]] unsigned int stride)\n>  {\n>  \timage_ = QImage();\n>  \n> diff --git a/src/qcam/viewfinder_qt.h b/src/qcam/viewfinder_qt.h\n> index 6b48ef48a7d1..756f3fa33055 100644\n> --- a/src/qcam/viewfinder_qt.h\n> +++ b/src/qcam/viewfinder_qt.h\n> @@ -31,7 +31,8 @@ public:\n>  \n>  \tconst QList<libcamera::PixelFormat> &nativeFormats() const override;\n>  \n> -\tint setFormat(const libcamera::PixelFormat &format, const QSize &size) override;\n> +\tint setFormat(const libcamera::PixelFormat &format, const QSize &size,\n> +\t\t      unsigned int stride) override;\n>  \tvoid render(libcamera::FrameBuffer *buffer, Image *image) override;\n>  \tvoid stop() override;\n>  \n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 06C8ABDC71\n\tfor <parsemail@patchwork.libcamera.org>;\n\tTue, 21 Sep 2021 14:43:21 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4CFD86918C;\n\tTue, 21 Sep 2021 16:43:20 +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 DFA9460247\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 21 Sep 2021 16:43:17 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 58D0F2BA;\n\tTue, 21 Sep 2021 16:43:17 +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=\"DyA2rSnF\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1632235397;\n\tbh=khtMvPcLGuAppBtzh1eHtKGlkji5a01dw5tFr/zz7xY=;\n\th=To:References:From:Subject:Date:In-Reply-To:From;\n\tb=DyA2rSnFha1kpwcdKdIJxNwwenSHtu57yfb6GoL6rTPSxB8jWt9PkaEa3ecuEoRa5\n\txc/yLhm+V0HT2854KuN1ODbVU0po4mWGW8ywcJN2XvSP7oZOs1l0/bkxC1x+W5qwvU\n\tm2uN9WudFM9GMXzvXDWpsHSplLJJGMzPaT68rGOo=","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210907002044.7319-1-laurent.pinchart@ideasonboard.com>\n\t<20210907002044.7319-2-laurent.pinchart@ideasonboard.com>","From":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Message-ID":"<e6474433-ecca-4b38-db83-1e99505b512f@ideasonboard.com>","Date":"Tue, 21 Sep 2021 15:43:12 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.11.0","MIME-Version":"1.0","In-Reply-To":"<20210907002044.7319-2-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 1/5] qcam: viewfinder: Pass stride\n\tvalue to viewfinder","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":19750,"web_url":"https://patchwork.libcamera.org/comment/19750/","msgid":"<YUoRYJwImQ53Oikz@pendragon.ideasonboard.com>","date":"2021-09-21T17:07:44","subject":"Re: [libcamera-devel] [PATCH v2 1/5] qcam: viewfinder: Pass stride\n\tvalue to viewfinder","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, Sep 21, 2021 at 03:43:12PM +0100, Kieran Bingham wrote:\n> On 07/09/2021 01:20, Laurent Pinchart wrote:\n> > qcam currently assumes that no padding is used at end of lines, and uses\n> > the image width as the stride. This leads to rendering failures with\n> > some formats on some platforms. To prepare for stride support, add a\n> > stride parameter to the ViewFinder::setFormat() function to pass the\n> > stride from the stream configuration to the viewfinder.\n> >\n> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> > ---\n> >  src/qcam/main_window.cpp   | 3 ++-\n> >  src/qcam/viewfinder.h      | 3 ++-\n> >  src/qcam/viewfinder_gl.cpp | 7 ++-----\n> >  src/qcam/viewfinder_gl.h   | 3 ++-\n> >  src/qcam/viewfinder_qt.cpp | 3 ++-\n> >  src/qcam/viewfinder_qt.h   | 3 ++-\n> >  6 files changed, 12 insertions(+), 10 deletions(-)\n> > \n> > diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp\n> > index 168dd5ce30e3..bb6b03993add 100644\n> > --- a/src/qcam/main_window.cpp\n> > +++ b/src/qcam/main_window.cpp\n> > @@ -448,7 +448,8 @@ int MainWindow::startCapture()\n> >  \n> >  \t/* Configure the viewfinder. */\n> >  \tret = viewfinder_->setFormat(vfConfig.pixelFormat,\n> > -\t\t\t\t     QSize(vfConfig.size.width, vfConfig.size.height));\n> > +\t\t\t\t     QSize(vfConfig.size.width, vfConfig.size.height),\n> > +\t\t\t\t     vfConfig.stride);\n> >  \tif (ret < 0) {\n> >  \t\tqInfo() << \"Failed to set viewfinder format\";\n> >  \t\treturn ret;\n> > diff --git a/src/qcam/viewfinder.h b/src/qcam/viewfinder.h\n> > index fb462835fb5f..4c2102a6ed04 100644\n> > --- a/src/qcam/viewfinder.h\n> > +++ b/src/qcam/viewfinder.h\n> > @@ -23,7 +23,8 @@ public:\n> >  \n> >  \tvirtual const QList<libcamera::PixelFormat> &nativeFormats() const = 0;\n> >  \n> > -\tvirtual int setFormat(const libcamera::PixelFormat &format, const QSize &size) = 0;\n> > +\tvirtual int setFormat(const libcamera::PixelFormat &format, const QSize &size,\n> > +\t\t\t      unsigned int stride) = 0;\n> >  \tvirtual void render(libcamera::FrameBuffer *buffer, Image *image) = 0;\n> >  \tvirtual void stop() = 0;\n> >  \n> > diff --git a/src/qcam/viewfinder_gl.cpp b/src/qcam/viewfinder_gl.cpp\n> > index 32232faa2ad8..aeb1ea02d2d5 100644\n> > --- a/src/qcam/viewfinder_gl.cpp\n> > +++ b/src/qcam/viewfinder_gl.cpp\n> > @@ -72,7 +72,7 @@ const QList<libcamera::PixelFormat> &ViewFinderGL::nativeFormats() const\n> >  }\n> >  \n> >  int ViewFinderGL::setFormat(const libcamera::PixelFormat &format,\n> > -\t\t\t    const QSize &size)\n> > +\t\t\t    const QSize &size, unsigned int stride)\n> >  {\n> >  \tif (format != format_) {\n> >  \t\t/*\n> > @@ -92,6 +92,7 @@ int ViewFinderGL::setFormat(const libcamera::PixelFormat &format,\n> >  \t}\n> >  \n> >  \tsize_ = size;\n> > +\tstride_ = stride;\n> >  \n> >  \tupdateGeometry();\n> >  \treturn 0;\n> > @@ -119,10 +120,6 @@ void ViewFinderGL::render(libcamera::FrameBuffer *buffer, Image *image)\n> >  \t\trenderComplete(buffer_);\n> >  \n> >  \timage_ = image;\n> > -\t/*\n> > -\t * \\todo Get the stride from the buffer instead of computing it naively\n> > -\t */\n> \n> We're getting it from the StreamConfiguration instead of the\n> PixelFormatInfo, or the buffer ... Are they (sufficiently) equivalent?\n\nMentioning \"buffer\" here was likely a mistake. Our current model sets\nthe stride at configuration time, and it's then considered to be fixed\nfor the duration of the session. The stride from the pixel format is\ndifferent, as a pixel format can only provide information about possible\nstrides, while the stream configuration contains the actual stride.\n\n> The documentation suggests subtle differences.\n> \n> Is this patch 'all' that is required to ensure the strides are passed\n> correctly, or do we need to keep/add a todo to make it clear that there\n> is further work up at \"Configure the viewfinder\" to handle plane\n> specific strides?\n\nThat's all there is to handle the stride correctly with the current API.\nWe may need to negotiate strides between the source and sink at some\npoint, in case the sink can't work with the default stride from the\nsource, but that's not covered by this \\todo anyway.\n\n> I guess that would become evident if StreamConfiguration::stride became\n> a span of strides... so perhaps the compiler will act as a todo if that\n> ever happens.\n\nYes, if the API changes, then the compiler will let us know :-) I'm not\nentirely sure how to handle that though, as we could support per-plane\nstrides, but most devices won't support that.\n\n> I don't want to complain too much though..\n> \n> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> \n> > -\tstride_ = buffer->metadata().planes()[0].bytesused / size_.height();\n> >  \tupdate();\n> >  \tbuffer_ = buffer;\n> >  }\n> > diff --git a/src/qcam/viewfinder_gl.h b/src/qcam/viewfinder_gl.h\n> > index 72a60ecb9159..2b2b1e86035a 100644\n> > --- a/src/qcam/viewfinder_gl.h\n> > +++ b/src/qcam/viewfinder_gl.h\n> > @@ -38,7 +38,8 @@ public:\n> >  \n> >  \tconst QList<libcamera::PixelFormat> &nativeFormats() const override;\n> >  \n> > -\tint setFormat(const libcamera::PixelFormat &format, const QSize &size) override;\n> > +\tint setFormat(const libcamera::PixelFormat &format, const QSize &size,\n> > +\t\t      unsigned int stride) override;\n> >  \tvoid render(libcamera::FrameBuffer *buffer, Image *image) override;\n> >  \tvoid stop() override;\n> >  \n> > diff --git a/src/qcam/viewfinder_qt.cpp b/src/qcam/viewfinder_qt.cpp\n> > index 0d357d860014..cd051760160c 100644\n> > --- a/src/qcam/viewfinder_qt.cpp\n> > +++ b/src/qcam/viewfinder_qt.cpp\n> > @@ -52,7 +52,8 @@ const QList<libcamera::PixelFormat> &ViewFinderQt::nativeFormats() const\n> >  }\n> >  \n> >  int ViewFinderQt::setFormat(const libcamera::PixelFormat &format,\n> > -\t\t\t    const QSize &size)\n> > +\t\t\t    const QSize &size,\n> > +\t\t\t    [[maybe_unused]] unsigned int stride)\n> >  {\n> >  \timage_ = QImage();\n> >  \n> > diff --git a/src/qcam/viewfinder_qt.h b/src/qcam/viewfinder_qt.h\n> > index 6b48ef48a7d1..756f3fa33055 100644\n> > --- a/src/qcam/viewfinder_qt.h\n> > +++ b/src/qcam/viewfinder_qt.h\n> > @@ -31,7 +31,8 @@ public:\n> >  \n> >  \tconst QList<libcamera::PixelFormat> &nativeFormats() const override;\n> >  \n> > -\tint setFormat(const libcamera::PixelFormat &format, const QSize &size) override;\n> > +\tint setFormat(const libcamera::PixelFormat &format, const QSize &size,\n> > +\t\t      unsigned int stride) override;\n> >  \tvoid render(libcamera::FrameBuffer *buffer, Image *image) override;\n> >  \tvoid stop() override;\n> >","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 719EABF01C\n\tfor <parsemail@patchwork.libcamera.org>;\n\tTue, 21 Sep 2021 17:08:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D0D556918C;\n\tTue, 21 Sep 2021 19:08:16 +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 A13CE60247\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 21 Sep 2021 19:08:15 +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 18B4E2BA;\n\tTue, 21 Sep 2021 19:08:15 +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=\"WpvT1+El\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1632244095;\n\tbh=Lj64n2u9Tl2h/RjSF3O5YKPjRJ3WdGoYmfuyCKbDMaY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=WpvT1+ElMOZSqzkD3R+CGLYSZw8wcP4aeKe6JXs41EZAsmCXEuga/faGdXRvs/lXZ\n\tQb9MqYYU5tj4cNRpAz1Z32n0ErVpFXLA7uYIt7GRSyWQLDkqbnFY5yxNZfA0Q0cYAu\n\tUjz7D66BJvs34Lp1ppcQALSlVShPC54G+8zR+VPo=","Date":"Tue, 21 Sep 2021 20:07:44 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Message-ID":"<YUoRYJwImQ53Oikz@pendragon.ideasonboard.com>","References":"<20210907002044.7319-1-laurent.pinchart@ideasonboard.com>\n\t<20210907002044.7319-2-laurent.pinchart@ideasonboard.com>\n\t<e6474433-ecca-4b38-db83-1e99505b512f@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<e6474433-ecca-4b38-db83-1e99505b512f@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 1/5] qcam: viewfinder: Pass stride\n\tvalue to viewfinder","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":19772,"web_url":"https://patchwork.libcamera.org/comment/19772/","msgid":"<20210922064433.GG4382@pyrite.rasen.tech>","date":"2021-09-22T06:44:33","subject":"Re: [libcamera-devel] [PATCH v2 1/5] qcam: viewfinder: Pass stride\n\tvalue to viewfinder","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Laurent,\n\nOn Tue, Sep 07, 2021 at 03:20:40AM +0300, Laurent Pinchart wrote:\n> qcam currently assumes that no padding is used at end of lines, and uses\n> the image width as the stride. This leads to rendering failures with\n> some formats on some platforms. To prepare for stride support, add a\n> stride parameter to the ViewFinder::setFormat() function to pass the\n> stride from the stream configuration to the viewfinder.\n> \n> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n\nReviewed-by: Paul Elder <paul.elder@ideasonboard.com>\n\n> ---\n>  src/qcam/main_window.cpp   | 3 ++-\n>  src/qcam/viewfinder.h      | 3 ++-\n>  src/qcam/viewfinder_gl.cpp | 7 ++-----\n>  src/qcam/viewfinder_gl.h   | 3 ++-\n>  src/qcam/viewfinder_qt.cpp | 3 ++-\n>  src/qcam/viewfinder_qt.h   | 3 ++-\n>  6 files changed, 12 insertions(+), 10 deletions(-)\n> \n> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp\n> index 168dd5ce30e3..bb6b03993add 100644\n> --- a/src/qcam/main_window.cpp\n> +++ b/src/qcam/main_window.cpp\n> @@ -448,7 +448,8 @@ int MainWindow::startCapture()\n>  \n>  \t/* Configure the viewfinder. */\n>  \tret = viewfinder_->setFormat(vfConfig.pixelFormat,\n> -\t\t\t\t     QSize(vfConfig.size.width, vfConfig.size.height));\n> +\t\t\t\t     QSize(vfConfig.size.width, vfConfig.size.height),\n> +\t\t\t\t     vfConfig.stride);\n>  \tif (ret < 0) {\n>  \t\tqInfo() << \"Failed to set viewfinder format\";\n>  \t\treturn ret;\n> diff --git a/src/qcam/viewfinder.h b/src/qcam/viewfinder.h\n> index fb462835fb5f..4c2102a6ed04 100644\n> --- a/src/qcam/viewfinder.h\n> +++ b/src/qcam/viewfinder.h\n> @@ -23,7 +23,8 @@ public:\n>  \n>  \tvirtual const QList<libcamera::PixelFormat> &nativeFormats() const = 0;\n>  \n> -\tvirtual int setFormat(const libcamera::PixelFormat &format, const QSize &size) = 0;\n> +\tvirtual int setFormat(const libcamera::PixelFormat &format, const QSize &size,\n> +\t\t\t      unsigned int stride) = 0;\n>  \tvirtual void render(libcamera::FrameBuffer *buffer, Image *image) = 0;\n>  \tvirtual void stop() = 0;\n>  \n> diff --git a/src/qcam/viewfinder_gl.cpp b/src/qcam/viewfinder_gl.cpp\n> index 32232faa2ad8..aeb1ea02d2d5 100644\n> --- a/src/qcam/viewfinder_gl.cpp\n> +++ b/src/qcam/viewfinder_gl.cpp\n> @@ -72,7 +72,7 @@ const QList<libcamera::PixelFormat> &ViewFinderGL::nativeFormats() const\n>  }\n>  \n>  int ViewFinderGL::setFormat(const libcamera::PixelFormat &format,\n> -\t\t\t    const QSize &size)\n> +\t\t\t    const QSize &size, unsigned int stride)\n>  {\n>  \tif (format != format_) {\n>  \t\t/*\n> @@ -92,6 +92,7 @@ int ViewFinderGL::setFormat(const libcamera::PixelFormat &format,\n>  \t}\n>  \n>  \tsize_ = size;\n> +\tstride_ = stride;\n>  \n>  \tupdateGeometry();\n>  \treturn 0;\n> @@ -119,10 +120,6 @@ void ViewFinderGL::render(libcamera::FrameBuffer *buffer, Image *image)\n>  \t\trenderComplete(buffer_);\n>  \n>  \timage_ = image;\n> -\t/*\n> -\t * \\todo Get the stride from the buffer instead of computing it naively\n> -\t */\n> -\tstride_ = buffer->metadata().planes()[0].bytesused / size_.height();\n>  \tupdate();\n>  \tbuffer_ = buffer;\n>  }\n> diff --git a/src/qcam/viewfinder_gl.h b/src/qcam/viewfinder_gl.h\n> index 72a60ecb9159..2b2b1e86035a 100644\n> --- a/src/qcam/viewfinder_gl.h\n> +++ b/src/qcam/viewfinder_gl.h\n> @@ -38,7 +38,8 @@ public:\n>  \n>  \tconst QList<libcamera::PixelFormat> &nativeFormats() const override;\n>  \n> -\tint setFormat(const libcamera::PixelFormat &format, const QSize &size) override;\n> +\tint setFormat(const libcamera::PixelFormat &format, const QSize &size,\n> +\t\t      unsigned int stride) override;\n>  \tvoid render(libcamera::FrameBuffer *buffer, Image *image) override;\n>  \tvoid stop() override;\n>  \n> diff --git a/src/qcam/viewfinder_qt.cpp b/src/qcam/viewfinder_qt.cpp\n> index 0d357d860014..cd051760160c 100644\n> --- a/src/qcam/viewfinder_qt.cpp\n> +++ b/src/qcam/viewfinder_qt.cpp\n> @@ -52,7 +52,8 @@ const QList<libcamera::PixelFormat> &ViewFinderQt::nativeFormats() const\n>  }\n>  \n>  int ViewFinderQt::setFormat(const libcamera::PixelFormat &format,\n> -\t\t\t    const QSize &size)\n> +\t\t\t    const QSize &size,\n> +\t\t\t    [[maybe_unused]] unsigned int stride)\n>  {\n>  \timage_ = QImage();\n>  \n> diff --git a/src/qcam/viewfinder_qt.h b/src/qcam/viewfinder_qt.h\n> index 6b48ef48a7d1..756f3fa33055 100644\n> --- a/src/qcam/viewfinder_qt.h\n> +++ b/src/qcam/viewfinder_qt.h\n> @@ -31,7 +31,8 @@ public:\n>  \n>  \tconst QList<libcamera::PixelFormat> &nativeFormats() const override;\n>  \n> -\tint setFormat(const libcamera::PixelFormat &format, const QSize &size) override;\n> +\tint setFormat(const libcamera::PixelFormat &format, const QSize &size,\n> +\t\t      unsigned int stride) override;\n>  \tvoid render(libcamera::FrameBuffer *buffer, Image *image) override;\n>  \tvoid stop() override;\n>  \n> -- \n> Regards,\n> \n> Laurent Pinchart\n>","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 86D29BDC71\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed, 22 Sep 2021 06:44:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 039DE6918C;\n\tWed, 22 Sep 2021 08:44:42 +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 467596012C\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 22 Sep 2021 08:44:41 +0200 (CEST)","from pyrite.rasen.tech (unknown\n\t[IPv6:2400:4051:61:600:2c71:1b79:d06d:5032])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B2C7DF1;\n\tWed, 22 Sep 2021 08:44:39 +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=\"YcpwcguM\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1632293080;\n\tbh=MAVeV5aEOLDUrAPs1WC2EOIk50in21IXIcADISqaKDE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=YcpwcguMO9EZLDb9IEkc/0xd7dSm9R0Czr//6dEESm1KNSAy3hjW/Yi4nNna1VEvS\n\tFWtGIimU8DgIxbkw4uWaw9igMiEwF1uUt1/WaiknRhBg2DEAZCRRyFzVdVQOpR4PCs\n\t9dhS1ey3sKJjgyKp9q1owfiJPyV87oDa+No38uBE=","Date":"Wed, 22 Sep 2021 15:44:33 +0900","From":"paul.elder@ideasonboard.com","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Message-ID":"<20210922064433.GG4382@pyrite.rasen.tech>","References":"<20210907002044.7319-1-laurent.pinchart@ideasonboard.com>\n\t<20210907002044.7319-2-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20210907002044.7319-2-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 1/5] qcam: viewfinder: Pass stride\n\tvalue to viewfinder","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]