[libcamera-devel] [PATCH v7 3/3] android: camera_device: Reorder configurations before requesting

[Laurent Pinchart]

Hi Hiro,

Thank you for the patch.

On Fri, Dec 11, 2020 at 09:53:36AM +0000, Hirokazu Honda wrote:
> This reorders Camera3Configs before executing
> CameraConfiguration::validate() to make it easier for the Camera
> to satisfy the Android framework request.
> 
> Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> Reviewed-by: Umang Jain <email at uajain.com>
> ---
>  src/android/camera_device.cpp | 110 +++++++++++++++++++++++++++++++++-
>  1 file changed, 108 insertions(+), 2 deletions(-)
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index 7e8b2818..0c58c1c5 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -27,6 +27,8 @@
> 
>  using namespace libcamera;
> 
> +LOG_DECLARE_CATEGORY(HAL)
> +
>  namespace {
> 
>  /*
> @@ -144,9 +146,112 @@ struct Camera3StreamConfig {
>  	std::vector<Camera3Stream> streams;
>  	StreamConfiguration config;
>  };
> -} /* namespace */
> 
> -LOG_DECLARE_CATEGORY(HAL)
> +/*
> + * Reorder the configurations so that libcamera::Camera can accept them as much
> + * as possible. The sort rule is as follows.
> + * 1.) The configuration for NV12 request whose resolution is the largest.
> + * 2.) The configuration for JPEG request.
> + * 3.) Others. Larger resolutions and different formats are put earlier.
> + */
> +void sortCamera3StreamConfigs(std::vector<Camera3StreamConfig> &unsortedConfigs,
> +			      const camera3_stream_t *jpegStream)
> +{
> +	const Camera3StreamConfig *jpegConfig = nullptr;
> +
> +	std::map<PixelFormat, std::vector<const Camera3StreamConfig *>> formatToConfigs;
> +	for (const auto &streamConfig : unsortedConfigs) {
> +		if (jpegStream && !jpegConfig) {
> +			const auto &streams = streamConfig.streams;
> +			if (std::find_if(streams.begin(), streams.end(),
> +					 [jpegStream](const auto &stream) {
> +						 return stream.stream == jpegStream;
> +					 }) != streams.end()) {
> +				jpegConfig = &streamConfig;
> +				continue;
> +			}
> +		}
> +		formatToConfigs[streamConfig.config.pixelFormat].push_back(&streamConfig);
> +	}
> +	if (jpegStream && !jpegConfig)
> +		LOG(HAL, Fatal) << "No Camera3StreamConfig is found for JPEG";
> +
> +	for (auto &[format, streamConfigs] : formatToConfigs) {

This breaks compilation on gcc 7 :-(

../../src/android/camera_device.cpp: In function ‘void {anonymous}::sortCamera3StreamConfigs(std::vector<{anonymous}::Camera3StreamConfig>&, const camera3_stream_t*)’:
../../src/android/camera_device.cpp:179:35: error: unused variable ‘format’ [-Werror=unused-variable]
  for (auto &[format, streamConfigs] : formatToConfigs) {

gcc 7 doesn't seem to support [[maybe_unused]] for structured bindings,
so the only option that seem to work is

	for (auto &fmt : formatToConfigs) {
		auto &streamConfigs = fmt.second;

> +		/* Sorted by resolution. Smaller is put first. */
> +		std::sort(streamConfigs.begin(), streamConfigs.end(),
> +			  [](const auto *streamConfigA, const auto *streamConfigB) {
> +				  const Size &sizeA = streamConfigA->config.size;
> +				  const Size &sizeB = streamConfigB->config.size;
> +				  return sizeA < sizeB;
> +			  });
> +	}
> +
> +	std::vector<Camera3StreamConfig> sortedConfigs;
> +	sortedConfigs.reserve(unsortedConfigs.size());
> +
> +	/*
> +	 * NV12 is the most prioritized format. Put the configuration with NV12
> +	 * and the largest resolution first.
> +	 */
> +	const auto nv12It = formatToConfigs.find(formats::NV12);
> +	if (nv12It != formatToConfigs.end()) {
> +		auto &nv12Configs = nv12It->second;
> +		const auto &nv12Largest = nv12Configs.back();

The use of auto makes it more difficult to read the code, as one has to
look up the variables types. auto has its uses, as some types are just
too long to type out explicitly (like for nv12It for instance, which
would be std::map<PixelFormat, std::vector<const Camera3StreamConfig *>>::iterator),
but for nv12Largest typing out

		const Camera3StreamConfig *nv12Largest = nv12Configs.back();

isn't too bad, and improves readability I think. This is nothing that
has to be addressed now, but could we keep this in mind for the future ?

> +
> +		/*
> +		 * If JPEG will be created from NV12 and the size is larger than
> +		 * the largest NV12 configurations, then put the NV12
> +		 * configuration for JPEG first.
> +		 */
> +		if (jpegConfig && jpegConfig->config.pixelFormat == formats::NV12) {
> +			const Size &nv12SizeForJpeg = jpegConfig->config.size;
> +			const Size &nv12LargestSize = nv12Largest->config.size;
> +
> +			if (nv12LargestSize < nv12SizeForJpeg) {
> +				LOG(HAL, Debug) << "Insert " << jpegConfig->config.toString();
> +				sortedConfigs.push_back(std::move(*jpegConfig));
> +				jpegConfig = nullptr;
> +			}
> +		}
> +
> +		LOG(HAL, Debug) << "Insert " << nv12Largest->config.toString();
> +		sortedConfigs.push_back(*nv12Largest);
> +		nv12Configs.pop_back();
> +
> +		if (nv12Configs.empty())
> +			formatToConfigs.erase(nv12It);
> +	}
> +
> +	/* If the configuration for JPEG is there, then put it. */
> +	if (jpegConfig) {
> +		LOG(HAL, Debug) << "Insert " << jpegConfig->config.toString();
> +		sortedConfigs.push_back(std::move(*jpegConfig));
> +		jpegConfig = nullptr;
> +	}
> +
> +	/*
> +	 * Put configurations with different formats and larger resolutions
> +	 * earlier.
> +	 */
> +	while (!formatToConfigs.empty()) {
> +		for (auto it = formatToConfigs.begin(); it != formatToConfigs.end();) {
> +			auto &configs = it->second;
> +			LOG(HAL, Debug) << "Insert " << configs.back()->config.toString();
> +			sortedConfigs.push_back(*configs.back());
> +			configs.pop_back();
> +
> +			if (configs.empty())
> +				it = formatToConfigs.erase(it);
> +			else
> +				it++;
> +		}
> +	}
> +
> +	ASSERT(sortedConfigs.size() == unsortedConfigs.size());
> +
> +	unsortedConfigs = sortedConfigs;
> +}

I've added a blank line here.

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

And pushed with the compilation fix and the blank line. Thanks for
bearing with us and the lengthy review. I'll do my best to review your
future patches faster.

> +} /* namespace */
> 
>  MappedCamera3Buffer::MappedCamera3Buffer(const buffer_handle_t camera3buffer,
>  					 int flags)
> @@ -1356,6 +1461,7 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>  		streamConfigs[index].streams.push_back({ jpegStream, type });
>  	}
> 
> +	sortCamera3StreamConfigs(streamConfigs, jpegStream);
>  	for (const auto &streamConfig : streamConfigs) {
>  		config_->addConfiguration(streamConfig.config);
> 

-- 
Regards,

Laurent Pinchart