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

[Jacopo Mondi]

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)

> +	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
>