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

[Laurent Pinchart]

Hi Jacopo,

On Thu, Jun 04, 2020 at 02:38:56PM +0200, Jacopo Mondi wrote:
> On Thu, Jun 04, 2020 at 05:39:42AM +0300, Laurent Pinchart wrote:
> > 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..

That's exactly what I meant :-)

I'm also not sure it's worth the effort, it was really an open question.
I'm tempted to get rid of the Android metadata library copy we have in
libcamera, to avoid having to analyze the LGPL-2.1-or-later + Apache-2.0
compatibility. One easy option for that would be to link against the
Android or Chrome OS versions of the code, but that would reduce our
ability to compile-test the Android HAL on a Linux distribution.
Reimplementing the parts we need with an API that would be nicer to use
could also be an option.

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

I'm not saying the new code is wrong, but it matches neither the old
comment, nor the old code, so I was wondering what was going out.

This is annoying to maintain manually :-S

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

I'll aim for just sleeping before even thinking about sleeping
peacefully :-) I see you've tried the tuple, what do you think about it
?

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