Message ID | 20250703114225.2074071-4-paul.elder@ideasonboard.com |
---|---|
State | New |
Headers | show |
Series |
|
Related | show |
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 >
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 > >
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 >
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;
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(-)