[libcamera-devel] [PATCH 03/11] android: camera_device: Build capture_result dynamically

[Umang Jain]

Hi Jacopo

On 10/18/21 9:18 PM, Jacopo Mondi wrote:
> Hi Umang,
>
> On Mon, Oct 18, 2021 at 06:59:15PM +0530, Umang Jain wrote:
>> From: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>>
>> The camera3_capture_result_t is only needed to convey capture results to
>> the camera service through the process_capture_result() callback.
>> There's no need to store it in the Camera3RequestDescriptor. Build it
>> dynamically in CameraDevice::sendCaptureResults() instead.
>>
>> This requires storing the result metadata created in
>> CameraDevice::requestComplete() in the Camera3RequestDescriptor. A side
>> effect of this change is that the request metadata lifetime will match
>> the Camera3RequestDescriptor instead of being destroyed at the end of
>> requestComplete(). This will be needed to support asynchronous
>> post-processing, where the request completion will be signaled to the
>> camera service asynchronously from requestComplete().
>>
>> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
>> ---
>>   src/android/camera_device.cpp | 43 ++++++++++++++++-------------------
>>   src/android/camera_request.h  |  2 +-
>>   2 files changed, 21 insertions(+), 24 deletions(-)
>>
>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
>> index b4ab5da1..c6ae8930 100644
>> --- a/src/android/camera_device.cpp
>> +++ b/src/android/camera_device.cpp
>> @@ -830,19 +830,11 @@ void CameraDevice::abortRequest(Camera3RequestDescriptor *descriptor) const
>>   {
>>   	notifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_REQUEST);
>>
>> -	camera3_capture_result_t &result = descriptor->captureResult_;
>> -	result.num_output_buffers = descriptor->buffers_.size();
>> -	result.frame_number = descriptor->frameNumber_;
>> -	result.partial_result = 0;
>> -
>> -	std::vector<camera3_stream_buffer_t> resultBuffers(result.num_output_buffers);
>> -	for (auto [i, buffer] : utils::enumerate(resultBuffers)) {
>> -		buffer = descriptor->buffers_[i];
>> -		buffer.release_fence = descriptor->buffers_[i].acquire_fence;
>> +	for (auto &buffer : descriptor->buffers_) {
>> +		buffer.release_fence = buffer.acquire_fence;
>>   		buffer.acquire_fence = -1;
>>   		buffer.status = CAMERA3_BUFFER_STATUS_ERROR;
>>   	}
>> -	result.output_buffers = resultBuffers.data();
>>
>>   	descriptor->status_ = Camera3RequestDescriptor::Status::Error;
>>   }
>> @@ -1090,9 +1082,6 @@ void CameraDevice::requestComplete(Request *request)
>>   	 * The buffer status is set to OK and later changed to ERROR if
>>   	 * post-processing/compression fails.
>>   	 */
>> -	camera3_capture_result_t &captureResult = descriptor->captureResult_;
>> -	captureResult.frame_number = descriptor->frameNumber_;
>> -	captureResult.num_output_buffers = descriptor->buffers_.size();
>>   	for (camera3_stream_buffer_t &buffer : descriptor->buffers_) {
>>   		CameraStream *cameraStream =
>>   			static_cast<CameraStream *>(buffer.stream->priv);
>> @@ -1113,8 +1102,6 @@ void CameraDevice::requestComplete(Request *request)
>>   		buffer.release_fence = -1;
>>   		buffer.status = CAMERA3_BUFFER_STATUS_OK;
>>   	}
>> -	captureResult.output_buffers = descriptor->buffers_.data();
>> -	captureResult.partial_result = 1;
>>
>>   	/*
>>   	 * If the Request has failed, abort the request by notifying the error
>> @@ -1128,7 +1115,6 @@ void CameraDevice::requestComplete(Request *request)
>>   		notifyError(descriptor->frameNumber_, nullptr,
>>   			    CAMERA3_MSG_ERROR_REQUEST);
>>
>> -		captureResult.partial_result = 0;
>>   		for (camera3_stream_buffer_t &buffer : descriptor->buffers_) {
>>   			/*
>>   			 * Signal to the framework it has to handle fences that
>> @@ -1165,12 +1151,12 @@ void CameraDevice::requestComplete(Request *request)
>>   	 * Notify if the metadata generation has failed, but continue processing
>>   	 * buffers and return an empty metadata pack.
>>   	 */
>> -	std::unique_ptr<CameraMetadata> resultMetadata = getResultMetadata(*descriptor);
>> -	if (!resultMetadata) {
>> +	descriptor->resultMetadata_ = getResultMetadata(*descriptor);
>> +	if (!descriptor->resultMetadata_) {
>>   		notifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_RESULT);
>>
>>   		/* The camera framework expects an empty metadata pack on error. */
>> -		resultMetadata = std::make_unique<CameraMetadata>(0, 0);
>> +		descriptor->resultMetadata_ = std::make_unique<CameraMetadata>(0, 0);
> Since we don't sendCaptureResult() and return here we get to
> post-processing with a (0, 0) metadata pack. I wonder if the
> post-processing code is robust enough to cope with that situation
> (even if I suspect I already know the answer :)
>
> It is my understanding the situation is like this already, so it's
> fine, but maybe we should add a todo.


\todo stating what? The situation or the solution. I am not sure of a 
potential solution of this yet

>
>>   	}
>>
>>   	/* Handle post-processing. */
>> @@ -1192,7 +1178,7 @@ void CameraDevice::requestComplete(Request *request)
>>
>>   		int ret = cameraStream->process(*src, buffer,
>>   						descriptor->settings_,
>> -						resultMetadata.get());
>> +						descriptor->resultMetadata_.get());
>>   		/*
>>   		 * Return the FrameBuffer to the CameraStream now that we're
>>   		 * done processing it.
>> @@ -1207,7 +1193,6 @@ void CameraDevice::requestComplete(Request *request)
>>   		}
>>   	}
>>
>> -	captureResult.result = resultMetadata->get();
>>   	descriptor->status_ = Camera3RequestDescriptor::Status::Success;
>>   	sendCaptureResults();
>>   }
>> @@ -1225,8 +1210,20 @@ void CameraDevice::sendCaptureResults()
>>   		 * impact on performance which should be measured.
>>   		 */
>>   		lock.unlock();
>> -		callbacks_->process_capture_result(callbacks_,
>> -						   &descriptor->captureResult_);
>> +
>> +		camera3_capture_result_t captureResult = {};
>> +
>> +		captureResult.frame_number = descriptor->frameNumber_;
>> +		if (descriptor->resultMetadata_)
> Do we need this ? As resultMetadata_ is a unique_ptr<> it is
> constructed owning nothing, and calling get() on it simnply return
> nullptr, which I think it's what we want.


Yes we need this. There are two get() here

a) one of the unique_ptr one, .get()

b) another is the descriptor->resultMetadata_->get()) one

If descriptor->resultMetadata_ is nullptr (initialized), ->get() on it,  
will segfault

So I found best to guard it with an if () block for now. Does it make sense?

>
> Thanks
>     j
>> +			captureResult.result = descriptor->resultMetadata_->get();
>> +		captureResult.num_output_buffers = descriptor->buffers_.size();
>> +		captureResult.output_buffers = descriptor->buffers_.data();
>> +
>> +		if (descriptor->status_ == Camera3RequestDescriptor::Status::Success)
>> +			captureResult.partial_result = 1;
>> +
>> +		callbacks_->process_capture_result(callbacks_, &captureResult);
>> +
>>   		lock.lock();
>>   	}
>>   }
>> diff --git a/src/android/camera_request.h b/src/android/camera_request.h
>> index 79dfdb58..db13f624 100644
>> --- a/src/android/camera_request.h
>> +++ b/src/android/camera_request.h
>> @@ -40,8 +40,8 @@ public:
>>   	std::vector<std::unique_ptr<libcamera::FrameBuffer>> frameBuffers_;
>>   	CameraMetadata settings_;
>>   	std::unique_ptr<CaptureRequest> request_;
>> +	std::unique_ptr<CameraMetadata> resultMetadata_;
>>
>> -	camera3_capture_result_t captureResult_ = {};
>>   	Status status_ = Status::Pending;
>>
>>   private:
>> --
>> 2.31.0
>>