[libcamera-devel,v3,3/3] android: capabilities: Centralize RAW support check
diff mbox series

Message ID 20210727110519.11587-4-jacopo@jmondi.org
State Accepted
Headers show
Series
  • android: Drive-by cleanups
Related show

Commit Message

Jacopo Mondi July 27, 2021, 11:05 a.m. UTC 
The validation of RAW stream support is performed in two different
places:

- At initializeStreamConfigurations() time, by verifying that the
  libcamera format associated with HAL_PIXEL_FORMAT_BLOB is a Raw format
  and ensuring the Camera successfully validates it
- As initializeStaticMetadata() time by generating a CameraConfiguration
  for the Raw stream role and ensuring it is a Raw format with a 16 bit
  depth

The first check is used to build the list of supported Raw stream
resolutions. The latter is used to register the
ANDROID_REQUEST_AVAILABLE_CAPABILITIES_RAW capability.

As building the list of supported Raw streams doesn't serve any
purpose if the RAW capability is not registered, centralize the Raw
format support verification at initializeStreamConfigurations() time by
ensuring the supported format is a Raw one with a 16 bit depth.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>
---
 src/android/camera_capabilities.cpp | 37 +++++++++++++++++------------
 src/android/camera_capabilities.h   |  1 +
 2 files changed, 23 insertions(+), 15 deletions(-)

Comments

Laurent Pinchart July 27, 2021, 2:56 p.m. UTC | #1 
Hi Jacopo,

Thank you for the patch.

On Tue, Jul 27, 2021 at 01:05:19PM +0200, Jacopo Mondi wrote:
> The validation of RAW stream support is performed in two different
> places:
> 
> - At initializeStreamConfigurations() time, by verifying that the
>   libcamera format associated with HAL_PIXEL_FORMAT_BLOB is a Raw format
>   and ensuring the Camera successfully validates it
> - As initializeStaticMetadata() time by generating a CameraConfiguration
>   for the Raw stream role and ensuring it is a Raw format with a 16 bit
>   depth
> 
> The first check is used to build the list of supported Raw stream
> resolutions. The latter is used to register the
> ANDROID_REQUEST_AVAILABLE_CAPABILITIES_RAW capability.
> 
> As building the list of supported Raw streams doesn't serve any
> purpose if the RAW capability is not registered, centralize the Raw
> format support verification at initializeStreamConfigurations() time by
> ensuring the supported format is a Raw one with a 16 bit depth.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>
> ---
>  src/android/camera_capabilities.cpp | 37 +++++++++++++++++------------
>  src/android/camera_capabilities.h   |  1 +
>  2 files changed, 23 insertions(+), 15 deletions(-)
> 
> diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp
> index 15e54192adff..709bfb2a4a19 100644
> --- a/src/android/camera_capabilities.cpp
> +++ b/src/android/camera_capabilities.cpp
> @@ -122,6 +122,7 @@ int CameraCapabilities::initialize(std::shared_ptr<libcamera::Camera> camera,
>  	camera_ = camera;
>  	orientation_ = orientation;
>  	facing_ = facing;
> +	rawStreamAvailable_ = false;
>  
>  	/* Acquire the camera and initialize available stream configurations. */
>  	int ret = camera_->acquire();
> @@ -324,11 +325,28 @@ int CameraCapabilities::initializeStreamConfigurations()
>  
>  		std::vector<Size> resolutions;
>  		const PixelFormatInfo &info = PixelFormatInfo::info(mappedFormat);
> -		if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW)
> +		switch (info.colourEncoding) {
> +		case PixelFormatInfo::ColourEncodingRAW:
> +			if (info.bitsPerPixel != 16)
> +				continue;
> +
> +			rawStreamAvailable_ = true;
>  			resolutions = initializeRawResolutions(mappedFormat);
> -		else
> +			break;
> +
> +		case PixelFormatInfo::ColourEncodingYUV:
> +		case PixelFormatInfo::ColourEncodingRGB:
> +			/*
> +			 * We support enumerating RGB streams here to allow
> +			 * mapping IMPLEMENTATION_DEFINED format to RGB.
> +			 */

Thanks for the comment, that makes me feel better.

>  			resolutions = initializeYUVResolutions(mappedFormat,
>  							       cameraResolutions);
> +			break;
> +
> +		default:
> +			continue;

Do you get a warning if you don't add the default case ? If not, I'd
drop it, in order to get a warning if we ever add another colour
encoding.

> +		}
>  
>  		for (const Size &res : resolutions) {
>  			streamConfigurations_.push_back({ res, androidFormat });
> @@ -866,22 +884,11 @@ int CameraCapabilities::initializeStaticMetadata()
>  	};
>  
>  	/* Report if camera supports RAW. */
> -	bool rawStreamAvailable = false;
> -	std::unique_ptr<CameraConfiguration> cameraConfig =
> -		camera_->generateConfiguration({ StreamRole::Raw });
> -	if (cameraConfig && !cameraConfig->empty()) {
> -		const PixelFormatInfo &info =
> -			PixelFormatInfo::info(cameraConfig->at(0).pixelFormat);
> -		/* Only advertise RAW support if RAW16 is possible. */
> -		if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW &&
> -		    info.bitsPerPixel == 16) {
> -			rawStreamAvailable = true;
> +	if (rawStreamAvailable_)
>  			availableCapabilities.push_back(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_RAW);

With one less indentation level,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> -		}
> -	}
>  
>  	/* Number of { RAW, YUV, JPEG } supported output streams */
> -	int32_t numOutStreams[] = { rawStreamAvailable, 2, 1 };
> +	int32_t numOutStreams[] = { rawStreamAvailable_, 2, 1 };
>  	staticMetadata_->addEntry(ANDROID_REQUEST_MAX_NUM_OUTPUT_STREAMS,
>  				  numOutStreams);
>  
> diff --git a/src/android/camera_capabilities.h b/src/android/camera_capabilities.h
> index e7aa46c0a689..42a976d3b482 100644
> --- a/src/android/camera_capabilities.h
> +++ b/src/android/camera_capabilities.h
> @@ -55,6 +55,7 @@ private:
>  
>  	int facing_;
>  	int orientation_;
> +	bool rawStreamAvailable_;
>  
>  	std::vector<Camera3StreamConfiguration> streamConfigurations_;
>  	std::map<int, libcamera::PixelFormat> formatsMap_;
Jacopo Mondi July 27, 2021, 3:36 p.m. UTC | #2 
Hi Laurent,

On Tue, Jul 27, 2021 at 05:56:49PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Tue, Jul 27, 2021 at 01:05:19PM +0200, Jacopo Mondi wrote:
> > The validation of RAW stream support is performed in two different
> > places:
> >
> > - At initializeStreamConfigurations() time, by verifying that the
> >   libcamera format associated with HAL_PIXEL_FORMAT_BLOB is a Raw format
> >   and ensuring the Camera successfully validates it
> > - As initializeStaticMetadata() time by generating a CameraConfiguration
> >   for the Raw stream role and ensuring it is a Raw format with a 16 bit
> >   depth
> >
> > The first check is used to build the list of supported Raw stream
> > resolutions. The latter is used to register the
> > ANDROID_REQUEST_AVAILABLE_CAPABILITIES_RAW capability.
> >
> > As building the list of supported Raw streams doesn't serve any
> > purpose if the RAW capability is not registered, centralize the Raw
> > format support verification at initializeStreamConfigurations() time by
> > ensuring the supported format is a Raw one with a 16 bit depth.
> >
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>
> > ---
> >  src/android/camera_capabilities.cpp | 37 +++++++++++++++++------------
> >  src/android/camera_capabilities.h   |  1 +
> >  2 files changed, 23 insertions(+), 15 deletions(-)
> >
> > diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp
> > index 15e54192adff..709bfb2a4a19 100644
> > --- a/src/android/camera_capabilities.cpp
> > +++ b/src/android/camera_capabilities.cpp
> > @@ -122,6 +122,7 @@ int CameraCapabilities::initialize(std::shared_ptr<libcamera::Camera> camera,
> >  	camera_ = camera;
> >  	orientation_ = orientation;
> >  	facing_ = facing;
> > +	rawStreamAvailable_ = false;
> >
> >  	/* Acquire the camera and initialize available stream configurations. */
> >  	int ret = camera_->acquire();
> > @@ -324,11 +325,28 @@ int CameraCapabilities::initializeStreamConfigurations()
> >
> >  		std::vector<Size> resolutions;
> >  		const PixelFormatInfo &info = PixelFormatInfo::info(mappedFormat);
> > -		if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW)
> > +		switch (info.colourEncoding) {
> > +		case PixelFormatInfo::ColourEncodingRAW:
> > +			if (info.bitsPerPixel != 16)
> > +				continue;
> > +
> > +			rawStreamAvailable_ = true;
> >  			resolutions = initializeRawResolutions(mappedFormat);
> > -		else
> > +			break;
> > +
> > +		case PixelFormatInfo::ColourEncodingYUV:
> > +		case PixelFormatInfo::ColourEncodingRGB:
> > +			/*
> > +			 * We support enumerating RGB streams here to allow
> > +			 * mapping IMPLEMENTATION_DEFINED format to RGB.
> > +			 */
>
> Thanks for the comment, that makes me feel better.
>

:)

> >  			resolutions = initializeYUVResolutions(mappedFormat,
> >  							       cameraResolutions);
> > +			break;
> > +
> > +		default:
> > +			continue;
>
> Do you get a warning if you don't add the default case ? If not, I'd
> drop it, in order to get a warning if we ever add another colour
> encoding.
>

No I don't, but I thought it would be cleaner to just continue, as
'resolutions' would be empty and there's no point in going to the
following loop. But your argument about getting a warning if we forget
a format is more solid, so I'll drop

> > +		}
> >
> >  		for (const Size &res : resolutions) {
> >  			streamConfigurations_.push_back({ res, androidFormat });
> > @@ -866,22 +884,11 @@ int CameraCapabilities::initializeStaticMetadata()
> >  	};
> >
> >  	/* Report if camera supports RAW. */
> > -	bool rawStreamAvailable = false;
> > -	std::unique_ptr<CameraConfiguration> cameraConfig =
> > -		camera_->generateConfiguration({ StreamRole::Raw });
> > -	if (cameraConfig && !cameraConfig->empty()) {
> > -		const PixelFormatInfo &info =
> > -			PixelFormatInfo::info(cameraConfig->at(0).pixelFormat);
> > -		/* Only advertise RAW support if RAW16 is possible. */
> > -		if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW &&
> > -		    info.bitsPerPixel == 16) {
> > -			rawStreamAvailable = true;
> > +	if (rawStreamAvailable_)
> >  			availableCapabilities.push_back(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_RAW);
>
> With one less indentation level,
>

Ouch, I had missed it, sorry!

> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Thanks
   j

>
> > -		}
> > -	}
> >
> >  	/* Number of { RAW, YUV, JPEG } supported output streams */
> > -	int32_t numOutStreams[] = { rawStreamAvailable, 2, 1 };
> > +	int32_t numOutStreams[] = { rawStreamAvailable_, 2, 1 };
> >  	staticMetadata_->addEntry(ANDROID_REQUEST_MAX_NUM_OUTPUT_STREAMS,
> >  				  numOutStreams);
> >
> > diff --git a/src/android/camera_capabilities.h b/src/android/camera_capabilities.h
> > index e7aa46c0a689..42a976d3b482 100644
> > --- a/src/android/camera_capabilities.h
> > +++ b/src/android/camera_capabilities.h
> > @@ -55,6 +55,7 @@ private:
> >
> >  	int facing_;
> >  	int orientation_;
> > +	bool rawStreamAvailable_;
> >
> >  	std::vector<Camera3StreamConfiguration> streamConfigurations_;
> >  	std::map<int, libcamera::PixelFormat> formatsMap_;
>
> --
> Regards,
>
> Laurent Pinchart

Patch
diff mbox series 

diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp
index 15e54192adff..709bfb2a4a19 100644
--- a/src/android/camera_capabilities.cpp
+++ b/src/android/camera_capabilities.cpp
@@ -122,6 +122,7 @@  int CameraCapabilities::initialize(std::shared_ptr<libcamera::Camera> camera,
 	camera_ = camera;
 	orientation_ = orientation;
 	facing_ = facing;
+	rawStreamAvailable_ = false;
 
 	/* Acquire the camera and initialize available stream configurations. */
 	int ret = camera_->acquire();
@@ -324,11 +325,28 @@  int CameraCapabilities::initializeStreamConfigurations()
 
 		std::vector<Size> resolutions;
 		const PixelFormatInfo &info = PixelFormatInfo::info(mappedFormat);
-		if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW)
+		switch (info.colourEncoding) {
+		case PixelFormatInfo::ColourEncodingRAW:
+			if (info.bitsPerPixel != 16)
+				continue;
+
+			rawStreamAvailable_ = true;
 			resolutions = initializeRawResolutions(mappedFormat);
-		else
+			break;
+
+		case PixelFormatInfo::ColourEncodingYUV:
+		case PixelFormatInfo::ColourEncodingRGB:
+			/*
+			 * We support enumerating RGB streams here to allow
+			 * mapping IMPLEMENTATION_DEFINED format to RGB.
+			 */
 			resolutions = initializeYUVResolutions(mappedFormat,
 							       cameraResolutions);
+			break;
+
+		default:
+			continue;
+		}
 
 		for (const Size &res : resolutions) {
 			streamConfigurations_.push_back({ res, androidFormat });
@@ -866,22 +884,11 @@  int CameraCapabilities::initializeStaticMetadata()
 	};
 
 	/* Report if camera supports RAW. */
-	bool rawStreamAvailable = false;
-	std::unique_ptr<CameraConfiguration> cameraConfig =
-		camera_->generateConfiguration({ StreamRole::Raw });
-	if (cameraConfig && !cameraConfig->empty()) {
-		const PixelFormatInfo &info =
-			PixelFormatInfo::info(cameraConfig->at(0).pixelFormat);
-		/* Only advertise RAW support if RAW16 is possible. */
-		if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW &&
-		    info.bitsPerPixel == 16) {
-			rawStreamAvailable = true;
+	if (rawStreamAvailable_)
 			availableCapabilities.push_back(ANDROID_REQUEST_AVAILABLE_CAPABILITIES_RAW);
-		}
-	}
 
 	/* Number of { RAW, YUV, JPEG } supported output streams */
-	int32_t numOutStreams[] = { rawStreamAvailable, 2, 1 };
+	int32_t numOutStreams[] = { rawStreamAvailable_, 2, 1 };
 	staticMetadata_->addEntry(ANDROID_REQUEST_MAX_NUM_OUTPUT_STREAMS,
 				  numOutStreams);
 
diff --git a/src/android/camera_capabilities.h b/src/android/camera_capabilities.h
index e7aa46c0a689..42a976d3b482 100644
--- a/src/android/camera_capabilities.h
+++ b/src/android/camera_capabilities.h
@@ -55,6 +55,7 @@  private:
 
 	int facing_;
 	int orientation_;
+	bool rawStreamAvailable_;
 
 	std::vector<Camera3StreamConfiguration> streamConfigurations_;
 	std::map<int, libcamera::PixelFormat> formatsMap_;