[PATCH v2 6/9] android: Cleanup CAMERA3_MSG_ERROR_REQUEST

[Cheng-Hao Yang]

Hi Jacopo,

On Thu, Nov 28, 2024 at 11:02 PM Jacopo Mondi
<jacopo.mondi at 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 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 | 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
> >