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

[Kieran Bingham]

One more thought,

On 09/12/2020 11:10, Kieran Bingham wrote:
> Hi Hiro,
> 
> On 09/12/2020 05:54, 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>
>> ---
>>  src/android/camera_device.cpp | 109 +++++++++++++++++++++++++++++++++-
>>  1 file changed, 107 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
>> index b7bf3d88..c9e0ec98 100644
>> --- a/src/android/camera_device.cpp
>> +++ b/src/android/camera_device.cpp
>> @@ -9,6 +9,7 @@
>>  #include "camera_ops.h"
>>  #include "post_processor.h"
>>
>> +#include <optional>
>>  #include <sys/mman.h>
>>  #include <tuple>
>>  #include <vector>
>> @@ -27,6 +28,8 @@
>>
>>  using namespace libcamera;
>>
>> +LOG_DECLARE_CATEGORY(HAL)
>> +
>>  namespace {
>>
>>  /*
>> @@ -140,9 +143,109 @@ struct Camera3StreamConfig {
>>  	std::vector<CameraStream::Type> types;
>>  	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.
>> + */
>> +std::vector<Camera3StreamConfig> sortCamera3StreamConfigs(
>> +	std::vector<Camera3StreamConfig> unsortedConfigs,
>> +	const camera3_stream_t *jpegStream)
>> +{
>> +	const size_t unsortedSize = unsortedConfigs.size();
>> +	std::optional<Camera3StreamConfig> jpegConfig = std::nullopt;
>> +
>> +	if (jpegStream) {
>> +		for (size_t i = 0; i < unsortedSize; ++i) {
>> +			const auto &streams = unsortedConfigs[i].streams;
>> +			if (std::find(streams.begin(), streams.end(),
>> +				      jpegStream) != streams.end()) {
>> +				jpegConfig = std::move(unsortedConfigs[i]);
>> +				unsortedConfigs.erase(unsortedConfigs.begin() + i);
>> +				break;
>> +			}
>> +		}
>> +		if (!jpegConfig)
>> +			LOG(HAL, Fatal) << "No Camera3StreamConfig is found for Jpeg";
>> +	}
>> +
>> +	std::map<uint32_t, std::vector<Camera3StreamConfig>> formatToConfigs;
>> +	for (const auto &streamConfig : unsortedConfigs) {
>> +		const StreamConfiguration &config = streamConfig.config;
>> +		formatToConfigs[config.pixelFormat].push_back(streamConfig);
>> +
>> +	}
>> +	for (auto &[format, streamConfigs] : formatToConfigs) {
>> +		/* 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(unsortedSize);
>> +	/*
>> +	 * 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 Size &nv12LargestSize = nv12Configs.back().config.size;
>> +		/*
>> +		 * 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;
>> +
>> +			if (nv12LargestSize < nv12SizeForJpeg) {
>> +				LOG(HAL, Debug) << "Insert " << jpegConfig->config.toString();
>> +				sortedConfigs.push_back(std::move(*jpegConfig));
>> +				jpegConfig = std::nullopt;
>> +			}
>> +		}
>> +		LOG(HAL, Debug) << "Insert " << nv12Configs.back().config.toString();
>> +		sortedConfigs.push_back(std::move(nv12Configs.back()));
>> +		nv12Configs.pop_back();
>> +	}
>> +
>> +	/* 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 = std::nullopt;
>> +	}
>> +
>> +	/*
>> +	 * Put configurations with different formats and larger resolutions
>> +	 * earlier.
>> +	 */
>> +	while (!formatToConfigs.empty()) {
>> +		for (auto it = formatToConfigs.begin(); it != formatToConfigs.end();) {
>> +			auto& configs = it->second;
>> +			if (configs.empty()) {
>> +				it = formatToConfigs.erase(it);
>> +				continue;
>> +			}
>> +			LOG(HAL, Debug) << "Insert " << configs.back().config.toString();
>> +			sortedConfigs.push_back(std::move(configs.back()));
>> +			configs.pop_back();
>> +			it++;
>> +		}
>> +	}
>> +	assert(sortedConfigs.size() == unsortedSize);
> 
> Should we assert(unsortedConfigs == 0) too?

We use a LOG(Fatal) above, which is an assert with a print...

We could either use that, or we have our own ASSERT macro defined, which
we use throughout libcamera.

But that's an easy change to capitalise to the macro instead, and could
perhaps be done when applying.

--
Kieran



> But I don't think it's a blocker or required.
> 
> The sorting seems like a good idea otherwise.
> 
> 
>> +
>> +	return sortedConfigs;
>> +}
>> +} /* namespace */
>>
>>  MappedCamera3Buffer::MappedCamera3Buffer(const buffer_handle_t camera3buffer,
>>  					 int flags)
>> @@ -1333,6 +1436,8 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>>  		streamConfigs[index].types.push_back(type);
>>  	}
>>
>> +	streamConfigs = sortCamera3StreamConfigs(std::move(streamConfigs),
>> +						 jpegStream);
> 
> It's hard to see here, but I assume we now sort before we infer any
> indexes, so the index remains valid.
> 
> As long as that statement is true,
> 
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> 
> 
> 
>>  	for (const auto &streamConfig : streamConfigs) {
>>  		config_->addConfiguration(streamConfig.config);
>>  		for (size_t i = 0; i < streamConfig.streams.size(); ++i) {
>> --
>> 2.29.2.576.ga3fc446d84-goog
>> _______________________________________________
>> libcamera-devel mailing list
>> libcamera-devel at lists.libcamera.org
>> https://lists.libcamera.org/listinfo/libcamera-devel
>>
> 

-- 
Regards
--
Kieran