[PATCH v2 6/9] android: Cleanup CAMERA3_MSG_ERROR_REQUEST

[Jacopo Mondi]

Hi Harvey

On Thu, Nov 28, 2024 at 11:30:15PM +0800, Cheng-Hao Yang wrote:
> 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

Where did this happen ? In flush ? I read it used to call
abortRequest() that

	notifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_REQUEST);

and used to call

void CameraDevice::completeDescriptor(Camera3RequestDescriptor *descriptor)
{
	MutexLocker lock(descriptorsMutex_);
	descriptor->complete_ = true;

	sendCaptureResults();
}

So I might have missed what do we gain with this change (as
abortRequest() has a single call place if I'm not mistaken.
Also, this new version won't set

	descriptor->complete_ = true;

as the previous call to completeDescriptor() did.

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

Ah yes, thanks, so we can't go through completeDescriptor() because it
completes in-order but you need to to go through a direct call to
sendCaptureResult().

Thanks, it helps. I would have helped even more if you have added it
to the commit message, because this

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

doesn't explain me anything and we had to go through three emails and
me trying to understand what you have done. Spending 5 more minutes to
write a proper commit message would have avoided that.

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

Is this part of the patch related to the above ? What happens here ?
Why do you delay the call to notifyError() ?

> > >       }
> > >  }
> > >
> > > --
> > > 2.47.0.338.g60cca15819-goog
> > >