1650572 - no more camera devices since bug #1637319 in OpenBSD

Status: RESOLVED FIXED

(Core :: WebRTC: Audio/Video, defect, P3)

Description

[Landry Breuil (:gaston)]

#1637319 added code to check for V4L2_CAP_VIDEO_CAPTURE capability, but the check is done on device_caps field. According to the doc on https://www.kernel.org/doc/html/v4.14/media/uapi/v4l/vidioc-querycap.html#description the device_caps field :

This field is only set if the capabilities field contains the V4L2_CAP_DEVICE_CAPS capability. Only the capabilities field can have the V4L2_CAP_DEVICE_CAPS capability, device_caps will never set V4L2_CAP_DEVICE_CAPS.

from my understanding https://hg.mozilla.org/integration/autoland/rev/33facf191f23 assumes that device_caps is set.

I think the check should be done on capabilities and not device_caps. For example on OpenBSD the uvideo(4) driver doesnt set device_caps but sets V4L2_CAP_DEVICE_CAPS in capabilities. Of course, we could add the capabilities in both struct members (per https://marc.info/?l=openbsd-tech&m=159387507328249&w=2) but maybe the right way to fix it is to check for the proper member ?

Comment 1

[Dan Minor [:dminor]]

From the documentation you linked: capabilities is Available capabilities of the physical device as a whole and device_caps is Device capabilities of the opened device. My reading of that is that if we have a single physical device that creates multiple /dev/videoX entries, capabilities will contain the same information for each /dev/videoX, and we'll be back to the same problem that Bug 1637319 was attempting to fix.

To me, the documentation implies that if you set V4L2_CAP_DEVICE_CAPS in capabilities, then device_caps should be valid. It seems like the right thing to do is read capabilities if V4L2_CAP_DEVICE_CAPS is not set, but otherwise look at the device_caps field. But it sounds like this won't fix your problem, since you're setting V4L2_CAP_DEVICE_CAPS but not populating device_caps.

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1637319

Comment 3

[Landry Breuil (:gaston)]

(In reply to Dan Minor [:dminor] from comment #1)

From the documentation you linked: capabilities is Available capabilities of the physical device as a whole and device_caps is Device capabilities of the opened device. My reading of that is that if we have a single physical device that creates multiple /dev/videoX entries, capabilities will contain the same information for each /dev/videoX, and we'll be back to the same problem that Bug 1637319 was attempting to fix.

To me, the documentation implies that if you set V4L2_CAP_DEVICE_CAPS in capabilities, then device_caps should be valid. It seems like the right thing to do is read capabilities if V4L2_CAP_DEVICE_CAPS is not set, but otherwise look at the device_caps field. But it sounds like this won't fix your problem, since you're setting V4L2_CAP_DEVICE_CAPS but not populating device_caps.

we werent setting V4L2_CAP_DEVICE_CAPS at all, so that would have worked for us.

In the meantime i've "solved" it in the driver by setting both (cf https://github.com/openbsd/src/commit/6c07c68ec7a72548650660e3830cd60fd8756c9c), with V4L2_CAP_DEVICE_CAPS set on capabilities as expected by the firefox webrtc code - brings my camera back.

Dunno for other drivers/OSes though, so i think for correctness then yes V4L2_CAP_DEVICE_CAPS should be checked in capabilities before directly accessing device_caps - and checking for V4L2_CAP_VIDEO_CAPTURE in capabilities if V4L2_CAP_DEVICE_CAPS isnt set.

Comment 4

[Dan Minor [:dminor]]

Attachment: Bug 1650572 - Check V4L2_CAP_DEVICE_CAPS before accessing device_caps; r=ng!

The capabilities field is for the physical device, device_caps is for the
specific /dev/videoX device that has been opened. The device_caps field is
only populated if V4L2_CAP_DEVICE_CAPS is set, so we should check that, and
fall back to capabilities if it is not set.

Comment 5

[Landry Breuil (:gaston)]

Comment on attachment 9161611 [details]
Bug 1650572 - Check V4L2_CAP_DEVICE_CAPS before accessing device_caps; r=ng!

logic reads good to me, thanks!

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b5acbf536c46
Check V4L2_CAP_DEVICE_CAPS before accessing device_caps; r=ng

Comment 7

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/b5acbf536c46

Comment 8

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:dminor, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Comment 9

[Dan Minor [:dminor]]

Comment on attachment 9161611 [details]
Bug 1650572 - Check V4L2_CAP_DEVICE_CAPS before accessing device_caps; r=ng!

Beta/Release Uplift Approval Request

• User impact if declined: Cameras will not be enumerated properly on some operating systems. OpenBSD now has a workaround, but others might be affected.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Not risky, this is a small change.
• String changes made/needed: None

Comment 10

[Dan Minor [:dminor]]

This will need upstreaming as well.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9161611 [details]
Bug 1650572 - Check V4L2_CAP_DEVICE_CAPS before accessing device_caps; r=ng!

Approved for 79.0b8. Do we need this on ESR78 also?

Comment 12

[Dan Minor [:dminor]]

Comment on attachment 9161611 [details]
Bug 1650572 - Check V4L2_CAP_DEVICE_CAPS before accessing device_caps; r=ng!

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: This breaks video capture device enumeration on some operating systems.
• User impact if declined: No cameras available when making WebRTC calls on affected operating systems.
• Fix Landed on Version: 80
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Low risk, this is a small change to the enumeration code.
• String or UUID changes made by this patch: None

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/0865e20708ee

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9161611 [details]
Bug 1650572 - Check V4L2_CAP_DEVICE_CAPS before accessing device_caps; r=ng!

Approved for 78.1esr.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr78/rev/f003d4897686

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr78/rev/f003d4897686

Comment 17

[Dan Minor [:dminor]]

Upstream issue: https://bugs.chromium.org/p/webrtc/issues/detail?id=11797
Upstream review: https://webrtc-review.googlesource.com/c/src/+/180140