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

[Jacopo Mondi]

Hi Kaaira,

On Sat, Mar 21, 2020 at 10:40:08PM +0530, Kaaira Gupta wrote:
> 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.
>

I did, I'm not sure you have read the clarification were I suggested to
replace resize() with erase() :)

> 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