[libcamera-devel] [PATCH v5 8/8] android: Implement flush() camera operation

[Jacopo Mondi]

Hi Me and Laurent

On Tue, Jun 08, 2021 at 11:42:48PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Tue, Jun 08, 2021 at 05:16:33PM +0200, Jacopo Mondi wrote:
> > Implement the flush() camera operation in the CameraDevice class
> > and make it available to the camera framework by implementing the
> > operation wrapper in camera_ops.cpp.
> >
> > Introduce a new camera state State::Flushing to handle concurrent
> > flush() and process_capture_request() calls.
> >
> > As flush() can race with processCaptureRequest() protect it
> > by introducing a new State::Flushing state that
> > processCaptureRequest() inspects before queuing the Request to the
> > Camera. If flush() is in progress while processCaptureRequest() is
> > called, return the current Request immediately in error state. If
> > flush() has completed and a new call to processCaptureRequest() is
> > made just after, start the camera again before queuing the request.
> >
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > Reviewed-by: Hirokazu Honda <hiroh at chromium.org>
> > Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > ---
> >  src/android/camera_device.cpp | 77 +++++++++++++++++++++++++++--------
> >  src/android/camera_device.h   |  3 ++
> >  src/android/camera_ops.cpp    |  8 +++-
> >  3 files changed, 71 insertions(+), 17 deletions(-)
> >
> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > index b29c1edc9970..3adb657bfeca 100644
> > --- a/src/android/camera_device.cpp
> > +++ b/src/android/camera_device.cpp
> > @@ -798,6 +798,23 @@ void CameraDevice::close()
> >  	camera_->release();
> >  }
> >
> > +void CameraDevice::flush()
> > +{
> > +	{
> > +		MutexLocker stateLock(stateMutex_);
> > +		if (state_ != State::Running)
> > +			return;
> > +
> > +		state_ = State::Flushing;
> > +	}
> > +
> > +	worker_.stop();
> > +	camera_->stop();
> > +
> > +	MutexLocker stateLock(stateMutex_);
> > +	state_ = State::Stopped;
> > +}
> > +
> >  void CameraDevice::stop()
> >  {
> >  	MutexLocker stateLock(stateMutex_);
> > @@ -1896,27 +1913,31 @@ int CameraDevice::processControls(Camera3RequestDescriptor *descriptor)
> >  	return 0;
> >  }
> >
> > -int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Request)
> > +void CameraDevice::abortRequest(camera3_capture_request_t *request)
> >  {
> > -	if (!isValidRequest(camera3Request))
> > -		return -EINVAL;
> > +	notifyError(request->frame_number, nullptr, CAMERA3_MSG_ERROR_REQUEST);
> >
> > -	{
> > -		MutexLocker stateLock(stateMutex_);
> > +	camera3_capture_result_t result = {};
> > +	result.num_output_buffers = request->num_output_buffers;
> > +	result.frame_number = request->frame_number;
> > +	result.partial_result = 0;
> >
> > -		/* Start the camera if that's the first request we handle. */
> > -		if (state_ == State::Stopped) {
> > -			worker_.start();
> > +	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.acquire_fence = -1;
> > +		buffer.status = CAMERA3_BUFFER_STATUS_ERROR;
> > +	}
> > +	result.output_buffers = resultBuffers.data();
> >
> > -			int ret = camera_->start();
> > -			if (ret) {
> > -				LOG(HAL, Error) << "Failed to start camera";
> > -				return ret;
> > -			}
> > +	callbacks_->process_capture_result(callbacks_, &result);
> > +}
> >
> > -			state_ = State::Running;
> > -		}
> > -	}
> > +int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Request)
> > +{
> > +	if (!isValidRequest(camera3Request))
> > +		return -EINVAL;
> >
> >  	/*
> >  	 * Save the request descriptors for use at completion time.
> > @@ -2006,6 +2027,30 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> >  	if (ret)
> >  		return ret;
> >
> > +	/*
> > +	 * Just before queuing the request, make sure flush() has not
> > +	 * been called while this function was running. If flush is in progress
> > +	 * abort the request. If flush has completed and has stopped the camera
> > +	 * we have to re-start it to be able to process the request.
> > +	 */
> > +	MutexLocker stateLock(stateMutex_);
>
> I'd add a blank line here.

I would also take the occasion to re-phrase the above comment to

+        * 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.
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
> > +	if (state_ == State::Flushing) {
> > +		abortRequest(camera3Request);
> > +		return 0;
> > +	}
> > +
> > +	if (state_ == State::Stopped) {
> > +		worker_.start();
> > +
> > +		ret = camera_->start();
> > +		if (ret) {

And stop the worker_ if we're returning here

With those I presume I can push the series

Thanks
  j

> > +			LOG(HAL, Error) << "Failed to start camera";
> > +			return ret;
> > +		}
> > +
> > +		state_ = State::Running;
> > +	}
> > +
> >  	worker_.queueRequest(descriptor.request_.get());
> >
> >  	{
> > diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> > index c949fa509ca4..4aadb27c562c 100644
> > --- a/src/android/camera_device.h
> > +++ b/src/android/camera_device.h
> > @@ -43,6 +43,7 @@ public:
> >
> >  	int open(const hw_module_t *hardwareModule);
> >  	void close();
> > +	void flush();
> >
> >  	unsigned int id() const { return id_; }
> >  	camera3_device_t *camera3Device() { return &camera3Device_; }
> > @@ -92,6 +93,7 @@ private:
> >
> >  	enum class State {
> >  		Stopped,
> > +		Flushing,
> >  		Running,
> >  	};
> >
> > @@ -106,6 +108,7 @@ private:
> >  	getRawResolutions(const libcamera::PixelFormat &pixelFormat);
> >
> >  	libcamera::FrameBuffer *createFrameBuffer(const buffer_handle_t camera3buffer);
> > +	void abortRequest(camera3_capture_request_t *request);
> >  	void notifyShutter(uint32_t frameNumber, uint64_t timestamp);
> >  	void notifyError(uint32_t frameNumber, camera3_stream_t *stream,
> >  			 camera3_error_msg_code code);
> > diff --git a/src/android/camera_ops.cpp b/src/android/camera_ops.cpp
> > index 696e80436821..8a3cfa175ff5 100644
> > --- a/src/android/camera_ops.cpp
> > +++ b/src/android/camera_ops.cpp
> > @@ -66,8 +66,14 @@ static void hal_dev_dump([[maybe_unused]] const struct camera3_device *dev,
> >  {
> >  }
> >
> > -static int hal_dev_flush([[maybe_unused]] const struct camera3_device *dev)
> > +static int hal_dev_flush(const struct camera3_device *dev)
> >  {
> > +	if (!dev)
> > +		return -EINVAL;
> > +
> > +	CameraDevice *camera = reinterpret_cast<CameraDevice *>(dev->priv);
> > +	camera->flush();
> > +
> >  	return 0;
> >  }
> >
>
> --
> Regards,
>
> Laurent Pinchart