[libcamera-devel] [PATCH v2 4/9] android: camera_device: Simplify FrameBuffer construction from a buffer_handle_t

[Niklas Söderlund]

Hi Kieran,

Thanks for your work.

On 2020-07-02 22:36:49 +0100, Kieran Bingham wrote:
> Move the code which constructs a FrameBuffer from the Android buffer handle
> to it's own function to simplify the code flow and readability.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> ---
>  src/android/camera_device.cpp | 35 +++++++++++++++++++----------------
>  1 file changed, 19 insertions(+), 16 deletions(-)
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index 03dcdd520794..78ecfd7c2ee2 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -1011,6 +1011,24 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>  	return 0;
>  }
>  
> +static FrameBuffer *newFrameBuffer(const buffer_handle_t camera3buffer)
> +{
> +	std::vector<FrameBuffer::Plane> planes;
> +	for (unsigned int i = 0; i < 3; i++) {
> +		FrameBuffer::Plane plane;
> +		plane.fd = FileDescriptor(camera3buffer->data[i]);
> +		/*
> +		 * Setting length to zero here is OK as the length is only used
> +		 * to map the memory of the plane. Libcamera do not need to poke
> +		 * at the memory content queued by the HAL.
> +		 */
> +		plane.length = 0;
> +		planes.push_back(std::move(plane));
> +	}
> +
> +	return new FrameBuffer(std::move(planes));
> +}
> +
>  int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Request)
>  {
>  	StreamConfiguration *streamConfiguration = &config_->at(0);
> @@ -1064,22 +1082,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>  	 * Create a libcamera buffer using the dmabuf descriptors of the first
>  	 * and (currently) only supported request buffer.
>  	 */
> -	const buffer_handle_t camera3Handle = *camera3Buffers[0].buffer;
> -
> -	std::vector<FrameBuffer::Plane> planes;
> -	for (int i = 0; i < 3; i++) {
> -		FrameBuffer::Plane plane;
> -		plane.fd = FileDescriptor(camera3Handle->data[i]);
> -		/*
> -		 * Setting length to zero here is OK as the length is only used
> -		 * to map the memory of the plane. Libcamera do not need to poke
> -		 * at the memory content queued by the HAL.
> -		 */
> -		plane.length = 0;
> -		planes.push_back(std::move(plane));
> -	}
> -
> -	FrameBuffer *buffer = new FrameBuffer(std::move(planes));
> +	FrameBuffer *buffer = newFrameBuffer(*camera3Buffers[0].buffer);

Would it make sens to change the argument to a pointer instead of 
dereferencing it here? Also I think I would callt the function 
constructFrameBuffer() or createFrameBuffer().

>  	if (!buffer) {
>  		LOG(HAL, Error) << "Failed to create buffer";
>  		delete descriptor;
> -- 
> 2.25.1
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

-- 
Regards,
Niklas Söderlund