[libcamera-devel,RFC] libcamera: Request: Add setHalfLife

11805 diff mbox series

From the perspective of a Buffer, a Request can suddenly become nullptr
in the middle of a stream. As Requests are supposed to be atomic, and
they are the nucleus of the libcamera camera streaming API, it would be
convenient if we could set the rate of decay of Requests.

Add a half life member variable to Request, and a setHalfLife() member
function to set this. This will affect whether the Request is reusable
or not, so Request::reuse() can now fail.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>

---
TODO: change the applications to reuse the request if reuse() fails
---
 include/libcamera/request.h |  8 +++++++-
 src/libcamera/request.cpp   | 36 ++++++++++++++++++++++++++++++++++--
 2 files changed, 41 insertions(+), 3 deletions(-)

Hey Paul,

Thank you for the patch.

I have a few questions, which are mostly related to helping understand
the issue a bit better.

On 01.04.2021 12:46, Paul Elder wrote:
>From the perspective of a Buffer, a Request can suddenly become nullptr
>in the middle of a stream. As Requests are supposed to be atomic, and
>they are the nucleus of the libcamera camera streaming API, it would be
>convenient if we could set the rate of decay of Requests.

So, what exactly affects the decay of a request? And how can I determine
when it will be unusable in advance, as you introduce a method called
`setHalfLife`?

>
>Add a half life member variable to Request, and a setHalfLife() member
>function to set this. This will affect whether the Request is reusable
>or not, so Request::reuse() can now fail.

If I look at the `cam` application for example, we usually take 4
buffers, which means that we create 4 requests and then we simply reuse
those for any amount of frames. If I make it possible for reuse to fail,
then I would have to create a new request, but why do I have to do
that?
I currently don't understand which problems can arise out of re using a
request for too long. I did some tests with the cam application, where I
had 4 buffers -> 4 requests and capture 1000 frames at 7,5 fps. The
capture took 133 seconds, and I was not able to spot any problems with the
requests.

>
>Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
>
>---
>TODO: change the applications to reuse the request if reuse() fails
>---
> include/libcamera/request.h |  8 +++++++-
> src/libcamera/request.cpp   | 36 ++++++++++++++++++++++++++++++++++--
> 2 files changed, 41 insertions(+), 3 deletions(-)
>
>diff --git a/include/libcamera/request.h b/include/libcamera/request.h
>index 4cf5ff3f..45c707f3 100644
>--- a/include/libcamera/request.h
>+++ b/include/libcamera/request.h
>@@ -7,6 +7,7 @@
> #ifndef __LIBCAMERA_REQUEST_H__
> #define __LIBCAMERA_REQUEST_H__
>
>+#include <chrono>
> #include <map>
> #include <memory>
> #include <stdint.h>
>@@ -43,7 +44,7 @@ public:
> 	Request(Camera *camera, uint64_t cookie = 0);
> 	~Request();
>
>-	void reuse(ReuseFlag flags = Default);
>+	bool reuse(ReuseFlag flags = Default);
>
> 	ControlList &controls() { return *controls_; }
> 	ControlList &metadata() { return *metadata_; }
>@@ -57,6 +58,8 @@ public:
>
> 	bool hasPendingBuffers() const { return !pending_.empty(); }
>
>+	void setHalfLife(std::chrono::duration<double> hl);
>+
> 	std::string toString() const;
>
> private:
>@@ -79,6 +82,9 @@ private:
> 	const uint64_t cookie_;
> 	Status status_;
> 	bool cancelled_;
>+
>+	std::chrono::steady_clock::time_point birthTime_;
>+	std::chrono::duration<double> halfLife_;
> };
>
> } /* namespace libcamera */
>diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
>index ce2dd7b1..0543abaf 100644
>--- a/src/libcamera/request.cpp
>+++ b/src/libcamera/request.cpp
>@@ -7,7 +7,9 @@
>
> #include <libcamera/request.h>
>
>+#include <chrono>
> #include <map>
>+#include <random>
> #include <sstream>
>
> #include <libcamera/buffer.h>
>@@ -74,7 +76,9 @@ LOG_DEFINE_CATEGORY(Request)
>  */
> Request::Request(Camera *camera, uint64_t cookie)
> 	: camera_(camera), sequence_(0), cookie_(cookie),
>-	  status_(RequestPending), cancelled_(false)
>+	  status_(RequestPending), cancelled_(false),
>+	  birthTime_(std::chrono::steady_clock::now()),
>+	  halfLife_(std::chrono::seconds(10))
> {
> 	/**
> 	 * \todo Should the Camera expose a validator instance, to avoid
>@@ -111,11 +115,28 @@ Request::~Request()
>  * prior to queueing the request to the camera, in lieu of constructing a new
>  * request. The application can reuse the buffers that were previously added
>  * to the request via addBuffer() by setting \a flags to ReuseBuffers.
>+ *
>+ * The request for reuse may fail, as Requests can decay based on the half
>+ * life.
>+ *
>+ * \return True on success, false otherwise
>  */
>-void Request::reuse(ReuseFlag flags)
>+bool Request::reuse(ReuseFlag flags)
> {
> 	LIBCAMERA_TRACEPOINT(request_reuse, this);
>
>+	std::chrono::steady_clock::time_point end =
>+		std::chrono::steady_clock::now();
>+	std::chrono::duration<double> diff = end - birthTime_;

What do we do with this variable `diff`, I don't see it being used
anywhere?

>+
>+	std::random_device rd{};
>+	std::mt19937 gen{ rd() };
>+	double hlns =
>+		std::chrono::duration_cast<std::chrono::nanoseconds>(halfLife_).count();
>+	std::normal_distribution<> dist{ hlns, hlns / 2 };
>+	if (dist(gen) > hlns)
>+		return false;
>+

So, this code generates a random 32-bit number, so the maximum is
2^32 = 4294967296, if I am not mistaken.
Then we get the halflife time in nano seconds so by default that would
be 10s = 10000000000ns, and then we get a normal distribution of the
random number with a mean of the halflife time and a standard deviation
of halfLife time / 2. And when that result is greater than the halfLife
time in nano seconds, we declare that request to be not reusable.
For me this currently looks like we randomly declare at some point that
a request is not reusable, did I miss something?

Greetings,
Sebastian

> 	pending_.clear();
> 	if (flags & ReuseBuffers) {
> 		for (auto pair : bufferMap_) {
>@@ -133,6 +154,8 @@ void Request::reuse(ReuseFlag flags)
>
> 	controls_->clear();
> 	metadata_->clear();
>+
>+	return true;
> }
>
> /**
>@@ -320,6 +343,15 @@ bool Request::completeBuffer(FrameBuffer *buffer)
> 	return !hasPendingBuffers();
> }
>
>+/**
>+ * \brief Set the half life of the request
>+ * \param[in] hl Half-life of the request
>+ */
>+void Request::setHalfLife(std::chrono::duration<double> hl)
>+{
>+	halfLife_ = hl;
>+}
>+
> /**
>  * \brief Generate a string representation of the Request internals
>  *
>-- 
>2.27.0
>
>_______________________________________________
>libcamera-devel mailing list
>libcamera-devel@lists.libcamera.org
>https://lists.libcamera.org/listinfo/libcamera-devel

Hi Sebastian,

On Thu, Apr 01, 2021 at 07:00:14AM +0200, Sebastian Fricke wrote:
> Hey Paul,
> 
> Thank you for the patch.

Thank you for the review.

> 
> I have a few questions, which are mostly related to helping understand
> the issue a bit better.
> 
> On 01.04.2021 12:46, Paul Elder wrote:
> > From the perspective of a Buffer, a Request can suddenly become nullptr
> > in the middle of a stream. As Requests are supposed to be atomic, and
> > they are the nucleus of the libcamera camera streaming API, it would be
> > convenient if we could set the rate of decay of Requests.
> 
> So, what exactly affects the decay of a request? And how can I determine
> when it will be unusable in advance, as you introduce a method called
> `setHalfLife`?
> 
> > 
> > Add a half life member variable to Request, and a setHalfLife() member
> > function to set this. This will affect whether the Request is reusable
> > or not, so Request::reuse() can now fail.
> 
> If I look at the `cam` application for example, we usually take 4
> buffers, which means that we create 4 requests and then we simply reuse
> those for any amount of frames. If I make it possible for reuse to fail,
> then I would have to create a new request, but why do I have to do
> that?
> I currently don't understand which problems can arise out of re using a
> request for too long. I did some tests with the cam application, where I
> had 4 buffers -> 4 requests and capture 1000 frames at 7,5 fps. The
> capture took 133 seconds, and I was not able to spot any problems with the
> requests.
> 
> > 
> > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > 
> > ---
> > TODO: change the applications to reuse the request if reuse() fails
> > ---
> > include/libcamera/request.h |  8 +++++++-
> > src/libcamera/request.cpp   | 36 ++++++++++++++++++++++++++++++++++--
> > 2 files changed, 41 insertions(+), 3 deletions(-)
> > 
> > diff --git a/include/libcamera/request.h b/include/libcamera/request.h
> > index 4cf5ff3f..45c707f3 100644
> > --- a/include/libcamera/request.h
> > +++ b/include/libcamera/request.h
> > @@ -7,6 +7,7 @@
> > #ifndef __LIBCAMERA_REQUEST_H__
> > #define __LIBCAMERA_REQUEST_H__
> > 
> > +#include <chrono>
> > #include <map>
> > #include <memory>
> > #include <stdint.h>
> > @@ -43,7 +44,7 @@ public:
> > 	Request(Camera *camera, uint64_t cookie = 0);
> > 	~Request();
> > 
> > -	void reuse(ReuseFlag flags = Default);
> > +	bool reuse(ReuseFlag flags = Default);
> > 
> > 	ControlList &controls() { return *controls_; }
> > 	ControlList &metadata() { return *metadata_; }
> > @@ -57,6 +58,8 @@ public:
> > 
> > 	bool hasPendingBuffers() const { return !pending_.empty(); }
> > 
> > +	void setHalfLife(std::chrono::duration<double> hl);
> > +
> > 	std::string toString() const;
> > 
> > private:
> > @@ -79,6 +82,9 @@ private:
> > 	const uint64_t cookie_;
> > 	Status status_;
> > 	bool cancelled_;
> > +
> > +	std::chrono::steady_clock::time_point birthTime_;
> > +	std::chrono::duration<double> halfLife_;
> > };
> > 
> > } /* namespace libcamera */
> > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
> > index ce2dd7b1..0543abaf 100644
> > --- a/src/libcamera/request.cpp
> > +++ b/src/libcamera/request.cpp
> > @@ -7,7 +7,9 @@
> > 
> > #include <libcamera/request.h>
> > 
> > +#include <chrono>
> > #include <map>
> > +#include <random>
> > #include <sstream>
> > 
> > #include <libcamera/buffer.h>
> > @@ -74,7 +76,9 @@ LOG_DEFINE_CATEGORY(Request)
> >  */
> > Request::Request(Camera *camera, uint64_t cookie)
> > 	: camera_(camera), sequence_(0), cookie_(cookie),
> > -	  status_(RequestPending), cancelled_(false)
> > +	  status_(RequestPending), cancelled_(false),
> > +	  birthTime_(std::chrono::steady_clock::now()),
> > +	  halfLife_(std::chrono::seconds(10))
> > {
> > 	/**
> > 	 * \todo Should the Camera expose a validator instance, to avoid
> > @@ -111,11 +115,28 @@ Request::~Request()
> >  * prior to queueing the request to the camera, in lieu of constructing a new
> >  * request. The application can reuse the buffers that were previously added
> >  * to the request via addBuffer() by setting \a flags to ReuseBuffers.
> > + *
> > + * The request for reuse may fail, as Requests can decay based on the half
> > + * life.
> > + *
> > + * \return True on success, false otherwise
> >  */
> > -void Request::reuse(ReuseFlag flags)
> > +bool Request::reuse(ReuseFlag flags)
> > {
> > 	LIBCAMERA_TRACEPOINT(request_reuse, this);
> > 
> > +	std::chrono::steady_clock::time_point end =
> > +		std::chrono::steady_clock::now();
> > +	std::chrono::duration<double> diff = end - birthTime_;
> 
> What do we do with this variable `diff`, I don't see it being used
> anywhere?
> 

Oh oops, I meant to use diff in the comparison with the random number
below. I wonder why I didn't get a compiler error for unused variable :/

> > +
> > +	std::random_device rd{};
> > +	std::mt19937 gen{ rd() };
> > +	double hlns =
> > +		std::chrono::duration_cast<std::chrono::nanoseconds>(halfLife_).count();
> > +	std::normal_distribution<> dist{ hlns, hlns / 2 };
> > +	if (dist(gen) > hlns)
> > +		return false;
> > +
> 
> So, this code generates a random 32-bit number, so the maximum is
> 2^32 = 4294967296, if I am not mistaken.
> Then we get the halflife time in nano seconds so by default that would
> be 10s = 10000000000ns, and then we get a normal distribution of the
> random number with a mean of the halflife time and a standard deviation
> of halfLife time / 2. And when that result is greater than the halfLife
> time in nano seconds, we declare that request to be not reusable.
> For me this currently looks like we randomly declare at some point that
> a request is not reusable, did I miss something?

I think you're missing... the date :)


Thanks,

Paul

> 
> > 	pending_.clear();
> > 	if (flags & ReuseBuffers) {
> > 		for (auto pair : bufferMap_) {
> > @@ -133,6 +154,8 @@ void Request::reuse(ReuseFlag flags)
> > 
> > 	controls_->clear();
> > 	metadata_->clear();
> > +
> > +	return true;
> > }
> > 
> > /**
> > @@ -320,6 +343,15 @@ bool Request::completeBuffer(FrameBuffer *buffer)
> > 	return !hasPendingBuffers();
> > }
> > 
> > +/**
> > + * \brief Set the half life of the request
> > + * \param[in] hl Half-life of the request
> > + */
> > +void Request::setHalfLife(std::chrono::duration<double> hl)
> > +{
> > +	halfLife_ = hl;
> > +}
> > +
> > /**
> >  * \brief Generate a string representation of the Request internals
> >  *
> > -- 
> > 2.27.0
> > 
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel@lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel

Hi Paul,

On 01/04/2021 04:46, Paul Elder wrote:
> From the perspective of a Buffer, a Request can suddenly become nullptr
> in the middle of a stream. As Requests are supposed to be atomic, and
> they are the nucleus of the libcamera camera streaming API, it would be
> convenient if we could set the rate of decay of Requests.
> 
> Add a half life member variable to Request, and a setHalfLife() member
> function to set this. This will affect whether the Request is reusable
> or not, so Request::reuse() can now fail.
> 
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> 
> ---
> TODO: change the applications to reuse the request if reuse() fails
> ---
>  include/libcamera/request.h |  8 +++++++-
>  src/libcamera/request.cpp   | 36 ++++++++++++++++++++++++++++++++++--
>  2 files changed, 41 insertions(+), 3 deletions(-)
> 
> diff --git a/include/libcamera/request.h b/include/libcamera/request.h
> index 4cf5ff3f..45c707f3 100644
> --- a/include/libcamera/request.h
> +++ b/include/libcamera/request.h
> @@ -7,6 +7,7 @@
>  #ifndef __LIBCAMERA_REQUEST_H__
>  #define __LIBCAMERA_REQUEST_H__
>  
> +#include <chrono>
>  #include <map>
>  #include <memory>
>  #include <stdint.h>
> @@ -43,7 +44,7 @@ public:
>  	Request(Camera *camera, uint64_t cookie = 0);
>  	~Request();
>  
> -	void reuse(ReuseFlag flags = Default);
> +	bool reuse(ReuseFlag flags = Default);
>  
>  	ControlList &controls() { return *controls_; }
>  	ControlList &metadata() { return *metadata_; }
> @@ -57,6 +58,8 @@ public:
>  
>  	bool hasPendingBuffers() const { return !pending_.empty(); }
>  
> +	void setHalfLife(std::chrono::duration<double> hl);
> +
>  	std::string toString() const;
>  
>  private:
> @@ -79,6 +82,9 @@ private:
>  	const uint64_t cookie_;
>  	Status status_;
>  	bool cancelled_;
> +
> +	std::chrono::steady_clock::time_point birthTime_;
> +	std::chrono::duration<double> halfLife_;
>  };
>  
>  } /* namespace libcamera */
> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
> index ce2dd7b1..0543abaf 100644
> --- a/src/libcamera/request.cpp
> +++ b/src/libcamera/request.cpp
> @@ -7,7 +7,9 @@
>  
>  #include <libcamera/request.h>
>  
> +#include <chrono>
>  #include <map>
> +#include <random>
>  #include <sstream>
>  
>  #include <libcamera/buffer.h>
> @@ -74,7 +76,9 @@ LOG_DEFINE_CATEGORY(Request)
>   */
>  Request::Request(Camera *camera, uint64_t cookie)
>  	: camera_(camera), sequence_(0), cookie_(cookie),
> -	  status_(RequestPending), cancelled_(false)
> +	  status_(RequestPending), cancelled_(false),
> +	  birthTime_(std::chrono::steady_clock::now()),
> +	  halfLife_(std::chrono::seconds(10))

I think we should reduce this to just one second in four. or four in
one, it depends on your location I think.

Other than that, looks like a great debug addition, lets do it.

Completely-Entirely-Reviewed-by: Kieran Bingham
<kieran.bingham@ideasonboard.com>


>  {
>  	/**
>  	 * \todo Should the Camera expose a validator instance, to avoid
> @@ -111,11 +115,28 @@ Request::~Request()
>   * prior to queueing the request to the camera, in lieu of constructing a new
>   * request. The application can reuse the buffers that were previously added
>   * to the request via addBuffer() by setting \a flags to ReuseBuffers.
> + *
> + * The request for reuse may fail, as Requests can decay based on the half
> + * life.
> + *
> + * \return True on success, false otherwise
>   */
> -void Request::reuse(ReuseFlag flags)
> +bool Request::reuse(ReuseFlag flags)
>  {
>  	LIBCAMERA_TRACEPOINT(request_reuse, this);
>  
> +	std::chrono::steady_clock::time_point end =
> +		std::chrono::steady_clock::now();
> +	std::chrono::duration<double> diff = end - birthTime_;
> +
> +	std::random_device rd{};
> +	std::mt19937 gen{ rd() };
> +	double hlns =
> +		std::chrono::duration_cast<std::chrono::nanoseconds>(halfLife_).count();
> +	std::normal_distribution<> dist{ hlns, hlns / 2 };
> +	if (dist(gen) > hlns)
> +		return false;
> +
>  	pending_.clear();
>  	if (flags & ReuseBuffers) {
>  		for (auto pair : bufferMap_) {
> @@ -133,6 +154,8 @@ void Request::reuse(ReuseFlag flags)
>  
>  	controls_->clear();
>  	metadata_->clear();
> +
> +	return true;
>  }
>  
>  /**
> @@ -320,6 +343,15 @@ bool Request::completeBuffer(FrameBuffer *buffer)
>  	return !hasPendingBuffers();
>  }
>  
> +/**
> + * \brief Set the half life of the request
> + * \param[in] hl Half-life of the request
> + */
> +void Request::setHalfLife(std::chrono::duration<double> hl)
> +{
> +	halfLife_ = hl;
> +}
> +
>  /**
>   * \brief Generate a string representation of the Request internals
>   *
>