[libcamera-devel] [PATCH 2/2] libcamera: ipu3: cio2: Do not proxy signal
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Jun 29 22:48:37 CEST 2020
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 at 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 at 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
More information about the libcamera-devel
mailing list