Hi Niklas,
Thank you for the patch.
On Mon, Jan 28, 2019 at 01:41:09AM +0100, Niklas Söderlund wrote:
> Add a option to configure the first stream of a camera from an argument
s/a option/an option/
> with options and parse the width, height and pixel format from that
> list.
>
> The pixel format is still specified as a integer which should correspond
> to the kernels FOURCC identifiers. Going forward this should be turned
> into a string representation and the cam parser should translate between
> the two.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
> src/cam/main.cpp | 104 +++++++++++++++++++++++++++++++++++++++++------
> 1 file changed, 92 insertions(+), 12 deletions(-)
>
> diff --git a/src/cam/main.cpp b/src/cam/main.cpp
> index bde47a8f17983912..4b4ce9aa29c80bd1 100644
> --- a/src/cam/main.cpp
> +++ b/src/cam/main.cpp
> @@ -20,7 +20,8 @@ using namespace libcamera;
> OptionsParser::Options options;
>
> enum {
> - OptCamera = 'c',
> + OptCamera = 'C',
Why do you rename this option ?
> + OptFormat = 'f',
> OptHelp = 'h',
> OptList = 'l',
> };
> @@ -35,10 +36,20 @@ void signalHandler(int signal)
>
> static int parseOptions(int argc, char *argv[])
> {
> + KeyValueParser formatKeyValue;
> + formatKeyValue.addOption("width", "Set width in pixels",
I'd drop the "Set " part: "Width in pixels". Same for the other options.
> + ArgumentRequired, "w");
I wonder if we need the argumentName. Is "width=w" better than
"width=value" ?
> + formatKeyValue.addOption("height", "Set height in pixels",
> + ArgumentRequired, "h");
> + formatKeyValue.addOption("pixelformat", "Set pixel format",
> + ArgumentRequired, "pf");
> +
> OptionsParser parser;
>
> parser.addOption(OptCamera, "Specify which camera to operate on",
> "camera", ArgumentRequired, "camera");
> + parser.addOption(OptFormat, "Set format of the cameras first stream",
s/cameras/camera's/
> + formatKeyValue, "format");
> parser.addOption(OptHelp, "Display this help message", "help");
> parser.addOption(OptList, "List all cameras", "list");
>
> @@ -54,6 +65,42 @@ static int parseOptions(int argc, char *argv[])
> return 0;
> }
>
> +int str2uint(std::string str, unsigned int *dest)
> +{
> + if (!dest || str == "" || !sscanf(str.c_str(), "%d", dest))
Wouldn't strtoul be simpler than sscanf ?
> + return -EINVAL;
> + return 0;
> +}
> +
> +bool configureStreams(Camera *camera, std::vector<Stream> &streams)
> +{
> + if (!options.isKeyValue(OptFormat))
Can this happen, given that OptFormat is a key-value argument ? I would
drop this check, and possibly the isKeyValue() method of the parser if
not needed elsewhere.
> + return false;
> +
> + KeyValueParser::Options format = options.keyValues(OptFormat);
> +
> + std::map<unsigned int, StreamConfiguration> config = camera->streamConfiguration(streams);
> + unsigned int id = streams.front().id();
> +
> + if (format.isSet("width"))
> + if (str2uint(format["width"], &config[id].width))
> + return false;
Down the road I would like the parser to take a value type argument, and
expose an API that will automatically convert to the right type.
> +
> + if (format.isSet("height"))
> + if (str2uint(format["height"], &config[id].height))
> + return false;
> +
> + /* TODO: Translate 4CC string to ID. */
> + if (format.isSet("pixelformat"))
> + if (str2uint(format["pixelformat"], &config[id].pixelFormat))
> + return false;
> +
> + if (camera->configureStreams(config))
> + return false;
> +
> + return true;
> +}
> +
> int main(int argc, char **argv)
> {
> int ret;
> @@ -63,6 +110,8 @@ int main(int argc, char **argv)
> return EXIT_FAILURE;
>
> CameraManager *cm = CameraManager::instance();
> + std::shared_ptr<Camera> camera;
> + std::vector<Stream> streams;
>
> ret = cm->start();
> if (ret) {
> @@ -71,31 +120,62 @@ int main(int argc, char **argv)
> return EXIT_FAILURE;
> }
>
> + loop = new EventLoop(cm->eventDispatcher());
> +
> + struct sigaction sa = {};
> + sa.sa_handler = &signalHandler;
> + sigaction(SIGINT, &sa, nullptr);
> +
> if (options.isSet(OptList)) {
> std::cout << "Available cameras:" << std::endl;
> - for (const std::shared_ptr<Camera> &camera : cm->cameras())
> - std::cout << "- " << camera->name() << std::endl;
> + for (const std::shared_ptr<Camera> &cam : cm->cameras())
> + std::cout << "- " << cam->name() << std::endl;
> }
>
> if (options.isSet(OptCamera)) {
> - std::shared_ptr<Camera> cam = cm->get(options[OptCamera]);
> -
> - if (cam) {
> - std::cout << "Using camera " << cam->name() << std::endl;
> - } else {
> + camera = cm->get(options[OptCamera]);
> + if (!camera) {
> std::cout << "Camera " << options[OptCamera]
> << " not found" << std::endl;
Should errors be printed to cerr ?
> + goto out;
> + }
> +
> + streams = camera->streams();
> + if (streams.size() != 1) {
> + std::cout << "Camera have " << streams.size()
s/have/has/
> + << " streams, I only know how to work with 1"
I'm not used to software talking to me, but why not ? :-)
> + << std::endl;
> + goto out;
> + }
> +
> + if (camera->acquire()) {
> + std::cout << "Failed to acquire camera" << std::endl;
> + goto out;
> }
> +
> + std::cout << "Using camera " << camera->name() << std::endl;
> }
>
> - loop = new EventLoop(cm->eventDispatcher());
> + if (options.isSet(OptFormat)) {
> + if (!camera) {
> + std::cout << "Can't configure stream, no camera selected" << std::endl;
> + goto out_camera;
> + }
I think we'll end up with the cam tool working in one of a small subset
of modes. I foresee at least a list mode and a capture mode, and
probably a few others, but not many. I'd make them mutually exclusive,
with list returning immediately after printing the list of cameras, and
capture requiring a camera, so this check will likely be moved out of
the OptFormat check. We can refactor this later.
>
> - struct sigaction sa = {};
> - sa.sa_handler = &signalHandler;
> - sigaction(SIGINT, &sa, nullptr);
> + if (!configureStreams(camera.get(), streams)) {
> + std::cout << "Failed to configure camera" << std::endl;
> + goto out_camera;
> + }
> + }
>
> ret = loop->exec();
>
> +out_camera:
> + if (camera) {
> + camera->release();
> + camera.reset();
> + }
> +out:
> delete loop;
>
> cm->stop();
--
Regards,
Laurent Pinchart