| Message ID | 20221201092733.2042078-2-chenghaoyang@google.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Harvey, Thank you for the patch. On Thu, Dec 01, 2022 at 09:27:28AM +0000, Harvey Yang via libcamera-devel wrote: > From: Harvey Yang <chenghaoyang@chromium.org> > > To add buffer_handle_t access in android, this patch allows inheritance > of FrameBuffer to add a derived class in android. > > Signed-off-by: Harvey Yang <chenghaoyang@chromium.org> Assuming the other patches in the series that require this change are fine, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> I'm interested to see where this will lead us, as it's an interesting semantics change. I'll copy here the comments I made in the review of v3 if anyone is interested in studying the philosophy of libcamera: This particular patch changes the meaning of the FrameBuffer class quite fundamentally. FrameBuffer, so far, has been an object that groups together data representing a frame buffe, but an instance of the class *isn't* a frame buffer. The distinction is important, the frame buffer is the memory in which frames are stored, and the FrameBuffer class stores handles to that memory, but it doesn't manage the memory. When using the FrameBufferAllocator this line is blurred, as the FrameBuffer instances end up storing the only instance of the dmabuf fds, so when a FrameBuffer is deleted, the memory is freed. This may have been a design mistake, I'm not entirely sure, but in any case, inheriting from FrameBuffer would allow creating frame buffer objects that *are* a frame buffer, in addition to being used as handles for frame buffers. I think it's an interesting change, but I'm not sure to fully see all the implications this will have on the API. > --- > include/libcamera/framebuffer.h | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/include/libcamera/framebuffer.h b/include/libcamera/framebuffer.h > index 69553999..61244829 100644 > --- a/include/libcamera/framebuffer.h > +++ b/include/libcamera/framebuffer.h > @@ -46,7 +46,7 @@ private: > std::vector<Plane> planes_; > }; > > -class FrameBuffer final : public Extensible > +class FrameBuffer : public Extensible > { > LIBCAMERA_DECLARE_PRIVATE() > > @@ -60,6 +60,7 @@ public: > > FrameBuffer(const std::vector<Plane> &planes, unsigned int cookie = 0); > FrameBuffer(std::unique_ptr<Private> d); > + virtual ~FrameBuffer() {} > > const std::vector<Plane> &planes() const; > Request *request() const;
diff --git a/include/libcamera/framebuffer.h b/include/libcamera/framebuffer.h index 69553999..61244829 100644 --- a/include/libcamera/framebuffer.h +++ b/include/libcamera/framebuffer.h @@ -46,7 +46,7 @@ private: std::vector<Plane> planes_; }; -class FrameBuffer final : public Extensible +class FrameBuffer : public Extensible { LIBCAMERA_DECLARE_PRIVATE() @@ -60,6 +60,7 @@ public: FrameBuffer(const std::vector<Plane> &planes, unsigned int cookie = 0); FrameBuffer(std::unique_ptr<Private> d); + virtual ~FrameBuffer() {} const std::vector<Plane> &planes() const; Request *request() const;