[libcamera-devel] [PATCH v2 2/8] libcamera: request: Add Request::cancel()

[Jacopo Mondi]

Hi Kieran,

On Mon, May 17, 2021 at 12:53:54PM +0100, Kieran Bingham wrote:
> Hi Jacopo,
>
> On 13/05/2021 10:22, Jacopo Mondi wrote:
> > Add a cancel() function to the Request class that allows to forcefully
> > complete the request and its associated buffers in error state.
> >
> > Only pending requests can be forcefully cancelled. Enforce that
> > by asserting the request state to be RequestPending.
> >
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> >  include/libcamera/request.h |  1 +
> >  src/libcamera/request.cpp   | 30 ++++++++++++++++++++++++++++++
> >  2 files changed, 31 insertions(+)
> >
> > diff --git a/include/libcamera/request.h b/include/libcamera/request.h
> > index 4cf5ff3f7d3b..5596901ddd8e 100644
> > --- a/include/libcamera/request.h
> > +++ b/include/libcamera/request.h
> > @@ -65,6 +65,7 @@ private:
> >  	friend class PipelineHandler;
> >
> >  	void complete();
> > +	void cancel();
> >
> >  	bool completeBuffer(FrameBuffer *buffer);
> >
> > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
> > index ce2dd7b17f10..fc5e25199112 100644
> > --- a/src/libcamera/request.cpp
> > +++ b/src/libcamera/request.cpp
> > @@ -292,6 +292,36 @@ void Request::complete()
> >  	LIBCAMERA_TRACEPOINT(request_complete, this);
> >  }
> >
> > +/**
> > + * \brief Cancel a queued request
> > + *
> > + * Mark the request and its associated buffers as cancelled and complete it.
> > + *
> > + * Set the status of each buffer in the request to the frame cancelled state and
> > + * remove them from the pending buffer queue before completing the request with
> > + * error.
> > + */
> > +void Request::cancel()
> > +{
> > +	LIBCAMERA_TRACEPOINT(request_cancel, this);
>
> Have you checked that this is sufficient?
>
> Doesn't this require an addition in
>    include/libcamera/internal/tracepoints/request.tp

AH! I had no idea...

It would be nice to be able to catch this automatically, thanks for
spotting!

>
>
> > +
> > +	ASSERT(status_ == RequestPending);
> > +
> > +	/*
> > +	 * We can't simply loop and call completeBuffer() as erase() invalidates
> > +	 * pointers and iterators, so we have to manually cancel the buffer and
> > +	 * erase it from the pending buffers list.
> > +	 */
> > +	for (auto buffer = pending_.begin(); buffer != pending_.end();) {
> > +		(*buffer)->cancel();
>
> Do buffers need to notify a completion event if they're cancelled? I
> don't think they do - but I'm curious if there is a use case to be
> notified in a buffer event handler of a buffer being cancelled...

We discussed buffer completion in relation to cancel here
https://patchwork.libcamera.org/patch/12243/

Although my position at the time was that buffers should be completed
by the caller of Request::cancel() as they know better and for some
use cases it is better to let the caller handle that. However it is
also true that we're iterating on the pending_ buffers, so my argument
on partial request completion is defintely moot....

Hiro suggested to emit buffer completion in Request::cancel() would
you be in favour of that as well ? I'm not too at ease in emitting a
signal on the camera from the Request class though.

>
>
>
> > +		(*buffer)->setRequest(nullptr);
>
> Even if the buffer is cancelled, it's still associated with this request
> isn't it?
>
> Does it need to be set to null?

Nope, especially if the completion signal is emitted later. The
handler would receive a buffer with no Request set, which is
dangerous.

I'll change this function to

        for (buf : pending_) {
                buf->cancel();
                /* Is this a layer violation ? */
                camera->bufferCompleted.emit(this, buf);
                buf = pending_.erase(buffer);
        }

        cancelled_ = true;
        complete();

What do you think ?

>
>
> > +		buffer = pending_.erase(buffer);
> > +	}
> > +
> > +	cancelled_ = true;
> > +	complete();
> > +}
> > +
> >  /**
> >   * \brief Complete a buffer for the request
> >   * \param[in] buffer The buffer that has completed
> >
>
> --
> Regards
> --
> Kieran