[libcamera-devel] [PATCH LIBCAMERA WIP] libcamera: rkisp1: Use parametered constructor instead of default
Jacopo Mondi
jacopo at jmondi.org
Sat Mar 21 19:30:14 CET 2020
Hi Kaaira,
On Sat, Mar 21, 2020 at 11:29:48PM +0530, Kaaira Gupta wrote:
> 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.
>
Great! be aware that clear() != erase()
void clear();
(until C++11)
Erases all elements from the container. After this call, size() returns zero.
This is not what you want, I'm sure :)
Thanks
j
> 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
More information about the libcamera-devel
mailing list