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

[Laurent Pinchart]

Hello,

On Tue, May 16, 2023 at 11:18:44AM +0530, Umang Jain via libcamera-devel wrote:
> On 3/21/23 10:50 PM, 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
> >
> > Before this patch, a single RAW size was enumerated in stream formats
> >
> >   * Pixelformat: SRGGB10 (4208x3120)-(4208x3120)/(+1,+1)
> >    - 4208x3120
> >
> > With this patch applied all sensor's supported resolutions are
> > enumerated but only when the stream role RAW is explicitly requested
> >
> >   * Pixelformat: SRGGB10 (1048x780)-(4208x3120)/(+0,+0)
> >    - 1048x780
> >    - 2104x1560
> >    - 4032x3024
> >    - 4208x3120
> >
> > 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 cca593b84260..8d606de7880f 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;
> >   		}
> 
> 
>                  /* Populate stream formats for non-RAW configurations. */
>                  if (role != StreamRole::Raw &&
>                      info.colourEncoding != PixelFormatInfo::ColourEncodingRAW) {
>                          streamFormats[format] = { { minResolution, maxResolution } };
>                          continue;
>                  }
> Possibly?

That's a different behaviour. Consider what happens if
info.colourEncoding != PixelFormatInfo::ColourEncodingRAW && role ==
StreamRole::Raw without and with this change.

> Rest looks good to me
> 
> Reviewed-by: Umang Jain <umang.jain at ideasonboard.com>
> 
> >   
> > -		/* 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_)
> > +				continue;
> > +
> > +			streamFormats[format].push_back({ rawSize, rawSize });
> > +		}
> >   
> >   		/*
> >   		 * Store the raw format with the highest bits per pixel for

-- 
Regards,

Laurent Pinchart