[PATCH v3 5/7] android: Drop notify CAMERA3_MSG_ERROR_REQUEST when a request fails

[Cheng-Hao Yang]

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.

>
> 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
> >