| Message ID | 20211123104042.3100902-4-paul.elder@ideasonboard.com |
|---|---|
| State | Superseded |
| Delegated to: | Paul Elder |
| Headers | show |
| Series |
|
| Related | show |
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. Otherwise it might be simpler to add this check into addEntry() directly? I assume we never want a tag to be added multiple times? If this is explicitly separated for efficiency reasons then: Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > Signed-off-by: Paul Elder <paul.elder@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 >
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@ideasonboard.com> Thanks, Paul > > > > > Signed-off-by: Paul Elder <paul.elder@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 > >
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>
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. Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> --- src/android/camera_metadata.h | 11 +++++++++++ 1 file changed, 11 insertions(+)