[libcamera-devel] [PATCH v5 4/4] android: camera_device: Protect descriptor status_ with lock
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Oct 21 15:31:09 CEST 2021
Hi Umang,
On Thu, Oct 21, 2021 at 11:46:20AM +0530, Umang Jain wrote:
> On 10/21/21 7:16 AM, Laurent Pinchart wrote:
> > On Wed, Oct 20, 2021 at 04:12:12PM +0530, Umang Jain wrote:
> >> From: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> >>
> >> The Camera3RequestDescriptor::status_ is checked as a stop condition for
> >> the sendCaptureResults() loop, protected by the descriptorsMutex_. The
> >> status is however not set with the mutex locked, which can cause a race
> >> condition with a concurrent sendCaptureResults() call (from the
> >> post-processor thread for instance).
> >>
> >> This should be harmless in practice, as the reader thread will either
> >> see the old status (Pending) and stop iterating over descriptors, or the
> >> new status and continue. Still, if the Camera3RequestDescriptor state
> >> machine were to change in the future, this could introduce hard to debug
> >> issues. Close the race window by always setting the status with the lock
> >> taken.
> >>
> >> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> >> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> >> ---
> >> src/android/camera_device.cpp | 29 +++++++++++++++++++----------
> >> src/android/camera_device.h | 5 ++++-
> >> 2 files changed, 23 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> >> index b14416ce..4e8fb2ee 100644
> >> --- a/src/android/camera_device.cpp
> >> +++ b/src/android/camera_device.cpp
> >> @@ -803,8 +803,6 @@ void CameraDevice::abortRequest(Camera3RequestDescriptor *descriptor) const
> >>
> >> for (auto &buffer : descriptor->buffers_)
> >> buffer.status = Camera3RequestDescriptor::Status::Error;
> >> -
> >> - descriptor->status_ = Camera3RequestDescriptor::Status::Error;
> >> }
> >>
> >> bool CameraDevice::isValidRequest(camera3_capture_request_t *camera3Request) const
> >> @@ -988,7 +986,8 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> >> descriptors_.push(std::move(descriptor));
> >> }
> >>
> >> - sendCaptureResults();
> >> + completeDescriptor(descriptor.get(),
> >
> > descriptor will be a nullptr here as it has been moved just above :-/
>
> Can't you just save .get() pointer and use it afterwards to fix this?
>
> A similar practice has been done at the end of same function for
> CaptureRequest
I've tried to avoid it, but it could be the simplest option, I agree.
Then I think I would prefer moving the abortRequest() call just before
completeDescriptor(), after adding the descriptor to the queue, to have
the same sequence as in CameraDevice::requestComplete().
> > I think one option would be to apply the following fixup.
>
> I don't like this option very much. It is because we are using
> sendCaptureResults() and completeDescriptor() both, as we please, to
> send back capture results.
>
> (If you haven't noticed already, sendCaptureResults() is now called
> only at one place in camera_device.cpp, i.e. completeDescriptor()). I
> would like to keep that status-quo as is, if possible
>
> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > index 4e8fb2ee49f2..e876d2ce8bfa 100644
> > --- a/src/android/camera_device.cpp
> > +++ b/src/android/camera_device.cpp
> > @@ -803,6 +803,9 @@ void CameraDevice::abortRequest(Camera3RequestDescriptor *descriptor) const
> >
> > for (auto &buffer : descriptor->buffers_)
> > buffer.status = Camera3RequestDescriptor::Status::Error;
> > +
> > + MutexLocker lock(descriptorsMutex_);
> > + descriptor->status_ = Camera3RequestDescriptor::Status::Error;
> > }
> >
> > bool CameraDevice::isValidRequest(camera3_capture_request_t *camera3Request) const
> > @@ -986,8 +989,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> > descriptors_.push(std::move(descriptor));
> > }
> >
> > - completeDescriptor(descriptor.get(),
> > - Camera3RequestDescriptor::Status::Error);
> > + sendCaptureResults();
> >
> > return 0;
> > }
> > @@ -1057,8 +1059,7 @@ void CameraDevice::requestComplete(Request *request)
> > << request->status();
> >
> > abortRequest(descriptor);
> > - completeDescriptor(descriptor,
> > - Camera3RequestDescriptor::Status::Error);
> > + sendCaptureResults();
> >
> > return;
> > }
> > @@ -1153,9 +1154,10 @@ void CameraDevice::requestComplete(Request *request)
> > void CameraDevice::completeDescriptor(Camera3RequestDescriptor *descriptor,
> > Camera3RequestDescriptor::Status status)
> > {
> > - MutexLocker lock(descriptorsMutex_);
> > - descriptor->status_ = status;
> > - lock.unlock();
> > + {
> > + MutexLocker lock(descriptorsMutex_);
> > + descriptor->status_ = status;
> > + }
> >
> > sendCaptureResults();
> > }
> > @@ -1262,14 +1264,14 @@ void CameraDevice::streamProcessingComplete(CameraStream *cameraStream,
> > hasPostProcessingErrors = true;
> > }
> >
> > + locker.unlock();
> > +
> > Camera3RequestDescriptor::Status descriptorStatus;
> > if (hasPostProcessingErrors)
> > descriptorStatus = Camera3RequestDescriptor::Status::Error;
> > else
> > descriptorStatus = Camera3RequestDescriptor::Status::Success;
> >
> > - locker.unlock();
> > -
> > completeDescriptor(request, descriptorStatus);
> > }
> >
> > diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> > index 46fb93ee777b..a85602cf178f 100644
> > --- a/src/android/camera_device.h
> > +++ b/src/android/camera_device.h
> > @@ -124,7 +124,7 @@ private:
> > std::vector<CameraStream> streams_;
> >
> > /* Protects descriptors_ and Camera3RequestDescriptor::status_. */
> > - libcamera::Mutex descriptorsMutex_;
> > + mutable libcamera::Mutex descriptorsMutex_;
> > std::queue<std::unique_ptr<Camera3RequestDescriptor>> descriptors_;
> >
> > std::string maker_;
> >
> >
> >> + Camera3RequestDescriptor::Status::Error);
> >>
> >> return 0;
> >> }
> >> @@ -1058,7 +1057,8 @@ void CameraDevice::requestComplete(Request *request)
> >> << request->status();
> >>
> >> abortRequest(descriptor);
> >> - sendCaptureResults();
> >> + completeDescriptor(descriptor,
> >> + Camera3RequestDescriptor::Status::Error);
> >>
> >> return;
> >> }
> >> @@ -1135,20 +1135,28 @@ void CameraDevice::requestComplete(Request *request)
> >>
> >> if (needsPostProcessing) {
> >> if (processingStatus == Camera3RequestDescriptor::Status::Error) {
> >> - descriptor->status_ = processingStatus;
> >> /*
> >> * \todo This is problematic when some streams are
> >> * queued successfully, but some fail to get queued.
> >> * We might end up with use-after-free situation for
> >> * descriptor in streamProcessingComplete().
> >> */
> >> - sendCaptureResults();
> >> + completeDescriptor(descriptor, processingStatus);
> >> }
> >>
> >> return;
> >> }
> >>
> >> - descriptor->status_ = Camera3RequestDescriptor::Status::Success;
> >> + completeDescriptor(descriptor, Camera3RequestDescriptor::Status::Success);
> >> +}
> >> +
> >> +void CameraDevice::completeDescriptor(Camera3RequestDescriptor *descriptor,
> >> + Camera3RequestDescriptor::Status status)
> >> +{
> >> + MutexLocker lock(descriptorsMutex_);
> >> + descriptor->status_ = status;
> >> + lock.unlock();
> >> +
> >> sendCaptureResults();
> >> }
> >>
> >> @@ -1254,14 +1262,15 @@ void CameraDevice::streamProcessingComplete(CameraStream *cameraStream,
> >> hasPostProcessingErrors = true;
> >> }
> >>
> >> + Camera3RequestDescriptor::Status descriptorStatus;
> >> if (hasPostProcessingErrors)
> >> - request->status_ = Camera3RequestDescriptor::Status::Error;
> >> + descriptorStatus = Camera3RequestDescriptor::Status::Error;
> >> else
> >> - request->status_ = Camera3RequestDescriptor::Status::Success;
> >> + descriptorStatus = Camera3RequestDescriptor::Status::Success;
> >>
> >> locker.unlock();
> >>
> >> - sendCaptureResults();
> >> + completeDescriptor(request, descriptorStatus);
> >> }
> >>
> >> std::string CameraDevice::logPrefix() const
> >> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> >> index 1ef933da..46fb93ee 100644
> >> --- a/src/android/camera_device.h
> >> +++ b/src/android/camera_device.h
> >> @@ -96,6 +96,8 @@ private:
> >> void notifyError(uint32_t frameNumber, camera3_stream_t *stream,
> >> camera3_error_msg_code code) const;
> >> int processControls(Camera3RequestDescriptor *descriptor);
> >> + void completeDescriptor(Camera3RequestDescriptor *descriptor,
> >> + Camera3RequestDescriptor::Status status);
> >> void sendCaptureResults();
> >> void setBufferStatus(CameraStream *cameraStream,
> >> Camera3RequestDescriptor::StreamBuffer &buffer,
> >> @@ -121,7 +123,8 @@ private:
> >>
> >> std::vector<CameraStream> streams_;
> >>
> >> - libcamera::Mutex descriptorsMutex_; /* Protects descriptors_. */
> >> + /* Protects descriptors_ and Camera3RequestDescriptor::status_. */
> >> + libcamera::Mutex descriptorsMutex_;
> >> std::queue<std::unique_ptr<Camera3RequestDescriptor>> descriptors_;
> >>
> >> std::string maker_;
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list