[libcamera-devel] [PATCH v2 2/2] android: Check if Stream configurations were generated correctly

[Jacopo Mondi]

Hi Javier,

On Fri, Oct 01, 2021 at 11:15:25PM +0200, Javier Martinez Canillas wrote:
> The libcamera Android Camera HAL generates camera configurations for the
> StillCapture, Raw and ViewFinder stream roles. But there is only a check
> if the configuration generation failed, for the StillCapture stream role.
>
> This could lead to a NULL pointer dereference if a pipeline handler fails
> to generate a default configuration for one of the other two stream roles.

I don't think that's strictly necessary.

The path towards which we get to genreate a config for a role is:

        for (androidFormat : AndroidRequiredFormats) {

                PixelFormat mappedFormat;
                for (libcameraFormat : androidFormat.compatibleFmts) {
                        cameraConfig.pixelFormat = libcameraFormat;

                        status = cameraConfig.validate()l
                        if (status == ok) {
                                mappedFormat = libameraFormat;
                                break;
                        }
                }

                if (!mappedFormat.valid())
                        continue;

                if (mappedFormat == RAW)
                        initializeRawResolutions()

                if (mappedFormat == YUV || mappedFormat == RGB)
                        initializeYUVResolutions()
        }

Hence, if we get to generate a config for a role we know there's a
supported compatible format.

However, as the association between roles and supported pixel formats
is a little fuzzy in pipeline handlers, I guess the better safe than
sorry approach is the correct one

>
> Signed-off-by: Javier Martinez Canillas <javierm at redhat.com>

Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>

> ---
>
> Changes in v2:
> - Fix typos in commit message.
>
>  src/android/camera_capabilities.cpp | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp
> index 87a6e1c6f26..baeedc11500 100644
> --- a/src/android/camera_capabilities.cpp
> +++ b/src/android/camera_capabilities.cpp
> @@ -408,6 +408,11 @@ CameraCapabilities::initializeYUVResolutions(const PixelFormat &pixelFormat,
>  	std::vector<Size> supportedResolutions;
>  	std::unique_ptr<CameraConfiguration> cameraConfig =
>  		camera_->generateConfiguration({ StreamRole::Viewfinder });
> +	if (!cameraConfig) {
> +		LOG(HAL, Error) << "Failed to get supported YUV resolutions";
> +		return supportedResolutions;
> +	}
> +
>  	StreamConfiguration &cfg = cameraConfig->at(0);
>
>  	for (const Size &res : resolutions) {
> @@ -431,11 +436,17 @@ CameraCapabilities::initializeYUVResolutions(const PixelFormat &pixelFormat,
>  std::vector<Size>
>  CameraCapabilities::initializeRawResolutions(const PixelFormat &pixelFormat)
>  {
> +	std::vector<Size> supportedResolutions;
>  	std::unique_ptr<CameraConfiguration> cameraConfig =
>  		camera_->generateConfiguration({ StreamRole::Raw });
> +	if (!cameraConfig) {
> +		LOG(HAL, Error) << "Failed to get supported Raw resolutions";
> +		return supportedResolutions;
> +	}
> +
>  	StreamConfiguration &cfg = cameraConfig->at(0);
>  	const StreamFormats &formats = cfg.formats();
> -	std::vector<Size> supportedResolutions = formats.sizes(pixelFormat);
> +	supportedResolutions = formats.sizes(pixelFormat);

Hiro's right, not required, but I prefer your way as it makes
initializeRawResolutions more similar to initializeYUVResolutions

Up to you!

Thanks
  j


>
>  	return supportedResolutions;
>  }
> --
> 2.31.1
>