[libcamera-devel] [PATCH v4 8/8] android: Implement flush() camera operation
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Jun 2 02:58:47 CEST 2021
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 at 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;
> > > }
> > >
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list