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

[Cheng-Hao Yang]

Hi Jacopo,

On Wed, Dec 11, 2024 at 7:06 PM Jacopo Mondi
<jacopo.mondi at ideasonboard.com> wrote:
>
> Hi Harvey
>
> On Tue, Dec 10, 2024 at 02:23:58PM +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 | 40 ++++++++++++++++++++++++++++-------
> >  1 file changed, 32 insertions(+), 8 deletions(-)
> >
> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > index f3f570544..3b10f207e 100644
> > --- a/src/android/camera_device.cpp
> > +++ b/src/android/camera_device.cpp
> > @@ -1182,6 +1182,18 @@ void CameraDevice::requestComplete(Request *request)
> >        * post-processing/compression fails.
> >        */
> >       for (auto &buffer : descriptor->buffers_) {
> > +             for (auto &[_, frameBuffer] : request->buffers()) {
>
> I've been thinking about ways to avoid the double lookup here, we
> don't have that many requests or descriptor, but avoiding doing it for every
> request might be desirable.
>
> I've been trying to do so by setting a pointer to a StreamBuffer as
> libcamera::FrameBuffer::cookie and retrieve it here. However, as
> now multiple Android streams can map to the same FrameBuffer there's
> no 1-to-1 association anymore between a StreamBuffer and a
> FrameBuffer...
>
> Too bad, I would have liked to avoid this

Yeah, unless we introduce new member variables, it's hard to fix it here.

I don't think we need to worry about this too much, as the upcoming
bufferCompleted only allows source buffers to be completed one by one.

>
> > +                     if (buffer.srcBuffer != frameBuffer &&
> > +                         buffer.frameBuffer.get() != frameBuffer)
> > +                             continue;
> > +
> > +                     StreamBuffer::Status status = StreamBuffer::Status::Success;
> > +                     if (frameBuffer->metadata().status != FrameMetadata::FrameSuccess) {
> > +                             status = StreamBuffer::Status::Error;
> > +                     }
>
> No {} for single line statements

Done

>
> > +                     setBufferStatus(buffer, status);
> > +             }
> > +
> >               CameraStream *stream = buffer.stream;
>
> Can you move this after the comment block if you have to resend ?

Sure

>
> >
> >               /*
> > @@ -1198,22 +1210,18 @@ void CameraDevice::requestComplete(Request *request)
> >                       if (fence)
> >                               buffer.fence = fence->release();
> >               }
> > -             buffer.status = StreamBuffer::Status::Success;
> >       }
> >
> >       /*
> > -      * If the Request has failed, abort the request by notifying the error
> > -      * and complete the request with all buffers in error state.
> > +      * If the Request has failed, complete the request with all buffers in
> > +      * error state and notify an error result.
>
> With this patch buffers are completed indivdually in error state just
> below. I would move the below if() before inspecting the single
> buffers or even drop it completely and just rely on the above loop, after
> all if one buffer has failed the whole request has failed and you now
> set its state to Camera3RequestDescriptor::Status::Error in the above
> call to setBufferStatus()

Yeah the comment doesn't make sense anymore. Let's remove it.

>
> >        */
> >       if (request->status() != Request::RequestComplete) {
> >               LOG(HAL, Error) << "Request " << request->cookie()
> >                               << " not successfully completed: "
> >                               << request->status();
> >
> > -             abortRequest(descriptor);
> > -             completeDescriptor(descriptor);
> > -
> > -             return;
> > +             descriptor->status_ = Camera3RequestDescriptor::Status::Error;
> >       }
> >
> >       /*
> > @@ -1238,7 +1246,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.
> > @@ -1271,7 +1279,13 @@ void CameraDevice::requestComplete(Request *request)
> >                       continue;
> >               }
> >
> > +             if (buffer->status == StreamBuffer::Status::Error) {
> > +                     iter = descriptor->pendingStreamsToProcess_.erase(iter);
> > +                     continue;
> > +             }
>
> nit: could you move this before the previous if() ? There's no point
> in searching the FrameBuffer if we know the stream has failed ?

Right, done.

>
> > +
> >               ++iter;
> > +
> >               int ret = stream->process(buffer);
> >               if (ret) {
> >                       setBufferStatus(*buffer, StreamBuffer::Status::Error);
> > @@ -1324,6 +1338,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
>
> metdata and buffers ?

Sure, updated.

>
> > +              * because the capture is cancelled by the camera. Only notify
> > +              * it when the final result is sent, since Android will ignore
> > +              * the following metadata.
> > +              */
>
> I would drop "since ..."

Done

BR,
Harvey

>
> All minors, next one should be good to go
>
> Thanks
>   j
>
> > +             if (descriptor->status_ == Camera3RequestDescriptor::Status::Error)
> > +                     notifyError(descriptor->frameNumber_, nullptr, CAMERA3_MSG_ERROR_RESULT);
> >       }
> >  }
> >
> > --
> > 2.47.0.338.g60cca15819-goog
> >