[libcamera-devel,v2,4/4] libcamera: rkisp1: Fix enumeration of RAW formats
diff mbox series

Message ID 20230222151917.669526-5-jacopo.mondi@ideasonboard.com
State Superseded
Headers show
Series
  • libcamera: rkisp1: Fix generateConfiguration
Related show

Commit Message

Jacopo Mondi Feb. 22, 2023, 3:19 p.m. UTC 
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@ideasonboard.com>
---
 src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

Comments

Paul Elder March 3, 2023, 8:53 a.m. UTC | #1 
On Wed, Feb 22, 2023 at 04:19:17PM +0100, Jacopo Mondi via libcamera-devel wrote:
> The current implementation enumerates a single RAW format (the sensor's

s/a single/a single size/ ?

> 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@ideasonboard.com>

Reviewed-by: Paul Elder <paul.elder@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_)
> +				continue;
> +
> +			streamFormats[format].push_back({ rawSize, rawSize });
> +		}
>  
>  		/*
>  		 * Store the raw format with the highest bits per pixel for
> -- 
> 2.39.0
>
Jacopo Mondi March 7, 2023, 11:30 a.m. UTC | #2 
Hi Paul

On Fri, Mar 03, 2023 at 05:53:57PM +0900, Paul Elder via libcamera-devel wrote:
> On Wed, Feb 22, 2023 at 04:19:17PM +0100, Jacopo Mondi via libcamera-devel wrote:
> > The current implementation enumerates a single RAW format (the sensor's
>
> s/a single/a single size/ ?

I actually meant "single RAW format" as I'm referring to
"StreamFormats". Can I keep it this way or is it grammatically wrong ?

>
> > 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@ideasonboard.com>
>
> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>

Thanks

>
> > ---
> >  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_)
> > +				continue;
> > +
> > +			streamFormats[format].push_back({ rawSize, rawSize });
> > +		}
> >
> >  		/*
> >  		 * Store the raw format with the highest bits per pixel for
> > --
> > 2.39.0
> >

Patch
diff mbox series 

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_)
+				continue;
+
+			streamFormats[format].push_back({ rawSize, rawSize });
+		}
 
 		/*
 		 * Store the raw format with the highest bits per pixel for