[libcamera-devel,v5,4/5] ipa: raspberrypi: Add Merge method to RPiController::Metadata
diff mbox series

Message ID 20210419133451.263733-5-naush@raspberrypi.com
State Superseded
Headers show
Series
  • ipa: raspberrypi: Rate-limit the controller algorithms
Related show

Commit Message

Naushir Patuck April 19, 2021, 1:34 p.m. UTC
Add a new Merge method to the Metadata class. This will move all
key/value pairs between a source and destination metadata object. Once
complete, the source Metadata object will be empty.

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
---
 src/ipa/raspberrypi/controller/metadata.hpp | 12 ++++++++++++
 1 file changed, 12 insertions(+)

Comments

David Plowman April 19, 2021, 1:50 p.m. UTC | #1
Hi Naush

Thanks for implementing this!

On Mon, 19 Apr 2021 at 14:35, Naushir Patuck <naush@raspberrypi.com> wrote:
>
> Add a new Merge method to the Metadata class. This will move all
> key/value pairs between a source and destination metadata object. Once
> complete, the source Metadata object will be empty.
>
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>

Reviewed-by: David Plowman <david.plowman@raspberrypi.com>

Thanks!
David

> ---
>  src/ipa/raspberrypi/controller/metadata.hpp | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/src/ipa/raspberrypi/controller/metadata.hpp b/src/ipa/raspberrypi/controller/metadata.hpp
> index 319f2320fc70..1d3e941b3e52 100644
> --- a/src/ipa/raspberrypi/controller/metadata.hpp
> +++ b/src/ipa/raspberrypi/controller/metadata.hpp
> @@ -75,6 +75,18 @@ public:
>                 return *this;
>         }
>
> +       void Merge(Metadata &other)
> +       {
> +               std::lock_guard<std::mutex> lock(mutex_);
> +               std::lock_guard<std::mutex> other_lock(other.mutex_);
> +
> +               for (auto const &kv: other.data_)
> +                       data_[kv.first] = std::move(kv.second);
> +
> +               /* Render the other object as empty now! */
> +               other.data_.clear();
> +       }
> +
>         template<typename T>
>         T *GetLocked(std::string const &tag)
>         {
> --
> 2.25.1
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Laurent Pinchart April 27, 2021, 7:18 a.m. UTC | #2
Hi Naush,

Thank you for the patch.

On Mon, Apr 19, 2021 at 02:34:50PM +0100, Naushir Patuck wrote:
> Add a new Merge method to the Metadata class. This will move all
> key/value pairs between a source and destination metadata object. Once
> complete, the source Metadata object will be empty.
> 
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> ---
>  src/ipa/raspberrypi/controller/metadata.hpp | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/src/ipa/raspberrypi/controller/metadata.hpp b/src/ipa/raspberrypi/controller/metadata.hpp
> index 319f2320fc70..1d3e941b3e52 100644
> --- a/src/ipa/raspberrypi/controller/metadata.hpp
> +++ b/src/ipa/raspberrypi/controller/metadata.hpp
> @@ -75,6 +75,18 @@ public:
>  		return *this;
>  	}
>  
> +	void Merge(Metadata &other)
> +	{
> +		std::lock_guard<std::mutex> lock(mutex_);
> +		std::lock_guard<std::mutex> other_lock(other.mutex_);
> +
> +		for (auto const &kv: other.data_)
> +			data_[kv.first] = std::move(kv.second);

Can't you use

		data_.merge(other.data_);

N

> +
> +		/* Render the other object as empty now! */
> +		other.data_.clear();
> +	}
> +
>  	template<typename T>
>  	T *GetLocked(std::string const &tag)
>  	{
Naushir Patuck April 27, 2021, 9:26 a.m. UTC | #3
Hi Laurent,

Thank you for your review feedback.

On Tue, 27 Apr 2021 at 08:18, Laurent Pinchart <
laurent.pinchart@ideasonboard.com> wrote:

> Hi Naush,
>
> Thank you for the patch.
>
> On Mon, Apr 19, 2021 at 02:34:50PM +0100, Naushir Patuck wrote:
> > Add a new Merge method to the Metadata class. This will move all
> > key/value pairs between a source and destination metadata object. Once
> > complete, the source Metadata object will be empty.
> >
> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > ---
> >  src/ipa/raspberrypi/controller/metadata.hpp | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> >
> > diff --git a/src/ipa/raspberrypi/controller/metadata.hpp
> b/src/ipa/raspberrypi/controller/metadata.hpp
> > index 319f2320fc70..1d3e941b3e52 100644
> > --- a/src/ipa/raspberrypi/controller/metadata.hpp
> > +++ b/src/ipa/raspberrypi/controller/metadata.hpp
> > @@ -75,6 +75,18 @@ public:
> >               return *this;
> >       }
> >
> > +     void Merge(Metadata &other)
> > +     {
> > +             std::lock_guard<std::mutex> lock(mutex_);
> > +             std::lock_guard<std::mutex> other_lock(other.mutex_);
> > +
> > +             for (auto const &kv: other.data_)
> > +                     data_[kv.first] = std::move(kv.second);
>
> Can't you use
>
>                 data_.merge(other.data_);
>

Sadly not.  std::map::merge() will not overwrite elements that are present
in the destination, which is what I want.  None of the insert type methods
do actually.  I toyed with doing something like:

other.data_.merge(data_);
std::swap(other.data_, data_);

which would achieve what I want, but thought it might look a bit cryptic.
Not sure about any performance benefits from using this construct vs
the explicit loop.

Perhaps I should be a bit clearer with the method name, and call it
Overwrite()?  Thoughts?

Regards,
Naush



>
> N
>
> > +
> > +             /* Render the other object as empty now! */
> > +             other.data_.clear();
> > +     }
> > +
> >       template<typename T>
> >       T *GetLocked(std::string const &tag)
> >       {
>
> --
> Regards,
>
> Laurent Pinchart
>

Patch
diff mbox series

diff --git a/src/ipa/raspberrypi/controller/metadata.hpp b/src/ipa/raspberrypi/controller/metadata.hpp
index 319f2320fc70..1d3e941b3e52 100644
--- a/src/ipa/raspberrypi/controller/metadata.hpp
+++ b/src/ipa/raspberrypi/controller/metadata.hpp
@@ -75,6 +75,18 @@  public:
 		return *this;
 	}
 
+	void Merge(Metadata &other)
+	{
+		std::lock_guard<std::mutex> lock(mutex_);
+		std::lock_guard<std::mutex> other_lock(other.mutex_);
+
+		for (auto const &kv: other.data_)
+			data_[kv.first] = std::move(kv.second);
+
+		/* Render the other object as empty now! */
+		other.data_.clear();
+	}
+
 	template<typename T>
 	T *GetLocked(std::string const &tag)
 	{