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

[Jacopo Mondi]

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.

>
> > > --
> > > 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