[libcamera-devel] [PATCH v2 2/3] libcamera: Request: Add RequestError to Status
Hirokazu Honda
hiroh at chromium.org
Tue Mar 30 08:08:46 CEST 2021
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
More information about the libcamera-devel
mailing list