[libcamera-devel] [PATCH v2 17/27] libcamera: framebuffer: Prevent modifying the number of metadata planes
Hirokazu Honda
hiroh at chromium.org
Mon Sep 6 15:54:13 CEST 2021
Hi Laurent,
On Mon, Sep 6, 2021 at 10:50 PM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Hiro,
>
> On Mon, Sep 06, 2021 at 10:28:28PM +0900, Hirokazu Honda wrote:
> > On Mon, Sep 6, 2021 at 4:37 PM Jean-Michel Hautbois wrote:
> > > On 06/09/2021 04:00, Laurent Pinchart wrote:
> > > > The number of metadata planes should always match the number of frame
> > > > buffer planes. Enforce this by making the vector private and providing
> > > > accessor functions.
> > > >
> > > > As this changes the public API, update all in-tree users accordingly.
> > > >
> > > > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > > > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > > > Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>
> > > > ---
> > > > include/libcamera/framebuffer.h | 10 +++++++++-
> > > > src/cam/camera_session.cpp | 4 ++--
> > > > src/cam/file_sink.cpp | 2 +-
> > > > src/libcamera/framebuffer.cpp | 12 +++++++++---
> > > > src/libcamera/v4l2_videodevice.cpp | 14 +++++++-------
> > > > src/qcam/main_window.cpp | 2 +-
> > > > src/qcam/viewfinder_gl.cpp | 2 +-
> > > > src/qcam/viewfinder_qt.cpp | 2 +-
> > > > src/v4l2/v4l2_camera_proxy.cpp | 2 +-
> > > > 9 files changed, 32 insertions(+), 18 deletions(-)
> > > >
> > > > diff --git a/include/libcamera/framebuffer.h b/include/libcamera/framebuffer.h
> > > > index fd68ed0a139d..7f2f176af691 100644
> > > > --- a/include/libcamera/framebuffer.h
> > > > +++ b/include/libcamera/framebuffer.h
> > > > @@ -13,6 +13,7 @@
> > > > #include <vector>
> > > >
> > > > #include <libcamera/base/class.h>
> > > > +#include <libcamera/base/span.h>
> > > >
> > > > #include <libcamera/file_descriptor.h>
> > > >
> > > > @@ -34,7 +35,14 @@ struct FrameMetadata {
> > > > Status status;
> > > > unsigned int sequence;
> > > > uint64_t timestamp;
> > > > - std::vector<Plane> planes;
> > > > +
> > > > + Span<Plane> planes() { return planes_; }
> > > > + Span<const Plane> planes() const { return planes_; }
> > > > +
> > > > +private:
> > > > + friend class FrameBuffer;
> > > > +
> > > > + std::vector<Plane> planes_;
> > > > };
> > > >
> > > > class FrameBuffer final : public Extensible
> > > > diff --git a/src/cam/camera_session.cpp b/src/cam/camera_session.cpp
> > > > index 60d640f2b15c..32a373a99b72 100644
> > > > --- a/src/cam/camera_session.cpp
> > > > +++ b/src/cam/camera_session.cpp
> > > > @@ -374,9 +374,9 @@ void CameraSession::processRequest(Request *request)
> > > > << " bytesused: ";
> > > >
> > > > unsigned int nplane = 0;
> > > > - for (const FrameMetadata::Plane &plane : metadata.planes) {
> > > > + for (const FrameMetadata::Plane &plane : metadata.planes()) {
> > > > info << plane.bytesused;
> > > > - if (++nplane < metadata.planes.size())
> > > > + if (++nplane < metadata.planes().size())
> > > > info << "/";
> > > > }
> > > > }
> > > > diff --git a/src/cam/file_sink.cpp b/src/cam/file_sink.cpp
> > > > index 0b529e3eb767..0fc7d621f50b 100644
> > > > --- a/src/cam/file_sink.cpp
> > > > +++ b/src/cam/file_sink.cpp
> > > > @@ -110,7 +110,7 @@ void FileSink::writeBuffer(const Stream *stream, FrameBuffer *buffer)
> > > >
> > > > for (unsigned int i = 0; i < buffer->planes().size(); ++i) {
> > > > const FrameBuffer::Plane &plane = buffer->planes()[i];
> > > > - const FrameMetadata::Plane &meta = buffer->metadata().planes[i];
> > > > + const FrameMetadata::Plane &meta = buffer->metadata().planes()[i];
> > > >
> > > > uint8_t *data = planeData_[&plane];
> > > > unsigned int length = std::min(meta.bytesused, plane.length);
> > > > diff --git a/src/libcamera/framebuffer.cpp b/src/libcamera/framebuffer.cpp
> > > > index e4f8419a9063..d44a98babd05 100644
> > > > --- a/src/libcamera/framebuffer.cpp
> > > > +++ b/src/libcamera/framebuffer.cpp
> > > > @@ -91,8 +91,14 @@ LOG_DEFINE_CATEGORY(Buffer)
> > > > */
> > > >
> > > > /**
> > > > - * \var FrameMetadata::planes
> > > > - * \brief Array of per-plane metadata
> > > > + * \fn FrameMetadata::planes()
> > > > + * \copydoc FrameMetadata::planes() const
> > > > + */
> > > > +
> > > > +/**
> > > > + * \fn FrameMetadata::planes() const
> > > > + * \brief Retrieve the array of per-plane metadata
> > > > + * \return The array of per-plane metadata
> > > > */
> > > >
> > > > /**
> > > > @@ -210,7 +216,7 @@ FrameBuffer::FrameBuffer(const std::vector<Plane> &planes, unsigned int cookie)
> > > > : Extensible(std::make_unique<Private>()), planes_(planes),
> > > > cookie_(cookie)
> > > > {
> > > > - metadata_.planes.resize(planes_.size());
> > > > + metadata_.planes_.resize(planes_.size());
> > > >
> > > > unsigned int offset = 0;
> > > > bool isContiguous = true;
> > > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> > > > index e729e608448c..296304fbd979 100644
> > > > --- a/src/libcamera/v4l2_videodevice.cpp
> > > > +++ b/src/libcamera/v4l2_videodevice.cpp
> > > > @@ -1538,7 +1538,7 @@ int V4L2VideoDevice::queueBuffer(FrameBuffer *buffer)
> > > > unsigned int length = 0;
> > > >
> > > > for (auto [i, plane] : utils::enumerate(planes)) {
> > > > - bytesused += metadata.planes[i].bytesused;
> > > > + bytesused += metadata.planes()[i].bytesused;
> > > > length += plane.length;
> > > >
> > > > if (i != planes.size() - 1 && bytesused != length) {
> > > > @@ -1562,7 +1562,7 @@ int V4L2VideoDevice::queueBuffer(FrameBuffer *buffer)
> > > > * V4L2 buffer is guaranteed to be equal at this point.
> > > > */
> > > > for (auto [i, plane] : utils::enumerate(planes)) {
> > > > - v4l2Planes[i].bytesused = metadata.planes[i].bytesused;
> > > > + v4l2Planes[i].bytesused = metadata.planes()[i].bytesused;
> > > > v4l2Planes[i].length = plane.length;
> > > > }
> > > > } else {
> > > > @@ -1570,7 +1570,7 @@ int V4L2VideoDevice::queueBuffer(FrameBuffer *buffer)
> > > > * Single-planar API with a single plane in the buffer
> > > > * is trivial to handle.
> > > > */
> > > > - buf.bytesused = metadata.planes[0].bytesused;
> > > > + buf.bytesused = metadata.planes()[for 0].bytesused;
> > > > buf.length = planes[0].length;
> > > > }
> > > >
> > > > @@ -1699,9 +1699,9 @@ FrameBuffer *V4L2VideoDevice::dequeueBuffer()
> > > > return buffer;
> > > > }
> > > >
> > > > - metadata.planes[i].bytesused =
> > > > + metadata.planes()[i].bytesused =
> > > > std::min(plane.length, bytesused);
> > > > - bytesused -= metadata.planes[i].bytesused;
> > > > + bytesused -= metadata.planes()[i].bytesused;
> > > > }
> > > > } else if (multiPlanar) {
> > > > /*
> > > > @@ -1710,9 +1710,9 @@ FrameBuffer *V4L2VideoDevice::dequeueBuffer()
> > > > * V4L2 buffer is guaranteed to be equal at this point.
> > > > */
> > > > for (unsigned int i = 0; i < numV4l2Planes; ++i)
> > > > - metadata.planes[i].bytesused = planes[i].bytesused;
> > > > + metadata.planes()[i].bytesused = planes[i].bytesused;
> >
> > Perhaps, using utils::enumerate() may be good here,
> > for (auto [i, plane] : metadata.planes())
> > plane.bytes_used = planes[i].bytesused
>
> That's what I initially wrote, but utils::enumerate() doesn't accept
> rvalue references, to avoid the problem described in
> https://en.cppreference.com/w/cpp/language/range-for#Temporary_range_expression.
> I would have to write
>
> Span<FrameMetaData::Plane> mplanes = metadata.planes();
> for (auto [i, plane] : utils::enumerate(mplanes))
> plane.bytesused = planes[i].bytesused;
>
> and I thought it wasn't worth it.
Acked.
-Hiro
>
> > Reviewed-by: Hirokazu Honda <hiroh at chromium.org>
> >
> > > > } else {
> > > > - metadata.planes[0].bytesused = buf.bytesused;
> > > > + metadata.planes()[0].bytesused = buf.bytesused;
> > > > }
> > > >
> > > > return buffer;
> > > > diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
> > > > index 1536b2b5bd66..ac853e360aea 100644
> > > > --- a/src/qcam/main_window.cpp
> > > > +++ b/src/qcam/main_window.cpp
> > > > @@ -756,7 +756,7 @@ void MainWindow::processViewfinder(FrameBuffer *buffer)
> > > >
> > > > qDebug().noquote()
> > > > << QString("seq: %1").arg(metadata.sequence, 6, 10, QLatin1Char('0'))
> > > > - << "bytesused:" << metadata.planes[0].bytesused
> > > > + << "bytesused:" << metadata.planes()[0].bytesused
> > > > << "timestamp:" << metadata.timestamp
> > > > << "fps:" << Qt::fixed << qSetRealNumberPrecision(2) << fps;
> > > >
> > > > diff --git a/src/qcam/viewfinder_gl.cpp b/src/qcam/viewfinder_gl.cpp
> > > > index 40226601f9fd..d2ef036974f4 100644
> > > > --- a/src/qcam/viewfinder_gl.cpp
> > > > +++ b/src/qcam/viewfinder_gl.cpp
> > > > @@ -125,7 +125,7 @@ void ViewFinderGL::render(libcamera::FrameBuffer *buffer,
> > > > /*
> > > > * \todo Get the stride from the buffer instead of computing it naively
> > > > */
> > > > - stride_ = buffer->metadata().planes[0].bytesused / size_.height();
> > > > + stride_ = buffer->metadata().planes()[0].bytesused / size_.height();
> > > > update();
> > > > buffer_ = buffer;
> > > > }
> > > > diff --git a/src/qcam/viewfinder_qt.cpp b/src/qcam/viewfinder_qt.cpp
> > > > index efa1d412584b..a0bf99b0b522 100644
> > > > --- a/src/qcam/viewfinder_qt.cpp
> > > > +++ b/src/qcam/viewfinder_qt.cpp
> > > > @@ -87,7 +87,7 @@ void ViewFinderQt::render(libcamera::FrameBuffer *buffer,
> > > > }
> > > >
> > > > unsigned char *memory = mem.data();
> > > > - size_t size = buffer->metadata().planes[0].bytesused;
> > > > + size_t size = buffer->metadata().planes()[0].bytesused;
> > > >
> > > > {
> > > > QMutexLocker locker(&mutex_);
> > > > diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp
> > > > index d926a7b77083..07d2250bb846 100644
> > > > --- a/src/v4l2/v4l2_camera_proxy.cpp
> > > > +++ b/src/v4l2/v4l2_camera_proxy.cpp
> > > > @@ -211,7 +211,7 @@ void V4L2CameraProxy::updateBuffers()
> > > >
> > > > switch (fmd.status) {
> > > > case FrameMetadata::FrameSuccess:
> > > > - buf.bytesused = fmd.planes[0].bytesused;
> > > > + buf.bytesused = fmd.planes()[0].bytesused;
> > > > buf.field = V4L2_FIELD_NONE;
> > > > buf.timestamp.tv_sec = fmd.timestamp / 1000000000;
> > > > buf.timestamp.tv_usec = fmd.timestamp % 1000000;
> > > >
>
> --
> Regards,
>
> Laurent Pinchart
More information about the libcamera-devel
mailing list