| Message ID | 20220426091409.1352047-2-chenghaoyang@chromium.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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;
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@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 >
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;
Thanks Laurent for the clear explanation! On Thu, Sep 1, 2022 at 9:42 PM Laurent Pinchart < laurent.pinchart@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 >
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;
To add buffer_handle_t access in android, this CL allows inheritance of 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(-)