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

[Jacopo Mondi]

Hi Laurent,

On Thu, Jun 04, 2020 at 05:39:42AM +0300, Laurent Pinchart wrote:
> 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 ?
>

How so ? If I'm not mistaken the Android camera_metadata library we
wrap requires pre-allocation.

We could add all metadata one by one to a temporary storage in CameraMetadata,
and calculate the size requirements as we add them. When done we could
write them to camera_metadata... I'm not sure it's worth the effort
though..

> > +	/*
> > +	 * 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.
>

Yes indeed, much better

> > +
> > +		/*
> > +		 * 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

666 ( \m/ ) was mentioned in a removed comment as well as 700 bytes (I
guess we were 'larger' on purpose).

> 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 ?

What's now reported is that we currently have 50 entries and 647 bytes
of static information, plus 52 bytes for each stream configuration
entry. Not sure what's wrong there.

>
> > +
> > +	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.

Well, that's not API code, but if it makes you sleep more peacefully I
could try with a tuple.

>
> >  	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