[libcamera-devel] [PATCH] android: Implement flush()

[Jacopo Mondi]

Hi Hiro,

On Tue, Apr 06, 2021 at 06:13:34PM +0900, Hirokazu Honda wrote:
> This implements camera3_device_ops::flush().
>

Is this enough ? reading the flush() operation documentation in
camera3.h it seems it can be called concurrently to
process_capture_request() and requests yet to be queued shall be
returned immediately. Should that be handled as well ?

> Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> ---
>  src/android/camera_device.cpp | 48 +++++++++++++++++++++++++++++++----
>  src/android/camera_device.h   |  3 +++
>  src/android/camera_ops.cpp    |  3 ++-
>  3 files changed, 48 insertions(+), 6 deletions(-)
>
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index 4a3f6f8e..5a56fe4b 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -2029,20 +2029,22 @@ void CameraDevice::requestComplete(Request *request)
>  	camera3_buffer_status status = CAMERA3_BUFFER_STATUS_OK;
>  	std::unique_ptr<CameraMetadata> resultMetadata;
>
> -	decltype(descriptors_)::node_type node;
> +	Camera3RequestDescriptor descriptor;
>  	{
>  		std::scoped_lock<std::mutex> lock(mutex_);
>  		auto it = descriptors_.find(request->cookie());
>  		if (it == descriptors_.end()) {
>  			LOG(HAL, Fatal)
>  				<< "Unknown request: " << request->cookie();
> -			status = CAMERA3_BUFFER_STATUS_ERROR;
>  			return;
>  		}
>
> -		node = descriptors_.extract(it);
> +		descriptor = std::move(it->second);
> +		/* Restore frameNumber_ because it is used in the case of flush
> +		 * timeout case.
> +		 */
> +		it->second.frameNumber_ = descriptor.frameNumber_;

Is accessing it->second after having moved it valid ? also, aren't
the lhs and rhs referring to the same variable ?

>  	}
> -	Camera3RequestDescriptor &descriptor = node.mapped();
>
>  	if (request->status() != Request::RequestComplete) {
>  		LOG(HAL, Error) << "Request not successfully completed: "
> @@ -2122,7 +2124,43 @@ void CameraDevice::requestComplete(Request *request)
>  			    descriptor.buffers_[0].stream);
>  	}
>
> -	callbacks_->process_capture_result(callbacks_, &captureResult);
> +	{
> +		std::scoped_lock<std::mutex> lock(mutex_);
> +		if (descriptors_.erase(request->cookie()) == 0) {
> +			// flush() cleans up the descriptor due to time out
> +			// during a post processing.

No C++ comments please

> +			return;
> +		}
> +		callbacks_->process_capture_result(callbacks_, &captureResult);
> +		conditionVariable_.notify_one();

aren't this function and flush() being run in the same thread ? Won't
this contend the mutex ?

To be honest I would have expected flush to stop the libcamera::Camera
to have all requests in-flight be immediately be completed with
errors. As said flush also prevents calls to
process_capture_request() to be queued if flush is called concurrently
(which makes me think my assumption on the flush calls happening on the same
thread to be wrong).

Another question is how to interrupt post-processing to happen once it
will be moved to a separate thread. If flush() times out and delete
the descriptor, won't post-processor then tries to access a non-valid
memory address ?

Thanks
  j

> +	}
> +}
> +
> +int CameraDevice::flush()
> +{
> +	std::unique_lock<std::mutex> lock(mutex_);
> +	/* flush() should return in less than one second. Sets the timeout to 900ms. */
> +	auto flushTimeout =
> +		std::chrono::system_clock::now() + std::chrono::milliseconds(900);
> +	bool completeAllRequests = conditionVariable_.wait_until(
> +		lock, flushTimeout, [this]() { return descriptors_.empty(); });
> +
> +	if (!completeAllRequests) {
> +		for (auto &node : descriptors_) {
> +			auto &descriptor = node.second;
> +			camera3_capture_result_t captureResult{};
> +			captureResult.frame_number = descriptor.frameNumber_;
> +			for (camera3_stream_buffer_t &buffer : descriptor.buffers_) {
> +				buffer.acquire_fence = -1;
> +				buffer.release_fence = -1;
> +				buffer.status = CAMERA3_BUFFER_STATUS_ERROR;
> +			}
> +			callbacks_->process_capture_result(callbacks_, &captureResult);
> +		}
> +		descriptors_.clear();
> +	}
> +
> +	return 0;
>  }
>
>  std::string CameraDevice::logPrefix() const
> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> index c63e8e21..dd5336ee 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>
> @@ -62,6 +63,7 @@ public:
>  	int configureStreams(camera3_stream_configuration_t *stream_list);
>  	int processCaptureRequest(camera3_capture_request_t *request);
>  	void requestComplete(libcamera::Request *request);
> +	int flush();
>
>  protected:
>  	std::string logPrefix() const override;
> @@ -128,6 +130,7 @@ private:
>  	std::vector<CameraStream> streams_;
>
>  	std::mutex mutex_; /* Protect descriptors_ */
> +	std::condition_variable conditionVariable_;
>  	std::map<uint64_t, Camera3RequestDescriptor> descriptors_;
>
>  	std::string maker_;
> diff --git a/src/android/camera_ops.cpp b/src/android/camera_ops.cpp
> index 696e8043..1b73af13 100644
> --- a/src/android/camera_ops.cpp
> +++ b/src/android/camera_ops.cpp
> @@ -68,7 +68,8 @@ static void hal_dev_dump([[maybe_unused]] const struct camera3_device *dev,
>
>  static int hal_dev_flush([[maybe_unused]] const struct camera3_device *dev)
>  {
> -	return 0;
> +	CameraDevice *camera = reinterpret_cast<CameraDevice *>(dev->priv);
> +	return camera->flush();
>  }
>
>  int hal_dev_close(hw_device_t *hw_device)
> --
> 2.31.0.208.g409f899ff0-goog
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel