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

[Kieran Bingham]

Hi Kaaira,

On 23/03/2020 08:44, Kaaira Gupta wrote:
> Use of default constructor StreamConfiguration() is 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/ ? If so, both here and in the subject line.

> counterpart by using StreamFormats in generateConfiguration() in rkisp1
> 
> Also, use erase(), instead of replace() in validate() to prevent it from
> creating a new instance with empty constructor.
> 
> Signed-off-by: Kaaira Gupta <kgupta at es.iitr.ac.in>
> ---
> 
> Changes since v1:
> 	- replace resize() by erase() in validate() to prevent it from
> creating a new instance with empty constructor.
> Changes since v2:
> 	- Rearaange the order of declaration of friend class.
> 	- Add constructor implementation again to stream.cpp
> 	- Corrections in range of erase()
> 
> include/libcamera/stream.h               |  4 ++-
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 39 ++++++++++++++++--------
>  src/libcamera/stream.cpp                 |  6 ++--
>  3 files changed, 32 insertions(+), 17 deletions(-)
> 
> diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h
> index b1441f8..a379ebb 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,6 +51,9 @@ struct StreamConfiguration {
>  	std::string toString() const;
>  
>  private:
> +	friend class Stream;
> +	StreamConfiguration();
> +

Because the change to move the StreamConfiguration(); to being private
causes multiple breakage, I think we should have it as a separate
change, so this has certainly already become a series :-)

The series should thus fix all breakages (before it breaks) then move
this at the end of the series. Probably along with the removal of the \todo


>  	Stream *stream_;
>  	StreamFormats formats_;
>  };
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 2f909ce..6839e3c 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;
>  
> @@ -449,7 +453,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
>  
>  	/* Cap the number of entries to the available streams. */
>  	if (config_.size() > 1) {
> -		config_.resize(1);
> +		config_.erase(config_.begin() + 1, config_.end());

Interesting, how does this differ from .resize(1)?

Ah, I see - it's required because if .resize() is used to 'increase' the
size, it uses the default constructor, and that is no longer available.

I think it would be useful to add a comment to explain /why/ we use
.erase() instead of .resize() here...


>  		status = Adjusted;
>  	}
>  
> @@ -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{

Where do the values for the following SizeRange come from? A comment to
indicate could be beneficial.

Presumably these are the minimum and maximum capabilities of the ISP
output ?

Ideally these values should be determined from the hardware driver, but
maybe that's not possible right now.


> +			SizeRange{ { 32, 16 }, { 4416, 3312 } }
> +		};
> +		pixelformats[pixelformat] = sizes;
> +	}

I think a newline could be added here,

> +	StreamFormats format(pixelformats);
> +	StreamConfiguration cfg(format);

And probably here to aid readability.

>  	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..7e41378 100644
> --- a/src/libcamera/stream.cpp
> +++ b/src/libcamera/stream.cpp
> @@ -275,9 +275,9 @@ SizeRange StreamFormats::range(const PixelFormat &pixelformat) const
>   */
>  
>  /**
> - * \todo This method is deprecated and should be removed once all pipeline
> - * handlers provied StreamFormats.
> - */
> +* \todo This method is deprecated and should be removed once all pipeline
> +* handlers provied StreamFormats.
> +*/

This looks like an unintentional change (just removing some whitespace
on the comment?), and shouldn't be in the patch.

Once the pipeline handlers all provide StreamFormats, a patch on top
should remove this comment of course.

>  StreamConfiguration::StreamConfiguration()
>  	: pixelFormat(0), stream_(nullptr)
>  {
> 

-- 
Regards
--
Kieran