[libcamera-devel] [SimpleCam PATCH 3/4] simple-cam: Fix the Buffer Allocation guidance

[Jacopo Mondi]

Hi Kieran,

On Wed, Aug 25, 2021 at 12:48:40PM +0100, Kieran Bingham wrote:
>
>
> On 25/08/2021 10:21, Jacopo Mondi wrote:
> > Hi Kieran,
> >
> > On Tue, Aug 24, 2021 at 03:24:49PM +0100, Kieran Bingham wrote:
> >> The Buffer Allocation notes contains a TODO. Improve the notes regarding
> >> the FrameBufferAllocation and remove the todo.
> >>
> >> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> >> ---
> >>  simple-cam.cpp | 12 +++++++++---
> >>  1 file changed, 9 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/simple-cam.cpp b/simple-cam.cpp
> >> index 2e646a5485c9..47c5dc9987bc 100644
> >> --- a/simple-cam.cpp
> >> +++ b/simple-cam.cpp
> >> @@ -274,10 +274,16 @@ int main()
> >>  	 * Buffer Allocation
> >>  	 *
> >>  	 * Now that a camera has been configured, it knows all about its
> >> -	 * Streams sizes and formats, so we now have to ask it to reserve
> >> -	 * memory for all of them.
> >> +	 * Streams sizes and formats. We need to provide buffers to store
> >> +	 * captured images in. An application might choose to provide buffers
> >> +	 * externally, for instance from a display driver which will render the
> >> +	 * captured images, however libcamera can also provide buffers from a
> >> +	 * configured camera.
> >
> > What about one paragraph for each of the two use cases ?
> >
> >
> > 	 * Streams sizes and formats. The captured images need to be stored in
> >          * memory buffers which can be either provided by the application to the
> >          * library, or allocated in the Camera memory and exposed to
> >          * the application by libcamera.
> >          *
> >          * An application can decide to allocate memory buffers from
> >          * elsewhere, in example in the memory of the display driver that
> >          * will render the captured frames, and provide them to
> >          * libcamera to capture images there.
> >          *
> >          * Alternatively libcamera can help the application by providing
> >          * buffers allocated in the Camera memory by the usage of a
> >          * FrameBufferAllocator instance, which references the
> >          * (configured) Camera for which the application wishes to
> >          * allocated buffers for.
> >          */
> >
> > feel free to take whatever or ignore.
>
>
>
> Trying to expand with your text, and fixing up a few parts I get this:
>

Thanks for considering the suggestion.
A few more comments

> > /*
> >  * Now that a camera has been configured, it knows all about its
> >  * Streams sizes and formats. The captured images need to be stored in
> >  * memory buffers which can either be provided by the application to the
> >  * library, or allocated in the Camera and exposed to the application by
> >  * libcamera.
> >  *
> >  * An application may decide to allocate memory buffers from elsewhere,

s/memory buffers/buffers
to avoid the 'memory' repetition with the below 'memory allocated'

> >  * for example in memory allocated by the display driver that will
> >  * render the captured frames. The application will provide them to
> >  * libcamera by constructing FrameBuffer instances to capture images
> >  * directly into.
> >  *
> >  * Alternatively libcamera can help the application by providing buffers

s/providing/exporting ?

> >  * allocated by the Camera using a FrameBufferAllocator instance and

s/by/in/ ?

> >  * referencing a configured Camera to determine the appropriate buffer
> >  * size and types to create.
> >  */
>
> Hows that? It feels a little bit repetitive, but I don't mind that if it
> expands more.

That's good! I think even if a bit pedantic, it's good to give more
details.

>
>
> > Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>

Feel free to push your current version or adjust it!

Thanks
   j

> >
> > Thanks
> >    j
> >
> >
> >> +	 *
> >> +	 * To request buffers from libcamera, we use the FrameBufferAllocator,
> >> +	 * and reference the (configured) camera for which we wish to allocate
> >> +	 * buffers for.
> >>  	 */
> >> -	/* TODO: Update the comment here too */
> >>  	FrameBufferAllocator *allocator = new FrameBufferAllocator(camera);
> >>
> >>  	for (StreamConfiguration &cfg : *config) {
> >> --
> >> 2.30.2
> >>