[libcamera-devel] [RFC 2/5] test: camera: Remove streams argument from configurationValid()

[Niklas Söderlund]

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 at 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 at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel



-- 
Regards,
Niklas Söderlund