[libcamera-devel] [PATCH v4 8/8] android: Implement flush() camera operation
Hirokazu Honda
hiroh at chromium.org
Fri Jun 4 11:59:38 CEST 2021
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?
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
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210604/70523acc/attachment-0001.htm>
More information about the libcamera-devel
mailing list