[libcamera-devel] [PATCH 2/2] libcamera: ipu3: cio2: Do not proxy signal

[Niklas Söderlund]

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 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.

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 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

-- 
Regards,
Niklas Söderlund