[libcamera-devel,v2,7/9] android: camera_device: Move buffer mapping for post processing
diff mbox series

Message ID 20210910070638.467294-8-umang.jain@ideasonboard.com
State Superseded
Delegated to: Umang Jain
Headers show
Series
  • Async Post Processor
Related show

Commit Message

Umang Jain Sept. 10, 2021, 7:06 a.m. UTC 
Buffer mapping for post processing currently happens in
CameraStream::process(). However, it can be easily be moved to
CameraDevice just before the call to CameraStream::process(). The
reason to do so is that, we can hold the mapped destination buffer
pointer as a part of Camera3RequestDescriptor. Camera3RequestDescriptor
already hold other post-processing related context to enable async post
processing in subsequent commits.

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---
 src/android/camera_device.cpp | 14 +++++++++++++-
 src/android/camera_device.h   |  1 +
 src/android/camera_stream.cpp | 16 ++--------------
 src/android/camera_stream.h   |  2 +-
 4 files changed, 17 insertions(+), 16 deletions(-)

Comments

Laurent Pinchart Sept. 13, 2021, 1:01 a.m. UTC | #1 
Hi Umang,

Thank you for the patch.

On Fri, Sep 10, 2021 at 12:36:36PM +0530, Umang Jain wrote:
> Buffer mapping for post processing currently happens in
> CameraStream::process(). However, it can be easily be moved to
> CameraDevice just before the call to CameraStream::process(). The
> reason to do so is that, we can hold the mapped destination buffer
> pointer as a part of Camera3RequestDescriptor. Camera3RequestDescriptor
> already hold other post-processing related context to enable async post
> processing in subsequent commits.

Why is it better to map the buffer in CameraDevice::requestComplete() ?
As mapping can be a costly operation, it seems better to me to move it
to the post-processor thread instead.

> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> ---
>  src/android/camera_device.cpp | 14 +++++++++++++-
>  src/android/camera_device.h   |  1 +
>  src/android/camera_stream.cpp | 16 ++--------------
>  src/android/camera_stream.h   |  2 +-
>  4 files changed, 17 insertions(+), 16 deletions(-)
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index 7f04d225..988d4232 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -1163,13 +1163,25 @@ void CameraDevice::requestComplete(Request *request)
>  		descriptor.resultMetadata_ = std::move(resultMetadata);
>  		descriptor.captureResult_ = captureResult;
>  
> +		/* \todo Buffer mapping should be moved to a separate thread? */
> +		const StreamConfiguration &output = cameraStream->configuration();
> +		descriptor.destBuffer_ = std::make_unique<CameraBuffer>(
> +			*buffer.buffer, formats::MJPEG, output.size,
> +			PROT_READ | PROT_WRITE);
> +
> +		if (!descriptor.destBuffer_->isValid()) {
> +			LOG(HAL, Error) << "Failed to map android blob buffer";
> +			return;
> +		}
> +
>  		std::unique_ptr<Camera3RequestDescriptor> reqDescriptor =
>  			std::make_unique<Camera3RequestDescriptor>();
>  		*reqDescriptor = std::move(descriptor);
>  		queuedDescriptor_.push_back(std::move(reqDescriptor));
>  
>  		Camera3RequestDescriptor *currentDescriptor = queuedDescriptor_.back().get();
> -		int ret = cameraStream->process(src, *buffer.buffer,
> +		int ret = cameraStream->process(src,
> +						currentDescriptor->destBuffer_.get(),
>  						currentDescriptor->settings_,
>  						currentDescriptor->resultMetadata_.get(),
>  						currentDescriptor);
> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> index e7318358..b62d373c 100644
> --- a/src/android/camera_device.h
> +++ b/src/android/camera_device.h
> @@ -58,6 +58,7 @@ struct Camera3RequestDescriptor {
>  		Failed,
>  	};
>  	std::unique_ptr<CameraMetadata> resultMetadata_;
> +	std::unique_ptr<CameraBuffer> destBuffer_;
>  	camera3_capture_result_t captureResult_;
>  	libcamera::FrameBuffer *internalBuffer_;
>  	completionStatus status_;
> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
> index c7d874b2..5fd04bbf 100644
> --- a/src/android/camera_stream.cpp
> +++ b/src/android/camera_stream.cpp
> @@ -102,7 +102,7 @@ int CameraStream::configure()
>  }
>  
>  int CameraStream::process(const FrameBuffer *source,
> -			  buffer_handle_t camera3Dest,
> +			  CameraBuffer *destBuffer,
>  			  const CameraMetadata &requestMetadata,
>  			  CameraMetadata *resultMetadata,
>  			  const Camera3RequestDescriptor *context)
> @@ -110,19 +110,7 @@ int CameraStream::process(const FrameBuffer *source,
>  	if (!postProcessor_)
>  		return 0;
>  
> -	/*
> -	 * \todo Buffer mapping and processing should be moved to a
> -	 * separate thread.
> -	 */
> -	const StreamConfiguration &output = configuration();
> -	CameraBuffer dest(camera3Dest, formats::MJPEG, output.size,
> -			  PROT_READ | PROT_WRITE);
> -	if (!dest.isValid()) {
> -		LOG(HAL, Error) << "Failed to map android blob buffer";
> -		return -EINVAL;
> -	}
> -
> -	return postProcessor_->process(source, &dest, requestMetadata,
> +	return postProcessor_->process(source, destBuffer, requestMetadata,
>  				       resultMetadata, context);
>  }
>  
> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
> index be076995..8097ddbc 100644
> --- a/src/android/camera_stream.h
> +++ b/src/android/camera_stream.h
> @@ -124,7 +124,7 @@ public:
>  
>  	int configure();
>  	int process(const libcamera::FrameBuffer *source,
> -		    buffer_handle_t camera3Dest,
> +		    CameraBuffer *destBuffer,
>  		    const CameraMetadata &requestMetadata,
>  		    CameraMetadata *resultMetadata,
>  		    const Camera3RequestDescriptor *context);
Umang Jain Sept. 13, 2021, 7:05 a.m. UTC | #2 
On 9/13/21 6:31 AM, Laurent Pinchart wrote:
> Hi Umang,
>
> Thank you for the patch.
>
> On Fri, Sep 10, 2021 at 12:36:36PM +0530, Umang Jain wrote:
>> Buffer mapping for post processing currently happens in
>> CameraStream::process(). However, it can be easily be moved to
>> CameraDevice just before the call to CameraStream::process(). The
>> reason to do so is that, we can hold the mapped destination buffer
>> pointer as a part of Camera3RequestDescriptor. Camera3RequestDescriptor
>> already hold other post-processing related context to enable async post
>> processing in subsequent commits.
> Why is it better to map the buffer in CameraDevice::requestComplete() ?
> As mapping can be a costly operation, it seems better to me to move it
> to the post-processor thread instead.


We can save a pointer in the context(aka Camera3RequestDescriptor) to 
destBuffer if I move it to CameraDevice.

>
>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
>> ---
>>   src/android/camera_device.cpp | 14 +++++++++++++-
>>   src/android/camera_device.h   |  1 +
>>   src/android/camera_stream.cpp | 16 ++--------------
>>   src/android/camera_stream.h   |  2 +-
>>   4 files changed, 17 insertions(+), 16 deletions(-)
>>
>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
>> index 7f04d225..988d4232 100644
>> --- a/src/android/camera_device.cpp
>> +++ b/src/android/camera_device.cpp
>> @@ -1163,13 +1163,25 @@ void CameraDevice::requestComplete(Request *request)
>>   		descriptor.resultMetadata_ = std::move(resultMetadata);
>>   		descriptor.captureResult_ = captureResult;
>>   
>> +		/* \todo Buffer mapping should be moved to a separate thread? */
>> +		const StreamConfiguration &output = cameraStream->configuration();
>> +		descriptor.destBuffer_ = std::make_unique<CameraBuffer>(
>> +			*buffer.buffer, formats::MJPEG, output.size,
>> +			PROT_READ | PROT_WRITE);
>> +
>> +		if (!descriptor.destBuffer_->isValid()) {
>> +			LOG(HAL, Error) << "Failed to map android blob buffer";
>> +			return;
>> +		}
>> +
>>   		std::unique_ptr<Camera3RequestDescriptor> reqDescriptor =
>>   			std::make_unique<Camera3RequestDescriptor>();
>>   		*reqDescriptor = std::move(descriptor);
>>   		queuedDescriptor_.push_back(std::move(reqDescriptor));
>>   
>>   		Camera3RequestDescriptor *currentDescriptor = queuedDescriptor_.back().get();
>> -		int ret = cameraStream->process(src, *buffer.buffer,
>> +		int ret = cameraStream->process(src,
>> +						currentDescriptor->destBuffer_.get(),
>>   						currentDescriptor->settings_,
>>   						currentDescriptor->resultMetadata_.get(),
>>   						currentDescriptor);
>> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
>> index e7318358..b62d373c 100644
>> --- a/src/android/camera_device.h
>> +++ b/src/android/camera_device.h
>> @@ -58,6 +58,7 @@ struct Camera3RequestDescriptor {
>>   		Failed,
>>   	};
>>   	std::unique_ptr<CameraMetadata> resultMetadata_;
>> +	std::unique_ptr<CameraBuffer> destBuffer_;
>>   	camera3_capture_result_t captureResult_;
>>   	libcamera::FrameBuffer *internalBuffer_;
>>   	completionStatus status_;
>> diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
>> index c7d874b2..5fd04bbf 100644
>> --- a/src/android/camera_stream.cpp
>> +++ b/src/android/camera_stream.cpp
>> @@ -102,7 +102,7 @@ int CameraStream::configure()
>>   }
>>   
>>   int CameraStream::process(const FrameBuffer *source,
>> -			  buffer_handle_t camera3Dest,
>> +			  CameraBuffer *destBuffer,
>>   			  const CameraMetadata &requestMetadata,
>>   			  CameraMetadata *resultMetadata,
>>   			  const Camera3RequestDescriptor *context)
>> @@ -110,19 +110,7 @@ int CameraStream::process(const FrameBuffer *source,
>>   	if (!postProcessor_)
>>   		return 0;
>>   
>> -	/*
>> -	 * \todo Buffer mapping and processing should be moved to a
>> -	 * separate thread.
>> -	 */
>> -	const StreamConfiguration &output = configuration();
>> -	CameraBuffer dest(camera3Dest, formats::MJPEG, output.size,
>> -			  PROT_READ | PROT_WRITE);
>> -	if (!dest.isValid()) {
>> -		LOG(HAL, Error) << "Failed to map android blob buffer";
>> -		return -EINVAL;
>> -	}
>> -
>> -	return postProcessor_->process(source, &dest, requestMetadata,
>> +	return postProcessor_->process(source, destBuffer, requestMetadata,
>>   				       resultMetadata, context);
>>   }
>>   
>> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
>> index be076995..8097ddbc 100644
>> --- a/src/android/camera_stream.h
>> +++ b/src/android/camera_stream.h
>> @@ -124,7 +124,7 @@ public:
>>   
>>   	int configure();
>>   	int process(const libcamera::FrameBuffer *source,
>> -		    buffer_handle_t camera3Dest,
>> +		    CameraBuffer *destBuffer,
>>   		    const CameraMetadata &requestMetadata,
>>   		    CameraMetadata *resultMetadata,
>>   		    const Camera3RequestDescriptor *context);

Patch
diff mbox series 

diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
index 7f04d225..988d4232 100644
--- a/src/android/camera_device.cpp
+++ b/src/android/camera_device.cpp
@@ -1163,13 +1163,25 @@  void CameraDevice::requestComplete(Request *request)
 		descriptor.resultMetadata_ = std::move(resultMetadata);
 		descriptor.captureResult_ = captureResult;
 
+		/* \todo Buffer mapping should be moved to a separate thread? */
+		const StreamConfiguration &output = cameraStream->configuration();
+		descriptor.destBuffer_ = std::make_unique<CameraBuffer>(
+			*buffer.buffer, formats::MJPEG, output.size,
+			PROT_READ | PROT_WRITE);
+
+		if (!descriptor.destBuffer_->isValid()) {
+			LOG(HAL, Error) << "Failed to map android blob buffer";
+			return;
+		}
+
 		std::unique_ptr<Camera3RequestDescriptor> reqDescriptor =
 			std::make_unique<Camera3RequestDescriptor>();
 		*reqDescriptor = std::move(descriptor);
 		queuedDescriptor_.push_back(std::move(reqDescriptor));
 
 		Camera3RequestDescriptor *currentDescriptor = queuedDescriptor_.back().get();
-		int ret = cameraStream->process(src, *buffer.buffer,
+		int ret = cameraStream->process(src,
+						currentDescriptor->destBuffer_.get(),
 						currentDescriptor->settings_,
 						currentDescriptor->resultMetadata_.get(),
 						currentDescriptor);
diff --git a/src/android/camera_device.h b/src/android/camera_device.h
index e7318358..b62d373c 100644
--- a/src/android/camera_device.h
+++ b/src/android/camera_device.h
@@ -58,6 +58,7 @@  struct Camera3RequestDescriptor {
 		Failed,
 	};
 	std::unique_ptr<CameraMetadata> resultMetadata_;
+	std::unique_ptr<CameraBuffer> destBuffer_;
 	camera3_capture_result_t captureResult_;
 	libcamera::FrameBuffer *internalBuffer_;
 	completionStatus status_;
diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
index c7d874b2..5fd04bbf 100644
--- a/src/android/camera_stream.cpp
+++ b/src/android/camera_stream.cpp
@@ -102,7 +102,7 @@  int CameraStream::configure()
 }
 
 int CameraStream::process(const FrameBuffer *source,
-			  buffer_handle_t camera3Dest,
+			  CameraBuffer *destBuffer,
 			  const CameraMetadata &requestMetadata,
 			  CameraMetadata *resultMetadata,
 			  const Camera3RequestDescriptor *context)
@@ -110,19 +110,7 @@  int CameraStream::process(const FrameBuffer *source,
 	if (!postProcessor_)
 		return 0;
 
-	/*
-	 * \todo Buffer mapping and processing should be moved to a
-	 * separate thread.
-	 */
-	const StreamConfiguration &output = configuration();
-	CameraBuffer dest(camera3Dest, formats::MJPEG, output.size,
-			  PROT_READ | PROT_WRITE);
-	if (!dest.isValid()) {
-		LOG(HAL, Error) << "Failed to map android blob buffer";
-		return -EINVAL;
-	}
-
-	return postProcessor_->process(source, &dest, requestMetadata,
+	return postProcessor_->process(source, destBuffer, requestMetadata,
 				       resultMetadata, context);
 }
 
diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
index be076995..8097ddbc 100644
--- a/src/android/camera_stream.h
+++ b/src/android/camera_stream.h
@@ -124,7 +124,7 @@  public:
 
 	int configure();
 	int process(const libcamera::FrameBuffer *source,
-		    buffer_handle_t camera3Dest,
+		    CameraBuffer *destBuffer,
 		    const CameraMetadata &requestMetadata,
 		    CameraMetadata *resultMetadata,
 		    const Camera3RequestDescriptor *context);