[libcamera-devel] [PATCH v3 4/4] libcamera: rkisp1: Fix enumeration of RAW formats

[Laurent Pinchart]

Hi Jacopo,

On Mon, Mar 20, 2023 at 08:30:13AM +0100, Jacopo Mondi wrote:
> On Sun, Mar 19, 2023 at 10:08:25PM +0200, Laurent Pinchart wrote:
> > On Tue, Mar 07, 2023 at 12:48:04PM +0100, Jacopo Mondi via libcamera-devel wrote:
> > > The current implementation enumerates a single RAW format (the sensor's
> > > resolution) and does that regardless of what role the
> > > CameraConfiguration has been generated for.
> > >
> > > Fix this by:
> > > - Enumerate RAW StreamFormats only when the requested role is
> > >   StreamRole::Raw
> > > - Add all the sensor's provided resolutions that fit the video device
> > >   output maximum size
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> > > Reviewed-by: Paul Elder <paul.elder at ideasonboard.com>
> > > ---
> > >  src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 19 +++++++++++++++++--
> > >  1 file changed, 17 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> > > index a27ac6fc35cb..0e4d76677732 100644
> > > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> > > @@ -150,18 +150,33 @@ RkISP1Path::generateConfiguration(const CameraSensor *sensor,
> > >  	for (const auto &format : streamFormats_) {
> > >  		const PixelFormatInfo &info = PixelFormatInfo::info(format);
> > >
> > > +		/* Populate stream formats for non-RAW configurations. */
> > >  		if (info.colourEncoding != PixelFormatInfo::ColourEncodingRAW) {
> > > +			if (role == StreamRole::Raw)
> > > +				continue;
> > > +
> > >  			streamFormats[format] = { { minResolution, maxResolution } };
> > >  			continue;
> > >  		}
> > >
> > > -		/* Skip raw formats not supported by the sensor. */
> > > +		/* Skip RAW formats for non-raw roles. */
> > > +		if (role != StreamRole::Raw)
> > > +			continue;
> > > +
> > > +		/* Populate stream formats for RAW configurations. */
> > >  		uint32_t mbusCode = formatToMediaBus.at(format);
> > >  		if (std::find(mbusCodes.begin(), mbusCodes.end(), mbusCode) ==
> > >  		    mbusCodes.end())
> > > +			/* Skip formats not supported by sensor. */
> > >  			continue;
> > >
> > > -		streamFormats[format] = { { resolution, resolution } };
> > > +		/* Add all the RAW sizes the sensor can produce for this code. */
> > > +		for (const auto &rawSize : sensor->sizes(mbusCode)) {
> > > +			if (rawSize > maxResolution_)
> >
> > I think you need to test the width and height independently, see how
> > Size::operator>() is defined.
> >
> 
> Ack
> 
> > What will happen if the sensor supports only sizes that are larger than
> > what the ISP can process ?
> 
> How can you capture -anything- if none of the sensor's sizes is
> compatible with the ISP input limits ?

You can't, but I was wondering if we would error out when creating the
camera, fail in other ways, or crash.

> I've also missed how is this realted to this patch

This patch adds new checks on the size, so I was wondering if it changes
the behaviour (for instance from a failure to a crash).

> > Also, I'm wondering if the same ISP size limitation applies to raw
> > formats, when bypassing the ISP for raw capture we may be able to
> > support larger sizes.
> >
> > > +				continue;
> > > +
> > > +			streamFormats[format].push_back({ rawSize, rawSize });
> > > +		}
> > >
> > >  		/*
> > >  		 * Store the raw format with the highest bits per pixel for

-- 
Regards,

Laurent Pinchart