[libcamera-devel] [PATCH v7 1/6] Allow inheritance of FrameBuffer

[Laurent Pinchart]

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 at 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 at chromium.org>

Assuming the other patches in the series that require this change are
fine,

Reviewed-by: Laurent Pinchart <laurent.pinchart at 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;

-- 
Regards,

Laurent Pinchart