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

[Kaaira Gupta]

On Mon, Mar 23, 2020 at 01:10:33PM +0000, Kieran Bingham wrote:
> 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 :-)

Exactly, it does cause a lot of breakage. But I am having a hard time
figuring out what all they are because I think I haven't figured it out
right how I can run tests on one thing at a time or maybe 'stop' the
tests on a particular thing? Can you please help me with it? :/

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

Yes

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

Okay I will

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

Yes, I took them from here in validate() :

 cfg.size.width = std::max(32U, std::min(4416U, cfg.size.width));
 cfg.size.height = std::max(16U, std::min(3312U, cfg.size.height));


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

Yes, I'll fix all the whitespace errors.

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