[libcamera-devel] [PATCH v2 1/4] android: camera_device: Build stream configuration

[Kieran Bingham]

Hi Jacopo,

On 04/06/2020 14:35, Jacopo Mondi wrote:
> Build the stream configuration map by applying the Android Camera3
> requested resolutions and formats to the libcamera Camera device.
> 
> For each required format test a list of required and optional
> resolutions, construct a map to translate from Android format to the
> libcamera formats and store the available stream configuration to
> be provided to the Android framework through static metadata.
> 
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  src/android/camera_device.cpp | 218 ++++++++++++++++++++++++++++++++++
>  src/android/camera_device.h   |  12 ++
>  2 files changed, 230 insertions(+)
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index b30263451b76..0c462f53c9cc 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -8,6 +8,8 @@
>  #include "camera_device.h"
>  #include "camera_ops.h"
>  
> +#include <vector>
> +
>  #include <libcamera/controls.h>
>  #include <libcamera/property_ids.h>
>  
> @@ -15,9 +17,70 @@
>  #include "libcamera/internal/utils.h"
>  
>  #include "camera_metadata.h"
> +#include "system/graphics.h"
>  
>  using namespace libcamera;
>  
> +namespace {
> +
> +/*
> + * \var camera3Resolutions
> + * \brief The list of image resolutions defined as mandatory to be supported by
> + * the Android Camera3 specification
> + */
> +const std::vector<Size> camera3Resolutions = {
> +	{ 320, 240 },
> +	{ 640, 480 },
> +	{ 1280, 720 },
> +	{ 1920, 1080 }
> +};
> +
> +/*
> + * \struct Camera3Format
> + * \brief Data associated with an Android format identifier
> + * \var Camera3Format::libcameraFormats: List of libcamera pixel formats

do you need to prefix Camera3Format:: in the same comment block?

> + * compatible with the Android format
> + * \var Camera3Format::scalerFormat: The format identifier to be reported to the
> + * android framework through the static format configuration map
> + */
> +struct Camera3Format {
> +	std::vector<PixelFormat> libcameraFormats;
> +	camera_metadata_enum_android_scaler_available_formats_t scalerFormat;
> +};
> +
> +/*
> + * \var camera3FormatsMap
> + * \brief Associate Android format code with ancillary data
> + */
> +const std::map<int, const Camera3Format> camera3FormatsMap = {
> +	{
> +		HAL_PIXEL_FORMAT_BLOB, {

Is there no defined MJPEG format for Android HAL ? 'just a blob' :-S


> +			{ PixelFormat(DRM_FORMAT_MJPEG) },
> +			ANDROID_SCALER_AVAILABLE_FORMATS_BLOB
> +		}
> +	},
> +
> +	{
> +		HAL_PIXEL_FORMAT_YCbCr_420_888, {
> +			{ PixelFormat(DRM_FORMAT_NV12), PixelFormat(DRM_FORMAT_NV21) },
> +			ANDROID_SCALER_AVAILABLE_FORMATS_YCbCr_420_888

How is the ordering determined between multiple formats like that ?

I.e. - how does something know what the correct format to choose will be?

Ah ok - I see below they get iterated with calls to validate them....

> +		}
> +	},
> +
> +	/*
> +	 * \todo Translate IMPLEMENTATION_DEFINED inspecting the
> +	 * gralloc usage flag. For now, copy the YCbCr_420 configuration.
> +	 */
> +	{
> +		HAL_PIXEL_FORMAT_IMPLEMENTATION_DEFINED, {
> +			{ PixelFormat(DRM_FORMAT_NV12), PixelFormat(DRM_FORMAT_NV21) },
> +			ANDROID_SCALER_AVAILABLE_FORMATS_YCbCr_420_888
> +		}
> +	},
> +};
> +
> +} /* namespace */
> +
>  LOG_DECLARE_CATEGORY(HAL);
>  
>  /*
> @@ -100,6 +163,161 @@ int CameraDevice::initialize()
>  	if (properties.contains(properties::Rotation))
>  		orientation_ = properties.get(properties::Rotation);
>  
> +	int ret = camera_->acquire();
> +	if (ret) {
> +		LOG(HAL, Error) << "Failed to temporarily acquire the camera";
> +		return ret;
> +	}
> +
> +	ret = initializeFormats();
> +	camera_->release();
> +	return ret;
> +}
> +
> +/*
> + * Initialize the format conversion map to translate from Android format
> + * identifier to libcamera pixel formats and fill in the list of supported
> + * stream configurations to be reported to the Android camera framework through
> + * the static stream configuration metadata.
> + */
> +int CameraDevice::initializeFormats()
> +{
> +	/*
> +	 * Get the maximum output resolutions
> +	 * \todo Get this from the camera properties once defined
> +	 */
> +	std::unique_ptr<CameraConfiguration> cameraConfig =
> +		camera_->generateConfiguration({ StillCapture });
> +	if (!cameraConfig) {
> +		LOG(HAL, Error) << "Failed to get maximum resolution";
> +		return -EINVAL;
> +	}
> +	StreamConfiguration &cfg = cameraConfig->at(0);
> +
> +	/*
> +	 * \todo JPEG - Adjust the maximum available resolution by taking the
> +	 * JPEG encoder requirements into account (alignment and aspect ratio).
> +	 */
> +	const Size maxRes = cfg.size;

Do we already define that a generateConfiguration({ StillCapture })
should return the largest size as a default?

Or should the 'max' size further inspect the cfg.formats() and determine
the largest supported size from there?

> +	LOG(HAL, Debug) << "Maximum supported resolution: " << maxRes.toString();
> +
> +	/*
> +	 * Build the list of supported image resolutions.
> +	 *
> +	 * The resolutions listed in camera3Resolution are mandatory to be
> +	 * supported, up to the camera maximum resolution.
> +	 *
> +	 * Augment the list by adding resolutions calculated from the camera
> +	 * maximum one.
> +	 */
> +	std::vector<Size> cameraResolutions;
> +	std::copy_if(camera3Resolutions.begin(), camera3Resolutions.end(),
> +		     std::back_inserter(cameraResolutions),
> +		     [&](const Size &res) { return res < maxRes; });
> +
> +	/* Camera3 specification suggests to add 1/2 and 1/4 max resolution. */
> +	for (unsigned int divider = 2;; divider <<= 1) {
> +		Size derivedSize{
> +			maxRes.width / divider,
> +			maxRes.height / divider,
> +		};
> +
> +		if (derivedSize.width < 320 ||
> +		    derivedSize.height < 240)
> +			break;
> +
> +		cameraResolutions.push_back(derivedSize);
> +	}
> +	cameraResolutions.push_back(maxRes);
> +
> +	/* Remove duplicated entries from the list of supported resolutions. */
> +	std::sort(cameraResolutions.begin(), cameraResolutions.end());
> +	auto last = std::unique(cameraResolutions.begin(), cameraResolutions.end());
> +	cameraResolutions.erase(last, cameraResolutions.end());

Ah, I had to look that up:
https://en.cppreference.com/w/cpp/algorithm/unique shows you have to
manually erase to the end after a unique. It looks odd in the code if
you don't know that ;-)

> +
> +	/*
> +	 * Build the list of supported camera formats.
> +	 *
> +	 * To each Android format a list of compatible libcamera formats is
> +	 * associated. The first libcamera format that tests successful is added
> +	 * to the format translation map used when configuring the streams.
> +	 * It is then tested against the list of supported camera resolutions to
> +	 * build the stream configuration map reported through the camera static
> +	 * metadata.
> +	 */
> +	for (const auto &format : camera3FormatsMap) {
> +		int androidFormat = format.first;
> +		const Camera3Format &camera3Format = format.second;
> +		const std::vector<PixelFormat> &libcameraFormats =
> +			camera3Format.libcameraFormats;
> +
> +		/*
> +		 * Test the libcamera formats that can produce images
> +		 * compatible with the Android defined format.
> +		 */
> +		PixelFormat mappedFormat{};
> +		for (const PixelFormat &format : libcameraFormats) {

You're aliasing const auto &format, and const PixelFormat &format here...

Not necessarily a problem, but prevents accessing the camera3FormatsMap
entry ...

> +			/* \todo Fixed mapping for JPEG. */

/me takes note....

> +			if (androidFormat == HAL_PIXEL_FORMAT_BLOB) {
> +				mappedFormat = PixelFormat(DRM_FORMAT_MJPEG);
> +				break;
> +			}
> +
> +			/*
> +			 * The stream configuration size can be adjusted,
> +			 * not the pixel format.
> +			 *
> +			 * \todo This could be simplified once all pipeline
> +			 * handlers will report the StreamFormats list of
> +			 * supported formats.
> +			 */
> +			cfg.pixelFormat = format;
> +
> +			CameraConfiguration::Status status = cameraConfig->validate();
> +			if (status != CameraConfiguration::Invalid &&
> +			    cfg.pixelFormat == format) {
> +				mappedFormat = format;
> +				break;
> +			}
> +		}
> +		if (!mappedFormat.isValid()) {
> +			LOG(HAL, Error) << "Failed to map Android format "
> +					<< utils::hex(androidFormat);

Is it worth putting a char * mapping of the android format name in
camera3FormatsMap to be able to easily diagnose which format failed here?

> +			return -EINVAL;
> +		}
> +
> +		/*
> +		 * Record the mapping and then proceed to generate the
> +		 * stream configurations map, by testing the image resolutions.
> +		 */
> +		formatsMap_[androidFormat] = mappedFormat;
> +
> +		for (const Size &res : cameraResolutions) {
> +			cfg.pixelFormat = mappedFormat;
> +			cfg.size = res;
> +
> +			CameraConfiguration::Status status = cameraConfig->validate();
> +			/*
> +			 * Unconditionally report we can produce JPEG.
> +			 *
> +			 * \todo The JPEG stream will be implemented as an
> +			 * HAL-only stream, but some cameras can produce it
> +			 * directly. As of now, claim support for JPEG without
> +			 * inspecting where the JPEG stream is produced.
> +			 */
> +			if (androidFormat != HAL_PIXEL_FORMAT_BLOB &&
> +			    status != CameraConfiguration::Valid)
> +				continue;
> +
> +			streamConfigurations_.push_back({ res, camera3Format.scalerFormat });
> +		}
> +	}
> +
> +	LOG(HAL, Debug) << "Collected stream configuration map: ";
> +	for (const auto &entry : streamConfigurations_)
> +		LOG(HAL, Debug) << "{ " << entry.resolution.toString() << " - "
> +				<< utils::hex(entry.androidScalerCode);
> +
>  	return 0;
>  }
>  
> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> index a87f7623c790..d31b7233e795 100644
> --- a/src/android/camera_device.h
> +++ b/src/android/camera_device.h
> @@ -7,12 +7,15 @@
>  #ifndef __ANDROID_CAMERA_DEVICE_H__
>  #define __ANDROID_CAMERA_DEVICE_H__
>  
> +#include <map>
>  #include <memory>
> +#include <vector>
>  
>  #include <hardware/camera3.h>
>  
>  #include <libcamera/buffer.h>
>  #include <libcamera/camera.h>
> +#include <libcamera/geometry.h>
>  #include <libcamera/request.h>
>  #include <libcamera/stream.h>
>  
> @@ -59,6 +62,12 @@ private:
>  		camera3_stream_buffer_t *buffers;
>  	};
>  
> +	struct Camera3StreamConfiguration {
> +		libcamera::Size resolution;
> +		int androidScalerCode;
> +	};
> +
> +	int initializeFormats();
>  	void notifyShutter(uint32_t frameNumber, uint64_t timestamp);
>  	void notifyError(uint32_t frameNumber, camera3_stream_t *stream);
>  	std::unique_ptr<CameraMetadata> getResultMetadata(int frame_number,
> @@ -75,6 +84,9 @@ private:
>  	std::map<unsigned int, CameraMetadata *> requestTemplates_;
>  	const camera3_callback_ops_t *callbacks_;
>  
> +	std::vector<Camera3StreamConfiguration> streamConfigurations_;
> +	std::map<int, libcamera::PixelFormat> formatsMap_;
> +
>  	int facing_;
>  	int orientation_;
>  };
> 

-- 
Regards
--
Kieran