| Message ID | 20210810161134.2243796-3-kieran.bingham@ideasonboard.com |
|---|---|
| State | Superseded |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series |
|
| Related | show |
Hi Kieran, On Tue, Aug 10, 2021 at 05:11:33PM +0100, Kieran Bingham wrote: > Create a Camera specific CameraControlValidator for the Camera instance. > This will allow requests to use a single validor instance without having > to construct their own. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > > Laurent: > - Is the use of the _o<Public>() reference valid here in the > initialiser list? Could the requirement to pass a Camera * to the CameraControlValidator constructor be dropped ? The Request class has access to the internal Camera::Private representation and the need for the Validator to have a Camera * is only to access the Camera's control info map. IOW: Can the CameraControlValidator be made to accept a Camera::Private ? There shouldn't be a need to expose it to applications, right ? > > > include/libcamera/internal/camera.h | 6 ++++++ > src/libcamera/camera.cpp | 2 +- > 2 files changed, 7 insertions(+), 1 deletion(-) > > diff --git a/include/libcamera/internal/camera.h b/include/libcamera/internal/camera.h > index b60ed140356a..f14bafc75e05 100644 > --- a/include/libcamera/internal/camera.h > +++ b/include/libcamera/internal/camera.h > @@ -16,6 +16,8 @@ > > #include <libcamera/camera.h> > > +#include "libcamera/internal/camera_controls.h" > + > namespace libcamera { > > class PipelineHandler; > @@ -30,6 +32,8 @@ public: > const std::set<Stream *> &streams); > ~Private(); > > + const CameraControlValidator &validator() const { return validator_; } > + > private: > enum State { > CameraAvailable, > @@ -56,6 +60,8 @@ private: > > bool disconnected_; > std::atomic<State> state_; > + > + CameraControlValidator validator_; > }; > > } /* namespace libcamera */ > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > index 6281e92057e4..b914bf188d57 100644 > --- a/src/libcamera/camera.cpp > +++ b/src/libcamera/camera.cpp > @@ -336,7 +336,7 @@ Camera::Private::Private(PipelineHandler *pipe, > const std::string &id, > const std::set<Stream *> &streams) > : pipe_(pipe->shared_from_this()), id_(id), streams_(streams), > - disconnected_(false), state_(CameraAvailable) > + disconnected_(false), state_(CameraAvailable), validator_(_o<Public>()) > { > } > > -- > 2.30.2 >
Hi Jacopo, On 11/08/2021 13:35, Jacopo Mondi wrote: > Hi Kieran, > > On Tue, Aug 10, 2021 at 05:11:33PM +0100, Kieran Bingham wrote: >> Create a Camera specific CameraControlValidator for the Camera instance. >> This will allow requests to use a single validor instance without having >> to construct their own. >> >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> --- >> >> Laurent: >> - Is the use of the _o<Public>() reference valid here in the >> initialiser list? > > Could the requirement to pass a Camera * to the CameraControlValidator > constructor be dropped ? The Request class has access to the internal > Camera::Private representation and the need for the Validator to have > a Camera * is only to access the Camera's control info map. It looks like Camera is used for both: camera_->name() and camera_->controls() indeed. the name is a little trivial, but still there. I'm not sure what the fear is about passing the Camera * Indeed, if we can pass the Camera::Private we can dig the pipe_->controls() ... but .. ahh. To get the ControlInfoMap - it's indexed from the Pipe based on the Camera * ... which is because it's in the CameraData ... Aha - Now I see where you're going with that - Laurent's series moves that CameraData into Camera::Private. So - at that point, yes we could. But I haven't got that series applied here yet ;-) > IOW: Can the CameraControlValidator be made to accept a > Camera::Private ? There shouldn't be a need to expose it to > applications, right ? Perhaps I need to rebase on top of Laurent's patches, or give this series to him ;-) -- Kieran > >> >> >> include/libcamera/internal/camera.h | 6 ++++++ >> src/libcamera/camera.cpp | 2 +- >> 2 files changed, 7 insertions(+), 1 deletion(-) >> >> diff --git a/include/libcamera/internal/camera.h b/include/libcamera/internal/camera.h >> index b60ed140356a..f14bafc75e05 100644 >> --- a/include/libcamera/internal/camera.h >> +++ b/include/libcamera/internal/camera.h >> @@ -16,6 +16,8 @@ >> >> #include <libcamera/camera.h> >> >> +#include "libcamera/internal/camera_controls.h" >> + >> namespace libcamera { >> >> class PipelineHandler; >> @@ -30,6 +32,8 @@ public: >> const std::set<Stream *> &streams); >> ~Private(); >> >> + const CameraControlValidator &validator() const { return validator_; } >> + >> private: >> enum State { >> CameraAvailable, >> @@ -56,6 +60,8 @@ private: >> >> bool disconnected_; >> std::atomic<State> state_; >> + >> + CameraControlValidator validator_; >> }; >> >> } /* namespace libcamera */ >> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp >> index 6281e92057e4..b914bf188d57 100644 >> --- a/src/libcamera/camera.cpp >> +++ b/src/libcamera/camera.cpp >> @@ -336,7 +336,7 @@ Camera::Private::Private(PipelineHandler *pipe, >> const std::string &id, >> const std::set<Stream *> &streams) >> : pipe_(pipe->shared_from_this()), id_(id), streams_(streams), >> - disconnected_(false), state_(CameraAvailable) >> + disconnected_(false), state_(CameraAvailable), validator_(_o<Public>()) >> { >> } >> >> -- >> 2.30.2 >>
Hi Kieran, On Wed, Aug 11, 2021 at 04:03:50PM +0100, Kieran Bingham wrote: > Hi Jacopo, > > On 11/08/2021 13:35, Jacopo Mondi wrote: > > Hi Kieran, > > > > On Tue, Aug 10, 2021 at 05:11:33PM +0100, Kieran Bingham wrote: > >> Create a Camera specific CameraControlValidator for the Camera instance. > >> This will allow requests to use a single validor instance without having > >> to construct their own. > >> > >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >> --- > >> > >> Laurent: > >> - Is the use of the _o<Public>() reference valid here in the > >> initialiser list? > > > > Could the requirement to pass a Camera * to the CameraControlValidator > > constructor be dropped ? The Request class has access to the internal > > Camera::Private representation and the need for the Validator to have > > a Camera * is only to access the Camera's control info map. > > > It looks like Camera is used for both: > camera_->name() > and > camera_->controls() > > indeed. > > the name is a little trivial, but still there. > I'm not sure what the fear is about passing the Camera * > > > Indeed, if we can pass the Camera::Private we can dig the > pipe_->controls() ... but .. ahh. > > To get the ControlInfoMap - it's indexed from the Pipe based on the > Camera * ... which is because it's in the CameraData ... > > Aha - Now I see where you're going with that - Laurent's series moves > that CameraData into Camera::Private. So - at that point, yes we could. > > But I haven't got that series applied here yet ;-) Yes indeed, I was already projected to when the control info and properties will be stored in Camera::Private... > > > > IOW: Can the CameraControlValidator be made to accept a > > Camera::Private ? There shouldn't be a need to expose it to > > applications, right ? > > Perhaps I need to rebase on top of Laurent's patches, or give this > series to him ;-) > > -- > Kieran > > > > > > >> > >> > >> include/libcamera/internal/camera.h | 6 ++++++ > >> src/libcamera/camera.cpp | 2 +- > >> 2 files changed, 7 insertions(+), 1 deletion(-) > >> > >> diff --git a/include/libcamera/internal/camera.h b/include/libcamera/internal/camera.h > >> index b60ed140356a..f14bafc75e05 100644 > >> --- a/include/libcamera/internal/camera.h > >> +++ b/include/libcamera/internal/camera.h > >> @@ -16,6 +16,8 @@ > >> > >> #include <libcamera/camera.h> > >> > >> +#include "libcamera/internal/camera_controls.h" > >> + > >> namespace libcamera { > >> > >> class PipelineHandler; > >> @@ -30,6 +32,8 @@ public: > >> const std::set<Stream *> &streams); > >> ~Private(); > >> > >> + const CameraControlValidator &validator() const { return validator_; } > >> + > >> private: > >> enum State { > >> CameraAvailable, > >> @@ -56,6 +60,8 @@ private: > >> > >> bool disconnected_; > >> std::atomic<State> state_; > >> + > >> + CameraControlValidator validator_; > >> }; > >> > >> } /* namespace libcamera */ > >> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > >> index 6281e92057e4..b914bf188d57 100644 > >> --- a/src/libcamera/camera.cpp > >> +++ b/src/libcamera/camera.cpp > >> @@ -336,7 +336,7 @@ Camera::Private::Private(PipelineHandler *pipe, > >> const std::string &id, > >> const std::set<Stream *> &streams) > >> : pipe_(pipe->shared_from_this()), id_(id), streams_(streams), > >> - disconnected_(false), state_(CameraAvailable) > >> + disconnected_(false), state_(CameraAvailable), validator_(_o<Public>()) > >> { > >> } > >> > >> -- > >> 2.30.2 > >>
Hello, On Wed, Aug 11, 2021 at 05:12:24PM +0200, Jacopo Mondi wrote: > On Wed, Aug 11, 2021 at 04:03:50PM +0100, Kieran Bingham wrote: > > On 11/08/2021 13:35, Jacopo Mondi wrote: > > > On Tue, Aug 10, 2021 at 05:11:33PM +0100, Kieran Bingham wrote: > > >> Create a Camera specific CameraControlValidator for the Camera instance. > > >> This will allow requests to use a single validor instance without having > > >> to construct their own. > > >> > > >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > >> --- > > >> > > >> Laurent: > > >> - Is the use of the _o<Public>() reference valid here in the > > >> initialiser list? The _o() function is defined as template<typename T> T *_o() { return static_cast<T *>(o_); } Since the recent change to decouple construction of Extensible and Extensible::Private, the Extensible::Private::o_ member variable is initialized by the Extensible constructor. With the pending patch series that makes the pipeline handlers subclass Camera::Private, construction of Camera::Private will happen before Camera is constructed. _o() is thus not valid here I'm afraid. > > > Could the requirement to pass a Camera * to the CameraControlValidator > > > constructor be dropped ? The Request class has access to the internal > > > Camera::Private representation and the need for the Validator to have > > > a Camera * is only to access the Camera's control info map. > > > > It looks like Camera is used for both: > > camera_->name() > > and > > camera_->controls() > > > > indeed. > > > > the name is a little trivial, but still there. > > I'm not sure what the fear is about passing the Camera * > > > > > > Indeed, if we can pass the Camera::Private we can dig the > > pipe_->controls() ... but .. ahh. > > > > To get the ControlInfoMap - it's indexed from the Pipe based on the > > Camera * ... which is because it's in the CameraData ... > > > > Aha - Now I see where you're going with that - Laurent's series moves > > that CameraData into Camera::Private. So - at that point, yes we could. > > > > But I haven't got that series applied here yet ;-) > > Yes indeed, I was already projected to when the control info and > properties will be stored in Camera::Private... > > > > IOW: Can the CameraControlValidator be made to accept a > > > Camera::Private ? There shouldn't be a need to expose it to > > > applications, right ? > > > > Perhaps I need to rebase on top of Laurent's patches, or give this > > series to him ;-) Please feel free to rebase :-) Please also feel free to change the way control validators are designed. The current implementation is more of a skeleton than anything else, and its design dates back to a time when libcamera was very different. Don't feel constrained by how control validators work today. > > >> include/libcamera/internal/camera.h | 6 ++++++ > > >> src/libcamera/camera.cpp | 2 +- > > >> 2 files changed, 7 insertions(+), 1 deletion(-) > > >> > > >> diff --git a/include/libcamera/internal/camera.h b/include/libcamera/internal/camera.h > > >> index b60ed140356a..f14bafc75e05 100644 > > >> --- a/include/libcamera/internal/camera.h > > >> +++ b/include/libcamera/internal/camera.h > > >> @@ -16,6 +16,8 @@ > > >> > > >> #include <libcamera/camera.h> > > >> > > >> +#include "libcamera/internal/camera_controls.h" > > >> + > > >> namespace libcamera { > > >> > > >> class PipelineHandler; > > >> @@ -30,6 +32,8 @@ public: > > >> const std::set<Stream *> &streams); > > >> ~Private(); > > >> > > >> + const CameraControlValidator &validator() const { return validator_; } > > >> + > > >> private: > > >> enum State { > > >> CameraAvailable, > > >> @@ -56,6 +60,8 @@ private: > > >> > > >> bool disconnected_; > > >> std::atomic<State> state_; > > >> + > > >> + CameraControlValidator validator_; > > >> }; > > >> > > >> } /* namespace libcamera */ > > >> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > > >> index 6281e92057e4..b914bf188d57 100644 > > >> --- a/src/libcamera/camera.cpp > > >> +++ b/src/libcamera/camera.cpp > > >> @@ -336,7 +336,7 @@ Camera::Private::Private(PipelineHandler *pipe, > > >> const std::string &id, > > >> const std::set<Stream *> &streams) > > >> : pipe_(pipe->shared_from_this()), id_(id), streams_(streams), > > >> - disconnected_(false), state_(CameraAvailable) > > >> + disconnected_(false), state_(CameraAvailable), validator_(_o<Public>()) > > >> { > > >> } > > >>
On Thu, Aug 12, 2021 at 01:50:10AM +0300, Laurent Pinchart wrote: > On Wed, Aug 11, 2021 at 05:12:24PM +0200, Jacopo Mondi wrote: > > On Wed, Aug 11, 2021 at 04:03:50PM +0100, Kieran Bingham wrote: > > > On 11/08/2021 13:35, Jacopo Mondi wrote: > > > > On Tue, Aug 10, 2021 at 05:11:33PM +0100, Kieran Bingham wrote: > > > >> Create a Camera specific CameraControlValidator for the Camera instance. > > > >> This will allow requests to use a single validor instance without having > > > >> to construct their own. > > > >> > > > >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > >> --- > > > >> > > > >> Laurent: > > > >> - Is the use of the _o<Public>() reference valid here in the > > > >> initialiser list? > > The _o() function is defined as > > template<typename T> > T *_o() > { > return static_cast<T *>(o_); > } > > Since the recent change to decouple construction of Extensible and > Extensible::Private, the Extensible::Private::o_ member variable is > initialized by the Extensible constructor. With the pending patch series > that makes the pipeline handlers subclass Camera::Private, construction > of Camera::Private will happen before Camera is constructed. _o() is > thus not valid here I'm afraid. Note that you can initialize members of the private class in the public class constructor. For instance, the Camera constructor is implemented as Camera::Camera(std::unique_ptr<Private> d, const std::string &id, const std::set<Stream *> &streams) : Extensible(std::move(d)) { _d()->id_ = id; _d()->streams_ = streams; } You could store a std::unique_ptr<CameraControlValidator> validator_; in Camera::Private, and construct and assign it in Camera::Camera() with _d()->validator_ = std::make_unique<CameraControlValidator>(this); (it may make sense to store the return value of _d() in a local variable, and no, you can't use the d parameter, as it's moved before the body of the Camera constructor is executed). > > > > Could the requirement to pass a Camera * to the CameraControlValidator > > > > constructor be dropped ? The Request class has access to the internal > > > > Camera::Private representation and the need for the Validator to have > > > > a Camera * is only to access the Camera's control info map. > > > > > > It looks like Camera is used for both: > > > camera_->name() > > > and > > > camera_->controls() > > > > > > indeed. > > > > > > the name is a little trivial, but still there. > > > I'm not sure what the fear is about passing the Camera * > > > > > > > > > Indeed, if we can pass the Camera::Private we can dig the > > > pipe_->controls() ... but .. ahh. > > > > > > To get the ControlInfoMap - it's indexed from the Pipe based on the > > > Camera * ... which is because it's in the CameraData ... > > > > > > Aha - Now I see where you're going with that - Laurent's series moves > > > that CameraData into Camera::Private. So - at that point, yes we could. > > > > > > But I haven't got that series applied here yet ;-) > > > > Yes indeed, I was already projected to when the control info and > > properties will be stored in Camera::Private... > > > > > > IOW: Can the CameraControlValidator be made to accept a > > > > Camera::Private ? There shouldn't be a need to expose it to > > > > applications, right ? > > > > > > Perhaps I need to rebase on top of Laurent's patches, or give this > > > series to him ;-) > > Please feel free to rebase :-) Please also feel free to change the way > control validators are designed. The current implementation is more of a > skeleton than anything else, and its design dates back to a time when > libcamera was very different. Don't feel constrained by how control > validators work today. > > > > >> include/libcamera/internal/camera.h | 6 ++++++ > > > >> src/libcamera/camera.cpp | 2 +- > > > >> 2 files changed, 7 insertions(+), 1 deletion(-) > > > >> > > > >> diff --git a/include/libcamera/internal/camera.h b/include/libcamera/internal/camera.h > > > >> index b60ed140356a..f14bafc75e05 100644 > > > >> --- a/include/libcamera/internal/camera.h > > > >> +++ b/include/libcamera/internal/camera.h > > > >> @@ -16,6 +16,8 @@ > > > >> > > > >> #include <libcamera/camera.h> > > > >> > > > >> +#include "libcamera/internal/camera_controls.h" > > > >> + > > > >> namespace libcamera { > > > >> > > > >> class PipelineHandler; > > > >> @@ -30,6 +32,8 @@ public: > > > >> const std::set<Stream *> &streams); > > > >> ~Private(); > > > >> > > > >> + const CameraControlValidator &validator() const { return validator_; } > > > >> + > > > >> private: > > > >> enum State { > > > >> CameraAvailable, > > > >> @@ -56,6 +60,8 @@ private: > > > >> > > > >> bool disconnected_; > > > >> std::atomic<State> state_; > > > >> + > > > >> + CameraControlValidator validator_; > > > >> }; > > > >> > > > >> } /* namespace libcamera */ > > > >> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > > > >> index 6281e92057e4..b914bf188d57 100644 > > > >> --- a/src/libcamera/camera.cpp > > > >> +++ b/src/libcamera/camera.cpp > > > >> @@ -336,7 +336,7 @@ Camera::Private::Private(PipelineHandler *pipe, > > > >> const std::string &id, > > > >> const std::set<Stream *> &streams) > > > >> : pipe_(pipe->shared_from_this()), id_(id), streams_(streams), > > > >> - disconnected_(false), state_(CameraAvailable) > > > >> + disconnected_(false), state_(CameraAvailable), validator_(_o<Public>()) > > > >> { > > > >> } > > > >>
diff --git a/include/libcamera/internal/camera.h b/include/libcamera/internal/camera.h index b60ed140356a..f14bafc75e05 100644 --- a/include/libcamera/internal/camera.h +++ b/include/libcamera/internal/camera.h @@ -16,6 +16,8 @@ #include <libcamera/camera.h> +#include "libcamera/internal/camera_controls.h" + namespace libcamera { class PipelineHandler; @@ -30,6 +32,8 @@ public: const std::set<Stream *> &streams); ~Private(); + const CameraControlValidator &validator() const { return validator_; } + private: enum State { CameraAvailable, @@ -56,6 +60,8 @@ private: bool disconnected_; std::atomic<State> state_; + + CameraControlValidator validator_; }; } /* namespace libcamera */ diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp index 6281e92057e4..b914bf188d57 100644 --- a/src/libcamera/camera.cpp +++ b/src/libcamera/camera.cpp @@ -336,7 +336,7 @@ Camera::Private::Private(PipelineHandler *pipe, const std::string &id, const std::set<Stream *> &streams) : pipe_(pipe->shared_from_this()), id_(id), streams_(streams), - disconnected_(false), state_(CameraAvailable) + disconnected_(false), state_(CameraAvailable), validator_(_o<Public>()) { }
Create a Camera specific CameraControlValidator for the Camera instance. This will allow requests to use a single validor instance without having to construct their own. Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> --- Laurent: - Is the use of the _o<Public>() reference valid here in the initialiser list? include/libcamera/internal/camera.h | 6 ++++++ src/libcamera/camera.cpp | 2 +- 2 files changed, 7 insertions(+), 1 deletion(-)