[libcamera-devel] [PATCH v6 3/7] android: camera_device: Refactor descriptor status and sendCaptureResults()
Hirokazu Honda
hiroh at chromium.org
Mon Oct 25 07:38:19 CEST 2021
Hi Umang,
On Mon, Oct 25, 2021 at 2:07 PM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Umang,
>
> Thank you for the patch.
>
> On Sat, Oct 23, 2021 at 04:02:58PM +0530, Umang Jain wrote:
> > Currently, we use Camera3RequestDescriptor::Status to determine:
> > - When the descriptor has been completely processed by HAL
> > - Whether any errors were encountered, during its processing
> >
> > Both of these are essential to know whether the descriptor is eligible
> > to call process_capture_results() through sendCaptureResults().
> > When a status(Success/Error) is set on the descriptor, it is ready to
> > be sent back via sendCaptureResults(). However, this might lead to
> > undesired results especially when sendCaptureResults() runs in a
> > different thread (for e.g. stream's post-processor async completion
> > slot).
> >
> > This patch decouples the descriptor status (Success/Error) from the
> > descriptor's completion status (pending or complete). The advantage
> > of this is we can set the completion status when the descriptor has
> > been processed fully by the layer and we can set the error status on
> > the descriptor wherever an error is encountered, throughout the
> > lifetime descriptor in the HAL layer.
> >
> > While at it, introduce a wrapper completeDescriptor() around
> > sendCaptureResults(). completeDescriptor() as the name suggests will
> > mark the descriptor as complete, so it is ready to be sent back.
> > The locking mechanism is moved from sendCaptureResults() to this wrapper
> > since the intention is to use completeDescriptor() in place of existing
> > sendCaptureResults() calls.
> >
> > Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> > ---
> > src/android/camera_device.cpp | 33 ++++++++++++++++++---------------
> > src/android/camera_device.h | 1 +
> > src/android/camera_request.cpp | 2 +-
> > src/android/camera_request.h | 7 ++++---
> > 4 files changed, 24 insertions(+), 19 deletions(-)
> >
> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > index 806b4090..f4d4fb09 100644
> > --- a/src/android/camera_device.cpp
> > +++ b/src/android/camera_device.cpp
> > @@ -982,13 +982,13 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> > MutexLocker stateLock(stateMutex_);
> >
> > if (state_ == State::Flushing) {
> > - abortRequest(descriptor.get());
> > + Camera3RequestDescriptor *rawDescriptor = descriptor.get();
> > + abortRequest(rawDescriptor);
>
> Could we move the abortRequest() call just before completeDescriptor(),
> to always operate with the descriptor already added to the queue ? It
> should make no difference today, but consistent call patterns will make
> it easier to understand the code and the possible race conditions.
>
> > {
> > MutexLocker descriptorsLock(descriptorsMutex_);
> > descriptors_.push(std::move(descriptor));
> > }
> > -
> > - sendCaptureResults();
> > + completeDescriptor(rawDescriptor);
> >
> > return 0;
> > }
> > @@ -1079,7 +1079,7 @@ void CameraDevice::requestComplete(Request *request)
> > << request->status();
> >
> > abortRequest(descriptor);
> > - sendCaptureResults();
> > + completeDescriptor(descriptor);
> >
> > return;
> > }
> > @@ -1117,6 +1117,7 @@ void CameraDevice::requestComplete(Request *request)
> > }
> >
> > /* Handle post-processing. */
> > + bool hasPostProcessingErrors = false;
> > for (auto &buffer : descriptor->buffers_) {
> > CameraStream *stream = buffer.stream;
> >
> > @@ -1129,6 +1130,7 @@ void CameraDevice::requestComplete(Request *request)
> > buffer.status = Camera3RequestDescriptor::Status::Error;
> > notifyError(descriptor->frameNumber_, stream->camera3Stream(),
> > CAMERA3_MSG_ERROR_BUFFER);
> > + hasPostProcessingErrors = true;
>
> You can set
>
> descriptor->status_ = Camera3RequestDescriptor::Status::Error;
>
> here.
>
> > continue;
> > }
> >
> > @@ -1143,29 +1145,32 @@ void CameraDevice::requestComplete(Request *request)
> >
> > if (ret) {
> > buffer.status = Camera3RequestDescriptor::Status::Error;
> > + hasPostProcessingErrors = true;
>
> And here too.
>
> > notifyError(descriptor->frameNumber_, stream->camera3Stream(),
> > CAMERA3_MSG_ERROR_BUFFER);
> > }
> > }
> >
> > - descriptor->status_ = Camera3RequestDescriptor::Status::Success;
> > + if (hasPostProcessingErrors)
> > + descriptor->status_ = Camera3RequestDescriptor::Status::Error;
>
> And then drop this.
>
> > +
> > + completeDescriptor(descriptor);
> > +}
> > +
> > +void CameraDevice::completeDescriptor(Camera3RequestDescriptor *descriptor)
> > +{
> > + MutexLocker lock(descriptorsMutex_);
> > + descriptor->completeDescriptor();
> > +
> > sendCaptureResults();
> > }
> >
> > void CameraDevice::sendCaptureResults()
> > {
> > - MutexLocker lock(descriptorsMutex_);
Just note:
We may want to have an assertion to acquire lock.
Looks like there is no way of doing this in std::mutex only.
The only way is to pass MutexLocker reference to here and
ASSERT(lock.owns_lock()).
Another brilliant way is to use thread annotation and annotate this
function by REQUIRES().
https://bugs.libcamera.org/show_bug.cgi?id=23
Change looks good.
Reviewed-by: Hirokazu Honda <hiroh at chromium.org>
> > while (!descriptors_.empty() && !descriptors_.front()->isPending()) {
> > auto descriptor = std::move(descriptors_.front());
> > descriptors_.pop();
> >
> > - /*
> > - * \todo Releasing and re-acquiring the critical section for
> > - * every request completion (grain-locking) might have an
> > - * impact on performance which should be measured.
> > - */
> > - lock.unlock();
> > -
> > camera3_capture_result_t captureResult = {};
> >
> > captureResult.frame_number = descriptor->frameNumber_;
> > @@ -1201,8 +1206,6 @@ void CameraDevice::sendCaptureResults()
> > captureResult.partial_result = 1;
> >
> > callbacks_->process_capture_result(callbacks_, &captureResult);
> > -
> > - lock.lock();
> > }
> > }
> >
> > diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> > index 863cf414..e544f2bd 100644
> > --- a/src/android/camera_device.h
> > +++ b/src/android/camera_device.h
> > @@ -93,6 +93,7 @@ private:
> > void notifyError(uint32_t frameNumber, camera3_stream_t *stream,
> > camera3_error_msg_code code) const;
> > int processControls(Camera3RequestDescriptor *descriptor);
> > + void completeDescriptor(Camera3RequestDescriptor *descriptor);
> > void sendCaptureResults();
> > std::unique_ptr<CameraMetadata> getResultMetadata(
> > const Camera3RequestDescriptor &descriptor) const;
> > diff --git a/src/android/camera_request.cpp b/src/android/camera_request.cpp
> > index faa85ada..16cf266f 100644
> > --- a/src/android/camera_request.cpp
> > +++ b/src/android/camera_request.cpp
> > @@ -36,7 +36,7 @@ Camera3RequestDescriptor::Camera3RequestDescriptor(
> > static_cast<CameraStream *>(buffer.stream->priv);
> >
> > buffers_.push_back({ stream, buffer.buffer, nullptr,
> > - buffer.acquire_fence, Status::Pending });
> > + buffer.acquire_fence, Status::Success });
> > }
> >
> > /* Clone the controls associated with the camera3 request. */
> > diff --git a/src/android/camera_request.h b/src/android/camera_request.h
> > index 05dabf89..904886da 100644
> > --- a/src/android/camera_request.h
> > +++ b/src/android/camera_request.h
> > @@ -26,7 +26,6 @@ class Camera3RequestDescriptor
> > {
> > public:
> > enum class Status {
> > - Pending,
> > Success,
> > Error,
> > };
> > @@ -43,7 +42,8 @@ public:
> > const camera3_capture_request_t *camera3Request);
> > ~Camera3RequestDescriptor();
> >
> > - bool isPending() const { return status_ == Status::Pending; }
> > + bool isPending() const { return !complete_; }
> > + void completeDescriptor() { complete_ = true; }
>
> As the function is a member of the Camera3RequestDescriptor class, I
> wouldn't repeat "descriptor" in the name. Given that the complete_
> member is public, I'd actually drop the function and access complete_
> directly, the same way we access status_. If you prefer an accessor, I'd
> make the complete_ member private, but that's probably overkill for now
> given that all other members are public.
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
> > uint32_t frameNumber_ = 0;
> >
> > @@ -53,7 +53,8 @@ public:
> > std::unique_ptr<CaptureRequest> request_;
> > std::unique_ptr<CameraMetadata> resultMetadata_;
> >
> > - Status status_ = Status::Pending;
> > + bool complete_ = false;
> > + Status status_ = Status::Success;
> >
> > private:
> > LIBCAMERA_DISABLE_COPY(Camera3RequestDescriptor)
>
> --
> Regards,
>
> Laurent Pinchart
More information about the libcamera-devel
mailing list