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

[Laurent Pinchart]

Hello,

On Wed, Aug 25, 2021 at 02:44:16PM +0200, Jacopo Mondi wrote:
> On Wed, Aug 25, 2021 at 12:48:40PM +0100, Kieran Bingham wrote:
> > On 25/08/2021 10:21, Jacopo Mondi wrote:
> > > 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'

I'd say framebuffers, as that's the term we use in libcamera. Same above
and below.

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

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> 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!
> 
> > >> +	 *
> > >> +	 * 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) {

-- 
Regards,

Laurent Pinchart