[media-ctl PATCH v2 1/2] New, more flexible syntax for format

[media-ctl PATCH v2 1/2] New, more flexible syntax for format

[Sakari Ailus]

More flexible and extensible syntax for format which allows better usage
of the selection API.

Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
---
 src/main.c       |   17 ++++++++++---
 src/options.c    |   27 +++++++++++++-------
 src/v4l2subdev.c |   70 ++++++++++++++++++++++++++++++++++--------------------
 3 files changed, 74 insertions(+), 40 deletions(-)

diff --git a/src/main.c b/src/main.c
index 53964e4..2f57352 100644
--- a/src/main.c
+++ b/src/main.c
@@ -59,15 +59,24 @@ static void v4l2_subdev_print_format(struct media_entity *entity,
 	if (ret != 0)
 		return;
 
-	printf("[%s %ux%u", v4l2_subdev_pixelcode_to_string(format.code),
-	       format.width, format.height);
+	printf("\t\t[fmt:%s/%ux%u",
+	       v4l2_subdev_pixelcode_to_string(format.code),
+ 	       format.width, format.height);
+
+	ret = v4l2_subdev_get_selection(entity, &rect, pad,
+					V4L2_SUBDEV_SEL_TGT_CROP_BOUNDS,
+					which);
+	if (ret == 0)
+		printf("\n\t\t crop.bounds:%u,%u/%ux%u", rect.left, rect.top,
+		       rect.width, rect.height);
 
 	ret = v4l2_subdev_get_selection(entity, &rect, pad,
 					V4L2_SUBDEV_SEL_TGT_CROP_ACTUAL,
 					which);
 	if (ret == 0)
-		printf(" (%u,%u)/%ux%u", rect.left, rect.top,
+		printf("\n\t\t crop:%u,%u/%ux%u", rect.left, rect.top,
 		       rect.width, rect.height);
+
 	printf("]");
 }
 
@@ -252,7 +261,7 @@ static void media_print_topology_text(struct media_device *media)
 		for (j = 0; j < entity->info.pads; j++) {
 			struct media_pad *pad = &entity->pads[j];
 
-			printf("\tpad%u: %s ", j, media_pad_type_to_string(pad->flags));
+			printf("\tpad%u: %s\n", j, media_pad_type_to_string(pad->flags));
 
 			if (media_entity_type(entity) == MEDIA_ENT_T_V4L2_SUBDEV)
 				v4l2_subdev_print_format(entity, j, V4L2_SUBDEV_FORMAT_ACTIVE);
diff --git a/src/options.c b/src/options.c
index 60cf6d5..46f6bef 100644
--- a/src/options.c
+++ b/src/options.c
@@ -37,8 +37,8 @@ static void usage(const char *argv0, int verbose)
 	printf("%s [options] device\n", argv0);
 	printf("-d, --device dev	Media device name (default: %s)\n", MEDIA_DEVNAME_DEFAULT);
 	printf("-e, --entity name	Print the device name associated with the given entity\n");
-	printf("-f, --set-format	Comma-separated list of formats to setup\n");
-	printf("    --get-format pad	Print the active format on a given pad\n");
+	printf("-V, --set-v4l2 v4l2	Comma-separated list of formats to setup\n");
+	printf("    --get-v4l2 pad	Print the active format on a given pad\n");
 	printf("-h, --help		Show verbose help and exit\n");
 	printf("-i, --interactive	Modify links interactively\n");
 	printf("-l, --links		Comma-separated list of links descriptors to setup\n");
@@ -52,13 +52,17 @@ static void usage(const char *argv0, int verbose)
 
 	printf("\n");
 	printf("Links and formats are defined as\n");
-	printf("\tlink            = pad, '->', pad, '[', flags, ']' ;\n");
-	printf("\tformat          = pad, '[', fcc, ' ', size, [ ' ', crop ], [ ' ', '@', frame interval ], ']' ;\n");
-	printf("\tpad             = entity, ':', pad number ;\n");
-	printf("\tentity          = entity number | ( '\"', entity name, '\"' ) ;\n");
-	printf("\tsize            = width, 'x', height ;\n");
-	printf("\tcrop            = '(', left, ',', top, ')', '/', size ;\n");
-	printf("\tframe interval  = numerator, '/', denominator ;\n");
+	printf("\tlink                = pad, '->', pad, '[', flags, ']' ;\n");
+	printf("\tv4l2                = pad, '[', v4l2-cfgs ']' ;\n");
+	printf("\tv4l2-cfgs           = v4l2-cfg [ ',' v4l2-cfg ] ;\n");
+	printf("\tv4l2-cfg            = v4l2-mbusfmt | v4l2-crop\n");
+	printf("\t                      | v4l2 frame interval ;\n");
+	printf("\tv4l2-mbusfmt        = 'fmt:', fcc, '/', size ;\n");
+	printf("\tpad                 = entity, ':', pad number ;\n");
+	printf("\tentity              = entity number | ( '\"', entity name, '\"' ) ;\n");
+	printf("\tsize                = width, 'x', height ;\n");
+	printf("\tv4l2-crop           = 'crop:(', left, ',', top, ')/', size ;\n");
+	printf("\tv4l2 frame interval = '@', numerator, '/', denominator ;\n");
 	printf("where the fields are\n");
 	printf("\tentity number   Entity numeric identifier\n");
 	printf("\tentity name     Entity name (string) \n");
@@ -77,7 +81,9 @@ static void usage(const char *argv0, int verbose)
 static struct option opts[] = {
 	{"device", 1, 0, 'd'},
 	{"entity", 1, 0, 'e'},
+	{"set-v4l2", 1, 0, 'V'},
 	{"set-format", 1, 0, 'f'},
+	{"get-v4l2", 1, 0, OPT_GET_FORMAT},
 	{"get-format", 1, 0, OPT_GET_FORMAT},
 	{"help", 0, 0, 'h'},
 	{"interactive", 0, 0, 'i'},
@@ -98,7 +104,7 @@ int parse_cmdline(int argc, char **argv)
 	}
 
 	/* parse options */
-	while ((opt = getopt_long(argc, argv, "d:e:f:hil:prv", opts, NULL)) != -1) {
+	while ((opt = getopt_long(argc, argv, "d:e:V:f:hil:prv", opts, NULL)) != -1) {
 		switch (opt) {
 		case 'd':
 			media_opts.devname = optarg;
@@ -108,6 +114,7 @@ int parse_cmdline(int argc, char **argv)
 			media_opts.entity = optarg;
 			break;
 
+		case 'V':
 		case 'f':
 			media_opts.formats = optarg;
 			break;
diff --git a/src/v4l2subdev.c b/src/v4l2subdev.c
index a2ab0c4..6881553 100644
--- a/src/v4l2subdev.c
+++ b/src/v4l2subdev.c
@@ -233,13 +233,13 @@ static int v4l2_subdev_parse_format(struct v4l2_mbus_framefmt *format,
 	char *end;
 
 	for (; isspace(*p); ++p);
-	for (end = (char *)p; !isspace(*end) && *end != '\0'; ++end);
+	for (end = (char *)p; *end != '/' && *end != '\0'; ++end);
 
 	code = v4l2_subdev_string_to_pixelcode(p, end - p);
 	if (code == (enum v4l2_mbus_pixelcode)-1)
 		return -EINVAL;
 
-	for (p = end; isspace(*p); ++p);
+	p = end + 1;
 	width = strtoul(p, &end, 10);
 	if (*end != 'x')
 		return -EINVAL;
@@ -256,32 +256,32 @@ static int v4l2_subdev_parse_format(struct v4l2_mbus_framefmt *format,
 	return 0;
 }
 
-static int v4l2_subdev_parse_crop(
-	struct v4l2_rect *crop, const char *p, char **endp)
+static int v4l2_subdev_parse_rectangle(
+	struct v4l2_rect *r, const char *p, char **endp)
 {
 	char *end;
 
 	if (*p++ != '(')
 		return -EINVAL;
 
-	crop->left = strtoul(p, &end, 10);
+	r->left = strtoul(p, &end, 10);
 	if (*end != ',')
 		return -EINVAL;
 
 	p = end + 1;
-	crop->top = strtoul(p, &end, 10);
+	r->top = strtoul(p, &end, 10);
 	if (*end++ != ')')
 		return -EINVAL;
 	if (*end != '/')
 		return -EINVAL;
 
 	p = end + 1;
-	crop->width = strtoul(p, &end, 10);
+	r->width = strtoul(p, &end, 10);
 	if (*end != 'x')
 		return -EINVAL;
 
 	p = end + 1;
-	crop->height = strtoul(p, &end, 10);
+	r->height = strtoul(p, &end, 10);
 	*endp = end;
 
 	return 0;
@@ -307,6 +307,17 @@ static int v4l2_subdev_parse_frame_interval(struct v4l2_fract *interval,
 	return 0;
 }
 
+static int strhazit(const char *str, const char **p)
+{
+	int len = strlen(str);
+
+	if (strncmp(str, *p, len))
+		return -ENOENT;
+
+	*p += len;
+	return 0;
+}
+
 static struct media_pad *v4l2_subdev_parse_pad_format(
 	struct media_device *media, struct v4l2_mbus_framefmt *format,
 	struct v4l2_rect *crop, struct v4l2_fract *interval, const char *p,
@@ -326,30 +337,37 @@ static struct media_pad *v4l2_subdev_parse_pad_format(
 	if (*p++ != '[')
 		return NULL;
 
-	for (; isspace(*p); ++p);
+	for (;;) {
+		for (; isspace(*p); p++);
 
-	if (isalnum(*p)) {
-		ret = v4l2_subdev_parse_format(format, p, &end);
-		if (ret < 0)
-			return NULL;
+		if (!strhazit("fmt:", &p)) {
+			ret = v4l2_subdev_parse_format(format, p, &end);
+			if (ret < 0)
+				return NULL;
 
-		for (p = end; isspace(*p); p++);
-	}
+			p = end;
+			continue;
+		}
 
-	if (*p == '(') {
-		ret = v4l2_subdev_parse_crop(crop, p, &end);
-		if (ret < 0)
-			return NULL;
+		if (!strhazit("crop:", &p) || *p == '(') {
+			ret = v4l2_subdev_parse_rectangle(crop, p, &end);
+			if (ret < 0)
+				return NULL;
 
-		for (p = end; isspace(*p); p++);
-	}
+			p = end;
+			continue;
+		}
 
-	if (*p == '@') {
-		ret = v4l2_subdev_parse_frame_interval(interval, ++p, &end);
-		if (ret < 0)
-			return NULL;
+		if (*p == '@') {
+			ret = v4l2_subdev_parse_frame_interval(interval, ++p, &end);
+			if (ret < 0)
+				return NULL;
+
+			p = end;
+			continue;
+		}
 
-		for (p = end; isspace(*p); p++);
+		break;
 	}
 
 	if (*p != ']')
-- 
1.7.2.5

[media-ctl PATCH v2 2/2] Compose rectangle support for libv4l2subdev

[Sakari Ailus]

Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
---
 src/main.c       |   14 ++++++++++++++
 src/options.c    |    6 ++++--
 src/v4l2subdev.c |   32 +++++++++++++++++++++++---------
 3 files changed, 41 insertions(+), 11 deletions(-)

diff --git a/src/main.c b/src/main.c
index 2f57352..a989669 100644
--- a/src/main.c
+++ b/src/main.c
@@ -77,6 +77,20 @@ static void v4l2_subdev_print_format(struct media_entity *entity,
 		printf("\n\t\t crop:%u,%u/%ux%u", rect.left, rect.top,
 		       rect.width, rect.height);
 
+	ret = v4l2_subdev_get_selection(entity, &rect, pad,
+					V4L2_SUBDEV_SEL_TGT_COMPOSE_BOUNDS,
+					which);
+	if (ret == 0)
+		printf("\n\t\t compose.bounds:%u,%u/%ux%u",
+		       rect.left, rect.top, rect.width, rect.height);
+
+	ret = v4l2_subdev_get_selection(entity, &rect, pad,
+					V4L2_SUBDEV_SEL_TGT_COMPOSE_ACTUAL,
+					which);
+	if (ret == 0)
+		printf("\n\t\t compose:%u,%u/%ux%u",
+		       rect.left, rect.top, rect.width, rect.height);
+
 	printf("]");
 }
 
diff --git a/src/options.c b/src/options.c
index 46f6bef..8e80bd0 100644
--- a/src/options.c
+++ b/src/options.c
@@ -56,12 +56,14 @@ static void usage(const char *argv0, int verbose)
 	printf("\tv4l2                = pad, '[', v4l2-cfgs ']' ;\n");
 	printf("\tv4l2-cfgs           = v4l2-cfg [ ',' v4l2-cfg ] ;\n");
 	printf("\tv4l2-cfg            = v4l2-mbusfmt | v4l2-crop\n");
-	printf("\t                      | v4l2 frame interval ;\n");
+	printf("\t                      | v4l2-compose | v4l2 frame interval ;\n");
 	printf("\tv4l2-mbusfmt        = 'fmt:', fcc, '/', size ;\n");
 	printf("\tpad                 = entity, ':', pad number ;\n");
 	printf("\tentity              = entity number | ( '\"', entity name, '\"' ) ;\n");
 	printf("\tsize                = width, 'x', height ;\n");
-	printf("\tv4l2-crop           = 'crop:(', left, ',', top, ')/', size ;\n");
+	printf("\tv4l2-crop           = 'crop:', v4l2-rectangle ;\n");
+	printf("\tv4l2-compose        = 'compose:', v4l2-rectangle ;\n");
+	printf("\tv4l2-rectangle      = '(', left, ',', top, ')/', size ;\n");
 	printf("\tv4l2 frame interval = '@', numerator, '/', denominator ;\n");
 	printf("where the fields are\n");
 	printf("\tentity number   Entity numeric identifier\n");
diff --git a/src/v4l2subdev.c b/src/v4l2subdev.c
index 6881553..0abb4f4 100644
--- a/src/v4l2subdev.c
+++ b/src/v4l2subdev.c
@@ -320,8 +320,8 @@ static int strhazit(const char *str, const char **p)
 
 static struct media_pad *v4l2_subdev_parse_pad_format(
 	struct media_device *media, struct v4l2_mbus_framefmt *format,
-	struct v4l2_rect *crop, struct v4l2_fract *interval, const char *p,
-	char **endp)
+	struct v4l2_rect *crop, struct v4l2_rect *compose,
+	struct v4l2_fract *interval, const char *p, char **endp)
 {
 	struct media_pad *pad;
 	char *end;
@@ -358,6 +358,15 @@ static struct media_pad *v4l2_subdev_parse_pad_format(
 			continue;
 		}
 
+		if (!strhazit("compose:", &p)) {
+			ret = v4l2_subdev_parse_rectangle(compose, p, &end);
+			if (ret < 0)
+				return NULL;
+
+			for (p = end; isspace(*p); p++);
+			continue;
+		}
+
 		if (*p == '@') {
 			ret = v4l2_subdev_parse_frame_interval(interval, ++p, &end);
 			if (ret < 0)
@@ -471,30 +480,35 @@ static int v4l2_subdev_parse_setup_format(struct media_device *media,
 	struct v4l2_mbus_framefmt format = { 0, 0, 0 };
 	struct media_pad *pad;
 	struct v4l2_rect crop = { -1, -1, -1, -1 };
+	struct v4l2_rect compose = crop;
 	struct v4l2_fract interval = { 0, 0 };
 	unsigned int i;
 	char *end;
 	int ret;
 
-	pad = v4l2_subdev_parse_pad_format(media, &format, &crop, &interval,
-					   p, &end);
+	pad = v4l2_subdev_parse_pad_format(media, &format, &crop, &compose,
+					   &interval, p, &end);
 	if (pad == NULL) {
 		media_dbg(media, "Unable to parse format\n");
 		return -EINVAL;
 	}
 
-	if (pad->flags & MEDIA_PAD_FL_SOURCE) {
-		ret = set_selection(pad, V4L2_SUBDEV_SEL_TGT_CROP_ACTUAL, &crop);
+	if (pad->flags & MEDIA_PAD_FL_SINK) {
+		ret = set_format(pad, &format);
 		if (ret < 0)
 			return ret;
 	}
 
-	ret = set_format(pad, &format);
+	ret = set_selection(pad, V4L2_SUBDEV_SEL_TGT_CROP_ACTUAL, &crop);
 	if (ret < 0)
 		return ret;
 
-	if (pad->flags & MEDIA_PAD_FL_SINK) {
-		ret = set_selection(pad, V4L2_SUBDEV_SEL_TGT_CROP_ACTUAL, &crop);
+	ret = set_selection(pad, V4L2_SUBDEV_SEL_TGT_COMPOSE_ACTUAL, &compose);
+	if (ret < 0)
+		return ret;
+
+	if (pad->flags & MEDIA_PAD_FL_SOURCE) {
+		ret = set_format(pad, &format);
 		if (ret < 0)
 			return ret;
 	}
-- 
1.7.2.5

Re: [media-ctl PATCH v2 1/2] New, more flexible syntax for format

[Laurent Pinchart]

Hi Sakari,

Thanks for the patch.

On Monday 07 May 2012 16:46:35 Sakari Ailus wrote:
> More flexible and extensible syntax for format which allows better usage
> of the selection API.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> ---
>  src/main.c       |   17 ++++++++++---
>  src/options.c    |   27 +++++++++++++-------
>  src/v4l2subdev.c |   70 ++++++++++++++++++++++++++++++++-------------------
>  3 files changed, 74 insertions(+), 40 deletions(-)

[snip]

> diff --git a/src/options.c b/src/options.c
> index 60cf6d5..46f6bef 100644
> --- a/src/options.c
> +++ b/src/options.c
> @@ -37,8 +37,8 @@ static void usage(const char *argv0, int verbose)
>  	printf("%s [options] device\n", argv0);
>  	printf("-d, --device dev	Media device name (default: %s)\n",
> MEDIA_DEVNAME_DEFAULT);
> 	printf("-e, --entity name	Print the device name associated with the
> given entity\n");
> -	printf("-f, --set-format	Comma-separated list of formats to
> setup\n");
> -	printf("   --get-format pad	Print the active format on a given pad\n");
> +	printf("-V, --set-v4l2 v4l2	Comma-separated list of formats to setup\n");
> +	printf("   --get-v4l2 pad	Print the active format on a given pad\n");

I still need to think about the name change.

>  	printf("-h, --help		Show verbose help and exit\n");
>  	printf("-i, --interactive	Modify links interactively\n");
>  	printf("-l, --links		Comma-separated list of links descriptors to
> setup\n");
> @@ -52,13 +52,17 @@ static void usage(const char *argv0, int verbose)
> 
>  	printf("\n");
>  	printf("Links and formats are defined as\n");
> -	printf("\tlink            = pad, '->', pad, '[', flags, ']' ;\n");
> -	printf("\tformat          = pad, '[', fcc, ' ', size, [ ' ', crop ], [ '
> ', '@', frame interval ], ']' ;\n"); -	printf("\tpad             = entity,
> ':', pad number ;\n");
> -	printf("\tentity          = entity number | ( '\"', entity name, '\"' )
> ;\n"); -	printf("\tsize            = width, 'x', height ;\n");
> -	printf("\tcrop            = '(', left, ',', top, ')', '/', size ;\n");
> -	printf("\tframe interval  = numerator, '/', denominator ;\n");
> +	printf("\tlink                = pad, '->', pad, '[', flags, ']' ;\n");
> +	printf("\tv4l2                = pad, '[', v4l2-cfgs ']' ;\n");
> +	printf("\tv4l2-cfgs           = v4l2-cfg [ ',' v4l2-cfg ] ;\n");
> +	printf("\tv4l2-cfg            = v4l2-mbusfmt | v4l2-crop\n");
> +	printf("\t                      | v4l2 frame interval ;\n");
> +	printf("\tv4l2-mbusfmt        = 'fmt:', fcc, '/', size ;\n");
> +	printf("\tpad                 = entity, ':', pad number ;\n");
> +	printf("\tentity              = entity number | ( '\"', entity name, '\"'
> ) ;\n"); +	printf("\tsize                = width, 'x', height ;\n");
> +	printf("\tv4l2-crop           = 'crop:(', left, ',', top, ')/', size
> ;\n"); +	printf("\tv4l2 frame interval = '@', numerator, '/', denominator
> ;\n"); printf("where the fields are\n");
>  	printf("\tentity number   Entity numeric identifier\n");
>  	printf("\tentity name     Entity name (string) \n");
> @@ -77,7 +81,9 @@ static void usage(const char *argv0, int verbose)
>  static struct option opts[] = {
>  	{"device", 1, 0, 'd'},
>  	{"entity", 1, 0, 'e'},
> +	{"set-v4l2", 1, 0, 'V'},
>  	{"set-format", 1, 0, 'f'},
> +	{"get-v4l2", 1, 0, OPT_GET_FORMAT},
>  	{"get-format", 1, 0, OPT_GET_FORMAT},
>  	{"help", 0, 0, 'h'},
>  	{"interactive", 0, 0, 'i'},
> @@ -98,7 +104,7 @@ int parse_cmdline(int argc, char **argv)
>  	}
> 
>  	/* parse options */
> -	while ((opt = getopt_long(argc, argv, "d:e:f:hil:prv", opts, NULL)) != 
-1)
> { +	while ((opt = getopt_long(argc, argv, "d:e:V:f:hil:prv", opts, NULL))
> != -1) { switch (opt) {
>  		case 'd':
>  			media_opts.devname = optarg;
> @@ -108,6 +114,7 @@ int parse_cmdline(int argc, char **argv)
>  			media_opts.entity = optarg;
>  			break;
> 
> +		case 'V':
>  		case 'f':
>  			media_opts.formats = optarg;
>  			break;
> diff --git a/src/v4l2subdev.c b/src/v4l2subdev.c
> index a2ab0c4..6881553 100644
> --- a/src/v4l2subdev.c
> +++ b/src/v4l2subdev.c
> @@ -233,13 +233,13 @@ static int v4l2_subdev_parse_format(struct
> v4l2_mbus_framefmt *format, char *end;
> 
>  	for (; isspace(*p); ++p);
> -	for (end = (char *)p; !isspace(*end) && *end != '\0'; ++end);
> +	for (end = (char *)p; *end != '/' && *end != '\0'; ++end);

I wouldn't change this to keep compatibility with the existing syntax.

> 
>  	code = v4l2_subdev_string_to_pixelcode(p, end - p);
>  	if (code == (enum v4l2_mbus_pixelcode)-1)
>  		return -EINVAL;
> 
> -	for (p = end; isspace(*p); ++p);
> +	p = end + 1;
>  	width = strtoul(p, &end, 10);
>  	if (*end != 'x')
>  		return -EINVAL;
> @@ -256,32 +256,32 @@ static int v4l2_subdev_parse_format(struct
> v4l2_mbus_framefmt *format, return 0;
>  }
> 
> -static int v4l2_subdev_parse_crop(
> -	struct v4l2_rect *crop, const char *p, char **endp)
> +static int v4l2_subdev_parse_rectangle(
> +	struct v4l2_rect *r, const char *p, char **endp)
>  {
>  	char *end;
> 
>  	if (*p++ != '(')
>  		return -EINVAL;
> 
> -	crop->left = strtoul(p, &end, 10);
> +	r->left = strtoul(p, &end, 10);
>  	if (*end != ',')
>  		return -EINVAL;
> 
>  	p = end + 1;
> -	crop->top = strtoul(p, &end, 10);
> +	r->top = strtoul(p, &end, 10);
>  	if (*end++ != ')')
>  		return -EINVAL;
>  	if (*end != '/')
>  		return -EINVAL;
> 
>  	p = end + 1;
> -	crop->width = strtoul(p, &end, 10);
> +	r->width = strtoul(p, &end, 10);
>  	if (*end != 'x')
>  		return -EINVAL;
> 
>  	p = end + 1;
> -	crop->height = strtoul(p, &end, 10);
> +	r->height = strtoul(p, &end, 10);
>  	*endp = end;
> 
>  	return 0;
> @@ -307,6 +307,17 @@ static int v4l2_subdev_parse_frame_interval(struct
> v4l2_fract *interval, return 0;
>  }
> 
> +static int strhazit(const char *str, const char **p)

I would make the function return a bool, or, if you want to keep the int 
return type, return 0 when there's no match and 1 when there's a match. I 
suppose you prefer keeping strhazit over strmatch ? :-)

> +{
> +	int len = strlen(str);

strlen() returns a size_t.

> +
> +	if (strncmp(str, *p, len))
> +		return -ENOENT;
> +
> +	*p += len;

What about also skipping white spaces here ?

> +	return 0;
> +}
> +
>  static struct media_pad *v4l2_subdev_parse_pad_format(
>  	struct media_device *media, struct v4l2_mbus_framefmt *format,
>  	struct v4l2_rect *crop, struct v4l2_fract *interval, const char *p,
> @@ -326,30 +337,37 @@ static struct media_pad *v4l2_subdev_parse_pad_format(
> if (*p++ != '[')
>  		return NULL;
> 
> -	for (; isspace(*p); ++p);
> +	for (;;) {
> +		for (; isspace(*p); p++);
> 
> -	if (isalnum(*p)) {
> -		ret = v4l2_subdev_parse_format(format, p, &end);
> -		if (ret < 0)
> -			return NULL;
> +		if (!strhazit("fmt:", &p)) {
> +			ret = v4l2_subdev_parse_format(format, p, &end);
> +			if (ret < 0)
> +				return NULL;
> 
> -		for (p = end; isspace(*p); p++);
> -	}
> +			p = end;
> +			continue;
> +		}

I'd like to keep compatibility with the existing syntax here. Checking whether 
this is the first argument and whether it starts with an uppercase letter 
should be enough, would you be OK with that ?

> 
> -	if (*p == '(') {
> -		ret = v4l2_subdev_parse_crop(crop, p, &end);
> -		if (ret < 0)
> -			return NULL;
> +		if (!strhazit("crop:", &p) || *p == '(') {
> +			ret = v4l2_subdev_parse_rectangle(crop, p, &end);
> +			if (ret < 0)
> +				return NULL;
> 
> -		for (p = end; isspace(*p); p++);
> -	}
> +			p = end;
> +			continue;
> +		}
> 
> -	if (*p == '@') {
> -		ret = v4l2_subdev_parse_frame_interval(interval, ++p, &end);
> -		if (ret < 0)
> -			return NULL;
> +		if (*p == '@') {
> +			ret = v4l2_subdev_parse_frame_interval(interval, ++p, &end);
> +			if (ret < 0)
> +				return NULL;
> +
> +			p = end;
> +			continue;
> +		}
> 
> -		for (p = end; isspace(*p); p++);
> +		break;
>  	}
> 
>  	if (*p != ']')

-- 
Regards,

Laurent Pinchart

Re: [media-ctl PATCH v2 2/2] Compose rectangle support for libv4l2subdev

[Laurent Pinchart]

Hi Sakari,

Thanks for the patch.

On Monday 07 May 2012 16:46:36 Sakari Ailus wrote:
> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>

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

> ---
>  src/main.c       |   14 ++++++++++++++
>  src/options.c    |    6 ++++--
>  src/v4l2subdev.c |   32 +++++++++++++++++++++++---------
>  3 files changed, 41 insertions(+), 11 deletions(-)
> 
> diff --git a/src/main.c b/src/main.c
> index 2f57352..a989669 100644
> --- a/src/main.c
> +++ b/src/main.c
> @@ -77,6 +77,20 @@ static void v4l2_subdev_print_format(struct media_entity
> *entity, printf("\n\t\t crop:%u,%u/%ux%u", rect.left, rect.top,
>  		       rect.width, rect.height);
> 
> +	ret = v4l2_subdev_get_selection(entity, &rect, pad,
> +					V4L2_SUBDEV_SEL_TGT_COMPOSE_BOUNDS,
> +					which);
> +	if (ret == 0)
> +		printf("\n\t\t compose.bounds:%u,%u/%ux%u",
> +		       rect.left, rect.top, rect.width, rect.height);
> +
> +	ret = v4l2_subdev_get_selection(entity, &rect, pad,
> +					V4L2_SUBDEV_SEL_TGT_COMPOSE_ACTUAL,
> +					which);
> +	if (ret == 0)
> +		printf("\n\t\t compose:%u,%u/%ux%u",
> +		       rect.left, rect.top, rect.width, rect.height);
> +
>  	printf("]");
>  }
> 
> diff --git a/src/options.c b/src/options.c
> index 46f6bef..8e80bd0 100644
> --- a/src/options.c
> +++ b/src/options.c
> @@ -56,12 +56,14 @@ static void usage(const char *argv0, int verbose)
>  	printf("\tv4l2                = pad, '[', v4l2-cfgs ']' ;\n");
>  	printf("\tv4l2-cfgs           = v4l2-cfg [ ',' v4l2-cfg ] ;\n");
>  	printf("\tv4l2-cfg            = v4l2-mbusfmt | v4l2-crop\n");
> -	printf("\t                      | v4l2 frame interval ;\n");
> +	printf("\t                      | v4l2-compose | v4l2 frame interval
> ;\n"); printf("\tv4l2-mbusfmt        = 'fmt:', fcc, '/', size ;\n");
>  	printf("\tpad                 = entity, ':', pad number ;\n");
>  	printf("\tentity              = entity number | ( '\"', entity name, '\"'
> ) ;\n"); printf("\tsize                = width, 'x', height ;\n");
> -	printf("\tv4l2-crop           = 'crop:(', left, ',', top, ')/', size
> ;\n"); +	printf("\tv4l2-crop           = 'crop:', v4l2-rectangle ;\n");
> +	printf("\tv4l2-compose        = 'compose:', v4l2-rectangle ;\n");
> +	printf("\tv4l2-rectangle      = '(', left, ',', top, ')/', size ;\n");
>  	printf("\tv4l2 frame interval = '@', numerator, '/', denominator ;\n");
>  	printf("where the fields are\n");
>  	printf("\tentity number   Entity numeric identifier\n");
> diff --git a/src/v4l2subdev.c b/src/v4l2subdev.c
> index 6881553..0abb4f4 100644
> --- a/src/v4l2subdev.c
> +++ b/src/v4l2subdev.c
> @@ -320,8 +320,8 @@ static int strhazit(const char *str, const char **p)
> 
>  static struct media_pad *v4l2_subdev_parse_pad_format(
>  	struct media_device *media, struct v4l2_mbus_framefmt *format,
> -	struct v4l2_rect *crop, struct v4l2_fract *interval, const char *p,
> -	char **endp)
> +	struct v4l2_rect *crop, struct v4l2_rect *compose,
> +	struct v4l2_fract *interval, const char *p, char **endp)
>  {
>  	struct media_pad *pad;
>  	char *end;
> @@ -358,6 +358,15 @@ static struct media_pad *v4l2_subdev_parse_pad_format(
>  			continue;
>  		}
> 
> +		if (!strhazit("compose:", &p)) {
> +			ret = v4l2_subdev_parse_rectangle(compose, p, &end);
> +			if (ret < 0)
> +				return NULL;
> +
> +			for (p = end; isspace(*p); p++);
> +			continue;
> +		}
> +
>  		if (*p == '@') {
>  			ret = v4l2_subdev_parse_frame_interval(interval, ++p, &end);
>  			if (ret < 0)
> @@ -471,30 +480,35 @@ static int v4l2_subdev_parse_setup_format(struct
> media_device *media, struct v4l2_mbus_framefmt format = { 0, 0, 0 };
>  	struct media_pad *pad;
>  	struct v4l2_rect crop = { -1, -1, -1, -1 };
> +	struct v4l2_rect compose = crop;
>  	struct v4l2_fract interval = { 0, 0 };
>  	unsigned int i;
>  	char *end;
>  	int ret;
> 
> -	pad = v4l2_subdev_parse_pad_format(media, &format, &crop, &interval,
> -					   p, &end);
> +	pad = v4l2_subdev_parse_pad_format(media, &format, &crop, &compose,
> +					   &interval, p, &end);
>  	if (pad == NULL) {
>  		media_dbg(media, "Unable to parse format\n");
>  		return -EINVAL;
>  	}
> 
> -	if (pad->flags & MEDIA_PAD_FL_SOURCE) {
> -		ret = set_selection(pad, V4L2_SUBDEV_SEL_TGT_CROP_ACTUAL, &crop);
> +	if (pad->flags & MEDIA_PAD_FL_SINK) {
> +		ret = set_format(pad, &format);
>  		if (ret < 0)
>  			return ret;
>  	}
> 
> -	ret = set_format(pad, &format);
> +	ret = set_selection(pad, V4L2_SUBDEV_SEL_TGT_CROP_ACTUAL, &crop);
>  	if (ret < 0)
>  		return ret;
> 
> -	if (pad->flags & MEDIA_PAD_FL_SINK) {
> -		ret = set_selection(pad, V4L2_SUBDEV_SEL_TGT_CROP_ACTUAL, &crop);
> +	ret = set_selection(pad, V4L2_SUBDEV_SEL_TGT_COMPOSE_ACTUAL, &compose);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (pad->flags & MEDIA_PAD_FL_SOURCE) {
> +		ret = set_format(pad, &format);
>  		if (ret < 0)
>  			return ret;
>  	}
-- 
Regards,

Laurent Pinchart

Re: [media-ctl PATCH v2 1/2] New, more flexible syntax for format

[Sakari Ailus]

Hi Laurent,

Laurent Pinchart wrote:
> Hi Sakari,
> 
> Thanks for the patch.
> 
> On Monday 07 May 2012 16:46:35 Sakari Ailus wrote:
>> More flexible and extensible syntax for format which allows better usage
>> of the selection API.
>>
>> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
>> ---
>>  src/main.c       |   17 ++++++++++---
>>  src/options.c    |   27 +++++++++++++-------
>>  src/v4l2subdev.c |   70 ++++++++++++++++++++++++++++++++-------------------
>>  3 files changed, 74 insertions(+), 40 deletions(-)
> 
> [snip]
> 
>> diff --git a/src/options.c b/src/options.c
>> index 60cf6d5..46f6bef 100644
>> --- a/src/options.c
>> +++ b/src/options.c
>> @@ -37,8 +37,8 @@ static void usage(const char *argv0, int verbose)
>>  	printf("%s [options] device\n", argv0);
>>  	printf("-d, --device dev	Media device name (default: %s)\n",
>> MEDIA_DEVNAME_DEFAULT);
>> 	printf("-e, --entity name	Print the device name associated with the
>> given entity\n");
>> -	printf("-f, --set-format	Comma-separated list of formats to
>> setup\n");
>> -	printf("   --get-format pad	Print the active format on a given pad\n");
>> +	printf("-V, --set-v4l2 v4l2	Comma-separated list of formats to setup\n");
>> +	printf("   --get-v4l2 pad	Print the active format on a given pad\n");
> 
> I still need to think about the name change.
> 
>>  	printf("-h, --help		Show verbose help and exit\n");
>>  	printf("-i, --interactive	Modify links interactively\n");
>>  	printf("-l, --links		Comma-separated list of links descriptors to
>> setup\n");
>> @@ -52,13 +52,17 @@ static void usage(const char *argv0, int verbose)
>>
>>  	printf("\n");
>>  	printf("Links and formats are defined as\n");
>> -	printf("\tlink            = pad, '->', pad, '[', flags, ']' ;\n");
>> -	printf("\tformat          = pad, '[', fcc, ' ', size, [ ' ', crop ], [ '
>> ', '@', frame interval ], ']' ;\n"); -	printf("\tpad             = entity,
>> ':', pad number ;\n");
>> -	printf("\tentity          = entity number | ( '\"', entity name, '\"' )
>> ;\n"); -	printf("\tsize            = width, 'x', height ;\n");
>> -	printf("\tcrop            = '(', left, ',', top, ')', '/', size ;\n");
>> -	printf("\tframe interval  = numerator, '/', denominator ;\n");
>> +	printf("\tlink                = pad, '->', pad, '[', flags, ']' ;\n");
>> +	printf("\tv4l2                = pad, '[', v4l2-cfgs ']' ;\n");
>> +	printf("\tv4l2-cfgs           = v4l2-cfg [ ',' v4l2-cfg ] ;\n");
>> +	printf("\tv4l2-cfg            = v4l2-mbusfmt | v4l2-crop\n");
>> +	printf("\t                      | v4l2 frame interval ;\n");
>> +	printf("\tv4l2-mbusfmt        = 'fmt:', fcc, '/', size ;\n");
>> +	printf("\tpad                 = entity, ':', pad number ;\n");
>> +	printf("\tentity              = entity number | ( '\"', entity name, '\"'
>> ) ;\n"); +	printf("\tsize                = width, 'x', height ;\n");
>> +	printf("\tv4l2-crop           = 'crop:(', left, ',', top, ')/', size
>> ;\n"); +	printf("\tv4l2 frame interval = '@', numerator, '/', denominator
>> ;\n"); printf("where the fields are\n");
>>  	printf("\tentity number   Entity numeric identifier\n");
>>  	printf("\tentity name     Entity name (string) \n");
>> @@ -77,7 +81,9 @@ static void usage(const char *argv0, int verbose)
>>  static struct option opts[] = {
>>  	{"device", 1, 0, 'd'},
>>  	{"entity", 1, 0, 'e'},
>> +	{"set-v4l2", 1, 0, 'V'},
>>  	{"set-format", 1, 0, 'f'},
>> +	{"get-v4l2", 1, 0, OPT_GET_FORMAT},
>>  	{"get-format", 1, 0, OPT_GET_FORMAT},
>>  	{"help", 0, 0, 'h'},
>>  	{"interactive", 0, 0, 'i'},
>> @@ -98,7 +104,7 @@ int parse_cmdline(int argc, char **argv)
>>  	}
>>
>>  	/* parse options */
>> -	while ((opt = getopt_long(argc, argv, "d:e:f:hil:prv", opts, NULL)) != 
> -1)
>> { +	while ((opt = getopt_long(argc, argv, "d:e:V:f:hil:prv", opts, NULL))
>> != -1) { switch (opt) {
>>  		case 'd':
>>  			media_opts.devname = optarg;
>> @@ -108,6 +114,7 @@ int parse_cmdline(int argc, char **argv)
>>  			media_opts.entity = optarg;
>>  			break;
>>
>> +		case 'V':
>>  		case 'f':
>>  			media_opts.formats = optarg;
>>  			break;
>> diff --git a/src/v4l2subdev.c b/src/v4l2subdev.c
>> index a2ab0c4..6881553 100644
>> --- a/src/v4l2subdev.c
>> +++ b/src/v4l2subdev.c
>> @@ -233,13 +233,13 @@ static int v4l2_subdev_parse_format(struct
>> v4l2_mbus_framefmt *format, char *end;
>>
>>  	for (; isspace(*p); ++p);
>> -	for (end = (char *)p; !isspace(*end) && *end != '\0'; ++end);
>> +	for (end = (char *)p; *end != '/' && *end != '\0'; ++end);
> 
> I wouldn't change this to keep compatibility with the existing syntax.

Ok. How about allowing both '/' and ' '?

>>
>>  	code = v4l2_subdev_string_to_pixelcode(p, end - p);
>>  	if (code == (enum v4l2_mbus_pixelcode)-1)
>>  		return -EINVAL;
>>
>> -	for (p = end; isspace(*p); ++p);
>> +	p = end + 1;
>>  	width = strtoul(p, &end, 10);
>>  	if (*end != 'x')
>>  		return -EINVAL;
>> @@ -256,32 +256,32 @@ static int v4l2_subdev_parse_format(struct
>> v4l2_mbus_framefmt *format, return 0;
>>  }
>>
>> -static int v4l2_subdev_parse_crop(
>> -	struct v4l2_rect *crop, const char *p, char **endp)
>> +static int v4l2_subdev_parse_rectangle(
>> +	struct v4l2_rect *r, const char *p, char **endp)
>>  {
>>  	char *end;
>>
>>  	if (*p++ != '(')
>>  		return -EINVAL;
>>
>> -	crop->left = strtoul(p, &end, 10);
>> +	r->left = strtoul(p, &end, 10);
>>  	if (*end != ',')
>>  		return -EINVAL;
>>
>>  	p = end + 1;
>> -	crop->top = strtoul(p, &end, 10);
>> +	r->top = strtoul(p, &end, 10);
>>  	if (*end++ != ')')
>>  		return -EINVAL;
>>  	if (*end != '/')
>>  		return -EINVAL;
>>
>>  	p = end + 1;
>> -	crop->width = strtoul(p, &end, 10);
>> +	r->width = strtoul(p, &end, 10);
>>  	if (*end != 'x')
>>  		return -EINVAL;
>>
>>  	p = end + 1;
>> -	crop->height = strtoul(p, &end, 10);
>> +	r->height = strtoul(p, &end, 10);
>>  	*endp = end;
>>
>>  	return 0;
>> @@ -307,6 +307,17 @@ static int v4l2_subdev_parse_frame_interval(struct
>> v4l2_fract *interval, return 0;
>>  }
>>
>> +static int strhazit(const char *str, const char **p)
> 
> I would make the function return a bool, or, if you want to keep the int 
> return type, return 0 when there's no match and 1 when there's a match. I 
> suppose you prefer keeping strhazit over strmatch ? :-)

Bool is fine.

>> +{
>> +	int len = strlen(str);
> 
> strlen() returns a size_t.

Ok.

>> +
>> +	if (strncmp(str, *p, len))
>> +		return -ENOENT;
>> +
>> +	*p += len;
> 
> What about also skipping white spaces here ?

Fine with me.

>> +	return 0;
>> +}
>> +
>>  static struct media_pad *v4l2_subdev_parse_pad_format(
>>  	struct media_device *media, struct v4l2_mbus_framefmt *format,
>>  	struct v4l2_rect *crop, struct v4l2_fract *interval, const char *p,
>> @@ -326,30 +337,37 @@ static struct media_pad *v4l2_subdev_parse_pad_format(
>> if (*p++ != '[')
>>  		return NULL;
>>
>> -	for (; isspace(*p); ++p);
>> +	for (;;) {
>> +		for (; isspace(*p); p++);
>>
>> -	if (isalnum(*p)) {
>> -		ret = v4l2_subdev_parse_format(format, p, &end);
>> -		if (ret < 0)
>> -			return NULL;
>> +		if (!strhazit("fmt:", &p)) {
>> +			ret = v4l2_subdev_parse_format(format, p, &end);
>> +			if (ret < 0)
>> +				return NULL;
>>
>> -		for (p = end; isspace(*p); p++);
>> -	}
>> +			p = end;
>> +			continue;
>> +		}
> 
> I'd like to keep compatibility with the existing syntax here. Checking whether 
> this is the first argument and whether it starts with an uppercase letter 
> should be enough, would you be OK with that ?

Right. I may have missed something related to keeping the compatibility.

Capital letter might not be enough in the future; for now it's ok
though. How about this: if the string doesn't match with anything else,
interpret it as format?

Cheers,

-- 
Sakari Ailus
sakari.ailus@iki.fi

Re: [media-ctl PATCH v2 1/2] New, more flexible syntax for format

[Laurent Pinchart]

Hi Sakari,

On Tuesday 08 May 2012 22:43:50 Sakari Ailus wrote:
> Laurent Pinchart wrote:
> > On Monday 07 May 2012 16:46:35 Sakari Ailus wrote:
> >> More flexible and extensible syntax for format which allows better usage
> >> of the selection API.

[snip]

> >> diff --git a/src/v4l2subdev.c b/src/v4l2subdev.c
> >> index a2ab0c4..6881553 100644
> >> --- a/src/v4l2subdev.c
> >> +++ b/src/v4l2subdev.c
> >> @@ -233,13 +233,13 @@ static int v4l2_subdev_parse_format(struct
> >> v4l2_mbus_framefmt *format, char *end;
> >> 
> >>  	for (; isspace(*p); ++p);
> >> 
> >> -	for (end = (char *)p; !isspace(*end) && *end != '\0'; ++end);
> >> +	for (end = (char *)p; *end != '/' && *end != '\0'; ++end);
> > 
> > I wouldn't change this to keep compatibility with the existing syntax.
> 
> Ok. How about allowing both '/' and ' '?

Do you hate the space that much ? :-) The format code and the resolution are 
not that closely related, / somehow doesn't look intuitive to me.

> >>  	code = v4l2_subdev_string_to_pixelcode(p, end - p);
> >>  	if (code == (enum v4l2_mbus_pixelcode)-1)
> >>  	
> >>  		return -EINVAL;
> >> 
> >> -	for (p = end; isspace(*p); ++p);
> >> +	p = end + 1;
> >> 
> >>  	width = strtoul(p, &end, 10);
> >>  	if (*end != 'x')
> >>  	
> >>  		return -EINVAL;
> >> 

[snip]

> >> @@ -326,30 +337,37 @@ static struct media_pad
> >> *v4l2_subdev_parse_pad_format( if (*p++ != '[')
> >> 
> >>  		return NULL;
> >> 
> >> -	for (; isspace(*p); ++p);
> >> +	for (;;) {
> >> +		for (; isspace(*p); p++);
> >> 
> >> -	if (isalnum(*p)) {
> >> -		ret = v4l2_subdev_parse_format(format, p, &end);
> >> -		if (ret < 0)
> >> -			return NULL;
> >> +		if (!strhazit("fmt:", &p)) {
> >> +			ret = v4l2_subdev_parse_format(format, p, &end);
> >> +			if (ret < 0)
> >> +				return NULL;
> >> 
> >> -		for (p = end; isspace(*p); p++);
> >> -	}
> >> +			p = end;
> >> +			continue;
> >> +		}
> > 
> > I'd like to keep compatibility with the existing syntax here. Checking
> > whether this is the first argument and whether it starts with an
> > uppercase letter should be enough, would you be OK with that ?
> 
> Right. I may have missed something related to keeping the compatibility.
> 
> Capital letter might not be enough in the future; for now it's ok
> though. How about this: if the string doesn't match with anything else,
> interpret it as format?

I've thought about this, but I'm not sure it's a good idea to introduce 
extensions to the existing syntax (we currently have no format starting with 
something else than an uppercase letter) as we're deprecating it.

-- 
Regards,

Laurent Pinchart

Re: [media-ctl PATCH v2 1/2] New, more flexible syntax for format

[Sakari Ailus]

Hi Laurent,

Laurent Pinchart wrote:
> Hi Sakari,
> 
> On Tuesday 08 May 2012 22:43:50 Sakari Ailus wrote:
>> Laurent Pinchart wrote:
>>> On Monday 07 May 2012 16:46:35 Sakari Ailus wrote:
>>>> More flexible and extensible syntax for format which allows better usage
>>>> of the selection API.
> 
> [snip]
> 
>>>> diff --git a/src/v4l2subdev.c b/src/v4l2subdev.c
>>>> index a2ab0c4..6881553 100644
>>>> --- a/src/v4l2subdev.c
>>>> +++ b/src/v4l2subdev.c
>>>> @@ -233,13 +233,13 @@ static int v4l2_subdev_parse_format(struct
>>>> v4l2_mbus_framefmt *format, char *end;
>>>>
>>>>  	for (; isspace(*p); ++p);
>>>>
>>>> -	for (end = (char *)p; !isspace(*end) && *end != '\0'; ++end);
>>>> +	for (end = (char *)p; *end != '/' && *end != '\0'; ++end);
>>>
>>> I wouldn't change this to keep compatibility with the existing syntax.
>>
>> Ok. How about allowing both '/' and ' '?
> 
> Do you hate the space that much ? :-) The format code and the resolution are 
> not that closely related, / somehow doesn't look intuitive to me.

I don't hate the space --- what I would prefer is to keep distinct sets
of configurations separated by something, and space is good for that. If
it was possible to choose pixel format without having to specify width
and height, I'd probably vote for the space.

That makes parsing easier, too, not that it would matter that much though.

>>>>  	code = v4l2_subdev_string_to_pixelcode(p, end - p);
>>>>  	if (code == (enum v4l2_mbus_pixelcode)-1)
>>>>  	
>>>>  		return -EINVAL;
>>>>
>>>> -	for (p = end; isspace(*p); ++p);
>>>> +	p = end + 1;
>>>>
>>>>  	width = strtoul(p, &end, 10);
>>>>  	if (*end != 'x')
>>>>  	
>>>>  		return -EINVAL;
>>>>
> 
> [snip]
> 
>>>> @@ -326,30 +337,37 @@ static struct media_pad
>>>> *v4l2_subdev_parse_pad_format( if (*p++ != '[')
>>>>
>>>>  		return NULL;
>>>>
>>>> -	for (; isspace(*p); ++p);
>>>> +	for (;;) {
>>>> +		for (; isspace(*p); p++);
>>>>
>>>> -	if (isalnum(*p)) {
>>>> -		ret = v4l2_subdev_parse_format(format, p, &end);
>>>> -		if (ret < 0)
>>>> -			return NULL;
>>>> +		if (!strhazit("fmt:", &p)) {
>>>> +			ret = v4l2_subdev_parse_format(format, p, &end);
>>>> +			if (ret < 0)
>>>> +				return NULL;
>>>>
>>>> -		for (p = end; isspace(*p); p++);
>>>> -	}
>>>> +			p = end;
>>>> +			continue;
>>>> +		}
>>>
>>> I'd like to keep compatibility with the existing syntax here. Checking
>>> whether this is the first argument and whether it starts with an
>>> uppercase letter should be enough, would you be OK with that ?
>>
>> Right. I may have missed something related to keeping the compatibility.
>>
>> Capital letter might not be enough in the future; for now it's ok
>> though. How about this: if the string doesn't match with anything else,
>> interpret it as format?
> 
> I've thought about this, but I'm not sure it's a good idea to introduce 
> extensions to the existing syntax (we currently have no format starting with 
> something else than an uppercase letter) as we're deprecating it.

I'm fine with considering capital letters to begin format.

I also thing we should make parsing to return just one configuration at
a time rather than parse everything and return a bunch of structs that
may or may not have been set according to the string given by the user.

But that's for later definitely.

Cheers,

-- 
Sakari Ailus
sakari.ailus@iki.fi

Re: [media-ctl PATCH v2 2/2] Compose rectangle support for libv4l2subdev

[Sakari Ailus]

Laurent Pinchart wrote:
> Hi Sakari,
> 
> Thanks for the patch.
> 
> On Monday 07 May 2012 16:46:36 Sakari Ailus wrote:
>> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> 
> Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Thanks!

-- 
Sakari Ailus
sakari.ailus@iki.fi