[PATCH v3 6/7] android: notify CAMERA3_MSG_ERROR_REQUEST out of order

[Cheng-Hao Yang]

Hi Jacopo,

On Fri, Dec 6, 2024 at 12:46 AM Jacopo Mondi
<jacopo.mondi at ideasonboard.com> wrote:
>
> On Wed, Dec 04, 2024 at 04:36:31PM +0000, Harvey Yang wrote:
> > When a request hasn't done any processing, CAMERA3_MSG_ERROR_REQUEST and
> > the following process_capture_result don't need to wait for the previous
> > requests' completion. This patch avoids pushing the aborted requests
> > into the request queue.
> >
> > 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 | 11 +++--------
> >  1 file changed, 3 insertions(+), 8 deletions(-)
> >
> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > index dd2c603e0..18e974f56 100644
> > --- a/src/android/camera_device.cpp
> > +++ b/src/android/camera_device.cpp
> > @@ -867,6 +867,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());
>
> Right, if we don't have to complete in order this indeed seems
> correct.
>
> However, we fall in this case, right ?
>
>      * 2. For pending requests that have not done any processing, the HAL must call notify
>      *    CAMERA3_MSG_ERROR_REQUEST, and return all the output buffers with
>      *    process_capture_result in the error state (CAMERA3_BUFFER_STATUS_ERROR).
>      *    The HAL must not place the release fence into an error state, instead,
>      *    the release fences must be set to the acquire fences passed by the framework,
>      *    or -1 if they have been waited on by the HAL already. This is also the path
>      *    to follow for any captures for which the HAL already called notify() with
>      *    CAMERA3_MSG_SHUTTER but won't be producing any metadata/valid buffers for.
>      *    After CAMERA3_MSG_ERROR_REQUEST, for a given frame, only process_capture_results with
>      *    buffers in CAMERA3_BUFFER_STATUS_ERROR are allowed. No further notifys or
>      *    process_capture_result with non-null metadata is allowed.
>
> Apparently we're not handling fences here. Not your fault, seems like
> it wasn't there already. While at it could you add a \todo or, if you
> feel like it's worth fixing it, pile up a patch ?

Yes we fall into this case, while I think the original logic already
handles the fence as described?
In `CameraDevice::sendCaptureResults()` (or
`CameraDevice::sendCaptureResult()` in my patches),
`buffer.fence.release()` is put in the `release_fence` field of
`camera3_steram_buffer_t`.

Please check if I missed anything.

BR,
Harvey


>
> The patch itself seems good to me
> Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
>
> >               return 0;
> >       }
> >
> > --
> > 2.47.0.338.g60cca15819-goog
> >