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

[Jacopo Mondi]

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

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

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.

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.

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