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@xxxxxx>
>> ---
>> 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@xxxxxx
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
