[libcamera-devel] [PATCH LIBCAMERA WIP] libcamera: rkisp1: Use parametered constructor instead of default

Sat Mar 21 15:09:23 CET 2020

On Sat, Mar 21, 2020 at 01:02:46PM +0100, Jacopo Mondi wrote:
> Hi again,
> 
> On Sat, Mar 21, 2020 at 12:56:12PM +0100, Jacopo Mondi wrote:
> > Hi Kaaira,
> >
> > On Sat, Mar 21, 2020 at 03:20:20AM +0530, Kaaira Gupta wrote:
> > > Use of default constructor StreamConfiguration() in deprecated, hence
> > > make it private. Make Stream class a friend of StreamConfiguration as it
> > > uses the default constructor.
> > >
> > > Replace default constructor StreamConfiguration() by it's parametered
> >
> > s/parametered/parametrized ?
> >
> > > counterpart by using StreamFormats in generateConfiguration() in rkisp1
> > >
> > > Signed-off-by: Kaaira Gupta <kgupta at es.iitr.ac.in>
> > > ---
> > > WIP:
> > > 	- It fails to build as other pipelines still use default
> > > 	  constructor.
> > > 	- Even rkisp1 fails as it uses default constructor in its
> > > 	  validate() function.
> >
> > The compiler points to
> >
> >                 config_.resize(1);
> >                         ^
> > ../include/libcamera/stream.h:54:2: note: declared private here
> >         StreamConfiguration();
> >
> > The std::vector::resize() documentation says
> > If the current size is greater than count, the container is reduced to its first count elements.
> >
> > So I guess the compiler is just worried about the possibility that the
> > current size is smaller than the resize(n) argument and a new instance
> > should be created, so it would need to access the default constructor.
> >
> > The most trivial option here is to manually delete the exceeding
> > instances if vector.size() > 1 and create a new one using the
> > parametrized constructor.
> 
> Sorry this is confused.
> 
> Delete the exceeding entries if the size of the vector is larger and
> create a new instance manually if the vector is empty (which can't
> happen as it is caught above :).

Can you please elaborate it a bit more? I don't catch it when you say
that "which can't happen as it is caught above", what exactly are you
talking about? 
Sorry if this seems trivial but I am confused of what you are trying to
get me to do here.

thanks!

> 
> >
> > > 	- I have taken care of generateConfiguration() only.
> > >
> > >  include/libcamera/stream.h               |  4 ++-
> > >  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 37 ++++++++++++++++--------
> > >  src/libcamera/stream.cpp                 |  9 ------
> > >  3 files changed, 28 insertions(+), 22 deletions(-)
> > >
> > > diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h
> > > index b1441f8..c19aed6 100644
> > > --- a/include/libcamera/stream.h
> > > +++ b/include/libcamera/stream.h
> > > @@ -37,7 +37,6 @@ private:
> > >  };
> > >
> > >  struct StreamConfiguration {
> > > -	StreamConfiguration();
> > >  	StreamConfiguration(const StreamFormats &formats);
> > >
> > >  	PixelFormat pixelFormat;
> > > @@ -52,8 +51,11 @@ struct StreamConfiguration {
> > >  	std::string toString() const;
> > >
> > >  private:
> > > +	StreamConfiguration();
> > >  	Stream *stream_;
> > >  	StreamFormats formats_;
> > > +
> > > +	friend class Stream;
> > >  };
> > >
> > >  enum StreamRole {
> > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > index 2f909ce..bf97b53 100644
> > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > @@ -218,6 +218,21 @@ private:
> > >  	Camera *activeCamera_;
> > >  };
> > >
> > > +namespace {
> > > +
> > > +static const std::array<PixelFormat, 8> formats{
> > > +	PixelFormat(DRM_FORMAT_YUYV),
> > > +	PixelFormat(DRM_FORMAT_YVYU),
> > > +	PixelFormat(DRM_FORMAT_VYUY),
> > > +	PixelFormat(DRM_FORMAT_NV16),
> > > +	PixelFormat(DRM_FORMAT_NV61),
> > > +	PixelFormat(DRM_FORMAT_NV21),
> > > +	PixelFormat(DRM_FORMAT_NV12),
> > > +	/* \todo Add support for 8-bit greyscale to DRM formats */
> > > +};
> > > +
> > > +} /* namespace */
> > > +
> > >  RkISP1Frames::RkISP1Frames(PipelineHandler *pipe)
> > >  	: pipe_(dynamic_cast<PipelineHandlerRkISP1 *>(pipe))
> > >  {
> > > @@ -430,17 +445,6 @@ RkISP1CameraConfiguration::RkISP1CameraConfiguration(Camera *camera,
> > >
> > >  CameraConfiguration::Status RkISP1CameraConfiguration::validate()
> > >  {
> > > -	static const std::array<PixelFormat, 8> formats{
> > > -		PixelFormat(DRM_FORMAT_YUYV),
> > > -		PixelFormat(DRM_FORMAT_YVYU),
> > > -		PixelFormat(DRM_FORMAT_VYUY),
> > > -		PixelFormat(DRM_FORMAT_NV16),
> > > -		PixelFormat(DRM_FORMAT_NV61),
> > > -		PixelFormat(DRM_FORMAT_NV21),
> > > -		PixelFormat(DRM_FORMAT_NV12),
> > > -		/* \todo Add support for 8-bit greyscale to DRM formats */
> > > -	};
> > > -
> > >  	const CameraSensor *sensor = data_->sensor_;
> > >  	Status status = Valid;
> > >
> > > @@ -537,7 +541,16 @@ CameraConfiguration *PipelineHandlerRkISP1::generateConfiguration(Camera *camera
> > >  	if (roles.empty())
> > >  		return config;
> > >
> > > -	StreamConfiguration cfg{};
> > > +	std::map<PixelFormat, std::vector<SizeRange>> pixelformats;
> > > +
> > > +	for (PixelFormat pixelformat : formats) {
> > > +		std::vector<SizeRange> sizes{
> > > +			SizeRange{ { 32, 16 }, { 4416, 3312 } }
> >
> > I might have missed where these sizes come from.
> >
> > > +		};
> > > +		pixelformats[pixelformat] = sizes;
> >
> > So you have an hardcoded list of PixelFormats in the pipeline handler.
> > And you got a map of V4L2PixelFormats from the video device.
> > What I would do is going through all the PixelFormats, get their
> > V4L2PixelFormat counterpart, access the map using that as key to
> > retrive the size vector, and associated them with the PixelFormat you
> > started with.
> >
> > I don't think it is necessary to make sure all the V4L2PixelFormat
> > obtained from the PixelFormat array are valid, as the pipeline handler
> > should know that.
> >
> > Thanks
> >   j
> >
> >
> > > +	}
> > > +	StreamFormats format(pixelformats);
> > > +	StreamConfiguration cfg(format);
> > >  	cfg.pixelFormat = PixelFormat(DRM_FORMAT_NV12);
> > >  	cfg.size = data->sensor_->resolution();
> > >
> > > diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp
> > > index ea3d214..96b3dc5 100644
> > > --- a/src/libcamera/stream.cpp
> > > +++ b/src/libcamera/stream.cpp
> > > @@ -274,15 +274,6 @@ SizeRange StreamFormats::range(const PixelFormat &pixelformat) const
> > >   * configured for a single video stream.
> > >   */
> > >
> > > -/**
> > > - * \todo This method is deprecated and should be removed once all pipeline
> > > - * handlers provied StreamFormats.
> > > - */
> > > -StreamConfiguration::StreamConfiguration()
> > > -	: pixelFormat(0), stream_(nullptr)
> > > -{
> > > -}
> > > -
> > >  /**
> > >   * \brief Construct a configuration with stream formats
> > >   */
> > > --
> > > 2.17.1
> > >
> > > _______________________________________________
> > > libcamera-devel mailing list
> > > libcamera-devel at lists.libcamera.org
> > > https://lists.libcamera.org/listinfo/libcamera-devel
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel