[libcamera-devel,v5,4/4] android: camera_device: Protect descriptor status_ with lock
diff mbox series

Message ID 20211020104212.121743-5-umang.jain@ideasonboard.com
State Superseded
Headers show
Series
  • Async Post Processor
Related show

Commit Message

Umang Jain Oct. 20, 2021, 10:42 a.m. UTC 
From: Laurent Pinchart <laurent.pinchart@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@ideasonboard.com>
Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---
 src/android/camera_device.cpp | 29 +++++++++++++++++++----------
 src/android/camera_device.h   |  5 ++++-
 2 files changed, 23 insertions(+), 11 deletions(-)

Comments

Laurent Pinchart Oct. 21, 2021, 1:46 a.m. UTC | #1 
On Wed, Oct 20, 2021 at 04:12:12PM +0530, Umang Jain wrote:
> From: Laurent Pinchart <laurent.pinchart@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@ideasonboard.com>
> Signed-off-by: Umang Jain <umang.jain@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 :-/
I think one option would be to apply the following fixup.

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_;
Umang Jain Oct. 21, 2021, 6:16 a.m. UTC | #2 
Hi Launent,

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@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@ideasonboard.com>
>> Signed-off-by: Umang Jain <umang.jain@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 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_;
Laurent Pinchart Oct. 21, 2021, 1:31 p.m. UTC | #3 
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@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@ideasonboard.com>
> >> Signed-off-by: Umang Jain <umang.jain@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_;

Patch
diff mbox series 

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(),
+				   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_;