| Message ID | 20241204164137.3938891-6-chenghaoyang@chromium.org |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi Harvey On Wed, Dec 04, 2024 at 04:36:30PM +0000, Harvey Yang wrote: > According to Android Camera API v3.2, CAMERA3_MSG_ERROR_REQUEST is used > for requests that have not done any processing. When a request is > completed with failure, CAMERA3_MSG_ERROR_RESULT should be used instead. > > To avoid code duplication, when CameraMetadata cannot be generated, > CAMERA3_MSG_ERROR_RESULT is notified after process_capture_result. > > 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 | 17 ++++++++++++----- > 1 file changed, 12 insertions(+), 5 deletions(-) > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > index 497d363d6..dd2c603e0 100644 > --- a/src/android/camera_device.cpp > +++ b/src/android/camera_device.cpp > @@ -1211,10 +1211,7 @@ void CameraDevice::requestComplete(Request *request) You should update the comment before the if() > << " not successfully completed: " > << request->status(); > > - abortRequest(descriptor); > - completeDescriptor(descriptor); > - > - return; Is it ok to continue and try to fetch metadata from a Request in error status ? Also, the if (request->status() != Request::RequestComplete) { case, doesn't fall in /** * S6. Error management: * ... * - The failure of an entire capture to occur must be reported by the HAL by * calling notify() with ERROR_REQUEST. Individual errors for the result * metadata or the output buffers must not be reported in this case. I think the abortRequest(descriptor); completeDescriptor(descriptor); sequence should be kept here ? Specifically, if you don't call completeDescriptor() the descriptor will remain in pending state and stall the queue of complete descriptors > + descriptor->status_ = Camera3RequestDescriptor::Status::Error; > } > > /* > @@ -1239,7 +1236,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; To be honest I don't see what we gain by delaying the notification > > /* > * The camera framework expects an empty metadata pack on error. > @@ -1325,6 +1322,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); If my understanding of the specs if correct, I think in case of if (request->status() != Request::RequestComplete) { the right error code to notify is ERROR_REQUEST, so if my understanding is correct, I would rather drop this whole change. Or am I mistaken maybe ? Have you got any failure in CTS or other tests because of this ? > } > } > > -- > 2.47.0.338.g60cca15819-goog >
Hi Jacopo, On Fri, Dec 6, 2024 at 12:39 AM Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote: > > Hi Harvey > > On Wed, Dec 04, 2024 at 04:36:30PM +0000, Harvey Yang wrote: > > According to Android Camera API v3.2, CAMERA3_MSG_ERROR_REQUEST is used > > for requests that have not done any processing. When a request is > > completed with failure, CAMERA3_MSG_ERROR_RESULT should be used instead. > > > > To avoid code duplication, when CameraMetadata cannot be generated, > > CAMERA3_MSG_ERROR_RESULT is notified after process_capture_result. > > > > 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 | 17 ++++++++++++----- > > 1 file changed, 12 insertions(+), 5 deletions(-) > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > index 497d363d6..dd2c603e0 100644 > > --- a/src/android/camera_device.cpp > > +++ b/src/android/camera_device.cpp > > @@ -1211,10 +1211,7 @@ void CameraDevice::requestComplete(Request *request) > > You should update the comment before the if() Updated. Please take another look. > > > << " not successfully completed: " > > << request->status(); > > > > - abortRequest(descriptor); > > - completeDescriptor(descriptor); > > - > > - return; > > Is it ok to continue and try to fetch metadata from a Request in error > status ? I think so, as libcamera core library sets a Request in error status if and only if there's a buffer in error status. There might be some valid metadata in the Request. > > Also, the > > if (request->status() != Request::RequestComplete) { > > case, doesn't fall in > > /** > * S6. Error management: > * > > ... > > * - The failure of an entire capture to occur must be reported by the HAL by > * calling notify() with ERROR_REQUEST. Individual errors for the result > * metadata or the output buffers must not be reported in this case. > > I think the > > abortRequest(descriptor); > completeDescriptor(descriptor); > > sequence should be kept here ? Not really. Perhaps we should update `camera3.h`, while this is what I wanted to discuss with you in the previous version of this patch: CAMERA3_MSG_ERROR_REQUEST is mostly used for requests that haven't done any processing. CAMERA3_MSG_ERROR_RESULT is used for requests that have missing metadata. A request in error status doesn't fall into either of the cases. If you can take another look at my last comments in `[PATCH v2 6/9] android: Cleanup CAMERA3_MSG_ERROR_REQUEST`, we can align and discuss how to proceed here. Thanks :) Ref: https://source.android.com/reference/hal/structcamera3__device__ops > > Specifically, if you don't call completeDescriptor() the descriptor > will remain in pending state and stall the queue of complete > descriptors As we don't return in this if clause, either there is at least one buffer to be post-processed, or completeDescriptor() will be called on the descriptor. Let me know if I misunderstand anything. > > > > + descriptor->status_ = Camera3RequestDescriptor::Status::Error; > > } > > > > /* > > @@ -1239,7 +1236,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; > > To be honest I don't see what we gain by delaying the notification We can also remove the logic entirely: getResultMetadata() only fails to return a valid CameraMetadata with memory issues I think, not due to some corrupted Request::metadata(). It will be removed in the end anyways. WDYT? > > > > > > /* > > * The camera framework expects an empty metadata pack on error. > > @@ -1325,6 +1322,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); > > If my understanding of the specs if correct, I think in case of > > if (request->status() != Request::RequestComplete) { > > the right error code to notify is ERROR_REQUEST, so if my > understanding is correct, I would rather drop this whole change. > > Or am I mistaken maybe ? Have you got any failure in CTS or other > tests because of this ? Yeah that's what I wanted to fix with this patch: ``` 3.4 For captures that will produce some results, the HAL must not call CAMERA3_MSG_ERROR_REQUEST, since that indicates complete failure. ``` Previously, we just ignored any potentially available metadata and buffers and notified CAMERA3_MSG_ERROR_REQUEST on a request in error status. This doesn't seem correct to me, while I believe it still fits in the Android API. Regarding failure in CTS or other tests, I just tried on mtkisp7. Without this patch, CTS still passes. I think it should be fine without this patch, if it's controversial. BR, Harvey > > > } > > } > > > > -- > > 2.47.0.338.g60cca15819-goog > >
Hi Jacopo, On Mon, Dec 9, 2024 at 6:03 PM Cheng-Hao Yang <chenghaoyang@chromium.org> wrote: > > Hi Jacopo, > > On Fri, Dec 6, 2024 at 12:39 AM Jacopo Mondi > <jacopo.mondi@ideasonboard.com> wrote: > > > > Hi Harvey > > > > On Wed, Dec 04, 2024 at 04:36:30PM +0000, Harvey Yang wrote: > > > According to Android Camera API v3.2, CAMERA3_MSG_ERROR_REQUEST is used > > > for requests that have not done any processing. When a request is > > > completed with failure, CAMERA3_MSG_ERROR_RESULT should be used instead. > > > > > > To avoid code duplication, when CameraMetadata cannot be generated, > > > CAMERA3_MSG_ERROR_RESULT is notified after process_capture_result. > > > > > > 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 | 17 ++++++++++++----- > > > 1 file changed, 12 insertions(+), 5 deletions(-) > > > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > > index 497d363d6..dd2c603e0 100644 > > > --- a/src/android/camera_device.cpp > > > +++ b/src/android/camera_device.cpp > > > @@ -1211,10 +1211,7 @@ void CameraDevice::requestComplete(Request *request) > > > > You should update the comment before the if() > > Updated. Please take another look. > > > > > > << " not successfully completed: " > > > << request->status(); > > > > > > - abortRequest(descriptor); > > > - completeDescriptor(descriptor); > > > - > > > - return; > > > > Is it ok to continue and try to fetch metadata from a Request in error > > status ? > > I think so, as libcamera core library sets a Request in error status > if and only if there's a buffer in error status. There might be some > valid metadata in the Request. > > > > > Also, the > > > > if (request->status() != Request::RequestComplete) { > > > > case, doesn't fall in > > > > /** > > * S6. Error management: > > * > > > > ... > > > > * - The failure of an entire capture to occur must be reported by the HAL by > > * calling notify() with ERROR_REQUEST. Individual errors for the result > > * metadata or the output buffers must not be reported in this case. > > > > I think the > > > > abortRequest(descriptor); > > completeDescriptor(descriptor); > > > > sequence should be kept here ? > > Not really. Perhaps we should update `camera3.h`, while this is what I > wanted to discuss with you in the previous version of this patch: > > CAMERA3_MSG_ERROR_REQUEST is mostly used for requests that haven't > done any processing. > CAMERA3_MSG_ERROR_RESULT is used for requests that have missing metadata. > > A request in error status doesn't fall into either of the cases. > > If you can take another look at my last comments in `[PATCH v2 6/9] > android: Cleanup CAMERA3_MSG_ERROR_REQUEST`, we can align and discuss > how to proceed here. Thanks :) > > Ref: https://source.android.com/reference/hal/structcamera3__device__ops > > > > > Specifically, if you don't call completeDescriptor() the descriptor > > will remain in pending state and stall the queue of complete > > descriptors > > As we don't return in this if clause, either there is at least one > buffer to be post-processed, or completeDescriptor() will be called on > the descriptor. Let me know if I misunderstand anything. > > > > > > > > + descriptor->status_ = Camera3RequestDescriptor::Status::Error; > > > } > > > > > > /* > > > @@ -1239,7 +1236,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; > > > > To be honest I don't see what we gain by delaying the notification > > We can also remove the logic entirely: getResultMetadata() only fails > to return a valid CameraMetadata with memory issues I think, not due > to some corrupted Request::metadata(). It will be removed in the end > anyways. > > WDYT? > > > > > > > > > > > /* > > > * The camera framework expects an empty metadata pack on error. > > > @@ -1325,6 +1322,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); > > > > If my understanding of the specs if correct, I think in case of > > > > if (request->status() != Request::RequestComplete) { > > > > the right error code to notify is ERROR_REQUEST, so if my > > understanding is correct, I would rather drop this whole change. > > > > Or am I mistaken maybe ? Have you got any failure in CTS or other > > tests because of this ? > > Yeah that's what I wanted to fix with this patch: > ``` > 3.4 For captures that will produce some results, the HAL must not call > CAMERA3_MSG_ERROR_REQUEST, since that indicates complete failure. > ``` > > Previously, we just ignored any potentially available metadata and > buffers and notified CAMERA3_MSG_ERROR_REQUEST on a request in error > status. This doesn't seem correct to me, while I believe it still fits > in the Android API. > > Regarding failure in CTS or other tests, I just tried on mtkisp7. > Without this patch, CTS still passes. I think it should be fine > without this patch, if it's controversial. Sorry, I take it back. I believe mtkisp7 passed CTS because we didn't produce any failure. In Android: ``` For captures that will produce some results, the HAL must not call CAMERA3_MSG_ERROR_REQUEST, since that indicates complete failure. ``` With partial results, it cannot be guaranteed that there was no buffer / metadata completed and sent to the application before we receive the requestCompleted signal. We have to deal with this issue before the partial result patches land. BR, Harvey > > BR, > Harvey > > > > > > } > > > } > > > > > > -- > > > 2.47.0.338.g60cca15819-goog > > >
Hi Harvey On Mon, Dec 09, 2024 at 06:27:13PM +0800, Cheng-Hao Yang wrote: > Hi Jacopo, > > On Mon, Dec 9, 2024 at 6:03 PM Cheng-Hao Yang <chenghaoyang@chromium.org> wrote: > > > > Hi Jacopo, > > > > On Fri, Dec 6, 2024 at 12:39 AM Jacopo Mondi > > <jacopo.mondi@ideasonboard.com> wrote: > > > > > > Hi Harvey > > > > > > On Wed, Dec 04, 2024 at 04:36:30PM +0000, Harvey Yang wrote: > > > > According to Android Camera API v3.2, CAMERA3_MSG_ERROR_REQUEST is used > > > > for requests that have not done any processing. When a request is > > > > completed with failure, CAMERA3_MSG_ERROR_RESULT should be used instead. > > > > > > > > To avoid code duplication, when CameraMetadata cannot be generated, > > > > CAMERA3_MSG_ERROR_RESULT is notified after process_capture_result. > > > > > > > > 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 | 17 ++++++++++++----- > > > > 1 file changed, 12 insertions(+), 5 deletions(-) > > > > > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > > > index 497d363d6..dd2c603e0 100644 > > > > --- a/src/android/camera_device.cpp > > > > +++ b/src/android/camera_device.cpp > > > > @@ -1211,10 +1211,7 @@ void CameraDevice::requestComplete(Request *request) > > > > > > You should update the comment before the if() > > > > Updated. Please take another look. > > > > > > > > > << " not successfully completed: " > > > > << request->status(); > > > > > > > > - abortRequest(descriptor); > > > > - completeDescriptor(descriptor); > > > > - > > > > - return; > > > > > > Is it ok to continue and try to fetch metadata from a Request in error > > > status ? > > > > I think so, as libcamera core library sets a Request in error status > > if and only if there's a buffer in error status. There might be some > > valid metadata in the Request. > > I guess the major takeaway here is that the libcamera error notification is really not sophisticated enough. > > > > > > Also, the > > > > > > if (request->status() != Request::RequestComplete) { > > > > > > case, doesn't fall in > > > > > > /** > > > * S6. Error management: > > > * > > > > > > ... > > > > > > * - The failure of an entire capture to occur must be reported by the HAL by > > > * calling notify() with ERROR_REQUEST. Individual errors for the result > > > * metadata or the output buffers must not be reported in this case. > > > > > > I think the > > > > > > abortRequest(descriptor); > > > completeDescriptor(descriptor); > > > > > > sequence should be kept here ? > > > > Not really. Perhaps we should update `camera3.h`, while this is what I > > wanted to discuss with you in the previous version of this patch: > > > > CAMERA3_MSG_ERROR_REQUEST is mostly used for requests that haven't > > done any processing. > > CAMERA3_MSG_ERROR_RESULT is used for requests that have missing metadata. > > > > A request in error status doesn't fall into either of the cases. > > Right > > If you can take another look at my last comments in `[PATCH v2 6/9] > > android: Cleanup CAMERA3_MSG_ERROR_REQUEST`, we can align and discuss > > how to proceed here. Thanks :) > > > > Ref: https://source.android.com/reference/hal/structcamera3__device__ops > > Yes, sorry I missed that and jumped on this version. I see your point: The RequestCancelled state doesn't tell us nothing about metadata, but only that some buffer has failed. However, this means some processing has been done, and you're right this should be transmitted to Android with an ERROR_RESULT code > > > > > > Specifically, if you don't call completeDescriptor() the descriptor > > > will remain in pending state and stall the queue of complete > > > descriptors > > > > As we don't return in this if clause, either there is at least one > > buffer to be post-processed, or completeDescriptor() will be called on > > the descriptor. Let me know if I misunderstand anything. This is the part that scary me a bit. When we try to post-process we don't check for the source buffer validity, am I wrong ? I would be fine with your approach, but if we continue processing the Request even if it's in Cancelled state, then any access to the buffers should be checked and handled correctly ? > > > > > > > > > > > > + descriptor->status_ = Camera3RequestDescriptor::Status::Error; > > > > } > > > > > > > > /* > > > > @@ -1239,7 +1236,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; > > > > > > To be honest I don't see what we gain by delaying the notification > > > > We can also remove the logic entirely: getResultMetadata() only fails > > to return a valid CameraMetadata with memory issues I think, not due > > to some corrupted Request::metadata(). It will be removed in the end Well, failure to reserve space is a metadata failure, isn't it ? > > anyways. > > > > WDYT? > > Thing is, to post-process you need metadata, don't you ? Is it worth trying to post-process without metadata ? > > > > > > > > > > > > > > /* > > > > * The camera framework expects an empty metadata pack on error. > > > > @@ -1325,6 +1322,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); > > > > > > If my understanding of the specs if correct, I think in case of > > > > > > if (request->status() != Request::RequestComplete) { > > > > > > the right error code to notify is ERROR_REQUEST, so if my > > > understanding is correct, I would rather drop this whole change. > > > > > > Or am I mistaken maybe ? Have you got any failure in CTS or other > > > tests because of this ? > > > > Yeah that's what I wanted to fix with this patch: > > ``` > > 3.4 For captures that will produce some results, the HAL must not call > > CAMERA3_MSG_ERROR_REQUEST, since that indicates complete failure. > > ``` > > > > Previously, we just ignored any potentially available metadata and > > buffers and notified CAMERA3_MSG_ERROR_REQUEST on a request in error > > status. This doesn't seem correct to me, while I believe it still fits > > in the Android API. Ok, I'll take this as we don't have expressive enough failures in libcamera. Error handling paths are particularly hard to test, that's why I'm so cautious here, before this patch we considered RequestCancelled == Complete Request Failure and we didn't deliver anything to the framework for this request. Moving forward some metadata might be notified to the framework before requestComplete() so ERROR_REQUEST is not the right code anymore. I'm fine with this (now that I understand it) but please let me know what do you think about checking the source buffer status when doing post-processing. > > > > Regarding failure in CTS or other tests, I just tried on mtkisp7. > > Without this patch, CTS still passes. I think it should be fine > > without this patch, if it's controversial. > > Sorry, I take it back. I believe mtkisp7 passed CTS because we didn't > produce any failure. Yes, failure path are particularly hard to test, I know... > > In Android: > ``` > For captures that will produce some results, the HAL must not call > CAMERA3_MSG_ERROR_REQUEST, since that indicates complete failure. > ``` > > With partial results, it cannot be guaranteed that there was no buffer > / metadata completed and sent to the application before we receive the > requestCompleted signal. We have to deal with this issue before the > partial result patches land. I agree. Thanks for explaining, let me know what you think about the last missing item (checking src buffer state when post-processing) Thanks j > > BR, > Harvey > > > > > BR, > > Harvey > > > > > > > > > } > > > > } > > > > > > > > -- > > > > 2.47.0.338.g60cca15819-goog > > > >
Hi Jacopo, On Tue, Dec 10, 2024 at 4:29 PM Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote: > > Hi Harvey > > On Mon, Dec 09, 2024 at 06:27:13PM +0800, Cheng-Hao Yang wrote: > > Hi Jacopo, > > > > On Mon, Dec 9, 2024 at 6:03 PM Cheng-Hao Yang <chenghaoyang@chromium.org> wrote: > > > > > > Hi Jacopo, > > > > > > On Fri, Dec 6, 2024 at 12:39 AM Jacopo Mondi > > > <jacopo.mondi@ideasonboard.com> wrote: > > > > > > > > Hi Harvey > > > > > > > > On Wed, Dec 04, 2024 at 04:36:30PM +0000, Harvey Yang wrote: > > > > > According to Android Camera API v3.2, CAMERA3_MSG_ERROR_REQUEST is used > > > > > for requests that have not done any processing. When a request is > > > > > completed with failure, CAMERA3_MSG_ERROR_RESULT should be used instead. > > > > > > > > > > To avoid code duplication, when CameraMetadata cannot be generated, > > > > > CAMERA3_MSG_ERROR_RESULT is notified after process_capture_result. > > > > > > > > > > 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 | 17 ++++++++++++----- > > > > > 1 file changed, 12 insertions(+), 5 deletions(-) > > > > > > > > > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp > > > > > index 497d363d6..dd2c603e0 100644 > > > > > --- a/src/android/camera_device.cpp > > > > > +++ b/src/android/camera_device.cpp > > > > > @@ -1211,10 +1211,7 @@ void CameraDevice::requestComplete(Request *request) > > > > > > > > You should update the comment before the if() > > > > > > Updated. Please take another look. > > > > > > > > > > > > << " not successfully completed: " > > > > > << request->status(); > > > > > > > > > > - abortRequest(descriptor); > > > > > - completeDescriptor(descriptor); > > > > > - > > > > > - return; > > > > > > > > Is it ok to continue and try to fetch metadata from a Request in error > > > > status ? > > > > > > I think so, as libcamera core library sets a Request in error status > > > if and only if there's a buffer in error status. There might be some > > > valid metadata in the Request. > > > > > I guess the major takeaway here is that the libcamera error > notification is really not sophisticated enough. Exactly :) > > > > > > > > > Also, the > > > > > > > > if (request->status() != Request::RequestComplete) { > > > > > > > > case, doesn't fall in > > > > > > > > /** > > > > * S6. Error management: > > > > * > > > > > > > > ... > > > > > > > > * - The failure of an entire capture to occur must be reported by the HAL by > > > > * calling notify() with ERROR_REQUEST. Individual errors for the result > > > > * metadata or the output buffers must not be reported in this case. > > > > > > > > I think the > > > > > > > > abortRequest(descriptor); > > > > completeDescriptor(descriptor); > > > > > > > > sequence should be kept here ? > > > > > > Not really. Perhaps we should update `camera3.h`, while this is what I > > > wanted to discuss with you in the previous version of this patch: > > > > > > CAMERA3_MSG_ERROR_REQUEST is mostly used for requests that haven't > > > done any processing. > > > CAMERA3_MSG_ERROR_RESULT is used for requests that have missing metadata. > > > > > > A request in error status doesn't fall into either of the cases. > > > > > Right > > > > If you can take another look at my last comments in `[PATCH v2 6/9] > > > android: Cleanup CAMERA3_MSG_ERROR_REQUEST`, we can align and discuss > > > how to proceed here. Thanks :) > > > > > > Ref: https://source.android.com/reference/hal/structcamera3__device__ops > > > > > Yes, sorry I missed that and jumped on this version. > > I see your point: The RequestCancelled state doesn't tell us nothing > about metadata, but only that some buffer has failed. > > However, this means some processing has been done, and you're right > this should be transmitted to Android with an ERROR_RESULT code > > > > > > > > > Specifically, if you don't call completeDescriptor() the descriptor > > > > will remain in pending state and stall the queue of complete > > > > descriptors > > > > > > As we don't return in this if clause, either there is at least one > > > buffer to be post-processed, or completeDescriptor() will be called on > > > the descriptor. Let me know if I misunderstand anything. > > This is the part that scary me a bit. > > When we try to post-process we don't check for the source buffer > validity, am I wrong ? I would be fine with your approach, but if we > continue processing the Request even if it's in Cancelled state, then > any access to the buffers should be checked and handled correctly ? Ah, thanks for the catch! I think we need to set the buffers that depend on a cancelled source buffer to be StreamBuffer::Status::Error as well. > > > > > > > > > > > > > > > > > + descriptor->status_ = Camera3RequestDescriptor::Status::Error; > > > > > } > > > > > > > > > > /* > > > > > @@ -1239,7 +1236,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; > > > > > > > > To be honest I don't see what we gain by delaying the notification > > > > > > We can also remove the logic entirely: getResultMetadata() only fails > > > to return a valid CameraMetadata with memory issues I think, not due > > > to some corrupted Request::metadata(). It will be removed in the end > > Well, failure to reserve space is a metadata failure, isn't it ? Well... fair... Delaying the notification is just to simplify the logic, as it might happen along with a cancelled request as well, so that we don't send the signal twice. > > > > anyways. > > > > > > WDYT? > > > > > Thing is, to post-process you need metadata, don't you ? Is it worth > trying to post-process without metadata ? I think it's a different thing: Failing to allocate memory for CameraMetadata doesn't mean that there are some metadata missing during the processing of the pipeline handler. True that currently JPEG post processor reads from `resultMetadata` to fill in jpeg metadata, while that might not always be the case. In the following patches, we'll provide some metadata with the new signal, right? As long as the current implementation of post processors don't fall into fatal errors, I think we can keep it this way. WDYT? (The original logic also allows post-processing when `getResultMetadata()` fails, doesn't it? > > > > > > > > > > > > > > > > > > > /* > > > > > * The camera framework expects an empty metadata pack on error. > > > > > @@ -1325,6 +1322,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); > > > > > > > > If my understanding of the specs if correct, I think in case of > > > > > > > > if (request->status() != Request::RequestComplete) { > > > > > > > > the right error code to notify is ERROR_REQUEST, so if my > > > > understanding is correct, I would rather drop this whole change. > > > > > > > > Or am I mistaken maybe ? Have you got any failure in CTS or other > > > > tests because of this ? > > > > > > Yeah that's what I wanted to fix with this patch: > > > ``` > > > 3.4 For captures that will produce some results, the HAL must not call > > > CAMERA3_MSG_ERROR_REQUEST, since that indicates complete failure. > > > ``` > > > > > > Previously, we just ignored any potentially available metadata and > > > buffers and notified CAMERA3_MSG_ERROR_REQUEST on a request in error > > > status. This doesn't seem correct to me, while I believe it still fits > > > in the Android API. > > Ok, I'll take this as we don't have expressive enough failures in > libcamera. Error handling paths are particularly hard to test, that's > why I'm so cautious here, before this patch we considered > RequestCancelled == Complete Request Failure and we didn't deliver > anything to the framework for this request. > > Moving forward some metadata might be notified to the framework before > requestComplete() so ERROR_REQUEST is not the right code anymore. > > I'm fine with this (now that I understand it) but please let me know > what do you think about checking the source buffer status when doing > post-processing. > > > > > > > Regarding failure in CTS or other tests, I just tried on mtkisp7. > > > Without this patch, CTS still passes. I think it should be fine > > > without this patch, if it's controversial. > > > > Sorry, I take it back. I believe mtkisp7 passed CTS because we didn't > > produce any failure. > > Yes, failure path are particularly hard to test, I know... > > > > > > In Android: > > ``` > > For captures that will produce some results, the HAL must not call > > CAMERA3_MSG_ERROR_REQUEST, since that indicates complete failure. > > ``` > > > > With partial results, it cannot be guaranteed that there was no buffer > > / metadata completed and sent to the application before we receive the > > requestCompleted signal. We have to deal with this issue before the > > partial result patches land. > > I agree. Thanks for explaining, let me know what you think about the > last missing item (checking src buffer state when post-processing) Yes, I'll upload a fix for that in the next version. BR, Harvey > > Thanks > j > > > > > BR, > > Harvey > > > > > > > > BR, > > > Harvey > > > > > > > > > > > > } > > > > > } > > > > > > > > > > -- > > > > > 2.47.0.338.g60cca15819-goog > > > > >
diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp index 497d363d6..dd2c603e0 100644 --- a/src/android/camera_device.cpp +++ b/src/android/camera_device.cpp @@ -1211,10 +1211,7 @@ void CameraDevice::requestComplete(Request *request) << " not successfully completed: " << request->status(); - abortRequest(descriptor); - completeDescriptor(descriptor); - - return; + descriptor->status_ = Camera3RequestDescriptor::Status::Error; } /* @@ -1239,7 +1236,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 +1322,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); } }