| Message ID | 20210816043138.957984-11-hiroh@chromium.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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@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. > 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 ? > + for (const auto &plane : planes_) > + assert(plane.offset != Plane::kInvalidOffset); > + return planes_; > + } > > Request *request() const; > const FrameMetadata &metadata() const { return metadata_; }
Hi Laurent, On Wed, Aug 18, 2021 at 10:42 AM Laurent Pinchart <laurent.pinchart@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@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
Hi Hiro, On Fri, Aug 20, 2021 at 08:18:47PM +0900, Hirokazu Honda wrote: > On Wed, Aug 18, 2021 at 10:42 AM Laurent Pinchart wrote: > > 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@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. Then let's write /* \todo Remove the assertions after sufficient testing */ > > > + for (const auto &plane : planes_) > > > + assert(plane.offset != Plane::kInvalidOffset); > > > + return planes_; > > > + } > > > > > > Request *request() const; > > > const FrameMetadata &metadata() const { return metadata_; }
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(); 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. */ + for (const auto &plane : planes_) + assert(plane.offset != Plane::kInvalidOffset); + return planes_; + } Request *request() const; const FrameMetadata &metadata() const { return metadata_; }
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@chromium.org> --- include/libcamera/framebuffer.h | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)