[libcamera-devel] [PATCH LIBCAMERA WIP] libcamera: rkisp1: Use parametered constructor instead of default
Jacopo Mondi
jacopo at jmondi.org
Sat Mar 21 18:27:54 CET 2020
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
More information about the libcamera-devel
mailing list