[libcamera-devel] [PATCH v3 09/10] android: camera_device: Send capture results inspecting the descriptor

[Umang Jain]

Hi Jacopo,

(Post in call discussion summary on this if anyone here is interested)

On 9/21/21 5:16 PM, Jacopo Mondi wrote:
> Hi Umang,
>
> On Mon, Sep 20, 2021 at 11:07:51PM +0530, Umang Jain wrote:
>> A Camera3RequestDescriptors is constructed and queued to descriptors_
>> queue as soon as an (incoming) capture request is received on the
>> libcamera HAL. The capture request is picked up by HAL, in order to run
>> it to completion. At completion, CameraDevice::requestComplete() gets
>> invoked and capture results are populated and ready to be sent back
>> to the framework.
>>
>> All the data and framebuffers associated with the request are alive
>> and encapsulated inside this Camera3RequestDescriptor descriptor.
>> By inspecting the ProcessStatus on the descriptor, we can now send
>> capture results via the process_capture_result() callback.
>>
>> Hence, introduce a new private member function sendCaptureResults()
>> which will be responsible to send capture results back to the
>> framework by inspecting the descriptor on the queue. In subsequent
>> commit, when the post processsor shall run async, sendCaptureResults()
>> can be called from the post-processor's thread for e.g.
>> streamProcessComplete() hence, introduce the mutex lock to avoid the
>> races.
>>
>> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
>> ---
>>   src/android/camera_device.cpp | 47 +++++++++++++++++++++++++----------
>>   src/android/camera_device.h   |  4 +++
>>   2 files changed, 38 insertions(+), 13 deletions(-)
>>
>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
>> index 4658e881..16ecdfc5 100644
>> --- a/src/android/camera_device.cpp
>> +++ b/src/android/camera_device.cpp
>> @@ -1078,7 +1078,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_) {
>> @@ -1156,26 +1156,25 @@ void CameraDevice::requestComplete(Request *request)
>>   			continue;
>>   		}
>>
>> +		if (cameraStream->type() == CameraStream::Type::Internal)
>> +			descriptor->internalBuffer_ = src;
>> +
>>   		descriptor->status_ = Camera3RequestDescriptor::ProcessStatus::Processing;
>>
>>   		cameraStream->process(src, *buffer.buffer, descriptor);
>> -
>> -		/*
>> -		 * Return the FrameBuffer to the CameraStream now that we're
>> -		 * done processing it.
>> -		 */
>> -		if (cameraStream->type() == CameraStream::Type::Internal)
>> -			cameraStream->putBuffer(src);
>> +		return;
>>   	}
>>
>> -	captureResult.result = descriptor->resultMetadata_->get();
>> -	callbacks_->process_capture_result(callbacks_, &captureResult);
>> -
>> -	descriptors_.pop_front();
>> +	/*
>> +	 * Mark the status on the descriptor as success as no processing
>> +	 * is neeeded.
>> +	 */
>> +	descriptor->status_ = Camera3RequestDescriptor::ProcessStatus::Success;
>> +	sendCaptureResults();
>>   }
>>
>>
>> -void CameraDevice::streamProcessingComplete([[maybe_unused]] CameraStream *cameraStream,
>> +void CameraDevice::streamProcessingComplete(CameraStream *cameraStream,
>>   					    Camera3RequestDescriptor *request)
>>   {
>>   	if (request->status_ == Camera3RequestDescriptor::ProcessStatus::Error) {
>> @@ -1190,6 +1189,28 @@ void CameraDevice::streamProcessingComplete([[maybe_unused]] CameraStream *camer
>>   				    CAMERA3_MSG_ERROR_BUFFER);
>>   		}
>>   	}
>> +
>> +	/*
>> +	 * Return the FrameBuffer to the CameraStream now that we're
>> +	 * done processing it.
>> +	 */
>> +	if (cameraStream->type() == CameraStream::Type::Internal)
>> +		cameraStream->putBuffer(request->internalBuffer_);
>> +
>> +	sendCaptureResults();
>> +}
>> +
>> +void CameraDevice::sendCaptureResults()
>> +{
>> +	Camera3RequestDescriptor *d = descriptors_.front().get();
> If I'm not mistaken, in case a request that doesn't need
> post-processing will complete while the post-processed one is still
> being worked on, this could lead to a completion order inversion
>
> processCaptureRequest(n)
>          descriptors.push(n)
>
> processCaptureRequest(n+1)
>          descriptors.push(n+1)
>
> equestComplete(n)
>          postProcess(n)     ----------->    process(n)
>                                                  |
>          ....                                   ...
>                                                  |
> requestComplete(n+1)                            |
>          sendCaptureResult(n+1)                  |
>                  d = descriptors.front()         |
>                  process_capture_res(n)          |
>                                                  |
>          sendCaptureResult(n)   <----------------
>                  d = descritpros.front()
>                  process_capture_res(n + 1)
>

Nice flow diagram btw, I need to get better at that ;-)

So, first of all sendCaptureResults() doesn't take an argument, but I 
get your point that you are trying to match for which 'n' request, 
sendCapture result is called...

sendCaptureResults() works on the queue's head. It doesn't care from 
where it's called; it can be called from requestComplete()[same thread] 
or streamProcessingComplete()[different thread]. All it does or suppose 
to do, is basically looks at the queue's head and see if the HEAD 
descriptor  says - "Send me back to the framework".
- If yes, it will send the results via process_capture_result  of the 
HEAD and drop the HEAD from the queue
- If no, simply return;

That's it. Since, it looks at the head, it will only send results in 
order (since it's a queue) and drop the HEAD.

However, the discussion has un-covered another point that 
sendCaptureResults() can lag behind w.r.t libcamera:Requests being 
completed, since there is no loop around to process other descriptors in 
the queue which are ready to send (these descriptors can potentially be 
the ones not requiring any post-processing). As discussed in the call, 
we both agree that we should probably iterate over the queue and send 
the results back to the framework as soon as possible, for ready 
descriptor. So, sendCaptureResults() will be wrapped in a loop in 
subsequent version.


>> +	if (d->status_ == Camera3RequestDescriptor::ProcessStatus::Processing ||
>> +	    d->status_ == Camera3RequestDescriptor::ProcessStatus::None)
>> +		return;
>> +
>> +	MutexLocker lock(descriptorsMutex_);
>> +	d->captureResult_.result = d->resultMetadata_->get();
>> +	callbacks_->process_capture_result(callbacks_, &(d->captureResult_));
>> +	descriptors_.pop_front();
>>   }
>>
>>   std::string CameraDevice::logPrefix() const
>> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
>> index 60c134dc..0bd570a1 100644
>> --- a/src/android/camera_device.h
>> +++ b/src/android/camera_device.h
>> @@ -56,6 +56,9 @@ struct Camera3RequestDescriptor {
>>   	std::unique_ptr<CaptureRequest> request_;
>>   	std::unique_ptr<CameraMetadata> resultMetadata_;
>>
>> +	camera3_capture_result_t captureResult_ = {};
>> +	libcamera::FrameBuffer *internalBuffer_;
>> +
>>   	ProcessStatus status_;
>>   };
>>
>> @@ -118,6 +121,7 @@ private:
>>   	int processControls(Camera3RequestDescriptor *descriptor);
>>   	std::unique_ptr<CameraMetadata> getResultMetadata(
>>   		const Camera3RequestDescriptor *descriptor) const;
>> +	void sendCaptureResults();
>>
>>   	unsigned int id_;
>>   	camera3_device_t camera3Device_;
>> --
>> 2.31.1
>>