<div dir="ltr"><div>Hi Laurent,</div><div><br></div><div><div>Sorry I forgot to send this reply before going on vacation...</div><div><br></div></div>Thanks for your detailed review! I spent quite some time to update my patches, so v4 is a bit delayed...<div>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.</div><div><br></div><div>Updated "CL" to "patch".<div><div><br></div><div>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?</div><div><br></div><div>BR,</div><div>Harvey</div></div></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Apr 27, 2022 at 5:55 AM Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com" target="_blank">laurent.pinchart@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Harvey,<br>
<br>
Thank you for the patch.<br>
<br>
On Tue, Apr 26, 2022 at 09:14:06AM +0000, Harvey Yang via libcamera-devel wrote:<br>
> To add buffer_handle_t access in android, this CL allows inheritance of<br>
<br>
We use the term "patch" or "change", not "CL". Apart from that, the<br>
patch looks good, assuming review of the subsequent patches don't reveal<br>
issues that affect this one.<br>
<br>
On a side note, if we allow inheriting from the FrameBuffer class, I<br>
wonder if we should drop the cookie() and setCookie() functions. Users<br>
of the class that need to associate private data with it can always<br>
inherit to extend FrameBuffer. That's a candidate for a separate change<br>
of course, but what does everybody think ?<br>
<br>
> FrameBuffer to add a derived class in android.<br>
> <br>
> Signed-off-by: Harvey Yang <chenghaoyang at <a href="http://chromium.org" rel="noreferrer" target="_blank">chromium.org</a>><br>
> ---<br>
>  include/libcamera/framebuffer.h | 3 ++-<br>
>  1 file changed, 2 insertions(+), 1 deletion(-)<br>
> <br>
> diff --git a/include/libcamera/framebuffer.h b/include/libcamera/framebuffer.h<br>
> index 3b1118d1..e6c769b4 100644<br>
> --- a/include/libcamera/framebuffer.h<br>
> +++ b/include/libcamera/framebuffer.h<br>
> @@ -46,7 +46,7 @@ private:<br>
>       std::vector<Plane> planes_;<br>
>  };<br>
>  <br>
> -class FrameBuffer final : public Extensible<br>
> +class FrameBuffer : public Extensible<br>
>  {<br>
>       LIBCAMERA_DECLARE_PRIVATE()<br>
>  <br>
> @@ -61,6 +61,7 @@ public:<br>
>       FrameBuffer(const std::vector<Plane> &planes, unsigned int cookie = 0);<br>
>       FrameBuffer(std::unique_ptr<Private> d,<br>
>                   const std::vector<Plane> &planes, unsigned int cookie = 0);<br>
> +     virtual ~FrameBuffer() {}<br>
>  <br>
>       const std::vector<Plane> &planes() const { return planes_; }<br>
>       Request *request() const;<br>
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
</blockquote></div>