Skip to content

UVC: move application decision to the application #93192

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Oct 23, 2025
Merged

UVC: move application decision to the application #93192

Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
413afdc
usb: device_next: uvc: propagate descriptor error to the application
josuah Oct 13, 2025
a44abde
usb: device_next: uvc: use fmt->size instead of computing it every time
josuah Oct 13, 2025
6e7b3cd
usb: device_next: uvc: remove application decisions from the UVC class
josuah Jul 15, 2025
121d8b1
samples: usb: uvc: add filtering of the format
josuah Jul 16, 2025
d6d97c6
doc: release: 4.3: add UVC changes
josuah Jul 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 50 additions & 18 deletions subsys/usb/device_next/class/usbd_uvc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -577,7 +577,7 @@ static int uvc_get_vs_probe_max_size(const struct device *dev, struct uvc_probe
{
struct uvc_data *data = dev->data;
struct video_format *fmt = &data->video_fmt;
uint32_t max_frame_size = MAX(fmt->pitch, fmt->width) * fmt->height;
uint32_t max_frame_size = fmt->size;
uint32_t max_payload_size = max_frame_size + UVC_MAX_HEADER_LENGTH;

switch (request) {
Expand Down Expand Up @@ -633,9 +633,8 @@ static int uvc_get_vs_format_from_desc(const struct device *dev, struct video_fo
/* Fill the format according to what the host selected */
fmt->width = frame_desc->wWidth;
fmt->height = frame_desc->wHeight;
fmt->pitch = fmt->width * video_bits_per_pixel(fmt->pixelformat) / BITS_PER_BYTE;

return 0;
return video_estimate_fmt_size(fmt);
}

static int uvc_get_vs_probe_struct(const struct device *dev, struct uvc_probe *const probe,
Expand Down Expand Up @@ -1374,7 +1373,7 @@ static int uvc_assign_desc(const struct device *dev, void *const desc,

return 0;
err:
LOG_ERR("Out of descriptor pointers, raise CONFIG_USBD_VIDEO_MAX_FORMATS above %u",
LOG_WRN("Out of descriptors, raise CONFIG_USBD_VIDEO_MAX_FORMATS above %u",
CONFIG_USBD_VIDEO_MAX_FORMATS);
return -ENOMEM;
}
Expand Down Expand Up @@ -1480,8 +1479,7 @@ static void uvc_set_vs_bitrate_range(struct uvc_frame_discrete_descriptor *const
uint32_t bitrate_max = sys_le32_to_cpu(desc->dwMaxBitRate);
uint32_t bitrate;

/* Multiplication/division in this order to avoid overflow */
bitrate = MAX(fmt->pitch, fmt->width) * frmival_nsec / (NSEC_PER_SEC / 100) * fmt->height;
bitrate = (uint64_t)fmt->size * frmival_nsec / (NSEC_PER_SEC / 100);

/* Extend the min/max value to include the bitrate of this format */
bitrate_min = MIN(bitrate_min, bitrate);
Expand All @@ -1506,8 +1504,9 @@ static int uvc_add_vs_frame_interval(struct uvc_frame_discrete_descriptor *const
int i = desc->bFrameIntervalType;

if (i >= CONFIG_USBD_VIDEO_MAX_FRMIVAL) {
LOG_WRN("Out of frame interval fields");
return -ENOSPC;
LOG_WRN("Out of descriptors, raise CONFIG_USBD_VIDEO_MAX_FRMIVAL above %u",
CONFIG_USBD_VIDEO_MAX_FRMIVAL);
return -ENOMEM;
}

desc->dwFrameInterval[i] = sys_cpu_to_le32(video_frmival_nsec(frmival) / 100);
Expand All @@ -1532,7 +1531,7 @@ static int uvc_add_vs_frame_desc(const struct device *dev,
struct video_format fmt = {.pixelformat = cap->pixelformat,
.width = w, .height = h, .pitch = p};
struct video_frmival_enum fie = {.format = &fmt};
uint32_t max_size = MAX(p, w) * h;
int ret;

__ASSERT_NO_MSG(data->video_dev != NULL);
__ASSERT_NO_MSG(format_desc != NULL);
Expand All @@ -1550,7 +1549,7 @@ static int uvc_add_vs_frame_desc(const struct device *dev,
desc->bFrameIndex = format_desc->bNumFrameDescriptors + 1;
desc->wWidth = sys_cpu_to_le16(w);
desc->wHeight = sys_cpu_to_le16(h);
desc->dwMaxVideoFrameBufferSize = sys_cpu_to_le32(max_size);
desc->dwMaxVideoFrameBufferSize = sys_cpu_to_le32(fmt.size);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry nitpicking: for commit "usb: device_next: uvc: use fmt->size instead of computing it every time",
fmt.size is not already set. This gets fixed in a later commit but I would suggest that this commit still uses

	desc->dwMaxVideoFrameBufferSize = sys_cpu_to_le32(MAX(p, w) * h);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I must have verified the final version of the file, but this fmt struct is local in this commit. Good catch for bissectability!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this got merged. Since this got raised past-review and is in git history by now. One commit will have a runtime (not compile time) error unfortunately 😞

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I was too slow to react. Now it's merged. My bad.

Copy link
Contributor Author

@josuah josuah Oct 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's also very useful as a reminder of what I need to check the next time, so still useful :)

desc->bDescriptorSubtype = (format_desc->bDescriptorSubtype == UVC_VS_FORMAT_UNCOMPRESSED)
? UVC_VS_FRAME_UNCOMPRESSED : UVC_VS_FRAME_MJPEG;
desc->dwMinBitRate = sys_cpu_to_le32(UINT32_MAX);
Expand All @@ -1561,12 +1560,26 @@ static int uvc_add_vs_frame_desc(const struct device *dev,
switch (fie.type) {
case VIDEO_FRMIVAL_TYPE_DISCRETE:
LOG_DBG("Adding discrete frame interval %u", fie.index);
uvc_add_vs_frame_interval(desc, &fie.discrete, &fmt);

ret = uvc_add_vs_frame_interval(desc, &fie.discrete, &fmt);
if (ret != 0) {
return ret;
}

break;
case VIDEO_FRMIVAL_TYPE_STEPWISE:
LOG_DBG("Adding stepwise frame interval %u", fie.index);
uvc_add_vs_frame_interval(desc, &fie.stepwise.min, &fmt);
uvc_add_vs_frame_interval(desc, &fie.stepwise.max, &fmt);

ret = uvc_add_vs_frame_interval(desc, &fie.stepwise.min, &fmt);
if (ret != 0) {
return ret;
}

ret = uvc_add_vs_frame_interval(desc, &fie.stepwise.max, &fmt);
if (ret != 0) {
return ret;
}

break;
default:
CODE_UNREACHABLE;
Expand All @@ -1578,7 +1591,10 @@ static int uvc_add_vs_frame_desc(const struct device *dev,
if (desc->bFrameIntervalType == 0) {
struct video_frmival frmival = {.numerator = 1, .denominator = 30};

uvc_add_vs_frame_interval(desc, &frmival, &fmt);
ret = uvc_add_vs_frame_interval(desc, &frmival, &fmt);
if (ret != 0) {
return ret;
}
}

/* UVC requires the frame intervals to be sorted, but not Zephyr */
Expand Down Expand Up @@ -1671,7 +1687,11 @@ static int uvc_init(struct usbd_class_data *const c_data)
if (prev_pixfmt != cap->pixelformat) {
if (prev_pixfmt != 0) {
cfg->desc->if1_hdr.wTotalLength += cfg->desc->if1_color.bLength;
uvc_assign_desc(dev, &cfg->desc->if1_color, true, true);

ret = uvc_assign_desc(dev, &cfg->desc->if1_color, true, true);
if (ret != 0) {
return ret;
}
}

ret = uvc_add_vs_format_desc(dev, &format_desc, cap);
Expand All @@ -1696,9 +1716,21 @@ static int uvc_init(struct usbd_class_data *const c_data)
}

cfg->desc->if1_hdr.wTotalLength += cfg->desc->if1_color.bLength;
uvc_assign_desc(dev, &cfg->desc->if1_color, true, true);
uvc_assign_desc(dev, &cfg->desc->if1_ep_fs, true, false);
uvc_assign_desc(dev, &cfg->desc->if1_ep_hs, false, true);

ret = uvc_assign_desc(dev, &cfg->desc->if1_color, true, true);
if (ret != 0) {
return ret;
}

ret = uvc_assign_desc(dev, &cfg->desc->if1_ep_fs, true, false);
if (ret != 0) {
return ret;
}

ret = uvc_assign_desc(dev, &cfg->desc->if1_ep_hs, false, true);
if (ret != 0) {
return ret;
}

cfg->desc->if1_hdr.wTotalLength = sys_cpu_to_le16(cfg->desc->if1_hdr.wTotalLength);

Expand Down