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

[Jacopo Mondi]

Hi Niklas,

On Mon, May 10, 2021 at 10:39:09PM +0200, Niklas Söderlund wrote:
> Hi Jacopo,
>
> Thanks for your work.
>
> On 2021-05-10 12:52:35 +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.
> >
> > The flush() implementation stops the Camera and the worker thread and
> > waits for all in-flight requests to be returned. Stopping the Camera
> > forces all Requests already queued to be returned immediately in error
> > state. As flush() has to wait until all of them have been returned, make it
> > wait on a newly introduced condition variable which is notified by the
> > request completion handler when the queue of pending requests has been
> > exhausted.
> >
> > As flush() can race with processCaptureRequest() protect the requests
> > queueing by introducing a new CameraState::CameraFlushing state that
> > processCaptureRequest() inspects before queuing the Request to the
> > Camera. If flush() has been called while processCaptureRequest() was
> > executing, return the current Request immediately in error state.
> >
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> >  src/android/camera_device.cpp | 63 +++++++++++++++++++++++++++++++++++
> >  src/android/camera_device.h   |  6 ++++
> >  src/android/camera_ops.cpp    |  8 ++++-
> >  3 files changed, 76 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > index fa12ce5b0133..01b3acd93c4b 100644
> > --- a/src/android/camera_device.cpp
> > +++ b/src/android/camera_device.cpp
> > @@ -756,6 +756,42 @@ void CameraDevice::close()
> >  	camera_->release();
> >  }
> >
> > +/*
> > + * Flush is similar to stop() but sets the camera state to 'flushing' and wait
> > + * until all the in-flight requests have been returned.
> > + */
> > +void CameraDevice::flush()
> > +{
> > +	{
> > +		MutexLocker cameraLock(cameraMutex_);
> > +
> > +		if (state_ != CameraRunning)
> > +			return;
> > +
> > +		state_ = CameraFlushing;
> > +
> > +		/*
> > +		 * Stop the camera and set the state to flushing to prevent any
> > +		 * new request to be queued from this point on.
> > +		 */
> > +		worker_.stop();
> > +		camera_->stop();
> > +	}
>
> Releasing cameraMutex_ while waiting for the flushMutex_ signal seems
> like a race waiting to happen as the operation is acting in
> synchronization with the state_ statemachine.

The two operations happens one after the other, not at the same time,
am I mistaken ?

>
> I understand you release it as you want to lock it in
> CameraDevice::stop(), is there any other potential race site? If not

Nope, that's not CameraDevice::stop() but raher
libcamera::Camera::stop()

cameraMutex serves to protect against any racing call to
processCaptureRequest() that wants to inspect the camera state to
decide if the request has to be queued or returned with error.


> maybe CameraDevice::stop() can be reworked? It only needs to read the
> state so is the mutex really needed, could it be reworked to an atomic?
> Or maybe there is something in the HAL itself that would prevent other
> callbacks to change the state from CameraFlushing -> CameraRunning while
> flush() is executing?
>
> > +
> > +	/*
> > +	 * Now wait for all the in-flight requests to be completed before
> > +	 * returning. Stopping the Camera guarantees that all in-flight requests
> > +	 * are completed in error state.
> > +	 */
> > +	{
> > +		MutexLocker flushLock(flushMutex_);
> > +		flushing_.wait(flushLock, [&] { return descriptors_.empty(); });
> > +	}
> > +
> > +	MutexLocker cameraLock(cameraMutex_);
> > +	state_ = CameraStopped;
> > +}
> > +
> >  void CameraDevice::stop()
> >  {
> >  	MutexLocker cameraLock(cameraMutex_);
> > @@ -2019,6 +2055,26 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> >  	if (ret)
> >  		return ret;
> >
> > +
> > +	/*
> > +	 * Just before queuing the request, make sure flush() has not
> > +	 * been called after this function has been executed. In that
> > +	 * case, immediately return the request with errors.
> > +	 */
> > +	MutexLocker cameraLock(cameraMutex_);
> > +	if (state_ == CameraFlushing || state_ == CameraStopped) {
> > +		for (camera3_stream_buffer_t &buffer : descriptor.buffers_) {
> > +			buffer.status = CAMERA3_BUFFER_STATUS_ERROR;
> > +			buffer.release_fence = buffer.acquire_fence;
> > +		}
> > +
> > +		notifyError(descriptor.frameNumber_,
> > +			    descriptor.buffers_[0].stream,
> > +			    CAMERA3_MSG_ERROR_REQUEST);
> > +
> > +		return 0;
> > +	}
> > +
> >  	worker_.queueRequest(descriptor.request_.get());
> >
> >  	{
> > @@ -2052,6 +2108,13 @@ void CameraDevice::requestComplete(Request *request)
> >  	}
> >  	Camera3RequestDescriptor &descriptor = node.mapped();
> >
> > +	/* Release flush if all the pending requests have been completed. */
> > +	{
> > +		MutexLocker flushLock(flushMutex_);
> > +		if (descriptors_.empty())
> > +			flushing_.notify_one();
> > +	}
> > +
> >  	/*
> >  	 * Prepare the capture result for the Android camera stack.
> >  	 *
> > diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> > index ed992ae56d5d..4a9ef495b776 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 <condition_variable>
> >  #include <map>
> >  #include <memory>
> >  #include <mutex>
> > @@ -42,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 +94,7 @@ private:
> >  	enum State {
> >  		CameraStopped,
> >  		CameraRunning,
> > +		CameraFlushing,
> >  	};
> >
> >  	void stop();
> > @@ -124,6 +127,9 @@ private:
> >  	libcamera::Mutex cameraMutex_; /* Protects access to the camera state. */
> >  	State state_;
> >
> > +	libcamera::Mutex flushMutex_; /* Protects the flushing condition variable. */
> > +	std::condition_variable flushing_;
> > +
> >  	std::shared_ptr<libcamera::Camera> camera_;
> >  	std::unique_ptr<libcamera::CameraConfiguration> config_;
> >
> > 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;
> >  }
> >
> > --
> > 2.31.1
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
>
> --
> Regards,
> Niklas Söderlund