[libcamera-devel] [RFC PATCH 1/3] libcamera: base: camera_sensor: Expose sensor's formats map

[Jacopo Mondi]

On Tue, Aug 03, 2021 at 10:48:51AM +0530, Umang Jain wrote:
> Hi Jacopo
>
> On 8/2/21 8:03 PM, Jacopo Mondi wrote:
> > Hello Umang,
> >
> > On Fri, Jul 30, 2021 at 01:58:26PM +0530, Umang Jain wrote:
> > > Add a getter function formats() to retrieve the V4L2Subdevice::format
> > > map of all the formats that the sensor supports. This will probably be
> > > used by pipeline handlers to match against a custom list of formats
> > > internally while making a selection on sensor's format to be used for
> > > image capture.
> > >
> > > Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> > > ---
> > >   include/libcamera/internal/camera_sensor.h | 1 +
> > >   1 file changed, 1 insertion(+)
> > >
> > > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
> > > index db12b07e..d8826e8a 100644
> > > --- a/include/libcamera/internal/camera_sensor.h
> > > +++ b/include/libcamera/internal/camera_sensor.h
> > > @@ -37,6 +37,7 @@ public:
> > >   	const std::string &model() const { return model_; }
> > >   	const std::string &id() const { return id_; }
> > >   	const MediaEntity *entity() const { return entity_; }
> > > +	const V4L2Subdevice::Formats &formats() const { return formats_; }
> > We currently have
> >
> >    	const std::vector<unsigned int> &mbusCodes() const { return mbusCodes_; }
> >    	const std::vector<Size> &sizes() const { return sizes_; }
> >
> > sizes() is used by the CIO2 class only to enumerate the supported raw
> > stream resolution.
> >
> > Honestly exposing the list of all resolution through sizes() without
> > context (the mbus code those sizes can be produced with) is not nice.
> > It only works so far as we know on IPU3 that all the produced raw formats
> > are good.
> >
> > I would transform sizes() in sizes(unsigned int) and if one needs to
> > know all the sizes it will be required to
> >
> >          std::vector<Size> allSizes;
> >          for (mbusCode : sensor->mbusCodes())
> >                  for (size : sensor->sizes(mbusCode))
> >                          allSizes.push_back(size);
> >
> > but this will have duplicates, so probably the caller should use an
> > std::set<>.
>
> I agree on the approach, but the good thing about having sizes_ queried
> from  CameraSensor, was that, it was assured to be non-duplicated and sorted
> everytime.
>
> Like you said, now we have to take care of these nuances for -all-sizes-
> use-cause, use std::set<> to maintain unique-ness, probably transform them
> into a vector as well before returning. All in all, cubersome to have to
> deal with it everything some ph needs to know about all the sizes.
>
> Currently we have only one call to sizes(), in CIO2Device::sizes(), so maybe
> we can go ahead with this approach, but I think it's /not/ looking the best.
>
>
> >
> > Anyway, my point is that having
> >
> > 	const V4L2Subdevice::Formats &formats() const { return formats_; }
> >
> > Renders
> >
> >    	const std::vector<unsigned int> &mbusCodes() const { return mbusCodes_; }
> >    	const std::vector<Size> &sizes() const { return sizes_; }
> >
> > A bit useless and exposing the v4l2 subdev formats map is possibly
> > undesirable, as ph will have to use the V4L2Subdevice::Format which is
> > V4L2 specific.
> Makes sense.
> >
> > I would then rather
> >
> >    	const std::vector<Size> &sizes(unsigned int mbusCode) const;
> >
> > What do you think ?
>
> Since the vector will be created and populated inside the function itself, I
> don't think we can return via a reference std::vector <>&  here.

Indeed, that was a copy&paste leftover :)

>
>
> >
> >
> >
> > >   	Size resolution() const;
> > > --
> > > 2.31.0
> > >