| Message ID | 20210902042303.2254-6-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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@ideasonboard.com> > --- > 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 ... But either way, Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > + for (const auto &plane : planes_) > + ASSERT(plane.offset != Plane::kInvalidOffset); > } > > /** >
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@ideasonboard.com> > > --- > > 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@ideasonboard.com> > > > + for (const auto &plane : planes_) > > + ASSERT(plane.offset != Plane::kInvalidOffset); > > } > > > > /**
Hi Laurent, thank you for the patch. On Fri, Sep 3, 2021 at 3:03 AM Laurent Pinchart <laurent.pinchart@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@ideasonboard.com> Reviewed-by: Hirokazu Honda <hiroh@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@ideasonboard.com> > > > > > + for (const auto &plane : planes_) > > > + ASSERT(plane.offset != Plane::kInvalidOffset); > > > } > > > > > > /** > > -- > Regards, > > Laurent Pinchart
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 */ + for (const auto &plane : planes_) + ASSERT(plane.offset != Plane::kInvalidOffset); } /**
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. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- include/libcamera/framebuffer.h | 9 +-------- src/libcamera/framebuffer.cpp | 3 +++ 2 files changed, 4 insertions(+), 8 deletions(-)