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

[Laurent Pinchart]

Hi Hiro,

Thank you for the patch.

On Tue, Mar 30, 2021 at 03:08:46PM +0900, Hirokazu Honda wrote:
> On Mon, Mar 29, 2021 at 6:40 PM Jacopo Mondi wrote:
> > 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?

I've reviewed 3/3 before 2/3 and have written a lengthy reply there, so
let's discuss it there :-)

On the above topic though, I think this series, with the introduction of
RequestError and without a separate error code, is the least intrusive
way to fix the issue at hand. It is, however, a work in progress, as we
need to reconsider the design of error reporting (let's discuss this in
3/3). As that will take a bit of time, the question is whether we need
to fast-track a quick fix for this issue and rework the API on top, or
if we can wait for the full fix with the API redesign. I'd prefer the
latter to avoid introducing temporary could, but if there are reasons
why a short-term quick fix is useful, I'm not opposed to it.

Let's discuss in 3/3 and then come back to this question.

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

-- 
Regards,

Laurent Pinchart