[v2,6/9] android: Cleanup CAMERA3_MSG_ERROR_REQUEST
diff mbox series

Message ID 20241127092632.3145984-7-chenghaoyang@chromium.org
State New
Headers show
Series
  • Signal metadataAvailable and Android partial result
Related show

Commit Message

Cheng-Hao Yang Nov. 27, 2024, 9:25 a.m. UTC 
When a request is completed with failure, CAMERA3_MSG_ERROR_RESULT
should be used instead.

This patch also cleans up aborting when flushing with
sendCaptureResult().

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 | 28 +++++++++++++++-------------
 1 file changed, 15 insertions(+), 13 deletions(-)

Comments

Jacopo Mondi Nov. 28, 2024, 3:02 p.m. UTC | #1 
Hi Harvey

On Wed, Nov 27, 2024 at 09:25:56AM +0000, Harvey Yang wrote:
> When a request is completed with failure, CAMERA3_MSG_ERROR_RESULT
> should be used instead.
>
> This patch also cleans up aborting when flushing with
> sendCaptureResult().
>
> 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 | 28 +++++++++++++++-------------
>  1 file changed, 15 insertions(+), 13 deletions(-)
>
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index 0377cf215..3fb92268e 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -868,6 +868,8 @@ void CameraDevice::abortRequest(Camera3RequestDescriptor *descriptor) const
>  		buffer.status = StreamBuffer::Status::Error;
>
>  	descriptor->status_ = Camera3RequestDescriptor::Status::Error;
> +
> +	sendCaptureResult(descriptor);
>  }
>
>  bool CameraDevice::isValidRequest(camera3_capture_request_t *camera3Request) const
> @@ -1136,14 +1138,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>  	MutexLocker stateLock(stateMutex_);
>
>  	if (state_ == State::Flushing) {
> -		Camera3RequestDescriptor *rawDescriptor = descriptor.get();
> -		{
> -			MutexLocker descriptorsLock(descriptorsMutex_);
> -			descriptors_.push(std::move(descriptor));
> -		}
> -		abortRequest(rawDescriptor);
> -		completeDescriptor(rawDescriptor);
> -
> +		abortRequest(descriptor.get());

I'm not sure I get this change ?

Specifically why we don't complete the descriptor anymore..


>  		return 0;
>  	}
>
> @@ -1211,10 +1206,7 @@ void CameraDevice::requestComplete(Request *request)
>  				<< " not successfully completed: "
>  				<< request->status();
>
> -		abortRequest(descriptor);
> -		completeDescriptor(descriptor);
> -
> -		return;
> +		descriptor->status_ = Camera3RequestDescriptor::Status::Error;
>  	}
>
>  	/*
> @@ -1239,7 +1231,7 @@ void CameraDevice::requestComplete(Request *request)
>  	 */
>  	descriptor->resultMetadata_ = getResultMetadata(*descriptor);
>  	if (!descriptor->resultMetadata_) {
> -		notifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_RESULT);
> +		descriptor->status_ = Camera3RequestDescriptor::Status::Error;
>
>  		/*
>  		 * The camera framework expects an empty metadata pack on error.
> @@ -1325,6 +1317,16 @@ void CameraDevice::sendCaptureResults()
>  		descriptors_.pop();
>
>  		sendCaptureResult(descriptor.get());
> +
> +		/*
> +		 * Call notify with CAMERA3_MSG_ERROR_RESULT to indicate some
> +		 * of the expected result metadata might not be available
> +		 * because the capture is cancelled by the camera. Only notify
> +		 * it when the final result is sent, since Android will ignore
> +		 * the following metadata.
> +		 */
> +		if (descriptor->status_ == Camera3RequestDescriptor::Status::Error)
> +			notifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_RESULT);
>  	}
>  }
>
> --
> 2.47.0.338.g60cca15819-goog
>
Cheng-Hao Yang Nov. 28, 2024, 3:30 p.m. UTC | #2 
Hi Jacopo,

On Thu, Nov 28, 2024 at 11:02 PM Jacopo Mondi
<jacopo.mondi@ideasonboard.com> wrote:
>
> Hi Harvey
>
> On Wed, Nov 27, 2024 at 09:25:56AM +0000, Harvey Yang wrote:
> > When a request is completed with failure, CAMERA3_MSG_ERROR_RESULT
> > should be used instead.
> >
> > This patch also cleans up aborting when flushing with
> > sendCaptureResult().
> >
> > 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 | 28 +++++++++++++++-------------
> >  1 file changed, 15 insertions(+), 13 deletions(-)
> >
> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > index 0377cf215..3fb92268e 100644
> > --- a/src/android/camera_device.cpp
> > +++ b/src/android/camera_device.cpp
> > @@ -868,6 +868,8 @@ void CameraDevice::abortRequest(Camera3RequestDescriptor *descriptor) const
> >               buffer.status = StreamBuffer::Status::Error;
> >
> >       descriptor->status_ = Camera3RequestDescriptor::Status::Error;
> > +
> > +     sendCaptureResult(descriptor);
> >  }
> >
> >  bool CameraDevice::isValidRequest(camera3_capture_request_t *camera3Request) const
> > @@ -1136,14 +1138,7 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
> >       MutexLocker stateLock(stateMutex_);
> >
> >       if (state_ == State::Flushing) {
> > -             Camera3RequestDescriptor *rawDescriptor = descriptor.get();
> > -             {
> > -                     MutexLocker descriptorsLock(descriptorsMutex_);
> > -                     descriptors_.push(std::move(descriptor));
> > -             }
> > -             abortRequest(rawDescriptor);
> > -             completeDescriptor(rawDescriptor);
> > -
> > +             abortRequest(descriptor.get());
>
> I'm not sure I get this change ?
>
> Specifically why we don't complete the descriptor anymore..

Okay, apart from the fix of CAMERA3_MSG_ERROR_RESULT
instead of CAMERA3_MSG_ERROR_REQUEST, this patch
contains one more thing that is only briefly mentioned in the
commit message:

When flushing and aborting new requests, we should notify
CAMERA3_MSG_ERROR_REQUEST, and call
`process_capture_result` earlier, instead of waiting for the
previous requests being completed first.

`completeDescriptor` would trigger the in-order completion
of requests only, while in this patch, we call
`sendCaptureResult` directly, without the necessity to
set `complete_` anymore.

Hope it helps explain.

BR,
Harvey

>
>
> >               return 0;
> >       }
> >
> > @@ -1211,10 +1206,7 @@ void CameraDevice::requestComplete(Request *request)
> >                               << " not successfully completed: "
> >                               << request->status();
> >
> > -             abortRequest(descriptor);
> > -             completeDescriptor(descriptor);
> > -
> > -             return;
> > +             descriptor->status_ = Camera3RequestDescriptor::Status::Error;
> >       }
> >
> >       /*
> > @@ -1239,7 +1231,7 @@ void CameraDevice::requestComplete(Request *request)
> >        */
> >       descriptor->resultMetadata_ = getResultMetadata(*descriptor);
> >       if (!descriptor->resultMetadata_) {
> > -             notifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_RESULT);
> > +             descriptor->status_ = Camera3RequestDescriptor::Status::Error;
> >
> >               /*
> >                * The camera framework expects an empty metadata pack on error.
> > @@ -1325,6 +1317,16 @@ void CameraDevice::sendCaptureResults()
> >               descriptors_.pop();
> >
> >               sendCaptureResult(descriptor.get());
> > +
> > +             /*
> > +              * Call notify with CAMERA3_MSG_ERROR_RESULT to indicate some
> > +              * of the expected result metadata might not be available
> > +              * because the capture is cancelled by the camera. Only notify
> > +              * it when the final result is sent, since Android will ignore
> > +              * the following metadata.
> > +              */
> > +             if (descriptor->status_ == Camera3RequestDescriptor::Status::Error)
> > +                     notifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_RESULT);
> >       }
> >  }
> >
> > --
> > 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 0377cf215..3fb92268e 100644
--- a/src/android/camera_device.cpp
+++ b/src/android/camera_device.cpp
@@ -868,6 +868,8 @@  void CameraDevice::abortRequest(Camera3RequestDescriptor *descriptor) const
 		buffer.status = StreamBuffer::Status::Error;
 
 	descriptor->status_ = Camera3RequestDescriptor::Status::Error;
+
+	sendCaptureResult(descriptor);
 }
 
 bool CameraDevice::isValidRequest(camera3_capture_request_t *camera3Request) const
@@ -1136,14 +1138,7 @@  int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
 	MutexLocker stateLock(stateMutex_);
 
 	if (state_ == State::Flushing) {
-		Camera3RequestDescriptor *rawDescriptor = descriptor.get();
-		{
-			MutexLocker descriptorsLock(descriptorsMutex_);
-			descriptors_.push(std::move(descriptor));
-		}
-		abortRequest(rawDescriptor);
-		completeDescriptor(rawDescriptor);
-
+		abortRequest(descriptor.get());
 		return 0;
 	}
 
@@ -1211,10 +1206,7 @@  void CameraDevice::requestComplete(Request *request)
 				<< " not successfully completed: "
 				<< request->status();
 
-		abortRequest(descriptor);
-		completeDescriptor(descriptor);
-
-		return;
+		descriptor->status_ = Camera3RequestDescriptor::Status::Error;
 	}
 
 	/*
@@ -1239,7 +1231,7 @@  void CameraDevice::requestComplete(Request *request)
 	 */
 	descriptor->resultMetadata_ = getResultMetadata(*descriptor);
 	if (!descriptor->resultMetadata_) {
-		notifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_RESULT);
+		descriptor->status_ = Camera3RequestDescriptor::Status::Error;
 
 		/*
 		 * The camera framework expects an empty metadata pack on error.
@@ -1325,6 +1317,16 @@  void CameraDevice::sendCaptureResults()
 		descriptors_.pop();
 
 		sendCaptureResult(descriptor.get());
+
+		/*
+		 * Call notify with CAMERA3_MSG_ERROR_RESULT to indicate some
+		 * of the expected result metadata might not be available
+		 * because the capture is cancelled by the camera. Only notify
+		 * it when the final result is sent, since Android will ignore
+		 * the following metadata.
+		 */
+		if (descriptor->status_ == Camera3RequestDescriptor::Status::Error)
+			notifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_RESULT);
 	}
 }