1331648 - Only one event is registered when plugging / unplugging camera on Ubuntu for mediaDevices.ondevicechange

Status: RESOLVED FIXED

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

Description

[Alexandru Simonca [:asimonca], Release Desktop QA]

[Note]:
- This issue is reproducible only on Ubuntu
- The steps provided are for reproducing the issue with the same hardware: 1 webcam & 1 headset with microphone (in this case: webcam - Microsoft® LifeCam HD-3000, headset: Plantronics Audio 355)

[Affected versions]:
- Aurora 52.0a2 

[Affected platforms]:
- Ubuntu 16.04 x64

[Steps to reproduce]:
1. Go to https://jsfiddle.net/jib1/Lqo4paed/
2. Press "gUM!" and "Share the selected device" on the doorhanger.
3. Press "Enumerate!". 4 devices are found: videoinput: Microsoft® LifeCam HD-3000, audioinput: default: LifeCam HD-3000 Analog Mono, audioinput: LifeCam HD-3000 Analog Mono, audioinput: Built-in Audio Analog Stereo
4. Disconnect the webcam. 

[Expected result]:
- 2 "device change!" messages should appear informing the user that there was a change in "videoinput" and in "audioinput"

[Actual result]:
- Only one "device change!" message is displayed when connecting and when disconnecting the webcam.

[Regression range]:
- This is not a regression.

Comment 1

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Cannot repro the issue with my c920.
Could you help run below command and copy & paste the result?

ls -lR /dev/v4l/ ; ls -lR /dev/snd/

Please run it before and after you disconnect LifeCam HD-3000.

Comment 2

[Alexandru Simonca [:asimonca], Release Desktop QA]

You can see the terminal output below. As you can see '/dev/v41/ is not a valid folder on my OS. I have run the snippet in Terminal on Ubuntu 16.04 x64.


Before disconnect:

ls: cannot access '/dev/v41/': No such file or directory
/dev/snd/:
total 0
drwxr-xr-x  2 root root       60 ian 18 14:08 by-id
drwxr-xr-x  2 root root       80 ian 18 14:08 by-path
crw-rw----+ 1 root audio 116,  2 ian 18 13:59 controlC0
crw-rw----+ 1 root audio 116, 11 ian 18 14:08 controlC1
crw-rw----+ 1 root audio 116,  9 ian 18 13:59 hwC0D0
crw-rw----+ 1 root audio 116, 10 ian 18 13:59 hwC0D2
crw-rw----+ 1 root audio 116,  4 ian 18 12:00 pcmC0D0c
crw-rw----+ 1 root audio 116,  3 ian 18 14:06 pcmC0D0p
crw-rw----+ 1 root audio 116,  5 ian 18 13:59 pcmC0D2c
crw-rw----+ 1 root audio 116,  6 ian 18 14:00 pcmC0D3p
crw-rw----+ 1 root audio 116,  7 ian 18 14:00 pcmC0D7p
crw-rw----+ 1 root audio 116,  8 ian 18 12:00 pcmC0D8p
crw-rw----+ 1 root audio 116, 12 ian 18 14:08 pcmC1D0c
crw-rw----+ 1 root audio 116,  1 ian 18 13:59 seq
crw-rw----+ 1 root audio 116, 33 ian 18 13:59 timer

/dev/snd/by-id:
total 0
lrwxrwxrwx 1 root root 12 ian 18 14:08 usb-Microsoft_Microsoft®_LifeCam_HD-3000-02 -> ../controlC1

/dev/snd/by-path:
total 0
lrwxrwxrwx 1 root root 12 ian 18 14:08 pci-0000:00:14.0-usb-0:10:1.2 -> ../controlC1
lrwxrwxrwx 1 root root 12 ian 18 13:59 pci-0000:00:1f.3 -> ../controlC0


After disconnect:

ls: cannot access '/dev/v41/': No such file or directory
/dev/snd/:
total 0
drwxr-xr-x  2 root root       60 ian 18 14:11 by-path
crw-rw----+ 1 root audio 116,  2 ian 18 13:59 controlC0
crw-rw----+ 1 root audio 116,  9 ian 18 13:59 hwC0D0
crw-rw----+ 1 root audio 116, 10 ian 18 13:59 hwC0D2
crw-rw----+ 1 root audio 116,  4 ian 18 12:00 pcmC0D0c
crw-rw----+ 1 root audio 116,  3 ian 18 14:06 pcmC0D0p
crw-rw----+ 1 root audio 116,  5 ian 18 13:59 pcmC0D2c
crw-rw----+ 1 root audio 116,  6 ian 18 14:00 pcmC0D3p
crw-rw----+ 1 root audio 116,  7 ian 18 14:00 pcmC0D7p
crw-rw----+ 1 root audio 116,  8 ian 18 12:00 pcmC0D8p
crw-rw----+ 1 root audio 116,  1 ian 18 13:59 seq
crw-rw----+ 1 root audio 116, 33 ian 18 13:59 timer

/dev/snd/by-path:
total 0
lrwxrwxrwx 1 root root 12 ian 18 13:59 pci-0000:00:1f.3 -> ../controlC0

Comment 3

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

The path should be /dev/v4l/, not /dev/v41/

Comment 4

[Alexandru Simonca [:asimonca], Release Desktop QA]

Same result:

alexandru.simonca@p6301:~$ ls -lR /dev/v4l/ ; ls -lR /dev/snd/
ls: cannot access '/dev/v4l/': No such file or directory
/dev/snd/:
total 0
drwxr-xr-x  2 root root       60 ian 18  2017 by-path
crw-rw----+ 1 root audio 116,  2 ian 18  2017 controlC0
crw-rw----+ 1 root audio 116,  9 ian 18  2017 hwC0D0
crw-rw----+ 1 root audio 116, 10 ian 18  2017 hwC0D2
crw-rw----+ 1 root audio 116,  4 ian 18  2017 pcmC0D0c
crw-rw----+ 1 root audio 116,  3 ian 18 14:28 pcmC0D0p
crw-rw----+ 1 root audio 116,  5 ian 18  2017 pcmC0D2c
crw-rw----+ 1 root audio 116,  6 ian 18  2017 pcmC0D3p
crw-rw----+ 1 root audio 116,  7 ian 18  2017 pcmC0D7p
crw-rw----+ 1 root audio 116,  8 ian 18  2017 pcmC0D8p
crw-rw----+ 1 root audio 116,  1 ian 18  2017 seq
crw-rw----+ 1 root audio 116, 33 ian 18  2017 timer

/dev/snd/by-path:
total 0
lrwxrwxrwx 1 root root 12 ian 18  2017 pci-0000:00:1f.3 -> ../controlC0

Comment 5

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

It's weird. I borrowed an Ubuntu 16.04 x64 laptop and the path /dev/v4l exists.
Could you help provide the whole /dev tree dump? (ls -lR /dev/)
Thanks!

Comment 6

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

ls -R /dev/ would be better. Otherwise it will output many pages.

Comment 7

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Please dump this info with LifeCam HD-3000 connected.

Comment 8

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

For the desktop computer without built-in camera, there is no /dev/v4l in default. We need to take this into account while detecting connection.

Comment 9

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Attachment: Bug 1331648 - detect device connection for the case /dev/v4l hasn't existed;

Review commit: https://reviewboard.mozilla.org/r/107052/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/107052/

Comment 10

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

Comment on attachment 8830158 [details]
Bug 1331648 - detect device connection for the case /dev/v4l hasn't existed;

https://reviewboard.mozilla.org/r/107052/#review108380

::: media/webrtc/trunk/webrtc/modules/video_capture/linux/device_info_linux.cc:57
(Diff revision 1)
> -    switch (event->mask) {
> -        case IN_CREATE:
> +    if (event->mask & IN_CREATE) {
> +        if (fd == _fd_v4l || fd == _fd_snd) {
>              DeviceChange();
> -            break;
> +        } else if ((event->mask & IN_ISDIR) && (fd == _fd_dev)) {

Logic is wrong here; the else if (... IN_ISDIR) can't ever be true (unless it's a subset of IN_CREATE, which I doubt)

::: media/webrtc/trunk/webrtc/modules/video_capture/linux/device_info_linux.cc:80
(Diff revision 1)
> -            WEBRTC_TRACE(webrtc::kTraceError, webrtc::kTraceVideoCapture, _id,
> -                "UNKNOWN EVENT OCCURRED for file \"%s\" on WD #%i\n",
> -                cur_event_filename, cur_event_wd);
> +    if (event->mask & IN_DELETE) {
> +        if (fd == _fd_v4l || fd == _fd_snd) {
> +            DeviceChange();

Can we keep the log message on unknown events?  that will make merging with upstream simpler

::: media/webrtc/trunk/webrtc/modules/video_capture/linux/device_info_linux.cc:90
(Diff revision 1)
> +        } else if (fd == _fd_snd) {
> +            inotify_rm_watch(_fd_snd, _wd_snd);
> +            _wd_snd = -1;
> +        }

else I assume we don't care?  If so, fine, just checking if we want any sort of log or assertion.

Comment 11

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Comment on attachment 8830158 [details]
Bug 1331648 - detect device connection for the case /dev/v4l hasn't existed;

https://reviewboard.mozilla.org/r/107052/#review108380

> Logic is wrong here; the else if (... IN_ISDIR) can't ever be true (unless it's a subset of IN_CREATE, which I doubt)

The case (... IN_ISDIR ) is indeed true in this case.

https://www.kernel.org/pub/linux/kernel/people/rml/inotify/headers/inotify.h
Only the first 11 events are mutually exclusive.

#define IN_ACCESS		0x00000001	/* File was accessed */
#define IN_MODIFY		0x00000002	/* File was modified */
#define IN_ATTRIB		0x00000004	/* Metadata changed */
#define IN_CLOSE_WRITE		0x00000008	/* Writtable file was closed */
#define IN_CLOSE_NOWRITE	0x00000010	/* Unwrittable file closed */
#define IN_OPEN			0x00000020	/* File was opened */
#define IN_MOVED_FROM		0x00000040	/* File was moved from X */
#define IN_MOVED_TO		0x00000080	/* File was moved to Y */
#define IN_CREATE		0x00000100	/* Subfile was created */
#define IN_DELETE		0x00000200	/* Subfile was deleted */
#define IN_DELETE_SELF		0x00000400	/* Self was deleted */

/* special flags */
#define IN_ISDIR		0x40000000	/* event occurred against dir */
#define IN_ONESHOT		0x80000000	/* only send event once */

While connecting a camera, a subfolder /dev/v4l is created, we will get a mask 0x40000100

> Can we keep the log message on unknown events?  that will make merging with upstream simpler

Will do

> else I assume we don't care?  If so, fine, just checking if we want any sort of log or assertion.

Will do

Comment 12

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Comment on attachment 8830158 [details]
Bug 1331648 - detect device connection for the case /dev/v4l hasn't existed;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/107052/diff/1-2/

Comment 13

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7f59b7ea555c64051d7cfc3530bd053abe57dd46

Comment 14

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Looks like jesup is already reviewing this, so I'm good if he's good.

Comment 15

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

Comment on attachment 8830158 [details]
Bug 1331648 - detect device connection for the case /dev/v4l hasn't existed;

https://reviewboard.mozilla.org/r/107052/#review108572

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b32816fc8d58
detect device connection for the case /dev/v4l hasn't existed; r=jesup

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 18

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

Lifecam 3000 is a common camera.  It's late for an uplift to 53, though this has had a lot of bake time, and this is linux-only (which reduces the risk of fixing it, and also the reward for fixing it).

Given the patch, which isn't trivial and involves fd's, I'm going to wontfix this for 53 at this point.  We could have uplifted it to aurora 53 without problems, but not to late beta.