[libcamera-devel] [PATCH v5 05/10] libcamera: request: Add metadata information

[Niklas Söderlund]

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 at ragnatech.se>
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart at 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

-- 
Regards,
Niklas Söderlund