[PATCH v3 5/7] android: Drop notify CAMERA3_MSG_ERROR_REQUEST when a request fails
Jacopo Mondi
jacopo.mondi at ideasonboard.com
Tue Dec 10 09:29:28 CET 2024
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 at chromium.org> wrote:
> >
> > Hi Jacopo,
> >
> > On Fri, Dec 6, 2024 at 12:39 AM Jacopo Mondi
> > <jacopo.mondi at 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 at chromium.org>
> > > > Co-developed-by: Harvey Yang <chenghaoyang at chromium.org>
> > > > Signed-off-by: Harvey Yang <chenghaoyang at chromium.org>
> > > > ---
> > > > src/android/camera_device.cpp | 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
> > > >
More information about the libcamera-devel
mailing list