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

Message ID 20200702213654.2129054-5-kieran.bingham@ideasonboard.com
State Superseded
Headers show
Series
  • android: Multi-stream support
Related show

Commit Message

Kieran Bingham July 2, 2020, 9:36 p.m. UTC
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@ideasonboard.com>
---
 src/android/camera_device.cpp | 35 +++++++++++++++++++----------------
 1 file changed, 19 insertions(+), 16 deletions(-)

Comments

Niklas Söderlund July 2, 2020, 11:25 p.m. UTC | #1
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@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@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Laurent Pinchart July 3, 2020, 12:29 a.m. UTC | #2
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@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.

> Also I think I would callt the function 
> constructFrameBuffer() or createFrameBuffer().

I like createFrameBuffer() better, and I would make it a private member
of CameraDevice. With these changes,

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

> >  	if (!buffer) {
> >  		LOG(HAL, Error) << "Failed to create buffer";
> >  		delete descriptor;
Jacopo Mondi July 3, 2020, 9:13 a.m. UTC | #3
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@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.
>
> > Also I think I would callt the function
> > constructFrameBuffer() or createFrameBuffer().
>
> I like createFrameBuffer() better, and I would make it a private member
> of CameraDevice. With these changes,

Me too, and I like this better as a private helper function

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

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

Thanks
  j
>
> > >  	if (!buffer) {
> > >  		LOG(HAL, Error) << "Failed to create buffer";
> > >  		delete descriptor;
>
> --
> Regards,
>
> Laurent Pinchart
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Kieran Bingham July 3, 2020, 9:38 a.m. UTC | #4
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@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@ideasonboard.com>
> 
> Reviewed-by: Jacopo Mondi <jacopo@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@lists.libcamera.org
>> https://lists.libcamera.org/listinfo/libcamera-devel

Patch

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);
 	if (!buffer) {
 		LOG(HAL, Error) << "Failed to create buffer";
 		delete descriptor;