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

[Jacopo Mondi]

Hi Umang,

On Wed, Sep 29, 2021 at 02:04:50PM +0530, Umang Jain wrote:
> Hi Jacopo,
>
> On 9/29/21 1:47 PM, Jacopo Mondi wrote:
> > Hi Umang,
> >
> > On Wed, Sep 29, 2021 at 12:47:08PM +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
> > > 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>
> > > ---
> > >   src/android/camera_device.cpp | 52 +++++++++++++++++++++++------------
> > >   src/android/camera_device.h   | 14 +++++++++-
> > >   2 files changed, 48 insertions(+), 18 deletions(-)
> > >
> > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > > index 499baec8..fab5a854 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)
> > > +void CameraDevice::abortRequest(Camera3RequestDescriptor *descriptor)
> > >   {
> > > -	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
> > > @@ -1051,13 +1051,19 @@ 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 abort the request and push the descriptor to
> > > +	 * the queue. If the camera has been stopped we have to re-start it to
> > I would
> >
> > 	 * 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();
> > Empty lines around sendCaptureResults() maybe to give the code some
> > more room to breath
> >
> > >   		return 0;
> > >   	}
> > >
> > > @@ -1115,7 +1121,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_) {
> > > @@ -1165,9 +1171,9 @@ void CameraDevice::requestComplete(Request *request)
> > >   			buffer.status = CAMERA3_BUFFER_STATUS_ERROR;
> > >   		}
> > >
> > > -		callbacks_->process_capture_result(callbacks_, &captureResult);
> > > +		descriptor->status_ = Camera3RequestDescriptor::Status::Error;
> > > +		sendCaptureResults();
> > >
> > > -		descriptors_.pop();
> > >   		return;
> > >   	}
> > >
> > > @@ -1233,10 +1239,22 @@ void CameraDevice::requestComplete(Request *request)
> > >   	}
> > >
> > >   	captureResult.result = resultMetadata->get();
> > This now becomes a problem. Before this patch we called back the
> > framework right here, and we were guaranteed resultMetadata was still
> > in scope. During the callback the framework copied the result metadata
> > and when process_capture_result() returned captureResult went out
> > of scope and got released.
> >
> > Now that a request sits in the queue and can be completed at a later
> > time, when it is passed to the framework captureResult.result points
>
>
> I have considered this. Please look at the sendCaptureResults() right after
> we set the state to ::Success
>
> The "at a later time" scenario, according to me, emerges when something is
> asynchronous, making a descriptor block while other ::Success descriptor
> queued. Currently everything is synchronous and every descriptor is
> synchronously processed as soon as we set a state on it. So, the
> resultMetdata should be valid until sendCaptureResults() returns.
>
> Hence, we should be handling this in upcoming post-processing series rather
> than this?  Does it make sense?

Indeed it does, sorry I was thinking already in the async
post-processing mode :)

As long as this is under your radar when moving the post-processing to
be async, it's all good!

Thanks
   j
>
> > to a memory location that has been invalidated because the here
> > declared unique_ptr<> has gone out of scope.
> >
> > I think we should make just store a
> >          std::unique_ptr<CameraMetadata> resultMetadata;
> > in the Camera3RequestDescriptor and set captureResult.result before
> > calling the process_capture_result callback.
>
>
> Yep, that's what should be happening as here:
>
> https://patchwork.libcamera.org/patch/13869/
>
> >
> > Thanks
> >     j
> >
> > > -	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();
> > > +
> > > +		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 9ec510d5..69c78f08 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);
> > > +	void abortRequest(Camera3RequestDescriptor *descriptor);
> > >   	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);
> > >   	int processControls(Camera3RequestDescriptor *descriptor);
> > > +	void sendCaptureResults();
> > >   	std::unique_ptr<CameraMetadata> getResultMetadata(
> > >   		const Camera3RequestDescriptor &descriptor) const;
> > >
> > > --
> > > 2.31.0
> > >