[RFC,v1,16/27] libcamera: pipeline_handler: completeBuffer(): Inline and `static`
diff mbox series

Message ID 20260618123844.656396-17-barnabas.pocze@ideasonboard.com
State Superseded
Headers show
Series
  • Misc. changes before request-buffer split
Related show

Commit Message

Barnabás Pőcze June 18, 2026, 12:38 p.m. UTC 
The function now only calls a method in `Request::Private`, so inline
it, and also make it `static` since it needs no access to the pipeline
handler members.

Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
---
 include/libcamera/internal/pipeline_handler.h | 7 ++++++-
 src/libcamera/pipeline_handler.cpp            | 5 +----
 2 files changed, 7 insertions(+), 5 deletions(-)

Comments

Jacopo Mondi June 22, 2026, 1:05 p.m. UTC | #1 
Pipeline handlers have access to Request::Private. if you make
PipelineHandler::completeBuffer() just a call wrapper, why should they
call it ?

I would drop these two patches honestly

On Thu, Jun 18, 2026 at 02:38:33PM +0200, Barnabás Pőcze wrote:
> The function now only calls a method in `Request::Private`, so inline
> it, and also make it `static` since it needs no access to the pipeline
> handler members.
>
> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> ---
>  include/libcamera/internal/pipeline_handler.h | 7 ++++++-
>  src/libcamera/pipeline_handler.cpp            | 5 +----
>  2 files changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h
> index 6922ce18ec..cc52c6b045 100644
> --- a/include/libcamera/internal/pipeline_handler.h
> +++ b/include/libcamera/internal/pipeline_handler.h
> @@ -19,6 +19,7 @@
>
>  #include "libcamera/internal/camera_manager.h"
>  #include "libcamera/internal/ipa_manager.h"
> +#include "libcamera/internal/request.h"
>
>  namespace libcamera {
>
> @@ -60,7 +61,11 @@ public:
>  	void registerRequest(Request *request);
>  	void queueRequest(Request *request);
>
> -	bool completeBuffer(Request *request, FrameBuffer *buffer);
> +	static bool completeBuffer(Request *request, FrameBuffer *buffer)
> +	{
> +		return request->_d()->completeBuffer(buffer);
> +	}
> +
>  	void completeRequest(Request *request);
>  	void cancelRequest(Request *request);
>
> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> index 3d49f85cfa..99f35d1e42 100644
> --- a/src/libcamera/pipeline_handler.cpp
> +++ b/src/libcamera/pipeline_handler.cpp
> @@ -544,6 +544,7 @@ void PipelineHandler::doQueueRequests(Camera *camera)
>   */
>
>  /**
> + * \fn PipelineHandler::completeBuffer(Request *request, FrameBuffer *buffer)
>   * \brief Complete a buffer for a request
>   * \param[in] request The request the buffer belongs to
>   * \param[in] buffer The buffer that has completed
> @@ -560,10 +561,6 @@ void PipelineHandler::doQueueRequests(Camera *camera)
>   * \return True if all buffers contained in the request have completed, false
>   * otherwise
>   */
> -bool PipelineHandler::completeBuffer(Request *request, FrameBuffer *buffer)
> -{
> -	return request->_d()->completeBuffer(buffer);
> -}
>
>  /**
>   * \brief Signal request completion
> --
> 2.54.0
>
Laurent Pinchart June 23, 2026, 9:39 p.m. UTC | #2 
On Mon, Jun 22, 2026 at 03:05:05PM +0200, Jacopo Mondi wrote:
> Pipeline handlers have access to Request::Private. if you make
> PipelineHandler::completeBuffer() just a call wrapper, why should they
> call it ?

I think having completeBuffers() and completeRequest() provided by the
same class (the PipelineHandler class) make sense. Calling

	request->_d()->completeBuffer()

directly from pipeline handlers seems a tiny bit less readable to me.
But that may of course be because I'm used to the current code base.

> I would drop these two patches honestly

I like 15/27. I wouldn't have gone as far as making
PipelineHandler::completeBuffer() static (as we may need to undo that
later), but I don't mind either way. I'll leave the decision to
Barnabás.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> On Thu, Jun 18, 2026 at 02:38:33PM +0200, Barnabás Pőcze wrote:
> > The function now only calls a method in `Request::Private`, so inline
> > it, and also make it `static` since it needs no access to the pipeline
> > handler members.
> >
> > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> > ---
> >  include/libcamera/internal/pipeline_handler.h | 7 ++++++-
> >  src/libcamera/pipeline_handler.cpp            | 5 +----
> >  2 files changed, 7 insertions(+), 5 deletions(-)
> >
> > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h
> > index 6922ce18ec..cc52c6b045 100644
> > --- a/include/libcamera/internal/pipeline_handler.h
> > +++ b/include/libcamera/internal/pipeline_handler.h
> > @@ -19,6 +19,7 @@
> >
> >  #include "libcamera/internal/camera_manager.h"
> >  #include "libcamera/internal/ipa_manager.h"
> > +#include "libcamera/internal/request.h"
> >
> >  namespace libcamera {
> >
> > @@ -60,7 +61,11 @@ public:
> >  	void registerRequest(Request *request);
> >  	void queueRequest(Request *request);
> >
> > -	bool completeBuffer(Request *request, FrameBuffer *buffer);
> > +	static bool completeBuffer(Request *request, FrameBuffer *buffer)
> > +	{
> > +		return request->_d()->completeBuffer(buffer);
> > +	}
> > +
> >  	void completeRequest(Request *request);
> >  	void cancelRequest(Request *request);
> >
> > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> > index 3d49f85cfa..99f35d1e42 100644
> > --- a/src/libcamera/pipeline_handler.cpp
> > +++ b/src/libcamera/pipeline_handler.cpp
> > @@ -544,6 +544,7 @@ void PipelineHandler::doQueueRequests(Camera *camera)
> >   */
> >
> >  /**
> > + * \fn PipelineHandler::completeBuffer(Request *request, FrameBuffer *buffer)
> >   * \brief Complete a buffer for a request
> >   * \param[in] request The request the buffer belongs to
> >   * \param[in] buffer The buffer that has completed
> > @@ -560,10 +561,6 @@ void PipelineHandler::doQueueRequests(Camera *camera)
> >   * \return True if all buffers contained in the request have completed, false
> >   * otherwise
> >   */
> > -bool PipelineHandler::completeBuffer(Request *request, FrameBuffer *buffer)
> > -{
> > -	return request->_d()->completeBuffer(buffer);
> > -}
> >
> >  /**
> >   * \brief Signal request completion
Barnabás Pőcze June 26, 2026, 3:44 p.m. UTC | #3 
2026. 06. 23. 23:39 keltezéssel, Laurent Pinchart írta:
> On Mon, Jun 22, 2026 at 03:05:05PM +0200, Jacopo Mondi wrote:
>> Pipeline handlers have access to Request::Private. if you make
>> PipelineHandler::completeBuffer() just a call wrapper, why should they
>> call it ?
> 
> I think having completeBuffers() and completeRequest() provided by the
> same class (the PipelineHandler class) make sense. Calling
> 
> 	request->_d()->completeBuffer()
> 
> directly from pipeline handlers seems a tiny bit less readable to me.

Yes, I don't like that, and wanted to keep the changes small.


> But that may of course be because I'm used to the current code base.
> 
>> I would drop these two patches honestly
> 
> I like 15/27. I wouldn't have gone as far as making
> PipelineHandler::completeBuffer() static (as we may need to undo that
> later), but I don't mind either way. I'll leave the decision to
> Barnabás.
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
>> On Thu, Jun 18, 2026 at 02:38:33PM +0200, Barnabás Pőcze wrote:
>>> The function now only calls a method in `Request::Private`, so inline
>>> it, and also make it `static` since it needs no access to the pipeline
>>> handler members.
>>>
>>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
>>> ---
>>>   include/libcamera/internal/pipeline_handler.h | 7 ++++++-
>>>   src/libcamera/pipeline_handler.cpp            | 5 +----
>>>   2 files changed, 7 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h
>>> index 6922ce18ec..cc52c6b045 100644
>>> --- a/include/libcamera/internal/pipeline_handler.h
>>> +++ b/include/libcamera/internal/pipeline_handler.h
>>> @@ -19,6 +19,7 @@
>>>
>>>   #include "libcamera/internal/camera_manager.h"
>>>   #include "libcamera/internal/ipa_manager.h"
>>> +#include "libcamera/internal/request.h"
>>>
>>>   namespace libcamera {
>>>
>>> @@ -60,7 +61,11 @@ public:
>>>   	void registerRequest(Request *request);
>>>   	void queueRequest(Request *request);
>>>
>>> -	bool completeBuffer(Request *request, FrameBuffer *buffer);
>>> +	static bool completeBuffer(Request *request, FrameBuffer *buffer)
>>> +	{
>>> +		return request->_d()->completeBuffer(buffer);
>>> +	}
>>> +
>>>   	void completeRequest(Request *request);
>>>   	void cancelRequest(Request *request);
>>>
>>> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
>>> index 3d49f85cfa..99f35d1e42 100644
>>> --- a/src/libcamera/pipeline_handler.cpp
>>> +++ b/src/libcamera/pipeline_handler.cpp
>>> @@ -544,6 +544,7 @@ void PipelineHandler::doQueueRequests(Camera *camera)
>>>    */
>>>
>>>   /**
>>> + * \fn PipelineHandler::completeBuffer(Request *request, FrameBuffer *buffer)
>>>    * \brief Complete a buffer for a request
>>>    * \param[in] request The request the buffer belongs to
>>>    * \param[in] buffer The buffer that has completed
>>> @@ -560,10 +561,6 @@ void PipelineHandler::doQueueRequests(Camera *camera)
>>>    * \return True if all buffers contained in the request have completed, false
>>>    * otherwise
>>>    */
>>> -bool PipelineHandler::completeBuffer(Request *request, FrameBuffer *buffer)
>>> -{
>>> -	return request->_d()->completeBuffer(buffer);
>>> -}
>>>
>>>   /**
>>>    * \brief Signal request completion
>

Patch
diff mbox series 

diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h
index 6922ce18ec..cc52c6b045 100644
--- a/include/libcamera/internal/pipeline_handler.h
+++ b/include/libcamera/internal/pipeline_handler.h
@@ -19,6 +19,7 @@ 
 
 #include "libcamera/internal/camera_manager.h"
 #include "libcamera/internal/ipa_manager.h"
+#include "libcamera/internal/request.h"
 
 namespace libcamera {
 
@@ -60,7 +61,11 @@  public:
 	void registerRequest(Request *request);
 	void queueRequest(Request *request);
 
-	bool completeBuffer(Request *request, FrameBuffer *buffer);
+	static bool completeBuffer(Request *request, FrameBuffer *buffer)
+	{
+		return request->_d()->completeBuffer(buffer);
+	}
+
 	void completeRequest(Request *request);
 	void cancelRequest(Request *request);
 
diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
index 3d49f85cfa..99f35d1e42 100644
--- a/src/libcamera/pipeline_handler.cpp
+++ b/src/libcamera/pipeline_handler.cpp
@@ -544,6 +544,7 @@  void PipelineHandler::doQueueRequests(Camera *camera)
  */
 
 /**
+ * \fn PipelineHandler::completeBuffer(Request *request, FrameBuffer *buffer)
  * \brief Complete a buffer for a request
  * \param[in] request The request the buffer belongs to
  * \param[in] buffer The buffer that has completed
@@ -560,10 +561,6 @@  void PipelineHandler::doQueueRequests(Camera *camera)
  * \return True if all buffers contained in the request have completed, false
  * otherwise
  */
-bool PipelineHandler::completeBuffer(Request *request, FrameBuffer *buffer)
-{
-	return request->_d()->completeBuffer(buffer);
-}
 
 /**
  * \brief Signal request completion