Skip to content

uvc: improve standard compliance and MacOS compat #92320

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

Jump to bottom
Merged
merged 2 commits into from
Jul 24, 2025
Merged

uvc: improve standard compliance and MacOS compat #92320

merged 2 commits into from
Jul 24, 2025

Conversation

josuah
Copy link
Contributor

@josuah josuah commented Jun 27, 2025

That also improve standard compliance:

If the parameter block is longer than is indicated in the wLength field, only the initial bytes of the parameter block are returned. -- https://www.usb.org/document-library/video-class-v15-document-set

cc @kartben who was interested in testing on this platform

@josuah josuah self-assigned this Jun 27, 2025
@josuah josuah added area: USB Universal Serial Bus area: Video Video subsystem labels Jun 27, 2025
@josuah josuah force-pushed the pr_fix_uvc_macos branch from 52aed03 to d2eedd9 Compare June 27, 2025 15:26
@josuah josuah force-pushed the pr_fix_uvc_macos branch from d2eedd9 to 2d1409c Compare July 15, 2025 16:14
@josuah
Copy link
Contributor Author

josuah commented Jul 15, 2025

Force-push:

  • rebase on top of v4.2.0-rc3 (main)

I could check that it fixes a bug, but another one appears after it.
More commits will address it.

@josuah
Copy link
Contributor Author

josuah commented Jul 18, 2025 •
edited
Loading

Force-push:

  • Add another commit to cap the response to wLength

Mac OSX should now work however some extra changes are required to get most applications work out of the box:

@josuah josuah force-pushed the pr_fix_uvc_macos branch 2 times, most recently from 429c057 to ba50197 Compare July 19, 2025 08:15
@josuah josuah removed the area: Video Video subsystem label Jul 19, 2025
@josuah
Copy link
Contributor Author

josuah commented Jul 19, 2025

Force-push:

  • reworded the commit message

@josuah josuah marked this pull request as ready for review July 19, 2025 08:16
@josuah josuah changed the title usb: uvc: improve MacOS compatibility uvc: improve standard compliance and MacOS compat Jul 19, 2025
@josuah josuah removed their assignment Jul 19, 2025
@josuah josuah added this to the v4.2.1 milestone Jul 19, 2025
@kartben kartben removed this from the v4.2.1 milestone Jul 19, 2025
@kartben kartben added backport v4.2-branch Request backport to the v4.2-branch area: Video Video subsystem labels Jul 19, 2025
@josuah
usb: uvc: allow the host to use short probe/commit messages
26cb178 
Some OSes like MacOS use shorter UVC 1.1 probe/commit messages even when
UVC 1.5 is supported, without bUsage, bBitDepthLuma, bmSettings,
bMaxNumberOfRefFramesPlus1, bmRateControlModes bmLayoutPerStream.
Accept messages of arbitrary size to safely be processed, ignoring all
missing fields, improving standard compliance.

Signed-off-by: Josuah Demangeon <[email protected]>
@josuah josuah force-pushed the pr_fix_uvc_macos branch from ba50197 to ff18028 Compare July 20, 2025 10:49
@josuah
Copy link
Contributor Author

josuah commented Jul 20, 2025

Force-push:

  • Rebase on latest main to fix CI endless loop on one arch

@josuah josuah mentioned this pull request Jul 21, 2025
Open
tmon-nordic
tmon-nordic previously approved these changes Jul 22, 2025
View reviewed changes
Copy link
Contributor

@tmon-nordic tmon-nordic left a comment

Choose a reason for hiding this comment

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

The "some cases" for wLength are generally related to alignment and granularity requirements.

jfischer-no
jfischer-no reviewed Jul 23, 2025
View reviewed changes
subsys/usb/device_next/class/usbd_uvc.c Outdated
Comment on lines 701 to 703
size = net_buf_tailroom(buf);
size = MIN(size, sizeof(struct uvc_probe));
size = MIN(size, setup->wLength);
Copy link
Contributor

Choose a reason for hiding this comment

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

Three lines are probably one or two too many, maybe just
size = MIN(net_buf_tailroom(buf), MIN(sizeof(struct uvc_probe), setup->wLength));

something like that should be more aligned to what we have in the tree.
Or keep the const size

const size_t size = MIN(sizeof(struct uvc_probe),
			MIN(net_buf_tailroom(buf), setup->wLength));

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 was hesitating between the two when making the pach... Applied.

@josuah
Copy link
Contributor Author

josuah commented Jul 23, 2025

The "some cases" for wLength are generally related to alignment and granularity requirements.

I was supposing some behavior from allocation but now I see more clearly, thanks.

@josuah josuah force-pushed the pr_fix_uvc_macos branch from ff18028 to 828063f Compare July 23, 2025 17:08
@tmon-nordic
Copy link
Contributor

I was supposing some behavior from allocation but now I see more clearly, thanks.

Please expand the "some cases" in commit message then. People who later come on this commit may not necessarily look into the PR.

@josuah
usb: uvc: respect setup->wLength in responses
c289f3d 
Due to the alignment and granularity requirements of memory allocation,
setup->wLength is shorter than the allocated buffer size.
This lead to responses larger than what the host requested, which it
rejected. Fix it by using the minimum between the allocated size, the
struct size, and the wLength requested.

Signed-off-by: Josuah Demangeon <[email protected]>
@josuah josuah force-pushed the pr_fix_uvc_macos branch from 828063f to c289f3d Compare July 24, 2025 09:38
@josuah
Copy link
Contributor Author

josuah commented Jul 24, 2025

You are right, applied. Thanks for the reminder.

@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

tmon-nordic
tmon-nordic approved these changes Jul 24, 2025
View reviewed changes
Copy link
Contributor

@tmon-nordic tmon-nordic left a comment

Choose a reason for hiding this comment

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

"setup->wLength is shorter than the allocated buffer size" -> "setup->wLength can be shorter than the allocated buffer size"

@fabiobaltieri fabiobaltieri merged commit 366c64e into zephyrproject-rtos:main Jul 24, 2025
25 checks passed
@github-actions github-actions bot mentioned this pull request Jul 24, 2025
Open
@josuah
Copy link
Contributor Author

josuah commented Jul 24, 2025

I'll get better at commit messages...

@jfischer-no
Copy link
Contributor

I'll get better at commit messages...

Don't worry, you are well above the project average.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jfischer-no jfischer-no jfischer-no approved these changes

@tmon-nordic tmon-nordic tmon-nordic approved these changes

Assignees

@jfischer-no jfischer-no

Labels
area: USB Universal Serial Bus area: Video Video subsystem backport v4.2-branch Request backport to the v4.2-branch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@josuah @tmon-nordic @jfischer-no @kartben @fabiobaltieri