[libcamera-devel] [PATCH v4 8/8] android: Implement flush() camera operation
Jacopo Mondi
jacopo at jmondi.org
Tue Jun 8 15:02:39 CEST 2021
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 at 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 at 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 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
> > >
> > > 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
> > >
> >
More information about the libcamera-devel
mailing list