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

[Jacopo Mondi]

Hi Laurent,

On Thu, Oct 08, 2020 at 07:24:38AM +0300, Laurent Pinchart wrote:
> 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.
>

depending if Paul's reusable request gets in first, this might become
less awful.


> > >  			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