[libcamera-devel] [RFC 2/2] android: camera_device: Queue request to Worker

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Tue, Oct 06, 2020 at 06:12:17PM +0200, Jacopo Mondi wrote:
> On Tue, Oct 06, 2020 at 06:06:37PM +0200, Jacopo Mondi wrote:
> > Add a CameraWorker class member to the CameraDevice class and
> > queue capture requests to it to delegate fence handling and capture
> > requests queueing to the camera.
> >
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> >  src/android/camera_device.cpp | 21 ++++++++-------------
> >  src/android/camera_device.h   |  3 +++
> >  2 files changed, 11 insertions(+), 13 deletions(-)
> >
> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > index 8da70e817b46..edac9f28ab67 100644
> > --- a/src/android/camera_device.cpp
> > +++ b/src/android/camera_device.cpp
> > @@ -194,8 +194,8 @@ CameraDevice::Camera3RequestDescriptor::~Camera3RequestDescriptor()
> >   */
> >
> >  CameraDevice::CameraDevice(unsigned int id, const std::shared_ptr<Camera> &camera)
> > -	: id_(id), running_(false), camera_(camera), staticMetadata_(nullptr),
> > -	  facing_(CAMERA_FACING_FRONT), orientation_(0)
> > +	: id_(id), worker_(camera), running_(false), camera_(camera),
> > +	  staticMetadata_(nullptr), facing_(CAMERA_FACING_FRONT), orientation_(0)
> >  {
> >  	camera_->requestCompleted.connect(this, &CameraDevice::requestComplete);
> >
> > @@ -1375,8 +1375,9 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> >  		new Camera3RequestDescriptor(camera3Request->frame_number,
> >  					     camera3Request->num_output_buffers);
> >
> > -	Request *request =
> > -		camera_->createRequest(reinterpret_cast<uint64_t>(descriptor));
> > +	std::unique_ptr<CaptureRequest> captureRequest =
> > +		std::make_unique<CaptureRequest>(
> > +			camera_->createRequest(reinterpret_cast<uint64_t>(descriptor)));
> >
> >  	LOG(HAL, Debug) << "Queueing Request to libcamera with "
> >  			<< descriptor->numBuffers << " HAL streams";
> > @@ -1440,21 +1441,15 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> >
> >  		if (!buffer) {
> >  			LOG(HAL, Error) << "Failed to create buffer";
> > -			delete request;
> 
> You're leaking request :p

Apart from that, and the need to start the worker on open and stop it on
close (with proper synchronization to make sure nothing gets lost on the
message queue), it looks good to me.

> >  			delete descriptor;
> >  			return -ENOMEM;
> >  		}
> >
> > -		request->addBuffer(cameraStream->stream(), buffer);
> > +		captureRequest->addBuffer(cameraStream->stream(), buffer,
> > +					  camera3Buffers[i].acquire_fence);
> >  	}
> >
> > -	int ret = camera_->queueRequest(request);
> > -	if (ret) {
> > -		LOG(HAL, Error) << "Failed to queue request";
> > -		delete request;
> > -		delete descriptor;
> > -		return ret;
> > -	}
> > +	worker_.queueRequest(std::move(captureRequest));
> >
> >  	return 0;
> >  }
> > diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> > index 777d1a35e801..b4b32f77a29a 100644
> > --- a/src/android/camera_device.h
> > +++ b/src/android/camera_device.h
> > @@ -25,6 +25,7 @@
> >  #include "libcamera/internal/message.h"
> >
> >  #include "camera_stream.h"
> > +#include "camera_worker.h"
> >  #include "jpeg/encoder.h"
> >
> >  class CameraMetadata;
> > @@ -108,6 +109,8 @@ private:
> >  	unsigned int id_;
> >  	camera3_device_t camera3Device_;
> >
> > +	CameraWorker worker_;
> > +
> >  	bool running_;
> >  	std::shared_ptr<libcamera::Camera> camera_;
> >  	std::unique_ptr<libcamera::CameraConfiguration> config_;

-- 
Regards,

Laurent Pinchart