[libcamera-devel] [RFC PATCH 10/10] libcamera: framebuffer: Add assertion to detect offset is unfilled

[Hirokazu Honda]

Hi Laurent,

On Wed, Aug 18, 2021 at 10:42 AM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Hiro,
>
> Thank you for the patch.
>
> On Mon, Aug 16, 2021 at 01:31:38PM +0900, Hirokazu Honda wrote:
> > The offset variable is introduced to FrameBuffer::Plane. In order to
> > detect that the plane is used while the offset is not set, this adds
> > the assertion to FrameBuffer::planes(). It should be removed in the
> > future.
> >
> > Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> > ---
> >  include/libcamera/framebuffer.h | 13 +++++++++++--
> >  1 file changed, 11 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/libcamera/framebuffer.h b/include/libcamera/framebuffer.h
> > index 5de3c744..79541557 100644
> > --- a/include/libcamera/framebuffer.h
> > +++ b/include/libcamera/framebuffer.h
> > @@ -7,6 +7,8 @@
> >  #ifndef __LIBCAMERA_FRAMEBUFFER_H__
> >  #define __LIBCAMERA_FRAMEBUFFER_H__
> >
> > +#include <assert.h>
> > +#include <limits>
> >  #include <stdint.h>
> >  #include <vector>
> >
> > @@ -41,14 +43,21 @@ class FrameBuffer final : public Extensible
> >
> >  public:
> >       struct Plane {
> > +             static constexpr unsigned int kInvalidOffset = std::numeric_limits<unsigned int>::max();
>
> Maybe a blank line here ?
>
> This also generates a doxygen warning as kInvalidOffset is not
> documented. As it's not meant to be used by anyone else than the code
> below, this could be addressed by making the constant private.
>

I couldn't move it to private because it is used in the below assert.
I document the variable.

> >               FileDescriptor fd;
> > -             unsigned int offset;
> > +             unsigned int offset = kInvalidOffset;
> >               unsigned int length;
> >       };
> >
> >       FrameBuffer(const std::vector<Plane> &planes, unsigned int cookie = 0);
> >
> > -     const std::vector<Plane> &planes() const { return planes_; }
> > +     const std::vector<Plane> &planes() const
> > +     {
> > +             /* \todo remove this assert. */
>
> When do you foresee this being removed ?
>

Maybe for a month. Just an arbitrary foresee.

Best Regards,
-Hiro
> > +             for (const auto &plane : planes_)
> > +                     assert(plane.offset != Plane::kInvalidOffset);
> > +             return planes_;
> > +     }
> >
> >       Request *request() const;
> >       const FrameMetadata &metadata() const { return metadata_; }
>
> --
> Regards,
>
> Laurent Pinchart