[libcamera-devel] [PATCH v4 4/4] android: camera_device: Send capture results by inspecting the queue

[Umang Jain]

Hi Jacopo

On 9/30/21 12:20 AM, Jacopo Mondi wrote:
> Hi Umang,
>
> On Wed, Sep 29, 2021 at 07:00:30PM +0530, Umang Jain wrote:
>> There is a possibility that an out-of-order completion of capture
>> request happens by calling process_capture_result() directly on error
>> paths. The framework expects that errors should be notified as soon as
>> possible, but the request completion order should remain intact.
>> An existing instance of this is abortRequest(), which sends the capture
>> results on flushing state, without considering order-of-completion.
>>
>> Since we have a queue of Camera3RequestDescriptor tracking each
>> capture request placed by framework to libcamera HAL, we should be only
>> sending back capture results from a single location, by inspecting
>> the queue. As per the patch, this now happens in
>> CameraDevice::sendCaptureResults().
>>
>> Each descriptor is now equipped with its own status to denote whether
>> the capture request is complete and ready to send back to the framework
> ready to be sent
>
>> or needs to be waited upon. This ensures that the order of completion is
>> respected for the requests.
>>
>> Since we are fixing out-of-order request completion in abortRequest(),
>> change the function to read from the Camera3RequestDescriptor directly,
>> instead of camera3_capture_request_t. The descriptor should have all the
>> information necessary to set the request buffers' state to error.
>>
>> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
>> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>> Reviewed-by: Hirokazu Honda <hiroh at chromium.org>
>> ---
>>   src/android/camera_device.cpp | 59 ++++++++++++++++++++++++-----------
>>   src/android/camera_device.h   | 14 ++++++++-
>>   2 files changed, 54 insertions(+), 19 deletions(-)
>>
>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
>> index 45350563..38c64bb7 100644
>> --- a/src/android/camera_device.cpp
>> +++ b/src/android/camera_device.cpp
>> @@ -860,25 +860,25 @@ int CameraDevice::processControls(Camera3RequestDescriptor *descriptor)
>>   	return 0;
>>   }
>>
>> -void CameraDevice::abortRequest(camera3_capture_request_t *request) const
>> +void CameraDevice::abortRequest(Camera3RequestDescriptor *descriptor) const
>>   {
>> -	notifyError(request->frame_number, nullptr, CAMERA3_MSG_ERROR_REQUEST);
>> +	notifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_REQUEST);
>>
>> -	camera3_capture_result_t result = {};
>> -	result.num_output_buffers = request->num_output_buffers;
>> -	result.frame_number = request->frame_number;
>> +	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 = request->output_buffers[i];
>> -		buffer.release_fence = request->output_buffers[i].acquire_fence;
>> +		buffer = descriptor->buffers_[i];
>> +		buffer.release_fence = descriptor->buffers_[i].acquire_fence;
>>   		buffer.acquire_fence = -1;
>>   		buffer.status = CAMERA3_BUFFER_STATUS_ERROR;
>>   	}
>>   	result.output_buffers = resultBuffers.data();
>>
>> -	callbacks_->process_capture_result(callbacks_, &result);
>> +	descriptor->status_ = Camera3RequestDescriptor::Status::Error;
>>   }
>>
>>   bool CameraDevice::isValidRequest(camera3_capture_request_t *camera3Request) const
>> @@ -1050,13 +1050,21 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>>   		return ret;
>>
>>   	/*
>> -	 * If flush is in progress abort the request. If the camera has been
>> -	 * stopped we have to re-start it to be able to process the request.
>> +	 * If flush is in progress set the request status to error and place it
>> +	 * on the queue to be later completed. If the camera has been stopped we
>> +	 * have to re-start it to be able to process the request.
>>   	 */
>>   	MutexLocker stateLock(stateMutex_);
>>
>>   	if (state_ == State::Flushing) {
>> -		abortRequest(camera3Request);
>> +		abortRequest(descriptor.get());
>> +		{
>> +			MutexLocker descriptorsLock(descriptorsMutex_);
>> +			descriptors_.push(std::move(descriptor));
>> +		}
>> +
>> +		sendCaptureResults();
>> +
>>   		return 0;
>>   	}
>>
>> @@ -1116,7 +1124,7 @@ 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 = {};
>> +	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_) {
>> @@ -1166,10 +1174,9 @@ void CameraDevice::requestComplete(Request *request)
>>   			buffer.status = CAMERA3_BUFFER_STATUS_ERROR;
>>   		}
>>
>> -		callbacks_->process_capture_result(callbacks_, &captureResult);
>> +		descriptor->status_ = Camera3RequestDescriptor::Status::Error;
>> +		sendCaptureResults();
>>
>> -		MutexLocker descriptorsLock(descriptorsMutex_);
>> -		descriptors_.pop();
>>   		return;
>>   	}
>>
>> @@ -1235,10 +1242,26 @@ void CameraDevice::requestComplete(Request *request)
>>   	}
>>
>>   	captureResult.result = resultMetadata->get();
>> -	callbacks_->process_capture_result(callbacks_, &captureResult);
>> +	descriptor->status_ = Camera3RequestDescriptor::Status::Success;
>> +	sendCaptureResults();
>> +}
>>
>> -	MutexLocker descriptorsLock(descriptorsMutex_);
>> -	descriptors_.pop();
>> +void CameraDevice::sendCaptureResults()
>> +{
>> +	MutexLocker lock(descriptorsMutex_);
>> +	while (!descriptors_.empty() && !descriptors_.front()->isPending()) {
>> +		auto descriptor = std::move(descriptors_.front());
>> +		descriptors_.pop();
>> +
>> +		/*
>> +		 * \todo Measure performance impact of grain locking here against
>> +		 * coarse locking.
> I find this a bit hard to parse. Maybe
>
> 		 * \todo Releasing and re-acquiring the critical
>                   * section for every request completion might have an
>                   * impact on performaces which should be measured.

s/performaces/performance/

I have updated to:

         * \todo Releasing and re-acquiring the critical section for
         * every request completion (grain-locking) might have an
         * impact on performance which should be measured.

>
> Up to you, really.
>
> All minors might be optionally fixed when applying
>
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>


Thanks!

>
>> +		 */
>> +		lock.unlock();
>> +		callbacks_->process_capture_result(callbacks_,
>> +						   &descriptor->captureResult_);
>> +		lock.lock();
>> +	}
>>   }
>>
>>   std::string CameraDevice::logPrefix() const
>> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
>> index 85497921..b7d774fe 100644
>> --- a/src/android/camera_device.h
>> +++ b/src/android/camera_device.h
>> @@ -74,17 +74,28 @@ private:
>>   	CameraDevice(unsigned int id, std::shared_ptr<libcamera::Camera> camera);
>>
>>   	struct Camera3RequestDescriptor {
>> +		enum class Status {
>> +			Pending,
>> +			Success,
>> +			Error,
>> +		};
>> +
>>   		Camera3RequestDescriptor() = default;
>>   		~Camera3RequestDescriptor() = default;
>>   		Camera3RequestDescriptor(libcamera::Camera *camera,
>>   					 const camera3_capture_request_t *camera3Request);
>>   		Camera3RequestDescriptor &operator=(Camera3RequestDescriptor &&) = default;
>>
>> +		bool isPending() const { return status_ == Status::Pending; }
>> +
>>   		uint32_t frameNumber_ = 0;
>>   		std::vector<camera3_stream_buffer_t> buffers_;
>>   		std::vector<std::unique_ptr<libcamera::FrameBuffer>> frameBuffers_;
>>   		CameraMetadata settings_;
>>   		std::unique_ptr<CaptureRequest> request_;
>> +
>> +		camera3_capture_result_t captureResult_ = {};
>> +		Status status_ = Status::Pending;
>>   	};
>>
>>   	enum class State {
>> @@ -99,12 +110,13 @@ private:
>>   	createFrameBuffer(const buffer_handle_t camera3buffer,
>>   			  libcamera::PixelFormat pixelFormat,
>>   			  const libcamera::Size &size);
>> -	void abortRequest(camera3_capture_request_t *request) const;
>> +	void abortRequest(Camera3RequestDescriptor *descriptor) const;
>>   	bool isValidRequest(camera3_capture_request_t *request) const;
>>   	void notifyShutter(uint32_t frameNumber, uint64_t timestamp);
>>   	void notifyError(uint32_t frameNumber, camera3_stream_t *stream,
>>   			 camera3_error_msg_code code) const;
>>   	int processControls(Camera3RequestDescriptor *descriptor);
>> +	void sendCaptureResults();
>>   	std::unique_ptr<CameraMetadata> getResultMetadata(
>>   		const Camera3RequestDescriptor &descriptor) const;
>>
>> --
>> 2.31.0
>>