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

[Jacopo Mondi]

Hi Harvey

On Tue, Dec 10, 2024 at 01:40:14PM +0800, Cheng-Hao Yang wrote:
> 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.

Yes, you're right there even is a comment here

        /*
         * Pass the buffer fence back to the camera framework as
         * a release fence. This instructs the framework to wait
         * on the acquire fence in case we haven't done so
         * ourselves for any reason.
         */

And I have checked the call stack and confirmed that once a Fence is
waited on by the library, it is reset to -1 so the Android specs
should be respected here.

>
> BR,
> Harvey
>
>
> >
> > The patch itself seems good to me
> > Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>

Still holds indeed

Thanks
  j

> >
> > >               return 0;
> > >       }
> > >
> > > --
> > > 2.47.0.338.g60cca15819-goog
> > >