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

[Laurent Pinchart]

Hi Hiro,

On Fri, Sep 03, 2021 at 08:29:19PM +0900, Hirokazu Honda wrote:
> On Fri, Sep 3, 2021 at 3:11 AM Laurent Pinchart wrote:
> > 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;
> };

That should go in another patch, which should then also initialize the
other members of the metadata.

> > > > +
> > > >     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)?

The whole point of this patch is to make sure that FrameBuffer::planes_
will always have the same size as FrameMetadata::planes_. As the former
never changes after the construction of the FrameBuffer, the latter
should never change either. In this patch we only update bytesused in
the first plane indeed, but later in the series that separate issue is
addressed.

> > > >     }
> > > >
> > > >     return buffer;
> > > >

-- 
Regards,

Laurent Pinchart