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

[Cheng-Hao Yang]

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