[v2,3/8] libcamera: camera: Add indirection to Camera signal emissions
diff mbox series

Message ID 20250703114225.2074071-4-paul.elder@ideasonboard.com
State New
Headers show
Series
  • Add Layers support
Related show

Commit Message

Paul Elder July 3, 2025, 11:42 a.m. UTC 
Add an extra level of indirection when emitting signals from the Camera.
This is to facilitate the implementation of the layer system in the near
future, which will need to hook into Camera signal emissions.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>

---
No change in v2
---
 include/libcamera/internal/camera.h |  4 ++++
 src/libcamera/camera.cpp            | 23 +++++++++++++++++++++--
 src/libcamera/pipeline_handler.cpp  |  2 +-
 src/libcamera/request.cpp           |  2 +-
 4 files changed, 27 insertions(+), 4 deletions(-)

Comments

Stefan Klug July 7, 2025, 10:18 a.m. UTC | #1 
Hi Paul,

Thank you for the patch.

Quoting Paul Elder (2025-07-03 13:42:18)
> Add an extra level of indirection when emitting signals from the Camera.
> This is to facilitate the implementation of the layer system in the near
> future, which will need to hook into Camera signal emissions.
> 
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> 
> ---
> No change in v2
> ---
>  include/libcamera/internal/camera.h |  4 ++++
>  src/libcamera/camera.cpp            | 23 +++++++++++++++++++++--
>  src/libcamera/pipeline_handler.cpp  |  2 +-
>  src/libcamera/request.cpp           |  2 +-
>  4 files changed, 27 insertions(+), 4 deletions(-)
> 
> diff --git a/include/libcamera/internal/camera.h b/include/libcamera/internal/camera.h
> index 18f5c32a18e4..967d4e1693ec 100644
> --- a/include/libcamera/internal/camera.h
> +++ b/include/libcamera/internal/camera.h
> @@ -35,6 +35,10 @@ public:
>         PipelineHandler *pipe() { return pipe_.get(); }
>         const PipelineHandler *pipe() const { return pipe_.get(); }
>  
> +       void emitBufferCompleted(Request *request, FrameBuffer *buffer);
> +       void emitRequestCompleted(Request *request);
> +       void emitDisconnected();
> +
>         std::list<Request *> queuedRequests_;
>         ControlInfoMap controlInfo_;
>         ControlList properties_;
> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> index c180a5fdde93..c3e656cab671 100644
> --- a/src/libcamera/camera.cpp
> +++ b/src/libcamera/camera.cpp
> @@ -737,6 +737,25 @@ void Camera::Private::setState(State state)
>  {
>         state_.store(state, std::memory_order_release);
>  }
> +

I'm often unsure about the order of the functions in the .cpp/.h. As you
placed it before setState in the header and the functions are before
setState alphabetically also, I would probably move them before
setState().

I thought doxygen would nag the missing documentation as these functions
are public. Anyways these are pretty self-explanatory.

Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com>

> +void Camera::Private::emitBufferCompleted(Request *request, FrameBuffer *buffer)
> +{
> +       Camera *camera = _o<Camera>();
> +       camera->bufferCompleted.emit(request, buffer);
> +}
> +
> +void Camera::Private::emitRequestCompleted(Request *request)
> +{
> +       Camera *camera = _o<Camera>();
> +       camera->requestCompleted.emit(request);
> +}
> +
> +void Camera::Private::emitDisconnected()
> +{
> +       Camera *camera = _o<Camera>();
> +       camera->disconnected.emit();
> +}
> +
>  #endif /* __DOXYGEN_PUBLIC__ */
>  
>  /**
> @@ -947,7 +966,7 @@ void Camera::disconnect()
>         LOG(Camera, Debug) << "Disconnecting camera " << id();
>  
>         _d()->disconnect();
> -       disconnected.emit();
> +       _d()->emitDisconnected();
>  }
>  
>  int Camera::exportFrameBuffers(Stream *stream,
> @@ -1451,7 +1470,7 @@ void Camera::requestComplete(Request *request)
>                                   true))
>                 LOG(Camera, Fatal) << "Trying to complete a request when stopped";
>  
> -       requestCompleted.emit(request);
> +       _d()->emitRequestCompleted(request);
>  }
>  
>  } /* namespace libcamera */
> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> index d84dff3c9f19..b5faaae08d4c 100644
> --- a/src/libcamera/pipeline_handler.cpp
> +++ b/src/libcamera/pipeline_handler.cpp
> @@ -527,7 +527,7 @@ void PipelineHandler::doQueueRequests()
>  bool PipelineHandler::completeBuffer(Request *request, FrameBuffer *buffer)
>  {
>         Camera *camera = request->_d()->camera();
> -       camera->bufferCompleted.emit(request, buffer);
> +       camera->_d()->emitBufferCompleted(request, buffer);
>         return request->_d()->completeBuffer(buffer);
>  }
>  
> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
> index 7f1e11e8f913..b4ae0f41a34c 100644
> --- a/src/libcamera/request.cpp
> +++ b/src/libcamera/request.cpp
> @@ -137,7 +137,7 @@ void Request::Private::doCancelRequest()
>  
>         for (FrameBuffer *buffer : pending_) {
>                 buffer->_d()->cancel();
> -               camera_->bufferCompleted.emit(request, buffer);
> +               camera_->_d()->emitBufferCompleted(request, buffer);
>         }
>  
>         cancelled_ = true;
> -- 
> 2.47.2
>
Paul Elder July 23, 2025, 7:59 a.m. UTC | #2 
Quoting Stefan Klug (2025-07-07 19:18:26)
> Hi Paul,
> 
> Thank you for the patch.
> 
> Quoting Paul Elder (2025-07-03 13:42:18)
> > Add an extra level of indirection when emitting signals from the Camera.
> > This is to facilitate the implementation of the layer system in the near
> > future, which will need to hook into Camera signal emissions.
> > 
> > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > 
> > ---
> > No change in v2
> > ---
> >  include/libcamera/internal/camera.h |  4 ++++
> >  src/libcamera/camera.cpp            | 23 +++++++++++++++++++++--
> >  src/libcamera/pipeline_handler.cpp  |  2 +-
> >  src/libcamera/request.cpp           |  2 +-
> >  4 files changed, 27 insertions(+), 4 deletions(-)
> > 
> > diff --git a/include/libcamera/internal/camera.h b/include/libcamera/internal/camera.h
> > index 18f5c32a18e4..967d4e1693ec 100644
> > --- a/include/libcamera/internal/camera.h
> > +++ b/include/libcamera/internal/camera.h
> > @@ -35,6 +35,10 @@ public:
> >         PipelineHandler *pipe() { return pipe_.get(); }
> >         const PipelineHandler *pipe() const { return pipe_.get(); }
> >  
> > +       void emitBufferCompleted(Request *request, FrameBuffer *buffer);
> > +       void emitRequestCompleted(Request *request);
> > +       void emitDisconnected();
> > +
> >         std::list<Request *> queuedRequests_;
> >         ControlInfoMap controlInfo_;
> >         ControlList properties_;
> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > index c180a5fdde93..c3e656cab671 100644
> > --- a/src/libcamera/camera.cpp
> > +++ b/src/libcamera/camera.cpp
> > @@ -737,6 +737,25 @@ void Camera::Private::setState(State state)
> >  {
> >         state_.store(state, std::memory_order_release);
> >  }
> > +
> 
> I'm often unsure about the order of the functions in the .cpp/.h. As you
> placed it before setState in the header and the functions are before
> setState alphabetically also, I would probably move them before
> setState().

I remember being told that they only have to be in the same order within the
public/private group. So these three functions are in the same order as the
header with respect to the other public functions, but they don't have to be
relative to the private functions.

> 
> I thought doxygen would nag the missing documentation as these functions
> are public. Anyways these are pretty self-explanatory.

It eventually did :)
I'll add them.

> 
> Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com>


Thanks,

Paul

> 
> > +void Camera::Private::emitBufferCompleted(Request *request, FrameBuffer *buffer)
> > +{
> > +       Camera *camera = _o<Camera>();
> > +       camera->bufferCompleted.emit(request, buffer);
> > +}
> > +
> > +void Camera::Private::emitRequestCompleted(Request *request)
> > +{
> > +       Camera *camera = _o<Camera>();
> > +       camera->requestCompleted.emit(request);
> > +}
> > +
> > +void Camera::Private::emitDisconnected()
> > +{
> > +       Camera *camera = _o<Camera>();
> > +       camera->disconnected.emit();
> > +}
> > +
> >  #endif /* __DOXYGEN_PUBLIC__ */
> >  
> >  /**
> > @@ -947,7 +966,7 @@ void Camera::disconnect()
> >         LOG(Camera, Debug) << "Disconnecting camera " << id();
> >  
> >         _d()->disconnect();
> > -       disconnected.emit();
> > +       _d()->emitDisconnected();
> >  }
> >  
> >  int Camera::exportFrameBuffers(Stream *stream,
> > @@ -1451,7 +1470,7 @@ void Camera::requestComplete(Request *request)
> >                                   true))
> >                 LOG(Camera, Fatal) << "Trying to complete a request when stopped";
> >  
> > -       requestCompleted.emit(request);
> > +       _d()->emitRequestCompleted(request);
> >  }
> >  
> >  } /* namespace libcamera */
> > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> > index d84dff3c9f19..b5faaae08d4c 100644
> > --- a/src/libcamera/pipeline_handler.cpp
> > +++ b/src/libcamera/pipeline_handler.cpp
> > @@ -527,7 +527,7 @@ void PipelineHandler::doQueueRequests()
> >  bool PipelineHandler::completeBuffer(Request *request, FrameBuffer *buffer)
> >  {
> >         Camera *camera = request->_d()->camera();
> > -       camera->bufferCompleted.emit(request, buffer);
> > +       camera->_d()->emitBufferCompleted(request, buffer);
> >         return request->_d()->completeBuffer(buffer);
> >  }
> >  
> > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
> > index 7f1e11e8f913..b4ae0f41a34c 100644
> > --- a/src/libcamera/request.cpp
> > +++ b/src/libcamera/request.cpp
> > @@ -137,7 +137,7 @@ void Request::Private::doCancelRequest()
> >  
> >         for (FrameBuffer *buffer : pending_) {
> >                 buffer->_d()->cancel();
> > -               camera_->bufferCompleted.emit(request, buffer);
> > +               camera_->_d()->emitBufferCompleted(request, buffer);
> >         }
> >  
> >         cancelled_ = true;
> > -- 
> > 2.47.2
> >
Kieran Bingham July 23, 2025, 4:33 p.m. UTC | #3 
Quoting Paul Elder (2025-07-03 12:42:18)
> Add an extra level of indirection when emitting signals from the Camera.
> This is to facilitate the implementation of the layer system in the near
> future, which will need to hook into Camera signal emissions.
> 

It almost makes me want to make the signal itself private to force the
usage through the helpers - but that can't work because applications use
the signal directly!

So I think this is reasonable - and we just have to keep an eye on it to
make sure the helpers are always used...


Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> 
> ---
> No change in v2
> ---
>  include/libcamera/internal/camera.h |  4 ++++
>  src/libcamera/camera.cpp            | 23 +++++++++++++++++++++--
>  src/libcamera/pipeline_handler.cpp  |  2 +-
>  src/libcamera/request.cpp           |  2 +-
>  4 files changed, 27 insertions(+), 4 deletions(-)
> 
> diff --git a/include/libcamera/internal/camera.h b/include/libcamera/internal/camera.h
> index 18f5c32a18e4..967d4e1693ec 100644
> --- a/include/libcamera/internal/camera.h
> +++ b/include/libcamera/internal/camera.h
> @@ -35,6 +35,10 @@ public:
>         PipelineHandler *pipe() { return pipe_.get(); }
>         const PipelineHandler *pipe() const { return pipe_.get(); }
>  
> +       void emitBufferCompleted(Request *request, FrameBuffer *buffer);
> +       void emitRequestCompleted(Request *request);
> +       void emitDisconnected();
> +
>         std::list<Request *> queuedRequests_;
>         ControlInfoMap controlInfo_;
>         ControlList properties_;
> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> index c180a5fdde93..c3e656cab671 100644
> --- a/src/libcamera/camera.cpp
> +++ b/src/libcamera/camera.cpp
> @@ -737,6 +737,25 @@ void Camera::Private::setState(State state)
>  {
>         state_.store(state, std::memory_order_release);
>  }
> +
> +void Camera::Private::emitBufferCompleted(Request *request, FrameBuffer *buffer)
> +{
> +       Camera *camera = _o<Camera>();
> +       camera->bufferCompleted.emit(request, buffer);
> +}
> +
> +void Camera::Private::emitRequestCompleted(Request *request)
> +{
> +       Camera *camera = _o<Camera>();
> +       camera->requestCompleted.emit(request);
> +}
> +
> +void Camera::Private::emitDisconnected()
> +{
> +       Camera *camera = _o<Camera>();
> +       camera->disconnected.emit();
> +}
> +
>  #endif /* __DOXYGEN_PUBLIC__ */
>  
>  /**
> @@ -947,7 +966,7 @@ void Camera::disconnect()
>         LOG(Camera, Debug) << "Disconnecting camera " << id();
>  
>         _d()->disconnect();
> -       disconnected.emit();
> +       _d()->emitDisconnected();
>  }
>  
>  int Camera::exportFrameBuffers(Stream *stream,
> @@ -1451,7 +1470,7 @@ void Camera::requestComplete(Request *request)
>                                   true))
>                 LOG(Camera, Fatal) << "Trying to complete a request when stopped";
>  
> -       requestCompleted.emit(request);
> +       _d()->emitRequestCompleted(request);
>  }
>  
>  } /* namespace libcamera */
> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> index d84dff3c9f19..b5faaae08d4c 100644
> --- a/src/libcamera/pipeline_handler.cpp
> +++ b/src/libcamera/pipeline_handler.cpp
> @@ -527,7 +527,7 @@ void PipelineHandler::doQueueRequests()
>  bool PipelineHandler::completeBuffer(Request *request, FrameBuffer *buffer)
>  {
>         Camera *camera = request->_d()->camera();
> -       camera->bufferCompleted.emit(request, buffer);
> +       camera->_d()->emitBufferCompleted(request, buffer);
>         return request->_d()->completeBuffer(buffer);
>  }
>  
> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
> index 7f1e11e8f913..b4ae0f41a34c 100644
> --- a/src/libcamera/request.cpp
> +++ b/src/libcamera/request.cpp
> @@ -137,7 +137,7 @@ void Request::Private::doCancelRequest()
>  
>         for (FrameBuffer *buffer : pending_) {
>                 buffer->_d()->cancel();
> -               camera_->bufferCompleted.emit(request, buffer);
> +               camera_->_d()->emitBufferCompleted(request, buffer);
>         }
>  
>         cancelled_ = true;
> -- 
> 2.47.2
>

Patch
diff mbox series 

diff --git a/include/libcamera/internal/camera.h b/include/libcamera/internal/camera.h
index 18f5c32a18e4..967d4e1693ec 100644
--- a/include/libcamera/internal/camera.h
+++ b/include/libcamera/internal/camera.h
@@ -35,6 +35,10 @@  public:
 	PipelineHandler *pipe() { return pipe_.get(); }
 	const PipelineHandler *pipe() const { return pipe_.get(); }
 
+	void emitBufferCompleted(Request *request, FrameBuffer *buffer);
+	void emitRequestCompleted(Request *request);
+	void emitDisconnected();
+
 	std::list<Request *> queuedRequests_;
 	ControlInfoMap controlInfo_;
 	ControlList properties_;
diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
index c180a5fdde93..c3e656cab671 100644
--- a/src/libcamera/camera.cpp
+++ b/src/libcamera/camera.cpp
@@ -737,6 +737,25 @@  void Camera::Private::setState(State state)
 {
 	state_.store(state, std::memory_order_release);
 }
+
+void Camera::Private::emitBufferCompleted(Request *request, FrameBuffer *buffer)
+{
+	Camera *camera = _o<Camera>();
+	camera->bufferCompleted.emit(request, buffer);
+}
+
+void Camera::Private::emitRequestCompleted(Request *request)
+{
+	Camera *camera = _o<Camera>();
+	camera->requestCompleted.emit(request);
+}
+
+void Camera::Private::emitDisconnected()
+{
+	Camera *camera = _o<Camera>();
+	camera->disconnected.emit();
+}
+
 #endif /* __DOXYGEN_PUBLIC__ */
 
 /**
@@ -947,7 +966,7 @@  void Camera::disconnect()
 	LOG(Camera, Debug) << "Disconnecting camera " << id();
 
 	_d()->disconnect();
-	disconnected.emit();
+	_d()->emitDisconnected();
 }
 
 int Camera::exportFrameBuffers(Stream *stream,
@@ -1451,7 +1470,7 @@  void Camera::requestComplete(Request *request)
 				  true))
 		LOG(Camera, Fatal) << "Trying to complete a request when stopped";
 
-	requestCompleted.emit(request);
+	_d()->emitRequestCompleted(request);
 }
 
 } /* namespace libcamera */
diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
index d84dff3c9f19..b5faaae08d4c 100644
--- a/src/libcamera/pipeline_handler.cpp
+++ b/src/libcamera/pipeline_handler.cpp
@@ -527,7 +527,7 @@  void PipelineHandler::doQueueRequests()
 bool PipelineHandler::completeBuffer(Request *request, FrameBuffer *buffer)
 {
 	Camera *camera = request->_d()->camera();
-	camera->bufferCompleted.emit(request, buffer);
+	camera->_d()->emitBufferCompleted(request, buffer);
 	return request->_d()->completeBuffer(buffer);
 }
 
diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
index 7f1e11e8f913..b4ae0f41a34c 100644
--- a/src/libcamera/request.cpp
+++ b/src/libcamera/request.cpp
@@ -137,7 +137,7 @@  void Request::Private::doCancelRequest()
 
 	for (FrameBuffer *buffer : pending_) {
 		buffer->_d()->cancel();
-		camera_->bufferCompleted.emit(request, buffer);
+		camera_->_d()->emitBufferCompleted(request, buffer);
 	}
 
 	cancelled_ = true;