[libcamera-devel] [PATCH v4 01/12] libcamera: ipu3: Sub-class Stream with IPU3Stream

[Laurent Pinchart]

Hi Jacopo,

On Sun, Apr 14, 2019 at 10:48:59PM +0200, Niklas Söderlund wrote:
> On 2019-04-09 21:25:37 +0200, Jacopo Mondi wrote:
> > In preparation for multiple stream support provide a Stream sub-class to

s/stream/streams/

> > maintain IPU3 specific data. In order to be able to sub-class Stream
> > remove the 'final' specifier from the class definition and make its
> > private members protected.
> 
> I liked that Stream was final, towards applications. I assume we will 
> make it so again once we introduce d-pointers :-)
> 
> This patch should be split in two, one touching stream.h whit a commit 
> message explaining why Stream needs to be able to be sub-classed and one 
> touching ipu3.cpp making use of the change.
> 
> The changes themself are good.
> 
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> >  include/libcamera/stream.h           |  4 ++--
> >  src/libcamera/pipeline/ipu3/ipu3.cpp | 16 ++++++++++++++--
> >  2 files changed, 16 insertions(+), 4 deletions(-)
> > 
> > diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h
> > index d0f7b0e12485..8a47930f8614 100644
> > --- a/include/libcamera/stream.h
> > +++ b/include/libcamera/stream.h
> > @@ -43,7 +43,7 @@ private:
> >  	Size size_;
> >  };
> >  
> > -class Stream final
> > +class Stream
> >  {
> >  public:
> >  	class StillCapture : public StreamUsage
> > @@ -68,7 +68,7 @@ public:
> >  	BufferPool &bufferPool() { return bufferPool_; }
> >  	const StreamConfiguration &configuration() const { return configuration_; }
> >  
> > -private:
> > +protected:
> >  	friend class Camera;
> >  
> >  	BufferPool bufferPool_;
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index ca09da753b90..00907bb53891 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -133,6 +133,18 @@ public:
> >  	BufferPool pool_;
> >  };
> >  
> > +class IPU3Stream : public Stream
> > +{
> > +public:
> > +	IPU3Stream()
> > +		: active_(false)
> > +	{
> > +	}
> > +
> > +	bool active_;
> > +	std::string name_;

You don't use those two fields yet, I'd add them in the patch that makes
use of them. Or, given that the IPU3 part of this patch will become very
thin after splitting the patch in two as requested by Niklas, you could
squash the IPU3 changes with 02/12 from this series.

> > +};
> > +
> >  class PipelineHandlerIPU3 : public PipelineHandler
> >  {
> >  public:
> > @@ -171,7 +183,7 @@ private:
> >  		CIO2Device cio2_;
> >  		ImgUDevice *imgu_;
> >  
> > -		Stream stream_;
> > +		IPU3Stream stream_;
> >  	};
> >  
> >  	static constexpr unsigned int IPU3_BUFFER_COUNT = 4;
> > @@ -404,7 +416,7 @@ int PipelineHandlerIPU3::queueRequest(Camera *camera, Request *request)
> >  {
> >  	IPU3CameraData *data = cameraData(camera);
> >  	V4L2Device *output = data->imgu_->output_.dev;
> > -	Stream *stream = &data->stream_;
> > +	IPU3Stream *stream = &data->stream_;
> >  
> >  	/* Queue a buffer to the ImgU output for capture. */
> >  	Buffer *buffer = request->findBuffer(stream);

-- 
Regards,

Laurent Pinchart