| Message ID | 20210730082832.152626-2-umang.jain@ideasonboard.com |
|---|---|
| State | Superseded |
| Delegated to: | Umang Jain |
| Headers | show |
| Series |
|
| Related | show |
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@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<>. 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. I would then rather const std::vector<Size> &sizes(unsigned int mbusCode) const; What do you think ? > Size resolution() const; > -- > 2.31.0 >
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@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. > > > >> Size resolution() const; >> -- >> 2.31.0 >>
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@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 > > >
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_; } const std::vector<unsigned int> &mbusCodes() const { return mbusCodes_; } const std::vector<Size> &sizes() const { return sizes_; } Size resolution() const;
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@ideasonboard.com> --- include/libcamera/internal/camera_sensor.h | 1 + 1 file changed, 1 insertion(+)