[libcamera-devel,RFC,1/5] cam: Rework how streams configuration is prepared
diff mbox series

Message ID 20190402005332.25018-2-niklas.soderlund@ragnatech.se
State Accepted
Headers show
Series
  • libcamera: camera: Add support for stream usage hint
Related show

Commit Message

Niklas Söderlund April 2, 2019, 12:53 a.m. UTC 
In preparation of reworking how a default configuration is retrieved
from a camera separate preparation of stream configuration and applying
of it into to different functions. Reason for this is that preparation
of camera configuration will become more complex.

Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
---
 src/cam/main.cpp | 35 ++++++++++++++++++++---------------
 1 file changed, 20 insertions(+), 15 deletions(-)

Comments

Jacopo Mondi April 2, 2019, 7:38 a.m. UTC | #1 
Hi Niklas,

On Tue, Apr 02, 2019 at 02:53:28AM +0200, Niklas Söderlund wrote:
> In preparation of reworking how a default configuration is retrieved
> from a camera separate preparation of stream configuration and applying
> of it into to different functions. Reason for this is that preparation
> of camera configuration will become more complex.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> ---
>  src/cam/main.cpp | 35 ++++++++++++++++++++---------------
>  1 file changed, 20 insertions(+), 15 deletions(-)
>
> diff --git a/src/cam/main.cpp b/src/cam/main.cpp
> index e7490c32f99a01ad..cc5302ca4e72ea6f 100644
> --- a/src/cam/main.cpp
> +++ b/src/cam/main.cpp
> @@ -78,27 +78,27 @@ static int parseOptions(int argc, char *argv[])
>  	return 0;
>  }
>
> -static int configureStreams(Camera *camera, std::set<Stream *> &streams)
> +static int prepare_camera_config(std::map<Stream *, StreamConfiguration> *config)

Why have you here gone for name_with_underscores while the rest of the
code uses camelCaseNames ?

>  {
> -	KeyValueParser::Options format = options[OptFormat];
> -	Stream *id = *streams.begin();
> -
> -	std::map<Stream *, StreamConfiguration> config =
> -		camera->streamConfiguration(streams);
> +	std::set<Stream *> streams = camera->streams();
> +	*config = camera->streamConfiguration(streams);
> +	Stream *stream = config->begin()->first;
>
>  	if (options.isSet(OptFormat)) {
> +		KeyValueParser::Options format = options[OptFormat];
> +
>  		if (format.isSet("width"))
> -			config[id].width = format["width"];
> +			(*config)[stream].width = format["width"];
>
>  		if (format.isSet("height"))
> -			config[id].height = format["height"];
> +			(*config)[stream].height = format["height"];
>
>  		/* TODO: Translate 4CC string to ID. */
>  		if (format.isSet("pixelformat"))
> -			config[id].pixelFormat = format["pixelformat"];
> +			(*config)[stream].pixelFormat = format["pixelformat"];
>  	}
>
> -	return camera->configureStreams(config);
> +	return 0;
>  }
>
>  static void requestComplete(Request *request, const std::map<Stream *, Buffer *> &buffers)
> @@ -136,18 +136,23 @@ static void requestComplete(Request *request, const std::map<Stream *, Buffer *>
>
>  static int capture()
>  {
> -	int ret;
> -
> -	std::set<Stream *> streams = camera->streams();
> +	std::map<Stream *, StreamConfiguration> config;
>  	std::vector<Request *> requests;
> +	int ret;
>
> -	ret = configureStreams(camera.get(), streams);
> +	ret = prepare_camera_config(&config);
> +	if (ret) {
> +		std::cout << "Failed to prepare camera configuration" << std::endl;
> +		return ret;
> +	}
> +
> +	ret = camera->configureStreams(config);
>  	if (ret < 0) {
>  		std::cout << "Failed to configure camera" << std::endl;
>  		return ret;
>  	}
>
> -	Stream *stream = *streams.begin();
> +	Stream *stream = config.begin()->first;
>
>  	ret = camera->allocateBuffers();
>  	if (ret) {
> --
> 2.21.0
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Niklas Söderlund April 2, 2019, 11:46 a.m. UTC | #2 
Hi Jacopo,

Thanks for your feedback.

On 2019-04-02 09:38:10 +0200, Jacopo Mondi wrote:
> Hi Niklas,
> 
> On Tue, Apr 02, 2019 at 02:53:28AM +0200, Niklas Söderlund wrote:
> > In preparation of reworking how a default configuration is retrieved
> > from a camera separate preparation of stream configuration and applying
> > of it into to different functions. Reason for this is that preparation
> > of camera configuration will become more complex.
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > ---
> >  src/cam/main.cpp | 35 ++++++++++++++++++++---------------
> >  1 file changed, 20 insertions(+), 15 deletions(-)
> >
> > diff --git a/src/cam/main.cpp b/src/cam/main.cpp
> > index e7490c32f99a01ad..cc5302ca4e72ea6f 100644
> > --- a/src/cam/main.cpp
> > +++ b/src/cam/main.cpp
> > @@ -78,27 +78,27 @@ static int parseOptions(int argc, char *argv[])
> >  	return 0;
> >  }
> >
> > -static int configureStreams(Camera *camera, std::set<Stream *> &streams)
> > +static int prepare_camera_config(std::map<Stream *, StreamConfiguration> *config)
> 
> Why have you here gone for name_with_underscores while the rest of the
> code uses camelCaseNames ?

Because camel_case is unnatural :-) Will remedy this in next version 
thanks for pointing it out.

> 
> >  {
> > -	KeyValueParser::Options format = options[OptFormat];
> > -	Stream *id = *streams.begin();
> > -
> > -	std::map<Stream *, StreamConfiguration> config =
> > -		camera->streamConfiguration(streams);
> > +	std::set<Stream *> streams = camera->streams();
> > +	*config = camera->streamConfiguration(streams);
> > +	Stream *stream = config->begin()->first;
> >
> >  	if (options.isSet(OptFormat)) {
> > +		KeyValueParser::Options format = options[OptFormat];
> > +
> >  		if (format.isSet("width"))
> > -			config[id].width = format["width"];
> > +			(*config)[stream].width = format["width"];
> >
> >  		if (format.isSet("height"))
> > -			config[id].height = format["height"];
> > +			(*config)[stream].height = format["height"];
> >
> >  		/* TODO: Translate 4CC string to ID. */
> >  		if (format.isSet("pixelformat"))
> > -			config[id].pixelFormat = format["pixelformat"];
> > +			(*config)[stream].pixelFormat = format["pixelformat"];
> >  	}
> >
> > -	return camera->configureStreams(config);
> > +	return 0;
> >  }
> >
> >  static void requestComplete(Request *request, const std::map<Stream *, Buffer *> &buffers)
> > @@ -136,18 +136,23 @@ static void requestComplete(Request *request, const std::map<Stream *, Buffer *>
> >
> >  static int capture()
> >  {
> > -	int ret;
> > -
> > -	std::set<Stream *> streams = camera->streams();
> > +	std::map<Stream *, StreamConfiguration> config;
> >  	std::vector<Request *> requests;
> > +	int ret;
> >
> > -	ret = configureStreams(camera.get(), streams);
> > +	ret = prepare_camera_config(&config);
> > +	if (ret) {
> > +		std::cout << "Failed to prepare camera configuration" << std::endl;
> > +		return ret;
> > +	}
> > +
> > +	ret = camera->configureStreams(config);
> >  	if (ret < 0) {
> >  		std::cout << "Failed to configure camera" << std::endl;
> >  		return ret;
> >  	}
> >
> > -	Stream *stream = *streams.begin();
> > +	Stream *stream = config.begin()->first;
> >
> >  	ret = camera->allocateBuffers();
> >  	if (ret) {
> > --
> > 2.21.0
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel@lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
Laurent Pinchart April 2, 2019, 2:16 p.m. UTC | #3 
Hi Niklas,

Thank you for the patch.

On Tue, Apr 02, 2019 at 02:53:28AM +0200, Niklas Söderlund wrote:
> In preparation of reworking how a default configuration is retrieved
> from a camera separate preparation of stream configuration and applying
> of it into to different functions. Reason for this is that preparation

s/into to/into two/

"applying of it" could also be replaced by "application".

> of camera configuration will become more complex.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> ---
>  src/cam/main.cpp | 35 ++++++++++++++++++++---------------
>  1 file changed, 20 insertions(+), 15 deletions(-)
> 
> diff --git a/src/cam/main.cpp b/src/cam/main.cpp
> index e7490c32f99a01ad..cc5302ca4e72ea6f 100644
> --- a/src/cam/main.cpp
> +++ b/src/cam/main.cpp
> @@ -78,27 +78,27 @@ static int parseOptions(int argc, char *argv[])
>  	return 0;
>  }
>  
> -static int configureStreams(Camera *camera, std::set<Stream *> &streams)
> +static int prepare_camera_config(std::map<Stream *, StreamConfiguration> *config)
>  {
> -	KeyValueParser::Options format = options[OptFormat];
> -	Stream *id = *streams.begin();
> -
> -	std::map<Stream *, StreamConfiguration> config =
> -		camera->streamConfiguration(streams);
> +	std::set<Stream *> streams = camera->streams();
> +	*config = camera->streamConfiguration(streams);
> +	Stream *stream = config->begin()->first;
>  
>  	if (options.isSet(OptFormat)) {
> +		KeyValueParser::Options format = options[OptFormat];
> +
>  		if (format.isSet("width"))
> -			config[id].width = format["width"];
> +			(*config)[stream].width = format["width"];
>  
>  		if (format.isSet("height"))
> -			config[id].height = format["height"];
> +			(*config)[stream].height = format["height"];
>  
>  		/* TODO: Translate 4CC string to ID. */
>  		if (format.isSet("pixelformat"))
> -			config[id].pixelFormat = format["pixelformat"];
> +			(*config)[stream].pixelFormat = format["pixelformat"];
>  	}
>  
> -	return camera->configureStreams(config);
> +	return 0;
>  }
>  
>  static void requestComplete(Request *request, const std::map<Stream *, Buffer *> &buffers)
> @@ -136,18 +136,23 @@ static void requestComplete(Request *request, const std::map<Stream *, Buffer *>
>  
>  static int capture()
>  {
> -	int ret;
> -
> -	std::set<Stream *> streams = camera->streams();
> +	std::map<Stream *, StreamConfiguration> config;
>  	std::vector<Request *> requests;
> +	int ret;
>  
> -	ret = configureStreams(camera.get(), streams);
> +	ret = prepare_camera_config(&config);

Would it make sense to return the map by value instead of passing a
pointer to it ? Apart from that,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +	if (ret) {
> +		std::cout << "Failed to prepare camera configuration" << std::endl;
> +		return ret;
> +	}
> +
> +	ret = camera->configureStreams(config);
>  	if (ret < 0) {
>  		std::cout << "Failed to configure camera" << std::endl;
>  		return ret;
>  	}
>  
> -	Stream *stream = *streams.begin();
> +	Stream *stream = config.begin()->first;
>  
>  	ret = camera->allocateBuffers();
>  	if (ret) {
Niklas Söderlund April 2, 2019, 6:49 p.m. UTC | #4 
Hi Laurent,

Thanks for your feedback.

On 2019-04-02 17:16:23 +0300, Laurent Pinchart wrote:
> Hi Niklas,
> 
> Thank you for the patch.
> 
> On Tue, Apr 02, 2019 at 02:53:28AM +0200, Niklas Söderlund wrote:
> > In preparation of reworking how a default configuration is retrieved
> > from a camera separate preparation of stream configuration and applying
> > of it into to different functions. Reason for this is that preparation
> 
> s/into to/into two/
> 
> "applying of it" could also be replaced by "application".
> 
> > of camera configuration will become more complex.
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > ---
> >  src/cam/main.cpp | 35 ++++++++++++++++++++---------------
> >  1 file changed, 20 insertions(+), 15 deletions(-)
> > 
> > diff --git a/src/cam/main.cpp b/src/cam/main.cpp
> > index e7490c32f99a01ad..cc5302ca4e72ea6f 100644
> > --- a/src/cam/main.cpp
> > +++ b/src/cam/main.cpp
> > @@ -78,27 +78,27 @@ static int parseOptions(int argc, char *argv[])
> >  	return 0;
> >  }
> >  
> > -static int configureStreams(Camera *camera, std::set<Stream *> &streams)
> > +static int prepare_camera_config(std::map<Stream *, StreamConfiguration> *config)
> >  {
> > -	KeyValueParser::Options format = options[OptFormat];
> > -	Stream *id = *streams.begin();
> > -
> > -	std::map<Stream *, StreamConfiguration> config =
> > -		camera->streamConfiguration(streams);
> > +	std::set<Stream *> streams = camera->streams();
> > +	*config = camera->streamConfiguration(streams);
> > +	Stream *stream = config->begin()->first;
> >  
> >  	if (options.isSet(OptFormat)) {
> > +		KeyValueParser::Options format = options[OptFormat];
> > +
> >  		if (format.isSet("width"))
> > -			config[id].width = format["width"];
> > +			(*config)[stream].width = format["width"];
> >  
> >  		if (format.isSet("height"))
> > -			config[id].height = format["height"];
> > +			(*config)[stream].height = format["height"];
> >  
> >  		/* TODO: Translate 4CC string to ID. */
> >  		if (format.isSet("pixelformat"))
> > -			config[id].pixelFormat = format["pixelformat"];
> > +			(*config)[stream].pixelFormat = format["pixelformat"];
> >  	}
> >  
> > -	return camera->configureStreams(config);
> > +	return 0;
> >  }
> >  
> >  static void requestComplete(Request *request, const std::map<Stream *, Buffer *> &buffers)
> > @@ -136,18 +136,23 @@ static void requestComplete(Request *request, const std::map<Stream *, Buffer *>
> >  
> >  static int capture()
> >  {
> > -	int ret;
> > -
> > -	std::set<Stream *> streams = camera->streams();
> > +	std::map<Stream *, StreamConfiguration> config;
> >  	std::vector<Request *> requests;
> > +	int ret;
> >  
> > -	ret = configureStreams(camera.get(), streams);
> > +	ret = prepare_camera_config(&config);
> 
> Would it make sense to return the map by value instead of passing a
> pointer to it ? Apart from that,

I have done so in the past in other places but I'm starting to dislike 
returning an empty map to indicate an error. I will keep it as is for v1 
but I'm open to change this.

> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> > +	if (ret) {
> > +		std::cout << "Failed to prepare camera configuration" << std::endl;
> > +		return ret;
> > +	}
> > +
> > +	ret = camera->configureStreams(config);
> >  	if (ret < 0) {
> >  		std::cout << "Failed to configure camera" << std::endl;
> >  		return ret;
> >  	}
> >  
> > -	Stream *stream = *streams.begin();
> > +	Stream *stream = config.begin()->first;
> >  
> >  	ret = camera->allocateBuffers();
> >  	if (ret) {
> 
> -- 
> Regards,
> 
> Laurent Pinchart

Patch
diff mbox series 

diff --git a/src/cam/main.cpp b/src/cam/main.cpp
index e7490c32f99a01ad..cc5302ca4e72ea6f 100644
--- a/src/cam/main.cpp
+++ b/src/cam/main.cpp
@@ -78,27 +78,27 @@  static int parseOptions(int argc, char *argv[])
 	return 0;
 }
 
-static int configureStreams(Camera *camera, std::set<Stream *> &streams)
+static int prepare_camera_config(std::map<Stream *, StreamConfiguration> *config)
 {
-	KeyValueParser::Options format = options[OptFormat];
-	Stream *id = *streams.begin();
-
-	std::map<Stream *, StreamConfiguration> config =
-		camera->streamConfiguration(streams);
+	std::set<Stream *> streams = camera->streams();
+	*config = camera->streamConfiguration(streams);
+	Stream *stream = config->begin()->first;
 
 	if (options.isSet(OptFormat)) {
+		KeyValueParser::Options format = options[OptFormat];
+
 		if (format.isSet("width"))
-			config[id].width = format["width"];
+			(*config)[stream].width = format["width"];
 
 		if (format.isSet("height"))
-			config[id].height = format["height"];
+			(*config)[stream].height = format["height"];
 
 		/* TODO: Translate 4CC string to ID. */
 		if (format.isSet("pixelformat"))
-			config[id].pixelFormat = format["pixelformat"];
+			(*config)[stream].pixelFormat = format["pixelformat"];
 	}
 
-	return camera->configureStreams(config);
+	return 0;
 }
 
 static void requestComplete(Request *request, const std::map<Stream *, Buffer *> &buffers)
@@ -136,18 +136,23 @@  static void requestComplete(Request *request, const std::map<Stream *, Buffer *>
 
 static int capture()
 {
-	int ret;
-
-	std::set<Stream *> streams = camera->streams();
+	std::map<Stream *, StreamConfiguration> config;
 	std::vector<Request *> requests;
+	int ret;
 
-	ret = configureStreams(camera.get(), streams);
+	ret = prepare_camera_config(&config);
+	if (ret) {
+		std::cout << "Failed to prepare camera configuration" << std::endl;
+		return ret;
+	}
+
+	ret = camera->configureStreams(config);
 	if (ret < 0) {
 		std::cout << "Failed to configure camera" << std::endl;
 		return ret;
 	}
 
-	Stream *stream = *streams.begin();
+	Stream *stream = config.begin()->first;
 
 	ret = camera->allocateBuffers();
 	if (ret) {