[libcamera-devel] [PATCH v2 2/3] libcamera: Request: Add RequestError to Status

[Hirokazu Honda]

Hi Jacopo, thanks for reviewing and comments.


On Mon, Mar 29, 2021 at 6:40 PM Jacopo Mondi <jacopo at jmondi.org> wrote:
>
> Hi Hiro,
>
> On Mon, Mar 29, 2021 at 06:15:51PM +0900, Hirokazu Honda wrote:
> > This adds a new libcamera::Request::Status, RequestError, which
> > represents a request isn't successfully processed due to some
> > error.
> >
>
> It seems to me that RequestCancelled is now only set if the frame
> metadata are cancelled by the IPA and is signaled by the pipeline
> handlers by calling completeBuffer() and have Request detect such an
> error condition.
>
> bool Request::completeBuffer(FrameBuffer *buffer)
> {
>         LIBCAMERA_TRACEPOINT(request_complete_buffer, this, buffer);
>
>         int ret = pending_.erase(buffer);
>         ASSERT(ret == 1);
>
>         buffer->request_ = nullptr;
>
>         if (buffer->metadata().status == FrameMetadata::FrameCancelled)
>                 cancelled_ = true;
>
>         return !hasPendingBuffers();
> }
>
> The cancelled request state is then propagate to application at
> request completion
>
> void Request::complete()
> {
>         ASSERT(status_ == RequestPending);
>         ASSERT(!hasPendingBuffers());
>
>         status_ = cancelled_ ? RequestCancelled : RequestComplete;
>
>         LOG(Request, Debug)
>                 << "Request has completed - cookie: " << cookie_
>                 << (cancelled_ ? " [Cancelled]" : "");
>
>         LIBCAMERA_TRACEPOINT(request_complete, this);
> }
>
> Wouldn't it be better to keep using RequestComplete and add an error_
> field like you're doing here to be inspected by Request::complete()
> that could transport different errors ? In example
>
>         enum RequestError {
>                 MetadataCancelled,
>                 BufferUnderrun,
>                 ...
>         };
>
> And change completeBuffers() to use the new error field as well as
> PipelineHandler::queueRequest() and change Request::complete() to
> inspect the new field so that you can drop cancelled_ ?
>
> In this way a failed Request will always have status ==
> RequestCancelled (which can be changed to RequestFailed if we like it
> more) and the exact failure reason can be deduced inspecting
> Request::error() ?
>

+Laurent Pinchart
I and Laurent have discussed in the patch version 1.
The concern was the coherency of status and error values.
Adding a new status enum, RequestError, to Request::Status is the
simplest solution.
I think we should add error() function to acquire integer error code
when status()=RequestError rather than declaring a new enum
RequestError.
How do you think?

Regards,
-Hiro
> > Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> > ---
> >  include/libcamera/request.h        |  2 ++
> >  src/libcamera/pipeline_handler.cpp |  5 ++++-
> >  src/libcamera/request.cpp          | 17 +++++++++++------
> >  3 files changed, 17 insertions(+), 7 deletions(-)
> >
> > diff --git a/include/libcamera/request.h b/include/libcamera/request.h
> > index 6e5aad5f..598fcf86 100644
> > --- a/include/libcamera/request.h
> > +++ b/include/libcamera/request.h
> > @@ -30,6 +30,7 @@ public:
> >               RequestPending,
> >               RequestComplete,
> >               RequestCancelled,
> > +             RequestError,
> >       };
> >
> >       enum ReuseFlag {
> > @@ -73,6 +74,7 @@ private:
> >
> >       const uint64_t cookie_;
> >       Status status_;
> > +     bool error_;
> >       bool cancelled_;
> >  };
> >
> > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> > index eba00ed3..66326ce0 100644
> > --- a/src/libcamera/pipeline_handler.cpp
> > +++ b/src/libcamera/pipeline_handler.cpp
> > @@ -381,8 +381,11 @@ void PipelineHandler::queueRequest(Request *request)
> >       data->queuedRequests_.push_back(request);
> >
> >       int ret = queueRequestDevice(camera, request);
> > -     if (ret)
> > +     if (ret) {
> > +             request->error_ = true;
> >               data->queuedRequests_.remove(request);
> > +             completeRequest(request);
> > +     }
> >  }
> >
> >  /**
> > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
> > index 0071667e..176f59dd 100644
> > --- a/src/libcamera/request.cpp
> > +++ b/src/libcamera/request.cpp
> > @@ -36,6 +36,8 @@ LOG_DEFINE_CATEGORY(Request)
> >   * The request has completed
> >   * \var Request::RequestCancelled
> >   * The request has been cancelled due to capture stop
> > + * \var Request::RequestError
> > + * The request is not completed successfully due to an error.
> >   */
> >
> >  /**
> > @@ -73,7 +75,7 @@ LOG_DEFINE_CATEGORY(Request)
> >   */
> >  Request::Request(Camera *camera, uint64_t cookie)
> >       : camera_(camera), cookie_(cookie), status_(RequestPending),
> > -       cancelled_(false)
> > +       error_(false), cancelled_(false)
> >  {
> >       /**
> >        * \todo Should the Camera expose a validator instance, to avoid
> > @@ -126,6 +128,7 @@ void Request::reuse(ReuseFlag flags)
> >       }
> >
> >       status_ = RequestPending;
> > +     error_ = false;
> >       cancelled_ = false;
> >
> >       controls_->clear();
> > @@ -256,20 +259,22 @@ FrameBuffer *Request::findBuffer(const Stream *stream) const
> >  /**
> >   * \brief Complete a queued request
> >   *
> > - * Mark the request as complete by updating its status to RequestComplete,
> > - * unless buffers have been cancelled in which case the status is set to
> > - * RequestCancelled.
> > + * Mark the request as complete by updating its status to RequestError on error,
> > + * RequestCancelled if buffers have been cancelled, or otherwise RequestComplete.
> >   */
> >  void Request::complete()
> >  {
> >       ASSERT(status_ == RequestPending);
> >       ASSERT(!hasPendingBuffers());
> >
> > -     status_ = cancelled_ ? RequestCancelled : RequestComplete;
> > +     if (error_)
> > +             status_ = RequestError;
> > +     else
> > +             status_ = cancelled_ ? RequestCancelled : RequestComplete;
> >
> >       LOG(Request, Debug)
> >               << "Request has completed - cookie: " << cookie_
> > -             << (cancelled_ ? " [Cancelled]" : "");
> > +             << (error_ ? " [Error]" : (cancelled_ ? " [Cancelled]" : ""));
> >
> >       LIBCAMERA_TRACEPOINT(request_complete, this);
> >  }
> > --
> > 2.31.0.291.g576ba9dcdaf-goog
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel