| Message ID | 20190621161401.28337-7-kieran.bingham@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Kieran, Thanks for your work. On 2019-06-21 17:13:58 +0100, Kieran Bingham wrote: > Provide a ControlList on request objects to facilitate setting controls > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > include/libcamera/request.h | 3 +++ > src/libcamera/request.cpp | 10 ++++++++++ > 2 files changed, 13 insertions(+) > > diff --git a/include/libcamera/request.h b/include/libcamera/request.h > index 58de6f00a554..8075270a9a12 100644 > --- a/include/libcamera/request.h > +++ b/include/libcamera/request.h > @@ -11,6 +11,7 @@ > #include <unordered_set> > > #include <libcamera/signal.h> > +#include <libcamera/controls.h> Alphabetical order please. With this fixed, Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > namespace libcamera { > > @@ -32,6 +33,7 @@ public: > Request(const Request &) = delete; > Request &operator=(const Request &) = delete; > > + ControlList &controls() { return controls_; } > const std::map<Stream *, Buffer *> &buffers() const { return bufferMap_; } > int setBuffers(const std::map<Stream *, Buffer *> &streamMap); > Buffer *findBuffer(Stream *stream) const; > @@ -50,6 +52,7 @@ private: > bool completeBuffer(Buffer *buffer); > > Camera *camera_; > + ControlList controls_; > std::map<Stream *, Buffer *> bufferMap_; > std::unordered_set<Buffer *> pending_; > > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp > index fa3ee46da440..9a0409b46017 100644 > --- a/src/libcamera/request.cpp > +++ b/src/libcamera/request.cpp > @@ -52,6 +52,16 @@ Request::Request(Camera *camera) > { > } > > +/** > + * \fn Request::controls() > + * \brief Retrieve the request's ControlList > + * > + * Return a reference to the ControlList that stores all the controls relevant > + * to this request. > + * > + * \return A reference to the ControlList in this request > + */ > + > /** > * \fn Request::buffers() > * \brief Retrieve the request's streams to buffers map > -- > 2.20.1 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Kieran, Thank you for the patch. On Sun, Jun 23, 2019 at 05:36:52PM +0200, Niklas Söderlund wrote: > On 2019-06-21 17:13:58 +0100, Kieran Bingham wrote: > > Provide a ControlList on request objects to facilitate setting controls > > > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > --- > > include/libcamera/request.h | 3 +++ > > src/libcamera/request.cpp | 10 ++++++++++ > > 2 files changed, 13 insertions(+) > > > > diff --git a/include/libcamera/request.h b/include/libcamera/request.h > > index 58de6f00a554..8075270a9a12 100644 > > --- a/include/libcamera/request.h > > +++ b/include/libcamera/request.h > > @@ -11,6 +11,7 @@ > > #include <unordered_set> > > > > #include <libcamera/signal.h> > > +#include <libcamera/controls.h> > > Alphabetical order please. > > With this fixed, > > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > > > > namespace libcamera { > > > > @@ -32,6 +33,7 @@ public: > > Request(const Request &) = delete; > > Request &operator=(const Request &) = delete; > > > > + ControlList &controls() { return controls_; } > > const std::map<Stream *, Buffer *> &buffers() const { return bufferMap_; } > > int setBuffers(const std::map<Stream *, Buffer *> &streamMap); > > Buffer *findBuffer(Stream *stream) const; > > @@ -50,6 +52,7 @@ private: > > bool completeBuffer(Buffer *buffer); > > > > Camera *camera_; > > + ControlList controls_; > > std::map<Stream *, Buffer *> bufferMap_; > > std::unordered_set<Buffer *> pending_; > > > > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp > > index fa3ee46da440..9a0409b46017 100644 > > --- a/src/libcamera/request.cpp > > +++ b/src/libcamera/request.cpp > > @@ -52,6 +52,16 @@ Request::Request(Camera *camera) > > { > > } > > > > +/** > > + * \fn Request::controls() > > + * \brief Retrieve the request's ControlList > > + * > > + * Return a reference to the ControlList that stores all the controls relevant > > + * to this request. > > + * > > + * \return A reference to the ControlList in this request > > + */ We need more documentation, with a focus on how this API should be used by applications. I also expect that you will need to reset the controls once the request complete, as request objects can be reused. > > + > > /** > > * \fn Request::buffers() > > * \brief Retrieve the request's streams to buffers map
Hi Laurent, On 23/06/2019 23:31, Laurent Pinchart wrote: > Hi Kieran, > > Thank you for the patch. > > On Sun, Jun 23, 2019 at 05:36:52PM +0200, Niklas Söderlund wrote: >> On 2019-06-21 17:13:58 +0100, Kieran Bingham wrote: >>> Provide a ControlList on request objects to facilitate setting controls >>> >>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >>> --- >>> include/libcamera/request.h | 3 +++ >>> src/libcamera/request.cpp | 10 ++++++++++ >>> 2 files changed, 13 insertions(+) >>> >>> diff --git a/include/libcamera/request.h b/include/libcamera/request.h >>> index 58de6f00a554..8075270a9a12 100644 >>> --- a/include/libcamera/request.h >>> +++ b/include/libcamera/request.h >>> @@ -11,6 +11,7 @@ >>> #include <unordered_set> >>> >>> #include <libcamera/signal.h> >>> +#include <libcamera/controls.h> >> >> Alphabetical order please. >> >> With this fixed, >> >> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> >> >>> >>> namespace libcamera { >>> >>> @@ -32,6 +33,7 @@ public: >>> Request(const Request &) = delete; >>> Request &operator=(const Request &) = delete; >>> >>> + ControlList &controls() { return controls_; } >>> const std::map<Stream *, Buffer *> &buffers() const { return bufferMap_; } >>> int setBuffers(const std::map<Stream *, Buffer *> &streamMap); >>> Buffer *findBuffer(Stream *stream) const; >>> @@ -50,6 +52,7 @@ private: >>> bool completeBuffer(Buffer *buffer); >>> >>> Camera *camera_; >>> + ControlList controls_; >>> std::map<Stream *, Buffer *> bufferMap_; >>> std::unordered_set<Buffer *> pending_; >>> >>> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp >>> index fa3ee46da440..9a0409b46017 100644 >>> --- a/src/libcamera/request.cpp >>> +++ b/src/libcamera/request.cpp >>> @@ -52,6 +52,16 @@ Request::Request(Camera *camera) >>> { >>> } >>> >>> +/** >>> + * \fn Request::controls() >>> + * \brief Retrieve the request's ControlList >>> + * >>> + * Return a reference to the ControlList that stores all the controls relevant >>> + * to this request. >>> + * >>> + * \return A reference to the ControlList in this request >>> + */ > > We need more documentation, with a focus on how this API should be used > by applications. Sure. RFC==WIP right? Maybe I am using RFC too strongly? I'm trying to get the underlying implementation right before fully documenting (and re-documenting for every change) This exposes the list, so I think is a good place for documenting how the Controls should be added to the request. I wonder if we're going to need some more 'higher level' documentation somewhere describing how to write a libcamera application in the first place... (as well, I'm not saying instead of this). > I also expect that you will need to reset the controls > once the request complete, as request objects can be reused. I don't think requests can be reused currently: /** * \brief Handle request completion and notify application * \param[in] request The request that has completed * * This function is called by the pipeline handler to notify the camera that * the request has completed. It emits the requestCompleted signal and deletes * the request. */ void Camera::requestComplete(Request *request) { std::map<Stream *, Buffer *> buffers(std::move(request->bufferMap_)); requestCompleted.emit(request, buffers); delete request; } If that changes, then probably yes, the ControlList should be cleared at some point during requestComplete();
Hi Kieran, On Wed, Jun 26, 2019 at 12:30:21PM +0100, Kieran Bingham wrote: > On 23/06/2019 23:31, Laurent Pinchart wrote: > > On Sun, Jun 23, 2019 at 05:36:52PM +0200, Niklas Söderlund wrote: > >> On 2019-06-21 17:13:58 +0100, Kieran Bingham wrote: > >>> Provide a ControlList on request objects to facilitate setting controls > >>> > >>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >>> --- > >>> include/libcamera/request.h | 3 +++ > >>> src/libcamera/request.cpp | 10 ++++++++++ > >>> 2 files changed, 13 insertions(+) > >>> > >>> diff --git a/include/libcamera/request.h b/include/libcamera/request.h > >>> index 58de6f00a554..8075270a9a12 100644 > >>> --- a/include/libcamera/request.h > >>> +++ b/include/libcamera/request.h > >>> @@ -11,6 +11,7 @@ > >>> #include <unordered_set> > >>> > >>> #include <libcamera/signal.h> > >>> +#include <libcamera/controls.h> > >> > >> Alphabetical order please. > >> > >> With this fixed, > >> > >> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > >> > >>> > >>> namespace libcamera { > >>> > >>> @@ -32,6 +33,7 @@ public: > >>> Request(const Request &) = delete; > >>> Request &operator=(const Request &) = delete; > >>> > >>> + ControlList &controls() { return controls_; } > >>> const std::map<Stream *, Buffer *> &buffers() const { return bufferMap_; } > >>> int setBuffers(const std::map<Stream *, Buffer *> &streamMap); > >>> Buffer *findBuffer(Stream *stream) const; > >>> @@ -50,6 +52,7 @@ private: > >>> bool completeBuffer(Buffer *buffer); > >>> > >>> Camera *camera_; > >>> + ControlList controls_; > >>> std::map<Stream *, Buffer *> bufferMap_; > >>> std::unordered_set<Buffer *> pending_; > >>> > >>> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp > >>> index fa3ee46da440..9a0409b46017 100644 > >>> --- a/src/libcamera/request.cpp > >>> +++ b/src/libcamera/request.cpp > >>> @@ -52,6 +52,16 @@ Request::Request(Camera *camera) > >>> { > >>> } > >>> > >>> +/** > >>> + * \fn Request::controls() > >>> + * \brief Retrieve the request's ControlList > >>> + * > >>> + * Return a reference to the ControlList that stores all the controls relevant > >>> + * to this request. > >>> + * > >>> + * \return A reference to the ControlList in this request > >>> + */ > > > > We need more documentation, with a focus on how this API should be used > > by applications. > > Sure. RFC==WIP right? Maybe I am using RFC too strongly? I wasn't blaming you, just pointing out a missing piece to make sure it wouldn't be forgotten :-) But for me RFC signals a more advanced state than WIP. > I'm trying to get the underlying implementation right before fully > documenting (and re-documenting for every change) > > This exposes the list, so I think is a good place for documenting how > the Controls should be added to the request. I agree. > I wonder if we're going to need some more 'higher level' documentation > somewhere describing how to write a libcamera application in the first > place... (as well, I'm not saying instead of this). Yes, I think we will. > > I also expect that you will need to reset the controls > > once the request complete, as request objects can be reused. > > I don't think requests can be reused currently: > > /** > * \brief Handle request completion and notify application > * \param[in] request The request that has completed > * > * This function is called by the pipeline handler to notify the camera that > * the request has completed. It emits the requestCompleted signal and deletes > * the request. > */ > void Camera::requestComplete(Request *request) > { > std::map<Stream *, Buffer *> buffers(std::move(request->bufferMap_)); > requestCompleted.emit(request, buffers); > delete request; > } > > > If that changes, then probably yes, the ControlList should be cleared at > some point during requestComplete(); You're right, my bad.
diff --git a/include/libcamera/request.h b/include/libcamera/request.h index 58de6f00a554..8075270a9a12 100644 --- a/include/libcamera/request.h +++ b/include/libcamera/request.h @@ -11,6 +11,7 @@ #include <unordered_set> #include <libcamera/signal.h> +#include <libcamera/controls.h> namespace libcamera { @@ -32,6 +33,7 @@ public: Request(const Request &) = delete; Request &operator=(const Request &) = delete; + ControlList &controls() { return controls_; } const std::map<Stream *, Buffer *> &buffers() const { return bufferMap_; } int setBuffers(const std::map<Stream *, Buffer *> &streamMap); Buffer *findBuffer(Stream *stream) const; @@ -50,6 +52,7 @@ private: bool completeBuffer(Buffer *buffer); Camera *camera_; + ControlList controls_; std::map<Stream *, Buffer *> bufferMap_; std::unordered_set<Buffer *> pending_; diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp index fa3ee46da440..9a0409b46017 100644 --- a/src/libcamera/request.cpp +++ b/src/libcamera/request.cpp @@ -52,6 +52,16 @@ Request::Request(Camera *camera) { } +/** + * \fn Request::controls() + * \brief Retrieve the request's ControlList + * + * Return a reference to the ControlList that stores all the controls relevant + * to this request. + * + * \return A reference to the ControlList in this request + */ + /** * \fn Request::buffers() * \brief Retrieve the request's streams to buffers map
Provide a ControlList on request objects to facilitate setting controls Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> --- include/libcamera/request.h | 3 +++ src/libcamera/request.cpp | 10 ++++++++++ 2 files changed, 13 insertions(+)