[libcamera-devel] [RFC PATCH v1 05/12] libcamera: framebuffer: Move planes check to constructor

[Hirokazu Honda]

Hi Laurent, thank you for the patch.

On Fri, Sep 3, 2021 at 3:03 AM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Kieran,
>
> On Thu, Sep 02, 2021 at 10:27:51AM +0100, Kieran Bingham wrote:
> > On 02/09/2021 05:22, Laurent Pinchart wrote:
> > > The FrameBuffer::planes() function checks that planes are correctly
> > > initialized with an offset. This can be done at construction time
> > > instead, as the planes are constant. The runtime overhead is reduced,
> > > and the backtrace generated by the assertion will show where the faulty
> > > frame buffer is created instead of where it is used, easing debugging.
> >
> > That sounds very enticing...
> >
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>


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

-Hiro
> > > ---
> > >  include/libcamera/framebuffer.h | 9 +--------
> > >  src/libcamera/framebuffer.cpp   | 3 +++
> > >  2 files changed, 4 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/include/libcamera/framebuffer.h b/include/libcamera/framebuffer.h
> > > index d5aeff00387b..fd68ed0a139d 100644
> > > --- a/include/libcamera/framebuffer.h
> > > +++ b/include/libcamera/framebuffer.h
> > > @@ -51,14 +51,7 @@ public:
> > >
> > >     FrameBuffer(const std::vector<Plane> &planes, unsigned int cookie = 0);
> > >
> > > -   const std::vector<Plane> &planes() const
> > > -   {
> > > -           /* \todo Remove the assertions after sufficient testing */
> > > -           for (const auto &plane : planes_)
> > > -                   assert(plane.offset != Plane::kInvalidOffset);
> > > -           return planes_;
> > > -   }
> > > -
> > > +   const std::vector<Plane> &planes() const { return planes_; }
> > >     Request *request() const;
> > >     const FrameMetadata &metadata() const { return metadata_; }
> > >
> > > diff --git a/src/libcamera/framebuffer.cpp b/src/libcamera/framebuffer.cpp
> > > index c99f5b15e6ff..53ef89bf458f 100644
> > > --- a/src/libcamera/framebuffer.cpp
> > > +++ b/src/libcamera/framebuffer.cpp
> > > @@ -199,6 +199,9 @@ FrameBuffer::FrameBuffer(const std::vector<Plane> &planes, unsigned int cookie)
> > >     : Extensible(std::make_unique<Private>()), planes_(planes),
> > >       cookie_(cookie)
> > >  {
> > > +   /* \todo Remove the assertions after sufficient testing */
> >
> > As a construction time validation event, which is compiled/optimised out
> > in release builds anyway, I'm tempted to remove the todo here and keep
> > the ASSERT ...
>
> I agree, I'll drop the comment and explain why in the commit message:
>
> libcamera: framebuffer: Move planes check to constructor
>
> The FrameBuffer::planes() function checks that planes are correctly
> initialized with an offset. This can be done at construction time
> instead, as the planes are constant. The backtrace generated by the
> assertion will show where the faulty frame buffer is created instead of
> where it is used, easing debugging.
>
> As the runtime overhead is reduced, there's no real need to drop the
> assertion in the future anymore, it can be useful to ensure that the
> planes are correctly populated by the caller. Drop the comment that
> calls for removing the check.
>
> > But either way,
> >
> > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> >
> > > +   for (const auto &plane : planes_)
> > > +           ASSERT(plane.offset != Plane::kInvalidOffset);
> > >  }
> > >
> > >  /**
>
> --
> Regards,
>
> Laurent Pinchart