809554 - Use device name as fallback if capture device doesn't provide uniqueId

Status: VERIFIED FIXED

(Core :: WebRTC, defect, P1)

Description

[(not currently active) Ted Mielczarek]

The Linux video code currently uses v4l2_capability::bus_id as the unique ID when enumerating devices. If you have a device that doesn't fill this field in you can't actually use it. I saw this with the v4l2loopback driver that provides a fake video device.

This patch makes the code fall back to using the device name if the bus_info field isn't present, which makes things work. I also sent a pull request upstream to v4l2loopback to fill in this field.

Comment 1

[(not currently active) Ted Mielczarek]

Attachment: Use device filename as fallback if v4l device doesn't provide bus_info

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Replace empty uniqueId's with deviceName

Still unresolved is verifying if the name is unique; we could add an integer to it, for example.  Because we're using the uuid as a hash key, we may need it to be really unique; we could also check for a collision first.  Mac and Linux don't even try to set the uniqueId for audio devices, which was causing all audio devices to have the same name

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

FYI, my patch is an alternate to Ted's, but it covers audio as well and doesn't need to be upstreamed (though we can submit a Issue to webrtc.org as well)

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

with the audio issues this is blocking

Comment 6

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 680908 [details] [diff] [review]
Replace empty uniqueId's with deviceName

Including ted mostly to verify this fixes his original problem.

Note that devicename and uniqueId are the same length and there are asserts to that in the tree.

Comment 7

[Timothy B. Terriberry (:derf)]

Comment on attachment 680908 [details] [diff] [review]
Replace empty uniqueId's with deviceName

Review of attachment 680908 [details] [diff] [review]:
-----------------------------------------------------------------

I'll r+ this as-is, as it does improve the situation, but this code is busted and needs additional follow-up work.

> Note that devicename and uniqueId are the same length and there are asserts
> to that in the tree.

This is not true in the video case. The kMaxDeviceNameLength half the size of kMaxUniqueIdLength (which, in this case, is safe). I don't see what asserts you're talking about.

::: content/media/webrtc/MediaEngineWebRTC.cpp
@@ +110,5 @@
>  #endif
>  
> +    if (uniqueId[0] == '\0') {
> +      // In case a device doesn't set uniqueId!
> +      strcpy(uniqueId,deviceName);

This is dangerous if the GetCaptureDevice() call fails. You don't know what's in deviceName in that case. For example, the Windows video capture driver passes that pointer directly to WideCharToMultiByte(), and it is not at all clear to me from reading the documentation of that function that it will NUL-terminate the string even when it fails due to an insufficient buffer size.

Inside the #ifdef DEBUG we check for an error, log it, and continue. I think we should be doing something like that even in non-DEBUG builds.

@@ +177,5 @@
>      ptrVoEHw->GetRecordingDeviceName(i, deviceName, uniqueId);
> +    LOG(("  Capture Device Index %d, Name %s Uuid %s", i, deviceName, uniqueId));
> +    if (uniqueId[0] == '\0') {
> +      // Mac and Linux don't set uniqueId!
> +      strcpy(uniqueId,deviceName);

Same problem as above, except now we're not even checking the error return in the DEBUG case.

Comment 8

[Randell Jesup [:jesup] (needinfo me)]

Attachment: if a device has no uniqueId, use the name

Comment 9

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 681012 [details] [diff] [review]
if a device has no uniqueId, use the name

The first patch was a quickie for audio, which has asserts in the webrtc code.  I extended it for video and didn't check asserts there.  Added error check to audio as well.

Comment 10

[Timothy B. Terriberry (:derf)]

Comment on attachment 681012 [details] [diff] [review]
if a device has no uniqueId, use the name

Review of attachment 681012 [details] [diff] [review]:
-----------------------------------------------------------------

We probably still need to do some follow-up investigation to see what we need to do to handle multiple devices with the same name and hotplugged devices.

There's some code that explicitly looks up the uuid in a hash table and re-uses it if already there (though it still adds the duplicate to the list: this is what you were seeing originally). But it also passes the loop index in that list. So any devices with duplicate names (if that's possible) will still appear multiple times in the list and all use the first one. Meanwhile hotplugging might invalidate the indices. So I'm not sure this code handles either case the way it should.

Comment 11

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 681012 [details] [diff] [review]
if a device has no uniqueId, use the name

I think we just need derf's review here.  Ted, please let us know if this fixes your issue as well.  We can spin off bugs for remaining issues.

Comment 12

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/213b5afa3068

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/213b5afa3068

Comment 14

[Jason Smith [:jsmith]]

Can't exactly tell if this can be verified from an end-user perspective or not. Sounds like it's possible, but I'm not sure. Randell - Can you clarify?

Comment 15

[Randell Jesup [:jesup] (needinfo me)]

Yes.  Verify if you see different audio devices offered (with different names) on Mac and Linux.  Also try Ted's fake webcam driver for linux.

Comment 16

[Jason Smith [:jsmith]]

(In reply to Randell Jesup [:jesup] from comment #15)
> Yes.  Verify if you see different audio devices offered (with different
> names) on Mac and Linux.  Also try Ted's fake webcam driver for linux.

On Mac with 3 different input devices attached, I end up seeing 4 input devices - 3 of the devices attached and 1 duplicated off of the default input device.

I'll test this on linux as well.

Comment 17

[Jason Smith [:jsmith]]

(In reply to Jason Smith [:jsmith] from comment #16)
> (In reply to Randell Jesup [:jesup] from comment #15)
> > Yes.  Verify if you see different audio devices offered (with different
> > names) on Mac and Linux.  Also try Ted's fake webcam driver for linux.
> 
> On Mac with 3 different input devices attached, I end up seeing 4 input
> devices - 3 of the devices attached and 1 duplicated off of the default
> input device.
> 
> I'll test this on linux as well.

Yikes. Linux umm....looks wrong? I'm getting somewhere along the lines of 20+ input devices listed off of Ubuntu 12 machine, all of which do not have the device name listed.

Err...what's going on here? Same issue or new bug?

Comment 18

[Randell Jesup [:jesup] (needinfo me)]

That's actually "normal" on Linux because of how it exports audio devices.  I'm looking at ways to collapse the list.  I have around 18 devices shown, many of which are not openable

Comment 19

[Jason Smith [:jsmith]]

Going to retest on Chrome for comparison.

Comment 20

[Jason Smith [:jsmith]]

So the mac piece looks to be expected - Chrome does the same behavior as us.

Comment 21

[Jason Smith [:jsmith]]

Linux does not - chrome actually provides descriptions of each variable device involved, where as our implementation uses "default" with no device name.

In Chrome, I saw my integrated mic broken down into 5 areas, the USB mic had six. Each started with their unique mic device name followed by some extra piece.

I'm filing a followup bug for the linux issue.

Marking as verified for now, although there's a followup needed.

Comment 22

[Jason Smith [:jsmith]]

Filed bug 813382 as the followup.

Comment 23

[Jason Smith [:jsmith]]

If we could fake the devices, we might be able to automate this. Although as it stands in the current framework, we don't have this ability. Putting in-testsuite- for now, although if device faking becomes possible at some point, then we should re-evaluate this one for a test.