[libcamera-devel] [PATCH 5/8] android: camera_device: Calculate metadata size

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Tue, May 26, 2020 at 04:22:34PM +0200, Jacopo Mondi wrote:
> As we move to have more and more dynamically generated static metadata
> entries, the size of the metadata buffer has to be calculated
> dynamically inspecting the information collected from the camera.
> 
> Provide a method to perform metadata buffers size calculation and
> use it when generating camera static metadata.
> 
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  src/android/camera_device.cpp | 42 ++++++++++++++++++++++++++++++-----
>  src/android/camera_device.h   |  2 ++
>  2 files changed, 38 insertions(+), 6 deletions(-)
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index 534bfb1df1ef..6cc377820210 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -331,6 +331,40 @@ void CameraDevice::setCallbacks(const camera3_callback_ops_t *callbacks)
>  /*
>   * Return static information for the camera.
>   */
> +std::pair<uint32_t, uint32_t> CameraDevice::calculateStaticMetadataSize()
> +{
> +	/*
> +	 * \todo Keep this in sync with the actual number of entries.
> +	 * Currently: 50 entries, 647 bytes of static metadata
> +	 */
> +	std::pair<uint32_t, uint32_t> metadataSize;
> +	metadataSize.first = 50;
> +	metadataSize.second = 647;
> +

Do you think we will end up reworking the CameraMetadata class to avoid
pre-allocation ?

> +	/*
> +	 * Calculate space occupation in bytes for dynamically built metadata
> +	 * entries.
> +	 */
> +
> +	/* std::forward_list does not provide a size() method :( */

Let's make it a vector :-)

> +	for (const auto &entry : streamConfigurations_) {
> +		/* Just please the compiler, otherwise entry is not used. */
> +		switch (entry.androidScalerCode) {
> +		default:
> +			break;
> +		}

This hack can be dropped if you use a vector.

> +
> +		/*
> +		 * 4 32bits integers for ANDROID_SCALER_AVAILABLE_STREAM_CONFIGURATIONS
> +		 * 1 32bits integer for ANDROID_SCALER_AVAILABLE_FORMATS
> +		 * 4 64bits integers for ANDROID_SCALER_AVAILABLE_MIN_FRAME_DURATIONS
> +		 */
> +		metadataSize.second += 52;
> +	}

We currently have 700 bytes, with a comment stating 666 bytes, and the
code here will return 699 bytes when there's a single stream
configuration. Which of those, if any, is correct ? :-) Could you add a
note about this to the commit message ?

> +
> +	return metadataSize;
> +}
> +
>  const camera_metadata_t *CameraDevice::getStaticMetadata()
>  {
>  	if (staticMetadata_)
> @@ -341,12 +375,8 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()
>  	 * example application, but a real camera implementation will require
>  	 * more.
>  	 */
> -
> -	/*
> -	 * \todo Keep this in sync with the actual number of entries.
> -	 * Currently: 50 entries, 666 bytes
> -	 */
> -	staticMetadata_ = new CameraMetadata(50, 700);
> +	const std::pair<uint32_t, uint32_t> sizes = calculateStaticMetadataSize();
> +	staticMetadata_ = new CameraMetadata(sizes.first, sizes.second);

Returning a pair is confusing, we could easily swap the first and second
arguments. Creating a struct would be best, but maybe using a std::tuple
could be an acceptable solution as you could write

	uint32_t numEntries;
	uint32_t numBytes;
	std::tie(numEntries, numBytes) = calculateStaticMetadataSize();

There's still a possibility of swapping the arguments, but at least
they're not called first and second.

>  	if (!staticMetadata_->isValid()) {
>  		LOG(HAL, Error) << "Failed to allocate static metadata";
>  		delete staticMetadata_;
> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> index 95bd39f590ab..f22a6e3e6c28 100644
> --- a/src/android/camera_device.h
> +++ b/src/android/camera_device.h
> @@ -10,6 +10,7 @@
>  #include <forward_list>
>  #include <map>
>  #include <memory>
> +#include <utility>
>  
>  #include <hardware/camera3.h>
>  
> @@ -69,6 +70,7 @@ private:
>  	};
>  
>  	int initializeFormats();
> +	std::pair<uint32_t, uint32_t> calculateStaticMetadataSize();
>  	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,

-- 
Regards,

Laurent Pinchart