[media-ctl PATCH 1/3] Support selections API for crop

[media-ctl PATCH 1/3] Support selections API for crop

[Sakari Ailus]

Support the new selections API for crop. Fall back to use the old crop API
in case the selection API isn't available.

Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
---
 src/main.c       |    4 ++-
 src/v4l2subdev.c |  100 +++++++++++++++++++++++++++++++++++++-----------------
 src/v4l2subdev.h |   37 +++++++++++---------
 3 files changed, 93 insertions(+), 48 deletions(-)

diff --git a/src/main.c b/src/main.c
index 4f3271c..53964e4 100644
--- a/src/main.c
+++ b/src/main.c
@@ -62,7 +62,9 @@ static void v4l2_subdev_print_format(struct media_entity *entity,
 	printf("[%s %ux%u", v4l2_subdev_pixelcode_to_string(format.code),
 	       format.width, format.height);
 
-	ret = v4l2_subdev_get_crop(entity, &rect, pad, which);
+	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,
 		       rect.width, rect.height);
diff --git a/src/v4l2subdev.c b/src/v4l2subdev.c
index b886b72..92360bb 100644
--- a/src/v4l2subdev.c
+++ b/src/v4l2subdev.c
@@ -104,48 +104,85 @@ int v4l2_subdev_set_format(struct media_entity *entity,
 	return 0;
 }
 
-int v4l2_subdev_get_crop(struct media_entity *entity, struct v4l2_rect *rect,
-			 unsigned int pad, enum v4l2_subdev_format_whence which)
+int v4l2_subdev_get_selection(
+	struct media_entity *entity, struct v4l2_rect *r,
+	unsigned int pad, int target, enum v4l2_subdev_format_whence which)
 {
-	struct v4l2_subdev_crop crop;
+	union {
+		struct v4l2_subdev_selection sel;
+		struct v4l2_subdev_crop crop;
+	} u;
 	int ret;
 
 	ret = v4l2_subdev_open(entity);
 	if (ret < 0)
 		return ret;
 
-	memset(&crop, 0, sizeof(crop));
-	crop.pad = pad;
-	crop.which = which;
+	memset(&u.sel, 0, sizeof(u.sel));
+	u.sel.pad = pad;
+	u.sel.target = target;
+	u.sel.which = which;
 
-	ret = ioctl(entity->fd, VIDIOC_SUBDEV_G_CROP, &crop);
+	ret = ioctl(entity->fd, VIDIOC_SUBDEV_G_SELECTION, &u.sel);
+ 	if (ret >= 0) {
+		*r = u.sel.r;
+		return 0;
+	}
+	if (errno != ENOTTY
+	    || target != V4L2_SUBDEV_SEL_TGT_CROP_ACTUAL)
+ 		return -errno;
+
+	memset(&u.crop, 0, sizeof(u.crop));
+	u.crop.pad = pad;
+	u.crop.which = which;
+
+	ret = ioctl(entity->fd, VIDIOC_SUBDEV_G_CROP, &u.crop);
 	if (ret < 0)
 		return -errno;
 
-	*rect = crop.rect;
+	*r = u.crop.rect;
 	return 0;
 }
 
-int v4l2_subdev_set_crop(struct media_entity *entity, struct v4l2_rect *rect,
-			 unsigned int pad, enum v4l2_subdev_format_whence which)
+int v4l2_subdev_set_selection(
+	struct media_entity *entity, struct v4l2_rect *r,
+	unsigned int pad, int target, enum v4l2_subdev_format_whence which)
 {
-	struct v4l2_subdev_crop crop;
+	union {
+		struct v4l2_subdev_selection sel;
+		struct v4l2_subdev_crop crop;
+	} u;
 	int ret;
 
 	ret = v4l2_subdev_open(entity);
 	if (ret < 0)
 		return ret;
 
-	memset(&crop, 0, sizeof(crop));
-	crop.pad = pad;
-	crop.which = which;
-	crop.rect = *rect;
+	memset(&u.sel, 0, sizeof(u.sel));
+	u.sel.pad = pad;
+	u.sel.target = target;
+	u.sel.which = which;
+	u.sel.r = *r;
+
+	ret = ioctl(entity->fd, VIDIOC_SUBDEV_S_SELECTION, &u.sel);
+ 	if (ret >= 0) {
+		*r = u.sel.r;
+		return 0;
+	}
+	if (errno != ENOTTY
+	    || target != V4L2_SUBDEV_SEL_TGT_CROP_ACTUAL)
+ 		return -errno;
+
+	memset(&u.crop, 0, sizeof(u.crop));
+	u.crop.pad = pad;
+	u.crop.which = which;
+	u.crop.rect = *r;
 
-	ret = ioctl(entity->fd, VIDIOC_SUBDEV_S_CROP, &crop);
+	ret = ioctl(entity->fd, VIDIOC_SUBDEV_S_CROP, &u.crop);
 	if (ret < 0)
 		return -errno;
 
-	*rect = crop.rect;
+	*r = u.crop.rect;
 	return 0;
 }
 
@@ -355,30 +392,31 @@ static int set_format(struct media_pad *pad,
 	return 0;
 }
 
-static int set_crop(struct media_pad *pad, struct v4l2_rect *crop)
+static int set_selection(struct media_pad *pad, int tgt,
+			 struct v4l2_rect *rect)
 {
 	int ret;
 
-	if (crop->left == -1 || crop->top == -1)
+	if (rect->left == -1 || rect->top == -1)
 		return 0;
 
 	media_dbg(pad->entity->media,
-		  "Setting up crop rectangle (%u,%u)/%ux%u on pad %s/%u\n",
-		  crop->left, crop->top, crop->width, crop->height,
+		  "Setting up selection target %d rectangle (%u,%u)/%ux%u on pad %s/%u\n",
+		  tgt, rect->left, rect->top, rect->width, rect->height,
 		  pad->entity->info.name, pad->index);
 
-	ret = v4l2_subdev_set_crop(pad->entity, crop, pad->index,
-				   V4L2_SUBDEV_FORMAT_ACTIVE);
+	ret = v4l2_subdev_set_selection(pad->entity, rect, pad->index,
+					tgt, V4L2_SUBDEV_FORMAT_ACTIVE);
 	if (ret < 0) {
 		media_dbg(pad->entity->media,
-			  "Unable to set crop rectangle: %s (%d)\n",
+			  "Unable to set selection rectangle: %s (%d)\n",
 			  strerror(-ret), ret);
 		return ret;
 	}
 
 	media_dbg(pad->entity->media,
-		  "Crop rectangle set: (%u,%u)/%ux%u\n",
-		  crop->left, crop->top, crop->width, crop->height);
+		  "Selection rectangle set: (%u,%u)/%ux%u\n",
+		  rect->left, rect->top, rect->width, rect->height);
 
 	return 0;
 }
@@ -429,18 +467,18 @@ static int v4l2_subdev_parse_setup_format(struct media_device *media,
 		return -EINVAL;
 	}
 
-	if (pad->flags & MEDIA_PAD_FL_SOURCE) {
-		ret = set_crop(pad, &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_crop(pad, &crop);
+	if (pad->flags & MEDIA_PAD_FL_SOURCE) {
+		ret = set_format(pad, &format);
 		if (ret < 0)
 			return ret;
 	}
diff --git a/src/v4l2subdev.h b/src/v4l2subdev.h
index 1e75f94..1020747 100644
--- a/src/v4l2subdev.h
+++ b/src/v4l2subdev.h
@@ -88,34 +88,38 @@ int v4l2_subdev_set_format(struct media_entity *entity,
 	enum v4l2_subdev_format_whence which);
 
 /**
- * @brief Retrieve the crop rectangle on a pad.
+ * @brief Retrieve a selection rectangle on a pad.
  * @param entity - subdev-device media entity.
- * @param rect - crop rectangle to be filled.
+ * @param r - rectangle to be filled.
  * @param pad - pad number.
+ * @param target - selection target
  * @param which - identifier of the format to get.
  *
- * Retrieve the current crop rectangleon the @a entity @a pad and store it in
- * the @a rect structure.
+ * Retrieve the @a target selection rectangle on the @a entity @a pad
+ * and store it in the @a rect structure.
  *
- * @a which is set to V4L2_SUBDEV_FORMAT_TRY to retrieve the try crop rectangle
- * stored in the file handle, of V4L2_SUBDEV_FORMAT_ACTIVE to retrieve the
- * current active crop rectangle.
+ * @a which is set to V4L2_SUBDEV_FORMAT_TRY to retrieve the try
+ * selection rectangle stored in the file handle, of
+ * V4L2_SUBDEV_FORMAT_ACTIVE to retrieve the current active selection
+ * rectangle.
  *
  * @return 0 on success, or a negative error code on failure.
  */
-int v4l2_subdev_get_crop(struct media_entity *entity, struct v4l2_rect *rect,
-	unsigned int pad, enum v4l2_subdev_format_whence which);
+int v4l2_subdev_get_selection(
+	struct media_entity *entity, struct v4l2_rect *r,
+	unsigned int pad, int target, enum v4l2_subdev_format_whence which);
 
 /**
- * @brief Set the crop rectangle on a pad.
+ * @brief Set a selection rectangle on a pad.
  * @param entity - subdev-device media entity.
- * @param rect - crop rectangle.
+ * @param r - crop rectangle.
  * @param pad - pad number.
+ * @param target - selection target
  * @param which - identifier of the format to set.
  *
- * Set the crop rectangle on the @a entity @a pad to @a rect. The driver is
- * allowed to modify the requested rectangle, in which case @a rect is updated
- * with the modifications.
+ * Set the @a target selection rectangle on the @a entity @a pad to @a
+ * rect. The driver is allowed to modify the requested rectangle, in
+ * which case @a rect is updated with the modifications.
  *
  * @a which is set to V4L2_SUBDEV_FORMAT_TRY to set the try crop rectangle
  * stored in the file handle, of V4L2_SUBDEV_FORMAT_ACTIVE to configure the
@@ -123,8 +127,9 @@ int v4l2_subdev_get_crop(struct media_entity *entity, struct v4l2_rect *rect,
  *
  * @return 0 on success, or a negative error code on failure.
  */
-int v4l2_subdev_set_crop(struct media_entity *entity, struct v4l2_rect *rect,
-	unsigned int pad, enum v4l2_subdev_format_whence which);
+int v4l2_subdev_set_selection(
+	struct media_entity *entity, struct v4l2_rect *r,
+	unsigned int pad, int target, enum v4l2_subdev_format_whence which);
 
 /**
  * @brief Retrieve the frame interval on a sub-device.
-- 
1.7.2.5

[media-ctl PATCH 2/3] New, more flexible syntax for media-ctl

[Sakari Ailus]

More flexible and extensible syntax for media-ctl which allows better usage
of the selection API.

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

diff --git a/src/main.c b/src/main.c
index 53964e4..6de1031 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.actual:%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..4d9d48f 100644
--- a/src/options.c
+++ b/src/options.c
@@ -53,12 +53,15 @@ 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("\tformat          = pad, '[', formats ']' ;\n");
+	printf("\tformats         = formats ',' formats ;\n");
+	printf("\tformats         = fmt | crop | frame interval ;\n");
+	printf("\fmt              = 'fmt:', fcc, '/', size ;\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("\tcrop            = 'crop.actual:', left, ',', top, '/', size ;\n");
+	printf("\tframe interval  = '@', numerator, '/', denominator ;\n");
 	printf("where the fields are\n");
 	printf("\tentity number   Entity numeric identifier\n");
 	printf("\tentity name     Entity name (string) \n");
diff --git a/src/v4l2subdev.c b/src/v4l2subdev.c
index 92360bb..87b22fc 100644
--- a/src/v4l2subdev.c
+++ b/src/v4l2subdev.c
@@ -235,13 +235,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;
@@ -258,32 +258,27 @@ 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);
-	if (*end++ != ')')
-		return -EINVAL;
+	r->top = strtoul(p, &end, 10);
 	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;
@@ -309,6 +304,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,
@@ -330,28 +336,35 @@ static struct media_pad *v4l2_subdev_parse_pad_format(
 
 	for (; isspace(*p); ++p);
 
-	if (isalnum(*p)) {
-		ret = v4l2_subdev_parse_format(format, p, &end);
-		if (ret < 0)
-			return NULL;
+	for (;;) {
+		if (!strhazit("fmt:", &p)) {
+			ret = v4l2_subdev_parse_format(format, p, &end);
+			if (ret < 0)
+				return NULL;
 
-		for (p = end; isspace(*p); p++);
-	}
+			for (p = end; isspace(*p); p++);
+			continue;
+		}
 
-	if (*p == '(') {
-		ret = v4l2_subdev_parse_crop(crop, p, &end);
-		if (ret < 0)
-			return NULL;
+		if (!strhazit("crop.actual:", &p)) {
+			ret = v4l2_subdev_parse_rectangle(crop, p, &end);
+			if (ret < 0)
+				return NULL;
 
-		for (p = end; isspace(*p); p++);
-	}
+			for (p = end; isspace(*p); p++);
+			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;
+
+			for (p = end; isspace(*p); p++);
+			continue;
+		}
 
-		for (p = end; isspace(*p); p++);
+		break;
 	}
 
 	if (*p != ']')
-- 
1.7.2.5

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

[Laurent Pinchart]

Hi Sakari,

Thanks for the patch.

On Friday 04 May 2012 11:24:42 Sakari Ailus wrote:
> More flexible and extensible syntax for media-ctl which allows better usage
> of the selection API.

[snip]

> diff --git a/src/options.c b/src/options.c
> index 60cf6d5..4d9d48f 100644
> --- a/src/options.c
> +++ b/src/options.c
> @@ -53,12 +53,15 @@ 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("\tformat          = pad, '[', formats ']' ;\n");
> +	printf("\tformats         = formats ',' formats ;\n");
> +	printf("\tformats         = fmt | crop | frame interval ;\n");

That's not a valid EBNF. I'm not an expert on the subject, but I think the 
following is better.

formats = format { ' ', formats }
format = fmt | crop | frame interval

> +	printf("\fmt              = 'fmt:', fcc, '/', size ;\n");

format, formats and fmt are becoming confusing. A different name for 'formats' 
would be good.

I find the '/' a bit confusing compared to the ' ' (but I think you find the 
space confusing compared to '/' :-)). I also wonder whether we shouldn't just 
drop 'fmt:', as there can be a single format only.

>  	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("\tcrop            = 'crop.actual:', left, ',', top, '/', size
> ;\n");

Would it make sense to make .actual implicit ? Both 'crop' and 'crop.actual' 
would be supported by the parser. The code would be more generic if the target 
was parsed in a generic way, not associated with the rectangle name.

I would keep the parenthesis around the (top,left) coordinates. You could then 
define

rectangle = '(', left, ',', top, ')', '/', size
crop = 'crop' [ '.', target ] ':', rectangle
target = 'actual'

or something similar.

What about also keeping compatibility with the existing syntax (both for 
formats and crop rectangles) ? That shouldn't be too difficult in the parser, 
crop rectangles start with a '(', and formats come first. We would then have

rectangle = '(', left, ',', top, ')', '/', size
crop = [ 'crop' [ '.', target ] ':' ], rectangle
target = 'actual'

> +	printf("\tframe interval  = '@', numerator, '/', denominator ;\n");
>  	printf("where the fields are\n");
>  	printf("\tentity number   Entity numeric identifier\n");
>  	printf("\tentity name     Entity name (string) \n");

-- 
Regards,

Laurent Pinchart

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

[Sakari Ailus]

Hi Laurent,

On Sat, May 05, 2012 at 02:22:26PM +0200, Laurent Pinchart wrote:
> Hi Sakari,
> 
> Thanks for the patch.

Thanks for the review!

> On Friday 04 May 2012 11:24:42 Sakari Ailus wrote:
> > More flexible and extensible syntax for media-ctl which allows better usage
> > of the selection API.
> 
> [snip]
> 
> > diff --git a/src/options.c b/src/options.c
> > index 60cf6d5..4d9d48f 100644
> > --- a/src/options.c
> > +++ b/src/options.c
> > @@ -53,12 +53,15 @@ 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("\tformat          = pad, '[', formats ']' ;\n");
> > +	printf("\tformats         = formats ',' formats ;\n");
> > +	printf("\tformats         = fmt | crop | frame interval ;\n");
> 
> That's not a valid EBNF. I'm not an expert on the subject, but I think the 
> following is better.
> 
> formats = format { ' ', formats }
> format = fmt | crop | frame interval

I'm fine with that change.

> > +	printf("\fmt              = 'fmt:', fcc, '/', size ;\n");
> 
> format, formats and fmt are becoming confusing. A different name for 'formats' 
> would be good.

I agree but I didn't immediately come up with something better.

The pixel format and the image size at the pad are clearly format
(VIDIOC_SUBDEV_S_FMT) but the other things are related to pads but not
format.

I see them different kinds of properties of pads. That suggests we might be
better renaming the option (-f) to something else as well.

> I find the '/' a bit confusing compared to the ' ' (but I think you find the 
> space confusing compared to '/' :-)). I also wonder whether we shouldn't just 
> drop 'fmt:', as there can be a single format only.

You can set it multiple times, or you may not set it at all. That's why I
think we should explicitly say it's the format.

> >  	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("\tcrop            = 'crop.actual:', left, ',', top, '/', size
> > ;\n");
> 
> Would it make sense to make .actual implicit ? Both 'crop' and 'crop.actual' 
> would be supported by the parser. The code would be more generic if the target 
> was parsed in a generic way, not associated with the rectangle name.

I've been also thinking does the actual / active really signify something,
or should that be even dropped form the V4L2 / V4L2 subdev interface
definitions.

> I would keep the parenthesis around the (top,left) coordinates. You could then 
> define
> 
> rectangle = '(', left, ',', top, ')', '/', size
> crop = 'crop' [ '.', target ] ':', rectangle
> target = 'actual'
> 
> or something similar.

Sounds good to me.

> What about also keeping compatibility with the existing syntax (both for 
> formats and crop rectangles) ? That shouldn't be too difficult in the parser, 
> crop rectangles start with a '(', and formats come first. We would then have
> 
> rectangle = '(', left, ',', top, ')', '/', size
> crop = [ 'crop' [ '.', target ] ':' ], rectangle
> target = 'actual'

I'll see what I can do. We still should drop the documentation for this so
that people will stop writing rules that look like that.

Cheers,

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi	jabber/XMPP/Gmail: sailus@retiisi.org.uk

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

[Laurent Pinchart]

Hi Sakari,

On Saturday 05 May 2012 16:09:33 Sakari Ailus wrote:
> On Sat, May 05, 2012 at 02:22:26PM +0200, Laurent Pinchart wrote:
> > On Friday 04 May 2012 11:24:42 Sakari Ailus wrote:
> > > More flexible and extensible syntax for media-ctl which allows better
> > > usage
> > > of the selection API.
> > 
> > [snip]
> > 
> > > diff --git a/src/options.c b/src/options.c
> > > index 60cf6d5..4d9d48f 100644
> > > --- a/src/options.c
> > > +++ b/src/options.c
> > > @@ -53,12 +53,15 @@ 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("\tformat          = pad, '[', formats ']' ;\n");
> > > +	printf("\tformats         = formats ',' formats ;\n");
> > > +	printf("\tformats         = fmt | crop | frame interval ;\n");
> > 
> > That's not a valid EBNF. I'm not an expert on the subject, but I think the
> > following is better.
> > 
> > formats = format { ' ', formats }
> > format = fmt | crop | frame interval
> 
> I'm fine with that change.
> 
> > > +	printf("\fmt              = 'fmt:', fcc, '/', size ;\n");
> > 
> > format, formats and fmt are becoming confusing. A different name for
> > 'formats' would be good.
> 
> I agree but I didn't immediately come up with something better.

I haven't found a better option at first sight either, let's try to sleep on 
it.

> The pixel format and the image size at the pad are clearly format
> (VIDIOC_SUBDEV_S_FMT) but the other things are related to pads but not
> format.
> 
> I see them different kinds of properties of pads. That suggests we might be
> better renaming the option (-f) to something else as well.

You like breaking interfaces, don't you ? :-D

> > I find the '/' a bit confusing compared to the ' ' (but I think you find
> > the space confusing compared to '/' :-)). I also wonder whether we
> > shouldn't just drop 'fmt:', as there can be a single format only.
> 
> You can set it multiple times, or you may not set it at all. That's why I
> think we should explicitly say it's the format.

Not at all makes sense, but why would you set it multiple times ?

> > >  	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("\tcrop            = 'crop.actual:', left, ',', top, '/', size
> > > ;\n");
> > 
> > Would it make sense to make .actual implicit ? Both 'crop' and
> > 'crop.actual' would be supported by the parser. The code would be more
> > generic if the target was parsed in a generic way, not associated with
> > the rectangle name.
> I've been also thinking does the actual / active really signify something,
> or should that be even dropped form the V4L2 / V4L2 subdev interface
> definitions.
> 
> > I would keep the parenthesis around the (top,left) coordinates. You could
> > then define
> > 
> > rectangle = '(', left, ',', top, ')', '/', size
> > crop = 'crop' [ '.', target ] ':', rectangle
> > target = 'actual'
> > 
> > or something similar.
> 
> Sounds good to me.
> 
> > What about also keeping compatibility with the existing syntax (both for
> > formats and crop rectangles) ? That shouldn't be too difficult in the
> > parser, crop rectangles start with a '(', and formats come first. We
> > would then have
> > 
> > rectangle = '(', left, ',', top, ')', '/', size
> > crop = [ 'crop' [ '.', target ] ':' ], rectangle
> > target = 'actual'
> 
> I'll see what I can do. We still should drop the documentation for this so
> that people will stop writing rules that look like that.

Good point, I agree.

-- 
Regards,

Laurent Pinchart

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

[Sakari Ailus]

Hi Laurent,

Laurent Pinchart wrote:
> On Saturday 05 May 2012 16:09:33 Sakari Ailus wrote:
...
>> The pixel format and the image size at the pad are clearly format
>> (VIDIOC_SUBDEV_S_FMT) but the other things are related to pads but not
>> format.
>>
>> I see them different kinds of properties of pads. That suggests we might be
>> better renaming the option (-f) to something else as well.
> 
> You like breaking interfaces, don't you ? :-D

I thought you said we have no stable release yet. :-D

The selection interface on subdevs is currently used to change format
related things (cropping and scaling, for example) but it was one of
Sylwester's patches ("V4L: Add auto focus targets to the selections
API") that adds a focus window target to the V4L2 selection interface. I
don't see why it couldn't be present on subdevs, too. That's got nothing
to do with the image format.

I've been pondering a bit using another option to configure things
related to selections. Conveniently "-s" is free. We could leave the
crop things to -f but remove the documentation related to them.

I'm fine with keeping the things as they are for now, too, but in that
case we should recognise that -f will not be for formats only. Or we
split handling selections into separate options, but I don't like that
idea either.

>>> I find the '/' a bit confusing compared to the ' ' (but I think you find
>>> the space confusing compared to '/' :-)). I also wonder whether we
>>> shouldn't just drop 'fmt:', as there can be a single format only.
>>
>> You can set it multiple times, or you may not set it at all. That's why I
>> think we should explicitly say it's the format.
> 
> Not at all makes sense, but why would you set it multiple times ?

I guess that's not a very practical use case, albeit there may be
dependencies between the two: Guennadi had a piece of hardware where the
hardware cropping or scaling capabilities depended on the format. But
not setting it at all definitely is a valid use case.

Kind regards,

-- 
Sakari Ailus
sakari.ailus@iki.fi

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

[Laurent Pinchart]

Hi Sakari,

On Sunday 06 May 2012 21:14:02 Sakari Ailus wrote:
> Laurent Pinchart wrote:
> > On Saturday 05 May 2012 16:09:33 Sakari Ailus wrote:
> ...
> 
> >> The pixel format and the image size at the pad are clearly format
> >> (VIDIOC_SUBDEV_S_FMT) but the other things are related to pads but not
> >> format.
> >> 
> >> I see them different kinds of properties of pads. That suggests we might
> >> be better renaming the option (-f) to something else as well.
> > 
> > You like breaking interfaces, don't you ? :-D
> 
> I thought you said we have no stable release yet. :-D
> 
> The selection interface on subdevs is currently used to change format
> related things (cropping and scaling, for example) but it was one of
> Sylwester's patches ("V4L: Add auto focus targets to the selections
> API") that adds a focus window target to the V4L2 selection interface. I
> don't see why it couldn't be present on subdevs, too. That's got nothing
> to do with the image format.
> 
> I've been pondering a bit using another option to configure things
> related to selections. Conveniently "-s" is free. We could leave the
> crop things to -f but remove the documentation related to them.

It would then become much more complex to setup a complete pipeline in a 
single command line, unless we completely modify the way the command line is 
parsed.

What would you think about renaming -f to -V (long option --video or --v4l2) ? 
media-ctl will hopefully be used for non-V4L2 devices in the future, so 
subsystem-specific options will likely be needed.

> I'm fine with keeping the things as they are for now, too, but in that
> case we should recognise that -f will not be for formats only. Or we
> split handling selections into separate options, but I don't like that
> idea either.
> 
> >>> I find the '/' a bit confusing compared to the ' ' (but I think you find
> >>> the space confusing compared to '/' :-)). I also wonder whether we
> >>> shouldn't just drop 'fmt:', as there can be a single format only.
> >> 
> >> You can set it multiple times, or you may not set it at all. That's why I
> >> think we should explicitly say it's the format.
> > 
> > Not at all makes sense, but why would you set it multiple times ?
> 
> I guess that's not a very practical use case, albeit there may be
> dependencies between the two: Guennadi had a piece of hardware where the
> hardware cropping or scaling capabilities depended on the format.

We don't have a way to handle that cleanly in the V4L2 API, do we ?

> But not setting it at all definitely is a valid use case.

-- 
Regards,

Laurent Pinchart

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

[Sakari Ailus]

Hi Laurent,

On Mon, May 07, 2012 at 12:44:45PM +0200, Laurent Pinchart wrote:
> Hi Sakari,
> 
> On Sunday 06 May 2012 21:14:02 Sakari Ailus wrote:
> > Laurent Pinchart wrote:
> > > On Saturday 05 May 2012 16:09:33 Sakari Ailus wrote:
> > ...
> > 
> > >> The pixel format and the image size at the pad are clearly format
> > >> (VIDIOC_SUBDEV_S_FMT) but the other things are related to pads but not
> > >> format.
> > >> 
> > >> I see them different kinds of properties of pads. That suggests we might
> > >> be better renaming the option (-f) to something else as well.
> > > 
> > > You like breaking interfaces, don't you ? :-D
> > 
> > I thought you said we have no stable release yet. :-D
> > 
> > The selection interface on subdevs is currently used to change format
> > related things (cropping and scaling, for example) but it was one of
> > Sylwester's patches ("V4L: Add auto focus targets to the selections
> > API") that adds a focus window target to the V4L2 selection interface. I
> > don't see why it couldn't be present on subdevs, too. That's got nothing
> > to do with the image format.
> > 
> > I've been pondering a bit using another option to configure things
> > related to selections. Conveniently "-s" is free. We could leave the
> > crop things to -f but remove the documentation related to them.
> 
> It would then become much more complex to setup a complete pipeline in a 
> single command line, unless we completely modify the way the command line is 
> parsed.
> 
> What would you think about renaming -f to -V (long option --video or --v4l2) ? 
> media-ctl will hopefully be used for non-V4L2 devices in the future, so 
> subsystem-specific options will likely be needed.

Sounds like a very good idea to me; it solves the issue properly and makes
the V4L2 functionality better separated in media-ctl. I'll put that on top
of the existing stack.

> > I'm fine with keeping the things as they are for now, too, but in that
> > case we should recognise that -f will not be for formats only. Or we
> > split handling selections into separate options, but I don't like that
> > idea either.
> > 
> > >>> I find the '/' a bit confusing compared to the ' ' (but I think you find
> > >>> the space confusing compared to '/' :-)). I also wonder whether we
> > >>> shouldn't just drop 'fmt:', as there can be a single format only.
> > >> 
> > >> You can set it multiple times, or you may not set it at all. That's why I
> > >> think we should explicitly say it's the format.
> > > 
> > > Not at all makes sense, but why would you set it multiple times ?
> > 
> > I guess that's not a very practical use case, albeit there may be
> > dependencies between the two: Guennadi had a piece of hardware where the
> > hardware cropping or scaling capabilities depended on the format.
> 
> We don't have a way to handle that cleanly in the V4L2 API, do we ?

No, not really. The user has to set the format before any selection targets
on either of the pads, wchich is a little bit against the documentation. But
this is a very special case.

Regards,

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi	jabber/XMPP/Gmail: sailus@retiisi.org.uk

[media-ctl PATCH 3/3] Compose rectangle support for media-ctl

[Sakari Ailus]

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

diff --git a/src/main.c b/src/main.c
index 6de1031..6362f3e 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.actual:%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.actual:%u,%u/%ux%u",
+		       rect.left, rect.top, rect.width, rect.height);
+
 	printf("]");
 }
 
diff --git a/src/options.c b/src/options.c
index 4d9d48f..c0b3d3b 100644
--- a/src/options.c
+++ b/src/options.c
@@ -55,13 +55,15 @@ static void usage(const char *argv0, int verbose)
 	printf("\tlink            = pad, '->', pad, '[', flags, ']' ;\n");
 	printf("\tformat          = pad, '[', formats ']' ;\n");
 	printf("\tformats         = formats ',' formats ;\n");
-	printf("\tformats         = fmt | crop | frame interval ;\n");
+	printf("\tformats         = fmt | crop | compose | frame interval ;\n");
 	printf("\fmt              = 'fmt:', fcc, '/', size ;\n");
 	printf("\tpad             = entity, ':', pad number ;\n");
 	printf("\tentity          = entity number | ( '\"', entity name, '\"' ) ;\n");
 	printf("\tsize            = width, 'x', height ;\n");
-	printf("\tcrop            = 'crop.actual:', left, ',', top, '/', size ;\n");
+	printf("\tcrop            = 'crop.actual:', window ;\n");
+	printf("\tcompose         = 'compose.actual:', window ;\n");
 	printf("\tframe interval  = '@', numerator, '/', denominator ;\n");
+	printf("\twindow          = left, ',', top, '/', size ;\n");
 	printf("where the fields are\n");
 	printf("\tentity number   Entity numeric identifier\n");
 	printf("\tentity name     Entity name (string) \n");
diff --git a/src/v4l2subdev.c b/src/v4l2subdev.c
index 87b22fc..8e3a447 100644
--- a/src/v4l2subdev.c
+++ b/src/v4l2subdev.c
@@ -317,8 +317,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;
@@ -355,6 +355,15 @@ static struct media_pad *v4l2_subdev_parse_pad_format(
 			continue;
 		}
 
+		if (!strhazit("compose.actual:", &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)
@@ -468,13 +477,14 @@ 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;
@@ -490,6 +500,10 @@ static int v4l2_subdev_parse_setup_format(struct media_device *media,
 	if (ret < 0)
 		return ret;
 
+	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)
-- 
1.7.2.5

Re: [media-ctl PATCH 1/3] Support selections API for crop

[Laurent Pinchart]

Hi Sakari,

Thanks for the patch.

On Friday 04 May 2012 11:24:41 Sakari Ailus wrote:
> Support the new selections API for crop. Fall back to use the old crop API
> in case the selection API isn't available.

Thanks for the patch. A few minor comments below. There's no need to resubmit, 
I've fixed the problems and applied the patch.

> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> ---
>  src/main.c       |    4 ++-
>  src/v4l2subdev.c |  100 +++++++++++++++++++++++++++++++++++----------------
>  src/v4l2subdev.h |   37 +++++++++++---------
>  3 files changed, 93 insertions(+), 48 deletions(-)

[snip]

> diff --git a/src/v4l2subdev.c b/src/v4l2subdev.c
> index b886b72..92360bb 100644
> --- a/src/v4l2subdev.c
> +++ b/src/v4l2subdev.c
> @@ -104,48 +104,85 @@ int v4l2_subdev_set_format(struct media_entity
> *entity, return 0;
>  }
> 
> -int v4l2_subdev_get_crop(struct media_entity *entity, struct v4l2_rect
> *rect,
> -			 unsigned int pad, enum v4l2_subdev_format_whence which)
> +int v4l2_subdev_get_selection(
> +	struct media_entity *entity, struct v4l2_rect *r,
> +	unsigned int pad, int target, enum v4l2_subdev_format_whence which)

Let's make target an unsigned int.

>  {
> -	struct v4l2_subdev_crop crop;
> +	union {
> +		struct v4l2_subdev_selection sel;
> +		struct v4l2_subdev_crop crop;
> +	} u;
>  	int ret;
> 
>  	ret = v4l2_subdev_open(entity);
>  	if (ret < 0)
>  		return ret;
> 
> -	memset(&crop, 0, sizeof(crop));
> -	crop.pad = pad;
> -	crop.which = which;
> +	memset(&u.sel, 0, sizeof(u.sel));
> +	u.sel.pad = pad;
> +	u.sel.target = target;
> +	u.sel.which = which;
> 
> -	ret = ioctl(entity->fd, VIDIOC_SUBDEV_G_CROP, &crop);
> +	ret = ioctl(entity->fd, VIDIOC_SUBDEV_G_SELECTION, &u.sel);
> + 	if (ret >= 0) {
> +		*r = u.sel.r;
> +		return 0;
> +	}
> +	if (errno != ENOTTY
> +	    || target != V4L2_SUBDEV_SEL_TGT_CROP_ACTUAL)

No need to split the line :-)

> + 		return -errno;
> +
> +	memset(&u.crop, 0, sizeof(u.crop));
> +	u.crop.pad = pad;
> +	u.crop.which = which;
> +
> +	ret = ioctl(entity->fd, VIDIOC_SUBDEV_G_CROP, &u.crop);
>  	if (ret < 0)
>  		return -errno;
> 
> -	*rect = crop.rect;
> +	*r = u.crop.rect;
>  	return 0;
>  }

[snip]

> @@ -355,30 +392,31 @@ static int set_format(struct media_pad *pad,
>  	return 0;
>  }
> 
> -static int set_crop(struct media_pad *pad, struct v4l2_rect *crop)
> +static int set_selection(struct media_pad *pad, int tgt,

unsigned int here as well.

> +			 struct v4l2_rect *rect)

[snip]

> @@ -429,18 +467,18 @@ static int v4l2_subdev_parse_setup_format(struct
> media_device *media, return -EINVAL;
>  	}
> 
> -	if (pad->flags & MEDIA_PAD_FL_SOURCE) {
> -		ret = set_crop(pad, &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_crop(pad, &crop);
> +	if (pad->flags & MEDIA_PAD_FL_SOURCE) {
> +		ret = set_format(pad, &format);
>  		if (ret < 0)
>  			return ret;
>  	}

I would just replace set_crop with set_selection here, and apply the rest of 
the change in patch 3/3.

> diff --git a/src/v4l2subdev.h b/src/v4l2subdev.h
> index 1e75f94..1020747 100644
> --- a/src/v4l2subdev.h
> +++ b/src/v4l2subdev.h
> @@ -88,34 +88,38 @@ int v4l2_subdev_set_format(struct media_entity *entity,
>  	enum v4l2_subdev_format_whence which);
> 
>  /**
> - * @brief Retrieve the crop rectangle on a pad.
> + * @brief Retrieve a selection rectangle on a pad.
>   * @param entity - subdev-device media entity.
> - * @param rect - crop rectangle to be filled.
> + * @param r - rectangle to be filled.
>   * @param pad - pad number.
> + * @param target - selection target
>   * @param which - identifier of the format to get.
>   *
> - * Retrieve the current crop rectangleon the @a entity @a pad and store it
> in
> - * the @a rect structure.
> + * Retrieve the @a target selection rectangle on the @a entity @a pad
> + * and store it in the @a rect structure.

'@a rect' doesn't match '@param r' (same for set_selection).

>   *
> - * @a which is set to V4L2_SUBDEV_FORMAT_TRY to retrieve the try crop
> rectangle
> - * stored in the file handle, of V4L2_SUBDEV_FORMAT_ACTIVE to retrieve the
> - * current active crop rectangle.
> + * @a which is set to V4L2_SUBDEV_FORMAT_TRY to retrieve the try
> + * selection rectangle stored in the file handle, of

s/of/or/ (the typo was already there, but let's fix it).

> + * V4L2_SUBDEV_FORMAT_ACTIVE to retrieve the current active selection
> + * rectangle.
>   *
>   * @return 0 on success, or a negative error code on failure.
>   */

-- 
Regards,

Laurent Pinchart