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

[Umang Jain]

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.


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