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

[Kaaira Gupta]

On Sat, Mar 21, 2020 at 06:27:54PM +0100, Jacopo Mondi wrote:
> 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() :)

Yes I did, and yes using clear() worked. But I was confused as to why the compiler
would worry about the size being less than the resize size when we
already check for it before sending it to resize. But now I understand
that it's the property of resize() to check for both larger and smaller
size whether or not we check beforehand. Hence, it's clear to me now.

Thanks

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