[libcamera-devel,v2,2/3] android: camera_device: Queue request using Worker
diff mbox series

Message ID 20201010095830.134084-3-jacopo@jmondi.org
State Accepted
Headers show
Series
  • android: Introduce CameraWorker
Related show

Commit Message

Jacopo Mondi Oct. 10, 2020, 9:58 a.m. UTC
Add a CameraWorker class member to the CameraDevice class and
queue capture requests to it to delegate its handling. Start and
stop the CameraWorker when the libcamera::Camera is started or
stopped.

Tie the CaptureRequest lifetime to the Camera3RequestDescriptor's one
by storing it as unique_ptr<> in the descriptor to simplify handling
of request creation and deletion.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 src/android/camera_device.cpp | 42 ++++++++++++++++++-----------------
 src/android/camera_device.h   |  7 +++++-
 2 files changed, 28 insertions(+), 21 deletions(-)

Comments

Laurent Pinchart Oct. 11, 2020, 11:30 p.m. UTC | #1
Hi Jacopo,

Thank you for the patch.

On Sat, Oct 10, 2020 at 11:58:29AM +0200, Jacopo Mondi wrote:
> Add a CameraWorker class member to the CameraDevice class and
> queue capture requests to it to delegate its handling. Start and
> stop the CameraWorker when the libcamera::Camera is started or
> stopped.
> 
> Tie the CaptureRequest lifetime to the Camera3RequestDescriptor's one
> by storing it as unique_ptr<> in the descriptor to simplify handling
> of request creation and deletion.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  src/android/camera_device.cpp | 42 ++++++++++++++++++-----------------
>  src/android/camera_device.h   |  7 +++++-
>  2 files changed, 28 insertions(+), 21 deletions(-)
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index c29fcb43fe2d..0036f1140560 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -168,11 +168,24 @@ MappedCamera3Buffer::MappedCamera3Buffer(const buffer_handle_t camera3buffer,
>   */
>  
>  CameraDevice::Camera3RequestDescriptor::Camera3RequestDescriptor(

A candidate for later, renaming CameraDevice::Camera3RequestDescriptor
to something shorter :-) Maybe CameraDevice::RequestDescriptor, or even
CameraDevice::Request ?

> -		unsigned int frameNumber, unsigned int numBuffers)
> +	Camera *camera, unsigned int frameNumber, unsigned int numBuffers)
>  	: frameNumber(frameNumber), numBuffers(numBuffers)
>  {
>  	buffers = new camera3_stream_buffer_t[numBuffers];
> +
> +	/*
> +	 * FrameBuffer instances created by wrapping a camera3 provided dmabuf
> +	 * are emplaced in this vector of unique_ptr<> for lifetime management.
> +	 */
>  	frameBuffers.reserve(numBuffers);
> +
> +	/*
> +	 * Create the libcamera::Request unique_ptr<> to tie its lifetime
> +	 * to the descriptor's one. Set the descriptor's address as the
> +	 * request's cookie to retrieve it at completion time.
> +	 */
> +	request = std::make_unique<CaptureRequest>(camera,
> +						   reinterpret_cast<uint64_t>(this));
>  }
>  
>  CameraDevice::Camera3RequestDescriptor::~Camera3RequestDescriptor()
> @@ -519,6 +532,7 @@ void CameraDevice::close()
>  {
>  	streams_.clear();
>  
> +	worker_.stop();
>  	camera_->stop();
>  	camera_->release();
>  
> @@ -1350,6 +1364,8 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>  
>  	/* Start the camera if that's the first request we handle. */
>  	if (!running_) {
> +		worker_.start();
> +
>  		int ret = camera_->start();
>  		if (ret) {
>  			LOG(HAL, Error) << "Failed to start camera";
> @@ -1372,16 +1388,9 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>  	 * at request complete time.
>  	 */
>  	Camera3RequestDescriptor *descriptor =
> -		new Camera3RequestDescriptor(camera3Request->frame_number,
> +		new Camera3RequestDescriptor(camera_.get(), camera3Request->frame_number,
>  					     camera3Request->num_output_buffers);
>  
> -	std::unique_ptr<Request> request =
> -		camera_->createRequest(reinterpret_cast<uint64_t>(descriptor));
> -	if (!request) {
> -		LOG(HAL, Error) << "Failed to create request";
> -		return -ENOMEM;
> -	}
> -
>  	LOG(HAL, Debug) << "Queueing Request to libcamera with "
>  			<< descriptor->numBuffers << " HAL streams";
>  	for (unsigned int i = 0; i < descriptor->numBuffers; ++i) {
> @@ -1448,18 +1457,12 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>  			return -ENOMEM;
>  		}
>  
> -		request->addBuffer(cameraStream->stream(), buffer);
> -	}
> -
> -	int ret = camera_->queueRequest(request.get());
> -	if (ret) {
> -		LOG(HAL, Error) << "Failed to queue request";
> -		delete descriptor;
> -		return ret;
> +		descriptor->request->addBuffer(cameraStream->stream(), buffer,
> +					       camera3Buffers[i].acquire_fence);
>  	}
>  
> -	/* The request will be deleted in the completion handler. */
> -	request.release();
> +	/* Queue the request on the CameraWorker. */

s/on the/to the/

> +	worker_.queueRequest(descriptor->request.get());
>  
>  	return 0;
>  }
> @@ -1564,7 +1567,6 @@ void CameraDevice::requestComplete(Request *request)
>  	callbacks_->process_capture_result(callbacks_, &captureResult);
>  
>  	delete descriptor;
> -	delete request;
>  }
>  
>  std::string CameraDevice::logPrefix() const
> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> index 777d1a35e801..86f2b8974b53 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;
> @@ -73,7 +74,8 @@ private:
>  	CameraDevice(unsigned int id, const std::shared_ptr<libcamera::Camera> &camera);
>  
>  	struct Camera3RequestDescriptor {
> -		Camera3RequestDescriptor(unsigned int frameNumber,
> +		Camera3RequestDescriptor(libcamera::Camera *camera,
> +					 unsigned int frameNumber,
>  					 unsigned int numBuffers);
>  		~Camera3RequestDescriptor();
>  
> @@ -81,6 +83,7 @@ private:
>  		uint32_t numBuffers;
>  		camera3_stream_buffer_t *buffers;
>  		std::vector<std::unique_ptr<libcamera::FrameBuffer>> frameBuffers;
> +		std::unique_ptr<CaptureRequest> request;

You could actually embed this. It may even make sense to merge the two
structures together as I don't think we need two separate structures. Up
to you, it can also be handled later.

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

I like the simplification :-)

>  	};
>  
>  	struct Camera3StreamConfiguration {
> @@ -108,6 +111,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_;
Niklas Söderlund Oct. 14, 2020, 12:02 p.m. UTC | #2
Hi Jacopo,

Nice work.

On 2020-10-10 11:58:29 +0200, Jacopo Mondi wrote:
> Add a CameraWorker class member to the CameraDevice class and
> queue capture requests to it to delegate its handling. Start and
> stop the CameraWorker when the libcamera::Camera is started or
> stopped.
> 
> Tie the CaptureRequest lifetime to the Camera3RequestDescriptor's one
> by storing it as unique_ptr<> in the descriptor to simplify handling
> of request creation and deletion.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>

Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>

> ---
>  src/android/camera_device.cpp | 42 ++++++++++++++++++-----------------
>  src/android/camera_device.h   |  7 +++++-
>  2 files changed, 28 insertions(+), 21 deletions(-)
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index c29fcb43fe2d..0036f1140560 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -168,11 +168,24 @@ MappedCamera3Buffer::MappedCamera3Buffer(const buffer_handle_t camera3buffer,
>   */
>  
>  CameraDevice::Camera3RequestDescriptor::Camera3RequestDescriptor(
> -		unsigned int frameNumber, unsigned int numBuffers)
> +	Camera *camera, unsigned int frameNumber, unsigned int numBuffers)
>  	: frameNumber(frameNumber), numBuffers(numBuffers)
>  {
>  	buffers = new camera3_stream_buffer_t[numBuffers];
> +
> +	/*
> +	 * FrameBuffer instances created by wrapping a camera3 provided dmabuf
> +	 * are emplaced in this vector of unique_ptr<> for lifetime management.
> +	 */
>  	frameBuffers.reserve(numBuffers);
> +
> +	/*
> +	 * Create the libcamera::Request unique_ptr<> to tie its lifetime
> +	 * to the descriptor's one. Set the descriptor's address as the
> +	 * request's cookie to retrieve it at completion time.
> +	 */
> +	request = std::make_unique<CaptureRequest>(camera,
> +						   reinterpret_cast<uint64_t>(this));
>  }
>  
>  CameraDevice::Camera3RequestDescriptor::~Camera3RequestDescriptor()
> @@ -519,6 +532,7 @@ void CameraDevice::close()
>  {
>  	streams_.clear();
>  
> +	worker_.stop();
>  	camera_->stop();
>  	camera_->release();
>  
> @@ -1350,6 +1364,8 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>  
>  	/* Start the camera if that's the first request we handle. */
>  	if (!running_) {
> +		worker_.start();
> +
>  		int ret = camera_->start();
>  		if (ret) {
>  			LOG(HAL, Error) << "Failed to start camera";
> @@ -1372,16 +1388,9 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>  	 * at request complete time.
>  	 */
>  	Camera3RequestDescriptor *descriptor =
> -		new Camera3RequestDescriptor(camera3Request->frame_number,
> +		new Camera3RequestDescriptor(camera_.get(), camera3Request->frame_number,
>  					     camera3Request->num_output_buffers);
>  
> -	std::unique_ptr<Request> request =
> -		camera_->createRequest(reinterpret_cast<uint64_t>(descriptor));
> -	if (!request) {
> -		LOG(HAL, Error) << "Failed to create request";
> -		return -ENOMEM;
> -	}
> -
>  	LOG(HAL, Debug) << "Queueing Request to libcamera with "
>  			<< descriptor->numBuffers << " HAL streams";
>  	for (unsigned int i = 0; i < descriptor->numBuffers; ++i) {
> @@ -1448,18 +1457,12 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>  			return -ENOMEM;
>  		}
>  
> -		request->addBuffer(cameraStream->stream(), buffer);
> -	}
> -
> -	int ret = camera_->queueRequest(request.get());
> -	if (ret) {
> -		LOG(HAL, Error) << "Failed to queue request";
> -		delete descriptor;
> -		return ret;
> +		descriptor->request->addBuffer(cameraStream->stream(), buffer,
> +					       camera3Buffers[i].acquire_fence);
>  	}
>  
> -	/* The request will be deleted in the completion handler. */
> -	request.release();
> +	/* Queue the request on the CameraWorker. */
> +	worker_.queueRequest(descriptor->request.get());
>  
>  	return 0;
>  }
> @@ -1564,7 +1567,6 @@ void CameraDevice::requestComplete(Request *request)
>  	callbacks_->process_capture_result(callbacks_, &captureResult);
>  
>  	delete descriptor;
> -	delete request;
>  }
>  
>  std::string CameraDevice::logPrefix() const
> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> index 777d1a35e801..86f2b8974b53 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;
> @@ -73,7 +74,8 @@ private:
>  	CameraDevice(unsigned int id, const std::shared_ptr<libcamera::Camera> &camera);
>  
>  	struct Camera3RequestDescriptor {
> -		Camera3RequestDescriptor(unsigned int frameNumber,
> +		Camera3RequestDescriptor(libcamera::Camera *camera,
> +					 unsigned int frameNumber,
>  					 unsigned int numBuffers);
>  		~Camera3RequestDescriptor();
>  
> @@ -81,6 +83,7 @@ private:
>  		uint32_t numBuffers;
>  		camera3_stream_buffer_t *buffers;
>  		std::vector<std::unique_ptr<libcamera::FrameBuffer>> frameBuffers;
> +		std::unique_ptr<CaptureRequest> request;
>  	};
>  
>  	struct Camera3StreamConfiguration {
> @@ -108,6 +111,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_;
> -- 
> 2.28.0
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Patch
diff mbox series

diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
index c29fcb43fe2d..0036f1140560 100644
--- a/src/android/camera_device.cpp
+++ b/src/android/camera_device.cpp
@@ -168,11 +168,24 @@  MappedCamera3Buffer::MappedCamera3Buffer(const buffer_handle_t camera3buffer,
  */
 
 CameraDevice::Camera3RequestDescriptor::Camera3RequestDescriptor(
-		unsigned int frameNumber, unsigned int numBuffers)
+	Camera *camera, unsigned int frameNumber, unsigned int numBuffers)
 	: frameNumber(frameNumber), numBuffers(numBuffers)
 {
 	buffers = new camera3_stream_buffer_t[numBuffers];
+
+	/*
+	 * FrameBuffer instances created by wrapping a camera3 provided dmabuf
+	 * are emplaced in this vector of unique_ptr<> for lifetime management.
+	 */
 	frameBuffers.reserve(numBuffers);
+
+	/*
+	 * Create the libcamera::Request unique_ptr<> to tie its lifetime
+	 * to the descriptor's one. Set the descriptor's address as the
+	 * request's cookie to retrieve it at completion time.
+	 */
+	request = std::make_unique<CaptureRequest>(camera,
+						   reinterpret_cast<uint64_t>(this));
 }
 
 CameraDevice::Camera3RequestDescriptor::~Camera3RequestDescriptor()
@@ -519,6 +532,7 @@  void CameraDevice::close()
 {
 	streams_.clear();
 
+	worker_.stop();
 	camera_->stop();
 	camera_->release();
 
@@ -1350,6 +1364,8 @@  int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
 
 	/* Start the camera if that's the first request we handle. */
 	if (!running_) {
+		worker_.start();
+
 		int ret = camera_->start();
 		if (ret) {
 			LOG(HAL, Error) << "Failed to start camera";
@@ -1372,16 +1388,9 @@  int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
 	 * at request complete time.
 	 */
 	Camera3RequestDescriptor *descriptor =
-		new Camera3RequestDescriptor(camera3Request->frame_number,
+		new Camera3RequestDescriptor(camera_.get(), camera3Request->frame_number,
 					     camera3Request->num_output_buffers);
 
-	std::unique_ptr<Request> request =
-		camera_->createRequest(reinterpret_cast<uint64_t>(descriptor));
-	if (!request) {
-		LOG(HAL, Error) << "Failed to create request";
-		return -ENOMEM;
-	}
-
 	LOG(HAL, Debug) << "Queueing Request to libcamera with "
 			<< descriptor->numBuffers << " HAL streams";
 	for (unsigned int i = 0; i < descriptor->numBuffers; ++i) {
@@ -1448,18 +1457,12 @@  int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
 			return -ENOMEM;
 		}
 
-		request->addBuffer(cameraStream->stream(), buffer);
-	}
-
-	int ret = camera_->queueRequest(request.get());
-	if (ret) {
-		LOG(HAL, Error) << "Failed to queue request";
-		delete descriptor;
-		return ret;
+		descriptor->request->addBuffer(cameraStream->stream(), buffer,
+					       camera3Buffers[i].acquire_fence);
 	}
 
-	/* The request will be deleted in the completion handler. */
-	request.release();
+	/* Queue the request on the CameraWorker. */
+	worker_.queueRequest(descriptor->request.get());
 
 	return 0;
 }
@@ -1564,7 +1567,6 @@  void CameraDevice::requestComplete(Request *request)
 	callbacks_->process_capture_result(callbacks_, &captureResult);
 
 	delete descriptor;
-	delete request;
 }
 
 std::string CameraDevice::logPrefix() const
diff --git a/src/android/camera_device.h b/src/android/camera_device.h
index 777d1a35e801..86f2b8974b53 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;
@@ -73,7 +74,8 @@  private:
 	CameraDevice(unsigned int id, const std::shared_ptr<libcamera::Camera> &camera);
 
 	struct Camera3RequestDescriptor {
-		Camera3RequestDescriptor(unsigned int frameNumber,
+		Camera3RequestDescriptor(libcamera::Camera *camera,
+					 unsigned int frameNumber,
 					 unsigned int numBuffers);
 		~Camera3RequestDescriptor();
 
@@ -81,6 +83,7 @@  private:
 		uint32_t numBuffers;
 		camera3_stream_buffer_t *buffers;
 		std::vector<std::unique_ptr<libcamera::FrameBuffer>> frameBuffers;
+		std::unique_ptr<CaptureRequest> request;
 	};
 
 	struct Camera3StreamConfiguration {
@@ -108,6 +111,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_;