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

12462 diff mbox series

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@jmondi.org>
---
 src/android/camera_device.cpp | 74 ++++++++++++++++++++++++++++++++++-
 src/android/camera_device.h   |  3 ++
 src/android/camera_ops.cpp    |  8 +++-
 3 files changed, 83 insertions(+), 2 deletions(-)

Hi Jacopo,

Thanks for your work.

On 2021-05-28 00:03:59 +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@jmondi.org>

Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>

> ---
>  src/android/camera_device.cpp | 74 ++++++++++++++++++++++++++++++++++-
>  src/android/camera_device.h   |  3 ++
>  src/android/camera_ops.cpp    |  8 +++-
>  3 files changed, 83 insertions(+), 2 deletions(-)
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index a20c3eaa0ff6..6a8d4d4d5f76 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -797,6 +797,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_);
> @@ -1894,15 +1911,46 @@ int CameraDevice::processControls(Camera3RequestDescriptor *descriptor)
>  	return 0;
>  }
>  
> +void CameraDevice::abortRequest(camera3_capture_request_t *request)
> +{
> +	notifyError(request->frame_number, nullptr, CAMERA3_MSG_ERROR_REQUEST);
> +
> +	camera3_capture_result_t result = {};
> +	result.num_output_buffers = request->num_output_buffers;
> +	result.frame_number = request->frame_number;
> +	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.acquire_fence = -1;
> +		buffer.status = CAMERA3_BUFFER_STATUS_ERROR;
> +	}
> +	result.output_buffers = resultBuffers.data();
> +
> +	callbacks_->process_capture_result(callbacks_, &result);
> +}
> +
>  int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Request)
>  {
>  	if (!isValidRequest(camera3Request))
>  		return -EINVAL;
>  
>  	{
> +		/*
> +		 * Start the camera if that's the first request we handle after
> +		 * a configuration or after a flush.
> +		 *
> +		 * If flush is in progress, return the pending request
> +		 * immediately in error state.
> +		 */
>  		MutexLocker stateLock(stateMutex_);
> +		if (state_ == State::Flushing) {
> +			abortRequest(camera3Request);
> +			return 0;
> +		}
>  
> -		/* Start the camera if that's the first request we handle. */
>  		if (state_ == State::Stopped) {
>  			worker_.start();
>  
> @@ -2004,6 +2052,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_);
> +	if (state_ == State::Flushing) {
> +		abortRequest(camera3Request);
> +		return 0;
> +	}
> +
> +	if (state_ == State::Stopped) {
> +		worker_.start();
> +
> +		ret = camera_->start();
> +		if (ret) {
> +			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;
>  }
>  
> -- 
> 2.31.1
>

Hi Jacopo, thank you for the work.

On Fri, May 28, 2021 at 4:33 PM Niklas Söderlund <
niklas.soderlund@ragnatech.se> wrote:

> Hi Jacopo,
>
> Thanks for your work.
>
> On 2021-05-28 00:03:59 +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@jmondi.org>
>
> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
>
>
Reviewed-by: Hirokazu Honda <hiroh@chromium.org>


> > ---
> >  src/android/camera_device.cpp | 74 ++++++++++++++++++++++++++++++++++-
> >  src/android/camera_device.h   |  3 ++
> >  src/android/camera_ops.cpp    |  8 +++-
> >  3 files changed, 83 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/android/camera_device.cpp
> b/src/android/camera_device.cpp
> > index a20c3eaa0ff6..6a8d4d4d5f76 100644
> > --- a/src/android/camera_device.cpp
> > +++ b/src/android/camera_device.cpp
> > @@ -797,6 +797,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_);
> > @@ -1894,15 +1911,46 @@ int
> CameraDevice::processControls(Camera3RequestDescriptor *descriptor)
> >       return 0;
> >  }
> >
> > +void CameraDevice::abortRequest(camera3_capture_request_t *request)
> > +{
> > +     notifyError(request->frame_number, nullptr,
> CAMERA3_MSG_ERROR_REQUEST);
> > +
> > +     camera3_capture_result_t result = {};
> > +     result.num_output_buffers = request->num_output_buffers;
> > +     result.frame_number = request->frame_number;
> > +     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.acquire_fence = -1;
> > +             buffer.status = CAMERA3_BUFFER_STATUS_ERROR;
> > +     }
> > +     result.output_buffers = resultBuffers.data();
> > +
> > +     callbacks_->process_capture_result(callbacks_, &result);
> > +}
> > +
> >  int CameraDevice::processCaptureRequest(camera3_capture_request_t
> *camera3Request)
> >  {
> >       if (!isValidRequest(camera3Request))
> >               return -EINVAL;
> >
> >       {
> > +             /*
> > +              * Start the camera if that's the first request we handle
> after
> > +              * a configuration or after a flush.
> > +              *
> > +              * If flush is in progress, return the pending request
> > +              * immediately in error state.
> > +              */
> >               MutexLocker stateLock(stateMutex_);
> > +             if (state_ == State::Flushing) {
> > +                     abortRequest(camera3Request);
> > +                     return 0;
> > +             }
> >
> > -             /* Start the camera if that's the first request we handle.
> */
> >               if (state_ == State::Stopped) {
> >                       worker_.start();
> >
> > @@ -2004,6 +2052,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_);
> > +     if (state_ == State::Flushing) {
> > +             abortRequest(camera3Request);
> > +             return 0;
> > +     }
> > +
> > +     if (state_ == State::Stopped) {
> > +             worker_.start();
> > +
> > +             ret = camera_->start();
> > +             if (ret) {
> > +                     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;
> >  }
> >
> > --
> > 2.31.1
> >
>
> --
> Regards,
> Niklas Söderlund
>

Hi Jacopo,

Thank you for the patch.

On Fri, May 28, 2021 at 12:03:59AM +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@jmondi.org>
> ---
>  src/android/camera_device.cpp | 74 ++++++++++++++++++++++++++++++++++-
>  src/android/camera_device.h   |  3 ++
>  src/android/camera_ops.cpp    |  8 +++-
>  3 files changed, 83 insertions(+), 2 deletions(-)
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index a20c3eaa0ff6..6a8d4d4d5f76 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -797,6 +797,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_);
> @@ -1894,15 +1911,46 @@ int CameraDevice::processControls(Camera3RequestDescriptor *descriptor)
>  	return 0;
>  }
>  
> +void CameraDevice::abortRequest(camera3_capture_request_t *request)
> +{
> +	notifyError(request->frame_number, nullptr, CAMERA3_MSG_ERROR_REQUEST);
> +
> +	camera3_capture_result_t result = {};
> +	result.num_output_buffers = request->num_output_buffers;
> +	result.frame_number = request->frame_number;
> +	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.acquire_fence = -1;
> +		buffer.status = CAMERA3_BUFFER_STATUS_ERROR;
> +	}
> +	result.output_buffers = resultBuffers.data();
> +
> +	callbacks_->process_capture_result(callbacks_, &result);
> +}
> +
>  int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Request)
>  {
>  	if (!isValidRequest(camera3Request))
>  		return -EINVAL;
>  
>  	{
> +		/*
> +		 * Start the camera if that's the first request we handle after
> +		 * a configuration or after a flush.
> +		 *
> +		 * If flush is in progress, return the pending request
> +		 * immediately in error state.
> +		 */
>  		MutexLocker stateLock(stateMutex_);
> +		if (state_ == State::Flushing) {
> +			abortRequest(camera3Request);
> +			return 0;
> +		}
>  
> -		/* Start the camera if that's the first request we handle. */
>  		if (state_ == State::Stopped) {
>  			worker_.start();
>  
> @@ -2004,6 +2052,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_);
> +	if (state_ == State::Flushing) {
> +		abortRequest(camera3Request);
> +		return 0;
> +	}

It seems possibly overkill to do this check twice, but it shouldn't
hurt. I suspect we'll rework this code later, possibly by adding a
Camera::flush() in the libcamera native API, although I'm not entirely
sure what benefit it would bring compared to a stop/start. For now this
should be fine.

> +
> +	if (state_ == State::Stopped) {
> +		worker_.start();
> +
> +		ret = camera_->start();
> +		if (ret) {
> +			LOG(HAL, Error) << "Failed to start camera";
> +			return ret;
> +		}
> +
> +		state_ = State::Running;
> +	}

This, however, bothers me a bit. Why do we need to start the camera in
two different locations ? Could we drop the first start above ? And if
we do so, given that preparing the request should be a short operation,
I wonder if we shouldn't also drop the first Flushing check at the top
of this function.

The rest of the patch looks good to me.

> +
>  	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;
>  }
>

Hi Jacopo,

On Fri, May 28, 2021 at 10:53:28AM +0300, Laurent Pinchart wrote:
> On Fri, May 28, 2021 at 12:03:59AM +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@jmondi.org>
> > ---
> >  src/android/camera_device.cpp | 74 ++++++++++++++++++++++++++++++++++-
> >  src/android/camera_device.h   |  3 ++
> >  src/android/camera_ops.cpp    |  8 +++-
> >  3 files changed, 83 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > index a20c3eaa0ff6..6a8d4d4d5f76 100644
> > --- a/src/android/camera_device.cpp
> > +++ b/src/android/camera_device.cpp
> > @@ -797,6 +797,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_);
> > @@ -1894,15 +1911,46 @@ int CameraDevice::processControls(Camera3RequestDescriptor *descriptor)
> >  	return 0;
> >  }
> >  
> > +void CameraDevice::abortRequest(camera3_capture_request_t *request)
> > +{
> > +	notifyError(request->frame_number, nullptr, CAMERA3_MSG_ERROR_REQUEST);
> > +
> > +	camera3_capture_result_t result = {};
> > +	result.num_output_buffers = request->num_output_buffers;
> > +	result.frame_number = request->frame_number;
> > +	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.acquire_fence = -1;
> > +		buffer.status = CAMERA3_BUFFER_STATUS_ERROR;
> > +	}
> > +	result.output_buffers = resultBuffers.data();
> > +
> > +	callbacks_->process_capture_result(callbacks_, &result);
> > +}
> > +
> >  int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Request)
> >  {
> >  	if (!isValidRequest(camera3Request))
> >  		return -EINVAL;
> >  
> >  	{
> > +		/*
> > +		 * Start the camera if that's the first request we handle after
> > +		 * a configuration or after a flush.
> > +		 *
> > +		 * If flush is in progress, return the pending request
> > +		 * immediately in error state.
> > +		 */
> >  		MutexLocker stateLock(stateMutex_);
> > +		if (state_ == State::Flushing) {
> > +			abortRequest(camera3Request);
> > +			return 0;
> > +		}
> >  
> > -		/* Start the camera if that's the first request we handle. */
> >  		if (state_ == State::Stopped) {
> >  			worker_.start();
> >  
> > @@ -2004,6 +2052,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_);
> > +	if (state_ == State::Flushing) {
> > +		abortRequest(camera3Request);
> > +		return 0;
> > +	}
> 
> It seems possibly overkill to do this check twice, but it shouldn't
> hurt. I suspect we'll rework this code later, possibly by adding a
> Camera::flush() in the libcamera native API, although I'm not entirely
> sure what benefit it would bring compared to a stop/start. For now this
> should be fine.
> 
> > +
> > +	if (state_ == State::Stopped) {
> > +		worker_.start();
> > +
> > +		ret = camera_->start();
> > +		if (ret) {
> > +			LOG(HAL, Error) << "Failed to start camera";
> > +			return ret;
> > +		}
> > +
> > +		state_ = State::Running;
> > +	}
> 
> This, however, bothers me a bit. Why do we need to start the camera in
> two different locations ? Could we drop the first start above ? And if
> we do so, given that preparing the request should be a short operation,
> I wonder if we shouldn't also drop the first Flushing check at the top
> of this function.

This shouldn't be a blocker though, so I'll merge the series after
running tests. We can address the issue on top.

> The rest of the patch looks good to me.
> 
> > +
> >  	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;
> >  }
> >

Hi Jacopo,

On Mon, May 31, 2021 at 03:59:53AM +0300, Laurent Pinchart wrote:
> On Fri, May 28, 2021 at 10:53:28AM +0300, Laurent Pinchart wrote:
> > On Fri, May 28, 2021 at 12:03:59AM +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@jmondi.org>
> > > ---
> > >  src/android/camera_device.cpp | 74 ++++++++++++++++++++++++++++++++++-
> > >  src/android/camera_device.h   |  3 ++
> > >  src/android/camera_ops.cpp    |  8 +++-
> > >  3 files changed, 83 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > > index a20c3eaa0ff6..6a8d4d4d5f76 100644
> > > --- a/src/android/camera_device.cpp
> > > +++ b/src/android/camera_device.cpp
> > > @@ -797,6 +797,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_);
> > > @@ -1894,15 +1911,46 @@ int CameraDevice::processControls(Camera3RequestDescriptor *descriptor)
> > >  	return 0;
> > >  }
> > >  
> > > +void CameraDevice::abortRequest(camera3_capture_request_t *request)
> > > +{
> > > +	notifyError(request->frame_number, nullptr, CAMERA3_MSG_ERROR_REQUEST);
> > > +
> > > +	camera3_capture_result_t result = {};
> > > +	result.num_output_buffers = request->num_output_buffers;
> > > +	result.frame_number = request->frame_number;
> > > +	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.acquire_fence = -1;
> > > +		buffer.status = CAMERA3_BUFFER_STATUS_ERROR;
> > > +	}
> > > +	result.output_buffers = resultBuffers.data();
> > > +
> > > +	callbacks_->process_capture_result(callbacks_, &result);
> > > +}
> > > +
> > >  int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Request)
> > >  {
> > >  	if (!isValidRequest(camera3Request))
> > >  		return -EINVAL;
> > >  
> > >  	{
> > > +		/*
> > > +		 * Start the camera if that's the first request we handle after
> > > +		 * a configuration or after a flush.
> > > +		 *
> > > +		 * If flush is in progress, return the pending request
> > > +		 * immediately in error state.
> > > +		 */
> > >  		MutexLocker stateLock(stateMutex_);
> > > +		if (state_ == State::Flushing) {
> > > +			abortRequest(camera3Request);
> > > +			return 0;
> > > +		}
> > >  
> > > -		/* Start the camera if that's the first request we handle. */
> > >  		if (state_ == State::Stopped) {
> > >  			worker_.start();
> > >  
> > > @@ -2004,6 +2052,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_);
> > > +	if (state_ == State::Flushing) {
> > > +		abortRequest(camera3Request);
> > > +		return 0;
> > > +	}
> > 
> > It seems possibly overkill to do this check twice, but it shouldn't
> > hurt. I suspect we'll rework this code later, possibly by adding a
> > Camera::flush() in the libcamera native API, although I'm not entirely
> > sure what benefit it would bring compared to a stop/start. For now this
> > should be fine.
> > 
> > > +
> > > +	if (state_ == State::Stopped) {
> > > +		worker_.start();
> > > +
> > > +		ret = camera_->start();
> > > +		if (ret) {
> > > +			LOG(HAL, Error) << "Failed to start camera";
> > > +			return ret;
> > > +		}
> > > +
> > > +		state_ = State::Running;
> > > +	}
> > 
> > This, however, bothers me a bit. Why do we need to start the camera in
> > two different locations ? Could we drop the first start above ? And if
> > we do so, given that preparing the request should be a short operation,
> > I wonder if we shouldn't also drop the first Flushing check at the top
> > of this function.
> 
> This shouldn't be a blocker though, so I'll merge the series after
> running tests. We can address the issue on top.

I'm afraid this series causes CTS regressions :-(

I'm running the full camera tests with

run cts-dev --skip-preconditions -a x86_64 -m CtsCameraTestCases

With the current master branch, I get 22 or 23 failures (some are a bit
random), and with this series, it increases to 25. Here's the diff:

@@ -3,14 +3,16 @@
 android.hardware.camera2.cts.ImageReaderTest#testRawPrivate
 android.hardware.camera2.cts.ImageReaderTest#testRepeatingRawPrivate
 android.hardware.camera2.cts.RecordingTest#testSupportedVideoSizes
-android.hardware.camera2.cts.RecordingTest#testVideoSnapshot
 android.hardware.camera2.cts.RobustnessTest#testMandatoryOutputCombinations
+android.hardware.camera2.cts.StillCaptureTest#testFocalLengths
+android.hardware.camera2.cts.StillCaptureTest#testJpegExif
 android.hardware.camera2.cts.StillCaptureTest#testStillPreviewCombination
 android.hardware.camera2.cts.SurfaceViewPreviewTest#testDeferredSurfaces
 android.hardware.cts.CameraGLTest#testSetPreviewTextureBothCallbacks
 android.hardware.cts.CameraGLTest#testSetPreviewTexturePreviewCallback
 android.hardware.cts.CameraTest#testFocusDistances
 android.hardware.cts.CameraTest#testImmediateZoom
+android.hardware.cts.CameraTest#testJpegExif
 android.hardware.cts.CameraTest#testPreviewCallback
 android.hardware.cts.CameraTest#testPreviewCallbackWithBuffer
 android.hardware.cts.CameraTest#testPreviewCallbackWithPicture

There's some randomness in the RecordingTest, so that may not be
significant. The other three tests seem to pass consistently in master,
and fail consistently with the series applied. They also fail when run
in isolation.

> > The rest of the patch looks good to me.
> > 
> > > +
> > >  	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;
> > >  }
> > >

Hi Jacopo,

On Wed, Jun 02, 2021 at 03:58:47AM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
> 
> On Mon, May 31, 2021 at 03:59:53AM +0300, Laurent Pinchart wrote:
> > On Fri, May 28, 2021 at 10:53:28AM +0300, Laurent Pinchart wrote:
> > > On Fri, May 28, 2021 at 12:03:59AM +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@jmondi.org>
> > > > ---
> > > >  src/android/camera_device.cpp | 74 ++++++++++++++++++++++++++++++++++-
> > > >  src/android/camera_device.h   |  3 ++
> > > >  src/android/camera_ops.cpp    |  8 +++-
> > > >  3 files changed, 83 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > > > index a20c3eaa0ff6..6a8d4d4d5f76 100644
> > > > --- a/src/android/camera_device.cpp
> > > > +++ b/src/android/camera_device.cpp
> > > > @@ -797,6 +797,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_);
> > > > @@ -1894,15 +1911,46 @@ int CameraDevice::processControls(Camera3RequestDescriptor *descriptor)
> > > >  	return 0;
> > > >  }
> > > >  
> > > > +void CameraDevice::abortRequest(camera3_capture_request_t *request)
> > > > +{
> > > > +	notifyError(request->frame_number, nullptr, CAMERA3_MSG_ERROR_REQUEST);
> > > > +
> > > > +	camera3_capture_result_t result = {};
> > > > +	result.num_output_buffers = request->num_output_buffers;
> > > > +	result.frame_number = request->frame_number;
> > > > +	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.acquire_fence = -1;
> > > > +		buffer.status = CAMERA3_BUFFER_STATUS_ERROR;
> > > > +	}
> > > > +	result.output_buffers = resultBuffers.data();
> > > > +
> > > > +	callbacks_->process_capture_result(callbacks_, &result);
> > > > +}
> > > > +
> > > >  int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Request)
> > > >  {
> > > >  	if (!isValidRequest(camera3Request))
> > > >  		return -EINVAL;
> > > >  
> > > >  	{
> > > > +		/*
> > > > +		 * Start the camera if that's the first request we handle after
> > > > +		 * a configuration or after a flush.
> > > > +		 *
> > > > +		 * If flush is in progress, return the pending request
> > > > +		 * immediately in error state.
> > > > +		 */
> > > >  		MutexLocker stateLock(stateMutex_);
> > > > +		if (state_ == State::Flushing) {
> > > > +			abortRequest(camera3Request);
> > > > +			return 0;
> > > > +		}
> > > >  
> > > > -		/* Start the camera if that's the first request we handle. */
> > > >  		if (state_ == State::Stopped) {
> > > >  			worker_.start();
> > > >  
> > > > @@ -2004,6 +2052,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_);
> > > > +	if (state_ == State::Flushing) {
> > > > +		abortRequest(camera3Request);
> > > > +		return 0;
> > > > +	}
> > > 
> > > It seems possibly overkill to do this check twice, but it shouldn't
> > > hurt. I suspect we'll rework this code later, possibly by adding a
> > > Camera::flush() in the libcamera native API, although I'm not entirely
> > > sure what benefit it would bring compared to a stop/start. For now this
> > > should be fine.
> > > 
> > > > +
> > > > +	if (state_ == State::Stopped) {
> > > > +		worker_.start();
> > > > +
> > > > +		ret = camera_->start();
> > > > +		if (ret) {
> > > > +			LOG(HAL, Error) << "Failed to start camera";
> > > > +			return ret;
> > > > +		}
> > > > +
> > > > +		state_ = State::Running;
> > > > +	}
> > > 
> > > This, however, bothers me a bit. Why do we need to start the camera in
> > > two different locations ? Could we drop the first start above ? And if
> > > we do so, given that preparing the request should be a short operation,
> > > I wonder if we shouldn't also drop the first Flushing check at the top
> > > of this function.
> > 
> > This shouldn't be a blocker though, so I'll merge the series after
> > running tests. We can address the issue on top.
> 
> I'm afraid this series causes CTS regressions :-(
> 
> I'm running the full camera tests with
> 
> run cts-dev --skip-preconditions -a x86_64 -m CtsCameraTestCases
> 
> With the current master branch, I get 22 or 23 failures (some are a bit
> random), and with this series, it increases to 25. Here's the diff:
> 
> @@ -3,14 +3,16 @@
>  android.hardware.camera2.cts.ImageReaderTest#testRawPrivate
>  android.hardware.camera2.cts.ImageReaderTest#testRepeatingRawPrivate
>  android.hardware.camera2.cts.RecordingTest#testSupportedVideoSizes
> -android.hardware.camera2.cts.RecordingTest#testVideoSnapshot
>  android.hardware.camera2.cts.RobustnessTest#testMandatoryOutputCombinations
> +android.hardware.camera2.cts.StillCaptureTest#testFocalLengths
> +android.hardware.camera2.cts.StillCaptureTest#testJpegExif
>  android.hardware.camera2.cts.StillCaptureTest#testStillPreviewCombination
>  android.hardware.camera2.cts.SurfaceViewPreviewTest#testDeferredSurfaces
>  android.hardware.cts.CameraGLTest#testSetPreviewTextureBothCallbacks
>  android.hardware.cts.CameraGLTest#testSetPreviewTexturePreviewCallback
>  android.hardware.cts.CameraTest#testFocusDistances
>  android.hardware.cts.CameraTest#testImmediateZoom
> +android.hardware.cts.CameraTest#testJpegExif
>  android.hardware.cts.CameraTest#testPreviewCallback
>  android.hardware.cts.CameraTest#testPreviewCallbackWithBuffer
>  android.hardware.cts.CameraTest#testPreviewCallbackWithPicture

With the subplan, all of the existing ones are excluded. The
RecordingTests are failing at random (I'll file a bug report for this).

I've managed to reproduce the three extra failures though, and they
consistently fail with a segfault.


Paul

> 
> There's some randomness in the RecordingTest, so that may not be
> significant. The other three tests seem to pass consistently in master,
> and fail consistently with the series applied. They also fail when run
> in isolation.
> 
> > > The rest of the patch looks good to me.
> > > 
> > > > +
> > > >  	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

Hi Paul and Laurent,

On Wed, Jun 2, 2021 at 1:38 PM <paul.elder@ideasonboard.com> wrote:

> Hi Jacopo,
>
> On Wed, Jun 02, 2021 at 03:58:47AM +0300, Laurent Pinchart wrote:
> > Hi Jacopo,
> >
> > On Mon, May 31, 2021 at 03:59:53AM +0300, Laurent Pinchart wrote:
> > > On Fri, May 28, 2021 at 10:53:28AM +0300, Laurent Pinchart wrote:
> > > > On Fri, May 28, 2021 at 12:03:59AM +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@jmondi.org>
> > > > > ---
> > > > >  src/android/camera_device.cpp | 74
> ++++++++++++++++++++++++++++++++++-
> > > > >  src/android/camera_device.h   |  3 ++
> > > > >  src/android/camera_ops.cpp    |  8 +++-
> > > > >  3 files changed, 83 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/src/android/camera_device.cpp
> b/src/android/camera_device.cpp
> > > > > index a20c3eaa0ff6..6a8d4d4d5f76 100644
> > > > > --- a/src/android/camera_device.cpp
> > > > > +++ b/src/android/camera_device.cpp
> > > > > @@ -797,6 +797,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_);
> > > > > @@ -1894,15 +1911,46 @@ int
> CameraDevice::processControls(Camera3RequestDescriptor *descriptor)
> > > > >         return 0;
> > > > >  }
> > > > >
> > > > > +void CameraDevice::abortRequest(camera3_capture_request_t
> *request)
> > > > > +{
> > > > > +       notifyError(request->frame_number, nullptr,
> CAMERA3_MSG_ERROR_REQUEST);
> > > > > +
> > > > > +       camera3_capture_result_t result = {};
> > > > > +       result.num_output_buffers = request->num_output_buffers;
> > > > > +       result.frame_number = request->frame_number;
> > > > > +       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.acquire_fence = -1;
> > > > > +               buffer.status = CAMERA3_BUFFER_STATUS_ERROR;
> > > > > +       }
> > > > > +       result.output_buffers = resultBuffers.data();
> > > > > +
> > > > > +       callbacks_->process_capture_result(callbacks_, &result);
> > > > > +}
> > > > > +
> > > > >  int CameraDevice::processCaptureRequest(camera3_capture_request_t
> *camera3Request)
> > > > >  {
> > > > >         if (!isValidRequest(camera3Request))
> > > > >                 return -EINVAL;
> > > > >
> > > > >         {
> > > > > +               /*
> > > > > +                * Start the camera if that's the first request we
> handle after
> > > > > +                * a configuration or after a flush.
> > > > > +                *
> > > > > +                * If flush is in progress, return the pending
> request
> > > > > +                * immediately in error state.
> > > > > +                */
> > > > >                 MutexLocker stateLock(stateMutex_);
> > > > > +               if (state_ == State::Flushing) {
> > > > > +                       abortRequest(camera3Request);
> > > > > +                       return 0;
> > > > > +               }
> > > > >
> > > > > -               /* Start the camera if that's the first request we
> handle. */
> > > > >                 if (state_ == State::Stopped) {
> > > > >                         worker_.start();
> > > > >
> > > > > @@ -2004,6 +2052,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_);
> > > > > +       if (state_ == State::Flushing) {
> > > > > +               abortRequest(camera3Request);
> > > > > +               return 0;
> > > > > +       }
> > > >
> > > > It seems possibly overkill to do this check twice, but it shouldn't
> > > > hurt. I suspect we'll rework this code later, possibly by adding a
> > > > Camera::flush() in the libcamera native API, although I'm not
> entirely
> > > > sure what benefit it would bring compared to a stop/start. For now
> this
> > > > should be fine.
> > > >
> > > > > +
> > > > > +       if (state_ == State::Stopped) {
> > > > > +               worker_.start();
> > > > > +
> > > > > +               ret = camera_->start();
> > > > > +               if (ret) {
> > > > > +                       LOG(HAL, Error) << "Failed to start
> camera";
> > > > > +                       return ret;
> > > > > +               }
> > > > > +
> > > > > +               state_ = State::Running;
> > > > > +       }
> > > >
> > > > This, however, bothers me a bit. Why do we need to start the camera
> in
> > > > two different locations ? Could we drop the first start above ? And
> if
> > > > we do so, given that preparing the request should be a short
> operation,
> > > > I wonder if we shouldn't also drop the first Flushing check at the
> top
> > > > of this function.
> > >
> > > This shouldn't be a blocker though, so I'll merge the series after
> > > running tests. We can address the issue on top.
> >
> > I'm afraid this series causes CTS regressions :-(
> >
> > I'm running the full camera tests with
> >
> > run cts-dev --skip-preconditions -a x86_64 -m CtsCameraTestCases
> >
> > With the current master branch, I get 22 or 23 failures (some are a bit
> > random), and with this series, it increases to 25. Here's the diff:
> >
> > @@ -3,14 +3,16 @@
> >  android.hardware.camera2.cts.ImageReaderTest#testRawPrivate
> >  android.hardware.camera2.cts.ImageReaderTest#testRepeatingRawPrivate
> >  android.hardware.camera2.cts.RecordingTest#testSupportedVideoSizes
> > -android.hardware.camera2.cts.RecordingTest#testVideoSnapshot
> >
> android.hardware.camera2.cts.RobustnessTest#testMandatoryOutputCombinations
> > +android.hardware.camera2.cts.StillCaptureTest#testFocalLengths
> > +android.hardware.camera2.cts.StillCaptureTest#testJpegExif
> >
> android.hardware.camera2.cts.StillCaptureTest#testStillPreviewCombination
> >  android.hardware.camera2.cts.SurfaceViewPreviewTest#testDeferredSurfaces
> >  android.hardware.cts.CameraGLTest#testSetPreviewTextureBothCallbacks
> >  android.hardware.cts.CameraGLTest#testSetPreviewTexturePreviewCallback
> >  android.hardware.cts.CameraTest#testFocusDistances
> >  android.hardware.cts.CameraTest#testImmediateZoom
> > +android.hardware.cts.CameraTest#testJpegExif
> >  android.hardware.cts.CameraTest#testPreviewCallback
> >  android.hardware.cts.CameraTest#testPreviewCallbackWithBuffer
> >  android.hardware.cts.CameraTest#testPreviewCallbackWithPicture
>
> With the subplan, all of the existing ones are excluded. The
> RecordingTests are failing at random (I'll file a bug report for this).
>
> I've managed to reproduce the three extra failures though, and they
> consistently fail with a segfault.
>
>
I ran android.hardware.camera2.cts.StillCaptureTest and the failure tests
pass.
Could you tell me on top of what commit you applied the patch series?

-Hiro


>
> Paul
>
> >
> > There's some randomness in the RecordingTest, so that may not be
> > significant. The other three tests seem to pass consistently in master,
> > and fail consistently with the series applied. They also fail when run
> > in isolation.
> >
> > > > The rest of the patch looks good to me.
> > > >
> > > > > +
> > > > >         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
>

Hi Hiro,

On Fri, Jun 04, 2021 at 05:48:45PM +0900, Hirokazu Honda wrote:
> Hi Paul and Laurent,
> 
> On Wed, Jun 2, 2021 at 1:38 PM <paul.elder@ideasonboard.com> wrote:
> 
>     Hi Jacopo,
> 
>     On Wed, Jun 02, 2021 at 03:58:47AM +0300, Laurent Pinchart wrote:
>     > Hi Jacopo,
>     >
>     > On Mon, May 31, 2021 at 03:59:53AM +0300, Laurent Pinchart wrote:
>     > > On Fri, May 28, 2021 at 10:53:28AM +0300, Laurent Pinchart wrote:
>     > > > On Fri, May 28, 2021 at 12:03:59AM +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@jmondi.org>
>     > > > > ---
>     > > > >  src/android/camera_device.cpp | 74
>     ++++++++++++++++++++++++++++++++++-
>     > > > >  src/android/camera_device.h   |  3 ++
>     > > > >  src/android/camera_ops.cpp    |  8 +++-
>     > > > >  3 files changed, 83 insertions(+), 2 deletions(-)
>     > > > >
>     > > > > diff --git a/src/android/camera_device.cpp b/src/android/
>     camera_device.cpp
>     > > > > index a20c3eaa0ff6..6a8d4d4d5f76 100644
>     > > > > --- a/src/android/camera_device.cpp
>     > > > > +++ b/src/android/camera_device.cpp
>     > > > > @@ -797,6 +797,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_);
>     > > > > @@ -1894,15 +1911,46 @@ int CameraDevice::processControls
>     (Camera3RequestDescriptor *descriptor)
>     > > > >         return 0;
>     > > > >  }
>     > > > > 
>     > > > > +void CameraDevice::abortRequest(camera3_capture_request_t
>     *request)
>     > > > > +{
>     > > > > +       notifyError(request->frame_number, nullptr,
>     CAMERA3_MSG_ERROR_REQUEST);
>     > > > > +
>     > > > > +       camera3_capture_result_t result = {};
>     > > > > +       result.num_output_buffers = request->num_output_buffers;
>     > > > > +       result.frame_number = request->frame_number;
>     > > > > +       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.acquire_fence = -1;
>     > > > > +               buffer.status = CAMERA3_BUFFER_STATUS_ERROR;
>     > > > > +       }
>     > > > > +       result.output_buffers = resultBuffers.data();
>     > > > > +
>     > > > > +       callbacks_->process_capture_result(callbacks_, &result);
>     > > > > +}
>     > > > > +
>     > > > >  int CameraDevice::processCaptureRequest(camera3_capture_request_t
>     *camera3Request)
>     > > > >  {
>     > > > >         if (!isValidRequest(camera3Request))
>     > > > >                 return -EINVAL;
>     > > > > 
>     > > > >         {
>     > > > > +               /*
>     > > > > +                * Start the camera if that's the first request we
>     handle after
>     > > > > +                * a configuration or after a flush.
>     > > > > +                *
>     > > > > +                * If flush is in progress, return the pending
>     request
>     > > > > +                * immediately in error state.
>     > > > > +                */
>     > > > >                 MutexLocker stateLock(stateMutex_);
>     > > > > +               if (state_ == State::Flushing) {
>     > > > > +                       abortRequest(camera3Request);
>     > > > > +                       return 0;
>     > > > > +               }
>     > > > > 
>     > > > > -               /* Start the camera if that's the first request we
>     handle. */
>     > > > >                 if (state_ == State::Stopped) {
>     > > > >                         worker_.start();
>     > > > > 
>     > > > > @@ -2004,6 +2052,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_);
>     > > > > +       if (state_ == State::Flushing) {
>     > > > > +               abortRequest(camera3Request);
>     > > > > +               return 0;
>     > > > > +       }
>     > > >
>     > > > It seems possibly overkill to do this check twice, but it shouldn't
>     > > > hurt. I suspect we'll rework this code later, possibly by adding a
>     > > > Camera::flush() in the libcamera native API, although I'm not
>     entirely
>     > > > sure what benefit it would bring compared to a stop/start. For now
>     this
>     > > > should be fine.
>     > > >
>     > > > > +
>     > > > > +       if (state_ == State::Stopped) {
>     > > > > +               worker_.start();
>     > > > > +
>     > > > > +               ret = camera_->start();
>     > > > > +               if (ret) {
>     > > > > +                       LOG(HAL, Error) << "Failed to start
>     camera";
>     > > > > +                       return ret;
>     > > > > +               }
>     > > > > +
>     > > > > +               state_ = State::Running;
>     > > > > +       }
>     > > >
>     > > > This, however, bothers me a bit. Why do we need to start the camera
>     in
>     > > > two different locations ? Could we drop the first start above ? And
>     if
>     > > > we do so, given that preparing the request should be a short
>     operation,
>     > > > I wonder if we shouldn't also drop the first Flushing check at the
>     top
>     > > > of this function.
>     > >
>     > > This shouldn't be a blocker though, so I'll merge the series after
>     > > running tests. We can address the issue on top.
>     >
>     > I'm afraid this series causes CTS regressions :-(
>     >
>     > I'm running the full camera tests with
>     >
>     > run cts-dev --skip-preconditions -a x86_64 -m CtsCameraTestCases
>     >
>     > With the current master branch, I get 22 or 23 failures (some are a bit
>     > random), and with this series, it increases to 25. Here's the diff:
>     >
>     > @@ -3,14 +3,16 @@
>     >  android.hardware.camera2.cts.ImageReaderTest#testRawPrivate
>     >  android.hardware.camera2.cts.ImageReaderTest#testRepeatingRawPrivate
>     >  android.hardware.camera2.cts.RecordingTest#testSupportedVideoSizes
>     > -android.hardware.camera2.cts.RecordingTest#testVideoSnapshot
>     >  android.hardware.camera2.cts.RobustnessTest#
>     testMandatoryOutputCombinations
>     > +android.hardware.camera2.cts.StillCaptureTest#testFocalLengths
>     > +android.hardware.camera2.cts.StillCaptureTest#testJpegExif
>     >  android.hardware.camera2.cts.StillCaptureTest#
>     testStillPreviewCombination
>     >  android.hardware.camera2.cts.SurfaceViewPreviewTest#testDeferredSurfaces
>     >  android.hardware.cts.CameraGLTest#testSetPreviewTextureBothCallbacks
>     >  android.hardware.cts.CameraGLTest#testSetPreviewTexturePreviewCallback
>     >  android.hardware.cts.CameraTest#testFocusDistances
>     >  android.hardware.cts.CameraTest#testImmediateZoom
>     > +android.hardware.cts.CameraTest#testJpegExif
>     >  android.hardware.cts.CameraTest#testPreviewCallback
>     >  android.hardware.cts.CameraTest#testPreviewCallbackWithBuffer
>     >  android.hardware.cts.CameraTest#testPreviewCallbackWithPicture
> 
>     With the subplan, all of the existing ones are excluded. The
>     RecordingTests are failing at random (I'll file a bug report for this).
> 
>     I've managed to reproduce the three extra failures though, and they
>     consistently fail with a segfault.
> 
> 
> 
> I ran android.hardware.camera2.cts.StillCaptureTest and the failure tests pass.
> Could you tell me on top of what commit you applied the patch series?

On... master. I think 59de56f4 "qcam: Add libatomic dependency".


Paul

>  
> 
> 
>     >
>     > There's some randomness in the RecordingTest, so that may not be
>     > significant. The other three tests seem to pass consistently in master,
>     > and fail consistently with the series applied. They also fail when run
>     > in isolation.
>     >
>     > > > The rest of the patch looks good to me.
>     > > >
>     > > > > +
>     > > > >         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
>

Hi Paul,



On Fri, Jun 4, 2021 at 6:27 PM <paul.elder@ideasonboard.com> wrote:

> Hi Hiro,
>
> On Fri, Jun 04, 2021 at 05:48:45PM +0900, Hirokazu Honda wrote:
> > Hi Paul and Laurent,
> >
> > On Wed, Jun 2, 2021 at 1:38 PM <paul.elder@ideasonboard.com> wrote:
> >
> >     Hi Jacopo,
> >
> >     On Wed, Jun 02, 2021 at 03:58:47AM +0300, Laurent Pinchart wrote:
> >     > Hi Jacopo,
> >     >
> >     > On Mon, May 31, 2021 at 03:59:53AM +0300, Laurent Pinchart wrote:
> >     > > On Fri, May 28, 2021 at 10:53:28AM +0300, Laurent Pinchart wrote:
> >     > > > On Fri, May 28, 2021 at 12:03:59AM +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@jmondi.org>
> >     > > > > ---
> >     > > > >  src/android/camera_device.cpp | 74
> >     ++++++++++++++++++++++++++++++++++-
> >     > > > >  src/android/camera_device.h   |  3 ++
> >     > > > >  src/android/camera_ops.cpp    |  8 +++-
> >     > > > >  3 files changed, 83 insertions(+), 2 deletions(-)
> >     > > > >
> >     > > > > diff --git a/src/android/camera_device.cpp b/src/android/
> >     camera_device.cpp
> >     > > > > index a20c3eaa0ff6..6a8d4d4d5f76 100644
> >     > > > > --- a/src/android/camera_device.cpp
> >     > > > > +++ b/src/android/camera_device.cpp
> >     > > > > @@ -797,6 +797,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_);
> >     > > > > @@ -1894,15 +1911,46 @@ int CameraDevice::processControls
> >     (Camera3RequestDescriptor *descriptor)
> >     > > > >         return 0;
> >     > > > >  }
> >     > > > >
> >     > > > > +void CameraDevice::abortRequest(camera3_capture_request_t
> >     *request)
> >     > > > > +{
> >     > > > > +       notifyError(request->frame_number, nullptr,
> >     CAMERA3_MSG_ERROR_REQUEST);
> >     > > > > +
> >     > > > > +       camera3_capture_result_t result = {};
> >     > > > > +       result.num_output_buffers =
> request->num_output_buffers;
> >     > > > > +       result.frame_number = request->frame_number;
> >     > > > > +       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.acquire_fence = -1;
> >     > > > > +               buffer.status = CAMERA3_BUFFER_STATUS_ERROR;
> >     > > > > +       }
> >     > > > > +       result.output_buffers = resultBuffers.data();
> >     > > > > +
> >     > > > > +       callbacks_->process_capture_result(callbacks_,
> &result);
> >     > > > > +}
> >     > > > > +
> >     > > > >  int
> CameraDevice::processCaptureRequest(camera3_capture_request_t
> >     *camera3Request)
> >     > > > >  {
> >     > > > >         if (!isValidRequest(camera3Request))
> >     > > > >                 return -EINVAL;
> >     > > > >
> >     > > > >         {
> >     > > > > +               /*
> >     > > > > +                * Start the camera if that's the first
> request we
> >     handle after
> >     > > > > +                * a configuration or after a flush.
> >     > > > > +                *
> >     > > > > +                * If flush is in progress, return the
> pending
> >     request
> >     > > > > +                * immediately in error state.
> >     > > > > +                */
> >     > > > >                 MutexLocker stateLock(stateMutex_);
> >     > > > > +               if (state_ == State::Flushing) {
> >     > > > > +                       abortRequest(camera3Request);
> >     > > > > +                       return 0;
> >     > > > > +               }
> >     > > > >
> >     > > > > -               /* Start the camera if that's the first
> request we
> >     handle. */
> >     > > > >                 if (state_ == State::Stopped) {
> >     > > > >                         worker_.start();
> >     > > > >
> >     > > > > @@ -2004,6 +2052,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_);
> >     > > > > +       if (state_ == State::Flushing) {
> >     > > > > +               abortRequest(camera3Request);
> >     > > > > +               return 0;
> >     > > > > +       }
> >     > > >
> >     > > > It seems possibly overkill to do this check twice, but it
> shouldn't
> >     > > > hurt. I suspect we'll rework this code later, possibly by
> adding a
> >     > > > Camera::flush() in the libcamera native API, although I'm not
> >     entirely
> >     > > > sure what benefit it would bring compared to a stop/start. For
> now
> >     this
> >     > > > should be fine.
> >     > > >
> >     > > > > +
> >     > > > > +       if (state_ == State::Stopped) {
> >     > > > > +               worker_.start();
> >     > > > > +
> >     > > > > +               ret = camera_->start();
> >     > > > > +               if (ret) {
> >     > > > > +                       LOG(HAL, Error) << "Failed to start
> >     camera";
> >     > > > > +                       return ret;
> >     > > > > +               }
> >     > > > > +
> >     > > > > +               state_ = State::Running;
> >     > > > > +       }
> >     > > >
> >     > > > This, however, bothers me a bit. Why do we need to start the
> camera
> >     in
> >     > > > two different locations ? Could we drop the first start above
> ? And
> >     if
> >     > > > we do so, given that preparing the request should be a short
> >     operation,
> >     > > > I wonder if we shouldn't also drop the first Flushing check at
> the
> >     top
> >     > > > of this function.
> >     > >
> >     > > This shouldn't be a blocker though, so I'll merge the series
> after
> >     > > running tests. We can address the issue on top.
> >     >
> >     > I'm afraid this series causes CTS regressions :-(
> >     >
> >     > I'm running the full camera tests with
> >     >
> >     > run cts-dev --skip-preconditions -a x86_64 -m CtsCameraTestCases
> >     >
> >     > With the current master branch, I get 22 or 23 failures (some are
> a bit
> >     > random), and with this series, it increases to 25. Here's the diff:
> >     >
> >     > @@ -3,14 +3,16 @@
> >     >  android.hardware.camera2.cts.ImageReaderTest#testRawPrivate
> >     >
> android.hardware.camera2.cts.ImageReaderTest#testRepeatingRawPrivate
> >     >  android.hardware.camera2.cts.RecordingTest#testSupportedVideoSizes
> >     > -android.hardware.camera2.cts.RecordingTest#testVideoSnapshot
> >     >  android.hardware.camera2.cts.RobustnessTest#
> >     testMandatoryOutputCombinations
> >     > +android.hardware.camera2.cts.StillCaptureTest#testFocalLengths
> >     > +android.hardware.camera2.cts.StillCaptureTest#testJpegExif
> >     >  android.hardware.camera2.cts.StillCaptureTest#
> >     testStillPreviewCombination
> >     >
> android.hardware.camera2.cts.SurfaceViewPreviewTest#testDeferredSurfaces
> >     >
> android.hardware.cts.CameraGLTest#testSetPreviewTextureBothCallbacks
> >     >
> android.hardware.cts.CameraGLTest#testSetPreviewTexturePreviewCallback
> >     >  android.hardware.cts.CameraTest#testFocusDistances
> >     >  android.hardware.cts.CameraTest#testImmediateZoom
> >     > +android.hardware.cts.CameraTest#testJpegExif
> >     >  android.hardware.cts.CameraTest#testPreviewCallback
> >     >  android.hardware.cts.CameraTest#testPreviewCallbackWithBuffer
> >     >  android.hardware.cts.CameraTest#testPreviewCallbackWithPicture
> >
> >     With the subplan, all of the existing ones are excluded. The
> >     RecordingTests are failing at random (I'll file a bug report for
> this).
> >
> >     I've managed to reproduce the three extra failures though, and they
> >     consistently fail with a segfault.
> >
> >
> >
> > I ran android.hardware.camera2.cts.StillCaptureTest and the failure
> tests pass.
> > Could you tell me on top of what commit you applied the patch series?
>
> On... master. I think 59de56f4 "qcam: Add libatomic dependency".
>
>
I couldn't reproduce on the commit with the flush patch.
Would you mind sharing the libcamera log with me while
running android.hardware.cts.CameraTest#testJpegExif?

Thanks,
-Hiro

>
> Paul
>
> >
> >
> >
> >     >
> >     > There's some randomness in the RecordingTest, so that may not be
> >     > significant. The other three tests seem to pass consistently in
> master,
> >     > and fail consistently with the series applied. They also fail when
> run
> >     > in isolation.
> >     >
> >     > > > The rest of the patch looks good to me.
> >     > > >
> >     > > > > +
> >     > > > >         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
> >
>

Hello,

On Fri, Jun 04, 2021 at 06:59:38PM +0900, Hirokazu Honda wrote:
> Hi Paul,
>
>
>
> On Fri, Jun 4, 2021 at 6:27 PM <paul.elder@ideasonboard.com> wrote:
>
> > Hi Hiro,
> >
> > On Fri, Jun 04, 2021 at 05:48:45PM +0900, Hirokazu Honda wrote:
> > > Hi Paul and Laurent,
> > >
> > > On Wed, Jun 2, 2021 at 1:38 PM <paul.elder@ideasonboard.com> wrote:
> > >
> > >     Hi Jacopo,
> > >
> > >     On Wed, Jun 02, 2021 at 03:58:47AM +0300, Laurent Pinchart wrote:
> > >     > Hi Jacopo,
> > >     >
> > >     > On Mon, May 31, 2021 at 03:59:53AM +0300, Laurent Pinchart wrote:
> > >     > > On Fri, May 28, 2021 at 10:53:28AM +0300, Laurent Pinchart wrote:
> > >     > > > On Fri, May 28, 2021 at 12:03:59AM +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@jmondi.org>
> > >     > > > > ---
> > >     > > > >  src/android/camera_device.cpp | 74
> > >     ++++++++++++++++++++++++++++++++++-
> > >     > > > >  src/android/camera_device.h   |  3 ++
> > >     > > > >  src/android/camera_ops.cpp    |  8 +++-
> > >     > > > >  3 files changed, 83 insertions(+), 2 deletions(-)
> > >     > > > >
> > >     > > > > diff --git a/src/android/camera_device.cpp b/src/android/
> > >     camera_device.cpp
> > >     > > > > index a20c3eaa0ff6..6a8d4d4d5f76 100644
> > >     > > > > --- a/src/android/camera_device.cpp
> > >     > > > > +++ b/src/android/camera_device.cpp
> > >     > > > > @@ -797,6 +797,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_);
> > >     > > > > @@ -1894,15 +1911,46 @@ int CameraDevice::processControls
> > >     (Camera3RequestDescriptor *descriptor)
> > >     > > > >         return 0;
> > >     > > > >  }
> > >     > > > >
> > >     > > > > +void CameraDevice::abortRequest(camera3_capture_request_t
> > >     *request)
> > >     > > > > +{
> > >     > > > > +       notifyError(request->frame_number, nullptr,
> > >     CAMERA3_MSG_ERROR_REQUEST);
> > >     > > > > +
> > >     > > > > +       camera3_capture_result_t result = {};
> > >     > > > > +       result.num_output_buffers =
> > request->num_output_buffers;
> > >     > > > > +       result.frame_number = request->frame_number;
> > >     > > > > +       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.acquire_fence = -1;
> > >     > > > > +               buffer.status = CAMERA3_BUFFER_STATUS_ERROR;
> > >     > > > > +       }
> > >     > > > > +       result.output_buffers = resultBuffers.data();
> > >     > > > > +
> > >     > > > > +       callbacks_->process_capture_result(callbacks_,
> > &result);
> > >     > > > > +}
> > >     > > > > +
> > >     > > > >  int
> > CameraDevice::processCaptureRequest(camera3_capture_request_t
> > >     *camera3Request)
> > >     > > > >  {
> > >     > > > >         if (!isValidRequest(camera3Request))
> > >     > > > >                 return -EINVAL;
> > >     > > > >
> > >     > > > >         {
> > >     > > > > +               /*
> > >     > > > > +                * Start the camera if that's the first
> > request we
> > >     handle after
> > >     > > > > +                * a configuration or after a flush.
> > >     > > > > +                *
> > >     > > > > +                * If flush is in progress, return the
> > pending
> > >     request
> > >     > > > > +                * immediately in error state.
> > >     > > > > +                */
> > >     > > > >                 MutexLocker stateLock(stateMutex_);
> > >     > > > > +               if (state_ == State::Flushing) {
> > >     > > > > +                       abortRequest(camera3Request);
> > >     > > > > +                       return 0;
> > >     > > > > +               }
> > >     > > > >
> > >     > > > > -               /* Start the camera if that's the first
> > request we
> > >     handle. */
> > >     > > > >                 if (state_ == State::Stopped) {
> > >     > > > >                         worker_.start();
> > >     > > > >
> > >     > > > > @@ -2004,6 +2052,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_);
> > >     > > > > +       if (state_ == State::Flushing) {
> > >     > > > > +               abortRequest(camera3Request);
> > >     > > > > +               return 0;
> > >     > > > > +       }
> > >     > > >
> > >     > > > It seems possibly overkill to do this check twice, but it
> > shouldn't
> > >     > > > hurt. I suspect we'll rework this code later, possibly by
> > adding a
> > >     > > > Camera::flush() in the libcamera native API, although I'm not
> > >     entirely
> > >     > > > sure what benefit it would bring compared to a stop/start. For
> > now
> > >     this
> > >     > > > should be fine.
> > >     > > >
> > >     > > > > +
> > >     > > > > +       if (state_ == State::Stopped) {
> > >     > > > > +               worker_.start();
> > >     > > > > +
> > >     > > > > +               ret = camera_->start();
> > >     > > > > +               if (ret) {
> > >     > > > > +                       LOG(HAL, Error) << "Failed to start
> > >     camera";
> > >     > > > > +                       return ret;
> > >     > > > > +               }
> > >     > > > > +
> > >     > > > > +               state_ = State::Running;
> > >     > > > > +       }
> > >     > > >
> > >     > > > This, however, bothers me a bit. Why do we need to start the
> > camera
> > >     in
> > >     > > > two different locations ? Could we drop the first start above
> > ? And
> > >     if
> > >     > > > we do so, given that preparing the request should be a short
> > >     operation,
> > >     > > > I wonder if we shouldn't also drop the first Flushing check at
> > the
> > >     top
> > >     > > > of this function.
> > >     > >
> > >     > > This shouldn't be a blocker though, so I'll merge the series
> > after
> > >     > > running tests. We can address the issue on top.
> > >     >
> > >     > I'm afraid this series causes CTS regressions :-(
> > >     >
> > >     > I'm running the full camera tests with
> > >     >
> > >     > run cts-dev --skip-preconditions -a x86_64 -m CtsCameraTestCases
> > >     >
> > >     > With the current master branch, I get 22 or 23 failures (some are
> > a bit
> > >     > random), and with this series, it increases to 25. Here's the diff:
> > >     >
> > >     > @@ -3,14 +3,16 @@
> > >     >  android.hardware.camera2.cts.ImageReaderTest#testRawPrivate
> > >     >
> > android.hardware.camera2.cts.ImageReaderTest#testRepeatingRawPrivate
> > >     >  android.hardware.camera2.cts.RecordingTest#testSupportedVideoSizes
> > >     > -android.hardware.camera2.cts.RecordingTest#testVideoSnapshot
> > >     >  android.hardware.camera2.cts.RobustnessTest#
> > >     testMandatoryOutputCombinations
> > >     > +android.hardware.camera2.cts.StillCaptureTest#testFocalLengths
> > >     > +android.hardware.camera2.cts.StillCaptureTest#testJpegExif
> > >     >  android.hardware.camera2.cts.StillCaptureTest#
> > >     testStillPreviewCombination
> > >     >
> > android.hardware.camera2.cts.SurfaceViewPreviewTest#testDeferredSurfaces
> > >     >
> > android.hardware.cts.CameraGLTest#testSetPreviewTextureBothCallbacks
> > >     >
> > android.hardware.cts.CameraGLTest#testSetPreviewTexturePreviewCallback
> > >     >  android.hardware.cts.CameraTest#testFocusDistances
> > >     >  android.hardware.cts.CameraTest#testImmediateZoom
> > >     > +android.hardware.cts.CameraTest#testJpegExif
> > >     >  android.hardware.cts.CameraTest#testPreviewCallback
> > >     >  android.hardware.cts.CameraTest#testPreviewCallbackWithBuffer
> > >     >  android.hardware.cts.CameraTest#testPreviewCallbackWithPicture
> > >
> > >     With the subplan, all of the existing ones are excluded. The
> > >     RecordingTests are failing at random (I'll file a bug report for
> > this).
> > >
> > >     I've managed to reproduce the three extra failures though, and they
> > >     consistently fail with a segfault.
> > >
> > >
> > >
> > > I ran android.hardware.camera2.cts.StillCaptureTest and the failure
> > tests pass.
> > > Could you tell me on top of what commit you applied the patch series?
> >
> > On... master. I think 59de56f4 "qcam: Add libatomic dependency".
> >
> >
> I couldn't reproduce on the commit with the flush patch.
> Would you mind sharing the libcamera log with me while
> running android.hardware.cts.CameraTest#testJpegExif?
>

I've been able to reproduce the segfault.
I have tested and sent the flush() series before Paul's metadata
auto-resize series went in. The rebase of flush on top of the most
recent master requires a little change in [1/6]

diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
index 075a8332f3b3..510fe0eae9ca 100644
--- a/src/android/camera_device.cpp
+++ b/src/android/camera_device.cpp
@@ -2174,7 +2174,6 @@ void CameraDevice::requestComplete(Request *request)
                /* The camera framework expects an empy metadata pack on error. */
                resultMetadata = std::make_unique<CameraMetadata>(0, 0);
        }
-       captureResult.result = resultMetadata->get();

        /* Handle any JPEG compression. */
        for (camera3_stream_buffer_t &buffer : descriptor.buffers_) {
@@ -2210,6 +2209,7 @@ void CameraDevice::requestComplete(Request *request)
                }
        }

+       captureResult.result = resultMetadata->get();
        callbacks_->process_capture_result(callbacks_, &captureResult);
 }

reason being that the new metadata auto-resize relocate the metadata
pack, hence when encoding is requested and new metadata are added to the
result pack, if it gets relocated the pointer assigned to
captureResult.result is invalid.

The fix is trivial, it is enough to assign captureResult.result just
after the encoding phase.

I'll re-send with this latest change.

Thanks
  j

> -Hiro
>
> >
> > Paul
> >
> > >
> > >
> > >
> > >     >
> > >     > There's some randomness in the RecordingTest, so that may not be
> > >     > significant. The other three tests seem to pass consistently in
> > master,
> > >     > and fail consistently with the series applied. They also fail when
> > run
> > >     > in isolation.
> > >     >
> > >     > > > The rest of the patch looks good to me.
> > >     > > >
> > >     > > > > +
> > >     > > > >         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
> > >
> >