[libcamera-devel] [PATCH v3 1/4] Allow inheritance of FrameBuffer

[Laurent Pinchart]

Hi Harvey,

Reviving an old discussion.

On Tue, May 03, 2022 at 11:29:40AM +0800, Cheng-Hao Yang wrote:
> Hi Laurent,
> 
> Sorry I forgot to send this reply before going on vacation...
> 
> Thanks for your detailed review! I spent quite some time to update my
> patches, so v4 is a bit delayed...
> I used ts=4 in previous patches. Now I use 8 instead, so the new version
> might look a bit different from the previous ones.
> 
> Updated "CL" to "patch".
> 
> As for the cookie() and setCookie() functions, I notice that currently
> pipeline handlers use them a lot, and src/android/camera_device.cpp relies
> on the value (cookie() only, without setCookie(), as the cookie is supposed
> to be used). If it's used under src/android and src/libcamera/pipeline, I
> think the functions are worth being defined in the base class? I don't
> think I fully understand the code hierarchy though. What do others think?

The purpose of the FrameBuffer and Request cookies is to associate
private data with an instance of those classes.

The cookie is clearly documented as being opaque to libcamera, and set
by the creator of the Request or FrameBuffer. For the former, the
creator is an application, for the latter, it can be an application, or
an internal component inside libcamera (IPA modules and pipeline
handlers are mentioned in the documentation, adaptation layers such as
the Android camera HAL or the V4L2 adaptation layer cound as
applications from that point of view).

I think this may be a bit of an API design mistake. Generally speaking,
I don't think libcamera should try to imagine ancillary application
needs and provide support for them, in this specific case I think it
should be handled by applications (a trivial option would be a
std::map<Request *, T> or std::map<FrameBuffer *, T>).

For frame buffers, I would however differentiate between data that is
needed by the user of the frame buffer, and data that is needed by the
implementor of the frame buffer. An example of the former would be
applications mapping the frame buffer memory to the CPU and wanting to
associate that mapping with the FrameBuffer object for easy lookup. That
is something that I believe should be handled by the user itself (again,
maybe with a std::map). Data used by the implementor of the frame buffer
is what you're after here, you're creating frame buffers of a specific
kind, and thus want to inherit from them to store additional data in the
instance itself. Cookies are not needed for this.

The bottom line is that I don't think cookies are actually needed.

This being said, 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.

> On Wed, Apr 27, 2022 at 5:55 AM Laurent Pinchart wrote:
> > On Tue, Apr 26, 2022 at 09:14:06AM +0000, Harvey Yang via libcamera-devel wrote:
> > > To add buffer_handle_t access in android, this CL allows inheritance of
> >
> > We use the term "patch" or "change", not "CL". Apart from that, the
> > patch looks good, assuming review of the subsequent patches don't reveal
> > issues that affect this one.
> >
> > On a side note, if we allow inheriting from the FrameBuffer class, I
> > wonder if we should drop the cookie() and setCookie() functions. Users
> > of the class that need to associate private data with it can always
> > inherit to extend FrameBuffer. That's a candidate for a separate change
> > of course, but what does everybody think ?
> >
> > > FrameBuffer to add a derived class in android.
> > >
> > > Signed-off-by: Harvey Yang <chenghaoyang at chromium.org>
> > > ---
> > >  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 3b1118d1..e6c769b4 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()
> > >
> > > @@ -61,6 +61,7 @@ public:
> > >       FrameBuffer(const std::vector<Plane> &planes, unsigned int cookie = 0);
> > >       FrameBuffer(std::unique_ptr<Private> d,
> > >                   const std::vector<Plane> &planes, unsigned int cookie = 0);
> > > +     virtual ~FrameBuffer() {}
> > >
> > >       const std::vector<Plane> &planes() const { return planes_; }
> > >       Request *request() const;

-- 
Regards,

Laurent Pinchart