[libcamera-devel] [PATCH v2 16/27] libcamera: framebuffer: Allocate metadata planes at construction time

[Hirokazu Honda]

Hi Laurent, thank you for the patch.

On Mon, Sep 6, 2021 at 4:36 PM Jean-Michel Hautbois
<jeanmichel.hautbois at ideasonboard.com> wrote:
>
> Hi Laurent,
>
> On 06/09/2021 04:00, Laurent Pinchart wrote:
> > The metadata planes are allocated by V4L2VideoDevice when dequeuing a
> > buffer. This causes the metadata planes to only be allocated after a
> > buffer gets dequeued, and doesn't provide any strong guarantee that
> > their number matches the number of FrameBuffer planes. The lack of this
> > invariant makes the FrameBuffer class fragile.
> >
> > As a first step towards fixing this, allocate the metadata planes when
> > the FrameBuffer is constructed. The FrameMetadata API should be further
> > improved by preventing a change in the number of planes.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois at ideasonboard.com>

Reviewed-by: Hirokazu Honda <hiroh at chromium.org>

> > ---
> > Changes since v1:
> >
> > - Return buffer with state set to FrameError on error
> > ---
> >  src/libcamera/framebuffer.cpp      |  2 ++
> >  src/libcamera/v4l2_videodevice.cpp | 10 +++++-----
> >  2 files changed, 7 insertions(+), 5 deletions(-)
> >
> > diff --git a/src/libcamera/framebuffer.cpp b/src/libcamera/framebuffer.cpp
> > index e71c2ffae034..e4f8419a9063 100644
> > --- a/src/libcamera/framebuffer.cpp
> > +++ b/src/libcamera/framebuffer.cpp
> > @@ -210,6 +210,8 @@ FrameBuffer::FrameBuffer(const std::vector<Plane> &planes, unsigned int cookie)
> >       : Extensible(std::make_unique<Private>()), planes_(planes),
> >         cookie_(cookie)
> >  {
> > +     metadata_.planes.resize(planes_.size());
> > +
> >       unsigned int offset = 0;
> >       bool isContiguous = true;
> >       ino_t inode = 0;
> > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> > index 59aa53c7c27e..e729e608448c 100644
> > --- a/src/libcamera/v4l2_videodevice.cpp
> > +++ b/src/libcamera/v4l2_videodevice.cpp
> > @@ -1670,7 +1670,6 @@ FrameBuffer *V4L2VideoDevice::dequeueBuffer()
> >
> >       unsigned int numV4l2Planes = multiPlanar ? buf.length : 1;
> >       FrameMetadata &metadata = buffer->metadata_;
> > -     metadata.planes.clear();
> >
> >       if (numV4l2Planes != buffer->planes().size()) {
> >               /*
> > @@ -1700,8 +1699,9 @@ FrameBuffer *V4L2VideoDevice::dequeueBuffer()
> >                               return buffer;
> >                       }
> >
> > -                     metadata.planes.push_back({ std::min(plane.length, bytesused) });
> > -                     bytesused -= metadata.planes.back().bytesused;
> > +                     metadata.planes[i].bytesused =
> > +                             std::min(plane.length, 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.push_back({ planes[i].bytesused });
> > +                     metadata.planes[i].bytesused = planes[i].bytesused;
> >       } else {
> > -             metadata.planes.push_back({ buf.bytesused });
> > +             metadata.planes[0].bytesused = buf.bytesused;
> >       }
> >
> >       return buffer;
> >