[libcamera-devel] [PATCH v3 11/16] libcamera: buffer: Re-work setRequest() documentation

[Laurent Pinchart]

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 at ideasonboard.com>
> > > > > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > > >
> > > > Reviewed-by: Niklas Söderlund <niklas.soderlund at 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 at chromium.org>

-- 
Regards,

Laurent Pinchart