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

Status: RESOLVED FIXED

(Core :: WebRTC, defect, P1)

Description

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

+++ This bug was initially created as a clone of Bug #809554 +++

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

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

So it turns out bug 809554 doesn't actually solve the problem...

This appears to.  You have to go deep in the webrtc.org code to override uniqueId

Comment 2

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

Attachment: use fake_N for uniqueId for video devices with no capability value we can use

Comment 3

[(not currently active) Ted Mielczarek]

This patch WFM with the v4l2loopback device.

Comment 4

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

Comment on attachment 736884 [details] [diff] [review]
use fake_N for uniqueId for video devices with no capability value we can use

Ted has tested this locally (as have I) and it works

Comment 5

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

I suspect this blocks automation as well

Comment 6

[(not currently active) Ted Mielczarek]

Yes, I hit this trying to use the v4l2loopback device on the test slaves.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+2]]

If it's failing on the test slaves then we block bug 815002, which is a blocker for us. So this bug will have to be put as blocker too.

Comment 8

[Timothy B. Terriberry (:derf)]

Comment on attachment 736884 [details] [diff] [review]
use fake_N for uniqueId for video devices with no capability value we can use

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

r- for missing at least one important piece of this patch.

::: media/webrtc/trunk/webrtc/modules/video_capture/linux/device_info_linux.cc
@@ +179,5 @@
>      return 0;
>  }
>  
>  WebRtc_Word32 DeviceInfoLinux::CreateCapabilityMap(
>                                          const char* deviceUniqueIdUTF8)

This function scans the devices looking for one with the ID deviceUniqueIdUTF8, but you haven't updated it to check for your "fake" IDs. It also already appears to have a fallback for the cap.bus_info[0] == 0 case that doesn't match what you're doing.

::: media/webrtc/trunk/webrtc/modules/video_capture/linux/video_capture_linux.cc
@@ +70,5 @@
>          memcpy(_deviceUniqueId, deviceUniqueIdUTF8, len + 1);
>      }
>  
> +    int device_index;
> +    if (sscanf(deviceUniqueIdUTF8,"fake_%d",&device_index) == 1)

Local style has spaces after commas in function parameter lists.

Comment 9

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

Attachment: use fake_N for uniqueId for video devices with no capability value we can use

Comment 10

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

Comment on attachment 740653 [details] [diff] [review]
use fake_N for uniqueId for video devices with no capability value we can use

Figured out how to get the v4l2loopback device to work on kernel 3.8 (needs a mod to the loopback driver).

Updated the capability code to match.  Verified with and without this change that capabilities are now returned (at least when the v4l2loopback device has an input), though gUM works even without the capability change.

Note: I've fixed the spacing on the last file locally, but uploaded this before doing so.

Comment 11

[Timothy B. Terriberry (:derf)]

Comment on attachment 740653 [details] [diff] [review]
use fake_N for uniqueId for video devices with no capability value we can use

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

::: media/webrtc/trunk/webrtc/modules/video_capture/linux/device_info_linux.cc
@@ +157,5 @@
>              return -1;
>          }
> +    } else {
> +        // if there's no bus info to use for uniqueId, invent one - and it has to be repeatable
> +#ifdef _WIN32

This would be for the Windows version of Linux?

@@ +226,5 @@
> +                        found = true;
> +                        break; // fd matches with device unique id supplied
> +                    }
> +                }
> +                else //match for device name

As I said in my previous review, this does not match what you're doing in GetDevice(). Either you need to make that code fall back to this before resorting the fake_%d case, or you need to get rid of this case.

@@ -211,5 @@
>          }
> -        close(fd); // close since this is not the matching device
> -    }
> -
> -    if (!found)

What happened to this code? It seems pretty important.

Comment 12

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

(In reply to Timothy B. Terriberry (:derf) from comment #11)
> Comment on attachment 740653 [details] [diff] [review]
> use fake_N for uniqueId for video devices with no capability value we can use
> 
> Review of attachment 740653 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> :::
> media/webrtc/trunk/webrtc/modules/video_capture/linux/device_info_linux.cc
> @@ +157,5 @@
> >              return -1;
> >          }
> > +    } else {
> > +        // if there's no bus info to use for uniqueId, invent one - and it has to be repeatable
> > +#ifdef _WIN32
> 
> This would be for the Windows version of Linux?

Excellent point. :-)  Habit when coding something with snprintf()

> 
> @@ +226,5 @@
> > +                        found = true;
> > +                        break; // fd matches with device unique id supplied
> > +                    }
> > +                }
> > +                else //match for device name
> 
> As I said in my previous review, this does not match what you're doing in
> GetDevice(). Either you need to make that code fall back to this before
> resorting the fake_%d case, or you need to get rid of this case.

Removed and replaced with:
// else can't be a match because the test above for fake_* would have matched it

> 
> @@ -211,5 @@
> >          }
> > -        close(fd); // close since this is not the matching device
> > -    }
> > -
> > -    if (!found)
> 
> What happened to this code? It seems pretty important.

The if (!found) block got mistakenly deleted (though it'd be an unusual but not quite impossible case, with a device disappearing between an earlier call and this one).

Comment 13

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

Attachment: use fake_N for uniqueId for video devices with no capability value we can use

Comment 14

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

Comment on attachment 743488 [details] [diff] [review]
use fake_N for uniqueId for video devices with no capability value we can use

Thanks, all issues should be addressed

Comment 15

[Timothy B. Terriberry (:derf)]

Comment on attachment 743488 [details] [diff] [review]
use fake_N for uniqueId for video devices with no capability value we can use

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

(In reply to Randell Jesup [:jesup] from comment #12)
> The if (!found) block got mistakenly deleted (though it'd be an unusual but
> not quite impossible case, with a device disappearing between an earlier
> call and this one).

All it takes is someone unplugging a device at the wrong time (with 100's of millions of users, more likely than you think).

But okay, this now LGTM.

Comment 16

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/8f5fcd8c730b

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8f5fcd8c730b