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

[Jacopo Mondi]

Hi Laurent,

On Mon, Jul 26, 2021 at 11:42:41AM +0300, Laurent Pinchart wrote:
> 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.
>

well, it's just about using a different member of the
camera_metadata_ro_entry_t.data union. Or do you expect more ?

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

Considering how small that part is I thought having it in a way nicer
to read was worth the extra code inserted in the calling place.
Anyway, we're really talking about details, I'm just sorry for Paul
which will have to backtrack due to being misleaded by my suggestion.

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