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

[Jacopo Mondi]

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.

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