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

[Kieran Bingham]

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:

> /*
>  * 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,
>  * 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
>  * allocated by the Camera using a FrameBufferAllocator instance and
>  * 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: Jacopo Mondi <jacopo at jmondi.org>
> 
> 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
>>