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

Thu Apr 1 12:20:32 CEST 2021

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 at 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 at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel