[libcamera-devel] [RFC PATCH v1 07/12] libcamera: framebuffer: Allocate metadata planes at construction time

[Hirokazu Honda]

Hi Laurent,

On Fri, Sep 3, 2021 at 3:11 AM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Kieran,
>
> On Thu, Sep 02, 2021 at 10:54:19AM +0100, Kieran Bingham wrote:
> > On 02/09/2021 05:22, 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>
> > > ---
> > >  src/libcamera/framebuffer.cpp      |  2 ++
> > >  src/libcamera/v4l2_videodevice.cpp | 12 +++++++++---
> > >  2 files changed, 11 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/src/libcamera/framebuffer.cpp b/src/libcamera/framebuffer.cpp
> > > index 99265b44da43..c1529dfb0ad1 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());

I would set bytesused to some default value, probably 0?
struct Plane {
  unsigned int bytesused = 0;
};

> > > +
> > >     unsigned int offset = 0;
> > >
> > >     /* \todo Remove the assertions after sufficient testing */
> > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> > > index adabd4720668..a51971879e75 100644
> > > --- a/src/libcamera/v4l2_videodevice.cpp
> > > +++ b/src/libcamera/v4l2_videodevice.cpp
> > > @@ -1586,12 +1586,18 @@ FrameBuffer *V4L2VideoDevice::dequeueBuffer()
> > >     buffer->metadata_.timestamp = buf.timestamp.tv_sec * 1000000000ULL
> > >                                 + buf.timestamp.tv_usec * 1000ULL;
> > >
> > > -   buffer->metadata_.planes.clear();
> > >     if (multiPlanar) {
> > > +           if (buf.length > buffer->metadata_.planes.size()) {
> > > +                   LOG(V4L2, Error)
> >
> > Are we going to 'leak' queued FrameBuffers at this point?
> > Is there a way to prevent this occurring at queue time rather than dequeue?
> >
> > In fact, Can it even happen? or should it be an ASSERT/Fatal?
>
> It's not supposed to happen, I'd say it's a kernel bug in that case. But
> it's a good point, I think we can return the buffer, and mark its status
> as erroneous. I'll do so.
>
> > > +                           << "Invalid number of planes (" << buf.length
> > > +                           << " != " << buffer->metadata_.planes.size() << ")";
> > > +                   return nullptr;
> > > +           }
> > > +
> > >             for (unsigned int nplane = 0; nplane < buf.length; nplane++)
> > > -                   buffer->metadata_.planes.push_back({ planes[nplane].bytesused });
> > > +                   buffer->metadata_.planes[nplane].bytesused = planes[nplane].bytesused;
> > >     } else {
> > > -           buffer->metadata_.planes.push_back({ buf.bytesused });
> > > +           buffer->metadata_.planes[0].bytesused = buf.bytesused;

Is metadata_.planes[1+] kept unfilled? Shall we execute
metadata_.planes.resize(1)?

Regards,
-Hiro
> > >     }
> > >
> > >     return buffer;
> > >
>
> --
> Regards,
>
> Laurent Pinchart