| Message ID | 20190402005332.25018-3-niklas.soderlund@ragnatech.se |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Niklas On Tue, Apr 02, 2019 at 02:53:29AM +0200, Niklas Söderlund wrote: > In preparation of reworking how a default configuration is retrieved > from a camera remove the streams and validation using the stream when > judging if a camera configuration is valid. This is needed as once > stream usage hints are added applications will no longer fetch default > configuration based on Stream IDs so using them to verify the returned > format is not useful. > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > test/camera/camera_test.cpp | 26 ++++++++------------------ > test/camera/camera_test.h | 3 +-- > test/camera/capture.cpp | 2 +- > test/camera/configuration_default.cpp | 2 +- > test/camera/configuration_set.cpp | 2 +- > 5 files changed, 12 insertions(+), 23 deletions(-) > > diff --git a/test/camera/camera_test.cpp b/test/camera/camera_test.cpp > index a92f2165bf3a53c1..5985b85c44816e30 100644 > --- a/test/camera/camera_test.cpp > +++ b/test/camera/camera_test.cpp > @@ -46,27 +46,17 @@ void CameraTest::cleanup() > cm_->stop(); > }; > > -bool CameraTest::configurationValid(const std::set<Stream *> &streams, > - const std::map<Stream *, StreamConfiguration> &conf) const > +bool CameraTest::configurationValid(const std::map<Stream *, StreamConfiguration> &config) const > { > - /* Test that the numbers of streams matches that of configuration. */ > - if (streams.size() != conf.size()) > + /* Test that the configuration is not empty. */ > + if (config.empty()) > return false; > > - /* > - * Test that stream can be found in configuration and that the > - * configuration is valid. > - */ > - for (Stream *stream : streams) { > - std::map<Stream *, StreamConfiguration>::const_iterator it = > - conf.find(stream); > - > - if (it == conf.end()) > - return false; > - > - const StreamConfiguration *sconf = &it->second; > - if (sconf->width == 0 || sconf->height == 0 || > - sconf->pixelFormat == 0 || sconf->bufferCount == 0) > + /* Test that configuration is valid. */ > + for (auto const &it : config) { > + const StreamConfiguration &conf = it.second; empty line maybe? > + if (conf.width == 0 || conf.height == 0 || > + conf.pixelFormat == 0 || conf.bufferCount == 0) Should we do any sanity check on the Stream * too? Like it's unique in the map? This might not be required, as we would actually validate the pipeline handler in that case, but... Thanks j > return false; > } > > diff --git a/test/camera/camera_test.h b/test/camera/camera_test.h > index 48fb47a23fe8f49c..5801fad3281e1653 100644 > --- a/test/camera/camera_test.h > +++ b/test/camera/camera_test.h > @@ -23,8 +23,7 @@ protected: > int init(); > void cleanup(); > > - bool configurationValid(const std::set<Stream *> &streams, > - const std::map<Stream *, StreamConfiguration> &conf) const; > + bool configurationValid(const std::map<Stream *, StreamConfiguration> &config) const; > > std::shared_ptr<Camera> camera_; > > diff --git a/test/camera/capture.cpp b/test/camera/capture.cpp > index 28eb61405d90a4c7..f6932b7505571712 100644 > --- a/test/camera/capture.cpp > +++ b/test/camera/capture.cpp > @@ -48,7 +48,7 @@ protected: > camera_->streamConfiguration(streams); > StreamConfiguration *sconf = &conf.begin()->second; > > - if (!configurationValid(streams, conf)) { > + if (!configurationValid(conf)) { > cout << "Failed to read default configuration" << endl; > return TestFail; > } > diff --git a/test/camera/configuration_default.cpp b/test/camera/configuration_default.cpp > index 71e79844667591b2..53ee021d33ca39b1 100644 > --- a/test/camera/configuration_default.cpp > +++ b/test/camera/configuration_default.cpp > @@ -32,7 +32,7 @@ protected: > return TestFail; > } > > - if (!configurationValid(streams, conf)) { > + if (!configurationValid(conf)) { > cout << "Default configuration invalid" << endl; > return TestFail; > } > diff --git a/test/camera/configuration_set.cpp b/test/camera/configuration_set.cpp > index dedb85009335aa46..cac1da959f241bc5 100644 > --- a/test/camera/configuration_set.cpp > +++ b/test/camera/configuration_set.cpp > @@ -23,7 +23,7 @@ protected: > camera_->streamConfiguration(streams); > StreamConfiguration *sconf = &conf.begin()->second; > > - if (!configurationValid(streams, conf)) { > + if (!configurationValid(conf)) { > cout << "Failed to read default configuration" << endl; > return TestFail; > } > -- > 2.21.0 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Jacopo, Thanks for your feedback. On 2019-04-02 09:42:11 +0200, Jacopo Mondi wrote: > Hi Niklas > > On Tue, Apr 02, 2019 at 02:53:29AM +0200, Niklas Söderlund wrote: > > In preparation of reworking how a default configuration is retrieved > > from a camera remove the streams and validation using the stream when > > judging if a camera configuration is valid. This is needed as once > > stream usage hints are added applications will no longer fetch default > > configuration based on Stream IDs so using them to verify the returned > > format is not useful. > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > --- > > test/camera/camera_test.cpp | 26 ++++++++------------------ > > test/camera/camera_test.h | 3 +-- > > test/camera/capture.cpp | 2 +- > > test/camera/configuration_default.cpp | 2 +- > > test/camera/configuration_set.cpp | 2 +- > > 5 files changed, 12 insertions(+), 23 deletions(-) > > > > diff --git a/test/camera/camera_test.cpp b/test/camera/camera_test.cpp > > index a92f2165bf3a53c1..5985b85c44816e30 100644 > > --- a/test/camera/camera_test.cpp > > +++ b/test/camera/camera_test.cpp > > @@ -46,27 +46,17 @@ void CameraTest::cleanup() > > cm_->stop(); > > }; > > > > -bool CameraTest::configurationValid(const std::set<Stream *> &streams, > > - const std::map<Stream *, StreamConfiguration> &conf) const > > +bool CameraTest::configurationValid(const std::map<Stream *, StreamConfiguration> &config) const > > { > > - /* Test that the numbers of streams matches that of configuration. */ > > - if (streams.size() != conf.size()) > > + /* Test that the configuration is not empty. */ > > + if (config.empty()) > > return false; > > > > - /* > > - * Test that stream can be found in configuration and that the > > - * configuration is valid. > > - */ > > - for (Stream *stream : streams) { > > - std::map<Stream *, StreamConfiguration>::const_iterator it = > > - conf.find(stream); > > - > > - if (it == conf.end()) > > - return false; > > - > > - const StreamConfiguration *sconf = &it->second; > > - if (sconf->width == 0 || sconf->height == 0 || > > - sconf->pixelFormat == 0 || sconf->bufferCount == 0) > > + /* Test that configuration is valid. */ > > + for (auto const &it : config) { > > + const StreamConfiguration &conf = it.second; > > empty line maybe? I have no strong opinion, will add for next version. > > > + if (conf.width == 0 || conf.height == 0 || > > + conf.pixelFormat == 0 || conf.bufferCount == 0) > > Should we do any sanity check on the Stream * too? Like it's unique in > the map? This might not be required, as we would actually validate the > pipeline handler in that case, but... The Stream * is always unique as it's the key of the map :-) > > Thanks > j > > > return false; > > } > > > > diff --git a/test/camera/camera_test.h b/test/camera/camera_test.h > > index 48fb47a23fe8f49c..5801fad3281e1653 100644 > > --- a/test/camera/camera_test.h > > +++ b/test/camera/camera_test.h > > @@ -23,8 +23,7 @@ protected: > > int init(); > > void cleanup(); > > > > - bool configurationValid(const std::set<Stream *> &streams, > > - const std::map<Stream *, StreamConfiguration> &conf) const; > > + bool configurationValid(const std::map<Stream *, StreamConfiguration> &config) const; > > > > std::shared_ptr<Camera> camera_; > > > > diff --git a/test/camera/capture.cpp b/test/camera/capture.cpp > > index 28eb61405d90a4c7..f6932b7505571712 100644 > > --- a/test/camera/capture.cpp > > +++ b/test/camera/capture.cpp > > @@ -48,7 +48,7 @@ protected: > > camera_->streamConfiguration(streams); > > StreamConfiguration *sconf = &conf.begin()->second; > > > > - if (!configurationValid(streams, conf)) { > > + if (!configurationValid(conf)) { > > cout << "Failed to read default configuration" << endl; > > return TestFail; > > } > > diff --git a/test/camera/configuration_default.cpp b/test/camera/configuration_default.cpp > > index 71e79844667591b2..53ee021d33ca39b1 100644 > > --- a/test/camera/configuration_default.cpp > > +++ b/test/camera/configuration_default.cpp > > @@ -32,7 +32,7 @@ protected: > > return TestFail; > > } > > > > - if (!configurationValid(streams, conf)) { > > + if (!configurationValid(conf)) { > > cout << "Default configuration invalid" << endl; > > return TestFail; > > } > > diff --git a/test/camera/configuration_set.cpp b/test/camera/configuration_set.cpp > > index dedb85009335aa46..cac1da959f241bc5 100644 > > --- a/test/camera/configuration_set.cpp > > +++ b/test/camera/configuration_set.cpp > > @@ -23,7 +23,7 @@ protected: > > camera_->streamConfiguration(streams); > > StreamConfiguration *sconf = &conf.begin()->second; > > > > - if (!configurationValid(streams, conf)) { > > + if (!configurationValid(conf)) { > > cout << "Failed to read default configuration" << endl; > > return TestFail; > > } > > -- > > 2.21.0 > > > > _______________________________________________ > > libcamera-devel mailing list > > libcamera-devel@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel
On Tue, Apr 02, 2019 at 02:53:29AM +0200, Niklas Söderlund wrote: > In preparation of reworking how a default configuration is retrieved > from a camera remove the streams and validation using the stream when > judging if a camera configuration is valid. This is needed as once > stream usage hints are added applications will no longer fetch default > configuration based on Stream IDs so using them to verify the returned > format is not useful. > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > test/camera/camera_test.cpp | 26 ++++++++------------------ > test/camera/camera_test.h | 3 +-- > test/camera/capture.cpp | 2 +- > test/camera/configuration_default.cpp | 2 +- > test/camera/configuration_set.cpp | 2 +- > 5 files changed, 12 insertions(+), 23 deletions(-) > > diff --git a/test/camera/camera_test.cpp b/test/camera/camera_test.cpp > index a92f2165bf3a53c1..5985b85c44816e30 100644 > --- a/test/camera/camera_test.cpp > +++ b/test/camera/camera_test.cpp > @@ -46,27 +46,17 @@ void CameraTest::cleanup() > cm_->stop(); > }; > > -bool CameraTest::configurationValid(const std::set<Stream *> &streams, > - const std::map<Stream *, StreamConfiguration> &conf) const > +bool CameraTest::configurationValid(const std::map<Stream *, StreamConfiguration> &config) const > { > - /* Test that the numbers of streams matches that of configuration. */ > - if (streams.size() != conf.size()) > + /* Test that the configuration is not empty. */ > + if (config.empty()) > return false; > > - /* > - * Test that stream can be found in configuration and that the > - * configuration is valid. > - */ > - for (Stream *stream : streams) { > - std::map<Stream *, StreamConfiguration>::const_iterator it = > - conf.find(stream); > - > - if (it == conf.end()) > - return false; > - > - const StreamConfiguration *sconf = &it->second; > - if (sconf->width == 0 || sconf->height == 0 || > - sconf->pixelFormat == 0 || sconf->bufferCount == 0) > + /* Test that configuration is valid. */ > + for (auto const &it : config) { > + const StreamConfiguration &conf = it.second; > + if (conf.width == 0 || conf.height == 0 || > + conf.pixelFormat == 0 || conf.bufferCount == 0) > return false; > } > > diff --git a/test/camera/camera_test.h b/test/camera/camera_test.h > index 48fb47a23fe8f49c..5801fad3281e1653 100644 > --- a/test/camera/camera_test.h > +++ b/test/camera/camera_test.h > @@ -23,8 +23,7 @@ protected: > int init(); > void cleanup(); > > - bool configurationValid(const std::set<Stream *> &streams, > - const std::map<Stream *, StreamConfiguration> &conf) const; > + bool configurationValid(const std::map<Stream *, StreamConfiguration> &config) const; > > std::shared_ptr<Camera> camera_; > > diff --git a/test/camera/capture.cpp b/test/camera/capture.cpp > index 28eb61405d90a4c7..f6932b7505571712 100644 > --- a/test/camera/capture.cpp > +++ b/test/camera/capture.cpp > @@ -48,7 +48,7 @@ protected: > camera_->streamConfiguration(streams); > StreamConfiguration *sconf = &conf.begin()->second; > > - if (!configurationValid(streams, conf)) { > + if (!configurationValid(conf)) { > cout << "Failed to read default configuration" << endl; > return TestFail; > } > diff --git a/test/camera/configuration_default.cpp b/test/camera/configuration_default.cpp > index 71e79844667591b2..53ee021d33ca39b1 100644 > --- a/test/camera/configuration_default.cpp > +++ b/test/camera/configuration_default.cpp > @@ -32,7 +32,7 @@ protected: > return TestFail; > } > > - if (!configurationValid(streams, conf)) { > + if (!configurationValid(conf)) { > cout << "Default configuration invalid" << endl; > return TestFail; > } > diff --git a/test/camera/configuration_set.cpp b/test/camera/configuration_set.cpp > index dedb85009335aa46..cac1da959f241bc5 100644 > --- a/test/camera/configuration_set.cpp > +++ b/test/camera/configuration_set.cpp > @@ -23,7 +23,7 @@ protected: > camera_->streamConfiguration(streams); > StreamConfiguration *sconf = &conf.begin()->second; > > - if (!configurationValid(streams, conf)) { > + if (!configurationValid(conf)) { > cout << "Failed to read default configuration" << endl; > return TestFail; > }
diff --git a/test/camera/camera_test.cpp b/test/camera/camera_test.cpp index a92f2165bf3a53c1..5985b85c44816e30 100644 --- a/test/camera/camera_test.cpp +++ b/test/camera/camera_test.cpp @@ -46,27 +46,17 @@ void CameraTest::cleanup() cm_->stop(); }; -bool CameraTest::configurationValid(const std::set<Stream *> &streams, - const std::map<Stream *, StreamConfiguration> &conf) const +bool CameraTest::configurationValid(const std::map<Stream *, StreamConfiguration> &config) const { - /* Test that the numbers of streams matches that of configuration. */ - if (streams.size() != conf.size()) + /* Test that the configuration is not empty. */ + if (config.empty()) return false; - /* - * Test that stream can be found in configuration and that the - * configuration is valid. - */ - for (Stream *stream : streams) { - std::map<Stream *, StreamConfiguration>::const_iterator it = - conf.find(stream); - - if (it == conf.end()) - return false; - - const StreamConfiguration *sconf = &it->second; - if (sconf->width == 0 || sconf->height == 0 || - sconf->pixelFormat == 0 || sconf->bufferCount == 0) + /* Test that configuration is valid. */ + for (auto const &it : config) { + const StreamConfiguration &conf = it.second; + if (conf.width == 0 || conf.height == 0 || + conf.pixelFormat == 0 || conf.bufferCount == 0) return false; } diff --git a/test/camera/camera_test.h b/test/camera/camera_test.h index 48fb47a23fe8f49c..5801fad3281e1653 100644 --- a/test/camera/camera_test.h +++ b/test/camera/camera_test.h @@ -23,8 +23,7 @@ protected: int init(); void cleanup(); - bool configurationValid(const std::set<Stream *> &streams, - const std::map<Stream *, StreamConfiguration> &conf) const; + bool configurationValid(const std::map<Stream *, StreamConfiguration> &config) const; std::shared_ptr<Camera> camera_; diff --git a/test/camera/capture.cpp b/test/camera/capture.cpp index 28eb61405d90a4c7..f6932b7505571712 100644 --- a/test/camera/capture.cpp +++ b/test/camera/capture.cpp @@ -48,7 +48,7 @@ protected: camera_->streamConfiguration(streams); StreamConfiguration *sconf = &conf.begin()->second; - if (!configurationValid(streams, conf)) { + if (!configurationValid(conf)) { cout << "Failed to read default configuration" << endl; return TestFail; } diff --git a/test/camera/configuration_default.cpp b/test/camera/configuration_default.cpp index 71e79844667591b2..53ee021d33ca39b1 100644 --- a/test/camera/configuration_default.cpp +++ b/test/camera/configuration_default.cpp @@ -32,7 +32,7 @@ protected: return TestFail; } - if (!configurationValid(streams, conf)) { + if (!configurationValid(conf)) { cout << "Default configuration invalid" << endl; return TestFail; } diff --git a/test/camera/configuration_set.cpp b/test/camera/configuration_set.cpp index dedb85009335aa46..cac1da959f241bc5 100644 --- a/test/camera/configuration_set.cpp +++ b/test/camera/configuration_set.cpp @@ -23,7 +23,7 @@ protected: camera_->streamConfiguration(streams); StreamConfiguration *sconf = &conf.begin()->second; - if (!configurationValid(streams, conf)) { + if (!configurationValid(conf)) { cout << "Failed to read default configuration" << endl; return TestFail; }
In preparation of reworking how a default configuration is retrieved from a camera remove the streams and validation using the stream when judging if a camera configuration is valid. This is needed as once stream usage hints are added applications will no longer fetch default configuration based on Stream IDs so using them to verify the returned format is not useful. Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> --- test/camera/camera_test.cpp | 26 ++++++++------------------ test/camera/camera_test.h | 3 +-- test/camera/capture.cpp | 2 +- test/camera/configuration_default.cpp | 2 +- test/camera/configuration_set.cpp | 2 +- 5 files changed, 12 insertions(+), 23 deletions(-)