[PATCH 2/2] clients: teach simple-dmabuf-v4l to deal with flipped input

[Pekka Paalanen]

On Wed,  1 Feb 2017 15:28:24 -0500
Micah Fedke <micah.fedke at collabora.co.uk> wrote:

> The v4l2 API can be queried to detect if the input video image is
> horizontally or vertically flipped. If the image is y-flipped, we can
> set the ZWP_LINUX_BUFFER_PARAMS_V1_FLAGS_Y_INVERT flag to notify the
> compositor.  If the image is h-flipped, we can only print a warning
> since linux_buffer_params_v1 does not support horizontal flipping.
> ---
>  clients/simple-dmabuf-v4l.c | 54 ++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 46 insertions(+), 8 deletions(-)
> 
> diff --git a/clients/simple-dmabuf-v4l.c b/clients/simple-dmabuf-v4l.c
> index af25d0ea..e4e2d7c7 100644
> --- a/clients/simple-dmabuf-v4l.c
> +++ b/clients/simple-dmabuf-v4l.c
> @@ -351,21 +351,59 @@ static const struct zwp_linux_buffer_params_v1_listener params_listener = {
>  	create_failed
>  };

Hi Micah,

I only have some easy comments, it would be nice if someone could
double-check the V4L2 API usage. The functionality looks correct to me.

>  
> +static bool
> +check_v4l2_control(const int fd,

Since this seems to be hardcoded to boolean controls, how about calling
it check_v4l2_control_bool() instead?

> +                   const struct v4l2_query_ext_ctrl *qectrl,
> +                   const char *control_name,
> +                   const int len,

I don't think 'len' is necessary, everything should be nul-terminated,
and we don't want to accidentally match a substring.

> +                   const int expected_value)
> +{
> +	struct v4l2_control ctrl;
> +
> +	memset(&ctrl, 0, sizeof(ctrl));
> +	ctrl.id = qectrl->id;
> +
> +	if (!(qectrl->flags & V4L2_CTRL_FLAG_DISABLED) &&
> +            (qectrl->type == V4L2_CTRL_TYPE_BOOLEAN) &&
> +            !strncmp(qectrl->name, control_name, len) &&
> +            !xioctl(fd, VIDIOC_G_CTRL, &ctrl) &&
> +            (ctrl.value == expected_value))

This pretty compactly written. Personally I'd prefer testing things a
bit more separately, to e.g. emphasize that there is a call to xioctl()
hidden in there. This is a small function where it is easy to just
return early when something doesn't match.

> +		return true;
> +	else
> +		return false;
> +}
> +
>  static void
>  create_dmabuf_buffer(struct display *display, struct buffer *buffer)
>  {
>  	struct zwp_linux_buffer_params_v1 *params;
>  	uint64_t modifier;
> -	uint32_t flags;
> +	uint32_t lbp_flags;
>  	unsigned i;
> +	struct v4l2_query_ext_ctrl qectrl;
> +	const unsigned int v4l2_qec_flags =
> +                           V4L2_CTRL_FLAG_NEXT_CTRL | V4L2_CTRL_FLAG_NEXT_COMPOUND;
> +	const char *vflip_ctrl = "Vertical Flip";
> +	const char *hflip_ctrl = "Horizontal Flip";
>  
>  	modifier = 0;
> -	flags = 0;
> -
> -	/* XXX: apparently some webcams may actually provide y-inverted images,
> -	 * in which case we should set
> -	 * flags = ZWP_LINUX_BUFFER_PARAMS_V1_FLAGS_Y_INVERT
> -	 */
> +	lbp_flags = 0;
> +
> +	/* handle relevant v4l2 controls */
> +	memset(&qectrl, 0, sizeof(qectrl));
> +	qectrl.id |= v4l2_qec_flags;
> +	while (!xioctl(display->v4l_fd, VIDIOC_QUERY_EXT_CTRL, &qectrl)) {
> +		if (check_v4l2_control(display->v4l_fd, &qectrl, vflip_ctrl,
> +                                       strlen(vflip_ctrl), 0x1)) {
> +			lbp_flags = ZWP_LINUX_BUFFER_PARAMS_V1_FLAGS_Y_INVERT;
> +			printf ("\"%s\" control is set, inverting Y\n",vflip_ctrl);
> +		} else if (check_v4l2_control(display->v4l_fd, &qectrl,hflip_ctrl,

Many missing spaces after commas.

> +                                              strlen(hflip_ctrl), 0x1)) {
> +			printf ("\"%s\" control is set, but dmabuf output cannot"
> +                                "be flipped horizontally\n", hflip_ctrl);
> +		}
> +		qectrl.id |= v4l2_qec_flags;
> +	}
>  
>  	params = zwp_linux_dmabuf_v1_create_params(display->dmabuf);
>  	for (i = 0; i < display->format.num_planes; ++i)
> @@ -382,7 +420,7 @@ create_dmabuf_buffer(struct display *display, struct buffer *buffer)
>  	                                  display->format.width,
>  	                                  display->format.height,
>  	                                  display->drm_format,
> -	                                  flags);
> +	                                  lbp_flags);
>  }
>  
>  static int

Anyway, this patch does what I wanted it to do.


Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 801 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20170206/e6531f37/attachment.sig>