[libcamera-devel,v5,05/10] libcamera: request: Add metadata information
diff mbox series

Message ID 20191008004534.1585142-6-niklas.soderlund@ragnatech.se
State Superseded
Headers show
Series
  • libcamera: ipa: Add basic IPA support
Related show

Commit Message

Niklas Söderlund Oct. 8, 2019, 12:45 a.m. UTC 
A new ControlList container is needed to hold metadata coming out of
the IPA. The list of supported controls in this list is expected to
grow, so for now do not add a validator for the list.

Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 include/libcamera/request.h |  2 ++
 src/libcamera/request.cpp   | 14 ++++++++++++++
 2 files changed, 16 insertions(+)

Comments

Jacopo Mondi Oct. 8, 2019, 8:35 a.m. UTC | #1 
Hi Niklas,

On Tue, Oct 08, 2019 at 02:45:29AM +0200, Niklas Söderlund wrote:
> A new ControlList container is needed to hold metadata coming out of
> the IPA. The list of supported controls in this list is expected to
> grow, so for now do not add a validator for the list.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  include/libcamera/request.h |  2 ++
>  src/libcamera/request.cpp   | 14 ++++++++++++++
>  2 files changed, 16 insertions(+)
>
> diff --git a/include/libcamera/request.h b/include/libcamera/request.h
> index e3db5243aaf3cf30..2d5a5964e99eb75f 100644
> --- a/include/libcamera/request.h
> +++ b/include/libcamera/request.h
> @@ -37,6 +37,7 @@ public:
>  	~Request();
>
>  	ControlList &controls() { return *controls_; }
> +	ControlList &metadata() { return *metadata_; }

Just thinking out loud, should the returned & be a const ? Might
application want to change the content of the metadata array ?

Thanks
  j
>  	const std::map<Stream *, Buffer *> &buffers() const { return bufferMap_; }
>  	int addBuffer(std::unique_ptr<Buffer> buffer);
>  	Buffer *findBuffer(Stream *stream) const;
> @@ -58,6 +59,7 @@ private:
>  	Camera *camera_;
>  	CameraControlValidator *validator_;
>  	ControlList *controls_;
> +	ControlList *metadata_;
>  	std::map<Stream *, Buffer *> bufferMap_;
>  	std::unordered_set<Buffer *> pending_;
>
> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
> index 19f6d0b9a0aeb03c..23d3ab6f422c406e 100644
> --- a/src/libcamera/request.cpp
> +++ b/src/libcamera/request.cpp
> @@ -65,6 +65,11 @@ Request::Request(Camera *camera, uint64_t cookie)
>  	 */
>  	validator_ = new CameraControlValidator(camera);
>  	controls_ = new ControlList(validator_);
> +
> +	/**
> +	 * \todo: Add a validator for metadata controls.
> +	 */
> +	metadata_ = new ControlList(nullptr);
>  }
>
>  Request::~Request()
> @@ -74,6 +79,7 @@ Request::~Request()
>  		delete buffer;
>  	}
>
> +	delete metadata_;
>  	delete controls_;
>  	delete validator_;
>  }
> @@ -161,6 +167,14 @@ Buffer *Request::findBuffer(Stream *stream) const
>  	return it->second;
>  }
>
> +/**
> + * \fn Request::metadata()
> + * \brief Retrieve the request's metadata
> + * \todo Offer a read-only API towards applications while keeping a read/write
> + * API internally.
> + * \return The metadata associated with the request
> + */
> +
>  /**
>   * \fn Request::cookie()
>   * \brief Retrieve the cookie set when the request was created
> --
> 2.23.0
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Laurent Pinchart Oct. 8, 2019, 11:22 a.m. UTC | #2 
Hi Jacopo,

On Tue, Oct 08, 2019 at 10:35:42AM +0200, Jacopo Mondi wrote:
> On Tue, Oct 08, 2019 at 02:45:29AM +0200, Niklas Söderlund wrote:
> > A new ControlList container is needed to hold metadata coming out of
> > the IPA. The list of supported controls in this list is expected to
> > grow, so for now do not add a validator for the list.
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  include/libcamera/request.h |  2 ++
> >  src/libcamera/request.cpp   | 14 ++++++++++++++
> >  2 files changed, 16 insertions(+)
> >
> > diff --git a/include/libcamera/request.h b/include/libcamera/request.h
> > index e3db5243aaf3cf30..2d5a5964e99eb75f 100644
> > --- a/include/libcamera/request.h
> > +++ b/include/libcamera/request.h
> > @@ -37,6 +37,7 @@ public:
> >  	~Request();
> >
> >  	ControlList &controls() { return *controls_; }
> > +	ControlList &metadata() { return *metadata_; }
> 
> Just thinking out loud, should the returned & be a const ? Might
> application want to change the content of the metadata array ?

Applications shouldn't change it, but pipeline handlers need write
access to the metadata. I think we need a better interface for this, in
order to set tighter permissions for applications, but that's a broad
issue with the current code base, so I don't think it needs to be fixed
as part of this patch series.

> >  	const std::map<Stream *, Buffer *> &buffers() const { return bufferMap_; }
> >  	int addBuffer(std::unique_ptr<Buffer> buffer);
> >  	Buffer *findBuffer(Stream *stream) const;
> > @@ -58,6 +59,7 @@ private:
> >  	Camera *camera_;
> >  	CameraControlValidator *validator_;
> >  	ControlList *controls_;
> > +	ControlList *metadata_;
> >  	std::map<Stream *, Buffer *> bufferMap_;
> >  	std::unordered_set<Buffer *> pending_;
> >
> > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
> > index 19f6d0b9a0aeb03c..23d3ab6f422c406e 100644
> > --- a/src/libcamera/request.cpp
> > +++ b/src/libcamera/request.cpp
> > @@ -65,6 +65,11 @@ Request::Request(Camera *camera, uint64_t cookie)
> >  	 */
> >  	validator_ = new CameraControlValidator(camera);
> >  	controls_ = new ControlList(validator_);
> > +
> > +	/**
> > +	 * \todo: Add a validator for metadata controls.
> > +	 */
> > +	metadata_ = new ControlList(nullptr);
> >  }
> >
> >  Request::~Request()
> > @@ -74,6 +79,7 @@ Request::~Request()
> >  		delete buffer;
> >  	}
> >
> > +	delete metadata_;
> >  	delete controls_;
> >  	delete validator_;
> >  }
> > @@ -161,6 +167,14 @@ Buffer *Request::findBuffer(Stream *stream) const
> >  	return it->second;
> >  }
> >
> > +/**
> > + * \fn Request::metadata()
> > + * \brief Retrieve the request's metadata
> > + * \todo Offer a read-only API towards applications while keeping a read/write
> > + * API internally.
> > + * \return The metadata associated with the request
> > + */
> > +
> >  /**
> >   * \fn Request::cookie()
> >   * \brief Retrieve the cookie set when the request was created
Niklas Söderlund Oct. 9, 2019, 11:12 p.m. UTC | #3 
Hi,

On 2019-10-08 14:22:19 +0300, Laurent Pinchart wrote:
> Hi Jacopo,
> 
> On Tue, Oct 08, 2019 at 10:35:42AM +0200, Jacopo Mondi wrote:
> > On Tue, Oct 08, 2019 at 02:45:29AM +0200, Niklas Söderlund wrote:
> > > A new ControlList container is needed to hold metadata coming out of
> > > the IPA. The list of supported controls in this list is expected to
> > > grow, so for now do not add a validator for the list.
> > >
> > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > ---
> > >  include/libcamera/request.h |  2 ++
> > >  src/libcamera/request.cpp   | 14 ++++++++++++++
> > >  2 files changed, 16 insertions(+)
> > >
> > > diff --git a/include/libcamera/request.h b/include/libcamera/request.h
> > > index e3db5243aaf3cf30..2d5a5964e99eb75f 100644
> > > --- a/include/libcamera/request.h
> > > +++ b/include/libcamera/request.h
> > > @@ -37,6 +37,7 @@ public:
> > >  	~Request();
> > >
> > >  	ControlList &controls() { return *controls_; }
> > > +	ControlList &metadata() { return *metadata_; }
> > 
> > Just thinking out loud, should the returned & be a const ? Might
> > application want to change the content of the metadata array ?
> 
> Applications shouldn't change it, but pipeline handlers need write
> access to the metadata. I think we need a better interface for this, in
> order to set tighter permissions for applications, but that's a broad
> issue with the current code base, so I don't think it needs to be fixed
> as part of this patch series.

I agree with both of you. The meta data should not be able to write to 
the Requests metadata. I have noted a todo in the documentation for 
Request::metadata() to highlight this already ;-)

> 
> > >  	const std::map<Stream *, Buffer *> &buffers() const { return bufferMap_; }
> > >  	int addBuffer(std::unique_ptr<Buffer> buffer);
> > >  	Buffer *findBuffer(Stream *stream) const;
> > > @@ -58,6 +59,7 @@ private:
> > >  	Camera *camera_;
> > >  	CameraControlValidator *validator_;
> > >  	ControlList *controls_;
> > > +	ControlList *metadata_;
> > >  	std::map<Stream *, Buffer *> bufferMap_;
> > >  	std::unordered_set<Buffer *> pending_;
> > >
> > > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
> > > index 19f6d0b9a0aeb03c..23d3ab6f422c406e 100644
> > > --- a/src/libcamera/request.cpp
> > > +++ b/src/libcamera/request.cpp
> > > @@ -65,6 +65,11 @@ Request::Request(Camera *camera, uint64_t cookie)
> > >  	 */
> > >  	validator_ = new CameraControlValidator(camera);
> > >  	controls_ = new ControlList(validator_);
> > > +
> > > +	/**
> > > +	 * \todo: Add a validator for metadata controls.
> > > +	 */
> > > +	metadata_ = new ControlList(nullptr);
> > >  }
> > >
> > >  Request::~Request()
> > > @@ -74,6 +79,7 @@ Request::~Request()
> > >  		delete buffer;
> > >  	}
> > >
> > > +	delete metadata_;
> > >  	delete controls_;
> > >  	delete validator_;
> > >  }
> > > @@ -161,6 +167,14 @@ Buffer *Request::findBuffer(Stream *stream) const
> > >  	return it->second;
> > >  }
> > >
> > > +/**
> > > + * \fn Request::metadata()
> > > + * \brief Retrieve the request's metadata
> > > + * \todo Offer a read-only API towards applications while keeping a read/write
> > > + * API internally.
> > > + * \return The metadata associated with the request
> > > + */
> > > +
> > >  /**
> > >   * \fn Request::cookie()
> > >   * \brief Retrieve the cookie set when the request was created
> 
> -- 
> Regards,
> 
> Laurent Pinchart

Patch
diff mbox series 

diff --git a/include/libcamera/request.h b/include/libcamera/request.h
index e3db5243aaf3cf30..2d5a5964e99eb75f 100644
--- a/include/libcamera/request.h
+++ b/include/libcamera/request.h
@@ -37,6 +37,7 @@  public:
 	~Request();
 
 	ControlList &controls() { return *controls_; }
+	ControlList &metadata() { return *metadata_; }
 	const std::map<Stream *, Buffer *> &buffers() const { return bufferMap_; }
 	int addBuffer(std::unique_ptr<Buffer> buffer);
 	Buffer *findBuffer(Stream *stream) const;
@@ -58,6 +59,7 @@  private:
 	Camera *camera_;
 	CameraControlValidator *validator_;
 	ControlList *controls_;
+	ControlList *metadata_;
 	std::map<Stream *, Buffer *> bufferMap_;
 	std::unordered_set<Buffer *> pending_;
 
diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
index 19f6d0b9a0aeb03c..23d3ab6f422c406e 100644
--- a/src/libcamera/request.cpp
+++ b/src/libcamera/request.cpp
@@ -65,6 +65,11 @@  Request::Request(Camera *camera, uint64_t cookie)
 	 */
 	validator_ = new CameraControlValidator(camera);
 	controls_ = new ControlList(validator_);
+
+	/**
+	 * \todo: Add a validator for metadata controls.
+	 */
+	metadata_ = new ControlList(nullptr);
 }
 
 Request::~Request()
@@ -74,6 +79,7 @@  Request::~Request()
 		delete buffer;
 	}
 
+	delete metadata_;
 	delete controls_;
 	delete validator_;
 }
@@ -161,6 +167,14 @@  Buffer *Request::findBuffer(Stream *stream) const
 	return it->second;
 }
 
+/**
+ * \fn Request::metadata()
+ * \brief Retrieve the request's metadata
+ * \todo Offer a read-only API towards applications while keeping a read/write
+ * API internally.
+ * \return The metadata associated with the request
+ */
+
 /**
  * \fn Request::cookie()
  * \brief Retrieve the cookie set when the request was created