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

[Kaaira Gupta]

On Sat, Mar 21, 2020 at 05:28:56PM +0100, Jacopo Mondi wrote:
> Hi Kaaira,
> 
> On Sat, Mar 21, 2020 at 08:05:38PM +0530, Kaaira Gupta wrote:
> > On Sat, Mar 21, 2020 at 12:56:12PM +0100, Jacopo Mondi wrote:
> 
> [snip]
> 
> > > >
> > > > @@ -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.
> >
> > I got these values from here, in the handler()
> 
> validate() ?

Yes, sorry.

> 
> > 	cfg.size.width = std::max(32U, std::min(4416U, cfg.size.width));
> > 	cfg.size.height = std::max(16U, std::min(3312U, cfg.size.height));
> >
> > That is how I defined them.
> >
> 
> Oh, sorry missed those. I'm not an expert of the platform, but from
> that piece of code I get it's lecit to assume for all pixel formats
> the valid size range is the one you reported.
> 
> 
> > >
> > > > +		};
> > > > +		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.
> >
> > Please read the above logic behind the hardcoded values once and let me
> > know if I still have to map the sizes this way?
> 
> No, I think it's fine the way it is, but I would wait for Niklas or
> Laurent to confirm this as they know this platform better.

Okay!

> 
> In the meantime, and that's for exercize, not because it's so critical
> at this point, be aware that this goes through 2 copies, if I'm not
> mistaken
> 
>                std::vector<SizeRange> sizes{
>                        SizeRange{ { 32, 16 }, { 4416, 3312 } }
>                };
>                pixelformats[pixelformat] = sizes;
> 
> You construct a SizeRange, then copy it inside the vector, than copy
> the vector inside the map. The compiler might optimize the fist copy,
> but I think the second one stays.
> 
> I think this could be reduced to
> 
> 		SizeRange range({32, 16}, {4416, 3312});
> 		pixelformats.emplace(std::piecewise_construct,
> 				     std::forward_as_tuple(pixelformat),
> 				     std::forward_as_tuple(1, range));
> 
> And someone which is more fluent in C++-voodoo than me could even be
> able to construct the Sizerange in place in the vector instead
> having to create an instance explicitly and copy it as I'm doing.

I'll try that for sure

> 
> This saves you the copy of of the vector in the map, by constructing the
> element in the map directly by forwarding the (1, range) arguments to the
> vector constructor. The gain is minumum but not so long ago the first
> time I saw a similar construct I was a bit puzzled, so I hope this
> could help decoding other parts of the library where we use it.

Thankyou so much for this help, it would surely have taken me time to
decode it myself as I am more fluent with Java and Python. Had C++ as a
course in just one sem..

Also, as for the error the compiler throws in validate(), if you get
time please check my mails regarding that.

Thanks!

> 
> Thanks
>    j
> 
> >
> > Thanks!
> >
> > >
> > > 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