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

[Kieran Bingham]

Hi All,

On 03/07/2020 10:13, Jacopo Mondi wrote:
> Hi Kieran,
> 
> On Fri, Jul 03, 2020 at 03:29:03AM +0300, Laurent Pinchart wrote:
>> Hi Kieran,
>>
>> Thank you for the patch.
>>
>> On Fri, Jul 03, 2020 at 01:25:59AM +0200, Niklas Söderlund wrote:
>>> 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.
>>
>> s/it's/its/
>>
>>>>
>>>> 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?
>>
>> buffer_handle_t is already a pointer, the function would then need to do
>>
>> 	plane.fd = FileDescriptor((*camera3buffer)->data[i]);
>>
>> which isn't nice. I think it's best to dereference it once only here, in
>> the case function needs to access more than one field.

Yup, the ** was a pain, so I chose to dereference it here.


>>
>>> Also I think I would callt the function
>>> constructFrameBuffer() or createFrameBuffer().
>>
>> I like createFrameBuffer() better, and I would make it a private member


I'll rename to createFrameBuffer()


>> of CameraDevice. With these changes,
> 
> Me too, and I like this better as a private helper function

Agreed, me too - this was just a remnant of hacking it in to get the
reworks done.



>> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> 
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>

Thanks.


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

-- 
Regards
--
Kieran