[libcamera-devel] [PATCH 03/11] android: camera_device: Build capture_result dynamically

[Jacopo Mondi]

Hi Umang,

On Mon, Oct 18, 2021 at 09:42:24PM +0530, Umang Jain wrote:
> Hi Jacopo
>
> On 10/18/21 9:18 PM, Jacopo Mondi wrote:
> > Hi Umang,
> >
> > On Mon, Oct 18, 2021 at 06:59:15PM +0530, Umang Jain wrote:
> > > From: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > >
> > > The camera3_capture_result_t is only needed to convey capture results to
> > > the camera service through the process_capture_result() callback.
> > > There's no need to store it in the Camera3RequestDescriptor. Build it
> > > dynamically in CameraDevice::sendCaptureResults() instead.
> > >
> > > This requires storing the result metadata created in
> > > CameraDevice::requestComplete() in the Camera3RequestDescriptor. A side
> > > effect of this change is that the request metadata lifetime will match
> > > the Camera3RequestDescriptor instead of being destroyed at the end of
> > > requestComplete(). This will be needed to support asynchronous
> > > post-processing, where the request completion will be signaled to the
> > > camera service asynchronously from requestComplete().
> > >
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > > Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> > > ---
> > >   src/android/camera_device.cpp | 43 ++++++++++++++++-------------------
> > >   src/android/camera_request.h  |  2 +-
> > >   2 files changed, 21 insertions(+), 24 deletions(-)
> > >
> > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > > index b4ab5da1..c6ae8930 100644
> > > --- a/src/android/camera_device.cpp
> > > +++ b/src/android/camera_device.cpp
> > > @@ -830,19 +830,11 @@ void CameraDevice::abortRequest(Camera3RequestDescriptor *descriptor) const
> > >   {
> > >   	notifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_REQUEST);
> > >
> > > -	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 = descriptor->buffers_[i];
> > > -		buffer.release_fence = descriptor->buffers_[i].acquire_fence;
> > > +	for (auto &buffer : descriptor->buffers_) {
> > > +		buffer.release_fence = buffer.acquire_fence;
> > >   		buffer.acquire_fence = -1;
> > >   		buffer.status = CAMERA3_BUFFER_STATUS_ERROR;
> > >   	}
> > > -	result.output_buffers = resultBuffers.data();
> > >
> > >   	descriptor->status_ = Camera3RequestDescriptor::Status::Error;
> > >   }
> > > @@ -1090,9 +1082,6 @@ 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 = descriptor->captureResult_;
> > > -	captureResult.frame_number = descriptor->frameNumber_;
> > > -	captureResult.num_output_buffers = descriptor->buffers_.size();
> > >   	for (camera3_stream_buffer_t &buffer : descriptor->buffers_) {
> > >   		CameraStream *cameraStream =
> > >   			static_cast<CameraStream *>(buffer.stream->priv);
> > > @@ -1113,8 +1102,6 @@ void CameraDevice::requestComplete(Request *request)
> > >   		buffer.release_fence = -1;
> > >   		buffer.status = CAMERA3_BUFFER_STATUS_OK;
> > >   	}
> > > -	captureResult.output_buffers = descriptor->buffers_.data();
> > > -	captureResult.partial_result = 1;
> > >
> > >   	/*
> > >   	 * If the Request has failed, abort the request by notifying the error
> > > @@ -1128,7 +1115,6 @@ void CameraDevice::requestComplete(Request *request)
> > >   		notifyError(descriptor->frameNumber_, nullptr,
> > >   			    CAMERA3_MSG_ERROR_REQUEST);
> > >
> > > -		captureResult.partial_result = 0;
> > >   		for (camera3_stream_buffer_t &buffer : descriptor->buffers_) {
> > >   			/*
> > >   			 * Signal to the framework it has to handle fences that
> > > @@ -1165,12 +1151,12 @@ void CameraDevice::requestComplete(Request *request)
> > >   	 * Notify if the metadata generation has failed, but continue processing
> > >   	 * buffers and return an empty metadata pack.
> > >   	 */
> > > -	std::unique_ptr<CameraMetadata> resultMetadata = getResultMetadata(*descriptor);
> > > -	if (!resultMetadata) {
> > > +	descriptor->resultMetadata_ = getResultMetadata(*descriptor);
> > > +	if (!descriptor->resultMetadata_) {
> > >   		notifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_RESULT);
> > >
> > >   		/* The camera framework expects an empty metadata pack on error. */
> > > -		resultMetadata = std::make_unique<CameraMetadata>(0, 0);
> > > +		descriptor->resultMetadata_ = std::make_unique<CameraMetadata>(0, 0);
> > Since we don't sendCaptureResult() and return here we get to
> > post-processing with a (0, 0) metadata pack. I wonder if the
> > post-processing code is robust enough to cope with that situation
> > (even if I suspect I already know the answer :)
> >
> > It is my understanding the situation is like this already, so it's
> > fine, but maybe we should add a todo.
>
>
> \todo stating what? The situation or the solution. I am not sure of a
> potential solution of this yet
>

I wish I had a solution. Just recording the potential issue, if you
think it's opportune.

> >
> > >   	}
> > >
> > >   	/* Handle post-processing. */
> > > @@ -1192,7 +1178,7 @@ void CameraDevice::requestComplete(Request *request)
> > >
> > >   		int ret = cameraStream->process(*src, buffer,
> > >   						descriptor->settings_,
> > > -						resultMetadata.get());
> > > +						descriptor->resultMetadata_.get());
> > >   		/*
> > >   		 * Return the FrameBuffer to the CameraStream now that we're
> > >   		 * done processing it.
> > > @@ -1207,7 +1193,6 @@ void CameraDevice::requestComplete(Request *request)
> > >   		}
> > >   	}
> > >
> > > -	captureResult.result = resultMetadata->get();
> > >   	descriptor->status_ = Camera3RequestDescriptor::Status::Success;
> > >   	sendCaptureResults();
> > >   }
> > > @@ -1225,8 +1210,20 @@ void CameraDevice::sendCaptureResults()
> > >   		 * impact on performance which should be measured.
> > >   		 */
> > >   		lock.unlock();
> > > -		callbacks_->process_capture_result(callbacks_,
> > > -						   &descriptor->captureResult_);
> > > +
> > > +		camera3_capture_result_t captureResult = {};
> > > +
> > > +		captureResult.frame_number = descriptor->frameNumber_;
> > > +		if (descriptor->resultMetadata_)
> > Do we need this ? As resultMetadata_ is a unique_ptr<> it is
> > constructed owning nothing, and calling get() on it simnply return
> > nullptr, which I think it's what we want.
>
>
> Yes we need this. There are two get() here
>
> a) one of the unique_ptr one, .get()
>
> b) another is the descriptor->resultMetadata_->get()) one
>
> If descriptor->resultMetadata_ is nullptr (initialized), ->get() on it, 
> will segfault
>
> So I found best to guard it with an if () block for now. Does it make sense?
>

Sorry, I confused unique_ptr<>.get() with CameraMetadata::get().

> >
> > Thanks
> >     j
> > > +			captureResult.result = descriptor->resultMetadata_->get();
> > > +		captureResult.num_output_buffers = descriptor->buffers_.size();
> > > +		captureResult.output_buffers = descriptor->buffers_.data();
> > > +
> > > +		if (descriptor->status_ == Camera3RequestDescriptor::Status::Success)
> > > +			captureResult.partial_result = 1;
> > > +
> > > +		callbacks_->process_capture_result(callbacks_, &captureResult);
> > > +
> > >   		lock.lock();
> > >   	}
> > >   }
> > > diff --git a/src/android/camera_request.h b/src/android/camera_request.h
> > > index 79dfdb58..db13f624 100644
> > > --- a/src/android/camera_request.h
> > > +++ b/src/android/camera_request.h
> > > @@ -40,8 +40,8 @@ public:
> > >   	std::vector<std::unique_ptr<libcamera::FrameBuffer>> frameBuffers_;
> > >   	CameraMetadata settings_;
> > >   	std::unique_ptr<CaptureRequest> request_;
> > > +	std::unique_ptr<CameraMetadata> resultMetadata_;
> > >
> > > -	camera3_capture_result_t captureResult_ = {};
> > >   	Status status_ = Status::Pending;
> > >
> > >   private:
> > > --
> > > 2.31.0
> > >