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

[Jacopo Mondi]

Hi Umang,

On Tue, Sep 21, 2021 at 01:46:18PM +0200, 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)

as you have clarified offline this can't happen as you skip
not-completed requests in sendCaptureResult().

At the same time it seems you need to iterate the queue when the
front() request has completed, to complete the next ones which are
ready but whose completion was post-poned.

Thanks
   j

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