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

[Jacopo Mondi]

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

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