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

[Laurent Pinchart]

Hi Jacopo,

On Mon, Jul 26, 2021 at 10:51:28AM +0200, Jacopo Mondi wrote:
> On Fri, Jul 23, 2021 at 04:43:19PM +0300, Laurent Pinchart wrote:
> > On Thu, Jul 15, 2021 at 05:32:41PM +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 nor 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>
> > > ---
> > >  src/android/camera_capabilities.cpp | 23 ++++++++---------------
> > >  src/android/camera_capabilities.h   |  1 +
> > >  2 files changed, 9 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp
> > > index 15e54192adff..9e2714f132c4 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,14 @@ int CameraCapabilities::initializeStreamConfigurations()
> > >
> > >  		std::vector<Size> resolutions;
> > >  		const PixelFormatInfo &info = PixelFormatInfo::info(mappedFormat);
> > > -		if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW)
> > > +		if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW &&
> > > +		    info.bitsPerPixel == 16) {
> > > +			rawStreamAvailable_ = true;
> > >  			resolutions = initializeRawResolutions(mappedFormat);
> > > -		else
> > > +		} else {
> >
> > Shouldn't you skip here the raw formats that have other bpp than 16 ?
> 
> Correct! Raw !16bpp formats should not be enumerated in the else
> branch
> 
> > And probably the RGB formats too ?
> 
> Should we ? The initialization walks the camera3FormatsMap where the
> formats mapping is defined. If for any reason (processed) RGB formats are
> listed there (in example to support IMPLEMENTATION_DEFINED) why would
> we ignore them ?

Indeed. That's probably unlikely, but it may happen. However, calling
initializeYUVResolutions() for an RGB format sounds weird.

> > >  			resolutions = initializeYUVResolutions(mappedFormat,
> > >  							       cameraResolutions);
> > > +		}
> > >
> > >  		for (const Size &res : resolutions) {
> > >  			streamConfigurations_.push_back({ res, androidFormat });
> > > @@ -866,22 +870,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_;

-- 
Regards,

Laurent Pinchart