| Message ID | 20210421160319.42251-12-jacopo@jmondi.org |
|---|---|
| State | Accepted |
| Delegated to: | Jacopo Mondi |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, On 2021-04-21 18:03:14 +0200, Jacopo Mondi wrote: > I got fooled by the documentation of setRequest() implying that the > function is meant to be called by pipeline handlers only, which it is > used in the Request class at Request::addBuffer() and Request::reuse() > time. > > Rework the documentation to report that. > > Fixes: 125be3436ae0 ("libcamera: FrameBuffer: Add a setRequest() interface") In all fairness this was true when 125be3436ae0 was merged it only changed in dcc024760a8d that was merged a year later ;-) > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > src/libcamera/buffer.cpp | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp > index 75b2693281a7..5baccfa99f10 100644 > --- a/src/libcamera/buffer.cpp > +++ b/src/libcamera/buffer.cpp > @@ -191,8 +191,9 @@ FrameBuffer::FrameBuffer(const std::vector<Plane> &planes, unsigned int cookie) > * \brief Set the request this buffer belongs to > * \param[in] request Request to set > * > - * The intended callers of this method are pipeline handlers and only for > - * buffers that are internal to the pipeline. > + * For buffers added to requests by applications, this method is called by > + * Request::addBuffer() or Request::reuse(). For buffers internal to pipeline > + * handlers, it is called by the pipeline handlers themselves. > * > * \todo Shall be hidden from applications with a d-pointer design. > */ > -- > 2.31.1 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Jacopo, thank you for the patch. On Thu, Apr 22, 2021 at 3:47 AM Niklas Söderlund <niklas.soderlund@ragnatech.se> wrote: > > Hi Jacopo, > > On 2021-04-21 18:03:14 +0200, Jacopo Mondi wrote: > > I got fooled by the documentation of setRequest() implying that the > > function is meant to be called by pipeline handlers only, which it is > > used in the Request class at Request::addBuffer() and Request::reuse() > > time. > > > > Rework the documentation to report that. > > > > Fixes: 125be3436ae0 ("libcamera: FrameBuffer: Add a setRequest() interface") > > In all fairness this was true when 125be3436ae0 was merged it only > changed in dcc024760a8d that was merged a year later ;-) > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > > --- > > src/libcamera/buffer.cpp | 5 +++-- > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp > > index 75b2693281a7..5baccfa99f10 100644 > > --- a/src/libcamera/buffer.cpp > > +++ b/src/libcamera/buffer.cpp > > @@ -191,8 +191,9 @@ FrameBuffer::FrameBuffer(const std::vector<Plane> &planes, unsigned int cookie) > > * \brief Set the request this buffer belongs to > > * \param[in] request Request to set > > * > > - * The intended callers of this method are pipeline handlers and only for > > - * buffers that are internal to the pipeline. > > + * For buffers added to requests by applications, this method is called by > > + * Request::addBuffer() or Request::reuse(). For buffers internal to pipeline > > + * handlers, it is called by the pipeline handlers themselves. > > * > > * \todo Shall be hidden from applications with a d-pointer design. > > */ Is this todo still reasonable? It needs to be public so that a pipeline handler calls it? > > -- > > 2.31.1 > > Reviewed-by: Hirokazu Honda <hiroh@chromium.org> > > _______________________________________________ > > libcamera-devel mailing list > > libcamera-devel@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel > > -- > Regards, > Niklas Söderlund > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Hiro, On Thu, Apr 22, 2021 at 02:07:12PM +0900, Hirokazu Honda wrote: > Hi Jacopo, thank you for the patch. > > On Thu, Apr 22, 2021 at 3:47 AM Niklas Söderlund > <niklas.soderlund@ragnatech.se> wrote: > > > > Hi Jacopo, > > > > On 2021-04-21 18:03:14 +0200, Jacopo Mondi wrote: > > > I got fooled by the documentation of setRequest() implying that the > > > function is meant to be called by pipeline handlers only, which it is > > > used in the Request class at Request::addBuffer() and Request::reuse() > > > time. > > > > > > Rework the documentation to report that. > > > > > > Fixes: 125be3436ae0 ("libcamera: FrameBuffer: Add a setRequest() interface") > > > > In all fairness this was true when 125be3436ae0 was merged it only > > changed in dcc024760a8d that was merged a year later ;-) > > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > > > > --- > > > src/libcamera/buffer.cpp | 5 +++-- > > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > > > diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp > > > index 75b2693281a7..5baccfa99f10 100644 > > > --- a/src/libcamera/buffer.cpp > > > +++ b/src/libcamera/buffer.cpp > > > @@ -191,8 +191,9 @@ FrameBuffer::FrameBuffer(const std::vector<Plane> &planes, unsigned int cookie) > > > * \brief Set the request this buffer belongs to > > > * \param[in] request Request to set > > > * > > > - * The intended callers of this method are pipeline handlers and only for > > > - * buffers that are internal to the pipeline. > > > + * For buffers added to requests by applications, this method is called by > > > + * Request::addBuffer() or Request::reuse(). For buffers internal to pipeline > > > + * handlers, it is called by the pipeline handlers themselves. > > > * > > > * \todo Shall be hidden from applications with a d-pointer design. > > > */ > > Is this todo still reasonable? It needs to be public so that a > pipeline handler calls it? I think "public" meant "visible from application" Probablya the todo has to be read as: - We need to implement d-pointer (or PIMPL, or whatever, I'm not happy we deviated from C++ to adhere to a naming convention introduced by another framework such as Qt, but...) not to expose methods that are intentended to be used by library internal components (such as PH or the Request class) to applications. > > > > -- > > > 2.31.1 > > > > > Reviewed-by: Hirokazu Honda <hiroh@chromium.org> > > > > _______________________________________________ > > > libcamera-devel mailing list > > > libcamera-devel@lists.libcamera.org > > > https://lists.libcamera.org/listinfo/libcamera-devel > > > > -- > > Regards, > > Niklas Söderlund > > _______________________________________________ > > libcamera-devel mailing list > > libcamera-devel@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel
On Thu, Apr 22, 2021 at 4:10 PM Jacopo Mondi <jacopo@jmondi.org> wrote: > > Hi Hiro, > > On Thu, Apr 22, 2021 at 02:07:12PM +0900, Hirokazu Honda wrote: > > Hi Jacopo, thank you for the patch. > > > > On Thu, Apr 22, 2021 at 3:47 AM Niklas Söderlund > > <niklas.soderlund@ragnatech.se> wrote: > > > > > > Hi Jacopo, > > > > > > On 2021-04-21 18:03:14 +0200, Jacopo Mondi wrote: > > > > I got fooled by the documentation of setRequest() implying that the > > > > function is meant to be called by pipeline handlers only, which it is > > > > used in the Request class at Request::addBuffer() and Request::reuse() > > > > time. > > > > > > > > Rework the documentation to report that. > > > > > > > > Fixes: 125be3436ae0 ("libcamera: FrameBuffer: Add a setRequest() interface") > > > > > > In all fairness this was true when 125be3436ae0 was merged it only > > > changed in dcc024760a8d that was merged a year later ;-) > > > > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > > > > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > > > > > > --- > > > > src/libcamera/buffer.cpp | 5 +++-- > > > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp > > > > index 75b2693281a7..5baccfa99f10 100644 > > > > --- a/src/libcamera/buffer.cpp > > > > +++ b/src/libcamera/buffer.cpp > > > > @@ -191,8 +191,9 @@ FrameBuffer::FrameBuffer(const std::vector<Plane> &planes, unsigned int cookie) > > > > * \brief Set the request this buffer belongs to > > > > * \param[in] request Request to set > > > > * > > > > - * The intended callers of this method are pipeline handlers and only for > > > > - * buffers that are internal to the pipeline. > > > > + * For buffers added to requests by applications, this method is called by > > > > + * Request::addBuffer() or Request::reuse(). For buffers internal to pipeline > > > > + * handlers, it is called by the pipeline handlers themselves. > > > > * > > > > * \todo Shall be hidden from applications with a d-pointer design. > > > > */ > > > > Is this todo still reasonable? It needs to be public so that a > > pipeline handler calls it? > > I think "public" meant "visible from application" > > Probablya the todo has to be read as: > > - We need to implement d-pointer (or PIMPL, or whatever, I'm not happy > we deviated from C++ to adhere to a naming convention introduced by another > framework such as Qt, but...) not to expose methods that are intentended > to be used by library internal components (such as PH or the Request > class) to applications. > PipelineHandler and Request cannot call a private function of FrameBuffer and a function of FrameBuffer::Private, can they? D-pointer doesn't help for hiding the function as PH, Request and Application are the same level from FrameBuffer. I this the possible way is to move setRequest to private and make PH and Request friends. But I think it is not a right direction.. I am honestly have no idea how to hide from applications keeping them accessible from PH and Request. I would drop "with d-pointer design". I may miss something. Appreciate if you correct me. Thanks, -Hiro > > > > > > -- > > > > 2.31.1 > > > > > > > > Reviewed-by: Hirokazu Honda <hiroh@chromium.org> > > > > > > _______________________________________________ > > > > libcamera-devel mailing list > > > > libcamera-devel@lists.libcamera.org > > > > https://lists.libcamera.org/listinfo/libcamera-devel > > > > > > -- > > > Regards, > > > Niklas Söderlund > > > _______________________________________________ > > > libcamera-devel mailing list > > > libcamera-devel@lists.libcamera.org > > > https://lists.libcamera.org/listinfo/libcamera-devel
Hello Hiro and Jacopo, On Thu, Apr 22, 2021 at 04:26:47PM +0900, Hirokazu Honda wrote: > On Thu, Apr 22, 2021 at 4:10 PM Jacopo Mondi wrote: > > On Thu, Apr 22, 2021 at 02:07:12PM +0900, Hirokazu Honda wrote: > > > On Thu, Apr 22, 2021 at 3:47 AM Niklas Söderlund wrote: > > > > On 2021-04-21 18:03:14 +0200, Jacopo Mondi wrote: > > > > > I got fooled by the documentation of setRequest() implying that the > > > > > function is meant to be called by pipeline handlers only, which it is > > > > > used in the Request class at Request::addBuffer() and Request::reuse() > > > > > time. > > > > > > > > > > Rework the documentation to report that. > > > > > > > > > > Fixes: 125be3436ae0 ("libcamera: FrameBuffer: Add a setRequest() interface") > > > > > > > > In all fairness this was true when 125be3436ae0 was merged it only > > > > changed in dcc024760a8d that was merged a year later ;-) > > > > > > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > > > > > > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > > > > > > > > --- > > > > > src/libcamera/buffer.cpp | 5 +++-- > > > > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > > > > > > > diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp > > > > > index 75b2693281a7..5baccfa99f10 100644 > > > > > --- a/src/libcamera/buffer.cpp > > > > > +++ b/src/libcamera/buffer.cpp > > > > > @@ -191,8 +191,9 @@ FrameBuffer::FrameBuffer(const std::vector<Plane> &planes, unsigned int cookie) > > > > > * \brief Set the request this buffer belongs to > > > > > * \param[in] request Request to set > > > > > * > > > > > - * The intended callers of this method are pipeline handlers and only for > > > > > - * buffers that are internal to the pipeline. > > > > > + * For buffers added to requests by applications, this method is called by > > > > > + * Request::addBuffer() or Request::reuse(). For buffers internal to pipeline > > > > > + * handlers, it is called by the pipeline handlers themselves. > > > > > * > > > > > * \todo Shall be hidden from applications with a d-pointer design. > > > > > */ > > > > > > Is this todo still reasonable? It needs to be public so that a > > > pipeline handler calls it? > > > > I think "public" meant "visible from application" > > > > Probablya the todo has to be read as: > > > > - We need to implement d-pointer (or PIMPL, or whatever, I'm not happy > > we deviated from C++ to adhere to a naming convention introduced by another > > framework such as Qt, but...) Note that this design pattern is known by different names (https://en.wikipedia.org/wiki/Opaque_pointer), and the name pimpl isn't part of the C++ standard as far as I know. I'm fine using a different name than d-pointer though, if one of the other ones is more common (I may veto Cheshire cat though :-)). > > not to expose methods that are intentended > > to be used by library internal components (such as PH or the Request > > class) to applications. > > PipelineHandler and Request cannot call a private function of > FrameBuffer and a function of FrameBuffer::Private, can they? > D-pointer doesn't help for hiding the function as PH, Request and > Application are the same level from FrameBuffer. > I this the possible way is to move setRequest to private and make PH > and Request friends. But I think it is not a right direction.. > I am honestly have no idea how to hide from applications keeping them > accessible from PH and Request. > I would drop "with d-pointer design". > I may miss something. Appreciate if you correct me. With this design pattern, the FrameBuffer::Private class can be defined in an *internal* header file. The class can then define public functions exposed to other classes. We would need to make the Extensible::_d() functions public (they're currently protected), which I don't think would be a problem. Applications could call _d() and get a pointer to the Private object, but given that the Private class would be defined in an internal header, they wouldn't be able to access it (and even if they did, they couldn't claim in good faith they didn't know it wasn't a public API :-)). > > > Reviewed-by: Hirokazu Honda <hiroh@chromium.org>
diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp index 75b2693281a7..5baccfa99f10 100644 --- a/src/libcamera/buffer.cpp +++ b/src/libcamera/buffer.cpp @@ -191,8 +191,9 @@ FrameBuffer::FrameBuffer(const std::vector<Plane> &planes, unsigned int cookie) * \brief Set the request this buffer belongs to * \param[in] request Request to set * - * The intended callers of this method are pipeline handlers and only for - * buffers that are internal to the pipeline. + * For buffers added to requests by applications, this method is called by + * Request::addBuffer() or Request::reuse(). For buffers internal to pipeline + * handlers, it is called by the pipeline handlers themselves. * * \todo Shall be hidden from applications with a d-pointer design. */
I got fooled by the documentation of setRequest() implying that the function is meant to be called by pipeline handlers only, which it is used in the Request class at Request::addBuffer() and Request::reuse() time. Rework the documentation to report that. Fixes: 125be3436ae0 ("libcamera: FrameBuffer: Add a setRequest() interface") Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- src/libcamera/buffer.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)