[libcamera-devel] [PATCH v3 3/3] android: capabilities: Centralize RAW support check

[Jacopo Mondi]

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 at jmondi.org>
> > Reviewed-by: Umang Jain <umang.jain at 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 at 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