1297337 - Implement mediaDevices.ondevicechange for Linux

Status: RESOLVED FIXED

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

Comment 1

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

Attachment: Bug 1297337 - implement mediaDevices.ondevicechange for Linux;

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

Comment 2

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

Comment on attachment 8787103 [details]
Bug 1297337 - implement mediaDevices.ondevicechange for Linux;

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

Comment 3

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

Comment on attachment 8787103 [details]
Bug 1297337 - implement mediaDevices.ondevicechange for Linux;

https://reviewboard.mozilla.org/r/75974/#review74278

Cool, do you have a try run that shows it working? I notice this is cameras only. Do you have plans to cover microphones as well?

Lost of nits mostly. A bit unfamiliar with linux, so I'm asking jesup to take a look as well, especially the threading.

::: media/webrtc/trunk/webrtc/modules/video_capture/linux/device_info_linux.h:51
(Diff revision 2)
>      bool IsDeviceNameMatches(const char* name, const char* deviceUniqueIdUTF8);
> +
> +    void handle_event(inotify_event* event);
> +    int event_check();
> +    int handle_events();
> +    int process_inotify_events();
> +    rtc::scoped_ptr<ThreadWrapper> _inotifyEventThread;
> +    static bool InotifyEventThread(void*);
> +    bool InotifyProcess();
> +    int _fd, _wd_v4l, _wd_snd;

This file appears to use PascalCase and camelCase. Should we be consistent? Applies throughout.

::: media/webrtc/trunk/webrtc/modules/video_capture/linux/device_info_linux.h:60
(Diff revision 2)
> +    int handle_events();
> +    int process_inotify_events();
> +    rtc::scoped_ptr<ThreadWrapper> _inotifyEventThread;
> +    static bool InotifyEventThread(void*);
> +    bool InotifyProcess();
> +    int _fd, _wd_v4l, _wd_snd;

Please add a comment about which thread these are accessed on.

::: media/webrtc/trunk/webrtc/modules/video_capture/linux/device_info_linux.cc:52
(Diff revision 2)
> +    char* cur_event_filename = NULL;
> +    int cur_event_wd = event->wd;
> +    if (event->len) {
> +        cur_event_filename = event->name;
> +    }

This looks like dead code mostly. Can it be moved to the default: case statement where it is (rarely) used?

::: media/webrtc/trunk/webrtc/modules/video_capture/linux/device_info_linux.cc:59
(Diff revision 2)
> +    if (event->len) {
> +        cur_event_filename = event->name;
> +    }
> +
> +    switch (event->mask) {
> +        case IN_CREATE: // fall through

Will this need MOZ_FALLTHROUGH; to pass static analysis (or is webrtc code exempt)?

::: media/webrtc/trunk/webrtc/modules/video_capture/linux/device_info_linux.cc:61
(Diff revision 2)
> +    }
> +
> +    switch (event->mask) {
> +        case IN_CREATE: // fall through
> +        case IN_DELETE:
> +            DeviceChange();

Remind me, where is the DeviceChange() code again, and is it safe to call from this thread?

::: media/webrtc/trunk/webrtc/modules/video_capture/linux/device_info_linux.cc:79
(Diff revision 2)
> +    r = select(_fd+1, &rfds, NULL, NULL, NULL);
> +
> +    return r;

Nit: auto r appears unneeded. Why not return select(...)?

::: media/webrtc/trunk/webrtc/modules/video_capture/linux/device_info_linux.cc:88
(Diff revision 2)
> +    inotify_event* pevent;
> +    inotify_event* event;
> +    ssize_t r;
> +    size_t event_size;

Nit: I'm a fan of declaring vars close to where they're used, or even where they're used. Can these be moved down?

::: media/webrtc/trunk/webrtc/modules/video_capture/linux/device_info_linux.cc:94
(Diff revision 2)
> +    inotify_event* event;
> +    ssize_t r;
> +    size_t event_size;
> +    int count = 0;
> +
> +    r = read(_fd, buffer, 16384);

s/16384/sizeof(buffer)/ ?

::: media/webrtc/trunk/webrtc/modules/video_capture/linux/device_info_linux.cc:96
(Diff revision 2)
> +    if (r <= 0)
> +        return r;

Missing {}

Also, if read() returns 0, will process_inotify_events() stall?

::: media/webrtc/trunk/webrtc/modules/video_capture/linux/device_info_linux.cc:104
(Diff revision 2)
> +        event = static_cast<inotify_event*> (malloc(event_size));
> +        memmove(event, pevent, event_size);
> +
> +        handle_event(event);
> +        free(event);

(Sorry I'm unfamiliar with linux events...)

Why is malloc/free needed here? Alignment?

And if needed, why slower memmove over memcpy?

Also, do we need to check for buffer overrun here?

::: media/webrtc/trunk/webrtc/modules/video_capture/linux/device_info_linux.cc:119
(Diff revision 2)
> +    return count;
> +}
> +
> +int DeviceInfoLinux::process_inotify_events()
> +{
> +    while (1) {

while (true) ?

::: media/webrtc/trunk/webrtc/modules/video_capture/linux/device_info_linux.cc:120
(Diff revision 2)
> +        if(event_check() > 0) {
> +            if(handle_events() < 0) {

Nit: space between if and (

::: media/webrtc/trunk/webrtc/modules/video_capture/linux/device_info_linux.cc:134
(Diff revision 2)
> +bool DeviceInfoLinux::InotifyProcess()
> +{
> +    _fd = inotify_init();
> +    if (_fd >= 0) {
> +        _wd_v4l = inotify_add_watch (_fd, "/dev/v4l/by-path/", IN_CREATE | IN_DELETE);
> +        _wd_snd = inotify_add_watch (_fd, "/dev/snd/by-path/", IN_CREATE | IN_DELETE);
> +        process_inotify_events();
> +        return true;
> +    } else {
> +        return false;
> +    }
> +}

jesup can you look at this?

::: media/webrtc/trunk/webrtc/modules/video_capture/linux/device_info_linux.cc:150
(Diff revision 2)
> +    if (!_inotifyEventThread)
> +    {

Always true

::: media/webrtc/trunk/webrtc/modules/video_capture/linux/device_info_linux.cc:152
(Diff revision 2)
> +        _inotifyEventThread = ThreadWrapper::CreateThread(
> +            DeviceInfoLinux::InotifyEventThread, this, "InotifyEventThread");
> +        _inotifyEventThread->Start();
> +        _inotifyEventThread->SetPriority(kHighPriority);

How many threads will this create? One? One for each camera device? More?

Also, does this really need to be a high priority thread?

jesup, thoughts?

::: media/webrtc/trunk/webrtc/modules/video_capture/linux/device_info_linux.cc:153
(Diff revision 2)
>      : DeviceInfoImpl(id)
>  {
> +    if (!_inotifyEventThread)
> +    {
> +        _inotifyEventThread = ThreadWrapper::CreateThread(
> +            DeviceInfoLinux::InotifyEventThread, this, "InotifyEventThread");

Nit: Redundant scope :: ?

::: media/webrtc/trunk/webrtc/modules/video_capture/linux/device_info_linux.cc:166
(Diff revision 2)
>      return 0;
>  }
>  
>  DeviceInfoLinux::~DeviceInfoLinux()
>  {
> +    if (_inotifyEventThread) {

Always true

Comment 4

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

I'm seeing an awful lot of orange crashes for MDA on linux here. You may want to rebase and do another try if you think they're not caused by your code.

Comment 5

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

Comment on attachment 8787103 [details]
Bug 1297337 - implement mediaDevices.ondevicechange for Linux;

https://reviewboard.mozilla.org/r/75974/#review75358

::: media/webrtc/trunk/webrtc/modules/video_capture/linux/device_info_linux.cc:164
(Diff revision 2)
>  DeviceInfoLinux::~DeviceInfoLinux()
>  {
> +    if (_inotifyEventThread) {
> +        _inotifyEventThread->Stop();
> +        _inotifyEventThread.reset();
> +    }

Also, I'm concerned about stopping a thread in a destructor. We tend to prefer deterministic shutdown() methods, since depending on object ownership and reference counting, things could be owned by JS.

Please breakpoint to figure out when this is called. If it's called from GC/CC that could be a problem.

Comment 6

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

(In reply to Jan-Ivar Bruaroey [:jib] from comment #3)


> :::
> media/webrtc/trunk/webrtc/modules/video_capture/linux/device_info_linux.cc:59
> (Diff revision 2)
> > +    if (event->len) {
> > +        cur_event_filename = event->name;
> > +    }
> > +
> > +    switch (event->mask) {
> > +        case IN_CREATE: // fall through
> 
> Will this need MOZ_FALLTHROUGH; to pass static analysis (or is webrtc code
> exempt)?

It is, or in any case we don't add them to imported code normally.


> :::
> media/webrtc/trunk/webrtc/modules/video_capture/linux/device_info_linux.cc:88
> (Diff revision 2)
> > +    inotify_event* pevent;
> > +    inotify_event* event;
> > +    ssize_t r;
> > +    size_t event_size;
> 
> Nit: I'm a fan of declaring vars close to where they're used, or even where
> they're used. Can these be moved down?

I agree; var-definitions-first was required in K&R C; we don't need to do that anymore.

> :::
> media/webrtc/trunk/webrtc/modules/video_capture/linux/device_info_linux.cc:94
> (Diff revision 2)
> > +    inotify_event* event;
> > +    ssize_t r;
> > +    size_t event_size;
> > +    int count = 0;
> > +
> > +    r = read(_fd, buffer, 16384);
> 
> s/16384/sizeof(buffer)/ ?


absolutely; these sorts of things are landmines if you use a hard constant.

> media/webrtc/trunk/webrtc/modules/video_capture/linux/device_info_linux.cc:
> 104
> (Diff revision 2)
> > +        event = static_cast<inotify_event*> (malloc(event_size));
> > +        memmove(event, pevent, event_size);
> > +
> > +        handle_event(event);
> > +        free(event);
> 
> (Sorry I'm unfamiliar with linux events...)
> 
> Why is malloc/free needed here? Alignment?
> 
> And if needed, why slower memmove over memcpy?
> 
> Also, do we need to check for buffer overrun here?

malloc/free for each event is unneeded:

char event_buffer[sizeof(inotify_event) + FILENAME_MAX+1] // null-terminated
  __attribute__ ((aligned(__alignof__(struct inotify_event))));

...
memcpy(event_buffer, pevent, event_size);
handle_event(static_cast<inotify_event>(event_buffer));


> :::
> media/webrtc/trunk/webrtc/modules/video_capture/linux/device_info_linux.cc:
> 134
> (Diff revision 2)
> > +bool DeviceInfoLinux::InotifyProcess()
> > +{
> > +    _fd = inotify_init();
> > +    if (_fd >= 0) {
> > +        _wd_v4l = inotify_add_watch (_fd, "/dev/v4l/by-path/", IN_CREATE | IN_DELETE);
> > +        _wd_snd = inotify_add_watch (_fd, "/dev/snd/by-path/", IN_CREATE | IN_DELETE);
> > +        process_inotify_events();
> > +        return true;
> > +    } else {
> > +        return false;
> > +    }
> > +}
> 
> jesup can you look at this?

I don't think we have to worry about CLOEXEC (fork() + execve()), so we can use inotify_init() (instead of init1().

You need to check the result of add_watch against -1, though if it fails there's probably nothing special to do, so you could probably ignore add_watch failures (the watch descriptors will be -1).  This also avoids a fair amount of cleanup/etc code.

What is "by-path"????  Shouldn't this just be /dev/v4l and /dev/snd?

> :::
> media/webrtc/trunk/webrtc/modules/video_capture/linux/device_info_linux.cc:
> 152
> (Diff revision 2)
> > +        _inotifyEventThread = ThreadWrapper::CreateThread(
> > +            DeviceInfoLinux::InotifyEventThread, this, "InotifyEventThread");
> > +        _inotifyEventThread->Start();
> > +        _inotifyEventThread->SetPriority(kHighPriority);
> 
> How many threads will this create? One? One for each camera device? More?
> 
> Also, does this really need to be a high priority thread?
> 
> jesup, thoughts?

If this is supposed to be a singleton, it should assert that is is.  Probably DeviceInfoLinux isn't a singleton, since two tabs could be capturing audio - video all goes through CamerasParent in the master process, and so is unified there - but if two cameras are in use, perhaps you have two of these.

In that case, you can have a thread-per-use (not *so* bad, but not cheap), or you can have a shared singleton device-change-notification service.  Note however in e10s that CamerasParent runs in the Master (Chrome) process, and audio capture runs in the Content process.  So either there should be a single audio/video notification service in the Chrome process (which would be painful to integrate with the webrtc.org code, and would have to use IPC to notify Content of audio changes) or the type of device (or more likely path) it watches should be passed in (v4l or snd), so the Master can watch for v4l changes, and the Content can watch for audio changes.

This would give us 1 Video inotify thread (Master), and one (in Content) per tab capturing the microphone (which is typically 1, so this isn't so bad, and is pretty close to the minimum in practice since using the single inotify in Master is a ton of extra code).  I'm not sure it's worth the complexity/code/time to have a true singleton audio inotify service, since it would rarely be used and just saves a minor amount of memory/CPU in a rare case.

It probably doesn't need to be high priority, though it shouldn't hurt either.

Comment 7

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

(In reply to Jan-Ivar Bruaroey [:jib] from comment #5)
> (Diff revision 2)
> >  DeviceInfoLinux::~DeviceInfoLinux()
> >  {
> > +    if (_inotifyEventThread) {
> > +        _inotifyEventThread->Stop();
> > +        _inotifyEventThread.reset();
> > +    }
> 
> Also, I'm concerned about stopping a thread in a destructor. We tend to
> prefer deterministic shutdown() methods, since depending on object ownership
> and reference counting, things could be owned by JS.
> 
> Please breakpoint to figure out when this is called. If it's called from
> GC/CC that could be a problem.

This should be called when the webrtc code (or the gum capture/CamerasParent) is shut down.  We shut down quite a few webrtc threads in this case already, so this should be ok.

Thinking more about it....

Audio capture in non-full-duplex is via cubeb, not via webrtc.org code.  Right now non-full-duplex is supported (and required for Android, and I think winmm).  We hope to remove non-full-duplex eventually, but it will be rare on everything but android very soon, and we could live without device-change notification in those cases.

So maybe we only need this for video...

Comment 8

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

Comment on attachment 8787103 [details]
Bug 1297337 - implement mediaDevices.ondevicechange for Linux;

https://reviewboard.mozilla.org/r/75974/#review74278

Yes, I have verified this patch with my Logitec webcam and headset.
The patch works for both camera and microphone. 
/dev/v4l/by-path/ -> camera
/dev/snd/by-path/ -> microphone

> Remind me, where is the DeviceChange() code again, and is it safe to call from this thread?

https://dxr.mozilla.org/mozilla-central/source/media/webrtc/trunk/webrtc/modules/video_capture/include/video_capture.h#43
It's safe. In Mac design, we also call DeviceChange() in a dedicated thread detecting hardware plug-in / plug-out.

> Missing {}
> 
> Also, if read() returns 0, will process_inotify_events() stall?

1. {} is added
2. if read() returns 0, ProcessInotifyEvents will stall at EventCheck() to wait for the next event as expected.

> (Sorry I'm unfamiliar with linux events...)
> 
> Why is malloc/free needed here? Alignment?
> 
> And if needed, why slower memmove over memcpy?
> 
> Also, do we need to check for buffer overrun here?

> malloc/free for each event is unneeded:
> char event_buffer[sizeof(inotify_event) + FILENAME_MAX+1] // null-terminated
>   __attribute__ ((aligned(__alignof__(struct inotify_event))));
> ...
> memcpy(event_buffer, pevent, event_size);
> handle_event(static_cast<inotify_event>(event_buffer));

Fixed.

> jesup can you look at this?

> I don't think we have to worry about CLOEXEC (fork() + execve()), so we can use inotify_init()
> (instead of init1().
>
> You need to check the result of add_watch against -1, though if it fails there's probably nothing
> special to do, so you could probably ignore add_watch failures (the watch descriptors will be -1).
> This also avoids a fair amount of cleanup/etc code.
>
> What is "by-path"????  Shouldn't this just be /dev/v4l and /dev/snd?

https://wiki.archlinux.org/index.php/persistent_block_device_naming#by-id_and_by-path
Since Inotify is not recursive, if we monitor /dev/v4l, we won't get the event for the device node creation / removal in the subdirectory.

> How many threads will this create? One? One for each camera device? More?
> 
> Also, does this really need to be a high priority thread?
> 
> jesup, thoughts?

\> We hope to remove non-full-duplex eventually, but it will be rare on everything
\> If this is supposed to be a singleton, it should assert that is is.  Probably
\> DeviceInfoLinux isn't a singleton, since two tabs could be capturing audio -
\> video all goes through CamerasParent in the master process, and so is unified
\> there - but if two cameras are in use, perhaps you have two of these.
\> In that case, you can have a thread-per-use (not \*so\* bad, but not cheap), or
\> you can have a shared singleton device-change-notification service.  Note however
\> in e10s that CamerasParent runs in the Master (Chrome) process, and audio capture
\> runs in the Content process.  So either there should be a single audio/video
\> notification service in the Chrome process (which would be painful to integrate
\> with the webrtc.org code, and would have to use IPC to notify Content of audio
\> changes) or the type of device (or more likely path) it watches should be passed
\> in (v4l or snd), so the Master can watch for v4l changes, and the Content can watch
\> for audio changes. This would give us 1 Video inotify thread (Master), and one (in
\> Content) per tab capturing the microphone (which is typically 1, so this isn't so
\> bad, and is pretty close to the minimum in practice since using the single inotify
\> in Master is a ton of extra code).  I'm not sure it's worth the complexity/code/time
\> to have a true singleton audio inotify service, since it would rarely be used and
\> just saves a minor amount of memory/CPU in a rare case
\> It probably doesn't need to be high priority, though it shouldn't hurt either.

1. ViEInputManager only creates DeviceInfoLinux once no matter how many tabs are capturing.
So only one thread will be created.

2. I will change it to kNormalPriority.

Comment 9

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

Comment on attachment 8787103 [details]
Bug 1297337 - implement mediaDevices.ondevicechange for Linux;

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

Comment 10

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

(In reply to Munro Mengjue Chiang [:mchiang] from comment #8)

> > malloc/free for each event is unneeded:
> > char event_buffer[sizeof(inotify_event) + FILENAME_MAX+1] // null-terminated
> >   __attribute__ ((aligned(__alignof__(struct inotify_event))));
> > ...
> > memcpy(event_buffer, pevent, event_size);
> > handle_event(static_cast<inotify_event>(event_buffer));
> 
> Fixed.

Thanks (though likely that should be static_cast<inotify_event*>(event_buffer), but the compiler likely caught that.

> 1. ViEInputManager only creates DeviceInfoLinux once no matter how many tabs
> are capturing.
> So only one thread will be created.

Ok.  Did you verify that in e10s mode, and e10s + setting media.navigator.audio.full_duplex to false?
 
> 2. I will change it to kNormalPriority.

thanks

Comment 11

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

(In reply to Randell Jesup [:jesup] from comment #10)
> (In reply to Munro Mengjue Chiang [:mchiang] from comment #8)
> 
> > > malloc/free for each event is unneeded:
> > > char event_buffer[sizeof(inotify_event) + FILENAME_MAX+1] // null-terminated
> > >   __attribute__ ((aligned(__alignof__(struct inotify_event))));
> > > ...
> > > memcpy(event_buffer, pevent, event_size);
> > > handle_event(static_cast<inotify_event>(event_buffer));
> > 
> > Fixed.
> 
> Thanks (though likely that should be
> static_cast<inotify_event*>(event_buffer), but the compiler likely caught
> that.
> 
> > 1. ViEInputManager only creates DeviceInfoLinux once no matter how many tabs
> > are capturing.
> > So only one thread will be created.
> 
> Ok.  Did you verify that in e10s mode, and e10s + setting
> media.navigator.audio.full_duplex to false?
Yes, I verified this in all 3 modes (e10s off / e10s on / e10s + setting full_duplex to false)

>  
> > 2. I will change it to kNormalPriority.
> 
> thanks

Comment 12

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=165666030ada

Comment 13

[Paul Adenot (:padenot)]

Clearing NI

Comment 14

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

Comment on attachment 8787103 [details]
Bug 1297337 - implement mediaDevices.ondevicechange for Linux;

https://reviewboard.mozilla.org/r/75974/#review76478

::: media/webrtc/trunk/webrtc/modules/video_capture/linux/device_info_linux.h:61
(Diff revisions 2 - 3)
> -    int process_inotify_events();
> +    int ProcessInotifyEvents();
>      rtc::scoped_ptr<ThreadWrapper> _inotifyEventThread;
>      static bool InotifyEventThread(void*);
>      bool InotifyProcess();
> -    int _fd, _wd_v4l, _wd_snd;
> +    int _fd, _wd_v4l, _wd_snd; /* accessed on InotifyEventThread thread */
> +    bool _isShutdown;

This is accessed from multiple threads.  It needs to be an atomic (or you could lock, but atomic makes more sense).  Note: don't use mozilla/Atomics.h; use the atomic support in the webrtc.org codebase (media/webrtc/trunk/webrtc/*)

::: media/webrtc/trunk/webrtc/modules/video_capture/linux/device_info_linux.cc:106
(Diff revisions 2 - 3)
>          pevent = (inotify_event *) (&buffer[buffer_i]);
> -        event_size = sizeof(inotify_event) + pevent->len;
> +        eventSize = sizeof(inotify_event) + pevent->len;

In theory on an architecture with alignment restrictions this could fail; for such architectures you must copy the inotify_event first, read the len, then copy the name.  We don't officially support any devices with that restriction anymore that I know of (perhaps some MIPS port might be affected), so this is probably ok.  Add a comment that this is an unaligned read of len.

::: media/webrtc/trunk/webrtc/modules/video_capture/linux/device_info_linux.cc:107
(Diff revisions 2 - 3)
> -        event_size = sizeof(inotify_event) + pevent->len;
> -        event = static_cast<inotify_event*> (malloc(event_size));
> -        memmove(event, pevent, event_size);
> +        eventSize = sizeof(inotify_event) + pevent->len;
> +        char event[eventSize];
> +
> +        memcpy(event, pevent, eventSize);
>  
> -        handle_event(event);
> +        HandleEvent((inotify_event*)(event));

This isn't cononically valid; see my comment; you need to add __attribute__ ((aligned(__alignof__(struct inotify_event))) if you want to do this.

::: media/webrtc/trunk/webrtc/modules/video_capture/linux/device_info_linux.cc:161
(Diff revisions 2 - 3)
> -            DeviceInfoLinux::InotifyEventThread, this, "InotifyEventThread");
> +        InotifyEventThread, this, "InotifyEventThread");
> +
> +    if (_inotifyEventThread)
> +    {
>          _inotifyEventThread->Start();
> -        _inotifyEventThread->SetPriority(kHighPriority);
> +        _inotifyEventThread->SetPriority(kNormalPriority);

I don't think this is needed

::: media/webrtc/trunk/webrtc/modules/video_capture/linux/device_info_linux.cc:179
(Diff revisions 2 - 3)
>      if (_wd_v4l >= 0) {
>        inotify_rm_watch(_fd, _wd_v4l);
>      }

You remove the watch on v4l but not snd

Comment 15

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

Comment on attachment 8787103 [details]
Bug 1297337 - implement mediaDevices.ondevicechange for Linux;

https://reviewboard.mozilla.org/r/75974/#review76478

> You remove the watch on v4l but not snd

I remove it at line 184.

Comment 16

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

Comment on attachment 8787103 [details]
Bug 1297337 - implement mediaDevices.ondevicechange for Linux;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/75974/diff/3-4/

Comment 17

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

Comment on attachment 8787103 [details]
Bug 1297337 - implement mediaDevices.ondevicechange for Linux;

https://reviewboard.mozilla.org/r/75974/#review78096

::: media/webrtc/trunk/webrtc/modules/video_capture/linux/device_info_linux.cc:89
(Diff revisions 3 - 4)
>  int DeviceInfoLinux::HandleEvents()
>  {
>      char buffer[BUF_LEN];
>  
>      ssize_t r = read(_fd, buffer, BUF_LEN);

Here you access _fd on one thread...

::: media/webrtc/trunk/webrtc/modules/video_capture/linux/device_info_linux.cc:180
(Diff revisions 3 - 4)
>      if (_wd_v4l >= 0) {
>        inotify_rm_watch(_fd, _wd_v4l);
>      }
>  
>      if (_wd_snd >= 0) {
>        inotify_rm_watch(_fd, _wd_snd);
>      }
>  
>      if (_fd >= 0) {
>        close(_fd);
>      }

And here you access it again on another.  While _isShutdown access is now safe, closing the fd while reading from the fd on another thread is very likely dangerous.  It's unclear to me if inotify_rm_watch() is cross-thread safe, but when in doubt about a system call I'd assume not.

I'm afraid you'll need a lock here.

Comment 18

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

Comment on attachment 8787103 [details]
Bug 1297337 - implement mediaDevices.ondevicechange for Linux;

https://reviewboard.mozilla.org/r/75974/#review78096

> And here you access it again on another.  While _isShutdown access is now safe, closing the fd while reading from the fd on another thread is very likely dangerous.  It's unclear to me if inotify_rm_watch() is cross-thread safe, but when in doubt about a system call I'd assume not.
> 
> I'm afraid you'll need a lock here.

Sorry I didn't notice that when I recommended using an Atomic; I could see the thread-safety issue, but didn't see what code this was triggering

Comment 19

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

Comment on attachment 8787103 [details]
Bug 1297337 - implement mediaDevices.ondevicechange for Linux;

https://reviewboard.mozilla.org/r/75974/#review78096

> Sorry I didn't notice that when I recommended using an Atomic; I could see the thread-safety issue, but didn't see what code this was triggering

I will move inotify_rm_watch and close(_fd) to the same thread which creates _fd, _wd_v4l, and _wd_snd.
Then we don't need to use lock.

Comment 20

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

Comment on attachment 8787103 [details]
Bug 1297337 - implement mediaDevices.ondevicechange for Linux;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/75974/diff/4-5/

Comment 21

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

Comment on attachment 8787103 [details]
Bug 1297337 - implement mediaDevices.ondevicechange for Linux;

https://reviewboard.mozilla.org/r/75974/#review78322

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/18762ffa1e92
implement mediaDevices.ondevicechange for Linux; r=jesup

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/18762ffa1e92

Comment 24

[Byron Campen [:bwc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8bfcfca63f55

Comment 25

[Andreas Pehrson [:pehrsons]]

I feel pretty confident when I say that the cause of all these intermittents is the deadlock I analysed in bug 1304270 comment 9. Thus, this bug is not the root cause but it triggered it because we now run the code that landed with bug 1286429 on linux. The only platform where we have full-stack MediaEngines - which is what is deadlocking.

Comment 26

[Eric Shepherd [:sheppy]]

Tracking all the pieces of this since it's being implemented on different platforms on different schedules, plus the pref. All of that will affect the developer docs and release notes in various ways.

Comment 27

[Eric Shepherd [:sheppy]]

Added info about Linux compat arriving in Firefox 52 to the following pages:

https://developer.mozilla.org/en-US/docs/Web/Events/devicechange
https://developer.mozilla.org/en-US/docs/Web/API/MediaDevices/ondevicechange
https://developer.mozilla.org/en-US/docs/Web/API/MediaDevices
https://developer.mozilla.org/en-US/Firefox/Experimental_features
https://developer.mozilla.org/en-US/Firefox/Releases/52

Comment 28

[vitor.graziano]

I'm trying mediadevices.ondevicechange on Linux(Mint cinnanon 17.3 64 bit, kernel 3.19.0-32-generic), firefox 57.0.1 64 bit and i'm having a bad working. The problem is: the event is fired every time a mediadevices.getUserMedia is called, and when this occurs the event is fired for a long time(about 1 minute or more) every 2~5 seconds.
When i plug or unplug a media device, the event is also fired just once, so in this case things are ok.

Comment 29

[Andreas Pehrson [:pehrsons]]

Hi Vitor,

Can you just double check for me that the pref "media.ondevicechange.fakeDeviceChangeEvent.enabled" in about:config is set to `false`?

If it is `false` and you're still hitting this, could you file a new bug in "Core::WebRTC: Audio/Video" please? Preferably with a minified test case. Thanks!

Comment 30

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

Please refer to bug 1426354.

Comment 31

[vitor.graziano]

(In reply to Andreas Pehrson [:pehrsons] from comment #29)
> Hi Vitor,
> 
> Can you just double check for me that the pref
> "media.ondevicechange.fakeDeviceChangeEvent.enabled" in about:config is set
> to `false`?
> 
> If it is `false` and you're still hitting this, could you file a new bug in
> "Core::WebRTC: Audio/Video" please? Preferably with a minified test case.
> Thanks!

Hi Andreas,
you got it, media.ondevicechange.fakeDeviceChangeEvent.enabled was set to tue, i never can't imagine the problem was that.
I also opened a bug(1426354), i'm going to update it. Many thanks!