[libcamera-devel] [PATCH 1/4] libcamera: camera_sensor: Transform CameraSensor::sizes()

[Jacopo Mondi]

Hi Umang,

On Tue, Aug 10, 2021 at 11:13:35AM +0530, Umang Jain wrote:
> Hi Jacopo
>
> On 8/9/21 9:19 PM, Jacopo Mondi wrote:
> > Hi Umang,
> >
> > On Tue, Aug 03, 2021 at 07:02:02PM +0530, Umang Jain wrote:
> > > In CameraSensor, the mbusCodes() and sizes() accessor functions
> > > retrieves all the supported media bus codes and the supported sizes
> > > respectively. However, this is quite limiting since the caller
> > > probably isn't in a position to match which range of sizes are
> > > supported for a particular mbusCode.
> > >
> > > Hence, the caller is most likely interested to know about the sizes
> > > supported for a particular media bus code. This patch transforms the
> > > existing CameraSensor::sizes() to CameraSensor::sizes(mbuscode) to
> > > achieve that goal.
> > >
> > > To know all the frame sizes of the CameraSensor as required in IPU3
> > > case Cio2Device::sizes(), one would now require to enumerate all the
> > > media bus codes (can be retrieved by CameraSensor::mbusCodes()) with
> > > CameraSensor::size(mbusCode). The result can be inserted in a
> > > std::set<> to avoid duplicates.
> > >
> > > Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> > > ---
> > >   include/libcamera/internal/camera_sensor.h |  2 +-
> > >   src/libcamera/camera_sensor.cpp            | 36 ++++++++++++++++------
> > >   src/libcamera/pipeline/ipu3/cio2.cpp       | 10 +++---
> > >   test/camera-sensor.cpp                     |  2 +-
> > >   4 files changed, 34 insertions(+), 16 deletions(-)
> > >
> <snip>
>
> > > +			allSizes.insert(sz);
> > > +	}
> > >
> > > -	return sizes;
> > > +	return std::vector<SizeRange>(allSizes.begin(), allSizes.end());
> > Looking at how CIO2::sizes() is used in IPU3::generateConfiguration()
> > I wonder if we shouldn't pass the desired PixelFormat to
>
> I think you meant s/shouldn't/should here? So essentially do you want
>
>    std::vector<SizeRange> CIO2Device::sizes(const PixelFormat &format)
>    const
>
>    ?
>
> (which looks okay to me)

Looking at how it is used to generate the raw stream sizes I think it
would be better

>
> > CIO2::sizes() so that we can actually enumerate the per-format sizes
> > for real (and avoid going through the set->vector conversion).
> >
> > >   }
> > >
> > >   /**
> > > diff --git a/test/camera-sensor.cpp b/test/camera-sensor.cpp
> > > index a8dcad82..372ee4af 100644
> > > --- a/test/camera-sensor.cpp
> > > +++ b/test/camera-sensor.cpp
> > > @@ -76,7 +76,7 @@ protected:
> > >   			return TestFail;
> > >   		}
> > >
> > > -		const std::vector<Size> &sizes = sensor_->sizes();
> > > +		const std::vector<Size> &sizes = sensor_->sizes(*iter);
> > This changes the semantic of the test from "let's check if a format
> > supports 4096x2160" to "let's check if ARGB8888_1X32 supports
> > 4096x2160". I think it's better now, and as long as the test passes,
> > since it uses VIMC, we should be good!
> I'll check!

Yeah please make sure tests still pass in full. This one shouldn't be
a problem...

Thanks
   j

> >
> > Thanks
> >     j
> >
> > >   		auto iter2 = std::find(sizes.begin(), sizes.end(),
> > >   				       Size(4096, 2160));
> > >   		if (iter2 == sizes.end()) {
> > > --
> > > 2.31.0
> > >