| Message ID | 20200909155457.153907-4-jacopo@jmondi.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, Thanks for your work. On 2020-09-09 17:54:52 +0200, Jacopo Mondi wrote: > Add a clear() method to the FrameBufferAllocator class that > frees all the buffers previously reserved by the allocator. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > include/libcamera/framebuffer_allocator.h | 1 + > src/libcamera/framebuffer_allocator.cpp | 8 ++++++++ > 2 files changed, 9 insertions(+) > > diff --git a/include/libcamera/framebuffer_allocator.h b/include/libcamera/framebuffer_allocator.h > index 78f1353964eb..2a4d538a0cb2 100644 > --- a/include/libcamera/framebuffer_allocator.h > +++ b/include/libcamera/framebuffer_allocator.h > @@ -28,6 +28,7 @@ public: > > int allocate(Stream *stream); > int free(Stream *stream); > + void clear(); > > bool allocated() const { return !buffers_.empty(); } > const std::vector<std::unique_ptr<FrameBuffer>> &buffers(Stream *stream) const; > diff --git a/src/libcamera/framebuffer_allocator.cpp b/src/libcamera/framebuffer_allocator.cpp > index 2fbba37a1b0b..7ed80011c845 100644 > --- a/src/libcamera/framebuffer_allocator.cpp > +++ b/src/libcamera/framebuffer_allocator.cpp > @@ -125,6 +125,14 @@ int FrameBufferAllocator::free(Stream *stream) > return 0; > } > > +/** > + * \brief Free all the buffers previously allocated > + */ > +void FrameBufferAllocator::clear() > +{ > + buffers_.clear(); > +} > + > /** > * \fn FrameBufferAllocator::allocated() > * \brief Check if the allocator has allocated buffers for any stream > -- > 2.28.0 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
On Thu, Sep 10, 2020 at 8:19 PM Niklas Söderlund <niklas.soderlund@ragnatech.se> wrote: > > Hi Jacopo, > > Thanks for your work. > > On 2020-09-09 17:54:52 +0200, Jacopo Mondi wrote: > > Add a clear() method to the FrameBufferAllocator class that > > frees all the buffers previously reserved by the allocator. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > > --- > > include/libcamera/framebuffer_allocator.h | 1 + > > src/libcamera/framebuffer_allocator.cpp | 8 ++++++++ > > 2 files changed, 9 insertions(+) > > > > diff --git a/include/libcamera/framebuffer_allocator.h b/include/libcamera/framebuffer_allocator.h > > index 78f1353964eb..2a4d538a0cb2 100644 > > --- a/include/libcamera/framebuffer_allocator.h > > +++ b/include/libcamera/framebuffer_allocator.h > > @@ -28,6 +28,7 @@ public: > > > > int allocate(Stream *stream); > > int free(Stream *stream); > > + void clear(); > > > > bool allocated() const { return !buffers_.empty(); } > > const std::vector<std::unique_ptr<FrameBuffer>> &buffers(Stream *stream) const; > > diff --git a/src/libcamera/framebuffer_allocator.cpp b/src/libcamera/framebuffer_allocator.cpp > > index 2fbba37a1b0b..7ed80011c845 100644 > > --- a/src/libcamera/framebuffer_allocator.cpp > > +++ b/src/libcamera/framebuffer_allocator.cpp > > @@ -125,6 +125,14 @@ int FrameBufferAllocator::free(Stream *stream) > > return 0; > > } > > > > +/** > > + * \brief Free all the buffers previously allocated > > + */ > > +void FrameBufferAllocator::clear() > > +{ > > + buffers_.clear(); > > +} > > + This is the first time I looked at FrameBufferAllocator. Those comments are not so related to this patch. I didn't get the difference between free() and clear() from the name. I would rename free() to erase(). If it sounds good to you, can I ask you to do it later? Besides, clear() in dtor seems to be unnecessary. Reviewed-by: Hirokazu Honda <hiroh@chromium.org> > > /** > > * \fn FrameBufferAllocator::allocated() > > * \brief Check if the allocator has allocated buffers for any stream > > -- > > 2.28.0 > > > > _______________________________________________ > > libcamera-devel mailing list > > libcamera-devel@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel > > -- > Regards, > Niklas Söderlund > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Hiro, On Fri, Sep 11, 2020 at 01:42:58PM +0900, Hirokazu Honda wrote: > On Thu, Sep 10, 2020 at 8:19 PM Niklas Söderlund > <niklas.soderlund@ragnatech.se> wrote: > > > > Hi Jacopo, > > > > Thanks for your work. > > > > On 2020-09-09 17:54:52 +0200, Jacopo Mondi wrote: > > > Add a clear() method to the FrameBufferAllocator class that > > > frees all the buffers previously reserved by the allocator. > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > > > > --- > > > include/libcamera/framebuffer_allocator.h | 1 + > > > src/libcamera/framebuffer_allocator.cpp | 8 ++++++++ > > > 2 files changed, 9 insertions(+) > > > > > > diff --git a/include/libcamera/framebuffer_allocator.h b/include/libcamera/framebuffer_allocator.h > > > index 78f1353964eb..2a4d538a0cb2 100644 > > > --- a/include/libcamera/framebuffer_allocator.h > > > +++ b/include/libcamera/framebuffer_allocator.h > > > @@ -28,6 +28,7 @@ public: > > > > > > int allocate(Stream *stream); > > > int free(Stream *stream); > > > + void clear(); > > > > > > bool allocated() const { return !buffers_.empty(); } > > > const std::vector<std::unique_ptr<FrameBuffer>> &buffers(Stream *stream) const; > > > diff --git a/src/libcamera/framebuffer_allocator.cpp b/src/libcamera/framebuffer_allocator.cpp > > > index 2fbba37a1b0b..7ed80011c845 100644 > > > --- a/src/libcamera/framebuffer_allocator.cpp > > > +++ b/src/libcamera/framebuffer_allocator.cpp > > > @@ -125,6 +125,14 @@ int FrameBufferAllocator::free(Stream *stream) > > > return 0; > > > } > > > > > > +/** > > > + * \brief Free all the buffers previously allocated > > > + */ > > > +void FrameBufferAllocator::clear() > > > +{ > > > + buffers_.clear(); > > > +} > > > + > > This is the first time I looked at FrameBufferAllocator. > Those comments are not so related to this patch. > I didn't get the difference between free() and clear() from the name. > I would rename free() to erase(). If it sounds good to you, can I ask > you to do it later? Being this a map underneath, using erase would align with std::map > Besides, clear() in dtor seems to be unnecessary. > You mean the one it's introduced in the next patch ? Probably not as the newly added availableBuffers_ vector is just a vector of pointers, there's not need to release memory... > Reviewed-by: Hirokazu Honda <hiroh@chromium.org> > > > > /** > > > * \fn FrameBufferAllocator::allocated() > > > * \brief Check if the allocator has allocated buffers for any stream > > > -- > > > 2.28.0 > > > > > > _______________________________________________ > > > libcamera-devel mailing list > > > libcamera-devel@lists.libcamera.org > > > https://lists.libcamera.org/listinfo/libcamera-devel > > > > -- > > Regards, > > Niklas Söderlund > > _______________________________________________ > > libcamera-devel mailing list > > libcamera-devel@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel
Hello, On Fri, Sep 11, 2020 at 06:31:08PM +0200, Jacopo Mondi wrote: > On Fri, Sep 11, 2020 at 01:42:58PM +0900, Hirokazu Honda wrote: > > On Thu, Sep 10, 2020 at 8:19 PM Niklas Söderlund wrote: > > > On 2020-09-09 17:54:52 +0200, Jacopo Mondi wrote: > > > > Add a clear() method to the FrameBufferAllocator class that > > > > frees all the buffers previously reserved by the allocator. > > > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > > > > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > > > > > > --- > > > > include/libcamera/framebuffer_allocator.h | 1 + > > > > src/libcamera/framebuffer_allocator.cpp | 8 ++++++++ > > > > 2 files changed, 9 insertions(+) > > > > > > > > diff --git a/include/libcamera/framebuffer_allocator.h b/include/libcamera/framebuffer_allocator.h > > > > index 78f1353964eb..2a4d538a0cb2 100644 > > > > --- a/include/libcamera/framebuffer_allocator.h > > > > +++ b/include/libcamera/framebuffer_allocator.h > > > > @@ -28,6 +28,7 @@ public: > > > > > > > > int allocate(Stream *stream); > > > > int free(Stream *stream); > > > > + void clear(); > > > > > > > > bool allocated() const { return !buffers_.empty(); } > > > > const std::vector<std::unique_ptr<FrameBuffer>> &buffers(Stream *stream) const; > > > > diff --git a/src/libcamera/framebuffer_allocator.cpp b/src/libcamera/framebuffer_allocator.cpp > > > > index 2fbba37a1b0b..7ed80011c845 100644 > > > > --- a/src/libcamera/framebuffer_allocator.cpp > > > > +++ b/src/libcamera/framebuffer_allocator.cpp > > > > @@ -125,6 +125,14 @@ int FrameBufferAllocator::free(Stream *stream) > > > > return 0; > > > > } > > > > > > > > +/** > > > > + * \brief Free all the buffers previously allocated > > > > + */ > > > > +void FrameBufferAllocator::clear() > > > > +{ > > > > + buffers_.clear(); > > > > +} > > > > + > > > > This is the first time I looked at FrameBufferAllocator. > > Those comments are not so related to this patch. > > I didn't get the difference between free() and clear() from the name. > > I would rename free() to erase(). If it sounds good to you, can I ask > > you to do it later? > > Being this a map underneath, using erase would align with std::map The free() operation is the counterpart of allocate(), I don't think allocate/erase would be a very good pair. I'd rather keep free(), or rename allocate too. Regarding clear(), I don't think that's a very intuitive name. freeAll() would be better. > > Besides, clear() in dtor seems to be unnecessary. > > You mean the one it's introduced in the next patch ? Probably not as > the newly added availableBuffers_ vector is just a vector of pointers, > there's not need to release memory... > > > Reviewed-by: Hirokazu Honda <hiroh@chromium.org> > > > > > > /** > > > > * \fn FrameBufferAllocator::allocated() > > > > * \brief Check if the allocator has allocated buffers for any stream
Hi Laurent, On Tue, Sep 15, 2020 at 04:41:48AM +0300, Laurent Pinchart wrote: > Hello, > > On Fri, Sep 11, 2020 at 06:31:08PM +0200, Jacopo Mondi wrote: > > On Fri, Sep 11, 2020 at 01:42:58PM +0900, Hirokazu Honda wrote: > > > On Thu, Sep 10, 2020 at 8:19 PM Niklas Söderlund wrote: > > > > On 2020-09-09 17:54:52 +0200, Jacopo Mondi wrote: > > > > > Add a clear() method to the FrameBufferAllocator class that > > > > > frees all the buffers previously reserved by the allocator. > > > > > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > > > > > > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > > > > > > > > --- > > > > > include/libcamera/framebuffer_allocator.h | 1 + > > > > > src/libcamera/framebuffer_allocator.cpp | 8 ++++++++ > > > > > 2 files changed, 9 insertions(+) > > > > > > > > > > diff --git a/include/libcamera/framebuffer_allocator.h b/include/libcamera/framebuffer_allocator.h > > > > > index 78f1353964eb..2a4d538a0cb2 100644 > > > > > --- a/include/libcamera/framebuffer_allocator.h > > > > > +++ b/include/libcamera/framebuffer_allocator.h > > > > > @@ -28,6 +28,7 @@ public: > > > > > > > > > > int allocate(Stream *stream); > > > > > int free(Stream *stream); > > > > > + void clear(); > > > > > > > > > > bool allocated() const { return !buffers_.empty(); } > > > > > const std::vector<std::unique_ptr<FrameBuffer>> &buffers(Stream *stream) const; > > > > > diff --git a/src/libcamera/framebuffer_allocator.cpp b/src/libcamera/framebuffer_allocator.cpp > > > > > index 2fbba37a1b0b..7ed80011c845 100644 > > > > > --- a/src/libcamera/framebuffer_allocator.cpp > > > > > +++ b/src/libcamera/framebuffer_allocator.cpp > > > > > @@ -125,6 +125,14 @@ int FrameBufferAllocator::free(Stream *stream) > > > > > return 0; > > > > > } > > > > > > > > > > +/** > > > > > + * \brief Free all the buffers previously allocated > > > > > + */ > > > > > +void FrameBufferAllocator::clear() > > > > > +{ > > > > > + buffers_.clear(); > > > > > +} > > > > > + > > > > > > This is the first time I looked at FrameBufferAllocator. > > > Those comments are not so related to this patch. > > > I didn't get the difference between free() and clear() from the name. > > > I would rename free() to erase(). If it sounds good to you, can I ask > > > you to do it later? > > > > Being this a map underneath, using erase would align with std::map > > The free() operation is the counterpart of allocate(), I don't think > allocate/erase would be a very good pair. I'd rather keep free(), or > rename allocate too. Ok. Even thoughI think erase() is in line with the std naming scheme which we try to respect when possible. > > Regarding clear(), I don't think that's a very intuitive name. freeAll() > would be better. What's not intuitive in clear() ? All the std containers have a clear() method with the same description: "Clear the content". Isn't that what it happens here? I would agree more on this if there was any explicity buffer releasing operation, but since memory release is a consequence of deleting the FrameBuffer instances that wraps the dmabuf fds, what we actually do is just call clear() on the buffes_ map. freeAll() is more clunky to me. I'll keep clear() and free(). Thanks j > > > > Besides, clear() in dtor seems to be unnecessary. > > > > You mean the one it's introduced in the next patch ? Probably not as > > the newly added availableBuffers_ vector is just a vector of pointers, > > there's not need to release memory... > > > > > Reviewed-by: Hirokazu Honda <hiroh@chromium.org> > > > > > > > > /** > > > > > * \fn FrameBufferAllocator::allocated() > > > > > * \brief Check if the allocator has allocated buffers for any stream > > -- > Regards, > > Laurent Pinchart
Hi Jacopo, On Thu, Sep 17, 2020 at 12:41:55PM +0200, Jacopo Mondi wrote: > On Tue, Sep 15, 2020 at 04:41:48AM +0300, Laurent Pinchart wrote: > > On Fri, Sep 11, 2020 at 06:31:08PM +0200, Jacopo Mondi wrote: > > > On Fri, Sep 11, 2020 at 01:42:58PM +0900, Hirokazu Honda wrote: > > > > On Thu, Sep 10, 2020 at 8:19 PM Niklas Söderlund wrote: > > > > > On 2020-09-09 17:54:52 +0200, Jacopo Mondi wrote: > > > > > > Add a clear() method to the FrameBufferAllocator class that > > > > > > frees all the buffers previously reserved by the allocator. > > > > > > > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > > > > > > > > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > > > > > > > > > > --- > > > > > > include/libcamera/framebuffer_allocator.h | 1 + > > > > > > src/libcamera/framebuffer_allocator.cpp | 8 ++++++++ > > > > > > 2 files changed, 9 insertions(+) > > > > > > > > > > > > diff --git a/include/libcamera/framebuffer_allocator.h b/include/libcamera/framebuffer_allocator.h > > > > > > index 78f1353964eb..2a4d538a0cb2 100644 > > > > > > --- a/include/libcamera/framebuffer_allocator.h > > > > > > +++ b/include/libcamera/framebuffer_allocator.h > > > > > > @@ -28,6 +28,7 @@ public: > > > > > > > > > > > > int allocate(Stream *stream); > > > > > > int free(Stream *stream); > > > > > > + void clear(); > > > > > > > > > > > > bool allocated() const { return !buffers_.empty(); } > > > > > > const std::vector<std::unique_ptr<FrameBuffer>> &buffers(Stream *stream) const; > > > > > > diff --git a/src/libcamera/framebuffer_allocator.cpp b/src/libcamera/framebuffer_allocator.cpp > > > > > > index 2fbba37a1b0b..7ed80011c845 100644 > > > > > > --- a/src/libcamera/framebuffer_allocator.cpp > > > > > > +++ b/src/libcamera/framebuffer_allocator.cpp > > > > > > @@ -125,6 +125,14 @@ int FrameBufferAllocator::free(Stream *stream) > > > > > > return 0; > > > > > > } > > > > > > > > > > > > +/** > > > > > > + * \brief Free all the buffers previously allocated > > > > > > + */ > > > > > > +void FrameBufferAllocator::clear() > > > > > > +{ > > > > > > + buffers_.clear(); > > > > > > +} > > > > > > + > > > > > > > > This is the first time I looked at FrameBufferAllocator. > > > > Those comments are not so related to this patch. > > > > I didn't get the difference between free() and clear() from the name. > > > > I would rename free() to erase(). If it sounds good to you, can I ask > > > > you to do it later? > > > > > > Being this a map underneath, using erase would align with std::map > > > > The free() operation is the counterpart of allocate(), I don't think > > allocate/erase would be a very good pair. I'd rather keep free(), or > > rename allocate too. > > Ok. Even thoughI think erase() is in line with the std naming scheme > which we try to respect when possible. Sorry, I meant allocate/free. > > Regarding clear(), I don't think that's a very intuitive name. freeAll() > > would be better. > > What's not intuitive in clear() ? All the std containers have a > clear() method with the same description: "Clear the content". > Isn't that what it happens here? But this isn't a container, it's an allocator... > I would agree more on this if there was any explicity buffer releasing > operation, but since memory release is a consequence of deleting the > FrameBuffer instances that wraps the dmabuf fds, what we actually do > is just call clear() on the buffes_ map. > > freeAll() is more clunky to me. > > I'll keep clear() and free(). > > > > > Besides, clear() in dtor seems to be unnecessary. > > > > > > You mean the one it's introduced in the next patch ? Probably not as > > > the newly added availableBuffers_ vector is just a vector of pointers, > > > there's not need to release memory... > > > > > > > Reviewed-by: Hirokazu Honda <hiroh@chromium.org> > > > > > > > > > > /** > > > > > > * \fn FrameBufferAllocator::allocated() > > > > > > * \brief Check if the allocator has allocated buffers for any stream
diff --git a/include/libcamera/framebuffer_allocator.h b/include/libcamera/framebuffer_allocator.h index 78f1353964eb..2a4d538a0cb2 100644 --- a/include/libcamera/framebuffer_allocator.h +++ b/include/libcamera/framebuffer_allocator.h @@ -28,6 +28,7 @@ public: int allocate(Stream *stream); int free(Stream *stream); + void clear(); bool allocated() const { return !buffers_.empty(); } const std::vector<std::unique_ptr<FrameBuffer>> &buffers(Stream *stream) const; diff --git a/src/libcamera/framebuffer_allocator.cpp b/src/libcamera/framebuffer_allocator.cpp index 2fbba37a1b0b..7ed80011c845 100644 --- a/src/libcamera/framebuffer_allocator.cpp +++ b/src/libcamera/framebuffer_allocator.cpp @@ -125,6 +125,14 @@ int FrameBufferAllocator::free(Stream *stream) return 0; } +/** + * \brief Free all the buffers previously allocated + */ +void FrameBufferAllocator::clear() +{ + buffers_.clear(); +} + /** * \fn FrameBufferAllocator::allocated() * \brief Check if the allocator has allocated buffers for any stream
Add a clear() method to the FrameBufferAllocator class that frees all the buffers previously reserved by the allocator. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- include/libcamera/framebuffer_allocator.h | 1 + src/libcamera/framebuffer_allocator.cpp | 8 ++++++++ 2 files changed, 9 insertions(+)