[libcamera-devel] [PATCH v2 2/9] android: CameraMetadata: Add copy constructor and getEntry

[Laurent Pinchart]

Hi Paul,

Thank you for the patch.

On Thu, Jan 21, 2021 at 12:23:35PM +0100, Jacopo Mondi wrote:
> On Thu, Jan 21, 2021 at 07:15:42PM +0900, Paul Elder wrote:
> > Add a copy constructor and assigment operator to CameraMetadata, as well
> > a constructor from camera_metadata_t. Also add a function getEntry to
> > allow getting metadata entries from CameraMetadata. This allows us to
> > use CameraMetadata for reading from camera_metadata_t.
> >
> > Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> >
> > ---
> > Changes in v2:
> > - getEntry returns bool instead of int
> > ---
> >  src/android/camera_metadata.cpp | 34 +++++++++++++++++++++++++++++++++
> >  src/android/camera_metadata.h   |  5 +++++
> >  2 files changed, 39 insertions(+)
> >
> > diff --git a/src/android/camera_metadata.cpp b/src/android/camera_metadata.cpp
> > index edea48fe..3b98d25c 100644
> > --- a/src/android/camera_metadata.cpp
> > +++ b/src/android/camera_metadata.cpp
> > @@ -19,12 +19,46 @@ CameraMetadata::CameraMetadata(size_t entryCapacity, size_t dataCapacity)
> >  	valid_ = metadata_ != nullptr;
> >  }
> >
> > +CameraMetadata::CameraMetadata(const camera_metadata_t *metadata)
> > +{
> > +	metadata_ = clone_camera_metadata(metadata);
> > +	valid_ = metadata_ != nullptr;
> > +}
> > +
> > +CameraMetadata::CameraMetadata(const CameraMetadata &other)
> > +{
> 
> This can probably simply be:
> 
> CameraMetadata::CameraMetadata(const CameraMetadata &other)
>         : CameraMetadata(other.get())
> {
> }
> 
> > +	metadata_ = clone_camera_metadata(other.get());
> > +	valid_ = metadata_ != nullptr;
> > +}
> > +
> >  CameraMetadata::~CameraMetadata()
> >  {
> >  	if (metadata_)
> >  		free_camera_metadata(metadata_);
> >  }
> >
> > +CameraMetadata &CameraMetadata::operator=(const CameraMetadata &other)
> > +{
> > +	if (this == &other)
> > +		return *this;
> 
> Do copy assignments need to protect agains this unlikely case ?

I had never considered this case. It's unlikely undeed, but handled in
https://en.cppreference.com/w/cpp/language/copy_assignment, it seems a
good idea to play safe.

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>


> All minors:
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> 
> > +
> > +	if (metadata_)
> > +		free_camera_metadata(metadata_);
> > +
> > +	metadata_ = clone_camera_metadata(other.get());
> > +	valid_ = metadata_ != nullptr;
> > +
> > +	return *this;
> > +}
> > +
> > +bool CameraMetadata::getEntry(uint32_t tag, camera_metadata_ro_entry_t *entry) const
> > +{
> > +	if (find_camera_metadata_ro_entry(metadata_, tag, entry))
> > +		return false;
> > +
> > +	return true;
> > +}
> > +
> >  bool CameraMetadata::addEntry(uint32_t tag, const void *data, size_t count)
> >  {
> >  	if (!valid_)
> > diff --git a/src/android/camera_metadata.h b/src/android/camera_metadata.h
> > index 9d047b1b..720b760d 100644
> > --- a/src/android/camera_metadata.h
> > +++ b/src/android/camera_metadata.h
> > @@ -15,9 +15,14 @@ class CameraMetadata
> >  {
> >  public:
> >  	CameraMetadata(size_t entryCapacity, size_t dataCapacity);
> > +	CameraMetadata(const camera_metadata_t *metadata);
> > +	CameraMetadata(const CameraMetadata &other);
> >  	~CameraMetadata();
> >
> > +	CameraMetadata &operator=(const CameraMetadata &other);
> > +
> >  	bool isValid() const { return valid_; }
> > +	bool getEntry(uint32_t tag, camera_metadata_ro_entry_t *entry) const;
> >  	bool addEntry(uint32_t tag, const void *data, size_t data_count);
> >  	bool updateEntry(uint32_t tag, const void *data, size_t data_count);
> >

-- 
Regards,

Laurent Pinchart