[PATCH v4 7/7] android: Remove Camera3RequestDescriptor::streamsProcessMutex_

[Cheng-Hao Yang]

Friendly ping for Laurent, please take a look when available.

Thanks,
Harvey

On Thu, Jan 2, 2025 at 2:17 PM Cheng-Hao Yang <chenghaoyang at chromium.org> wrote:
>
> Hi Laurent,
>
> I think Jacopo left this to your review. Could you take a look when
> you've got some time?
> This should be the last patch in this series that hasn't been reviewed.
>
> Thanks,
> Harvey
>
> On Tue, Dec 10, 2024 at 3:26 PM Harvey Yang <chenghaoyang at chromium.org> wrote:
> >
> > This mutex was needed when CameraStream's worker thread posts a result
> > back to CameraDevice. We can simplify it by calling CameraDevice's
> > function on libcamera::Camera's owner thread. With this delegation,
> > `Camera3RequestDescriptor::pendingStreamsToProcess_` will be firstly
> > setup in the application's thread in processCaptureRequest(), and the
> > rest accesses will be done in libcamera::CameraManager's thread.
> >
> > Signed-off-by: Han-Lin Chen <hanlinchen at chromium.org>
> > Co-developed-by: Harvey Yang <chenghaoyang at chromium.org>
> > Signed-off-by: Harvey Yang <chenghaoyang at chromium.org>
> > ---
> >  src/android/camera_device.cpp | 33 +++++++++++++++++----------------
> >  src/android/camera_device.h   |  6 ++++--
> >  src/android/camera_request.h  |  4 +---
> >  src/android/camera_stream.cpp |  4 ++--
> >  4 files changed, 24 insertions(+), 23 deletions(-)
> >
> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > index a95114c8d..cb819938a 100644
> > --- a/src/android/camera_device.cpp
> > +++ b/src/android/camera_device.cpp
> > @@ -989,8 +989,6 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> >                 FrameBuffer *frameBuffer = nullptr;
> >                 UniqueFD acquireFence;
> >
> > -               MutexLocker lock(descriptor->streamsProcessMutex_);
> > -
> >                 switch (cameraStream->type()) {
> >                 case CameraStream::Type::Mapped:
> >                         /* Mapped streams will be handled in the next loop. */
> > @@ -1066,7 +1064,6 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> >                                 << cameraStream->configuration().pixelFormat << "]"
> >                                 << " (mapped)";
> >
> > -               MutexLocker lock(descriptor->streamsProcessMutex_);
> >                 descriptor->pendingStreamsToProcess_.insert({ cameraStream, &buffer });
> >
> >                 /*
> > @@ -1252,9 +1249,6 @@ void CameraDevice::requestComplete(Request *request)
> >                 descriptor->resultMetadata_ = std::make_unique<CameraMetadata>(0, 0);
> >         }
> >
> > -       /* Handle post-processing. */
> > -       MutexLocker locker(descriptor->streamsProcessMutex_);
> > -
> >         /*
> >          * Queue all the post-processing streams request at once. The completion
> >          * slot streamProcessingComplete() can only execute when we are out
> > @@ -1288,10 +1282,8 @@ void CameraDevice::requestComplete(Request *request)
> >                 }
> >         }
> >
> > -       if (descriptor->pendingStreamsToProcess_.empty()) {
> > -               locker.unlock();
> > +       if (descriptor->pendingStreamsToProcess_.empty())
> >                 completeDescriptor(descriptor);
> > -       }
> >  }
> >
> >  /**
> > @@ -1398,6 +1390,19 @@ void CameraDevice::setBufferStatus(StreamBuffer &streamBuffer,
> >         }
> >  }
> >
> > +void CameraDevice::streamProcessingCompleteDelegate(StreamBuffer *streamBuffer,
> > +                                                   StreamBuffer::Status status)
> > +{
> > +       /*
> > +        * Delegate the callback to the camera manager thread to simplify race condition.
> > +        */
> > +       auto *method = new BoundMethodMember{
> > +               this, camera_.get(), &CameraDevice::streamProcessingComplete, ConnectionTypeQueued
> > +       };
> > +
> > +       method->activate(streamBuffer, status);
> > +}
> > +
> >  /**
> >   * \brief Handle post-processing completion of a stream in a capture request
> >   * \param[in] streamBuffer The StreamBuffer for which processing is complete
> > @@ -1418,13 +1423,9 @@ void CameraDevice::streamProcessingComplete(StreamBuffer *streamBuffer,
> >
> >         Camera3RequestDescriptor *request = streamBuffer->request;
> >
> > -       {
> > -               MutexLocker locker(request->streamsProcessMutex_);
> > -
> > -               request->pendingStreamsToProcess_.erase(streamBuffer->stream);
> > -               if (!request->pendingStreamsToProcess_.empty())
> > -                       return;
> > -       }
> > +       request->pendingStreamsToProcess_.erase(streamBuffer->stream);
> > +       if (!request->pendingStreamsToProcess_.empty())
> > +               return;
> >
> >         completeDescriptor(streamBuffer->request);
> >  }
> > diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> > index 699aa8f17..69d163d76 100644
> > --- a/src/android/camera_device.h
> > +++ b/src/android/camera_device.h
> > @@ -65,8 +65,8 @@ public:
> >         int configureStreams(camera3_stream_configuration_t *stream_list);
> >         int processCaptureRequest(camera3_capture_request_t *request);
> >         void requestComplete(libcamera::Request *request);
> > -       void streamProcessingComplete(StreamBuffer *bufferStream,
> > -                                     StreamBuffer::Status status);
> > +       void streamProcessingCompleteDelegate(StreamBuffer *bufferStream,
> > +                                             StreamBuffer::Status status);
> >
> >  protected:
> >         std::string logPrefix() const override;
> > @@ -96,6 +96,8 @@ private:
> >         int processControls(Camera3RequestDescriptor *descriptor);
> >         void completeDescriptor(Camera3RequestDescriptor *descriptor)
> >                 LIBCAMERA_TSA_EXCLUDES(descriptorsMutex_);
> > +       void streamProcessingComplete(StreamBuffer *bufferStream,
> > +                                     StreamBuffer::Status status);
> >         void sendCaptureResults() LIBCAMERA_TSA_REQUIRES(descriptorsMutex_);
> >         void sendCaptureResult(Camera3RequestDescriptor *request) const;
> >         void setBufferStatus(StreamBuffer &buffer,
> > diff --git a/src/android/camera_request.h b/src/android/camera_request.h
> > index 6b2a00795..bd75d4595 100644
> > --- a/src/android/camera_request.h
> > +++ b/src/android/camera_request.h
> > @@ -66,9 +66,7 @@ public:
> >         };
> >
> >         /* Keeps track of streams requiring post-processing. */
> > -       std::map<CameraStream *, StreamBuffer *> pendingStreamsToProcess_
> > -               LIBCAMERA_TSA_GUARDED_BY(streamsProcessMutex_);
> > -       libcamera::Mutex streamsProcessMutex_;
> > +       std::map<CameraStream *, StreamBuffer *> pendingStreamsToProcess_;
> >
> >         Camera3RequestDescriptor(libcamera::Camera *camera,
> >                                  const camera3_capture_request_t *camera3Request);
> > diff --git a/src/android/camera_stream.cpp b/src/android/camera_stream.cpp
> > index 53f292d4b..7837fd7aa 100644
> > --- a/src/android/camera_stream.cpp
> > +++ b/src/android/camera_stream.cpp
> > @@ -121,8 +121,8 @@ int CameraStream::configure()
> >                                 else
> >                                         bufferStatus = StreamBuffer::Status::Error;
> >
> > -                               cameraDevice_->streamProcessingComplete(streamBuffer,
> > -                                                                       bufferStatus);
> > +                               cameraDevice_->streamProcessingCompleteDelegate(streamBuffer,
> > +                                                                               bufferStatus);
> >                         });
> >
> >                 worker_->start();
> > --
> > 2.47.0.338.g60cca15819-goog
> >