Closed Bug 861280 Opened 12 years ago Closed 11 years ago

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

Categories

(Core :: WebRTC, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: jesup, Assigned: jesup)

References

(Blocks 1 open bug)

Details

(Whiteboard: [getUserMedia][blocking-webrtc-][qa-automation-blocked][qa-])

Attachments

(1 file, 2 obsolete files)

+++ 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.
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
Whiteboard: [getUserMedia] → [getUserMedia][blocking-webrtc-]
This patch WFM with the v4l2loopback device.
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
Attachment #736884 - Flags: review?(tterribe)
I suspect this blocks automation as well
Yes, I hit this trying to use the v4l2loopback device on the test slaves.
Whiteboard: [getUserMedia][blocking-webrtc-] → [getUserMedia][blocking-webrtc-][qa-automation-blocked]
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.
Blocks: 815002
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.
Attachment #736884 - Flags: review?(tterribe) → review-
Attachment #736884 - Attachment is obsolete: true
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.
Attachment #740653 - Flags: review?(tterribe)
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.
Attachment #740653 - Flags: review?(tterribe) → review-
(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).
Attachment #740653 - Attachment is obsolete: true
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
Attachment #743488 - Flags: review?(tterribe)
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.
Attachment #743488 - Flags: review?(tterribe) → review+
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: mozilla23 → mozilla24
Whiteboard: [getUserMedia][blocking-webrtc-][qa-automation-blocked] → [getUserMedia][blocking-webrtc-][qa-automation-blocked][qa-]
No longer depends on: webrtc_upstream_bugs
No longer blocks: webrtc_upstream_bugs
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: