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

[Cheng-Hao Yang]

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
>