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

[Jacopo Mondi]

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.
>
> > 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 at ideasonboard.com>

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

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