[libcamera-devel] [PATCH 3/7] android: camera_metadata: Add appendEntry helper

[paul.elder at ideasonboard.com]

Hi Kieran,

On Thu, Nov 25, 2021 at 11:05:13AM +0000, Kieran Bingham wrote:
> Quoting Paul Elder (2021-11-23 10:40:38)
> > Add appendEntry() helper, that automatically detects if updateEntry() or
> > addEntry() should be used.
> > 
> > For now only implement it for enums and arithmetic values, as they will
> > mainly be used in populating templates, where the preview template
> > generator may or may not have (based on capabilities) already added a
> > key.
> 
> I'm curious, should this always be the case? or is it better to be
> explicit to only use update when we know it could already be there to
> ensure efficiencies.

updateEntry() will fail if the entry is not yet added, and addEntry()
will fail if the entry is already added. It's not a nice noisy failure
either, you get weird behavior and scratch your head like "wtf I added
this key with this value what are you talking about".

> 
> Otherwise it might be simpler to add this check into addEntry()
> directly?

I considered it tbh. But decided against it for efficiency reasons (that
were never really quantitatively assesed).

> 
> I assume we never want a tag to be added multiple times?

Yeah, it'll fail.

> 
> If this is explicitly separated for efficiency reasons then:

It's for correctness. For keys up to now, the preview template generator
would *always* add a key, and the other template generators, that use
the preview template as a base, could reliably use updateEntry. Now we
can't anymore, since the preview template generator would do if
(capability) { addEntry() }, so the later template generators can't
guarantee updateEntry() is the correct function.

I thought I explained this in the commit message :/

Clearly it wasn't sufficient. I'll add a line about correctness with
addEntry and updateEntry.

> 
> 
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>


Thanks,

Paul

> 
> > 
> > Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> > ---
> >  src/android/camera_metadata.h | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> > 
> > diff --git a/src/android/camera_metadata.h b/src/android/camera_metadata.h
> > index 8555c7c3..cca14d6c 100644
> > --- a/src/android/camera_metadata.h
> > +++ b/src/android/camera_metadata.h
> > @@ -33,6 +33,17 @@ public:
> >  
> >         bool hasEntry(uint32_t tag) const;
> >  
> > +       template<typename T,
> > +                std::enable_if_t<std::is_arithmetic_v<T> ||
> > +                                 std::is_enum_v<T>> * = nullptr>
> > +       bool appendEntry(uint32_t tag, const T &data)
> > +       {
> > +               if (hasEntry(tag))
> > +                       return updateEntry(tag, &data, 1, sizeof(T));
> > +               else
> > +                       return addEntry(tag, &data, 1, sizeof(T));
> > +       }
> > +
> >         template<typename T,
> >                  std::enable_if_t<std::is_arithmetic_v<T> ||
> >                                   std::is_enum_v<T>> * = nullptr>
> > -- 
> > 2.27.0
> >