[{"id":19401,"web_url":"https://patchwork.libcamera.org/comment/19401/","msgid":"","date":"2021-09-06T07:37:23","subject":"Re: [libcamera-devel] [PATCH v2 17/27] libcamera: framebuffer:\n\tPrevent modifying the number of metadata planes","submitter":{"id":75,"url":"https://patchwork.libcamera.org/api/people/75/","name":"Jean-Michel Hautbois","email":"jeanmichel.hautbois@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 06/09/2021 04:00, Laurent Pinchart wrote:\n> The number of metadata planes should always match the number of frame\n> buffer planes. Enforce this by making the vector private and providing\n> accessor functions.\n> \n> As this changes the public API, update all in-tree users accordingly.\n> \n> Signed-off-by: Laurent Pinchart \n> Reviewed-by: Kieran Bingham \nReviewed-by: Jean-Michel Hautbois \n> ---\n> include/libcamera/framebuffer.h | 10 +++++++++-\n> src/cam/camera_session.cpp | 4 ++--\n> src/cam/file_sink.cpp | 2 +-\n> src/libcamera/framebuffer.cpp | 12 +++++++++---\n> src/libcamera/v4l2_videodevice.cpp | 14 +++++++-------\n> src/qcam/main_window.cpp | 2 +-\n> src/qcam/viewfinder_gl.cpp | 2 +-\n> src/qcam/viewfinder_qt.cpp | 2 +-\n> src/v4l2/v4l2_camera_proxy.cpp | 2 +-\n> 9 files changed, 32 insertions(+), 18 deletions(-)\n> \n> diff --git a/include/libcamera/framebuffer.h b/include/libcamera/framebuffer.h\n> index fd68ed0a139d..7f2f176af691 100644\n> --- a/include/libcamera/framebuffer.h\n> +++ b/include/libcamera/framebuffer.h\n> @@ -13,6 +13,7 @@\n> #include \n> \n> #include \n> +#include \n> \n> #include \n> \n> @@ -34,7 +35,14 @@ struct FrameMetadata {\n> \tStatus status;\n> \tunsigned int sequence;\n> \tuint64_t timestamp;\n> -\tstd::vector planes;\n> +\n> +\tSpan planes() { return planes_; }\n> +\tSpan planes() const { return planes_; }\n> +\n> +private:\n> +\tfriend class FrameBuffer;\n> +\n> +\tstd::vector planes_;\n> };\n> \n> class FrameBuffer final : public Extensible\n> diff --git a/src/cam/camera_session.cpp b/src/cam/camera_session.cpp\n> index 60d640f2b15c..32a373a99b72 100644\n> --- a/src/cam/camera_session.cpp\n> +++ b/src/cam/camera_session.cpp\n> @@ -374,9 +374,9 @@ void CameraSession::processRequest(Request *request)\n> \t\t << \" bytesused: \";\n> \n> \t\tunsigned int nplane = 0;\n> -\t\tfor (const FrameMetadata::Plane &plane : metadata.planes) {\n> +\t\tfor (const FrameMetadata::Plane &plane : metadata.planes()) {\n> \t\t\tinfo << plane.bytesused;\n> -\t\t\tif (++nplane < metadata.planes.size())\n> +\t\t\tif (++nplane < metadata.planes().size())\n> \t\t\t\tinfo << \"/\";\n> \t\t}\n> \t}\n> diff --git a/src/cam/file_sink.cpp b/src/cam/file_sink.cpp\n> index 0b529e3eb767..0fc7d621f50b 100644\n> --- a/src/cam/file_sink.cpp\n> +++ b/src/cam/file_sink.cpp\n> @@ -110,7 +110,7 @@ void FileSink::writeBuffer(const Stream *stream, FrameBuffer *buffer)\n> \n> \tfor (unsigned int i = 0; i < buffer->planes().size(); ++i) {\n> \t\tconst FrameBuffer::Plane &plane = buffer->planes()[i];\n> -\t\tconst FrameMetadata::Plane &meta = buffer->metadata().planes[i];\n> +\t\tconst FrameMetadata::Plane &meta = buffer->metadata().planes()[i];\n> \n> \t\tuint8_t *data = planeData_[&plane];\n> \t\tunsigned int length = std::min(meta.bytesused, plane.length);\n> diff --git a/src/libcamera/framebuffer.cpp b/src/libcamera/framebuffer.cpp\n> index e4f8419a9063..d44a98babd05 100644\n> --- a/src/libcamera/framebuffer.cpp\n> +++ b/src/libcamera/framebuffer.cpp\n> @@ -91,8 +91,14 @@ LOG_DEFINE_CATEGORY(Buffer)\n> */\n> \n> /**\n> - * \\var FrameMetadata::planes\n> - * \\brief Array of per-plane metadata\n> + * \\fn FrameMetadata::planes()\n> + * \\copydoc FrameMetadata::planes() const\n> + */\n> +\n> +/**\n> + * \\fn FrameMetadata::planes() const\n> + * \\brief Retrieve the array of per-plane metadata\n> + * \\return The array of per-plane metadata\n> */\n> \n> /**\n> @@ -210,7 +216,7 @@ FrameBuffer::FrameBuffer(const std::vector &planes, unsigned int cookie)\n> \t: Extensible(std::make_unique()), planes_(planes),\n> \t cookie_(cookie)\n> {\n> -\tmetadata_.planes.resize(planes_.size());\n> +\tmetadata_.planes_.resize(planes_.size());\n> \n> \tunsigned int offset = 0;\n> \tbool isContiguous = true;\n> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> index e729e608448c..296304fbd979 100644\n> --- a/src/libcamera/v4l2_videodevice.cpp\n> +++ b/src/libcamera/v4l2_videodevice.cpp\n> @@ -1538,7 +1538,7 @@ int V4L2VideoDevice::queueBuffer(FrameBuffer *buffer)\n> \t\t\tunsigned int length = 0;\n> \n> \t\t\tfor (auto [i, plane] : utils::enumerate(planes)) {\n> -\t\t\t\tbytesused += metadata.planes[i].bytesused;\n> +\t\t\t\tbytesused += metadata.planes()[i].bytesused;\n> \t\t\t\tlength += plane.length;\n> \n> \t\t\t\tif (i != planes.size() - 1 && bytesused != length) {\n> @@ -1562,7 +1562,7 @@ int V4L2VideoDevice::queueBuffer(FrameBuffer *buffer)\n> \t\t\t * V4L2 buffer is guaranteed to be equal at this point.\n> \t\t\t */\n> \t\t\tfor (auto [i, plane] : utils::enumerate(planes)) {\n> -\t\t\t\tv4l2Planes[i].bytesused = metadata.planes[i].bytesused;\n> +\t\t\t\tv4l2Planes[i].bytesused = metadata.planes()[i].bytesused;\n> \t\t\t\tv4l2Planes[i].length = plane.length;\n> \t\t\t}\n> \t\t} else {\n> @@ -1570,7 +1570,7 @@ int V4L2VideoDevice::queueBuffer(FrameBuffer *buffer)\n> \t\t\t * Single-planar API with a single plane in the buffer\n> \t\t\t * is trivial to handle.\n> \t\t\t */\n> -\t\t\tbuf.bytesused = metadata.planes[0].bytesused;\n> +\t\t\tbuf.bytesused = metadata.planes()[0].bytesused;\n> \t\t\tbuf.length = planes[0].length;\n> \t\t}\n> \n> @@ -1699,9 +1699,9 @@ FrameBuffer *V4L2VideoDevice::dequeueBuffer()\n> \t\t\t\treturn buffer;\n> \t\t\t}\n> \n> -\t\t\tmetadata.planes[i].bytesused =\n> +\t\t\tmetadata.planes()[i].bytesused =\n> \t\t\t\tstd::min(plane.length, bytesused);\n> -\t\t\tbytesused -= metadata.planes[i].bytesused;\n> +\t\t\tbytesused -= metadata.planes()[i].bytesused;\n> \t\t}\n> \t} else if (multiPlanar) {\n> \t\t/*\n> @@ -1710,9 +1710,9 @@ FrameBuffer *V4L2VideoDevice::dequeueBuffer()\n> \t\t * V4L2 buffer is guaranteed to be equal at this point.\n> \t\t */\n> \t\tfor (unsigned int i = 0; i < numV4l2Planes; ++i)\n> -\t\t\tmetadata.planes[i].bytesused = planes[i].bytesused;\n> +\t\t\tmetadata.planes()[i].bytesused = planes[i].bytesused;\n> \t} else {\n> -\t\tmetadata.planes[0].bytesused = buf.bytesused;\n> +\t\tmetadata.planes()[0].bytesused = buf.bytesused;\n> \t}\n> \n> \treturn buffer;\n> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp\n> index 1536b2b5bd66..ac853e360aea 100644\n> --- a/src/qcam/main_window.cpp\n> +++ b/src/qcam/main_window.cpp\n> @@ -756,7 +756,7 @@ void MainWindow::processViewfinder(FrameBuffer *buffer)\n> \n> \tqDebug().noquote()\n> \t\t<< QString(\"seq: %1\").arg(metadata.sequence, 6, 10, QLatin1Char('0'))\n> -\t\t<< \"bytesused:\" << metadata.planes[0].bytesused\n> +\t\t<< \"bytesused:\" << metadata.planes()[0].bytesused\n> \t\t<< \"timestamp:\" << metadata.timestamp\n> \t\t<< \"fps:\" << Qt::fixed << qSetRealNumberPrecision(2) << fps;\n> \n> diff --git a/src/qcam/viewfinder_gl.cpp b/src/qcam/viewfinder_gl.cpp\n> index 40226601f9fd..d2ef036974f4 100644\n> --- a/src/qcam/viewfinder_gl.cpp\n> +++ b/src/qcam/viewfinder_gl.cpp\n> @@ -125,7 +125,7 @@ void ViewFinderGL::render(libcamera::FrameBuffer *buffer,\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> +\tstride_ = buffer->metadata().planes()[0].bytesused / size_.height();\n> \tupdate();\n> \tbuffer_ = buffer;\n> }\n> diff --git a/src/qcam/viewfinder_qt.cpp b/src/qcam/viewfinder_qt.cpp\n> index efa1d412584b..a0bf99b0b522 100644\n> --- a/src/qcam/viewfinder_qt.cpp\n> +++ b/src/qcam/viewfinder_qt.cpp\n> @@ -87,7 +87,7 @@ void ViewFinderQt::render(libcamera::FrameBuffer *buffer,\n> \t}\n> \n> \tunsigned char *memory = mem.data();\n> -\tsize_t size = buffer->metadata().planes[0].bytesused;\n> +\tsize_t size = buffer->metadata().planes()[0].bytesused;\n> \n> \t{\n> \t\tQMutexLocker locker(&mutex_);\n> diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp\n> index d926a7b77083..07d2250bb846 100644\n> --- a/src/v4l2/v4l2_camera_proxy.cpp\n> +++ b/src/v4l2/v4l2_camera_proxy.cpp\n> @@ -211,7 +211,7 @@ void V4L2CameraProxy::updateBuffers()\n> \n> \t\tswitch (fmd.status) {\n> \t\tcase FrameMetadata::FrameSuccess:\n> -\t\t\tbuf.bytesused = fmd.planes[0].bytesused;\n> +\t\t\tbuf.bytesused = fmd.planes()[0].bytesused;\n> \t\t\tbuf.field = V4L2_FIELD_NONE;\n> \t\t\tbuf.timestamp.tv_sec = fmd.timestamp / 1000000000;\n> \t\t\tbuf.timestamp.tv_usec = fmd.timestamp % 1000000;\n>","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 539FABD87D\n\tfor ;\n\tMon, 6 Sep 2021 07:37:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0C3E06916C;\n\tMon, 6 Sep 2021 09:37: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 BA81160503\n\tfor ;\n\tMon, 6 Sep 2021 09:37:26 +0200 (CEST)","from tatooine.ideasonboard.com (unknown\n\t[IPv6:2a01:e0a:169:7140:eb18:8e30:9b7:f998])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 817C524F;\n\tMon, 6 Sep 2021 09:37: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=\"nPqxzc2j\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1630913846;\n\tbh=l+pGhF979HtD+LcyJwN0AhjNd+OSqtF+N/GO82FwqVc=;\n\th=Subject:To:References:From:Date:In-Reply-To:From;\n\tb=nPqxzc2jwQr+i6G9/gE6PEmKJORhgWHuKmjLIhl8Md2DKZ2gEOruWnRx7vo1yisnQ\n\tm2S04cv18WzuogdBJq7+rYVrzyfM9Yjl4KToxJpYX01N+1BD0kTn5FilfckW2fsZBI\n\tfU4a0jG21bfHvk5w2wpzw2hYnmDUyikdqxkpfI1M=","To":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210906020100.14430-1-laurent.pinchart@ideasonboard.com>\n\t<20210906020100.14430-18-laurent.pinchart@ideasonboard.com>","From":"Jean-Michel Hautbois ","Message-ID":"","Date":"Mon, 6 Sep 2021 09:37:23 +0200","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.13.0","MIME-Version":"1.0","In-Reply-To":"<20210906020100.14430-18-laurent.pinchart@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH v2 17/27] libcamera: framebuffer:\n\tPrevent modifying the number of metadata planes","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":19450,"web_url":"https://patchwork.libcamera.org/comment/19450/","msgid":"","date":"2021-09-06T13:28:28","subject":"Re: [libcamera-devel] [PATCH v2 17/27] libcamera: framebuffer:\n\tPrevent modifying the number of metadata planes","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Laurent,\n\nOn Mon, Sep 6, 2021 at 4:37 PM Jean-Michel Hautbois\n wrote:\n>\n> Hi Laurent,\n>\n> On 06/09/2021 04:00, Laurent Pinchart wrote:\n> > The number of metadata planes should always match the number of frame\n> > buffer planes. Enforce this by making the vector private and providing\n> > accessor functions.\n> >\n> > As this changes the public API, update all in-tree users accordingly.\n> >\n> > Signed-off-by: Laurent Pinchart \n> > Reviewed-by: Kieran Bingham \n> Reviewed-by: Jean-Michel Hautbois \n> > ---\n> > include/libcamera/framebuffer.h | 10 +++++++++-\n> > src/cam/camera_session.cpp | 4 ++--\n> > src/cam/file_sink.cpp | 2 +-\n> > src/libcamera/framebuffer.cpp | 12 +++++++++---\n> > src/libcamera/v4l2_videodevice.cpp | 14 +++++++-------\n> > src/qcam/main_window.cpp | 2 +-\n> > src/qcam/viewfinder_gl.cpp | 2 +-\n> > src/qcam/viewfinder_qt.cpp | 2 +-\n> > src/v4l2/v4l2_camera_proxy.cpp | 2 +-\n> > 9 files changed, 32 insertions(+), 18 deletions(-)\n> >\n> > diff --git a/include/libcamera/framebuffer.h b/include/libcamera/framebuffer.h\n> > index fd68ed0a139d..7f2f176af691 100644\n> > --- a/include/libcamera/framebuffer.h\n> > +++ b/include/libcamera/framebuffer.h\n> > @@ -13,6 +13,7 @@\n> > #include \n> >\n> > #include \n> > +#include \n> >\n> > #include \n> >\n> > @@ -34,7 +35,14 @@ struct FrameMetadata {\n> > Status status;\n> > unsigned int sequence;\n> > uint64_t timestamp;\n> > - std::vector planes;\n> > +\n> > + Span planes() { return planes_; }\n> > + Span planes() const { return planes_; }\n> > +\n> > +private:\n> > + friend class FrameBuffer;\n> > +\n> > + std::vector planes_;\n> > };\n> >\n> > class FrameBuffer final : public Extensible\n> > diff --git a/src/cam/camera_session.cpp b/src/cam/camera_session.cpp\n> > index 60d640f2b15c..32a373a99b72 100644\n> > --- a/src/cam/camera_session.cpp\n> > +++ b/src/cam/camera_session.cpp\n> > @@ -374,9 +374,9 @@ void CameraSession::processRequest(Request *request)\n> > << \" bytesused: \";\n> >\n> > unsigned int nplane = 0;\n> > - for (const FrameMetadata::Plane &plane : metadata.planes) {\n> > + for (const FrameMetadata::Plane &plane : metadata.planes()) {\n> > info << plane.bytesused;\n> > - if (++nplane < metadata.planes.size())\n> > + if (++nplane < metadata.planes().size())\n> > info << \"/\";\n> > }\n> > }\n> > diff --git a/src/cam/file_sink.cpp b/src/cam/file_sink.cpp\n> > index 0b529e3eb767..0fc7d621f50b 100644\n> > --- a/src/cam/file_sink.cpp\n> > +++ b/src/cam/file_sink.cpp\n> > @@ -110,7 +110,7 @@ void FileSink::writeBuffer(const Stream *stream, FrameBuffer *buffer)\n> >\n> > for (unsigned int i = 0; i < buffer->planes().size(); ++i) {\n> > const FrameBuffer::Plane &plane = buffer->planes()[i];\n> > - const FrameMetadata::Plane &meta = buffer->metadata().planes[i];\n> > + const FrameMetadata::Plane &meta = buffer->metadata().planes()[i];\n> >\n> > uint8_t *data = planeData_[&plane];\n> > unsigned int length = std::min(meta.bytesused, plane.length);\n> > diff --git a/src/libcamera/framebuffer.cpp b/src/libcamera/framebuffer.cpp\n> > index e4f8419a9063..d44a98babd05 100644\n> > --- a/src/libcamera/framebuffer.cpp\n> > +++ b/src/libcamera/framebuffer.cpp\n> > @@ -91,8 +91,14 @@ LOG_DEFINE_CATEGORY(Buffer)\n> > */\n> >\n> > /**\n> > - * \\var FrameMetadata::planes\n> > - * \\brief Array of per-plane metadata\n> > + * \\fn FrameMetadata::planes()\n> > + * \\copydoc FrameMetadata::planes() const\n> > + */\n> > +\n> > +/**\n> > + * \\fn FrameMetadata::planes() const\n> > + * \\brief Retrieve the array of per-plane metadata\n> > + * \\return The array of per-plane metadata\n> > */\n> >\n> > /**\n> > @@ -210,7 +216,7 @@ FrameBuffer::FrameBuffer(const std::vector &planes, unsigned int cookie)\n> > : Extensible(std::make_unique()), planes_(planes),\n> > cookie_(cookie)\n> > {\n> > - metadata_.planes.resize(planes_.size());\n> > + metadata_.planes_.resize(planes_.size());\n> >\n> > unsigned int offset = 0;\n> > bool isContiguous = true;\n> > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> > index e729e608448c..296304fbd979 100644\n> > --- a/src/libcamera/v4l2_videodevice.cpp\n> > +++ b/src/libcamera/v4l2_videodevice.cpp\n> > @@ -1538,7 +1538,7 @@ int V4L2VideoDevice::queueBuffer(FrameBuffer *buffer)\n> > unsigned int length = 0;\n> >\n> > for (auto [i, plane] : utils::enumerate(planes)) {\n> > - bytesused += metadata.planes[i].bytesused;\n> > + bytesused += metadata.planes()[i].bytesused;\n> > length += plane.length;\n> >\n> > if (i != planes.size() - 1 && bytesused != length) {\n> > @@ -1562,7 +1562,7 @@ int V4L2VideoDevice::queueBuffer(FrameBuffer *buffer)\n> > * V4L2 buffer is guaranteed to be equal at this point.\n> > */\n> > for (auto [i, plane] : utils::enumerate(planes)) {\n> > - v4l2Planes[i].bytesused = metadata.planes[i].bytesused;\n> > + v4l2Planes[i].bytesused = metadata.planes()[i].bytesused;\n> > v4l2Planes[i].length = plane.length;\n> > }\n> > } else {\n> > @@ -1570,7 +1570,7 @@ int V4L2VideoDevice::queueBuffer(FrameBuffer *buffer)\n> > * Single-planar API with a single plane in the buffer\n> > * is trivial to handle.\n> > */\n> > - buf.bytesused = metadata.planes[0].bytesused;\n> > + buf.bytesused = metadata.planes()[for 0].bytesused;\n> > buf.length = planes[0].length;\n> > }\n> >\n> > @@ -1699,9 +1699,9 @@ FrameBuffer *V4L2VideoDevice::dequeueBuffer()\n> > return buffer;\n> > }\n> >\n> > - metadata.planes[i].bytesused =\n> > + metadata.planes()[i].bytesused =\n> > std::min(plane.length, bytesused);\n> > - bytesused -= metadata.planes[i].bytesused;\n> > + bytesused -= metadata.planes()[i].bytesused;\n> > }\n> > } else if (multiPlanar) {\n> > /*\n> > @@ -1710,9 +1710,9 @@ FrameBuffer *V4L2VideoDevice::dequeueBuffer()\n> > * V4L2 buffer is guaranteed to be equal at this point.\n> > */\n> > for (unsigned int i = 0; i < numV4l2Planes; ++i)\n> > - metadata.planes[i].bytesused = planes[i].bytesused;\n> > + metadata.planes()[i].bytesused = planes[i].bytesused;\n\nPerhaps, using utils::enumerate() may be good here,\nfor (auto [i, plane] : metadata.planes())\n plane.bytes_used = planes[i].bytesused\n\nReviewed-by: Hirokazu Honda \n-Hiro\n> > } else {\n> > - metadata.planes[0].bytesused = buf.bytesused;\n> > + metadata.planes()[0].bytesused = buf.bytesused;\n> > }\n> >\n> > return buffer;\n> > diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp\n> > index 1536b2b5bd66..ac853e360aea 100644\n> > --- a/src/qcam/main_window.cpp\n> > +++ b/src/qcam/main_window.cpp\n> > @@ -756,7 +756,7 @@ void MainWindow::processViewfinder(FrameBuffer *buffer)\n> >\n> > qDebug().noquote()\n> > << QString(\"seq: %1\").arg(metadata.sequence, 6, 10, QLatin1Char('0'))\n> > - << \"bytesused:\" << metadata.planes[0].bytesused\n> > + << \"bytesused:\" << metadata.planes()[0].bytesused\n> > << \"timestamp:\" << metadata.timestamp\n> > << \"fps:\" << Qt::fixed << qSetRealNumberPrecision(2) << fps;\n> >\n> > diff --git a/src/qcam/viewfinder_gl.cpp b/src/qcam/viewfinder_gl.cpp\n> > index 40226601f9fd..d2ef036974f4 100644\n> > --- a/src/qcam/viewfinder_gl.cpp\n> > +++ b/src/qcam/viewfinder_gl.cpp\n> > @@ -125,7 +125,7 @@ void ViewFinderGL::render(libcamera::FrameBuffer *buffer,\n> > /*\n> > * \\todo Get the stride from the buffer instead of computing it naively\n> > */\n> > - stride_ = buffer->metadata().planes[0].bytesused / size_.height();\n> > + stride_ = buffer->metadata().planes()[0].bytesused / size_.height();\n> > update();\n> > buffer_ = buffer;\n> > }\n> > diff --git a/src/qcam/viewfinder_qt.cpp b/src/qcam/viewfinder_qt.cpp\n> > index efa1d412584b..a0bf99b0b522 100644\n> > --- a/src/qcam/viewfinder_qt.cpp\n> > +++ b/src/qcam/viewfinder_qt.cpp\n> > @@ -87,7 +87,7 @@ void ViewFinderQt::render(libcamera::FrameBuffer *buffer,\n> > }\n> >\n> > unsigned char *memory = mem.data();\n> > - size_t size = buffer->metadata().planes[0].bytesused;\n> > + size_t size = buffer->metadata().planes()[0].bytesused;\n> >\n> > {\n> > QMutexLocker locker(&mutex_);\n> > diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp\n> > index d926a7b77083..07d2250bb846 100644\n> > --- a/src/v4l2/v4l2_camera_proxy.cpp\n> > +++ b/src/v4l2/v4l2_camera_proxy.cpp\n> > @@ -211,7 +211,7 @@ void V4L2CameraProxy::updateBuffers()\n> >\n> > switch (fmd.status) {\n> > case FrameMetadata::FrameSuccess:\n> > - buf.bytesused = fmd.planes[0].bytesused;\n> > + buf.bytesused = fmd.planes()[0].bytesused;\n> > buf.field = V4L2_FIELD_NONE;\n> > buf.timestamp.tv_sec = fmd.timestamp / 1000000000;\n> > buf.timestamp.tv_usec = fmd.timestamp % 1000000;\n> >","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 148A7BD87D\n\tfor ;\n\tMon, 6 Sep 2021 13:28:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 900BC69167;\n\tMon, 6 Sep 2021 15:28:41 +0200 (CEST)","from mail-ej1-x635.google.com (mail-ej1-x635.google.com\n\t[IPv6:2a00:1450:4864:20::635])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1D82A60137\n\tfor ;\n\tMon, 6 Sep 2021 15:28:40 +0200 (CEST)","by mail-ej1-x635.google.com with SMTP id a25so13546591ejv.6\n\tfor ;\n\tMon, 06 Sep 2021 06:28:40 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"obGu74O+\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=f67u4vcgwqICD5MG6mRRgHkrn+m9iQRGj7lKqWvAD+I=;\n\tb=obGu74O+IddQzQPzcWeTH23DNOHfMb4aMwva+Ffol+c5cC+Rjjf3HuDUoxDz9ZBkXr\n\tNPzZmbIbuYrykQzdWSKJ/eLyD9zPXGVZb8sHcBN+OJwr0UsM44E9gR6OCEKlY0ZWLu+B\n\tVxciSaqOaaLbC1LPyBWxheMksMFrkiw6a11To=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=f67u4vcgwqICD5MG6mRRgHkrn+m9iQRGj7lKqWvAD+I=;\n\tb=ncc4JwHlvzs0X/In0oKvqycDagCOuiK/3C/8hrGRmSAqzHZyANjOSbHdKEYvLJJEQc\n\t05GmI1WXppF5TU6jbuZVBT8Zv/smly21a3Q6cPo+fXCdgq4zHwv7u+dxRvVAToCVrek7\n\trZC1/tYz2aiqApWXDEHkWhp1AcrduqQnEf49dkiJnE/LlO24WKVceEuSUnSGaXx9VBHa\n\tO2ThgkhOU+zIOBTeVaP3ir2Yli5j8+eVNnQ9Uc+otqghNAb7qjm50GIYumhsqNP6E07V\n\tu+VkMv6BdpM3/mOstJXnenOYJtTMrOsSFSFjaqNq/dFJuNTFW9Tw4DNQnW4EfTA7VDOM\n\t6Hbg==","X-Gm-Message-State":"AOAM531NU7GSI4jmLZEQuptii5C+88f/q8DqQtUJHnd8PRi6u3GC8+C2\n\tkhbbwNuc3HgLmdMjIc/87osQZ3IFz5vGHyXz1D5qWg==","X-Google-Smtp-Source":"ABdhPJy8/vQN26VbKqEbjfhgc3qiMvbys2SarkkJYBVnjIBpzyCpxunPmDo2tSwBXVkxD680jLdH/FlYoS11ol5e6bM=","X-Received":"by 2002:a17:906:3589:: with SMTP id\n\to9mr14076290ejb.150.1630934919546; \n\tMon, 06 Sep 2021 06:28:39 -0700 (PDT)","MIME-Version":"1.0","References":"<20210906020100.14430-1-laurent.pinchart@ideasonboard.com>\n\t<20210906020100.14430-18-laurent.pinchart@ideasonboard.com>\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Mon, 6 Sep 2021 22:28:28 +0900","Message-ID":"","To":"Jean-Michel Hautbois ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v2 17/27] libcamera: framebuffer:\n\tPrevent modifying the number of metadata planes","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":19457,"web_url":"https://patchwork.libcamera.org/comment/19457/","msgid":"","date":"2021-09-06T13:50:28","subject":"Re: [libcamera-devel] [PATCH v2 17/27] libcamera: framebuffer:\n\tPrevent modifying the number of metadata planes","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Hiro,\n\nOn Mon, Sep 06, 2021 at 10:28:28PM +0900, Hirokazu Honda wrote:\n> On Mon, Sep 6, 2021 at 4:37 PM Jean-Michel Hautbois wrote:\n> > On 06/09/2021 04:00, Laurent Pinchart wrote:\n> > > The number of metadata planes should always match the number of frame\n> > > buffer planes. Enforce this by making the vector private and providing\n> > > accessor functions.\n> > >\n> > > As this changes the public API, update all in-tree users accordingly.\n> > >\n> > > Signed-off-by: Laurent Pinchart \n> > > Reviewed-by: Kieran Bingham \n> > > Reviewed-by: Jean-Michel Hautbois \n> > > ---\n> > > include/libcamera/framebuffer.h | 10 +++++++++-\n> > > src/cam/camera_session.cpp | 4 ++--\n> > > src/cam/file_sink.cpp | 2 +-\n> > > src/libcamera/framebuffer.cpp | 12 +++++++++---\n> > > src/libcamera/v4l2_videodevice.cpp | 14 +++++++-------\n> > > src/qcam/main_window.cpp | 2 +-\n> > > src/qcam/viewfinder_gl.cpp | 2 +-\n> > > src/qcam/viewfinder_qt.cpp | 2 +-\n> > > src/v4l2/v4l2_camera_proxy.cpp | 2 +-\n> > > 9 files changed, 32 insertions(+), 18 deletions(-)\n> > >\n> > > diff --git a/include/libcamera/framebuffer.h b/include/libcamera/framebuffer.h\n> > > index fd68ed0a139d..7f2f176af691 100644\n> > > --- a/include/libcamera/framebuffer.h\n> > > +++ b/include/libcamera/framebuffer.h\n> > > @@ -13,6 +13,7 @@\n> > > #include \n> > >\n> > > #include \n> > > +#include \n> > >\n> > > #include \n> > >\n> > > @@ -34,7 +35,14 @@ struct FrameMetadata {\n> > > Status status;\n> > > unsigned int sequence;\n> > > uint64_t timestamp;\n> > > - std::vector planes;\n> > > +\n> > > + Span planes() { return planes_; }\n> > > + Span planes() const { return planes_; }\n> > > +\n> > > +private:\n> > > + friend class FrameBuffer;\n> > > +\n> > > + std::vector planes_;\n> > > };\n> > >\n> > > class FrameBuffer final : public Extensible\n> > > diff --git a/src/cam/camera_session.cpp b/src/cam/camera_session.cpp\n> > > index 60d640f2b15c..32a373a99b72 100644\n> > > --- a/src/cam/camera_session.cpp\n> > > +++ b/src/cam/camera_session.cpp\n> > > @@ -374,9 +374,9 @@ void CameraSession::processRequest(Request *request)\n> > > << \" bytesused: \";\n> > >\n> > > unsigned int nplane = 0;\n> > > - for (const FrameMetadata::Plane &plane : metadata.planes) {\n> > > + for (const FrameMetadata::Plane &plane : metadata.planes()) {\n> > > info << plane.bytesused;\n> > > - if (++nplane < metadata.planes.size())\n> > > + if (++nplane < metadata.planes().size())\n> > > info << \"/\";\n> > > }\n> > > }\n> > > diff --git a/src/cam/file_sink.cpp b/src/cam/file_sink.cpp\n> > > index 0b529e3eb767..0fc7d621f50b 100644\n> > > --- a/src/cam/file_sink.cpp\n> > > +++ b/src/cam/file_sink.cpp\n> > > @@ -110,7 +110,7 @@ void FileSink::writeBuffer(const Stream *stream, FrameBuffer *buffer)\n> > >\n> > > for (unsigned int i = 0; i < buffer->planes().size(); ++i) {\n> > > const FrameBuffer::Plane &plane = buffer->planes()[i];\n> > > - const FrameMetadata::Plane &meta = buffer->metadata().planes[i];\n> > > + const FrameMetadata::Plane &meta = buffer->metadata().planes()[i];\n> > >\n> > > uint8_t *data = planeData_[&plane];\n> > > unsigned int length = std::min(meta.bytesused, plane.length);\n> > > diff --git a/src/libcamera/framebuffer.cpp b/src/libcamera/framebuffer.cpp\n> > > index e4f8419a9063..d44a98babd05 100644\n> > > --- a/src/libcamera/framebuffer.cpp\n> > > +++ b/src/libcamera/framebuffer.cpp\n> > > @@ -91,8 +91,14 @@ LOG_DEFINE_CATEGORY(Buffer)\n> > > */\n> > >\n> > > /**\n> > > - * \\var FrameMetadata::planes\n> > > - * \\brief Array of per-plane metadata\n> > > + * \\fn FrameMetadata::planes()\n> > > + * \\copydoc FrameMetadata::planes() const\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn FrameMetadata::planes() const\n> > > + * \\brief Retrieve the array of per-plane metadata\n> > > + * \\return The array of per-plane metadata\n> > > */\n> > >\n> > > /**\n> > > @@ -210,7 +216,7 @@ FrameBuffer::FrameBuffer(const std::vector &planes, unsigned int cookie)\n> > > : Extensible(std::make_unique()), planes_(planes),\n> > > cookie_(cookie)\n> > > {\n> > > - metadata_.planes.resize(planes_.size());\n> > > + metadata_.planes_.resize(planes_.size());\n> > >\n> > > unsigned int offset = 0;\n> > > bool isContiguous = true;\n> > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> > > index e729e608448c..296304fbd979 100644\n> > > --- a/src/libcamera/v4l2_videodevice.cpp\n> > > +++ b/src/libcamera/v4l2_videodevice.cpp\n> > > @@ -1538,7 +1538,7 @@ int V4L2VideoDevice::queueBuffer(FrameBuffer *buffer)\n> > > unsigned int length = 0;\n> > >\n> > > for (auto [i, plane] : utils::enumerate(planes)) {\n> > > - bytesused += metadata.planes[i].bytesused;\n> > > + bytesused += metadata.planes()[i].bytesused;\n> > > length += plane.length;\n> > >\n> > > if (i != planes.size() - 1 && bytesused != length) {\n> > > @@ -1562,7 +1562,7 @@ int V4L2VideoDevice::queueBuffer(FrameBuffer *buffer)\n> > > * V4L2 buffer is guaranteed to be equal at this point.\n> > > */\n> > > for (auto [i, plane] : utils::enumerate(planes)) {\n> > > - v4l2Planes[i].bytesused = metadata.planes[i].bytesused;\n> > > + v4l2Planes[i].bytesused = metadata.planes()[i].bytesused;\n> > > v4l2Planes[i].length = plane.length;\n> > > }\n> > > } else {\n> > > @@ -1570,7 +1570,7 @@ int V4L2VideoDevice::queueBuffer(FrameBuffer *buffer)\n> > > * Single-planar API with a single plane in the buffer\n> > > * is trivial to handle.\n> > > */\n> > > - buf.bytesused = metadata.planes[0].bytesused;\n> > > + buf.bytesused = metadata.planes()[for 0].bytesused;\n> > > buf.length = planes[0].length;\n> > > }\n> > >\n> > > @@ -1699,9 +1699,9 @@ FrameBuffer *V4L2VideoDevice::dequeueBuffer()\n> > > return buffer;\n> > > }\n> > >\n> > > - metadata.planes[i].bytesused =\n> > > + metadata.planes()[i].bytesused =\n> > > std::min(plane.length, bytesused);\n> > > - bytesused -= metadata.planes[i].bytesused;\n> > > + bytesused -= metadata.planes()[i].bytesused;\n> > > }\n> > > } else if (multiPlanar) {\n> > > /*\n> > > @@ -1710,9 +1710,9 @@ FrameBuffer *V4L2VideoDevice::dequeueBuffer()\n> > > * V4L2 buffer is guaranteed to be equal at this point.\n> > > */\n> > > for (unsigned int i = 0; i < numV4l2Planes; ++i)\n> > > - metadata.planes[i].bytesused = planes[i].bytesused;\n> > > + metadata.planes()[i].bytesused = planes[i].bytesused;\n> \n> Perhaps, using utils::enumerate() may be good here,\n> for (auto [i, plane] : metadata.planes())\n> plane.bytes_used = planes[i].bytesused\n\nThat's what I initially wrote, but utils::enumerate() doesn't accept\nrvalue references, to avoid the problem described in\nhttps://en.cppreference.com/w/cpp/language/range-for#Temporary_range_expression.\nI would have to write\n\n\t\tSpan mplanes = metadata.planes();\n\t\tfor (auto [i, plane] : utils::enumerate(mplanes))\n\t\t\tplane.bytesused = planes[i].bytesused;\n\nand I thought it wasn't worth it.\n\n> Reviewed-by: Hirokazu Honda \n>\n> > > } else {\n> > > - metadata.planes[0].bytesused = buf.bytesused;\n> > > + metadata.planes()[0].bytesused = buf.bytesused;\n> > > }\n> > >\n> > > return buffer;\n> > > diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp\n> > > index 1536b2b5bd66..ac853e360aea 100644\n> > > --- a/src/qcam/main_window.cpp\n> > > +++ b/src/qcam/main_window.cpp\n> > > @@ -756,7 +756,7 @@ void MainWindow::processViewfinder(FrameBuffer *buffer)\n> > >\n> > > qDebug().noquote()\n> > > << QString(\"seq: %1\").arg(metadata.sequence, 6, 10, QLatin1Char('0'))\n> > > - << \"bytesused:\" << metadata.planes[0].bytesused\n> > > + << \"bytesused:\" << metadata.planes()[0].bytesused\n> > > << \"timestamp:\" << metadata.timestamp\n> > > << \"fps:\" << Qt::fixed << qSetRealNumberPrecision(2) << fps;\n> > >\n> > > diff --git a/src/qcam/viewfinder_gl.cpp b/src/qcam/viewfinder_gl.cpp\n> > > index 40226601f9fd..d2ef036974f4 100644\n> > > --- a/src/qcam/viewfinder_gl.cpp\n> > > +++ b/src/qcam/viewfinder_gl.cpp\n> > > @@ -125,7 +125,7 @@ void ViewFinderGL::render(libcamera::FrameBuffer *buffer,\n> > > /*\n> > > * \\todo Get the stride from the buffer instead of computing it naively\n> > > */\n> > > - stride_ = buffer->metadata().planes[0].bytesused / size_.height();\n> > > + stride_ = buffer->metadata().planes()[0].bytesused / size_.height();\n> > > update();\n> > > buffer_ = buffer;\n> > > }\n> > > diff --git a/src/qcam/viewfinder_qt.cpp b/src/qcam/viewfinder_qt.cpp\n> > > index efa1d412584b..a0bf99b0b522 100644\n> > > --- a/src/qcam/viewfinder_qt.cpp\n> > > +++ b/src/qcam/viewfinder_qt.cpp\n> > > @@ -87,7 +87,7 @@ void ViewFinderQt::render(libcamera::FrameBuffer *buffer,\n> > > }\n> > >\n> > > unsigned char *memory = mem.data();\n> > > - size_t size = buffer->metadata().planes[0].bytesused;\n> > > + size_t size = buffer->metadata().planes()[0].bytesused;\n> > >\n> > > {\n> > > QMutexLocker locker(&mutex_);\n> > > diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp\n> > > index d926a7b77083..07d2250bb846 100644\n> > > --- a/src/v4l2/v4l2_camera_proxy.cpp\n> > > +++ b/src/v4l2/v4l2_camera_proxy.cpp\n> > > @@ -211,7 +211,7 @@ void V4L2CameraProxy::updateBuffers()\n> > >\n> > > switch (fmd.status) {\n> > > case FrameMetadata::FrameSuccess:\n> > > - buf.bytesused = fmd.planes[0].bytesused;\n> > > + buf.bytesused = fmd.planes()[0].bytesused;\n> > > buf.field = V4L2_FIELD_NONE;\n> > > buf.timestamp.tv_sec = fmd.timestamp / 1000000000;\n> > > buf.timestamp.tv_usec = fmd.timestamp % 1000000;\n> > >","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 C1B3EBDC71\n\tfor ;\n\tMon, 6 Sep 2021 13:50:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3880D69167;\n\tMon, 6 Sep 2021 15:50:49 +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 1D2E360137\n\tfor ;\n\tMon, 6 Sep 2021 15:50:48 +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 7DB098AD;\n\tMon, 6 Sep 2021 15:50:47 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Gkzg27A7\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1630936247;\n\tbh=+VgRHS91nDzyBYtrbG6Ry6Hh1P/KVduhyc3UJLfb+R4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Gkzg27A7KPbn4CUPWP9xY/cR/sB6CDCYrEsTTXR5Ii5Kp5gEdQoIiNt+VHR93B52s\n\tD911MWnanhKd6J8RVDI01kngnCR62gbYwZiwWYhy6eGOsnz2uT2x56zOuayaDaZXIR\n\txF656V9eUS1V0kVkItqrWrQSfC1pHpGaZEOtalRI=","Date":"Mon, 6 Sep 2021 16:50:28 +0300","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20210906020100.14430-1-laurent.pinchart@ideasonboard.com>\n\t<20210906020100.14430-18-laurent.pinchart@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 17/27] libcamera: framebuffer:\n\tPrevent modifying the number of metadata planes","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":19459,"web_url":"https://patchwork.libcamera.org/comment/19459/","msgid":"","date":"2021-09-06T13:54:13","subject":"Re: [libcamera-devel] [PATCH v2 17/27] libcamera: framebuffer:\n\tPrevent modifying the number of metadata planes","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"Hi Laurent,\n\nOn Mon, Sep 6, 2021 at 10:50 PM Laurent Pinchart\n wrote:\n>\n> Hi Hiro,\n>\n> On Mon, Sep 06, 2021 at 10:28:28PM +0900, Hirokazu Honda wrote:\n> > On Mon, Sep 6, 2021 at 4:37 PM Jean-Michel Hautbois wrote:\n> > > On 06/09/2021 04:00, Laurent Pinchart wrote:\n> > > > The number of metadata planes should always match the number of frame\n> > > > buffer planes. Enforce this by making the vector private and providing\n> > > > accessor functions.\n> > > >\n> > > > As this changes the public API, update all in-tree users accordingly.\n> > > >\n> > > > Signed-off-by: Laurent Pinchart \n> > > > Reviewed-by: Kieran Bingham \n> > > > Reviewed-by: Jean-Michel Hautbois \n> > > > ---\n> > > > include/libcamera/framebuffer.h | 10 +++++++++-\n> > > > src/cam/camera_session.cpp | 4 ++--\n> > > > src/cam/file_sink.cpp | 2 +-\n> > > > src/libcamera/framebuffer.cpp | 12 +++++++++---\n> > > > src/libcamera/v4l2_videodevice.cpp | 14 +++++++-------\n> > > > src/qcam/main_window.cpp | 2 +-\n> > > > src/qcam/viewfinder_gl.cpp | 2 +-\n> > > > src/qcam/viewfinder_qt.cpp | 2 +-\n> > > > src/v4l2/v4l2_camera_proxy.cpp | 2 +-\n> > > > 9 files changed, 32 insertions(+), 18 deletions(-)\n> > > >\n> > > > diff --git a/include/libcamera/framebuffer.h b/include/libcamera/framebuffer.h\n> > > > index fd68ed0a139d..7f2f176af691 100644\n> > > > --- a/include/libcamera/framebuffer.h\n> > > > +++ b/include/libcamera/framebuffer.h\n> > > > @@ -13,6 +13,7 @@\n> > > > #include \n> > > >\n> > > > #include \n> > > > +#include \n> > > >\n> > > > #include \n> > > >\n> > > > @@ -34,7 +35,14 @@ struct FrameMetadata {\n> > > > Status status;\n> > > > unsigned int sequence;\n> > > > uint64_t timestamp;\n> > > > - std::vector planes;\n> > > > +\n> > > > + Span planes() { return planes_; }\n> > > > + Span planes() const { return planes_; }\n> > > > +\n> > > > +private:\n> > > > + friend class FrameBuffer;\n> > > > +\n> > > > + std::vector planes_;\n> > > > };\n> > > >\n> > > > class FrameBuffer final : public Extensible\n> > > > diff --git a/src/cam/camera_session.cpp b/src/cam/camera_session.cpp\n> > > > index 60d640f2b15c..32a373a99b72 100644\n> > > > --- a/src/cam/camera_session.cpp\n> > > > +++ b/src/cam/camera_session.cpp\n> > > > @@ -374,9 +374,9 @@ void CameraSession::processRequest(Request *request)\n> > > > << \" bytesused: \";\n> > > >\n> > > > unsigned int nplane = 0;\n> > > > - for (const FrameMetadata::Plane &plane : metadata.planes) {\n> > > > + for (const FrameMetadata::Plane &plane : metadata.planes()) {\n> > > > info << plane.bytesused;\n> > > > - if (++nplane < metadata.planes.size())\n> > > > + if (++nplane < metadata.planes().size())\n> > > > info << \"/\";\n> > > > }\n> > > > }\n> > > > diff --git a/src/cam/file_sink.cpp b/src/cam/file_sink.cpp\n> > > > index 0b529e3eb767..0fc7d621f50b 100644\n> > > > --- a/src/cam/file_sink.cpp\n> > > > +++ b/src/cam/file_sink.cpp\n> > > > @@ -110,7 +110,7 @@ void FileSink::writeBuffer(const Stream *stream, FrameBuffer *buffer)\n> > > >\n> > > > for (unsigned int i = 0; i < buffer->planes().size(); ++i) {\n> > > > const FrameBuffer::Plane &plane = buffer->planes()[i];\n> > > > - const FrameMetadata::Plane &meta = buffer->metadata().planes[i];\n> > > > + const FrameMetadata::Plane &meta = buffer->metadata().planes()[i];\n> > > >\n> > > > uint8_t *data = planeData_[&plane];\n> > > > unsigned int length = std::min(meta.bytesused, plane.length);\n> > > > diff --git a/src/libcamera/framebuffer.cpp b/src/libcamera/framebuffer.cpp\n> > > > index e4f8419a9063..d44a98babd05 100644\n> > > > --- a/src/libcamera/framebuffer.cpp\n> > > > +++ b/src/libcamera/framebuffer.cpp\n> > > > @@ -91,8 +91,14 @@ LOG_DEFINE_CATEGORY(Buffer)\n> > > > */\n> > > >\n> > > > /**\n> > > > - * \\var FrameMetadata::planes\n> > > > - * \\brief Array of per-plane metadata\n> > > > + * \\fn FrameMetadata::planes()\n> > > > + * \\copydoc FrameMetadata::planes() const\n> > > > + */\n> > > > +\n> > > > +/**\n> > > > + * \\fn FrameMetadata::planes() const\n> > > > + * \\brief Retrieve the array of per-plane metadata\n> > > > + * \\return The array of per-plane metadata\n> > > > */\n> > > >\n> > > > /**\n> > > > @@ -210,7 +216,7 @@ FrameBuffer::FrameBuffer(const std::vector &planes, unsigned int cookie)\n> > > > : Extensible(std::make_unique()), planes_(planes),\n> > > > cookie_(cookie)\n> > > > {\n> > > > - metadata_.planes.resize(planes_.size());\n> > > > + metadata_.planes_.resize(planes_.size());\n> > > >\n> > > > unsigned int offset = 0;\n> > > > bool isContiguous = true;\n> > > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp\n> > > > index e729e608448c..296304fbd979 100644\n> > > > --- a/src/libcamera/v4l2_videodevice.cpp\n> > > > +++ b/src/libcamera/v4l2_videodevice.cpp\n> > > > @@ -1538,7 +1538,7 @@ int V4L2VideoDevice::queueBuffer(FrameBuffer *buffer)\n> > > > unsigned int length = 0;\n> > > >\n> > > > for (auto [i, plane] : utils::enumerate(planes)) {\n> > > > - bytesused += metadata.planes[i].bytesused;\n> > > > + bytesused += metadata.planes()[i].bytesused;\n> > > > length += plane.length;\n> > > >\n> > > > if (i != planes.size() - 1 && bytesused != length) {\n> > > > @@ -1562,7 +1562,7 @@ int V4L2VideoDevice::queueBuffer(FrameBuffer *buffer)\n> > > > * V4L2 buffer is guaranteed to be equal at this point.\n> > > > */\n> > > > for (auto [i, plane] : utils::enumerate(planes)) {\n> > > > - v4l2Planes[i].bytesused = metadata.planes[i].bytesused;\n> > > > + v4l2Planes[i].bytesused = metadata.planes()[i].bytesused;\n> > > > v4l2Planes[i].length = plane.length;\n> > > > }\n> > > > } else {\n> > > > @@ -1570,7 +1570,7 @@ int V4L2VideoDevice::queueBuffer(FrameBuffer *buffer)\n> > > > * Single-planar API with a single plane in the buffer\n> > > > * is trivial to handle.\n> > > > */\n> > > > - buf.bytesused = metadata.planes[0].bytesused;\n> > > > + buf.bytesused = metadata.planes()[for 0].bytesused;\n> > > > buf.length = planes[0].length;\n> > > > }\n> > > >\n> > > > @@ -1699,9 +1699,9 @@ FrameBuffer *V4L2VideoDevice::dequeueBuffer()\n> > > > return buffer;\n> > > > }\n> > > >\n> > > > - metadata.planes[i].bytesused =\n> > > > + metadata.planes()[i].bytesused =\n> > > > std::min(plane.length, bytesused);\n> > > > - bytesused -= metadata.planes[i].bytesused;\n> > > > + bytesused -= metadata.planes()[i].bytesused;\n> > > > }\n> > > > } else if (multiPlanar) {\n> > > > /*\n> > > > @@ -1710,9 +1710,9 @@ FrameBuffer *V4L2VideoDevice::dequeueBuffer()\n> > > > * V4L2 buffer is guaranteed to be equal at this point.\n> > > > */\n> > > > for (unsigned int i = 0; i < numV4l2Planes; ++i)\n> > > > - metadata.planes[i].bytesused = planes[i].bytesused;\n> > > > + metadata.planes()[i].bytesused = planes[i].bytesused;\n> >\n> > Perhaps, using utils::enumerate() may be good here,\n> > for (auto [i, plane] : metadata.planes())\n> > plane.bytes_used = planes[i].bytesused\n>\n> That's what I initially wrote, but utils::enumerate() doesn't accept\n> rvalue references, to avoid the problem described in\n> https://en.cppreference.com/w/cpp/language/range-for#Temporary_range_expression.\n> I would have to write\n>\n> Span mplanes = metadata.planes();\n> for (auto [i, plane] : utils::enumerate(mplanes))\n> plane.bytesused = planes[i].bytesused;\n>\n> and I thought it wasn't worth it.\n\nAcked.\n\n-Hiro\n>\n> > Reviewed-by: Hirokazu Honda \n> >\n> > > > } else {\n> > > > - metadata.planes[0].bytesused = buf.bytesused;\n> > > > + metadata.planes()[0].bytesused = buf.bytesused;\n> > > > }\n> > > >\n> > > > return buffer;\n> > > > diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp\n> > > > index 1536b2b5bd66..ac853e360aea 100644\n> > > > --- a/src/qcam/main_window.cpp\n> > > > +++ b/src/qcam/main_window.cpp\n> > > > @@ -756,7 +756,7 @@ void MainWindow::processViewfinder(FrameBuffer *buffer)\n> > > >\n> > > > qDebug().noquote()\n> > > > << QString(\"seq: %1\").arg(metadata.sequence, 6, 10, QLatin1Char('0'))\n> > > > - << \"bytesused:\" << metadata.planes[0].bytesused\n> > > > + << \"bytesused:\" << metadata.planes()[0].bytesused\n> > > > << \"timestamp:\" << metadata.timestamp\n> > > > << \"fps:\" << Qt::fixed << qSetRealNumberPrecision(2) << fps;\n> > > >\n> > > > diff --git a/src/qcam/viewfinder_gl.cpp b/src/qcam/viewfinder_gl.cpp\n> > > > index 40226601f9fd..d2ef036974f4 100644\n> > > > --- a/src/qcam/viewfinder_gl.cpp\n> > > > +++ b/src/qcam/viewfinder_gl.cpp\n> > > > @@ -125,7 +125,7 @@ void ViewFinderGL::render(libcamera::FrameBuffer *buffer,\n> > > > /*\n> > > > * \\todo Get the stride from the buffer instead of computing it naively\n> > > > */\n> > > > - stride_ = buffer->metadata().planes[0].bytesused / size_.height();\n> > > > + stride_ = buffer->metadata().planes()[0].bytesused / size_.height();\n> > > > update();\n> > > > buffer_ = buffer;\n> > > > }\n> > > > diff --git a/src/qcam/viewfinder_qt.cpp b/src/qcam/viewfinder_qt.cpp\n> > > > index efa1d412584b..a0bf99b0b522 100644\n> > > > --- a/src/qcam/viewfinder_qt.cpp\n> > > > +++ b/src/qcam/viewfinder_qt.cpp\n> > > > @@ -87,7 +87,7 @@ void ViewFinderQt::render(libcamera::FrameBuffer *buffer,\n> > > > }\n> > > >\n> > > > unsigned char *memory = mem.data();\n> > > > - size_t size = buffer->metadata().planes[0].bytesused;\n> > > > + size_t size = buffer->metadata().planes()[0].bytesused;\n> > > >\n> > > > {\n> > > > QMutexLocker locker(&mutex_);\n> > > > diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp\n> > > > index d926a7b77083..07d2250bb846 100644\n> > > > --- a/src/v4l2/v4l2_camera_proxy.cpp\n> > > > +++ b/src/v4l2/v4l2_camera_proxy.cpp\n> > > > @@ -211,7 +211,7 @@ void V4L2CameraProxy::updateBuffers()\n> > > >\n> > > > switch (fmd.status) {\n> > > > case FrameMetadata::FrameSuccess:\n> > > > - buf.bytesused = fmd.planes[0].bytesused;\n> > > > + buf.bytesused = fmd.planes()[0].bytesused;\n> > > > buf.field = V4L2_FIELD_NONE;\n> > > > buf.timestamp.tv_sec = fmd.timestamp / 1000000000;\n> > > > buf.timestamp.tv_usec = fmd.timestamp % 1000000;\n> > > >\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","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 A5011BDC71\n\tfor ;\n\tMon, 6 Sep 2021 13:54:27 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 556526916C;\n\tMon, 6 Sep 2021 15:54:27 +0200 (CEST)","from mail-ed1-x52d.google.com (mail-ed1-x52d.google.com\n\t[IPv6:2a00:1450:4864:20::52d])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6F25560137\n\tfor ;\n\tMon, 6 Sep 2021 15:54:26 +0200 (CEST)","by mail-ed1-x52d.google.com with SMTP id u19so9658195edb.3\n\tfor ;\n\tMon, 06 Sep 2021 06:54:26 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"U7tGLTH6\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=80umj7c1W5zzh8AIjNEq2MH7/zwu7k2iDOgoiKZ9Q/g=;\n\tb=U7tGLTH6xWELPQqUa3zRtC1orHoeH2eKWR9vWKdCh1bj8m0C/7hBjtLp+uRyvrktlf\n\tWZN/ZyXw2pmEG17nZ77PoBXR1l3clCxHdb+CeI0d1cwtax/jvStwOIzH7G5nLjLDIP9l\n\txohE8mhp3sSL39rM7EjuiIZq7VaZD6nYPUYw8=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=80umj7c1W5zzh8AIjNEq2MH7/zwu7k2iDOgoiKZ9Q/g=;\n\tb=UNWtYCXb1Ya8ViGX+SKm44O0uSMWk1qzaiVOObAx8V+PI9OocCXosCSFliL+OFp8jY\n\tUzxRBu323ZGX25/EySltqhR9QhJ98HOTDBwMN91dCKyZtTRb/zoaDRy7LYwgfuxpXlk/\n\tiCOkEfHt05WmiW5F8gwsbruNd7pfysnTQ57lSwBv+b6n/XQuB5IaBd/KiZP8VCHH14wW\n\t/q3XYgB4y/YJY+2wejjlg/VwzsM9bq6zB7CBQzsYJjQ5Y714LJR03ZFkZlTypYeCoMy6\n\tCQg7p3RYKKaOigAMODrcEr2nr+jtXeljXQPha7Q7YIQoeGui1RtSzfsLEkwmiyZ1oMjl\n\tkfUQ==","X-Gm-Message-State":"AOAM530tYQ4M6xxzVp2ZhprzTRYOWqMQirHhWsi7DXxcotMgDPzqK2sx\n\tTTv7FPYcUN9v7vZ4BE3zqZqMrj8HTHUqkYi5u/uNSV21Hgk=","X-Google-Smtp-Source":"ABdhPJx1tN2ZxbrfeEjnMMT9VGRf2U8H83SyedTo1tbytEeA4DSKXlRwquFBh7rGYlMyjpwMCZ73EVh+rRUgkHvxerk=","X-Received":"by 2002:a05:6402:1385:: with SMTP id\n\tb5mr13148096edv.276.1630936466053; \n\tMon, 06 Sep 2021 06:54:26 -0700 (PDT)","MIME-Version":"1.0","References":"<20210906020100.14430-1-laurent.pinchart@ideasonboard.com>\n\t<20210906020100.14430-18-laurent.pinchart@ideasonboard.com>\n\t\n\t\n\t","In-Reply-To":"","From":"Hirokazu Honda ","Date":"Mon, 6 Sep 2021 22:54:13 +0900","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v2 17/27] libcamera: framebuffer:\n\tPrevent modifying the number of metadata planes","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\" "}}]