[PATCH v5 1/3] libcamera: pipeline_handler: Provide cancelRequest

[Laurent Pinchart]

On Wed, Nov 06, 2024 at 09:25:41PM +0100, Milan Zamazal wrote:
> Laurent Pinchart <laurent.pinchart at ideasonboard.com> writes:
> > On Mon, Oct 21, 2024 at 03:37:16PM +0200, Milan Zamazal wrote:
> >> Let's extract the two occurrences of canceling a request to a common
> >> helper.  This is especially useful for the followup patch, which needs
> >> to cancel a request from outside.
> >> 
> >> Signed-off-by: Milan Zamazal <mzamazal at redhat.com>
> >> ---
> >>  include/libcamera/internal/pipeline_handler.h |  1 +
> >>  src/libcamera/pipeline_handler.cpp            | 23 +++++++++++++------
> >>  2 files changed, 17 insertions(+), 7 deletions(-)
> >> 
> >> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h
> >> index 0d3808036..fb28a18d0 100644
> >> --- a/include/libcamera/internal/pipeline_handler.h
> >> +++ b/include/libcamera/internal/pipeline_handler.h
> >> @@ -60,6 +60,7 @@ public:
> >>  
> >>  	bool completeBuffer(Request *request, FrameBuffer *buffer);
> >>  	void completeRequest(Request *request);
> >> +	void cancelRequest(Request *request);
> >>  
> >>  	std::string configurationFile(const std::string &subdir,
> >>  				      const std::string &name) const;
> >> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> >> index e59404691..c9cb11f0f 100644
> >> --- a/src/libcamera/pipeline_handler.cpp
> >> +++ b/src/libcamera/pipeline_handler.cpp
> >> @@ -367,9 +367,7 @@ void PipelineHandler::stop(Camera *camera)
> >>  	while (!waitingRequests_.empty()) {
> >>  		Request *request = waitingRequests_.front();
> >>  		waitingRequests_.pop();
> >> -
> >> -		request->_d()->cancel();
> >> -		completeRequest(request);
> >> +		cancelRequest(request);
> >>  	}
> >>  
> >>  	/* Make sure no requests are pending. */
> >> @@ -470,10 +468,8 @@ void PipelineHandler::doQueueRequest(Request *request)
> >>  	}
> >>  
> >>  	int ret = queueRequestDevice(camera, request);
> >> -	if (ret) {
> >> -		request->_d()->cancel();
> >> -		completeRequest(request);
> >> -	}
> >> +	if (ret)
> >> +		cancelRequest(request);
> >>  }
> >>  
> >>  /**
> >> @@ -568,6 +564,19 @@ void PipelineHandler::completeRequest(Request *request)
> >>  	}
> >>  }
> >>  
> >> +/**
> >> + * \brief Cancel request and signal its completion
> >> + * \param[in] request The request to cancel
> >> + *
> >> + * This function cancels the request in addition to its completion. The same
> >
> > "in additiona to its completion" confuses me. I would simply write
> >
> >  * This function cancels and completes the request. The same rules as for
> >  * completeRequest() apply.
> 
> Indeed, this sounds better.
> 
> > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> >
> > Although, I wonder if we could instead add a Request::Status argument to
> > completeRequest(), default it to Request::Status::RequestComplete, and
> > avoid adding a new function. I don't mind much either way.
> 
> A Request::Status argument could cause questions about possible
> Request::Status::Pending value.  Which could be avoided by using
> something like `bool cancel' argument instead but I think it's better to
> stop here and leave things as they are; other than that I also don't
> mind much either way.

I was also considering the Pending issue, and I don't like boolean
arguments much in this case, as they're not very readable. Does

	completeRequest(request, true);

cancel the request or complete it successfully ?

Let's leave things as-is.

> >> + * rules as for completeRequest() apply.
> >> + */
> >> +void PipelineHandler::cancelRequest(Request *request)
> >> +{
> >> +	request->_d()->cancel();
> >> +	completeRequest(request);
> >> +}
> >> +
> >>  /**
> >>   * \brief Retrieve the absolute path to a platform configuration file
> >>   * \param[in] subdir The pipeline handler specific subdirectory name

-- 
Regards,

Laurent Pinchart