[libcamera-devel] [PATCH v5 2/9] android: metadata: Add hasEntry and entryContains

[Laurent Pinchart]

Hi Jacopo,

On Mon, Jul 26, 2021 at 10:17:07AM +0200, Jacopo Mondi wrote:
> On Sun, Jul 25, 2021 at 03:37:42AM +0300, Laurent Pinchart wrote:
> > On Tue, Jul 20, 2021 at 07:13:00PM +0900, Paul Elder wrote:
> > > Add convenience functions for checking if an entry is present in a
> > > CameraMetadata instance, and to check if an array entry includes a
> > > specific value.
> > >
> > > Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> > >
> > > ---
> > > Changes in v5:
> > > - refactor template code to reduce the footprint of specialized template
> > >   code
> > >
> > > New in v4
> > > ---
> > >  src/android/camera_metadata.cpp | 18 ++++++++++++++++++
> > >  src/android/camera_metadata.h   | 14 ++++++++++++++
> > >  2 files changed, 32 insertions(+)
> > >
> > > diff --git a/src/android/camera_metadata.cpp b/src/android/camera_metadata.cpp
> > > index 3fc7cf27..ac86c5fc 100644
> > > --- a/src/android/camera_metadata.cpp
> > > +++ b/src/android/camera_metadata.cpp
> > > @@ -121,6 +121,24 @@ bool CameraMetadata::resize(size_t count, size_t size)
> > >  	return true;
> > >  }
> > >
> > > +template<>
> > > +bool CameraMetadata::entryContainsOne<uint8_t>(const camera_metadata_ro_entry_t &entry,
> > > +					       uint8_t value) const
> > > +{
> > > +	for (unsigned int i = 0; i < entry.count; i++) {
> > > +		if (entry.data.u8[i] == value)
> > > +			return true;
> > > +	}
> > > +
> > > +	return false;
> > > +}
> >
> > You'll get a linker error if you call entryContains() with a type
> > different than uint8_t. Is there a use case for that ?
> 
> Why is this an issue ? As one needs more specialization they will be
> added, otherwise should the function be specialized for all types even
> if not used ? There are not many many types so that could be done
> already, but I don't think it's strictly necessary.

Not necessarily, but I'd like to make sure I understand the foreseen use
cases in order to review this correctly.

> > > +
> > > +bool CameraMetadata::hasEntry(uint32_t tag) const
> > > +{
> > > +	camera_metadata_ro_entry_t entry;
> > > +	return getEntry(tag, &entry);
> > > +}
> > > +
> > >  bool CameraMetadata::addEntry(uint32_t tag, const void *data, size_t count,
> > >  			      size_t elementSize)
> > >  {
> > > diff --git a/src/android/camera_metadata.h b/src/android/camera_metadata.h
> > > index 3b7c9e24..72ba4db2 100644
> > > --- a/src/android/camera_metadata.h
> > > +++ b/src/android/camera_metadata.h
> > > @@ -29,6 +29,20 @@ public:
> > >  	bool isValid() const { return valid_; }
> > >  	bool getEntry(uint32_t tag, camera_metadata_ro_entry_t *entry) const;
> > >
> > > +	template<typename T>
> > > +	bool entryContainsOne(const camera_metadata_ro_entry_t &entry, T value) const;
> >
> > Unless you intend for this to be called directly, let's make it private.
> >
> > > +
> > > +	template<typename T> bool entryContains(uint32_t tag, T value) const
> > > +	{
> > > +		camera_metadata_ro_entry_t entry;
> > > +		if (!getEntry(tag, &entry))
> > > +			return false;
> > > +
> > > +		return entryContainsOne<T>(entry, value);
> > > +	}
> >
> > So the part you inline is the one that doesn't depend on the type T, and
> > the part you don't inline is that one that does ? That seems a bit
> > backwards, I'd move merge the two functions and move all the code to the
> > .cpp file.
> 
> I suggested Paul to inline the common part and only specialize the
> type-dependent part. That's why I was asking about the memory
> footprint.
> 
> How would you do it backwards ?
> 
> Otherwise yes, merge all in the .cpp file but be aware that the
> entryContains() function content will be replicated in each
> specialization ?

Isn't it better than replicating it in each call site in the binary ?
:-) Especially if we don't expect further specializations.

> > > +
> > > +	bool hasEntry(uint32_t tag) const;
> > > +
> > >  	template<typename T,
> > >  		 std::enable_if_t<std::is_arithmetic_v<T>> * = nullptr>
> > >  	bool addEntry(uint32_t tag, const T &data)

-- 
Regards,

Laurent Pinchart