[v4,7/7] android: Remove Camera3RequestDescriptor::streamsProcessMutex_
diff mbox series

Message ID 20241210142557.2886315-8-chenghaoyang@chromium.org
State New
Headers show
Series
  • Refactor Android HAL before supporting partial result
Related show

Commit Message

Cheng-Hao Yang Dec. 10, 2024, 2:24 p.m. UTC 
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@chromium.org>
Co-developed-by: Harvey Yang <chenghaoyang@chromium.org>
Signed-off-by: Harvey Yang <chenghaoyang@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(-)

Comments

Cheng-Hao Yang Jan. 2, 2025, 1:17 p.m. UTC | #1 
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@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@chromium.org>
> Co-developed-by: Harvey Yang <chenghaoyang@chromium.org>
> Signed-off-by: Harvey Yang <chenghaoyang@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
>
Cheng-Hao Yang Jan. 20, 2025, 9:25 p.m. UTC | #2 
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@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@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@chromium.org>
> > Co-developed-by: Harvey Yang <chenghaoyang@chromium.org>
> > Signed-off-by: Harvey Yang <chenghaoyang@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
> >

Patch
diff mbox series 

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();