| Message ID | 20200629153518.3734304-2-niklas.soderlund@ragnatech.se |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Niklas, Thank you for the patch. On Mon, Jun 29, 2020 at 05:35:18PM +0200, Niklas Söderlund wrote: > Do not proxy the signal in the CI2Device when there is no need for it, > remove it. > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > This is a leftover form the CIO2 rework which fell thru the cracks I'm > sorry I missed to resort it in the last version. > --- > src/libcamera/pipeline/ipu3/cio2.cpp | 8 -------- > src/libcamera/pipeline/ipu3/cio2.h | 6 +++--- > src/libcamera/pipeline/ipu3/ipu3.cpp | 2 +- > 3 files changed, 4 insertions(+), 12 deletions(-) > > diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp > index aa1459fb3599283b..08b6a9a12916299b 100644 > --- a/src/libcamera/pipeline/ipu3/cio2.cpp > +++ b/src/libcamera/pipeline/ipu3/cio2.cpp > @@ -15,7 +15,6 @@ > #include "libcamera/internal/camera_sensor.h" > #include "libcamera/internal/media_device.h" > #include "libcamera/internal/v4l2_subdevice.h" > -#include "libcamera/internal/v4l2_videodevice.h" > > namespace libcamera { > > @@ -125,8 +124,6 @@ int CIO2Device::init(const MediaDevice *media, unsigned int index) > if (ret) > return ret; > > - output_->bufferReady.connect(this, &CIO2Device::cio2BufferReady); > - > return 0; You could also merge the return with the function call just above this hunk if you want to. > } > > @@ -289,9 +286,4 @@ void CIO2Device::freeBuffers() > LOG(IPU3, Error) << "Failed to release CIO2 buffers"; > } > > -void CIO2Device::cio2BufferReady(FrameBuffer *buffer) > -{ > - bufferReady.emit(buffer); > -} > - > } /* namespace libcamera */ > diff --git a/src/libcamera/pipeline/ipu3/cio2.h b/src/libcamera/pipeline/ipu3/cio2.h > index dc764b101f112f05..4fd949f8e5132e07 100644 > --- a/src/libcamera/pipeline/ipu3/cio2.h > +++ b/src/libcamera/pipeline/ipu3/cio2.h > @@ -13,15 +13,15 @@ > > #include <libcamera/signal.h> > > +#include "libcamera/internal/v4l2_videodevice.h" > + > namespace libcamera { > > class CameraSensor; > class FrameBuffer; > class MediaDevice; > class Request; > -class V4L2DeviceFormat; > class V4L2Subdevice; > -class V4L2VideoDevice; > struct Size; > struct StreamConfiguration; > > @@ -48,7 +48,7 @@ public: > > int queueBuffer(Request *request, FrameBuffer *rawBuffer); > void tryReturnBuffer(FrameBuffer *buffer); > - Signal<FrameBuffer *> bufferReady; > + Signal<FrameBuffer *> &bufferReady() { return output_->bufferReady; } I don't like how our signals are public member variables without a trailing _, and I though about adding a macro along the lines of SIGNAL(bufferReady, FrameBuffer *); that would result in public: Signal<FrameBuffer *> &bufferReady() { return bufferReady_; } private: Signal<FrameBuffer *> bufferReady_; public: except that I don't like how it would implicitly reset the access level to public. And it probably doesn't bring much either. Maybe I'll have a clever idea I like in the future :-) In any case, it's not really related to this patch, except for the fact that we would then always write data->cio2_.bufferReady().connect(...); regardless of whether the signal is proxied or not, which I think would be nice. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > private: > void freeBuffers(); > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index 4e75bd40c66f821a..c1dc377d1fd5b58c 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -805,7 +805,7 @@ int PipelineHandlerIPU3::registerCameras() > * associated ImgU input where they get processed and > * returned through the ImgU main and secondary outputs. > */ > - data->cio2_.bufferReady.connect(data.get(), > + data->cio2_.bufferReady().connect(data.get(), > &IPU3CameraData::cio2BufferReady); > data->imgu_->input_->bufferReady.connect(&data->cio2_, > &CIO2Device::tryReturnBuffer);
Hi Laurent, Thanks for your feedback. On 2020-06-29 23:48:37 +0300, Laurent Pinchart wrote: > Hi Niklas, > > Thank you for the patch. > > On Mon, Jun 29, 2020 at 05:35:18PM +0200, Niklas Söderlund wrote: > > Do not proxy the signal in the CI2Device when there is no need for it, > > remove it. > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > --- > > This is a leftover form the CIO2 rework which fell thru the cracks I'm > > sorry I missed to resort it in the last version. > > --- > > src/libcamera/pipeline/ipu3/cio2.cpp | 8 -------- > > src/libcamera/pipeline/ipu3/cio2.h | 6 +++--- > > src/libcamera/pipeline/ipu3/ipu3.cpp | 2 +- > > 3 files changed, 4 insertions(+), 12 deletions(-) > > > > diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp > > index aa1459fb3599283b..08b6a9a12916299b 100644 > > --- a/src/libcamera/pipeline/ipu3/cio2.cpp > > +++ b/src/libcamera/pipeline/ipu3/cio2.cpp > > @@ -15,7 +15,6 @@ > > #include "libcamera/internal/camera_sensor.h" > > #include "libcamera/internal/media_device.h" > > #include "libcamera/internal/v4l2_subdevice.h" > > -#include "libcamera/internal/v4l2_videodevice.h" > > > > namespace libcamera { > > > > @@ -125,8 +124,6 @@ int CIO2Device::init(const MediaDevice *media, unsigned int index) > > if (ret) > > return ret; > > > > - output_->bufferReady.connect(this, &CIO2Device::cio2BufferReady); > > - > > return 0; > > You could also merge the return with the function call just above this > hunk if you want to. Good point, will do so for v2. > > > } > > > > @@ -289,9 +286,4 @@ void CIO2Device::freeBuffers() > > LOG(IPU3, Error) << "Failed to release CIO2 buffers"; > > } > > > > -void CIO2Device::cio2BufferReady(FrameBuffer *buffer) > > -{ > > - bufferReady.emit(buffer); > > -} > > - > > } /* namespace libcamera */ > > diff --git a/src/libcamera/pipeline/ipu3/cio2.h b/src/libcamera/pipeline/ipu3/cio2.h > > index dc764b101f112f05..4fd949f8e5132e07 100644 > > --- a/src/libcamera/pipeline/ipu3/cio2.h > > +++ b/src/libcamera/pipeline/ipu3/cio2.h > > @@ -13,15 +13,15 @@ > > > > #include <libcamera/signal.h> > > > > +#include "libcamera/internal/v4l2_videodevice.h" > > + > > namespace libcamera { > > > > class CameraSensor; > > class FrameBuffer; > > class MediaDevice; > > class Request; > > -class V4L2DeviceFormat; > > class V4L2Subdevice; > > -class V4L2VideoDevice; > > struct Size; > > struct StreamConfiguration; > > > > @@ -48,7 +48,7 @@ public: > > > > int queueBuffer(Request *request, FrameBuffer *rawBuffer); > > void tryReturnBuffer(FrameBuffer *buffer); > > - Signal<FrameBuffer *> bufferReady; > > + Signal<FrameBuffer *> &bufferReady() { return output_->bufferReady; } > > I don't like how our signals are public member variables without a > trailing _, and I though about adding a macro along the lines of > > SIGNAL(bufferReady, FrameBuffer *); > > that would result in > > public: > Signal<FrameBuffer *> &bufferReady() { return bufferReady_; } > private: > Signal<FrameBuffer *> bufferReady_; > public: LIBCAMERA_OBJECT signals: void bufferReady(FrameBuffer *); ;-P > > except that I don't like how it would implicitly reset the access level > to public. And it probably doesn't bring much either. Maybe I'll have a > clever idea I like in the future :-) In any case, it's not really > related to this patch, except for the fact that we would then always > write > > data->cio2_.bufferReady().connect(...); > > regardless of whether the signal is proxied or not, which I think would > be nice. Joking aside I think this would be a nice thing if something clever could be figure out about a SIGNAL() macro along the lines you suggest. It would also need to be figured out how the Doxygen section for such signals should be written with ease. > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > private: > > void freeBuffers(); > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > > index 4e75bd40c66f821a..c1dc377d1fd5b58c 100644 > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > > @@ -805,7 +805,7 @@ int PipelineHandlerIPU3::registerCameras() > > * associated ImgU input where they get processed and > > * returned through the ImgU main and secondary outputs. > > */ > > - data->cio2_.bufferReady.connect(data.get(), > > + data->cio2_.bufferReady().connect(data.get(), > > &IPU3CameraData::cio2BufferReady); > > data->imgu_->input_->bufferReady.connect(&data->cio2_, > > &CIO2Device::tryReturnBuffer); > > -- > Regards, > > Laurent Pinchart
diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp index aa1459fb3599283b..08b6a9a12916299b 100644 --- a/src/libcamera/pipeline/ipu3/cio2.cpp +++ b/src/libcamera/pipeline/ipu3/cio2.cpp @@ -15,7 +15,6 @@ #include "libcamera/internal/camera_sensor.h" #include "libcamera/internal/media_device.h" #include "libcamera/internal/v4l2_subdevice.h" -#include "libcamera/internal/v4l2_videodevice.h" namespace libcamera { @@ -125,8 +124,6 @@ int CIO2Device::init(const MediaDevice *media, unsigned int index) if (ret) return ret; - output_->bufferReady.connect(this, &CIO2Device::cio2BufferReady); - return 0; } @@ -289,9 +286,4 @@ void CIO2Device::freeBuffers() LOG(IPU3, Error) << "Failed to release CIO2 buffers"; } -void CIO2Device::cio2BufferReady(FrameBuffer *buffer) -{ - bufferReady.emit(buffer); -} - } /* namespace libcamera */ diff --git a/src/libcamera/pipeline/ipu3/cio2.h b/src/libcamera/pipeline/ipu3/cio2.h index dc764b101f112f05..4fd949f8e5132e07 100644 --- a/src/libcamera/pipeline/ipu3/cio2.h +++ b/src/libcamera/pipeline/ipu3/cio2.h @@ -13,15 +13,15 @@ #include <libcamera/signal.h> +#include "libcamera/internal/v4l2_videodevice.h" + namespace libcamera { class CameraSensor; class FrameBuffer; class MediaDevice; class Request; -class V4L2DeviceFormat; class V4L2Subdevice; -class V4L2VideoDevice; struct Size; struct StreamConfiguration; @@ -48,7 +48,7 @@ public: int queueBuffer(Request *request, FrameBuffer *rawBuffer); void tryReturnBuffer(FrameBuffer *buffer); - Signal<FrameBuffer *> bufferReady; + Signal<FrameBuffer *> &bufferReady() { return output_->bufferReady; } private: void freeBuffers(); diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp index 4e75bd40c66f821a..c1dc377d1fd5b58c 100644 --- a/src/libcamera/pipeline/ipu3/ipu3.cpp +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp @@ -805,7 +805,7 @@ int PipelineHandlerIPU3::registerCameras() * associated ImgU input where they get processed and * returned through the ImgU main and secondary outputs. */ - data->cio2_.bufferReady.connect(data.get(), + data->cio2_.bufferReady().connect(data.get(), &IPU3CameraData::cio2BufferReady); data->imgu_->input_->bufferReady.connect(&data->cio2_, &CIO2Device::tryReturnBuffer);
Do not proxy the signal in the CI2Device when there is no need for it, remove it. Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> --- This is a leftover form the CIO2 rework which fell thru the cracks I'm sorry I missed to resort it in the last version. --- src/libcamera/pipeline/ipu3/cio2.cpp | 8 -------- src/libcamera/pipeline/ipu3/cio2.h | 6 +++--- src/libcamera/pipeline/ipu3/ipu3.cpp | 2 +- 3 files changed, 4 insertions(+), 12 deletions(-)