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

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

Commit Message

Jacopo Mondi March 21, 2023, 5:20 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

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@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(-)

Comments

Umang Jain May 16, 2023, 5:48 a.m. UTC | #1 
Hi Jacopo,

Thank you for the patch

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@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 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?

Rest looks good to me

Reviewed-by: Umang Jain <umang.jain@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
Laurent Pinchart June 5, 2023, 2:46 p.m. UTC | #2 
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@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 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@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
Laurent Pinchart June 5, 2023, 3:06 p.m. UTC | #3 
Hi Jacopo,

Thank you for the patch.

On Tue, Mar 21, 2023 at 06:20: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
> 
> 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@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 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;
>  		}
>  
> -		/* 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;

In the review of the previous version, I mentioned that I think you need
to test the width and height independently (see how Size::operator>() is
defined). You "acked" that, have you kept the code as-is on purpose, or
was it an oversight ?

> +
> +			streamFormats[format].push_back({ rawSize, rawSize });
> +		}
>  
>  		/*
>  		 * Store the raw format with the highest bits per pixel for

Patch
diff mbox series 

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;
 		}
 
-		/* 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