[libcamera-devel,1/3] Documentation: Update code examples to match Request related changes

16223 diff mbox series

- Camera::createRequest() now returns unique_ptr instead of raw pointer
- Request::BufferMap key type is now const
---
 Documentation/guides/application-developer.rst | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

Hi Daniel,

On Wed, Jun 15, 2022 at 06:25:59PM +0200, Daniel Semkowicz via libcamera-devel wrote:
> - Camera::createRequest() now returns unique_ptr instead of raw pointer
> - Request::BufferMap key type is now const

As a general comment: all your patches are missing your Signed-off-by.
Just 'git commit -s' to add it automatically.

On this patch, we have received a very similar one
https://patchwork.libcamera.org/patch/16207/

> ---
>  Documentation/guides/application-developer.rst | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/guides/application-developer.rst b/Documentation/guides/application-developer.rst
> index 16bea9c4..6ab8b7e5 100644
> --- a/Documentation/guides/application-developer.rst
> +++ b/Documentation/guides/application-developer.rst
> @@ -308,7 +308,7 @@ the camera.
>
>     Stream *stream = streamConfig.stream();
>     const std::vector<std::unique_ptr<FrameBuffer>> &buffers = allocator->buffers(stream);
> -   std::vector<Request *> requests;
> +   std::vector<std::unique_ptr<Request>> requests;
>
>  Proceed to fill the request vector by creating ``Request`` instances from the
>  camera device, and associate a buffer for each of them for the ``Stream``.
> @@ -316,7 +316,7 @@ camera device, and associate a buffer for each of them for the ``Stream``.
>  .. code:: cpp
>
>         for (unsigned int i = 0; i < buffers.size(); ++i) {
> -           Request *request = camera->createRequest();
> +           std::unique_ptr<Request> request = camera->createRequest();
>             if (!request)
>             {
>                 std::cerr << "Can't create request" << std::endl;
> @@ -332,7 +332,7 @@ camera device, and associate a buffer for each of them for the ``Stream``.
>                 return ret;
>             }
>
> -           requests.push_back(request);
> +           requests.push_back(std::move(request));
>         }
>
>  .. TODO: Controls
> @@ -410,7 +410,7 @@ images.
>
>  .. code:: cpp
>
> -   const std::map<Stream *, FrameBuffer *> &buffers = request->buffers();
> +   const std::map<const Stream *, FrameBuffer *> &buffers = request->buffers();
>

Which does not include this hunk

I think we can merge Tommaso's one as it has been sent before and you can
rebase on that one ?

Thanks
   j

>  Iterating through the map allows applications to inspect each completed buffer
>  in this request, and access the metadata associated to each frame.
> @@ -517,8 +517,8 @@ and queue all the previously created requests.
>  .. code:: cpp
>
>     camera->start();
> -   for (Request *request : requests)
> -       camera->queueRequest(request);
> +   for (std::unique_ptr<Request> &request : requests)
> +       camera->queueRequest(request.get());
>
>  Start an event loop
>  ~~~~~~~~~~~~~~~~~~~
> --
> 2.34.1
>

Hi Jacopo,

On Thu, Jun 16, 2022 at 12:46 PM Jacopo Mondi <jacopo@jmondi.org> wrote:
>
> Hi Daniel,
>
> On Wed, Jun 15, 2022 at 06:25:59PM +0200, Daniel Semkowicz via libcamera-devel wrote:
> > - Camera::createRequest() now returns unique_ptr instead of raw pointer
> > - Request::BufferMap key type is now const
>
> As a general comment: all your patches are missing your Signed-off-by.
> Just 'git commit -s' to add it automatically.
>

Oh, sorry I missed that. I will add it in the updated version.

> On this patch, we have received a very similar one
> https://patchwork.libcamera.org/patch/16207/
>
> > ---
> >  Documentation/guides/application-developer.rst | 12 ++++++------
> >  1 file changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/Documentation/guides/application-developer.rst b/Documentation/guides/application-developer.rst
> > index 16bea9c4..6ab8b7e5 100644
> > --- a/Documentation/guides/application-developer.rst
> > +++ b/Documentation/guides/application-developer.rst
> > @@ -308,7 +308,7 @@ the camera.
> >
> >     Stream *stream = streamConfig.stream();
> >     const std::vector<std::unique_ptr<FrameBuffer>> &buffers = allocator->buffers(stream);
> > -   std::vector<Request *> requests;
> > +   std::vector<std::unique_ptr<Request>> requests;
> >
> >  Proceed to fill the request vector by creating ``Request`` instances from the
> >  camera device, and associate a buffer for each of them for the ``Stream``.
> > @@ -316,7 +316,7 @@ camera device, and associate a buffer for each of them for the ``Stream``.
> >  .. code:: cpp
> >
> >         for (unsigned int i = 0; i < buffers.size(); ++i) {
> > -           Request *request = camera->createRequest();
> > +           std::unique_ptr<Request> request = camera->createRequest();
> >             if (!request)
> >             {
> >                 std::cerr << "Can't create request" << std::endl;
> > @@ -332,7 +332,7 @@ camera device, and associate a buffer for each of them for the ``Stream``.
> >                 return ret;
> >             }
> >
> > -           requests.push_back(request);
> > +           requests.push_back(std::move(request));
> >         }
> >
> >  .. TODO: Controls
> > @@ -410,7 +410,7 @@ images.
> >
> >  .. code:: cpp
> >
> > -   const std::map<Stream *, FrameBuffer *> &buffers = request->buffers();
> > +   const std::map<const Stream *, FrameBuffer *> &buffers = request->buffers();
> >
>
> Which does not include this hunk
>
> I think we can merge Tommaso's one as it has been sent before and you can
> rebase on that one ?
>

Yes, sure. We can do it this way.

Best regards
Daniel Semkowicz

> Thanks
>    j
>
> >  Iterating through the map allows applications to inspect each completed buffer
> >  in this request, and access the metadata associated to each frame.
> > @@ -517,8 +517,8 @@ and queue all the previously created requests.
> >  .. code:: cpp
> >
> >     camera->start();
> > -   for (Request *request : requests)
> > -       camera->queueRequest(request);
> > +   for (std::unique_ptr<Request> &request : requests)
> > +       camera->queueRequest(request.get());
> >
> >  Start an event loop
> >  ~~~~~~~~~~~~~~~~~~~
> > --
> > 2.34.1
> >

Hello Daniel,

On Fri, Jun 17, 2022 at 09:02:23AM +0200, Daniel Semkowicz via libcamera-devel wrote:
> On Thu, Jun 16, 2022 at 12:46 PM Jacopo Mondi <jacopo@jmondi.org> wrote:
> > On Wed, Jun 15, 2022 at 06:25:59PM +0200, Daniel Semkowicz via libcamera-devel wrote:
> > > - Camera::createRequest() now returns unique_ptr instead of raw pointer
> > > - Request::BufferMap key type is now const
> >
> > As a general comment: all your patches are missing your Signed-off-by.
> > Just 'git commit -s' to add it automatically.
> 
> Oh, sorry I missed that. I will add it in the updated version.

For context, please see
https://libcamera.org/contributing.html#submitting-patches.
https://libcamera.org/coding-style.html#coding-style-guidelines is also
worth a read if you haven't yet, especially the part about the
checkstyle.py script (although that's mostly for code, not
documentation).

> > On this patch, we have received a very similar one
> > https://patchwork.libcamera.org/patch/16207/
> >
> > > ---
> > >  Documentation/guides/application-developer.rst | 12 ++++++------
> > >  1 file changed, 6 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/Documentation/guides/application-developer.rst b/Documentation/guides/application-developer.rst
> > > index 16bea9c4..6ab8b7e5 100644
> > > --- a/Documentation/guides/application-developer.rst
> > > +++ b/Documentation/guides/application-developer.rst
> > > @@ -308,7 +308,7 @@ the camera.
> > >
> > >     Stream *stream = streamConfig.stream();
> > >     const std::vector<std::unique_ptr<FrameBuffer>> &buffers = allocator->buffers(stream);
> > > -   std::vector<Request *> requests;
> > > +   std::vector<std::unique_ptr<Request>> requests;
> > >
> > >  Proceed to fill the request vector by creating ``Request`` instances from the
> > >  camera device, and associate a buffer for each of them for the ``Stream``.
> > > @@ -316,7 +316,7 @@ camera device, and associate a buffer for each of them for the ``Stream``.
> > >  .. code:: cpp
> > >
> > >         for (unsigned int i = 0; i < buffers.size(); ++i) {
> > > -           Request *request = camera->createRequest();
> > > +           std::unique_ptr<Request> request = camera->createRequest();
> > >             if (!request)
> > >             {
> > >                 std::cerr << "Can't create request" << std::endl;
> > > @@ -332,7 +332,7 @@ camera device, and associate a buffer for each of them for the ``Stream``.
> > >                 return ret;
> > >             }
> > >
> > > -           requests.push_back(request);
> > > +           requests.push_back(std::move(request));
> > >         }
> > >
> > >  .. TODO: Controls
> > > @@ -410,7 +410,7 @@ images.
> > >
> > >  .. code:: cpp
> > >
> > > -   const std::map<Stream *, FrameBuffer *> &buffers = request->buffers();
> > > +   const std::map<const Stream *, FrameBuffer *> &buffers = request->buffers();
> > >
> >
> > Which does not include this hunk
> >
> > I think we can merge Tommaso's one as it has been sent before and you can
> > rebase on that one ?
> 
> Yes, sure. We can do it this way.

I've merged Tommaso's patch in the libcamera git tree, so you can now
rebase easily.

> > >  Iterating through the map allows applications to inspect each completed buffer
> > >  in this request, and access the metadata associated to each frame.
> > > @@ -517,8 +517,8 @@ and queue all the previously created requests.
> > >  .. code:: cpp
> > >
> > >     camera->start();
> > > -   for (Request *request : requests)
> > > -       camera->queueRequest(request);
> > > +   for (std::unique_ptr<Request> &request : requests)
> > > +       camera->queueRequest(request.get());
> > >
> > >  Start an event loop
> > >  ~~~~~~~~~~~~~~~~~~~