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

[Sakari Ailus <sakari.ailus@iki.fi>]

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