[libcamera-devel] [PATCH v3 1/4] Allow inheritance of FrameBuffer
Cheng-Hao Yang
chenghaoyang at chromium.org
Fri Sep 2 12:48:08 CEST 2022
Thanks Laurent for the clear explanation!
On Thu, Sep 1, 2022 at 9:42 PM Laurent Pinchart <
laurent.pinchart at ideasonboard.com> wrote:
> 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.
>
>
I agree with you that the purpose of the data "cookie" in base classes
Request and FrameBuffer is unclear, and it's not used by other base
classes. Android adapter even stores the memory address of
Camera3RequestDescriptor into a Request's cookie.
A pretty common use case of FrameBuffer's cookie is the buffer id
identified between the pipeline handler and IPA, which is defined as
libcamera::IPABuffer::id. However, we should specifically define
FrameBuffer's member variable "id", instead of utilizing the opaque member
variable "cookie". Or we can simply leave it to pipeline handlers to store
the mappings with something like std::map.
If others also agree with this, I can help with the migration and remove
the member variable "cookie" in Request & FrameBuffer for good :)
> > 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20220902/84cc89f8/attachment.htm>
More information about the libcamera-devel
mailing list