[libcamera-devel] [PATCH v1 4/6] android: camera_device: Add a queue for sending capture results

[Laurent Pinchart]

On Wed, Sep 08, 2021 at 02:00:41PM +0100, Kieran Bingham wrote:
> On 07/09/2021 20:57, Umang Jain wrote:
> > When a camera capture request completes, the next step is to send the
> > capture results to the framework via process_capture_results(). All
> > the capture associated result metadata is prepared and populated. If
> > any post-processing is required, it will also take place in the same
> > thread (which can be blocking for a subtaintial amount of time) before
> 
> s/subtaintial/substantial/
> 
> > the results can be sent back to the framework.
> > 
> > A follow up commit will move the post-processing to run in a separate
> > thread. In order to do so, there is few bits of groundwork which this
> > patch entails. Mainly, we need to preserve the order of sending
> > the capture results back in which they were queued by the framework
> > to the HAL (i.e. the order is sequential). Hence, we need to add a queue
> > in which capture results can be queued with context.
> > 
> > As per this patch, the post-processor still runs synchronously as
> > before, but it will queue up the current capture results with context.
> > Later down the line, the capture results are dequeud in order and
> 
> s/dequeud/dequeued/
> 
> > sent back to the framework.
> > 
> > The context is preserved using Camera3RequestDescriptor utility
> > structure. It has been expanded accordingly to address the needs of
> > the functionality.
> > 
> > Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> > ---
> >  src/android/camera_device.cpp | 113 +++++++++++++++++++++++++++++++---
> >  src/android/camera_device.h   |  23 +++++++
> >  2 files changed, 126 insertions(+), 10 deletions(-)
> > 
> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > index f2f36f32..9d4ec02e 100644
> > --- a/src/android/camera_device.cpp
> > +++ b/src/android/camera_device.cpp
> > @@ -240,6 +240,8 @@ CameraDevice::Camera3RequestDescriptor::Camera3RequestDescriptor(
> >  	/* Clone the controls associated with the camera3 request. */
> >  	settings_ = CameraMetadata(camera3Request->settings);
> >  
> > +	internalBuffer_ = nullptr;
> > +
> >  	/*
> >  	 * Create the CaptureRequest, stored as a unique_ptr<> to tie its
> >  	 * lifetime to the descriptor.
> > @@ -989,6 +991,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> >  			 * once it has been processed.
> >  			 */
> >  			buffer = cameraStream->getBuffer();
> > +			descriptor.internalBuffer_ = buffer;
> >  			LOG(HAL, Debug) << ss.str() << " (internal)";
> >  			break;
> >  		}
> > @@ -1148,25 +1151,115 @@ void CameraDevice::requestComplete(Request *request)
> >  			continue;
> >  		}
> >  
> > +		/*
> > +		 * Save the current context of capture result and queue the
> > +		 * descriptor before processing the camera stream.
> > +		 *
> > +		 * When the processing completes, the descriptor will be
> > +		 * dequeued and capture results will be sent to the framework.
> > +		 */
> > +		descriptor.status_ = Camera3RequestDescriptor::NOT_FINISHED;
> > +		descriptor.resultMetadata_ = std::move(resultMetadata);
> > +		descriptor.captureResult_ = captureResult;
> > +
> > +		std::unique_ptr<Camera3RequestDescriptor> reqDescriptor =
> > +			std::make_unique<Camera3RequestDescriptor>();
> > +		*reqDescriptor = std::move(descriptor);
> > +		queuedDescriptor_.push_back(std::move(reqDescriptor));
> > +		Camera3RequestDescriptor *currentDescriptor = queuedDescriptor_.back().get();
> > +
> > +		CameraMetadata *metadata = currentDescriptor->resultMetadata_.get();
> > +		cameraStream->processComplete.connect(
> > +			this, [=](CameraStream::ProcessStatus status) {
> > +				streamProcessingComplete(cameraStream, status,
> > +							 metadata);
> 
> I believe this to be unsafe to manage multiple operations.
> 
> cameraStream->processComplete() should not be connected with per-run
> parameters. Those should be passed as the context of the signal emit(),
> not by the connection.
> 
> Otherwise, the lamba will be overwritten by the next requestComplete()
> and incorrect parameters will be processed when the signal actually fires.

It's worse than that, ever connect() will create a new connection, so
the slot will be called once for the first request, twice for the
second, ...

> > +			});
> > +
> >  		int ret = cameraStream->process(src, *buffer.buffer,
> > -						descriptor.settings_,
> > -						resultMetadata.get());
> > +						currentDescriptor->settings_,
> > +						metadata);
> > +		return;
> > +	}
> > +
> > +	if (queuedDescriptor_.empty()) {
> > +		captureResult.result = resultMetadata->get();
> > +		callbacks_->process_capture_result(callbacks_, &captureResult);
> 
> *1 duplication of completion callbacks (see below)
> 
> > +	} else {
> > +		/*
> > +		 * Previous capture results waiting to be sent to framework
> > +		 * hence, queue the current capture results as well. After that,
> > +		 * check if any results are ready to be sent back to the
> > +		 * framework.
> > +		 */
> > +		descriptor.status_ = Camera3RequestDescriptor::FINISHED_SUCCESS;
> > +		descriptor.resultMetadata_ = std::move(resultMetadata);
> > +		descriptor.captureResult_ = captureResult;
> > +		std::unique_ptr<Camera3RequestDescriptor> reqDescriptor =
> > +			std::make_unique<Camera3RequestDescriptor>();
> > +		*reqDescriptor = std::move(descriptor);
> > +		queuedDescriptor_.push_back(std::move(reqDescriptor));
> > +
> > +		sendQueuedCaptureResults();
> > +	}
> > +}
> > +
> > +void CameraDevice::streamProcessingComplete(CameraStream *cameraStream,
> > +					    CameraStream::ProcessStatus status,
> > +					    CameraMetadata *resultMetadata)
> > +{
> > +	for (auto &d : queuedDescriptor_) {
> > +		if (d->resultMetadata_.get() != resultMetadata)
> > +			continue;
> > +
> >  		/*
> >  		 * Return the FrameBuffer to the CameraStream now that we're
> >  		 * done processing it.
> >  		 */
> >  		if (cameraStream->type() == CameraStream::Type::Internal)
> > -			cameraStream->putBuffer(src);
> > -
> > -		if (ret) {
> > -			buffer.status = CAMERA3_BUFFER_STATUS_ERROR;
> > -			notifyError(descriptor.frameNumber_, buffer.stream,
> > -				    CAMERA3_MSG_ERROR_BUFFER);
> > +			cameraStream->putBuffer(d->internalBuffer_);
> > +
> > +		if (status == CameraStream::ProcessStatus::Success) {
> > +			d->status_ = Camera3RequestDescriptor::FINISHED_SUCCESS;
> > +		} else {
> > +			d->status_ = Camera3RequestDescriptor::FINISHED_FAILED;
> > +			d->captureResult_.partial_result = 0;
> > +			for (camera3_stream_buffer_t &buffer : d->buffers_) {
> > +				CameraStream *cs = static_cast<CameraStream *>(buffer.stream->priv);
> > +
> > +				if (cs->camera3Stream().format != HAL_PIXEL_FORMAT_BLOB)
> > +					continue;
> > +
> > +				buffer.status = CAMERA3_BUFFER_STATUS_ERROR;
> > +				notifyError(d->frameNumber_, buffer.stream,
> > +					    CAMERA3_MSG_ERROR_BUFFER);
> > +			}
> >  		}
> > +
> > +		break;
> >  	}
> >  
> > -	captureResult.result = resultMetadata->get();
> > -	callbacks_->process_capture_result(callbacks_, &captureResult);
> > +	/*
> > +	 * Send back capture results to the framework by inspecting the queue.
> > +	 * The framework can defer queueing further requests to the HAL (via
> > +	 * process_capture_request) unless until it receives the capture results
> > +	 * for already queued requests.
> > +	 */
> > +	sendQueuedCaptureResults();
> > +}
> > +
> > +void CameraDevice::sendQueuedCaptureResults()
> > +{
> > +	while (!queuedDescriptor_.empty()) {
> > +		std::unique_ptr<Camera3RequestDescriptor> &d = queuedDescriptor_.front();
> 
> I'd have this exit early if the front descriptor is not finished.
> 
> 
> > +		if (d->status_ != Camera3RequestDescriptor::NOT_FINISHED) {
> > +			d->captureResult_.result = d->resultMetadata_->get();
> > +			callbacks_->process_capture_result(callbacks_,
> > +							   &(d->captureResult_));
> 
> Not as part of this patch, but I feel like Camera3RequestDescriptor
> could have a method called complete() to wrap the management of the
> completion against a request descriptor.
> 
> There's lots of occasions where the metadata is obtained or processed
> into a result structure and called back with a flag.
> 
> (such as *1 above, but I think there are more)
> 
> It seems to me like that could be made into
> 
> 		d->complete(CameraRequest::Success);
> 
> or something.... But that's an overall design of breaking out the
> Camera3RequestDescriptor to a full class.
> 
> As this series is extending Camera3RequestDescriptor to maintain extra
> state required for the asynchronous post-processing, that makes me think
> it would be better to have a full class to manage that state. (and thus
> methods to operate on it)·
> 
> 
> 
> > +			queuedDescriptor_.pop_front();
> > +		} else {
> > +			break;
> > +		}
> > +	}
> >  }
> >  
> >  std::string CameraDevice::logPrefix() const
> > diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> > index a5576927..36425773 100644
> > --- a/src/android/camera_device.h
> > +++ b/src/android/camera_device.h
> > @@ -7,6 +7,7 @@
> >  #ifndef __ANDROID_CAMERA_DEVICE_H__
> >  #define __ANDROID_CAMERA_DEVICE_H__
> >  
> > +#include <deque>
> >  #include <map>
> >  #include <memory>
> >  #include <mutex>
> > @@ -83,6 +84,21 @@ private:
> >  		std::vector<std::unique_ptr<libcamera::FrameBuffer>> frameBuffers_;
> >  		CameraMetadata settings_;
> >  		std::unique_ptr<CaptureRequest> request_;
> > +
> > +		/*
> > +		 * Below are utility placeholders used when a capture result
> > +		 * needs to be queued before completion via
> > +		 * process_capture_result() callback.
> > +		 */
> > +		enum completionStatus {
> > +			NOT_FINISHED,
> 
> I'd call this "Active" or "Pending" to reduce the negation...
> 
> 	"If (state == Active)"
> rather than
> 	"if (state != NOT_FINISHED)"
> 
> 
> > +			FINISHED_SUCCESS,
> 
> I'd call this Success or Succeeded, or Successful ..
> 
> > +			FINISHED_FAILED,
> 
> And Failed
> 
> 
> 
> > +		};
> > +		std::unique_ptr<CameraMetadata> resultMetadata_;
> > +		camera3_capture_result_t captureResult_;
> > +		libcamera::FrameBuffer *internalBuffer_;
> > +		completionStatus status_;
> 
> These are all used to track the post-processor context.
> 
> 
> 
> 
> >  	};
> >  
> >  	enum class State {
> > @@ -105,6 +121,11 @@ private:
> >  	std::unique_ptr<CameraMetadata> getResultMetadata(
> >  		const Camera3RequestDescriptor &descriptor) const;
> >  
> > +	void sendQueuedCaptureResults();
> > +	void streamProcessingComplete(CameraStream *stream,
> > +				      CameraStream::ProcessStatus status,
> > +				      CameraMetadata *resultMetadata);
> > +
> >  	unsigned int id_;
> >  	camera3_device_t camera3Device_;
> >  
> > @@ -125,6 +146,8 @@ private:
> >  	libcamera::Mutex descriptorsMutex_; /* Protects descriptors_. */
> >  	std::map<uint64_t, Camera3RequestDescriptor> descriptors_;
> >  
> > +	std::deque<std::unique_ptr<Camera3RequestDescriptor>> queuedDescriptor_;
> 
> I'm not yet sure if this should be a unique_ptr ..
> 
> 
> > +
> >  	std::string maker_;
> >  	std::string model_;
> >  
> > 

-- 
Regards,

Laurent Pinchart