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

[Hirokazu Honda]

On Thu, Apr 22, 2021 at 4:10 PM Jacopo Mondi <jacopo at 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 at 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 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...) 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 at chromium.org>
> >
> > > > _______________________________________________
> > > > libcamera-devel mailing list
> > > > libcamera-devel at lists.libcamera.org
> > > > https://lists.libcamera.org/listinfo/libcamera-devel
> > >
> > > --
> > > Regards,
> > > Niklas Söderlund
> > > _______________________________________________
> > > libcamera-devel mailing list
> > > libcamera-devel at lists.libcamera.org
> > > https://lists.libcamera.org/listinfo/libcamera-devel