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

[Cheng-Hao Yang]

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?

BR,
Harvey

On Wed, Apr 27, 2022 at 5:55 AM Laurent Pinchart <
laurent.pinchart at ideasonboard.com> wrote:

> Hi Harvey,
>
> Thank you for the patch.
>
> 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/20220503/ee94976e/attachment.htm>