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

Jacopo Mondi jacopo at jmondi.org
Sat Mar 21 16:38:26 CET 2020 

Hi Kaaira,

On Sat, Mar 21, 2020 at 07:39:23PM +0530, Kaaira Gupta wrote:
> 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.

No worries, I have indeed confused you.

My point was that the compiler points to std::vector::resize() and,
even if it can't happen that in this code path that a new element has
to be created, I assume the resize implementation calls the StreamConfiguration
default constructor in case the vector has to be enlarged, hence the error.

That call to default constructor can't happen here, as we check for
the vector not to be empty just before calling resize, and as we
resize to 1, the only option left to enter the here below second if
branch is that (config_.size() > 1) returns true.

	if (config_.empty())
		return Invalid;

	/* Cap the number of entries to the available streams. */
	if (config_.size() > 1) {
		config_.resize(1);
		status = Adjusted;
	}

As long as this pipeline handler only support a single stream and its
valide() implementation only accept a single StreamConfiguration,
I would just erase all the entries beyond the first one to
shrink the vector size to 1 and release the memory containing the
exceeding entries.

To sum it up I would got for

	if (config_.empty())
		return Invalid;

	/* Cap the number of entries to the available streams. */
	if (config_.size() > 1) {
		config_.erase(config_.begin() + 1, config_.end());
		status = Adjusted;
	}

That should make sure there are no calls to the class default
constructor in any code path.

Also, please note you removed the default constructor implementation
in stream.cpp, and you will likely get a linking error. You rightfully
changed the method visibility to private, but its implementation has
to be defined somewhere.

Hope it clears it up
Thanks
   j

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

More information about the libcamera-devel mailing list