Skip to contentSkip to conversation

media: uvcvideo: support multiple frame descriptors with the same dimensions

PATCH In review Philipp Zabel 2 participants 2 messages Dec 19, 2017 – Jan 4, 2018

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 media: uvcvideo: support multiple frame descriptors with the same dimensions

The Microsoft HoloLens Sensors device has two separate frame descriptors with the same dimensions, each with a single different frame interval:

VideoStreaming Interface Descriptor:
bLength 30
bDescriptorType 36
bDescriptorSubtype 5 (FRAME_UNCOMPRESSED)
bFrameIndex 1
bmCapabilities 0x00
Still image unsupported
wWidth 1280
wHeight 481
dwMinBitRate 147763200
dwMaxBitRate 147763200
dwMaxVideoFrameBufferSize 615680
dwDefaultFrameInterval 333333
bFrameIntervalType 1
dwFrameInterval( 0) 333333
VideoStreaming Interface Descriptor:
bLength 30
bDescriptorType 36
bDescriptorSubtype 5 (FRAME_UNCOMPRESSED)
bFrameIndex 2
bmCapabilities 0x00
Still image unsupported
wWidth 1280
wHeight 481
dwMinBitRate 443289600
dwMaxBitRate 443289600
dwMaxVideoFrameBufferSize 615680
dwDefaultFrameInterval 111111
bFrameIntervalType 1
dwFrameInterval( 0) 111111

Skip duplicate dimensions in enum_framesizes, let enum_frameintervals list the intervals from both frame descriptors. Change set_streamparm to switch to the correct frame index when changing the interval. This enables 90 fps capture on a Lenovo Explorer Windows Mixed Reality headset.

---
 drivers/media/usb/uvc/uvc_v4l2.c | 66 ++++++++++++++++++++++++++++++----------
 1 file changed, 50 insertions(+), 16 deletions(-)
drivers/media/usb/uvc/uvc_v4l2.c +50 -16
index 3e7e283a44a8..7d5bf8d56a99 100644
@@ -373,8 +373,11 @@ static int uvc_v4l2_set_streamparm(struct uvc_streaming *stream,
 {
 	struct uvc_streaming_control probe;
 	struct v4l2_fract timeperframe;
-	uint32_t interval;
+	struct uvc_format *format;
+	struct uvc_frame *frame;
+	__u32 interval, tmp, d, maxd;
 	int ret;
+	int i;
 
 	if (parm->type != stream->type)
 		return -EINVAL;
@@ -396,9 +399,31 @@ static int uvc_v4l2_set_streamparm(struct uvc_streaming *stream,
 		return -EBUSY;
 	}
 
+	format = stream->cur_format;
+	frame = stream->cur_frame;
 	probe = stream->ctrl;
-	probe.dwFrameInterval =
-		uvc_try_frame_interval(stream->cur_frame, interval);
+	probe.dwFrameInterval = uvc_try_frame_interval(frame, interval);
+	maxd = abs((__s32)probe.dwFrameInterval - interval);
+
+	/* Try frames with matching size to find the best frame interval. */
+	for (i = 0; i < format->nframes; i++) {
+		if (&format->frame[i] == stream->cur_frame)
+			continue;
+
+		if (format->frame[i].wWidth != stream->cur_frame->wWidth ||
+		    format->frame[i].wHeight != stream->cur_frame->wHeight)
+			continue;
+
+		tmp = uvc_try_frame_interval(&format->frame[i], interval);
+		d = abs((__s32)tmp - interval);
+		if (d >= maxd)
+			continue;
+
+		frame = &format->frame[i];
+		probe.bFrameIndex = frame->bFrameIndex;
+		probe.dwFrameInterval = tmp;
+		maxd = d;
+	}
 
 	/* Probe the device with the new settings. */
 	ret = uvc_probe_video(stream, &probe);
@@ -408,6 +433,7 @@ static int uvc_v4l2_set_streamparm(struct uvc_streaming *stream,
 	}
 
 	stream->ctrl = probe;
+	stream->cur_frame = frame;
 	mutex_unlock(&stream->mutex);
 
 	/* Return the actual frame period. */
@@ -1150,7 +1176,7 @@ static int uvc_ioctl_enum_framesizes(struct file *file, void *fh,
 	struct uvc_streaming *stream = handle->stream;
 	struct uvc_format *format = NULL;
 	struct uvc_frame *frame;
-	int i;
+	int i, index;
 
 	/* Look for the given pixel format */
 	for (i = 0; i < stream->nformats; i++) {
@@ -1162,10 +1188,20 @@ static int uvc_ioctl_enum_framesizes(struct file *file, void *fh,
 	if (format == NULL)
 		return -EINVAL;
 
-	if (fsize->index >= format->nframes)
+	/* Skip duplicate frame sizes */
+	for (i = 0, index = 0; i < format->nframes; i++) {
+		if (i && frame->wWidth == format->frame[i].wWidth &&
+		    frame->wHeight == format->frame[i].wHeight)
+			continue;
+		frame = &format->frame[i];
+		if (index == fsize->index)
+			break;
+		index++;
+	}
+
+	if (i == format->nframes)
 		return -EINVAL;
 
-	frame = &format->frame[fsize->index];
 	fsize->type = V4L2_FRMSIZE_TYPE_DISCRETE;
 	fsize->discrete.width = frame->wWidth;
 	fsize->discrete.height = frame->wHeight;
@@ -1179,7 +1215,7 @@ static int uvc_ioctl_enum_frameintervals(struct file *file, void *fh,
 	struct uvc_streaming *stream = handle->stream;
 	struct uvc_format *format = NULL;
 	struct uvc_frame *frame = NULL;
-	int i;
+	int i, index, nintervals;
 
 	/* Look for the given pixel format and frame size */
 	for (i = 0; i < stream->nformats; i++) {
@@ -1191,30 +1227,28 @@ static int uvc_ioctl_enum_frameintervals(struct file *file, void *fh,
 	if (format == NULL)
 		return -EINVAL;
 
+	index = fival->index;
 	for (i = 0; i < format->nframes; i++) {
 		if (format->frame[i].wWidth == fival->width &&
 		    format->frame[i].wHeight == fival->height) {
 			frame = &format->frame[i];
-			break;
+			nintervals = frame->bFrameIntervalType ?: 1;
+			if (index < nintervals)
+				break;
+			index -= nintervals;
 		}
 	}
-	if (frame == NULL)
+	if (i == format->nframes)
 		return -EINVAL;
 
 	if (frame->bFrameIntervalType) {
-		if (fival->index >= frame->bFrameIntervalType)
-			return -EINVAL;
-
 		fival->type = V4L2_FRMIVAL_TYPE_DISCRETE;
 		fival->discrete.numerator =
-			frame->dwFrameInterval[fival->index];
+			frame->dwFrameInterval[index];
 		fival->discrete.denominator = 10000000;
 		uvc_simplify_fraction(&fival->discrete.numerator,
 			&fival->discrete.denominator, 8, 333);
 	} else {
-		if (fival->index)
-			return -EINVAL;
-
 		fival->type = V4L2_FRMIVAL_TYPE_STEPWISE;
 		fival->stepwise.min.numerator = frame->dwFrameInterval[0];
 		fival->stepwise.min.denominator = 10000000;
Signature · 1 line
2.15.1
maintainer
#1 lore raw

Hi Philipp,

Thank you for the patch.

Quoted 2 lines · On Tuesday, 19 December 2017 10:17:35 EET Philipp Zabel wrote:

The Microsoft HoloLens Sensors device has two separate frame descriptors with the same dimensions, each with a single different frame interval:

Why, oh why ? :-(

Quoted 46 lines

VideoStreaming Interface Descriptor:
bLength 30
bDescriptorType 36
bDescriptorSubtype 5 (FRAME_UNCOMPRESSED)
bFrameIndex 1
bmCapabilities 0x00
Still image unsupported
wWidth 1280
wHeight 481
dwMinBitRate 147763200
dwMaxBitRate 147763200
dwMaxVideoFrameBufferSize 615680
dwDefaultFrameInterval 333333
bFrameIntervalType 1
dwFrameInterval( 0) 333333
VideoStreaming Interface Descriptor:
bLength 30
bDescriptorType 36
bDescriptorSubtype 5 (FRAME_UNCOMPRESSED)
bFrameIndex 2
bmCapabilities 0x00
Still image unsupported
wWidth 1280
wHeight 481
dwMinBitRate 443289600
dwMaxBitRate 443289600
dwMaxVideoFrameBufferSize 615680
dwDefaultFrameInterval 111111
bFrameIntervalType 1
dwFrameInterval( 0) 111111

Skip duplicate dimensions in enum_framesizes, let enum_frameintervals list the intervals from both frame descriptors. Change set_streamparm to switch to the correct frame index when changing the interval. This enables 90 fps capture on a Lenovo Explorer Windows Mixed Reality headset.

---
 drivers/media/usb/uvc/uvc_v4l2.c | 66 +++++++++++++++++++++++++++----------
 1 file changed, 50 insertions(+), 16 deletions(-)
(quoted diff) +0 -0

b/drivers/media/usb/uvc/uvc_v4l2.c index 3e7e283a44a8..7d5bf8d56a99 100644

drivers/media/usb/uvc/uvc_v4l2.c static int uvc_v4l2_set_streamparm(struct uvc_streaming · line 378

Quoted 7 lines
drivers/media/usb/uvc/uvc_v4l2.c +3 -1
@@ -373,8 +373,11 @@ static int uvc_v4l2_set_streamparm(struct uvc_streaming
 *stream, {
 	struct uvc_streaming_control probe;
 	struct v4l2_fract timeperframe;
-	uint32_t interval;
+	struct uvc_format *format;
+	struct uvc_frame *frame;
+	__u32 interval, tmp, d, maxd;

You can declare tmp and d inside the loop, it should simplify the code slightly. Please also avoid naming the variable tmp.

drivers/media/usb/uvc/uvc_v4l2.c static int uvc_v4l2_set_streamparm(struct uvc_streaming *stream, · line 380

Quoted 2 lines
drivers/media/usb/uvc/uvc_v4l2.c +1 -0
 	int ret;
+	int i;

i can be unsigned.

Quoted 6 lines
drivers/media/usb/uvc/uvc_v4l2.c +0 -0
 	if (parm->type != stream->type)
 		return -EINVAL;
@@ -396,9 +399,31 @@ static int uvc_v4l2_set_streamparm(struct uvc_streaming
 *stream, return -EBUSY;
 	}
 

drivers/media/usb/uvc/uvc_v4l2.c static int uvc_v4l2_set_streamparm(struct uvc_streaming · line 409

Quoted 10 lines
drivers/media/usb/uvc/uvc_v4l2.c +7 -2
+	format = stream->cur_format;
+	frame = stream->cur_frame;
 	probe = stream->ctrl;
-	probe.dwFrameInterval =
-		uvc_try_frame_interval(stream->cur_frame, interval);
+	probe.dwFrameInterval = uvc_try_frame_interval(frame, interval);
+	maxd = abs((__s32)probe.dwFrameInterval - interval);
+
+	/* Try frames with matching size to find the best frame interval. */
+	for (i = 0; i < format->nframes; i++) {

As a small optimization you can also stop when maxd == 0.

Quoted 29 lines
drivers/media/usb/uvc/uvc_v4l2.c +18 -0
+		if (&format->frame[i] == stream->cur_frame)
+			continue;
+
+		if (format->frame[i].wWidth != stream->cur_frame->wWidth ||
+		    format->frame[i].wHeight != stream->cur_frame->wHeight)
+			continue;
+
+		tmp = uvc_try_frame_interval(&format->frame[i], interval);
+		d = abs((__s32)tmp - interval);
+		if (d >= maxd)
+			continue;
+
+		frame = &format->frame[i];
+		probe.bFrameIndex = frame->bFrameIndex;
+		probe.dwFrameInterval = tmp;
+		maxd = d;
+	}
 
 	/* Probe the device with the new settings. */
 	ret = uvc_probe_video(stream, &probe);
@@ -408,6 +433,7 @@ static int uvc_v4l2_set_streamparm(struct uvc_streaming
 *stream, }
 
 	stream->ctrl = probe;
+	stream->cur_frame = frame;
 	mutex_unlock(&stream->mutex);
 
 	/* Return the actual frame period. */

drivers/media/usb/uvc/uvc_v4l2.c static int uvc_ioctl_enum_framesizes(struct file · line 1179

Quoted 5 lines
drivers/media/usb/uvc/uvc_v4l2.c +1 -1
@@ -1150,7 +1176,7 @@ static int uvc_ioctl_enum_framesizes(struct file
 *file, void *fh, struct uvc_streaming *stream = handle->stream;
 	struct uvc_format *format = NULL;
 	struct uvc_frame *frame;
-	int i;
+	int i, index;

Could you declare unrelated variables on separate lines ?

I think index can be unsigned (and it seems i can be as well for that matter).

Quoted 26 lines
drivers/media/usb/uvc/uvc_v4l2.c +12 -2
 	/* Look for the given pixel format */
 	for (i = 0; i < stream->nformats; i++) {
@@ -1162,10 +1188,20 @@ static int uvc_ioctl_enum_framesizes(struct file
 *file, void *fh, if (format == NULL)
 		return -EINVAL;
 
-	if (fsize->index >= format->nframes)
+	/* Skip duplicate frame sizes */
+	for (i = 0, index = 0; i < format->nframes; i++) {
+		if (i && frame->wWidth == format->frame[i].wWidth &&
+		    frame->wHeight == format->frame[i].wHeight)
+			continue;
+		frame = &format->frame[i];
+		if (index == fsize->index)
+			break;
+		index++;
+	}
+
+	if (i == format->nframes)
 		return -EINVAL;
 
-	frame = &format->frame[fsize->index];
 	fsize->type = V4L2_FRMSIZE_TYPE_DISCRETE;
 	fsize->discrete.width = frame->wWidth;
 	fsize->discrete.height = frame->wHeight;

drivers/media/usb/uvc/uvc_v4l2.c static int uvc_ioctl_enum_frameintervals(struct file · line 1218

Quoted 5 lines
drivers/media/usb/uvc/uvc_v4l2.c +1 -1
@@ -1179,7 +1215,7 @@ static int uvc_ioctl_enum_frameintervals(struct file
 *file, void *fh, struct uvc_streaming *stream = handle->stream;
 	struct uvc_format *format = NULL;
 	struct uvc_frame *frame = NULL;
-	int i;
+	int i, index, nintervals;

Same comments here.

Quoted 30 lines
drivers/media/usb/uvc/uvc_v4l2.c +7 -6
 	/* Look for the given pixel format and frame size */
 	for (i = 0; i < stream->nformats; i++) {
@@ -1191,30 +1227,28 @@ static int uvc_ioctl_enum_frameintervals(struct file
 *file, void *fh, if (format == NULL)
 		return -EINVAL;
 
+	index = fival->index;
 	for (i = 0; i < format->nframes; i++) {
 		if (format->frame[i].wWidth == fival->width &&
 		    format->frame[i].wHeight == fival->height) {
 			frame = &format->frame[i];
-			break;
+			nintervals = frame->bFrameIntervalType ?: 1;
+			if (index < nintervals)
+				break;
+			index -= nintervals;
 		}
 	}
-	if (frame == NULL)
+	if (i == format->nframes)
 		return -EINVAL;
 
 	if (frame->bFrameIntervalType) {
-		if (fival->index >= frame->bFrameIntervalType)
-			return -EINVAL;
-
 		fival->type = V4L2_FRMIVAL_TYPE_DISCRETE;
 		fival->discrete.numerator =
-			frame->dwFrameInterval[fival->index];
+			frame->dwFrameInterval[index];

drivers/media/usb/uvc/uvc_v4l2.c static int uvc_ioctl_enum_frameintervals(struct file · line 1254

Quoted 10 lines
drivers/media/usb/uvc/uvc_v4l2.c +0 -3
 		fival->discrete.denominator = 10000000;
 		uvc_simplify_fraction(&fival->discrete.numerator,
 			&fival->discrete.denominator, 8, 333);
 	} else {
-		if (fival->index)
-			return -EINVAL;
-
 		fival->type = V4L2_FRMIVAL_TYPE_STEPWISE;
 		fival->stepwise.min.numerator = frame->dwFrameInterval[0];
 		fival->stepwise.min.denominator = 10000000;

I have to say I dislike the complexity added by this patch, but it's not your fault. The code itself is probably as clean as it can be :)

Signature · 3 lines
Regards,

Laurent Pinchart