Message ID | 20190929190254.18920-14-laurent.pinchart@ideasonboard.com |
---|---|
State | Accepted |
Headers | show |
Series |
|
Related | show |
Hi Laurent, Thanks for your patch. On 2019-09-29 22:02:54 +0300, Laurent Pinchart wrote: > Replace the manual validation of controls against a Camera with usage of > the new ControlValidator interface. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > include/libcamera/controls.h | 6 +++--- > include/libcamera/request.h | 7 ++++--- > src/libcamera/controls.cpp | 27 ++++++++++----------------- > src/libcamera/request.cpp | 14 ++++++++++++-- > test/controls/control_list.cpp | 15 ++++++++++++++- > 5 files changed, 43 insertions(+), 26 deletions(-) > > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h > index d3eea643c0ec..90426753f40f 100644 > --- a/include/libcamera/controls.h > +++ b/include/libcamera/controls.h > @@ -13,7 +13,7 @@ > > namespace libcamera { > > -class Camera; > +class ControlValidator; > > enum ControlType { > ControlTypeNone, > @@ -119,7 +119,7 @@ private: > using ControlListMap = std::unordered_map<const ControlId *, ControlValue>; > > public: > - ControlList(Camera *camera); > + ControlList(ControlValidator *validator); > > using iterator = ControlListMap::iterator; > using const_iterator = ControlListMap::const_iterator; > @@ -160,7 +160,7 @@ private: > const ControlValue *find(const ControlId &id) const; > ControlValue *find(const ControlId &id); > > - Camera *camera_; > + ControlValidator *validator_; > ControlListMap controls_; > }; > > diff --git a/include/libcamera/request.h b/include/libcamera/request.h > index 9edf1cedc054..e3db5243aaf3 100644 > --- a/include/libcamera/request.h > +++ b/include/libcamera/request.h > @@ -19,9 +19,9 @@ namespace libcamera { > > class Buffer; > class Camera; > +class CameraControlValidator; > class Stream; > > - > class Request > { > public: > @@ -36,7 +36,7 @@ public: > Request &operator=(const Request &) = delete; > ~Request(); > > - ControlList &controls() { return controls_; } > + ControlList &controls() { return *controls_; } > const std::map<Stream *, Buffer *> &buffers() const { return bufferMap_; } > int addBuffer(std::unique_ptr<Buffer> buffer); > Buffer *findBuffer(Stream *stream) const; > @@ -56,7 +56,8 @@ private: > bool completeBuffer(Buffer *buffer); > > Camera *camera_; > - ControlList controls_; > + CameraControlValidator *validator_; > + ControlList *controls_; > std::map<Stream *, Buffer *> bufferMap_; > std::unordered_set<Buffer *> pending_; > > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp > index a7e9d069b31a..f3260edce0bc 100644 > --- a/src/libcamera/controls.cpp > +++ b/src/libcamera/controls.cpp > @@ -10,8 +10,7 @@ > #include <sstream> > #include <string> > > -#include <libcamera/camera.h> > - > +#include "control_validator.h" > #include "log.h" > #include "utils.h" > > @@ -365,20 +364,16 @@ std::string ControlRange::toString() const > * \class ControlList > * \brief Associate a list of ControlId with their values for a camera > * > - * A ControlList wraps a map of ControlId to ControlValue and provide > - * additional validation against the control information exposed by a Camera. > - * > - * A list is only valid for as long as the camera it refers to is valid. After > - * that calling any method of the ControlList class other than its destructor > - * will cause undefined behaviour. > + * A ControlList wraps a map of ControlId to ControlValue and optionally > + * validates controls against a ControlValidator. > */ > > /** > - * \brief Construct a ControlList with a reference to the Camera it applies on > - * \param[in] camera The camera > + * \brief Construct a ControlList with an optional control validator > + * \param[in] validator The validator (may be null) > */ > -ControlList::ControlList(Camera *camera) > - : camera_(camera) > +ControlList::ControlList(ControlValidator *validator) > + : validator_(validator) > { > } > > @@ -493,12 +488,10 @@ const ControlValue *ControlList::find(const ControlId &id) const > > ControlValue *ControlList::find(const ControlId &id) > { > - const ControlInfoMap &controls = camera_->controls(); > - const auto iter = controls.find(&id); > - if (iter == controls.end()) { > + if (validator_ && !validator_->validate(id)) { > LOG(Controls, Error) > - << "Camera " << camera_->name() > - << " does not support control " << id.name(); > + << "Control " << id.name() > + << " is not valid for " << validator_->name(); > return nullptr; > } > > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp > index ee2158fc7a9c..19f6d0b9a0ae 100644 > --- a/src/libcamera/request.cpp > +++ b/src/libcamera/request.cpp > @@ -13,6 +13,7 @@ > #include <libcamera/camera.h> > #include <libcamera/stream.h> > > +#include "camera_controls.h" > #include "log.h" > > /** > @@ -55,9 +56,15 @@ LOG_DEFINE_CATEGORY(Request) > * > */ > Request::Request(Camera *camera, uint64_t cookie) > - : camera_(camera), controls_(camera), cookie_(cookie), > - status_(RequestPending), cancelled_(false) > + : camera_(camera), cookie_(cookie), status_(RequestPending), > + cancelled_(false) > { > + /** > + * \todo Should the Camera expose a validator instance, to avoid > + * creating a new instance for each request? > + */ > + validator_ = new CameraControlValidator(camera); > + controls_ = new ControlList(validator_); > } > > Request::~Request() > @@ -66,6 +73,9 @@ Request::~Request() > Buffer *buffer = it.second; > delete buffer; > } > + > + delete controls_; > + delete validator_; > } > > /** > diff --git a/test/controls/control_list.cpp b/test/controls/control_list.cpp > index 8469ecf09439..1bcfecc467b5 100644 > --- a/test/controls/control_list.cpp > +++ b/test/controls/control_list.cpp > @@ -12,6 +12,7 @@ > #include <libcamera/control_ids.h> > #include <libcamera/controls.h> > > +#include "camera_controls.h" > #include "test.h" > > using namespace std; > @@ -40,7 +41,8 @@ protected: > > int run() > { > - ControlList list(camera_.get()); > + CameraControlValidator validator(camera_.get()); > + ControlList list(&validator); > > /* Test that the list is initially empty. */ > if (!list.empty()) { > @@ -141,6 +143,17 @@ protected: > return TestFail; > } > > + /* > + * Attempt to set an invalid control and verify that the > + * operation failed. > + */ > + list.set(controls::AwbEnable, true); > + > + if (list.contains(controls::AwbEnable)) { > + cout << "List shouldn't contain AwbEnable control" << endl; > + return TestFail; > + } > + > return TestPass; > } > > -- > Regards, > > Laurent Pinchart > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h index d3eea643c0ec..90426753f40f 100644 --- a/include/libcamera/controls.h +++ b/include/libcamera/controls.h @@ -13,7 +13,7 @@ namespace libcamera { -class Camera; +class ControlValidator; enum ControlType { ControlTypeNone, @@ -119,7 +119,7 @@ private: using ControlListMap = std::unordered_map<const ControlId *, ControlValue>; public: - ControlList(Camera *camera); + ControlList(ControlValidator *validator); using iterator = ControlListMap::iterator; using const_iterator = ControlListMap::const_iterator; @@ -160,7 +160,7 @@ private: const ControlValue *find(const ControlId &id) const; ControlValue *find(const ControlId &id); - Camera *camera_; + ControlValidator *validator_; ControlListMap controls_; }; diff --git a/include/libcamera/request.h b/include/libcamera/request.h index 9edf1cedc054..e3db5243aaf3 100644 --- a/include/libcamera/request.h +++ b/include/libcamera/request.h @@ -19,9 +19,9 @@ namespace libcamera { class Buffer; class Camera; +class CameraControlValidator; class Stream; - class Request { public: @@ -36,7 +36,7 @@ public: Request &operator=(const Request &) = delete; ~Request(); - ControlList &controls() { return controls_; } + ControlList &controls() { return *controls_; } const std::map<Stream *, Buffer *> &buffers() const { return bufferMap_; } int addBuffer(std::unique_ptr<Buffer> buffer); Buffer *findBuffer(Stream *stream) const; @@ -56,7 +56,8 @@ private: bool completeBuffer(Buffer *buffer); Camera *camera_; - ControlList controls_; + CameraControlValidator *validator_; + ControlList *controls_; std::map<Stream *, Buffer *> bufferMap_; std::unordered_set<Buffer *> pending_; diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp index a7e9d069b31a..f3260edce0bc 100644 --- a/src/libcamera/controls.cpp +++ b/src/libcamera/controls.cpp @@ -10,8 +10,7 @@ #include <sstream> #include <string> -#include <libcamera/camera.h> - +#include "control_validator.h" #include "log.h" #include "utils.h" @@ -365,20 +364,16 @@ std::string ControlRange::toString() const * \class ControlList * \brief Associate a list of ControlId with their values for a camera * - * A ControlList wraps a map of ControlId to ControlValue and provide - * additional validation against the control information exposed by a Camera. - * - * A list is only valid for as long as the camera it refers to is valid. After - * that calling any method of the ControlList class other than its destructor - * will cause undefined behaviour. + * A ControlList wraps a map of ControlId to ControlValue and optionally + * validates controls against a ControlValidator. */ /** - * \brief Construct a ControlList with a reference to the Camera it applies on - * \param[in] camera The camera + * \brief Construct a ControlList with an optional control validator + * \param[in] validator The validator (may be null) */ -ControlList::ControlList(Camera *camera) - : camera_(camera) +ControlList::ControlList(ControlValidator *validator) + : validator_(validator) { } @@ -493,12 +488,10 @@ const ControlValue *ControlList::find(const ControlId &id) const ControlValue *ControlList::find(const ControlId &id) { - const ControlInfoMap &controls = camera_->controls(); - const auto iter = controls.find(&id); - if (iter == controls.end()) { + if (validator_ && !validator_->validate(id)) { LOG(Controls, Error) - << "Camera " << camera_->name() - << " does not support control " << id.name(); + << "Control " << id.name() + << " is not valid for " << validator_->name(); return nullptr; } diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp index ee2158fc7a9c..19f6d0b9a0ae 100644 --- a/src/libcamera/request.cpp +++ b/src/libcamera/request.cpp @@ -13,6 +13,7 @@ #include <libcamera/camera.h> #include <libcamera/stream.h> +#include "camera_controls.h" #include "log.h" /** @@ -55,9 +56,15 @@ LOG_DEFINE_CATEGORY(Request) * */ Request::Request(Camera *camera, uint64_t cookie) - : camera_(camera), controls_(camera), cookie_(cookie), - status_(RequestPending), cancelled_(false) + : camera_(camera), cookie_(cookie), status_(RequestPending), + cancelled_(false) { + /** + * \todo Should the Camera expose a validator instance, to avoid + * creating a new instance for each request? + */ + validator_ = new CameraControlValidator(camera); + controls_ = new ControlList(validator_); } Request::~Request() @@ -66,6 +73,9 @@ Request::~Request() Buffer *buffer = it.second; delete buffer; } + + delete controls_; + delete validator_; } /** diff --git a/test/controls/control_list.cpp b/test/controls/control_list.cpp index 8469ecf09439..1bcfecc467b5 100644 --- a/test/controls/control_list.cpp +++ b/test/controls/control_list.cpp @@ -12,6 +12,7 @@ #include <libcamera/control_ids.h> #include <libcamera/controls.h> +#include "camera_controls.h" #include "test.h" using namespace std; @@ -40,7 +41,8 @@ protected: int run() { - ControlList list(camera_.get()); + CameraControlValidator validator(camera_.get()); + ControlList list(&validator); /* Test that the list is initially empty. */ if (!list.empty()) { @@ -141,6 +143,17 @@ protected: return TestFail; } + /* + * Attempt to set an invalid control and verify that the + * operation failed. + */ + list.set(controls::AwbEnable, true); + + if (list.contains(controls::AwbEnable)) { + cout << "List shouldn't contain AwbEnable control" << endl; + return TestFail; + } + return TestPass; }
Replace the manual validation of controls against a Camera with usage of the new ControlValidator interface. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- include/libcamera/controls.h | 6 +++--- include/libcamera/request.h | 7 ++++--- src/libcamera/controls.cpp | 27 ++++++++++----------------- src/libcamera/request.cpp | 14 ++++++++++++-- test/controls/control_list.cpp | 15 ++++++++++++++- 5 files changed, 43 insertions(+), 26 deletions(-)